Skip to content

Commit

Permalink
Rollup merge of #103478 - SpanishPear:spanishpear/issue_103366_fix, r…
Browse files Browse the repository at this point in the history
…=TaKO8Ki

 Suggest fix for misplaced generic params on fn item #103366

fixes #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
  • Loading branch information
matthiaskrgr committed Feb 14, 2023
2 parents 9bb6e60 + a3d32bb commit 202c706
Show file tree
Hide file tree
Showing 23 changed files with 287 additions and 2 deletions.
58 changes: 56 additions & 2 deletions compiler/rustc_parse/src/parser/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ impl<'a> Parser<'a> {
self.sess.source_map().span_to_snippet(span)
}

pub(super) fn expected_ident_found(&self) -> DiagnosticBuilder<'a, ErrorGuaranteed> {
pub(super) fn expected_ident_found(&mut self) -> DiagnosticBuilder<'a, ErrorGuaranteed> {
let valid_follow = &[
TokenKind::Eq,
TokenKind::Colon,
Expand Down Expand Up @@ -324,7 +324,61 @@ impl<'a> Parser<'a> {
suggest_raw,
suggest_remove_comma,
};
err.into_diagnostic(&self.sess.span_diagnostic)
let mut err = err.into_diagnostic(&self.sess.span_diagnostic);

// if the token we have is a `<`
// it *might* be a misplaced generic
if self.token == token::Lt {
// all keywords that could have generic applied
let valid_prev_keywords =
[kw::Fn, kw::Type, kw::Struct, kw::Enum, kw::Union, kw::Trait];

// If we've expected an identifier,
// and the current token is a '<'
// if the previous token is a valid keyword
// that might use a generic, then suggest a correct
// generic placement (later on)
let maybe_keyword = self.prev_token.clone();
if valid_prev_keywords.into_iter().any(|x| maybe_keyword.is_keyword(x)) {
// if we have a valid keyword, attempt to parse generics
// also obtain the keywords symbol
match self.parse_generics() {
Ok(generic) => {
if let TokenKind::Ident(symbol, _) = maybe_keyword.kind {
let ident_name = symbol;
// at this point, we've found something like
// `fn <T>id`
// and current token should be Ident with the item name (i.e. the function name)
// if there is a `<` after the fn name, then don't show a suggestion, show help

if !self.look_ahead(1, |t| *t == token::Lt) &&
let Ok(snippet) = self.sess.source_map().span_to_snippet(generic.span) {
err.multipart_suggestion_verbose(
format!("place the generic parameter name after the {ident_name} name"),
vec![
(self.token.span.shrink_to_hi(), snippet),
(generic.span, String::new())
],
Applicability::MaybeIncorrect,
);
} else {
err.help(format!(
"place the generic parameter name after the {ident_name} name"
));
}
}
}
Err(err) => {
// if there's an error parsing the generics,
// then don't do a misplaced generics suggestion
// and emit the expected ident error instead;
err.cancel();
}
}
}
}

err
}

pub(super) fn expected_one_of_not_found(
Expand Down
9 changes: 9 additions & 0 deletions tests/ui/parser/suggest_misplaced_generics/enum.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Issue: 103366 , Suggest fix for misplaced generic params
// run-rustfix

#[allow(unused)]
enum Foo<T> { Variant(T) }
//~^ ERROR expected identifier, found `<`
//~| HELP place the generic parameter name after the enum name

fn main() {}
9 changes: 9 additions & 0 deletions tests/ui/parser/suggest_misplaced_generics/enum.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Issue: 103366 , Suggest fix for misplaced generic params
// run-rustfix

#[allow(unused)]
enum<T> Foo { Variant(T) }
//~^ ERROR expected identifier, found `<`
//~| HELP place the generic parameter name after the enum name

fn main() {}
14 changes: 14 additions & 0 deletions tests/ui/parser/suggest_misplaced_generics/enum.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error: expected identifier, found `<`
--> $DIR/enum.rs:5:5
|
LL | enum<T> Foo { Variant(T) }
| ^ expected identifier
|
help: place the generic parameter name after the enum name
|
LL - enum<T> Foo { Variant(T) }
LL + enum Foo<T> { Variant(T) }
|

error: aborting due to previous error

Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Issue: 103366
// there is already an existing generic on f, so don't show a suggestion

#[allow(unused)]
fn<'a, B: 'a + std::ops::Add<Output = u32>> f<T>(_x: B) { }
//~^ ERROR expected identifier, found `<`
//~| HELP place the generic parameter name after the fn name

fn main() {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
error: expected identifier, found `<`
--> $DIR/existing_generics.rs:5:3
|
LL | fn<'a, B: 'a + std::ops::Add<Output = u32>> f<T>(_x: B) { }
| ^ expected identifier
|
= help: place the generic parameter name after the fn name

error: aborting due to previous error

Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Issue: 103366 , Suggest fix for misplaced generic params
// run-rustfix

#[allow(unused)]
fn f<'a, B: 'a + std::ops::Add<Output = u32>>(_x: B) { }
//~^ ERROR expected identifier, found `<`
//~| HELP place the generic parameter name after the fn name

fn main() {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Issue: 103366 , Suggest fix for misplaced generic params
// run-rustfix

#[allow(unused)]
fn<'a, B: 'a + std::ops::Add<Output = u32>> f(_x: B) { }
//~^ ERROR expected identifier, found `<`
//~| HELP place the generic parameter name after the fn name

fn main() {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error: expected identifier, found `<`
--> $DIR/fn-complex-generics.rs:5:3
|
LL | fn<'a, B: 'a + std::ops::Add<Output = u32>> f(_x: B) { }
| ^ expected identifier
|
help: place the generic parameter name after the fn name
|
LL - fn<'a, B: 'a + std::ops::Add<Output = u32>> f(_x: B) { }
LL + fn f<'a, B: 'a + std::ops::Add<Output = u32>>(_x: B) { }
|

error: aborting due to previous error

Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// Issue: 103366 , Suggest fix for misplaced generic params
// The generics fail to parse here, so don't make any suggestions/help

#[allow(unused)]
fn<~>()> id(x: T) -> T { x }
//~^ ERROR expected identifier, found `<`

fn main() {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
error: expected identifier, found `<`
--> $DIR/fn-invalid-generics.rs:5:3
|
LL | fn<~>()> id(x: T) -> T { x }
| ^ expected identifier

error: aborting due to previous error

9 changes: 9 additions & 0 deletions tests/ui/parser/suggest_misplaced_generics/fn-simple.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Issue: 103366 , Suggest fix for misplaced generic params
// run-rustfix

#[allow(unused)]
fn id<T>(x: T) -> T { x }
//~^ ERROR expected identifier, found `<`
//~| HELP place the generic parameter name after the fn name

fn main() {}
9 changes: 9 additions & 0 deletions tests/ui/parser/suggest_misplaced_generics/fn-simple.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Issue: 103366 , Suggest fix for misplaced generic params
// run-rustfix

#[allow(unused)]
fn<T> id(x: T) -> T { x }
//~^ ERROR expected identifier, found `<`
//~| HELP place the generic parameter name after the fn name

fn main() {}
14 changes: 14 additions & 0 deletions tests/ui/parser/suggest_misplaced_generics/fn-simple.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
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(x: T) -> T { x }
LL + fn id<T>(x: T) -> T { x }
|

error: aborting due to previous error

9 changes: 9 additions & 0 deletions tests/ui/parser/suggest_misplaced_generics/struct.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Issue: 103366 , Suggest fix for misplaced generic params
// run-rustfix

#[allow(unused)]
struct Foo<T> { x: T }
//~^ ERROR expected identifier, found `<`
//~| HELP place the generic parameter name after the struct name

fn main() {}
9 changes: 9 additions & 0 deletions tests/ui/parser/suggest_misplaced_generics/struct.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Issue: 103366 , Suggest fix for misplaced generic params
// run-rustfix

#[allow(unused)]
struct<T> Foo { x: T }
//~^ ERROR expected identifier, found `<`
//~| HELP place the generic parameter name after the struct name

fn main() {}
14 changes: 14 additions & 0 deletions tests/ui/parser/suggest_misplaced_generics/struct.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error: expected identifier, found `<`
--> $DIR/struct.rs:5:7
|
LL | struct<T> Foo { x: T }
| ^ expected identifier
|
help: place the generic parameter name after the struct name
|
LL - struct<T> Foo { x: T }
LL + struct Foo<T> { x: T }
|

error: aborting due to previous error

11 changes: 11 additions & 0 deletions tests/ui/parser/suggest_misplaced_generics/trait.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Issue: 103366 , Suggest fix for misplaced generic params
// run-rustfix

#[allow(unused)]
trait Foo<T> {
//~^ ERROR expected identifier, found `<`
//~| HELP place the generic parameter name after the trait name
}


fn main() {}
11 changes: 11 additions & 0 deletions tests/ui/parser/suggest_misplaced_generics/trait.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Issue: 103366 , Suggest fix for misplaced generic params
// run-rustfix

#[allow(unused)]
trait<T> Foo {
//~^ ERROR expected identifier, found `<`
//~| HELP place the generic parameter name after the trait name
}


fn main() {}
14 changes: 14 additions & 0 deletions tests/ui/parser/suggest_misplaced_generics/trait.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error: expected identifier, found `<`
--> $DIR/trait.rs:5:6
|
LL | trait<T> Foo {
| ^ expected identifier
|
help: place the generic parameter name after the trait name
|
LL - trait<T> Foo {
LL + trait Foo<T> {
|

error: aborting due to previous error

9 changes: 9 additions & 0 deletions tests/ui/parser/suggest_misplaced_generics/type.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Issue: 103366 , Suggest fix for misplaced generic params
// run-rustfix

#[allow(unused)]
type Foo<T> = T;
//~^ ERROR expected identifier, found `<`
//~| HELP place the generic parameter name after the type name

fn main() {}
9 changes: 9 additions & 0 deletions tests/ui/parser/suggest_misplaced_generics/type.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Issue: 103366 , Suggest fix for misplaced generic params
// run-rustfix

#[allow(unused)]
type<T> Foo = T;
//~^ ERROR expected identifier, found `<`
//~| HELP place the generic parameter name after the type name

fn main() {}
14 changes: 14 additions & 0 deletions tests/ui/parser/suggest_misplaced_generics/type.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error: expected identifier, found `<`
--> $DIR/type.rs:5:5
|
LL | type<T> Foo = T;
| ^ expected identifier
|
help: place the generic parameter name after the type name
|
LL - type<T> Foo = T;
LL + type Foo<T> = T;
|

error: aborting due to previous error

0 comments on commit 202c706

Please sign in to comment.