[clang-tidy] Improve add_new_check.py to recognize more checks

[LegalizeAdulthood]

Summary

When looking for whether or not a check provides fixits, the script
examines the implementation of the check. Some checks are not
implemented in source files that correspond one-to-one with the check
name, e.g. cert-dcl21-cpp. So if we can't find the check implementation
directly from the check name, open up the corresponding module file and
look for the class name that is registered with the check. Then consult
the file corresponding to the class name.

Some checks are derived from a base class that implements fixits. So if
we can't find fixits in the implementation file for a check, scrape out
the name of it's base class. If it's not ClangTidyCheck, then consult
the base class implementation to look for fixit support.

Fixes #55630

[aaron.ballman]

Thanks for this! It generally LGTM (though my Python skills are not particularly awesome, so take it with a grain of salt). Just a few questions at this point.

clang-tools-extra/clang-tidy/add_new_check.py

335–336

Do we have to check this or can we rely on open failing because it's not a file? (It tripped my psychic TOCTOU sensor.)

352

It's a bit early for me to fully grok regex, but: is this going to handle line continuations/newlines okay? I don't know if those show up in cases that matter right now, but I wanted to make sure it was being thought about.

371

[LegalizeAdulthood]

clang-tools-extra/clang-tidy/add_new_check.py

335–336

If we don't check here, then it throws an exception when attempting to open the file.

352

\s matches any single whitespace character and \s* matches zero or more whitespace characters. I could sprinkle some more in between tokens, but this catches the existing code correctly.

[LegalizeAdulthood]

Another observation:
If some new pattern comes up and fixits aren't recognized for a check, it might be better
to switch to a whitelist for checks with fixits rather than going crazier on the file scraping.

[LegalizeAdulthood]

clang-tools-extra/clang-tidy/add_new_check.py

352

There is some code like this: CheckFactories.registerCheck<bugprone::UnhandledSelfAssignmentCheck>("cert-oop54-cpp"); and I intentionally omit searching for this ([^:>]*) because these are aliases for other checks.

Which makes me wonder if we want to explicitly register aliases differently from regular checks? If you want to run a list of checks and exclude aliases (so the check doesn't run twice), how do you do that?

[LegalizeAdulthood]

Update from review comments

[LegalizeAdulthood]

clang-tools-extra/clang-tidy/add_new_check.py

335–336

Also, I was following the existing pattern in this file :)

[aaron.ballman]

LGTM, thank you!

clang-tools-extra/clang-tidy/add_new_check.py

335–336

Sounds like a good reason to leave it in then, thanks!

352

Which makes me wonder if we want to explicitly register aliases differently from regular checks? If you want to run a list of checks and exclude aliases (so the check doesn't run twice), how do you do that?

You can't, which was the subject of https://reviews.llvm.org/D114317 but it's complicated in practice.

clang-tools-extra/clang-tidy/add_new_check.py

 } // namespace %(namespace)s
 } // namespace tidy
 } // namespace clang
 """ % {'check_name': check_name_camel,
        'module': module,
        'namespace': namespace})
 
 
-# Modifies the module to include the new check.
-def adapt_module(module_path, module, check_name, check_name_camel):
+# Returns the source filename that implements the module.
+def get_module_filename(module_path, module):
   modulecpp = list(filter(
       lambda p: p.lower() == module.lower() + 'tidymodule.cpp',
       os.listdir(module_path)))[0]
-  filename = os.path.join(module_path, modulecpp)
+  return os.path.join(module_path, modulecpp)
+
+
+# Modifies the module to include the new check.
+def adapt_module(module_path, module, check_name, check_name_camel):
+  filename = get_module_filename(module_path, module)
   with io.open(filename, 'r', encoding='utf8') as f:
     lines = f.readlines()
 
   print('Updating %s...' % filename)
   with io.open(filename, 'w', encoding='utf8', newline='\n') as f:
     header_added = False
     header_found = False
     check_added = False
def update_checks_list(clang_tidy_path):
   # Read the content of the current list.rst file
   with io.open(filename, 'r', encoding='utf8') as f:
     lines = f.readlines()
   # Get all existing docs
   doc_files = list(filter(lambda s: s.endswith('.rst') and s != 'list.rst',
                      os.listdir(docs_dir)))
   doc_files.sort()
 
+  # We couldn't find the source file from the check name, so try to find the
+  # class name that corresponds to the check in the module file.
+  def filename_from_module(module_name, check_name):
+    module_path = os.path.join(clang_tidy_path, module_name)
+    if not os.path.isdir(module_path):
+      return ''
+    module_file = get_module_filename(module_path, module_name)
+    if not os.path.isfile(module_file):
+      return ''
+    with io.open(module_file, 'r') as f:
+      code = f.read()
+      full_check_name = module_name + '-' + check_name
+      name_pos = code.find('"' + full_check_name + '"')
+      if name_pos == -1:
+        return ''
+      stmt_end_pos = code.find(';', name_pos)
+      if stmt_end_pos == -1:
+        return ''
+      stmt_start_pos = code.rfind(';', 0, name_pos)
+      if stmt_start_pos == -1:
+        stmt_start_pos = code.rfind('{', 0, name_pos)
+      if stmt_start_pos == -1:
+        return ''
+      stmt = code[stmt_start_pos+1:stmt_end_pos]
+      matches = re.search('registerCheck<([^>:]*)>\(\s*"([^"]*)"\s*\)', stmt)
+      if matches and matches[2] == full_check_name:
+        class_name = matches[1]
+        if '::' in class_name:
+          parts = class_name.split('::')
+          class_name = parts[-1]
+          class_path = os.path.join(clang_tidy_path, module_name, '..', *parts[0:-1])
+        else:
+          class_path = os.path.join(clang_tidy_path, module_name)
+        return get_actual_filename(class_path, class_name + '.cpp')
+
+    return ''
+
+  # Examine code looking for a c'tor definition to get the base class name.
+  def get_base_class(code, check_file):
+    check_class_name = os.path.splitext(os.path.basename(check_file))[0]
+    ctor_pattern = check_class_name + '\([^:]*\)\s*:\s*([A-Z][A-Za-z0-9]*Check)\('
+    matches = re.search('\s+' + check_class_name + '::' + ctor_pattern, code)
+
+    # The constructor might be inline in the header.
+    if not matches:
+      header_file = os.path.splitext(check_file)[0] + '.h'
+      if not os.path.isfile(header_file):
+        return ''
+      with io.open(header_file, encoding='utf8') as f:
+        code = f.read()
+      matches = re.search(' ' + ctor_pattern, code)
+
+    if matches and matches[1] != 'ClangTidyCheck':
+      return matches[1]
+    return ''
+
+  # Some simple heuristics to figure out if a check has an autofix or not.
+  def has_fixits(code):
+    for needle in ['FixItHint', 'ReplacementText', 'fixit',
+                   'TransformerClangTidyCheck']:
+      if needle in code:
+        return True
+    return False
+
+  # Try to figure out of the check supports fixits.
   def has_auto_fix(check_name):
     dirname, _, check_name = check_name.partition('-')
 
-    checker_code = get_actual_filename(os.path.join(clang_tidy_path, dirname),
+    check_file = get_actual_filename(os.path.join(clang_tidy_path, dirname),
                                        get_camel_check_name(check_name) + '.cpp')
-    if not os.path.isfile(checker_code):
+    if not os.path.isfile(check_file):
       # Some older checks don't end with 'Check.cpp'
-      checker_code = get_actual_filename(os.path.join(clang_tidy_path, dirname),
+      check_file = get_actual_filename(os.path.join(clang_tidy_path, dirname),
                                          get_camel_name(check_name) + '.cpp')
-      if not os.path.isfile(checker_code):
+      if not os.path.isfile(check_file):
+        # Some checks aren't in a file based on the check name.
+        check_file = filename_from_module(dirname, check_name)
+        if not check_file or not os.path.isfile(check_file):
           return ''
 
-    with io.open(checker_code, encoding='utf8') as f:
+    with io.open(check_file, encoding='utf8') as f:
       code = f.read()
-      for needle in ['FixItHint', 'ReplacementText', 'fixit', 'TransformerClangTidyCheck']:
-        if needle in code:
-          # Some simple heuristics to figure out if a checker has an autofix or not.
-          return ' "Yes"'
+      if has_fixits(code):
+
+    base_class = get_base_class(code, check_file)
+    if base_class:
+      base_file = os.path.join(clang_tidy_path, dirname, base_class + '.cpp')
+      if os.path.isfile(base_file):
+        with io.open(base_file, encoding='utf8') as f:
+          code = f.read()
+          if has_fixits(code):
+            return ' "Yes"'
+
     return ''
 
   def process_doc(doc_file):
     check_name = doc_file.replace('.rst', '')
 
     with io.open(os.path.join(docs_dir, doc_file), 'r', encoding='utf8') as doc:
       content = doc.read()
       match = re.search('.*:orphan:.*', content)

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `abseil-time-subtraction <abseil-time-subtraction.html>`_, "Yes"
    `abseil-upgrade-duration-conversions <abseil-upgrade-duration-conversions.html>`_, "Yes"
    `altera-id-dependent-backward-branch <altera-id-dependent-backward-branch.html>`_,
    `altera-kernel-name-restriction <altera-kernel-name-restriction.html>`_,
    `altera-single-work-item-barrier <altera-single-work-item-barrier.html>`_,
    `altera-struct-pack-align <altera-struct-pack-align.html>`_, "Yes"
    `altera-unroll-loops <altera-unroll-loops.html>`_,
    `android-cloexec-accept <android-cloexec-accept.html>`_, "Yes"
-   `android-cloexec-accept4 <android-cloexec-accept4.html>`_,
+   `android-cloexec-accept4 <android-cloexec-accept4.html>`_, "Yes"
    `android-cloexec-creat <android-cloexec-creat.html>`_, "Yes"
    `android-cloexec-dup <android-cloexec-dup.html>`_, "Yes"
-   `android-cloexec-epoll-create <android-cloexec-epoll-create.html>`_,
-   `android-cloexec-epoll-create1 <android-cloexec-epoll-create1.html>`_,
-   `android-cloexec-fopen <android-cloexec-fopen.html>`_,
-   `android-cloexec-inotify-init <android-cloexec-inotify-init.html>`_,
-   `android-cloexec-inotify-init1 <android-cloexec-inotify-init1.html>`_,
-   `android-cloexec-memfd-create <android-cloexec-memfd-create.html>`_,
-   `android-cloexec-open <android-cloexec-open.html>`_,
+   `android-cloexec-epoll-create <android-cloexec-epoll-create.html>`_, "Yes"
+   `android-cloexec-epoll-create1 <android-cloexec-epoll-create1.html>`_, "Yes"
+   `android-cloexec-fopen <android-cloexec-fopen.html>`_, "Yes"
+   `android-cloexec-inotify-init <android-cloexec-inotify-init.html>`_, "Yes"
+   `android-cloexec-inotify-init1 <android-cloexec-inotify-init1.html>`_, "Yes"
+   `android-cloexec-memfd-create <android-cloexec-memfd-create.html>`_, "Yes"
+   `android-cloexec-open <android-cloexec-open.html>`_, "Yes"
    `android-cloexec-pipe <android-cloexec-pipe.html>`_, "Yes"
-   `android-cloexec-pipe2 <android-cloexec-pipe2.html>`_,
-   `android-cloexec-socket <android-cloexec-socket.html>`_,
+   `android-cloexec-pipe2 <android-cloexec-pipe2.html>`_, "Yes"
+   `android-cloexec-socket <android-cloexec-socket.html>`_, "Yes"
    `android-comparison-in-temp-failure-retry <android-comparison-in-temp-failure-retry.html>`_,
    `boost-use-to-string <boost-use-to-string.html>`_, "Yes"
    `bugprone-argument-comment <bugprone-argument-comment.html>`_, "Yes"
    `bugprone-assert-side-effect <bugprone-assert-side-effect.html>`_,
    `bugprone-bad-signal-to-kill-thread <bugprone-bad-signal-to-kill-thread.html>`_,
    `bugprone-bool-pointer-implicit-conversion <bugprone-bool-pointer-implicit-conversion.html>`_, "Yes"
    `bugprone-branch-clone <bugprone-branch-clone.html>`_,
    `bugprone-copy-constructor-init <bugprone-copy-constructor-init.html>`_, "Yes"
.. csv-table::
    `bugprone-suspicious-memset-usage <bugprone-suspicious-memset-usage.html>`_, "Yes"
    `bugprone-suspicious-missing-comma <bugprone-suspicious-missing-comma.html>`_,
    `bugprone-suspicious-semicolon <bugprone-suspicious-semicolon.html>`_, "Yes"
    `bugprone-suspicious-string-compare <bugprone-suspicious-string-compare.html>`_, "Yes"
    `bugprone-swapped-arguments <bugprone-swapped-arguments.html>`_, "Yes"
    `bugprone-terminating-continue <bugprone-terminating-continue.html>`_, "Yes"
    `bugprone-throw-keyword-missing <bugprone-throw-keyword-missing.html>`_,
    `bugprone-too-small-loop-variable <bugprone-too-small-loop-variable.html>`_,
-   `bugprone-unchecked-optional-access <bugprone-unchecked-optional-access.html>`_, "Yes"
+   `bugprone-unchecked-optional-access <bugprone-unchecked-optional-access.html>`_,
    `bugprone-undefined-memory-manipulation <bugprone-undefined-memory-manipulation.html>`_,
    `bugprone-undelegated-constructor <bugprone-undelegated-constructor.html>`_,
    `bugprone-unhandled-exception-at-new <bugprone-unhandled-exception-at-new.html>`_,
    `bugprone-unhandled-self-assignment <bugprone-unhandled-self-assignment.html>`_,
    `bugprone-unused-raii <bugprone-unused-raii.html>`_, "Yes"
    `bugprone-unused-return-value <bugprone-unused-return-value.html>`_,
    `bugprone-use-after-move <bugprone-use-after-move.html>`_,
    `bugprone-virtual-near-miss <bugprone-virtual-near-miss.html>`_, "Yes"