[clang-tidy] Provide fine control of color in run-clang-tidy

[kesyog]

Summary

D90110 modified the behavior of run-clang-tidy to always pass the
--use-color option to clang-tidy, which enabled colored diagnostics
output regardless of TTY status or .clang-tidy settings. This left the
user with no option to disable the colored output.

This presents an issue when trying to parse the output of run-clang-tidy
programmaticall, as the output is polluted with ANSI escape characters.

This PR fixes this issue in two ways:

1. It restores the default behavior of run-clang-tidy to let clang-tidy decide whether to color output. This allows the user to configure color via the UseColor option in a .clang-tidy file.
2. It adds mutually exclusive, optional -use-color and -no-use-color argument flags that let the user explicitly set the color option via the invocation.

After this change the default behavior of run-clang-tidy when no
.clang-tidy file is available is now to show no color, presumably
because clang-tidy detects that the output is being piped and defaults
to not showing colored output. This seems like an acceptable tradeoff
to respect .clang-tidy configurations, as users can still use the
-use-color option to explicitly enable color.

Fixes #49441 (50097 in Bugzilla)

[Eugene.Zelenko]

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

111

Shouldn't it be just else:?

[kesyog]

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

111

There are three cases:

Argument	use_color	Behavior
-use-color	True	--use-color is passed to clang-tidy, force enabling color
-no-use-color	False	--use-color=false is passed to clang-tidy, force disabling color
(none provided)	None	Nothing passed to clang-tidy. clang-tidy follows its default coloring behavior

The case on the highlighted line is the second row of the table, and we have to check that use_color is not None to exclude the case of the third row.

I was trying to avoid the extra nesting of something like below, but maybe the intent would be clearer?

if use_color is not None:
  if use_color:
    start.append('--use-color')
  else:
    start.append('--use-color=false')

[Eugene.Zelenko]

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

111

I think you implementation is incorrect. You should check for not None first and than set proper value for --use-color.

[kesyog]

Refactor tri-state logic for readability

[kesyog]

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

111

fixed

[kesyog]

ping

[njames93]

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

255–264

All the arguments to this script mirror clang-tidy arguments, therefore I'm against introducing gnc style command line flags for controlling colour output. I'd suggest something like this which mirrors clang-tidys behaviour much more closely,
This requires an import

from distutils.util import strtobool

Tbh it may be better writing a strtobool that is in sync with llvms command line bool parser

{ "true", "TRUE", "True", "1"` -> true }
{ "false", "FALSE", "False", "0" -> false }
{ ... -> Error }

[njames93]

Why have you removed execute perms form the scripts, can you add them back please
$ chmod +x run-clang-tidy.py'
Unless you are using windows in which case you will have to manually edit the diff

[kesyog]

Use argument style more consistent with clang-tidy & LLVM

[kesyog]

Thanks for the suggestions. I removed -no-use-color and modified -use-color to follow the behavior of LLVM's CLI parser. I also fixed the (very unintentional) file mode change.

[njames93]

LGTM

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

69

Didn't even realise an empty string was acceptable in LLVM CLI. Learn something new everyday.

[kesyog]

I don't have commit privileges, so I'll need help committing this to main. My name is "Kesavan Yogeswaran" and my email is hikes [at] google [dot] com

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

 is_py2 = sys.version[0] == '2'
 
 if is_py2:
     import Queue as queue
 else:
     import queue as queue
 
 
+def strtobool(val):
+  """Convert a string representation of truth to a bool following LLVM's CLI argument parsing."""
+
+  val = val.lower()
+  if val in ['', 'true', '1']:
+    return True
+  elif val in ['false', '0']:
+    return False
+
+  # Return ArgumentTypeError so that argparse does not substitute its own error message
+  raise argparse.ArgumentTypeError(
+    "'{}' is invalid value for boolean argument! Try 0 or 1.".format(val)
+  )
+
+
 def find_compilation_database(path):
   """Adjusts the directory until a compilation database is found."""
   result = './'
   while not os.path.isfile(os.path.join(result, path)):
     if os.path.realpath(result) == '/':
       print('Error: could not find compilation database.')
       sys.exit(1)
     result += '../'
   return os.path.realpath(result)
 
 
 def make_absolute(f, directory):
   if os.path.isabs(f):
     return f
   return os.path.normpath(os.path.join(directory, f))
 
 
 def get_tidy_invocation(f, clang_tidy_binary, checks, tmpdir, build_path,
                         header_filter, allow_enabling_alpha_checkers,
                         extra_arg, extra_arg_before, quiet, config,
-                        line_filter):
+                        line_filter, use_color):
   """Gets a command line for clang-tidy."""
-  start = [clang_tidy_binary, '--use-color']
+  start = [clang_tidy_binary]
   if allow_enabling_alpha_checkers:
     start.append('-allow-enabling-analyzer-alpha-checkers')
   if header_filter is not None:
     start.append('-header-filter=' + header_filter)
   if line_filter is not None:
     start.append('-line-filter=' + line_filter)
+  if use_color is not None:
+    if use_color:
+      start.append('--use-color')
+    else:
+      start.append('--use-color=false')
   if checks:
     start.append('-checks=' + checks)
   if tmpdir is not None:
     start.append('-export-fixes')
     # Get a temporary file. We immediately close the handle so clang-tidy can
     # overwrite it.
     (handle, name) = tempfile.mkstemp(suffix='.yaml', dir=tmpdir)
     os.close(handle)

 def run_tidy(args, tmpdir, build_path, queue, lock, failed_files):
   """Takes filenames out of queue and runs clang-tidy on them."""
   while True:
     name = queue.get()
     invocation = get_tidy_invocation(name, args.clang_tidy_binary, args.checks,
                                      tmpdir, build_path, args.header_filter,
                                      args.allow_enabling_alpha_checkers,
                                      args.extra_arg, args.extra_arg_before,
-                                     args.quiet, args.config, args.line_filter)
+                                     args.quiet, args.config, args.line_filter,
+                                     args.use_color)
 
     proc = subprocess.Popen(invocation, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
     output, err = proc.communicate()
     if proc.returncode != 0:
       if proc.returncode < 0:
         msg = "%s: terminated by signal %d\n" % (name, -proc.returncode)
         err += msg.encode('utf-8')
       failed_files.append(name)
parser.add_argument('-j', type=int, default=0,
                       help='number of tidy instances to be run in parallel.')
   parser.add_argument('files', nargs='*', default=['.*'],
                       help='files to be processed (regex on path)')
   parser.add_argument('-fix', action='store_true', help='apply fix-its')
   parser.add_argument('-format', action='store_true', help='Reformat code '
                       'after applying fixes')
   parser.add_argument('-style', default='file', help='The style of reformat '
                       'code after applying fixes')
+  parser.add_argument('-use-color', type=strtobool, nargs='?', const=True,
+                      help='Use colors in diagnostics, overriding clang-tidy\'s'
+                      ' default behavior. This option overrides the \'UseColor'
+                      '\' option in .clang-tidy file, if any.')
   parser.add_argument('-p', dest='build_path',
                       help='Path used to read a compile command database.')
   parser.add_argument('-extra-arg', dest='extra_arg',
                       action='append', default=[],
                       help='Additional argument to append to the compiler '
                       'command line.')
   parser.add_argument('-extra-arg-before', dest='extra_arg_before',
                       action='append', default=[],
                       help='Additional argument to prepend to the compiler '
                       'command line.')
   parser.add_argument('-quiet', action='store_true',
                       help='Run clang-tidy in quiet mode')
   args = parser.parse_args()
 
   db_path = 'compile_commands.json'
 
   if args.build_path is not None:
     build_path = args.build_path
   else:
     # Find our database
     build_path = find_compilation_database(db_path)
 
   try:
     invocation = get_tidy_invocation("", args.clang_tidy_binary, args.checks,
                                      None, build_path, args.header_filter,
                                      args.allow_enabling_alpha_checkers,
                                      args.extra_arg, args.extra_arg_before,
-                                     args.quiet, args.config, args.line_filter)
+                                     args.quiet, args.config, args.line_filter,
+                                     args.use_color)
     invocation.append('-list-checks')
     invocation.append('-')
     if args.quiet:
       # Even with -quiet we still want to check if we can call clang-tidy.
       with open(os.devnull, 'w') as dev_null:
         subprocess.check_call(invocation, stdout=dev_null)
     else:
       subprocess.check_call(invocation)