[clang-tools-extra] fae01d1 - [clangd] Fix bugs in main-file include patching for stale preambles

[Kadir Cetinkaya via cfe-commits]

Author: Kadir Cetinkaya
Date: 2023-02-13T09:49:13+01:00
New Revision: fae01d175a29270ec01211d3988c7ae57ddabfd3

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

LOG: [clangd] Fix bugs in main-file include patching for stale preambles

- Make sure main file includes are present even when they're not patched
  (because they didn't change or we're explicitly not patching them).
- Populate extra fields for includes, which can be used by include-cleaner.

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

Added: 
    

Modified: 
    clang-tools-extra/clangd/Preamble.cpp
    clang-tools-extra/clangd/unittests/PreambleTests.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp
index 15eea9bb036bd..cdb8db14722d2 100644
--- a/clang-tools-extra/clangd/Preamble.cpp
+++ b/clang-tools-extra/clangd/Preamble.cpp
@@ -671,10 +671,10 @@ PreamblePatch PreamblePatch::create(llvm::StringRef FileName,
     // We are only interested in newly added includes, record the ones in
     // Baseline for exclusion.
     llvm::DenseMap<std::pair<tok::PPKeywordKind, llvm::StringRef>,
-                   /*Resolved=*/llvm::StringRef>
+                   const Inclusion *>
         ExistingIncludes;
     for (const auto &Inc : Baseline.Includes.MainFileIncludes)
-      ExistingIncludes[{Inc.Directive, Inc.Written}] = Inc.Resolved;
+      ExistingIncludes[{Inc.Directive, Inc.Written}] = &Inc;
     // There might be includes coming from disabled regions, record these for
     // exclusion too. note that we don't have resolved paths for those.
     for (const auto &Inc : BaselineScan->Includes)
@@ -685,8 +685,13 @@ PreamblePatch PreamblePatch::create(llvm::StringRef FileName,
       // Include already present in the baseline preamble. Set resolved path and
       // put into preamble includes.
       if (It != ExistingIncludes.end()) {
-        Inc.Resolved = It->second.str();
-        PP.PreambleIncludes.push_back(Inc);
+        auto &PatchedInc = PP.PreambleIncludes.emplace_back();
+        // Copy everything from existing include, apart from the location, when
+        // it's coming from baseline preamble.
+        if (It->second)
+          PatchedInc = *It->second;
+        PatchedInc.HashLine = Inc.HashLine;
+        PatchedInc.HashOffset = Inc.HashOffset;
         continue;
       }
       // Include is new in the modified preamble. Inject it into the patch and
@@ -696,6 +701,11 @@ PreamblePatch PreamblePatch::create(llvm::StringRef FileName,
       Patch << llvm::formatv(
           "#{0} {1}\n", spellingForIncDirective(Inc.Directive), Inc.Written);
     }
+  } else {
+    // Make sure we have the full set of includes available even when we're not
+    // patching. As these are used by features we provide afterwards like hover,
+    // go-to-def or include-cleaner when preamble is stale.
+    PP.PreambleIncludes = Baseline.Includes.MainFileIncludes;
   }
 
   if (DirectivesChanged) {

diff  --git a/clang-tools-extra/clangd/unittests/PreambleTests.cpp b/clang-tools-extra/clangd/unittests/PreambleTests.cpp
index 1e95b62884f69..ac46bfd06a138 100644
--- a/clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ b/clang-tools-extra/clangd/unittests/PreambleTests.cpp
@@ -15,6 +15,7 @@
 #include "TestFS.h"
 #include "TestTU.h"
 #include "XRefs.h"
+#include "clang/Basic/SourceManager.h"
 #include "clang/Format/Format.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Frontend/PrecompiledPreamble.h"
@@ -23,6 +24,7 @@
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "gmock/gmock.h"
+#include "gtest/gtest-matchers.h"
 #include "gtest/gtest.h"
 #include <memory>
 #include <optional>
@@ -166,7 +168,7 @@ TEST(PreamblePatchTest, PatchesPreambleIncludes) {
   MockFS FS;
   IgnoreDiagnostics Diags;
   auto TU = TestTU::withCode(R"cpp(
-    #include "a.h"
+    #include "a.h" // IWYU pragma: keep
     #include "c.h"
   )cpp");
   TU.AdditionalFiles["a.h"] = "#include \"b.h\"";
@@ -185,9 +187,14 @@ TEST(PreamblePatchTest, PatchesPreambleIncludes) {
                                            *BaselinePreamble);
   // Only a.h should exists in the preamble, as c.h has been dropped and b.h was
   // newly introduced.
-  EXPECT_THAT(PP.preambleIncludes(),
-              ElementsAre(AllOf(Field(&Inclusion::Written, "\"a.h\""),
-                                Field(&Inclusion::Resolved, testPath("a.h")))));
+  EXPECT_THAT(
+      PP.preambleIncludes(),
+      ElementsAre(AllOf(
+          Field(&Inclusion::Written, "\"a.h\""),
+          Field(&Inclusion::Resolved, testPath("a.h")),
+          Field(&Inclusion::HeaderID, testing::Not(testing::Eq(std::nullopt))),
+          Field(&Inclusion::BehindPragmaKeep, true),
+          Field(&Inclusion::FileKind, SrcMgr::CharacteristicKind::C_User))));
 }
 
 std::optional<ParsedAST> createPatchedAST(llvm::StringRef Baseline,
@@ -225,6 +232,26 @@ std::string getPreamblePatch(llvm::StringRef Baseline,
       .str();
 }
 
+TEST(PreamblePatchTest, IncludesArePreserved) {
+  llvm::StringLiteral Baseline = R"(//error-ok
+#include <foo>
+#include <bar>
+)";
+  llvm::StringLiteral Modified = R"(//error-ok
+#include <foo>
+#include <bar>
+#define FOO)";
+
+  auto Includes = createPatchedAST(Baseline, Modified.str())
+                      ->getIncludeStructure()
+                      .MainFileIncludes;
+  EXPECT_TRUE(!Includes.empty());
+  EXPECT_EQ(Includes, TestTU::withCode(Baseline)
+                          .build()
+                          .getIncludeStructure()
+                          .MainFileIncludes);
+}
+
 TEST(PreamblePatchTest, Define) {
   // BAR should be defined while parsing the AST.
   struct {