Check for union field accesses in THIR unsafeck

[syvb]

see also #85259, #83129, rust-lang/project-thir-unsafeck#7

r? @LeSeulArtichaut

[LeSeulArtichaut]

The implementation itself looks good to me (though, again, I'm not a reviewer).

I think we should add revisions for existing tests. If we do, we can mark the errors that we don't emit yet as

//[mirunsafeck]~ ERROR
// FIXME(thir-unsafeck)

Unsure though. Anyway I'd want to hear from @nikomatsakis

[LeSeulArtichaut]

r? @nikomatsakis

[nikomatsakis]

Hm, the actual check seems quite a bit more complicated! (cc @RalfJung, who authored those changes)

https://github.com/rust-lang/rust/blob/24379879acc0959e8ae0f3c19d249b3beb278836/compiler/rustc_mir/src/transform/check_unsafety.rs#L214-L266

I'm wondering why more tests didn't fail. I think we're going to have to tweak these rules here.

[LeSeulArtichaut]

Oh, right, you need unsafe to read from the union. (Tbh I've never used unions in Rust 😄)

I'm wondering why more tests didn't fail.

I think we don't run the existing check-fail tests, see my comment above.
EDIT: nevermind, this should be too conservative instead

[RalfJung]

All reads and some writes should be unsafe, yes -- and we should have tests covering that... tough probably not very precise ones.

[RalfJung]

Yes, the src/test/ui/union/union-unsafe.rs should help; you probably want to run that under thirunsafeck as well.

[syvb]

@RalfJung I'm trying to properly check if something is reading from a union, and encountered this weirdness. Is this a bug or feature of the MIR borrow checker?

// this is safe
match foo {
    Foo { zst: () } => {},
}
// but this is so unsafe it prints the same error message twice?
match foo {
    Foo { pizza: Pizza { topping: Some(PizzaTopping::Cheese) | Some(PizzaTopping::Pineapple) | None } } => {},
    // ^~ ERROR access to union field is unsafe and requires unsafe function or block
}

playground

[syvb]

I think that behavior in the first match is a bug in the MIR borrowck, since RFC 1444 says that pattern matching against unions must be unsafe. (and MIR borrowck is okay with it since it doesn't actually involve any reads)

[RalfJung]

I guess this has to do with how pattern matching is compiled -- matching zst against the () pattern does not actually read anything (we know the pattern matches without even looking). So it makes sense to me that the former would not be unsafe. But one could easily decide either way and both ways make sense.

[RalfJung]

Note that this pattern is unsafe:

Foo { zst: x }

[syvb]

I've updated the checking, I think it should properly detect if something is a read or write to union now (I am also now running almost all of the src/test/ui/union test suite).

[syvb]

The THIR checker now rejects code like this while the MIR checker doesn't. I'm pretty sure that this is a bug with the MIR checker though.

union Foo { bar: i8 }
let Foo { bar: _ } = Foo { bar: 5 };

union Foo2 { bar: () }
let Foo2 { bar: () } = Foo2 { bar: () };

None of those actually do anything though, so it doesn't seem like this would have any real-world implications.

(playground)

[RalfJung]

I'm on the fence about whether those should require unsafe or not -- as I said, given that matching against () is quite clearly not actually reading the union field, making this safe seems reasonable to me.

Making them unsafe would be a breaking change, so the question is also if that change is worth it. My inclination here is "no" -- either way is a good option, and so in the absence of a preference either way we should err on the side of not doing a breaking change.

What is the argument for why you consider this a bug in the MIR checker?

[syvb]

@RalfJung The MIR rules result in really odd behavior like this in safe code:

#[derive(Copy, Clone)]
#[repr(u8)]
enum OneVal {
    One = 1,
}

union Foo {
    bar: u8,
    oneval: OneVal
}

fn main() {
    match (Foo { bar: 42 }) {
        Foo { oneval: OneVal::One } => {
            println!("reached!"); // always prints
        },
    }
}

The value of Foo isn't 1 and doesn't represent OneVal::One when the first and only branch is reached, but the first arm always matches. (I'm not sure if this is UB) Even weirder, this code does require an unsafe block, even though x is being bound to a constant (OneVal::One):

match (Foo { bar: 42 }) {
    Foo { oneval: x @ OneVal::One } => { //~ ERROR access to union field is unsafe
        println!("reached!");
    },
}

And if we add another field to the enum suddenly even exhaustive matching against it using or patterns is unsafe:

#[derive(Copy, Clone)]
#[repr(u8)]
pub enum TwoVal {
    One = 1,
    Two = 2,
}

union Foo {
    bar: u8,
    oneval: TwoVal
}

fn main() {
    match (Foo { bar: 42 }) {
        Foo { oneval: TwoVal::One | TwoVal::Two } => { //~ ERROR access to union field is unsafe
            println!("reached!");
        },
    }
}

[LeSeulArtichaut]

(This is a debate that looks somewhat similar to #80059)

[syvb]

That weird behavior is actually a regression. Rust 1.19 (first stable release with unions) rejects the first snippet while Rust 1.22 accepts it.

[RalfJung]

I agree the OneVal behavior is surprising and possibly a bug, since OneVal has size 1.

But that does not imply that the () behavior is a bug. If you remove the repr(u8), the behavior you see is what I would expect.

[syvb]

I've loosened up the safety rules a little bit to allow binding union fields to wildcards:

#[derive(Copy, Clone)]
struct Pie {
    slices: u8,
    size: u8,
}

union Foo {
    bar: i8,
    baz: Pie
}

// safe with THIR and MIR checker:
match u {
    Foo { baz: _ } => {},
}

// unsafe to THIR, safe to MIR:
match u {
    Foo { baz: Pie { .. } } => {},
}
match u {
    Foo { baz: Pie { slices: _, size: _ } } => {},
}

[bors]

☔ The latest upstream changes (presumably #85259) made this pull request unmergeable. Please resolve the merge conflicts.

[syvb]

I've updated the way it checks unsafety to match the MIR rules as closely as possible. Specifically, the rule it enforces is "Irrefutable pattern matching against unions without any bindings is safe". This makes the THIR checker almost entirely match the MIR checker. The only difference I know of is

union Foo { bar: i8, pizza: Pizza }
#[derive(Copy, Clone)]
struct Pizza {
    topping: Option<PizzaTopping>
}
#[derive(Copy, Clone)]
enum PizzaTopping { Cheese, Pineapple }

let foo = Foo { bar: 5 };
match foo { Foo {
    pizza: Pizza {
        topping: Some(PizzaTopping::Cheese) | Some(PizzaTopping::Pineapple) | None
    }
} => {} };

Which the THIR checker accepts but MIR rejects. This seems to be a quirk of the way the MIR desugars nested or expressions.

[RalfJung]

"Irrefutable pattern matching against unions without any bindings is safe"

Uh... is that really what the old checker did? "Irrefutable" is a very subtle notion, in particular when considering unions.
The patterns we were talking about, like (), aren't just irrefutable -- they were literally NOPs without even considering the "set of possible values", since two zero-sized values are always equal! Anything that actually matches on a non-zero-sized enum should IMO definitely be unsafe.

[syvb]

@RalfJung As far as I can tell that's what's happening. Here's another example:

union Foo { bar: i8 }
let foo = Foo { bar: 5 };
match foo { Foo {
    bar: i8::MIN..=i8::MAX, // safe since this always matches
} => {} };

(playground)

Perhaps it would be better to change to rejecting patterns like this and only make an exception for ZSTs?

[RalfJung]

Hm, interesting.
I'm honestly not sure any more what would be better.^^ Looks like T-lang needs to make a decision here about which patterns on union fields should be considered safe and which need unsafe.

[bors]

☔ The latest upstream changes (presumably #86378) made this pull request unmergeable. Please resolve the merge conflicts.

[oli-obk]

r? @oli-obk

[oli-obk]

I just found a small edge case that is probably an unreachable pattern.

The union "drop field" assignment unsafety can be solved in a follow up PR, they are marked with FIXMEs after all.

[oli-obk]

@bors r+

[bors]

📌 Commit b86ed4a has been approved by oli-obk

[bors]

⌛ Testing commit b86ed4a with merge 240ff4c...

[bors]

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 240ff4c to master...

[LeSeulArtichaut]

Moved the discussion about pattern matching on unions to #87520.