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

[sammccall]

Summary

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

[sammccall]

revert unintended change

[kadircet]

thanks!

clang-tools-extra/clangd/Preamble.cpp

87

nit: maybe do this at the top and keep the early exit?

400

any reason for not making this part of the constructor ?

[sammccall]

Sorry, forgot to address the style comments. Will send a followup patch.

clang-tools-extra/clangd/Preamble.cpp

87

that makes sense if ParsedCallback is the "main line" case that we're bailing out of (which is how it was written before), but I don't really see it that way.

400

I think this pattern of constructor (long list of args, forwards them into public fields) isn't terribly useful, but I should have been consistent here.

Will send a cleanup.

clang-tools-extra/clangd/ParsedAST.cpp

ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
 
   auto Action = std::make_unique<ClangdFrontendAction>();
   const FrontendInputFile &MainInput = Clang->getFrontendOpts().Inputs[0];
   if (!Action->BeginSourceFile(*Clang, MainInput)) {
     log("BeginSourceFile() failed when building AST for {0}",
         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:
   //  - checks don't see all preprocessor events in the preamble
   //  - matchers run only over the main-file top-level decls (and can't see
   //    ancestors outside this scope).
   // In practice almost all checks work well without modifications.
   std::vector<std::unique_ptr<tidy::ClangTidyCheck>> CTChecks;

clang-tools-extra/clangd/Preamble.h

struct PreambleData {
   // Macros defined in the preamble section of the main file.
   // Users care about headers vs main-file, not preamble vs non-preamble.
   // These should be treated as main-file entities e.g. for code completion.
   MainFileMacros Macros;
   // Cache of FS operations performed when building the preamble.
   // 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 =
     std::function<void(ASTContext &, std::shared_ptr<clang::Preprocessor>,
                        const CanonicalIncludes &)>;
 
 /// Build a preamble for the new inputs unless an old one can be reused.
 /// If \p PreambleCallback is set, it will be run on top of the AST while

clang-tools-extra/clangd/Preamble.cpp

CppFilePreambleCallbacks(PathRef File, PreambleParsedCallback ParsedCallback)
       : File(File), ParsedCallback(ParsedCallback) {}
 
   IncludeStructure takeIncludes() { return std::move(Includes); }
 
   MainFileMacros takeMacros() { return std::move(Macros); }
 
   CanonicalIncludes takeCanonicalIncludes() { return std::move(CanonIncludes); }
 
+  bool isMainFileIncludeGuarded() const { return IsMainFileIncludeGuarded; }
+
   void AfterExecute(CompilerInstance &CI) override {
-    if (!ParsedCallback)
-      return;
+    if (ParsedCallback) {
       trace::Span Tracer("Running PreambleCallback");
-    ParsedCallback(CI.getASTContext(), CI.getPreprocessorPtr(), CanonIncludes);
+      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 {
     CanonIncludes.addSystemHeadersMapping(CI.getLangOpts());
     LangOpts = &CI.getLangOpts();
     SourceMgr = &CI.getSourceManager();
   }
 
public:
   }
 
 private:
   PathRef File;
   PreambleParsedCallback ParsedCallback;
   IncludeStructure Includes;
   CanonicalIncludes CanonIncludes;
   MainFileMacros Macros;
+  bool IsMainFileIncludeGuarded = false;
   std::unique_ptr<CommentHandler> IWYUHandler = nullptr;
   const clang::LangOptions *LangOpts = nullptr;
   const SourceManager *SourceMgr = nullptr;
 };
 
 // Represents directives other than includes, where basic textual information is
 // enough.
 struct TextualPPDirective {
buildPreamble(PathRef FileName, CompilerInvocation CI,
   // Skip function bodies when building the preamble to speed up building
   // the preamble and make it smaller.
   assert(!CI.getFrontendOpts().SkipFunctionBodies);
   CI.getFrontendOpts().SkipFunctionBodies = true;
   // We don't want to write comment locations into PCH. They are racy and slow
   // 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);
   auto StatCache = std::make_unique<PreambleFileStatusCache>(AbsFileName);
   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.
   CI.getFrontendOpts().SkipFunctionBodies = false;
 
   if (BuiltPreamble) {
     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;
+  }
+
 
 bool isPreambleCompatible(const PreambleData &Preamble,
                           const ParseInputs &Inputs, PathRef FileName,
                           const CompilerInvocation &CI) {
   auto ContentsBuffer =
       llvm::MemoryBuffer::getMemBuffer(Inputs.Contents, FileName);
   auto Bounds = ComputePreambleBounds(*CI.getLangOpts(), *ContentsBuffer, 0);
   auto VFS = Inputs.TFS->view(Inputs.CompileCommand.Directory);

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

TEST(ParsedASTTest, HeaderGuards) {
 
   TU.Code = ";";
   EXPECT_FALSE(mainIsGuarded(TU.build()));
 
   TU.Code = guard(";");
   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(
     ;
     #pragma once
   )cpp";
   EXPECT_FALSE(mainIsGuarded(TU.build())); // FIXME: true
 
   TU.Code = R"cpp(
TEST(ParsedASTTest, HeaderGuardsSelfInclude) {
 
   TU.Code = R"cpp(
     #pragma once
     #include "self.h"
     ;
   )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
     ;
     #include "self.h"
   )cpp";
   AST = TU.build();
   EXPECT_THAT(*AST.getDiagnostics(), IsEmpty());
TU.Code = R"cpp(
     #define GUARD
     #include "self.h" // error-ok: FIXME, this would be nice to support
     #endif
     ;
   )cpp";
   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
     #define GUARD
     ;
     #include "self.h"
     #endif
   )cpp";
TEST(ParsedASTTest, HeaderGuardsSelfInclude) {
   TU.Code = R"cpp(
     #include "self.h" // error-ok
     #pragma once
     ;
   )cpp";
   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
     ;
     #pragma once
   )cpp";
   AST = TU.build();
   EXPECT_THAT(*AST.getDiagnostics(),
TEST(ParsedASTTest, HeaderGuardsImplIface) {
   TU.AdditionalFiles = {{"impl.h", Implementation}};
   auto AST = TU.build();
   EXPECT_THAT(*AST.getDiagnostics(), IsEmpty());
   EXPECT_FALSE(mainIsGuarded(AST)); // FIXME: true
   // Slightly harder: the `#pragma once` is part of the preamble, and we
   // 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.
   TU.Filename = "impl.h";
   TU.Code = Implementation;
   TU.AdditionalFiles = {{"iface.h", guard(Interface)}};
   AST = TU.build();
   // The diagnostic is unfortunate in this case, but correct per our model.