[clangd] Heuristically resolve dependent call through smart pointer type

[nridge]

Summary

Fixes https://github.com/clangd/clangd/issues/227

[nridge]

clang-tools-extra/clangd/FindTarget.cpp

122

I don't intend for this function to be in the final patch.

Rather, I'm wondering: am I missing some obvious general way to recover the ASTContext from a Type (or a Stmt)?

If not, I think we'll need to modify targetDecl(), allTargetDecls(), findExplicitReferences() etc. to have their callers pass in an ASTContext?

[nridge]

Shopping around to other reviewers while Sam's OOO.

[sammccall]

This is great! Thank you!

clang-tools-extra/clangd/FindTarget.cpp

122

Haha, I see what you mean. There isn't a general way, but I think your hack is better than changing the public API, and we can improve it a little.

By the time you actually do the lookup, you have a CXXRecordDecl you're going to look into, and decls have a pointer to the ASTContext. So instead of a DeclarationName, you could pass a function_ref<const DeclarationName&(ASTContext&)> or such into getMembersReferencedViaDependentName, and have that function lazily call the factory to get the DeclarationName to use. Still hacky, but it's an internal function, and we don't have the weird parallel codepath...

clang-tools-extra/clangd/unittests/XRefsTests.cpp

488

you can remove the FIXME

[nridge]

Address review comments

clang-tools-extra/clangd/FindTarget.cpp

 #include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/StmtVisitor.h"
 #include "clang/AST/TemplateBase.h"
 #include "clang/AST/Type.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/AST/TypeLocVisitor.h"
 #include "clang/Basic/LangOptions.h"
+#include "clang/Basic/OperatorKinds.h"
 #include "clang/Basic/SourceLocation.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/raw_ostream.h"
 #include <utility>
 

 // name to one or more declarations.
 // The current heuristic is simply to look up the name in the primary
 // template. This is a heuristic because the template could potentially
 // have specializations that declare different members.
 // Multiple declarations could be returned if the name is overloaded
 // (e.g. an overloaded method in the primary template).
 // This heuristic will give the desired answer in many cases, e.g.
 // for a call to vector<T>::size().
-std::vector<const NamedDecl *>
-getMembersReferencedViaDependentName(const Type *T, const DeclarationName &Name,
-                                     bool IsNonstaticMember) {
+// The name to look up is provided in the form of a factory that takes
+// an ASTContext, because an ASTContext may be needed to obtain the
+// name (e.g. if it's an operator name), but the caller may not have
+// access to an ASTContext.
+std::vector<const NamedDecl *> getMembersReferencedViaDependentName(
+    const Type *T,
+    llvm::function_ref<DeclarationName(ASTContext &)> NameFactory,
   if (!T)
     return {};
   if (auto *ICNT = T->getAs<InjectedClassNameType>()) {
     T = ICNT->getInjectedSpecializationType().getTypePtrOrNull();
   }
   auto *TST = T->getAs<TemplateSpecializationType>();
   if (!TST)
     return {};
   const ClassTemplateDecl *TD = dyn_cast_or_null<ClassTemplateDecl>(
       TST->getTemplateName().getAsTemplateDecl());
   if (!TD)
     return {};
   CXXRecordDecl *RD = TD->getTemplatedDecl();
   if (!RD->hasDefinition())
     return {};
   RD = RD->getDefinition();
+  DeclarationName Name = NameFactory(RD->getASTContext());
   return RD->lookupDependentName(Name, [=](const NamedDecl *D) {
     return IsNonstaticMember ? D->isCXXInstanceMember()
                              : !D->isCXXInstanceMember();
   });
 }
 
+// Given the type T of a dependent expression that appears of the LHS of a "->",
+// heuristically find a corresponding pointee type in whose scope we could look
+// up the name appearing on the RHS.
+const Type *getPointeeType(const Type *T) {
+  if (!T)
+    return nullptr;
+
+  if (T->isPointerType()) {
+    return T->getAs<PointerType>()->getPointeeType().getTypePtrOrNull();
+  }
+
+  // Try to handle smart pointer types.
+
+  // Look up operator-> in the primary template. If we find one, it's probably a
+  // smart pointer type.
+  auto ArrowOps = getMembersReferencedViaDependentName(
+      T,
+      [](ASTContext &Ctx) {
+        return Ctx.DeclarationNames.getCXXOperatorName(OO_Arrow);
+      },
+      /*IsNonStaticMember=*/true);
+  if (ArrowOps.empty())
+    return nullptr;
+
+  // Getting the return type of the found operator-> method decl isn't useful,
+  // because we discarded template arguments to perform lookup in the primary
+  // template scope, so the return type would just have the form U* where U is a
+  // template parameter type.
+  // Instead, just handle the common case where the smart pointer type has the
+  // form of SmartPtr<X, ...>, and assume X is the pointee type.
+  auto *TST = T->getAs<TemplateSpecializationType>();
+  if (!TST)
+    return nullptr;
+  if (TST->getNumArgs() == 0)
+    return nullptr;
+  const TemplateArgument &FirstArg = TST->getArg(0);
+  if (FirstArg.getKind() != TemplateArgument::Type)
+    return nullptr;
+  return FirstArg.getAsType().getTypePtrOrNull();
+}
+
 // TargetFinder locates the entities that an AST node refers to.
 //
 // Typically this is (possibly) one declaration and (possibly) one type, but
 // may be more:
 //  - for ambiguous nodes like OverloadExpr
 //  - if we want to include e.g. both typedefs and the underlying type
 //
 // This is organized as a set of mutually recursive helpers for particular node
struct Visitor : public ConstStmtVisitor<Visitor> {
             // We don't know which designator was intended, we assume the outer.
             break;
           }
       }
       void
       VisitCXXDependentScopeMemberExpr(const CXXDependentScopeMemberExpr *E) {
         const Type *BaseType = E->getBaseType().getTypePtrOrNull();
         if (E->isArrow()) {
-          // FIXME: Handle smart pointer types by looking up operator->
-          // in the primary template.
-          if (!BaseType || !BaseType->isPointerType()) {
-            return;
-          }
-          BaseType = BaseType->getAs<PointerType>()
-                         ->getPointeeType()
-                         .getTypePtrOrNull();
-        }
-        for (const NamedDecl *D :
-             getMembersReferencedViaDependentName(BaseType, E->getMember(),
-                                                  /*IsNonstaticMember=*/true)) {
+          BaseType = getPointeeType(BaseType);
+        for (const NamedDecl *D : getMembersReferencedViaDependentName(
+                 BaseType, [E](ASTContext &) { return E->getMember(); },
           Outer.add(D, Flags);
         }
       }
       void VisitDependentScopeDeclRefExpr(const DependentScopeDeclRefExpr *E) {
         for (const NamedDecl *D : getMembersReferencedViaDependentName(
-                 E->getQualifier()->getAsType(), E->getDeclName(),
+                 E->getQualifier()->getAsType(),
+                 [E](ASTContext &) { return E->getDeclName(); },
                  /*IsNonstaticMember=*/false)) {
           Outer.add(D, Flags);
         }
       }
       void VisitObjCIvarRefExpr(const ObjCIvarRefExpr *OIRE) {
         Outer.add(OIRE->getDecl(), Flags);
       }
       void VisitObjCMessageExpr(const ObjCMessageExpr *OME) {

clang-tools-extra/clangd/unittests/XRefsTests.cpp

const char *Tests[] = {
         };
 
         template <typename T>
         void foo() {
           S<T>::ba^r();
         }
       )cpp",
 
-      R"cpp(// FIXME: Heuristic resolution of dependent method
+      R"cpp(// Heuristic resolution of dependent method
             // invoked via smart pointer
-        template <typename> struct S { void foo(); };
+        template <typename> struct S { void [[foo]]() {} };
         template <typename T> struct unique_ptr {
           T* operator->();
         };
         template <typename T>
         void test(unique_ptr<S<T>>& V) {
           V->fo^o();
         }
       )cpp"};