[clang-tidy] Add a -show-color flag.

[itessier]

Summary

This change will allow enabling of colour diagnostics when not directly running within a terminal, but colour output is possible. For example, if stdout is being captured and redirected to a real terminal.

[aaron.ballman]

This patch is missing test coverage.

clang-tidy/ClangTidy.cpp

99–101

I think this can be simplified to:

DiagOpts->ShowColors = Context.getOptions().ShowColor.getValueOr(llvm::sys::Process::StandardOutHasColors());

clang-tidy/tool/ClangTidyMain.cpp

150–152

I think this raw string can be reflowed a bit?

Instead of "defaults to on", how about "Defaults to true when a color-capable terminal is detected."?

[itessier]

This patch is missing test coverage.

Done.

clang-tidy/tool/ClangTidyMain.cpp

150–152

I copied that part from the -fcolor-diagnostics flag to be consistent. I don't changing "on" to "true" if you prefer that instead.

[alexfh]

Clang supports -fcolor-diagnostics. I guess, we could use it, e.g. via -extra-arg=-fcolor-diagnostics.

clang-tidy/ClangTidyOptions.h

93

This doesn't belong to ClangTidyOptions. It's specific to the CLI, but CLI is not the only frontend for clang-tidy.

[alexfh]

In D41720#968338, @alexfh wrote:

Clang supports -fcolor-diagnostics. I guess, we could use it, e.g. via -extra-arg=-fcolor-diagnostics.

On a second thought, configuring the colors using -extra-arg doesn't seem like a good solution to me. A separate command line option is fine, but it should be just a frontend option independent of ClangTidyOptions.

[itessier]

clang-tidy/ClangTidyOptions.h

93

Since we have to propagate the value to the ErrorReporter, how about adding a bool param to the ErrorReporter ctor? We could add a setter instead, but that would require moving a diag printer call out of the ctor since it uses the DiagOpts instance.

The colour logic would then be moved into either clangTidyMain or handleErrors.

clang-tidy/tool/ClangTidyMain.cpp

150–152

That should have read "I don't mind changing..."

[aaron.ballman]

clang-tidy/tool/ClangTidyMain.cpp

150–152

I'd say let's go ahead and change it, and once this patch lands, change the help text for -fcolor-diagnostics in a follow-up.

[alexfh]

clang-tidy/ClangTidyOptions.h

93

A bool parameter in ErrorReporter ctor seems good.

[alexfh]

test/clang-tidy/show-color.cpp

7

nit: I'd prefer to depend on the wording and format of static analyzer warnings only where necessary. Static analyzer is in a different repository and when it changes, it's a bit more difficult for folks to detect failures in clang-tidy. Here we could use any native clang-tidy check instead.

[jhasse]

What are you thoughts about using an environment variable for this? Due to the number of differently named flags, it's often hard to configure a CI system to display color in every tool. (I'm trying to push for a standard for this, see https://bixense.com/clicolors/)

[• leoleoasd]

Any update on this?

[avemilia]

As a workaround, you can use a standalone utility called unbuffer from Tcl/Tk's expect bundle. Just put it in front of the clang-tidy invocation. In case you use run-clang-tidy, you can put invocation.insert(0, 'unbuffer') right above proc = subprocess.Popen(invocation, stdout=subprocess.PIPE, stderr=subprocess.PIPE).

clang-tidy/ClangTidy.cpp

 public:
   ErrorReporter(ClangTidyContext &Context, bool ApplyFixes)
       : Files(FileSystemOptions()), 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().ShowColor.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);
     // Contains a pair for each attempted fix: location and whether the fix was

clang-tidy/ClangTidyOptions.h

struct ClangTidyOptions {
   ///     names;
   ///   * 'file' - use the .clang-format file in the closest parent directory of
   ///     each source file;
   ///   * '{inline-formatting-style-in-yaml-format}'.
   ///
   /// See clang-format documentation for more about configuring format style.
   llvm::Optional<std::string> FormatStyle;
 
+  /// \brief Show color diagnostics.
+  llvm::Optional<bool> ShowColor;
+
   /// \brief Specifies the name or e-mail of the user running clang-tidy.
   ///
   /// 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;

clang-tidy/ClangTidyOptions.cpp

template <> struct MappingTraits<ClangTidyOptions> {
   static void mapping(IO &IO, ClangTidyOptions &Options) {
     MappingNormalization<NOptionMap, ClangTidyOptions::OptionMap> NOpts(
         IO, Options.CheckOptions);
     IO.mapOptional("Checks", Options.Checks);
     IO.mapOptional("WarningsAsErrors", Options.WarningsAsErrors);
     IO.mapOptional("HeaderFilterRegex", Options.HeaderFilterRegex);
     IO.mapOptional("AnalyzeTemporaryDtors", Options.AnalyzeTemporaryDtors);
     IO.mapOptional("FormatStyle", Options.FormatStyle);
+    IO.mapOptional("ShowColor", Options.ShowColor);
     IO.mapOptional("User", Options.User);
     IO.mapOptional("CheckOptions", NOpts->Options);
     IO.mapOptional("ExtraArgs", Options.ExtraArgs);
     IO.mapOptional("ExtraArgsBefore", Options.ExtraArgsBefore);
   }
 };
 
 } // namespace yaml
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.AnalyzeTemporaryDtors, Other.AnalyzeTemporaryDtors);
   overrideValue(Result.FormatStyle, Other.FormatStyle);
+  overrideValue(Result.ShowColor, Other.ShowColor);
   overrideValue(Result.User, Other.User);
   mergeVectors(Result.ExtraArgs, Other.ExtraArgs);
   mergeVectors(Result.ExtraArgsBefore, Other.ExtraArgsBefore);
 
   for (const auto &KeyValue : Other.CheckOptions)
     Result.CheckOptions[KeyValue.first] = KeyValue.second;
 
   return Result;

clang-tidy/tool/ClangTidyMain.cpp

 See clang-format documentation for the up-to-date
 information about formatting styles and options.
 This option overrides the 'FormatStyle` option in
 .clang-tidy file, if any.
 )"),
                                    cl::init("none"),
                                    cl::cat(ClangTidyCategory));
 
+static cl::opt<bool> ShowColor("show-color", cl::desc(R"(
+Show color diagnostics. If not specified,
+defaults to on when a color-capable terminal
+is detected.)"),
+                               cl::ValueOptional, cl::cat(ClangTidyCategory));
+
 static cl::opt<bool> ListChecks("list-checks", cl::desc(R"(
 List all enabled checks and exit. Use with
 -checks=* to list all available checks.
 )"),
                                 cl::init(false), cl::cat(ClangTidyCategory));
 
 static cl::opt<bool> ExplainConfig("explain-config", cl::desc(R"(
 For each enabled check explains, where it is
static std::unique_ptr<ClangTidyOptionsProvider> createOptionsProvider(
 
   ClangTidyOptions DefaultOptions;
   DefaultOptions.Checks = DefaultChecks;
   DefaultOptions.WarningsAsErrors = "";
   DefaultOptions.HeaderFilterRegex = HeaderFilter;
   DefaultOptions.SystemHeaders = SystemHeaders;
   DefaultOptions.AnalyzeTemporaryDtors = AnalyzeTemporaryDtors;
   DefaultOptions.FormatStyle = FormatStyle;
+  DefaultOptions.ShowColor = ShowColor;
   DefaultOptions.User = llvm::sys::Process::GetEnv("USER");
   // USERNAME is used on Windows.
   if (!DefaultOptions.User)
     DefaultOptions.User = llvm::sys::Process::GetEnv("USERNAME");
 
   ClangTidyOptions OverrideOptions;
   if (Checks.getNumOccurrences() > 0)
     OverrideOptions.Checks = Checks;
   if (WarningsAsErrors.getNumOccurrences() > 0)
     OverrideOptions.WarningsAsErrors = WarningsAsErrors;
   if (HeaderFilter.getNumOccurrences() > 0)
     OverrideOptions.HeaderFilterRegex = HeaderFilter;
   if (SystemHeaders.getNumOccurrences() > 0)
     OverrideOptions.SystemHeaders = SystemHeaders;
   if (AnalyzeTemporaryDtors.getNumOccurrences() > 0)
     OverrideOptions.AnalyzeTemporaryDtors = AnalyzeTemporaryDtors;
   if (FormatStyle.getNumOccurrences() > 0)
     OverrideOptions.FormatStyle = FormatStyle;
+  if (ShowColor.getNumOccurrences() > 0)
+    OverrideOptions.ShowColor = ShowColor;
 
   if (!Config.empty()) {
     if (llvm::ErrorOr<ClangTidyOptions> ParsedConfig =
             parseConfiguration(Config)) {
       return llvm::make_unique<ConfigOptionsProvider>(
           GlobalOptions,
           ClangTidyOptions::getDefaults().mergeWith(DefaultOptions),
           *ParsedConfig, OverrideOptions);

test/clang-tidy/show-color.cpp

+// RUN: clang-tidy -checks='-*,clang-analyzer-core.NullDereference' -show-color %s -- | FileCheck --check-prefix=CHECK-COLOR %s
+// RUN: clang-tidy -checks='-*,clang-analyzer-core.NullDereference' -show-color=0 %s -- | FileCheck --check-prefix=CHECK-NO-COLOR %s
+// REQUIRES: ansi-escape-sequences
+
+void test() {
+  int *value = 0;
+  // CHECK-COLOR: [[BOLD:.\[1m]]{{.*}}[[@LINE+2]]:10: [[RESET:.\[0m]][[MAGENTA:.\[0;1;35m]]warning: [[RESET]][[BOLD]]Dereference of null pointer (loaded from variable 'value') [clang-analyzer-core.NullDereference][[RESET]]
+  // CHECK-NO-COLOR: [[@LINE+1]]:10: warning: Dereference of null pointer (loaded from variable 'value') [clang-analyzer-core.NullDereference]
+  *value = 1;
+}