[clangd] add inlay hints for std::forward-ed parameter packs

[upsj]

Summary

This adds special-case treatment for parameter packs in make_unique-like functions to forward parameter names to inlay hints.

[sammccall]

clang-tools-extra/clangd/InlayHints.cpp

382

We tend not to use ASTMatchers for these kind of tasks in clangd (except when embedding clang-tidy checks of course).

I guess the biggest technical reason is it's hard to reason about their performance so they end up slow and opaque (and indeed the clang-tidy checks are slow and we haven't got a clear idea how to fix it). It's also easy to match too much.

But part of it is just convention - because we have more RecursiveASTVisitors, the maintainers are more familiar with the quirks there than the quirks of matchers.

---

To be clear, I don't see any specific problems with this code! But we still might ask to change it as there are costs to mixing styles, and we don't want to end up in a situation where a bug fix requires choosing between an expensive hasAncestor() matcher and rewriting the logic.

[upsj]

remove dependency on D124359 and add tests

[upsj]

remove unnecessary changes

[upsj]

clang-tools-extra/clangd/InlayHints.cpp

382

That makes sense. From the Visitor standpoint, do I understand correctly that you mean something like remembering the top-level templated FunctionDecl (or stack of FunctionDecls if we have something like nested Lambdas?) and check every CallExpr and CXXConstructExpr for ParmVarDecls or std::forward(ParmVarDecl) matching the parameters of the higher-level FunctionDecls? That means basically lazily evaluating the parameter names, so more storage inside the Visitor, but allows us to do recursive resolution in a rather straightforward fashion.

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

358

I haven't been able to figure out why, but for some reason the CXXConstructExpr and CXXTemporaryObjectExpr are not being picked up by the RecursiveASTVisitor, while in a similar situation below, the CallExpr gets picked up by VisitCallExpr.

373

This is a bit of an unfortunate side-effect of looking at instantiated templates.

[nridge]

clang-tools-extra/clangd/InlayHints.cpp

382

I imagine something like running a RecursiveASTVisitor on Callee->getBody(), and overriding VisitCallExpr() and VisitCXXConstructExpr() to check whether it is a call of the right form.

I don't think theres a need to worry about lambdas nested in the body, I think we may still be able to get useful parameter names out of them, as in e.g.:

struct S {
 S(int a, int b);
};

template <typename T, typename... Args>
auto Make(Args... args) {
  auto ConstructTask = [&](){ return T(args...); };
  return ConstructTask();
}

int main() {
  auto s = Make<S>(1, 2);
}

409

What is the reason for the Args.size() == Params.size() condition?

Doesn't this break cases where there is more than one argument matching the variadic parameter? For example:

void foo(int a, int b);
template <typename... Args>
void bar(Args&&... args) { foo(args...); }
int main() {
  bar(1, 2);
}

Here, I expect we'd have Args.size() == 2 and Params.size() == 1.

(We should probably have test coverage for such multiple-arguments cases as well. We probably don't need it for all combinations, but we should have at least one test case exercising it.)

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

281

What does "converted into builtin" mean here?

306

This comment seems out of sync with the testcase (same for a few others)

312

The wrapped range doesn't seem to be used anywhere (same for a few other testcases)

373

Perhaps it would make sense to exclude arguments which are pack expansion expressions from getting parameter hints?

[upsj]

clang-tools-extra/clangd/InlayHints.cpp

409

The function we are looking at is an already instantiated template. The check Args.size() == Params.size() is only necessary because of va_args

[nridge]

clang-tools-extra/clangd/InlayHints.cpp

409

Ah, thanks, I overlooked that. A lot of things in this patch that initially confused me make a lot more sense now :)

[upsj]

I will take a different approach to the matchers: For each CallExpr involving parameter packs, record the arguments (if they are a ParmVarDecl or stdForward(ParmVarDecl)) and the matching ParmVarDecl of the FunctionDecl in a map. Then I will compact that map to skip intermediate forwarding functions like emplace_back -> allocator::construct -> constructor and in a second pass resolve all previously unresolved forwarded parameters.

[sammccall]

In D124690#3492981, @upsj wrote:

I will take a different approach to the matchers: For each CallExpr involving parameter packs, record the arguments (if they are a ParmVarDecl or stdForward(ParmVarDecl)) and the matching ParmVarDecl of the FunctionDecl in a map. Then I will compact that map to skip intermediate forwarding functions like emplace_back -> allocator::construct -> constructor and in a second pass resolve all previously unresolved forwarded parameters.

This makes sense to me.
This sounds more complicated, but if I'm reading correctly this multi-level forwarding isn't handled in the current version of the patch either?
At some point we'll want to extract this logic out so it can be used by hover etc, but not yet.

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

358

The AST here of the instantiated bar looks like

-FunctionDecl <line:5:1, line:7:1> line:5:4 used bar 'S *(int &&)'
  |-TemplateArgument type 'S'
  |-TemplateArgument pack
  | `-TemplateArgument type 'int'
  |-ParmVarDecl <col:8, col:18> col:18 used args 'int &&'
  `-CompoundStmt <col:24, line:7:1>
    `-ReturnStmt <line:6:3, col:23>
      `-CXXNewExpr <col:10, col:23> 'S *' Function 'operator new' 'void *(unsigned long)'
        `-CXXConstructExpr <col:14, col:23> 'S':'S' 'void (int)' list
          `-ImplicitCastExpr <col:16> 'int':'int' <LValueToRValue>
            `-DeclRefExpr <col:16> 'int':'int' lvalue ParmVar 'args' 'int &&'

So there's no CXXTemporaryObjectExpr (the value here is a pointer), but should be a CXXConstructExpr.

Are you sure you're traversing bar's instantiation/specializaion, as opposed to its templated/generic FunctionDecl? The AST of the templated FunctionDecl indeed has an InitListExpr in place of the CXXConstructExpr because it's not resolved yet.

[upsj]

! In D124690#3493246, @sammccall wrote:

This makes sense to me.
This sounds more complicated, but if I'm reading correctly this multi-level forwarding isn't handled in the current version of the patch either?
At some point we'll want to extract this logic out so it can be used by hover etc, but not yet.

Yes, currently only direct forwarding works. I originally looked at this when inlay hints were not yet available, and it seemed possible to do this for signature help as well, only most likely based on the template instantiation pattern instead of the instantiated function declaration.

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

358

It's quite possible this is related to the visitor not/only inconsistently traversing template instantiations due to shouldVisitTemplateInstantiations returning false. But I need to implement the new approach first, anyways.

[sammccall]

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

358

Ah, I think I see the confusion.

We don't want to gather all the information in a single traversal of the whole AST. Such a traversal would need to walk all instantiated templates including those from headers, which is far too much/too slow.

Instead, you want to analyze particular forwarding functions by walking through their bodies looking for the forwarding call. RecursiveASTVisitor is the tool for this job - you just call TraverseDecl(FunctionDecl) to limit the scope. The visitor should not visit template instantiations, but the traversal can still be rooted at an instantiation!

So something roughly like:

Optional<vector<ParmVarDecl*>> ultimateParams(FunctionDecl* Callee) {
  if (!Callee || !isa<FunctionProtoType>(Callee->getType()))
    return None;
  if (not instantiated from template || Pattern doesn't use a pack)
    return Callee->parameters();

  for (P : Pattern->parameters()) {
    if (!P->isPack())
      Result.push_back(Callee->parameters[Pos++]);
    else {
      if (PackExpansionSize = ...) {
        if (ForwardedParams = forwardedParams(Callee, P))
          Result.append(*ForwardedParams);
        else // failed, no param info for forwarded args
          Result.append(nullptr, PackExpansionSize);
        Pos += PackExpansionSize;
      }
    }
  }
}

Optional<vector<ParmVarDecl*>> forwardedParams(FunctionDecl *Callee, ParmVarDecl *Pack) {
  class ForwardFinder : RAV<ForwardFinder> {
    bool VisitCallExpr(CallExpr *E) {
      if (E->params() expands Pack) {
        if (CallParams = ultimateParams(E->getCallee()->getAsFunctionDecl()))
          Result = CallParams.slice(section corresponding to Pack);
      }
    }
  };
  ForwardFinder Finder;
  if (Callee->hasBody())
    return Finder.VisitCallExpr(Callee);  
}

you could also memoize with a map, but I suspect simple recursion is going to be good enough

[upsj]

use an RecursiveASTVisitor instead of ASTMatcher

[upsj]

This almost works now, the only thing that needs to be fixed is isExpandedParameter, which for some reason picks up the first call, but not the second: (Compare ParameterHints.Forwarded and ParameterHints.VariadicPlain)

cpp
void foo(int a);
    template <typename... Args>
    void bar(Args&&... args) { return foo(args...); }
    void baz() {
      int b;
      bar(42); // This parameter is recognized as expanded
      bar(b); // This one doesn't, so its inlay hint is @args: instead of a:
    }

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

281

I am talking about std::forward being a builtin function, which can be detected by its BuiltinID, so it doesn't matter if the signature doesn't match 100%.

[upsj]

Work around the currently broken isExpandedParameter, also fix the broken diff

[upsj]

updated patch baseline

[upsj]

fix iterator invalidation issue, handle UnresolvedLookupExpr and test recursive and split variadic lookup

[upsj]

• add test for emplace-like functions
• fix RecursiveASTVisitor early exit
• fix handling of skipped parameters

[upsj]

attempt to fix the patch issue

[upsj]

clang-tools-extra/clangd/InlayHints.cpp

208

This needs to be fixed, see ParameterHints.VariadicPlain vs. ParameterHints.VariadicForwarded if uncommented - I'd need some input from somebody with more knowledge about the AST

314

There is probably more generic functionality available for this?

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

468

This is not portable, but I don't have access to size_t

[upsj]

It seems like except for the caveats I listed before, all the obvious cases seem to work: make_unique, make_shared, emplace_back with exact type matches. One point that still needs some work is if the parameter needs to be converted inside one of the forwarding functions (probably just needs another unpack inside ForwardingParameterVisitor), as well as a way to remove duplicate parameters that come from recursive templates like std::tuple. One obvious way would be removing inlay hints for duplicate parameters altogether, but that may not be enough/too heuristic? Alternatively, we could inspect the template instantiation pattern.

[nridge]

Had a read through this. I'm still digesting it, but the high-level approach seems reasonable to me.

Could we add a test case for the recursive scenario that came up during chat:

void foo();

template <typename Head, typename... Tail>
void foo(Head head, Tail... tail) {
  foo(tail...);
}

int main() {
  foo(1, 2, 3);
}
`

(even if the behaviour on this testcase isn't what we want yet, it's useful to have it in the test suite as a reminder)

clang-tools-extra/clangd/InlayHints.cpp

344

unwrap? "unpack" sounds like something related to parameter packs

374

I think it would be more readable if the state that persists across calls (TraversedFunctions, ForwardedParams, ParamDeclToDef) would be separate from the state that does not (UnresolvedParams, TraversalQueue).

A concrete suggestion:

• Factor the first set out into a ParameterMappingCache struct
• Do not keep a ForwardingParameterVisitor as a member of InlayHintVisitor, only the ParameterMappingCache
• Create the ForwardingParameterVisitor as a local for each call, and pass it a reference to the ParameterMappingCache in the constructor

[nridge]

Another test case that comes to mind is:

void f1(int a, int b);
void f2(int c, int d);

template <typename... Args>
void foo(Args... args) {
  if (cond) {
    f1(args...);
  } else {
    f2(args...);
  }
}

int main() {
  foo(1, 2);
}

I guess in this case it will use the parameters names from f1, because that's the first call that appears in the function body in traversal order?

I think that's fine, functions written like this are probably not very common. Still good to add to the test suite to document the behaviour.

[nridge]

One more testcase:

template <typename... Args>
void foo(Args...);

template <typename... Args>
void bar(Args... args) {
  foo(args...);
}

template <typename... Args>
void foo(Args... args) {
  bar(args...);
}

int main() {
  foo(1, 2);
}

Sure, this is a stack overflow at runtime, but there's no excuse for it to be one in clangd :)

[nridge]

clang-tools-extra/clangd/InlayHints.cpp

208

It looks like isExpandedParameter() relies on the SubstTemplateTypeParmType type sugar being present in the ParmVarDecl's type, but in some cases, the ParmVarDecl's type points to the canonical type directly.

I'm not sure what sort of guarantees the AST intends to make about the presence of type sugar. Based on past experience with Eclipse CDT, it's very easy to lose type sugar and maintaining it in all the right places takes some effort.

[nridge]

clang-tools-extra/clangd/InlayHints.cpp

208

Upon further investigation, it looks like the ParmVarDecl is retaining the type sugar fine, it's the getNonReferenceType() call in isExpandedParameter() that loses it.

What happens with perfect forwarding when the argument is an lvalue is a bit subtle. In this testcase:

template <typename... Args>
void bar(Args&&... args) { return foo(std::forward<Args>(args)...); }
void baz() {
  int b;
  bar($param[[b]]);
}

the template argument that bar() is instantiated with is Args = [int &]. Substituting into Args&&, that then becomes int& && which collapses into int&, leaving the instantiated parameter type an lvalue reference type.

Clang does in fact model this accurately, which means the type structure is:

BuiltinType
  ReferenceType
    SubstTemplateTypeParmType
      ReferenceType

The outer reference type is the && that's outside the Args, the SubstTemplateTypeParmType reflects the substitution Args = int&, and the inner ReferenceType is the int&.

The problem is, getNonReferenceType() unpacks _all_ the reference types, skipping past the SubstTemplateTypeParmType and giving you the BuiltinType.

[sammccall]

Glad this is working! It looks exciting...

My high level comments are:

• the core "what are the ultimate param vars for this call" function is large and reusable enough that we should give this a public interface in AST.h
• the implementation does a few "extra" things and it's not clear they're worth their complexity cost
• the RecursiveASTVisitor is a bit of a "god object" at the moment, and should probably be split up

(Sorry if the comments below are duplicative)

clang-tools-extra/clangd/InlayHints.cpp

189

nit: isPackExpandedParameter
(usually "expanded" means macro expansion)

197

I think this visitor has too many responsibilities:

• it controls the lifetime of caches for reuse of partial results. (FWIW, I'm skeptical there's any serious benefit of this on real code, and would prefer to avoid paying any complexity costs for it)
• it begins a traversal over function bodies, hooking interesting nodes to find forwarding calls and analyzing them
• it owns the queues that are used for recursive resolution. (Again, it's unclear if these are providing value, but it's hard to separate them out from the design).
• (via inheritance) it implements the mechanics of traversal itself

Could you try to reduce the scope of the visitor down to its very minimum: the inherited implementation of traversal + recording of forwarding calls?

e.g.

// Traverses a function body, recording locations where a particular
// parameter pack was forwarded to another call.
class FindForwardingCalls : public RecursiveASTVisitor {
  FindForwardingCalls(ParmVarDecl Pack);

  struct ForwardingCall {
    FunctionDecl *Callee;
    unsigned PackOffset; // e.g. 0 for make_unique, 1 for map::try_emplace
  };

  vector<pair<FunctionDecl*, ArrayRef<Expr*>> ForwardingCalls;
};

Then the other pieces can be decoupled and structured in ways that make the most sense individually.

201

this is only called once, and the Params is always Callee->params().

Can this be a function instead that takes only Callee and returns the params?

If possible, I'd declare this function in AST.h and hide the RecursiveASTVisitor in AST.cpp.

208

Ah, great catch.

I think it's fine to assume that ParmVarDecls in particular will carry their sugar. It's more the types of expressions where these things get lost.

I think for our purposes we can assume that the *only* sugar present is the &&, and you can simply do

const Type PlainType* = Param->getType().getTypePtr();
if (auto *RT = dyn_cast<ReferenceType>(PlainType)) // not get! no desugaring
  PlainType = RT;
if (auto *SubstType = ...)

261

Unless I'm missing something, going looking in the redecls of the function for a parameter name doesn't seem in scope for this patch.

We don't support it in inlay hints elsewhere, and it's not clear it has anything to do with forwarding functions.
Maybe the added complexity is justifiable if this logic can be shared with different functions (hover, signature help) but I don't think it belongs in this patch.

286

The data flow here is uncomfortably implicit.

We're traversing a bunch of functions according to logic A, populating a big map, and then querying the map with logic B, and hoping/asserting that A provides all the information that B needs.
The contract of these void handleXXX functions is pretty vague.

I think it would be clearer if these functions were asking questions.
We got here by investigating the ParmVarDecls associated with a *particular* pack.
So if we were more direct here, we'd verify that the param here is actually tied to *that* pack, and reporting the call (i.e. this function should return Optional<ForwardingCall> for some struct we define, and not also worrying about recursion).

303

(this looks like the wrong place to test that the function hasn't been visited yet: it would be possible to enqueue the same callee multiple times)

355

this seems like an unnecessarily clever optimization, and I can't really imagine a situation where it might matter.
(This whole analysis only lives for a single main-file call, so this only kicks in if multiple arguments eventually end up forwarded to the same parameter!)

I think this can be simplified to:

while (const ParamVarDecl *F = ForwardedParams.lookup(Param))
  Param = F;

and inlined into its caller

374

I agree that these are too tangled, but I wouldn't think there's a strong performance reason for having a cache here.

If the storage is needed for correctness, I think we should give it the minimum lifetime needed for correctness, and try to express the algorithm more directly if possible (e.g. recursion rather than a work queue).
If the storage is for performance, can we give some intuitive explanation of what is being reused and how many times in common cases?

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

281

Ah, right!
Maybe say "This prototype of std::forward is sufficient for clang to recognize it"?

468

Hmm, I think you can use using size_t = decltype(sizeof(0))?

[upsj]

I will update this to split up the control flow into a visitor just collecting a single recursion level and the high-level mapping. A general question here would be whether we should do the resolution by parameter or by parameter pack. Doing it by parameter (like I did it right now) is much easier for inlay hints, especially if the pack gets split up into different parts, but that may not necessarily be the case for template functions and more fuzzy cases like signature help/autocomplete.

clang-tools-extra/clangd/InlayHints.cpp

261

This was already functionality previously available in chooseParameterNames, I thought I would need to do the same thing here, but turns out that I can get the FunctionDecl from a ParmVarDecl, so this can stay in its previous place.

[sammccall]

In D124690#3530272, @upsj wrote:

I will update this to split up the control flow into a visitor just collecting a single recursion level and the high-level mapping. A general question here would be whether we should do the resolution by parameter or by parameter pack. Doing it by parameter (like I did it right now) is much easier for inlay hints, especially if the pack gets split up into different parts, but that may not necessarily be the case for template functions and more fuzzy cases like signature help/autocomplete.

I'm not sure I have perfectly grasped the distinction you're drawing, but IIUC the dilemma:

• we could query separately where each arg is forwarded to. For multiple forwards, the recursive step is "what param is arg N of this callee ultimately bound to"
• we could query where a range of args is forwarded to. The recursive step is "what params are args [N-M] of this callee ultimately bound to"

Seems like the first is simpler but the second is more efficient. The second also more clearly ensures we don't give inconsistent results for adjacent parameters.
The approach you take now where you walk->cache->query does manage to combine the best of both (though I find it complex in other ways).
This is a tricky tradeoff. My guess is that it's fine to start with the simple param-by-param query, and leave a FIXME on the recursive function that querying a range at a time would be more efficient for large packs.

clang-tools-extra/clangd/InlayHints.cpp

261

You're right, sorry!
I think the the version we have is pretty opportunistic: this is a couple of lines of code, so why not?
I don't think we should aim for parity with it, but rather just handle whatever cases are both useful & trivial to implement (in this patch)

I can get the FunctionDecl from a ParmVarDecl, so this can stay in its previous place.

This sounds good to me - means we can drop this map right? That was the main piece I was concerned with.

314

Not AFAIK. There's a bunch of rich info about overload sets and their quality, but it's discarded after Sema and not preserved in the AST I think.

[upsj]

Yes, I think that's a good summary. Only a small clarification:

we could query separately where each arg is forwarded to. For multiple forwards, the recursive step is "what param is arg N of this callee ultimately bound to"

I was thinking of implementing this as "what parameters are the following arguments bound to in a direct call", which doesn't have the aforementioned efficiency issues, and is basically a single recursion level of what I had previously implemented. Since (at least afaik) there is no way to have a parameter pack split up between different function calls (at least from the AST standpoint where we here only allow std::forward or plain parameters), the input would be a FunctionDecl and vector/set of ParmVarDecl and the output would be a map ParmVarDecl -> ParmVarDecl plus an Optional<FunctionDecl> telling us whether we need to recurse further. Unrolling the recursion would also make it easier to limit the depth to which we unpack forwarded parameters.

clang-tools-extra/clangd/InlayHints.cpp

261

Yes, we can do the same in the InlayHintVisitor, though it becomes a bit more complex there, since we don't have the index of the argument, which might require a linear search, as different forwarded ParmVarDecls can come from different FunctionDecls

[upsj]

I've hit a roadblock I'm not sure how to proceed with. It seems that in some cases, a ParmVarDecl of an instantiated function template indeed doesn't preserve the SubstTemplateTypeParmType type sugar allowing me to check which template parameter it was instantiated from. This might be related to reference collapsing on type deduction, since Args... preserves the sugar, but Args&&... doesn't. Without this ability to distinguish between expanded packs and normal parameters, I risk recursing into other functions and pulling up unrelated type names. Any ideas how to proceed here? Essentially, it's the same issue that @nridge already provided a partial solution for by not using getNonReferenceType(), but more complex. I would really like to avoid having to reconstruct parameter indices from arbitrarily many levels of template parameters packs from surrounding scopes.

Example where this pops up:

cpp
    namespace std { template <typename T> T&& forward(T&); }
    struct S { S(int a); };
    template <typename T, typename... Args>
    T bar(Args&&... args) { return T{std::forward<Args>($fwd[[args]])...}; }
    void baz() {
      int b;
      bar<S>($param[[b]]);
    }

[nridge]

In D124690#3540704, @upsj wrote:

Example where this pops up:

cpp
    namespace std { template <typename T> T&& forward(T&); }
    struct S { S(int a); };
    template <typename T, typename... Args>
    T bar(Args&&... args) { return T{std::forward<Args>($fwd[[args]])...}; }
    void baz() {
      int b;
      bar<S>($param[[b]]);
    }

I'm happy to take a look at this, but I would need some updated code to run (including in particular the update to the call site that currently uses getNonReferenceType())

[upsj]

Of course, I forgot to create a new diff. There is some debug output and logic errors for some of the tests, but the simple ones already show the issue.
The function in question is getPackTemplateParameter, which provides similar functionality to isExpandedParameter before

[nridge]

Haven't had a chance to try it yet, but based on a quick glance, my suspicion is that the problem is the use of ReferenceType::getPointeeType(), which may do more unwrapping than we want (its implementation contains a loop).

I would try using getPointeeTypeAsWritten() instead and see if that helps.

[nridge]

In D124690#3542961, @nridge wrote:

I would try using getPointeeTypeAsWritten() instead and see if that helps.

Indeed, this change makes two tests (VariadicPlainNewConstructor and VariadicForwardedNewConstructor) pass. I assume the other failures are due to other issues in the patch.

[upsj]

Thanks Nathan, that was exactly it. I thought I looked at getTypePtr() directly, but I must have been mistaken.
After a small fix (only adding inlay hints until the first unexpanded pack expression, it seems to work now).

Short explanation of what is going on: I split the parameters into head, pack and tail where pack are all parameters matching the variadic template type parameter we are expanding.
This way, I need to make no allocations inside the visitor.

The only thing to be fixed is a make_tuple-like call, where each parameter is called head, probably simple deduplication makes most sense.

[upsj]

• fix varargs, again :)
• remove parameter names entirely if they occur multiple times

[upsj]

This is now ready to review, only needed to fix a formatting issue

[upsj]

add test for varargs function called from forwarding function

[upsj]

clang-tools-extra/clangd/AST.cpp

775

Similar to processCall in InlayHints.cpp, this may have issues with varargs functions. Maybe on top of checking for unexpanded pack expression arguments, I should add a check Callee->getNumParams() == Args.size(). A corresponding test fails by not forwarding a fixed parameter right now.

[upsj]

@nridge @sammccall can you give this another look?

[nridge]

I will do my best to take a look this weekend. Your patience is appreciated :)

[nridge]

Will continue looking at the implementation tomorrow, for now a few minor comments and a suggested variation on a testcase.

clang-tools-extra/clangd/AST.h

19

nit: this include seems unnecessary given the code being added

209

This function does something a bit more specific than what this description suggests: given a type parameter pack typename... T, it will return true for function parameters instantiated from a parameter pack of type T... or T&... or T&&..., but not T*... or vector<T>... or anything else.

Suggested description:

/// Checks whether D is instantiated from a function parameter pack
/// whose type is a bare type parameter pack (e.g. `Args...`), or a 
/// reference to one (e.g. `Args&...` or `Args&&...`).

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

200

As an aside, & does not seem like a useful hint to show for std::forward -- should we try to omit it? (We don't need to do it in this patch as it's not really related.)

483

I think a variation of this test case where foo is also variadic, would be valuable to add:

template <typename... Args>
void foo(int fixed, Args&&... args);

template <typename... Args>
void bar(Args&&... args) {
  foo(args...);
}

void baz() { 
  bar($fixed[[41]], 42, 43);
}

This case does seem to already work with the current patch, and I think it's the more common case; I'm happy to defer the varargs one as a less important edge case.

[nridge]

Finished looking through the patch; I found it nicely organized and fairly easy to understand (as easy as code involving the analysis of C++ variadic templates can be, anyways :-D)!

clang-tools-extra/clangd/AST.cpp

685

I don't think there is any requirement that a pack be a trailing template parameter. For example, the following is valid:

template <typename... B, typename A>
void foo(A, B...);

void bar() {
  foo(1, 2, 3);
}

764

These names (FullyResolved, PartiallyResolved, NextTarget) sound like they might be leftovers from a previous implementation?

834

nit: D is an odd name for a CallExpr, maybe Call or E?

891

can just capture TTPT here

902

The second argument is presumably meant to be Tail.rend()

clang-tools-extra/clangd/InlayHints.cpp

562

let's add // will not be hinted for clarity

584

This is an interesting approach for handling VariadicRecursive.

I had in mind a different approach, something like keeping a std::set<FunctionTemplateDecl*> SeenFunctionTemplates in resolveForwardingParameters(), populating it with CurrentFunction->getPrimaryTemplate() on each iteration, and bailing if the same function template is seen more than once (indicating recursion). But this approach seems to work too, as a parameter name can't legitimately appear twice in a function declaration.

That said, maybe having such a SeenFunctionTemplates recursion guard would be helpful anyways, so that e.g. in VariadicInfinite, we would bail after a single recursion rather than going until MaxDepth?

[upsj]

• improve documentation
• add more edge case tests
• fix reference hints for parameter packs
• remove unnecessary headers
• fix bug in handling of tail parameters

[upsj]

clang-tools-extra/clangd/AST.cpp

685

Do you have a suggestion for how to find this pack in general? I would like to keep this function as efficient as possible, since it's used everywhere

clang-tools-extra/clangd/InlayHints.cpp

584

I see your point here - I would also like an AST based approach more than this purely string-based one. The main issue is that if I deduplicate based on the function templates, I would still be left with the first parameter being named, which doesn't make much sense in something like make_tuple.

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

200

see https://reviews.llvm.org/D127859 ;)

483

The main reason I added this is to test that the visitor doesn't break on varargs, the output is not that important anyways. I added your suggestion as well, which highlighted another issue, thanks :)

[nridge]

Thanks for the update and the additional test cases!

clang-tools-extra/clangd/AST.cpp

685

Do you have a suggestion for how to find this pack in general?

Just iterate backwards through TemplateParams rather than only considering TemplateParams.back(), I suppose.

I would like to keep this function as efficient as possible, since it's used everywhere

The ParmVarDecl* overload of getPackTemplateParameter() is called a lot via the IsExpandedPack lambdas, but this overload is only called once per depth level.

clang-tools-extra/clangd/InlayHints.cpp

584

One idea is that we could return the original Parameters from resolveFowardingParameters() if we encounter recursion.

[upsj]

• remove inlay hints from std::forward in tests
• identify recursive variadic calls from template, not post-processing
• allow template parameter packs to appear at any place
• improve documentation

clang-tools-extra/clangd/AST.cpp

685

Ah thanks, of course! I got tricked by my own overloads.

[nridge]

Thanks -- the patch looks quite good to me now!

I will defer to @sammccall for final approval in case he has any additional feedback.

[upsj]

can you give this another look, if you have some time @sammccall?

[sammccall]

Sorry about the delay. This is complicated stuff and every time I put it down for a day I forget how it works!
Thanks for your hard work here.

I have some more suggestions but hopefully fairly mechanical.
Feel free to land once you're done, or send it back over to Nathan or I to commit.

clang-tools-extra/clangd/AST.cpp

677

nit: this comment doesn't really say anything that the method name doesn't say already.

Maybe replace with an example like "returns true for X in template <class...X> int m;"

680

TTPD->isParameterPack() does the same thing with less code

689

it seems confusing and unneccesary to use the same name for the two versions of this function. The name applies to both, but they don't really do the same thing.

Maybe call this one getFunctionPackType and the other getUnderylingPackType?
(not get*TemplateParameter as they return a type, not the parameter decl)

690

This is doing something pretty strange if Callee is a function template specialization.

It's not clear to me whether this function should be handling that case (which AFAICS it doesn't, but could inspect the specialization kind), or whether resolveForwardingParameters is responsible for not calling this function in that case (in which case we should probably have an assert here).

Can you also add a test case that function template specialization doesn't confuse us? i.e. it should return the parmvardecls from the specialization's definition.

720

This class could use a high-level comment explaining what it does.

e.g.

This visitor walks over the body of an instantiated function template.
The template accepts a parameter pack and the visitor records whether
the pack parameters were forwarded to another call. For example, given:

template <class T, class...Args>
auto make_unique(Args..args) {
  return unique_ptr<T>(new T(args...));
}

When called as `make_unique<std::string>(2, 'x')` this yields a function
`make_unique<std::string, int, char>` with two parameters.
The visitor records that those two parameters are forwarded to the
`constructor std::string(int, char);`.

This information is recorded in the `ForwardingInfo` (in general,
more complicated scenarios are also possible).

clang-tools-extra/clangd/AST.h

212

nit: I think isExpandedFromParameterPack might be clearer, up to you

clang-tools-extra/clangd/InlayHints.cpp

398

I can't understand what this comment is saying, can you rephrase it or provide an example?

From looking at the code, I'm guessing something like:
"If we're passing a parameter pack into this call, we need to give up matching arguments to parameters at that point as we don't know how long it is".

---

I think the interaction between getExpandedArgCount and chooseParameterNames is unclear and brittle here. (i.e. the invariant that getExpandedArgCount(Args).size() < chooseParameterNames(Params).size() is very non-local)

I'd suggest writing this more directly as:

for (I = 0; I < ParameterNames.size() && I < Args.size(); ++I) {
  // ... explanation ...
  if (isa<PackExpansionExpr>(Args[I]))
    break;

  // ... generate param hint ...
}

510

why is this check needed if we already decline to provide a name for the parameter on line 534 in chooseParameterNames?

562

say why, not what

// If we haven't resolved a pack paramater (e.g. foo(Args... args)) then
// hinting as foo(args: 1, args: 2, args: 3) is unlikely to be useful.

569

factor out a function getParamDefinition?

600–604

the name of this function is very confusing - even after understanding what it does, I can't understand how that corresponds to the name.

However as noted above I don't think we need this function returning an index at all, instead we can just check the condition while looping.

605

isa<PackExpansionExpr>(E)

[nridge]

clang-tools-extra/clangd/InlayHints.cpp

510

shouldHintName and shouldHintReference are two independent conditions governing whether we show the parameter name and/or a & indicating pass-by-mutable-ref, respectively

(I did approve the patch that introduced shouldHintReference myself, hope that's ok)

[sammccall]

clang-tools-extra/clangd/InlayHints.cpp

482–505

The new condition is nonobvious, but the new comment just echoes the code.

Instead add a second sentence explaining *why*.
(I can't provide a suggestion, because I don't understand why)

510

Thanks, that makes sense! I just hadn't understood that change.

510

What exactly *is* the motivation for suppressing reference hints in the pack case?

(I can imagine there are cases where they're annoying, but it's hard to know if the condition is right without knowing what those are)

[upsj]

• simplify parameter pack detection
• improve function naming
• make handling of unexpanded packs and varargs more visible
• add tests involving template specializations
• make documentation more descriptive

clang-tools-extra/clangd/AST.cpp

690

Do the new tests VariadicNameFromSpecialization(Recursive)? match what you had in mind?

clang-tools-extra/clangd/InlayHints.cpp

510

I added an explanation. Basically, if we are unable to figure out which parameter the arguments are being forwarded to, the type of the ParmVarDecl for Args&&... gets deduced as T& or T&&, so that would mean even though we don't know whether the argument will eventually be forwarded to a reference parameter, we still claim all mutable lvalue arguments will be mutated, which IMO introduces more noise than necessary. But I think there are also good arguments for adding them to be safe.

There is another detail here, which is that we don't record whether we used std::forward, so the corresponding rvalue-to-lvalue conversions may lead to some unnecessary & annotations for rvalue arguments.

[sammccall]

Thanks for the readability improvements! I've forgotten if you have commit access?

This stuff is complicated and I'm definitely going to forget the reasoning, the intuitive explanations are going to help a lot if changes are needed in future.
(Maybe it's just me, but the formal language around instantiations, deductions, etc makes my eyes glaze over - as if it's the words themselves rather than how you use them!)

clang-tools-extra/clangd/AST.cpp

690

Yes, that's fantastic, thank you!

clang-tools-extra/clangd/InlayHints.cpp

510

This makes sense, the comment explains well, thank you!
I have a couple of quibbles, up to you whether to change the logic.

#1: There's an unstated assumption that pack arguments *will* be forwarded (there are other things we can do with them, like use them in fold-expressions). It's a pretty good assumption but if the comment talks about forwarding, it should probably mention explicitly ("it's likely the params will be somehow forwarded, and...")

#2: the idea is that if the reference-ness is deduced from the callsite, then it's not meaningful as an "is the param modified" signal, it's just "is this arg modifiable". Fair enough, but this is a property of universal/forwarding references (T&& where T is a template param), not of packs. So I *think* this check should rather be !isInstantiatedFromForwardingReference(Param).
But maybe that's more complexity and what you have is a good heuristic - I think at least we should call out that it's a heuristic for the true condition.

[upsj]

yes, I have commit access

clang-tools-extra/clangd/InlayHints.cpp

510

#1: I agree, I'll make that more clear before committing.

#2: Now that I think about it, there are actually two things we don't keep track of: parameters could lose their reference-ness via Args... instead of Args&&... and their rvalue-ness by not using std::forward. We only look at whether the innermost call takes a reference parameter, but as I said, we may lose some of that information on the way, claiming the function may modify the argument when it actually creates a copy on the way (losing reference-ness). I think the case of an rvalue being mistaken for an lvalue should not be much of an issue, since the reference annotation almost makes sense.

To visualize the situation: These three snippets all add &: hints to the parameter of bar

void foo(int&);
template <typename... Args>
void bar(Args... args) { return foo(args...); }
void baz() {
  bar(1);
}

void foo(int&);
template <typename... Args>
void bar(Args&&... args) { return foo(args...); }
void baz() {
  bar(1);
}

void foo(int&);
template <typename... Args>
void bar(Args&&... args) { return foo(std::forward<Args>(args)...); }
void baz() {
  int a;
  bar(a);
}

Two of these three cases probably shouldn't have this annotation?

[sammccall]

clang-tools-extra/clangd/InlayHints.cpp

510

parameters could lose their reference-ness via Args... instead of Args&&...

(I'm not quite following what you mean here: if we deduce as Args rather than Args&& then the parameters are not references in the first place, we're passing by value)

and their rvalue-ness by not using std::forward

Yes. Fundamentally if we're deducing the ref type then we should be looking for a concrete signal of how the value is ultimately used, which involves tracking casts like std::forward. This is true whether it's a pack or not.
It's a bunch of work and I'm not sure it's worth it (and I'm certainly not asking you to add it!)

Two of these three cases probably shouldn't have this annotation?

Yes. Claiming we're passing a numeric literal (prvalue) by mutable reference doesn't pass the laugh test.

We only look at whether the innermost call takes a reference parameter, but as I said, we may lose some of that information on the way, claiming the function may modify the argument when it actually creates a copy on the way (losing reference-ness)

Right, there can be copies on the way for various reasons (passing by value, but also e.g. forwarding a T as a const U& where U is constructible from T).

I'm starting to think the simplest answer for now is never to include & if there's a forwarding T&& reference involved, as it's not always clear what it means and it's hard to do reliably.

This means we're accepting that make_unique<Foo>(RefParamToConstructor) will lack its &. (It already does today - this patch would need extensions to do this robustly).

I think we only need to check the outer call in the usual way, not any inner forwarded calls:

• if we're passing as (deduced) T then that's by value and no & is needed
• if we're passing as (deduced) T& then that's explicitly by reference (will only bind to a mutable lvalue) and & is needed
• if we're passing as (deduced) T&& then we're going to conservatively not include the &

I don't think packs need to be involved in this logic at all.

[upsj]

detect whether forwarding functions preserve reference-ness and value categories of their arguments

[upsj]

clang-tools-extra/clangd/InlayHints.cpp

510

I think I found a suitable proxy for figuring out whether the function uses Args&&... and/or std::forward, can you take a final look at shouldHintReference whether this makes sense to you or I may have missed any important edge cases? I also added a few tests for this behavior (VariadicReferenceHint...) that should show that it works.

[upsj]

don't add reference hints to unresolved parameter packs

[upsj]

@nridge @sammccall if you don't have any further comments, I will commit this tomorrow, and maybe think about how to extend it to signature help/code completion :)

clang-tools-extra/clangd/AST.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_AST_H_
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_AST_H_
 
 #include "index/SymbolID.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/NestedNameSpecifier.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Lex/MacroInfo.h"
 #include "llvm/ADT/StringRef.h"
 #include <string>
 #include <vector>
 
 namespace clang {

 bool hasUnstableLinkage(const Decl *D);
 
 /// Checks whether \p D is more than \p MaxDepth away from translation unit
 /// scope.
 /// This is useful for limiting traversals to keep operation latencies
 /// reasonable.
 bool isDeeplyNested(const Decl *D, unsigned MaxDepth = 10);
 
+/// Recursively resolves the parameters of a FunctionDecl that forwards its
+/// parameters to another function via variadic template parameters. This can
+/// for example be used to retrieve the constructor parameter ParmVarDecl for a
+/// make_unique or emplace_back call.
+llvm::SmallVector<const ParmVarDecl *>
+resolveForwardingParameters(const FunctionDecl *D, unsigned MaxDepth = 10);
+
+/// Checks whether D is instantiated from a function parameter pack
+/// whose type is a bare type parameter pack (e.g. `Args...`), or a
+/// reference to one (e.g. `Args&...` or `Args&&...`).
+bool isExpandedFromParameterPack(const ParmVarDecl *D);
+
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_AST_H_

clang-tools-extra/clangd/AST.cpp

 #include "SourceCode.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTTypeTraits.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/DeclarationName.h"
+#include "clang/AST/ExprCXX.h"
 #include "clang/AST/NestedNameSpecifier.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/Stmt.h"
 #include "clang/AST/TemplateBase.h"
 #include "clang/AST/TypeLoc.h"
+#include "clang/Basic/Builtins.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/Specifiers.h"
 #include "clang/Index/USRGeneration.h"
 #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 <string>
 #include <vector>
 
 namespace clang {
 namespace clangd {
bool isDeeplyNested(const Decl *D, unsigned MaxDepth) {
   size_t ContextDepth = 0;
   for (auto *Ctx = D->getDeclContext(); Ctx && !Ctx->isTranslationUnit();
        Ctx = Ctx->getParent()) {
     if (++ContextDepth == MaxDepth)
       return true;
   }
   return false;
 }
+
+namespace {
+
+// returns true for `X` in `template <typename... X> void foo()`
+bool isTemplateTypeParameterPack(NamedDecl *D) {
+  if (const auto *TTPD = dyn_cast<TemplateTypeParmDecl>(D)) {
+    return TTPD->isParameterPack();
+  }
+  return false;
+}
+
+// Returns the template parameter pack type from an instantiated function
+// template, if it exists, nullptr otherwise.
+const TemplateTypeParmType *getFunctionPackType(const FunctionDecl *Callee) {
+  if (const auto *TemplateDecl = Callee->getPrimaryTemplate()) {
+    auto TemplateParams = TemplateDecl->getTemplateParameters()->asArray();
+    // find the template parameter pack from the back
+    const auto It = std::find_if(TemplateParams.rbegin(), TemplateParams.rend(),
+                                 isTemplateTypeParameterPack);
+    if (It != TemplateParams.rend()) {
+      const auto *TTPD = dyn_cast<TemplateTypeParmDecl>(*It);
+      return TTPD->getTypeForDecl()->castAs<TemplateTypeParmType>();
+    }
+  }
+  return nullptr;
+}
+
+// Returns the template parameter pack type that this parameter was expanded
+// from (if in the Args... or Args&... or Args&&... form), if this is the case,
+// nullptr otherwise.
+const TemplateTypeParmType *getUnderylingPackType(const ParmVarDecl *Param) {
+  const auto *PlainType = Param->getType().getTypePtr();
+  if (auto *RT = dyn_cast<ReferenceType>(PlainType))
+    PlainType = RT->getPointeeTypeAsWritten().getTypePtr();
+  if (const auto *SubstType = dyn_cast<SubstTemplateTypeParmType>(PlainType)) {
+    const auto *ReplacedParameter = SubstType->getReplacedParameter();
+    if (ReplacedParameter->isParameterPack()) {
+      return dyn_cast<TemplateTypeParmType>(
+          ReplacedParameter->getCanonicalTypeUnqualified()->getTypePtr());
+    }
+  }
+  return nullptr;
+}
+
+// This visitor walks over the body of an instantiated function template.
+// The template accepts a parameter pack and the visitor records whether
+// the pack parameters were forwarded to another call. For example, given:
+//
+// template <typename T, typename... Args>
+// auto make_unique(Args... args) {
+//   return unique_ptr<T>(new T(args...));
+// }
+//
+// When called as `make_unique<std::string>(2, 'x')` this yields a function
+// `make_unique<std::string, int, char>` with two parameters.
+// The visitor records that those two parameters are forwarded to the
+// `constructor std::string(int, char);`.
+//
+// This information is recorded in the `ForwardingInfo` split into fully
+// resolved parameters (passed as argument to a parameter that is not an
+// expanded template type parameter pack) and forwarding parameters (passed to a
+// parameter that is an expanded template type parameter pack).
+class ForwardingCallVisitor
+    : public RecursiveASTVisitor<ForwardingCallVisitor> {
+public:
+  ForwardingCallVisitor(ArrayRef<const ParmVarDecl *> Parameters)
+      : Parameters{Parameters}, PackType{getUnderylingPackType(
+                                    Parameters.front())} {}
+
+  bool VisitCallExpr(CallExpr *E) {
+    auto *Callee = getCalleeDeclOrUniqueOverload(E);
+    if (Callee) {
+      handleCall(Callee, E->arguments());
+    }
+    return !Info.hasValue();
+  }
+
+  bool VisitCXXConstructExpr(CXXConstructExpr *E) {
+    auto *Callee = E->getConstructor();
+    if (Callee) {
+      handleCall(Callee, E->arguments());
+    }
+    return !Info.hasValue();
+  }
+
+  // The expanded parameter pack to be resolved
+  ArrayRef<const ParmVarDecl *> Parameters;
+  // The type of the parameter pack
+  const TemplateTypeParmType *PackType;
+
+  struct ForwardingInfo {
+    // If the parameters were resolved to another FunctionDecl, these are its
+    // first non-variadic parameters (i.e. the first entries of the parameter
+    // pack that are passed as arguments bound to a non-pack parameter.)
+    ArrayRef<const ParmVarDecl *> Head;
+    // If the parameters were resolved to another FunctionDecl, these are its
+    // variadic parameters (i.e. the entries of the parameter pack that are
+    // passed as arguments bound to a pack parameter.)
+    ArrayRef<const ParmVarDecl *> Pack;
+    // If the parameters were resolved to another FunctionDecl, these are its
+    // last non-variadic parameters (i.e. the last entries of the parameter pack
+    // that are passed as arguments bound to a non-pack parameter.)
+    ArrayRef<const ParmVarDecl *> Tail;
+    // If the parameters were resolved to another forwarding FunctionDecl, this
+    // is it.
+    Optional<FunctionDecl *> PackTarget;
+  };
+
+  // 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) {
+    if (std::any_of(Args.begin(), Args.end(), [](const Expr *E) {
+          return dyn_cast<PackExpansionExpr>(E) != nullptr;
+        })) {
+      return;
+    }
+    auto OptPackLocation = findPack(Args);
+    if (OptPackLocation) {
+      size_t PackLocation = OptPackLocation.getValue();
+      ArrayRef<ParmVarDecl *> MatchingParams =
+          Callee->parameters().slice(PackLocation, Parameters.size());
+      // Check whether the function has a parameter pack as the last template
+      // parameter
+      if (const auto *TTPT = getFunctionPackType(Callee)) {
+        // In this case: Separate the parameters into head, pack and tail
+        auto IsExpandedPack = [&](const ParmVarDecl *P) {
+          return getUnderylingPackType(P) == TTPT;
+        };
+        ForwardingInfo FI;
+        FI.Head = MatchingParams.take_until(IsExpandedPack);
+        FI.Pack = MatchingParams.drop_front(FI.Head.size())
+                      .take_while(IsExpandedPack);
+        FI.Tail = MatchingParams.drop_front(FI.Head.size() + FI.Pack.size());
+        FI.PackTarget = Callee;
+        Info = FI;
+        return;
+      }
+      // Default case: assume all parameters were fully resolved
+      ForwardingInfo FI;
+      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
+    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;
+    }
+    return std::distance(Args.begin(), FirstMatch);
+  }
+
+  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);
+      }
+    }
+    // Ignore the callee if the number of arguments is wrong (deal with va_args)
+    if (Callee->getNumParams() == E->getNumArgs())
+      return Callee;
+    return nullptr;
+  }
+
+  static FunctionDecl *resolveOverload(UnresolvedLookupExpr *Lookup,
+                                       CallExpr *E) {
+    FunctionDecl *MatchingDecl = nullptr;
+    if (!Lookup->requiresADL()) {
+      // Check whether there is a single overload with this number of
+      // parameters
+      for (auto *Candidate : Lookup->decls()) {
+        if (auto *FuncCandidate = dyn_cast_or_null<FunctionDecl>(Candidate)) {
+          if (FuncCandidate->getNumParams() == E->getNumArgs()) {
+            if (MatchingDecl) {
+              // there are multiple candidates - abort
+              return nullptr;
+            }
+            MatchingDecl = FuncCandidate;
+          }
+        }
+      }
+    }
+    return MatchingDecl;
+  }
+
+  // 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
+  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 std::forward(DeclRefExpr) to DeclRefExpr, removing any intermediate
+  // implicit casts, nullptr otherwise
+  static const DeclRefExpr *unwrapArgument(const Expr *E) {
+    E = unwrapImplicitCast(E);
+    E = unwrapForward(E);
+    E = unwrapImplicitCast(E);
+    return dyn_cast<DeclRefExpr>(E);
+  }
+};
+
+} // namespace
+
+SmallVector<const ParmVarDecl *>
+resolveForwardingParameters(const FunctionDecl *D, unsigned MaxDepth) {
+  auto Parameters = D->parameters();
+  // If the function has a template parameter pack
+  if (const auto *TTPT = getFunctionPackType(D)) {
+    // Split the parameters into head, pack and tail
+    auto IsExpandedPack = [TTPT](const ParmVarDecl *P) {
+      return getUnderylingPackType(P) == TTPT;
+    };
+    ArrayRef<const ParmVarDecl *> Head = Parameters.take_until(IsExpandedPack);
+    ArrayRef<const ParmVarDecl *> Pack =
+        Parameters.drop_front(Head.size()).take_while(IsExpandedPack);
+    ArrayRef<const ParmVarDecl *> Tail =
+        Parameters.drop_front(Head.size() + Pack.size());
+    SmallVector<const ParmVarDecl *> Result(Parameters.size());
+    // Fill in non-pack parameters
+    auto HeadIt = std::copy(Head.begin(), Head.end(), Result.begin());
+    auto TailIt = std::copy(Tail.rbegin(), Tail.rend(), Result.rbegin());
+    // Recurse on pack parameters
+    size_t Depth = 0;
+    const FunctionDecl *CurrentFunction = D;
+    llvm::SmallSet<const FunctionTemplateDecl *, 4> SeenTemplates;
+    if (const auto *Template = D->getPrimaryTemplate()) {
+      SeenTemplates.insert(Template);
+    }
+    while (!Pack.empty() && CurrentFunction && Depth < MaxDepth) {
+      // Find call expressions involving the pack
+      ForwardingCallVisitor V{Pack};
+      V.TraverseStmt(CurrentFunction->getBody());
+      if (!V.Info) {
+        break;
+      }
+      // If we found something: Fill in non-pack parameters
+      auto Info = V.Info.getValue();
+      HeadIt = std::copy(Info.Head.begin(), Info.Head.end(), HeadIt);
+      TailIt = std::copy(Info.Tail.rbegin(), Info.Tail.rend(), TailIt);
+      // Prepare next recursion level
+      Pack = Info.Pack;
+      CurrentFunction = Info.PackTarget.getValueOr(nullptr);
+      Depth++;
+      // If we are recursing into a previously encountered function: Abort
+      if (CurrentFunction) {
+        if (const auto *Template = CurrentFunction->getPrimaryTemplate()) {
+          bool NewFunction = SeenTemplates.insert(Template).second;
+          if (!NewFunction) {
+            return {Parameters.begin(), Parameters.end()};
+          }
+        }
+      }
+    }
+    // Fill in the remaining unresolved pack parameters
+    HeadIt = std::copy(Pack.begin(), Pack.end(), HeadIt);
+    assert(TailIt.base() == HeadIt);
+    return Result;
+  }
+  return {Parameters.begin(), Parameters.end()};
+}
+
+bool isExpandedFromParameterPack(const ParmVarDecl *D) {
+  return getUnderylingPackType(D) != nullptr;
+}
+
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/InlayHints.cpp

getDesignators(const InitListExpr *Syn) {
   // We use their SourceLocation to correlate with the syntactic form later.
   llvm::DenseMap<SourceLocation, std::string> Designators;
   std::string EmptyPrefix;
   collectDesignators(Syn->isSemanticForm() ? Syn : Syn->getSemanticForm(),
                      Designators, NestedBraces, EmptyPrefix);
   return Designators;
 }
 
 class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
 public:
   InlayHintVisitor(std::vector<InlayHint> &Results, ParsedAST &AST,
                    const Config &Cfg, llvm::Optional<Range> RestrictRange)
       : Results(Results), AST(AST.getASTContext()), Cfg(Cfg),
         RestrictRange(std::move(RestrictRange)),
         MainFileID(AST.getSourceManager().getMainFileID()),
         Resolver(AST.getHeuristicResolver()),
         TypeHintPolicy(this->AST.getPrintingPolicy()),
         StructuredBindingPolicy(this->AST.getPrintingPolicy()) {
     bool Invalid = false;
     llvm::StringRef Buf =
         AST.getSourceManager().getBufferData(MainFileID, &Invalid);
     MainFileBuf = Invalid ? StringRef{} : Buf;
 
     TypeHintPolicy.SuppressScope = true; // keep type names short
     TypeHintPolicy.AnonymousTagLocations =
         false; // do not print lambda locations
 
     // For structured bindings, print canonical types. This is important because
     // for bindings that use the tuple_element protocol, the non-canonical types
     // would be "tuple_element<I, A>::type".
     // For "auto", we often prefer sugared types.
     // Not setting PrintCanonicalTypes for "auto" allows
     // SuppressDefaultTemplateArgs (set by default) to have an effect.
     StructuredBindingPolicy = TypeHintPolicy;
     StructuredBindingPolicy.PrintCanonicalTypes = true;
   }
if (!Callee)
       return true;
 
     processCall(E->getRParenLoc(), Callee, {E->getArgs(), E->getNumArgs()});
     return true;
   }
 
   bool VisitFunctionDecl(FunctionDecl *D) {
     if (auto *FPT =
             llvm::dyn_cast<FunctionProtoType>(D->getType().getTypePtr())) {
       if (!FPT->hasTrailingReturn()) {
         if (auto FTL = D->getFunctionTypeLoc())
           addReturnTypeHint(D, FTL.getRParenLoc());
       }
     }
     return true;
   }
 
   bool VisitLambdaExpr(LambdaExpr *E) {
     FunctionDecl *D = E->getCallOperator();
     if (!E->hasExplicitResultType())
       addReturnTypeHint(D, E->hasExplicitParameters()
                                ? D->getFunctionTypeLoc().getRParenLoc()
                                : E->getIntroducerRange().getEnd());
     return true;
   }
 
   void addReturnTypeHint(FunctionDecl *D, SourceLocation Loc) {
     auto *AT = D->getReturnType()->getContainedAutoType();
     if (!AT || AT->getDeducedType().isNull())
       return;
     addTypeHint(Loc, D->getReturnType(), /*Prefix=*/"-> ");
   }
 
   bool VisitVarDecl(VarDecl *D) {
     // Do not show hints for the aggregate in a structured binding,
     // but show hints for the individual bindings.
     if (auto *DD = dyn_cast<DecompositionDecl>(D)) {
       for (auto *Binding : DD->bindings()) {
         addTypeHint(Binding->getLocation(), Binding->getType(), /*Prefix=*/": ",
                     StructuredBindingPolicy);
       }
       return true;
     }
 
     if (D->getType()->getContainedAutoType()) {
       if (!D->getType()->isDependentType()) {
         // Our current approach is to place the hint on the variable
         // and accordingly print the full type
         // (e.g. for `const auto& x = 42`, print `const int&`).
         // Alternatively, we could place the hint on the `auto`
         // (and then just print the type deduced for the `auto`).
         addTypeHint(D->getLocation(), D->getType(), /*Prefix=*/": ");
       }
     }
 
     // Handle templates like `int foo(auto x)` with exactly one instantiation.
     if (auto *PVD = llvm::dyn_cast<ParmVarDecl>(D)) {
       if (D->getIdentifier() && PVD->getType()->isDependentType() &&
           !getContainedAutoParamType(D->getTypeSourceInfo()->getTypeLoc())
                .isNull()) {
         if (auto *IPVD = getOnlyParamInstantiation(PVD))
           addTypeHint(D->getLocation(), IPVD->getType(), /*Prefix=*/": ");
       }
     }
 
     return true;
   }
 
   ParmVarDecl *getOnlyParamInstantiation(ParmVarDecl *D) {
     auto *TemplateFunction = llvm::dyn_cast<FunctionDecl>(D->getDeclContext());
for (auto *Param : TemplateFunction->parameters()) {
       if (Param == D)
         break;
       ++ParamIdx;
     }
     assert(ParamIdx < TemplateFunction->getNumParams() &&
            "Couldn't find param in list?");
     assert(ParamIdx < InstantiatedFunction->getNumParams() &&
            "Instantiated function has fewer (non-pack) parameters?");
     return InstantiatedFunction->getParamDecl(ParamIdx);
   }
 
   bool VisitInitListExpr(InitListExpr *Syn) {
     // We receive the syntactic form here (shouldVisitImplicitCode() is false).
     // This is the one we will ultimately attach designators to.
     // It may have subobject initializers inlined without braces. The *semantic*
     // form of the init-list has nested init-lists for these.
     // getDesignators will look at the semantic form to determine the labels.
     assert(Syn->isSyntacticForm() && "RAV should not visit implicit code!");
     if (!Cfg.InlayHints.Designators)
       return true;
     if (Syn->isIdiomaticZeroInitializer(AST.getLangOpts()))
       return true;
     llvm::DenseMap<SourceLocation, std::string> Designators =
         getDesignators(Syn);
     for (const Expr *Init : Syn->inits()) {
       if (llvm::isa<DesignatedInitExpr>(Init))
         continue;
       auto It = Designators.find(Init->getBeginLoc());
       if (It != Designators.end() &&
           !isPrecededByParamNameComment(Init, It->second))
         addDesignatorHint(Init->getSourceRange(), It->second);
     }
     return true;
   }
 
   // FIXME: Handle RecoveryExpr to try to hint some invalid calls.
 
 private:
   using NameVec = SmallVector<StringRef, 8>;
 
   // The purpose of Anchor is to deal with macros. It should be the call's
   // opening or closing parenthesis or brace. (Always using the opening would
   // make more sense but CallExpr only exposes the closing.) We heuristically
   // assume that if this location does not come from a macro definition, then
   // the entire argument list likely appears in the main file and can be hinted.
   void processCall(SourceLocation Anchor, const FunctionDecl *Callee,
-                   llvm::ArrayRef<const Expr *const> Args) {
+                   llvm::ArrayRef<const Expr *> Args) {
     if (!Cfg.InlayHints.Parameters || Args.size() == 0 || !Callee)
       return;
 
     // If the anchor location comes from a macro defintion, there's nowhere to
     // put hints.
     if (!AST.getSourceManager().getTopMacroCallerLoc(Anchor).isFileID())
       return;
 
     // The parameter name of a move or copy constructor is not very interesting.
     if (auto *Ctor = dyn_cast<CXXConstructorDecl>(Callee))
       if (Ctor->isCopyOrMoveConstructor())
         return;
 
-    // Don't show hints for variadic parameters.
-    size_t FixedParamCount = getFixedParamCount(Callee);
-    size_t ArgCount = std::min(FixedParamCount, Args.size());
-    auto Params = Callee->parameters();
+    // Resolve parameter packs to their forwarded parameter
+    auto ForwardedParams = resolveForwardingParameters(Callee);
 
-    NameVec ParameterNames = chooseParameterNames(Callee, ArgCount);
+    NameVec ParameterNames = chooseParameterNames(ForwardedParams);
 
     // Exclude setters (i.e. functions with one argument whose name begins with
     // "set"), and builtins like std::move/forward/... as their parameter name
     // is also not likely to be interesting.
     if (isSetter(Callee, ParameterNames) || isSimpleBuiltin(Callee))
       return;
 
-    for (size_t I = 0; I < ArgCount; ++I) {
+    for (size_t I = 0; I < ParameterNames.size() && I < Args.size(); ++I) {
+      // Pack expansion expressions cause the 1:1 mapping between arguments and
+      // parameters to break down, so we don't add further inlay hints if we
+      // encounter one.
+      if (isa<PackExpansionExpr>(Args[I])) {
+        break;
+      }
+
       StringRef Name = ParameterNames[I];
       bool NameHint = shouldHintName(Args[I], Name);
-      bool ReferenceHint = shouldHintReference(Params[I]);
+      bool ReferenceHint =
+          shouldHintReference(Callee->getParamDecl(I), ForwardedParams[I]);
 
       if (NameHint || ReferenceHint) {
         addInlayHint(Args[I]->getSourceRange(), HintSide::Left,
                      InlayHintKind::Parameter, ReferenceHint ? "&" : "",
                      NameHint ? Name : "", ": ");
       }
     }
   }
bool shouldHintName(const Expr *Arg, StringRef ParamName) {
 
     // Exclude argument expressions preceded by a /*paramName*/.
     if (isPrecededByParamNameComment(Arg, ParamName))
       return false;
 
     return true;
   }
 
-  bool shouldHintReference(const ParmVarDecl *Param) {
-    // If the parameter is a non-const reference type, print an inlay hint
+  bool shouldHintReference(const ParmVarDecl *Param,
+                           const ParmVarDecl *ForwardedParam) {
+    // We add a & hint only when the argument is passed as mutable reference.
+    // For parameters that are not part of an expanded pack, this is
+    // straightforward. For expanded pack parameters, it's likely that they will
+    // be forwarded to another function. In this situation, we only want to add
+    // the reference hint if the argument is actually being used via mutable
+    // reference. This means we need to check
+    // 1. whether the value category of the argument is preserved, i.e. each
+    //    pack expansion uses std::forward correctly.
+    // 2. whether the argument is ever copied/cast instead of passed
+    //    by-reference
+    // Instead of checking this explicitly, we use the following proxy:
+    // 1. the value category can only change from rvalue to lvalue during
+    //    forwarding, so checking whether both the parameter of the forwarding
+    //    function and the forwarded function are lvalue references detects such
+    //    a conversion.
+    // 2. if the argument is copied/cast somewhere in the chain of forwarding
+    //    calls, it can only be passed on to an rvalue reference or const lvalue
+    //    reference parameter. Thus if the forwarded parameter is a mutable
+    //    lvalue reference, it cannot have been copied/cast to on the way.
+    // Additionally, we should not add a reference hint if the forwarded
+    // parameter was only partially resolved, i.e. points to an expanded pack
+    // parameter, since we do not know how it will be used eventually.
     auto Type = Param->getType();
+    auto ForwardedType = ForwardedParam->getType();
     return Type->isLValueReferenceType() &&
-           !Type.getNonReferenceType().isConstQualified();
+           ForwardedType->isLValueReferenceType() &&
+           !ForwardedType.getNonReferenceType().isConstQualified() &&
+           !isExpandedFromParameterPack(ForwardedParam);
   }
 
   // Checks if "E" is spelled in the main file and preceded by a C-style comment
   // whose contents match ParamName (allowing for whitespace and an optional "="
   // at the end.
   bool isPrecededByParamNameComment(const Expr *E, StringRef ParamName) {
     auto &SM = AST.getSourceManager();
     auto ExprStartLoc = SM.getTopMacroCallerLoc(E->getBeginLoc());
static StringRef getSpelledIdentifier(const Expr *E) {
 
     if (auto *ME = dyn_cast<MemberExpr>(E))
       if (!ME->getQualifier() && ME->isImplicitAccess())
         return getSimpleName(*ME->getMemberDecl());
 
     return {};
   }
 
-  NameVec chooseParameterNames(const FunctionDecl *Callee, size_t ArgCount) {
-    // The current strategy here is to use all the parameter names from the
-    // canonical declaration, unless they're all empty, in which case we
-    // use all the parameter names from the definition (in present in the
-    // translation unit).
-    // We could try a bit harder, e.g.:
-    //   - try all re-declarations, not just canonical + definition
-    //   - fall back arg-by-arg rather than wholesale
-
-    NameVec ParameterNames = getParameterNamesForDecl(Callee, ArgCount);
-
-    if (llvm::all_of(ParameterNames, std::mem_fn(&StringRef::empty))) {
-      if (const FunctionDecl *Def = Callee->getDefinition()) {
-        ParameterNames = getParameterNamesForDecl(Def, ArgCount);
+  NameVec chooseParameterNames(SmallVector<const ParmVarDecl *> Parameters) {
+    NameVec ParameterNames;
+    for (const auto *P : Parameters) {
+      if (isExpandedFromParameterPack(P)) {
+        // If we haven't resolved a pack paramater (e.g. foo(Args... args)) to a
+        // non-pack parameter, then hinting as foo(args: 1, args: 2, args: 3) is
+        // unlikely to be useful.
+        ParameterNames.emplace_back();
+      } else {
+        auto SimpleName = getSimpleName(*P);
+        // If the parameter is unnamed in the declaration:
+        // attempt to get its name from the definition
+        if (SimpleName.empty()) {
+          if (const auto *PD = getParamDefinition(P)) {
+            SimpleName = getSimpleName(*PD);
+          }
+        }
+        ParameterNames.emplace_back(SimpleName);
       }
     }
-    assert(ParameterNames.size() == ArgCount);
 
     // Standard library functions often have parameter names that start
     // with underscores, which makes the hints noisy, so strip them out.
     for (auto &Name : ParameterNames)
       stripLeadingUnderscores(Name);
 
     return ParameterNames;
   }
 
-  static void stripLeadingUnderscores(StringRef &Name) {
-    Name = Name.ltrim('_');
+  // for a ParmVarDecl from a function declaration, returns the corresponding
+  // ParmVarDecl from the definition if possible, nullptr otherwise.
+  static const ParmVarDecl *getParamDefinition(const ParmVarDecl *P) {
+    if (auto *Callee = dyn_cast<FunctionDecl>(P->getDeclContext())) {
+      if (auto *Def = Callee->getDefinition()) {
+        auto I = std::distance(
+            Callee->param_begin(),
+            std::find(Callee->param_begin(), Callee->param_end(), P));
+        if (I < Callee->getNumParams()) {
+          return Def->getParamDecl(I);
         }
-
-  // Return the number of fixed parameters Function has, that is, not counting
-  // parameters that are variadic (instantiated from a parameter pack) or
-  // C-style varargs.
-  static size_t getFixedParamCount(const FunctionDecl *Function) {
-    if (FunctionTemplateDecl *Template = Function->getPrimaryTemplate()) {
-      FunctionDecl *F = Template->getTemplatedDecl();
-      size_t Result = 0;
-      for (ParmVarDecl *Parm : F->parameters()) {
-        if (Parm->isParameterPack()) {
-          break;
-        }
-        ++Result;
-      }
-      return Result;
-    }
-    // C-style varargs don't need special handling, they're already
-    // not included in getNumParams().
-    return Function->getNumParams();
+    return nullptr;
+
+  static void stripLeadingUnderscores(StringRef &Name) {
+    Name = Name.ltrim('_');
   }
 
   static StringRef getSimpleName(const NamedDecl &D) {
     if (IdentifierInfo *Ident = D.getDeclName().getAsIdentifierInfo()) {
       return Ident->getName();
     }
 
     return StringRef();
   }
 
-  NameVec getParameterNamesForDecl(const FunctionDecl *Function,
-                                   size_t ArgCount) {
-    NameVec Result;
-    for (size_t I = 0; I < ArgCount; ++I) {
-      const ParmVarDecl *Parm = Function->getParamDecl(I);
-      assert(Parm);
-      Result.emplace_back(getSimpleName(*Parm));
-    }
-    return Result;
-  }
-
   // We pass HintSide rather than SourceLocation because we want to ensure
   // it is in the same file as the common file range.
   void addInlayHint(SourceRange R, HintSide Side, InlayHintKind Kind,
                     llvm::StringRef Prefix, llvm::StringRef Label,
                     llvm::StringRef Suffix) {
     // We shouldn't get as far as adding a hint if the category is disabled.
     // We'd like to disable as much of the analysis as possible above instead.
     // Assert in debug mode but add a dynamic check in production.

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

TEST(ParameterHints, NoNameRValueReference) {
   assertParameterHints(R"cpp(
     void foo(int&&);
     void bar() {
       foo(42);
     }
   )cpp");
 }
 
+TEST(ParameterHints, NoNameVariadicDeclaration) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+    template <typename... Args>
+    void foo(Args&& ...);
+    void bar() {
+      foo(42);
+    }
+  )cpp");
+}
+
+TEST(ParameterHints, NoNameVariadicForwarded) {
+  // No hint for anonymous variadic parameter
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+    namespace std { template <typename T> T&& forward(T&); }
+    void foo(int);
+    template <typename... Args>
+    void bar(Args&&... args) { return foo(std::forward<Args>(args)...); }
+    void baz() {
+      bar(42);
+    }
+  )cpp");
+}
+
+TEST(ParameterHints, NoNameVariadicPlain) {
+  // No hint for anonymous variadic parameter
+  assertParameterHints(R"cpp(
+    void foo(int);
+    template <typename... Args>
+    void bar(Args&&... args) { return foo(args...); }
+    void baz() {
+      bar(42);
+    }
+  )cpp");
+}
+
 TEST(ParameterHints, NameInDefinition) {
   // Parameter name picked up from definition if necessary.
   assertParameterHints(R"cpp(
     void foo(int);
     void bar() {
       foo($param[[42]]);
     }
     void foo(int param) {};
   )cpp",
                        ExpectedHint{"param: ", "param"});
 }
 
+TEST(ParameterHints, NamePartiallyInDefinition) {
+  // Parameter name picked up from definition if necessary.
+  assertParameterHints(R"cpp(
+    void foo(int, int b);
+    void bar() {
+      foo($param1[[42]], $param2[[42]]);
+    }
+    void foo(int a, int) {};
+  )cpp",
+                       ExpectedHint{"a: ", "param1"},
+                       ExpectedHint{"b: ", "param2"});
+}
+
+TEST(ParameterHints, NameInDefinitionVariadic) {
+  // Parameter name picked up from definition in a resolved forwarded parameter.
+  assertParameterHints(R"cpp(
+    void foo(int, int);
+    template <typename... Args>
+    void bar(Args... args) {
+      foo(args...);
+    }
+    void baz() {
+      bar($param1[[42]], $param2[[42]]);
+    }
+    void foo(int a, int b) {};
+  )cpp",
+                       ExpectedHint{"a: ", "param1"},
+                       ExpectedHint{"b: ", "param2"});
+}
+
 TEST(ParameterHints, NameMismatch) {
   // Prefer name from declaration.
   assertParameterHints(R"cpp(
     void foo(int good);
     void bar() {
       foo($good[[42]]);
     }
     void foo(int bad) {};
void bar() {
       foo($param[[42]]);
     }
   )cpp",
                        ExpectedHint{"param: ", "param"});
 }
 
 TEST(ParameterHints, NameTypeAliasConstReference) {
   // Only name hint for const l-value ref parameter via type alias.
   assertParameterHints(R"cpp(
     using alias = const int&;
     void foo(alias param);
     void bar() {
       int i;
       foo($param[[i]]);
     }
   )cpp",
                        ExpectedHint{"param: ", "param"});
 }
 
 TEST(ParameterHints, NameReference) {
   // Reference and name hint for l-value ref parameter.
   assertParameterHints(R"cpp(
     void foo(int& param);
     void bar() {
       int i;
       foo($param[[i]]);
     }
   )cpp",
                        ExpectedHint{"&param: ", "param"});
 }
 
 TEST(ParameterHints, NameTypeAliasReference) {
   // Reference and name hint for l-value ref parameter via type alias.
   assertParameterHints(R"cpp(
     using alias = int&;
     void foo(alias param);
     void bar() {
       int i;
       foo($param[[i]]);
     }
   )cpp",
                        ExpectedHint{"&param: ", "param"});
 }
 
 TEST(ParameterHints, NameRValueReference) {
   // Only name hint for r-value ref parameter.
   assertParameterHints(R"cpp(
     void foo(int&& param);
     void bar() {
       foo($param[[42]]);
     }
   )cpp",
                        ExpectedHint{"param: ", "param"});
 }
 
+TEST(ParameterHints, VariadicForwardedConstructor) {
+  // Name hint for variadic parameter using std::forward in a constructor call
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+    namespace std { template <typename T> T&& forward(T&); }
+    struct S { S(int a); };
+    template <typename T, typename... Args>
+    T bar(Args&&... args) { return T{std::forward<Args>(args)...}; }
+    void baz() {
+      int b;
+      bar<S>($param[[b]]);
+    }
+  )cpp",
+                       ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainConstructor) {
+  // Name hint for variadic parameter in a constructor call
+  assertParameterHints(R"cpp(
+    struct S { S(int a); };
+    template <typename T, typename... Args>
+    T bar(Args&&... args) { return T{args...}; }
+    void baz() {
+      int b;
+      bar<S>($param[[b]]);
+    }
+  )cpp",
+                       ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicForwardedNewConstructor) {
+  // Name hint for variadic parameter using std::forward in a new expression
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+    namespace std { template <typename T> T&& forward(T&); }
+    struct S { S(int a); };
+    template <typename T, typename... Args>
+    T* bar(Args&&... args) { return new T{std::forward<Args>(args)...}; }
+    void baz() {
+      int b;
+      bar<S>($param[[b]]);
+    }
+  )cpp",
+                       ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainNewConstructor) {
+  // Name hint for variadic parameter in a new expression
+  assertParameterHints(R"cpp(
+    struct S { S(int a); };
+    template <typename T, typename... Args>
+    T* bar(Args&&... args) { return new T{args...}; }
+    void baz() {
+      int b;
+      bar<S>($param[[b]]);
+    }
+  )cpp",
+                       ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicForwarded) {
+  // Name for variadic parameter using std::forward
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+    namespace std { template <typename T> T&& forward(T&); }
+    void foo(int a);
+    template <typename... Args>
+    void bar(Args&&... args) { return foo(std::forward<Args>(args)...); }
+    void baz() {
+      int b;
+      bar($param[[b]]);
+    }
+  )cpp",
+                       ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlain) {
+  // Name hint for variadic parameter
+  assertParameterHints(R"cpp(
+    void foo(int a);
+    template <typename... Args>
+    void bar(Args&&... args) { return foo(args...); }
+    void baz() {
+      bar($param[[42]]);
+    }
+  )cpp",
+                       ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicPlainWithPackFirst) {
+  // Name hint for variadic parameter when the parameter pack is not the last
+  // template parameter
+  assertParameterHints(R"cpp(
+    void foo(int a);
+    template <typename... Args, typename Arg>
+    void bar(Arg, Args&&... args) { return foo(args...); }
+    void baz() {
+      bar(1, $param[[42]]);
+    }
+  )cpp",
+                       ExpectedHint{"a: ", "param"});
+}
+
+TEST(ParameterHints, VariadicSplitTwolevel) {
+  // Name for variadic parameter that involves both head and tail parameters to
+  // deal with.
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+    namespace std { template <typename T> T&& forward(T&); }
+    void baz(int, int b, double);
+    template <typename... Args>
+    void foo(int a, Args&&... args) {
+      return baz(1, std::forward<Args>(args)..., 1.0);
+    }
+    template <typename... Args>
+    void bar(Args&&... args) { return foo(std::forward<Args>(args)...); }
+    void bazz() {
+      bar($param1[[32]], $param2[[42]]);
+    }
+  )cpp",
+                       ExpectedHint{"a: ", "param1"},
+                       ExpectedHint{"b: ", "param2"});
+}
+
+TEST(ParameterHints, VariadicNameFromSpecialization) {
+  // We don't try to resolve forwarding parameters if the function call uses a
+  // specialization.
+  assertParameterHints(R"cpp(
+    void foo(int a);
+    template <typename... Args>
+    void bar(Args... args) {
+      foo(args...);
+    }
+    template <>
+    void bar<int>(int b);
+    void baz() {
+      bar($param[[42]]);
+    }
+  )cpp",
+                       ExpectedHint{"b: ", "param"});
+}
+
+TEST(ParameterHints, VariadicNameFromSpecializationRecursive) {
+  // We don't try to resolve forwarding parameters inside a forwarding function
+  // call if that function call uses a specialization.
+  assertParameterHints(R"cpp(
+    void foo2(int a);
+    template <typename... Args>
+    void foo(Args... args) {
+      foo2(args...);
+    }
+    template <typename... Args>
+    void bar(Args... args) {
+      foo(args...);
+    }
+    template <>
+    void foo<int>(int b);
+    void baz() {
+      bar($param[[42]]);
+    }
+  )cpp",
+                       ExpectedHint{"b: ", "param"});
+}
+
+TEST(ParameterHints, VariadicOverloaded) {
+  // Name for variadic parameter for an overloaded function with unique number
+  // of parameters.
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(
+      R"cpp(
+    namespace std { template <typename T> T&& forward(T&); }
+    void baz(int b, int c);
+    void baz(int bb, int cc, int dd);
+    template <typename... Args>
+    void foo(int a, Args&&... args) {
+      return baz(std::forward<Args>(args)...);
+    }
+    template <typename... Args>
+    void bar(Args&&... args) { return foo(std::forward<Args>(args)...); }
+    void bazz() {
+      bar($param1[[32]], $param2[[42]], $param3[[52]]);
+      bar($param4[[1]], $param5[[2]], $param6[[3]], $param7[[4]]);
+    }
+  )cpp",
+      ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"},
+      ExpectedHint{"c: ", "param3"}, ExpectedHint{"a: ", "param4"},
+      ExpectedHint{"bb: ", "param5"}, ExpectedHint{"cc: ", "param6"},
+      ExpectedHint{"dd: ", "param7"});
+}
+
+TEST(ParameterHints, VariadicRecursive) {
+  // make_tuple-like recursive variadic call
+  assertParameterHints(
+      R"cpp(
+    void foo();
+
+    template <typename Head, typename... Tail>
+    void foo(Head head, Tail... tail) {
+      foo(tail...);
+    }
+
+    template <typename... Args>
+    void bar(Args... args) {
+      foo(args...);
+    }
+
+    int main() {
+      bar(1, 2, 3);
+    }
+  )cpp");
+}
+
+TEST(ParameterHints, VariadicVarargs) {
+  // variadic call involving varargs (to make sure we don't crash)
+  assertParameterHints(R"cpp(
+    void foo(int fixed, ...);
+    template <typename... Args>
+    void bar(Args&&... args) {
+      foo(args...);
+    }
+
+    void baz() {
+      bar($fixed[[41]], 42, 43);
+    }
+  )cpp");
+}
+
+TEST(ParameterHints, VariadicTwolevelUnresolved) {
+  // the same setting as VariadicVarargs, only with parameter pack
+  assertParameterHints(R"cpp(
+    template <typename... Args>
+    void foo(int fixed, Args&& ... args);
+    template <typename... Args>
+    void bar(Args&&... args) {
+      foo(args...);
+    }
+
+    void baz() {
+      bar($fixed[[41]], 42, 43);
+    }
+  )cpp",
+                       ExpectedHint{"fixed: ", "fixed"});
+}
+
+TEST(ParameterHints, VariadicTwoCalls) {
+  // only the first call using the parameter pack should be picked up
+  assertParameterHints(
+      R"cpp(
+    void f1(int a, int b);
+    void f2(int c, int d);
+
+    bool cond;
+
+    template <typename... Args>
+    void foo(Args... args) {
+      if (cond) {
+        f1(args...);
+      } else {
+        f2(args...);
+      }
+    }
+
+    int main() {
+      foo($param1[[1]], $param2[[2]]);
+    }
+  )cpp",
+      ExpectedHint{"a: ", "param1"}, ExpectedHint{"b: ", "param2"});
+}
+
+TEST(ParameterHints, VariadicInfinite) {
+  // infinite recursion should not break clangd
+  assertParameterHints(
+      R"cpp(
+    template <typename... Args>
+    void foo(Args...);
+
+    template <typename... Args>
+    void bar(Args... args) {
+      foo(args...);
+    }
+
+    template <typename... Args>
+    void foo(Args... args) {
+      bar(args...);
+    }
+
+    int main() {
+      foo(1, 2);
+    }
+  )cpp");
+}
+
+TEST(ParameterHints, VariadicDuplicatePack) {
+  // edge cases with multiple adjacent packs should work
+  assertParameterHints(
+      R"cpp(
+    void foo(int a, int b, int c, int);
+
+    template <typename... Args>
+    void bar(int, Args... args, int d) {
+      foo(args..., d);
+    }
+
+    template <typename... Args>
+    void baz(Args... args, Args... args2) {
+      bar<Args..., int>(1, args..., args2...);
+    }
+
+    int main() {
+      baz<int, int>($p1[[1]], $p2[[2]], $p3[[3]], $p4[[4]]);
+    }
+  )cpp",
+      ExpectedHint{"a: ", "p1"}, ExpectedHint{"b: ", "p2"},
+      ExpectedHint{"c: ", "p3"}, ExpectedHint{"d: ", "p4"});
+}
+
+TEST(ParameterHints, VariadicEmplace) {
+  // emplace-like calls should forward constructor parameters
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(
+      R"cpp(
+    namespace std { template <typename T> T&& forward(T&); }
+    using size_t = decltype(sizeof(0));
+    void *operator new(size_t, void *);
+    struct S {
+      S(int A);
+      S(int B, int C);
+    };
+    struct alloc {
+      template <typename T>
+      T* allocate();
+      template <typename T, typename... Args>
+      void construct(T* ptr, Args&&... args) {
+        ::new ((void*)ptr) T{std::forward<Args>(args)...};
+      }
+    };
+    template <typename T>
+    struct container {
+      template <typename... Args>
+      void emplace(Args&&... args) {
+        alloc a;
+        auto ptr = a.template allocate<T>();
+        a.construct(ptr, std::forward<Args>(args)...);
+      }
+    };
+    void foo() {
+      container<S> c;
+      c.emplace($param1[[1]]);
+      c.emplace($param2[[2]], $param3[[3]]);
+    }
+  )cpp",
+      ExpectedHint{"A: ", "param1"}, ExpectedHint{"B: ", "param2"},
+      ExpectedHint{"C: ", "param3"});
+}
+
+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);
+      bar(1);
+    }
+  )cpp");
+}
+
+TEST(ParameterHints, VariadicReferenceHintForwardingRef) {
+  assertParameterHints(R"cpp(
+    void foo(int&);
+    template <typename... Args>
+    void bar(Args&&... args) { return foo(args...); }
+    void baz() {
+      int a;
+      bar($param[[a]]);
+      bar(1);
+    }
+  )cpp",
+                       ExpectedHint{"&: ", "param"});
+}
+
+TEST(ParameterHints, VariadicReferenceHintForwardingRefStdForward) {
+  assertParameterHints(R"cpp(
+    namespace std { template <typename T> T&& forward(T&); }
+    void foo(int&);
+    template <typename... Args>
+    void bar(Args&&... args) { return foo(std::forward<Args>(args)...); }
+    void baz() {
+      int a;
+      bar($param[[a]]);
+    }
+  )cpp",
+                       ExpectedHint{"&: ", "param"});
+}
+
+TEST(ParameterHints, VariadicNoReferenceHintForwardingRefStdForward) {
+  assertParameterHints(R"cpp(
+    namespace std { template <typename T> T&& forward(T&); }
+    void foo(int);
+    template <typename... Args>
+    void bar(Args&&... args) { return foo(std::forward<Args>(args)...); }
+    void baz() {
+      int a;
+      bar(a);
+      bar(1);
+    }
+  )cpp");
+}
+
+TEST(ParameterHints, VariadicNoReferenceHintUnresolvedForward) {
+  assertParameterHints(R"cpp(
+    template <typename... Args>
+    void foo(Args&&... args);
+    void bar() {
+      int a;
+      foo(a);
+    }
+  )cpp");
+}
+
+TEST(ParameterHints, MatchingNameVariadicForwarded) {
+  // No name hint for variadic parameter with matching name
+  // This prototype of std::forward is sufficient for clang to recognize it
+  assertParameterHints(R"cpp(
+    namespace std { template <typename T> T&& forward(T&); }
+    void foo(int a);
+    template <typename... Args>
+    void bar(Args&&... args) { return foo(std::forward<Args>(args)...); }
+    void baz() {
+      int a;
+      bar(a);
+    }
+  )cpp");
+}
+
+TEST(ParameterHints, MatchingNameVariadicPlain) {
+  // No name hint for variadic parameter with matching name
+  assertParameterHints(R"cpp(
+    void foo(int a);
+    template <typename... Args>
+    void bar(Args&&... args) { return foo(args...); }
+    void baz() {
+      int a;
+      bar(a);
+    }
+  )cpp");
+}
+
 TEST(ParameterHints, Operator) {
   // No hint for operator call with operator syntax.
   assertParameterHints(R"cpp(
     struct S {};
     void operator+(S lhs, S rhs);
     void bar() {
       S a, b;
       a + b;
TEST(ParameterHints, VariadicFunction) {
   )cpp",
                        ExpectedHint{"fixed: ", "fixed"});
 }
 
 TEST(ParameterHints, VarargsFunction) {
   assertParameterHints(R"cpp(
     void foo(int fixed, ...);
 
-    void bar() { 
+    void bar() {
       foo($fixed[[41]], 42, 43);
     }
   )cpp",
                        ExpectedHint{"fixed: ", "fixed"});
 }
 
 TEST(ParameterHints, CopyOrMoveConstructor) {
   // Do not show hint for parameter of copy or move constructor.