Rollup merge of #81912 - sexxi-goose:Migrations2_review, r=nikomatsakis

Implement the precise analysis pass for lint `disjoint_capture_drop_reorder`

The precision pass for the lint prevents the lint from triggering for a variable (that was previously entirely captured by the closure) if all paths that need Drop starting at root variable have been captured by the closure.

r? `@nikomatsakis`

4 files changed, 502 insertions, 13 deletions

diff --git a/compiler/rustc_typeck/src/check/upvar.rs b/compiler/rustc_typeck/src/check/upvar.rs
index c666cd774c4..d097f863ad2 100644
--- a/compiler/rustc_typeck/src/check/upvar.rs
+++ b/compiler/rustc_typeck/src/check/upvar.rs
@@ -40,13 +40,16 @@ use rustc_hir::def_id::DefId;
 use rustc_hir::def_id::LocalDefId;
 use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor};
 use rustc_infer::infer::UpvarRegion;
-use rustc_middle::hir::place::{Place, PlaceBase, PlaceWithHirId, ProjectionKind};
+use rustc_middle::hir::place::{Place, PlaceBase, PlaceWithHirId, Projection, ProjectionKind};
 use rustc_middle::ty::fold::TypeFoldable;
 use rustc_middle::ty::{self, Ty, TyCtxt, TypeckResults, UpvarSubsts};
 use rustc_session::lint;
 use rustc_span::sym;
 use rustc_span::{MultiSpan, Span, Symbol};
 
+use rustc_index::vec::Idx;
+use rustc_target::abi::VariantIdx;
+
 /// Describe the relationship between the paths of two places
 /// eg:
 /// - `foo` is ancestor of `foo.bar.baz`
@@ -535,7 +538,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         span: Span,
         body: &'tcx hir::Body<'tcx>,
     ) {
-        let need_migrations = self.compute_2229_migrations_first_pass(
+        let need_migrations = self.compute_2229_migrations(
             closure_def_id,
             span,
             capture_clause,
@@ -544,9 +547,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         );
 
         if !need_migrations.is_empty() {
-            let need_migrations_hir_id = need_migrations.iter().map(|m| m.0).collect::<Vec<_>>();
-
-            let migrations_text = migration_suggestion_for_2229(self.tcx, &need_migrations_hir_id);
+            let migrations_text = migration_suggestion_for_2229(self.tcx, &need_migrations);
 
             let local_def_id = closure_def_id.expect_local();
             let closure_hir_id = self.tcx.hir().local_def_id_to_hir_id(local_def_id);
@@ -573,15 +574,15 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
     /// - It would have been moved into the closure when `capture_disjoint_fields` wasn't
     ///   enabled, **and**
     /// - It wasn't completely captured by the closure, **and**
-    /// - The type of the root variable needs Drop.
-    fn compute_2229_migrations_first_pass(
+    /// - One of the paths starting at this root variable, that is not captured needs Drop.
+    fn compute_2229_migrations(
         &self,
         closure_def_id: DefId,
         closure_span: Span,
         closure_clause: hir::CaptureBy,
         body: &'tcx hir::Body<'tcx>,
         min_captures: Option<&ty::RootVariableMinCaptureList<'tcx>>,
-    ) -> Vec<(hir::HirId, Ty<'tcx>)> {
+    ) -> Vec<hir::HirId> {
         fn resolve_ty<T: TypeFoldable<'tcx>>(
             fcx: &FnCtxt<'_, 'tcx>,
             span: Span,
@@ -617,7 +618,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
 
                 match closure_clause {
                     // Only migrate if closure is a move closure
-                    hir::CaptureBy::Value => need_migrations.push((var_hir_id, ty)),
+                    hir::CaptureBy::Value => need_migrations.push(var_hir_id),
 
                     hir::CaptureBy::Ref => {}
                 }
@@ -625,21 +626,277 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                 continue;
             };
 
-            let is_moved = root_var_min_capture_list
+            let projections_list = root_var_min_capture_list
                 .iter()
-                .any(|capture| matches!(capture.info.capture_kind, ty::UpvarCapture::ByValue(_)));
+                .filter_map(|captured_place| match captured_place.info.capture_kind {
+                    // Only care about captures that are moved into the closure
+                    ty::UpvarCapture::ByValue(..) => {
+                        Some(captured_place.place.projections.as_slice())
+                    }
+                    ty::UpvarCapture::ByRef(..) => None,
+                })
+                .collect::<Vec<_>>();
+
+            let is_moved = !projections_list.is_empty();
 
             let is_not_completely_captured =
                 root_var_min_capture_list.iter().any(|capture| capture.place.projections.len() > 0);
 
-            if is_moved && is_not_completely_captured {
-                need_migrations.push((var_hir_id, ty));
+            if is_moved
+                && is_not_completely_captured
+                && self.has_significant_drop_outside_of_captures(
+                    closure_def_id,
+                    closure_span,
+                    ty,
+                    projections_list,
+                )
+            {
+                need_migrations.push(var_hir_id);
             }
         }
 
         need_migrations
     }
 
+    /// This is a helper function to `compute_2229_migrations_precise_pass`. Provided the type
+    /// of a root variable and a list of captured paths starting at this root variable (expressed
+    /// using list of `Projection` slices), it returns true if there is a path that is not
+    /// captured starting at this root variable that implements Drop.
+    ///
+    /// FIXME(project-rfc-2229#35): This should return true only for significant drops.
+    ///                             A drop is significant if it's implemented by the user or does
+    ///                             anything that will have any observable behavior (other than
+    ///                             freeing up memory).
+    ///
+    /// The way this function works is at a given call it looks at type `base_path_ty` of some base
+    /// path say P and then list of projection slices which represent the different captures moved
+    /// into the closure starting off of P.
+    ///
+    /// This will make more sense with an example:
+    ///
+    /// ```rust
+    /// #![feature(capture_disjoint_fields)]
+    ///
+    /// struct FancyInteger(i32); // This implements Drop
+    ///
+    /// struct Point { x: FancyInteger, y: FancyInteger }
+    /// struct Color;
+    ///
+    /// struct Wrapper { p: Point, c: Color }
+    ///
+    /// fn f(w: Wrapper) {
+    ///   let c = || {
+    ///       // Closure captures w.p.x and w.c by move.
+    ///   };
+    ///
+    ///   c();
+    /// }
+    /// ```
+    ///
+    /// If `capture_disjoint_fields` wasn't enabled the closure would've moved `w` instead of the
+    /// precise paths. If we look closely `w.p.y` isn't captured which implements Drop and
+    /// therefore Drop ordering would change and we want this function to return true.
+    ///
+    /// Call stack to figure out if we need to migrate for `w` would look as follows:
+    ///
+    /// Our initial base path is just `w`, and the paths captured from it are `w[p, x]` and
+    /// `w[c]`.
+    /// Notation:
+    /// - Ty(place): Type of place
+    /// - `(a, b)`: Represents the function parameters `base_path_ty` and `captured_projs`
+    /// respectively.
+    /// ```
+    ///                  (Ty(w), [ &[p, x], &[c] ])
+    ///                                 |
+    ///                    ----------------------------
+    ///                    |                          |
+    ///                    v                          v
+    ///        (Ty(w.p), [ &[x] ])          (Ty(w.c), [ &[] ]) // I(1)
+    ///                    |                          |
+    ///                    v                          v
+    ///        (Ty(w.p), [ &[x] ])                 false
+    ///                    |
+    ///                    |
+    ///          -------------------------------
+    ///          |                             |
+    ///          v                             v
+    ///     (Ty((w.p).x), [ &[] ])     (Ty((w.p).y), []) // IMP 2
+    ///          |                             |
+    ///          v                             v
+    ///        false                     NeedsDrop(Ty(w.p.y))
+    ///                                        |
+    ///                                        v
+    ///                                      true
+    /// ```
+    ///
+    /// IMP 1 `(Ty(w.c), [ &[] ])`: Notice the single empty slice inside `captured_projs`.
+    ///                             This implies that the `w.c` is completely captured by the closure.
+    ///                             Since drop for this path will be called when the closure is
+    ///                             dropped we don't need to migrate for it.
+    ///
+    /// IMP 2 `(Ty((w.p).y), [])`: Notice that `captured_projs` is empty. This implies that this
+    ///                             path wasn't captured by the closure. Also note that even
+    ///                             though we didn't capture this path, the function visits it,
+    ///                             which is kind of the point of this function. We then return
+    ///                             if the type of `w.p.y` implements Drop, which in this case is
+    ///                             true.
+    ///
+    /// Consider another example:
+    ///
+    /// ```rust
+    /// struct X;
+    /// impl Drop for X {}
+    ///
+    /// struct Y(X);
+    /// impl Drop for Y {}
+    ///
+    /// fn foo() {
+    ///     let y = Y(X);
+    ///     let c = || move(y.0);
+    /// }
+    /// ```
+    ///
+    /// Note that `y.0` is captured by the closure. When this function is called for `y`, it will
+    /// return true, because even though all paths starting at `y` are captured, `y` itself
+    /// implements Drop which will be affected since `y` isn't completely captured.
+    fn has_significant_drop_outside_of_captures(
+        &self,
+        closure_def_id: DefId,
+        closure_span: Span,
+        base_path_ty: Ty<'tcx>,
+        captured_projs: Vec<&[Projection<'tcx>]>,
+    ) -> bool {
+        let needs_drop = |ty: Ty<'tcx>| {
+            ty.needs_drop(self.tcx, self.tcx.param_env(closure_def_id.expect_local()))
+        };
+
+        let is_drop_defined_for_ty = |ty: Ty<'tcx>| {
+            let drop_trait = self.tcx.require_lang_item(hir::LangItem::Drop, Some(closure_span));
+            let ty_params = self.tcx.mk_substs_trait(base_path_ty, &[]);
+            self.tcx.type_implements_trait((
+                drop_trait,
+                ty,
+                ty_params,
+                self.tcx.param_env(closure_def_id.expect_local()),
+            ))
+        };
+
+        let is_drop_defined_for_ty = is_drop_defined_for_ty(base_path_ty);
+
+        // If there is a case where no projection is applied on top of current place
+        // then there must be exactly one capture corresponding to such a case. Note that this
+        // represents the case of the path being completely captured by the variable.
+        //
+        // eg. If `a.b` is captured and we are processing `a.b`, then we can't have the closure also
+        //     capture `a.b.c`, because that voilates min capture.
+        let is_completely_captured = captured_projs.iter().any(|projs| projs.is_empty());
+
+        assert!(!is_completely_captured || (captured_projs.len() == 1));
+
+        if is_completely_captured {
+            // The place is captured entirely, so doesn't matter if needs dtor, it will be drop
+            // when the closure is dropped.
+            return false;
+        }
+
+        if is_drop_defined_for_ty {
+            // If drop is implemented for this type then we need it to be fully captured,
+            // which we know it is not because of the previous check. Therefore we need to
+            // do migrate.
+            return true;
+        }
+
+        if captured_projs.is_empty() {
+            return needs_drop(base_path_ty);
+        }
+
+        match base_path_ty.kind() {
+            // Observations:
+            // - `captured_projs` is not empty. Therefore we can call
+            //   `captured_projs.first().unwrap()` safely.
+            // - All entries in `captured_projs` have atleast one projection.
+            //   Therefore we can call `captured_projs.first().unwrap().first().unwrap()` safely.
+
+            // We don't capture derefs in case of move captures, which would have be applied to
+            // access any further paths.
+            ty::Adt(def, _) if def.is_box() => unreachable!(),
+            ty::Ref(..) => unreachable!(),
+            ty::RawPtr(..) => unreachable!(),
+
+            ty::Adt(def, substs) => {
+                // Multi-varaint enums are captured in entirety,
+                // which would've been handled in the case of single empty slice in `captured_projs`.
+                assert_eq!(def.variants.len(), 1);
+
+                // Only Field projections can be applied to a non-box Adt.
+                assert!(
+                    captured_projs.iter().all(|projs| matches!(
+                        projs.first().unwrap().kind,
+                        ProjectionKind::Field(..)
+                    ))
+                );
+                def.variants.get(VariantIdx::new(0)).unwrap().fields.iter().enumerate().any(
+                    |(i, field)| {
+                        let paths_using_field = captured_projs
+                            .iter()
+                            .filter_map(|projs| {
+                                if let ProjectionKind::Field(field_idx, _) =
+                                    projs.first().unwrap().kind
+                                {
+                                    if (field_idx as usize) == i { Some(&projs[1..]) } else { None }
+                                } else {
+                                    unreachable!();
+                                }
+                            })
+                            .collect();
+
+                        let after_field_ty = field.ty(self.tcx, substs);
+                        self.has_significant_drop_outside_of_captures(
+                            closure_def_id,
+                            closure_span,
+                            after_field_ty,
+                            paths_using_field,
+                        )
+                    },
+                )
+            }
+
+            ty::Tuple(..) => {
+                // Only Field projections can be applied to a tuple.
+                assert!(
+                    captured_projs.iter().all(|projs| matches!(
+                        projs.first().unwrap().kind,
+                        ProjectionKind::Field(..)
+                    ))
+                );
+
+                base_path_ty.tuple_fields().enumerate().any(|(i, element_ty)| {
+                    let paths_using_field = captured_projs
+                        .iter()
+                        .filter_map(|projs| {
+                            if let ProjectionKind::Field(field_idx, _) = projs.first().unwrap().kind
+                            {
+                                if (field_idx as usize) == i { Some(&projs[1..]) } else { None }
+                            } else {
+                                unreachable!();
+                            }
+                        })
+                        .collect();
+
+                    self.has_significant_drop_outside_of_captures(
+                        closure_def_id,
+                        closure_span,
+                        element_ty,
+                        paths_using_field,
+                    )
+                })
+            }
+
+            // Anything else would be completely captured and therefore handled already.
+            _ => unreachable!(),
+        }
+    }
+
     fn init_capture_kind(
         &self,
         capture_clause: hir::CaptureBy,
diff --git a/src/test/ui/closures/2229_closure_analysis/migrations/precise.rs b/src/test/ui/closures/2229_closure_analysis/migrations/precise.rs
new file mode 100644
index 00000000000..79702cc6b56
--- /dev/null
+++ b/src/test/ui/closures/2229_closure_analysis/migrations/precise.rs
@@ -0,0 +1,78 @@
+#![deny(disjoint_capture_drop_reorder)]
+//~^ NOTE: the lint level is defined here
+
+#[derive(Debug)]
+struct Foo(i32);
+impl Drop for Foo {
+    fn drop(&mut self) {
+        println!("{:?} dropped", self.0);
+    }
+}
+
+struct ConstainsDropField(Foo, Foo);
+
+#[derive(Debug)]
+struct ContainsAndImplsDrop(Foo);
+impl Drop for ContainsAndImplsDrop {
+    fn drop(&mut self) {
+        println!("{:?} dropped", self.0);
+    }
+}
+
+// Test that even if all paths starting at root variable that implement Drop are captured,
+// the lint is triggered if the root variable implements drop and isn't captured.
+fn test_precise_analysis_parent_root_impl_drop_not_captured() {
+    let t = ContainsAndImplsDrop(Foo(10));
+
+    let c = || {
+    //~^ERROR: drop order affected for closure because of `capture_disjoint_fields`
+    //~| NOTE: drop(&(t));
+        let _t = t.0;
+    };
+
+    c();
+}
+
+// Test that lint is triggered if a path that implements Drop is not captured by move
+fn test_precise_analysis_drop_paths_not_captured_by_move() {
+    let t = ConstainsDropField(Foo(10), Foo(20));
+
+    let c = || {
+    //~^ERROR: drop order affected for closure because of `capture_disjoint_fields`
+    //~| NOTE: drop(&(t));
+        let _t = t.0;
+        let _t = &t.1;
+    };
+
+    c();
+}
+
+struct S;
+impl Drop for S {
+    fn drop(&mut self) {
+    }
+}
+
+struct T(S, S);
+struct U(T, T);
+
+// Test precise analysis for the lint works with paths longer than one.
+fn test_precise_analysis_long_path_missing() {
+    let u = U(T(S, S), T(S, S));
+
+    let c = || {
+    //~^ERROR: drop order affected for closure because of `capture_disjoint_fields`
+    //~| NOTE: drop(&(u));
+        let _x = u.0.0;
+        let _x = u.0.1;
+        let _x = u.1.0;
+    };
+
+    c();
+}
+
+fn main() {
+    test_precise_analysis_parent_root_impl_drop_not_captured();
+    test_precise_analysis_drop_paths_not_captured_by_move();
+    test_precise_analysis_long_path_missing();
+}
diff --git a/src/test/ui/closures/2229_closure_analysis/migrations/precise.stderr b/src/test/ui/closures/2229_closure_analysis/migrations/precise.stderr
new file mode 100644
index 00000000000..968ca395f94
--- /dev/null
+++ b/src/test/ui/closures/2229_closure_analysis/migrations/precise.stderr
@@ -0,0 +1,49 @@
+error: drop order affected for closure because of `capture_disjoint_fields`
+  --> $DIR/precise.rs:27:13
+   |
+LL |       let c = || {
+   |  _____________^
+LL | |
+LL | |
+LL | |         let _t = t.0;
+LL | |     };
+   | |_____^
+   |
+note: the lint level is defined here
+  --> $DIR/precise.rs:1:9
+   |
+LL | #![deny(disjoint_capture_drop_reorder)]
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   = note: drop(&(t));
+
+error: drop order affected for closure because of `capture_disjoint_fields`
+  --> $DIR/precise.rs:40:13
+   |
+LL |       let c = || {
+   |  _____________^
+LL | |
+LL | |
+LL | |         let _t = t.0;
+LL | |         let _t = &t.1;
+LL | |     };
+   | |_____^
+   |
+   = note: drop(&(t));
+
+error: drop order affected for closure because of `capture_disjoint_fields`
+  --> $DIR/precise.rs:63:13
+   |
+LL |       let c = || {
+   |  _____________^
+LL | |
+LL | |
+LL | |         let _x = u.0.0;
+LL | |         let _x = u.0.1;
+LL | |         let _x = u.1.0;
+LL | |     };
+   | |_____^
+   |
+   = note: drop(&(u));
+
+error: aborting due to 3 previous errors
+
diff --git a/src/test/ui/closures/2229_closure_analysis/migrations/precise_no_migrations.rs b/src/test/ui/closures/2229_closure_analysis/migrations/precise_no_migrations.rs
new file mode 100644
index 00000000000..8af48501ca2
--- /dev/null
+++ b/src/test/ui/closures/2229_closure_analysis/migrations/precise_no_migrations.rs
@@ -0,0 +1,105 @@
+// run-pass
+
+#![deny(disjoint_capture_drop_reorder)]
+
+#[derive(Debug)]
+struct Foo(i32);
+impl Drop for Foo {
+    fn drop(&mut self) {
+        println!("{:?} dropped", self.0);
+    }
+}
+
+struct ConstainsDropField(Foo, Foo);
+
+// Test that if all paths starting at root variable that implement Drop are captured
+// then it doesn't trigger the lint.
+fn test_precise_analysis_simple_1() {
+    let t = (Foo(10), Foo(20), Foo(30));
+
+    let c = || {
+        let _t = t.0;
+        let _t = t.1;
+        let _t = t.2;
+    };
+
+    c();
+}
+
+// Test that if all paths starting at root variable that implement Drop are captured
+// then it doesn't trigger the lint.
+fn test_precise_analysis_simple_2() {
+    let t = ConstainsDropField(Foo(10), Foo(20));
+
+    let c = || {
+        let _t = t.0;
+        let _t = t.1;
+    };
+
+    c();
+}
+
+#[derive(Debug)]
+struct ContainsAndImplsDrop(Foo);
+impl Drop for ContainsAndImplsDrop {
+    fn drop(&mut self) {
+        println!("{:?} dropped", self.0);
+    }
+}
+
+// If a path isn't directly captured but requires Drop, then this tests that migrations aren't
+// needed if the a parent to that path is captured.
+fn test_precise_analysis_parent_captured_1() {
+    let t = ConstainsDropField(Foo(10), Foo(20));
+
+    let c = || {
+        let _t = t;
+    };
+
+    c();
+}
+
+// If a path isn't directly captured but requires Drop, then this tests that migrations aren't
+// needed if the a parent to that path is captured.
+fn test_precise_analysis_parent_captured_2() {
+    let t = ContainsAndImplsDrop(Foo(10));
+
+    let c = || {
+        let _t = t;
+    };
+
+    c();
+}
+
+struct S;
+impl Drop for S {
+    fn drop(&mut self) {
+    }
+}
+
+struct T(S, S);
+struct U(T, T);
+
+// Test that if the path is longer than just one element, precise analysis works correctly.
+fn test_precise_analysis_long_path() {
+    let u = U(T(S, S), T(S, S));
+
+    let c = || {
+        let _x = u.0.0;
+        let _x = u.0.1;
+        let _x = u.1.0;
+        let _x = u.1.1;
+    };
+
+    c();
+}
+
+fn main() {
+    test_precise_analysis_simple_1();
+    test_precise_analysis_simple_2();
+
+    test_precise_analysis_parent_captured_1();
+    test_precise_analysis_parent_captured_2();
+
+    test_precise_analysis_long_path();
+}