bjoernager/rust
bjoernager/rust
1
Fork 0
Code Activity

Rollup merge of #80092 - sexxi-goose:restrict_precision, r=nikomatsakis

Browse source 
2229: Fix issues with move closures and mutability

This PR fixes two issues when feature `capture_disjoint_fields` is used.

1. Can't mutate using a mutable reference
2. Move closures try to move value out through a reference.

To do so, we
1. Compute the mutability of the capture and store it as part of the `CapturedPlace`  that is written in TypeckResults
2. Restrict capture precision. Note this is temporary for now, to allow the feature to be used with move closures and ByValue captures and might change depending on discussions with the lang team.
    - No Derefs are captured for ByValue captures, since that will result in value behind a reference getting moved.
    - No projections are applied to raw pointers since these require unsafe blocks. We capture
    them completely.

r? `````@nikomatsakis`````
This commit is contained in:
Jonas Schievink 2021-01-31 16:36:41 +01:00 • committed by GitHub
commit 7e3a8ec688
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
33 changed files with 1183 additions and 77 deletions
Download patch file Download diff file Expand all files Collapse all files

19
compiler/rustc_middle/src/ty/mod.rs
View file

@ -661,11 +661,28 @@ pub type RootVariableMinCaptureList<'tcx> = FxIndexMap<hir::HirId, MinCaptureLis
/// Part of `MinCaptureInformationMap`; List of `CapturePlace`s.
pub type MinCaptureList<'tcx> = Vec<CapturedPlace<'tcx>>;
/// A `Place` and the corresponding `CaptureInfo`.
/// A composite describing a `Place` that is captured by a closure.
#[derive(PartialEq, Clone, Debug, TyEncodable, TyDecodable, TypeFoldable, HashStable)]
pub struct CapturedPlace<'tcx> {
/// The `Place` that is captured.
pub place: HirPlace<'tcx>,
/// `CaptureKind` and expression(s) that resulted in such capture of `place`.
pub info: CaptureInfo<'tcx>,
/// Represents if `place` can be mutated or not.
pub mutability: hir::Mutability,
}
impl CapturedPlace<'tcx> {
/// Returns the hir-id of the root variable for the captured place.
/// e.g., if `a.b.c` was captured, would return the hir-id for `a`.
pub fn get_root_variable(&self) -> hir::HirId {
match self.place.base {
HirPlaceBase::Upvar(upvar_id) => upvar_id.var_path.hir_id,
base => bug!("Expected upvar, found={:?}", base),
}
}
}
pub fn place_to_string_for_capture(tcx: TyCtxt<'tcx>, place: &HirPlace<'tcx>) -> String {

2
compiler/rustc_mir/src/borrow_check/diagnostics/mod.rs
View file

@ -215,6 +215,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
PlaceRef { local, projection: [proj_base @ .., elem] } => {
match elem {
ProjectionElem::Deref => {
// FIXME(project-rfc_2229#36): print capture precisely here.
let upvar_field_projection = self.is_upvar_field_projection(place);
if let Some(field) = upvar_field_projection {
let var_index = field.index();
@ -259,6 +260,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
ProjectionElem::Field(field, _ty) => {
autoderef = true;
// FIXME(project-rfc_2229#36): print capture precisely here.
let upvar_field_projection = self.is_upvar_field_projection(place);
if let Some(field) = upvar_field_projection {
let var_index = field.index();

4
compiler/rustc_mir/src/borrow_check/diagnostics/move_errors.rs
View file

@ -345,7 +345,9 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
};
let upvar = &self.upvars[upvar_field.unwrap().index()];
let upvar_hir_id = upvar.var_hir_id;
// FIXME(project-rfc-2229#8): Improve borrow-check diagnostics in case of precise
// capture.
let upvar_hir_id = upvar.place.get_root_variable();
let upvar_name = upvar.name;
let upvar_span = self.infcx.tcx.hir().span(upvar_hir_id);

32
compiler/rustc_mir/src/borrow_check/diagnostics/mutability_errors.rs
View file

@ -64,12 +64,29 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
Place::ty_from(local, proj_base, self.body, self.infcx.tcx).ty
));
item_msg = format!("`{}`", access_place_desc.unwrap());
if self.is_upvar_field_projection(access_place.as_ref()).is_some() {
reason = ", as it is not declared as mutable".to_string();
let imm_borrow_derefed = self.upvars[upvar_index.index()]
.place
.place
.deref_tys()
.any(|ty| matches!(ty.kind(), ty::Ref(.., hir::Mutability::Not)));
// If the place is immutable then:
//
// - Either we deref a immutable ref to get to our final place.
// - We don't capture derefs of raw ptrs
// - Or the final place is immut because the root variable of the capture
// isn't marked mut and we should suggest that to the user.
if imm_borrow_derefed {
// If we deref an immutable ref then the suggestion here doesn't help.
return;
} else {
let name = self.upvars[upvar_index.index()].name;
reason = format!(", as `{}` is not declared as mutable", name);
item_msg = format!("`{}`", access_place_desc.unwrap());
if self.is_upvar_field_projection(access_place.as_ref()).is_some() {
reason = ", as it is not declared as mutable".to_string();
} else {
let name = self.upvars[upvar_index.index()].name;
reason = format!(", as `{}` is not declared as mutable", name);
}
}
}
@ -259,9 +276,12 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
Place::ty_from(local, proj_base, self.body, self.infcx.tcx).ty
));
let captured_place = &self.upvars[upvar_index.index()].place;
err.span_label(span, format!("cannot {ACT}", ACT = act));
let upvar_hir_id = self.upvars[upvar_index.index()].var_hir_id;
let upvar_hir_id = captured_place.get_root_variable();
if let Some(Node::Binding(pat)) = self.infcx.tcx.hir().find(upvar_hir_id) {
if let hir::PatKind::Binding(
hir::BindingAnnotation::Unannotated,

7
compiler/rustc_mir/src/borrow_check/diagnostics/var_name.rs
View file

@ -12,7 +12,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
tcx: TyCtxt<'tcx>,
body: &Body<'tcx>,
local_names: &IndexVec<Local, Option<Symbol>>,
upvars: &[Upvar],
upvars: &[Upvar<'tcx>],
fr: RegionVid,
) -> Option<(Option<Symbol>, Span)> {
debug!("get_var_name_and_span_for_region(fr={:?})", fr);
@ -21,6 +21,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
debug!("get_var_name_and_span_for_region: attempting upvar");
self.get_upvar_index_for_region(tcx, fr)
.map(|index| {
// FIXME(project-rfc-2229#8): Use place span for diagnostics
let (name, span) = self.get_upvar_name_and_span_for_region(tcx, upvars, index);
(Some(name), span)
})
@ -59,10 +60,10 @@ impl<'tcx> RegionInferenceContext<'tcx> {
crate fn get_upvar_name_and_span_for_region(
&self,
tcx: TyCtxt<'tcx>,
upvars: &[Upvar],
upvars: &[Upvar<'tcx>],
upvar_index: usize,
) -> (Symbol, Span) {
let upvar_hir_id = upvars[upvar_index].var_hir_id;
let upvar_hir_id = upvars[upvar_index].place.get_root_variable();
debug!("get_upvar_name_and_span_for_region: upvar_hir_id={:?}", upvar_hir_id);
let upvar_name = tcx.hir().name(upvar_hir_id);

78
compiler/rustc_mir/src/borrow_check/mod.rs
View file

@ -5,11 +5,10 @@ use rustc_data_structures::graph::dominators::Dominators;
use rustc_errors::{Applicability, Diagnostic, DiagnosticBuilder, ErrorReported};
use rustc_hir as hir;
use rustc_hir::def_id::LocalDefId;
use rustc_hir::{HirId, Node};
use rustc_hir::Node;
use rustc_index::bit_set::BitSet;
use rustc_index::vec::IndexVec;
use rustc_infer::infer::{InferCtxt, TyCtxtInferExt};
use rustc_middle::hir::place::PlaceBase as HirPlaceBase;
use rustc_middle::mir::{
traversal, Body, ClearCrossCrate, Local, Location, Mutability, Operand, Place, PlaceElem,
PlaceRef, VarDebugInfoContents,
@ -18,7 +17,7 @@ use rustc_middle::mir::{AggregateKind, BasicBlock, BorrowCheckResult, BorrowKind
use rustc_middle::mir::{Field, ProjectionElem, Promoted, Rvalue, Statement, StatementKind};
use rustc_middle::mir::{InlineAsmOperand, Terminator, TerminatorKind};
use rustc_middle::ty::query::Providers;
use rustc_middle::ty::{self, ParamEnv, RegionVid, TyCtxt};
use rustc_middle::ty::{self, CapturedPlace, ParamEnv, RegionVid, TyCtxt};
use rustc_session::lint::builtin::{MUTABLE_BORROW_RESERVATION_CONFLICT, UNUSED_MUT};
use rustc_span::{Span, Symbol, DUMMY_SP};
@ -73,16 +72,14 @@ crate use region_infer::RegionInferenceContext;
// FIXME(eddyb) perhaps move this somewhere more centrally.
#[derive(Debug)]
crate struct Upvar {
crate struct Upvar<'tcx> {
// FIXME(project-rfc_2229#36): print capture precisely here.
name: Symbol,
// FIXME(project-rfc-2229#8): This should use Place or something similar
var_hir_id: HirId,
place: CapturedPlace<'tcx>,
/// If true, the capture is behind a reference.
by_ref: bool,
mutability: Mutability,
}
const DEREF_PROJECTION: &[PlaceElem<'_>; 1] = &[ProjectionElem::Deref];
@ -161,26 +158,13 @@ fn do_mir_borrowck<'a, 'tcx>(
let upvars: Vec<_> = tables
.closure_min_captures_flattened(def.did.to_def_id())
.map(|captured_place| {
let var_hir_id = match captured_place.place.base {
HirPlaceBase::Upvar(upvar_id) => upvar_id.var_path.hir_id,
_ => bug!("Expected upvar"),
};
let var_hir_id = captured_place.get_root_variable();
let capture = captured_place.info.capture_kind;
let by_ref = match capture {
ty::UpvarCapture::ByValue(_) => false,
ty::UpvarCapture::ByRef(..) => true,
};
let mut upvar = Upvar {
name: tcx.hir().name(var_hir_id),
var_hir_id,
by_ref,
mutability: Mutability::Not,
};
let bm = *tables.pat_binding_modes().get(var_hir_id).expect("missing binding mode");
if bm == ty::BindByValue(hir::Mutability::Mut) {
upvar.mutability = Mutability::Mut;
}
upvar
Upvar { name: tcx.hir().name(var_hir_id), place: captured_place.clone(), by_ref }
})
.collect();
@ -549,7 +533,7 @@ crate struct MirBorrowckCtxt<'cx, 'tcx> {
dominators: Dominators<BasicBlock>,
/// Information about upvars not necessarily preserved in types or MIR
upvars: Vec<Upvar>,
upvars: Vec<Upvar<'tcx>>,
/// Names of local (user) variables (extracted from `var_debug_info`).
local_names: IndexVec<Local, Option<Symbol>>,
@ -1374,13 +1358,38 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
fn propagate_closure_used_mut_upvar(&mut self, operand: &Operand<'tcx>) {
let propagate_closure_used_mut_place = |this: &mut Self, place: Place<'tcx>| {
if !place.projection.is_empty() {
if let Some(field) = this.is_upvar_field_projection(place.as_ref()) {
this.used_mut_upvars.push(field);
}
} else {
this.used_mut.insert(place.local);
// We have three possibilities here:
// a. We are modifying something through a mut-ref
// b. We are modifying something that is local to our parent
// c. Current body is a nested closure, and we are modifying path starting from
// a Place captured by our parent closure.
// Handle (c), the path being modified is exactly the path captured by our parent
if let Some(field) = this.is_upvar_field_projection(place.as_ref()) {
this.used_mut_upvars.push(field);
return;
}
for (place_ref, proj) in place.iter_projections().rev() {
// Handle (a)
if proj == ProjectionElem::Deref {
match place_ref.ty(this.body(), this.infcx.tcx).ty.kind() {
// We aren't modifying a variable directly
ty::Ref(_, _, hir::Mutability::Mut) => return,
_ => {}
}
}
// Handle (c)
if let Some(field) = this.is_upvar_field_projection(place_ref) {
this.used_mut_upvars.push(field);
return;
}
}
// Handle(b)
this.used_mut.insert(place.local);
};
// This relies on the current way that by-value
@ -2146,6 +2155,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
place: PlaceRef<'tcx>,
is_local_mutation_allowed: LocalMutationIsAllowed,
) -> Result<RootPlace<'tcx>, PlaceRef<'tcx>> {
debug!("is_mutable: place={:?}, is_local...={:?}", place, is_local_mutation_allowed);
match place.last_projection() {
None => {
let local = &self.body.local_decls[place.local];
@ -2227,11 +2237,11 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
if let Some(field) = upvar_field_projection {
let upvar = &self.upvars[field.index()];
debug!(
"upvar.mutability={:?} local_mutation_is_allowed={:?} \
place={:?}",
upvar, is_local_mutation_allowed, place
"is_mutable: upvar.mutability={:?} local_mutation_is_allowed={:?} \
place={:?}, place_base={:?}",
upvar, is_local_mutation_allowed, place, place_base
);
match (upvar.mutability, is_local_mutation_allowed) {
match (upvar.place.mutability, is_local_mutation_allowed) {
(
Mutability::Not,
LocalMutationIsAllowed::No

2
compiler/rustc_mir/src/borrow_check/nll.rs
View file

@ -165,7 +165,7 @@ pub(in crate::borrow_check) fn compute_regions<'cx, 'tcx>(
flow_inits: &mut ResultsCursor<'cx, 'tcx, MaybeInitializedPlaces<'cx, 'tcx>>,
move_data: &MoveData<'tcx>,
borrow_set: &BorrowSet<'tcx>,
upvars: &[Upvar],
upvars: &[Upvar<'tcx>],
) -> NllOutput<'tcx> {
let mut all_facts = AllFacts::enabled(infcx.tcx).then_some(AllFacts::default());

2
compiler/rustc_mir/src/borrow_check/path_utils.rs
View file

@ -143,7 +143,7 @@ pub(super) fn borrow_of_local_data(place: Place<'_>) -> bool {
/// of a closure type.
pub(crate) fn is_upvar_field_projection(
tcx: TyCtxt<'tcx>,
upvars: &[Upvar],
upvars: &[Upvar<'tcx>],
place_ref: PlaceRef<'tcx>,
body: &Body<'tcx>,
) -> Option<Field> {

8
compiler/rustc_mir/src/borrow_check/type_check/mod.rs
View file

@ -132,7 +132,7 @@ pub(crate) fn type_check<'mir, 'tcx>(
flow_inits: &mut ResultsCursor<'mir, 'tcx, MaybeInitializedPlaces<'mir, 'tcx>>,
move_data: &MoveData<'tcx>,
elements: &Rc<RegionValueElements>,
upvars: &[Upvar],
upvars: &[Upvar<'tcx>],
) -> MirTypeckResults<'tcx> {
let implicit_region_bound = infcx.tcx.mk_region(ty::ReVar(universal_regions.fr_fn_body));
let mut constraints = MirTypeckRegionConstraints {
@ -821,7 +821,7 @@ struct BorrowCheckContext<'a, 'tcx> {
all_facts: &'a mut Option<AllFacts>,
borrow_set: &'a BorrowSet<'tcx>,
constraints: &'a mut MirTypeckRegionConstraints<'tcx>,
upvars: &'a [Upvar],
upvars: &'a [Upvar<'tcx>],
}
crate struct MirTypeckResults<'tcx> {
@ -2490,7 +2490,9 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
body,
);
let category = if let Some(field) = field {
ConstraintCategory::ClosureUpvar(self.borrowck_context.upvars[field.index()].var_hir_id)
let var_hir_id = self.borrowck_context.upvars[field.index()].place.get_root_variable();
// FIXME(project-rfc-2229#8): Use Place for better diagnostics
ConstraintCategory::ClosureUpvar(var_hir_id)
} else {
ConstraintCategory::Boring
};

11
compiler/rustc_mir_build/src/build/mod.rs
View file

@ -851,22 +851,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
_ => bug!("Expected an upvar")
};
let mut mutability = Mutability::Not;
let mutability = captured_place.mutability;
// FIXME(project-rfc-2229#8): Store more precise information
let mut name = kw::Empty;
if let Some(Node::Binding(pat)) = tcx_hir.find(var_id) {
if let hir::PatKind::Binding(_, _, ident, _) = pat.kind {
name = ident.name;
match hir_typeck_results
.extract_binding_mode(tcx.sess, pat.hir_id, pat.span)
{
Some(ty::BindByValue(hir::Mutability::Mut)) => {
mutability = Mutability::Mut;
}
Some(_) => mutability = Mutability::Not,
_ => {}
}
}
}

131
compiler/rustc_typeck/src/check/upvar.rs
View file

@ -184,10 +184,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let origin = if self.tcx.features().capture_disjoint_fields {
origin
} else {
// FIXME(project-rfc-2229#26): Once rust-lang#80092 is merged, we should restrict the
// precision of origin as well. Otherwise, this will cause issues when project-rfc-2229#26
// is fixed as we might see Index projections in the origin, which we can't print because
// we don't store enough information.
// FIXME(project-rfc-2229#31): Once the changes to support reborrowing are
// made, make sure we are selecting and restricting
// the origin correctly.
(origin.0, Place { projections: vec![], ..origin.1 })
};
@ -252,8 +251,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let capture = captured_place.info.capture_kind;
debug!(
"place={:?} upvar_ty={:?} capture={:?}",
captured_place.place, upvar_ty, capture
"final_upvar_tys: place={:?} upvar_ty={:?} capture={:?}, mutability={:?}",
captured_place.place, upvar_ty, capture, captured_place.mutability,
);
match capture {
@ -419,19 +418,13 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
base => bug!("Expected upvar, found={:?}", base),
};
// Arrays are captured in entirety, drop Index projections and projections
// after Index projections.
let first_index_projection =
place.projections.split(|proj| ProjectionKind::Index == proj.kind).next();
let place = Place {
base_ty: place.base_ty,
base: place.base,
projections: first_index_projection.map_or(Vec::new(), |p| p.to_vec()),
};
let place = restrict_capture_precision(place, capture_info.capture_kind);
let min_cap_list = match root_var_min_capture_list.get_mut(&var_hir_id) {
None => {
let min_cap_list = vec![ty::CapturedPlace { place, info: capture_info }];
let mutability = self.determine_capture_mutability(&place);
let min_cap_list =
vec![ty::CapturedPlace { place, info: capture_info, mutability }];
root_var_min_capture_list.insert(var_hir_id, min_cap_list);
continue;
}
@ -494,8 +487,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
// Only need to insert when we don't have an ancestor in the existing min capture list
if !ancestor_found {
let mutability = self.determine_capture_mutability(&place);
let captured_place =
ty::CapturedPlace { place: place.clone(), info: updated_capture_info };
ty::CapturedPlace { place, info: updated_capture_info, mutability };
min_cap_list.push(captured_place);
}
}
@ -615,6 +609,49 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}
}
}
/// A captured place is mutable if
/// 1. Projections don't include a Deref of an immut-borrow, **and**
/// 2. PlaceBase is mut or projections include a Deref of a mut-borrow.
fn determine_capture_mutability(&self, place: &Place<'tcx>) -> hir::Mutability {
let var_hir_id = match place.base {
PlaceBase::Upvar(upvar_id) => upvar_id.var_path.hir_id,
_ => unreachable!(),
};
let bm = *self
.typeck_results
.borrow()
.pat_binding_modes()
.get(var_hir_id)
.expect("missing binding mode");
let mut is_mutbl = match bm {
ty::BindByValue(mutability) => mutability,
ty::BindByReference(_) => hir::Mutability::Not,
};
for pointer_ty in place.deref_tys() {
match pointer_ty.kind() {
// We don't capture derefs of raw ptrs
ty::RawPtr(_) => unreachable!(),
// Derefencing a mut-ref allows us to mut the Place if we don't deref
// an immut-ref after on top of this.
ty::Ref(.., hir::Mutability::Mut) => is_mutbl = hir::Mutability::Mut,
// The place isn't mutable once we dereference a immutable reference.
ty::Ref(.., hir::Mutability::Not) => return hir::Mutability::Not,
// Dereferencing a box doesn't change mutability
ty::Adt(def, ..) if def.is_box() => {}
unexpected_ty => bug!("deref of unexpected pointer type {:?}", unexpected_ty),
}
}
is_mutbl
}
}
struct InferBorrowKind<'a, 'tcx> {
@ -960,6 +997,66 @@ impl<'a, 'tcx> euv::Delegate<'tcx> for InferBorrowKind<'a, 'tcx> {
}
}
/// Truncate projections so that following rules are obeyed by the captured `place`:
///
/// - No Derefs in move closure, this will result in value behind a reference getting moved.
/// - No projections are applied to raw pointers, since these require unsafe blocks. We capture
/// them completely.
/// - No Index projections are captured, since arrays are captured completely.
fn restrict_capture_precision<'tcx>(
mut place: Place<'tcx>,
capture_kind: ty::UpvarCapture<'tcx>,
) -> Place<'tcx> {
if place.projections.is_empty() {
// Nothing to do here
return place;
}
if place.base_ty.is_unsafe_ptr() {
place.projections.truncate(0);
return place;
}
let mut truncated_length = usize::MAX;
let mut first_deref_projection = usize::MAX;
for (i, proj) in place.projections.iter().enumerate() {
if proj.ty.is_unsafe_ptr() {
// Don't apply any projections on top of an unsafe ptr
truncated_length = truncated_length.min(i + 1);
break;
}
match proj.kind {
ProjectionKind::Index => {
// Arrays are completely captured, so we drop Index projections
truncated_length = truncated_length.min(i);
break;
}
ProjectionKind::Deref => {
// We only drop Derefs in case of move closures
// There might be an index projection or raw ptr ahead, so we don't stop here.
first_deref_projection = first_deref_projection.min(i);
}
ProjectionKind::Field(..) => {} // ignore
ProjectionKind::Subslice => {} // We never capture this
}
}
let length = place
.projections
.len()
.min(truncated_length)
// In case of capture `ByValue` we want to not capture derefs
.min(match capture_kind {
ty::UpvarCapture::ByValue(..) => first_deref_projection,
ty::UpvarCapture::ByRef(..) => usize::MAX,
});
place.projections.truncate(length);
place
}
fn construct_place_string(tcx: TyCtxt<'_>, place: &Place<'tcx>) -> String {
let variable_name = match place.base {
PlaceBase::Upvar(upvar_id) => var_name(tcx, upvar_id.var_path.hir_id).to_string(),