aturon · GitHub

@glaebhoerl

I don't see why the PartialEq thing is such a big deal at all. The PartialEq impl stays the same as it always was, and independently of it, some new capabilities get added.....There's no principle saying that every method/impl of a type should require the same trait bounds. I don't even understand why you would expect that.

I agree with you on the general principle; but of course one has to evaluate each case on its merits. In this case particular case I feel conflicted. You're making me rethink my opinion a bit. Let me just throw out some various chaotic thoughts.

Combining sharing and mutability is still a weak spot

I feel like in general we need to smooth out our story on sharing + mutability. This begins with education: explaining better when it is legal to mutate (the naming of &mut T strongly suggests to people that it is the only way to mutate, reasonably enough, and I think our messaging on sharing + mutability needs to get more crisp).

But also just looking at the APIs: The cell vs ref-cell split is logical, but not ideal. I think it's not well-explained, for one thing, but also it is often stricter than normal. Many uses of RefCell that I find myself doing in practice would be perfectly safe as a Cell that called clone. For example, RefCell<Rc<T>>, which is known not to invoke any methods of T; similarly I often want ref-cells with more complex types where I do a lot of swapping (as well as things like a RefCell<&mut Vec<T>> where I just push to accumulate, and then later recover mutable access).

Moreover, and most importantly I think, RefCell is really hard to use right. It's all too easy to hold the lock longer than you mean to, and tracking down a "double borrow" is more annoying than I would like (that's something else we could try to address, at least in debug builds).

I'd prefer if we had a widely usable alternative that avoided these pitfalls, so that we could promote it as the preferred way to have sharing and mutability (something like "avoid it if you can, but if not, use Cell").

Note that there is nothing stopping me from prototyping these ideas in a crates.io crate except for laziness. =)

What I would prefer: an expanded Cell

I would prefer if we could expand on Cell so that it can do a wide range of operations on a wide range of types. Ideally we would maintain the invariant that none of these operations will lead to a borrow error or other cell-related panic at runtime, but it might be better to loosen that to being "extremely unlikely" to lead to a borrow error.

Since we want to eliminate cell-related panics, we can't permit open-ended borrows like foo.borrow(). That requires a ref-cell still. But we can permit a bunch of operations:

- `get` -- clones the value (requires `T: Clone`, but see below)
- `set` -- overwrites the value (works for all `T`)
- `swap` -- swaps the value (works for all `T`)
- `take` for `Cell<Option<T>>`, which swaps with `None`
- `push` for `Cell<Vec<T>>`, which just pushes an element
- `pop` for `Cell<Vec<T>>`, which just pops an element

The last few might seem surprising. The idea here is that we know that push() on Vec<T> doesn't invoke any user-defined code, and that it doesn't mutate any cells, so this should be ok. (I think?) We can basically grow the set of operations here over time.

Note that I said get could work any T: Clone. Obviously in the general case this would require a safety flag and could panic -- but it's very unlikely to happen in practice unless you have some wacky clone impls. I think I'd prefer to support it for all T: Clone and use specialization to optimize away the flag, but I could see it also being nice to never have a flag and only support get() for T: Copy or types (notably Rc<T> and Arc<T>) that can be cloned without risking mutating the cell (this would be an unsafe trait).

Why partial eq bothers me

OK, without that context, why do I care about PartialEq? I'm not sure if I should, but I guess it just feels like a surprising case of the API changing (more so than other things). It's because the reason it can't work for arbitrary T is tied to this sort of subtle fact that PartialEq might do random things and bad code could cause problems (but in practice such problems are very unlikely).

Random middle ground

I wonder if it'd be a good first step to just extend Cell to copy types or those that implement an unsafe CellSafeClone trait (notably Rc and Arc). This would basically be things that promise that their clone method cannot affect any cell that contains self.

An irrelevant, whiny aside: why I wish cell did not have PartialEq

In retrospect, my preference would be that Cell<T> does not implement PartialEq under any circumstances. Cell is the marker that converts types from being "a value" into something with their own identity, and that is precisely where the semantics of PartialEq get murky. I'd rather we had no default, so that you can write your own wrappers. If we were to have behavior, I would have chosen pointer identity.

My analogy is to things like Python:

class Counter:
    def __init__(value):
        self.value = 0
x = Counter(0)
y = Counter(0)
x == x // True
x == y // False
x.value += 1
x == x // still True
x == y // still False

I consider this Rust code to be a direct translation, but of course it behaves differently:

let x = Rc::new(Cell::new(0));
let y = Rc::new(Cell::new(0));
x == x // true
x == y // true!
x.set(x.get() + 1);
x == x // still true, of course
x == y // now false, hmm.

This is because == in Python defaults to identity. This is a safer default when you have sharing+mutability, since it means that if x == y is true, it remains true no matter what you do.

OTOH, there are times when you want a value-like semantics. Usually if you are "freezing" a value. But in rust those are much rarer, I think, because you would usually use ownership for such scenarios and not require Cell. So I'd rather that people "opt-in" to that by wrapping the Cell with their own type.

But ... yeah, water under the bridge.

Read the original on github.com ↗