auto merge of #18388 : nikomatsakis/rust/fn-trait-hierarchy, r=acrichto

Add blanket impls to allow the various `Fn` traits to be interconverted.

Fixes #18387.

Author: bors

src/libcore/ops.rs

@@ -866,34 +866,64 @@ pub trait FnOnce<Args,Result> {
     extern "rust-call" fn call_once(self, args: Args) -> Result;
 }
 
-macro_rules! def_fn_mut(
+impl<F,A,R> FnMut<A,R> for F
+    where F : Fn<A,R>
+{
+    extern "rust-call" fn call_mut(&mut self, args: A) -> R {
+        self.call(args)
+    }
+}
+
+impl<F,A,R> FnOnce<A,R> for F
+    where F : FnMut<A,R>
+{
+    extern "rust-call" fn call_once(mut self, args: A) -> R {
+        self.call_mut(args)
+    }
+}
+
+
+impl<Result> Fn<(),Result> for extern "Rust" fn() -> Result {
+    #[allow(non_snake_case)]
+    extern "rust-call" fn call(&self, _args: ()) -> Result {
+        (*self)()
+    }
+}
+
+impl<Result,A0> Fn<(A0,),Result> for extern "Rust" fn(A0) -> Result {
+    #[allow(non_snake_case)]
+    extern "rust-call" fn call(&self, args: (A0,)) -> Result {
+        let (a0,) = args;
+        (*self)(a0)
+    }
+}
+
+macro_rules! def_fn(
     ($($args:ident)*) => (
         impl<Result$(,$args)*>
-        FnMut<($($args,)*),Result>
+        Fn<($($args,)*),Result>
         for extern "Rust" fn($($args: $args,)*) -> Result {
             #[allow(non_snake_case)]
-            extern "rust-call" fn call_mut(&mut self, args: ($($args,)*)) -> Result {
+            extern "rust-call" fn call(&self, args: ($($args,)*)) -> Result {
                 let ($($args,)*) = args;
                 (*self)($($args,)*)
             }
         }
     )
 )
 
-def_fn_mut!()
-def_fn_mut!(A0)
-def_fn_mut!(A0 A1)
-def_fn_mut!(A0 A1 A2)
-def_fn_mut!(A0 A1 A2 A3)
-def_fn_mut!(A0 A1 A2 A3 A4)
-def_fn_mut!(A0 A1 A2 A3 A4 A5)
-def_fn_mut!(A0 A1 A2 A3 A4 A5 A6)
-def_fn_mut!(A0 A1 A2 A3 A4 A5 A6 A7)
-def_fn_mut!(A0 A1 A2 A3 A4 A5 A6 A7 A8)
-def_fn_mut!(A0 A1 A2 A3 A4 A5 A6 A7 A8 A9)
-def_fn_mut!(A0 A1 A2 A3 A4 A5 A6 A7 A8 A9 A10)
-def_fn_mut!(A0 A1 A2 A3 A4 A5 A6 A7 A8 A9 A10 A11)
-def_fn_mut!(A0 A1 A2 A3 A4 A5 A6 A7 A8 A9 A10 A11 A12)
-def_fn_mut!(A0 A1 A2 A3 A4 A5 A6 A7 A8 A9 A10 A11 A12 A13)
-def_fn_mut!(A0 A1 A2 A3 A4 A5 A6 A7 A8 A9 A10 A11 A12 A13 A14)
-def_fn_mut!(A0 A1 A2 A3 A4 A5 A6 A7 A8 A9 A10 A11 A12 A13 A14 A15)
+def_fn!(A0 A1)
+def_fn!(A0 A1 A2)
+def_fn!(A0 A1 A2 A3)
+def_fn!(A0 A1 A2 A3 A4)
+def_fn!(A0 A1 A2 A3 A4 A5)
+def_fn!(A0 A1 A2 A3 A4 A5 A6)
+def_fn!(A0 A1 A2 A3 A4 A5 A6 A7)
+def_fn!(A0 A1 A2 A3 A4 A5 A6 A7 A8)
+def_fn!(A0 A1 A2 A3 A4 A5 A6 A7 A8 A9)
+def_fn!(A0 A1 A2 A3 A4 A5 A6 A7 A8 A9 A10)
+def_fn!(A0 A1 A2 A3 A4 A5 A6 A7 A8 A9 A10 A11)
+def_fn!(A0 A1 A2 A3 A4 A5 A6 A7 A8 A9 A10 A11 A12)
+def_fn!(A0 A1 A2 A3 A4 A5 A6 A7 A8 A9 A10 A11 A12 A13)
+def_fn!(A0 A1 A2 A3 A4 A5 A6 A7 A8 A9 A10 A11 A12 A13 A14)
+def_fn!(A0 A1 A2 A3 A4 A5 A6 A7 A8 A9 A10 A11 A12 A13 A14 A15)

src/librustc/middle/traits/coherence.rs

@@ -43,7 +43,7 @@ pub fn impl_can_satisfy(infcx: &InferCtxt,
     // Determine whether `impl2` can provide an implementation for those
     // same types.
     let param_env = ty::empty_parameter_environment();
-    let mut selcx = SelectionContext::new(infcx, &param_env, infcx.tcx);
+    let mut selcx = SelectionContext::intercrate(infcx, &param_env, infcx.tcx);
     let obligation = Obligation::misc(DUMMY_SP, impl1_trait_ref);
     debug!("impl_can_satisfy obligation={}", obligation.repr(infcx.tcx));
     selcx.evaluate_impl(impl2_def_id, &obligation)

src/librustc/middle/traits/select.rs

@@ -45,6 +45,22 @@ pub struct SelectionContext<'cx, 'tcx:'cx> {
     /// which is important for checking for trait bounds that
     /// recursively require themselves.
     skolemizer: TypeSkolemizer<'cx, 'tcx>,
+
+    /// If true, indicates that the evaluation should be conservative
+    /// and consider the possibility of types outside this crate.
+    /// This comes up primarily when resolving ambiguity. Imagine
+    /// there is some trait reference `$0 : Bar` where `$0` is an
+    /// inference variable. If `intercrate` is true, then we can never
+    /// say for sure that this reference is not implemented, even if
+    /// there are *no impls at all for `Bar`*, because `$0` could be
+    /// bound to some type that in a downstream crate that implements
+    /// `Bar`. This is the suitable mode for coherence. Elsewhere,
+    /// though, we set this to false, because we are only interested
+    /// in types that the user could actually have written --- in
+    /// other words, we consider `$0 : Bar` to be unimplemented if
+    /// there is no type that the user could *actually name* that
+    /// would satisfy it. This avoids crippling inference, basically.
+    intercrate: bool,
 }
 
 // A stack that walks back up the stack frame.
@@ -142,6 +158,20 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
             param_env: param_env,
             typer: typer,
             skolemizer: infcx.skolemizer(),
+            intercrate: false,
+        }
+    }
+
+    pub fn intercrate(infcx: &'cx InferCtxt<'cx, 'tcx>,
+                      param_env: &'cx ty::ParameterEnvironment,
+                      typer: &'cx Typer<'tcx>)
+                      -> SelectionContext<'cx, 'tcx> {
+        SelectionContext {
+            infcx: infcx,
+            param_env: param_env,
+            typer: typer,
+            skolemizer: infcx.skolemizer(),
+            intercrate: true,
         }
     }
 
@@ -214,44 +244,20 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
     // The result is "true" if the obligation *may* hold and "false" if
     // we can be sure it does not.
 
-    pub fn evaluate_obligation_intercrate(&mut self,
-                                          obligation: &Obligation)
-                                          -> bool
+    pub fn evaluate_obligation(&mut self,
+                               obligation: &Obligation)
+                               -> bool
     {
         /*!
          * Evaluates whether the obligation `obligation` can be
-         * satisfied (by any means). This "intercrate" version allows
-         * for the possibility that unbound type variables may be
-         * instantiated with types from another crate. This is
-         * important for coherence. In practice this means that
-         * unbound type variables must always be considered ambiguous.
+         * satisfied (by any means).
          */
 
-        debug!("evaluate_obligation_intercrate({})",
+        debug!("evaluate_obligation({})",
                obligation.repr(self.tcx()));
 
         let stack = self.push_stack(None, obligation);
-        self.evaluate_stack_intercrate(&stack).may_apply()
-    }
-
-    pub fn evaluate_obligation_intracrate(&mut self,
-                                            obligation: &Obligation)
-                                            -> bool
-    {
-        /*!
-         * Evaluates whether the obligation `obligation` can be
-         * satisfied (by any means). This "intracrate" version does
-         * not allow for the possibility that unbound type variables
-         * may be instantiated with types from another crate; hence,
-         * if there are unbound inputs but no crates locally visible,
-         * it considers the result to be unimplemented.
-         */
-
-        debug!("evaluate_obligation_intracrate({})",
-               obligation.repr(self.tcx()));
-
-        let stack = self.push_stack(None, obligation);
-        self.evaluate_stack_intracrate(&stack).may_apply()
+        self.evaluate_stack(&stack).may_apply()
     }
 
     fn evaluate_builtin_bound_recursively(&mut self,
@@ -288,46 +294,53 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
 
         let stack = self.push_stack(previous_stack.map(|x| x), obligation);
 
-        // FIXME(#17901) -- Intercrate vs intracrate resolution is a
-        // tricky question here. For coherence, we want
-        // intercrate. Also, there was a nasty cycle around impls like
-        // `impl<T:Eq> Eq for Vec<T>` (which would wind up checking
-        // whether `$0:Eq`, where $0 was the value substituted for
-        // `T`, which could then be checked against the very same
-        // impl). This problem is avoided by the stricter rules around
-        // unbound type variables by intercrate. I suspect that in the
-        // latter case a more fine-grained rule would suffice (i.e.,
-        // consider it ambiguous if even 1 impl matches, no need to
-        // figure out which one, but call it unimplemented if 0 impls
-        // match).
-        let result = self.evaluate_stack_intercrate(&stack);
+        let result = self.evaluate_stack(&stack);
 
         debug!("result: {}", result);
         result
     }
 
-    fn evaluate_stack_intercrate(&mut self,
+    fn evaluate_stack(&mut self,
                       stack: &ObligationStack)
                       -> EvaluationResult
     {
-        // Whenever any of the types are unbound, there can always be
-        // an impl.  Even if there are no impls in this crate, perhaps
-        // the type would be unified with something from another crate
-        // that does provide an impl.
+        // In intercrate mode, whenever any of the types are unbound,
+        // there can always be an impl. Even if there are no impls in
+        // this crate, perhaps the type would be unified with
+        // something from another crate that does provide an impl.
+        //
+        // In intracrate mode, we must still be conservative. The reason is
+        // that we want to avoid cycles. Imagine an impl like:
+        //
+        //     impl<T:Eq> Eq for Vec<T>
+        //
+        // and a trait reference like `$0 : Eq` where `$0` is an
+        // unbound variable. When we evaluate this trait-reference, we
+        // will unify `$0` with `Vec<$1>` (for some fresh variable
+        // `$1`), on the condition that `$1 : Eq`. We will then wind
+        // up with many candidates (since that are other `Eq` impls
+        // that apply) and try to winnow things down. This results in
+        // a recurssive evaluation that `$1 : Eq` -- as you can
+        // imagine, this is just where we started. To avoid that, we
+        // check for unbound variables and return an ambiguous (hence possible)
+        // match if we've seen this trait before.
+        //
+        // This suffices to allow chains like `FnMut` implemented in
+        // terms of `Fn` etc, but we could probably make this more
+        // precise still.
         let input_types = stack.skol_trait_ref.input_types();
-        if input_types.iter().any(|&t| ty::type_is_skolemized(t)) {
-            debug!("evaluate_stack_intercrate({}) --> unbound argument, must be ambiguous",
+        let unbound_input_types = input_types.iter().any(|&t| ty::type_is_skolemized(t));
+        if
+            unbound_input_types &&
+             (self.intercrate ||
+              stack.iter().skip(1).any(
+                  |prev| stack.skol_trait_ref.def_id == prev.skol_trait_ref.def_id))
+        {
+            debug!("evaluate_stack_intracrate({}) --> unbound argument, recursion -->  ambiguous",
                    stack.skol_trait_ref.repr(self.tcx()));
             return EvaluatedToAmbig;
         }
 
-        self.evaluate_stack_intracrate(stack)
-    }
-
-    fn evaluate_stack_intracrate(&mut self,
-                                 stack: &ObligationStack)
-                                 -> EvaluationResult
-    {
         // If there is any previous entry on the stack that precisely
         // matches this obligation, then we can assume that the
         // obligation is satisfied for now (still all other conditions
@@ -592,7 +605,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
                 Err(_) => { return Err(()); }
             }
 
-            if self.evaluate_obligation_intracrate(obligation) {
+            if self.evaluate_obligation(obligation) {
                 Ok(())
             } else {
                 Err(())
@@ -804,12 +817,12 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
                                                                           &candidates[i],
                                                                           &candidates[j]));
                 if is_dup {
-                    debug!("Dropping candidate #{}/#{}: {}",
+                    debug!("Dropping candidate #{}/{}: {}",
                            i, candidates.len(), candidates[i].repr(self.tcx()));
                     candidates.swap_remove(i);
                 } else {
-                    debug!("Retaining candidate #{}/#{}",
-                           i, candidates.len());
+                    debug!("Retaining candidate #{}/{}: {}",
+                           i, candidates.len(), candidates[i].repr(self.tcx()));
                     i += 1;
                 }
             }
@@ -828,7 +841,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
         // be the case that you could still satisfy the obligation
         // from another crate by instantiating the type variables with
         // a type from another crate that does have an impl. This case
-        // is checked for in `evaluate_obligation` (and hence users
+        // is checked for in `evaluate_stack` (and hence users
         // who might care about this case, like coherence, should use
         // that function).
         if candidates.len() == 0 {
@@ -849,6 +862,17 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
         // global cache. We want the cache that is specific to this
         // scope whenever where clauses might affect the result.
 
+        // Avoid using the master cache during coherence and just rely
+        // on the local cache. This effectively disables caching
+        // during coherence. It is really just a simplification to
+        // avoid us having to fear that coherence results "pollute"
+        // the master cache. Since coherence executes pretty quickly,
+        // it's not worth going to more trouble to increase the
+        // hit-rate I don't think.
+        if self.intercrate {
+            return &self.param_env.selection_cache;
+        }
+
         // If the trait refers to any parameters in scope, then use
         // the cache of the param-environment.
         if

src/librustc/middle/typeck/check/method.rs

@@ -235,7 +235,7 @@ pub fn lookup_in_trait_adjusted<'a, 'tcx>(
     let mut selcx = traits::SelectionContext::new(fcx.infcx(),
                                                   &fcx.inh.param_env,
                                                   fcx);
-    if !selcx.evaluate_obligation_intracrate(&obligation) {
+    if !selcx.evaluate_obligation(&obligation) {
         debug!("--> Cannot match obligation");
         return None; // Cannot be matched, no such method resolution is possible.
     }

src/librustc/middle/typeck/check/mod.rs

@@ -2147,11 +2147,11 @@ fn try_overloaded_call<'a>(fcx: &FnCtxt,
         _ => {}
     }
 
-    // Try `FnOnce`, then `FnMut`, then `Fn`.
+    // Try the options that are least restrictive on the caller first.
     for &(maybe_function_trait, method_name) in [
-        (fcx.tcx().lang_items.fn_once_trait(), token::intern("call_once")),
+        (fcx.tcx().lang_items.fn_trait(), token::intern("call")),
         (fcx.tcx().lang_items.fn_mut_trait(), token::intern("call_mut")),
-        (fcx.tcx().lang_items.fn_trait(), token::intern("call"))
+        (fcx.tcx().lang_items.fn_once_trait(), token::intern("call_once")),
     ].iter() {
         let function_trait = match maybe_function_trait {
             None => continue,
@@ -3493,6 +3493,11 @@ fn check_expr_with_unifier(fcx: &FnCtxt,
             ast::FnOnceUnboxedClosureKind => ty::FnOnceUnboxedClosureKind,
         };
 
+        debug!("unboxed_closure for {} --> sig={} kind={}",
+               local_def(expr.id).repr(fcx.tcx()),
+               fn_ty.sig.repr(fcx.tcx()),
+               kind);
+
         let unboxed_closure = ty::UnboxedClosure {
             closure_type: fn_ty,
             kind: kind,

src/test/compile-fail/unboxed-closures-fnmut-as-fn.rs

@@ -0,0 +1,34 @@
+// Copyright 2014 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.
+
+// Checks that the Fn trait hierarchy rules do not permit
+// Fn to be used where FnMut is implemented.
+
+#![feature(unboxed_closure_sugar)]
+#![feature(overloaded_calls)]
+
+use std::ops::{Fn,FnMut,FnOnce};
+
+struct S;
+
+impl FnMut<(int,),int> for S {
+    extern "rust-call" fn call_mut(&mut self, (x,): (int,)) -> int {
+        x * x
+    }
+}
+
+fn call_it<F:Fn(int)->int>(f: &F, x: int) -> int {
+    f.call((x,))
+}
+
+fn main() {
+    let x = call_it(&S, 22); //~ ERROR not implemented
+}
+