Skip to content

NLL: disallow creation of immediately unusable variables #54188

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.

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 9 commits into from
Sep 22, 2018
5 changes: 4 additions & 1 deletion src/librustc/ich/impls_mir.rs
Original file line number Diff line number Diff line change
@@ -238,7 +238,8 @@ for mir::StatementKind<'gcx> {
place.hash_stable(hcx, hasher);
rvalue.hash_stable(hcx, hasher);
}
mir::StatementKind::ReadForMatch(ref place) => {
mir::StatementKind::FakeRead(ref cause, ref place) => {
cause.hash_stable(hcx, hasher);
place.hash_stable(hcx, hasher);
}
mir::StatementKind::SetDiscriminant { ref place, variant_index } => {
@@ -271,6 +272,8 @@ for mir::StatementKind<'gcx> {
}
}

impl_stable_hash_for!(enum mir::FakeReadCause { ForMatch, ForLet });

impl<'a, 'gcx, T> HashStable<StableHashingContext<'a>>
for mir::ValidationOperand<'gcx, T>
where T: HashStable<StableHashingContext<'a>>
36 changes: 32 additions & 4 deletions src/librustc/mir/mod.rs
Original file line number Diff line number Diff line change
@@ -1613,8 +1613,10 @@ pub enum StatementKind<'tcx> {
Assign(Place<'tcx>, Rvalue<'tcx>),

/// This represents all the reading that a pattern match may do
/// (e.g. inspecting constants and discriminant values).
ReadForMatch(Place<'tcx>),
/// (e.g. inspecting constants and discriminant values), and the
/// kind of pattern it comes from. This is in order to adapt potential
/// error messages to these specific patterns.
FakeRead(FakeReadCause, Place<'tcx>),

/// Write the discriminant for a variant to the enum Place.
SetDiscriminant {
@@ -1662,6 +1664,31 @@ pub enum StatementKind<'tcx> {
Nop,
}

/// The `FakeReadCause` describes the type of pattern why a `FakeRead` statement exists.
#[derive(Copy, Clone, RustcEncodable, RustcDecodable, Debug)]
pub enum FakeReadCause {
/// Inject a fake read of the borrowed input at the start of each arm's
/// pattern testing code.
///
/// This should ensure that you cannot change the variant for an enum
/// while you are in the midst of matching on it.
ForMatch,

/// Officially, the semantics of
///
/// `let pattern = <expr>;`
///
/// is that `<expr>` is evaluated into a temporary and then this temporary is
/// into the pattern.
///
/// However, if we see the simple pattern `let var = <expr>`, we optimize this to
/// evaluate `<expr>` directly into the variable `var`. This is mostly unobservable,
/// but in some cases it can affect the borrow checker, as in #53695.
/// Therefore, we insert a "fake read" here to ensure that we get
/// appropriate errors.
ForLet,
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add some comments and links here.

I think the best option is to move the comments from the point of insertion to here, and then (in those spots) just refer to

// See comments on `FakeReadCause::ForLet

This is also good because we may insert fake read from multiple places, but this centralizes the comment in one clearly identifiable location.

}

/// The `ValidationOp` describes what happens with each of the operands of a
/// `Validate` statement.
#[derive(Copy, Clone, RustcEncodable, RustcDecodable, PartialEq, Eq)]
@@ -1718,7 +1745,7 @@ impl<'tcx> Debug for Statement<'tcx> {
use self::StatementKind::*;
match self.kind {
Assign(ref place, ref rv) => write!(fmt, "{:?} = {:?}", place, rv),
ReadForMatch(ref place) => write!(fmt, "ReadForMatch({:?})", place),
FakeRead(ref cause, ref place) => write!(fmt, "FakeRead({:?}, {:?})", cause, place),
// (reuse lifetime rendering policy from ppaux.)
EndRegion(ref ce) => write!(fmt, "EndRegion({})", ty::ReScope(*ce)),
Validate(ref op, ref places) => write!(fmt, "Validate({:?}, {:?})", op, places),
@@ -2585,6 +2612,7 @@ CloneTypeFoldableAndLiftImpls! {
Mutability,
SourceInfo,
UpvarDecl,
FakeReadCause,
ValidationOp,
SourceScope,
SourceScopeData,
@@ -2651,7 +2679,7 @@ BraceStructTypeFoldableImpl! {
EnumTypeFoldableImpl! {
impl<'tcx> TypeFoldable<'tcx> for StatementKind<'tcx> {
(StatementKind::Assign)(a, b),
(StatementKind::ReadForMatch)(place),
(StatementKind::FakeRead)(cause, place),
(StatementKind::SetDiscriminant) { place, variant_index },
(StatementKind::StorageLive)(a),
(StatementKind::StorageDead)(a),
2 changes: 1 addition & 1 deletion src/librustc/mir/visit.rs
Original file line number Diff line number Diff line change
@@ -354,7 +354,7 @@ macro_rules! make_mir_visitor {
ref $($mutability)* rvalue) => {
self.visit_assign(block, place, rvalue, location);
}
StatementKind::ReadForMatch(ref $($mutability)* place) => {
StatementKind::FakeRead(_, ref $($mutability)* place) => {
self.visit_place(place,
PlaceContext::Inspect,
location);
2 changes: 1 addition & 1 deletion src/librustc/session/config.rs
Original file line number Diff line number Diff line change
@@ -1330,7 +1330,7 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options,
disable_nll_user_type_assert: bool = (false, parse_bool, [UNTRACKED],
"disable user provided type assertion in NLL"),
nll_dont_emit_read_for_match: bool = (false, parse_bool, [UNTRACKED],
"in match codegen, do not include ReadForMatch statements (used by mir-borrowck)"),
"in match codegen, do not include FakeRead statements (used by mir-borrowck)"),
dont_buffer_diagnostics: bool = (false, parse_bool, [UNTRACKED],
"emit diagnostics rather than buffering (breaks NLL error downgrading, sorting)."),
polonius: bool = (false, parse_bool, [UNTRACKED],
2 changes: 1 addition & 1 deletion src/librustc/ty/context.rs
Original file line number Diff line number Diff line change
@@ -1478,7 +1478,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
self.emit_read_for_match()
}

/// If true, make MIR codegen for `match` emit ReadForMatch
/// If true, make MIR codegen for `match` emit FakeRead
/// statements (which simulate the maximal effect of executing the
/// patterns in a match arm).
pub fn emit_read_for_match(&self) -> bool {
2 changes: 1 addition & 1 deletion src/librustc_codegen_llvm/mir/statement.rs
Original file line number Diff line number Diff line change
@@ -89,7 +89,7 @@ impl FunctionCx<'a, 'll, 'tcx> {
asm::codegen_inline_asm(&bx, asm, outputs, input_vals);
bx
}
mir::StatementKind::ReadForMatch(_) |
mir::StatementKind::FakeRead(..) |
mir::StatementKind::EndRegion(_) |
mir::StatementKind::Validate(..) |
mir::StatementKind::AscribeUserType(..) |
17 changes: 16 additions & 1 deletion src/librustc_mir/borrow_check/error_reporting.rs
Original file line number Diff line number Diff line change
@@ -12,7 +12,7 @@ use borrow_check::WriteKind;
use rustc::middle::region::ScopeTree;
use rustc::mir::VarBindingForm;
use rustc::mir::{BindingForm, BorrowKind, ClearCrossCrate, Field, Local};
use rustc::mir::{LocalDecl, LocalKind, Location, Operand, Place};
use rustc::mir::{FakeReadCause, LocalDecl, LocalKind, Location, Operand, Place};
use rustc::mir::{ProjectionElem, Rvalue, Statement, StatementKind};
use rustc::ty;
use rustc_data_structures::fx::FxHashSet;
@@ -989,6 +989,21 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
false
}
}

/// Returns the `FakeReadCause` at this location if it is a `FakeRead` statement.
pub(super) fn retrieve_fake_read_cause_for_location(
&self,
location: &Location,
) -> Option<FakeReadCause> {
let stmt = self.mir.basic_blocks()[location.block]
.statements
.get(location.statement_index)?;
if let StatementKind::FakeRead(cause, _) = stmt.kind {
Some(cause)
} else {
None
}
}
}

// The span(s) associated to a use of a place.
6 changes: 3 additions & 3 deletions src/librustc_mir/borrow_check/mod.rs
Original file line number Diff line number Diff line change
@@ -478,9 +478,9 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx
flow_state,
);
}
StatementKind::ReadForMatch(ref place) => {
StatementKind::FakeRead(_, ref place) => {
self.access_place(
ContextKind::ReadForMatch.new(location),
ContextKind::FakeRead.new(location),
(place, span),
(Deep, Read(ReadKind::Borrow(BorrowKind::Shared))),
LocalMutationIsAllowed::No,
@@ -2206,7 +2206,7 @@ enum ContextKind {
CallDest,
Assert,
Yield,
ReadForMatch,
FakeRead,
StorageDead,
}

10 changes: 8 additions & 2 deletions src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs
Original file line number Diff line number Diff line change
@@ -11,7 +11,7 @@
use borrow_check::borrow_set::BorrowData;
use borrow_check::nll::region_infer::Cause;
use borrow_check::{Context, MirBorrowckCtxt, WriteKind};
use rustc::mir::{Local, Location, Place, TerminatorKind};
use rustc::mir::{FakeReadCause, Local, Location, Place, TerminatorKind};
use rustc_errors::DiagnosticBuilder;
use rustc::ty::Region;

@@ -142,7 +142,13 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
if spans.for_closure() {
"borrow later captured here by closure"
} else {
"borrow later used here"
// Check if the location represents a `FakeRead`, and adapt the error
// message to the `FakeReadCause` it is from: in particular,
// the ones inserted in optimized `let var = <expr>` patterns.
match self.retrieve_fake_read_cause_for_location(&location) {
Some(FakeReadCause::ForLet) => "borrow later stored here",
_ => "borrow later used here"
}
}
};
err.span_label(spans.var_or_use(), message);
4 changes: 2 additions & 2 deletions src/librustc_mir/borrow_check/nll/invalidation.rs
Original file line number Diff line number Diff line change
@@ -93,9 +93,9 @@ impl<'cg, 'cx, 'tcx, 'gcx> Visitor<'tcx> for InvalidationGenerator<'cg, 'cx, 'tc
JustWrite
);
}
StatementKind::ReadForMatch(ref place) => {
StatementKind::FakeRead(_, ref place) => {
self.access_place(
ContextKind::ReadForMatch.new(location),
ContextKind::FakeRead.new(location),
place,
(Deep, Read(ReadKind::Borrow(BorrowKind::Shared))),
LocalMutationIsAllowed::No,
2 changes: 1 addition & 1 deletion src/librustc_mir/borrow_check/nll/type_check/mod.rs
Original file line number Diff line number Diff line change
@@ -995,7 +995,7 @@ impl<'a, 'gcx, 'tcx> TypeChecker<'a, 'gcx, 'tcx> {
);
}
}
StatementKind::ReadForMatch(_)
StatementKind::FakeRead(..)
| StatementKind::StorageLive(_)
| StatementKind::StorageDead(_)
| StatementKind::InlineAsm { .. }
34 changes: 26 additions & 8 deletions src/librustc_mir/build/matches/mod.rs
Original file line number Diff line number Diff line change
@@ -145,19 +145,16 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
if let (true, Some(borrow_temp)) =
(tcx.emit_read_for_match(), borrowed_input_temp.clone())
{
// inject a fake read of the borrowed input at
// the start of each arm's pattern testing
// code.
//
// This should ensure that you cannot change
// the variant for an enum while you are in
// the midst of matching on it.
// Inject a fake read, see comments on `FakeReadCause::ForMatch`.
let pattern_source_info = self.source_info(pattern.span);
self.cfg.push(
*pre_binding_block,
Statement {
source_info: pattern_source_info,
kind: StatementKind::ReadForMatch(borrow_temp.clone()),
kind: StatementKind::FakeRead(
FakeReadCause::ForMatch,
borrow_temp.clone(),
),
},
);
}
@@ -264,6 +261,18 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
let place =
self.storage_live_binding(block, var, irrefutable_pat.span, OutsideGuard);
unpack!(block = self.into(&place, block, initializer));


// Inject a fake read, see comments on `FakeReadCause::ForLet`.
let source_info = self.source_info(irrefutable_pat.span);
self.cfg.push(
block,
Statement {
source_info,
kind: StatementKind::FakeRead(FakeReadCause::ForLet, place.clone()),
},
);

self.schedule_drop_for_binding(var, irrefutable_pat.span, OutsideGuard);
block.unit()
}
@@ -305,6 +314,15 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
},
);

// Inject a fake read, see comments on `FakeReadCause::ForLet`.
self.cfg.push(
block,
Statement {
source_info,
kind: StatementKind::FakeRead(FakeReadCause::ForLet, place.clone()),
},
);

self.schedule_drop_for_binding(var, irrefutable_pat.span, OutsideGuard);
block.unit()
}
2 changes: 1 addition & 1 deletion src/librustc_mir/dataflow/impls/borrows.rs
Original file line number Diff line number Diff line change
@@ -336,7 +336,7 @@ impl<'a, 'gcx, 'tcx> BitDenotation for Borrows<'a, 'gcx, 'tcx> {
}
}

mir::StatementKind::ReadForMatch(..) |
mir::StatementKind::FakeRead(..) |
mir::StatementKind::SetDiscriminant { .. } |
mir::StatementKind::StorageLive(..) |
mir::StatementKind::Validate(..) |
2 changes: 1 addition & 1 deletion src/librustc_mir/dataflow/move_paths/builder.rs
Original file line number Diff line number Diff line change
@@ -281,7 +281,7 @@ impl<'b, 'a, 'gcx, 'tcx> Gatherer<'b, 'a, 'gcx, 'tcx> {
}
self.gather_rvalue(rval);
}
StatementKind::ReadForMatch(ref place) => {
StatementKind::FakeRead(_, ref place) => {
self.create_move_path(place);
}
StatementKind::InlineAsm { ref outputs, ref inputs, ref asm } => {
4 changes: 2 additions & 2 deletions src/librustc_mir/interpret/step.rs
Original file line number Diff line number Diff line change
@@ -150,9 +150,9 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
self.deallocate_local(old_val)?;
}

// No dynamic semantics attached to `ReadForMatch`; MIR
// No dynamic semantics attached to `FakeRead`; MIR
// interpreter is solely intended for borrowck'ed code.
ReadForMatch(..) => {}
FakeRead(..) => {}

// Validity checks.
Validate(op, ref places) => {
2 changes: 1 addition & 1 deletion src/librustc_mir/transform/check_unsafety.rs
Original file line number Diff line number Diff line change
@@ -108,7 +108,7 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
self.source_info = statement.source_info;
match statement.kind {
StatementKind::Assign(..) |
StatementKind::ReadForMatch(..) |
StatementKind::FakeRead(..) |
StatementKind::SetDiscriminant { .. } |
StatementKind::StorageLive(..) |
StatementKind::StorageDead(..) |
2 changes: 1 addition & 1 deletion src/librustc_mir/transform/qualify_consts.rs
Original file line number Diff line number Diff line change
@@ -1090,7 +1090,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> {
StatementKind::Assign(ref place, ref rvalue) => {
this.visit_assign(bb, place, rvalue, location);
}
StatementKind::ReadForMatch(..) |
StatementKind::FakeRead(..) |
StatementKind::SetDiscriminant { .. } |
StatementKind::StorageLive(_) |
StatementKind::StorageDead(_) |
2 changes: 1 addition & 1 deletion src/librustc_mir/transform/qualify_min_const_fn.rs
Original file line number Diff line number Diff line change
@@ -216,7 +216,7 @@ fn check_statement(
check_rvalue(tcx, mir, rval, span)
}

StatementKind::ReadForMatch(_) => Err((span, "match in const fn is unstable".into())),
StatementKind::FakeRead(..) => Err((span, "match in const fn is unstable".into())),

// just an assignment
StatementKind::SetDiscriminant { .. } => Ok(()),
2 changes: 1 addition & 1 deletion src/librustc_mir/transform/remove_noop_landing_pads.rs
Original file line number Diff line number Diff line change
@@ -49,7 +49,7 @@ impl RemoveNoopLandingPads {
) -> bool {
for stmt in &mir[bb].statements {
match stmt.kind {
StatementKind::ReadForMatch(_) |
StatementKind::FakeRead(..) |
StatementKind::StorageLive(_) |
StatementKind::StorageDead(_) |
StatementKind::EndRegion(_) |
2 changes: 1 addition & 1 deletion src/librustc_mir/transform/rustc_peek.rs
Original file line number Diff line number Diff line change
@@ -157,7 +157,7 @@ fn each_block<'a, 'tcx, O>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
mir::StatementKind::Assign(ref place, ref rvalue) => {
(place, rvalue)
}
mir::StatementKind::ReadForMatch(_) |
mir::StatementKind::FakeRead(..) |
mir::StatementKind::StorageLive(_) |
mir::StatementKind::StorageDead(_) |
mir::StatementKind::InlineAsm { .. } |
2 changes: 1 addition & 1 deletion src/librustc_passes/mir_stats.rs
Original file line number Diff line number Diff line change
@@ -82,7 +82,7 @@ impl<'a, 'tcx> mir_visit::Visitor<'tcx> for StatCollector<'a, 'tcx> {
self.record("Statement", statement);
self.record(match statement.kind {
StatementKind::Assign(..) => "StatementKind::Assign",
StatementKind::ReadForMatch(..) => "StatementKind::ReadForMatch",
StatementKind::FakeRead(..) => "StatementKind::FakeRead",
StatementKind::EndRegion(..) => "StatementKind::EndRegion",
StatementKind::Validate(..) => "StatementKind::Validate",
StatementKind::SetDiscriminant { .. } => "StatementKind::SetDiscriminant",
2 changes: 2 additions & 0 deletions src/test/mir-opt/basic_assignment.rs
Original file line number Diff line number Diff line change
@@ -47,6 +47,7 @@ fn main() {
// bb0: {
// StorageLive(_1);
// _1 = const false;
// FakeRead(ForLet, _1);
// StorageLive(_2);
// StorageLive(_3);
// _3 = _1;
@@ -55,6 +56,7 @@ fn main() {
// StorageLive(_4);
// _4 = std::option::Option<std::boxed::Box<u32>>::None;
// AscribeUserType(_4, o, Canonical { variables: [], value: std::option::Option<std::boxed::Box<u32>> });
// FakeRead(ForLet, _4);
// StorageLive(_5);
// StorageLive(_6);
// _6 = move _4;
1 change: 1 addition & 0 deletions src/test/mir-opt/box_expr.rs
Original file line number Diff line number Diff line change
@@ -63,6 +63,7 @@ impl Drop for S {
//
// bb4: {
// StorageDead(_2);
// FakeRead(ForLet, _1);
// StorageLive(_4);
// _4 = move _1;
// _3 = const std::mem::drop(move _4) -> [return: bb5, unwind: bb7];
Loading