[Parse] Use empty RecoveryExpr when if/while/do/switch conditions fail to parse

This allows the body to be parsed.
An special-case that would replace a missing if condition with OpaqueValueExpr
was removed as it's now redundant (unless recovery-expr is disabled).

For loops are not handled at this point, as the parsing is more complicated.

Differential Revision: https://reviews.llvm.org/D113752

clang/include/clang/Parse/Parser.h

@@ -1975,6 +1975,7 @@ private:
   Sema::ConditionResult ParseCXXCondition(StmtResult *InitStmt,
                                           SourceLocation Loc,
                                           Sema::ConditionKind CK,
+                                          bool MissingOK,
                                           ForRangeInfo *FRI = nullptr,
                                           bool EnterForConditionScope = false);
   DeclGroupPtrTy
@@ -2079,8 +2080,8 @@ private:
   bool ParseParenExprOrCondition(StmtResult *InitStmt,
                                  Sema::ConditionResult &CondResult,
                                  SourceLocation Loc, Sema::ConditionKind CK,
-                                 SourceLocation *LParenLoc = nullptr,
+                                 bool MissingOK, SourceLocation *LParenLoc,
-                                 SourceLocation *RParenLoc = nullptr);
+                                 SourceLocation *RParenLoc);
   StmtResult ParseIfStatement(SourceLocation *TrailingElseLoc);
   StmtResult ParseSwitchStatement(SourceLocation *TrailingElseLoc);
   StmtResult ParseWhileStatement(SourceLocation *TrailingElseLoc);

clang/include/clang/Sema/Sema.h

@@ -12080,9 +12080,12 @@ public:
     ConstexprIf, ///< A constant boolean condition from 'if constexpr'.
     Switch       ///< An integral condition for a 'switch' statement.
   };
+  QualType PreferredConditionType(ConditionKind K) const {
+    return K == ConditionKind::Switch ? Context.IntTy : Context.BoolTy;
+  }
 
-  ConditionResult ActOnCondition(Scope *S, SourceLocation Loc,
+  ConditionResult ActOnCondition(Scope *S, SourceLocation Loc, Expr *SubExpr,
-                                 Expr *SubExpr, ConditionKind CK);
+                                 ConditionKind CK, bool MissingOK = false);
 
   ConditionResult ActOnConditionVariable(Decl *ConditionVar,
                                          SourceLocation StmtLoc,

clang/lib/AST/ExprConstant.cpp

@@ -4933,8 +4933,13 @@ static EvalStmtResult EvaluateSwitch(StmtResult &Result, EvalInfo &Info,
     if (SS->getConditionVariable() &&
         !EvaluateDecl(Info, SS->getConditionVariable()))
       return ESR_Failed;
-    if (!EvaluateInteger(SS->getCond(), Value, Info))
+    if (SS->getCond()->isValueDependent()) {
-      return ESR_Failed;
+      if (!EvaluateDependentExpr(SS->getCond(), Info))
+        return ESR_Failed;
+    } else {
+      if (!EvaluateInteger(SS->getCond(), Value, Info))
+        return ESR_Failed;
+    }
     if (!CondScope.destroy())
       return ESR_Failed;
   }

clang/lib/Parse/ParseExprCXX.cpp

@@ -1953,6 +1953,9 @@ Parser::ParseAliasDeclarationInInitStatement(DeclaratorContext Context,
 /// \param Loc The location of the start of the statement that requires this
 /// condition, e.g., the "for" in a for loop.
 ///
+/// \param MissingOK Whether an empty condition is acceptable here. Otherwise
+/// it is considered an error to be recovered from.
+///
 /// \param FRI If non-null, a for range declaration is permitted, and if
 /// present will be parsed and stored here, and a null result will be returned.
 ///
@@ -1960,11 +1963,10 @@ Parser::ParseAliasDeclarationInInitStatement(DeclaratorContext Context,
 /// appropriate moment for a 'for' loop.
 ///
 /// \returns The parsed condition.
-Sema::ConditionResult Parser::ParseCXXCondition(StmtResult *InitStmt,
+Sema::ConditionResult
-                                                SourceLocation Loc,
+Parser::ParseCXXCondition(StmtResult *InitStmt, SourceLocation Loc,
-                                                Sema::ConditionKind CK,
+                          Sema::ConditionKind CK, bool MissingOK,
-                                                ForRangeInfo *FRI,
+                          ForRangeInfo *FRI, bool EnterForConditionScope) {
-                                                bool EnterForConditionScope) {
   // Helper to ensure we always enter a continue/break scope if requested.
   struct ForConditionScopeRAII {
     Scope *S;
@@ -2019,7 +2021,7 @@ Sema::ConditionResult Parser::ParseCXXCondition(StmtResult *InitStmt,
       }
       ConsumeToken();
       *InitStmt = Actions.ActOnNullStmt(SemiLoc);
-      return ParseCXXCondition(nullptr, Loc, CK);
+      return ParseCXXCondition(nullptr, Loc, CK, MissingOK);
     }
 
     // Parse the expression.
@@ -2031,10 +2033,11 @@ Sema::ConditionResult Parser::ParseCXXCondition(StmtResult *InitStmt,
       WarnOnInit();
       *InitStmt = Actions.ActOnExprStmt(Expr.get());
       ConsumeToken();
-      return ParseCXXCondition(nullptr, Loc, CK);
+      return ParseCXXCondition(nullptr, Loc, CK, MissingOK);
     }
 
-    return Actions.ActOnCondition(getCurScope(), Loc, Expr.get(), CK);
+    return Actions.ActOnCondition(getCurScope(), Loc, Expr.get(), CK,
+                                  MissingOK);
   }
 
   case ConditionOrInitStatement::InitStmtDecl: {
@@ -2048,7 +2051,7 @@ Sema::ConditionResult Parser::ParseCXXCondition(StmtResult *InitStmt,
       DG = ParseSimpleDeclaration(DeclaratorContext::SelectionInit, DeclEnd,
                                   attrs, /*RequireSemi=*/true);
     *InitStmt = Actions.ActOnDeclStmt(DG, DeclStart, DeclEnd);
-    return ParseCXXCondition(nullptr, Loc, CK);
+    return ParseCXXCondition(nullptr, Loc, CK, MissingOK);
   }
 
   case ConditionOrInitStatement::ForRangeDecl: {

clang/lib/Parse/ParseStmt.cpp

@@ -1191,22 +1191,24 @@ StmtResult Parser::ParseCompoundStatementBody(bool isStmtExpr) {
 bool Parser::ParseParenExprOrCondition(StmtResult *InitStmt,
                                        Sema::ConditionResult &Cond,
                                        SourceLocation Loc,
-                                       Sema::ConditionKind CK,
+                                       Sema::ConditionKind CK, bool MissingOK,
                                        SourceLocation *LParenLoc,
                                        SourceLocation *RParenLoc) {
   BalancedDelimiterTracker T(*this, tok::l_paren);
   T.consumeOpen();
+  SourceLocation Start = Tok.getLocation();
 
-  if (getLangOpts().CPlusPlus)
+  if (getLangOpts().CPlusPlus) {
-    Cond = ParseCXXCondition(InitStmt, Loc, CK);
+    Cond = ParseCXXCondition(InitStmt, Loc, CK, MissingOK);
-  else {
+  } else {
     ExprResult CondExpr = ParseExpression();
 
     // If required, convert to a boolean value.
     if (CondExpr.isInvalid())
       Cond = Sema::ConditionError();
     else
-      Cond = Actions.ActOnCondition(getCurScope(), Loc, CondExpr.get(), CK);
+      Cond = Actions.ActOnCondition(getCurScope(), Loc, CondExpr.get(), CK,
+                                    MissingOK);
   }
 
   // If the parser was confused by the condition and we don't have a ')', try to
@@ -1220,7 +1222,16 @@ bool Parser::ParseParenExprOrCondition(StmtResult *InitStmt,
       return true;
   }
 
-  // Otherwise the condition is valid or the rparen is present.
+  if (Cond.isInvalid()) {
+    ExprResult CondExpr = Actions.CreateRecoveryExpr(
+        Start, Tok.getLocation() == Start ? Start : PrevTokLocation, {},
+        Actions.PreferredConditionType(CK));
+    if (!CondExpr.isInvalid())
+      Cond = Actions.ActOnCondition(getCurScope(), Loc, CondExpr.get(), CK,
+                                    MissingOK);
+  }
+
+  // Either the condition is valid or the rparen is present.
   T.consumeClose();
 
   if (LParenLoc != nullptr) {
@@ -1404,7 +1415,7 @@ StmtResult Parser::ParseIfStatement(SourceLocation *TrailingElseLoc) {
     if (ParseParenExprOrCondition(&InitStmt, Cond, IfLoc,
                                   IsConstexpr ? Sema::ConditionKind::ConstexprIf
                                               : Sema::ConditionKind::Boolean,
-                                  &LParen, &RParen))
+                                  /*MissingOK=*/false, &LParen, &RParen))
       return StmtError();
 
     if (IsConstexpr)
@@ -1599,7 +1610,8 @@ StmtResult Parser::ParseSwitchStatement(SourceLocation *TrailingElseLoc) {
   SourceLocation LParen;
   SourceLocation RParen;
   if (ParseParenExprOrCondition(&InitStmt, Cond, SwitchLoc,
-                                Sema::ConditionKind::Switch, &LParen, &RParen))
+                                Sema::ConditionKind::Switch,
+                                /*MissingOK=*/false, &LParen, &RParen))
     return StmtError();
 
   StmtResult Switch = Actions.ActOnStartOfSwitchStmt(
@@ -1689,7 +1701,8 @@ StmtResult Parser::ParseWhileStatement(SourceLocation *TrailingElseLoc) {
   SourceLocation LParen;
   SourceLocation RParen;
   if (ParseParenExprOrCondition(nullptr, Cond, WhileLoc,
-                                Sema::ConditionKind::Boolean, &LParen, &RParen))
+                                Sema::ConditionKind::Boolean,
+                                /*MissingOK=*/false, &LParen, &RParen))
     return StmtError();
 
   // C99 6.8.5p5 - In C99, the body of the while statement is a scope, even if
@@ -1780,10 +1793,18 @@ StmtResult Parser::ParseDoStatement() {
   // A do-while expression is not a condition, so can't have attributes.
   DiagnoseAndSkipCXX11Attributes();
 
+  SourceLocation Start = Tok.getLocation();
   ExprResult Cond = ParseExpression();
   // Correct the typos in condition before closing the scope.
   if (Cond.isUsable())
     Cond = Actions.CorrectDelayedTyposInExpr(Cond);
+  else {
+    if (!Tok.isOneOf(tok::r_paren, tok::r_square, tok::r_brace))
+      SkipUntil(tok::semi);
+    Cond = Actions.CreateRecoveryExpr(
+        Start, Start == Tok.getLocation() ? Start : PrevTokLocation, {},
+        Actions.getASTContext().BoolTy);
+  }
   T.consumeClose();
   DoScope.Exit();
 
@@ -2038,10 +2059,11 @@ StmtResult Parser::ParseForStatement(SourceLocation *TrailingElseLoc) {
         // for-range-declaration next.
         bool MightBeForRangeStmt = !ForRangeInfo.ParsedForRangeDecl();
         ColonProtectionRAIIObject ColonProtection(*this, MightBeForRangeStmt);
-        SecondPart =
+        SecondPart = ParseCXXCondition(
-            ParseCXXCondition(nullptr, ForLoc, Sema::ConditionKind::Boolean,
+            nullptr, ForLoc, Sema::ConditionKind::Boolean,
-                              MightBeForRangeStmt ? &ForRangeInfo : nullptr,
+            // FIXME: recovery if we don't see another semi!
-                              /*EnterForConditionScope*/ true);
+            /*MissingOK=*/true, MightBeForRangeStmt ? &ForRangeInfo : nullptr,
+            /*EnterForConditionScope*/ true);
 
         if (ForRangeInfo.ParsedForRangeDecl()) {
           Diag(FirstPart.get() ? FirstPart.get()->getBeginLoc()
@@ -2065,9 +2087,9 @@ StmtResult Parser::ParseForStatement(SourceLocation *TrailingElseLoc) {
         if (SecondExpr.isInvalid())
           SecondPart = Sema::ConditionError();
         else
-          SecondPart =
+          SecondPart = Actions.ActOnCondition(
-              Actions.ActOnCondition(getCurScope(), ForLoc, SecondExpr.get(),
+              getCurScope(), ForLoc, SecondExpr.get(),
-                                     Sema::ConditionKind::Boolean);
+              Sema::ConditionKind::Boolean, /*MissingOK=*/true);
       }
     }
   }

clang/lib/Sema/SemaExpr.cpp

@@ -19209,10 +19209,12 @@ ExprResult Sema::CheckBooleanCondition(SourceLocation Loc, Expr *E,
 }
 
 Sema::ConditionResult Sema::ActOnCondition(Scope *S, SourceLocation Loc,
-                                           Expr *SubExpr, ConditionKind CK) {
+                                           Expr *SubExpr, ConditionKind CK,
-  // Empty conditions are valid in for-statements.
+                                           bool MissingOK) {
+  // MissingOK indicates whether having no condition expression is valid
+  // (for loop) or invalid (e.g. while loop).
   if (!SubExpr)
-    return ConditionResult();
+    return MissingOK ? ConditionResult() : ConditionError();
 
   ExprResult Cond;
   switch (CK) {
@@ -19230,7 +19232,7 @@ Sema::ConditionResult Sema::ActOnCondition(Scope *S, SourceLocation Loc,
   }
   if (Cond.isInvalid()) {
     Cond = CreateRecoveryExpr(SubExpr->getBeginLoc(), SubExpr->getEndLoc(),
-                              {SubExpr});
+                              {SubExpr}, PreferredConditionType(CK));
     if (!Cond.get())
       return ConditionError();
   }

clang/lib/Sema/SemaStmt.cpp

@@ -869,12 +869,7 @@ StmtResult Sema::ActOnIfStmt(SourceLocation IfLoc,
                              Stmt *thenStmt, SourceLocation ElseLoc,
                              Stmt *elseStmt) {
   if (Cond.isInvalid())
-    Cond = ConditionResult(
+    return StmtError();
-        *this, nullptr,
-        MakeFullExpr(new (Context) OpaqueValueExpr(SourceLocation(),
-                                                   Context.BoolTy, VK_PRValue),
-                     IfLoc),
-        false);
 
   bool ConstevalOrNegatedConsteval =
       StatementKind == IfStatementKind::ConstevalNonNegated ||
@@ -2468,6 +2463,7 @@ StmtResult Sema::ActOnCXXForRangeStmt(Scope *S, SourceLocation ForLoc,
                                       Stmt *First, SourceLocation ColonLoc,
                                       Expr *Range, SourceLocation RParenLoc,
                                       BuildForRangeKind Kind) {
+  // FIXME: recover in order to allow the body to be parsed.
   if (!First)
     return StmtError();
 

clang/lib/Sema/TreeTransform.h

@@ -4076,7 +4076,8 @@ Sema::ConditionResult TreeTransform<Derived>::TransformCondition(
     if (CondExpr.isInvalid())
       return Sema::ConditionError();
 
-    return getSema().ActOnCondition(nullptr, Loc, CondExpr.get(), Kind);
+    return getSema().ActOnCondition(nullptr, Loc, CondExpr.get(), Kind,
+                                    /*MissingOK=*/true);
   }
 
   return Sema::ConditionResult();

clang/test/AST/ast-dump-invalid.cpp

@@ -33,7 +33,7 @@ int g(int i) {
 // CHECK-NEXT:   |-ParmVarDecl
 // CHECK-NEXT:   `-CompoundStmt
 // CHECK-NEXT:     `-IfStmt {{.*}} <line:25:3, line:28:12>
-// CHECK-NEXT:       |-OpaqueValueExpr {{.*}} <<invalid sloc>> 'bool'
+// CHECK-NEXT:       |-RecoveryExpr {{.*}} <line:25:7> 'bool'
 // CHECK-NEXT:       |-ReturnStmt {{.*}} <line:26:5, col:12>
 // CHECK-NEXT:       | `-IntegerLiteral {{.*}} <col:12> 'int' 4
 // CHECK-NEXT:       `-ReturnStmt {{.*}} <line:28:5, col:12>

clang/test/AST/loop-recovery.cpp

@@ -0,0 +1,65 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 %s
+// RUN: not %clang_cc1 -fsyntax-only -ast-dump %s -std=c++17 | FileCheck %s
+
+void test() {
+  while(!!!) // expected-error {{expected expression}}
+    int whileBody;
+  // CHECK: WhileStmt
+  // CHECK: RecoveryExpr {{.*}} <line:{{.*}}:9, col:11> 'bool'
+  // CHECK: whileBody 'int'
+
+  for(!!!) // expected-error {{expected expression}} expected-error {{expected ';'}}
+    int forBody;
+  // CHECK: ForStmt
+  // FIXME: the AST should have a RecoveryExpr to distinguish from for(;;)
+  // CHECK-NOT: RecoveryExpr
+  // CHECK: forBody 'int'
+
+  for(auto c : !!!) // expected-error {{expected expression}}
+    int forEachBody;
+  // FIXME: parse the foreach body
+  // CHECK-NOT: CXXForRangeStmt
+  // CHECK-NOT: forEachBody 'int'
+
+  do
+    int doBody;
+  while(!!!); // expected-error {{expected expression}}
+  // CHECK: DoStmt
+  // CHECK: doBody 'int'
+  // CHECK: RecoveryExpr {{.*}} <line:{{.*}}:9, col:11> 'bool'
+
+  if(!!!) // expected-error {{expected expression}}
+    int ifBody;
+  else
+    int elseBody;
+  // CHECK: IfStmt
+  // CHECK: RecoveryExpr {{.*}} <line:{{.*}}:6, col:8> 'bool'
+  // CHECK: ifBody 'int'
+  // CHECK: elseBody 'int'
+
+  switch(!!!) // expected-error {{expected expression}}
+    int switchBody;
+  // CHECK: SwitchStmt
+  // CHECK: RecoveryExpr {{.*}} <line:{{.*}}:10, col:12> 'int'
+  // CHECK: switchBody 'int'
+
+  switch (;) // expected-error {{expected expression}}
+    int switchBody;
+  // CHECK: SwitchStmt
+  // CHECK: NullStmt
+  // CHECK: RecoveryExpr {{.*}} <col:11> 'int'
+  // CHECK: switchBody 'int'
+
+  switch (;;) // expected-error {{expected expression}}
+    int switchBody;
+  // CHECK: SwitchStmt
+  // CHECK: NullStmt
+  // CHECK: RecoveryExpr {{.*}} <col:11, col:12> 'int'
+  // CHECK: switchBody 'int'
+
+  switch (!!!;) // expected-error {{expected expression}}
+    int switchBody;
+  // CHECK: SwitchStmt
+  // CHECK: RecoveryExpr {{.*}} <line:{{.*}}:11, col:14> 'int'
+  // CHECK: switchBody 'int'
+}

clang/test/Parser/cxx0x-attributes.cpp

@@ -152,6 +152,7 @@ void bad_attributes_in_do_while() {
       [[ab]ab] ns::i); // expected-error {{an attribute list cannot appear here}}
   do {} while ( // expected-note {{to match this '('}}
       alignas(4 ns::i; // expected-note {{to match this '('}}
+                       // expected-error@-1 {{expected ';' after do/while}}
 } // expected-error 2{{expected ')'}} expected-error {{expected expression}}
 
 [[]] using T = int; // expected-error {{an attribute list cannot appear here}}

clang/test/Sema/complex-int.c

@@ -18,8 +18,8 @@ result = arr*brr;
 result = xx*yy;
 
 switch (arr) { // expected-error{{statement requires expression of integer type ('_Complex int' invalid)}}
-  case brr: ;
+  case brr: ; // expected-error{{integer constant expression must have integer type}}
-  case xx: ;
+  case xx: ; // expected-error{{integer constant expression must have integer type}}
 }
 switch (ii) {
   case brr: ; // expected-error{{integer constant expression must have integer type}}

clang/test/SemaCXX/condition.cpp

@@ -20,6 +20,8 @@ void test() {
   while (struct S {} *x=0) ; // expected-error {{'S' cannot be defined in a condition}}
   while (struct {} *x=0) ; // expected-error-re {{'(unnamed struct at {{.*}})' cannot be defined in a condition}}
   switch (enum {E} x=0) ; // expected-error-re {{'(unnamed enum at {{.*}})' cannot be defined in a condition}}
+                          // expected-warning@-1 {{switch statement has empty body}}
+                          // expected-note@-2 {{put the semicolon on a separate line}}
 
   if (int x=0) { // expected-note 2 {{previous definition is here}}
     int x;  // expected-error {{redefinition of 'x'}}

clang/test/SemaCXX/constexpr-function-recovery-crash.cpp

@@ -77,3 +77,25 @@ constexpr void test11() {
 
 constexpr int test12() { return "wrong"; } // expected-error {{cannot initialize return object of type 'int'}}
 constexpr int force12 = test12();          // expected-error {{must be initialized by a constant}}
+
+#define TEST_EVALUATE(Name, X)         \
+  constexpr int testEvaluate##Name() { \
+    X return 0;                        \
+  }                                    \
+  constexpr int forceEvaluate##Name = testEvaluate##Name()
+// Check that a variety of broken loops don't crash constant evaluation.
+// We're not checking specific recovery here so don't assert diagnostics.
+TEST_EVALUATE(Switch, switch (!!){});              // expected-error + {{}}
+TEST_EVALUATE(SwitchInit, switch (auto x = !!){}); // expected-error + {{}}
+TEST_EVALUATE(For, for (!!){}); // expected-error + {{}}
+                                // FIXME: should bail out instead of looping.
+                                // expected-note@-2 + {{infinite loop}}
+                                // expected-note@-3 {{in call}}
+TEST_EVALUATE(ForRange, for (auto x : !!){}); // expected-error + {{}}
+TEST_EVALUATE(While, while (!!){});           // expected-error + {{}}
+TEST_EVALUATE(DoWhile, do {} while (!!););    // expected-error + {{}}
+TEST_EVALUATE(If, if (!!){};);                // expected-error + {{}}
+TEST_EVALUATE(IfInit, if (auto x = !!; 1){};);// expected-error + {{}}
+TEST_EVALUATE(ForInit, if (!!;;){};);         // expected-error + {{}}
+TEST_EVALUATE(ForCond, if (; !!;){};);        // expected-error + {{}}
+TEST_EVALUATE(ForInc, if (;; !!){};);         // expected-error + {{}}