Add more methods in Cell for non-Copy types

[F001]

Complement to #39264

r? @alexcrichton

[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. Due to 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 the contribution instructions for more information.

[hanna-kruppe]

Isn't this super unsafe? If a panic occurs here, *self will be dropped (not in this scope, but later during stack unwinding) which will drop uninitialized memory which will cause UB.

[hanna-kruppe]

1. AssertUnwindSafe is not unsafe, and for good reason. Safe code using AssertUnwindSafe unwisely may get very undesirable behavior, but not UB.
2. AssertUnwindSafe is irrelevant, as this problem occurs without any catch_unwind calls. In fact, a well-placed catch_unwind can stop a panic in this clone call from dropping *self (by stopping the unwinding before it gets to the stack frame where the Cell would be dropped).

[F001]

Well, you are right. I need to figure out a solution.

[TimNN]

@F001: You'll probably want to use something similar to the Hole structure described in the nomicon, which writes a valid value back into the original cell in case of a panic.

[TimNN]

Or rather, is there a reason, you can't just use self.value.get().clone()?

[hanna-kruppe]

@TimNN @F001 I haven't thought through the details but https://users.rust-lang.org/t/why-does-cell-require-copy-instead-of-clone/5769/3?u=rkruppe seems relevant.

[TimNN]

@rkruppe: Thanks for the link! After having a quick read through it and the comments on the original RFC, I think that safely implementing the methods proposed by this PR is fundamentally incompatible with "MoveCells", since we are not allowed to call any method on the contained Type, as I understand it. (This is not a problem for "CopyCells" since we can create a new copy of the value without calling any methods on it).

[F001]

Thanks for your comments!

@TimNN I don't agree that these methods are fundamentally incompatible with "MoveCells". Cell is not intended to prohibit methods of the contained type. The most important guarantee is "disallowing any reference (mutable or immutable) to the data contained in the cell."

[TimNN]

The most important guarantee is "disallowing any reference (mutable or immutable) to the data contained in the cell."

I agree with that statement. However I think exactly this is happening here: As soon as you call clone or partial_eq, you provide those methods with a reference to the data in the cell, which then allows for bad things to happen, as can be seen by the example in the post linked by @rkruppe.

[hanna-kruppe]

For the record: same UB issue here as in the Clone impl, so this affects most of the other impls added in this PR too.

[F001]

Thank you. Updated.

[alexcrichton]

Thanks for the PR! Unfortunately these implementations are fundamentally unsound as-written. The purpose of Cell is for types where you can never acquire an interior reference, but all of these methods require an interior reference.

Implementing such methods but accepting panics-as-aborts or something like that is an RFC-level change (as opposed to just happening in a PR), and I think otherwise tweaking these implementations to be sound will likely also be an RFC-level change (e.g. requiring Default for the implementations). As a result I'm going to close this for now, but you can of course feel free to reach out to me on IRC or we can continue to talk here!