[clangd] Revert the symbol collector behavior to old pre-include-cleaner-library behavior due to a regression.

[kadircet]

clang-tools-extra/clangd/index/SymbolCollector.cpp

841–858

can you also get rid of this piece, and SymbolProviders map ?

883–884

we can drop this too

910–934

rather than duplicating some of the checks, can we rewrite this block as:

llvm::StringRef IncludeHeader = getStdlibHeader(*S, ASTCtx->getLangOpts());
if (IncludeHeader.empty())
  IncludeHeader = HeaderFileURIs->getIncludeHeader(FID);
if (!IncludeHeader.empty()) {
      auto NewSym = *S;
      NewSym.IncludeHeaders.push_back({IncludeHeader, 1, Directives});
      Symbols.insert(NewSym);
}

912

let's pass in the language options from ASTCtx into here

915–919

i think the old order of, first dealing with std::move and then using tooling::stdlib is less error-prone and the order in which we apply the mapping in include-cleaner, so that would be a more compatible change going forward.

919–954

let's drop this block to not create confusion

[VitaNuo]

Address comments.

[kadircet]

clang-tools-extra/clangd/index/SymbolCollector.cpp

910

we shouldn't be overriding the include header if it's already set above. also i think it would be easier to have this out of the way so that reverting to old behaviour is easier:

llvm::StringRef getStdHeader(const Symbol *S, const LanguageOptions &LO) {
  tooling::stdlib::Lang Lang = tooling::stdlib::Lang::CXX;
    if (ASTCtx->getLangOpts().C11)
      Lang = tooling::stdlib::Lang::C;
    else if(!ASTCtx->getLangOpts().CPlusPlus)
      return "";

    if (S->Scope == "std::" && S->Name == "move") {
      if (!S->Signature.contains(','))
        return "<utility>";
      return "<algorithm>";
    }
   
    if (auto StdSym = tooling::stdlib::Symbol::named(S->Scope, S->Name, Lang))
     if (auto Header = StdSym->header())
       return Header->name();
    return "";
}

[VitaNuo]

Address comment.

[kadircet]

clang-tools-extra/clangd/index/SymbolCollector.cpp

842–858

can you also move this into an anonymous namespace?

clang-tools-extra/clangd/index/SymbolCollector.h

private:
   SymbolSlab::Builder Symbols;
   // File IDs used to determine if the code contains Obj-C constructs.
   // For Obj-C symbols, these File IDs are used to compute the include
   // headers.
   llvm::DenseMap<SymbolID, FileID> IncludeFiles;
   void setIncludeLocation(const Symbol &S, SourceLocation,
                           const include_cleaner::Symbol &Sym);
 
-  // Providers for Symbol.IncludeHeaders.
-  // The final spelling is calculated in finish().
-  llvm::DenseMap<SymbolID, std::optional<include_cleaner::Header>>
-      SymbolProviders;
-
   // Files which contain ObjC symbols.
   // This is finalized and used in finish().
   llvm::DenseSet<FileID> FilesWithObjCConstructs;
 
   // Indexed macros, to be erased if they turned out to be include guards.
   llvm::DenseSet<const IdentifierInfo *> IndexedMacros;
   // All refs collected from the AST. It includes:
   //   1) symbols declared in the preamble and referenced from the main file (

clang-tools-extra/clangd/index/SymbolCollector.cpp

 #include "clang/Basic/FileEntry.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Index/IndexSymbol.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Lex/Token.h"
 #include "clang/Tooling/Inclusions/HeaderAnalysis.h"
+#include "clang/Tooling/Inclusions/StandardLibrary.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include <cassert>
void SymbolCollector::setIncludeLocation(const Symbol &S, SourceLocation DefLoc,
   const auto &SM = PP->getSourceManager();
   if (!Opts.CollectIncludePath ||
       shouldCollectIncludePath(S.SymInfo.Kind) == Symbol::Invalid)
     return;
 
   // Use the expansion location to get the #include header since this is
   // where the symbol is exposed.
   IncludeFiles[S.ID] = SM.getDecomposedExpansionLoc(DefLoc).first;
+}
+
+llvm::StringRef getStdHeader(const Symbol *S, const LangOptions &LangOpts) {
+  tooling::stdlib::Lang Lang = tooling::stdlib::Lang::CXX;
+    if (LangOpts.C11)
+      Lang = tooling::stdlib::Lang::C;
+    else if(!LangOpts.CPlusPlus)
+      return "";
 
-  auto [It, Inserted] = SymbolProviders.try_emplace(S.ID);
-  if (Inserted) {
-    auto Headers =
-        include_cleaner::headersForSymbol(Sym, SM, Opts.PragmaIncludes);
-    if (!Headers.empty())
-      It->second = Headers.front();
+    if (S->Scope == "std::" && S->Name == "move") {
+      if (!S->Signature.contains(','))
+        return "<utility>";
+      return "<algorithm>";
     }
+   
+    if (auto StdSym = tooling::stdlib::Symbol::named(S->Scope, S->Name, Lang))
+     if (auto Header = StdSym->header())
+       return Header->name();
+    return "";
 }
 
 void SymbolCollector::finish() {
   // At the end of the TU, add 1 to the refcount of all referenced symbols.
   for (const auto &ID : ReferencedSymbols) {
     if (const auto *S = Symbols.find(ID)) {
       // SymbolSlab::Builder returns const symbols because strings are interned
       // and modifying returned symbols without inserting again wouldn't go
       // well. const_cast is safe here as we're modifying a data owned by the
       // Symbol. This reduces time spent in SymbolCollector by ~1%.
       ++const_cast<Symbol *>(S)->References;
     }
   }
   if (Opts.CollectMacro) {
     assert(PP);
     // First, drop header guards. We can't identify these until EOF.
     for (const IdentifierInfo *II : IndexedMacros) {
       if (const auto *MI = PP->getMacroDefinition(II).getMacroInfo())
         if (auto ID =
                 getSymbolIDCached(II->getName(), MI, PP->getSourceManager()))
           if (MI->isUsedForHeaderGuard())
             Symbols.erase(ID);
     }
   }
   llvm::DenseMap<FileID, bool> FileToContainsImportsOrObjC;
-  llvm::DenseMap<include_cleaner::Header, std::string> HeaderSpelling;
   // Fill in IncludeHeaders.
   // We delay this until end of TU so header guards are all resolved.
-  for (const auto &[SID, OptionalProvider] : SymbolProviders) {
+  for (const auto &[SID, FID] : IncludeFiles) {
     const Symbol *S = Symbols.find(SID);
     if (!S)
       continue;
-    assert(IncludeFiles.find(SID) != IncludeFiles.end());
 
-    const auto FID = IncludeFiles.at(SID);
     // Determine if the FID is #include'd or #import'ed.
     Symbol::IncludeDirective Directives = Symbol::Invalid;
     auto CollectDirectives = shouldCollectIncludePath(S->SymInfo.Kind);
     if ((CollectDirectives & Symbol::Include) != 0)
       Directives |= Symbol::Include;
     // Only allow #import for symbols from ObjC-like files.
     if ((CollectDirectives & Symbol::Import) != 0) {
       auto [It, Inserted] = FileToContainsImportsOrObjC.try_emplace(FID);
       if (Inserted)
         It->second = FilesWithObjCConstructs.contains(FID) ||
                      tooling::codeContainsImports(
                          ASTCtx->getSourceManager().getBufferData(FID));
       if (It->second)
         Directives |= Symbol::Import;
     }
 
     if (Directives == Symbol::Invalid)
       continue;
 
-    // Use the include location-based logic for Objective-C symbols.
-    if (Directives & Symbol::Import) {
-      if (auto IncludeHeader = HeaderFileURIs->getIncludeHeader(FID);
-          !IncludeHeader.empty()) {
-        auto NewSym = *S;
-        NewSym.IncludeHeaders.push_back({IncludeHeader, 1, Directives});
-        Symbols.insert(NewSym);
-      }
-      // FIXME: use providers from include-cleaner library once it's polished
-      // for Objective-C.
-      continue;
-    }
-
-    assert(Directives == Symbol::Include);
-    // For #include's, use the providers computed by the include-cleaner
-    // library.
-    if (!OptionalProvider)
-      continue;
-    const auto &H = *OptionalProvider;
-    const auto [SpellingIt, Inserted] = HeaderSpelling.try_emplace(H);
-    if (Inserted) {
-      auto &SM = ASTCtx->getSourceManager();
-      if (H.kind() == include_cleaner::Header::Kind::Physical) {
-        if (auto Canonical =
-                HeaderFileURIs->mapCanonical(H.physical()->getName());
-            !Canonical.empty())
-          SpellingIt->second = Canonical;
-        else if (tooling::isSelfContainedHeader(H.physical(), SM,
-                                                PP->getHeaderSearchInfo()))
-          SpellingIt->second =
-              HeaderFileURIs->toURI(H.physical()->getLastRef());
-      } else {
-        SpellingIt->second = include_cleaner::spellHeader(
-            {H, PP->getHeaderSearchInfo(),
-             SM.getFileEntryForID(SM.getMainFileID())});
-      }
-    }
+    // FIXME: Use the include-cleaner library instead.
+    llvm::StringRef IncludeHeader = getStdHeader(S, ASTCtx->getLangOpts());
+    if (IncludeHeader.empty())
+      IncludeHeader = HeaderFileURIs->getIncludeHeader(FID);
 
-    if (!SpellingIt->second.empty()) {
+    if (!IncludeHeader.empty()) {
       auto NewSym = *S;
-      NewSym.IncludeHeaders.push_back({SpellingIt->second, 1, Directives});
+      NewSym.IncludeHeaders.push_back({IncludeHeader, 1, Directives});
       Symbols.insert(NewSym);
     }
   }
 
   ReferencedSymbols.clear();
   IncludeFiles.clear();
-  SymbolProviders.clear();
   FilesWithObjCConstructs.clear();
 }
 
 const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND, SymbolID ID,
                                               bool IsMainFileOnly) {
   auto &Ctx = ND.getASTContext();
   auto &SM = Ctx.getSourceManager();
 
   Symbol S;
   S.ID = std::move(ID);
   std::string QName = printQualifiedName(ND);
   // FIXME: this returns foo:bar: for objective-C methods, we prefer only foo:
   // for consistency with CodeCompletionString and a clean name/signature split.
   std::tie(S.Scope, S.Name) = splitQualifiedName(QName);
   std::string TemplateSpecializationArgs = printTemplateSpecializationArgs(ND);
   S.TemplateSpecializationArgs = TemplateSpecializationArgs;
 
   // We collect main-file symbols, but do not use them for code completion.
   if (!IsMainFileOnly && isIndexedForCodeCompletion(ND, Ctx))
     S.Flags |= Symbol::IndexedForCodeCompletion;
   if (isImplementationDetail(&ND))
     S.Flags |= Symbol::ImplementationDetail;
   if (!IsMainFileOnly)
     S.Flags |= Symbol::VisibleOutsideFile;
   S.SymInfo = index::getSymbolInfo(&ND);
   auto Loc = nameLocation(ND, SM);
   assert(Loc.isValid() && "Invalid source location for NamedDecl");
   // FIXME: use the result to filter out symbols.
   auto FID = SM.getFileID(Loc);
   shouldIndexFile(FID);
   if (auto DeclLoc = getTokenLocation(Loc))
     S.CanonicalDeclaration = *DeclLoc;
 
   S.Origin = Opts.Origin;
   if (ND.getAvailability() == AR_Deprecated)
     S.Flags |= Symbol::Deprecated;
 
   // Add completion info.
   // FIXME: we may want to choose a different redecl, or combine from several.

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

update(
       "// IWYU pragma: private, include <the/good/header.h>\nclass string {};");
 
   auto Symbols = runFuzzyFind(M, "");
   EXPECT_THAT(Symbols, ElementsAre(_));
   EXPECT_THAT(Symbols.begin()->IncludeHeaders.front().IncludeHeader,
               "<the/good/header.h>");
 }
 
-TEST(FileIndexTest, IWYUPragmaExport) {
+TEST(FileIndexTest, DISABLED_IWYUPragmaExport) {
   FileIndex M;
 
   TestTU File;
   File.Code = R"cpp(#pragma once
     #include "exporter.h"
   )cpp";
   File.HeaderFilename = "exporter.h";
   File.HeaderCode = R"cpp(#pragma once

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

const std::string Header = R"(
     class Foo {};
   )";
   runSymbolCollector(Header, /*Main=*/"");
   EXPECT_THAT(Symbols, UnorderedElementsAre(
                            AllOf(qName("Foo"), declURI(TestHeaderURI),
                                  includeHeader("\"the/good/header.h\""))));
 }
 
-TEST_F(SymbolCollectorTest, IWYUPragmaExport) {
+TEST_F(SymbolCollectorTest, DISABLED_IWYUPragmaExport) {
   CollectorOpts.CollectIncludePath = true;
   const std::string Header = R"cpp(#pragma once
     #include "exporter.h"
   )cpp";
   auto ExporterFile = testPath("exporter.h");
   InMemoryFileSystem->addFile(
       ExporterFile, 0, llvm::MemoryBuffer::getMemBuffer(R"cpp(#pragma once
     #include "private.h" // IWYU pragma: export