[clangd] Drop FS usage in ClangTidyOpts

[kadircet]

Summary

Depends on D81998

[kadircet]

• Accept an inner opt provider instead to enable wrapping other types of providers.

[kadircet]

• Rebase

[sammccall]

clang-tools-extra/clangd/ClangdServer.h

46

Hmm, I like the idea of avoiding a custom type and just reusing the standard one here, but:

• taking someone else's interface (which has existing implementations, some of the key ones being subtly non-threadsafe) and slapping "must be threadsafe" on it seems like a recipe for bugs
• ClangTidyOptionsProvider is a really horrible interface that exposes several things we don't need

anything wrong with just std::function<ClangTidyOptions(StringRef)>?
It's reasonable to use a ClangTidyOptionsProvider to create them, though I'm not sure it actually saves anything.

[njames93]

clang-tools-extra/clangd/tool/ClangdMain.cpp

495–498

if (llvm::none_of(Sources, [](const auto &Source) { return Source.first.Checks.hasValue(); }))

[njames93]

I'm of the idea that rather than having ClangdTidyOptionsProvider inherit form tidy::ClangTidyOptionsProvider, just have it as its own class. We don't need the interface offered by clang tidy here. It would solve the must be threadsafe comment issue as well as reduce the need for some unnecessary code in there.

clang-tools-extra/clangd/ClangdServer.h

 #include <future>
 #include <string>
 #include <type_traits>
 #include <utility>
 
 namespace clang {
 namespace clangd {
 
-/// When set, used by ClangdServer to get clang-tidy options for each particular
-/// file. Must be thread-safe. We use this instead of ClangTidyOptionsProvider
-/// to allow reading tidy configs from the VFS used for parsing.
-using ClangTidyOptionsBuilder = std::function<tidy::ClangTidyOptions(
-    llvm::vfs::FileSystem &, llvm::StringRef /*File*/)>;
-
 /// Manages a collection of source files and derived data (ASTs, indexes),
 /// and provides language-aware features such as code completion.
 ///
 /// The primary client is ClangdLSPServer which exposes these features via
 /// the Language Server protocol. ClangdServer may also be embedded directly,
 /// though its API is not stable over time.
 ///
 /// ClangdServer should be used from a single thread. Many potentially-slow
struct Options {
     /// If set, use this index to augment code completion results.
     SymbolIndex *StaticIndex = nullptr;
 
     /// If set, enable clang-tidy in clangd and use to it get clang-tidy
     /// configurations for a particular file.
     /// Clangd supports only a small subset of ClangTidyOptions, these options
     /// (Checks, CheckOptions) are about which clang-tidy checks will be
     /// enabled.
-    ClangTidyOptionsBuilder GetClangTidyOptions;
+    /// Note that this must be thread-safe.
+    tidy::ClangTidyOptionsProvider *TidyOptProvider = nullptr;
 
     /// If true, turn on the `-frecovery-ast` clang flag.
     bool BuildRecoveryAST = true;
 
     /// If true, turn on the `-frecovery-ast-type` clang flag.
     bool PreserveRecoveryASTType = false;
 
     /// Clangd's workspace root. Relevant for "workspace" operations not bound
private:
   // If present, an index of symbols in open files. Read via *Index.
   std::unique_ptr<FileIndex> DynamicIdx;
   // If present, the new "auto-index" maintained in background threads.
   std::unique_ptr<BackgroundIndex> BackgroundIdx;
   // Storage for merged views of the various indexes.
   std::vector<std::unique_ptr<SymbolIndex>> MergedIdx;
 
   // When set, provides clang-tidy options for a specific file.
-  ClangTidyOptionsBuilder GetClangTidyOptions;
+  tidy::ClangTidyOptionsProvider *TidyOptProvider = nullptr;
 
   // If this is true, suggest include insertion fixes for diagnostic errors that
   // can be caused by missing includes (e.g. member access in incomplete type).
   bool SuggestMissingIncludes = false;
 
   // If true, preserve expressions in AST for broken code.
   bool BuildRecoveryAST = true;
   // If true, preserve the type for recovery AST.

clang-tools-extra/clangd/ClangdServer.cpp

 
 ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB,
                            const ThreadSafeFS &FSProvider, const Options &Opts,
                            Callbacks *Callbacks)
     : FSProvider(FSProvider),
       DynamicIdx(Opts.BuildDynamicSymbolIndex
                      ? new FileIndex(Opts.HeavyweightDynamicSymbolIndex)
                      : nullptr),
-      GetClangTidyOptions(Opts.GetClangTidyOptions),
+      TidyOptProvider(Opts.TidyOptProvider),
       SuggestMissingIncludes(Opts.SuggestMissingIncludes),
       BuildRecoveryAST(Opts.BuildRecoveryAST),
       PreserveRecoveryASTType(Opts.PreserveRecoveryASTType),
       TweakFilter(Opts.TweakFilter), WorkspaceRoot(Opts.WorkspaceRoot),
       // Pass a callback into `WorkScheduler` to extract symbols from a newly
       // parsed file and rebuild the file index synchronously each time an AST
       // is parsed.
       // FIXME(ioeric): this can be slow and we may be able to index on less

 }
 
 void ClangdServer::addDocument(PathRef File, llvm::StringRef Contents,
                                llvm::StringRef Version,
                                WantDiagnostics WantDiags, bool ForceRebuild) {
   ParseOptions Opts;
   Opts.ClangTidyOpts = tidy::ClangTidyOptions::getDefaults();
   // FIXME: call tidy options builder on the worker thread, it can do IO.
-  if (GetClangTidyOptions)
-    Opts.ClangTidyOpts =
-        GetClangTidyOptions(*FSProvider.view(/*CWD=*/llvm::None), File);
+  if (TidyOptProvider)
+    Opts.ClangTidyOpts = TidyOptProvider->getOptions(File);
   Opts.SuggestMissingIncludes = SuggestMissingIncludes;
 
   // Compile command is set asynchronously during update, as it can be slow.
   ParseInputs Inputs;
   Inputs.FSProvider = &FSProvider;
   Inputs.Contents = std::string(Contents);
   Inputs.Version = Version.str();
   Inputs.ForceRebuild = ForceRebuild;

clang-tools-extra/clangd/tool/ClangdMain.cpp

 #include "CodeComplete.h"
 #include "Features.inc"
 #include "PathMapping.h"
 #include "Protocol.h"
 #include "Transport.h"
 #include "index/Background.h"
 #include "index/Serialization.h"
 #include "refactor/Rename.h"
+#include "support/FSProvider.h"
 #include "support/Path.h"
 #include "support/Shutdown.h"
 #include "support/Trace.h"
 #include "clang/Basic/Version.h"
 #include "clang/Format/Format.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"

 };
 
 #ifdef _WIN32
 const char TestScheme::TestDir[] = "C:\\clangd-test";
 #else
 const char TestScheme::TestDir[] = "/clangd-test";
 #endif
 
+// A thread-safe options provider suitable for use by ClangdServer. It also
+// provides some default checks if user has specified none.
+class ClangdTidyOptionsProvider : public clang::tidy::ClangTidyOptionsProvider {
+public:
+  ClangdTidyOptionsProvider(
+      std::unique_ptr<tidy::ClangTidyOptionsProvider> Inner)
+      : Inner(std::move(Inner)) {
+    assert(this->Inner);
+  }
+
+  std::vector<OptionsSource> getRawOptions(llvm::StringRef FileName) override {
+    std::vector<OptionsSource> Sources;
+    {
+      std::lock_guard<std::mutex> Lock(Mu);
+      Sources = Inner->getRawOptions(FileName);
+    }
+    // If the user hasn't configured clang-tidy checks at all, including via
+    // .clang-tidy, give them a nice set of checks.
+    // (This should be what the "default" options does, but it isn't...)
+    bool HasChecks = false;
+    for (const auto &Source : Sources)
+      HasChecks |= Source.first.Checks.hasValue();
+    if (!HasChecks)
+      Sources.push_back(DefaultClangdSource);
+    return Sources;
+  }
+
+  const tidy::ClangTidyGlobalOptions &getGlobalOptions() override {
+    std::lock_guard<std::mutex> Lock(Mu);
+    return Inner->getGlobalOptions();
+  }
+
+private:
+  std::mutex Mu;
+  std::unique_ptr<ClangTidyOptionsProvider> Inner;
+  const OptionsSource DefaultClangdSource = []() -> OptionsSource {
+    tidy::ClangTidyOptions Opts;
+    // These default checks are chosen for:
+    //  - low false-positive rate
+    //  - providing a lot of value
+    //  - being reasonably efficient
+    Opts.Checks = llvm::join_items(
+        ",", "readability-misleading-indentation",
+        "readability-deleted-default", "bugprone-integer-division",
+        "bugprone-sizeof-expression", "bugprone-suspicious-missing-comma",
+        "bugprone-unused-raii", "bugprone-unused-return-value",
+        "misc-unused-using-decls", "misc-unused-alias-decls",
+        "misc-definitions-in-headers");
+    return {Opts, "-clangd-opts"};
+  }();
+};
 } // namespace
 } // namespace clangd
 } // namespace clang
 
 enum class ErrorResultCode : int {
   NoShutdownRequest = 1,
   CantRunAsXPCService = 2
 };
if (!PathMappingsArg.empty()) {
     auto Mappings = parsePathMappings(PathMappingsArg);
     if (!Mappings) {
       elog("Invalid -path-mappings: {0}", Mappings.takeError());
       return 1;
     }
     TransportLayer = createPathMappingTransport(std::move(TransportLayer),
                                                 std::move(*Mappings));
   }
-  // Create an empty clang-tidy option.
-  std::mutex ClangTidyOptMu;
-  std::unique_ptr<tidy::ClangTidyOptionsProvider>
-      ClangTidyOptProvider; /*GUARDED_BY(ClangTidyOptMu)*/
+  std::unique_ptr<tidy::ClangTidyOptionsProvider> ClangTidyOptProvider;
   if (EnableClangTidy) {
     auto EmptyDefaults = tidy::ClangTidyOptions::getDefaults();
     EmptyDefaults.Checks.reset(); // So we can tell if checks were ever set.
     tidy::ClangTidyOptions OverrideClangTidyOptions;
     if (!ClangTidyChecks.empty())
       OverrideClangTidyOptions.Checks = ClangTidyChecks;
-    ClangTidyOptProvider = std::make_unique<tidy::FileOptionsProvider>(
-        tidy::ClangTidyGlobalOptions(),
-        /* Default */ EmptyDefaults,
-        /* Override */ OverrideClangTidyOptions,
-        FSProvider.view(/*CWD=*/llvm::None));
-    Opts.GetClangTidyOptions = [&](llvm::vfs::FileSystem &,
-                                   llvm::StringRef File) {
-      // This function must be thread-safe and tidy option providers are not.
-      tidy::ClangTidyOptions Opts;
-      {
-        std::lock_guard<std::mutex> Lock(ClangTidyOptMu);
-        // FIXME: use the FS provided to the function.
-        Opts = ClangTidyOptProvider->getOptions(File);
-      }
-      if (!Opts.Checks) {
-        // If the user hasn't configured clang-tidy checks at all, including
-        // via .clang-tidy, give them a nice set of checks.
-        // (This should be what the "default" options does, but it isn't...)
-        //
-        // These default checks are chosen for:
-        //  - low false-positive rate
-        //  - providing a lot of value
-        //  - being reasonably efficient
-        Opts.Checks = llvm::join_items(
-            ",", "readability-misleading-indentation",
-            "readability-deleted-default", "bugprone-integer-division",
-            "bugprone-sizeof-expression", "bugprone-suspicious-missing-comma",
-            "bugprone-unused-raii", "bugprone-unused-return-value",
-            "misc-unused-using-decls", "misc-unused-alias-decls",
-            "misc-definitions-in-headers");
-      }
-      return Opts;
-    };
+    auto InnerProvider = std::make_unique<tidy::FileOptionsProvider>(
+        tidy::ClangTidyGlobalOptions(), std::move(EmptyDefaults),
+        std::move(OverrideClangTidyOptions), FSProvider.view(llvm::None));
+    ClangTidyOptProvider =
+        std::make_unique<ClangdTidyOptionsProvider>(std::move(InnerProvider));
   }
+  Opts.TidyOptProvider = ClangTidyOptProvider.get();
   Opts.SuggestMissingIncludes = SuggestMissingIncludes;
   Opts.QueryDriverGlobs = std::move(QueryDriverGlobs);
 
   Opts.TweakFilter = [&](const Tweak &T) {
     if (T.hidden() && !HiddenFeatures)
       return false;
     if (TweakList.getNumOccurrences())
       return llvm::is_contained(TweakList, T.id());