auto merge of #7455 : nikomatsakis/rust/issue-7336-constrain-closure-lifetimes, r=pnkfelix

Constrain maximum lifetime of stack closures that capture variables to be limited by the innermost repeating scope.

Fixes #7336.

r? whomever.

6 files changed, 209 insertions, 39 deletions

diff --git a/src/librustc/middle/trans/closure.rs b/src/librustc/middle/trans/closure.rs
index 5b0212cc05c..d2953f8913e 100644
--- a/src/librustc/middle/trans/closure.rs
+++ b/src/librustc/middle/trans/closure.rs
@@ -220,16 +220,24 @@ pub fn store_environment(bcx: block,
     // compute the type of the closure
     let cdata_ty = mk_closure_tys(tcx, bound_values);
 
-    // allocate closure in the heap
-    let Result {bcx: bcx, val: llbox} = allocate_cbox(bcx, sigil, cdata_ty);
-
     // cbox_ty has the form of a tuple: (a, b, c) we want a ptr to a
     // tuple.  This could be a ptr in uniq or a box or on stack,
     // whatever.
     let cbox_ty = tuplify_box_ty(tcx, cdata_ty);
     let cboxptr_ty = ty::mk_ptr(tcx, ty::mt {ty:cbox_ty, mutbl:ast::m_imm});
+    let llboxptr_ty = type_of(ccx, cboxptr_ty);
+
+    // If there are no bound values, no point in allocating anything.
+    if bound_values.is_empty() {
+        return ClosureResult {llbox: C_null(llboxptr_ty),
+                              cdata_ty: cdata_ty,
+                              bcx: bcx};
+    }
 
-    let llbox = PointerCast(bcx, llbox, type_of(ccx, cboxptr_ty));
+    // allocate closure in the heap
+    let Result {bcx: bcx, val: llbox} = allocate_cbox(bcx, sigil, cdata_ty);
+
+    let llbox = PointerCast(bcx, llbox, llboxptr_ty);
     debug!("tuplify_box_ty = %s", ty_to_str(tcx, cbox_ty));
 
     // Copy expr values into boxed bindings.
@@ -268,6 +276,7 @@ pub fn build_closure(bcx0: block,
                      sigil: ast::Sigil,
                      include_ret_handle: Option<ValueRef>) -> ClosureResult {
     let _icx = push_ctxt("closure::build_closure");
+
     // If we need to, package up the iterator body to call
     let bcx = bcx0;
 
diff --git a/src/librustc/middle/typeck/check/regionck.rs b/src/librustc/middle/typeck/check/regionck.rs
index 65b40fbd48c..6e3f0a20843 100644
--- a/src/librustc/middle/typeck/check/regionck.rs
+++ b/src/librustc/middle/typeck/check/regionck.rs
@@ -48,7 +48,10 @@ use syntax::visit;
 
 pub struct Rcx {
     fcx: @mut FnCtxt,
-    errors_reported: uint
+    errors_reported: uint,
+
+    // id of innermost fn or loop
+    repeating_scope: ast::node_id,
 }
 
 pub type rvt = visit::vt<@mut Rcx>;
@@ -78,6 +81,12 @@ impl Rcx {
         self.fcx.ccx.tcx
     }
 
+    pub fn set_repeating_scope(&mut self, scope: ast::node_id) -> ast::node_id {
+        let old_scope = self.repeating_scope;
+        self.repeating_scope = scope;
+        old_scope
+    }
+
     pub fn resolve_type(&mut self, unresolved_ty: ty::t) -> ty::t {
         /*!
          * Try to resolve the type for the given node, returning
@@ -134,7 +143,8 @@ impl Rcx {
 }
 
 pub fn regionck_expr(fcx: @mut FnCtxt, e: @ast::expr) {
-    let rcx = @mut Rcx { fcx: fcx, errors_reported: 0 };
+    let rcx = @mut Rcx { fcx: fcx, errors_reported: 0,
+                         repeating_scope: e.id };
     if fcx.err_count_since_creation() == 0 {
         // regionck assumes typeck succeeded
         let v = regionck_visitor();
@@ -144,7 +154,8 @@ pub fn regionck_expr(fcx: @mut FnCtxt, e: @ast::expr) {
 }
 
 pub fn regionck_fn(fcx: @mut FnCtxt, blk: &ast::blk) {
-    let rcx = @mut Rcx { fcx: fcx, errors_reported: 0 };
+    let rcx = @mut Rcx { fcx: fcx, errors_reported: 0,
+                         repeating_scope: blk.node.id };
     if fcx.err_count_since_creation() == 0 {
         // regionck assumes typeck succeeded
         let v = regionck_visitor();
@@ -231,7 +242,8 @@ fn constrain_bindings_in_pat(pat: @ast::pat, rcx: @mut Rcx) {
 }
 
 fn visit_expr(expr: @ast::expr, (rcx, v): (@mut Rcx, rvt)) {
-    debug!("regionck::visit_expr(e=?)");
+    debug!("regionck::visit_expr(e=%s, repeating_scope=%?)",
+           expr.repr(rcx.fcx.tcx()), rcx.repeating_scope);
 
     let has_method_map = rcx.fcx.inh.method_map.contains_key(&expr.id);
 
@@ -274,6 +286,9 @@ fn visit_expr(expr: @ast::expr, (rcx, v): (@mut Rcx, rvt)) {
                 }
             }
         }
+        ast::expr_loop(ref body, _) => {
+            tcx.region_maps.record_cleanup_scope(body.node.id);
+        }
         ast::expr_while(cond, ref body) => {
             tcx.region_maps.record_cleanup_scope(cond.id);
             tcx.region_maps.record_cleanup_scope(body.node.id);
@@ -313,10 +328,14 @@ fn visit_expr(expr: @ast::expr, (rcx, v): (@mut Rcx, rvt)) {
         ast::expr_call(callee, ref args, _) => {
             constrain_callee(rcx, callee.id, expr, callee);
             constrain_call(rcx, callee.id, expr, None, *args, false);
+
+            visit::visit_expr(expr, (rcx, v));
         }
 
         ast::expr_method_call(callee_id, arg0, _, _, ref args, _) => {
             constrain_call(rcx, callee_id, expr, Some(arg0), *args, false);
+
+            visit::visit_expr(expr, (rcx, v));
         }
 
         ast::expr_index(callee_id, lhs, rhs) |
@@ -327,23 +346,31 @@ fn visit_expr(expr: @ast::expr, (rcx, v): (@mut Rcx, rvt)) {
             // implicit "by ref" sort of passing style here.  This
             // should be converted to an adjustment!
             constrain_call(rcx, callee_id, expr, Some(lhs), [rhs], true);
+
+            visit::visit_expr(expr, (rcx, v));
         }
 
         ast::expr_unary(callee_id, _, lhs) if has_method_map => {
             // As above.
             constrain_call(rcx, callee_id, expr, Some(lhs), [], true);
+
+            visit::visit_expr(expr, (rcx, v));
         }
 
         ast::expr_unary(_, ast::deref, base) => {
             // For *a, the lifetime of a must enclose the deref
             let base_ty = rcx.resolve_node_type(base.id);
             constrain_derefs(rcx, expr, 1, base_ty);
+
+            visit::visit_expr(expr, (rcx, v));
         }
 
         ast::expr_index(_, vec_expr, _) => {
             // For a[b], the lifetime of a must enclose the deref
             let vec_type = rcx.resolve_expr_type_adjusted(vec_expr);
             constrain_index(rcx, expr, vec_type);
+
+            visit::visit_expr(expr, (rcx, v));
         }
 
         ast::expr_cast(source, _) => {
@@ -372,6 +399,8 @@ fn visit_expr(expr: @ast::expr, (rcx, v): (@mut Rcx, rvt)) {
                 }
                 _ => ()
             }
+
+            visit::visit_expr(expr, (rcx, v));
         }
 
         ast::expr_addr_of(_, base) => {
@@ -387,29 +416,87 @@ fn visit_expr(expr: @ast::expr, (rcx, v): (@mut Rcx, rvt)) {
             let ty0 = rcx.resolve_node_type(expr.id);
             constrain_regions_in_type(rcx, ty::re_scope(expr.id),
                                       infer::AddrOf(expr.span), ty0);
+            visit::visit_expr(expr, (rcx, v));
         }
 
         ast::expr_match(discr, ref arms) => {
             guarantor::for_match(rcx, discr, *arms);
+
+            visit::visit_expr(expr, (rcx, v));
+        }
+
+        ast::expr_loop_body(subexpr) => {
+            check_expr_fn_block(rcx, subexpr, v, true);
         }
 
         ast::expr_fn_block(*) => {
-            // The lifetime of a block fn must not outlive the variables
-            // it closes over
+            check_expr_fn_block(rcx, expr, v, false);
+        }
+
+        ast::expr_loop(ref body, _) => {
+            let repeating_scope = rcx.set_repeating_scope(body.node.id);
+            visit::visit_expr(expr, (rcx, v));
+            rcx.set_repeating_scope(repeating_scope);
+        }
+
+        ast::expr_while(cond, ref body) => {
+            let repeating_scope = rcx.set_repeating_scope(cond.id);
+            (v.visit_expr)(cond, (rcx, v));
+
+            rcx.set_repeating_scope(body.node.id);
+            (v.visit_block)(body, (rcx, v));
+
+            rcx.set_repeating_scope(repeating_scope);
+        }
+
+        _ => {
+            visit::visit_expr(expr, (rcx, v));
+        }
+    }
+}
+
+fn check_expr_fn_block(rcx: @mut Rcx,
+                       expr: @ast::expr,
+                       v: rvt,
+                       is_loop_body: bool) {
+    let tcx = rcx.fcx.tcx();
+    match expr.node {
+        ast::expr_fn_block(_, ref body) => {
             let function_type = rcx.resolve_node_type(expr.id);
             match ty::get(function_type).sty {
-                ty::ty_closure(ty::ClosureTy {sigil: ast::BorrowedSigil,
-                                              region: region, _}) => {
-                    constrain_free_variables(rcx, region, expr);
+                ty::ty_closure(
+                    ty::ClosureTy {
+                        sigil: ast::BorrowedSigil, region: region, _}) => {
+                    if get_freevars(tcx, expr.id).is_empty() && !is_loop_body {
+                        // No free variables means that the environment
+                        // will be NULL at runtime and hence the closure
+                        // has static lifetime.
+                    } else {
+                        // Otherwise, the closure must not outlive the
+                        // variables it closes over, nor can it
+                        // outlive the innermost repeating scope
+                        // (since otherwise that would require
+                        // infinite stack).
+                        constrain_free_variables(rcx, region, expr);
+                        let repeating_scope = ty::re_scope(rcx.repeating_scope);
+                        rcx.fcx.mk_subr(true, infer::InfStackClosure(expr.span),
+                                        region, repeating_scope);
+                    }
                 }
                 _ => ()
             }
+
+            let repeating_scope = rcx.set_repeating_scope(body.node.id);
+            visit::visit_expr(expr, (rcx, v));
+            rcx.set_repeating_scope(repeating_scope);
         }
 
-        _ => ()
+        _ => {
+            tcx.sess.span_bug(
+                expr.span,
+                "Expected expr_fn_block");
+        }
     }
-
-    visit::visit_expr(expr, (rcx, v));
 }
 
 fn constrain_callee(rcx: @mut Rcx,
diff --git a/src/librustc/middle/typeck/infer/error_reporting.rs b/src/librustc/middle/typeck/infer/error_reporting.rs
index 928f33803dd..ae8cab4c4db 100644
--- a/src/librustc/middle/typeck/infer/error_reporting.rs
+++ b/src/librustc/middle/typeck/infer/error_reporting.rs
@@ -231,6 +231,21 @@ impl ErrorReporting for InferCtxt {
                     sup,
                     "");
             }
+            infer::InfStackClosure(span) => {
+                self.tcx.sess.span_err(
+                    span,
+                    "closure outlives stack frame");
+                note_and_explain_region(
+                    self.tcx,
+                    "...the closure must be valid for ",
+                    sub,
+                    "...");
+                note_and_explain_region(
+                    self.tcx,
+                    "...but the closure's stack frame is only valid for ",
+                    sup,
+                    "");
+            }
             infer::InvokeClosure(span) => {
                 self.tcx.sess.span_err(
                     span,
diff --git a/src/librustc/middle/typeck/infer/mod.rs b/src/librustc/middle/typeck/infer/mod.rs
index d0a778139db..2a8d68c9e67 100644
--- a/src/librustc/middle/typeck/infer/mod.rs
+++ b/src/librustc/middle/typeck/infer/mod.rs
@@ -141,6 +141,10 @@ pub enum SubregionOrigin {
     // Arose from a subtyping relation
     Subtype(TypeTrace),
 
+    // Stack-allocated closures cannot outlive innermost loop
+    // or function so as to ensure we only require finite stack
+    InfStackClosure(span),
+
     // Invocation of closure must be within its lifetime
     InvokeClosure(span),
 
@@ -829,6 +833,7 @@ impl SubregionOrigin {
     pub fn span(&self) -> span {
         match *self {
             Subtype(a) => a.span(),
+            InfStackClosure(a) => a,
             InvokeClosure(a) => a,
             DerefPointer(a) => a,
             FreeVariable(a) => a,
@@ -850,6 +855,7 @@ impl Repr for SubregionOrigin {
     fn repr(&self, tcx: ty::ctxt) -> ~str {
         match *self {
             Subtype(a) => fmt!("Subtype(%s)", a.repr(tcx)),
+            InfStackClosure(a) => fmt!("InfStackClosure(%s)", a.repr(tcx)),
             InvokeClosure(a) => fmt!("InvokeClosure(%s)", a.repr(tcx)),
             DerefPointer(a) => fmt!("DerefPointer(%s)", a.repr(tcx)),
             FreeVariable(a) => fmt!("FreeVariable(%s)", a.repr(tcx)),
diff --git a/src/test/compile-fail/regionck-closure-lifetimes.rs b/src/test/compile-fail/regionck-closure-lifetimes.rs
new file mode 100644
index 00000000000..604b324cbbd
--- /dev/null
+++ b/src/test/compile-fail/regionck-closure-lifetimes.rs
@@ -0,0 +1,76 @@
+// Copyright 2013 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 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+fn env<'a>(_: &'a uint, blk: &fn(p: &'a fn())) {
+    // Test that the closure here cannot be assigned
+    // the lifetime `'a`, which outlives the current
+    // block.
+    //
+    // FIXME(#4846): The `&'a uint` parameter is needed to ensure that `'a`
+    // is a free and not bound region name.
+
+    let mut state = 0;
+    let statep = &mut state;
+    blk(|| *statep = 1); //~ ERROR cannot infer an appropriate lifetime
+}
+
+fn no_env_no_for<'a>(_: &'a uint, blk: &fn(p: &'a fn())) {
+    // Test that a closure with no free variables CAN
+    // outlive the block in which it is created.
+    //
+    // FIXME(#4846): The `&'a uint` parameter is needed to ensure that `'a`
+    // is a free and not bound region name.
+
+    blk(|| ())
+}
+
+fn no_env_but_for<'a>(_: &'a uint, blk: &fn(p: &'a fn() -> bool) -> bool) {
+    // Test that a `for` loop is considered to hvae
+    // implicit free variables.
+    //
+    // FIXME(#4846): The `&'a uint` parameter is needed to ensure that `'a`
+    // is a free and not bound region name.
+
+    for blk { } //~ ERROR cannot infer an appropriate lifetime
+}
+
+fn repeating_loop() {
+    // Test that the closure cannot be created within `loop` loop and
+    // called without, even though the state that it closes over is
+    // external to the loop.
+
+    let closure;
+    let state = 0;
+
+    loop {
+        closure = || state; //~ ERROR cannot infer an appropriate lifetime
+        break;
+    }
+
+    closure();
+}
+
+fn repeating_while() {
+    // Test that the closure cannot be created within `while` loop and
+    // called without, even though the state that it closes over is
+    // external to the loop.
+
+    let closure;
+    let state = 0;
+
+    while true {
+        closure = || state; //~ ERROR cannot infer an appropriate lifetime
+        break;
+    }
+
+    closure();
+}
+
+fn main() {}
diff --git a/src/test/run-pass/unconstrained-region.rs b/src/test/run-pass/unconstrained-region.rs
deleted file mode 100644
index 2341ee8d100..00000000000
--- a/src/test/run-pass/unconstrained-region.rs
+++ /dev/null
@@ -1,23 +0,0 @@
-// Copyright 2013 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 <LICENSE-APACHE or
-// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
-// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
-// option. This file may not be copied, modified, or distributed
-// except according to those terms.
-
-// xfail-test
-// FIXME: #7336: codegen bug makes this segfault on Linux x86_64
-
-fn foo<'a>(blk: &fn(p: &'a fn() -> &'a fn())) {
-    let mut state = 0;
-    let statep = &mut state;
-    do blk {
-        || { *statep = 1; }
-    }
-}
-fn main() {
-    do foo |p| { p()() }
-}