diff --git a/CHANGELOG.md b/CHANGELOG.md index 76cc7852b01..cfa110543c1 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.md b/README.md index 01bf489f384..18166118361 100644 --- a/README.md +++ b/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 diff --git a/clippy_lints/src/large_enum_variant.rs b/clippy_lints/src/large_enum_variant.rs new file mode 100644 index 00000000000..ca812067878 --- /dev/null +++ b/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"); + }); + } + }); + } + } + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 37e3953f7f8..fb524df1a8f 100644 --- a/clippy_lints/src/lib.rs +++ b/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, diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs index d80fa17e29f..cfe3f00b717 100644 --- a/clippy_lints/src/utils/conf.rs +++ b/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. diff --git a/tests/compile-fail/large_enum_variant.rs b/tests/compile-fail/large_enum_variant.rs new file mode 100644 index 00000000000..8d289a32832 --- /dev/null +++ b/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 { + 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 { + 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 + 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() { + +}