From 2a1aa9fb5356dc43e649b86622bd12463722d2af Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Sun, 14 Oct 2012 13:39:17 -0700 Subject: [PATCH] Check whether loans conflict with old loans or with themselves. Along the way, convert from dvec-of-dvec representation to track loans in scope to just a single flattened list. It's more convenient. Fixes #3765. r+ pcwalton. --- src/rustc/middle/borrowck.rs | 9 +- src/rustc/middle/borrowck/check_loans.rs | 85 ++++++++++------- src/rustc/middle/borrowck/gather_loans.rs | 95 +++++++++++-------- src/rustc/middle/borrowck/loan.rs | 45 +++++---- ...borrowck-loan-local-as-both-mut-and-imm.rs | 25 +++++ 5 files changed, 165 insertions(+), 94 deletions(-) create mode 100644 src/test/compile-fail/borrowck-loan-local-as-both-mut-and-imm.rs diff --git a/src/rustc/middle/borrowck.rs b/src/rustc/middle/borrowck.rs index e2f7ba20642aa..02fd2998f4dba 100644 --- a/src/rustc/middle/borrowck.rs +++ b/src/rustc/middle/borrowck.rs @@ -383,7 +383,7 @@ impl bckerr : cmp::Eq { type bckres = Result; /// a complete record of a loan that was granted -type loan = {lp: @loan_path, cmt: cmt, mutbl: ast::mutability}; +struct Loan {lp: @loan_path, cmt: cmt, mutbl: ast::mutability} /// maps computed by `gather_loans` that are then used by `check_loans` /// @@ -392,7 +392,7 @@ type loan = {lp: @loan_path, cmt: cmt, mutbl: ast::mutability}; /// - `pure_map`: map from block/expr that must be pure to the error message /// that should be reported if they are not pure type req_maps = { - req_loan_map: HashMap>>, + req_loan_map: HashMap>, pure_map: HashMap }; @@ -582,6 +582,11 @@ impl borrowck_ctxt { method_map: self.method_map}; mc.mut_to_str(mutbl) } + + fn loan_to_repr(loan: &Loan) -> ~str { + fmt!("Loan(lp=%?, cmt=%s, mutbl=%?)", + loan.lp, self.cmt_to_repr(loan.cmt), loan.mutbl) + } } // The inherent mutability of a component is its default mutability diff --git a/src/rustc/middle/borrowck/check_loans.rs b/src/rustc/middle/borrowck/check_loans.rs index 6a9195b45096b..7f95d44fd3b85 100644 --- a/src/rustc/middle/borrowck/check_loans.rs +++ b/src/rustc/middle/borrowck/check_loans.rs @@ -131,18 +131,15 @@ impl check_loan_ctxt { } } - fn walk_loans(scope_id: ast::node_id, - f: fn(v: &loan) -> bool) { + fn walk_loans(scope_id: ast::node_id, f: fn(v: &Loan) -> bool) { let mut scope_id = scope_id; let region_map = self.tcx().region_map; let req_loan_map = self.req_maps.req_loan_map; loop { - for req_loan_map.find(scope_id).each |loanss| { - for loanss.each |loans| { - for loans.each |loan| { - if !f(loan) { return; } - } + for req_loan_map.find(scope_id).each |loans| { + for loans.each |loan| { + if !f(loan) { return; } } } @@ -155,7 +152,7 @@ impl check_loan_ctxt { fn walk_loans_of(scope_id: ast::node_id, lp: @loan_path, - f: fn(v: &loan) -> bool) { + f: fn(v: &Loan) -> bool) { for self.walk_loans(scope_id) |loan| { if loan.lp == lp { if !f(loan) { return; } @@ -256,36 +253,58 @@ impl check_loan_ctxt { } fn check_for_conflicting_loans(scope_id: ast::node_id) { - let new_loanss = match self.req_maps.req_loan_map.find(scope_id) { + debug!("check_for_conflicting_loans(scope_id=%?)", scope_id); + + let new_loans = match self.req_maps.req_loan_map.find(scope_id) { None => return, - Some(loanss) => loanss + Some(loans) => loans }; + debug!("new_loans has length %?", new_loans.len()); + let par_scope_id = self.tcx().region_map.get(scope_id); for self.walk_loans(par_scope_id) |old_loan| { - for new_loanss.each |new_loans| { - for new_loans.each |new_loan| { - if old_loan.lp != new_loan.lp { loop; } - match (old_loan.mutbl, new_loan.mutbl) { - (m_const, _) | (_, m_const) | - (m_mutbl, m_mutbl) | (m_imm, m_imm) => { - /*ok*/ - } - - (m_mutbl, m_imm) | (m_imm, m_mutbl) => { - self.bccx.span_err( - new_loan.cmt.span, - fmt!("loan of %s as %s \ - conflicts with prior loan", - self.bccx.cmt_to_str(new_loan.cmt), - self.bccx.mut_to_str(new_loan.mutbl))); - self.bccx.span_note( - old_loan.cmt.span, - fmt!("prior loan as %s granted here", - self.bccx.mut_to_str(old_loan.mutbl))); - } - } - } + debug!("old_loan=%?", self.bccx.loan_to_repr(old_loan)); + + for new_loans.each |new_loan| { + self.report_error_if_loans_conflict(old_loan, new_loan); + } + } + + let len = new_loans.len(); + for uint::range(0, len) |i| { + let loan_i = new_loans[i]; + for uint::range(i+1, len) |j| { + let loan_j = new_loans[j]; + self.report_error_if_loans_conflict(&loan_i, &loan_j); + } + } + } + + fn report_error_if_loans_conflict(&self, + old_loan: &Loan, + new_loan: &Loan) { + if old_loan.lp != new_loan.lp { + return; + } + + match (old_loan.mutbl, new_loan.mutbl) { + (m_const, _) | (_, m_const) | + (m_mutbl, m_mutbl) | (m_imm, m_imm) => { + /*ok*/ + } + + (m_mutbl, m_imm) | (m_imm, m_mutbl) => { + self.bccx.span_err( + new_loan.cmt.span, + fmt!("loan of %s as %s \ + conflicts with prior loan", + self.bccx.cmt_to_str(new_loan.cmt), + self.bccx.mut_to_str(new_loan.mutbl))); + self.bccx.span_note( + old_loan.cmt.span, + fmt!("prior loan as %s granted here", + self.bccx.mut_to_str(old_loan.mutbl))); } } } diff --git a/src/rustc/middle/borrowck/gather_loans.rs b/src/rustc/middle/borrowck/gather_loans.rs index a2c8f18507138..e8d11fd1708f9 100644 --- a/src/rustc/middle/borrowck/gather_loans.rs +++ b/src/rustc/middle/borrowck/gather_loans.rs @@ -213,9 +213,10 @@ fn req_loans_in_expr(ex: @ast::expr, } impl gather_loan_ctxt { - fn tcx() -> ty::ctxt { self.bccx.tcx } + fn tcx(&self) -> ty::ctxt { self.bccx.tcx } - fn guarantee_adjustments(expr: @ast::expr, + fn guarantee_adjustments(&self, + expr: @ast::expr, adjustment: &ty::AutoAdjustment) { debug!("guarantee_adjustments(expr=%s, adjustment=%?)", expr_repr(self.tcx(), expr), adjustment); @@ -256,7 +257,8 @@ impl gather_loan_ctxt { // out loans, which will be added to the `req_loan_map`. This can // also entail "rooting" GC'd pointers, which means ensuring // dynamically that they are not freed. - fn guarantee_valid(cmt: cmt, + fn guarantee_valid(&self, + cmt: cmt, req_mutbl: ast::mutability, scope_r: ty::region) { @@ -280,35 +282,12 @@ impl gather_loan_ctxt { // it within that scope, the loan will be detected and an // error will be reported. Some(_) => { - match self.bccx.loan(cmt, scope_r, req_mutbl) { - Err(e) => { self.bccx.report(e); } - Ok(loans) if loans.len() == 0 => {} - Ok(loans) => { - match scope_r { - ty::re_scope(scope_id) => { - self.add_loans(scope_id, loans); - - if req_mutbl == m_imm && cmt.mutbl != m_imm { - self.bccx.loaned_paths_imm += 1; - - if self.tcx().sess.borrowck_note_loan() { - self.bccx.span_note( - cmt.span, - fmt!("immutable loan required")); - } - } else { - self.bccx.loaned_paths_same += 1; - } + match self.bccx.loan(cmt, scope_r, req_mutbl) { + Err(e) => { self.bccx.report(e); } + Ok(move loans) => { + self.add_loans(cmt, req_mutbl, scope_r, move loans); } - _ => { - self.bccx.tcx.sess.span_bug( - cmt.span, - fmt!("loans required but scope is scope_region is %s", - region_to_str(self.tcx(), scope_r))); - } - } } - } } // The path is not loanable: in that case, we must try and @@ -385,7 +364,8 @@ impl gather_loan_ctxt { // has type `@mut{f:int}`, this check might fail because `&x.f` // reqires an immutable pointer, but `f` lives in (aliased) // mutable memory. - fn check_mutbl(req_mutbl: ast::mutability, + fn check_mutbl(&self, + req_mutbl: ast::mutability, cmt: cmt) -> bckres { debug!("check_mutbl(req_mutbl=%?, cmt.mutbl=%?)", req_mutbl, cmt.mutbl); @@ -407,21 +387,58 @@ impl gather_loan_ctxt { } } - fn add_loans(scope_id: ast::node_id, loans: @DVec) { + fn add_loans(&self, + cmt: cmt, + req_mutbl: ast::mutability, + scope_r: ty::region, + +loans: ~[Loan]) { + if loans.len() == 0 { + return; + } + + let scope_id = match scope_r { + ty::re_scope(scope_id) => scope_id, + _ => { + self.bccx.tcx.sess.span_bug( + cmt.span, + fmt!("loans required but scope is scope_region is %s", + region_to_str(self.tcx(), scope_r))); + } + }; + + self.add_loans_to_scope_id(scope_id, move loans); + + if req_mutbl == m_imm && cmt.mutbl != m_imm { + self.bccx.loaned_paths_imm += 1; + + if self.tcx().sess.borrowck_note_loan() { + self.bccx.span_note( + cmt.span, + fmt!("immutable loan required")); + } + } else { + self.bccx.loaned_paths_same += 1; + } + } + + fn add_loans_to_scope_id(&self, scope_id: ast::node_id, +loans: ~[Loan]) { debug!("adding %u loans to scope_id %?", loans.len(), scope_id); match self.req_maps.req_loan_map.find(scope_id) { - Some(l) => { - l.push(loans); + Some(req_loans) => { + req_loans.push_all(loans); } None => { - self.req_maps.req_loan_map.insert( - scope_id, @dvec::from_vec(~[loans])); + let dvec = @dvec::from_vec(move loans); + self.req_maps.req_loan_map.insert(scope_id, dvec); } } } - fn gather_pat(discr_cmt: cmt, root_pat: @ast::pat, - arm_id: ast::node_id, alt_id: ast::node_id) { + fn gather_pat(&self, + discr_cmt: cmt, + root_pat: @ast::pat, + arm_id: ast::node_id, + alt_id: ast::node_id) { do self.bccx.cat_pattern(discr_cmt, root_pat) |cmt, pat| { match pat.node { ast::pat_ident(bm, _, _) if !self.pat_is_variant(pat) => { @@ -475,7 +492,7 @@ impl gather_loan_ctxt { } } - fn pat_is_variant(pat: @ast::pat) -> bool { + fn pat_is_variant(&self, pat: @ast::pat) -> bool { pat_util::pat_is_variant(self.bccx.tcx.def_map, pat) } } diff --git a/src/rustc/middle/borrowck/loan.rs b/src/rustc/middle/borrowck/loan.rs index 8d9d7a5796a9a..5d3ccc392139e 100644 --- a/src/rustc/middle/borrowck/loan.rs +++ b/src/rustc/middle/borrowck/loan.rs @@ -8,35 +8,37 @@ use result::{Result, Ok, Err}; impl borrowck_ctxt { fn loan(cmt: cmt, scope_region: ty::region, - mutbl: ast::mutability) -> bckres<@DVec> { - let lc = loan_ctxt_(@{bccx: self, - scope_region: scope_region, - loans: @DVec()}); + mutbl: ast::mutability) -> bckres<~[Loan]> { + let lc = LoanContext { + bccx: self, + scope_region: scope_region, + loans: ~[] + }; match lc.loan(cmt, mutbl) { - Ok(()) => {Ok(lc.loans)} - Err(e) => {Err(e)} + Err(e) => Err(e), + Ok(()) => { + let LoanContext {loans, _} = move lc; + Ok(loans) + } } } } -type loan_ctxt_ = { +struct LoanContext { bccx: borrowck_ctxt, // the region scope for which we must preserve the memory scope_region: ty::region, // accumulated list of loans that will be required - loans: @DVec -}; - -enum loan_ctxt { - loan_ctxt_(@loan_ctxt_) + mut loans: ~[Loan] } -impl loan_ctxt { - fn tcx() -> ty::ctxt { self.bccx.tcx } +impl LoanContext { + fn tcx(&self) -> ty::ctxt { self.bccx.tcx } - fn issue_loan(cmt: cmt, + fn issue_loan(&self, + cmt: cmt, scope_ub: ty::region, req_mutbl: ast::mutability) -> bckres<()> { if self.bccx.is_subregion_of(self.scope_region, scope_ub) { @@ -57,12 +59,13 @@ impl loan_ctxt { } } - (*self.loans).push({ + self.loans.push(Loan { // Note: cmt.lp must be Some(_) because otherwise this // loan process does not apply at all. lp: cmt.lp.get(), cmt: cmt, - mutbl: req_mutbl}); + mutbl: req_mutbl + }); return Ok(()); } else { // The loan being requested lives longer than the data @@ -73,7 +76,7 @@ impl loan_ctxt { } } - fn loan(cmt: cmt, req_mutbl: ast::mutability) -> bckres<()> { + fn loan(&self, cmt: cmt, req_mutbl: ast::mutability) -> bckres<()> { debug!("loan(%s, %s)", self.bccx.cmt_to_repr(cmt), self.bccx.mut_to_str(req_mutbl)); @@ -144,7 +147,8 @@ impl loan_ctxt { // A "stable component" is one where assigning the base of the // component cannot cause the component itself to change types. // Example: record fields. - fn loan_stable_comp(cmt: cmt, + fn loan_stable_comp(&self, + cmt: cmt, cmt_base: cmt, req_mutbl: ast::mutability) -> bckres<()> { let base_mutbl = match req_mutbl { @@ -162,7 +166,8 @@ impl loan_ctxt { // An "unstable deref" means a deref of a ptr/comp where, if the // base of the deref is assigned to, pointers into the result of the // deref would be invalidated. Examples: interior of variants, uniques. - fn loan_unstable_deref(cmt: cmt, + fn loan_unstable_deref(&self, + cmt: cmt, cmt_base: cmt, req_mutbl: ast::mutability) -> bckres<()> { // Variant components: the base must be immutable, because diff --git a/src/test/compile-fail/borrowck-loan-local-as-both-mut-and-imm.rs b/src/test/compile-fail/borrowck-loan-local-as-both-mut-and-imm.rs new file mode 100644 index 0000000000000..54048ed2fd8cb --- /dev/null +++ b/src/test/compile-fail/borrowck-loan-local-as-both-mut-and-imm.rs @@ -0,0 +1,25 @@ +use core::either::{Either, Left, Right}; + + fn f(x: &mut Either, y: &Either) -> int { + match *y { + Left(ref z) => { + *x = Right(1.0); + *z + } + _ => fail + } + } + + fn g() { + let mut x: Either = Left(3); + io::println(f(&mut x, &x).to_str()); //~ ERROR conflicts with prior loan + } + + fn h() { + let mut x: Either = Left(3); + let y: &Either = &x; + let z: &mut Either = &mut x; //~ ERROR conflicts with prior loan + *z = *y; + } + + fn main() {}