[AST][RecoveryExpr] Part1: Support dependent binary operator in C for error recovery.

[hokein]

Summary

Introduce a new cc1 flag to enable build the dependent AST nodes for C.

see the whole view in: https://reviews.llvm.org/D85025

[hokein]

clang/lib/Sema/SemaExpr.cpp

14271

I think we probably need a new cc1 flag e.g. CDependenceType to control the typo-correction and whether we build dependent ast node for C.

[hokein]

Add a cc1 flag to enable building dependent nodes in C.

[hokein]

• rebase;
• add comments for CreateDependentBinOp, and make it private based on Richard's comment;

[sammccall]

clang/include/clang/Basic/LangOptions.def

153 ↗

(On Diff #284358)

Why is this not just the RecoveryAST flag? Is there some use of dependence outside the error-handling path?

If the language is C, recovery-AST only works if this flag is on too. So they might as well always have the same value.
If the language is C++, this flag is meaningless, right?

[sammccall]

clang/lib/Sema/SemaExpr.cpp

14271

I think we can do better on types here: for several binary operators the type is boolean in C regardless of operands. (except for vector types I think, but those might be rare enough to ignore in recovery)

14274

In C the result type is the unqualified type of the LHS

14276

I don't think we need to override the computation types, we can just leave them null?

[hokein]

address comments:

• reuse the RecoveryAST to control the early typo-correction;
• uses the fixed result type for some operators

[hokein]

clang/include/clang/Basic/LangOptions.def

153 ↗

(On Diff #284358)

This flag is mainly for turning off early typo-corrections for C.

Thinking more about this, you're right, looks like using the RecoveryAST is possible:

• these early typo-corrections applies non-C++ code, and have to be there until we fully implemented recovery-expr-on-c;
• the current default mode of RecoveryAST: on for C++, and off for non-C++;
• for recovery-expr-on-c development, we need to turn on the RecoveryAST and turn off these early typo-corrections;

so if (!S.getLangOpts().CPlusPlus && !S.getLangOpts().RecoveryAST) { works.

[sammccall]

clang/include/clang/Basic/LangOptions.def

153 ↗

(On Diff #284358)

If that's going to be a common condition, I'd suggest a helper function like

/// (explanation referencing early typo correction here)
isDependenceAllowed(const LangOpts &LO) { return CPlusPlus || RecoveryAST; }

and then guard early typo correction with if (!isDependenceAllowed())

clang/include/clang/Sema/Sema.h

5169 ↗

(On Diff #284358)

nit: unqualified
nit: remove "results"

[hokein]

add isDependenceAllowed function.

clang/include/clang/Basic/LangOptions.def

153 ↗

(On Diff #284358)

sounds a good idea.

clang/include/clang/Sema/Sema.h

5169 ↗

(On Diff #284358)

this was in the first snapshot, we don't need this change now.

[hokein]

I think this patch is ready for another round of review.

[sammccall]

clang/include/clang/AST/ASTContext.h

668

This is a bit hard to follow.
I think it's two separate sentences, and the second is something like
"If this condition is false, typo correction must be performed eagerly rather than delayed in many places, as it makes use of dependent types".

clang/lib/Sema/SemaExpr.cpp

14373

isAssignmentOp instead? including = itself

14391

comma could get the RHS :-)
May get some mileage for comma operators in macros, not sure though.

(Other aspect is value category: fortunately *in C* comma is an rvalue)

clang/test/AST/ast-dump-recovery.c

45–48

'int *' lvalue contains-errors '='

52

'int' lvalue contains-errors '+='

[hokein]

address comments.

clang/lib/Sema/SemaExpr.cpp

14373

Simple assignment = doesn't belong to CompoundAssignOperator, it should be BinaryOperator.

Added the handling logic in the switch-case below.

clang/test/AST/ast-dump-recovery.c

45–48

oh, this spots a bug in our code -- unlike C++, it should not be a lvalue.

From C [6.15.16] p3:

An assignment expression has the value of the left operand after the assignment, but is not an lvalue.

[sammccall]

clang/lib/Sema/SemaExpr.cpp

14381

now the only thing that differs between the cases is the type. You could consider putting the type in a variable, and factoring the Create after the switch.

14393

(why the line break here)

[hokein]

address comments.

clang/include/clang/AST/ASTContext.h

public:
   /// sets floating point QualTy according to specified bitwidth.
   /// Returns empty type if there is no appropriate target types.
   QualType getRealTypeForBitwidth(unsigned DestWidth, bool ExplicitIEEE) const;
 
   bool AtomicUsesUnsupportedLibcall(const AtomicExpr *E) const;
 
   const LangOptions& getLangOpts() const { return LangOpts; }
 
+  // If this condition is false, typo correction must be performed eagerly
+  // rather than delayed in many places, as it makes use of dependent types.
+  // the condition is false for clang's C-only codepath, as it doesn't support
+  // dependent types yet.
+  bool isDependenceAllowed() const {
+    return LangOpts.CPlusPlus || LangOpts.RecoveryAST;
+  }
+
   const SanitizerBlacklist &getSanitizerBlacklist() const {
     return *SanitizerBL;
   }
 
   const XRayFunctionFilter &getXRayFilter() const {
     return *XRayFilter;
   }
 

clang/lib/Sema/SemaExpr.cpp

 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/EvaluatedExprVisitor.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/ExprOpenMP.h"
+#include "clang/AST/OperationKinds.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/Builtins.h"
 #include "clang/Basic/PartialDiagnostic.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Lex/LiteralSupport.h"
 #include "clang/Lex/Preprocessor.h"
auto *BO = BinaryOperator::Create(Context, LHS.get(), RHS.get(), Opc,
                                     BinOpResTy, VK, OK, OpLoc, FPFeatures);
   return convertVector(BO, ResultTy->castAs<VectorType>()->getElementType(), S);
 }
 
 static std::pair<ExprResult, ExprResult>
 CorrectDelayedTyposInBinOp(Sema &S, BinaryOperatorKind Opc, Expr *LHSExpr,
                            Expr *RHSExpr) {
   ExprResult LHS = LHSExpr, RHS = RHSExpr;
-  if (!S.getLangOpts().CPlusPlus) {
+  if (!S.Context.isDependenceAllowed()) {
     // C cannot handle TypoExpr nodes on either side of a binop because it
     // doesn't handle dependent types properly, so make sure any TypoExprs have
     // been dealt with before checking the operands.
     LHS = S.CorrectDelayedTyposInExpr(LHS);
     RHS = S.CorrectDelayedTyposInExpr(
         RHS, /*InitDecl=*/nullptr, /*RecoverUncorrectedTypos=*/false,
         [Opc, LHS](Expr *E) {
           if (Opc != BO_Assign)
static ExprResult BuildOverloadedBinOp(Sema &S, Scope *Sc, SourceLocation OpLoc,
   return S.CreateOverloadedBinOp(OpLoc, Opc, Functions, LHS, RHS);
 }
 
 ExprResult Sema::BuildBinOp(Scope *S, SourceLocation OpLoc,
                             BinaryOperatorKind Opc,
                             Expr *LHSExpr, Expr *RHSExpr) {
   ExprResult LHS, RHS;
   std::tie(LHS, RHS) = CorrectDelayedTyposInBinOp(*this, Opc, LHSExpr, RHSExpr);
   if (!LHS.isUsable() || !RHS.isUsable())
     return ExprError();
   LHSExpr = LHS.get();
   RHSExpr = RHS.get();
 
   // We want to end up calling one of checkPseudoObjectAssignment
   // (if the LHS is a pseudo-object), BuildOverloadedBinOp (if
   // both expressions are overloadable or either is type-dependent),
   // or CreateBuiltinBinOp (in any other case).  We also want to get
   // any placeholder types out of the way.
 
   // Handle pseudo-objects in the LHS.
   if (const BuiltinType *pty = LHSExpr->getType()->getAsPlaceholderType()) {
     // Assignments with a pseudo-object l-value need special analysis.
if (getLangOpts().CPlusPlus) {
 
     // Otherwise, build an overloaded op if either expression has an
     // overloadable type.
     if (LHSExpr->getType()->isOverloadableType() ||
         RHSExpr->getType()->isOverloadableType())
       return BuildOverloadedBinOp(*this, S, OpLoc, Opc, LHSExpr, RHSExpr);
   }
 
+  if (getLangOpts().RecoveryAST &&
+      (LHSExpr->isTypeDependent() || RHSExpr->isTypeDependent())) {
+    assert(!getLangOpts().CPlusPlus);
+    assert((LHSExpr->containsErrors() || RHSExpr->containsErrors()) &&
+           "Should only occur in error-recovery path.");
+    if (BinaryOperator::isCompoundAssignmentOp(Opc))
+      // C [6.15.16] p3:
+      // An assignment expression has the value of the left operand after the
+      // assignment, but is not an lvalue.
+      return CompoundAssignOperator::Create(
+          Context, LHSExpr, RHSExpr, Opc,
+          LHSExpr->getType().getUnqualifiedType(), VK_RValue, OK_Ordinary,
+          OpLoc, CurFPFeatureOverrides());
+    QualType ResultType;
+    switch (Opc) {
+    case BO_Assign:
+      ResultType = LHSExpr->getType().getUnqualifiedType();
+      break;
+    case BO_LT:
+    case BO_GT:
+    case BO_LE:
+    case BO_GE:
+    case BO_EQ:
+    case BO_NE:
+    case BO_LAnd:
+    case BO_LOr:
+      // These operators have a fixed result type regardless of operands.
+      ResultType = Context.IntTy;
+      break;
+    case BO_Comma:
+      ResultType = RHSExpr->getType();
+      break;
+    default:
+      ResultType = Context.DependentTy;
+      break;
+    }
+    return BinaryOperator::Create(Context, LHSExpr, RHSExpr, Opc, ResultType,
+                                  VK_RValue, OK_Ordinary, OpLoc,
+                                  CurFPFeatureOverrides());
+  }
+
   // Build a built-in binary operation.
   return CreateBuiltinBinOp(OpLoc, Opc, LHSExpr, RHSExpr);
 }
 
 static bool isOverflowingIntegerType(ASTContext &Ctx, QualType T) {
   if (T.isNull() || T->isDependentType())
     return false;
 

clang/test/AST/ast-dump-recovery.c

 void test1() {
   // CHECK:     `-RecoveryExpr {{.*}} contains-errors
   // CHECK-NEXT:  `-DeclRefExpr {{.*}} 'a' 'const int'
   static int foo = a++; // verify no crash on local static var decl.
 }
 
 void test2() {
   int* ptr;
-  // FIXME: the top-level expr should be a binary operator.
-  // CHECK:      ImplicitCastExpr {{.*}} contains-errors <LValueToRValue>
-  // CHECK-NEXT: `-RecoveryExpr {{.*}} contains-errors lvalue
-  // CHECK-NEXT:   |-DeclRefExpr {{.*}} 'ptr' 'int *'
-  // CHECK-NEXT:   `-RecoveryExpr {{.*}}
-  // CHECK-NEXT:     `-DeclRefExpr {{.*}} 'some_func'
+  // CHECK:     BinaryOperator {{.*}} 'int *' contains-errors '='
+  // CHECK-NEXT: |-DeclRefExpr {{.*}} 'ptr' 'int *'
+  // CHECK-NEXT: `-RecoveryExpr {{.*}}
+  // CHECK-NEXT:   `-DeclRefExpr {{.*}} 'some_func'
   ptr = some_func(); // should not crash
+
+  int compoundOp;
+  // CHECK:     CompoundAssignOperator {{.*}} 'int' contains-errors '+='
+  // CHECK-NEXT: |-DeclRefExpr {{.*}} 'compoundOp'
+  // CHECK-NEXT: `-RecoveryExpr {{.*}} contains-errors
+  // CHECK-NEXT:   `-DeclRefExpr {{.*}} 'some_func'
+  compoundOp += some_func();
+
+  // CHECK:     BinaryOperator {{.*}} 'int' contains-errors '||'
+  // CHECK-NEXT: |-RecoveryExpr {{.*}}
+  // CHECK-NEXT: | `-DeclRefExpr {{.*}} 'some_func'
+  // CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 1
+  some_func() || 1;
+
+  // CHECK:     BinaryOperator {{.*}} '<dependent type>' contains-errors ','
+  // CHECK-NEXT: |-IntegerLiteral {{.*}} 'int' 1
+  // CHECK-NEXT: `-RecoveryExpr {{.*}}
+  // CHECK-NEXT:   `-DeclRefExpr {{.*}} 'some_func'
+  1, some_func();
+  // CHECK:     BinaryOperator {{.*}} 'int' contains-errors ','
+  // CHECK-NEXT: |-RecoveryExpr {{.*}} '<dependent type>'
+  // CHECK-NEXT: | `-DeclRefExpr {{.*}} 'some_func'
+  // CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 1
+  some_func(), 1;
 }

clang/test/Sema/error-dependence.c

+// RUN: %clang_cc1 -fsyntax-only -verify -frecovery-ast -fno-recovery-ast-type %s
+
+int call(int); // expected-note {{'call' declared here}}
+
+void test1(int s) {
+  // verify "assigning to 'int' from incompatible type '<dependent type>'" is
+  // not emitted.
+  s = call(); // expected-error {{too few arguments to function call}}
+}