[clangd] Clean-up XRefs.cpp from Lexer usages and unnecessary SourceLoc transformations

[kadircet]

Summary

Get rid of calls to lexer and unnecessary source location
transformations.

[kadircet]

Add more cleaning.

getDeclAtPosition builds a SelectionTree underneath and it only operates on
spelling locations inside main file. So it is invalid to pass any expansion
locations to it.

There is no need to call getBeginningOfIdentifier for offsets being thrown at
SelectionTrees.

[kadircet]

• Also clean usage in getSymbolInfo

[kadircet]

• Handle llvm::Expected failures

[sammccall]

clang-tools-extra/clangd/XRefs.cpp

225

used only once - inline?

228–245

covering->touching

or just TouchedIdentifier for brevity

421–426

apparently we don't have tests for non-identifier cases, but they did work. let's keep them working for now :) CurLoc here

496

again, CurLoc?

[kadircet]

• Address comments

clang-tools-extra/clangd/XRefs.cpp

421–426

also get rid of the bail-out when no tokens were touched. (which means we can now possibly trigger on ^~Foo().)

496

ah thanks for catching this one especially, it also means we don't need to bail out when there are no identifiers touching cursor(it is only necessary for macros).
Updated it to do that, PTAL.

[sammccall]

clang-tools-extra/clangd/XRefs.cpp

421–426

can I bother you to add a test for this (or some non-identifier case, assuming any works)

[kadircet]

• Add tests for non-identifier cases.

clang-tools-extra/clangd/XRefs.cpp

 #include "clang/Index/USRGeneration.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
 // Returns the single definition of the entity declared by D, if visible.
if (!Inc.Resolved.empty() && Inc.R.start.line == Pos.line) {
       File.PreferredDeclaration = {
           URIForFile::canonicalize(Inc.Resolved, *MainFilePath), Range{}};
       File.Definition = File.PreferredDeclaration;
       // We're not going to find any further symbols on #include lines.
       return {std::move(File)};
     }
   }
 
+  auto CurLoc = sourceLocationInMainFile(SM, Pos);
+  if (!CurLoc) {
+    elog("locateSymbolAt failed to convert position to source location: {0}",
+         CurLoc.takeError());
+    return {};
+  }
+
   // Macros are simple: there's no declaration/definition distinction.
   // As a consequence, there's no need to look them up in the index either.
-  SourceLocation IdentStartLoc = SM.getMacroArgExpandedLocation(
-      getBeginningOfIdentifier(Pos, AST.getSourceManager(), AST.getLangOpts()));
   std::vector<LocatedSymbol> Result;
-  if (auto M = locateMacroAt(IdentStartLoc, AST.getPreprocessor())) {
+  const auto *TouchedIdentifier =
+      syntax::spelledIdentifierTouching(*CurLoc, AST.getTokens());
+  if (TouchedIdentifier) {
+    if (auto M = locateMacroAt(TouchedIdentifier->location(),
+                               AST.getPreprocessor())) {
       if (auto Loc = makeLocation(AST.getASTContext(),
                                   M->Info->getDefinitionLoc(), *MainFilePath)) {
         LocatedSymbol Macro;
         Macro.Name = std::string(M->Name);
         Macro.PreferredDeclaration = *Loc;
         Macro.Definition = Loc;
         Result.push_back(std::move(Macro));
 
         // 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 Result;
       }
     }
+  }
 
   // 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.
 
   // Results follow the order of Symbols.Decls.
   // Keep track of SymbolID -> index mapping, to fill in index data later.
   llvm::DenseMap<SymbolID, size_t> ResultIndex;
 
-  SourceLocation SourceLoc;
-  if (auto L = sourceLocationInMainFile(SM, Pos)) {
-    SourceLoc = *L;
-  } else {
-    elog("locateSymbolAt failed to convert position to source location: {0}",
-         L.takeError());
-    return Result;
-  }
-
   auto AddResultDecl = [&](const NamedDecl *D) {
     const NamedDecl *Def = getDefinition(D);
     const NamedDecl *Preferred = Def ? Def : D;
 
     auto Loc = makeLocation(AST.getASTContext(), nameLocation(*Preferred, SM),
                             *MainFilePath);
     if (!Loc)
       return;
 
     Result.emplace_back();
     Result.back().Name = printName(AST.getASTContext(), *Preferred);
     Result.back().PreferredDeclaration = *Loc;
     // Preferred is always a definition if possible, so this check works.
     if (Def == Preferred)
       Result.back().Definition = *Loc;
 
     // Record SymbolID for index lookup later.
     if (auto ID = getSymbolID(Preferred))
       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, SourceLoc, Relations)) {
+  for (const NamedDecl *D : getDeclAtPosition(AST, *CurLoc, Relations)) {
     // 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>();
+      const InheritableAttr *Attr = D->getAttr<OverrideAttr>();
       if (!Attr)
         Attr = D->getAttr<FinalAttr>();
-      const syntax::Token *Tok =
-          spelledIdentifierTouching(SourceLoc, AST.getTokens());
-      if (Attr && Tok &&
-          SM.getSpellingLoc(Attr->getLocation()) == Tok->location()) {
+      if (Attr && TouchedIdentifier &&
+          SM.getSpellingLoc(Attr->getLocation()) ==
+              TouchedIdentifier->location()) {
         // We may be overridding multiple methods - offer them all.
         for (const NamedDecl *ND : CMD->overridden_methods())
           AddResultDecl(ND);
         continue;
       }
     }
 
     // Special case: the point of declaration of a template specialization,
     // it's more useful to navigate to the template declaration.
-    if (SM.getMacroArgExpandedLocation(D->getLocation()) == IdentStartLoc) {
-      if (auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(D)) {
+      if (TouchedIdentifier &&
+          D->getLocation() == TouchedIdentifier->location()) {
         AddResultDecl(CTSD->getSpecializedTemplate());
         continue;
       }
     }
 
     // Otherwise the target declaration is the right one.
     AddResultDecl(D);
   }

 } // namespace
 
 std::vector<DocumentHighlight> findDocumentHighlights(ParsedAST &AST,
                                                       Position Pos) {
   const SourceManager &SM = AST.getSourceManager();
   // FIXME: show references to macro within file?
   DeclRelationSet Relations =
       DeclRelation::TemplatePattern | DeclRelation::Alias;
-  auto References = findRefs(
-      getDeclAtPosition(AST,
-                        SM.getMacroArgExpandedLocation(getBeginningOfIdentifier(
-                            Pos, SM, AST.getLangOpts())),
-                        Relations),
-      AST);
+  auto CurLoc = sourceLocationInMainFile(SM, Pos);
+  if (!CurLoc) {
+    llvm::consumeError(CurLoc.takeError());
+    return {};
+  }
+  auto References = findRefs(getDeclAtPosition(AST, *CurLoc, Relations), AST);
 
   // FIXME: we may get multiple DocumentHighlights with the same location and
   // different kinds, deduplicate them.
   std::vector<DocumentHighlight> Result;
   for (const auto &Ref : References) {
     if (auto Range =
             getTokenRange(AST.getSourceManager(), AST.getLangOpts(), Ref.Loc)) {
       DocumentHighlight DH;
ReferencesResult findReferences(ParsedAST &AST, Position Pos, uint32_t Limit,
   const SourceManager &SM = AST.getSourceManager();
   auto MainFilePath =
       getCanonicalPath(SM.getFileEntryForID(SM.getMainFileID()), SM);
   if (!MainFilePath) {
     elog("Failed to get a path for the main file, so no references");
     return Results;
   }
   auto URIMainFile = URIForFile::canonicalize(*MainFilePath, *MainFilePath);
-  auto Loc = SM.getMacroArgExpandedLocation(
-      getBeginningOfIdentifier(Pos, SM, AST.getLangOpts()));
+  auto CurLoc = sourceLocationInMainFile(SM, Pos);
+  if (!CurLoc) {
+    llvm::consumeError(CurLoc.takeError());
+    return {};
+  }
+  SourceLocation SLocId;
+  if (const auto *IdentifierAtCursor =
+          syntax::spelledIdentifierTouching(*CurLoc, AST.getTokens()))
+    SLocId = IdentifierAtCursor->location();
   RefsRequest Req;
 
-  if (auto Macro = locateMacroAt(Loc, AST.getPreprocessor())) {
+  if (auto Macro = locateMacroAt(SLocId, AST.getPreprocessor())) {
     // Handle references to macro.
     if (auto MacroSID = getSymbolID(Macro->Name, Macro->Info, SM)) {
       // Collect macro references from main file.
       const auto &IDToRefs = AST.getMacros().MacroRefs;
       auto Refs = IDToRefs.find(*MacroSID);
       if (Refs != IDToRefs.end()) {
         for (const auto Ref : Refs->second) {
           Location Result;
           Result.range = Ref;
           Result.uri = URIMainFile;
           Results.References.push_back(std::move(Result));
         }
       }
       Req.IDs.insert(*MacroSID);
     }
   } else {
     // Handle references to Decls.
 
     // We also show references to the targets of using-decls, so we include
     // DeclRelation::Underlying.
     DeclRelationSet Relations = DeclRelation::TemplatePattern |
                                 DeclRelation::Alias | DeclRelation::Underlying;
-    auto Decls = getDeclAtPosition(AST, Loc, Relations);
+    auto Decls = getDeclAtPosition(AST, *CurLoc, Relations);
 
     // We traverse the AST to find references in the main file.
     auto MainFileRefs = findRefs(Decls, AST);
     // We may get multiple refs with the same location and different Roles, as
     // cross-reference is only interested in locations, we deduplicate them
     // by the location to avoid emitting duplicated locations.
     MainFileRefs.erase(std::unique(MainFileRefs.begin(), MainFileRefs.end(),
                                    [](const ReferenceFinder::Reference &L,
                                       const ReferenceFinder::Reference &R) {
if (Results.References.size() > Limit) {
     Results.HasMore = true;
     Results.References.resize(Limit);
   }
   return Results;
 }
 
 std::vector<SymbolDetails> getSymbolInfo(ParsedAST &AST, Position Pos) {
   const SourceManager &SM = AST.getSourceManager();
-  auto Loc = SM.getMacroArgExpandedLocation(
-      getBeginningOfIdentifier(Pos, SM, AST.getLangOpts()));
+  auto CurLoc = sourceLocationInMainFile(SM, Pos);
+  if (!CurLoc) {
+    llvm::consumeError(CurLoc.takeError());
+    return {};
+  }
 
   std::vector<SymbolDetails> Results;
 
   // We also want the targets of using-decls, so we include
   // DeclRelation::Underlying.
   DeclRelationSet Relations = DeclRelation::TemplatePattern |
                               DeclRelation::Alias | DeclRelation::Underlying;
-  for (const NamedDecl *D : getDeclAtPosition(AST, Loc, Relations)) {
+  for (const NamedDecl *D : getDeclAtPosition(AST, *CurLoc, Relations)) {
     SymbolDetails NewSymbol;
     std::string QName = printQualifiedName(*D);
     auto SplitQName = splitQualifiedName(QName);
     NewSymbol.containerName = std::string(SplitQName.first);
     NewSymbol.name = std::string(SplitQName.second);
 
     if (NewSymbol.containerName.empty()) {
       if (const auto *ParentND =
               dyn_cast_or_null<NamedDecl>(D->getDeclContext()))
         NewSymbol.containerName = printQualifiedName(*ParentND);
     }
     llvm::SmallString<32> USR;
     if (!index::generateUSRForDecl(D, USR)) {
       NewSymbol.USR = std::string(USR.str());
       NewSymbol.ID = SymbolID(NewSymbol.USR);
     }
     Results.push_back(std::move(NewSymbol));
   }
 
-  if (auto M = locateMacroAt(Loc, AST.getPreprocessor())) {
+  const auto *IdentifierAtCursor =
+      syntax::spelledIdentifierTouching(*CurLoc, AST.getTokens());
+  if (!IdentifierAtCursor)
+    return Results;
+
+  if (auto M = locateMacroAt(IdentifierAtCursor->location(),
+                             AST.getPreprocessor())) {
     SymbolDetails NewMacro;
     NewMacro.name = std::string(M->Name);
     llvm::SmallString<32> USR;
     if (!index::generateUSRForMacro(NewMacro.name, M->Info->getDefinitionLoc(),
                                     SM, USR)) {
       NewMacro.USR = std::string(USR.str());
       NewMacro.ID = SymbolID(NewMacro.USR);
     }
auto RecordFromNode =
     // We don't handle FieldDecl because it's not clear what behaviour
     // the user would expect: the enclosing class type (as with a
     // method), or the field's type (as with a variable).
 
     return dyn_cast<CXXRecordDecl>(D);
   };
 
   const SourceManager &SM = AST.getSourceManager();
-  SourceLocation SourceLocationBeg = SM.getMacroArgExpandedLocation(
-      getBeginningOfIdentifier(Pos, SM, AST.getLangOpts()));
-  unsigned Offset = SM.getDecomposedSpellingLoc(SourceLocationBeg).second;
   const CXXRecordDecl *Result = nullptr;
-  SelectionTree::createEach(AST.getASTContext(), AST.getTokens(), Offset,
-                            Offset, [&](SelectionTree ST) {
+  auto Offset = positionToOffset(SM.getBufferData(SM.getMainFileID()), Pos);
+  if (!Offset) {
+    llvm::consumeError(Offset.takeError());
+    return Result;
+  }
+  SelectionTree::createEach(AST.getASTContext(), AST.getTokens(), *Offset,
+                            *Offset, [&](SelectionTree ST) {
                               Result = RecordFromNode(ST.commonAncestor());
                               return Result != nullptr;
                             });
   return Result;
-
 }
 
 std::vector<const CXXRecordDecl *> typeParents(const CXXRecordDecl *CXXRD) {
   std::vector<const CXXRecordDecl *> Result;
 
   // If this is an invalid instantiation, instantiation of the bases
   // may not have succeeded, so fall back to the template pattern.
   if (auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(CXXRD)) {

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

const char *Tests[] = {
           [[foo]]([[foo]](42));
           auto *X = &[[foo]];
         }
       )cpp",
 
       R"cpp(// Function parameter in decl
         void foo(int [[^bar]]);
       )cpp",
+      R"cpp(// Not touching any identifiers.
+        struct Foo {
+          [[~]]Foo() {};
+        };
+        void foo() {
+          Foo f;
+          f.[[^~]]Foo();
+        }
+      )cpp",
   };
   for (const char *Test : Tests) {
     Annotations T(Test);
     auto AST = TestTU::withCode(T.code()).build();
     EXPECT_THAT(findDocumentHighlights(AST, T.point()), HighlightsFrom(T))
         << Test;
   }
 }
const char *Tests[] = {
        void func([[Foo]]<int>);
       )cpp",
 
       R"cpp(
        template <typename T>
        class [[Foo]] {};
        void func([[Fo^o]]<int>);
       )cpp",
+      R"cpp(// Not touching any identifiers.
+        struct Foo {
+          [[~]]Foo() {};
+        };
+        void foo() {
+          Foo f;
+          f.[[^~]]Foo();
+        }
+      )cpp",
   };
   for (const char *Test : Tests) {
     Annotations T(Test);
     auto AST = TestTU::withCode(T.code()).build();
     std::vector<Matcher<Location>> ExpectedLocations;
     for (const auto &R : T.ranges())
       ExpectedLocations.push_back(RangeIs(R));
     EXPECT_THAT(findReferences(AST, T.point(), 0).References,