Lint also in trait def for wrong_self_convention

[ThibsG]

Extends wrong_self_convention to lint also in trait definition.

By the way, I think the wrong_pub_self_convention example is misleading.
On playground, it fires wrong_self_convention, so the example (or the lint maybe?) needs to be reworked.
The difference with wrong_self_convention example is mainly the pub keyword on the method as_str, but the lint doesn't use the function visibility as condition to choose which lint to fire (in fact it uses the visibility of the impl item).

fixes: #6307

changelog: Lint wrong_self_convention lint in trait def also

[rust-highfive]

r? @ebroto

(rust_highfive has picked a reviewer for you, use r? to override)

[ebroto]

I'm hesitating about this... do you think we should only lint if Sized is a supertrait of the given trait? Otherwise it would not make sense to refer to self in the error message; OTOH the conventions probably still hold and we should not use those names if we are not following them...

---

BTW sorry for the long delay in reviewing, I had almost no free time lately

[ebroto]

By the way, I think the wrong_pub_self_convention example is misleading.
On playground, it fires wrong_self_convention, so the example (or the lint maybe?) needs to be reworked.
The difference with wrong_self_convention example is mainly the pub keyword on the method as_str, but the lint doesn't use the >function visibility as condition to choose which lint to fire (in fact it uses the visibility of the impl item).

Would you mind opening an issue about it? It would probably make sense to also look into the history of the lints, what were the reasons to split them (if that was the case)? It seems there are no tests for the pub variant, it would make sense to mention that in the issue.

Thanks!

[ebroto]

Hey @ThibsG, something I can help with?

[ThibsG]

I am short on time recently, but I want to find some time to finish this soonish.
Btw it doesn't trigger when methods are only declared so more fun is waiting for me 😉

[ebroto]

I am short on time recently, but I want to find some time to finish this soonish.

No problem!

Btw it doesn't trigger when methods are only declared so more fun is waiting for me wink

😆

[ebroto]

BTW I checked and it seems to properly lint in required methods. Maybe you changed one of the negative tests? I tried changing the from_i32 one to:

fn from_i32(self);

[ThibsG]

You're right, the problem was between the chair and the desk, now it's all good by running the proper test 😅

[ebroto]

LGTM mostly, but I'm hesitant about assuming that not linting in traits was an oversight.

This lint is one of the oldest ones, and I could not find the original issue or PR that added it to double check if there was a reason to avoid linting in trait defs.

@llogiq I know it's been a while 😄 but do you remember if this was intentional?

[ebroto]

@bors r+

Thanks!

[bors]

📌 Commit 90a16e4 has been approved by ebroto

[bors]

⌛ Testing commit 90a16e4 with merge 9f9e9f7...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: ebroto
Pushing 9f9e9f7 to master...

[g2p]

Hey @ThibsG !
It seems that when refactoring wrong_self_convention, you dropped a refinement that allowed passing Copy types by value; see #273 .
Could you restore this?

[ThibsG]

Oh you right, thanks @g2p for noting this.
I wasn't aware of this Copy exception.
I will fix this in coming days 😉

[ThibsG]

I double checked the changes made here and this PR doesn't modify the original behavior of the lint.
Particularly, when using #[derive(Copy)], the lint keeps quiet, as tested here.
Maybe the problem is coming from a particular situation I didn't see, so do not hesitate to reach me out if you have a precise case which was working earlier.

Not really interesting for your mentioned PR, but I agree one test could be added on the trait side, with a trait definition like trait C: Copy.
In this case the lint triggers both on from_i32(self) which is expected (same behavior with the derive macro test) and on into_i32(self) which is unexpected (not triggering with the derive macro test).
EDIT: this wasn't exactly the same test, one is with self, the other one with &self, so it's all good, I will just add this test in a future PR.