[clangd] Patch PP directives to use stale preambles while building ASTs

[kadircet]

Summary

Depends on D79930.

This enables more accurate parsing of the AST, by making changes in PP
directives in preamble section visible. This is handled by injecting conditional
directives and macro defines/undefs. Note that include insertions are handled
separately in D77644, to not duplicate existing include headers.

This patch doesn't handle any location mappings yet, so features like go-to-def,
go-to-refs and hover might not work as expected. These will be addressed in a
follow-up patch.

[kadircet]

• Only store defines
• Make use of source text directly instead of printing tokens

[kadircet]

• MacroInfo's definition range is a token range, convert it to a char range.

[sammccall]

clang-tools-extra/clangd/Preamble.cpp

415

Hmm, with N macros in the file and O(1) changed, it seems like the inaccuracy of the conditional-scanning may outweigh defining that one in the wrong order in complex cases.

Especially since this only seems to come up if there are multiple definitions of the same macro, which seems easy enough to detect if that's an important case.

427

don't you need a #line directive too? Seems like you're not using the DirectiveLine anywhere.

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

205

do you think it makes sense to have a test that just asserts on the contents of the preamble patch? it seems like a more direct way to test some of these things.

These tests are nice, but debugging them seems like it might be a bit of work.

[kadircet]

clang-tools-extra/clangd/Preamble.cpp

415

makes sense, will change it to only patch new defines. And actually most of the time any define directive below this one would also be considered new, because line information has changed for them, hence we need to patch. Unless user both inserts and deletes a line.

427

spoileers. that's in D80198, I didn't want to make this one any bigger.

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

205

I had 2 reasons for not doing that:

• Not clobbering the API of PreamblePatch just for testing.
• Making tests less fragile for changes in the patch format.

I suppose the first one is not that important, but I believe the latter is quite useful. We can always to something like hassubstr I suppose, but in the end we only care about its effect while creating an AST. We can do something like hassubstr, but we would still need these tests to ensure they interact as expected with the preprocessor.

[kadircet]

• Rebase

[sammccall]

clang-tools-extra/clangd/Preamble.cpp

110–167

this is worth a comment saying what this is used for: directives *other* than includes where we need only basic structure.

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

205

Agree that having robust tests of the whole thing is important.

Maybe we could add an additional (sorry) single smoke test that asserts on the whole patch?
It'd be brittle indeed but those diffs would actually be really interesting to me as a reader of the code and reviewer of changes... gives a different perspective on what the code is doing.

I don't think having a text() or textForTests() accessor is too bad, especially if it's trivial.

[kadircet]

• Assert on patch contents, using regexes for main file match.

[kadircet]

• Rebase

[kadircet]

• Rebase

clang-tools-extra/clangd/Preamble.cpp

 #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/SourceManager.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"
private:
   IncludeStructure Includes;
   CanonicalIncludes CanonIncludes;
   MainFileMacros Macros;
   std::unique_ptr<CommentHandler> IWYUHandler = nullptr;
   const clang::LangOptions *LangOpts = nullptr;
   const SourceManager *SourceMgr = nullptr;
 };
 
-/// Gets the includes in the preamble section of the file by running
-/// preprocessor over \p Contents. Returned includes do not contain resolved
-/// paths. \p VFS and \p Cmd is used to build the compiler invocation, which
-/// might stat/read files.
-llvm::Expected<std::vector<Inclusion>>
-scanPreambleIncludes(llvm::StringRef Contents,
-                     llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS,
-                     const tooling::CompileCommand &Cmd) {
+struct TextualPPDirective {
+  unsigned DirectiveLine;
+  // Full text that's representing the directive, including the `#`.
+  std::string Text;
+
+  bool operator==(const TextualPPDirective &RHS) const {
+    return std::tie(DirectiveLine, Text) ==
+           std::tie(RHS.DirectiveLine, RHS.Text);
+  }
+};
+
+// Collects #define directives inside the main file.
+struct DirectiveCollector : public PPCallbacks {
+  DirectiveCollector(const Preprocessor &PP,
+                     std::vector<TextualPPDirective> &TextualDirectives)
+      : SM(PP.getSourceManager()), TextualDirectives(TextualDirectives) {}
+
+  void FileChanged(SourceLocation Loc, FileChangeReason Reason,
+                   SrcMgr::CharacteristicKind FileType,
+                   FileID PrevFID) override {
+    InMainFile = SM.isWrittenInMainFile(Loc);
+  }
+
+  void MacroDefined(const Token &MacroNameTok,
+                    const MacroDirective *MD) override {
+    if (!InMainFile)
+      return;
+    TextualDirectives.emplace_back();
+    TextualPPDirective &TD = TextualDirectives.back();
+
+    auto DecompLoc = SM.getDecomposedLoc(MacroNameTok.getLocation());
+    TD.DirectiveLine = SM.getLineNumber(DecompLoc.first, DecompLoc.second);
+
+    SourceRange DefRange(MD->getMacroInfo()->getDefinitionLoc(),
+                         MD->getMacroInfo()->getDefinitionEndLoc());
+    llvm::raw_string_ostream OS(TD.Text);
+    OS << "#define " << toSourceCode(SM, DefRange);
+  }
+
+private:
+  bool InMainFile = true;
+  const SourceManager &SM;
+  std::vector<TextualPPDirective> &TextualDirectives;
+};
+
+struct ScannedPreamble {
+  std::vector<Inclusion> Includes;
+  std::vector<TextualPPDirective> TextualDirectives;
+};
+
+/// Scans the preprocessor directives in the preamble section of the file by
+/// running preprocessor over \p Contents. Returned includes do not contain
+/// resolved paths. \p VFS and \p Cmd is used to build the compiler invocation,
+/// which might stat/read files.
+llvm::Expected<ScannedPreamble>
+scanPreamble(llvm::StringRef Contents,
   // Build and run Preprocessor over the preamble.
   ParseInputs PI;
   PI.Contents = Contents.str();
   PI.FS = std::move(VFS);
   PI.CompileCommand = Cmd;
   IgnoringDiagConsumer IgnoreDiags;
   auto CI = buildCompilerInvocation(PI, IgnoreDiags);
   if (!CI)
if (Clang->getFrontendOpts().Inputs.empty())
     return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                    "compiler instance had no inputs");
   // We are only interested in main file includes.
   Clang->getPreprocessorOpts().SingleFileParseMode = true;
   PreprocessOnlyAction Action;
   if (!Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0]))
     return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                    "failed BeginSourceFile");
+  const auto &SM = Clang->getSourceManager();
   Preprocessor &PP = Clang->getPreprocessor();
   IncludeStructure Includes;
+  PP.addPPCallbacks(collectIncludeStructureCallback(SM, &Includes));
+  ScannedPreamble SP;
   PP.addPPCallbacks(
-      collectIncludeStructureCallback(Clang->getSourceManager(), &Includes));
+      std::make_unique<DirectiveCollector>(PP, SP.TextualDirectives));
   if (llvm::Error Err = Action.Execute())
     return std::move(Err);
   Action.EndSourceFile();
-  return Includes.MainFileIncludes;
+  SP.Includes = std::move(Includes.MainFileIncludes);
+  return SP;
 }
 
 const char *spellingForIncDirective(tok::PPKeywordKind IncludeDirective) {
   switch (IncludeDirective) {
   case tok::pp_include:
     return "include";
   case tok::pp_import:
     return "import";
for (char C : Text) {
     OS << C;
   }
 }
 
 PreamblePatch PreamblePatch::create(llvm::StringRef FileName,
                                     const ParseInputs &Modified,
                                     const PreambleData &Baseline) {
   assert(llvm::sys::path::is_absolute(FileName) && "relative FileName!");
-  // First scan the include directives in Baseline and Modified. These will be
+  // First scan preprocessor 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
   //   whole preamble, which is terribly slow.
   // - If scanning for Modified fails, cannot figure out newly added ones so
   //   there's nothing to do but generate an empty patch.
-  auto BaselineIncludes = scanPreambleIncludes(
+  auto BaselineScan = scanPreamble(
       // Contents needs to be null-terminated.
       Baseline.Preamble.getContents().str(),
       Baseline.StatCache->getConsumingFS(Modified.FS), Modified.CompileCommand);
-  if (!BaselineIncludes) {
-    elog("Failed to scan includes for baseline of {0}: {1}", FileName,
-         BaselineIncludes.takeError());
-    return {};
+  if (!BaselineScan) {
+    elog("Failed to scan baseline of {0}: {1}", FileName,
+         BaselineScan.takeError());
+    return PreamblePatch::unmodified(Baseline);
   }
-  auto ModifiedIncludes = scanPreambleIncludes(
+  auto ModifiedScan = scanPreamble(
       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 {};
+  if (!ModifiedScan) {
+    elog("Failed to scan modified contents of {0}: {1}", FileName,
+         ModifiedScan.takeError());
+    return PreamblePatch::unmodified(Baseline);
   }
-  // No patch needed if includes are equal.
-  if (*BaselineIncludes == *ModifiedIncludes)
+
+  bool IncludesChanged = BaselineScan->Includes != ModifiedScan->Includes;
+  bool DirectivesChanged =
+      BaselineScan->TextualDirectives != ModifiedScan->TextualDirectives;
+  if (!IncludesChanged && !DirectivesChanged)
     return PreamblePatch::unmodified(Baseline);
 
   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::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";
+
+  if (IncludesChanged) {
+    // 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>
         ExistingIncludes;
     for (const auto &Inc : Baseline.Includes.MainFileIncludes)
       ExistingIncludes[{Inc.Directive, Inc.Written}] = Inc.Resolved;
     // 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 : *BaselineIncludes)
+    for (const auto &Inc : BaselineScan->Includes)
       ExistingIncludes.try_emplace({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 (auto &Inc : *ModifiedIncludes) {
+    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.
+      // 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 << llvm::formatv(
+          "#{0} {1}\n", spellingForIncDirective(Inc.Directive), Inc.Written);
+    }
   }
-  Patch.flush();
 
-  // FIXME: Handle more directives, e.g. define/undef.
+  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
+    // been wrong for the rest of the file, since patch is applied after the
+    // baseline preamble.
+    //
+    // Note that we deliberately ignore conditional directives and undefs to
+    // reduce complexity. The former might cause problems because scanning is
+    // imprecise and might pick directives from disabled regions.
+    for (const auto &TD : ModifiedScan->TextualDirectives)
+      Patch << TD.Text << '\n';
+  }
+  dlog("Created preamble patch: {0}", Patch.str());
+  Patch.flush();
   return PP;
 }
 
 void PreamblePatch::apply(CompilerInvocation &CI) const {
   // No need to map an empty file.
   if (PatchContents.empty())
     return;
   auto &PPOpts = CI.getPreprocessorOpts();

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

 #include "llvm/Support/VirtualFileSystem.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <clang/Frontend/FrontendActions.h>
 #include <memory>
 #include <string>
 #include <vector>
 
+using testing::Contains;
 using testing::Field;
 
 namespace clang {
 namespace clangd {
 namespace {
 
 MATCHER_P2(Distance, File, D, "") {
   return arg.first() == File && arg.second == D;
TEST(PreamblePatchTest, PatchesPreambleIncludes) {
   auto PP = PreamblePatch::create(testPath(TU.Filename), TU.inputs(),
                                   *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")))));
 }
+
+llvm::Optional<ParsedAST> createPatchedAST(llvm::StringRef Baseline,
+                                           llvm::StringRef Modified) {
+  auto BaselinePreamble = TestTU::withCode(Baseline).buildPreamble();
+  if (!BaselinePreamble) {
+    ADD_FAILURE() << "Failed to build baseline preamble";
+    return llvm::None;
+  }
+
+  IgnoreDiagnostics Diags;
+  auto TU = TestTU::withCode(Modified);
+  auto CI = buildCompilerInvocation(TU.inputs(), Diags);
+  if (!CI) {
+    ADD_FAILURE() << "Failed to build compiler invocation";
+    return llvm::None;
+  }
+  return ParsedAST::build(testPath("main.cpp"), TU.inputs(), std::move(CI), {},
+                          BaselinePreamble);
+}
+
+TEST(PreamblePatchTest, Define) {
+  // BAR should be defined while parsing the AST.
+  llvm::StringLiteral Cases[] = {
+      R"cpp(
+        #define BAR(X) X
+        [[BAR]](int y);)cpp",
+      // multiline macro
+      R"cpp(
+        #define BAR(X) \
+                X
+        [[BAR]](int y);)cpp",
+  };
+
+  for (llvm::StringLiteral Case : Cases) {
+    Annotations Modified(Case);
+    auto AST = createPatchedAST("", Modified.code());
+    ASSERT_TRUE(AST);
+    EXPECT_THAT(AST->getDiagnostics(),
+                Not(Contains(Field(&Diag::Range, Modified.range()))));
+  }
+}
+
+TEST(PreamblePatchTest, OrderingPreserved) {
+  llvm::StringLiteral Baseline = "#define BAR(X) X";
+  Annotations Modified(R"cpp(
+    #define BAR(X, Y) X Y
+    #define BAR(X) X
+    [[BAR]](int y);
+  )cpp");
+
+  auto AST = createPatchedAST(Baseline, Modified.code());
+  ASSERT_TRUE(AST);
+  EXPECT_THAT(AST->getDiagnostics(),
+              Not(Contains(Field(&Diag::Range, Modified.range()))));
+}
 } // namespace
 } // namespace clangd
 } // namespace clang