diff --git a/cpp_linter_hooks/clang_format.py b/cpp_linter_hooks/clang_format.py index f96f4d3..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 +from cpp_linter_hooks.util import file_argument, resolve_install_with_diagnostics parser = ArgumentParser() parser.add_argument( @@ -23,6 +23,7 @@ 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 ) @@ -35,19 +36,23 @@ 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 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: + 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") + command.extend(other_args) + try: # Run the clang-format command with captured output sp = subprocess.run( 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..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 +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,6 +133,8 @@ + r""")-[\w.*-]*)['"]?$""" ) QUOTES = ("'", '"') +# 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: @@ -665,6 +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] clang_tidy_args, notes = _repair_check_args(clang_tidy_args) lines = None @@ -717,23 +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 "-fix" not in clang_tidy_args - and "-fix-errors" not in clang_tidy_args - ): + # 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), 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 = 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 978702d..9b5983b 100644 --- a/cpp_linter_hooks/util.py +++ b/cpp_linter_hooks/util.py @@ -17,6 +17,19 @@ LOG = logging.getLogger(__name__) +def file_argument(arg: str) -> str: + """Return a file name that a clang tool 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, and one named + ``--files=list.cpp`` would make clang-format format the files listed in it. + Such a name gets a ``./`` prefix. + """ + if arg.startswith("-") and os.path.isfile(arg): + return f"./{arg}" + return arg + + @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 7fc3288..d92590c 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_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)]) + 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 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)]) + 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, ) @@ -209,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"], @@ -229,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 b25706e..d758f96 100644 --- a/tests/test_clang_tidy.py +++ b/tests/test_clang_tidy.py @@ -779,6 +779,26 @@ def test_fix_flag_is_inserted_before_source_files(mock_exec): ) +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", "--fix-notes", "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"]) def test_fix_flag_does_not_add_a_second_fix_option(mock_exec, existing): run_clang_tidy(["--fix", "-p", "./build", existing, "a.cpp"])