diff --git a/src/librustc/middle/borrowck/check_loans.rs b/src/librustc/middle/borrowck/check_loans.rs index f2bba4f694a9..9395c3f2a40d 100644 --- a/src/librustc/middle/borrowck/check_loans.rs +++ b/src/librustc/middle/borrowck/check_loans.rs @@ -65,7 +65,7 @@ pub fn check_loans(bccx: @BorrowckCtxt, enum MoveError { MoveOk, - MoveWhileBorrowed(/*loan*/@LoanPath, /*loan*/span) + MoveWhileBorrowed(/*move*/@LoanPath, /*loan*/@LoanPath, /*loan*/span) } pub impl<'self> CheckLoanCtxt<'self> { @@ -200,9 +200,9 @@ pub impl<'self> CheckLoanCtxt<'self> { loan1.repr(self.tcx()), loan2.repr(self.tcx())); - // Restrictions that would cause the new loan to be immutable: + // Restrictions that would cause the new loan to be illegal: let illegal_if = match loan2.mutbl { - m_mutbl => RESTR_ALIAS | RESTR_FREEZE | RESTR_MUTATE, + m_mutbl => RESTR_ALIAS | RESTR_FREEZE | RESTR_CLAIM, m_imm => RESTR_ALIAS | RESTR_FREEZE, m_const => RESTR_ALIAS, }; @@ -557,12 +557,12 @@ pub impl<'self> CheckLoanCtxt<'self> { let cmt = self.bccx.cat_expr(ex); match self.analyze_move_out_from_cmt(cmt) { MoveOk => {} - MoveWhileBorrowed(loan_path, loan_span) => { + MoveWhileBorrowed(move_path, loan_path, loan_span) => { self.bccx.span_err( cmt.span, fmt!("cannot move out of `%s` \ because it is borrowed", - self.bccx.loan_path_to_str(loan_path))); + self.bccx.loan_path_to_str(move_path))); self.bccx.span_note( loan_span, fmt!("borrow of `%s` occurs here", @@ -582,7 +582,7 @@ pub impl<'self> CheckLoanCtxt<'self> { for opt_loan_path(cmt).each |&lp| { for self.each_in_scope_restriction(cmt.id, lp) |loan, _| { // Any restriction prevents moves. - return MoveWhileBorrowed(loan.loan_path, loan.span); + return MoveWhileBorrowed(lp, loan.loan_path, loan.span); } } diff --git a/src/librustc/middle/borrowck/doc.rs b/src/librustc/middle/borrowck/doc.rs index 2151c86519a0..5e96e14dbc2c 100644 --- a/src/librustc/middle/borrowck/doc.rs +++ b/src/librustc/middle/borrowck/doc.rs @@ -580,15 +580,27 @@ borrow, `LV` remains the *sole pointer with mutable access* to `*LV`. Restrictions against mutations and claims are necessary because if the pointer in `LV` were to be somehow copied or moved to a different location, then the restriction issued for `*LV` would not apply to the -new location. Consider this example, where `*t0` is frozen, but then -`t0` and `t1` are swapped, so by mutating `*t1` the user can mutate -the frozen memory that was originally found at `*t0`: +new location. Note that because `&mut` values are non-copyable, a +simple attempt to move the base pointer will fail due to the +(implicit) restriction against moves: - fn foo(t0: &mut T, - t1: &mut T) { - let p: &T = &*t0; // Freezes `*t0` - t0 <-> t1; - *t1 = ...; // Invalidates `p` + // src/test/compile-fail/borrowck-move-mut-base-ptr.rs + fn foo(t0: &mut int) { + let p: &int = &*t0; // Freezes `*t0` + let t1 = t0; //~ ERROR cannot move out of `t0` + *t1 = 22; + } + +However, the additional restrictions against mutation mean that even a +clever attempt to use a swap to circumvent the type system will +encounter an error: + + // src/test/compile-fail/borrowck-swap-mut-base-ptr.rs + fn foo<'a>(mut t0: &'a mut int, + mut t1: &'a mut int) { + let p: &int = &*t0; // Freezes `*t0` + swap(&mut t0, &mut t1); //~ ERROR cannot borrow `t0` + *t1 = 22; } The restriction against *aliasing* (and, in turn, freezing) is @@ -598,12 +610,32 @@ pointee. Since we are only issuing restrictions against `*LV`, these other aliases would be unrestricted, and the result would be unsound. For example: - fn foo(t0: &mut T) { - let p: &T = &*t0; // Freezes *t0 - let q: &&mut T = &t0; - **q = ...; // Invalidates `p` + // src/test/compile-fail/borrowck-alias-mut-base-ptr.rs + fn foo(t0: &mut int) { + let p: &int = &*t0; // Freezes `*t0` + let q: &const &mut int = &const t0; //~ ERROR cannot borrow `t0` + **q = 22; // (*) } +Note that the current rules also report an error at the assignment in +`(*)`, because we only permit `&mut` poiners to be assigned if they +are located in a non-aliasable location. However, I do not believe +this restriction is strictly necessary. It was added, I believe, to +discourage `&mut` from being placed in aliasable locations in the +first place. One (desirable) side-effect of restricting aliasing on +`LV` is that borrowing an `&mut` pointee found inside an aliasable +pointee yields an error: + + // src/test/compile-fail/borrowck-borrow-mut-base-ptr-in-aliasable-loc: + fn foo(t0: & &mut int) { + let t1 = t0; + let p: &int = &**t0; //~ ERROR cannot borrow an `&mut` in a `&` pointer + **t1 = 22; // (*) + } + +Here at the line `(*)` you will also see the error I referred to +above, which I do not believe is strictly necessary. + The second rule for `&mut` handles the case where we are not adding any restrictions (beyond the default of "no move"): @@ -622,4 +654,22 @@ that way if we *can* find a simple static error, we will: RESTRICTIONS(*LV, ACTIONS) = [*LV, ACTIONS] // R-Deref-Managed-Borrowed TYPE(LV) = @mut Ty +# Some notes for future work + +While writing up these docs, I encountered some rules I believe to be +stricter than necessary: + +- I think the restriction against mutating `&mut` pointers found in an + aliasable location is unnecessary. They cannot be reborrowed, to be sure, + so it should be safe to mutate them. Lifting this might cause some common + cases (`&mut int`) to work just fine, but might lead to further confusion + in other cases, so maybe it's best to leave it as is. +- I think restricting the `&mut` LV against moves and `ALIAS` is sufficient, + `MUTATE` and `CLAIM` are overkill. `MUTATE` was necessary when swap was + a built-in operator, but as it is not, it is implied by `CLAIM`, + and `CLAIM` is implied by `ALIAS`. The only net effect of this is an + extra error message in some cases, though. +- I have not described how closures interact. Current code is unsound. + I am working on describing and implementing the fix. + */ diff --git a/src/librustc/middle/borrowck/gather_loans/mod.rs b/src/librustc/middle/borrowck/gather_loans/mod.rs index f733d57573c1..c2ae364e54ce 100644 --- a/src/librustc/middle/borrowck/gather_loans/mod.rs +++ b/src/librustc/middle/borrowck/gather_loans/mod.rs @@ -517,8 +517,8 @@ pub impl GatherLoanCtxt { fn restriction_set(&self, req_mutbl: ast::mutability) -> RestrictionSet { match req_mutbl { m_const => RESTR_EMPTY, - m_imm => RESTR_EMPTY | RESTR_MUTATE, - m_mutbl => RESTR_EMPTY | RESTR_MUTATE | RESTR_FREEZE + m_imm => RESTR_EMPTY | RESTR_MUTATE | RESTR_CLAIM, + m_mutbl => RESTR_EMPTY | RESTR_MUTATE | RESTR_CLAIM | RESTR_FREEZE } } diff --git a/src/librustc/middle/borrowck/gather_loans/restrictions.rs b/src/librustc/middle/borrowck/gather_loans/restrictions.rs index bf054b72ecf2..4527cdec782d 100644 --- a/src/librustc/middle/borrowck/gather_loans/restrictions.rs +++ b/src/librustc/middle/borrowck/gather_loans/restrictions.rs @@ -86,7 +86,7 @@ impl RestrictionsContext { // 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.compute(cmt_base, restrictions | RESTR_MUTATE) + self.compute(cmt_base, restrictions | RESTR_MUTATE | RESTR_CLAIM) } mc::cat_interior(cmt_base, i) => { @@ -105,7 +105,9 @@ impl RestrictionsContext { // When we borrow the interior of an owned pointer, we // cannot permit the base to be mutated, because that // would cause the unique pointer to be freed. - let result = self.compute(cmt_base, restrictions | RESTR_MUTATE); + let result = self.compute( + cmt_base, + restrictions | RESTR_MUTATE | RESTR_CLAIM); self.extend(result, cmt.mutbl, LpDeref, restrictions) } @@ -178,7 +180,9 @@ impl RestrictionsContext { // mutability, we can only prevent mutation or prevent // freezing if it is not aliased. Therefore, in such // cases we restrict aliasing on `cmt_base`. - if restrictions.intersects(RESTR_MUTATE | RESTR_FREEZE) { + if restrictions.intersects(RESTR_MUTATE | + RESTR_CLAIM | + RESTR_FREEZE) { // R-Deref-Mut-Borrowed-1 let result = self.compute(cmt_base, restrictions | RESTR_ALIAS); self.extend(result, cmt.mutbl, LpDeref, restrictions) @@ -244,7 +248,7 @@ impl RestrictionsContext { fn check_no_mutability_control(&self, cmt: mc::cmt, restrictions: RestrictionSet) { - if restrictions.intersects(RESTR_MUTATE | RESTR_FREEZE) { + if restrictions.intersects(RESTR_MUTATE | RESTR_FREEZE | RESTR_CLAIM) { self.bccx.report(BckError {span: self.span, cmt: cmt, code: err_freeze_aliasable_const}); diff --git a/src/librustc/middle/borrowck/mod.rs b/src/librustc/middle/borrowck/mod.rs index 4554fde15fad..b8c1504dbd07 100644 --- a/src/librustc/middle/borrowck/mod.rs +++ b/src/librustc/middle/borrowck/mod.rs @@ -301,10 +301,10 @@ pub fn opt_loan_path(cmt: mc::cmt) -> Option<@LoanPath> { // 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 and mutable pointers to -// the value cannot be created. -// - `RESTR_FREEZE`: Immutable pointers to the value cannot be created. -// - `RESTR_ALIAS`: The lvalue may not be aliased in any way. +// - `RESTR_MUTATE`: The lvalue may not be modified. +// - `RESTR_CLAIM`: `&mut` borrows of the lvalue are forbidden. +// - `RESTR_FREEZE`: `&` borrows of the lvalue are forbidden. +// - `RESTR_ALIAS`: All 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, @@ -319,10 +319,11 @@ pub struct RestrictionSet { bits: u32 } -pub static RESTR_EMPTY: RestrictionSet = RestrictionSet {bits: 0b000}; -pub static RESTR_MUTATE: RestrictionSet = RestrictionSet {bits: 0b001}; -pub static RESTR_FREEZE: RestrictionSet = RestrictionSet {bits: 0b010}; -pub static RESTR_ALIAS: RestrictionSet = RestrictionSet {bits: 0b100}; +pub static RESTR_EMPTY: RestrictionSet = RestrictionSet {bits: 0b0000}; +pub static RESTR_MUTATE: RestrictionSet = RestrictionSet {bits: 0b0001}; +pub static RESTR_CLAIM: RestrictionSet = RestrictionSet {bits: 0b0010}; +pub static RESTR_FREEZE: RestrictionSet = RestrictionSet {bits: 0b0100}; +pub static RESTR_ALIAS: RestrictionSet = RestrictionSet {bits: 0b1000}; pub impl RestrictionSet { fn intersects(&self, restr: RestrictionSet) -> bool { diff --git a/src/test/compile-fail/borrowck-alias-mut-base-ptr.rs b/src/test/compile-fail/borrowck-alias-mut-base-ptr.rs new file mode 100644 index 000000000000..c51cf5b9538d --- /dev/null +++ b/src/test/compile-fail/borrowck-alias-mut-base-ptr.rs @@ -0,0 +1,15 @@ +// Test that attempt to alias `&mut` pointer while pointee is borrowed +// yields an error. +// +// Example from src/middle/borrowck/doc.rs + +use std::util::swap; + +fn foo(t0: &mut int) { + let p: &int = &*t0; // Freezes `*t0` + let q: &const &mut int = &const t0; //~ ERROR cannot borrow `t0` + **q = 22; //~ ERROR cannot assign to an `&mut` in a `&const` pointer +} + +fn main() { +} \ No newline at end of file diff --git a/src/test/compile-fail/borrowck-borrow-mut-base-ptr-in-aliasable-loc.rs b/src/test/compile-fail/borrowck-borrow-mut-base-ptr-in-aliasable-loc.rs new file mode 100644 index 000000000000..7e9c298ba473 --- /dev/null +++ b/src/test/compile-fail/borrowck-borrow-mut-base-ptr-in-aliasable-loc.rs @@ -0,0 +1,31 @@ +// Test that attempt to reborrow an `&mut` pointer in an aliasable +// location yields an error. +// +// Example from src/middle/borrowck/doc.rs + +use std::util::swap; + +fn foo(t0: & &mut int) { + let t1 = t0; + let p: &int = &**t0; //~ ERROR cannot borrow an `&mut` in a `&` pointer + **t1 = 22; //~ ERROR cannot assign +} + +fn foo2(t0: &const &mut int) { + // Note: reborrowing from an &const actually yields two errors, since it + // is unsafe in two ways: we can't control the aliasing, and we can't + // control the mutation. + let t1 = t0; + let p: &int = &**t0; //~ ERROR cannot borrow an `&mut` in a `&const` pointer + //~^ ERROR unsafe borrow of aliasable, const value + **t1 = 22; //~ ERROR cannot assign +} + +fn foo3(t0: &mut &mut int) { + let t1 = &mut *t0; + let p: &int = &**t0; //~ ERROR cannot borrow + **t1 = 22; +} + +fn main() { +} \ No newline at end of file diff --git a/src/test/compile-fail/borrowck-move-mut-base-ptr.rs b/src/test/compile-fail/borrowck-move-mut-base-ptr.rs new file mode 100644 index 000000000000..6a3832d2304c --- /dev/null +++ b/src/test/compile-fail/borrowck-move-mut-base-ptr.rs @@ -0,0 +1,15 @@ +// Test that attempt to move `&mut` pointer while pointee is borrowed +// yields an error. +// +// Example from src/middle/borrowck/doc.rs + +use std::util::swap; + +fn foo(t0: &mut int) { + let p: &int = &*t0; // Freezes `*t0` + let t1 = t0; //~ ERROR cannot move out of `t0` + *t1 = 22; +} + +fn main() { +} \ No newline at end of file diff --git a/src/test/compile-fail/borrowck-swap-mut-base-ptr.rs b/src/test/compile-fail/borrowck-swap-mut-base-ptr.rs new file mode 100644 index 000000000000..bea5f1f6ea76 --- /dev/null +++ b/src/test/compile-fail/borrowck-swap-mut-base-ptr.rs @@ -0,0 +1,16 @@ +// Test that attempt to swap `&mut` pointer while pointee is borrowed +// yields an error. +// +// Example from src/middle/borrowck/doc.rs + +use std::util::swap; + +fn foo<'a>(mut t0: &'a mut int, + mut t1: &'a mut int) { + let p: &int = &*t0; // Freezes `*t0` + swap(&mut t0, &mut t1); //~ ERROR cannot borrow `t0` + *t1 = 22; +} + +fn main() { +} \ No newline at end of file