[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

14268

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

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

14255

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)

14258

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

14260

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

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

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

nit: unqualified
nit: remove "results"

[hokein]

add isDependenceAllowed function.

clang/include/clang/Basic/LangOptions.def

153

sounds a good idea.

clang/include/clang/Sema/Sema.h

5169

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 ↗

(On Diff #294903)

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

14370

isAssignmentOp instead? including = itself

14388

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

14370

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

14378

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.

14390

(why the line break here)

[hokein]

address comments.

clang/include/clang/Basic/LangOptions.def

 LANGOPT(Coroutines        , 1, 0, "C++20 coroutines")
 LANGOPT(DllExportInlines  , 1, 1, "dllexported classes dllexport inline methods")
 LANGOPT(RelaxedTemplateTemplateArgs, 1, 0, "C++17 relaxed matching of template template arguments")
 
 LANGOPT(DoubleSquareBracketAttributes, 1, 0, "'[[]]' attributes extension for all language standard modes")
 
 COMPATIBLE_LANGOPT(RecoveryAST, 1, 0, "Preserve expressions in AST when encountering errors")
 COMPATIBLE_LANGOPT(RecoveryASTType, 1, 0, "Preserve the type in recovery expressions")
+COMPATIBLE_LANGOPT(CDependence, 1, 0, "Build dependent AST nodes in C for better error recovery")
 
 BENIGN_LANGOPT(ThreadsafeStatics , 1, 1, "thread-safe static initializers")
 LANGOPT(POSIXThreads      , 1, 0, "POSIX thread support")
 LANGOPT(Blocks            , 1, 0, "blocks extension to C")
 BENIGN_LANGOPT(EmitAllDecls      , 1, 0, "emitting all declarations")
 LANGOPT(MathErrno         , 1, 1, "errno in math functions")
 BENIGN_LANGOPT(HeinousExtensions , 1, 0, "extensions that we really don't like and may be ripped out at any time")
 LANGOPT(Modules           , 1, 0, "modules semantics")

clang/include/clang/Driver/Options.td

def frecovery_ast : Flag<["-"], "frecovery-ast">,
   HelpText<"Preserve expressions in AST rather than dropping them when "
            "encountering semantic errors">;
 def fno_recovery_ast : Flag<["-"], "fno-recovery-ast">;
 def frecovery_ast_type : Flag<["-"], "frecovery-ast-type">,
   HelpText<"Preserve the type for recovery expressions when possible "
           "(experimental)">;
 def fno_recovery_ast_type : Flag<["-"], "fno-recovery-ast-type">;
 
+def fc_dependence : Flag<["-"], "fc-dependence">,
+  HelpText<"Build dependent AST nodes in C for error recovery">;
+def fno_c_dependence : Flag<["-"], "fno-c-dependence">;
+
 let Group = Action_Group in {
 
 def Eonly : Flag<["-"], "Eonly">,
   HelpText<"Just run preprocessor, no output (for timings)">;
 def dump_raw_tokens : Flag<["-"], "dump-raw-tokens">,
   HelpText<"Lex file in raw mode and dump raw tokens">;
 def analyze : Flag<["-"], "analyze">,
   HelpText<"Run static analysis engine">;

clang/include/clang/Sema/Sema.h

public:
 
   ExprResult ActOnDesignatedInitializer(Designation &Desig,
                                         SourceLocation EqualOrColonLoc,
                                         bool GNUSyntax,
                                         ExprResult Init);
 
 private:
   static BinaryOperatorKind ConvertTokenKindToBinaryOpcode(tok::TokenKind Kind);
+  /// Build a dependent binary operator.
+  ///
+  /// In C++, we need to perform unqualified lookups when creating a dependent
+  /// binary operator, thus this should be only used when the unqalified lookup
+  /// found nothing.
+  /// results.
+  ExprResult CreateDependentBinOp(SourceLocation OpLoc, BinaryOperatorKind Opc,
+                                  Expr *LHS, Expr *RHS);
 
 public:
   ExprResult ActOnBinOp(Scope *S, SourceLocation TokLoc,
                         tok::TokenKind Kind, Expr *LHSExpr, Expr *RHSExpr);
   ExprResult BuildBinOp(Scope *S, SourceLocation OpLoc,
                         BinaryOperatorKind Opc, Expr *LHSExpr, Expr *RHSExpr);
   ExprResult CreateBuiltinBinOp(SourceLocation OpLoc, BinaryOperatorKind Opc,
                                 Expr *LHSExpr, Expr *RHSExpr);

clang/lib/Frontend/CompilerInvocation.cpp

if (Opts.NewAlignOverride && !llvm::isPowerOf2_32(Opts.NewAlignOverride)) {
     Diags.Report(diag::err_fe_invalid_alignment) << A->getAsString(Args)
                                                  << A->getValue();
     Opts.NewAlignOverride = 0;
   }
   Opts.ConceptSatisfactionCaching =
       !Args.hasArg(OPT_fno_concept_satisfaction_caching);
   if (Args.hasArg(OPT_fconcepts_ts))
     Diags.Report(diag::warn_fe_concepts_ts_flag);
+  Opts.CDependence =
+      Args.hasFlag(OPT_fc_dependence, OPT_fno_c_dependence, false);
   // Recovery AST still heavily relies on dependent-type machinery.
   Opts.RecoveryAST =
       Args.hasFlag(OPT_frecovery_ast, OPT_fno_recovery_ast, Opts.CPlusPlus);
   Opts.RecoveryASTType =
       Args.hasFlag(OPT_frecovery_ast_type, OPT_fno_recovery_ast_type, false);
   Opts.HeinousExtensions = Args.hasArg(OPT_fheinous_gnu_extensions);
   Opts.AccessControl = !Args.hasArg(OPT_fno_access_control);
   Opts.ElideConstructors = !Args.hasArg(OPT_fno_elide_constructors);

clang/lib/Sema/SemaExpr.cpp

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.getLangOpts().CPlusPlus && !S.getLangOpts().CDependence) {
     // 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,
   UnresolvedSet<16> Functions;
   S.LookupBinOp(Sc, OpLoc, Opc, Functions);
 
   // Build the (potentially-overloaded, potentially-dependent)
   // binary operation.
   return S.CreateOverloadedBinOp(OpLoc, Opc, Functions, LHS, RHS);
 }
 
+ExprResult Sema::CreateDependentBinOp(SourceLocation OpLoc,
+                                      BinaryOperatorKind Opc, Expr *LHS,
+                                      Expr *RHS) {
+  assert(LHS->isTypeDependent() || RHS->isTypeDependent());
+  if (Opc <= BO_Assign || Opc > BO_OrAssign)
+    return BinaryOperator::Create(Context, LHS, RHS, Opc, Context.DependentTy,
+                                  VK_RValue, OK_Ordinary, OpLoc,
+                                  CurFPFeatureOverrides());
+  return CompoundAssignOperator::Create(
+      Context, LHS, RHS, Opc, Context.DependentTy, VK_LValue, OK_Ordinary,
+      OpLoc, CurFPFeatureOverrides(), Context.DependentTy, Context.DependentTy);
+}
+
 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
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().CDependence &&
+      (LHSExpr->isTypeDependent() || RHSExpr->isTypeDependent())) {
+    assert(!getLangOpts().CPlusPlus);
+    assert((LHSExpr->containsErrors() || RHSExpr->containsErrors()) &&
+           "Should only occur in error-recovery path.");
+    return CreateDependentBinOp(OpLoc, Opc, LHSExpr, RHSExpr);
+  }
+
   // 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;
 
   if (!T->isPromotableIntegerType())
     return true;
 
   return Ctx.getIntWidth(T) >= Ctx.getIntWidth(Ctx.IntTy);
 }
 
 ExprResult Sema::CreateBuiltinUnaryOp(SourceLocation OpLoc,
                                       UnaryOperatorKind Opc,
                                       Expr *InputExpr) {
   ExprResult Input = InputExpr;
   ExprValueKind VK = VK_RValue;
   ExprObjectKind OK = OK_Ordinary;
   QualType resultType;
   bool CanOverflow = false;
 
   bool ConvertHalfVec = false;
   if (getLangOpts().OpenCL) {
     QualType Ty = InputExpr->getType();

clang/lib/Sema/SemaOverload.cpp

ExprResult Sema::CreateOverloadedBinOp(SourceLocation OpLoc,
   if (!getLangOpts().CPlusPlus20)
     AllowRewrittenCandidates = false;
 
   OverloadedOperatorKind Op = BinaryOperator::getOverloadedOperator(Opc);
 
   // If either side is type-dependent, create an appropriate dependent
   // expression.
   if (Args[0]->isTypeDependent() || Args[1]->isTypeDependent()) {
-    if (Fns.empty()) {
-      // If there are no functions to store, just build a dependent
-      // BinaryOperator or CompoundAssignment.
-      if (Opc <= BO_Assign || Opc > BO_OrAssign)
-        return BinaryOperator::Create(
-            Context, Args[0], Args[1], Opc, Context.DependentTy, VK_RValue,
-            OK_Ordinary, OpLoc, CurFPFeatureOverrides());
-      return CompoundAssignOperator::Create(
-          Context, Args[0], Args[1], Opc, Context.DependentTy, VK_LValue,
-          OK_Ordinary, OpLoc, CurFPFeatureOverrides(), Context.DependentTy,
-          Context.DependentTy);
-    }
+    if (Fns.empty())
+      return CreateDependentBinOp(OpLoc, Opc, Args[0], Args[1]);
 
     // FIXME: save results of ADL from here?
     CXXRecordDecl *NamingClass = nullptr; // lookup ignores member operators
     // TODO: provide better source location info in DNLoc component.
     DeclarationName OpName = Context.DeclarationNames.getCXXOperatorName(Op);
     DeclarationNameInfo OpNameInfo(OpName, OpLoc);
     ExprResult Fn = CreateUnresolvedLookupExpr(
         NamingClass, NestedNameSpecifierLoc(), OpNameInfo, Fns, PerformADL);

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

-// RUN: not %clang_cc1 -triple x86_64-unknown-unknown -frecovery-ast -fno-recovery-ast-type -ast-dump %s | FileCheck -strict-whitespace %s
+// RUN: not %clang_cc1 -triple x86_64-unknown-unknown -frecovery-ast -fno-recovery-ast-type -fc-dependence -ast-dump %s | FileCheck -strict-whitespace %s
 
 int some_func(int);
 
 // CHECK:     VarDecl {{.*}} unmatch_arg_call 'int' cinit
 // CHECK-NEXT: `-RecoveryExpr {{.*}} contains-errors
 // CHECK-NEXT:   `-DeclRefExpr {{.*}} 'some_func'
 int unmatch_arg_call = some_func();
 

 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 {{.*}} 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 {{.*}} '+='
+  // CHECK-NEXT: |-DeclRefExpr {{.*}} 'compoundOp'
+  // CHECK-NEXT: `-RecoveryExpr {{.*}} contains-errors
+  // CHECK-NEXT:   `-DeclRefExpr {{.*}} 'some_func'
+  compoundOp += some_func();
 }

clang/test/Sema/error-dependence.c

+// RUN: %clang_cc1 -fsyntax-only -verify -frecovery-ast -fno-recovery-ast-type -fc-dependence %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}}
+}