Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lint also in trait def for wrong_self_convention #6316

Merged
merged 5 commits into from
Dec 19, 2020
Merged

Lint also in trait def for wrong_self_convention #6316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a6bb927
Lint wrong self convention in trait also
ThibsG Nov 9, 2020
4af9382
Common function to lint wrong self convention from impl and trait def
ThibsG Nov 10, 2020
db98651
Allow `wrong_self_convention` in `use_self` test for trait def
ThibsG Nov 10, 2020
1e0f85b
Update tests/ui/use_self.rs
ThibsG Nov 24, 2020
90a16e4
Add tests for unsized trait in `wrong_self_convention` lint
ThibsG Dec 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 64 additions & 33 deletions clippy_lints/src/methods/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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);
Expand All @@ -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>(
Expand Down
1 change: 1 addition & 0 deletions tests/ui/use_self.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ mod lifetimes {

mod issue2894 {
trait IntoBytes {
#[allow(clippy::wrong_self_convention)]
fn into_bytes(&self) -> Vec<u8>;
}

Expand Down
1 change: 1 addition & 0 deletions tests/ui/use_self.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ mod lifetimes {

mod issue2894 {
trait IntoBytes {
#[allow(clippy::wrong_self_convention)]
fn into_bytes(&self) -> Vec<u8>;
}

Expand Down
38 changes: 19 additions & 19 deletions tests/ui/use_self.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand All @@ -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`
Expand All @@ -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`
Expand Down
49 changes: 49 additions & 0 deletions tests/ui/wrong_self_convention.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,3 +88,52 @@ mod issue4037 {
}
}
}

// Lint also in trait definition (see #6307)
mod issue6307 {
trait T: Sized {
ThibsG marked this conversation as resolved.
Show resolved Hide resolved
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);
}
}
Loading