[Parser] Fix a crash on invalid where a delayed TypoExpr was corrected twice

[erik.pilkington]

Summary

Previously, Clang crashed when parsing a BinaryOperator in C with a typo when a typo was already found. This is because the parser called Sema::ActOnBinOp, which corrects the found typo in C mode, then corrects the typo again from the parser. During the first correction pass, the TypoExprState corresponding to the typo was cleared from Sema when it was corrected. During a second pass, an assert fails in Sema::getTypoExprState because it cannot find the TypoExprState. The fix is to avoid correcting delayed typos in the parser in that case.

This patch looks like it fixes PR26700, PR27231, and PR27038.

On a more general note, the handling of delayed typo expressions is very messy right now, some of them are handled in semantic analysis, and some are handled in the parser, leading to easy to make responsibility bugs like this one. I think I might take a look at moving the correcting to one side or the other in a future patch.

[erik.pilkington]

Ping!!

[erik.pilkington]

Pong!!

[rsmith]

lib/Parse/ParseExpr.cpp

450–452 ↗

(On Diff #57980)

The inconsistent behavior of ActOnBinOp seems somewhere between an implementation detail and a bug; it doesn't seem reasonable for the parser to rely on that. I'm not particularly happy about making changes like this without some documentation of the overall design that shows whose responsibility it is to correct typos in which cases.

Before we introduced TypoExpr, the parser was permitted to simply discard Expr nodes that it didn't use (because it'd hit a parse error). Ideally, I'd like to return to that state of affairs, by removing the relevant CorrectDelayedTyposInExpr calls from the parser and having Sema automatically diagnose them when we get to the end of the relevant context, if we've not already done so.

Another reasonable-seeming option would be to add a Sema::ActOnDiscardedExpr(Expr*) that the parser can call (which calls CorrectDelayedTyposInExpr), and make it clear that the parser is responsible for passing each Expr that it receives from Sema to exactly one ActOn* function (unless otherwise specified) -- that way, at least the responsibilities will be clear, but it doesn't help us avoid bugs where TypoExprs are accidentally discarded.

[rsmith]

Let's go ahead with this for now and figure out the proper way to handle this as a follow-up change.

cfe/trunk/lib/Parse/ParseExpr.cpp

if (!LHS.isInvalid()) {
         if (!GreaterThanIsOperator && OpToken.is(tok::greatergreater))
           SuggestParentheses(OpToken.getLocation(),
                              diag::warn_cxx11_right_shift_in_template_arg,
                          SourceRange(Actions.getExprRange(LHS.get()).getBegin(),
                                      Actions.getExprRange(RHS.get()).getEnd()));
 
         LHS = Actions.ActOnBinOp(getCurScope(), OpToken.getLocation(),
                                  OpToken.getKind(), LHS.get(), RHS.get());
+
+        // In this case, ActOnBinOp performed the CorrectDelayedTyposInExpr check.
+        if (!getLangOpts().CPlusPlus)
+          continue;
       } else {
         LHS = Actions.ActOnConditionalOp(OpToken.getLocation(), ColonLoc,
                                          LHS.get(), TernaryMiddle.get(),
                                          RHS.get());
       }
     }
     // Ensure potential typos aren't left undiagnosed.
     if (LHS.isInvalid()) {

cfe/trunk/test/Sema/typo-correction.c

 
 extern long afunction(int); // expected-note {{'afunction' declared here}}
 void fn2() {
   f(THIS_IS_AN_ERROR, // expected-error {{use of undeclared identifier 'THIS_IS_AN_ERROR'}}
     afunction(afunction_));  // expected-error {{use of undeclared identifier 'afunction_'; did you mean 'afunction'?}}
 }
 
 int d = X ? d : L; // expected-error 2 {{use of undeclared identifier}}
+
+int fn_with_ids() { ID = ID == ID >= ID ; } // expected-error 4 {{use of undeclared identifier}}
+
+int fn_with_rs(int r) { r = TYPO + r * TYPO; } // expected-error 2 {{use of undeclared identifier}}
+
+void fn_with_unknown(int a, int b) {
+  fn_with_unknown(unknown, unknown | unknown); // expected-error 3 {{use of undeclared identifier}}
+}