From 655894baf96e02b4a7270447b8c636a52b357bdf Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 20 Jul 2018 16:38:00 +0200 Subject: [PATCH 01/10] Add flag indicating whether AST `borrowck` query signalled any error. --- src/librustc/middle/borrowck.rs | 8 ++++++ src/librustc_borrowck/borrowck/check_loans.rs | 13 ++++++++-- .../borrowck/gather_loans/move_error.rs | 1 + src/librustc_borrowck/borrowck/mod.rs | 26 +++++++++++++++++-- 4 files changed, 44 insertions(+), 4 deletions(-) diff --git a/src/librustc/middle/borrowck.rs b/src/librustc/middle/borrowck.rs index 6f5791ed5d71b..c8d513a59f00d 100644 --- a/src/librustc/middle/borrowck.rs +++ b/src/librustc/middle/borrowck.rs @@ -15,9 +15,15 @@ use util::nodemap::FxHashSet; use rustc_data_structures::stable_hasher::{HashStable, StableHasher, StableHasherResult}; +#[derive(Copy, Clone, Debug, RustcEncodable, RustcDecodable)] +pub enum SignalledError { SawSomeError, NoErrorsSeen } + +impl_stable_hash_for!(enum self::SignalledError { SawSomeError, NoErrorsSeen }); + #[derive(Debug, RustcEncodable, RustcDecodable)] pub struct BorrowCheckResult { pub used_mut_nodes: FxHashSet, + pub signalled_any_error: SignalledError, } impl<'a> HashStable> for BorrowCheckResult { @@ -26,7 +32,9 @@ impl<'a> HashStable> for BorrowCheckResult { hasher: &mut StableHasher) { let BorrowCheckResult { ref used_mut_nodes, + ref signalled_any_error, } = *self; used_mut_nodes.hash_stable(hcx, hasher); + signalled_any_error.hash_stable(hcx, hasher); } } diff --git a/src/librustc_borrowck/borrowck/check_loans.rs b/src/librustc_borrowck/borrowck/check_loans.rs index 002e8697588fc..49bd69f826216 100644 --- a/src/librustc_borrowck/borrowck/check_loans.rs +++ b/src/librustc_borrowck/borrowck/check_loans.rs @@ -447,10 +447,12 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> { .region_scope_tree .yield_in_scope_for_expr(scope, cmt.hir_id, - self.bccx.body) { + self.bccx.body) + { self.bccx.cannot_borrow_across_generator_yield(borrow_span, yield_span, Origin::Ast).emit(); + self.bccx.signal_error(); } } @@ -507,9 +509,13 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> { new_loan, old_loan, old_loan, new_loan).err(); match (err_old_new, err_new_old) { - (Some(mut err), None) | (None, Some(mut err)) => err.emit(), + (Some(mut err), None) | (None, Some(mut err)) => { + err.emit(); + self.bccx.signal_error(); + } (Some(mut err_old), Some(mut err_new)) => { err_old.emit(); + self.bccx.signal_error(); err_new.cancel(); } (None, None) => return true, @@ -695,6 +701,7 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> { loan_span, &self.bccx.loan_path_to_string(&loan_path), Origin::Ast) .emit(); + self.bccx.signal_error(); } } } @@ -745,6 +752,7 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> { }; err.emit(); + self.bccx.signal_error(); } } } @@ -914,5 +922,6 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> { self.bccx.cannot_assign_to_borrowed( span, loan.span, &self.bccx.loan_path_to_string(loan_path), Origin::Ast) .emit(); + self.bccx.signal_error(); } } diff --git a/src/librustc_borrowck/borrowck/gather_loans/move_error.rs b/src/librustc_borrowck/borrowck/gather_loans/move_error.rs index f221565c7f391..e51caf89ee651 100644 --- a/src/librustc_borrowck/borrowck/gather_loans/move_error.rs +++ b/src/librustc_borrowck/borrowck/gather_loans/move_error.rs @@ -99,6 +99,7 @@ fn report_move_errors<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>, errors: &Vec(tcx: TyCtxt<'a, 'tcx, 'tcx>, owner_def_id: DefId) // and do not need borrowchecking. return Lrc::new(BorrowCheckResult { used_mut_nodes: FxHashSet(), + signalled_any_error: SignalledError::NoErrorsSeen, }) } _ => { } @@ -121,6 +122,7 @@ fn borrowck<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, owner_def_id: DefId) owner_def_id, body, used_mut_nodes: RefCell::new(FxHashSet()), + signalled_any_error: Cell::new(SignalledError::NoErrorsSeen), }; // Eventually, borrowck will always read the MIR, but at the @@ -154,6 +156,7 @@ fn borrowck<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, owner_def_id: DefId) Lrc::new(BorrowCheckResult { used_mut_nodes: bccx.used_mut_nodes.into_inner(), + signalled_any_error: bccx.signalled_any_error.into_inner(), }) } @@ -234,6 +237,7 @@ pub fn build_borrowck_dataflow_data_for_fn<'a, 'tcx>( owner_def_id, body, used_mut_nodes: RefCell::new(FxHashSet()), + signalled_any_error: Cell::new(SignalledError::NoErrorsSeen), }; let dataflow_data = build_borrowck_dataflow_data(&mut bccx, true, body_id, |_| cfg); @@ -257,6 +261,15 @@ pub struct BorrowckCtxt<'a, 'tcx: 'a> { body: &'tcx hir::Body, used_mut_nodes: RefCell>, + + signalled_any_error: Cell, +} + + +impl<'a, 'tcx: 'a> BorrowckCtxt<'a, 'tcx> { + fn signal_error(&self) { + self.signalled_any_error.set(SignalledError::SawSomeError); + } } impl<'a, 'b, 'tcx: 'b> BorrowckErrors<'a> for &'a BorrowckCtxt<'b, 'tcx> { @@ -645,6 +658,7 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> { .span_label(use_span, format!("use of possibly uninitialized `{}`", self.loan_path_to_string(lp))) .emit(); + self.signal_error(); return; } _ => { @@ -760,6 +774,7 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> { // not considered particularly helpful. err.emit(); + self.signal_error(); } pub fn report_partial_reinitialization_of_uninitialized_structure( @@ -770,6 +785,7 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> { &self.loan_path_to_string(lp), Origin::Ast) .emit(); + self.signal_error(); } pub fn report_reassigned_immutable_variable(&self, @@ -787,6 +803,7 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> { self.loan_path_to_string(lp))); } err.emit(); + self.signal_error(); } pub fn struct_span_err_with_code>(&self, @@ -908,6 +925,7 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> { self.tcx.hir.hir_to_node_id(err.cmt.hir_id) ); db.emit(); + self.signal_error(); } err_out_of_scope(super_scope, sub_scope, cause) => { let msg = match opt_loan_path(&err.cmt) { @@ -1022,6 +1040,7 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> { } db.emit(); + self.signal_error(); } err_borrowed_pointer_too_short(loan_scope, ptr_scope) => { let descr = self.cmt_to_path_or_string(err.cmt); @@ -1047,6 +1066,7 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> { ""); db.emit(); + self.signal_error(); } } } @@ -1125,6 +1145,7 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> { err.help("closures behind references must be called via `&mut`"); } err.emit(); + self.signal_error(); } /// Given a type, if it is an immutable reference, return a suggestion to make it mutable @@ -1307,6 +1328,7 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> { cmt_path_or_string), suggestion) .emit(); + self.signal_error(); } fn region_end_span(&self, region: ty::Region<'tcx>) -> Option { From a23e8a726c167cf806840d35c787240d4008d3d0 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 20 Jul 2018 17:29:29 +0200 Subject: [PATCH 02/10] Add `-Z borrowck=migrate` flag, use it to link NLL up to AST-borrowck. --- src/librustc/session/config.rs | 15 +++++++++++++ src/librustc/ty/context.rs | 7 ++++++ src/librustc_borrowck/borrowck/mod.rs | 2 +- src/librustc_errors/diagnostic.rs | 19 ++++++++++++++++ src/librustc_errors/diagnostic_builder.rs | 19 ---------------- src/librustc_mir/borrow_check/mod.rs | 27 ++++++++++++++++++++--- 6 files changed, 66 insertions(+), 23 deletions(-) diff --git a/src/librustc/session/config.rs b/src/librustc/session/config.rs index 293b5c63cf06a..1dadf07808f83 100644 --- a/src/librustc/session/config.rs +++ b/src/librustc/session/config.rs @@ -455,15 +455,28 @@ pub enum BorrowckMode { Ast, Mir, Compare, + Migrate, } impl BorrowckMode { + /// Should we run the MIR-based borrow check, but also fall back + /// on the AST borrow check if the MIR-based one errors. + pub fn migrate(self) -> bool { + match self { + BorrowckMode::Ast => false, + BorrowckMode::Compare => false, + BorrowckMode::Mir => false, + BorrowckMode::Migrate => true, + } + } + /// Should we emit the AST-based borrow checker errors? pub fn use_ast(self) -> bool { match self { BorrowckMode::Ast => true, BorrowckMode::Compare => true, BorrowckMode::Mir => false, + BorrowckMode::Migrate => false, } } /// Should we emit the MIR-based borrow checker errors? @@ -472,6 +485,7 @@ impl BorrowckMode { BorrowckMode::Ast => false, BorrowckMode::Compare => true, BorrowckMode::Mir => true, + BorrowckMode::Migrate => true, } } } @@ -2166,6 +2180,7 @@ pub fn build_session_options_and_crate_config( None | Some("ast") => BorrowckMode::Ast, Some("mir") => BorrowckMode::Mir, Some("compare") => BorrowckMode::Compare, + Some("migrate") => BorrowckMode::Migrate, Some(m) => early_error(error_format, &format!("unknown borrowck mode `{}`", m)), }; diff --git a/src/librustc/ty/context.rs b/src/librustc/ty/context.rs index 41007508c5060..d30f656098dc1 100644 --- a/src/librustc/ty/context.rs +++ b/src/librustc/ty/context.rs @@ -1366,6 +1366,12 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { self.borrowck_mode().use_mir() } + /// If true, we should use the MIR-based borrow check, but also + /// fall back on the AST borrow check if the MIR-based one errors. + pub fn migrate_borrowck(self) -> bool { + self.borrowck_mode().migrate() + } + /// If true, make MIR codegen for `match` emit a temp that holds a /// borrow of the input to the match expression. pub fn generate_borrow_of_any_match_input(&self) -> bool { @@ -1399,6 +1405,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { pub fn borrowck_mode(&self) -> BorrowckMode { match self.sess.opts.borrowck_mode { mode @ BorrowckMode::Mir | + mode @ BorrowckMode::Migrate | mode @ BorrowckMode::Compare => mode, mode @ BorrowckMode::Ast => { diff --git a/src/librustc_borrowck/borrowck/mod.rs b/src/librustc_borrowck/borrowck/mod.rs index 86a7f30147024..0cb4a766e8079 100644 --- a/src/librustc_borrowck/borrowck/mod.rs +++ b/src/librustc_borrowck/borrowck/mod.rs @@ -90,7 +90,7 @@ pub struct AnalysisData<'a, 'tcx: 'a> { fn borrowck<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, owner_def_id: DefId) -> Lrc { - assert!(tcx.use_ast_borrowck()); + assert!(tcx.use_ast_borrowck() || tcx.migrate_borrowck()); debug!("borrowck(body_owner_def_id={:?})", owner_def_id); diff --git a/src/librustc_errors/diagnostic.rs b/src/librustc_errors/diagnostic.rs index b1578b697bb8c..825e31539c8be 100644 --- a/src/librustc_errors/diagnostic.rs +++ b/src/librustc_errors/diagnostic.rs @@ -99,6 +99,25 @@ impl Diagnostic { } } + pub fn is_error(&self) -> bool { + match self.level { + Level::Bug | + Level::Fatal | + Level::PhaseFatal | + Level::Error | + Level::FailureNote => { + true + } + + Level::Warning | + Level::Note | + Level::Help | + Level::Cancelled => { + false + } + } + } + /// Cancel the diagnostic (a structured diagnostic must either be emitted or /// canceled or it will panic when dropped). pub fn cancel(&mut self) { diff --git a/src/librustc_errors/diagnostic_builder.rs b/src/librustc_errors/diagnostic_builder.rs index 8f99ad87cb8e0..a0f3abda077f9 100644 --- a/src/librustc_errors/diagnostic_builder.rs +++ b/src/librustc_errors/diagnostic_builder.rs @@ -100,25 +100,6 @@ impl<'a> DiagnosticBuilder<'a> { buffered_diagnostics.push(diagnostic); } - pub fn is_error(&self) -> bool { - match self.level { - Level::Bug | - Level::Fatal | - Level::PhaseFatal | - Level::Error | - Level::FailureNote => { - true - } - - Level::Warning | - Level::Note | - Level::Help | - Level::Cancelled => { - false - } - } - } - /// Convenience function for internal use, clients should use one of the /// span_* methods instead. pub fn sub>( diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index b3d7337cffe11..ad663000f938b 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -16,6 +16,7 @@ use rustc::hir::def_id::DefId; use rustc::hir::map::definitions::DefPathData; use rustc::infer::InferCtxt; use rustc::lint::builtin::UNUSED_MUT; +use rustc::middle::borrowck::SignalledError; use rustc::mir::{AggregateKind, BasicBlock, BorrowCheckResult, BorrowKind}; use rustc::mir::{ClearCrossCrate, Local, Location, Mir, Mutability, Operand, Place}; use rustc::mir::{Field, Projection, ProjectionElem, Rvalue, Statement, StatementKind}; @@ -23,7 +24,7 @@ use rustc::mir::{Terminator, TerminatorKind}; use rustc::ty::query::Providers; use rustc::ty::{self, ParamEnv, TyCtxt}; -use rustc_errors::{Diagnostic, DiagnosticBuilder}; +use rustc_errors::{Diagnostic, DiagnosticBuilder, Level}; use rustc_data_structures::graph::dominators::Dominators; use rustc_data_structures::fx::FxHashSet; use rustc_data_structures::indexed_set::IdxSetBuf; @@ -329,8 +330,28 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>( } } - for diag in mbcx.errors_buffer.drain(..) { - DiagnosticBuilder::new_diagnostic(mbcx.tcx.sess.diagnostic(), diag).emit(); + if mbcx.errors_buffer.len() > 0 { + if tcx.migrate_borrowck() { + match tcx.borrowck(def_id).signalled_any_error { + SignalledError::NoErrorsSeen => { + // if AST-borrowck signalled no errors, then + // downgrade all the buffered MIR-borrowck errors + // to warnings. + for err in &mut mbcx.errors_buffer { + if err.is_error() { err.level = Level::Warning; } + } + } + SignalledError::SawSomeError => { + // if AST-borrowck signalled a (cancelled) error, + // then we will just emit the buffered + // MIR-borrowck errors as normal. + } + } + } + + for diag in mbcx.errors_buffer.drain(..) { + DiagnosticBuilder::new_diagnostic(mbcx.tcx.sess.diagnostic(), diag).emit(); + } } let result = BorrowCheckResult { From ff2f9e02345cac102510870a67bef51e0c409a4f Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 24 Jul 2018 13:56:40 +0200 Subject: [PATCH 03/10] Add `migrate` to list of values for `-Z borrowck=...` --- src/librustc/session/config.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc/session/config.rs b/src/librustc/session/config.rs index 1dadf07808f83..441b13b8c4460 100644 --- a/src/librustc/session/config.rs +++ b/src/librustc/session/config.rs @@ -1141,7 +1141,7 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options, emit_end_regions: bool = (false, parse_bool, [UNTRACKED], "emit EndRegion as part of MIR; enable transforms that solely process EndRegion"), borrowck: Option = (None, parse_opt_string, [UNTRACKED], - "select which borrowck is used (`ast`, `mir`, or `compare`)"), + "select which borrowck is used (`ast`, `mir`, `migrate`, or `compare`)"), two_phase_borrows: bool = (false, parse_bool, [UNTRACKED], "use two-phase reserved/active distinction for `&mut` borrows in MIR borrowck"), two_phase_beyond_autoref: bool = (false, parse_bool, [UNTRACKED], From 346011515760dd552bd41d4abf8a2a55471a9e84 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Wed, 25 Jul 2018 01:31:40 +0200 Subject: [PATCH 04/10] Allow elaborate_drops to progress under errors that come up during borrowck=migrate. --- src/librustc_mir/transform/elaborate_drops.rs | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/librustc_mir/transform/elaborate_drops.rs b/src/librustc_mir/transform/elaborate_drops.rs index d35608068a6a8..937d01a0c5e88 100644 --- a/src/librustc_mir/transform/elaborate_drops.rs +++ b/src/librustc_mir/transform/elaborate_drops.rs @@ -41,7 +41,20 @@ impl MirPass for ElaborateDrops { let id = tcx.hir.as_local_node_id(src.def_id).unwrap(); let param_env = tcx.param_env(src.def_id).with_reveal_all(); - let move_data = MoveData::gather_moves(mir, tcx).unwrap(); + let move_data = match MoveData::gather_moves(mir, tcx) { + Ok(move_data) => move_data, + Err((move_data, _move_errors)) => { + // The only way we should be allowing any move_errors + // in here is if we are in the migration path for the + // NLL-based MIR-borrowck. + // + // If we are in the migration path, we have already + // reported these errors as warnings to the user. So + // we will just ignore them here. + assert!(tcx.migrate_borrowck()); + move_data + } + }; let elaborate_patch = { let mir = &*mir; let env = MoveDataParamEnv { From 91dc3e5b56ec36dcebbb982c4cfebb9541d14684 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Wed, 25 Jul 2018 01:34:17 +0200 Subject: [PATCH 05/10] Add scary warnings to errors-downgraded-to-warnings in borrowck=migrate. Also convert an ICE that became reachable code under borrowck=migrate into a normally reported error (which is then downgraded to a warning). This actually has a nice side benefit of providing a somewhat more useful error message, at least in the particular case of the example from issue #27282. --- src/librustc_mir/borrow_check/mod.rs | 58 ++++++++++++++----- .../borrow_check/mutability_errors.rs | 9 +++ 2 files changed, 53 insertions(+), 14 deletions(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index ad663000f938b..beedcac02f625 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -338,7 +338,13 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>( // downgrade all the buffered MIR-borrowck errors // to warnings. for err in &mut mbcx.errors_buffer { - if err.is_error() { err.level = Level::Warning; } + if err.is_error() { + err.level = Level::Warning; + err.warn("This error has been downgraded to a warning \ + for backwards compatibility with previous releases.\n\ + It represents potential unsoundness in your code.\n\ + This warning will become a hard error in the future."); + } } } SignalledError::SawSomeError => { @@ -1768,20 +1774,44 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } } - Reservation(WriteKind::Move) - | Write(WriteKind::Move) - | Reservation(WriteKind::StorageDeadOrDrop) - | Reservation(WriteKind::MutableBorrow(BorrowKind::Shared)) - | Write(WriteKind::StorageDeadOrDrop) - | Write(WriteKind::MutableBorrow(BorrowKind::Shared)) => { + Reservation(wk @ WriteKind::Move) + | Write(wk @ WriteKind::Move) + | Reservation(wk @ WriteKind::StorageDeadOrDrop) + | Reservation(wk @ WriteKind::MutableBorrow(BorrowKind::Shared)) + | Write(wk @ WriteKind::StorageDeadOrDrop) + | Write(wk @ WriteKind::MutableBorrow(BorrowKind::Shared)) => { if let Err(_place_err) = self.is_mutable(place, is_local_mutation_allowed) { - self.tcx.sess.delay_span_bug( - span, - &format!( - "Accessing `{:?}` with the kind `{:?}` shouldn't be possible", - place, kind - ), - ); + if self.tcx.migrate_borrowck() { + // rust-lang/rust#46908: In pure NLL mode this + // code path should be unreachable (and thus + // we signal an ICE in the else branch + // here). But we can legitimately get here + // under borrowck=migrate mode, so instead of + // ICE'ing we instead report a legitimate + // error (which will then be downgraded to a + // warning by the migrate machinery). + error_access = match wk { + WriteKind::MutableBorrow(_) => AccessKind::MutableBorrow, + WriteKind::Move => AccessKind::Move, + WriteKind::StorageDeadOrDrop | + WriteKind::Mutate => AccessKind::Mutate, + }; + self.report_mutability_error( + place, + span, + _place_err, + error_access, + location, + ); + } else { + self.tcx.sess.delay_span_bug( + span, + &format!( + "Accessing `{:?}` with the kind `{:?}` shouldn't be possible", + place, kind + ), + ); + } } return false; } diff --git a/src/librustc_mir/borrow_check/mutability_errors.rs b/src/librustc_mir/borrow_check/mutability_errors.rs index ab1ff1fa347d6..571a1188d49ca 100644 --- a/src/librustc_mir/borrow_check/mutability_errors.rs +++ b/src/librustc_mir/borrow_check/mutability_errors.rs @@ -24,6 +24,7 @@ use util::suggest_ref_mut; pub(super) enum AccessKind { MutableBorrow, Mutate, + Move, } impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> { @@ -110,6 +111,7 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> { if let Some(desc) = access_place_desc { item_msg = format!("`{}`", desc); reason = match error_access { + AccessKind::Move | AccessKind::Mutate => format!(" which is behind a {}", pointer_type), AccessKind::MutableBorrow => { format!(", as it is behind a {}", pointer_type) @@ -160,6 +162,13 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> { let span = match error_access { + AccessKind::Move => { + err = self.tcx + .cannot_move_out_of(span, &(item_msg + &reason), Origin::Mir); + act = "move"; + acted_on = "moved"; + span + } AccessKind::Mutate => { err = self.tcx .cannot_assign(span, &(item_msg + &reason), Origin::Mir); From f8084053280d4557c603f3cfeb6b0f12cffb46e3 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 24 Jul 2018 13:58:19 +0200 Subject: [PATCH 06/10] Test for `-Z borrowck=migrate`. Note that this test is carefully crafted to *try* to not segfault during its run. Howver, it really is representing unsound code that should be rejected after we manage to remove the AST-borrowck entirely from the compiler. --- .../ui/borrowck/borrowck-migrate-to-nll.rs | 32 +++++++++++++++++++ .../borrowck/borrowck-migrate-to-nll.stderr | 24 ++++++++++++++ 2 files changed, 56 insertions(+) create mode 100644 src/test/ui/borrowck/borrowck-migrate-to-nll.rs create mode 100644 src/test/ui/borrowck/borrowck-migrate-to-nll.stderr diff --git a/src/test/ui/borrowck/borrowck-migrate-to-nll.rs b/src/test/ui/borrowck/borrowck-migrate-to-nll.rs new file mode 100644 index 0000000000000..cac595e6ae5c3 --- /dev/null +++ b/src/test/ui/borrowck/borrowck-migrate-to-nll.rs @@ -0,0 +1,32 @@ +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// This is a test of the borrowck migrate mode. It leverages #27282, a +// bug that is fixed by NLL: this code is (unsoundly) accepted by +// AST-borrowck, but is correctly rejected by the NLL borrowck. +// +// Therefore, for backwards-compatiblity, under borrowck=migrate the +// NLL checks will be emitted as *warnings*. + +// compile-flags: -Z borrowck=migrate +// run-pass + +fn main() { + match Some(&4) { + None => {}, + ref mut foo + if { + (|| { let bar = foo; bar.take() })(); + false + } => {}, + Some(ref _s) => println!("Note this arm is bogus; the `Some` became `None` in the guard."), + _ => println!("Here is some supposedly unreachable code."), + } +} diff --git a/src/test/ui/borrowck/borrowck-migrate-to-nll.stderr b/src/test/ui/borrowck/borrowck-migrate-to-nll.stderr new file mode 100644 index 0000000000000..115b094667353 --- /dev/null +++ b/src/test/ui/borrowck/borrowck-migrate-to-nll.stderr @@ -0,0 +1,24 @@ +warning[E0507]: cannot move out of borrowed content + --> $DIR/borrowck-migrate-to-nll.rs:26:17 + | +LL | (|| { let bar = foo; bar.take() })(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot move out of borrowed content + | + = warning: This error has been downgraded to a warning for backwards compatibility with previous releases. + It represents potential unsoundness in your code. + This warning will become a hard error in the future. + +warning[E0507]: cannot move out of `foo`, as it is immutable for the pattern guard + --> $DIR/borrowck-migrate-to-nll.rs:26:17 + | +LL | (|| { let bar = foo; bar.take() })(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | | + | cannot move out of `foo`, as it is immutable for the pattern guard + | cannot move + | + = note: variables bound in patterns are immutable until the end of the pattern guard + = warning: This error has been downgraded to a warning for backwards compatibility with previous releases. + It represents potential unsoundness in your code. + This warning will become a hard error in the future. + From 1a59dafe1198b9f36d7c72e536f5d57495de1b9d Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Wed, 25 Jul 2018 16:59:03 +0200 Subject: [PATCH 07/10] Bug fix: `#![feature(nll)]` takes precedence over `-Z borrowck=migrate`. (Includes test illustrating desired behavior; compare its diagnostic output to that of the file `borrowck-migreate-to-nll.rs`.) --- src/librustc/ty/context.rs | 17 ++++++++- .../borrowck-feature-nll-overrides-migrate.rs | 37 +++++++++++++++++++ ...rowck-feature-nll-overrides-migrate.stderr | 9 +++++ 3 files changed, 62 insertions(+), 1 deletion(-) create mode 100644 src/test/ui/borrowck/borrowck-feature-nll-overrides-migrate.rs create mode 100644 src/test/ui/borrowck/borrowck-feature-nll-overrides-migrate.stderr diff --git a/src/librustc/ty/context.rs b/src/librustc/ty/context.rs index d30f656098dc1..94dc8e2901577 100644 --- a/src/librustc/ty/context.rs +++ b/src/librustc/ty/context.rs @@ -1405,9 +1405,11 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { pub fn borrowck_mode(&self) -> BorrowckMode { match self.sess.opts.borrowck_mode { mode @ BorrowckMode::Mir | - mode @ BorrowckMode::Migrate | mode @ BorrowckMode::Compare => mode, + // `BorrowckMode::Ast` is synonymous with no `-Z + // borrowck=...` flag at all. Therefore, we definitely + // want `#![feature(nll)]` to override it. mode @ BorrowckMode::Ast => { if self.features().nll { BorrowckMode::Mir @@ -1416,6 +1418,19 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { } } + // `BorrowckMode::Migrate` is modelling the behavior one + // will eventually specify via `--edition 2018`. We want + // to allow developers on the Nightly channel to opt back + // into the "hard error" mode for NLL, which they can do + // via specifying `#![feature(nll)]` explicitly in their + // crate. + mode @ BorrowckMode::Migrate => { + if self.features().nll { + BorrowckMode::Mir + } else { + mode + } + } } } diff --git a/src/test/ui/borrowck/borrowck-feature-nll-overrides-migrate.rs b/src/test/ui/borrowck/borrowck-feature-nll-overrides-migrate.rs new file mode 100644 index 0000000000000..b9cb97eeec1a1 --- /dev/null +++ b/src/test/ui/borrowck/borrowck-feature-nll-overrides-migrate.rs @@ -0,0 +1,37 @@ +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// This is a test that the `#![feature(nll)]` opt-in overrides the +// migration mode. The intention here is to emulate the goal behavior +// that `--edition 2018` effects on borrowck (modeled here by `-Z +// borrowck=migrate`) are themselves overridden by the +// `#![feature(nll)]` opt-in. +// +// Therefore, for developer convenience, under `#[feature(nll)]` the +// NLL checks will be emitted as errors *even* in the presence of `-Z +// borrowck=migrate`. + +// compile-flags: -Z borrowck=migrate + +#![feature(nll)] + +fn main() { + match Some(&4) { + None => {}, + ref mut foo + if { + (|| { let bar = foo; bar.take() })(); + //~^ ERROR cannot move out of borrowed content [E0507] + false + } => {}, + Some(ref _s) => println!("Note this arm is bogus; the `Some` became `None` in the guard."), + _ => println!("Here is some supposedly unreachable code."), + } +} diff --git a/src/test/ui/borrowck/borrowck-feature-nll-overrides-migrate.stderr b/src/test/ui/borrowck/borrowck-feature-nll-overrides-migrate.stderr new file mode 100644 index 0000000000000..f12da562f1b40 --- /dev/null +++ b/src/test/ui/borrowck/borrowck-feature-nll-overrides-migrate.stderr @@ -0,0 +1,9 @@ +error[E0507]: cannot move out of borrowed content + --> $DIR/borrowck-feature-nll-overrides-migrate.rs:30:17 + | +LL | (|| { let bar = foo; bar.take() })(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot move out of borrowed content + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0507`. From 4b2e553dac205609277607b3a192cc7fc905fc2c Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Thu, 26 Jul 2018 13:17:33 +0200 Subject: [PATCH 08/10] integrate the edition code. As a driveby change, I made `#![feature(nll)]` *always* take precedence over `-Z borrowck`. The main effect this had is that it means tests with `#![feature(nll)]` will ignore uses of `-Z borrowck=compare`. This affected only one test as far as I can tell, and I think that test used `-Z borrowck=compare` only as a historical accident. --- src/librustc/ty/context.rs | 70 ++++++++++++------- src/test/ui/generator/generator-with-nll.rs | 8 +-- .../ui/generator/generator-with-nll.stderr | 28 ++------ 3 files changed, 52 insertions(+), 54 deletions(-) diff --git a/src/librustc/ty/context.rs b/src/librustc/ty/context.rs index 94dc8e2901577..98568f860a4fd 100644 --- a/src/librustc/ty/context.rs +++ b/src/librustc/ty/context.rs @@ -74,6 +74,7 @@ use rustc_target::spec::abi; use syntax::ast::{self, NodeId}; use syntax::attr; use syntax::codemap::MultiSpan; +use syntax::edition::Edition; use syntax::feature_gate; use syntax::symbol::{Symbol, keywords, InternedString}; use syntax_pos::Span; @@ -1403,34 +1404,51 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { /// What mode(s) of borrowck should we run? AST? MIR? both? /// (Also considers the `#![feature(nll)]` setting.) pub fn borrowck_mode(&self) -> BorrowckMode { + // Here are the main constraints we need to deal with: + // + // 1. An opts.borrowck_mode of `BorrowckMode::Ast` is + // synonymous with no `-Z borrowck=...` flag at all. + // (This is arguably a historical accident.) + // + // 2. `BorrowckMode::Migrate` is the limited migration to + // NLL that we are deploying with the 2018 edition. + // + // 3. We want to allow developers on the Nightly channel + // to opt back into the "hard error" mode for NLL, + // (which they can do via specifying `#![feature(nll)]` + // explicitly in their crate). + // + // So, this precedence list is how pnkfelix chose to work with + // the above constraints: + // + // * `#![feature(nll)]` *always* means use NLL with hard + // errors. (To simplify the code here, it now even overrides + // a user's attempt to specify `-Z borrowck=compare`, which + // we arguably do not need anymore and should remove.) + // + // * Otherwise, if no `-Z borrowck=...` flag was given (or + // if `borrowck=ast` was specified), then use the default + // as required by the edition. + // + // * Otherwise, use the behavior requested via `-Z borrowck=...` + + if self.features().nll { return BorrowckMode::Mir; } + match self.sess.opts.borrowck_mode { mode @ BorrowckMode::Mir | - mode @ BorrowckMode::Compare => mode, - - // `BorrowckMode::Ast` is synonymous with no `-Z - // borrowck=...` flag at all. Therefore, we definitely - // want `#![feature(nll)]` to override it. - mode @ BorrowckMode::Ast => { - if self.features().nll { - BorrowckMode::Mir - } else { - mode - } - } - - // `BorrowckMode::Migrate` is modelling the behavior one - // will eventually specify via `--edition 2018`. We want - // to allow developers on the Nightly channel to opt back - // into the "hard error" mode for NLL, which they can do - // via specifying `#![feature(nll)]` explicitly in their - // crate. - mode @ BorrowckMode::Migrate => { - if self.features().nll { - BorrowckMode::Mir - } else { - mode - } - } + mode @ BorrowckMode::Compare | + mode @ BorrowckMode::Migrate => mode, + + BorrowckMode::Ast => match self.sess.edition() { + Edition::Edition2015 => BorrowckMode::Ast, + Edition::Edition2018 => BorrowckMode::Migrate, + + // For now, future editions mean Migrate. (But it + // would make a lot of sense for it to be changed to + // `BorrowckMode::Mir`, depending on how we plan to + // time the forcing of full migration to NLL.) + _ => BorrowckMode::Migrate, + }, } } diff --git a/src/test/ui/generator/generator-with-nll.rs b/src/test/ui/generator/generator-with-nll.rs index 3223ff4dc8b59..fdfe9e2c562e0 100644 --- a/src/test/ui/generator/generator-with-nll.rs +++ b/src/test/ui/generator/generator-with-nll.rs @@ -8,17 +8,15 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// compile-flags: -Z borrowck=compare - #![feature(generators)] #![feature(nll)] fn main() { || { // The reference in `_a` is a Legal with NLL since it ends before the yield - let _a = &mut true; //~ ERROR borrow may still be in use when generator yields (Ast) - let b = &mut true; //~ ERROR borrow may still be in use when generator yields (Ast) - //~^ borrow may still be in use when generator yields (Mir) + let _a = &mut true; + let b = &mut true; + //~^ borrow may still be in use when generator yields yield (); println!("{}", b); }; diff --git a/src/test/ui/generator/generator-with-nll.stderr b/src/test/ui/generator/generator-with-nll.stderr index 7e39d3c545903..1dc663d8bcbf7 100644 --- a/src/test/ui/generator/generator-with-nll.stderr +++ b/src/test/ui/generator/generator-with-nll.stderr @@ -1,30 +1,12 @@ -error[E0626]: borrow may still be in use when generator yields (Ast) - --> $DIR/generator-with-nll.rs:19:23 +error[E0626]: borrow may still be in use when generator yields + --> $DIR/generator-with-nll.rs:18:17 | -LL | let _a = &mut true; //~ ERROR borrow may still be in use when generator yields (Ast) - | ^^^^ -... -LL | yield (); - | -------- possible yield occurs here - -error[E0626]: borrow may still be in use when generator yields (Ast) - --> $DIR/generator-with-nll.rs:20:22 - | -LL | let b = &mut true; //~ ERROR borrow may still be in use when generator yields (Ast) - | ^^^^ -LL | //~^ borrow may still be in use when generator yields (Mir) -LL | yield (); - | -------- possible yield occurs here - -error[E0626]: borrow may still be in use when generator yields (Mir) - --> $DIR/generator-with-nll.rs:20:17 - | -LL | let b = &mut true; //~ ERROR borrow may still be in use when generator yields (Ast) +LL | let b = &mut true; | ^^^^^^^^^ -LL | //~^ borrow may still be in use when generator yields (Mir) +LL | //~^ borrow may still be in use when generator yields LL | yield (); | -------- possible yield occurs here -error: aborting due to 3 previous errors +error: aborting due to previous error For more information about this error, try `rustc --explain E0626`. From 94a2972d3f4e727c4e05246b65e4f0d8effe2d41 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Thu, 26 Jul 2018 14:49:02 +0200 Subject: [PATCH 09/10] compiletest: Add support for ignoring certain tests under `--compare-mode=...` --- src/tools/compiletest/src/header.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/tools/compiletest/src/header.rs b/src/tools/compiletest/src/header.rs index eeb280e1de36c..e9b853722c2d3 100644 --- a/src/tools/compiletest/src/header.rs +++ b/src/tools/compiletest/src/header.rs @@ -14,7 +14,7 @@ use std::io::prelude::*; use std::io::BufReader; use std::path::{Path, PathBuf}; -use common::{self, Config, Mode}; +use common::{self, CompareMode, Config, Mode}; use util; use extract_gdb_version; @@ -608,7 +608,12 @@ impl Config { common::DebugInfoLldb => name == "lldb", common::Pretty => name == "pretty", _ => false, - } || (self.target != self.host && name == "cross-compile") + } || (self.target != self.host && name == "cross-compile") || + match self.compare_mode { + Some(CompareMode::Nll) => name == "compare-mode-nll", + Some(CompareMode::Polonius) => name == "compare-mode-polonius", + None => false, + } } else { false } From 9f05f29e564c03a432df78f7c4b6421e4fb1a338 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Thu, 26 Jul 2018 14:50:55 +0200 Subject: [PATCH 10/10] Incorporate edition flag testing into tests of `-Z borrowck=migrate`. --- ...ture-nll-overrides-migrate.edition.stderr} | 2 +- .../borrowck-feature-nll-overrides-migrate.rs | 7 ++++-- ...feature-nll-overrides-migrate.zflag.stderr | 9 +++++++ ...=> borrowck-migrate-to-nll.edition.stderr} | 4 ++-- .../ui/borrowck/borrowck-migrate-to-nll.rs | 13 ++++++++-- .../borrowck-migrate-to-nll.zflag.stderr | 24 +++++++++++++++++++ 6 files changed, 52 insertions(+), 7 deletions(-) rename src/test/ui/borrowck/{borrowck-feature-nll-overrides-migrate.stderr => borrowck-feature-nll-overrides-migrate.edition.stderr} (84%) create mode 100644 src/test/ui/borrowck/borrowck-feature-nll-overrides-migrate.zflag.stderr rename src/test/ui/borrowck/{borrowck-migrate-to-nll.stderr => borrowck-migrate-to-nll.edition.stderr} (92%) create mode 100644 src/test/ui/borrowck/borrowck-migrate-to-nll.zflag.stderr diff --git a/src/test/ui/borrowck/borrowck-feature-nll-overrides-migrate.stderr b/src/test/ui/borrowck/borrowck-feature-nll-overrides-migrate.edition.stderr similarity index 84% rename from src/test/ui/borrowck/borrowck-feature-nll-overrides-migrate.stderr rename to src/test/ui/borrowck/borrowck-feature-nll-overrides-migrate.edition.stderr index f12da562f1b40..fa82efa353384 100644 --- a/src/test/ui/borrowck/borrowck-feature-nll-overrides-migrate.stderr +++ b/src/test/ui/borrowck/borrowck-feature-nll-overrides-migrate.edition.stderr @@ -1,5 +1,5 @@ error[E0507]: cannot move out of borrowed content - --> $DIR/borrowck-feature-nll-overrides-migrate.rs:30:17 + --> $DIR/borrowck-feature-nll-overrides-migrate.rs:32:17 | LL | (|| { let bar = foo; bar.take() })(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot move out of borrowed content diff --git a/src/test/ui/borrowck/borrowck-feature-nll-overrides-migrate.rs b/src/test/ui/borrowck/borrowck-feature-nll-overrides-migrate.rs index b9cb97eeec1a1..72043938f5351 100644 --- a/src/test/ui/borrowck/borrowck-feature-nll-overrides-migrate.rs +++ b/src/test/ui/borrowck/borrowck-feature-nll-overrides-migrate.rs @@ -18,7 +18,9 @@ // NLL checks will be emitted as errors *even* in the presence of `-Z // borrowck=migrate`. -// compile-flags: -Z borrowck=migrate +// revisions: zflag edition +// [zflag]compile-flags: -Z borrowck=migrate +// [edition]compile-flags: --edition 2018 #![feature(nll)] @@ -28,7 +30,8 @@ fn main() { ref mut foo if { (|| { let bar = foo; bar.take() })(); - //~^ ERROR cannot move out of borrowed content [E0507] + //[zflag]~^ ERROR cannot move out of borrowed content [E0507] + //[edition]~^^ ERROR cannot move out of borrowed content [E0507] false } => {}, Some(ref _s) => println!("Note this arm is bogus; the `Some` became `None` in the guard."), diff --git a/src/test/ui/borrowck/borrowck-feature-nll-overrides-migrate.zflag.stderr b/src/test/ui/borrowck/borrowck-feature-nll-overrides-migrate.zflag.stderr new file mode 100644 index 0000000000000..fa82efa353384 --- /dev/null +++ b/src/test/ui/borrowck/borrowck-feature-nll-overrides-migrate.zflag.stderr @@ -0,0 +1,9 @@ +error[E0507]: cannot move out of borrowed content + --> $DIR/borrowck-feature-nll-overrides-migrate.rs:32:17 + | +LL | (|| { let bar = foo; bar.take() })(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot move out of borrowed content + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0507`. diff --git a/src/test/ui/borrowck/borrowck-migrate-to-nll.stderr b/src/test/ui/borrowck/borrowck-migrate-to-nll.edition.stderr similarity index 92% rename from src/test/ui/borrowck/borrowck-migrate-to-nll.stderr rename to src/test/ui/borrowck/borrowck-migrate-to-nll.edition.stderr index 115b094667353..f5a9db364065f 100644 --- a/src/test/ui/borrowck/borrowck-migrate-to-nll.stderr +++ b/src/test/ui/borrowck/borrowck-migrate-to-nll.edition.stderr @@ -1,5 +1,5 @@ warning[E0507]: cannot move out of borrowed content - --> $DIR/borrowck-migrate-to-nll.rs:26:17 + --> $DIR/borrowck-migrate-to-nll.rs:35:17 | LL | (|| { let bar = foo; bar.take() })(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot move out of borrowed content @@ -9,7 +9,7 @@ LL | (|| { let bar = foo; bar.take() })(); This warning will become a hard error in the future. warning[E0507]: cannot move out of `foo`, as it is immutable for the pattern guard - --> $DIR/borrowck-migrate-to-nll.rs:26:17 + --> $DIR/borrowck-migrate-to-nll.rs:35:17 | LL | (|| { let bar = foo; bar.take() })(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/src/test/ui/borrowck/borrowck-migrate-to-nll.rs b/src/test/ui/borrowck/borrowck-migrate-to-nll.rs index cac595e6ae5c3..e7f2bfbfedba7 100644 --- a/src/test/ui/borrowck/borrowck-migrate-to-nll.rs +++ b/src/test/ui/borrowck/borrowck-migrate-to-nll.rs @@ -15,8 +15,17 @@ // Therefore, for backwards-compatiblity, under borrowck=migrate the // NLL checks will be emitted as *warnings*. -// compile-flags: -Z borrowck=migrate -// run-pass +// NLL mode makes this compile-fail; we cannot currently encode a +// test that is run-pass or compile-fail based on compare-mode. So +// just ignore it instead: + +// ignore-compare-mode-nll + +// revisions: zflag edition +//[zflag]compile-flags: -Z borrowck=migrate +//[edition]compile-flags: --edition 2018 +//[zflag] run-pass +//[edition] run-pass fn main() { match Some(&4) { diff --git a/src/test/ui/borrowck/borrowck-migrate-to-nll.zflag.stderr b/src/test/ui/borrowck/borrowck-migrate-to-nll.zflag.stderr new file mode 100644 index 0000000000000..f5a9db364065f --- /dev/null +++ b/src/test/ui/borrowck/borrowck-migrate-to-nll.zflag.stderr @@ -0,0 +1,24 @@ +warning[E0507]: cannot move out of borrowed content + --> $DIR/borrowck-migrate-to-nll.rs:35:17 + | +LL | (|| { let bar = foo; bar.take() })(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot move out of borrowed content + | + = warning: This error has been downgraded to a warning for backwards compatibility with previous releases. + It represents potential unsoundness in your code. + This warning will become a hard error in the future. + +warning[E0507]: cannot move out of `foo`, as it is immutable for the pattern guard + --> $DIR/borrowck-migrate-to-nll.rs:35:17 + | +LL | (|| { let bar = foo; bar.take() })(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | | + | cannot move out of `foo`, as it is immutable for the pattern guard + | cannot move + | + = note: variables bound in patterns are immutable until the end of the pattern guard + = warning: This error has been downgraded to a warning for backwards compatibility with previous releases. + It represents potential unsoundness in your code. + This warning will become a hard error in the future. +