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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In D130260#3671290 <https://reviews.llvm.org/D130260#3671290>, @sammccall wrote:

> driveby thoughts, but please don't block on them.
>
> (if this fix is a heuristic that fixes a common crash but isn't completely correct, that may still be worth landing but warrants a fixme)

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



================
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;
----------------
sammccall wrote:
> 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
i agree that we can do things differently here, but with this patch i am mostly trying to make sure we address the resiliency so that this doesn't bite us in production (and a couple of easy improvements to the existing cases for common code patterns).

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


================
Comment at: clang-tools-extra/clangd/AST.cpp:833-837
+        auto ParamEnd = It + Parameters.size() - 1;
+        assert(std::distance(Args.begin(), ParamEnd) <
+                   std::distance(Args.begin(), Args.end()) &&
+               "Only functions with greater than or equal number of parameters "
+               "should be checked.");
----------------
upsj wrote:
> I think it would be safer to check this explicitly, since advancing the iterator past its end might be UB (depending on the implementation).
i didn't do an explicit check, because we already have that explicit check inside the call site and doing a possibly linear operation for each argument in production setup doesn't feel nice


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1468
+    void bar(Args... args) {
+      foo(id($param1[[args]], $param2[[1]])...);
+    }
----------------
sammccall wrote:
> (Just so I understand: these are getting hinted due to the "only instantiation" logic, right?)
not only instantiation but "only overload" logic (i suppose that's what you mean?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130260