From 711cad188abfc36e0a3d178ece2af41487ec5a45 Mon Sep 17 00:00:00 2001 From: scott-linder <scott.b.linder@wmich.edu> Date: Tue, 31 Jan 2017 23:19:33 -0500 Subject: [PATCH 01/13] check for borrowed box types --- CHANGELOG.md | 1 + README.md | 1 + clippy_lints/src/lib.rs | 1 + clippy_lints/src/types.rs | 39 +++++++++++++++++++++++++++++--- tests/compile-fail/borrow_box.rs | 14 ++++++++++++ 5 files changed, 53 insertions(+), 3 deletions(-) create mode 100644 tests/compile-fail/borrow_box.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 837f160f770..d2b5f6f2fe7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -364,6 +364,7 @@ All notable changes to this project will be documented in this file. [`block_in_if_condition_expr`]: https://github.com/Manishearth/rust-clippy/wiki#block_in_if_condition_expr [`block_in_if_condition_stmt`]: https://github.com/Manishearth/rust-clippy/wiki#block_in_if_condition_stmt [`bool_comparison`]: https://github.com/Manishearth/rust-clippy/wiki#bool_comparison +[`borrowed_box`]: https://github.com/Manishearth/rust-clippy/wiki#borrowed_box [`box_vec`]: https://github.com/Manishearth/rust-clippy/wiki#box_vec [`boxed_local`]: https://github.com/Manishearth/rust-clippy/wiki#boxed_local [`builtin_type_shadow`]: https://github.com/Manishearth/rust-clippy/wiki#builtin_type_shadow diff --git a/README.md b/README.md index c5b28812e94..1cd64745f12 100644 --- a/README.md +++ b/README.md @@ -194,6 +194,7 @@ name [block_in_if_condition_expr](https://github.com/Manishearth/rust-clippy/wiki#block_in_if_condition_expr) | warn | braces that can be eliminated in conditions, e.g. `if { true } ...` [block_in_if_condition_stmt](https://github.com/Manishearth/rust-clippy/wiki#block_in_if_condition_stmt) | warn | complex blocks in conditions, e.g. `if { let x = true; x } ...` [bool_comparison](https://github.com/Manishearth/rust-clippy/wiki#bool_comparison) | warn | comparing a variable to a boolean, e.g. `if x == true` +[borrowed_box](https://github.com/Manishearth/rust-clippy/wiki#borrowed_box) | warn | a borrow of a boxed type [box_vec](https://github.com/Manishearth/rust-clippy/wiki#box_vec) | warn | usage of `Box<Vec<T>>`, vector elements are already on the heap [boxed_local](https://github.com/Manishearth/rust-clippy/wiki#boxed_local) | warn | using `Box<T>` where unnecessary [builtin_type_shadow](https://github.com/Manishearth/rust-clippy/wiki#builtin_type_shadow) | warn | shadowing a builtin type diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index e212aaeaf7b..70b2e0c4385 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -506,6 +506,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { transmute::USELESS_TRANSMUTE, transmute::WRONG_TRANSMUTE, types::ABSURD_EXTREME_COMPARISONS, + types::BORROWED_BOX, types::BOX_VEC, types::CHAR_LIT_AS_U8, types::LET_UNIT_VALUE, diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index f608c7808a0..a073e7e9852 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -65,9 +65,25 @@ declare_lint! { structure like a VecDeque" } +/// **What it does:** Checks for use of `&Box<T>` anywhere in the code. +/// +/// **Why is this bad?** Any `&Box<T>` can also be a `&T`, which is more general. +/// +/// **Known problems:** None. +/// +/// **Example:** +/// ```rust +/// fn foo(bar: &Box<T>) { ... } +/// ``` +declare_lint! { + pub BORROWED_BOX, + Warn, + "a borrow of a boxed type" +} + impl LintPass for TypePass { fn get_lints(&self) -> LintArray { - lint_array!(BOX_VEC, LINKEDLIST) + lint_array!(BOX_VEC, LINKEDLIST, BORROWED_BOX) } } @@ -161,11 +177,28 @@ fn check_ty(cx: &LateContext, ast_ty: &hir::Ty) { }, } }, + TyRptr(_, MutTy { ref ty, .. }) => { + match ty.node { + TyPath(ref qpath) => { + let def = cx.tables.qpath_def(qpath, ast_ty.id); + if let Some(def_id) = opt_def_id(def) { + if Some(def_id) == cx.tcx.lang_items.owned_box() { + span_help_and_lint(cx, + BOX_VEC, + ast_ty.span, + "you seem to be trying to use `&Box<T>`. Consider using just `&T`", + "replace `&Box<T>` with simply `&T`"); + return; // don't recurse into the type + } + } + }, + _ => check_ty(cx, ty), + } + }, // recurse TySlice(ref ty) | TyArray(ref ty, _) | - TyPtr(MutTy { ref ty, .. }) | - TyRptr(_, MutTy { ref ty, .. }) => check_ty(cx, ty), + TyPtr(MutTy { ref ty, .. }) => check_ty(cx, ty), TyTup(ref tys) => { for ty in tys { check_ty(cx, ty); diff --git a/tests/compile-fail/borrow_box.rs b/tests/compile-fail/borrow_box.rs new file mode 100644 index 00000000000..54dee4f9016 --- /dev/null +++ b/tests/compile-fail/borrow_box.rs @@ -0,0 +1,14 @@ +#![feature(plugin)] +#![plugin(clippy)] + +#![deny(clippy)] +#![allow(boxed_local)] +#![allow(blacklisted_name)] + +pub fn test(foo: &Box<bool>) { //~ ERROR you seem to be trying to use `&Box<T>` + println!("{:?}", foo) +} + +fn main(){ + test(&Box::new(false)); +} From a4c4da1c4b4d4356fecf998ce7499c1a836573e2 Mon Sep 17 00:00:00 2001 From: scott-linder <scott.b.linder@wmich.edu> Date: Wed, 1 Feb 2017 11:17:17 -0500 Subject: [PATCH 02/13] Fix typo in types check --- clippy_lints/src/types.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index a073e7e9852..5918d76a5ff 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -142,7 +142,7 @@ fn check_ty(cx: &LateContext, ast_ty: &hir::Ty) { match_def_path(cx.tcx, did, &paths::VEC), ], { span_help_and_lint(cx, - BOX_VEC, + BORROWED_BOX, ast_ty.span, "you seem to be trying to use `Box<Vec<T>>`. Consider using just `Vec<T>`", "`Vec<T>` is already on the heap, `Box<Vec<T>>` makes an extra allocation."); From 80cb48ca1ae5bd4c146d0fdecbd7a87e7359d7ad Mon Sep 17 00:00:00 2001 From: scott-linder <scott.b.linder@wmich.edu> Date: Thu, 2 Feb 2017 08:36:40 -0500 Subject: [PATCH 03/13] Actually fix the lint applied --- clippy_lints/src/types.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index 5918d76a5ff..5751c5b0e54 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -142,7 +142,7 @@ fn check_ty(cx: &LateContext, ast_ty: &hir::Ty) { match_def_path(cx.tcx, did, &paths::VEC), ], { span_help_and_lint(cx, - BORROWED_BOX, + BOX_VEC, ast_ty.span, "you seem to be trying to use `Box<Vec<T>>`. Consider using just `Vec<T>`", "`Vec<T>` is already on the heap, `Box<Vec<T>>` makes an extra allocation."); @@ -184,7 +184,7 @@ fn check_ty(cx: &LateContext, ast_ty: &hir::Ty) { if let Some(def_id) = opt_def_id(def) { if Some(def_id) == cx.tcx.lang_items.owned_box() { span_help_and_lint(cx, - BOX_VEC, + BORROWED_BOX, ast_ty.span, "you seem to be trying to use `&Box<T>`. Consider using just `&T`", "replace `&Box<T>` with simply `&T`"); From e6eaa726e2aa07ca25b139f09c6f29ba71da58b7 Mon Sep 17 00:00:00 2001 From: scott-linder <scott.b.linder@wmich.edu> Date: Thu, 2 Feb 2017 09:29:03 -0500 Subject: [PATCH 04/13] Recurse into inner type when not `&Box<T>` --- clippy_lints/src/types.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index 5751c5b0e54..10cfa57891b 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -191,6 +191,7 @@ fn check_ty(cx: &LateContext, ast_ty: &hir::Ty) { return; // don't recurse into the type } } + check_ty(cx, ty); }, _ => check_ty(cx, ty), } From c061464f20b25684751948bae2c4f8d6df1de0cb Mon Sep 17 00:00:00 2001 From: scott-linder <scott.b.linder@wmich.edu> Date: Thu, 2 Feb 2017 10:55:27 -0500 Subject: [PATCH 05/13] Add more exhaustive tests for borrow_box --- tests/compile-fail/borrow_box.rs | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/tests/compile-fail/borrow_box.rs b/tests/compile-fail/borrow_box.rs index 54dee4f9016..9c1c36e6475 100644 --- a/tests/compile-fail/borrow_box.rs +++ b/tests/compile-fail/borrow_box.rs @@ -4,11 +4,22 @@ #![deny(clippy)] #![allow(boxed_local)] #![allow(blacklisted_name)] +#![allow(unused_variables)] +#![allow(dead_code)] -pub fn test(foo: &Box<bool>) { //~ ERROR you seem to be trying to use `&Box<T>` +pub fn test1(foo: &Box<bool>) { //~ ERROR you seem to be trying to use `&Box<T>` println!("{:?}", foo) } -fn main(){ - test(&Box::new(false)); +pub fn test2() { + let foo: &Box<bool>; //~ ERROR you seem to be trying to use `&Box<T>` +} + +struct Test3<'a> { + foo: &'a Box<bool> //~ ERROR you seem to be trying to use `&Box<T>` +} + +fn main(){ + test1(&Box::new(false)); + test2(); } From 663688f70db1e1c4df764de1c0d07037f1a96e9c Mon Sep 17 00:00:00 2001 From: scott-linder <scott.b.linder@wmich.edu> Date: Sat, 10 Jun 2017 23:50:57 -0400 Subject: [PATCH 06/13] Move old-style test to examples --- clippy_tests/examples/borrow_box.rs | 24 +++++++++++++++++++++ clippy_tests/examples/borrow_box.stderr | 28 +++++++++++++++++++++++++ tests/compile-fail/borrow_box.rs | 25 ---------------------- 3 files changed, 52 insertions(+), 25 deletions(-) create mode 100644 clippy_tests/examples/borrow_box.rs create mode 100644 clippy_tests/examples/borrow_box.stderr delete mode 100644 tests/compile-fail/borrow_box.rs diff --git a/clippy_tests/examples/borrow_box.rs b/clippy_tests/examples/borrow_box.rs new file mode 100644 index 00000000000..a72c664376f --- /dev/null +++ b/clippy_tests/examples/borrow_box.rs @@ -0,0 +1,24 @@ +#![feature(plugin)] +#![plugin(clippy)] + +#![deny(borrowed_box)] +#![allow(blacklisted_name)] +#![allow(unused_variables)] +#![allow(dead_code)] + +pub fn test1(foo: &mut Box<bool>) { + println!("{:?}", foo) +} + +pub fn test2() { + let foo: &Box<bool>; +} + +struct Test3<'a> { + foo: &'a Box<bool> +} + +fn main(){ + test1(&mut Box::new(false)); + test2(); +} diff --git a/clippy_tests/examples/borrow_box.stderr b/clippy_tests/examples/borrow_box.stderr new file mode 100644 index 00000000000..1b4b2c1efa6 --- /dev/null +++ b/clippy_tests/examples/borrow_box.stderr @@ -0,0 +1,28 @@ +error: you seem to be trying to use `&Box<T>`. Consider using just `&T` + --> borrow_box.rs:10:19 + | +10 | pub fn test1(foo: &Box<bool>) { //~ ERROR you seem to be trying to use `&Box<T>` + | ^^^^^^^^^^ + | + = note: #[deny(borrowed_box)] implied by #[deny(clippy)] +note: lint level defined here + --> borrow_box.rs:4:9 + | +4 | #![deny(clippy)] + | ^^^^^^ + = help: replace `&Box<T>` with simply `&T` + +error: you seem to be trying to use `&Box<T>`. Consider using just `&T` + --> borrow_box.rs:19:10 + | +19 | foo: &'a Box<bool> //~ ERROR you seem to be trying to use `&Box<T>` + | ^^^^^^^^^^^^^ + | + = note: #[deny(borrowed_box)] implied by #[deny(clippy)] + = help: replace `&Box<T>` with simply `&T` + +error: aborting due to previous error(s) + +error: Could not compile `clippy_tests`. + +To learn more, run the command again with --verbose. diff --git a/tests/compile-fail/borrow_box.rs b/tests/compile-fail/borrow_box.rs deleted file mode 100644 index 9c1c36e6475..00000000000 --- a/tests/compile-fail/borrow_box.rs +++ /dev/null @@ -1,25 +0,0 @@ -#![feature(plugin)] -#![plugin(clippy)] - -#![deny(clippy)] -#![allow(boxed_local)] -#![allow(blacklisted_name)] -#![allow(unused_variables)] -#![allow(dead_code)] - -pub fn test1(foo: &Box<bool>) { //~ ERROR you seem to be trying to use `&Box<T>` - println!("{:?}", foo) -} - -pub fn test2() { - let foo: &Box<bool>; //~ ERROR you seem to be trying to use `&Box<T>` -} - -struct Test3<'a> { - foo: &'a Box<bool> //~ ERROR you seem to be trying to use `&Box<T>` -} - -fn main(){ - test1(&Box::new(false)); - test2(); -} From deef81a3fc26169180729c4a8ddeb406d14acbc8 Mon Sep 17 00:00:00 2001 From: scott-linder <scott.b.linder@wmich.edu> Date: Sat, 10 Jun 2017 23:51:37 -0400 Subject: [PATCH 07/13] Use span_suggestion in borrowed_box lint --- clippy_lints/src/types.rs | 38 ++++++++++++++++++++++++++++++-------- 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index 10cfa57891b..85f4ea8b7b9 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -8,7 +8,7 @@ use std::cmp::Ordering; use syntax::ast::{IntTy, UintTy, FloatTy}; use syntax::attr::IntType; use syntax::codemap::Span; -use utils::{comparisons, higher, in_external_macro, in_macro, match_def_path, snippet, span_help_and_lint, span_lint, +use utils::{comparisons, higher, in_external_macro, in_macro, match_def_path, snippet, span_help_and_lint, span_lint, span_lint_and_then, opt_def_id, last_path_segment, type_size}; use utils::paths; @@ -177,18 +177,40 @@ fn check_ty(cx: &LateContext, ast_ty: &hir::Ty) { }, } }, - TyRptr(_, MutTy { ref ty, .. }) => { + TyRptr(ref lt, MutTy { ref ty, ref mutbl }) => { match ty.node { TyPath(ref qpath) => { let def = cx.tables.qpath_def(qpath, ast_ty.id); if let Some(def_id) = opt_def_id(def) { if Some(def_id) == cx.tcx.lang_items.owned_box() { - span_help_and_lint(cx, - BORROWED_BOX, - ast_ty.span, - "you seem to be trying to use `&Box<T>`. Consider using just `&T`", - "replace `&Box<T>` with simply `&T`"); - return; // don't recurse into the type + if_let_chain! {[ + let &QPath::Resolved(None, ref path) = qpath, + let [ref bx] = *path.segments, + let PathParameters::AngleBracketedParameters(ref ab_data) = bx.parameters, + let [ref inner] = *ab_data.types + ], { + let ltopt = if lt.is_elided() { + "".to_owned() + } else { + format!("{} ", lt.name.as_str()) + }; + let mutopt = if *mutbl == Mutability::MutMutable { + "mut " + } else { + "" + }; + span_lint_and_then(cx, + BORROWED_BOX, + ast_ty.span, + "you seem to be trying to use `&Box<T>`. Consider using just `&T`", + |db| { + db.span_suggestion(ast_ty.span, + "try", + format!("&{}{}{}", ltopt, mutopt, &snippet(cx, inner.span, ".."))); + } + ); + return; // don't recurse into the type + }}; } } check_ty(cx, ty); From c29f5ea83b7184b575bb23abac4b47553f807b54 Mon Sep 17 00:00:00 2001 From: scott-linder <scott.b.linder@wmich.edu> Date: Sun, 11 Jun 2017 10:49:34 -0400 Subject: [PATCH 08/13] Commit updated example stderr --- clippy_lints/src/types.rs | 2 +- clippy_tests/examples/borrow_box.stderr | 29 ++++++++++--------------- 2 files changed, 13 insertions(+), 18 deletions(-) diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index 85f4ea8b7b9..ef143222326 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -184,7 +184,7 @@ fn check_ty(cx: &LateContext, ast_ty: &hir::Ty) { if let Some(def_id) = opt_def_id(def) { if Some(def_id) == cx.tcx.lang_items.owned_box() { if_let_chain! {[ - let &QPath::Resolved(None, ref path) = qpath, + let QPath::Resolved(None, ref path) = *qpath, let [ref bx] = *path.segments, let PathParameters::AngleBracketedParameters(ref ab_data) = bx.parameters, let [ref inner] = *ab_data.types diff --git a/clippy_tests/examples/borrow_box.stderr b/clippy_tests/examples/borrow_box.stderr index 1b4b2c1efa6..1b354dd81df 100644 --- a/clippy_tests/examples/borrow_box.stderr +++ b/clippy_tests/examples/borrow_box.stderr @@ -1,25 +1,20 @@ error: you seem to be trying to use `&Box<T>`. Consider using just `&T` - --> borrow_box.rs:10:19 - | -10 | pub fn test1(foo: &Box<bool>) { //~ ERROR you seem to be trying to use `&Box<T>` - | ^^^^^^^^^^ - | - = note: #[deny(borrowed_box)] implied by #[deny(clippy)] + --> borrow_box.rs:9:19 + | +9 | pub fn test1(foo: &mut Box<bool>) { + | ^^^^^^^^^^^^^^ help: try `&mut bool` + | note: lint level defined here - --> borrow_box.rs:4:9 - | -4 | #![deny(clippy)] - | ^^^^^^ - = help: replace `&Box<T>` with simply `&T` + --> borrow_box.rs:4:9 + | +4 | #![deny(borrowed_box)] + | ^^^^^^^^^^^^ error: you seem to be trying to use `&Box<T>`. Consider using just `&T` - --> borrow_box.rs:19:10 + --> borrow_box.rs:18:10 | -19 | foo: &'a Box<bool> //~ ERROR you seem to be trying to use `&Box<T>` - | ^^^^^^^^^^^^^ - | - = note: #[deny(borrowed_box)] implied by #[deny(clippy)] - = help: replace `&Box<T>` with simply `&T` +18 | foo: &'a Box<bool> + | ^^^^^^^^^^^^^ help: try `&'a bool` error: aborting due to previous error(s) From 74ebe6e69e41ad3b3efaf9095fd45d63c3a76dd0 Mon Sep 17 00:00:00 2001 From: scott-linder <scott.b.linder@wmich.edu> Date: Sun, 11 Jun 2017 12:11:20 -0400 Subject: [PATCH 09/13] Add `check_local` to `TypePass` for `BORROWED_BOX` Adds a boolean flag to indicate whether the current type in `check_ty` is in a local declaration, as only the borrowed box lint should consider these types. --- clippy_lints/src/types.rs | 36 ++++++++++++++----------- clippy_tests/examples/borrow_box.stderr | 6 +++++ 2 files changed, 27 insertions(+), 15 deletions(-) diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index ef143222326..da4fe963c30 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -100,35 +100,41 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for TypePass { } fn check_struct_field(&mut self, cx: &LateContext, field: &StructField) { - check_ty(cx, &field.ty); + check_ty(cx, &field.ty, false); } fn check_trait_item(&mut self, cx: &LateContext, item: &TraitItem) { match item.node { TraitItemKind::Const(ref ty, _) | - TraitItemKind::Type(_, Some(ref ty)) => check_ty(cx, ty), + TraitItemKind::Type(_, Some(ref ty)) => check_ty(cx, ty, false), TraitItemKind::Method(ref sig, _) => 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); + } + } } fn check_fn_decl(cx: &LateContext, decl: &FnDecl) { for input in &decl.inputs { - check_ty(cx, input); + check_ty(cx, input, false); } if let FunctionRetTy::Return(ref ty) = decl.output { - check_ty(cx, ty); + check_ty(cx, ty, false); } } -fn check_ty(cx: &LateContext, ast_ty: &hir::Ty) { +fn check_ty(cx: &LateContext, ast_ty: &hir::Ty, is_local: bool) { if in_macro(ast_ty.span) { return; } match ast_ty.node { - TyPath(ref qpath) => { + TyPath(ref qpath) if !is_local => { let def = cx.tables.qpath_def(qpath, ast_ty.id); if let Some(def_id) = opt_def_id(def) { if Some(def_id) == cx.tcx.lang_items.owned_box() { @@ -159,20 +165,20 @@ fn check_ty(cx: &LateContext, ast_ty: &hir::Ty) { } match *qpath { QPath::Resolved(Some(ref ty), ref p) => { - check_ty(cx, ty); + check_ty(cx, ty, is_local); for ty in p.segments.iter().flat_map(|seg| seg.parameters.types()) { - check_ty(cx, ty); + check_ty(cx, ty, is_local); } }, QPath::Resolved(None, ref p) => { for ty in p.segments.iter().flat_map(|seg| seg.parameters.types()) { - check_ty(cx, ty); + check_ty(cx, ty, is_local); } }, QPath::TypeRelative(ref ty, ref seg) => { - check_ty(cx, ty); + check_ty(cx, ty, is_local); for ty in seg.parameters.types() { - check_ty(cx, ty); + check_ty(cx, ty, is_local); } }, } @@ -213,18 +219,18 @@ fn check_ty(cx: &LateContext, ast_ty: &hir::Ty) { }}; } } - check_ty(cx, ty); + check_ty(cx, ty, is_local); }, - _ => check_ty(cx, ty), + _ => check_ty(cx, ty, is_local), } }, // recurse TySlice(ref ty) | TyArray(ref ty, _) | - TyPtr(MutTy { ref ty, .. }) => check_ty(cx, ty), + TyPtr(MutTy { ref ty, .. }) => check_ty(cx, ty, is_local), TyTup(ref tys) => { for ty in tys { - check_ty(cx, ty); + check_ty(cx, ty, is_local); } }, _ => {}, diff --git a/clippy_tests/examples/borrow_box.stderr b/clippy_tests/examples/borrow_box.stderr index 1b354dd81df..11838a0ec1c 100644 --- a/clippy_tests/examples/borrow_box.stderr +++ b/clippy_tests/examples/borrow_box.stderr @@ -10,6 +10,12 @@ note: lint level defined here 4 | #![deny(borrowed_box)] | ^^^^^^^^^^^^ +error: you seem to be trying to use `&Box<T>`. Consider using just `&T` + --> borrow_box.rs:14:14 + | +14 | let foo: &Box<bool>; + | ^^^^^^^^^^ help: try `&bool` + error: you seem to be trying to use `&Box<T>`. Consider using just `&T` --> borrow_box.rs:18:10 | From 54b52054c98e641b18e3043fff3f6c6deaa001bb Mon Sep 17 00:00:00 2001 From: scott-linder <scott.b.linder@wmich.edu> Date: Sun, 11 Jun 2017 12:30:48 -0400 Subject: [PATCH 10/13] Test for local types in `LINKEDLIST` and `BOX_VEC` Add negative tests for types in local declarations in the `LINKEDLIST` and `BOX_VEC` lints. They share a pass with `BORROWED_BOX` which does check local delclarations. --- clippy_tests/examples/box_vec.rs | 5 +++++ clippy_tests/examples/dlist.rs | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/clippy_tests/examples/box_vec.rs b/clippy_tests/examples/box_vec.rs index 507d9e77da0..f8c5a80c59d 100644 --- a/clippy_tests/examples/box_vec.rs +++ b/clippy_tests/examples/box_vec.rs @@ -22,8 +22,13 @@ pub fn test2(foo: Box<Fn(Vec<u32>)>) { // pass if #31 is fixed foo(vec![1, 2, 3]) } +pub fn test_local_not_linted() { + let _: Box<Vec<bool>>; +} + fn main(){ test(Box::new(Vec::new())); test2(Box::new(|v| println!("{:?}", v))); test_macro(); + test_local_not_linted(); } diff --git a/clippy_tests/examples/dlist.rs b/clippy_tests/examples/dlist.rs index a41bf1b52e0..b23aeb70a63 100644 --- a/clippy_tests/examples/dlist.rs +++ b/clippy_tests/examples/dlist.rs @@ -34,6 +34,11 @@ pub fn test_ret() -> Option<LinkedList<u8>> { unimplemented!(); } +pub fn test_local_not_linted() { + let _: LinkedList<u8>; +} + fn main(){ test(LinkedList::new()); + test_local_not_linted(); } From 49bba315e4adea34617193ec51f89c3416b95b6c Mon Sep 17 00:00:00 2001 From: scott-linder <scott.b.linder@wmich.edu> Date: Mon, 12 Jun 2017 08:38:29 -0400 Subject: [PATCH 11/13] Merge nested `if` into adjacent `if_let_chain!` --- clippy_lints/src/types.rs | 62 +++++++++++++++++++-------------------- 1 file changed, 30 insertions(+), 32 deletions(-) diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index da4fe963c30..b3d2a374865 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -187,38 +187,36 @@ fn check_ty(cx: &LateContext, ast_ty: &hir::Ty, is_local: bool) { match ty.node { TyPath(ref qpath) => { let def = cx.tables.qpath_def(qpath, ast_ty.id); - if let Some(def_id) = opt_def_id(def) { - if Some(def_id) == cx.tcx.lang_items.owned_box() { - if_let_chain! {[ - let QPath::Resolved(None, ref path) = *qpath, - let [ref bx] = *path.segments, - let PathParameters::AngleBracketedParameters(ref ab_data) = bx.parameters, - let [ref inner] = *ab_data.types - ], { - let ltopt = if lt.is_elided() { - "".to_owned() - } else { - format!("{} ", lt.name.as_str()) - }; - let mutopt = if *mutbl == Mutability::MutMutable { - "mut " - } else { - "" - }; - span_lint_and_then(cx, - BORROWED_BOX, - ast_ty.span, - "you seem to be trying to use `&Box<T>`. Consider using just `&T`", - |db| { - db.span_suggestion(ast_ty.span, - "try", - format!("&{}{}{}", ltopt, mutopt, &snippet(cx, inner.span, ".."))); - } - ); - return; // don't recurse into the type - }}; - } - } + if_let_chain! {[ + let Some(def_id) = opt_def_id(def), + Some(def_id) == cx.tcx.lang_items.owned_box(), + let QPath::Resolved(None, ref path) = *qpath, + let [ref bx] = *path.segments, + let PathParameters::AngleBracketedParameters(ref ab_data) = bx.parameters, + let [ref inner] = *ab_data.types + ], { + let ltopt = if lt.is_elided() { + "".to_owned() + } else { + format!("{} ", lt.name.as_str()) + }; + let mutopt = if *mutbl == Mutability::MutMutable { + "mut " + } else { + "" + }; + span_lint_and_then(cx, + BORROWED_BOX, + ast_ty.span, + "you seem to be trying to use `&Box<T>`. Consider using just `&T`", + |db| { + db.span_suggestion(ast_ty.span, + "try", + format!("&{}{}{}", ltopt, mutopt, &snippet(cx, inner.span, ".."))); + } + ); + return; // don't recurse into the type + }}; check_ty(cx, ty, is_local); }, _ => check_ty(cx, ty, is_local), From 1a50755f214b6fe9322878375ca60199f1bf3b51 Mon Sep 17 00:00:00 2001 From: scott-linder <scott.b.linder@wmich.edu> Date: Mon, 12 Jun 2017 08:43:02 -0400 Subject: [PATCH 12/13] Document `check_ty` and its new `is_local` arg. --- clippy_lints/src/types.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index b3d2a374865..0204bfff0d4 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -129,6 +129,11 @@ fn check_fn_decl(cx: &LateContext, decl: &FnDecl) { } } +/// 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. fn check_ty(cx: &LateContext, ast_ty: &hir::Ty, is_local: bool) { if in_macro(ast_ty.span) { return; From 5db8647c5e32bc93255d1edea279eec9b1c29c62 Mon Sep 17 00:00:00 2001 From: scott-linder <scott.b.linder@wmich.edu> Date: Mon, 12 Jun 2017 08:44:08 -0400 Subject: [PATCH 13/13] Test for trait method decl/impl for borrowed box. --- clippy_tests/examples/borrow_box.rs | 10 ++++++++++ clippy_tests/examples/borrow_box.stderr | 6 ++++++ 2 files changed, 16 insertions(+) diff --git a/clippy_tests/examples/borrow_box.rs b/clippy_tests/examples/borrow_box.rs index a72c664376f..ef569ab037f 100644 --- a/clippy_tests/examples/borrow_box.rs +++ b/clippy_tests/examples/borrow_box.rs @@ -18,6 +18,16 @@ struct Test3<'a> { foo: &'a Box<bool> } +trait Test4 { + fn test4(a: &Box<bool>); +} + +impl<'a> Test4 for Test3<'a> { + fn test4(a: &Box<bool>) { + unimplemented!(); + } +} + fn main(){ test1(&mut Box::new(false)); test2(); diff --git a/clippy_tests/examples/borrow_box.stderr b/clippy_tests/examples/borrow_box.stderr index 11838a0ec1c..6670a8c4083 100644 --- a/clippy_tests/examples/borrow_box.stderr +++ b/clippy_tests/examples/borrow_box.stderr @@ -22,6 +22,12 @@ error: you seem to be trying to use `&Box<T>`. Consider using just `&T` 18 | foo: &'a Box<bool> | ^^^^^^^^^^^^^ help: try `&'a bool` +error: you seem to be trying to use `&Box<T>`. Consider using just `&T` + --> borrow_box.rs:22:17 + | +22 | fn test4(a: &Box<bool>); + | ^^^^^^^^^^ help: try `&bool` + error: aborting due to previous error(s) error: Could not compile `clippy_tests`.