document assumptions about Clone and Eq traits
#144330
Conversation
|
rustbot has assigned @Mark-Simulacrum. Use |
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
library/core/src/clone.rs
Outdated
| /// Violating this property is a logic error. The behavior resulting from a logic error is not | ||
| /// specified, but users of the trait must ensure that such logic errors do *not* result in | ||
| /// undefined behavior. This means that `unsafe` code **must not** rely on the correctness of these | ||
| /// methods. |
There was a problem hiding this comment.
It's a bit confusing to say "these methods" here IMO - maybe "this property"?
There was a problem hiding this comment.
I agree, I could change it to "... rely on this property being satisfied."
The reason behind the "correctness of these methods" wording is that it is phrased like this in the documentation on Hash and Eq, as well as the Eq and PartialEq trait documentation. I found it confusing there, too, but I thought it was just because I lacked experience with the terminology.
So, would you say the wording in these other places is confusing, too? And if so, should I make it part of this PR to change it there, too? (my first time doing a PR like this, so I'm still learning best practices)
Mark-Simulacrum
added
T-libs-api
|
Moving into libs-api queue for FCP, since this is a new property (even if likely already required implicitly) so merits some team discussion. |
Amanieu
removed
the
T-libs
| @@ -139,6 +139,30 @@ mod uninit; | |||
| /// // Note: With the manual implementations the above line will compile. | |||
| /// ``` | |||
| /// | |||
| /// ## `Clone` and `PartialEq`/`Eq` | |||
There was a problem hiding this comment.
It's unclear whether this should apply to PartialEq.
However this definitely needs to be extended to include Hash and Ord since HashMap and BTreeMap rely on this. It could also have vague wording that this may extend to other traits as well.
There was a problem hiding this comment.
It's unclear whether this should apply to PartialEq.
Do you mean this in the sense that the property cannot be required of PartialEq and Clone as we might have to deliberalty break it in some contexts? Or in the sense that we just don't know of anything that depends on this property for PartialEq?
In case of the latter, I would argue that the documentation of Clone – "creation of a duplicate value" – very strongly suggests that I get the "same" thing by cloning, and PartialEq defines "sameness" of objects. E.g., if f == 1.0_f64, I expect that f.clone() == 1.0_f64, too. But without the property x == x -> x.clone() == x, this is never explicitly guaranteed anywhere.
However this definitely needs to be extended to include Hash and Ord since HashMap and BTreeMap rely on this. It could also have vague wording that this may extend to other traits as well.
I only wrote about PartialEq/Eq because x.clone() == x already ensures that clone preserves the hash and ordering of elements, when put together with the guarantees that are required of Hash (see "Hash and Eq") and Ord (due to the conditions for PartialOrd). But I could make this interaction explicit.
There was a problem hiding this comment.
Now that I think about it, you are correct and PartialEq is the right place to introduce this requirement.
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
…=Amanieu document assumptions about `Clone` and `Eq` traits Most standard library collections break if `Clone` has a non-standard implementation which violates `x.clone() == x`. [Here](https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=b7fc6dfa8410cbb673eb8d38393d81de) the resulting broken behaviour of different collections is shown. I originally created an issue at rust-lang/hashbrown#629, but the conclusion there was that `x.clone()` resulting in an object that compares equal to the original one is probably a very universal assumption. However, this assumption is (to my knowledge) not documented anywhere. I propose to make this assumption explicit in the `Clone` trait documentation. The property that seems the most reasonable to me is the following: When implementing both `Clone` and `PartialEq`, then ```text x == x -> x.clone() == x ``` is expected to hold. This way, when also implementing `Eq`, it automatically follows that `x.clone() == x` has to hold, which should be enough for the collections to not break. At the same time, the property also works for the "normal" elements of a type with `PartialEq`. For the wording, I tried to follow the [`Hash` and `Eq`](https://doc.rust-lang.org/std/hash/trait.Hash.html#hash-and-eq) documentation. As I am fairly new to Rust, it might well be that this property cannot be generally expected – it seems reasonable to me, but any counter-examples or critique, both content- and wording-wise, would be very welcome. If the property turns out to be too general, I would suggest to at least document the assumption of `x.clone() == x` for the collections somehow. An additional thought of mine: If it is indeed generally expected that `x == x -> x.clone() == x`, then, for the sake of completeness, one could also define that `x != x -> y != y for y = x.clone()` should hold, i.e., that an object that did not compare equal to itself before cloning, should also not compare equal to itself afterwards.
Rollup of 7 pull requests Successful merges: - #143949 (Constify remaining traits/impls for `const_ops`) - #144330 (document assumptions about `Clone` and `Eq` traits) - #144350 (std: sys: io: io_slice: Add UEFI types) - #144558 (Point at the `Fn()` or `FnMut()` bound that coerced a closure, which caused a move error) - #145149 (Make config method invoke inside parse use dwn_ctx) - #145227 (Tweak spans providing type context on errors when involving macros) - #145228 (Remove unnecessary parentheses in `assert!`s) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #144330 - gewitternacht:document-clone-eq, r=Amanieu document assumptions about `Clone` and `Eq` traits Most standard library collections break if `Clone` has a non-standard implementation which violates `x.clone() == x`. [Here](https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=b7fc6dfa8410cbb673eb8d38393d81de) the resulting broken behaviour of different collections is shown. I originally created an issue at rust-lang/hashbrown#629, but the conclusion there was that `x.clone()` resulting in an object that compares equal to the original one is probably a very universal assumption. However, this assumption is (to my knowledge) not documented anywhere. I propose to make this assumption explicit in the `Clone` trait documentation. The property that seems the most reasonable to me is the following: When implementing both `Clone` and `PartialEq`, then ```text x == x -> x.clone() == x ``` is expected to hold. This way, when also implementing `Eq`, it automatically follows that `x.clone() == x` has to hold, which should be enough for the collections to not break. At the same time, the property also works for the "normal" elements of a type with `PartialEq`. For the wording, I tried to follow the [`Hash` and `Eq`](https://doc.rust-lang.org/std/hash/trait.Hash.html#hash-and-eq) documentation. As I am fairly new to Rust, it might well be that this property cannot be generally expected – it seems reasonable to me, but any counter-examples or critique, both content- and wording-wise, would be very welcome. If the property turns out to be too general, I would suggest to at least document the assumption of `x.clone() == x` for the collections somehow. An additional thought of mine: If it is indeed generally expected that `x == x -> x.clone() == x`, then, for the sake of completeness, one could also define that `x != x -> y != y for y = x.clone()` should hold, i.e., that an object that did not compare equal to itself before cloning, should also not compare equal to itself afterwards.
…=Amanieu document assumptions about `Clone` and `Eq` traits Most standard library collections break if `Clone` has a non-standard implementation which violates `x.clone() == x`. [Here](https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=b7fc6dfa8410cbb673eb8d38393d81de) the resulting broken behaviour of different collections is shown. I originally created an issue at rust-lang/hashbrown#629, but the conclusion there was that `x.clone()` resulting in an object that compares equal to the original one is probably a very universal assumption. However, this assumption is (to my knowledge) not documented anywhere. I propose to make this assumption explicit in the `Clone` trait documentation. The property that seems the most reasonable to me is the following: When implementing both `Clone` and `PartialEq`, then ```text x == x -> x.clone() == x ``` is expected to hold. This way, when also implementing `Eq`, it automatically follows that `x.clone() == x` has to hold, which should be enough for the collections to not break. At the same time, the property also works for the "normal" elements of a type with `PartialEq`. For the wording, I tried to follow the [`Hash` and `Eq`](https://doc.rust-lang.org/std/hash/trait.Hash.html#hash-and-eq) documentation. As I am fairly new to Rust, it might well be that this property cannot be generally expected – it seems reasonable to me, but any counter-examples or critique, both content- and wording-wise, would be very welcome. If the property turns out to be too general, I would suggest to at least document the assumption of `x.clone() == x` for the collections somehow. An additional thought of mine: If it is indeed generally expected that `x == x -> x.clone() == x`, then, for the sake of completeness, one could also define that `x != x -> y != y for y = x.clone()` should hold, i.e., that an object that did not compare equal to itself before cloning, should also not compare equal to itself afterwards.
Most standard library collections break if
Clonehas a non-standard implementation which violatesx.clone() == x. Here the resulting broken behaviour of different collections is shown. I originally created an issue at rust-lang/hashbrown#629, but the conclusion there was thatx.clone()resulting in an object that compares equal to the original one is probably a very universal assumption. However, this assumption is (to my knowledge) not documented anywhere.I propose to make this assumption explicit in the
Clonetrait documentation. The property that seems the most reasonable to me is the following: When implementing bothCloneandPartialEq, thenis expected to hold. This way, when also implementing
Eq, it automatically follows thatx.clone() == xhas to hold, which should be enough for the collections to not break. At the same time, the property also works for the "normal" elements of a type withPartialEq. For the wording, I tried to follow theHashandEqdocumentation.As I am fairly new to Rust, it might well be that this property cannot be generally expected – it seems reasonable to me, but any counter-examples or critique, both content- and wording-wise, would be very welcome. If the property turns out to be too general, I would suggest to at least document the assumption of
x.clone() == xfor the collections somehow.An additional thought of mine:
If it is indeed generally expected that
x == x -> x.clone() == x, then, for the sake of completeness, one could also define thatx != x -> y != y for y = x.clone()should hold, i.e., that an object that did not compare equal to itself before cloning, should also not compare equal to itself afterwards.