From cfac9b6833be05a476b5694f855089b48c53697b Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 1 Jun 2012 15:46:32 -0700 Subject: [PATCH] improve borrowck to handle some frankly rather tricky cases - receivers of method calls are also borrowed - by-val arguments are also borrowed (needs tests) - assignment to components can interfere with loans --- src/rustc/middle/borrowck.rs | 24 +++++-- src/rustc/middle/borrowck/categorization.rs | 33 ++++++---- src/rustc/middle/borrowck/check_loans.rs | 63 +++++++++++++------ src/rustc/middle/borrowck/gather_loans.rs | 56 ++++++++++++++++- src/rustc/middle/borrowck/loan.rs | 4 +- src/rustc/middle/borrowck/preserve.rs | 4 +- src/rustc/util/ppaux.rs | 8 ++- .../compile-fail/borrowck-assign-comp-idx.rs | 38 +++++++++++ src/test/compile-fail/borrowck-assign-comp.rs | 47 ++++++++++++++ src/test/compile-fail/borrowck-loan-rcvr.rs | 46 ++++++++++++++ 10 files changed, 278 insertions(+), 45 deletions(-) create mode 100644 src/test/compile-fail/borrowck-assign-comp-idx.rs create mode 100644 src/test/compile-fail/borrowck-assign-comp.rs create mode 100644 src/test/compile-fail/borrowck-loan-rcvr.rs diff --git a/src/rustc/middle/borrowck.rs b/src/rustc/middle/borrowck.rs index babee32aeb8..261ee3ea802 100644 --- a/src/rustc/middle/borrowck.rs +++ b/src/rustc/middle/borrowck.rs @@ -149,7 +149,7 @@ Borrowck results in two maps. "]; import syntax::ast; -import syntax::ast::{m_mutbl, m_imm, m_const}; +import syntax::ast::{mutability, m_mutbl, m_imm, m_const}; import syntax::visit; import syntax::ast_util; import syntax::ast_map; @@ -254,7 +254,8 @@ enum ptr_kind {uniq_ptr, gc_ptr, region_ptr, unsafe_ptr} // I am coining the term "components" to mean "pieces of a data // structure accessible without a dereference": enum comp_kind {comp_tuple, comp_res, comp_variant, - comp_field(str), comp_index(ty::t)} + comp_field(str, ast::mutability), + comp_index(ty::t, ast::mutability)} // We pun on *T to mean both actual deref of a ptr as well // as accessing of components: @@ -422,8 +423,8 @@ impl to_str_methods for borrowck_ctxt { fn comp_to_repr(comp: comp_kind) -> str { alt comp { - comp_field(fld) { fld } - comp_index(_) { "[]" } + comp_field(fld, _) { fld } + comp_index(*) { "[]" } comp_tuple { "()" } comp_res { "" } comp_variant { "" } @@ -480,11 +481,11 @@ impl to_str_methods for borrowck_ctxt { cat_deref(_, _, pk) { #fmt["dereference of %s %s pointer", mut_str, self.pk_to_sigil(pk)] } cat_stack_upvar(_) { mut_str + " upvar" } - cat_comp(_, comp_field(_)) { mut_str + " field" } + cat_comp(_, comp_field(*)) { mut_str + " field" } cat_comp(_, comp_tuple) { "tuple content" } cat_comp(_, comp_res) { "resource content" } cat_comp(_, comp_variant) { "enum content" } - cat_comp(_, comp_index(t)) { + cat_comp(_, comp_index(t, _)) { alt ty::get(t).struct { ty::ty_vec(*) | ty::ty_evec(*) { mut_str + " vec content" @@ -522,3 +523,14 @@ impl to_str_methods for borrowck_ctxt { } } } + +// The inherent mutability of a component is its default mutability +// assuming it is embedded in an immutable context. In general, the +// mutability can be "overridden" if the component is embedded in a +// mutable structure. +fn inherent_mutability(ck: comp_kind) -> mutability { + alt ck { + comp_tuple | comp_res | comp_variant {m_imm} + comp_field(_, m) | comp_index(_, m) {m} + } +} \ No newline at end of file diff --git a/src/rustc/middle/borrowck/categorization.rs b/src/rustc/middle/borrowck/categorization.rs index 325460bddbe..31763bb11c2 100644 --- a/src/rustc/middle/borrowck/categorization.rs +++ b/src/rustc/middle/borrowck/categorization.rs @@ -30,43 +30,53 @@ then an index to jump forward to the relevant item. "]; export public_methods; +export opt_deref_kind; // Categorizes a derefable type. Note that we include vectors and strings as // derefable (we model an index as the combination of a deref and then a // pointer adjustment). -fn deref_kind(tcx: ty::ctxt, t: ty::t) -> deref_kind { +fn opt_deref_kind(t: ty::t) -> option { alt ty::get(t).struct { ty::ty_uniq(*) | ty::ty_vec(*) | ty::ty_str | ty::ty_evec(_, ty::vstore_uniq) | ty::ty_estr(ty::vstore_uniq) { - deref_ptr(uniq_ptr) + some(deref_ptr(uniq_ptr)) } ty::ty_rptr(*) | ty::ty_evec(_, ty::vstore_slice(_)) | ty::ty_estr(ty::vstore_slice(_)) { - deref_ptr(region_ptr) + some(deref_ptr(region_ptr)) } ty::ty_box(*) | ty::ty_evec(_, ty::vstore_box) | ty::ty_estr(ty::vstore_box) { - deref_ptr(gc_ptr) + some(deref_ptr(gc_ptr)) } ty::ty_ptr(*) { - deref_ptr(unsafe_ptr) + some(deref_ptr(unsafe_ptr)) } ty::ty_enum(*) { - deref_comp(comp_variant) + some(deref_comp(comp_variant)) } ty::ty_res(*) { - deref_comp(comp_res) + some(deref_comp(comp_res)) } _ { + none + } + } +} + +fn deref_kind(tcx: ty::ctxt, t: ty::t) -> deref_kind { + alt opt_deref_kind(t) { + some(k) {k} + none { tcx.sess.bug( #fmt["deref_cat() invoked on non-derefable type %s", ty_to_str(tcx, t)]); @@ -281,11 +291,12 @@ impl public_methods for borrowck_ctxt { m_imm { base_cmt.mutbl } // imm: as mutable as the container m_mutbl | m_const { f_mutbl } }; + let f_comp = comp_field(f_name, f_mutbl); let lp = base_cmt.lp.map { |lp| - @lp_comp(lp, comp_field(f_name)) + @lp_comp(lp, f_comp) }; @{id: node.id(), span: node.span(), - cat: cat_comp(base_cmt, comp_field(f_name)), lp:lp, + cat: cat_comp(base_cmt, f_comp), lp:lp, mutbl: m, ty: self.tcx.ty(node)} } @@ -347,8 +358,8 @@ impl public_methods for borrowck_ctxt { let deref_lp = base_cmt.lp.map { |lp| @lp_deref(lp, ptr) }; let deref_cmt = @{id:expr.id, span:expr.span, cat:cat_deref(base_cmt, 0u, ptr), lp:deref_lp, - mutbl:mt.mutbl, ty:mt.ty}; - let comp = comp_index(base_cmt.ty); + mutbl:m_imm, ty:mt.ty}; + let comp = comp_index(base_cmt.ty, mt.mutbl); let index_lp = deref_lp.map { |lp| @lp_comp(lp, comp) }; @{id:expr.id, span:expr.span, cat:cat_comp(deref_cmt, comp), lp:index_lp, diff --git a/src/rustc/middle/borrowck/check_loans.rs b/src/rustc/middle/borrowck/check_loans.rs index a989a294401..414f7f3dac7 100644 --- a/src/rustc/middle/borrowck/check_loans.rs +++ b/src/rustc/middle/borrowck/check_loans.rs @@ -262,7 +262,7 @@ impl methods for check_loan_ctxt { fn is_self_field(cmt: cmt) -> bool { alt cmt.cat { - cat_comp(cmt_base, comp_field(_)) { + cat_comp(cmt_base, comp_field(*)) { alt cmt_base.cat { cat_special(sk_self) { true } _ { false } @@ -314,31 +314,54 @@ impl methods for check_loan_ctxt { // which will be checked for compat separately in // check_for_conflicting_loans() if at != at_mutbl_ref { - let lp = alt cmt.lp { - none { ret; } - some(lp) { lp } - }; - for self.walk_loans_of(ex.id, lp) { |loan| - alt loan.mutbl { - m_mutbl | m_const { /*ok*/ } - m_imm { - self.bccx.span_err( - ex.span, - #fmt["%s prohibited due to outstanding loan", - at.ing_form(self.bccx.cmt_to_str(cmt))]); - self.bccx.span_note( - loan.cmt.span, - #fmt["loan of %s granted here", - self.bccx.cmt_to_str(loan.cmt)]); - ret; - } - } + for cmt.lp.each { |lp| + self.check_for_loan_conflicting_with_assignment( + at, ex, cmt, lp); } } self.bccx.add_to_mutbl_map(cmt); } + fn check_for_loan_conflicting_with_assignment( + at: assignment_type, + ex: @ast::expr, + cmt: cmt, + lp: @loan_path) { + + for self.walk_loans_of(ex.id, lp) { |loan| + alt loan.mutbl { + m_mutbl | m_const { /*ok*/ } + m_imm { + self.bccx.span_err( + ex.span, + #fmt["%s prohibited due to outstanding loan", + at.ing_form(self.bccx.cmt_to_str(cmt))]); + self.bccx.span_note( + loan.cmt.span, + #fmt["loan of %s granted here", + self.bccx.cmt_to_str(loan.cmt)]); + ret; + } + } + } + + // Subtle: if the mutability of the component being assigned + // is inherited from the thing that the component is embedded + // within, then we have to check whether that thing has been + // loaned out as immutable! An example: + // let mut x = {f: some(3)}; + // let y = &x; // x loaned out as immutable + // x.f = none; // changes type of y.f, which appears to be imm + alt *lp { + lp_comp(lp_base, ck) if inherent_mutability(ck) != m_mutbl { + self.check_for_loan_conflicting_with_assignment( + at, ex, cmt, lp_base); + } + lp_comp(*) | lp_local(*) | lp_arg(*) | lp_deref(*) {} + } + } + fn report_purity_error(pc: purity_cause, sp: span, msg: str) { alt pc { pc_pure_fn { diff --git a/src/rustc/middle/borrowck/gather_loans.rs b/src/rustc/middle/borrowck/gather_loans.rs index 04ea50bee03..da6dbe460ed 100644 --- a/src/rustc/middle/borrowck/gather_loans.rs +++ b/src/rustc/middle/borrowck/gather_loans.rs @@ -6,7 +6,7 @@ // their associated scopes. In phase two, checking loans, we will then make // sure that all of these loans are honored. -import categorization::public_methods; +import categorization::{public_methods, opt_deref_kind}; import loan::public_methods; import preserve::public_methods; @@ -30,6 +30,8 @@ fn req_loans_in_expr(ex: @ast::expr, let bccx = self.bccx; let tcx = bccx.tcx; + #debug["req_loans_in_expr(ex=%s)", pprust::expr_to_str(ex)]; + // If this expression is borrowed, have to ensure it remains valid: for tcx.borrowings.find(ex.id).each { |borrow| let cmt = self.bccx.cat_borrow_of_expr(ex); @@ -64,7 +66,39 @@ fn req_loans_in_expr(ex: @ast::expr, let arg_cmt = self.bccx.cat_expr(arg); self.guarantee_valid(arg_cmt, m_imm, scope_r); } - ast::by_move | ast::by_copy | ast::by_val {} + ast::by_val { + // Rust's by-val does not actually give ownership to + // the callee. This means that if a pointer type is + // passed, it is effectively a borrow, and so the + // caller must guarantee that the data remains valid. + // + // Subtle: we only guarantee that the pointer is valid + // and const. Technically, we ought to pass in the + // mutability that the caller expects (e.g., if the + // formal argument has type @mut, we should guarantee + // validity and mutability, not validity and const). + // However, the type system already guarantees that + // the caller's mutability is compatible with the + // callee, so this is not necessary. (Note that with + // actual borrows, typeck is more liberal and allows + // the pointer to be borrowed as immutable even if it + // is mutable in the caller's frame, thus effectively + // passing the buck onto us to enforce this) + + alt opt_deref_kind(arg_ty.ty) { + some(deref_ptr(region_ptr)) { + /* region pointers are (by induction) guaranteed */ + } + none { + /* not a pointer, no worries */ + } + some(_) { + let arg_cmt = self.bccx.cat_borrow_of_expr(arg); + self.guarantee_valid(arg_cmt, m_const, scope_r); + } + } + } + ast::by_move | ast::by_copy {} } } } @@ -78,6 +112,24 @@ fn req_loans_in_expr(ex: @ast::expr, } } + ast::expr_field(rcvr, _, _) | + ast::expr_binary(_, rcvr, _) | + ast::expr_unary(_, rcvr) if self.bccx.method_map.contains_key(ex.id) { + // Receivers in method calls are always passed by ref. + // + // FIXME--this scope is both too large and too small. We make + // the scope the enclosing block, which surely includes any + // immediate call (a.b()) but which is too big. OTOH, in the + // case of a naked field `a.b`, the value is copied + // anyhow. This is probably best fixed if we address the + // syntactic ambiguity. + + // let scope_r = ty::re_scope(ex.id); + let scope_r = ty::re_scope(self.tcx().region_map.get(ex.id)); + let rcvr_cmt = self.bccx.cat_expr(rcvr); + self.guarantee_valid(rcvr_cmt, m_imm, scope_r); + } + _ { /*ok*/ } } diff --git a/src/rustc/middle/borrowck/loan.rs b/src/rustc/middle/borrowck/loan.rs index 26682661aad..e54ac388f0f 100644 --- a/src/rustc/middle/borrowck/loan.rs +++ b/src/rustc/middle/borrowck/loan.rs @@ -55,8 +55,8 @@ impl loan_methods for loan_ctxt { cat_discr(base, _) { self.loan(base, req_mutbl) } - cat_comp(cmt_base, comp_field(_)) | - cat_comp(cmt_base, comp_index(_)) | + cat_comp(cmt_base, comp_field(*)) | + cat_comp(cmt_base, comp_index(*)) | cat_comp(cmt_base, comp_tuple) | cat_comp(cmt_base, comp_res) { // For most components, the type of the embedded data is diff --git a/src/rustc/middle/borrowck/preserve.rs b/src/rustc/middle/borrowck/preserve.rs index d79c9b480e5..250d1daed12 100644 --- a/src/rustc/middle/borrowck/preserve.rs +++ b/src/rustc/middle/borrowck/preserve.rs @@ -29,8 +29,8 @@ impl public_methods for borrowck_ctxt { // This is basically a deref of a region ptr. ok(()) } - cat_comp(cmt_base, comp_field(_)) | - cat_comp(cmt_base, comp_index(_)) | + cat_comp(cmt_base, comp_field(*)) | + cat_comp(cmt_base, comp_index(*)) | cat_comp(cmt_base, comp_tuple) | cat_comp(cmt_base, comp_res) { // Most embedded components: if the base is stable, the diff --git a/src/rustc/util/ppaux.rs b/src/rustc/util/ppaux.rs index 47cdd8cf153..664bd268039 100644 --- a/src/rustc/util/ppaux.rs +++ b/src/rustc/util/ppaux.rs @@ -27,14 +27,18 @@ fn re_scope_id_to_str(cx: ctxt, node_id: ast::node_id) -> str { } ast_map::node_expr(expr) { alt expr.node { - ast::expr_call(_, _, _) { + ast::expr_call(*) { #fmt("", codemap::span_to_str(expr.span, cx.sess.codemap)) } - ast::expr_alt(_, _, _) { + ast::expr_alt(*) { #fmt("", codemap::span_to_str(expr.span, cx.sess.codemap)) } + ast::expr_field(*) | ast::expr_unary(*) | ast::expr_binary(*) { + #fmt("", + codemap::span_to_str(expr.span, cx.sess.codemap)) + } _ { cx.sess.bug( #fmt["re_scope refers to %s", ast_map::node_id_to_str(cx.items, node_id)]) } diff --git a/src/test/compile-fail/borrowck-assign-comp-idx.rs b/src/test/compile-fail/borrowck-assign-comp-idx.rs new file mode 100644 index 00000000000..c418c855237 --- /dev/null +++ b/src/test/compile-fail/borrowck-assign-comp-idx.rs @@ -0,0 +1,38 @@ +// xfail-fast (compile-flags unsupported on windows) +// compile-flags:--borrowck=err + +type point = { x: int, y: int }; + +fn a() { + let mut p = [mut 1]; + + // Create an immutable pointer into p's contents: + let _q: &int = &p[0]; //! NOTE loan of mutable vec content granted here + + p[0] = 5; //! ERROR assigning to mutable vec content prohibited due to outstanding loan +} + +fn borrow(_x: [int]/&, _f: fn()) {} + +fn b() { + // here we alias the mutable vector into an imm slice and try to + // modify the original: + + let mut p = [mut 1]; + + borrow(p) {|| //! NOTE loan of mutable vec content granted here + p[0] = 5; //! ERROR assigning to mutable vec content prohibited due to outstanding loan + } +} + +fn c() { + // Legal because the scope of the borrow does not include the + // modification: + let mut p = [mut 1]; + borrow(p, {||}); + p[0] = 5; +} + +fn main() { +} + diff --git a/src/test/compile-fail/borrowck-assign-comp.rs b/src/test/compile-fail/borrowck-assign-comp.rs new file mode 100644 index 00000000000..c8830957c76 --- /dev/null +++ b/src/test/compile-fail/borrowck-assign-comp.rs @@ -0,0 +1,47 @@ +// xfail-fast (compile-flags unsupported on windows) +// compile-flags:--borrowck=err + +type point = { x: int, y: int }; + +fn a() { + let mut p = {x: 3, y: 4}; + let _q = &p; //! NOTE loan of mutable local variable granted here + + // This assignment is illegal because the field x is not + // inherently mutable; since `p` was made immutable, `p.x` is now + // immutable. Otherwise the type of &_q.x (&int) would be wrong. + p.x = 5; //! ERROR assigning to mutable field prohibited due to outstanding loan +} + +fn b() { + let mut p = {x: 3, mut y: 4}; + let _q = &p; + + // This assignment is legal because `y` is inherently mutable (and + // hence &_q.y is &mut int). + p.y = 5; +} + +fn c() { + // this is sort of the opposite. We take a loan to the interior of `p` + // and then try to overwrite `p` as a whole. + + let mut p = {x: 3, mut y: 4}; + let _q = &p.y; //! NOTE loan of mutable local variable granted here + p = {x: 5, mut y: 7};//! ERROR assigning to mutable local variable prohibited due to outstanding loan + copy p; +} + +fn d() { + // just for completeness's sake, the easy case, where we take the + // address of a subcomponent and then modify that subcomponent: + + let mut p = {x: 3, mut y: 4}; + let _q = &p.y; //! NOTE loan of mutable field granted here + p.y = 5; //! ERROR assigning to mutable field prohibited due to outstanding loan + copy p; +} + +fn main() { +} + diff --git a/src/test/compile-fail/borrowck-loan-rcvr.rs b/src/test/compile-fail/borrowck-loan-rcvr.rs new file mode 100644 index 00000000000..87dd933aa7f --- /dev/null +++ b/src/test/compile-fail/borrowck-loan-rcvr.rs @@ -0,0 +1,46 @@ +// xfail-fast (compile-flags unsupported on windows) +// compile-flags:--borrowck=err + +type point = { x: int, y: int }; + +impl foo for point { + fn impurem() { + } + + pure fn purem() { + } +} + +fn a() { + let mut p = {x: 3, y: 4}; + p.purem(); + p.impurem(); +} + +fn a2() { + let mut p = {x: 3, y: 4}; + p.purem(); + p.impurem(); + p.x = p.y; +} + +fn b() { + let mut p = {x: 3, y: 4}; + + &mut p; //! NOTE prior loan as mutable granted here + //!^ NOTE prior loan as mutable granted here + + p.purem(); //! ERROR loan of mutable local variable as immutable conflicts with prior loan + p.impurem(); //! ERROR loan of mutable local variable as immutable conflicts with prior loan +} + +fn c() { + let q = @mut {x: 3, y: 4}; + (*q).purem(); + (*q).impurem(); //! ERROR illegal borrow unless pure: creating immutable alias to aliasable, mutable memory + //!^ NOTE impure due to access to impure function +} + +fn main() { +} +