Skip to content
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

Point only to the identifiers in the typo suggestions of shadowed names instead of the entire struct #103560

Merged
merged 4 commits into from
Oct 30, 2022
Merged

Point only to the identifiers in the typo suggestions of shadowed names instead of the entire struct #103560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
be61f02
Add Span in TypoSuggestion and TypoCandidate
zbyrn Oct 25, 2022
0b936d2
Add check to only output 'you might have meant' when the candidate na…
zbyrn Oct 25, 2022
775328c
Modify check to output 'you might have meant' for indirect reference
zbyrn Oct 25, 2022
0eaf6d5
Modify ui tests to reflect the change
zbyrn Oct 25, 2022
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
45 changes: 31 additions & 14 deletions compiler/rustc_resolve/src/diagnostics.rs
View file
Open in desktop
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
Open in desktop
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))
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have an example where removing the is_local and is_in_same_file filters give the wrong suggestions?
TBH, I'm not fond of the filename-based logic. We should either formulate the logic in terms of modules, or drop is_in_same_file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we shadow something in another crate, the suggestion would potentially point at a distant file in which the shadowed name is defined. For example, compiling

mod m {
    pub enum MyEnum {
        Result
    }
}

use m::MyEnum::Result;

fn foo() -> Result<u8, str> {
    unimplemented!()
}

fn main() { }

will give the following error message:

error[E0573]: expected type, found variant `Result`
  --> test.rs:9:13
   |
9  | fn foo() -> Result<u8, str> {
   |             ^^^^^^^^^^^^^^^ not a type
   |
  ::: /[REDACTED]/rust/library/std/src/prelude/v1.rs:35:24
   |
35 | pub use crate::result::Result::{self, Err, Ok};
   |                        ------ you might have meant to refer to this enum
   |
help: consider importing one of these items instead
   |
7  | use std::fmt::Result;
   |
7  | use std::io::Result;
   |
7  | use std::result::Result;
   |
7  | use std::thread::Result;
   |

error: aborting due to previous error

For more information about this error, try `rustc --explain E0573`.

Note that the suggestion here points at something in the implementation of std. If this is okay, we could just remove both checks and simplify the code. The suggestion given here is not wrong technically, but I think it is not very helpful either.

I'm not a fan of the filename hack as well, and it was certainly a last resort as I'm still familiarizing myself with the code base. Could we achieve the same effect by comparing the ctx's of the two spans?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On a second thought, we could actually use the span of the resolver's module span (self.r.graph_root.span) to test whether the underlined definition is in the current module. Pushing a change and amending this PR shortly

Copy link
Contributor

Choose a reason for hiding this comment

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

graph_root.span is the span of the lib.rs file. Using it to decide is definitely wrong.
The former version of this PR is the better middle ground.
Could you remove the last commit, and I'll approve the PR. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, my bad for misunderstanding what graph_root means! A reversion has been pushed.

Thanks for your comments and your approval!

{
err.span_label(
sugg_span,
Expand Down Expand Up @@ -973,10 +984,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 @@ -1618,7 +1626,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 @@ -1647,9 +1655,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 @@ -1668,7 +1674,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 @@ -1695,7 +1701,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
Open in desktop
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
Open in desktop
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
Open in desktop
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