To handle more complex cases, modify the deferred call handler to be

specialized to closures, and invoke them as soon as we know the
closure kind. I thought initially we would need a fixed-point
inference algorithm but it appears I was mistaken, so we can do this.

Author: nikomatsakis

src/librustc_typeck/check/callee.rs

@@ -14,7 +14,7 @@ use super::check_argument_types;
 use super::check_expr;
 use super::check_method_argument_types;
 use super::demand;
-use super::DeferredResolution;
+use super::DeferredCallResolution;
 use super::err_args;
 use super::Expectation;
 use super::expected_types_for_fn_args;
@@ -99,8 +99,8 @@ pub fn check_call<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>,
             confirm_builtin_call(fcx, call_expr, callee_ty, arg_exprs, expected);
         }
 
-        Some(CallStep::Closure(fn_sig)) => {
-            confirm_closure_call(fcx, call_expr, arg_exprs, expected, fn_sig);
+        Some(CallStep::DeferredClosure(fn_sig)) => {
+            confirm_deferred_closure_call(fcx, call_expr, arg_exprs, expected, fn_sig);
         }
 
         Some(CallStep::Overloaded(method_callee)) => {
@@ -112,7 +112,7 @@ pub fn check_call<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>,
 
 enum CallStep<'tcx> {
     Builtin,
-    Closure(ty::FnSig<'tcx>),
+    DeferredClosure(ty::FnSig<'tcx>),
     Overloaded(ty::MethodCallee<'tcx>)
 }
 
@@ -138,21 +138,28 @@ fn try_overloaded_call_step<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>,
         }
 
         ty::ty_closure(def_id, _, substs) => {
-            let closure_ty =
-                fcx.closure_type(def_id, substs);
-            let fn_sig =
-                fcx.infcx().replace_late_bound_regions_with_fresh_var(call_expr.span,
-                                                                      infer::FnCall,
-                                                                      &closure_ty.sig).0;
-            fcx.record_deferred_resolution(box CallResolution {
-                call_expr: call_expr,
-                callee_expr: callee_expr,
-                adjusted_ty: adjusted_ty,
-                autoderefref: autoderefref,
-                fn_sig: fn_sig.clone(),
-                closure_def_id: def_id,
-            });
-            return Some(CallStep::Closure(fn_sig));
+            assert_eq!(def_id.krate, ast::LOCAL_CRATE);
+
+            // Check whether this is a call to a closure where we
+            // haven't yet decided on whether the closure is fn vs
+            // fnmut vs fnonce. If so, we have to defer further processing.
+            if fcx.closure_kind(def_id).is_none() {
+                let closure_ty =
+                    fcx.closure_type(def_id, substs);
+                let fn_sig =
+                    fcx.infcx().replace_late_bound_regions_with_fresh_var(call_expr.span,
+                                                                          infer::FnCall,
+                                                                          &closure_ty.sig).0;
+                fcx.record_deferred_call_resolution(
+                    def_id,
+                    box CallResolution {call_expr: call_expr,
+                                        callee_expr: callee_expr,
+                                        adjusted_ty: adjusted_ty,
+                                        autoderefref: autoderefref,
+                                        fn_sig: fn_sig.clone(),
+                                        closure_def_id: def_id});
+                return Some(CallStep::DeferredClosure(fn_sig));
+            }
         }
 
         _ => {}
@@ -258,11 +265,11 @@ fn confirm_builtin_call<'a,'tcx>(fcx: &FnCtxt<'a,'tcx>,
     write_call(fcx, call_expr, fn_sig.output);
 }
 
-fn confirm_closure_call<'a,'tcx>(fcx: &FnCtxt<'a,'tcx>,
-                                 call_expr: &ast::Expr,
-                                 arg_exprs: &'tcx [P<ast::Expr>],
-                                 expected: Expectation<'tcx>,
-                                 fn_sig: ty::FnSig<'tcx>)
+fn confirm_deferred_closure_call<'a,'tcx>(fcx: &FnCtxt<'a,'tcx>,
+                                          call_expr: &ast::Expr,
+                                          arg_exprs: &'tcx [P<ast::Expr>],
+                                          expected: Expectation<'tcx>,
+                                          fn_sig: ty::FnSig<'tcx>)
 {
     // `fn_sig` is the *signature* of the cosure being called. We
     // don't know the full details yet (`Fn` vs `FnMut` etc), but we
@@ -338,22 +345,18 @@ impl<'tcx> Repr<'tcx> for CallResolution<'tcx> {
     }
 }
 
-impl<'tcx> DeferredResolution<'tcx> for CallResolution<'tcx> {
-    fn attempt_resolution<'a>(&self, fcx: &FnCtxt<'a,'tcx>) -> bool {
-        debug!("attempt_resolution() {}",
+impl<'tcx> DeferredCallResolution<'tcx> for CallResolution<'tcx> {
+    fn resolve<'a>(&mut self, fcx: &FnCtxt<'a,'tcx>) {
+        debug!("DeferredCallResolution::resolve() {}",
                self.repr(fcx.tcx()));
 
-        match fcx.closure_kind(self.closure_def_id) {
-            Some(_) => { }
-            None => {
-                return false;
-            }
-        }
+        // we should not be invoked until the closure kind has been
+        // determined by upvar inference
+        assert!(fcx.closure_kind(self.closure_def_id).is_some());
 
         // We may now know enough to figure out fn vs fnmut etc.
         match try_overloaded_call_traits(fcx, self.call_expr, self.callee_expr,
                                          self.adjusted_ty, self.autoderefref.clone()) {
-            None => false,
             Some(method_callee) => {
                 // One problem is that when we get here, we are going
                 // to have a newly instantiated function signature
@@ -382,8 +385,11 @@ impl<'tcx> DeferredResolution<'tcx> for CallResolution<'tcx> {
                                self.fn_sig.output.unwrap());
 
                 write_overloaded_call_method_map(fcx, self.call_expr, method_callee);
-
-                true
+            }
+            None => {
+                fcx.tcx().sess.span_bug(
+                    self.call_expr.span,
+                    "failed to find an overloaded call trait for closure call");
             }
         }
     }

src/librustc_typeck/check/mod.rs

@@ -110,6 +110,7 @@ use util::nodemap::{DefIdMap, FnvHashMap, NodeMap};
 use util::lev_distance::lev_distance;
 
 use std::cell::{Cell, Ref, RefCell};
+use std::collections::hash_map::Entry::{Occupied, Vacant};
 use std::mem::replace;
 use std::rc::Rc;
 use std::iter::repeat;
@@ -172,15 +173,21 @@ pub struct Inherited<'a, 'tcx: 'a> {
     // Tracks trait obligations incurred during this function body.
     fulfillment_cx: RefCell<traits::FulfillmentContext<'tcx>>,
 
-    //
-    deferred_resolutions: RefCell<Vec<DeferredResolutionHandler<'tcx>>>,
+    // When we process a call like `c()` where `c` is a closure type,
+    // we may not have decided yet whether `c` is a `Fn`, `FnMut`, or
+    // `FnOnce` closure. In that case, we defer full resolution of the
+    // call until upvar inference can kick in and make the
+    // decision. We keep these deferred resolutions sorted by the
+    // def-id of the closure, so that once we decide, we can easily go
+    // back and process them.
+    deferred_call_resolutions: RefCell<DefIdMap<Vec<DeferredCallResolutionHandler<'tcx>>>>,
 }
 
-trait DeferredResolution<'tcx> {
-    fn attempt_resolution<'a>(&self, fcx: &FnCtxt<'a,'tcx>) -> bool;
+trait DeferredCallResolution<'tcx> {
+    fn resolve<'a>(&mut self, fcx: &FnCtxt<'a,'tcx>);
 }
 
-type DeferredResolutionHandler<'tcx> = Box<DeferredResolution<'tcx>+'tcx>;
+type DeferredCallResolutionHandler<'tcx> = Box<DeferredCallResolution<'tcx>+'tcx>;
 
 /// When type-checking an expression, we propagate downward
 /// whatever type hint we are able in the form of an `Expectation`.
@@ -391,7 +398,7 @@ impl<'a, 'tcx> Inherited<'a, 'tcx> {
             closure_kinds: RefCell::new(DefIdMap()),
             fn_sig_map: RefCell::new(NodeMap()),
             fulfillment_cx: RefCell::new(traits::FulfillmentContext::new()),
-            deferred_resolutions: RefCell::new(Vec::new()),
+            deferred_call_resolutions: RefCell::new(DefIdMap()),
         }
     }
 
@@ -1295,8 +1302,23 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         if ty::type_has_ty_infer(t) || ty::type_is_error(t) { Err(()) } else { Ok(t) }
     }
 
-    fn record_deferred_resolution(&self, r: DeferredResolutionHandler<'tcx>) {
-        self.inh.deferred_resolutions.borrow_mut().push(r);
+    fn record_deferred_call_resolution(&self,
+                                       closure_def_id: ast::DefId,
+                                       r: DeferredCallResolutionHandler<'tcx>) {
+        let mut deferred_call_resolutions = self.inh.deferred_call_resolutions.borrow_mut();
+        let mut vec = match deferred_call_resolutions.entry(closure_def_id) {
+            Occupied(entry) => entry.into_mut(),
+            Vacant(entry) => entry.insert(Vec::new()),
+        };
+        vec.push(r);
+    }
+
+    fn remove_deferred_call_resolutions(&self,
+                                        closure_def_id: ast::DefId)
+                                        -> Vec<DeferredCallResolutionHandler<'tcx>>
+    {
+        let mut deferred_call_resolutions = self.inh.deferred_call_resolutions.borrow_mut();
+        deferred_call_resolutions.remove(&closure_def_id).unwrap_or(Vec::new())
     }
 
     pub fn tag(&self) -> String {

src/librustc_typeck/check/upvar.rs

@@ -67,6 +67,9 @@ pub fn closure_analyze_fn(fcx: &FnCtxt,
 
     let mut adjust = AdjustBorrowKind::new(fcx, &closures_with_inferred_kinds);
     adjust.visit_block(body);
+
+    // it's our job to process these.
+    assert!(fcx.inh.deferred_call_resolutions.borrow().is_empty());
 }
 
 ///////////////////////////////////////////////////////////////////////////
@@ -186,10 +189,56 @@ impl<'a,'tcx> AdjustBorrowKind<'a,'tcx> {
 
         self.visit_block(body);
 
-        debug!("analyzing fn body with id {}", body.id);
+        debug!("analyzing closure `{}` with fn body id `{}`", id, body.id);
 
         let mut euv = euv::ExprUseVisitor::new(self, self.fcx);
         euv.walk_fn(decl, body);
+
+        // If we had not yet settled on a closure kind for this closure,
+        // then we should have by now. Process and remove any deferred resolutions.
+        //
+        // Interesting fact: all calls to this closure must come
+        // *after* its definition.  Initially, I thought that some
+        // kind of fixed-point iteration would be required, due to the
+        // possibility of twisted examples like this one:
+        //
+        // ```rust
+        // let mut closure0 = None;
+        // let vec = vec!(1, 2, 3);
+        //
+        // loop {
+        //     {
+        //         let closure1 = || {
+        //             match closure0.take() {
+        //                 Some(c) => {
+        //                     return c(); // (*) call to `closure0` before it is defined
+        //                 }
+        //                 None => { }
+        //             }
+        //         };
+        //         closure1();
+        //     }
+        //
+        //     closure0 = || vec;
+        // }
+        // ```
+        //
+        // However, this turns out to be wrong. Examples like this
+        // fail to compile because the type of the variable `c` above
+        // is an inference variable.  And in fact since closure types
+        // cannot be written, there is no way to make this example
+        // work without a boxed closure. This implies that we can't
+        // have two closures that recursively call one another without
+        // some form of boxing (and hence explicit writing of a
+        // closure kind) involved. Huzzah. -nmatsakis
+        let closure_def_id = ast_util::local_def(id);
+        if self.closures_with_inferred_kinds.contains(&id) {
+            let mut deferred_call_resolutions =
+                self.fcx.remove_deferred_call_resolutions(closure_def_id);
+            for deferred_call_resolution in deferred_call_resolutions.iter_mut() {
+                deferred_call_resolution.resolve(self.fcx);
+            }
+        }
     }
 
     fn adjust_upvar_borrow_kind_for_consume(&self,

src/librustc_typeck/check/vtable.rs

@@ -280,8 +280,6 @@ fn check_object_type_binds_all_associated_types<'tcx>(tcx: &ty::ctxt<'tcx>,
 pub fn select_all_fcx_obligations_and_apply_defaults(fcx: &FnCtxt) {
     debug!("select_all_fcx_obligations_and_apply_defaults");
 
-    fcx.inh.deferred_resolutions.borrow_mut()
-                                .retain(|r| !r.attempt_resolution(fcx));
     select_fcx_obligations_where_possible(fcx);
     fcx.default_type_parameters();
     select_fcx_obligations_where_possible(fcx);
@@ -290,6 +288,10 @@ pub fn select_all_fcx_obligations_and_apply_defaults(fcx: &FnCtxt) {
 pub fn select_all_fcx_obligations_or_error(fcx: &FnCtxt) {
     debug!("select_all_fcx_obligations_or_error");
 
+    // upvar inference should have ensured that all deferrred call
+    // resolutions are handled by now.
+    assert!(fcx.inh.deferred_call_resolutions.borrow().is_empty());
+
     select_all_fcx_obligations_and_apply_defaults(fcx);
     let mut fulfillment_cx = fcx.inh.fulfillment_cx.borrow_mut();
     let r = fulfillment_cx.select_all_or_error(fcx.infcx(), fcx);

src/test/compile-fail/unboxed-closures-failed-recursive-fn-1.rs

@@ -0,0 +1,33 @@
+// 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 <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.
+
+// Various unsuccessful attempts to put the unboxed closure kind
+// inference into an awkward position that might require fixed point
+// iteration (basically where inferring the kind of a closure `c`
+// would require knowing the kind of `c`). I currently believe this is
+// impossible.
+
+fn a() {
+    // This case of recursion wouldn't even require fixed-point
+    // iteration, but it still doesn't work. The weird structure with
+    // the `Option` is to avoid giving any useful hints about the `Fn`
+    // kind via the expected type.
+    let mut factorial: Option<Box<Fn(u32) -> u32>> = None;
+
+    let f = |x: u32| -> u32 {
+        let g = factorial.as_ref().unwrap();
+        if x == 0 {1} else {x * g(x-1)}
+    };
+
+    factorial = Some(Box::new(f));
+    //~^ ERROR cannot assign to `factorial` because it is borrowed
+}
+
+fn main() { }

src/test/compile-fail/unboxed-closures-failed-recursive-fn-2.rs

@@ -0,0 +1,39 @@
+// 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 <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.
+
+// Various unsuccessful attempts to put the unboxed closure kind
+// inference into an awkward position that might require fixed point
+// iteration (basically where inferring the kind of a closure `c`
+// would require knowing the kind of `c`). I currently believe this is
+// impossible.
+
+fn a() {
+    let mut closure0 = None;
+    let vec = vec!(1, 2, 3);
+
+    loop {
+        {
+            let closure1 = || {
+                match closure0.take() {
+                    Some(c) => {
+                        return c();
+                        //~^ ERROR the type of this value must be known in this context
+                    }
+                    None => { }
+                }
+            };
+            closure1();
+        }
+
+        closure0 = || vec;
+    }
+}
+
+fn main() { }

src/test/compile-fail/unboxed-closures-infer-fnmut-calling-fnmut-no-mut.rs

@@ -0,0 +1,32 @@
+// 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 <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.
+
+// Test that we are able to infer a suitable kind for this closure
+// that is just called (`FnMut`).
+
+fn main() {
+    let mut counter = 0;
+
+    // Here this must be inferred to FnMut so that it can mutate counter,
+    // but we forgot the mut.
+    let tick1 = || {
+        counter += 1;
+    };
+
+    // In turn, tick2 must be inferred to FnMut so that it can call
+    // tick1, but we forgot the mut. The error message we currently
+    // get seems... suboptimal.
+    let tick2 = || { //~ ERROR closure cannot assign to immutable local variable `tick1`
+        tick1();
+    };
+
+    tick2(); //~ ERROR cannot borrow
+}
+