Extend check for UnsafeCell in consts to cover unions by tmiasko · Pull Request...

Wow, what a test. :D

That said, which part of this test causes UB? With a repr, I think changing the tag of an enum via raw ptr manipulation is totally fine. It seems promotion analysis is making some IMO unfounded assumptions if it thinks the tag cannot change here.

Oh wait, you are writing through a shared ref. That is more tricky... though the exact scope of UnsafeCell inside enums is not decided yet, so this is not as clear-cut as one might think at first sight. For example, for an Option<Cell<NonZeroI32>>, mutating the discriminant through a shared ref is almost certainly going to be allowed.
Cc rust-lang/unsafe-code-guidelines#204 rust-lang/unsafe-code-guidelines#236

For example, for an Option<Cell<NonZeroI32>>, mutating the discriminant through a shared ref is almost certainly going to be allowed.

was this a typo? I thought this specifically wasnt allowed, we made UnsafeCell inhibit niches, i.e. size_of::<Option<UnsafeCell<NonZeroU32>>>() == 8

edit: oh no I just checked and it doesnt apply when the unsafecell is newtyped..? That seems bad

Test case creates a reference to S::x field which has a u32 type, so no interior mutability here. The const qualification assumes that resulting pointer cannot be used to access the other parts of a local. The assumption is violated here by offsetting the pointer and overwriting an adjacent tag.

edit: oh no I just checked and it doesnt apply when the unsafecell is newtyped..? That seems bad

Good catch. I think we already have an open issue for niche hiding not working exactly as intended.

Ah right UnsafeCell masks niches these days (except when newtpyed but that is a bug: #87341).

Test case creates a reference to S::x field which has a u32 type, so no interior mutability here. The const qualification assumes that resulting pointer cannot be used to access the other parts of a local. The assumption is violated here by offsetting the pointer and overwriting an adjacent tag.

Ah I see. So this makes certain assumptions about the aliasing model, but this particular case is probably fine -- however, I am still a bit worried that promotion analysis might make more assumptions than we can actually uphold. In this case we are creating an &T where T: Freeze. But if we ever do anything value-based here (e.g., &None::<T> where T might be !Freeze), that would be sketchy.