rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
97.97k stars 12.69k forks source link

Further restricting what coercions are allowed on places of type `!` #131297

Open compiler-errors opened 2 weeks ago

compiler-errors commented 2 weeks ago

In #129392, I fixed a couple soundness holes having to do with the distinction between values and places and the ! type, which has special divergence and coercion behavior in HIR typeck.

To do so, I implemented a heuristic called expr_guaranteed_to_constitute_read_for_never (and same for patterns, pat_guaranteed...), which determines whether an expression/pattern is guaranteed to perform a read of a value of type !, which triggers the special divergence and coercion behavior. Read the description of the PR to see what the behavior is after the PR.

There's still some open questions about what expressions and patterns should be considered reads in the HIR (such as reading structs with only field subpatterns that don't read, like Struct { .. }), but I'd like to separate that since the PR #129392 is strictly an improvement over the existing behavior.

compiler-errors commented 2 weeks ago

@lcnr also brought up a good point that ref/ref mut bindings probably also don't constitute reads. We don't allow coercions on let if ref/ref mut right now anyways, so it's less of an issue.

RalfJung commented 2 weeks ago

Cc @rust-lang/opsem

digama0 commented 2 weeks ago

The question about struct and 1-variant enum matches also leads to the following formulation of the question:

Is the following UB?

#[repr(u8)]
enum Foo {
    Foo,
}
fn main() {
    let x: Foo = unsafe { std::mem::uninitialized() };
    let Foo::Foo = x;
}

I think the answer is no, it is defined behavior to do a discriminant read on a 1-variant enum because nothing needs to be read and there is no data to read in the first place (Foo::Foo is all padding, so the fact that it is uninit makes no difference).

If we agree on this, then I think we should say that struct patterns, and 1-variant enum patterns, should not be considered reads in this HIR analysis.

compiler-errors commented 2 weeks ago

If we agree on this, then I think we should say that struct patterns, and 1-variant enum patterns, should not be considered reads in this HIR analysis.

Yep, that's approximately what I was thinking.

jwong101 commented 2 weeks ago

I think the answer is no, it is defined behavior to do a discriminant read on a 1-variant enum because nothing needs to be read and there is no data to read in the first place.

I could see that argument for repr(Rust) 1-variant enums. But the Foo enum has an explicit discriminant representation of type u8 that can only have 0 as it's value(the default discriminant value). Codegen currently exploits that by slapping range metadata on parameters+returns of type Foo.

RalfJung commented 2 weeks ago

Yeah I think that code is definitely UB on the let x: Foo = unsafe line. It doesn't have a padding byte, it has a 1-byte discriminant, and that must be initialized.