[clang-tidy] Refactor: Extract Class CheckRunner on check_clang_tidy.py

[LegalizeAdulthood]

Summary

Break up the huge function by extracting a class, storing intermediate
state as class members and breaking up the big function into a group
of class methods all at the same level of abstraction.

[LegalizeAdulthood]

Extract Function try_run

[JonasToth]

test/clang-tidy/check_clang_tidy.py

112 ↗

(On Diff #180349)

Its better to use .format() instead of % syntax

206 ↗

(On Diff #180349)

If would prefer keeping the if check_notes outside of the function call and remove that one argument. Same for the other check_... functions

[LegalizeAdulthood]

I really want to get these reviewed quickly. Otherwise, I will run out of available time to complete them and get them submitted.

[JonasToth]

In D56343#1347352, @LegalizeAdulthood wrote:

I really want to get these reviewed quickly. Otherwise, I will run out of available time to complete them and get them submitted.

I will give my best to be available this weekend but the review takes time until the patch is in an acceptable state, same for the other revision.
This process is required to keep good quality and catch bugs, even subtile ones early. Breaking code is the worst of all options.

[serge-sans-paille]

test/clang-tidy/check_clang_tidy.py

103 ↗

(On Diff #180349)

Such a long list of returned values make me wonder if it would be worth creating a NamedTuple / class to hold them.
This may make the call site more readable too.

141 ↗

(On Diff #180349)

These three check_* function all use the same 'FileCheck', '-....'] pattern. Maybe it's worth syndicating that to a try_run_filecheck(input_file0, input_file1, *extra_args)` function.

178 ↗

(On Diff #180349)

This is the verbose call site I was pointing to above.

[LegalizeAdulthood]

To avoid passing lots of state to/from extracted functions, extract a class instead and use member variables for most of the state

[JonasToth]

From my side its LGTM, but I would let @serge-sans-paille accept, as he is probably more familiar with python then I am.

[LegalizeAdulthood]

test/clang-tidy/check_clang_tidy.py

112 ↗

(On Diff #180349)

That can be done in a later commit; it is not the point of this review.

141 ↗

(On Diff #180349)

That can be done in a later commit; it is not the point of this review.

178 ↗

(On Diff #180349)

Addressed by Extract Class instead of Extract Function

206 ↗

(On Diff #180349)

It's the wrong level of abstraction for the if check to be inside run()

[serge-sans-paille]

Sorry for the long delay, this LGTM

[LegalizeAdulthood]

Rebase on top of tree

[LegalizeAdulthood]

This was accepted before in August, 2019, but the script changed in the meantime. I've repeated the Extract Class on top-of-tree,
so presumably this is still acceptable. However, I don't know how commit access has changed. (I don't think I have it anymore.)

[LegalizeAdulthood]

If there are no new comments by the end of the week, I'm going to accept this revision and commit it.

[aaron.ballman]

Resigning only because I don't think I have strong enough Python skills to give a quality review. I didn't spot anything that concerned me specific to this patch though.

clang-tools-extra/test/clang-tidy/check_clang_tidy.py

 
 
 def write_file(file_name, text):
   with open(file_name, 'w', encoding='utf-8') as f:
     f.write(text)
     f.truncate()
 
 
-def run_test_once(args, extra_args):
-  resource_dir = args.resource_dir
-  assume_file_name = args.assume_filename
-  input_file_name = args.input_file_name
-  check_name = args.check_name
-  temp_file_name = args.temp_file_name
-  expect_clang_tidy_error = args.expect_clang_tidy_error
-  std = args.std
+def try_run(args, raise_error=True):
+  try:
+    process_output = \
+      subprocess.check_output(args, stderr=subprocess.STDOUT).decode(errors='ignore')
+  except subprocess.CalledProcessError as e:
+    process_output = e.output.decode(errors='ignore')
+    print('%s failed:\n%s' % (' '.join(args), process_output))
+    if raise_error:
+      raise
+  return process_output
+
 
-  file_name_with_extension = assume_file_name or input_file_name
+class CheckRunner:
+  def __init__(self, args, extra_args):
+    self.resource_dir = args.resource_dir
+    self.assume_file_name = args.assume_filename
+    self.input_file_name = args.input_file_name
+    self.check_name = args.check_name
+    self.temp_file_name = args.temp_file_name
+    self.original_file_name = self.temp_file_name + ".orig"
+    self.expect_clang_tidy_error = args.expect_clang_tidy_error
+    self.std = args.std
+    self.check_suffix = args.check_suffix
+    self.input_text = ''
+    self.check_fixes_prefixes = []
+    self.check_messages_prefixes = []
+    self.check_notes_prefixes = []
+    self.has_check_fixes = False
+    self.has_check_messages = False
+    self.has_check_notes = False
+
+    file_name_with_extension = self.assume_file_name or self.input_file_name
     _, extension = os.path.splitext(file_name_with_extension)
     if extension not in ['.c', '.hpp', '.m', '.mm']:
       extension = '.cpp'
-  temp_file_name = temp_file_name + extension
+    self.temp_file_name = self.temp_file_name + extension
 
-  clang_tidy_extra_args = extra_args
-  clang_extra_args = []
+    self.clang_extra_args = []
+    self.clang_tidy_extra_args = extra_args
     if '--' in extra_args:
-    i = clang_tidy_extra_args.index('--')
-    clang_extra_args = clang_tidy_extra_args[i + 1:]
-    clang_tidy_extra_args = clang_tidy_extra_args[:i]
+      i = self.clang_tidy_extra_args.index('--')
+      self.clang_extra_args = self.clang_tidy_extra_args[i + 1:]
+      self.clang_tidy_extra_args = self.clang_tidy_extra_args[:i]
 
     # If the test does not specify a config style, force an empty one; otherwise
-  # autodetection logic can discover a ".clang-tidy" file that is not related to
+    # auto-detection logic can discover a ".clang-tidy" file that is not related to
     # the test.
     if not any(
-      [arg.startswith('-config=') for arg in clang_tidy_extra_args]):
-    clang_tidy_extra_args.append('-config={}')
+        [arg.startswith('-config=') for arg in self.clang_tidy_extra_args]):
+      self.clang_tidy_extra_args.append('-config={}')
 
     if extension in ['.m', '.mm']:
-    clang_extra_args = ['-fobjc-abi-version=2', '-fobjc-arc', '-fblocks'] + \
-        clang_extra_args
+      self.clang_extra_args = ['-fobjc-abi-version=2', '-fobjc-arc', '-fblocks'] + \
+          self.clang_extra_args
 
     if extension in ['.cpp', '.hpp', '.mm']:
-    clang_extra_args.append('-std=' + std)
+      self.clang_extra_args.append('-std=' + self.std)
 
     # Tests should not rely on STL being available, and instead provide mock
     # implementations of relevant APIs.
-  clang_extra_args.append('-nostdinc++')
-
-  if resource_dir is not None:
-    clang_extra_args.append('-resource-dir=%s' % resource_dir)
+    self.clang_extra_args.append('-nostdinc++')
 
-  with open(input_file_name, 'r', encoding='utf-8') as input_file:
-    input_text = input_file.read()
+    if self.resource_dir is not None:
+      self.clang_extra_args.append('-resource-dir=%s' % self.resource_dir)
 
-  check_fixes_prefixes = []
-  check_messages_prefixes = []
-  check_notes_prefixes = []
-
-  has_check_fixes = False
-  has_check_messages = False
-  has_check_notes = False
-
-  for check in args.check_suffix:
-    if check and not re.match('^[A-Z0-9\-]+$', check):
-      sys.exit('Only A..Z, 0..9 and "-" are ' +
-        'allowed in check suffixes list, but "%s" was given' % (check))
+  def read_input(self):
+    with open(self.input_file_name, 'r', encoding='utf-8') as input_file:
+      self.input_text = input_file.read()
+
+  def get_prefixes(self):
+    for check in self.check_suffix:
+      if check and not re.match('^[A-Z0-9\\-]+$', check):
+        sys.exit('Only A..Z, 0..9 and "-" are allowed in check suffixes list,'
+                 + ' but "%s" was given' % check)
 
       file_check_suffix = ('-' + check) if check else ''
       check_fixes_prefix = 'CHECK-FIXES' + file_check_suffix
       check_messages_prefix = 'CHECK-MESSAGES' + file_check_suffix
       check_notes_prefix = 'CHECK-NOTES' + file_check_suffix
 
-    has_check_fix = check_fixes_prefix in input_text
-    has_check_message = check_messages_prefix in input_text
-    has_check_note = check_notes_prefix in input_text
+      has_check_fix = check_fixes_prefix in self.input_text
+      has_check_message = check_messages_prefix in self.input_text
+      has_check_note = check_notes_prefix in self.input_text
 
       if has_check_note and has_check_message:
         sys.exit('Please use either %s or %s but not both' %
           (check_notes_prefix, check_messages_prefix))
 
       if not has_check_fix and not has_check_message and not has_check_note:
         sys.exit('%s, %s or %s not found in the input' %
           (check_fixes_prefix, check_messages_prefix, check_notes_prefix))
 
-    has_check_fixes = has_check_fixes or has_check_fix
-    has_check_messages = has_check_messages or has_check_message
-    has_check_notes = has_check_notes or has_check_note
+      self.has_check_fixes = self.has_check_fixes or has_check_fix
+      self.has_check_messages = self.has_check_messages or has_check_message
+      self.has_check_notes = self.has_check_notes or has_check_note
 
       if has_check_fix:
-      check_fixes_prefixes.append(check_fixes_prefix)
+        self.check_fixes_prefixes.append(check_fixes_prefix)
       if has_check_message:
-      check_messages_prefixes.append(check_messages_prefix)
+        self.check_messages_prefixes.append(check_messages_prefix)
       if has_check_note:
-      check_notes_prefixes.append(check_notes_prefix)
+        self.check_notes_prefixes.append(check_notes_prefix)
 
-  assert has_check_fixes or has_check_messages or has_check_notes
+    assert self.has_check_fixes or self.has_check_messages or self.has_check_notes
+
+  def prepare_test_inputs(self):
     # Remove the contents of the CHECK lines to avoid CHECKs matching on
     # themselves.  We need to keep the comments to preserve line numbers while
     # avoiding empty lines which could potentially trigger formatting-related
     # checks.
-  cleaned_test = re.sub('// *CHECK-[A-Z0-9\-]*:[^\r\n]*', '//', input_text)
-
-  write_file(temp_file_name, cleaned_test)
-
-  original_file_name = temp_file_name + ".orig"
-  write_file(original_file_name, cleaned_test)
-
-  args = ['clang-tidy', temp_file_name, '-fix', '--checks=-*,' + check_name] + \
-      clang_tidy_extra_args + ['--'] + clang_extra_args
-  if expect_clang_tidy_error:
+    cleaned_test = re.sub('// *CHECK-[A-Z0-9\\-]*:[^\r\n]*', '//', self.input_text)
+    write_file(self.temp_file_name, cleaned_test)
+    write_file(self.original_file_name, cleaned_test)
+
+  def run_clang_tidy(self):
+    args = ['clang-tidy', self.temp_file_name, '-fix', '--checks=-*,' + self.check_name] + \
+        self.clang_tidy_extra_args + ['--'] + self.clang_extra_args
+    if self.expect_clang_tidy_error:
       args.insert(0, 'not')
     print('Running ' + repr(args) + '...')
-  try:
-    clang_tidy_output = \
-        subprocess.check_output(args, stderr=subprocess.STDOUT).decode()
-  except subprocess.CalledProcessError as e:
-    print('clang-tidy failed:\n' + e.output.decode())
-    raise
-
+    clang_tidy_output = try_run(args)
     print('------------------------ clang-tidy output -----------------------')
     print(clang_tidy_output.encode())
     print('\n------------------------------------------------------------------')
 
-  try:
-    diff_output = subprocess.check_output(
-        ['diff', '-u', original_file_name, temp_file_name],
-        stderr=subprocess.STDOUT)
-  except subprocess.CalledProcessError as e:
-    diff_output = e.output
-
+    diff_output = try_run(['diff', '-u', self.original_file_name, self.temp_file_name], False)
     print('------------------------------ Fixes -----------------------------\n' +
-        diff_output.decode(errors='ignore') +
+          diff_output +
           '\n------------------------------------------------------------------')
+    return clang_tidy_output
 
-  if has_check_fixes:
-    try:
-      subprocess.check_output(
-          ['FileCheck', '-input-file=' + temp_file_name, input_file_name,
-           '-check-prefixes=' + ','.join(check_fixes_prefixes),
-           '-strict-whitespace'],
-          stderr=subprocess.STDOUT)
-    except subprocess.CalledProcessError as e:
-      print('FileCheck failed:\n' + e.output.decode())
-      raise
-
-  if has_check_messages:
-    messages_file = temp_file_name + '.msg'
+  def check_fixes(self):
+    if self.has_check_fixes:
+      try_run(['FileCheck', '-input-file=' + self.temp_file_name, self.input_file_name,
+              '-check-prefixes=' + ','.join(self.check_fixes_prefixes),
+              '-strict-whitespace'])
+
+  def check_messages(self, clang_tidy_output):
+    if self.has_check_messages:
+      messages_file = self.temp_file_name + '.msg'
       write_file(messages_file, clang_tidy_output)
-    try:
-      subprocess.check_output(
-          ['FileCheck', '-input-file=' + messages_file, input_file_name,
-           '-check-prefixes=' + ','.join(check_messages_prefixes),
-           '-implicit-check-not={{warning|error}}:'],
-          stderr=subprocess.STDOUT)
-    except subprocess.CalledProcessError as e:
-      print('FileCheck failed:\n' + e.output.decode())
-      raise
-
-  if has_check_notes:
-    notes_file = temp_file_name + '.notes'
+      try_run(['FileCheck', '-input-file=' + messages_file, self.input_file_name,
+             '-check-prefixes=' + ','.join(self.check_messages_prefixes),
+             '-implicit-check-not={{warning|error}}:'])
+
+  def check_notes(self, clang_tidy_output):
+    if self.has_check_notes:
+      notes_file = self.temp_file_name + '.notes'
       filtered_output = [line for line in clang_tidy_output.splitlines()
-                       if not "note: FIX-IT applied" in line]
+                         if not ("note: FIX-IT applied" in line)]
       write_file(notes_file, '\n'.join(filtered_output))
-    try:
-      subprocess.check_output(
-          ['FileCheck', '-input-file=' + notes_file, input_file_name,
-           '-check-prefixes=' + ','.join(check_notes_prefixes),
-           '-implicit-check-not={{note|warning|error}}:'],
-          stderr=subprocess.STDOUT)
-    except subprocess.CalledProcessError as e:
-      print('FileCheck failed:\n' + e.output.decode())
-      raise
+      try_run(['FileCheck', '-input-file=' + notes_file, self.input_file_name,
+             '-check-prefixes=' + ','.join(self.check_notes_prefixes),
+             '-implicit-check-not={{note|warning|error}}:'])
+
+  def run(self):
+    self.read_input()
+    self.get_prefixes()
+    self.prepare_test_inputs()
+    clang_tidy_output = self.run_clang_tidy()
+    self.check_fixes()
+    self.check_messages(clang_tidy_output)
+    self.check_notes(clang_tidy_output)
 
 
 def expand_std(std):
   if std == 'c++98-or-later':
     return ['c++98', 'c++11', 'c++14', 'c++17', 'c++20']
   if std == 'c++11-or-later':
     return ['c++11', 'c++14', 'c++17', 'c++20']
   if std == 'c++14-or-later':
     return ['c++14', 'c++17', 'c++20']
   if std == 'c++17-or-later':
     return ['c++17', 'c++20']
   if std == 'c++20-or-later':
     return ['c++20']
   return [std]
 
 
 def csv(string):
   return string.split(',')
 
 
-def main():
+def parse_arguments():
   parser = argparse.ArgumentParser()
   parser.add_argument('-expect-clang-tidy-error', action='store_true')
   parser.add_argument('-resource-dir')
   parser.add_argument('-assume-filename')
   parser.add_argument('input_file_name')
   parser.add_argument('check_name')
   parser.add_argument('temp_file_name')
   parser.add_argument(
-      '-check-suffix',
-      '-check-suffixes',
-      default=[''],
-      type=csv,
-      help='comma-separated list of FileCheck suffixes')
   parser.add_argument('-std', type=csv, default=['c++11-or-later'])
+  return parser.parse_known_args()
+
 
-  args, extra_args = parser.parse_known_args()
+def main():
+  args, extra_args = parse_arguments()
 
   abbreviated_stds = args.std
   for abbreviated_std in abbreviated_stds:
     for std in expand_std(abbreviated_std):
       args.std = std
-      run_test_once(args, extra_args)
+      CheckRunner(args, extra_args).run()
 
 
 if __name__ == '__main__':
   main()