Parser: Suggest Placing the Return Type After Function Parameters by veera-sivarajan · Pull Request #127350 · rust-lang/rust

veera-sivarajan · 2024-07-05T01:10:31Z

This PR suggests placing the return type after the function parameters when it's misplaced after a where clause.

This also tangentially improves diagnostics for cases like this and adds doc comments for parser::AllowPlus.

rustbot · 2024-07-05T01:10:39Z

rustbot has assigned @lcnr.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

lcnr · 2024-07-10T14:19:19Z

r? compiler

compiler/rustc_parse/src/parser/item.rs

lcnr · 2024-07-10T14:20:47Z

compiler/rustc_parse/src/parser/item.rs


+        // `fn_params_end` is needed only when it's followed by a where clause.
+        let fn_params_end =
+            if generics.where_clause.has_where_token { Some(fn_params_end) } else { None };


what's the harm of always passing in that span?

Checking if a where clause exists makes sure we don't incorrectly suggest to place the second return type after function parameters for code like fn fun<T>() -> u8 -> u8 {}.

I've added that as a test in https://github.com/rust-lang/rust/blob/d14e2818e8d5e9d65f464517d1a88272b626629e/tests/ui/parser/issues/misplaced-return-type-without-where-issue-126311.stderr.

lcnr

I guess this makes sense 🤔 can you split the error handling out of fn parse_fn_body and always return ErrorGuaranteed from that function?

veera-sivarajan · 2024-07-16T19:41:37Z

Sure, thank you for the suggestion. I moved the error handling to a separate function.

But, it's not possible to return an ErrorGuaranteed because a couple of cases are irrecoverable errors that return a Diag.

lcnr · 2024-07-16T22:26:21Z

compiler/rustc_parse/src/parser/item.rs

+                Err(err)
+            }
+        } else {
+            Ok(())


that Ok(()) is very questionable 🤔 I would expect that this is unreachable?

Replacing the Ok(()) with unreachable!() fails on this UI test:

extern "C" { fn foo() //~ERROR expected `;` }

It's because expected_one_of_not_found() returns an Ok() when it's able to recover from an error.

👍 two more requests then. expected_one_of_not_found currently always returns Recovered::Yes(ErrorGuaranteed) in that case afaict. Please change its return type to PResult<'a, ErrorGuaranteed> and do the same for error_fn_body_not_found to just propagate the error upwards

Sure, changed the return type of those two functions to PResult<'a, ErrorGuaranteed>. Also, made some small changes to expect_one_of to map ErrorGuaranteed to Recovered::Yes(ErrorGuaranteed).

Thank you for the suggestions.

lcnr · 2024-07-18T22:03:06Z

@bors r+ rollup

bors · 2024-07-18T22:03:10Z

📌 Commit 4cad705 has been approved by lcnr

It is now in the queue for this repository.

Parser: Suggest Placing the Return Type After Function Parameters Fixes rust-lang#126311 This PR suggests placing the return type after the function parameters when it's misplaced after a `where` clause. This also tangentially improves diagnostics for cases like [this](https://github.com/veera-sivarajan/rust/blob/86d6f1312a77997ef994240e716288d61a343a6d/tests/ui/parser/issues/misplaced-return-type-without-where-issue-126311.rs#L1C1-L1C28) and adds doc comments for `parser::AllowPlus`.

Rollup of 7 pull requests Successful merges: - rust-lang#123196 (Add Process support for UEFI) - rust-lang#127350 (Parser: Suggest Placing the Return Type After Function Parameters) - rust-lang#127523 (Migrate `dump-ice-to-disk` and `panic-abort-eh_frame` `run-make` tests to rmake) - rust-lang#127662 (When finding item gated behind a `cfg` flag, point at it) - rust-lang#127903 (`force_collect` improvements) - rust-lang#127932 (rustdoc: fix `current` class on sidebar modnav) - rust-lang#127943 (Don't allow unsafe statics outside of extern blocks) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#127350 (Parser: Suggest Placing the Return Type After Function Parameters) - rust-lang#127621 (Rewrite and rename `issue-22131` and `issue-26006` `run-make` tests to rmake) - rust-lang#127662 (When finding item gated behind a `cfg` flag, point at it) - rust-lang#127903 (`force_collect` improvements) - rust-lang#127932 (rustdoc: fix `current` class on sidebar modnav) - rust-lang#127943 (Don't allow unsafe statics outside of extern blocks) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#127350 - veera-sivarajan:bugfix-126311, r=lcnr Parser: Suggest Placing the Return Type After Function Parameters Fixes rust-lang#126311 This PR suggests placing the return type after the function parameters when it's misplaced after a `where` clause. This also tangentially improves diagnostics for cases like [this](https://github.com/veera-sivarajan/rust/blob/86d6f1312a77997ef994240e716288d61a343a6d/tests/ui/parser/issues/misplaced-return-type-without-where-issue-126311.rs#L1C1-L1C28) and adds doc comments for `parser::AllowPlus`.

Update Tests

5834772

rustbot assigned lcnr Jul 5, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 5, 2024

veera-sivarajan changed the title ~~Parser: Suggest Placing the Return After Function Parameters~~ Parser: Suggest Placing the Return Type After Function Parameters Jul 5, 2024

rustbot assigned oli-obk and unassigned lcnr Jul 10, 2024

lcnr reviewed Jul 10, 2024

View reviewed changes

veera-sivarajan force-pushed the bugfix-126311 branch from 86d6f13 to d14e281 Compare July 13, 2024 22:21

lcnr reviewed Jul 16, 2024

View reviewed changes

veera-sivarajan force-pushed the bugfix-126311 branch from d14e281 to 7ca42a2 Compare July 16, 2024 19:37

lcnr reviewed Jul 16, 2024

View reviewed changes

Parser: Suggest Placing the Return Type After Function Parameters

4cad705

veera-sivarajan force-pushed the bugfix-126311 branch from 7ca42a2 to 4cad705 Compare July 18, 2024 21:57

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 Jul 18, 2024

matthiaskrgr mentioned this pull request Jul 19, 2024

Rollup of 6 pull requests #127952

Closed

tgross35 mentioned this pull request Jul 19, 2024

Rollup of 7 pull requests #127954

Closed

matthiaskrgr mentioned this pull request Jul 19, 2024

Rollup of 6 pull requests #127957

Merged

bors merged commit c86e13f into rust-lang:master Jul 19, 2024

rustbot added this to the 1.81.0 milestone Jul 19, 2024

veera-sivarajan deleted the bugfix-126311 branch July 19, 2024 16:43

Uh oh!

Conversation

veera-sivarajan commented Jul 5, 2024

Uh oh!

rustbot commented Jul 5, 2024

Uh oh!

lcnr commented Jul 10, 2024

Uh oh!

Uh oh!

lcnr Jul 10, 2024

Choose a reason for hiding this comment

Uh oh!

veera-sivarajan Jul 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lcnr left a comment

Choose a reason for hiding this comment

Uh oh!

veera-sivarajan commented Jul 16, 2024

Uh oh!

lcnr Jul 16, 2024

Choose a reason for hiding this comment

Uh oh!

veera-sivarajan Jul 17, 2024

Choose a reason for hiding this comment

Uh oh!

lcnr Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

veera-sivarajan Jul 18, 2024

Choose a reason for hiding this comment

Uh oh!

lcnr commented Jul 18, 2024

Uh oh!

bors commented Jul 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

veera-sivarajan Jul 13, 2024 •

edited

Loading

lcnr Jul 18, 2024 •

edited

Loading