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

Do not re-parse function signatures to suggest generics #99249

Merged
merged 6 commits into from
Aug 23, 2022
Merged

Do not re-parse function signatures to suggest generics #99249

merged 6 commits into from
Aug 23, 2022

Conversation

cjgillot
Copy link
Contributor

This PR uses the existing resolution rib infrastructure to channel the correct span information to suggest generic parameters. This allows to avoid re-parsing a function's source code.

Drive-by cleanup: this removes useless FnItemRibKind from late resolution ribs. All the use cases are already covered by ItemRibKind and AssocItemRibKind which have more precise semantics.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 14, 2022
@rust-highfive
Copy link
Collaborator

r? @wesleywiser

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 14, 2022
@bors
Copy link
Contributor

bors commented Jul 25, 2022

☔ The latest upstream changes (presumably #97313) made this pull request unmergeable. Please resolve the merge conflicts.

@cjgillot
Copy link
Contributor Author

Re-rolling the dice.
r? compiler

@rust-highfive rust-highfive assigned nagisa and unassigned wesleywiser Aug 21, 2022
@cjgillot
Copy link
Contributor Author

nagisa is on vacation.
r? compiler

@rust-highfive rust-highfive assigned fee1-dead and unassigned nagisa Aug 21, 2022
fee1-dead
fee1-dead reviewed Aug 21, 2022
View reviewed changes
compiler/rustc_resolve/src/diagnostics.rs
} else {
err.help("try using a local generic parameter instead");
}
let span = sm.span_through_char(span, '<').shrink_to_hi();
Copy link
Member

Choose a reason for hiding this comment

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

Is this going to work if there are lifetime parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It won't. I'll file an issue to track this (the former implementation did not support this either).

src/test/ui/error-codes/E0401.stderr
| help: try using a local generic parameter instead: `bfnr<U, V: Baz<U>, W: Fn(), T>`
| - ^ use of generic parameter from outer function
| |
| help: try using a local generic parameter instead: `T,`
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the space gets trimmed. Should we re-emit the whole generics so that it is more accurate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I already find this suggestion quite verbose as it is. Forcing a verbose diagnostic may be a bit too much, wouldn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

This output is fine. At most, we should make it a span_suggestion_verbose to get the diff output which I find clearer for these kind of additions, instead of modifying the span/suggestion to make it work better with the inline suggestion.

compiler/rustc_resolve/src/diagnostics.rs Outdated Show resolved Hide resolved
@fee1-dead
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Aug 23, 2022

📌 Commit dff4280 has been approved by fee1-dead

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 Aug 23, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Aug 23, 2022
@Dylan-DPC
Rollup merge of rust-lang#99249 - cjgillot:no-reparse-fn, r=fee1-dead
d3d8463 
Do not re-parse function signatures to suggest generics

This PR uses the existing resolution rib infrastructure to channel the correct span information to suggest generic parameters.  This allows to avoid re-parsing a function's source code.

Drive-by cleanup: this removes useless `FnItemRibKind` from late resolution ribs.  All the use cases are already covered by `ItemRibKind` and `AssocItemRibKind` which have more precise semantics.
@Dylan-DPC Dylan-DPC mentioned this pull request Aug 23, 2022
Closed
@cjgillot cjgillot mentioned this pull request Aug 23, 2022
Closed
@TaKO8Ki TaKO8Ki linked an issue Aug 23, 2022 that may be closed by this pull request
thread 'rustc' panicked at 'no label after fn' #100875
Closed
@Dylan-DPC Dylan-DPC mentioned this pull request Aug 23, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 23, 2022
@bors
Auto merge of rust-lang#100920 - Dylan-DPC:rollup-vlcw3sr, r=Dylan-DPC
a1bea15 
Rollup of 9 pull requests

Successful merges:

 - rust-lang#99249 (Do not re-parse function signatures to suggest generics)
 - rust-lang#100309 (Extend comma suggestion to cases where fields arent missing)
 - rust-lang#100368 (InferCtxt tainted_by_errors_flag should be Option<ErrorGuaranteed>)
 - rust-lang#100768 (Migrate `rustc_plugin_impl` to `SessionDiagnostic`)
 - rust-lang#100835 (net listen backlog update, follow-up from rust-lang#97963.)
 - rust-lang#100851 (Fix rustc_parse_format precision & width spans)
 - rust-lang#100857 (Refactor query modifier parsing)
 - rust-lang#100907 (Fix typo in UnreachableProp)
 - rust-lang#100909 (Minor `ast::LitKind` improvements)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4d1c273 into rust-lang:master Aug 23, 2022
@rustbot rustbot added this to the 1.65.0 milestone Aug 23, 2022
estebank
estebank reviewed Aug 23, 2022
View reviewed changes
src/test/ui/generics/issue-98432.stderr
Comment on lines 6 to +10
LL | const CONST: fn() = || {
LL | struct _Obligation where T:;
| ^ use of generic parameter from outer function
|
= help: try using a local generic parameter instead
| - ^ use of generic parameter from outer function
| |
| help: try using a local generic parameter instead: `<T>`
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an unfortunate case.

@cjgillot cjgillot deleted the no-reparse-fn branch August 23, 2022 22:06
@steffahn steffahn mentioned this pull request Jul 31, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@estebank estebank estebank left review comments

@compiler-errors compiler-errors compiler-errors left review comments

@fee1-dead fee1-dead fee1-dead approved these changes

Assignees

@fee1-dead fee1-dead

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.65.0
Development

Successfully merging this pull request may close these issues.

thread 'rustc' panicked at 'no label after fn'
9 participants
@cjgillot @rust-highfive @bors @fee1-dead @estebank @compiler-errors @nagisa @wesleywiser @rustbot