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

[Nathan James via Phabricator via cfe-commits]

njames93 marked 12 inline comments as done and an inline comment as not done.
njames93 added inline comments.


================
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) {}
----------------
sammccall wrote:
> or just use fixedTidyProvider("-*")
Issue with that approach is fixedTidyProvider has state so that would need a stable storage when passing to `asClangTidyOptionsProvider`.


================
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");
----------------
sammccall wrote:
> I'm not sure if this applies anymore - it's going to be the bottom of the stack, and nullopt is the default
This is just incase `ClangTidyOptions::getDefaults()` ever changes.


================
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:
> sammccall wrote:
> > nit: hoist the getenv out of the lambda?
> seems like we can just assign to Opts.User directly?
We can't as `EmptyDefaults` also contains the Options clang tidy modules may want to set.


================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:167
+
+    if (FS->makeAbsolute(AbsolutePath))
+      return;
----------------
sammccall wrote:
> (as noted elsewhere, the path is always absolute in clangd and we should make this an interface requiremnet)
See previous comment about clang-tidy checks not adhering to this requirement.


================
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);
----------------
sammccall wrote:
> 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
Ditto


================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:194
+    for (auto &Option : llvm::reverse(OptionStack))
+      Opts.mergeWith(Option, ++Order);
+  };
----------------
sammccall wrote:
> 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.
That approach is a simplification but it only makes sense in how we are using the providers in clangd. Downstream someone may want to add another provider that may get ran after `.clangd` providers. Forcing `.clangd` to have the highest priority will result in unexpected behaviour in that instance.
I'm happy to go with your approach if you don't see it as a blocking issue.


================
Comment at: clang-tools-extra/clangd/TidyProvider.h:25
+                                   /*Priority=*/unsigned &,
+                                   /*CWD*/ PathRef);
+} // namespace detail
----------------
sammccall wrote:
> 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!
While clangd may always use absolute paths, clang-tidy may not. Checks are able to query the context and get options for other files.
When I put asserts on the Filename being absolute, things started failing.


================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:850
+    Providers.push_back(provideEnvironment());
+    Providers.push_back(provideClangTidyFiles(TFS));
+    if (EnableConfig)
----------------
sammccall wrote:
> 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!)
The old behaviour, and the behaviour of the `-checks` option in clang-tidy is to append the checks specified on the command line to whats found in .clang-tidy files. This is so that the checks will respect other fields in the configuration.


================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:853
+      Providers.push_back(provideClangdConfig());
+    if (ClangTidyChecks.empty())
+      Providers.push_back(fixedTidyProvider(ClangTidyChecks));
----------------
sammccall wrote:
> I think the condition is backwards
Yes it was, good spot.


================
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
----------------
sammccall wrote:
> 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)
`provideClangTidyFiles` here will enable use-after-move/header-guard but disable all other checks, which is what we want.
Hopefully `disableUnusableChecks` will then disable those 2 checks and all will work well.


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


================
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;
----------------
sammccall wrote:
> I think the condition is not needed
I think it is, the operator= for function_ref doesn't seem to correctly handle the case when the argument doesn't hold a callable. Well asserts were failing before I added the condition, may be a bug somewhere else.


================
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.
----------------
sammccall wrote:
> mutable here smells funny, can we do `using TidyProvider = unique_function<void(args) const>` instead? (so it's const-callable)
I was debating between the two but that does look a little nicer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91029