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
35 changes: 34 additions & 1 deletion src/libs/maintaining-std.md
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,38 @@ 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) {
// Safety: The cell is being dropped, so it can't be accessed again.
unsafe { self.take_inner() };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be good to at least state the type signature for take_inner. I assume it returns Option<T>.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or just write out the code:

if self.is_init {
    let _ = unsafe { self.value.read() };
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd find drop(unsafe { self.value.read() }) clearer, after all the point is that this drops the value.

}
}
```

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 +291,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