diff --git a/.cspell.config.yaml b/.cspell.config.yaml index 3c5191f5e1..19b3970865 100644 --- a/.cspell.config.yaml +++ b/.cspell.config.yaml @@ -65,6 +65,7 @@ words: - Btrfs - Buildx - canonicality + - canonicalised - changespq - checkme - choco diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index d3bf0209fb..8a689c0f8b 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -32,6 +32,11 @@ repos: # as standalone translation units, so they have no compile_commands.json # entry to lint (verify_headers checks them transitively). exclude: '^include/xrpl/protocol_autogen|\.ipp$' + # run-clang-tidy --fix may edit headers included by files it is not run on, + # so pre-commit must not split the files across parallel hook invocations. + # The script determines the staged files itself and lets run-clang-tidy + # handle parallelism internally. + pass_filenames: false - id: fix-include-style name: fix include style entry: ./bin/pre-commit/fix_include_style.py diff --git a/bin/pre-commit/clang_tidy_check.py b/bin/pre-commit/clang_tidy_check.py index 5b5792b405..cf4808d2ea 100755 --- a/bin/pre-commit/clang_tidy_check.py +++ b/bin/pre-commit/clang_tidy_check.py @@ -1,27 +1,46 @@ #!/usr/bin/env python3 -"""Pre-commit hook that runs clang-tidy on changed files using run-clang-tidy. +"""Pre-commit hook that runs clang-tidy on staged files using run-clang-tidy. -The set of files is chosen by pre-commit (see .pre-commit-config.yaml), which -filters to C/C++ sources and excludes `.ipp` fragments. Headers are linted -directly: the `verify_headers` build option (ON by default) compiles every -`.h`/`.hpp` on its own, so each header is the main file of its own -compile_commands.json entry and run-clang-tidy can analyse it just like a -`.cpp`. +The script determines the staged files itself (see `pass_filenames: false` in +.pre-commit-config.yaml) so run-clang-tidy is run once and handles parallelism +internally: pre-commit would otherwise split the files across parallel hook +invocations that race when fixes edit a shared header. + +Fixes are collected with `-export-fixes` and applied by clang-apply-replacements +in a separate step rather than with run-clang-tidy's `-fix`. The `add_module` +build isolates each module's headers behind a per-module symlink directory +(build/modules//...), so a header reachable from several translation +units is referenced through different paths that all resolve to the same source +file. clang-apply-replacements deduplicates identical replacements by their +literal path, so those paths must be canonicalised to the real source path +first; otherwise the same fix is applied once per path and corrupts the header. """ from __future__ import annotations import os +import re import shutil import subprocess import sys +import tempfile from pathlib import Path CLANG_TIDY_VERSION = 22 +# Extensions run-clang-tidy can analyse: `.cpp` translation units and, thanks to +# the `verify_headers` build option, `.h`/`.hpp` headers (each has its own +# compile_commands.json entry). `.ipp` fragments have no entry and are skipped. +TIDY_EXTENSIONS = {".cpp", ".h", ".hpp"} -def find_run_clang_tidy() -> str | None: - for candidate in (f"run-clang-tidy-{CLANG_TIDY_VERSION}", "run-clang-tidy"): +# A single-quoted `FilePath:` entry in an -export-fixes YAML file, allowing the +# `- ` marker that precedes it inside a `Replacements:` sequence. clang-tidy +# emits paths single-quoted and doubles any embedded quote per YAML rules. +FILEPATH_RE = re.compile(r"^(\s*(?:-\s+)?FilePath:\s*)'((?:[^']|'')*)'\s*$") + + +def find_tool(name: str) -> str | None: + for candidate in (f"{name}-{CLANG_TIDY_VERSION}", name): if path := shutil.which(candidate): return path return None @@ -35,23 +54,43 @@ def find_build_dir(repo_root: Path) -> Path | None: return None +def staged_files(repo_root: Path) -> list[Path]: + """Return absolute paths of staged, lint-able C/C++ files. + + `--diff-filter=d` excludes deletions so we never lint a removed file. + """ + output = subprocess.check_output( + ["git", "diff", "--staged", "--name-only", "--diff-filter=d", "--"] + + [f"*{ext}" for ext in TIDY_EXTENSIONS], + text=True, + cwd=repo_root, + ) + return [repo_root / rel for rel in output.splitlines() if rel] + + +def canonicalize_fix_paths(fixes_dir: Path) -> None: + """Rewrite every `FilePath` in the exported fixes to its real source path. + + A header included through a module's isolation symlink is recorded under that + symlink's path; collapsing all paths to the same real file lets + clang-apply-replacements recognise the per-translation-unit duplicates and + apply each fix once. + """ + for yaml in fixes_dir.glob("*.yaml"): + lines = [] + for line in yaml.read_text().splitlines(): + if m := FILEPATH_RE.match(line): + path = m.group(2).replace("''", "'") + real = os.path.realpath(path).replace("'", "''") + line = f"{m.group(1)}'{real}'" + lines.append(line) + yaml.write_text("\n".join(lines) + "\n") + + def main(): if not os.environ.get("TIDY"): return 0 - files = sys.argv[1:] - if not files: - return 0 - - run_clang_tidy = find_run_clang_tidy() - if not run_clang_tidy: - print( - f"clang-tidy check failed: TIDY is enabled but neither " - f"'run-clang-tidy-{CLANG_TIDY_VERSION}' nor 'run-clang-tidy' was found in PATH.", - file=sys.stderr, - ) - return 1 - repo_root = Path( subprocess.check_output( ["git", "rev-parse", "--show-toplevel"], @@ -59,6 +98,29 @@ def main(): text=True, ).strip() ) + + files = staged_files(repo_root) + if not files: + return 0 + + run_clang_tidy = find_tool("run-clang-tidy") + clang_apply_replacements = find_tool("clang-apply-replacements") + missing = [ + name + for name, path in ( + ("run-clang-tidy", run_clang_tidy), + ("clang-apply-replacements", clang_apply_replacements), + ) + if not path + ] + if missing: + print( + f"clang-tidy check failed: TIDY is enabled but {' and '.join(missing)} " + f"was not found in PATH (tried the '-{CLANG_TIDY_VERSION}' suffix too).", + file=sys.stderr, + ) + return 1 + build_dir = find_build_dir(repo_root) if not build_dir: print( @@ -68,11 +130,23 @@ def main(): ) return 1 - result = subprocess.run( - [run_clang_tidy, "-quiet", "-p", str(build_dir), "-fix", "-allow-no-checks"] - + files - ) - return result.returncode + with tempfile.TemporaryDirectory() as fixes_dir: + result = subprocess.run( + [ + run_clang_tidy, + "-quiet", + "-p", + build_dir, + "-export-fixes", + fixes_dir, + "-allow-no-checks", + ] + + files + ) + canonicalize_fix_paths(Path(fixes_dir)) + applied = subprocess.run([clang_apply_replacements, fixes_dir]) + + return result.returncode or applied.returncode if __name__ == "__main__":