[PATCH] D91029: [clangd] Implement clang-tidy options from config

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for working on this!

I think the scope of this patch is probably bigger than we need, at least initially:

- it adds a new TidyProvider system with a lot of flexibility. But our config needs are quite static. The only flexibility we need initially is being able to swap out reading .clang-tidy for a different strategy, and the ability to replace the whole thing with a fixed config (for `-checks`)
- the clangd::Config based check configuration probably does need depend on some refactoring of how ClangTidyOptions are created, but it doesn't need to land in the same patch
- it adds caching around the file IO, which may be nice to have but is complicated and unrelated and shouldn't be mixed with the refactoring

Do you want to take a shot at scoping this down? Is it useful if I sketch what I mean for the refactoring part?



================
Comment at: clang-tools-extra/clangd/ClangdServer.h:367
 
-  // When set, provides clang-tidy options for a specific file.
-  ClangTidyOptionsBuilder GetClangTidyOptions;
+  // OptionsPrivder for clang-tidy.
+  ClangdTidyProvider *ClangTidyProvider = nullptr;
----------------
new comment doesn't really say anything, revert to the old one?


================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:250
   if (Preamble && Preamble->StatCache)
-    VFS = Preamble->StatCache->getConsumingFS(std::move(VFS));
+    VFS = Preamble->StatCache->getConsumingFS(VFS.get());
 
----------------
why this change?


================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:310
     CTContext->setCurrentFile(Filename);
+    dlog("ClangTidy configuration for file {0}: {1}", Filename,
+         tidy::configurationAsText(CTContext->getOptions()));
----------------
nit: move a few lines above to after the options-initialization if-stmt?


================
Comment at: clang-tools-extra/clangd/TidyProvider.h:20
+/// The base class of all clang-tidy config providers for clangd. The option
+/// getters take a pointer to a Filesystem that should be used for any file IO.
+class ClangdTidyProvider {
----------------
Any reason to pass in a VFS to get a ClangTidyOptionsProvider here, rather than having the subclass take a ThreadsafeFS in its constructor if it needs one?

This interface seems a bit strange, because not all implementations of ClangTidyProvider would necessarily get their config from a VFS.

(Moreover, it seems like ClangTidyProvider could just be a ClangTidyOptionsProvider with a threadsafety guarantee, if it weren't for the fact that ClangTidyContext wants to take ownership of the optionsprovider, which doesn't make a lot of sense - maybe we should fix that instead?)


================
Comment at: clang-tools-extra/clangd/TidyProvider.h:21
+/// getters take a pointer to a Filesystem that should be used for any file IO.
+class ClangdTidyProvider {
+public:
----------------
we don't usually use a "Clangd" prefix in the clangd namespace. (Clangd[LSP]Server being notable and very old exceptions)


================
Comment at: clang-tools-extra/clangd/TidyProvider.h:40
+/// A Provider that will mimic the behaviour of tidy::FileOptionsProvider but
+/// it caches options retrieved in a thread safe manner. To be completely thread
+/// safe it must be passed a thread safe filesystem when reading files.
----------------
how is the cache invalidated? (AFAICT currently it never is)


================
Comment at: clang-tools-extra/clangd/TidyProvider.h:41
+/// it caches options retrieved in a thread safe manner. To be completely thread
+/// safe it must be passed a thread safe filesystem when reading files.
+class FileTidyProvider : public ClangdTidyProvider {
----------------
We don't have a way to guarantee this, the vfs::FileSystems provided by ThreadsafeFS are arbitrary (ThreadsafeFS is an extension point) and don't have to be threadsafe.

If you need a filesystem that's threadsafe, that's what ThreadsafeFS is for.


================
Comment at: clang-tools-extra/clangd/TidyProvider.h:74
+  // time, while preventing any thread updating the cache.
+  std::shared_timed_mutex CacheGuard;
+  llvm::StringMap<OptionsSource> CachedOptions;
----------------
reader-writer locks are a nice model but very commonly perform equal or worse to plain mutexes in practice.
I think we should have evidence this is an improvement before using it.


================
Comment at: clang-tools-extra/clangd/TidyProvider.h:85
+/// removing checks known to cause issues in clang-tidy.
+class CheckAdjustingTidyProvider : public FileTidyProvider {
+public:
----------------
Using a deep inheritance hierarchy here parallel to the hierarchy of ClangTidyOptionsProvider is IMO too much complexity for the problem being solved. Most of these classes want to be functions.


================
Comment at: clang-tools-extra/clangd/TidyProvider.h:85
+/// removing checks known to cause issues in clang-tidy.
+class CheckAdjustingTidyProvider : public FileTidyProvider {
+public:
----------------
sammccall wrote:
> Using a deep inheritance hierarchy here parallel to the hierarchy of ClangTidyOptionsProvider is IMO too much complexity for the problem being solved. Most of these classes want to be functions.
in this design, we don't have a good way to use the check-adjustment together with a non-file source of configs.

(At google we use this capability to support a different project configuration format that's used instead of .clang-tidy files)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91029