[clangd] Added C++ API code for semantic highlighting

[ilya-biryukov]

clang-tools-extra/clangd/TUScheduler.h

157 ↗

(On Diff #206661)

Can we avoid updating TUScheduler, ParsingCallbacks and compute the highlights inside onMainAST?

[hokein]

clang-tools-extra/clangd/ClangdServer.h

65

we may add this interface to the existing DiagnosticsConsumer.

clang-tools-extra/clangd/TUScheduler.h

157 ↗

(On Diff #206661)

+1.

[jvikstrom]

Moved semantic highlighting to be processed in onMainAST

[jvikstrom]

clang-tools-extra/clangd/ClangdServer.h

65

Probably want to rename DiagnosticsConsumer as well, can't come up with a good name though. Any suggestions?

[nik]

clang-tools-extra/clangd/ClangdServer.h

65

One could summarize diagnostics and highlightings as annotations, so maybe FileAnnotationsConsumer or DocumentAnnotationsConsumer? Not sure how onFileUpdated() fits into this...

[hokein]

thanks, it is much clearer now.

Could you please make the commit message be more specific what this patch does? C++ APIs is too generous, (we already have C++ APIs and data structures for semantic highlightings which are in SemanticHighlighting.h).

clang-tools-extra/clangd/ClangdLSPServer.h

60

clang-format: intention is not correct.

clang-tools-extra/clangd/ClangdServer.h

54

nit: triple / here.

56

just provide an empty implementation (like onFileUpdated), and you don't need to update all the subclasses.

65

There is a FIXME on Line 43. I'd keep it unchanged now (I don't have a good name either).

141

just: Enable semantic highlighting features, and name it bool SemanticHighlighting.

160

not sure whether we need this comment, because the intention is not obvious in code here (the flag is hidden in Opts), it might be more sensible to move it to Options::SemanticHighlighting.

And the comment is not precise in the new code now (we collect the highlighting tokens in the DiagConsumer now).

317

We don't need to store it as a class member, we only pass this value to ParsingCallBack in the constructor.

clang-tools-extra/clangd/SemanticHighlighting.h

35 ↗

(On Diff #206792)

I think it is unrelated to this patch, we should include this when we implements the LSP part.

clang-tools-extra/clangd/unittests/ClangdTests.cpp

898

We should keep the test isolated, this test is for diagnostics, please add a new TEST_F for merely testing the highlightings.

[jvikstrom]

Separated test and gave consumer an empty definition.

[hokein]

clang-tools-extra/clangd/ClangdLSPServer.h

60

nit: you can remove this override, since we have provided an empty default implementation.

clang-tools-extra/clangd/unittests/ClangdTests.cpp

881

Looks like the current test could be simplified, I think we want to test that when the AST is build/rebuild, we will emit the highlighting tokens.
Just use Count and check the count is to 1?

Server.addDocument(FooCpp, "int a;");
ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for server";
ASSERT_EQ(DiagConsumer.Count, 1);

[jvikstrom]

clang-tools-extra/clangd/ClangdLSPServer.h

60

I get an -Winconsistent-missing-override warnings without the override.

[jvikstrom]

Simplified test.

[hokein]

clang-tools-extra/clangd/ClangdLSPServer.h

60

sorry for the confusion, I meant we remove onHighlightingsReady, not the override keyword.

clang-tools-extra/clangd/unittests/ClangdTests.cpp

863

this is not safe, we need to wait until the server finishes building AST.

add ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for server";

[jvikstrom]

Made test safe again

[hokein]

Let's rephrase the commit message, I think this patch is just to "emit the semantic highlighting tokens when the main AST is built".

clang-tools-extra/clangd/unittests/ClangdTests.cpp

844

maybe move this test to SemanticHighlightingTests.

clang-tools-extra/clangd/ClangdLSPServer.h

 
 #include "ClangdServer.h"
 #include "DraftStore.h"
 #include "Features.inc"
 #include "FindSymbols.h"
 #include "GlobalCompilationDatabase.h"
 #include "Path.h"
 #include "Protocol.h"
+#include "SemanticHighlighting.h"
 #include "Transport.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/Optional.h"
 #include <memory>
 
 namespace clang {
 namespace clangd {
 
public:
   ///
   /// \return Whether we shut down cleanly with a 'shutdown' -> 'exit' sequence.
   bool run();
 
 private:
   // Implement DiagnosticsConsumer.
   void onDiagnosticsReady(PathRef File, std::vector<Diag> Diagnostics) override;
   void onFileUpdated(PathRef File, const TUStatus &Status) override;
+  void
+  onHighlightingsReady(PathRef File,
+                       std::vector<HighlightingToken> Highlightings) override;
 
   // LSP methods. Notifications have signature void(const Params&).
   // Calls have signature void(const Params&, Callback<Response>).
   void onInitialize(const InitializeParams &, Callback<llvm::json::Value>);
   void onShutdown(const ShutdownParams &, Callback<std::nullptr_t>);
   void onSync(const NoParams &, Callback<std::nullptr_t>);
   void onDocumentDidOpen(const DidOpenTextDocumentParams &);
   void onDocumentDidChange(const DidChangeTextDocumentParams &);

clang-tools-extra/clangd/ClangdLSPServer.cpp

bool ClangdLSPServer::shouldRunCompletion(
   if (Trigger == ">")
     return (*Code)[*Offset - 2] == '-'; // trigger only on '->'.
   if (Trigger == ":")
     return (*Code)[*Offset - 2] == ':'; // trigger only on '::'.
   assert(false && "unhandled trigger character");
   return true;
 }
 
+void ClangdLSPServer::onHighlightingsReady(PathRef File,
+                                        std::vector<HighlightingToken> Highlightings) {
+
+}
+
 void ClangdLSPServer::onDiagnosticsReady(PathRef File,
                                          std::vector<Diag> Diagnostics) {
   auto URI = URIForFile::canonicalize(File, /*TUPath=*/File);
   std::vector<Diagnostic> LSPDiagnostics;
   DiagnosticToReplacementMap LocalFixIts; // Temporary storage
   for (auto &Diag : Diagnostics) {
     toLSPDiags(Diag, URI, DiagOpts,
                [&](clangd::Diagnostic Diag, llvm::ArrayRef<Fix> Fixes) {

clang-tools-extra/clangd/ClangdServer.h

 #include "Cancellation.h"
 #include "ClangdUnit.h"
 #include "CodeComplete.h"
 #include "FSProvider.h"
 #include "FormattedString.h"
 #include "Function.h"
 #include "GlobalCompilationDatabase.h"
 #include "Protocol.h"
+#include "SemanticHighlighting.h"
 #include "TUScheduler.h"
 #include "XRefs.h"
 #include "index/Background.h"
 #include "index/FileIndex.h"
 #include "index/Index.h"
 #include "refactor/Tweak.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "clang/Tooling/Core/Replacement.h"

 public:
   virtual ~DiagnosticsConsumer() = default;
 
   /// Called by ClangdServer when \p Diagnostics for \p File are ready.
   virtual void onDiagnosticsReady(PathRef File,
                                   std::vector<Diag> Diagnostics) = 0;
   /// Called whenever the file status is updated.
   virtual void onFileUpdated(PathRef File, const TUStatus &Status){};
+
+  /// Called by ClangdServer when some \p Highlightings for \p File are ready.
+  virtual void
+  onHighlightingsReady(PathRef File,
+                       std::vector<HighlightingToken> Highlightings) {}
 };
 
 /// When set, used by ClangdServer to get clang-tidy options for each particular
 /// file. Must be thread-safe. We use this instead of ClangTidyOptionsProvider
 /// to allow reading tidy configs from the VFS used for parsing.
 using ClangTidyOptionsBuilder = std::function<tidy::ClangTidyOptions(
     llvm::vfs::FileSystem &, llvm::StringRef /*File*/)>;
 
 /// Manages a collection of source files and derived data (ASTs, indexes),
 /// and provides language-aware features such as code completion.
 ///
 /// The primary client is ClangdLSPServer which exposes these features via
 /// the Language Server protocol. ClangdServer may also be embedded directly,
 /// though its API is not stable over time.
 ///
 /// ClangdServer should be used from a single thread. Many potentially-slow
struct Options {
 
     /// Enable hidden features mostly useful to clangd developers.
     /// e.g. tweaks to dump the AST.
     bool HiddenFeatures = false;
 
     /// Clangd will execute compiler drivers matching one of these globs to
     /// fetch system include path.
     std::vector<std::string> QueryDriverGlobs;
+
+    /// Enable semantic highlighting features.
+    bool SemanticHighlighting = false;
   };
   // Sensible default options for use in tests.
   // Features like indexing must be enabled if desired.
   static Options optsForTest();
 
   /// Creates a new ClangdServer instance.
   ///
   /// ClangdServer uses \p CDB to obtain compilation arguments for parsing. Note
   /// that ClangdServer only obtains compilation arguments once for each newly
   /// added file (i.e., when processing a first call to addDocument) and reuses
   /// those arguments for subsequent reparses. However, ClangdServer will check
   /// if compilation arguments changed on calls to forceReparse().
   ///
   /// After each parsing request finishes, ClangdServer reports diagnostics to
   /// \p DiagConsumer. Note that a callback to \p DiagConsumer happens on a
   /// worker thread. Therefore, instances of \p DiagConsumer must properly
   /// synchronize access to shared state.
   ClangdServer(const GlobalCompilationDatabase &CDB,
                const FileSystemProvider &FSProvider,
                DiagnosticsConsumer &DiagConsumer, const Options &Opts);
 
   /// Add a \p File to the list of tracked C++ files or update the contents if
   /// \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.
   void addDocument(PathRef File, StringRef Contents,
private:
 
   // When set, provides clang-tidy options for a specific file.
   ClangTidyOptionsBuilder GetClangTidyOptions;
 
   // If this is true, suggest include insertion fixes for diagnostic errors that
   // can be caused by missing includes (e.g. member access in incomplete type).
   bool SuggestMissingIncludes = false;
   bool EnableHiddenFeatures = false;
   
   // GUARDED_BY(CachedCompletionFuzzyFindRequestMutex)
   llvm::StringMap<llvm::Optional<FuzzyFindRequest>>
       CachedCompletionFuzzyFindRequestByFile;
   mutable std::mutex CachedCompletionFuzzyFindRequestMutex;
 
   llvm::Optional<std::string> WorkspaceRoot;
   // WorkScheduler has to be the last member, because its destructor has to be
   // called before all other members to stop the worker thread that references
   // ClangdServer.
   TUScheduler WorkScheduler;
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/clangd/ClangdServer.cpp

 #include <mutex>
 
 namespace clang {
 namespace clangd {
 namespace {
 
 // Update the FileIndex with new ASTs and plumb the diagnostics responses.
 struct UpdateIndexCallbacks : public ParsingCallbacks {
-  UpdateIndexCallbacks(FileIndex *FIndex, DiagnosticsConsumer &DiagConsumer)
-      : FIndex(FIndex), DiagConsumer(DiagConsumer) {}
+  UpdateIndexCallbacks(FileIndex *FIndex, DiagnosticsConsumer &DiagConsumer,
+                       bool SemanticHighlighting)
+      : FIndex(FIndex), DiagConsumer(DiagConsumer),
+        SemanticHighlighting(SemanticHighlighting) {}
 
   void onPreambleAST(PathRef Path, ASTContext &Ctx,
                      std::shared_ptr<clang::Preprocessor> PP,
                      const CanonicalIncludes &CanonIncludes) override {
     if (FIndex)
       FIndex->updatePreamble(Path, Ctx, std::move(PP), CanonIncludes);
   }
 
   void onMainAST(PathRef Path, ParsedAST &AST) override {
     if (FIndex)
       FIndex->updateMain(Path, AST);
+    if (SemanticHighlighting)
+      DiagConsumer.onHighlightingsReady(Path, getSemanticHighlightings(AST));
   }
 
   void onDiagnostics(PathRef File, std::vector<Diag> Diags) override {
     DiagConsumer.onDiagnosticsReady(File, std::move(Diags));
   }
 
   void onFileUpdated(PathRef File, const TUStatus &Status) override {
     DiagConsumer.onFileUpdated(File, Status);
   }
 
 private:
   FileIndex *FIndex;
   DiagnosticsConsumer &DiagConsumer;
+  bool SemanticHighlighting;
 };
 } // namespace
 
 ClangdServer::Options ClangdServer::optsForTest() {
   ClangdServer::Options Opts;
   Opts.UpdateDebounce = std::chrono::steady_clock::duration::zero(); // Faster!
   Opts.StorePreamblesInMemory = true;
   Opts.AsyncThreadsCount = 4; // Consistent!
+  Opts.SemanticHighlighting = true;
   return Opts;
 }
 
 ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB,
                            const FileSystemProvider &FSProvider,
                            DiagnosticsConsumer &DiagConsumer,
                            const Options &Opts)
     : FSProvider(FSProvider),
       DynamicIdx(Opts.BuildDynamicSymbolIndex
                      ? new FileIndex(Opts.HeavyweightDynamicSymbolIndex)
                      : nullptr),
       GetClangTidyOptions(Opts.GetClangTidyOptions),
       SuggestMissingIncludes(Opts.SuggestMissingIncludes),
       EnableHiddenFeatures(Opts.HiddenFeatures),
       WorkspaceRoot(Opts.WorkspaceRoot),
       // 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(CDB, Opts.AsyncThreadsCount, Opts.StorePreamblesInMemory,
-                    llvm::make_unique<UpdateIndexCallbacks>(DynamicIdx.get(),
-                                                            DiagConsumer),
-                    Opts.UpdateDebounce, Opts.RetentionPolicy) {
+      WorkScheduler(
+          CDB, Opts.AsyncThreadsCount, Opts.StorePreamblesInMemory,
+          llvm::make_unique<UpdateIndexCallbacks>(
+              DynamicIdx.get(), DiagConsumer, Opts.SemanticHighlighting),
   // Adds an index to the stack, at higher priority than existing indexes.
   auto AddIndex = [&](SymbolIndex *Idx) {
     if (this->Index != nullptr) {
       MergedIdx.push_back(llvm::make_unique<MergedIndex>(Idx, this->Index));
       this->Index = MergedIdx.back().get();
     } else {
       this->Index = Idx;
     }

clang-tools-extra/clangd/unittests/ClangdTests.cpp

TEST_F(ClangdVFSTest, CheckSourceHeaderSwitch) {
   ASSERT_EQ(PathResult.getValue(), Foo3HXX);
 
   // Test if asking for a corresponding file that doesn't exist returns an empty
   // string.
   PathResult = Server.switchSourceHeader(Invalid);
   EXPECT_FALSE(PathResult.hasValue());
 }
 
+TEST(ClangdSemanticHighlightingTest, GeneratesHighlightsWhenFileChange) {
+  class HighlightingsCounterDiagConsumer : public DiagnosticsConsumer {
+  public:
+    std::atomic<int> Count = {0};
+
+    HighlightingsCounterDiagConsumer(std::promise<void> StartSecondReparse)
+        : StartSecondReparse(std::move(StartSecondReparse)) {}
+
+    void onDiagnosticsReady(PathRef, std::vector<Diag>) override {}
+
+    virtual void onHighlightingsReady(
+        PathRef File, std::vector<HighlightingToken> Highlightings) override {
+      ++Count;
+      if (FirstRequest) {
+        FirstRequest = false;
+        StartSecondReparse.set_value();
+      }
+    }
+
+  private:
+    std::mutex Mutex;
+    bool FirstRequest = true;
+    std::promise<void> StartSecondReparse;
+  };
+
+  const auto SourceContents1 = R"cpp(
+int a;
+)cpp";
+
+  const auto SourceContents2 = R"cpp(
+int a = ;
+)cpp";
+
+  auto FooCpp = testPath("foo.cpp");
+  MockFSProvider FS;
+  FS.Files[FooCpp] = "";
+
+  std::promise<void> StartSecondPromise;
+  std::future<void> StartSecond = StartSecondPromise.get_future();
+  MockCompilationDatabase MCD;
+  HighlightingsCounterDiagConsumer DiagConsumer(std::move(StartSecondPromise));
+  ClangdServer Server(MCD, FS, DiagConsumer,
+                      ClangdServer::optsForTest());
+  Server.addDocument(FooCpp, SourceContents1);
+  StartSecond.wait();
+  Server.addDocument(FooCpp, SourceContents2);
+  ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for server";
+  ASSERT_EQ(DiagConsumer.Count, 2);
+}
+
 TEST_F(ClangdThreadingTest, NoConcurrentDiagnostics) {
   class NoConcurrentAccessDiagConsumer : public DiagnosticsConsumer {
   public:
     std::atomic<int> Count = {0};
 
     NoConcurrentAccessDiagConsumer(std::promise<void> StartSecondReparse)
         : StartSecondReparse(std::move(StartSecondReparse)) {}
 
     void onDiagnosticsReady(PathRef, std::vector<Diag>) override {
       ++Count;
       std::unique_lock<std::mutex> Lock(Mutex, std::try_to_lock_t());
       ASSERT_TRUE(Lock.owns_lock())
           << "Detected concurrent onDiagnosticsReady calls for the same file.";