Auto merge of #6828 - mgacek8:issue_6758_enhance_wrong_self_convention, r=flip1995

bors · bors · commit 0929a24d7281 · 2021-03-15T22:36:57.000Z
wrong_self_convention: fix lint in case of `to_*_mut` method fixes #6758 changelog: wrong_self_convention: fix lint in case of `to_*_mut` method. When a method starts with `to_` and ends with `_mut`, clippy expects a `&mut self` parameter, otherwise `&self`. Any feedback is welcome. I was also thinking if shouldn't we treat `to_` the same way as `as_`. Namely to accept `self` taken: `&self` or `&mut self`.
diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
@@ -192,13 +192,14 @@ declare_clippy_lint! {
     /// **What it does:** Checks for methods with certain name prefixes and which
     /// doesn't match how self is taken. The actual rules are:
     ///
-    /// |Prefix |`self` taken          |
-    /// |-------|----------------------|
-    /// |`as_`  |`&self` or `&mut self`|
-    /// |`from_`| none                 |
-    /// |`into_`|`self`                |
-    /// |`is_`  |`&self` or none       |
-    /// |`to_`  |`&self`               |
+    /// |Prefix |Postfix     |`self` taken          |
+    /// |-------|------------|----------------------|
+    /// |`as_`  | none       |`&self` or `&mut self`|
+    /// |`from_`| none       | none                 |
+    /// |`into_`| none       |`self`                |
+    /// |`is_`  | none       |`&self` or none       |
+    /// |`to_`  | `_mut`     |`&mut &self`          |
+    /// |`to_`  | not `_mut` |`&self`               |
     ///
     /// **Why is this bad?** Consistency breeds readability. If you follow the
     /// conventions, your users won't be surprised that they, e.g., need to supply a
diff --git a/clippy_lints/src/methods/wrong_self_convention.rs b/clippy_lints/src/methods/wrong_self_convention.rs
@@ -1,5 +1,5 @@
 use crate::methods::SelfKind;
-use crate::utils::span_lint;
+use crate::utils::span_lint_and_help;
 use rustc_lint::LateContext;
 use rustc_middle::ty::TyS;
 use rustc_span::source_map::Span;
@@ -9,18 +9,22 @@ use super::WRONG_PUB_SELF_CONVENTION;
 use super::WRONG_SELF_CONVENTION;
 
 #[rustfmt::skip]
-const CONVENTIONS: [(Convention, &[SelfKind]); 7] = [
-    (Convention::Eq("new"), &[SelfKind::No]),
-    (Convention::StartsWith("as_"), &[SelfKind::Ref, SelfKind::RefMut]),
-    (Convention::StartsWith("from_"), &[SelfKind::No]),
-    (Convention::StartsWith("into_"), &[SelfKind::Value]),
-    (Convention::StartsWith("is_"), &[SelfKind::Ref, SelfKind::No]),
-    (Convention::Eq("to_mut"), &[SelfKind::RefMut]),
-    (Convention::StartsWith("to_"), &[SelfKind::Ref]),
+const CONVENTIONS: [(&[Convention], &[SelfKind]); 8] = [
+    (&[Convention::Eq("new")], &[SelfKind::No]),
+    (&[Convention::StartsWith("as_")], &[SelfKind::Ref, SelfKind::RefMut]),
+    (&[Convention::StartsWith("from_")], &[SelfKind::No]),
+    (&[Convention::StartsWith("into_")], &[SelfKind::Value]),
+    (&[Convention::StartsWith("is_")], &[SelfKind::Ref, SelfKind::No]),
+    (&[Convention::Eq("to_mut")], &[SelfKind::RefMut]),
+    (&[Convention::StartsWith("to_"), Convention::EndsWith("_mut")], &[SelfKind::RefMut]),
+    (&[Convention::StartsWith("to_"), Convention::NotEndsWith("_mut")], &[SelfKind::Ref]),
 ];
+
 enum Convention {
     Eq(&'static str),
     StartsWith(&'static str),
+    EndsWith(&'static str),
+    NotEndsWith(&'static str),
 }
 
 impl Convention {
@@ -29,6 +33,8 @@ impl Convention {
         match *self {
             Self::Eq(this) => this == other,
             Self::StartsWith(this) => other.starts_with(this) && this != other,
+            Self::EndsWith(this) => other.ends_with(this) && this != other,
+            Self::NotEndsWith(this) => !Self::EndsWith(this).check(other),
         }
     }
 }
@@ -38,6 +44,8 @@ impl fmt::Display for Convention {
         match *self {
             Self::Eq(this) => this.fmt(f),
             Self::StartsWith(this) => this.fmt(f).and_then(|_| '*'.fmt(f)),
+            Self::EndsWith(this) => '*'.fmt(f).and_then(|_| this.fmt(f)),
+            Self::NotEndsWith(this) => '~'.fmt(f).and_then(|_| this.fmt(f)),
         }
     }
 }
@@ -55,21 +63,59 @@ pub(super) fn check<'tcx>(
     } else {
         WRONG_SELF_CONVENTION
     };
-    if let Some((ref conv, self_kinds)) = &CONVENTIONS.iter().find(|(ref conv, _)| conv.check(item_name)) {
+    if let Some((conventions, self_kinds)) = &CONVENTIONS
+        .iter()
+        .find(|(convs, _)| convs.iter().all(|conv| conv.check(item_name)))
+    {
         if !self_kinds.iter().any(|k| k.matches(cx, self_ty, first_arg_ty)) {
-            span_lint(
+            let suggestion = {
+                if conventions.len() > 1 {
+                    let special_case = {
+                        // Don't mention `NotEndsWith` when there is also `StartsWith` convention present
+                        if conventions.len() == 2 {
+                            match conventions {
+                                [Convention::StartsWith(starts_with), Convention::NotEndsWith(_)]
+                                | [Convention::NotEndsWith(_), Convention::StartsWith(starts_with)] => {
+                                    Some(format!("methods called `{}`", Convention::StartsWith(starts_with)))
+                                },
+                                _ => None,
+                            }
+                        } else {
+                            None
+                        }
+                    };
+
+                    if let Some(suggestion) = special_case {
+                        suggestion
+                    } else {
+                        let s = conventions
+                            .iter()
+                            .map(|c| format!("`{}`", &c.to_string()))
+                            .collect::<Vec<_>>()
+                            .join(" and ");
+
+                        format!("methods called like this: ({})", &s)
+                    }
+                } else {
+                    format!("methods called `{}`", &conventions[0])
+                }
+            };
+
+            span_lint_and_help(
                 cx,
                 lint,
                 first_arg_span,
                 &format!(
-                    "methods called `{}` usually take {}; consider choosing a less ambiguous name",
-                    conv,
+                    "{} usually take {}",
+                    suggestion,
                     &self_kinds
                         .iter()
                         .map(|k| k.description())
                         .collect::<Vec<_>>()
                         .join(" or ")
                 ),
+                None,
+                "consider choosing a less ambiguous name",
             );
         }
     }
diff --git a/tests/ui/def_id_nocore.stderr b/tests/ui/def_id_nocore.stderr
@@ -1,10 +1,11 @@
-error: methods called `as_*` usually take self by reference or self by mutable reference; consider choosing a less ambiguous name
+error: methods called `as_*` usually take self by reference or self by mutable reference
   --> $DIR/def_id_nocore.rs:26:19
    |
 LL |     pub fn as_ref(self) -> &'static str {
    |                   ^^^^
    |
    = note: `-D clippy::wrong-self-convention` implied by `-D warnings`
+   = help: consider choosing a less ambiguous name
 
 error: aborting due to previous error
 
diff --git a/tests/ui/wrong_self_convention.stderr b/tests/ui/wrong_self_convention.stderr
@@ -1,148 +1,195 @@
-error: methods called `from_*` usually take no self; consider choosing a less ambiguous name
+error: methods called `from_*` usually take no self
   --> $DIR/wrong_self_convention.rs:18:17
    |
 LL |     fn from_i32(self) {}
    |                 ^^^^
    |
    = note: `-D clippy::wrong-self-convention` implied by `-D warnings`
+   = help: consider choosing a less ambiguous name
 
-error: methods called `from_*` usually take no self; consider choosing a less ambiguous name
+error: methods called `from_*` usually take no self
   --> $DIR/wrong_self_convention.rs:24:21
    |
 LL |     pub fn from_i64(self) {}
    |                     ^^^^
+   |
+   = help: consider choosing a less ambiguous name
 
-error: methods called `as_*` usually take self by reference or self by mutable reference; consider choosing a less ambiguous name
+error: methods called `as_*` usually take self by reference or self by mutable reference
   --> $DIR/wrong_self_convention.rs:36:15
    |
 LL |     fn as_i32(self) {}
    |               ^^^^
+   |
+   = help: consider choosing a less ambiguous name
 
-error: methods called `into_*` usually take self by value; consider choosing a less ambiguous name
+error: methods called `into_*` usually take self by value
   --> $DIR/wrong_self_convention.rs:38:17
    |
 LL |     fn into_i32(&self) {}
    |                 ^^^^^
+   |
+   = help: consider choosing a less ambiguous name
 
-error: methods called `is_*` usually take self by reference or no self; consider choosing a less ambiguous name
+error: methods called `is_*` usually take self by reference or no self
   --> $DIR/wrong_self_convention.rs:40:15
    |
 LL |     fn is_i32(self) {}
    |               ^^^^
+   |
+   = help: consider choosing a less ambiguous name
 
-error: methods called `to_*` usually take self by reference; consider choosing a less ambiguous name
+error: methods called `to_*` usually take self by reference
   --> $DIR/wrong_self_convention.rs:42:15
    |
 LL |     fn to_i32(self) {}
    |               ^^^^
+   |
+   = help: consider choosing a less ambiguous name
 
-error: methods called `from_*` usually take no self; consider choosing a less ambiguous name
+error: methods called `from_*` usually take no self
   --> $DIR/wrong_self_convention.rs:44:17
    |
 LL |     fn from_i32(self) {}
    |                 ^^^^
+   |
+   = help: consider choosing a less ambiguous name
 
-error: methods called `as_*` usually take self by reference or self by mutable reference; consider choosing a less ambiguous name
+error: methods called `as_*` usually take self by reference or self by mutable reference
   --> $DIR/wrong_self_convention.rs:46:19
    |
 LL |     pub fn as_i64(self) {}
    |                   ^^^^
+   |
+   = help: consider choosing a less ambiguous name
 
-error: methods called `into_*` usually take self by value; consider choosing a less ambiguous name
+error: methods called `into_*` usually take self by value
   --> $DIR/wrong_self_convention.rs:47:21
    |
 LL |     pub fn into_i64(&self) {}
    |                     ^^^^^
+   |
+   = help: consider choosing a less ambiguous name
 
-error: methods called `is_*` usually take self by reference or no self; consider choosing a less ambiguous name
+error: methods called `is_*` usually take self by reference or no self
   --> $DIR/wrong_self_convention.rs:48:19
    |
 LL |     pub fn is_i64(self) {}
    |                   ^^^^
+   |
+   = help: consider choosing a less ambiguous name
 
-error: methods called `to_*` usually take self by reference; consider choosing a less ambiguous name
+error: methods called `to_*` usually take self by reference
   --> $DIR/wrong_self_convention.rs:49:19
    |
 LL |     pub fn to_i64(self) {}
    |                   ^^^^
+   |
+   = help: consider choosing a less ambiguous name
 
-error: methods called `from_*` usually take no self; consider choosing a less ambiguous name
+error: methods called `from_*` usually take no self
   --> $DIR/wrong_self_convention.rs:50:21
    |
 LL |     pub fn from_i64(self) {}
    |                     ^^^^
+   |
+   = help: consider choosing a less ambiguous name
 
-error: methods called `as_*` usually take self by reference or self by mutable reference; consider choosing a less ambiguous name
+error: methods called `as_*` usually take self by reference or self by mutable reference
   --> $DIR/wrong_self_convention.rs:95:19
    |
 LL |         fn as_i32(self) {}
    |                   ^^^^
+   |
+   = help: consider choosing a less ambiguous name
 
-error: methods called `into_*` usually take self by value; consider choosing a less ambiguous name
+error: methods called `into_*` usually take self by value
   --> $DIR/wrong_self_convention.rs:98:25
    |
 LL |         fn into_i32_ref(&self) {}
    |                         ^^^^^
+   |
+   = help: consider choosing a less ambiguous name
 
-error: methods called `is_*` usually take self by reference or no self; consider choosing a less ambiguous name
+error: methods called `is_*` usually take self by reference or no self
   --> $DIR/wrong_self_convention.rs:100:19
    |
 LL |         fn is_i32(self) {}
    |                   ^^^^
+   |
+   = help: consider choosing a less ambiguous name
 
-error: methods called `to_*` usually take self by reference; consider choosing a less ambiguous name
+error: methods called `to_*` usually take self by reference
   --> $DIR/wrong_self_convention.rs:102:19
    |
 LL |         fn to_i32(self) {}
    |                   ^^^^
+   |
+   = help: consider choosing a less ambiguous name
 
-error: methods called `from_*` usually take no self; consider choosing a less ambiguous name
+error: methods called `from_*` usually take no self
   --> $DIR/wrong_self_convention.rs:104:21
    |
 LL |         fn from_i32(self) {}
    |                     ^^^^
+   |
+   = help: consider choosing a less ambiguous name
 
-error: methods called `as_*` usually take self by reference or self by mutable reference; consider choosing a less ambiguous name
+error: methods called `as_*` usually take self by reference or self by mutable reference
   --> $DIR/wrong_self_convention.rs:119:19
    |
 LL |         fn as_i32(self);
    |                   ^^^^
+   |
+   = help: consider choosing a less ambiguous name
 
-error: methods called `into_*` usually take self by value; consider choosing a less ambiguous name
+error: methods called `into_*` usually take self by value
   --> $DIR/wrong_self_convention.rs:122:25
    |
 LL |         fn into_i32_ref(&self);
    |                         ^^^^^
+   |
+   = help: consider choosing a less ambiguous name
 
-error: methods called `is_*` usually take self by reference or no self; consider choosing a less ambiguous name
+error: methods called `is_*` usually take self by reference or no self
   --> $DIR/wrong_self_convention.rs:124:19
    |
 LL |         fn is_i32(self);
    |                   ^^^^
+   |
+   = help: consider choosing a less ambiguous name
 
-error: methods called `to_*` usually take self by reference; consider choosing a less ambiguous name
+error: methods called `to_*` usually take self by reference
   --> $DIR/wrong_self_convention.rs:126:19
    |
 LL |         fn to_i32(self);
    |                   ^^^^
+   |
+   = help: consider choosing a less ambiguous name
 
-error: methods called `from_*` usually take no self; consider choosing a less ambiguous name
+error: methods called `from_*` usually take no self
   --> $DIR/wrong_self_convention.rs:128:21
    |
 LL |         fn from_i32(self);
    |                     ^^^^
+   |
+   = help: consider choosing a less ambiguous name
 
-error: methods called `into_*` usually take self by value; consider choosing a less ambiguous name
+error: methods called `into_*` usually take self by value
   --> $DIR/wrong_self_convention.rs:146:25
    |
 LL |         fn into_i32_ref(&self);
    |                         ^^^^^
+   |
+   = help: consider choosing a less ambiguous name
 
-error: methods called `from_*` usually take no self; consider choosing a less ambiguous name
+error: methods called `from_*` usually take no self
   --> $DIR/wrong_self_convention.rs:152:21
    |
 LL |         fn from_i32(self);
    |                     ^^^^
+   |
+   = help: consider choosing a less ambiguous name
 
 error: aborting due to 24 previous errors
 
diff --git a/tests/ui/wrong_self_conventions_mut.rs b/tests/ui/wrong_self_conventions_mut.rs
diff --git a/tests/ui/wrong_self_conventions_mut.stderr b/tests/ui/wrong_self_conventions_mut.stderr

Original file line number	Diff line number	Diff line change
`@@ -1,10 +1,11 @@`
`1`		-error: methods called `as_*` usually take self by reference or self by mutable reference; consider choosing a less ambiguous name
	`1`	+error: methods called `as_*` usually take self by reference or self by mutable reference
`2`	`2`	`--> $DIR/def_id_nocore.rs:26:19`
`3`	`3`	`\|`
`4`	`4`	`LL \| pub fn as_ref(self) -> &'static str {`
`5`	`5`	`\| ^^^^`
`6`	`6`	`\|`
`7`	`7`	= note: `-D clippy::wrong-self-convention` implied by `-D warnings`
	`8`	`+ = help: consider choosing a less ambiguous name`
`8`	`9`
`9`	`10`	`error: aborting due to previous error`
`10`	`11`