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

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

Merged
merged 1 commit into from
Feb 14, 2024
Merged

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

merged 1 commit into from
Feb 14, 2024

Conversation

chenyukang
Copy link
Member

@chenyukang chenyukang commented Feb 12, 2024 •
edited
Loading

Fixes #120559

@rustbot
Copy link
Collaborator

rustbot commented Feb 12, 2024

r? @pnkfelix

rustbot has assigned @pnkfelix.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 12, 2024
@chenyukang chenyukang changed the title remove importing suggestions when there is a shadowed typo candidate Remove importing suggestions when there is a shadowed typo candidate Feb 12, 2024
pnkfelix
pnkfelix reviewed Feb 12, 2024
View reviewed changes
compiler/rustc_resolve/src/late/diagnostics.rs Outdated
@@ -867,6 +875,7 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> {
following_seg: Option<&Segment>,
span: Span,
base_error: &BaseError,
candidates: &mut Vec<ImportSuggestion>,
Copy link
Member

Choose a reason for hiding this comment

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

You need, at the very least, to add a comment above fn suggest_typo that describes in what scenarios the input candidates will be modified, and what kind of modification it will undergo.

From reading the code change here, this looks like a very blunt instrument of outright clearing the candidates in the presence of a shadowed name.

Are you planning on doing something more sophisticated to candidates in the long term (e.g. to still suggest some new imports that are based on some small revision to the given symbol)?

If that is not the case, and all you intend is to just clear the candidate set, I think this interface is more confusing than it needs to be. I would consider something else, e.g. returning something richer than a bool as the return value, and then letting the caller interpret that bool in deciding whether to return the candidates, or to return a fresh vec![].

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, &mut makes the functions harder to reason.
I think splitting out the code logic about suggesting shadowed names makes it more readable.
I updated the code and removed an outdated comment by the way.

// if we have suggested using pattern matching, then don't add needless suggestions
// for typos.

@fmease
Copy link
Member

fmease commented Feb 13, 2024

This also fixes #109950.

fmease
fmease reviewed Feb 13, 2024
View reviewed changes
tests/ui/resolve/issue-120559.rs
@@ -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>() {}

@pnkfelix
Copy link
Member

I don't have feedback beyond the request I wrote above regarding the change to function signature.

@rustbot label: -S-waiting-on-review +S-waiting-on-author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 13, 2024
@chenyukang chenyukang force-pushed the yukang-fix-120599-resolve branch 2 times, most recently from 9461800 to 15f1cdc Compare February 14, 2024 03:45
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@chenyukang
Copy link
Member Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 14, 2024
pnkfelix
pnkfelix reviewed Feb 14, 2024
View reviewed changes
tests/ui/span/issue-35987.stderr
@@ -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.)

@pnkfelix
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 14, 2024

📌 Commit 2a229c9 has been approved by pnkfelix

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 14, 2024
@GuillaumeGomez GuillaumeGomez mentioned this pull request Feb 14, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 14, 2024
@bors
Auto merge of rust-lang#121086 - GuillaumeGomez:rollup-y82fs9y, r=Gui…
502ce82 
…llaumeGomez

Rollup of 6 pull requests

Successful merges:

 - rust-lang#120893 (Move some tests)
 - rust-lang#120966 (Remove importing suggestions when there is a shadowed typo candidate)
 - rust-lang#121035 (Format `async` trait bounds in rustfmt)
 - rust-lang#121075 (Fix false positive with if let and ranges)
 - rust-lang#121083 (Extend documentation for `Ty::to_opt_closure_kind` method)
 - rust-lang#121084 (Make sure `tcx.create_def` also depends on the forever red node, instead of just `tcx.at(span).create_def`)

r? `@ghost`
`@rustbot` modify labels: rollup