wrong_self_convention: fix FP inside trait impl for to_* method taking &self
#7002
Conversation
|
r? @phansch (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
mgacek8
force-pushed
the
issue6983_wrong_self_convention_inside_trait_impls
branch
from
10d9622
to
409cbe8
Compare
When the `to_*` method takes `&self` and it is a trait implementation, we don't trigger the lint.
mgacek8
force-pushed
the
issue6983_wrong_self_convention_inside_trait_impls
branch
from
409cbe8
to
6966c78
Compare
|
@bors r+ thanks! |
|
📌 Commit 6966c78 has been approved by |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
| --> $DIR/wrong_self_convention2.rs:28:19 | ||
| | | ||
| LL | fn to_u64(self) -> u64 { |
There was a problem hiding this comment.
This still triggers on the trait implementation, not on the trait definition. Is this intended?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Makes sense. Do you think we should stop linting completely then, if traits are involved?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ok, agree that having this as allow-by-default would be better!
…ip1995 For `to_*` variant don't lint in trait impl taking `self` when non-`Copy` type Lint name: `wrong_self_convention`. It relaxes rules for `to_*` variant, so it doesn't lint in trait definitions and implementations anymore. Although, non-`Copy` type implementing trait's `to_*` method taking `self` feels not good (consumes ownership, so should be rather named `into_`), it would be better if this case was a pedantic lint (allow-by-default) instead. More information in the discussion with `@flip1995` [here](#7002 (comment)) changelog: `wrong_self_convention`: For `to_*` variant don't lint in trait impl taking `self` when non-`Copy` type r? `@flip1995`
fixes #6983
changelog:
wrong_self_convention: fix FP inside trait impl forto_*method taking&self