Report using stmts and expr + tests

rust-lang · Mar 1, 2020 · a034f20 · a034f20
1 parent ef81e60
commit a034f20
Show file tree

Hide file tree

Showing 5 changed files with 150 additions and 70 deletions.
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 358 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are 359 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,9 +1,11 @@
-use rustc_hir::{Expr, ExprKind, QPath};
+use crate::utils::{get_parent_expr, method_calls, snippet, span_lint_and_sugg};
+use if_chain::if_chain;
 use rustc_errors::Applicability;
+use rustc_hir as hir;
+use rustc_hir::{Expr, ExprKind, QPath, StmtKind};
 use rustc_lint::{LateContext, LateLintPass};
-use rustc_session::{declare_tool_lint, declare_lint_pass};
-use crate::utils::{in_macro, span_lint_and_sugg};
-use if_chain::if_chain;
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::source_map::Span;
 
 declare_clippy_lint! {
     /// **What it does:** Checks for explicit `deref()` or `deref_mut()` method calls.
@@ -21,7 +23,7 @@ declare_clippy_lint! {
     /// let b = &*a;
     /// let c = &mut *a;
     /// ```
-    /// 
+    ///
     /// This lint excludes
     /// ```rust
     /// let e = d.unwrap().deref();
@@ -36,45 +38,105 @@ declare_lint_pass!(Dereferencing => [
 ]);
 
 impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Dereferencing {
-    fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) {
-        if in_macro(expr.span) {
-            return;
+    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 (ex: &*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].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 (ex: &*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, args[0].span, init.span);
+                            }
+                        }
+                    }
+                    _ => ()
+                }
+            }
         }
+    }
 
+    fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) {
         if_chain! {
-            // if this is a method call
             if let ExprKind::MethodCall(ref method_name, _, ref args) = &expr.kind;
-            // on a Path (i.e. a variable/name, not another method)
-            if let ExprKind::Path(QPath::Resolved(None, path)) = &args[0].kind;
+            if args.len() == 1;
+            if let Some(parent) = get_parent_expr(cx, &expr);
+
             then {
-                let name = method_name.ident.as_str();
-                // alter help slightly to account for _mut
-                match &*name {
-                    "deref" => {
-                        span_lint_and_sugg(
-                            cx,
-                            EXPLICIT_DEREF_METHOD,
-                            expr.span,
-                            "explicit deref method call",
-                            "try this",
-                            format!("&*{}", path),
-                            Applicability::MachineApplicable
-                        );
-                    },
-                    "deref_mut" => {
-                        span_lint_and_sugg(
-                            cx,
-                            EXPLICIT_DEREF_METHOD,
-                            expr.span,
-                            "explicit deref_mut method call",
-                            "try this",
-                            format!("&mut *{}", path),
-                            Applicability::MachineApplicable
-                        );
-                    },
-                    _ => ()
-                };
+                // Call and MethodCall exprs are better reported using statements
+                match parent.kind {
+                    ExprKind::Call(_, _) => return,
+                    ExprKind::MethodCall(_, _, _) => return,
+                    _ => {
+                        let name = method_name.ident.as_str();
+                        lint_deref(cx, &*name, args[0].span, expr.span);
+                    }
+                }
             }
         }
     }
 }
+
+fn lint_deref(cx: &LateContext<'_, '_>, fn_name: &str, var_span: Span, expr_span: Span) {
+    match fn_name {
+        "deref" => {
+            span_lint_and_sugg(
+                cx,
+                EXPLICIT_DEREF_METHOD,
+                expr_span,
+                "explicit deref method call",
+                "try this",
+                format!("&*{}", &snippet(cx, var_span, "..")),
+                Applicability::MachineApplicable,
+            );
+        },
+        "deref_mut" => {
+            span_lint_and_sugg(
+                cx,
+                EXPLICIT_DEREF_METHOD,
+                expr_span,
+                "explicit deref_mut method call",
+                "try this",
+                format!("&mut *{}", &snippet(cx, var_span, "..")),
+                Applicability::MachineApplicable,
+            );
+        },
+        _ => (),
+    }
+}
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; 358] = [
+pub const ALL_LINTS: [Lint; 359] = [
     Lint {
         name: "absurd_extreme_comparisons",
         group: "correctness",

diff --git a/tests/ui/dereference.rs b/tests/ui/dereference.rs
@@ -3,28 +3,49 @@
 
 use std::ops::{Deref, DerefMut};
 
+fn concat(deref_str: &str) -> String {
+    format!("{}bar", deref_str)
+}
+
+fn just_return(deref_str: &str) -> &str {
+    deref_str
+}
+
 fn main() {
     let a: &mut String = &mut String::from("foo");
 
     // these should require linting
+
     let b: &str = a.deref();
 
     let b: &mut str = a.deref_mut();
 
+    // both derefs should get linted here
+    let b: String = format!("{}, {}", a.deref(), a.deref());
+
+    println!("{}", a.deref());
+
+    #[allow(clippy::match_single_binding)]
+    match a.deref() {
+        _ => (),
+    }
+
+    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: String = a.deref().clone();
 
     let b: usize = a.deref_mut().len();
 
     let b: &usize = &a.deref().len();
 
-    // only first deref should get linted here
     let b: &str = a.deref().deref();
 
-    // both derefs should get linted here
-    let b: String = format!("{}, {}", a.deref(), a.deref());
-
-    // these should not require linting
-
     let b: &str = &*a;
 
     let b: &mut str = &mut *a;
@@ -35,4 +56,7 @@ fn main() {
         };
     }
     let b: &str = expr_deref!(a);
+
+    let opt_a = Some(a);
+    let b = opt_a.unwrap().deref();
 }
diff --git a/tests/ui/dereference.stderr b/tests/ui/dereference.stderr
@@ -1,52 +1,46 @@
 error: explicit deref method call
-  --> $DIR/dereference.rs:10:19
+  --> $DIR/dereference.rs:19:19
    |
 LL |     let b: &str = a.deref();
    |                   ^^^^^^^^^ help: try this: `&*a`
    |
    = note: `-D clippy::explicit-deref-method` implied by `-D warnings`
 
 error: explicit deref_mut method call
-  --> $DIR/dereference.rs:12:23
+  --> $DIR/dereference.rs:21:23
    |
 LL |     let b: &mut str = a.deref_mut();
    |                       ^^^^^^^^^^^^^ help: try this: `&mut *a`
 
 error: explicit deref method call
-  --> $DIR/dereference.rs:14:21
-   |
-LL |     let b: String = a.deref().clone();
-   |                     ^^^^^^^^^ help: try this: `&*a`
-
-error: explicit deref_mut method call
-  --> $DIR/dereference.rs:16:20
+  --> $DIR/dereference.rs:24:39
    |
-LL |     let b: usize = a.deref_mut().len();
-   |                    ^^^^^^^^^^^^^ help: try this: `&mut *a`
+LL |     let b: String = format!("{}, {}", a.deref(), a.deref());
+   |                                       ^^^^^^^^^ help: try this: `&*a`
 
 error: explicit deref method call
-  --> $DIR/dereference.rs:18:22
+  --> $DIR/dereference.rs:24:50
    |
-LL |     let b: &usize = &a.deref().len();
-   |                      ^^^^^^^^^ help: try this: `&*a`
+LL |     let b: String = format!("{}, {}", a.deref(), a.deref());
+   |                                                  ^^^^^^^^^ help: try this: `&*a`
 
 error: explicit deref method call
-  --> $DIR/dereference.rs:21:19
+  --> $DIR/dereference.rs:26:20
    |
-LL |     let b: &str = a.deref().deref();
-   |                   ^^^^^^^^^ help: try this: `&*a`
+LL |     println!("{}", a.deref());
+   |                    ^^^^^^^^^ help: try this: `&*a`
 
 error: explicit deref method call
-  --> $DIR/dereference.rs:24:39
+  --> $DIR/dereference.rs:29:11
    |
-LL |     let b: String = format!("{}, {}", a.deref(), a.deref());
-   |                                       ^^^^^^^^^ help: try this: `&*a`
+LL |     match a.deref() {
+   |           ^^^^^^^^^ help: try this: `&*a`
 
 error: explicit deref method call
-  --> $DIR/dereference.rs:24:50
+  --> $DIR/dereference.rs:33:28
    |
-LL |     let b: String = format!("{}, {}", a.deref(), a.deref());
-   |                                                  ^^^^^^^^^ help: try this: `&*a`
+LL |     let b: String = concat(a.deref());
+   |                            ^^^^^^^^^ help: try this: `&*a`
 
-error: aborting due to 8 previous errors
+error: aborting due to 7 previous errors