From 86bde933f863424b090129a49aa8bf328b491ffd Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Mon, 26 Jan 2015 13:38:33 +0100 Subject: [PATCH 1/6] accommodate new scoping rules in rustc and rustdoc source. --- src/librustc/middle/infer/error_reporting.rs | 4 +-- src/librustc_driver/driver.rs | 2 +- src/librustc_trans/trans/base.rs | 26 +++++++++++--------- src/librustc_trans/trans/callee.rs | 19 +++++++------- src/librustc_trans/trans/glue.rs | 9 ++++--- src/librustc_trans/trans/meth.rs | 20 +++++++-------- src/librustc_typeck/check/method/probe.rs | 2 +- src/librustc_typeck/check/mod.rs | 2 +- src/librustdoc/core.rs | 2 +- 9 files changed, 45 insertions(+), 41 deletions(-) diff --git a/src/librustc/middle/infer/error_reporting.rs b/src/librustc/middle/infer/error_reporting.rs index 18c36f870b5bc..cfef88a8deb44 100644 --- a/src/librustc/middle/infer/error_reporting.rs +++ b/src/librustc/middle/infer/error_reporting.rs @@ -809,6 +809,8 @@ impl<'a, 'tcx> ErrorReporting<'tcx> for InferCtxt<'a, 'tcx> { let scope_id = same_regions[0].scope_id; let parent = self.tcx.map.get_parent(scope_id); let parent_node = self.tcx.map.find(parent); + let taken = lifetimes_in_scope(self.tcx, scope_id); + let life_giver = LifeGiver::with_taken(&taken[]); let node_inner = match parent_node { Some(ref node) => match *node { ast_map::NodeItem(ref item) => { @@ -851,8 +853,6 @@ impl<'a, 'tcx> ErrorReporting<'tcx> for InferCtxt<'a, 'tcx> { }; let (fn_decl, generics, unsafety, ident, expl_self, span) = node_inner.expect("expect item fn"); - let taken = lifetimes_in_scope(self.tcx, scope_id); - let life_giver = LifeGiver::with_taken(&taken[]); let rebuilder = Rebuilder::new(self.tcx, fn_decl, expl_self, generics, same_regions, &life_giver); let (fn_decl, expl_self, generics) = rebuilder.rebuild(); diff --git a/src/librustc_driver/driver.rs b/src/librustc_driver/driver.rs index 36a9c0e16f018..25b90041505a7 100644 --- a/src/librustc_driver/driver.rs +++ b/src/librustc_driver/driver.rs @@ -100,6 +100,7 @@ pub fn compile_input(sess: Session, &id[])); let mut forest = ast_map::Forest::new(expanded_crate); + let arenas = ty::CtxtArenas::new(); let ast_map = assign_node_ids_and_map(&sess, &mut forest); write_out_deps(&sess, input, &outputs, &id[]); @@ -111,7 +112,6 @@ pub fn compile_input(sess: Session, &ast_map, &id[])); - let arenas = ty::CtxtArenas::new(); let analysis = phase_3_run_analysis_passes(sess, ast_map, &arenas, diff --git a/src/librustc_trans/trans/base.rs b/src/librustc_trans/trans/base.rs index 230a4c5d4272e..5a98bc4da3682 100644 --- a/src/librustc_trans/trans/base.rs +++ b/src/librustc_trans/trans/base.rs @@ -1784,15 +1784,16 @@ pub fn trans_closure<'a, 'b, 'tcx>(ccx: &CrateContext<'a, 'tcx>, debug!("trans_closure(..., param_substs={})", param_substs.repr(ccx.tcx())); - let arena = TypedArena::new(); - let fcx = new_fn_ctxt(ccx, - llfndecl, - fn_ast_id, - closure_env.kind != closure::NotClosure, - output_type, - param_substs, - Some(body.span), - &arena); + let (arena, fcx): (TypedArena<_>, FunctionContext); + arena = TypedArena::new(); + fcx = new_fn_ctxt(ccx, + llfndecl, + fn_ast_id, + closure_env.kind != closure::NotClosure, + output_type, + param_substs, + Some(body.span), + &arena); let mut bcx = init_function(&fcx, false, output_type); // cleanup scope for the incoming arguments @@ -2046,9 +2047,10 @@ fn trans_enum_variant_or_tuple_like_struct<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx ty_to_string(ccx.tcx(), ctor_ty))[]) }; - let arena = TypedArena::new(); - let fcx = new_fn_ctxt(ccx, llfndecl, ctor_id, false, result_ty, - param_substs, None, &arena); + let (arena, fcx): (TypedArena<_>, FunctionContext); + arena = TypedArena::new(); + fcx = new_fn_ctxt(ccx, llfndecl, ctor_id, false, result_ty, + param_substs, None, &arena); let bcx = init_function(&fcx, false, result_ty); assert!(!fcx.needs_ret_allocas); diff --git a/src/librustc_trans/trans/callee.rs b/src/librustc_trans/trans/callee.rs index 73d174c2c3419..d038407791ef6 100644 --- a/src/librustc_trans/trans/callee.rs +++ b/src/librustc_trans/trans/callee.rs @@ -322,16 +322,17 @@ pub fn trans_fn_pointer_shim<'a, 'tcx>( &function_name[]); // - let block_arena = TypedArena::new(); let empty_substs = Substs::trans_empty(); - let fcx = new_fn_ctxt(ccx, - llfn, - ast::DUMMY_NODE_ID, - false, - sig.output, - &empty_substs, - None, - &block_arena); + let (block_arena, fcx): (TypedArena<_>, FunctionContext); + block_arena = TypedArena::new(); + fcx = new_fn_ctxt(ccx, + llfn, + ast::DUMMY_NODE_ID, + false, + sig.output, + &empty_substs, + None, + &block_arena); let mut bcx = init_function(&fcx, false, sig.output); // the first argument (`self`) will be ptr to the the fn pointer diff --git a/src/librustc_trans/trans/glue.rs b/src/librustc_trans/trans/glue.rs index c1d98987991e3..69d1922ab9adb 100644 --- a/src/librustc_trans/trans/glue.rs +++ b/src/librustc_trans/trans/glue.rs @@ -540,11 +540,12 @@ fn make_generic_glue<'a, 'tcx, F>(ccx: &CrateContext<'a, 'tcx>, let glue_name = format!("glue {} {}", name, ty_to_short_str(ccx.tcx(), t)); let _s = StatRecorder::new(ccx, glue_name); - let arena = TypedArena::new(); let empty_param_substs = Substs::trans_empty(); - let fcx = new_fn_ctxt(ccx, llfn, ast::DUMMY_NODE_ID, false, - ty::FnConverging(ty::mk_nil(ccx.tcx())), - &empty_param_substs, None, &arena); + let (arena, fcx): (TypedArena<_>, FunctionContext); + arena = TypedArena::new(); + fcx = new_fn_ctxt(ccx, llfn, ast::DUMMY_NODE_ID, false, + ty::FnConverging(ty::mk_nil(ccx.tcx())), + &empty_param_substs, None, &arena); let bcx = init_function(&fcx, false, ty::FnConverging(ty::mk_nil(ccx.tcx()))); diff --git a/src/librustc_trans/trans/meth.rs b/src/librustc_trans/trans/meth.rs index 580a2d5ae3dc7..187b73b1b0952 100644 --- a/src/librustc_trans/trans/meth.rs +++ b/src/librustc_trans/trans/meth.rs @@ -601,17 +601,17 @@ pub fn trans_object_shim<'a, 'tcx>( let sig = ty::erase_late_bound_regions(ccx.tcx(), &fty.sig); - // - let block_arena = TypedArena::new(); let empty_substs = Substs::trans_empty(); - let fcx = new_fn_ctxt(ccx, - llfn, - ast::DUMMY_NODE_ID, - false, - sig.output, - &empty_substs, - None, - &block_arena); + let (block_arena, fcx): (TypedArena<_>, FunctionContext); + block_arena = TypedArena::new(); + fcx = new_fn_ctxt(ccx, + llfn, + ast::DUMMY_NODE_ID, + false, + sig.output, + &empty_substs, + None, + &block_arena); let mut bcx = init_function(&fcx, false, sig.output); // the first argument (`self`) will be a trait object diff --git a/src/librustc_typeck/check/method/probe.rs b/src/librustc_typeck/check/method/probe.rs index 61332ada506de..4c5a8144cbd06 100644 --- a/src/librustc_typeck/check/method/probe.rs +++ b/src/librustc_typeck/check/method/probe.rs @@ -1049,8 +1049,8 @@ impl<'a,'tcx> ProbeContext<'a,'tcx> { // if there are any. assert_eq!(substs.types.len(subst::FnSpace), 0); assert_eq!(substs.regions().len(subst::FnSpace), 0); - let mut substs = substs; let placeholder; + let mut substs = substs; if !method.generics.types.is_empty_in(subst::FnSpace) || !method.generics.regions.is_empty_in(subst::FnSpace) diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index 8de1ffd37e01b..65560040f47c2 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -3101,8 +3101,8 @@ fn check_expr_with_unifier<'a, 'tcx, F>(fcx: &FnCtxt<'a, 'tcx>, let name = ident.get(); // only find fits with at least one matching letter let mut best_dist = name.len(); - let mut best = None; let fields = ty::lookup_struct_fields(tcx, id); + let mut best = None; for elem in fields.iter() { let n = elem.name.as_str(); // ignore already set fields diff --git a/src/librustdoc/core.rs b/src/librustdoc/core.rs index 04947e41663ec..90a0a0f843e94 100644 --- a/src/librustdoc/core.rs +++ b/src/librustdoc/core.rs @@ -126,9 +126,9 @@ pub fn run_core(search_paths: SearchPaths, cfgs: Vec, externs: Externs, .expect("phase_2_configure_and_expand aborted in rustdoc!"); let mut forest = ast_map::Forest::new(krate); + let arenas = ty::CtxtArenas::new(); let ast_map = driver::assign_node_ids_and_map(&sess, &mut forest); - let arenas = ty::CtxtArenas::new(); let ty::CrateAnalysis { exported_items, public_items, ty_cx, .. } = driver::phase_3_run_analysis_passes(sess, From 9fe8d8602d9400131ebcd258014c8d9b8ddfcf05 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Mon, 26 Jan 2015 15:16:16 +0100 Subject: [PATCH 2/6] accommodate new scoping rules in test/run-pass. --- src/test/run-pass/issue-3026.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/test/run-pass/issue-3026.rs b/src/test/run-pass/issue-3026.rs index cd71bfce27428..25663f2605fcb 100644 --- a/src/test/run-pass/issue-3026.rs +++ b/src/test/run-pass/issue-3026.rs @@ -17,7 +17,8 @@ extern crate collections; use std::collections::HashMap; pub fn main() { + let x; let mut buggy_map: HashMap = HashMap::new(); - let x = box 1; + x = box 1; buggy_map.insert(42, &*x); } From 70192ab7796b5c25b0f6ef015f6e3ceacf73a9b8 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Mon, 26 Jan 2015 15:21:24 +0100 Subject: [PATCH 3/6] accommodate new scoping rules in test/compile-fail. --- src/test/compile-fail/borrowck-borrowed-uniq-rvalue.rs | 3 ++- src/test/compile-fail/issue-18783.rs | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/test/compile-fail/borrowck-borrowed-uniq-rvalue.rs b/src/test/compile-fail/borrowck-borrowed-uniq-rvalue.rs index 98d1905ed9068..9c10f01e02755 100644 --- a/src/test/compile-fail/borrowck-borrowed-uniq-rvalue.rs +++ b/src/test/compile-fail/borrowck-borrowed-uniq-rvalue.rs @@ -16,10 +16,11 @@ extern crate collections; use std::collections::HashMap; fn main() { + let tmp; let mut buggy_map: HashMap = HashMap::new(); buggy_map.insert(42, &*box 1); //~ ERROR borrowed value does not live long enough // but it is ok if we use a temporary - let tmp = box 2; + tmp = box 2; buggy_map.insert(43, &*tmp); } diff --git a/src/test/compile-fail/issue-18783.rs b/src/test/compile-fail/issue-18783.rs index bed835d9bde8c..d26bf68cb5dfe 100644 --- a/src/test/compile-fail/issue-18783.rs +++ b/src/test/compile-fail/issue-18783.rs @@ -13,16 +13,16 @@ use std::cell::RefCell; fn main() { - let c = RefCell::new(vec![]); let mut y = 1us; + let c = RefCell::new(vec![]); c.push(box || y = 0); c.push(box || y = 0); //~^ ERROR cannot borrow `y` as mutable more than once at a time } fn ufcs() { - let c = RefCell::new(vec![]); let mut y = 1us; + let c = RefCell::new(vec![]); Push::push(&c, box || y = 0); Push::push(&c, box || y = 0); From 2c2f0f121691c51e1f259801204a48e959ee7957 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Mon, 26 Jan 2015 15:39:56 +0100 Subject: [PATCH 4/6] accommodate new scoping rules in libstd unit tests. --- src/libstd/old_io/process.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstd/old_io/process.rs b/src/libstd/old_io/process.rs index d7c263b3d1fcd..280d2a8add5ea 100644 --- a/src/libstd/old_io/process.rs +++ b/src/libstd/old_io/process.rs @@ -1079,13 +1079,13 @@ mod tests { #[test] fn test_override_env() { use os; - let mut new_env = vec![("RUN_TEST_NEW_ENV", "123")]; // In some build environments (such as chrooted Nix builds), `env` can // only be found in the explicitly-provided PATH env variable, not in // default places such as /bin or /usr/bin. So we need to pass through // PATH to our sub-process. let path_val: String; + let mut new_env = vec![("RUN_TEST_NEW_ENV", "123")]; match os::getenv("PATH") { None => {} Some(val) => { From 43d08f861a3cca10f385e8f7ec34f85d7b9f54dc Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Mon, 26 Jan 2015 15:44:19 +0100 Subject: [PATCH 5/6] accommodate new scoping rules in librustc_driver unit tests. --- src/librustc_driver/test.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_driver/test.rs b/src/librustc_driver/test.rs index cd28a27f9886b..67b15ce8a871e 100644 --- a/src/librustc_driver/test.rs +++ b/src/librustc_driver/test.rs @@ -115,6 +115,7 @@ fn test_env(source_string: &str, .expect("phase 2 aborted"); let mut forest = ast_map::Forest::new(krate); + let arenas = ty::CtxtArenas::new(); let ast_map = driver::assign_node_ids_and_map(&sess, &mut forest); let krate = ast_map.krate(); @@ -125,7 +126,6 @@ fn test_env(source_string: &str, let named_region_map = resolve_lifetime::krate(&sess, krate, &def_map); let region_map = region::resolve_crate(&sess, krate); let stability_index = stability::Index::build(krate); - let arenas = ty::CtxtArenas::new(); let tcx = ty::mk_ctxt(sess, &arenas, def_map, From d6bf04a22e78afbe62e9d81e3578418f420123bc Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Mon, 26 Jan 2015 12:48:19 +0100 Subject: [PATCH 6/6] Add `CodeExtent::Remainder` variant; pre-req for new scoping/drop rules. This new variant introduces finer-grain code extents, i.e. we now track that a binding lives only for a suffix of a block, and (importantly) will be dropped when it goes out of scope *before* the bindings that occurred earlier in the block. Both of these notions are neatly captured by marking the block (and each suffix) as an enclosing scope of the next suffix beneath it. This is work that is part of the foundation for issue #8861. (It actually has been seen in earlier posted pull requests; I have just factored it out into its own PR to ease my own rebasing.) ---- These finer grained scopes do mean that some code is newly rejected by `rustc`; for example: ```rust let mut map : HashMap = HashMap::new(); let tmp = Box::new(2); map.insert(43, &*tmp); ``` This will now fail to compile with a message that `*tmp` does not live long enough, because the scope of `tmp` is now strictly smaller than that of `map`, and the use of `&u8` in map's type requires that the borrowed references are all to data that live at least as long as the map. The usual fix for a case like this is to move the binding for `tmp` up above that of `map`; note that you can still leave the initialization in the original spot, like so: ```rust let tmp; let mut map : HashMap = HashMap::new(); tmp = box 2; map.insert(43, &*tmp); ``` Similarly, one can encounter an analogous situation with `Vec`: one would need to rewrite: ```rust let mut vec = Vec::new(); let tmp = 'c'; vec.push(&tmp); ``` as: ``` let tmp; let mut vec = Vec::new(); tmp = 'c'; vec.push(&tmp); ``` ---- In some corner cases, it does not suffice to reorder the bindings; in particular, when the types for both bindings need to reflect exactly the *same* code extent, and a parent/child relationship between them does not work. In pnkfelix's experience this has arisen most often when mixing uses of cyclic data structures while also allowing a lifetime parameter `'a` to flow into a type parameter context where the type is *invariant* with respect to the type parameter. An important instance of this is `arena::TypedArena`, which is invariant with respect to `T`. (The reason that variance is relevant is this: *if* `TypedArena` were covariant with respect to its type parameter, then we could assign it the longer lifetime when it is initialized, and then convert it to a subtype (via covariance) with a shorter lifetime when necessary. But `TypedArena` is invariant with respect to its type parameter, and thus if `S` is a subtype of `T` (in particular, if `S` has a lifetime parameter that is shorter than that of `T`), then a `TypedArena` is unrelated to `TypedArena`.) Concretely, consider code like this: ```rust struct Node<'a> { sibling: Option<&'a Node<'a>> } struct Context<'a> { // because of this field, `Context<'a>` is invariant with respect to `'a`. arena: &'a TypedArena>, ... } fn new_ctxt<'a>(arena: &'a TypedArena>) -> Context<'a> { ... } fn use_ctxt<'a>(fcx: &'a Context<'a>) { ... } let arena = TypedArena::new(); let ctxt = new_ctxt(&arena); use_ctxt(&ctxt); ``` In these situations, if you try to introduce two bindings via two distinct `let` statements, each is (with this commit) assigned a distinct extent, and the region inference system cannot find a single region to assign to the lifetime `'a` that works for both of the bindings. So you get an error that `ctxt` does not live long enough; but moving its binding up above that of `arena` just shifts the error so now the compiler complains that `arena` does not live long enough. SO: What to do? The easiest fix in this case is to ensure that the two bindings *do* get assigned the same static extent, by stuffing both bindings into the same let statement, like so: ```rust let (arena, ctxt): (TypedArena, Context); arena = TypedArena::new(); ctxt = new_ctxt(&arena); use_ctxt(&ctxt); ``` Due to the new code rejections outlined above, this is a ... [breaking-change] --- src/librustc/metadata/tydecode.rs | 8 + src/librustc/metadata/tyencode.rs | 4 +- src/librustc/middle/region.rs | 289 ++++++++++++++---- src/librustc/util/ppaux.rs | 72 +++-- .../borrowck-let-suggestion-suffixes.rs | 57 ++++ .../compile-fail/borrowck-let-suggestion.rs | 2 +- 6 files changed, 351 insertions(+), 81 deletions(-) create mode 100644 src/test/compile-fail/borrowck-let-suggestion-suffixes.rs diff --git a/src/librustc/metadata/tydecode.rs b/src/librustc/metadata/tydecode.rs index cb6b7e56b5770..2ee4b6fbbd4ad 100644 --- a/src/librustc/metadata/tydecode.rs +++ b/src/librustc/metadata/tydecode.rs @@ -377,6 +377,14 @@ fn parse_scope(st: &mut PState) -> region::CodeExtent { let node_id = parse_uint(st) as ast::NodeId; region::CodeExtent::Misc(node_id) } + 'B' => { + let node_id = parse_uint(st) as ast::NodeId; + let first_stmt_index = parse_uint(st); + let block_remainder = region::BlockRemainder { + block: node_id, first_statement_index: first_stmt_index, + }; + region::CodeExtent::Remainder(block_remainder) + } _ => panic!("parse_scope: bad input") } } diff --git a/src/librustc/metadata/tyencode.rs b/src/librustc/metadata/tyencode.rs index 658bacdb576c3..2dc334bfe95fc 100644 --- a/src/librustc/metadata/tyencode.rs +++ b/src/librustc/metadata/tyencode.rs @@ -276,7 +276,9 @@ pub fn enc_region(w: &mut SeekableMemWriter, cx: &ctxt, r: ty::Region) { fn enc_scope(w: &mut SeekableMemWriter, _cx: &ctxt, scope: region::CodeExtent) { match scope { - region::CodeExtent::Misc(node_id) => mywrite!(w, "M{}", node_id) + region::CodeExtent::Misc(node_id) => mywrite!(w, "M{}", node_id), + region::CodeExtent::Remainder(region::BlockRemainder { + block: b, first_statement_index: i }) => mywrite!(w, "B{}{}", b, i), } } diff --git a/src/librustc/middle/region.rs b/src/librustc/middle/region.rs index 67c0e52d6649a..9bba01f8af771 100644 --- a/src/librustc/middle/region.rs +++ b/src/librustc/middle/region.rs @@ -26,6 +26,7 @@ use syntax::codemap::{self, Span}; use syntax::{ast, visit}; use syntax::ast::{Block, Item, FnDecl, NodeId, Arm, Pat, Stmt, Expr, Local}; use syntax::ast_util::{stmt_id}; +use syntax::ast_map; use syntax::visit::{Visitor, FnKind}; /// CodeExtent represents a statically-describable extent that can be @@ -38,7 +39,32 @@ use syntax::visit::{Visitor, FnKind}; #[derive(Clone, PartialEq, PartialOrd, Eq, Ord, Hash, RustcEncodable, RustcDecodable, Show, Copy)] pub enum CodeExtent { - Misc(ast::NodeId) + Misc(ast::NodeId), + Remainder(BlockRemainder), +} + +/// Represents a subscope of `block` for a binding that is introduced +/// by `block.stmts[first_statement_index]`. Such subscopes represent +/// a suffix of the block. Note that each subscope does not include +/// the initializer expression, if any, for the statement indexed by +/// `first_statement_index`. +/// +/// For example, given `{ let (a, b) = EXPR_1; let c = EXPR_2; ... }`: +/// +/// * the subscope with `first_statement_index == 0` is scope of both +/// `a` and `b`; it does not include EXPR_1, but does include +/// everything after that first `let`. (If you want a scope that +/// includes EXPR_1 as well, then do not use `CodeExtent::Remainder`, +/// but instead another `CodeExtent` that encompasses the whole block, +/// e.g. `CodeExtent::Misc`. +/// +/// * the subscope with `first_statement_index == 1` is scope of `c`, +/// and thus does not include EXPR_2, but covers the `...`. +#[derive(Clone, PartialEq, PartialOrd, Eq, Ord, Hash, RustcEncodable, + RustcDecodable, Show, Copy)] +pub struct BlockRemainder { + pub block: ast::NodeId, + pub first_statement_index: uint, } impl CodeExtent { @@ -55,6 +81,7 @@ impl CodeExtent { pub fn node_id(&self) -> ast::NodeId { match *self { CodeExtent::Misc(node_id) => node_id, + CodeExtent::Remainder(br) => br.block, } } @@ -65,8 +92,41 @@ impl CodeExtent { { match *self { CodeExtent::Misc(node_id) => CodeExtent::Misc(f_id(node_id)), + CodeExtent::Remainder(br) => + CodeExtent::Remainder(BlockRemainder { + block: f_id(br.block), first_statement_index: br.first_statement_index }), } } + + /// Returns the span of this CodeExtent. Note that in general the + /// returned span may not correspond to the span of any node id in + /// the AST. + pub fn span(&self, ast_map: &ast_map::Map) -> Option { + match ast_map.find(self.node_id()) { + Some(ast_map::NodeBlock(ref blk)) => { + match *self { + CodeExtent::Misc(_) => Some(blk.span), + + CodeExtent::Remainder(r) => { + assert_eq!(r.block, blk.id); + // Want span for extent starting after the + // indexed statement and ending at end of + // `blk`; reuse span of `blk` and shift `lo` + // forward to end of indexed statement. + // + // (This is the special case aluded to in the + // doc-comment for this method) + let stmt_span = blk.stmts[r.first_statement_index].span; + Some(Span { lo: stmt_span.hi, ..blk.span }) + } + } + } + Some(ast_map::NodeExpr(ref expr)) => Some(expr.span), + Some(ast_map::NodeStmt(ref stmt)) => Some(stmt.span), + Some(ast_map::NodeItem(ref item)) => Some(item.span), + Some(_) | None => None, + } + } } /// The region maps encode information about region relationships. @@ -74,7 +134,8 @@ impl CodeExtent { /// - `scope_map` maps from a scope id to the enclosing scope id; this is /// usually corresponding to the lexical nesting, though in the case of /// closures the parent scope is the innermost conditional expression or repeating -/// block +/// block. (Note that the enclosing scope id for the block +/// associated with a closure is the closure itself.) /// /// - `var_map` maps from a variable or binding id to the block in which /// that variable is declared. @@ -115,12 +176,77 @@ pub struct RegionMaps { terminating_scopes: RefCell>, } -#[derive(Copy)] +/// Carries the node id for the innermost block or match expression, +/// for building up the `var_map` which maps ids to the blocks in +/// which they were declared. +#[derive(PartialEq, Eq, Show, Copy)] +enum InnermostDeclaringBlock { + None, + Block(ast::NodeId), + Statement(DeclaringStatementContext), + Match(ast::NodeId), +} + +impl InnermostDeclaringBlock { + fn to_code_extent(&self) -> Option { + let extent = match *self { + InnermostDeclaringBlock::None => { + return Option::None; + } + InnermostDeclaringBlock::Block(id) | + InnermostDeclaringBlock::Match(id) => CodeExtent::from_node_id(id), + InnermostDeclaringBlock::Statement(s) => s.to_code_extent(), + }; + Option::Some(extent) + } +} + +/// Contextual information for declarations introduced by a statement +/// (i.e. `let`). It carries node-id's for statement and enclosing +/// block both, as well as the statement's index within the block. +#[derive(PartialEq, Eq, Show, Copy)] +struct DeclaringStatementContext { + stmt_id: ast::NodeId, + block_id: ast::NodeId, + stmt_index: uint, +} + +impl DeclaringStatementContext { + fn to_code_extent(&self) -> CodeExtent { + CodeExtent::Remainder(BlockRemainder { + block: self.block_id, + first_statement_index: self.stmt_index, + }) + } +} + +#[derive(PartialEq, Eq, Show, Copy)] +enum InnermostEnclosingExpr { + None, + Some(ast::NodeId), + Statement(DeclaringStatementContext), +} + +impl InnermostEnclosingExpr { + fn to_code_extent(&self) -> Option { + let extent = match *self { + InnermostEnclosingExpr::None => { + return Option::None; + } + InnermostEnclosingExpr::Statement(s) => + s.to_code_extent(), + InnermostEnclosingExpr::Some(parent_id) => + CodeExtent::from_node_id(parent_id), + }; + Some(extent) + } +} + +#[derive(Show, Copy)] pub struct Context { - var_parent: Option, + var_parent: InnermostDeclaringBlock, - // Innermost enclosing expression - parent: Option, + parent: InnermostEnclosingExpr, } struct RegionResolutionVisitor<'a> { @@ -381,16 +507,13 @@ impl RegionMaps { } } -/// Records the current parent (if any) as the parent of `child_id`. +/// Records the current parent (if any) as the parent of `child_scope`. fn record_superlifetime(visitor: &mut RegionResolutionVisitor, - child_id: ast::NodeId, + child_scope: CodeExtent, _sp: Span) { - match visitor.cx.parent { - Some(parent_id) => { - let child_scope = CodeExtent::from_node_id(child_id); - let parent_scope = CodeExtent::from_node_id(parent_id); - visitor.region_maps.record_encl_scope(child_scope, parent_scope); - } + match visitor.cx.parent.to_code_extent() { + Some(parent_scope) => + visitor.region_maps.record_encl_scope(child_scope, parent_scope), None => {} } } @@ -399,11 +522,9 @@ fn record_superlifetime(visitor: &mut RegionResolutionVisitor, fn record_var_lifetime(visitor: &mut RegionResolutionVisitor, var_id: ast::NodeId, _sp: Span) { - match visitor.cx.var_parent { - Some(parent_id) => { - let parent_scope = CodeExtent::from_node_id(parent_id); - visitor.region_maps.record_var_scope(var_id, parent_scope); - } + match visitor.cx.var_parent.to_code_extent() { + Some(parent_scope) => + visitor.region_maps.record_var_scope(var_id, parent_scope), None => { // this can happen in extern fn declarations like // @@ -415,21 +536,72 @@ fn record_var_lifetime(visitor: &mut RegionResolutionVisitor, fn resolve_block(visitor: &mut RegionResolutionVisitor, blk: &ast::Block) { debug!("resolve_block(blk.id={:?})", blk.id); - // Record the parent of this block. - record_superlifetime(visitor, blk.id, blk.span); + let prev_cx = visitor.cx; + + let blk_scope = CodeExtent::Misc(blk.id); + record_superlifetime(visitor, blk_scope, blk.span); // We treat the tail expression in the block (if any) somewhat // differently from the statements. The issue has to do with - // temporary lifetimes. If the user writes: + // temporary lifetimes. Consider the following: // - // { - // ... (&foo()) ... - // } + // quux({ + // let inner = ... (&bar()) ...; // + // (... (&foo()) ...) // (the tail expression) + // }, other_argument()); + // + // Each of the statements within the block is a terminating + // scope, and thus a temporary (e.g. the result of calling + // `bar()` in the initalizer expression for `let inner = ...;`) + // will be cleaned up immediately after its corresponding + // statement (i.e. `let inner = ...;`) executes. + // + // On the other hand, temporaries associated with evaluating the + // tail expression for the block are assigned lifetimes so that + // they will be cleaned up as part of the terminating scope + // *surrounding* the block expression. Here, the terminating + // scope for the block expression is the `quux(..)` call; so + // those temporaries will only be cleaned up *after* both + // `other_argument()` has run and also the call to `quux(..)` + // itself has returned. + + visitor.cx = Context { + var_parent: InnermostDeclaringBlock::Block(blk.id), + parent: InnermostEnclosingExpr::Some(blk.id), + }; + + { + // This block should be kept approximately in sync with + // `visit::walk_block`. (We manually walk the block, rather + // than call `walk_block`, in order to maintain precise + // `InnermostDeclaringBlock` information.) + + for (i, statement) in blk.stmts.iter().enumerate() { + if let ast::StmtDecl(_, stmt_id) = statement.node { + // Each StmtDecl introduces a subscope for bindings + // introduced by the declaration; this subscope covers + // a suffix of the block . Each subscope in a block + // has the previous subscope in the block as a parent, + // except for the first such subscope, which has the + // block itself as a parent. + let declaring = DeclaringStatementContext { + stmt_id: stmt_id, + block_id: blk.id, + stmt_index: i, + }; + record_superlifetime( + visitor, declaring.to_code_extent(), statement.span); + visitor.cx = Context { + var_parent: InnermostDeclaringBlock::Statement(declaring), + parent: InnermostEnclosingExpr::Statement(declaring), + }; + } + visitor.visit_stmt(&**statement) + } + visit::walk_expr_opt(visitor, &blk.expr) + } - let prev_cx = visitor.cx; - visitor.cx = Context {var_parent: Some(blk.id), parent: Some(blk.id)}; - visit::walk_block(visitor, blk); visitor.cx = prev_cx; } @@ -449,7 +621,7 @@ fn resolve_arm(visitor: &mut RegionResolutionVisitor, arm: &ast::Arm) { } fn resolve_pat(visitor: &mut RegionResolutionVisitor, pat: &ast::Pat) { - record_superlifetime(visitor, pat.id, pat.span); + record_superlifetime(visitor, CodeExtent::from_node_id(pat.id), pat.span); // If this is a binding (or maybe a binding, I'm too lazy to check // the def map) then record the lifetime of that binding. @@ -468,11 +640,17 @@ fn resolve_stmt(visitor: &mut RegionResolutionVisitor, stmt: &ast::Stmt) { debug!("resolve_stmt(stmt.id={:?})", stmt_id); let stmt_scope = CodeExtent::from_node_id(stmt_id); + + // Every statement will clean up the temporaries created during + // execution of that statement. Therefore each statement has an + // associated destruction scope that represents the extent of the + // statement plus its destructors, and thus the extent for which + // regions referenced by the destructors need to survive. visitor.region_maps.mark_as_terminating_scope(stmt_scope); - record_superlifetime(visitor, stmt_id, stmt.span); + record_superlifetime(visitor, stmt_scope, stmt.span); let prev_parent = visitor.cx.parent; - visitor.cx.parent = Some(stmt_id); + visitor.cx.parent = InnermostEnclosingExpr::Some(stmt_id); visit::walk_stmt(visitor, stmt); visitor.cx.parent = prev_parent; } @@ -480,10 +658,11 @@ fn resolve_stmt(visitor: &mut RegionResolutionVisitor, stmt: &ast::Stmt) { fn resolve_expr(visitor: &mut RegionResolutionVisitor, expr: &ast::Expr) { debug!("resolve_expr(expr.id={:?})", expr.id); - record_superlifetime(visitor, expr.id, expr.span); + let expr_scope = CodeExtent::Misc(expr.id); + record_superlifetime(visitor, expr_scope, expr.span); let prev_cx = visitor.cx; - visitor.cx.parent = Some(expr.id); + visitor.cx.parent = InnermostEnclosingExpr::Some(expr.id); { let region_maps = &mut visitor.region_maps; @@ -527,11 +706,11 @@ fn resolve_expr(visitor: &mut RegionResolutionVisitor, expr: &ast::Expr) { // The variable parent of everything inside (most importantly, the // pattern) is the body. - visitor.cx.var_parent = Some(body.id); + visitor.cx.var_parent = InnermostDeclaringBlock::Block(body.id); } ast::ExprMatch(..) => { - visitor.cx.var_parent = Some(expr.id); + visitor.cx.var_parent = InnermostDeclaringBlock::Match(expr.id); } ast::ExprAssignOp(..) | ast::ExprIndex(..) | @@ -568,19 +747,13 @@ fn resolve_local(visitor: &mut RegionResolutionVisitor, local: &ast::Local) { debug!("resolve_local(local.id={:?},local.init={:?})", local.id,local.init.is_some()); - let blk_id = match visitor.cx.var_parent { - Some(id) => id, - None => { - visitor.sess.span_bug( - local.span, - "local without enclosing block"); - } - }; - // For convenience in trans, associate with the local-id the var // scope that will be used for any bindings declared in this // pattern. - let blk_scope = CodeExtent::from_node_id(blk_id); + let blk_scope = visitor.cx.var_parent.to_code_extent() + .unwrap_or_else(|| visitor.sess.span_bug( + local.span, "local without enclosing block")); + visitor.region_maps.record_var_scope(local.id, blk_scope); // As an exception to the normal rules governing temporary @@ -803,7 +976,10 @@ fn resolve_local(visitor: &mut RegionResolutionVisitor, local: &ast::Local) { fn resolve_item(visitor: &mut RegionResolutionVisitor, item: &ast::Item) { // Items create a new outer block scope as far as we're concerned. let prev_cx = visitor.cx; - visitor.cx = Context {var_parent: None, parent: None}; + visitor.cx = Context { + var_parent: InnermostDeclaringBlock::None, + parent: InnermostEnclosingExpr::None + }; visit::walk_item(visitor, item); visitor.cx = prev_cx; } @@ -829,15 +1005,20 @@ fn resolve_fn(visitor: &mut RegionResolutionVisitor, let outer_cx = visitor.cx; // The arguments and `self` are parented to the body of the fn. - visitor.cx = Context { parent: Some(body.id), - var_parent: Some(body.id) }; + visitor.cx = Context { + parent: InnermostEnclosingExpr::Some(body.id), + var_parent: InnermostDeclaringBlock::Block(body.id) + }; visit::walk_fn_decl(visitor, decl); // The body of the fn itself is either a root scope (top-level fn) // or it continues with the inherited scope (closures). match fk { visit::FkItemFn(..) | visit::FkMethod(..) => { - visitor.cx = Context { parent: None, var_parent: None }; + visitor.cx = Context { + parent: InnermostEnclosingExpr::None, + var_parent: InnermostDeclaringBlock::None + }; visitor.visit_block(body); visitor.cx = outer_cx; } @@ -898,7 +1079,10 @@ pub fn resolve_crate(sess: &Session, krate: &ast::Crate) -> RegionMaps { let mut visitor = RegionResolutionVisitor { sess: sess, region_maps: &maps, - cx: Context { parent: None, var_parent: None } + cx: Context { + parent: InnermostEnclosingExpr::None, + var_parent: InnermostDeclaringBlock::None, + } }; visit::walk_crate(&mut visitor, krate); } @@ -911,7 +1095,10 @@ pub fn resolve_inlined_item(sess: &Session, let mut visitor = RegionResolutionVisitor { sess: sess, region_maps: region_maps, - cx: Context { parent: None, var_parent: None } + cx: Context { + parent: InnermostEnclosingExpr::None, + var_parent: InnermostDeclaringBlock::None + } }; visit::walk_inlined_item(&mut visitor, item); } diff --git a/src/librustc/util/ppaux.rs b/src/librustc/util/ppaux.rs index f93f5903e014d..5601898136c79 100644 --- a/src/librustc/util/ppaux.rs +++ b/src/librustc/util/ppaux.rs @@ -10,6 +10,7 @@ use middle::def; +use middle::region; use middle::subst::{VecPerParamSpace,Subst}; use middle::subst; use middle::ty::{BoundRegion, BrAnon, BrNamed}; @@ -84,37 +85,41 @@ pub fn explain_region_and_span(cx: &ctxt, region: ty::Region) -> (String, Option) { return match region { ReScope(scope) => { - match cx.map.find(scope.node_id()) { - Some(ast_map::NodeBlock(ref blk)) => { - explain_span(cx, "block", blk.span) - } - Some(ast_map::NodeExpr(expr)) => { - match expr.node { - ast::ExprCall(..) => explain_span(cx, "call", expr.span), - ast::ExprMethodCall(..) => { - explain_span(cx, "method call", expr.span) - }, - ast::ExprMatch(_, _, ast::MatchSource::IfLetDesugar { .. }) => - explain_span(cx, "if let", expr.span), - ast::ExprMatch(_, _, ast::MatchSource::WhileLetDesugar) => { - explain_span(cx, "while let", expr.span) - }, - ast::ExprMatch(..) => explain_span(cx, "match", expr.span), - _ => explain_span(cx, "expression", expr.span) - } - } - Some(ast_map::NodeStmt(stmt)) => { - explain_span(cx, "statement", stmt.span) - } - Some(ast_map::NodeItem(it)) => { - let tag = item_scope_tag(&*it); - explain_span(cx, tag, it.span) - } + let new_string; + let on_unknown_scope = |&:| { + (format!("unknown scope: {:?}. Please report a bug.", scope), None) + }; + let span = match scope.span(&cx.map) { + Some(s) => s, + None => return on_unknown_scope(), + }; + let tag = match cx.map.find(scope.node_id()) { + Some(ast_map::NodeBlock(_)) => "block", + Some(ast_map::NodeExpr(expr)) => match expr.node { + ast::ExprCall(..) => "call", + ast::ExprMethodCall(..) => "method call", + ast::ExprMatch(_, _, ast::MatchSource::IfLetDesugar { .. }) => "if let", + ast::ExprMatch(_, _, ast::MatchSource::WhileLetDesugar) => "while let", + ast::ExprMatch(..) => "match", + _ => "expression", + }, + Some(ast_map::NodeStmt(_)) => "statement", + Some(ast_map::NodeItem(it)) => item_scope_tag(&*it), Some(_) | None => { // this really should not happen - (format!("unknown scope: {:?}. Please report a bug.", scope), None) + return on_unknown_scope(); } - } + }; + let scope_decorated_tag = match scope { + region::CodeExtent::Misc(_) => tag, + region::CodeExtent::Remainder(r) => { + new_string = format!("block suffix following statement {}", + r.first_statement_index); + new_string.as_slice() + } + }; + explain_span(cx, scope_decorated_tag, span) + } ReFree(ref fr) => { @@ -867,6 +872,17 @@ impl<'tcx> Repr<'tcx> for ty::FreeRegion { } } +impl<'tcx> Repr<'tcx> for region::CodeExtent { + fn repr(&self, _tcx: &ctxt) -> String { + match *self { + region::CodeExtent::Misc(node_id) => + format!("Misc({})", node_id), + region::CodeExtent::Remainder(rem) => + format!("Remainder({}, {})", rem.block, rem.first_statement_index), + } + } +} + impl<'tcx> Repr<'tcx> for ast::DefId { fn repr(&self, tcx: &ctxt) -> String { // Unfortunately, there seems to be no way to attempt to print diff --git a/src/test/compile-fail/borrowck-let-suggestion-suffixes.rs b/src/test/compile-fail/borrowck-let-suggestion-suffixes.rs new file mode 100644 index 0000000000000..f551a2aa81155 --- /dev/null +++ b/src/test/compile-fail/borrowck-let-suggestion-suffixes.rs @@ -0,0 +1,57 @@ +// Copyright 2015 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. + +fn f() { + let old = ['o']; // statement 0 + let mut v1 = Vec::new(); // statement 1 + + let mut v2 = Vec::new(); // statement 2 + //~^ NOTE reference must be valid for the block suffix following statement 2 + + let young = ['y']; // statement 3 + //~^ NOTE ...but borrowed value is only valid for the block suffix following statement 3 + + v2.push(&young[0]); // statement 4 + //~^ ERROR `young[..]` does not live long enough + + let mut v3 = Vec::new(); // statement 5 + //~^ NOTE reference must be valid for the block suffix following statement 5 + + v3.push(&'x'); // statement 6 + //~^ ERROR borrowed value does not live long enough + //~| NOTE ...but borrowed value is only valid for the statement + //~| HELP consider using a `let` binding to increase its lifetime + + { + + let mut v4 = Vec::new(); // (sub) statement 0 + //~^ NOTE reference must be valid for the block suffix following statement 0 + + v4.push(&'y'); + //~^ ERROR borrowed value does not live long enough + //~| NOTE ...but borrowed value is only valid for the statement + //~| HELP consider using a `let` binding to increase its lifetime + + } // (statement 7) + + let mut v5 = Vec::new(); // statement 8 + //~^ NOTE reference must be valid for the block suffix following statement 8 + + v5.push(&'z'); + //~^ ERROR borrowed value does not live long enough + //~| NOTE ...but borrowed value is only valid for the statement + //~| HELP consider using a `let` binding to increase its lifetime + + v1.push(&old[0]); +} + +fn main() { + f(); +} diff --git a/src/test/compile-fail/borrowck-let-suggestion.rs b/src/test/compile-fail/borrowck-let-suggestion.rs index 5f5ff4014e109..a08021919df85 100644 --- a/src/test/compile-fail/borrowck-let-suggestion.rs +++ b/src/test/compile-fail/borrowck-let-suggestion.rs @@ -10,7 +10,7 @@ fn f() { let x = [1is].iter(); //~ ERROR borrowed value does not live long enough - //~^^ NOTE reference must be valid for the block + //~^ NOTE reference must be valid for the block suffix following statement //~^^ HELP consider using a `let` binding to increase its lifetime }