Borrow checker unsoundness with unions

[petrochenkov]

#![allow(unused)]

#[derive(Clone, Copy, Default)]
struct S {
    a: u8,
    b: u8,
}
#[derive(Clone, Copy, Default)]
struct Z {
    c: u8,
    d: u8,
}

union U {
    s: S,
    z: Z,
}

fn main() { unsafe {
    let mut u = U { s: Default::default() };
    
    let mref = &mut u.s.a;
    let err = &u.z.c; // This line compiles, but it certainly shouldn't ...
    drop(mref); // ... (at least if `mref` is used after `err`)
}}

"Cousins" of borrowed union sub-fields (and their further children) are not marked as borrowed.
The same bug should happen with move checking as well, but I haven't made an example yet.

[nikomatsakis]

This is still true in NLL, though I sort of thought we had tried to fix this.

[nikomatsakis]

I'm tagging this as WG-compiler-nll; I think it's something we should fix there.

[joshtriplett]

For the sake of potential testsuite additions: this also should not compile with a simultaneous borrow of u.s.a and u.z.d.

[davidtwco]

I'll take a look at this one.

[nikomatsakis]

I'm not really sure what the bug is here, but I can explain what I think should be happening. This function:

rust/src/librustc_mir/borrow_check/mod.rs

Line 1630 in fdc18b3

fn place_element_conflict(&self, elem1: &Place<'tcx>, elem2: &Place<'tcx>) -> Overlap {

is responsible for determining when two Place values overlap. I expect us to say that two fields from a union are always considered to potentially overlap. In this case, u.s and u.z would be considered to overlap. Specifically, comparing those two fields would return Arbitrary:

rust/src/librustc_mir/borrow_check/mod.rs

Line 1615 in fdc18b3

Arbitrary,

This is the code that I think should be responsible for doing that:

rust/src/librustc_mir/borrow_check/mod.rs

Lines 1675 to 1679 in fdc18b3

	ty::TyAdt(def, _) if def.is_union() => {
	// Different fields of a union, we are basically stuck.
	debug!("place_element_conflict: STUCK-UNION");
	Overlap::Arbitrary
	}

It already looks a little fishy to me; in particular, it is returning Arbitrary if pi1 is a union, but I guess it should be equally true if pi2 is a union. Anyway, something here seems to be going awry!

[nikomatsakis]

So it seems like I forgot about how smart the compiler is now. This version of the test does error, as expected:

https://play.rust-lang.org/?gist=86c25066ab9197ebfd7a91b573f97595&version=nightly

[nikomatsakis]

I think the policy is that we will keep this bug open, but file it under #47366

[nikomatsakis]

(Since it is only fixed when NLL is enabled)

[Centril]

@nikomatsakis should this be NLL-fixed-by-NLL?

[arielb1]

Note that to get an NLL test failure, you have to use mref after declaring err:

#![allow(unused)]

#[derive(Clone, Copy, Default)]
struct S {
    a: u8,
    b: u8,
}
#[derive(Clone, Copy, Default)]
struct Z {
    c: u8,
    d: u8,
}

union U {
    s: S,
    z: Z,
}



fn main() { unsafe {
    let mut u = U { s: Default::default() };
    
    let mref = &mut u.s.a;
    let err = &u.z.c; // This line compiles, but it certainly shouldn't 
    drop(mref);
}}

[pnkfelix]

NLL (migrate mode) is enabled in all editions as of PR #59114.

The only policy question is that, under migrate mode, we only emit a warning on this (unsound) test case. Therefore, I am not 100% sure whether we should close this until that warning has been turned into a hard-error under our (still in development) future-compatibility lint policy.