diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index af8fe7abd962..a3652c18a788 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -193,14 +193,18 @@ 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 |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` | + /// |Prefix |Postfix |`self` taken | `self` type | + /// |-------|------------|-----------------------|--------------| + /// |`as_` | none |`&self` or `&mut self` | any | + /// |`from_`| none | none | any | + /// |`into_`| none |`self` | any | + /// |`is_` | none |`&self` or none | any | + /// |`to_` | `_mut` |`&mut self` | any | + /// |`to_` | not `_mut` |`self` | `Copy` | + /// |`to_` | not `_mut` |`&self` | not `Copy` | + /// + /// Please find more info here: + /// https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv /// /// **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 @@ -1837,10 +1841,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods { let item = cx.tcx.hir().expect_item(parent); let self_ty = cx.tcx.type_of(item.def_id); - // if this impl block implements a trait, lint in trait definition instead - if let hir::ItemKind::Impl(hir::Impl { of_trait: Some(_), .. }) = item.kind { - return; - } + let implements_trait = matches!(item.kind, hir::ItemKind::Impl(hir::Impl { of_trait: Some(_), .. })); if_chain! { if let hir::ImplItemKind::Fn(ref sig, id) = impl_item.kind; @@ -1855,7 +1856,8 @@ impl<'tcx> LateLintPass<'tcx> for Methods { if let Some(first_arg_ty) = first_arg_ty; then { - if cx.access_levels.is_exported(impl_item.hir_id()) { + // if this impl block implements a trait, lint in trait definition instead + if !implements_trait && cx.access_levels.is_exported(impl_item.hir_id()) { // check missing trait implementations for method_config in &TRAIT_METHODS { if name == method_config.method_name && @@ -1891,11 +1893,17 @@ impl<'tcx> LateLintPass<'tcx> for Methods { item.vis.node.is_pub(), self_ty, first_arg_ty, - first_arg.pat.span + first_arg.pat.span, + false ); } } + // if this impl block implements a trait, lint in trait definition instead + if implements_trait { + return; + } + if let hir::ImplItemKind::Fn(_, _) = impl_item.kind { let ret_ty = return_ty(cx, impl_item.hir_id()); @@ -1947,7 +1955,8 @@ impl<'tcx> LateLintPass<'tcx> for Methods { false, self_ty, first_arg_ty, - first_arg_span + first_arg_span, + true ); } } @@ -2339,10 +2348,10 @@ impl SelfKind { #[must_use] fn description(self) -> &'static str { match self { - Self::Value => "self by value", - Self::Ref => "self by reference", - Self::RefMut => "self by mutable reference", - Self::No => "no self", + Self::Value => "`self` by value", + Self::Ref => "`self` by reference", + Self::RefMut => "`self` by mutable reference", + Self::No => "no `self`", } } } diff --git a/clippy_lints/src/methods/wrong_self_convention.rs b/clippy_lints/src/methods/wrong_self_convention.rs index b728d7d8d08e..59e683aa9a78 100644 --- a/clippy_lints/src/methods/wrong_self_convention.rs +++ b/clippy_lints/src/methods/wrong_self_convention.rs @@ -1,5 +1,6 @@ use crate::methods::SelfKind; use clippy_utils::diagnostics::span_lint_and_help; +use clippy_utils::ty::is_copy; use rustc_lint::LateContext; use rustc_middle::ty::TyS; use rustc_span::source_map::Span; @@ -9,7 +10,7 @@ use super::WRONG_PUB_SELF_CONVENTION; use super::WRONG_SELF_CONVENTION; #[rustfmt::skip] -const CONVENTIONS: [(&[Convention], &[SelfKind]); 8] = [ +const CONVENTIONS: [(&[Convention], &[SelfKind]); 9] = [ (&[Convention::Eq("new")], &[SelfKind::No]), (&[Convention::StartsWith("as_")], &[SelfKind::Ref, SelfKind::RefMut]), (&[Convention::StartsWith("from_")], &[SelfKind::No]), @@ -17,7 +18,11 @@ const CONVENTIONS: [(&[Convention], &[SelfKind]); 8] = [ (&[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]), + + // Conversion using `to_` can use borrowed (non-Copy types) or owned (Copy types). + // Source: https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv + (&[Convention::StartsWith("to_"), Convention::NotEndsWith("_mut"), Convention::IsSelfTypeCopy(false), Convention::ImplementsTrait(false)], &[SelfKind::Ref]), + (&[Convention::StartsWith("to_"), Convention::NotEndsWith("_mut"), Convention::IsSelfTypeCopy(true), Convention::ImplementsTrait(false)], &[SelfKind::Value]), ]; enum Convention { @@ -25,16 +30,20 @@ enum Convention { StartsWith(&'static str), EndsWith(&'static str), NotEndsWith(&'static str), + IsSelfTypeCopy(bool), + ImplementsTrait(bool), } impl Convention { #[must_use] - fn check(&self, other: &str) -> bool { + fn check<'tcx>(&self, cx: &LateContext<'tcx>, self_ty: &'tcx TyS<'tcx>, other: &str, is_trait_def: bool) -> bool { 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), + Self::NotEndsWith(this) => !Self::EndsWith(this).check(cx, self_ty, other, is_trait_def), + Self::IsSelfTypeCopy(is_true) => is_true == is_copy(cx, self_ty), + Self::ImplementsTrait(is_true) => is_true == is_trait_def, } } } @@ -42,10 +51,17 @@ impl Convention { impl fmt::Display for Convention { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> { 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)), + Self::Eq(this) => format!("`{}`", this).fmt(f), + Self::StartsWith(this) => format!("`{}*`", this).fmt(f), + Self::EndsWith(this) => format!("`*{}`", this).fmt(f), + Self::NotEndsWith(this) => format!("`~{}`", this).fmt(f), + Self::IsSelfTypeCopy(is_true) => { + format!("`self` type is{} `Copy`", if is_true { "" } else { " not" }).fmt(f) + }, + Self::ImplementsTrait(is_true) => { + let (negation, s_suffix) = if is_true { ("", "s") } else { (" does not", "") }; + format!("Method{} implement{} a trait", negation, s_suffix).fmt(f) + }, } } } @@ -57,47 +73,44 @@ pub(super) fn check<'tcx>( self_ty: &'tcx TyS<'tcx>, first_arg_ty: &'tcx TyS<'tcx>, first_arg_span: Span, + is_trait_item: bool, ) { let lint = if is_pub { WRONG_PUB_SELF_CONVENTION } else { WRONG_SELF_CONVENTION }; - if let Some((conventions, self_kinds)) = &CONVENTIONS - .iter() - .find(|(convs, _)| convs.iter().all(|conv| conv.check(item_name))) - { + if let Some((conventions, self_kinds)) = &CONVENTIONS.iter().find(|(convs, _)| { + convs + .iter() + .all(|conv| conv.check(cx, self_ty, item_name, is_trait_item)) + }) { if !self_kinds.iter().any(|k| k.matches(cx, self_ty, first_arg_ty)) { 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 + // Don't mention `NotEndsWith` when there is also `StartsWith` convention present + let cut_ends_with_conv = conventions.iter().any(|conv| matches!(conv, Convention::StartsWith(_))) + && conventions .iter() - .map(|c| format!("`{}`", &c.to_string())) - .collect::>() - .join(" and "); + .any(|conv| matches!(conv, Convention::NotEndsWith(_))); + + let s = conventions + .iter() + .filter_map(|conv| { + if (cut_ends_with_conv && matches!(conv, Convention::NotEndsWith(_))) + || matches!(conv, Convention::ImplementsTrait(_)) + { + None + } else { + Some(conv.to_string()) + } + }) + .collect::>() + .join(" and "); - format!("methods called like this: ({})", &s) - } + format!("methods with the following characteristics: ({})", &s) } else { - format!("methods called `{}`", &conventions[0]) + format!("methods called {}", &conventions[0]) } }; diff --git a/tests/ui/def_id_nocore.stderr b/tests/ui/def_id_nocore.stderr index a3e9cc75b084..702684f6b43a 100644 --- a/tests/ui/def_id_nocore.stderr +++ b/tests/ui/def_id_nocore.stderr @@ -1,4 +1,4 @@ -error: methods called `as_*` usually take self by reference or self by mutable reference +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 { diff --git a/tests/ui/use_self.fixed b/tests/ui/use_self.fixed index a630936e3b1d..a6619f358920 100644 --- a/tests/ui/use_self.fixed +++ b/tests/ui/use_self.fixed @@ -75,13 +75,13 @@ mod lifetimes { mod issue2894 { trait IntoBytes { - fn to_bytes(&self) -> Vec; + fn to_bytes(self) -> Vec; } // This should not be linted impl IntoBytes for u8 { - fn to_bytes(&self) -> Vec { - vec![*self] + fn to_bytes(self) -> Vec { + vec![self] } } } diff --git a/tests/ui/use_self.rs b/tests/ui/use_self.rs index f3e081dd2032..3c41ce500e0d 100644 --- a/tests/ui/use_self.rs +++ b/tests/ui/use_self.rs @@ -75,13 +75,13 @@ mod lifetimes { mod issue2894 { trait IntoBytes { - fn to_bytes(&self) -> Vec; + fn to_bytes(self) -> Vec; } // This should not be linted impl IntoBytes for u8 { - fn to_bytes(&self) -> Vec { - vec![*self] + fn to_bytes(self) -> Vec { + vec![self] } } } diff --git a/tests/ui/wrong_self_convention.rs b/tests/ui/wrong_self_convention.rs index 6cfc0fcb4cae..ba9e19a17220 100644 --- a/tests/ui/wrong_self_convention.rs +++ b/tests/ui/wrong_self_convention.rs @@ -163,3 +163,35 @@ mod issue6307 { fn to_mut(&mut self); } } + +mod issue6727 { + trait ToU64 { + fn to_u64(self) -> u64; + fn to_u64_v2(&self) -> u64; + } + + #[derive(Clone, Copy)] + struct FooCopy; + + impl ToU64 for FooCopy { + fn to_u64(self) -> u64 { + 1 + } + // trigger lint + fn to_u64_v2(&self) -> u64 { + 1 + } + } + + struct FooNoCopy; + + impl ToU64 for FooNoCopy { + // trigger lint + fn to_u64(self) -> u64 { + 2 + } + fn to_u64_v2(&self) -> u64 { + 2 + } + } +} diff --git a/tests/ui/wrong_self_convention.stderr b/tests/ui/wrong_self_convention.stderr index f43fea0d5137..1d58a12ac795 100644 --- a/tests/ui/wrong_self_convention.stderr +++ b/tests/ui/wrong_self_convention.stderr @@ -1,4 +1,4 @@ -error: methods called `from_*` usually take no self +error: methods called `from_*` usually take no `self` --> $DIR/wrong_self_convention.rs:18:17 | LL | fn from_i32(self) {} @@ -7,7 +7,7 @@ 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 +error: methods called `from_*` usually take no `self` --> $DIR/wrong_self_convention.rs:24:21 | LL | pub fn from_i64(self) {} @@ -15,7 +15,7 @@ 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 +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) {} @@ -23,7 +23,7 @@ LL | fn as_i32(self) {} | = help: consider choosing a less ambiguous name -error: methods called `into_*` usually take self by value +error: methods called `into_*` usually take `self` by value --> $DIR/wrong_self_convention.rs:38:17 | LL | fn into_i32(&self) {} @@ -31,7 +31,7 @@ LL | fn into_i32(&self) {} | = help: consider choosing a less ambiguous name -error: methods called `is_*` usually take self by reference or no self +error: methods called `is_*` usually take `self` by reference or no `self` --> $DIR/wrong_self_convention.rs:40:15 | LL | fn is_i32(self) {} @@ -39,7 +39,7 @@ LL | fn is_i32(self) {} | = help: consider choosing a less ambiguous name -error: methods called `to_*` usually take self by reference +error: methods with the following characteristics: (`to_*` and `self` type is not `Copy`) usually take `self` by reference --> $DIR/wrong_self_convention.rs:42:15 | LL | fn to_i32(self) {} @@ -47,7 +47,7 @@ LL | fn to_i32(self) {} | = help: consider choosing a less ambiguous name -error: methods called `from_*` usually take no self +error: methods called `from_*` usually take no `self` --> $DIR/wrong_self_convention.rs:44:17 | LL | fn from_i32(self) {} @@ -55,7 +55,7 @@ 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 +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) {} @@ -63,7 +63,7 @@ LL | pub fn as_i64(self) {} | = help: consider choosing a less ambiguous name -error: methods called `into_*` usually take self by value +error: methods called `into_*` usually take `self` by value --> $DIR/wrong_self_convention.rs:47:21 | LL | pub fn into_i64(&self) {} @@ -71,7 +71,7 @@ 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 +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) {} @@ -79,7 +79,7 @@ LL | pub fn is_i64(self) {} | = help: consider choosing a less ambiguous name -error: methods called `to_*` usually take self by reference +error: methods with the following characteristics: (`to_*` and `self` type is not `Copy`) usually take `self` by reference --> $DIR/wrong_self_convention.rs:49:19 | LL | pub fn to_i64(self) {} @@ -87,7 +87,7 @@ LL | pub fn to_i64(self) {} | = help: consider choosing a less ambiguous name -error: methods called `from_*` usually take no self +error: methods called `from_*` usually take no `self` --> $DIR/wrong_self_convention.rs:50:21 | LL | pub fn from_i64(self) {} @@ -95,7 +95,7 @@ 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 +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) {} @@ -103,7 +103,7 @@ LL | fn as_i32(self) {} | = help: consider choosing a less ambiguous name -error: methods called `into_*` usually take self by value +error: methods called `into_*` usually take `self` by value --> $DIR/wrong_self_convention.rs:98:25 | LL | fn into_i32_ref(&self) {} @@ -111,7 +111,7 @@ 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 +error: methods called `is_*` usually take `self` by reference or no `self` --> $DIR/wrong_self_convention.rs:100:19 | LL | fn is_i32(self) {} @@ -119,15 +119,7 @@ LL | fn is_i32(self) {} | = help: 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 +error: methods called `from_*` usually take no `self` --> $DIR/wrong_self_convention.rs:104:21 | LL | fn from_i32(self) {} @@ -135,7 +127,7 @@ 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 +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); @@ -143,7 +135,7 @@ LL | fn as_i32(self); | = help: consider choosing a less ambiguous name -error: methods called `into_*` usually take self by value +error: methods called `into_*` usually take `self` by value --> $DIR/wrong_self_convention.rs:122:25 | LL | fn into_i32_ref(&self); @@ -151,7 +143,7 @@ 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 +error: methods called `is_*` usually take `self` by reference or no `self` --> $DIR/wrong_self_convention.rs:124:19 | LL | fn is_i32(self); @@ -159,15 +151,7 @@ LL | fn is_i32(self); | = help: 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 +error: methods called `from_*` usually take no `self` --> $DIR/wrong_self_convention.rs:128:21 | LL | fn from_i32(self); @@ -175,7 +159,7 @@ LL | fn from_i32(self); | = help: consider choosing a less ambiguous name -error: methods called `into_*` usually take self by value +error: methods called `into_*` usually take `self` by value --> $DIR/wrong_self_convention.rs:146:25 | LL | fn into_i32_ref(&self); @@ -183,7 +167,7 @@ LL | fn into_i32_ref(&self); | = help: consider choosing a less ambiguous name -error: methods called `from_*` usually take no self +error: methods called `from_*` usually take no `self` --> $DIR/wrong_self_convention.rs:152:21 | LL | fn from_i32(self); @@ -191,5 +175,21 @@ LL | fn from_i32(self); | = help: consider choosing a less ambiguous name +error: methods with the following characteristics: (`to_*` and `self` type is `Copy`) usually take `self` by value + --> $DIR/wrong_self_convention.rs:181:22 + | +LL | fn to_u64_v2(&self) -> u64 { + | ^^^^^ + | + = help: consider choosing a less ambiguous name + +error: methods with the following characteristics: (`to_*` and `self` type is not `Copy`) usually take `self` by reference + --> $DIR/wrong_self_convention.rs:190:19 + | +LL | fn to_u64(self) -> u64 { + | ^^^^ + | + = help: consider choosing a less ambiguous name + error: aborting due to 24 previous errors diff --git a/tests/ui/wrong_self_conventions_mut.stderr b/tests/ui/wrong_self_conventions_mut.stderr index 7662b38e67d0..6ce37c594911 100644 --- a/tests/ui/wrong_self_conventions_mut.stderr +++ b/tests/ui/wrong_self_conventions_mut.stderr @@ -1,4 +1,4 @@ -error: methods called `to_*` usually take self by reference +error: methods with the following characteristics: (`to_*` and `self` type is not `Copy`) usually take `self` by reference --> $DIR/wrong_self_conventions_mut.rs:15:24 | LL | pub fn to_many(&mut self) -> Option<&mut [T]> { @@ -7,7 +7,7 @@ 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 +error: methods with the following characteristics: (`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]> {