keep fake borrows alive until exiting their scopes

[dianne]

This is an alternative to #161581 and #161857 that implements a more restrictive semantics: fake borrowed places may not be mutated until execution leaves the fake borrow's scope (i.e. a match guard or the outermost indexing expression in a place expression).

Fixes #161578
Fixes #161852

In contrast to the above PRs, the following are also no longer allowed:

fn always_return_after_mutation_in_guard() {
    let mut x: Option<Box<u64>> = Some(Box::new(7));
    match x {
        Some(_) if { x = None; return } => {}
        //~^ ERROR: cannot assign `x` in match guard
        Some(b) => println!("{b}"),
        None => println!("none"),
    }
}

fn always_return_after_mutation_in_indexing() {
    let mut x: &[&[&[u32]]] = &[&[&[0]]];
    let y: &[&[&[u32]]] = &[];
    x[0][{ x = y; return; 0 }];
    //~^ ERROR: cannot assign `x` in indexing expression
}

Not being sensitive to CFG shape should make this easier to describe at a high-level than #161581 and #161857 would be. The tradeoff here is implementation complexity: since borrow-checking is sensitive to CFG shape, this puts fake reads at every point where execution leaves fake borrows' scopes, as though the fake reads are drops. The resultant MIR might not be quite as nice in some cases as something purpose-built, but I think it's worth being able to reuse the machinery we have for drops.

[dianne]

@bors try @rust-timer queue

[dianne]

maybe this should have a test involving coroutine drops just to make sure nothing breaks horribly?

View changes since the review

[dianne]

Not sure we'll have crater results by the next lang meeting, but I'll lang-nominate this alongside #161581 and #161857. This implements the more restrictive semantics we discussed in yesterday's meeting.

[rust-bors]

☀️ Try build successful (CI)
Build commit: 4162643 (416264364aa47959f5827723e1d7dabc95d0744b)
Base parent: d8df826 (d8df82673d5911b6112a85bf91d9adefb2c66a1a)

[rust-timer]

Finished benchmarking commit (4162643): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.4%	[0.4%, 0.4%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary 4.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.4%	[1.5%, 7.7%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results (secondary 1.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.2%	[3.1%, 5.6%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-5.1%	[-6.0%, -4.1%]	2
All ❌✅ (primary)	-	-	0

Binary size

Results (primary 0.0%, secondary 0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.1%]	15
Regressions ❌ (secondary)	0.1%	[0.0%, 0.1%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[0.0%, 0.1%]	15

Bootstrap: 477.112s -> 479.569s (0.51%)
Artifact size: 401.86 MiB -> 401.04 MiB (-0.20%)

[dianne]

@craterbot check

[craterbot]

👌 Experiment pr-162242 created and queued.
🤖 Automatically detected try build 4162643
🔍 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]

r? @matthewjasper since I gave you those other PRs, but feel free to reassign.

To address the pings regarding changing MIR syntax: I had to add derives for Eq and Hash to FakeReadCause. No actual syntax change has occurred.

[rustbot]

Some changes occurred in coverage tests.

cc @Zalathar

Some changes occurred in match lowering

cc @Nadrieril

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @vakaras

[dianne]

Another note for language team consideration: this only affects fake borrows. Separate from that, we also prevent mutation within guards of places we move/copy from to produce that arm's by-value bindings. That behavior is unaffected by this PR:

fn fails() {
    let mut x = 0;
    match x {
        // These are forbidden: there's a fake read for `y`'s ref-within-guard
        // binding after the guard succeeds.
        y if { x = 1; true } => {}
        //~^ error[E0506]: cannot assign to `x` because it is borrowed
        y if { x = 1; true } || return => {}
        //~^ error[E0506]: cannot assign to `x` because it is borrowed
        _ => {}
    }
}

fn works() {
    let mut x = 0;
    match x {
        // The fake read isn't present on failure, so this is fine:
        y if { x = 1; true } && return => {}
        // The fake read isn't present for by-ref bindings, so this is fine as
        // long as we never use `y`.
        ref y if { x = 1; true } => {}
        _ => {}
    }
}

If changing this would be desirable, I'd prefer to save doing so for a follow-up.