[clangd] Fix header-guard check for include insertion, and don't index header guards.

Summary:
Both of these attempt to check whether a header guard exists while parsing the
file. However the file is only marked as guarded once clang finishes processing
it. We defer the checks and work until SymbolCollector::finish().

This is ugly and ad-hoc, deferring *all* work might be cleaner.

Reviewers: kadircet

Subscribers: ilya-biryukov, MaskRay, jkorous, mgrang, arphaman, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D61442

llvm-svn: 359880

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

@@ -48,27 +48,23 @@ static void own(Symbol &S, llvm::UniqueStringSaver &Strings) {
 }
 
 void SymbolSlab::Builder::insert(const Symbol &S) {
-  auto R = SymbolIndex.try_emplace(S.ID, Symbols.size());
-  if (R.second) {
-    Symbols.push_back(S);
-    own(Symbols.back(), UniqueStrings);
-  } else {
-    auto &Copy = Symbols[R.first->second] = S;
-    own(Copy, UniqueStrings);
-  }
+  own(Symbols[S.ID] = S, UniqueStrings);
 }
 
 SymbolSlab SymbolSlab::Builder::build() && {
-  Symbols = {Symbols.begin(), Symbols.end()}; // Force shrink-to-fit.
-  // Sort symbols so the slab can binary search over them.
-  llvm::sort(Symbols,
+  // Sort symbols into vector so the slab can binary search over them.
+  std::vector<Symbol> SortedSymbols;
+  SortedSymbols.reserve(Symbols.size());
+  for (auto &Entry : Symbols)
+    SortedSymbols.push_back(std::move(Entry.second));
+  llvm::sort(SortedSymbols,
              [](const Symbol &L, const Symbol &R) { return L.ID < R.ID; });
   // We may have unused strings from overwritten symbols. Build a new arena.
   llvm::BumpPtrAllocator NewArena;
   llvm::UniqueStringSaver Strings(NewArena);
-  for (auto &S : Symbols)
+  for (auto &S : SortedSymbols)
     own(S, Strings);
-  return SymbolSlab(std::move(NewArena), std::move(Symbols));
+  return SymbolSlab(std::move(NewArena), std::move(SortedSymbols));
 }
 
 } // namespace clangd

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

@@ -204,10 +204,13 @@ public:
     /// This is a deep copy: underlying strings will be owned by the slab.
     void insert(const Symbol &S);
 
-    /// Returns the symbol with an ID, if it exists. Valid until next insert().
+    /// Removes the symbol with an ID, if it exists.
+    void erase(const SymbolID &ID) { Symbols.erase(ID); }
+
+    /// Returns the symbol with an ID, if it exists. Valid until insert/remove.
     const Symbol *find(const SymbolID &ID) {
-      auto I = SymbolIndex.find(ID);
-      return I == SymbolIndex.end() ? nullptr : &Symbols[I->second];
+      auto I = Symbols.find(ID);
+      return I == Symbols.end() ? nullptr : &I->second;
     }
 
     /// Consumes the builder to finalize the slab.
@@ -217,9 +220,8 @@ public:
     llvm::BumpPtrAllocator Arena;
     /// Intern table for strings. Contents are on the arena.
     llvm::UniqueStringSaver UniqueStrings;
-    std::vector<Symbol> Symbols;
     /// Values are indices into Symbols vector.
-    llvm::DenseMap<SymbolID, size_t> SymbolIndex;
+    llvm::DenseMap<SymbolID, Symbol> Symbols;
   };
 
 private:

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

@@ -352,9 +352,8 @@ bool SymbolCollector::handleMacroOccurence(const IdentifierInfo *Name,
   const auto &SM = PP->getSourceManager();
   auto DefLoc = MI->getDefinitionLoc();
 
-  // Header guards are not interesting in index. Builtin macros don't have
-  // useful locations and are not needed for code completions.
-  if (MI->isUsedForHeaderGuard() || MI->isBuiltinMacro())
+  // Builtin macros don't have useful locations and aren't needed in completion.
+  if (MI->isBuiltinMacro())
     return true;
 
   // Skip main-file symbols if we are not collecting them.
@@ -408,22 +407,25 @@ bool SymbolCollector::handleMacroOccurence(const IdentifierInfo *Name,
   std::string Signature;
   std::string SnippetSuffix;
   getSignature(*CCS, &Signature, &SnippetSuffix);
-
-  std::string Include;
-  if (Opts.CollectIncludePath && shouldCollectIncludePath(S.SymInfo.Kind)) {
-    if (auto Header = getIncludeHeader(
-            Name->getName(), SM.getDecomposedExpansionLoc(DefLoc).first))
-      Include = std::move(*Header);
-  }
   S.Signature = Signature;
   S.CompletionSnippetSuffix = SnippetSuffix;
-  if (!Include.empty())
-    S.IncludeHeaders.emplace_back(Include, 1);
 
+  IndexedMacros.insert(Name);
+  setIncludeLocation(S, DefLoc);
   Symbols.insert(S);
   return true;
 }
 
+void SymbolCollector::setIncludeLocation(const Symbol &S,
+                                         SourceLocation Loc) {
+  if (Opts.CollectIncludePath)
+    if (shouldCollectIncludePath(S.SymInfo.Kind))
+      // Use the expansion location to get the #include header since this is
+      // where the symbol is exposed.
+      IncludeFiles[S.ID] =
+          PP->getSourceManager().getDecomposedExpansionLoc(Loc).first;
+}
+
 void SymbolCollector::finish() {
   // At the end of the TU, add 1 to the refcount of all referenced symbols.
   auto IncRef = [this](const SymbolID &ID) {
@@ -440,6 +442,14 @@ void SymbolCollector::finish() {
   }
   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 = getSymbolID(*II, MI, PP->getSourceManager()))
+          if (MI->isUsedForHeaderGuard())
+            Symbols.erase(*ID);
+    }
+    // Now increment refcounts.
     for (const IdentifierInfo *II : ReferencedMacros) {
       if (const auto *MI = PP->getMacroDefinition(II).getMacroInfo())
         if (auto ID = getSymbolID(*II, MI, PP->getSourceManager()))
@@ -447,6 +457,21 @@ void SymbolCollector::finish() {
     }
   }
 
+  // Fill in IncludeHeaders.
+  // We delay this until end of TU so header guards are all resolved.
+  // Symbols in slabs aren' mutable, so insert() has to walk all the strings :-(
+  llvm::SmallString<256> QName;
+  for (const auto &Entry : IncludeFiles)
+    if (const Symbol *S = Symbols.find(Entry.first)) {
+      QName = S->Scope;
+      QName.append(S->Name);
+      if (auto Header = getIncludeHeader(QName, Entry.second)) {
+        Symbol NewSym = *S;
+        NewSym.IncludeHeaders.push_back({*Header, 1});
+        Symbols.insert(NewSym);
+      }
+    }
+
   const auto &SM = ASTCtx->getSourceManager();
   llvm::DenseMap<FileID, std::string> URICache;
   auto GetURI = [&](FileID FID) -> llvm::Optional<std::string> {
@@ -464,7 +489,7 @@ void SymbolCollector::finish() {
     }
     return Found->second;
   };
-
+  // Populate Refs slab from DeclRefs.
   if (auto MainFileURI = GetURI(SM.getMainFileID())) {
     for (const auto &It : DeclRefs) {
       if (auto ID = getSymbolID(It.first)) {
@@ -492,6 +517,7 @@ void SymbolCollector::finish() {
   DeclRefs.clear();
   FilesToIndexCache.clear();
   HeaderIsSelfContainedCache.clear();
+  IncludeFiles.clear();
 }
 
 const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND, SymbolID ID,
@@ -556,17 +582,6 @@ const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND, SymbolID ID,
   std::string ReturnType = getReturnType(*CCS);
   S.ReturnType = ReturnType;
 
-  std::string Include;
-  if (Opts.CollectIncludePath && shouldCollectIncludePath(S.SymInfo.Kind)) {
-    // Use the expansion location to get the #include header since this is
-    // where the symbol is exposed.
-    if (auto Header = getIncludeHeader(
-            QName, SM.getDecomposedExpansionLoc(ND.getLocation()).first))
-      Include = std::move(*Header);
-  }
-  if (!Include.empty())
-    S.IncludeHeaders.emplace_back(Include, 1);
-
   llvm::Optional<OpaqueType> TypeStorage;
   if (S.Flags & Symbol::IndexedForCodeCompletion) {
     TypeStorage = OpaqueType::fromCompletionResult(*ASTCtx, SymbolCompletion);
@@ -575,6 +590,7 @@ const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND, SymbolID ID,
   }
 
   Symbols.insert(S);
+  setIncludeLocation(S, ND.getLocation());
   return Symbols.find(S.ID);
 }
 

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

@@ -125,6 +125,12 @@ private:
 
   // All Symbols collected from the AST.
   SymbolSlab::Builder Symbols;
+  // File IDs for Symbol.IncludeHeaders.
+  // The final spelling is calculated in finish().
+  llvm::DenseMap<SymbolID, FileID> IncludeFiles;
+  void setIncludeLocation(const Symbol &S, SourceLocation);
+  // Indexed macros, to be erased if they turned out to be include guards.
+  llvm::DenseSet<const IdentifierInfo *> IndexedMacros;
   // All refs collected from the AST.
   // Only symbols declared in preamble (from #include) and referenced from the
   // main file will be included.

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

@@ -20,6 +20,7 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/VirtualFileSystem.h"
+#include "gmock/gmock-matchers.h"
 #include "gmock/gmock-more-matchers.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
@@ -34,9 +35,9 @@ namespace {
 using testing::_;
 using testing::AllOf;
 using testing::Contains;
+using testing::Each;
 using testing::ElementsAre;
 using testing::Field;
-using testing::IsEmpty;
 using testing::Not;
 using testing::Pair;
 using testing::UnorderedElementsAre;
@@ -1028,6 +1029,26 @@ TEST_F(SymbolCollectorTest, IncFileInNonHeader) {
                                                   Not(IncludeHeader()))));
 }
 
+// Features that depend on header-guards are fragile. Header guards are only
+// recognized when the file ends, so we have to defer checking for them.
+TEST_F(SymbolCollectorTest, HeaderGuardDetected) {
+  CollectorOpts.CollectIncludePath = true;
+  CollectorOpts.CollectMacro = true;
+  runSymbolCollector(R"cpp(
+    #ifndef HEADER_GUARD_
+    #define HEADER_GUARD_
+
+    // Symbols are seen before the header guard is complete.
+    #define MACRO
+    int decl();
+
+    #endif // Header guard is recognized here.
+  )cpp",
+                     "");
+  EXPECT_THAT(Symbols, Not(Contains(QName("HEADER_GUARD_"))));
+  EXPECT_THAT(Symbols, Each(IncludeHeader()));
+}
+
 TEST_F(SymbolCollectorTest, NonModularHeader) {
   auto TU = TestTU::withHeaderCode("int x();");
   EXPECT_THAT(TU.headerSymbols(), ElementsAre(IncludeHeader()));