The integration of clang-tidy into clangd is currently scattered across many files: ClangdServer.cpp/h, Diagnostics.cpp/h, Compiler.h, Preamble.cpp, ParsedAST.cpp, Check.cpp, and ClangdMain.cpp.
Changes to the clang-tidy integration typically require multiple changes in clangd, which is quite inconvenient and creates a risk of overlooking something.
clangd supports extending functionality via “feature modules”, a mechanism implemented quite some time ago (see [clangd] Introduce Modules · llvm/llvm-project@2423a38 · GitHub ), yet there is still not a single real-world example of their use (only tests) and no documentation.
Moving clang-tidy into a feature module will not only decouple the clang-tidy integration from clangd but also provide a real-world example of the feature module in action, serving as a foundation for potential new functional integrations without requiring deep modifications to clangd.
I’ve already tried moving clang-tidy into a feature module in my fork here, and it works overall. However, I had to extend the FeatureModule interface to make this possible. So, my plan is as follows:
- Create a separate PR to extend the feature module functionality. This shouldn’t affect anything; even if there are existing internal (e.g. inside a company) feature modules, the interaction interface will remain fully compatible
I like the idea here. Clangd is designed towards the LSP functionalities. The main benefit you are achieving is the consolidation of the tidy code. However, this could also be done with direct function calls on your consolidated class instead of passing through the module API.
Given you state that this would be the first use case for the modules since it’s introduction (in LLVM 13), I wonder how good of a fit this is. Are we working with a gem or are we trying to push a square peg through a round hole? With my experiences, I’m really skeptical towards first writing a framework and then finding use-cases for it. I’ve seen much more success with abstracting a framework out of the use-cases.
The other question would be: what are the other use-cases for this framework?
I’d be inclined to suggest that you keep the abstractions and hooks, though don’t use the module framework for it, just call the consolidated class directly. I’n even considering removing the module framework if there are no (other) use-cases.
In this case we will still have several clang-tidy specific function calls across clangd. So, this can’t be done in this way.
Originally, FeatureModule functionality was implemented by Google developers and according to this comment, they have some internal feature modules.
I also use feature module functionality in internal fork to minimize changes with original clangd and I find feature module functionality very useful (e.g. quite easy to write LSP extensions without original clangd code modifications).
So, we for sure have feature module users, but there is no real-word example in open source, that was my point. Nobody use this API not because it’s useless, but because nobody knows about this ability.
You’re right. I think that feature modules were originally created precisely to avoid releasing certain internal, company-specific functionality as open source. However, in the case of clang-tidy, implementing it as an in-tree feature module still makes sense, given how widely clang-tidy-specific code is distributed throughout clangd.
I think whether a refactor like this is a net win for maintainability in the longer term is likely to be highly dependent on the specifics of the FeatureModule abstraction (including any proposed extensions), and how good of a proposed fit clang-tidy is for it.
I haven’t studied the current FeatureModule abstraction or your patches so I don’t have an opinion on the specifics at this time. That said, I’ve definitely encountered cases in the LLVM project where I’ve found an abstraction to be pretty constraining. (For example, I’ve recently been looking at proposed enhancements to clangd-indexer and the ToolExecutor/ClangTool/FrontendActionFactory stuff that it’s built on is fairly limiting.)
That said, I think the following are two positive signs:
The fact that you’ve used the abstraction in an internal fork and found it useful.
The fact that the abstraction is internal to clangd, so we should have wide latitude to change/extend it to fit our needs. (For example, if a new piece of clang-tidy integration is introduced in the future, and support for it requires an extension to the FeatureModule interface, I’d hope that would be uncontroversial.)
One thought: what do you think about creating a documentation page under Design of clangd about FeatureModules which explains the idea behind the interface and maybe give a couple of examples of potential uses? I think that could be a useful aid in understanding and evaluating the proposed changes.