Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

What is the behavior of a place-to-value/reference conversion of a variant place? #120369

Open
RalfJung opened this issue Jan 26, 2024 · 3 comments
Labels
T-opsem Relevant to the opsem team

Comments

@RalfJung
Copy link
Member

RalfJung commented Jan 26, 2024

MIR in principle can represent code like this (but custom MIR currently cannot):

fn enum_shenanigans(_1: S) -> () {
    let mut _0: ();
    let mut _2: ??;
    let mut _3: ??;

    bb0: {
        _2 = (_1 as variant#0);
        _3 = &raw (_1 as variant#0);
        return;
    }
}

I think things would explode in all sorts of ways if such MIR was fed to Miri: enum variants can have ABIs that are entirely different from the enum they are contained in, so the projection can produce ImmTy that are incoherent. Currently this works because it seems to only affect enum variants without fields / uninhabited variants (but maybe I just haven't found the right example yet), and rustc only generates variant projections that are immediately followed by field projections.

But this is not very satisfying. We have an invariant (an Immediate::Scalar must have Scalar type, and similar for ScalarPair), and that invariant is blatantly violated by valid MIR -- or at least MIR that does not break any documented rule I am aware of. It's also unclear what code like the above should even mean: given that enum variants have no type, what should the type of _2 and _3 be?

I can think of two proper solutions here:

  • Instead of a separate Variant projection, have a combined "Variant+Field" projection for accessing enum fields.
  • Give enum variants a proper type, and to ensure that downcasts can't go from Scalar to non-Scalar ABI. The fact that downcast places do not have a proper type has already caused me plenty of confusion and pain so this would be a good change to do anyway IMO, even if we don't expose those types to the user. (One day we might want to expose them as well, but that's not the goal of this issue.)

Cc @oli-obk @JakobDegen @tmiasko @cjgillot

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jan 26, 2024
@fmease fmease added T-opsem Relevant to the opsem team and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jan 26, 2024
@oli-obk
Copy link
Contributor

oli-obk commented Jan 29, 2024

We could use pattern types to represent enum variant types (which would also make it clear that they have the same representation (modulo niches))

@RalfJung
Copy link
Member Author

RalfJung commented May 27, 2024

Yeah that sounds pretty nice long-term.

Though short term I wonder if "have a combined Variant+Field projection for accessing enum fields" isn't the best choice -- basically, equip the field projection with an Option<VariantIdx>, similar to what we already do for Rvalue::Aggregate.

@rust-lang/wg-mir-opt would that work reasonably well for MIR opts or not?

@RalfJung
Copy link
Member Author

As a first step, let's declare the programs above invalid by requiring Downcast to be followed by Field: #125616.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue May 27, 2024
…jection, r=compiler-errors

MIR validation: ensure that downcast projection is followed by field projection

Cc rust-lang#120369
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue May 27, 2024
…jection, r=compiler-errors

MIR validation: ensure that downcast projection is followed by field projection

Cc rust-lang#120369
rust-timer added a commit to rust-lang-ci/rust that referenced this issue May 27, 2024
Rollup merge of rust-lang#125616 - RalfJung:mir-validate-downcast-projection, r=compiler-errors

MIR validation: ensure that downcast projection is followed by field projection

Cc rust-lang#120369
github-actions bot pushed a commit to rust-lang/miri that referenced this issue May 28, 2024
…r=compiler-errors

MIR validation: ensure that downcast projection is followed by field projection

Cc rust-lang/rust#120369
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-opsem Relevant to the opsem team
Projects
None yet
Development

No branches or pull requests

4 participants