From 3f8cc1683bc08d15986c617bced79a1f3eb197d2 Mon Sep 17 00:00:00 2001 From: Corey Richardson Date: Sun, 18 May 2014 21:43:18 -0700 Subject: [PATCH 1/4] rustc: middle: lint: use more doc comments --- src/librustc/middle/lint.rs | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/librustc/middle/lint.rs b/src/librustc/middle/lint.rs index 8feacba6e006d..88ded969ad5d1 100644 --- a/src/librustc/middle/lint.rs +++ b/src/librustc/middle/lint.rs @@ -437,29 +437,29 @@ 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, } @@ -610,8 +610,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) @@ -645,8 +645,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) From f122ad08a57d3e250f0c8c5036327eaa320dd4eb Mon Sep 17 00:00:00 2001 From: Corey Richardson Date: Mon, 19 May 2014 10:33:26 -0700 Subject: [PATCH 2/4] rustc: middle: ty: use doc comments for the tcx --- src/librustc/middle/ty.rs | 82 +++++++++++++++++++-------------------- 1 file changed, 41 insertions(+), 41 deletions(-) diff --git a/src/librustc/middle/ty.rs b/src/librustc/middle/ty.rs index 64619b636a33a..62679fa222bf6 100644 --- a/src/librustc/middle/ty.rs +++ b/src/librustc/middle/ty.rs @@ -237,8 +237,8 @@ pub enum AutoRef { /// generates so that so that it can be reused and doesn't have to be redone /// later on. pub struct ctxt { - // Specifically use a speedy hash algorithm for this hash map, it's used - // quite often. + /// Specifically use a speedy hash algorithm for this hash map, it's used + /// quite often. pub interner: RefCell>>, pub next_id: Cell, pub sess: Session, @@ -248,24 +248,24 @@ pub struct ctxt { pub region_maps: middle::region::RegionMaps, - // Stores the types for various nodes in the AST. Note that this table - // is not guaranteed to be populated until after typeck. See - // typeck::check::fn_ctxt for details. + /// Stores the types for various nodes in the AST. Note that this table + /// is not guaranteed to be populated until after typeck. See + /// typeck::check::fn_ctxt for details. pub node_types: node_type_table, - // Stores the type parameters which were substituted to obtain the type - // of this node. This only applies to nodes that refer to entities - // param>, - // Maps from a method to the method "descriptor" + /// Maps from a method to the method "descriptor" pub methods: RefCell>>, - // Maps from a trait def-id to a list of the def-ids of its methods + /// Maps from a trait def-id to a list of the def-ids of its methods pub trait_method_def_ids: RefCell>>>, - // A cache for the trait_methods() routine + /// A cache for the trait_methods() routine pub trait_methods_cache: RefCell>>>>, pub impl_trait_cache: RefCell>>>, @@ -287,64 +287,64 @@ pub struct ctxt { pub adjustments: RefCell>, pub normalized_cache: RefCell>, pub lang_items: middle::lang_items::LanguageItems, - // A mapping of fake provided method def_ids to the default implementation + /// A mapping of fake provided method def_ids to the default implementation pub provided_method_sources: RefCell>, pub supertraits: RefCell>>>>, pub superstructs: RefCell>>, pub struct_fields: RefCell>>>, - // Maps from def-id of a type or region parameter to its - // (inferred) variance. + /// Maps from def-id of a type or region parameter to its + /// (inferred) variance. pub item_variance_map: RefCell>>, - // A mapping from the def ID of an enum or struct type to the def ID - // of the method that implements its destructor. If the type is not - // present in this map, it does not have a destructor. This map is - // populated during the coherence phase of typechecking. + /// A mapping from the def ID of an enum or struct type to the def ID + /// of the method that implements its destructor. If the type is not + /// present in this map, it does not have a destructor. This map is + /// populated during the coherence phase of typechecking. pub destructor_for_type: RefCell>, - // A method will be in this list if and only if it is a destructor. + /// A method will be in this list if and only if it is a destructor. pub destructors: RefCell, - // Maps a trait onto a list of impls of that trait. + /// Maps a trait onto a list of impls of that trait. pub trait_impls: RefCell>>>>, - // Maps a DefId of a type to a list of its inherent impls. - // Contains implementations of methods that are inherent to a type. - // Methods in these implementations don't need to be exported. + /// Maps a DefId of a type to a list of its inherent impls. + /// Contains implementations of methods that are inherent to a type. + /// Methods in these implementations don't need to be exported. pub inherent_impls: RefCell>>>>, - // Maps a DefId of an impl to a list of its methods. - // Note that this contains all of the impls that we know about, - // including ones in other crates. It's not clear that this is the best - // way to do it. + /// Maps a DefId of an impl to a list of its methods. + /// Note that this contains all of the impls that we know about, + /// including ones in other crates. It's not clear that this is the best + /// way to do it. pub impl_methods: RefCell>>, - // Set of used unsafe nodes (functions or blocks). Unsafe nodes not - // present in this set can be warned about. + /// Set of used unsafe nodes (functions or blocks). Unsafe nodes not + /// present in this set can be warned about. pub used_unsafe: RefCell, - // Set of nodes which mark locals as mutable which end up getting used at - // some point. Local variable definitions not in this set can be warned - // about. + /// Set of nodes which mark locals as mutable which end up getting used at + /// some point. Local variable definitions not in this set can be warned + /// about. pub used_mut_nodes: RefCell, - // vtable resolution information for impl declarations + /// vtable resolution information for impl declarations pub impl_vtables: typeck::impl_vtable_map, - // The set of external nominal types whose implementations have been read. - // This is used for lazy resolution of methods. + /// The set of external nominal types whose implementations have been read. + /// This is used for lazy resolution of methods. pub populated_external_types: RefCell, - // The set of external traits whose implementations have been read. This - // is used for lazy resolution of traits. + /// The set of external traits whose implementations have been read. This + /// is used for lazy resolution of traits. pub populated_external_traits: RefCell, - // Borrows + /// Borrows pub upvar_borrow_map: RefCell, - // These two caches are used by const_eval when decoding external statics - // and variants that are found. + /// These two caches are used by const_eval when decoding external statics + /// and variants that are found. pub extern_const_statics: RefCell>>, pub extern_const_variants: RefCell>>, From c327080ee04e641a34f30ae71b713a91106680b1 Mon Sep 17 00:00:00 2001 From: Corey Richardson Date: Mon, 19 May 2014 14:57:24 -0700 Subject: [PATCH 3/4] rustc: add a lint for large enum variants It can be easy to accidentally bloat the size of an enum by making one variant larger than the others. When this happens, it usually goes unnoticed. This commit adds a lint that can warn when the largest variant in an enum is more than 3 times larger than the second-largest variant. This requires a little bit of rejiggering, because size information is only available in trans, but lint levels are only available in the lint context. It is allow by default because it's pretty noisy, and isn't really *that* undesirable. Closes #10362 --- src/librustc/middle/lint.rs | 118 ++++++++++++++++-------- src/librustc/middle/trans/base.rs | 58 +++++++++++- src/librustc/middle/ty.rs | 4 + src/test/run-pass/enum-size-variance.rs | 42 +++++++++ 4 files changed, 181 insertions(+), 41 deletions(-) create mode 100644 src/test/run-pass/enum-size-variance.rs diff --git a/src/librustc/middle/lint.rs b/src/librustc/middle/lint.rs index 88ded969ad5d1..7dc3db0a5d1ab 100644 --- a/src/librustc/middle/lint.rs +++ b/src/librustc/middle/lint.rs @@ -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)] pub enum Lint { CTypes, UnusedImports, @@ -93,6 +93,7 @@ pub enum Lint { UnknownFeatures, UnknownCrateType, UnsignedNegate, + VariantSizeDifference, ManagedHeapMemory, OwnedHeapMemory, @@ -146,8 +147,9 @@ pub struct LintSpec { pub type LintDict = HashMap<&'static str, LintSpec>; +// this is public for the lints that run in trans #[deriving(Eq)] -enum LintSource { +pub enum LintSource { Node(Span), Default, CommandLine @@ -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, @@ -461,6 +470,54 @@ struct Context<'a> { /// Ids of structs/enums which have been checked for raw_pointer_deriving checked_raw_pointers: NodeSet, + + /// Level of EnumSizeVariance lint for each enum, stored here because the + /// body of the lint needs to run in trans. + enum_levels: HashMap, +} + +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> { @@ -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) { @@ -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); } /** @@ -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); @@ -1878,6 +1921,7 @@ pub fn check_crate(tcx: &ty::ctxt, lint_stack: Vec::new(), negated_expr_id: -1, checked_raw_pointers: NodeSet::new(), + enum_levels: HashMap::new(), }; // Install default lint levels, followed by the command line levels, and @@ -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.enum_lint_levels.borrow_mut() = cx.enum_levels; } diff --git a/src/librustc/middle/trans/base.rs b/src/librustc/middle/trans/base.rs index e208097e99b33..5f708b2b8bfc2 100644 --- a/src/librustc/middle/trans/base.rs +++ b/src/librustc/middle/trans/base.rs @@ -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; @@ -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; @@ -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], + sp: Span, id: ast::NodeId, vi: &[Rc], i: &mut uint) { for &variant in enum_definition.variants.iter() { let disr_val = vi[*i].disr_val; @@ -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> { @@ -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) => { diff --git a/src/librustc/middle/ty.rs b/src/librustc/middle/ty.rs index 62679fa222bf6..1ce1eb0a82a6c 100644 --- a/src/librustc/middle/ty.rs +++ b/src/librustc/middle/ty.rs @@ -14,6 +14,7 @@ use back::svh::Svh; use driver::session::Session; use metadata::csearch; use mc = middle::mem_categorization; +use middle::lint; use middle::const_eval; use middle::dependency_format; use middle::lang_items::{ExchangeHeapLangItem, OpaqueStructLangItem}; @@ -352,6 +353,8 @@ pub struct ctxt { pub vtable_map: typeck::vtable_map, pub dependency_formats: RefCell, + + pub enum_lint_levels: RefCell>, } pub enum tbox_flag { @@ -1134,6 +1137,7 @@ pub fn mk_ctxt(s: Session, method_map: RefCell::new(FnvHashMap::new()), vtable_map: RefCell::new(FnvHashMap::new()), dependency_formats: RefCell::new(HashMap::new()), + enum_lint_levels: RefCell::new(HashMap::new()), } } diff --git a/src/test/run-pass/enum-size-variance.rs b/src/test/run-pass/enum-size-variance.rs new file mode 100644 index 0000000000000..39ab8316958a6 --- /dev/null +++ b/src/test/run-pass/enum-size-variance.rs @@ -0,0 +1,42 @@ +// Copyright 2014 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. +// +// ignore-pretty + +#![deny(enum_size_variance)] +#![allow(dead_code)] + +enum Enum1 { } + +enum Enum2 { A, B, C } + +enum Enum3 { D(int), E, F } + +enum Enum4 { H(int), I(int), J } + +enum Enum5 { //~ ERROR three times larger + L(int, int, int, int), //~ NOTE this variant is the largest + M(int), + N +} + +enum Enum6 { + O(T), + P(U), + Q(int) +} + +#[allow(enum_size_variance)] +enum Enum7 { + R(int, int, int, int), + S(int), + T +} +pub fn main() { } From d8467e23e74fba8447c4a48f75cc9fa78d66b1c3 Mon Sep 17 00:00:00 2001 From: Corey Richardson Date: Mon, 19 May 2014 22:28:09 -0700 Subject: [PATCH 4/4] rustc: abstract lint level exporting from EnumSizeVariance --- src/librustc/middle/lint.rs | 12 ++++++------ src/librustc/middle/ty.rs | 5 +++-- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/librustc/middle/lint.rs b/src/librustc/middle/lint.rs index 7dc3db0a5d1ab..e577102c4fe3e 100644 --- a/src/librustc/middle/lint.rs +++ b/src/librustc/middle/lint.rs @@ -72,7 +72,7 @@ use syntax::parse::token; use syntax::visit::Visitor; use syntax::{ast, ast_util, visit}; -#[deriving(Clone, Show, Eq, Ord, TotalEq, TotalOrd)] +#[deriving(Clone, Show, Eq, Ord, TotalEq, TotalOrd, Hash)] pub enum Lint { CTypes, UnusedImports, @@ -471,9 +471,9 @@ struct Context<'a> { /// Ids of structs/enums which have been checked for raw_pointer_deriving checked_raw_pointers: NodeSet, - /// Level of EnumSizeVariance lint for each enum, stored here because the - /// body of the lint needs to run in trans. - enum_levels: HashMap, + /// 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, @@ -1921,7 +1921,7 @@ pub fn check_crate(tcx: &ty::ctxt, lint_stack: Vec::new(), negated_expr_id: -1, checked_raw_pointers: NodeSet::new(), - enum_levels: HashMap::new(), + node_levels: HashMap::new(), }; // Install default lint levels, followed by the command line levels, and @@ -1963,5 +1963,5 @@ pub fn check_crate(tcx: &ty::ctxt, } tcx.sess.abort_if_errors(); - *tcx.enum_lint_levels.borrow_mut() = cx.enum_levels; + *tcx.node_lint_levels.borrow_mut() = cx.node_levels; } diff --git a/src/librustc/middle/ty.rs b/src/librustc/middle/ty.rs index 1ce1eb0a82a6c..3a7a94cdbcede 100644 --- a/src/librustc/middle/ty.rs +++ b/src/librustc/middle/ty.rs @@ -354,7 +354,8 @@ pub struct ctxt { pub dependency_formats: RefCell, - pub enum_lint_levels: RefCell>, + pub node_lint_levels: RefCell>, } pub enum tbox_flag { @@ -1137,7 +1138,7 @@ pub fn mk_ctxt(s: Session, method_map: RefCell::new(FnvHashMap::new()), vtable_map: RefCell::new(FnvHashMap::new()), dependency_formats: RefCell::new(HashMap::new()), - enum_lint_levels: RefCell::new(HashMap::new()), + node_lint_levels: RefCell::new(HashMap::new()), } }