You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Auto merge of #138961 - meithecatte:expr-use-visitor, r=<try>
ExprUseVisitor: properly report discriminant reads
This PR fixes#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 #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 #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 #137467, and in reducing the cases where redundant variables do – fixing #137553.
This PR introduces the necessary logic into `walk_pat`, fixing #137467. A subsequent PR will remove `maybe_read_scrutinee` entirely, which should now be redundant, fixing #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.
0 commit comments