[clang-tidy] Support SystemHeaders in .clang-tidy

[carlosgalvezp]

Summary

A previous patch update the clang-tidy documentation
incorrectly claiming that SystemHeaders can be provided
in the .clang-tidy configuration file.

This patch adds support for it, together with tests.

Fixes https://github.com/llvm/llvm-project/issues/62547

[carlosgalvezp]

Update commit message with Github issue.

[PiotrZSL]

From functionally point of view, LGTM.

clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp

12

I missing 2 types of tests here:

• config value override, -config='SystemHeaders: true' -system-headers=false
• config file override, when one file has false, other true, but InheritConfig is used.

Both should work, but would be good to test them.

[carlosgalvezp]

clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp

12

Thanks for the input! Essentially I followed the same testing pattern as UseColor, which is also a global boolean option.

Adding the first suggestion should be trivial in the current test case, but I have some doubts about the proposed second test. When I look at the tests/clang-tidy/infrastructure/config-files.cpp it only takes into account enabled checks, no other global options are tested such that one overrides the other. Therefore I wonder if there is a rationale for not having such level of testing, perhaps easier maintenance, given that it's already tested by the unit tests? In that case I would propose to not add such a test here, for consistency. WDYT?

[PiotrZSL]

clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp

12

I'm fine with that. I'm mainly concern about the first one, simply because I'm not sure about outcome, what will have higher priority, command line argument or config option.

[carlosgalvezp]

• Add test where both command line and config settings are used.
• Document that the command-line option overrides the config option, for consistency with other command-line options.

[carlosgalvezp]

Revert unwanted change to UseColor

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

static void mapping(IO &IO, ClangTidyOptions &Options) {
     IO.mapOptional("AnalyzeTemporaryDtors", Ignored); // deprecated
     IO.mapOptional("FormatStyle", Options.FormatStyle);
     IO.mapOptional("User", Options.User);
     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);
+    IO.mapOptional("SystemHeaders", Options.SystemHeaders);
   }
 };
 
 } // namespace llvm::yaml
 
 namespace clang::tidy {
 
 ClangTidyOptions ClangTidyOptions::getDefaults() {

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

 always displayed.
 Can be used together with -line-filter.
 This option overrides the 'HeaderFilterRegex'
 option in .clang-tidy file, if any.
 )"),
                                          cl::init(""),
                                          cl::cat(ClangTidyCategory));
 
-static cl::opt<bool>
-    SystemHeaders("system-headers",
-                  desc("Display the errors from system headers."),
+static cl::opt<bool> SystemHeaders("system-headers", desc(R"(
+Display the errors from system headers.
+This option overrides the 'SystemHeaders' option
+in .clang-tidy file, if any.
+)"),
                                    cl::init(false), cl::cat(ClangTidyCategory));
+
 static cl::opt<std::string> LineFilter("line-filter", desc(R"(
 List of files with line ranges to filter the
 warnings. Can be used together with
 -header-filter. The format of the list is a
 JSON array of objects:
   [
     {"name":"file1.cpp","lines":[[1,3],[5,7]]},
     {"name":"file2.h"}

clang-tools-extra/docs/ReleaseNotes.rst

- New global configuration file options `HeaderFileExtensions` and
   `ImplementationFileExtensions`, replacing the check-local options of the
   same name.
 
 - Support specifying `Checks` as a YAML list in the `.clang-tidy` configuration
   file.
 
 - Fix a potential crash when using the `--dump-config` option.
 
+- Support specifying `SystemHeaders` in the `.clang-tidy` configuration file,
+  with the same functionality as the command-line option `--system-headers`.
+
 New checks
 ^^^^^^^^^^
 
 - New :doc:`bugprone-multiple-new-in-one-expression
   <clang-tidy/checks/bugprone/multiple-new-in-one-expression>` check.
 
   Finds multiple ``new`` operator calls in a single expression, where the allocated
   memory by the first ``new`` may leak if the second allocation fails and throws exception.

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

clang-tidy options:
     --quiet                        - Run clang-tidy in quiet mode. This suppresses
                                      printing statistics about ignored warnings and
                                      warnings treated as errors if the respective
                                      options are specified.
     --store-check-profile=<prefix> - By default reports are printed in tabulated
                                      format to stderr. When this option is passed,
                                      these per-TU profiles are instead stored as JSON.
     --system-headers               - Display the errors from system headers.
+                                     This option overrides the 'SystemHeaders' option
+                                     in .clang-tidy file, if any.
     --use-color                    - Use colors in diagnostics. If not set, colors
                                      will be used if the terminal connected to
                                      standard output supports colors.
                                      This option overrides the 'UseColor' option in
                                      .clang-tidy file, if any.
     --verify-config                - Check the config files to ensure each check and
                                      option is recognized.
     --vfsoverlay=<filename>        - Overlay the virtual filesystem described by file

clang-tools-extra/test/clang-tidy/infrastructure/Inputs/system-headers/system_header.h

+class Foo { Foo(int); };

clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp

+// RUN: clang-tidy -dump-config -system-headers=true | FileCheck -check-prefix=CHECK-CONFIG-SYSTEM-HEADERS %s
+// RUN: clang-tidy -dump-config -system-headers=false | FileCheck -check-prefix=CHECK-CONFIG-NO-SYSTEM-HEADERS %s
+// RUN: clang-tidy -config='SystemHeaders: true' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-SYSTEM-HEADERS %s
+// RUN: clang-tidy -config='SystemHeaders: false' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-NO-SYSTEM-HEADERS %s
+
+// RUN: clang-tidy -system-headers=true -config='SystemHeaders: true' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-SYSTEM-HEADERS %s
+// RUN: clang-tidy -system-headers=true -config='SystemHeaders: false' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-SYSTEM-HEADERS %s
+// RUN: clang-tidy -system-headers=false -config='SystemHeaders: true' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-NO-SYSTEM-HEADERS %s
+// RUN: clang-tidy -system-headers=false -config='SystemHeaders: false' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-NO-SYSTEM-HEADERS %s
+
+// RUN: clang-tidy -help | FileCheck -check-prefix=CHECK-OPT-PRESENT %s
+
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers=true %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-SYSTEM-HEADERS %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers=false %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-NO-SYSTEM-HEADERS %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -config='SystemHeaders: true' %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-SYSTEM-HEADERS %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -config='SystemHeaders: false' %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-NO-SYSTEM-HEADERS %s
+
+#include <system_header.h>
+// CHECK-SYSTEM-HEADERS: system_header.h:1:13: warning: single-argument constructors must be marked explicit
+// CHECK-NO-SYSTEM-HEADERS-NOT: system_header.h:1:13: warning: single-argument constructors must be marked explicit
+
+// CHECK-CONFIG-NO-SYSTEM-HEADERS: SystemHeaders: false
+// CHECK-CONFIG-SYSTEM-HEADERS: SystemHeaders: true
+// CHECK-OPT-PRESENT: --system-headers

clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp

llvm::ErrorOr<ClangTidyOptions> Options1 =
       HeaderFileExtensions: ["h","hh"]
       ImplementationFileExtensions: ["c","cc"]
       HeaderFilterRegex: "filter1"
       AnalyzeTemporaryDtors: true
       User: user1
       ExtraArgs: ['arg1', 'arg2']
       ExtraArgsBefore: ['arg-before1', 'arg-before2']
       UseColor: false
+      SystemHeaders: false
   )",
                                                "Options1"));
   ASSERT_TRUE(!!Options1);
   llvm::ErrorOr<ClangTidyOptions> Options2 =
       parseConfiguration(llvm::MemoryBufferRef(R"(
       Checks: "check3,check4"
       HeaderFileExtensions: ["hpp","hxx"]
       ImplementationFileExtensions: ["cpp","cxx"]
       HeaderFilterRegex: "filter2"
       AnalyzeTemporaryDtors: false
       User: user2
       ExtraArgs: ['arg3', 'arg4']
       ExtraArgsBefore: ['arg-before3', 'arg-before4']
       UseColor: true
+      SystemHeaders: true
   )",
                                                "Options2"));
   ASSERT_TRUE(!!Options2);
   ClangTidyOptions Options = Options1->merge(*Options2, 0);
   EXPECT_EQ("check1,check2,check3,check4", *Options.Checks);
   EXPECT_EQ(std::vector<std::string>({"hpp", "hxx"}),
             *Options.HeaderFileExtensions);
   EXPECT_EQ(std::vector<std::string>({"cpp", "cxx"}),
             *Options.ImplementationFileExtensions);
   EXPECT_EQ("filter2", *Options.HeaderFilterRegex);
   EXPECT_EQ("user2", *Options.User);
   ASSERT_TRUE(Options.ExtraArgs.has_value());
   EXPECT_EQ("arg1,arg2,arg3,arg4", llvm::join(Options.ExtraArgs->begin(),
                                               Options.ExtraArgs->end(), ","));
   ASSERT_TRUE(Options.ExtraArgsBefore.has_value());
   EXPECT_EQ("arg-before1,arg-before2,arg-before3,arg-before4",
             llvm::join(Options.ExtraArgsBefore->begin(),
                        Options.ExtraArgsBefore->end(), ","));
   ASSERT_TRUE(Options.UseColor.has_value());
   EXPECT_TRUE(*Options.UseColor);
+
+  ASSERT_TRUE(Options.SystemHeaders.has_value());
+  EXPECT_TRUE(*Options.SystemHeaders);
 }
 
 namespace {
 class DiagCollecter {
 public:
   struct Diag {
   private:
     static size_t posToOffset(const llvm::SMLoc Loc,
TEST(ParseConfiguration, CollectDiags) {
   )");
   ParsedOpt = ParseWithDiags(Options.code());
   EXPECT_TRUE(!ParsedOpt);
   EXPECT_THAT(Collector.getDiags(),
               testing::ElementsAre(AllOf(DiagMessage("invalid boolean"),
                                          DiagKind(llvm::SourceMgr::DK_Error),
                                          DiagPos(Options.range().Begin),
                                          DiagRange(Options.range()))));
+
+  Options = llvm::Annotations(R"(
+    SystemHeaders: [[NotABool]]
+  )");
+  ParsedOpt = ParseWithDiags(Options.code());
+  EXPECT_TRUE(!ParsedOpt);
+  EXPECT_THAT(Collector.getDiags(),
+              testing::ElementsAre(AllOf(DiagMessage("invalid boolean"),
+                                         DiagKind(llvm::SourceMgr::DK_Error),
+                                         DiagPos(Options.range().Begin),
+                                         DiagRange(Options.range()))));
 }
 
 namespace {
 class TestCheck : public ClangTidyCheck {
 public:
   TestCheck(ClangTidyContext *Context) : ClangTidyCheck("test", Context) {}
 
   template <typename... Args> auto getLocal(Args &&... Arguments) {