Skip to content

new large_enum_variants lint #1187

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@ All notable changes to this project will be documented in this file.
[`items_after_statements`]: https://github.com/Manishearth/rust-clippy/wiki#items_after_statements
[`iter_next_loop`]: https://github.com/Manishearth/rust-clippy/wiki#iter_next_loop
[`iter_nth`]: https://github.com/Manishearth/rust-clippy/wiki#iter_nth
[`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
Expand Down
3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ Table of contents:

## Lints

There are 167 lints included in this crate:
There are 168 lints included in this crate:

name | default | triggers on
---------------------------------------------------------------------------------------------------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------
Expand Down Expand Up @@ -83,6 +83,7 @@ name
[items_after_statements](https://github.com/Manishearth/rust-clippy/wiki#items_after_statements) | allow | blocks where an item comes after a statement
[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
[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 and impls that have `.len()` but not `.is_empty()`
[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
Expand Down
71 changes: 71 additions & 0 deletions clippy_lints/src/large_enum_variant.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
//! lint when there are large variants on an enum

use rustc::lint::*;
use rustc::hir::*;
use utils::span_help_and_lint;
use rustc::ty::layout::TargetDataLayout;

/// **What it does:** Checks for large variants on enums.
///
/// **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 LateLintPass for LargeEnumVariant {
fn check_variant(&mut self, cx: &LateContext, variant: &Variant, _ :&Generics) {
let data_layout = TargetDataLayout::parse(cx.sess());
let param_env = cx.tcx.empty_parameter_environment();
let infcx = cx.tcx.borrowck_fake_infer_ctxt(param_env);
let mut variant_size = 0;

for field in variant.node.data.fields() {
let ty = cx.tcx.node_id_to_type(field.id);
if let Ok(layout) = ty.layout(&infcx) {
variant_size += layout.size(&data_layout).bytes();
}
}

if variant_size > self.maximum_variant_size_allowed {
span_help_and_lint(
cx,
LARGE_ENUM_VARIANT,
variant.span,
&format!("large enum variant found on variant `{}`", variant.node.name),
"consider boxing the large branches to reduce the total size of the enum",
);
}
}
}
3 changes: 3 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ pub mod functions;
pub mod identity_op;
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;
Expand Down Expand Up @@ -260,6 +261,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
reg.register_late_lint_pass(box assign_ops::AssignOps);
reg.register_late_lint_pass(box let_if_seq::LetIfSeq);
reg.register_late_lint_pass(box eval_order_dependence::EvalOrderDependence);
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,
Expand Down Expand Up @@ -337,6 +339,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
functions::NOT_UNSAFE_PTR_ARG_DEREF,
functions::TOO_MANY_ARGUMENTS,
identity_op::IDENTITY_OP,
large_enum_variant::LARGE_ENUM_VARIANT,
len_zero::LEN_WITHOUT_IS_EMPTY,
len_zero::LEN_ZERO,
let_if_seq::USELESS_LET_IF_SEQ,
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/utils/conf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,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),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used the same value as 'too-large-for-stack'. I don't know which value use as default

Copy link
Member

@mcarton mcarton Aug 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's already quite big for an enum variant IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So is the default value ok? Should I leave it to 200?

}

/// Read the `toml` configuration file. The function will ignore “File not found” errors iif
Expand Down
24 changes: 24 additions & 0 deletions tests/compile-fail/large_enum_variant.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
#![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 on variant `B`
}

enum AnotherLargeEnum {
VariantOk(i32, u32),
ContainingLargeEnum(LargeEnum), //~ ERROR large enum variant found on variant `ContainingLargeEnum`
ContainingMoreThanOneField(i32, [i32; 8000], [i32; 9500]), //~ ERROR large enum variant found on variant `ContainingMoreThanOneField`
VoidVariant,
StructLikeLittle { x: i32, y: i32 },
StructLikeLarge { x: [i32; 8000], y: i32 }, //~ ERROR large enum variant found on variant `StructLikeLarge`
}

fn main() {

}