[clangd] Avoid calling resolveTypeOfCallExpr() twice in HeuristicResolver::resolveExprToType()

[HighCommander4]

Fixes the performance regression reported at #156282 (comment)

[llvmbot]

@llvm/pr-subscribers-clang

Author: Nathan Ridge (HighCommander4)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/164353.diff

1 Files Affected:

• (modified) clang/lib/Sema/HeuristicResolver.cpp (+5)

diff --git a/clang/lib/Sema/HeuristicResolver.cpp b/clang/lib/Sema/HeuristicResolver.cpp
index cbdefaa57aacb..056e13308b7d3 100644
--- a/clang/lib/Sema/HeuristicResolver.cpp
+++ b/clang/lib/Sema/HeuristicResolver.cpp
@@ -450,7 +450,12 @@ QualType HeuristicResolverImpl::resolveExprToType(const Expr *E) {
   if (const auto *CE = dyn_cast<CallExpr>(E)) {
     if (QualType Resolved = resolveTypeOfCallExpr(CE); !Resolved.isNull())
       return Resolved;
+
+    // Don't proceed to try resolveExprToDecls(), it would just call
+    // resolveTypeOfCallExpr() again.
+    return E->getType();
   }
+
   // Similarly, unwrapping a unary dereference operation does not work via
   // resolveExprToDecls.
   if (const auto *UO = dyn_cast<UnaryOperator>(E->IgnoreParenCasts())) {

[HighCommander4]

@kadircet Do we have a good way of writing a regression test for something like this? Checking elapsed time seems flaky.

[kadircet]

thanks a lot for the quick fix!

@kadircet Do we have a good way of writing a regression test for something like this? Checking elapsed time seems flaky.

Unfortunately, no, I don't think we do :/ Best that comes to mind is have a callback somewhere down the stack when we resolve a dependent name, print the stack there and check for certain invariants (and hope that optimizations etc. don't trip that up). I don't think that's gonna be worthwhile either.

Still having a unittest, with a comment saying "this shouldn't increase testing times", would likely be better than nothing. Long running tests are likely to catch some attention, at least from us :)

[HighCommander4]

@kadircet Do we have a good way of writing a regression test for something like this? Checking elapsed time seems flaky.

Unfortunately, no, I don't think we do :/ Best that comes to mind is have a callback somewhere down the stack when we resolve a dependent name, print the stack there and check for certain invariants (and hope that optimizations etc. don't trip that up). I don't think that's gonna be worthwhile either.

One idea I had was to put something like a trace::Span in a function which gets called expontentially many times before the fix, and have the test install a custom tracer which counts the number of calls, and asserts that it's below a threshold. However, trace::Span is a clangd utility, and the affected functions now live in libSema, so that doesn't quite work.

Still having a unittest, with a comment saying "this shouldn't increase testing times", would likely be better than nothing. Long running tests are likely to catch some attention, at least from us :)

Sounds good, I added a test like this.