Page MenuHomePhabricator

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

Authored by upsj on Apr 29 2022, 11:10 AM.

Details

Summary

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

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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.

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 updated this revision to Diff 432900.May 30 2022, 6:00 AM
upsj marked an inline comment as done.

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 updated this revision to Diff 433732.Jun 2 2022, 7:06 AM
  • fix varargs, again :)
  • remove parameter names entirely if they occur multiple times
upsj updated this revision to Diff 434257.Jun 4 2022, 3:51 AM

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

upsj updated this revision to Diff 434262.Jun 4 2022, 5:23 AM

add test for varargs function called from forwarding function

upsj added inline comments.Jun 4 2022, 5:24 AM
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 added a comment.Jun 10 2022, 5:45 AM

@nridge @sammccall can you give this another look?

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

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.

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 updated this revision to Diff 437194.Jun 15 2022, 8:42 AM
upsj marked 9 inline comments as done.
  • improve documentation
  • add more edge case tests
  • fix reference hints for parameter packs
  • remove unnecessary headers
  • fix bug in handling of tail parameters
upsj added inline comments.Jun 15 2022, 8:44 AM
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
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 :)

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 updated this revision to Diff 437508.Jun 16 2022, 5:09 AM
upsj marked 4 inline comments as done.
  • 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.

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 added a comment.Jun 28 2022, 1:18 AM

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

sammccall accepted this revision.Jun 28 2022, 6:37 AM

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)

This revision is now accepted and ready to land.Jun 28 2022, 6:37 AM
nridge added inline comments.Jun 28 2022, 3:59 PM
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 added inline comments.Jun 29 2022, 2:14 AM
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 updated this revision to Diff 440921.Jun 29 2022, 3:13 AM
upsj marked 14 inline comments as done.
  • 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 accepted this revision.Jun 29 2022, 3:56 AM

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 marked an inline comment as done.Jun 29 2022, 5:08 AM

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 added inline comments.Jun 29 2022, 7:26 AM
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 updated this revision to Diff 441520.Jun 30 2022, 2:18 PM
upsj marked 3 inline comments as done.

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

upsj added inline comments.Jun 30 2022, 2:23 PM
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 updated this revision to Diff 441646.Jul 1 2022, 2:21 AM

don't add reference hints to unresolved parameter packs

upsj added a comment.Jul 5 2022, 7:21 AM

@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 :)

This revision was automatically updated to reflect the committed changes.