Rollup merge of #5226 - ThibsG:DerefExplicit1566, r=flip1995

Add lint for explicit deref and deref_mut method calls

This PR adds the lint `explicit_deref_method` that suggests replacing `deref()` and `deref_mut()` with `&*a` and `&mut *a`.

It doesn't lint inside macros.

This PR is the continuation of  #3258.

changelog: Add lint `explicit_deref_method`.

Fixes: #1566

CHANGELOG.md

@@ -1256,6 +1256,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_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

clippy_lints/src/dereference.rs

@@ -0,0 +1,113 @@
+use crate::utils::{get_parent_expr, implements_trait, snippet, span_lint_and_sugg};
+use if_chain::if_chain;
+use rustc_ast::util::parser::{ExprPrecedence, PREC_POSTFIX, PREC_PREFIX};
+use rustc_errors::Applicability;
+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;
+
+declare_clippy_lint! {
+    /// **What it does:** Checks for explicit `deref()` or `deref_mut()` method calls.
+    ///
+    /// **Why is this bad?** Derefencing by `&*x` or `&mut *x` is clearer and more concise,
+    /// when not part of a method chain.
+    ///
+    /// **Example:**
+    /// ```rust
+    /// 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;
+    /// ```
+    ///
+    /// This lint excludes
+    /// ```rust,ignore
+    /// let _ = d.unwrap().deref();
+    /// ```
+    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_METHODS
+]);
+
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Dereferencing {
+    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;
+
+            then {
+                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 Expr that we don't want to be linted
+                    let precedence = parent_expr.precedence();
+                    match precedence {
+                        // Lint a Call is ok though
+                        ExprPrecedence::Call | ExprPrecedence::AddrOf => (),
+                        _ => {
+                            if precedence.order() >= PREC_PREFIX && precedence.order() <= PREC_POSTFIX {
+                                return;
+                            }
+                        }
+                    }
+                }
+                let name = method_name.ident.as_str();
+                lint_deref(cx, &*name, &args[0], args[0].span, expr.span);
+            }
+        }
+    }
+}
+
+fn lint_deref(cx: &LateContext<'_, '_>, method_name: &str, call_expr: &Expr<'_>, var_span: Span, expr_span: Span) {
+    match method_name {
+        "deref" => {
+            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_METHODS,
+                    expr_span,
+                    "explicit deref method call",
+                    "try this",
+                    format!("&*{}", &snippet(cx, var_span, "..")),
+                    Applicability::MachineApplicable,
+                );
+            }
+        },
+        "deref_mut" => {
+            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_METHODS,
+                    expr_span,
+                    "explicit deref_mut method call",
+                    "try this",
+                    format!("&mut *{}", &snippet(cx, var_span, "..")),
+                    Applicability::MachineApplicable,
+                );
+            }
+        },
+        _ => (),
+    }
+}

clippy_lints/src/lib.rs

@@ -191,6 +191,7 @@ mod copies;
 mod copy_iterator;
 mod dbg_macro;
 mod default_trait_access;
+mod dereference;
 mod derive;
 mod doc;
 mod double_comparison;
@@ -513,6 +514,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_METHODS,
         &derive::DERIVE_HASH_XOR_EQ,
         &derive::EXPL_IMPL_CLONE_ON_COPY,
         &doc::DOC_MARKDOWN,
@@ -1039,6 +1041,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| box verbose_file_reads::VerboseFileReads);
     store.register_late_pass(|| box redundant_pub_crate::RedundantPubCrate::default());
     store.register_late_pass(|| box unnamed_address::UnnamedAddress);
+    store.register_late_pass(|| box dereference::Dereferencing);
 
     store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
         LintId::of(&arithmetic::FLOAT_ARITHMETIC),
@@ -1089,6 +1092,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_METHODS),
         LintId::of(&derive::EXPL_IMPL_CLONE_ON_COPY),
         LintId::of(&doc::DOC_MARKDOWN),
         LintId::of(&doc::MISSING_ERRORS_DOC),

src/lintlist/mod.rs

@@ -528,6 +528,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
         deprecation: None,
         module: "loops",
     },
+    Lint {
+        name: "explicit_deref_methods",
+        group: "pedantic",
+        desc: "Explicit use of deref or deref_mut method while not in a method chain.",
+        deprecation: None,
+        module: "dereference",
+    },
     Lint {
         name: "explicit_into_iter_loop",
         group: "pedantic",

tests/ui/dereference.fixed

@@ -0,0 +1,93 @@
+// run-rustfix
+
+#![allow(unused_variables, clippy::many_single_char_names, clippy::clone_double_ref)]
+#![warn(clippy::explicit_deref_methods)]
+
+use std::ops::{Deref, DerefMut};
+
+fn concat(deref_str: &str) -> String {
+    format!("{}bar", deref_str)
+}
+
+fn just_return(deref_str: &str) -> &str {
+    deref_str
+}
+
+struct CustomVec(Vec<u8>);
+impl Deref for CustomVec {
+    type Target = Vec<u8>;
+
+    fn deref(&self) -> &Vec<u8> {
+        &self.0
+    }
+}
+
+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);
+
+    let b = &*just_return(a);
+
+    let b: String = concat(&*just_return(a));
+
+    let b: &str = &*a.deref();
+
+    let opt_a = Some(a.clone());
+    let b = &*opt_a.unwrap();
+
+    // following should not require linting
+
+    let cv = CustomVec(vec![0, 42]);
+    let c = cv.deref()[0];
+
+    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;
+
+    let b: &mut str = &mut *a;
+
+    macro_rules! expr_deref {
+        ($body:expr) => {
+            $body.deref()
+        };
+    }
+    let b: &str = expr_deref!(a);
+
+    // 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();
+}

tests/ui/dereference.rs

@@ -0,0 +1,93 @@
+// run-rustfix
+
+#![allow(unused_variables, clippy::many_single_char_names, clippy::clone_double_ref)]
+#![warn(clippy::explicit_deref_methods)]
+
+use std::ops::{Deref, DerefMut};
+
+fn concat(deref_str: &str) -> String {
+    format!("{}bar", deref_str)
+}
+
+fn just_return(deref_str: &str) -> &str {
+    deref_str
+}
+
+struct CustomVec(Vec<u8>);
+impl Deref for CustomVec {
+    type Target = Vec<u8>;
+
+    fn deref(&self) -> &Vec<u8> {
+        &self.0
+    }
+}
+
+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());
+
+    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 cv = CustomVec(vec![0, 42]);
+    let c = cv.deref()[0];
+
+    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;
+
+    let b: &mut str = &mut *a;
+
+    macro_rules! expr_deref {
+        ($body:expr) => {
+            $body.deref()
+        };
+    }
+    let b: &str = expr_deref!(a);
+
+    // 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();
+}

tests/ui/dereference.stderr

@@ -0,0 +1,70 @@
+error: explicit deref method call
+  --> $DIR/dereference.rs:30:19
+   |
+LL |     let b: &str = a.deref();
+   |                   ^^^^^^^^^ help: try this: `&*a`
+   |
+   = note: `-D clippy::explicit-deref-methods` implied by `-D warnings`
+
+error: explicit deref_mut method call
+  --> $DIR/dereference.rs:32:23
+   |
+LL |     let b: &mut str = a.deref_mut();
+   |                       ^^^^^^^^^^^^^ help: try this: `&mut *a`
+
+error: explicit deref method call
+  --> $DIR/dereference.rs:35:39
+   |
+LL |     let b: String = format!("{}, {}", a.deref(), a.deref());
+   |                                       ^^^^^^^^^ help: try this: `&*a`
+
+error: explicit deref method call
+  --> $DIR/dereference.rs:35:50
+   |
+LL |     let b: String = format!("{}, {}", a.deref(), a.deref());
+   |                                                  ^^^^^^^^^ help: try this: `&*a`
+
+error: explicit deref method call
+  --> $DIR/dereference.rs:37:20
+   |
+LL |     println!("{}", a.deref());
+   |                    ^^^^^^^^^ help: try this: `&*a`
+
+error: explicit deref method call
+  --> $DIR/dereference.rs:40:11
+   |
+LL |     match a.deref() {
+   |           ^^^^^^^^^ help: try this: `&*a`
+
+error: explicit deref method call
+  --> $DIR/dereference.rs:44:28
+   |
+LL |     let b: String = concat(a.deref());
+   |                            ^^^^^^^^^ help: try this: `&*a`
+
+error: explicit deref method call
+  --> $DIR/dereference.rs:46:13
+   |
+LL |     let b = just_return(a).deref();
+   |             ^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&*just_return(a)`
+
+error: explicit deref method call
+  --> $DIR/dereference.rs:48:28
+   |
+LL |     let b: String = concat(just_return(a).deref());
+   |                            ^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&*just_return(a)`
+
+error: explicit deref method call
+  --> $DIR/dereference.rs:50:19
+   |
+LL |     let b: &str = a.deref().deref();
+   |                   ^^^^^^^^^^^^^^^^^ help: try this: `&*a.deref()`
+
+error: explicit deref method call
+  --> $DIR/dereference.rs:53:13
+   |
+LL |     let b = opt_a.unwrap().deref();
+   |             ^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&*opt_a.unwrap()`
+
+error: aborting due to 11 previous errors
+