[clang-tidy] Extend CheckOptions to support grouping checks options

[njames93]

Summary

Remove a lot of duplication in configuration files by supporting nested dictionaries in the CheckOptions entry

For Example to set ParameterCase, VariableCase and MemberCase in readability-identifier-naming this new syntax is supported

CheckOptions:
  readability-identifier-naming:
    ParameterCase: CamelCase
    VariableCase: camelBack
    MemberCase: UPPER_CASE

This maintains backwards compatability with the old method of input:

CheckOptions:
  readability-identifier-naming.ParameterCase: CamelCase

And:

CheckOptions:
  - key: readability-identifier-naming.ParameterCase
    value: CamelCase

This new syntax is also able to be interleaved with the current dictionary type input.

CheckOptions:
  readability-identifier-naming.ParameterCase: CamelCase
  readability-identifier-naming:
    VariableCase: camelBack
    MemberCase: UPPER_CASE

Typically the current dictionary syntax is meant for checks with only one configured option

The -dump-config option has been updated to use the grouped syntax for checks with more than one option(while also having a deterministic ordering)

[PiotrZSL]

There are so many ways of defining check configurations, and I never heard of them.
Maybe some separate section in documentation is needed for them.

[njames93]

In D147955#4256649, @PiotrZSL wrote:

There are so many ways of defining check configurations, and I never heard of them.
Maybe some separate section in documentation is needed for them.

The current documentation only defines this method

CheckOptions:
  <check_name.option_name>: <option_value>

It doesn't mention flow syntax {CheckOptions: {<check_name.option_name>: <option_value>, ...}} because thats implicit when using yaml.
The other syntax of using

CheckOptions:
  - key: <check_name.option_name>
    value: <option_value>
  - key: ...
    value: ...

And its Flow counterpart {CheckOptions: [{key: <check_option.option_name>, value: <option_value>}, {...} ]} were removed from the documentation as an old deprecated format, left for compatibility with old config files

[carlosgalvezp]

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

94–95

This function has become very large and hard to read. Would it be possible to split it into subfunctions so it's easier to follow? I think it would make sense to create 1 function per different format supported (I believe with this patch there's now 3 different formats supported).

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

ClangTidyOptions::OptionMap denormalize(IO &) {
     return Map;
   }
   std::vector<ClangTidyOptions::StringPair> Options;
 };
 
 template <>
 void yamlize(IO &IO, ClangTidyOptions::OptionMap &Options, bool,
              EmptyContext &Ctx) {
-  if (IO.outputting()) {
-    IO.beginMapping();
-    // Only output as a map
-    for (auto &Key : Options) {
-      bool UseDefault;
-      void *SaveInfo;
-      IO.preflightKey(Key.getKey().data(), true, false, UseDefault, SaveInfo);
-      StringRef S = Key.getValue().Value;
+
+  bool UseDefault2;
+  void *SaveInfo2;
+  if (IO.outputting()) {
+    // Nested option names need to be converted to null terminated strings for
+    // yaml serialization
+    llvm::SmallString<64> Buffer;
+    auto ToCString = [&Buffer](StringRef Str) {
+      Buffer.assign(Str);
+      return Buffer.c_str();
+    };
+    IO.beginMapping();
+    // Using a map here means we have a deterministic order
+    std::map<StringRef, std::vector<std::pair<StringRef, StringRef>>>
+        LocalItems;
+    SmallVector<std::pair<StringRef, StringRef>, 8> GlobalItems;
+    for (auto &Entry : Options) {
+      auto Key = Entry.getKey();
+      // Look for a '.' seperator in the key name
+      auto Idx = Key.find('.');
+      if (Idx == StringRef::npos) {
+        GlobalItems.emplace_back(Key, Entry.getValue().Value);
+      } else {
+        LocalItems[Key.take_front(Idx)].emplace_back(Key.drop_front(Idx + 1),
+                                                     Entry.getValue().Value);
+      }
+    }
+    llvm::sort(GlobalItems,
+               [](auto &Lhs, auto &Rhs) { return Lhs.first < Rhs.first; });
+    for (auto &Item : GlobalItems) {
+      IO.preflightKey(Item.first.data(), true, false, UseDefault, SaveInfo);
+      StringRef S = Item.second;
       IO.scalarString(S, needsQuotes(S));
       IO.postflightKey(SaveInfo);
     }
+    for (auto &Item : LocalItems) {
+      // If a check only has 1 option, Emit it directly as a scalar.
+      if (Item.second.size() == 1) {
+        // This is a bit of a hack as Item.first only contains the check name,
+        // but the null terminated buffer it references will contain the whole
+        // option name.
+        IO.preflightKey(Item.first.data(), true, false, UseDefault, SaveInfo);
+        StringRef S = Item.second.front().second;
+        IO.scalarString(S, needsQuotes(S));
+        IO.postflightKey(SaveInfo);
+        continue;
+      }
+      // Sort all of the checks options for nicer output
+      llvm::sort(Item.second,
+                 [](auto &Lhs, auto &Rhs) { return Lhs.first < Rhs.first; });
+      // The stringref in Item.first is not null terminated, so we need to copy
+      // to a null terminated buffer
+      IO.preflightKey(ToCString(Item.first), true, false, UseDefault, SaveInfo);
+      IO.beginMapping();
+      for (auto &SubItem : Item.second) {
+        IO.preflightKey(SubItem.first.data(), true, false, UseDefault2,
+                        SaveInfo2);
+        StringRef S = SubItem.second;
+        IO.scalarString(S, needsQuotes(S));
+        IO.postflightKey(SaveInfo2);
+      }
+      IO.endMapping();
+      IO.postflightKey(SaveInfo);
+    }
     IO.endMapping();
   } else {
     // We need custom logic here to support the old method of specifying check
     // options using a list of maps containing key and value keys.
     Input &I = reinterpret_cast<Input &>(IO);
     if (isa<SequenceNode>(I.getCurrentNode())) {
       MappingNormalization<NOptionMap, ClangTidyOptions::OptionMap> NOpts(
           IO, Options);
       EmptyContext Ctx;
       yamlize(IO, NOpts->Options, true, Ctx);
     } else if (isa<MappingNode>(I.getCurrentNode())) {
       IO.beginMapping();
+      llvm::SmallString<128> Buffer;
       for (StringRef Key : IO.keys()) {
-        IO.mapRequired(Key.data(), Options[Key].Value);
+        IO.preflightKey(Key.data(), true, false, UseDefault, SaveInfo);
+        if (isa<ScalarNode, BlockScalarNode>(I.getCurrentNode())) {
+          yamlize(IO, Options[Key].Value, true, Ctx);
+        } else if (isa<MappingNode>(I.getCurrentNode())) {
+          IO.beginMapping();
+          Buffer.assign({Key, "."});
+          auto Size = Buffer.size();
+          for (StringRef Key : IO.keys()) {
+            IO.preflightKey(Key.data(), true, false, UseDefault2, SaveInfo2);
+            Buffer.truncate(Size);
+            Buffer.append(Key);
+            yamlize(I, Options[Buffer].Value, true, Ctx);
+            IO.postflightKey(SaveInfo2);
+          }
+          IO.endMapping();
+        } else {
+          IO.setError("Expected a scalar or a map");
+        }
+        IO.postflightKey(SaveInfo);
       }
       IO.endMapping();
     } else {
       IO.setError("expected a sequence or map");
     }
   }
 }
 

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

Configuration files:
   precedence.
 
   The following configuration options may be used in a .clang-tidy file:
 
   CheckOptions                 - List of key-value pairs defining check-specific
                                  options. Example:
                                    CheckOptions:
                                      some-check.SomeOption: 'some value'
+                                     multi-option-check:
+                                       Option1: 'option_1_value'
+                                       Option2: 'option_2_value'
   Checks                       - Same as '--checks'.
   ExtraArgs                    - Same as '--extra-args'.
   ExtraArgsBefore              - Same as '--extra-args-before'.
   FormatStyle                  - Same as '--format-style'.
   HeaderFileExtensions         - File extensions to consider to determine if a
                                  given diagnostic is located in a header file.
   HeaderFilterRegex            - Same as '--header-filter-regex'.
   ImplementationFileExtensions - File extensions to consider to determine if a

clang-tools-extra/docs/ReleaseNotes.rst

 
 Improvements to clang-tidy
 --------------------------
 
 - New global configuration file options `HeaderFileExtensions` and
   `ImplementationFileExtensions`, replacing the check-local options of the
   same name.
 
+- Ability to group `CheckOptions` for a specific check in the `.clang-tidy`
+  configuration file.
+
 New checks
 ^^^^^^^^^^
 
 - New :doc:`bugprone-unsafe-functions
   <clang-tidy/checks/bugprone/unsafe-functions>` check.
 
   Checks for functions that have safer, more secure replacements available, or
   are considered deprecated due to design flaws.

clang-tools-extra/docs/clang-tidy/index.rst

Configuration files:
     precedence.
 
     The following configuration options may be used in a .clang-tidy file:
 
     CheckOptions                 - List of key-value pairs defining check-specific
                                    options. Example:
                                      CheckOptions:
                                        some-check.SomeOption: 'some value'
+                                       multi-option-check:
+                                         Option1: 'option_1_value'
+                                         Option2: 'option_2_value'
     Checks                       - Same as '--checks'.
     ExtraArgs                    - Same as '--extra-args'.
     ExtraArgsBefore              - Same as '--extra-args-before'.
     FormatStyle                  - Same as '--format-style'.
     HeaderFileExtensions         - File extensions to consider to determine if a
                                    given diagnostic is located in a header file.
     HeaderFilterRegex            - Same as '--header-filter-regex'.
     ImplementationFileExtensions - File extensions to consider to determine if a

clang-tools-extra/test/clang-tidy/checkers/google/module.cpp

 // RUN: clang-tidy -checks='-*,google*' -config='{}' -dump-config - -- | FileCheck %s
 // CHECK: CheckOptions:
-// CHECK-DAG: {{google-readability-braces-around-statements.ShortStatementLines: '1'}}
-// CHECK-DAG: {{google-readability-function-size.StatementThreshold: '800'}}
-// CHECK-DAG: {{google-readability-namespace-comments.ShortNamespaceLines: '10'}}
-// CHECK-DAG: {{google-readability-namespace-comments.SpacesBeforeComments: '2'}}
+// CHECK: {{  google-readability-braces-around-statements.ShortStatementLines: '1'}}
+// CHECK: {{  google-readability-function-size:}}
+// CHECK: {{    StatementThreshold: '800'}}
+// CHECK: {{  google-readability-namespace-comments:}}
+// CHECK-NEXT: {{    ShortNamespaceLines: '10'}}
+// CHECK-NEXT: {{    SpacesBeforeComments: '2'}}

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

 InheritParentConfig: true
 Checks: 'llvm-qualified-auto'
 CheckOptions:
   - key:             modernize-loop-convert.MaxCopySize
     value:           '20'
   - key:             llvm-qualified-auto.AddConstToQualified
     value:           'true'
   - key:             IgnoreMacros
     value:           'false'
+  - key:             modernize-loop-convert.MakeReverseRangeFunction
+    value:           Reverse
+  - key:             modernize-loop-convert.UseCxx20ReverseRanges
+    value:           false

clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-files/4/key-dict/.clang-tidy

 InheritParentConfig: true
 Checks: 'llvm-qualified-auto'
 CheckOptions:
   modernize-loop-convert.MaxCopySize: '20'
   llvm-qualified-auto.AddConstToQualified: 'true'
   IgnoreMacros: 'false'
+  modernize-loop-convert: 
+    MakeReverseRangeFunction: Reverse
+    UseCxx20ReverseRanges: false
 

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

 
 // For this test we have to use names of the real checks because otherwise values are ignored.
 // Running with the old key: <Key>, value: <value> CheckOptions
 // RUN: clang-tidy -dump-config %S/Inputs/config-files/4/44/- -- | FileCheck %s -check-prefix=CHECK-CHILD4
 // Running with the new <key>: <value> syntax
 // RUN: clang-tidy -dump-config %S/Inputs/config-files/4/key-dict/- -- | FileCheck %s -check-prefix=CHECK-CHILD4
 
 // CHECK-CHILD4: Checks: {{.*}}modernize-loop-convert,modernize-use-using,llvm-qualified-auto
-// CHECK-CHILD4-DAG: llvm-qualified-auto.AddConstToQualified: 'true'
-// CHECK-CHILD4-DAG: modernize-loop-convert.MaxCopySize: '20'
-// CHECK-CHILD4-DAG: modernize-loop-convert.MinConfidence: reasonable
-// CHECK-CHILD4-DAG: modernize-use-using.IgnoreMacros: 'false'
+// CHECK-CHILD4: llvm-qualified-auto.AddConstToQualified: 'true'
+// CHECK-CHILD4: modernize-loop-convert:
+// CHECK-CHILD4-NEXT:   IncludeStyle: llvm
+// CHECK-CHILD4-NEXT:   MakeReverseRangeFunction: Reverse
+// CHECK-CHILD4-NEXT:   MakeReverseRangeHeader: ''
+// CHECK-CHILD4-NEXT:   MaxCopySize: '20'
+// CHECK-CHILD4-NEXT:   MinConfidence: reasonable
+// CHECK-CHILD4: modernize-use-using.IgnoreMacros: 'false'
 
 // RUN: clang-tidy --explain-config %S/Inputs/config-files/4/44/- -- | FileCheck %s -check-prefix=CHECK-EXPLAIN
 // CHECK-EXPLAIN: 'llvm-qualified-auto' is enabled in the {{.*}}{{[/\\]}}Inputs{{[/\\]}}config-files{{[/\\]}}4{{[/\\]}}44{{[/\\]}}.clang-tidy.
 // CHECK-EXPLAIN: 'modernize-loop-convert' is enabled in the {{.*}}{{[/\\]}}Inputs{{[/\\]}}config-files{{[/\\]}}4{{[/\\]}}.clang-tidy.
 // CHECK-EXPLAIN: 'modernize-use-using' is enabled in the {{.*}}{{[/\\]}}Inputs{{[/\\]}}config-files{{[/\\]}}4{{[/\\]}}.clang-tidy.
 
 // RUN: clang-tidy -dump-config \
 // RUN: --config='{InheritParentConfig: true, \
 // RUN: Checks: -llvm-qualified-auto, \
 // RUN: CheckOptions: [{key: modernize-loop-convert.MaxCopySize, value: 21}]}' \
 // RUN: %S/Inputs/config-files/4/44/- -- | FileCheck %s -check-prefix=CHECK-CHILD5
 // Also test with the {Key: Value} Syntax specified on command line
 // RUN: clang-tidy -dump-config \
 // RUN: --config='{InheritParentConfig: true, \
 // RUN: Checks: -llvm-qualified-auto, \
 // RUN: CheckOptions: {modernize-loop-convert.MaxCopySize: 21}}' \
 // RUN: %S/Inputs/config-files/4/44/- -- | FileCheck %s -check-prefix=CHECK-CHILD5
 
 // CHECK-CHILD5: Checks: {{.*}}modernize-loop-convert,modernize-use-using,llvm-qualified-auto,-llvm-qualified-auto
-// CHECK-CHILD5-DAG: modernize-loop-convert.MaxCopySize: '21'
-// CHECK-CHILD5-DAG: modernize-loop-convert.MinConfidence: reasonable
-// CHECK-CHILD5-DAG: modernize-use-using.IgnoreMacros: 'false'
+// CHECK-CHILD5: modernize-loop-convert:
+// CHECK-CHILD5: MaxCopySize: '21'
+// CHECK-CHILD5: MinConfidence: reasonable
+// CHECK-CHILD5: modernize-use-using.IgnoreMacros: 'false'
 
 // RUN: clang-tidy -dump-config \
 // RUN: --config='{InheritParentConfig: false, \
 // RUN: Checks: -llvm-qualified-auto}' \
 // RUN: %S/Inputs/config-files/4/44/- -- | FileCheck %s -check-prefix=CHECK-CHILD6
 // CHECK-CHILD6: Checks: {{.*-llvm-qualified-auto'? *$}}
 // CHECK-CHILD6-NOT: modernize-use-using.IgnoreMacros