[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

336

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"

352

I doubt we'd ever bother fixing this.

356

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

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

137–138

nit: baseline is one word

137–138

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 {
   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 #.
   int HashLine = 0;        // Line number containing the directive, 0-indexed.
   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 line" << Inc.HashLine;
 }
 
+bool operator==(const Inclusion &LHS, const Inclusion &RHS) {
+  return std::tie(LHS.Directive, LHS.FileKind, LHS.HashOffset, LHS.HashLine,
+                  LHS.Resolved, LHS.Written) ==
+         std::tie(RHS.Directive, RHS.FileKind, RHS.HashOffset, RHS.HashLine,
+                  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"
 #include "clang/Lex/Lexer.h"
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/VirtualFileSystem.h"
bool isPreambleCompatible(const PreambleData &Preamble,
   auto Bounds =
       ComputePreambleBounds(*CI.getLangOpts(), ContentsBuffer.get(), 0);
   return compileCommandsAreEqual(Inputs.CompileCommand,
                                  Preamble.CompileCommand) &&
          Preamble.Preamble.CanReuse(CI, ContentsBuffer.get(), Bounds,
                                     Inputs.FS.get());
 }
 
+void escapeBackslashAndQuotes(llvm::StringRef Text, llvm::raw_ostream &OS) {
+  for (char C : Text) {
+    switch (C) {
+    case '\\':
+    case '"':
+      OS << '\\';
+      break;
+    default:
+      break;
+    }
+    OS << C;
+  }
+}
+
 PreamblePatch PreamblePatch::create(llvm::StringRef FileName,
                                     const ParseInputs &Modified,
                                     const PreambleData &Baseline) {
   // First scan the include directives in Baseline and Modified. These will be
   // used to figure out newly added directives in Modified. Scanning can fail,
   // the code just bails out and creates an empty patch in such cases, as:
   // - If scanning for Baseline fails, no knowledge of existing includes hence
   //   patch will contain all the includes in Modified. Leading to rebuild of
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);
+  // Set default filename for subsequent #line directives
+  Patch << "#line 0 \"";
+  // FileName part of a line directive is subject to backslash escaping, which
+  // might lead to problems on windows especially.
+  escapeBackslashAndQuotes(FileName, Patch);
+  Patch << "\"\n";
   for (const auto &Inc : *ModifiedIncludes) {
     if (ExistingIncludes.count({Inc.Directive, Inc.Written}))
       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);
   }
   Patch.flush();
 
   // FIXME: Handle more directives, e.g. define/undef.
   return PP;
 }
 
 void PreamblePatch::apply(CompilerInvocation &CI) const {
   // No need to map an empty file.
   if (PatchContents.empty())
     return;
   auto &PPOpts = CI.getPreprocessorOpts();
   auto PatchBuffer =
       // we copy here to ensure contents are still valid if CI outlives the

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,
+                       llvm::StringRef MainFileName = "main.cpp") {
+  std::string MainFile = testPath(MainFileName);
+  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();
+    ASSERT_EQ(Includes.size(), Points.size());
+    for (size_t I = 0, E = Includes.size(); I != E; ++I)
+      EXPECT_EQ(Includes[I].HashLine, Points[I].line);
   }
 }
 
 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 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;
+  constexpr llvm::StringLiteral BaselineContents = R"cpp(
+    #include <a.h>
+    #include <b.h> // This will be removed
+    #include <c.h>
+  )cpp";
+  constexpr llvm::StringLiteral ModifiedContents = R"cpp(
+    #include <a.h>
+    #include <c.h> // This has changed a line.
+    #include <c.h> // This is a duplicate.
+    #include <d.h> // This is newly introduced.
+  )cpp";
+  auto Includes = collectPatchedIncludes(ModifiedContents, BaselineContents)
+                      .MainFileIncludes;
+  EXPECT_THAT(Includes, ElementsAre(AllOf(Field(&Inclusion::Written, "<d.h>"),
+                                          Field(&Inclusion::HashLine, 4))));
 }
 
+TEST(PreamblePatchTest, MainFileIsEscaped) {
+  auto Includes = collectPatchedIncludes("#include <a.h>", "", "file\"name.cpp")
+                      .MainFileIncludes;
+  EXPECT_THAT(Includes, ElementsAre(AllOf(Field(&Inclusion::Written, "<a.h>"),
+                                          Field(&Inclusion::HashLine, 0))));
+}
 } // namespace
 } // namespace clangd
 } // namespace clang