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

Diagnostic when Fn*-family trait bounds is missing parens could be improved #108109

Closed
Fishrock123 opened this issue Feb 16, 2023 · 6 comments · Fixed by #117297
Closed

Diagnostic when Fn*-family trait bounds is missing parens could be improved #108109

Fishrock123 opened this issue Feb 16, 2023 · 6 comments · Fixed by #117297
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-parser Area: The parsing of Rust source code to an AST T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Fishrock123
Copy link
Contributor

Fishrock123 commented Feb 16, 2023

Code

fn handle<F>() where F: FnOnce -> () {}

Current output

error: expected one of `(`, `+`, `,`, `::`, `<`, or `{`, found `->`
 --> src/main.rs:1:32
  |
1 | fn handle<F>() where F: FnOnce -> () {}
  |                                ^^ expected one of `(`, `+`, `,`, `::`, `<`, or `{`

Desired output

error: expected `(`, found `->`
 --> src/main.rs:1:32
  |
1 | fn handle<F>() where F: FnOnce -> () {}
  |                               ^ `Fn*`-family trait bounds must be followed by parentheses
help: try using parenthesis here:
  |
1 | fn handle<F>() where F: FnOnce() -> () {}
  |                               ^^

Rationale and extra context

It feels like rustc should be able to determine that a trait followed by a -> is a Fn*-family trait bound and be able to suggest this behavior. If that is too general it could just check for Fn, FnOnce, FnMut.

Other cases

Not that i know of...

Very similar is if parenthesis are missing from regular fn declarations:

error: expected one of `(` or `<`, found `->`
 --> src/lib.rs:3:9
  |
3 | fn main -> () {}
  |         ^^ expected one of `(` or `<`

Anything else?

Also happens on nightly according to Rust Playground.

I'd like to try my hand at implementing this diagnostic, but I'll likely need a little bit of locational guidance. A naive code search show that the message is emitted by this code is the parser diagnostics, but I'm not sure if the adjustment would need to be made within that or from where it is invoked in this case.

pub(super) fn expected_one_of_not_found(
&mut self,
edible: &[TokenKind],
inedible: &[TokenKind],
) -> PResult<'a, bool /* recovered */> {


Edit: Updated to use the wording "Fn*-family trait bounds" rather than "HRTBs".

@Fishrock123 Fishrock123 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 Feb 16, 2023
@compiler-errors
Copy link
Member

but I'm not sure if the adjustment would need to be made within that or from where it is invoked in this case

Where this function is invoked. Try looking for the code that parses "parenthesized" generics.

@fmease
Copy link
Member

fmease commented Feb 16, 2023

@Fishrock123 If you end up writing a fix, note that those trait bounds are usually called Fn*-family trait bounds, not “HRTBs”. The latter would require at least one higher-rank parameter to be involved (e.g. for<'a>).

@Fishrock123 Fishrock123 changed the title Diagnostic when Fn HRTB is missing parens could be improved Diagnostic when Fn*-family trait bounds is missing parens could be improved Feb 16, 2023
@Fishrock123
Copy link
Contributor Author

It seems this diagnostic contextually comes from parse_fn_params() which does this token search by parse_paren_comma_seq().

As corroboration it also seems that declaring a regular fn without parenthesis causes a similar if more contextually understandable error:

error: expected one of `(` or `<`, found `->`
 --> src/lib.rs:3:9
  |
3 | fn main -> () {}
  |         ^^ expected one of `(` or `<`

/// Parses the parameter list of a function, including the `(` and `)` delimiters.
pub(super) fn parse_fn_params(&mut self, req_name: ReqName) -> PResult<'a, Vec<Param>> {
let mut first_param = true;
// Parse the arguments, starting out with `self` being allowed...
let (mut params, _) = self.parse_paren_comma_seq(|p| {
p.recover_diff_marker();
let param = p.parse_param_general(req_name, first_param).or_else(|mut e| {
e.emit();
let lo = p.prev_token.span;
// Skip every token until next possible arg or end.
p.eat_to_tokens(&[&token::Comma, &token::CloseDelim(Delimiter::Parenthesis)]);
// Create a placeholder argument for proper arg count (issue #34264).
Ok(dummy_arg(Ident::new(kw::Empty, lo.to(p.prev_token.span))))
});
// ...now that we've parsed the first argument, `self` is no longer allowed.
first_param = false;
param
})?;
// Replace duplicated recovered params with `_` pattern to avoid unnecessary errors.
self.deduplicate_recovered_params_names(&mut params);
Ok(params)
}

I naively think that the adjustment point should probably be parse_fn_params() rather than parse_paren_comma_seq()...

However I don't understand how the output gets multiple possibilities e.g. ( and < in this output:

3 | fn main -> () {}
  |         ^^ expected one of `(` or `<`

or the whole list of

expected one of `(`, `+`, `,`, `::`, `<`, or `{`

Since it's only calling Parser::expect() with Delimiter::Parenthesis from self.parse_delim_comma_seq(Delimiter::Parenthesis, f)...

I guess this requires catching the result in parse_fn_params and handling this result type somehow?

pub type PErr<'a> = DiagnosticBuilder<'a, ErrorGuaranteed>;

@compiler-errors
Copy link
Member

Any time you call Parser::check, it stashes the token type you tried probing for and it'll present that in a list whenever you get to expected_one_of_not_found. (Or something like that).

@compiler-errors
Copy link
Member

compiler-errors commented Feb 16, 2023

Anyways, it shouldn't matter -- in the caller to parse_fn_params (when parsing Fn ->), you could add a check for -> before calling parse_fn_params and handle the -> token specifically.

@estebank estebank added the A-parser Area: The parsing of Rust source code to an AST label Feb 18, 2023
@estebank
Copy link
Contributor

Just chiming in to mention: in any parser change you make here make sure you don't start accepting new code and add tests for all combinations you can think of.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Nov 1, 2023
…rochenkov

Recover from missing param list in function definitions

Addresses the other issue mentioned in rust-lang#108109
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Nov 1, 2023
…rochenkov

Recover from missing param list in function definitions

Addresses the other issue mentioned in rust-lang#108109
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 2, 2023
Rollup merge of rust-lang#117298 - clubby789:fn-missing-params, r=petrochenkov

Recover from missing param list in function definitions

Addresses the other issue mentioned in rust-lang#108109
@bors bors closed this as completed in 187d1af Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-parser Area: The parsing of Rust source code to an AST T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants