From 61058937e5a9d58ce1f9c5e0ac6beef22c54cadf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Sat, 10 Aug 2024 00:32:10 +0000 Subject: [PATCH] Rework suggestion method Make checking slightly cheaper (by restricting to the right item only). Add tests. --- .../rustc_hir_typeck/src/method/suggest.rs | 98 +++++++++---------- .../crate-loading/multiple-dep-versions-1.rs | 2 + .../crate-loading/multiple-dep-versions-2.rs | 2 + .../crate-loading/multiple-dep-versions.rs | 1 + tests/run-make/crate-loading/rmake.rs | 84 ++++++++++++++-- 5 files changed, 127 insertions(+), 60 deletions(-) diff --git a/compiler/rustc_hir_typeck/src/method/suggest.rs b/compiler/rustc_hir_typeck/src/method/suggest.rs index cf9f496ed56..31366a30760 100644 --- a/compiler/rustc_hir_typeck/src/method/suggest.rs +++ b/compiler/rustc_hir_typeck/src/method/suggest.rs @@ -3497,8 +3497,7 @@ fn suggest_traits_to_import( .detect_and_explain_multiple_crate_versions( err, pick.item.def_id, - pick.item.ident(self.tcx).span, - rcvr.hir_id.owner, + rcvr.hir_id, *rcvr_ty, ); if pick.autoderefs == 0 && !trait_in_other_version_found { @@ -3705,8 +3704,7 @@ fn suggest_traits_to_import( trait_in_other_version_found = self.detect_and_explain_multiple_crate_versions( err, assoc.def_id, - self.tcx.def_span(assoc.def_id), - ty.hir_id.owner, + ty.hir_id, rcvr_ty, ); } @@ -4081,55 +4079,55 @@ fn detect_and_explain_multiple_crate_versions( &self, err: &mut Diag<'_>, item_def_id: DefId, - item_span: Span, - owner: hir::OwnerId, + hir_id: hir::HirId, rcvr_ty: Ty<'_>, ) -> bool { - let pick_name = self.tcx.crate_name(item_def_id.krate); - let trait_did = self.tcx.parent(item_def_id); - let trait_name = self.tcx.item_name(trait_did); - if let Some(map) = self.tcx.in_scope_traits_map(owner) { - for trait_candidate in map.to_sorted_stable_ord().into_iter().flat_map(|v| v.1.iter()) { - let crate_name = self.tcx.crate_name(trait_candidate.def_id.krate); - if trait_candidate.def_id.krate != item_def_id.krate && crate_name == pick_name { - let msg = format!( - "there are multiple different versions of crate `{crate_name}` in the \ - dependency graph", - ); - let candidate_name = self.tcx.item_name(trait_candidate.def_id); - if candidate_name == trait_name - && let Some(def_id) = trait_candidate.import_ids.get(0) - { - let span = self.tcx.def_span(*def_id); - let mut multi_span: MultiSpan = span.into(); - multi_span.push_span_label( - span, - format!( - "`{crate_name}` imported here doesn't correspond to the right \ - crate version", - ), - ); - multi_span.push_span_label( - self.tcx.def_span(trait_candidate.def_id), - format!("this is the trait that was imported"), - ); - multi_span.push_span_label( - self.tcx.def_span(trait_did), - format!("this is the trait that is needed"), - ); - multi_span.push_span_label( - item_span, - format!("the method is available for `{rcvr_ty}` here"), - ); - err.span_note(multi_span, msg); - } else { - err.note(msg); - } - return true; - } - } + let hir_id = self.tcx.parent_hir_id(hir_id); + let Some(traits) = self.tcx.in_scope_traits(hir_id) else { return false }; + if traits.is_empty() { + return false; } - false + let trait_def_id = self.tcx.parent(item_def_id); + let krate = self.tcx.crate_name(trait_def_id.krate); + let name = self.tcx.item_name(trait_def_id); + let candidates: Vec<_> = traits + .iter() + .filter(|c| { + c.def_id.krate != trait_def_id.krate + && self.tcx.crate_name(c.def_id.krate) == krate + && self.tcx.item_name(c.def_id) == name + }) + .map(|c| (c.def_id, c.import_ids.get(0).cloned())) + .collect(); + if candidates.is_empty() { + return false; + } + let item_span = self.tcx.def_span(item_def_id); + let msg = format!( + "there are multiple different versions of crate `{krate}` in the dependency graph", + ); + let trait_span = self.tcx.def_span(trait_def_id); + let mut multi_span: MultiSpan = trait_span.into(); + multi_span.push_span_label(trait_span, format!("this is the trait that is needed")); + multi_span + .push_span_label(item_span, format!("the method is available for `{rcvr_ty}` here")); + for (def_id, import_def_id) in candidates { + if let Some(import_def_id) = import_def_id { + multi_span.push_span_label( + self.tcx.def_span(import_def_id), + format!( + "`{name}` imported here doesn't correspond to the right version of crate \ + `{krate}`", + ), + ); + } + multi_span.push_span_label( + self.tcx.def_span(def_id), + format!("this is the trait that was imported"), + ); + } + err.span_note(multi_span, msg); + true } /// issue #102320, for `unwrap_or` with closure as argument, suggest `unwrap_or_else` diff --git a/tests/run-make/crate-loading/multiple-dep-versions-1.rs b/tests/run-make/crate-loading/multiple-dep-versions-1.rs index 94f30ca326f..d81462504dd 100644 --- a/tests/run-make/crate-loading/multiple-dep-versions-1.rs +++ b/tests/run-make/crate-loading/multiple-dep-versions-1.rs @@ -3,8 +3,10 @@ pub struct Type(pub i32); pub trait Trait { fn foo(&self); + fn bar(); } impl Trait for Type { fn foo(&self) {} + fn bar() {} } pub fn do_something(_: X) {} diff --git a/tests/run-make/crate-loading/multiple-dep-versions-2.rs b/tests/run-make/crate-loading/multiple-dep-versions-2.rs index 0a4626be560..0a566fe2c60 100644 --- a/tests/run-make/crate-loading/multiple-dep-versions-2.rs +++ b/tests/run-make/crate-loading/multiple-dep-versions-2.rs @@ -3,8 +3,10 @@ pub struct Type; pub trait Trait { fn foo(&self); + fn bar(); } impl Trait for Type { fn foo(&self) {} + fn bar() {} } pub fn do_something(_: X) {} diff --git a/tests/run-make/crate-loading/multiple-dep-versions.rs b/tests/run-make/crate-loading/multiple-dep-versions.rs index 113a6b76d7d..8ef042bf418 100644 --- a/tests/run-make/crate-loading/multiple-dep-versions.rs +++ b/tests/run-make/crate-loading/multiple-dep-versions.rs @@ -6,4 +6,5 @@ fn main() { do_something(Type); Type.foo(); + Type::bar(); } diff --git a/tests/run-make/crate-loading/rmake.rs b/tests/run-make/crate-loading/rmake.rs index 1b9c13c1f6a..65cbc8b0b1b 100644 --- a/tests/run-make/crate-loading/rmake.rs +++ b/tests/run-make/crate-loading/rmake.rs @@ -1,6 +1,7 @@ //@ only-linux //@ ignore-wasm32 //@ ignore-wasm64 +// ignore-tidy-linelength use run_make_support::{rust_lib_name, rustc}; @@ -18,19 +19,82 @@ fn main() { .extern_("dep_2_reexport", rust_lib_name("foo")) .run_fail() .assert_stderr_contains( - "there are multiple different versions of crate `dependency` in the dependency graph", + 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` the your 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( - "two types coming from two different versions of the same crate are different types \ - even if they look the same", + 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("this type doesn't implement the required trait") - .assert_stderr_contains("this type implements the required trait") - .assert_stderr_contains("this is the required trait") .assert_stderr_contains( - "`dependency` imported here doesn't correspond to the right crate version", + r#" +3 | pub struct Type; + | ^^^^^^^^^^^^^^^ this type doesn't implement the required trait"#, ) - .assert_stderr_contains("this is the trait that was imported") - .assert_stderr_contains("this is the trait that is needed") - .assert_stderr_contains("the method is available for `dep_2_reexport::Type` here"); + .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#" +4 | pub trait Trait { + | ^^^^^^^^^^^^^^^ this is the trait that is needed +5 | fn foo(&self); + | -------------- the method is available for `dep_2_reexport::Type` here + | + ::: multiple-dep-versions.rs:4:32 + | +4 | use dependency::{do_something, Trait}; + | ----- `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#" +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#" +4 | pub trait Trait { + | ^^^^^^^^^^^^^^^ this is the trait that is needed +5 | fn foo(&self); +6 | fn bar(); + | --------- the method is available for `dep_2_reexport::Type` here + | + ::: multiple-dep-versions.rs:4:32 + | +4 | use dependency::{do_something, Trait}; + | ----- `Trait` imported here doesn't correspond to the right version of crate `dependency`"#, + ); }