[clangd] Propagate header-guarded flag from preamble to main AST

Fixes https://github.com/clangd/clangd/issues/377

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

diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp
index 2520062..a60b9b6 100644 (file)
--- a/clang-tools-extra/clangd/ParsedAST.cpp
+++ b/clang-tools-extra/clangd/ParsedAST.cpp
@@ -309,6 +309,16 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
         MainInput.getFile());
     return None;
   }
+  // If we saw an include guard in the preamble section of the main file,
+  // mark the main-file as include-guarded.
+  // This information is part of the HeaderFileInfo but is not loaded from the
+  // preamble as the file's size is part of its identity and may have changed.
+  // (The rest of HeaderFileInfo is not relevant for our purposes).
+  if (Preamble && Preamble->MainIsIncludeGuarded) {
+    const SourceManager &SM = Clang->getSourceManager();
+    const FileEntry *MainFE = SM.getFileEntryForID(SM.getMainFileID());
+    Clang->getPreprocessor().getHeaderSearchInfo().MarkFileIncludeOnce(MainFE);
+  }
 
   // Set up ClangTidy. Must happen after BeginSourceFile() so ASTContext exists.
   // Clang-tidy has some limitations to ensure reasonable performance:

diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp
index 9173dcd..bc8c51b 100644 (file)
--- a/clang-tools-extra/clangd/Preamble.cpp
+++ b/clang-tools-extra/clangd/Preamble.cpp
@@ -75,11 +75,20 @@ public:
 
   CanonicalIncludes takeCanonicalIncludes() { return std::move(CanonIncludes); }
 
+  bool isMainFileIncludeGuarded() const { return IsMainFileIncludeGuarded; }
+
   void AfterExecute(CompilerInstance &CI) override {
-    if (!ParsedCallback)
-      return;
-    trace::Span Tracer("Running PreambleCallback");
-    ParsedCallback(CI.getASTContext(), CI.getPreprocessorPtr(), CanonIncludes);
+    if (ParsedCallback) {
+      trace::Span Tracer("Running PreambleCallback");
+      ParsedCallback(CI.getASTContext(), CI.getPreprocessorPtr(),
+                     CanonIncludes);
+    }
+
+    const SourceManager &SM = CI.getSourceManager();
+    const FileEntry *MainFE = SM.getFileEntryForID(SM.getMainFileID());
+    IsMainFileIncludeGuarded =
+        CI.getPreprocessor().getHeaderSearchInfo().isFileMultipleIncludeGuarded(
+            MainFE);
   }
 
   void BeforeExecute(CompilerInstance &CI) override {
@@ -121,6 +130,7 @@ private:
   IncludeStructure Includes;
   CanonicalIncludes CanonIncludes;
   MainFileMacros Macros;
+  bool IsMainFileIncludeGuarded = false;
   std::unique_ptr<CommentHandler> IWYUHandler = nullptr;
   const clang::LangOptions *LangOpts = nullptr;
   const SourceManager *SourceMgr = nullptr;
@@ -365,7 +375,7 @@ buildPreamble(PathRef FileName, CompilerInvocation CI,
   // to read back. We rely on dynamic index for the comments instead.
   CI.getPreprocessorOpts().WriteCommentListToPCH = false;
 
-  CppFilePreambleCallbacks SerializedDeclsCollector(FileName, PreambleCallback);
+  CppFilePreambleCallbacks CapturedInfo(FileName, PreambleCallback);
   auto VFS = Inputs.TFS->view(Inputs.CompileCommand.Directory);
   llvm::SmallString<32> AbsFileName(FileName);
   VFS->makeAbsolute(AbsFileName);
@@ -373,8 +383,7 @@ buildPreamble(PathRef FileName, CompilerInvocation CI,
   auto BuiltPreamble = PrecompiledPreamble::Build(
       CI, ContentsBuffer.get(), Bounds, *PreambleDiagsEngine,
       StatCache->getProducingFS(VFS),
-      std::make_shared<PCHContainerOperations>(), StoreInMemory,
-      SerializedDeclsCollector);
+      std::make_shared<PCHContainerOperations>(), StoreInMemory, CapturedInfo);
 
   // When building the AST for the main file, we do want the function
   // bodies.
@@ -384,16 +393,17 @@ buildPreamble(PathRef FileName, CompilerInvocation CI,
     vlog("Built preamble of size {0} for file {1} version {2}",
          BuiltPreamble->getSize(), FileName, Inputs.Version);
     std::vector<Diag> Diags = PreambleDiagnostics.take();
-    return std::make_shared<PreambleData>(
+    auto Result = std::make_shared<PreambleData>(
         Inputs, std::move(*BuiltPreamble), std::move(Diags),
-        SerializedDeclsCollector.takeIncludes(),
-        SerializedDeclsCollector.takeMacros(), std::move(StatCache),
-        SerializedDeclsCollector.takeCanonicalIncludes());
-  } else {
-    elog("Could not build a preamble for file {0} version {1}: {2}", FileName,
-         Inputs.Version, BuiltPreamble.getError().message());
-    return nullptr;
+        CapturedInfo.takeIncludes(), CapturedInfo.takeMacros(),
+        std::move(StatCache), CapturedInfo.takeCanonicalIncludes());
+    Result->MainIsIncludeGuarded = CapturedInfo.isMainFileIncludeGuarded();
+    return Result;
   }
+
+  elog("Could not build a preamble for file {0} version {1}: {2}", FileName,
+       Inputs.Version, BuiltPreamble.getError().message());
+  return nullptr;
 }
 
 bool isPreambleCompatible(const PreambleData &Preamble,

diff --git a/clang-tools-extra/clangd/Preamble.h b/clang-tools-extra/clangd/Preamble.h
index 5b9d178..fa48d67 100644 (file)
--- a/clang-tools-extra/clangd/Preamble.h
+++ b/clang-tools-extra/clangd/Preamble.h
@@ -69,6 +69,9 @@ struct PreambleData {
   // When reusing a preamble, this cache can be consumed to save IO.
   std::unique_ptr<PreambleFileStatusCache> StatCache;
   CanonicalIncludes CanonIncludes;
+  // Whether there was a (possibly-incomplete) include-guard on the main file.
+  // We need to propagate this information "by hand" to subsequent parses.
+  bool MainIsIncludeGuarded = false;
 };
 
 using PreambleParsedCallback =

diff --git a/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp b/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
index 1c960c8..365dbc5 100644 (file)
--- a/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
@@ -670,7 +670,7 @@ TEST(ParsedASTTest, HeaderGuards) {
   EXPECT_FALSE(mainIsGuarded(TU.build())); // FIXME: true
 
   TU.Code = once(";");
-  EXPECT_FALSE(mainIsGuarded(TU.build())); // FIXME: true
+  EXPECT_TRUE(mainIsGuarded(TU.build()));
 
   TU.Code = R"cpp(
     ;
@@ -727,7 +727,7 @@ TEST(ParsedASTTest, HeaderGuardsSelfInclude) {
   )cpp";
   AST = TU.build();
   EXPECT_THAT(*AST.getDiagnostics(), IsEmpty());
-  EXPECT_FALSE(mainIsGuarded(AST)); // FIXME: true
+  EXPECT_TRUE(mainIsGuarded(AST));
 
   TU.Code = R"cpp(
     #pragma once
@@ -757,7 +757,7 @@ TEST(ParsedASTTest, HeaderGuardsSelfInclude) {
   AST = TU.build();
   EXPECT_THAT(*AST.getDiagnostics(),
               ElementsAre(Diag("recursively when building a preamble")));
-  EXPECT_FALSE(mainIsGuarded(AST)); // FIXME: true
+  EXPECT_TRUE(mainIsGuarded(AST));
 
   TU.Code = R"cpp(
     #ifndef GUARD
@@ -814,7 +814,7 @@ TEST(ParsedASTTest, HeaderGuardsSelfInclude) {
   AST = TU.build();
   EXPECT_THAT(*AST.getDiagnostics(),
               ElementsAre(Diag("recursively when building a preamble")));
-  EXPECT_FALSE(mainIsGuarded(AST)); // FIXME: true
+  EXPECT_TRUE(mainIsGuarded(AST));
 
   TU.Code = R"cpp(
     #include "self.h" // error-ok
@@ -863,9 +863,7 @@ TEST(ParsedASTTest, HeaderGuardsImplIface) {
   // need to transfer it to the main file's HeaderFileInfo.
   TU.Code = once(Interface);
   AST = TU.build();
-  // FIXME: empty
-  EXPECT_THAT(*AST.getDiagnostics(),
-              ElementsAre(Diag("in included file: redefinition of 'Traits'")));
+  EXPECT_THAT(*AST.getDiagnostics(), IsEmpty());
   EXPECT_TRUE(mainIsGuarded(AST));
 
   // Editing the implementation file, which is not include guarded.