This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Introduce Modules
ClosedPublic

Authored by kadircet on Feb 7 2021, 11:08 PM.

Details

Summary

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

Diff Detail

Event Timeline

kadircet created this revision.Feb 7 2021, 11:08 PM
kadircet requested review of this revision.Feb 7 2021, 11:08 PM
kadircet added inline comments.Feb 8 2021, 2:54 AM
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 updated this revision to Diff 322744.Feb 10 2021, 10:40 AM
  • Rename Plugin to Module
  • Have a ModuleSet that enables type-safe traversal of existing modules (with questions about implementation options)
kadircet retitled this revision from [clangd][RFC] Introudce Plugins to [clangd] Introduce Modules.Feb 10 2021, 10:42 AM
kadircet edited the summary of this revision. (Show Details)
kadircet updated this revision to Diff 322754.Feb 10 2021, 10:55 AM
  • Changes to propagate moduleset into clangd(lsp)server

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 updated this revision to Diff 323290.Feb 12 2021, 4:40 AM
kadircet marked 7 inline comments as done.
  • 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 accepted this revision.Feb 12 2021, 6:40 AM
sammccall added inline comments.
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())
This revision is now accepted and ready to land.Feb 12 2021, 6:40 AM
kadircet updated this revision to Diff 323361.Feb 12 2021, 9:28 AM
kadircet marked 7 inline comments as done.
  • Address comments
kadircet added inline comments.Feb 12 2021, 9:29 AM
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)

This revision was landed with ongoing or failed builds.Feb 12 2021, 9:37 AM
This revision was automatically updated to reflect the committed changes.

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

"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?

"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?