From ea549e7a71964625f1eb24d7432f222c407d9999 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 17 Aug 2012 14:09:20 -0700 Subject: [PATCH] make borrowck more conservative around rvalues. this will require more temporaries, but is probably less magical. also, it means that borrowck matches trans better, so fewer crashes. bonus. Finally, stop warning about implicit copies when we are actually borrowing. Also, one test (vec-res-add) stopped failing due to #2587, and hence I added an xfail-test. Fixes #3217, #2977, #3067 --- src/libsyntax/ast_map.rs | 13 +++- src/rustc/middle/borrowck/preserve.rs | 3 +- src/rustc/middle/kind.rs | 13 +++- src/rustc/middle/region.rs | 69 ++++++++++++------- src/rustc/middle/trans/base.rs | 24 +++---- src/rustc/middle/ty.rs | 7 +- .../borrowck-loan-rcvr-overloaded-op.rs | 4 +- src/test/compile-fail/borrowck-loan-rcvr.rs | 4 +- src/test/compile-fail/vec-res-add.rs | 1 + 9 files changed, 89 insertions(+), 49 deletions(-) diff --git a/src/libsyntax/ast_map.rs b/src/libsyntax/ast_map.rs index c9b6b87942724..70aaa5e8be6f3 100644 --- a/src/libsyntax/ast_map.rs +++ b/src/libsyntax/ast_map.rs @@ -2,7 +2,7 @@ import std::map; import std::map::hashmap; import ast::*; import print::pprust; -import ast_util::path_to_ident; +import ast_util::{path_to_ident, stmt_id}; import diagnostic::span_handler; enum path_elt { path_mod(ident), path_name(ident) } @@ -39,6 +39,7 @@ enum ast_node { node_method(@method, def_id /* impl did */, @path /* path to the impl */), node_variant(variant, @item, @path), node_expr(@expr), + node_stmt(@stmt), node_export(@view_path, @path), // Locals are numbered, because the alias analysis needs to know in which // order they are introduced. @@ -65,6 +66,7 @@ fn mk_ast_map_visitor() -> vt { return visit::mk_vt(@{ visit_item: map_item, visit_expr: map_expr, + visit_stmt: map_stmt, visit_fn: map_fn, visit_local: map_local, visit_arm: map_arm, @@ -284,6 +286,11 @@ fn map_expr(ex: @expr, cx: ctx, v: vt) { visit::visit_expr(ex, cx, v); } +fn map_stmt(stmt: @stmt, cx: ctx, v: vt) { + cx.map.insert(stmt_id(*stmt), node_stmt(stmt)); + visit::visit_stmt(stmt, cx, v); +} + fn node_id_to_str(map: map, id: node_id) -> ~str { match map.find(id) { none => { @@ -313,6 +320,10 @@ fn node_id_to_str(map: map, id: node_id) -> ~str { fmt!{"expr %s (id=%?)", pprust::expr_to_str(expr), id} } + some(node_stmt(stmt)) => { + fmt!{"stmt %s (id=%?)", + pprust::stmt_to_str(*stmt), id} + } // FIXMEs are as per #2410 some(node_export(_, path)) => { fmt!{"export %s (id=%?)", // add more info here diff --git a/src/rustc/middle/borrowck/preserve.rs b/src/rustc/middle/borrowck/preserve.rs index 964a54bcad432..7b3b20db26978 100644 --- a/src/rustc/middle/borrowck/preserve.rs +++ b/src/rustc/middle/borrowck/preserve.rs @@ -79,10 +79,9 @@ priv impl &preserve_ctxt { let scope_region = if self.root_ub == 0 { ty::re_static } else { - ty::re_scope(self.root_ub) + ty::re_scope(self.tcx().region_map.get(cmt.id)) }; - // FIXME(#2977)--need to update trans! self.compare_scope(cmt, scope_region) } cat_stack_upvar(cmt) => { diff --git a/src/rustc/middle/kind.rs b/src/rustc/middle/kind.rs index e960e21c7525b..c41758f705282 100644 --- a/src/rustc/middle/kind.rs +++ b/src/rustc/middle/kind.rs @@ -389,8 +389,17 @@ fn is_nullary_variant(cx: ctx, ex: @expr) -> bool { fn check_copy_ex(cx: ctx, ex: @expr, implicit_copy: bool) { if ty::expr_is_lval(cx.method_map, ex) && - !cx.last_use_map.contains_key(ex.id) && - !is_nullary_variant(cx, ex) { + + // this is a move + !cx.last_use_map.contains_key(ex.id) && + + // a reference to a constant like `none`... no need to warn + // about *this* even if the type is option<~int> + !is_nullary_variant(cx, ex) && + + // borrowed unique value isn't really a copy + !cx.tcx.borrowings.contains_key(ex.id) + { let ty = ty::expr_ty(cx.tcx, ex); check_copy(cx, ex.id, ty, ex.span, implicit_copy); } diff --git a/src/rustc/middle/region.rs b/src/rustc/middle/region.rs index e226e0ef9c58a..4071800d4cfeb 100644 --- a/src/rustc/middle/region.rs +++ b/src/rustc/middle/region.rs @@ -28,16 +28,24 @@ type binding = {node_id: ast::node_id, name: ~str, br: ty::bound_region}; -/// Mapping from a block/expr/binding to the innermost scope that -/// bounds its lifetime. For a block/expression, this is the lifetime -/// in which it will be evaluated. For a binding, this is the lifetime -/// in which is in scope. +/** +Encodes the bounding lifetime for a given AST node: + +- Expressions are mapped to the expression or block encoding the maximum + (static) lifetime of a value produced by that expression. This is + generally the innermost call, statement, match, or block. + +- Variables and bindings are mapped to the block in which they are declared. + +*/ type region_map = hashmap; -type ctxt = { - sess: session, - def_map: resolve3::DefMap, - region_map: region_map, +struct ctxt { + sess: session; + def_map: resolve3::DefMap; + + // Generated maps: + region_map: region_map; // Generally speaking, expressions are parented to their innermost // enclosing block. But some kinds of expressions serve as @@ -46,9 +54,9 @@ type ctxt = { // the condition in a while loop is always a parent. In those // cases, we add the node id of such an expression to this set so // that when we visit it we can view it as a parent. - root_exprs: hashmap, + root_exprs: hashmap; - // The parent scope is the innermost block, call, or alt + // The parent scope is the innermost block, statement, call, or alt // expression during the execution of which the current expression // will be evaluated. Generally speaking, the innermost parent // scope is also the closest suitable ancestor in the AST tree. @@ -79,8 +87,8 @@ type ctxt = { // Here, the first argument `&**x` will be a borrow of the `~int`, // but the second argument overwrites that very value! Bad. // (This test is borrowck-pure-scope-in-call.rs, btw) - parent: parent -}; + parent: parent; +} /// Returns true if `subscope` is equal to or is lexically nested inside /// `superscope` and false otherwise. @@ -186,12 +194,9 @@ fn parent_id(cx: ctxt, span: span) -> ast::node_id { /// Records the current parent (if any) as the parent of `child_id`. fn record_parent(cx: ctxt, child_id: ast::node_id) { - match cx.parent { - none => { /* no-op */ } - some(parent_id) => { + for cx.parent.each |parent_id| { debug!{"parent of node %d is node %d", child_id, parent_id}; cx.region_map.insert(child_id, parent_id); - } } } @@ -200,7 +205,7 @@ fn resolve_block(blk: ast::blk, cx: ctxt, visitor: visit::vt) { record_parent(cx, blk.node.id); // Descend. - let new_cx: ctxt = {parent: some(blk.node.id) with cx}; + let new_cx: ctxt = ctxt {parent: some(blk.node.id) with cx}; visit::visit_block(blk, new_cx, visitor); } @@ -228,6 +233,21 @@ fn resolve_pat(pat: @ast::pat, cx: ctxt, visitor: visit::vt) { visit::visit_pat(pat, cx, visitor); } +fn resolve_stmt(stmt: @ast::stmt, cx: ctxt, visitor: visit::vt) { + match stmt.node { + ast::stmt_decl(*) => { + visit::visit_stmt(stmt, cx, visitor); + } + ast::stmt_expr(expr, stmt_id) | + ast::stmt_semi(expr, stmt_id) => { + record_parent(cx, stmt_id); + let mut expr_cx = cx; + expr_cx.parent = some(stmt_id); + visit::visit_stmt(stmt, expr_cx, visitor); + } + } +} + fn resolve_expr(expr: @ast::expr, cx: ctxt, visitor: visit::vt) { record_parent(cx, expr.id); @@ -270,7 +290,7 @@ fn resolve_local(local: @ast::local, cx: ctxt, visitor: visit::vt) { fn resolve_item(item: @ast::item, cx: ctxt, visitor: visit::vt) { // Items create a new outer block scope as far as we're concerned. - let new_cx: ctxt = {parent: none with cx}; + let new_cx: ctxt = ctxt {parent: none with cx}; visit::visit_item(item, new_cx, visitor); } @@ -282,7 +302,7 @@ fn resolve_fn(fk: visit::fn_kind, decl: ast::fn_decl, body: ast::blk, visit::fk_item_fn(*) | visit::fk_method(*) | visit::fk_ctor(*) | visit::fk_dtor(*) => { // Top-level functions are a root scope. - {parent: some(id) with cx} + ctxt {parent: some(id) with cx} } visit::fk_anon(*) | visit::fk_fn_block(*) => { @@ -304,17 +324,18 @@ fn resolve_fn(fk: visit::fn_kind, decl: ast::fn_decl, body: ast::blk, fn resolve_crate(sess: session, def_map: resolve3::DefMap, crate: @ast::crate) -> region_map { - let cx: ctxt = {sess: sess, - def_map: def_map, - region_map: int_hash(), - root_exprs: int_hash(), - parent: none}; + let cx: ctxt = ctxt {sess: sess, + def_map: def_map, + region_map: int_hash(), + root_exprs: int_hash(), + parent: none}; let visitor = visit::mk_vt(@{ visit_block: resolve_block, visit_item: resolve_item, visit_fn: resolve_fn, visit_arm: resolve_arm, visit_pat: resolve_pat, + visit_stmt: resolve_stmt, visit_expr: resolve_expr, visit_local: resolve_local with *visit::default_visitor() diff --git a/src/rustc/middle/trans/base.rs b/src/rustc/middle/trans/base.rs index 6e1f2e7980508..d266a5b59d1d0 100644 --- a/src/rustc/middle/trans/base.rs +++ b/src/rustc/middle/trans/base.rs @@ -2171,6 +2171,9 @@ fn monomorphic_fn(ccx: @crate_ctxt, fn_id: ast::def_id, ast_map::node_expr(*) => { ccx.tcx.sess.bug(~"Can't monomorphize an expr") } + ast_map::node_stmt(*) => { + ccx.tcx.sess.bug(~"Can't monomorphize a stmt") + } ast_map::node_export(*) => { ccx.tcx.sess.bug(~"Can't monomorphize an export") } @@ -2270,21 +2273,14 @@ fn monomorphic_fn(ccx: @crate_ctxt, fn_id: ast::def_id, dtor.node.id, psubsts, some(hash_id), parent_id) } // Ugh -- but this ensures any new variants won't be forgotten - ast_map::node_expr(*) => { - ccx.tcx.sess.bug(~"Can't monomorphize an expr") - } - ast_map::node_trait_method(*) => { - ccx.tcx.sess.bug(~"Can't monomorphize a trait method") - } - ast_map::node_export(*) => { - ccx.tcx.sess.bug(~"Can't monomorphize an export") - } - ast_map::node_arg(*) => ccx.tcx.sess.bug(~"Can't monomorphize an arg"), - ast_map::node_block(*) => { - ccx.tcx.sess.bug(~"Can't monomorphize a block") - } + ast_map::node_expr(*) | + ast_map::node_stmt(*) | + ast_map::node_trait_method(*) | + ast_map::node_export(*) | + ast_map::node_arg(*) | + ast_map::node_block(*) | ast_map::node_local(*) => { - ccx.tcx.sess.bug(~"Can't monomorphize a local") + ccx.tcx.sess.bug(fmt!("Can't monomorphize a %?", map_node)) } }; ccx.monomorphizing.insert(fn_id, depth); diff --git a/src/rustc/middle/ty.rs b/src/rustc/middle/ty.rs index 8936e203e0d98..e01fdeee60a16 100644 --- a/src/rustc/middle/ty.rs +++ b/src/rustc/middle/ty.rs @@ -2861,10 +2861,9 @@ fn item_path(cx: ctxt, id: ast::def_id) -> ast_map::path { vec::append_one(*path, ast_map::path_name(@~"dtor")) } - - ast_map::node_expr(_) | ast_map::node_arg(_, _) | - ast_map::node_local(_) | ast_map::node_export(_, _) | - ast_map::node_block(_) => { + ast_map::node_stmt(*) | ast_map::node_expr(*) | + ast_map::node_arg(*) | ast_map::node_local(*) | + ast_map::node_export(*) | ast_map::node_block(*) => { cx.sess.bug(fmt!{"cannot find item_path for node %?", node}); } } diff --git a/src/test/compile-fail/borrowck-loan-rcvr-overloaded-op.rs b/src/test/compile-fail/borrowck-loan-rcvr-overloaded-op.rs index 00ce2485b68c1..ecaad792fb9aa 100644 --- a/src/test/compile-fail/borrowck-loan-rcvr-overloaded-op.rs +++ b/src/test/compile-fail/borrowck-loan-rcvr-overloaded-op.rs @@ -28,11 +28,13 @@ fn b() { // Here I create an outstanding loan and check that we get conflicts: - &mut p; //~ NOTE prior loan as mutable granted here + let q = &mut p; //~ NOTE prior loan as mutable granted here //~^ NOTE prior loan as mutable granted here p + 3; //~ ERROR loan of mutable local variable as immutable conflicts with prior loan p.times(3); //~ ERROR loan of mutable local variable as immutable conflicts with prior loan + + q.x += 1; } fn c() { diff --git a/src/test/compile-fail/borrowck-loan-rcvr.rs b/src/test/compile-fail/borrowck-loan-rcvr.rs index 6972905a8ccc9..39550e6ecd437 100644 --- a/src/test/compile-fail/borrowck-loan-rcvr.rs +++ b/src/test/compile-fail/borrowck-loan-rcvr.rs @@ -35,11 +35,13 @@ fn b() { // Here I create an outstanding loan and check that we get conflicts: - &mut p; //~ NOTE prior loan as mutable granted here + let l = &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 + + l.x += 1; } fn c() { diff --git a/src/test/compile-fail/vec-res-add.rs b/src/test/compile-fail/vec-res-add.rs index 7fbcc67da17d7..4b558d7fe8f15 100644 --- a/src/test/compile-fail/vec-res-add.rs +++ b/src/test/compile-fail/vec-res-add.rs @@ -1,3 +1,4 @@ +// xfail-test #2587 // error-pattern: copying a noncopyable value struct r {