From d37a7ab32b4f4f1a0b944abaa3ca104978aecb12 Mon Sep 17 00:00:00 2001 From: bobtwinkles Date: Sat, 24 Mar 2018 22:00:38 -0400 Subject: [PATCH 1/4] Extend two-phase borrows to apply to method receiver autorefs This is required to compile things like src/test/ui/borrowck/two-phase-method-receivers.rs --- src/librustc_typeck/check/cast.rs | 5 +- src/librustc_typeck/check/coercion.rs | 37 ++++++++++---- src/librustc_typeck/check/demand.rs | 10 ++-- src/librustc_typeck/check/mod.rs | 7 +-- .../borrowck/two-phase-nonrecv-autoref.rs | 49 ++++++++++++------- .../ui/borrowck/two-phase-method-receivers.rs | 29 +++++++++++ 6 files changed, 100 insertions(+), 37 deletions(-) create mode 100644 src/test/ui/borrowck/two-phase-method-receivers.rs diff --git a/src/librustc_typeck/check/cast.rs b/src/librustc_typeck/check/cast.rs index e4bad8349ea2b..70fe3afa6d25b 100644 --- a/src/librustc_typeck/check/cast.rs +++ b/src/librustc_typeck/check/cast.rs @@ -434,7 +434,8 @@ impl<'a, 'gcx, 'tcx> CastCheck<'tcx> { let f = self.expr_ty.fn_sig(fcx.tcx); let res = fcx.try_coerce(self.expr, self.expr_ty, - fcx.tcx.mk_fn_ptr(f)); + fcx.tcx.mk_fn_ptr(f), + false); if !res.is_ok() { return Err(CastError::NonScalar); } @@ -616,7 +617,7 @@ impl<'a, 'gcx, 'tcx> CastCheck<'tcx> { } fn try_coercion_cast(&self, fcx: &FnCtxt<'a, 'gcx, 'tcx>) -> bool { - fcx.try_coerce(self.expr, self.expr_ty, self.cast_ty).is_ok() + fcx.try_coerce(self.expr, self.expr_ty, self.cast_ty, false).is_ok() } } diff --git a/src/librustc_typeck/check/coercion.rs b/src/librustc_typeck/check/coercion.rs index 269ee49f38e70..255794aeab432 100644 --- a/src/librustc_typeck/check/coercion.rs +++ b/src/librustc_typeck/check/coercion.rs @@ -84,6 +84,12 @@ struct Coerce<'a, 'gcx: 'a + 'tcx, 'tcx: 'a> { fcx: &'a FnCtxt<'a, 'gcx, 'tcx>, cause: ObligationCause<'tcx>, use_lub: bool, + /// Determines whether or not allow_two_phase_borrow is set on any + /// autoref adjustments we create while coercing. We don't want to + /// allow deref coercions to create two-phase borrows, at least initially, + /// but we do need two-phase borrows for function argument reborrows. + /// See #47489 and #48598 + allow_two_phase: bool, } impl<'a, 'gcx, 'tcx> Deref for Coerce<'a, 'gcx, 'tcx> { @@ -123,10 +129,13 @@ fn success<'tcx>(adj: Vec>, } impl<'f, 'gcx, 'tcx> Coerce<'f, 'gcx, 'tcx> { - fn new(fcx: &'f FnCtxt<'f, 'gcx, 'tcx>, cause: ObligationCause<'tcx>) -> Self { + fn new(fcx: &'f FnCtxt<'f, 'gcx, 'tcx>, + cause: ObligationCause<'tcx>, + allow_two_phase: bool) -> Self { Coerce { fcx, cause, + allow_two_phase, use_lub: false, } } @@ -424,10 +433,7 @@ impl<'f, 'gcx, 'tcx> Coerce<'f, 'gcx, 'tcx> { 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, + allow_two_phase_borrow: self.allow_two_phase, } }; adjustments.push(Adjustment { @@ -473,6 +479,9 @@ impl<'f, 'gcx, 'tcx> Coerce<'f, 'gcx, 'tcx> { let mutbl = match mt_b.mutbl { hir::MutImmutable => AutoBorrowMutability::Immutable, hir::MutMutable => AutoBorrowMutability::Mutable { + // We don't allow two-phase borrows here, at least for initial + // implementation. If it happens that this coercion is a function argument, + // the reborrow in coerce_borrowed_ptr will pick it up. allow_two_phase_borrow: false, } }; @@ -751,13 +760,14 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { pub fn try_coerce(&self, expr: &hir::Expr, expr_ty: Ty<'tcx>, - target: Ty<'tcx>) + target: Ty<'tcx>, + allow_two_phase: bool) -> RelateResult<'tcx, Ty<'tcx>> { let source = self.resolve_type_vars_with_obligations(expr_ty); debug!("coercion::try({:?}: {:?} -> {:?})", expr, source, target); let cause = self.cause(expr.span, ObligationCauseCode::ExprAssignable); - let coerce = Coerce::new(self, cause); + let coerce = Coerce::new(self, cause, allow_two_phase); let ok = self.commit_if_ok(|_| coerce.coerce(source, target))?; let (adjustments, _) = self.register_infer_ok_obligations(ok); @@ -771,7 +781,8 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { debug!("coercion::can({:?} -> {:?})", source, target); let cause = self.cause(syntax_pos::DUMMY_SP, ObligationCauseCode::ExprAssignable); - let coerce = Coerce::new(self, cause); + // We don't ever need two-phase here since we throw out the result of the coercion + let coerce = Coerce::new(self, cause, false); self.probe(|_| coerce.coerce(source, target)).is_ok() } @@ -840,7 +851,12 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { return Ok(fn_ptr); } - let mut coerce = Coerce::new(self, cause.clone()); + // Configure a Coerce instance to compute the LUB. + // We don't allow two-phase borrows on any autorefs this creates since we + // probably aren't processing function arguments here and even if we were, + // they're going to get autorefed again anyway and we can apply 2-phase borrows + // at that time. + let mut coerce = Coerce::new(self, cause.clone(), false); coerce.use_lub = true; // First try to coerce the new expression to the type of the previous ones, @@ -1106,7 +1122,8 @@ impl<'gcx, 'tcx, 'exprs, E> CoerceMany<'gcx, 'tcx, 'exprs, E> if self.pushed == 0 { // Special-case the first expression we are coercing. // To be honest, I'm not entirely sure why we do this. - fcx.try_coerce(expression, expression_ty, self.expected_ty) + // We don't allow two-phase borrows, see comment in try_find_coercion_lub for why + fcx.try_coerce(expression, expression_ty, self.expected_ty, false) } else { match self.expressions { Expressions::Dynamic(ref exprs) => diff --git a/src/librustc_typeck/check/demand.rs b/src/librustc_typeck/check/demand.rs index 634a7ee569917..ab89e17d81fe6 100644 --- a/src/librustc_typeck/check/demand.rs +++ b/src/librustc_typeck/check/demand.rs @@ -79,9 +79,10 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { pub fn demand_coerce(&self, expr: &hir::Expr, checked_ty: Ty<'tcx>, - expected: Ty<'tcx>) + expected: Ty<'tcx>, + allow_two_phase: bool) -> Ty<'tcx> { - let (ty, err) = self.demand_coerce_diag(expr, checked_ty, expected); + let (ty, err) = self.demand_coerce_diag(expr, checked_ty, expected, allow_two_phase); if let Some(mut err) = err { err.emit(); } @@ -96,11 +97,12 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { pub fn demand_coerce_diag(&self, expr: &hir::Expr, checked_ty: Ty<'tcx>, - expected: Ty<'tcx>) + expected: Ty<'tcx>, + allow_two_phase: bool) -> (Ty<'tcx>, Option>) { let expected = self.resolve_type_vars_with_obligations(expected); - let e = match self.try_coerce(expr, checked_ty, expected) { + let e = match self.try_coerce(expr, checked_ty, expected, allow_two_phase) { Ok(ty) => return (ty, None), Err(e) => e }; diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index 4a685cfddb7a4..fe27dd50af472 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -2649,7 +2649,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { // to, which is `expected_ty` if `rvalue_hint` returns an // `ExpectHasType(expected_ty)`, or the `formal_ty` otherwise. let coerce_ty = expected.and_then(|e| e.only_has_type(self)); - self.demand_coerce(&arg, checked_ty, coerce_ty.unwrap_or(formal_ty)); + self.demand_coerce(&arg, checked_ty, coerce_ty.unwrap_or(formal_ty), true); // 3. Relate the expected type and the formal one, // if the expected type was used for the coercion. @@ -2812,7 +2812,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { expr, ExpectHasType(expected), needs); - self.demand_coerce(expr, ty, expected) + self.demand_coerce(expr, ty, expected, false) } fn check_expr_with_hint(&self, expr: &'gcx hir::Expr, @@ -4112,7 +4112,8 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { let base_t = self.structurally_resolved_type(expr.span, base_t); match self.lookup_indexing(expr, base, base_t, idx_t, needs) { Some((index_ty, element_ty)) => { - self.demand_coerce(idx, idx_t, index_ty); + // two-phase not needed because index_ty is never mutable + self.demand_coerce(idx, idx_t, index_ty, false); element_ty } None => { 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 bf8e02adb1ae6..ef39fabda10e6 100644 --- a/src/test/compile-fail/borrowck/two-phase-nonrecv-autoref.rs +++ b/src/test/compile-fail/borrowck/two-phase-nonrecv-autoref.rs @@ -8,7 +8,8 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// revisions: lxl nll +// revisions: ast lxl nll +//[ast]compile-flags: //[lxl]compile-flags: -Z borrowck=mir -Z two-phase-borrows //[nll]compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z nll @@ -33,17 +34,14 @@ use std::ops::{Index, IndexMut}; -// 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] + //[ast]~^ ERROR cannot use `*x` because it was mutably borrowed [E0503] + // Above error is a known limitation of AST borrowck } // While adding a flag to adjustments (indicating whether they @@ -74,22 +72,25 @@ fn overloaded_call_traits() { //[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 //[g2p]~^^^ ERROR cannot borrow `*f` as mutable more than once at a time + //[ast]~^^^^ 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` + //[lxl]~^ ERROR use of moved value: `*f` + //[nll]~^^ ERROR use of moved value: `*f` + //[g2p]~^^^ ERROR use of moved value: `*f` + //[ast]~^^^^ 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 - //[g2p]~^^^ 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 + //[ast]~^^^^ ERROR cannot borrow `*f` as mutable more than once at a time } fn twice_ten_oi(f: &mut Fn(i32) -> i32) { f(f(10)); @@ -105,6 +106,7 @@ fn overloaded_call_traits() { //[g2p]~^^^^^^^ ERROR cannot move a value of type //[g2p]~^^^^^^^^ ERROR cannot move a value of type //[g2p]~^^^^^^^^^ ERROR use of moved value: `*f` + //[ast]~^^^^^^^^^^ ERROR use of moved value: `*f` } twice_ten_sm(&mut |x| x + 1); @@ -142,12 +144,15 @@ fn coerce_unsized() { // 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] + //[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] + //[ast]~^^^^ ERROR cannot borrow `a` as immutable because it is also borrowed as mutable [E0502] // But this is okay. a.m(a.i(10)); + //[ast]~^ ERROR cannot borrow `a` as immutable because it is also borrowed as mutable [E0502] + // Above error is an expected limitation of AST borrowck } struct I(i32); @@ -168,14 +173,16 @@ impl IndexMut for I { 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] + //[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] + //[ast]~^^^ 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] + //[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] + //[ast]~^^^ ERROR cannot borrow `i` as immutable because it is also borrowed as mutable [E0502] } fn main() { @@ -183,6 +190,8 @@ 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()); + //[ast]~^ ERROR cannot borrow `v` as immutable because it is also borrowed as mutable [E0502] + // Error above is an expected limitation of AST borrowck // (as a rule, pnkfelix does not like to write tests with dead code.) @@ -192,9 +201,13 @@ fn main() { let mut s = S; s.m(s.i(10)); + //[ast]~^ ERROR cannot borrow `s` as immutable because it is also borrowed as mutable [E0502] + // Error above is an expected limitation of AST borrowck let mut t = T; t.m(t.i(10)); + //[ast]~^ ERROR cannot borrow `t` as immutable because it is also borrowed as mutable [E0502] + // Error above is an expected limitation of AST borrowck coerce_unsized(); coerce_index_op(); diff --git a/src/test/ui/borrowck/two-phase-method-receivers.rs b/src/test/ui/borrowck/two-phase-method-receivers.rs new file mode 100644 index 0000000000000..e690263a916f3 --- /dev/null +++ b/src/test/ui/borrowck/two-phase-method-receivers.rs @@ -0,0 +1,29 @@ +// 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 +//[lxl]compile-flags: -Z borrowck=mir -Z two-phase-borrows +//[nll]compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z nll + +// run-pass + +struct Foo<'a> { + x: &'a i32 +} + +impl<'a> Foo<'a> { + fn method(&mut self, _: &i32) { + } +} + +fn main() { + let a = &mut Foo { x: &22 }; + Foo::method(a, a.x); +} From 96ae0ee382a32d8218da454dc4fd2b2a6fa37c4a Mon Sep 17 00:00:00 2001 From: bobtwinkles Date: Tue, 27 Mar 2018 23:48:50 -0400 Subject: [PATCH 2/4] Use a new type to track if two-phase borrows are allowed Because more type safe is more better, and random boolean parameters everywhere were not the greatest thing. --- src/librustc/ty/adjustment.rs | 17 +++++++++++++++++ src/librustc_typeck/check/cast.rs | 5 +++-- src/librustc_typeck/check/coercion.rs | 20 ++++++++++++-------- src/librustc_typeck/check/demand.rs | 5 +++-- src/librustc_typeck/check/mod.rs | 10 ++++++---- 5 files changed, 41 insertions(+), 16 deletions(-) diff --git a/src/librustc/ty/adjustment.rs b/src/librustc/ty/adjustment.rs index 7579d95a8fe68..edd6d56759d94 100644 --- a/src/librustc/ty/adjustment.rs +++ b/src/librustc/ty/adjustment.rs @@ -119,6 +119,23 @@ impl<'a, 'gcx, 'tcx> OverloadedDeref<'tcx> { } } +/// At least for initial deployment, we want to limit two-phase borrows to +/// only a few specific cases. Right now, those mostly "things that desugar" +/// into method calls +/// - using x.some_method() syntax, where some_method takes &mut self +/// - using Foo::some_method(&mut x, ...) syntax +/// - binary assignment operators (+=, -=, *=, etc.) +/// Anything else should be rejected until generalized two phase borrow support +/// is implemented. Right now, dataflow can't handle the general case where there +/// is more than one use of a mutable borrow, and we don't want to accept too much +/// new code via two-phase borrows, so we try to limit where we create two-phase +/// capable mutable borrows. +/// See #49434 for tracking. +pub enum AllowTwoPhase { + Yes, + No +} + #[derive(Copy, Clone, PartialEq, Debug, RustcEncodable, RustcDecodable)] pub enum AutoBorrowMutability { Mutable { allow_two_phase_borrow: bool }, diff --git a/src/librustc_typeck/check/cast.rs b/src/librustc_typeck/check/cast.rs index 70fe3afa6d25b..8db8e52b10d4a 100644 --- a/src/librustc_typeck/check/cast.rs +++ b/src/librustc_typeck/check/cast.rs @@ -47,6 +47,7 @@ use rustc::hir; use rustc::session::Session; use rustc::traits; use rustc::ty::{self, Ty, TypeFoldable}; +use rustc::ty::adjustment::AllowTwoPhase; use rustc::ty::cast::{CastKind, CastTy}; use rustc::ty::subst::Substs; use rustc::middle::lang_items; @@ -435,7 +436,7 @@ impl<'a, 'gcx, 'tcx> CastCheck<'tcx> { let res = fcx.try_coerce(self.expr, self.expr_ty, fcx.tcx.mk_fn_ptr(f), - false); + AllowTwoPhase::No); if !res.is_ok() { return Err(CastError::NonScalar); } @@ -617,7 +618,7 @@ impl<'a, 'gcx, 'tcx> CastCheck<'tcx> { } fn try_coercion_cast(&self, fcx: &FnCtxt<'a, 'gcx, 'tcx>) -> bool { - fcx.try_coerce(self.expr, self.expr_ty, self.cast_ty, false).is_ok() + fcx.try_coerce(self.expr, self.expr_ty, self.cast_ty, AllowTwoPhase::No).is_ok() } } diff --git a/src/librustc_typeck/check/coercion.rs b/src/librustc_typeck/check/coercion.rs index 255794aeab432..8b4b4bab7c410 100644 --- a/src/librustc_typeck/check/coercion.rs +++ b/src/librustc_typeck/check/coercion.rs @@ -67,7 +67,7 @@ use rustc::hir::def_id::DefId; use rustc::infer::{Coercion, InferResult, InferOk}; use rustc::infer::type_variable::TypeVariableOrigin; use rustc::traits::{self, ObligationCause, ObligationCauseCode}; -use rustc::ty::adjustment::{Adjustment, Adjust, AutoBorrow, AutoBorrowMutability}; +use rustc::ty::adjustment::{Adjustment, Adjust, AllowTwoPhase, AutoBorrow, AutoBorrowMutability}; use rustc::ty::{self, TypeAndMut, Ty, ClosureSubsts}; use rustc::ty::fold::TypeFoldable; use rustc::ty::error::TypeError; @@ -89,7 +89,8 @@ struct Coerce<'a, 'gcx: 'a + 'tcx, 'tcx: 'a> { /// allow deref coercions to create two-phase borrows, at least initially, /// but we do need two-phase borrows for function argument reborrows. /// See #47489 and #48598 - allow_two_phase: bool, + /// See docs on the "AllowTwoPhase" type for a more detailed discussion + allow_two_phase: AllowTwoPhase, } impl<'a, 'gcx, 'tcx> Deref for Coerce<'a, 'gcx, 'tcx> { @@ -131,7 +132,7 @@ fn success<'tcx>(adj: Vec>, impl<'f, 'gcx, 'tcx> Coerce<'f, 'gcx, 'tcx> { fn new(fcx: &'f FnCtxt<'f, 'gcx, 'tcx>, cause: ObligationCause<'tcx>, - allow_two_phase: bool) -> Self { + allow_two_phase: AllowTwoPhase) -> Self { Coerce { fcx, cause, @@ -433,7 +434,10 @@ impl<'f, 'gcx, 'tcx> Coerce<'f, 'gcx, 'tcx> { let mutbl = match mt_b.mutbl { hir::MutImmutable => AutoBorrowMutability::Immutable, hir::MutMutable => AutoBorrowMutability::Mutable { - allow_two_phase_borrow: self.allow_two_phase, + allow_two_phase_borrow: match self.allow_two_phase { + AllowTwoPhase::Yes => true, + AllowTwoPhase::No => false + }, } }; adjustments.push(Adjustment { @@ -761,7 +765,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { expr: &hir::Expr, expr_ty: Ty<'tcx>, target: Ty<'tcx>, - allow_two_phase: bool) + allow_two_phase: AllowTwoPhase) -> RelateResult<'tcx, Ty<'tcx>> { let source = self.resolve_type_vars_with_obligations(expr_ty); debug!("coercion::try({:?}: {:?} -> {:?})", expr, source, target); @@ -782,7 +786,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { let cause = self.cause(syntax_pos::DUMMY_SP, ObligationCauseCode::ExprAssignable); // We don't ever need two-phase here since we throw out the result of the coercion - let coerce = Coerce::new(self, cause, false); + let coerce = Coerce::new(self, cause, AllowTwoPhase::No); self.probe(|_| coerce.coerce(source, target)).is_ok() } @@ -856,7 +860,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { // probably aren't processing function arguments here and even if we were, // they're going to get autorefed again anyway and we can apply 2-phase borrows // at that time. - let mut coerce = Coerce::new(self, cause.clone(), false); + let mut coerce = Coerce::new(self, cause.clone(), AllowTwoPhase::No); coerce.use_lub = true; // First try to coerce the new expression to the type of the previous ones, @@ -1123,7 +1127,7 @@ impl<'gcx, 'tcx, 'exprs, E> CoerceMany<'gcx, 'tcx, 'exprs, E> // Special-case the first expression we are coercing. // To be honest, I'm not entirely sure why we do this. // We don't allow two-phase borrows, see comment in try_find_coercion_lub for why - fcx.try_coerce(expression, expression_ty, self.expected_ty, false) + fcx.try_coerce(expression, expression_ty, self.expected_ty, AllowTwoPhase::No) } else { match self.expressions { Expressions::Dynamic(ref exprs) => diff --git a/src/librustc_typeck/check/demand.rs b/src/librustc_typeck/check/demand.rs index ab89e17d81fe6..36445e7100150 100644 --- a/src/librustc_typeck/check/demand.rs +++ b/src/librustc_typeck/check/demand.rs @@ -21,6 +21,7 @@ use rustc::hir; use rustc::hir::print; use rustc::hir::def::Def; use rustc::ty::{self, Ty, AssociatedItem}; +use rustc::ty::adjustment::AllowTwoPhase; use errors::{DiagnosticBuilder, CodeMapper}; use super::method::probe; @@ -80,7 +81,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { expr: &hir::Expr, checked_ty: Ty<'tcx>, expected: Ty<'tcx>, - allow_two_phase: bool) + allow_two_phase: AllowTwoPhase) -> Ty<'tcx> { let (ty, err) = self.demand_coerce_diag(expr, checked_ty, expected, allow_two_phase); if let Some(mut err) = err { @@ -98,7 +99,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { expr: &hir::Expr, checked_ty: Ty<'tcx>, expected: Ty<'tcx>, - allow_two_phase: bool) + allow_two_phase: AllowTwoPhase) -> (Ty<'tcx>, Option>) { let expected = self.resolve_type_vars_with_obligations(expected); diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index fe27dd50af472..0e1d3fdbb9767 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -97,7 +97,7 @@ use rustc::mir::interpret::{GlobalId}; 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, AutoBorrowMutability}; +use rustc::ty::adjustment::{Adjust, Adjustment, AllowTwoPhase, AutoBorrow, AutoBorrowMutability}; use rustc::ty::fold::TypeFoldable; use rustc::ty::maps::Providers; use rustc::ty::util::{Representability, IntTypeExt, Discr}; @@ -2649,7 +2649,8 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { // to, which is `expected_ty` if `rvalue_hint` returns an // `ExpectHasType(expected_ty)`, or the `formal_ty` otherwise. let coerce_ty = expected.and_then(|e| e.only_has_type(self)); - self.demand_coerce(&arg, checked_ty, coerce_ty.unwrap_or(formal_ty), true); + // We're processing function arguments so we definitely want to use two-phase borrows. + self.demand_coerce(&arg, checked_ty, coerce_ty.unwrap_or(formal_ty), AllowTwoPhase::Yes); // 3. Relate the expected type and the formal one, // if the expected type was used for the coercion. @@ -2812,7 +2813,8 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { expr, ExpectHasType(expected), needs); - self.demand_coerce(expr, ty, expected, false) + // checks don't need two phase + self.demand_coerce(expr, ty, expected, AllowTwoPhase::No) } fn check_expr_with_hint(&self, expr: &'gcx hir::Expr, @@ -4113,7 +4115,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { match self.lookup_indexing(expr, base, base_t, idx_t, needs) { Some((index_ty, element_ty)) => { // two-phase not needed because index_ty is never mutable - self.demand_coerce(idx, idx_t, index_ty, false); + self.demand_coerce(idx, idx_t, index_ty, AllowTwoPhase::No); element_ty } None => { From d64bd2afc3ee46b9167bb06b3bdacd3bd79add7e Mon Sep 17 00:00:00 2001 From: bobtwinkles Date: Wed, 28 Mar 2018 04:08:03 -0400 Subject: [PATCH 3/4] Push AllowTwoPhase down to the HAIR level For consistency, use AllowTwoPhase everywhere between the frontend and MIR. --- src/librustc/ich/impls_ty.rs | 4 ++++ src/librustc/ty/adjustment.rs | 3 ++- src/librustc_mir/hair/cx/expr.rs | 6 +++++- src/librustc_typeck/check/callee.rs | 4 ++-- src/librustc_typeck/check/coercion.rs | 7 ++----- src/librustc_typeck/check/method/confirm.rs | 6 +++--- src/librustc_typeck/check/mod.rs | 13 ++++++------- src/librustc_typeck/check/op.rs | 6 +++--- 8 files changed, 27 insertions(+), 22 deletions(-) diff --git a/src/librustc/ich/impls_ty.rs b/src/librustc/ich/impls_ty.rs index 9a442e0529938..89b8bfcd4ca23 100644 --- a/src/librustc/ich/impls_ty.rs +++ b/src/librustc/ich/impls_ty.rs @@ -188,6 +188,10 @@ for ty::adjustment::Adjust<'gcx> { 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_stable_hash_for!(enum ty::adjustment::AllowTwoPhase { + Yes, + No +}); impl<'gcx> HashStable> for ty::adjustment::AutoBorrowMutability { fn hash_stable(&self, diff --git a/src/librustc/ty/adjustment.rs b/src/librustc/ty/adjustment.rs index edd6d56759d94..a0c31e8b50923 100644 --- a/src/librustc/ty/adjustment.rs +++ b/src/librustc/ty/adjustment.rs @@ -131,6 +131,7 @@ impl<'a, 'gcx, 'tcx> OverloadedDeref<'tcx> { /// new code via two-phase borrows, so we try to limit where we create two-phase /// capable mutable borrows. /// See #49434 for tracking. +#[derive(Copy, Clone, PartialEq, Debug, RustcEncodable, RustcDecodable)] pub enum AllowTwoPhase { Yes, No @@ -138,7 +139,7 @@ pub enum AllowTwoPhase { #[derive(Copy, Clone, PartialEq, Debug, RustcEncodable, RustcDecodable)] pub enum AutoBorrowMutability { - Mutable { allow_two_phase_borrow: bool }, + Mutable { allow_two_phase_borrow: AllowTwoPhase }, Immutable, } diff --git a/src/librustc_mir/hair/cx/expr.rs b/src/librustc_mir/hair/cx/expr.rs index 62d1b43d62570..5b3739084801f 100644 --- a/src/librustc_mir/hair/cx/expr.rs +++ b/src/librustc_mir/hair/cx/expr.rs @@ -662,9 +662,13 @@ trait ToBorrowKind { fn to_borrow_kind(&self) -> BorrowKind; } impl ToBorrowKind for AutoBorrowMutability { fn to_borrow_kind(&self) -> BorrowKind { + use rustc::ty::adjustment::AllowTwoPhase; match *self { AutoBorrowMutability::Mutable { allow_two_phase_borrow } => - BorrowKind::Mut { allow_two_phase_borrow }, + BorrowKind::Mut { allow_two_phase_borrow: match allow_two_phase_borrow { + AllowTwoPhase::Yes => true, + AllowTwoPhase::No => false + }}, AutoBorrowMutability::Immutable => BorrowKind::Shared, } diff --git a/src/librustc_typeck/check/callee.rs b/src/librustc_typeck/check/callee.rs index 3d61ffe39336a..b1fb0938698cb 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, AutoBorrowMutability}; +use rustc::ty::adjustment::{Adjustment, Adjust, AllowTwoPhase, AutoBorrow, AutoBorrowMutability}; use syntax::abi; use syntax::symbol::Symbol; use syntax_pos::Span; @@ -182,7 +182,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { // For initial two-phase borrow // deployment, conservatively omit // overloaded function call ops. - allow_two_phase_borrow: false, + allow_two_phase_borrow: AllowTwoPhase::No, } }; autoref = Some(Adjustment { diff --git a/src/librustc_typeck/check/coercion.rs b/src/librustc_typeck/check/coercion.rs index 8b4b4bab7c410..a956dd9a4ee03 100644 --- a/src/librustc_typeck/check/coercion.rs +++ b/src/librustc_typeck/check/coercion.rs @@ -434,10 +434,7 @@ impl<'f, 'gcx, 'tcx> Coerce<'f, 'gcx, 'tcx> { let mutbl = match mt_b.mutbl { hir::MutImmutable => AutoBorrowMutability::Immutable, hir::MutMutable => AutoBorrowMutability::Mutable { - allow_two_phase_borrow: match self.allow_two_phase { - AllowTwoPhase::Yes => true, - AllowTwoPhase::No => false - }, + allow_two_phase_borrow: self.allow_two_phase, } }; adjustments.push(Adjustment { @@ -486,7 +483,7 @@ impl<'f, 'gcx, 'tcx> Coerce<'f, 'gcx, 'tcx> { // We don't allow two-phase borrows here, at least for initial // implementation. If it happens that this coercion is a function argument, // the reborrow in coerce_borrowed_ptr will pick it up. - allow_two_phase_borrow: false, + allow_two_phase_borrow: AllowTwoPhase::No, } }; Some((Adjustment { diff --git a/src/librustc_typeck/check/method/confirm.rs b/src/librustc_typeck/check/method/confirm.rs index b777ac30920cd..8a37c11f191e7 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, AutoBorrowMutability, OverloadedDeref}; +use rustc::ty::adjustment::{Adjustment, Adjust, AllowTwoPhase, AutoBorrow, AutoBorrowMutability, OverloadedDeref}; use rustc::ty::fold::TypeFoldable; use rustc::infer::{self, InferOk}; use syntax_pos::Span; @@ -170,7 +170,7 @@ impl<'a, 'gcx, 'tcx> ConfirmContext<'a, 'gcx, 'tcx> { hir::MutMutable => AutoBorrowMutability::Mutable { // Method call receivers are the primary use case // for two-phase borrows. - allow_two_phase_borrow: true, + allow_two_phase_borrow: AllowTwoPhase::Yes, } }; adjustments.push(Adjustment { @@ -544,7 +544,7 @@ impl<'a, 'gcx, 'tcx> ConfirmContext<'a, 'gcx, 'tcx> { // For initial two-phase borrow // deployment, conservatively omit // overloaded operators. - allow_two_phase_borrow: false, + allow_two_phase_borrow: AllowTwoPhase::No, } }; adjustment.kind = Adjust::Borrow(AutoBorrow::Ref(region, mutbl)); diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index 0e1d3fdbb9767..a377ff4d29d2d 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -2341,12 +2341,11 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { 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, + // Indexing can be desugared to a method call, + // so maybe we could use two-phase here. + // See the documentation of AllowTwoPhase for why that's + // not the case today. + allow_two_phase_borrow: AllowTwoPhase::No, } }; adjustments.push(Adjustment { @@ -3647,7 +3646,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { // (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, + allow_two_phase_borrow: AllowTwoPhase::No, } }; self.apply_adjustments(oprnd, vec![Adjustment { diff --git a/src/librustc_typeck/check/op.rs b/src/librustc_typeck/check/op.rs index eae692f4cdad9..a6fa3a6453fd0 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, TyAdt}; -use rustc::ty::adjustment::{Adjustment, Adjust, AutoBorrow, AutoBorrowMutability}; +use rustc::ty::adjustment::{Adjustment, Adjust, AllowTwoPhase, AutoBorrow, AutoBorrowMutability}; use rustc::infer::type_variable::TypeVariableOrigin; use errors; use syntax_pos::Span; @@ -206,7 +206,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { hir::MutMutable => AutoBorrowMutability::Mutable { // Allow two-phase borrows for binops in initial deployment // since they desugar to methods - allow_two_phase_borrow: true, + allow_two_phase_borrow: AllowTwoPhase::Yes, } }; let autoref = Adjustment { @@ -223,7 +223,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { hir::MutMutable => AutoBorrowMutability::Mutable { // Allow two-phase borrows for binops in initial deployment // since they desugar to methods - allow_two_phase_borrow: true, + allow_two_phase_borrow: AllowTwoPhase::Yes, } }; let autoref = Adjustment { From d8352af934ef751bb75d9ca310ed5b02ea0753cb Mon Sep 17 00:00:00 2001 From: bobtwinkles Date: Wed, 28 Mar 2018 14:43:05 -0400 Subject: [PATCH 4/4] Fix up tidy errors --- src/librustc_typeck/check/method/confirm.rs | 3 ++- src/librustc_typeck/check/mod.rs | 8 ++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/librustc_typeck/check/method/confirm.rs b/src/librustc_typeck/check/method/confirm.rs index 8a37c11f191e7..9c8c7d4df74f9 100644 --- a/src/librustc_typeck/check/method/confirm.rs +++ b/src/librustc_typeck/check/method/confirm.rs @@ -17,7 +17,8 @@ use rustc::ty::subst::Substs; use rustc::traits; use rustc::ty::{self, Ty}; use rustc::ty::subst::Subst; -use rustc::ty::adjustment::{Adjustment, Adjust, AllowTwoPhase, AutoBorrow, AutoBorrowMutability, OverloadedDeref}; +use rustc::ty::adjustment::{Adjustment, Adjust, OverloadedDeref}; +use rustc::ty::adjustment::{AllowTwoPhase, AutoBorrow, AutoBorrowMutability}; use rustc::ty::fold::TypeFoldable; use rustc::infer::{self, InferOk}; use syntax_pos::Span; diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index a377ff4d29d2d..9ae7a74954b29 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -2648,8 +2648,12 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { // to, which is `expected_ty` if `rvalue_hint` returns an // `ExpectHasType(expected_ty)`, or the `formal_ty` otherwise. let coerce_ty = expected.and_then(|e| e.only_has_type(self)); - // We're processing function arguments so we definitely want to use two-phase borrows. - self.demand_coerce(&arg, checked_ty, coerce_ty.unwrap_or(formal_ty), AllowTwoPhase::Yes); + // We're processing function arguments so we definitely want to use + // two-phase borrows. + self.demand_coerce(&arg, + checked_ty, + coerce_ty.unwrap_or(formal_ty), + AllowTwoPhase::Yes); // 3. Relate the expected type and the formal one, // if the expected type was used for the coercion.