Modifying a single subpattern in match changes MIR dramatically

[fee1-dead]

This is originated from ~const Drop, but is applicable to non-const functions as well. The problem is best seen as the MIR output for two implementations of Result::ok with a minor change: (play)

pub fn wildcard<T, E>(res: Result<T, E>) -> Option<T> {
    match res {
        Ok(x) => Some(x),
        Err(_) => None,
    }
}

pub fn bind_to_variable<T, E>(res: Result<T, E>) -> Option<T> {
    match res {
        Ok(x) => Some(x),
        Err(_x) => None,
    }
}

I won't post the whole MIR output here, but let me illustrate this issue with a handwritten control flow graph:

           wildcard
              |
switch on discriminant for `res` ------------ `Ok(x)`
              |                                 |
            `Err`             Move `x` into return value as `Some(x)`
              |                                 |
Set return value as `None`                      /
              |---------------------------------
              |
switch on discriminant for `res` ------------ `Ok`
              |                                 |
            `Err`                               |
              |                                 |
    `drop(res: Result<T, E>)`                   /
              |---------------------------------
              |
            return



       bind_to_variable
              |
switch on discriminant for `res` ------------- `Ok(x)`
              |                                  |
            `Err`             Move `x` into return value as `Some(x)`
              |                                  |
   Move value in `Err` to `_x`                   |
              |                                  |
        `drop(_x: E)`                            |
              |                                  |
    Set return value as `None`                   /
              |----------------------------------
              |
            return

wildcard is unexpectedly more complex and performs switchInt on x for two times instead of one. I think ideally we should always generate MIR as the second graph.

See #92433 (comment) and Zulip

[camelid]

_ patterns in general have special behavior, so it's not totally surprising that changing _ to _x leads to MIR changes. By the way, it'd help understanding the issue if you renamed some of the variables so they're not all named x ;)

[nbdd0121]

I think this is expected. In bind_to_variable case the enum is completed destructured and thus res is uninitialized at the end of block and thus drop elaboration won't generate drop code. In the wildcard case though the Err(_) arm wouldn't move the error, so at the end of block res is "maybe initialized" so the drop elaboration would have to add a switch to drop it.

[fee1-dead]

I'm not saying this is unexpected, but wouldn't it make more sense to change this specific case to all use the second control flow? It then wouldn't cause errors in const fn saying that destructors cannot be run at compile time because dropping Result<T, E> could mean dropping T and we only want to bound E: ~const Drop.

[nbdd0121]

It might make sense as an optimisation. But even with precise_enum_drop_elaboration we've had #90752, so I am not sure that this optimisation should happen before const drop check even if we have it.

cc @ecstatic-morse who's more knowledgeable in this area.

[ecstatic-morse]

I'd have to look at drop elaboration again to see what exactly is going on. Presumably dropping the whole Result even though we've just checked the discriminant makes codegen easier when there's more than two enum variants.

I still think it's valuable to make pre-const-check drop elaboration more precise when possible. It's bad UX when rustc demands that you write an unnecessary bound for a type that is never dropped. However, the "precise drop" stuff is in a precarious position after #90752, and I'd like to get some buy-in from the compiler team before tweaking it.