PhantomData<T> no longer dropck?

[matklad]

Consider this code:

use std::marker::PhantomData;

struct Pending<T> {
    phantom: PhantomData<T>,
}

fn pending<T>(value: T) -> Pending<T> {
    Pending {
        phantom: PhantomData,
    }
}

struct Inspector<'a> {
    value: &'a String,
}

impl Drop for Inspector<'_> {
    fn drop(&mut self) {
        eprintln!("drop inspector {}", self.value)
    }
}

fn main() {
    let p: Pending<Inspector>;
    let s = "hello".to_string();
    p = pending(Inspector { value: &s });
}

Playground

I believe it should not compile (by failing dropcheck). It, however, compiles and runs on 1.42 and 1.31. On 1.24 it indeed fails as I expect.

The equivalent code, where PhantomData<T> is replaced with T rightfully fails to compiles: Playground. So, dropchk somehow observes the difference between T and PhantomData<T>?

Either I misunderstand how PhantomData<T> is supposed to work, or this is a stable-to-stable regression and a soundless hole.

[jonas-schievink]

It, however, compiles and runs on 1.42 and 1.31.

It fails to compile with the 2015 edition on 1.31, so it's probably caused by NLL.

I suspect the issue is something like: PhantomData never "needs drop", so your struct also doesn't, so no Drop terminator is created that could access the borrowed value.

[jonas-schievink]

Adding an unrelated field that has a destructor makes the error appear again

[tesuji]

Regression from 1.36: https://rust.godbolt.org/z/hnEi6A

[jonas-schievink]

cc @matthewjasper

[spastorino]

Assigning P-high to this bug and leaving the nomination, this could have been P-critical too. This was discussed as part of our pre-triage meeting in Zulip.

[eddyb]

Should this error, though, if *no <X as Drop>::drop code actually runs?
This still errors as expected.

[nikomatsakis]

We definitely need to check into this, but I think @eddyb is correct that the special role of PhantomData only comes into play when Pending actually implements Drop; otherwise, the compiler is allowed to know that dropping Pending is a no-op (except in so far as it drops the contents of Pending).

[pnkfelix]

Injected between +nightly-2019-04-22 and +nightly-2019-04-23

% rustc +nightly-2019-04-22 --version
rustc 1.36.0-nightly (31a75a172 2019-04-21)
% rustc +nightly-2019-04-23 --version
rustc 1.36.0-nightly (6d599337f 2019-04-22)
% git log 31a75a172..6d599337f --author=bors --format=oneline
6d599337fa7047307ba72786bbabe6b9c9e4daac Auto merge of #60168 - varkor:tidy-leading-newline, r=alexcrichton
c21fbfe7e310b9055ed6b7c46b7d37b831a516e3 Auto merge of #59114 - matthewjasper:enable-migate-2015, r=pnkfelix
a850a426491e14186af2250549bf41256b5938d2 Auto merge of #60133 - phansch:deny_rust_2018_idioms, r=Centril
8f06188991e8e7c764f0775a35432d39e2596c9a Auto merge of #60053 - Xanewok:serde-save-analysis, r=nrc
247b505099ce96b0fbf686249ff410a893793ed7 Auto merge of #60158 - Xanewok:update-clippy, r=matthiaskrgr
% 

So current guess is that this was fallout from PR #59114

[pnkfelix]

And I have now confirmed that if you enable edition=2018, you will see this behavior going all the way back to Rust version 1.31.0 (i.e. back when we first deployed stable support for the 2018 edition).

• Ugh, @jonas-schievink said exactly this up above; sorry for overlooking that!

So now I just want to confirm the running hypothesis that this is logical according to NLL.

[pnkfelix]

At this point I agree with the logic put forth by @eddyb and @nikomatsakis up above: There is no destructor operating on the type that holds the PhantomData (i.e. Pending in this example), so it is sound to treat dropping pending as a no-op.

I do worry a little bit about the user's mental model for this case, however. The claim "PhantomData<T> is just like a T" doesn't quite hold up. (not that it ever did ...)

More specifically, here is the current text from the std library docs for PhantomData, section ownership and the drop check:

Adding a field of type PhantomData<T> indicates that your type owns data of type T. This in turn implies that when your type is dropped, it may drop one or more instances of the type T. This has bearing on the Rust compiler's drop check analysis.

So, it seems at the very least we have a documentation bug to resolve here...

[danielhenrymantilla]

Does PhantomData<T> play any role whatsoever when there is no impl<#[may_dangle] T> Drop ... for a type containing one?

The way I understand it, and if I'm right, that's a solution for the documentation, is that, when there is no field containing an actual T or a type that may drop a T, there are three cases:

1. no explicit Drop impl ⟹ no instance of type T may be used by the drop glue, hence it is fine for T to dangle. Although it could, there is no reason for PhantomData<T>to play any role here.

2. an explicit Drop impl, which is thus generic over T (thus leading to <Something<T> as Drop>::drop() being part of the drop glue). In that case, if T dangles, there will be dropck errors, whether there is a PhantomData<T> or not (by that I mean that one can replace PhantomData<T> by *const T or fn() -> *const T and there will still be dropck errors). So PhantomData<T> plays no role here either.

3. an explicit Drop impl, which is thus generic over T, but this time the type parameter T has the unsafe (and unstable) #[may_dangle] annotation on it. In that case, if T does dangle, dropck errors will happen if and only if there is an "owned" T in the type definition, and PhantomData<T> is counted as owned. Playground. So here is the situation where PhantomData<T> interacts with dropck.

So I think the simpler phrasing w.r.t. the PhantomData<T> + dropck interaction, is that, instead, a PhantomData<T> field interacts with unsafe impl<#[may_dangle] T> Drop.

• and the difference between the field + may_dangle, and a classic drop impl is that the classic drop impl does not allow a dangling T, even when T has no drop glue (e.g., T = &'_ String) whereas the #[may_dangle] T + PhantomData<T> does allow it. But both do deny T = Inspector<'_>.

---

So, back to the OP, the question is: could / should the case 1. be merged into the case 3.? And when phrased like that, it looks like there is no special reason for it. In stable Rust we can always go from 1. (and 3.) to 2. by having a classic Drop impl (even an empty one), but it is true that going from 2. to 3. (and thus from 1. to 3.) requires nightly Rust.

[RalfJung]

Does PhantomData play any role whatsoever when there is no impl<#[may_dangle] T> Drop ... for a type containing one?

Yes. First of all, it is also relevant for variance.
Secondly, it is relevant for a "normal" impl<T> Drop ... even without #[may_dangle], because then the compiler assumes that this will call drop_in_place::<T> and dropck's accordingly.

For example:

struct MyBox<T>(NonNull<T>);
// Mirror `Box` API, including `Drop`.

This is unsound because of the lack of PhantomData, no matter whether the drop impl is may_dangle or not.

[danielhenrymantilla]

This is unsound because of the lack of PhantomData, no matter whether the drop impl is may_dangle or not.

Could you provide an example of an implementation being unsound that does not use #[may_dangle]?

The following (case 2. from my previous post), for instance, fails to compile:

struct MyBox<T>(ptr::NonNull<T>);

/// Mirror `Box` API, including `Drop`.
impl<T> Drop for MyBox<T> {
    fn drop (self: &'_ mut Self)
    {
        // ...
    }
}

fn main ()
{
    let (my_box, s): (MyBox<&'_ String>, String);
    s = String::from("hi");
    let at_s = &s;
    let at_s_ptr = ptr::NonNull::from(Box::leak(Box::new(at_s)));
    my_box = MyBox(at_s_ptr);
}

• Playground

---

The way I see it, #[may_dangle] was created to allow this kind of situations, where, since &'_ String has no drop glue, it is sound to drop_in_place it. So, the way to express to Rust that the only usage of a type T that happens in an explicit Drop impl is drop_in_place(), is to:

1. first, #[may_dangle]-annotate the type parameter ("Hey Rust, I am not using T..."),

2. then, ensure the type contains a field that dropck-owns a T, such as a PhantomData ("... except for my drop_in_place::<T>()-ing it")

So, without step 1., I see no difference between PhantomData<T> and PhantomData<*const T>.

[RalfJung]

Hm... okay things don't work entirely as I thought they would. To rephrase what you said, a #[may_dangle] T drop may not call any methods on T except for dropping it, and moreover if it drops it we also need the PhantomData so that rustc can check that? But without #[may_dangle] I may call T's drop even if I don't have a PhantomData<T>? Interesting.

I still have an idea for a counterexample but it's getting tricky: a type not having T in its parameters at all (some kind oft type erasure going on) would have to still drop a T. The T would be remembered by an outer wrapper type that however does not have a destructor. This needs some awful unsafe code to extract the destructor from the vtable... I don't think I'll write all this right now, not sure what that would accomplish.

[pythonesque]

Yeah that actually sounds like an interesting and potentially useful data structure (a "view" into untyped memory responsible for calling destructors on it before handing it back).

Beyond the soundness issue, I also don't like this because it makes the rules more complicated. "treat PhantomData like T during structural recursion" is a simple rule that applies to other "auto traits" (to the extent that inherent drop impls are auto trait implementations), and I was hoping to use it for dropck as well.

[RalfJung]

Are you sure dropck isn't using that rule, though? Your own example demonstrates that dropck works field-by-field. Isn't it just doing that here, too? (EDIT: Hm okay, in the original example replacing PhantomData<T> by T changes behavior... so I guess it is violating that rule, I am just still confused about how exactly dropck works.)

Arguably, dropck needs a special case for PhantomData because otherwise it would never have any effect, given that it never needs dropping...

[bjorn3]

Or special case needs_drop for PhantomData to depend on the inner T. mem::needs_drop is documented to be only an optimization, so changing it's value to true is allowed:

This is purely an optimization hint, and may be implemented conservatively: it may return true for types that don't actually need to be dropped.

[danielhenrymantilla]

@RalfJung I think I have managed to implement what you were talking about: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=22b7c3d56e8cadceb49210b51fdb3255

• I have crafted a custom MyPhantomData that works as PhantomData except for it expressing that it owns and thus may drop a T independently of whether the containing struct impls any Drop whatsoever (I have achieved this by making MyPhantomData by a containing struct by itself, with its own dummy Drop).

• I have attempted to write an unsound program, which ought to be sound with the expected ownership semantics described in this thread (which MyPhantomData carries, but which PhantomData does not), and the former fails to compile, whereas the latter does not.

• I find it unclear whether it is the library's design fault of it is PhantomData's, so I'll let you be the judge.

[RalfJung]

@danielhenrymantilla Yeah, something like that is what I was imagining... though your code still has #[may_dangle] drop on the same type RemoteDrop that also has the PhantomData. I think I was imagining putting the function pointer and drop impl down into another type, so that #[may_dangle] and PhantomData are on two different types.

I am not sure why RawOption would be needed?

[TOETOE55]

@RalfJung I'm still confused about:

This is unsound because of the lack of PhantomData, no matter whether the drop impl is may_dangle or not.

From my understanding of dropck, considering MyBox<T> impl no #[may_dangle] Drop:

If it construct

• without PhantomData<T>, the dropck would be:

  dropck(MyBox<T>, 'scope) := T: 'scope (strictly)

• with PhantomData<T>:

  dropck(MyBox<T>, 'scope) := T: 'scope & dropck(T, 'scope) where dropck(PhantomData<T>, 'scope) := dropck(T, 'scope)

If you want to construct a ub from the former, it is equivalent to finding a T that satisfies T: 'scope but not dropck(T, 'scope), and causes ub when it destructs.

But I tried to construct such a T and failed. I can't even find a counterexample for T:' scope - > dropck(T, 'scope).

And I'm not sure how this violates dropck:

I still have an idea for a counterexample but it's getting tricky: a type not having T in its parameters at all (some kind oft type erasure going on) would have to still drop a T. The T would be remembered by an outer wrapper type that however does not have a destructor.

[RalfJung]

Wow this is an ancient thread, and I lost all context. Could you link to the context where I said that? Also I recently opened #103413, does that help?

[RalfJung]

Re-reading the thread above, it seems to be about the same fundamental question as #102810. Curious.

My PR fixes the documentation paragraph that @pnkfelix quoted above

Adding a field of type PhantomData indicates that your type owns data of type T. This in turn implies that when your type is dropped, it may drop one or more instances of the type T. This has bearing on the Rust compiler's drop check analysis.

They also wrote

I do worry a little bit about the user's mental model for this case, however. The claim "PhantomData is just like a T" doesn't quite hold up. (not that it ever did ...)

which is still true and would require pretty fundamental changes to NLL at this point.

[TOETOE55]

I know PhatomData only works on needs_drop types dropping.

I'm just curious how the following code causes ub, and how it violates dropck rules:

For example:

struct MyBox<T>(NonNull<T>);
// Mirror `Box` API, including `Drop`.

This is unsound because of the lack of PhantomData, no matter whether the drop impl is may_dangle or not.

#70841 (comment)

[RalfJung]

Ah, you mean here. A link would have been helpful. :)
Note that I go on saying

Hm... okay things don't work entirely as I thought they would.

#103413 clarifies the misunderstanding me and many others had -- PhantomData is never needed for Drop unless you use may_dangle.

[TOETOE55]

Oh, I will note that :D.

PhantomData is never needed for Drop unless you use may_dangle

But as the example in #103413 (comment), PhantomData still works for "normal" dropping.(without #[may_dangle])

[RalfJung]

Yeah I read that thread after this one.^^ The summary is "it's complicated, hopefully we'll figure it out in #103413". Let's not re-post everything in two spots. :)