Tweak detection of multiple crate versions to be more encompassing

compiler/rustc_trait_selection/src/error_reporting/traits/fulfillment_errors.rs

@@ -1707,15 +1707,31 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
         // one crate version and the type comes from another crate version, even though they both
         // are from the same crate.
         let trait_def_id = trait_ref.def_id();
-        if let ty::Adt(def, _) = trait_ref.self_ty().skip_binder().peel_refs().kind()
-            && let found_type = def.did()
-            && trait_def_id.krate != found_type.krate
-            && self.tcx.crate_name(trait_def_id.krate) == self.tcx.crate_name(found_type.krate)
-        {
-            let name = self.tcx.crate_name(trait_def_id.krate);
-            let spans: Vec<_> = [trait_def_id, found_type]
-                .into_iter()
-                .filter(|def_id| def_id.krate != LOCAL_CRATE)
+        let trait_name = self.tcx.item_name(trait_def_id);
+        let crate_name = self.tcx.crate_name(trait_def_id.krate);
+        if let Some(other_trait_def_id) = self.tcx.all_traits().find(|def_id| {
+            trait_name == self.tcx.item_name(trait_def_id)
+                && trait_def_id.krate != def_id.krate
+                && crate_name == self.tcx.crate_name(def_id.krate)
+        }) {
+            // We've found two different traits with the same name, same crate name, but
+            // different crate `DefId`. We highlight the traits.
+
+            let found_type =
+                if let ty::Adt(def, _) = trait_ref.self_ty().skip_binder().peel_refs().kind() {
+                    Some(def.did())
+                } else {
+                    None
+                };
+            let candidates = if impl_candidates.is_empty() {
+                alternative_candidates(trait_def_id)
+            } else {
+                impl_candidates.into_iter().map(|cand| cand.trait_ref).collect()
+            };
+            let mut span: MultiSpan = self.tcx.def_span(trait_def_id).into();
+            span.push_span_label(self.tcx.def_span(trait_def_id), "this is the required trait");
+            for (sp, label) in [trait_def_id, other_trait_def_id]
+                .iter()
                 .filter_map(|def_id| self.tcx.extern_crate(def_id.krate))
                 .map(|data| {
                     let dependency = if data.dependency_of == LOCAL_CRATE {
@@ -1726,57 +1742,86 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
                     };
                     (
                         data.span,
-                        format!("one version of crate `{name}` is used here, as a {dependency}"),
+                        format!(
+                            "one version of crate `{crate_name}` is used here, as a {dependency}"
+                        ),
                     )
                 })
-                .collect();
-            let mut span: MultiSpan = spans.iter().map(|(sp, _)| *sp).collect::<Vec<Span>>().into();
-            for (sp, label) in spans.into_iter() {
+            {
                 span.push_span_label(sp, label);
             }
-            err.highlighted_span_help(span, vec![
+            let mut points_at_type = false;
+            if let Some(found_type) = found_type {
+                span.push_span_label(
+                    self.tcx.def_span(found_type),
+                    "this type doesn't implement the required trait",
+                );
+                for trait_ref in candidates {
+                    if let ty::Adt(def, _) = trait_ref.self_ty().peel_refs().kind()
+                        && let candidate_def_id = def.did()
+                        && let Some(name) = self.tcx.opt_item_name(candidate_def_id)
+                        && let Some(found) = self.tcx.opt_item_name(found_type)
+                        && name == found
+                        && candidate_def_id.krate != found_type.krate
+                        && self.tcx.crate_name(candidate_def_id.krate)
+                            == self.tcx.crate_name(found_type.krate)
+                    {
+                        // A candidate was found of an item with the same name, from two separate
+                        // versions of the same crate, let's clarify.
+                        let candidate_span = self.tcx.def_span(candidate_def_id);
+                        span.push_span_label(
+                            candidate_span,
+                            "this type implements the required trait",
+                        );
+                        points_at_type = true;
+                    }
+                }
+            }
+            span.push_span_label(self.tcx.def_span(other_trait_def_id), "this is the found trait");
+            err.highlighted_span_note(span, vec![
                 StringPart::normal("there are ".to_string()),
                 StringPart::highlighted("multiple different versions".to_string()),
                 StringPart::normal(" of crate `".to_string()),
-                StringPart::highlighted(format!("{name}")),
-                StringPart::normal("` in the dependency graph".to_string()),
+                StringPart::highlighted(format!("{crate_name}")),
+                StringPart::normal("` in the dependency graph\n".to_string()),
             ]);
-            let candidates = if impl_candidates.is_empty() {
-                alternative_candidates(trait_def_id)
-            } else {
-                impl_candidates.into_iter().map(|cand| cand.trait_ref).collect()
-            };
-            if let Some((sp_candidate, sp_found)) = candidates.iter().find_map(|trait_ref| {
-                if let ty::Adt(def, _) = trait_ref.self_ty().peel_refs().kind()
-                    && let candidate_def_id = def.did()
-                    && let Some(name) = self.tcx.opt_item_name(candidate_def_id)
-                    && let Some(found) = self.tcx.opt_item_name(found_type)
-                    && name == found
-                    && candidate_def_id.krate != found_type.krate
-                    && self.tcx.crate_name(candidate_def_id.krate)
-                        == self.tcx.crate_name(found_type.krate)
-                {
-                    // A candidate was found of an item with the same name, from two separate
-                    // versions of the same crate, let's clarify.
-                    Some((self.tcx.def_span(candidate_def_id), self.tcx.def_span(found_type)))
-                } else {
-                    None
-                }
-            }) {
-                let mut span: MultiSpan = vec![sp_candidate, sp_found].into();
-                span.push_span_label(self.tcx.def_span(trait_def_id), "this is the required trait");
-                span.push_span_label(sp_candidate, "this type implements the required trait");
-                span.push_span_label(sp_found, "this type doesn't implement the required trait");
-                err.highlighted_span_note(span, vec![
+            if points_at_type {
+                // We only clarify that the same type from different crate versions are not the
+                // same when we *find* the same type coming from different crate versions, otherwise
+                // it could be that it was a type provided by a different crate than the one that
+                // provides the trait, and mentioning this adds verbosity without clarification.
+                err.highlighted_note(vec![
                     StringPart::normal(
                         "two types coming from two different versions of the same crate are \
-                             different types "
+                         different types "
                             .to_string(),
                     ),
                     StringPart::highlighted("even if they look the same".to_string()),
                 ]);
             }
-            err.help("you can use `cargo tree` to explore your dependency tree");
+            err.highlighted_help(vec![
+                StringPart::normal("you can use `".to_string()),
+                StringPart::highlighted("cargo tree".to_string()),
+                StringPart::normal("` to explore your dependency tree".to_string()),
+            ]);
+
+            // FIXME: this is a giant hack for the benefit of this specific diagnostic. Because
+            // we're so nested in method calls before the error gets emitted, bubbling a single bit
+            // flag informing the top level caller to stop adding extra detail to the diagnostic,
+            // would actually be harder to follow. So we do something naughty here: we consume the
+            // diagnostic, emit it and leave in its place a "delayed bug" that will continue being
+            // modified but won't actually be printed to end users. This *is not ideal*, but allows
+            // us to reduce the verbosity of an error that is already quite verbose and increase its
+            // specificity. Below we modify the main message as well, in a way that *could* break if
+            // the implementation of Diagnostics change significantly, but that would be caught with
+            // a make test failure when this diagnostic is tested.
+            err.primary_message(format!(
+                "{} because the trait comes from a different crate version",
+                err.messages[0].0.as_str().unwrap(),
+            ));
+            let diag = err.clone();
+            err.downgrade_to_delayed_bug();
+            self.tcx.dcx().emit_diagnostic(diag);
             return true;
         }
 

tests/run-make/crate-loading/multiple-dep-versions-3.rs

@@ -3,3 +3,8 @@
 
 extern crate dependency;
 pub use dependency::Type;
+pub struct OtherType;
+impl dependency::Trait for OtherType {
+    fn foo(&self) {}
+    fn bar() {}
+}

tests/run-make/crate-loading/multiple-dep-versions.rs

@@ -1,10 +1,11 @@
 extern crate dep_2_reexport;
 extern crate dependency;
-use dep_2_reexport::Type;
+use dep_2_reexport::{OtherType, Type};
 use dependency::{Trait, do_something};
 
 fn main() {
     do_something(Type);
     Type.foo();
     Type::bar();
+    do_something(OtherType);
 }

tests/run-make/crate-loading/rmake.rs

@@ -18,47 +18,39 @@ fn main() {
         .extern_("dependency", rust_lib_name("dependency"))
         .extern_("dep_2_reexport", rust_lib_name("foo"))
         .run_fail()
-        .assert_stderr_contains(
-            r#"error[E0277]: the trait bound `dep_2_reexport::Type: Trait` is not satisfied
-  --> multiple-dep-versions.rs:7:18
-   |
-7  |     do_something(Type);
-   |     ------------ ^^^^ the trait `Trait` is not implemented for `dep_2_reexport::Type`
-   |     |
-   |     required by a bound introduced by this call
-   |
-help: there are multiple different versions of crate `dependency` in the dependency graph
-  --> multiple-dep-versions.rs:1:1
-   |
-1  | extern crate dep_2_reexport;
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ one version of crate `dependency` is used here, as a dependency of crate `foo`
-2  | extern crate dependency;
-   | ^^^^^^^^^^^^^^^^^^^^^^^^ one version of crate `dependency` is used here, as a direct dependency of the current crate"#,
-        )
-        .assert_stderr_contains(
-            r#"
-3  | pub struct Type(pub i32);
-   | ^^^^^^^^^^^^^^^ this type implements the required trait
-4  | pub trait Trait {
-   | --------------- this is the required trait"#,
-        )
-        .assert_stderr_contains(
-            r#"
-3  | pub struct Type;
-   | ^^^^^^^^^^^^^^^ this type doesn't implement the required trait"#,
-        )
-        .assert_stderr_contains(
-            r#"
-error[E0599]: no method named `foo` found for struct `dep_2_reexport::Type` in the current scope
+        .assert_stderr_contains(r#"error[E0277]: the trait bound `dep_2_reexport::Type: Trait` is not satisfied because the trait comes from a different crate version
+ --> multiple-dep-versions.rs:7:18
+  |
+7 |     do_something(Type);
+  |                  ^^^^ the trait `Trait` is not implemented for `dep_2_reexport::Type`
+  |
+note: there are multiple different versions of crate `dependency` in the dependency graph"#)
+        .assert_stderr_contains(r#"
+3 | pub struct Type(pub i32);
+  | --------------- this type implements the required trait
+4 | pub trait Trait {
+  | ^^^^^^^^^^^^^^^ this is the required trait
+"#)
+        .assert_stderr_contains(r#"
+1 | extern crate dep_2_reexport;
+  | ---------------------------- one version of crate `dependency` is used here, as a dependency of crate `foo`
+2 | extern crate dependency;
+  | ------------------------ one version of crate `dependency` is used here, as a direct dependency of the current crate"#)
+        .assert_stderr_contains(r#"
+3 | pub struct Type;
+  | --------------- this type doesn't implement the required trait
+4 | pub trait Trait {
+  | --------------- this is the found trait
+  = note: two types coming from two different versions of the same crate are different types even if they look the same
+  = help: you can use `cargo tree` to explore your dependency tree"#)
+        .assert_stderr_contains(r#"error[E0599]: no method named `foo` found for struct `dep_2_reexport::Type` in the current scope
  --> multiple-dep-versions.rs:8:10
   |
 8 |     Type.foo();
   |          ^^^ method not found in `Type`
   |
-note: there are multiple different versions of crate `dependency` in the dependency graph"#,
-        )
-        .assert_stderr_contains(
-            r#"
+note: there are multiple different versions of crate `dependency` in the dependency graph"#)
+        .assert_stderr_contains(r#"
 4 | pub trait Trait {
   | ^^^^^^^^^^^^^^^ this is the trait that is needed
 5 |     fn foo(&self);
@@ -67,25 +59,19 @@ note: there are multiple different versions of crate `dependency` in the depende
  ::: multiple-dep-versions.rs:4:18
   |
 4 | use dependency::{Trait, do_something};
-  |                  ----- `Trait` imported here doesn't correspond to the right version of crate `dependency`"#,
-        )
-        .assert_stderr_contains(
-            r#"
+  |                  ----- `Trait` imported here doesn't correspond to the right version of crate `dependency`"#)
+        .assert_stderr_contains(r#"
 4 | pub trait Trait {
-  | --------------- this is the trait that was imported"#,
-        )
-        .assert_stderr_contains(
-            r#"
+  | --------------- this is the trait that was imported"#)
+        .assert_stderr_contains(r#"
 error[E0599]: no function or associated item named `bar` found for struct `dep_2_reexport::Type` in the current scope
  --> multiple-dep-versions.rs:9:11
   |
 9 |     Type::bar();
   |           ^^^ function or associated item not found in `Type`
   |
-note: there are multiple different versions of crate `dependency` in the dependency graph"#,
-        )
-        .assert_stderr_contains(
-            r#"
+note: there are multiple different versions of crate `dependency` in the dependency graph"#)
+        .assert_stderr_contains(r#"
 4 | pub trait Trait {
   | ^^^^^^^^^^^^^^^ this is the trait that is needed
 5 |     fn foo(&self);
@@ -95,6 +81,9 @@ note: there are multiple different versions of crate `dependency` in the depende
  ::: multiple-dep-versions.rs:4:18
   |
 4 | use dependency::{Trait, do_something};
-  |                  ----- `Trait` imported here doesn't correspond to the right version of crate `dependency`"#,
-        );
+  |                  ----- `Trait` imported here doesn't correspond to the right version of crate `dependency`"#)
+        .assert_stderr_contains(
+          r#"
+6  | pub struct OtherType;
+   | -------------------- this type doesn't implement the required trait"#);
 }

tests/ui/typeck/foreign_struct_trait_unimplemented.stderr

@@ -6,12 +6,7 @@ LL |     needs_test(foreign_struct_trait_unimplemented::B);
    |     |
    |     required by a bound introduced by this call
    |
-help: there are multiple different versions of crate `foreign_struct_trait_unimplemented` in the dependency graph
-  --> $DIR/foreign_struct_trait_unimplemented.rs:3:1
-   |
-LL | extern crate foreign_struct_trait_unimplemented;
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ one version of crate `foreign_struct_trait_unimplemented` is used here, as a direct dependency of the current crate
-   = help: you can use `cargo tree` to explore your dependency tree
+   = help: the trait `Test` is implemented for `A`
 note: required by a bound in `needs_test`
   --> $DIR/foreign_struct_trait_unimplemented.rs:10:23
    |