put guards' fake reads in the failure block and only put necessary fake reads in the success block

[dianne]

This is #161581 (rebased onto a fresher main branch) but with an additional commit relaxing fake reads of fake borrows on guards' success branches: on guard success, we only keep alive the fake borrows required to ensure we can create that arm's by-value bindings. This is unlikely to come up in practice, but it permits a more compelling explanation of why we prevent mutation of matched-on places in guards.

This is now allowed; none of the guards can fail after assigning to x.0:

fn main() {
    let mut x: (Option<&Box<u64>>, u64) = (Some(&Box::new(7)), 0);
    match x {
        // By-ref bindings are created before evaluating the guard, so we don't need fake borrows
        // to catch when they would be invalidated. As long as we don't use `b` after, it's ok.
        (Some(ref b), c) if { x.0 = None; false } || return => {}
        (Some(ref b), c) if false && ({ x.0 = None; false } || return) => {}

        // The assignment doesn't invalidate borrows of the lifetime-extended temporary box. It's
        // unchanged even when `x` is set to `None`, so it's fine to read from after.
        (Some(&ref b), c) if { x.0 = None; false } || return => { **b; }
        (Some(&ref b), c) if false && ({ x.0 = None; false } || return) => { **b; }

        a => { a; }
    }
}

Since this includes the change made by #161581, it fixes #161578

[dianne]

dealing with deref patterns is a tiny bit messy, but hopefully it's not too bad? this isn't as efficient as it maybe could be, but I expect it's not a bottleneck. (edit: it indeed seems like the perf impact is negligible)

I'll also lang-nominate this with all the others.

[rust-bors]

☀️ Try build successful (CI)
Build commit: e4cc928 (e4cc9287c58a7ec7dcbbded5f93412a12a2b4d76)
Base parent: 8d1a764 (8d1a76430406c877b35d0b627e7f796dcf0dfeca)

[rust-timer]

Finished benchmarking commit (e4cc928): comparison URL.

Overall result: ✅ improvements - 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
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.3%	[-0.3%, -0.3%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary 0.8%)

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)	3.0%	[2.5%, 4.0%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.5%	[-2.7%, -2.2%]	2
All ❌✅ (primary)	-	-	0

Cycles

Results (primary -2.5%, secondary 5.0%)

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)	5.9%	[1.8%, 16.3%]	9
Improvements ✅ (primary)	-2.5%	[-2.5%, -2.5%]	1
Improvements ✅ (secondary)	-2.5%	[-2.5%, -2.5%]	1
All ❌✅ (primary)	-2.5%	[-2.5%, -2.5%]	1

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 489.893s -> 491.696s (0.37%)
Artifact size: 408.60 MiB -> 409.31 MiB (0.17%)

[traviscross]

To summarize: for soundness, we add fake shallow borrows and fake reads in match lowering. But we have a problem (#161578): we're inserting reads after guard success but not after guard failure, which allows later arms to read memory made invalid by guards of earlier arms. How do we fix this?

Obviously we need to add the fake reads after guard failure. But this problem raises the question of what we're trying to express, fundamentally. We asked @dianne to explore whether we should forbid mutation of the relevant places anywhere in guards, as a possible way to simplify the mental model. @dianne tried that in #162242. But this turns out to not be so simple. The fake reads necessary for this also prevent moving values during early exits. This breaks reasonable-looking code, found by crater. That's enough to convince me we shouldn't do that, as it would be in tension with the intuitions that people pick up, due to NLL, about how borrows work in Rust otherwise. Niko was also persuaded.

This suggests another model, which @dianne set out in #162242 (comment): adopt precise semantics that place the borrows and the reads so as to achieve articulable language properties. What are those properties?

1. Checks we've done earlier remain valid as we evaluate the arms sequentially.
2. When an arm matches, the by-value bindings can be created.
3. Whether a guard is valid does not depend on how the compiler orders tests or reuses results.

Upholding those properties suggests that we (fake) borrow each place the match inspects and every reference it reads through; on guard failure, we (fake) read all of those borrows; on guard success, we (fake) read only the borrows needed to create the arm's by-value bindings.

That's what this PR does. This is both a breaking change for the soundness fix (we're not expecting ecosystem breakage) and a change that accepts new code (due to being more precise about the reads after guard success). Let's (propose to) do it.

@rfcbot fcp merge lang

(Cc @rust-lang/types, @rust-lang/opsem. If you want in on this FCP (#161581 (comment)), no worries, say so and I'll restart.)

[dianne]

r? @matthewjasper or @Nadrieril or anyone familiar with fake borrows and deref patterns

[rustbot]

Some changes occurred in match lowering

cc @Nadrieril

[lcnr]

(Cc @rust-lang/types, @rust-lang/opsem. If you want in on this FCP (#161581 (comment)), no worries, say so and I'll restart.)

same reasoning as in #161857 (comment) 😊

[traviscross]

@rfcbot fcp merge lang,types

[rust-rfcbot]

@traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @BoxyUwU
• @jackh726
• @joshtriplett
• @lcnr
• @nikomatsakis
• @oli-obk
• @scottmcm
• @spastorino
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.