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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

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)



================
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;
----------------
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...)


================
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).
Right, this is definitely UB as written.

FWIW I find this easier to understand in classic bounds-check form:

if ((It - Args.begin()) + Parameters.size() >= Args.size())


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1468
+    void bar(Args... args) {
+      foo(id($param1[[args]], $param2[[1]])...);
+    }
----------------
(Just so I understand: these are getting hinted due to the "only instantiation" logic, right?)


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1471
+    void foo() {
+      bar(1, 2);
+    }
----------------
could reasonably expect this to be `bar(a:1, 2)` or `bar(a:1, a:2)` - leave a comment explaining why?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130260