Auto merge of #29962 - aturon:coherence-errors, r=nmatsakis

Currently, a coherence error based on overlapping impls simply mentions
the trait, and points to the two conflicting impls:

```
error: conflicting implementations for trait `Foo`
```

With this commit, the error will include all input types to the
trait (including the `Self` type) after unification between the
overlapping impls. In other words, the error message will provide
feedback with full type details, like:

```
error: conflicting implementations of trait `Foo<u32>` for type `u8`:
```

When the `Self` type for the two impls unify to an inference variable,
it is elided in the output, since "for type `_`" is just noise in that
case.

Closes #23980

r? @nikomatsakis

Author: bors

src/librustc/middle/traits/coherence.rs

@@ -27,11 +27,12 @@ use syntax::codemap::{DUMMY_SP, Span};
 #[derive(Copy, Clone)]
 struct InferIsLocal(bool);
 
-/// True if there exist types that satisfy both of the two given impls.
-pub fn overlapping_impls(infcx: &InferCtxt,
-                         impl1_def_id: DefId,
-                         impl2_def_id: DefId)
-                         -> bool
+/// If there are types that satisfy both impls, returns a `TraitRef`
+/// with those types substituted (by updating the given `infcx`)
+pub fn overlapping_impls<'cx, 'tcx>(infcx: &InferCtxt<'cx, 'tcx>,
+                                    impl1_def_id: DefId,
+                                    impl2_def_id: DefId)
+                                    -> Option<ty::TraitRef<'tcx>>
 {
     debug!("impl_can_satisfy(\
            impl1_def_id={:?}, \
@@ -40,16 +41,15 @@ pub fn overlapping_impls(infcx: &InferCtxt,
            impl2_def_id);
 
     let selcx = &mut SelectionContext::intercrate(infcx);
-    infcx.probe(|_| {
-        overlap(selcx, impl1_def_id, impl2_def_id)
-    })
+    overlap(selcx, impl1_def_id, impl2_def_id)
 }
 
-/// Can both impl `a` and impl `b` be satisfied by a common type (including `where` clauses)?
-fn overlap(selcx: &mut SelectionContext,
-           a_def_id: DefId,
-           b_def_id: DefId)
-           -> bool
+/// Can both impl `a` and impl `b` be satisfied by a common type (including
+/// `where` clauses)? If so, returns a `TraitRef` that unifies the two impls.
+fn overlap<'cx, 'tcx>(selcx: &mut SelectionContext<'cx, 'tcx>,
+                      a_def_id: DefId,
+                      b_def_id: DefId)
+                      -> Option<ty::TraitRef<'tcx>>
 {
     debug!("overlap(a_def_id={:?}, b_def_id={:?})",
            a_def_id,
@@ -73,7 +73,7 @@ fn overlap(selcx: &mut SelectionContext,
                                             TypeOrigin::Misc(DUMMY_SP),
                                             a_trait_ref,
                                             b_trait_ref) {
-        return false;
+        return None;
     }
 
     debug!("overlap: unification check succeeded");
@@ -88,10 +88,10 @@ fn overlap(selcx: &mut SelectionContext,
 
     if let Some(failing_obligation) = opt_failing_obligation {
         debug!("overlap: obligation unsatisfiable {:?}", failing_obligation);
-        return false
+        return None
     }
 
-    true
+    Some(selcx.infcx().resolve_type_vars_if_possible(&a_trait_ref))
 }
 
 pub fn trait_ref_is_knowable<'tcx>(tcx: &ty::ctxt<'tcx>, trait_ref: &ty::TraitRef<'tcx>) -> bool

src/librustc_typeck/coherence/overlap.rs

@@ -65,22 +65,19 @@ impl<'cx, 'tcx> OverlapChecker<'cx, 'tcx> {
         // borrows are safe.
         let blanket_impls = trait_def.blanket_impls.borrow();
         let nonblanket_impls = trait_def.nonblanket_impls.borrow();
-        let trait_def_id = trait_def.trait_ref.def_id;
 
         // Conflicts can only occur between a blanket impl and another impl,
         // or between 2 non-blanket impls of the same kind.
 
         for (i, &impl1_def_id) in blanket_impls.iter().enumerate() {
             for &impl2_def_id in &blanket_impls[(i+1)..] {
-                self.check_if_impls_overlap(trait_def_id,
-                                            impl1_def_id,
+                self.check_if_impls_overlap(impl1_def_id,
                                             impl2_def_id);
             }
 
             for v in nonblanket_impls.values() {
                 for &impl2_def_id in v {
-                    self.check_if_impls_overlap(trait_def_id,
-                                                impl1_def_id,
+                    self.check_if_impls_overlap(impl1_def_id,
                                                 impl2_def_id);
                 }
             }
@@ -89,8 +86,7 @@ impl<'cx, 'tcx> OverlapChecker<'cx, 'tcx> {
         for impl_group in nonblanket_impls.values() {
             for (i, &impl1_def_id) in impl_group.iter().enumerate() {
                 for &impl2_def_id in &impl_group[(i+1)..] {
-                    self.check_if_impls_overlap(trait_def_id,
-                                                impl1_def_id,
+                    self.check_if_impls_overlap(impl1_def_id,
                                                 impl2_def_id);
                 }
             }
@@ -121,40 +117,47 @@ impl<'cx, 'tcx> OverlapChecker<'cx, 'tcx> {
 
 
     fn check_if_impls_overlap(&self,
-                              trait_def_id: DefId,
                               impl1_def_id: DefId,
                               impl2_def_id: DefId)
     {
         if let Some((impl1_def_id, impl2_def_id)) = self.order_impls(
             impl1_def_id, impl2_def_id)
         {
-            debug!("check_if_impls_overlap({:?}, {:?}, {:?})",
-                   trait_def_id,
+            debug!("check_if_impls_overlap({:?}, {:?})",
                    impl1_def_id,
                    impl2_def_id);
 
             let infcx = infer::new_infer_ctxt(self.tcx, &self.tcx.tables, None, false);
-            if traits::overlapping_impls(&infcx, impl1_def_id, impl2_def_id) {
-                self.report_overlap_error(trait_def_id, impl1_def_id, impl2_def_id);
+            if let Some(trait_ref) = traits::overlapping_impls(&infcx, impl1_def_id, impl2_def_id) {
+                self.report_overlap_error(impl1_def_id, impl2_def_id, trait_ref);
             }
         }
     }
 
-    fn report_overlap_error(&self, trait_def_id: DefId,
-                            impl1: DefId, impl2: DefId) {
+    fn report_overlap_error(&self,
+                            impl1: DefId,
+                            impl2: DefId,
+                            trait_ref: ty::TraitRef)
+    {
+        // only print the Self type if it's concrete; otherwise, it's not adding much information.
+        let self_type = {
+            trait_ref.substs.self_ty().and_then(|ty| {
+                if let ty::TyInfer(_) = ty.sty {
+                    None
+                } else {
+                    Some(format!(" for type `{}`", ty))
+                }
+            }).unwrap_or(String::new())
+        };
 
         span_err!(self.tcx.sess, self.span_of_impl(impl1), E0119,
-                  "conflicting implementations for trait `{}`",
-                  self.tcx.item_path_str(trait_def_id));
-
-        self.report_overlap_note(impl2);
-    }
-
-    fn report_overlap_note(&self, impl2: DefId) {
+                  "conflicting implementations of trait `{}`{}:",
+                  trait_ref,
+                  self_type);
 
         if impl2.is_local() {
             span_note!(self.tcx.sess, self.span_of_impl(impl2),
-                       "note conflicting implementation here");
+                       "conflicting implementation is here:");
         } else {
             let cname = self.tcx.sess.cstore.crate_name(impl2.krate);
             self.tcx.sess.note(&format!("conflicting implementation in crate `{}`", cname));
@@ -180,9 +183,9 @@ impl<'cx, 'tcx,'v> intravisit::Visitor<'v> for OverlapChecker<'cx, 'tcx> {
                 let prev_default_impl = self.default_impls.insert(trait_ref.def_id, item.id);
                 match prev_default_impl {
                     Some(prev_id) => {
-                        self.report_overlap_error(trait_ref.def_id,
-                                                  impl_def_id,
-                                                  self.tcx.map.local_def_id(prev_id));
+                        self.report_overlap_error(impl_def_id,
+                                                  self.tcx.map.local_def_id(prev_id),
+                                                  trait_ref);
                     }
                     None => { }
                 }

src/librustc_typeck/diagnostics.rs

@@ -1569,8 +1569,8 @@ struct Foo {
     value: usize
 }
 
-impl MyTrait for Foo { // error: conflicting implementations for trait
-                       //        `MyTrait`
+impl MyTrait for Foo { // error: conflicting implementations of trait
+                       //        `MyTrait` for type `Foo`
     fn get(&self) -> usize { self.value }
 }
 ```

src/test/compile-fail/coherence-conflicting-negative-trait-impl.rs

@@ -15,14 +15,14 @@ trait MyTrait {}
 struct TestType<T>(::std::marker::PhantomData<T>);
 
 unsafe impl<T: MyTrait+'static> Send for TestType<T> {}
-//~^ ERROR conflicting implementations for trait `core::marker::Send`
-//~^^ ERROR conflicting implementations for trait `core::marker::Send`
+//~^ ERROR conflicting implementations of trait `core::marker::Send`
+//~^^ ERROR conflicting implementations of trait `core::marker::Send`
 
 impl<T: MyTrait> !Send for TestType<T> {}
-//~^ ERROR conflicting implementations for trait `core::marker::Send`
+//~^ ERROR conflicting implementations of trait `core::marker::Send`
 
 unsafe impl<T:'static> Send for TestType<T> {}
-//~^ ERROR error: conflicting implementations for trait `core::marker::Send`
+//~^ ERROR error: conflicting implementations of trait `core::marker::Send`
 
 impl !Send for TestType<i32> {}
 

src/test/compile-fail/coherence-default-trait-impl.rs

@@ -15,7 +15,7 @@ trait MyTrait {}
 impl MyTrait for .. {}
 
 impl MyTrait for .. {}
-//~^ ERROR conflicting implementations for trait `MyTrait`
+//~^ ERROR conflicting implementations of trait `MyTrait`
 
 trait MySafeTrait {}
 

src/test/compile-fail/coherence-overlap-messages.rs

@@ -0,0 +1,42 @@
+// 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.
+
+trait Foo {}
+
+impl<T> Foo for T {} //~ ERROR conflicting implementations of trait `Foo`:
+impl<U> Foo for U {}
+
+trait Bar {}
+
+impl<T> Bar for T {} //~ ERROR conflicting implementations of trait `Bar` for type `u8`:
+impl Bar for u8 {}
+
+trait Baz<T> {}
+
+impl<T, U> Baz<U> for T {} //~ ERROR conflicting implementations of trait `Baz<_>` for type `u8`:
+impl<T> Baz<T> for u8 {}
+
+trait Quux<T> {}
+
+impl<T, U> Quux<U> for T {} //~ ERROR conflicting implementations of trait `Quux<_>`:
+impl<T> Quux<T> for T {}
+
+trait Qaar<T> {}
+
+impl<T, U> Qaar<U> for T {} //~ ERROR conflicting implementations of trait `Qaar<u8>`:
+impl<T> Qaar<u8> for T {}
+
+trait Qaax<T> {}
+
+impl<T, U> Qaax<U> for T {}
+//~^ ERROR conflicting implementations of trait `Qaax<u8>` for type `u32`:
+impl Qaax<u8> for u32 {}
+
+fn main() {}

src/test/compile-fail/issue-28568.rs

@@ -11,12 +11,12 @@
 struct MyStruct;
 
 impl Drop for MyStruct {
-//~^ ERROR conflicting implementations for trait
+//~^ ERROR conflicting implementations of trait
     fn drop(&mut self) { }
 }
 
 impl Drop for MyStruct {
-//~^ NOTE conflicting implementation here
+//~^ NOTE conflicting implementation is here
     fn drop(&mut self) { }
 }