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 #103478

Merged
merged 7 commits into from
Feb 14, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 55 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,60 @@ 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) &&
let Ok(ident) = self.sess.source_map().span_to_snippet(self.token.span) {
err.span_suggestion_verbose(
generic.span.to(self.token.span),
format!("place the generic parameter name after the {ident_name} name"),
format!(" {ident}{snippet}"),
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@spanishpear spanishpear Dec 3, 2022

Choose a reason for hiding this comment

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

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:
image

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 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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
10 changes: 10 additions & 0 deletions tests/ui/parser/suggest_misplaced_generics/enum.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// 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
//~| SUGGESTION Foo<T>

fn main() {}
10 changes: 10 additions & 0 deletions tests/ui/parser/suggest_misplaced_generics/enum.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// 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
//~| SUGGESTION Foo<T>

fn main() {}
13 changes: 13 additions & 0 deletions tests/ui/parser/suggest_misplaced_generics/enum.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
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 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,10 @@
// 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
//~| SUGGESTION f<'a, B: 'a + std::ops::Add<Output = u32>>

fn main() {}
10 changes: 10 additions & 0 deletions tests/ui/parser/suggest_misplaced_generics/fn-complex-generics.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// 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
//~| SUGGESTION f<'a, B: 'a + std::ops::Add<Output = u32>>

fn main() {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
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 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

10 changes: 10 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,10 @@
// 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
//~| SUGGESTION id<T>

fn main() {}
10 changes: 10 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,10 @@
// 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
//~| SUGGESTION id<T>

fn main() {}
13 changes: 13 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,13 @@
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

10 changes: 10 additions & 0 deletions tests/ui/parser/suggest_misplaced_generics/struct.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// 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
//~| SUGGESTION Foo<T>

fn main() {}
10 changes: 10 additions & 0 deletions tests/ui/parser/suggest_misplaced_generics/struct.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// 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
//~| SUGGESTION Foo<T>

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

error: aborting due to previous error

12 changes: 12 additions & 0 deletions tests/ui/parser/suggest_misplaced_generics/trait.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// 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
//~| SUGGESTION Foo<T>
}


fn main() {}
12 changes: 12 additions & 0 deletions tests/ui/parser/suggest_misplaced_generics/trait.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// 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
//~| SUGGESTION Foo<T>
}


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

error: aborting due to previous error

10 changes: 10 additions & 0 deletions tests/ui/parser/suggest_misplaced_generics/type.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// 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
//~| SUGGESTION Foo<T>

fn main() {}
10 changes: 10 additions & 0 deletions tests/ui/parser/suggest_misplaced_generics/type.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// 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
//~| SUGGESTION Foo<T>

fn main() {}
13 changes: 13 additions & 0 deletions tests/ui/parser/suggest_misplaced_generics/type.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
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 Foo<T> = T;
| ~~~~~~

error: aborting due to previous error