Auto merge of #6316 - ThibsG:WrongSelfConventionTraitDef, r=ebroto
Lint also in trait def for `wrong_self_convention` Extends `wrong_self_convention` to lint also in trait definition. By the way, I think the `wrong_pub_self_convention` [example](dd826b4626/clippy_lints/src/methods/mod.rs (L197)) is misleading. On [playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=32615ab3f6009e7e42cc3754be0ca17f), it fires `wrong_self_convention`, so the example (or the lint maybe?) needs to be reworked. The difference with `wrong_self_convention` [example](dd826b4626/clippy_lints/src/methods/mod.rs (L172)) is mainly the `pub` keyword on the method `as_str`, but the lint doesn't use the function visibility as condition to choose which lint to fire (in fact it uses the visibility of the impl item). fixes: #6307 changelog: Lint `wrong_self_convention` lint in trait def also
This commit is contained in:
bors
commit
9f9e9f7e52
@ -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::<Vec<_>>()
|
||||
.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::<Vec<_>>()
|
||||
.join(" or ")
|
||||
),
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Checks for the `OR_FUN_CALL` lint.
|
||||
#[allow(clippy::too_many_lines)]
|
||||
fn lint_or_fun_call<'tcx>(
|
||||
|
||||
@ -71,6 +71,7 @@ mod lifetimes {
|
||||
|
||||
mod issue2894 {
|
||||
trait IntoBytes {
|
||||
#[allow(clippy::wrong_self_convention)]
|
||||
fn into_bytes(&self) -> Vec<u8>;
|
||||
}
|
||||
|
||||
|
||||
@ -71,6 +71,7 @@ mod lifetimes {
|
||||
|
||||
mod issue2894 {
|
||||
trait IntoBytes {
|
||||
#[allow(clippy::wrong_self_convention)]
|
||||
fn into_bytes(&self) -> Vec<u8>;
|
||||
}
|
||||
|
||||
|
||||
@ -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<Item = &Foo> {
|
||||
| ^^^ 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`
|
||||
|
||||
@ -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);
|
||||
}
|
||||
}
|
||||
|
||||
@ -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
|
||||
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user