[clangd] Get rid of getTokenRange helper

[kadircet]

• Also get rid of the call in collectmacros by using clang::Token's endlocation instead.

[sammccall]

clang-tools-extra/clangd/Hover.cpp

542

nit: back() would fit better with the biases elsewhere

clang-tools-extra/clangd/XRefs.cpp

155–160

so this is just running the raw lexer...

both callers have a ParsedAST, can we call spelledTokenAt()? Either here, passing in (TokenBuffer, Loc, TUPath) or in the caller, passing in (syntax::Token*, SourceMgr, TUPath).

155–160

It *seems* to always be the case that TokLoc is a spelled token, but this seems slightly subtle and the previous code defended against marco locations. Worth a comment

355

document spelled or expanded.

It seems to be spelled, but I don't know why - expanded is more similar to the old code, allows us to defer more work, and doesn't require the dubious front().

You could add a function here range(SourceMgr) --> Range

396

why is it safe to discard the rest of toks?

[sammccall]

clang-tools-extra/clangd/XRefs.cpp

155–160

can we call spelledTokenAt

as you pointed out offline, we only have spelled tokens for the main file.
I think we have to lex here to get the token length, and the clearest thing is to do this completely directly (Lexer::measureTokenLength -> Loc.getLocWithOffset) rather than obfuscating with tokenbuffer.
We should have a comment *why* we lex here.

[kadircet]

• Use getFileLoc instead of spelledForExpanded

clang-tools-extra/clangd/XRefs.cpp

155–160

so this is just running the raw lexer...
both callers have a ParsedAST, can we call spelledTokenAt()? Either here, passing in (TokenBuffer, Loc, TUPath) or in the caller, passing in (syntax::Token*, SourceMgr, TUPath).

as discussed offline the problem is TokenBuffer won't hold spelled tokens for included files. so if you've got a macro definition or declaration coming from a header, spelledTokenAt will fail.
making use of Lexer::MeasureTokenLength instead.

155–160

as discussed offline, in case of macro locations we'll fail at getting the file path and stop there and that logic is the same.

[sammccall]

clang-tools-extra/clangd/XRefs.cpp

172

(nit: consider extracting a variable for end loc or token length just for readability)

442

Ref.range(SM)

518

Ref.range(SM)

[kadircet]

• Address comments

clang-tools-extra/clangd/CMakeLists.txt

 
 add_clang_library(clangDaemon
   AST.cpp
   Cancellation.cpp
   ClangdLSPServer.cpp
   ClangdServer.cpp
   CodeComplete.cpp
   CodeCompletionStrings.cpp
+  CollectMacros.cpp
   CompileCommands.cpp
   Compiler.cpp
   Context.cpp
   Diagnostics.cpp
   DraftStore.cpp
   ExpectedTypes.cpp
   FindSymbols.cpp
   FindTarget.cpp

clang-tools-extra/clangd/CollectMacros.h

void SourceRangeSkipped(SourceRange R, SourceLocation EndifLoc) override {
     if (!InMainFile)
       return;
     Position Begin = sourceLocToPosition(SM, R.getBegin());
     Position End = sourceLocToPosition(SM, R.getEnd());
     Out.SkippedRanges.push_back(Range{Begin, End});
   }
 
 private:
-  void add(const Token &MacroNameTok, const MacroInfo *MI) {
-    if (!InMainFile)
-      return;
-    auto Loc = MacroNameTok.getLocation();
-    if (Loc.isMacroID())
-      return;
-
-    if (auto Range = getTokenRange(SM, LangOpts, Loc)) {
-      auto Name = MacroNameTok.getIdentifierInfo()->getName();
-      Out.Names.insert(Name);
-      if (auto SID = getSymbolID(Name, MI, SM))
-        Out.MacroRefs[*SID].push_back(*Range);
-      else
-        Out.UnknownMacros.push_back(*Range);
-    }
-  }
+  void add(const Token &MacroNameTok, const MacroInfo *MI);
   const SourceManager &SM;
   const LangOptions &LangOpts;
   bool InMainFile = true;
   MainFileMacros &Out;
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_COLLECTEDMACROS_H

clang-tools-extra/clangd/CollectMacros.cpp

+//===--- CollectMacros.cpp ---------------------------------------*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "CollectMacros.h"
+#include "clang/Lex/Lexer.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+/// Returns the taken range at \p TokLoc.
+llvm::Optional<Range> getTokenRange(const SourceManager &SM,
+                                    const LangOptions &LangOpts,
+                                    SourceLocation TokLoc) {
+  if (!TokLoc.isValid())
+    return llvm::None;
+  SourceLocation End = Lexer::getLocForEndOfToken(TokLoc, 0, SM, LangOpts);
+  if (!End.isValid())
+    return llvm::None;
+  return halfOpenToRange(SM, CharSourceRange::getCharRange(TokLoc, End));
+}
+} // namespace
+
+void CollectMainFileMacros::add(const Token &MacroNameTok,
+                                const MacroInfo *MI) {
+  if (!InMainFile)
+    return;
+  auto Loc = MacroNameTok.getLocation();
+  if (Loc.isMacroID())
+    return;
+
+  if (auto Range = getTokenRange(SM, LangOpts, Loc)) {
+    auto Name = MacroNameTok.getIdentifierInfo()->getName();
+    Out.Names.insert(Name);
+    if (auto SID = getSymbolID(Name, MI, SM))
+      Out.MacroRefs[*SID].push_back(*Range);
+    else
+      Out.UnknownMacros.push_back(*Range);
+  }
+}
+} // namespace clangd
+} // namespace clang

clang-tools-extra/clangd/Hover.cpp

 #include "clang/AST/ASTTypeTraits.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/Type.h"
+#include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/Specifiers.h"
 #include "clang/Basic/TokenKinds.h"
 #include "clang/Index/IndexSymbol.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
llvm::Optional<HoverInfo> getHover(ParsedAST &AST, Position Pos,
                                    format::FormatStyle Style,
                                    const SymbolIndex *Index) {
   const SourceManager &SM = AST.getSourceManager();
   auto CurLoc = sourceLocationInMainFile(SM, Pos);
   if (!CurLoc) {
     llvm::consumeError(CurLoc.takeError());
     return llvm::None;
   }
-  auto TokensTouchingCursor =
-      syntax::spelledTokensTouching(*CurLoc, AST.getTokens());
+  const auto &TB = AST.getTokens();
+  auto TokensTouchingCursor = syntax::spelledTokensTouching(*CurLoc, TB);
   // Early exit if there were no tokens around the cursor.
   if (TokensTouchingCursor.empty())
     return llvm::None;
 
   // To be used as a backup for highlighting the selected token.
-  SourceLocation IdentLoc;
+  CharSourceRange HighlightRange =
+      TokensTouchingCursor.front().range(SM).toCharRange(SM);
   llvm::Optional<HoverInfo> HI;
   // Macros and deducedtype only works on identifiers and auto/decltype keywords
   // respectively. Therefore they are only trggered on whichever works for them,
   // similar to SelectionTree::create().
   for (const auto &Tok : TokensTouchingCursor) {
     if (Tok.kind() == tok::identifier) {
-      IdentLoc = Tok.location();
+      // Prefer the identifier token as a fallback highlighting range.
+      HighlightRange = Tok.range(SM).toCharRange(SM);
       if (auto M = locateMacroAt(Tok, AST.getPreprocessor())) {
         HI = getHoverContents(*M, AST);
-        HI->SymRange = getTokenRange(AST.getSourceManager(), AST.getLangOpts(),
-                                     Tok.location());
         break;
       }
     } else if (Tok.kind() == tok::kw_auto || Tok.kind() == tok::kw_decltype) {
       if (auto Deduced = getDeducedType(AST.getASTContext(), Tok.location())) {
         HI = getHoverContents(*Deduced, AST.getASTContext(), Index);
-        HI->SymRange = getTokenRange(AST.getSourceManager(), AST.getLangOpts(),
-                                     Tok.location());
+        HighlightRange = Tok.range(SM).toCharRange(SM);
         break;
       }
     }
   }
 
   // If it wasn't auto/decltype or macro, look for decls and expressions.
   if (!HI) {
     auto Offset = SM.getFileOffset(*CurLoc);
     // Editors send the position on the left of the hovered character.
     // So our selection tree should be biased right. (Tested with VSCode).
-    SelectionTree ST = SelectionTree::createRight(
-        AST.getASTContext(), AST.getTokens(), Offset, Offset);
+    SelectionTree ST =
+        SelectionTree::createRight(AST.getASTContext(), TB, Offset, Offset);
     std::vector<const Decl *> Result;
     if (const SelectionTree::Node *N = ST.commonAncestor()) {
+      // FIXME: Fill in HighlightRange with range coming from N->ASTNode.
       auto Decls = explicitReferenceTargets(N->ASTNode, DeclRelation::Alias);
       if (!Decls.empty()) {
         HI = getHoverContents(Decls.front(), Index);
         // Look for a close enclosing expression to show the value of.
         if (!HI->Value)
           HI->Value = printExprValue(N, AST.getASTContext());
       } else if (const Expr *E = N->ASTNode.get<Expr>()) {
         HI = getHoverContents(E, AST);
       }
       // FIXME: support hovers for other nodes?
       //  - built-in types
     }
   }
 
   if (!HI)
     return llvm::None;
 
   auto Replacements = format::reformat(
       Style, HI->Definition, tooling::Range(0, HI->Definition.size()));
   if (auto Formatted =
           tooling::applyAllReplacements(HI->Definition, Replacements))
     HI->Definition = *Formatted;
-  // FIXME: We should rather fill this with info coming from SelectionTree node.
-  if (!HI->SymRange) {
-    SourceLocation ToHighlight = TokensTouchingCursor.front().location();
-    if (IdentLoc.isValid())
-      ToHighlight = IdentLoc;
-    HI->SymRange =
-        getTokenRange(AST.getSourceManager(), AST.getLangOpts(), ToHighlight);
-  }
+  HI->SymRange = halfOpenToRange(SM, HighlightRange);
 
   return HI;
 }
 
 markup::Document HoverInfo::present() const {
   markup::Document Output;
   // Header contains a text of the form:
   // variable `var`

clang-tools-extra/clangd/SourceCode.h

 /// Turn an offset in Code into a [line, column] pair.
 /// The offset must be in range [0, Code.size()].
 Position offsetToPosition(llvm::StringRef Code, size_t Offset);
 
 /// Turn a SourceLocation into a [line, column] pair.
 /// FIXME: This should return an error if the location is invalid.
 Position sourceLocToPosition(const SourceManager &SM, SourceLocation Loc);
 
-/// Returns the taken range at \p TokLoc.
-llvm::Optional<Range> getTokenRange(const SourceManager &SM,
-                                    const LangOptions &LangOpts,
-                                    SourceLocation TokLoc);
-
 /// Return the file location, corresponding to \p P. Note that one should take
 /// care to avoid comparing the result with expansion locations.
 llvm::Expected<SourceLocation> sourceLocationInMainFile(const SourceManager &SM,
                                                         Position P);
 
 /// Returns true iff \p Loc is inside the main file. This function handles
 /// file & macro locations. For macro locations, returns iff the macro is being
 /// expanded inside the main file.

clang-tools-extra/clangd/SourceCode.cpp

if (Loc.isMacroID()) {
     std::string PrintLoc = SM.getSpellingLoc(Loc).printToString(SM);
     if (llvm::StringRef(PrintLoc).startswith("<scratch") ||
         llvm::StringRef(PrintLoc).startswith("<command line>"))
       return false;
   }
   return true;
 }
 
-llvm::Optional<Range> getTokenRange(const SourceManager &SM,
-                                    const LangOptions &LangOpts,
-                                    SourceLocation TokLoc) {
-  if (!TokLoc.isValid())
-    return llvm::None;
-  SourceLocation End = Lexer::getLocForEndOfToken(TokLoc, 0, SM, LangOpts);
-  if (!End.isValid())
-    return llvm::None;
-  return halfOpenToRange(SM, CharSourceRange::getCharRange(TokLoc, End));
-}
-
 bool isValidFileRange(const SourceManager &Mgr, SourceRange R) {
   if (!R.getBegin().isValid() || !R.getEnd().isValid())
     return false;
 
   FileID BeginFID;
   size_t BeginOffset = 0;
   std::tie(BeginFID, BeginOffset) = Mgr.getDecomposedLoc(R.getBegin());
 

 std::vector<Range> collectIdentifierRanges(llvm::StringRef Identifier,
                                            llvm::StringRef Content,
                                            const LangOptions &LangOpts) {
   std::vector<Range> Ranges;
   lex(Content, LangOpts,
       [&](const syntax::Token &Tok, const SourceManager &SM) {
         if (Tok.kind() != tok::identifier || Tok.text(SM) != Identifier)
           return;
-        if (auto Range = getTokenRange(SM, LangOpts, Tok.location()))
-          Ranges.push_back(*Range);
+        Ranges.push_back(halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
       });
   return Ranges;
 }
 
 namespace {
 struct NamespaceEvent {
   enum {
     BeginNamespace, // namespace <ns> {.     Payload is resolved <ns>.

clang-tools-extra/clangd/XRefs.cpp

SelectionTree::createEach(AST.getASTContext(), AST.getTokens(), Offset,
                               return !Result.empty();
                             });
   return Result;
 }
 
 llvm::Optional<Location> makeLocation(ASTContext &AST, SourceLocation TokLoc,
                                       llvm::StringRef TUPath) {
   const SourceManager &SourceMgr = AST.getSourceManager();
-  const FileEntry *F = SourceMgr.getFileEntryForID(SourceMgr.getFileID(TokLoc));
+  auto Tok = syntax::tokenize(syntax::FileRange(SourceMgr, TokLoc, 1),
+                              SourceMgr, AST.getLangOpts());
+  if (Tok.empty())
+    return None;
+  auto Range = Tok.front().range(SourceMgr);
+  const FileEntry *F = SourceMgr.getFileEntryForID(Range.file());
   if (!F)
     return None;
   auto FilePath = getCanonicalPath(F, SourceMgr);
   if (!FilePath) {
     log("failed to get path!");
     return None;
   }
-  if (auto Range =
-          getTokenRange(AST.getSourceManager(), AST.getLangOpts(), TokLoc)) {
-    Location L;
-    L.uri = URIForFile::canonicalize(*FilePath, TUPath);
-    L.range = *Range;
-    return L;
-  }
-  return None;
-}
+  L.range = halfOpenToRange(SourceMgr, Range.toCharRange(SourceMgr));
 
 } // namespace
 
 std::vector<DocumentLink> getDocumentLinks(ParsedAST &AST) {
   const auto &SM = AST.getSourceManager();
   auto MainFilePath =
       getCanonicalPath(SM.getFileEntryForID(SM.getMainFileID()), SM);
   if (!MainFilePath) {

 }
 
 namespace {
 
 /// Collects references to symbols within the main file.
 class ReferenceFinder : public index::IndexDataConsumer {
 public:
   struct Reference {
-    SourceLocation Loc;
+    syntax::Token Tok;
     index::SymbolRoleSet Role;
   };
 
-  ReferenceFinder(ASTContext &AST, Preprocessor &PP,
+  ReferenceFinder(const ParsedAST &AST,
                   const std::vector<const NamedDecl *> &TargetDecls)
       : AST(AST) {
     for (const NamedDecl *D : TargetDecls)
       CanonicalTargets.insert(D->getCanonicalDecl());
   }
 
   std::vector<Reference> take() && {
     llvm::sort(References, [](const Reference &L, const Reference &R) {
-      return std::tie(L.Loc, L.Role) < std::tie(R.Loc, R.Role);
+      auto LTok = L.Tok.location();
+      auto RTok = R.Tok.location();
+      return std::tie(LTok, L.Role) < std::tie(RTok, R.Role);
     });
     // We sometimes see duplicates when parts of the AST get traversed twice.
     References.erase(std::unique(References.begin(), References.end(),
                                  [](const Reference &L, const Reference &R) {
-                                   return std::tie(L.Loc, L.Role) ==
-                                          std::tie(R.Loc, R.Role);
+                                   auto LTok = L.Tok.location();
+                                   auto RTok = R.Tok.location();
+                                   return std::tie(LTok, L.Role) ==
+                                          std::tie(RTok, R.Role);
                                  }),
                      References.end());
     return std::move(References);
   }
 
   bool
   handleDeclOccurrence(const Decl *D, index::SymbolRoleSet Roles,
                        llvm::ArrayRef<index::SymbolRelation> Relations,
                        SourceLocation Loc,
                        index::IndexDataConsumer::ASTNodeInfo ASTNode) override {
     assert(D->isCanonicalDecl() && "expect D to be a canonical declaration");
+    if (!CanonicalTargets.count(D))
+      return true;
+    const auto &TB = AST.getTokens();
     const SourceManager &SM = AST.getSourceManager();
-    Loc = SM.getFileLoc(Loc);
-    if (isInsideMainFile(Loc, SM) && CanonicalTargets.count(D))
-      References.push_back({Loc, Roles});
+    auto Toks = TB.spelledForExpanded(TB.expandedTokens(Loc));
+    if (!Toks || Toks->empty() ||
+        !isInsideMainFile(Toks->front().location(), SM))
+      return true;
+    References.push_back({Toks->front(), Roles});
     return true;
   }
 
 private:
   llvm::SmallSet<const Decl *, 4> CanonicalTargets;
   std::vector<Reference> References;
-  const ASTContext &AST;
+  const ParsedAST &AST;
 };
 
 std::vector<ReferenceFinder::Reference>
 findRefs(const std::vector<const NamedDecl *> &Decls, ParsedAST &AST) {
-  ReferenceFinder RefFinder(AST.getASTContext(), AST.getPreprocessor(), Decls);
+  ReferenceFinder RefFinder(AST, Decls);
   index::IndexingOptions IndexOpts;
   IndexOpts.SystemSymbolFilter =
       index::IndexingOptions::SystemSymbolFilterKind::All;
   IndexOpts.IndexFunctionLocals = true;
   IndexOpts.IndexParametersInDeclarations = true;
   IndexOpts.IndexTemplateParameters = true;
   indexTopLevelDecls(AST.getASTContext(), AST.getPreprocessor(),
                      AST.getLocalTopLevelDecls(), RefFinder, IndexOpts);
if (!CurLoc) {
     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;
-      DH.range = *Range;
-      if (Ref.Role & index::SymbolRoleSet(index::SymbolRole::Write))
-        DH.kind = DocumentHighlightKind::Write;
-      else if (Ref.Role & index::SymbolRoleSet(index::SymbolRole::Read))
-        DH.kind = DocumentHighlightKind::Read;
-      else
-        DH.kind = DocumentHighlightKind::Text;
-      Result.push_back(std::move(DH));
-    }
-  }
+    DH.range = halfOpenToRange(SM, Ref.Tok.range(SM).toCharRange(SM));
   return Result;
 }
 
 ReferencesResult findReferences(ParsedAST &AST, Position Pos, uint32_t Limit,
                                 const SymbolIndex *Index) {
   if (!Limit)
     Limit = std::numeric_limits<uint32_t>::max();
   ReferencesResult Results;
if (Macro) {
     // 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) {
-                                     return L.Loc == R.Loc;
+                                     return L.Tok.location() ==
+                                            R.Tok.location();
                                    }),
                        MainFileRefs.end());
     for (const auto &Ref : MainFileRefs) {
-      if (auto Range = getTokenRange(SM, AST.getLangOpts(), Ref.Loc)) {
-        Location Result;
-        Result.range = *Range;
-        Result.uri = URIMainFile;
-        Results.References.push_back(std::move(Result));
-      }
-    }
+      Result.range = halfOpenToRange(SM, Ref.Tok.range(SM).toCharRange(SM));
     if (Index && Results.References.size() <= Limit) {
       for (const Decl *D : Decls) {
         // Not all symbols can be referenced from outside (e.g.
         // function-locals).
         // TODO: we could skip TU-scoped symbols here (e.g. static functions) if
         // we know this file isn't a header. The details might be tricky.
         if (D->getParentFunctionOrMethod())
           continue;