Auto merge of #5349 - jpospychala:useless_rc, r=Manishearth

useless Rc<Rc<T>>, Rc<Box<T>>, Rc<&T>, Box<&T> refers to #2394 changelog: Add lints for Rc<Rc<T>> and Rc<Box<T>> and Rc<&T>, Box<&T> this is based on top of another change #5310 so probably should go after that one.
rust-lang · Apr 2, 2020 · a840d59 · a840d59
2 parents 7ebb3aa + f8e892d
commit a840d59
Show file tree

Hide file tree

Showing 10 changed files with 259 additions and 8 deletions.
diff --git 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
@@ -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::FN_TO_NUMERIC_CAST_WITH_TRUNCATION),
         LintId::of(&types::IMPLICIT_HASHER),
         LintId::of(&types::LET_UNIT_VALUE),
+        LintId::of(&types::REDUNDANT_ALLOCATION),
         LintId::of(&types::TYPE_COMPLEXITY),
         LintId::of(&types::UNIT_ARG),
         LintId::of(&types::UNIT_CMP),
@@ -1660,6 +1662,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
@@ -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<T>>`, `Rc<Box<T>>`, `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<Span> {
     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<Span> {
+    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<Rc<T>>`",
+                                "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<Box<T>>`",
+                                "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 match_type_parameter(cx, qpath, &paths::OPTION).is_some() {
                             span_lint(
                                 cx,
                                 OPTION_OPTION,

diff --git 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
@@ -1725,6 +1725,13 @@ pub static ref ALL_LINTS: Vec<Lint> = 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
@@ -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
@@ -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
@@ -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<T> {
+    foo: T,
+}
+
+pub enum MyEnum {
+    One,
+    Two,
+}
+
+// Rc<&T>
+
+pub fn test1<T>(foo: &T) {}
+
+pub fn test2(foo: &MyStruct) {}
+
+pub fn test3(foo: &MyEnum) {}
+
+pub fn test4_neg(foo: Rc<SubT<&usize>>) {}
+
+// Rc<Rc<T>>
+
+pub fn test5(a: Rc<bool>) {}
+
+// Rc<Box<T>>
+
+pub fn test6(a: Box<bool>) {}
+
+// Box<&T>
+
+pub fn test7<T>(foo: &T) {}
+
+pub fn test8(foo: &MyStruct) {}
+
+pub fn test9(foo: &MyEnum) {}
+
+pub fn test10_neg(foo: Box<SubT<&usize>>) {}
+
+fn main() {}
diff --git a/tests/ui/redundant_allocation.rs 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<T> {
+    foo: T,
+}
+
+pub enum MyEnum {
+    One,
+    Two,
+}
+
+// Rc<&T>
+
+pub fn test1<T>(foo: Rc<&T>) {}
+
+pub fn test2(foo: Rc<&MyStruct>) {}
+
+pub fn test3(foo: Rc<&MyEnum>) {}
+
+pub fn test4_neg(foo: Rc<SubT<&usize>>) {}
+
+// Rc<Rc<T>>
+
+pub fn test5(a: Rc<Rc<bool>>) {}
+
+// Rc<Box<T>>
+
+pub fn test6(a: Rc<Box<bool>>) {}
+
+// Box<&T>
+
+pub fn test7<T>(foo: Box<&T>) {}
+
+pub fn test8(foo: Box<&MyStruct>) {}
+
+pub fn test9(foo: Box<&MyEnum>) {}
+
+pub fn test10_neg(foo: Box<SubT<&usize>>) {}
+
+fn main() {}
diff --git a/tests/ui/redundant_allocation.stderr 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<T>(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<Rc<T>>`
+  --> $DIR/redundant_allocation.rs:32:17
+   |
+LL | pub fn test5(a: Rc<Rc<bool>>) {}
+   |                 ^^^^^^^^^^^^ help: try: `Rc<bool>`
+
+error: usage of `Rc<Box<T>>`
+  --> $DIR/redundant_allocation.rs:36:17
+   |
+LL | pub fn test6(a: Rc<Box<bool>>) {}
+   |                 ^^^^^^^^^^^^^ help: try: `Box<bool>`
+
+error: usage of `Box<&T>`
+  --> $DIR/redundant_allocation.rs:40:22
+   |
+LL | pub fn test7<T>(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
+