[clangd] Enable textual fallback for go-to-definition on dependent names

[nridge]

clang-tools-extra/clangd/XRefs.cpp

228

In the future, I can envision expanding this to return additional information, possibly the AST node itself. For now, I just started with a boolean.

[nridge]

Review ping :)

[nridge]

Rebase on top of D75479

[sammccall]

Thanks for your patience!

clang-tools-extra/clangd/XRefs.cpp

154

shovelling this IsDependentName bool around seems a bit ad-hoc. The actual node doesn't live long enough, but can you make it an ASTNodeKind instead, and move the isDependentName check down to locateTextually where it makes sense to be that specific?

354

This sentence is hard to parse now, and also doesn't really justify the behaviour.
Suggest leaving it alone and adding:
// Exception: dependent names, because XYZ

570–572

the way the two (possible!) writes get combined is not obvious here.

I'd suggest using two different variables and writing IsDependent || IsNearbyIdentDependent in the usage below, if that's the intent.

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

665–666

move these into the header so it can't be identifier-based heuristics? (And so the test doesn't change behaviour when we turn that on)

693

this has a loop but only one test...

780

this looks like a very close superset of TextualDependent, do we need both?

Again, these decls should move into the header I think.

[nridge]

Address review comments

clang-tools-extra/clangd/XRefs.cpp

570–572

Good catch. There's actually no point letting this second locateASTReferent() call set IsDependentName, because IsDependentName only changes the outcome if the original location was an expanded token, but findNearbyIdentifier() bails in that case. So now I just pass nullptr instead.

(We could work harder and make "comment referencing nearby dependent name" work, but I think that's enough of an edge that I didn't bother.)

[sammccall]

Great, thank you!

[thakis]

This breaks tests on Windows: http://45.33.8.238/win/13855/step_9.txt

Please take a look and revert while you do if it takes a while to investigate.

[sammccall]

In D76451#2004069, @thakis wrote:

This breaks tests on Windows: http://45.33.8.238/win/13855/step_9.txt

Please take a look and revert while you do if it takes a while to investigate.

@nridge hold off on the revert, I suspect this is our good friend -fdelayed-template-parsing and I'm about to nuke it in D78848. Committing now, let's see if that fixes it.

[sammccall]

Yes, looks fixed by 6d7637d.

clang-tools-extra/clangd/XRefs.h

 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_XREFS_H
 
 #include "FormattedString.h"
 #include "Path.h"
 #include "Protocol.h"
 #include "SourceCode.h"
 #include "index/Index.h"
 #include "index/SymbolLocation.h"
+#include "clang/AST/ASTTypeTraits.h"
 #include "clang/AST/Type.h"
 #include "clang/Format/Format.h"
 #include "clang/Index/IndexSymbol.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/raw_ostream.h"
 #include <vector>
 
 namespace clang {

 // Tries to provide a textual fallback for locating a symbol by looking up the
 // word under the cursor as a symbol name in the index.
 // The aim is to pick up references to symbols in contexts where
 // AST-based resolution does not work, such as comments, strings, and PP
 // disabled regions.
 // (This is for internal use by locateSymbolAt, and is exposed for testing).
 std::vector<LocatedSymbol>
 locateSymbolTextually(const SpelledWord &Word, ParsedAST &AST,
-                      const SymbolIndex *Index,
-                      const std::string &MainFilePath);
+                      const SymbolIndex *Index, const std::string &MainFilePath,
+                      ASTNodeKind NodeKind);
 
 // Try to find a proximate occurrence of `Word` as an identifier, which can be
 // used to resolve it.
 // (This is for internal use by locateSymbolAt, and is exposed for testing).
 const syntax::Token *findNearbyIdentifier(const SpelledWord &Word,
                                           const syntax::TokenBuffer &TB);
 
 /// Get all document links

clang-tools-extra/clangd/XRefs.cpp

 #include "Selection.h"
 #include "SourceCode.h"
 #include "URI.h"
 #include "index/Index.h"
 #include "index/Merge.h"
 #include "index/Relation.h"
 #include "index/SymbolLocation.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/ASTTypeTraits.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/Attrs.inc"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/Type.h"
 #include "clang/Basic/CharInfo.h"
SymbolLocation getPreferredLocation(const Location &ASTLoc,
   Symbol ASTSym, IdxSym;
   ASTSym.ID = IdxSym.ID = SymbolID("dummy_id");
   ASTSym.CanonicalDeclaration = toIndexLocation(ASTLoc, Scratch);
   IdxSym.CanonicalDeclaration = IdxLoc;
   auto Merged = mergeSymbol(ASTSym, IdxSym);
   return Merged.CanonicalDeclaration;
 }
 
-std::vector<const NamedDecl *> getDeclAtPosition(ParsedAST &AST,
-                                                 SourceLocation Pos,
-                                                 DeclRelationSet Relations) {
+std::vector<const NamedDecl *>
+getDeclAtPosition(ParsedAST &AST, SourceLocation Pos, DeclRelationSet Relations,
+                  ASTNodeKind *NodeKind = nullptr) {
   unsigned Offset = AST.getSourceManager().getDecomposedSpellingLoc(Pos).second;
   std::vector<const NamedDecl *> Result;
   SelectionTree::createEach(AST.getASTContext(), AST.getTokens(), Offset,
                             Offset, [&](SelectionTree ST) {
                               if (const SelectionTree::Node *N =
-                                      ST.commonAncestor())
+                                      ST.commonAncestor()) {
+                                if (NodeKind)
+                                  *NodeKind = N->ASTNode.getNodeKind();
                                 llvm::copy(targetDecl(N->ASTNode, Relations),
                                            std::back_inserter(Result));
+                              }
                               return !Result.empty();
                             });
   return Result;
 }
 
 // Expects Loc to be a SpellingLocation, will bail out otherwise as it can't
 // figure out a filename.
 llvm::Optional<Location> makeLocation(const ASTContext &AST, SourceLocation Loc,

 
 // Decls are more complicated.
 // The AST contains at least a declaration, maybe a definition.
 // These are up-to-date, and so generally preferred over index results.
 // We perform a single batch index lookup to find additional definitions.
 std::vector<LocatedSymbol>
 locateASTReferent(SourceLocation CurLoc, const syntax::Token *TouchedIdentifier,
                   ParsedAST &AST, llvm::StringRef MainFilePath,
-                  const SymbolIndex *Index) {
+                  const SymbolIndex *Index, ASTNodeKind *NodeKind) {
   const SourceManager &SM = AST.getSourceManager();
   // Results follow the order of Symbols.Decls.
   std::vector<LocatedSymbol> Result;
   // Keep track of SymbolID -> index mapping, to fill in index data later.
   llvm::DenseMap<SymbolID, size_t> ResultIndex;
 
   auto AddResultDecl = [&](const NamedDecl *D) {
     D = llvm::cast<NamedDecl>(D->getCanonicalDecl());
auto AddResultDecl = [&](const NamedDecl *D) {
     // Record SymbolID for index lookup later.
     if (auto ID = getSymbolID(D))
       ResultIndex[*ID] = Result.size() - 1;
   };
 
   // Emit all symbol locations (declaration or definition) from AST.
   DeclRelationSet Relations =
       DeclRelation::TemplatePattern | DeclRelation::Alias;
-  for (const NamedDecl *D : getDeclAtPosition(AST, CurLoc, Relations)) {
+  for (const NamedDecl *D :
+       getDeclAtPosition(AST, CurLoc, Relations, NodeKind)) {
     // Special case: void foo() ^override: jump to the overridden method.
     if (const auto *CMD = llvm::dyn_cast<CXXMethodDecl>(D)) {
       const InheritableAttr *Attr = D->getAttr<OverrideAttr>();
       if (!Attr)
         Attr = D->getAttr<FinalAttr>();
       if (Attr && TouchedIdentifier &&
           SM.getSpellingLoc(Attr->getLocation()) ==
               TouchedIdentifier->location()) {
llvm::StringRef sourcePrefix(SourceLocation Loc, const SourceManager &SM) {
   auto D = SM.getDecomposedLoc(Loc);
   bool Invalid = false;
   llvm::StringRef Buf = SM.getBufferData(D.first, &Invalid);
   if (Invalid || D.second > Buf.size())
     return "";
   return Buf.substr(0, D.second);
 }
 
+bool isDependentName(ASTNodeKind NodeKind) {
+  return NodeKind.isSame(ASTNodeKind::getFromNodeKind<OverloadExpr>()) ||
+         NodeKind.isSame(
+             ASTNodeKind::getFromNodeKind<CXXDependentScopeMemberExpr>()) ||
+         NodeKind.isSame(
+             ASTNodeKind::getFromNodeKind<DependentScopeDeclRefExpr>());
+}
+
 } // namespace
 
 std::vector<LocatedSymbol>
 locateSymbolTextually(const SpelledWord &Word, ParsedAST &AST,
-                      const SymbolIndex *Index,
-                      const std::string &MainFilePath) {
-  // Don't use heuristics if this is a real identifier, or not an identifier.
-  if (Word.ExpandedToken || !Word.LikelyIdentifier || !Index)
+                      const SymbolIndex *Index, const std::string &MainFilePath,
+                      ASTNodeKind NodeKind) {
+  // Don't use heuristics if this is a real identifier, or not an
+  // identifier.
+  // Exception: dependent names, because those may have useful textual
+  // matches that AST-based heuristics cannot find.
+  if ((Word.ExpandedToken && !isDependentName(NodeKind)) ||
+      !Word.LikelyIdentifier || !Index)
     return {};
   // We don't want to handle words in string literals. It'd be nice to whitelist
   // comments instead, but they're not retained in TokenBuffer.
   if (Word.PartOfSpelledToken &&
       isStringLiteral(Word.PartOfSpelledToken->kind()))
     return {};
 
   const auto &SM = AST.getSourceManager();
std::vector<LocatedSymbol> locateSymbolAt(ParsedAST &AST, Position Pos,
   if (TouchedIdentifier)
     if (auto Macro =
             locateMacroReferent(*TouchedIdentifier, AST, *MainFilePath))
       // Don't look at the AST or index if we have a macro result.
       // (We'd just return declarations referenced from the macro's
       // expansion.)
       return {*std::move(Macro)};
 
-  auto ASTResults =
-      locateASTReferent(*CurLoc, TouchedIdentifier, AST, *MainFilePath, Index);
+  ASTNodeKind NodeKind;
+  auto ASTResults = locateASTReferent(*CurLoc, TouchedIdentifier, AST,
+                                      *MainFilePath, Index, &NodeKind);
   if (!ASTResults.empty())
     return ASTResults;
 
   // If the cursor can't be resolved directly, try fallback strategies.
   auto Word =
       SpelledWord::touching(*CurLoc, AST.getTokens(), AST.getLangOpts());
   if (Word) {
     // Is the same word nearby a real identifier that might refer to something?
     if (const syntax::Token *NearbyIdent =
             findNearbyIdentifier(*Word, AST.getTokens())) {
       if (auto Macro = locateMacroReferent(*NearbyIdent, AST, *MainFilePath))
         return {*std::move(Macro)};
-      ASTResults = locateASTReferent(NearbyIdent->location(), NearbyIdent, AST,
-                                     *MainFilePath, Index);
+      ASTResults =
+          locateASTReferent(NearbyIdent->location(), NearbyIdent, AST,
+                            *MainFilePath, Index, /*NodeKind=*/nullptr);
       if (!ASTResults.empty())
         return ASTResults;
     }
     // No nearby word, or it didn't refer to anything either. Try the index.
     auto TextualResults =
-        locateSymbolTextually(*Word, AST, Index, *MainFilePath);
+        locateSymbolTextually(*Word, AST, Index, *MainFilePath, NodeKind);
     if (!TextualResults.empty())
       return TextualResults;
   }
 
   return {};
 }
 
 std::vector<DocumentLink> getDocumentLinks(ParsedAST &AST) {

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

const char *Tests[] = {
       R"cpp(// Macro definition
         struct [[MyClass]] {};
         #define DECLARE_MYCLASS_OBJ(name) M^yClass name;
       )cpp",
       R"cpp(// Invalid code
         /*error-ok*/
         int myFunction(int);
         // Not triggered for token which survived preprocessing.
         int var = m^yFunction();
-      )cpp",
-      R"cpp(// Dependent type
-        struct Foo {
-          void uniqueMethodName();
-        };
-        template <typename T>
-        void f(T t) {
-          // Not triggered for token which survived preprocessing.
-          t->u^niqueMethodName();
-        }
       )cpp"};
 
   for (const char *Test : Tests) {
     Annotations T(Test);
     llvm::Optional<Range> WantDecl;
     if (!T.ranges().empty())
       WantDecl = T.range();
 
     auto TU = TestTU::withCode(T.code());
 
     auto AST = TU.build();
     auto Index = TU.index();
     auto Word = SpelledWord::touching(
         cantFail(sourceLocationInMainFile(AST.getSourceManager(), T.point())),
         AST.getTokens(), AST.getLangOpts());
     if (!Word) {
       ADD_FAILURE() << "No word touching point!" << Test;
       continue;
     }
-    auto Results =
-        locateSymbolTextually(*Word, AST, Index.get(), testPath(TU.Filename));
+    auto Results = locateSymbolTextually(*Word, AST, Index.get(),
+                                         testPath(TU.Filename), ASTNodeKind());
 
     if (!WantDecl) {
       EXPECT_THAT(Results, IsEmpty()) << Test;
     } else {
       ASSERT_THAT(Results, ::testing::SizeIs(1)) << Test;
       EXPECT_EQ(Results[0].PreferredDeclaration.range, *WantDecl) << Test;
     }
   }
 } // namespace
 
 TEST(LocateSymbol, Ambiguous) {
   auto T = Annotations(R"cpp(
     struct Foo {
       Foo();
       Foo(Foo&&);
TEST(LocateSymbol, Ambiguous) {
   EXPECT_THAT(locateSymbolAt(AST, T.point("12")),
               UnorderedElementsAre(Sym("bar", T.range("NonstaticOverload1")),
                                    Sym("bar", T.range("NonstaticOverload2"))));
   EXPECT_THAT(locateSymbolAt(AST, T.point("13")),
               UnorderedElementsAre(Sym("baz", T.range("StaticOverload1")),
                                    Sym("baz", T.range("StaticOverload2"))));
 }
 
-TEST(LocateSymbol, TextualAmbiguous) {
-  auto T = Annotations(R"cpp(
+TEST(LocateSymbol, TextualDependent) {
+  // Put the declarations in the header to make sure we are
+  // finding them via the index heuristic and not the
+  // nearby-ident heuristic.
+  Annotations Header(R"cpp(
         struct Foo {
           void $FooLoc[[uniqueMethodName]]();
         };
         struct Bar {
           void $BarLoc[[uniqueMethodName]]();
         };
-        // Will call u^niqueMethodName() on t.
+        )cpp");
+  Annotations Source(R"cpp(
         template <typename T>
-        void f(T t);
+        void f(T t) {
+          t.u^niqueMethodName();
+        }
       )cpp");
-  auto TU = TestTU::withCode(T.code());
+  TestTU TU;
+  TU.Code = std::string(Source.code());
+  TU.HeaderCode = std::string(Header.code());
   auto AST = TU.build();
   auto Index = TU.index();
-  auto Word = SpelledWord::touching(
-      cantFail(sourceLocationInMainFile(AST.getSourceManager(), T.point())),
-      AST.getTokens(), AST.getLangOpts());
-  ASSERT_TRUE(Word);
-  auto Results =
-      locateSymbolTextually(*Word, AST, Index.get(), testPath(TU.Filename));
-  EXPECT_THAT(Results,
-              UnorderedElementsAre(Sym("uniqueMethodName", T.range("FooLoc")),
-                                   Sym("uniqueMethodName", T.range("BarLoc"))));
+  // Need to use locateSymbolAt() since we are testing an
+  // interaction between locateASTReferent() and
+  // locateSymbolNamedTextuallyAt().
+  auto Results = locateSymbolAt(AST, Source.point(), Index.get());
+  EXPECT_THAT(Results, UnorderedElementsAre(
+                           Sym("uniqueMethodName", Header.range("FooLoc")),
+                           Sym("uniqueMethodName", Header.range("BarLoc"))));
 }
 
 TEST(LocateSymbol, TemplateTypedefs) {
   auto T = Annotations(R"cpp(
     template <class T> struct function {};
     template <class T> using callback = function<T()>;
 
     c^allback<int> foo;