[AST][RecoveryAST] Preserve type for member call expr if argments are not matched.

[sammccall]

The thrust of this change is great, I'm just having trouble being sure there are no bad side-effects.

---

I almost left a comment here saying we might want to recover here... before noticing that we recover all the way up at ParsePostfixExpressionSuffix().

It may be worth a comment on the function implementation that RecoveryExpr fallback is done at a higher level.
But on this note, I think we have to consider the effect on *other* callsites.
For example Sema::tryExprAsCall checks if the result is non-null in order to produce "function must be callled, did you mean to call with no arguments"... but now I think we're returning non-null even if args are required. The fix there is to check containsErrors too.

There are a bunch of "internal" uses to Sema::BuildCallExpr (which calls this function), like coroutines, decomposition, pseudo-object etc that might need to be checked.

Finally BuildRecoveryCallExpr in SemaOverload itself calls BuildCallExpr, and maybe we can end up with a RecoveryExpr wrapping a RecoveryExpr.

---

I'm not sure what the most principled thing to do here is. Recovering (only) at a higher level seems sensible, but then we lack a way to propagate the type. Checking for containsErrors() where needed seems fragile. Adding a new state to ExprResult (null/error/Expr/QualType) is a pretty big change that may not be useful in general.

I'm not opposed to this patch if we carefully audit all callers but this doesn't feel great. Maybe an ugly "allowRecovery" flag to BuildCallExpr is the right compromise, at least it forces us to be explicit.

[hokein]

address comments

• add a allowRecovery flag to avoid bad side-effect on other callsides;
• recover more cases;

[hokein]

In D92298#2439325, @sammccall wrote:

The thrust of this change is great, I'm just having trouble being sure there are no bad side-effects.

---

I almost left a comment here saying we might want to recover here... before noticing that we recover all the way up at ParsePostfixExpressionSuffix().

It may be worth a comment on the function implementation that RecoveryExpr fallback is done at a higher level.
But on this note, I think we have to consider the effect on *other* callsites.
For example Sema::tryExprAsCall checks if the result is non-null in order to produce "function must be callled, did you mean to call with no arguments"... but now I think we're returning non-null even if args are required. The fix there is to check containsErrors too.

There are a bunch of "internal" uses to Sema::BuildCallExpr (which calls this function), like coroutines, decomposition, pseudo-object etc that might need to be checked.

Finally BuildRecoveryCallExpr in SemaOverload itself calls BuildCallExpr, and maybe we can end up with a RecoveryExpr wrapping a RecoveryExpr.

---

I'm not sure what the most principled thing to do here is. Recovering (only) at a higher level seems sensible, but then we lack a way to propagate the type. Checking for containsErrors() where needed seems fragile. Adding a new state to ExprResult (null/error/Expr/QualType) is a pretty big change that may not be useful in general.

I'm not opposed to this patch if we carefully audit all callers but this doesn't feel great. Maybe an ugly "allowRecovery" flag to BuildCallExpr is the right compromise, at least it forces us to be explicit.

This is a good point, thanks for raising it.

Recovering (only) at a higher level seems sensible, but then we lack a way to propagate the type.

This looks reasonable. Currently we already perform the recovery when a CallExpr node is failed to build (in ParseExpr.cpp), but the type is always dependent. We could add some heuristics there to propagate the type as the Fn is known (but we might end up with duplicated logics as those in SemaOverload.cpp)

so yeah, adding a flag to BuildCallExpr and other related methods seems like a right compromise, it doesn't hurt the existing diagnostics while allowing us to preserve the type, and https://reviews.llvm.org/D80109 also needs that.

[sammccall]

Some case errors on the allowRecovery variable name throughout.

clang/include/clang/Sema/Sema.h

3730

nit: AllowRecovery (case)

5242

(and here)

clang/lib/Sema/SemaExpr.cpp

6313

/*IsExecConfig=*/false, /*AllowRecovery=*/true

clang/lib/Sema/SemaOverload.cpp

14218

This would be a good place to add a comment
// We only try to build a recovery expr at this level if we can preserve the return type, otherwise we return ExprError() and let the caller recover.

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

45

Is this a leftover?

[sammccall]

(It seems like there are probably other callsites to these functions that could be updated to pass AllowRecovery=true. If so, those can easily be separate patches of course)

[hokein]

address comments.

clang/include/clang/Sema/Sema.h

ExprResult BuildSynthesizedThreeWayComparison(SourceLocation OpLoc,
                                                 const UnresolvedSetImpl &Fns,
                                                 Expr *LHS, Expr *RHS,
                                                 FunctionDecl *DefaultedFn);
 
   ExprResult CreateOverloadedArraySubscriptExpr(SourceLocation LLoc,
                                                 SourceLocation RLoc,
                                                 Expr *Base,Expr *Idx);
 
-  ExprResult
-  BuildCallToMemberFunction(Scope *S, Expr *MemExpr,
+  ExprResult BuildCallToMemberFunction(Scope *S, Expr *MemExpr,
                                        SourceLocation LParenLoc,
                                        MultiExprArg Args,
-                            SourceLocation RParenLoc);
+                                       SourceLocation RParenLoc,
+                                       bool AllowRecovery = false);
   ExprResult
   BuildCallToObjectOfClassType(Scope *S, Expr *Object, SourceLocation LParenLoc,
                                MultiExprArg Args,
                                SourceLocation RParenLoc);
 
   ExprResult BuildOverloadedArrowExpr(Scope *S, Expr *Base,
                                       SourceLocation OpLoc,
                                       bool *NoArrowOperatorFound = nullptr);
public:
   /// This provides the location of the left/right parens and a list of comma
   /// locations.
   ExprResult ActOnCallExpr(Scope *S, Expr *Fn, SourceLocation LParenLoc,
                            MultiExprArg ArgExprs, SourceLocation RParenLoc,
                            Expr *ExecConfig = nullptr);
   ExprResult BuildCallExpr(Scope *S, Expr *Fn, SourceLocation LParenLoc,
                            MultiExprArg ArgExprs, SourceLocation RParenLoc,
                            Expr *ExecConfig = nullptr,
-                           bool IsExecConfig = false);
+                           bool IsExecConfig = false,
+                           bool AllowRecovery = false);
   enum class AtomicArgumentOrder { API, AST };
   ExprResult
   BuildAtomicExpr(SourceRange CallRange, SourceRange ExprRange,
                   SourceLocation RParenLoc, MultiExprArg Args,
                   AtomicExpr::AtomicOp Op,
                   AtomicArgumentOrder ArgOrder = AtomicArgumentOrder::API);
   ExprResult
   BuildResolvedCallExpr(Expr *Fn, NamedDecl *NDecl, SourceLocation LParenLoc,

clang/lib/Sema/SemaExpr.cpp

if (CurLSI->ImpCaptureStyle != CurLSI->ImpCap_None)
       CurLSI->addPotentialThisCapture(CallLoc);
   }
 }
 
 ExprResult Sema::ActOnCallExpr(Scope *Scope, Expr *Fn, SourceLocation LParenLoc,
                                MultiExprArg ArgExprs, SourceLocation RParenLoc,
                                Expr *ExecConfig) {
   ExprResult Call =
-      BuildCallExpr(Scope, Fn, LParenLoc, ArgExprs, RParenLoc, ExecConfig);
+      BuildCallExpr(Scope, Fn, LParenLoc, ArgExprs, RParenLoc, ExecConfig,
+                    /*IsExecConfig=*/false, /*AllowRecovery=*/true);
   if (Call.isInvalid())
     return Call;
 
   // Diagnose uses of the C++20 "ADL-only template-id call" feature in earlier
   // language modes.
   if (auto *ULE = dyn_cast<UnresolvedLookupExpr>(Fn)) {
     if (ULE->hasExplicitTemplateArgs() &&
         ULE->decls_begin() == ULE->decls_end()) {
ExprResult Sema::ActOnCallExpr(Scope *Scope, Expr *Fn, SourceLocation LParenLoc,
   return Call;
 }
 
 /// BuildCallExpr - Handle a call to Fn with the specified array of arguments.
 /// This provides the location of the left/right parens and a list of comma
 /// locations.
 ExprResult Sema::BuildCallExpr(Scope *Scope, Expr *Fn, SourceLocation LParenLoc,
                                MultiExprArg ArgExprs, SourceLocation RParenLoc,
-                               Expr *ExecConfig, bool IsExecConfig) {
+                               Expr *ExecConfig, bool IsExecConfig,
+                               bool AllowRecovery) {
   // Since this might be a postfix expression, get rid of ParenListExprs.
   ExprResult Result = MaybeConvertParenListExprToParenExpr(Scope, Fn);
   if (Result.isInvalid()) return ExprError();
   Fn = Result.get();
 
   if (checkArgsForPlaceholders(*this, ArgExprs))
     return ExprError();
 
if (getLangOpts().CPlusPlus) {
     if (Fn->getType() == Context.UnknownAnyTy) {
       ExprResult result = rebuildUnknownAnyFunction(*this, Fn);
       if (result.isInvalid()) return ExprError();
       Fn = result.get();
     }
 
     if (Fn->getType() == Context.BoundMemberTy) {
       return BuildCallToMemberFunction(Scope, Fn, LParenLoc, ArgExprs,
-                                       RParenLoc);
+                                       RParenLoc, AllowRecovery);
     }
   }
 
   // Check for overloaded calls.  This can happen even in C due to extensions.
   if (Fn->getType() == Context.OverloadTy) {
     OverloadExpr::FindResult find = OverloadExpr::find(Fn);
 
     // We aren't supposed to apply this logic if there's an '&' involved.
     if (!find.HasFormOfMemberPointer) {
       if (Expr::hasAnyTypeDependentArguments(ArgExprs))
         return CallExpr::Create(Context, Fn, ArgExprs, Context.DependentTy,
                                 VK_RValue, RParenLoc, CurFPFeatureOverrides());
       OverloadExpr *ovl = find.Expression;
       if (UnresolvedLookupExpr *ULE = dyn_cast<UnresolvedLookupExpr>(ovl))
         return BuildOverloadedCallExpr(
             Scope, Fn, ULE, LParenLoc, ArgExprs, RParenLoc, ExecConfig,
             /*AllowTypoCorrection=*/true, find.IsAddressOfOperand);
       return BuildCallToMemberFunction(Scope, Fn, LParenLoc, ArgExprs,
-                                       RParenLoc);
+                                       RParenLoc, AllowRecovery);
     }
   }
 
   // If we're directly calling a function, get the appropriate declaration.
   if (Fn->getType() == Context.UnknownAnyTy) {
     ExprResult result = rebuildUnknownAnyFunction(*this, Fn);
     if (result.isInvalid()) return ExprError();
     Fn = result.get();

clang/lib/Sema/SemaOverload.cpp

 
 /// BuildCallToMemberFunction - Build a call to a member
 /// function. MemExpr is the expression that refers to the member
 /// function (and includes the object parameter), Args/NumArgs are the
 /// arguments to the function call (not including the object
 /// parameter). The caller needs to validate that the member
 /// expression refers to a non-static member function or an overloaded
 /// member function.
-ExprResult
-Sema::BuildCallToMemberFunction(Scope *S, Expr *MemExprE,
+ExprResult Sema::BuildCallToMemberFunction(Scope *S, Expr *MemExprE,
                                            SourceLocation LParenLoc,
                                            MultiExprArg Args,
-                                SourceLocation RParenLoc) {
+                                           SourceLocation RParenLoc,
+                                           bool AllowRecovery) {
   assert(MemExprE->getType() == Context.BoundMemberTy ||
          MemExprE->getType() == Context.OverloadTy);
 
   // Dig out the member expression. This holds both the object
   // argument and the member function we're referring to.
   Expr *NakedMemExpr = MemExprE->IgnoreParens();
 
   // Determine whether this is a call to a pointer-to-member function.
if (ConvertArgumentsForCall(call, op, nullptr, proto, Args, RParenLoc))
       return ExprError();
 
     if (CheckOtherCall(call, proto))
       return ExprError();
 
     return MaybeBindToTemporary(call);
   }
 
+  // We only try to build a recovery expr at this level if we can preserve
+  // the return type, otherwise we return ExprError() and let the caller
+  // recover.
+  auto BuildRecoveryExpr = [&](QualType Type) {
+    if (!AllowRecovery)
+      return ExprError();
+    std::vector<Expr *> SubExprs = {MemExprE};
+    llvm::for_each(Args, [&SubExprs](Expr *E) { SubExprs.push_back(E); });
+    return CreateRecoveryExpr(MemExprE->getBeginLoc(), RParenLoc, SubExprs,
+                              Type);
+  };
   if (isa<CXXPseudoDestructorExpr>(NakedMemExpr))
     return CallExpr::Create(Context, MemExprE, Args, Context.VoidTy, VK_RValue,
                             RParenLoc, CurFPFeatureOverrides());
 
   UnbridgedCastsSet UnbridgedCasts;
   if (checkArgPlaceholdersForOverload(*this, Args, UnbridgedCasts))
     return ExprError();
 
ExprResult Sema::BuildCallToMemberFunction(Scope *S, Expr *MemExprE,
   const auto *Proto = Method->getType()->castAs<FunctionProtoType>();
   CXXMemberCallExpr *TheCall = CXXMemberCallExpr::Create(
       Context, MemExprE, Args, ResultType, VK, RParenLoc,
       CurFPFeatureOverrides(), Proto->getNumParams());
 
   // Check for a valid return type.
   if (CheckCallReturnType(Method->getReturnType(), MemExpr->getMemberLoc(),
                           TheCall, Method))
-    return ExprError();
+    return BuildRecoveryExpr(ResultType);
 
   // Convert the object argument (for a non-static member function call).
   // We only need to do this if there was actually an overload; otherwise
   // it was done at lookup.
   if (!Method->isStatic()) {
     ExprResult ObjectArg =
       PerformObjectArgumentInitialization(MemExpr->getBase(), Qualifier,
                                           FoundDecl, Method);
     if (ObjectArg.isInvalid())
       return ExprError();
     MemExpr->setBase(ObjectArg.get());
   }
 
   // Convert the rest of the arguments
   if (ConvertArgumentsForCall(TheCall, MemExpr, Method, Proto, Args,
                               RParenLoc))
-    return ExprError();
+    return BuildRecoveryExpr(ResultType);
 
   DiagnoseSentinelCalls(Method, LParenLoc, Args);
 
   if (CheckFunctionCall(Method, TheCall, Proto))
     return ExprError();
 
   // In the case the method to call was not selected by the overloading
   // resolution process, we still need to handle the enable_if attribute. Do

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

 
 // CHECK:     VarDecl {{.*}} ambig_call
 // CHECK-NEXT:  `-RecoveryExpr {{.*}} 'int' contains-errors
 // CHECK-NEXT:    |-UnresolvedLookupExpr {{.*}} 'ambig_func'
 // CHECK-NEXT:    `-IntegerLiteral {{.*}} 123
 // DISABLED-NOT: -RecoveryExpr {{.*}} contains-errors
 int ambig_call = ambig_func(123);
 
 // CHECK:     VarDecl {{.*}} unresolved_call1
 // CHECK-NEXT:`-RecoveryExpr {{.*}} '<dependent type>' contains-errors
 // CHECK-NEXT:  `-UnresolvedLookupExpr {{.*}} 'bar'
 // DISABLED-NOT: -RecoveryExpr {{.*}} contains-errors
 int unresolved_call1 = bar();
 
 // CHECK:     VarDecl {{.*}} unresolved_call2
 // CHECK-NEXT:`-CallExpr {{.*}} contains-errors
 // CHECK-NEXT:  |-UnresolvedLookupExpr {{.*}} 'bar'

 // CHECK-NEXT:  | `-DeclRefExpr {{.*}} 'foo'
 // CHECK-NEXT:  `-DeclRefExpr {{.*}} 'x'
 struct Foo {} foo;
 void test(int x) {
   foo.abc;
   foo->func(x);
 }
 
+struct Foo2 {
+  double func();
+  class ForwardClass;
+  ForwardClass createFwd();
+};
+void test2(Foo2 f) {
+  // CHECK:      RecoveryExpr {{.*}} 'double'
+  // CHECK-NEXT:   |-MemberExpr {{.*}} '<bound member function type>'
+  // CHECK-NEXT:   | `-DeclRefExpr {{.*}} 'f'
+  // CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 1
+  f.func(1);
+  // CHECK:      RecoveryExpr {{.*}} 'Foo2::ForwardClass'
+  // CHECK-NEXT: `-MemberExpr {{.*}} '<bound member function type>' .createFwd
+  // CHECK-NEXT:   `-DeclRefExpr {{.*}} 'f'
+  f.createFwd();
+}
+
 // CHECK:     |-AlignedAttr {{.*}} alignas
 // CHECK-NEXT:| `-RecoveryExpr {{.*}} contains-errors
 // CHECK-NEXT:|   `-UnresolvedLookupExpr {{.*}} 'invalid'
 struct alignas(invalid()) Aligned {};
 
 auto f();
 int f(double);
 // CHECK:      VarDecl {{.*}} unknown_type_call 'int'