Merge pull request #1492 from Manishearth/largeEnumVariant

large_enum_variants lint suggests to box variants above a configurable limit

CHANGELOG.md

@@ -337,6 +337,7 @@ All notable changes to this project will be documented in this file.
 [`iter_next_loop`]: https://github.com/Manishearth/rust-clippy/wiki#iter_next_loop
 [`iter_nth`]: https://github.com/Manishearth/rust-clippy/wiki#iter_nth
 [`iter_skip_next`]: https://github.com/Manishearth/rust-clippy/wiki#iter_skip_next
+[`large_enum_variant`]: https://github.com/Manishearth/rust-clippy/wiki#large_enum_variant
 [`len_without_is_empty`]: https://github.com/Manishearth/rust-clippy/wiki#len_without_is_empty
 [`len_zero`]: https://github.com/Manishearth/rust-clippy/wiki#len_zero
 [`let_and_return`]: https://github.com/Manishearth/rust-clippy/wiki#let_and_return

README.md

@@ -180,7 +180,7 @@ transparently:
 
 ## Lints
 
-There are 183 lints included in this crate:
+There are 184 lints included in this crate:
 
 name                                                                                                                   | default | triggers on
 -----------------------------------------------------------------------------------------------------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------
@@ -255,6 +255,7 @@ name
 [iter_next_loop](https://github.com/Manishearth/rust-clippy/wiki#iter_next_loop)                                       | warn    | for-looping over `_.next()` which is probably not intended
 [iter_nth](https://github.com/Manishearth/rust-clippy/wiki#iter_nth)                                                   | warn    | using `.iter().nth()` on a standard library type with O(1) element access
 [iter_skip_next](https://github.com/Manishearth/rust-clippy/wiki#iter_skip_next)                                       | warn    | using `.skip(x).next()` on an iterator
+[large_enum_variant](https://github.com/Manishearth/rust-clippy/wiki#large_enum_variant)                               | warn    | large variants on an enum
 [len_without_is_empty](https://github.com/Manishearth/rust-clippy/wiki#len_without_is_empty)                           | warn    | traits or impls with a public `len` method but no corresponding `is_empty` method
 [len_zero](https://github.com/Manishearth/rust-clippy/wiki#len_zero)                                                   | warn    | checking `.len() == 0` or `.len() > 0` (or similar) when `.is_empty()` could be used instead
 [let_and_return](https://github.com/Manishearth/rust-clippy/wiki#let_and_return)                                       | warn    | creating a let-binding and then immediately returning it like `let x = expr; x` at the end of a block

clippy_lints/src/large_enum_variant.rs

@@ -0,0 +1,98 @@
+//! lint when there are large variants on an enum
+
+use rustc::lint::*;
+use rustc::hir::*;
+use utils::{span_lint_and_then, snippet_opt};
+use rustc::ty::layout::TargetDataLayout;
+use rustc::ty::TypeFoldable;
+use rustc::traits::Reveal;
+
+/// **What it does:** Checks for large variants on `enum`s.
+///
+/// **Why is this bad?** Enum size is bounded by the largest variant. Having a large variant
+/// can penalize the memory layout of that enum.
+///
+/// **Known problems:** None.
+///
+/// **Example:**
+/// ```rust
+/// enum Test {
+///    A(i32),
+///    B([i32; 8000]),
+/// }
+/// ```
+declare_lint! {
+    pub LARGE_ENUM_VARIANT,
+    Warn,
+    "large variants on an enum"
+}
+
+#[derive(Copy,Clone)]
+pub struct LargeEnumVariant {
+    maximum_variant_size_allowed: u64,
+}
+
+impl LargeEnumVariant {
+    pub fn new(maximum_variant_size_allowed: u64) -> Self {
+        LargeEnumVariant { maximum_variant_size_allowed: maximum_variant_size_allowed }
+    }
+}
+
+impl LintPass for LargeEnumVariant {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(LARGE_ENUM_VARIANT)
+    }
+}
+
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for LargeEnumVariant {
+    fn check_item(&mut self, cx: &LateContext, item: &Item) {
+        let did = cx.tcx.map.local_def_id(item.id);
+        if let ItemEnum(ref def, _) = item.node {
+            let ty = cx.tcx.item_type(did);
+            let adt = ty.ty_adt_def().expect("already checked whether this is an enum");
+            for (i, variant) in adt.variants.iter().enumerate() {
+                let data_layout = TargetDataLayout::parse(cx.sess());
+                cx.tcx.infer_ctxt((), Reveal::All).enter(|infcx| {
+                    let size: u64 = variant.fields
+                        .iter()
+                        .map(|f| {
+                            let ty = cx.tcx.item_type(f.did);
+                            if ty.needs_subst() {
+                                0 // we can't reason about generics, so we treat them as zero sized
+                            } else {
+                                ty.layout(&infcx)
+                                    .expect("layout should be computable for concrete type")
+                                    .size(&data_layout)
+                                    .bytes()
+                            }
+                        })
+                        .sum();
+                    if size > self.maximum_variant_size_allowed {
+                        span_lint_and_then(cx,
+                                           LARGE_ENUM_VARIANT,
+                                           def.variants[i].span,
+                                           "large enum variant found",
+                                           |db| {
+                            if variant.fields.len() == 1 {
+                                let span = match def.variants[i].node.data {
+                                    VariantData::Struct(ref fields, _) |
+                                    VariantData::Tuple(ref fields, _) => fields[0].ty.span,
+                                    VariantData::Unit(_) => unreachable!(),
+                                };
+                                if let Some(snip) = snippet_opt(cx, span) {
+                                    db.span_suggestion(span,
+                                                       "consider boxing the large fields to reduce the total size of \
+                                                        the enum",
+                                                       format!("Box<{}>", snip));
+                                    return;
+                                }
+                            }
+                            db.span_help(def.variants[i].span,
+                                         "consider boxing the large fields to reduce the total size of the enum");
+                        });
+                    }
+                });
+            }
+        }
+    }
+}

clippy_lints/src/lib.rs

@@ -88,6 +88,7 @@ pub mod identity_op;
 pub mod if_let_redundant_pattern_matching;
 pub mod if_not_else;
 pub mod items_after_statements;
+pub mod large_enum_variant;
 pub mod len_zero;
 pub mod let_if_seq;
 pub mod lifetimes;
@@ -293,6 +294,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
     reg.register_early_lint_pass(box reference::Pass);
     reg.register_early_lint_pass(box double_parens::DoubleParens);
     reg.register_late_lint_pass(box unused_io_amount::UnusedIoAmount);
+    reg.register_late_lint_pass(box large_enum_variant::LargeEnumVariant::new(conf.enum_variant_size_threshold));
 
     reg.register_lint_group("clippy_restrictions", vec![
         arithmetic::FLOAT_ARITHMETIC,
@@ -384,6 +386,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         functions::TOO_MANY_ARGUMENTS,
         identity_op::IDENTITY_OP,
         if_let_redundant_pattern_matching::IF_LET_REDUNDANT_PATTERN_MATCHING,
+        large_enum_variant::LARGE_ENUM_VARIANT,
         len_zero::LEN_WITHOUT_IS_EMPTY,
         len_zero::LEN_ZERO,
         let_if_seq::USELESS_LET_IF_SEQ,

clippy_lints/src/utils/conf.rs

@@ -186,6 +186,8 @@ define_Conf! {
     ("too-large-for-stack", too_large_for_stack, 200 => u64),
     /// Lint: ENUM_VARIANT_NAMES. The minimum number of enum variants for the lints about variant names to trigger
     ("enum-variant-name-threshold", enum_variant_name_threshold, 3 => u64),
+    /// Lint: LARGE_ENUM_VARIANT. The maximum size of a emum's variant to avoid box suggestion
+    ("enum-variant-size-threshold", enum_variant_size_threshold, 200 => u64),
 }
 
 /// Search for the configuration file.

tests/compile-fail/large_enum_variant.rs

@@ -0,0 +1,53 @@
+#![feature(plugin)]
+#![plugin(clippy)]
+
+#![allow(dead_code)]
+#![allow(unused_variables)]
+#![deny(large_enum_variant)]
+
+enum LargeEnum {
+    A(i32),
+    B([i32; 8000]), //~ ERROR large enum variant found
+    //~^ HELP consider boxing the large fields to reduce the total size of the enum
+    //~| SUGGESTION Box<[i32; 8000]>
+}
+
+enum GenericEnum<T> {
+    A(i32),
+    B([i32; 8000]), //~ ERROR large enum variant found
+    //~^ HELP consider boxing the large fields to reduce the total size of the enum
+    //~| SUGGESTION Box<[i32; 8000]>
+    C([T; 8000]),
+    D(T, [i32; 8000]), //~ ERROR large enum variant found
+    //~^ HELP consider boxing the large fields to reduce the total size of the enum
+}
+
+trait SomeTrait {
+    type Item;
+}
+
+enum LargeEnumGeneric<A: SomeTrait> {
+    Var(A::Item), // regression test, this used to ICE
+}
+
+enum AnotherLargeEnum {
+    VariantOk(i32, u32),
+    ContainingLargeEnum(LargeEnum), //~ ERROR large enum variant found
+    //~^ HELP consider boxing the large fields to reduce the total size of the enum
+    //~| SUGGESTION Box<LargeEnum>
+    ContainingMoreThanOneField(i32, [i32; 8000], [i32; 9500]), //~ ERROR large enum variant found
+    //~^ HELP consider boxing the large fields to reduce the total size of the enum
+    VoidVariant,
+    StructLikeLittle { x: i32, y: i32 },
+    StructLikeLarge { x: [i32; 8000], y: i32 }, //~ ERROR large enum variant found
+    //~^ HELP consider boxing the large fields to reduce the total size of the enum
+    StructLikeLarge2 { //~ ERROR large enum variant found
+        x:
+        [i32; 8000] //~ SUGGESTION Box<[i32; 8000]>
+        //~^ HELP consider boxing the large fields to reduce the total size of the enum
+    },
+}
+
+fn main() {
+
+}