diff --git a/src/libextra/dlist.rs b/src/libextra/dlist.rs index 22109c98dc8..3507201b839 100644 --- a/src/libextra/dlist.rs +++ b/src/libextra/dlist.rs @@ -225,8 +225,9 @@ impl Deque for DList { /// Provide a mutable reference to the back element, or None if the list is empty #[inline] fn back_mut<'a>(&'a mut self) -> Option<&'a mut T> { - let mut tmp = self.list_tail.resolve(); // FIXME: #3511: shouldn't need variable - tmp.as_mut().map(|tail| &mut tail.value) + let tmp: Option<&'a mut Node> = + self.list_tail.resolve(); // FIXME: #3511: shouldn't need variable + tmp.map(|tail| &mut tail.value) } /// Add an element first in the list diff --git a/src/libextra/ringbuf.rs b/src/libextra/ringbuf.rs index f6bf6c21335..5580ca32bf9 100644 --- a/src/libextra/ringbuf.rs +++ b/src/libextra/ringbuf.rs @@ -198,7 +198,28 @@ impl RingBuf { /// Front-to-back iterator which returns mutable values. pub fn mut_iter<'a>(&'a mut self) -> RingBufMutIterator<'a, T> { - RingBufMutIterator{index: 0, rindex: self.nelts, lo: self.lo, elts: self.elts} + let start_index = raw_index(self.lo, self.elts.len(), 0); + let end_index = raw_index(self.lo, self.elts.len(), self.nelts); + + // Divide up the array + if end_index <= start_index { + // Items to iterate goes from: + // start_index to self.elts.len() + // and then + // 0 to end_index + let (temp, remaining1) = self.elts.mut_split(start_index); + let (remaining2, _) = temp.mut_split(end_index); + RingBufMutIterator { remaining1: remaining1, + remaining2: remaining2, + nelts: self.nelts } + } else { + // Items to iterate goes from start_index to end_index: + let (empty, elts) = self.elts.mut_split(0); + let remaining1 = elts.mut_slice(start_index, end_index); + RingBufMutIterator { remaining1: remaining1, + remaining2: empty, + nelts: self.nelts } + } } /// Back-to-front iterator which returns mutable values. @@ -207,45 +228,6 @@ impl RingBuf { } } -macro_rules! iterator { - (impl $name:ident -> $elem:ty, $getter:ident) => { - impl<'self, T> Iterator<$elem> for $name<'self, T> { - #[inline] - fn next(&mut self) -> Option<$elem> { - if self.index == self.rindex { - return None; - } - let raw_index = raw_index(self.lo, self.elts.len(), self.index); - self.index += 1; - Some(self.elts[raw_index] . $getter ()) - } - - #[inline] - fn size_hint(&self) -> (uint, Option) { - let len = self.rindex - self.index; - (len, Some(len)) - } - } - } -} - -macro_rules! iterator_rev { - (impl $name:ident -> $elem:ty, $getter:ident) => { - impl<'self, T> DoubleEndedIterator<$elem> for $name<'self, T> { - #[inline] - fn next_back(&mut self) -> Option<$elem> { - if self.index == self.rindex { - return None; - } - self.rindex -= 1; - let raw_index = raw_index(self.lo, self.elts.len(), self.rindex); - Some(self.elts[raw_index] . $getter ()) - } - } - } -} - - /// RingBuf iterator pub struct RingBufIterator<'self, T> { priv lo: uint, @@ -253,8 +235,36 @@ pub struct RingBufIterator<'self, T> { priv rindex: uint, priv elts: &'self [Option], } -iterator!{impl RingBufIterator -> &'self T, get_ref} -iterator_rev!{impl RingBufIterator -> &'self T, get_ref} + +impl<'self, T> Iterator<&'self T> for RingBufIterator<'self, T> { + #[inline] + fn next(&mut self) -> Option<&'self T> { + if self.index == self.rindex { + return None; + } + let raw_index = raw_index(self.lo, self.elts.len(), self.index); + self.index += 1; + Some(self.elts[raw_index].get_ref()) + } + + #[inline] + fn size_hint(&self) -> (uint, Option) { + let len = self.rindex - self.index; + (len, Some(len)) + } +} + +impl<'self, T> DoubleEndedIterator<&'self T> for RingBufIterator<'self, T> { + #[inline] + fn next_back(&mut self) -> Option<&'self T> { + if self.index == self.rindex { + return None; + } + self.rindex -= 1; + let raw_index = raw_index(self.lo, self.elts.len(), self.rindex); + Some(self.elts[raw_index].get_ref()) + } +} impl<'self, T> ExactSize<&'self T> for RingBufIterator<'self, T> {} @@ -275,13 +285,49 @@ impl<'self, T> RandomAccessIterator<&'self T> for RingBufIterator<'self, T> { /// RingBuf mutable iterator pub struct RingBufMutIterator<'self, T> { - priv lo: uint, - priv index: uint, - priv rindex: uint, - priv elts: &'self mut [Option], + priv remaining1: &'self mut [Option], + priv remaining2: &'self mut [Option], + priv nelts: uint, +} + +impl<'self, T> Iterator<&'self mut T> for RingBufMutIterator<'self, T> { + #[inline] + fn next(&mut self) -> Option<&'self mut T> { + if self.nelts == 0 { + return None; + } + let r = if self.remaining1.len() > 0 { + &mut self.remaining1 + } else { + assert!(self.remaining2.len() > 0); + &mut self.remaining2 + }; + self.nelts -= 1; + Some(r.mut_shift_ref().get_mut_ref()) + } + + #[inline] + fn size_hint(&self) -> (uint, Option) { + (self.nelts, Some(self.nelts)) + } +} + +impl<'self, T> DoubleEndedIterator<&'self mut T> for RingBufMutIterator<'self, T> { + #[inline] + fn next_back(&mut self) -> Option<&'self mut T> { + if self.nelts == 0 { + return None; + } + let r = if self.remaining2.len() > 0 { + &mut self.remaining2 + } else { + assert!(self.remaining1.len() > 0); + &mut self.remaining1 + }; + self.nelts -= 1; + Some(r.mut_pop_ref().get_mut_ref()) + } } -iterator!{impl RingBufMutIterator -> &'self mut T, get_mut_ref} -iterator_rev!{impl RingBufMutIterator -> &'self mut T, get_mut_ref} impl<'self, T> ExactSize<&'self mut T> for RingBufMutIterator<'self, T> {} @@ -667,6 +713,21 @@ mod tests { assert_eq!(d.rev_iter().collect::<~[&int]>(), ~[&4,&3,&2,&1,&0,&6,&7,&8]); } + #[test] + fn test_mut_rev_iter_wrap() { + let mut d = RingBuf::with_capacity(3); + assert!(d.mut_rev_iter().next().is_none()); + + d.push_back(1); + d.push_back(2); + d.push_back(3); + assert_eq!(d.pop_front(), Some(1)); + d.push_back(4); + + assert_eq!(d.mut_rev_iter().map(|x| *x).collect::<~[int]>(), + ~[4, 3, 2]); + } + #[test] fn test_mut_iter() { let mut d = RingBuf::new(); diff --git a/src/librustc/middle/borrowck/doc.rs b/src/librustc/middle/borrowck/doc.rs index 8bb5c4620ef..7cc1395a9e5 100644 --- a/src/librustc/middle/borrowck/doc.rs +++ b/src/librustc/middle/borrowck/doc.rs @@ -233,7 +233,7 @@ the lifetime of the value being borrowed. This pass is also responsible for inserting root annotations to keep managed values alive and for dynamically freezing `@mut` boxes. -3. `RESTRICTIONS(LV, ACTIONS) = RS`: This pass checks and computes the +3. `RESTRICTIONS(LV, LT, ACTIONS) = RS`: This pass checks and computes the restrictions to maintain memory safety. These are the restrictions that will go into the final loan. We'll discuss in more detail below. @@ -451,7 +451,7 @@ the scope `LT`. The final rules govern the computation of *restrictions*, meaning that we compute the set of actions that will be illegal for the life of the -loan. The predicate is written `RESTRICTIONS(LV, ACTIONS) = +loan. The predicate is written `RESTRICTIONS(LV, LT, ACTIONS) = RESTRICTION*`, which can be read "in order to prevent `ACTIONS` from occuring on `LV`, the restrictions `RESTRICTION*` must be respected for the lifetime of the loan". @@ -459,9 +459,9 @@ for the lifetime of the loan". Note that there is an initial set of restrictions: these restrictions are computed based on the kind of borrow: - &mut LV => RESTRICTIONS(LV, MUTATE|CLAIM|FREEZE) - &LV => RESTRICTIONS(LV, MUTATE|CLAIM) - &const LV => RESTRICTIONS(LV, []) + &mut LV => RESTRICTIONS(LV, LT, MUTATE|CLAIM|FREEZE) + &LV => RESTRICTIONS(LV, LT, MUTATE|CLAIM) + &const LV => RESTRICTIONS(LV, LT, []) The reasoning here is that a mutable borrow must be the only writer, therefore it prevents other writes (`MUTATE`), mutable borrows @@ -474,7 +474,7 @@ moved out from under it, so no actions are forbidden. The simplest case is a borrow of a local variable `X`: - RESTRICTIONS(X, ACTIONS) = (X, ACTIONS) // R-Variable + RESTRICTIONS(X, LT, ACTIONS) = (X, ACTIONS) // R-Variable In such cases we just record the actions that are not permitted. @@ -483,8 +483,8 @@ In such cases we just record the actions that are not permitted. Restricting a field is the same as restricting the owner of that field: - RESTRICTIONS(LV.f, ACTIONS) = RS, (LV.f, ACTIONS) // R-Field - RESTRICTIONS(LV, ACTIONS) = RS + RESTRICTIONS(LV.f, LT, ACTIONS) = RS, (LV.f, ACTIONS) // R-Field + RESTRICTIONS(LV, LT, ACTIONS) = RS The reasoning here is as follows. If the field must not be mutated, then you must not mutate the owner of the field either, since that @@ -504,9 +504,9 @@ must prevent the owned pointer `LV` from being mutated, which means that we always add `MUTATE` and `CLAIM` to the restriction set imposed on `LV`: - RESTRICTIONS(*LV, ACTIONS) = RS, (*LV, ACTIONS) // R-Deref-Send-Pointer + RESTRICTIONS(*LV, LT, ACTIONS) = RS, (*LV, ACTIONS) // R-Deref-Send-Pointer TYPE(LV) = ~Ty - RESTRICTIONS(LV, ACTIONS|MUTATE|CLAIM) = RS + RESTRICTIONS(LV, LT, ACTIONS|MUTATE|CLAIM) = RS ### Restrictions for loans of immutable managed/borrowed pointees @@ -519,7 +519,7 @@ restricting that path. Therefore, the rule for `&Ty` and `@Ty` pointers always returns an empty set of restrictions, and it only permits restricting `MUTATE` and `CLAIM` actions: - RESTRICTIONS(*LV, ACTIONS) = [] // R-Deref-Imm-Borrowed + RESTRICTIONS(*LV, LT, ACTIONS) = [] // R-Deref-Imm-Borrowed TYPE(LV) = &Ty or @Ty ACTIONS subset of [MUTATE, CLAIM] @@ -546,7 +546,7 @@ Because moves from a `&const` or `@const` lvalue are never legal, it is not necessary to add any restrictions at all to the final result. - RESTRICTIONS(*LV, []) = [] // R-Deref-Freeze-Borrowed + RESTRICTIONS(*LV, LT, []) = [] // R-Deref-Freeze-Borrowed TYPE(LV) = &const Ty or @const Ty ### Restrictions for loans of mutable borrowed pointees @@ -581,83 +581,149 @@ an `&mut` pointee from being mutated, claimed, or frozen, as occurs whenever the `&mut` pointee `*LV` is reborrowed as mutable or immutable: - RESTRICTIONS(*LV, ACTIONS) = RS, (*LV, ACTIONS) // R-Deref-Mut-Borrowed-1 - TYPE(LV) = &mut Ty - RESTRICTIONS(LV, MUTATE|CLAIM|ALIAS) = RS + RESTRICTIONS(*LV, LT, ACTIONS) = RS, (*LV, ACTIONS) // R-Deref-Mut-Borrowed-1 + TYPE(LV) = <' mut Ty + LT <= LT' // (1) + RESTRICTIONS(LV, LT, MUTATE|CLAIM|ALIAS) = RS // (2) -The main interesting part of the rule is the final line, which -requires that the `&mut` *pointer* `LV` be restricted from being -mutated, claimed, or aliased. The goal of these restrictions is to -ensure that, not considering the pointer that will result from this -borrow, `LV` remains the *sole pointer with mutable access* to `*LV`. +There are two interesting parts to this rule: -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. 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: +1. The lifetime of the loan (`LT`) cannot exceed the lifetime of the + `&mut` pointer (`LT'`). The reason for this is that the `&mut` + pointer is guaranteed to be the only legal way to mutate its + pointee -- but only for the lifetime `LT'`. After that lifetime, + the loan on the pointee expires and hence the data may be modified + by its owner again. This implies that we are only able to guarantee that + the pointee will not be modified or aliased for a maximum of `LT'`. - // 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; - } + Here is a concrete example of a bug this rule prevents: -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: + // Test region-reborrow-from-shorter-mut-ref.rs: + fn copy_pointer<'a,'b,T>(x: &'a mut &'b mut T) -> &'b mut T { + &mut **p // ERROR due to clause (1) + } + fn main() { + let mut x = 1; + let mut y = &mut x; // <-'b-----------------------------+ + // +-'a--------------------+ | + // v v | + let z = copy_borrowed_ptr(&mut y); // y is lent | + *y += 1; // Here y==z, so both should not be usable... | + *z += 1; // ...and yet they would be, but for clause 1. | + } <---------------------------------------------------------+ - // 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; - } +2. The final line recursively requires that the `&mut` *pointer* `LV` + be restricted from being mutated, claimed, or aliased (not just the + pointee). The goal of these restrictions is to ensure that, not + considering the pointer that will result from this borrow, `LV` + remains the *sole pointer with mutable access* to `*LV`. -The restriction against *aliasing* (and, in turn, freezing) is -necessary because, if an alias were of `LV` were to be produced, then -`LV` would no longer be the sole path to access the `&mut` -pointee. Since we are only issuing restrictions against `*LV`, these -other aliases would be unrestricted, and the result would be -unsound. For example: + Restrictions against 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. 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: + + // 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 claims 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 + necessary because, if an alias were of `LV` were to be produced, + then `LV` would no longer be the sole path to access the `&mut` + pointee. Since we are only issuing restrictions against `*LV`, + these other aliases would be unrestricted, and the result would be + unsound. For example: // 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; // (*) + **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: +The current rules could use some correction: - // 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; // (*) - } +1. Issue #10520. Now that the swap operator has been removed, I do not + believe the restriction against mutating `LV` is needed, and in + fact it prevents some useful patterns. For example, the following + function will fail to compile: -Here at the line `(*)` you will also see the error I referred to -above, which I do not believe is strictly necessary. + fn mut_shift_ref<'a,T>(x: &mut &'a mut [T]) -> &'a mut T { + // `mut_split` will restrict mutation against *x: + let (head, tail) = (*x).mut_split(1); + + // Hence mutating `*x` yields an error here: + *x = tail; + &mut head[0] + } + + Note that this function -- which adjusts the slice `*x` in place so + that it no longer contains the head element and then returns a + pointer to that element separately -- is perfectly valid. It is + currently implemented using unsafe code. I believe that now that + the swap operator is removed from the language, we could liberalize + the rules and make this function be accepted normally. The idea + would be to have the assignment to `*x` kill the loans of `*x` and + its subpaths -- after all, those subpaths are no longer accessible + through `*x`, since it has been overwritten with a new value. Thus + those subpaths are only accessible through prior existing borrows + of `*x`, if any. The danger of the *swap* operator was that it + allowed `*x` to be mutated without making the subpaths of `*x` + inaccessible: worse, they became accessible through a new path (I + suppose that we could support swap, too, if needed, by moving the + loans over to the new path). + + Note: the `swap()` function doesn't pose the same danger as the + swap operator because it requires taking `&mut` refs to invoke it. + +2. Issue #9629. The current rules correctly prohibit `&mut` pointees + from being assigned unless they are in a unique location. However, + we *also* prohibit `&mut` pointees from being frozen. This prevents + compositional patterns, like this one: + + struct BorrowedMap<'a> { + map: &'a mut HashMap + } + + If we have a pointer `x:&BorrowedMap`, we can't freeze `x.map`, + and hence can't call `find` etc on it. But that's silly, since + fact that the `&mut` exists in frozen data implies that it + will not be mutable by anyone. For example, this program nets an + error: + + fn main() { + let a = &mut 2; + let b = &a; + *a += 1; // ERROR: cannot assign to `*a` because it is borrowed + } + + (Naturally `&mut` reborrows from an `&&mut` pointee should be illegal.) The second rule for `&mut` handles the case where we are not adding any restrictions (beyond the default of "no move"): - RESTRICTIONS(*LV, []) = [] // R-Deref-Mut-Borrowed-2 + RESTRICTIONS(*LV, LT, []) = [] // R-Deref-Mut-Borrowed-2 TYPE(LV) = &mut Ty Moving from an `&mut` pointee is never legal, so no special -restrictions are needed. +restrictions are needed. This rule is used for `&const` borrows. ### Restrictions for loans of mutable managed pointees @@ -665,7 +731,7 @@ With `@mut` pointees, we don't make any static guarantees. But as a convenience, we still register a restriction against `*LV`, because that way if we *can* find a simple static error, we will: - RESTRICTIONS(*LV, ACTIONS) = [*LV, ACTIONS] // R-Deref-Managed-Borrowed + RESTRICTIONS(*LV, LT, ACTIONS) = [*LV, ACTIONS] // R-Deref-Managed-Borrowed TYPE(LV) = @mut Ty # Moves and initialization diff --git a/src/librustc/middle/borrowck/gather_loans/lifetime.rs b/src/librustc/middle/borrowck/gather_loans/lifetime.rs index a5f1709058c..8cfb47bd04d 100644 --- a/src/librustc/middle/borrowck/gather_loans/lifetime.rs +++ b/src/librustc/middle/borrowck/gather_loans/lifetime.rs @@ -8,9 +8,10 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -//! This module implements the check that the lifetime of a borrow -//! does not exceed the lifetime of the value being borrowed. - +/*! + * This module implements the check that the lifetime of a borrow + * does not exceed the lifetime of the value being borrowed. + */ use middle::borrowck::*; use mc = middle::mem_categorization; @@ -20,13 +21,15 @@ use syntax::ast; use syntax::codemap::Span; use util::ppaux::{note_and_explain_region}; +type R = Result<(),()>; + pub fn guarantee_lifetime(bccx: &BorrowckCtxt, item_scope_id: ast::NodeId, root_scope_id: ast::NodeId, span: Span, cmt: mc::cmt, loan_region: ty::Region, - loan_mutbl: LoanMutability) { + loan_mutbl: LoanMutability) -> R { debug!("guarantee_lifetime(cmt={}, loan_region={})", cmt.repr(bccx.tcx), loan_region.repr(bccx.tcx)); let ctxt = GuaranteeLifetimeContext {bccx: bccx, @@ -36,7 +39,7 @@ pub fn guarantee_lifetime(bccx: &BorrowckCtxt, loan_mutbl: loan_mutbl, cmt_original: cmt, root_scope_id: root_scope_id}; - ctxt.check(cmt, None); + ctxt.check(cmt, None) } /////////////////////////////////////////////////////////////////////////// @@ -63,7 +66,7 @@ impl<'self> GuaranteeLifetimeContext<'self> { self.bccx.tcx } - fn check(&self, cmt: mc::cmt, discr_scope: Option) { + fn check(&self, cmt: mc::cmt, discr_scope: Option) -> R { //! Main routine. Walks down `cmt` until we find the "guarantor". match cmt.cat { @@ -83,6 +86,7 @@ impl<'self> GuaranteeLifetimeContext<'self> { } mc::cat_static_item => { + Ok(()) } mc::cat_deref(base, derefs, mc::gc_ptr(ptr_mutbl)) => { @@ -99,10 +103,11 @@ impl<'self> GuaranteeLifetimeContext<'self> { if !omit_root { // L-Deref-Managed-Imm-Compiler-Root // L-Deref-Managed-Mut-Compiler-Root - self.check_root(cmt, base, derefs, ptr_mutbl, discr_scope); + self.check_root(cmt, base, derefs, ptr_mutbl, discr_scope) } else { debug!("omitting root, base={}, base_scope={:?}", base.repr(self.tcx()), base_scope); + Ok(()) } } @@ -120,7 +125,7 @@ impl<'self> GuaranteeLifetimeContext<'self> { // for one arm. Therefore, we insert a cat_discr(), // basically a special kind of category that says "if this // value must be dynamically rooted, root it for the scope - // `match_id`. + // `match_id`". // // As an example, consider this scenario: // @@ -188,7 +193,7 @@ impl<'self> GuaranteeLifetimeContext<'self> { cmt_base: mc::cmt, derefs: uint, ptr_mutbl: ast::Mutability, - discr_scope: Option) { + discr_scope: Option) -> R { debug!("check_root(cmt_deref={}, cmt_base={}, derefs={:?}, ptr_mutbl={:?}, \ discr_scope={:?})", cmt_deref.repr(self.tcx()), @@ -201,9 +206,8 @@ impl<'self> GuaranteeLifetimeContext<'self> { // that we can root the value, dynamically. let root_region = ty::ReScope(self.root_scope_id); if !self.bccx.is_subregion_of(self.loan_region, root_region) { - self.report_error( - err_out_of_root_scope(root_region, self.loan_region)); - return; + return Err(self.report_error( + err_out_of_root_scope(root_region, self.loan_region))); } // Extract the scope id that indicates how long the rooting is required @@ -278,13 +282,16 @@ impl<'self> GuaranteeLifetimeContext<'self> { self.bccx.root_map.insert(rm_key, root_info); debug!("root_key: {:?} root_info: {:?}", rm_key, root_info); + Ok(()) } - fn check_scope(&self, max_scope: ty::Region) { + fn check_scope(&self, max_scope: ty::Region) -> R { //! Reports an error if `loan_region` is larger than `valid_scope` if !self.bccx.is_subregion_of(self.loan_region, max_scope) { - self.report_error(err_out_of_scope(max_scope, self.loan_region)); + Err(self.report_error(err_out_of_scope(max_scope, self.loan_region))) + } else { + Ok(()) } } diff --git a/src/librustc/middle/borrowck/gather_loans/mod.rs b/src/librustc/middle/borrowck/gather_loans/mod.rs index 410cf658a98..44a5f6fe49d 100644 --- a/src/librustc/middle/borrowck/gather_loans/mod.rs +++ b/src/librustc/middle/borrowck/gather_loans/mod.rs @@ -447,17 +447,22 @@ impl<'self> GatherLoanCtxt<'self> { // Check that the lifetime of the borrow does not exceed // the lifetime of the data being borrowed. - lifetime::guarantee_lifetime(self.bccx, self.item_ub, root_ub, - borrow_span, cmt, loan_region, req_mutbl); + if lifetime::guarantee_lifetime(self.bccx, self.item_ub, root_ub, + borrow_span, cmt, loan_region, + req_mutbl).is_err() { + return; // reported an error, no sense in reporting more. + } // Check that we don't allow mutable borrows of non-mutable data. - check_mutability(self.bccx, borrow_span, cmt, req_mutbl); + if check_mutability(self.bccx, borrow_span, cmt, req_mutbl).is_err() { + return; // reported an error, no sense in reporting more. + } // Compute the restrictions that are required to enforce the // loan is safe. let restr = restrictions::compute_restrictions( self.bccx, borrow_span, - cmt, self.restriction_set(req_mutbl)); + cmt, loan_region, self.restriction_set(req_mutbl)); // Create the loan record (if needed). let loan = match restr { @@ -554,25 +559,29 @@ impl<'self> GatherLoanCtxt<'self> { fn check_mutability(bccx: &BorrowckCtxt, borrow_span: Span, cmt: mc::cmt, - req_mutbl: LoanMutability) { + req_mutbl: LoanMutability) -> Result<(),()> { //! Implements the M-* rules in doc.rs. match req_mutbl { ConstMutability => { // Data of any mutability can be lent as const. + Ok(()) } ImmutableMutability => { // both imm and mut data can be lent as imm; // for mutable data, this is a freeze + Ok(()) } MutableMutability => { // Only mutable data can be lent as mutable. if !cmt.mutbl.is_mutable() { - bccx.report(BckError {span: borrow_span, - cmt: cmt, - code: err_mutbl(req_mutbl)}); + Err(bccx.report(BckError {span: borrow_span, + cmt: cmt, + code: err_mutbl(req_mutbl)})) + } else { + Ok(()) } } } diff --git a/src/librustc/middle/borrowck/gather_loans/restrictions.rs b/src/librustc/middle/borrowck/gather_loans/restrictions.rs index 7dbcb90327f..d4fe23e57b4 100644 --- a/src/librustc/middle/borrowck/gather_loans/restrictions.rs +++ b/src/librustc/middle/borrowck/gather_loans/restrictions.rs @@ -8,8 +8,9 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -//! Computes the restrictions that result from a borrow. - +/*! + * Computes the restrictions that result from a borrow. + */ use std::vec; use middle::borrowck::*; @@ -26,11 +27,13 @@ pub enum RestrictionResult { pub fn compute_restrictions(bccx: &BorrowckCtxt, span: Span, cmt: mc::cmt, + loan_region: ty::Region, restr: RestrictionSet) -> RestrictionResult { let ctxt = RestrictionsContext { bccx: bccx, span: span, - cmt_original: cmt + cmt_original: cmt, + loan_region: loan_region, }; ctxt.restrict(cmt, restr) @@ -42,7 +45,8 @@ pub fn compute_restrictions(bccx: &BorrowckCtxt, struct RestrictionsContext<'self> { bccx: &'self BorrowckCtxt, span: Span, - cmt_original: mc::cmt + cmt_original: mc::cmt, + loan_region: ty::Region, } impl<'self> RestrictionsContext<'self> { @@ -169,12 +173,22 @@ impl<'self> RestrictionsContext<'self> { } } - mc::cat_deref(cmt_base, _, pk @ mc::region_ptr(MutMutable, _)) => { + mc::cat_deref(cmt_base, _, pk @ mc::region_ptr(MutMutable, lt)) => { // Because an `&mut` pointer does not inherit its // 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 != RESTR_EMPTY { + if !self.bccx.is_subregion_of(self.loan_region, lt) { + self.bccx.report( + BckError { + span: self.span, + cmt: cmt_base, + code: err_mut_pointer_too_short( + self.loan_region, lt, restrictions)}); + return Safe; + } + // R-Deref-Mut-Borrowed-1 let result = self.restrict( cmt_base, diff --git a/src/librustc/middle/borrowck/mod.rs b/src/librustc/middle/borrowck/mod.rs index abeaef05431..51002889b2f 100644 --- a/src/librustc/middle/borrowck/mod.rs +++ b/src/librustc/middle/borrowck/mod.rs @@ -443,7 +443,8 @@ pub enum bckerr_code { err_mutbl(LoanMutability), err_out_of_root_scope(ty::Region, ty::Region), // superscope, subscope err_out_of_scope(ty::Region, ty::Region), // superscope, subscope - err_freeze_aliasable_const + err_freeze_aliasable_const, + err_mut_pointer_too_short(ty::Region, ty::Region, RestrictionSet), // loan, ptr } // Combination of an error code and the categorization of the expression @@ -669,6 +670,22 @@ impl BorrowckCtxt { // supposed to be going away. format!("unsafe borrow of aliasable, const value") } + err_mut_pointer_too_short(_, _, r) => { + let descr = match opt_loan_path(err.cmt) { + Some(lp) => format!("`{}`", self.loan_path_to_str(lp)), + None => ~"`&mut` pointer" + }; + + let tag = if r.intersects(RESTR_ALIAS) { + "its contents are unique" + } else { + "its contents are not otherwise mutable" + }; + + format!("lifetime of {} is too short to guarantee {} \ + so they can be safely reborrowed", + descr, tag) + } } } @@ -742,7 +759,24 @@ impl BorrowckCtxt { "...but borrowed value is only valid for ", super_scope, ""); - } + } + + err_mut_pointer_too_short(loan_scope, ptr_scope, _) => { + let descr = match opt_loan_path(err.cmt) { + Some(lp) => format!("`{}`", self.loan_path_to_str(lp)), + None => ~"`&mut` pointer" + }; + note_and_explain_region( + self.tcx, + format!("{} would have to be valid for ", descr), + loan_scope, + "..."); + note_and_explain_region( + self.tcx, + format!("...but {} is only valid for ", descr), + ptr_scope, + ""); + } } } diff --git a/src/libstd/vec.rs b/src/libstd/vec.rs index 1d562d64849..e76251e8a1e 100644 --- a/src/libstd/vec.rs +++ b/src/libstd/vec.rs @@ -975,6 +975,40 @@ pub trait ImmutableVector<'self, T> { * foreign interop. */ fn as_imm_buf(&self, f: |*T, uint| -> U) -> U; + + /** + * Returns a mutable reference to the first element in this slice + * and adjusts the slice in place so that it no longer contains + * that element. O(1). + * + * Equivalent to: + * + * ``` + * let head = &self[0]; + * *self = self.slice_from(1); + * head + * ``` + * + * Fails if slice is empty. + */ + fn shift_ref(&mut self) -> &'self T; + + /** + * Returns a mutable reference to the last element in this slice + * and adjusts the slice in place so that it no longer contains + * that element. O(1). + * + * Equivalent to: + * + * ``` + * let tail = &self[self.len() - 1]; + * *self = self.slice_to(self.len() - 1); + * tail + * ``` + * + * Fails if slice is empty. + */ + fn pop_ref(&mut self) -> &'self T; } impl<'self,T> ImmutableVector<'self, T> for &'self [T] { @@ -1146,6 +1180,20 @@ impl<'self,T> ImmutableVector<'self, T> for &'self [T] { let s = self.repr(); f(s.data, s.len) } + + fn shift_ref(&mut self) -> &'self T { + unsafe { + let s: &mut Slice = cast::transmute(self); + &*raw::shift_ptr(s) + } + } + + fn pop_ref(&mut self) -> &'self T { + unsafe { + let s: &mut Slice = cast::transmute(self); + &*raw::pop_ptr(s) + } + } } /// Extension methods for vectors contain `Eq` elements. @@ -1864,23 +1912,61 @@ impl OwnedEqVector for ~[T] { pub trait MutableVector<'self, T> { /// Return a slice that points into another slice. fn mut_slice(self, start: uint, end: uint) -> &'self mut [T]; + /** * Returns a slice of self from `start` to the end of the vec. * * Fails when `start` points outside the bounds of self. */ fn mut_slice_from(self, start: uint) -> &'self mut [T]; + /** * Returns a slice of self from the start of the vec to `end`. * * Fails when `end` points outside the bounds of self. */ fn mut_slice_to(self, end: uint) -> &'self mut [T]; + /// Returns an iterator that allows modifying each value fn mut_iter(self) -> VecMutIterator<'self, T>; + /// Returns a reversed iterator that allows modifying each value fn mut_rev_iter(self) -> MutRevIterator<'self, T>; + /** + * Returns a mutable reference to the first element in this slice + * and adjusts the slice in place so that it no longer contains + * that element. O(1). + * + * Equivalent to: + * + * ``` + * let head = &mut self[0]; + * *self = self.mut_slice_from(1); + * head + * ``` + * + * Fails if slice is empty. + */ + fn mut_shift_ref(&mut self) -> &'self mut T; + + /** + * Returns a mutable reference to the last element in this slice + * and adjusts the slice in place so that it no longer contains + * that element. O(1). + * + * Equivalent to: + * + * ``` + * let tail = &mut self[self.len() - 1]; + * *self = self.mut_slice_to(self.len() - 1); + * tail + * ``` + * + * Fails if slice is empty. + */ + fn mut_pop_ref(&mut self) -> &'self mut T; + /** * Swaps two elements in a vector * @@ -1983,6 +2069,20 @@ impl<'self,T> MutableVector<'self, T> for &'self mut [T] { self.mut_iter().invert() } + fn mut_shift_ref(&mut self) -> &'self mut T { + unsafe { + let s: &mut Slice = cast::transmute(self); + cast::transmute_mut(&*raw::shift_ptr(s)) + } + } + + fn mut_pop_ref(&mut self) -> &'self mut T { + unsafe { + let s: &mut Slice = cast::transmute(self); + cast::transmute_mut(&*raw::pop_ptr(s)) + } + } + fn swap(self, a: uint, b: uint) { unsafe { // Can't take two mutable loans from one vector, so instead just cast @@ -2194,6 +2294,31 @@ pub mod raw { }) }) } + + /** + * Returns a pointer to first element in slice and adjusts + * slice so it no longer contains that element. Fails if + * slice is empty. O(1). + */ + pub unsafe fn shift_ptr(slice: &mut Slice) -> *T { + if slice.len == 0 { fail!("shift on empty slice"); } + let head: *T = slice.data; + slice.data = ptr::offset(slice.data, 1); + slice.len -= 1; + head + } + + /** + * Returns a pointer to last element in slice and adjusts + * slice so it no longer contains that element. Fails if + * slice is empty. O(1). + */ + pub unsafe fn pop_ptr(slice: &mut Slice) -> *T { + if slice.len == 0 { fail!("pop on empty slice"); } + let tail: *T = ptr::offset(slice.data, (slice.len - 1) as int); + slice.len -= 1; + tail + } } /// Operations on `[u8]` @@ -3827,6 +3952,75 @@ mod tests { assert!(!empty.ends_with(bytes!("foo"))); assert!(bytes!("foobar").ends_with(empty)); } + + #[test] + fn test_shift_ref() { + let mut x: &[int] = [1, 2, 3, 4, 5]; + let h = x.shift_ref(); + assert_eq!(*h, 1); + assert_eq!(x.len(), 4); + assert_eq!(x[0], 2); + assert_eq!(x[3], 5); + } + + #[test] + #[should_fail] + fn test_shift_ref_empty() { + let mut x: &[int] = []; + x.shift_ref(); + } + + #[test] + fn test_pop_ref() { + let mut x: &[int] = [1, 2, 3, 4, 5]; + let h = x.pop_ref(); + assert_eq!(*h, 5); + assert_eq!(x.len(), 4); + assert_eq!(x[0], 1); + assert_eq!(x[3], 4); + } + + #[test] + #[should_fail] + fn test_pop_ref_empty() { + let mut x: &[int] = []; + x.pop_ref(); + } + + + #[test] + fn test_mut_shift_ref() { + let mut x: &mut [int] = [1, 2, 3, 4, 5]; + let h = x.mut_shift_ref(); + assert_eq!(*h, 1); + assert_eq!(x.len(), 4); + assert_eq!(x[0], 2); + assert_eq!(x[3], 5); + } + + #[test] + #[should_fail] + fn test_mut_shift_ref_empty() { + let mut x: &mut [int] = []; + x.mut_shift_ref(); + } + + #[test] + fn test_mut_pop_ref() { + let mut x: &mut [int] = [1, 2, 3, 4, 5]; + let h = x.mut_pop_ref(); + assert_eq!(*h, 5); + assert_eq!(x.len(), 4); + assert_eq!(x[0], 1); + assert_eq!(x[3], 4); + } + + #[test] + #[should_fail] + fn test_mut_pop_ref_empty() { + let mut x: &mut [int] = []; + x.mut_pop_ref(); + } } #[cfg(test)] diff --git a/src/test/compile-fail/regions-reborrow-from-shorter-mut-ref-mut-ref.rs b/src/test/compile-fail/regions-reborrow-from-shorter-mut-ref-mut-ref.rs new file mode 100644 index 00000000000..d3e0740a0fe --- /dev/null +++ b/src/test/compile-fail/regions-reborrow-from-shorter-mut-ref-mut-ref.rs @@ -0,0 +1,18 @@ +// Copyright 2012 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Issue #8624. Test for reborrowing with 3 levels, not just two. + +fn copy_borrowed_ptr<'a, 'b, 'c>(p: &'a mut &'b mut &'c mut int) -> &'b mut int { + &mut ***p //~ ERROR cannot infer an appropriate lifetime +} + +fn main() { +} diff --git a/src/test/compile-fail/regions-reborrow-from-shorter-mut-ref.rs b/src/test/compile-fail/regions-reborrow-from-shorter-mut-ref.rs new file mode 100644 index 00000000000..385bc11d1a9 --- /dev/null +++ b/src/test/compile-fail/regions-reborrow-from-shorter-mut-ref.rs @@ -0,0 +1,25 @@ +// Copyright 2012 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Issue #8624. Tests that reborrowing the contents of an `&'b mut` +// pointer which is backed by another `&'a mut` can only be done +// for `'a` (which must be a sublifetime of `'b`). + +fn copy_borrowed_ptr<'a, 'b>(p: &'a mut &'b mut int) -> &'b mut int { + &mut **p //~ ERROR lifetime of `p` is too short +} + +fn main() { + let mut x = 1; + let mut y = &mut x; + let z = copy_borrowed_ptr(&mut y); + *y += 1; + *z += 1; +}