Skip to content

Commit c01036a

Browse files
arora-amanarora-aman
committed
Implement the precise analysis pass for lint disjoint_capture_drop_reorder 
1 parent 36931ce commit c01036a

File tree

1 file changed

+317
-5
lines changed

1 file changed

+317
-5
lines changed

‎compiler/rustc_typeck/src/check/upvar.rs‎

Lines changed: 317 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -40,13 +40,16 @@ use rustc_hir::def_id::DefId;
4040
use rustc_hir::def_id::LocalDefId;
4141
use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor};
4242
use rustc_infer::infer::UpvarRegion;
43-
use rustc_middle::hir::place::{Place, PlaceBase, PlaceWithHirId, ProjectionKind};
43+
use rustc_middle::hir::place::{Place, PlaceBase, PlaceWithHirId, Projection, ProjectionKind};
4444
use rustc_middle::ty::fold::TypeFoldable;
4545
use rustc_middle::ty::{self, Ty, TyCtxt, TypeckResults, UpvarSubsts};
4646
use rustc_session::lint;
4747
use rustc_span::sym;
4848
use rustc_span::{MultiSpan, Span, Symbol};
4949

50+
use rustc_index::vec::Idx;
51+
use rustc_target::abi::VariantIdx;
52+
5053
/// Describe the relationship between the paths of two places
5154
/// eg:
5255
/// - `foo` is ancestor of `foo.bar.baz`
@@ -537,18 +540,23 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
537540
span: Span,
538541
body: &'tcx hir::Body<'tcx>,
539542
) {
540-
let need_migrations = self.compute_2229_migrations_first_pass(
543+
let need_migrations_first_pass = self.compute_2229_migrations_first_pass(
541544
closure_def_id,
542545
span,
543546
capture_clause,
544547
body,
545548
self.typeck_results.borrow().closure_min_captures.get(&closure_def_id),
546549
);
547550

548-
if !need_migrations.is_empty() {
549-
let need_migrations_hir_id = need_migrations.iter().map(|m| m.0).collect::<Vec<_>>();
551+
let need_migrations = self.compute_2229_migrations_precise_pass(
552+
closure_def_id,
553+
span,
554+
self.typeck_results.borrow().closure_min_captures.get(&closure_def_id),
555+
&need_migrations_first_pass,
556+
);
550557

551-
let migrations_text = migration_suggestion_for_2229(self.tcx, &need_migrations_hir_id);
558+
if !need_migrations.is_empty() {
559+
let migrations_text = migration_suggestion_for_2229(self.tcx, &need_migrations);
552560

553561
let local_def_id = closure_def_id.expect_local();
554562
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> {
642650
need_migrations
643651
}
644652

653+
fn compute_2229_migrations_precise_pass(
654+
&self,
655+
closure_def_id: DefId,
656+
closure_span: Span,
657+
min_captures: Option<&ty::RootVariableMinCaptureList<'tcx>>,
658+
need_migrations: &[(hir::HirId, Ty<'tcx>)],
659+
) -> Vec<hir::HirId> {
660+
// Need migrations -- second pass
661+
let mut need_migrations_2 = Vec::new();
662+
663+
for (hir_id, ty) in need_migrations {
664+
let projections_list = min_captures
665+
.and_then(|m| m.get(hir_id))
666+
.into_iter()
667+
.flatten()
668+
.filter_map(|captured_place| match captured_place.info.capture_kind {
669+
// Only care about captures that are moved into the closure
670+
ty::UpvarCapture::ByValue(..) => {
671+
Some(captured_place.place.projections.as_slice())
672+
}
673+
ty::UpvarCapture::ByRef(..) => None,
674+
})
675+
.collect();
676+
677+
if self.has_significant_drop_outside_of_captures(
678+
closure_def_id,
679+
closure_span,
680+
ty,
681+
projections_list,
682+
) {
683+
need_migrations_2.push(*hir_id);
684+
}
685+
}
686+
687+
need_migrations_2
688+
}
689+
690+
/// This is a helper function to `compute_2229_migrations_precise_pass`. Provided the type
691+
/// of a root variable and a list of captured paths starting at this root variable (expressed
692+
/// using list of `Projection` slices), it returns true if there is a path that is not
693+
/// captured starting at this root variable that implements Drop.
694+
///
695+
/// FIXME(project-rfc-2229#35): This should return true only for significant drops.
696+
/// A drop is significant if it's implemented by the user or does
697+
/// anything that will have any observable behavior (other than
698+
/// freeing up memory).
699+
///
700+
/// The way this function works is at a given call it looks at type `base_path_ty` of some base
701+
/// path say P and then vector of projection slices which represent the different captures
702+
/// starting off of P.
703+
///
704+
/// This will make more sense with an example:
705+
///
706+
/// ```rust
707+
/// #![feature(capture_disjoint_fields)]
708+
///
709+
/// struct FancyInteger(i32); // This implements Drop
710+
///
711+
/// struct Point { x: FancyInteger, y: FancyInteger }
712+
/// struct Color;
713+
///
714+
/// struct Wrapper { p: Point, c: Color }
715+
///
716+
/// fn f(w: Wrapper) {
717+
/// let c = || {
718+
/// // Closure captures w.p.x and w.c by move.
719+
/// };
720+
///
721+
/// c();
722+
/// }
723+
/// ```
724+
///
725+
/// If `capture_disjoint_fields` wasn't enabled the closure would've moved `w` instead of the
726+
/// precise paths. If we look closely `w.p.y` isn't captured which implements Drop and
727+
/// therefore Drop ordering would change and we want this function to return true.
728+
///
729+
/// Call stack to figure out if we need to migrate for `w` would look as follows:
730+
///
731+
/// Our initial base path is just `w`, and the paths captured from it are `w[p, x]` and
732+
/// `w[c]`.
733+
/// Notation:
734+
/// - Ty(place): Type of place
735+
/// - `(a, b)`: Represents the function parameters `base_path_ty` and `captured_projs`
736+
/// respectively.
737+
/// ```
738+
/// (Ty(w), [ &[p, x], &[c] ])
739+
/// |
740+
/// ----------------------------
741+
/// | |
742+
/// v v
743+
/// (Ty(w.p), [ &[x] ]) (Ty(w.c), [ &[] ]) // I(1)
744+
/// | |
745+
/// v v
746+
/// (Ty(w.p), [ &[x] ]) false
747+
/// |
748+
/// |
749+
/// -------------------------------
750+
/// | |
751+
/// v v
752+
/// (Ty((w.p).x), [ &[] ]) (Ty((w.p).y), []) // IMP 2
753+
/// | |
754+
/// v v
755+
/// false NeedsDrop(Ty(w.p.y))
756+
/// |
757+
/// v
758+
/// true
759+
/// ```
760+
///
761+
/// IMP 1 `(Ty(w.c), [ &[] ])`: Notice the single empty slice inside `captured_projs`.
762+
/// This implies that the `w.c` is completely captured by the closure.
763+
/// Since drop for this path will be called when the closure is
764+
/// dropped we don't need to migrate for it.
765+
///
766+
/// IMP 2 `(Ty((w.p).y), [])`: Notice that `captured_projs` is empty. This implies that this
767+
/// path wasn't captured by the closure. Also note that even
768+
/// though we didn't capture this path, the function visits it,
769+
/// which is kind of the point of this function. We then return
770+
/// if the type of `w.p.y` implements Drop, which in this case is
771+
/// true.
772+
///
773+
/// Consider another example:
774+
///
775+
/// ```rust
776+
/// struct X;
777+
/// impl Drop for X {}
778+
///
779+
/// struct Y(X);
780+
/// impl Drop for Y {}
781+
///
782+
/// fn foo() {
783+
/// let y = Y(X);
784+
/// let c = || move(y.0);
785+
/// }
786+
/// ```
787+
///
788+
/// Note that `y.0` is captured by the closure. When this function is called for `y`, it will
789+
/// return true, because even though all paths starting at `y` are captured, `y` itself
790+
/// implements Drop which will be affected since `y` isn't completely captured.
791+
fn has_significant_drop_outside_of_captures(
792+
&self,
793+
closure_def_id: DefId,
794+
closure_span: Span,
795+
base_path_ty: Ty<'tcx>,
796+
captured_projs: Vec<&[Projection<'tcx>]>,
797+
) -> bool {
798+
let needs_drop = |ty: Ty<'tcx>| {
799+
ty.needs_drop(self.tcx, self.tcx.param_env(closure_def_id.expect_local()))
800+
};
801+
802+
let is_drop_defined_for_ty = |ty: Ty<'tcx>| {
803+
let drop_trait = self.tcx.require_lang_item(hir::LangItem::Drop, Some(closure_span));
804+
let ty_params = self.tcx.mk_substs_trait(base_path_ty, &[]);
805+
self.tcx.type_implements_trait((
806+
drop_trait,
807+
ty,
808+
ty_params,
809+
self.tcx.param_env(closure_def_id.expect_local()),
810+
))
811+
};
812+
813+
let is_drop_defined_for_ty = is_drop_defined_for_ty(base_path_ty);
814+
815+
// If there is a case where no projection is applied on top of current place
816+
// then there must be exactly one capture corresponding to such a case. Note that this
817+
// represents the case of the path being completely captured by the variable.
818+
//
819+
// eg. If `a.b` is captured and we are processing `a.b`, then we can't have the closure also
820+
// capture `a.b.c`, because that voilates min capture.
821+
let is_completely_captured = captured_projs.iter().any(|projs| projs.is_empty());
822+
823+
assert!(!is_completely_captured || (captured_projs.len() == 1));
824+
825+
if is_drop_defined_for_ty {
826+
// If drop is implemented for this type then we need it to be fully captured, or
827+
// it will require migration.
828+
return !is_completely_captured;
829+
}
830+
831+
if is_completely_captured {
832+
// The place is captured entirely, so doesn't matter if needs dtor, it will be drop
833+
// when the closure is dropped.
834+
return false;
835+
}
836+
837+
match base_path_ty.kind() {
838+
_ if captured_projs.is_empty() => needs_drop(base_path_ty),
839+
840+
// Observations:
841+
// - `captured_projs` is not empty. Therefore we can call
842+
// `captured_projs.first().unwrap()` safely.
843+
// - All entries in `captured_projs` have atleast one projection.
844+
// Therefore we can call `captured_projs.first().unwrap().first().unwrap()` safely.
845+
ty::Adt(def, _) if def.is_box() => {
846+
// We must deref to access paths on top of a Box.
847+
assert!(
848+
captured_projs
849+
.iter()
850+
.all(|projs| matches!(projs.first().unwrap().kind, ProjectionKind::Deref))
851+
);
852+
853+
let next_ty = captured_projs.first().unwrap().first().unwrap().ty;
854+
let captured_projs = captured_projs.iter().map(|projs| &projs[1..]).collect();
855+
self.has_significant_drop_outside_of_captures(
856+
closure_def_id,
857+
closure_span,
858+
next_ty,
859+
captured_projs,
860+
)
861+
}
862+
863+
ty::Adt(def, substs) => {
864+
// Multi-varaint enums are captured in entirety,
865+
// which would've been handled in the case of single empty slice in `captured_projs`.
866+
assert_eq!(def.variants.len(), 1);
867+
868+
// Only Field projections can be applied to a non-box Adt.
869+
assert!(
870+
captured_projs.iter().all(|projs| matches!(
871+
projs.first().unwrap().kind,
872+
ProjectionKind::Field(..)
873+
))
874+
);
875+
def.variants.get(VariantIdx::new(0)).unwrap().fields.iter().enumerate().any(
876+
|(i, field)| {
877+
let paths_using_field = captured_projs
878+
.iter()
879+
.filter_map(|projs| {
880+
if let ProjectionKind::Field(field_idx, _) =
881+
projs.first().unwrap().kind
882+
{
883+
if (field_idx as usize) == i { Some(&projs[1..]) } else { None }
884+
} else {
885+
unreachable!();
886+
}
887+
})
888+
.collect();
889+
890+
let after_field_ty = field.ty(self.tcx, substs);
891+
self.has_significant_drop_outside_of_captures(
892+
closure_def_id,
893+
closure_span,
894+
after_field_ty,
895+
paths_using_field,
896+
)
897+
},
898+
)
899+
}
900+
901+
ty::Tuple(..) => {
902+
// Only Field projections can be applied to a tuple.
903+
assert!(
904+
captured_projs.iter().all(|projs| matches!(
905+
projs.first().unwrap().kind,
906+
ProjectionKind::Field(..)
907+
))
908+
);
909+
910+
base_path_ty.tuple_fields().enumerate().any(|(i, element_ty)| {
911+
let paths_using_field = captured_projs
912+
.iter()
913+
.filter_map(|projs| {
914+
if let ProjectionKind::Field(field_idx, _) = projs.first().unwrap().kind
915+
{
916+
if (field_idx as usize) == i { Some(&projs[1..]) } else { None }
917+
} else {
918+
unreachable!();
919+
}
920+
})
921+
.collect();
922+
923+
self.has_significant_drop_outside_of_captures(
924+
closure_def_id,
925+
closure_span,
926+
element_ty,
927+
paths_using_field,
928+
)
929+
})
930+
}
931+
932+
ty::Ref(_, deref_ty, _) => {
933+
// Only Derefs can be applied to a Ref
934+
assert!(
935+
captured_projs
936+
.iter()
937+
.all(|projs| matches!(projs.first().unwrap().kind, ProjectionKind::Deref))
938+
);
939+
940+
let captured_projs = captured_projs.iter().map(|projs| &projs[1..]).collect();
941+
self.has_significant_drop_outside_of_captures(
942+
closure_def_id,
943+
closure_span,
944+
deref_ty,
945+
captured_projs,
946+
)
947+
}
948+
949+
// Unsafe Ptrs are captured in their entirety, which would've have been handled in
950+
// the case of single empty slice in `captured_projs`.
951+
ty::RawPtr(..) => unreachable!(),
952+
953+
_ => unreachable!(),
954+
}
955+
}
956+
645957
fn init_capture_kind(
646958
&self,
647959
capture_clause: hir::CaptureBy,

0 commit comments

Comments
 (0)