Implement Eq for Cell and RefCell.

[SimonSapin]

core::cell::Cell<T> and core::cell::RefCell<T> currently implement PartialEq when T does, and just defer to comparing T values. There is no reason the same shouldn’t apply to Eq.

This enables #[derive(Eq, PartialEq)] on e.g. structs that have a RefCell field.

r? @alexcrichton

I’m unsure what to do with #[stable] attributes on impls. impls generated by #[derive] don’t have them.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTING.md for more information.

[tbu-]

There might be a reason why RefCell does not implement Eq, namely that x == x may panic instead of returning true.

[SimonSapin]

@tbu- I don’t understand how that’s more of a problem for Eq than for PartialEq.

[tbu-]

@SimonSapinEq guarantees that x == x returns true, which is not true in all cases for RefCell.

[SimonSapin]

It’s not clear to me that “don’t panic” is a promise that Eq makes.

[tbu-]

Well, it'd be useful for HashMaps. Maybe Eq needs to be clarified.

[SimonSapin]

Anyway, here is how I came to this:

• I tried using assert_eq! in some tests, which failed because one of my structs was not comparable.
• I went to add #[derive(PartialEq)] on my struct, and while I was at it also added Eq because I didn’t see a reason not to.
• This failed because my struct contains a RefCell, which does not implement Eq
• I didn’t see a reason for me not to other than maybe no one thought of it yet, so I sent this PR.

If there is a good reason not to implement Eq, feel free to close this PR. I don’t really need it, PartialEq is enough for assert_eq!.

[alexcrichton]

I think that the Cell implementation of Eq is fine (just forgotten), but I agree with @tbu- that the implementation for RefCell gives me a moment of pause. I definitely don't consider it a part of the contract of Eq, however, that the implementation does not panic (this isn't mentioned at all in the documentation for one), so in that sense I think that this is fine. I feel that this is such a core primitive type that we need to make it as ergonomic as possible to use, which entails implementing all of these core traits (including those that may panic).

cc @rust-lang/libs, do others have a strong opinion one way or another about this?

[BurntSushi]

I'm looking over the existing impls for Eq, and I don't think I see any that could cause panics. So setting a precedent also gives me pause. That said, panic'ing is not part of the contract for either PartialEq or Eq, so I'd be fine with it.

[Kimundi]

I remember that RefCell did not implement any traits that require it to borrow its content at some point due to the panic issue. But if it already implements PartialEq, then that ship has probably sailed and we should go for feature parity with other libstd types.

[shepmaster]

See also #16714, #19388

[sfackler]

The PartialEq impl has existed since the introduction of RefCell in #10514 (oops).

We could either treat it as "this should have never existed, and we're not going to add any more delegating trait implementations", or decide that it's fine, in which case we should probably add on impls of PartialOrd, Ord etc in addition to Eq. I'd kind of lean towards the former personally.

[aturon]

I consider the Eq contract to be that x == x should never return false, but panicking seems like fair game. Similarly with Ord, the main point here is to avoid getting results that can screw up data structure invariants, but a panic isn't likely to do that (though it may raise exception safety issues, but these are mainly a concern for unsafe code and have to be considered regardless.)

So, I'm +1 for adding these impls.

[alexcrichton]

@sfackler in the past we've found that omitting implementations of core traits from core types (e.g. Debug from Path) ended up causing more pain than it was worth, so I'm curious, could you elaborate more on why you think we shouldn't do this?

[sfackler]

I'm a bit leery in general of invisible sources of panics.

[alexcrichton]

True, although technically this PR introduces 0 extra panics because Eq has no methods internally (e.g. the panics only come from RefCell::eq)

[Gankra]

fwiw unsafe code already needs to guard against this sort of thing because "you never know". As such adding this is basically a "no-lose" scenario to me.

[tbu-]

@gankro I don't think @sfackler ment this as part of unsafe code, invisible panics are always bad for program correctness.

[alexcrichton]

@bors: r+ bbf8ba7

[bors]

⌛ Testing commit bbf8ba7 with merge 53941be...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt