[clangd] Add callbacks on parsed AST in addition to parsed preambles

[ilya-biryukov]

Summary

Will be used for updating the dynamic index on updates to the open files.
Currently we collect only information coming from the preamble
AST. This has a bunch of limitations:

• Dynamic index misses important information from the body of the file, e.g. locations of definitions.
• XRefs cannot be collected at all, since we can only obtain full information for the current file (preamble is parsed with skipped function bodies, therefore not reliable).

This patch only adds the new callback, actually updating the index
will be added in a follow-up patch.

[ioeric]

Dynamic index misses important information from the body of the file, e.g. locations of definitions
XRefs cannot be collected at all, since we can only obtain full information for the current file (preamble is parsed with skipped function bodies, therefore not reliable).

These seem to be known limitations of the dynamic index. The dynamic index, in general, only handles things that are in headers; information in the main file can usually be obtained from Sema/AST (outside of the index). For example, definition/xrefs locations can be merged from index and the AST.

This patch only adds the new callback, actually updating the index will be added in a follow-up patch.

How do we plan to merge symbols from the preamble callback and the main file callback? The current FileIndex only allows swapping all symbols.

[ilya-biryukov]

In D50847#1202658, @ioeric wrote:

Dynamic index misses important information from the body of the file, e.g. locations of definitions
XRefs cannot be collected at all, since we can only obtain full information for the current file (preamble is parsed with skipped function bodies, therefore not reliable).

These seem to be known limitations of the dynamic index. The dynamic index, in general, only handles things that are in headers; information in the main file can usually be obtained from Sema/AST (outside of the index). For example, definition/xrefs locations can be merged from index and the AST.

We still want this information in the index. One can obtain this information for the current file. However, we still need to keep this info in the index for definitions in the other open files.

This patch only adds the new callback, actually updating the index will be added in a follow-up patch.

How do we plan to merge symbols from the preamble callback and the main file callback? The current FileIndex only allows swapping all symbols.

There are multiple ways to do that, lowest overhead is having two separate slabs: one for preamble symbols, another for symbols in the main file. They will be merged on-the-go when requested.

[ilya-biryukov]

@ioeric, specifically, see D50889 for an updated FileIndex

[hokein]

Thanks for adding it.

clangd/TUScheduler.cpp

406 ↗

(On Diff #161038)

The AST might not get built if WantDiags::No, and will be built in runWithAST.

clangd/TUScheduler.h

66 ↗

(On Diff #161038)

Does the callback get called every time we built an AST? clangd only has 3 idle ASTs, if the AST is not there, we rebuild it when needed (even the source code is not changed), and we will update the dynamic index, which seems unnecessary.

It may rarely happen, 3 idle ASTs might cover most cases, I think?

[ilya-biryukov]

clangd/TUScheduler.cpp

406 ↗

(On Diff #161038)

I suggest we keep it a known limitation and not rebuild the index in that case.
The original intent of WantDiags::No was to update the contents of the file, so that upcoming request can see the new contents (e.g. this is a hack to avoid passing overriden contents of the file in the request itself).
WantDiags::No is always followed by an actual request that wants the diagnostics (and will, therefore, build the AST and emit the callback).

Alternatively, we could unify the code paths building the AST. Would be a bit more intrusive change, but I guess it's worth it.

clangd/TUScheduler.h

66 ↗

(On Diff #161038)

Currently we only call this when AST is built for diagnostics, so we will have only a single callback, even if the AST will be rebuilt multiple times because it was flushed out of the cash (as long as the file contents didn't change, of course).

So we do behave optimally in that case and I suggest we keep it this way

[ioeric]

clangd/TUScheduler.h

66 ↗

(On Diff #161038)

is there any overlap between preamble AST and main AST? If so, could you elaborate in the documentation?

[hokein]

The interfaces look mostly good to me.

clangd/ClangdServer.cpp

73 ↗

(On Diff #161038)

I think Ctx should be a pointer which gives us a way (passing a nullptr) to clean up the FileIndex, the same as ParsedAST below.

And I discover that we don't cleanup dynamic index when a file is being closed, is it expected or a bug?

clangd/TUScheduler.cpp

406 ↗

(On Diff #161038)

I see, SG, thanks!

clangd/TUScheduler.h

66 ↗

(On Diff #161038)

Currently we only call this when AST is built for diagnostics, so we will have only a single callback, even if the AST will be rebuilt multiple times because it was flushed out of the cash (as long as the file contents didn't change, of course).

It sounds reasonable, thanks for the explanation. Could you clarify it in the comment?

is there any overlap between preamble AST and main AST? If so, could you elaborate in the documentation?

AFAIK, both of them contain different TopLevelDecls, but it is still possible to get the Decl* (declared in header) when visiting the main AST (e.g. when visiting the void foo() {} definition in MainAST, we can still get the void foo(); declaration in PreambleAST).

[ilya-biryukov]

• Provide noop callbacks implementation by default.
• Update docs.

[ilya-biryukov]

clangd/ClangdServer.cpp

73 ↗

(On Diff #161038)

I suggest we add an extra method to DynamicIndex that we call when the file is closed. I don't think it's intentional that we don't clean up the index for the closed files.
Not sure what's the best way to handle invalid ASTs, but we're never calling the current callback with nullptr anyway, so I suggest we keep it a reference to better reflect that callbacks don't actually handle any errors for us.

[hokein]

Thanks! Looks good.

clangd/ClangdServer.cpp

73 ↗

(On Diff #161038)

SG, and it is out of scope of this patch. Let's figure it out in the clean-up-index patch.

[ilya-biryukov]

clangd/ClangdServer.cpp

73 ↗

(On Diff #161038)

LG, I'll come up with a follow-up cleanup patch when done with these patches

clang-tools-extra/trunk/clangd/ClangdServer.h

private:
   // The index used to look up symbols. This could be:
   //   - null (all index functionality is optional)
   //   - the dynamic index owned by ClangdServer (FileIdx)
   //   - the static index passed to the constructor
   //   - a merged view of a static and dynamic index (MergedIndex)
   SymbolIndex *Index;
   // If present, an up-to-date of symbols in open files. Read via Index.
   std::unique_ptr<FileIndex> FileIdx;
+  /// Callbacks responsible for updating FileIdx.
+  std::unique_ptr<ParsingCallbacks> FileIdxUpdater;
   // If present, a merged view of FileIdx and an external index. Read via Index.
   std::unique_ptr<SymbolIndex> MergedIndex;
   // If set, this represents the workspace path.
   llvm::Optional<std::string> RootPath;
   std::shared_ptr<PCHContainerOperations> PCHs;
   /// Used to serialize diagnostic callbacks.
   /// FIXME(ibiryukov): get rid of an extra map and put all version counters
   /// into CppFile.

clang-tools-extra/trunk/clangd/ClangdServer.cpp

public:
   void handle(tooling::AtomicChanges SourceReplacements) override {
     assert(!Result.hasValue());
     Result = std::move(SourceReplacements);
   }
 
   Optional<Expected<tooling::AtomicChanges>> Result;
 };
 
+class UpdateFileIndex : public ParsingCallbacks {
+public:
+  UpdateFileIndex(FileIndex *FileIdx) : FileIdx(FileIdx) {}
+
+  void onPreambleAST(PathRef Path, ASTContext &Ctx,
+                     std::shared_ptr<clang::Preprocessor> PP) override {
+    if (FileIdx)
+      FileIdx->update(Path, &Ctx, std::move(PP));
+  }
+
+  void onMainAST(PathRef Path, ParsedAST &AST) override {
+    // FIXME: merge results from the main file into the index too.
+  }
+
+private:
+  FileIndex *FileIdx;
+};
+
 } // namespace
 
 ClangdServer::Options ClangdServer::optsForTest() {
   ClangdServer::Options Opts;
   Opts.UpdateDebounce = std::chrono::steady_clock::duration::zero(); // Faster!
   Opts.StorePreamblesInMemory = true;
   Opts.AsyncThreadsCount = 4; // Consistent!
   return Opts;
 }
 
 ClangdServer::ClangdServer(GlobalCompilationDatabase &CDB,
                            FileSystemProvider &FSProvider,
                            DiagnosticsConsumer &DiagConsumer,
                            const Options &Opts)
     : CDB(CDB), DiagConsumer(DiagConsumer), FSProvider(FSProvider),
       ResourceDir(Opts.ResourceDir ? Opts.ResourceDir->str()
                                    : getStandardResourceDir()),
       FileIdx(Opts.BuildDynamicSymbolIndex ? new FileIndex(Opts.URISchemes)
                                            : nullptr),
+      FileIdxUpdater(llvm::make_unique<UpdateFileIndex>(FileIdx.get())),
       PCHs(std::make_shared<PCHContainerOperations>()),
       // Pass a callback into `WorkScheduler` to extract symbols from a newly
       // parsed file and rebuild the file index synchronously each time an AST
       // is parsed.
       // FIXME(ioeric): this can be slow and we may be able to index on less
       // critical paths.
-      WorkScheduler(
-          Opts.AsyncThreadsCount, Opts.StorePreamblesInMemory,
-          FileIdx
-              ? [this](PathRef Path, ASTContext &AST,
-                       std::shared_ptr<Preprocessor>
-                           PP) { FileIdx->update(Path, &AST, std::move(PP)); }
-              : PreambleParsedCallback(),
-          Opts.UpdateDebounce, Opts.RetentionPolicy) {
+      WorkScheduler(Opts.AsyncThreadsCount, Opts.StorePreamblesInMemory,
+                    *FileIdxUpdater, Opts.UpdateDebounce,
+                    Opts.RetentionPolicy) {
   if (FileIdx && Opts.StaticIndex) {
     MergedIndex = mergeIndex(FileIdx.get(), Opts.StaticIndex);
     Index = MergedIndex.get();
   } else if (FileIdx)
     Index = FileIdx.get();
   else if (Opts.StaticIndex)
     Index = Opts.StaticIndex;
   else

clang-tools-extra/trunk/clangd/TUScheduler.h

 /// *idle* ASTs. If queue has operations requiring the AST, they might be
 /// kept in memory.
 struct ASTRetentionPolicy {
   /// Maximum number of ASTs to be retained in memory when there are no pending
   /// requests for them.
   unsigned MaxRetainedASTs = 3;
 };
 
+class ParsingCallbacks {
+public:
+  virtual ~ParsingCallbacks() = default;
+
+  /// Called on the AST that was built for emitting the preamble. The built AST
+  /// contains only AST nodes from the #include directives at the start of the
+  /// file. AST node in the current file should be observed on onMainAST call.
+  virtual void onPreambleAST(PathRef Path, ASTContext &Ctx,
+                             std::shared_ptr<clang::Preprocessor> PP) {}
+  /// Called on the AST built for the file itself. Note that preamble AST nodes
+  /// are not deserialized and should be processed in the onPreambleAST call
+  /// instead.
+  /// The \p AST always contains all AST nodes for the main file itself, and
+  /// only a portion of the AST nodes deserialized from the preamble. Note that
+  /// some nodes from the preamble may have been deserialized and may also be
+  /// accessed from the main file AST, e.g. redecls of functions from preamble,
+  /// etc. Clients are expected to process only the AST nodes from the main file
+  /// in this callback (obtained via ParsedAST::getLocalTopLevelDecls) to obtain
+  /// optimal performance.
+  virtual void onMainAST(PathRef Path, ParsedAST &AST) {}
+};
+
+ParsingCallbacks &noopParsingCallbacks();
+
 /// Handles running tasks for ClangdServer and managing the resources (e.g.,
 /// preambles and ASTs) for opened files.
 /// TUScheduler is not thread-safe, only one thread should be providing updates
 /// and scheduling tasks.
 /// Callbacks are run on a threadpool and it's appropriate to do slow work in
 /// them. Each task has a name, used for tracing (should be UpperCamelCase).
 /// FIXME(sammccall): pull out a scheduler options struct.
 class TUScheduler {
 public:
   TUScheduler(unsigned AsyncThreadsCount, bool StorePreamblesInMemory,
-              PreambleParsedCallback PreambleCallback,
+              ParsingCallbacks &ASTCallbacks,
               std::chrono::steady_clock::duration UpdateDebounce,
               ASTRetentionPolicy RetentionPolicy);
   ~TUScheduler();
 
   /// Returns estimated memory usage for each of the currently open files.
   /// The order of results is unspecified.
   std::vector<std::pair<Path, std::size_t>> getUsedBytesPerFile() const;
 
public:
   // this inside clangd.
   // FIXME: remove this when there is proper index support via build system
   // integration.
   static llvm::Optional<llvm::StringRef> getFileBeingProcessedInContext();
 
 private:
   const bool StorePreamblesInMemory;
   const std::shared_ptr<PCHContainerOperations> PCHOps;
-  const PreambleParsedCallback PreambleCallback;
+  ParsingCallbacks &Callbacks;
   Semaphore Barrier;
   llvm::StringMap<std::unique_ptr<FileData>> Files;
   std::unique_ptr<ASTCache> IdleASTs;
   // None when running tasks synchronously and non-None when running tasks
   // asynchronously.
   llvm::Optional<AsyncTaskRunner> PreambleTasks;
   llvm::Optional<AsyncTaskRunner> WorkerThreads;
   std::chrono::steady_clock::duration UpdateDebounce;
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/trunk/clangd/TUScheduler.cpp

 /// signals the worker to exit its run loop and gives up shared ownership of the
 /// worker.
 class ASTWorker {
   friend class ASTWorkerHandle;
   ASTWorker(PathRef FileName, TUScheduler::ASTCache &LRUCache,
             Semaphore &Barrier, bool RunSync,
             steady_clock::duration UpdateDebounce,
             std::shared_ptr<PCHContainerOperations> PCHs,
-            bool StorePreamblesInMemory,
-            PreambleParsedCallback PreambleCallback);
+            bool StorePreamblesInMemory, ParsingCallbacks &Callbacks);
 
 public:
   /// Create a new ASTWorker and return a handle to it.
   /// The processing thread is spawned using \p Tasks. However, when \p Tasks
   /// is null, all requests will be processed on the calling thread
   /// synchronously instead. \p Barrier is acquired when processing each
   /// request, it is be used to limit the number of actively running threads.
   static ASTWorkerHandle create(PathRef FileName,
                                 TUScheduler::ASTCache &IdleASTs,
                                 AsyncTaskRunner *Tasks, Semaphore &Barrier,
                                 steady_clock::duration UpdateDebounce,
                                 std::shared_ptr<PCHContainerOperations> PCHs,
                                 bool StorePreamblesInMemory,
-                                PreambleParsedCallback PreambleCallback);
+                                ParsingCallbacks &Callbacks);
   ~ASTWorker();
 
   void update(ParseInputs Inputs, WantDiagnostics,
               llvm::unique_function<void(std::vector<Diag>)> OnUpdated);
   void
   runWithAST(llvm::StringRef Name,
              llvm::unique_function<void(llvm::Expected<InputsAndAST>)> Action);
   bool blockUntilIdle(Deadline Timeout) const;
private:
   TUScheduler::ASTCache &IdleASTs;
   const bool RunSync;
   /// Time to wait after an update to see whether another update obsoletes it.
   const steady_clock::duration UpdateDebounce;
   /// File that ASTWorker is reponsible for.
   const Path FileName;
   /// Whether to keep the built preambles in memory or on disk.
   const bool StorePreambleInMemory;
-  /// Callback, passed to the preamble builder.
-  const PreambleParsedCallback PreambleCallback;
+  /// Callback, invoked when preamble or main file AST is built.
+  ParsingCallbacks &Callbacks;
   /// Helper class required to build the ASTs.
   const std::shared_ptr<PCHContainerOperations> PCHs;
 
   Semaphore &Barrier;
   /// Inputs, corresponding to the current state of AST.
   ParseInputs FileInputs;
   /// Whether the diagnostics for the current FileInputs were reported to the
   /// users before.

 };
 
 ASTWorkerHandle ASTWorker::create(PathRef FileName,
                                   TUScheduler::ASTCache &IdleASTs,
                                   AsyncTaskRunner *Tasks, Semaphore &Barrier,
                                   steady_clock::duration UpdateDebounce,
                                   std::shared_ptr<PCHContainerOperations> PCHs,
                                   bool StorePreamblesInMemory,
-                                  PreambleParsedCallback PreambleCallback) {
+                                  ParsingCallbacks &Callbacks) {
   std::shared_ptr<ASTWorker> Worker(new ASTWorker(
       FileName, IdleASTs, Barrier, /*RunSync=*/!Tasks, UpdateDebounce,
-      std::move(PCHs), StorePreamblesInMemory, std::move(PreambleCallback)));
+      std::move(PCHs), StorePreamblesInMemory, Callbacks));
   if (Tasks)
     Tasks->runAsync("worker:" + llvm::sys::path::filename(FileName),
                     [Worker]() { Worker->run(); });
 
   return ASTWorkerHandle(std::move(Worker));
 }
 
 ASTWorker::ASTWorker(PathRef FileName, TUScheduler::ASTCache &LRUCache,
                      Semaphore &Barrier, bool RunSync,
                      steady_clock::duration UpdateDebounce,
                      std::shared_ptr<PCHContainerOperations> PCHs,
-                     bool StorePreamblesInMemory,
-                     PreambleParsedCallback PreambleCallback)
+                     bool StorePreamblesInMemory, ParsingCallbacks &Callbacks)
     : IdleASTs(LRUCache), RunSync(RunSync), UpdateDebounce(UpdateDebounce),
       FileName(FileName), StorePreambleInMemory(StorePreamblesInMemory),
-      PreambleCallback(std::move(PreambleCallback)), PCHs(std::move(PCHs)),
-      Barrier(Barrier), Done(false) {}
+      Callbacks(Callbacks), PCHs(std::move(PCHs)), Barrier(Barrier),
+      Done(false) {}
 
 ASTWorker::~ASTWorker() {
   // Make sure we remove the cached AST, if any.
   IdleASTs.take(this);
 #ifndef NDEBUG
   std::lock_guard<std::mutex> Lock(Mutex);
   assert(Done && "handle was not destroyed");
   assert(Requests.empty() && "unprocessed requests when destroying ASTWorker");
if (!Invocation) {
       PreambleWasBuilt.notify();
       return;
     }
 
     std::shared_ptr<const PreambleData> OldPreamble =
         getPossiblyStalePreamble();
     std::shared_ptr<const PreambleData> NewPreamble =
         buildPreamble(FileName, *Invocation, OldPreamble, OldCommand, Inputs,
-                      PCHs, StorePreambleInMemory, PreambleCallback);
+                      PCHs, StorePreambleInMemory,
+                      [this](PathRef Path, ASTContext &Ctx,
+                             std::shared_ptr<clang::Preprocessor> PP) {
+                        Callbacks.onPreambleAST(FileName, Ctx, std::move(PP));
+                      });
 
     bool CanReuseAST = InputsAreTheSame && (OldPreamble == NewPreamble);
     {
       std::lock_guard<std::mutex> Lock(Mutex);
       LastBuiltPreamble = NewPreamble;
     }
     // Before doing the expensive AST reparse, we want to release our reference
     // to the old preamble, so it can be freed if there are no other references
if (!AST) {
       AST = NewAST ? llvm::make_unique<ParsedAST>(std::move(*NewAST)) : nullptr;
     }
     // We want to report the diagnostics even if this update was cancelled.
     // It seems more useful than making the clients wait indefinitely if they
     // spam us with updates.
     // Note *AST can be still be null if buildAST fails.
     if (*AST) {
       OnUpdated((*AST)->getDiagnostics());
+      Callbacks.onMainAST(FileName, **AST);
       DiagsWereReported = true;
     }
     // Stash the AST in the cache for further use.
     IdleASTs.put(this, std::move(*AST));
   };
 
   startTask("Update", Bind(Task, std::move(OnUpdated)), WantDiags);
 }
unsigned getDefaultAsyncThreadsCount() {
   // C++ standard says that hardware_concurrency()
   // may return 0, fallback to 1 worker thread in
   // that case.
   if (HardwareConcurrency == 0)
     return 1;
   return HardwareConcurrency;
 }
 
+ParsingCallbacks &noopParsingCallbacks() {
+  static ParsingCallbacks *Instance = new ParsingCallbacks;
+  return *Instance;
+}
+
 struct TUScheduler::FileData {
   /// Latest inputs, passed to TUScheduler::update().
   std::string Contents;
   tooling::CompileCommand Command;
   ASTWorkerHandle Worker;
 };
 
 TUScheduler::TUScheduler(unsigned AsyncThreadsCount,
                          bool StorePreamblesInMemory,
-                         PreambleParsedCallback PreambleCallback,
+                         ParsingCallbacks &Callbacks,
                          std::chrono::steady_clock::duration UpdateDebounce,
                          ASTRetentionPolicy RetentionPolicy)
     : StorePreamblesInMemory(StorePreamblesInMemory),
-      PCHOps(std::make_shared<PCHContainerOperations>()),
-      PreambleCallback(std::move(PreambleCallback)), Barrier(AsyncThreadsCount),
+      PCHOps(std::make_shared<PCHContainerOperations>()), Callbacks(Callbacks),
+      Barrier(AsyncThreadsCount),
       IdleASTs(llvm::make_unique<ASTCache>(RetentionPolicy.MaxRetainedASTs)),
       UpdateDebounce(UpdateDebounce) {
   if (0 < AsyncThreadsCount) {
     PreambleTasks.emplace();
     WorkerThreads.emplace();
   }
 }
 

 void TUScheduler::update(
     PathRef File, ParseInputs Inputs, WantDiagnostics WantDiags,
     llvm::unique_function<void(std::vector<Diag>)> OnUpdated) {
   std::unique_ptr<FileData> &FD = Files[File];
   if (!FD) {
     // Create a new worker to process the AST-related tasks.
     ASTWorkerHandle Worker = ASTWorker::create(
         File, *IdleASTs, WorkerThreads ? WorkerThreads.getPointer() : nullptr,
-        Barrier, UpdateDebounce, PCHOps, StorePreamblesInMemory,
-        PreambleCallback);
+        Barrier, UpdateDebounce, PCHOps, StorePreamblesInMemory, Callbacks);
     FD = std::unique_ptr<FileData>(new FileData{
         Inputs.Contents, Inputs.CompileCommand, std::move(Worker)});
   } else {
     FD->Contents = Inputs.Contents;
     FD->Command = Inputs.CompileCommand;
   }
   FD->Worker->update(std::move(Inputs), WantDiags, std::move(OnUpdated));
 }

clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp

 #include "TestFS.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <algorithm>
 #include <utility>
 
 namespace clang {
 namespace clangd {
+namespace {
 
 using ::testing::_;
-using ::testing::Each;
 using ::testing::AnyOf;
+using ::testing::Each;
 using ::testing::Pair;
 using ::testing::Pointee;
 using ::testing::UnorderedElementsAre;
 
 void ignoreUpdate(llvm::Optional<std::vector<Diag>>) {}
 void ignoreError(llvm::Error Err) {
   handleAllErrors(std::move(Err), [](const llvm::ErrorInfoBase &) {});
 }
 
 class TUSchedulerTests : public ::testing::Test {
 protected:
   ParseInputs getInputs(PathRef File, std::string Contents) {
     return ParseInputs{*CDB.getCompileCommand(File),
                        buildTestFS(Files, Timestamps), std::move(Contents)};
   }
 
   llvm::StringMap<std::string> Files;
   llvm::StringMap<time_t> Timestamps;
   MockCompilationDatabase CDB;
 };
 
 TEST_F(TUSchedulerTests, MissingFiles) {
   TUScheduler S(getDefaultAsyncThreadsCount(),
-                /*StorePreamblesInMemory=*/true,
-                /*PreambleParsedCallback=*/nullptr,
+                /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
                 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
                 ASTRetentionPolicy());
 
   auto Added = testPath("added.cpp");
   Files[Added] = "";
 
   auto Missing = testPath("missing.cpp");
   Files[Missing] = "";

 TEST_F(TUSchedulerTests, WantDiagnostics) {
   std::atomic<int> CallbackCount(0);
   {
     // To avoid a racy test, don't allow tasks to actualy run on the worker
     // thread until we've scheduled them all.
     Notification Ready;
     TUScheduler S(
         getDefaultAsyncThreadsCount(),
-        /*StorePreamblesInMemory=*/true,
-        /*PreambleParsedCallback=*/nullptr,
+        /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
         /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
         ASTRetentionPolicy());
     auto Path = testPath("foo.cpp");
     S.update(Path, getInputs(Path, ""), WantDiagnostics::Yes,
              [&](std::vector<Diag>) { Ready.wait(); });
 
     S.update(Path, getInputs(Path, "request diags"), WantDiagnostics::Yes,
              [&](std::vector<Diag> Diags) { ++CallbackCount; });
TEST_F(TUSchedulerTests, WantDiagnostics) {
   }
   EXPECT_EQ(2, CallbackCount);
 }
 
 TEST_F(TUSchedulerTests, Debounce) {
   std::atomic<int> CallbackCount(0);
   {
     TUScheduler S(getDefaultAsyncThreadsCount(),
-                  /*StorePreamblesInMemory=*/true,
-                  /*PreambleParsedCallback=*/nullptr,
+                  /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
                   /*UpdateDebounce=*/std::chrono::seconds(1),
                   ASTRetentionPolicy());
     // FIXME: we could probably use timeouts lower than 1 second here.
     auto Path = testPath("foo.cpp");
     S.update(Path, getInputs(Path, "auto (debounced)"), WantDiagnostics::Auto,
              [&](std::vector<Diag> Diags) {
                ADD_FAILURE() << "auto should have been debounced and canceled";
              });
TEST_F(TUSchedulerTests, ManyUpdates) {
   std::mutex Mut;
   int TotalASTReads = 0;
   int TotalPreambleReads = 0;
   int TotalUpdates = 0;
 
   // Run TUScheduler and collect some stats.
   {
     TUScheduler S(getDefaultAsyncThreadsCount(),
-                  /*StorePreamblesInMemory=*/true,
-                  /*PreambleParsedCallback=*/nullptr,
+                  /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
                   /*UpdateDebounce=*/std::chrono::milliseconds(50),
                   ASTRetentionPolicy());
 
     std::vector<std::string> Files;
     for (int I = 0; I < FilesCount; ++I) {
       std::string Name = "foo" + std::to_string(I) + ".cpp";
       Files.push_back(testPath(Name));
       this->Files[Files.back()] = "";

 }
 
 TEST_F(TUSchedulerTests, EvictedAST) {
   std::atomic<int> BuiltASTCounter(0);
   ASTRetentionPolicy Policy;
   Policy.MaxRetainedASTs = 2;
   TUScheduler S(
       /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
-      PreambleParsedCallback(),
+      noopParsingCallbacks(),
       /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
 
   llvm::StringLiteral SourceContents = R"cpp(
     int* a;
     double* b = a;
   )cpp";
   llvm::StringLiteral OtherSourceContents = R"cpp(
     int* a;
TEST_F(TUSchedulerTests, EvictedAST) {
   // evicted.
   EXPECT_THAT(S.getFilesWithCachedAST(),
               UnorderedElementsAre(Foo, AnyOf(Bar, Baz)));
 }
 
 TEST_F(TUSchedulerTests, EmptyPreamble) {
   TUScheduler S(
       /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-      PreambleParsedCallback(),
+      noopParsingCallbacks(),
       /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
       ASTRetentionPolicy());
 
   auto Foo = testPath("foo.cpp");
   auto Header = testPath("foo.h");
 
   Files[Header] = "void foo()";
   Timestamps[Header] = time_t(0);
S.runWithPreamble("getEmptyPreamble", Foo,
                     });
 }
 
 TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
   // Testing strategy: we update the file and schedule a few preamble reads at
   // the same time. All reads should get the same non-null preamble.
   TUScheduler S(
       /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-      PreambleParsedCallback(),
+      noopParsingCallbacks(),
       /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
       ASTRetentionPolicy());
   auto Foo = testPath("foo.cpp");
   auto NonEmptyPreamble = R"cpp(
     #define FOO 1
     #define BAR 2
 
     int main() {}
TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
   std::lock_guard<std::mutex> Lock(PreamblesMut);
   ASSERT_NE(Preambles[0], nullptr);
   ASSERT_THAT(Preambles, Each(Preambles[0]));
 }
 
 TEST_F(TUSchedulerTests, NoopOnEmptyChanges) {
   TUScheduler S(
       /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
-      /*StorePreambleInMemory=*/true, PreambleParsedCallback(),
+      /*StorePreambleInMemory=*/true, noopParsingCallbacks(),
       /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
       ASTRetentionPolicy());
 
   auto Source = testPath("foo.cpp");
   auto Header = testPath("foo.h");
 
   Files[Header] = "int a;";
   Timestamps[Header] = time_t(0);
TEST_F(TUSchedulerTests, NoopOnEmptyChanges) {
   CDB.ExtraClangFlags.push_back("-DSOMETHING");
   ASSERT_TRUE(DoUpdate(getInputs(Source, OtherSourceContents)));
   ASSERT_FALSE(DoUpdate(getInputs(Source, OtherSourceContents)));
 }
 
 TEST_F(TUSchedulerTests, NoChangeDiags) {
   TUScheduler S(
       /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
-      /*StorePreambleInMemory=*/true, PreambleParsedCallback(),
+      /*StorePreambleInMemory=*/true, noopParsingCallbacks(),
       /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
       ASTRetentionPolicy());
 
   auto FooCpp = testPath("foo.cpp");
   auto Contents = "int a; int b;";
 
   S.update(FooCpp, getInputs(FooCpp, Contents), WantDiagnostics::No,
            [](std::vector<Diag>) { ADD_FAILURE() << "Should not be called."; });
TEST_F(TUSchedulerTests, NoChangeDiags) {
   // Subsequent request does not get any diagnostics callback because the same
   // diags have previously been reported and the inputs didn't change.
   S.update(
       FooCpp, getInputs(FooCpp, Contents), WantDiagnostics::Auto,
       [&](std::vector<Diag>) { ADD_FAILURE() << "Should not be called."; });
   ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
 }
 
+} // namespace
 } // namespace clangd
 } // namespace clang