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

[kadircet]

Summary

• 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.

[kadircet]

• Also populate additional fields

[sammccall]

Code looks reasonable, but I don't understand why the changes are being made - can you explain/link to a bug in the commit message?

clang-tools-extra/clangd/Preamble.cpp

688

This doesn't seem related to the patch description, can you update it?

690

why the null check?

691

seems a bit more future-proof to copy the whole thing and just overwrite the fields you want to preserve. (also would avoid the need for the comment)

701

this just echoes the code - say why instead?

(would also be good to have the reason in the commit message)

[kadircet]

• Address comments & update commit message

clang-tools-extra/clangd/Preamble.cpp

690

as discussed offline, we might have includes coming from scanned contents whose are not part of the preamble, e.g. because they're in a disabled PP region.

[sammccall]

clang-tools-extra/clangd/Preamble.cpp

692

if It->second is null, then all the #includes of this header from the baseline preamble were in disabled sections, so it's *very* likely this one is too.

I think we're better not pushing onto PP.PreambleIncludes at all in this case, rather than pushing an unresolved one - this is how the MainFileIncludes looks like when an #include is in a disabled section and there's no patching happening.

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

229

rawstrings?

[kadircet]

• use rawstrings in test

[kadircet]

clang-tools-extra/clangd/Preamble.cpp

692

agreed. as discussed sent out D143597

clang-tools-extra/clangd/Preamble.cpp

if (IncludesChanged && PatchType == PatchType::All) {
     for (const auto &Inc : BaselineScan->Includes)
       ExistingIncludes.try_emplace({Inc.Directive, Inc.Written});
     // Calculate extra includes that needs to be inserted.
     for (auto &Inc : ModifiedScan->Includes) {
       auto It = ExistingIncludes.find({Inc.Directive, Inc.Written});
       // 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);
         continue;
       }
       // Include is new in the modified preamble. Inject it into the patch and
       // use #line to set the presumed location to where it is spelled.
       auto LineCol = offsetToClangLineColumn(Modified.Contents, Inc.HashOffset);
       Patch << llvm::formatv("#line {0}\n", LineCol.first);
       Patch << llvm::formatv(
           "#{0} {1}\n", spellingForIncDirective(Inc.Directive), Inc.Written);
     }
+  } else {
+    // Includes haven't changed or we're not patching.
+    // So, copy them over from the baseline.
+    PP.PreambleIncludes = Baseline.Includes.MainFileIncludes;
   }
 
   if (DirectivesChanged) {
     // We need to patch all the directives, since they are order dependent. e.g:
     // #define BAR(X) NEW(X) // Newly introduced in Modified
     // #define BAR(X) OLD(X) // Exists in the Baseline
     //
     // If we've patched only the first directive, the macro definition would've

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

std::string getPreamblePatch(llvm::StringRef Baseline,
   MockFS FS;
   auto TU = TestTU::withCode(Modified);
   return PreamblePatch::createFullPatch(testPath("main.cpp"), TU.inputs(FS),
                                         *BaselinePreamble)
       .text()
       .str();
 }
 
+TEST(PreamblePatchTest, IncludesArePreserved) {
+  llvm::StringLiteral Baseline = "//error-ok\n#include <foo>\n#include <bar>\n";
+  auto Modified = Baseline + "#define FOO\n";
+
+  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 {
     const char *const Contents;
     const char *const ExpectedPatch;
   } Cases[] = {
       {
           R"cpp(