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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

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)



================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:189
 
+bool isExpandedParameter(const ParmVarDecl *Param) {
+  auto PlainType = Param->getType().getNonReferenceType();
----------------
nit: isPackExpandedParameter
(usually "expanded" means macro expansion)


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:197
+
+class ForwardingParameterVisitor
+    : public RecursiveASTVisitor<ForwardingParameterVisitor> {
----------------
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.


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:201
+  void
+  resolveForwardedParameters(const FunctionDecl *Callee,
+                             llvm::SmallVector<const ParmVarDecl *> &Params) {
----------------
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`.


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:208
+        // If the parameter is part of an expanded pack and not yet resolved
+        if (/*isExpandedParameter(Param) && */
+            ForwardedParams.find(Param) == ForwardedParams.end()) {
----------------
nridge wrote:
> nridge wrote:
> > upsj wrote:
> > > 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
> > 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.
> 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`.
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 = ...)
```


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:261
+private:
+  void handleParmVarDeclName(const FunctionDecl *Callee, size_t I) {
+    const auto *Param = Callee->getParamDecl(I);
----------------
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.


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:286
+            const auto *Param = Callee->getParamDecl(ArgIdx);
+            // If this parameter is part of an expanded pack, we need to recurse
+            if (isExpandedParameter(Param)) {
----------------
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).


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:303
+    }
+    if (Recurse && !TraversedFunctions.contains(Callee)) {
+      TraversalQueue.push_back(Callee);
----------------
(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)


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:355
+    }
+    // path compression
+    It->second = getForwardedParameter(It->second);
----------------
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


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:374
+  // have one in the definition, store this mapping here.
+  llvm::DenseMap<const ParmVarDecl *, const ParmVarDecl *> ParamDeclToDef;
+};
----------------
nridge wrote:
> 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
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?


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:447
+    namespace std { template <typename T> T&& forward(T&); }
+    void *operator new(unsigned long, void *);
+    struct S {
----------------
upsj wrote:
> This is not portable, but I don't have access to size_t
Hmm, I think you can use `using size_t = decltype(sizeof(0))`?


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:178
+  // No hint for anonymous variadic parameter
+  // The prototype is not correct, but is converted into builtin anyways.
+  assertParameterHints(R"cpp(
----------------
upsj wrote:
> nridge wrote:
> > What does "converted into builtin" mean here?
> 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%.
Ah, right!
Maybe say "This prototype of std::forward is sufficient for clang to recognize it"?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690