[clangd] Track document versions, include them with diags, enhance logs

Summary:
This ties to an LSP feature (diagnostic versioning) but really a lot
of the value is in being able to log what's happening with file versions
and queues more descriptively and clearly.

As such it's fairly invasive, for a logging patch :-\

Key decisions:
 - at the LSP layer, we don't reqire the client to provide versions (LSP
   makes it mandatory but we never enforced it). If not provided,
   versions start at 0 and increment. DraftStore handles this.
 - don't propagate magically using contexts, but rather manually:
   addDocument -> ParseInputs -> (ParsedAST, Preamble, various callbacks)
   Context-propagation would hide the versions from ClangdServer, which
   would make producing good log messages hard
 - within ClangdServer, treat versions as opaque and unordered.
   std::string is a convenient type for this, and allows richer versions
   for embedders. They're "mandatory" but "null" is a reasonable default.

Subscribers: ilya-biryukov, javed.absar, MaskRay, jkorous, arphaman, kadircet, usaxena95, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D75582

Author: sam-mccall

clang-tools-extra/clangd/ClangdLSPServer.cpp

@@ -41,6 +41,21 @@
 namespace clang {
 namespace clangd {
 namespace {
+
+// LSP defines file versions as numbers that increase.
+// ClangdServer treats them as opaque and therefore uses strings instead.
+std::string encodeVersion(int64_t LSPVersion) {
+  return llvm::to_string(LSPVersion);
+}
+llvm::Optional<int64_t> decodeVersion(llvm::StringRef Encoded) {
+  int64_t Result;
+  if (llvm::to_integer(Encoded, Result, 10))
+    return Result;
+  else if (!Encoded.empty()) // Empty can be e.g. diagnostics on close.
+    elog("unexpected non-numeric version {0}", Encoded);
+  return llvm::None;
+}
+
 /// Transforms a tweak into a code action that would apply it if executed.
 /// EXPECTS: T.prepare() was called and returned true.
 CodeAction toCodeAction(const ClangdServer::TweakRef &T, const URIForFile &File,
@@ -630,8 +645,9 @@ void ClangdLSPServer::onDocumentDidOpen(
 
   const std::string &Contents = Params.textDocument.text;
 
-  DraftMgr.addDraft(File, Params.textDocument.version, Contents);
-  Server->addDocument(File, Contents, WantDiagnostics::Yes);
+  auto Version = DraftMgr.addDraft(File, Params.textDocument.version, Contents);
+  Server->addDocument(File, Contents, encodeVersion(Version),
+                      WantDiagnostics::Yes);
 }
 
 void ClangdLSPServer::onDocumentDidChange(
@@ -654,7 +670,8 @@ void ClangdLSPServer::onDocumentDidChange(
     return;
   }
 
-  Server->addDocument(File, Draft->Contents, WantDiags, Params.forceRebuild);
+  Server->addDocument(File, Draft->Contents, encodeVersion(Draft->Version),
+                      WantDiags, Params.forceRebuild);
 }
 
 void ClangdLSPServer::onFileEvent(const DidChangeWatchedFilesParams &Params) {
@@ -1347,7 +1364,8 @@ bool ClangdLSPServer::shouldRunCompletion(
 }
 
 void ClangdLSPServer::onHighlightingsReady(
-    PathRef File, std::vector<HighlightingToken> Highlightings) {
+    PathRef File, llvm::StringRef Version,
+    std::vector<HighlightingToken> Highlightings) {
   std::vector<HighlightingToken> Old;
   std::vector<HighlightingToken> HighlightingsCopy = Highlightings;
   {
@@ -1358,14 +1376,18 @@ void ClangdLSPServer::onHighlightingsReady(
   // LSP allows us to send incremental edits of highlightings. Also need to diff
   // to remove highlightings from tokens that should no longer have them.
   std::vector<LineHighlightings> Diffed = diffHighlightings(Highlightings, Old);
-  publishSemanticHighlighting(
-      {{URIForFile::canonicalize(File, /*TUPath=*/File)},
-       toSemanticHighlightingInformation(Diffed)});
+  SemanticHighlightingParams Notification;
+  Notification.TextDocument.uri =
+      URIForFile::canonicalize(File, /*TUPath=*/File);
+  Notification.TextDocument.version = decodeVersion(Version);
+  Notification.Lines = toSemanticHighlightingInformation(Diffed);
+  publishSemanticHighlighting(Notification);
 }
 
-void ClangdLSPServer::onDiagnosticsReady(PathRef File,
+void ClangdLSPServer::onDiagnosticsReady(PathRef File, llvm::StringRef Version,
                                          std::vector<Diag> Diagnostics) {
   PublishDiagnosticsParams Notification;
+  Notification.version = decodeVersion(Version);
   Notification.uri = URIForFile::canonicalize(File, /*TUPath=*/File);
   DiagnosticToReplacementMap LocalFixIts; // Temporary storage
   for (auto &Diag : Diagnostics) {
@@ -1475,8 +1497,10 @@ void ClangdLSPServer::reparseOpenedFiles(
   // Reparse only opened files that were modified.
   for (const Path &FilePath : DraftMgr.getActiveFiles())
     if (ModifiedFiles.find(FilePath) != ModifiedFiles.end())
-      Server->addDocument(FilePath, DraftMgr.getDraft(FilePath)->Contents,
-                          WantDiagnostics::Auto);
+      if (auto Draft = DraftMgr.getDraft(FilePath)) // else disappeared in race?
+        Server->addDocument(FilePath, std::move(Draft->Contents),
+                            encodeVersion(Draft->Version),
+                            WantDiagnostics::Auto);
 }
 
 } // namespace clangd

clang-tools-extra/clangd/ClangdLSPServer.h

@@ -21,6 +21,7 @@
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringSet.h"
+#include "llvm/Support/JSON.h"
 #include <memory>
 
 namespace clang {
@@ -57,10 +58,11 @@ class ClangdLSPServer : private ClangdServer::Callbacks {
 
 private:
   // Implement ClangdServer::Callbacks.
-  void onDiagnosticsReady(PathRef File, std::vector<Diag> Diagnostics) override;
+  void onDiagnosticsReady(PathRef File, llvm::StringRef Version,
+                          std::vector<Diag> Diagnostics) override;
   void onFileUpdated(PathRef File, const TUStatus &Status) override;
   void
-  onHighlightingsReady(PathRef File,
+  onHighlightingsReady(PathRef File, llvm::StringRef Version,
                        std::vector<HighlightingToken> Highlightings) override;
   void onBackgroundIndexProgress(const BackgroundQueue::Stats &Stats) override;
 

clang-tools-extra/clangd/ClangdServer.cpp

@@ -62,11 +62,11 @@ struct UpdateIndexCallbacks : public ParsingCallbacks {
       : FIndex(FIndex), ServerCallbacks(ServerCallbacks),
         SemanticHighlighting(SemanticHighlighting) {}
 
-  void onPreambleAST(PathRef Path, ASTContext &Ctx,
+  void onPreambleAST(PathRef Path, llvm::StringRef Version, ASTContext &Ctx,
                      std::shared_ptr<clang::Preprocessor> PP,
                      const CanonicalIncludes &CanonIncludes) override {
     if (FIndex)
-      FIndex->updatePreamble(Path, Ctx, std::move(PP), CanonIncludes);
+      FIndex->updatePreamble(Path, Version, Ctx, std::move(PP), CanonIncludes);
   }
 
   void onMainAST(PathRef Path, ParsedAST &AST, PublishFn Publish) override {
@@ -80,16 +80,19 @@ struct UpdateIndexCallbacks : public ParsingCallbacks {
 
     if (ServerCallbacks)
       Publish([&]() {
-        ServerCallbacks->onDiagnosticsReady(Path, std::move(Diagnostics));
+        ServerCallbacks->onDiagnosticsReady(Path, AST.version(),
+                                            std::move(Diagnostics));
         if (SemanticHighlighting)
-          ServerCallbacks->onHighlightingsReady(Path, std::move(Highlightings));
+          ServerCallbacks->onHighlightingsReady(Path, AST.version(),
+                                                std::move(Highlightings));
       });
   }
 
-  void onFailedAST(PathRef Path, std::vector<Diag> Diags,
-                   PublishFn Publish) override {
+  void onFailedAST(PathRef Path, llvm::StringRef Version,
+                   std::vector<Diag> Diags, PublishFn Publish) override {
     if (ServerCallbacks)
-      Publish([&]() { ServerCallbacks->onDiagnosticsReady(Path, Diags); });
+      Publish(
+          [&]() { ServerCallbacks->onDiagnosticsReady(Path, Version, Diags); });
   }
 
   void onFileUpdated(PathRef File, const TUStatus &Status) override {
@@ -169,6 +172,7 @@ ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB,
 }
 
 void ClangdServer::addDocument(PathRef File, llvm::StringRef Contents,
+                               llvm::StringRef Version,
                                WantDiagnostics WantDiags, bool ForceRebuild) {
   auto FS = FSProvider.getFileSystem();
 
@@ -183,6 +187,7 @@ void ClangdServer::addDocument(PathRef File, llvm::StringRef Contents,
   ParseInputs Inputs;
   Inputs.FS = FS;
   Inputs.Contents = std::string(Contents);
+  Inputs.Version = Version.str();
   Inputs.ForceRebuild = ForceRebuild;
   Inputs.Opts = std::move(Opts);
   Inputs.Index = Index;

clang-tools-extra/clangd/ClangdServer.h

@@ -69,7 +69,7 @@ class ClangdServer {
 
     /// Called by ClangdServer when \p Diagnostics for \p File are ready.
     /// May be called concurrently for separate files, not for a single file.
-    virtual void onDiagnosticsReady(PathRef File,
+    virtual void onDiagnosticsReady(PathRef File, llvm::StringRef Version,
                                     std::vector<Diag> Diagnostics) {}
     /// Called whenever the file status is updated.
     /// May be called concurrently for separate files, not for a single file.
@@ -78,7 +78,7 @@ class ClangdServer {
     /// Called by ClangdServer when some \p Highlightings for \p File are ready.
     /// May be called concurrently for separate files, not for a single file.
     virtual void
-    onHighlightingsReady(PathRef File,
+    onHighlightingsReady(PathRef File, llvm::StringRef Version,
                          std::vector<HighlightingToken> Highlightings) {}
 
     /// Called when background indexing tasks are enqueued/started/completed.
@@ -171,13 +171,17 @@ class ClangdServer {
   /// \p File is already tracked. Also schedules parsing of the AST for it on a
   /// separate thread. When the parsing is complete, DiagConsumer passed in
   /// constructor will receive onDiagnosticsReady callback.
+  /// Version identifies this snapshot and is propagated to ASTs, preambles,
+  /// diagnostics etc built from it.
   void addDocument(PathRef File, StringRef Contents,
+                   llvm::StringRef Version = "null",
                    WantDiagnostics WD = WantDiagnostics::Auto,
                    bool ForceRebuild = false);
 
   /// Remove \p File from list of tracked files, schedule a request to free
   /// resources associated with it. Pending diagnostics for closed files may not
   /// be delivered, even if requested with WantDiags::Auto or WantDiags::Yes.
+  /// An empty set of diagnostics will be delivered, with Version = "".
   void removeDocument(PathRef File);
 
   /// Run code completion for \p File at \p Pos.

clang-tools-extra/clangd/Compiler.h

@@ -45,6 +45,8 @@ struct ParseInputs {
   tooling::CompileCommand CompileCommand;
   IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS;
   std::string Contents;
+  // Version identifier for Contents, provided by the client and opaque to us.
+  std::string Version = "null";
   // Prevent reuse of the cached preamble/AST. Slow! Useful to workaround
   // clangd's assumption that missing header files will stay missing.
   bool ForceRebuild = false;

clang-tools-extra/clangd/ParsedAST.cpp

@@ -239,7 +239,8 @@ void dumpAST(ParsedAST &AST, llvm::raw_ostream &OS) {
 }
 
 llvm::Optional<ParsedAST>
-ParsedAST::build(std::unique_ptr<clang::CompilerInvocation> CI,
+ParsedAST::build(llvm::StringRef Version,
+                 std::unique_ptr<clang::CompilerInvocation> CI,
                  llvm::ArrayRef<Diag> CompilerInvocationDiags,
                  std::shared_ptr<const PreambleData> Preamble,
                  std::unique_ptr<llvm::MemoryBuffer> Buffer,
@@ -427,10 +428,10 @@ ParsedAST::build(std::unique_ptr<clang::CompilerInvocation> CI,
     std::vector<Diag> D = ASTDiags.take(CTContext.getPointer());
     Diags.insert(Diags.end(), D.begin(), D.end());
   }
-  return ParsedAST(std::move(Preamble), std::move(Clang), std::move(Action),
-                   std::move(Tokens), std::move(Macros), std::move(ParsedDecls),
-                   std::move(Diags), std::move(Includes),
-                   std::move(CanonIncludes));
+  return ParsedAST(Version, std::move(Preamble), std::move(Clang),
+                   std::move(Action), std::move(Tokens), std::move(Macros),
+                   std::move(ParsedDecls), std::move(Diags),
+                   std::move(Includes), std::move(CanonIncludes));
 }
 
 ParsedAST::ParsedAST(ParsedAST &&Other) = default;
@@ -512,14 +513,15 @@ const CanonicalIncludes &ParsedAST::getCanonicalIncludes() const {
   return CanonIncludes;
 }
 
-ParsedAST::ParsedAST(std::shared_ptr<const PreambleData> Preamble,
+ParsedAST::ParsedAST(llvm::StringRef Version,
+                     std::shared_ptr<const PreambleData> Preamble,
                      std::unique_ptr<CompilerInstance> Clang,
                      std::unique_ptr<FrontendAction> Action,
                      syntax::TokenBuffer Tokens, MainFileMacros Macros,
                      std::vector<Decl *> LocalTopLevelDecls,
                      std::vector<Diag> Diags, IncludeStructure Includes,
                      CanonicalIncludes CanonIncludes)
-    : Preamble(std::move(Preamble)), Clang(std::move(Clang)),
+    : Version(Version), Preamble(std::move(Preamble)), Clang(std::move(Clang)),
       Action(std::move(Action)), Tokens(std::move(Tokens)),
       Macros(std::move(Macros)), Diags(std::move(Diags)),
       LocalTopLevelDecls(std::move(LocalTopLevelDecls)),
@@ -546,7 +548,7 @@ buildAST(PathRef FileName, std::unique_ptr<CompilerInvocation> Invocation,
   }
 
   return ParsedAST::build(
-      std::make_unique<CompilerInvocation>(*Invocation),
+      Inputs.Version, std::make_unique<CompilerInvocation>(*Invocation),
       CompilerInvocationDiags, Preamble,
       llvm::MemoryBuffer::getMemBufferCopy(Inputs.Contents, FileName),
       std::move(VFS), Inputs.Index, Inputs.Opts);

clang-tools-extra/clangd/ParsedAST.h

@@ -48,7 +48,7 @@ class ParsedAST {
   /// Attempts to run Clang and store parsed AST. If \p Preamble is non-null
   /// it is reused during parsing.
   static llvm::Optional<ParsedAST>
-  build(std::unique_ptr<clang::CompilerInvocation> CI,
+  build(llvm::StringRef Version, std::unique_ptr<clang::CompilerInvocation> CI,
         llvm::ArrayRef<Diag> CompilerInvocationDiags,
         std::shared_ptr<const PreambleData> Preamble,
         std::unique_ptr<llvm::MemoryBuffer> Buffer,
@@ -101,14 +101,19 @@ class ParsedAST {
   /// (!) does not have tokens from the preamble.
   const syntax::TokenBuffer &getTokens() const { return Tokens; }
 
+  /// Returns the version of the ParseInputs this AST was built from.
+  llvm::StringRef version() const { return Version; }
+
 private:
-  ParsedAST(std::shared_ptr<const PreambleData> Preamble,
+  ParsedAST(llvm::StringRef Version,
+            std::shared_ptr<const PreambleData> Preamble,
             std::unique_ptr<CompilerInstance> Clang,
             std::unique_ptr<FrontendAction> Action, syntax::TokenBuffer Tokens,
             MainFileMacros Macros, std::vector<Decl *> LocalTopLevelDecls,
             std::vector<Diag> Diags, IncludeStructure Includes,
             CanonicalIncludes CanonIncludes);
 
+  std::string Version;
   // In-memory preambles must outlive the AST, it is important that this member
   // goes before Clang and Action.
   std::shared_ptr<const PreambleData> Preamble;

clang-tools-extra/clangd/Preamble.cpp

@@ -75,12 +75,13 @@ class CppFilePreambleCallbacks : public PreambleCallbacks {
 
 } // namespace
 
-PreambleData::PreambleData(PrecompiledPreamble Preamble,
+PreambleData::PreambleData(llvm::StringRef Version,
+                           PrecompiledPreamble Preamble,
                            std::vector<Diag> Diags, IncludeStructure Includes,
                            MainFileMacros Macros,
                            std::unique_ptr<PreambleFileStatusCache> StatCache,
                            CanonicalIncludes CanonIncludes)
-    : Preamble(std::move(Preamble)), Diags(std::move(Diags)),
+    : Version(Version), Preamble(std::move(Preamble)), Diags(std::move(Diags)),
       Includes(std::move(Includes)), Macros(std::move(Macros)),
       StatCache(std::move(StatCache)), CanonIncludes(std::move(CanonIncludes)) {
 }
@@ -102,12 +103,17 @@ buildPreamble(PathRef FileName, CompilerInvocation &CI,
       compileCommandsAreEqual(Inputs.CompileCommand, OldCompileCommand) &&
       OldPreamble->Preamble.CanReuse(CI, ContentsBuffer.get(), Bounds,
                                      Inputs.FS.get())) {
-    vlog("Reusing preamble for {0}", FileName);
+    vlog("Reusing preamble version {0} for version {1} of {2}",
+         OldPreamble->Version, Inputs.Version, FileName);
     return OldPreamble;
   }
-  vlog(OldPreamble ? "Rebuilding invalidated preamble for {0}"
-                   : "Building first preamble for {0}",
-       FileName);
+  if (OldPreamble)
+    vlog("Rebuilding invalidated preamble for {0} version {1} "
+         "(previous was version {2})",
+         FileName, Inputs.Version, OldPreamble->Version);
+  else
+    vlog("Building first preamble for {0} verson {1}", FileName,
+         Inputs.Version);
 
   trace::Span Tracer("BuildPreamble");
   SPAN_ATTACH(Tracer, "File", FileName);
@@ -145,16 +151,17 @@ buildPreamble(PathRef FileName, CompilerInvocation &CI,
   CI.getFrontendOpts().SkipFunctionBodies = false;
 
   if (BuiltPreamble) {
-    vlog("Built preamble of size {0} for file {1}", BuiltPreamble->getSize(),
-         FileName);
+    vlog("Built preamble of size {0} for file {1} version {2}",
+         BuiltPreamble->getSize(), FileName, Inputs.Version);
     std::vector<Diag> Diags = PreambleDiagnostics.take();
     return std::make_shared<PreambleData>(
-        std::move(*BuiltPreamble), std::move(Diags),
+        Inputs.Version, std::move(*BuiltPreamble), std::move(Diags),
         SerializedDeclsCollector.takeIncludes(),
         SerializedDeclsCollector.takeMacros(), std::move(StatCache),
         SerializedDeclsCollector.takeCanonicalIncludes());
   } else {
-    elog("Could not build a preamble for file {0}", FileName);
+    elog("Could not build a preamble for file {0} version {2}", FileName,
+         Inputs.Version);
     return nullptr;
   }
 }

clang-tools-extra/clangd/Preamble.h

@@ -43,11 +43,14 @@ namespace clangd {
 /// As we must avoid re-parsing the preamble, any information that can only
 /// be obtained during parsing must be eagerly captured and stored here.
 struct PreambleData {
-  PreambleData(PrecompiledPreamble Preamble, std::vector<Diag> Diags,
-               IncludeStructure Includes, MainFileMacros Macros,
+  PreambleData(llvm::StringRef Version, PrecompiledPreamble Preamble,
+               std::vector<Diag> Diags, IncludeStructure Includes,
+               MainFileMacros Macros,
                std::unique_ptr<PreambleFileStatusCache> StatCache,
                CanonicalIncludes CanonIncludes);
 
+  // Version of the ParseInputs this preamble was built from.
+  std::string Version;
   tooling::CompileCommand CompileCommand;
   PrecompiledPreamble Preamble;
   std::vector<Diag> Diags;

clang-tools-extra/clangd/Protocol.cpp

@@ -92,8 +92,7 @@ bool fromJSON(const llvm::json::Value &Params, TextDocumentIdentifier &R) {
 
 llvm::json::Value toJSON(const VersionedTextDocumentIdentifier &R) {
   auto Result = toJSON(static_cast<const TextDocumentIdentifier &>(R));
-  if (R.version)
-    Result.getAsObject()->try_emplace("version", R.version);
+  Result.getAsObject()->try_emplace("version", R.version);
   return Result;
 }
 
@@ -547,8 +546,9 @@ bool fromJSON(const llvm::json::Value &Params, Diagnostic &R) {
 
 llvm::json::Value toJSON(const PublishDiagnosticsParams &PDP) {
   return llvm::json::Object{
-    {"uri", PDP.uri},
-    {"diagnostics", PDP.diagnostics},
+      {"uri", PDP.uri},
+      {"diagnostics", PDP.diagnostics},
+      {"version", PDP.version},
   };
 }