[clangd] New ParsingCallback for semantics changes

[kadircet]

Summary

Previously notification of the Server about semantic happened strictly
before notification of the AST thread.
Hence a racy Server could make a request (like semantic tokens) after
the notification, with the assumption that it'll be served fresh
content. But it wasn't true if AST thread wasn't notified about the
change yet.

This change reverses the order of those notifications to prevent racy
interactions.

[sammccall]

It seems like a lit test for this would be terrible. A ClangdServer one should be possible, but I can't quite wrap my head around how to write it.

(Delivering the PreambleData as a param would make the test easier, you could assert that the preamble version was > any previously notified preamble & also > any preamble previously obtained via runWithPreamble.)

clang-tools-extra/clangd/TUScheduler.h

175

This seems a little vague/abstract for TUScheduler.
The "caller" (i.e. ClangdServer impl) needs to know about preambles etc, c.f. onPreambleAST in this same interface.

I'd suggest onPreambleActive or onPreamblePublished or something like that.
(Passing the PreambleData struct as a param would also hint nicely at the distinction vs onPreambleAST but is maybe too cute if we're not actually going to use it...)

[kadircet]

• s/onSemanticsMaybeChanged/onPreamblePublished

[kadircet]

It seems like a lit test for this would be terrible. A ClangdServer one should be possible, but I can't quite wrap my head around how to write it.
(Delivering the PreambleData as a param would make the test easier, you could assert that the preamble version was > any previously notified preamble & also > any preamble previously obtained via runWithPreamble.)

I also put some thought into it, but couldn't figure out a way that wouldn't be flaky. we can write a test that might catch a regression, but it won't catch all of them for sure unless we somehow block the execution of threads.
I tried to find some locking solution that wouldn't depend too much on the TUScheduler's implementation details but couldn't find any, and eventually gave up :/.

I can include a test that ensures runWithAST always sees a new preamble after onPreamblePublished, but as mentioned it is going to be a flaky at best. WDYT?

clang-tools-extra/clangd/TUScheduler.h

175

going with onPreamblePublished, since Active might mean it wasn't previously before (e.g. first preamble for a TU).

clang-tools-extra/clangd/ClangdServer.cpp

UpdateIndexCallbacks(FileIndex *FIndex,
                        ClangdServer::Callbacks *ServerCallbacks)
       : FIndex(FIndex), ServerCallbacks(ServerCallbacks) {}
 
   void onPreambleAST(PathRef Path, llvm::StringRef Version, ASTContext &Ctx,
                      std::shared_ptr<clang::Preprocessor> PP,
                      const CanonicalIncludes &CanonIncludes) override {
     if (FIndex)
       FIndex->updatePreamble(Path, Version, Ctx, std::move(PP), CanonIncludes);
-    if (ServerCallbacks)
-      ServerCallbacks->onSemanticsMaybeChanged(Path);
   }
 
   void onMainAST(PathRef Path, ParsedAST &AST, PublishFn Publish) override {
     if (FIndex)
       FIndex->updateMain(Path, AST);
 
     assert(AST.getDiagnostics().hasValue() &&
            "We issue callback only with fresh preambles");
if (ServerCallbacks)
           [&]() { ServerCallbacks->onDiagnosticsReady(Path, Version, Diags); });
   }
 
   void onFileUpdated(PathRef File, const TUStatus &Status) override {
     if (ServerCallbacks)
       ServerCallbacks->onFileUpdated(File, Status);
   }
 
+  void onPreamblePublished(PathRef File) override {
+    if (ServerCallbacks)
+      ServerCallbacks->onSemanticsMaybeChanged(File);
+  }
+
 private:
   FileIndex *FIndex;
   ClangdServer::Callbacks *ServerCallbacks;
 };
 
 class DraftStoreFS : public ThreadsafeFS {
 public:
   DraftStoreFS(const ThreadsafeFS &Base, const DraftStore &Drafts)

clang-tools-extra/clangd/TUScheduler.h

public:
 
   /// Called whenever the AST fails to build. \p Diags will have the diagnostics
   /// that led to failure.
   virtual void onFailedAST(PathRef Path, llvm::StringRef Version,
                            std::vector<Diag> Diags, PublishFn Publish) {}
 
   /// Called whenever the TU status is updated.
   virtual void onFileUpdated(PathRef File, const TUStatus &Status) {}
+
+  /// Preamble for the TU have changed. This might imply new semantics (e.g.
+  /// different highlightings). Any actions on the file are guranteed to see new
+  /// preamble after the callback.
+  virtual void onPreamblePublished(PathRef File) {}
 };
 
 /// 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).

clang-tools-extra/clangd/TUScheduler.cpp

void PreambleThread::build(Request Req) {
 
   Status.update([&](TUStatus &Status) {
     Status.PreambleActivity = PreambleAction::Building;
   });
   auto _ = llvm::make_scope_exit([this, &Req] {
     ASTPeer.updatePreamble(std::move(Req.CI), std::move(Req.Inputs),
                            LatestBuild, std::move(Req.CIDiags),
                            std::move(Req.WantDiags));
+    Callbacks.onPreamblePublished(FileName);
   });
 
   if (!LatestBuild || Inputs.ForceRebuild) {
     vlog("Building first preamble for {0} version {1}", FileName,
          Inputs.Version);
   } else if (isPreambleCompatible(*LatestBuild, Inputs, FileName, *Req.CI)) {
     vlog("Reusing preamble version {0} for version {1} of {2}",
          LatestBuild->Version, Inputs.Version, FileName);