From c01036af1d8da0e5e16f8c126fa0017049d52636 Mon Sep 17 00:00:00 2001 From: Aman Arora Date: Sun, 27 Dec 2020 01:13:51 -0500 Subject: [PATCH] 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 04a9e65e6647..4d21629cd69f 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,