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.

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

Suggest fix for misplaced generic params on fn item #103366

Closed
jruderman opened this issue Oct 21, 2022 · 4 comments · Fixed by #103478
Closed

Suggest fix for misplaced generic params on fn item #103366

jruderman opened this issue Oct 21, 2022 · 4 comments · Fixed by #103478
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jruderman
Copy link
Contributor

jruderman commented Oct 21, 2022

Given the following code (playground):

fn<T> id(x: T) -> T { x }

The current output is:

error: expected identifier, found `<`
 --> src/lib.rs:1:3
  |
1 | fn<T> id(x: T) -> T { x }
  |   ^ expected identifier

Ideally the output should look also include:

help: place the generic parameter list after the function name:
  |
1 | fn id<T>(x: T) -> T { x }
  |      ~~~

This is an easy mistake to make because impl items do require the generic parameters to go immediately after the keyword.

@jruderman jruderman added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 21, 2022
@compiler-errors
Copy link
Member

this probably should be generalized for the other item kinds as well (struct, enum, trait, and union), if someone is planning on implementing this.

@SpanishPear
Copy link
Contributor

SpanishPear commented Oct 23, 2022

Would this make a good first issue to look at? If so, would be happy to take a stab :)

@SpanishPear
Copy link
Contributor

@rustbot claim

Let's give it a try anyways 🥳

@jruderman
Copy link
Contributor Author

Here are some potentially tricky cases:

Complicated bounds

aka "the programmer probably should have moved some of this into a where clause"

fn<'a, B: 'a + std::ops::Add<Output = u32>> f(_x: B) { }

fn main() {
    f(1_u32);
}

Generic bounds also present in the correct place

fn<T: Send> g<T>(x: T) -> T { x }

Here, the compiler should avoid suggesting modified code that also won't parse. Simply moving <T: Send> to after the function name in this case would result in fn g<T: Send><T>, which isn't right.

Ideally, for this specific example, it would suggest replacing the second with the first, creating fn g<T: Send>. But it's probably not worth the complexity to try to determine "how compatible" two generics lists are and how best to merge them.

For this case, I'd be happy enough with an error message that doesn't include a suggestion, or simply suggesting the removal of the first generics list.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Feb 14, 2023
…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
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 14, 2023
…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
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 14, 2023
…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
@bors bors closed this as completed in 202c706 Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants