[clang-tools-extra] 8506877 - [clangd] Copy existing includes in ReplayPreamble

[Kadir Cetinkaya via cfe-commits]

Author: Kadir Cetinkaya
Date: 2020-06-02T14:31:45+02:00
New Revision: 8506877c87aa602736aee1fffbd80b886fa40b79

URL: https://github.com/llvm/llvm-project/commit/8506877c87aa602736aee1fffbd80b886fa40b79
DIFF: https://github.com/llvm/llvm-project/commit/8506877c87aa602736aee1fffbd80b886fa40b79.diff

LOG: [clangd] Copy existing includes in ReplayPreamble

Summary:
ReplayPreamble was just grabbing the reference of IncludeStructure
passed to it and then replayed any includes seen so while exiting
built-in file.

This implies any include seen in built-in files being replayed as part
of preamble, even though they are not. This wasn't an issue until we've
started patching preambles, as includes from built-in files were not
mapped back to main-file.

This patch copies over existing includes at the time of
ReplayPreamble::attach and only replies those to prevent any includes
from the preamble patch getting mixed-in.

Reviewers: sammccall, jkorous

Subscribers: ilya-biryukov, MaskRay, dexonsmith, arphaman, usaxena95, cfe-commits

Tags: #clang

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

Added: 
    

Modified: 
    clang-tools-extra/clangd/ParsedAST.cpp
    clang-tools-extra/clangd/unittests/ParsedASTTests.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp
index 082c7cae0021..f3568c34f99a 100644
--- a/clang-tools-extra/clangd/ParsedAST.cpp
+++ b/clang-tools-extra/clangd/ParsedAST.cpp
@@ -116,7 +116,7 @@ class ReplayPreamble : private PPCallbacks {
   // Attach preprocessor hooks such that preamble events will be injected at
   // the appropriate time.
   // Events will be delivered to the *currently registered* PP callbacks.
-  static void attach(const IncludeStructure &Includes, CompilerInstance &Clang,
+  static void attach(std::vector<Inclusion> Includes, CompilerInstance &Clang,
                      const PreambleBounds &PB) {
     auto &PP = Clang.getPreprocessor();
     auto *ExistingCallbacks = PP.getPPCallbacks();
@@ -124,7 +124,7 @@ class ReplayPreamble : private PPCallbacks {
     if (!ExistingCallbacks)
       return;
     PP.addPPCallbacks(std::unique_ptr<PPCallbacks>(new ReplayPreamble(
-        Includes, ExistingCallbacks, Clang.getSourceManager(), PP,
+        std::move(Includes), ExistingCallbacks, Clang.getSourceManager(), PP,
         Clang.getLangOpts(), PB)));
     // We're relying on the fact that addPPCallbacks keeps the old PPCallbacks
     // around, creating a chaining wrapper. Guard against other implementations.
@@ -133,10 +133,10 @@ class ReplayPreamble : private PPCallbacks {
   }
 
 private:
-  ReplayPreamble(const IncludeStructure &Includes, PPCallbacks *Delegate,
+  ReplayPreamble(std::vector<Inclusion> Includes, PPCallbacks *Delegate,
                  const SourceManager &SM, Preprocessor &PP,
                  const LangOptions &LangOpts, const PreambleBounds &PB)
-      : Includes(Includes), Delegate(Delegate), SM(SM), PP(PP) {
+      : Includes(std::move(Includes)), Delegate(Delegate), SM(SM), PP(PP) {
     // Only tokenize the preamble section of the main file, as we are not
     // interested in the rest of the tokens.
     MainFileTokens = syntax::tokenize(
@@ -167,7 +167,7 @@ class ReplayPreamble : private PPCallbacks {
   }
 
   void replay() {
-    for (const auto &Inc : Includes.MainFileIncludes) {
+    for (const auto &Inc : Includes) {
       const FileEntry *File = nullptr;
       if (Inc.Resolved != "")
         if (auto FE = SM.getFileManager().getFile(Inc.Resolved))
@@ -227,7 +227,7 @@ class ReplayPreamble : private PPCallbacks {
     }
   }
 
-  const IncludeStructure &Includes;
+  const std::vector<Inclusion> Includes;
   PPCallbacks *Delegate;
   const SourceManager &SM;
   Preprocessor &PP;
@@ -382,7 +382,8 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
     Includes = Preamble->Includes;
     Includes.MainFileIncludes = Patch->preambleIncludes();
     // Replay the preamble includes so that clang-tidy checks can see them.
-    ReplayPreamble::attach(Includes, *Clang, Preamble->Preamble.getBounds());
+    ReplayPreamble::attach(Patch->preambleIncludes(), *Clang,
+                           Preamble->Preamble.getBounds());
   }
   // Important: collectIncludeStructure is registered *after* ReplayPreamble!
   // Otherwise we would collect the replayed includes again...

diff  --git a/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp b/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
index d86f741d9e72..0ad4a9f20ccb 100644
--- a/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
@@ -328,6 +328,8 @@ TEST(ParsedASTTest, CollectsMainFileMacroExpansions) {
               testing::UnorderedElementsAreArray(TestCase.points()));
 }
 
+MATCHER_P(WithFileName, Inc, "") { return arg.FileName == Inc; }
+
 TEST(ParsedASTTest, ReplayPreambleForTidyCheckers) {
   struct Inclusion {
     Inclusion(const SourceManager &SM, SourceLocation HashLoc,
@@ -439,6 +441,24 @@ TEST(ParsedASTTest, ReplayPreambleForTidyCheckers) {
         Code.substr(FileRange.Begin - 1, FileRange.End - FileRange.Begin + 2));
     EXPECT_EQ(SkippedFiles[I].kind(), tok::header_name);
   }
+
+  // Make sure replay logic works with patched preambles.
+  TU.Code = "";
+  StoreDiags Diags;
+  auto Inputs = TU.inputs();
+  auto CI = buildCompilerInvocation(Inputs, Diags);
+  auto EmptyPreamble =
+      buildPreamble(testPath(TU.Filename), *CI, Inputs, true, nullptr);
+  ASSERT_TRUE(EmptyPreamble);
+  TU.Code = "#include <a.h>";
+  Includes.clear();
+  auto PatchedAST = ParsedAST::build(testPath(TU.Filename), TU.inputs(),
+                                     std::move(CI), {}, EmptyPreamble);
+  ASSERT_TRUE(PatchedAST);
+  // Make sure includes were seen only once.
+  EXPECT_THAT(Includes,
+              ElementsAre(WithFileName(testPath("__preamble_patch__.h")),
+                          WithFileName("a.h")));
 }
 
 TEST(ParsedASTTest, PatchesAdditionalIncludes) {