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.

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

derive_partial_eq_without_eq should acknowledge the constraint it adds #9063

Open
Tracked by #79
kpreid opened this issue Jun 29, 2022 · 6 comments
Open
Tracked by #79

derive_partial_eq_without_eq should acknowledge the constraint it adds #9063

kpreid opened this issue Jun 29, 2022 · 6 comments
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have L-nursery Lint: Currently in the nursery group

Comments

@kpreid
Copy link
Contributor

kpreid commented Jun 29, 2022

Summary

The derive_partial_eq_without_eq lint and its documentation recommends deriving Eq whenever PartialEq is implemented on a public type. However, doing so may constrain the evolution of a library: adding derive(Eq) prevents later adding a private field, or public field or variant in a non_exhaustive enum or struct, which contains an !Eq type such as f64.

Notably, Clippy reported this lint on my library's non-exhaustive error enums — types that are more likely than most to gain new variants with new kinds of data.

At a minimum, the lint documentation should acknowledge that adding Eq constrains future choices.

Lint Name

derive_partial_eq_without_eq

Reproducer

I tried this code:

#[derive(Clone, Debug, PartialEq)]
pub struct Value {
    x: bool,
    // y: f64,            // adding this line would not be a breaking change, but it conflicts with `derive(Eq)`
}

I saw this happen:

warning: you are deriving `PartialEq` and can implement `Eq`
 --> src/lib.rs:1:24
  |
1 | #[derive(Clone, Debug, PartialEq)]
  |                        ^^^^^^^^^ help: consider deriving `Eq` as well: `PartialEq, Eq`
  |
  = note: `#[warn(clippy::derive_partial_eq_without_eq)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#derive_partial_eq_without_eq

Version

Tested with clippy 0.1.63 (2022-06-27 2f3ddd9) on Rust Playground

(The original detection was with `rustc 1.64.0-nightly (c80c4b8fd 2022-06-26)` — sorry, neither my CI nor the Playground provides `rustc -Vv` output)

Additional Labels

No response

@kpreid kpreid added C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have labels Jun 29, 2022
@mickvangelderen
Copy link
Contributor

I do not think that the benefit of having people consider also implementing Eq outweighs everyone making deliberate design choices around their public API having to disable this linting rule.

At the very least, it should not suggest adding Eq for public types.

@Enet4
Copy link
Contributor

Enet4 commented Aug 29, 2022

I came here after hearing criticism over this lint. I have to agree that it feels like a mistake having it as warn by default. It introduces unnecessary noise to packages choosing to be more conservative about what impls to expose.

Part of me suspects that the C-COMMON-TRAITS had some influence on how it was raised, considering that it advocates for making types implement as many traits as it can. The real design decision criterion is not that clear-cut, but more of a balance between early interoperability and API stability.

@rsalmei
Copy link

rsalmei commented Sep 8, 2022

I was just surprised by this.
I don't derive any traits I don't strictly need, so I can keep track of the changes that force each derive, and also to always have the fastest possible compiling experience, even the slightest increases adds up.
PS.: They're private types so I can easily change derives when/if needed.

g2p added a commit to g2p/h3 that referenced this issue Sep 22, 2022
Also enforce mod.rs module style, and allow PartialEq without Eq
due to this: rust-lang/rust-clippy#9063
g2p added a commit to g2p/h3 that referenced this issue Sep 26, 2022
Also enforce mod.rs module style, and allow PartialEq without Eq
due to this: rust-lang/rust-clippy#9063
g2p added a commit to g2p/h3 that referenced this issue Oct 6, 2022
Also enforce mod.rs module style, and allow PartialEq without Eq
due to this: rust-lang/rust-clippy#9063
g2p added a commit to g2p/h3 that referenced this issue Oct 7, 2022
Also enforce mod.rs module style, and allow PartialEq without Eq
due to this: rust-lang/rust-clippy#9063
g2p added a commit to g2p/h3 that referenced this issue Oct 7, 2022
Also enforce mod.rs module style, and allow PartialEq without Eq
due to this: rust-lang/rust-clippy#9063
ariel-miculas added a commit to ariel-miculas/puzzlefs that referenced this issue Oct 11, 2022
This may pose some problems in the future if we ever extend these
structures with fields that support PartialEq, but not Eq, such as
floating point types, see:
rust-lang/rust-clippy#9063

Signed-off-by: Ariel Miculas <amiculas@cisco.com>
g2p added a commit to g2p/h3 that referenced this issue Oct 28, 2022
Also enforce mod.rs module style, and allow PartialEq without Eq
due to this: rust-lang/rust-clippy#9063
seanmonstar pushed a commit to hyperium/h3 that referenced this issue Nov 30, 2022
Also enforce mod.rs module style, and allow PartialEq without Eq
due to this: rust-lang/rust-clippy#9063
@nyurik
Copy link
Contributor

nyurik commented May 23, 2023

So what's the best proposed solution for derive_partial_eq_without_eq lint would be?

  • Move this lint to pedantic? The lint is currently in the nursery, so this seems to already be the case?
  • Expand documentation to explain that doing so would prevent future changes to the struct - i.e. inability to add non-Eq-capable private fields (this is not exactly true because someone could implement Eq by hand, but it won't be perfectly Eq)
  • something else?
  • no action needed, can close the issue?

@kpreid
Copy link
Contributor Author

kpreid commented May 23, 2023

@nyurik As I wrote when I filed this issue:

At a minimum, the lint documentation should acknowledge that adding Eq constrains future choices.

I have no strong opinion on the other elements given that it is now not warn-by-default.

@GuillaumeGomez
Copy link
Member

I opened #12153 which should mitigate this issue a bit by non emitting the lint on non-exhaustive types.

bors added a commit that referenced this issue Jan 20, 2024
Don't emit `derive_partial_eq_without_eq` lint if the type has the `non_exhaustive` attribute

Part of #9063.

If a type has a field/variant with the `#[non_exhaustive]` attribute or the type itself has it, then do no emit the `derive_partial_eq_without_eq` lint.

changelog: Don't emit `derive_partial_eq_without_eq` lint if the type has the `non_exhaustive` attribute
bors added a commit that referenced this issue Jan 22, 2024
Don't emit `derive_partial_eq_without_eq` lint if the type has the `non_exhaustive` attribute

Part of #9063.

If a type has a field/variant with the `#[non_exhaustive]` attribute or the type itself has it, then do no emit the `derive_partial_eq_without_eq` lint.

changelog: Don't emit `derive_partial_eq_without_eq` lint if the type has the `non_exhaustive` attribute
@J-ZhengLi J-ZhengLi added the L-nursery Lint: Currently in the nursery group label Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have L-nursery Lint: Currently in the nursery group
Projects
None yet
Development

No branches or pull requests

7 participants