Feature modules (FeatureModule.*) provide great ability to extend clangd functionality without modifications of clangd itself (e.g. add an LSP extension without modifications inside ClangdLSPServer.cpp) , but there is no registry for these module: we have no something similar to ClangTidyModuleRegistry in clang-tidy.
Such a registry can significantly simplify integration of feature module plugins, even such plugins can be dynamic libraries. Code inside main() can be like this:
FeatureModuleSet ModuleSet;
for (FeatureModuleRegistry::entry E : FeatureModuleRegistry::entries()) {
ModuleSet.add(….);
}
Opts.FeatureModules = &ModuleSet;
At trying to add a feature module registry locally I found that with current feature modules design this is not possible: feature module can expose an individual public API (i.e. call a feature module public method, which exists only in this module) and this imposes restrictions on a feature module set :
- only one feature module of each type can be in a set
- key of feature module set is a feature module type id (so we can’t just add a
FeatureModuleinto a set without exact type knowledge)
Thus, here I see a tradeoff:
expose a public API of individual feature module VS feature module registry.
From my point of view, registry for feature modules looks more promising.
Maybe some kind of hybrid solution can be acceptable here? FeatureModuleSet class can contain two maps inside:
- where key is a module type id (for modules which expose public API)
- where key is a module name (for modules which don’t expose public API and can be added to a registry)
@kadircet @HighCommander4 WDYT?
hi, we initially didn’t want to (and need to) register these opaquely through link-time magic and rather wanted it to be explicit at compile-time. this decision was mostly based on the assumption that, if people can link custom modules, they can also patch sources to add them at compile time (yes this is a local patch, but maintenance burden there should rather be negligible).
Regarding loading dynamic libraries, there lies more dragons, so I’d deliberately stay away from that. Clang AST is not ABI-stable. Hence clangd and dynamic libraries need to be build against the same clang version for anything to work properly, at which point everyone might as well link them statically.
So I’d probably still lean towards listing modules explicitly at compile-time and maintaining downstream patches to clangdmain, but if this doesn’t work for you (I’d be curious to know about your setup in that case), or if you still feel like listing feature-modules at link-time is preferred by you, I think it’s fine to move in this direction. I think we can just extend FeatureModuleSet to add modules only into the underlying vector (without concrete type information), without adding an entry into the map. As most of the use cases only iterate over the vector. Later on we can extend this if there’s a need to access modules by name. WDYT?
1 Like
Hi.
Thank you for your reply!
The main scenario is to cover different expected behavior of clangd from different users. E.g. textDocument/definitionjumps to declaration if we do this request on a function definition, and maybe 90% of users are happy with this, but 10% don’t and expect jump to definition even if we are already staying on it. In such a case we can provide a dynamic library with a feature module which overrides textDocument/definitionsbehavior, making it expected to these 10%.
For now I am not sure, do I really need an ability to access modules by name or not, so I think adding API to add a feature module into FeatureModuleSet without updating the map is a good starting point here.
I was rather asking about how you build and deploy clangd binaries.
As I mentioned above, loading such feature-modules from dynamic libraries is problematic (I am not saying it’s impossible, but we need to think this thoroughly, and it’s more about clang’s API/ABI stability then clangd’s feature modules). I am still OK with discovery of statically linked-in feature-modules in clangd main though (as in your proposal above).
So I believe the best we can do here is, make all the feature-modules available at compile/link-time and let the user enable disable them via command line flags or config options.
I have internal changes to export clangd APIs if CLANG_PLUGIN_SUPPORT is enabled (similar to what we have for clang-tidy):
- call export_executable_symbols_for_plugins() for clangd
- copy clangd headers into LLVM installation
With such changes, LLVM installation can be used to build dynamic libraries for clangd without accessing to the codebase (but only LLVM installation). Thus, adding some extra functionality doesn’t require clangd update with extra command line options. Dynamic feature modules can be distributed separately and can be managed on the client side, which starts clangd (e.g. feature modules enumeration can be done by the client, and some GUI with checkboxes can be generated).
In my case dynamic feature modules build process uses the same version of LLVM installation, from which clangd comes.
It’s all clear about clang’s API instability, but we have the same situation for other tools, where plugins as a dynamic libraries can be used (e.g. clang itself)
Anyway, for me it’s ok to start from this:
So, I will start to work on a patch to provide this functionality.
