From 2b7bc260de3fedd94d4134b3e3bda48f60c5fd17 Mon Sep 17 00:00:00 2001 From: Mikhail Babenko Date: Thu, 23 Jan 2020 17:52:41 +0300 Subject: [PATCH] add size parameter for lint --- clippy_lints/src/lib.rs | 3 +- clippy_lints/src/types.rs | 386 +++++++++--------- clippy_lints/src/utils/conf.rs | 2 + .../toml_unknown_key/conf_unknown_key.stderr | 2 +- tests/ui-toml/vec_box_sized/clippy.toml | 1 + tests/ui-toml/vec_box_sized/test.rs | 15 + tests/ui-toml/vec_box_sized/test.stderr | 22 + tests/ui/vec_box_sized.fixed | 4 +- tests/ui/vec_box_sized.rs | 4 +- tests/ui/vec_box_sized.stderr | 6 +- 10 files changed, 254 insertions(+), 191 deletions(-) create mode 100644 tests/ui-toml/vec_box_sized/clippy.toml create mode 100644 tests/ui-toml/vec_box_sized/test.rs create mode 100644 tests/ui-toml/vec_box_sized/test.stderr diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 962ff38eb6c..7e05166b1a6 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -818,7 +818,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box utils::internal_lints::OuterExpnDataPass); store.register_late_pass(|| box utils::inspector::DeepCodeInspector); store.register_late_pass(|| box utils::author::Author); - store.register_late_pass(|| box types::Types); + let vec_box_size_threshold = conf.vec_box_size_threshold; + store.register_late_pass(move || box types::Types::new(vec_box_size_threshold)); store.register_late_pass(|| box booleans::NonminimalBool); store.register_late_pass(|| box eq_op::EqOp); store.register_late_pass(|| box enum_glob_use::EnumGlobUse); diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index 52ced27c477..b271b58d903 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -167,7 +167,11 @@ declare_clippy_lint! { "a borrow of a boxed type" } -declare_lint_pass!(Types => [BOX_VEC, VEC_BOX, OPTION_OPTION, LINKEDLIST, BORROWED_BOX]); +pub struct Types { + vec_box_size_threshold: u64, +} + +impl_lint_pass!(Types => [BOX_VEC, VEC_BOX, OPTION_OPTION, LINKEDLIST, BORROWED_BOX]); impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Types { fn check_fn( @@ -186,38 +190,28 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Types { } } - check_fn_decl(cx, decl); + self.check_fn_decl(cx, decl); } fn check_struct_field(&mut self, cx: &LateContext<'_, '_>, field: &hir::StructField<'_>) { - check_ty(cx, &field.ty, false); + self.check_ty(cx, &field.ty, false); } fn check_trait_item(&mut self, cx: &LateContext<'_, '_>, item: &TraitItem<'_>) { match item.kind { - TraitItemKind::Const(ref ty, _) | TraitItemKind::Type(_, Some(ref ty)) => check_ty(cx, ty, false), - TraitItemKind::Method(ref sig, _) => check_fn_decl(cx, &sig.decl), + TraitItemKind::Const(ref ty, _) | TraitItemKind::Type(_, Some(ref ty)) => self.check_ty(cx, ty, false), + TraitItemKind::Method(ref sig, _) => self.check_fn_decl(cx, &sig.decl), _ => (), } } fn check_local(&mut self, cx: &LateContext<'_, '_>, local: &Local<'_>) { if let Some(ref ty) = local.ty { - check_ty(cx, ty, true); + self.check_ty(cx, ty, true); } } } -fn check_fn_decl(cx: &LateContext<'_, '_>, decl: &FnDecl<'_>) { - for input in decl.inputs { - check_ty(cx, input, false); - } - - if let FunctionRetTy::Return(ref ty) = decl.output { - check_ty(cx, ty, false); - } -} - /// Checks if `qpath` has last segment with type parameter matching `path` fn match_type_parameter(cx: &LateContext<'_, '_>, qpath: &QPath<'_>, path: &[&str]) -> bool { let last = last_path_segment(qpath); @@ -238,54 +232,71 @@ fn match_type_parameter(cx: &LateContext<'_, '_>, qpath: &QPath<'_>, path: &[&st false } -/// Recursively check for `TypePass` lints in the given type. Stop at the first -/// lint found. -/// -/// The parameter `is_local` distinguishes the context of the type; types from -/// local bindings should only be checked for the `BORROWED_BOX` lint. -#[allow(clippy::too_many_lines)] -fn check_ty(cx: &LateContext<'_, '_>, hir_ty: &hir::Ty<'_>, is_local: bool) { - if hir_ty.span.from_expansion() { - return; +impl Types { + pub fn new(vec_box_size_threshold: u64) -> Self { + Self { vec_box_size_threshold } } - match hir_ty.kind { - TyKind::Path(ref qpath) if !is_local => { - let hir_id = hir_ty.hir_id; - let res = qpath_res(cx, qpath, hir_id); - if let Some(def_id) = res.opt_def_id() { - if Some(def_id) == cx.tcx.lang_items().owned_box() { - if match_type_parameter(cx, qpath, &paths::VEC) { - span_help_and_lint( - cx, - BOX_VEC, - hir_ty.span, - "you seem to be trying to use `Box>`. Consider using just `Vec`", - "`Vec` is already on the heap, `Box>` makes an extra allocation.", - ); - return; // don't recurse into the type - } - } else if cx.tcx.is_diagnostic_item(Symbol::intern("vec_type"), def_id) { - if_chain! { - // Get the _ part of Vec<_> - if let Some(ref last) = last_path_segment(qpath).args; - if let Some(ty) = last.args.iter().find_map(|arg| match arg { - GenericArg::Type(ty) => Some(ty), - _ => None, - }); - // ty is now _ at this point - if let TyKind::Path(ref ty_qpath) = ty.kind; - let res = qpath_res(cx, ty_qpath, ty.hir_id); - if let Some(def_id) = res.opt_def_id(); - if Some(def_id) == cx.tcx.lang_items().owned_box(); - // At this point, we know ty is Box, now get T - if let Some(ref last) = last_path_segment(ty_qpath).args; - if let Some(boxed_ty) = last.args.iter().find_map(|arg| match arg { - GenericArg::Type(ty) => Some(ty), - _ => None, - }); - then { + + fn check_fn_decl(&mut self, cx: &LateContext<'_, '_>, decl: &FnDecl<'_>) { + for input in decl.inputs { + self.check_ty(cx, input, false); + } + + if let FunctionRetTy::Return(ref ty) = decl.output { + self.check_ty(cx, ty, false); + } + } + + /// Recursively check for `TypePass` lints in the given type. Stop at the first + /// lint found. + /// + /// The parameter `is_local` distinguishes the context of the type; types from + /// local bindings should only be checked for the `BORROWED_BOX` lint. + #[allow(clippy::too_many_lines)] + fn check_ty(&mut self, cx: &LateContext<'_, '_>, hir_ty: &hir::Ty<'_>, is_local: bool) { + if hir_ty.span.from_expansion() { + return; + } + match hir_ty.kind { + TyKind::Path(ref qpath) if !is_local => { + let hir_id = hir_ty.hir_id; + let res = qpath_res(cx, qpath, hir_id); + if let Some(def_id) = res.opt_def_id() { + if Some(def_id) == cx.tcx.lang_items().owned_box() { + if match_type_parameter(cx, qpath, &paths::VEC) { + span_help_and_lint( + cx, + BOX_VEC, + hir_ty.span, + "you seem to be trying to use `Box>`. Consider using just `Vec`", + "`Vec` is already on the heap, `Box>` makes an extra allocation.", + ); + return; // don't recurse into the type + } + } else if cx.tcx.is_diagnostic_item(Symbol::intern("vec_type"), def_id) { + if_chain! { + // Get the _ part of Vec<_> + if let Some(ref last) = last_path_segment(qpath).args; + if let Some(ty) = last.args.iter().find_map(|arg| match arg { + GenericArg::Type(ty) => Some(ty), + _ => None, + }); + // ty is now _ at this point + if let TyKind::Path(ref ty_qpath) = ty.kind; + let res = qpath_res(cx, ty_qpath, ty.hir_id); + if let Some(def_id) = res.opt_def_id(); + if Some(def_id) == cx.tcx.lang_items().owned_box(); + // At this point, we know ty is Box, now get T + if let Some(ref last) = last_path_segment(ty_qpath).args; + if let Some(boxed_ty) = last.args.iter().find_map(|arg| match arg { + GenericArg::Type(ty) => Some(ty), + _ => None, + }); let ty_ty = hir_ty_to_ty(cx.tcx, boxed_ty); - if ty_ty.is_sized(cx.tcx.at(ty.span), cx.param_env) { + if ty_ty.is_sized(cx.tcx.at(ty.span), cx.param_env); + if let Ok(ty_ty_size) = cx.layout_of(ty_ty).map(|l| l.size.bytes()); + if ty_ty_size <= self.vec_box_size_threshold; + then { span_lint_and_sugg( cx, VEC_BOX, @@ -298,137 +309,144 @@ fn check_ty(cx: &LateContext<'_, '_>, hir_ty: &hir::Ty<'_>, is_local: bool) { return; // don't recurse into the type } } - } - } else if match_def_path(cx, def_id, &paths::OPTION) { - if match_type_parameter(cx, qpath, &paths::OPTION) { - span_lint( + } else if match_def_path(cx, def_id, &paths::OPTION) { + if match_type_parameter(cx, qpath, &paths::OPTION) { + span_lint( + cx, + OPTION_OPTION, + hir_ty.span, + "consider using `Option` instead of `Option>` or a custom \ + enum if you need to distinguish all 3 cases", + ); + return; // don't recurse into the type + } + } else if match_def_path(cx, def_id, &paths::LINKED_LIST) { + span_help_and_lint( cx, - OPTION_OPTION, + LINKEDLIST, hir_ty.span, - "consider using `Option` instead of `Option>` or a custom \ - enum if you need to distinguish all 3 cases", + "I see you're using a LinkedList! Perhaps you meant some other data structure?", + "a `VecDeque` might work", ); return; // don't recurse into the type } - } else if match_def_path(cx, def_id, &paths::LINKED_LIST) { - span_help_and_lint( - cx, - LINKEDLIST, - hir_ty.span, - "I see you're using a LinkedList! Perhaps you meant some other data structure?", - "a `VecDeque` might work", - ); - return; // don't recurse into the type } - } - match *qpath { - QPath::Resolved(Some(ref ty), ref p) => { - check_ty(cx, ty, is_local); - for ty in p.segments.iter().flat_map(|seg| { - seg.args - .as_ref() - .map_or_else(|| [].iter(), |params| params.args.iter()) - .filter_map(|arg| match arg { - GenericArg::Type(ty) => Some(ty), - _ => None, - }) - }) { - check_ty(cx, ty, is_local); - } - }, - QPath::Resolved(None, ref p) => { - for ty in p.segments.iter().flat_map(|seg| { - seg.args - .as_ref() - .map_or_else(|| [].iter(), |params| params.args.iter()) - .filter_map(|arg| match arg { - GenericArg::Type(ty) => Some(ty), - _ => None, - }) - }) { - check_ty(cx, ty, is_local); - } - }, - QPath::TypeRelative(ref ty, ref seg) => { - check_ty(cx, ty, is_local); - if let Some(ref params) = seg.args { - for ty in params.args.iter().filter_map(|arg| match arg { - GenericArg::Type(ty) => Some(ty), - _ => None, + match *qpath { + QPath::Resolved(Some(ref ty), ref p) => { + self.check_ty(cx, ty, is_local); + for ty in p.segments.iter().flat_map(|seg| { + seg.args + .as_ref() + .map_or_else(|| [].iter(), |params| params.args.iter()) + .filter_map(|arg| match arg { + GenericArg::Type(ty) => Some(ty), + _ => None, + }) }) { - check_ty(cx, ty, is_local); + self.check_ty(cx, ty, is_local); } - } - }, - } - }, - TyKind::Rptr(ref lt, ref mut_ty) => check_ty_rptr(cx, hir_ty, is_local, lt, mut_ty), - // recurse - TyKind::Slice(ref ty) | TyKind::Array(ref ty, _) | TyKind::Ptr(MutTy { ref ty, .. }) => { - check_ty(cx, ty, is_local) - }, - TyKind::Tup(tys) => { - for ty in tys { - check_ty(cx, ty, is_local); - } - }, - _ => {}, - } -} - -fn check_ty_rptr(cx: &LateContext<'_, '_>, hir_ty: &hir::Ty<'_>, is_local: bool, lt: &Lifetime, mut_ty: &MutTy<'_>) { - match mut_ty.ty.kind { - TyKind::Path(ref qpath) => { - let hir_id = mut_ty.ty.hir_id; - let def = qpath_res(cx, qpath, hir_id); - if_chain! { - if let Some(def_id) = def.opt_def_id(); - if Some(def_id) == cx.tcx.lang_items().owned_box(); - if let QPath::Resolved(None, ref path) = *qpath; - if let [ref bx] = *path.segments; - if let Some(ref params) = bx.args; - if !params.parenthesized; - if let Some(inner) = params.args.iter().find_map(|arg| match arg { - GenericArg::Type(ty) => Some(ty), - _ => None, - }); - then { - if is_any_trait(inner) { - // Ignore `Box` types; see issue #1884 for details. - return; - } - - let ltopt = if lt.is_elided() { - String::new() - } else { - format!("{} ", lt.name.ident().as_str()) - }; - let mutopt = if mut_ty.mutbl == Mutability::Mut { - "mut " - } else { - "" - }; - let mut applicability = Applicability::MachineApplicable; - span_lint_and_sugg( - cx, - BORROWED_BOX, - hir_ty.span, - "you seem to be trying to use `&Box`. Consider using just `&T`", - "try", - format!( - "&{}{}{}", - ltopt, - mutopt, - &snippet_with_applicability(cx, inner.span, "..", &mut applicability) - ), - Applicability::Unspecified, - ); - return; // don't recurse into the type + }, + QPath::Resolved(None, ref p) => { + for ty in p.segments.iter().flat_map(|seg| { + seg.args + .as_ref() + .map_or_else(|| [].iter(), |params| params.args.iter()) + .filter_map(|arg| match arg { + GenericArg::Type(ty) => Some(ty), + _ => None, + }) + }) { + self.check_ty(cx, ty, is_local); + } + }, + QPath::TypeRelative(ref ty, ref seg) => { + self.check_ty(cx, ty, is_local); + if let Some(ref params) = seg.args { + for ty in params.args.iter().filter_map(|arg| match arg { + GenericArg::Type(ty) => Some(ty), + _ => None, + }) { + self.check_ty(cx, ty, is_local); + } + } + }, } - }; - check_ty(cx, &mut_ty.ty, is_local); - }, - _ => check_ty(cx, &mut_ty.ty, is_local), + }, + TyKind::Rptr(ref lt, ref mut_ty) => self.check_ty_rptr(cx, hir_ty, is_local, lt, mut_ty), + // recurse + TyKind::Slice(ref ty) | TyKind::Array(ref ty, _) | TyKind::Ptr(MutTy { ref ty, .. }) => { + self.check_ty(cx, ty, is_local) + }, + TyKind::Tup(tys) => { + for ty in tys { + self.check_ty(cx, ty, is_local); + } + }, + _ => {}, + } + } + + fn check_ty_rptr( + &mut self, + cx: &LateContext<'_, '_>, + hir_ty: &hir::Ty<'_>, + is_local: bool, + lt: &Lifetime, + mut_ty: &MutTy<'_>, + ) { + match mut_ty.ty.kind { + TyKind::Path(ref qpath) => { + let hir_id = mut_ty.ty.hir_id; + let def = qpath_res(cx, qpath, hir_id); + if_chain! { + if let Some(def_id) = def.opt_def_id(); + if Some(def_id) == cx.tcx.lang_items().owned_box(); + if let QPath::Resolved(None, ref path) = *qpath; + if let [ref bx] = *path.segments; + if let Some(ref params) = bx.args; + if !params.parenthesized; + if let Some(inner) = params.args.iter().find_map(|arg| match arg { + GenericArg::Type(ty) => Some(ty), + _ => None, + }); + then { + if is_any_trait(inner) { + // Ignore `Box` types; see issue #1884 for details. + return; + } + + let ltopt = if lt.is_elided() { + String::new() + } else { + format!("{} ", lt.name.ident().as_str()) + }; + let mutopt = if mut_ty.mutbl == Mutability::Mut { + "mut " + } else { + "" + }; + let mut applicability = Applicability::MachineApplicable; + span_lint_and_sugg( + cx, + BORROWED_BOX, + hir_ty.span, + "you seem to be trying to use `&Box`. Consider using just `&T`", + "try", + format!( + "&{}{}{}", + ltopt, + mutopt, + &snippet_with_applicability(cx, inner.span, "..", &mut applicability) + ), + Applicability::Unspecified, + ); + return; // don't recurse into the type + } + }; + self.check_ty(cx, &mut_ty.ty, is_local); + }, + _ => self.check_ty(cx, &mut_ty.ty, is_local), + } } } diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs index 33435e6b253..2b7c6d8920c 100644 --- a/clippy_lints/src/utils/conf.rs +++ b/clippy_lints/src/utils/conf.rs @@ -152,6 +152,8 @@ define_Conf! { (too_many_lines_threshold, "too_many_lines_threshold", 100 => u64), /// Lint: LARGE_STACK_ARRAYS. The maximum allowed size for arrays on the stack (array_size_threshold, "array_size_threshold", 512_000 => u64), + /// Lint: VEC_BOX. The size of the boxed type in bytes, where boxing in a `Vec` is allowed + (vec_box_size_threshold, "vec_box_size_threshold", 4096 => u64), } impl Default for Conf { diff --git a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr index cbb4126a866..9eebc0bcf3f 100644 --- a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr +++ b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr @@ -1,4 +1,4 @@ -error: error reading Clippy's configuration file `$DIR/clippy.toml`: unknown field `foobar`, expected one of `blacklisted-names`, `cognitive-complexity-threshold`, `cyclomatic-complexity-threshold`, `doc-valid-idents`, `too-many-arguments-threshold`, `type-complexity-threshold`, `single-char-binding-names-threshold`, `too-large-for-stack`, `enum-variant-name-threshold`, `enum-variant-size-threshold`, `verbose-bit-mask-threshold`, `literal-representation-threshold`, `trivial-copy-size-limit`, `too-many-lines-threshold`, `array-size-threshold`, `third-party` at line 5 column 1 +error: error reading Clippy's configuration file `$DIR/clippy.toml`: unknown field `foobar`, expected one of `blacklisted-names`, `cognitive-complexity-threshold`, `cyclomatic-complexity-threshold`, `doc-valid-idents`, `too-many-arguments-threshold`, `type-complexity-threshold`, `single-char-binding-names-threshold`, `too-large-for-stack`, `enum-variant-name-threshold`, `enum-variant-size-threshold`, `verbose-bit-mask-threshold`, `literal-representation-threshold`, `trivial-copy-size-limit`, `too-many-lines-threshold`, `array-size-threshold`, `vec-box-size-threshold`, `third-party` at line 5 column 1 error: aborting due to previous error diff --git a/tests/ui-toml/vec_box_sized/clippy.toml b/tests/ui-toml/vec_box_sized/clippy.toml new file mode 100644 index 00000000000..039ea47fc32 --- /dev/null +++ b/tests/ui-toml/vec_box_sized/clippy.toml @@ -0,0 +1 @@ +vec-box-size-threshold = 4 diff --git a/tests/ui-toml/vec_box_sized/test.rs b/tests/ui-toml/vec_box_sized/test.rs new file mode 100644 index 00000000000..bf04bee1637 --- /dev/null +++ b/tests/ui-toml/vec_box_sized/test.rs @@ -0,0 +1,15 @@ +struct S { + x: u64, +} + +struct C { + y: u16, +} + +struct Foo(Vec>); +struct Bar(Vec>); +struct Baz(Vec>); +struct BarBaz(Vec>); +struct FooBarBaz(Vec>); + +fn main() {} diff --git a/tests/ui-toml/vec_box_sized/test.stderr b/tests/ui-toml/vec_box_sized/test.stderr new file mode 100644 index 00000000000..3bdeca0bc87 --- /dev/null +++ b/tests/ui-toml/vec_box_sized/test.stderr @@ -0,0 +1,22 @@ +error: `Vec` is already on the heap, the boxing is unnecessary. + --> $DIR/test.rs:9:12 + | +LL | struct Foo(Vec>); + | ^^^^^^^^^^^^ help: try: `Vec` + | + = note: `-D clippy::vec-box` implied by `-D warnings` + +error: `Vec` is already on the heap, the boxing is unnecessary. + --> $DIR/test.rs:10:12 + | +LL | struct Bar(Vec>); + | ^^^^^^^^^^^^^ help: try: `Vec` + +error: `Vec` is already on the heap, the boxing is unnecessary. + --> $DIR/test.rs:13:18 + | +LL | struct FooBarBaz(Vec>); + | ^^^^^^^^^^^ help: try: `Vec` + +error: aborting due to 3 previous errors + diff --git a/tests/ui/vec_box_sized.fixed b/tests/ui/vec_box_sized.fixed index a56dac8aa23..d0bee2460dd 100644 --- a/tests/ui/vec_box_sized.fixed +++ b/tests/ui/vec_box_sized.fixed @@ -4,6 +4,7 @@ struct SizedStruct(i32); struct UnsizedStruct([i32]); +struct BigStruct([i32; 10000]); /// The following should trigger the lint mod should_trigger { @@ -19,9 +20,10 @@ mod should_trigger { /// The following should not trigger the lint mod should_not_trigger { - use super::UnsizedStruct; + use super::{BigStruct, UnsizedStruct}; struct C(Vec>); + struct D(Vec>); struct StructWithVecBoxButItsUnsized { unsized_type: Vec>, diff --git a/tests/ui/vec_box_sized.rs b/tests/ui/vec_box_sized.rs index 32d1e940f27..500a0ae263e 100644 --- a/tests/ui/vec_box_sized.rs +++ b/tests/ui/vec_box_sized.rs @@ -4,6 +4,7 @@ struct SizedStruct(i32); struct UnsizedStruct([i32]); +struct BigStruct([i32; 10000]); /// The following should trigger the lint mod should_trigger { @@ -19,9 +20,10 @@ mod should_trigger { /// The following should not trigger the lint mod should_not_trigger { - use super::UnsizedStruct; + use super::{BigStruct, UnsizedStruct}; struct C(Vec>); + struct D(Vec>); struct StructWithVecBoxButItsUnsized { unsized_type: Vec>, diff --git a/tests/ui/vec_box_sized.stderr b/tests/ui/vec_box_sized.stderr index b33880b46bd..29bf7069e8a 100644 --- a/tests/ui/vec_box_sized.stderr +++ b/tests/ui/vec_box_sized.stderr @@ -1,5 +1,5 @@ error: `Vec` is already on the heap, the boxing is unnecessary. - --> $DIR/vec_box_sized.rs:13:21 + --> $DIR/vec_box_sized.rs:14:21 | LL | sized_type: Vec>, | ^^^^^^^^^^^^^^^^^^^^^ help: try: `Vec` @@ -7,13 +7,13 @@ LL | sized_type: Vec>, = note: `-D clippy::vec-box` implied by `-D warnings` error: `Vec` is already on the heap, the boxing is unnecessary. - --> $DIR/vec_box_sized.rs:16:14 + --> $DIR/vec_box_sized.rs:17:14 | LL | struct A(Vec>); | ^^^^^^^^^^^^^^^^^^^^^ help: try: `Vec` error: `Vec` is already on the heap, the boxing is unnecessary. - --> $DIR/vec_box_sized.rs:17:18 + --> $DIR/vec_box_sized.rs:18:18 | LL | struct B(Vec>>); | ^^^^^^^^^^^^^^^ help: try: `Vec`