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

wrong_self_convention: fix FP inside trait impl for to_* method taking &self #7002

Merged
merged 1 commit into from
Apr 1, 2021
Merged

wrong_self_convention: fix FP inside trait impl for to_* method taking &self #7002

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 9 additions & 1 deletion clippy_lints/src/methods/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,13 @@ declare_clippy_lint! {
/// |`to_` | not `_mut` |`self` | `Copy` |
/// |`to_` | not `_mut` |`&self` | not `Copy` |
///
/// Note: Clippy doesn't trigger methods with `to_` prefix in:
/// - Traits definition.
/// Clippy can not tell if a type that implements a trait is `Copy` or not.
/// - Traits implementation, when `&self` is taken.
/// The method signature is controlled by the trait and often `&self` is required for all types that implement the trait
/// (see e.g. the `std::string::ToString` trait).
///
/// Please find more info here:
/// https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv
///
Expand Down Expand Up @@ -1850,7 +1857,6 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
let self_ty = cx.tcx.type_of(item.def_id);

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;
if let Some(first_arg) = iter_input_pats(&sig.decl, cx.tcx.hir().body(id)).next();
Expand Down Expand Up @@ -1902,6 +1908,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
self_ty,
first_arg_ty,
first_arg.pat.span,
implements_trait,
false
);
}
Expand Down Expand Up @@ -1972,6 +1979,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
self_ty,
first_arg_ty,
first_arg_span,
false,
true
);
}
Expand Down
32 changes: 25 additions & 7 deletions clippy_lints/src/methods/wrong_self_convention.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,10 @@ const CONVENTIONS: [(&[Convention], &[SelfKind]); 9] = [

// 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]),
(&[Convention::StartsWith("to_"), Convention::NotEndsWith("_mut"), Convention::IsSelfTypeCopy(false),
Convention::IsTraitItem(false)], &[SelfKind::Ref]),
(&[Convention::StartsWith("to_"), Convention::NotEndsWith("_mut"), Convention::IsSelfTypeCopy(true),
Convention::IsTraitItem(false), Convention::ImplementsTrait(false)], &[SelfKind::Value]),
];

enum Convention {
Expand All @@ -32,18 +34,27 @@ enum Convention {
NotEndsWith(&'static str),
IsSelfTypeCopy(bool),
ImplementsTrait(bool),
IsTraitItem(bool),
}

impl Convention {
#[must_use]
fn check<'tcx>(&self, cx: &LateContext<'tcx>, self_ty: &'tcx TyS<'tcx>, other: &str, is_trait_def: bool) -> bool {
fn check<'tcx>(
&self,
cx: &LateContext<'tcx>,
self_ty: &'tcx TyS<'tcx>,
other: &str,
implements_trait: bool,
is_trait_item: 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(cx, self_ty, other, is_trait_def),
Self::NotEndsWith(this) => !Self::EndsWith(this).check(cx, self_ty, other, implements_trait, is_trait_item),
Self::IsSelfTypeCopy(is_true) => is_true == is_copy(cx, self_ty),
Self::ImplementsTrait(is_true) => is_true == is_trait_def,
Self::ImplementsTrait(is_true) => is_true == implements_trait,
Self::IsTraitItem(is_true) => is_true == is_trait_item,
}
}
}
Expand All @@ -60,19 +71,25 @@ impl fmt::Display for Convention {
},
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)
format!("method{} implement{} a trait", negation, s_suffix).fmt(f)
},
Self::IsTraitItem(is_true) => {
let suffix = if is_true { " is" } else { " is not" };
format!("method{} a trait item", suffix).fmt(f)
},
}
}
}

#[allow(clippy::too_many_arguments)]
pub(super) fn check<'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,
implements_trait: bool,
is_trait_item: bool,
) {
let lint = if is_pub {
Expand All @@ -83,7 +100,7 @@ pub(super) fn check<'tcx>(
if let Some((conventions, self_kinds)) = &CONVENTIONS.iter().find(|(convs, _)| {
convs
.iter()
.all(|conv| conv.check(cx, self_ty, item_name, is_trait_item))
.all(|conv| conv.check(cx, self_ty, item_name, implements_trait, is_trait_item))
}) {
if !self_kinds.iter().any(|k| k.matches(cx, self_ty, first_arg_ty)) {
let suggestion = {
Expand All @@ -99,6 +116,7 @@ pub(super) fn check<'tcx>(
.filter_map(|conv| {
if (cut_ends_with_conv && matches!(conv, Convention::NotEndsWith(_)))
|| matches!(conv, Convention::ImplementsTrait(_))
|| matches!(conv, Convention::IsTraitItem(_))
{
None
} else {
Expand Down
9 changes: 2 additions & 7 deletions tests/ui/wrong_self_convention.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,15 +165,10 @@ mod issue6307 {
}

mod issue6727 {
trait ToU64 {
fn to_u64(self) -> u64;
fn to_u64_v2(&self) -> u64;
}

#[derive(Clone, Copy)]
struct FooCopy;

impl ToU64 for FooCopy {
impl FooCopy {
fn to_u64(self) -> u64 {
1
}
Expand All @@ -185,7 +180,7 @@ mod issue6727 {

struct FooNoCopy;

impl ToU64 for FooNoCopy {
impl FooNoCopy {
// trigger lint
fn to_u64(self) -> u64 {
2
Expand Down
4 changes: 2 additions & 2 deletions tests/ui/wrong_self_convention.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,15 +176,15 @@ 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
--> $DIR/wrong_self_convention.rs:176: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
--> $DIR/wrong_self_convention.rs:185:19
|
LL | fn to_u64(self) -> u64 {
| ^^^^
Expand Down
32 changes: 32 additions & 0 deletions tests/ui/wrong_self_convention2.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// edition:2018
#![warn(clippy::wrong_self_convention)]
#![warn(clippy::wrong_pub_self_convention)]
#![allow(dead_code)]

fn main() {}

mod issue6983 {
pub struct Thing;
pub trait Trait {
fn to_thing(&self) -> Thing;
}

impl Trait for u8 {
// don't trigger, e.g. `ToString` from `std` requires `&self`
fn to_thing(&self) -> Thing {
Thing
}
}

trait ToU64 {
fn to_u64(self) -> u64;
}

struct FooNoCopy;
// trigger lint
impl ToU64 for FooNoCopy {
fn to_u64(self) -> u64 {
2
}
}
}
11 changes: 11 additions & 0 deletions tests/ui/wrong_self_convention2.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
error: methods with the following characteristics: (`to_*` and `self` type is not `Copy`) usually take `self` by reference
--> $DIR/wrong_self_convention2.rs:28:19
|
LL | fn to_u64(self) -> u64 {
Comment on lines +2 to +4
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still triggers on the trait implementation, not on the trait definition. Is this intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I put that intentionally.
The issue pointed out that a trait method signature is controlled by the trait, and there might be other types implementing the same trait that are not Copy. That's why we should allow a Copy type to take &self instead of just self in the case of to_* method when it comes from a trait.
But, the other way around, where we have a to_* method from a trait that requires self and the type implementing it is not Copy then probably that's not something desirable. As I see (e.g. based on to_string(&self) from std::string::ToString trait) if you expect types implementing your trait to not always be Copy you should mark your to_* method as &self. That's why I thought a lint, in this case, might be useful. Having said that, I'm not super strong on that, so if someone thinks differently that we should also allow this case, I can change that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, but in order to change this, you would also have to change the trait definition, so you could just as well directly lint on the trait definition. Do you have a reason why it shouldn't lint on trait definitions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One reason is that from trait definition alone you don't know what types will implement it. It might be that a particular trait is implemented only by Copy types. E.g. as in the #6727

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Do you think we should stop linting completely then, if traits are involved?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you motivated to put some more work into this, so that this lint doesn't trigger on the to variant in trait definitions and implementations anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sure, I can do that. Do you think there might be some valid use-cases for that scenario ? (non-Copy type implementing trait's to_* method taking self). Or is it just better to stay less stringent?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sure, I can do that.

Thanks!

Do you think there might be some valid use-cases for that scenario ?

Yes, I think there is value to a lint saying "your trying to implement a trait that is probably meant for a Copy type, but you're implementing it on a non-Copy type". But I don't think we should do this in this lint, but rather make a new allow-by-default (pedantic) lint for this. I don't think Clippy should warn by default for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, agree that having this as allow-by-default would be better!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR: #7182

| ^^^^
|
= note: `-D clippy::wrong-self-convention` implied by `-D warnings`
= help: consider choosing a less ambiguous name

error: aborting due to previous error