Suggest fix for misplaced generic params on fn item #103366 #103478

SpanishPear · 2022-10-24T12:50:37Z

This still has some work to go, but works for 2/3 of the initial base cases described in #1033366

simple fn:

error: expected identifier, found `<`
 --> shreys/test_1.rs:1:3
  |
1 | fn<T> id(x: T) -> T { x }
  |   ^ expected identifier
  |
help: help: place the generic parameter list after the function name:
  |
1 | fn id<T>(x: T) -> T { x }
  |    ~~~~

Complicated bounds

error: expected identifier, found `<`
 --> spanishpear/test_2.rs:1:3
  |
1 | fn<'a, B: 'a + std::ops::Add<Output = u32>> f(_x: B) { }
  |   ^ expected identifier
  |
help: help: place the generic parameter list after the function name:
  |
1 | fn f<'a, B: 'a + std::ops::Add<Output = u32>>(_x: B) { }
  |    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Opening a draft PR for comments on approach, particularly I have the following questions:

Is it okay to be using err.span_suggestion over struct derives? I struggled to get the initial implementation (particularly the correct suggestion message) on struct derives, although I think given what I've learned since starting, I could attempt re-doing it with that approach.
in the case where the snippet cannot be obtained from a span, is the help but no suggestion okay? I think yes (also, when does this case occur?)
are there any red flags for the generalisation of this work for relevant item kinds (i.e. struct, enum, trait, and union). My basic testing indicates it does work for those types except the help tip is currently hardcoded to after the function name - which should change dependent on the item.
I am planning to not show the suggestion if there is already a < after the item identifier, (i.e. if there are already generics, as after a function name per the original issue). Any major objections?
Is the style of error okay? I wasn't sure if there was a way to make it display nicer, or if thats handled by span_suggestion

These aren't blocking questions, and I will keep working on:

check if there is a < after the ident (and if so, not showing the suggestion)
generalize the help message
figuring out how to write/run/etc ui tests (including reading the docs for them)
logic cleanups

rust-highfive · 2022-10-24T12:50:41Z

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

Please see the contribution instructions for more information.

compiler/rustc_error_messages/locales/en-US/parser.ftl

SpanishPear · 2022-10-29T10:08:36Z

Marking this as ready for review - only note is that this does not currently give help/suggestions in the case of a union - as unions are parsed differently, and I got dizzy trying to figure out how to generalize the code to include unions as well.

If this PR gets approved, I can look at tackling the refactor to work with unions in future! 🥳

Alternatively if desired, I can look into doing all in the same PR - but I will be unable to dev for the next ~3 months as I'll be on holiday with a very tiny dev machine 😂

SpanishPear · 2022-11-12T20:17:13Z

ping <3

TaKO8Ki · 2022-11-15T08:00:22Z

Sorry for the late reply. I will review in a day or two.

SpanishPear · 2022-11-15T11:48:22Z

Thank you :D

…

On Tue, Nov 15, 2022 at 7:00 PM Takayuki Maeda ***@***.***> wrote: Sorry for the late reply. I will review in a day or two. — Reply to this email directly, view it on GitHub <#103478 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGVFWMOP34GMGVKIYCCCFXTWIM7KDANCNFSM6AAAAAARM6IPHA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

TaKO8Ki · 2022-11-18T02:48:29Z

compiler/rustc_parse/src/parser/diagnostics.rs

+                                        generic.span.to(self.token.span),
+                                        format!("place the generic parameter name after the {ident_name} name"),
+                                        format!(" {ident}{snippet}"),


Could you use a smaller span maybe like this?

Suggested change

generic.span.to(self.token.span),

format!("place the generic parameter name after the {ident_name} name"),

format!(" {ident}{snippet}"),

self.token.span.shrink_to_hi(),

format!("place the generic parameter name after the {ident_name} name"),

snippet,

Example

error: expected identifier, found `<` --> $DIR/fn-simple.rs:5:3 | LL | fn<T> id(x: T) -> T { x } | ^ expected identifier | help: place the generic parameter name after the fn name | LL | fn id<T>(x: T) -> T { x } | ~~~ error: aborting due to previous error

The suggestion ends up with

error: expected identifier, found `<` --> $DIR/fn-simple.rs:5:3 | LL | fn<T> id(x: T) -> T { x } | ^ expected identifier | help: place the generic parameter name after the fn name | LL | fn<T> id<T>(x: T) -> T { x } | +++ error: aborting due to previous error

If you want colors:

notably since we are inserting a suggestion (<T>) at the end of id - we don't remove the original <T> on the right of the fn.

I looked around in the compiler docs, but couldn't quite see a way to do the insertion as before, but able to highlight a portion of the suggestion. The initial PR does the correct insertion (i.e. remove the invalid <T> and insert the correct place), but I suppose what we want is the original suggestion, but with only the insertion of <T> after id highlighted (and not the removal/id highlighted).

Thoughts/advice? Let me know if I need to phrase it differently 😅

@TaKO8Ki Just a bump on ^ - In the meantime, I've pushed a revert to the previous span (i.e. the ident and the generic)

For this, you'll need a multipart_suggestion, which takes a vector of (Span, String) pairs. In your case, one of the pair would be (self.token.span.shrink_to_hi(), snippet) that @TaKO8Ki suggested, and a second pair (generics.span, String::new()) to require deletion of the generics.

Ooh thanks!! I'll have a look 🎉🎉

compiler/rustc_parse/src/parser/diagnostics.rs

TaKO8Ki · 2022-11-22T01:54:54Z

@rustbot author

SpanishPear · 2022-11-22T13:35:16Z

Thanks for the review :)

~~Unfortunately I won't be able to get around to fixing for about 7 weeks as I'm currently traveling.~~

~~If people would like to see it in sooner, please feel free to build off my work :)~~

EDIT: I realise I can probably edit through github directly and see how that goes 😛

--wip-- [skip ci] get the generic text and put it int he suggestion, but suggestion not working on derive subdiagnostic refactor away from derives and use span_suggestion() instead. Show's the correct(?) generic contents, but overwrites the fn name :( x fmt drop commented code and s/todo/fixme get the correct diagnostic for functions, at least x fmt remove some debugs remove format remove debugs remove useless change remove useless change remove legacy approach correct lookahead + error message contains the ident name fmt refactor code tests add tests remoev debug remove comment

Co-authored-by: Takayuki Maeda <takoyaki0316@gmail.com>

SpanishPear · 2023-01-25T07:44:01Z

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

SpanishPear · 2023-01-31T10:38:45Z

@rustbot author

SpanishPear · 2023-02-01T08:15:12Z

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

TaKO8Ki · 2023-02-14T05:50:39Z

Sorry for the late review. @bors r+ rollup

bors · 2023-02-14T05:50:41Z

📌 Commit a3d32bb has been approved by TaKO8Ki

It is now in the queue for this repository.

…66_fix, r=TaKO8Ki Suggest fix for misplaced generic params on fn item rust-lang#103366 fixes rust-lang#103366 This still has some work to go, but works for 2/3 of the initial base cases described in #1033366 simple fn: ``` error: expected identifier, found `<` --> shreys/test_1.rs:1:3 | 1 | fn<T> id(x: T) -> T { x } | ^ expected identifier | help: help: place the generic parameter list after the function name: | 1 | fn id<T>(x: T) -> T { x } | ~~~~ ``` Complicated bounds ``` error: expected identifier, found `<` --> spanishpear/test_2.rs:1:3 | 1 | fn<'a, B: 'a + std::ops::Add<Output = u32>> f(_x: B) { } | ^ expected identifier | help: help: place the generic parameter list after the function name: | 1 | fn f<'a, B: 'a + std::ops::Add<Output = u32>>(_x: B) { } | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ``` Opening a draft PR for comments on approach, particularly I have the following questions: - [x] Is it okay to be using `err.span_suggestion` over struct derives? I struggled to get the initial implementation (particularly the correct suggestion message) on struct derives, although I think given what I've learned since starting, I could attempt re-doing it with that approach. - [x] in the case where the snippet cannot be obtained from a span, is the `help` but no suggestion okay? I think yes (also, when does this case occur?) - [x] are there any red flags for the generalisation of this work for relevant item kinds (i.e. `struct`, `enum`, `trait`, and `union`). My basic testing indicates it does work for those types except the help tip is currently hardcoded to `after the function name` - which should change dependent on the item. - [x] I am planning to not show the suggestion if there is already a `<` after the item identifier, (i.e. if there are already generics, as after a function name per the original issue). Any major objections? - [x] Is the style of error okay? I wasn't sure if there was a way to make it display nicer, or if thats handled by span_suggestion These aren't blocking questions, and I will keep working on: - check if there is a `<` after the ident (and if so, not showing the suggestion) - generalize the help message - figuring out how to write/run/etc ui tests (including reading the docs for them) - logic cleanups

…iaskrgr Rollup of 10 pull requests Successful merges: - rust-lang#103478 ( Suggest fix for misplaced generic params on fn item rust-lang#103366 ) - rust-lang#107739 (Check for overflow in evaluate_canonical_goal) - rust-lang#108003 (Avoid ICE when the generic_span is empty) - rust-lang#108016 ("Basic usage" is redundant for there is just one example) - rust-lang#108023 (Shrink size of array benchmarks) - rust-lang#108024 (add message to update Cargo.toml when x is changed) - rust-lang#108025 (rustdoc: add more tooltips to intra-doc links) - rust-lang#108029 (s/eval_usize/eval_target_usize/ for clarity) - rust-lang#108035 (Avoid using a dead email address as the main email address) - rust-lang#108038 (Remove needless supertrait constraints from Interner projections) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned TaKO8Ki Oct 24, 2022

rustbot added A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 24, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 24, 2022

SpanishPear commented Oct 24, 2022

View reviewed changes

compiler/rustc_error_messages/locales/en-US/parser.ftl Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

SpanishPear force-pushed the spanishpear/issue_103366_fix branch from 54a3773 to 97c17e2 Compare October 25, 2022 08:57

This comment has been minimized.

Sign in to view

SpanishPear marked this pull request as ready for review October 29, 2022 10:08

TaKO8Ki reviewed Nov 18, 2022

View reviewed changes

compiler/rustc_parse/src/parser/diagnostics.rs Outdated Show resolved Hide resolved

TaKO8Ki reviewed Nov 18, 2022

View reviewed changes

compiler/rustc_parse/src/parser/diagnostics.rs Outdated Show resolved Hide resolved

TaKO8Ki reviewed Nov 18, 2022

View reviewed changes

compiler/rustc_parse/src/parser/diagnostics.rs Outdated Show resolved Hide resolved

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 Nov 22, 2022

This comment has been minimized.

Sign in to view

SpanishPear and others added 5 commits January 22, 2023 17:05

Apply automatic suggestions from code review

5287004

Co-authored-by: Takayuki Maeda <takoyaki0316@gmail.com>

Attempt to address review comments via github web...

655beb4

revert to previous span

4447949

move tests to new rust-lang location

8292d07

SpanishPear force-pushed the spanishpear/issue_103366_fix branch from 4868ad6 to 8292d07 Compare January 22, 2023 06:17

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 Jan 25, 2023

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 Jan 31, 2023

move to multipart spans

70bfcc2

This comment has been minimized.

Sign in to view

fix formatting + test syntax

a3d32bb

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 1, 2023

SpanishPear requested a review from TaKO8Ki February 11, 2023 10:23

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, 2023

Dylan-DPC mentioned this pull request Feb 14, 2023

Rollup of 8 pull requests #108027

Closed

matthiaskrgr mentioned this pull request Feb 14, 2023

Rollup of 8 pull requests #108028

Closed

matthiaskrgr mentioned this pull request Feb 14, 2023

Rollup of 10 pull requests #108052

Merged

bors merged commit 202c706 into rust-lang:master Feb 14, 2023

rustbot added this to the 1.69.0 milestone Feb 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suggest fix for misplaced generic params on fn item #103366 #103478

Suggest fix for misplaced generic params on fn item #103366 #103478

SpanishPear commented Oct 24, 2022 •

edited

Loading

rust-highfive commented Oct 24, 2022

This comment has been minimized.

This comment has been minimized.

SpanishPear commented Oct 29, 2022 •

edited

Loading

SpanishPear commented Nov 12, 2022 •

edited

Loading

TaKO8Ki commented Nov 15, 2022

SpanishPear commented Nov 15, 2022 via email

TaKO8Ki Nov 18, 2022

SpanishPear Dec 3, 2022 •

edited

Loading

SpanishPear Jan 22, 2023

cjgillot Jan 29, 2023

SpanishPear Jan 30, 2023

TaKO8Ki commented Nov 22, 2022

SpanishPear commented Nov 22, 2022 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

SpanishPear commented Jan 25, 2023

SpanishPear commented Jan 31, 2023

This comment has been minimized.

SpanishPear commented Feb 1, 2023

TaKO8Ki commented Feb 14, 2023

bors commented Feb 14, 2023

Suggest fix for misplaced generic params on fn item #103366 #103478

Suggest fix for misplaced generic params on fn item #103366 #103478

Conversation

SpanishPear commented Oct 24, 2022 • edited Loading

rust-highfive commented Oct 24, 2022

This comment has been minimized.

This comment has been minimized.

SpanishPear commented Oct 29, 2022 • edited Loading

SpanishPear commented Nov 12, 2022 • edited Loading

TaKO8Ki commented Nov 15, 2022

SpanishPear commented Nov 15, 2022 via email

TaKO8Ki Nov 18, 2022

Choose a reason for hiding this comment

SpanishPear Dec 3, 2022 • edited Loading

Choose a reason for hiding this comment

SpanishPear Jan 22, 2023

Choose a reason for hiding this comment

cjgillot Jan 29, 2023

Choose a reason for hiding this comment

SpanishPear Jan 30, 2023

Choose a reason for hiding this comment

TaKO8Ki commented Nov 22, 2022

SpanishPear commented Nov 22, 2022 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

SpanishPear commented Jan 25, 2023

SpanishPear commented Jan 31, 2023

This comment has been minimized.

SpanishPear commented Feb 1, 2023

TaKO8Ki commented Feb 14, 2023

bors commented Feb 14, 2023

SpanishPear commented Oct 24, 2022 •

edited

Loading

SpanishPear commented Oct 29, 2022 •

edited

Loading

SpanishPear commented Nov 12, 2022 •

edited

Loading

SpanishPear Dec 3, 2022 •

edited

Loading

SpanishPear commented Nov 22, 2022 •

edited

Loading