Use only check_expr with parent expr and precedence

rust-lang · Mar 14, 2020 · d524ef1 · d524ef1
1 parent 0c0237c
commit d524ef1
Show file tree

Hide file tree

Showing 9 changed files with 81 additions and 102 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1227,7 +1227,7 @@ Released 2018-09-13
 [`expect_fun_call`]: https://rust-lang.github.io/rust-clippy/master/index.html#expect_fun_call
 [`expl_impl_clone_on_copy`]: https://rust-lang.github.io/rust-clippy/master/index.html#expl_impl_clone_on_copy
 [`explicit_counter_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_counter_loop
-[`explicit_deref_method`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_deref_method
+[`explicit_deref_methods`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_deref_methods
 [`explicit_into_iter_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_into_iter_loop
 [`explicit_iter_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_iter_loop
 [`explicit_write`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_write

diff --git 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 362 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/dereference.rs b/clippy_lints/src/dereference.rs
@@ -1,10 +1,8 @@
-use crate::utils::{
-    get_parent_expr, get_trait_def_id, implements_trait, method_calls, paths, snippet, span_lint_and_sugg,
-};
+use crate::utils::{get_parent_expr, implements_trait, snippet, span_lint_and_sugg};
 use if_chain::if_chain;
+use rustc_ast::util::parser::ExprPrecedence;
 use rustc_errors::Applicability;
-use rustc_hir as hir;
-use rustc_hir::{Expr, ExprKind, QPath, StmtKind};
+use rustc_hir::{Expr, ExprKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::source_map::Span;
@@ -31,100 +29,52 @@ declare_clippy_lint! {
     /// ```rust,ignore
     /// let _ = d.unwrap().deref();
     /// ```
-    pub EXPLICIT_DEREF_METHOD,
+    pub EXPLICIT_DEREF_METHODS,
     pedantic,
     "Explicit use of deref or deref_mut method while not in a method chain."
 }
 
 declare_lint_pass!(Dereferencing => [
-    EXPLICIT_DEREF_METHOD
+    EXPLICIT_DEREF_METHODS
 ]);
 
 impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Dereferencing {
-    fn check_stmt(&mut self, cx: &LateContext<'a, 'tcx>, stmt: &'tcx hir::Stmt<'_>) {
-        if_chain! {
-            if let StmtKind::Local(ref local) = stmt.kind;
-            if let Some(ref init) = local.init;
-
-            then {
-                match init.kind {
-                    ExprKind::Call(ref _method, args) => {
-                        for arg in args {
-                            if_chain! {
-                                // Caller must call only one other function (deref or deref_mut)
-                                // otherwise it can lead to error prone suggestions (ie: &*a.len())
-                                let (method_names, arg_list, _) = method_calls(arg, 2);
-                                if method_names.len() == 1;
-                                // Caller must be a variable
-                                let variables = arg_list[0];
-                                if variables.len() == 1;
-                                if let ExprKind::Path(QPath::Resolved(None, _)) = variables[0].kind;
-
-                                then {
-                                    let name = method_names[0].as_str();
-                                    lint_deref(cx, &*name, &variables[0], variables[0].span, arg.span);
-                                }
-                            }
-                        }
-                    }
-                    ExprKind::MethodCall(ref method_name, _, ref args) => {
-                        if init.span.from_expansion() {
-                            return;
-                        }
-                        if_chain! {
-                            if args.len() == 1;
-                            if let ExprKind::Path(QPath::Resolved(None, _)) = args[0].kind;
-                            // Caller must call only one other function (deref or deref_mut)
-                            // otherwise it can lead to error prone suggestions (ie: &*a.len())
-                            let (method_names, arg_list, _) = method_calls(init, 2);
-                            if method_names.len() == 1;
-                            // Caller must be a variable
-                            let variables = arg_list[0];
-                            if variables.len() == 1;
-                            if let ExprKind::Path(QPath::Resolved(None, _)) = variables[0].kind;
-
-                            then {
-                                let name = method_name.ident.as_str();
-                                lint_deref(cx, &*name, init, args[0].span, init.span);
-                            }
-                        }
-                    }
-                    _ => ()
-                }
-            }
-        }
-    }
-
     fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) {
         if_chain! {
+            if !expr.span.from_expansion();
             if let ExprKind::MethodCall(ref method_name, _, ref args) = &expr.kind;
             if args.len() == 1;
-            if let ExprKind::Path(QPath::Resolved(None, _)) = args[0].kind;
-            if let Some(parent) = get_parent_expr(cx, &expr);
 
             then {
-                match parent.kind {
-                    // Already linted using statements
-                    ExprKind::Call(_, _) | ExprKind::MethodCall(_, _, _) => (),
-                    _ => {
-                        let name = method_name.ident.as_str();
-                        lint_deref(cx, &*name, &args[0], args[0].span, expr.span);
+                if let Some(parent_expr) = get_parent_expr(cx, expr) {
+                    // Check if we have the whole call chain here
+                    if let ExprKind::MethodCall(..) = parent_expr.kind {
+                        return;
+                    }
+                    // Check for unary precedence
+                    if let ExprPrecedence::Unary = parent_expr.precedence() {
+                        return;
                     }
                 }
+                let name = method_name.ident.as_str();
+                lint_deref(cx, &*name, &args[0], args[0].span, expr.span);
             }
         }
     }
 }
 
-fn lint_deref(cx: &LateContext<'_, '_>, fn_name: &str, call_expr: &Expr<'_>, var_span: Span, expr_span: Span) {
-    match fn_name {
+fn lint_deref(cx: &LateContext<'_, '_>, method_name: &str, call_expr: &Expr<'_>, var_span: Span, expr_span: Span) {
+    match method_name {
         "deref" => {
-            if get_trait_def_id(cx, &paths::DEREF_TRAIT)
+            if cx
+                .tcx
+                .lang_items()
+                .deref_trait()
                 .map_or(false, |id| implements_trait(cx, cx.tables.expr_ty(&call_expr), id, &[]))
             {
                 span_lint_and_sugg(
                     cx,
-                    EXPLICIT_DEREF_METHOD,
+                    EXPLICIT_DEREF_METHODS,
                     expr_span,
                     "explicit deref method call",
                     "try this",
@@ -134,12 +84,15 @@ fn lint_deref(cx: &LateContext<'_, '_>, fn_name: &str, call_expr: &Expr<'_>, var
             }
         },
         "deref_mut" => {
-            if get_trait_def_id(cx, &paths::DEREF_MUT_TRAIT)
+            if cx
+                .tcx
+                .lang_items()
+                .deref_mut_trait()
                 .map_or(false, |id| implements_trait(cx, cx.tables.expr_ty(&call_expr), id, &[]))
             {
                 span_lint_and_sugg(
                     cx,
-                    EXPLICIT_DEREF_METHOD,
+                    EXPLICIT_DEREF_METHODS,
                     expr_span,
                     "explicit deref_mut method call",
                     "try this",

diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -506,7 +506,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &copy_iterator::COPY_ITERATOR,
         &dbg_macro::DBG_MACRO,
         &default_trait_access::DEFAULT_TRAIT_ACCESS,
-        &dereference::EXPLICIT_DEREF_METHOD,
+        &dereference::EXPLICIT_DEREF_METHODS,
         &derive::DERIVE_HASH_XOR_EQ,
         &derive::EXPL_IMPL_CLONE_ON_COPY,
         &doc::DOC_MARKDOWN,
@@ -1072,7 +1072,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&copies::SAME_FUNCTIONS_IN_IF_CONDITION),
         LintId::of(&copy_iterator::COPY_ITERATOR),
         LintId::of(&default_trait_access::DEFAULT_TRAIT_ACCESS),
-        LintId::of(&dereference::EXPLICIT_DEREF_METHOD),
+        LintId::of(&dereference::EXPLICIT_DEREF_METHODS),
         LintId::of(&derive::EXPL_IMPL_CLONE_ON_COPY),
         LintId::of(&doc::DOC_MARKDOWN),
         LintId::of(&doc::MISSING_ERRORS_DOC),

diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs
@@ -21,9 +21,7 @@ pub const CSTRING: [&str; 4] = ["std", "ffi", "c_str", "CString"];
 pub const CSTRING_AS_C_STR: [&str; 5] = ["std", "ffi", "c_str", "CString", "as_c_str"];
 pub const DEFAULT_TRAIT: [&str; 3] = ["core", "default", "Default"];
 pub const DEFAULT_TRAIT_METHOD: [&str; 4] = ["core", "default", "Default", "default"];
-pub const DEREF_MUT_TRAIT: [&str; 4] = ["core", "ops", "deref", "DerefMut"];
 pub const DEREF_MUT_TRAIT_METHOD: [&str; 5] = ["core", "ops", "deref", "DerefMut", "deref_mut"];
-pub const DEREF_TRAIT: [&str; 4] = ["core", "ops", "deref", "Deref"];
 pub const DEREF_TRAIT_METHOD: [&str; 5] = ["core", "ops", "deref", "Deref", "deref"];
 pub const DISPLAY_FMT_METHOD: [&str; 4] = ["core", "fmt", "Display", "fmt"];
 pub const DISPLAY_TRAIT: [&str; 3] = ["core", "fmt", "Display"];

diff --git 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; 362] = [
     Lint {
         name: "absurd_extreme_comparisons",
         group: "correctness",
@@ -526,7 +526,7 @@ pub const ALL_LINTS: [Lint; 361] = [
         module: "loops",
     },
     Lint {
-        name: "explicit_deref_method",
+        name: "explicit_deref_methods",
         group: "pedantic",
         desc: "Explicit use of deref or deref_mut method while not in a method chain.",
         deprecation: None,

diff --git a/tests/ui/dereference.fixed b/tests/ui/dereference.fixed
@@ -1,7 +1,7 @@
 // run-rustfix
 
 #![allow(unused_variables, clippy::many_single_char_names, clippy::clone_double_ref)]
-#![warn(clippy::explicit_deref_method)]
+#![warn(clippy::explicit_deref_methods)]
 
 use std::ops::{Deref, DerefMut};
 
@@ -34,20 +34,25 @@ fn main() {
 
     let b: String = concat(&*a);
 
-    // following should not require linting
+    let b = &*just_return(a);
+
+    let b: String = concat(&*just_return(a));
 
-    let b = just_return(a).deref();
+    let b: &str = &*a.deref();
 
-    let b: String = concat(just_return(a).deref());
+    let opt_a = Some(a.clone());
+    let b = &*opt_a.unwrap();
+
+    // following should not require linting
+
+    let b: &str = &*a.deref();
 
     let b: String = a.deref().clone();
 
     let b: usize = a.deref_mut().len();
 
     let b: &usize = &a.deref().len();
 
-    let b: &str = a.deref().deref();
-
     let b: &str = &*a;
 
     let b: &mut str = &mut *a;
@@ -59,9 +64,6 @@ fn main() {
     }
     let b: &str = expr_deref!(a);
 
-    let opt_a = Some(a);
-    let b = opt_a.unwrap().deref();
-
     // The struct does not implement Deref trait
     #[derive(Copy, Clone)]
     struct NoLint(u32);

diff --git a/tests/ui/dereference.rs b/tests/ui/dereference.rs
@@ -1,7 +1,7 @@
 // run-rustfix
 
 #![allow(unused_variables, clippy::many_single_char_names, clippy::clone_double_ref)]
-#![warn(clippy::explicit_deref_method)]
+#![warn(clippy::explicit_deref_methods)]
 
 use std::ops::{Deref, DerefMut};
 
@@ -34,20 +34,25 @@ fn main() {
 
     let b: String = concat(a.deref());
 
-    // following should not require linting
-
     let b = just_return(a).deref();
 
     let b: String = concat(just_return(a).deref());
 
+    let b: &str = a.deref().deref();
+
+    let opt_a = Some(a.clone());
+    let b = opt_a.unwrap().deref();
+
+    // following should not require linting
+
+    let b: &str = &*a.deref();
+
     let b: String = a.deref().clone();
 
     let b: usize = a.deref_mut().len();
 
     let b: &usize = &a.deref().len();
 
-    let b: &str = a.deref().deref();
-
     let b: &str = &*a;
 
     let b: &mut str = &mut *a;
@@ -59,9 +64,6 @@ fn main() {
     }
     let b: &str = expr_deref!(a);
 
-    let opt_a = Some(a);
-    let b = opt_a.unwrap().deref();
-
     // The struct does not implement Deref trait
     #[derive(Copy, Clone)]
     struct NoLint(u32);

diff --git a/tests/ui/dereference.stderr b/tests/ui/dereference.stderr
@@ -4,7 +4,7 @@ error: explicit deref method call
 LL |     let b: &str = a.deref();
    |                   ^^^^^^^^^ help: try this: `&*a`
    |
-   = note: `-D clippy::explicit-deref-method` implied by `-D warnings`
+   = note: `-D clippy::explicit-deref-methods` implied by `-D warnings`
 
 error: explicit deref_mut method call
   --> $DIR/dereference.rs:23:23
@@ -42,5 +42,29 @@ error: explicit deref method call
 LL |     let b: String = concat(a.deref());
    |                            ^^^^^^^^^ help: try this: `&*a`
 
-error: aborting due to 7 previous errors
+error: explicit deref method call
+  --> $DIR/dereference.rs:37:13
+   |
+LL |     let b = just_return(a).deref();
+   |             ^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&*just_return(a)`
+
+error: explicit deref method call
+  --> $DIR/dereference.rs:39:28
+   |
+LL |     let b: String = concat(just_return(a).deref());
+   |                            ^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&*just_return(a)`
+
+error: explicit deref method call
+  --> $DIR/dereference.rs:41:19
+   |
+LL |     let b: &str = a.deref().deref();
+   |                   ^^^^^^^^^^^^^^^^^ help: try this: `&*a.deref()`
+
+error: explicit deref method call
+  --> $DIR/dereference.rs:44:13
+   |
+LL |     let b = opt_a.unwrap().deref();
+   |             ^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&*opt_a.unwrap()`
+
+error: aborting due to 11 previous errors