Auto merge of rust-lang#138961 - meithecatte:expr-use-visitor, r=<try>

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.

Author: bors

compiler/rustc_hir_typeck/src/expr_use_visitor.rs

@@ -1447,6 +1447,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
     pub(crate) fn try_structurally_resolve_type(&self, sp: Span, ty: Ty<'tcx>) -> Ty<'tcx> {
         let ty = self.resolve_vars_with_obligations(ty);
 
+        debug!("next_solver = {:?}", self.next_trait_solver());
         if self.next_trait_solver()
             && let ty::Alias(..) = ty.kind()
         {

compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs

@@ -18,12 +18,12 @@
 //! from there).
 //!
 //! The fact that we are inferring borrow kinds as we go results in a
-//! semi-hacky interaction with mem-categorization. In particular,
-//! mem-categorization will query the current borrow kind as it
-//! categorizes, and we'll return the *current* value, but this may get
+//! semi-hacky interaction with the way `ExprUseVisitor` is computing
+//! `Place`s. In particular, it will query the current borrow kind as it
+//! goes, and we'll return the *current* value, but this may get
 //! adjusted later. Therefore, in this module, we generally ignore the
-//! borrow kind (and derived mutabilities) that are returned from
-//! mem-categorization, since they may be inaccurate. (Another option
+//! borrow kind (and derived mutabilities) that `ExprUseVisitor` returns
+//! within `Place`s, since they may be inaccurate. (Another option
 //! would be to use a unification scheme, where instead of returning a
 //! concrete borrow kind like `ty::ImmBorrow`, we return a
 //! `ty::InferBorrow(upvar_id)` or something like that, but this would

compiler/rustc_hir_typeck/src/upvar.rs

@@ -53,7 +53,10 @@ pub struct Projection<'tcx> {
     pub kind: ProjectionKind,
 }
 
-/// A `Place` represents how a value is located in memory.
+/// A `Place` represents how a value is located in memory. This does not
+/// always correspond to a syntactic place expression. For example, when
+/// processing a pattern, a `Place` can be used to refer to the sub-value
+/// currently being inspected.
 ///
 /// This is an HIR version of [`rustc_middle::mir::Place`].
 #[derive(Clone, Debug, PartialEq, Eq, Hash, TyEncodable, TyDecodable, HashStable)]
@@ -67,7 +70,10 @@ pub struct Place<'tcx> {
     pub projections: Vec<Projection<'tcx>>,
 }
 
-/// A `PlaceWithHirId` represents how a value is located in memory.
+/// A `PlaceWithHirId` represents how a value is located in memory. This does not
+/// always correspond to a syntactic place expression. For example, when
+/// processing a pattern, a `Place` can be used to refer to the sub-value
+/// currently being inspected.
 ///
 /// This is an HIR version of [`rustc_middle::mir::Place`].
 #[derive(Clone, Debug, PartialEq, Eq, Hash, TyEncodable, TyDecodable, HashStable)]

compiler/rustc_middle/src/hir/place.rs

@@ -0,0 +1,15 @@
+//@ known-bug: #119786
+//@ edition:2021
+
+fn enum_upvar() {
+    type T = impl Copy;
+    let foo: T = Some((1u32, 2u32));
+    let x = move || {
+        match foo {
+            None => (),
+            Some(_) => (),
+        }
+    };
+}
+
+pub fn main() {}

tests/crashes/119786.rs renamed to tests/crashes/119786-1.rs

@@ -0,0 +1,15 @@
+//@ known-bug: #119786
+//@ edition:2021
+
+fn enum_upvar() {
+    type T = impl Copy;
+    let foo: T = Some((1u32, 2u32));
+    let x = move || {
+        match foo {
+            None => (),
+            Some((a, b)) => (),
+        }
+    };
+}
+
+pub fn main() {}

tests/crashes/119786-2.rs

@@ -22,13 +22,15 @@ fn multi_variant_enum() {
     //~| Min Capture analysis includes:
         if let Info::Point(_, _, str) = point {
             //~^ NOTE: Capturing point[] -> Immutable
+            //~| NOTE: Capturing point[] -> Immutable
             //~| NOTE: Capturing point[(2, 0)] -> ByValue
             //~| NOTE: Min Capture point[] -> ByValue
             println!("{}", str);
         }
 
         if let Info::Meta(_, v) = meta {
             //~^ NOTE: Capturing meta[] -> Immutable
+            //~| NOTE: Capturing meta[] -> Immutable
             //~| NOTE: Capturing meta[(1, 1)] -> ByValue
             //~| NOTE: Min Capture meta[] -> ByValue
             println!("{:?}", v);

tests/crashes/119786-3.rs

@@ -9,7 +9,7 @@ LL |     let c = #[rustc_capture_analysis]
    = note: this compiler was built on YYYY-MM-DD; consider upgrading it if it is out of date
 
 error[E0658]: attributes on expressions are experimental
-  --> $DIR/capture-enums.rs:48:13
+  --> $DIR/capture-enums.rs:50:13
    |
 LL |     let c = #[rustc_capture_analysis]
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -34,18 +34,28 @@ note: Capturing point[] -> Immutable
    |
 LL |         if let Info::Point(_, _, str) = point {
    |                                         ^^^^^
+note: Capturing point[] -> Immutable
+  --> $DIR/capture-enums.rs:23:41
+   |
+LL |         if let Info::Point(_, _, str) = point {
+   |                                         ^^^^^
 note: Capturing point[(2, 0)] -> ByValue
   --> $DIR/capture-enums.rs:23:41
    |
 LL |         if let Info::Point(_, _, str) = point {
    |                                         ^^^^^
 note: Capturing meta[] -> Immutable
-  --> $DIR/capture-enums.rs:30:35
+  --> $DIR/capture-enums.rs:31:35
+   |
+LL |         if let Info::Meta(_, v) = meta {
+   |                                   ^^^^
+note: Capturing meta[] -> Immutable
+  --> $DIR/capture-enums.rs:31:35
    |
 LL |         if let Info::Meta(_, v) = meta {
    |                                   ^^^^
 note: Capturing meta[(1, 1)] -> ByValue
-  --> $DIR/capture-enums.rs:30:35
+  --> $DIR/capture-enums.rs:31:35
    |
 LL |         if let Info::Meta(_, v) = meta {
    |                                   ^^^^
@@ -67,13 +77,13 @@ note: Min Capture point[] -> ByValue
 LL |         if let Info::Point(_, _, str) = point {
    |                                         ^^^^^
 note: Min Capture meta[] -> ByValue
-  --> $DIR/capture-enums.rs:30:35
+  --> $DIR/capture-enums.rs:31:35
    |
 LL |         if let Info::Meta(_, v) = meta {
    |                                   ^^^^
 
 error: First Pass analysis includes:
-  --> $DIR/capture-enums.rs:52:5
+  --> $DIR/capture-enums.rs:54:5
    |
 LL | /     || {
 LL | |
@@ -85,13 +95,13 @@ LL | |     };
    | |_____^
    |
 note: Capturing point[(2, 0)] -> ByValue
-  --> $DIR/capture-enums.rs:55:47
+  --> $DIR/capture-enums.rs:57:47
    |
 LL |         let SingleVariant::Point(_, _, str) = point;
    |                                               ^^^^^
 
 error: Min Capture analysis includes:
-  --> $DIR/capture-enums.rs:52:5
+  --> $DIR/capture-enums.rs:54:5
    |
 LL | /     || {
 LL | |
@@ -103,7 +113,7 @@ LL | |     };
    | |_____^
    |
 note: Min Capture point[(2, 0)] -> ByValue
-  --> $DIR/capture-enums.rs:55:47
+  --> $DIR/capture-enums.rs:57:47
    |
 LL |         let SingleVariant::Point(_, _, str) = point;
    |                                               ^^^^^

tests/crashes/137467-1.rs

@@ -4,7 +4,7 @@ error[E0505]: cannot move out of `ts` because it is borrowed
 LL |     let _b = || { match ts {
    |              --         -- borrow occurs due to use in closure
    |              |
-   |              borrow of `ts` occurs here
+   |              borrow of `ts.x` occurs here
 ...
 LL |     let mut mut_ts = ts;
    |                      ^^ move out of `ts` occurs here

tests/crashes/137467-2.rs

@@ -64,9 +64,8 @@ fn test_6_should_capture_single_variant() {
     //~^ First Pass analysis includes:
     //~| Min Capture analysis includes:
         match variant {
-            //~^ NOTE: Capturing variant[] -> Immutable
-            //~| NOTE: Capturing variant[(0, 0)] -> Immutable
-            //~| NOTE: Min Capture variant[] -> Immutable
+            //~^ NOTE: Capturing variant[(0, 0)] -> Immutable
+            //~| NOTE: Min Capture variant[(0, 0)] -> Immutable
             SingleVariant::Points(a) => {
                 println!("{:?}", a);
             }
@@ -149,8 +148,8 @@ fn test_7_should_capture_slice_len() {
     //~^ First Pass analysis includes:
     //~| Min Capture analysis includes:
         match slice {
-            //~^ NOTE: Capturing slice[] -> Immutable
-            //~| NOTE: Min Capture slice[] -> Immutable
+            //~^ NOTE: Capturing slice[Deref] -> Immutable
+            //~| NOTE: Min Capture slice[Deref] -> Immutable
             [_,_,_] => {},
             _ => {}
         }
@@ -161,8 +160,8 @@ fn test_7_should_capture_slice_len() {
     //~^ First Pass analysis includes:
     //~| Min Capture analysis includes:
         match slice {
-            //~^ NOTE: Capturing slice[] -> Immutable
-            //~| NOTE: Min Capture slice[] -> Immutable
+            //~^ NOTE: Capturing slice[Deref] -> Immutable
+            //~| NOTE: Min Capture slice[Deref] -> Immutable
             [] => {},
             _ => {}
         }
@@ -173,8 +172,8 @@ fn test_7_should_capture_slice_len() {
     //~^ First Pass analysis includes:
     //~| Min Capture analysis includes:
         match slice {
-            //~^ NOTE: Capturing slice[] -> Immutable
-            //~| NOTE: Min Capture slice[] -> Immutable
+            //~^ NOTE: Capturing slice[Deref] -> Immutable
+            //~| NOTE: Min Capture slice[Deref] -> Immutable
             [_, .. ,_] => {},
             _ => {}
         }