From 96ae0ee382a32d8218da454dc4fd2b2a6fa37c4a Mon Sep 17 00:00:00 2001 From: bobtwinkles Date: Tue, 27 Mar 2018 23:48:50 -0400 Subject: [PATCH] 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 7579d95a8fe..edd6d56759d 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 70fe3afa6d2..8db8e52b10d 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 255794aeab4..8b4b4bab7c4 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 ab89e17d81f..36445e71001 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 fe27dd50af4..0e1d3fdbb97 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 => {