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

Parser: Suggest Placing the Return Type After Function Parameters #127350

Merged
merged 2 commits into from
Jul 19, 2024

Conversation

veera-sivarajan
Copy link
Contributor

Fixes #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 and adds doc comments for parser::AllowPlus.

@rustbot
Copy link
Collaborator

rustbot commented Jul 5, 2024

r? @lcnr

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

@rustbot 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 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
@lcnr
Copy link
Contributor

lcnr commented Jul 10, 2024

r? compiler

@rustbot rustbot assigned oli-obk and unassigned lcnr Jul 10, 2024
compiler/rustc_parse/src/parser/item.rs Outdated Show resolved Hide resolved
generics.where_clause = self.parse_where_clause()?; // `where T: Ord`

// `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 };
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@veera-sivarajan veera-sivarajan Jul 13, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

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.

Err(err)
}
} else {
Ok(())
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@lcnr lcnr Jul 18, 2024

Choose a reason for hiding this comment

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

👍 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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

lcnr commented Jul 18, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 18, 2024

📌 Commit 4cad705 has been approved by lcnr

It is now in the queue for this repository.

@bors 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 added a commit to matthiaskrgr/rust that referenced this pull request Jul 19, 2024
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`.
tgross35 added a commit to tgross35/rust that referenced this pull request Jul 19, 2024
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`.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 19, 2024
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
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 19, 2024
…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
@bors bors merged commit c86e13f into rust-lang:master Jul 19, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jul 19, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 19, 2024
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`.
@veera-sivarajan veera-sivarajan deleted the bugfix-126311 branch July 19, 2024 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bad diagnostic for misplaced where clause
5 participants