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

Parse expression after else as a condition if followed by { #97298

Merged
merged 1 commit into from
May 24, 2022
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
62 changes: 57 additions & 5 deletions compiler/rustc_parse/src/parser/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2010,6 +2010,12 @@ impl<'a> Parser<'a> {
Ok(self.mk_expr(blk.span, ExprKind::Block(blk, opt_label), attrs))
}

/// Parse a block which takes no attributes and has no label
fn parse_simple_block(&mut self) -> PResult<'a, P<Expr>> {
let blk = self.parse_block()?;
Ok(self.mk_expr(blk.span, ExprKind::Block(blk, None), AttrVec::new()))
}

/// Recover on an explicitly quantified closure expression, e.g., `for<'a> |x: &'a u8| *x + 1`.
fn recover_quantified_closure_expr(&mut self, attrs: AttrVec) -> PResult<'a, P<Expr>> {
let lo = self.token.span;
Expand Down Expand Up @@ -2157,14 +2163,22 @@ impl<'a> Parser<'a> {
let lo = self.prev_token.span;
let cond = self.parse_cond_expr()?;

self.parse_if_after_cond(attrs, lo, cond)
}

fn parse_if_after_cond(
&mut self,
attrs: AttrVec,
lo: Span,
cond: P<Expr>,
) -> PResult<'a, P<Expr>> {
let missing_then_block_binop_span = || {
match cond.kind {
ExprKind::Binary(Spanned { span: binop_span, .. }, _, ref right)
if let ExprKind::Block(..) = right.kind => Some(binop_span),
_ => None
}
};

// Verify that the parsed `if` condition makes sense as a condition. If it is a block, then
// verify that the last statement is either an implicit return (no `;`) or an explicit
// return. This won't catch blocks with an explicit `return`, but that would be caught by
Expand Down Expand Up @@ -2256,15 +2270,53 @@ impl<'a> Parser<'a> {

/// Parses an `else { ... }` expression (`else` token already eaten).
fn parse_else_expr(&mut self) -> PResult<'a, P<Expr>> {
let ctx_span = self.prev_token.span; // `else`
let else_span = self.prev_token.span; // `else`
let attrs = self.parse_outer_attributes()?.take_for_recovery(); // For recovery.
let expr = if self.eat_keyword(kw::If) {
self.parse_if_expr(AttrVec::new())?
} else if self.check(&TokenKind::OpenDelim(Delimiter::Brace)) {
self.parse_simple_block()?
} else {
let blk = self.parse_block()?;
self.mk_expr(blk.span, ExprKind::Block(blk, None), AttrVec::new())
let snapshot = self.create_snapshot_for_diagnostic();
let first_tok = super::token_descr(&self.token);
let first_tok_span = self.token.span;
match self.parse_expr() {
Ok(cond)
// If it's not a free-standing expression, and is followed by a block,
// then it's very likely the condition to an `else if`.
if self.check(&TokenKind::OpenDelim(Delimiter::Brace))
&& classify::expr_requires_semi_to_be_stmt(&cond) =>
{
Comment on lines +2284 to +2289
Copy link
Contributor

Choose a reason for hiding this comment

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

Given all the checks you have here...

self.struct_span_err(first_tok_span, format!("expected `{{`, found {first_tok}"))
.span_label(else_span, "expected an `if` or a block after this `else`")
.span_suggestion(
cond.span.shrink_to_lo(),
"add an `if` if this is the condition to an chained `if` statement after the `else`",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"add an `if` if this is the condition to an chained `if` statement after the `else`",
"add an `if` if this is the condition to a chained `if` statement after the `else`",

Copy link
Member Author

Choose a reason for hiding this comment

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

#97370 addresses this suggestion and @estebank's above.

"if ".to_string(),
Applicability::MaybeIncorrect,
).multipart_suggestion(
"... otherwise, place this expression inside of a block if it is not an `if` condition",
vec![
(cond.span.shrink_to_lo(), "{ ".to_string()),
(cond.span.shrink_to_hi(), " }".to_string()),
],
Applicability::MaybeIncorrect,
)
Comment on lines +2297 to +2304
Copy link
Contributor

Choose a reason for hiding this comment

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

...I would have likely skipped this suggestion :)

.emit();
self.parse_if_after_cond(AttrVec::new(), cond.span.shrink_to_lo(), cond)?
}
Err(e) => {
e.cancel();
self.restore_snapshot(snapshot);
self.parse_simple_block()?
},
Ok(_) => {
self.restore_snapshot(snapshot);
self.parse_simple_block()?
},
}
};
self.error_on_if_block_attrs(ctx_span, true, expr.span, &attrs);
self.error_on_if_block_attrs(else_span, true, expr.span, &attrs);
Ok(expr)
}

Expand Down
32 changes: 32 additions & 0 deletions src/test/ui/parser/else-no-if.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
fn foo() {
if true {
} else false {
//~^ ERROR expected `{`, found keyword `false`
}
}

fn foo2() {
if true {
} else falsy() {
//~^ ERROR expected `{`, found `falsy`
}
}

fn foo3() {
if true {
} else falsy();
//~^ ERROR expected `{`, found `falsy`
}

fn foo4() {
if true {
} else loop{}
//~^ ERROR expected `{`, found keyword `loop`
{}
}

fn falsy() -> bool {
false
}

fn main() {}
58 changes: 58 additions & 0 deletions src/test/ui/parser/else-no-if.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
error: expected `{`, found keyword `false`
--> $DIR/else-no-if.rs:3:12
|
LL | } else false {
| ---- ^^^^^
| |
| expected an `if` or a block after this `else`
|
help: add an `if` if this is the condition to an chained `if` statement after the `else`
|
LL | } else if false {
| ++
help: ... otherwise, place this expression inside of a block if it is not an `if` condition
|
LL | } else { false } {
| + +

error: expected `{`, found `falsy`
--> $DIR/else-no-if.rs:10:12
|
LL | } else falsy() {
| ---- ^^^^^
| |
| expected an `if` or a block after this `else`
|
help: add an `if` if this is the condition to an chained `if` statement after the `else`
|
LL | } else if falsy() {
| ++
help: ... otherwise, place this expression inside of a block if it is not an `if` condition
|
LL | } else { falsy() } {
| + +

error: expected `{`, found `falsy`
--> $DIR/else-no-if.rs:17:12
|
LL | } else falsy();
| ^^^^^ expected `{`
|
help: try placing this code inside a block
|
LL | } else { falsy() };
| + +

error: expected `{`, found keyword `loop`
--> $DIR/else-no-if.rs:23:12
|
LL | } else loop{}
| ^^^^ expected `{`
|
help: try placing this code inside a block
|
LL | } else { loop{} }
| + +

error: aborting due to 4 previous errors