Do not automatically DerefMut ManuallyDrop through union references

[eggyal]

Extends #75584 to situations where the union itself is accessed through a reference.

Fixes #141621
r? compiler

[rust-log-analyzer]

The job aarch64-gnu-llvm-20-1 failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

   Compiling smallvec v1.15.1
error: not automatically applying `DerefMut` on `ManuallyDrop` union field
   --> /cargo/registry/src/index.crates.io-1949cf8c6b5b557f/smallvec-1.15.1/src/lib.rs:646:22
    |
646 |         NonNull::new(self.inline.as_mut_ptr() as *mut A::Item).unwrap()
    |                      ^^^^^^^^^^^
    |
    = 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

[RUSTC-TIMING] smallvec test:false 0.281
error: could not compile `smallvec` (lib) due to 1 previous error
warning: build failed, waiting for other jobs to finish...
[RUSTC-TIMING] quote test:false 0.443

[eggyal]

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[eggyal]

Hm. Well, that build failure isn't wrong... the smallvec crate is indeed accessing the contents of a ManuallyDrop field of a union through an implied DerefMut... but clearly this change would cause a lot of breakage. I suppose this should be a (suppressible) lint, rather than a hard error?

[eggyal]

That said, reading the motivating Potential pitfalls around DerefMut section of RFC 2514 leads to me to believe the intention is that this should be a hard error, albeit now a breaking change to stable?

Not sure who needs to be brought in on resolving this? @RalfJung (as author of the RFC and current implementation), can you point me the right way or ping the relevant team?

[RalfJung]

I lost all context for this.^^

Can you summarize what the problem is?

[eggyal]

On reflection, I'm not sure that there's an issue here after all. The reporting issue cited the following discrepancy:

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
}

The reason for the error is that assignment might cause a drop handler to execute on an uninitialised value.

But, if I (now) understand correctly, the error in the former case is necessary because the unsafe block can be omitted for assignment through owned u; whereas in the latter case no error is required because one must use unsafe to access r.f through a reference irrespective that it's an assignment.

If consistency is required, then perhaps the error should be suppressed in the former case when it's already in an unsafe block, but personally I don't think that's necessary.

Feel free to close if there's nothing more here.

[RalfJung]

I'm not sure I follow -- the assignment is unsafe either way precisely because the destructor gets called.

Can you explain what smallvec does and why it triggers your version of the lint?