[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

60 ↗

(On Diff #206661)

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

60 ↗

(On Diff #206661)

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

60 ↗

(On Diff #206661)

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 ↗

(On Diff #206792)

clang-format: intention is not correct.

clang-tools-extra/clangd/ClangdServer.h

54 ↗

(On Diff #206792)

nit: triple / here.

56 ↗

(On Diff #206792)

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

140 ↗

(On Diff #206792)

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

160 ↗

(On Diff #206792)

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).

320 ↗

(On Diff #206792)

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

60 ↗

(On Diff #206661)

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

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

863 ↗

(On Diff #206792)

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 ↗

(On Diff #206832)

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

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

881 ↗

(On Diff #206832)

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 ↗

(On Diff #206832)

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

[jvikstrom]

Simplified test.

[hokein]

clang-tools-extra/clangd/ClangdLSPServer.h

60 ↗

(On Diff #206832)

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

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

864 ↗

(On Diff #206841)

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 ↗

(On Diff #206854)

maybe move this test to SemanticHighlightingTests.

clang-tools-extra/trunk/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*/)>;
 
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
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/trunk/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/trunk/clangd/unittests/SemanticHighlightingTests.cpp

 //==- SemanticHighlightingTests.cpp - SemanticHighlighting tests-*- C++ -* -==//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Annotations.h"
+#include "ClangdServer.h"
 #include "SemanticHighlighting.h"
+#include "TestFS.h"
 #include "TestTU.h"
 #include "gmock/gmock.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
 std::vector<HighlightingToken>
)cpp",
       R"cpp(
     void $Function[[foo]](int);
   )cpp"};
   for (const auto &TestCase : TestCases) {
     checkHighlightings(TestCase);
   }
 }
 
+TEST(ClangdSemanticHighlightingTest, GeneratesHighlightsWhenFileChange) {
+  class HighlightingsCounterDiagConsumer : public DiagnosticsConsumer {
+  public:
+    std::atomic<int> Count = {0};
+
+    void onDiagnosticsReady(PathRef, std::vector<Diag>) override {}
+    void onHighlightingsReady(
+        PathRef File, std::vector<HighlightingToken> Highlightings) override {
+      ++Count;
+    }
+  };
+
+  auto FooCpp = testPath("foo.cpp");
+  MockFSProvider FS;
+  FS.Files[FooCpp] = "";
+
+  MockCompilationDatabase MCD;
+  HighlightingsCounterDiagConsumer DiagConsumer;
+  ClangdServer Server(MCD, FS, DiagConsumer, ClangdServer::optsForTest());
+  Server.addDocument(FooCpp, "int a;");
+  ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for server";
+  ASSERT_EQ(DiagConsumer.Count, 1);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang