Skip to content

2229: Handle patterns within closures correctly when capture_disjoint_fields is enabled #82536

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Mar 16, 2021
Merged

2229: Handle patterns within closures correctly when capture_disjoint_fields is enabled #82536

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ec10b71
Introduce new fake reads
roxelo Feb 3, 2021
d4f8729
Delay use of Place in favor of PlaceBuilder
roxelo Feb 19, 2021
b6cf070
Attempt to deal with nested closures properly
roxelo Feb 21, 2021
685a4c6
Use the correct FakeReadCause
roxelo Feb 23, 2021
74fc643
Only borrow place for matching under specific conditions
roxelo Feb 24, 2021
fb3b77a
Add fake_read() to clippy
roxelo Feb 25, 2021
22eaffe
Add comments with examples and tests
roxelo Feb 25, 2021
189d206
Fix error after rebase
roxelo Mar 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions compiler/rustc_middle/src/ty/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ use rustc_hir::{
};
use rustc_index::vec::{Idx, IndexVec};
use rustc_macros::HashStable;
use rustc_middle::mir::FakeReadCause;
use rustc_serialize::opaque::{FileEncodeResult, FileEncoder};
use rustc_session::config::{BorrowckMode, CrateType, OutputFilenames};
use rustc_session::lint::{Level, Lint};
Expand Down Expand Up @@ -430,6 +431,30 @@ pub struct TypeckResults<'tcx> {
/// see `MinCaptureInformationMap` for more details.
pub closure_min_captures: ty::MinCaptureInformationMap<'tcx>,

/// Tracks the fake reads required for a closure and the reason for the fake read.
/// When performing pattern matching for closures, there are times we don't end up
/// reading places that are mentioned in a closure (because of _ patterns). However,
/// to ensure the places are initialized, we introduce fake reads.
/// Consider these two examples:
/// ``` (discriminant matching with only wildcard arm)
/// let x: u8;
/// let c = || match x { _ => () };
/// ```
/// In this example, we don't need to actually read/borrow `x` in `c`, and so we don't
/// want to capture it. However, we do still want an error here, because `x` should have
/// to be initialized at the point where c is created. Therefore, we add a "fake read"
/// instead.
/// ``` (destructured assignments)
/// let c = || {
/// let (t1, t2) = t;
/// }
/// ```
/// In the second example, we capture the disjoint fields of `t` (`t.0` & `t.1`), but
/// we never capture `t`. This becomes an issue when we build MIR as we require
/// information on `t` in order to create place `t.0` and `t.1`. We can solve this
/// issue by fake reading `t`.
pub closure_fake_reads: FxHashMap<DefId, Vec<(HirPlace<'tcx>, FakeReadCause, hir::HirId)>>,

/// Stores the type, expression, span and optional scope span of all types
/// that are live across the yield of this generator (if a generator).
pub generator_interior_types: ty::Binder<Vec<GeneratorInteriorTypeCause<'tcx>>>,
Expand Down Expand Up @@ -464,6 +489,7 @@ impl<'tcx> TypeckResults<'tcx> {
concrete_opaque_types: Default::default(),
closure_captures: Default::default(),
closure_min_captures: Default::default(),
closure_fake_reads: Default::default(),
generator_interior_types: ty::Binder::dummy(Default::default()),
treat_byte_string_as_slice: Default::default(),
}
Expand Down Expand Up @@ -715,6 +741,7 @@ impl<'a, 'tcx> HashStable<StableHashingContext<'a>> for TypeckResults<'tcx> {
ref concrete_opaque_types,
ref closure_captures,
ref closure_min_captures,
ref closure_fake_reads,
ref generator_interior_types,
ref treat_byte_string_as_slice,
} = *self;
Expand Down Expand Up @@ -750,6 +777,7 @@ impl<'a, 'tcx> HashStable<StableHashingContext<'a>> for TypeckResults<'tcx> {
concrete_opaque_types.hash_stable(hcx, hasher);
closure_captures.hash_stable(hcx, hasher);
closure_min_captures.hash_stable(hcx, hasher);
closure_fake_reads.hash_stable(hcx, hasher);
generator_interior_types.hash_stable(hcx, hasher);
treat_byte_string_as_slice.hash_stable(hcx, hasher);
})
Expand Down
80 changes: 54 additions & 26 deletions compiler/rustc_mir_build/src/build/expr/as_place.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,15 @@ use rustc_middle::hir::place::ProjectionKind as HirProjectionKind;
use rustc_middle::middle::region;
use rustc_middle::mir::AssertKind::BoundsCheck;
use rustc_middle::mir::*;
use rustc_middle::ty::AdtDef;
use rustc_middle::ty::{self, CanonicalUserTypeAnnotation, Ty, TyCtxt, Variance};
use rustc_span::Span;
use rustc_target::abi::VariantIdx;

use rustc_index::vec::Idx;

/// The "outermost" place that holds this value.
#[derive(Copy, Clone)]
#[derive(Copy, Clone, Debug, PartialEq)]
crate enum PlaceBase {
/// Denotes the start of a `Place`.
Local(Local),
Expand Down Expand Up @@ -67,7 +68,7 @@ crate enum PlaceBase {
///
/// This is used internally when building a place for an expression like `a.b.c`. The fields `b`
/// and `c` can be progressively pushed onto the place builder that is created when converting `a`.
#[derive(Clone)]
#[derive(Clone, Debug, PartialEq)]
crate struct PlaceBuilder<'tcx> {
base: PlaceBase,
projection: Vec<PlaceElem<'tcx>>,
Expand All @@ -83,20 +84,23 @@ fn convert_to_hir_projections_and_truncate_for_capture<'tcx>(
mir_projections: &[PlaceElem<'tcx>],
) -> Vec<HirProjectionKind> {
let mut hir_projections = Vec::new();
let mut variant = None;

for mir_projection in mir_projections {
let hir_projection = match mir_projection {
ProjectionElem::Deref => HirProjectionKind::Deref,
ProjectionElem::Field(field, _) => {
// We will never encouter this for multivariant enums,
// read the comment for `Downcast`.
HirProjectionKind::Field(field.index() as u32, VariantIdx::new(0))
let variant = variant.unwrap_or(VariantIdx::new(0));
HirProjectionKind::Field(field.index() as u32, variant)
}
ProjectionElem::Downcast(..) => {
// This projections exist only for enums that have
// multiple variants. Since such enums that are captured
// completely, we can stop here.
break;
ProjectionElem::Downcast(.., idx) => {
// We don't expect to see multi-variant enums here, as earlier
// phases will have truncated them already. However, there can
// still be downcasts, thanks to single-variant enums.
// We keep track of VariantIdx so we can use this information
// if the next ProjectionElem is a Field.
variant = Some(*idx);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So-- I'm a bit confused by this change. Are we trying to truncate for multi-variant enums? Do we expect multi-variant enums to show up in this code path?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we capture a multi-variant enum, we will do it in entierty in typchk, i.e. they will be trunacted by the time we get here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though i think the comment should be something like, "We capture multi-varirant enums completely, but we might see a downcast projection in case of single variant enums, so we need to account for it here."

We just happen to be accounting for it more generally. Since we search for ancestors in the list of captures of the place returned by this function, I think its fine to do what we are doing here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the "we capture multi-variant enums completely" is actually sort of confusingly phrased. I would say something like:

We don't expect to see multi-variant enums here, as earlier phases will have truncated them already. However, there can still be downcasts, thanks to single-variant enums.

If this is correct, can we add an assertion that idx is 0?

Copy link
Contributor

@arora-aman arora-aman Mar 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think that is quite right. We are going from a MIR place -> HIR place. For all the HIR places we have in our capture set, we would stop at the multivairant enum and not capture anything on top of it. However the MIR place might be for something precise, eg:

struct Foo(String, String)
x: Option<Foo>
let c = || match x {
Some(Foo(, y)) => // Upvar(x), [downcast(0), field(0), field(1)]
// ...

The ideal thing to do here would really be to look at the type before a downcast projection and see if it's a multi-variant enum and break, but that is a little hard to do here -- because we don't know the actual place since we don't know the capture index, and hence can't get the type. We can make it work by checking if we know any anscetors (in our capture set) of the partially converted place. This sort of makes the code hard to follow and somewhat cyclic in logic where to find a valid ancestor we need to convert the mir place into some hir place and to do that we need to find a valid ancestor.
EDIT: Though this is like base case of a recursion.

On the otherhand, the field projection in HirPlace is defined as Field(field_idx, variant_idx) which as I understand is Downcast to variant_idx and then read the field with index field_idx. Which is cleaner to express, somewhat more precise and not an incorrect expression of what is happening here. We can then make the function that compares two places to check for the ancestor relation to handle this case properly, and since our capture rules don't really capture multivariant enums it doesn't have to do much for it.
EDIT: Not sure if this is any better to follow for a person who is not aware about these portions of the codebase

Another thing to note here is that a multivariant enum has a variant with index 0, so checking for 0 is not an "if and only if" like check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we agree on this approach to thinking about the problem, we would need to update the comments to reflect this

continue;
}
ProjectionElem::Index(..)
| ProjectionElem::ConstantIndex { .. }
Expand All @@ -106,7 +110,7 @@ fn convert_to_hir_projections_and_truncate_for_capture<'tcx>(
break;
}
};

variant = None;
hir_projections.push(hir_projection);
}

Expand Down Expand Up @@ -194,12 +198,12 @@ fn find_capture_matching_projections<'a, 'tcx>(
/// Takes a PlaceBuilder and resolves the upvar (if any) within it, so that the
/// `PlaceBuilder` now starts from `PlaceBase::Local`.
///
/// Returns a Result with the error being the HirId of the Upvar that was not found.
/// Returns a Result with the error being the PlaceBuilder (`from_builder`) that was not found.
fn to_upvars_resolved_place_builder<'a, 'tcx>(
from_builder: PlaceBuilder<'tcx>,
tcx: TyCtxt<'tcx>,
typeck_results: &'a ty::TypeckResults<'tcx>,
) -> Result<PlaceBuilder<'tcx>, HirId> {
) -> Result<PlaceBuilder<'tcx>, PlaceBuilder<'tcx>> {
match from_builder.base {
PlaceBase::Local(_) => Ok(from_builder),
PlaceBase::Upvar { var_hir_id, closure_def_id, closure_kind } => {
Expand Down Expand Up @@ -230,13 +234,12 @@ fn to_upvars_resolved_place_builder<'a, 'tcx>(
from_builder.projection
)
} else {
// FIXME(project-rfc-2229#24): Handle this case properly
debug!(
"No associated capture found for {:?}[{:#?}]",
var_hir_id, from_builder.projection,
);
}
return Err(var_hir_id);
return Err(from_builder);
};

let closure_ty = typeck_results
Expand Down Expand Up @@ -300,6 +303,25 @@ impl<'tcx> PlaceBuilder<'tcx> {
to_upvars_resolved_place_builder(self, tcx, typeck_results).unwrap()
}

/// Attempts to resolve the `PlaceBuilder`.
/// On success, it will return the resolved `PlaceBuilder`.
/// On failure, it will return itself.
///
/// Upvars resolve may fail for a `PlaceBuilder` when attempting to
/// resolve a disjoint field whose root variable is not captured
/// (destructured assignments) or when attempting to resolve a root
/// variable (discriminant matching with only wildcard arm) that is
/// not captured. This can happen because the final mir that will be
/// generated doesn't require a read for this place. Failures will only
/// happen inside closures.
crate fn try_upvars_resolved<'a>(
self,
tcx: TyCtxt<'tcx>,
typeck_results: &'a ty::TypeckResults<'tcx>,
) -> Result<PlaceBuilder<'tcx>, PlaceBuilder<'tcx>> {
to_upvars_resolved_place_builder(self, tcx, typeck_results)
}

crate fn base(&self) -> PlaceBase {
self.base
}
Expand All @@ -308,15 +330,22 @@ impl<'tcx> PlaceBuilder<'tcx> {
self.project(PlaceElem::Field(f, ty))
}

fn deref(self) -> Self {
crate fn deref(self) -> Self {
self.project(PlaceElem::Deref)
}

crate fn downcast(self, adt_def: &'tcx AdtDef, variant_index: VariantIdx) -> Self {
self.project(PlaceElem::Downcast(
Some(adt_def.variants[variant_index].ident.name),
variant_index,
))
}

fn index(self, index: Local) -> Self {
self.project(PlaceElem::Index(index))
}

fn project(mut self, elem: PlaceElem<'tcx>) -> Self {
crate fn project(mut self, elem: PlaceElem<'tcx>) -> Self {
self.projection.push(elem);
self
}
Expand Down Expand Up @@ -602,13 +631,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
// The "retagging" transformation (for Stacked Borrows) relies on this.
let idx = unpack!(block = self.as_temp(block, temp_lifetime, index, Mutability::Not,));

block = self.bounds_check(
block,
base_place.clone().into_place(self.tcx, self.typeck_results),
idx,
expr_span,
source_info,
);
block = self.bounds_check(block, base_place.clone(), idx, expr_span, source_info);

if is_outermost_index {
self.read_fake_borrows(block, fake_borrow_temps, source_info)
Expand All @@ -629,7 +652,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
fn bounds_check(
&mut self,
block: BasicBlock,
slice: Place<'tcx>,
slice: PlaceBuilder<'tcx>,
index: Local,
expr_span: Span,
source_info: SourceInfo,
Expand All @@ -641,7 +664,12 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let lt = self.temp(bool_ty, expr_span);

// len = len(slice)
self.cfg.push_assign(block, source_info, len, Rvalue::Len(slice));
self.cfg.push_assign(
block,
source_info,
len,
Rvalue::Len(slice.into_place(self.tcx, self.typeck_results)),
);
// lt = idx < len
self.cfg.push_assign(
block,
Expand Down
38 changes: 37 additions & 1 deletion compiler/rustc_mir_build/src/build/expr/as_rvalue.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use crate::build::{BlockAnd, BlockAndExtension, Builder};
use crate::thir::*;
use rustc_middle::middle::region;
use rustc_middle::mir::AssertKind;
use rustc_middle::mir::Place;
use rustc_middle::mir::*;
use rustc_middle::ty::{self, Ty, UpvarSubsts};
use rustc_span::Span;
Expand Down Expand Up @@ -164,7 +165,41 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {

block.and(Rvalue::Aggregate(box AggregateKind::Tuple, fields))
}
ExprKind::Closure { closure_id, substs, upvars, movability } => {
ExprKind::Closure { closure_id, substs, upvars, movability, ref fake_reads } => {
// Convert the closure fake reads, if any, from `ExprRef` to mir `Place`
// and push the fake reads.
// This must come before creating the operands. This is required in case
// there is a fake read and a borrow of the same path, since otherwise the
// fake read might interfere with the borrow. Consider an example like this
// one:
// ```
// let mut x = 0;
// let c = || {
// &mut x; // mutable borrow of `x`
// match x { _ => () } // fake read of `x`
// };
// ```
// FIXME(RFC2229): Remove feature gate once diagnostics are improved
if this.tcx.features().capture_disjoint_fields {
for (thir_place, cause, hir_id) in fake_reads.into_iter() {
let place_builder =
unpack!(block = this.as_place_builder(block, thir_place));

if let Ok(place_builder_resolved) =
place_builder.try_upvars_resolved(this.tcx, this.typeck_results)
{
let mir_place =
place_builder_resolved.into_place(this.tcx, this.typeck_results);
this.cfg.push_fake_read(
block,
this.source_info(this.tcx.hir().span(*hir_id)),
*cause,
mir_place,
);
}
}
}

// see (*) above
let operands: Vec<_> = upvars
.into_iter()
Expand Down Expand Up @@ -203,6 +238,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
}
})
.collect();

let result = match substs {
UpvarSubsts::Generator(substs) => {
// We implicitly set the discriminant to 0. See
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_build/src/build/expr/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@

mod as_constant;
mod as_operand;
mod as_place;
pub mod as_place;
mod as_rvalue;
mod as_temp;
mod category;
Expand Down
Loading