Skip to content

Commit

Permalink
wrong_self_convention: to_ respects Copy types
Browse files Browse the repository at this point in the history 
More details:
https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv
  • Loading branch information
mgacek committed Mar 17, 2021
1 parent 56161b2 commit e1abe44
Show file tree
Hide file tree
Showing 8 changed files with 143 additions and 90 deletions.
39 changes: 24 additions & 15 deletions clippy_lints/src/methods/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand All @@ -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 &&
Expand Down Expand Up @@ -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());

Expand Down Expand Up @@ -1947,7 +1955,8 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
false,
self_ty,
first_arg_ty,
first_arg_span
first_arg_span,
true
);
}
}
Expand Down
74 changes: 42 additions & 32 deletions clippy_lints/src/methods/wrong_self_convention.rs
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -9,32 +10,40 @@ 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]),
(&[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]),

// 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 {
Eq(&'static str),
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,
}
}
}
Expand All @@ -46,6 +55,10 @@ impl fmt::Display for Convention {
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::IsSelfTypeCopy(is_true) => format!("self type is {} Copy", if is_true { "" } else { "not" }).fmt(f),
Self::ImplementsTrait(is_true) => {
format!("Method {} implement a trait", if is_true { "" } else { "do not" }).fmt(f)
},
}
}
}
Expand All @@ -57,45 +70,42 @@ 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::<Vec<_>>()
.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(format!("`{}`", &conv.to_string()))
}
})
.collect::<Vec<_>>()
.join(" and ");

format!("methods called like this: ({})", &s)
}
format!("methods with the following characteristics: ({})", &s)
} else {
format!("methods called `{}`", &conventions[0])
}
Expand Down
1 change: 1 addition & 0 deletions tests/ui/use_self.fixed
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ mod issue2894 {
}

// This should not be linted
#[allow(clippy::wrong_self_convention)]
impl IntoBytes for u8 {
fn to_bytes(&self) -> Vec<u8> {
vec![*self]
Expand Down
1 change: 1 addition & 0 deletions tests/ui/use_self.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ mod issue2894 {
}

// This should not be linted
#[allow(clippy::wrong_self_convention)]
impl IntoBytes for u8 {
fn to_bytes(&self) -> Vec<u8> {
vec![*self]
Expand Down
46 changes: 23 additions & 23 deletions tests/ui/use_self.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,139 +37,139 @@ LL | Foo::new()
| ^^^ help: use the applicable keyword: `Self`

error: unnecessary structure name repetition
--> $DIR/use_self.rs:93:24
--> $DIR/use_self.rs:94:24
|
LL | fn bad(foos: &[Foo]) -> impl Iterator<Item = &Foo> {
| ^^^ help: use the applicable keyword: `Self`

error: unnecessary structure name repetition
--> $DIR/use_self.rs:93:55
--> $DIR/use_self.rs:94:55
|
LL | fn bad(foos: &[Foo]) -> impl Iterator<Item = &Foo> {
| ^^^ help: use the applicable keyword: `Self`

error: unnecessary structure name repetition
--> $DIR/use_self.rs:108:13
--> $DIR/use_self.rs:109:13
|
LL | TS(0)
| ^^ help: use the applicable keyword: `Self`

error: unnecessary structure name repetition
--> $DIR/use_self.rs:143:29
--> $DIR/use_self.rs:144:29
|
LL | fn bar() -> Bar {
| ^^^ help: use the applicable keyword: `Self`

error: unnecessary structure name repetition
--> $DIR/use_self.rs:144:21
--> $DIR/use_self.rs:145:21
|
LL | Bar { foo: Foo {} }
| ^^^ help: use the applicable keyword: `Self`

error: unnecessary structure name repetition
--> $DIR/use_self.rs:155:21
--> $DIR/use_self.rs:156:21
|
LL | fn baz() -> Foo {
| ^^^ help: use the applicable keyword: `Self`

error: unnecessary structure name repetition
--> $DIR/use_self.rs:156:13
--> $DIR/use_self.rs:157:13
|
LL | Foo {}
| ^^^ help: use the applicable keyword: `Self`

error: unnecessary structure name repetition
--> $DIR/use_self.rs:173:21
--> $DIR/use_self.rs:174:21
|
LL | let _ = Enum::B(42);
| ^^^^ help: use the applicable keyword: `Self`

error: unnecessary structure name repetition
--> $DIR/use_self.rs:174:21
--> $DIR/use_self.rs:175:21
|
LL | let _ = Enum::C { field: true };
| ^^^^ help: use the applicable keyword: `Self`

error: unnecessary structure name repetition
--> $DIR/use_self.rs:175:21
--> $DIR/use_self.rs:176:21
|
LL | let _ = Enum::A;
| ^^^^ help: use the applicable keyword: `Self`

error: unnecessary structure name repetition
--> $DIR/use_self.rs:217:13
--> $DIR/use_self.rs:218:13
|
LL | nested::A::fun_1();
| ^^^^^^^^^ help: use the applicable keyword: `Self`

error: unnecessary structure name repetition
--> $DIR/use_self.rs:218:13
--> $DIR/use_self.rs:219:13
|
LL | nested::A::A;
| ^^^^^^^^^ help: use the applicable keyword: `Self`

error: unnecessary structure name repetition
--> $DIR/use_self.rs:220:13
--> $DIR/use_self.rs:221:13
|
LL | nested::A {};
| ^^^^^^^^^ help: use the applicable keyword: `Self`

error: unnecessary structure name repetition
--> $DIR/use_self.rs:239:13
--> $DIR/use_self.rs:240:13
|
LL | TestStruct::from_something()
| ^^^^^^^^^^ help: use the applicable keyword: `Self`

error: unnecessary structure name repetition
--> $DIR/use_self.rs:253:25
--> $DIR/use_self.rs:254:25
|
LL | async fn g() -> S {
| ^ help: use the applicable keyword: `Self`

error: unnecessary structure name repetition
--> $DIR/use_self.rs:254:13
--> $DIR/use_self.rs:255:13
|
LL | S {}
| ^ help: use the applicable keyword: `Self`

error: unnecessary structure name repetition
--> $DIR/use_self.rs:258:16
--> $DIR/use_self.rs:259:16
|
LL | &p[S::A..S::B]
| ^ help: use the applicable keyword: `Self`

error: unnecessary structure name repetition
--> $DIR/use_self.rs:258:22
--> $DIR/use_self.rs:259:22
|
LL | &p[S::A..S::B]
| ^ help: use the applicable keyword: `Self`

error: unnecessary structure name repetition
--> $DIR/use_self.rs:281:29
--> $DIR/use_self.rs:282:29
|
LL | fn foo(value: T) -> Foo<T> {
| ^^^^^^ help: use the applicable keyword: `Self`

error: unnecessary structure name repetition
--> $DIR/use_self.rs:282:13
--> $DIR/use_self.rs:283:13
|
LL | Foo { value }
| ^^^ help: use the applicable keyword: `Self`

error: unnecessary structure name repetition
--> $DIR/use_self.rs:319:21
--> $DIR/use_self.rs:320:21
|
LL | type From = T::From;
| ^^^^^^^ help: use the applicable keyword: `Self`

error: unnecessary structure name repetition
--> $DIR/use_self.rs:320:19
--> $DIR/use_self.rs:321:19
|
LL | type To = T::To;
| ^^^^^ help: use the applicable keyword: `Self`

error: unnecessary structure name repetition
--> $DIR/use_self.rs:453:13
--> $DIR/use_self.rs:454:13
|
LL | A::new::<submod::B>(submod::B {})
| ^ help: use the applicable keyword: `Self`
Expand Down
Loading

0 comments on commit e1abe44

Please sign in to comment.