diff --git a/Makefile b/Makefile index 94688032c42..63cbef8732a 100644 --- a/Makefile +++ b/Makefile @@ -71,6 +71,7 @@ setup_lint: fi pip install jinja2 pip install -r tools/linter/clang_tidy/requirements.txt + $(PYTHON) -m tools.linter.install.clang_tidy quick_checks: # TODO: This is broken when 'git config submodule.recurse' is 'true' since the @@ -104,9 +105,10 @@ cmakelint: --job 'cmakelint' \ --step 'Run cmakelint' -clang_tidy: - echo "clang-tidy local lint is not yet implemented" - exit 1 +clang-tidy: + @$(PYTHON) tools/actions_local_runner.py \ + $(CHANGED_ONLY) \ + --job 'clang-tidy' toc: @$(PYTHON) tools/actions_local_runner.py \ @@ -117,4 +119,4 @@ toc: lint: flake8 mypy quick_checks cmakelint shellcheck quicklint: CHANGED_ONLY=--changed-only -quicklint: mypy flake8 quick_checks cmakelint shellcheck +quicklint: mypy flake8 quick_checks cmakelint shellcheck clang-tidy diff --git a/tools/actions_local_runner.py b/tools/actions_local_runner.py index 3968057107b..3a0c745da3e 100755 --- a/tools/actions_local_runner.py +++ b/tools/actions_local_runner.py @@ -270,7 +270,8 @@ class ShellCheck(Check): async def quick(self, files: List[str]) -> CommandResult: return await shell_cmd( - ["tools/linter/run_shellcheck.sh"] + [os.path.join(REPO_ROOT, f) for f in files], + ["tools/linter/run_shellcheck.sh"] + + [os.path.join(REPO_ROOT, f) for f in files], ) async def full(self) -> None: @@ -289,6 +290,30 @@ class ShellCheck(Check): ) +class ClangTidy(Check): + name = "clang-tidy: Run clang-tidy" + common_options = [ + "--clang-tidy-exe", + ".clang-tidy-bin/clang-tidy", + "--parallel", + ] + + def filter_files(self, files: List[str]) -> List[str]: + return self.filter_ext(files, {".c", ".cc", ".cpp"}) + + async def quick(self, files: List[str]) -> CommandResult: + return await shell_cmd( + [sys.executable, "tools/linter/clang_tidy", "--paths"] + + [os.path.join(REPO_ROOT, f) for f in files] + + self.common_options, + ) + + async def full(self) -> None: + await shell_cmd( + [sys.executable, "tools/linter/clang_tidy"] + self.common_options + ) + + class YamlStep(Check): def __init__(self, step: Dict[str, Any], job_name: str, quiet: bool): super().__init__(files=None, quiet=quiet) @@ -405,6 +430,7 @@ ad_hoc_steps = { "mypy": Mypy, "flake8-py3": Flake8, "shellcheck": ShellCheck, + "clang-tidy": ClangTidy, } if __name__ == "__main__": diff --git a/tools/linter/clang_tidy/__main__.py b/tools/linter/clang_tidy/__main__.py index 0254ddc43b3..3c36ffe7ac6 100644 --- a/tools/linter/clang_tidy/__main__.py +++ b/tools/linter/clang_tidy/__main__.py @@ -1,10 +1,52 @@ import argparse import pathlib +import os +import shutil +import subprocess +import re +from typing import List + from run import run from generate_build_files import generate_build_files +def clang_search_dirs() -> List[str]: + # Compilers are ordered based on fallback preference + # We pick the first one that is available on the system + compilers = ["clang", "gcc", "cpp", "cc"] + compilers = [c for c in compilers if shutil.which(c) is not None] + if len(compilers) == 0: + raise RuntimeError(f"None of {compilers} were found") + compiler = compilers[0] + + result = subprocess.run( + [compiler, "-E", "-x", "c++", "-", "-v"], + stdin=subprocess.DEVNULL, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + check=True, + ) + stderr = result.stderr.decode().strip().split("\n") + search_start = r"#include.*search starts here:" + search_end = r"End of search list." + + append_path = False + search_paths = [] + for line in stderr: + if re.match(search_start, line): + if append_path: + continue + else: + append_path = True + elif re.match(search_end, line): + break + elif append_path: + search_paths.append(line.strip()) + + return search_paths + + DEFAULTS = { "glob": [ # The negative filters below are to exclude files that include onnx_pb.h or @@ -32,7 +74,7 @@ DEFAULTS = { "-torch/csrc/deploy/interpreter/test_main.cpp", ], "paths": ["torch/csrc/"], - "include-dir": ["/usr/lib/llvm-11/include/openmp"], + "include-dir": ["/usr/lib/llvm-11/include/openmp"] + clang_search_dirs(), } @@ -68,7 +110,8 @@ def parse_args() -> argparse.Namespace: help="Path to the folder containing compile_commands.json", ) parser.add_argument( - "--diff-file", help="File containing diff to use for determining files to lint and line filters" + "--diff-file", + help="File containing diff to use for determining files to lint and line filters", ) parser.add_argument( "-p", @@ -103,7 +146,7 @@ def parse_args() -> argparse.Namespace: parser.add_argument( "--print-include-paths", action="store_true", - help="Print the search paths used for include directives" + help="Print the search paths used for include directives", ) parser.add_argument( "-I", @@ -112,8 +155,12 @@ def parse_args() -> argparse.Namespace: default=DEFAULTS["include-dir"], help="Add the specified directory to the search path for include files", ) - parser.add_argument("-s", "--suppress-diagnostics", action="store_true", - help="Add NOLINT to suppress clang-tidy violations") + parser.add_argument( + "-s", + "--suppress-diagnostics", + action="store_true", + help="Add NOLINT to suppress clang-tidy violations", + ) parser.add_argument( "extra_args", nargs="*", help="Extra arguments to forward to clang-tidy" ) @@ -124,7 +171,23 @@ def main() -> None: if not pathlib.Path("build").exists(): generate_build_files() options = parse_args() - run(options) + + # Check if clang-tidy executable exists + exists = os.access(options.clang_tidy_exe, os.X_OK) or shutil.which( + options.clang_tidy_exe + ) + if not exists: + msg = ( + "Could not find 'clang-tidy' binary\n" + "You can install it by running:\n" + " python3 tools/linter/install/clang_tidy.py" + ) + print(msg) + exit(1) + + return_code = run(options) + if return_code != 0: + raise RuntimeError("Warnings found in clang-tidy output!") main() diff --git a/tools/linter/clang_tidy/run.py b/tools/linter/clang_tidy/run.py index ea0babbae29..a0328a907d1 100644 --- a/tools/linter/clang_tidy/run.py +++ b/tools/linter/clang_tidy/run.py @@ -13,7 +13,6 @@ glob or regular expressions. """ - import collections import fnmatch import json @@ -52,7 +51,9 @@ def map_filename(build_folder: str, fname: str) -> str: build_cpu_prefix = os.path.join(build_folder, native_cpu_prefix, "") default_arch_suffix = ".DEFAULT.cpp" if fname.startswith(native_cpu_prefix) and fname.endswith(".cpp"): - return f"{build_cpu_prefix}{fname[len(native_cpu_prefix):]}{default_arch_suffix}" + return ( + f"{build_cpu_prefix}{fname[len(native_cpu_prefix):]}{default_arch_suffix}" + ) if fname.startswith(build_cpu_prefix) and fname.endswith(default_arch_suffix): return f"{native_cpu_prefix}{fname[len(build_cpu_prefix):-len(default_arch_suffix)]}" return fname @@ -165,11 +166,12 @@ build {i}: do_cmd def run_shell_commands_in_parallel(commands: Iterable[List[str]]) -> str: """runs all the commands in parallel with ninja, commands is a List[List[str]]""" + async def run_command(cmd: List[str]) -> str: proc = await asyncio.create_subprocess_shell( - ' '.join(shlex.quote(x) for x in cmd), # type: ignore[attr-defined] + " ".join(shlex.quote(x) for x in cmd), # type: ignore[attr-defined] stdout=asyncio.subprocess.PIPE, - stderr=asyncio.subprocess.PIPE + stderr=asyncio.subprocess.PIPE, ) stdout, stderr = await proc.communicate() return f">>>\nstdout:\n{stdout.decode()}\nstderr:\n{stderr.decode()}\n<<<" @@ -181,7 +183,9 @@ def run_shell_commands_in_parallel(commands: Iterable[List[str]]) -> str: async with semaphore: return await task - return await asyncio.gather(*(sem_task(task) for task in tasks), return_exceptions=True) + return await asyncio.gather( + *(sem_task(task) for task in tasks), return_exceptions=True + ) async def helper() -> Any: coros = [run_command(cmd) for cmd in commands] @@ -192,7 +196,9 @@ def run_shell_commands_in_parallel(commands: Iterable[List[str]]) -> str: return "\n".join(results) -def run_clang_tidy(options: Any, line_filters: List[Dict[str, Any]], files: Iterable[str]) -> str: +def run_clang_tidy( + options: Any, line_filters: List[Dict[str, Any]], files: Iterable[str] +) -> str: """Executes the actual clang-tidy command in the shell.""" command = [options.clang_tidy_exe, "-p", options.compile_commands_dir] if not options.config_file and os.path.exists(".clang-tidy"): @@ -202,7 +208,10 @@ def run_clang_tidy(options: Any, line_filters: List[Dict[str, Any]], files: Iter with open(options.config_file) as config: # Here we convert the YAML config file to a JSON blob. - command += ["-config", json.dumps(yaml.load(config, Loader=yaml.SafeLoader))] + command += [ + "-config", + json.dumps(yaml.load(config, Loader=yaml.SafeLoader)), + ] if options.print_include_paths: command += ["--extra-arg", "-v"] if options.include_dir: @@ -215,7 +224,10 @@ def run_clang_tidy(options: Any, line_filters: List[Dict[str, Any]], files: Iter command += ["-line-filter", json.dumps(line_filters)] if options.parallel: - commands = [list(command) + [map_filename(options.compile_commands_dir, f)] for f in files] + commands = [ + list(command) + [map_filename(options.compile_commands_dir, f)] + for f in files + ] output = run_shell_commands_in_parallel(commands) else: command += map_filenames(options.compile_commands_dir, files) @@ -232,7 +244,9 @@ def run_clang_tidy(options: Any, line_filters: List[Dict[str, Any]], files: Iter return output -def extract_warnings(output: str, base_dir: str = ".") -> Dict[str, Dict[int, Set[str]]]: +def extract_warnings( + output: str, base_dir: str = "." +) -> Dict[str, Dict[int, Set[str]]]: rc: Dict[str, Dict[int, Set[str]]] = {} for line in output.split("\n"): p = CLANG_WARNING_PATTERN.match(line) @@ -258,17 +272,50 @@ def apply_nolint(fname: str, warnings: Dict[int, Set[str]]) -> None: line_offset = -1 # As in .cpp files lines are numbered starting from 1 for line_no in sorted(warnings.keys()): - nolint_diagnostics = ','.join(warnings[line_no]) + nolint_diagnostics = ",".join(warnings[line_no]) line_no += line_offset - indent = ' ' * (len(lines[line_no]) - len(lines[line_no].lstrip(' '))) - lines.insert(line_no, f'{indent}// NOLINTNEXTLINE({nolint_diagnostics})\n') + indent = " " * (len(lines[line_no]) - len(lines[line_no].lstrip(" "))) + lines.insert(line_no, f"{indent}// NOLINTNEXTLINE({nolint_diagnostics})\n") line_offset += 1 with open(fname, mode="w") as f: f.write("".join(lines)) -def run(options: Any) -> None: +def filter_from_diff( + paths: List[str], diffs: List[str] +) -> Tuple[List[str], List[Dict[Any, Any]]]: + files = [] + line_filters = [] + + for diff in diffs: + changed_files = find_changed_lines(diff) + changed_files = { + filename: v + for filename, v in changed_files.items() + if any(filename.startswith(path) for path in paths) + } + line_filters += [ + {"name": name, "lines": lines} for name, lines, in changed_files.items() + ] + files += list(changed_files.keys()) + + return files, line_filters + + +def filter_from_diff_file( + paths: List[str], filename: str +) -> Tuple[List[str], List[Dict[Any, Any]]]: + with open(filename, "r") as f: + diff = f.read() + return filter_from_diff(paths, [diff]) + + +def filter_default(paths: List[str]) -> Tuple[List[str], List[Dict[Any, Any]]]: + return get_all_files(paths), [] + + +def run(options: Any) -> int: # This flag is pervasive enough to set it globally. It makes the code # cleaner compared to threading it through every single function. global VERBOSE @@ -276,25 +323,12 @@ def run(options: Any) -> None: # Normalize the paths first. paths = [path.rstrip("/") for path in options.paths] + if options.diff_file: - with open(options.diff_file, "r") as f: - changed_files = find_changed_lines(f.read()) - changed_files = { - filename: v - for filename, v in changed_files.items() - if any(filename.startswith(path) for path in options.paths) - } - line_filters = [ - {"name": name, "lines": lines} for name, lines, in changed_files.items() - ] - files = list(changed_files.keys()) - # Since header files are excluded, add .cpp file if it exists in the same folder - cpp_files = [f[:-1] + "cpp" for f in files if f.endswith(".h")] - cpp_files = [f for f in cpp_files if os.path.exists(f)] - files = list(set(files + cpp_files)) + files, line_filters = filter_from_diff_file(options.paths, options.diff_file) else: - line_filters = [] - files = get_all_files(paths) + files, line_filters = filter_default(options.paths) + file_patterns = get_file_patterns(options.glob, options.regex) files = list(filter_files(files, file_patterns)) @@ -304,8 +338,13 @@ def run(options: Any) -> None: sys.exit() clang_tidy_output = run_clang_tidy(options, line_filters, files) + warnings = extract_warnings( + clang_tidy_output, base_dir=options.compile_commands_dir + ) if options.suppress_diagnostics: - warnings = extract_warnings(clang_tidy_output, base_dir=options.compile_commands_dir) + warnings = extract_warnings( + clang_tidy_output, base_dir=options.compile_commands_dir + ) for fname in warnings.keys(): mapped_fname = map_filename(options.compile_commands_dir, fname) print(f"Applying fixes to {mapped_fname}") @@ -318,4 +357,6 @@ def run(options: Any) -> None: print(clang_tidy_output) for line in clang_tidy_output.splitlines(): if line.startswith(pwd): - print(line[len(pwd):]) + print(line[len(pwd) :]) + + return len(warnings.keys())