]> git.lizzy.rs Git - rust.git/commitdiff
rustc: add a lint for large enum variants
authorCorey Richardson <corey@octayn.net>
Mon, 19 May 2014 21:57:24 +0000 (14:57 -0700)
committerCorey Richardson <corey@octayn.net>
Fri, 23 May 2014 06:01:47 +0000 (23:01 -0700)
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
src/librustc/middle/trans/base.rs
src/librustc/middle/ty.rs
src/test/run-pass/enum-size-variance.rs [new file with mode: 0644]

index 88ded969ad5d141e430b6eaa10f9bf4450eb5d37..7dc3db0a5d1ab977c5ee882b15febecd81043780 100644 (file)
@@ -72,7 +72,7 @@
 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 @@ enum LintSource {
         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<ast::NodeId, (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> {
@@ -492,7 +549,7 @@ fn lint_to_str(&self, lint: Lint) -> &'static str {
                 return *k;
             }
         }
-        fail!("unregistered lint {:?}", lint);
+        fail!("unregistered lint {}", lint);
     }
 
     fn span_lint(&self, lint: Lint, span: Span, msg: &str) {
@@ -501,37 +558,8 @@ fn span_lint(&self, lint: Lint, span: Span, msg: &str) {
             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;
 }
index e208097e99b33a27cd8261bf66aad140039878ff..5f708b2b8bfc24b6281a40f96704af2123444b9e 100644 (file)
@@ -36,6 +36,7 @@
 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::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<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;
@@ -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) => {
index 62679fa222bf6121c930015c352ede5eb10f438d..1ce1eb0a82a6ccffdbf5ab5758f12caef31e03e4 100644 (file)
@@ -14,6 +14,7 @@
 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<dependency_format::Dependencies>,
+
+    pub enum_lint_levels: RefCell<HashMap<ast::NodeId, (lint::Level, lint::LintSource)>>,
 }
 
 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 (file)
index 0000000..39ab831
--- /dev/null
@@ -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 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, 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<T, U> {
+    O(T),
+    P(U),
+    Q(int)
+}
+
+#[allow(enum_size_variance)]
+enum Enum7 {
+    R(int, int, int, int),
+    S(int),
+    T
+}
+pub fn main() { }