[clangd] Rollforward include-cleaner library usage in symbol collector.

[kadircet]

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

851–862

can you add a comment here saying, We update providers for a symbol with each occurence, as SymbolCollector might run while parsing, rather than at the end of a translation unit. Hence we see more and more redecls over time.

919

we should keep the getStdHeaders logic for objc

941

can you add a // FIXME: Get rid of this once include-cleaner has support for system headers. to this branch

944

again a comment saying For physical files, prefer URIs as spellings might change depending on the translation unit.

clang-tools-extra/include-cleaner/lib/FindHeaders.cpp

127 ↗

(On Diff #545562)

can you also add comment move(ExecutionPolicy&& policy, ForwardIt1 first, ForwardIt1 last, ForwardIt2 d_first ); and move this change into a separate patch with a test case in clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp?

clang/lib/Tooling/Inclusions/Stdlib/StdSpecialSymbolMap.inc

365 ↗

(On Diff #545562)

can you move this into a separate patch?

[VitaNuo]

Address review comments.

[VitaNuo]

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

919

getStdHeaders returns empty string for Obj-C, are you sure you meant it?

[kadircet]

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

919

it reads a little bit weird, but it still actually works, for objective-c++ to be more specific. as we'll have LangOpts.CPlusPlus set for objc++

[VitaNuo]

Address the comment.

[VitaNuo]

Thanks!

[kadircet]

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

828

sorry i got confused, this also works for ObjC, not just ObjC++. we set C-like features for ObjectiveC. can you just restore as-is?

921

i guess LHS of the assignment got dropped by mistake?

[VitaNuo]

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

921

Sorry, juggling unconnected stuff makes me inattentive.

[VitaNuo]

Address comments.

[kadircet]

thanks!

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

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;
+
+  // We update providers for a symbol with each occurence, as SymbolCollector
+  // might run while parsing, rather than at the end of a translation unit.
+  // Hence we see more and more redecls over time.
+  auto [It, Inserted] = SymbolProviders.try_emplace(S.ID);
+  auto Headers =
+      include_cleaner::headersForSymbol(Sym, SM, Opts.PragmaIncludes);
+  if (Headers.empty())
+    return;
+
+  auto *HeadersIter = Headers.begin();
+  include_cleaner::Header H = *HeadersIter;
+  while (HeadersIter != Headers.end() &&
+         H.kind() == include_cleaner::Header::Physical &&
+         !tooling::isSelfContainedHeader(H.physical(), SM,
+                                         PP->getHeaderSearchInfo())) {
+    H = *HeadersIter;
+    HeadersIter++;
+  }
+  It->second = H;
 }
 
 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 "";
 
-    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
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, FID] : IncludeFiles) {
+  for (const auto &[SID, OptionalProvider] : SymbolProviders) {
     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;
 
-    // FIXME: Use the include-cleaner library instead.
+    // Use the include location-based logic for Objective-C symbols.
+    if (Directives & Symbol::Import) {
       llvm::StringRef IncludeHeader = getStdHeader(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);
       }
+      // 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) {
+        // FIXME: Get rid of this once include-cleaner has support for system
+        // headers.
+        if (auto Canonical =
+                HeaderFileURIs->mapCanonical(H.physical()->getName());
+            !Canonical.empty())
+          SpellingIt->second = Canonical;
+        // For physical files, prefer URIs as spellings might change
+        // depending on the translation unit.
+        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())});
+      }
+    }
+
+    if (!SpellingIt->second.empty()) {
+      auto NewSym = *S;
+      NewSym.IncludeHeaders.push_back({SpellingIt->second, 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();
 

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, DISABLED_IWYUPragmaExport) {
+TEST(FileIndexTest, 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/IndexActionTests.cpp

 //===------ IndexActionTests.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 "Headers.h"
 #include "TestFS.h"
+#include "URI.h"
 #include "index/IndexAction.h"
 #include "index/Serialization.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
 #include "clang/Tooling/Tooling.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include <string>
 
 namespace clang {
 namespace clangd {
 namespace {
 
 using ::testing::AllOf;
 using ::testing::ElementsAre;
 using ::testing::EndsWith;

 MATCHER_P(hasName, Name, "") { return arg.Name == Name; }
 
 MATCHER(hasSameURI, "") {
   llvm::StringRef URI = ::testing::get<0>(arg);
   const std::string &Path = ::testing::get<1>(arg);
   return toUri(Path) == URI;
 }
 
+MATCHER_P(includeHeader, P, "") {
+  return (arg.IncludeHeaders.size() == 1) &&
+         (arg.IncludeHeaders.begin()->IncludeHeader == P);
+}
+
 ::testing::Matcher<const IncludeGraphNode &>
 includesAre(const std::vector<std::string> &Includes) {
   return ::testing::Field(&IncludeGraphNode::DirectIncludes,
                           UnorderedPointwise(hasSameURI(), Includes));
 }
 
 void checkNodesAreInitialized(const IndexFileIn &IndexFile,
                               const std::vector<std::string> &Paths) {
TEST_F(IndexActionTest, SkipNestedSymbols) {
   }
   }
   })cpp");
   IndexFileIn IndexFile = runIndexingAction(MainFilePath, {"-std=c++14"});
   EXPECT_THAT(*IndexFile.Symbols, testing::Contains(hasName("foo")));
   EXPECT_THAT(*IndexFile.Symbols, testing::Contains(hasName("Bar")));
   EXPECT_THAT(*IndexFile.Symbols, Not(testing::Contains(hasName("Baz"))));
 }
+
+TEST_F(IndexActionTest, SymbolFromCC) {
+  std::string MainFilePath = testPath("main.cpp");
+  addFile(MainFilePath, R"cpp(
+ #include "main.h"
+ void foo() {}
+ )cpp");
+  addFile(testPath("main.h"), R"cpp(
+ #pragma once
+ void foo();
+ )cpp");
+  Opts.FileFilter = [](const SourceManager &SM, FileID F) {
+    return !SM.getFileEntryRefForID(F)->getName().endswith("main.h");
+  };
+  IndexFileIn IndexFile = runIndexingAction(MainFilePath, {"-std=c++14"});
+  EXPECT_THAT(*IndexFile.Symbols,
+              UnorderedElementsAre(AllOf(
+                  hasName("foo"),
+                  includeHeader(URI::create(testPath("main.h")).toString()))));
+}
 } // namespace
 } // namespace clangd
 } // namespace clang

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

 //===-- SymbolCollectorTests.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 "Annotations.h"
 #include "TestFS.h"
 #include "TestTU.h"
 #include "URI.h"
 #include "clang-include-cleaner/Record.h"
 #include "index/SymbolCollector.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/FileSystemOptions.h"
+#include "clang/Basic/SourceLocation.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Index/IndexingAction.h"
 #include "clang/Index/IndexingOptions.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/VirtualFileSystem.h"
TEST_F(SymbolCollectorTest, CanonicalSTLHeader) {
   CollectorOpts.CollectIncludePath = true;
   runSymbolCollector(
       R"cpp(
       namespace std {
         class string {};
         // Move overloads have special handling.
         template <typename _T> T&& move(_T&& __value);
         template <typename _I, typename _O> _O move(_I, _I, _O);
+        template <typename _T, typename _O, typename _I> _O move(
+          _T&&, _O, _O, _I);
       }
       )cpp",
       /*Main=*/"");
   EXPECT_THAT(
       Symbols,
       UnorderedElementsAre(
           qName("std"),
           AllOf(qName("std::string"), declURI(TestHeaderURI),
                 includeHeader("<string>")),
           // Parameter names are demangled.
           AllOf(labeled("move(T &&value)"), includeHeader("<utility>")),
-          AllOf(labeled("move(I, I, O)"), includeHeader("<algorithm>"))));
+          AllOf(labeled("move(I, I, O)"), includeHeader("<algorithm>")),
+          AllOf(labeled("move(T &&, O, O, I)"), includeHeader("<algorithm>"))));
 }
 
 TEST_F(SymbolCollectorTest, IWYUPragma) {
   CollectorOpts.CollectIncludePath = true;
   const std::string Header = R"(
     // IWYU pragma: private, include the/good/header.h
     class Foo {};
   )";
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, DISABLED_IWYUPragmaExport) {
+TEST_F(SymbolCollectorTest, 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
const char *Header = R"cpp(
     concept A = sizeof(T) <= 8;
   )cpp";
   runSymbolCollector("", Header, {"-std=c++20"});
   EXPECT_THAT(Symbols,
               UnorderedElementsAre(AllOf(
                   qName("A"), hasKind(clang::index::SymbolKind::Concept))));
 }
 
+TEST_F(SymbolCollectorTest, IncludeHeaderForwardDecls) {
+  CollectorOpts.CollectIncludePath = true;
+  const std::string Header = R"cpp(#pragma once 
+struct Foo;
+#include "full.h"
+)cpp";
+  auto FullFile = testPath("full.h");
+  InMemoryFileSystem->addFile(FullFile, 0,
+                              llvm::MemoryBuffer::getMemBuffer(R"cpp(
+#pragma once 
+struct Foo {};)cpp"));
+  runSymbolCollector(Header, /*Main=*/"",
+                     /*ExtraArgs=*/{"-I", testRoot()});
+  EXPECT_THAT(Symbols, UnorderedElementsAre(AllOf(
+                           qName("Foo"),
+                           includeHeader(URI::create(FullFile).toString()))))
+      << *Symbols.begin();
+}
 } // namespace
 } // namespace clangd
 } // namespace clang