Skip to content

Remove importing suggestions when there is a shadowed typo candidate #120966

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

Merged
merged 1 commit into from
Feb 14, 2024
+58 −28
Merged

Remove importing suggestions when there is a shadowed typo candidate #120966

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
62 changes: 39 additions & 23 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 @@ -460,7 +460,7 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> {
return (err, Vec::new());
}

let (found, candidates) = self.try_lookup_name_relaxed(
let (found, mut candidates) = self.try_lookup_name_relaxed(
&mut err,
source,
path,
Expand All @@ -473,10 +473,12 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> {
return (err, candidates);
}

let mut fallback = self.suggest_trait_and_bounds(&mut err, source, res, span, &base_error);
if self.suggest_shadowed(&mut err, source, path, following_seg, span) {
// if there is already a shadowed name, don'suggest candidates for importing
candidates.clear();
}

// if we have suggested using pattern matching, then don't add needless suggestions
// for typos.
let mut fallback = self.suggest_trait_and_bounds(&mut err, source, res, span, &base_error);
fallback |= self.suggest_typo(&mut err, source, path, following_seg, span, &base_error);

if fallback {
Expand Down Expand Up @@ -872,25 +874,6 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> {
let ident_span = path.last().map_or(span, |ident| ident.ident.span);
let typo_sugg =
self.lookup_typo_candidate(path, following_seg, source.namespace(), is_expected);
let is_in_same_file = &|sp1, sp2| {
let source_map = self.r.tcx.sess.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().is_some_and(|id| id.is_local() || is_in_same_file(span, sugg_span))
{
err.span_label(
sugg_span,
format!("you might have meant to refer to this {}", res.descr()),
);
return true;
}
let mut fallback = false;
let typo_sugg = typo_sugg.to_opt_suggestion();
if !self.r.add_typo_suggestion(err, typo_sugg, ident_span) {
Expand Down Expand Up @@ -918,6 +901,39 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> {
fallback
}

fn suggest_shadowed(
&mut self,
err: &mut Diagnostic,
source: PathSource<'_>,
path: &[Segment],
following_seg: Option<&Segment>,
span: Span,
) -> bool {
let is_expected = &|res| source.is_expected(res);
let typo_sugg =
self.lookup_typo_candidate(path, following_seg, source.namespace(), is_expected);
let is_in_same_file = &|sp1, sp2| {
let source_map = self.r.tcx.sess.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().is_some_and(|id| id.is_local() || is_in_same_file(span, sugg_span))
{
err.span_label(
sugg_span,
format!("you might have meant to refer to this {}", res.descr()),
);
return true;
}
false
}

fn err_code_special_cases(
&mut self,
err: &mut Diagnostic,
Expand Down
5 changes: 5 additions & 0 deletions tests/ui/resolve/issue-120559.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
use std::io::Read;

fn f<T: Read, U, Read>() {} //~ ERROR expected trait, found type parameter `Read`
Copy link
Member

@fmease fmease Feb 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We could suggest explicitly qualifying Read here if feasible: T: std::io::Read or even T: self::Read to pick up the otherwise unused import item. cc #109950 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

seems it's hard to give a detailed suggestion to fix the code, for this case all these 3 scenarios will compile successfully:

fn f<T: self::Read, U, Read>() {}

or

fn f<T: Read, U: Read>() {}

or

fn f<T: Read, U>() {}


fn main() {}
14 changes: 14 additions & 0 deletions tests/ui/resolve/issue-120559.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error[E0404]: expected trait, found type parameter `Read`
--> $DIR/issue-120559.rs:3:9
|
LL | use std::io::Read;
| ---- you might have meant to refer to this trait
LL |
LL | fn f<T: Read, U, Read>() {}
| ^^^^ ---- found this type parameter
| |
| not a trait

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0404`.
5 changes: 0 additions & 5 deletions tests/ui/span/issue-35987.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,6 @@ LL | impl<T: Clone, Add> Add for Foo<T> {
| --- ^^^ not a trait
Copy link
Member

Choose a reason for hiding this comment

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

An aside: I'm realizing now, looking at the code in question (and confirmed by the first line of the description of issue #35987):

impl<T: Clone, Add> Add for Foo<T> { ... }

that the most likely explanation for what has happened is that someone was trying to put both the Clone and Add bounds onto the type parameter T, and did not realize that the traits needed to be separated by + rather than ,.

In an ideal world, with full LLM linguistic modelling of the user's intent, we might hope to provide a hint that the comma, while syntatically correct, could instead have been a +.

(I know that Vadim already pointed out that the compiler "can't" complain about the comma in all cases, since it is syntactically correct there. I'm just saying to potentially give to the developer.)

| |
| found this type parameter
|
help: consider importing this trait instead
|
LL + use std::ops::Add;
|

error: aborting due to 1 previous error

Expand Down