Skip to content

Commit

Permalink
Address review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@arora-aman
arora-aman committed Nov 9, 2020
1 parent a80f2df commit c2deba7
Show file tree
Hide file tree
Showing 3 changed files with 131 additions and 85 deletions.
5 changes: 2 additions & 3 deletions compiler/rustc_middle/src/ty/context.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -415,9 +415,8 @@ pub struct TypeckResults<'tcx> {
/// entire variable.
pub closure_captures: ty::UpvarListMap,

/// Given the closure DefId this map provides a map of
/// root variables to minimum set of `Place`s (and how) that need to be tracked
/// to support all captures of that closure.
/// Tracks the minimum captures required for a closure;
/// see `MinCaptureInformationMap` for more details.
pub closure_min_captures: ty::MinCaptureInformationMap<'tcx>,

/// Stores the type, expression, span and optional scope span of all types
Expand Down
35 changes: 25 additions & 10 deletions compiler/rustc_middle/src/ty/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -763,6 +763,31 @@ pub struct UpvarBorrow<'tcx> {
pub region: ty::Region<'tcx>,
}

/// Given the closure DefId this map provides a map of root variables to minimum
/// set of `CapturedPlace`s that need to be tracked to support all captures of that closure.
pub type MinCaptureInformationMap<'tcx> = FxHashMap<DefId, RootVariableMinCaptureList<'tcx>>;

/// Part of `MinCaptureInformationMap`; Maps a root variable to the list of `CapturedPlace`.
/// Used to track the minimum set of `Place`s that need to be captured to support all
/// Places captured by the closure starting at a given root variable.
///
/// This provides a convenient and quick way of checking if a variable being used within
/// a closure is a capture of a local variable.
pub type RootVariableMinCaptureList<'tcx> = FxIndexMap<hir::HirId, MinCaptureList<'tcx>>;

/// Part of `MinCaptureInformationMap`; List of `CapturePlace`s.
pub type MinCaptureList<'tcx> = Vec<CapturedPlace<'tcx>>;

/// A `Place` and the corresponding `CaptureInfo`.
#[derive(PartialEq, Clone, Debug, TyEncodable, TyDecodable, HashStable)]
pub struct CapturedPlace<'tcx> {
pub place: HirPlace<'tcx>,
pub info: CaptureInfo<'tcx>,
}

/// Part of `MinCaptureInformationMap`; describes the capture kind (&, &mut, move)
/// for a particular capture as well as identifying the part of the source code
/// that triggered this capture to occur.
#[derive(PartialEq, Clone, Debug, Copy, TyEncodable, TyDecodable, HashStable)]
pub struct CaptureInfo<'tcx> {
/// Expr Id pointing to use that resulted in selecting the current capture kind
Expand All @@ -788,19 +813,9 @@ pub struct CaptureInfo<'tcx> {
pub capture_kind: UpvarCapture<'tcx>,
}

#[derive(PartialEq, Clone, Debug, TyEncodable, TyDecodable, HashStable)]
pub struct CapturedPlace<'tcx> {
pub place: HirPlace<'tcx>,
pub info: CaptureInfo<'tcx>,
}

pub type UpvarListMap = FxHashMap<DefId, FxIndexMap<hir::HirId, UpvarId>>;
pub type UpvarCaptureMap<'tcx> = FxHashMap<UpvarId, UpvarCapture<'tcx>>;

pub type MinCaptureList<'tcx> = Vec<CapturedPlace<'tcx>>;
pub type RootVariableMinCaptureList<'tcx> = FxIndexMap<hir::HirId, MinCaptureList<'tcx>>;
pub type MinCaptureInformationMap<'tcx> = FxHashMap<DefId, RootVariableMinCaptureList<'tcx>>;

#[derive(Clone, Copy, PartialEq, Eq)]
pub enum IntVarValue {
IntType(ast::IntTy),
Expand Down
176 changes: 104 additions & 72 deletions compiler/rustc_typeck/src/check/upvar.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,9 @@ use rustc_span::{Span, Symbol};

/// Describe the relationship between the paths of two places
/// eg:
/// - foo is ancestor of foo.bar.baz
/// - foo.bar.baz is an descendant of foo.bar,
/// - foo.bar and foo.baz are divergent
/// - `foo` is ancestor of `foo.bar.baz`
/// - `foo.bar.baz` is an descendant of `foo.bar`
/// - `foo.bar` and `foo.baz` are divergent
enum PlaceAncestryRelation {
Ancestor,
Descendant,
Expand Down Expand Up @@ -124,7 +124,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

let local_def_id = closure_def_id.expect_local();

let mut capture_information = FxIndexMap::<Place<'tcx>, ty::CaptureInfo<'tcx>>::default();
let mut capture_information: FxIndexMap<Place<'tcx>, ty::CaptureInfo<'tcx>> =
Default::default();
if !self.tcx.features().capture_disjoint_fields {
if let Some(upvars) = self.tcx.upvars_mentioned(closure_def_id) {
for (&var_hir_id, _) in upvars.iter() {
Expand Down Expand Up @@ -186,7 +187,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
self.compute_min_captures(closure_def_id, delegate);
self.log_closure_min_capture_info(closure_def_id, span);

self.set_closure_captures(closure_def_id);
self.min_captures_to_closure_captures_bridge(closure_def_id);

// Now that we've analyzed the closure, we know how each
// variable is borrowed, and we know what traits the closure
Expand Down Expand Up @@ -274,8 +275,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
///
/// 2. upvar_capture_map
/// UpvarId(foo,c) -> MutBorrow, UpvarId(bar, c) -> ByValue

fn set_closure_captures(&self, closure_def_id: DefId) {
fn min_captures_to_closure_captures_bridge(&self, closure_def_id: DefId) {
let mut closure_captures: FxIndexMap<hir::HirId, ty::UpvarId> = Default::default();
let mut upvar_capture_map = ty::UpvarCaptureMap::default();

Expand Down Expand Up @@ -304,8 +304,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
// so we create a fake capture info with no expression.
let fake_capture_info =
ty::CaptureInfo { expr_id: None, capture_kind: capture_kind.clone() };
self.determine_capture_info(fake_capture_info, capture_info.clone())
.capture_kind
determine_capture_info(fake_capture_info, capture_info).capture_kind
} else {
capture_info.capture_kind
};
Expand All @@ -329,7 +328,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}
}

/// Analyses the information collected by InferBorrowKind to compute the min number of
/// Analyzes the information collected by `InferBorrowKind` to compute the min number of
/// Places (and corresponding capture kind) that we need to keep track of to support all
/// the required captured paths.
///
Expand Down Expand Up @@ -420,8 +419,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
// current place is ancestor of possible_descendant
PlaceAncestryRelation::Ancestor => {
descendant_found = true;
updated_capture_info = self
.determine_capture_info(updated_capture_info, possible_descendant.info);
updated_capture_info =
determine_capture_info(updated_capture_info, possible_descendant.info);
false
}

Expand All @@ -437,7 +436,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
PlaceAncestryRelation::Descendant => {
ancestor_found = true;
possible_ancestor.info =
self.determine_capture_info(possible_ancestor.info, capture_info);
determine_capture_info(possible_ancestor.info, capture_info);

// Only one ancestor of the current place will be in the list.
break;
Expand Down Expand Up @@ -500,60 +499,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
self.tcx.has_attr(closure_def_id, sym::rustc_capture_analysis)
}

/// Helper function to determine if we need to escalate CaptureKind from
/// CaptureInfo A to B and returns the escalated CaptureInfo.
/// (Note: CaptureInfo contains CaptureKind and an expression that led to capture it in that way)
///
/// If both `CaptureKind`s are considered equivalent, then the CaptureInfo is selected based
/// on the `CaptureInfo` containing an associated expression id.
///
/// If both the CaptureKind and Expression are considered to be equivalent,
/// then `CaptureInfo` A is preferred.
fn determine_capture_info(
&self,
capture_info_a: ty::CaptureInfo<'tcx>,
capture_info_b: ty::CaptureInfo<'tcx>,
) -> ty::CaptureInfo<'tcx> {
// If the capture kind is equivalent then, we don't need to escalate and can compare the
// expressions.
let eq_capture_kind = match (capture_info_a.capture_kind, capture_info_b.capture_kind) {
(ty::UpvarCapture::ByValue(_), ty::UpvarCapture::ByValue(_)) => true,
(ty::UpvarCapture::ByRef(ref_a), ty::UpvarCapture::ByRef(ref_b)) => {
ref_a.kind == ref_b.kind
}
_ => false,
};

if eq_capture_kind {
match (capture_info_a.expr_id, capture_info_b.expr_id) {
(Some(_), _) | (None, None) => capture_info_a,
(None, Some(_)) => capture_info_b,
}
} else {
match (capture_info_a.capture_kind, capture_info_b.capture_kind) {
(ty::UpvarCapture::ByValue(_), _) => capture_info_a,
(_, ty::UpvarCapture::ByValue(_)) => capture_info_b,
(ty::UpvarCapture::ByRef(ref_a), ty::UpvarCapture::ByRef(ref_b)) => {
match (ref_a.kind, ref_b.kind) {
// Take LHS:
(ty::UniqueImmBorrow | ty::MutBorrow, ty::ImmBorrow)
| (ty::MutBorrow, ty::UniqueImmBorrow) => capture_info_a,

// Take RHS:
(ty::ImmBorrow, ty::UniqueImmBorrow | ty::MutBorrow)
| (ty::UniqueImmBorrow, ty::MutBorrow) => capture_info_b,

(ty::ImmBorrow, ty::ImmBorrow)
| (ty::UniqueImmBorrow, ty::UniqueImmBorrow)
| (ty::MutBorrow, ty::MutBorrow) => {
bug!("Expected unequal capture kinds");
}
}
}
}
}
}

fn log_closure_capture_info(
&self,
closure_def_id: rustc_hir::def_id::DefId,
Expand Down Expand Up @@ -617,8 +562,9 @@ struct InferBorrowKind<'a, 'tcx> {
// variable access that caused us to do so.
current_origin: Option<(Span, Symbol)>,

/// For each Place that we access, we track the minimal kind of
/// For each Place that is captured by the closure, we track the minimal kind of
/// access we need (ref, ref mut, move, etc) and the expression that resulted in such access.
///
/// Consider closure where s.str1 is captured via an ImmutableBorrow and
/// s.str2 via a MutableBorrow
///
Expand Down Expand Up @@ -686,7 +632,7 @@ impl<'a, 'tcx> InferBorrowKind<'a, 'tcx> {
};

let curr_info = self.capture_information[&place_with_id.place];
let updated_info = self.fcx.determine_capture_info(curr_info, capture_info);
let updated_info = determine_capture_info(curr_info, capture_info);

self.capture_information[&place_with_id.place] = updated_info;
}
Expand Down Expand Up @@ -804,7 +750,7 @@ impl<'a, 'tcx> InferBorrowKind<'a, 'tcx> {
expr_id: Some(diag_expr_id),
capture_kind: ty::UpvarCapture::ByRef(new_upvar_borrow),
};
let updated_info = self.fcx.determine_capture_info(curr_capture_info, capture_info);
let updated_info = determine_capture_info(curr_capture_info, capture_info);
self.capture_information[&place_with_id.place] = updated_info;
};
}
Expand Down Expand Up @@ -859,14 +805,14 @@ impl<'a, 'tcx> InferBorrowKind<'a, 'tcx> {
if let PlaceBase::Upvar(upvar_id) = place_with_id.place.base {
assert_eq!(self.closure_def_id.expect_local(), upvar_id.closure_expr_id);

debug!("Capturing new place {:?}", place_with_id);

let capture_kind =
self.fcx.init_capture_kind(self.capture_clause, upvar_id, self.closure_span);

let expr_id = Some(diag_expr_id);
let capture_info = ty::CaptureInfo { expr_id, capture_kind };

debug!("Capturing new place {:?}, capture_info={:?}", place_with_id, capture_info);

self.capture_information.insert(place_with_id.place.clone(), capture_info);
} else {
debug!("Not upvar: {:?}", place_with_id);
Expand Down Expand Up @@ -964,6 +910,92 @@ fn var_name(tcx: TyCtxt<'_>, var_hir_id: hir::HirId) -> Symbol {
tcx.hir().name(var_hir_id)
}

/// Helper function to determine if we need to escalate CaptureKind from
/// CaptureInfo A to B and returns the escalated CaptureInfo.
/// (Note: CaptureInfo contains CaptureKind and an expression that led to capture it in that way)
///
/// If both `CaptureKind`s are considered equivalent, then the CaptureInfo is selected based
/// on the `CaptureInfo` containing an associated expression id.
///
/// If both the CaptureKind and Expression are considered to be equivalent,
/// then `CaptureInfo` A is preferred. This can be useful in cases where we want to priortize
/// expressions reported back to the user as part of diagnostics based on which appears earlier
/// in the closure. This can be acheived simply by calling
/// `determine_capture_info(existing_info, current_info)`. This works out because the
/// expressions that occur earlier in the closure body than the current expression are processed before.
/// Consider the following example
/// ```rust
/// let mut p: Point { x: 10, y: 10 };
///
/// let c = || {
/// p.x += 10;
/// // ^ E1 ^
/// // ...
/// // More code
/// // ...
/// p.x += 10; // E2
/// // ^ E2 ^
/// }
/// ```
/// `CaptureKind` associated with both `E1` and `E2` will be ByRef(MutBorrow),
/// and both have an expression associated, however for diagnostics we prfer reporting
/// `E1` since it appears earlier in the closure body. When `E2` is being processed we
/// would've already handled `E1`, and have an existing capture_information for it.
/// Calling `determine_capture_info(existing_info_e1, current_info_e2)` will return
/// `existing_info_e1` in this case, allowing us to point to `E1` in case of diagnostics.
fn determine_capture_info(
capture_info_a: ty::CaptureInfo<'tcx>,
capture_info_b: ty::CaptureInfo<'tcx>,
) -> ty::CaptureInfo<'tcx> {
// If the capture kind is equivalent then, we don't need to escalate and can compare the
// expressions.
let eq_capture_kind = match (capture_info_a.capture_kind, capture_info_b.capture_kind) {
(ty::UpvarCapture::ByValue(_), ty::UpvarCapture::ByValue(_)) => {
// We don't need to worry about the spans being ignored here.
//
// The expr_id in capture_info corresponds to the span that is stored within
// ByValue(span) and therefore it gets handled with priortizing based on
// expressions below.
true
}
(ty::UpvarCapture::ByRef(ref_a), ty::UpvarCapture::ByRef(ref_b)) => {
ref_a.kind == ref_b.kind
}
(ty::UpvarCapture::ByValue(_), _) | (ty::UpvarCapture::ByRef(_), _) => false,
};

if eq_capture_kind {
match (capture_info_a.expr_id, capture_info_b.expr_id) {
(Some(_), _) | (None, None) => capture_info_a,
(None, Some(_)) => capture_info_b,
}
} else {
// We select the CaptureKind which ranks higher based the following priority order:
// ByValue > MutBorrow > UniqueImmBorrow > ImmBorrow
match (capture_info_a.capture_kind, capture_info_b.capture_kind) {
(ty::UpvarCapture::ByValue(_), _) => capture_info_a,
(_, ty::UpvarCapture::ByValue(_)) => capture_info_b,
(ty::UpvarCapture::ByRef(ref_a), ty::UpvarCapture::ByRef(ref_b)) => {
match (ref_a.kind, ref_b.kind) {
// Take LHS:
(ty::UniqueImmBorrow | ty::MutBorrow, ty::ImmBorrow)
| (ty::MutBorrow, ty::UniqueImmBorrow) => capture_info_a,

// Take RHS:
(ty::ImmBorrow, ty::UniqueImmBorrow | ty::MutBorrow)
| (ty::UniqueImmBorrow, ty::MutBorrow) => capture_info_b,

(ty::ImmBorrow, ty::ImmBorrow)
| (ty::UniqueImmBorrow, ty::UniqueImmBorrow)
| (ty::MutBorrow, ty::MutBorrow) => {
bug!("Expected unequal capture kinds");
}
}
}
}
}
}

/// Determines the Ancestry relationship of Place A relative to Place B
///
/// `PlaceAncestryRelation::Ancestor` implies Place A is ancestor of Place B
Expand Down

0 comments on commit c2deba7

Please sign in to comment.