Allow &raw [mut | const] for union field in safe

[Kivooeo]

fixes #141264

r? @Veykril

Unresolved questions:

• Any edge cases?
• How this works with rust-analyzer (because all I've did is prevent compiler from emitting error in &raw context) (Allow &raw [mut | const] for union field  rust-analyzer#19867)
• Should we allow addr_of! and addr_of_mut! as well? In current version they both (&raw and addr_of!) are allowed (They are the same)
• Is chain of union fields is a safe? (Yes)

[rustbot]

Failed to set assignee to Veykril: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

[jieyouxu]

cc @RalfJung

[RalfJung]

Uh, the unsafety checker is not actually code I know very well, sorry.
r? compiler

[RalfJung]

Should we allow addr_of! and addr_of_mut! as well? In current version they both (&raw and addr_of!) are allowed

The macros just expand to &raw, it's not even possible to treat them differently.

[Kivooeo]

I will debug this later today but this is very weird

[Kivooeo]

@rustbot ready

[apiraino]

Seems this is now up for grab to anyone having interest to push it to the finish line. Start an FCP, then find a reviewer, IIUC.

cc @fmease that added the FCP label (comment), can you elaborate on this? thanks

[fmease]

Well, it's an insta-stable change in semantics and as such it warrants an FCP, as simple as that ;)

Likely a T-lang or T-lang+T-opsem FCP and for that it needs a lang nomination first.

[traviscross]

Seems right to me. The Reference is already correct on this matter.

@rfcbot fcp merge

cc @rust-lang/opsem

[rust-rfcbot]

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[scottmcm]

Makes sense to me. Getting the pointer is fine, and inside the allocation of the union the fields have to be GEPi too so no need to worry about that the way pointer projection does.

@rust-rfcbot reviewed

[compiler-errors]

This PR causes UB:

union A {
    a: usize,
    b: &'static &'static B,
}

union B {
    c: usize,
}

fn main() {
    let a = A { a: 0 };
    let p = &raw const (**a.b).c;
    println!("{p:?}");
}

Specifically, using global state for the nested union access is not the right choice, because it means that we suppress union accesses in arbitrary nested accesses. I don't think we should (or need to) be using a field on UnsafetyVisitor to properly implement this behavior.

View changes since this review

[Kivooeo]

Good catch, thanks! I was tried to reproduce a way to exploit this global state and am currently thinking about how to do it better. If anyone has ideas, I would appreciate it

[compiler-errors]

I don't believe this should compile. These implicitly desugar to derefs:

let ptr = &raw const (*(*(*super_outer.outer).lessouter).moreinner).a;

[RalfJung]

Yeah, projecting into a ManuallyDrop should still be unsafe.

[compiler-errors]

And allowing arbitrary derefs here is also UB, of course:

use std::ops::Deref;

union A {
    a: usize,
    b: B,
}

#[derive(Copy, Clone)]
struct B(&'static str);

impl Deref for B {
    type Target = C;

    fn deref(&self) -> &C {
        println!("{:?}", self.0);
        &C { c: 0 }
    }
}

union C {
    c: usize,
}

fn main() {
    let a = A { a: 0 };
    let p = &raw const (*a.b).c;
    println!("{p:?}");
}

// Or not implicit:
fn main2() {
    let a = A { a: 0 };
    let p = &raw const a.b.c;
    println!("{p:?}");
}

[compiler-errors]

I believe the actual implementation of this feature can be significantly simplified:

https://github.com/compiler-errors/rust/pull/new/proper-union-field-unsafety

[Kivooeo]

Yes, your approach looks more simple and fixes things above because of no leaking global state in chains. Would you mind pushing the changes directly to this PR?

[compiler-errors]

I could push directly but I also would like for you to add additional tests exercising the corner cases I pointed out, so I think you can just push the changes when you get around to adding those tests.

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Kivooeo]

Not sure if I shoulded to split this test into separate files