[PATCH] D98499: [clangd] Introduce ASTHooks to FeatureModules

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D98499#2644313 <https://reviews.llvm.org/D98499#2644313>, @kadircet wrote:

> In D98499#2626502 <https://reviews.llvm.org/D98499#2626502>, @sammccall wrote:
>
>> My model for modules using this diagnostic stuff (apart from the build-system stuff which sadly can't be meaningfully upstreamed) are IncludeFixer, ClangTidy, and IWYU - worth thinking about how we'd build those on top of this model. (Doesn't mean we need to add a hook to emit diagnostics, but it probably means we should know where it would go)
>
> Agreed. I believe they all would need extra end points to ASTHooks though.

Yup. Let's not bite off more for now.

> - make ASTHooks own it and instantiate a new one on every parse (i think the cleanest and most explicit)

Agreed. (And this only one that overlaps this patch a lot)



================
Comment at: clang-tools-extra/clangd/FeatureModule.h:112
+  };
+  /// Can be called asynchronously before building an AST.
+  virtual std::unique_ptr<ParseASTHooks> astHooks() { return nullptr; }
----------------
kadircet wrote:
> sammccall wrote:
> > This comment hints at what I *thought* was the idea: astHooks() is called each time we parse a file, the returned object has methods called on it while the file is being parsed, and is then destroyed.
> > 
> > But the code suggests we call once globally and it has effectively the same lifetime as the module.
> > This seems much less useful, e.g. if we want to observe several diagnostics, examine the preamble, and emit new diagnostics, then we have to plumb around some notion of "AST identity" rather than just tying it to the identity of the ParseASTHooks object itself.
> > 
> > (Lots of natural extensions here like providing ParseInputs to astHooks(), but YAGNI for now)
> > This comment hints at what I *thought* was the idea: astHooks() is called each time we parse a file, the returned object has methods called on it while the file is being parsed, and is then destroyed.
> 
> This was the intent actually (to enable modularization of other features like clangtidy, includefixer, etc. as mentioned above), but looks like i got confused while integrating this into clangdserver :/
> 
> While trying to resolve my confusion i actually noticed that we cannot uphold the contract of "hooks being called synchronously", because we actually have both a preamblethread and astworker that can invoke hooks (embarrassing of me to forget that 😓).
> 
> So we can:
> - Give up on that promise and make life slightly complicated for module implementers
> - Don't invoke hooks from preamblethread, (at the cost of limiting functionality, we can still have downstream users that act on PPCallbacks, but no direct integration with compiler, and that's where layering violations are generated :/)
> - Handle the synchronization ourselves, only complicates TUScheduler more, rather than all the module implementers.
> - Propogate FeatureModuleSet into TUScheduler and create 2 set of hooks on each thread :)
> 
> I am leaning towards 4, but unsure (mostly hesitant about dependency schema, as featuremodules also depend on TUScheduler..). WDYT?
> we actually have both a preamblethread and astworker that can invoke hooks (embarrassing of me to forget that 😓)

Ha! And yes, in our motivating case of fixing build system rules, the diagnostics are mostly going to be in the preamble.

The options that seem most tempting to me are:
 - don't attempt to create/run astHooks while building preambles, but *do* feed the preamble's Diags into the main-file's AST hooks every time it's used. (you won't have a clang::Diagnostic, so that param would have to be gone/optional, and we'd scrape the messages instead of extracting args). This is kind of thematic, remember how we replay PPCallbacks for clang-tidy :-). This is the smallest tweak to your #2 that actually works for us, I think.
 - create one astHooks for the preamble, and another for the AST build, and try to make the interface suitable for both. This is cute but there may be too much tension between the two cases. (Is this what you mean by #4?)
 - or have separate ASTHooks & PreambleHooks interfaces and support all this crap for both. (Or is *this* what you mean by #4?) Hybrid is also possible, give the interfaces an inheritance relationship, or have one interface but pass boolean parameters to indicate which version we're doing, or...
 - preambles and ASTs are a tree, so... give modules a PreambleHooks factory, and the factory function for ASTHooks is PreambleHooks::astHooks(). Holy overengineering batman...

These are roughly in order of complexity so we should probably start toward the top of the list somewhere. Up to you.

(I don't like the #1 or #3 in your list above much at all.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98499/new/

https://reviews.llvm.org/D98499