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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked an inline comment as done.
kadircet added a comment.

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.

`ClangTidy`:

- needs extra hooks to register PP callbacks, and take in a diagnostics engine
- needs a new endpoint to traverse ast and *emit* diags
- CTContext needs to be alive until the parsing is done: so we can:
  - make ASTHooks own it and instantiate a new one on every parse (i think the cleanest and most explicit)
  - make the module own them and control the lifetime with entry/exit calls on the asthooks. (there's more burden on the modules now, and they'll need extra synchronisation on enter/exit calls)
  - return some other object on parsing entry hook that'll be kept alive by the caller (needs design around semantics of that object).

`IncludeFixer`:

- needs a new hook to act as an external sema source, so that it can fix unresolved names.
- similar to CTContext issue, it references per-tu state like HeaderSearchInfo, so will need to incorporate these into entry hook, and somehow disambiguate hooks for different TUs (all the options proposed for tidy).

`IWYU`:

- I'd expect this to make use of ~same API as `IncludeFixer`.

`PreamblePatching`:

- can mutate compiler instance via a hook
- drop diagnostics from ParsedAST on exit (requires some mutation on parsedast though)



================
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; }
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98499