[clangd] Introduce Modules

[kadircet]

Summary

Modules can be used to augment clangd's behaviours in various features.

[kadircet]

clang-tools-extra/clangd/Plugin.h

26 ↗

(On Diff #322034)

i think it would be better to have these as separate modules. for example a

class DiagnosticsModule {
public:
virtual llvm::Optional<CodeAction>
  actOnDiagnostic(DiagnosticsEngine::Level L, const clang::Diagnostic &Diag) = 0;
};
class HoverModule {
public:
virtual llvm::Optional<HoverInfo> generateHover(ParsedAST, Selection, SymbolIndex) = 0;
};
class LSPModule {
public:
virtual void registerHandler(EndPoint, Functor);
virtual json::Object dispatch(EndPoint, Params, ClangdServer&); // as some of these might need to mutate ClangdServer or access other modules.
};
// and so on.

and have multiple registries for each of these modules.

Then have a PluginManager in ClangdServer that provides a unified API for accessing these modules. (e.g. to implement each of our features in terms of these modules as discussed offline).

but I am not sure if it'll be worth the complexity with only diagnostics making use of that mechanism in the foreseeable future. So I am leaning towards keeping it simple in this version, but open for suggestions.

[kadircet]

• Rename Plugin to Module
• Have a ModuleSet that enables type-safe traversal of existing modules (with questions about implementation options)

[kadircet]

• Changes to propagate moduleset into clangd(lsp)server

[sammccall]

I'm a little bit nervous about adding this with *no* usage, but it keeps the patch size down :-)

clang-tools-extra/clangd/Module.cpp

1 ↗

(On Diff #322754)

please add if/when it's actually needed

clang-tools-extra/clangd/Module.h

14

This isn't implemented - i'm not sure if i should be reviewing the comment or the impl :-)

we'll need it at some point (if we're going to move features which have public APIs in clangdserver into modules) but not in this patch

16

nit: not just updating but also reading client caps

25

need some guarantee that module *usage* ends before clangdserver is destroyed, for some sensible definition of usage

26

this doesn't make sense to me - neither of these own the modules (right?), so who would be destroying them while the server is still alive (and how would we ensure this is safe)?

37

what is this for? is it needed?

40

Some indication that this is for testing.

41

this isn't called - call it or leave it out?

44

As discussed offline, I don't think sub-interfaces are a great way to express the various ways to express clangd, at least initially.
You end up either having to deal with multiple inheritance or multiple module instances.
The former is a hassle we only need consider if the interface gets wide, the latter loses some of the simplicity of the model (if a feature contains 3 hooks that share state, where does that state go?)

For now, I think it's enough to be able to iterate over Module&s (Given the ModuleSet name, I think defining begin()/end() etc make sense)

54

this interface isn't compatible with modules eventually having public interfaces, because it loses the type information. but we can change this later

[kadircet]

• Define destruction order
• Get rid of Module.cpp and Module::id
• Define begin/end iterators for ModuleSet

clang-tools-extra/clangd/Module.h

26

I was leaving this out from initial patch because current module interface didn't have any dependence on server facilities (hence the fixme below). Happy to declare the semantics now though, all your points are right.

37

no more. i was planning to make use of it to dispatch command executions to relevant module, but as we discussed offline when modules register those themselves it won't be needed.

40

this is needed to block clangdserver shutdown until modules finish any background tasks. would it make sense to rename it to stop instead?

44

SG. I've copied the iterators from your patch :)

54

agreed.

[sammccall]

clang-tools-extra/clangd/ClangdServer.h

33

(include no longer used?)

clang-tools-extra/clangd/Module.h

20

not at creation, but rather at initialize time

21

maybe this is a separate fixme (basically ClangdLSPServer vs Server)

23

after what? at this point?

32

Should either:

• doc that destructor should cancel as much background work as possible and block until it's done
• add a requestStop() and doc that destructor should block
• remove the concept of background work completely (i.e. blockUntilIdle())

[kadircet]

• Address comments

[kadircet]

clang-tools-extra/clangd/ClangdServer.h

33

well it is still used by formatCode :P (but dropping to keep the irrelevant changes out, same for memory and vector)

clang-tools-extra/clangd/Module.h

32

as discussed offline going with remove the concept of background work completely (i.e. blockUntilIdle()). we can go with requestStop and blocking destructors, once the need arises. (i.e. we better understand the usecases)

[RKSimon]

In D96244#2558345, @sammccall wrote:

I'm a little bit nervous about adding this with *no* usage, but it keeps the patch size down :-)

Indeed - this has broken at least one buildbot (http://lab.llvm.org:8011/#/builders/57) due to -Werror,-Wunused-private-field

[kadircet]

oops, d25fbaa4a4a1284e998f545d45280e976c29cc85 should fix the issue.

[dblaikie]

"modules" is a pretty overloaded term in Clang (between llvm Modules and Clang Modules and now C++ standard Modules) - any chance of some other term that might be less overloaded in this space?

[sammccall]

In D96244#2566225, @dblaikie wrote:

"modules" is a pretty overloaded term in Clang (between llvm Modules and Clang Modules and now C++ standard Modules) - any chance of some other term that might be less overloaded in this space?

You're right, and I'm not sure why we didn't bikeshed this name harder :-\
(FWIW Module is also a nod to Guice and its terrible DSLs for binding things things, which we borrow a bit here...)

ideas...
Component isn't terrible: I don't like it quite as much as i'm not sure it suggests the uniformity-of-interface as well.
Feature is what VSCode calls this on the client side. Personally I find this terribly confusing.
Plugin I don't think is great, because there's no dynamic loading or injection going on (clang plugins are well-named, but a different concept)
@kadircet any thoughts on these or more?

clang-tools-extra/clangd/ClangdServer.h

 //===--- ClangdServer.h - Main clangd server code ----------------*- 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_CLANGDSERVER_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CLANGDSERVER_H
 
 #include "../clang-tidy/ClangTidyOptions.h"
 #include "CodeComplete.h"
 #include "ConfigProvider.h"
 #include "GlobalCompilationDatabase.h"
 #include "Hover.h"
+#include "Module.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/Rename.h"
 #include "refactor/Tweak.h"
 #include "support/Cancellation.h"
 #include "support/Function.h"
 #include "support/MemoryTree.h"
 #include "support/ThreadsafeFS.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/FunctionExtras.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include <functional>
 #include <string>
 #include <type_traits>
 #include <utility>
 
struct Options {
 
     /// Clangd will execute compiler drivers matching one of these globs to
     /// fetch system include path.
     std::vector<std::string> QueryDriverGlobs;
 
     /// Enable preview of FoldingRanges feature.
     bool FoldingRanges = false;
 
+    ModuleSet *Modules = nullptr;
+
     explicit operator TUScheduler::Options() const;
   };
   // Sensible default options for use in tests.
   // Features like indexing must be enabled if desired.
   static Options optsForTest();
 
   /// Creates a new ClangdServer instance.
   ///

 
 private:
   void formatCode(PathRef File, llvm::StringRef Code,
                   ArrayRef<tooling::Range> Ranges,
                   Callback<tooling::Replacements> CB);
 
   const GlobalCompilationDatabase &CDB;
   const ThreadsafeFS &TFS;
+  ModuleSet *Modules = nullptr;
 
   Path ResourceDir;
   // The index used to look up symbols. This could be:
   //   - null (all index functionality is optional)
   //   - the dynamic index owned by ClangdServer (DynamicIdx)
   //   - the static index passed to the constructor
   //   - a merged view of a static and dynamic index (MergedIndex)
   const SymbolIndex *Index = nullptr;

clang-tools-extra/clangd/ClangdServer.cpp

ClangdServer::Options::operator TUScheduler::Options() const {
   Opts.UpdateDebounce = UpdateDebounce;
   Opts.ContextProvider = ContextProvider;
   return Opts;
 }
 
 ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB,
                            const ThreadsafeFS &TFS, const Options &Opts,
                            Callbacks *Callbacks)
-    : CDB(CDB), TFS(TFS),
+    : CDB(CDB), TFS(TFS), Modules(Opts.Modules),
       DynamicIdx(Opts.BuildDynamicSymbolIndex ? new FileIndex() : nullptr),
       ClangTidyProvider(Opts.ClangTidyProvider),
       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.

clang-tools-extra/clangd/Headers.cpp

public:
 
   // Record existing #includes - both written and resolved paths. Only #includes
   // in the main file are collected.
   void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
                           llvm::StringRef FileName, bool IsAngled,
                           CharSourceRange /*FilenameRange*/,
                           const FileEntry *File, llvm::StringRef /*SearchPath*/,
                           llvm::StringRef /*RelativePath*/,
-                          const Module * /*Imported*/,
+                          const clang::Module * /*Imported*/,
                           SrcMgr::CharacteristicKind FileKind) override {
     auto MainFID = SM.getMainFileID();
     // If an include is part of the preamble patch, translate #line directives.
     if (InBuiltinFile)
       HashLoc = translatePreamblePatchLocation(HashLoc, SM);
 
     // Record main-file inclusions (including those mapped from the preamble
     // patch).

clang-tools-extra/clangd/Module.h

+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_MODULE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_MODULE_H
+
+#include "llvm/ADT/StringRef.h"
+#include <memory>
+#include <vector>
+
+namespace clang {
+namespace clangd {
+
+/// A Module contributes a vertical feature to clangd.
+///
+/// FIXME: Extend this with LSP bindings to support reading/updating
+/// capabilities and implementing LSP endpoints.
+///
+/// The lifetime of a module is roughly:
+///  - modules are created before the LSP server, in ClangdMain.cpp
+///  - these modules are then passed to ClangdLSPServer and ClangdServer
+///    FIXME: LSP bindings should be registered at ClangdLSPServer
+///    initialization.
+///  - module hooks can be called at this point.
+///    FIXME: We should make some server facilities like TUScheduler and index
+///    available to those modules after ClangdServer is initalized.
+///  - ClangdServer will not be destroyed until all the requests are done.
+///    FIXME: Block server shutdown until all the modules are idle.
+///  - modules will be destroyed after ClangdLSPServer is destroyed.
+///
+/// Conventionally, standard modules live in the `clangd` namespace, and other
+/// exposed details live in a sub-namespace.
+class Module {
+public:
+  virtual ~Module() = default;
+};
+
+class ModuleSet {
+  std::vector<std::unique_ptr<Module>> Modules;
+
+public:
+  explicit ModuleSet(std::vector<std::unique_ptr<Module>> Modules)
+      : Modules(std::move(Modules)) {}
+
+  using iterator = llvm::pointee_iterator<decltype(Modules)::iterator>;
+  using const_iterator =
+      llvm::pointee_iterator<decltype(Modules)::const_iterator>;
+  iterator begin() { return iterator(Modules.begin()); }
+  iterator end() { return iterator(Modules.end()); }
+  const_iterator begin() const { return const_iterator(Modules.begin()); }
+  const_iterator end() const { return const_iterator(Modules.end()); }
+};
+} // namespace clangd
+} // namespace clang
+#endif

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

TEST(ParsedASTTest, ReplayPreambleForTidyCheckers) {
   static std::vector<syntax::Token> SkippedFiles;
   struct ReplayPreamblePPCallback : public PPCallbacks {
     const SourceManager &SM;
     explicit ReplayPreamblePPCallback(const SourceManager &SM) : SM(SM) {}
 
     void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
                             StringRef FileName, bool IsAngled,
                             CharSourceRange FilenameRange, const FileEntry *,
-                            StringRef, StringRef, const Module *,
+                            StringRef, StringRef, const clang::Module *,
                             SrcMgr::CharacteristicKind) override {
       Includes.emplace_back(SM, HashLoc, IncludeTok, FileName, IsAngled,
                             FilenameRange);
     }
 
     void FileSkipped(const FileEntryRef &, const Token &FilenameTok,
                      SrcMgr::CharacteristicKind) override {
       SkippedFiles.emplace_back(FilenameTok);