[clang-tidy] Extend spelling for CheckOptions

[njames93]

Summary

The current way to specify CheckOptions is pretty verbose and unintuitive.
Given that the options are a dictionary it makes much more sense to treat them as such in the config files.
Example:

CheckOptions: {SomeCheck.Option: true, SomeCheck.OtherOption: 'ignore'}
# Or
CheckOptions:
  SomeCheck.Option: true
  SomeCheck.OtherOption: 'ignore'

This change will still handle the old syntax with no issue, ensuring we don't screw up users current config files.

The only observable differences are support for the new syntax and -dump=config will emit using the new syntax.

[njames93]

Update some test files using new syntax.

[aaron.ballman]

I like the changes -- this is a much nicer syntax for specifying configuration options!

The only observable differences are support for the new syntax and -dump=config will emit using the new syntax.

Do you expect the behavior of -dump to cause any problems for folks using that option from a script? I can't think of any that aren't super contrived, but maybe you've got more thoughts there.

(Note, you should probably rebase your patch as it doesn't seem to apply cleaning, so there's no precommit CI happening for it.)

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

85

I'm not super tickled with IO Io so if you want to use a more descriptive name, feel free. Mostly just changing it for coding style conformance.

clang-tools-extra/docs/ReleaseNotes.rst

113

[njames93]

In D128337#3602694, @aaron.ballman wrote:

I like the changes -- this is a much nicer syntax for specifying configuration options!

The only observable differences are support for the new syntax and -dump=config will emit using the new syntax.

Do you expect the behavior of -dump to cause any problems for folks using that option from a script? I can't think of any that aren't super contrived, but maybe you've got more thoughts there.

I wouldn't expect people are using the dump-config option for any scripts, It's only really there for debugging.

(Note, you should probably rebase your patch as it doesn't seem to apply cleaning, so there's no precommit CI happening for it.)

That's your fault :) https://github.com/llvm/llvm-project/commit/bb297024fad2f6c3ccaaa6a5c3a270f73f15f3ac

[njames93]

Rebased and addressed comments

[aaron.ballman]

In D128337#3602784, @njames93 wrote:

In D128337#3602694, @aaron.ballman wrote:

I like the changes -- this is a much nicer syntax for specifying configuration options!

The only observable differences are support for the new syntax and -dump=config will emit using the new syntax.

Do you expect the behavior of -dump to cause any problems for folks using that option from a script? I can't think of any that aren't super contrived, but maybe you've got more thoughts there.

I wouldn't expect people are using the dump-config option for any scripts, It's only really there for debugging.

Okay, that works for me!

(Note, you should probably rebase your patch as it doesn't seem to apply cleaning, so there's no precommit CI happening for it.)

That's your fault :) https://github.com/llvm/llvm-project/commit/bb297024fad2f6c3ccaaa6a5c3a270f73f15f3ac

Heh, sorry about that. :-)

LGTM!

[njames93]

Rebase to check CI

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

ClangTidyOptions::OptionMap denormalize(IO &) {
     ClangTidyOptions::OptionMap Map;
     for (const auto &KeyValue : Options)
       Map[KeyValue.first] = ClangTidyOptions::ClangTidyValue(KeyValue.second);
     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;
+      IO.scalarString(S, needsQuotes(S));
+      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();
+      for (StringRef Key : IO.keys()) {
+        IO.mapRequired(Key.data(), Options[Key].Value);
+      }
+      IO.endMapping();
+    } else {
+      IO.setError("expected a sequence or map");
+    }
+  }
+}
+
 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("CheckOptions", Options.CheckOptions);
     IO.mapOptional("ExtraArgs", Options.ExtraArgs);
     IO.mapOptional("ExtraArgsBefore", Options.ExtraArgsBefore);
     IO.mapOptional("InheritParentConfig", Options.InheritParentConfig);
     IO.mapOptional("UseColor", Options.UseColor);
   }
 };
 
 } // namespace yaml

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

The effective configuration can be inspected using -dump-config:
     ---
     Checks:              '-*,some-check'
     WarningsAsErrors:    ''
     HeaderFilterRegex:   ''
     FormatStyle:         none
     InheritParentConfig: true
     User:                user
     CheckOptions:
-      - key:             some-check.SomeOption
-        value:           'some value'
+      some-check.SomeOption: 'some value'
     ...
 
 )");
 
 const char DefaultChecks[] = // Enable these checks by default:
     "clang-diagnostic-*,"    //   * compiler diagnostics
     "clang-analyzer-*";      //   * Static Analyzer checks
 

 enabled, i.e. in clang-tidy binary, command
 line or a specific configuration file.
 )"),
                                    cl::init(false), cl::cat(ClangTidyCategory));
 
 static cl::opt<std::string> Config("config", cl::desc(R"(
 Specifies a configuration in YAML/JSON format:
   -config="{Checks: '*',
-            CheckOptions: [{key: x,
-                            value: y}]}"
+            CheckOptions: {x: y}}"
 When the value is empty, clang-tidy will
 attempt to find a file named .clang-tidy for
 each source file in its parent directories.
 )"),
                                    cl::init(""), cl::cat(ClangTidyCategory));
 
 static cl::opt<std::string> ConfigFile("config-file", cl::desc(R"(
 Specify the path of .clang-tidy or custom config file:

clang-tools-extra/clang-tidy/tool/run-clang-tidy.py

parser.add_argument('-clang-apply-replacements-binary', metavar='PATH',
                       help='path to clang-apply-replacements binary')
   parser.add_argument('-checks', default=None,
                       help='checks filter, when not specified, use clang-tidy '
                       'default')
   config_group = parser.add_mutually_exclusive_group()
   config_group.add_argument('-config', default=None,
                       help='Specifies a configuration in YAML/JSON format: '
                       '  -config="{Checks: \'*\', '
-                      '                       CheckOptions: [{key: x, '
-                      '                                       value: y}]}" '
+                      '                       CheckOptions: {x: y}}" '
                       'When the value is empty, clang-tidy will '
                       'attempt to find a file named .clang-tidy for '
                       'each source file in its parent directories.')
   config_group.add_argument('-config-file', default=None,
                       help='Specify the path of .clang-tidy or custom config '
                       'file: e.g. -config-file=/some/path/myTidyConfigFile. '
                       'This option internally works exactly the same way as '
                       '-config option after reading specified config file. '

clang-tools-extra/clangd/unittests/TidyProviderTests.cpp

 
 namespace {
 
 TEST(TidyProvider, NestedDirectories) {
   MockFS FS;
   FS.Files[testPath(".clang-tidy")] = R"yaml(
   Checks: 'llvm-*'
   CheckOptions:
-    - key: TestKey
-      value: 1
+    TestKey: 1
 )yaml";
   FS.Files[testPath("sub1/.clang-tidy")] = R"yaml(
   Checks: 'misc-*'
   CheckOptions:
-    - key: TestKey
-      value: 2
+    TestKey: 2
 )yaml";
   FS.Files[testPath("sub1/sub2/.clang-tidy")] = R"yaml(
   Checks: 'bugprone-*'
   CheckOptions:
-    - key: TestKey
-      value: 3
+    TestKey: 3
   InheritParentConfig: true
 )yaml";
 
   TidyProvider Provider = provideClangTidyFiles(FS);
 
   auto BaseOptions = getTidyOptionsForFile(Provider, testPath("File.cpp"));
   ASSERT_TRUE(BaseOptions.Checks.hasValue());
   EXPECT_EQ(*BaseOptions.Checks, "llvm-*");

clang-tools-extra/docs/ReleaseNotes.rst

- Clang-tidy now consideres newlines as separators of single elements in the `Checks` section in
   this list from each other, newline characters now also work as separators in the parsed YAML. That
   means it is advised to use YAML's block style initiated by the pipe character `|` for the `Checks`
   section in order to benefit from the easier syntax that works without commas.
 
 - Fixed a regression introduced in clang-tidy 14.0.0, which prevented NOLINTs
   from suppressing diagnostics associated with macro arguments. This fixes
   `Issue 55134 <https://github.com/llvm/llvm-project/issues/55134>`_.
 
 - Added an option -verify-config which will check the config file to ensure each
   `Checks` and `CheckOptions` entries are recognised.
 
+- .clang-tidy files can now use the more natural dictionary syntax for specifying `CheckOptions`.
+
 New checks
 ^^^^^^^^^^
 
 - New :doc:`bugprone-shared-ptr-array-mismatch <clang-tidy/checks/bugprone/shared-ptr-array-mismatch>` check.
 
   Finds initializations of C++ shared pointers to non-array type that are initialized with an array.
 
 - New :doc:`bugprone-unchecked-optional-access

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

public:
     ...
 
 Assuming the check is registered with the name "my-check", the option can then
 be set in a ``.clang-tidy`` file in the following way:
 
 .. code-block:: yaml
 
   CheckOptions:
-    - key: my-check.SomeOption1
-      value: 123
-    - key: my-check.SomeOption2
-      value: 'some other value'
+    my-check.SomeOption1: 123
+    my-check.SomeOption2: 'some other value'
 
 If you need to specify check options on a command line, you can use the inline
 YAML format:
 
 .. code-block:: console
 
-  $ clang-tidy -config="{CheckOptions: [{key: a, value: b}, {key: x, value: y}]}" ...
+  $ clang-tidy -config="{CheckOptions: {a: b, x: y}}" ...
 
 
 Testing Checks
 --------------
 
 To run tests for :program:`clang-tidy`, build the ``check-clang-tools`` target.
 For instance, if you configured your CMake build with the ninja project generator,
 use the command:

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

--checks=<string>              -
                                      set, globs with the '-' prefix remove checks
                                      with matching names from the set of enabled
                                      checks. This option's value is appended to the
                                      value of the 'Checks' option in .clang-tidy
                                      file, if any.
     --config=<string>              -
                                      Specifies a configuration in YAML/JSON format:
                                        -config="{Checks: '*',
-                                                 CheckOptions: [{key: x,
-                                                                 value: y}]}"
+                                                 CheckOptions: {x, y}}"
                                      When the value is empty, clang-tidy will
                                      attempt to find a file named .clang-tidy for
                                      each source file in its parent directories.
     --config-file=<string>         - 
                                     Specify the path of .clang-tidy or custom config file:
                                       e.g. --config-file=/some/path/myTidyConfigFile
                                     This option internally works exactly the same way as
                                       --config option after reading specified config file.
The effective configuration can be inspected using -dump-config:
       ---
       Checks:              '-*,some-check'
       WarningsAsErrors:    ''
       HeaderFilterRegex:   ''
       FormatStyle:         none
       InheritParentConfig: true
       User:                user
       CheckOptions:
-        - key:             some-check.SomeOption
-          value:           'some value'
+        some-check.SomeOption: 'some value'
       ...
 
 .. _clang-tidy-nolint:
 
 Suppressing Undesired Diagnostics
 =================================
 
 :program:`clang-tidy` diagnostics are intended to call out code that does not

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

 // RUN: clang-tidy -checks='-*,google*' -config='{}' -dump-config - -- | FileCheck %s
 // CHECK: CheckOptions:
-// CHECK-DAG: {{- key: *google-readability-braces-around-statements.ShortStatementLines *[[:space:]] *value: *'1'}}
-// CHECK-DAG: {{- key: *google-readability-function-size.StatementThreshold *[[:space:]] *value: *'800'}}
-// CHECK-DAG: {{- key: *google-readability-namespace-comments.ShortNamespaceLines *[[:space:]] *value: *'10'}}
-// CHECK-DAG: {{- key: *google-readability-namespace-comments.SpacesBeforeComments *[[:space:]] *value: *'2'}}
+// 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'}}

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'
+

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

 // 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
 
 // 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: - key: llvm-qualified-auto.AddConstToQualified{{ *[[:space:]] *}}value: 'true'
-// CHECK-CHILD4-DAG: - key: modernize-loop-convert.MaxCopySize{{ *[[:space:]] *}}value: '20'
-// CHECK-CHILD4-DAG: - key: modernize-loop-convert.MinConfidence{{ *[[:space:]] *}}value: reasonable
-// CHECK-CHILD4-DAG: - key: modernize-use-using.IgnoreMacros{{ *[[:space:]] *}}value: 'false'
+// 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'
 
 // 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: - key: modernize-loop-convert.MaxCopySize{{ *[[:space:]] *}}value: '21'
-// CHECK-CHILD5-DAG: - key: modernize-loop-convert.MinConfidence{{ *[[:space:]] *}}value: reasonable
-// CHECK-CHILD5-DAG: - key: modernize-use-using.IgnoreMacros{{ *[[:space:]] *}}value: 'false'
+// CHECK-CHILD5-DAG: modernize-loop-convert.MaxCopySize: '21'
+// CHECK-CHILD5-DAG: modernize-loop-convert.MinConfidence: reasonable
+// CHECK-CHILD5-DAG: 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: - key: modernize-use-using.IgnoreMacros
+// CHECK-CHILD6-NOT: modernize-use-using.IgnoreMacros