From 2f45294a506904f2768a8f991b0cf33b7cb0bcd2 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Wed, 17 Jun 2015 21:54:22 +0300 Subject: [PATCH] Clean-up assignment checking in borrowck --- src/librustc_borrowck/borrowck/check_loans.rs | 217 +++--------------- .../borrowck/gather_loans/lifetime.rs | 2 +- .../borrowck/gather_loans/mod.rs | 155 ++++++++----- .../borrowck/gather_loans/restrictions.rs | 2 +- src/librustc_borrowck/borrowck/mod.rs | 35 +-- 5 files changed, 142 insertions(+), 269 deletions(-) diff --git a/src/librustc_borrowck/borrowck/check_loans.rs b/src/librustc_borrowck/borrowck/check_loans.rs index 9d4fb4c994d4..f06dc105d9cc 100644 --- a/src/librustc_borrowck/borrowck/check_loans.rs +++ b/src/librustc_borrowck/borrowck/check_loans.rs @@ -565,13 +565,6 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> { true } - fn is_local_variable_or_arg(&self, cmt: mc::cmt<'tcx>) -> bool { - match cmt.cat { - mc::cat_local(_) => true, - _ => false - } - } - fn consume_common(&self, id: ast::NodeId, span: Span, @@ -793,198 +786,40 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> { mode: euv::MutateMode) { debug!("check_assignment(assignee_cmt={:?})", assignee_cmt); - // Mutable values can be assigned, as long as they obey loans - // and aliasing restrictions: - if assignee_cmt.mutbl.is_mutable() { - if check_for_aliasable_mutable_writes(self, assignment_span, assignee_cmt.clone()) { - if mode != euv::Init { - check_for_assignment_to_borrowed_path( - self, assignment_id, assignment_span, assignee_cmt.clone()); - mark_variable_as_used_mut(self, assignee_cmt); - } - } - - return; - } - - // Initializations are OK if and only if they aren't partial - // reinitialization of a partially-uninitialized structure. + // Initializations never cause borrow errors as they only + // affect a fresh local. if mode == euv::Init { return } - // For immutable local variables, assignments are legal - // if they cannot already have been assigned - if self.is_local_variable_or_arg(assignee_cmt.clone()) { - assert!(assignee_cmt.mutbl.is_immutable()); // no "const" locals + // Check that we don't invalidate any outstanding loans + if let Some(loan_path) = opt_loan_path(&assignee_cmt) { + let scope = region::CodeExtent::from_node_id(assignment_id); + self.each_in_scope_loan_affecting_path(scope, &*loan_path, |loan| { + self.report_illegal_mutation(assignment_span, &*loan_path, loan); + false + }); + } + + // Local variables can always be assigned to, expect for reassignments + // of immutable variables (or assignments that invalidate loans, + // of course). + if let mc::cat_local(local_id) = assignee_cmt.cat { + if assignee_cmt.mutbl.is_mutable() { + self.tcx().used_mut_nodes.borrow_mut().insert(local_id); + } + let lp = opt_loan_path(&assignee_cmt).unwrap(); self.move_data.each_assignment_of(assignment_id, &lp, |assign| { - self.bccx.report_reassigned_immutable_variable( - assignment_span, - &*lp, - assign); - false - }); - return; - } - - // Otherwise, just a plain error. - match assignee_cmt.note { - mc::NoteClosureEnv(upvar_id) => { - // If this is an `Fn` closure, it simply can't mutate upvars. - // If it's an `FnMut` closure, the original variable was declared immutable. - // We need to determine which is the case here. - let kind = match assignee_cmt.upvar().unwrap().cat { - mc::cat_upvar(mc::Upvar { kind, .. }) => kind, - _ => unreachable!() - }; - if kind == ty::FnClosureKind { - self.bccx.span_err( - assignment_span, - &format!("cannot assign to {}", - self.bccx.cmt_to_string(&*assignee_cmt))); - self.bccx.span_help( - self.tcx().map.span(upvar_id.closure_expr_id), - "consider changing this closure to take self by mutable reference"); - } else { - self.bccx.span_err( - assignment_span, - &format!("cannot assign to {} {}", - assignee_cmt.mutbl.to_user_str(), - self.bccx.cmt_to_string(&*assignee_cmt))); - } - } - _ => match opt_loan_path(&assignee_cmt) { - Some(lp) => { - self.bccx.span_err( - assignment_span, - &format!("cannot assign to {} {} `{}`", - assignee_cmt.mutbl.to_user_str(), - self.bccx.cmt_to_string(&*assignee_cmt), - self.bccx.loan_path_to_string(&*lp))); - } - None => { - self.bccx.span_err( - assignment_span, - &format!("cannot assign to {} {}", - assignee_cmt.mutbl.to_user_str(), - self.bccx.cmt_to_string(&*assignee_cmt))); - } - } - } - return; - - fn mark_variable_as_used_mut<'a, 'tcx>(this: &CheckLoanCtxt<'a, 'tcx>, - mut cmt: mc::cmt<'tcx>) { - //! If the mutability of the `cmt` being written is inherited - //! from a local variable, liveness will - //! not have been able to detect that this variable's mutability - //! is important, so we must add the variable to the - //! `used_mut_nodes` table here. - - loop { - debug!("mark_variable_as_used_mut(cmt={:?})", cmt); - match cmt.cat.clone() { - mc::cat_upvar(mc::Upvar { id: ty::UpvarId { var_id: id, .. }, .. }) | - mc::cat_local(id) => { - this.tcx().used_mut_nodes.borrow_mut().insert(id); - return; - } - - mc::cat_rvalue(..) | - mc::cat_static_item | - mc::cat_deref(_, _, mc::UnsafePtr(..)) | - mc::cat_deref(_, _, mc::Implicit(..)) => { - assert_eq!(cmt.mutbl, mc::McDeclared); - return; - } - - mc::cat_deref(_, _, mc::BorrowedPtr(..)) => { - assert_eq!(cmt.mutbl, mc::McDeclared); - // We need to drill down to upvar if applicable - match cmt.upvar() { - Some(b) => cmt = b, - None => return - } - } - - mc::cat_deref(b, _, mc::Unique) => { - assert_eq!(cmt.mutbl, mc::McInherited); - cmt = b; - } - - mc::cat_downcast(b, _) | - mc::cat_interior(b, _) => { - assert_eq!(cmt.mutbl, mc::McInherited); - cmt = b; - } - } - } - } - - fn check_for_aliasable_mutable_writes<'a, 'tcx>(this: &CheckLoanCtxt<'a, 'tcx>, - span: Span, - cmt: mc::cmt<'tcx>) -> bool { - //! Safety checks related to writes to aliasable, mutable locations - - let guarantor = cmt.guarantor(); - debug!("check_for_aliasable_mutable_writes(cmt={:?}, guarantor={:?})", - cmt, guarantor); - if let mc::cat_deref(ref b, _, mc::BorrowedPtr(ty::MutBorrow, _)) = guarantor.cat { - // Statically prohibit writes to `&mut` when aliasable - check_for_aliasability_violation(this, span, b.clone()); - } - - return true; // no errors reported - } - - fn check_for_aliasability_violation<'a, 'tcx>(this: &CheckLoanCtxt<'a, 'tcx>, - span: Span, - cmt: mc::cmt<'tcx>) - -> bool { - match cmt.freely_aliasable(this.tcx()) { - mc::Aliasability::NonAliasable => { - return true; - } - mc::Aliasability::FreelyAliasable(mc::AliasableStaticMut(..)) => { - return true; - } - mc::Aliasability::ImmutableUnique(_) => { - this.bccx.report_aliasability_violation( - span, - MutabilityViolation, - mc::AliasableReason::UnaliasableImmutable); - return false; - } - mc::Aliasability::FreelyAliasable(cause) => { - this.bccx.report_aliasability_violation( - span, - MutabilityViolation, - cause); - return false; - } - } - } - - fn check_for_assignment_to_borrowed_path<'a, 'tcx>( - this: &CheckLoanCtxt<'a, 'tcx>, - assignment_id: ast::NodeId, - assignment_span: Span, - assignee_cmt: mc::cmt<'tcx>) - { - //! Check for assignments that violate the terms of an - //! outstanding loan. - - let loan_path = match opt_loan_path(&assignee_cmt) { - Some(lp) => lp, - None => { return; /* no loan path, can't be any loans */ } - }; - - let scope = region::CodeExtent::from_node_id(assignment_id); - this.each_in_scope_loan_affecting_path(scope, &*loan_path, |loan| { - this.report_illegal_mutation(assignment_span, &*loan_path, loan); + if !assignee_cmt.mutbl.is_mutable() { + self.bccx.report_reassigned_immutable_variable( + assignment_span, + &*lp, + assign); + } false }); + return } } diff --git a/src/librustc_borrowck/borrowck/gather_loans/lifetime.rs b/src/librustc_borrowck/borrowck/gather_loans/lifetime.rs index 427d78e89b3e..919bc45f00dd 100644 --- a/src/librustc_borrowck/borrowck/gather_loans/lifetime.rs +++ b/src/librustc_borrowck/borrowck/gather_loans/lifetime.rs @@ -137,7 +137,7 @@ impl<'a, 'tcx> GuaranteeLifetimeContext<'a, 'tcx> { fn report_error(&self, code: bckerr_code) { self.bccx.report(BckError { cmt: self.cmt_original.clone(), span: self.span, - cause: self.cause, + cause: BorrowViolation(self.cause), code: code }); } } diff --git a/src/librustc_borrowck/borrowck/gather_loans/mod.rs b/src/librustc_borrowck/borrowck/gather_loans/mod.rs index 44a4a0d25040..39b9f0760435 100644 --- a/src/librustc_borrowck/borrowck/gather_loans/mod.rs +++ b/src/librustc_borrowck/borrowck/gather_loans/mod.rs @@ -151,22 +151,10 @@ impl<'a, 'tcx> euv::Delegate<'tcx> for GatherLoanCtxt<'a, 'tcx> { assignee_cmt: mc::cmt<'tcx>, mode: euv::MutateMode) { - let opt_lp = opt_loan_path(&assignee_cmt); - debug!("mutate(assignment_id={}, assignee_cmt={:?}) opt_lp={:?}", - assignment_id, assignee_cmt, opt_lp); - - match opt_lp { - Some(lp) => { - gather_moves::gather_assignment(self.bccx, &self.move_data, - assignment_id, assignment_span, - lp, assignee_cmt.id, mode); - } - None => { - // This can occur with e.g. `*foo() = 5`. In such - // cases, there is no need to check for conflicts - // with moves etc, just ignore. - } - } + self.guarantee_assignment_valid(assignment_id, + assignment_span, + assignee_cmt, + mode); } fn decl_without_init(&mut self, id: ast::NodeId, span: Span) { @@ -177,7 +165,7 @@ impl<'a, 'tcx> euv::Delegate<'tcx> for GatherLoanCtxt<'a, 'tcx> { /// Implements the A-* rules in README.md. fn check_aliasability<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>, borrow_span: Span, - loan_cause: euv::LoanCause, + loan_cause: AliasableViolationKind, cmt: mc::cmt<'tcx>, req_kind: ty::BorrowKind) -> Result<(),()> { @@ -203,7 +191,7 @@ fn check_aliasability<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>, (mc::Aliasability::ImmutableUnique(_), ty::MutBorrow) => { bccx.report_aliasability_violation( borrow_span, - BorrowViolation(loan_cause), + loan_cause, mc::AliasableReason::UnaliasableImmutable); Err(()) } @@ -211,7 +199,7 @@ fn check_aliasability<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>, (mc::Aliasability::FreelyAliasable(alias_cause), ty::MutBorrow) => { bccx.report_aliasability_violation( borrow_span, - BorrowViolation(loan_cause), + loan_cause, alias_cause); Err(()) } @@ -221,9 +209,94 @@ fn check_aliasability<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>, } } +/// Implements the M-* rules in README.md. +fn check_mutability<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>, + borrow_span: Span, + cause: AliasableViolationKind, + cmt: mc::cmt<'tcx>, + req_kind: ty::BorrowKind) + -> Result<(),()> { + debug!("check_mutability(cause={:?} cmt={:?} req_kind={:?}", + cause, cmt, req_kind); + match req_kind { + ty::UniqueImmBorrow | ty::ImmBorrow => { + match cmt.mutbl { + // I am intentionally leaving this here to help + // refactoring if, in the future, we should add new + // kinds of mutability. + mc::McImmutable | mc::McDeclared | mc::McInherited => { + // both imm and mut data can be lent as imm; + // for mutable data, this is a freeze + Ok(()) + } + } + } + + ty::MutBorrow => { + // Only mutable data can be lent as mutable. + if !cmt.mutbl.is_mutable() { + Err(bccx.report(BckError { span: borrow_span, + cause: cause, + cmt: cmt, + code: err_mutbl })) + } else { + Ok(()) + } + } + } +} + impl<'a, 'tcx> GatherLoanCtxt<'a, 'tcx> { pub fn tcx(&self) -> &'a ty::ctxt<'tcx> { self.bccx.tcx } + /// Guarantees that `cmt` is assignable, or reports an error. + fn guarantee_assignment_valid(&mut self, + assignment_id: ast::NodeId, + assignment_span: Span, + cmt: mc::cmt<'tcx>, + mode: euv::MutateMode) { + + let opt_lp = opt_loan_path(&cmt); + debug!("guarantee_assignment_valid(assignment_id={}, cmt={:?}) opt_lp={:?}", + assignment_id, cmt, opt_lp); + + if let mc::cat_local(..) = cmt.cat { + // Only re-assignments to locals require it to be + // mutable - this is checked in check_loans. + } else { + // Check that we don't allow assignments to non-mutable data. + if check_mutability(self.bccx, assignment_span, MutabilityViolation, + cmt.clone(), ty::MutBorrow).is_err() { + return; // reported an error, no sense in reporting more. + } + } + + // Check that we don't allow assignments to aliasable data + if check_aliasability(self.bccx, assignment_span, MutabilityViolation, + cmt.clone(), ty::MutBorrow).is_err() { + return; // reported an error, no sense in reporting more. + } + + match opt_lp { + Some(lp) => { + if let mc::cat_local(..) = cmt.cat { + // Only re-assignments to locals require it to be + // mutable - this is checked in check_loans. + } else { + self.mark_loan_path_as_mutated(&lp); + } + gather_moves::gather_assignment(self.bccx, &self.move_data, + assignment_id, assignment_span, + lp, cmt.id, mode); + } + None => { + // This can occur with e.g. `*foo() = 5`. In such + // cases, there is no need to check for conflicts + // with moves etc, just ignore. + } + } + } + /// Guarantees that `addr_of(cmt)` will be valid for the duration of `static_scope_r`, or /// reports an error. This may entail taking out loans, which will be added to the /// `req_loan_map`. @@ -256,13 +329,13 @@ impl<'a, 'tcx> GatherLoanCtxt<'a, 'tcx> { } // Check that we don't allow mutable borrows of non-mutable data. - if check_mutability(self.bccx, borrow_span, cause, + if check_mutability(self.bccx, borrow_span, BorrowViolation(cause), cmt.clone(), req_kind).is_err() { return; // reported an error, no sense in reporting more. } // Check that we don't allow mutable borrows of aliasable data. - if check_aliasability(self.bccx, borrow_span, cause, + if check_aliasability(self.bccx, borrow_span, BorrowViolation(cause), cmt.clone(), req_kind).is_err() { return; // reported an error, no sense in reporting more. } @@ -368,43 +441,6 @@ impl<'a, 'tcx> GatherLoanCtxt<'a, 'tcx> { // restrictions: restrictions // } // } - - fn check_mutability<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>, - borrow_span: Span, - cause: euv::LoanCause, - cmt: mc::cmt<'tcx>, - req_kind: ty::BorrowKind) - -> Result<(),()> { - //! Implements the M-* rules in README.md. - debug!("check_mutability(cause={:?} cmt={:?} req_kind={:?}", - cause, cmt, req_kind); - match req_kind { - ty::UniqueImmBorrow | ty::ImmBorrow => { - match cmt.mutbl { - // I am intentionally leaving this here to help - // refactoring if, in the future, we should add new - // kinds of mutability. - mc::McImmutable | mc::McDeclared | mc::McInherited => { - // both imm and mut data can be lent as imm; - // for mutable data, this is a freeze - Ok(()) - } - } - } - - ty::MutBorrow => { - // Only mutable data can be lent as mutable. - if !cmt.mutbl.is_mutable() { - Err(bccx.report(BckError { span: borrow_span, - cause: cause, - cmt: cmt, - code: err_mutbl })) - } else { - Ok(()) - } - } - } - } } pub fn mark_loan_path_as_mutated(&self, loan_path: &LoanPath) { @@ -495,7 +531,8 @@ impl<'a, 'tcx, 'v> Visitor<'v> for StaticInitializerCtxt<'a, 'tcx> { let base_cmt = mc.cat_expr(&**base).unwrap(); let borrow_kind = ty::BorrowKind::from_mutbl(mutbl); // Check that we don't allow borrows of unsafe static items. - if check_aliasability(self.bccx, ex.span, euv::AddrOf, + if check_aliasability(self.bccx, ex.span, + BorrowViolation(euv::AddrOf), base_cmt, borrow_kind).is_err() { return; // reported an error, no sense in reporting more. } diff --git a/src/librustc_borrowck/borrowck/gather_loans/restrictions.rs b/src/librustc_borrowck/borrowck/gather_loans/restrictions.rs index 345f5378f69e..4c186dd84061 100644 --- a/src/librustc_borrowck/borrowck/gather_loans/restrictions.rs +++ b/src/librustc_borrowck/borrowck/gather_loans/restrictions.rs @@ -124,7 +124,7 @@ impl<'a, 'tcx> RestrictionsContext<'a, 'tcx> { self.bccx.report( BckError { span: self.span, - cause: self.cause, + cause: BorrowViolation(self.cause), cmt: cmt_base, code: err_borrowed_pointer_too_short( self.loan_region, lt)}); diff --git a/src/librustc_borrowck/borrowck/mod.rs b/src/librustc_borrowck/borrowck/mod.rs index 4f726044a1ba..65e3e443e7de 100644 --- a/src/librustc_borrowck/borrowck/mod.rs +++ b/src/librustc_borrowck/borrowck/mod.rs @@ -546,12 +546,12 @@ pub enum bckerr_code { #[derive(PartialEq)] pub struct BckError<'tcx> { span: Span, - cause: euv::LoanCause, + cause: AliasableViolationKind, cmt: mc::cmt<'tcx>, code: bckerr_code } -#[derive(Copy, Clone)] +#[derive(Copy, Clone, Debug, PartialEq)] pub enum AliasableViolationKind { MutabilityViolation, BorrowViolation(euv::LoanCause) @@ -576,8 +576,10 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> { pub fn report(&self, err: BckError<'tcx>) { // Catch and handle some particular cases. match (&err.code, &err.cause) { - (&err_out_of_scope(ty::ReScope(_), ty::ReStatic), &euv::ClosureCapture(span)) | - (&err_out_of_scope(ty::ReScope(_), ty::ReFree(..)), &euv::ClosureCapture(span)) => { + (&err_out_of_scope(ty::ReScope(_), ty::ReStatic), + &BorrowViolation(euv::ClosureCapture(span))) | + (&err_out_of_scope(ty::ReScope(_), ty::ReFree(..)), + &BorrowViolation(euv::ClosureCapture(span))) => { return self.report_out_of_scope_escaping_closure_capture(&err, span); } _ => { } @@ -796,10 +798,6 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> { self.tcx.sess.span_end_note(s, m); } - pub fn span_help(&self, s: Span, m: &str) { - self.tcx.sess.span_help(s, m); - } - pub fn fileline_help(&self, s: Span, m: &str) { self.tcx.sess.fileline_help(s, m); } @@ -827,19 +825,22 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> { }; match err.cause { - euv::ClosureCapture(_) => { + MutabilityViolation => { + format!("cannot assign to {}", descr) + } + BorrowViolation(euv::ClosureCapture(_)) => { format!("closure cannot assign to {}", descr) } - euv::OverloadedOperator | - euv::AddrOf | - euv::RefBinding | - euv::AutoRef | - euv::AutoUnsafe | - euv::ForLoop | - euv::MatchDiscriminant => { + BorrowViolation(euv::OverloadedOperator) | + BorrowViolation(euv::AddrOf) | + BorrowViolation(euv::RefBinding) | + BorrowViolation(euv::AutoRef) | + BorrowViolation(euv::AutoUnsafe) | + BorrowViolation(euv::ForLoop) | + BorrowViolation(euv::MatchDiscriminant) => { format!("cannot borrow {} as mutable", descr) } - euv::ClosureInvocation => { + BorrowViolation(euv::ClosureInvocation) => { self.tcx.sess.span_bug(err.span, "err_mutbl with a closure invocation"); }