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

[DmitryPolukhin]

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.

[DmitryPolukhin]

Rebase on top of master

[DmitryPolukhin]

Fix issue with config inheritance and caching configs

[DmitryPolukhin]

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]

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

111

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.

[DmitryPolukhin]

Comments resolved, please take another look.

[DmitryPolukhin]

Also updated rst file

[DmitryPolukhin]

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

[njames93]

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.

[DmitryPolukhin]

Handle global options

[DmitryPolukhin]

In D75184#1912650, @njames93 wrote:

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.

[njames93]

In D75184#1914705, @DmitryPolukhin wrote:

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

144

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

[DmitryPolukhin]

Applied suggested changes

[DmitryPolukhin]

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

[alexfh]

In D75184#1914788, @njames93 wrote:

In D75184#1914705, @DmitryPolukhin wrote:

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?

[njames93]

In D75184#1932764, @alexfh wrote:

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

[DmitryPolukhin]

In D75184#1932764, @alexfh wrote:

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.

[DmitryPolukhin]

Use options priority instead of overriding local options by global

[DmitryPolukhin]

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

[DmitryPolukhin]

@alexfh friend ping, please take a look.

[alexfh]

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

52 ↗

(On Diff #254238)

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

74

nit: would emplace_back work?

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

99–102

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

105

Could you describe how the priority is used?

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

41 ↗

(On Diff #254238)

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

[DmitryPolukhin]

Comments resolved + rebase

[DmitryPolukhin]

@alexfh thank you for review!

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

99–102

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 ↗

(On Diff #254238)

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.

[DmitryPolukhin]

Remove clang-format errors in diff

[DmitryPolukhin]

One more try to remove useless lint issues

[DmitryPolukhin]

And one more time :(

[thakis]

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]

i attemtped a fix in 99e4061bd800a525ce1d7e2cbd6672b2fe0ec33d

[DmitryPolukhin]

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

[DmitryPolukhin]

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

[njames93]

In D75184#1984565, @DmitryPolukhin wrote:

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

Sorry about that :)

[njames93]

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.

[DmitryPolukhin]

In D75184#2093089, @njames93 wrote:

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]

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

113–118

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]

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

113–118

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.

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

struct ClangTidyOptions {
   ///
   /// This option is used, for example, to place the correct user name in TODO()
   /// comments in the relevant check.
   llvm::Optional<std::string> User;
 
   typedef std::pair<std::string, std::string> StringPair;
   typedef std::map<std::string, std::string> OptionMap;
 
   /// Key-value mapping used to store check-specific options.
   OptionMap CheckOptions;
 
   typedef std::vector<std::string> ArgList;
 
   /// Add extra compilation arguments to the end of the list.
   llvm::Optional<ArgList> ExtraArgs;
 
   /// Add extra compilation arguments to the start of the list.
   llvm::Optional<ArgList> ExtraArgsBefore;
+
+  /// Config flag for FileOptionsProvider to also include config from parent
+  /// dir.
+  llvm::Optional<bool> InheritParentConfig;
 };
 
 /// Abstract interface for retrieving various ClangTidy options.
 class ClangTidyOptionsProvider {
 public:
   static const char OptionsSourceTypeDefaultBinary[];
   static const char OptionsSourceTypeCheckCommandLineOption[];
   static const char OptionsSourceTypeConfigCommandLineOption[];

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

 };
 
 struct NOptionMap {
   NOptionMap(IO &) {}
   NOptionMap(IO &, const ClangTidyOptions::OptionMap &OptionMap)
       : Options(OptionMap.begin(), OptionMap.end()) {}
   ClangTidyOptions::OptionMap denormalize(IO &) {
     ClangTidyOptions::OptionMap Map;
     for (const auto &KeyValue : Options)
       Map[KeyValue.first] = KeyValue.second;
     return Map;
   }
   std::vector<ClangTidyOptions::StringPair> Options;
 };
 
 template <> struct MappingTraits<ClangTidyOptions> {
   static void mapping(IO &IO, ClangTidyOptions &Options) {
     MappingNormalization<NOptionMap, ClangTidyOptions::OptionMap> NOpts(
         IO, Options.CheckOptions);
     bool Ignored = false;
     IO.mapOptional("Checks", Options.Checks);
     IO.mapOptional("WarningsAsErrors", Options.WarningsAsErrors);
     IO.mapOptional("HeaderFilterRegex", Options.HeaderFilterRegex);
     IO.mapOptional("AnalyzeTemporaryDtors", Ignored); // legacy compatibility
     IO.mapOptional("FormatStyle", Options.FormatStyle);
     IO.mapOptional("User", Options.User);
     IO.mapOptional("CheckOptions", NOpts->Options);
     IO.mapOptional("ExtraArgs", Options.ExtraArgs);
     IO.mapOptional("ExtraArgsBefore", Options.ExtraArgsBefore);
+    IO.mapOptional("InheritParentConfig", Options.InheritParentConfig);
   }
 };
 
 } // namespace yaml
 } // namespace llvm
 
 namespace clang {
 namespace tidy {
 
 ClangTidyOptions ClangTidyOptions::getDefaults() {
   ClangTidyOptions Options;
   Options.Checks = "";
   Options.WarningsAsErrors = "";
   Options.HeaderFilterRegex = "";
   Options.SystemHeaders = false;
   Options.FormatStyle = "none";
   Options.User = llvm::None;
   for (ClangTidyModuleRegistry::iterator I = ClangTidyModuleRegistry::begin(),
                                          E = ClangTidyModuleRegistry::end();
        I != E; ++I)
     Options = Options.mergeWith(I->instantiate()->getModuleOptions());
   return Options;
 }
 
 template <typename T>
 static void mergeVectors(Optional<T> &Dest, const Optional<T> &Src) {
   if (Src) {
     if (Dest)
       Dest->insert(Dest->end(), Src->begin(), Src->end());
     else
       Dest = Src;

 template <typename T>
 static void overrideValue(Optional<T> &Dest, const Optional<T> &Src) {
   if (Src)
     Dest = Src;
 }
 
 ClangTidyOptions
 ClangTidyOptions::mergeWith(const ClangTidyOptions &Other) const {
   ClangTidyOptions Result = *this;
 
   mergeCommaSeparatedLists(Result.Checks, Other.Checks);
   mergeCommaSeparatedLists(Result.WarningsAsErrors, Other.WarningsAsErrors);
   overrideValue(Result.HeaderFilterRegex, Other.HeaderFilterRegex);
   overrideValue(Result.SystemHeaders, Other.SystemHeaders);
   overrideValue(Result.FormatStyle, Other.FormatStyle);
   overrideValue(Result.User, Other.User);
   mergeVectors(Result.ExtraArgs, Other.ExtraArgs);
FileOptionsProvider::getRawOptions(StringRef FileName) {
 
   if (FS->makeAbsolute(AbsoluteFilePath))
     return {};
 
   std::vector<OptionsSource> RawOptions =
       DefaultOptionsProvider::getRawOptions(AbsoluteFilePath.str());
   OptionsSource CommandLineOptions(OverrideOptions,
                                    OptionsSourceTypeCheckCommandLineOption);
+  size_t FirstFileConfig = RawOptions.size();
   // Look for a suitable configuration file in all parent directories of the
   // file. Start with the immediate parent directory and move up.
   StringRef Path = llvm::sys::path::parent_path(AbsoluteFilePath.str());
   for (StringRef CurrentPath = Path; !CurrentPath.empty();
        CurrentPath = llvm::sys::path::parent_path(CurrentPath)) {
     llvm::Optional<OptionsSource> Result;
 
     auto Iter = CachedOptions.find(CurrentPath);
if (Result) {
         LLVM_DEBUG(llvm::dbgs()
                    << "Caching configuration for path " << Path << ".\n");
         CachedOptions[Path] = *Result;
         Path = llvm::sys::path::parent_path(Path);
       }
       CachedOptions[Path] = *Result;
 
       RawOptions.push_back(*Result);
+      if (!Result->first.InheritParentConfig ||
+          !*Result->first.InheritParentConfig)
         break;
     }
   }
+  // Reverse order of file configs because closer configs should have higher
+  // priority.
+  std::reverse(RawOptions.begin() + FirstFileConfig, RawOptions.end());
   RawOptions.push_back(CommandLineOptions);
   return RawOptions;
 }
 
 llvm::Optional<OptionsSource>
 FileOptionsProvider::tryReadConfigFile(StringRef Directory) {
   assert(!Directory.empty());
 

clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-files/3/.clang-tidy

+InheritParentConfig: true
+Checks: 'from-child3'
+HeaderFilterRegex: 'child3'

clang-tools-extra/test/clang-tidy/infrastructure/config-files.cpp

 // RUN: clang-tidy -dump-config %S/Inputs/config-files/- -- | FileCheck %s -check-prefix=CHECK-BASE
 // CHECK-BASE: Checks: {{.*}}from-parent
 // CHECK-BASE: HeaderFilterRegex: parent
 // RUN: clang-tidy -dump-config %S/Inputs/config-files/1/- -- | FileCheck %s -check-prefix=CHECK-CHILD1
 // CHECK-CHILD1: Checks: {{.*}}from-child1
 // CHECK-CHILD1: HeaderFilterRegex: child1
 // RUN: clang-tidy -dump-config %S/Inputs/config-files/2/- -- | FileCheck %s -check-prefix=CHECK-CHILD2
 // CHECK-CHILD2: Checks: {{.*}}from-parent
 // CHECK-CHILD2: HeaderFilterRegex: parent
+// RUN: clang-tidy -dump-config %S/Inputs/config-files/3/- -- | FileCheck %s -check-prefix=CHECK-CHILD3
+// CHECK-CHILD3: Checks: {{.*}}from-parent,from-child3
+// CHECK-CHILD3: HeaderFilterRegex: child3
 // RUN: clang-tidy -dump-config -checks='from-command-line' -header-filter='from command line' %S/Inputs/config-files/- -- | FileCheck %s -check-prefix=CHECK-COMMAND-LINE
 // CHECK-COMMAND-LINE: Checks: {{.*}}from-parent,from-command-line
 // CHECK-COMMAND-LINE: HeaderFilterRegex: from command line