Skip to content

Commit

Permalink
Auto merge of #80624 - RalfJung:place-ref, r=oli-obk
Browse files Browse the repository at this point in the history 
use PlaceRef more consistently instead of loosely coupled local+projection

Instead of working directly with the `projections` array, use `iter_projections` and `last_projection`. This avoids having to construct new `PlaceRef` from the pieces everywhere.

I only did this for a few files, to see how people think about this. If y'all are happy with this, I'll open an E-mentor issue to complete this. I grepped for `Place::ty_from` to find the places that need adjusting -- this could miss some, but I am not sure what else to grep for.
  • Loading branch information
@bors
bors committed Jan 4, 2021
2 parents 9a22f99 + afa7408 commit 61f5a00
Show file tree
Hide file tree
Showing 6 changed files with 61 additions and 111 deletions.
11 changes: 3 additions & 8 deletions compiler/rustc_codegen_ssa/src/mir/analyze.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ impl<Bx: BuilderMethods<'a, 'tcx>> LocalAnalyzer<'mir, 'a, 'tcx, Bx> {
) {
let cx = self.fx.cx;

if let &[ref proj_base @ .., elem] = place_ref.projection {
if let Some((place_base, elem)) = place_ref.last_projection() {
let mut base_context = if context.is_mutating_use() {
PlaceContext::MutatingUse(MutatingUseContext::Projection)
} else {
Expand All @@ -119,8 +119,7 @@ impl<Bx: BuilderMethods<'a, 'tcx>> LocalAnalyzer<'mir, 'a, 'tcx, Bx> {
)
);
if is_consume {
let base_ty =
mir::Place::ty_from(place_ref.local, proj_base, self.fx.mir, cx.tcx());
let base_ty = mir::PlaceRef::ty(&place_base, self.fx.mir, cx.tcx());
let base_ty = self.fx.monomorphize(base_ty);

// ZSTs don't require any actual memory access.
Expand Down Expand Up @@ -175,11 +174,7 @@ impl<Bx: BuilderMethods<'a, 'tcx>> LocalAnalyzer<'mir, 'a, 'tcx, Bx> {
base_context = context;
}

self.process_place(
&mir::PlaceRef { local: place_ref.local, projection: proj_base },
base_context,
location,
);
self.process_place(&place_base, base_context, location);
// HACK(eddyb) this emulates the old `visit_projection_elem`, this
// entire `visit_place`-like `process_place` method should be rewritten,
// now that we have moved to the "slice of projections" representation.
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_ssa/src/mir/place.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -514,7 +514,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {

pub fn monomorphized_place_ty(&self, place_ref: mir::PlaceRef<'tcx>) -> Ty<'tcx> {
let tcx = self.cx.tcx();
let place_ty = mir::Place::ty_from(place_ref.local, place_ref.projection, self.mir, tcx);
let place_ty = mir::PlaceRef::ty(&place_ref, self.mir, tcx);
self.monomorphize(place_ty.ty)
}
}
21 changes: 12 additions & 9 deletions compiler/rustc_middle/src/mir/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1745,18 +1745,14 @@ impl<'tcx> Place<'tcx> {

/// Finds the innermost `Local` from this `Place`, *if* it is either a local itself or
/// a single deref of a local.
//
// FIXME: can we safely swap the semantics of `fn base_local` below in here instead?
#[inline(always)]
pub fn local_or_deref_local(&self) -> Option<Local> {
match self.as_ref() {
PlaceRef { local, projection: [] }
| PlaceRef { local, projection: [ProjectionElem::Deref] } => Some(local),
_ => None,
}
self.as_ref().local_or_deref_local()
}

/// If this place represents a local variable like `_X` with no
/// projections, return `Some(_X)`.
#[inline(always)]
pub fn as_local(&self) -> Option<Local> {
self.as_ref().as_local()
}
Expand All @@ -1770,6 +1766,7 @@ impl<'tcx> Place<'tcx> {
/// As a concrete example, given the place a.b.c, this would yield:
/// - (a, .b)
/// - (a.b, .c)
///
/// Given a place without projections, the iterator is empty.
pub fn iter_projections(
self,
Expand All @@ -1790,8 +1787,6 @@ impl From<Local> for Place<'_> {
impl<'tcx> PlaceRef<'tcx> {
/// Finds the innermost `Local` from this `Place`, *if* it is either a local itself or
/// a single deref of a local.
//
// FIXME: can we safely swap the semantics of `fn base_local` below in here instead?
pub fn local_or_deref_local(&self) -> Option<Local> {
match *self {
PlaceRef { local, projection: [] }
Expand All @@ -1808,6 +1803,14 @@ impl<'tcx> PlaceRef<'tcx> {
_ => None,
}
}

pub fn last_projection(&self) -> Option<(PlaceRef<'tcx>, PlaceElem<'tcx>)> {
if let &[ref proj_base @ .., elem] = self.projection {
Some((PlaceRef { local: self.local, projection: proj_base }, elem))
} else {
None
}
}
}

impl Debug for Place<'_> {
Expand Down
38 changes: 10 additions & 28 deletions compiler/rustc_mir/src/borrow_check/diagnostics/conflict_errors.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -293,9 +293,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
);
}

let ty =
Place::ty_from(used_place.local, used_place.projection, self.body, self.infcx.tcx)
.ty;
let ty = PlaceRef::ty(&used_place, self.body, self.infcx.tcx).ty;
let needs_note = match ty.kind() {
ty::Closure(id, _) => {
let tables = self.infcx.tcx.typeck(id.expect_local());
Expand Down Expand Up @@ -732,8 +730,8 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
) -> (String, String, String, String) {
// Define a small closure that we can use to check if the type of a place
// is a union.
let union_ty = |place_base, place_projection| {
let ty = Place::ty_from(place_base, place_projection, self.body, self.infcx.tcx).ty;
let union_ty = |place_base| {
let ty = PlaceRef::ty(&place_base, self.body, self.infcx.tcx).ty;
ty.ty_adt_def().filter(|adt| adt.is_union()).map(|_| ty)
};

Expand All @@ -751,15 +749,10 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
// field access to a union. If we find that, then we will keep the place of the
// union being accessed and the field that was being accessed so we can check the
// second borrowed place for the same union and a access to a different field.
let Place { local, projection } = first_borrowed_place;

let mut cursor = projection.as_ref();
while let [proj_base @ .., elem] = cursor {
cursor = proj_base;

for (place_base, elem) in first_borrowed_place.iter_projections().rev() {
match elem {
ProjectionElem::Field(field, _) if union_ty(local, proj_base).is_some() => {
return Some((PlaceRef { local, projection: proj_base }, field));
ProjectionElem::Field(field, _) if union_ty(place_base).is_some() => {
return Some((place_base, field));
}
_ => {}
}
Expand All @@ -769,23 +762,12 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
.and_then(|(target_base, target_field)| {
// With the place of a union and a field access into it, we traverse the second
// borrowed place and look for a access to a different field of the same union.
let Place { local, ref projection } = second_borrowed_place;

let mut cursor = &projection[..];
while let [proj_base @ .., elem] = cursor {
cursor = proj_base;

for (place_base, elem) in second_borrowed_place.iter_projections().rev() {
if let ProjectionElem::Field(field, _) = elem {
if let Some(union_ty) = union_ty(local, proj_base) {
if field != target_field
&& local == target_base.local
&& proj_base == target_base.projection
{
if let Some(union_ty) = union_ty(place_base) {
if field != target_field && place_base == target_base {
return Some((
self.describe_any_place(PlaceRef {
local,
projection: proj_base,
}),
self.describe_any_place(place_base),
self.describe_any_place(first_borrowed_place.as_ref()),
self.describe_any_place(second_borrowed_place.as_ref()),
union_ty.to_string(),
Expand Down
74 changes: 24 additions & 50 deletions compiler/rustc_mir/src/borrow_check/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1740,20 +1740,18 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {

self.check_if_full_path_is_moved(location, desired_action, place_span, flow_state);

if let [base_proj @ .., ProjectionElem::Subslice { from, to, from_end: false }] =
place_span.0.projection
if let Some((place_base, ProjectionElem::Subslice { from, to, from_end: false })) =
place_span.0.last_projection()
{
let place_ty =
Place::ty_from(place_span.0.local, base_proj, self.body(), self.infcx.tcx);
let place_ty = PlaceRef::ty(&place_base, self.body(), self.infcx.tcx);
if let ty::Array(..) = place_ty.ty.kind() {
let array_place = PlaceRef { local: place_span.0.local, projection: base_proj };
self.check_if_subslice_element_is_moved(
location,
desired_action,
(array_place, place_span.1),
(place_base, place_span.1),
maybe_uninits,
*from,
*to,
from,
to,
);
return;
}
Expand Down Expand Up @@ -1825,10 +1823,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
debug!("check_if_assigned_path_is_moved place: {:?}", place);

// None case => assigning to `x` does not require `x` be initialized.
let mut cursor = &*place.projection.as_ref();
while let [proj_base @ .., elem] = cursor {
cursor = proj_base;

for (place_base, elem) in place.iter_projections().rev() {
match elem {
ProjectionElem::Index(_/*operand*/) |
ProjectionElem::ConstantIndex { .. } |
Expand All @@ -1843,10 +1838,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
ProjectionElem::Deref => {
self.check_if_full_path_is_moved(
location, InitializationRequiringAction::Use,
(PlaceRef {
local: place.local,
projection: proj_base,
}, span), flow_state);
(place_base, span), flow_state);
// (base initialized; no need to
// recur further)
break;
Expand All @@ -1862,15 +1854,12 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
// assigning to `P.f` requires `P` itself
// be already initialized
let tcx = self.infcx.tcx;
let base_ty = Place::ty_from(place.local, proj_base, self.body(), tcx).ty;
let base_ty = PlaceRef::ty(&place_base, self.body(), tcx).ty;
match base_ty.kind() {
ty::Adt(def, _) if def.has_dtor(tcx) => {
self.check_if_path_or_subpath_is_moved(
location, InitializationRequiringAction::Assignment,
(PlaceRef {
local: place.local,
projection: proj_base,
}, span), flow_state);
(place_base, span), flow_state);

// (base initialized; no need to
// recur further)
Expand All @@ -1880,10 +1869,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
// Once `let s; s.x = V; read(s.x);`,
// is allowed, remove this match arm.
ty::Adt(..) | ty::Tuple(..) => {
check_parent_of_field(self, location, PlaceRef {
local: place.local,
projection: proj_base,
}, span, flow_state);
check_parent_of_field(self, location, place_base, span, flow_state);

// rust-lang/rust#21232, #54499, #54986: during period where we reject
// partial initialization, do not complain about unnecessary `mut` on
Expand Down Expand Up @@ -1965,9 +1951,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
// no move out from an earlier location) then this is an attempt at initialization
// of the union - we should error in that case.
let tcx = this.infcx.tcx;
if let ty::Adt(def, _) =
Place::ty_from(base.local, base.projection, this.body(), tcx).ty.kind()
{
if let ty::Adt(def, _) = PlaceRef::ty(&base, this.body(), tcx).ty.kind() {
if def.is_union() {
if this.move_data.path_map[mpi].iter().any(|moi| {
this.move_data.moves[*moi].source.is_predecessor_of(location, this.body)
Expand Down Expand Up @@ -2162,9 +2146,9 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
place: PlaceRef<'tcx>,
is_local_mutation_allowed: LocalMutationIsAllowed,
) -> Result<RootPlace<'tcx>, PlaceRef<'tcx>> {
match place {
PlaceRef { local, projection: [] } => {
let local = &self.body.local_decls[local];
match place.last_projection() {
None => {
let local = &self.body.local_decls[place.local];
match local.mutability {
Mutability::Not => match is_local_mutation_allowed {
LocalMutationIsAllowed::Yes => Ok(RootPlace {
Expand All @@ -2186,11 +2170,10 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
}),
}
}
PlaceRef { local: _, projection: [proj_base @ .., elem] } => {
Some((place_base, elem)) => {
match elem {
ProjectionElem::Deref => {
let base_ty =
Place::ty_from(place.local, proj_base, self.body(), self.infcx.tcx).ty;
let base_ty = PlaceRef::ty(&place_base, self.body(), self.infcx.tcx).ty;

// Check the kind of deref to decide
match base_ty.kind() {
Expand All @@ -2208,10 +2191,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
_ => LocalMutationIsAllowed::Yes,
};

self.is_mutable(
PlaceRef { local: place.local, projection: proj_base },
mode,
)
self.is_mutable(place_base, mode)
}
}
}
Expand All @@ -2229,10 +2209,9 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
}
}
// `Box<T>` owns its content, so mutable if its location is mutable
_ if base_ty.is_box() => self.is_mutable(
PlaceRef { local: place.local, projection: proj_base },
is_local_mutation_allowed,
),
_ if base_ty.is_box() => {
self.is_mutable(place_base, is_local_mutation_allowed)
}
// Deref should only be for reference, pointers or boxes
_ => bug!("Deref of unexpected type: {:?}", base_ty),
}
Expand Down Expand Up @@ -2286,10 +2265,8 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
// });
// }
// ```
let _ = self.is_mutable(
PlaceRef { local: place.local, projection: proj_base },
is_local_mutation_allowed,
)?;
let _ =
self.is_mutable(place_base, is_local_mutation_allowed)?;
Ok(RootPlace {
place_local: place.local,
place_projection: place.projection,
Expand All @@ -2298,10 +2275,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
}
}
} else {
self.is_mutable(
PlaceRef { local: place.local, projection: proj_base },
is_local_mutation_allowed,
)
self.is_mutable(place_base, is_local_mutation_allowed)
}
}
}
Expand Down
Loading

0 comments on commit 61f5a00

Please sign in to comment.