Skip to content

Add a diagnostic for similarly named traits #144674

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+264 −245
Open

Add a diagnostic for similarly named traits #144674

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 14 additions & 44 deletions compiler/rustc_hir_typeck/src/method/suggest.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3478,7 +3478,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
err,
pick.item.def_id,
rcvr.hir_id,
Some(*rcvr_ty),
*rcvr_ty,
);
if pick.autoderefs == 0 && !trait_in_other_version_found {
err.span_label(
Expand Down Expand Up @@ -3686,6 +3686,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
// same crate.

let rcvr_ty = self.node_ty_opt(ty.hir_id);
let Some(rcvr_ty) = rcvr_ty else {
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this return changes behavior instead of only returning from the fn itself? is this intentional?

};
trait_in_other_version_found = self
.detect_and_explain_multiple_crate_versions_of_trait_item(
err,
Expand Down Expand Up @@ -4089,7 +4092,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
err: &mut Diag<'_>,
item_def_id: DefId,
hir_id: hir::HirId,
rcvr_ty: Option<Ty<'_>>,
rcvr_ty: Ty<'tcx>,
) -> bool {
let hir_id = self.tcx.parent_hir_id(hir_id);
let Some(traits) = self.tcx.in_scope_traits(hir_id) else { return false };
Expand All @@ -4100,49 +4103,16 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
if !self.tcx.is_trait(trait_def_id) {
return false;
}
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() {
let hir::Node::Expr(rcvr) = self.tcx.hir_node(hir_id) else {
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, "this is the trait that is needed".to_string());
let descr = self.tcx.associated_item(item_def_id).descr();
let rcvr_ty =
rcvr_ty.map(|t| format!("`{t}`")).unwrap_or_else(|| "the receiver".to_string());
multi_span
.push_span_label(item_span, format!("the {descr} 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),
"this is the trait that was imported".to_string(),
);
}
err.span_note(multi_span, msg);
true
};
let trait_ref = ty::TraitRef::new(self.tcx, trait_def_id, [rcvr_ty]);
let trait_pred = ty::Binder::dummy(ty::TraitPredicate {
trait_ref,
polarity: ty::PredicatePolarity::Positive,
});
let obligation = Obligation::new(self.tcx, self.misc(rcvr.span), self.param_env, trait_ref);
self.err_ctxt().note_different_trait_with_same_name(err, &obligation, trait_pred)
}

/// issue #102320, for `unwrap_or` with closure as argument, suggest `unwrap_or_else`
Expand Down
246 changes: 115 additions & 131 deletions compiler/rustc_trait_selection/src/error_reporting/traits/fulfillment_errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use rustc_errors::{
Applicability, Diag, ErrorGuaranteed, Level, MultiSpan, StashKey, StringPart, Suggestions,
pluralize, struct_span_code_err,
};
use rustc_hir::def_id::{DefId, LOCAL_CRATE, LocalDefId};
use rustc_hir::def_id::{DefId, LocalDefId};
use rustc_hir::intravisit::Visitor;
use rustc_hir::{self as hir, LangItem, Node};
use rustc_infer::infer::{InferOk, TypeTrace};
Expand Down Expand Up @@ -467,7 +467,7 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
span,
leaf_trait_predicate,
);
self.note_version_mismatch(&mut err, leaf_trait_predicate);
self.note_different_trait_with_same_name(&mut err, &obligation, leaf_trait_predicate);
self.suggest_remove_await(&obligation, &mut err);
self.suggest_derive(&obligation, &mut err, leaf_trait_predicate);

Expand Down Expand Up @@ -1948,115 +1948,6 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
impl_candidates
};

// We'll check for the case where the reason for the mismatch is that the trait comes from
// 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_pred.def_id();
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_including_private().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_pred.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()
// The current crate-version might depend on another version of the same crate
// (Think "semver-trick"). Do not call `extern_crate` in that case for the local
// crate as that doesn't make sense and ICEs (#133563).
.filter(|def_id| !def_id.is_local())
.filter_map(|def_id| self.tcx.extern_crate(def_id.krate))
.map(|data| {
let dependency = if data.dependency_of == LOCAL_CRATE {
"direct dependency of the current crate".to_string()
} else {
let dep = self.tcx.crate_name(data.dependency_of);
format!("dependency of crate `{dep}`")
};
(
data.span,
format!("one version of crate `{crate_name}` used here, as a {dependency}"),
)
})
{
span.push_span_label(sp, label);
}
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!("{crate_name}")),
StringPart::normal("` in the dependency graph".to_string()),
],
);
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 "
.to_string(),
),
StringPart::highlighted("even if they look the same".to_string()),
]);
}
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()),
]);
return true;
}

if let [single] = &impl_candidates {
// If we have a single implementation, try to unify it with the trait ref
// that failed. This should uncover a better hint for what *is* implemented.
Expand Down Expand Up @@ -2421,10 +2312,7 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
}
}

/// If the `Self` type of the unsatisfied trait `trait_ref` implements a trait
/// with the same path as `trait_ref`, a help message about
/// a probable version mismatch is added to `err`
fn note_version_mismatch(
fn check_same_trait_different_version(
&self,
err: &mut Diag<'_>,
trait_pred: ty::PolyTraitPredicate<'tcx>,
Expand All @@ -2441,38 +2329,134 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
trait_impls
};

let krate = self.tcx.crate_name(trait_pred.def_id().krate);
let name = self.tcx.item_name(trait_pred.def_id());
let required_trait_path = self.tcx.def_path_str(trait_pred.def_id());
let traits_with_same_path: UnordSet<_> = self
.tcx
.visible_traits()
.filter(|trait_def_id| *trait_def_id != trait_pred.def_id())
.filter(|trait_def_id| {
trait_def_id.krate != trait_pred.def_id().krate
&& (self.tcx.def_path_str(trait_def_id) == required_trait_path
|| self.tcx.crate_name(trait_def_id.krate) == krate
&& self.tcx.item_name(trait_def_id) == name)
Comment on lines +2340 to +2342
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when is self.tcx.crate_name(trait_def_id.krate) == krate && self.tcx.item_name(trait_def_id) == name false but self.tcx.def_path_str(trait_def_id) == required_trait_path holds?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It used by tests/ui/traits/bound/same-crate-name.rs , disabling this case makes the test fail.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so that's crate::mod::Trait and crate::Trait?

Wait, disabling which case? Each of the two cases should handle some distinct cases I would expect, what are the test covering eacvh of them and could we generalize the condition even further. It feels weird to have 2 different conditions if the rest of the diagnostic is exactly the same

})
.map(|trait_def_id| (self.tcx.def_path_str(trait_def_id), trait_def_id))
.filter(|(p, _)| *p == required_trait_path)
.collect();

let traits_with_same_path =
traits_with_same_path.into_items().into_sorted_stable_ord_by_key(|(p, _)| p);
let mut suggested = false;
for (_, trait_with_same_path) in traits_with_same_path {
let trait_impls = get_trait_impls(trait_with_same_path);
if trait_impls.is_empty() {
continue;
}
let impl_spans: Vec<_> =
trait_impls.iter().map(|impl_def_id| self.tcx.def_span(*impl_def_id)).collect();
err.span_help(
impl_spans,
format!("trait impl{} with same name found", pluralize!(trait_impls.len())),
let mut trait_is_impl = false;

if !traits_with_same_path.is_empty() {
let msg = format!(
"there are multiple different versions of crate `{krate}` in the dependency graph"
);
let trait_crate = self.tcx.crate_name(trait_with_same_path.krate);
let crate_msg =
format!("perhaps two different versions of crate `{trait_crate}` are being used?");
err.note(crate_msg);
let mut span: MultiSpan = self.tcx.def_span(trait_pred.def_id()).into();
span.push_span_label(
self.tcx.def_span(trait_pred.def_id()),
"this is the required trait",
);
suggested = true;
for (_, trait_with_same_path) in &traits_with_same_path {
let trait_impls = get_trait_impls(*trait_with_same_path);
if trait_impls.is_empty() {
continue;
}

for candidate_def_id in trait_impls {
let Some(impl_trait_header) = self.tcx.impl_trait_header(candidate_def_id)
else {
continue;
};
let candidate_span =
self.tcx.def_span(impl_trait_header.trait_ref.skip_binder().def_id);
span.push_span_label(candidate_span, "this is the implemented trait");
trait_is_impl = true;
}
}
if !trait_is_impl {
for (_, def_id) in traits_with_same_path {
span.push_span_label(
self.tcx.def_span(def_id),
"this is the trait that was imported",
);
}
}
err.span_note(span, msg);
}
suggested
}

fn check_same_name_different_path(
&self,
err: &mut Diag<'_>,
obligation: &PredicateObligation<'tcx>,
trait_pred: ty::PolyTraitPredicate<'tcx>,
) -> bool {
let mut suggested = false;
let trait_def_id = trait_pred.def_id();
let trait_has_same_params = |other_trait_def_id: DefId| -> bool {
let trait_generics = self.tcx.generics_of(trait_def_id);
let other_trait_generics = self.tcx.generics_of(other_trait_def_id);

if trait_generics.count() != other_trait_generics.count() {
return false;
}
trait_generics.own_params.iter().zip(other_trait_generics.own_params.iter()).all(
|(a, b)| {
(matches!(a.kind, ty::GenericParamDefKind::Type { .. })
&& matches!(b.kind, ty::GenericParamDefKind::Type { .. }))
|| (matches!(a.kind, ty::GenericParamDefKind::Lifetime,)
&& matches!(b.kind, ty::GenericParamDefKind::Lifetime))
|| (matches!(a.kind, ty::GenericParamDefKind::Const { .. })
&& matches!(b.kind, ty::GenericParamDefKind::Const { .. }))
},
)
};
let trait_name = self.tcx.item_name(trait_def_id);
if let Some(other_trait_def_id) = self.tcx.all_traits_including_private().find(|def_id| {
trait_def_id != *def_id
&& trait_name == self.tcx.item_name(def_id)
&& trait_has_same_params(*def_id)
&& self.predicate_must_hold_modulo_regions(&Obligation::new(
self.tcx,
obligation.cause.clone(),
obligation.param_env,
trait_pred.map_bound(|tr| ty::TraitPredicate {
trait_ref: ty::TraitRef::new(self.tcx, *def_id, tr.trait_ref.args),
..tr
}),
))
}) {
err.note(format!(
"`{}` implements similarly named `{}`, but not `{}`",
trait_pred.self_ty(),
self.tcx.def_path_str(other_trait_def_id),
trait_pred.print_modifiers_and_trait_path()
));
suggested = true;
}
suggested
}

/// If the `Self` type of the unsatisfied trait `trait_ref` implements a trait
/// with the same path as `trait_ref`, a help message about a multiple different
/// versions of the same crate is added to `err`. Otherwise if it implements another
/// trait with the same name, a note message about a similarly named trait is added to `err`.
pub fn note_different_trait_with_same_name(
&self,
err: &mut Diag<'_>,
obligation: &PredicateObligation<'tcx>,
trait_pred: ty::PolyTraitPredicate<'tcx>,
) -> bool {
if self.check_same_trait_different_version(err, trait_pred) {
return true;
}
self.check_same_name_different_path(err, obligation, trait_pred)
}

/// Creates a `PredicateObligation` with `new_self_ty` replacing the existing type in the
/// `trait_ref`.
///
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_trait_selection/src/error_reporting/traits/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
pub mod ambiguity;
pub mod call_kind;
mod fulfillment_errors;
pub mod fulfillment_errors;
pub mod on_unimplemented;
pub mod on_unimplemented_condition;
pub mod on_unimplemented_format;
Expand Down
Loading
Loading