PhantomData behavior is very surprising w.r.t. lack of Drop implications

[SoniEx2]

I tried this code:

use core::cell::Cell;
use core::marker::PhantomData as PDOrBox;
//use std::boxed::Box as PDOrBox;
struct Foo<'a> {
  selfref: Cell<Option<&'a Foo<'a>>>,
}
impl<'a> Drop for Foo<'a> {
  fn drop(&mut self) {
  }
}
fn make_selfref<'a>(x: &'a PDOrBox<Foo<'a>>) {}
fn make_pdorbox<'a>() -> PDOrBox<Foo<'a>> {
    unimplemented!()
}
fn main() {
  let x = make_pdorbox();
  make_selfref(&x);
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=14dee41511095d6e940069db08cdc23f

I expected to see this happen: since a PhantomData claims to act like a T including for the purposes of borrowck/dropck then this should absolutely compile-fail. just as it does if you use a Box, or T directly!

Instead, this happened: it doesn't!

this is clearly unsound.

Meta

rustc --version --verbose:

playground

Backtrace

<backtrace>

[SkiFire13]

this is clearly unsound

It isn't clear to me though. I don't see how this could ever be exploited to create unsoundness.

It seems the error happens because Box<T> actually drops T, which counts as a use of the T, while PhantomData<T> doesn't, so it compiles.

[SoniEx2]

see also https://docs.rs/selfref/0.2.4/selfref/

(maybe we should just ship it with the PhantomData and let y'all deal with it instead. but we hear that counts as "releasing exploits into the wild" and some ppl are very litigious about that stuff and we don't really wanna deal with that just yet.)

it's also worth noting that it fails to compile if you use a T directly, and PhantomData is supposed to act like a T, is it not? so why does it fail to act like a T?

[SkiFire13]

PhantomData is supposed to act like a T

It's supposed to make a struct act as if it contains a T, it doesn't act itself as a T.

[SoniEx2]

why can't/shouldn't it act itself as a T when used directly?

[Monadic-Cat]

PhantomData is less like Box<T> and more like [T; 0].
Using your example, this compiles:

use core::cell::Cell;
//use core::marker::PhantomData as PDOrBox;
//use std::boxed::Box as PDOrBox;

type PDOrBox<T> = [T; 0];

struct Foo<'a> {
  selfref: Cell<Option<&'a Foo<'a>>>,
}
impl<'a> Drop for Foo<'a> {
  fn drop(&mut self) {
  }
}
fn make_selfref<'a>(x: &'a PDOrBox<Foo<'a>>) {}
fn make_pdorbox<'a>() -> PDOrBox<Foo<'a>> {
    unimplemented!()
}
fn main() {
  let x = make_pdorbox();
  make_selfref(&x);
}

I haven't stared at this long enough to decide whether this is correct behavior for PhantomData<T>, but I think it's a bit of a stretch to call it "unsound" without qualification- it's clearly not an "unsound" API in the sense of "we wrap something unsafe but still cause UB in some cases" which is usual in Rust.

[SoniEx2]

it's unsound as in "if you use this with unsafe, you'll have soundness issues". and the worst part is that there's no appropriate alternative that can be used on stable. there's no "PhantomDrop", no "PhantomBox", no anything like that.

[Lee-Janggun]

AFAIK, PhantomData are special markers to place in structs to aid rustc on doing some analysis. When it is used by itself, it should really do nothing as it is a zero sized type, so should be optimized away.

it's unsound as in "if you use this with unsafe, you'll have soundness issues". and the worst part is that there's no appropriate alternative that can be used on stable. there's no "PhantomDrop", no "PhantomBox", no anything like that.

So for this, I really don't get the point. For the first sentence, are you saying that using PhantomData in any unsafe code is UB? For the second, are you recommending new types that "act" as if they are Box (which I have no idea what it would mean)?

[SoniEx2]

a PhantomActuallyData<T: ?Sized> that acts like a T except for size. which should be just PhantomData but for some reason isn't.

acts like a T for borrowck, dropck, Drop impl, Copy/Clone, projections, etc etc.

[Lee-Janggun]

1. What would be the motivation for such type?
2. Can such type ever be sound? Since it has size 0, in actual compilation it will be optimized away, and in actual execution there will be nothing. For example if you had a PhantomActuallyData<Cell<u64>> and called get() on it to get the inner value, what would the value of this at runtime?

Anything related to such PhantomActuallyData will need to be make sure to be no-ops at runtime, and I don't know if that is possible or even needed.

Although this itself is slightly off-topic to the original question. Regarding it, I think you are trying to give PhantomData semantic requirements it was never meant to have, hence why you thought the above example was unsound.

[nbdd0121]

The difference is solely because PhantomData does not implement Drop.

[SoniEx2]

@Lee-Janggun the actual receiver is Cell<u64> not PhantomActuallyData<Cell<u64>> so you can't actually call get() on it unless it explicitly supported it. but the point is that it should work just like a Foo<T> for some Foo except this one happens to be zero-sized for any T (including T: !Sized).

@nbdd0121 it needs the #[may_dangle] like Box has, otherwise it would have the wrong semantics. (are we allowed to break the Copy^Drop rule for PhantomData? because that'd be awesome!)

[nbdd0121]

Anyway, PhantomData is not unsound, but surely it is not doing what the document suggests. You can't produce UB without unsafe code, it's just that your unsafe code depends on the property described in the document.

I think it'll be a breaking change to modify the behaviour of PhantomData does though, because if PhantomData<T> were truly behave like T, the need of a drop glue will make things containing PhantomData ineligible to NLL.

Probably we should take this an opportunity to stablise #[may_dangle] or some alternative instead.

[SkiFire13]

Note that PhantomData's documentation specifically talks about using it as a field, not by itself like in this issue.
Quoting from the documentation (emphasis mine):

Zero-sized type used to mark things that “act like” they own a T.

Adding a PhantomData<T> field to your type tells the compiler that your type acts as though it stores a value of type T, even though it doesn’t really. This information is used when computing certain safety properties.

Does this issue also affect PhantomData when used as a field?

[SoniEx2]

yes.

[Lee-Janggun]

it should work just like a Foo for some Foo except this one happens to be zero-sized for any T (including T: !Sized).

Ok I think I get your point, in that it should act like a struct with a T field, but be zero sized at runtime.

Still, is that intended for PhantomData? IDK about the full technical details but my understanding has been that PhantomData is just a marker to properly able certain compiler analysis for certain types, and nothing really more.

[nbdd0121]

Maybe we should change the doc to say that PhantomData<T> act like ManuallyDrop<T>.

[SoniEx2]

and so anyone who inadvertently (without testing it) relied on the as-documented behaviour is left with no usable alternative and an unsound crate.

why not... fix PhantomData?

[nagisa]

I’m going to nominate this for a look by T-lang. The lang team seems like the best candidate to make a call here as to how the PhantomData should be behaving and what the implications of that decision may be.

[scottmcm]

This changed between 1.35 and 1.36 in the 2015 edition: https://rust.godbolt.org/z/bYvjeGvM5

[nikomatsakis]

We discussed this in the 2022-10-11 meeting.

Some things we realized:

• PhantomData is Copy, which in NLL implies that dropping it has no effect.
• The docs as written do state that however that PhantomData<T> acts "as though" T is dropped (and I at least expected that), which is incompatible with that.

If you modify the example that that the PDOrBox type requires drop (e.g., by making it a new type that implements Drop, or which has a field that does), then PhantomData<T> plays its expected role -- it makes the drop code for PDOrBox be considered to drop T, even if no other fields own data of type T.

So the more correct summary seems to be:

• when a struct S owns a PhantomData<T> and either has Drop or has fields which require Drop, the drop code for S will be assumed to potential drop values of type T.

As @nbdd0121 pointed out, this is more like saying "it is similar to ManuallyDrop<T>".

That is the current behavior, at least. The question @SoniEx2 is whether you can adjust your code to be sound given those semantics?

[nbdd0121]

I think the answer to the last question is yes.

For the selfref crate, the UBCheck:

pub struct UBCheck<T: ?Sized>(PhantomData<T>);

unsafe impl<#[may_dangle] T: ?Sized> Drop for UBCheck<T> {
    fn drop(&mut self) {}
}

can be implemented as

struct NopDrop;
impl Drop for NopDrop {
	fn drop(&mut self) {}
}

pub struct UBCheck<T: ?Sized>(NopDrop, PhantomData<T>);

[SoniEx2]

@nikomatsakis honestly we'd lean towards considering this behaviour of NLL incorrect in the presence of PhantomData, because PhantomData is just that special. but if we were to do that, then how do you even fix that? we mean, what would dropck for Copy types even translate to in NLL terms? maybe it creates more problems than it solves...

@nbdd0121 huh. interesting.

edit: tho the meeting notes seem a bit inconsistent. specifically:

If something requires drop (for some other reason), and it contains a PhantomData<T>, the Drop is assumed to access data of type T.

wouldn't that be "the drop glue"?

which is really weird/surprising btw! if this is gonna be a thing then there should definitely be tests for it. (that is to say: can we rely on it?)

[SoniEx2]

also it's not similar to ManuallyDrop: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=ef62faa188b41fd69f1561da05605db6

[SoniEx2]

even more surprising is the behaviour of 0-length arrays:

with drop sibling: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=a2e6095c7257fe95f3d07abc33b6417f
without drop sibling: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=9810be804c68884cf6b2071f8bd4ad2a

note that 0-length arrays are not copy: #94313