From c00266b7ac97a8c04136e14f10eb70fb64ec2c94 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Mon, 15 Jan 2018 12:47:26 +0100 Subject: [PATCH 1/5] Encode (in MIR) whether borrows are explicit in source or arise due to autoref. This is foundation for issue 46747 (limit two-phase borrows to method-call autorefs). --- src/librustc/ich/impls_mir.rs | 20 ++++++++++++++++++- src/librustc/mir/mod.rs | 8 ++++++-- src/librustc/mir/tcx.rs | 2 +- src/librustc/mir/visit.rs | 6 ++++-- src/librustc_const_eval/pattern.rs | 4 ++-- .../borrow_check/error_reporting.rs | 8 ++++---- src/librustc_mir/borrow_check/mod.rs | 14 ++++++------- src/librustc_mir/dataflow/impls/borrows.rs | 2 +- src/librustc_mir/hair/cx/expr.rs | 15 +++++++------- src/librustc_mir/shim.rs | 5 ++++- src/librustc_mir/transform/inline.rs | 4 ++-- src/librustc_mir/transform/qualify_consts.rs | 2 +- src/librustc_mir/util/elaborate_drops.rs | 10 +++++++--- src/librustc_trans/mir/constant.rs | 2 +- 14 files changed, 67 insertions(+), 35 deletions(-) diff --git a/src/librustc/ich/impls_mir.rs b/src/librustc/ich/impls_mir.rs index f46b590d2dc59..03a369577a31e 100644 --- a/src/librustc/ich/impls_mir.rs +++ b/src/librustc/ich/impls_mir.rs @@ -20,7 +20,6 @@ use std::mem; impl_stable_hash_for!(struct mir::GeneratorLayout<'tcx> { fields }); impl_stable_hash_for!(struct mir::SourceInfo { span, scope }); impl_stable_hash_for!(enum mir::Mutability { Mut, Not }); -impl_stable_hash_for!(enum mir::BorrowKind { Shared, Unique, Mut }); impl_stable_hash_for!(enum mir::LocalKind { Var, Temp, Arg, ReturnPointer }); impl_stable_hash_for!(struct mir::LocalDecl<'tcx> { mutability, @@ -36,6 +35,25 @@ impl_stable_hash_for!(struct mir::BasicBlockData<'tcx> { statements, terminator, impl_stable_hash_for!(struct mir::UnsafetyViolation { source_info, description, kind }); impl_stable_hash_for!(struct mir::UnsafetyCheckResult { violations, unsafe_blocks }); +impl<'gcx> HashStable> +for mir::BorrowKind { + #[inline] + fn hash_stable(&self, + hcx: &mut StableHashingContext<'gcx>, + hasher: &mut StableHasher) { + mem::discriminant(self).hash_stable(hcx, hasher); + + match *self { + mir::BorrowKind::Shared | + mir::BorrowKind::Unique => {} + mir::BorrowKind::Mut { allow_two_phase_borrow } => { + allow_two_phase_borrow.hash_stable(hcx, hasher); + } + } + } +} + + impl<'gcx> HashStable> for mir::UnsafetyViolationKind { #[inline] diff --git a/src/librustc/mir/mod.rs b/src/librustc/mir/mod.rs index d82691f882c73..70bfc1e2d327f 100644 --- a/src/librustc/mir/mod.rs +++ b/src/librustc/mir/mod.rs @@ -413,7 +413,11 @@ pub enum BorrowKind { Unique, /// Data is mutable and not aliasable. - Mut, + Mut { + /// True if this borrow arose from method-call auto-ref + /// (i.e. `adjustment::Adjust::Borrow`) + allow_two_phase_borrow: bool + } } /////////////////////////////////////////////////////////////////////////// @@ -1611,7 +1615,7 @@ impl<'tcx> Debug for Rvalue<'tcx> { Ref(region, borrow_kind, ref place) => { let kind_str = match borrow_kind { BorrowKind::Shared => "", - BorrowKind::Mut | BorrowKind::Unique => "mut ", + BorrowKind::Mut { .. } | BorrowKind::Unique => "mut ", }; // When printing regions, add trailing space if necessary. diff --git a/src/librustc/mir/tcx.rs b/src/librustc/mir/tcx.rs index 53607764b3984..5433c54fb949c 100644 --- a/src/librustc/mir/tcx.rs +++ b/src/librustc/mir/tcx.rs @@ -264,7 +264,7 @@ impl<'tcx> BinOp { impl BorrowKind { pub fn to_mutbl_lossy(self) -> hir::Mutability { match self { - BorrowKind::Mut => hir::MutMutable, + BorrowKind::Mut { .. } => hir::MutMutable, BorrowKind::Shared => hir::MutImmutable, // We have no type corresponding to a unique imm borrow, so diff --git a/src/librustc/mir/visit.rs b/src/librustc/mir/visit.rs index 57ed41f2f06e6..afaf7d41e92ff 100644 --- a/src/librustc/mir/visit.rs +++ b/src/librustc/mir/visit.rs @@ -951,9 +951,10 @@ impl<'tcx> PlaceContext<'tcx> { pub fn is_mutating_use(&self) -> bool { match *self { PlaceContext::Store | PlaceContext::AsmOutput | PlaceContext::Call | - PlaceContext::Borrow { kind: BorrowKind::Mut, .. } | + PlaceContext::Borrow { kind: BorrowKind::Mut { .. }, .. } | PlaceContext::Projection(Mutability::Mut) | PlaceContext::Drop => true, + PlaceContext::Inspect | PlaceContext::Borrow { kind: BorrowKind::Shared, .. } | PlaceContext::Borrow { kind: BorrowKind::Unique, .. } | @@ -971,7 +972,8 @@ impl<'tcx> PlaceContext<'tcx> { PlaceContext::Borrow { kind: BorrowKind::Unique, .. } | PlaceContext::Projection(Mutability::Not) | PlaceContext::Copy | PlaceContext::Move => true, - PlaceContext::Borrow { kind: BorrowKind::Mut, .. } | PlaceContext::Store | + + PlaceContext::Borrow { kind: BorrowKind::Mut { .. }, .. } | PlaceContext::Store | PlaceContext::AsmOutput | PlaceContext::Call | PlaceContext::Projection(Mutability::Mut) | PlaceContext::Drop | PlaceContext::StorageLive | PlaceContext::StorageDead | diff --git a/src/librustc_const_eval/pattern.rs b/src/librustc_const_eval/pattern.rs index e0b3929e32a8d..bdb1001124de6 100644 --- a/src/librustc_const_eval/pattern.rs +++ b/src/librustc_const_eval/pattern.rs @@ -134,7 +134,7 @@ impl<'tcx> fmt::Display for Pattern<'tcx> { BindingMode::ByValue => mutability == Mutability::Mut, BindingMode::ByRef(_, bk) => { write!(f, "ref ")?; - bk == BorrowKind::Mut + match bk { BorrowKind::Mut { .. } => true, _ => false } } }; if is_mut { @@ -429,7 +429,7 @@ impl<'a, 'tcx> PatternContext<'a, 'tcx> { (Mutability::Not, BindingMode::ByValue), ty::BindByReference(hir::MutMutable) => (Mutability::Not, BindingMode::ByRef( - region.unwrap(), BorrowKind::Mut)), + region.unwrap(), BorrowKind::Mut { allow_two_phase_borrow: false })), ty::BindByReference(hir::MutImmutable) => (Mutability::Not, BindingMode::ByRef( region.unwrap(), BorrowKind::Shared)), diff --git a/src/librustc_mir/borrow_check/error_reporting.rs b/src/librustc_mir/borrow_check/error_reporting.rs index 1ea897bf27ca5..34551e8e76f59 100644 --- a/src/librustc_mir/borrow_check/error_reporting.rs +++ b/src/librustc_mir/borrow_check/error_reporting.rs @@ -256,8 +256,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { "immutable", "mutable", ) { - (BorrowKind::Shared, lft, _, BorrowKind::Mut, _, rgt) | - (BorrowKind::Mut, _, lft, BorrowKind::Shared, rgt, _) => self.tcx + (BorrowKind::Shared, lft, _, BorrowKind::Mut { .. }, _, rgt) | + (BorrowKind::Mut { .. }, _, lft, BorrowKind::Shared, rgt, _) => self.tcx .cannot_reborrow_already_borrowed( span, &desc_place, @@ -271,7 +271,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { Origin::Mir, ), - (BorrowKind::Mut, _, _, BorrowKind::Mut, _, _) => self.tcx + (BorrowKind::Mut { .. }, _, _, BorrowKind::Mut { .. }, _, _) => self.tcx .cannot_mutably_borrow_multiply( span, &desc_place, @@ -314,7 +314,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { Origin::Mir, ), - (BorrowKind::Mut, _, lft, BorrowKind::Unique, _, _) => self.tcx + (BorrowKind::Mut { .. }, _, lft, BorrowKind::Unique, _, _) => self.tcx .cannot_reborrow_already_uniquely_borrowed( span, &desc_place, diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index d90209993aa48..647cf178310ad 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -797,7 +797,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { Control::Continue } - (Read(kind), BorrowKind::Unique) | (Read(kind), BorrowKind::Mut) => { + (Read(kind), BorrowKind::Unique) | (Read(kind), BorrowKind::Mut { .. }) => { // Reading from mere reservations of mutable-borrows is OK. if this.tcx.sess.two_phase_borrows() && index.is_reservation() { @@ -828,7 +828,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } (Reservation(kind), BorrowKind::Unique) - | (Reservation(kind), BorrowKind::Mut) + | (Reservation(kind), BorrowKind::Mut { .. }) | (Activation(kind, _), _) | (Write(kind), _) => { match rw { @@ -945,7 +945,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { Rvalue::Ref(_ /*rgn*/, bk, ref place) => { let access_kind = match bk { BorrowKind::Shared => (Deep, Read(ReadKind::Borrow(bk))), - BorrowKind::Unique | BorrowKind::Mut => { + BorrowKind::Unique | BorrowKind::Mut { .. } => { let wk = WriteKind::MutableBorrow(bk); if self.tcx.sess.two_phase_borrows() { (Deep, Reservation(wk)) @@ -1196,7 +1196,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { // mutable borrow before we check it. match borrow.kind { BorrowKind::Shared => return, - BorrowKind::Unique | BorrowKind::Mut => {} + BorrowKind::Unique | BorrowKind::Mut { .. } => {} } self.access_place( @@ -1467,8 +1467,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { span_bug!(span, "&unique borrow for {:?} should not fail", place); } } - Reservation(WriteKind::MutableBorrow(BorrowKind::Mut)) - | Write(WriteKind::MutableBorrow(BorrowKind::Mut)) => if let Err(place_err) = + Reservation(WriteKind::MutableBorrow(BorrowKind::Mut { .. })) + | Write(WriteKind::MutableBorrow(BorrowKind::Mut { .. })) => if let Err(place_err) = self.is_mutable(place, is_local_mutation_allowed) { error_reported = true; @@ -1532,7 +1532,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { Activation(..) => {} // permission checks are done at Reservation point. Read(ReadKind::Borrow(BorrowKind::Unique)) - | Read(ReadKind::Borrow(BorrowKind::Mut)) + | Read(ReadKind::Borrow(BorrowKind::Mut { .. })) | Read(ReadKind::Borrow(BorrowKind::Shared)) | Read(ReadKind::Copy) => {} // Access authorized } diff --git a/src/librustc_mir/dataflow/impls/borrows.rs b/src/librustc_mir/dataflow/impls/borrows.rs index 80990bcc08089..fe9b0b86befc3 100644 --- a/src/librustc_mir/dataflow/impls/borrows.rs +++ b/src/librustc_mir/dataflow/impls/borrows.rs @@ -122,7 +122,7 @@ impl<'tcx> fmt::Display for BorrowData<'tcx> { let kind = match self.kind { mir::BorrowKind::Shared => "", mir::BorrowKind::Unique => "uniq ", - mir::BorrowKind::Mut => "mut ", + mir::BorrowKind::Mut { .. } => "mut ", }; let region = format!("{}", self.region); let region = if region.len() > 0 { format!("{} ", region) } else { region }; diff --git a/src/librustc_mir/hair/cx/expr.rs b/src/librustc_mir/hair/cx/expr.rs index 317b038c48295..e33147a915b5f 100644 --- a/src/librustc_mir/hair/cx/expr.rs +++ b/src/librustc_mir/hair/cx/expr.rs @@ -21,6 +21,7 @@ use rustc::ty::adjustment::{Adjustment, Adjust, AutoBorrow}; use rustc::ty::cast::CastKind as TyCastKind; use rustc::hir; use rustc::hir::def_id::LocalDefId; +use rustc::mir::{BorrowKind}; impl<'tcx> Mirror<'tcx> for &'tcx hir::Expr { type Output = Expr<'tcx>; @@ -111,7 +112,7 @@ fn apply_adjustment<'a, 'gcx, 'tcx>(cx: &mut Cx<'a, 'gcx, 'tcx>, span, kind: ExprKind::Borrow { region: deref.region, - borrow_kind: to_borrow_kind(deref.mutbl), + borrow_kind: to_borrow_kind(deref.mutbl, true), arg: expr.to_ref(), }, }; @@ -121,7 +122,7 @@ fn apply_adjustment<'a, 'gcx, 'tcx>(cx: &mut Cx<'a, 'gcx, 'tcx>, Adjust::Borrow(AutoBorrow::Ref(r, m)) => { ExprKind::Borrow { region: r, - borrow_kind: to_borrow_kind(m), + borrow_kind: to_borrow_kind(m, true), arg: expr.to_ref(), } } @@ -141,7 +142,7 @@ fn apply_adjustment<'a, 'gcx, 'tcx>(cx: &mut Cx<'a, 'gcx, 'tcx>, span, kind: ExprKind::Borrow { region, - borrow_kind: to_borrow_kind(m), + borrow_kind: to_borrow_kind(m, true), arg: expr.to_ref(), }, }; @@ -287,7 +288,7 @@ fn make_mirror_unadjusted<'a, 'gcx, 'tcx>(cx: &mut Cx<'a, 'gcx, 'tcx>, }; ExprKind::Borrow { region, - borrow_kind: to_borrow_kind(mutbl), + borrow_kind: to_borrow_kind(mutbl, false), arg: expr.to_ref(), } } @@ -642,9 +643,9 @@ fn method_callee<'a, 'gcx, 'tcx>(cx: &mut Cx<'a, 'gcx, 'tcx>, } } -fn to_borrow_kind(m: hir::Mutability) -> BorrowKind { +fn to_borrow_kind(m: hir::Mutability, allow_two_phase_borrow: bool) -> BorrowKind { match m { - hir::MutMutable => BorrowKind::Mut, + hir::MutMutable => BorrowKind::Mut { allow_two_phase_borrow }, hir::MutImmutable => BorrowKind::Shared, } } @@ -947,7 +948,7 @@ fn capture_freevar<'a, 'gcx, 'tcx>(cx: &mut Cx<'a, 'gcx, 'tcx>, let borrow_kind = match upvar_borrow.kind { ty::BorrowKind::ImmBorrow => BorrowKind::Shared, ty::BorrowKind::UniqueImmBorrow => BorrowKind::Unique, - ty::BorrowKind::MutBorrow => BorrowKind::Mut, + ty::BorrowKind::MutBorrow => BorrowKind::Mut { allow_two_phase_borrow: false } }; Expr { temp_lifetime, diff --git a/src/librustc_mir/shim.rs b/src/librustc_mir/shim.rs index 42ffcc194ca8c..e6ebdd3d6c167 100644 --- a/src/librustc_mir/shim.rs +++ b/src/librustc_mir/shim.rs @@ -716,11 +716,14 @@ fn build_call_shim<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, }), span )); + let borrow_kind = BorrowKind::Mut { + allow_two_phase_borrow: false, + }; statements.push(Statement { source_info, kind: StatementKind::Assign( Place::Local(ref_rcvr), - Rvalue::Ref(tcx.types.re_erased, BorrowKind::Mut, rcvr_l) + Rvalue::Ref(tcx.types.re_erased, borrow_kind, rcvr_l) ) }); Operand::Move(Place::Local(ref_rcvr)) diff --git a/src/librustc_mir/transform/inline.rs b/src/librustc_mir/transform/inline.rs index ceea97e3ed3b0..15bbcad7325be 100644 --- a/src/librustc_mir/transform/inline.rs +++ b/src/librustc_mir/transform/inline.rs @@ -426,7 +426,7 @@ impl<'a, 'tcx> Inliner<'a, 'tcx> { debug!("Creating temp for return destination"); let dest = Rvalue::Ref( self.tcx.types.re_erased, - BorrowKind::Mut, + BorrowKind::Mut { allow_two_phase_borrow: false }, destination.0); let ty = dest.ty(caller_mir, self.tcx); @@ -511,7 +511,7 @@ impl<'a, 'tcx> Inliner<'a, 'tcx> { callsite: &CallSite<'tcx>, caller_mir: &mut Mir<'tcx>) -> Local { let arg = Rvalue::Ref( self.tcx.types.re_erased, - BorrowKind::Mut, + BorrowKind::Mut { allow_two_phase_borrow: false }, arg.deref()); let ty = arg.ty(caller_mir, self.tcx); diff --git a/src/librustc_mir/transform/qualify_consts.rs b/src/librustc_mir/transform/qualify_consts.rs index da76adfd48f3f..d4ef90a7d7cf2 100644 --- a/src/librustc_mir/transform/qualify_consts.rs +++ b/src/librustc_mir/transform/qualify_consts.rs @@ -600,7 +600,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> { } let ty = place.ty(self.mir, self.tcx).to_ty(self.tcx); - if kind == BorrowKind::Mut { + if let BorrowKind::Mut { .. } = kind { // In theory, any zero-sized value could be borrowed // mutably without consequences. However, only &mut [] // is allowed right now, and only in functions. diff --git a/src/librustc_mir/util/elaborate_drops.rs b/src/librustc_mir/util/elaborate_drops.rs index 6577106801499..e2feb0ed39054 100644 --- a/src/librustc_mir/util/elaborate_drops.rs +++ b/src/librustc_mir/util/elaborate_drops.rs @@ -531,7 +531,9 @@ impl<'l, 'b, 'tcx, D> DropCtxt<'l, 'b, 'tcx, D> let result = BasicBlockData { statements: vec![self.assign( &Place::Local(ref_place), - Rvalue::Ref(tcx.types.re_erased, BorrowKind::Mut, self.place.clone()) + Rvalue::Ref(tcx.types.re_erased, + BorrowKind::Mut { allow_two_phase_borrow: false }, + self.place.clone()) )], terminator: Some(Terminator { kind: TerminatorKind::Call { @@ -591,7 +593,7 @@ impl<'l, 'b, 'tcx, D> DropCtxt<'l, 'b, 'tcx, D> } else { (Rvalue::Ref( tcx.types.re_erased, - BorrowKind::Mut, + BorrowKind::Mut { allow_two_phase_borrow: false }, self.place.clone().index(cur)), Rvalue::BinaryOp(BinOp::Add, copy(&Place::Local(cur)), one)) }; @@ -735,7 +737,9 @@ impl<'l, 'b, 'tcx, D> DropCtxt<'l, 'b, 'tcx, D> // cur = tmp as *mut T; // end = Offset(cur, len); drop_block_stmts.push(self.assign(&tmp, Rvalue::Ref( - tcx.types.re_erased, BorrowKind::Mut, self.place.clone() + tcx.types.re_erased, + BorrowKind::Mut { allow_two_phase_borrow: false }, + self.place.clone() ))); drop_block_stmts.push(self.assign(&cur, Rvalue::Cast( CastKind::Misc, Operand::Move(tmp.clone()), iter_ty diff --git a/src/librustc_trans/mir/constant.rs b/src/librustc_trans/mir/constant.rs index cd1975488a24a..49b4ef0d38549 100644 --- a/src/librustc_trans/mir/constant.rs +++ b/src/librustc_trans/mir/constant.rs @@ -870,7 +870,7 @@ impl<'a, 'tcx> MirConstContext<'a, 'tcx> { } else { self.cx.tcx.data_layout.pointer_align }; - if bk == mir::BorrowKind::Mut { + if let mir::BorrowKind::Mut { .. } = bk { consts::addr_of_mut(self.cx, llval, align, "ref_mut") } else { consts::addr_of(self.cx, llval, align, "ref") From 1855ab742458cc4359e27deadbdf3d8747ce361d Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 16 Jan 2018 10:52:52 +0100 Subject: [PATCH 2/5] Restrict two-phase borrows to solely borrows introduced via autoref. Added `-Z two-phase-beyond-autoref` to bring back old behavior (mainly to allow demonstration of desugared examples). Updated tests to use aforementioned flag when necessary. (But in each case where I added the flag, I made sure to also include a revision without the flag so that one can readily see what the actual behavior we expect is for the initial deployment of NLL.) --- src/librustc/mir/mod.rs | 9 +++++ src/librustc/session/config.rs | 2 ++ src/librustc_mir/borrow_check/mod.rs | 13 +++++-- ...o-phase-activation-sharing-interference.rs | 34 ++++++++++++++----- ...o-phase-allow-access-during-reservation.rs | 23 ++++++++++--- ...-phase-reservation-sharing-interference.rs | 28 ++++++++++++--- 6 files changed, 89 insertions(+), 20 deletions(-) diff --git a/src/librustc/mir/mod.rs b/src/librustc/mir/mod.rs index 70bfc1e2d327f..c035d02a61232 100644 --- a/src/librustc/mir/mod.rs +++ b/src/librustc/mir/mod.rs @@ -420,6 +420,15 @@ pub enum BorrowKind { } } +impl BorrowKind { + pub fn allows_two_phase_borrow(&self) -> bool { + match *self { + BorrowKind::Shared | BorrowKind::Unique => false, + BorrowKind::Mut { allow_two_phase_borrow } => allow_two_phase_borrow, + } + } +} + /////////////////////////////////////////////////////////////////////////// // Variables and temps diff --git a/src/librustc/session/config.rs b/src/librustc/session/config.rs index c56575f432d1e..76564b9b7d2ac 100644 --- a/src/librustc/session/config.rs +++ b/src/librustc/session/config.rs @@ -1085,6 +1085,8 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options, "select which borrowck is used (`ast`, `mir`, 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], + "when using two-phase-borrows, allow two phases even for non-autoref `&mut` borrows"), time_passes: bool = (false, parse_bool, [UNTRACKED], "measure time of each rustc pass"), count_llvm_insns: bool = (false, parse_bool, diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 647cf178310ad..217dfdf8d416e 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -707,6 +707,15 @@ impl InitializationRequiringAction { } impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { + /// Returns true if the borrow represented by `kind` is + /// allowed to be split into separate Reservation and + /// Activation phases. + fn allow_two_phase_borrow(&self, kind: BorrowKind) -> bool { + self.tcx.sess.two_phase_borrows() && + (kind.allows_two_phase_borrow() || + self.tcx.sess.opts.debugging_opts.two_phase_beyond_autoref) + } + /// Checks an access to the given place to see if it is allowed. Examines the set of borrows /// that are in scope, as well as which paths have been initialized, to ensure that (a) the /// place is initialized and (b) it is not borrowed in some way that would prevent this @@ -799,7 +808,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { (Read(kind), BorrowKind::Unique) | (Read(kind), BorrowKind::Mut { .. }) => { // Reading from mere reservations of mutable-borrows is OK. - if this.tcx.sess.two_phase_borrows() && index.is_reservation() + if this.allow_two_phase_borrow(borrow.kind) && index.is_reservation() { return Control::Continue; } @@ -947,7 +956,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { BorrowKind::Shared => (Deep, Read(ReadKind::Borrow(bk))), BorrowKind::Unique | BorrowKind::Mut { .. } => { let wk = WriteKind::MutableBorrow(bk); - if self.tcx.sess.two_phase_borrows() { + if self.allow_two_phase_borrow(bk) { (Deep, Reservation(wk)) } else { (Deep, Write(wk)) diff --git a/src/test/compile-fail/borrowck/two-phase-activation-sharing-interference.rs b/src/test/compile-fail/borrowck/two-phase-activation-sharing-interference.rs index a9797e4d215a5..709c00ba8464a 100644 --- a/src/test/compile-fail/borrowck/two-phase-activation-sharing-interference.rs +++ b/src/test/compile-fail/borrowck/two-phase-activation-sharing-interference.rs @@ -8,9 +8,13 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// revisions: lxl nll -//[lxl]compile-flags: -Z borrowck=mir -Z two-phase-borrows -//[nll]compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z nll +// ignore-tidy-linelength + +// revisions: lxl_beyond nll_beyond nll_target + +//[lxl_beyond] compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z two-phase-beyond-autoref +//[nll_beyond] compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z two-phase-beyond-autoref -Z nll +//[nll_target] compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z nll // This is an important corner case pointed out by Niko: one is // allowed to initiate a shared borrow during a reservation, but it @@ -18,6 +22,14 @@ // // FIXME: for clarity, diagnostics for these cases might be better off // if they specifically said "cannot activate mutable borrow of `x`" +// +// The convention for the listed revisions: "lxl" means lexical +// lifetimes (which can be easier to reason about). "nll" means +// non-lexical lifetimes. "nll_target" means the initial conservative +// two-phase borrows that only applies to autoref-introduced borrows. +// "nll_beyond" means the generalization of two-phase borrows to all +// `&mut`-borrows (doing so makes it easier to write code for specific +// corner cases). #![allow(dead_code)] @@ -27,6 +39,7 @@ fn ok() { let mut x = 3; let y = &mut x; { let z = &x; read(z); } + //[nll_target]~^ ERROR cannot borrow `x` as immutable because it is also borrowed as mutable *y += 1; } @@ -34,9 +47,11 @@ fn not_ok() { let mut x = 3; let y = &mut x; let z = &x; + //[nll_target]~^ ERROR cannot borrow `x` as immutable because it is also borrowed as mutable *y += 1; - //[lxl]~^ ERROR cannot borrow `x` as mutable because it is also borrowed as immutable - //[nll]~^^ ERROR cannot borrow `x` as mutable because it is also borrowed as immutable + //[lxl_beyond]~^ ERROR cannot borrow `x` as mutable because it is also borrowed as immutable + //[nll_beyond]~^^ ERROR cannot borrow `x` as mutable because it is also borrowed as immutable + //[nll_target]~^^^ ERROR cannot borrow `x` as mutable because it is also borrowed as immutable read(z); } @@ -44,18 +59,21 @@ fn should_be_ok_with_nll() { let mut x = 3; let y = &mut x; let z = &x; + //[nll_target]~^ ERROR cannot borrow `x` as immutable because it is also borrowed as mutable read(z); *y += 1; - //[lxl]~^ ERROR cannot borrow `x` as mutable because it is also borrowed as immutable - // (okay with nll today) + //[lxl_beyond]~^ ERROR cannot borrow `x` as mutable because it is also borrowed as immutable + // (okay with (generalized) nll today) } fn should_also_eventually_be_ok_with_nll() { let mut x = 3; let y = &mut x; let _z = &x; + //[nll_target]~^ ERROR cannot borrow `x` as immutable because it is also borrowed as mutable *y += 1; - //[lxl]~^ ERROR cannot borrow `x` as mutable because it is also borrowed as immutable + //[lxl_beyond]~^ ERROR cannot borrow `x` as mutable because it is also borrowed as immutable + // (okay with (generalized) nll today) } fn main() { } diff --git a/src/test/compile-fail/borrowck/two-phase-allow-access-during-reservation.rs b/src/test/compile-fail/borrowck/two-phase-allow-access-during-reservation.rs index 7695bd3e4652c..dd174981fb1e2 100644 --- a/src/test/compile-fail/borrowck/two-phase-allow-access-during-reservation.rs +++ b/src/test/compile-fail/borrowck/two-phase-allow-access-during-reservation.rs @@ -8,9 +8,12 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// revisions: lxl nll -//[lxl]compile-flags: -Z borrowck=mir -Z two-phase-borrows -//[nll]compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z nll +// ignore-tidy-linelength + +// revisions: lxl_beyond nll_beyond nll_target +//[lxl_beyond] compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z two_phase_beyond_autoref +//[nll_beyond] compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z two_phase_beyond_autoref -Z nll +//[nll_target] compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z nll // This is the second counter-example from Niko's blog post // smallcultfollowing.com/babysteps/blog/2017/03/01/nested-method-calls-via-two-phase-borrowing/ @@ -18,6 +21,14 @@ // It is "artificial". It is meant to illustrate directly that we // should allow an aliasing access during reservation, but *not* while // the mutable borrow is active. +// +// The convention for the listed revisions: "lxl" means lexical +// lifetimes (which can be easier to reason about). "nll" means +// non-lexical lifetimes. "nll_target" means the initial conservative +// two-phase borrows that only applies to autoref-introduced borrows. +// "nll_beyond" means the generalization of two-phase borrows to all +// `&mut`-borrows (doing so makes it easier to write code for specific +// corner cases). fn main() { /*0*/ let mut i = 0; @@ -25,11 +36,13 @@ fn main() { /*1*/ let p = &mut i; // (reservation of `i` starts here) /*2*/ let j = i; // OK: `i` is only reserved here + //[nll_target]~^ ERROR cannot use `i` because it was mutably borrowed [E0503] /*3*/ *p += 1; // (mutable borrow of `i` starts here, since `p` is used) - /*4*/ let k = i; //[lxl]~ ERROR cannot use `i` because it was mutably borrowed [E0503] - //[nll]~^ ERROR cannot use `i` because it was mutably borrowed [E0503] + /*4*/ let k = i; //[lxl_beyond]~ ERROR cannot use `i` because it was mutably borrowed [E0503] + //[nll_beyond]~^ ERROR cannot use `i` because it was mutably borrowed [E0503] + //[nll_target]~^^ ERROR cannot use `i` because it was mutably borrowed [E0503] /*5*/ *p += 1; diff --git a/src/test/compile-fail/borrowck/two-phase-reservation-sharing-interference.rs b/src/test/compile-fail/borrowck/two-phase-reservation-sharing-interference.rs index cc85315263a4f..b5fda4985f23f 100644 --- a/src/test/compile-fail/borrowck/two-phase-reservation-sharing-interference.rs +++ b/src/test/compile-fail/borrowck/two-phase-reservation-sharing-interference.rs @@ -8,9 +8,13 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// revisions: lxl nll -//[lxl]compile-flags: -Z borrowck=mir -Z two-phase-borrows -//[nll]compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z nll +// ignore-tidy-linelength + +// revisions: lxl_beyond nll_beyond nll_target + +//[lxl_beyond]compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z two-phase-beyond-autoref +//[nll_beyond]compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z two-phase-beyond-autoref -Z nll +//[nll_target]compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z nll // This is a corner case that the current implementation is (probably) // treating more conservatively than is necessary. But it also does @@ -19,6 +23,18 @@ // So this test is just making a note of the current behavior, with // the caveat that in the future, the rules may be loosened, at which // point this test might be thrown out. +// +// The convention for the listed revisions: "lxl" means lexical +// lifetimes (which can be easier to reason about). "nll" means +// non-lexical lifetimes. "nll_target" means the initial conservative +// two-phase borrows that only applies to autoref-introduced borrows. +// "nll_beyond" means the generalization of two-phase borrows to all +// `&mut`-borrows (doing so makes it easier to write code for specific +// corner cases). +// +// FIXME: in "nll_target", we currently see the same error reported +// twice. This is injected by `-Z two-phase-borrows`; not sure why as +// of yet. fn main() { let mut vec = vec![0, 1]; @@ -30,8 +46,10 @@ fn main() { // with the shared borrow. But in the current implementation, // its an error. delay = &mut vec; - //[lxl]~^ ERROR cannot borrow `vec` as mutable because it is also borrowed as immutable - //[nll]~^^ ERROR cannot borrow `vec` as mutable because it is also borrowed as immutable + //[lxl_beyond]~^ ERROR cannot borrow `vec` as mutable because it is also borrowed as immutable + //[nll_beyond]~^^ ERROR cannot borrow `vec` as mutable because it is also borrowed as immutable + //[nll_target]~^^^ ERROR cannot borrow `vec` as mutable because it is also borrowed as immutable + //[nll_target]~| ERROR cannot borrow `vec` as mutable because it is also borrowed as immutable shared[0]; } From c8041dd8ac780425f880e5e4e00d055e3bd0bece Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 23 Jan 2018 13:31:11 +0100 Subject: [PATCH 3/5] Add `AutoBorrowMutability`; its like `hir::Mutability` but w/ two-phase borrow info too. Namely, the mutable borrows also carries a flag indicating whether they should support two-phase borrows. This allows us to thread down, from the point of the borrow's introduction, whether the particular adjustment that created it is one that yields two-phase mutable borrows. --- src/librustc/ich/impls_ty.rs | 14 +++++++++ src/librustc/middle/expr_use_visitor.rs | 2 +- src/librustc/ty/adjustment.rs | 17 ++++++++++- src/librustc_lint/unused.rs | 6 ++-- src/librustc_mir/hair/cx/expr.rs | 33 +++++++++++++++------ src/librustc_typeck/check/callee.rs | 13 ++++++-- src/librustc_typeck/check/coercion.rs | 22 ++++++++++++-- src/librustc_typeck/check/method/confirm.rs | 23 ++++++++++++-- src/librustc_typeck/check/mod.rs | 26 ++++++++++++++-- src/librustc_typeck/check/op.rs | 24 +++++++++++++-- src/librustc_typeck/check/regionck.rs | 2 +- 11 files changed, 154 insertions(+), 28 deletions(-) diff --git a/src/librustc/ich/impls_ty.rs b/src/librustc/ich/impls_ty.rs index 107779ec3fa15..d1e431597e745 100644 --- a/src/librustc/ich/impls_ty.rs +++ b/src/librustc/ich/impls_ty.rs @@ -163,6 +163,20 @@ impl_stable_hash_for!(struct ty::adjustment::Adjustment<'tcx> { kind, target }); impl_stable_hash_for!(struct ty::adjustment::OverloadedDeref<'tcx> { region, mutbl }); impl_stable_hash_for!(struct ty::UpvarBorrow<'tcx> { kind, region }); +impl<'gcx> HashStable> for ty::adjustment::AutoBorrowMutability { + fn hash_stable(&self, + hcx: &mut StableHashingContext<'gcx>, + hasher: &mut StableHasher) { + mem::discriminant(self).hash_stable(hcx, hasher); + match *self { + ty::adjustment::AutoBorrowMutability::Mutable { ref allow_two_phase_borrow } => { + allow_two_phase_borrow.hash_stable(hcx, hasher); + } + ty::adjustment::AutoBorrowMutability::Immutable => {} + } + } +} + impl_stable_hash_for!(struct ty::UpvarId { var_id, closure_expr_id }); impl_stable_hash_for!(enum ty::BorrowKind { diff --git a/src/librustc/middle/expr_use_visitor.rs b/src/librustc/middle/expr_use_visitor.rs index c69005101c671..7db75a5166898 100644 --- a/src/librustc/middle/expr_use_visitor.rs +++ b/src/librustc/middle/expr_use_visitor.rs @@ -760,7 +760,7 @@ impl<'a, 'gcx, 'tcx> ExprUseVisitor<'a, 'gcx, 'tcx> { expr.span, cmt_base, r, - ty::BorrowKind::from_mutbl(m), + ty::BorrowKind::from_mutbl(m.into()), AutoRef); } diff --git a/src/librustc/ty/adjustment.rs b/src/librustc/ty/adjustment.rs index 96d69b4fba21a..7579d95a8fe68 100644 --- a/src/librustc/ty/adjustment.rs +++ b/src/librustc/ty/adjustment.rs @@ -119,10 +119,25 @@ impl<'a, 'gcx, 'tcx> OverloadedDeref<'tcx> { } } +#[derive(Copy, Clone, PartialEq, Debug, RustcEncodable, RustcDecodable)] +pub enum AutoBorrowMutability { + Mutable { allow_two_phase_borrow: bool }, + Immutable, +} + +impl From for hir::Mutability { + fn from(m: AutoBorrowMutability) -> Self { + match m { + AutoBorrowMutability::Mutable { .. } => hir::MutMutable, + AutoBorrowMutability::Immutable => hir::MutImmutable, + } + } +} + #[derive(Copy, Clone, PartialEq, Debug, RustcEncodable, RustcDecodable)] pub enum AutoBorrow<'tcx> { /// Convert from T to &T. - Ref(ty::Region<'tcx>, hir::Mutability), + Ref(ty::Region<'tcx>, AutoBorrowMutability), /// Convert from T to *T. RawPtr(hir::Mutability), diff --git a/src/librustc_lint/unused.rs b/src/librustc_lint/unused.rs index 56f863ab3aa84..439533fae49d9 100644 --- a/src/librustc_lint/unused.rs +++ b/src/librustc_lint/unused.rs @@ -437,8 +437,10 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnusedAllocation { for adj in cx.tables.expr_adjustments(e) { if let adjustment::Adjust::Borrow(adjustment::AutoBorrow::Ref(_, m)) = adj.kind { let msg = match m { - hir::MutImmutable => "unnecessary allocation, use & instead", - hir::MutMutable => "unnecessary allocation, use &mut instead" + adjustment::AutoBorrowMutability::Immutable => + "unnecessary allocation, use & instead", + adjustment::AutoBorrowMutability::Mutable { .. }=> + "unnecessary allocation, use &mut instead" }; cx.span_lint(UNUSED_ALLOCATION, e.span, msg); } diff --git a/src/librustc_mir/hair/cx/expr.rs b/src/librustc_mir/hair/cx/expr.rs index e33147a915b5f..00ab2e4599528 100644 --- a/src/librustc_mir/hair/cx/expr.rs +++ b/src/librustc_mir/hair/cx/expr.rs @@ -17,7 +17,7 @@ use hair::cx::to_ref::ToRef; use rustc::hir::def::{Def, CtorKind}; use rustc::middle::const_val::ConstVal; use rustc::ty::{self, AdtKind, VariantDef, Ty}; -use rustc::ty::adjustment::{Adjustment, Adjust, AutoBorrow}; +use rustc::ty::adjustment::{Adjustment, Adjust, AutoBorrow, AutoBorrowMutability}; use rustc::ty::cast::CastKind as TyCastKind; use rustc::hir; use rustc::hir::def_id::LocalDefId; @@ -112,7 +112,7 @@ fn apply_adjustment<'a, 'gcx, 'tcx>(cx: &mut Cx<'a, 'gcx, 'tcx>, span, kind: ExprKind::Borrow { region: deref.region, - borrow_kind: to_borrow_kind(deref.mutbl, true), + borrow_kind: deref.mutbl.to_borrow_kind(), arg: expr.to_ref(), }, }; @@ -122,7 +122,7 @@ fn apply_adjustment<'a, 'gcx, 'tcx>(cx: &mut Cx<'a, 'gcx, 'tcx>, Adjust::Borrow(AutoBorrow::Ref(r, m)) => { ExprKind::Borrow { region: r, - borrow_kind: to_borrow_kind(m, true), + borrow_kind: m.to_borrow_kind(), arg: expr.to_ref(), } } @@ -142,7 +142,7 @@ fn apply_adjustment<'a, 'gcx, 'tcx>(cx: &mut Cx<'a, 'gcx, 'tcx>, span, kind: ExprKind::Borrow { region, - borrow_kind: to_borrow_kind(m, true), + borrow_kind: m.to_borrow_kind(), arg: expr.to_ref(), }, }; @@ -288,7 +288,7 @@ fn make_mirror_unadjusted<'a, 'gcx, 'tcx>(cx: &mut Cx<'a, 'gcx, 'tcx>, }; ExprKind::Borrow { region, - borrow_kind: to_borrow_kind(mutbl, false), + borrow_kind: mutbl.to_borrow_kind(), arg: expr.to_ref(), } } @@ -643,10 +643,25 @@ fn method_callee<'a, 'gcx, 'tcx>(cx: &mut Cx<'a, 'gcx, 'tcx>, } } -fn to_borrow_kind(m: hir::Mutability, allow_two_phase_borrow: bool) -> BorrowKind { - match m { - hir::MutMutable => BorrowKind::Mut { allow_two_phase_borrow }, - hir::MutImmutable => BorrowKind::Shared, +trait ToBorrowKind { fn to_borrow_kind(&self) -> BorrowKind; } + +impl ToBorrowKind for AutoBorrowMutability { + fn to_borrow_kind(&self) -> BorrowKind { + match *self { + AutoBorrowMutability::Mutable { allow_two_phase_borrow } => + BorrowKind::Mut { allow_two_phase_borrow }, + AutoBorrowMutability::Immutable => + BorrowKind::Shared, + } + } +} + +impl ToBorrowKind for hir::Mutability { + fn to_borrow_kind(&self) -> BorrowKind { + match *self { + hir::MutMutable => BorrowKind::Mut { allow_two_phase_borrow: false }, + hir::MutImmutable => BorrowKind::Shared, + } } } diff --git a/src/librustc_typeck/check/callee.rs b/src/librustc_typeck/check/callee.rs index 76df9be48386d..3d61ffe39336a 100644 --- a/src/librustc_typeck/check/callee.rs +++ b/src/librustc_typeck/check/callee.rs @@ -16,7 +16,7 @@ use hir::def::Def; use hir::def_id::{DefId, LOCAL_CRATE}; use rustc::{infer, traits}; use rustc::ty::{self, TyCtxt, TypeFoldable, Ty}; -use rustc::ty::adjustment::{Adjustment, Adjust, AutoBorrow}; +use rustc::ty::adjustment::{Adjustment, Adjust, AutoBorrow, AutoBorrowMutability}; use syntax::abi; use syntax::symbol::Symbol; use syntax_pos::Span; @@ -176,8 +176,17 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { let mut autoref = None; if borrow { if let ty::TyRef(region, mt) = method.sig.inputs()[0].sty { + let mutbl = match mt.mutbl { + hir::MutImmutable => AutoBorrowMutability::Immutable, + hir::MutMutable => AutoBorrowMutability::Mutable { + // For initial two-phase borrow + // deployment, conservatively omit + // overloaded function call ops. + allow_two_phase_borrow: false, + } + }; autoref = Some(Adjustment { - kind: Adjust::Borrow(AutoBorrow::Ref(region, mt.mutbl)), + kind: Adjust::Borrow(AutoBorrow::Ref(region, mutbl)), target: method.sig.inputs()[0] }); } diff --git a/src/librustc_typeck/check/coercion.rs b/src/librustc_typeck/check/coercion.rs index d0280bf0b30be..47e4b0272bed4 100644 --- a/src/librustc_typeck/check/coercion.rs +++ b/src/librustc_typeck/check/coercion.rs @@ -68,7 +68,7 @@ use rustc::infer::{Coercion, InferResult, InferOk}; use rustc::infer::type_variable::TypeVariableOrigin; use rustc::lint; use rustc::traits::{self, ObligationCause, ObligationCauseCode}; -use rustc::ty::adjustment::{Adjustment, Adjust, AutoBorrow}; +use rustc::ty::adjustment::{Adjustment, Adjust, AutoBorrow, AutoBorrowMutability}; use rustc::ty::{self, TypeAndMut, Ty, ClosureSubsts}; use rustc::ty::fold::TypeFoldable; use rustc::ty::error::TypeError; @@ -421,8 +421,17 @@ impl<'f, 'gcx, 'tcx> Coerce<'f, 'gcx, 'tcx> { ty::TyRef(r_borrow, _) => r_borrow, _ => span_bug!(span, "expected a ref type, got {:?}", ty), }; + let mutbl = match mt_b.mutbl { + hir::MutImmutable => AutoBorrowMutability::Immutable, + hir::MutMutable => AutoBorrowMutability::Mutable { + // Deref-coercion is a case where we deliberately + // disallow two-phase borrows in its initial + // deployment; see discussion on PR #47489. + allow_two_phase_borrow: false, + } + }; adjustments.push(Adjustment { - kind: Adjust::Borrow(AutoBorrow::Ref(r_borrow, mt_b.mutbl)), + kind: Adjust::Borrow(AutoBorrow::Ref(r_borrow, mutbl)), target: ty }); @@ -461,11 +470,17 @@ impl<'f, 'gcx, 'tcx> Coerce<'f, 'gcx, 'tcx> { let coercion = Coercion(self.cause.span); let r_borrow = self.next_region_var(coercion); + let mutbl = match mt_b.mutbl { + hir::MutImmutable => AutoBorrowMutability::Immutable, + hir::MutMutable => AutoBorrowMutability::Mutable { + allow_two_phase_borrow: false, + } + }; Some((Adjustment { kind: Adjust::Deref(None), target: mt_a.ty }, Adjustment { - kind: Adjust::Borrow(AutoBorrow::Ref(r_borrow, mt_b.mutbl)), + kind: Adjust::Borrow(AutoBorrow::Ref(r_borrow, mutbl)), target: self.tcx.mk_ref(r_borrow, ty::TypeAndMut { mutbl: mt_b.mutbl, ty: mt_a.ty @@ -871,6 +886,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { ] => { match self.node_ty(expr.hir_id).sty { ty::TyRef(_, mt_orig) => { + let mutbl_adj: hir::Mutability = mutbl_adj.into(); // Reborrow that we can safely ignore, because // the next adjustment can only be a Deref // which will be merged into it. diff --git a/src/librustc_typeck/check/method/confirm.rs b/src/librustc_typeck/check/method/confirm.rs index 7f5b353f79ef7..20d5899149645 100644 --- a/src/librustc_typeck/check/method/confirm.rs +++ b/src/librustc_typeck/check/method/confirm.rs @@ -17,7 +17,7 @@ use rustc::ty::subst::Substs; use rustc::traits; use rustc::ty::{self, Ty}; use rustc::ty::subst::Subst; -use rustc::ty::adjustment::{Adjustment, Adjust, AutoBorrow, OverloadedDeref}; +use rustc::ty::adjustment::{Adjustment, Adjust, AutoBorrow, AutoBorrowMutability, OverloadedDeref}; use rustc::ty::fold::TypeFoldable; use rustc::infer::{self, InferOk}; use syntax_pos::Span; @@ -165,6 +165,14 @@ impl<'a, 'gcx, 'tcx> ConfirmContext<'a, 'gcx, 'tcx> { mutbl, ty: target }); + let mutbl = match mutbl { + hir::MutImmutable => AutoBorrowMutability::Immutable, + hir::MutMutable => AutoBorrowMutability::Mutable { + // Method call receivers are the primary use case + // for two-phase borrows. + allow_two_phase_borrow: true, + } + }; adjustments.push(Adjustment { kind: Adjust::Borrow(AutoBorrow::Ref(region, mutbl)), target @@ -172,7 +180,7 @@ impl<'a, 'gcx, 'tcx> ConfirmContext<'a, 'gcx, 'tcx> { if let Some(unsize_target) = pick.unsize { target = self.tcx.mk_ref(region, ty::TypeAndMut { - mutbl, + mutbl: mutbl.into(), ty: unsize_target }); adjustments.push(Adjustment { @@ -530,10 +538,19 @@ impl<'a, 'gcx, 'tcx> ConfirmContext<'a, 'gcx, 'tcx> { for adjustment in &mut adjustments[..] { if let Adjust::Borrow(AutoBorrow::Ref(..)) = adjustment.kind { debug!("convert_place_op_to_mutable: converting autoref {:?}", adjustment); + let mutbl = match mutbl { + hir::MutImmutable => AutoBorrowMutability::Immutable, + hir::MutMutable => AutoBorrowMutability::Mutable { + // For initial two-phase borrow + // deployment, conservatively omit + // overloaded operators. + allow_two_phase_borrow: false, + } + }; adjustment.kind = Adjust::Borrow(AutoBorrow::Ref(region, mutbl)); adjustment.target = self.tcx.mk_ref(region, ty::TypeAndMut { ty: source, - mutbl + mutbl: mutbl.into(), }); } source = adjustment.target; diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index 363d4a9dc0cd3..47bf085c9c05b 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -96,7 +96,7 @@ use rustc::middle::region; use rustc::ty::subst::{Kind, Subst, Substs}; use rustc::traits::{self, FulfillmentContext, ObligationCause, ObligationCauseCode}; use rustc::ty::{self, Ty, TyCtxt, Visibility, ToPredicate}; -use rustc::ty::adjustment::{Adjust, Adjustment, AutoBorrow}; +use rustc::ty::adjustment::{Adjust, Adjustment, AutoBorrow, AutoBorrowMutability}; use rustc::ty::fold::TypeFoldable; use rustc::ty::maps::Providers; use rustc::ty::util::{Representability, IntTypeExt}; @@ -2357,8 +2357,19 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { let mut adjustments = autoderef.adjust_steps(needs); if let ty::TyRef(region, mt) = method.sig.inputs()[0].sty { + let mutbl = match mt.mutbl { + hir::MutImmutable => AutoBorrowMutability::Immutable, + hir::MutMutable => AutoBorrowMutability::Mutable { + // FIXME (#46747): arguably indexing is + // "just another kind of call"; perhaps it + // would be more consistent to allow + // two-phase borrows for .index() + // receivers here. + allow_two_phase_borrow: false, + } + }; adjustments.push(Adjustment { - kind: Adjust::Borrow(AutoBorrow::Ref(region, mt.mutbl)), + kind: Adjust::Borrow(AutoBorrow::Ref(region, mutbl)), target: self.tcx.mk_ref(region, ty::TypeAndMut { mutbl: mt.mutbl, ty: adjusted_ty @@ -3646,8 +3657,17 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { expr.span, oprnd_t, needs) { let method = self.register_infer_ok_obligations(ok); if let ty::TyRef(region, mt) = method.sig.inputs()[0].sty { + let mutbl = match mt.mutbl { + hir::MutImmutable => AutoBorrowMutability::Immutable, + hir::MutMutable => AutoBorrowMutability::Mutable { + // (It shouldn't actually matter for unary ops whether + // we enable two-phase borrows or not, since a unary + // op has no additional operands.) + allow_two_phase_borrow: false, + } + }; self.apply_adjustments(oprnd, vec![Adjustment { - kind: Adjust::Borrow(AutoBorrow::Ref(region, mt.mutbl)), + kind: Adjust::Borrow(AutoBorrow::Ref(region, mutbl)), target: method.sig.inputs()[0] }]); } diff --git a/src/librustc_typeck/check/op.rs b/src/librustc_typeck/check/op.rs index 0698e3ecb6edd..a6776a0fe8612 100644 --- a/src/librustc_typeck/check/op.rs +++ b/src/librustc_typeck/check/op.rs @@ -14,7 +14,7 @@ use super::{FnCtxt, Needs}; use super::method::MethodCallee; use rustc::ty::{self, Ty, TypeFoldable, TypeVariants}; use rustc::ty::TypeVariants::{TyStr, TyRef}; -use rustc::ty::adjustment::{Adjustment, Adjust, AutoBorrow}; +use rustc::ty::adjustment::{Adjustment, Adjust, AutoBorrow, AutoBorrowMutability}; use rustc::infer::type_variable::TypeVariableOrigin; use errors; use syntax_pos::Span; @@ -198,8 +198,17 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { let by_ref_binop = !op.node.is_by_value(); if is_assign == IsAssign::Yes || by_ref_binop { if let ty::TyRef(region, mt) = method.sig.inputs()[0].sty { + let mutbl = match mt.mutbl { + hir::MutImmutable => AutoBorrowMutability::Immutable, + hir::MutMutable => AutoBorrowMutability::Mutable { + // For initial two-phase borrow + // deployment, conservatively omit + // overloaded binary ops. + allow_two_phase_borrow: false, + } + }; let autoref = Adjustment { - kind: Adjust::Borrow(AutoBorrow::Ref(region, mt.mutbl)), + kind: Adjust::Borrow(AutoBorrow::Ref(region, mutbl)), target: method.sig.inputs()[0] }; self.apply_adjustments(lhs_expr, vec![autoref]); @@ -207,8 +216,17 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { } if by_ref_binop { if let ty::TyRef(region, mt) = method.sig.inputs()[1].sty { + let mutbl = match mt.mutbl { + hir::MutImmutable => AutoBorrowMutability::Immutable, + hir::MutMutable => AutoBorrowMutability::Mutable { + // For initial two-phase borrow + // deployment, conservatively omit + // overloaded binary ops. + allow_two_phase_borrow: false, + } + }; let autoref = Adjustment { - kind: Adjust::Borrow(AutoBorrow::Ref(region, mt.mutbl)), + kind: Adjust::Borrow(AutoBorrow::Ref(region, mutbl)), target: method.sig.inputs()[1] }; // HACK(eddyb) Bypass checks due to reborrows being in diff --git a/src/librustc_typeck/check/regionck.rs b/src/librustc_typeck/check/regionck.rs index 64063ec5beda9..b5bf59fef9afc 100644 --- a/src/librustc_typeck/check/regionck.rs +++ b/src/librustc_typeck/check/regionck.rs @@ -1063,7 +1063,7 @@ impl<'a, 'gcx, 'tcx> RegionCtxt<'a, 'gcx, 'tcx> { match *autoref { adjustment::AutoBorrow::Ref(r, m) => { self.link_region(expr.span, r, - ty::BorrowKind::from_mutbl(m), expr_cmt); + ty::BorrowKind::from_mutbl(m.into()), expr_cmt); } adjustment::AutoBorrow::RawPtr(m) => { From 81b93fa0b364e106f13c5810941857426a6c6756 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 6 Feb 2018 12:48:36 +0100 Subject: [PATCH 4/5] Test that autoref'ing beyond method receivers does not leak into two-phase borrows. --- .../borrowck/two-phase-nonrecv-autoref.rs | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 src/test/compile-fail/borrowck/two-phase-nonrecv-autoref.rs diff --git a/src/test/compile-fail/borrowck/two-phase-nonrecv-autoref.rs b/src/test/compile-fail/borrowck/two-phase-nonrecv-autoref.rs new file mode 100644 index 0000000000000..ad34d4d00dd92 --- /dev/null +++ b/src/test/compile-fail/borrowck/two-phase-nonrecv-autoref.rs @@ -0,0 +1,39 @@ +// 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. + +// revisions: lxl nll g2p +//[lxl]compile-flags: -Z borrowck=mir -Z two-phase-borrows +//[nll]compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z nll +//[g2p]compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z nll -Z two-phase-beyond-autoref + +#![feature(rustc_attrs)] + +// This is a test checking that when we limit two-phase borrows to +// method receivers, we do not let other kinds of auto-ref to leak +// through. +// +// The g2p revision illustrates the "undesirable" behavior you would +// otherwise observe without limiting the phasing to autoref on method +// receivers (namely, that the test would pass). + +fn bar(x: &mut u32) { + foo(x, *x); + //[lxl]~^ ERROR cannot use `*x` because it was mutably borrowed [E0503] + //[nll]~^^ ERROR cannot use `*x` because it was mutably borrowed [E0503] +} + +fn foo(x: &mut u32, y: u32) { + *x += y; +} + +#[rustc_error] +fn main() { //[g2p]~ ERROR compilation successful + bar(&mut 5); +} From b55cd8cc7c55a50941cc55cd399f6d5aebdf77f8 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 6 Feb 2018 16:47:38 +0100 Subject: [PATCH 5/5] Fleshed out the test a lot more. --- .../borrowck/two-phase-nonrecv-autoref.rs | 239 +++++++++++++++++- 1 file changed, 230 insertions(+), 9 deletions(-) diff --git a/src/test/compile-fail/borrowck/two-phase-nonrecv-autoref.rs b/src/test/compile-fail/borrowck/two-phase-nonrecv-autoref.rs index ad34d4d00dd92..795d45a776db5 100644 --- a/src/test/compile-fail/borrowck/two-phase-nonrecv-autoref.rs +++ b/src/test/compile-fail/borrowck/two-phase-nonrecv-autoref.rs @@ -13,27 +13,248 @@ //[nll]compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z nll //[g2p]compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z nll -Z two-phase-beyond-autoref -#![feature(rustc_attrs)] - // This is a test checking that when we limit two-phase borrows to // method receivers, we do not let other kinds of auto-ref to leak // through. // // The g2p revision illustrates the "undesirable" behavior you would // otherwise observe without limiting the phasing to autoref on method -// receivers (namely, that the test would pass). +// receivers (namely, in many cases demonstrated below, the error +// would not arise). + +// (If we revise the compiler or this test so that the g2p revision +// passes, turn the `rustc_attrs` feature back on and tag the `fn +// main` with `#[rustc_error]` so that this remains a valid +// compile-fail test.) +// +// #![feature(rustc_attrs)] + +use std::ops::{Index, IndexMut}; +use std::ops::{AddAssign, SubAssign, MulAssign, DivAssign, RemAssign}; +use std::ops::{BitAndAssign, BitOrAssign, BitXorAssign, ShlAssign, ShrAssign}; -fn bar(x: &mut u32) { +// This is case outlined by Niko that we want to ensure we reject +// (at least initially). + +fn foo(x: &mut u32, y: u32) { + *x += y; +} + +fn deref_coercion(x: &mut u32) { foo(x, *x); //[lxl]~^ ERROR cannot use `*x` because it was mutably borrowed [E0503] //[nll]~^^ ERROR cannot use `*x` because it was mutably borrowed [E0503] } -fn foo(x: &mut u32, y: u32) { - *x += y; +// While adding a flag to adjustments (indicating whether they +// should support two-phase borrows, here are the cases I +// encountered: +// +// - [x] Resolving overloaded_call_traits (call, call_mut, call_once) +// - [x] deref_coercion (shown above) +// - [x] coerce_unsized e.g. `&[T; n]`, `&mut [T; n] -> &[T]`, +// `&mut [T; n] -> &mut [T]`, `&Concrete -> &Trait` +// - [x] Method Call Receivers (the case we want to support!) +// - [x] ExprIndex and ExprUnary Deref; only need to handle coerce_index_op +// - [x] overloaded_binops + +fn overloaded_call_traits() { + // Regarding overloaded call traits, note that there is no + // scenario where adding two-phase borrows should "fix" these + // cases, because either we will resolve both invocations to + // `call_mut` (in which case the inner call requires a mutable + // borrow which will conflict with the outer reservation), or we + // will resolve both to `call` (which will just work, regardless + // of two-phase borrow support), or we will resolve both to + // `call_once` (in which case the inner call requires moving the + // receiver, invalidating the outer call). + + fn twice_ten_sm i32>(f: &mut F) { + f(f(10)); + //[lxl]~^ ERROR cannot borrow `*f` as mutable more than once at a time + //[lxl]~| ERROR cannot borrow `*f` as mutable more than once at a time + //[nll]~^^^ ERROR cannot borrow `*f` as mutable more than once at a time + //[nll]~| ERROR cannot borrow `*f` as mutable more than once at a time + //[g2p]~^^^^^ ERROR cannot borrow `*f` as mutable more than once at a time + } + fn twice_ten_si i32>(f: &mut F) { + f(f(10)); + } + fn twice_ten_so i32>(f: Box) { + f(f(10)); + //[lxl]~^ ERROR use of moved value: `*f` + //[nll]~^^ ERROR use of moved value: `*f` + //[g2p]~^^^ ERROR use of moved value: `*f` + } + + fn twice_ten_om(f: &mut FnMut(i32) -> i32) { + f(f(10)); + //[lxl]~^ ERROR cannot borrow `*f` as mutable more than once at a time + //[lxl]~| ERROR cannot borrow `*f` as mutable more than once at a time + //[nll]~^^^ ERROR cannot borrow `*f` as mutable more than once at a time + //[nll]~| ERROR cannot borrow `*f` as mutable more than once at a time + //[g2p]~^^^^^ ERROR cannot borrow `*f` as mutable more than once at a time + } + fn twice_ten_oi(f: &mut Fn(i32) -> i32) { + f(f(10)); + } + fn twice_ten_oo(f: Box i32>) { + f(f(10)); + //[lxl]~^ ERROR cannot move a value of type + //[lxl]~^^ ERROR cannot move a value of type + //[lxl]~^^^ ERROR use of moved value: `*f` + //[nll]~^^^^ ERROR cannot move a value of type + //[nll]~^^^^^ ERROR cannot move a value of type + //[nll]~^^^^^^ ERROR cannot move a value of type + //[nll]~^^^^^^^ ERROR cannot move a value of type + //[nll]~^^^^^^^^ ERROR use of moved value: `*f` + //[g2p]~^^^^^^^^^ ERROR cannot move a value of type + //[g2p]~^^^^^^^^^^ ERROR cannot move a value of type + //[g2p]~^^^^^^^^^^^ ERROR cannot move a value of type + //[g2p]~^^^^^^^^^^^^ ERROR cannot move a value of type + //[g2p]~^^^^^^^^^^^^^ ERROR use of moved value: `*f` + } + + twice_ten_sm(&mut |x| x + 1); + twice_ten_si(&mut |x| x + 1); + twice_ten_so(Box::new(|x| x + 1)); + twice_ten_om(&mut |x| x + 1); + twice_ten_oi(&mut |x| x + 1); + twice_ten_oo(Box::new(|x| x + 1)); +} + +trait TwoMethods { + fn m(&mut self, x: i32) -> i32 { x + 1 } + fn i(&self, x: i32) -> i32 { x + 1 } +} + +struct T; + +impl TwoMethods for T { } + +struct S; + +impl S { + fn m(&mut self, x: i32) -> i32 { x + 1 } + fn i(&self, x: i32) -> i32 { x + 1 } +} + +impl TwoMethods for [i32; 3] { } + +fn double_access(m: &mut [X], s: &[X]) { + m[0] = s[1]; } -#[rustc_error] -fn main() { //[g2p]~ ERROR compilation successful - bar(&mut 5); +fn coerce_unsized() { + let mut a = [1, 2, 3]; + + // This is not okay. + double_access(&mut a, &a); + //[lxl]~^ ERROR cannot borrow `a` as immutable because it is also borrowed as mutable [E0502] + //[nll]~^^ ERROR cannot borrow `a` as immutable because it is also borrowed as mutable [E0502] + //[g2p]~^^^ ERROR cannot borrow `a` as immutable because it is also borrowed as mutable [E0502] + + // But this is okay. + a.m(a.i(10)); +} + +struct I(i32); + +impl Index for I { + type Output = i32; + fn index(&self, _: i32) -> &i32 { + &self.0 + } +} + +impl IndexMut for I { + fn index_mut(&mut self, _: i32) -> &mut i32 { + &mut self.0 + } +} + +fn coerce_index_op() { + let mut i = I(10); + i[i[3]] = 4; + //[lxl]~^ ERROR cannot borrow `i` as immutable because it is also borrowed as mutable [E0502] + //[nll]~^^ ERROR cannot borrow `i` as immutable because it is also borrowed as mutable [E0502] + + i[3] = i[4]; + + i[i[3]] = i[4]; + //[lxl]~^ ERROR cannot borrow `i` as immutable because it is also borrowed as mutable [E0502] + //[nll]~^^ ERROR cannot borrow `i` as immutable because it is also borrowed as mutable [E0502] +} + +struct A(i32); + +macro_rules! trivial_binop { + ($Trait:ident, $m:ident) => { + impl $Trait for A { fn $m(&mut self, rhs: i32) { self.0 = rhs; } } + } +} + +trivial_binop!(AddAssign, add_assign); +trivial_binop!(SubAssign, sub_assign); +trivial_binop!(MulAssign, mul_assign); +trivial_binop!(DivAssign, div_assign); +trivial_binop!(RemAssign, rem_assign); +trivial_binop!(BitAndAssign, bitand_assign); +trivial_binop!(BitOrAssign, bitor_assign); +trivial_binop!(BitXorAssign, bitxor_assign); +trivial_binop!(ShlAssign, shl_assign); +trivial_binop!(ShrAssign, shr_assign); + +fn overloaded_binops() { + let mut a = A(10); + a += a.0; + //[lxl]~^ ERROR cannot use `a.0` because it was mutably borrowed + //[nll]~^^ ERROR cannot use `a.0` because it was mutably borrowed + a -= a.0; + //[lxl]~^ ERROR cannot use `a.0` because it was mutably borrowed + //[nll]~^^ ERROR cannot use `a.0` because it was mutably borrowed + a *= a.0; + //[lxl]~^ ERROR cannot use `a.0` because it was mutably borrowed + //[nll]~^^ ERROR cannot use `a.0` because it was mutably borrowed + a /= a.0; + //[lxl]~^ ERROR cannot use `a.0` because it was mutably borrowed + //[nll]~^^ ERROR cannot use `a.0` because it was mutably borrowed + a &= a.0; + //[lxl]~^ ERROR cannot use `a.0` because it was mutably borrowed + //[nll]~^^ ERROR cannot use `a.0` because it was mutably borrowed + a |= a.0; + //[lxl]~^ ERROR cannot use `a.0` because it was mutably borrowed + //[nll]~^^ ERROR cannot use `a.0` because it was mutably borrowed + a ^= a.0; + //[lxl]~^ ERROR cannot use `a.0` because it was mutably borrowed + //[nll]~^^ ERROR cannot use `a.0` because it was mutably borrowed + a <<= a.0; + //[lxl]~^ ERROR cannot use `a.0` because it was mutably borrowed + //[nll]~^^ ERROR cannot use `a.0` because it was mutably borrowed + a >>= a.0; + //[lxl]~^ ERROR cannot use `a.0` because it was mutably borrowed + //[nll]~^^ ERROR cannot use `a.0` because it was mutably borrowed +} + +fn main() { + + // As a reminder, this is the basic case we want to ensure we handle. + let mut v = vec![1, 2, 3]; + v.push(v.len()); + + // (as a rule, pnkfelix does not like to write tests with dead code.) + + deref_coercion(&mut 5); + overloaded_call_traits(); + + + let mut s = S; + s.m(s.i(10)); + + let mut t = T; + t.m(t.i(10)); + + coerce_unsized(); + coerce_index_op(); + overloaded_binops(); }