Skip to content

Commit 2531563

Browse files
Dylan-DPCDylan-DPC
authored
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`
2 parents 641c378 + 96c12f9 commit 2531563

File tree

4 files changed

+502
-13
lines changed

4 files changed

+502
-13
lines changed

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

+270-13
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`
@@ -535,7 +538,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
535538
span: Span,
536539
body: &'tcx hir::Body<'tcx>,
537540
) {
538-
let need_migrations = self.compute_2229_migrations_first_pass(
541+
let need_migrations = self.compute_2229_migrations(
539542
closure_def_id,
540543
span,
541544
capture_clause,
@@ -544,9 +547,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
544547
);
545548

546549
if !need_migrations.is_empty() {
547-
let need_migrations_hir_id = need_migrations.iter().map(|m| m.0).collect::<Vec<_>>();
548-
549-
let migrations_text = migration_suggestion_for_2229(self.tcx, &need_migrations_hir_id);
550+
let migrations_text = migration_suggestion_for_2229(self.tcx, &need_migrations);
550551

551552
let local_def_id = closure_def_id.expect_local();
552553
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> {
573574
/// - It would have been moved into the closure when `capture_disjoint_fields` wasn't
574575
/// enabled, **and**
575576
/// - It wasn't completely captured by the closure, **and**
576-
/// - The type of the root variable needs Drop.
577-
fn compute_2229_migrations_first_pass(
577+
/// - One of the paths starting at this root variable, that is not captured needs Drop.
578+
fn compute_2229_migrations(
578579
&self,
579580
closure_def_id: DefId,
580581
closure_span: Span,
581582
closure_clause: hir::CaptureBy,
582583
body: &'tcx hir::Body<'tcx>,
583584
min_captures: Option<&ty::RootVariableMinCaptureList<'tcx>>,
584-
) -> Vec<(hir::HirId, Ty<'tcx>)> {
585+
) -> Vec<hir::HirId> {
585586
fn resolve_ty<T: TypeFoldable<'tcx>>(
586587
fcx: &FnCtxt<'_, 'tcx>,
587588
span: Span,
@@ -617,29 +618,285 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
617618

618619
match closure_clause {
619620
// Only migrate if closure is a move closure
620-
hir::CaptureBy::Value => need_migrations.push((var_hir_id, ty)),
621+
hir::CaptureBy::Value => need_migrations.push(var_hir_id),
621622

622623
hir::CaptureBy::Ref => {}
623624
}
624625

625626
continue;
626627
};
627628

628-
let is_moved = root_var_min_capture_list
629+
let projections_list = root_var_min_capture_list
629630
.iter()
630-
.any(|capture| matches!(capture.info.capture_kind, ty::UpvarCapture::ByValue(_)));
631+
.filter_map(|captured_place| match captured_place.info.capture_kind {
632+
// Only care about captures that are moved into the closure
633+
ty::UpvarCapture::ByValue(..) => {
634+
Some(captured_place.place.projections.as_slice())
635+
}
636+
ty::UpvarCapture::ByRef(..) => None,
637+
})
638+
.collect::<Vec<_>>();
639+
640+
let is_moved = !projections_list.is_empty();
631641

632642
let is_not_completely_captured =
633643
root_var_min_capture_list.iter().any(|capture| capture.place.projections.len() > 0);
634644

635-
if is_moved && is_not_completely_captured {
636-
need_migrations.push((var_hir_id, ty));
645+
if is_moved
646+
&& is_not_completely_captured
647+
&& self.has_significant_drop_outside_of_captures(
648+
closure_def_id,
649+
closure_span,
650+
ty,
651+
projections_list,
652+
)
653+
{
654+
need_migrations.push(var_hir_id);
637655
}
638656
}
639657

640658
need_migrations
641659
}
642660

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

0 commit comments

Comments
 (0)