Auto merge of #97474 - compiler-errors:if-cond-and-block, r=oli-obk

Improve parsing errors and suggestions for bad `if` statements

1. Parses `if {}` as `if <err> {}` (block-like conditions that are missing a "then" block), and `if true && {}` as `if true && <err> {}` (unfinished binary operation), which is a more faithful recovery and leads to better typeck errors later on.
1. Points out the span of the condition if we don't see a "then" block after it, to help the user understand what is being parsed as a condition (and by elimination, what isn't).
1. Allow `if cond token else { }` to be fixed properly to `if cond { token } else { }`.
1. Fudge with the error messages a bit. This is somewhat arbitrary and I can revert my rewordings if they're useless.

----

Also this PR addresses a strange parsing regression (1.20 -> 1.21) where we chose to reject this piece of code somewhat arbitrarily, even though we should parse it fine:

```rust
fn main() {
    if { if true { return } else { return }; } {}
}
```

For context, all of these other expressions parse correctly:

```rust
fn main() {
    if { if true { return } else { return } } {}
    if { return; } {}
    if { return } {}
    if { return if true { } else { }; } {}
}
```

The parser used a heuristic to determine if the "the parsed `if` condition makes sense as a condition" that did like a one-expr-deep reachability analysis. This should not be handled by the parser though.

Author: bors

compiler/rustc_parse/src/parser/expr.rs

@@ -2248,36 +2248,59 @@ impl<'a> Parser<'a> {
         &mut self,
         attrs: AttrVec,
         lo: Span,
-        cond: P<Expr>,
+        mut 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
+        let cond_span = cond.span;
+        // Tries to interpret `cond` as either a missing expression if it's a block,
+        // or as an unfinished expression if it's a binop and the RHS is a block.
+        // We could probably add more recoveries here too...
+        let mut recover_block_from_condition = |this: &mut Self| {
+            let block = match &mut cond.kind {
+                ExprKind::Binary(Spanned { span: binop_span, .. }, _, right)
+                    if let ExprKind::Block(_, None) = right.kind => {
+                        this.error_missing_if_then_block(lo, cond_span.shrink_to_lo().to(*binop_span), true).emit();
+                        std::mem::replace(right, this.mk_expr_err(binop_span.shrink_to_hi()))
+                    },
+                ExprKind::Block(_, None) => {
+                    this.error_missing_if_cond(lo, cond_span).emit();
+                    std::mem::replace(&mut cond, this.mk_expr_err(cond_span.shrink_to_hi()))
+                }
+                _ => {
+                    return None;
+                }
+            };
+            if let ExprKind::Block(block, _) = &block.kind {
+                Some(block.clone())
+            } else {
+                unreachable!()
             }
         };
-        // 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
-        // the dead code lint.
-        let thn = if self.token.is_keyword(kw::Else) || !cond.returns() {
-            if let Some(binop_span) = missing_then_block_binop_span() {
-                self.error_missing_if_then_block(lo, None, Some(binop_span)).emit();
-                self.mk_block_err(cond.span)
+        // Parse then block
+        let thn = if self.token.is_keyword(kw::Else) {
+            if let Some(block) = recover_block_from_condition(self) {
+                block
             } else {
-                self.error_missing_if_cond(lo, cond.span)
+                self.error_missing_if_then_block(lo, cond_span, false).emit();
+                self.mk_block_err(cond_span.shrink_to_hi())
             }
         } else {
             let attrs = self.parse_outer_attributes()?.take_for_recovery(); // For recovery.
-            let not_block = self.token != token::OpenDelim(Delimiter::Brace);
-            let block = self.parse_block().map_err(|err| {
-                if not_block {
-                    self.error_missing_if_then_block(lo, Some(err), missing_then_block_binop_span())
+            let block = if self.check(&token::OpenDelim(Delimiter::Brace)) {
+                self.parse_block()?
+            } else {
+                if let Some(block) = recover_block_from_condition(self) {
+                    block
                 } else {
-                    err
+                    // Parse block, which will always fail, but we can add a nice note to the error
+                    self.parse_block().map_err(|mut err| {
+                        err.span_note(
+                            cond_span,
+                            "the `if` expression is missing a block after this condition",
+                        );
+                        err
+                    })?
                 }
-            })?;
+            };
             self.error_on_if_block_attrs(lo, false, block.span, &attrs);
             block
         };
@@ -2288,31 +2311,34 @@ impl<'a> Parser<'a> {
     fn error_missing_if_then_block(
         &self,
         if_span: Span,
-        err: Option<DiagnosticBuilder<'a, ErrorGuaranteed>>,
-        binop_span: Option<Span>,
+        cond_span: Span,
+        is_unfinished: bool,
     ) -> DiagnosticBuilder<'a, ErrorGuaranteed> {
-        let msg = "this `if` expression has a condition, but no block";
-
-        let mut err = if let Some(mut err) = err {
-            err.span_label(if_span, msg);
-            err
+        let mut err = self.struct_span_err(
+            if_span,
+            "this `if` expression is missing a block after the condition",
+        );
+        if is_unfinished {
+            err.span_help(cond_span, "this binary operation is possibly unfinished");
         } else {
-            self.struct_span_err(if_span, msg)
-        };
-
-        if let Some(binop_span) = binop_span {
-            err.span_help(binop_span, "maybe you forgot the right operand of the condition?");
+            err.span_help(cond_span.shrink_to_hi(), "add a block here");
         }
-
         err
     }
 
-    fn error_missing_if_cond(&self, lo: Span, span: Span) -> P<ast::Block> {
-        let sp = self.sess.source_map().next_point(lo);
-        self.struct_span_err(sp, "missing condition for `if` expression")
-            .span_label(sp, "expected if condition here")
-            .emit();
-        self.mk_block_err(span)
+    fn error_missing_if_cond(
+        &self,
+        lo: Span,
+        span: Span,
+    ) -> DiagnosticBuilder<'a, ErrorGuaranteed> {
+        let next_span = self.sess.source_map().next_point(lo);
+        let mut err = self.struct_span_err(next_span, "missing condition for `if` expression");
+        err.span_label(next_span, "expected condition here");
+        err.span_label(
+            self.sess.source_map().start_point(span),
+            "if this block is the condition of the `if` expression, then it must be followed by another block"
+        );
+        err
     }
 
     /// Parses the condition of a `if` or `while` expression.

compiler/rustc_parse/src/parser/stmt.rs

@@ -432,10 +432,23 @@ impl<'a> Parser<'a> {
         //
         // which is valid in other languages, but not Rust.
         match self.parse_stmt_without_recovery(false, ForceCollect::No) {
-            // If the next token is an open brace (e.g., `if a b {`), the place-
-            // inside-a-block suggestion would be more likely wrong than right.
+            // If the next token is an open brace, e.g., we have:
+            //
+            //     if expr other_expr {
+            //        ^    ^          ^- lookahead(1) is a brace
+            //        |    |- current token is not "else"
+            //        |- (statement we just parsed)
+            //
+            // the place-inside-a-block suggestion would be more likely wrong than right.
+            //
+            // FIXME(compiler-errors): this should probably parse an arbitrary expr and not
+            // just lookahead one token, so we can see if there's a brace after _that_,
+            // since we want to protect against:
+            //     `if 1 1 + 1 {` being suggested as  `if { 1 } 1 + 1 {`
+            //                                            +   +
             Ok(Some(_))
-                if self.look_ahead(1, |t| t == &token::OpenDelim(Delimiter::Brace))
+                if (!self.token.is_keyword(kw::Else)
+                    && self.look_ahead(1, |t| t == &token::OpenDelim(Delimiter::Brace)))
                     || do_not_suggest_help => {}
             // Do not suggest `if foo println!("") {;}` (as would be seen in test for #46836).
             Ok(Some(Stmt { kind: StmtKind::Empty, .. })) => {}

src/test/ui/did_you_mean/issue-46836-identifier-not-instead-of-negation.stderr

@@ -25,10 +25,16 @@ LL |         println!("Then when?");
 error: expected `{`, found `;`
   --> $DIR/issue-46836-identifier-not-instead-of-negation.rs:20:31
    |
-LL |     if not  // lack of braces is [sic]
-   |     -- this `if` expression has a condition, but no block
 LL |         println!("Then when?");
    |                               ^ expected `{`
+   |
+note: the `if` expression is missing a block after this condition
+  --> $DIR/issue-46836-identifier-not-instead-of-negation.rs:19:8
+   |
+LL |       if not  // lack of braces is [sic]
+   |  ________^
+LL | |         println!("Then when?");
+   | |______________________________^
 
 error: unexpected `2` after identifier
   --> $DIR/issue-46836-identifier-not-instead-of-negation.rs:26:24

src/test/ui/expr/if/if-without-block.rs

@@ -1,9 +1,7 @@
 fn main() {
     let n = 1;
     if 5 == {
-    //~^ NOTE this `if` expression has a condition, but no block
+    //~^ ERROR this `if` expression is missing a block after the condition
         println!("five");
     }
 }
-//~^ ERROR expected `{`, found `}`
-//~| NOTE expected `{`

src/test/ui/expr/if/if-without-block.stderr

@@ -1,17 +1,14 @@
-error: expected `{`, found `}`
-  --> $DIR/if-without-block.rs:7:1
+error: this `if` expression is missing a block after the condition
+  --> $DIR/if-without-block.rs:3:5
    |
 LL |     if 5 == {
-   |     -- this `if` expression has a condition, but no block
-...
-LL | }
-   | ^ expected `{`
+   |     ^^
    |
-help: maybe you forgot the right operand of the condition?
-  --> $DIR/if-without-block.rs:3:10
+help: this binary operation is possibly unfinished
+  --> $DIR/if-without-block.rs:3:8
    |
 LL |     if 5 == {
-   |          ^^
+   |        ^^^^
 
 error: aborting due to previous error
 

src/test/ui/issues/issue-39848.stderr

@@ -2,13 +2,19 @@ error: expected `{`, found `foo`
   --> $DIR/issue-39848.rs:3:21
    |
 LL |         if $tgt.has_$field() {}
-   |         --          ^^^^^^ expected `{`
-   |         |
-   |         this `if` expression has a condition, but no block
+   |                     ^^^^^^ expected `{`
 ...
 LL |     get_opt!(bar, foo);
    |     ------------------ in this macro invocation
    |
+note: the `if` expression is missing a block after this condition
+  --> $DIR/issue-39848.rs:3:12
+   |
+LL |         if $tgt.has_$field() {}
+   |            ^^^^^^^^^
+...
+LL |     get_opt!(bar, foo);
+   |     ------------------ in this macro invocation
    = note: this error originates in the macro `get_opt` (in Nightly builds, run with -Z macro-backtrace for more info)
 help: try placing this code inside a block
    |

src/test/ui/missing/missing-block-hint.stderr

@@ -2,18 +2,25 @@ error: expected `{`, found `=>`
   --> $DIR/missing-block-hint.rs:3:18
    |
 LL |         if (foo) => {}
-   |         --       ^^ expected `{`
-   |         |
-   |         this `if` expression has a condition, but no block
+   |                  ^^ expected `{`
+   |
+note: the `if` expression is missing a block after this condition
+  --> $DIR/missing-block-hint.rs:3:12
+   |
+LL |         if (foo) => {}
+   |            ^^^^^
 
 error: expected `{`, found `bar`
   --> $DIR/missing-block-hint.rs:7:13
    |
-LL |         if (foo)
-   |         -- this `if` expression has a condition, but no block
 LL |             bar;
    |             ^^^ expected `{`
    |
+note: the `if` expression is missing a block after this condition
+  --> $DIR/missing-block-hint.rs:6:12
+   |
+LL |         if (foo)
+   |            ^^^^^
 help: try placing this code inside a block
    |
 LL |             { bar; }

src/test/ui/parser/bad-if-statements.rs

@@ -0,0 +1,38 @@
+fn a() {
+    if {}
+    //~^ ERROR missing condition for `if` expression
+}
+
+fn b() {
+    if true && {}
+    //~^ ERROR this `if` expression is missing a block after the condition
+}
+
+fn c() {
+    let x = {};
+    if true x
+    //~^ ERROR expected `{`, found `x`
+}
+
+fn a2() {
+    if {} else {}
+    //~^ ERROR missing condition for `if` expression
+}
+
+fn b2() {
+    if true && {} else {}
+    //~^ ERROR this `if` expression is missing a block after the condition
+}
+
+fn c2() {
+    let x = {};
+    if true x else {}
+    //~^ ERROR expected `{`, found `x`
+}
+
+fn d() {
+    if true else {}
+    //~^ ERROR this `if` expression is missing a block after the condition
+}
+
+fn main() {}

src/test/ui/parser/bad-if-statements.stderr

@@ -0,0 +1,86 @@
+error: missing condition for `if` expression
+  --> $DIR/bad-if-statements.rs:2:7
+   |
+LL |     if {}
+   |       ^- if this block is the condition of the `if` expression, then it must be followed by another block
+   |       |
+   |       expected condition here
+
+error: this `if` expression is missing a block after the condition
+  --> $DIR/bad-if-statements.rs:7:5
+   |
+LL |     if true && {}
+   |     ^^
+   |
+help: this binary operation is possibly unfinished
+  --> $DIR/bad-if-statements.rs:7:8
+   |
+LL |     if true && {}
+   |        ^^^^^^^
+
+error: expected `{`, found `x`
+  --> $DIR/bad-if-statements.rs:13:13
+   |
+LL |     if true x
+   |             ^ expected `{`
+   |
+note: the `if` expression is missing a block after this condition
+  --> $DIR/bad-if-statements.rs:13:8
+   |
+LL |     if true x
+   |        ^^^^
+help: try placing this code inside a block
+   |
+LL |     if true { x }
+   |             +   +
+
+error: missing condition for `if` expression
+  --> $DIR/bad-if-statements.rs:18:7
+   |
+LL |     if {} else {}
+   |       ^- if this block is the condition of the `if` expression, then it must be followed by another block
+   |       |
+   |       expected condition here
+
+error: this `if` expression is missing a block after the condition
+  --> $DIR/bad-if-statements.rs:23:5
+   |
+LL |     if true && {} else {}
+   |     ^^
+   |
+help: this binary operation is possibly unfinished
+  --> $DIR/bad-if-statements.rs:23:8
+   |
+LL |     if true && {} else {}
+   |        ^^^^^^^
+
+error: expected `{`, found `x`
+  --> $DIR/bad-if-statements.rs:29:13
+   |
+LL |     if true x else {}
+   |             ^ expected `{`
+   |
+note: the `if` expression is missing a block after this condition
+  --> $DIR/bad-if-statements.rs:29:8
+   |
+LL |     if true x else {}
+   |        ^^^^
+help: try placing this code inside a block
+   |
+LL |     if true { x } else {}
+   |             +   +
+
+error: this `if` expression is missing a block after the condition
+  --> $DIR/bad-if-statements.rs:34:5
+   |
+LL |     if true else {}
+   |     ^^
+   |
+help: add a block here
+  --> $DIR/bad-if-statements.rs:34:12
+   |
+LL |     if true else {}
+   |            ^
+
+error: aborting due to 7 previous errors
+