[PATCH] D26350: Keep invalid Switch in the AST

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: lib/Parse/ParseStmt.cpp:1297
 
-  if (Switch.isInvalid()) {
-    // Skip the switch body.
-    // FIXME: This is not optimal recovery, but parsing the body is more
-    // dangerous due to the presence of case and default statements, which
-    // will have no place to connect back with the switch.
-    if (Tok.is(tok::l_brace)) {
-      ConsumeBrace();
-      SkipUntil(tok::r_brace);
-    } else
-      SkipUntil(tok::semi);
-    return Switch;
-  }
+  assert(!Switch.isInvalid() && "ActOnStartOfSwitchStmt always succeed");
 
----------------
ogoffart wrote:
> aaron.ballman wrote:
> > succeed -> succeeds
> > 
> > Are the concerns pointed out in the FIXME addressed by code not in this patch?
> The FIXME is pointing out problems occuring if the parser found 'default' or 'case' statement, but cannot connect it to the corresponding 'switch' statement (because that switch statement did not exist as it was removed from the AST). 
> 
> Now that we always keep the 'switch' statement, this is no longer a problem.
I'm uncomfortable about this; this change couples Parser to the implementation details of Sema. How about this: remove this assert and change `ActOnFinishSwitchStmt` to take a `StmtResult` instead (which might be invalid). Then you can tell from within `ActOnFinishSwitchStmt` whether to check the case statements against the condition based on whether the switch is in fact invalid.

(Alternatively: change `ActOnStartSwitchStmt` to return `void` and make `ActOnFinishSwitchStmt` pick up the switch statement from the `SwitchStack`.)


================
Comment at: lib/Sema/SemaStmt.cpp:672
+        MakeFullExpr(new (Context) OpaqueValueExpr(SourceLocation(),
+                                                   Context.IntTy, VK_RValue),
+                     SwitchLoc),
----------------
Won't this result in warnings or errors later on if we have `case` labels with expressions of other types? (Eg, narrowing warnings/errors)

Please instead (somehow) track that the switch condition is invalid and skip those checks -- perhaps either by returning an invalid-but-not-null statement here and passing that back into `ActOnFinishSwitchStmt`, or by tracking an "invalid" flag on the `SwitchStack` entry.


================
Comment at: lib/Sema/SemaStmt.cpp:1173-1177
-  // 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();
-
----------------
Hmm. Removing this will result in us producing invalid ASTs in some cases (with duplicate `case` or `default` labels). That's a condition that it would be reasonable for AST consumers to assert on currently, so this is concerning.

That said... it's inevitable that this work to keep more invalid constructs in the AST will result in such changes. Perhaps what we need is just a marker to say "beyond this point the AST does not necessarily correspond to any valid source code" for `Stmt` nodes, analogous to the `Invalid` marker on declarations. (Maybe a wrapper `InvalidStmt` node, so that tree traversals can easily avoid walking through it.)

Let's try this change out as-is. It may be that this concern is baseless.


https://reviews.llvm.org/D26350