new lint: inherent methods that should be trait impls (fixes #218) #228
Conversation
| } | ||
|
|
||
| impl SelfKind { | ||
| fn check(&self, slf: &ExplicitSelf_) -> bool { |
There was a problem hiding this comment.
Perhaps 'matches' instead of 'check'?
birkenfeld
force-pushed
the
should_impl
branch
from
847277e to
050a1b3
Compare
|
Addressed review comments and added quite a few more traits/methods. |
|
There's still my comment from an hour ago, I think the lint should not fire if the trait is already implemented. Otherwise LGTM. |
I thought you asked for the test I added. Other than that, I don't know what you mean. |
| impl T { | ||
| fn add(self, other: T) -> T { self } //~ERROR defining a method called `add` | ||
| fn drop(&mut self) { } //~ERROR defining a method called `drop` | ||
|
|
There was a problem hiding this comment.
Add a fn mul(self, other: T) -> T { self } // no error, because trait impl exists here.
There was a problem hiding this comment.
Ah, I see. But does that make any sense? Now you have defined two mul methods for the type, one of which is used for * and one for explicit .mul() calls. Is there a use case for this?
I know this can't be disallowed by the compiler, since it doesn't know about all the trait impls when compiling a crate, but I wouldn't add extra complexity to the lint just to not warn on a case that should not be written anyway.
There was a problem hiding this comment.
Yes, it should warn, but it should be a different warning, namely that it has both a .mul(_) method and a Mul impl and that may confuse users.
There was a problem hiding this comment.
But that does not belong to this lint.
There was a problem hiding this comment.
Ok, that's an acceptable position to take. In that case we should make sure that the message clearly conveys that the trait impl should be the only single implementation of this function.
birkenfeld
force-pushed
the
should_impl
branch
from
050a1b3 to
42f52bb
Compare
birkenfeld
force-pushed
the
should_impl
branch
from
42f52bb to
56e8db4
Compare
|
LGTM. We could opt to add r+, but I'll let @Manishearth merge. |
No description provided.