From 020a85b0439b98cf0f3561a83ea9f29185800736 Mon Sep 17 00:00:00 2001 From: Jacek Pospychala Date: Tue, 17 Mar 2020 22:59:16 +0100 Subject: [PATCH] useless Rc>, Rc>, Rc<&T> --- CHANGELOG.md | 3 + README.md | 2 +- clippy_lints/src/lib.rs | 9 ++ clippy_lints/src/types.rs | 167 +++++++++++++++++++++++++---- clippy_lints/src/utils/paths.rs | 1 + src/lintlist/mod.rs | 23 +++- tests/ui/must_use_candidates.fixed | 2 +- tests/ui/must_use_candidates.rs | 2 +- tests/ui/rc_borrows.rs | 30 ++++++ tests/ui/rc_borrows.stderr | 43 ++++++++ tests/ui/rc_rc.rs | 8 ++ tests/ui/rc_rc.stderr | 20 ++++ 12 files changed, 285 insertions(+), 25 deletions(-) create mode 100644 tests/ui/rc_borrows.rs create mode 100644 tests/ui/rc_borrows.stderr create mode 100644 tests/ui/rc_rc.rs create mode 100644 tests/ui/rc_rc.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 027a73edf86b..eb67a881fc9b 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.md b/README.md index fefbb3486fdc..97a7ad5cd17c 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 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: diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 36075fd478cc..3ee68d3929f8 100644 --- a/clippy_lints/src/lib.rs +++ b/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), ]); diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index a3398cf9c309..99954adf279d 100644 --- a/clippy_lints/src/types.rs +++ b/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>` 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>) {} + /// ``` + /// + /// Better: + /// + /// ```rust + /// # use std::rc::Rc; + /// fn foo(bar: Rc) {} + /// ``` + pub RC_RC, + perf, + "an Rc of Rc" +} + +declare_clippy_lint! { + /// **What it does:** Checks for use of `Rc>` 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>) {} + /// ``` + /// + /// Better: + /// + /// ```rust + /// # use std::rc::Rc; + /// # use std::boxed::Box; + /// fn foo(bar: Rc) {} + /// ``` + 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> { + 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> can be simplified to Rc", + ); + 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>`", + "Rc> can be simplified to Rc", + ); + 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<_> diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index 6cb1f694fd5e..18f849d6945a 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/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"]; diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 9fe08f6c7da4..4bc65f4eff5e 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; 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", diff --git a/tests/ui/must_use_candidates.fixed b/tests/ui/must_use_candidates.fixed index e2ceb8baded2..1f7d854e1c03 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::rc_borrows)] #![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..1f151a7060ad 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::rc_borrows)] #![warn(clippy::must_use_candidate)] use std::rc::Rc; use std::sync::atomic::{AtomicBool, Ordering}; diff --git a/tests/ui/rc_borrows.rs b/tests/ui/rc_borrows.rs new file mode 100644 index 000000000000..96458f33108f --- /dev/null +++ b/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 { + foo: T, +} + +pub enum MyEnum { + One, + Two, +} + +pub fn test(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>) {} + +fn main() {} diff --git a/tests/ui/rc_borrows.stderr b/tests/ui/rc_borrows.stderr new file mode 100644 index 000000000000..15d139a710e9 --- /dev/null +++ b/tests/ui/rc_borrows.stderr @@ -0,0 +1,43 @@ +error: usage of `Rc<&T>` + --> $DIR/rc_borrows.rs:18:21 + | +LL | pub fn test(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 + diff --git a/tests/ui/rc_rc.rs b/tests/ui/rc_rc.rs new file mode 100644 index 000000000000..c6729118f907 --- /dev/null +++ b/tests/ui/rc_rc.rs @@ -0,0 +1,8 @@ +use std::boxed::Box; +use std::rc::Rc; + +pub fn test(a: Rc>) {} + +pub fn test2(a: Rc>) {} + +fn main() {} diff --git a/tests/ui/rc_rc.stderr b/tests/ui/rc_rc.stderr new file mode 100644 index 000000000000..b21d77c276cd --- /dev/null +++ b/tests/ui/rc_rc.stderr @@ -0,0 +1,20 @@ +error: usage of `Rc>` + --> $DIR/rc_rc.rs:4:16 + | +LL | pub fn test(a: Rc>) {} + | ^^^^^^^^^^^^ + | + = note: `-D clippy::rc-rc` implied by `-D warnings` + = help: Rc> can be simplified to Rc + +error: usage of `Rc>` + --> $DIR/rc_rc.rs:6:17 + | +LL | pub fn test2(a: Rc>) {} + | ^^^^^^^^^^^^^ + | + = note: `-D clippy::rc-box` implied by `-D warnings` + = help: Rc> can be simplified to Rc + +error: aborting due to 2 previous errors +