[clangd] Preserve line information while build PreamblePatch

[kadircet]

Summary

Depends on D78740.

[kadircet]

• Rebase, add comments and introduce Inclusion equality

[sammccall]

clang-tools-extra/clangd/Preamble.cpp

321

what's the purpose of this? to avoid repeating the filename everywhere in the patch? maybe add a comment "// set default filename for subsequent #line directives"

327

I doubt we'd ever bother fixing this.

331

(this needs an update for the deleting-R patch)

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

139

nit: baseline is one word

142

I missed this last time, but this test seems unneccesarily confusing

• can we just out the baseline and modified preambles instead of concatenating things together?
• duplicated headers is worth testing but hardly the only case. Can we have a removed header as well, as one that was moved but not duplicated?
• I think it'd be worth asserting the line numbers too

[kadircet]

• Rebase and address comments

[kadircet]

• Escape backslashes and quotes while generating line directive for filename

clang-tools-extra/clangd/Headers.h

struct Inclusion {
   Range R;                      // Inclusion range.
   tok::PPKeywordKind Directive; // Directive used for inclusion, e.g. import
   std::string Written;          // Inclusion name as written e.g. <vector>.
   Path Resolved; // Resolved path of included file. Empty if not resolved.
   unsigned HashOffset = 0; // Byte offset from start of file to #.
   SrcMgr::CharacteristicKind FileKind = SrcMgr::C_User;
 };
 llvm::raw_ostream &operator<<(llvm::raw_ostream &, const Inclusion &);
+bool operator==(const Inclusion &LHS, const Inclusion &RHS);
 
 // Contains information about one file in the build grpah and its direct
 // dependencies. Doesn't own the strings it references (IncludeGraph is
 // self-contained).
 struct IncludeGraphNode {
   enum class SourceFlag : uint8_t {
     None = 0,
     // Whether current file is a main file rather than a header.

clang-tools-extra/clangd/Headers.cpp

 }
 
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Inclusion &Inc) {
   return OS << Inc.Written << " = "
             << (!Inc.Resolved.empty() ? Inc.Resolved : "[unresolved]") << " at "
             << Inc.R;
 }
 
+bool operator==(const Inclusion &LHS, const Inclusion &RHS) {
+  return std::tie(LHS.Directive, LHS.FileKind, LHS.HashOffset, LHS.R,
+                  LHS.Resolved, LHS.Written) ==
+         std::tie(RHS.Directive, RHS.FileKind, RHS.HashOffset, RHS.R,
+                  RHS.Resolved, RHS.Written);
+}
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/Preamble.cpp

 //===--- Preamble.cpp - Reusing expensive parts of the AST ----------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Preamble.h"
 #include "Compiler.h"
 #include "Headers.h"
+#include "SourceCode.h"
 #include "support/Logger.h"
 #include "support/Trace.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/TokenKinds.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/FrontendActions.h"
PreamblePatch PreamblePatch::create(llvm::StringRef FileName,
   auto ModifiedIncludes = scanPreambleIncludes(
       Modified.Contents, Baseline.StatCache->getConsumingFS(Modified.FS),
       Modified.CompileCommand);
   if (!ModifiedIncludes) {
     elog("Failed to scan includes for modified contents of {0}: {1}", FileName,
          ModifiedIncludes.takeError());
     return {};
   }
+  // No patch needed if includes are equal.
+  if (*BaselineIncludes == *ModifiedIncludes)
+    return {};
 
   PreamblePatch PP;
   // This shouldn't coincide with any real file name.
   llvm::SmallString<128> PatchName;
   llvm::sys::path::append(PatchName, llvm::sys::path::parent_path(FileName),
                           "__preamble_patch__.h");
   PP.PatchFileName = PatchName.str().str();
 
   // We are only interested in newly added includes, record the ones in Baseline
   // for exclusion.
   llvm::DenseSet<std::pair<tok::PPKeywordKind, llvm::StringRef>>
       ExistingIncludes;
   for (const auto &Inc : *BaselineIncludes)
     ExistingIncludes.insert({Inc.Directive, Inc.Written});
   // Calculate extra includes that needs to be inserted.
   llvm::raw_string_ostream Patch(PP.PatchContents);
+  Patch << llvm::formatv("#line 0 \"{0}\"\n", FileName);
   for (const auto &Inc : *ModifiedIncludes) {
     if (ExistingIncludes.count({Inc.Directive, Inc.Written}))
       continue;
-    Patch << llvm::formatv("#{0} {1}\n", spellingForIncDirective(Inc.Directive),
-                           Inc.Written);
+    // 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
+    // FIXME: Traverse once.
+    auto LineCol = offsetToClangLineColumn(Modified.Contents, Inc.HashOffset);
+    Patch << llvm::formatv("#line {0}\n", LineCol.first);
+    llvm::StringRef Directive = spellingForIncDirective(Inc.Directive);
+    size_t Padding = Inc.R.start.character - Directive.size() - LineCol.second;
+    Patch << llvm::formatv("{0}#{1}{2}{3}\n",
+                           std::string(LineCol.second - 1, ' '), Directive,
+                           std::string(Padding, ' '), Inc.Written);
   }
   Patch.flush();
 
   // FIXME: Handle more directives, e.g. define/undef.
   return PP;
 }
 
 void PreamblePatch::apply(CompilerInvocation &CI) const {

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

 //===--- PreambleTests.cpp --------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Annotations.h"
 #include "Compiler.h"
 #include "Preamble.h"
 #include "TestFS.h"
+#include "TestTU.h"
+#include "clang/Frontend/PrecompiledPreamble.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include <clang/Frontend/FrontendActions.h>
 #include <string>
 #include <vector>
 
+using testing::Field;
+
 namespace clang {
 namespace clangd {
 namespace {
 
-using testing::_;
-using testing::Contains;
-using testing::Pair;
-
-MATCHER_P(HasContents, Contents, "") { return arg->getBuffer() == Contents; }
-
-TEST(PreamblePatchTest, IncludeParsing) {
-  MockFSProvider FS;
+// Builds a preamble for BaselineContents, patches it for ModifiedContents and
+// returns the includes in the patch.
+IncludeStructure collectPatchedIncludes(llvm::StringRef ModifiedContents,
+                                        llvm::StringRef BaselineContents) {
+  std::string MainFile = testPath("main.cpp");
+  ParseInputs PI;
+  PI.FS = new llvm::vfs::InMemoryFileSystem;
   MockCompilationDatabase CDB;
+  // ms-compatibility changes meaning of #import, make sure it is turned off.
+  CDB.ExtraClangFlags.push_back("-fno-ms-compatibility");
+  PI.CompileCommand = CDB.getCompileCommand(MainFile).getValue();
+  // Create invocation
   IgnoreDiagnostics Diags;
-  ParseInputs PI;
-  PI.FS = FS.getFileSystem();
+  auto CI = buildCompilerInvocation(PI, Diags);
+  assert(CI && "failed to create compiler invocation");
+  // Build baseline preamble.
+  PI.Contents = BaselineContents.str();
+  PI.Version = "baseline preamble";
+  auto BaselinePreamble = buildPreamble(MainFile, *CI, PI, true, nullptr);
+  assert(BaselinePreamble && "failed to build baseline preamble");
+  // Create the patch.
+  PI.Contents = ModifiedContents.str();
+  PI.Version = "modified contents";
+  auto PP = PreamblePatch::create(MainFile, PI, *BaselinePreamble);
+  // Collect patch contents.
+  PP.apply(*CI);
+  llvm::StringRef PatchContents;
+  for (const auto &Rempaped : CI->getPreprocessorOpts().RemappedFileBuffers) {
+    if (Rempaped.first == testPath("__preamble_patch__.h")) {
+      PatchContents = Rempaped.second->getBuffer();
+      break;
+    }
+  }
+  // Run preprocessor over the modified contents with patched Invocation to and
+  // BaselinePreamble to collect includes in the patch. We trim the input to
+  // only preamble section to not collect includes in the mainfile.
+  auto Bounds = Lexer::ComputePreamble(ModifiedContents, *CI->getLangOpts());
+  auto Clang =
+      prepareCompilerInstance(std::move(CI), &BaselinePreamble->Preamble,
+                              llvm::MemoryBuffer::getMemBufferCopy(
+                                  ModifiedContents.slice(0, Bounds.Size).str()),
+                              PI.FS, Diags);
+  Clang->getPreprocessorOpts().ImplicitPCHInclude.clear();
+  PreprocessOnlyAction Action;
+  if (!Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0])) {
+    ADD_FAILURE() << "failed begin source file";
+    return {};
+  }
+  IncludeStructure Includes;
+  Clang->getPreprocessor().addPPCallbacks(
+      collectIncludeStructureCallback(Clang->getSourceManager(), &Includes));
+  if (llvm::Error Err = Action.Execute()) {
+    ADD_FAILURE() << "failed to execute action: " << std::move(Err);
+    return {};
+  }
+  Action.EndSourceFile();
+  return Includes;
+}
 
+// Check preamble lexing logic by building an empty preamble and patching it
+// with all the contents.
+TEST(PreamblePatchTest, IncludeParsing) {
   // We expect any line with a point to show up in the patch.
   llvm::StringRef Cases[] = {
       // Only preamble
       R"cpp(^#include "a.h")cpp",
       // Both preamble and mainfile
       R"cpp(
         ^#include "a.h"
         garbage, finishes preamble
llvm::StringRef Cases[] = {
       // Broken directives
       R"cpp(
         #include "a
         ^#include "a.h"
         #include <b
         ^#include <b.h>)cpp",
   };
 
-  // ms-compatibility changes meaning of #import, make sure it is turned off.
-  CDB.ExtraClangFlags.push_back("-fno-ms-compatibility");
-  const auto FileName = testPath("foo.cc");
   for (const auto Case : Cases) {
     Annotations Test(Case);
     const auto Code = Test.code();
-    PI.CompileCommand = *CDB.getCompileCommand(FileName);
-
     SCOPED_TRACE(Code);
-    // Check preamble lexing logic by building an empty preamble and patching it
-    // with all the contents.
-    PI.Contents = "";
-    const auto CI = buildCompilerInvocation(PI, Diags);
-    const auto EmptyPreamble = buildPreamble(FileName, *CI, PI, true, nullptr);
-    PI.Contents = Code.str();
-
-    std::string ExpectedBuffer;
-    const auto Points = Test.points();
-    for (const auto &P : Points) {
-      // Copy the whole line.
-      auto StartOffset = llvm::cantFail(positionToOffset(Code, P));
-      ExpectedBuffer.append(Code.substr(StartOffset)
-                                .take_until([](char C) { return C == '\n'; })
-                                .str());
-      ExpectedBuffer += '\n';
-    }
-
-    PreamblePatch::create(FileName, PI, *EmptyPreamble).apply(*CI);
-    EXPECT_THAT(CI->getPreprocessorOpts().RemappedFileBuffers,
-                Contains(Pair(_, HasContents(ExpectedBuffer))));
-    for (const auto &RB : CI->getPreprocessorOpts().RemappedFileBuffers)
-      delete RB.second;
+
+    auto Includes =
+        collectPatchedIncludes(Code, /*BaselineContents=*/"").MainFileIncludes;
+    auto Points = Test.points();
+    EXPECT_EQ(Includes.size(), Points.size());
+    for (size_t I = 0, E = Includes.size(); I != E; ++I) {
+      auto &Inc = Includes[I];
+      auto StartOffset = llvm::cantFail(positionToOffset(Code, Points[I]));
+      EXPECT_EQ(Inc.HashOffset, StartOffset);
+    }
   }
 }
 
 TEST(PreamblePatchTest, ContainsNewIncludes) {
-  MockFSProvider FS;
-  MockCompilationDatabase CDB;
-  IgnoreDiagnostics Diags;
-  // ms-compatibility changes meaning of #import, make sure it is turned off.
-  CDB.ExtraClangFlags.push_back("-fno-ms-compatibility");
-
-  const auto FileName = testPath("foo.cc");
-  ParseInputs PI;
-  PI.FS = FS.getFileSystem();
-  PI.CompileCommand = *CDB.getCompileCommand(FileName);
-  PI.Contents = "#include <existing.h>\n";
-
-  // Check diffing logic by adding a new header to the preamble and ensuring
-  // only it is patched.
-  const auto CI = buildCompilerInvocation(PI, Diags);
-  const auto FullPreamble = buildPreamble(FileName, *CI, PI, true, nullptr);
-
+  constexpr llvm::StringLiteral BaseLineContents = "#include <existing.h>\n";
   constexpr llvm::StringLiteral Patch =
       "#include <test>\n#import <existing.h>\n";
   // We provide the same includes twice, they shouldn't be included in the
   // patch.
-  PI.Contents = (Patch + PI.Contents + PI.Contents).str();
-  PreamblePatch::create(FileName, PI, *FullPreamble).apply(*CI);
-  EXPECT_THAT(CI->getPreprocessorOpts().RemappedFileBuffers,
-              Contains(Pair(_, HasContents(Patch))));
-  for (const auto &RB : CI->getPreprocessorOpts().RemappedFileBuffers)
-    delete RB.second;
+  auto Includes =
+      collectPatchedIncludes(
+          (Patch + BaseLineContents + BaseLineContents).str(), BaseLineContents)
+          .MainFileIncludes;
+  EXPECT_THAT(Includes,
+              ElementsAre(Field(&Inclusion::Written, "<test>"),
+                          Field(&Inclusion::Written, "<existing.h>")));
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang