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
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
2 changes: 2 additions & 0 deletions compiler/rustc_parse/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -526,6 +526,8 @@ parse_mismatched_closing_delimiter = mismatched closing delimiter: `{$delimiter}
.label_opening_candidate = closing delimiter possibly meant for this
.label_unclosed = unclosed delimiter

parse_misplaced_return_type = place the return type after the function parameters

parse_missing_comma_after_match_arm = expected `,` following `match` arm
.suggestion = missing a comma here to end this `match` arm

Expand Down
22 changes: 15 additions & 7 deletions compiler/rustc_parse/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1434,6 +1434,20 @@ pub(crate) struct FnPtrWithGenerics {
pub sugg: Option<FnPtrWithGenericsSugg>,
}

#[derive(Subdiagnostic)]
#[multipart_suggestion(
parse_misplaced_return_type,
style = "verbose",
applicability = "maybe-incorrect"
)]
pub(crate) struct MisplacedReturnType {
#[suggestion_part(code = " {snippet}")]
pub fn_params_end: Span,
pub snippet: String,
#[suggestion_part(code = "")]
pub ret_ty_span: Span,
}

#[derive(Subdiagnostic)]
#[multipart_suggestion(parse_suggestion, applicability = "maybe-incorrect")]
pub(crate) struct FnPtrWithGenericsSugg {
Expand All @@ -1448,7 +1462,6 @@ pub(crate) struct FnPtrWithGenericsSugg {

pub(crate) struct FnTraitMissingParen {
pub span: Span,
pub machine_applicable: bool,
}

impl Subdiagnostic for FnTraitMissingParen {
Expand All @@ -1458,16 +1471,11 @@ impl Subdiagnostic for FnTraitMissingParen {
_: &F,
) {
diag.span_label(self.span, crate::fluent_generated::parse_fn_trait_missing_paren);
let applicability = if self.machine_applicable {
Applicability::MachineApplicable
} else {
Applicability::MaybeIncorrect
};
diag.span_suggestion_short(
self.span.shrink_to_hi(),
crate::fluent_generated::parse_add_paren,
"()",
applicability,
Applicability::MachineApplicable,
);
}
}
Expand Down
8 changes: 4 additions & 4 deletions compiler/rustc_parse/src/parser/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,7 @@ impl<'a> Parser<'a> {
&mut self,
edible: &[TokenKind],
inedible: &[TokenKind],
) -> PResult<'a, Recovered> {
) -> PResult<'a, ErrorGuaranteed> {
debug!("expected_one_of_not_found(edible: {:?}, inedible: {:?})", edible, inedible);
fn tokens_to_string(tokens: &[TokenType]) -> String {
let mut i = tokens.iter();
Expand Down Expand Up @@ -533,7 +533,7 @@ impl<'a> Parser<'a> {
sugg: ExpectedSemiSugg::ChangeToSemi(self.token.span),
});
self.bump();
return Ok(Recovered::Yes(guar));
return Ok(guar);
} else if self.look_ahead(0, |t| {
t == &token::CloseDelim(Delimiter::Brace)
|| ((t.can_begin_expr() || t.can_begin_item())
Expand All @@ -557,7 +557,7 @@ impl<'a> Parser<'a> {
unexpected_token_label: Some(self.token.span),
sugg: ExpectedSemiSugg::AddSemi(span),
});
return Ok(Recovered::Yes(guar));
return Ok(guar);
}
}

Expand Down Expand Up @@ -712,7 +712,7 @@ impl<'a> Parser<'a> {
if self.check_too_many_raw_str_terminators(&mut err) {
if expected.contains(&TokenType::Token(token::Semi)) && self.eat(&token::Semi) {
let guar = err.emit();
return Ok(Recovered::Yes(guar));
return Ok(guar);
} else {
return Err(err);
}
Expand Down
124 changes: 96 additions & 28 deletions compiler/rustc_parse/src/parser/item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use rustc_span::edit_distance::edit_distance;
use rustc_span::edition::Edition;
use rustc_span::source_map;
use rustc_span::symbol::{kw, sym, Ident, Symbol};
use rustc_span::ErrorGuaranteed;
use rustc_span::{Span, DUMMY_SP};
use std::fmt::Write;
use std::mem;
Expand Down Expand Up @@ -2332,14 +2333,106 @@ impl<'a> Parser<'a> {
}
}
};

// Store the end of function parameters to give better diagnostics
// inside `parse_fn_body()`.
let fn_params_end = self.prev_token.span.shrink_to_hi();

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.


let mut sig_hi = self.prev_token.span;
let body = self.parse_fn_body(attrs, &ident, &mut sig_hi, fn_parse_mode.req_body)?; // `;` or `{ ... }`.
// Either `;` or `{ ... }`.
let body =
self.parse_fn_body(attrs, &ident, &mut sig_hi, fn_parse_mode.req_body, fn_params_end)?;
let fn_sig_span = sig_lo.to(sig_hi);
Ok((ident, FnSig { header, decl, span: fn_sig_span }, generics, body))
}

/// Provide diagnostics when function body is not found
fn error_fn_body_not_found(
&mut self,
ident_span: Span,
req_body: bool,
fn_params_end: Option<Span>,
) -> PResult<'a, ErrorGuaranteed> {
let expected = if req_body {
&[token::OpenDelim(Delimiter::Brace)][..]
} else {
&[token::Semi, token::OpenDelim(Delimiter::Brace)]
};
match self.expected_one_of_not_found(&[], expected) {
Ok(error_guaranteed) => Ok(error_guaranteed),
Err(mut err) => {
if self.token.kind == token::CloseDelim(Delimiter::Brace) {
// The enclosing `mod`, `trait` or `impl` is being closed, so keep the `fn` in
// the AST for typechecking.
err.span_label(ident_span, "while parsing this `fn`");
Ok(err.emit())
} else if self.token.kind == token::RArrow
&& let Some(fn_params_end) = fn_params_end
{
// Instead of a function body, the parser has encountered a right arrow
// preceded by a where clause.

// Find whether token behind the right arrow is a function trait and
// store its span.
let fn_trait_span =
[sym::FnOnce, sym::FnMut, sym::Fn].into_iter().find_map(|symbol| {
if self.prev_token.is_ident_named(symbol) {
Some(self.prev_token.span)
} else {
None
}
});

// Parse the return type (along with the right arrow) and store its span.
// If there's a parse error, cancel it and return the existing error
// as we are primarily concerned with the
// expected-function-body-but-found-something-else error here.
let arrow_span = self.token.span;
let ty_span = match self.parse_ret_ty(
AllowPlus::Yes,
RecoverQPath::Yes,
RecoverReturnSign::Yes,
) {
Ok(ty_span) => ty_span.span().shrink_to_hi(),
Err(parse_error) => {
parse_error.cancel();
return Err(err);
}
};
let ret_ty_span = arrow_span.to(ty_span);

if let Some(fn_trait_span) = fn_trait_span {
// Typo'd Fn* trait bounds such as
// fn foo<F>() where F: FnOnce -> () {}
err.subdiagnostic(errors::FnTraitMissingParen { span: fn_trait_span });
} else if let Ok(snippet) = self.psess.source_map().span_to_snippet(ret_ty_span)
{
// If token behind right arrow is not a Fn* trait, the programmer
// probably misplaced the return type after the where clause like
// `fn foo<T>() where T: Default -> u8 {}`
err.primary_message(
"return type should be specified after the function parameters",
);
err.subdiagnostic(errors::MisplacedReturnType {
fn_params_end,
snippet,
ret_ty_span,
});
}
Err(err)
} else {
Err(err)
}
}
}
}

/// Parse the "body" of a function.
/// This can either be `;` when there's no body,
/// or e.g. a block when the function is a provided one.
Expand All @@ -2349,6 +2442,7 @@ impl<'a> Parser<'a> {
ident: &Ident,
sig_hi: &mut Span,
req_body: bool,
fn_params_end: Option<Span>,
) -> PResult<'a, Option<P<Block>>> {
let has_semi = if req_body {
self.token.kind == TokenKind::Semi
Expand Down Expand Up @@ -2377,33 +2471,7 @@ impl<'a> Parser<'a> {
});
(AttrVec::new(), Some(self.mk_block_err(span, guar)))
} else {
let expected = if req_body {
&[token::OpenDelim(Delimiter::Brace)][..]
} else {
&[token::Semi, token::OpenDelim(Delimiter::Brace)]
};
if let Err(mut err) = self.expected_one_of_not_found(&[], expected) {
if self.token.kind == token::CloseDelim(Delimiter::Brace) {
// The enclosing `mod`, `trait` or `impl` is being closed, so keep the `fn` in
// the AST for typechecking.
err.span_label(ident.span, "while parsing this `fn`");
err.emit();
} else {
// check for typo'd Fn* trait bounds such as
// fn foo<F>() where F: FnOnce -> () {}
if self.token.kind == token::RArrow {
let machine_applicable = [sym::FnOnce, sym::FnMut, sym::Fn]
.into_iter()
.any(|s| self.prev_token.is_ident_named(s));

err.subdiagnostic(errors::FnTraitMissingParen {
span: self.prev_token.span,
machine_applicable,
});
}
return Err(err);
}
}
self.error_fn_body_not_found(ident.span, req_body, fn_params_end)?;
(AttrVec::new(), None)
};
attrs.extend(inner_attrs);
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_parse/src/parser/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -506,6 +506,7 @@ impl<'a> Parser<'a> {
FatalError.raise();
} else {
self.expected_one_of_not_found(edible, inedible)
.map(|error_guaranteed| Recovered::Yes(error_guaranteed))
}
}

Expand Down
5 changes: 5 additions & 0 deletions compiler/rustc_parse/src/parser/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ use rustc_span::symbol::{kw, sym, Ident};
use rustc_span::{ErrorGuaranteed, Span, Symbol};
use thin_vec::{thin_vec, ThinVec};

/// Signals whether parsing a type should allow `+`.
///
/// For example, let T be the type `impl Default + 'static`
/// With `AllowPlus::Yes`, T will be parsed successfully
/// With `AllowPlus::No`, parsing T will return a parse error
#[derive(Copy, Clone, PartialEq)]
pub(super) enum AllowPlus {
Yes,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
fn foo<T>() where T: Default -> impl Default + 'static {}
//~^ ERROR return type should be specified after the function parameters
//~| HELP place the return type after the function parameters

fn main() {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error: return type should be specified after the function parameters
--> $DIR/misplaced-return-type-complex-type-issue-126311.rs:1:30
|
LL | fn foo<T>() where T: Default -> impl Default + 'static {}
| ^^ expected one of `(`, `+`, `,`, `::`, `<`, or `{`
|
help: place the return type after the function parameters
|
LL - fn foo<T>() where T: Default -> impl Default + 'static {}
LL + fn foo<T>() -> impl Default + 'static where T: Default {}
|

error: aborting due to 1 previous error

5 changes: 5 additions & 0 deletions tests/ui/parser/issues/misplaced-return-type-issue-126311.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
fn foo<T>() where T: Default -> u8 {}
//~^ ERROR return type should be specified after the function parameters
//~| HELP place the return type after the function parameters

fn main() {}
14 changes: 14 additions & 0 deletions tests/ui/parser/issues/misplaced-return-type-issue-126311.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error: return type should be specified after the function parameters
--> $DIR/misplaced-return-type-issue-126311.rs:1:30
|
LL | fn foo<T>() where T: Default -> u8 {}
| ^^ expected one of `(`, `+`, `,`, `::`, `<`, or `{`
|
help: place the return type after the function parameters
|
LL - fn foo<T>() where T: Default -> u8 {}
LL + fn foo<T>() -> u8 where T: Default {}
|

error: aborting due to 1 previous error

Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
fn foo<T, K>()
//~^ HELP place the return type after the function parameters
where
T: Default,
K: Clone, -> Result<u8, String>
//~^ ERROR return type should be specified after the function parameters
{
Ok(0)
}

fn main() {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
error: return type should be specified after the function parameters
--> $DIR/misplaced-return-type-where-in-next-line-issue-126311.rs:5:15
|
LL | K: Clone, -> Result<u8, String>
| ^^ expected one of `{`, lifetime, or type
|
help: place the return type after the function parameters
|
LL ~ fn foo<T, K>() -> Result<u8, String>
LL |
LL | where
LL | T: Default,
LL ~ K: Clone,
|

error: aborting due to 1 previous error

Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
fn foo<T>() where T: Default -> {
//~^ ERROR expected one of `(`, `+`, `,`, `::`, `<`, or `{`, found `->`
0
}

fn main() {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
error: expected one of `(`, `+`, `,`, `::`, `<`, or `{`, found `->`
--> $DIR/misplaced-return-type-without-type-issue-126311.rs:1:30
|
LL | fn foo<T>() where T: Default -> {
| ^^ expected one of `(`, `+`, `,`, `::`, `<`, or `{`

error: aborting due to 1 previous error

Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
fn bar<T>() -> u8 -> u64 {}
//~^ ERROR expected one of `!`, `(`, `+`, `::`, `<`, `where`, or `{`, found `->`

fn main() {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
error: expected one of `!`, `(`, `+`, `::`, `<`, `where`, or `{`, found `->`
--> $DIR/misplaced-return-type-without-where-issue-126311.rs:1:19
|
LL | fn bar<T>() -> u8 -> u64 {}
| ^^ expected one of 7 possible tokens

error: aborting due to 1 previous error

Loading