`DerefMut` auto-deref error for `union` fields sometimes doesn't trigger

[Alexendoo]

An implicit DerefMut of a MaybeUninit union field is an error, but sometimes that error does not fire. This is not a soundness issue as it's more like a lint

https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=3df4479c28c8a9ce5d2372e161c0e292

use std::mem::ManuallyDrop;

union U {
    x: (),
    f: ManuallyDrop<(Vec<u8>,)>,
}

fn main() {
    let mut u = U { x: () };
    // Errors about the implicit deref of the ManuallyDrop
    unsafe { u.f.0 = Vec::new() };
    // equivalent to (*u.f).0

    let r = &mut u;
    // implicitly derefs the ManuallyDrop but does not error
    unsafe { r.f.0 = Vec::new() };
    // equivalent to (*(*r).f).0
}

I expected to see this happen: Both assignments should error

Instead, this happened: Only the first produces an error

error: not automatically applying `DerefMut` on `ManuallyDrop` union field
  --> src/main.rs:24:9
   |
24 |         a.tup.0.num = 0;
   |         ^^^^^^^
   |
   = help: writing to this reference calls the destructor for the old value
   = help: add an explicit `*` if that is desired, or call `ptr::write` to not run the destructor

---

We ran into a similar case in rust-lang/rust-clippy#14387

https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=223c79eac2bb82ba9834354d2043d67f

use std::mem::ManuallyDrop;

struct Data {
    num: u64,
}

union DataWithPadding {
    u: (),
    tup: (ManuallyDrop<Data>, ()),
}

fn main() {
    let mut a = DataWithPadding { u: () };

    unsafe {
        // No error, but the ManuallyDrop in the tuple is being implicitly dereferenced
        (*&mut a.tup).0.num = 0;
        // equivalent to (*(*&mut a.tup).0).num
        
        // similar
        (&mut a.tup).0.num = 0;

        // Does error when written plainly
        a.tup.0.num = 0;
    }
}

[eggyal]

@rustbot claim

[RalfJung]

@rust-lang/lang @rust-lang/types this issue is about an oversight in the implementation of RFC 2514, specifically the check which prevents using the DerefMut coercion on ManuallyDrop inside union fields. (See here for the original motivating example for this check.) We reject this because it can lead to executing drop glue, where union fields usually are prevented from having drop glue which prevents those issues.

However, the check has a bug, so it does not always kick in:

use std::ptr::NonNull;
use std::mem::MaybeUninit;
use std::mem::ManuallyDrop;

union SmallVecData<A> {
    inline: ManuallyDrop<MaybeUninit<A>>,
    heap: (NonNull<A>, usize),
}

impl<A> SmallVecData<A> {
    unsafe fn inline_mut_val(mut self) -> NonNull<A> {
        NonNull::new(self.inline.as_mut_ptr() as *mut A).unwrap()
//                   ^^^^^^^^^^^ this errors
    }
    unsafe fn inline_mut_val_fixed(mut self) -> NonNull<A> {
        NonNull::new((*self.inline).as_mut_ptr() as *mut A).unwrap()
//                   ^^^^^^^^^^^^^ this is fine
    }

    unsafe fn inline_mut_ref(&mut self) -> NonNull<A> {
        NonNull::new(self.inline.as_mut_ptr() as *mut A).unwrap()
//                   ^^^^^^^^^^^ this does not error
    }
}

There's a fix proposed in #151920 (I can't really say whether it's the right fix since I don't understand our coercion handling code), which uncovered the problem that not even the compiler itself builds if we fix this since a snippet very much like the above can be found in SmallVec.

Do you have thoughts on how we should proceed here? I lost all context from that RFC so I am still trying to put together the pieces here, but these are the options that come to my mind at the moment:

• Move towards enforcing this restriction properly also for unions below mutable references. Will require a FCW and transition period.
• Maybe we can get a more fine-grained check that has less fallout? For instance, in the code above, we are using the ManuallyDrop deref coercion to project to a type that itself has no drop glue. There doesn't seem to be a good reason for even erroring at all in that case. I do seem to remember that we deliberately require the union field to be either ManuallyDrop or Copy to avoid a semver hazard where code depends on whether the field type has drop glue (but isn't that anyway already semver-breaking?). That would then probably be a good reason to also not check for drop glue in this deref coercion restriction. I don't remember if that was the only reason.
• We could add MaybeUninit to the list of types that implement the BikeshedGuaranteedNoDrop trait, and use that trait also as part of the check. That would fix the particular SmallVec issue encountered above; unclear what it would do to the rest of the ecosystem.
• Some other third option?

(Not nominating yet as I hope we can get some crater data first, but still interested in everyone's thoughts here. :)

[BoxyUwU]

I wonder why this is a hard error instead of a lint. It certainly seems like a footgun but also doesn't seem necessarily wrong? If this was a deny-by-default lint then we'd avoid the problem with dependencies failing to build (though it wouldn't change the extent of the breakage)

[RalfJung]

That's a fair question and I don't remember whether we adjusted other parts of the compiler to rely on this being a hard error.

[eggyal]

I've read through the tracking issue #55149, the implementing PR #75584 and the stabilisation PR #77547. I can't see anything in them, their discussions or issues linking to them that suggest anything in the compiler is relying on this being a hard error. I'm really not sure how we can ascertain whether something since is relying upon it? Shall I just got ahead and make it a lint?

[RalfJung]

We should double-check the unsafety checker for union field accesses -- there could be temptation there to say that writes are fine because they can't trigger drop glue.

But anyway I don't think we should make it a lint at this point, first we should gather some more data. My suggestion would to adjust the code so that MaybeUninit is also recognized as a "guaranteed no-drop" type and then run crater with your fix.