Skip to content

Commit

Permalink
Rollup merge of rust-lang#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 rust-lang#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 rust-lang#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.