diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 37691f27be4..e99fe1b97ff 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -22,6 +22,7 @@ use rustc_semver::RustcVersion; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::source_map::Span; use rustc_span::symbol::{sym, SymbolStr}; +use rustc_typeck::hir_ty_to_ty; use crate::consts::{constant, Constant}; use crate::utils::eager_or_lazy::is_lazyness_candidate; @@ -1623,10 +1624,15 @@ impl<'tcx> LateLintPass<'tcx> for Methods { let item = cx.tcx.hir().expect_item(parent); let def_id = cx.tcx.hir().local_def_id(item.hir_id); let self_ty = cx.tcx.type_of(def_id); + + // if this impl block implements a trait, lint in trait definition instead + if let hir::ItemKind::Impl { of_trait: Some(_), .. } = item.kind { + return; + } + if_chain! { if let hir::ImplItemKind::Fn(ref sig, id) = impl_item.kind; if let Some(first_arg) = iter_input_pats(&sig.decl, cx.tcx.hir().body(id)).next(); - if let hir::ItemKind::Impl{ of_trait: None, .. } = item.kind; let method_def_id = cx.tcx.hir().local_def_id(impl_item.hir_id); let method_sig = cx.tcx.fn_sig(method_def_id); @@ -1668,40 +1674,17 @@ impl<'tcx> LateLintPass<'tcx> for Methods { } } - if let Some((ref conv, self_kinds)) = &CONVENTIONS - .iter() - .find(|(ref conv, _)| conv.check(&name)) - { - if !self_kinds.iter().any(|k| k.matches(cx, self_ty, first_arg_ty)) { - let lint = if item.vis.node.is_pub() { - WRONG_PUB_SELF_CONVENTION - } else { - WRONG_SELF_CONVENTION - }; - - span_lint( - cx, - lint, - first_arg.pat.span, - &format!("methods called `{}` usually take {}; consider choosing a less ambiguous name", - conv, - &self_kinds - .iter() - .map(|k| k.description()) - .collect::>() - .join(" or ") - ), - ); - } - } + lint_wrong_self_convention( + cx, + &name, + item.vis.node.is_pub(), + self_ty, + first_arg_ty, + first_arg.pat.span + ); } } - // if this impl block implements a trait, lint in trait definition instead - if let hir::ItemKind::Impl { of_trait: Some(_), .. } = item.kind { - return; - } - if let hir::ImplItemKind::Fn(_, _) = impl_item.kind { let ret_ty = return_ty(cx, impl_item.hir_id); @@ -1735,8 +1718,23 @@ impl<'tcx> LateLintPass<'tcx> for Methods { } fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx TraitItem<'_>) { + if in_external_macro(cx.tcx.sess, item.span) { + return; + } + + if_chain! { + if let TraitItemKind::Fn(ref sig, _) = item.kind; + if let Some(first_arg_ty) = sig.decl.inputs.iter().next(); + let first_arg_span = first_arg_ty.span; + let first_arg_ty = hir_ty_to_ty(cx.tcx, first_arg_ty); + let self_ty = TraitRef::identity(cx.tcx, item.hir_id.owner.to_def_id()).self_ty(); + + then { + lint_wrong_self_convention(cx, &item.ident.name.as_str(), false, self_ty, first_arg_ty, first_arg_span); + } + } + if_chain! { - if !in_external_macro(cx.tcx.sess, item.span); if item.ident.name == sym::new; if let TraitItemKind::Fn(_, _) = item.kind; let ret_ty = return_ty(cx, item.hir_id); @@ -1757,6 +1755,39 @@ impl<'tcx> LateLintPass<'tcx> for Methods { extract_msrv_attr!(LateContext); } +fn lint_wrong_self_convention<'tcx>( + cx: &LateContext<'tcx>, + item_name: &str, + is_pub: bool, + self_ty: &'tcx TyS<'tcx>, + first_arg_ty: &'tcx TyS<'tcx>, + first_arg_span: Span, +) { + let lint = if is_pub { + WRONG_PUB_SELF_CONVENTION + } else { + WRONG_SELF_CONVENTION + }; + if let Some((ref conv, self_kinds)) = &CONVENTIONS.iter().find(|(ref conv, _)| conv.check(item_name)) { + if !self_kinds.iter().any(|k| k.matches(cx, self_ty, first_arg_ty)) { + span_lint( + cx, + lint, + first_arg_span, + &format!( + "methods called `{}` usually take {}; consider choosing a less ambiguous name", + conv, + &self_kinds + .iter() + .map(|k| k.description()) + .collect::>() + .join(" or ") + ), + ); + } + } +} + /// Checks for the `OR_FUN_CALL` lint. #[allow(clippy::too_many_lines)] fn lint_or_fun_call<'tcx>( diff --git a/tests/ui/use_self.fixed b/tests/ui/use_self.fixed index ebb3aa28daf..d6a890014e6 100644 --- a/tests/ui/use_self.fixed +++ b/tests/ui/use_self.fixed @@ -71,6 +71,7 @@ mod lifetimes { mod issue2894 { trait IntoBytes { + #[allow(clippy::wrong_self_convention)] fn into_bytes(&self) -> Vec; } diff --git a/tests/ui/use_self.rs b/tests/ui/use_self.rs index 8a182192ab3..b04d9ce75b2 100644 --- a/tests/ui/use_self.rs +++ b/tests/ui/use_self.rs @@ -71,6 +71,7 @@ mod lifetimes { mod issue2894 { trait IntoBytes { + #[allow(clippy::wrong_self_convention)] fn into_bytes(&self) -> Vec; } diff --git a/tests/ui/use_self.stderr b/tests/ui/use_self.stderr index b33928597c1..80e1bfc75e8 100644 --- a/tests/ui/use_self.stderr +++ b/tests/ui/use_self.stderr @@ -37,19 +37,19 @@ LL | Foo::new() | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:89:56 + --> $DIR/use_self.rs:90:56 | LL | fn bad(foos: &[Self]) -> impl Iterator { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:104:13 + --> $DIR/use_self.rs:105:13 | LL | TS(0) | ^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:112:25 + --> $DIR/use_self.rs:113:25 | LL | fn new() -> Foo { | ^^^ help: use the applicable keyword: `Self` @@ -60,7 +60,7 @@ LL | use_self_expand!(); // Should lint in local macros = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) error: unnecessary structure name repetition - --> $DIR/use_self.rs:113:17 + --> $DIR/use_self.rs:114:17 | LL | Foo {} | ^^^ help: use the applicable keyword: `Self` @@ -71,91 +71,91 @@ LL | use_self_expand!(); // Should lint in local macros = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) error: unnecessary structure name repetition - --> $DIR/use_self.rs:148:21 + --> $DIR/use_self.rs:149:21 | LL | fn baz() -> Foo { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:149:13 + --> $DIR/use_self.rs:150:13 | LL | Foo {} | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:136:29 + --> $DIR/use_self.rs:137:29 | LL | fn bar() -> Bar { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:137:21 + --> $DIR/use_self.rs:138:21 | LL | Bar { foo: Foo {} } | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:166:21 + --> $DIR/use_self.rs:167:21 | LL | let _ = Enum::B(42); | ^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:167:21 + --> $DIR/use_self.rs:168:21 | LL | let _ = Enum::C { field: true }; | ^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:168:21 + --> $DIR/use_self.rs:169:21 | LL | let _ = Enum::A; | ^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:199:13 + --> $DIR/use_self.rs:200:13 | LL | nested::A::fun_1(); | ^^^^^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:200:13 + --> $DIR/use_self.rs:201:13 | LL | nested::A::A; | ^^^^^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:202:13 + --> $DIR/use_self.rs:203:13 | LL | nested::A {}; | ^^^^^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:221:13 + --> $DIR/use_self.rs:222:13 | LL | TestStruct::from_something() | ^^^^^^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:235:25 + --> $DIR/use_self.rs:236:25 | LL | async fn g() -> S { | ^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:236:13 + --> $DIR/use_self.rs:237:13 | LL | S {} | ^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:240:16 + --> $DIR/use_self.rs:241:16 | LL | &p[S::A..S::B] | ^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:240:22 + --> $DIR/use_self.rs:241:22 | LL | &p[S::A..S::B] | ^ help: use the applicable keyword: `Self` diff --git a/tests/ui/wrong_self_convention.rs b/tests/ui/wrong_self_convention.rs index f44305d7e48..5282eba74fd 100644 --- a/tests/ui/wrong_self_convention.rs +++ b/tests/ui/wrong_self_convention.rs @@ -88,3 +88,52 @@ mod issue4037 { } } } + +// Lint also in trait definition (see #6307) +mod issue6307 { + trait T: Sized { + fn as_i32(self) {} + fn as_u32(&self) {} + fn into_i32(&self) {} + fn into_u32(self) {} + fn is_i32(self) {} + fn is_u32(&self) {} + fn to_i32(self) {} + fn to_u32(&self) {} + fn from_i32(self) {} + // check whether the lint can be allowed at the function level + #[allow(clippy::wrong_self_convention)] + fn from_cake(self) {} + + // test for false positives + fn as_(self) {} + fn into_(&self) {} + fn is_(self) {} + fn to_(self) {} + fn from_(self) {} + fn to_mut(&mut self) {} + } + + trait U { + fn as_i32(self); + fn as_u32(&self); + fn into_i32(&self); + fn into_u32(self); + fn is_i32(self); + fn is_u32(&self); + fn to_i32(self); + fn to_u32(&self); + fn from_i32(self); + // check whether the lint can be allowed at the function level + #[allow(clippy::wrong_self_convention)] + fn from_cake(self); + + // test for false positives + fn as_(self); + fn into_(&self); + fn is_(self); + fn to_(self); + fn from_(self); + fn to_mut(&mut self); + } +} diff --git a/tests/ui/wrong_self_convention.stderr b/tests/ui/wrong_self_convention.stderr index ef3ad73ebc7..86467eb0fc7 100644 --- a/tests/ui/wrong_self_convention.stderr +++ b/tests/ui/wrong_self_convention.stderr @@ -72,5 +72,65 @@ error: methods called `from_*` usually take no self; consider choosing a less am LL | pub fn from_i64(self) {} | ^^^^ -error: aborting due to 12 previous errors +error: methods called `as_*` usually take self by reference or self by mutable reference; consider choosing a less ambiguous name + --> $DIR/wrong_self_convention.rs:95:19 + | +LL | fn as_i32(self) {} + | ^^^^ + +error: methods called `into_*` usually take self by value; consider choosing a less ambiguous name + --> $DIR/wrong_self_convention.rs:97:21 + | +LL | fn into_i32(&self) {} + | ^^^^^ + +error: methods called `is_*` usually take self by reference or no self; consider choosing a less ambiguous name + --> $DIR/wrong_self_convention.rs:99:19 + | +LL | fn is_i32(self) {} + | ^^^^ + +error: methods called `to_*` usually take self by reference; consider choosing a less ambiguous name + --> $DIR/wrong_self_convention.rs:101:19 + | +LL | fn to_i32(self) {} + | ^^^^ + +error: methods called `from_*` usually take no self; consider choosing a less ambiguous name + --> $DIR/wrong_self_convention.rs:103:21 + | +LL | fn from_i32(self) {} + | ^^^^ + +error: methods called `as_*` usually take self by reference or self by mutable reference; consider choosing a less ambiguous name + --> $DIR/wrong_self_convention.rs:118:19 + | +LL | fn as_i32(self); + | ^^^^ + +error: methods called `into_*` usually take self by value; consider choosing a less ambiguous name + --> $DIR/wrong_self_convention.rs:120:21 + | +LL | fn into_i32(&self); + | ^^^^^ + +error: methods called `is_*` usually take self by reference or no self; consider choosing a less ambiguous name + --> $DIR/wrong_self_convention.rs:122:19 + | +LL | fn is_i32(self); + | ^^^^ + +error: methods called `to_*` usually take self by reference; consider choosing a less ambiguous name + --> $DIR/wrong_self_convention.rs:124:19 + | +LL | fn to_i32(self); + | ^^^^ + +error: methods called `from_*` usually take no self; consider choosing a less ambiguous name + --> $DIR/wrong_self_convention.rs:126:21 + | +LL | fn from_i32(self); + | ^^^^ + +error: aborting due to 22 previous errors