Select projections over impls in case of ambiguity. Fixes #23336.

Author: arielb1

src/librustc/middle/traits/select.rs

@@ -117,19 +117,48 @@ pub enum MethodMatchedData {
 /// candidate is one that might match or might not, depending on how
 /// type variables wind up being resolved. This only occurs during inference.
 ///
-/// For selection to succeed, there must be exactly one non-ambiguous
-/// candidate.  Usually, it is not possible to have more than one
-/// definitive candidate, due to the coherence rules. However, there is
-/// one case where it could occur: if there is a blanket impl for a
-/// trait (that is, an impl applied to all T), and a type parameter
-/// with a where clause. In that case, we can have a candidate from the
-/// where clause and a second candidate from the impl. This is not a
-/// problem because coherence guarantees us that the impl which would
-/// be used to satisfy the where clause is the same one that we see
-/// now. To resolve this issue, therefore, we ignore impls if we find a
-/// matching where clause. Part of the reason for this is that where
-/// clauses can give additional information (like, the types of output
-/// parameters) that would have to be inferred from the impl.
+/// For selection to succeed, there must be exactly one potential
+/// candidate. Coherence mostly ensures that (disregarding
+/// still-uninferred type variables), but there's a subtlety - coherence
+/// does not consider the parameter environment, so we can have a candidate
+/// from a where-clause (or associated-type bound) along with the
+/// global candidate.
+///
+/// For example, if we have a blanket impl
+///     impl<T> MyTrait for T
+/// and a where-clause
+///     fn f<T>(t: T) where T: MyTrait
+/// Then there are 2 candidates, but they are actually equivalent, so
+/// it doesn't really matter which one is chosen. Note that the impl
+/// can have bounds - even in this example, we have T: Sized, and the
+/// where-clause could just as well have been
+/// and the where-clause could have been Option<T> : MyTrait).
+///
+/// This gets subtler. The global candidate can have bounds that
+/// are not already known:
+///     impl<T: Copy> MyTrait for T
+/// If our type is known, we know that this impl will be the one chosen,
+/// so we can just mark a `T: Copy` obligation and move on. This helps
+/// inference in some cases. However, if we have a where-clause for
+/// MyTrait in scope, we can't make this assumption, because another crate
+/// could have a `impl MyTrait for FreshNonCopyType`, which was "passed"
+/// through the where-clause. This had caused several bugs, including #18453.
+///
+/// From the above, it would seem to be that the best option is to
+/// just pick the where-clause. This is what we do today. However,
+/// it is not a panacea. Assume we have:
+///
+///     impl<'a> MyTrait<'a> for T
+///     fn f<'s, T>(u: &'s T) where T: MyTrait<'s>
+///
+/// Then we know that, for all lifetimes 'a, `T: MyTrait<'a>`.
+/// However, because lifetimes are not known at trait-selection time,
+/// we pick the where-clause, and infer that the lifetime must
+/// be 's - the additional clause "poisons" selection. This could
+/// get subtler, as there could have been a `T: Copy` bound on
+/// the blanket impl, or multiple clauses on T:
+///     fn f<'s, T>(u: &'s T) where T: MyTrait<'s>+MyTrait<'tcx>
+/// We don't handle this at this moment.
 #[derive(PartialEq,Eq,Debug,Clone)]
 enum SelectionCandidate<'tcx> {
     PhantomFnCandidate,
@@ -1307,54 +1336,50 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
         self.evaluate_predicates_recursively(stack, selection.iter_nested())
     }
 
-    /// Returns true if `candidate_i` should be dropped in favor of
-    /// `candidate_j`.  Generally speaking we will drop duplicate
+    /// Returns true if `victim` should be dropped in favor of
+    /// `other`.  Generally speaking we will drop duplicate
     /// candidates and prefer where-clause candidates.
     fn candidate_should_be_dropped_in_favor_of<'o>(&mut self,
-                                                   candidate_i: &SelectionCandidate<'tcx>,
-                                                   candidate_j: &SelectionCandidate<'tcx>)
+                                                   victim: &SelectionCandidate<'tcx>,
+                                                   other: &SelectionCandidate<'tcx>)
                                                    -> bool
     {
-        if candidate_i == candidate_j {
+        if victim == other {
             return true;
         }
 
-        match (candidate_i, candidate_j) {
-            (&ImplCandidate(..), &ParamCandidate(..)) |
-            (&ClosureCandidate(..), &ParamCandidate(..)) |
-            (&FnPointerCandidate(..), &ParamCandidate(..)) |
-            (&BuiltinObjectCandidate(..), &ParamCandidate(_)) |
-            (&BuiltinCandidate(..), &ParamCandidate(..)) => {
-                // We basically prefer always prefer to use a
-                // where-clause over another option. Where clauses
-                // impose the burden of finding the exact match onto
-                // the caller. Using an impl in preference of a where
-                // clause can also lead us to "overspecialize", as in
-                // #18453.
-                true
-            }
-            (&DefaultImplCandidate(_), _) => {
-                // Prefer other candidates over default implementations.
-                self.tcx().sess.bug(
-                    "default implementations shouldn't be recorded \
-                     when there are other valid candidates");
-            }
-            (&ProjectionCandidate, &ParamCandidate(_)) => {
-                // FIXME(#20297) -- this gives where clauses precedent
-                // over projections. Really these are just two means
-                // of deducing information (one based on the where
-                // clauses on the trait definition; one based on those
-                // on the enclosing scope), and it'd be better to
-                // integrate them more intelligently. But for now this
-                // seems ok. If we DON'T give where clauses
-                // precedence, we run into trouble in default methods,
-                // where both the projection bounds for `Self::A` and
-                // the where clauses are in scope.
-                true
-            }
-            _ => {
-                false
-            }
+        match other {
+            &ParamCandidate(_) | &ProjectionCandidate => match victim {
+                &DefaultImplCandidate(..) => {
+                    self.tcx().sess.bug(
+                        "default implementations shouldn't be recorded \
+                         when there are other valid candidates");
+                }
+                &PhantomFnCandidate => {
+                    self.tcx().sess.bug("PhantomFn didn't short-circuit selection");
+                }
+                &ImplCandidate(..) |
+                &ClosureCandidate(..) |
+                &FnPointerCandidate(..) |
+                &BuiltinObjectCandidate(..) |
+                &ObjectCandidate(..) |
+                &BuiltinCandidate(..) => {
+                    // A param/projection candidate is a sure match,
+                    // choose it.
+                    // FIXME: This isn't exactly correct because of
+                    // lifetimes.
+                    true
+                }
+                &ProjectionCandidate => {
+                    // Arbitrarily give param candidates priority
+                    // over projection candidates. Both are "sure"
+                    // matches.
+                    true
+                },
+                &ParamCandidate(..) => false,
+                &ErrorCandidate => false // propagate errors
+            },
+            _ => false
         }
     }
 

src/test/run-pass/issue-23336.rs

@@ -0,0 +1,20 @@
+// 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.
+
+pub trait Data { fn doit(&self) {} }
+impl<T> Data for T {}
+pub trait UnaryLogic { type D: Data; }
+impl UnaryLogic for () { type D = i32; }
+
+pub fn crashes<T: UnaryLogic>(t: T::D) {
+    t.doit();
+}
+
+fn main() { crashes::<()>(0); }