Auto merge of #128849 - estebank:issue-89143, r=jackh726

Tweak detection of multiple crate versions to be more encompassing

Previously, we only emitted the additional context if the type was in the same crate as the trait that appeared multiple times in the dependency tree. Now, we look at all traits looking for two with the same name in different crates with the same crate number, and we are more flexible looking for the types involved. This will work even if the type that implements the wrong trait version is from a different crate entirely.

```
error[E0277]: the trait bound `CustomErrorHandler: ErrorHandler` is not satisfied because the trait comes from a different crate version
 --> src/main.rs:5:17
  |
5 |     cnb_runtime(CustomErrorHandler {});
  |                 ^^^^^^^^^^^^^^^^^^^^^ the trait `ErrorHandler` is not implemented for `CustomErrorHandler`
  |
note: there are multiple different versions of crate `c` in the dependency graph
 --> /home/gh-estebank/testcase-rustc-crate-version-mismatch/c-v0.2/src/lib.rs:1:1
  |
1 | pub trait ErrorHandler {}
  | ^^^^^^^^^^^^^^^^^^^^^^ this is the required trait
  |
 ::: src/main.rs:1:5
  |
1 | use b::CustomErrorHandler;
  |     - one version of crate `c` is used here, as a dependency of crate `b`
2 | use c::cnb_runtime;
  |     - one version of crate `c` is used here, as a direct dependency of the current crate
  |
 ::: /home/gh-estebank/testcase-rustc-crate-version-mismatch/b/src/lib.rs:1:1
  |
1 | pub struct CustomErrorHandler {}
  | ----------------------------- this type doesn't implement the required trait
  |
 ::: /home/gh-estebank/testcase-rustc-crate-version-mismatch/c-v0.1/src/lib.rs:1:1
  |
1 | pub trait ErrorHandler {}
  | ---------------------- 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
```

Fix #89143.

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 trait_name = self.tcx.item_name(trait_def_id);
-            && let found_type = def.did()
+        let crate_name = self.tcx.crate_name(trait_def_id.krate);
-            && trait_def_id.krate != found_type.krate
+        if let Some(other_trait_def_id) = self.tcx.all_traits().find(|def_id| {
-            && self.tcx.crate_name(trait_def_id.krate) == self.tcx.crate_name(found_type.krate)
+            trait_name == self.tcx.item_name(trait_def_id)
-        {
+                && trait_def_id.krate != def_id.krate
-            let name = self.tcx.crate_name(trait_def_id.krate);
+                && crate_name == self.tcx.crate_name(def_id.krate)
-            let spans: Vec<_> = [trait_def_id, found_type]
+        }) {
-                .into_iter()
+            // We've found two different traits with the same name, same crate name, but
-                .filter(|def_id| def_id.krate != LOCAL_CRATE)
+            // 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::highlighted(format!("{crate_name}")),
-                StringPart::normal("` in the dependency graph".to_string()),
+                StringPart::normal("` in the dependency graph\n".to_string()),
             ]);
-            let candidates = if impl_candidates.is_empty() {
+            if points_at_type {
-                alternative_candidates(trait_def_id)
+                // We only clarify that the same type from different crate versions are not the
-            } else {
+                // same when we *find* the same type coming from different crate versions, otherwise
-                impl_candidates.into_iter().map(|cand| cand.trait_ref).collect()
+                // 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.
-            if let Some((sp_candidate, sp_found)) = candidates.iter().find_map(|trait_ref| {
+                err.highlighted_note(vec![
-                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![
                     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(
+        .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
-            r#"error[E0277]: the trait bound `dep_2_reexport::Type: Trait` is not satisfied
+ --> multiple-dep-versions.rs:7:18
-  --> multiple-dep-versions.rs:7:18
+  |
-   |
+7 |     do_something(Type);
-7  |     do_something(Type);
+  |                  ^^^^ the trait `Trait` is not implemented for `dep_2_reexport::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"#)
-   |     required by a bound introduced by this call
+        .assert_stderr_contains(r#"
-   |
+3 | pub struct Type(pub i32);
-help: there are multiple different versions of crate `dependency` in the dependency graph
+  | --------------- this type implements the required trait
-  --> multiple-dep-versions.rs:1:1
+4 | pub trait Trait {
-   |
+  | ^^^^^^^^^^^^^^^ this is the required trait
-1  | extern crate dep_2_reexport;
+"#)
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ one version of crate `dependency` is used here, as a dependency of crate `foo`
+        .assert_stderr_contains(r#"
-2  | extern crate dependency;
+1 | extern crate dep_2_reexport;
-   | ^^^^^^^^^^^^^^^^^^^^^^^^ one version of crate `dependency` is used here, as a direct dependency of the current crate"#,
+  | ---------------------------- one version of crate `dependency` is used here, as a dependency of crate `foo`
-        )
+2 | extern crate dependency;
-        .assert_stderr_contains(
+  | ------------------------ one version of crate `dependency` is used here, as a direct dependency of the current crate"#)
-            r#"
+        .assert_stderr_contains(r#"
-3  | pub struct Type(pub i32);
+3 | pub struct Type;
-   | ^^^^^^^^^^^^^^^ this type implements the required trait
+  | --------------- this type doesn't implement the required trait
-4  | pub trait Trait {
+4 | pub trait Trait {
-   | --------------- this is the required 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
-        .assert_stderr_contains(
+  = help: you can use `cargo tree` to explore your dependency tree"#)
-            r#"
+        .assert_stderr_contains(r#"error[E0599]: no method named `foo` found for struct `dep_2_reexport::Type` in the current scope
-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
  --> 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"#,
+note: there are multiple different versions of crate `dependency` in the dependency graph"#)
-        )
+        .assert_stderr_contains(r#"
-        .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`"#,
+  |                  ----- `Trait` imported here doesn't correspond to the right version of crate `dependency`"#)
-        )
+        .assert_stderr_contains(r#"
-        .assert_stderr_contains(
-            r#"
 4 | pub trait Trait {
-  | --------------- this is the trait that was imported"#,
+  | --------------- this is the trait that was imported"#)
-        )
+        .assert_stderr_contains(r#"
-        .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"#,
+note: there are multiple different versions of crate `dependency` in the dependency graph"#)
-        )
+        .assert_stderr_contains(r#"
-        .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
+   = help: the trait `Test` is implemented for `A`
-  --> $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
 note: required by a bound in `needs_test`
   --> $DIR/foreign_struct_trait_unimplemented.rs:10:23
    |