From a35a8b29183bf7caad56f513366853a682916c6e Mon Sep 17 00:00:00 2001 From: Jacek Pospychala Date: Thu, 12 Mar 2020 21:41:13 +0100 Subject: [PATCH] box_vec: dont use Box<&T> --- CHANGELOG.md | 1 + README.md | 2 +- clippy_lints/src/lib.rs | 3 +++ clippy_lints/src/types.rs | 51 ++++++++++++++++++++++++++++++++++++- src/lintlist/mod.rs | 9 ++++++- tests/ui/box_borrows.rs | 30 ++++++++++++++++++++++ tests/ui/box_borrows.stderr | 51 +++++++++++++++++++++++++++++++++++++ 7 files changed, 144 insertions(+), 3 deletions(-) create mode 100644 tests/ui/box_borrows.rs create mode 100644 tests/ui/box_borrows.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index c8d41e3b31d2..d5212d325356 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1166,6 +1166,7 @@ Released 2018-09-13 [`bool_comparison`]: https://rust-lang.github.io/rust-clippy/master/index.html#bool_comparison [`borrow_interior_mutable_const`]: https://rust-lang.github.io/rust-clippy/master/index.html#borrow_interior_mutable_const [`borrowed_box`]: https://rust-lang.github.io/rust-clippy/master/index.html#borrowed_box +[`box_borrows`]: https://rust-lang.github.io/rust-clippy/master/index.html#box_borrows [`box_vec`]: https://rust-lang.github.io/rust-clippy/master/index.html#box_vec [`boxed_local`]: https://rust-lang.github.io/rust-clippy/master/index.html#boxed_local [`builtin_type_shadow`]: https://rust-lang.github.io/rust-clippy/master/index.html#builtin_type_shadow diff --git a/README.md b/README.md index 2181c296e9b2..7d6fcbc90986 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 361 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 362 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index fb79ad271e0a..e0b911ccf1da 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -788,6 +788,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &try_err::TRY_ERR, &types::ABSURD_EXTREME_COMPARISONS, &types::BORROWED_BOX, + &types::BOX_BORROWS, &types::BOX_VEC, &types::CAST_LOSSLESS, &types::CAST_POSSIBLE_TRUNCATION, @@ -1358,6 +1359,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&try_err::TRY_ERR), LintId::of(&types::ABSURD_EXTREME_COMPARISONS), LintId::of(&types::BORROWED_BOX), + LintId::of(&types::BOX_BORROWS), LintId::of(&types::BOX_VEC), LintId::of(&types::CAST_PTR_ALIGNMENT), LintId::of(&types::CAST_REF_TO_MUT), @@ -1650,6 +1652,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&redundant_clone::REDUNDANT_CLONE), LintId::of(&slow_vector_initialization::SLOW_VECTOR_INITIALIZATION), LintId::of(&trivially_copy_pass_by_ref::TRIVIALLY_COPY_PASS_BY_REF), + LintId::of(&types::BOX_BORROWS), LintId::of(&types::BOX_VEC), LintId::of(&vec::USELESS_VEC), ]); diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index 26d425cf2416..34cbcc8f1a70 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -171,11 +171,38 @@ declare_clippy_lint! { "a borrow of a boxed type" } +declare_clippy_lint! { + /// **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 + /// struct X { + /// values: Box<&T>, + /// } + /// ``` + /// + /// Better: + /// + /// ```rust + /// struct X { + /// values: &T, + /// } + /// ``` + pub BOX_BORROWS, + perf, + "a box of borrowed type" +} + pub struct Types { vec_box_size_threshold: u64, } -impl_lint_pass!(Types => [BOX_VEC, VEC_BOX, OPTION_OPTION, LINKEDLIST, BORROWED_BOX]); +impl_lint_pass!(Types => [BOX_VEC, VEC_BOX, OPTION_OPTION, LINKEDLIST, BORROWED_BOX, BOX_BORROWS]); impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Types { fn check_fn( @@ -257,6 +284,7 @@ impl Types { /// 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)] + #[allow(clippy::cognitive_complexity)] fn check_ty(&mut self, cx: &LateContext<'_, '_>, hir_ty: &hir::Ty<'_>, is_local: bool) { if hir_ty.span.from_expansion() { return; @@ -267,6 +295,27 @@ impl Types { 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_chain! { + let last = last_path_segment(qpath); + if let Some(ref params) = last.args; + if !params.parenthesized; + if let Some(ty) = params.args.iter().find_map(|arg| match arg { + GenericArg::Type(ty) => Some(ty), + _ => None, + }); + if let TyKind::Rptr(..) = ty.kind; + let ty_ty = hir_ty_to_ty(cx.tcx, ty); + then { + span_lint_and_help( + cx, + BOX_BORROWS, + hir_ty.span, + "usage of `Box<&T>`", + format!("try `{}`", ty_ty).as_str(), + ); + return; // don't recurse into the type + } + } if match_type_parameter(cx, qpath, &paths::VEC) { span_lint_and_help( cx, diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index fd948953ea23..a5246d2c73a6 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -6,7 +6,7 @@ pub use lint::Lint; pub use lint::LINT_LEVELS; // begin lint list, do not remove this comment, it’s used in `update_lints` -pub const ALL_LINTS: [Lint; 361] = [ +pub const ALL_LINTS: [Lint; 362] = [ Lint { name: "absurd_extreme_comparisons", group: "correctness", @@ -98,6 +98,13 @@ pub const ALL_LINTS: [Lint; 361] = [ deprecation: None, module: "types", }, + Lint { + name: "box_borrows", + group: "perf", + desc: "a box of borrowed type", + deprecation: None, + module: "types", + }, Lint { name: "box_vec", group: "perf", diff --git a/tests/ui/box_borrows.rs b/tests/ui/box_borrows.rs new file mode 100644 index 000000000000..92e448372f77 --- /dev/null +++ b/tests/ui/box_borrows.rs @@ -0,0 +1,30 @@ +#![warn(clippy::all)] +#![allow(clippy::boxed_local, clippy::needless_pass_by_value)] +#![allow(clippy::blacklisted_name)] + +pub struct MyStruct {} + +pub struct SubT { + foo: T, +} + +pub enum MyEnum { + One, + Two, +} + +pub fn test(foo: Box<&T>) {} + +pub fn test1(foo: Box<&usize>) {} + +pub fn test2(foo: Box<&MyStruct>) {} + +pub fn test3(foo: Box<&MyEnum>) {} + +pub fn test4(foo: Box<&&MyEnum>) {} + +pub fn test5(foo: Box>) {} + +pub fn test6(foo: Box>) {} + +fn main() {} diff --git a/tests/ui/box_borrows.stderr b/tests/ui/box_borrows.stderr new file mode 100644 index 000000000000..1fe9a47e59b7 --- /dev/null +++ b/tests/ui/box_borrows.stderr @@ -0,0 +1,51 @@ +error: usage of `Box<&T>` + --> $DIR/box_borrows.rs:16:21 + | +LL | pub fn test(foo: Box<&T>) {} + | ^^^^^^^ + | + = note: `-D clippy::box-borrows` implied by `-D warnings` + = help: try `&T` + +error: usage of `Box<&T>` + --> $DIR/box_borrows.rs:18:19 + | +LL | pub fn test1(foo: Box<&usize>) {} + | ^^^^^^^^^^^ + | + = help: try `&usize` + +error: usage of `Box<&T>` + --> $DIR/box_borrows.rs:20:19 + | +LL | pub fn test2(foo: Box<&MyStruct>) {} + | ^^^^^^^^^^^^^^ + | + = help: try `&MyStruct` + +error: usage of `Box<&T>` + --> $DIR/box_borrows.rs:22:19 + | +LL | pub fn test3(foo: Box<&MyEnum>) {} + | ^^^^^^^^^^^^ + | + = help: try `&MyEnum` + +error: usage of `Box<&T>` + --> $DIR/box_borrows.rs:24:19 + | +LL | pub fn test4(foo: Box<&&MyEnum>) {} + | ^^^^^^^^^^^^^ + | + = help: try `&&MyEnum` + +error: usage of `Box<&T>` + --> $DIR/box_borrows.rs:26:23 + | +LL | pub fn test5(foo: Box>) {} + | ^^^^^^^^^^^ + | + = help: try `&usize` + +error: aborting due to 6 previous errors +