[PATCH] D130260: [clangd] Make forwarding parameter detection logic resilient

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/AST.cpp:832
           continue;
+        // Check that this expands all the way until the last parameter.
+        auto ParamEnd = It + Parameters.size() - 1;
----------------
upsj wrote:
> sammccall wrote:
> > The essence of this test seems to be "if we can't rule it out by counting parameters, it must be right" which... doesn't seem very robust? Am I missing something?
> > 
> > AIUI the idea here is that
> > A) `foo(pack...)` is "expanded all the way" into consecutive arguments
> > B) `foo(pack)...` is not considered forwarding for our purposes
> > C) we want to treat `foo(bar(pack)...)` specially if `bar` is "just a cast" like move or forward
> > 
> > The difference between cases A and B aren't about counting arguments though, they're about what operations occur intercede between `pack` and the `...` operator. (i.e. `foo((&pack)...)` is fine for the same reason `forward` is.
> > 
> > This seems like something best handled by:
> >  - finding the `PackExpansionExpr` controlling `pack` in the non-instantiated template AST
> >  - walking down to try to find `pack`
> >  - continue as we walk through e.g. parens or std::forward
> >  - bail out if we walk through something unknown (e.g. another function call)
> > 
> > Does this seem sensible/feasible?
> > (Walking up from `pack` is nicer but the AST doesn't support walking up well...)
> This goes a bit more generally into the question how to approach parameter packs across clangd. For inlay hints and hover, we are already looking at instantiated templates (is that true in all cases? Haven't dug in yet), so implementing forwarding there was straightforward. For signature help and code completion, we'd probably need to go via the uninstantiated template (and probably lose a bit of type information on the way, e.g. reference vs. value, l-value vs. r-value). If we wanted a generic way to deal with forwarding, the instantiation pattern would be my first choice, but I didn't have the time to go down this rabbit hole yet.
> Unless there is a nice way to map between template and instantiation AST, I don't think combining them is feasible.
> 
> On the heuristic itself: Is there a way in C++ to "partially" unpack a parameter pack without this showing up in the AST? I am not aware of any, which is why the test "do the first and last parameters appear in a matching range of the arguments" seems sufficient and necessary in all but single-entry parameter packs.
> Unless there is a nice way to map between template and instantiation AST, I don't think combining them is feasible.

Fair enough, I don't see one.

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

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

There's an exception I think, if the pack size is one it falls over:
```
bar(int bar0);
bar(int bar0, int bar1);
bar(int bar0i, int bar1, int bar2);

template <class...Args>
void foo(Args...args) {
  bar(args...); // should yield labels when expanded
  bar(args)...; // should not yield labels when expanded
}
// but when there's one arg (foo<int>) the two have the same AST
```
I think it's OK to just get this one wrong


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130260