[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

[dianne]

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.

on a technical level, there's two things we need, which are encompassed by the properties TC mentioned:

• match lowering and exhaustiveness checking assume that any places we test¹ don't change in a way that can affect test results; we rely on tests we've done holding throughout the match. if we can break that assumption, our implementations of both of those are unsound².
• when an arm matches, the places we copy/move from to create by-value bindings must still be valid to access through the path used by pattern matching³. this can similarly be expressed as needing certain test results not to change before bindings are created⁴. this isn't needed for by-ref bindings since they're borrowed normally before the guard executes.

the way we do this is through fake borrows: we shallowly fake-borrow any places we test and any references or pointers that we go through to reach them when matching³. we store the fake borrows in temporaries and fake-read those temporaries at the points where we rely on guards upholding the above properties. in polonius terms⁵, this should keep the loan introduced by a fake borrow active as long as we need⁶ it to be, as long as we're bug-free and don't remove it from scope early (as in #160599). mutably accessing matched-on places at points where we need⁶ those places not to be mutated will thus invalidate an active loan, resulting in a conflict error.

the problem is that these fake reads were only on the guard's success path; they were ensuring bindings could be created, but not that test results would remain unchanged if the guard could never succeed after a fake-borrowed place was mutated⁷. in order to maintain that property, we need the fake borrow temporaries to be live at the start of the path back into the match body on guard failure. thus these PRs all add the missing fake reads (with varying precision as determined by differing interpretations of what they express abstractly).

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.

I'll make a unified test suite soon and rebase all the PRs to use it.

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.

for what it's worth, I agree with this. I see #161581 as a separate change from the extra precision this adds, though I understand it could be useful to keep the shared context in mind while considering both changes (hence opening this PR but basing it on #161581). all the more reason to go back and fix up the test suite ^^

to restate the motivation for extra precision more bluntly, it makes the language semantics agree more closely with a description from first principles of why we need to prevent mutation of matched-on places in guards. it (hopefully) shouldn't affect any code anyone would actually write, so the value is purely in how we want to be able to describe the semantics of pattern matching.

---

as for process, is it generally fine for anyone to types-nominate PRs or ping the whole types team until someone notices? I'd been meaning from the start to propose #161581 to T-types since it is a borrow-checking change, but I'm less familiar with types team procedure. I thought T-lang might also be interested given that this is also entwined with how we can describe abstract properties of pattern matching, so I wanted to check with them too, even if the final decision was delegated entirely to types. my usual approach is to request a types reviewer so they can propose FCP, but I wasn't sure if anyone on rotation at the time was familiar with match lowering specifically (though by this point I've hopefully written enough of a primer on the relevant parts!). would it have been better to wait or roll a random types reviewer? my plan if this didn't get noticed was to ask if I could crash a types team meeting to bring it up, but that ended up not being necessary (and I hadn't accounted for this week being EuroRust). in any case, I'm sorry I wasn't more proactive. I'll keep this in mind for future PRs of mine which I suspect should belong to types.

---

regarding the needs-crater This change needs a crater run to check for possible breakage in the ecosystem. , since this should be covered by the crater run done on the more-restrictive #162242, my guess would be that it's fine to leave it for the beta crater runs. including this or #161581 in a crater rollup would also be fine if we want to be extra sure that we don't make beta crater triage harder, but I think a dedicated run would probably be overkill.

Footnotes

1. e.g. to see if the discriminant of an Option<T> is that of Some or None, if an integer falls within a range, or if a slice fits the bounds specified by a slice pattern ↩

2. e.g. most of the tests follow this template: if we test that a place is Some(_), we shouldn't have to test it again. if it's changed to None by a guard, we may then enter an arm that expects it to be Some(_), at which point accessing its field to create bindings or do further tests is unsound. ↩

3. ignoring #![feature(deref_patterns)], all places accessed by pattern matching are relative to the scrutinee. any derefs in a matched-on or bound place will be done freshly at the time that test is done or that binding is created. that means reassigning to swap out a reference could change the results of tests on its referent without conflicting with borrows on places within it (since assignment is a shallow access). ↩ ↩²

4. for stable Rust, we specifically can't change the results of tests required to access fields or slice elements the that by-value bindings of the current arm are nested within; that's what this PR captures. currently and in put fake reads for guards' fake borrows on the guard's failure path #161581, we over-approximate by requiring that no tests can change before making by-value bindings⁷. ↩

5. n.b. I'm not that familiar with polonius yet, so I'm taking this as an opportunity to learn. my terminology is going to be a bit mixed up between introductions to it, the new implementation in the compiler, the stable borrow checker, and maybe also accidental peeks at legacy polonius. I'll happily clarify if something I said doesn't make sense. ↩

6. with varying over-approximations for each of these PRs of where fake borrow temporaries "need" to be kept live ↩ ↩²

7. since borrow-checking is sensitive to CFG shape, if a divergent path from a guard doesn't include fake reads for fake borrow temporaries, those temporaries won't be live on that path, so the corresponding borrows/loans go out of scope. as such, it's necessary to have fake borrows reachable both when going back into the match body and when creating bindings, and it's possible for them to differ if desired for precision in cases where failure isn't reachable (i.e. we don't go back into the match body). ↩ ↩²

[dianne]

I have an amendment to the above. writing up the background for how fake borrows work for slice indexing reminded me that there's a third reason we create fake borrows for matches: when the scrutinee involves slice indexing, we need to add fake borrows to keep guards from replacing those slices, since that could invalidate bounds checks¹. e.g.

fn main() {
    let mut x: &[u8] = &[0];
    let y: &[u8] = &[];
    match x[0] {
        _ if { x = y; false } => {}
        a => {}
    }
}

this is rejected (as it should be): creating the binding a would perform an out-of-bounds access. it reads the place (*x)[0], but at that point x is empty. we need to ensure bounds checks for any slice indexing within the scrutinee place² hold throughout the match if any patterns access the scrutinee at all. per my above comment, if the match has any tests, we'd add the necessary fake borrows anyway to keep those tests from changing. but we also need to keep slices from being replaced in guards if the scrutinee is accessed through a binding.

this happens to work fine with the model (and implementation, I think!) proposed by this PR, though I'll need to add tests when I redo those. as long as we create fake borrows on those places (which we do),

• all fake borrows are kept alive when going back into the match on guard failure, so the guard can't replace slices in the scrutinee if we can reach another arm that may have a binding in it.
• if the current arm contains a by-value binding, then on guard success we'll keep alive the fake borrows on places that are prefixes of it, which will include any derefs before slice indexing in the scrutinee. as above, we don't need to worry about by-ref bindings if a guard can never fail, since those bindings are created before the guard executes and will be unchanged if references to indexed-into slices in the scrutinee are swapped out.

however, we don't create fake borrows only on those places. the compiler implementation for identifying that we need those fake borrows is more restrictive: for any binding³, the compiler identifies that it should shallowly fake-borrow any references/pointers that are dereferenced to reach that binding (regardless of whether slice indexing is involved). the comments in the compiler include an example where this isn't currently required for soundness. similarly, on guard success, we insert fake reads of the by-ref versions of that arm's by-value bindings (which are used instead within the guard to avoid moving before guard success)⁴; I don't think these are required for soundness (at least not anymore). I don't have a proposal at the moment for how to think about the current behavior as a whole. apart from liveness issues due to control-flow, it covers what's needed for soundness, plus a bunch of other things that I can't figure out a fully cohesive description for. I could abstractly describe what certain subsets of fake reads do, but not as a unified whole. mostly, it aims to keep the scrutinee from being mutated, but in varying ways that don't feel like they fit together nicely to me. for reference, the comment in current main is below.

rust/compiler/rustc_mir_build/src/builder/matches/util.rs

Lines 207 to 228 in bf9448e

	// Insert a borrows of prefixes of places that are bound and are
	// behind a dereference projection.
	//
	// These borrows are taken to avoid situations like the following:
	//
	// match x[10] {
	// _ if { x = &[0]; false } => (),
	// y => (), // Out of bounds array access!
	// }
	//
	// match *x {
	// // y is bound by reference in the guard and then by copy in the
	// // arm, so y is 2 in the arm!
	// y if { y == 1 && (x = &2) == () } => y,
	// _ => 3,
	// }
	//
	// We don't just fake borrow the whole place because this is allowed:
	// match u {
	// _ if { u = true; false } => (),
	// x => (),
	// }

Footnotes

1. this sounds a lot like why we create fake borrows for slice indexing. it is! it's exactly the same. however, the fake borrows we create for slice indexing are only kept alive until the final bounds check, so that bounds checks aren't invalidated within subsequent index expressions. by the time we enter the match, those are all out of scope⁵, so we usually need to (re)create them if the match has guards ↩

2. fake borrows to preserve bounds checks done within the match by slice patterns are already covered by the previous property that we need test results to remain unchanged ↩

3. that isn't under a #![feature(deref_patterns)] deref pattern ↩

4. I intentionally haven't messed with these or mentioned them in my descriptions. this started as a soundness fix, so I've described what I believe is necessary for soundness without exposing too many details of match lowering (modulo some holes in my memory at times; oops). maybe I should have brought them up as this began to lurch towards wanting an all-encompassing description of how we borrow-check patterns and scrutinees. if that's what we're proposing to figure out here, I'd encourage moving the FCP to put fake reads for guards' fake borrows on the guard's failure path #161581 so we can merge the bug fix separately ↩

5. and if the final bounds check is for a slice index, it doesn't even get a fake borrow as part of index expression lowering, since there aren't any more index expressions after that could invalidate it ↩