This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Implement clang-tidy options from config
ClosedPublic

Authored by njames93 on Nov 8 2020, 5:08 AM.

Details

Summary

Added some new ClangTidyOptionsProvider like classes designed for clangd work flow.
These providers are designed to source the options on the worker thread but in a thread safe manner.
This is done through making the options getter take a pointer to the filesystem used by the worker thread which natuarally is from a ThreadsafeFS.
Internal caching in the providers is also guarded.

The providers don't inherit from ClangTidyOptionsProvider instead they share a base class which is able to create a provider for the ClangTidyContext using a specific FileSystem.
This approach means one provider can be used for multiple contexts even though ClangTidyContext owns its provider.

Depends on D90531

Diff Detail

Event Timeline

njames93 created this revision.Nov 8 2020, 5:08 AM
njames93 requested review of this revision.Nov 8 2020, 5:08 AM
njames93 updated this revision to Diff 303720.Nov 8 2020, 7:18 AM

Fix potential race when updating cache in FileTidyProvider.

njames93 updated this revision to Diff 303721.Nov 8 2020, 7:23 AM

Fix compile errors.

njames93 updated this revision to Diff 303723.Nov 8 2020, 8:00 AM

Fix last compile error, check-clangd ran without a hitch. Testing the binary in another project seems to work ok.

njames93 updated this revision to Diff 303774.Nov 9 2020, 1:30 AM
  • Fix compile error for gcc 5.3.
  • Add back logging configuration when creating CTContext.

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?

clang-tools-extra/clangd/ClangdServer.h
374

new comment doesn't really say anything, revert to the old one?

clang-tools-extra/clangd/ParsedAST.cpp
254

why this change?

309

nit: move a few lines above to after the options-initialization if-stmt?

clang-tools-extra/clangd/TidyProvider.h
21

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?)

22

we don't usually use a "Clangd" prefix in the clangd namespace. (Clangd[LSP]Server being notable and very old exceptions)

41

how is the cache invalidated? (AFAICT currently it never is)

42

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.

75

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.

86

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.

86

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)

njames93 updated this revision to Diff 304780.Nov 12 2020, 4:33 AM
njames93 marked 4 inline comments as done.

Reworked large chunks of this:

  • Renamed ClangdTidyProvider->TidyProvider.
  • Removed the obselete interface mirroring ClangTidyOptionsProvider, it wasn't needed.
  • Incorporated the ThreadsafeFS inside the provider.
  • Removed some unnecessary changes.
  • Removed the internal caching of the ThreadsafeFileTidyProvider.
  • Trimmed down inheritance hierarchy.

Thanks for the comments, I agree it was a little too much. Likely due in part to how I first tried to mirror the interface of ClangTidyOptionsProvider.

clang-tools-extra/clangd/ParsedAST.cpp
254

So we can use the same VFS a bit further down. However as thats not a thing now it can be reverted.

309

The options for the context are only sourced once the currentFile has been set

njames93 updated this revision to Diff 304940.Nov 12 2020, 12:43 PM
  • Small tweaks

Thanks, this looks massively simpler.
It seems clear that we get config by applying a sequence of strategies in order, and these strategies (e.g. .clang-tidy files, config, disabled checks) are mostly independent.
So I have a suggestion for expressing this composition more directly in clangdmain, details in the comments.

clang-tools-extra/clangd/TidyProvider.cpp
35

it'd be nice if this class could focus on a single responsibility (adapting between interfaces) rather than also trying to apply defaults.

clang-tools-extra/clangd/TidyProvider.h
21

we're using inheritance for a couple of purposes here.

First, giving a common interface to the various types of provider.
This makes sense, though we use unique_function<ClangTidyOptions(StringRef)> for this as the type as it's essentially just one function.
This would give us the option of using lambdas instead of having TestClangTidyProvider...

The other purpose is composition: ConfigTidyProvider inherits from ThreadSafeFileTidyProvider in order to provide both sets of config.
This is awkward and constraining, e.g. we can't now enable config but not .clang-tidy (which is the desired config inside google), and more immediately the handling of overrides is awkward.
It seems more natural to have several units and compose them explicitly. The most simple way to compose them is have them be functions that mutate a ClangTidyOptions, instead of producing one.

So in its minimal form this could look like:

using TidyProvider = unique_function<void(ClangTidyOptions&, /*Filename*/llvm::StringRef)>;
TidyProvider combine(vector<TidyProvider>); 
ClangTidyOptionsProvider asClangTidyOptionsProvider(TidyProvider);

TidyProvider provideEnvironment();
TidyProvider provideDefaultChecks();
TidyProvider disableUnusableChecks(ArrayRef<std::string> OverrideChecks={});
TidyProvider provideClangdConfig();
TidyProvider provideClangTidyFiles(ThreadsafeFS&);

// In ClangdMain:
TidyProvider Tidy = combine({provideEnvironment(), provideClangTidyFiles(FS), provideClangdConfig(), provideDefaultChecks(), disableUnusableChecks());

What do you think?

25

This is just adapting this interface to another one, it doesn't need to be virtual (and in fact could be a free function...).

Maybe call it asClangTidyOptionsProvider or so? The current name makes it sound like some kind of singleton.

33

these subclasses are only constructed and used through the TidyProvider interface. I think they can be moved into the impl file and just expose factories.

46

It'd be nice to be able to customize the set of checks that are disabled. In our hosted environment, when we find a check that is crashing in production, we disable it with a command-line flag (but without a new release). This flag needs to be propagated through here somehow...

Of course if tidy providers are easy to compose, we can just add our own for this.

clang-tools-extra/clangd/unittests/TestTidyProvider.h
15 ↗(On Diff #304940)

(I think it's reasonable to put this trivial implementation into TidyProvider.h as FixedTidyProvider or so, to avoid the extra test-only files: the implementation is trivial and there's it does *make sense* as a production provider)

njames93 updated this revision to Diff 306794.Nov 20 2020, 3:50 PM
njames93 marked an inline comment as done.

Reworked everything.

njames93 updated this revision to Diff 306796.Nov 20 2020, 3:52 PM
njames93 marked 7 inline comments as done.

Messed up branches, fixed that

njames93 marked 2 inline comments as done.Nov 20 2020, 3:52 PM
njames93 added inline comments.
clang-tools-extra/clangd/TidyProvider.h
21

That is a much nicer interface

njames93 marked an inline comment as done.Nov 20 2020, 3:54 PM
njames93 updated this revision to Diff 306850.Nov 21 2020, 8:58 AM

Fix ChecksToDisable to actually disable the checks.

njames93 updated this revision to Diff 307213.Nov 23 2020, 4:28 PM

Fix assertion in provideDefaultChecks

njames93 updated this revision to Diff 307229.Nov 23 2020, 5:54 PM

Added PathRef to TidyProvider, this handles finding clang-tidy files when the compile command directory isn't the project root.
Use elog for reporting errors instead of llvm::errs - It was an artefact from the code in ClangTidyOptions.cpp.

njames93 updated this revision to Diff 307349.Nov 24 2020, 7:21 AM

Small tweaks

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...

clang-tools-extra/clangd/ParsedAST.cpp
241

or just use fixedTidyProvider("-*")

clang-tools-extra/clangd/TidyProvider.cpp
74

I'm not sure if this applies anymore - it's going to be the bottom of the stack, and nullopt is the default

75

nit: hoist the getenv out of the lambda?

75

seems like we can just assign to Opts.User directly?

99

nit: = seems clearer than emplace

110

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)

167

(as noted elsewhere, the path is always absolute in clangd and we should make this an interface requiremnet)

170

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
181

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)

194

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.

209

nit: "adapter" may be clearer than "instance", up to you

clang-tools-extra/clangd/TidyProvider.h
25

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!

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

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).

850

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!)

853

I think the condition is backwards

clang-tools-extra/clangd/unittests/ClangdTests.cpp
1221

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)

1226

I think we probably don't want the environment here, if it does anything at all it probably makes the test non-hermetic

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
133

We sholudn't need -* here and elsewhere. This is the whole config provider, nothing could be turning any checks on.

clang-tools-extra/clangd/unittests/TestTU.cpp
62

I think the condition is not needed

clang-tools-extra/clangd/unittests/TestTU.h
62

mutable here smells funny, can we do using TidyProvider = unique_function<void(args) const> instead? (so it's const-callable)

njames93 marked 12 inline comments as done and an inline comment as not done.Nov 24 2020, 11:33 AM
njames93 added inline comments.
clang-tools-extra/clangd/ParsedAST.cpp
241

Issue with that approach is fixedTidyProvider has state so that would need a stable storage when passing to asClangTidyOptionsProvider.

clang-tools-extra/clangd/TidyProvider.cpp
74

This is just incase ClangTidyOptions::getDefaults() ever changes.

75

We can't as EmptyDefaults also contains the Options clang tidy modules may want to set.

167

See previous comment about clang-tidy checks not adhering to this requirement.

170

Ditto

194

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.

clang-tools-extra/clangd/TidyProvider.h
25

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.

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

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.

853

Yes it was, good spot.

clang-tools-extra/clangd/unittests/ClangdTests.cpp
1221

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.

1226

I agree, can trim this down a little.

clang-tools-extra/clangd/unittests/TestTU.cpp
62

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.

clang-tools-extra/clangd/unittests/TestTU.h
62

I was debating between the two but that does look a little nicer.

njames93 updated this revision to Diff 307416.Nov 24 2020, 11:33 AM
njames93 marked 5 inline comments as done.

Address (most of the) comments.

sammccall added inline comments.Nov 24 2020, 3:10 PM
clang-tools-extra/clangd/ParsedAST.cpp
303–305

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.

clang-tools-extra/clangd/TidyProvider.cpp
75

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.

194

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...)

clang-tools-extra/clangd/TidyProvider.h
25

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)

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

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.

clang-tools-extra/clangd/unittests/TestTU.cpp
62

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!

njames93 marked 6 inline comments as done.Nov 24 2020, 4:35 PM
njames93 added inline comments.
clang-tools-extra/clangd/ParsedAST.cpp
303–305

Kind of how it is set already out with a DefaultOptionsProvider?

clang-tools-extra/clangd/TidyProvider.h
25

That option in IdentifierNamingCheck is itself controlled by an option, we could just subvert that option in disableUnusableChecks as a, somewhat dirty, fix.

njames93 updated this revision to Diff 307478.Nov 24 2020, 4:35 PM

Getting closer.

njames93 added inline comments.Nov 24 2020, 4:45 PM
clang-tools-extra/clangd/TidyProvider.cpp
194

(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...)

Or just give everything a high priority as default

njames93 updated this revision to Diff 307549.Nov 25 2020, 2:11 AM

Tweak disableUnusableChecks to take an ArrayRef over a vector.

OK, I would love to get this landed.
I think the simplest thing is to compute the config eagerly and load it in with DefaultOptionsProvider as you mentioned.

I might fiddle with checking the filename afterwards, but I don't think this is blocking.
Similarly I want to add the IO caching after this lands.

clang-tools-extra/clangd/ParsedAST.cpp
303–305

Right! The only wrinkle would be the use of empty options vs same options when requesting config for a different file.
But happy to go with DefaultOptionsProvider in to get this landed!

clang-tools-extra/clangd/TidyProvider.h
25

Yeah, that has slightly different behavior though: it means that it will assume every other file has the same config as the current file. (i.e. the check *is* enabled for those files, and the style is the same).
Returning an empty config seems more principled, and more likely to do the right thing if other such checks get added.

njames93 updated this revision to Diff 307565.Nov 25 2020, 2:59 AM

Use DefaultOptionsProvider for initializing ClangTidyContext.

njames93 marked 4 inline comments as done.Nov 25 2020, 3:02 AM
njames93 added inline comments.
clang-tools-extra/clangd/TidyProvider.h
25

and more likely to do the right thing if other such checks get added.

I'm going to change the identifier-naming option to make it a global. I can think of other checks in clang-tidy land that would benefit from it.

sammccall accepted this revision.Nov 25 2020, 3:34 AM

Thanks, this LG!

(The change to global makes sense but doesn't block us I think)

clang-tools-extra/clangd/TidyProvider.h
26

CWD is now effectively unused and can be dropped

This revision is now accepted and ready to land.Nov 25 2020, 3:34 AM
njames93 updated this revision to Diff 307615.Nov 25 2020, 7:46 AM

Removed duplicated logging of options.

njames93 added inline comments.Nov 25 2020, 8:27 AM
clang-tools-extra/clangd/TidyProvider.h
26

So I tried removing CWD and it leads to assertions failing in the tests(the ones with the extension .test).
I do know that the TFS has some issues in those tests anyway:

[build] E[16:04:40.948] VFS: failed to set CWD to /clangd-test: No such file or directory.

I could remove the CWD but not have assertions, instead just bail out if we encounter those cases, maybe with a log. WDYT?

sammccall added inline comments.Nov 25 2020, 8:34 AM
clang-tools-extra/clangd/TidyProvider.h
26

It's a bit hard to say without more details - what assertions/cases?

AFAICT:

  • Filename now only ever has the value passed from ParsedAST::build().
  • That should always be absolute
  • CWD is only used for the filesystem examining Filename and its parents, and if they're absolute it should be irrelevant

One of these is clearly not true, though :-)

sammccall added inline comments.Nov 25 2020, 8:43 AM
clang-tools-extra/clangd/TidyProvider.h
26

(FWIW I patched in the latest revision of this patch, deleted CWD and check-clangd passed without assertion failures)

njames93 added inline comments.Nov 25 2020, 9:01 AM
clang-tools-extra/clangd/TidyProvider.h
26

Oh I added some assertions to ensure that it was a directory, those are the ones that failed. The ones ensuring its absolute ran without a hitch.

njames93 updated this revision to Diff 307642.Nov 25 2020, 9:40 AM

Removed CWD references.
Added some fixme comments about function_ref<->unique_function and null values.
Added asserts in provideClangTidyFiles ensuring path is absolute.

sammccall accepted this revision.Nov 25 2020, 10:01 AM

Hooray, ship it!

This revision was automatically updated to reflect the committed changes.

Whoops, probably should've updated the summary before pushing, ah well.