Page MenuHomePhabricator

[clangd] Add clang-tidy options to config
ClosedPublic

Authored by njames93 on Oct 31 2020, 7:37 AM.

Details

Summary

First step of implementing clang-tidy configuration into clangd config.
This is just adding support for reading and verifying the clang tidy options from the config fragments.
No support is added for actually using the options within clang-tidy yet.

That will be added in a follow up as its a little more involved.

Diff Detail

Event Timeline

njames93 created this revision.Oct 31 2020, 7:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2020, 7:37 AM
njames93 requested review of this revision.Oct 31 2020, 7:37 AM
njames93 added inline comments.Oct 31 2020, 7:49 AM
clang-tools-extra/clangd/ConfigCompile.cpp
273–274

Is this the direction we want to go, the first draft paper on this seems to suggest so.

clang-tools-extra/clangd/ConfigTidyProvider.cpp
38–41 ↗(On Diff #302090)

This definitely isn't right, but I'm unsure of the correct way to get the config for the specified file.
Is it better to not have this class share the interface of ClangTidyOptionsProvider and instead have it take an interface that takes a Config and a Filename and works from there instead?

nridge added a subscriber: nridge.Oct 31 2020, 4:01 PM
njames93 updated this revision to Diff 302141.Nov 1 2020, 4:48 AM

Added unittests for reading and compiling config and fixed the bugs causing said unittests to fail.

njames93 updated this revision to Diff 302431.Nov 2 2020, 4:34 PM

Rename Tidy to ClangTidy in Fragment
Extract CheckOptions logic into a new class DynamicDictParser, this should help if config is extended to load other dictionaries where keys are unknown strings.e

njames93 updated this revision to Diff 302552.Nov 3 2020, 5:27 AM

Removed support for using clang-tidy that will happen in a follow up as its a more involved change.

njames93 retitled this revision from [clangd][WIP] Start implementing clang-tidy options into clangd config to [clangd] Add clang-tidy options to config.Nov 3 2020, 5:29 AM
njames93 edited the summary of this revision. (Show Details)
njames93 added a project: Restricted Project.
njames93 updated this revision to Diff 302818.Nov 4 2020, 4:54 AM

Rebase and make apply take reference to Params.
Fix fragments checks being applied on top of each other instead of overwriting the current config checks.

Thank you!

This design looks really good, just have some nits on comments, possible simplifications etc.

(Sorry about the delay getting to these, on it now!)

clang-tools-extra/clangd/ConfigCompile.cpp
268

nit: method name is confusing because it uses "check" as a noun where you'd expect it to be a verb (this method validates Adjuster)

Maybe appendTidyCheckSpec?

272

nit: I think this code is going to too much trouble to produce a precise error message

  • not sure "Add check item can't start with '-'" is clearer than without the word "add". Errors show the error location/range.
  • "check name" would be clearer than "check item" (even if wildcards are allowed, IMO)
  • it feels a bit weird to be really specific about - and , - these are references to a fixed list of checks, so all that really matters is that it's not the name of a check. I'd consider just returning fixed "Invalid clang-tidy check name" for both cases
273–274

Yeah it looks like a good solution to me. It's not as powerful as an ordered list of globs, but it's simple and consistent with the other settings we have. .clang-tidy is still the right thing for full control.

275

Str.startswith("-") - we haven't checked that it's empty (and shouldn't bother IMO)

301

nit: I'd suggest *including" the leading comma in these
e.g. Checks is ",foo,bar". (Maybe call it ChecksSuffix)

Then when applying,
C.ClangTidy.Checks.append(Checks, C.ClangTidy.Checks.empty() ? /*skip comma*/1 : 0);

advantages:

  • marginally simplifies checkAdjuster (no need to conditionally add the comma)
  • the Apply step (which is somewhat performance-sensitive) is at most a single allocation (which is awkward to achieve otherwise)
clang-tools-extra/clangd/ConfigFragment.h
178

nit: drop "this section" and add trailing period, consistent with other sections.

I think "code" would be clearer than codebase.

183

I wonder if it's worth having this when we support Remove: *.

technically this does something slightly different:

  • it still runs the machinery just with no actual checks
  • you can enable: true later to override it, without losing the previously configured list of checks

Is one of these important? Is there some other benefit?
(I'm not opposed to having this if you want it, but I'm curious)

184

nit: we should document each separately.

e.g. "/// List of checks to enable - can be wildcards like "readability-*".

185

nit: "get ran" is confusing - this doesn't affect the order in which checks *run*, just which overrrides which.
And this text/example could probably be a little more compact.

I'd suggest:

List of checks to disable.
Takes precedence over Add. To enable all llvm checks except include order:
  Add: llvm-*
  Remove: llvm-include-onder
188

should this be llvm-*?

clang-tools-extra/clangd/ConfigYAML.cpp
192

instead of adding a second class for this, can we reuse DictParser, and change unrecognized() so:

  • the callback gets access to the key location and the value
  • the callback is responsible for emitting the "unknown key" error if it wants one

i.e. the default unrecognized handler is:

[this](Located<std::string> Key, Node &Value) { Outer->warning("Unknown " + Description + " key " + *Key); }

and we replace it for parsing CheckOptions

239

can we implement this on top of scalarValue? seems like it would avoid a bunch of repetition and the efficiency doesn't seem that important

243

not sure why we're trimming here

250

Technically we're probably supposed to accept
y|Y|yes|Yes|YES|n|N|no|No|NO|true|True|TRUE|false|False|FALSE|on|On|ON|off|Off|OFF
(https://yaml.org/type/bool.html)

I'd be happy accepting y/n/yes/no/true/false/on/off case-insensitively though :-)

255

conversely, not sure why we'd accept a number here

njames93 marked 13 inline comments as done.Nov 4 2020, 12:24 PM
njames93 added inline comments.
clang-tools-extra/clangd/ConfigFragment.h
183

I'm not 100% sure what you are asking here.

clang-tools-extra/clangd/ConfigYAML.cpp
192

Not sure I'm a huge fan of that approach. I can't imagine a use case where the 2 dictionary modes will be used at the same time so there isn't a need for it to support both.
By that I mean you wont have a dictionary where its expected to have both known and unknown keys.

DictParser could well be implemented as a a specialisation of DynamicDictParser but not much would be gained from that.

njames93 updated this revision to Diff 302935.Nov 4 2020, 12:24 PM

Address comments

sammccall accepted this revision.Fri, Nov 6, 1:44 AM

Thanks! Just some simplifications and doc nits left, then please go ahead and land

clang-tools-extra/clangd/Config.h
75

nit: we're using triple slash comments for these...

78

Enable is trivial enough to go without documentation, but the format of Checks certainly needs to be documented.

79

I think this should be a StringMap<string>

It makes sense to use a vector-of-pairs in ConfigFragment to preserve the Located information for keys, but we don't need to do that in Config.

clang-tools-extra/clangd/ConfigFragment.h
183

I'm asking whether we really need Enable, or whether we should remove it and recommend Remove: * to disable the checks.

If there isn't a clear reason we need it, my preference is to omit it for simplicity (we can add more setting later, but it's harder to remove them).

I don't feel strongly though, this is up to you.

clang-tools-extra/clangd/ConfigYAML.cpp
192

Yes, they're being used for different purposes, so using the same class isn't the most expressive.
But this isn't a public API, it's a local helper. The implementation is nontrivial and almost identical. This is just about reducing the implementation complexity.

239

You've extracted a common getScalar function here instead, I think to avoid scalarValue copying the string?
But this isn't a hot path, and all the legal values for this string are in SSO range anyway - can we avoid this extra complexity?

This revision is now accepted and ready to land.Fri, Nov 6, 1:44 AM
njames93 added inline comments.Fri, Nov 6, 11:27 AM
clang-tools-extra/clangd/Config.h
79

If we do use a StringMap<string>, whats the policy when multiple fragments specify the same option.
Currently at the Fragment level there is logic that ignores duplicated keys, but when compiling the fragments, should we give priority to keys declared in earlier or later fragments. In any case would it also be wise to warn about that.

clang-tools-extra/clangd/ConfigFragment.h
183

If we use Remove: * when it comes to actually implementing this, it would be wise to disable running clang-tidy if there are no checks enabled. But yes, I do agree that it could be removed at first, then added at a later date if needs must.

clang-tools-extra/clangd/ConfigYAML.cpp
192

Ok I'll update it.

239

It's not about that, I'd prefer the warning message to explicitly say it needs a boolean even if they pass something that's not a scalar. Reusing scalarValue will give a warning saying SomeKey should be scalar. What we actually want is a warning saying SomeKey should be a boolean

sammccall added inline comments.Fri, Nov 6, 2:11 PM
clang-tools-extra/clangd/Config.h
79

We merge with the later fragments taking priority. So in this case each fragment writes its keys into the single stringmap, replacing whatever is there.

(Most common case of later fragments is subdirectory overriding parent, or user config overriding in-tree).

I don't think we need to warn for this - does clang-tidy warn if there are multiple options sources configured and one overrides a setting from another? This is the same thing.

clang-tools-extra/clangd/ConfigFragment.h
183

This sounds like a good idea to me. This would happen in ParsedAST::build.
If the loop through CTChecks never actually manages to call registerPPCallbacks/Matchers, then we can skip the matchAST call.

I'll send a patch.

njames93 updated this revision to Diff 303596.Fri, Nov 6, 5:28 PM
njames93 marked an inline comment as done.

Removed Enable.
Removed the scalarBool parser as it was only needed by Enable.
Removed DynamicDictParser in favour of reworking DictParser to get better control of how to handle unknown keys.
Use a StringMap to store the CheckOptions in the Config.

njames93 marked 9 inline comments as done.Fri, Nov 6, 5:30 PM
njames93 updated this revision to Diff 303598.Fri, Nov 6, 5:36 PM

Fix the unit tests.

This revision was landed with ongoing or failed builds.Sun, Nov 22, 2:04 AM
This revision was automatically updated to reflect the committed changes.