[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 ↗

(On Diff #310450)

nit: AllowRecovery (case)

5242 ↗

(On Diff #310450)

(and here)

clang/lib/Sema/SemaExpr.cpp

6312 ↗

(On Diff #310450)

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

clang/lib/Sema/SemaOverload.cpp

14168

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/lib/Sema/SemaOverload.cpp

if (ConvertArgumentsForCall(call, op, nullptr, proto, Args, RParenLoc))
       return ExprError();
 
     if (CheckOtherCall(call, proto))
       return ExprError();
 
     return MaybeBindToTemporary(call);
   }
 
   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();
 
   MemberExpr *MemExpr;
ExprResult ObjectArg =
                                           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();
+                              RParenLoc)) {
+    std::vector<Expr *> SubExprs = {MemExprE};
+    llvm::for_each(Args, [&SubExprs](Expr *E) { SubExprs.push_back(E); });
+    return CreateRecoveryExpr(MemExprE->getBeginLoc(), RParenLoc, SubExprs,
+                              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);
 }
 
+// CHECK:      FunctionDecl {{.*}} test2
+// CHECK-NEXT: |-ParmVarDecl {{.*}} f
+// CHECK-NEXT: `-CompoundStmt
+// CHECK-NEXT:   `-RecoveryExpr {{.*}} 'double'
+// CHECK-NEXT:     |-MemberExpr {{.*}} '<bound member function type>'
+// CHECK-NEXT:     | `-DeclRefExpr {{.*}}
+// CHECK-NEXT:   `-IntegerLiteral {{.*}} 'int' 1
+struct Foo2 {
+  double func();
+};
+void test2(Foo2 f) {
+  f.func(1);
+}
+
 // 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'

clang/test/SemaCXX/ms-property-error.cpp

 // RUN: %clang_cc1 -verify -fms-compatibility %s -fsyntax-only -o -
 
 class S {
 public:
   __declspec(property(get=GetX,put=PutX)) int x[];
   int GetX(int i, int j) { return i+j; } // expected-note {{'GetX' declared here}}
   void PutX(int i, int j, int k) { j = i = k; } // expected-note {{'PutX' declared here}}
 };
 
 char *ptr;
 template <typename T>
 class St {
 public:
   __declspec(property(get=GetX,put=PutX)) T x[];
   T GetX(T i, T j) { return i+j; } // expected-note 3 {{'GetX' declared here}}
-  T PutX(T i, T j, T k) { return j = i = k; }  // expected-note 2 {{'PutX' declared here}}
+  T PutX(T i, T j, T k) { return j = i = k; }  // expected-note 3 {{'PutX' declared here}}
   ~St() {
     x[1] = 0; // expected-error {{too few arguments to function call, expected 3, have 2}}
     x[2][3] = 4;
     ++x[2][3];
     x[1][2] = x[3][4][5]; // expected-error {{too many arguments to function call, expected 2, have 3}}
     ptr = x[1][2] = x[3][4]; // expected-error {{incompatible integer to pointer conversion assigning to 'char *' from 'int'}}
   }
 };
 
 // CHECK-LABEL: main
 int main(int argc, char **argv) {
   S *p1 = 0;
   St<float> *p2 = 0;
   St<int> a; // expected-note {{in instantiation of member function 'St<int>::~St' requested here}}
   int j = (p1->x)[223][11][2]; // expected-error {{too many arguments to function call, expected 2, have 3}}
   (p1->x[23]) = argc; // expected-error {{too few arguments to function call, expected 3, have 2}}
   float j1 = (p2->x); // expected-error {{too few arguments to function call, expected 2, have 0}}
   ((p2->x)[23])[1][2] = *argv; // expected-error {{too many arguments to function call, expected 3, have 4}}
   argv = p2->x[11][22] = argc; // expected-error {{assigning to 'char **' from incompatible type 'float'}}
-  return ++(((p2->x)[23])); // expected-error {{too few arguments to function call, expected 2, have 1}}
+  return ++(((p2->x)[23])); // expected-error {{too few arguments to function call, expected 2, have 1}} \
+                               expected-error {{too few arguments to function call, expected 3, have 2}} 
 }