Skip to content

Commit

Permalink
Auto merge of #46022 - matthewjasper:cannot-assign-twice-error, r=ari…
Browse files Browse the repository at this point in the history 
…elb1

Mir Borrowck: Parity with Ast for E0384 (Cannot assign twice to immutable)

- Closes #45199
- Don't allow assigning to dropped immutable variables
- Show the "first assignment" note on the first assignment that can actually come before the second assignment.
- Make "first assignment" notes point to function parameters if needed.
- Don't show a "first assignment" note if the first and second assignment have the same span (in a loop). This matches ast borrowck for now, but maybe this we should add "in previous loop iteration" as with some other borrowck errors. (Commit 2)
- Use revisions to check mir borrowck for the existing tests for this error. (Commit 3)

~~Still working on a less ad-hoc way to get 'first assignment' notes to show on the correct assignment. Also need to check mutating function arguments.~~ Now using a new dataflow pass.
  • Loading branch information
@bors
bors committed Nov 27, 2017
2 parents 58e1234 + d64a318 commit 560a5da
Show file tree
Hide file tree
Showing 15 changed files with 575 additions and 140 deletions.
82 changes: 50 additions & 32 deletions src/librustc_mir/borrow_check.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ use dataflow::{do_dataflow};
use dataflow::{MoveDataParamEnv};
use dataflow::{BitDenotation, BlockSets, DataflowResults, DataflowResultsConsumer};
use dataflow::{MaybeInitializedLvals, MaybeUninitializedLvals};
use dataflow::{MovingOutStatements};
use dataflow::{MovingOutStatements, EverInitializedLvals};
use dataflow::{Borrows, BorrowData, BorrowIndex};
use dataflow::move_paths::{MoveError, IllegalMoveOriginKind};
use dataflow::move_paths::{HasMoveData, MoveData, MovePathIndex, LookupResult, MoveOutIndex};
Expand Down Expand Up @@ -130,6 +130,9 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>(infcx: &InferCtxt<'a, 'gcx, 'tcx>,
let flow_move_outs = do_dataflow(tcx, mir, id, &attributes, &dead_unwinds,
MovingOutStatements::new(tcx, mir, &mdpe),
|bd, i| &bd.move_data().moves[i]);
let flow_ever_inits = do_dataflow(tcx, mir, id, &attributes, &dead_unwinds,
EverInitializedLvals::new(tcx, mir, &mdpe),
|bd, i| &bd.move_data().inits[i]);

let mut mbcx = MirBorrowckCtxt {
tcx: tcx,
Expand All @@ -143,7 +146,8 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>(infcx: &InferCtxt<'a, 'gcx, 'tcx>,
let mut state = InProgress::new(flow_borrows,
flow_inits,
flow_uninits,
flow_move_outs);
flow_move_outs,
flow_ever_inits);

mbcx.analyze_results(&mut state); // entry point for DataflowResultsConsumer
}
Expand All @@ -167,6 +171,7 @@ pub struct InProgress<'b, 'gcx: 'tcx, 'tcx: 'b> {
inits: FlowInProgress<MaybeInitializedLvals<'b, 'gcx, 'tcx>>,
uninits: FlowInProgress<MaybeUninitializedLvals<'b, 'gcx, 'tcx>>,
move_outs: FlowInProgress<MovingOutStatements<'b, 'gcx, 'tcx>>,
ever_inits: FlowInProgress<EverInitializedLvals<'b, 'gcx, 'tcx>>,
}

struct FlowInProgress<BD> where BD: BitDenotation {
Expand All @@ -190,7 +195,8 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx
flow_state.each_flow(|b| b.reset_to_entry_of(bb),
|i| i.reset_to_entry_of(bb),
|u| u.reset_to_entry_of(bb),
|m| m.reset_to_entry_of(bb));
|m| m.reset_to_entry_of(bb),
|e| e.reset_to_entry_of(bb));
}

fn reconstruct_statement_effect(&mut self,
Expand All @@ -199,7 +205,8 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx
flow_state.each_flow(|b| b.reconstruct_statement_effect(location),
|i| i.reconstruct_statement_effect(location),
|u| u.reconstruct_statement_effect(location),
|m| m.reconstruct_statement_effect(location));
|m| m.reconstruct_statement_effect(location),
|e| e.reconstruct_statement_effect(location));
}

fn apply_local_effect(&mut self,
Expand All @@ -208,7 +215,8 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx
flow_state.each_flow(|b| b.apply_local_effect(),
|i| i.apply_local_effect(),
|u| u.apply_local_effect(),
|m| m.apply_local_effect());
|m| m.apply_local_effect(),
|e| e.apply_local_effect());
}

fn reconstruct_terminator_effect(&mut self,
Expand All @@ -217,7 +225,8 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx
flow_state.each_flow(|b| b.reconstruct_terminator_effect(location),
|i| i.reconstruct_terminator_effect(location),
|u| u.reconstruct_terminator_effect(location),
|m| m.reconstruct_terminator_effect(location));
|m| m.reconstruct_terminator_effect(location),
|e| e.reconstruct_terminator_effect(location));
}

fn visit_block_entry(&mut self,
Expand Down Expand Up @@ -750,22 +759,13 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
}

if let Some(mpi) = self.move_path_for_lvalue(lvalue) {
if flow_state.inits.curr_state.contains(&mpi) {
// may already be assigned before reaching this statement;
// report error.
// FIXME: Not ideal, it only finds the assignment that lexically comes first
let assigned_lvalue = &move_data.move_paths[mpi].lvalue;
let assignment_stmt = self.mir.basic_blocks().iter().filter_map(|bb| {
bb.statements.iter().find(|stmt| {
if let StatementKind::Assign(ref lv, _) = stmt.kind {
*lv == *assigned_lvalue
} else {
false
}
})
}).next().unwrap();
self.report_illegal_reassignment(
context, (lvalue, span), assignment_stmt.source_info.span);
for ii in &move_data.init_path_map[mpi] {
if flow_state.ever_inits.curr_state.contains(ii) {
let first_assign_span = self.move_data.inits[*ii].span;
self.report_illegal_reassignment(
context, (lvalue, span), first_assign_span);
break;
}
}
}
}
Expand Down Expand Up @@ -1586,13 +1586,15 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
_context: Context,
(lvalue, span): (&Lvalue<'tcx>, Span),
assigned_span: Span) {
self.tcx.cannot_reassign_immutable(span,
let mut err = self.tcx.cannot_reassign_immutable(span,
&self.describe_lvalue(lvalue),
Origin::Mir)
.span_label(span, "cannot assign twice to immutable variable")
.span_label(assigned_span, format!("first assignment to `{}`",
self.describe_lvalue(lvalue)))
.emit();
Origin::Mir);
err.span_label(span, "cannot assign twice to immutable variable");
if span != assigned_span {
err.span_label(assigned_span, format!("first assignment to `{}`",
self.describe_lvalue(lvalue)));
}
err.emit();
}

fn report_assignment_to_static(&mut self,
Expand Down Expand Up @@ -1852,30 +1854,35 @@ impl<'b, 'gcx, 'tcx> InProgress<'b, 'gcx, 'tcx> {
pub(super) fn new(borrows: DataflowResults<Borrows<'b, 'gcx, 'tcx>>,
inits: DataflowResults<MaybeInitializedLvals<'b, 'gcx, 'tcx>>,
uninits: DataflowResults<MaybeUninitializedLvals<'b, 'gcx, 'tcx>>,
move_out: DataflowResults<MovingOutStatements<'b, 'gcx, 'tcx>>)
move_out: DataflowResults<MovingOutStatements<'b, 'gcx, 'tcx>>,
ever_inits: DataflowResults<EverInitializedLvals<'b, 'gcx, 'tcx>>)
-> Self {
InProgress {
borrows: FlowInProgress::new(borrows),
inits: FlowInProgress::new(inits),
uninits: FlowInProgress::new(uninits),
move_outs: FlowInProgress::new(move_out)
move_outs: FlowInProgress::new(move_out),
ever_inits: FlowInProgress::new(ever_inits)
}
}

fn each_flow<XB, XI, XU, XM>(&mut self,
fn each_flow<XB, XI, XU, XM, XE>(&mut self,
mut xform_borrows: XB,
mut xform_inits: XI,
mut xform_uninits: XU,
mut xform_move_outs: XM) where
mut xform_move_outs: XM,
mut xform_ever_inits: XE) where
XB: FnMut(&mut FlowInProgress<Borrows<'b, 'gcx, 'tcx>>),
XI: FnMut(&mut FlowInProgress<MaybeInitializedLvals<'b, 'gcx, 'tcx>>),
XU: FnMut(&mut FlowInProgress<MaybeUninitializedLvals<'b, 'gcx, 'tcx>>),
XM: FnMut(&mut FlowInProgress<MovingOutStatements<'b, 'gcx, 'tcx>>),
XE: FnMut(&mut FlowInProgress<EverInitializedLvals<'b, 'gcx, 'tcx>>),
{
xform_borrows(&mut self.borrows);
xform_inits(&mut self.inits);
xform_uninits(&mut self.uninits);
xform_move_outs(&mut self.move_outs);
xform_ever_inits(&mut self.ever_inits);
}

fn summary(&self) -> String {
Expand Down Expand Up @@ -1932,6 +1939,17 @@ impl<'b, 'gcx, 'tcx> InProgress<'b, 'gcx, 'tcx> {
&self.move_outs.base_results.operator().move_data().moves[mpi_move_out];
s.push_str(&format!("{:?}", move_out));
});
s.push_str("] ");

s.push_str("ever_init: [");
let mut saw_one = false;
self.ever_inits.each_state_bit(|mpi_ever_init| {
if saw_one { s.push_str(", "); };
saw_one = true;
let ever_init =
&self.ever_inits.base_results.operator().move_data().inits[mpi_ever_init];
s.push_str(&format!("{:?}", ever_init));
});
s.push_str("]");

return s;
Expand Down
75 changes: 34 additions & 41 deletions src/librustc_mir/dataflow/drop_flag_effects.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use util::elaborate_drops::DropFlagState;

use super::{MoveDataParamEnv};
use super::indexes::MovePathIndex;
use super::move_paths::{MoveData, LookupResult};
use super::move_paths::{MoveData, LookupResult, InitKind};

pub fn move_path_children_matching<'tcx, F>(move_data: &MoveData<'tcx>,
path: MovePathIndex,
Expand Down Expand Up @@ -197,47 +197,40 @@ pub(crate) fn drop_flag_effects_for_location<'a, 'gcx, 'tcx, F>(
|mpi| callback(mpi, DropFlagState::Absent))
}

let block = &mir[loc.block];
match block.statements.get(loc.statement_index) {
Some(stmt) => match stmt.kind {
mir::StatementKind::SetDiscriminant{ .. } => {
span_bug!(stmt.source_info.span, "SetDiscrimant should not exist during borrowck");
}
mir::StatementKind::Assign(ref lvalue, ref rvalue) => {
match rvalue.initialization_state() {
mir::tcx::RvalueInitializationState::Shallow => {
debug!("drop_flag_effects: box assignment {:?}", stmt);
if let LookupResult::Exact(mpi) = move_data.rev_lookup.find(lvalue) {
callback(mpi, DropFlagState::Present);
}
}
mir::tcx::RvalueInitializationState::Deep => {
debug!("drop_flag_effects: assignment {:?}", stmt);
on_lookup_result_bits(tcx, mir, move_data,
move_data.rev_lookup.find(lvalue),
|mpi| callback(mpi, DropFlagState::Present))
}
}
}
mir::StatementKind::StorageLive(_) |
mir::StatementKind::StorageDead(_) |
mir::StatementKind::InlineAsm { .. } |
mir::StatementKind::EndRegion(_) |
mir::StatementKind::Validate(..) |
mir::StatementKind::Nop => {}
},
None => {
debug!("drop_flag_effects: replace {:?}", block.terminator());
match block.terminator().kind {
mir::TerminatorKind::DropAndReplace { ref location, .. } => {
on_lookup_result_bits(tcx, mir, move_data,
move_data.rev_lookup.find(location),
|mpi| callback(mpi, DropFlagState::Present))
}
_ => {
// other terminators do not contain move-ins
}
debug!("drop_flag_effects: assignment for location({:?})", loc);

for_location_inits(
tcx,
mir,
move_data,
loc,
|mpi| callback(mpi, DropFlagState::Present)
);
}

pub(crate) fn for_location_inits<'a, 'gcx, 'tcx, F>(
tcx: TyCtxt<'a, 'gcx, 'tcx>,
mir: &Mir<'tcx>,
move_data: &MoveData<'tcx>,
loc: Location,
mut callback: F)
where F: FnMut(MovePathIndex)
{
for ii in &move_data.init_loc_map[loc] {
let init = move_data.inits[*ii];
match init.kind {
InitKind::Deep => {
let path = init.path;

on_all_children_bits(tcx, mir, move_data,
path,
&mut callback)
},
InitKind::Shallow => {
let mpi = init.path;
callback(mpi);
}
InitKind::NonPanicPathOnly => (),
}
}
}
Loading

0 comments on commit 560a5da

Please sign in to comment.