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

Improve diagnostics for wrongly ordered keywords in function declaration #87235

Merged
merged 1 commit into from
Aug 9, 2021
Merged
Show file tree
Hide file tree
Changes from all 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
35 changes: 34 additions & 1 deletion compiler/rustc_parse/src/parser/item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1771,8 +1771,14 @@ impl<'a> Parser<'a> {
pub(super) fn parse_fn_front_matter(&mut self) -> PResult<'a, FnHeader> {
let sp_start = self.token.span;
let constness = self.parse_constness();

let async_start_sp = self.token.span;
let asyncness = self.parse_asyncness();

let unsafe_start_sp = self.token.span;
let unsafety = self.parse_unsafety();

let ext_start_sp = self.token.span;
let ext = self.parse_extern();

if let Async::Yes { span, .. } = asyncness {
Expand All @@ -1787,8 +1793,35 @@ impl<'a> Parser<'a> {
Ok(true) => {}
Ok(false) => unreachable!(),
Err(mut err) => {
// Qualifier keywords ordering check

// This will allow the machine fix to directly place the keyword in the correct place
let current_qual_sp = if self.check_keyword(kw::Const) {
Copy link
Member

Choose a reason for hiding this comment

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

If I'm reading this correctly, we will trigger the diagnostic here below for code that has duplicate keywords, such as e.g. in

const async const fn

and I believe that the machine applicable suggestion will suggest a const const async fn which is invalid.

It may not be critical to fix this now in this PR, but could you please at least add a test for that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, I will add a test for it.

pub pub fn test() {} already behaves like that and has a MachineApplicable fix, I think it is because it is expected to sometimes misplace keywords but repeating them is not something that happens often so it should be okay ? I can try to fix it in this PR or open another issue+PR combo, I don't know what's best here

Copy link
Member

Choose a reason for hiding this comment

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

I don't believe this must be fixed in this PR, but I would consider that a bug.

Some(async_start_sp)
} else if self.check_keyword(kw::Async) {
Some(unsafe_start_sp)
} else if self.check_keyword(kw::Unsafe) {
Some(ext_start_sp)
} else {
None
};

if let Some(current_qual_sp) = current_qual_sp {
let current_qual_sp = current_qual_sp.to(self.prev_token.span);
if let Ok(current_qual) = self.span_to_snippet(current_qual_sp) {
let invalid_qual_sp = self.token.uninterpolated_span();
let invalid_qual = self.span_to_snippet(invalid_qual_sp).unwrap();

err.span_suggestion(
current_qual_sp.to(invalid_qual_sp),
&format!("`{}` must come before `{}`", invalid_qual, current_qual),
format!("{} {}", invalid_qual, current_qual),
Applicability::MachineApplicable,
).note("keyword order for functions declaration is `default`, `pub`, `const`, `async`, `unsafe`, `extern`");
}
}
// Recover incorrect visibility order such as `async pub`.
if self.check_keyword(kw::Pub) {
else if self.check_keyword(kw::Pub) {
let sp = sp_start.to(self.prev_token.span);
if let Ok(snippet) = self.span_to_snippet(sp) {
let vis = match self.parse_visibility(FollowedByType::No) {
Expand Down
7 changes: 6 additions & 1 deletion src/test/ui/async-await/no-async-const.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,12 @@ error: expected one of `extern`, `fn`, or `unsafe`, found keyword `const`
--> $DIR/no-async-const.rs:4:11
|
LL | pub async const fn x() {}
| ^^^^^ expected one of `extern`, `fn`, or `unsafe`
| ------^^^^^
| | |
| | expected one of `extern`, `fn`, or `unsafe`
| help: `const` must come before `async`: `const async`
|
= note: keyword order for functions declaration is `default`, `pub`, `const`, `async`, `unsafe`, `extern`

error: aborting due to previous error

14 changes: 12 additions & 2 deletions src/test/ui/async-await/no-unsafe-async.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,25 @@ LL | impl S {
| - while parsing this item list starting here
LL | #[cfg(FALSE)]
LL | unsafe async fn g() {}
| ^^^^^ expected one of `extern` or `fn`
| -------^^^^^
| | |
| | expected one of `extern` or `fn`
| help: `async` must come before `unsafe`: `async unsafe`
LL | }
| - the item list ends here
|
= note: keyword order for functions declaration is `default`, `pub`, `const`, `async`, `unsafe`, `extern`

error: expected one of `extern` or `fn`, found keyword `async`
--> $DIR/no-unsafe-async.rs:11:8
|
LL | unsafe async fn f() {}
| ^^^^^ expected one of `extern` or `fn`
| -------^^^^^
| | |
| | expected one of `extern` or `fn`
| help: `async` must come before `unsafe`: `async unsafe`
|
= note: keyword order for functions declaration is `default`, `pub`, `const`, `async`, `unsafe`, `extern`

error: aborting due to 2 previous errors

11 changes: 11 additions & 0 deletions src/test/ui/parser/issue-87217-keyword-order/const-async-const.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// edition:2018

// Test that even when `const` is already present, the proposed fix is `const const async`,
// like for `pub pub`.

const async const fn test() {}
//~^ ERROR expected one of `extern`, `fn`, or `unsafe`, found keyword `const`
//~| NOTE expected one of `extern`, `fn`, or `unsafe`
//~| HELP `const` must come before `async`
//~| SUGGESTION const async
//~| NOTE keyword order for functions declaration is `default`, `pub`, `const`, `async`, `unsafe`, `extern`
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
error: expected one of `extern`, `fn`, or `unsafe`, found keyword `const`
--> $DIR/const-async-const.rs:6:13
|
LL | const async const fn test() {}
| ------^^^^^
| | |
| | expected one of `extern`, `fn`, or `unsafe`
| help: `const` must come before `async`: `const async`
|
= note: keyword order for functions declaration is `default`, `pub`, `const`, `async`, `unsafe`, `extern`

error: aborting due to previous error

14 changes: 14 additions & 0 deletions src/test/ui/parser/issue-87217-keyword-order/several-kw-jump.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// edition:2018

// There is an order to respect for keywords before a function:
// `<visibility>, const, async, unsafe, extern, "<ABI>"`
//
// This test ensures the compiler is helpful about them being misplaced.
// Visibilities are tested elsewhere.

async unsafe const fn test() {}
//~^ ERROR expected one of `extern` or `fn`, found keyword `const`
//~| NOTE expected one of `extern` or `fn`
//~| HELP `const` must come before `async unsafe`
//~| SUGGESTION const async unsafe
//~| NOTE keyword order for functions declaration is `default`, `pub`, `const`, `async`, `unsafe`, `extern`
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
error: expected one of `extern` or `fn`, found keyword `const`
--> $DIR/several-kw-jump.rs:9:14
|
LL | async unsafe const fn test() {}
| -------------^^^^^
| | |
| | expected one of `extern` or `fn`
| help: `const` must come before `async unsafe`: `const async unsafe`
|
= note: keyword order for functions declaration is `default`, `pub`, `const`, `async`, `unsafe`, `extern`

error: aborting due to previous error

14 changes: 14 additions & 0 deletions src/test/ui/parser/issue-87217-keyword-order/wrong-async.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// edition:2018

// There is an order to respect for keywords before a function:
// `<visibility>, const, async, unsafe, extern, "<ABI>"`
//
// This test ensures the compiler is helpful about them being misplaced.
// Visibilities are tested elsewhere.

unsafe async fn test() {}
//~^ ERROR expected one of `extern` or `fn`, found keyword `async`
//~| NOTE expected one of `extern` or `fn`
//~| HELP `async` must come before `unsafe`
//~| SUGGESTION async unsafe
//~| NOTE keyword order for functions declaration is `default`, `pub`, `const`, `async`, `unsafe`, `extern`
13 changes: 13 additions & 0 deletions src/test/ui/parser/issue-87217-keyword-order/wrong-async.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
error: expected one of `extern` or `fn`, found keyword `async`
--> $DIR/wrong-async.rs:9:8
|
LL | unsafe async fn test() {}
| -------^^^^^
| | |
| | expected one of `extern` or `fn`
| help: `async` must come before `unsafe`: `async unsafe`
|
= note: keyword order for functions declaration is `default`, `pub`, `const`, `async`, `unsafe`, `extern`

error: aborting due to previous error

14 changes: 14 additions & 0 deletions src/test/ui/parser/issue-87217-keyword-order/wrong-const.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// edition:2018

// There is an order to respect for keywords before a function:
// `<visibility>, const, async, unsafe, extern, "<ABI>"`
//
// This test ensures the compiler is helpful about them being misplaced.
// Visibilities are tested elsewhere.

unsafe const fn test() {}
//~^ ERROR expected one of `extern` or `fn`, found keyword `const`
//~| NOTE expected one of `extern` or `fn`
//~| HELP `const` must come before `unsafe`
//~| SUGGESTION const unsafe
//~| NOTE keyword order for functions declaration is `default`, `pub`, `const`, `async`, `unsafe`, `extern`
13 changes: 13 additions & 0 deletions src/test/ui/parser/issue-87217-keyword-order/wrong-const.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
error: expected one of `extern` or `fn`, found keyword `const`
--> $DIR/wrong-const.rs:9:8
|
LL | unsafe const fn test() {}
| -------^^^^^
| | |
| | expected one of `extern` or `fn`
| help: `const` must come before `unsafe`: `const unsafe`
|
= note: keyword order for functions declaration is `default`, `pub`, `const`, `async`, `unsafe`, `extern`

error: aborting due to previous error

14 changes: 14 additions & 0 deletions src/test/ui/parser/issue-87217-keyword-order/wrong-unsafe.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// edition:2018

// There is an order to respect for keywords before a function:
// `<visibility>, const, async, unsafe, extern, "<ABI>"`
//
// This test ensures the compiler is helpful about them being misplaced.
// Visibilities are tested elsewhere.

extern unsafe fn test() {}
//~^ ERROR expected `fn`, found keyword `unsafe`
//~| NOTE expected `fn`
//~| HELP `unsafe` must come before `extern`
//~| SUGGESTION unsafe extern
//~| NOTE keyword order for functions declaration is `default`, `pub`, `const`, `async`, `unsafe`, `extern`
13 changes: 13 additions & 0 deletions src/test/ui/parser/issue-87217-keyword-order/wrong-unsafe.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
error: expected `fn`, found keyword `unsafe`
--> $DIR/wrong-unsafe.rs:9:8
|
LL | extern unsafe fn test() {}
| -------^^^^^^
| | |
| | expected `fn`
| help: `unsafe` must come before `extern`: `unsafe extern`
|
= note: keyword order for functions declaration is `default`, `pub`, `const`, `async`, `unsafe`, `extern`

error: aborting due to previous error