Page MenuHomePhabricator

[clang-tidy] Optional inheritance of file configs from parent directories 
ClosedPublic

Authored by DmitryPolukhin on Feb 26 2020, 8:17 AM.

Details

Summary

Without this patch clang-tidy stops finding file configs on the nearest
.clang-tidy file. In some cases it is not very convenient because it
results in common parts duplication into every child .clang-tidy file.
This diff adds optional config inheritance from the parent directories
config files.

Test Plan:

Added test case in existing config test.

Diff Detail

Event Timeline

DmitryPolukhin created this revision.Feb 26 2020, 8:17 AM

Rebase on top of master

Fix issue with config inheritance and caching configs

Friendly ping, please take a look.

In large repos there is a significant problem with maintaining .clang-tidy configs in sync without some kind of inheritance with local overrides. If you have thoughts how to make it better, please speak up.

alexfh requested changes to this revision.Mar 4 2020, 5:33 AM

Internally we have something similar, but with unconditional inheritance and a way to include other configs. I was thinking about implementing this in the FileOptionsProvider, but decided that this would be an overkill for most real projects. Overall the patch looks good to me, but please update the documentation and add a test to ensure that check options get merged correctly.

clang-tools-extra/clang-tidy/ClangTidyOptions.h
125

I'd rephrase this a bit: "Only used in the FileOptionsProvider. If true, FileOptionsProvider will take a configuration file in the parent directory (if any exists) and apply this config file on top of the parent one. If false, only this configuration file will be used."

Or something like this.

This revision now requires changes to proceed.Mar 4 2020, 5:33 AM

Comments resolved, please take another look.

DmitryPolukhin marked an inline comment as done.

Also updated rst file

@alexfh could you please take another look, I added more tests and updated the doc.

How are local and global options handled.
For example in root .clang-tidy:

CheckOptions:
  - key:             some-check.GlobalOption
    value:           '1'

and in a subfolder .clang-tidy:

CheckOptions:
  - key:             GlobalOption
    value:           '0'

Should some-check read GlobalOption (with Options::getLocalOrGlobal) as:

  • 0 - from the highest level config file or
  • 1 - from the highest level local config option.

From what I can gather it will read the value as 1 as the local option is checked first no matter which file it was defined in.

Handle global options

How are local and global options handled.
From what I can gather it will read the value as 1 as the local option is checked first no matter which file it was defined in.

You are absolutely right about current behaviour. Thank you for catching this odd behaviour. I'm not 100% confident what is the right behaviour but my guess is that overriding local option from parent config with global from child folder is better so I implemented it and added corresponding test.

You are absolutely right about current behaviour. Thank you for catching this odd behaviour. I'm not 100% confident what is the right behaviour but my guess is that overriding local option from parent config with global from child folder is better so I implemented it and added corresponding test.

It's a tricky one that, but I thing overriding the local option with the global in the sub directory is the correct way to go about this as well.

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
149

May I suggest using return !Name.contains('.'); to make the code more readable.

Applied suggested changes

DmitryPolukhin marked an inline comment as done.Mar 10 2020, 9:28 AM

@alexfh could you please take another look to the diff?
All comments resolved.

You are absolutely right about current behaviour. Thank you for catching this odd behaviour. I'm not 100% confident what is the right behaviour but my guess is that overriding local option from parent config with global from child folder is better so I implemented it and added corresponding test.

It's a tricky one that, but I thing overriding the local option with the global in the sub directory is the correct way to go about this as well.

There's one more thing to consider: just by looking at the name of a local option we don't know whether it will be read using get() or getLocalOrGlobal(). By removing local options we may introduce an even more surprising behavior than without this special treatment. WDYT?

There's one more thing to consider: just by looking at the name of a local option we don't know whether it will be read using get() or getLocalOrGlobal(). By removing local options we may introduce an even more surprising behavior than without this special treatment. WDYT?

There's no perfect solution, but in general clang tidy checks use the getLocalOrGlobal method when a option is generic. There is a way to implement what you are asking for and that's storing a precedence value with each option when you load the config. Then if you call getLocalOrGlobalit will choose the option (local or global) with the highest precedence. That solution would require a little retooling of the options class but could be the best solution

There's one more thing to consider: just by looking at the name of a local option we don't know whether it will be read using get() or getLocalOrGlobal(). By removing local options we may introduce an even more surprising behavior than without this special treatment. WDYT?

As far as I can see there are no global options that can be used as local one. I found following global options: IncludeStyle|StrictMode|HeaderFileExtensions|IgnoreMacros|EnableProto|CheckFirstDeclaration
All of them used only with getLocalOrGlobal() and never with pure get() so on practice I think we shouldn't have this problem. If you still think that it is better to not remove local options, I can revert that part of my diff to the state before or implement approach that @njames93 suggested but IMHO it will be logic complication without real benefits right now. Please let me know what do you prefer.

Use options priority instead of overriding local options by global

@alexfh friendly ping
I implemented solution with priorities to resolve your concerns about get() vs getLocalOrGlobal()
Could you please take another look to this diff?

@alexfh friend ping, please take a look.

alexfh accepted this revision.Apr 13 2020, 5:15 AM

Apologies for the delay! It's sort of a crazy time now =\

The code looks mostly good now modulo a few comments inline.

clang-tools-extra/clang-tidy/ClangTidyCheck.cpp
85

supernit: Let's swap the compared values to make the code slightly more intuitive ("if local is higher priority, use local" vs "if global is lower priority, use local").

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
73

nit: would emplace_back work?

clang-tools-extra/clang-tidy/ClangTidyOptions.h
101–104

Maybe just ClangTidyValue(StringRef Value, unsigned Priority = 0)?

107

Could you describe how the priority is used?

clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
41

Does the new logic related to local and global options deserve a separate mention here?

This revision is now accepted and ready to land.Apr 13 2020, 5:15 AM
DmitryPolukhin marked 6 inline comments as done.

Comments resolved + rebase

@alexfh thank you for review!

clang-tools-extra/clang-tidy/ClangTidyOptions.h
101–104

Reduced number of c-tors to 2 with default value. We need ClangTidyValue(const char *Value) for supporting lots of constructs like Opts["cert-dcl16-c.NewSuffixes"] = "L;LL;LU;LLU"; without changing lots of code without good reason.

clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
41

I think adding global vs local here will only add more confusion and general statement about precedence still true. As far as I can see there are no оreferences to global options in user documentation.

Remove clang-format errors in diff

One more try to remove useless lint issues

Harbormaster completed remote builds in B53137: Diff 257336.

And one more time :(

This revision was automatically updated to reflect the committed changes.

Breaks Windows due to slashiness in the test: http://45.33.8.238/win/12984/step_8.txt

Please take a look, and revert if it takes a while to investigate.

(I'd find the diag easier to read without the "the": just "is disabled in")

@thakis sorry, I didn't notice the issue because of massive breakage with diff landed just be bore mine and also cmake issues on Windows bots. Thank you for trying to fix it.

@thakis, I don't see this bot on LLVM http://lab.llvm.org:8011/console
Windows bots there still fail due to cmake issues. The issue is very real and thank you for pointing out but how should I find the bot?

I didn't notice the issue because of massive breakage with diff landed just be bore mine

Sorry about that :)

As an afterthought, Would you think it a good idea to extend this logic to the ConfigOptionsProvider That way you can use something along the lines of:

clang-tidy --config='{InheritParentConfig: true, CheckOptions: [<Options overriding/complimenting .clang-tidy files>]}'

clang-tidy would then go to read the .clang-tidy configuration files in much the same was as is done here. This would let you set custom configuration options on the command line on top of whats in configuration files.
Obviously is InheritParentConfig is unset or false then we wouldn't go looking for .clang-tidy files.

As an afterthought, Would you think it a good idea to extend this logic to the ConfigOptionsProvider That way you can use something along the lines of:

clang-tidy --config='{InheritParentConfig: true, CheckOptions: [<Options overriding/complimenting .clang-tidy files>]}'

clang-tidy would then go to read the .clang-tidy configuration files in much the same was as is done here. This would let you set custom configuration options on the command line on top of whats in configuration files.
Obviously is InheritParentConfig is unset or false then we wouldn't go looking for .clang-tidy files.

It sounds logical. I would even extend it a little bit to allow case --config=@filepath where filepath file will be read and expanded as the highest priority config. @njames93 are you going to work on this?

njames93 added inline comments.Tue, Jul 28, 4:16 PM
clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
116–121

Is there a reason for incrementing the priority on each successive iteration, Seems like a bug that will lead to the later registered modules having higher priority for their options.

DmitryPolukhin added inline comments.Wed, Jul 29, 5:56 AM
clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
116–121

It seems that you are right and it is just a bug. Modules are registered as static object in different translation units so there is no guarantee about their order. But in general I didn't expect conflicting options here. If you have a diff, I'll be happy to stamp it; if not, I'll create the diff.