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

Semantics of SetDiscriminant with niched variants #487

Open
RalfJung opened this issue Jan 7, 2024 · 5 comments
Open

Semantics of SetDiscriminant with niched variants #487

RalfJung opened this issue Jan 7, 2024 · 5 comments

Comments

@RalfJung
Copy link
Member

RalfJung commented Jan 7, 2024

This is motivated by rust-lang/rust#119674:

For a type like enum E { A, B(char) }, currently SetDiscriminant(place, 1) is just a NOP, since niche-encoded variants do not need to store the discriminant explicitly.

However, that means that SetDiscriminant; Discriminant doesn't necessarily return the just-written discriminant, even for enums: we might be calling SetDiscriminant(place, 1) on a place that doesn't actually carry the niched value, but it remains a NOP. That seems unfortunate.

I think it'd make sense if SetDiscriminant on niched variants were to do the equivalent of read_discriminant, and cause UB if the already encoded discriminant does not match the niched variant.

Cc @cjgillot @tmiasko

@tmiasko
Copy link

tmiasko commented Jan 7, 2024

SetDiscriminant is only used for coroutines. Unless there are plans to reintroduce it for ADTs, I would go in the direction of restricting it to coroutines and eventually replacing it with a field access.

@RalfJung
Copy link
Member Author

RalfJung commented Jan 7, 2024

SetDiscriminant is only used for coroutines.

That's news to me. Since when? How are enum variants set?

@cjgillot
Copy link

cjgillot commented Jan 7, 2024

As we do not deaggregate ADTs any more, we only construct enum variants using an aggregate assignment. So rustc does not produce SetDiscriminant on enums any more.

@RalfJung
Copy link
Member Author

RalfJung commented Jan 7, 2024

Oh I see. Internally in the MIR semantics that will still be a bunch of assignments followed by SetDiscriminant though... but it's probably impossible to hit the "bad" case that way.

Still, having SetDiscriminant "verify" the discriminant it wrote makes a lot of sense to me.

@RalfJung
Copy link
Member Author

Still, having SetDiscriminant "verify" the discriminant it wrote makes a lot of sense to me.

rust-lang/rust#120882 implements that in Miri.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 11, 2024
…ler-errors

interpret/write_discriminant: when encoding niched variant, ensure the stored value matches

Cc rust-lang/unsafe-code-guidelines#487
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 11, 2024
Rollup merge of rust-lang#120882 - RalfJung:set-discriminant, r=compiler-errors

interpret/write_discriminant: when encoding niched variant, ensure the stored value matches

Cc rust-lang/unsafe-code-guidelines#487
RalfJung pushed a commit to RalfJung/miri that referenced this issue Feb 12, 2024
interpret/write_discriminant: when encoding niched variant, ensure the stored value matches

Cc rust-lang/unsafe-code-guidelines#487
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants