[clangd] Remove TokenBuffer usage in TypeHierarchy

[ArcsinX]

Summary

This patch mostly reverts D74850.
We could not use AST.getTokens() here, because it does not have tokens from the preamble.

[kadircet]

ah embarrassing :/ thanks for catching this! It should've been obvious from the fact that there's a "FilePath" and a "TUPath" going on ..

clang-tools-extra/clangd/XRefs.cpp

1191

let's rename it to DeclRange rather than NameRange.

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

550

you can keep using the TestTU. just provide the header via AdditionalFiles and TU.build should first build a preamble and use it. That way you can get rid of additions to the SyncAPI and usage of ClangdServer in here.

[ArcsinX]

NameRange => DeclRange
Simplify TypeHierarchy.Preamble test.

[ArcsinX]

clang-tools-extra/clangd/XRefs.cpp

1191

Renamed.

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

550

Thanks, done.

[kadircet]

LGTM, just some improvements to the testing. also please let me know if I should land this for you.

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

533

this doesn't need to be an Annotation and sorry for missing it in the first pass but since this only has a single header, you can actually do something like:

TestTU TU = TestTU::withCode(SourceAnnotations.code());
TU.HeaderCode = "struct Parent { int a; }";

and also drop the include directive in SourceAnnotations as TU.HeaderCode is implicitly included.

548

could you also make sure selection range is correct at least for Parent (as main file ranges are tested elsewhere already), so that we don't regress it in the future.

[ArcsinX]

Check source range for Parent

[ArcsinX]

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

533

Seems now I need it to check selection range.

548

Added Parent selection range check.

[ArcsinX]

In D84144#2161579, @kadircet wrote:

also please let me know if I should land this for you.

Yes, could you please land this for me? I do not have commit access.
Aleksandr Platonov <[e-mail]>

[kadircet]

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

533

right, you need annotations for the range, but you still can get away with just setting TU.HeaderCode instead of populating AdditionalFiles and including the header in the source. but not that important.

[ArcsinX]

AdditionalFiles["header_in_preamble.h"] => HeaderCode

[ArcsinX]

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

533

Thanks. fixed

clang-tools-extra/clangd/XRefs.cpp

llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const LocatedSymbol &S) {
   OS << S.Name << ": " << S.PreferredDeclaration;
   if (S.Definition)
     OS << " def=" << *S.Definition;
   return OS;
 }
 
 // FIXME(nridge): Reduce duplication between this function and declToSym().
 static llvm::Optional<TypeHierarchyItem>
-declToTypeHierarchyItem(ASTContext &Ctx, const NamedDecl &ND,
-                        const syntax::TokenBuffer &TB) {
+declToTypeHierarchyItem(ASTContext &Ctx, const NamedDecl &ND) {
   auto &SM = Ctx.getSourceManager();
   SourceLocation NameLoc = nameLocation(ND, Ctx.getSourceManager());
+  SourceLocation BeginLoc = SM.getSpellingLoc(SM.getFileLoc(ND.getBeginLoc()));
+  SourceLocation EndLoc = SM.getSpellingLoc(SM.getFileLoc(ND.getEndLoc()));
+  const auto DeclRange =
+      toHalfOpenFileRange(SM, Ctx.getLangOpts(), {BeginLoc, EndLoc});
+  if (!DeclRange)
+    return llvm::None;
   auto FilePath =
       getCanonicalPath(SM.getFileEntryForID(SM.getFileID(NameLoc)), SM);
   auto TUPath = getCanonicalPath(SM.getFileEntryForID(SM.getMainFileID()), SM);
   if (!FilePath || !TUPath)
     return llvm::None; // Not useful without a uri.
 
-  auto DeclToks = TB.spelledForExpanded(TB.expandedTokens(ND.getSourceRange()));
-  if (!DeclToks || DeclToks->empty())
-    return llvm::None;
-
-  auto NameToks = TB.spelledForExpanded(TB.expandedTokens(NameLoc));
-  if (!NameToks || NameToks->empty())
-    return llvm::None;
+  Position NameBegin = sourceLocToPosition(SM, NameLoc);
+  Position NameEnd = sourceLocToPosition(
+      SM, Lexer::getLocForEndOfToken(NameLoc, 0, SM, Ctx.getLangOpts()));
 
   index::SymbolInfo SymInfo = index::getSymbolInfo(&ND);
   // FIXME: this is not classifying constructors, destructors and operators
   //        correctly (they're all "methods").
   SymbolKind SK = indexSymbolKindToSymbolKind(SymInfo.Kind);
 
   TypeHierarchyItem THI;
   THI.name = printName(Ctx, ND);
   THI.kind = SK;
   THI.deprecated = ND.isDeprecated();
-  THI.range = halfOpenToRange(
-      SM, syntax::Token::range(SM, DeclToks->front(), DeclToks->back())
-              .toCharRange(SM));
-  THI.selectionRange = halfOpenToRange(
-      SM, syntax::Token::range(SM, NameToks->front(), NameToks->back())
-              .toCharRange(SM));
+  THI.range = Range{sourceLocToPosition(SM, DeclRange->getBegin()),
+                    sourceLocToPosition(SM, DeclRange->getEnd())};
+  THI.selectionRange = Range{NameBegin, NameEnd};
   if (!THI.range.contains(THI.selectionRange)) {
     // 'selectionRange' must be contained in 'range', so in cases where clang
     // reports unrelated ranges we need to reconcile somehow.
     THI.range = THI.selectionRange;
   }
 
   THI.uri = URIForFile::canonicalize(*FilePath, *TUPath);
 
Index->relations(Req, [&](const SymbolID &Subject, const Symbol &Object) {
     }
   });
 }
 
 using RecursionProtectionSet = llvm::SmallSet<const CXXRecordDecl *, 4>;
 
 static void fillSuperTypes(const CXXRecordDecl &CXXRD, ASTContext &ASTCtx,
                            std::vector<TypeHierarchyItem> &SuperTypes,
-                           RecursionProtectionSet &RPSet,
-                           const syntax::TokenBuffer &TB) {
+                           RecursionProtectionSet &RPSet) {
   // typeParents() will replace dependent template specializations
   // with their class template, so to avoid infinite recursion for
   // certain types of hierarchies, keep the templates encountered
   // along the parent chain in a set, and stop the recursion if one
   // starts to repeat.
   auto *Pattern = CXXRD.getDescribedTemplate() ? &CXXRD : nullptr;
   if (Pattern) {
     if (!RPSet.insert(Pattern).second) {
       return;
     }
   }
 
   for (const CXXRecordDecl *ParentDecl : typeParents(&CXXRD)) {
     if (Optional<TypeHierarchyItem> ParentSym =
-            declToTypeHierarchyItem(ASTCtx, *ParentDecl, TB)) {
+            declToTypeHierarchyItem(ASTCtx, *ParentDecl)) {
       ParentSym->parents.emplace();
-      fillSuperTypes(*ParentDecl, ASTCtx, *ParentSym->parents, RPSet, TB);
+      fillSuperTypes(*ParentDecl, ASTCtx, *ParentSym->parents, RPSet);
       SuperTypes.emplace_back(std::move(*ParentSym));
     }
   }
 
   if (Pattern) {
     RPSet.erase(Pattern);
   }
 }

 getTypeHierarchy(ParsedAST &AST, Position Pos, int ResolveLevels,
                  TypeHierarchyDirection Direction, const SymbolIndex *Index,
                  PathRef TUPath) {
   const CXXRecordDecl *CXXRD = findRecordTypeAt(AST, Pos);
   if (!CXXRD)
     return llvm::None;
 
   Optional<TypeHierarchyItem> Result =
-      declToTypeHierarchyItem(AST.getASTContext(), *CXXRD, AST.getTokens());
+      declToTypeHierarchyItem(AST.getASTContext(), *CXXRD);
   if (!Result)
     return Result;
 
   if (Direction == TypeHierarchyDirection::Parents ||
       Direction == TypeHierarchyDirection::Both) {
     Result->parents.emplace();
 
     RecursionProtectionSet RPSet;
-    fillSuperTypes(*CXXRD, AST.getASTContext(), *Result->parents, RPSet,
-                   AST.getTokens());
+    fillSuperTypes(*CXXRD, AST.getASTContext(), *Result->parents, RPSet);
   }
 
   if ((Direction == TypeHierarchyDirection::Children ||
        Direction == TypeHierarchyDirection::Both) &&
       ResolveLevels > 0) {
     Result->children.emplace();
 
     if (Index) {

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

llvm::Optional<TypeHierarchyItem> Result = getTypeHierarchy(
       testPath(TU.Filename));
   ASSERT_TRUE(bool(Result));
   EXPECT_THAT(*Result,
               AllOf(WithName("Parent<int>"), WithKind(SymbolKind::Struct),
                     Children(AllOf(WithName("Child"),
                                    WithKind(SymbolKind::Struct), Children()))));
 }
 
+TEST(TypeHierarchy, Preamble) {
+  Annotations SourceAnnotations(R"cpp(
+struct Ch^ild : Parent {
+  int b;
+};)cpp");
+
+  Annotations HeaderInPreambleAnnotations(R"cpp(
+struct [[Parent]] {
+  int a;
+};)cpp");
+
+  TestTU TU = TestTU::withCode(SourceAnnotations.code());
+  TU.HeaderCode = HeaderInPreambleAnnotations.code().str();
+  auto AST = TU.build();
+
+  llvm::Optional<TypeHierarchyItem> Result = getTypeHierarchy(
+      AST, SourceAnnotations.point(), 1, TypeHierarchyDirection::Parents);
+
+  ASSERT_TRUE(Result);
+  EXPECT_THAT(
+      *Result,
+      AllOf(WithName("Child"),
+            Parents(AllOf(WithName("Parent"),
+                          SelectionRangeIs(HeaderInPreambleAnnotations.range()),
+                          Parents()))));
+}
+
 SymbolID findSymbolIDByName(SymbolIndex *Index, llvm::StringRef Name,
                             llvm::StringRef TemplateArgs = "") {
   SymbolID Result;
   FuzzyFindRequest Request;
   Request.Query = std::string(Name);
   Request.AnyScope = true;
   bool GotResult = false;
   Index->fuzzyFind(Request, [&](const Symbol &S) {