From 1c15e9efeb2f816f2615042bd9e8125ebf48bcfa Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 22 Jul 2014 05:43:19 -0400 Subject: [PATCH] Fix regionck to account for the uniqueness requirements on ref mut reborrows --- src/librustc/middle/typeck/check/regionck.rs | 268 ++++++++++++------ .../borrowck-closures-mut-of-imm.rs | 3 +- ...owck-reborrow-from-shorter-lived-andmut.rs | 2 +- .../regions-reborrow-from-shorter-mut-ref.rs | 2 +- .../regions-infer-reborrow-ref-mut-recurse.rs | 24 ++ 5 files changed, 211 insertions(+), 88 deletions(-) create mode 100644 src/test/run-pass/regions-infer-reborrow-ref-mut-recurse.rs diff --git a/src/librustc/middle/typeck/check/regionck.rs b/src/librustc/middle/typeck/check/regionck.rs index e7676301946d..f54e650a1734 100644 --- a/src/librustc/middle/typeck/check/regionck.rs +++ b/src/librustc/middle/typeck/check/regionck.rs @@ -1381,8 +1381,8 @@ fn link_by_ref(rcx: &Rcx, expr.repr(tcx), callee_scope); let mc = mc::MemCategorizationContext::new(rcx); let expr_cmt = ignore_err!(mc.cat_expr(expr)); - let region_min = ty::ReScope(callee_scope); - link_region(rcx, expr.span, region_min, ty::ImmBorrow, expr_cmt); + let borrow_region = ty::ReScope(callee_scope); + link_region(rcx, expr.span, borrow_region, ty::ImmBorrow, expr_cmt); } fn link_region_from_node_type(rcx: &Rcx, @@ -1408,102 +1408,54 @@ fn link_region_from_node_type(rcx: &Rcx, fn link_region(rcx: &Rcx, span: Span, - region_min: ty::Region, - kind: ty::BorrowKind, - cmt_borrowed: mc::cmt) { + borrow_region: ty::Region, + borrow_kind: ty::BorrowKind, + borrow_cmt: mc::cmt) { /*! - * Informs the inference engine that a borrow of `cmt` - * must have the borrow kind `kind` and lifetime `region_min`. - * If `cmt` is a deref of a region pointer with - * lifetime `r_borrowed`, this will add the constraint that - * `region_min <= r_borrowed`. + * Informs the inference engine that `borrow_cmt` is being + * borrowed with kind `borrow_kind` and lifetime `borrow_region`. + * In order to ensure borrowck is satisfied, this may create + * constraints between regions, as explained in + * `link_reborrowed_region()`. */ - // Iterate through all the things that must be live at least - // for the lifetime `region_min` for the borrow to be valid: - let mut cmt_borrowed = cmt_borrowed; + let mut borrow_cmt = borrow_cmt; + let mut borrow_kind = borrow_kind; + loop { - debug!("link_region(region_min={}, kind={}, cmt_borrowed={})", - region_min.repr(rcx.tcx()), - kind.repr(rcx.tcx()), - cmt_borrowed.repr(rcx.tcx())); - match cmt_borrowed.cat.clone() { - mc::cat_deref(base, _, mc::BorrowedPtr(_, r_borrowed)) | - mc::cat_deref(base, _, mc::Implicit(_, r_borrowed)) => { - // References to an upvar `x` are translated to - // `*x`, since that is what happens in the - // underlying machine. We detect such references - // and treat them slightly differently, both to - // offer better error messages and because we need - // to infer the kind of borrow (mut, const, etc) - // to use for each upvar. - let cause = match base.cat { - mc::cat_upvar(ref upvar_id, _) => { - match rcx.fcx.inh.upvar_borrow_map.borrow_mut() - .find_mut(upvar_id) { - Some(upvar_borrow) => { - debug!("link_region: {} <= {}", - region_min.repr(rcx.tcx()), - upvar_borrow.region.repr(rcx.tcx())); - adjust_upvar_borrow_kind_for_loan( - *upvar_id, - upvar_borrow, - kind); - infer::ReborrowUpvar(span, *upvar_id) - } - None => { - rcx.tcx().sess.span_bug( - span, - format!("Illegal upvar id: {}", - upvar_id.repr( - rcx.tcx())).as_slice()); - } - } + debug!("link_region(borrow_region={}, borrow_kind={}, borrow_cmt={})", + borrow_region.repr(rcx.tcx()), + borrow_kind.repr(rcx.tcx()), + borrow_cmt.repr(rcx.tcx())); + match borrow_cmt.cat.clone() { + mc::cat_deref(ref_cmt, _, + mc::Implicit(ref_kind, ref_region)) | + mc::cat_deref(ref_cmt, _, + mc::BorrowedPtr(ref_kind, ref_region)) => { + match link_reborrowed_region(rcx, span, + borrow_region, borrow_kind, + ref_cmt, ref_region, ref_kind) { + Some((c, k)) => { + borrow_cmt = c; + borrow_kind = k; } - - _ => { - infer::Reborrow(span) + None => { + return; } - }; - - debug!("link_region: {} <= {}", - region_min.repr(rcx.tcx()), - r_borrowed.repr(rcx.tcx())); - rcx.fcx.mk_subr(cause, region_min, r_borrowed); - - if kind != ty::ImmBorrow { - // If this is a mutable borrow, then the thing - // being borrowed will have to be unique. - // In user code, this means it must be an `&mut` - // borrow, but for an upvar, we might opt - // for an immutable-unique borrow. - adjust_upvar_borrow_kind_for_unique(rcx, base); } - - // Borrowing an `&mut` pointee for `region_min` is - // only valid if the pointer resides in a unique - // location which is itself valid for - // `region_min`. We don't care about the unique - // part, but we may need to influence the - // inference to ensure that the location remains - // valid. - // - // FIXME(#8624) fixing borrowck will require this - // if m == ast::m_mutbl { - // cmt_borrowed = cmt_base; - // } else { - // return; - // } - return; } + mc::cat_discr(cmt_base, _) | mc::cat_downcast(cmt_base) | mc::cat_deref(cmt_base, _, mc::GcPtr(..)) | mc::cat_deref(cmt_base, _, mc::OwnedPtr) | mc::cat_interior(cmt_base, _) => { - // Interior or owned data requires its base to be valid - cmt_borrowed = cmt_base; + // Borrowing interior or owned data requires the base + // to be valid and borrowable in the same fashion. + borrow_cmt = cmt_base; + borrow_kind = borrow_kind; } + mc::cat_deref(_, _, mc::UnsafePtr(..)) | mc::cat_static_item | mc::cat_copied_upvar(..) | @@ -1519,6 +1471,154 @@ fn link_region(rcx: &Rcx, } } +fn link_reborrowed_region(rcx: &Rcx, + span: Span, + borrow_region: ty::Region, + borrow_kind: ty::BorrowKind, + ref_cmt: mc::cmt, + ref_region: ty::Region, + ref_kind: ty::BorrowKind) + -> Option<(mc::cmt, ty::BorrowKind)> +{ + /*! + * This is the most complicated case: the path being borrowed is + * itself the referent of a borrowed pointer. Let me give an + * example fragment of code to make clear(er) the situation: + * + * let r: &'a mut T = ...; // the original reference "r" has lifetime 'a + * ... + * &'z *r // the reborrow has lifetime 'z + * + * Now, in this case, our primary job is to add the inference + * constraint that `'z <= 'a`. Given this setup, let's clarify the + * parameters in (roughly) terms of the example: + * + * A borrow of: `& 'z bk * r` where `r` has type `& 'a bk T` + * borrow_region ^~ ref_region ^~ + * borrow_kind ^~ ref_kind ^~ + * ref_cmt ^ + * + * Here `bk` stands for some borrow-kind (e.g., `mut`, `uniq`, etc). + * + * Unfortunately, there are some complications beyond the simple + * scenario I just painted: + * + * 1. The reference `r` might in fact be a "by-ref" upvar. In that + * case, we have two jobs. First, we are inferring whether this reference + * should be an `&T`, `&mut T`, or `&uniq T` reference, and we must + * adjust that based on this borrow (e.g., if this is an `&mut` borrow, + * then `r` must be an `&mut` reference). Second, whenever we link + * two regions (here, `'z <= 'a`), we supply a *cause*, and in this + * case we adjust the cause to indicate that the reference being + * "reborrowed" is itself an upvar. This provides a nicer error message + * should something go wrong. + * + * 2. There may in fact be more levels of reborrowing. In the + * example, I said the borrow was like `&'z *r`, but it might + * in fact be a borrow like `&'z **q` where `q` has type `&'a + * &'b mut T`. In that case, we want to ensure that `'z <= 'a` + * and `'z <= 'b`. This is explained more below. + * + * The return value of this function indicates whether we need to + * recurse and process `ref_cmt` (see case 2 above). + */ + + // Detect references to an upvar `x`: + let cause = match ref_cmt.cat { + mc::cat_upvar(ref upvar_id, _) => { + let mut upvar_borrow_map = + rcx.fcx.inh.upvar_borrow_map.borrow_mut(); + match upvar_borrow_map.find_mut(upvar_id) { + Some(upvar_borrow) => { + // Adjust mutability that we infer for the upvar + // so it can accommodate being borrowed with + // mutability `kind`: + adjust_upvar_borrow_kind_for_loan(*upvar_id, + upvar_borrow, + borrow_kind); + + infer::ReborrowUpvar(span, *upvar_id) + } + None => { + rcx.tcx().sess.span_bug( + span, + format!("Illegal upvar id: {}", + upvar_id.repr( + rcx.tcx())).as_slice()); + } + } + } + + _ => { + infer::Reborrow(span) + } + }; + + debug!("link_reborrowed_region: {} <= {}", + borrow_region.repr(rcx.tcx()), + ref_region.repr(rcx.tcx())); + rcx.fcx.mk_subr(cause, borrow_region, ref_region); + + // Decide whether we need to recurse and link any regions within + // the `ref_cmt`. This is concerned for the case where the value + // being reborrowed is in fact a borrowed pointer found within + // another borrowed pointer. For example: + // + // let p: &'b &'a mut T = ...; + // ... + // &'z **p + // + // What makes this case particularly tricky is that, if the data + // being borrowed is a `&mut` or `&uniq` borrow, borrowck requires + // not only that `'z <= 'a`, (as before) but also `'z <= 'b` + // (otherwise the user might mutate through the `&mut T` reference + // after `'b` expires and invalidate the borrow we are looking at + // now). + // + // So let's re-examine our parameters in light of this more + // complicated (possible) scenario: + // + // A borrow of: `& 'z bk * * p` where `p` has type `&'b bk & 'a bk T` + // borrow_region ^~ ref_region ^~ + // borrow_kind ^~ ref_kind ^~ + // ref_cmt ^~~ + // + // (Note that since we have not examined `ref_cmt.cat`, we don't + // know whether this scenario has occurred; but I wanted to show + // how all the types get adjusted.) + match ref_kind { + ty::ImmBorrow => { + // The reference being reborrowed is a sharable ref of + // type `&'a T`. In this case, it doesn't matter where we + // *found* the `&T` pointer, the memory it references will + // be valid and immutable for `'a`. So we can stop here. + // + // (Note that the `borrow_kind` must also be ImmBorrow or + // else the user is borrowed imm memory as mut memory, + // which means they'll get an error downstream in borrowck + // anyhow.) + return None; + } + + ty::MutBorrow | ty::UniqueImmBorrow => { + // The reference being reborrowed is either an `&mut T` or + // `&uniq T`. This is the case where recursion is needed. + // + // One interesting twist is that we can weaken the borrow + // kind when we recurse: to reborrow an `&mut` referent as + // mutable, borrowck requires a unique path to the `&mut` + // reference but not necessarily a *mutable* path. + let new_borrow_kind = match borrow_kind { + ty::ImmBorrow => + ty::ImmBorrow, + ty::MutBorrow | ty::UniqueImmBorrow => + ty::UniqueImmBorrow + }; + return Some((ref_cmt, new_borrow_kind)); + } + } +} + fn adjust_borrow_kind_for_assignment_lhs(rcx: &Rcx, lhs: &ast::Expr) { /*! diff --git a/src/test/compile-fail/borrowck-closures-mut-of-imm.rs b/src/test/compile-fail/borrowck-closures-mut-of-imm.rs index cdfb569762de..6360a9135005 100644 --- a/src/test/compile-fail/borrowck-closures-mut-of-imm.rs +++ b/src/test/compile-fail/borrowck-closures-mut-of-imm.rs @@ -23,8 +23,7 @@ fn a(x: &int) { let c1 = || set(&mut *x); //~^ ERROR cannot borrow let c2 = || set(&mut *x); - //~^ ERROR closure requires unique access to `x` - //~^^ ERROR cannot borrow + //~^ ERROR cannot borrow } fn main() { diff --git a/src/test/compile-fail/borrowck-reborrow-from-shorter-lived-andmut.rs b/src/test/compile-fail/borrowck-reborrow-from-shorter-lived-andmut.rs index 0e1c4758c1be..05cadfd53656 100644 --- a/src/test/compile-fail/borrowck-reborrow-from-shorter-lived-andmut.rs +++ b/src/test/compile-fail/borrowck-reborrow-from-shorter-lived-andmut.rs @@ -17,7 +17,7 @@ struct S<'a> { fn copy_borrowed_ptr<'a,'b>(p: &'a mut S<'b>) -> S<'b> { S { pointer: &mut *p.pointer } - //~^ ERROR lifetime of `p` is too short to guarantee its contents can be safely reborrowed + //~^ ERROR cannot infer } 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 index 385bc11d1a98..4b1c7a2928b8 100644 --- a/src/test/compile-fail/regions-reborrow-from-shorter-mut-ref.rs +++ b/src/test/compile-fail/regions-reborrow-from-shorter-mut-ref.rs @@ -13,7 +13,7 @@ // 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 + &mut **p //~ ERROR cannot infer } fn main() { diff --git a/src/test/run-pass/regions-infer-reborrow-ref-mut-recurse.rs b/src/test/run-pass/regions-infer-reborrow-ref-mut-recurse.rs new file mode 100644 index 000000000000..efe3994dbb7b --- /dev/null +++ b/src/test/run-pass/regions-infer-reborrow-ref-mut-recurse.rs @@ -0,0 +1,24 @@ +// Copyright 2014 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. + +// Test an edge case in region inference: the lifetime of the borrow +// of `*x` must be extended to at least 'a. + +fn foo<'a,'b>(x: &'a &'b mut int) -> &'a int { + let y = &*x; // should be inferred to have type &'a &'b mut int... + + // ...because if we inferred, say, &'x &'b mut int where 'x <= 'a, + // this reborrow would be illegal: + &**y +} + +pub fn main() { + /* Just want to know that it compiles. */ +}