[PATCH] D102761: [clangd] New ParsingCallback for semantics changes

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added subscribers: usaxena95, arphaman, javed.absar.
kadircet requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D102761

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/TUScheduler.h


Index: clang-tools-extra/clangd/TUScheduler.h
===================================================================
--- clang-tools-extra/clangd/TUScheduler.h
+++ clang-tools-extra/clangd/TUScheduler.h
@@ -169,6 +169,10 @@
 
   /// Called whenever the TU status is updated.
   virtual void onFileUpdated(PathRef File, const TUStatus &Status) {}
+
+  /// Semantics for the TU might've changed (e.g. a new preamble), any actions
+  /// on the file are guranteed to see new semantics after the callback.
+  virtual void onSemanticsMaybeChanged(PathRef File) {}
 };
 
 /// Handles running tasks for ClangdServer and managing the resources (e.g.,
Index: clang-tools-extra/clangd/TUScheduler.cpp
===================================================================
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -909,6 +909,7 @@
     ASTPeer.updatePreamble(std::move(Req.CI), std::move(Req.Inputs),
                            LatestBuild, std::move(Req.CIDiags),
                            std::move(Req.WantDiags));
+    Callbacks.onSemanticsMaybeChanged(FileName);
   });
 
   if (!LatestBuild || Inputs.ForceRebuild) {
Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -75,8 +75,6 @@
                      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 {
@@ -105,6 +103,11 @@
       ServerCallbacks->onFileUpdated(File, Status);
   }
 
+  void onSemanticsMaybeChanged(PathRef File) override {
+    if (ServerCallbacks)
+      ServerCallbacks->onSemanticsMaybeChanged(File);
+  }
+
 private:
   FileIndex *FIndex;
   ClangdServer::Callbacks *ServerCallbacks;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D102761.346410.patch
Type: text/x-patch
Size: 2028 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210519/a5edb11b/attachment-0001.bin>