Add --fix support to clang-tidy runner (#16646)

* Make clang variant as a requirement for asan/tsan builds, add support for required variants for build systems

* Restyle

* Also update workflows

* Adding a clang-tidy helper script that is outside the build process.

Replaces the pw_command_runner path we had before and allows
independent execution of the checker.

* Remove pw command launcher from darwin as well. clang tidy on linux should be sufficient for now

* Use clang builds and validate more compile databases

* Adjust test group ordering since we have placed clang as a last variant

* More moving of chip tool variants to make the options in yaml file make sense

* add missin $ for var specifier

* Add clang variant to tsan

* Asan/tsan not limited by clang, so updated as such

* Restyle

* Ensure darwin clang-tidy is also run

* Ensure compile commands are exported

* Update to use same coverage for tidy for linux as well as before

* Undo changes to TestCommand

* Remove modernize-nullptr for now: it is still too strict

* Select individual OS compilations and do not compile gcc variants on mac

* It looks like IOS is always compiled with gcc - see build/toolchain/build/ios. Do not attempt to clang-enable it nor clang-tidy

* Tidy gcc/g++ as well

* fix typo

* Remove PWcommandlauncher from default build as well

* Bump up the timeout value for clang validation a lot just in case

* Make code easier to read

* Fix darwin paths: when using gcc/g++, sysroot is required

* More robust gcc finding of sysroot

* Typo fix and restyle

* Fix support to clangtidy

* Add support for specific checks to be run. Tested with modernize-redundant-void-arg (quite a few found)

* Disabled optin-osx-cocoa-localizability-emptylocalizationcontextchecker-objc for clang tidy default

* Fix optin to be case correct: clang-analyzer-optin.osx.cocoa.localizability.EmptyLocalizationContextChecker

* Restyle

scripts/run-clang-tidy-on-compile-commands.py

@@ -18,6 +18,13 @@
 
 ./scripts/run-clang-tidy-on-compile-commands.py check
 
+# Run and output a fix yaml
+
+./scripts/run-clang-tidy-on-compile-commands.py --export-fixes out/fixes.yaml check
+
+# Apply the fixes
+clang-apply-replacements out/fixes.yaml
+
 """
 
 import build
@@ -28,13 +35,15 @@
 import logging
 import multiprocessing
 import os
+import queue
 import re
 import shlex
 import subprocess
 import sys
+import tempfile
 import threading
 import traceback
-import queue
+import yaml
 
 
 class TidyResult:
@@ -97,10 +106,15 @@ def ExportFixesTo(self, f: str):
         self.tidy_arguments.append("--export-fixes")
         self.tidy_arguments.append(f)
 
+    def SetChecks(self, checks: str):
+        self.tidy_arguments.append("--checks")
+        self.tidy_arguments.append(checks)
+
     def Check(self):
         logging.debug("Running tidy on %s from %s", self.file, self.directory)
         try:
-            cmd = ["clang-tidy", self.file, "--"] + self.clang_arguments
+            cmd = ["clang-tidy", self.file] + \
+                self.tidy_arguments + ["--"] + self.clang_arguments
             logging.debug("Executing: %r" % cmd)
 
             proc = subprocess.Popen(
@@ -172,6 +186,8 @@ class ClangTidyRunner:
     def __init__(self):
         self.entries = []
         self.state = TidyState()
+        self.fixes_file = None
+        self.fixes_temporary_file_dir = None
         self.gcc_sysroot = None
 
         if sys.platform == 'darwin':
@@ -192,11 +208,67 @@ def AddDatabase(self, compile_commands_json):
 
             self.entries.append(item)
 
+    def Cleanup(self):
+        if self.fixes_temporary_file_dir:
+            all_diagnostics = []
+
+            # When running over several files, fixes may be applied to the same
+            # file over and over again, like 'append override' can result in the
+            # same override being appended multiple times.
+            already_seen = set()
+            for name in glob.iglob(
+                os.path.join(self.fixes_temporary_file_dir.name, "*.yaml")
+            ):
+                content = yaml.safe_load(open(name, "r"))
+                if not content:
+                    continue
+                diagnostics = content.get("Diagnostics", [])
+
+                # Allow all diagnostics for distinct paths to be applied
+                # at once but never again for future paths
+                for d in diagnostics:
+                    if d['DiagnosticMessage']['FilePath'] not in already_seen:
+                        all_diagnostics.append(d)
+
+                # in the future assume these files were already processed
+                for d in diagnostics:
+                    already_seen.add(d['DiagnosticMessage']['FilePath'])
+
+            if all_diagnostics:
+                with open(self.fixes_file, "w") as out:
+                    yaml.safe_dump(
+                        {"MainSourceFile": "", "Diagnostics": all_diagnostics}, out
+                    )
+            else:
+                open(self.fixes_file, "w").close()
+
+            logging.info(
+                "Cleaning up directory: %r", self.fixes_temporary_file_dir.name
+            )
+            self.fixes_temporary_file_dir.cleanup()
+            self.fixes_temporary_file_dir = None
+
     def ExportFixesTo(self, f):
         # use absolute path since running things will change working directories
-        f = os.path.abspath(f)
+        self.fixes_file = os.path.abspath(f)
+        self.fixes_temporary_file_dir = tempfile.TemporaryDirectory(
+            prefix="tidy-", suffix="-fixes"
+        )
+
+        logging.info(
+            "Storing temporary fix files into %s", self.fixes_temporary_file_dir.name
+        )
+        for idx, e in enumerate(self.entries):
+            e.ExportFixesTo(
+                os.path.join(
+                    self.fixes_temporary_file_dir.name, "fixes%d.yaml" % (
+                        idx + 1,)
+                )
+            )
+
+    def SetChecks(self, checks: str):
         for e in self.entries:
-            e.ExportFixesTo(f)
+            e.SetChecks(checks)
 
     def FilterEntries(self, f):
         for e in self.entries:
@@ -235,7 +307,7 @@ def Check(self):
             for name in self.state.failed_files:
                 logging.warning("Failure reported for %s", name)
 
-            sys.exit(1)
+        return self.state.failures == 0
 
 
 # Supported log levels, mapping string values required for argument
@@ -283,7 +355,13 @@ def Check(self):
     "--export-fixes",
     default=None,
     type=click.Path(),
-    help="Where to export fixes to apply. TODO(fix apply not yet implemented).",
+    help="Where to export fixes to apply.",
+)
+@click.option(
+    "--checks",
+    default=None,
+    type=str,
+    help="Checks to run (passed in to clang-tidy). If not set the .clang-tidy file is used.",
 )
 @click.pass_context
 def main(
@@ -294,6 +372,7 @@ def main(
     log_level,
     no_log_timestamps,
     export_fixes,
+    checks,
 ):
     log_fmt = "%(asctime)s %(levelname)-7s %(message)s"
     if no_log_timestamps:
@@ -311,21 +390,28 @@ def main(
             raise Exception("Could not find `compile_commands.json` in ./out")
         logging.info("Will use %s for compile", compile_database)
 
-    context.obj = ClangTidyRunner()
+    context.obj = runner = ClangTidyRunner()
+
+    @context.call_on_close
+    def cleanup():
+        runner.Cleanup()
 
     for name in compile_database:
-        context.obj.AddDatabase(name)
+        runner.AddDatabase(name)
 
     if file_include_regex:
         r = re.compile(file_include_regex)
-        context.obj.FilterEntries(lambda e: r.search(e.file))
+        runner.FilterEntries(lambda e: r.search(e.file))
 
     if file_exclude_regex:
         r = re.compile(file_exclude_regex)
-        context.obj.FilterEntries(lambda e: not r.search(e.file))
+        runner.FilterEntries(lambda e: not r.search(e.file))
 
     if export_fixes:
-        context.obj.ExportFixesTo(export_fixes)
+        runner.ExportFixesTo(export_fixes)
+
+    if checks:
+        runner.SetChecks(checks)
 
     for e in context.obj.entries:
         logging.info("Will tidy %s", e.full_path)
@@ -334,7 +420,30 @@ def main(
 @main.command("check", help="Run clang-tidy check")
 @click.pass_context
 def cmd_check(context):
-    context.obj.Check()
+    if not context.obj.Check():
+        sys.exit(1)
+
+
+@main.command("fix", help="Run check followd by fix")
+@click.pass_context
+def cmd_fix(context):
+    runner = context.obj
+    with tempfile.TemporaryDirectory(prefix="tidy-apply-fixes") as tmpdir:
+        if not runner.fixes_file:
+            runner.ExportFixesTo(os.path.join(tmpdir, "fixes.tmp"))
+
+        runner.Check()
+        runner.Cleanup()
+
+        if runner.state.failures:
+            fixes_yaml = os.path.join(tmpdir, "fixes.yaml")
+            with open(fixes_yaml, "w") as out:
+                out.write(open(runner.fixes_file, "r").read())
+
+            logging.info("Applying fixes in %s", tmpdir)
+            subprocess.check_call(["clang-apply-replacements", tmpdir])
+        else:
+            logging.info("No failures detected, no fixes to apply.")
 
 
 if __name__ == "__main__":