Check discriminant during const eval

[Aaron1011]

In #73137 (comment), an invalid Downcast is being generated for the generator state enum (the wrong variant is used). The MIR interpreter code doesn't actually verify that the expected and actual discriminants match when performing a Downcast. While doing this would not have caught the previous issue, it would allow us to detect U.B. at the point where it actually occurs, rather than needing to wait for invalid data to be read due to the wrong downcast.

This would require us to do several operations where we currently just change the layout of an MPlaceTy, so it might make sense to only run this check in Miri (not normal const-eval).

[tmandry]

The generator MIR does this intentionally. Situations can arise where a piece of code won't know which variant a generator is in, but does know it's valid to project through a downcast to get to a particular field. That's because the field exists in multiple variants, and is in the same location regardless of which variant you access it through.

Example:

let _ = || {
  let mut count = 0usize;
  loop {
    count += 1; // <-- What variant should we use?
    if count % 2 == 0 {
      yield 2;  // Suspend0
    }
    yield count;  // Suspend1
  }
}

At the point where count is incremented, our generator could either be in the Unresumed for Suspend1 state, going into either Suspend0 or Suspend1. The code we generate should be able to access count regardless.

[tmandry]

Also, I think MIR usually initializes fields of a variant before setting the discriminant, so we would have to reverse that.

[RalfJung]

@Aaron1011 I am not sure I fully understand what you are suggesting to check here (and this two-line snippet is not very readable and about 4 times as wide as my screen^^).

Is the proposal to alter the semantics of Downcast to also read the current discriminant and compare that to the discriminant of the downcast-to variant? Is that what you mean by "expected discriminant"? As @tmandry pointed out, that is clearly not the intended behavior of the current Downcast -- currently it is just meant to be changing the type of a place without any side-effects. So this would be a major change and introduce a new kind of UB on the MIR level. Not that that's bad, but it seems non-trivial.

[Aaron1011]

Is the proposal to alter the semantics of Downcast to also read the current discriminant and compare that to the discriminant of the downcast-to variant?

Yes, that's correct

So this would be a major change and introduce a new kind of UB on the MIR level. Not that that's bad, but it seems non-trivial.

My intention was for this to catch 'obviously wrong' downcasts, like None.downcast(Some(_)). I just learned that generators can use the 'wrong' variant when downcasting, so this kind of a check might not make sense for generators (unless we introduce some notion of 'compatible variants').

[RalfJung]

IMO UB should be motivated by optimizations. Catching bugs in our MIR building is not a good enough reason to add a new class of UB.

[RalfJung]

Given what I said above, I am in favor of closing this issue -- currently the intended semantics of Downcast clearly involve it being legal to downcast to "other" variants as long as you only access fields common to both the actual and the downcast-to variant.