Skip to content

Closure captures are inconsistent between x and x @ _ irrefutable patterns #137553

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
meithecatte opened this issue Feb 24, 2025 · 2 comments · May be fixed by #138961
Open

Closure captures are inconsistent between x and x @ _ irrefutable patterns #137553

meithecatte opened this issue Feb 24, 2025 · 2 comments · May be fixed by #138961
Assignees
Labels
A-closures Area: Closures (`|…| { … }`) C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@meithecatte
Copy link
Contributor

meithecatte commented Feb 24, 2025

Background: apart from paths, another way to make use of the precise capture of variables by closures is with irrefutable patterns:

fn main() {
    let mut a = (21, 37);
    // only captures a.0, example compiles fine
    let mut f = || {
        let (ref mut x, _) = a;
        *x = 42;
    };
    a.1 = 69;
    f();
}

One would expect this to be equivalent to an if let or match that happens to be irrefutable. However, this isn't the case:

fn main() {
    let mut a = (21, 37);
    // captures the entirety of a, example doesn't pass borrow check
    let mut f = || {
        match a {
            (ref mut x, _) => *x = 42,
        }
    };
    a.1 = 69;
    f();
}

However, one can get the code to compile again by introducing a no-op @-pattern:

fn main() {
    let mut a = (21, 37);
    // captures only a.0 again, and against all odds, the example compiles
    let mut f = || {
        match a {
            (ref mut x @ _, _) => *x = 42,
        }
    };
    a.1 = 69;
    f();
}

This suggests that the inconsistency is definitely a bug.

I have discovered this while investigating #137467, and I am working on resolving both issues. I am creating this issue to make it easy to keep track of the bug.

@rustbot claim

Meta

rustc --version --verbose:

rustc 1.87.0-nightly (f280acf4c 2025-02-19)
binary: rustc
commit-hash: f280acf4c743806abbbbcfe65050ac52ec4bdec0
commit-date: 2025-02-19
host: x86_64-unknown-linux-gnu
release: 1.87.0-nightly
LLVM version: 20.1.0

(git blame suggests that this behavior got introduced 3 years ago or so)

@meithecatte meithecatte added the C-bug Category: This is a bug. label Feb 24, 2025
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Feb 24, 2025
@compiler-errors
Copy link
Member

This almost certainly has to do with the difference between fake read vs. borrow we introduce in maybe_read_scrutinee when there is a subpat on PatKind::Binding:

if opt_sub_pat.is_none() {

if needs_to_be_read {
self.borrow_expr(discr, BorrowKind::Immutable)?;
} else {
let closure_def_id = match discr_place.place.base {
PlaceBase::Upvar(upvar_id) => Some(upvar_id.closure_expr_id),
_ => None,
};
self.delegate.borrow_mut().fake_read(
&discr_place,
FakeReadCause::ForMatchedPlace(closure_def_id),
discr_place.hir_id,
);
// We always want to walk the discriminant. We want to make sure, for instance,
// that the discriminant has been initialized.
self.walk_expr(discr)?;
}

Funny enough I was just looking at this code bc it's very subtle and confusing lol -- @meithecatte pls reach out on zulip if you want help investigating or fixing this issue.

@compiler-errors
Copy link
Member

I'd also recommend that if this is separable from #137467, then ideally we fix it in a different PR, just because reasoning about (+ possibly running analysis for) the fallout of any changes to closure capture analysis should happen in parallel if possible.

@lolbinarycat lolbinarycat added A-closures Area: Closures (`|…| { … }`) T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Feb 24, 2025
@jieyouxu jieyouxu added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Feb 26, 2025
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 26, 2025
ExprUseVisitor: properly report discriminant reads

This PR fixes rust-lang#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!

## Background

The current upvar inference code handles patterns in two parts:
- `ExprUseVisitor::walk_pat` finds the *bindings* being done by the pattern and captures the relevant parts
- `ExprUseVisitor::maybe_read_scrutinee` determines whether matching against the pattern will at any point require inspecting a discriminant, and if so, captures *the entire scrutinee*. It also has some weird logic around bindings, deciding to also capture the entire scrutinee if *pretty much any binding exists in the pattern*, with some weird behavior like rust-lang#137553.

Nevertheless, something like `|| let (a, _) = x;` will only capture `x.0`, because `maybe_read_scrutinee` does not run for irrefutable patterns at all. This causes issues like rust-lang#137467, where the closure wouldn't be capturing enough, because an irrefutable or-pattern can still require inspecting a discriminant, and the match lowering would then panic, because it couldn't find an appropriate upvar in the closure.

My thesis is that this is not a reasonable implementation. To that end, I intend to merge the functionality of both these parts into `walk_pat`, which will bring upvar inference closer to what the MIR lowering actually needs – both in making sure that necessary variables get captured, fixing rust-lang#137467, and in reducing the cases where redundant variables do – fixing rust-lang#137553.

This PR introduces the necessary logic into `walk_pat`, fixing rust-lang#137467. A subsequent PR will remove `maybe_read_scrutinee` entirely, which should now be redundant, fixing rust-lang#137553. The latter is still pending, as my current revision doesn't handle opaque types correctly for some reason I haven't looked into yet.

## The breaking change

The following example, adapted from the testsuite, compiles on current stable, but will not compile with this PR:

```rust
#[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)));
}
```

The issue is that, to determine that matching against `Err` here doesn't require inspecting the discriminant, we need to query the `InhabitedPredicate` of the types involved. However, as upvar inference is done during typechecking, the relevant type might not yet be fully inferred. Because of this, performing such a check hits this assertion:

https://github.com./rust-lang/rust/blob/43f0014ef0f242418674f49052ed39b70f73bc1c/compiler/rustc_middle/src/ty/inhabitedness/mod.rs#L121

The code used to compile fine, but only because the compiler incorrectly assumed that patterns used within a `let` cannot possibly be inspecting any discriminants.

## Is the breaking change necessary?

One other option would be to double down, and introduce a deliberate semantics difference between `let $pat = $expr;` and `match $expr { $pat => ... }`, that syntactically determines whether the pattern is in an irrefutable position, instead of querying the types.

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

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

The cost here, would be the complexity added with very little benefit.

## Other notes

- I performed various cleanups while working on this. The last commit of the PR is the interesting one.
- Due to the temporary duplication of logic between `maybe_read_scrutinee` and `walk_pat`, some of the `#[rustc_capture_analysis]` tests report duplicate messages before deduplication. This is harmless.
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 26, 2025
ExprUseVisitor: murder maybe_read_scrutinee in cold blood

This PR fixes rust-lang#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!

## Background

The current upvar inference code handles patterns in two parts:
- `ExprUseVisitor::walk_pat` finds the *bindings* being done by the pattern and captures the relevant parts
- `ExprUseVisitor::maybe_read_scrutinee` determines whether matching against the pattern will at any point require inspecting a discriminant, and if so, captures *the entire scrutinee*. It also has some weird logic around bindings, deciding to also capture the entire scrutinee if *pretty much any binding exists in the pattern*, with some weird behavior like rust-lang#137553.

Nevertheless, something like `|| let (a, _) = x;` will only capture `x.0`, because `maybe_read_scrutinee` does not run for irrefutable patterns at all. This causes issues like rust-lang#137467, where the closure wouldn't be capturing enough, because an irrefutable or-pattern can still require inspecting a discriminant, and the match lowering would then panic, because it couldn't find an appropriate upvar in the closure.

My thesis is that this is not a reasonable implementation. To that end, I intend to merge the functionality of both these parts into `walk_pat`, which will bring upvar inference closer to what the MIR lowering actually needs – both in making sure that necessary variables get captured, fixing rust-lang#137467, and in reducing the cases where redundant variables do – fixing rust-lang#137553.

This PR introduces the necessary logic into `walk_pat`, fixing rust-lang#137467. A subsequent PR will remove `maybe_read_scrutinee` entirely, which should now be redundant, fixing rust-lang#137553. The latter is still pending, as my current revision doesn't handle opaque types correctly for some reason I haven't looked into yet.

## The breaking change

The following example, adapted from the testsuite, compiles on current stable, but will not compile with this PR:

```rust
#[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)));
}
```

The issue is that, to determine that matching against `Err` here doesn't require inspecting the discriminant, we need to query the `InhabitedPredicate` of the types involved. However, as upvar inference is done during typechecking, the relevant type might not yet be fully inferred. Because of this, performing such a check hits this assertion:

https://github.com./rust-lang/rust/blob/43f0014ef0f242418674f49052ed39b70f73bc1c/compiler/rustc_middle/src/ty/inhabitedness/mod.rs#L121

The code used to compile fine, but only because the compiler incorrectly assumed that patterns used within a `let` cannot possibly be inspecting any discriminants.

## Is the breaking change necessary?

One other option would be to double down, and introduce a deliberate semantics difference between `let $pat = $expr;` and `match $expr { $pat => ... }`, that syntactically determines whether the pattern is in an irrefutable position, instead of querying the types.

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

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

The cost here, would be the complexity added with very little benefit.

## Other notes

- I performed various cleanups while working on this. The last commit of the PR is the interesting one.
- Due to the temporary duplication of logic between `maybe_read_scrutinee` and `walk_pat`, some of the `#[rustc_capture_analysis]` tests report duplicate messages before deduplication. This is harmless.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-closures Area: Closures (`|…| { … }`) C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants