From 48b2b18adc4f8f9f3036975db318460da0427917 Mon Sep 17 00:00:00 2001 From: lengyijun Date: Tue, 17 Aug 2021 10:15:03 +0800 Subject: [PATCH] needless_deref --- CHANGELOG.md | 1 + clippy_lints/src/attrs.rs | 2 +- clippy_lints/src/lib.rs | 4 + clippy_lints/src/needless_bool.rs | 2 +- clippy_lints/src/needless_deref.rs | 113 ++++++++++++++++++ .../src/redundant_static_lifetimes.rs | 2 +- tests/ui/needless_deref.rs | 85 +++++++++++++ tests/ui/needless_deref.stderr | 28 +++++ 8 files changed, 234 insertions(+), 3 deletions(-) create mode 100644 clippy_lints/src/needless_deref.rs create mode 100644 tests/ui/needless_deref.rs create mode 100644 tests/ui/needless_deref.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b89170073be..b29f9d923318 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2818,6 +2818,7 @@ Released 2018-09-13 [`needless_borrowed_reference`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrowed_reference [`needless_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_collect [`needless_continue`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_continue +[`needless_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_deref [`needless_doctest_main`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_doctest_main [`needless_for_each`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_for_each [`needless_lifetimes`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_lifetimes diff --git a/clippy_lints/src/attrs.rs b/clippy_lints/src/attrs.rs index c9ff468874b5..24d8860e9408 100644 --- a/clippy_lints/src/attrs.rs +++ b/clippy_lints/src/attrs.rs @@ -603,7 +603,7 @@ fn check_mismatched_target_os(cx: &EarlyContext<'_>, attr: &Attribute) { MetaItemKind::Word => { if_chain! { if let Some(ident) = meta.ident(); - if let Some(os) = find_os(&*ident.name.as_str()); + if let Some(os) = find_os(&ident.name.as_str()); then { mismatched.push((os, ident.span)); } diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 8372d681078d..1fbc7364d8dd 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -288,6 +288,7 @@ mod needless_bool; mod needless_borrow; mod needless_borrowed_ref; mod needless_continue; +mod needless_deref; mod needless_for_each; mod needless_pass_by_value; mod needless_question_mark; @@ -842,6 +843,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: needless_borrow::REF_BINDING_TO_REFERENCE, needless_borrowed_ref::NEEDLESS_BORROWED_REFERENCE, needless_continue::NEEDLESS_CONTINUE, + needless_deref::NEEDLESS_DEREF, needless_for_each::NEEDLESS_FOR_EACH, needless_pass_by_value::NEEDLESS_PASS_BY_VALUE, needless_question_mark::NEEDLESS_QUESTION_MARK, @@ -1122,6 +1124,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(needless_bitwise_bool::NEEDLESS_BITWISE_BOOL), LintId::of(needless_borrow::REF_BINDING_TO_REFERENCE), LintId::of(needless_continue::NEEDLESS_CONTINUE), + LintId::of(needless_deref::NEEDLESS_DEREF), LintId::of(needless_for_each::NEEDLESS_FOR_EACH), LintId::of(needless_pass_by_value::NEEDLESS_PASS_BY_VALUE), LintId::of(non_expressive_names::SIMILAR_NAMES), @@ -1918,6 +1921,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box mutex_atomic::Mutex); store.register_late_pass(|| box needless_update::NeedlessUpdate); store.register_late_pass(|| box needless_borrow::NeedlessBorrow::default()); + store.register_late_pass(|| box needless_deref::NeedlessDeref); store.register_late_pass(|| box needless_borrowed_ref::NeedlessBorrowedRef); store.register_late_pass(|| box no_effect::NoEffect); store.register_late_pass(|| box temporary_assignment::TemporaryAssignment); diff --git a/clippy_lints/src/needless_bool.rs b/clippy_lints/src/needless_bool.rs index 36f2829a5b94..70e3e96f9bb6 100644 --- a/clippy_lints/src/needless_bool.rs +++ b/clippy_lints/src/needless_bool.rs @@ -328,7 +328,7 @@ enum Expression { fn fetch_bool_block(block: &Block<'_>) -> Expression { match (&*block.stmts, block.expr.as_ref()) { - (&[], Some(e)) => fetch_bool_expr(&**e), + (&[], Some(e)) => fetch_bool_expr(e), (&[ref e], None) => { if let StmtKind::Semi(e) = e.kind { if let ExprKind::Ret(_) = e.kind { diff --git a/clippy_lints/src/needless_deref.rs b/clippy_lints/src/needless_deref.rs new file mode 100644 index 000000000000..7dab70c36881 --- /dev/null +++ b/clippy_lints/src/needless_deref.rs @@ -0,0 +1,113 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::source::snippet_opt; +use rustc_errors::Applicability; +use rustc_hir::UnOp; +use rustc_hir::{Expr, ExprKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::{self, Ty}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; + +declare_clippy_lint! { + /// ### What it does + /// Checks for manual deref in function parameter. + /// + /// ### Why is this bad? + /// There is no need to deref manually. Compiler will auto deref. + /// + /// ### Known problems + /// Complicate type is not handled. For example `foo(&*****(&&T))`. + /// + /// ### Example + /// ```rust + /// fn foo(_: &str) {} + /// let pf = &String::new(); + /// // Bad + /// foo(&**pf); + /// foo(&*String::new()); + /// + /// // Good + /// foo(pf); + /// foo(&String::new()); + /// ``` + pub NEEDLESS_DEREF, + pedantic, + "remove needless deref" +} + +declare_lint_pass!(NeedlessDeref => [NEEDLESS_DEREF]); + +impl LateLintPass<'_> for NeedlessDeref { + fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { + match e.kind { + ExprKind::Call(fn_expr, arguments) => { + if let ExprKind::Path(..) = fn_expr.kind { + // contain no generic parameter + if cx.typeck_results().node_substs_opt(fn_expr.hir_id).is_none() { + check_arguments(cx, arguments, cx.typeck_results().expr_ty(fn_expr)); + } + } + }, + ExprKind::MethodCall(path, _, arguments, _) => { + // contain no generic parameter + if path.args.is_none() && cx.typeck_results().node_substs_opt(e.hir_id).is_none() { + let def_id = cx.typeck_results().type_dependent_def_id(e.hir_id).unwrap(); + let method_type = cx.tcx.type_of(def_id); + check_arguments(cx, arguments, method_type); + } + }, + _ => (), + } + } +} + +fn check_arguments<'tcx>(cx: &LateContext<'tcx>, arguments: &[Expr<'_>], type_definition: Ty<'tcx>) { + match type_definition.kind() { + ty::FnDef(..) | ty::FnPtr(_) => { + for argument in arguments.iter() { + // a: &T + // foo(&** a) -> foo(a) + if_chain! { + if let ExprKind::AddrOf(_, _, child1) = argument.kind ; + if let ExprKind::Unary(UnOp::Deref, child2) = child1.kind ; + if let ExprKind::Unary(UnOp::Deref, child3) = child2.kind ; + if !matches!(child3.kind,ExprKind::Unary(UnOp::Deref, ..) ); + let ty = cx.typeck_results().expr_ty(child3); + if matches!(ty.kind(),ty::Ref(..)); + then{ + span_lint_and_sugg( + cx, + NEEDLESS_DEREF, + argument.span, + "needless explicit deref in function parameter", + "try remove the `&**` and just keep", + snippet_opt(cx, child3.span).unwrap(), + Applicability::MachineApplicable, + ); + } + } + + // a: T + // foo(&*a) -> foo(&a) + if_chain! { + if let ExprKind::AddrOf(_, _, child1) = argument.kind ; + if let ExprKind::Unary(UnOp::Deref, child2) = child1.kind ; + if !matches!(child2.kind,ExprKind::Unary(UnOp::Deref, ..) ); + let ty = cx.typeck_results().expr_ty(child2); + if !matches!(ty.kind(),ty::Ref(..)); + then{ + span_lint_and_sugg( + cx, + NEEDLESS_DEREF, + argument.span, + "needless explicit deref in function parameter", + "you can replace this with", + ("&".to_owned()+&snippet_opt(cx, child2.span).unwrap()).clone(), + Applicability::MachineApplicable, + ); + } + } + } + }, + _ => (), + } +} diff --git a/clippy_lints/src/redundant_static_lifetimes.rs b/clippy_lints/src/redundant_static_lifetimes.rs index d5a1a61da6bf..48779e6ebec1 100644 --- a/clippy_lints/src/redundant_static_lifetimes.rs +++ b/clippy_lints/src/redundant_static_lifetimes.rs @@ -86,7 +86,7 @@ impl RedundantStaticLifetimes { _ => {}, } } - self.visit_type(&*borrow_type.ty, cx, reason); + self.visit_type(&borrow_type.ty, cx, reason); }, TyKind::Slice(ref ty) => { self.visit_type(ty, cx, reason); diff --git a/tests/ui/needless_deref.rs b/tests/ui/needless_deref.rs new file mode 100644 index 000000000000..ca60e2923b02 --- /dev/null +++ b/tests/ui/needless_deref.rs @@ -0,0 +1,85 @@ +#![warn(clippy::needless_deref)] + +use std::path::Path; +use std::path::PathBuf; +use std::sync::Arc; + +fn main() { + test_call(); + test_method_call(); +} + +// Arc -> PathBuf -> Path +fn test_call() { + fn foo(_: &Path) {} + fn bar(_: T) {} + + { + let a = Arc::new(PathBuf::new()); + foo(&**a); // should not lint + + let a = &Arc::new(PathBuf::new()); + foo(&***a); // should not lint + + foo(&*PathBuf::new()); // should lint + + let b = &PathBuf::new(); + foo(b); // should not lint + foo(&*b); // should not lint + foo(&**b); // should lint + } + + { + let a = Arc::new(PathBuf::new()); + bar(&**a); // should not lint + + let a = &Arc::new(PathBuf::new()); + bar(&***a); // should not lint + + bar(&*PathBuf::new()); // should not lint + + let b = &PathBuf::new(); + bar(b); // should not lint + bar(&*b); // should not lint + bar(&**b); // should not lint + } +} + +struct S; +impl S { + fn foo(&self, _a: &Path) {} + fn bar(&self, _a: T) {} +} + +fn test_method_call() { + let s = S; + { + let a = Arc::new(PathBuf::new()); + s.foo(&**a); // should not lint + + let a = &Arc::new(PathBuf::new()); + s.foo(&***a); // should not lint + + s.foo(&*PathBuf::new()); // should lint + + let b = &PathBuf::new(); + s.foo(b); // should not lint + s.foo(&*b); // should not lint + s.foo(&**b); // should lint + } + + { + let a = Arc::new(PathBuf::new()); + s.bar(&**a); // should not lint + + let a = &Arc::new(PathBuf::new()); + s.bar(&***a); // should not lint + + s.bar(&*PathBuf::new()); // should not lint + + let b = &PathBuf::new(); + s.bar(b); // should not lint + s.bar(&*b); // should not lint + s.bar(&**b); // should not lint + } +} diff --git a/tests/ui/needless_deref.stderr b/tests/ui/needless_deref.stderr new file mode 100644 index 000000000000..9e07a2cf5b2a --- /dev/null +++ b/tests/ui/needless_deref.stderr @@ -0,0 +1,28 @@ +error: needless explicit deref in function parameter + --> $DIR/needless_deref.rs:24:13 + | +LL | foo(&*PathBuf::new()); // should lint + | ^^^^^^^^^^^^^^^^ help: you can replace this with: `&PathBuf::new()` + | + = note: `-D clippy::needless-deref` implied by `-D warnings` + +error: needless explicit deref in function parameter + --> $DIR/needless_deref.rs:29:13 + | +LL | foo(&**b); // should lint + | ^^^^ help: try remove the `&**` and just keep: `b` + +error: needless explicit deref in function parameter + --> $DIR/needless_deref.rs:63:15 + | +LL | s.foo(&*PathBuf::new()); // should lint + | ^^^^^^^^^^^^^^^^ help: you can replace this with: `&PathBuf::new()` + +error: needless explicit deref in function parameter + --> $DIR/needless_deref.rs:68:15 + | +LL | s.foo(&**b); // should lint + | ^^^^ help: try remove the `&**` and just keep: `b` + +error: aborting due to 4 previous errors +