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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:303
       E.instantiate()->addCheckFactories(CTFactories);
-    CTContext.emplace(std::make_unique<tidy::DefaultOptionsProvider>(
-        tidy::ClangTidyGlobalOptions(), Inputs.Opts.ClangTidyOpts));
+    CTContext.emplace(asClangTidyOptionsProvider(
+        Inputs.ClangTidyProvider ? Inputs.ClangTidyProvider : emptyTidyProvider,
----------------
do you think we should just directly/eagerly create the clang-tidy config here, and wrap it in a provider that always returns it?
(with a check that the requested filename matches, see other comment)

Now that I understand why CTContext is lazy, I'm not sure we're benefitting from it.


================
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
----------------
njames93 wrote:
> 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.
Oh, I missed how ClangTidyOptions::getDefaults was being used here.

This makes sense but rather than merging the defaults using a TidyProvider, I think getDefaults should be the base object that the TidyProviders mutate. I.e. in TidyProviderAdapter, when we add the single OptionsSource, it should be initialized to ClangTidyOptions::getDefaults() before having the TidyProvider applied to it.


================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:194
+    for (auto &Option : llvm::reverse(OptionStack))
+      Opts.mergeWith(Option, ++Order);
+  };
----------------
njames93 wrote:
> 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.
Any arbitrary fixed number seems like it should work here - we could have .clangd use priority 10000 and then people could use a range higher or lower if they want to support this.

Mostly it seems like this is probably a theoretical issue only and we shouldn't let it dominate the design. Customizing check options is somewhat rare, config inheritance is rare, getLocalOrGlobal is rare, downstream modifications are rare, etc. I'd rather have the simple version and add the complexity if it causes practical problems.

(If we really need to solve this at some point, we can probably do it non-intrusively by having combine() lower the priority of all config options by a bunch after each step. We'd need to make priority signed though...)


================
Comment at: clang-tools-extra/clangd/TidyProvider.h:25
+                                   /*Priority=*/unsigned &,
+                                   /*CWD*/ PathRef);
+} // namespace detail
----------------
njames93 wrote:
> 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.
Ah, learn something new and horrifying every day!

AFAICT the only thing that actually does this is is readability/IdentifierNamingCheck, which looks like it uses the style of the file where an identifier is declared to check its name.
Our intent is very much that we're running clang-tidy strictly limited to the current file. The clangd config certainly doesn't support querying config for other files.

I think we should really only expose the current file config, and return empty config when asked for any other file.
We can have the TidyConfigAdapter do this, I think.
(Slight catch: we want getConfigForFile("rel/path/to/current/file.cpp") to work, for IdentifierNamingCheck. I think if it's a relative path, we can just check whether the basename matches - shouldn't have too many matches)


================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:850
+    Providers.push_back(provideEnvironment());
+    Providers.push_back(provideClangTidyFiles(TFS));
+    if (EnableConfig)
----------------
njames93 wrote:
> 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.
Oops... this wasn't the intent - see the docstring for the flag.

In any case, fine to leave as is, we can change this separately.


================
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;
----------------
njames93 wrote:
> 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.
Oh, of course, the RHS is a "callable" type that's null at runtime, and nothing checks for that and converts to a null function_ref.
Feature request for function_ref, I suppose!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91029