Skip to content

Forbid type ascriptions of place expressions in lvalue contexts #80788

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

Closed
wants to merge 7 commits into from
Closed

Forbid type ascriptions of place expressions in lvalue contexts #80788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
312c9e6
coerce type ascriptions by default
b-naber Jan 6, 2021
d70f5bc
forbid type ascriptions in lvalue contexts
b-naber Jan 6, 2021
d2485ca
add tests
b-naber Jan 7, 2021
ad7ba4d
get rid of duplicate errors
b-naber Feb 11, 2021
40af99d
treat type ascriptions as rvalues
b-naber Feb 11, 2021
f699d41
update tests
b-naber Feb 11, 2021
597cb28
consume subexpr of type ascription in expr_use_visitor
b-naber Feb 11, 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
6 changes: 2 additions & 4 deletions compiler/rustc_hir/src/hir.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1430,10 +1430,8 @@ impl Expr<'_> {
matches!(path.res, Res::Local(..) | Res::Def(DefKind::Static, _) | Res::Err)
}

// Type ascription inherits its place expression kind from its
// operand. See:
// https://github.com/rust-lang/rfcs/blob/master/text/0803-type-ascription.md#type-ascription-and-temporaries
ExprKind::Type(ref e, _) => e.is_place_expr(allow_projections_from),
// We always treat type ascriptions as rvalues
Copy link
Contributor

Choose a reason for hiding this comment

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

probably worth adding a comment here like ", but if a type ascription is used in a reference, it will be an error"

ExprKind::Type(_, _) => false,

ExprKind::Unary(UnOp::UnDeref, _) => true,

Expand Down
7 changes: 7 additions & 0 deletions compiler/rustc_middle/src/hir/map/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -792,6 +792,13 @@ impl<'hir> Map<'hir> {
}
}

pub fn maybe_expr(&self, id: HirId) -> Option<&'hir Expr<'hir>> {
match self.find(id) {
Some(Node::Expr(expr)) => Some(expr),
_ => None,
}
}

pub fn opt_name(&self, id: HirId) -> Option<Symbol> {
Some(match self.get(id) {
Node::Item(i) => i.ident.name,
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/hir/place.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use rustc_target::abi::VariantIdx;
)]
pub enum PlaceBase {
/// A temporary variable.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we extend this comment to indicate what the HirId is?

Rvalue,
Rvalue(HirId),
/// A named `static` item.
StaticItem,
/// A named local variable.
Expand Down
119 changes: 47 additions & 72 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 @@ -6,8 +6,8 @@ use crate::build::{BlockAnd, BlockAndExtension, Builder};
use crate::thir::*;
use rustc_hir::def_id::DefId;
use rustc_hir::HirId;
use rustc_middle::middle::region;
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::{self, CanonicalUserTypeAnnotation, Ty, TyCtxt, Variance};
Expand Down Expand Up @@ -57,7 +57,8 @@ crate enum PlaceBase {
/// DefId of the closure
closure_def_id: DefId,
/// The trait closure implements, `Fn`, `FnMut`, `FnOnce`
closure_kind: ty::ClosureKind },
closure_kind: ty::ClosureKind,
},
}

/// `PlaceBuilder` is used to create places during MIR construction. It allows you to "build up" a
Expand All @@ -81,8 +82,7 @@ crate struct PlaceBuilder<'tcx> {
fn convert_to_hir_projections_and_truncate_for_capture<'tcx>(
mir_projections: &[PlaceElem<'tcx>],
) -> Vec<HirProjectionKind> {

let mut hir_projections = Vec::new();
let mut hir_projections = Vec::new();

for mir_projection in mir_projections {
let hir_projection = match mir_projection {
Expand All @@ -91,20 +91,20 @@ fn convert_to_hir_projections_and_truncate_for_capture<'tcx>(
// We will never encouter this for multivariant enums,
// read the comment for `Downcast`.
HirProjectionKind::Field(field.index() as u32, VariantIdx::new(0))
},
}
ProjectionElem::Downcast(..) => {
// This projections exist only for enums that have
// multiple variants. Since such enums that are captured
// completely, we can stop here.
break
},
break;
}
ProjectionElem::Index(..)
| ProjectionElem::ConstantIndex { .. }
| ProjectionElem::Subslice { .. } => {
// We don't capture array-access projections.
// We can stop here as arrays are captured completely.
break
},
break;
}
};

hir_projections.push(hir_projection);
Expand Down Expand Up @@ -181,9 +181,9 @@ fn find_capture_matching_projections<'a, 'tcx>(
// If an ancestor is found, `idx` is the index within the list of captured places
// for root variable `var_hir_id` and `capture` is the `ty::CapturedPlace` itself.
let (idx, capture) = root_variable_min_captures.iter().enumerate().find(|(_, capture)| {
let possible_ancestor_proj_kinds =
capture.place.projections.iter().map(|proj| proj.kind).collect();
is_ancestor_or_same_capture(&possible_ancestor_proj_kinds, &hir_projections)
let possible_ancestor_proj_kinds =
Copy link
Contributor

Choose a reason for hiding this comment

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

what's going on with these changes :) they seem like random whitespace changes. Are they a side-effect of running rustfmt or something?

capture.place.projections.iter().map(|proj| proj.kind).collect();
is_ancestor_or_same_capture(&possible_ancestor_proj_kinds, &hir_projections)
})?;

// Convert index to be from the presepective of the entire closure_min_captures map
Expand Down Expand Up @@ -213,35 +213,34 @@ fn to_upvars_resolved_place_builder<'a, 'tcx>(
ty::ClosureKind::FnOnce => {}
}

let (capture_index, capture) =
if let Some(capture_details) = find_capture_matching_projections(
let (capture_index, capture) = if let Some(capture_details) =
find_capture_matching_projections(
typeck_results,
var_hir_id,
closure_def_id,
&from_builder.projection,
) {
capture_details
} else {
if !tcx.features().capture_disjoint_fields {
bug!(
"No associated capture found for {:?}[{:#?}] even though \
capture_details
} else {
if !tcx.features().capture_disjoint_fields {
bug!(
"No associated capture found for {:?}[{:#?}] even though \
capture_disjoint_fields isn't enabled",
var_hir_id,
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);
};
var_hir_id,
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);
};

let closure_ty =
typeck_results.node_type(tcx.hir().local_def_id_to_hir_id(closure_def_id.expect_local()));
let closure_ty = typeck_results
.node_type(tcx.hir().local_def_id_to_hir_id(closure_def_id.expect_local()));

let substs = match closure_ty.kind() {
ty::Closure(_, substs) => ty::UpvarSubsts::Closure(substs),
Expand All @@ -256,7 +255,8 @@ fn to_upvars_resolved_place_builder<'a, 'tcx>(
// we know that the capture exists and is the `capture_index`-th capture.
let var_ty = substs.tupled_upvars_ty().tuple_element_ty(capture_index).unwrap();

upvar_resolved_place_builder = upvar_resolved_place_builder.field(Field::new(capture_index), var_ty);
upvar_resolved_place_builder =
upvar_resolved_place_builder.field(Field::new(capture_index), var_ty);

// If the variable is captured via ByRef(Immutable/Mutable) Borrow,
// we need to deref it
Expand All @@ -270,8 +270,9 @@ fn to_upvars_resolved_place_builder<'a, 'tcx>(

// We used some of the projections to build the capture itself,
// now we apply the remaining to the upvar resolved place.
upvar_resolved_place_builder.projection.extend(
curr_projections.drain(next_projection..));
upvar_resolved_place_builder
.projection
.extend(curr_projections.drain(next_projection..));

Ok(upvar_resolved_place_builder)
}
Expand Down Expand Up @@ -356,7 +357,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {

/// This is used when constructing a compound `Place`, so that we can avoid creating
/// intermediate `Place` values until we know the full set of projections.
crate fn as_place_builder<M>(&mut self, block: BasicBlock, expr: M) -> BlockAnd<PlaceBuilder<'tcx>>
crate fn as_place_builder<M>(
&mut self,
block: BasicBlock,
expr: M,
) -> BlockAnd<PlaceBuilder<'tcx>>
where
M: Mirror<'tcx, Output = Expr<'tcx>>,
{
Expand Down Expand Up @@ -456,38 +461,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
block.and(place_builder)
}

ExprKind::PlaceTypeAscription { source, user_ty } => {
let source = this.hir.mirror(source);
let place_builder = unpack!(
block = this.expr_as_place(block, source, mutability, fake_borrow_temps,)
);
if let Some(user_ty) = user_ty {
let annotation_index =
this.canonical_user_type_annotations.push(CanonicalUserTypeAnnotation {
span: source_info.span,
user_ty,
inferred_ty: expr.ty,
});

let place =
place_builder.clone().into_place(this.hir.tcx(), this.hir.typeck_results());
this.cfg.push(
block,
Statement {
source_info,
kind: StatementKind::AscribeUserType(
box (
place,
UserTypeProjection { base: annotation_index, projs: vec![] },
),
Variance::Invariant,
),
},
);
}
block.and(place_builder)
}
ExprKind::ValueTypeAscription { source, user_ty } => {
ExprKind::TypeAscription { source, user_ty } => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Huzzah for code being deleted! 🎉

let source = this.hir.mirror(source);
let temp =
unpack!(block = this.as_temp(block, source.temp_lifetime, source, mutability));
Expand Down Expand Up @@ -626,7 +600,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
if is_outermost_index {
self.read_fake_borrows(block, fake_borrow_temps, source_info)
} else {
base_place = base_place.expect_upvars_resolved(self.hir.tcx(), self.hir.typeck_results());
base_place =
base_place.expect_upvars_resolved(self.hir.tcx(), self.hir.typeck_results());
self.add_fake_borrows_of_base(
&base_place,
block,
Expand Down Expand Up @@ -678,7 +653,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let tcx = self.hir.tcx();
let local = match base_place.base {
PlaceBase::Local(local) => local,
PlaceBase::Upvar { .. } => bug!("Expected PlacseBase::Local found Upvar")
PlaceBase::Upvar { .. } => bug!("Expected PlacseBase::Local found Upvar"),
};

let place_ty = Place::ty_from(local, &base_place.projection, &self.local_decls, tcx);
Expand Down
59 changes: 32 additions & 27 deletions 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 @@ -2,9 +2,9 @@

use rustc_index::vec::Idx;

use crate::build::expr::as_place::PlaceBase;
use crate::build::expr::category::{Category, RvalueFunc};
use crate::build::{BlockAnd, BlockAndExtension, Builder};
use crate::build::expr::as_place::PlaceBase;
use crate::thir::*;
use rustc_middle::middle::region;
use rustc_middle::mir::AssertKind;
Expand Down Expand Up @@ -269,11 +269,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
| ExprKind::Return { .. }
| ExprKind::InlineAsm { .. }
| ExprKind::LlvmInlineAsm { .. }
| ExprKind::PlaceTypeAscription { .. }
| ExprKind::ValueTypeAscription { .. } => {
| ExprKind::TypeAscription { .. } => {
// these do not have corresponding `Rvalue` variants,
// so make an operand and then return that
debug_assert!(!matches!(Category::of(&expr.kind), Some(Category::Rvalue(RvalueFunc::AsRvalue))));
debug_assert!(!matches!(
Category::of(&expr.kind),
Some(Category::Rvalue(RvalueFunc::AsRvalue))
));
let operand = unpack!(block = this.as_operand(block, scope, expr));
block.and(Rvalue::Use(operand))
}
Expand Down Expand Up @@ -400,34 +402,39 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
// We are capturing a path that starts off a local variable in the parent.
// The mutability of the current capture is same as the mutability
// of the local declaration in the parent.
PlaceBase::Local(local) => this.local_decls[local].mutability,
PlaceBase::Local(local) => this.local_decls[local].mutability,
// Parent is a closure and we are capturing a path that is captured
// by the parent itself. The mutability of the current capture
// is same as that of the capture in the parent closure.
PlaceBase::Upvar { .. } => {
let enclosing_upvars_resolved = arg_place_builder.clone().into_place(
this.hir.tcx(),
this.hir.typeck_results());
let enclosing_upvars_resolved =
arg_place_builder.clone().into_place(this.hir.tcx(), this.hir.typeck_results());

match enclosing_upvars_resolved.as_ref() {
PlaceRef { local, projection: &[ProjectionElem::Field(upvar_index, _), ..] }
PlaceRef {
local,
projection: &[ProjectionElem::Field(upvar_index, _), ..],
}
| PlaceRef {
local,
projection: &[ProjectionElem::Deref, ProjectionElem::Field(upvar_index, _), ..] } => {
// Not in a closure
debug_assert!(
local == Local::new(1),
"Expected local to be Local(1), found {:?}",
local
);
// Not in a closure
debug_assert!(
this.upvar_mutbls.len() > upvar_index.index(),
"Unexpected capture place, upvar_mutbls={:#?}, upvar_index={:?}",
this.upvar_mutbls, upvar_index
);
this.upvar_mutbls[upvar_index.index()]
}
projection:
&[ProjectionElem::Deref, ProjectionElem::Field(upvar_index, _), ..],
} => {
// Not in a closure
debug_assert!(
local == Local::new(1),
"Expected local to be Local(1), found {:?}",
local
);
// Not in a closure
debug_assert!(
this.upvar_mutbls.len() > upvar_index.index(),
"Unexpected capture place, upvar_mutbls={:#?}, upvar_index={:?}",
this.upvar_mutbls,
upvar_index
);
this.upvar_mutbls[upvar_index.index()]
}
_ => bug!("Unexpected capture place"),
}
}
Expand All @@ -438,9 +445,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
Mutability::Mut => BorrowKind::Mut { allow_two_phase_borrow: false },
};

let arg_place = arg_place_builder.into_place(
this.hir.tcx(),
this.hir.typeck_results());
let arg_place = arg_place_builder.into_place(this.hir.tcx(), this.hir.typeck_results());

this.cfg.push_assign(
block,
Expand Down
3 changes: 1 addition & 2 deletions compiler/rustc_mir_build/src/build/expr/category.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,7 @@ impl Category {
| ExprKind::Index { .. }
| ExprKind::UpvarRef { .. }
| ExprKind::VarRef { .. }
| ExprKind::PlaceTypeAscription { .. }
| ExprKind::ValueTypeAscription { .. } => Some(Category::Place),
| ExprKind::TypeAscription { .. } => Some(Category::Place),

ExprKind::LogicalOp { .. }
| ExprKind::Match { .. }
Expand Down
Loading