Skip to content

Commit

Permalink
Rollup merge of #103560 - zbyrn:issue-103358-fix, r=cjgillot
Browse files Browse the repository at this point in the history 
Point only to the identifiers in the typo suggestions of shadowed names instead of the entire struct

Fixes #103358.

As discussed in the issue, the `Span` of the candidate `Ident` for a typo replacement is stored alongside its `Symbol` in `TypoSuggestion`. Then, the span of the identifier is what the "you might have meant to refer to" note is pointed at, rather than the entire struct definition.

Comments in #103111 and the issue both suggest that it is desirable to:
1. include names defined in the same crate as the typo,
2. ignore names defined elsewhere such as in `std`, _and_
3. include names introduced indirectly via `use`.

Since a name from another crate but introduced via `use` has non-local `def_id`, to achieve this, a suggestion is displayed if either the `def_id` of the suggested name is local, or the `span` of the suggested name is in the same file as the typo itself.

Some UI tests have also been modified to reflect this change.

r? `@cjgillot`
  • Loading branch information
@Dylan-DPC
Dylan-DPC authored Oct 30, 2022
2 parents b4cf523 + 0eaf6d5 commit 3143472
Show file tree
Hide file tree
Showing 5 changed files with 63 additions and 39 deletions.
45 changes: 31 additions & 14 deletions compiler/rustc_resolve/src/diagnostics.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,16 +58,32 @@ pub(crate) enum SuggestionTarget {
#[derive(Debug)]
pub(crate) struct TypoSuggestion {
pub candidate: Symbol,
/// The source location where the name is defined; None if the name is not defined
/// in source e.g. primitives
pub span: Option<Span>,
pub res: Res,
pub target: SuggestionTarget,
}

impl TypoSuggestion {
pub(crate) fn typo_from_res(candidate: Symbol, res: Res) -> TypoSuggestion {
Self { candidate, res, target: SuggestionTarget::SimilarlyNamed }
pub(crate) fn typo_from_ident(ident: Ident, res: Res) -> TypoSuggestion {
Self {
candidate: ident.name,
span: Some(ident.span),
res,
target: SuggestionTarget::SimilarlyNamed,
}
}
pub(crate) fn typo_from_name(candidate: Symbol, res: Res) -> TypoSuggestion {
Self { candidate, span: None, res, target: SuggestionTarget::SimilarlyNamed }
}
pub(crate) fn single_item_from_res(candidate: Symbol, res: Res) -> TypoSuggestion {
Self { candidate, res, target: SuggestionTarget::SingleItem }
pub(crate) fn single_item_from_ident(ident: Ident, res: Res) -> TypoSuggestion {
Self {
candidate: ident.name,
span: Some(ident.span),
res,
target: SuggestionTarget::SingleItem,
}
}
}

Expand Down Expand Up @@ -490,7 +506,7 @@ impl<'a> Resolver<'a> {
if let Some(binding) = resolution.borrow().binding {
let res = binding.res();
if filter_fn(res) && ctxt.map_or(true, |ctxt| ctxt == key.ident.span.ctxt()) {
names.push(TypoSuggestion::typo_from_res(key.ident.name, res));
names.push(TypoSuggestion::typo_from_ident(key.ident, res));
}
}
}
Expand Down Expand Up @@ -1145,7 +1161,7 @@ impl<'a> Resolver<'a> {
.get(&expn_id)
.into_iter()
.flatten()
.map(|ident| TypoSuggestion::typo_from_res(ident.name, res)),
.map(|ident| TypoSuggestion::typo_from_ident(*ident, res)),
);
}
}
Expand All @@ -1164,7 +1180,7 @@ impl<'a> Resolver<'a> {
suggestions.extend(
ext.helper_attrs
.iter()
.map(|name| TypoSuggestion::typo_from_res(*name, res)),
.map(|name| TypoSuggestion::typo_from_name(*name, res)),
);
}
}
Expand All @@ -1174,8 +1190,8 @@ impl<'a> Resolver<'a> {
if let MacroRulesScope::Binding(macro_rules_binding) = macro_rules_scope.get() {
let res = macro_rules_binding.binding.res();
if filter_fn(res) {
suggestions.push(TypoSuggestion::typo_from_res(
macro_rules_binding.ident.name,
suggestions.push(TypoSuggestion::typo_from_ident(
macro_rules_binding.ident,
res,
))
}
Expand All @@ -1193,7 +1209,7 @@ impl<'a> Resolver<'a> {
suggestions.extend(this.macro_use_prelude.iter().filter_map(
|(name, binding)| {
let res = binding.res();
filter_fn(res).then_some(TypoSuggestion::typo_from_res(*name, res))
filter_fn(res).then_some(TypoSuggestion::typo_from_name(*name, res))
},
));
}
Expand All @@ -1203,22 +1219,22 @@ impl<'a> Resolver<'a> {
suggestions.extend(
BUILTIN_ATTRIBUTES
.iter()
.map(|attr| TypoSuggestion::typo_from_res(attr.name, res)),
.map(|attr| TypoSuggestion::typo_from_name(attr.name, res)),
);
}
}
Scope::ExternPrelude => {
suggestions.extend(this.extern_prelude.iter().filter_map(|(ident, _)| {
let res = Res::Def(DefKind::Mod, CRATE_DEF_ID.to_def_id());
filter_fn(res).then_some(TypoSuggestion::typo_from_res(ident.name, res))
filter_fn(res).then_some(TypoSuggestion::typo_from_ident(*ident, res))
}));
}
Scope::ToolPrelude => {
let res = Res::NonMacroAttr(NonMacroAttrKind::Tool);
suggestions.extend(
this.registered_tools
.iter()
.map(|ident| TypoSuggestion::typo_from_res(ident.name, res)),
.map(|ident| TypoSuggestion::typo_from_ident(*ident, res)),
);
}
Scope::StdLibPrelude => {
Expand All @@ -1235,7 +1251,8 @@ impl<'a> Resolver<'a> {
Scope::BuiltinTypes => {
suggestions.extend(PrimTy::ALL.iter().filter_map(|prim_ty| {
let res = Res::PrimTy(*prim_ty);
filter_fn(res).then_some(TypoSuggestion::typo_from_res(prim_ty.name(), res))
filter_fn(res)
.then_some(TypoSuggestion::typo_from_name(prim_ty.name(), res))
}))
}
}
Expand Down
36 changes: 21 additions & 15 deletions compiler/rustc_resolve/src/late/diagnostics.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,15 +150,15 @@ struct BaseError {
#[derive(Debug)]
enum TypoCandidate {
Typo(TypoSuggestion),
Shadowed(Res),
Shadowed(Res, Option<Span>),
None,
}

impl TypoCandidate {
fn to_opt_suggestion(self) -> Option<TypoSuggestion> {
match self {
TypoCandidate::Typo(sugg) => Some(sugg),
TypoCandidate::Shadowed(_) | TypoCandidate::None => None,
TypoCandidate::Shadowed(_, _) | TypoCandidate::None => None,
}
}
}
Expand Down Expand Up @@ -691,9 +691,20 @@ impl<'a: 'ast, 'ast> LateResolutionVisitor<'a, '_, 'ast> {
let is_expected = &|res| source.is_expected(res);
let ident_span = path.last().map_or(span, |ident| ident.ident.span);
let typo_sugg = self.lookup_typo_candidate(path, source.namespace(), is_expected);
if let TypoCandidate::Shadowed(res) = typo_sugg
&& let Some(id) = res.opt_def_id()
&& let Some(sugg_span) = self.r.opt_span(id)
let is_in_same_file = &|sp1, sp2| {
let source_map = self.r.session.source_map();
let file1 = source_map.span_to_filename(sp1);
let file2 = source_map.span_to_filename(sp2);
file1 == file2
};
// print 'you might have meant' if the candidate is (1) is a shadowed name with
// accessible definition and (2) either defined in the same crate as the typo
// (could be in a different file) or introduced in the same file as the typo
// (could belong to a different crate)
if let TypoCandidate::Shadowed(res, Some(sugg_span)) = typo_sugg
&& res
.opt_def_id()
.map_or(false, |id| id.is_local() || is_in_same_file(span, sugg_span))
{
err.span_label(
sugg_span,
Expand Down Expand Up @@ -970,10 +981,7 @@ impl<'a: 'ast, 'ast> LateResolutionVisitor<'a, '_, 'ast> {
.collect();
if targets.len() == 1 {
let target = targets[0];
return Some(TypoSuggestion::single_item_from_res(
target.0.ident.name,
target.1,
));
return Some(TypoSuggestion::single_item_from_ident(target.0.ident, target.1));
}
}
}
Expand Down Expand Up @@ -1615,7 +1623,7 @@ impl<'a: 'ast, 'ast> LateResolutionVisitor<'a, '_, 'ast> {
// Locals and type parameters
for (ident, &res) in &rib.bindings {
if filter_fn(res) && ident.span.ctxt() == rib_ctxt {
names.push(TypoSuggestion::typo_from_res(ident.name, res));
names.push(TypoSuggestion::typo_from_ident(*ident, res));
}
}

Expand Down Expand Up @@ -1644,9 +1652,7 @@ impl<'a: 'ast, 'ast> LateResolutionVisitor<'a, '_, 'ast> {
Res::Def(DefKind::Mod, crate_id.as_def_id());

if filter_fn(crate_mod) {
Some(TypoSuggestion::typo_from_res(
ident.name, crate_mod,
))
Some(TypoSuggestion::typo_from_ident(*ident, crate_mod))
} else {
None
}
Expand All @@ -1665,7 +1671,7 @@ impl<'a: 'ast, 'ast> LateResolutionVisitor<'a, '_, 'ast> {
// Add primitive types to the mix
if filter_fn(Res::PrimTy(PrimTy::Bool)) {
names.extend(PrimTy::ALL.iter().map(|prim_ty| {
TypoSuggestion::typo_from_res(prim_ty.name(), Res::PrimTy(*prim_ty))
TypoSuggestion::typo_from_name(prim_ty.name(), Res::PrimTy(*prim_ty))
}))
}
} else {
Expand All @@ -1692,7 +1698,7 @@ impl<'a: 'ast, 'ast> LateResolutionVisitor<'a, '_, 'ast> {
return TypoCandidate::None;
};
if found == name {
TypoCandidate::Shadowed(sugg.res)
TypoCandidate::Shadowed(sugg.res, sugg.span)
} else {
TypoCandidate::Typo(sugg)
}
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/lexical-scopes.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ error[E0574]: expected struct, variant or union type, found type parameter `T`
--> $DIR/lexical-scopes.rs:3:13
|
LL | struct T { i: i32 }
| ------------------- you might have meant to refer to this struct
| - you might have meant to refer to this struct
LL | fn f<T>() {
| - found this type parameter
LL | let t = T { i: 0 };
Expand Down
16 changes: 7 additions & 9 deletions src/test/ui/resolve/point-at-type-parameter-shadowing-another-type.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,16 +1,14 @@
error[E0574]: expected struct, variant or union type, found type parameter `Baz`
--> $DIR/point-at-type-parameter-shadowing-another-type.rs:16:13
|
LL | / struct Baz {
LL | | num: usize,
LL | | }
| |_- you might have meant to refer to this struct
LL |
LL | impl<Baz> Foo<Baz> for Bar {
| --- found this type parameter
LL | struct Baz {
| --- you might have meant to refer to this struct
...
LL | Baz { num } => num,
| ^^^ not a struct, variant or union type
LL | impl<Baz> Foo<Baz> for Bar {
| --- found this type parameter
...
LL | Baz { num } => num,
| ^^^ not a struct, variant or union type

error: aborting due to previous error

Expand Down
3 changes: 3 additions & 0 deletions src/test/ui/span/issue-35987.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
error[E0404]: expected trait, found type parameter `Add`
--> $DIR/issue-35987.rs:5:21
|
LL | use std::ops::Add;
| --- you might have meant to refer to this trait
LL |
LL | impl<T: Clone, Add> Add for Foo<T> {
| --- ^^^ not a trait
| |
Expand Down

0 comments on commit 3143472

Please sign in to comment.