make it illegal to implicitly capture mutable variables

this is the final part of #1273

src/rustc/front/test.rs

@@ -368,7 +368,7 @@ fn mk_test_wrapper(cx: test_ctxt,
     let wrapper_expr: ast::expr = {
         id: cx.sess.next_node_id(),
         node: ast::expr_fn(ast::proto_bare, wrapper_decl,
-                           wrapper_body, []),
+                           wrapper_body, @[]),
         span: span
     };
 

src/rustc/metadata/astencode.rs

@@ -223,8 +223,7 @@ fn visit_ids(item: ast::inlined_item, vfn: fn@(ast::node_id)) {
                 vfn(m.self_id);
                 vec::iter(tps) {|tp| vfn(tp.id)}
               }
-              visit::fk_anon(_) |
-              visit::fk_fn_block {
+              visit::fk_anon(*) | visit::fk_fn_block(*) {
               }
             }
 

src/rustc/middle/capture.rs

@@ -31,12 +31,11 @@ type capture_map = map::hashmap<ast::def_id, capture_var>;
 // errors for any irregularities which we identify.
 fn check_capture_clause(tcx: ty::ctxt,
                         fn_expr_id: ast::node_id,
-                        fn_proto: ast::proto,
                         cap_clause: ast::capture_clause) {
     let freevars = freevars::get_freevars(tcx, fn_expr_id);
     let seen_defs = map::int_hash();
 
-    let check_capture_item = fn@(cap_item: ast::capture_item) {
+    for (*cap_clause).each { |cap_item|
         let cap_def = tcx.def_map.get(cap_item.id);
         if !vec::any(*freevars, {|fv| fv.def == cap_def}) {
             tcx.sess.span_warn(
@@ -52,22 +51,6 @@ fn check_capture_clause(tcx: ty::ctxt,
                 #fmt("variable '%s' captured more than once",
                      cap_item.name));
         }
-    };
-
-    alt fn_proto {
-      ast::proto_any | ast::proto_block {
-        if vec::is_not_empty(cap_clause) {
-            let cap_item0 = vec::head(cap_clause);
-            tcx.sess.span_err(
-                cap_item0.span,
-                "cannot capture values explicitly with a block closure");
-        }
-      }
-      ast::proto_bare | ast::proto_box | ast::proto_uniq {
-        for cap_clause.each { |cap_item|
-            check_capture_item(cap_item);
-        }
-      }
     }
 }
 
@@ -80,7 +63,7 @@ fn compute_capture_vars(tcx: ty::ctxt,
 
     // first add entries for anything explicitly named in the cap clause
 
-    for cap_clause.each { |cap_item|
+    for (*cap_clause).each { |cap_item|
         let cap_def = tcx.def_map.get(cap_item.id);
         let cap_def_id = ast_util::def_id_of_def(cap_def).node;
         if cap_item.is_move {

src/rustc/middle/kind.rs

@@ -5,6 +5,7 @@ import ty::{kind, kind_copyable, kind_sendable, kind_noncopyable};
 import driver::session::session;
 import std::map::hashmap;
 import syntax::print::pprust::expr_to_str;
+import freevars::freevar_entry;
 
 // Kind analysis pass. There are three kinds:
 //
@@ -56,17 +57,61 @@ fn check_crate(tcx: ty::ctxt, method_map: typeck::method_map,
     ret ctx.rval_map;
 }
 
+type check_fn = fn@(ctx, option<@freevar_entry>, bool, ty::t, sp: span);
+
 // Yields the appropriate function to check the kind of closed over
 // variables. `id` is the node_id for some expression that creates the
 // closure.
-fn with_appropriate_checker(cx: ctx, id: node_id,
-                            b: fn(fn@(ctx, ty::t, sp: span))) {
+fn with_appropriate_checker(cx: ctx, id: node_id, b: fn(check_fn)) {
+    fn check_for_uniq(cx: ctx, fv: option<@freevar_entry>, is_move: bool,
+                      var_t: ty::t, sp: span) {
+        // all captured data must be sendable, regardless of whether it is
+        // moved in or copied in
+        check_send(cx, var_t, sp);
+
+        // check that only immutable variables are implicitly copied in
+        if !is_move {
+            for fv.each { |fv|
+                check_imm_free_var(cx, fv.def, fv.span);
+            }
+        }
+    }
+
+    fn check_for_box(cx: ctx, fv: option<@freevar_entry>, is_move: bool,
+                     var_t: ty::t, sp: span) {
+        // copied in data must be copyable, but moved in data can be anything
+        if !is_move { check_copy(cx, var_t, sp); }
+
+        // check that only immutable variables are implicitly copied in
+        if !is_move {
+            for fv.each { |fv|
+                check_imm_free_var(cx, fv.def, fv.span);
+            }
+        }
+    }
+
+    fn check_for_block(cx: ctx, fv: option<@freevar_entry>, _is_move: bool,
+                       _var_t: ty::t, sp: span) {
+        // only restriction: no capture clauses (we would have to take
+        // ownership of the moved/copied in data).
+        if fv.is_none() {
+            cx.tcx.sess.span_err(
+                sp,
+                "cannot capture values explicitly with a block closure");
+        }
+    }
+
+    fn check_for_bare(cx: ctx, _fv: option<@freevar_entry>, _is_move: bool,
+                      _var_t: ty::t, sp: span) {
+        cx.tcx.sess.span_err(sp, "attempted dynamic environment capture");
+    }
+
     let fty = ty::node_id_to_type(cx.tcx, id);
     alt ty::ty_fn_proto(fty) {
-      proto_uniq { b(check_send); }
-      proto_box { b(check_copy); }
-      proto_bare { b(check_none); }
-      proto_any | proto_block { /* no check needed */ }
+      proto_uniq { b(check_for_uniq) }
+      proto_box { b(check_for_box) }
+      proto_bare { b(check_for_bare) }
+      proto_any | proto_block { b(check_for_block) }
     }
 }
 
@@ -75,59 +120,54 @@ fn with_appropriate_checker(cx: ctx, id: node_id,
 fn check_fn(fk: visit::fn_kind, decl: fn_decl, body: blk, sp: span,
             fn_id: node_id, cx: ctx, v: visit::vt<ctx>) {
 
-    // n.b.: This could be the body of either a fn decl or a fn expr.  In the
-    // former case, the prototype will be proto_bare and no check occurs.  In
-    // the latter case, we do not check the variables that in the capture
-    // clause (as we don't have access to that here) but just those that
-    // appear free.  The capture clauses are checked below, in check_expr().
-    //
-    // We could do this check also in check_expr(), but it seems more
-    // "future-proof" to do it this way, as check_fn_body() is supposed to be
-    // the common flow point for all functions that appear in the AST.
+    // Find the check function that enforces the appropriate bounds for this
+    // kind of function:
+    with_appropriate_checker(cx, fn_id) { |chk|
 
-    with_appropriate_checker(cx, fn_id) { |checker|
+        // Begin by checking the variables in the capture clause, if any.
+        // Here we slightly abuse the map function to both check and report
+        // errors and produce a list of the def id's for all capture
+        // variables.  This list is used below to avoid checking and reporting
+        // on a given variable twice.
+        let cap_clause = alt fk {
+          visit::fk_anon(_, cc) | visit::fk_fn_block(cc) { cc }
+          visit::fk_item_fn(*) | visit::fk_method(*) |
+          visit::fk_res(*) | visit::fk_ctor(*) { @[] }
+        };
+        let captured_vars = (*cap_clause).map { |cap_item|
+            let cap_def = cx.tcx.def_map.get(cap_item.id);
+            let cap_def_id = ast_util::def_id_of_def(cap_def).node;
+            let ty = ty::node_id_to_type(cx.tcx, cap_def_id);
+            chk(cx, none, cap_item.is_move, ty, cap_item.span);
+            cap_def_id
+        };
+
+        // Iterate over any free variables that may not have appeared in the
+        // capture list.  Ensure that they too are of the appropriate kind.
         for vec::each(*freevars::get_freevars(cx.tcx, fn_id)) {|fv|
             let id = ast_util::def_id_of_def(fv.def).node;
-            if checker == check_copy {
+
+            // skip over free variables that appear in the cap clause
+            if captured_vars.contains(id) { cont; }
+
+            // if this is the last use of the variable, then it will be
+            // a move and not a copy
+            let is_move = {
                 let last_uses = alt check cx.last_uses.find(fn_id) {
                   some(last_use::closes_over(vars)) { vars }
                   none { [] }
                 };
-                if option::is_some(
-                    vec::position_elem(last_uses, id)) { cont; }
-            }
+                last_uses.contains(id)
+            };
+
             let ty = ty::node_id_to_type(cx.tcx, id);
-            checker(cx, ty, fv.span);
+            chk(cx, some(fv), is_move, ty, fv.span);
         }
     }
 
     visit::visit_fn(fk, decl, body, sp, fn_id, cx, v);
 }
 
-fn check_fn_cap_clause(cx: ctx,
-                       id: node_id,
-                       cap_clause: capture_clause) {
-    // Check that the variables named in the clause which are not free vars
-    // (if any) are also legal.  freevars are checked above in check_fn().
-    // This is kind of a degenerate case, as captured variables will generally
-    // appear free in the body.
-    let freevars = freevars::get_freevars(cx.tcx, id);
-    let freevar_ids = vec::map(*freevars, { |freevar|
-        ast_util::def_id_of_def(freevar.def).node
-    });
-    //log("freevar_ids", freevar_ids);
-    with_appropriate_checker(cx, id) { |checker|
-        for cap_clause.each { |cap_item|
-            let cap_def = cx.tcx.def_map.get(cap_item.id);
-            let cap_def_id = ast_util::def_id_of_def(cap_def).node;
-            if !vec::contains(freevar_ids, cap_def_id) {
-                let ty = ty::node_id_to_type(cx.tcx, cap_def_id);
-                checker(cx, ty, cap_item.span);
-            }
-        }
-    }
-}
-
 fn check_block(b: blk, cx: ctx, v: visit::vt<ctx>) {
     alt b.node.expr {
       some(ex) { maybe_copy(cx, ex); }
@@ -225,9 +265,6 @@ fn check_expr(e: @expr, cx: ctx, v: visit::vt<ctx>) {
             }
         }
       }
-      expr_fn(_, _, _, cap_clause) | expr_fn_block(_, _, cap_clause) {
-        check_fn_cap_clause(cx, e.id, cap_clause);
-      }
       _ { }
     }
     visit::visit_expr(e, cx, v);
@@ -307,6 +344,35 @@ fn check_copy_ex(cx: ctx, ex: @expr, _warn: bool) {
     }
 }
 
+fn check_imm_free_var(cx: ctx, def: def, sp: span) {
+    let msg = "mutable variables cannot be implicitly captured; \
+               use a capture clause";
+    alt def {
+      def_local(_, is_mutbl) {
+        if is_mutbl {
+            cx.tcx.sess.span_err(sp, msg);
+        }
+      }
+      def_arg(_, mode) {
+        alt ty::resolved_mode(cx.tcx, mode) {
+          by_ref | by_val { /* ok */ }
+          by_mutbl_ref | by_move | by_copy {
+            cx.tcx.sess.span_err(sp, msg);
+          }
+        }
+      }
+      def_upvar(_, def1, _) {
+        check_imm_free_var(cx, *def1, sp);
+      }
+      def_binding(*) | def_self(*) { /*ok*/ }
+      _ {
+        cx.tcx.sess.span_bug(
+            sp,
+            #fmt["unknown def for free variable: %?", def]);
+      }
+    }
+}
+
 fn check_copy(cx: ctx, ty: ty::t, sp: span) {
     if !ty::kind_can_be_copied(ty::type_kind(cx.tcx, ty)) {
         cx.tcx.sess.span_err(sp, "copying a noncopyable value");
@@ -319,10 +385,6 @@ fn check_send(cx: ctx, ty: ty::t, sp: span) {
     }
 }
 
-fn check_none(cx: ctx, _ty: ty::t, sp: span) {
-    cx.tcx.sess.span_err(sp, "attempted dynamic environment capture");
-}
-
 //
 // Local Variables:
 // mode: rust

src/rustc/middle/last_use.rs

@@ -159,7 +159,7 @@ fn visit_expr(ex: @expr, cx: ctx, v: visit::vt<ctx>) {
         // n.b.: safe to ignore copies, as if they are unused
         // then they are ignored, otherwise they will show up
         // as freevars in the body.
-        for cap_clause.each { |ci|
+        for (*cap_clause).each { |ci|
             if ci.is_move {
                 clear_def_if_local(cx, cx.def_map.get(ci.id), false);
             }

src/rustc/middle/mutbl.rs

@@ -200,7 +200,7 @@ fn visit_expr(ex: @expr, &&cx: @ctx, v: visit::vt<@ctx>) {
         check_lval(cx, dest, msg_assign);
       }
       expr_fn(_, _, _, cap_clause) | expr_fn_block(_, _, cap_clause) {
-        for cap_clause.each { |cap_item|
+        for (*cap_clause).each { |cap_item|
             if cap_item.is_move {
                 let def = cx.tcx.def_map.get(cap_item.id);
                 alt is_illegal_to_modify_def(cx, def, msg_move_out) {

src/rustc/middle/resolve.rs

@@ -455,7 +455,7 @@ fn resolve_names(e: @env, c: @ast::crate) {
           }
           ast::expr_fn(_, _, _, cap_clause) |
           ast::expr_fn_block(_, _, cap_clause) {
-            for cap_clause.each { |ci|
+            for (*cap_clause).each { |ci|
                 resolve_capture_item(e, sc, ci);
             }
           }
@@ -615,10 +615,12 @@ fn visit_fn_with_scope(e: @env, fk: visit::fn_kind, decl: ast::fn_decl,
     for decl.constraints.each {|c| resolve_constr(e, c, sc, v); }
     let scope = alt fk {
       visit::fk_item_fn(_, tps) | visit::fk_res(_, tps, _) |
-      visit::fk_method(_, tps, _) | visit::fk_ctor(_, tps, _, _)
-         { scope_bare_fn(decl, id, tps) }
-      visit::fk_anon(ast::proto_bare) { scope_bare_fn(decl, id, []) }
-      visit::fk_anon(_) | visit::fk_fn_block { scope_fn_expr(decl, id, []) }
+      visit::fk_method(_, tps, _) | visit::fk_ctor(_, tps, _, _) {
+        scope_bare_fn(decl, id, tps) }
+      visit::fk_anon(ast::proto_bare, _) {
+        scope_bare_fn(decl, id, []) }
+      visit::fk_anon(_, _) | visit::fk_fn_block(_) {
+        scope_fn_expr(decl, id, []) }
     };
 
     visit::visit_fn(fk, decl, body, sp, id, cons(scope, @sc), v);

src/rustc/middle/tstate/pre_post_conditions.rs

@@ -343,12 +343,12 @@ fn find_pre_post_expr(fcx: fn_ctxt, e: @expr) {
       expr_fn(_, _, _, cap_clause) | expr_fn_block(_, _, cap_clause) {
         find_pre_post_expr_fn_upvars(fcx, e);
 
-        for cap_clause.each { |cap_item|
+        for (*cap_clause).each { |cap_item|
             let d = local_node_id_to_local_def_id(fcx, cap_item.id);
             option::iter(d, { |id| use_var(fcx, id) });
         }
 
-        for cap_clause.each { |cap_item|
+        for (*cap_clause).each { |cap_item|
             if cap_item.is_move {
                 log(debug, ("forget_in_postcond: ", cap_item));
                 forget_in_postcond(fcx, e.id, cap_item.id);

src/rustc/middle/tstate/states.rs

@@ -363,7 +363,7 @@ fn find_pre_post_state_cap_clause(fcx: fn_ctxt, e_id: node_id,
     let ccx = fcx.ccx;
     let pres_changed = set_prestate_ann(ccx, e_id, pres);
     let post = tritv_clone(pres);
-    for cap_clause.each { |cap_item|
+    for (*cap_clause).each { |cap_item|
         if cap_item.is_move {
             forget_in_poststate(fcx, post, cap_item.id);
         }

src/rustc/middle/typeck.rs

@@ -3565,7 +3565,7 @@ fn check_expr_with_unifier(fcx: @fn_ctxt,
       }
       ast::expr_fn(proto, decl, body, cap_clause) {
         check_expr_fn(fcx, expr, proto, decl, body, false, expected);
-        capture::check_capture_clause(tcx, expr.id, proto, cap_clause);
+        capture::check_capture_clause(tcx, expr.id, cap_clause);
       }
       ast::expr_fn_block(decl, body, cap_clause) {
         // Take the prototype from the expected type, but default to block:
@@ -3573,7 +3573,7 @@ fn check_expr_with_unifier(fcx: @fn_ctxt,
             alt sty { ty::ty_fn({proto, _}) { some(proto) } _ { none } }
         }).get_default(ast::proto_box);
         check_expr_fn(fcx, expr, proto, decl, body, false, expected);
-        capture::check_capture_clause(tcx, expr.id, proto, cap_clause);
+        capture::check_capture_clause(tcx, expr.id, cap_clause);
       }
       ast::expr_loop_body(b) {
         // a loop body is the special argument to a `for` loop.  We know that
@@ -3605,7 +3605,7 @@ fn check_expr_with_unifier(fcx: @fn_ctxt,
           ast::expr_fn_block(decl, body, cap_clause) {
             check_expr_fn(fcx, b, proto, decl, body, true, some(inner_ty));
             demand::suptype(fcx, b.span, inner_ty, fcx.expr_ty(b));
-            capture::check_capture_clause(tcx, b.id, proto, cap_clause);
+            capture::check_capture_clause(tcx, b.id, cap_clause);
           }
         }
         let block_ty = structurally_resolved_type(