From c01036af1d8da0e5e16f8c126fa0017049d52636 Mon Sep 17 00:00:00 2001 From: Aman Arora Date: Sun, 27 Dec 2020 01:13:51 -0500 Subject: [PATCH 1/4] Implement the precise analysis pass for lint `disjoint_capture_drop_reorder` --- compiler/rustc_typeck/src/check/upvar.rs | 322 ++++++++++++++++++++++- 1 file changed, 317 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_typeck/src/check/upvar.rs b/compiler/rustc_typeck/src/check/upvar.rs index 04a9e65e6647d..4d21629cd69fc 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` @@ -537,7 +540,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_first_pass = self.compute_2229_migrations_first_pass( closure_def_id, span, capture_clause, @@ -545,10 +548,15 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { self.typeck_results.borrow().closure_min_captures.get(&closure_def_id), ); - if !need_migrations.is_empty() { - let need_migrations_hir_id = need_migrations.iter().map(|m| m.0).collect::>(); + let need_migrations = self.compute_2229_migrations_precise_pass( + closure_def_id, + span, + self.typeck_results.borrow().closure_min_captures.get(&closure_def_id), + &need_migrations_first_pass, + ); - let migrations_text = migration_suggestion_for_2229(self.tcx, &need_migrations_hir_id); + if !need_migrations.is_empty() { + 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); @@ -642,6 +650,310 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { need_migrations } + fn compute_2229_migrations_precise_pass( + &self, + closure_def_id: DefId, + closure_span: Span, + min_captures: Option<&ty::RootVariableMinCaptureList<'tcx>>, + need_migrations: &[(hir::HirId, Ty<'tcx>)], + ) -> Vec { + // Need migrations -- second pass + let mut need_migrations_2 = Vec::new(); + + for (hir_id, ty) in need_migrations { + let projections_list = min_captures + .and_then(|m| m.get(hir_id)) + .into_iter() + .flatten() + .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(); + + if self.has_significant_drop_outside_of_captures( + closure_def_id, + closure_span, + ty, + projections_list, + ) { + need_migrations_2.push(*hir_id); + } + } + + need_migrations_2 + } + + /// 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 vector of projection slices which represent the different captures + /// 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_drop_defined_for_ty { + // If drop is implemented for this type then we need it to be fully captured, or + // it will require migration. + return !is_completely_captured; + } + + 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; + } + + match base_path_ty.kind() { + _ if captured_projs.is_empty() => needs_drop(base_path_ty), + + // 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. + ty::Adt(def, _) if def.is_box() => { + // We must deref to access paths on top of a Box. + assert!( + captured_projs + .iter() + .all(|projs| matches!(projs.first().unwrap().kind, ProjectionKind::Deref)) + ); + + let next_ty = captured_projs.first().unwrap().first().unwrap().ty; + let captured_projs = captured_projs.iter().map(|projs| &projs[1..]).collect(); + self.has_significant_drop_outside_of_captures( + closure_def_id, + closure_span, + next_ty, + captured_projs, + ) + } + + 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, + ) + }) + } + + ty::Ref(_, deref_ty, _) => { + // Only Derefs can be applied to a Ref + assert!( + captured_projs + .iter() + .all(|projs| matches!(projs.first().unwrap().kind, ProjectionKind::Deref)) + ); + + let captured_projs = captured_projs.iter().map(|projs| &projs[1..]).collect(); + self.has_significant_drop_outside_of_captures( + closure_def_id, + closure_span, + deref_ty, + captured_projs, + ) + } + + // Unsafe Ptrs are captured in their entirety, which would've have been handled in + // the case of single empty slice in `captured_projs`. + ty::RawPtr(..) => unreachable!(), + + _ => unreachable!(), + } + } + fn init_capture_kind( &self, capture_clause: hir::CaptureBy, From 319f1aba6281adb6ca22522003b49578c406745f Mon Sep 17 00:00:00 2001 From: Aman Arora Date: Thu, 4 Feb 2021 18:50:32 -0500 Subject: [PATCH 2/4] Tests for precise lint analysis --- .../migrations/precise.rs | 78 +++++++++++++ .../migrations/precise.stderr | 49 ++++++++ .../migrations/precise_no_migrations.rs | 105 ++++++++++++++++++ 3 files changed, 232 insertions(+) create mode 100644 src/test/ui/closures/2229_closure_analysis/migrations/precise.rs create mode 100644 src/test/ui/closures/2229_closure_analysis/migrations/precise.stderr create mode 100644 src/test/ui/closures/2229_closure_analysis/migrations/precise_no_migrations.rs 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 0000000000000..79702cc6b56f3 --- /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 0000000000000..968ca395f946e --- /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 0000000000000..8af48501ca295 --- /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(); +} From 5b54640128766d967d5d7366f5d068cd4a774ead Mon Sep 17 00:00:00 2001 From: Aman Arora Date: Thu, 4 Feb 2021 19:55:16 -0500 Subject: [PATCH 3/4] Mark migration code that relies on Deref unreachable --- compiler/rustc_typeck/src/check/upvar.rs | 47 ++++-------------------- 1 file changed, 8 insertions(+), 39 deletions(-) diff --git a/compiler/rustc_typeck/src/check/upvar.rs b/compiler/rustc_typeck/src/check/upvar.rs index 4d21629cd69fc..1ffa8c37a8032 100644 --- a/compiler/rustc_typeck/src/check/upvar.rs +++ b/compiler/rustc_typeck/src/check/upvar.rs @@ -698,8 +698,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { /// 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 vector of projection slices which represent the different captures - /// starting off of P. + /// 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: /// @@ -842,23 +842,12 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { // `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. - ty::Adt(def, _) if def.is_box() => { - // We must deref to access paths on top of a Box. - assert!( - captured_projs - .iter() - .all(|projs| matches!(projs.first().unwrap().kind, ProjectionKind::Deref)) - ); - let next_ty = captured_projs.first().unwrap().first().unwrap().ty; - let captured_projs = captured_projs.iter().map(|projs| &projs[1..]).collect(); - self.has_significant_drop_outside_of_captures( - closure_def_id, - closure_span, - next_ty, - captured_projs, - ) - } + // 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, @@ -929,27 +918,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { }) } - ty::Ref(_, deref_ty, _) => { - // Only Derefs can be applied to a Ref - assert!( - captured_projs - .iter() - .all(|projs| matches!(projs.first().unwrap().kind, ProjectionKind::Deref)) - ); - - let captured_projs = captured_projs.iter().map(|projs| &projs[1..]).collect(); - self.has_significant_drop_outside_of_captures( - closure_def_id, - closure_span, - deref_ty, - captured_projs, - ) - } - - // Unsafe Ptrs are captured in their entirety, which would've have been handled in - // the case of single empty slice in `captured_projs`. - ty::RawPtr(..) => unreachable!(), - + // Anything else would be completely captured and therefore handled already. _ => unreachable!(), } } From 96c12f90cf62442bc5cba1d8c1c8049ee4652237 Mon Sep 17 00:00:00 2001 From: Aman Arora Date: Fri, 12 Feb 2021 04:07:41 -0500 Subject: [PATCH 4/4] fixup! Implement the precise analysis pass for lint `disjoint_capture_drop_reorder` --- compiler/rustc_typeck/src/check/upvar.rs | 92 +++++++++--------------- 1 file changed, 34 insertions(+), 58 deletions(-) diff --git a/compiler/rustc_typeck/src/check/upvar.rs b/compiler/rustc_typeck/src/check/upvar.rs index 1ffa8c37a8032..0d045c1b9c475 100644 --- a/compiler/rustc_typeck/src/check/upvar.rs +++ b/compiler/rustc_typeck/src/check/upvar.rs @@ -540,7 +540,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { span: Span, body: &'tcx hir::Body<'tcx>, ) { - let need_migrations_first_pass = self.compute_2229_migrations_first_pass( + let need_migrations = self.compute_2229_migrations( closure_def_id, span, capture_clause, @@ -548,13 +548,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { self.typeck_results.borrow().closure_min_captures.get(&closure_def_id), ); - let need_migrations = self.compute_2229_migrations_precise_pass( - closure_def_id, - span, - self.typeck_results.borrow().closure_min_captures.get(&closure_def_id), - &need_migrations_first_pass, - ); - if !need_migrations.is_empty() { let migrations_text = migration_suggestion_for_2229(self.tcx, &need_migrations); @@ -583,15 +576,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 { fn resolve_ty>( fcx: &FnCtxt<'_, 'tcx>, span: Span, @@ -627,7 +620,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 => {} } @@ -635,36 +628,8 @@ 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(_))); - - 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)); - } - } - - need_migrations - } - - fn compute_2229_migrations_precise_pass( - &self, - closure_def_id: DefId, - closure_span: Span, - min_captures: Option<&ty::RootVariableMinCaptureList<'tcx>>, - need_migrations: &[(hir::HirId, Ty<'tcx>)], - ) -> Vec { - // Need migrations -- second pass - let mut need_migrations_2 = Vec::new(); - - for (hir_id, ty) in need_migrations { - let projections_list = min_captures - .and_then(|m| m.get(hir_id)) - .into_iter() - .flatten() .filter_map(|captured_place| match captured_place.info.capture_kind { // Only care about captures that are moved into the closure ty::UpvarCapture::ByValue(..) => { @@ -672,19 +637,27 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { } ty::UpvarCapture::ByRef(..) => None, }) - .collect(); + .collect::>(); - if self.has_significant_drop_outside_of_captures( - closure_def_id, - closure_span, - ty, - projections_list, - ) { - need_migrations_2.push(*hir_id); + 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 + && self.has_significant_drop_outside_of_captures( + closure_def_id, + closure_span, + ty, + projections_list, + ) + { + need_migrations.push(var_hir_id); } } - need_migrations_2 + need_migrations } /// This is a helper function to `compute_2229_migrations_precise_pass`. Provided the type @@ -822,21 +795,24 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { assert!(!is_completely_captured || (captured_projs.len() == 1)); - if is_drop_defined_for_ty { - // If drop is implemented for this type then we need it to be fully captured, or - // it will require migration. - return !is_completely_captured; - } - 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; } - match base_path_ty.kind() { - _ if captured_projs.is_empty() => needs_drop(base_path_ty), + 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.