Move variant_size_differences out of trans

Also enhances the error message a bit, fixes #30505 on the way, and adds
a test (which was missing).

Closes #34018

src/librustc/lint/builtin.rs

@@ -94,12 +94,6 @@
     "unknown crate type found in #[crate_type] directive"
 }
 
-declare_lint! {
-    pub VARIANT_SIZE_DIFFERENCES,
-    Allow,
-    "detects enums with widely varying variant sizes"
-}
-
 declare_lint! {
     pub FAT_PTR_TRANSMUTES,
     Allow,
@@ -230,7 +224,6 @@ fn get_lints(&self) -> LintArray {
             UNUSED_FEATURES,
             STABLE_FEATURES,
             UNKNOWN_CRATE_TYPES,
-            VARIANT_SIZE_DIFFERENCES,
             FAT_PTR_TRANSMUTES,
             TRIVIAL_CASTS,
             TRIVIAL_NUMERIC_CASTS,

src/librustc/lint/context.rs

@@ -29,8 +29,8 @@
 use middle::privacy::AccessLevels;
 use ty::TyCtxt;
 use session::{config, early_error, Session};
-use lint::{Level, LevelSource, Lint, LintId, LintArray, LintPass};
-use lint::{EarlyLintPassObject, LateLintPass, LateLintPassObject};
+use lint::{Level, LevelSource, Lint, LintId, LintPass};
+use lint::{EarlyLintPassObject, LateLintPassObject};
 use lint::{Default, CommandLine, Node, Allow, Warn, Deny, Forbid};
 use lint::builtin;
 use util::nodemap::FnvHashMap;
@@ -1064,38 +1064,6 @@ fn visit_id(&mut self, id: ast::NodeId) {
     }
 }
 
-// This lint pass is defined here because it touches parts of the `LateContext`
-// that we don't want to expose. It records the lint level at certain AST
-// nodes, so that the variant size difference check in trans can call
-// `raw_emit_lint`.
-
-pub struct GatherNodeLevels;
-
-impl LintPass for GatherNodeLevels {
-    fn get_lints(&self) -> LintArray {
-        lint_array!()
-    }
-}
-
-impl LateLintPass for GatherNodeLevels {
-    fn check_item(&mut self, cx: &LateContext, it: &hir::Item) {
-        match it.node {
-            hir::ItemEnum(..) => {
-                let lint_id = LintId::of(builtin::VARIANT_SIZE_DIFFERENCES);
-                let lvlsrc = cx.lints.get_level_source(lint_id);
-                match lvlsrc {
-                    (lvl, _) if lvl != Allow => {
-                        cx.node_levels.borrow_mut()
-                            .insert((it.id, lint_id), lvlsrc);
-                    },
-                    _ => { }
-                }
-            },
-            _ => { }
-        }
-    }
-}
-
 enum CheckLintNameResult {
     Ok,
     // Lint doesn't exist

src/librustc/lint/mod.rs

@@ -41,7 +41,7 @@
 
 pub use lint::context::{LateContext, EarlyContext, LintContext, LintStore,
                         raw_emit_lint, check_crate, check_ast_crate, gather_attrs,
-                        raw_struct_lint, GatherNodeLevels, FutureIncompatibleInfo};
+                        raw_struct_lint, FutureIncompatibleInfo};
 
 /// Specification of a single lint.
 #[derive(Copy, Clone, Debug)]

src/librustc/session/config.rs

@@ -727,8 +727,6 @@ fn parse_panic_strategy(slot: &mut PanicStrategy, v: Option<&str>) -> bool {
         "load extra plugins"),
     unstable_options: bool = (false, parse_bool,
           "adds unstable command line options to rustc interface"),
-    print_enum_sizes: bool = (false, parse_bool,
-          "print the size of enums and their variants"),
     force_overflow_checks: Option<bool> = (None, parse_opt_bool,
           "force overflow checks on or off"),
     force_dropflag_checks: Option<bool> = (None, parse_opt_bool,

src/librustc/session/mod.rs

@@ -304,9 +304,6 @@ pub fn no_landing_pads(&self) -> bool {
     pub fn unstable_options(&self) -> bool {
         self.opts.debugging_opts.unstable_options
     }
-    pub fn print_enum_sizes(&self) -> bool {
-        self.opts.debugging_opts.print_enum_sizes
-    }
     pub fn nonzeroing_move_hints(&self) -> bool {
         self.opts.debugging_opts.enable_nonzeroing_move_hints
     }

src/librustc_lint/lib.rs

@@ -108,6 +108,7 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) {
                  HardwiredLints,
                  WhileTrue,
                  ImproperCTypes,
+                 VariantSizeDifferences,
                  BoxPointers,
                  UnusedAttributes,
                  PathStatements,
@@ -209,9 +210,6 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) {
         },
         ]);
 
-    // We have one lint pass defined specially
-    store.register_late_pass(sess, false, box lint::GatherNodeLevels);
-
     // Register renamed and removed lints
     store.register_renamed("unknown_features", "unused_features");
     store.register_removed("unsigned_negation", "replaced by negate_unsigned feature gate");

src/librustc_lint/types.rs

@@ -13,6 +13,8 @@
 use rustc::hir::def_id::DefId;
 use rustc::ty::subst::Substs;
 use rustc::ty::{self, Ty, TyCtxt};
+use rustc::ty::layout::{Layout, Primitive};
+use rustc::traits::ProjectionMode;
 use middle::const_val::ConstVal;
 use rustc_const_eval::eval_const_expr_partial;
 use rustc_const_eval::EvalHint::ExprTypeChecked;
@@ -77,6 +79,12 @@
     "shift exceeds the type's number of bits"
 }
 
+declare_lint! {
+    VARIANT_SIZE_DIFFERENCES,
+    Allow,
+    "detects enums with widely varying variant sizes"
+}
+
 #[derive(Copy, Clone)]
 pub struct TypeLimits {
     /// Id of the last visited negated expression
@@ -676,3 +684,63 @@ fn check_item(&mut self, cx: &LateContext, it: &hir::Item) {
         }
     }
 }
+
+pub struct VariantSizeDifferences;
+
+impl LintPass for VariantSizeDifferences {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(VARIANT_SIZE_DIFFERENCES)
+    }
+}
+
+impl LateLintPass for VariantSizeDifferences {
+    fn check_item(&mut self, cx: &LateContext, it: &hir::Item) {
+        if let hir::ItemEnum(ref enum_definition, ref gens) = it.node {
+            if gens.ty_params.is_empty() {  // sizes only make sense for non-generic types
+                let mut sizes = vec![];
+                let t = cx.tcx.node_id_to_type(it.id);
+                let layout = cx.tcx.normalizing_infer_ctxt(ProjectionMode::Any).enter(|infcx| {
+                    t.layout(&infcx).unwrap_or_else(|e| {
+                        bug!("failed to get layout for `{}`: {}", t, e)
+                    })
+                });
+
+                if let Layout::General { ref variants, ref size, discr, .. } = *layout {
+                    let discr_size = Primitive::Int(discr).size(&cx.tcx.data_layout).bytes();
+
+                    debug!("enum `{}` is {} bytes large", t, size.bytes());
+
+                    for (variant, variant_layout) in enum_definition.variants.iter().zip(variants) {
+                        // Subtract the size of the enum discriminant
+                        let bytes = variant_layout.min_size().bytes().saturating_sub(discr_size);
+                        sizes.push(bytes);
+
+                        debug!("- variant `{}` is {} bytes large", variant.node.name, bytes);
+                    }
+
+                    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 {
+                        cx.span_lint(VARIANT_SIZE_DIFFERENCES,
+                                     enum_definition.variants[largest_index].span,
+                                     &format!("enum variant is more than three times larger \
+                                               ({} bytes) than the next largest", largest));
+                    }
+                }
+            }
+        }
+    }
+}

src/librustc_trans/base.rs

@@ -30,7 +30,6 @@
 
 use back::link;
 use back::linker::LinkerInfo;
-use lint;
 use llvm::{BasicBlockRef, Linkage, ValueRef, Vector, get_param};
 use llvm;
 use rustc::cfg;
@@ -75,7 +74,7 @@
 use glue;
 use inline;
 use machine;
-use machine::{llalign_of_min, llsize_of, llsize_of_real};
+use machine::{llalign_of_min, llsize_of};
 use meth;
 use mir;
 use monomorphize::{self, Instance};
@@ -86,7 +85,6 @@
 use tvec;
 use type_::Type;
 use type_of;
-use type_of::*;
 use value::Value;
 use Disr;
 use util::common::indenter;
@@ -2074,87 +2072,6 @@ pub fn trans_ctor_shim<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
     fcx.finish(bcx, DebugLoc::None);
 }
 
-fn enum_variant_size_lint(ccx: &CrateContext, enum_def: &hir::EnumDef, sp: Span, id: ast::NodeId) {
-    let mut sizes = Vec::new(); // does no allocation if no pushes, thankfully
-
-    let print_info = ccx.sess().print_enum_sizes();
-
-    let levels = ccx.tcx().node_lint_levels.borrow();
-    let lint_id = lint::LintId::of(lint::builtin::VARIANT_SIZE_DIFFERENCES);
-    let lvlsrc = levels.get(&(id, lint_id));
-    let is_allow = lvlsrc.map_or(true, |&(lvl, _)| lvl == lint::Allow);
-
-    if is_allow && !print_info {
-        // we're not interested in anything here
-        return;
-    }
-
-    let ty = ccx.tcx().node_id_to_type(id);
-    let avar = adt::represent_type(ccx, ty);
-    match *avar {
-        adt::General(_, ref variants, _) => {
-            for var in variants {
-                let mut size = 0;
-                for field in var.fields.iter().skip(1) {
-                    // skip the discriminant
-                    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)
-            }
-    );
-
-    // FIXME(#30505) Should use logging for this.
-    if print_info {
-        let llty = type_of::sizing_type_of(ccx, ty);
-
-        let sess = &ccx.tcx().sess;
-        sess.span_note_without_error(sp,
-                                     &format!("total size: {} bytes", llsize_of_real(ccx, llty)));
-        match *avar {
-            adt::General(..) => {
-                for (i, var) in enum_def.variants.iter().enumerate() {
-                    ccx.tcx()
-                       .sess
-                       .span_note_without_error(var.span,
-                                                &format!("variant data: {} bytes", sizes[i]));
-                }
-            }
-            _ => {}
-        }
-    }
-
-    // we only warn if the largest variant is at least thrice as large as
-    // the second-largest.
-    if !is_allow && largest > slargest * 3 && slargest > 0 {
-        // Use lint::raw_emit_lint rather than sess.add_lint because the lint-printing
-        // pass for the latter already ran.
-        lint::raw_struct_lint(&ccx.tcx().sess,
-                              &ccx.tcx().sess.lint_store.borrow(),
-                              lint::builtin::VARIANT_SIZE_DIFFERENCES,
-                              *lvlsrc.unwrap(),
-                              Some(sp),
-                              &format!("enum variant is more than three times larger ({} bytes) \
-                                        than the next largest (ignoring padding)",
-                                       largest))
-            .span_note(enum_def.variants[largest_index].span,
-                       "this variant is the largest")
-            .emit();
-    }
-}
-
 pub fn llvm_linkage_by_name(name: &str) -> Option<Linkage> {
     // Use the names from src/llvm/docs/LangRef.rst here. Most types are only
     // applicable to variable declarations and may not really make sense for
@@ -2194,26 +2111,6 @@ pub fn set_link_section(ccx: &CrateContext,
     }
 }
 
-fn trans_item(ccx: &CrateContext, item: &hir::Item) {
-    let _icx = push_ctxt("trans_item");
-
-    match item.node {
-        hir::ItemEnum(ref enum_definition, ref gens) => {
-            if gens.ty_params.is_empty() {
-                // sizes only make sense for non-generic types
-                enum_variant_size_lint(ccx, enum_definition, item.span, item.id);
-            }
-        }
-        hir::ItemFn(..) |
-        hir::ItemImpl(..) |
-        hir::ItemStatic(..) => {
-            // Don't do anything here. Translation has been moved to
-            // being "collector-driven".
-        }
-        _ => {}
-    }
-}
-
 /// Create the `main` function which will initialise the rust runtime and call
 /// users’ main function.
 pub fn maybe_create_entry_wrapper(ccx: &CrateContext) {
@@ -2659,19 +2556,6 @@ pub fn trans_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
 
     symbol_names_test::report_symbol_names(&shared_ccx);
 
-    {
-        let ccx = crate_context_list.get_ccx(0);
-
-        // FIXME: #34018
-        // At this point, we only walk the HIR for running
-        // enum_variant_size_lint(). This should arguably be moved somewhere
-        // else.
-        {
-            intravisit::walk_mod(&mut TransItemsWithinModVisitor { ccx: &ccx }, &krate.module);
-            krate.visit_all_items(&mut TransModVisitor { ccx: &ccx });
-        }
-    }
-
     if shared_ccx.sess().trans_stats() {
         let stats = shared_ccx.stats();
         println!("--- trans stats ---");
@@ -2758,72 +2642,6 @@ pub fn trans_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
     }
 }
 
-/// We visit all the items in the krate and translate them.  We do
-/// this in two walks. The first walk just finds module items. It then
-/// walks the full contents of those module items and translates all
-/// the items within. Note that this entire process is O(n). The
-/// reason for this two phased walk is that each module is
-/// (potentially) placed into a distinct codegen-unit. This walk also
-/// ensures that the immediate contents of each module is processed
-/// entirely before we proceed to find more modules, helping to ensure
-/// an equitable distribution amongst codegen-units.
-pub struct TransModVisitor<'a, 'tcx: 'a> {
-    pub ccx: &'a CrateContext<'a, 'tcx>,
-}
-
-impl<'a, 'tcx, 'v> Visitor<'v> for TransModVisitor<'a, 'tcx> {
-    fn visit_item(&mut self, i: &hir::Item) {
-        match i.node {
-            hir::ItemMod(_) => {
-                let item_ccx = self.ccx.rotate();
-                intravisit::walk_item(&mut TransItemsWithinModVisitor { ccx: &item_ccx }, i);
-            }
-            _ => { }
-        }
-    }
-}
-
-/// Translates all the items within a given module. Expects owner to
-/// invoke `walk_item` on a module item. Ignores nested modules.
-pub struct TransItemsWithinModVisitor<'a, 'tcx: 'a> {
-    pub ccx: &'a CrateContext<'a, 'tcx>,
-}
-
-impl<'a, 'tcx, 'v> Visitor<'v> for TransItemsWithinModVisitor<'a, 'tcx> {
-    fn visit_nested_item(&mut self, item_id: hir::ItemId) {
-        self.visit_item(self.ccx.tcx().map.expect_item(item_id.id));
-    }
-
-    fn visit_item(&mut self, i: &hir::Item) {
-        match i.node {
-            hir::ItemMod(..) => {
-                // skip modules, they will be uncovered by the TransModVisitor
-            }
-            _ => {
-                let def_id = self.ccx.tcx().map.local_def_id(i.id);
-                let tcx = self.ccx.tcx();
-
-                // Create a subtask for trans'ing a particular item. We are
-                // giving `trans_item` access to this item, so also record a read.
-                tcx.dep_graph.with_task(DepNode::TransCrateItem(def_id), || {
-                    tcx.dep_graph.read(DepNode::Hir(def_id));
-
-                    // We are going to be accessing various tables
-                    // generated by TypeckItemBody; we also assume
-                    // that the body passes type check. These tables
-                    // are not individually tracked, so just register
-                    // a read here.
-                    tcx.dep_graph.read(DepNode::TypeckItemBody(def_id));
-
-                    trans_item(self.ccx, i);
-                });
-
-                intravisit::walk_item(self, i);
-            }
-        }
-    }
-}
-
 fn collect_and_partition_translation_items<'a, 'tcx>(scx: &SharedCrateContext<'a, 'tcx>)
                                                      -> (Vec<CodegenUnit<'tcx>>, SymbolMap<'tcx>) {
     let time_passes = scx.sess().time_passes();

src/librustc_trans/context.rs

@@ -219,14 +219,6 @@ pub fn iter<'b>(&'b self) -> CrateContextIterator<'b, 'tcx> {
         }
     }
 
-    pub fn get_ccx<'b>(&'b self, index: usize) -> CrateContext<'b, 'tcx> {
-        CrateContext {
-            shared: self.shared,
-            index: index,
-            local_ccxs: &self.local_ccxs[..],
-        }
-    }
-
     pub fn shared(&self) -> &'a SharedCrateContext<'a, 'tcx> {
         self.shared
     }

src/librustc_trans/trans_item.rs

@@ -28,6 +28,7 @@
 use rustc::hir::def_id::DefId;
 use rustc::ty::{self, Ty, TyCtxt, TypeFoldable};
 use rustc::ty::subst;
+use rustc::dep_graph::DepNode;
 use std::hash::{Hash, Hasher};
 use syntax::ast::{self, NodeId};
 use syntax::{attr,errors};
@@ -73,6 +74,8 @@ pub fn define(&self, ccx: &CrateContext<'a, 'tcx>) {
                   self.to_raw_string(),
                   ccx.codegen_unit().name);
 
+        self.register_reads(ccx);
+
         match *self {
             TransItem::Static(node_id) => {
                 let item = ccx.tcx().map.expect_item(node_id);
@@ -99,6 +102,35 @@ pub fn define(&self, ccx: &CrateContext<'a, 'tcx>) {
                ccx.codegen_unit().name);
     }
 
+    /// If necessary, creates a subtask for trans'ing a particular item and registers reads on
+    /// `TypeckItemBody` and `Hir`.
+    fn register_reads(&self, ccx: &CrateContext<'a, 'tcx>) {
+        let tcx = ccx.tcx();
+        let def_id = match *self {
+            TransItem::Static(node_id) => {
+                tcx.map.local_def_id(node_id)
+            }
+            TransItem::Fn(instance) => {
+                instance.def
+            }
+            TransItem::DropGlue(_) => {
+                // Nothing to track for drop glue
+                return;
+            }
+        };
+
+        tcx.dep_graph.with_task(DepNode::TransCrateItem(def_id), || {
+            tcx.dep_graph.read(DepNode::Hir(def_id));
+
+            // We are going to be accessing various tables
+            // generated by TypeckItemBody; we also assume
+            // that the body passes type check. These tables
+            // are not individually tracked, so just register
+            // a read here.
+            tcx.dep_graph.read(DepNode::TypeckItemBody(def_id));
+        });
+    }
+
     pub fn predefine(&self,
                      ccx: &CrateContext<'a, 'tcx>,
                      linkage: llvm::Linkage) {

src/test/compile-fail/variant-size-differences.rs

+// Copyright 2016 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.
+
+#![deny(variant_size_differences)]
+
+enum _En {
+    V0(u8),
+    VBig([u8; 1024]),   //~ ERROR variant is more than three times larger
+}
+
+fn main() {}