Make closure capturing have consistent and correct behaviour around patterns

[meithecatte]

This PR has two goals:

• firstly, it fixes internal compiler error: two identical projections #137467. In order to do so, it needs to introduce a small breaking change surrounding the interaction of closure captures with matching against enums with uninhabited variants. Yes – to fix an ICE!
  • this also fixes ICE: called Option::unwrap() on a None value with refutable patterns #138973, a slightly different case with the same root cause.
• secondly, it fixes Closure captures are inconsistent between x and x @ _ irrefutable patterns #137553, making the closure capturing rules consistent between let patterns and match patterns. This is new insta-stable behavior.

Background

This change concerns how precise closure captures interact with patterns. As a little known feature, patterns that require inspecting only part of a value will only cause that part of the value to get captured:

fn main() {
    let mut a = (21, 37);
    // only captures a.0, writing to a.1 does not invalidate the closure
    let mut f = || {
        let (ref mut x, _) = a;
        *x = 42;
    };
    a.1 = 69;
    f();
}

I was not able to find any discussion of this behavior being introduced, or discussion of its edge-cases, but it is documented in the Rust reference.

The currently stable behavior is as follows:

• if any pattern contains a binding, the place it binds gets captured (implemented in current walk_pat)
• patterns in refutable positions (match, if let, let ... else, but not destructuring let or destructuring function parameters) get processed as follows (maybe_read_scrutinee):
  • if matching against the pattern will at any point require inspecting a discriminant, or it includes a variable binding not followed by an @-pattern, capture the entire scrutinee by reference

You will note that this behavior is quite weird and it's hard to imagine a sensible rationale for at least some of its aspects. It has the following issues:

• firstly, it assumes that matching against an irrefutable pattern cannot possibly require inspecting any discriminants. With or-patterns, this isn't true, and it is the cause of the internal compiler error: two identical projections #137467 ICE.
• secondly, the presence of an @-pattern doesn't really have any semantics by itself. This is the weird behavior tracked as Closure captures are inconsistent between x and x @ _ irrefutable patterns #137553.
• thirdly, the behavior is different between pattern-matching done through let and pattern-matching done through match – which is a superficial syntactic difference

This PR aims to address all of the above issues. The new behavior is as follows:

• like before, if a pattern contains a binding, the place it binds gets captured as required by the binding mode
• if matching against the pattern requires inspecting a disciminant, the place whose discriminant needs to be inspected gets captured by reference

"requires inspecting a discriminant" is also used here to mean "compare something with a constant" and other such decisions. For types other than ADTs, the details are not interesting and aren't changing.

The breaking change

During closure capture analysis, matching an enum against a constructor is considered to require inspecting a discriminant if the enum has more than one variant. Notably, this is the case even if all the other variants happen to be uninhabited. This is motivated by implementation difficulties involved in querying whether types are inhabited before we're done with type inference – without moving mountains to make it happen, you hit this assert:

rust/compiler/rustc_middle/src/ty/inhabitedness/mod.rs

Line 121 in 43f0014

debug_assert!(!self.has_infer());

Now, because the previous implementation did not concern itself with capturing the discriminants for irrefutable patterns at all, this is a breaking change – the following example, adapted from the testsuite, compiles on current stable, but will not compile with this PR:

#[derive(Clone, Copy, PartialEq, Eq, Debug)]
enum Void {}

pub fn main() {
    let mut r = Result::<Void, (u32, u32)>::Err((0, 0));
    let mut f = || {
        let Err((ref mut a, _)) = r;
        *a = 1;
    };
    let mut g = || {
    //~^ ERROR: cannot borrow `r` as mutable more than once at a time
        let Err((_, ref mut b)) = r;
        *b = 2;
    };
    f();
    g();
    assert_eq!(r, Err((1, 2)));
}

Is the breaking change necessary?

One other option would be to double down, and introduce a set of syntactic rules for determining whether a sub-pattern is in an irrefutable position, instead of querying the types and checking how many variants there are.

This would not eliminate the breaking change, but it would limit it to more contrived examples, such as

let ((true, Err((ref mut a, _, _))) | (false, Err((_, ref mut a, _)))) = x;

In this example, the Errs would not be considered in an irrefutable position, because they are part of an or-pattern. However, current stable would treat this just like a tuple (bool, (T, U, _)).

While introducing such a distinction would limit the impact, I would say that the added complexity would not be commensurate with the benefit it introduces.

The new insta-stable behavior

If a pattern in a match expression or similar has parts it will never read, this part will not be captured anymore:

fn main() {
    let mut a = (21, 37);
    // now only captures a.0, instead of the whole a
    let mut f = || {
        match a {
            (ref mut x, _) => *x = 42,
        }
    };
    a.1 = 69;
    f();
}

Note that this behavior was pretty much already present, but only accessible with this One Weird Trick™:

fn main() {
    let mut a = (21, 37);
    // both stable and this PR only capture a.0, because of the no-op @-pattern
    let mut f = || {
        match a {
            (ref mut x @ _, _) => *x = 42,
        }
    };
    a.1 = 69;
    f();
}

Implementation notes

The PR has two main commits:

• "ExprUseVisitor: properly report discriminant reads" makes walk_pat perform all necessary capturing. This is the part that fixes internal compiler error: two identical projections #137467.
• "ExprUseVisitor: remove maybe_read_scrutinee" removes the unnecessary "capture the entire scrutinee" behavior, fixing Closure captures are inconsistent between x and x @ _ irrefutable patterns #137553.

The new logic stops making the distinction between one particular example that used to work, and another ICE, tracked as #119786. As this requires an unstable feature, I am leaving this as future work.

[rustbot]

This PR changes a file inside tests/crashes. If a crash was fixed, please move into the corresponding ui subdir and add 'Fixes #' to the PR description to autoclose the issue upon merge.

[meithecatte]

Nadrieril suggested that this should be resolved through a breaking change – updated the PR description accordingly.

@rustbot label +needs-crater

r? @Nadrieril

[rustbot]

Error: Label needs-crater can only be set by Rust team members

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

[meithecatte]

@compiler-errors You've requested that the fix for #137553 land in a separate PR. However, ironically, the breaking changes are actually required by #137467 and not #137553. Do you think the removal of the now-obsolete maybe_read_scrutinee should happen in a separate PR, or should I do it here so that it also benefits from the crater run?

[compiler-errors]

We can crater both together if you think they're not worth separating. I was just trying to accelerate landing the parts that are obviously-not-breaking but it's up to you if you think that effort is worth it or if you're willing to be patient about waiting for the breaking parts (and FCP, etc).

@bors try

[bors]

⌛ Trying commit 8ed61e4 with merge 3b30da3...

[meithecatte]

We can crater both together if you think they're not worth separating. I was just trying to accelerate landing the parts that are obviously-not-breaking but it's up to you if you think that effort is worth it or if you're willing to be patient about waiting for the breaking parts (and FCP, etc).

That's the thing – one part is a breaking change, the other introduces insta-stable new behavior. There's no easily mergeable part to this.

[compiler-errors]

could we give this a less weird pr title pls 💀

@bors try

[bors]

⌛ Trying commit 7d5a892 with merge 630b4e8...

[meithecatte]

could we give this a less weird pr title pls 💀

Sure thing. I also updated the PR description to describe both changes. I want to add a section on what exactly the insta-stable behavior will be, but I realized that I haven't added a test for that. Should I hold off on pushing that to not break the bors try and crater?

[compiler-errors]

Once bors is done with the try build then you can push, no need to wait until crater is done.

[RalfJung]

This all makes sense to me at first sight, in particular the part about uninhabited variants in the last 2 comments. From a Miri perspective, we'll want to see a discriminant read at that point in the closure so that we can report UB as appropriate; this can't even work if the discriminant does not get captured in the first place. That said, I'd definitely like to hear from @Nadrieril before we land this. :)

Would it be possible to have a Miri test like this? I'm thinking of a repr(C) enum with 2 inhabited and an uninhabited variant, and then capturing &Enum that points to something invalid (i.e., the uninhabited variant), and then a let pattern like let (Var1 | Var2) = x; or so... IIUC, this so far would not have captured (since it's a let). Now with this PR, calling the closure should cause UB due to the invalid enum (but there should be no UB before since Miri does not enforce validity behind references).

[meithecatte]

Would it be possible to have a Miri test like this? I'm thinking of a repr(C) enum with 2 inhabited and an uninhabited variant, and then capturing &Enum that points to something invalid (i.e., the uninhabited variant), and then a let pattern like let (Var1 | Var2) = x; or so... IIUC, this so far would not have captured (since it's a let). Now with this PR, calling the closure should cause UB due to the invalid enum (but there should be no UB before since Miri does not enforce validity behind references).

This should pass, and I'll investigate creating a test like that. But do note that currently, the lowering actual MIR lowering code does check for inhabitedness, so a test with one inhabited variant, and one uninhabited one, would not pass, because the read of the discriminant would be skipped. To do this in an "honest" manner, we would want to also break compat outside of closures as well, and make the actual lowering perform a read in that case, bringing the semantics into a consistent state between the two.

[RalfJung]

Hm, I think we only skip the read in cases where it cannot fail since the place is by-val, or were there still some gaps in that? Though if let only does this for by-val cases we will not be able to do it in a closure either. I though there should be a test here that is UB with this PR but was accepted before. But maybe not.

[meithecatte]

Hm, I think we only skip the read in cases where it cannot fail since the place is by-val, or were there still some gaps in that? Though if let only does this for by-val cases we will not be able to do it in a closure either.

I think we're talking about a different check. See this part of the code, where if we're matching the only inhabited variant, we don't emit the TestCase that would read its discriminant into the decision tree.

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

Lines 269 to 282 in 7bfd952

	PatKind::Variant { adt_def, variant_index, args, ref subpatterns } => {
	let downcast_place = place_builder.downcast(adt_def, variant_index); // `(x as Variant)`
	cx.field_match_pairs(&mut subpairs, extra_data, downcast_place, subpatterns);
	let irrefutable = adt_def.variants().iter_enumerated().all(|(i, v)| {
	i == variant_index
	|| !v.inhabited_predicate(cx.tcx, adt_def).instantiate(cx.tcx, args).apply(
	cx.tcx,
	cx.infcx.typing_env(cx.param_env),
	cx.def_id.into(),
	)
	}) && !adt_def.variant_list_has_applicable_non_exhaustive();
	if irrefutable { None } else { Some(TestCase::Variant { adt_def, variant_index }) }
	}

[RalfJung]

That's deep in the guts of pattern elaboration, I cannot follow that code or relate it to the MIR we generate.^^ I am entirely talking about what it looks like in MIR. Though maybe I am entirely on the wrong train here. This PR does not change the MIR in the closure, does it? But it does mean more things get captured in some cases which should leave a trace in the MIR where the closure is constructed?

[meithecatte]

This PR does not change the MIR in the closure, does it?

Pretty much. It only changes what gets captured into the closure, and by extension the exact place expressions MIR later uses to access these upvars.

But it does mean more things get captured in some cases which should leave a trace in the MIR where the closure is constructed?

Yes, more things in some cases (#137467), less things in others (#137553).

Though if let only does this for by-val cases we will not be able to do it in a closure either.

To the best of my knowledge, this PR makes let and match equivalent in terms of closure capture behavior.

Hm, I think we only skip the read in cases where it cannot fail since the place is by-val, or were there still some gaps in that?

Okay, looks like I was not aware of some of the behavior here.

As it turns out, exhaustiveness only allows omitting the uninhabited branch when matching by value.

I tried this test case:

enum Void {}

enum Foo {
    A(u32),
    B(Void),
}

fn check_foo(x: &Foo) {
    match x {
        Foo::A(x) => {},
    }
}

and got this error, which I haven't seen before:

error[E0004]: non-exhaustive patterns: `&Foo::B(_)` not covered
 --> patlowering.rs:9:11
  |
9 |     match x {
  |           ^ pattern `&Foo::B(_)` not covered
  |
note: `Foo` defined here
 --> patlowering.rs:3:6
  |
3 | enum Foo {
  |      ^^^
4 |     A(u32),
5 |     B(Void),
  |     - not covered
  = note: the matched value is of type `&Foo`
  = note: `Void` is uninhabited but is not being matched by value, so a wildcard `_` is required
help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern or an explicit pattern as shown
  |
10~         Foo::A(x) => {},
11~         &Foo::B(_) => todo!(),
  |

This means that we only get to the code I linked when the uninhabitedness doesn't require reading through any references.

In this case, I will try to add a testcase like you described. Off the top of your head, are you aware of any similar tests that create enum values with invalid discriminants? I'm not sure how I'd go about doing that, I must admit unsafe code is not my strong suit.

[RalfJung]

To the best of my knowledge, this PR makes let and match equivalent in terms of closure capture behavior.

Right, but are they equivalent in terms of the MIR that is generated for the pattern?

In this case, I will try to add a testcase like you described. Off the top of your head, are you aware of any similar tests that create enum values with invalid discriminants? I'm not sure how I'd go about doing that, I must admit unsafe code is not my strong suit.

For this case I'd do something like:

#![feature(never_type)]

#[repr(C)]
enum E {
  V1, // discrminant: 0
  V2, // 1
  V3(!), // 2
}

fn main() {
    assert_eq!(std::mem::size_of::<E>(), 4);
    
    let val = 2u32;
    let ptr = (&raw const val).cast::<E>();
    // This is invalid:
    unsafe { ptr.read() };
}

[meithecatte]

BTW, playing around with this, I've realized that due to the very nature of the PR, it changes when some dereferences happen, and because of that, this passes Miri on nightly but not on my branch:

use std::ptr;

fn f(x: &&(u32, u32)) {
    || match x {
        (y, _) => {},
    };
}

fn main() {
    let r = unsafe {
        let x: (u32, u32) = (1, 2);
        &* &raw const x
    };

    f(&r);
}

I assume that this is not an actually important difference, because the UB was there either way, and just wasn't detected by Miri before.

[meithecatte]

(rebased onto latest master, now that #139086 got merged)

[RalfJung]

That's a nice testcase, please add that.

I assume that this is not an actually important difference, because the UB was there either way, and just wasn't detected by Miri before.

On which basis was there UB before?
The closure never gets executed, so it's not at all clear to me that this code should be UB. I guess it is due to the reference that is created when building the closure environment?

[meithecatte]

On which basis was there UB before?

My mental model is that merely touching a reference, in any way, such as moving it into a closure, must mean that the memory it points to is valid. Is that not an accurate mental model?

[RalfJung]

That's generally correct, but references likely are not required to be recursively valid (the docs just forbid this so that we have time to make up our minds). An &&(u32, u32) where the inner reference is dangling is hence okay to touch and move around in Miri, and we should not rely on it being UB in our reasoning.

[Nadrieril]

It is a bit surprising, though, given that we need to know that Void is uninhabited to allow those irrefutable pattern matches. Do we defer that check somehow to after type checking?

Yep, match exhaustiveness runs after type checking and is the one running the is_unihabited queries. Type checking itself doesn't know about inhabitedness; it's a property we only compute on fully-inferred types.

As it turns out, exhaustiveness only allows omitting the uninhabited branch when matching by value.

Yep, for more power, activate the exhaustive_patterns feature. I have on my TODO list to RFC something sensible in that direction (along with never patterns).

To the best of my knowledge, this PR makes let and match equivalent in terms of closure capture behavior.

Right, but are they equivalent in terms of the MIR that is generated for the pattern?

We call the same pattern-lowering code for both, so if the upvars are the same the MIR should be the same. I'm not totally confident tho.

Hm, I think we only skip the read in cases where it cannot fail since the place is by-val, or were there still some gaps in that?

There are gaps because MIR lowering does not emit discriminant reads for single-nonempty-variant-enums. That's the bit of code that @meithecatte linked to. See point below.

I'd definitely like to hear from @Nadrieril before we land this. :)

I think this boils down to: does matching on a single-nonempty-variant enums need a discriminant read? In both closure capture (what this PR is about) and borrow-checking (see example below), today's Rust seems to say "no".

#[derive(Clone, Copy, PartialEq, Eq, Debug)]
enum Void {}

pub fn main() {
    let mut r = Result::<Void, (u32, u32)>::Err((0, 0));

    // This is allowed by the borrow-checker. If `Result<Void, (bool, bool)>` had a discriminant and stored it in a niche
    // then reading the discriminant while one of the fields is mutably borrowed would be UB. Hence this being
    // accepted (plus the fact that we reserve the option to use niches) implies that this pattern-match does not
    // do a discriminant read.
    let Err((ref mut a, _)) = r;
    let Err((_, ref mut b)) = r;
    *a = 1;
    *b = 2;

    assert_eq!(r, Err((1, 2)));
}

I don't believe that fact was ever decided explicitly. See a relevant t-opsem discussion.

Of note is that truly-single-variant enums do skip the discriminant read:

enum Foo {
    Single(u32, u32),
}
pub fn main() {
    let mut r = Foo::Single(0, 0);
    let Foo::Single(ref mut a, _) = r;
    let Foo::Single(_, ref mut b) = r;
    *a = 1;
    *b = 2;
}

This is likely what motivated implementors of the exhaustive_patterns feature to do the same for single-nonempty-variant enums.

However, MIR is polymorphic: Option::is_none for unknown T has a discriminant read, so I'd want it to still be there if T is Void. That just seems to make everything easier. I am in favor of deciding that matching on an enum with !=1 variants causes a discriminant read.

As Ralf said, this implies the closure should capture the discriminant so we can read it, i.e. what this here PR does. The alternative would be an implicit discriminant read when we construct the closure, which goes against the desire for patterns to be explicit about accesses.

This also implies that the borrow-checking example above should error. I'm ok with this but we'd need a second crater run for that one.

[meithecatte]

Hm, I think we only skip the read in cases where it cannot fail since the place is by-val, or were there still some gaps in that?

I have performed some more testing, and this isn't actually the case. For example, this code passes miri on current master (as well as on top of #139042, FWIW):

#![feature(never_type)]

#[repr(C)]
#[allow(dead_code)]
enum E {
  V1, // discriminant: 0
  V2(!), // 1
}

fn main() {
    assert_eq!(std::mem::size_of::<E>(), 4);

    let val = 1u32;
    let ptr = (&raw const val).cast::<E>();
    let r = unsafe { &*ptr };
    match r { //~ we'd probably want ERROR: read discriminant of an uninhabited enum variant
        E::V1 => {}
        E::V2(_) => {}
    }
}

This is due to the code within match_pair.rs I linked above, which means that the match tree construction and MIR lowering considers the first arm to always match regardless of the discriminant, i.e.

    match r {
        _ => {}
        E::V2(_) => {} // dead arm, only false edges lead to it (for borrowck)
    }

This means that swapping the order of the arms means that the UB is again detected, even though the patterns aren't actually overlapping.

This relates to the same semantics question of "does matching against the only inhabited variant of an enum involve reading its discriminant". Looks like the answer we want to be going for is "yes". From the way you're talking about it, it kinda feels like there may have been prior discussion of this, specifically considering the by-value details you're mentioning? If so, could you link me to it?

Certainly, this PR means that the closure capture code will consider the answer to be "yes". Do we want to make sure that the match lowering code also does that? If so, should that be part of the same PR & FCP, or a separate one? Seems to me like it'd be useful to get consensus on both parts of the issue at the same time, and perhaps double-check the crater, but OTOH not sure if it makes sense as part of the same PR.

[Nadrieril]

Great minds think alike, we reached basically the same conclusion at the same time :D

[meithecatte]

In both closure capture (what this PR is about) and borrow-checking (see example below), today's Rust seems to say "no".

Not exactly. matching within a closure will consider the discriminant read to be happening even today, it is only let destructuring that is weird due to its current ICE-prone implementation.

[Nadrieril]

Not exactly. matching within a closure will consider the discriminant read to be happening even today, it is only let destructuring that is weird due to its current ICE-prone implementation.

Yeah I agree impl-wise; I meant that in terms of code accepted, the let behavior can only be sound if there isn't a discriminant read. Which suggests that whoever implemented the exhaustive_patterns feature intended for that to be true (and I accidentally inherited+stabilized that decision when stabilizing min_exhaustive_patterns).

[traviscross]

Do we want to make sure that the match lowering code also does that? If so, should that be part of the same PR & FCP, or a separate one? Seems to me like it'd be useful to get consensus on both parts of the issue at the same time...

We have a meeting coming up today where we might get to this, so if you're able to write it up for us with respect to the various implications here, we can consider it together.

[meithecatte]

We have a meeting coming up today where we might get to this, so if you're able to write it up for us with respect to the various implications here, we can consider it together.

Sure thing. Let me know if you'd like me to also join the meeting to clarify things if necessary – I do have a lot of the context paged in already.

Pretty sure the main points were all mentioned already, but to summarize, there are three related language changes with various degrees of "necessary":

1. The first part of this PR, which fixes internal compiler error: two identical projections #137467. See the "breaking change" section of the PR description, this comment: Make closure capturing have consistent and correct behaviour around patterns #138961 (comment) and my response: Make closure capturing have consistent and correct behaviour around patterns #138961 (comment)
   • Since this fixes a pretty important ICE, I'm assuming this is desirable no matter what.
   • Crater picked up no instances of this actually breaking anything.
2. Then, we have the second part of this PR, which fixes Closure captures are inconsistent between x and x @ _ irrefutable patterns #137553. This means that the semantics for let $pat = $expr and match $expr { $pat => ... } will become the same w.r.t. closure captures, and that match will go from "most real-life patterns capture the entire scrutinee" to "most real-life patterns will capture only the relevant part". This is described in the "insta-stable behavior" section of the PR description, but as it turns out, can also be a breaking change as discovered by crater and described in Make closure capturing have consistent and correct behaviour around patterns #138961 (comment)
   • Crater picked up one instance of this actually breaking someone's project, a PR fixing this has already been merged.
   • Not doing this change would leave some technical debt in the closure capture analysis, warts in the semantics and the weird behavior where x and x @ _ do different things.
3. Now, the third change, which hasn't been implemented yet but should be straight-forward, is when you go from asking "do we need to capture the discriminant when in a closure", to the more general "is the discriminant being read". For consistency, we might want to make sure that matching against an enum { A, B(!) } requires reading its discriminant. As far as I know, this also goes in the direction of the interpretation that feature(never_patterns) is going for. Changing this affects borrowck, as in Make closure capturing have consistent and correct behaviour around patterns #138961 (comment), as well as the presence of UB, as in Make closure capturing have consistent and correct behaviour around patterns #138961 (comment).
   • This is asking questions about things that probably barely anybody would write, so I feel safe conjecturing that Crater will likely not pick up many instances of this being a breaking change.
   • In my opinion, making this change makes perfect sense, and will be necessitated by other attempts to clean up the semantics, sooner or later.
   • However, to me it does feel like much more of an edgecase than the other two things.

Once we agree on whether it's a desired change, I could also use some feedback on whether this should all be one PR.

[RalfJung]

I am in favor of deciding that matching on an enum with !=1 variants causes a discriminant read.

Yeah, agreed.

(I wonder if we should just always do this even for enums that have exactly one variant, but don't have a set opinion on that case currently.)

[meithecatte]

(I wonder if we should just always do this even for enums that have exactly one variant, but don't have a set opinion on that case currently.)

That would create a significant difference in the borrow-checking behavior of patterns matching against enum E { V(...) } vs struct V(...). Currently a single-variant enum is, AFAIK, equivalent to a struct, modulo x.field_name syntax.

[RalfJung]

Certainly, this PR means that the closure capture code will consider the answer to be "yes". Do we want to make sure that the match lowering code also does that?

Is it necessary for closure capturing to reimplement the same logic here? That seems like a hazard. Or am I misunderstanding something?

[meithecatte]

Is it necessary for closure capturing to reimplement the same logic here? That seems like a hazard. Or am I misunderstanding something?

Yes, I'm not particularly happy with the way this logic is duplicated between upvar inference and MIR lowering. Not only are they two separate implementations, they work on different IRs: upvar inference does its work on HIR, while MIR lowering and exhaustiveness checking work on THIR.

Unfortunately I don't see a good way of unifying this, because upvar inference is part of type inference, and THIR requires typechecking to be finished.

The best way I see is having a comment going "be careful! these two things need to have matching behavior!"...