[clangd] Don't use DraftMgr in implementation of forceReparse.

[ilya-biryukov]

Summary

To make the removal of DraftMgr from ClangdServer easier (D44408).

[simark]

I rebased my patch (D44408) on top of this one, it looks good.

clangd/ClangdServer.cpp

143

I was wondering if we should increment the version here. From what I understand, it is used to identify each version of the parse, so that when diagnostics are ready, we know if they are for the latest version (and should be sent to the user) or if they are stale and should be ignored. If we don't change the document content but change the compile commands, I would consider it as a new version, since the diagnostics coming from the same document but old compile commands should be considered stale.

clangd/TUScheduler.h

62

"trakced"

[sammccall]

This patch is the right direction, but if you're up for it we can simplify interfaces further I think.

clangd/TUScheduler.h

62

tracked

69

(summarizing offline discussion)

this is so close to update, it'd be nice if we could just call update instead.

For that we need the contents, so forceReparse needs contents, so... can forceReparse just be addDocument(skipCache=true) or something?

Requiring content to be passed doesn't seem like a big burden in practice, and makes it clear that clangdserver is never responsible for maintaining copies of the content on the callers behalf (and clangdlspserver is).

reparseOpenFiles needs to move to clangdlspserver, but this seems consistent with the design. (so I think we can drop getTrackedFiles?)

[simark]

clangd/TUScheduler.h

69

I also thought it would be nice to have only one method update. What about if the Contents member of ParseInputs is optional? When it is not instantiated (such as when calling forceReparse), it would mean to re-use the previously sent source.

[sammccall]

clangd/ClangdServer.cpp

143

Diagnostics happen on the ASTWorker thread, of which there's only one per TU, so they're actually already serialized along with the decision of whether to publish.

The only "gap" is that if we add/remove/add the same document, we can get two concurrent astworkers that might deliver diagnostics out-of-order. That's on me to fix, at which point we can remove version comparisons from clangdserver entirely.

(We may still want a version concept - LSP has them, and it's a useful building block. One option is to use them only in ClangdLSPServer and propagate using context. This fails if ClangdServer starts watching disk for file changes, so maybe we'll want to explicitly pass opaque version tokens around at some point. But it's unrelated to the way versions are currently used)

clangd/TUScheduler.h

69

That would also work. If we can get away with just requiring the contents to always be passed in, I think it's simpler to understand.

[ilya-biryukov]

• Remove TUScheduler::updateCompileCommands, add a new optional parameter to clear the cache of compile commands instead
• Address review comments

[ilya-biryukov]

clangd/TUScheduler.h

69

That would also work. If we can get away with just requiring the contents to always be passed in, I think it's simpler to understand.

Also makes the implementation of update a bit simpler, because it doesn't have to care about missing files.
And ParseInputs is used in other places too, all of them need to be updated or we need two versions of ParseInputs: with optional and required contents.
Requiring contents for forceReparse seems easier. And when we got automated tracking of changes to compilation database, this will go away

[sammccall]

clangd/TUScheduler.cpp

220

you might want to inline this function here again - it's not overly long, and we're now back to one callsite

unittests/clangd/SyncAPI.h

23 ↗

(On Diff #138394)

it's slightly odd that wantdiagnostics is missing here. Previously this was "forwards compatible" and could easily be added later. But now we'll have callsites that pass different arguments than addDocument takes.

Maybe just add the defaulted param to the signature?

[ilya-biryukov]

• Add WantDiagnostics param to runAddDocument
• Revert changes in TUScheduler

unittests/clangd/SyncAPI.h

23 ↗

(On Diff #138394)

Done. I thought it's slightly confusing to have both Auto and Yes for synchronous case, but being consistent with addDocument is a good cause to do this.

[simark]

clangd/TUScheduler.h

69

Do you mean forceReparse will go away? Won't it still be required for when the user manually changes the compilation database path?

[cfe-commits]

• msg-4647-88.txt159 BDownload

clangd/ClangdServer.h

 
 private:
   /// FIXME: This stats several files to find a .clang-format file. I/O can be
   /// slow. Think of a way to cache this.
   llvm::Expected<tooling::Replacements>
   formatCode(llvm::StringRef Code, PathRef File,
              ArrayRef<tooling::Range> Ranges);
 
-  void scheduleReparseAndDiags(PathRef File, VersionedDraft Contents,
-                               WantDiagnostics WD,
-                               IntrusiveRefCntPtr<vfs::FileSystem> FS);
+  void consumeDiagnostics(PathRef File, DocVersion Version,
+                          std::vector<Diag> Diags);
 
   CompileArgsCache CompileArgs;
   DiagnosticsConsumer &DiagConsumer;
   FileSystemProvider &FSProvider;
   DraftStore DraftMgr;
   // The index used to look up symbols. This could be:
   //   - null (all index functionality is optional)
   //   - the dynamic index owned by ClangdServer (FileIdx)

clangd/ClangdServer.cpp

std::string NewRootPath = llvm::sys::path::convert_to_slash(
       RootPath, llvm::sys::path::Style::posix);
   if (llvm::sys::fs::is_directory(NewRootPath))
     this->RootPath = NewRootPath;
 }
 
 void ClangdServer::addDocument(PathRef File, StringRef Contents,
                                WantDiagnostics WantDiags) {
   DocVersion Version = DraftMgr.updateDraft(File, Contents);
-  scheduleReparseAndDiags(File, VersionedDraft{Version, Contents.str()},
-                          WantDiags, FSProvider.getFileSystem());
+  ParseInputs Inputs = {CompileArgs.getCompileCommand(File),
+                        FSProvider.getFileSystem(), Contents.str()};
+
+  Path FileStr = File.str();
+  WorkScheduler.update(File, std::move(Inputs), WantDiags,
+                       [this, FileStr, Version](std::vector<Diag> Diags) {
+                         consumeDiagnostics(FileStr, Version, std::move(Diags));
+                       });
 }
 
 void ClangdServer::removeDocument(PathRef File) {
   DraftMgr.removeDraft(File);
   CompileArgs.invalidate(File);
   WorkScheduler.remove(File);
 }
 
 void ClangdServer::forceReparse(PathRef File) {
-  auto FileContents = DraftMgr.getDraft(File);
-  assert(FileContents.Draft &&
-         "forceReparse() was called for non-added document");
-
   // forceReparse promises to request new compilation flags from CDB, so we
   // remove any cahced flags.
   CompileArgs.invalidate(File);
 
-  scheduleReparseAndDiags(File, std::move(FileContents), WantDiagnostics::Yes,
-                          FSProvider.getFileSystem());
+  tooling::CompileCommand NewCommand = CompileArgs.getCompileCommand(File);
+  DocVersion Version = DraftMgr.getVersion(File);
+  Path FileStr = File.str();
+  WorkScheduler.updateCompileCommand(
+      File, std::move(NewCommand), FSProvider.getFileSystem(),
+      WantDiagnostics::Yes, [this, FileStr, Version](std::vector<Diag> Diags) {
+        consumeDiagnostics(FileStr, Version, std::move(Diags));
+      });
 }
 
 void ClangdServer::codeComplete(PathRef File, Position Pos,
                                 const clangd::CodeCompleteOptions &Opts,
                                 Callback<CompletionList> CB) {
   // Copy completion options for passing them to async task handler.
   auto CodeCompleteOpts = Opts;
   if (!CodeCompleteOpts.Index) // Respect overridden index.
auto Action = [Pos, FS](Callback<Hover> CB,
     if (!InpAST)
       return CB(InpAST.takeError());
     CB(clangd::getHover(InpAST->AST, Pos));
   };
 
   WorkScheduler.runWithAST("Hover", File, Bind(Action, std::move(CB)));
 }
 
-void ClangdServer::scheduleReparseAndDiags(
-    PathRef File, VersionedDraft Contents, WantDiagnostics WantDiags,
-    IntrusiveRefCntPtr<vfs::FileSystem> FS) {
-  tooling::CompileCommand Command = CompileArgs.getCompileCommand(File);
-
-  DocVersion Version = Contents.Version;
-  Path FileStr = File.str();
-
-  auto Callback = [this, Version, FileStr](std::vector<Diag> Diags) {
-    // We need to serialize access to resulting diagnostics to avoid calling
-    // `onDiagnosticsReady` in the wrong order.
-    std::lock_guard<std::mutex> DiagsLock(DiagnosticsMutex);
-    DocVersion &LastReportedDiagsVersion = ReportedDiagnosticVersions[FileStr];
-    // FIXME(ibiryukov): get rid of '<' comparison here. In the current
-    // implementation diagnostics will not be reported after version counters'
-    // overflow. This should not happen in practice, since DocVersion is a
-    // 64-bit unsigned integer.
-    if (Version < LastReportedDiagsVersion)
-      return;
-    LastReportedDiagsVersion = Version;
+void ClangdServer::consumeDiagnostics(PathRef File, DocVersion Version,
+                                      std::vector<Diag> Diags) {
+  DocVersion &LastReportedDiagsVersion = ReportedDiagnosticVersions[File];
+
 
-    DiagConsumer.onDiagnosticsReady(FileStr, std::move(Diags));
-  };
-
-  WorkScheduler.update(File,
-                       ParseInputs{std::move(Command), std::move(FS),
-                                   std::move(*Contents.Draft)},
-                       WantDiags, std::move(Callback));
+  DiagConsumer.onDiagnosticsReady(File, std::move(Diags));
 }
 
 void ClangdServer::reparseOpenedFiles() {
-  for (const Path &FilePath : DraftMgr.getActiveFiles())
+  for (const Path &FilePath : WorkScheduler.getTrackedFiles())
     forceReparse(FilePath);
 }
 
 void ClangdServer::onFileEvent(const DidChangeWatchedFilesParams &Params) {
   // FIXME: Do nothing for now. This will be used for indexing and potentially
   // invalidating other caches.
 }
 

clangd/TUScheduler.h

TUScheduler(unsigned AsyncThreadsCount, bool StorePreamblesInMemory,
               ASTParsedCallback ASTCallback,
               std::chrono::steady_clock::duration UpdateDebounce);
   ~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;
 
+  /// Returns a list of currently tracked files. File starts being trakced on
+  /// first update() call to it and stops being tracked on remove() call.
+  std::vector<Path> getTrackedFiles() const;
+
+  /// Schedule a reparse for \p File with a new compile command. File must be
+  /// tracked.
+  /// FIXME: remove the callback from this function
+  void updateCompileCommand(PathRef File, tooling::CompileCommand NewCommand,
+                            IntrusiveRefCntPtr<vfs::FileSystem> FS,
+                            WantDiagnostics WantDiags,
+                            UniqueFunction<void(std::vector<Diag>)> OnUpdated);
+
   /// Schedule an update for \p File. Adds \p File to a list of tracked files if
   /// \p File was not part of it before.
   /// FIXME(ibiryukov): remove the callback from this function.
   void update(PathRef File, ParseInputs Inputs, WantDiagnostics WD,
               UniqueFunction<void(std::vector<Diag>)> OnUpdated);
 
   /// Remove \p File from the list of tracked files and schedule removal of its
   /// resources.

clangd/TUScheduler.cpp

void startTask(llvm::StringRef Name, UniqueFunction<void()> Task,
                  llvm::Optional<WantDiagnostics> UpdateType);
   /// Determines the next action to perform.
   /// All actions that should never run are disarded.
   /// Returns a deadline for the next action. If it's expired, run now.
   /// scheduleLocked() is called again at the deadline, or if requests arrive.
   Deadline scheduleLocked();
   /// Should the first task in the queue be skipped instead of run?
   bool shouldSkipHeadLocked() const;
+  /// Rebuilds AST and pushes the new diagnostics, if required.
+  void rebuildASTLocked(WantDiagnostics WantDiags,
+                        UniqueFunction<void(std::vector<Diag>)> OnUpdated);
 
   struct Request {
     UniqueFunction<void()> Action;
     std::string Name;
     steady_clock::time_point AddTime;
     Context Ctx;
     llvm::Optional<WantDiagnostics> UpdateType;
   };
#ifndef NDEBUG
   assert(Done && "handle was not destroyed");
   assert(Requests.empty() && "unprocessed requests when destroying ASTWorker");
 #endif
 }
 
 void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags,
                        UniqueFunction<void(std::vector<Diag>)> OnUpdated) {
   auto Task = [=](decltype(OnUpdated) OnUpdated) mutable {
-    FileInputs = Inputs;
-    auto Diags = AST.rebuild(std::move(Inputs));
-
-    {
-      std::lock_guard<std::mutex> Lock(Mutex);
-      if (AST.getPreamble())
-        LastBuiltPreamble = AST.getPreamble();
-      LastASTSize = AST.getUsedBytes();
-    }
-    // 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.
-    if (Diags && WantDiags != WantDiagnostics::No)
-      OnUpdated(std::move(*Diags));
+    FileInputs = std::move(Inputs);
+    rebuildASTLocked(WantDiags, std::move(OnUpdated));
   };
 
   startTask("Update", Bind(Task, std::move(OnUpdated)), WantDiags);
 }
 
 void ASTWorker::runWithAST(
     llvm::StringRef Name,
     UniqueFunction<void(llvm::Expected<InputsAndAST>)> Action) {
for (; Next != Requests.end(); ++Next)
       if (Next->UpdateType == WantDiagnostics::Yes ||
           Next->UpdateType == WantDiagnostics::Auto)
         return true; // Prefer later diagnostics.
     return false;
   }
   llvm_unreachable("Unknown WantDiagnostics");
 }
 
+void ASTWorker::rebuildASTLocked(
+    WantDiagnostics WantDiags,
+    UniqueFunction<void(std::vector<Diag>)> OnUpdated) {
+  auto Diags = AST.rebuild(ParseInputs(FileInputs));
+  {
+    std::lock_guard<std::mutex> Lock(Mutex);
+    if (AST.getPreamble())
+      LastBuiltPreamble = AST.getPreamble();
+    LastASTSize = AST.getUsedBytes();
+  }
+  // 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.
+  if (Diags && WantDiags != WantDiagnostics::No)
+    OnUpdated(std::move(*Diags));
+}
+
 bool ASTWorker::blockUntilIdle(Deadline Timeout) const {
   std::unique_lock<std::mutex> Lock(Mutex);
   return wait(Lock, RequestsCV, Timeout, [&] { return Requests.empty(); });
 }
 
 } // namespace
 
 unsigned getDefaultAsyncThreadsCount() {
for (auto &File : Files)
     if (!File.getValue()->Worker->blockUntilIdle(D))
       return false;
   if (PreambleTasks)
     if (!PreambleTasks->wait(D))
       return false;
   return true;
 }
 
+void TUScheduler::updateCompileCommand(
+    PathRef File, tooling::CompileCommand NewCommand,
+    IntrusiveRefCntPtr<vfs::FileSystem> FS, WantDiagnostics WantDiags,
+    UniqueFunction<void(std::vector<Diag>)> OnUpdated) {
+  auto FDIt = Files.find(File);
+  assert(FDIt != Files.end() &&
+         "calling updateCompileCommand on non-tracked file");
+
+  FDIt->second->Inputs.CompileCommand = std::move(NewCommand);
+  FDIt->second->Inputs.FS = std::move(FS);
+  FDIt->second->Worker->update(FDIt->second->Inputs, WantDiags,
+                               std::move(OnUpdated));
+}
+
 void TUScheduler::update(PathRef File, ParseInputs Inputs,
                          WantDiagnostics WantDiags,
                          UniqueFunction<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, WorkerThreads ? WorkerThreads.getPointer() : nullptr, Barrier,
TUScheduler::getUsedBytesPerFile() const {
   std::vector<std::pair<Path, std::size_t>> Result;
   Result.reserve(Files.size());
   for (auto &&PathAndFile : Files)
     Result.push_back(
         {PathAndFile.first(), PathAndFile.second->Worker->getUsedBytes()});
   return Result;
 }
 
+std::vector<Path> TUScheduler::getTrackedFiles() const {
+  return std::vector<Path>(Files.keys().begin(), Files.keys().end());
+}
+
 } // namespace clangd
 } // namespace clang