[clang][AST] Handle overload callee type in CallExpr::getCallReturnType.

[balazske]

Summary

The function did not handle every case. In some cases this
caused assertion failure.
After the fix the function returns DependentTy if the exact
return type can not be determined.

It seems that clang itself does not call the function in the
affected cases but some checker or other code may call it.

[balazske]

Should add some responsible (for this code) reviewers.

[balazske]

clang/lib/AST/Expr.cpp

1402

Here occurs the problem: If CalleeType is of type BuiltinType::Overload it does not cast to FunctionType.

[martong]

Hi Balázs,

Could you please create a lit test file which ignites the related crash?

[balazske]

Adding a test, fixing other problems revealed by the test.

[balazske]

I could add a test in ToolingTests, lit test is much more difficult to add because the function is normally not called on "bad" code.

clang/lib/AST/Expr.cpp

1396

I am not sure if dependent type is here (and later) the correct value to return. But only a null type is another possibility.

[martong]

Adding reviewers.
@rsmith as code owner,
@majnemer based on git blame of CallExpr::getCallReturnType.

[balazske]

ping

[balazske]

Ping

[balazske]

Ping

[balazske]

Ping.

[martong]

This causes a crash if the function is called in such case.

Is the crash caused by the below assertion?

QualType Expr::findBoundMemberType(const Expr *expr) {
  assert(expr->hasPlaceholderType(BuiltinType::BoundMember));

clang/lib/AST/Expr.cpp

1396

Perhaps we should handle this similarly as the type of UnresolvedMemberExpr or OverloadExpr? Which I assume is a null type.

clang/unittests/Tooling/SourceCodeTest.cpp

642–644 ↗

(On Diff #319261)

Do we have the crash with both f_overload and with a.f(p) calls?
If yes, then please create two separate test cases.

[martong]

clang/lib/AST/Expr.cpp

1399

Can't we create a built in place holder type here? Or could we simply just return CalleeType?

[balazske]

rebase, split the test case

[martong]

Hi @aaron.ballman @riccibruno @dblaikie, I am adding you as a reviewer because it seems you have great experience with Clang's AST.
Could you please take a look on this patch? The problem is that we currently do not handle a CallExpr's return type properly in case of a dependent context. This seems to be a flaw in the implementation of CallExpr.

[dblaikie]

Added a couple of other folks who might have more context on the tooling front...

[hokein]

This looks like a reasonable solution to me -- if we don't know the function call result type (e.g. dependent-type callExpr in template context), modeling it as a dependent type.

clang/lib/AST/Expr.cpp

1386

this can be removed, dependent type is a builtin type, so you could just use Ctx.DependentTy.

1395

The only case is UnresolvedMemberExpr, I think we'd keep the previous comment and special-case the UnresolvedMemberExpr (like the CXXPseudoDestructorExpr above).

if (isa<UnresolvedMemberExpr>(...))
  return Ctx.DependentTy;

 // This should never be overloaded and so should never return null.
 CalleeType = Expr::findBoundMemberType(Callee);
 CHECK(!CalleeType.isNull());

clang/unittests/Tooling/SourceCodeTest.cpp

630 ↗

(On Diff #329647)

CalleeType is not a specific term in getCallReturnType, just CalleeType is overload and dependent, I think we can also verify this in the Visitor.OnCall callback.

IIUC, the patch fixes three different crashes

• calling getCallReturntype on the f(t) expr
• calling getCallReturntype on the f_overload() expr
• calling getCallReturntype on the a.f_overload(p); expr

I think it would be much clear to express them in the OnCall callback (rather than calling getCallReturnType on every Expr). One option is to use the annotation, and identify the expression by location like below. An other benefit is that we can unify the test code (instead of duplicating them) without hurting the code readability.

template<class T, class F>
void templ(const T& t, F f) {
  $crash1[[f]](t);
  // CalleeType in getCallReturntype is Overload and dependent
}

struct A {
  void f_overload(int);
  void f_overload(double);
};

void f() {
  int i = 0;
  templ(i, [](const auto &p) {
    $crash2[[f_overload]](p); // UnresolvedLookupExpr
    // CalleeType in getCallReturntype is Overload and not dependent
  });

  templ(i, [](const auto &p) {
    A a;
    a.$crash3[[f_overload]](p); // UnresolvedMemberExpr
    // CalleeType in getCallReturntype is BoundMember and has overloads
  });
  
}

[balazske]

clang/lib/AST/Expr.cpp

1386

useful, I did not observe that

clang/unittests/Tooling/SourceCodeTest.cpp

630 ↗

(On Diff #329647)

The current test finds every CallExpr and calls the getCallReturnType on it. The test should verify that this function works, so at least calling it is needed. An additional check could be to verify that the "Callee" is really of the specific kind (UnresolvedLookupExpr and the others), this can be added. I do not get it how the annotations can be used here, it is possible only to get the position of the code in the string but how to use this value?

[balazske]

Fixed according to the comments.

[balazske]

Ping.
I am not sure how to the test should be changed.

[hokein]

clang/unittests/Tooling/SourceCodeTest.cpp

630 ↗

(On Diff #329647)

yes, I'd like to avoid the current test pattern where we invoke getCallReturnType on every CallExpr, and don't verify the call-expr kind.

you can find the corresponding expr by matching the annotation locations, mostly can be done in the OnCall callback, like

Visitor.OnCall = [](..Expr) {
   // check if location of Expr is one of the annotation locations
   //  - no: return
   //  - yes: find out the which one -- let's say, crash2, then we know the Expr should be UnresolvedLookupExpr, and the CalleeType is Overload and not dependent, and verify them.
};

[balazske]

Rebase, changed test according to review comments.

[balazske]

clang/unittests/Tooling/SourceCodeTest.cpp

630 ↗

(On Diff #329647)

I am not familiar with this kind of tests but updated the code to check CallExpr at the affected places only.

[hokein]

thanks, looks good!

clang/lib/AST/Expr.cpp

1385

nit: looks like an accident change, remove it.

[balazske]

clang/lib/AST/Expr.cpp

1385

Fixed in the committed code.

clang/lib/AST/Expr.cpp

 //===--- Expr.cpp - Expression AST Node Implementation --------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
bool CallExpr::isUnevaluatedBuiltinCall(const ASTContext &Ctx) const {
   if (unsigned BI = getBuiltinCallee())
     return Ctx.BuiltinInfo.isUnevaluated(BI);
   return false;
 }
 
 QualType CallExpr::getCallReturnType(const ASTContext &Ctx) const {
   const Expr *Callee = getCallee();
   QualType CalleeType = Callee->getType();
   if (const auto *FnTypePtr = CalleeType->getAs<PointerType>()) {
     CalleeType = FnTypePtr->getPointeeType();
   } else if (const auto *BPT = CalleeType->getAs<BlockPointerType>()) {
     CalleeType = BPT->getPointeeType();
   } else if (CalleeType->isSpecificPlaceholderType(BuiltinType::BoundMember)) {
     if (isa<CXXPseudoDestructorExpr>(Callee->IgnoreParens()))
       return Ctx.VoidTy;
 
     // This should never be overloaded and so should never return null.
     CalleeType = Expr::findBoundMemberType(Callee);
+  } else if (CalleeType->isSpecificPlaceholderType(BuiltinType::Overload)) {
+    ASTContext::GetBuiltinTypeError Error;
+    QualType RetTy = Ctx.GetBuiltinType(BuiltinType::Dependent, Error);
+    assert(Error == ASTContext::GE_None);
+    return RetTy;
   }
 
   const FunctionType *FnType = CalleeType->castAs<FunctionType>();
   return FnType->getReturnType();
 }
 
 const Attr *CallExpr::getUnusedResultAttr(const ASTContext &Ctx) const {
   // If the return type is a struct, union, or enum that is marked nodiscard,
   // then return the return type attribute.
   if (const TagDecl *TD = getCallReturnType(Ctx)->getAsTagDecl())
     if (const auto *A = TD->getAttr<WarnUnusedResultAttr>())