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

[dianne]

View all comments

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

Concerns:

• complexity (put guards' fake reads in the failure block and only put necessary fake reads in the success block #163948 (comment))
• details-and-motivation (put guards' fake reads in the failure block and only put necessary fake reads in the success block #163948 (comment))

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.

[tmandry]

On the mental model, I want to double check a property I expect to be true:

The success or failure of each arm's pattern should be independent of the other arms; it should never be allowed to change based on code in an earlier arm's guard.

This might be implied by TC's point 3 about "how the compiler orders tests", but I'm not sure whether that means within an arm or globally within the match.

[RalfJung]

That's definitely not true when considering interior mutability.

[Nadrieril]

The success or failure of each arm's pattern should be independent of the other arms; it should never be allowed to change based on code in an earlier arm's guard.

Yes, that's the property we'd like, and in fact it's necessary for soundness: if a guard can change the outcomes of further tests within the match, then exhaustiveness checking is no longer enough to ensure at least one arm of the match will succeed.

That's definitely not true when considering interior mutability.

Even with just raw pointers you can invalidate the property above, but not in safe code: there is currently no way for a pattern to inspect data that's inside an interior mutable container.

[RalfJung]

there is currently no way for a pattern to inspect data that's inside an interior mutable container.

Yeah but two guards can still influence each other that way.

Re-reading what Tyler wrote I guess I am not sure whether "The success or failure of each arm's pattern" includes the guard; I thought it would.

[Nadrieril]

Re-reading what Tyler wrote I guess I am not sure whether "The success or failure of each arm's pattern" includes the guard; I thought it would.

Oh, I understood "pattern" to not include the guard. If we include the guards, then you don't even need interior mutability (play):

fn main() {
    let mut x = 0;
    match true {
        true if { x += 1; false} => unreachable!(),
        true if x == 1 => {}
        _ => unreachable!(),
    }
}

[RalfJung]

Oh right I forgot guards can mutate outside variables.

there is currently no way for a pattern to inspect data that's inside an interior mutable container.

That's only true outside the module that defines UnsafeCell. 🙃

[tmandry]

Yes, I was excluding guards in my statement above. It seems like we are in agreement about the model here, so

@rfcbot reviewed

[nikomatsakis]

@rfcbot reviewed

[scottmcm]

Thanks for exploring so many directions here, dianne!

@rfcbot reviewed

[jackh726]

I'm trying to compare with #161581, but tests are largely different.

Can we land tests separately (at least as a separate commit, but ideally as a separate PR that each of these candidate changes are based on)? That way we can see how each PR modifies them.

View changes since this review

[jackh726]

I am missing an example like

    let mut x: Option<&u64> = Some(&7);
    match x {
        Some(ref b) if { x = None; false } || return => {}
        _ => {}
    }

Does this pass?

[jackh726]

Are these disallowed in #161581?

[jackh726]

Some(_) if { x = None; false } || return => {} and Some(_) if false && ({ x = None; false } || return) => {} are missing, which is in #161581.

[jackh726]

@rustbot concern details-and-motivation

I am reading over the PR description, the changes, and the tests - and I am still not following on exactly the principles trying to be upheld here in terms how and when guard blocks invalidate loans from the match arm patterns.

TC's comment in #163948 (comment) sort of does this:

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.

But this very much lacks the technical details of how we do that.

As I said in my review comments, tests here don't line up with tests in #161581 and #162242, which makes it harder to evaluate these changes against each other. I would also expect a test that covers the exact crater regression we saw in #162242.

@rust concern complexity

I understand the desire for extra preciseness of this PR compared to #161581. As said above, I'm not even really clear what exactly the extra preciseness in this PR even gives us.

Nonetheless, I'm not convinced that the extra preciseness here is warranted compared to the added complexity. I would much rather land #161581 to fix the unsoundness, then maybe later add extra preciseness here.

---

I will also say: I am very frustrated with the process both here and in #161857. This is very clearly in T-types territory - both the implementation and the logic here is extremely subtle. I don't necessarily agree that T-lang should be involved here, but also don't particularly care from a T-types perspective if the team wants to be (as a lang advisor, I think the team should be leaning into its delegation to T-types, not away from it). HOWEVER, like I said this is clearly something that T-types must be involved in, and the fact that both FCPs here and there started without T-types at first is imo pretty demeaning and distasteful.

[rustbot]

Error: Cannot set @rustbot concern on an active rfcbot FCP, use @rfcbot concern instead.

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #triagebot on Zulip.

[jackh726]

@rfcbot concern details-and-motivation

@rfcbot concern complexity

see above