Rollup merge of rust-lang#138961 - meithecatte:expr-use-visitor, r=Nadrieril,traviscross

Make closure capturing have consistent and correct behaviour around patterns

Reference PR:

- rust-lang/reference#1837

This PR has two goals:
- firstly, it 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!
    - this also fixes rust-lang#138973, a slightly different case with the same root cause.
    - likewise, fixes rust-lang#140011.
- secondly, it fixes rust-lang#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:

```rust
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](https://doc.rust-lang.org/reference/types/closure.html#r-type.closure.capture.precision.wildcard).

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 rust-lang#137467 ICE.
- secondly, the presence of an ``@`-pattern` doesn't really have any semantics by itself. This is the weird behavior tracked as rust-lang#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: https://github.com/rust-lang/rust/blob/43f0014ef0f242418674f49052ed39b70f73bc1c/compiler/rustc_middle/src/ty/inhabitedness/mod.rs#L121

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:

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

## 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

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

In this example, the `Err`s 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:

```rust
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™:

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

## The second, more practically-relevant breaking change

After running crater, we have discovered that the aforementioned insta-stable behavior, where sometimes closures will now capture less, can also manifest as a breaking change. This is because it is possible that previously a closure would capture an entire struct by-move, and now it'll start capturing only part of it – some by move, and some by reference. This then causes the closure to have a more restrictive lifetime than it did previously.

See:
- rust-lang#138961 (comment)
- EC-labs/cec-assignment#1
- tryandromeda/andromeda#43

## 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 rust-lang#137467.
- "ExprUseVisitor: remove maybe_read_scrutinee" removes the unnecessary "capture the entire scrutinee" behavior, fixing rust-lang#137553.

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

Author: matthiaskrgr

compiler/rustc_hir_typeck/src/expr_use_visitor.rs

@@ -761,6 +761,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
     ///       ],
     /// }
     /// ```
+    #[instrument(level = "debug", skip(self))]
     fn compute_min_captures(
         &self,
         closure_def_id: LocalDefId,
@@ -2029,6 +2030,7 @@ struct InferBorrowKind<'tcx> {
 }
 
 impl<'tcx> euv::Delegate<'tcx> for InferBorrowKind<'tcx> {
+    #[instrument(skip(self), level = "debug")]
     fn fake_read(
         &mut self,
         place_with_id: &PlaceWithHirId<'tcx>,
@@ -2119,6 +2121,7 @@ impl<'tcx> euv::Delegate<'tcx> for InferBorrowKind<'tcx> {
 }
 
 /// Rust doesn't permit moving fields out of a type that implements drop
+#[instrument(skip(fcx), ret, level = "debug")]
 fn restrict_precision_for_drop_types<'a, 'tcx>(
     fcx: &'a FnCtxt<'a, 'tcx>,
     mut place: Place<'tcx>,
@@ -2179,6 +2182,7 @@ fn restrict_precision_for_unsafe(
 /// - No unsafe block is required to capture `place`.
 ///
 /// Returns the truncated place and updated capture mode.
+#[instrument(ret, level = "debug")]
 fn restrict_capture_precision(
     place: Place<'_>,
     curr_mode: ty::UpvarCapture,
@@ -2208,6 +2212,7 @@ fn restrict_capture_precision(
 }
 
 /// Truncate deref of any reference.
+#[instrument(ret, level = "debug")]
 fn adjust_for_move_closure(
     mut place: Place<'_>,
     mut kind: ty::UpvarCapture,
@@ -2222,6 +2227,7 @@ fn adjust_for_move_closure(
 }
 
 /// Truncate deref of any reference.
+#[instrument(ret, level = "debug")]
 fn adjust_for_use_closure(
     mut place: Place<'_>,
     mut kind: ty::UpvarCapture,
@@ -2237,6 +2243,7 @@ fn adjust_for_use_closure(
 
 /// Adjust closure capture just that if taking ownership of data, only move data
 /// from enclosing stack frame.
+#[instrument(ret, level = "debug")]
 fn adjust_for_non_move_closure(
     mut place: Place<'_>,
     mut kind: ty::UpvarCapture,
@@ -2559,6 +2566,7 @@ fn determine_place_ancestry_relation<'tcx>(
 ///     // it is constrained to `'a`
 /// }
 /// ```
+#[instrument(ret, level = "debug")]
 fn truncate_capture_for_optimization(
     mut place: Place<'_>,
     mut curr_mode: ty::UpvarCapture,

compiler/rustc_hir_typeck/src/upvar.rs

@@ -339,6 +339,12 @@ impl<'tcx> MatchPairTree<'tcx> {
 
         if let Some(test_case) = test_case {
             // This pattern is refutable, so push a new match-pair node.
+            //
+            // Note: unless test_case is TestCase::Or, place must not be None.
+            // This means that the closure capture analysis in
+            // rustc_hir_typeck::upvar, and in particular the pattern handling
+            // code of ExprUseVisitor, must capture all of the places we'll use.
+            // Make sure to keep these two parts in sync!
             match_pairs.push(MatchPairTree {
                 place,
                 test_case,

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

@@ -888,7 +888,7 @@ declare_clippy_lint! {
     /// ```
     #[clippy::version = "pre 1.29.0"]
     pub SEARCH_IS_SOME,
-    complexity,
+    nursery,
     "using an iterator or string search followed by `is_some()` or `is_none()`, which is more succinctly expressed as a call to `any()` or `contains()` (with negation in case of `is_none()`)"
 }
 

src/tools/clippy/clippy_lints/src/methods/mod.rs

@@ -1,3 +1,4 @@
+#![warn(clippy::search_is_some)]
 pub struct Thing;
 //@no-rustfix
 pub fn has_thing(things: &[Thing]) -> bool {

src/tools/clippy/tests/ui/crashes/ice-9041.rs

@@ -1,5 +1,5 @@
 error: called `is_some()` after searching an `Iterator` with `find`
-  --> tests/ui/crashes/ice-9041.rs:5:19
+  --> tests/ui/crashes/ice-9041.rs:6:19
    |
 LL |     things.iter().find(|p| is_thing_ready(p)).is_some()
    |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `any(|p| is_thing_ready(&p))`

src/tools/clippy/tests/ui/crashes/ice-9041.stderr

@@ -311,19 +311,23 @@ mod issue9120 {
     }
 }
 
+// skip this test due to rust-lang/rust-clippy#16086
+/*
 #[allow(clippy::match_like_matches_macro)]
 fn issue15102() {
     let values = [None, Some(3)];
-    let has_even = values.iter().any(|v| matches!(&v, Some(x) if x % 2 == 0));
-    //~^ search_is_some
+    let has_even = values.iter().find(|v| matches!(v, Some(x) if x % 2 == 0)).is_some();
+    ~^ search_is_some
     println!("{has_even}");
 
     let has_even = values
         .iter()
-        .any(|v| match &v {
-            //~^ search_is_some
+        .find(|v| match v {
+            ~^ search_is_some
             Some(x) if x % 2 == 0 => true,
             _ => false,
-        });
+        })
+        .is_some();
     println!("{has_even}");
 }
+*/

src/tools/clippy/tests/ui/search_is_some_fixable_some.fixed

@@ -322,20 +322,23 @@ mod issue9120 {
     }
 }
 
+// skip this test due to rust-lang/rust-clippy#16086
+/*
 #[allow(clippy::match_like_matches_macro)]
 fn issue15102() {
     let values = [None, Some(3)];
     let has_even = values.iter().find(|v| matches!(v, Some(x) if x % 2 == 0)).is_some();
-    //~^ search_is_some
+    ~^ search_is_some
     println!("{has_even}");
 
     let has_even = values
         .iter()
         .find(|v| match v {
-            //~^ search_is_some
+            ~^ search_is_some
             Some(x) if x % 2 == 0 => true,
             _ => false,
         })
         .is_some();
     println!("{has_even}");
 }
+*/

src/tools/clippy/tests/ui/search_is_some_fixable_some.rs

@@ -346,32 +346,5 @@ error: called `is_some()` after searching an `Iterator` with `find`
 LL |         let _ = v.iter().find(|x: &&u32| (*arg_no_deref_dyn)(x)).is_some();
    |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `any(|x: &u32| (*arg_no_deref_dyn)(&x))`
 
-error: called `is_some()` after searching an `Iterator` with `find`
-  --> tests/ui/search_is_some_fixable_some.rs:328:34
-   |
-LL |     let has_even = values.iter().find(|v| matches!(v, Some(x) if x % 2 == 0)).is_some();
-   |                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `any(|v| matches!(&v, Some(x) if x % 2 == 0))`
-
-error: called `is_some()` after searching an `Iterator` with `find`
-  --> tests/ui/search_is_some_fixable_some.rs:334:10
-   |
-LL |           .find(|v| match v {
-   |  __________^
-LL | |
-LL | |             Some(x) if x % 2 == 0 => true,
-LL | |             _ => false,
-LL | |         })
-LL | |         .is_some();
-   | |__________________^
-   |
-help: consider using
-   |
-LL ~         .any(|v| match &v {
-LL +
-LL +             Some(x) if x % 2 == 0 => true,
-LL +             _ => false,
-LL ~         });
-   |
-
-error: aborting due to 51 previous errors
+error: aborting due to 49 previous errors
 

src/tools/clippy/tests/ui/search_is_some_fixable_some.stderr

@@ -0,0 +1,20 @@
+// This test serves to document the change in semantics introduced by
+// rust-lang/rust#138961.
+//
+// A corollary of partial-pattern.rs: while the tuple access testcase makes
+// it clear why these semantics are useful, it is actually the dereference
+// being performed by the pattern that matters.
+
+fn main() {
+    // the inner reference is dangling
+    let x: &&u32 = unsafe {
+        let x: u32 = 42;
+        &&* &raw const x
+    };
+
+    let _ = || { //~ ERROR: encountered a dangling reference
+        match x {
+            &&_y => {},
+        }
+    };
+}