[PATCH] D71240: [clangd] Heuristically resolve dependent method calls

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Awesome, thanks, I'd been hoping to get around to this! LG with some naming/structure nits.
This doesn't handle references into smart pointers (details below), but feel free to do that in this patch, separately, or not at all.



================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:81
 // This affects:
 //  - DependentTemplateSpecializationType,
 //  - DependentNameType
----------------
while here, I think UnresolvedUsingValueDecl and UnresolvedUsingTypenameDecl can be added here :-)


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:217
+        if (E->isArrow()) {
+          if (!BaseType || !BaseType->isPointerType()) {
+            return;
----------------
If I'm understanding the AST correctly, this drops the `operator->` case, where the base is a smart-pointer.

e.g. `void run(unique_ptr<Action<T>> X) { X->go(); }`

There are potentially two reasons then that `x->go` is a *dependent* memberexpr:
 - `operator->` probably returns T*, but `unique_ptr` might be specialized (you don't handle this)
 - we probably want the primary `Action<T>::go`, but `Action` might be specialized (you do handle this)

So it might be worth trying to get the pointee type (if the base type is a TemplateSpecializationType, look up operator-> in the primary template, and replace the base with its return type).
If you don't want to do this yet, we should leave a comment (like `FIXME: handle unique_ptr<T> by looking up operator-> in the primary template`)


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:255
+                                                bool IsNonstaticMember) {
+        // This code was adapted in part from indexDependentReference() in
+        // IndexBody.cpp.
----------------
In the long term, I'm not sure this comment will be as useful as one saying something like:
"Dependent name references like `vector<T>::size()` can't be definitively resolved, but the using the definition from the primary template is probably better than giving up"


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:278
+            });
+        for (const NamedDecl *D : Decls) {
+          Outer.add(D, Flags);
----------------
can we make this function return the decls, and make the callers call add() in a loop?

That way this function could be reused for the `operator->` lookup, and could be a standalone helper function outside this class. (Maybe eventually exposed in AST.h, as I can imagine it being useful in other places, but static in this file seems fine)


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:278
+            });
+        for (const NamedDecl *D : Decls) {
+          Outer.add(D, Flags);
----------------
sammccall wrote:
> can we make this function return the decls, and make the callers call add() in a loop?
> 
> That way this function could be reused for the `operator->` lookup, and could be a standalone helper function outside this class. (Maybe eventually exposed in AST.h, as I can imagine it being useful in other places, but static in this file seems fine)
with multiple lookup results we could add all, discard all, or pick one arbitrarily.

I'm not sure what the best policy is, but it's worth pointing out in a comment what the cases are and what the choice is.

I think this should only affects overloads, where returning all seems reasonable (resolution would be way too much work).

Can you add a test?


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:470
+
+      R"cpp(// Heuristic resolution of method
+        template <typename T>
----------------
nit: dependent method (just so it's easier to search for)


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:501
+        }
       )cpp"};
   for (const char *Test : Tests) {
----------------
could you add a test for the `operator->` case too, even if we don't handle it yet? (in which case with a FIXME)

Something like:
```
template <typename T> struct unique_ptr<T> { T* operator->(); };
template <typename T> struct S { void [[foo]](); }
template <typename T> void test(unique_ptr<S<T>>& V) { V->f^oo(); }
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71240