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

[zbyrn]

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

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cjgillot (or someone else) soon.

Please see the contribution instructions for more information.

[cjgillot]

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.

[zbyrn]

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?

[zbyrn]

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

[cjgillot]

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

[zbyrn]

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

Thanks for your comments and your approval!

[zbyrn]

@cjgillot I believe everything is ready

[cjgillot]

Could you squash the commits to remove the back-and-forth?
Thanks.

[zbyrn]

@cjgillot Done! Sorry for the confusion

[cjgillot]

@bors r+

[bors]

📌 Commit 0eaf6d5 has been approved by cjgillot

It is now in the queue for this repository.