diff --git a/src/libs/maintaining-std.md b/src/libs/maintaining-std.md index 0912f6097..05706fe0d 100644 --- a/src/libs/maintaining-std.md +++ b/src/libs/maintaining-std.md @@ -187,7 +187,41 @@ Changes to collection internals may affect the order their items are dropped in. ### Is there a manual `Drop` implementation? -Generic types that manually implement `Drop` should consider whether a `#[may_dangle]` attribute is appropriate. The [Nomicon][dropck] has some details on what `#[may_dangle]` is all about. +A generic `Type` that manually implements `Drop` should consider whether a `#[may_dangle]` attribute is appropriate on `T`. The [Nomicon][dropck] has some details on what `#[may_dangle]` is all about. + +If a generic `Type` has a manual drop implementation that may also involve dropping `T` then dropck needs to know about it. If `Type`'s ownership of `T` is expressed through types that don't drop `T` themselves such as `ManuallyDrop`, `*mut T`, or `MaybeUninit` then `Type` also [needs a `PhantomData` field][RFC 0769 PhantomData] to tell dropck that `T` may be dropped. Types in the standard library that use the internal `Unique` pointer type don't need a `PhantomData` marker field. That's taken care of for them by `Unique`. + +As a real-world example of where this can go wrong, consider an `OptionCell` that looks something like this: + +```rust +struct OptionCell { + is_init: bool, + value: MaybeUninit, +} + +impl Drop for OptionCell { + fn drop(&mut self) { + if self.is_init { + // Safety: `value` is guaranteed to be fully initialized when `is_init` is true. + // Safety: The cell is being dropped, so it can't be accessed again. + drop(unsafe { self.value.read() }); + } + } +} +``` + +Adding a `#[may_dangle]` attribute to this `OptionCell` that didn't have a `PhantomData` marker field opened up [a soundness hole][rust/issues/76367] for `T`'s that didn't strictly outlive the `OptionCell`, and so could be accessed after being dropped in their own `Drop` implementations. The correct application of `#[may_dangle]` also required a `PhantomData` field: + +```diff +struct OptionCell { + is_init: bool, + value: MaybeUninit, ++ _marker: PhantomData, +} + +- impl Drop for OptionCell { ++ unsafe impl<#[may_dangle] T> Drop for OptionCell { +``` ### How could `mem` break assumptions? @@ -260,7 +294,9 @@ Where `unsafe` and `const` is involved, e.g., for operations which are "unconst" [Forge]: https://forge.rust-lang.org/ [RFC 1023]: https://rust-lang.github.io/rfcs/1023-rebalancing-coherence.html [RFC 1105]: https://rust-lang.github.io/rfcs/1105-api-evolution.html +[RFC 0769 PhantomData]: https://github.com/rust-lang/rfcs/blob/master/text/0769-sound-generic-drop.md#phantom-data [Everyone Poops]: http://cglab.ca/~abeinges/blah/everyone-poops [rust/pull/46799]: https://github.com/rust-lang/rust/pull/46799 +[rust/issues/76367]: https://github.com/rust-lang/rust/issues/76367 [hashbrown/pull/119]: https://github.com/rust-lang/hashbrown/pull/119 [rollup guidelines]: ../compiler/reviews.md#rollups