[clangd] Make forwarding parameter detection logic resilient

[kadircet]

Summary

This could crash when our heuristic picks the wrong function. Make sure
there is enough parameters in the candidate to prevent those crashes.

Also special case copy/move constructors to make the heuristic work in
presence of those.

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

[kadircet]

• Rebase

[upsj]

Your approach to handling implicit conversions is much nicer than mine, so I abandoned my revision. There is still one case (that might even occur in the code base I'm working on?) that this change would lead to incorrect hints on. WDYT about keeping the other changes, but using my pack detection logic?

clang-tools-extra/clangd/AST.cpp

791

This is not a sufficient check, since the other parameters need not be from an expanded pack.
Example:

void foo(int a, int b);
int baz(int x, int y);
template <typename... Args>
void bar(Args... args) {
  foo(baz(args, 1)...);
}

void foo() {
  bar(1, 42);
}

Here we shouldn't print a hint at all, but we print x: and y:
The important distinction is between Expr(args...) and Expr(args)..., which can be decided in the instantiated case by the check I implemented in https://reviews.llvm.org/D130259 for all cases, except when args only consists of a single element.

[kadircet]

• Check to ensure function call receives all the arguments

[kadircet]

• Add OOB check as an asssertion.

[kadircet]

clang-tools-extra/clangd/AST.cpp

791

yeah that makes sense, added an extra check.

[upsj]

Not sure I qualify as a suitable reviewer, but this looks really good to me, save for maybe a small safety measure :)

clang-tools-extra/clangd/AST.cpp

837–841

I think it would be safer to check this explicitly, since advancing the iterator past its end might be UB (depending on the implementation).

[kadircet]

• Rather than asserting limit the traversal
• Have more comments

[ilya-biryukov]

clang-tools-extra/clangd/AST.cpp

837

This could crash in foo(1, args...), right?

890

NIT: should fit in a single line

[kadircet]

• Update test case

[sammccall]

driveby thoughts, but please don't block on them.

(if this fix is a heuristic that fixes a common crash but isn't completely correct, that may still be worth landing but warrants a fixme)

clang-tools-extra/clangd/AST.cpp

836

The essence of this test seems to be "if we can't rule it out by counting parameters, it must be right" which... doesn't seem very robust? Am I missing something?

AIUI the idea here is that
A) foo(pack...) is "expanded all the way" into consecutive arguments
B) foo(pack)... is not considered forwarding for our purposes
C) we want to treat foo(bar(pack)...) specially if bar is "just a cast" like move or forward

The difference between cases A and B aren't about counting arguments though, they're about what operations occur intercede between pack and the ... operator. (i.e. foo((&pack)...) is fine for the same reason forward is.

This seems like something best handled by:

• finding the PackExpansionExpr controlling pack in the non-instantiated template AST
• walking down to try to find pack
• continue as we walk through e.g. parens or std::forward
• bail out if we walk through something unknown (e.g. another function call)

Does this seem sensible/feasible?
(Walking up from pack is nicer but the AST doesn't support walking up well...)

837–841

Right, this is definitely UB as written.

FWIW I find this easier to understand in classic bounds-check form:

if ((It - Args.begin()) + Parameters.size() >= Args.size())

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

1468

(Just so I understand: these are getting hinted due to the "only instantiation" logic, right?)

1471

could reasonably expect this to be bar(a:1, 2) or bar(a:1, a:2) - leave a comment explaining why?

[upsj]

clang-tools-extra/clangd/AST.cpp

836

This goes a bit more generally into the question how to approach parameter packs across clangd. For inlay hints and hover, we are already looking at instantiated templates (is that true in all cases? Haven't dug in yet), so implementing forwarding there was straightforward. For signature help and code completion, we'd probably need to go via the uninstantiated template (and probably lose a bit of type information on the way, e.g. reference vs. value, l-value vs. r-value). If we wanted a generic way to deal with forwarding, the instantiation pattern would be my first choice, but I didn't have the time to go down this rabbit hole yet.
Unless there is a nice way to map between template and instantiation AST, I don't think combining them is feasible.

On the heuristic itself: Is there a way in C++ to "partially" unpack a parameter pack without this showing up in the AST? I am not aware of any, which is why the test "do the first and last parameters appear in a matching range of the arguments" seems sufficient and necessary in all but single-entry parameter packs.

[ilya-biryukov]

clang-tools-extra/clangd/AST.cpp

829–833

NIT: you could use Args.end() - Parameters.size() or std::advance equivalent.

[sammccall]

clang-tools-extra/clangd/AST.cpp

836

Unless there is a nice way to map between template and instantiation AST, I don't think combining them is feasible.

Fair enough, I don't see one.

On the heuristic itself: Is there a way in C++ to "partially" unpack a parameter pack without this showing up in the AST?

Not AFAIK, the is either expanded at "this level" yielding sibling args, or at "a higher level" yielding a single arg.
So maybe the "first & last" test is actually robust, but it would need some justification in comments.

There's an exception I think, if the pack size is one it falls over:

bar(int bar0);
bar(int bar0, int bar1);
bar(int bar0i, int bar1, int bar2);

template <class...Args>
void foo(Args...args) {
  bar(args...); // should yield labels when expanded
  bar(args)...; // should not yield labels when expanded
}
// but when there's one arg (foo<int>) the two have the same AST

I think it's OK to just get this one wrong

[kadircet]

• Address comments

[kadircet]

In D130260#3671290, @sammccall wrote:

driveby thoughts, but please don't block on them.

(if this fix is a heuristic that fixes a common crash but isn't completely correct, that may still be worth landing but warrants a fixme)

The fix is not an heuristic. We just make sure we never consider a function with less parameters than the arguments we have, that way rest of the assumptions hold (we might still pick a wrong function call, as we could in the past, but we won't crash).
In addition to that heuristics are improved a little bit to work in presence of copy/move constructors.

clang-tools-extra/clangd/AST.cpp

836

i agree that we can do things differently here, but with this patch i am mostly trying to make sure we address the resiliency so that this doesn't bite us in production (and a couple of easy improvements to the existing cases for common code patterns).

i'd land this patch as-is and discuss doing these changes in a separate issue if possible.

837–841

i didn't do an explicit check, because we already have that explicit check inside the call site and doing a possibly linear operation for each argument in production setup doesn't feel nice

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

1468

not only instantiation but "only overload" logic (i suppose that's what you mean?)

[ilya-biryukov]

LGTM

clang-tools-extra/clangd/AST.cpp

830

NIT: could be shorter with llvm::size(Args)

[sammccall]

In D130260#3671494, @kadircet wrote:

In D130260#3671290, @sammccall wrote:

driveby thoughts, but please don't block on them.

(if this fix is a heuristic that fixes a common crash but isn't completely correct, that may still be worth landing but warrants a fixme)

The fix is not an heuristic. We just make sure we never consider a function with less parameters than the arguments we have, that way rest of the assumptions hold (we might still pick a wrong function call, as we could in the past, but we won't crash).
In addition to that heuristics are improved a little bit to work in presence of copy/move constructors.

OK, but it's missing a comment that says "this is a heuristic and may not be correct, at least it won't crash"!

The original code was intended to be *correct*, not a heuristic. If we're not going to fix it properly (which is reasonable in the circumstances), we need to document the limitations.

clang-tools-extra/clangd/AST.cpp

 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/raw_ostream.h"
+#include <iterator>
 #include <string>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 
 namespace {
 llvm::Optional<llvm::ArrayRef<TemplateArgumentLoc>>
public:
 
   // The output of this visitor
   Optional<ForwardingInfo> Info;
 
 private:
   // inspects the given callee with the given args to check whether it
   // contains Parameters, and sets Info accordingly.
   void handleCall(FunctionDecl *Callee, typename CallExpr::arg_range Args) {
+    // Skip functions with less parameters, they can't be the target.
+    if (Callee->parameters().size() < Parameters.size())
+      return;
     if (std::any_of(Args.begin(), Args.end(), [](const Expr *E) {
           return dyn_cast<PackExpansionExpr>(E) != nullptr;
         })) {
       return;
     }
     auto PackLocation = findPack(Args);
     if (!PackLocation)
       return;
void handleCall(FunctionDecl *Callee, typename CallExpr::arg_range Args) {
     FI.Head = MatchingParams;
     Info = FI;
   }
 
   // Returns the beginning of the expanded pack represented by Parameters
   // in the given arguments, if it is there.
   llvm::Optional<size_t> findPack(typename CallExpr::arg_range Args) {
     // find the argument directly referring to the first parameter
-    for (auto It = Args.begin(); It != Args.end(); ++It) {
-      const Expr *Arg = *It;
-      if (const auto *RefArg = unwrapForward(Arg)) {
+    assert(Parameters.size() <=
+           static_cast<size_t>(std::distance(Args.begin(), Args.end())));
+    for (auto Begin = Args.begin(), End = Args.end() - Parameters.size() + 1;
+         Begin != End; ++Begin) {
+      if (const auto *RefArg = unwrapForward(*Begin)) {
         if (Parameters.front() != RefArg->getDecl())
           continue;
-        return std::distance(Args.begin(), It);
+        // Check that this expands all the way until the last parameter.
+        // It's enough to look at the last parameter, because it isn't possible
+        // to expand without expanding all of them.
+        auto ParamEnd = Begin + Parameters.size() - 1;
+        RefArg = unwrapForward(*ParamEnd);
+        if (!RefArg || Parameters.back() != RefArg->getDecl())
+          continue;
+        return std::distance(Args.begin(), Begin);
       }
     }
     return llvm::None;
   }
 
   static FunctionDecl *getCalleeDeclOrUniqueOverload(CallExpr *E) {
     Decl *CalleeDecl = E->getCalleeDecl();
     auto *Callee = dyn_cast_or_null<FunctionDecl>(CalleeDecl);
static FunctionDecl *resolveOverload(UnresolvedLookupExpr *Lookup,
     }
     return MatchingDecl;
   }
 
   // Tries to get to the underlying argument by unwrapping implicit nodes and
   // std::forward.
   static const DeclRefExpr *unwrapForward(const Expr *E) {
     E = E->IgnoreImplicitAsWritten();
+    // There might be an implicit copy/move constructor call on top of the
+    // forwarded arg.
+    // FIXME: Maybe mark implicit calls in the AST to properly filter here.
+    if (const auto *Const = dyn_cast<CXXConstructExpr>(E))
+      if (Const->getConstructor()->isCopyOrMoveConstructor())
+        E = Const->getArg(0)->IgnoreImplicitAsWritten();
     if (const auto *Call = dyn_cast<CallExpr>(E)) {
       const auto Callee = Call->getBuiltinCallee();
       if (Callee == Builtin::BIforward) {
         return dyn_cast<DeclRefExpr>(
             Call->getArg(0)->IgnoreImplicitAsWritten());
       }
     }
     return dyn_cast<DeclRefExpr>(E);

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

Annotations Code(R"cpp(
     auto c = '2';]]
     auto d = 3.f;
   )cpp");
   auto AST = TestTU::withCode(Code.code()).build();
   EXPECT_THAT(inlayHints(AST, Code.range()),
               ElementsAre(labelIs(": int"), labelIs(": char")));
 }
 
+TEST(ParameterHints, ArgPacksAndConstructors) {
+  assertParameterHints(
+      R"cpp(
+    struct Foo{ Foo(); Foo(int x); };
+    void foo(Foo a, int b);
+    template <typename... Args>
+    void bar(Args... args) {
+      foo(args...);
+    }
+    template <typename... Args>
+    void baz(Args... args) { foo($param1[[Foo{args...}]], $param2[[1]]); }
+
+    template <typename... Args>
+    void bax(Args... args) { foo($param3[[{args...}]], args...); }
+
+    void foo() {
+      bar($param4[[Foo{}]], $param5[[42]]);
+      bar($param6[[42]], $param7[[42]]);
+      baz($param8[[42]]);
+      bax($param9[[42]]);
+    }
+  )cpp",
+      ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"},
+      ExpectedHint{"a: ", "param3"}, ExpectedHint{"a: ", "param4"},
+      ExpectedHint{"b: ", "param5"}, ExpectedHint{"a: ", "param6"},
+      ExpectedHint{"b: ", "param7"}, ExpectedHint{"x: ", "param8"},
+      ExpectedHint{"b: ", "param9"});
+}
+
+TEST(ParameterHints, DoesntExpandAllArgs) {
+  assertParameterHints(
+      R"cpp(
+    void foo(int x, int y);
+    int id(int a, int b, int c);
+    template <typename... Args>
+    void bar(Args... args) {
+      foo(id($param1[[args]], $param2[[1]], $param3[[args]])...);
+    }
+    void foo() {
+      bar(1, 2); // FIXME: We could have `bar(a: 1, a: 2)` here.
+    }
+  )cpp",
+      ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"},
+      ExpectedHint{"c: ", "param3"});
+}
 // FIXME: Low-hanging fruit where we could omit a type hint:
 //  - auto x = TypeName(...);
 //  - auto x = (TypeName) (...);
 //  - auto x = static_cast<TypeName>(...);  // and other built-in casts
 
 // Annoyances for which a heuristic is not obvious:
 //  - auto x = llvm::dyn_cast<LongTypeName>(y);  // and similar
 //  - stdlib algos return unwieldy __normal_iterator<X*, ...> type
 //    (For this one, perhaps we should omit type hints that start
 //     with a double underscore.)
 
 } // namespace
 } // namespace clangd
 } // namespace clang