diff --git a/src/librustc/middle/borrowck/check_loans.rs b/src/librustc/middle/borrowck/check_loans.rs index 11144118047..166d069880f 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 } @@ -189,29 +195,75 @@ impl<'a> CheckLoanCtxt<'a> { }) } - pub fn each_in_scope_restriction(&self, - scope_id: ast::NodeId, - loan_path: &LoanPath, - op: |&Loan, &Restriction| -> bool) - -> bool { - //! Iterates through all the in-scope restrictions for the - //! given `loan_path` + 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. - self.each_in_scope_loan(scope_id, |loan| { - debug!("each_in_scope_restriction found loan: {:?}", - loan.repr(self.tcx())); + // 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 in loan.restrictions.iter() { - if *restr.loan_path == *loan_path { - if !op(loan, restr) { + 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 { @@ -288,26 +340,12 @@ 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, + if compatible_borrow_kinds(loan1.kind, loan2.kind) { + return true; + } - // 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); - - for restr in loan1.restrictions.iter() { - if !restr.set.intersects(illegal_if) { continue; } - 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() @@ -534,15 +572,8 @@ 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, _restr| { - if incompatible(loan.kind, borrow_kind) { + 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 } else { @@ -550,47 +581,7 @@ 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 && - incompatible(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; - - 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, @@ -668,11 +659,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 && - check_for_assignment_to_restricted_or_frozen_location( - self, assignment_id, assignment_span, assignee_cmt.clone()) - { - // Safe, but record for lint pass later: + 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); } } @@ -807,138 +796,24 @@ 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, - 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* - // 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` 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. - 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 - } + 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 false } - - // 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 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). - // - // 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 { - 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, _))) => { - lp_base.clone() - } - - // Otherwise stop iterating - LpExtend(_, mc::McDeclared, _) | - LpExtend(_, mc::McImmutable, _) | - LpVar(_) => { - return true; - } - }; - - // Check for a non-const loan of `loan_path` - let cont = 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 - } - }); - - if !cont { return false } - } } } diff --git a/src/librustc/middle/borrowck/gather_loans/mod.rs b/src/librustc/middle/borrowck/gather_loans/mod.rs index ac94b735640..89f304513ff 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 { @@ -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, } } @@ -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 7c1f5937472..5b3e1ac0b2c 100644 --- a/src/librustc/middle/borrowck/gather_loans/restrictions.rs +++ b/src/librustc/middle/borrowck/gather_loans/restrictions.rs @@ -23,15 +23,14 @@ use std::rc::Rc; pub enum RestrictionResult { Safe, - SafeIf(Rc, Vec) + SafeIf(Rc, Vec>) } 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!(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 @@ -133,7 +122,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,12 +137,12 @@ 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; } - 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(lp.clone()); SafeIf(lp, base_vec) } } diff --git a/src/librustc/middle/borrowck/mod.rs b/src/librustc/middle/borrowck/mod.rs index 44d722c2094..18cd0b1326d 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; @@ -182,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, @@ -250,58 +249,6 @@ 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) - } -} - /////////////////////////////////////////////////////////////////////////// // Errors @@ -310,8 +257,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 +657,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)) @@ -827,15 +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({}, {:x})", - self.loan_path.repr(tcx), - self.set.bits as uint)).to_string() + self.restricted_paths.repr(tcx))).to_string() } }