New lint: [derive_partial_eq_without_eq]

[nsunderland1]

Introduces a new lint, [derive_partial_eq_without_eq].

See: #1781 (doesn't close it though).

changelog: add lint [derive_partial_eq_without_eq]

[rust-highfive]

r? @giraffate

(rust-highfive has picked a reviewer for you, use r? to override)

[nsunderland1]

Wasn't sure about the right place to put this.

[giraffate]

@rust-lang/clippy @dswij @Alexendoo @Jarcho Which category do you think is appropriate? I think suspicious looks good, but the original code isn't wrong.

[Jarcho]

Given that it's more an API guideline, style is the closest fit. It doesn't invite misuse, nor is there anything wrong with it other than a potential annoyance, so suspicious seems a little to strong a category.

[nsunderland1]

So this might be me being silly, but when I see a type that implements PartialEq but not Eq, I kind of read that as an assertion that "This is not an equivalence relation, it's only a partial equivalence relation". In that sense I see it as suspicious to not implement Eq when you do have an equivalence relation, but that's pretty pedantic. I think you're correct in saying that this is primarily a question of adhering to API guidelines.

[Jarcho]

The only reason not to implement Eq when it could be derived (assuming PartialEq is derived), other than laziness, is when there's the potential to extend the type in a way which could not implement Eq. This is not most cases, but it is a possibility. This possibility is, to me, enough of a reason to not categorize it as suspicious.

If not deriving Eq would cause the type to be misused, then suspicious would be a perfectly reasonable category. I think all our suspicious lints basically point out things which are easy to miswrite, or easy to misread in such a way that it creates a bug, but they are still reasonablish things to type. I haven't actually checked through all the suspicious lints, so that might not actually describe all suspicious lints.

[flip1995]

Completely agree with @Jarcho on this one. sus lints are for code that is "most likely wrong or useless". Not implementing Eq is not wrong, at least there is no way for Clippy to tell if it is intentional (possibly expanding the struct in the future). With that the API concern is the one left and for those, style is the best group.

[giraffate]

Thanks for the comments!

[nsunderland1]

Tentatively put the lint under style in the latest revision. As a side effect of this, I had to fix cases where tests were hitting the lint. In some of those I just added the derive, unless the tests cared specifically about whether Eq was implemented, in which case I just told them to ignore the lint. Tried to be fairly conservative there.

Also had to fix a few places in clippy where trivial enums were not being marked as Eq, and added a rustfix test for the lint.

[nsunderland1]

Took me a couple of tries to realize that cargo test --features deny-warnings,internal would catch everything for me :) Couple more places where this required an Eq added in clippy internals, but all the cases were pretty straightforward.

[giraffate]

@bors r+

Thanks!

[bors]

📌 Commit fe84ff3 has been approved by giraffate

[bors]

⌛ Testing commit fe84ff3 with merge d422baa...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: giraffate
Pushing d422baa to master...