Check for Deref trait impl + add fixed version

rust-lang · Feb 26, 2020 · edb0798 · edb0798
1 parent 172fd00
commit edb0798
Show file tree

Hide file tree

Showing 5 changed files with 149 additions and 40 deletions.
diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs
@@ -1,4 +1,6 @@
-use crate::utils::{get_parent_expr, method_calls, snippet, span_lint_and_sugg};
+use crate::utils::{
+    get_parent_expr, get_trait_def_id, implements_trait, method_calls, paths, snippet, span_lint_and_sugg,
+};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir as hir;
@@ -15,18 +17,19 @@ declare_clippy_lint! {
     ///
     /// **Example:**
     /// ```rust
-    /// let b = a.deref();
-    /// let c = a.deref_mut();
+    /// use std::ops::Deref;
+    /// let a: &mut String = &mut String::from("foo");
+    /// let b: &str = a.deref();
     /// ```
     /// Could be written as:
     /// ```rust
+    /// let a: &mut String = &mut String::from("foo");
     /// let b = &*a;
-    /// let c = &mut *a;
     /// ```
     ///
     /// This lint excludes
-    /// ```rust
-    /// let e = d.unwrap().deref();
+    /// ```rust,ignore
+    /// let _ = d.unwrap().deref();
     /// ```
     pub EXPLICIT_DEREF_METHOD,
     pedantic,
@@ -49,7 +52,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Dereferencing {
                         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())
+                                // 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
@@ -59,7 +62,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Dereferencing {
 
                                 then {
                                     let name = method_names[0].as_str();
-                                    lint_deref(cx, &*name, variables[0].span, arg.span);
+                                    lint_deref(cx, &*name, &variables[0], variables[0].span, arg.span);
                                 }
                             }
                         }
@@ -72,7 +75,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Dereferencing {
                             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())
+                            // 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
@@ -82,7 +85,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Dereferencing {
 
                             then {
                                 let name = method_name.ident.as_str();
-                                lint_deref(cx, &*name, args[0].span, init.span);
+                                lint_deref(cx, &*name, init, args[0].span, init.span);
                             }
                         }
                     }
@@ -96,46 +99,54 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Dereferencing {
         if_chain! {
             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 {
-                // Call and MethodCall exprs are better reported using statements
                 match parent.kind {
-                    ExprKind::Call(_, _) => return,
-                    ExprKind::MethodCall(_, _, _) => return,
+                    // Already linted using statements
+                    ExprKind::Call(_, _) | ExprKind::MethodCall(_, _, _) => (),
                     _ => {
                         let name = method_name.ident.as_str();
-                        lint_deref(cx, &*name, args[0].span, expr.span);
+                        lint_deref(cx, &*name, &args[0], args[0].span, expr.span);
                     }
                 }
             }
         }
     }
 }
 
-fn lint_deref(cx: &LateContext<'_, '_>, fn_name: &str, var_span: Span, expr_span: Span) {
+fn lint_deref(cx: &LateContext<'_, '_>, fn_name: &str, call_expr: &Expr<'_>, 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,
-            );
+            if get_trait_def_id(cx, &paths::DEREF_TRAIT)
+                .map_or(false, |id| implements_trait(cx, cx.tables.expr_ty(&call_expr), id, &[]))
+            {
+                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,
-            );
+            if get_trait_def_id(cx, &paths::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,
+                    expr_span,
+                    "explicit deref_mut method call",
+                    "try this",
+                    format!("&mut *{}", &snippet(cx, var_span, "..")),
+                    Applicability::MachineApplicable,
+                );
+            }
         },
         _ => (),
     }

diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs
@@ -21,7 +21,9 @@ 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 DOUBLE_ENDED_ITERATOR: [&str; 4] = ["core", "iter", "traits", "DoubleEndedIterator"];

diff --git a/tests/ui/dereference.fixed b/tests/ui/dereference.fixed
@@ -0,0 +1,79 @@
+// run-rustfix
+
+#![allow(unused_variables, clippy::many_single_char_names, clippy::clone_double_ref)]
+#![warn(clippy::explicit_deref_method)]
+
+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;
+
+    let b: &mut str = &mut *a;
+
+    // both derefs should get linted here
+    let b: String = format!("{}, {}", &*a, &*a);
+
+    println!("{}", &*a);
+
+    #[allow(clippy::match_single_binding)]
+    match &*a {
+        _ => (),
+    }
+
+    let b: String = concat(&*a);
+
+    // 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();
+
+    let b: &str = a.deref().deref();
+
+    let b: &str = &*a;
+
+    let b: &mut str = &mut *a;
+
+    macro_rules! expr_deref {
+        ($body:expr) => {
+            $body.deref()
+        };
+    }
+    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);
+    impl NoLint {
+        pub fn deref(self) -> u32 {
+            self.0
+        }
+        pub fn deref_mut(self) -> u32 {
+            self.0
+        }
+    }
+    let no_lint = NoLint(42);
+    let b = no_lint.deref();
+    let b = no_lint.deref_mut();
+}
diff --git a/tests/ui/dereference.rs b/tests/ui/dereference.rs
@@ -1,3 +1,5 @@
+// run-rustfix
+
 #![allow(unused_variables, clippy::many_single_char_names, clippy::clone_double_ref)]
 #![warn(clippy::explicit_deref_method)]
 
@@ -59,4 +61,19 @@ fn main() {
 
     let opt_a = Some(a);
     let b = opt_a.unwrap().deref();
+
+    // The struct does not implement Deref trait
+    #[derive(Copy, Clone)]
+    struct NoLint(u32);
+    impl NoLint {
+        pub fn deref(self) -> u32 {
+            self.0
+        }
+        pub fn deref_mut(self) -> u32 {
+            self.0
+        }
+    }
+    let no_lint = NoLint(42);
+    let b = no_lint.deref();
+    let b = no_lint.deref_mut();
 }
diff --git a/tests/ui/dereference.stderr b/tests/ui/dereference.stderr
@@ -1,43 +1,43 @@
 error: explicit deref method call
-  --> $DIR/dereference.rs:19:19
+  --> $DIR/dereference.rs:21: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:21:23
+  --> $DIR/dereference.rs:23:23
    |
 LL |     let b: &mut str = a.deref_mut();
    |                       ^^^^^^^^^^^^^ help: try this: `&mut *a`
 
 error: explicit deref method call
-  --> $DIR/dereference.rs:24:39
+  --> $DIR/dereference.rs:26:39
    |
 LL |     let b: String = format!("{}, {}", a.deref(), a.deref());
    |                                       ^^^^^^^^^ help: try this: `&*a`
 
 error: explicit deref method call
-  --> $DIR/dereference.rs:24:50
+  --> $DIR/dereference.rs:26:50
    |
 LL |     let b: String = format!("{}, {}", a.deref(), a.deref());
    |                                                  ^^^^^^^^^ help: try this: `&*a`
 
 error: explicit deref method call
-  --> $DIR/dereference.rs:26:20
+  --> $DIR/dereference.rs:28:20
    |
 LL |     println!("{}", a.deref());
    |                    ^^^^^^^^^ help: try this: `&*a`
 
 error: explicit deref method call
-  --> $DIR/dereference.rs:29:11
+  --> $DIR/dereference.rs:31:11
    |
 LL |     match a.deref() {
    |           ^^^^^^^^^ help: try this: `&*a`
 
 error: explicit deref method call
-  --> $DIR/dereference.rs:33:28
+  --> $DIR/dereference.rs:35:28
    |
 LL |     let b: String = concat(a.deref());
    |                            ^^^^^^^^^ help: try this: `&*a`