Rename ManuallyDrop::take to read

[CAD97]

Tracking issue: #55422

Renames ManuallyDrop::take to match ptr::read and MaybeUninit::read, and updates the documentation to more mirror MaybeUninit::read's documentation.

[rust-highfive]

r? @dtolnay

(rust_highfive has picked a reviewer for you, use r? to override)

[CAD97]

r? @SimonSapin

[Centril]

cc @RalfJung

[RalfJung]

This is more consistent with MaybeUninit. I am not sure if that's what we want, but I proposed it, so I am curious what T-libs thinks. :)

[cramertj]

It's surprising to me that a method called read would mutate the container, especially to the point of leaving its contents in an invalid state.

[CAD97]

method called read would mutate the container

It doesn't actually mutate the container, just like ptr::read doesn't mutate what's behind the pointer. Rather, the state is changed logically (just like with ptr::read which logically moves the value out) without changing anything in memory.

I only used &mut slot.value because I already had &mut and forgot I didn't need it 😅. The only reason ManuallyDrop::read takes &mut currently rather than & like MaybeUninit::read is as a lint; it's almost certainly unsound to use if someone else has a reference as well. That said, MaybeUninit::read uses &self and ptr::read uses *const, so if the idea is to parallel those, this should probably use &slot as well.

The other option where we use take(&mut self) is treating ManuallyDrop as basically an untagged, unchecked Option with Deref sugar due to being Some 99.99% of its lifetime. The only time I expect this function will be used soundly is in drop(&mut self).

[RalfJung]

That said, MaybeUninit::read uses &self and ptr::read uses *const, so if the idea is to parallel those, this should probably use &slot as well.

Good point, now I wonder if MaybeUninit::read should take &mut self...

[CAD97]

Relevant note: this method has just been mentioned in a blog post as take.

[RalfJung]

The link seems to be broken?

[CAD97]

@RalfJung fixed (missing http:// messed up GitHub's URL parser)

[Centril]

Relevant note: this method has just been mentioned in a blog post as take.

I'm sure @pnkfelix could change that if need be when we decide. :)

[Dylan-DPC-zz]

ping from triage @SimonSapin any updates on this?

[CAD97]

Per #55422 (comment), MaybeUninit::read may get renamed, removing that reasoning for the rename. Whether the function gets called read or take, the documentation updates in this PR I think are a clear improvement.

I'm happy to remove the rename if we can agree take is the better name.

[wirelessringo]

Ping from triage. @SimonSapin any updates on this? Thanks.

[Dylan-DPC-zz]

can anyone from @rust-lang/libs review this?