From 9eda2b974dcac0300466d8c8569bab4a72bedef5 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com> Date: Wed, 4 Nov 2026 16:52:13 +0000 Subject: [PATCH 1/3] fix: harden clang-format and clang-tidy hook arguments --- cpp_linter_hooks/clang_format.py | 29 ++++++++++++++++-- cpp_linter_hooks/clang_tidy.py | 51 +++++++++++++++++++++++--------- tests/test_clang_format.py | 33 +++++++++++++++++++++ tests/test_clang_tidy.py | 44 +++++++++++++++++++++++++-- 4 files changed, 139 insertions(+), 18 deletions(-) diff --git a/cpp_linter_hooks/clang_format.py b/cpp_linter_hooks/clang_format.py index f96f4d3..c818bac 100644 --- a/cpp_linter_hooks/clang_format.py +++ b/cpp_linter_hooks/clang_format.py @@ -19,6 +19,26 @@ "-v", "--verbose", action="store_true", help="Enable verbose output" ) +# clang-format's spellings of --dry-run and --Werror. +DRY_RUN_OPTIONS = ("-n", "-dry-run", "--dry-run") +WERROR_OPTIONS = ("-Werror", "--Werror") + + +def _option_name(arg: str) -> str: + """Return the option name of an argument such as ``--dry-run=true``.""" + return arg.partition("=")[0] + + +def _is_dry_run(args: list[str]) -> bool: + """Return whether args turn on --dry-run, in any spelling, e.g. ``-n``.""" + for arg in args: + name, has_value, value = arg.partition("=") + if name in DRY_RUN_OPTIONS and ( + not has_value or value.lower() in ("true", "1") + ): + return True + return False + def run_clang_format(args=None) -> tuple[int, str]: """Run clang-format with hook-specific arguments removed.""" @@ -37,9 +57,12 @@ def run_clang_format(args=None) -> tuple[int, str]: command.extend(other_args) - # Auto-inject --Werror when --dry-run is used, so clang-format returns + # Auto-inject --Werror when --dry-run (or -n) is used, so clang-format returns # non-zero when formatting changes are needed (mirrors-clang-format behavior). - if "--dry-run" in command and "--Werror" not in command: + # A --Werror the user set, even --Werror=false, is kept as it is. + if _is_dry_run(other_args) and not any( + _option_name(arg) in WERROR_OPTIONS for arg in other_args + ): command.append("--Werror") try: @@ -48,6 +71,8 @@ def run_clang_format(args=None) -> tuple[int, str]: command, capture_output=True, encoding="utf-8", + # clang-format prints source lines, which may not be UTF-8. + errors="replace", check=False, ) diff --git a/cpp_linter_hooks/clang_tidy.py b/cpp_linter_hooks/clang_tidy.py index f5fd4da..0e7b8c7 100644 --- a/cpp_linter_hooks/clang_tidy.py +++ b/cpp_linter_hooks/clang_tidy.py @@ -133,6 +133,17 @@ + r""")-[\w.*-]*)['"]?$""" ) QUOTES = ("'", '"') +# Options that change files in place, and the one that writes a fixes file; +# clang-tidy accepts each with one or two dashes. +FIX_OPTIONS = ( + "-fix", + "--fix", + "-fix-errors", + "--fix-errors", + "-fix-notes", + "--fix-notes", +) +EXPORT_FIXES_OPTIONS = ("-export-fixes", "--export-fixes") def _positive_int(value: str) -> int: @@ -426,7 +437,14 @@ def _set_macos_sdkroot(verbose: bool) -> None: def _exec_clang_tidy(command) -> tuple[int, str]: """Run clang-tidy and return (retval, output).""" try: - sp = subprocess.run(command, capture_output=True, encoding="utf-8", check=False) + # errors="replace": clang-tidy prints source lines, which may not be UTF-8. + sp = subprocess.run( + command, + capture_output=True, + encoding="utf-8", + errors="replace", + check=False, + ) output = (sp.stdout or "") + (sp.stderr or "") if NO_CHECKS_ERROR in output: # Keep the error line, not the whole --help text that follows it. @@ -604,6 +622,19 @@ def _split_source_files(args: list[str]) -> tuple[list[str], list[str]]: return args[:split_idx], list(reversed(source_files)) +def _file_argument(path: str) -> str: + """Return a file name that clang-tidy cannot read as an option. + + pre-commit passes file names as they are, so a file named + ``--load=plugin.cpp`` would make clang-tidy load a plugin. Such a name gets + a ``./`` prefix. clang-tidy has no ``--`` to end its options, because ``--`` + starts the compiler flags. + """ + if path.startswith("-") and os.path.isfile(path): + return f"./{path}" + return path + + def _combine_outputs(results: list[tuple[int, str]]) -> str: """Join non-empty clang-tidy outputs from multiple executions.""" return "\n".join(output.rstrip("\n") for _, output in results if output) @@ -665,6 +696,7 @@ def run_clang_tidy(args=None) -> tuple[int, str]: return error clang_tidy_args, source_files = _split_source_files(other_args) + source_files = [_file_argument(f) for f in source_files] clang_tidy_args, notes = _repair_check_args(clang_tidy_args) lines = None @@ -717,23 +749,14 @@ def run_clang_tidy(args=None) -> tuple[int, str]: if not hook_args.verbose and not _has_quiet_option(clang_tidy_args): clang_tidy_args.insert(0, "-quiet") - if ( - hook_args.fix - and "-fix" not in clang_tidy_args - and "-fix-errors" not in clang_tidy_args - ): + if hook_args.fix and not _has_option(clang_tidy_args, *FIX_OPTIONS): clang_tidy_args.append("-fix") # Parallel execution is unsafe when arguments include flags that write to a # shared output path (e.g., --export-fixes fixes.yaml) or that apply in-place - # fixes (-fix, -fix-errors), since multiple clang-tidy processes may attempt - # to modify the same header file concurrently. - unsafe_parallel = any( - arg == "--export-fixes" - or arg.startswith("--export-fixes=") - or arg in ("-fix", "-fix-errors") - for arg in clang_tidy_args - ) + # fixes (-fix, -fix-errors, -fix-notes), since multiple clang-tidy processes + # may attempt to modify the same header file concurrently. + unsafe_parallel = _has_option(clang_tidy_args, *FIX_OPTIONS, *EXPORT_FIXES_OPTIONS) if hook_args.jobs > 1 and len(source_files) > 1 and not unsafe_parallel: retval, output = _exec_parallel_clang_tidy( diff --git a/tests/test_clang_format.py b/tests/test_clang_format.py index 7fc3288..34ca603 100644 --- a/tests/test_clang_format.py +++ b/tests/test_clang_format.py @@ -76,6 +76,24 @@ def test_run_clang_format_dry_run_unformatted(tmp_path): assert output.strip() # Should report which file needs formatting +def test_run_clang_format_dry_run_short_option_fails_unformatted(tmp_path): + """-n is --dry-run, so it also reports unformatted files as a failure.""" + test_file = tmp_path / "main.c" + test_file.write_bytes(Path("testing/main.c").read_bytes()) + ret, output = run_clang_format(["-n", "--style=Google", str(test_file)]) + assert ret == 1 + assert "clang-format-violations" in output + + +def test_run_clang_format_dry_run_on_a_file_that_is_not_utf8(tmp_path): + """clang-format prints the source line; Latin-1 bytes must not crash the hook.""" + test_file = tmp_path / "latin1.c" + test_file.write_bytes(b"int x = 1; /* caf\xe9 */\n") + ret, output = run_clang_format(["--dry-run", str(test_file)]) + assert ret == 1 + assert "caf\ufffd" in output + + @pytest.mark.benchmark def test_run_clang_format_dry_run_formatted(tmp_path): """Dry-run on already-formatted files returns success.""" @@ -200,6 +218,7 @@ def test_run_clang_format_formats_files_in_place(mock_clang_format): ["clang-format", "-i", "--style=Google", "a.cpp", "b.cpp"], capture_output=True, encoding="utf-8", + errors="replace", check=False, ) @@ -217,6 +236,20 @@ def test_run_clang_format_formats_files_in_place(mock_clang_format): ), (["--Werror", "a.cpp"], ["clang-format", "-i", "--Werror", "a.cpp"]), (["a.cpp"], ["clang-format", "-i", "a.cpp"]), + # -n is clang-format's short form of --dry-run. + (["-n", "a.cpp"], ["clang-format", "-i", "-n", "a.cpp", "--Werror"]), + ( + ["--dry-run=true", "a.cpp"], + ["clang-format", "-i", "--dry-run=true", "a.cpp", "--Werror"], + ), + ( + ["--dry-run=false", "a.cpp"], + ["clang-format", "-i", "--dry-run=false", "a.cpp"], + ), + ( + ["-n", "--Werror=false", "a.cpp"], + ["clang-format", "-i", "-n", "--Werror=false", "a.cpp"], + ), ), ) def test_run_clang_format_adds_werror_only_for_dry_run( diff --git a/tests/test_clang_tidy.py b/tests/test_clang_tidy.py index b25706e..d2d92d4 100644 --- a/tests/test_clang_tidy.py +++ b/tests/test_clang_tidy.py @@ -600,10 +600,21 @@ def test_exec_clang_tidy_return_value(returncode, stdout, stderr, expected_retva assert (ret, output) == (expected_retval, stdout + stderr) mock_run.assert_called_once_with( - ["clang-tidy", "a.cpp"], capture_output=True, encoding="utf-8", check=False + ["clang-tidy", "a.cpp"], + capture_output=True, + encoding="utf-8", + errors="replace", + check=False, ) +def test_exec_clang_tidy_reads_output_that_is_not_utf8(): + # Source lines in the output can be Latin-1 or GBK. + command = [sys.executable, "-c", "import sys; sys.stdout.buffer.write(b'caf\\xe9')"] + + assert _exec_clang_tidy(command) == (0, "caf\ufffd") + + def test_exec_clang_tidy_handles_missing_streams(): completed = MagicMock(returncode=0, stdout=None, stderr=None) with patch("cpp_linter_hooks.clang_tidy.subprocess.run", return_value=completed): @@ -779,7 +790,36 @@ def test_fix_flag_is_inserted_before_source_files(mock_exec): ) -@pytest.mark.parametrize("existing", ["-fix", "-fix-errors"]) +@pytest.mark.parametrize( + "option", + [ + "--fix-errors", + "-fix-notes", + "--fix-notes", + "-export-fixes=fixes.yaml", + ], +) +def test_jobs_with_any_fix_option_spelling_forces_serial_execution(mock_exec, option): + run_clang_tidy(["--jobs=4", "-p", "./build", option, "a.cpp", "b.cpp"]) + + mock_exec.assert_called_once_with( + ["clang-tidy", "-quiet", "-p", "./build", option, "a.cpp", "b.cpp"] + ) + + +def test_file_named_like_an_option_is_not_read_as_one(tmp_path, monkeypatch, mock_exec): + # A file named --load=plugin.cpp would make clang-tidy load a plugin. + monkeypatch.chdir(tmp_path) + (tmp_path / "--load=plugin.cpp").write_text("int x;\n") + + run_clang_tidy(["--all-lines", "-p", "./build", "--load=plugin.cpp", "a.cpp"]) + + mock_exec.assert_called_once_with( + ["clang-tidy", "-quiet", "-p", "./build", "./--load=plugin.cpp", "a.cpp"] + ) + + +@pytest.mark.parametrize("existing", ["-fix", "-fix-errors", "--fix-errors"]) def test_fix_flag_does_not_add_a_second_fix_option(mock_exec, existing): run_clang_tidy(["--fix", "-p", "./build", existing, "a.cpp"]) From 214a5e483cb6c10c9b4542ed99784ade081a7a47 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com> Date: Thu, 5 Nov 2026 18:00:32 +0000 Subject: [PATCH 2/3] fix: read --dry-run= and --fix-errors=false the way clang tools do Put the injected --Werror before the user's arguments, which may end the options with "--", and test a dash-prefixed file through the real changed-line path. --- cpp_linter_hooks/clang_format.py | 29 +++++++---------------------- cpp_linter_hooks/clang_tidy.py | 8 +++++--- cpp_linter_hooks/util.py | 13 +++++++++++++ tests/test_clang_format.py | 16 +++++++++++++--- tests/test_clang_tidy.py | 30 ++++++++++++++++++++++++++++++ tests/test_util.py | 20 ++++++++++++++++++++ 6 files changed, 88 insertions(+), 28 deletions(-) diff --git a/cpp_linter_hooks/clang_format.py b/cpp_linter_hooks/clang_format.py index c818bac..79cdc71 100644 --- a/cpp_linter_hooks/clang_format.py +++ b/cpp_linter_hooks/clang_format.py @@ -4,7 +4,7 @@ import sys from argparse import ArgumentParser -from cpp_linter_hooks.util import resolve_install_with_diagnostics +from cpp_linter_hooks.util import resolve_install_with_diagnostics, turns_on parser = ArgumentParser() parser.add_argument( @@ -24,22 +24,6 @@ WERROR_OPTIONS = ("-Werror", "--Werror") -def _option_name(arg: str) -> str: - """Return the option name of an argument such as ``--dry-run=true``.""" - return arg.partition("=")[0] - - -def _is_dry_run(args: list[str]) -> bool: - """Return whether args turn on --dry-run, in any spelling, e.g. ``-n``.""" - for arg in args: - name, has_value, value = arg.partition("=") - if name in DRY_RUN_OPTIONS and ( - not has_value or value.lower() in ("true", "1") - ): - return True - return False - - def run_clang_format(args=None) -> tuple[int, str]: """Run clang-format with hook-specific arguments removed.""" hook_args, other_args = parser.parse_known_args(args) @@ -55,16 +39,17 @@ def run_clang_format(args=None) -> tuple[int, str]: if hook_args.verbose: command.append("--verbose") - command.extend(other_args) - # Auto-inject --Werror when --dry-run (or -n) is used, so clang-format returns # non-zero when formatting changes are needed (mirrors-clang-format behavior). - # A --Werror the user set, even --Werror=false, is kept as it is. - if _is_dry_run(other_args) and not any( - _option_name(arg) in WERROR_OPTIONS for arg in other_args + # A --Werror the user set, even --Werror=false, is kept as it is. It goes + # before the user's arguments, which may end the options with "--". + if turns_on(other_args, DRY_RUN_OPTIONS) and not any( + arg.partition("=")[0] in WERROR_OPTIONS for arg in other_args ): command.append("--Werror") + command.extend(other_args) + try: # Run the clang-format command with captured output sp = subprocess.run( diff --git a/cpp_linter_hooks/clang_tidy.py b/cpp_linter_hooks/clang_tidy.py index 0e7b8c7..fd203de 100644 --- a/cpp_linter_hooks/clang_tidy.py +++ b/cpp_linter_hooks/clang_tidy.py @@ -15,7 +15,7 @@ diff_range, normalize, ) -from cpp_linter_hooks.util import resolve_install_with_diagnostics +from cpp_linter_hooks.util import resolve_install_with_diagnostics, turns_on COMPILE_DB_SEARCH_DIRS = ["build", "out", "cmake-build-debug", "_build", "builddir"] # Searched after COMPILE_DB_SEARCH_DIRS. When a pattern matches several @@ -749,14 +749,16 @@ def run_clang_tidy(args=None) -> tuple[int, str]: if not hook_args.verbose and not _has_quiet_option(clang_tidy_args): clang_tidy_args.insert(0, "-quiet") - if hook_args.fix and not _has_option(clang_tidy_args, *FIX_OPTIONS): + if hook_args.fix and not turns_on(clang_tidy_args, FIX_OPTIONS): clang_tidy_args.append("-fix") # Parallel execution is unsafe when arguments include flags that write to a # shared output path (e.g., --export-fixes fixes.yaml) or that apply in-place # fixes (-fix, -fix-errors, -fix-notes), since multiple clang-tidy processes # may attempt to modify the same header file concurrently. - unsafe_parallel = _has_option(clang_tidy_args, *FIX_OPTIONS, *EXPORT_FIXES_OPTIONS) + unsafe_parallel = turns_on(clang_tidy_args, FIX_OPTIONS) or _has_option( + clang_tidy_args, *EXPORT_FIXES_OPTIONS + ) if hook_args.jobs > 1 and len(source_files) > 1 and not unsafe_parallel: retval, output = _exec_parallel_clang_tidy( diff --git a/cpp_linter_hooks/util.py b/cpp_linter_hooks/util.py index 978702d..a70aa1e 100644 --- a/cpp_linter_hooks/util.py +++ b/cpp_linter_hooks/util.py @@ -17,6 +17,19 @@ LOG = logging.getLogger(__name__) +def turns_on(args: list[str], names: tuple[str, ...]) -> bool: + """Return whether args turn on a boolean option, in any of its spellings. + + LLVM tools take ``-n``, ``--dry-run``, ``--dry-run=true`` or ``--dry-run=1`` + (older versions also an empty value), and ``=false`` or ``=0`` turns it off. + """ + for arg in args: + name, _, value = arg.partition("=") + if name in names and value.lower() in ("", "true", "1"): + return True + return False + + @lru_cache(maxsize=4) def _get_pypi_versions(tool: str) -> tuple[str | None, list]: """Fetch (latest_version, [stable_versions_descending]) from PyPI JSON API. diff --git a/tests/test_clang_format.py b/tests/test_clang_format.py index 34ca603..155bae9 100644 --- a/tests/test_clang_format.py +++ b/tests/test_clang_format.py @@ -228,7 +228,7 @@ def test_run_clang_format_formats_files_in_place(mock_clang_format): ( ( ["--dry-run", "a.cpp"], - ["clang-format", "-i", "--dry-run", "a.cpp", "--Werror"], + ["clang-format", "-i", "--Werror", "--dry-run", "a.cpp"], ), ( ["--dry-run", "--Werror", "a.cpp"], @@ -237,15 +237,25 @@ def test_run_clang_format_formats_files_in_place(mock_clang_format): (["--Werror", "a.cpp"], ["clang-format", "-i", "--Werror", "a.cpp"]), (["a.cpp"], ["clang-format", "-i", "a.cpp"]), # -n is clang-format's short form of --dry-run. - (["-n", "a.cpp"], ["clang-format", "-i", "-n", "a.cpp", "--Werror"]), + (["-n", "a.cpp"], ["clang-format", "-i", "--Werror", "-n", "a.cpp"]), ( ["--dry-run=true", "a.cpp"], - ["clang-format", "-i", "--dry-run=true", "a.cpp", "--Werror"], + ["clang-format", "-i", "--Werror", "--dry-run=true", "a.cpp"], ), ( ["--dry-run=false", "a.cpp"], ["clang-format", "-i", "--dry-run=false", "a.cpp"], ), + # Older clang-format versions read an empty value as true. + ( + ["--dry-run=", "a.cpp"], + ["clang-format", "-i", "--Werror", "--dry-run=", "a.cpp"], + ), + # Everything after "--" is a file name, so --Werror goes before it. + ( + ["-n", "--", "a.cpp"], + ["clang-format", "-i", "--Werror", "-n", "--", "a.cpp"], + ), ( ["-n", "--Werror=false", "a.cpp"], ["clang-format", "-i", "-n", "--Werror=false", "a.cpp"], diff --git a/tests/test_clang_tidy.py b/tests/test_clang_tidy.py index d2d92d4..7337cbd 100644 --- a/tests/test_clang_tidy.py +++ b/tests/test_clang_tidy.py @@ -828,6 +828,15 @@ def test_fix_flag_does_not_add_a_second_fix_option(mock_exec, existing): ) +@pytest.mark.parametrize("disabled", ["-fix-errors=false", "--fix-notes=0"]) +def test_fix_flag_is_added_when_a_fix_option_is_turned_off(mock_exec, disabled): + run_clang_tidy(["--fix", "-p", "./build", disabled, "a.cpp"]) + + mock_exec.assert_called_once_with( + ["clang-tidy", "-quiet", "-p", "./build", disabled, "-fix", "a.cpp"] + ) + + @pytest.mark.parametrize("db_dir", COMPILE_DB_SEARCH_DIRS) def test_compile_commands_auto_detect_each_search_dir( tmp_path, monkeypatch, mock_exec, db_dir @@ -1430,6 +1439,27 @@ def test_real_tool_keeps_the_lines_of_files_with_similar_names(braces_repo, tmp_ assert "data.cpp:2:" in output +def test_real_tool_checks_the_changed_lines_of_a_file_named_like_an_option( + braces_repo, tmp_path +): + # Without the ./ prefix, clang-tidy would read "-b.cpp" as an option. + dashed = tmp_path / "-b.cpp" + dashed.write_text("int g(int x) {\n if (x)\n return 1;\n return 0;\n}\n") + _git(tmp_path, "add", "--", "-b.cpp") + _git(tmp_path, "commit", "-q", "-m", "dashed") + dashed.write_text( + "int g(int x) {\n if (x)\n return 1;\n if (x > 1)\n return 2;\n" + " return 0;\n}\n" + ) + _git(tmp_path, "add", "--", "-b.cpp") + + ret, output = run_clang_tidy(CHECK_BRACES + ["-b.cpp"]) + + assert ret == 1 + assert "-b.cpp:4:" in output + assert "-b.cpp:2:" not in output + + # --- what happens without a compilation database --- diff --git a/tests/test_util.py b/tests/test_util.py index 3279970..45a135b 100644 --- a/tests/test_util.py +++ b/tests/test_util.py @@ -32,6 +32,7 @@ _versions_dir, resolve_install, resolve_install_with_diagnostics, + turns_on, ) # ── sample PyPI responses for consistent test data ────────────────────── @@ -1230,3 +1231,22 @@ def test_resolve_install_versions_coexist_without_pypi(): assert resolve_install("clang-format", "18.1.8") == v18 pypi.assert_not_called() install.assert_not_called() + + +@pytest.mark.parametrize( + ("args", "expected"), + ( + (["--dry-run"], True), + (["-n", "a.cpp"], True), + (["--dry-run=true"], True), + (["--dry-run=TRUE"], True), + (["--dry-run=1"], True), + (["--dry-run="], True), + (["--dry-run=false"], False), + (["--dry-run=0"], False), + (["--dry-run-x"], False), + (["a.cpp"], False), + ), +) +def test_turns_on(args, expected): + assert turns_on(args, ("-n", "--dry-run")) is expected From 6b16cc62cf6a49d94ee2a6467582d4150c944368 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com> Date: Thu, 5 Nov 2026 19:25:28 +0000 Subject: [PATCH 3/3] refactor: simplify the argument fixes Use plain checks for -n and the fix options, keep errors="replace" only for clang-format (clang-tidy escapes non-UTF-8 bytes itself), and give clang-format the same ./ prefix for file names that look like options. --- cpp_linter_hooks/clang_format.py | 13 +++---- cpp_linter_hooks/clang_tidy.py | 47 +++++-------------------- cpp_linter_hooks/util.py | 18 +++++----- tests/test_clang_format.py | 44 +++++++++-------------- tests/test_clang_tidy.py | 60 +++----------------------------- tests/test_util.py | 20 ----------- 6 files changed, 44 insertions(+), 158 deletions(-) diff --git a/cpp_linter_hooks/clang_format.py b/cpp_linter_hooks/clang_format.py index 79cdc71..db7f888 100644 --- a/cpp_linter_hooks/clang_format.py +++ b/cpp_linter_hooks/clang_format.py @@ -4,7 +4,7 @@ import sys from argparse import ArgumentParser -from cpp_linter_hooks.util import resolve_install_with_diagnostics, turns_on +from cpp_linter_hooks.util import file_argument, resolve_install_with_diagnostics parser = ArgumentParser() parser.add_argument( @@ -19,14 +19,11 @@ "-v", "--verbose", action="store_true", help="Enable verbose output" ) -# clang-format's spellings of --dry-run and --Werror. -DRY_RUN_OPTIONS = ("-n", "-dry-run", "--dry-run") -WERROR_OPTIONS = ("-Werror", "--Werror") - def run_clang_format(args=None) -> tuple[int, str]: """Run clang-format with hook-specific arguments removed.""" hook_args, other_args = parser.parse_known_args(args) + other_args = [file_argument(arg) for arg in other_args] tool_path, version_error = resolve_install_with_diagnostics( "clang-format", hook_args.version, hook_args.verbose ) @@ -41,10 +38,8 @@ def run_clang_format(args=None) -> tuple[int, str]: # Auto-inject --Werror when --dry-run (or -n) is used, so clang-format returns # non-zero when formatting changes are needed (mirrors-clang-format behavior). - # A --Werror the user set, even --Werror=false, is kept as it is. It goes - # before the user's arguments, which may end the options with "--". - if turns_on(other_args, DRY_RUN_OPTIONS) and not any( - arg.partition("=")[0] in WERROR_OPTIONS for arg in other_args + if any(a in ("-n", "--dry-run") for a in other_args) and not any( + a.startswith("--Werror") for a in other_args ): command.append("--Werror") diff --git a/cpp_linter_hooks/clang_tidy.py b/cpp_linter_hooks/clang_tidy.py index fd203de..bbdcf40 100644 --- a/cpp_linter_hooks/clang_tidy.py +++ b/cpp_linter_hooks/clang_tidy.py @@ -15,7 +15,7 @@ diff_range, normalize, ) -from cpp_linter_hooks.util import resolve_install_with_diagnostics, turns_on +from cpp_linter_hooks.util import file_argument, resolve_install_with_diagnostics COMPILE_DB_SEARCH_DIRS = ["build", "out", "cmake-build-debug", "_build", "builddir"] # Searched after COMPILE_DB_SEARCH_DIRS. When a pattern matches several @@ -133,17 +133,8 @@ + r""")-[\w.*-]*)['"]?$""" ) QUOTES = ("'", '"') -# Options that change files in place, and the one that writes a fixes file; -# clang-tidy accepts each with one or two dashes. -FIX_OPTIONS = ( - "-fix", - "--fix", - "-fix-errors", - "--fix-errors", - "-fix-notes", - "--fix-notes", -) -EXPORT_FIXES_OPTIONS = ("-export-fixes", "--export-fixes") +# clang-tidy options that change files, with either one or two dashes. +FIX_NAMES = {"fix", "fix-errors", "fix-notes", "export-fixes"} def _positive_int(value: str) -> int: @@ -437,14 +428,7 @@ def _set_macos_sdkroot(verbose: bool) -> None: def _exec_clang_tidy(command) -> tuple[int, str]: """Run clang-tidy and return (retval, output).""" try: - # errors="replace": clang-tidy prints source lines, which may not be UTF-8. - sp = subprocess.run( - command, - capture_output=True, - encoding="utf-8", - errors="replace", - check=False, - ) + sp = subprocess.run(command, capture_output=True, encoding="utf-8", check=False) output = (sp.stdout or "") + (sp.stderr or "") if NO_CHECKS_ERROR in output: # Keep the error line, not the whole --help text that follows it. @@ -622,19 +606,6 @@ def _split_source_files(args: list[str]) -> tuple[list[str], list[str]]: return args[:split_idx], list(reversed(source_files)) -def _file_argument(path: str) -> str: - """Return a file name that clang-tidy cannot read as an option. - - pre-commit passes file names as they are, so a file named - ``--load=plugin.cpp`` would make clang-tidy load a plugin. Such a name gets - a ``./`` prefix. clang-tidy has no ``--`` to end its options, because ``--`` - starts the compiler flags. - """ - if path.startswith("-") and os.path.isfile(path): - return f"./{path}" - return path - - def _combine_outputs(results: list[tuple[int, str]]) -> str: """Join non-empty clang-tidy outputs from multiple executions.""" return "\n".join(output.rstrip("\n") for _, output in results if output) @@ -696,7 +667,7 @@ def run_clang_tidy(args=None) -> tuple[int, str]: return error clang_tidy_args, source_files = _split_source_files(other_args) - source_files = [_file_argument(f) for f in source_files] + source_files = [file_argument(f) for f in source_files] clang_tidy_args, notes = _repair_check_args(clang_tidy_args) lines = None @@ -749,16 +720,16 @@ def run_clang_tidy(args=None) -> tuple[int, str]: if not hook_args.verbose and not _has_quiet_option(clang_tidy_args): clang_tidy_args.insert(0, "-quiet") - if hook_args.fix and not turns_on(clang_tidy_args, FIX_OPTIONS): + # Option names without dashes or value, e.g. "fix-errors" for --fix-errors. + names = {arg.lstrip("-").partition("=")[0] for arg in clang_tidy_args} + if hook_args.fix and not names & {"fix", "fix-errors", "fix-notes"}: clang_tidy_args.append("-fix") # Parallel execution is unsafe when arguments include flags that write to a # shared output path (e.g., --export-fixes fixes.yaml) or that apply in-place # fixes (-fix, -fix-errors, -fix-notes), since multiple clang-tidy processes # may attempt to modify the same header file concurrently. - unsafe_parallel = turns_on(clang_tidy_args, FIX_OPTIONS) or _has_option( - clang_tidy_args, *EXPORT_FIXES_OPTIONS - ) + unsafe_parallel = hook_args.fix or bool(names & FIX_NAMES) if hook_args.jobs > 1 and len(source_files) > 1 and not unsafe_parallel: retval, output = _exec_parallel_clang_tidy( diff --git a/cpp_linter_hooks/util.py b/cpp_linter_hooks/util.py index a70aa1e..9b5983b 100644 --- a/cpp_linter_hooks/util.py +++ b/cpp_linter_hooks/util.py @@ -17,17 +17,17 @@ LOG = logging.getLogger(__name__) -def turns_on(args: list[str], names: tuple[str, ...]) -> bool: - """Return whether args turn on a boolean option, in any of its spellings. +def file_argument(arg: str) -> str: + """Return a file name that a clang tool cannot read as an option. - LLVM tools take ``-n``, ``--dry-run``, ``--dry-run=true`` or ``--dry-run=1`` - (older versions also an empty value), and ``=false`` or ``=0`` turns it off. + pre-commit passes file names as they are, so a file named + ``--load=plugin.cpp`` would make clang-tidy load a plugin, and one named + ``--files=list.cpp`` would make clang-format format the files listed in it. + Such a name gets a ``./`` prefix. """ - for arg in args: - name, _, value = arg.partition("=") - if name in names and value.lower() in ("", "true", "1"): - return True - return False + if arg.startswith("-") and os.path.isfile(arg): + return f"./{arg}" + return arg @lru_cache(maxsize=4) diff --git a/tests/test_clang_format.py b/tests/test_clang_format.py index 155bae9..d92590c 100644 --- a/tests/test_clang_format.py +++ b/tests/test_clang_format.py @@ -76,8 +76,8 @@ def test_run_clang_format_dry_run_unformatted(tmp_path): assert output.strip() # Should report which file needs formatting -def test_run_clang_format_dry_run_short_option_fails_unformatted(tmp_path): - """-n is --dry-run, so it also reports unformatted files as a failure.""" +def test_run_clang_format_short_dry_run_fails_unformatted(tmp_path): + """-n is short for --dry-run, so it also fails on unformatted files.""" test_file = tmp_path / "main.c" test_file.write_bytes(Path("testing/main.c").read_bytes()) ret, output = run_clang_format(["-n", "--style=Google", str(test_file)]) @@ -86,7 +86,7 @@ def test_run_clang_format_dry_run_short_option_fails_unformatted(tmp_path): def test_run_clang_format_dry_run_on_a_file_that_is_not_utf8(tmp_path): - """clang-format prints the source line; Latin-1 bytes must not crash the hook.""" + """clang-format prints source lines; Latin-1 bytes must not crash the hook.""" test_file = tmp_path / "latin1.c" test_file.write_bytes(b"int x = 1; /* caf\xe9 */\n") ret, output = run_clang_format(["--dry-run", str(test_file)]) @@ -236,30 +236,6 @@ def test_run_clang_format_formats_files_in_place(mock_clang_format): ), (["--Werror", "a.cpp"], ["clang-format", "-i", "--Werror", "a.cpp"]), (["a.cpp"], ["clang-format", "-i", "a.cpp"]), - # -n is clang-format's short form of --dry-run. - (["-n", "a.cpp"], ["clang-format", "-i", "--Werror", "-n", "a.cpp"]), - ( - ["--dry-run=true", "a.cpp"], - ["clang-format", "-i", "--Werror", "--dry-run=true", "a.cpp"], - ), - ( - ["--dry-run=false", "a.cpp"], - ["clang-format", "-i", "--dry-run=false", "a.cpp"], - ), - # Older clang-format versions read an empty value as true. - ( - ["--dry-run=", "a.cpp"], - ["clang-format", "-i", "--Werror", "--dry-run=", "a.cpp"], - ), - # Everything after "--" is a file name, so --Werror goes before it. - ( - ["-n", "--", "a.cpp"], - ["clang-format", "-i", "--Werror", "-n", "--", "a.cpp"], - ), - ( - ["-n", "--Werror=false", "a.cpp"], - ["clang-format", "-i", "-n", "--Werror=false", "a.cpp"], - ), ), ) def test_run_clang_format_adds_werror_only_for_dry_run( @@ -272,6 +248,20 @@ def test_run_clang_format_adds_werror_only_for_dry_run( assert mock_run.call_args.args[0] == expected_command +def test_run_clang_format_does_not_read_a_file_name_as_an_option( + mock_clang_format, tmp_path, monkeypatch +): + # A file named --files=list.cpp would make clang-format format the files + # listed in it, even outside the repository. + monkeypatch.chdir(tmp_path) + (tmp_path / "--files=list.cpp").write_text("/etc/hosts\n") + _, mock_run = mock_clang_format + + run_clang_format(["--files=list.cpp"]) + + assert mock_run.call_args.args[0] == ["clang-format", "-i", "./--files=list.cpp"] + + def test_run_clang_format_combines_stdout_and_stderr(mock_clang_format): _, mock_run = mock_clang_format mock_run.return_value = subprocess.CompletedProcess( diff --git a/tests/test_clang_tidy.py b/tests/test_clang_tidy.py index 7337cbd..d758f96 100644 --- a/tests/test_clang_tidy.py +++ b/tests/test_clang_tidy.py @@ -600,21 +600,10 @@ def test_exec_clang_tidy_return_value(returncode, stdout, stderr, expected_retva assert (ret, output) == (expected_retval, stdout + stderr) mock_run.assert_called_once_with( - ["clang-tidy", "a.cpp"], - capture_output=True, - encoding="utf-8", - errors="replace", - check=False, + ["clang-tidy", "a.cpp"], capture_output=True, encoding="utf-8", check=False ) -def test_exec_clang_tidy_reads_output_that_is_not_utf8(): - # Source lines in the output can be Latin-1 or GBK. - command = [sys.executable, "-c", "import sys; sys.stdout.buffer.write(b'caf\\xe9')"] - - assert _exec_clang_tidy(command) == (0, "caf\ufffd") - - def test_exec_clang_tidy_handles_missing_streams(): completed = MagicMock(returncode=0, stdout=None, stderr=None) with patch("cpp_linter_hooks.clang_tidy.subprocess.run", return_value=completed): @@ -790,20 +779,11 @@ def test_fix_flag_is_inserted_before_source_files(mock_exec): ) -@pytest.mark.parametrize( - "option", - [ - "--fix-errors", - "-fix-notes", - "--fix-notes", - "-export-fixes=fixes.yaml", - ], -) -def test_jobs_with_any_fix_option_spelling_forces_serial_execution(mock_exec, option): - run_clang_tidy(["--jobs=4", "-p", "./build", option, "a.cpp", "b.cpp"]) +def test_jobs_with_fix_notes_forces_serial_execution(mock_exec): + run_clang_tidy(["--jobs=4", "-p", "./build", "--fix-notes", "a.cpp", "b.cpp"]) mock_exec.assert_called_once_with( - ["clang-tidy", "-quiet", "-p", "./build", option, "a.cpp", "b.cpp"] + ["clang-tidy", "-quiet", "-p", "./build", "--fix-notes", "a.cpp", "b.cpp"] ) @@ -819,7 +799,7 @@ def test_file_named_like_an_option_is_not_read_as_one(tmp_path, monkeypatch, moc ) -@pytest.mark.parametrize("existing", ["-fix", "-fix-errors", "--fix-errors"]) +@pytest.mark.parametrize("existing", ["-fix", "-fix-errors"]) def test_fix_flag_does_not_add_a_second_fix_option(mock_exec, existing): run_clang_tidy(["--fix", "-p", "./build", existing, "a.cpp"]) @@ -828,15 +808,6 @@ def test_fix_flag_does_not_add_a_second_fix_option(mock_exec, existing): ) -@pytest.mark.parametrize("disabled", ["-fix-errors=false", "--fix-notes=0"]) -def test_fix_flag_is_added_when_a_fix_option_is_turned_off(mock_exec, disabled): - run_clang_tidy(["--fix", "-p", "./build", disabled, "a.cpp"]) - - mock_exec.assert_called_once_with( - ["clang-tidy", "-quiet", "-p", "./build", disabled, "-fix", "a.cpp"] - ) - - @pytest.mark.parametrize("db_dir", COMPILE_DB_SEARCH_DIRS) def test_compile_commands_auto_detect_each_search_dir( tmp_path, monkeypatch, mock_exec, db_dir @@ -1439,27 +1410,6 @@ def test_real_tool_keeps_the_lines_of_files_with_similar_names(braces_repo, tmp_ assert "data.cpp:2:" in output -def test_real_tool_checks_the_changed_lines_of_a_file_named_like_an_option( - braces_repo, tmp_path -): - # Without the ./ prefix, clang-tidy would read "-b.cpp" as an option. - dashed = tmp_path / "-b.cpp" - dashed.write_text("int g(int x) {\n if (x)\n return 1;\n return 0;\n}\n") - _git(tmp_path, "add", "--", "-b.cpp") - _git(tmp_path, "commit", "-q", "-m", "dashed") - dashed.write_text( - "int g(int x) {\n if (x)\n return 1;\n if (x > 1)\n return 2;\n" - " return 0;\n}\n" - ) - _git(tmp_path, "add", "--", "-b.cpp") - - ret, output = run_clang_tidy(CHECK_BRACES + ["-b.cpp"]) - - assert ret == 1 - assert "-b.cpp:4:" in output - assert "-b.cpp:2:" not in output - - # --- what happens without a compilation database --- diff --git a/tests/test_util.py b/tests/test_util.py index 45a135b..3279970 100644 --- a/tests/test_util.py +++ b/tests/test_util.py @@ -32,7 +32,6 @@ _versions_dir, resolve_install, resolve_install_with_diagnostics, - turns_on, ) # ── sample PyPI responses for consistent test data ────────────────────── @@ -1231,22 +1230,3 @@ def test_resolve_install_versions_coexist_without_pypi(): assert resolve_install("clang-format", "18.1.8") == v18 pypi.assert_not_called() install.assert_not_called() - - -@pytest.mark.parametrize( - ("args", "expected"), - ( - (["--dry-run"], True), - (["-n", "a.cpp"], True), - (["--dry-run=true"], True), - (["--dry-run=TRUE"], True), - (["--dry-run=1"], True), - (["--dry-run="], True), - (["--dry-run=false"], False), - (["--dry-run=0"], False), - (["--dry-run-x"], False), - (["a.cpp"], False), - ), -) -def test_turns_on(args, expected): - assert turns_on(args, ("-n", "--dry-run")) is expected