useless Rc<Rc<T>>, Rc<Box<T>>, Rc<&T>

Author: jpospychala

CHANGELOG.md

@@ -1433,6 +1433,9 @@ 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
+[`rc_borrows`]: https://rust-lang.github.io/rust-clippy/master/index.html#rc_borrows
+[`rc_box`]: https://rust-lang.github.io/rust-clippy/master/index.html#rc_box
+[`rc_rc`]: https://rust-lang.github.io/rust-clippy/master/index.html#rc_rc
 [`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

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 365 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:
 

clippy_lints/src/lib.rs

@@ -811,6 +811,9 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &types::LET_UNIT_VALUE,
         &types::LINKEDLIST,
         &types::OPTION_OPTION,
+        &types::RC_BORROWS,
+        &types::RC_BOX,
+        &types::RC_RC,
         &types::TYPE_COMPLEXITY,
         &types::UNIT_ARG,
         &types::UNIT_CMP,
@@ -1374,6 +1377,9 @@ 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::RC_BORROWS),
+        LintId::of(&types::RC_BOX),
+        LintId::of(&types::RC_RC),
         LintId::of(&types::TYPE_COMPLEXITY),
         LintId::of(&types::UNIT_ARG),
         LintId::of(&types::UNIT_CMP),
@@ -1656,6 +1662,9 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&trivially_copy_pass_by_ref::TRIVIALLY_COPY_PASS_BY_REF),
         LintId::of(&types::BOX_BORROWS),
         LintId::of(&types::BOX_VEC),
+        LintId::of(&types::RC_BORROWS),
+        LintId::of(&types::RC_BOX),
+        LintId::of(&types::RC_RC),
         LintId::of(&vec::USELESS_VEC),
     ]);
 

clippy_lints/src/types.rs

@@ -194,11 +194,98 @@ declare_clippy_lint! {
     "a box of borrowed type"
 }
 
+declare_clippy_lint! {
+    /// **What it does:** Checks for use of `Rc<&T>` anywhere in the code.
+    ///
+    /// **Why is this bad?** Wrapping a reference `&T` with an `Rc` adds unnecessary
+    /// level of indirection.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    /// ```rust
+    /// # use std::rc::Rc;
+    /// fn foo(bar: Rc<&usize>) {}
+    /// ```
+    ///
+    /// Better:
+    ///
+    /// ```rust
+    /// # use std::rc::Rc;
+    /// fn foo(bar: &usize) {}
+    /// ```
+    pub RC_BORROWS,
+    perf,
+    "a Rc of borrowed type"
+}
+
+declare_clippy_lint! {
+    /// **What it does:** Checks for use of `Rc<Rc<T>>` anywhere in the code.
+    ///
+    /// **Why is this bad?** Reference counting pointer of reference counting pointer
+    /// is an unnecessary level of indirection.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    /// ```rust
+    /// # use std::rc::Rc;
+    /// fn foo(bar: Rc<Rc<usize>>) {}
+    /// ```
+    ///
+    /// Better:
+    ///
+    /// ```rust
+    /// # use std::rc::Rc;
+    /// fn foo(bar: Rc<usize>) {}
+    /// ```
+    pub RC_RC,
+    perf,
+    "an Rc of Rc"
+}
+
+declare_clippy_lint! {
+    /// **What it does:** Checks for use of `Rc<Box<T>>` anywhere in the code.
+    ///
+    /// **Why is this bad?** `Rc` already keeps its contents in a separate area on
+    /// the heap. So if you `Box` its contents, you just add another level of indirection.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    /// ```rust
+    /// # use std::rc::Rc;
+    /// # use std::boxed::Box;
+    /// fn foo(bar: Rc<Box<usize>>) {}
+    /// ```
+    ///
+    /// Better:
+    ///
+    /// ```rust
+    /// # use std::rc::Rc;
+    /// # use std::boxed::Box;
+    /// fn foo(bar: Rc<usize>) {}
+    /// ```
+    pub RC_BOX,
+    perf,
+    "an Rc of Box"
+}
+
 pub struct Types {
     vec_box_size_threshold: u64,
 }
 
-impl_lint_pass!(Types => [BOX_VEC, VEC_BOX, OPTION_OPTION, LINKEDLIST, BORROWED_BOX, BOX_BORROWS]);
+impl_lint_pass!(Types => [
+    BOX_VEC,
+    VEC_BOX,
+    OPTION_OPTION,
+    LINKEDLIST,
+    BORROWED_BOX,
+    BOX_BORROWS,
+    RC_RC,
+    RC_BOX,
+    RC_BORROWS
+]);
 
 impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Types {
     fn check_fn(
@@ -259,6 +346,24 @@ fn match_type_parameter(cx: &LateContext<'_, '_>, qpath: &QPath<'_>, path: &[&st
     false
 }
 
+fn match_borrows_parameter<'a>(cx: &'a LateContext<'_, '_>, qpath: &QPath<'_>) -> Option<Ty<'a>> {
+    if_chain! {
+        let last = last_path_segment(qpath); // why last path segment?
+        if let Some(ref params) = last.args;
+        if !params.parenthesized; // what are parenthesized params?
+        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 {
+            return Some(ty_ty);
+        }
+    }
+    None
+}
+
 impl Types {
     pub fn new(vec_box_size_threshold: u64) -> Self {
         Self { vec_box_size_threshold }
@@ -291,26 +396,15 @@ 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 let Some(ty_ty) = match_borrows_parameter(cx, qpath) {
+                            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(
@@ -322,6 +416,37 @@ impl Types {
                             );
                             return; // don't recurse into the type
                         }
+                    } else if Some(def_id) == cx.tcx.lang_items().rc() {
+                        if match_type_parameter(cx, qpath, &paths::RC) {
+                            span_lint_and_help(
+                                cx,
+                                RC_RC,
+                                hir_ty.span,
+                                "usage of `Rc<Rc<T>>`",
+                                "Rc<Rc<T>> can be simplified to Rc<T>",
+                            );
+                            return; // don't recurse into the type
+                        }
+                        if match_type_parameter(cx, qpath, &paths::BOX) {
+                            span_lint_and_help(
+                                cx,
+                                RC_BOX,
+                                hir_ty.span,
+                                "usage of `Rc<Box<T>>`",
+                                "Rc<Box<T>> can be simplified to Rc<T>",
+                            );
+                            return; // don't recurse into the type
+                        }
+                        if let Some(ty_ty) = match_borrows_parameter(cx, qpath) {
+                            span_lint_and_help(
+                                cx,
+                                RC_BORROWS,
+                                hir_ty.span,
+                                "usage of `Rc<&T>`",
+                                format!("try `{}`", ty_ty).as_str(),
+                            );
+                            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<_>

clippy_lints/src/utils/paths.rs

@@ -9,6 +9,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"];

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; 365] = [
     Lint {
         name: "absurd_extreme_comparisons",
         group: "correctness",
@@ -1722,6 +1722,27 @@ pub const ALL_LINTS: [Lint; 361] = [
         deprecation: None,
         module: "ranges",
     },
+    Lint {
+        name: "rc_borrows",
+        group: "perf",
+        desc: "a Rc of borrowed type",
+        deprecation: None,
+        module: "types",
+    },
+    Lint {
+        name: "rc_box",
+        group: "perf",
+        desc: "an Rc of Box",
+        deprecation: None,
+        module: "types",
+    },
+    Lint {
+        name: "rc_rc",
+        group: "perf",
+        desc: "an Rc of Rc",
+        deprecation: None,
+        module: "types",
+    },
     Lint {
         name: "redundant_clone",
         group: "perf",

tests/ui/must_use_candidates.fixed

@@ -1,6 +1,6 @@
 // run-rustfix
 #![feature(never_type)]
-#![allow(unused_mut)]
+#![allow(unused_mut, clippy::rc_borrows)]
 #![warn(clippy::must_use_candidate)]
 use std::rc::Rc;
 use std::sync::atomic::{AtomicBool, Ordering};

tests/ui/must_use_candidates.rs

@@ -1,6 +1,6 @@
 // run-rustfix
 #![feature(never_type)]
-#![allow(unused_mut)]
+#![allow(unused_mut, clippy::rc_borrows)]
 #![warn(clippy::must_use_candidate)]
 use std::rc::Rc;
 use std::sync::atomic::{AtomicBool, Ordering};

tests/ui/rc_borrows.rs

@@ -0,0 +1,30 @@
+#![warn(clippy::all)]
+#![allow(clippy::boxed_local, clippy::needless_pass_by_value)]
+#![allow(clippy::blacklisted_name)]
+
+use std::rc::Rc;
+
+pub struct MyStruct {}
+
+pub struct SubT<T> {
+    foo: T,
+}
+
+pub enum MyEnum {
+    One,
+    Two,
+}
+
+pub fn test<T>(foo: Rc<&T>) {}
+
+pub fn test1(foo: Rc<&usize>) {}
+
+pub fn test2(foo: Rc<&MyStruct>) {}
+
+pub fn test3(foo: Rc<&MyEnum>) {}
+
+pub fn test4(foo: Rc<&&MyEnum>) {}
+
+pub fn test6(foo: Rc<SubT<&usize>>) {}
+
+fn main() {}

tests/ui/rc_borrows.stderr

@@ -0,0 +1,43 @@
+error: usage of `Rc<&T>`
+  --> $DIR/rc_borrows.rs:18:21
+   |
+LL | pub fn test<T>(foo: Rc<&T>) {}
+   |                     ^^^^^^
+   |
+   = note: `-D clippy::rc-borrows` implied by `-D warnings`
+   = help: try `&T`
+
+error: usage of `Rc<&T>`
+  --> $DIR/rc_borrows.rs:20:19
+   |
+LL | pub fn test1(foo: Rc<&usize>) {}
+   |                   ^^^^^^^^^^
+   |
+   = help: try `&usize`
+
+error: usage of `Rc<&T>`
+  --> $DIR/rc_borrows.rs:22:19
+   |
+LL | pub fn test2(foo: Rc<&MyStruct>) {}
+   |                   ^^^^^^^^^^^^^
+   |
+   = help: try `&MyStruct`
+
+error: usage of `Rc<&T>`
+  --> $DIR/rc_borrows.rs:24:19
+   |
+LL | pub fn test3(foo: Rc<&MyEnum>) {}
+   |                   ^^^^^^^^^^^
+   |
+   = help: try `&MyEnum`
+
+error: usage of `Rc<&T>`
+  --> $DIR/rc_borrows.rs:26:19
+   |
+LL | pub fn test4(foo: Rc<&&MyEnum>) {}
+   |                   ^^^^^^^^^^^^
+   |
+   = help: try `&&MyEnum`
+
+error: aborting due to 5 previous errors
+

tests/ui/rc_rc.rs

@@ -0,0 +1,8 @@
+use std::boxed::Box;
+use std::rc::Rc;
+
+pub fn test(a: Rc<Rc<bool>>) {}
+
+pub fn test2(a: Rc<Box<bool>>) {}
+
+fn main() {}