Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enum size lint #14300

Merged
merged 4 commits into from
May 26, 2014
Merged

enum size lint #14300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3f8cc16
rustc: middle: lint: use more doc comments
emberian May 19, 2014
f122ad0
rustc: middle: ty: use doc comments for the tcx
emberian May 19, 2014
c327080
rustc: add a lint for large enum variants
emberian May 19, 2014
d8467e2
rustc: abstract lint level exporting from EnumSizeVariance
emberian May 20, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
150 changes: 96 additions & 54 deletions src/librustc/middle/lint.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ use syntax::parse::token;
use syntax::visit::Visitor;
use syntax::{ast, ast_util, visit};

#[deriving(Clone, Eq, Ord, TotalEq, TotalOrd)]
#[deriving(Clone, Show, Eq, Ord, TotalEq, TotalOrd, Hash)]
pub enum Lint {
CTypes,
UnusedImports,
Expand All @@ -93,6 +93,7 @@ pub enum Lint {
UnknownFeatures,
UnknownCrateType,
UnsignedNegate,
VariantSizeDifference,

ManagedHeapMemory,
OwnedHeapMemory,
Expand Down Expand Up @@ -146,8 +147,9 @@ pub struct LintSpec {

pub type LintDict = HashMap<&'static str, LintSpec>;

// this is public for the lints that run in trans
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could define a struct like

pub struct LintState {
   lvl: level,
   src: LintSource
}

and then have the emit_lint function being a public .emit method. The node_levels map would then store LintStates, so that trans doesn't need to know about the internals of lint.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is desirable. Knowing the level is very much desired, so you can avoid unnecesary work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, lvl could just be public, but I don't feel like what it has now is that bad?

#[deriving(Eq)]
enum LintSource {
pub enum LintSource {
Node(Span),
Default,
CommandLine
Expand Down Expand Up @@ -399,6 +401,13 @@ static lint_table: &'static [(&'static str, LintSpec)] = &[
default: Warn
}),

("variant_size_difference",
LintSpec {
lint: VariantSizeDifference,
desc: "detects enums with widely varying variant sizes",
default: Allow,
}),

("unused_must_use",
LintSpec {
lint: UnusedMustUse,
Expand Down Expand Up @@ -437,30 +446,78 @@ pub fn get_lint_dict() -> LintDict {
}

struct Context<'a> {
// All known lint modes (string versions)
/// All known lint modes (string versions)
dict: LintDict,
// Current levels of each lint warning
/// Current levels of each lint warning
cur: SmallIntMap<(Level, LintSource)>,
// context we're checking in (used to access fields like sess)
/// Context we're checking in (used to access fields like sess)
tcx: &'a ty::ctxt,
// Items exported by the crate; used by the missing_doc lint.
/// Items exported by the crate; used by the missing_doc lint.
exported_items: &'a privacy::ExportedItems,
// The id of the current `ast::StructDef` being walked.
/// The id of the current `ast::StructDef` being walked.
cur_struct_def_id: ast::NodeId,
// Whether some ancestor of the current node was marked
// #[doc(hidden)].
/// Whether some ancestor of the current node was marked
/// #[doc(hidden)].
is_doc_hidden: bool,

// When recursing into an attributed node of the ast which modifies lint
// levels, this stack keeps track of the previous lint levels of whatever
// was modified.
/// When recursing into an attributed node of the ast which modifies lint
/// levels, this stack keeps track of the previous lint levels of whatever
/// was modified.
lint_stack: Vec<(Lint, Level, LintSource)>,

// id of the last visited negated expression
/// Id of the last visited negated expression
negated_expr_id: ast::NodeId,

// ids of structs/enums which have been checked for raw_pointer_deriving
/// Ids of structs/enums which have been checked for raw_pointer_deriving
checked_raw_pointers: NodeSet,

/// Level of lints for certain NodeIds, stored here because the body of
/// the lint needs to run in trans.
node_levels: HashMap<(ast::NodeId, Lint), (Level, LintSource)>,
}

pub fn emit_lint(level: Level, src: LintSource, msg: &str, span: Span,
lint_str: &str, tcx: &ty::ctxt) {
if level == Allow { return }

let mut note = None;
let msg = match src {
Default => {
format!("{}, \\#[{}({})] on by default", msg,
level_to_str(level), lint_str)
},
CommandLine => {
format!("{} [-{} {}]", msg,
match level {
Warn => 'W', Deny => 'D', Forbid => 'F',
Allow => fail!()
}, lint_str.replace("_", "-"))
},
Node(src) => {
note = Some(src);
msg.to_str()
}
};

match level {
Warn => { tcx.sess.span_warn(span, msg.as_slice()); }
Deny | Forbid => { tcx.sess.span_err(span, msg.as_slice()); }
Allow => fail!(),
}

for &span in note.iter() {
tcx.sess.span_note(span, "lint level defined here");
}
}

pub fn lint_to_str(lint: Lint) -> &'static str {
for &(name, lspec) in lint_table.iter() {
if lspec.lint == lint {
return name;
}
}

fail!("unrecognized lint: {}", lint);
}

impl<'a> Context<'a> {
Expand Down Expand Up @@ -492,7 +549,7 @@ impl<'a> Context<'a> {
return *k;
}
}
fail!("unregistered lint {:?}", lint);
fail!("unregistered lint {}", lint);
}

fn span_lint(&self, lint: Lint, span: Span, msg: &str) {
Expand All @@ -501,37 +558,8 @@ impl<'a> Context<'a> {
Some(&(Warn, src)) => (self.get_level(Warnings), src),
Some(&pair) => pair,
};
if level == Allow { return }

let mut note = None;
let msg = match src {
Default => {
format_strbuf!("{}, \\#[{}({})] on by default",
msg,
level_to_str(level),
self.lint_to_str(lint))
},
CommandLine => {
format!("{} [-{} {}]", msg,
match level {
Warn => 'W', Deny => 'D', Forbid => 'F',
Allow => fail!()
}, self.lint_to_str(lint).replace("_", "-"))
},
Node(src) => {
note = Some(src);
msg.to_str()
}
};
match level {
Warn => self.tcx.sess.span_warn(span, msg.as_slice()),
Deny | Forbid => self.tcx.sess.span_err(span, msg.as_slice()),
Allow => fail!(),
}

for &span in note.iter() {
self.tcx.sess.span_note(span, "lint level defined here");
}
emit_lint(level, src, msg, span, self.lint_to_str(lint), self.tcx);
}

/**
Expand Down Expand Up @@ -610,8 +638,8 @@ impl<'a> Context<'a> {
}
}

// Check that every lint from the list of attributes satisfies `f`.
// Return true if that's the case. Otherwise return false.
/// Check that every lint from the list of attributes satisfies `f`.
/// Return true if that's the case. Otherwise return false.
pub fn each_lint(sess: &session::Session,
attrs: &[ast::Attribute],
f: |@ast::MetaItem, Level, InternedString| -> bool)
Expand Down Expand Up @@ -645,8 +673,8 @@ pub fn each_lint(sess: &session::Session,
true
}

// Check from a list of attributes if it contains the appropriate
// `#[level(lintname)]` attribute (e.g. `#[allow(dead_code)]).
/// Check from a list of attributes if it contains the appropriate
/// `#[level(lintname)]` attribute (e.g. `#[allow(dead_code)]).
pub fn contains_lint(attrs: &[ast::Attribute],
level: Level,
lintname: &'static str)
Expand Down Expand Up @@ -1685,9 +1713,24 @@ fn check_stability(cx: &Context, e: &ast::Expr) {
cx.span_lint(lint, e.span, msg.as_slice());
}

fn check_enum_variant_sizes(cx: &mut Context, it: &ast::Item) {
match it.node {
ast::ItemEnum(..) => {
match cx.cur.find(&(VariantSizeDifference as uint)) {
Some(&(lvl, src)) if lvl != Allow => {
cx.node_levels.insert((it.id, VariantSizeDifference), (lvl, src));
},
_ => { }
}
},
_ => { }
}
}

impl<'a> Visitor<()> for Context<'a> {
fn visit_item(&mut self, it: &ast::Item, _: ()) {
self.with_lint_attrs(it.attrs.as_slice(), |cx| {
check_enum_variant_sizes(cx, it);
check_item_ctypes(cx, it);
check_item_non_camel_case_types(cx, it);
check_item_non_uppercase_statics(cx, it);
Expand Down Expand Up @@ -1878,6 +1921,7 @@ pub fn check_crate(tcx: &ty::ctxt,
lint_stack: Vec::new(),
negated_expr_id: -1,
checked_raw_pointers: NodeSet::new(),
node_levels: HashMap::new(),
};

// Install default lint levels, followed by the command line levels, and
Expand Down Expand Up @@ -1913,13 +1957,11 @@ pub fn check_crate(tcx: &ty::ctxt,
// in the iteration code.
for (id, v) in tcx.sess.lints.borrow().iter() {
for &(lint, span, ref msg) in v.iter() {
tcx.sess.span_bug(span,
format!("unprocessed lint {:?} at {}: {}",
lint,
tcx.map.node_to_str(*id),
*msg).as_slice())
tcx.sess.span_bug(span, format!("unprocessed lint {} at {}: {}",
lint, tcx.map.node_to_str(*id), *msg).as_slice())
}
}

tcx.sess.abort_if_errors();
*tcx.node_lint_levels.borrow_mut() = cx.node_levels;
}
58 changes: 55 additions & 3 deletions src/librustc/middle/trans/base.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ use lib::llvm::{ModuleRef, ValueRef, BasicBlockRef};
use lib::llvm::{llvm, Vector};
use lib;
use metadata::{csearch, encoder};
use middle::lint;
use middle::astencode;
use middle::lang_items::{LangItem, ExchangeMallocFnLangItem, StartFnLangItem};
use middle::weak_lang_items;
Expand All @@ -57,7 +58,7 @@ use middle::trans::foreign;
use middle::trans::glue;
use middle::trans::inline;
use middle::trans::machine;
use middle::trans::machine::{llalign_of_min, llsize_of};
use middle::trans::machine::{llalign_of_min, llsize_of, llsize_of_real};
use middle::trans::meth;
use middle::trans::monomorphize;
use middle::trans::tvec;
Expand Down Expand Up @@ -1539,7 +1540,7 @@ fn trans_enum_variant_or_tuple_like_struct(ccx: &CrateContext,
}

fn trans_enum_def(ccx: &CrateContext, enum_definition: &ast::EnumDef,
id: ast::NodeId, vi: &[Rc<ty::VariantInfo>],
sp: Span, id: ast::NodeId, vi: &[Rc<ty::VariantInfo>],
i: &mut uint) {
for &variant in enum_definition.variants.iter() {
let disr_val = vi[*i].disr_val;
Expand All @@ -1559,6 +1560,57 @@ fn trans_enum_def(ccx: &CrateContext, enum_definition: &ast::EnumDef,
}
}
}

enum_variant_size_lint(ccx, enum_definition, sp, id);
}

fn enum_variant_size_lint(ccx: &CrateContext, enum_def: &ast::EnumDef, sp: Span, id: ast::NodeId) {
let mut sizes = Vec::new(); // does no allocation if no pushes, thankfully

let (lvl, src) = ccx.tcx.node_lint_levels.borrow()
.find(&(id, lint::VariantSizeDifference))
.map_or((lint::Allow, lint::Default), |&(lvl,src)| (lvl, src));

if lvl != lint::Allow {
let avar = adt::represent_type(ccx, ty::node_id_to_type(ccx.tcx(), id));
match *avar {
adt::General(_, ref variants) => {
for var in variants.iter() {
let mut size = 0;
for field in var.fields.iter().skip(1) {
// skip the dicriminant
size += llsize_of_real(ccx, sizing_type_of(ccx, *field));
}
sizes.push(size);
}
},
_ => { /* its size is either constant or unimportant */ }
}

let (largest, slargest, largest_index) = sizes.iter().enumerate().fold((0, 0, 0),
|(l, s, li), (idx, &size)|
if size > l {
(size, l, idx)
} else if size > s {
(l, size, li)
} else {
(l, s, li)
}
);

// we only warn if the largest variant is at least thrice as large as
// the second-largest.
if largest > slargest * 3 && slargest > 0 {
lint::emit_lint(lvl, src,
format!("enum variant is more than three times larger \
({} bytes) than the next largest (ignoring padding)",
largest).as_slice(),
sp, lint::lint_to_str(lint::VariantSizeDifference), ccx.tcx());

ccx.sess().span_note(enum_def.variants.get(largest_index).span,
"this variant is the largest");
}
}
}

pub struct TransItemVisitor<'a> {
Expand Down Expand Up @@ -1605,7 +1657,7 @@ pub fn trans_item(ccx: &CrateContext, item: &ast::Item) {
if !generics.is_type_parameterized() {
let vi = ty::enum_variants(ccx.tcx(), local_def(item.id));
let mut i = 0;
trans_enum_def(ccx, enum_definition, item.id, vi.as_slice(), &mut i);
trans_enum_def(ccx, enum_definition, item.span, item.id, vi.as_slice(), &mut i);
}
}
ast::ItemStatic(_, m, expr) => {
Expand Down
Loading