put fake reads for guards' fake borrows on the guard's failure path

[dianne]

Conceptually, I think it makes sense to have fake reads be on guard failure, since it's the failure path where we continue matching (and thus it's soundness-critical to prevent mutation). Practically, this fixes #161578.

This is technically a breaking change, so it'll need a crater run and FCP.

[dianne]

@bors try

[dianne]

I'm guessing a types fcp would make the most sense since this is kind of a borrow-checking change?

r? @oli-obk maybe since you're familiar with both MIR building and T-types procedure, but feel free to reassign ^^

[rustbot]

oli-obk is not on the review rotation at the moment.
They may take a while to respond.

[rustbot]

Some changes occurred in match lowering

cc @Nadrieril

[rustbot]

Some changes occurred in coverage tests.

cc @Zalathar

[rust-bors]

☀️ Try build successful (CI)
Build commit: 8511135 (85111357388f7cbb75083e86a3f3a1cc76d512fb)
Base parent: baf1f7c (baf1f7cc4faa79b938148ad52c53e96c0102cbc4)

[dianne]

@craterbot check

[craterbot]

👌 Experiment pr-161581 created and queued.
🤖 Automatically detected try build 8511135
⚠️ Try build based on commit 6579371, but latest commit is 80441e9. Did you forget to make a new try build?
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[dianne]

just noticed oli is on vacation; oops! @rustbot reroll

[Zalathar]

You will also need to bless the coverage-run tests with ./x test coverage --bless --set=build.profiler=true.

(You can also [build] profiler = true in your bootstrap.toml to make running/blessing the coverage-run tests work by default.)

I'm not sure whether the resulting changes to coverage output are a problem or not, but my hope is that #161517 will make them vanish anyway.

[dianne]

the coverage-run tests should be blessed now. I'm not familiar with how coverage uses spans (or how to read coverage tests), so if there is a problem I'll need some guidance. though I imagine #161517 should help if it means coverage won't scrape spans from fake reads anymore

[chenyukang]

not familiar with this part, @rustbot reroll

[dianne]

@craterbot abort

just realized that as-is this accidentally allows some new things by making guards with unreachable failure blocks but reachable success blocks ignore their fake borrows. while that should be sound, and it's consistent with unconditionally diverging guards still ignoring their fake borrows, we probably don't want to start allowing that. I expect ideally we'd want unconditionally diverging guards to respect their fake borrows too. I doubt it'd impact the crater run, but I'd prefer not to crater an out-of-date version of this

[craterbot]

🗑️ Experiment pr-161581 deleted!

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[dianne]

On further consideration, I'm not a fan of the behavior we'd get from doing the minimal fix that also doesn't allow anything new (i.e. putting fake reads in both the success and failure blocks but nowhere else). If we did that to keep things like

fn main() {
    let mut x: Option<Box<u64>> = Some(Box::new(7));
    match x {
        Some(_) if { x = None; false } || return => {}
        Some(b) => println!("{b}"),
        None => println!("none"),
    }
}

from compiling without stopping things like

fn main() {
    let mut x: Option<Box<u64>> = Some(Box::new(7));
    match x {
        Some(_) if { x = None; return } => {}
        Some(b) => println!("{b}"),
        None => println!("none"),
    }
}

from compiling, I'd want some plan to eventually change things to be more consistent, either allowing both (like this PR currently does) or rejecting both (which would be a more involved change).

In argument for rejecting both, it would best preserve the abstraction that you can't mutate x in the match guard.

In argument for accepting both, I think it makes sense that we only prevent mutation if you can continue matching after the guard.. not that I'd expect we want to expose the control-flow graph like that, but borrow-checking already does that, especially with divergence. I'd be content with being consistent with that. I don't think we'd have to expose the implementation detail of using fake borrows in order to explain that behavior.

I can't think of a mental model that works for rejecting the former but accepting the latter.