[clangd] fix crash and handle implicit conversions in inlay hints for forwarding functions

[upsj]

Summary

The ForwardingCallVisitor previously picked up on (constructor or function) calls inside a single argument of a variadic function, where the corresponding pack expression is (partially) expanded, but only consists of a single entry. This mismatch between expecting the full pack and finding only a single entry caused a crash, which the first half of this diff fixes.

The second half deals with the failure of resolveForwardingParameters to detect forwarded parameters when implicit conversions happen via constructor calls, not normal ImplicitCastExpr.

[upsj]

clang-tools-extra/clangd/unittests/InlayHintTests.cpp

721

I'm not sure whether we should detect this case, since it would be nice to handle, but also involves digging deep through CXXConstructExpression -> MaterializeTemporaryExpression -> CXXMemberCallExpr -> MemberExpr -> DeclRefExpr

[kadircet]

sorry for not mentioning in the bug that i was also working on a patch, D130260 seems to be a little bit more contained and focused on making the check not crash (as we're getting close to release cut).

WDYT about landing it now, and iterating on this patch afterwards if you still have cases that it doesn't handle but this patch does? (I can't really see such cases in hindsight)

clang-tools-extra/clangd/AST.cpp

844

so in theory this is still a heuristic, and somewhat complicated. what about just checking if we have "enough parameters" to satisfy the pack expansion (as i did in D130260)

914

this is already handled by IgnoreImplicitAsWritten the underlying function is using now (landed in https://reviews.llvm.org/D130261)

919

this is checking for constructor being explicit, and not the callexpr itself. so for example it won't fire up if call is implicit to an explicitly defined copy constructor.

[nridge]

In D130259#3671009, @kadircet wrote:

sorry for not mentioning in the bug that i was also working on a patch,

Which bug/issue is this?

[kadircet]

In D130259#3671022, @nridge wrote:

Which bug/issue is this?

https://github.com/llvm/llvm-project/issues/56620

[upsj]

Superseded by https://reviews.llvm.org/D130260

clang-tools-extra/clangd/AST.cpp

auto FirstMatch = std::find_if(Args.begin(), Args.end(), [&](Expr *Arg) {
       const auto *RefArg = unwrapArgument(Arg);
       if (RefArg) {
         if (Parameters.front() == dyn_cast<ParmVarDecl>(RefArg->getDecl())) {
           return true;
         }
       }
       return false;
     });
-    if (FirstMatch == Args.end()) {
-      return llvm::None;
+    if (FirstMatch != Args.end()) {
+      const auto NumArgs =
+          static_cast<size_t>(std::distance(Args.begin(), Args.end()));
+      const auto FirstMatchIdx =
+          static_cast<size_t>(std::distance(Args.begin(), FirstMatch));
+      // Now we found the first parameter, check whether the last one also fits
+      // This is necessary because we may be matching a function call inside an
+      // argument of the variadic function if we didn't successfully match the
+      // outer variadic call.
+      const auto LastMatchIdx = FirstMatchIdx + Parameters.size() - 1;
+      if (LastMatchIdx < NumArgs) {
+        if (const auto *RefArg =
+                unwrapArgument(*(Args.begin() + LastMatchIdx))) {
+          if (Parameters.back() == dyn_cast<ParmVarDecl>(RefArg->getDecl())) {
+            return FirstMatchIdx;
+          }
+        }
       }
-    return std::distance(Args.begin(), FirstMatch);
+    }
+    return llvm::None;
   }
 
   static FunctionDecl *getCalleeDeclOrUniqueOverload(CallExpr *E) {
     Decl *CalleeDecl = E->getCalleeDecl();
     auto *Callee = dyn_cast_or_null<FunctionDecl>(CalleeDecl);
     if (!Callee) {
       if (auto *Lookup = dyn_cast<UnresolvedLookupExpr>(E->getCallee())) {
         Callee = resolveOverload(Lookup, E);
private:
   // Removes any implicit cast expressions around the given expression.
   static const Expr *unwrapImplicitCast(const Expr *E) {
     while (const auto *Cast = dyn_cast<ImplicitCastExpr>(E)) {
       E = Cast->getSubExpr();
     }
     return E;
   }
 
-  // Maps std::forward(E) to E, nullptr otherwise
+  // Maps std::forward(E) to E
   static const Expr *unwrapForward(const Expr *E) {
     if (const auto *Call = dyn_cast<CallExpr>(E)) {
       const auto Callee = Call->getBuiltinCallee();
       if (Callee == Builtin::BIforward) {
         return Call->getArg(0);
       }
     }
     return E;
   }
 
+  // Maps an argument implicitly calling a converting constructor to the
+  // constructor's argument
+  static const Expr *unwrapConstructorConversion(const Expr *E) {
+    if (const auto *CBTE = dyn_cast<CXXBindTemporaryExpr>(E)) {
+      E = CBTE->getSubExpr();
+    }
+    if (const auto *CCE = dyn_cast<CXXConstructExpr>(E)) {
+      const auto *C = CCE->getConstructor();
+      if (!C->isExplicit() && C->getNumParams() == 1) {
+        E = CCE->getArg(0);
+      }
+    }
+    return E;
+  }
+
   // Maps std::forward(DeclRefExpr) to DeclRefExpr, removing any intermediate
   // implicit casts, nullptr otherwise
   static const DeclRefExpr *unwrapArgument(const Expr *E) {
     E = unwrapImplicitCast(E);
+    E = unwrapConstructorConversion(E);
     E = unwrapForward(E);
     E = unwrapImplicitCast(E);
     return dyn_cast<DeclRefExpr>(E);
   }
 };
 
 } // namespace
 

clang-tools-extra/clangd/unittests/InlayHintTests.cpp

void foo() {
       c.emplace($param1[[1]]);
       c.emplace($param2[[2]], $param3[[3]]);
     }
   )cpp",
       ExpectedHint{"A: ", "param1"}, ExpectedHint{"B: ", "param2"},
       ExpectedHint{"C: ", "param3"});
 }
 
+TEST(ParameterHints, VariadicWithConstructorConversion) {
+  // variadic call that involves a conversion via a (potentially implicitly
+  // provided) constructor, here move-construction Foo&& -> Foo
+  assertParameterHints(
+      R"cpp(
+    struct Foo {};
+    void foo(Foo s, int x);
+
+    template <typename... _Args>
+    void bar(_Args ...__args) {
+      foo(__args...);
+    }
+
+    void foo() {
+      bar($param1[[Foo{}]], $param2[[3]]);
+    }
+  )cpp",
+      ExpectedHint{"s: ", "param1"}, ExpectedHint{"x: ", "param2"});
+}
+
+TEST(ParameterHints, VariadicWithOperatorConversion) {
+  // variadic call that involves a conversion via an operator, here Foo -> Bar
+  assertParameterHints(
+      R"cpp(
+    struct Bar {};
+    struct Foo {
+      operator Bar();
+    };
+    void foo(Bar s, int x);
+
+    template <typename... _Args>
+    void bar(_Args ...__args) {
+      foo(__args...);
+    }
+
+    void foo() {
+      bar($param1[[Foo{}]], $param2[[3]]);
+    }
+  )cpp" /*, ExpectedHint{"s: ", "param1"}, ExpectedHint{"x: ", "param2"}*/);
+}
+
+TEST(ParameterHints, VariadicWithFunctionCall) {
+  // variadic call that involves an explicit transformation via a function, here
+  // we don't output an inlay hint, since it's not clear what transform does.
+  assertParameterHints(
+      R"cpp(
+    struct Bar {};
+    struct Foo {
+      operator Bar();
+    };
+    template <typename T>
+    T transform(T);
+    void foo(Bar s, int x);
+
+    template <typename... _Args>
+    void bar(_Args ...__args) {
+      foo(transform(__args)...);
+    }
+
+    void foo() {
+      bar(Foo{}, 3);
+    }
+  )cpp");
+}
+
 TEST(ParameterHints, VariadicReferenceHint) {
   assertParameterHints(R"cpp(
     void foo(int&);
     template <typename... Args>
     void bar(Args... args) { return foo(args...); }
     void baz() {
       int a;
       bar(a);