From 0dcf54a21ddf7f6b4fd66b075eeb6a27cddd2d40 Mon Sep 17 00:00:00 2001 From: Jacek Pospychala Date: Thu, 12 Mar 2020 21:41:13 +0100 Subject: [PATCH] useless Rc>, Rc>, Rc<&T>, Box<&T> --- CHANGELOG.md | 1 + clippy_lints/src/lib.rs | 3 + clippy_lints/src/types.rs | 103 +++++++++++++++++++++++++-- clippy_lints/src/utils/paths.rs | 1 + src/lintlist/mod.rs | 7 ++ tests/ui/must_use_candidates.fixed | 2 +- tests/ui/must_use_candidates.rs | 2 +- tests/ui/redundant_allocation.fixed | 48 +++++++++++++ tests/ui/redundant_allocation.rs | 48 +++++++++++++ tests/ui/redundant_allocation.stderr | 52 ++++++++++++++ 10 files changed, 259 insertions(+), 8 deletions(-) create mode 100644 tests/ui/redundant_allocation.fixed create mode 100644 tests/ui/redundant_allocation.rs create mode 100644 tests/ui/redundant_allocation.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index f3b1073988b0..894aab21fb37 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1433,6 +1433,7 @@ Released 2018-09-13 [`range_plus_one`]: https://rust-lang.github.io/rust-clippy/master/index.html#range_plus_one [`range_step_by_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#range_step_by_zero [`range_zip_with_len`]: https://rust-lang.github.io/rust-clippy/master/index.html#range_zip_with_len +[`redundant_allocation`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_allocation [`redundant_clone`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_clone [`redundant_closure`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure [`redundant_closure_call`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure_call diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 5d5b5d9a6da5..adebefcd6c55 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -811,6 +811,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &types::LET_UNIT_VALUE, &types::LINKEDLIST, &types::OPTION_OPTION, + &types::REDUNDANT_ALLOCATION, &types::TYPE_COMPLEXITY, &types::UNIT_ARG, &types::UNIT_CMP, @@ -1376,6 +1377,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&types::IMPLICIT_HASHER), LintId::of(&types::LET_UNIT_VALUE), LintId::of(&types::OPTION_OPTION), + LintId::of(&types::REDUNDANT_ALLOCATION), LintId::of(&types::TYPE_COMPLEXITY), LintId::of(&types::UNIT_ARG), LintId::of(&types::UNIT_CMP), @@ -1661,6 +1663,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&slow_vector_initialization::SLOW_VECTOR_INITIALIZATION), LintId::of(&trivially_copy_pass_by_ref::TRIVIALLY_COPY_PASS_BY_REF), LintId::of(&types::BOX_VEC), + LintId::of(&types::REDUNDANT_ALLOCATION), LintId::of(&vec::USELESS_VEC), ]); diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index 4ff2947378f8..530c3e363201 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -171,11 +171,35 @@ declare_clippy_lint! { "a borrow of a boxed type" } +declare_clippy_lint! { + /// **What it does:** Checks for use of redundant allocations anywhere in the code. + /// + /// **Why is this bad?** Expressions such as `Rc<&T>`, `Rc>`, `Rc>`, `Box<&T>` + /// add an unnecessary level of indirection. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// ```rust + /// # use std::rc::Rc; + /// fn foo(bar: Rc<&usize>) {} + /// ``` + /// + /// Better: + /// + /// ```rust + /// fn foo(bar: &usize) {} + /// ``` + pub REDUNDANT_ALLOCATION, + perf, + "redundant allocation" +} + 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, REDUNDANT_ALLOCATION]); impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Types { fn check_fn( @@ -217,7 +241,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Types { } /// Checks if `qpath` has last segment with type parameter matching `path` -fn match_type_parameter(cx: &LateContext<'_, '_>, qpath: &QPath<'_>, path: &[&str]) -> bool { +fn match_type_parameter(cx: &LateContext<'_, '_>, qpath: &QPath<'_>, path: &[&str]) -> Option { let last = last_path_segment(qpath); if_chain! { if let Some(ref params) = last.args; @@ -230,10 +254,27 @@ fn match_type_parameter(cx: &LateContext<'_, '_>, qpath: &QPath<'_>, path: &[&st if let Some(did) = qpath_res(cx, qpath, ty.hir_id).opt_def_id(); if match_def_path(cx, did, path); then { - return true; + return Some(ty.span); } } - false + None +} + +fn match_borrows_parameter(_cx: &LateContext<'_, '_>, qpath: &QPath<'_>) -> Option { + let last = last_path_segment(qpath); + if_chain! { + 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; + then { + return Some(ty.span); + } + } + None } impl Types { @@ -257,6 +298,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,7 +309,19 @@ 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 match_type_parameter(cx, qpath, &paths::VEC) { + if let Some(span) = match_borrows_parameter(cx, qpath) { + span_lint_and_sugg( + cx, + REDUNDANT_ALLOCATION, + hir_ty.span, + "usage of `Box<&T>`", + "try", + snippet(cx, span, "..").to_string(), + Applicability::MachineApplicable, + ); + return; // don't recurse into the type + } + if match_type_parameter(cx, qpath, &paths::VEC).is_some() { span_lint_and_help( cx, BOX_VEC, @@ -277,6 +331,43 @@ impl Types { ); return; // don't recurse into the type } + } else if Some(def_id) == cx.tcx.lang_items().rc() { + if let Some(span) = match_type_parameter(cx, qpath, &paths::RC) { + span_lint_and_sugg( + cx, + REDUNDANT_ALLOCATION, + hir_ty.span, + "usage of `Rc>`", + "try", + snippet(cx, span, "..").to_string(), + Applicability::MachineApplicable, + ); + return; // don't recurse into the type + } + if let Some(span) = match_type_parameter(cx, qpath, &paths::BOX) { + span_lint_and_sugg( + cx, + REDUNDANT_ALLOCATION, + hir_ty.span, + "usage of `Rc>`", + "try", + snippet(cx, span, "..").to_string(), + Applicability::MachineApplicable, + ); + return; // don't recurse into the type + } + if let Some(span) = match_borrows_parameter(cx, qpath) { + span_lint_and_sugg( + cx, + REDUNDANT_ALLOCATION, + hir_ty.span, + "usage of `Rc<&T>`", + "try", + snippet(cx, span, "..").to_string(), + Applicability::MachineApplicable, + ); + 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<_> @@ -314,7 +405,7 @@ impl Types { } } } else if match_def_path(cx, def_id, &paths::OPTION) { - if match_type_parameter(cx, qpath, &paths::OPTION) { + if let Some(_) = match_type_parameter(cx, qpath, &paths::OPTION) { span_lint( cx, OPTION_OPTION, diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index d443d63cc186..b79ba345df4f 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/clippy_lints/src/utils/paths.rs @@ -10,6 +10,7 @@ pub const BEGIN_PANIC: [&str; 3] = ["std", "panicking", "begin_panic"]; pub const BEGIN_PANIC_FMT: [&str; 3] = ["std", "panicking", "begin_panic_fmt"]; pub const BINARY_HEAP: [&str; 4] = ["alloc", "collections", "binary_heap", "BinaryHeap"]; pub const BORROW_TRAIT: [&str; 3] = ["core", "borrow", "Borrow"]; +pub const BOX: [&str; 3] = ["alloc", "boxed", "Box"]; pub const BTREEMAP: [&str; 5] = ["alloc", "collections", "btree", "map", "BTreeMap"]; pub const BTREEMAP_ENTRY: [&str; 5] = ["alloc", "collections", "btree", "map", "Entry"]; pub const BTREESET: [&str; 5] = ["alloc", "collections", "btree", "set", "BTreeSet"]; diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 3b89f5d19477..464deea5a4ca 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -1725,6 +1725,13 @@ pub static ref ALL_LINTS: Vec = vec![ deprecation: None, module: "ranges", }, + Lint { + name: "redundant_allocation", + group: "perf", + desc: "redundant allocation", + deprecation: None, + module: "types", + }, Lint { name: "redundant_clone", group: "perf", diff --git a/tests/ui/must_use_candidates.fixed b/tests/ui/must_use_candidates.fixed index e2ceb8baded2..9556f6f82cc6 100644 --- a/tests/ui/must_use_candidates.fixed +++ b/tests/ui/must_use_candidates.fixed @@ -1,6 +1,6 @@ // run-rustfix #![feature(never_type)] -#![allow(unused_mut)] +#![allow(unused_mut, clippy::redundant_allocation)] #![warn(clippy::must_use_candidate)] use std::rc::Rc; use std::sync::atomic::{AtomicBool, Ordering}; diff --git a/tests/ui/must_use_candidates.rs b/tests/ui/must_use_candidates.rs index 29ef8d1ed9c2..373242201710 100644 --- a/tests/ui/must_use_candidates.rs +++ b/tests/ui/must_use_candidates.rs @@ -1,6 +1,6 @@ // run-rustfix #![feature(never_type)] -#![allow(unused_mut)] +#![allow(unused_mut, clippy::redundant_allocation)] #![warn(clippy::must_use_candidate)] use std::rc::Rc; use std::sync::atomic::{AtomicBool, Ordering}; diff --git a/tests/ui/redundant_allocation.fixed b/tests/ui/redundant_allocation.fixed new file mode 100644 index 000000000000..266358334587 --- /dev/null +++ b/tests/ui/redundant_allocation.fixed @@ -0,0 +1,48 @@ +// run-rustfix +#![warn(clippy::all)] +#![allow(clippy::boxed_local, clippy::needless_pass_by_value)] +#![allow(clippy::blacklisted_name, unused_variables, dead_code)] + +use std::boxed::Box; +use std::rc::Rc; + +pub struct MyStruct {} + +pub struct SubT { + foo: T, +} + +pub enum MyEnum { + One, + Two, +} + +// Rc<&T> + +pub fn test1(foo: &T) {} + +pub fn test2(foo: &MyStruct) {} + +pub fn test3(foo: &MyEnum) {} + +pub fn test4_neg(foo: Rc>) {} + +// Rc> + +pub fn test5(a: Rc) {} + +// Rc> + +pub fn test6(a: Box) {} + +// Box<&T> + +pub fn test7(foo: &T) {} + +pub fn test8(foo: &MyStruct) {} + +pub fn test9(foo: &MyEnum) {} + +pub fn test10_neg(foo: Box>) {} + +fn main() {} diff --git a/tests/ui/redundant_allocation.rs b/tests/ui/redundant_allocation.rs new file mode 100644 index 000000000000..677b3e56d4dc --- /dev/null +++ b/tests/ui/redundant_allocation.rs @@ -0,0 +1,48 @@ +// run-rustfix +#![warn(clippy::all)] +#![allow(clippy::boxed_local, clippy::needless_pass_by_value)] +#![allow(clippy::blacklisted_name, unused_variables, dead_code)] + +use std::boxed::Box; +use std::rc::Rc; + +pub struct MyStruct {} + +pub struct SubT { + foo: T, +} + +pub enum MyEnum { + One, + Two, +} + +// Rc<&T> + +pub fn test1(foo: Rc<&T>) {} + +pub fn test2(foo: Rc<&MyStruct>) {} + +pub fn test3(foo: Rc<&MyEnum>) {} + +pub fn test4_neg(foo: Rc>) {} + +// Rc> + +pub fn test5(a: Rc>) {} + +// Rc> + +pub fn test6(a: Rc>) {} + +// Box<&T> + +pub fn test7(foo: Box<&T>) {} + +pub fn test8(foo: Box<&MyStruct>) {} + +pub fn test9(foo: Box<&MyEnum>) {} + +pub fn test10_neg(foo: Box>) {} + +fn main() {} diff --git a/tests/ui/redundant_allocation.stderr b/tests/ui/redundant_allocation.stderr new file mode 100644 index 000000000000..eaa57ce3024b --- /dev/null +++ b/tests/ui/redundant_allocation.stderr @@ -0,0 +1,52 @@ +error: usage of `Rc<&T>` + --> $DIR/redundant_allocation.rs:22:22 + | +LL | pub fn test1(foo: Rc<&T>) {} + | ^^^^^^ help: try: `&T` + | + = note: `-D clippy::redundant-allocation` implied by `-D warnings` + +error: usage of `Rc<&T>` + --> $DIR/redundant_allocation.rs:24:19 + | +LL | pub fn test2(foo: Rc<&MyStruct>) {} + | ^^^^^^^^^^^^^ help: try: `&MyStruct` + +error: usage of `Rc<&T>` + --> $DIR/redundant_allocation.rs:26:19 + | +LL | pub fn test3(foo: Rc<&MyEnum>) {} + | ^^^^^^^^^^^ help: try: `&MyEnum` + +error: usage of `Rc>` + --> $DIR/redundant_allocation.rs:32:17 + | +LL | pub fn test5(a: Rc>) {} + | ^^^^^^^^^^^^ help: try: `Rc` + +error: usage of `Rc>` + --> $DIR/redundant_allocation.rs:36:17 + | +LL | pub fn test6(a: Rc>) {} + | ^^^^^^^^^^^^^ help: try: `Box` + +error: usage of `Box<&T>` + --> $DIR/redundant_allocation.rs:40:22 + | +LL | pub fn test7(foo: Box<&T>) {} + | ^^^^^^^ help: try: `&T` + +error: usage of `Box<&T>` + --> $DIR/redundant_allocation.rs:42:19 + | +LL | pub fn test8(foo: Box<&MyStruct>) {} + | ^^^^^^^^^^^^^^ help: try: `&MyStruct` + +error: usage of `Box<&T>` + --> $DIR/redundant_allocation.rs:44:19 + | +LL | pub fn test9(foo: Box<&MyEnum>) {} + | ^^^^^^^^^^^^ help: try: `&MyEnum` + +error: aborting due to 8 previous errors +