[clang-tidy] Add --use-color command line option and UseColor option to control colors in diagnostics

[hyd-dev]

Summary

This patch adds --use-color command line option and UseColor option to clang-tidy to control colors in diagnostics. With these options, users can force colorful output. This is useful when using clang-tidy with parallelization command line tools (like ninja and GNU parallel), as they often pipe clang-tidy's standard output and make the colors disappear.

[hokein]

mostly good, just nits.

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

130

nit: ... wil be used if the terminal connected to standard output supports colors, or even simple if missing, it will be auto detect.

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

233

nit: just use-color

237

nit: set cl::init(false)

[hyd-dev]

Address the comments above.
I also removed the incorrect DefaultOptions.ColorDiagnostics = ColorDiagnostics in ClangTidyMain.cpp. That produces incorrect UseColor: false with -dump-config.

[hyd-dev]

Rename clang-tools-extra/test/clang-tidy/infrastructure/color-diagnostics.cpp to use-color.cpp.

[njames93]

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

99

I'm not entirely sure this option belongs in the config file. However if it will get read in here, maybe update the help string for the command line option to say it overrides the UseColor option from the .clang-tidy configuration file.

[hyd-dev]

Say the --use-color command line option overrides the UseColor option in .clang-tidy file in its help text.

[hyd-dev]

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

99

I'm not entirely sure this option belongs in the config file.

It will be useful in the config file if the user invokes clang-tidy via another tool that does not allow the user to set clang-tidy's command line options.

However if it will get read in here, maybe update the help string for the command line option to say it overrides the UseColor option from the .clang-tidy configuration file.

I've updated it.

[aaron.ballman]

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

130

detect -> detected

[hyd-dev]

Change incorrect detect to detected in the comment of UseColor option.

[njames93]

Could this not use the -fcolor-diagnostics and fno-color-diagnostics command line flags that clang uses for its diagnostics to keep everything the same?

[hyd-dev]

None of the clang-tidy command line options are prefixed with -f.
This command line option used to be --color-diagnostics, but I've followed @hokein's advice to change it to --use-color.
If you mean to use the -fcolor-diagnostics option from compile_commands.json, clang-tidy and clang are two separate tools. The users may not want them to share the same setting, because they may invoke them in different environments.

[njames93]

In D79477#2027978, @hyd-dev wrote:

None of the clang-tidy command line options are prefixed with -f.
This command line option used to be --color-diagnostics, but I've followed @hokein's advice to change it to --use-color.
If you mean to use the -fcolor-diagnostics option from compile_commands.json, clang-tidy and clang are two separate tools. The users may not want them to share the same setting, because they may invoke them in different environments.

Fair point, will this option also control the color of the diagnostics emitted by clang or just clang tidy specific diagnostics?

[hyd-dev]

Fair point, will this option also control the color of the diagnostics emitted by clang or just clang tidy specific diagnostics?

This option sets DiagOpts->ShowColors to true. As I known, it controls all diagnostics reported by the clang-tidy program (by clang::tidy::(anonymous namespace)::ErrorReporter), including clang-diagnostic-* and other clang-tidy checks.

[njames93]

In D79477#2028609, @hyd-dev wrote:

This option sets DiagOpts->ShowColors to true. As I known, it controls all diagnostics reported by the clang-tidy program (by clang::tidy::(anonymous namespace)::ErrorReporter), including clang-diagnostic-* and other clang-tidy checks.

Would a test case be needed?

clang-tools-extra/test/clang-tidy/infrastructure/use-color.cpp

11

Not a fan of this test case as it only demonstrates the color behaviour of the process running the check not the actual option itself

[hyd-dev]

Would a test case be needed?

clang::tidy::clangTidyMain() shows that clang-tidy reports all diagnostics by clang::tidy::handleErrors(), which constructs a clang::tidy::(anonymous namespace)::ErrorReporter, no matter where they come from, so I think a test case is not required.

Not a fan of this test case as it only demonstrates the color behaviour of the process running the check not the actual option itself

What does "option itself" mean?

[njames93]

In D79477#2028618, @hyd-dev wrote:

Not a fan of this test case as it only demonstrates the color behaviour of the process running the check not the actual option itself

What does "option itself" mean?

What I mean to say is, if the behaviour of the testing environment changes to pipe the result to a terminal that supports color, it could cause this test case to also fail.

[hyd-dev]

In D79477#2028637, @njames93 wrote:

What I mean to say is, if the behaviour of the testing environment changes to pipe the result to a terminal that supports color, it could cause this test case to also fail.

The standard output is always piped to FileCheck's standard input. It will never become "a terminal that supports color".
Standard error is not redirected explicitly, but it is not used to detect color support.

[hyd-dev]

Any reply?

[hyd-dev]

Fix typos in the summary.

[hyd-dev]

Ping?

[hyd-dev]

Ping?

[njames93]

LGTM

[hyd-dev]

I don't have commit access. @njames93 Could you please commit it for me (hyd-dev <[e-mail]>)? Thanks!

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

public:
   ErrorReporter(ClangTidyContext &Context, bool ApplyFixes,
                 llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS)
       : Files(FileSystemOptions(), BaseFS), DiagOpts(new DiagnosticOptions()),
         DiagPrinter(new TextDiagnosticPrinter(llvm::outs(), &*DiagOpts)),
         Diags(IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs), &*DiagOpts,
               DiagPrinter),
         SourceMgr(Diags, Files), Context(Context), ApplyFixes(ApplyFixes),
         TotalFixes(0), AppliedFixes(0), WarningsAsErrors(0) {
-    DiagOpts->ShowColors = llvm::sys::Process::StandardOutHasColors();
+    DiagOpts->ShowColors = Context.getOptions().UseColor.getValueOr(
+        llvm::sys::Process::StandardOutHasColors());
     DiagPrinter->BeginSourceFile(LangOpts);
   }
 
   SourceManager &getSourceManager() { return SourceMgr; }
 
   void reportDiagnostic(const ClangTidyError &Error) {
     const tooling::DiagnosticMessage &Message = Error.Message;
     SourceLocation Loc = getLocation(Message.FilePath, Message.FileOffset);

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

struct ClangTidyOptions {
   /// Add extra compilation arguments to the start of the list.
   llvm::Optional<ArgList> ExtraArgsBefore;
 
   /// 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 or missing,
   /// only this configuration file will be used.
   llvm::Optional<bool> InheritParentConfig;
+
+  /// Use colors in diagnostics. If missing, it will be auto detected.
+  llvm::Optional<bool> UseColor;
 };
 
 /// 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

static void mapping(IO &IO, ClangTidyOptions &Options) {
     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);
+    IO.mapOptional("UseColor", Options.UseColor);
   }
 };
 
 } // namespace yaml
 } // namespace llvm
 
 namespace clang {
 namespace tidy {
ClangTidyOptions ClangTidyOptions::mergeWith(const ClangTidyOptions &Other,
   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);
+  overrideValue(Result.UseColor, Other.UseColor);
   mergeVectors(Result.ExtraArgs, Other.ExtraArgs);
   mergeVectors(Result.ExtraArgsBefore, Other.ExtraArgsBefore);
 
   for (const auto &KeyValue : Other.CheckOptions) {
     Result.CheckOptions[KeyValue.first] = ClangTidyValue(
         KeyValue.second.Value, KeyValue.second.Priority + Priority);
   }
 

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

 
 static cl::opt<std::string> VfsOverlay("vfsoverlay", cl::desc(R"(
 Overlay the virtual filesystem described by file
 over the real file system.
 )"),
                                        cl::value_desc("filename"),
                                        cl::cat(ClangTidyCategory));
 
+static cl::opt<bool> UseColor("use-color", cl::desc(R"(
+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.
+)"),
+                              cl::init(false), cl::cat(ClangTidyCategory));
+
 namespace clang {
 namespace tidy {
 
 static void printStats(const ClangTidyStats &Stats) {
   if (Stats.errorsIgnored()) {
     llvm::errs() << "Suppressed " << Stats.errorsIgnored() << " warnings (";
     StringRef Separator = "";
     if (Stats.ErrorsIgnoredNonUserCode) {
static std::unique_ptr<ClangTidyOptionsProvider> createOptionsProvider(
   if (WarningsAsErrors.getNumOccurrences() > 0)
     OverrideOptions.WarningsAsErrors = WarningsAsErrors;
   if (HeaderFilter.getNumOccurrences() > 0)
     OverrideOptions.HeaderFilterRegex = HeaderFilter;
   if (SystemHeaders.getNumOccurrences() > 0)
     OverrideOptions.SystemHeaders = SystemHeaders;
   if (FormatStyle.getNumOccurrences() > 0)
     OverrideOptions.FormatStyle = FormatStyle;
+  if (UseColor.getNumOccurrences() > 0)
+    OverrideOptions.UseColor = UseColor;
 
   if (!Config.empty()) {
     if (llvm::ErrorOr<ClangTidyOptions> ParsedConfig =
             parseConfiguration(Config)) {
       return std::make_unique<ConfigOptionsProvider>(
           GlobalOptions,
           ClangTidyOptions::getDefaults().mergeWith(DefaultOptions, 0),
           *ParsedConfig, OverrideOptions);

clang-tools-extra/test/clang-tidy/infrastructure/use-color.cpp

+// RUN: clang-tidy -dump-config | FileCheck %s
+// RUN: clang-tidy -dump-config -use-color | FileCheck -check-prefix=CHECK-CONFIG-COLOR %s
+// RUN: clang-tidy -dump-config -use-color=false | FileCheck -check-prefix=CHECK-CONFIG-NO-COLOR %s
+// RUN: clang-tidy -config='UseColor: true' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-COLOR %s
+// RUN: clang-tidy -config='UseColor: false' -dump-config | FileCheck -check-prefix=CHECK-CONFIG-NO-COLOR %s
+// RUN: clang-tidy -help | FileCheck -check-prefix=CHECK-OPT-PRESENT %s
+
+// REQUIRES: ansi-escape-sequences
+// RUN: clang-tidy -checks='-*, modernize-use-override' -extra-arg=-std=c++11 -use-color=false %s | FileCheck -check-prefix=CHECK-NO-COLOR %s
+// RUN: clang-tidy -checks='-*, modernize-use-override' -extra-arg=-std=c++11 %s | FileCheck -check-prefix=CHECK-NO-COLOR %s
+// RUN: clang-tidy -checks='-*, modernize-use-override' -extra-arg=-std=c++11 -use-color %s | FileCheck -check-prefix=CHECK-COLOR %s
+
+// CHECK-NOT: UseColor
+// CHECK-CONFIG-NO-COLOR: UseColor: false
+// CHECK-CONFIG-COLOR: UseColor: true
+// CHECK-OPT-PRESENT: --use-color
+
+class Base {
+public:
+  virtual ~Base() = 0;
+};
+
+class Delivered : public Base {
+public:
+  virtual ~Delivered() = default;
+  // CHECK-NO-COLOR: warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [modernize-use-override]
+  // CHECK-COLOR: {{.\[0;1;35m}}warning: {{.\[0m}}{{.\[1m}}prefer using 'override' or (rarely) 'final' instead of 'virtual' [modernize-use-override]
+};

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

 TEST(ParseConfiguration, MergeConfigurations) {
   llvm::ErrorOr<ClangTidyOptions> Options1 = parseConfiguration(R"(
       Checks: "check1,check2"
       HeaderFilterRegex: "filter1"
       AnalyzeTemporaryDtors: true
       User: user1
       ExtraArgs: ['arg1', 'arg2']
       ExtraArgsBefore: ['arg-before1', 'arg-before2']
+      UseColor: false
   )");
   ASSERT_TRUE(!!Options1);
   llvm::ErrorOr<ClangTidyOptions> Options2 = parseConfiguration(R"(
       Checks: "check3,check4"
       HeaderFilterRegex: "filter2"
       AnalyzeTemporaryDtors: false
       User: user2
       ExtraArgs: ['arg3', 'arg4']
       ExtraArgsBefore: ['arg-before3', 'arg-before4']
+      UseColor: true
   )");
   ASSERT_TRUE(!!Options2);
   ClangTidyOptions Options = Options1->mergeWith(*Options2, 0);
   EXPECT_EQ("check1,check2,check3,check4", *Options.Checks);
   EXPECT_EQ("filter2", *Options.HeaderFilterRegex);
   EXPECT_EQ("user2", *Options.User);
   ASSERT_TRUE(Options.ExtraArgs.hasValue());
   EXPECT_EQ("arg1,arg2,arg3,arg4", llvm::join(Options.ExtraArgs->begin(),
                                               Options.ExtraArgs->end(), ","));
   ASSERT_TRUE(Options.ExtraArgsBefore.hasValue());
   EXPECT_EQ("arg-before1,arg-before2,arg-before3,arg-before4",
             llvm::join(Options.ExtraArgsBefore->begin(),
                        Options.ExtraArgsBefore->end(), ","));
+  ASSERT_TRUE(Options.UseColor.hasValue());
+  EXPECT_TRUE(*Options.UseColor);
 }
 
 class TestCheck : public ClangTidyCheck {
 public:
   TestCheck(ClangTidyContext *Context) : ClangTidyCheck("test", Context) {}
 
   template <typename... Args> auto getLocal(Args &&... Arguments) {
     return Options.get(std::forward<Args>(Arguments)...);