Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add note about PhantomData and dropck #432

Merged
merged 9 commits into from
Sep 7, 2020
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 37 additions & 1 deletion src/libs/maintaining-std.md
Original file line number Diff line number Diff line change
Expand Up @@ -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<T>` 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<T>` has a manual drop implementation that may also involve dropping `T` then dropck needs to know about it. If `Type<T>`'s ownership of `T` is expressed through types that don't drop `T` themselves such as `ManuallyDrop<T>`, `*mut T`, or `MaybeUninit<T>` then `Type<T>` also [needs a `PhantomData<T>` field][RFC 0769 PhantomData] to tell dropck that `T` may be dropped. Types in the standard library that use the internal `Unique<T>` pointer type don't need a `PhantomData<T>` marker field. That's taken care of for them by `Unique<T>`.

As a real-world example of where this can go wrong, consider an `OptionCell<T>` that looks something like this:

```rust
struct OptionCell<T> {
is_init: bool,
value: MaybeUninit<T>,
}

impl<T> Drop for OptionCell<T> {
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.
KodrAus marked this conversation as resolved.
Show resolved Hide resolved
drop(unsafe { self.value.read() });
}
}
}
```

Adding a `#[may_dangle]` attribute to this `OptionCell<T>` that didn't have a `PhantomData<T>` marker field opened up [a soundness hole][rust/issues/76367] for `T`'s that didn't strictly outlive the `OptionCell<T>`, and so could be accessed after being dropped in their own `Drop` implementations. The correct application of `#[may_dangle]` also required a `PhantomData<T>` field:

```diff
struct OptionCell<T> {
is_init: bool,
value: MaybeUninit<T>,
+ _marker: PhantomData<T>,
}

- impl<T> Drop for OptionCell<T> {
+ unsafe impl<#[may_dangle] T> Drop for OptionCell<T> {
```

### How could `mem` break assumptions?

Expand Down Expand Up @@ -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