document assumptions about Clone and Eq traits

[gewitternacht]

Most standard library collections break if Clone has a non-standard implementation which violates x.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 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

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 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.

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[Mark-Simulacrum]

It's a bit confusing to say "these methods" here IMO - maybe "this property"?

[gewitternacht]

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]

Moving into libs-api queue for FCP, since this is a new property (even if likely already required implicitly) so merits some team discussion.