Recovery of switch statement

[sepavloff]

Summary

Fix to PR19022 as well as some other problems of error recovery in switch statement.

[sepavloff]

Updated patch.

Added comments, tests, handle more cases.
Please review.

Thank you.

[rsmith]

lib/Parse/ParseStmt.cpp

701–705 ↗

(On Diff #8396)

This doesn't look right in the case where ColonLoc is invalid. AfterColonLoc will point to the *start* of the previous token in this case, and we'll provide a fix-it inserting a semicolon in a fairly random place.

In the case where ColonLoc is invalid, we've already hit at least one error, so it seems better to suppress the follow-on diagnostic here.

I think this will also do the wrong thing if we have a sequence of case statements, and the last one is missing a colon or is otherwise malformed; we'll use the last ColonLoc for any of the case statements we succeeded in handling. Maybe reset ColonLoc to SourceLocation() each time around the do loop to fix this?

lib/Sema/SemaStmt.cpp

707–708 ↗

(On Diff #8396)

This comment doesn't explain what's happening here. Something like:

// This happens if the body was ill-formed. Synthesize a null statement at the end of the switch condition.

... would be better. But our normal approach here would be to simply return StmtError() in this case. Any reason not to do that?

[sepavloff]

Thank you for review.

lib/Parse/ParseStmt.cpp

701–705 ↗

(On Diff #8396)

Yes, this is complicated point, thank you for the detailed explanation. It looks like there is no reliable way to determine location for error message if ColonLoc is undefined due to previous error. Suppressing follow-on diagnostic gives better results.

Resetting ColonLoc at the beginning of case label is also necessary.

lib/Sema/SemaStmt.cpp

707–708 ↗

(On Diff #8396)

It looks like preparing bogus statement body is not necessary, StmtError is enough.

[sepavloff]

Updated patch.

[rsmith]

Looks fine with some tiny fixes.

lib/Parse/ParseStmt.cpp

706 ↗

(On Diff #8713)

"a valid text location"
"an extra diagnostic" or "extra diagnostics"?

lib/Sema/SemaStmt.cpp

705–706 ↗

(On Diff #8713)

Why the extra variable?

[sepavloff]

Closed by commit rL209302 (authored by @sepavloff).

[sepavloff]

Thank you for review!
And special thanks for language corrections.

2014-05-21 8:07 GMT+07:00 Richard Smith <[e-mail]>:

Looks fine with some tiny fixes.

Comment at: lib/Parse/ParseStmt.cpp:706
@@ +705,3 @@
+ not valid. If ColonLoc doesn't point to valid text location,
there was
+ another parsing error, so don't produce extra diagnostic.

+ if (ColonLoc.isValid()) {

"a valid text location"
"an extra diagnostic" or "extra diagnostics"?

Comment at: lib/Sema/SemaStmt.cpp:705-706
@@ -704,2 +704,4 @@

• SS->setBody(BodyStmt, SwitchLoc);

+ Stmt *Body = BodyStmt;
+ if (!Body) return StmtError();

+ SS->setBody(Body, SwitchLoc);

Why the extra variable?

http://reviews.llvm.org/D3137

cfe/trunk/lib/Parse/ParseStmt.cpp

StmtResult Parser::ParseCaseStatement(bool MissingCase, ExprResult Expr) {
   // far.  When parsing 'case 4', this is the 'case 3' node.
   Stmt *DeepestParsedCaseStmt = nullptr;
 
   // While we have case statements, eat and stack them.
   SourceLocation ColonLoc;
   do {
     SourceLocation CaseLoc = MissingCase ? Expr.get()->getExprLoc() :
                                            ConsumeToken();  // eat the 'case'.
+    ColonLoc = SourceLocation();
 
     if (Tok.is(tok::code_completion)) {
       Actions.CodeCompleteCase(getCurScope());
       cutOffParsing();
       return StmtError();
     }
 
     /// We don't want to treat 'case x : y' as a potential typo for 'case x::y'.
     /// Disable this form of error recovery while we're parsing the case
     /// expression.
     ColonProtectionRAIIObject ColonProtection(*this);
 
-    ExprResult LHS(MissingCase ? Expr : ParseConstantExpression());
-    MissingCase = false;
+    ExprResult LHS;
+    if (!MissingCase) {
+      LHS = ParseConstantExpression();
       if (LHS.isInvalid()) {
-      SkipUntil(tok::colon, StopAtSemi);
+        // If constant-expression is parsed unsuccessfully, recover by skipping
+        // current case statement (moving to the colon that ends it).
+        if (SkipUntil(tok::colon, tok::r_brace, StopAtSemi | StopBeforeMatch)) {
+          TryConsumeToken(tok::colon, ColonLoc);
+          continue;
+        }
         return StmtError();
       }
+    } else {
+      LHS = Expr;
+      MissingCase = false;
+    }
 
     // GNU case range extension.
     SourceLocation DotDotDotLoc;
     ExprResult RHS;
     if (TryConsumeToken(tok::ellipsis, DotDotDotLoc)) {
       Diag(DotDotDotLoc, diag::ext_gnu_case_range);
       RHS = ParseConstantExpression();
       if (RHS.isInvalid()) {
-        SkipUntil(tok::colon, StopAtSemi);
+        if (SkipUntil(tok::colon, tok::r_brace, StopAtSemi | StopBeforeMatch)) {
+          TryConsumeToken(tok::colon, ColonLoc);
+          continue;
+        }
         return StmtError();
       }
     }
 
     ColonProtection.restore();
 
     if (TryConsumeToken(tok::colon, ColonLoc)) {
     } else if (TryConsumeToken(tok::semi, ColonLoc) ||
if (Case.isInvalid()) {
       else
         Actions.ActOnCaseStmtBody(DeepestParsedCaseStmt, Case.get());
       DeepestParsedCaseStmt = NextDeepest;
     }
 
     // Handle all case statements.
   } while (Tok.is(tok::kw_case));
 
-  assert(!TopLevelCase.isInvalid() && "Should have parsed at least one case!");
-
   // If we found a non-case statement, start by parsing it.
   StmtResult SubStmt;
 
   if (Tok.isNot(tok::r_brace)) {
     SubStmt = ParseStatement();
   } else {
     // Nicely diagnose the common error "switch (X) { case 4: }", which is
-    // not valid.
+    // not valid.  If ColonLoc doesn't point to a valid text location, there was
+    // another parsing error, so avoid producing extra diagnostics.
+    if (ColonLoc.isValid()) {
       SourceLocation AfterColonLoc = PP.getLocForEndOfToken(ColonLoc);
       Diag(AfterColonLoc, diag::err_label_end_of_compound_statement)
         << FixItHint::CreateInsertion(AfterColonLoc, " ;");
-    SubStmt = true;
+    }
+    SubStmt = StmtError();
   }
 
+  // Install the body into the most deeply-nested case.
+  if (DeepestParsedCaseStmt) {
     // Broken sub-stmt shouldn't prevent forming the case statement properly.
     if (SubStmt.isInvalid())
       SubStmt = Actions.ActOnNullStmt(SourceLocation());
-
-  // Install the body into the most deeply-nested case.
     Actions.ActOnCaseStmtBody(DeepestParsedCaseStmt, SubStmt.get());
+  }
 
   // Return the top level parsed statement tree.
   return TopLevelCase;
 }
 
 /// ParseDefaultStatement
 ///       labeled-statement:
 ///         'default' ':' statement
StmtResult Parser::ParseSwitchStatement(SourceLocation *TrailingElseLoc) {
 
   // Read the body statement.
   StmtResult Body(ParseStatement(TrailingElseLoc));
 
   // Pop the scopes.
   InnerScope.Exit();
   SwitchScope.Exit();
 
-  if (Body.isInvalid()) {
-    // FIXME: Remove the case statement list from the Switch statement.
-
-    // Put the synthesized null statement on the same line as the end of switch
-    // condition.
-    SourceLocation SynthesizedNullStmtLocation = Cond.get()->getLocEnd();
-    Body = Actions.ActOnNullStmt(SynthesizedNullStmtLocation);
-  }
-
   return Actions.ActOnFinishSwitchStmt(SwitchLoc, Switch.get(), Body.get());
 }
 
 /// ParseWhileStatement
 ///       while-statement: [C99 6.8.5.1]
 ///         'while' '(' expression ')' statement
 /// [C++]   'while' '(' condition ')' statement
 StmtResult Parser::ParseWhileStatement(SourceLocation *TrailingElseLoc) {

cfe/trunk/lib/Sema/SemaStmt.cpp

 
 StmtResult
 Sema::ActOnFinishSwitchStmt(SourceLocation SwitchLoc, Stmt *Switch,
                             Stmt *BodyStmt) {
   SwitchStmt *SS = cast<SwitchStmt>(Switch);
   assert(SS == getCurFunction()->SwitchStack.back() &&
          "switch stack missing push/pop!");
 
+  if (!BodyStmt) return StmtError();
   SS->setBody(BodyStmt, SwitchLoc);
   getCurFunction()->SwitchStack.pop_back();
 
   Expr *CondExpr = SS->getCond();
   if (!CondExpr) return StmtError();
 
   QualType CondType = CondExpr->getType();
 
if (!CaseListIsErroneous  && !HasConstantCond && ET) {
         break;
       }
 
       if (!hasCasesNotInSwitch)
         SS->setAllEnumCasesCovered();
     }
   }
 
+  if (BodyStmt)
     DiagnoseEmptyStmtBody(CondExpr->getLocEnd(), BodyStmt,
                           diag::warn_empty_switch_body);
 
   // FIXME: If the case list was broken is some way, we don't have a good system
   // to patch it up.  Instead, just return the whole substmt as broken.
   if (CaseListIsErroneous)
     return StmtError();
 
   return Owned(SS);
 }

cfe/trunk/test/Parser/switch-recovery.cpp

 }
 
 void missing_statement_default(int x) {
   switch (x) {
     case 0:
     default: // expected-error {{label at end of compound statement: expected statement}}
   }
 }
+
+void pr19022_1() {
+  switch (int x)  // expected-error {{variable declaration in condition must have an initializer}}
+  case v: ;  // expected-error {{use of undeclared identifier 'v'}}
+}
+
+void pr19022_1a(int x) {
+  switch(x) {
+  case 1  // expected-error{{expected ':' after 'case'}} \
+          // expected-error{{label at end of compound statement: expected statement}}
+  }
+}
+
+void pr19022_1b(int x) {
+  switch(x) {
+  case v  // expected-error{{use of undeclared identifier 'v'}}
+  }
+ }
+
+void pr19022_2() {
+  switch (int x)  // expected-error {{variable declaration in condition must have an initializer}}
+  case v1: case v2: ;  // expected-error {{use of undeclared identifier 'v1'}} \
+                       // expected-error {{use of undeclared identifier 'v2'}}
+}
+
+void pr19022_3(int x) {
+  switch (x)
+  case 1: case v2: ;  // expected-error {{use of undeclared identifier 'v2'}}
+}
+
+int pr19022_4(int x) {
+  switch(x) {
+  case 1  // expected-error{{expected ':' after 'case'}} expected-note{{previous case defined here}}
+  case 1 : return x;  // expected-error{{duplicate case value '1'}}
+  }
+}
+
+void pr19022_5(int x) {
+  switch(x) {
+  case 1: case
+  }  // expected-error{{expected expression}}
+}
+
+namespace pr19022 {
+int baz5() {}
+bool bar0() {
+  switch (int foo0)  //expected-error{{variable declaration in condition must have an initializer}}
+  case bar5: ;  // expected-error{{use of undeclared identifier 'bar5'}}
+}
+}

cfe/trunk/test/Sema/statements.c

 
 void *test10() { 
 bar:
   return &&bar;  // expected-warning {{returning address of label, which is local}}
 }
 
 // PR6034
 void test11(int bit) {
-  switch (bit) // expected-warning {{switch statement has empty body}} expected-note {{put the semicolon on a separate line to silence this warning}}
+  switch (bit)
   switch (env->fpscr)  // expected-error {{use of undeclared identifier 'env'}}
   {
   }
 }
 
 // rdar://3271964
 enum Numbers { kOne,  kTwo,  kThree,  kFour};
 int test12(enum Numbers num) {