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

Tue Nov 24 09:45:34 PST 2020

sammccall added a comment.

OK this looks really great, thanks so much for persisting with this.

Comments are mostly simple nits/simplifications, with the exception of `Order` which is a... slightly trickier simplification, but seems worth doing.

Tomorrow I'll adapt our internal clang-tidy config bits to work with this interface...

================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:241
+/// Empty clang tidy provider, using this as a provider will disable clang-tidy.
+static void emptyTidyProvider(tidy::ClangTidyOptions &, llvm::StringRef,
+                              unsigned &, PathRef) {}
----------------
or just use fixedTidyProvider("-*")

================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:74
+    auto EmptyDefaults = tidy::ClangTidyOptions::getDefaults();
+    EmptyDefaults.Checks.reset(); // So we can tell if checks were ever set.
+    EmptyDefaults.User = llvm::sys::Process::GetEnv("USER");
----------------
I'm not sure if this applies anymore - it's going to be the bottom of the stack, and nullopt is the default

================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:75
+    EmptyDefaults.Checks.reset(); // So we can tell if checks were ever set.
+    EmptyDefaults.User = llvm::sys::Process::GetEnv("USER");
+#ifdef _WIN32
----------------
nit: hoist the getenv out of the lambda?

================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:75
+    EmptyDefaults.Checks.reset(); // So we can tell if checks were ever set.
+    EmptyDefaults.User = llvm::sys::Process::GetEnv("USER");
+#ifdef _WIN32
----------------
sammccall wrote:
> nit: hoist the getenv out of the lambda?
seems like we can just assign to Opts.User directly?

================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:99
+              "misc-definitions-in-headers");
+          Opts.Checks.emplace(DefaultChecks);
+        }
----------------
nit: = seems clearer than emplace

================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:110
+          tidy::ClangTidyOptions &Opts, llvm::StringRef, unsigned &, PathRef) {
+        mergeCheckList(Opts.Checks, Checks);
+        mergeCheckList(Opts.WarningsAsErrors, WarningsAsErrors);
----------------
I think should just be assignment rather than merge (same with WarningsAsErrors).
The name suggests that it always returns the same set of checks.

(Failing that, maybe call it `addTidyChecks`)

================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:167
+
+    if (FS->makeAbsolute(AbsolutePath))
+      return;
----------------
(as noted elsewhere, the path is always absolute in clangd and we should make this an interface requiremnet)

================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:170
+
+    llvm::sys::path::remove_dots(AbsolutePath, true);
+    llvm::StringRef Directory = llvm::sys::path::parent_path(AbsolutePath);
----------------
I don't think we need to try to stat the directory here.

FileOptionsProvider does, but looking at the history, this seems related to some combination of:
 - arg validation (the parameter there is a directory name, rather than a filename)
 - trying to return an appropriate error_code (original return type was ErrorOr<Options>)
 - iterating over multiple possible config files
 - maybe caching

================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:181
+    }
+
+    for (llvm::StringRef CurrentDirectory = Directory;
----------------
can you add a FIXME to add caching here?

I hadn't realized FileOptionsProvider *was* actually doing caching.

(I don't think this is a big problem in the short term, and I don't think we should solve it inline in this patch, rather I should land D88172 and use that, which doesn't have the annoying "cache forever" behavior)

================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:194
+    for (auto &Option : llvm::reverse(OptionStack))
+      Opts.mergeWith(Option, ++Order);
+  };
----------------
This order business is a real tangle, and looks like legacy to me (qualified vs unqualified names).

I don't really think we want to use/expose it beyond the requirement that we don't change the meaning of existing sets of `.clang-tidy` config files.
And I don't think we want to bother teaching clangd's config system about it.

So I'd suggest a hack/simplification:
 - we remove the `order` parameter from TidyProvider
 - here in provideClangTidyFiles, we just use a local Order which counts up from 1. This preserves the relative precedence of .clang-tidy files
 - in provideClangdConfig, we always set the order to max_uint (always wins)
 - if we ever add checkoptions to any other providers, we set the order to 0 (always loses)
 - combine() doesn't touch order

This *does* duplicate the fact that .clangd config is stacked on top of .clang-tidy in ClangdMain, but in a way that only matters with disputes between qualified/unqualified names.

================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:209
+
+class TidyProviderInstance : public tidy::ClangTidyOptionsProvider {
+public:
----------------
nit: "adapter" may be clearer than "instance", up to you

================
Comment at: clang-tools-extra/clangd/TidyProvider.h:25
+                                   /*Priority=*/unsigned &,
+                                   /*CWD*/ PathRef);
+} // namespace detail
----------------
we should always be passing an absolute path, so CWD shouldn't be needed.

I'm a bit surprised that ParsedAST::build doesn't assert that (though it calls PreamblePatch::create, which does). Feel free to add it!

================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:166

+list<std::string> DisabledClangTidyChecks{
+    "disable-clang-tidy-checks", cat(Features),
----------------
ah, sorry, I led you astray here... I don't think we actually need a flag to control this in ClangdMain.cpp, just that the layering allows one :-)

Google's production deployment of clangd has a different entrypoint (not ClangdMain.cpp) which has a flag to disable checks. A previous version of this patch buried the list of disabled checks very deeply so that it couldn't be changed, but building the TidyProvider stack in clangdmain addresses this (our bizarro_clangd_main.cpp can do its own version making use of its flag).

You can keep this if you like, but I don't think it's necessary (for debugging we can just use -clang-tidy-checks, and for users working around bugs we can use clangd config).

================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:850
+    Providers.push_back(provideEnvironment());
+    Providers.push_back(provideClangTidyFiles(TFS));
+    if (EnableConfig)
----------------
if -clang-tidy-checks is provided, we don't want to parse .clang-tidy files, respect clangd config, or disable unusable checks - just environment + the flag.
The main purpose of that flag is to debug e.g. isolate crashes down to individual checks, so full control is best.

(I believe this matches the old behavior, but it's really hard to tell from the code - the new code is much nicer!)

================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:853
+      Providers.push_back(provideClangdConfig());
+    if (ClangTidyChecks.empty())
+      Providers.push_back(fixedTidyProvider(ClangTidyChecks));
----------------
I think the condition is backwards

================
Comment at: clang-tools-extra/clangd/unittests/ClangdTests.cpp:1221
+  // run.
+  FS.Files[testPath(".clang-tidy")] = R"(
+    Checks: -*,bugprone-use-after-move,llvm-header-guard
----------------
This is confusing... I think these are now disabled twice (by disableUnusableChecks(), and via provideClangTidyFiles).

(I've got no objection to checking that elements of the standard tidy config stack work together properly, but I don't think that's what's happening in this test)

================
Comment at: clang-tools-extra/clangd/unittests/ClangdTests.cpp:1226
+  std::vector<TidyProvider> Stack;
+  Stack.push_back(provideEnvironment());
+  Stack.push_back(provideClangTidyFiles(FS));
----------------
I think we probably don't want the environment here, if it does anything at all it probably makes the test non-hermetic

================
Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:133
   auto TU = TestTU::withCode(Test.code());
-  TU.ClangTidyChecks = "-*,google-explicit-constructor";
+  TU.ClangTidyProvider = fixedTidyProvider("-*,google-explicit-constructor");
   EXPECT_THAT(
----------------
We sholudn't need -* here and elsewhere. This is the whole config provider, nothing could be turning any checks on.

================
Comment at: clang-tools-extra/clangd/unittests/TestTU.cpp:62
   Inputs.Opts = ParseOptions();
-  Inputs.Opts.ClangTidyOpts.Checks = ClangTidyChecks;
-  Inputs.Opts.ClangTidyOpts.WarningsAsErrors = ClangTidyWarningsAsErrors;
+  if (ClangTidyProvider)
+    Inputs.ClangTidyProvider = ClangTidyProvider;
----------------
I think the condition is not needed

================
Comment at: clang-tools-extra/clangd/unittests/TestTU.h:62

-  llvm::Optional<std::string> ClangTidyChecks;
-  llvm::Optional<std::string> ClangTidyWarningsAsErrors;
+  mutable TidyProvider ClangTidyProvider = {};
   // Index to use when building AST.
----------------
mutable here smells funny, can we do `using TidyProvider = unique_function<void(args) const>` instead? (so it's const-callable)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91029