[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

2

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 ↗

(On Diff #323290)

(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 ↗

(On Diff #323290)

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/CMakeLists.txt

add_clang_library(clangDaemon
   FS.cpp
   FuzzyMatch.cpp
   GlobalCompilationDatabase.cpp
   Headers.cpp
   HeaderSourceSwitch.cpp
   Hover.cpp
   IncludeFixer.cpp
   JSONTransport.cpp
+  Module.cpp
   PathMapping.cpp
   Protocol.cpp
   Quality.cpp
   ParsedAST.cpp
   Preamble.cpp
   RIFF.cpp
   Selection.cpp
   SemanticHighlighting.cpp

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. Currently this consists
+/// of only a public interface to access the functionality from C++ embedders,
+///    ClangdServer::getModule<FooModule>()->foo(...)
+///
+/// FIXME: Extend this with LSP bindings to support 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 creation, and
+///    we should make some server facilities like TUScheduler and index
+///    available to those modules after ClangdServer is initalized.
+///  - module hooks can be called afterwards.
+///  - modules can be destroyed before/after ClangdServer and ClangdLSPServer
+///    FIXME: Once we make server facilities available to modules, we'll need to
+///    ensure ClangdServer is destroyed after all the modules are done/gone.
+///
+/// Conventionally, standard modules live in the `clangd` namespace, and other
+/// exposed details live in a sub-namespace.
+class Module {
+public:
+  virtual ~Module() = default;
+
+  /// Identifier for the plugin, should be unique.
+  virtual llvm::StringLiteral id() = 0;
+
+  /// Some modules might own background tasks. They should override this method
+  /// to indicate status of these tasks.
+  virtual void blockUntilIdle() {}
+};
+
+/// Wrapper around a set of modules to provide type based grouping.
+// Ideas for implementing this:
+// - dyn_cast (with possibly some caching on top to only traverse once)
+// - have a plugin registry for each individual module type?
+// - make constructor of derived modules register themselves to a
+//   singleton-list, and provide an endpoint to iterate over.
+// I am leaning towards dyn_cast option and precomputing the grouping at
+// construction time in ModuleSet. WDYT?
+class ModuleSet {
+public:
+  explicit ModuleSet(std::vector<std::unique_ptr<Module>> Modules);
+  // Returns all the modules of type T.
+  template <typename T> std::vector<T *> getModules() { return {}; }
+};
+
+} // namespace clangd
+} // namespace clang
+#endif

clang-tools-extra/clangd/Module.cpp

+//===--- Module.cpp - 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "Module.h"
+
+namespace clang {
+namespace clangd {} // namespace clangd
+} // namespace clang

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