From 2f77212e0c37ee31df12373622974977ba771a04 Mon Sep 17 00:00:00 2001 From: Cameron Zwarich Date: Mon, 16 Jun 2014 15:40:20 -0700 Subject: [PATCH 01/14] Remove comments that no longer apply after the removal of `const` --- src/librustc/middle/borrowck/check_loans.rs | 42 ++------------------- 1 file changed, 4 insertions(+), 38 deletions(-) diff --git a/src/librustc/middle/borrowck/check_loans.rs b/src/librustc/middle/borrowck/check_loans.rs index 111441180475..077bb99e21b6 100644 --- a/src/librustc/middle/borrowck/check_loans.rs +++ b/src/librustc/middle/borrowck/check_loans.rs @@ -829,28 +829,8 @@ impl<'a> CheckLoanCtxt<'a> { // let p = &v; // v = ~[4]; // - // In this case, creating `p` triggers a RESTR_MUTATE - // restriction on the path `v`. - // - // Here is a second, more subtle example: - // - // let mut v = ~[1, 2, 3]; - // let p = &const v[0]; - // v[0] = 4; // OK - // v[1] = 5; // OK - // v = ~[4, 5, 3]; // Error - // - // In this case, `p` is pointing to `v[0]`, and it is a - // `const` pointer in any case. So the first two - // assignments are legal (and would be permitted by this - // check). However, the final assignment (which is - // logically equivalent) is forbidden, because it would - // cause the existing `v` array to be freed, thus - // invalidating `p`. In the code, this error results - // because `gather_loans::restrictions` adds a - // `RESTR_MUTATE` restriction whenever the contents of an - // owned pointer are borrowed, and hence while `v[*]` is not - // restricted from being written, `v` is. + // In this case, creating `p` restricts the mutation of `v`. + let cont = this.each_in_scope_restriction(assignment_id, &*loan_path, |loan, restr| { @@ -882,28 +862,14 @@ impl<'a> CheckLoanCtxt<'a> { // // So in this loop, we walk back up the loan path so long // as the mutability of the path is dependent on a super - // path, and check that the super path was not lent out as - // mutable or immutable (a const loan is ok). + // path, and check that the super path was not borrowed. // // Mutability of a path can be dependent on the super path // in two ways. First, it might be inherited mutability. // Second, the pointee of an `&mut` pointer can only be // mutated if it is found in an unaliased location, so we // have to check that the owner location is not borrowed. - // - // Note that we are *not* checking for any and all - // restrictions. We are only interested in the pointers - // that the user created, whereas we add restrictions for - // all kinds of paths that are not directly aliased. If we checked - // for all restrictions, and not just loans, then the following - // valid program would be considered illegal: - // - // let mut v = ~[1, 2, 3]; - // let p = &const v[0]; - // v[1] = 5; // ok - // - // Here the restriction that `v` not be mutated would be misapplied - // to block the subpath `v[1]`. + let full_loan_path = loan_path.clone(); let mut loan_path = loan_path; loop { From 2d3f1225342760bcf986d0a7d3945ddacce47ab2 Mon Sep 17 00:00:00 2001 From: Cameron Zwarich Date: Mon, 16 Jun 2014 15:40:20 -0700 Subject: [PATCH 02/14] Remove a pointless check for intersection with RESTR_MUTATE Now that all loans restrict mutation, there's no point in checking for intersection with RESTR_MUTATE. --- src/librustc/middle/borrowck/check_loans.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/librustc/middle/borrowck/check_loans.rs b/src/librustc/middle/borrowck/check_loans.rs index 077bb99e21b6..969a8c139a4b 100644 --- a/src/librustc/middle/borrowck/check_loans.rs +++ b/src/librustc/middle/borrowck/check_loans.rs @@ -833,13 +833,9 @@ impl<'a> CheckLoanCtxt<'a> { let cont = this.each_in_scope_restriction(assignment_id, &*loan_path, - |loan, restr| { - if restr.set.intersects(RESTR_MUTATE) { - this.report_illegal_mutation(assignment_span, &*loan_path, loan); - false - } else { - true - } + |loan, _restr| { + this.report_illegal_mutation(assignment_span, &*loan_path, loan); + false }); if !cont { return false } From d2ca7465aa0b589b9a3e84423f5073da33bf0bb3 Mon Sep 17 00:00:00 2001 From: Cameron Zwarich Date: Mon, 16 Jun 2014 15:40:20 -0700 Subject: [PATCH 03/14] Remove an unused closure parameter Remove the unused &Restriction parameter of each_in_scope_restriction's op parameter. --- src/librustc/middle/borrowck/check_loans.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/librustc/middle/borrowck/check_loans.rs b/src/librustc/middle/borrowck/check_loans.rs index 969a8c139a4b..3c8516a57066 100644 --- a/src/librustc/middle/borrowck/check_loans.rs +++ b/src/librustc/middle/borrowck/check_loans.rs @@ -192,7 +192,7 @@ impl<'a> CheckLoanCtxt<'a> { pub fn each_in_scope_restriction(&self, scope_id: ast::NodeId, loan_path: &LoanPath, - op: |&Loan, &Restriction| -> bool) + op: |&Loan| -> bool) -> bool { //! Iterates through all the in-scope restrictions for the //! given `loan_path` @@ -204,7 +204,7 @@ impl<'a> CheckLoanCtxt<'a> { let mut ret = true; for restr in loan.restrictions.iter() { if *restr.loan_path == *loan_path { - if !op(loan, restr) { + if !op(loan) { ret = false; break; } @@ -541,7 +541,7 @@ impl<'a> CheckLoanCtxt<'a> { // let x = &mut a.b.c; // Restricts a, a.b, and a.b.c // let y = a; // Conflicts with restriction - self.each_in_scope_restriction(expr_id, use_path, |loan, _restr| { + self.each_in_scope_restriction(expr_id, use_path, |loan| { if incompatible(loan.kind, borrow_kind) { ret = UseWhileBorrowed(loan.loan_path.clone(), loan.span); false @@ -833,7 +833,7 @@ impl<'a> CheckLoanCtxt<'a> { let cont = this.each_in_scope_restriction(assignment_id, &*loan_path, - |loan, _restr| { + |loan| { this.report_illegal_mutation(assignment_span, &*loan_path, loan); false }); From 6ca8dbfaedc9d37b9b40fd562755a358e7810266 Mon Sep 17 00:00:00 2001 From: Cameron Zwarich Date: Mon, 16 Jun 2014 15:40:20 -0700 Subject: [PATCH 04/14] Make a new compatible_borrow_kinds helper function Move the `incompatible` helper function from analyze_restrictions_on_use to the file scope and invert its meaning to account for future uses. --- src/librustc/middle/borrowck/check_loans.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/librustc/middle/borrowck/check_loans.rs b/src/librustc/middle/borrowck/check_loans.rs index 3c8516a57066..9061d2d38d3b 100644 --- a/src/librustc/middle/borrowck/check_loans.rs +++ b/src/librustc/middle/borrowck/check_loans.rs @@ -155,6 +155,12 @@ enum UseError { UseWhileBorrowed(/*loan*/Rc, /*loan*/Span) } +fn compatible_borrow_kinds(borrow_kind1: ty::BorrowKind, + borrow_kind2: ty::BorrowKind) + -> bool { + borrow_kind1 == ty::ImmBorrow && borrow_kind2 == ty::ImmBorrow +} + impl<'a> CheckLoanCtxt<'a> { pub fn tcx(&self) -> &'a ty::ctxt { self.bccx.tcx } @@ -542,7 +548,7 @@ impl<'a> CheckLoanCtxt<'a> { // let y = a; // Conflicts with restriction self.each_in_scope_restriction(expr_id, use_path, |loan| { - if incompatible(loan.kind, borrow_kind) { + if !compatible_borrow_kinds(loan.kind, borrow_kind) { ret = UseWhileBorrowed(loan.loan_path.clone(), loan.span); false } else { @@ -566,7 +572,7 @@ impl<'a> CheckLoanCtxt<'a> { loop { self.each_in_scope_loan(expr_id, |loan| { if *loan.loan_path == *loan_path && - incompatible(loan.kind, borrow_kind) { + !compatible_borrow_kinds(loan.kind, borrow_kind) { ret = UseWhileBorrowed(loan.loan_path.clone(), loan.span); false } else { @@ -585,12 +591,6 @@ impl<'a> CheckLoanCtxt<'a> { } return ret; - - fn incompatible(borrow_kind1: ty::BorrowKind, - borrow_kind2: ty::BorrowKind) - -> bool { - borrow_kind1 != ty::ImmBorrow || borrow_kind2 != ty::ImmBorrow - } } fn check_if_path_is_moved(&self, From 6849362f383c9c21a72cea5377c28596061c6c13 Mon Sep 17 00:00:00 2001 From: Cameron Zwarich Date: Mon, 16 Jun 2014 15:40:20 -0700 Subject: [PATCH 05/14] Remove the last actual usage of RestrictionSet Switch to checking BorrowKind values of loans instead of their RestrictionSet values. This was the last code that made a decision based on a RestrictionSet. --- src/librustc/middle/borrowck/check_loans.rs | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-) diff --git a/src/librustc/middle/borrowck/check_loans.rs b/src/librustc/middle/borrowck/check_loans.rs index 9061d2d38d3b..20c0f8f78665 100644 --- a/src/librustc/middle/borrowck/check_loans.rs +++ b/src/librustc/middle/borrowck/check_loans.rs @@ -294,25 +294,11 @@ impl<'a> CheckLoanCtxt<'a> { loan1.repr(self.tcx()), loan2.repr(self.tcx())); - // Restrictions that would cause the new loan to be illegal: - let illegal_if = match loan2.kind { - // Look for restrictions against mutation. These are - // generated by all other borrows. - ty::MutBorrow => RESTR_MUTATE, - - // Look for restrictions against freezing (immutable borrows). - // These are generated by `&mut` borrows. - ty::ImmBorrow => RESTR_FREEZE, - - // No matter how the data is borrowed (as `&`, as `&mut`, - // or as `&unique imm`) it will always generate a - // restriction against mutating the data. So look for those. - ty::UniqueImmBorrow => RESTR_MUTATE, - }; - debug!("illegal_if={:?}", illegal_if); + if compatible_borrow_kinds(loan1.kind, loan2.kind) { + return true; + } for restr in loan1.restrictions.iter() { - if !restr.set.intersects(illegal_if) { continue; } if restr.loan_path != loan2.loan_path { continue; } let old_pronoun = if new_loan.loan_path == old_loan.loan_path { From 99347591956958ab62074a41a83b4a62587c2ee6 Mon Sep 17 00:00:00 2001 From: Cameron Zwarich Date: Mon, 16 Jun 2014 15:40:20 -0700 Subject: [PATCH 06/14] Remove an unused bkcerr_code constructor parameter The RestrictionSet parameter of the err_borrowed_pointer_too_short constructor isn't actually used, so it can be removed. --- src/librustc/middle/borrowck/gather_loans/restrictions.rs | 4 ++-- src/librustc/middle/borrowck/mod.rs | 5 ++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/librustc/middle/borrowck/gather_loans/restrictions.rs b/src/librustc/middle/borrowck/gather_loans/restrictions.rs index 7c1f59374729..94d88915b56a 100644 --- a/src/librustc/middle/borrowck/gather_loans/restrictions.rs +++ b/src/librustc/middle/borrowck/gather_loans/restrictions.rs @@ -133,7 +133,7 @@ impl<'a> RestrictionsContext<'a> { cause: self.cause, cmt: cmt_base, code: err_borrowed_pointer_too_short( - self.loan_region, lt, restrictions)}); + self.loan_region, lt)}); return Safe; } Safe @@ -148,7 +148,7 @@ impl<'a> RestrictionsContext<'a> { cause: self.cause, cmt: cmt_base, code: err_borrowed_pointer_too_short( - self.loan_region, lt, restrictions)}); + self.loan_region, lt)}); return Safe; } diff --git a/src/librustc/middle/borrowck/mod.rs b/src/librustc/middle/borrowck/mod.rs index 44d722c20949..aded2974c125 100644 --- a/src/librustc/middle/borrowck/mod.rs +++ b/src/librustc/middle/borrowck/mod.rs @@ -310,8 +310,7 @@ impl Repr for RestrictionSet { pub enum bckerr_code { err_mutbl, err_out_of_scope(ty::Region, ty::Region), // superscope, subscope - err_borrowed_pointer_too_short( - ty::Region, ty::Region, RestrictionSet), // loan, ptr + err_borrowed_pointer_too_short(ty::Region, ty::Region), // loan, ptr } // Combination of an error code and the categorization of the expression @@ -711,7 +710,7 @@ impl<'a> BorrowckCtxt<'a> { suggestion); } - err_borrowed_pointer_too_short(loan_scope, ptr_scope, _) => { + err_borrowed_pointer_too_short(loan_scope, ptr_scope) => { let descr = match opt_loan_path(&err.cmt) { Some(lp) => { format!("`{}`", self.loan_path_to_str(&*lp)) From 59309e0d9bf3eba0f4219062b451ff10cbdf0e2b Mon Sep 17 00:00:00 2001 From: Cameron Zwarich Date: Mon, 16 Jun 2014 15:40:20 -0700 Subject: [PATCH 07/14] Remove RestrictionSet Now that RestrictionSet is no longer being used for anything meaningful, it can be removed, along with any other associated functions and RestrictionSet fields of other types. --- .../middle/borrowck/gather_loans/mod.rs | 17 +----- .../borrowck/gather_loans/restrictions.rs | 45 ++++++---------- src/librustc/middle/borrowck/mod.rs | 53 +------------------ 3 files changed, 17 insertions(+), 98 deletions(-) diff --git a/src/librustc/middle/borrowck/gather_loans/mod.rs b/src/librustc/middle/borrowck/gather_loans/mod.rs index ac94b7356406..a9420a3c4095 100644 --- a/src/librustc/middle/borrowck/gather_loans/mod.rs +++ b/src/librustc/middle/borrowck/gather_loans/mod.rs @@ -259,7 +259,7 @@ impl<'a> GatherLoanCtxt<'a> { // loan is safe. let restr = restrictions::compute_restrictions( self.bccx, borrow_span, cause, - cmt.clone(), loan_region, self.restriction_set(req_kind)); + cmt.clone(), loan_region); // Create the loan record (if needed). let loan = match restr { @@ -390,21 +390,6 @@ impl<'a> GatherLoanCtxt<'a> { } } - fn restriction_set(&self, req_kind: ty::BorrowKind) -> RestrictionSet { - match req_kind { - // If borrowing data as immutable, no mutation allowed: - ty::ImmBorrow => RESTR_MUTATE, - - // If borrowing data as mutable, no mutation nor other - // borrows allowed: - ty::MutBorrow => RESTR_MUTATE | RESTR_FREEZE, - - // If borrowing data as unique imm, no mutation nor other - // borrows allowed: - ty::UniqueImmBorrow => RESTR_MUTATE | RESTR_FREEZE, - } - } - pub fn mark_loan_path_as_mutated(&self, loan_path: &LoanPath) { //! For mutable loans of content whose mutability derives //! from a local variable, mark the mutability decl as necessary. diff --git a/src/librustc/middle/borrowck/gather_loans/restrictions.rs b/src/librustc/middle/borrowck/gather_loans/restrictions.rs index 94d88915b56a..de875dfff3ee 100644 --- a/src/librustc/middle/borrowck/gather_loans/restrictions.rs +++ b/src/librustc/middle/borrowck/gather_loans/restrictions.rs @@ -30,8 +30,7 @@ pub fn compute_restrictions(bccx: &BorrowckCtxt, span: Span, cause: euv::LoanCause, cmt: mc::cmt, - loan_region: ty::Region, - restr: RestrictionSet) -> RestrictionResult { + loan_region: ty::Region) -> RestrictionResult { let ctxt = RestrictionsContext { bccx: bccx, span: span, @@ -39,7 +38,7 @@ pub fn compute_restrictions(bccx: &BorrowckCtxt, loan_region: loan_region, }; - ctxt.restrict(cmt, restr) + ctxt.restrict(cmt) } /////////////////////////////////////////////////////////////////////////// @@ -54,11 +53,8 @@ struct RestrictionsContext<'a> { impl<'a> RestrictionsContext<'a> { fn restrict(&self, - cmt: mc::cmt, - restrictions: RestrictionSet) -> RestrictionResult { - debug!("restrict(cmt={}, restrictions={})", - cmt.repr(self.bccx.tcx), - restrictions.repr(self.bccx.tcx)); + cmt: mc::cmt) -> RestrictionResult { + debug!("restrict(cmt={})", cmt.repr(self.bccx.tcx)); match cmt.cat.clone() { mc::cat_rvalue(..) => { @@ -75,19 +71,14 @@ impl<'a> RestrictionsContext<'a> { mc::cat_upvar(ty::UpvarId {var_id: local_id, ..}, _) => { // R-Variable let lp = Rc::new(LpVar(local_id)); - SafeIf(lp.clone(), vec!(Restriction { - loan_path: lp, - set: restrictions - })) + SafeIf(lp.clone(), vec!(Restriction { loan_path: lp })) } mc::cat_downcast(cmt_base) => { // When we borrow the interior of an enum, we have to // ensure the enum itself is not mutated, because that // could cause the type of the memory to change. - self.restrict( - cmt_base, - restrictions | RESTR_MUTATE) + self.restrict(cmt_base) } mc::cat_interior(cmt_base, i) => { @@ -96,8 +87,8 @@ impl<'a> RestrictionsContext<'a> { // Overwriting the base would not change the type of // the memory, so no additional restrictions are // needed. - let result = self.restrict(cmt_base, restrictions); - self.extend(result, cmt.mutbl, LpInterior(i), restrictions) + let result = self.restrict(cmt_base); + self.extend(result, cmt.mutbl, LpInterior(i)) } mc::cat_deref(cmt_base, _, pk @ mc::OwnedPtr) | @@ -112,10 +103,8 @@ impl<'a> RestrictionsContext<'a> { // same, because this could be the last ref. // Eventually we should make these non-special and // just rely on Deref implementation. - let result = self.restrict( - cmt_base, - restrictions | RESTR_MUTATE); - self.extend(result, cmt.mutbl, LpDeref(pk), restrictions) + let result = self.restrict(cmt_base); + self.extend(result, cmt.mutbl, LpDeref(pk)) } mc::cat_copied_upvar(..) | // FIXME(#2152) allow mutation of upvars @@ -152,8 +141,8 @@ impl<'a> RestrictionsContext<'a> { return Safe; } - let result = self.restrict(cmt_base, restrictions); - self.extend(result, cmt.mutbl, LpDeref(pk), restrictions) + let result = self.restrict(cmt_base); + self.extend(result, cmt.mutbl, LpDeref(pk)) } mc::cat_deref(_, _, mc::UnsafePtr(..)) => { @@ -162,7 +151,7 @@ impl<'a> RestrictionsContext<'a> { } mc::cat_discr(cmt_base, _) => { - self.restrict(cmt_base, restrictions) + self.restrict(cmt_base) } } } @@ -170,16 +159,12 @@ impl<'a> RestrictionsContext<'a> { fn extend(&self, result: RestrictionResult, mc: mc::MutabilityCategory, - elem: LoanPathElem, - restrictions: RestrictionSet) -> RestrictionResult { + elem: LoanPathElem) -> RestrictionResult { match result { Safe => Safe, SafeIf(base_lp, mut base_vec) => { let lp = Rc::new(LpExtend(base_lp, mc, elem)); - base_vec.push(Restriction { - loan_path: lp.clone(), - set: restrictions - }); + base_vec.push(Restriction { loan_path: lp.clone() }); SafeIf(lp, base_vec) } } diff --git a/src/librustc/middle/borrowck/mod.rs b/src/librustc/middle/borrowck/mod.rs index aded2974c125..77b57dfc593d 100644 --- a/src/librustc/middle/borrowck/mod.rs +++ b/src/librustc/middle/borrowck/mod.rs @@ -21,7 +21,6 @@ use middle::ty; use util::ppaux::{note_and_explain_region, Repr, UserString}; use std::cell::{Cell}; -use std::ops::{BitOr, BitAnd}; use std::rc::Rc; use std::gc::{Gc, GC}; use std::string::String; @@ -250,56 +249,8 @@ pub fn opt_loan_path(cmt: &mc::cmt) -> Option> { } } -/////////////////////////////////////////////////////////////////////////// -// Restrictions -// -// Borrowing an lvalue often results in *restrictions* that limit what -// can be done with this lvalue during the scope of the loan: -// -// - `RESTR_MUTATE`: The lvalue may not be modified or `&mut` borrowed. -// - `RESTR_FREEZE`: `&` borrows of the lvalue are forbidden. -// -// In addition, no value which is restricted may be moved. Therefore, -// restrictions are meaningful even if the RestrictionSet is empty, -// because the restriction against moves is implied. - pub struct Restriction { loan_path: Rc, - set: RestrictionSet -} - -#[deriving(PartialEq)] -pub struct RestrictionSet { - bits: u32 -} - -#[allow(dead_code)] // potentially useful -pub static RESTR_EMPTY: RestrictionSet = RestrictionSet {bits: 0b0000}; -pub static RESTR_MUTATE: RestrictionSet = RestrictionSet {bits: 0b0001}; -pub static RESTR_FREEZE: RestrictionSet = RestrictionSet {bits: 0b0010}; - -impl RestrictionSet { - pub fn intersects(&self, restr: RestrictionSet) -> bool { - (self.bits & restr.bits) != 0 - } -} - -impl BitOr for RestrictionSet { - fn bitor(&self, rhs: &RestrictionSet) -> RestrictionSet { - RestrictionSet {bits: self.bits | rhs.bits} - } -} - -impl BitAnd for RestrictionSet { - fn bitand(&self, rhs: &RestrictionSet) -> RestrictionSet { - RestrictionSet {bits: self.bits & rhs.bits} - } -} - -impl Repr for RestrictionSet { - fn repr(&self, _tcx: &ty::ctxt) -> String { - format!("RestrictionSet(0x{:x})", self.bits as uint) - } } /////////////////////////////////////////////////////////////////////////// @@ -832,9 +783,7 @@ impl Repr for Loan { impl Repr for Restriction { fn repr(&self, tcx: &ty::ctxt) -> String { - (format!("Restriction({}, {:x})", - self.loan_path.repr(tcx), - self.set.bits as uint)).to_string() + (format!("Restriction({})", self.loan_path.repr(tcx))).to_string() } } From e018fc36d17685c31d6b31acf17a9a8f2f73f8d9 Mon Sep 17 00:00:00 2001 From: Cameron Zwarich Date: Mon, 16 Jun 2014 15:40:20 -0700 Subject: [PATCH 08/14] Remove Restriction The Restriction struct now consists of a single Rc field, so it can be replaced with Rc. --- src/librustc/middle/borrowck/check_loans.rs | 8 ++++---- src/librustc/middle/borrowck/gather_loans/mod.rs | 4 ++-- .../middle/borrowck/gather_loans/restrictions.rs | 6 +++--- src/librustc/middle/borrowck/mod.rs | 14 ++------------ 4 files changed, 11 insertions(+), 21 deletions(-) diff --git a/src/librustc/middle/borrowck/check_loans.rs b/src/librustc/middle/borrowck/check_loans.rs index 20c0f8f78665..70cb06e2a303 100644 --- a/src/librustc/middle/borrowck/check_loans.rs +++ b/src/librustc/middle/borrowck/check_loans.rs @@ -208,8 +208,8 @@ impl<'a> CheckLoanCtxt<'a> { loan.repr(self.tcx())); let mut ret = true; - for restr in loan.restrictions.iter() { - if *restr.loan_path == *loan_path { + for restr_path in loan.restricted_paths.iter() { + if **restr_path == *loan_path { if !op(loan) { ret = false; break; @@ -298,8 +298,8 @@ impl<'a> CheckLoanCtxt<'a> { return true; } - for restr in loan1.restrictions.iter() { - if restr.loan_path != loan2.loan_path { continue; } + for restr_path in loan1.restricted_paths.iter() { + if *restr_path != loan2.loan_path { continue; } let old_pronoun = if new_loan.loan_path == old_loan.loan_path { "it".to_string() diff --git a/src/librustc/middle/borrowck/gather_loans/mod.rs b/src/librustc/middle/borrowck/gather_loans/mod.rs index a9420a3c4095..89f304513ffb 100644 --- a/src/librustc/middle/borrowck/gather_loans/mod.rs +++ b/src/librustc/middle/borrowck/gather_loans/mod.rs @@ -268,7 +268,7 @@ impl<'a> GatherLoanCtxt<'a> { return; } - restrictions::SafeIf(loan_path, restrictions) => { + restrictions::SafeIf(loan_path, restricted_paths) => { let loan_scope = match loan_region { ty::ReScope(id) => id, ty::ReFree(ref fr) => fr.scope_id, @@ -314,7 +314,7 @@ impl<'a> GatherLoanCtxt<'a> { gen_scope: gen_scope, kill_scope: kill_scope, span: borrow_span, - restrictions: restrictions, + restricted_paths: restricted_paths, cause: cause, } } diff --git a/src/librustc/middle/borrowck/gather_loans/restrictions.rs b/src/librustc/middle/borrowck/gather_loans/restrictions.rs index de875dfff3ee..5b3e1ac0b2c7 100644 --- a/src/librustc/middle/borrowck/gather_loans/restrictions.rs +++ b/src/librustc/middle/borrowck/gather_loans/restrictions.rs @@ -23,7 +23,7 @@ use std::rc::Rc; pub enum RestrictionResult { Safe, - SafeIf(Rc, Vec) + SafeIf(Rc, Vec>) } pub fn compute_restrictions(bccx: &BorrowckCtxt, @@ -71,7 +71,7 @@ impl<'a> RestrictionsContext<'a> { mc::cat_upvar(ty::UpvarId {var_id: local_id, ..}, _) => { // R-Variable let lp = Rc::new(LpVar(local_id)); - SafeIf(lp.clone(), vec!(Restriction { loan_path: lp })) + SafeIf(lp.clone(), vec!(lp)) } mc::cat_downcast(cmt_base) => { @@ -164,7 +164,7 @@ impl<'a> RestrictionsContext<'a> { Safe => Safe, SafeIf(base_lp, mut base_vec) => { let lp = Rc::new(LpExtend(base_lp, mc, elem)); - base_vec.push(Restriction { loan_path: lp.clone() }); + base_vec.push(lp.clone()); SafeIf(lp, base_vec) } } diff --git a/src/librustc/middle/borrowck/mod.rs b/src/librustc/middle/borrowck/mod.rs index 77b57dfc593d..18cd0b1326d9 100644 --- a/src/librustc/middle/borrowck/mod.rs +++ b/src/librustc/middle/borrowck/mod.rs @@ -181,7 +181,7 @@ pub struct Loan { index: uint, loan_path: Rc, kind: ty::BorrowKind, - restrictions: Vec, + restricted_paths: Vec>, gen_scope: ast::NodeId, kill_scope: ast::NodeId, span: Span, @@ -249,10 +249,6 @@ pub fn opt_loan_path(cmt: &mc::cmt) -> Option> { } } -pub struct Restriction { - loan_path: Rc, -} - /////////////////////////////////////////////////////////////////////////// // Errors @@ -777,13 +773,7 @@ impl Repr for Loan { self.kind, self.gen_scope, self.kill_scope, - self.restrictions.repr(tcx))).to_string() - } -} - -impl Repr for Restriction { - fn repr(&self, tcx: &ty::ctxt) -> String { - (format!("Restriction({})", self.loan_path.repr(tcx))).to_string() + self.restricted_paths.repr(tcx))).to_string() } } From ba203c5c5de5af9941dc5be4b7c200db69b3c4d4 Mon Sep 17 00:00:00 2001 From: Cameron Zwarich Date: Mon, 16 Jun 2014 15:40:20 -0700 Subject: [PATCH 09/14] Add a new each_in_scope_loan_affecting_path helper function Add a helper function that generalizes the loan path restriction strategy used by analyze_restrictions_on_use. --- src/librustc/middle/borrowck/check_loans.rs | 114 ++++++++++++-------- 1 file changed, 72 insertions(+), 42 deletions(-) diff --git a/src/librustc/middle/borrowck/check_loans.rs b/src/librustc/middle/borrowck/check_loans.rs index 70cb06e2a303..131ed8dbba65 100644 --- a/src/librustc/middle/borrowck/check_loans.rs +++ b/src/librustc/middle/borrowck/check_loans.rs @@ -220,6 +220,77 @@ impl<'a> CheckLoanCtxt<'a> { }) } + fn each_in_scope_loan_affecting_path(&self, + scope_id: ast::NodeId, + loan_path: &LoanPath, + op: |&Loan| -> bool) + -> bool { + //! Iterates through all of the in-scope loans affecting `loan_path`, + //! calling `op`, and ceasing iteration if `false` is returned. + + // First, we check for a loan restricting the path P being used. This + // accounts for borrows of P but also borrows of subpaths, like P.a.b. + // Consider the following example: + // + // let x = &mut a.b.c; // Restricts a, a.b, and a.b.c + // let y = a; // Conflicts with restriction + + let cont = self.each_in_scope_loan(scope_id, |loan| { + let mut ret = true; + for restr_path in loan.restricted_paths.iter() { + if **restr_path == *loan_path { + if !op(loan) { + ret = false; + break; + } + } + } + ret + }); + + if !cont { + return false; + } + + // Next, we must check for *loans* (not restrictions) on the path P or + // any base path. This rejects examples like the following: + // + // let x = &mut a.b; + // let y = a.b.c; + // + // Limiting this search to *loans* and not *restrictions* means that + // examples like the following continue to work: + // + // let x = &mut a.b; + // let y = a.c; + + let mut loan_path = loan_path; + loop { + match *loan_path { + LpVar(_) => { + break; + } + LpExtend(ref lp_base, _, _) => { + loan_path = &**lp_base; + } + } + + let cont = self.each_in_scope_loan(scope_id, |loan| { + if *loan.loan_path == *loan_path { + op(loan) + } else { + true + } + }); + + if !cont { + return false; + } + } + + return true; + } + pub fn loans_generated_by(&self, scope_id: ast::NodeId) -> Vec { //! Returns a vector of the loans that are generated as //! we encounter `scope_id`. @@ -526,14 +597,7 @@ impl<'a> CheckLoanCtxt<'a> { let mut ret = UseOk; - // First, we check for a restriction on the path P being used. This - // accounts for borrows of P but also borrows of subpaths, like P.a.b. - // Consider the following example: - // - // let x = &mut a.b.c; // Restricts a, a.b, and a.b.c - // let y = a; // Conflicts with restriction - - self.each_in_scope_restriction(expr_id, use_path, |loan| { + self.each_in_scope_loan_affecting_path(expr_id, use_path, |loan| { if !compatible_borrow_kinds(loan.kind, borrow_kind) { ret = UseWhileBorrowed(loan.loan_path.clone(), loan.span); false @@ -542,40 +606,6 @@ impl<'a> CheckLoanCtxt<'a> { } }); - // Next, we must check for *loans* (not restrictions) on the path P or - // any base path. This rejects examples like the following: - // - // let x = &mut a.b; - // let y = a.b.c; - // - // Limiting this search to *loans* and not *restrictions* means that - // examples like the following continue to work: - // - // let x = &mut a.b; - // let y = a.c; - - let mut loan_path = use_path; - loop { - self.each_in_scope_loan(expr_id, |loan| { - if *loan.loan_path == *loan_path && - !compatible_borrow_kinds(loan.kind, borrow_kind) { - ret = UseWhileBorrowed(loan.loan_path.clone(), loan.span); - false - } else { - true - } - }); - - match *loan_path { - LpVar(_) => { - break; - } - LpExtend(ref lp_base, _, _) => { - loan_path = &**lp_base; - } - } - } - return ret; } From 702ef1b7215e310a0c5e2e761bef922f36dd5f65 Mon Sep 17 00:00:00 2001 From: Cameron Zwarich Date: Mon, 16 Jun 2014 15:40:20 -0700 Subject: [PATCH 10/14] Call mark_variable_as_used_mut even after assignment errors It doesn't actually matter if we call mark_variable_as_used_mut when the assignment was invalid, since the variable was still used mutably. --- src/librustc/middle/borrowck/check_loans.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/librustc/middle/borrowck/check_loans.rs b/src/librustc/middle/borrowck/check_loans.rs index 131ed8dbba65..ac8b0867f7ac 100644 --- a/src/librustc/middle/borrowck/check_loans.rs +++ b/src/librustc/middle/borrowck/check_loans.rs @@ -684,11 +684,9 @@ impl<'a> CheckLoanCtxt<'a> { // 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 && + if mode != euv::Init { check_for_assignment_to_restricted_or_frozen_location( - self, assignment_id, assignment_span, assignee_cmt.clone()) - { - // Safe, but record for lint pass later: + self, assignment_id, assignment_span, assignee_cmt.clone()); mark_variable_as_used_mut(self, assignee_cmt); } } From 178c4fbccbc59fc7c554cb3cda33413d7d455366 Mon Sep 17 00:00:00 2001 From: Cameron Zwarich Date: Mon, 16 Jun 2014 15:40:20 -0700 Subject: [PATCH 11/14] Remove an unused return value The only caller of check_for_assignment_to_restricted_or_frozen_location isn't checking its return value, so we can remove it and simplify the internal logic of the function. --- src/librustc/middle/borrowck/check_loans.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/librustc/middle/borrowck/check_loans.rs b/src/librustc/middle/borrowck/check_loans.rs index ac8b0867f7ac..42712206c016 100644 --- a/src/librustc/middle/borrowck/check_loans.rs +++ b/src/librustc/middle/borrowck/check_loans.rs @@ -825,14 +825,14 @@ impl<'a> CheckLoanCtxt<'a> { this: &CheckLoanCtxt, assignment_id: ast::NodeId, assignment_span: Span, - assignee_cmt: mc::cmt) -> bool + assignee_cmt: mc::cmt) { //! 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 true; /* no loan path, can't be any loans */ } + None => { return; /* no loan path, can't be any loans */ } }; // Start by searching for an assignment to a *restricted* @@ -852,7 +852,7 @@ impl<'a> CheckLoanCtxt<'a> { false }); - if !cont { return false } + if !cont { return; } // The previous code handled assignments to paths that // have been restricted. This covers paths that have been @@ -899,12 +899,12 @@ impl<'a> CheckLoanCtxt<'a> { LpExtend(_, mc::McDeclared, _) | LpExtend(_, mc::McImmutable, _) | LpVar(_) => { - return true; + return; } }; // Check for a non-const loan of `loan_path` - let cont = this.each_in_scope_loan(assignment_id, |loan| { + this.each_in_scope_loan(assignment_id, |loan| { if loan.loan_path == loan_path { this.report_illegal_mutation(assignment_span, &*full_loan_path, loan); false @@ -912,8 +912,6 @@ impl<'a> CheckLoanCtxt<'a> { true } }); - - if !cont { return false } } } } From 69f4839b926d6435df83cd80945d9f843cf60e27 Mon Sep 17 00:00:00 2001 From: Cameron Zwarich Date: Mon, 16 Jun 2014 15:40:20 -0700 Subject: [PATCH 12/14] Always check assigned loan paths to the top of the path Currently, check_for_assignment_to_restricted_or_frozen_location bails out early when looking for loaned base paths when it hits an McDeclared or McImmutable extension. With the current type system, this is actually irrelevant, since mutation can only occur given a unique mutable access path, forcing the same requirement on each base path. --- src/librustc/middle/borrowck/check_loans.rs | 24 +++------------------ 1 file changed, 3 insertions(+), 21 deletions(-) diff --git a/src/librustc/middle/borrowck/check_loans.rs b/src/librustc/middle/borrowck/check_loans.rs index 42712206c016..d36eb6751b16 100644 --- a/src/librustc/middle/borrowck/check_loans.rs +++ b/src/librustc/middle/borrowck/check_loans.rs @@ -870,34 +870,16 @@ impl<'a> CheckLoanCtxt<'a> { // here is to `v[*]`, and the existing restrictions were issued // for `v`, not `v[*]`. // - // So in this loop, we walk back up the loan path so long - // as the mutability of the path is dependent on a super - // path, and check that the super path was not borrowed. - // - // Mutability of a path can be dependent on the super path - // in two ways. First, it might be inherited mutability. - // Second, the pointee of an `&mut` pointer can only be - // mutated if it is found in an unaliased location, so we - // have to check that the owner location is not borrowed. + // So in this loop, we walk back up the path and look for + // loans, not restrictions. let full_loan_path = loan_path.clone(); let mut loan_path = loan_path; loop { loan_path = match *loan_path { - // Peel back one layer if, for `loan_path` to be - // mutable, `lp_base` must be mutable. This occurs - // with inherited mutability, owned pointers and - // `&mut` pointers. - LpExtend(ref lp_base, mc::McInherited, _) | - LpExtend(ref lp_base, _, LpDeref(mc::OwnedPtr)) | - LpExtend(ref lp_base, _, LpDeref(mc::GcPtr)) | - LpExtend(ref lp_base, _, LpDeref(mc::BorrowedPtr(ty::MutBorrow, _))) => { + LpExtend(ref lp_base, _, _) => { lp_base.clone() } - - // Otherwise stop iterating - LpExtend(_, mc::McDeclared, _) | - LpExtend(_, mc::McImmutable, _) | LpVar(_) => { return; } From a924d740df600e2a71656ed477711b0f2a68c722 Mon Sep 17 00:00:00 2001 From: Cameron Zwarich Date: Mon, 16 Jun 2014 15:40:21 -0700 Subject: [PATCH 13/14] Switch to each_in_scope_loan_affecting_path The last remaining use of each_in_scope_restriction in check_for_assignment_to_restricted_or_frozen_location is using the pattern captured by each_in_scope_loan_affecting_path, so it can be removed. --- src/librustc/middle/borrowck/check_loans.rs | 83 +-------------------- 1 file changed, 1 insertion(+), 82 deletions(-) diff --git a/src/librustc/middle/borrowck/check_loans.rs b/src/librustc/middle/borrowck/check_loans.rs index d36eb6751b16..5c18001930e2 100644 --- a/src/librustc/middle/borrowck/check_loans.rs +++ b/src/librustc/middle/borrowck/check_loans.rs @@ -195,31 +195,6 @@ impl<'a> CheckLoanCtxt<'a> { }) } - pub fn each_in_scope_restriction(&self, - scope_id: ast::NodeId, - loan_path: &LoanPath, - op: |&Loan| -> bool) - -> bool { - //! Iterates through all the in-scope restrictions for the - //! given `loan_path` - - self.each_in_scope_loan(scope_id, |loan| { - debug!("each_in_scope_restriction found loan: {:?}", - loan.repr(self.tcx())); - - let mut ret = true; - for restr_path in loan.restricted_paths.iter() { - if **restr_path == *loan_path { - if !op(loan) { - ret = false; - break; - } - } - } - ret - }) - } - fn each_in_scope_loan_affecting_path(&self, scope_id: ast::NodeId, loan_path: &LoanPath, @@ -835,66 +810,10 @@ impl<'a> CheckLoanCtxt<'a> { None => { return; /* no loan path, can't be any loans */ } }; - // Start by searching for an assignment to a *restricted* - // location. Here is one example of the kind of error caught - // by this check: - // - // let mut v = ~[1, 2, 3]; - // let p = &v; - // v = ~[4]; - // - // In this case, creating `p` restricts the mutation of `v`. - - let cont = this.each_in_scope_restriction(assignment_id, - &*loan_path, - |loan| { + this.each_in_scope_loan_affecting_path(assignment_id, &*loan_path, |loan| { this.report_illegal_mutation(assignment_span, &*loan_path, loan); false }); - - if !cont { return; } - - // The previous code handled assignments to paths that - // have been restricted. This covers paths that have been - // directly lent out and their base paths, but does not - // cover random extensions of those paths. For example, - // the following program is not declared illegal by the - // previous check: - // - // let mut v = ~[1, 2, 3]; - // let p = &v; - // v[0] = 4; // declared error by loop below, not code above - // - // The reason that this passes the previous check whereas - // an assignment like `v = ~[4]` fails is because the assignment - // here is to `v[*]`, and the existing restrictions were issued - // for `v`, not `v[*]`. - // - // So in this loop, we walk back up the path and look for - // loans, not restrictions. - - let full_loan_path = loan_path.clone(); - let mut loan_path = loan_path; - loop { - loan_path = match *loan_path { - LpExtend(ref lp_base, _, _) => { - lp_base.clone() - } - LpVar(_) => { - return; - } - }; - - // Check for a non-const loan of `loan_path` - this.each_in_scope_loan(assignment_id, |loan| { - if loan.loan_path == loan_path { - this.report_illegal_mutation(assignment_span, &*full_loan_path, loan); - false - } else { - true - } - }); - } } } From 480cd6fb9052e3db33e01c94e1193f80801b292f Mon Sep 17 00:00:00 2001 From: Cameron Zwarich Date: Mon, 16 Jun 2014 15:40:21 -0700 Subject: [PATCH 14/14] Rename check_for_assignment_to_restricted_or_frozen_location Rename check_for_assignment_to_restricted_or_frozen_location to check_for_assignment_to_borrowed_path. --- src/librustc/middle/borrowck/check_loans.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/librustc/middle/borrowck/check_loans.rs b/src/librustc/middle/borrowck/check_loans.rs index 5c18001930e2..166d069880f9 100644 --- a/src/librustc/middle/borrowck/check_loans.rs +++ b/src/librustc/middle/borrowck/check_loans.rs @@ -660,7 +660,7 @@ impl<'a> CheckLoanCtxt<'a> { 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_restricted_or_frozen_location( + check_for_assignment_to_borrowed_path( self, assignment_id, assignment_span, assignee_cmt.clone()); mark_variable_as_used_mut(self, assignee_cmt); } @@ -796,7 +796,7 @@ impl<'a> CheckLoanCtxt<'a> { } } - fn check_for_assignment_to_restricted_or_frozen_location( + fn check_for_assignment_to_borrowed_path( this: &CheckLoanCtxt, assignment_id: ast::NodeId, assignment_span: Span,