Skip to content

Point out shadowed associated types on E0191 #117642

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

Closed
wants to merge 4 commits into from
Closed

Point out shadowed associated types on E0191 #117642

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2aa897f
Added shadowed hint for overlapping associated types
TheLazyDutchman Nov 5, 2023
a41cd83
Added help message to suggest renaming
TheLazyDutchman Nov 6, 2023
40064db
Added rename help message on subtrait
TheLazyDutchman Nov 6, 2023
e9a900d
Remove rename suggestions on unrelated associated types
TheLazyDutchman Nov 6, 2023
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
50 changes: 49 additions & 1 deletion compiler/rustc_hir_analysis/src/astconv/errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use rustc_errors::{pluralize, struct_span_err, Applicability, Diagnostic, ErrorG
use rustc_hir as hir;
use rustc_hir::def_id::{DefId, LocalDefId};
use rustc_infer::traits::FulfillmentError;
use rustc_middle::ty::{self, suggest_constraining_type_param, Ty, TyCtxt};
use rustc_middle::ty::{self, suggest_constraining_type_param, AssocItem, Ty, TyCtxt};
use rustc_session::parse::feature_err;
use rustc_span::edit_distance::find_best_match_for_name;
use rustc_span::symbol::{sym, Ident};
Expand Down Expand Up @@ -513,6 +513,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
if associated_types.values().all(|v| v.is_empty()) {
return;
}

let tcx = self.tcx();
// FIXME: Marked `mut` so that we can replace the spans further below with a more
// appropriate one, but this should be handled earlier in the span assignment.
Expand Down Expand Up @@ -585,6 +586,24 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
}
}

let bound_names = trait_bounds
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain what this code is doing? Some comments throuhout may be helpful.

.iter()
.flat_map(|poly_trait_ref| {
poly_trait_ref.trait_ref.path.segments.iter().flat_map(|x| {
x.args.iter().flat_map(|args| {
Copy link
Member

Choose a reason for hiding this comment

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

Why are you looking at all of the segments in the trait ref? That shouldn't be necessary. Only the final path segment should be interesting here.

Copy link
Member

Choose a reason for hiding this comment

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

Also, please don't iter() over an Option -- if let or let .. else should work here more effectively.

args.bindings.iter().map(|binding| {
let name = binding.ident.name;
let trait_def = poly_trait_ref.trait_ref.path.res.opt_def_id();
let assoc_item = trait_def.and_then(|did| {
tcx.associated_items(did).filter_by_name_unhygienic(name).next()
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't be filtering unhygenically. You have an identifier in binding.ident.

});
(name, assoc_item)
})
})
})
})
.collect::<FxHashMap<Symbol, Option<&AssocItem>>>();

let mut names = names
.into_iter()
.map(|(trait_, mut assocs)| {
Expand Down Expand Up @@ -614,6 +633,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
pluralize!(names_len),
names,
);
let mut rename_suggestions = vec![];
let mut suggestions = vec![];
let mut types_count = 0;
let mut where_constraints = vec![];
Expand All @@ -625,23 +645,47 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
*names.entry(item.name).or_insert(0) += 1;
}
let mut dupes = false;
let mut shadows = false;
for item in assoc_items {
let prefix = if names[&item.name] > 1 {
let trait_def_id = item.container_id(tcx);
dupes = true;
format!("{}::", tcx.def_path_str(trait_def_id))
} else if bound_names.contains_key(&item.name) {
let trait_def_id = item.container_id(tcx);
shadows = true;
format!("{}::", tcx.def_path_str(trait_def_id))
} else {
String::new()
};
if let Some(sp) = tcx.hir().span_if_local(item.def_id) {
err.span_label(sp, format!("`{}{}` defined here", prefix, item.name));
}

if let Some(Some(assoc_item)) = bound_names.get(&item.name) {
err.span_label(
tcx.def_span(assoc_item.def_id),
format!("`{}{}` shadowed here", prefix, item.name),
);
}
}
if potential_assoc_types.len() == assoc_items.len() {
// When the amount of missing associated types equals the number of
// extra type arguments present. A suggesting to replace the generic args with
// associated types is already emitted.
already_has_generics_args_suggestion = true;
} else if shadows {
for item in assoc_items {
if let Some(Some(assoc_item)) = bound_names.get(&item.name) {
if let Some(sp) = tcx.hir().span_if_local(item.def_id) {
rename_suggestions.push(sp);
}

if let Some(sp) = tcx.hir().span_if_local(assoc_item.def_id) {
rename_suggestions.push(sp);
}
}
}
} else if let (Ok(snippet), false) =
(tcx.sess.source_map().span_to_snippet(*span), dupes)
{
Expand Down Expand Up @@ -725,6 +769,10 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
err.span_help(where_constraints, where_msg);
}
}

for span in rename_suggestions {
err.span_help(span, "consider renaming this associated type");
}
err.emit();
}
}
Expand Down