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

Jump to bottom

Point at shadowed name binding in you might have meant to refer to suggestion #103358

Closed
cjgillot opened this issue Oct 21, 2022 · 3 comments · Fixed by #103560
Closed

Point at shadowed name binding in you might have meant to refer to suggestion #103358

cjgillot opened this issue Oct 21, 2022 · 3 comments · Fixed by #103560
Assignees
@zbyrn
Labels
A-diagnostics Area: Messages for errors, warnings, and lints D-papercut Diagnostics: An error or lint that needs small tweaks. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@cjgillot
Copy link
Contributor

cjgillot commented Oct 21, 2022 •
edited by rustbot
Loading 

    Makes sense, I just would rather we pointed at the ident for the item and not the whole thing.

Originally posted by @estebank in #103111 (comment)

The suggestion was introduced by #103111. The suggestion point to the definition of the suggested item. Pointing to the shadowed identifier would be more precise:

  • the ident of the item in case of direct reference;
  • the import in case of indirect reference.
@estebank estebank added A-diagnostics Area: Messages for errors, warnings, and lints P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. D-papercut Diagnostics: An error or lint that needs small tweaks. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. labels Oct 21, 2022
@fee1-dead
Copy link
Member

For this to work, either we would try to look at the DefKind of the Res as according to the docs, and then make the span point to the item name using the SourceMap if we know how the item looks. Alternatively, we could use guess_head_span to make it a bit more precise. Am I missing something here? Would there be a better way to get the span of the ident of the item?

@cjgillot
Copy link
Contributor Author

There is probably a simpler way, by storing the Span of the Ident that introduced the name in TypoSuggestion, and use that span instead of trying to get one from the Res.

@zbyrn
Copy link
Contributor

zbyrn commented Oct 24, 2022

I would like to work on this! Let me know if this is work in progress for someone else!
@rustbot claim

@zbyrn zbyrn mentioned this issue Oct 26, 2022
Merged
@bors bors closed this as completed in 3143472 Oct 30, 2022
Aaron1011 pushed a commit to Aaron1011/rust that referenced this issue Jan 6, 2023
@Dylan-DPC
Rollup merge of rust-lang#103560 - zbyrn:issue-103358-fix, r=cjgillot
591daa9 
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`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@zbyrn zbyrn

Labels
A-diagnostics Area: Messages for errors, warnings, and lints D-papercut Diagnostics: An error or lint that needs small tweaks. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
4 participants
@estebank @cjgillot @zbyrn @fee1-dead