Skip to content

Commit

Permalink
make borrowck more conservative around rvalues.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
nikomatsakis committed Aug 17, 2012
1 parent 8f01343 commit ea549e7
Show file tree
Hide file tree
Showing 9 changed files with 89 additions and 49 deletions.
13 changes: 12 additions & 1 deletion src/libsyntax/ast_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) }
Expand Down Expand Up @@ -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.
Expand All @@ -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,
Expand Down Expand Up @@ -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 => {
Expand Down Expand Up @@ -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
Expand Down
3 changes: 1 addition & 2 deletions src/rustc/middle/borrowck/preserve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down
13 changes: 11 additions & 2 deletions src/rustc/middle/kind.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
69 changes: 45 additions & 24 deletions src/rustc/middle/region.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<ast::node_id, ast::node_id>;

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
Expand All @@ -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<ast::node_id, ()>,
root_exprs: hashmap<ast::node_id, ()>;

// 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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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);
}
}
}

Expand All @@ -200,7 +205,7 @@ fn resolve_block(blk: ast::blk, cx: ctxt, visitor: visit::vt<ctxt>) {
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);
}

Expand Down Expand Up @@ -228,6 +233,21 @@ fn resolve_pat(pat: @ast::pat, cx: ctxt, visitor: visit::vt<ctxt>) {
visit::visit_pat(pat, cx, visitor);
}

fn resolve_stmt(stmt: @ast::stmt, cx: ctxt, visitor: visit::vt<ctxt>) {
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<ctxt>) {
record_parent(cx, expr.id);

Expand Down Expand Up @@ -270,7 +290,7 @@ fn resolve_local(local: @ast::local, cx: ctxt, visitor: visit::vt<ctxt>) {

fn resolve_item(item: @ast::item, cx: ctxt, visitor: visit::vt<ctxt>) {
// 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);
}

Expand All @@ -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(*) => {
Expand All @@ -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()
Expand Down
24 changes: 10 additions & 14 deletions src/rustc/middle/trans/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand Down Expand Up @@ -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);
Expand Down
7 changes: 3 additions & 4 deletions src/rustc/middle/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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});
}
}
Expand Down
4 changes: 3 additions & 1 deletion src/test/compile-fail/borrowck-loan-rcvr-overloaded-op.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
4 changes: 3 additions & 1 deletion src/test/compile-fail/borrowck-loan-rcvr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
1 change: 1 addition & 0 deletions src/test/compile-fail/vec-res-add.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// xfail-test #2587
// error-pattern: copying a noncopyable value

struct r {
Expand Down

0 comments on commit ea549e7

Please sign in to comment.