diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 7fd14c4f9..16d5e3bb4 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -191,13 +191,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 index 90fab5774..c8bcad7be 100644 --- 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::>() + .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::>() .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 index ed87a5054..a3e9cc75b 100644 --- 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 index 32bd9075b..f43fea0d5 100644 --- 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 new file mode 100644 index 000000000..486a0d772 --- /dev/null +++ b/tests/ui/wrong_self_conventions_mut.rs @@ -0,0 +1,30 @@ +// edition:2018 +#![warn(clippy::wrong_self_convention)] +#![allow(dead_code)] + +fn main() {} + +mod issue6758 { + pub enum Test { + One(T), + Many(Vec), + } + + impl Test { + // If a method starts with `to_` and not ends with `_mut` it should expect `&self` + pub fn to_many(&mut self) -> Option<&mut [T]> { + match self { + Self::Many(data) => Some(data), + _ => None, + } + } + + // If a method starts with `to_` and ends with `_mut` it should expect `&mut self` + pub fn to_many_mut(&self) -> Option<&[T]> { + match self { + Self::Many(data) => Some(data), + _ => None, + } + } + } +} diff --git a/tests/ui/wrong_self_conventions_mut.stderr b/tests/ui/wrong_self_conventions_mut.stderr new file mode 100644 index 000000000..7662b38e6 --- /dev/null +++ b/tests/ui/wrong_self_conventions_mut.stderr @@ -0,0 +1,19 @@ +error: methods called `to_*` usually take self by reference + --> $DIR/wrong_self_conventions_mut.rs:15:24 + | +LL | pub fn to_many(&mut self) -> Option<&mut [T]> { + | ^^^^^^^^^ + | + = note: `-D clippy::wrong-self-convention` implied by `-D warnings` + = help: consider choosing a less ambiguous name + +error: methods called like this: (`to_*` and `*_mut`) usually take self by mutable reference + --> $DIR/wrong_self_conventions_mut.rs:23:28 + | +LL | pub fn to_many_mut(&self) -> Option<&[T]> { + | ^^^^^ + | + = help: consider choosing a less ambiguous name + +error: aborting due to 2 previous errors +