Improve diagnostics for wrongly ordered keywords in function declaration #87235

poliorcetics · 2021-07-17T23:55:34Z

@rustbot label A-diagnostics T-compiler

rust-highfive · 2021-07-17T23:55:37Z

(rust-highfive has picked a reviewer for you, use r? to override)

nagisa

Reviewed the code changes as they are right now. The test failures seem to indicate there are a fair number of effects on other diagnostics so those need to be reviewed still (which I can do after the ui test outputs are blessed)

src/test/ui/parser/issue-87217-keyword-order/wrong-unsafe copy.rs

compiler/rustc_parse/src/parser/item.rs

poliorcetics · 2021-07-18T15:42:29Z

I reverted the changes to check_fn_front_matter for now to make it easier to check what tests where affected by the actual parsing.

I would still like to change check_fn_front_matter and I have left two tests that are currently failing about this. I can remove them to allow merging this PR and do it in a separate one if needed.

poliorcetics · 2021-07-24T16:05:06Z

I tried to write a more precise check_fn_front_matter function but so many diagnostics are dependent on the current one I stopped trying. This means extern "ABI" keyword fn will still propose { after "ABI" instead of hinting at changing keyword position but apart from that, everything should work.

poliorcetics · 2021-07-24T16:05:44Z

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

poliorcetics · 2021-07-24T16:31:59Z

I don't understand how my changes could have broken those tests. I can't run them locally too, they're always ignored and I see no way to force run them.

bors · 2021-07-24T20:27:28Z

☔ The latest upstream changes (presumably #87434) made this pull request unmergeable. Please resolve the merge conflicts.

compiler/rustc_parse/src/parser/item.rs

nagisa · 2021-08-01T08:41:46Z

compiler/rustc_parse/src/parser/item.rs

+                    // 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) {


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?

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

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

nagisa · 2021-08-01T15:21:47Z

Please squash the commits a little so that its 1 or 2 commits.

After this commit, `unsafe async fn ...` now suggests the `async unsafe` fix instead of misunderstanding the issue. This is not perfect for repeated keywords (`const async const`) and for keywords that are misplaced after `extern "some abi"` because of the way `check_fn_font_matter` works, but changing it breaks so many tests and diagnostics it has been judged too high a cost for this PR.

nagisa · 2021-08-08T16:52:39Z

@bors r+

bors · 2021-08-08T16:52:41Z

📌 Commit 690cbb7 has been approved by nagisa

bors · 2021-08-08T22:28:04Z

⌛ Testing commit 690cbb7 with merge 74a11c6...

bors · 2021-08-09T01:08:45Z

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing 74a11c6 to master...

…compiler-errors Improve `extern "<abi>" unsafe fn()` error message These errors were already reported in rust-lang#87217, and fixed by rust-lang#87235 but missed the case of an explicit ABI. This PR does not cover multiple keywords like `extern "C" pub const unsafe fn()`, but I don't know what a good way to cover this would be. It also seems rarer than `extern "C" unsafe` which I saw happen a few times in workshops.

Rollup merge of rust-lang#128229 - tdittr:unsafe-extern-abi-error, r=compiler-errors Improve `extern "<abi>" unsafe fn()` error message These errors were already reported in rust-lang#87217, and fixed by rust-lang#87235 but missed the case of an explicit ABI. This PR does not cover multiple keywords like `extern "C" pub const unsafe fn()`, but I don't know what a good way to cover this would be. It also seems rarer than `extern "C" unsafe` which I saw happen a few times in workshops.

rust-highfive assigned nagisa Jul 17, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 17, 2021

This comment has been minimized.

Sign in to view

nagisa reviewed Jul 18, 2021

View reviewed changes

poliorcetics force-pushed the issue-87217-fn-quali-order branch 2 times, most recently from c7793a6 to 99097d4 Compare July 18, 2021 15:39

This comment has been minimized.

Sign in to view

nagisa 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 Jul 23, 2021

poliorcetics force-pushed the issue-87217-fn-quali-order branch from 99097d4 to 1be06cc Compare July 24, 2021 16:02

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 Jul 24, 2021

This comment has been minimized.

Sign in to view

poliorcetics force-pushed the issue-87217-fn-quali-order branch from 1be06cc to 0212a09 Compare July 26, 2021 18:27

This comment has been minimized.

Sign in to view

poliorcetics force-pushed the issue-87217-fn-quali-order branch from 0212a09 to ad3d233 Compare July 26, 2021 18:48

nagisa reviewed Aug 1, 2021

View reviewed changes

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

nagisa reviewed Aug 1, 2021

View reviewed changes

poliorcetics force-pushed the issue-87217-fn-quali-order branch from 910096a to eabf19b Compare August 1, 2021 13:05

poliorcetics force-pushed the issue-87217-fn-quali-order branch from eabf19b to 690cbb7 Compare August 2, 2021 09:17

rustbot 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 Aug 2, 2021

poliorcetics mentioned this pull request Aug 2, 2021

Error suggests additional pub when using pub pub fn ... #87694

Closed

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 Aug 8, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 9, 2021

bors merged commit 74a11c6 into rust-lang:master Aug 9, 2021

rustbot added this to the 1.56.0 milestone Aug 9, 2021

poliorcetics deleted the issue-87217-fn-quali-order branch August 9, 2021 16:12

tdittr mentioned this pull request Jul 26, 2024

Improve extern "<abi>" unsafe fn() error message #128229

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve diagnostics for wrongly ordered keywords in function declaration #87235

Improve diagnostics for wrongly ordered keywords in function declaration #87235

poliorcetics commented Jul 17, 2021 •

edited

Loading

rust-highfive commented Jul 17, 2021

This comment has been minimized.

nagisa left a comment

poliorcetics commented Jul 18, 2021

This comment has been minimized.

poliorcetics commented Jul 24, 2021

poliorcetics commented Jul 24, 2021 •

edited

Loading

This comment has been minimized.

poliorcetics commented Jul 24, 2021

bors commented Jul 24, 2021

This comment has been minimized.

nagisa Aug 1, 2021

poliorcetics Aug 1, 2021

nagisa Aug 1, 2021

nagisa commented Aug 1, 2021

nagisa commented Aug 8, 2021

bors commented Aug 8, 2021

bors commented Aug 8, 2021

bors commented Aug 9, 2021

Improve diagnostics for wrongly ordered keywords in function declaration #87235

Improve diagnostics for wrongly ordered keywords in function declaration #87235

Conversation

poliorcetics commented Jul 17, 2021 • edited Loading

rust-highfive commented Jul 17, 2021

This comment has been minimized.

nagisa left a comment

Choose a reason for hiding this comment

poliorcetics commented Jul 18, 2021

This comment has been minimized.

poliorcetics commented Jul 24, 2021

poliorcetics commented Jul 24, 2021 • edited Loading

This comment has been minimized.

poliorcetics commented Jul 24, 2021

bors commented Jul 24, 2021

This comment has been minimized.

nagisa Aug 1, 2021

Choose a reason for hiding this comment

poliorcetics Aug 1, 2021

Choose a reason for hiding this comment

nagisa Aug 1, 2021

Choose a reason for hiding this comment

nagisa commented Aug 1, 2021

nagisa commented Aug 8, 2021

bors commented Aug 8, 2021

bors commented Aug 8, 2021

bors commented Aug 9, 2021

poliorcetics commented Jul 17, 2021 •

edited

Loading

poliorcetics commented Jul 24, 2021 •

edited

Loading