diff --git a/.bca-baseline.toml b/.bca-baseline.toml index a0a29dcb3..fffe27890 100644 --- a/.bca-baseline.toml +++ b/.bca-baseline.toml @@ -45,7 +45,7 @@ value = 69608.24451083591 path = "big-code-analysis-cli/src/commands/analyze.rs" qualified = "write_aggregate" metric = "halstead.effort" -value = 48544.21441032752 +value = 54210.895759662584 [[entry]] path = "big-code-analysis-cli/src/commands/check.rs" @@ -69,7 +69,7 @@ value = 6.0 path = "big-code-analysis-cli/src/commands/check.rs" qualified = "run_check" metric = "halstead.effort" -value = 62885.05529367418 +value = 63636.29428662941 [[entry]] path = "big-code-analysis-cli/src/commands/check/effective_config.rs" @@ -813,7 +813,7 @@ value = 64170.027458982564 path = "src/getter/elixir.rs" qualified = "ElixirCode::get_op_type" metric = "halstead.effort" -value = 56464.822908007496 +value = 54685.446169787065 [[entry]] path = "src/getter/irules.rs" @@ -831,7 +831,7 @@ value = 76538.42828561828 path = "src/getter/php.rs" qualified = "PhpCode::get_op_type" metric = "halstead.effort" -value = 54691.60133906107 +value = 71190.04074846079 [[entry]] path = "src/getter/python.rs" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 80b25aece..a9f6968e0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -594,6 +594,15 @@ jobs: # it from the aggregate recipe. - name: check-manpage-assets (explicit) run: python3 utils/check-manpage-assets.py + # Defensive twin for the man-page drift gate's self-tests + # (#1249). The gate itself runs in the `manpage` job, which + # invokes the script directly and so cannot be dropped by a + # refactor of the `make lint` aggregate — but the tests that + # prove the gate still catches an untracked page can be, and a + # gate that has silently stopped gating is indistinguishable + # from a clean tree. That is the failure this pair exists for. + - name: check-manpage-drift-test (explicit) + run: python3 -m unittest -q utils/check-manpage-drift-test.py # Defensive twin for the diagnostic-prefix gate (#1199): a # capitalised `Warning:` / `Error:` / `Note:` baked into a string # literal bypasses the one-helper-per-crate severity ladder and @@ -971,12 +980,16 @@ jobs: - uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2 - name: Regenerate man pages run: cargo xtask + # Fails on every direction the regenerated tree can differ from + # the committed one — modified, deleted, *and* added pages. The + # added direction needs `git ls-files --others`: the page xtask + # writes for a brand-new subcommand is untracked, and `git diff` + # alone passed green over it until #1249. The check lives in the + # script rather than inline here so this step and + # `make manpages-check` cannot drift apart; the `::error::` + # annotation is emitted by the script when GITHUB_ACTIONS is set. - name: Verify no drift - run: | - if ! git diff --exit-code -- man/; then - echo "::error::Committed man pages drift from the clap schema. Run 'cargo xtask' locally and commit the result." - exit 1 - fi + run: python3 utils/check-manpage-drift.py # Single required status check for branch protection — depends on # every gating job. Passing this one check guarantees the full matrix diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index dfe171c93..c1acaa5d7 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -309,13 +309,17 @@ repos: # Catch clap-vs-man-page drift before push. The # `.github/workflows/ci.yml` `manpage` job (Regenerate, then - # `git diff --exit-code -- man/`) is the upstream backstop; + # assert no drift) is the upstream backstop; # this hook makes the same check available locally and via # `pre-commit run --all-files`. The hook fails on drift and # leaves the regenerated files in the working tree — same # side effect CI runs into, accepted because man pages are # generated artifacts. # + # Since #1249 the assertion half lives in + # `utils/check-manpage-drift.py`, shared with the CI job, and + # covers added pages as well as modified and deleted ones. + # # The `files:` filter covers every input that can change the # generated `.1` output: # - clap definitions in the two CLI crates and xtask itself @@ -333,6 +337,18 @@ repos: entry: make manpages-check pass_filenames: false + # Self-tests for the man-page drift gate. Separate hook so the + # gate above stays a regenerate-then-assert pair and the tests + # get their own pass/fail in `pre-commit run` output. They run + # against scratch repositories only — never the real `man/`, + # which the `manpages` hook rewrites. + - id: check-manpage-drift-test + name: check-manpage-drift-test + language: system + files: '^utils/check-manpage-drift(-test)?\.py$' + entry: python3 -m unittest -q utils/check-manpage-drift-test.py + pass_filenames: false + # Lockstep version invariant — every owned crate and every # `=` internal-dep pin must match the workspace version. # See RELEASING.md "Lockstep version policy". diff --git a/.rustfmt-bail-baseline.txt b/.rustfmt-bail-baseline.txt index a43330e70..9e6213c29 100644 --- a/.rustfmt-bail-baseline.txt +++ b/.rustfmt-bail-baseline.txt @@ -38,6 +38,17 @@ # tests/common/mod.rs an `insta::with_settings!` invocation, # whose `key => value` token tree is not a # match at all +# src/vcs/error.rs `classify_error_variants!` (#1245): its +# matcher, plus the 11 `$pat => $sample` +# entries at the invocation, which are +# `pattern => expression` pairs and so +# parse as neither. Deliberate: generating +# `is_client_input` and +# `client_input_samples` from one list is +# what makes a new client-input variant a +# compile error. Those 11 lines sit outside +# `cargo fmt` — keep them one per line and +# under 100 columns by hand. # # Every remaining entry is cause 1, and all of them share one shape: # a section comment sitting between `|` alternatives of a single long @@ -99,7 +110,7 @@ src/getter.rs 2 src/getter/bash.rs 3 src/getter/c.rs 3 src/getter/cpp.rs 6 -src/getter/csharp.rs 2 +src/getter/csharp.rs 5 src/getter/elixir.rs 2 src/getter/go.rs 1 src/getter/groovy.rs 4 @@ -110,7 +121,7 @@ src/getter/lua.rs 3 src/getter/mozcpp.rs 6 src/getter/objc.rs 3 src/getter/perl.rs 2 -src/getter/php.rs 2 +src/getter/php.rs 8 src/getter/python.rs 7 src/getter/ruby.rs 2 src/getter/tcl.rs 4 @@ -126,4 +137,5 @@ src/metrics/cyclomatic/php.rs 4 src/metrics/cyclomatic/ruby.rs 5 src/metrics/loc/perl.rs 6 src/metrics/loc/python.rs 6 +src/vcs/error.rs 12 tests/common/mod.rs 2 diff --git a/AGENTS.md b/AGENTS.md index 613d1eb8b..d3e347402 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -61,7 +61,8 @@ and `cargo run -p big-code-analysis-web --`. - `utils/` — repo-maintenance helper scripts, including the gates run by `make pre-commit` / `make ci`: `check-versions.py`, `check-snapshot-anchors.py`, `check-rustfmt-bail.py`, - `check-manpage-assets.py`, `check-diagnostic-prefix.py`, + `check-manpage-assets.py`, `check-manpage-drift.py`, + `check-diagnostic-prefix.py`, `check-grammar-marker-sync.py`, `check-enums-codegen-drift.sh`, `check-grammar-crate.py`, `check-grammars-crates.sh`, `check-excluded-manifests.py`, `check-ruff-lockstep.py`, @@ -267,8 +268,10 @@ cannot run), `cargo doc --no-deps --workspace --all-features` with `RUSTDOCFLAGS="-D warnings"`, `cargo +nightly udeps`, the markdown / TOML / shell / Makefile / GitHub Actions lint families, the man-page -drift gate (`cargo xtask` + `git diff --exit-code -- man/`, mirroring -the `manpage` CI job), the diagnostic-prefix gate +drift gate (`cargo xtask` + `utils/check-manpage-drift.py`, mirroring +the `manpage` CI job, which calls the same script — it fails on +modified, deleted, **and** newly added pages, the last of which +`git diff` alone cannot see, #1249), the diagnostic-prefix gate (`make check-diagnostic-prefix`, which blocks a capitalised `Warning:` / `Error:` / `Note:` string literal — see "Rust conventions"), the bca self-scan threshold gate at both diff --git a/CHANGELOG.md b/CHANGELOG.md index d1423bb12..c8c3911fe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -90,6 +90,10 @@ for historical reference. against a lockfile resolving 0.15.22), which stays invisible until the two versions disagree and then presents as "works locally, red in CI" (#1230). +- **web:** `error_kind` token `vcs_invalid_author_hash_key`. The + `STABILITY.md` vocabulary list also gains `not_acceptable` and + `serialize_failed`, added by #657 and never documented (#1245). + ### Fixed - `fix_includes` returned its diagnostics in a different order on every @@ -117,6 +121,93 @@ for historical reference. `cpp_ast_source_reflects_preproc_expansion`; the `C` arm had nothing. Found while verifying that the new `preproc_macro` fuzz target is not vacuous (#1154). + +- **Halstead, PHP:** variable references no longer count twice. `$x` parses + as a `variable_name` wrapping a `name` leaf and both were classified as + operands, so `N2` roughly doubled on variable-dense PHP and `n2` carried a + sigil-less twin per variable; variable-variable syntax compounded it (`$$a` + scored 3, `$$$b` scored 4). `Name` is now suppressed under a + `variable_name` or `dynamic_variable_name` parent, and those wrappers under + a `dynamic_variable_name` parent, so every reference counts once at any + nesting depth. Volume, difficulty, effort and MI change for every PHP file + (#1259). + +- **Halstead, Elixir and C#:** `true` / `false` / `nil` / `null` literals no + longer count twice in `N2`. The grammar wraps the keyword leaf in a named + literal node and `get_op_type` classified both as operands; because + operands are keyed by source text, `n2` hid the duplication while `N2` — + and the length, volume, difficulty and effort derived from it — inflated by + one per literal occurrence. Elixir drops the leaves outright; C# parent- + guards them, because `operator true` emits a bare leaf with no + `boolean_literal` wrapper (#1253). + +- **ABC, Java and Groovy:** generic type syntax is no longer counted as + conditions. A generic declaration (`class Gen`, ` void m()`, Groovy's + `def U m(U x)`) scored two conditions per bracket pair, and a wildcard + bound (`List`) scored one as a phantom ternary. Both tokens + are now gated on their parent node kind, matching the polarity C, C++, + Objective-C, mozcpp, Rust and Go already use (#1274). + +- **npm, C++ and Mozcpp:** a templated member function with an inline body + was not counted. The `template_declaration` guard resolves through a shared + helper that hunted a `function_declarator`, but a templated member *with a + body* parses as `template_declaration > function_definition` and has no + such node at that level — so the method scored zero while `nom` opened a + function space for it and `wmc` weighted its cyclomatic, leaving three + metrics disagreeing about one class. The helper now accepts a + `function_definition` child outright rather than recursing into it, which + also fixes a templated conversion operator (whose declarator is an + `operator_cast`, so no `function_declarator` exists at any depth). C++ + `npm` values rise on classes with templated inline-bodied members (#1258). + +- `bca count -t call` and `bca find call` reported 0 on all C, C++ and Mozcpp + source. `Checker::is_call` matched only the unsuffixed `CallExpression` + kind_id, which is the grammar's always-aliased `preproc_call_expression` + and never reaches `kind_id()`; every real call carries the aliased variant. + No metric values are affected (#1254). + +- **web:** a misused `author_hash_key` on `POST /v1/vcs` and `/v1/vcs/trend` + reported `error_kind: "vcs_internal_error"`, the token reserved for backend + faults, so a client branching on the token saw its own mistake as a server + failure. It now carries `vcs_invalid_author_hash_key`. The status was + already correct (`400`) (#1245). + +- `bca metrics --output ` and `bca ops --output ` wrote the + aggregate document's per-file elements in worker-completion order, so two + runs over an unchanged tree produced differently-ordered files at + `--jobs > 1`. The elements are now sorted by emitted path in every format + (JSON, YAML, TOML, CBOR, CSV), making the artifact diffable and usable as a + cache key. Multi-seed runs change too: the walk sorts per seed and + concatenates, so `bca metrics -p b -p a --output x.json` previously emitted + `b`'s files first and now emits one globally sorted document (#1244). + +- `bca check`'s remediation block printed a baseline-refresh command in the + pre-#597 flag order (`bca --paths … check …`), which exits 1 with a clap + usage error. The command now names the `check` subcommand first and mirrors + every flag that decides what `--write-baseline` records — including + `--threshold`, `--no-config`, `--check-exclude` and `--check-exclude-from`, + without which the suggested refresh wrote a different baseline than the + gate measured, or failed outright (#1243). + +- The man-page drift gate now fails on a *newly generated* page. + `git diff --exit-code -- man/` reports tracked content only, so the page + `cargo xtask` writes for a brand-new subcommand passed both the CI + `manpage` job and `make manpages-check` green and silently never shipped. + The check moved out of the two hand-mirrored shell blocks into + `utils/check-manpage-drift.py`, which both sites now call, and covers + modified, deleted, and added pages (#1249). + +- `cargo xtask` deleted the man page it had just written when a command was + renamed case-only, on macOS/APFS and Windows/NTFS, while still exiting 0. + `render_man_page`'s collision guard folded ASCII case; `sweep_orphans` + compared byte-for-byte. Because case-insensitive filesystems are also + case-preserving, the write for `BCA.1` landed in the existing `bca.1` + directory entry and the case-sensitive sweep then unlinked it as an orphan. + The relation now has a single definition that both sites call, and the + sweep classifies three ways rather than two: byte-equal keeps, no match + removes, and a case-only match is refused with an error naming every + conflicting spelling (#1250). + - The pre-tag `cargo publish --dry-run` for `big-code-analysis` was skipped on every release, not only the first. It was gated on a crates.io sparse index probe for a leaf version that the Lockstep policy guarantees is the diff --git a/Makefile b/Makefile index 6ecfd0750..c8efae943 100644 --- a/Makefile +++ b/Makefile @@ -88,7 +88,7 @@ find-by-ext = $(if $(FD),$(FD) --extension $(1) $(FD_EXCLUDE) $(2),find . -name NEXTEST := $(shell command -v cargo-nextest 2>/dev/null) TEST_CMD = $(if $(NEXTEST),$(NEXTEST) nextest run --workspace --all-features,cargo test --workspace --all-features --lib --bins --tests) -.PHONY: help check-tools worktree-setup worktree-setup-test build build-release check test test-doc chain-audit fmt fmt-check markdown-fmt markdown-lint shellcheck sh-fmt sh-fmt-check toml-fmt toml-fmt-check toml-lint makefile-check actionlint snapshot-anchors snapshot-anchors-test rustfmt-bail rustfmt-bail-test grammar-marker-sync grammar-marker-sync-test check-versions check-excluded-manifests check-excluded-manifests-test check-ruff-lockstep check-ruff-lockstep-test check-publish-metadata check-publish-metadata-test check-manpage-assets check-diagnostic-prefix check-diagnostic-prefix-test gate-status-test check-tools-test enums-check enums-codegen-drift enums-codegen-drift-test self-scan self-scan-headroom self-scan-write-baseline self-scan-write-baseline-headroom vcs lint clippy udeps insta-review insta-accept clean distclean install install-cli install-web doc doc-open doc-check doc-check-docsrs book book-serve book-pot book-po-update book-ja book-deploy all pre-commit ci release-check verify-changelog pkg-deb-local pkg-rpm-local dev-env-build dev-env-run dev-env-shell dev-env-rm py-bootstrap py-sync py-relock py-clean py-fmt py-fmt-check py-lint py-typecheck py-test py-stubtest smoke smoke-cli smoke-lib bench bench-scaling bench-walk fuzz-check fuzz-smoke fuzz-replay fuzz-run fuzz-tmin _check-find _pc-all _pc-fmt _pc-clippy _pc-test _pc-doc-check _pc-udeps _pc-shellcheck _pc-markdown-lint _pc-toml-lint _pc-makefile-check _pc-actionlint _pc-snapshot-anchors _pc-snapshot-anchors-test _pc-rustfmt-bail _pc-rustfmt-bail-test _pc-grammar-marker-sync _pc-grammar-marker-sync-test _pc-check-versions _pc-check-versions-test _pc-check-grammar-crate-test _pc-check-excluded-manifests _pc-check-excluded-manifests-test _pc-check-ruff-lockstep _pc-check-ruff-lockstep-test _pc-check-publish-metadata _pc-check-publish-metadata-test _pc-check-manpage-assets _pc-check-diagnostic-prefix _pc-check-diagnostic-prefix-test _pc-worktree-setup-test _pc-gate-status-test _pc-check-tools-test _pc-enums-check _pc-enums-codegen-drift _pc-enums-codegen-drift-test _pc-self-scan _pc-self-scan-headroom _pc-py-fmt _pc-py-typecheck _pc-py-test _pc-py-stubtest _ci-all _ci-fmt-check _ci-clippy _ci-test _ci-doc-check _ci-build _ci-udeps _ci-shellcheck _ci-markdown-lint _ci-toml-lint _ci-makefile-check _ci-actionlint _ci-snapshot-anchors _ci-snapshot-anchors-test _ci-rustfmt-bail _ci-rustfmt-bail-test _ci-grammar-marker-sync _ci-grammar-marker-sync-test _ci-check-versions _ci-check-versions-test _ci-check-grammar-crate-test _ci-check-excluded-manifests _ci-check-excluded-manifests-test _ci-check-ruff-lockstep _ci-check-ruff-lockstep-test _ci-check-publish-metadata _ci-check-publish-metadata-test _ci-check-manpage-assets _ci-check-diagnostic-prefix _ci-check-diagnostic-prefix-test _ci-worktree-setup-test _ci-gate-status-test _ci-check-tools-test _ci-enums-check _ci-enums-codegen-drift _ci-enums-codegen-drift-test _ci-enums-codegen-drift-test _ci-self-scan _ci-self-scan-headroom _ci-cargo-pipeline _ci-py-fmt-check _ci-py-lint _ci-py-typecheck _ci-py-test _ci-py-stubtest +.PHONY: help check-tools worktree-setup worktree-setup-test build build-release check test test-doc chain-audit fmt fmt-check markdown-fmt markdown-lint shellcheck sh-fmt sh-fmt-check toml-fmt toml-fmt-check toml-lint makefile-check actionlint snapshot-anchors snapshot-anchors-test rustfmt-bail rustfmt-bail-test grammar-marker-sync grammar-marker-sync-test check-versions check-excluded-manifests check-excluded-manifests-test check-ruff-lockstep check-ruff-lockstep-test check-publish-metadata check-publish-metadata-test check-manpage-assets check-manpage-drift-test check-diagnostic-prefix check-diagnostic-prefix-test gate-status-test check-tools-test enums-check enums-codegen-drift enums-codegen-drift-test self-scan self-scan-headroom self-scan-write-baseline self-scan-write-baseline-headroom vcs lint clippy udeps insta-review insta-accept clean distclean install install-cli install-web doc doc-open doc-check doc-check-docsrs book book-serve book-pot book-po-update book-ja book-deploy all pre-commit ci release-check verify-changelog pkg-deb-local pkg-rpm-local dev-env-build dev-env-run dev-env-shell dev-env-rm py-bootstrap py-sync py-relock py-clean py-fmt py-fmt-check py-lint py-typecheck py-test py-stubtest smoke smoke-cli smoke-lib bench bench-scaling bench-walk fuzz-check fuzz-smoke fuzz-replay fuzz-run fuzz-tmin _check-find _pc-all _pc-fmt _pc-clippy _pc-test _pc-doc-check _pc-udeps _pc-shellcheck _pc-markdown-lint _pc-toml-lint _pc-makefile-check _pc-actionlint _pc-snapshot-anchors _pc-snapshot-anchors-test _pc-rustfmt-bail _pc-rustfmt-bail-test _pc-grammar-marker-sync _pc-grammar-marker-sync-test _pc-check-versions _pc-check-versions-test _pc-check-grammar-crate-test _pc-check-excluded-manifests _pc-check-excluded-manifests-test _pc-check-ruff-lockstep _pc-check-ruff-lockstep-test _pc-check-publish-metadata _pc-check-publish-metadata-test _pc-check-manpage-assets _pc-check-manpage-drift-test _pc-check-diagnostic-prefix _pc-check-diagnostic-prefix-test _pc-worktree-setup-test _pc-gate-status-test _pc-check-tools-test _pc-enums-check _pc-enums-codegen-drift _pc-enums-codegen-drift-test _pc-self-scan _pc-self-scan-headroom _pc-py-fmt _pc-py-typecheck _pc-py-test _pc-py-stubtest _ci-all _ci-fmt-check _ci-clippy _ci-test _ci-doc-check _ci-build _ci-udeps _ci-shellcheck _ci-markdown-lint _ci-toml-lint _ci-makefile-check _ci-actionlint _ci-snapshot-anchors _ci-snapshot-anchors-test _ci-rustfmt-bail _ci-rustfmt-bail-test _ci-grammar-marker-sync _ci-grammar-marker-sync-test _ci-check-versions _ci-check-versions-test _ci-check-grammar-crate-test _ci-check-excluded-manifests _ci-check-excluded-manifests-test _ci-check-ruff-lockstep _ci-check-ruff-lockstep-test _ci-check-publish-metadata _ci-check-publish-metadata-test _ci-check-manpage-assets _ci-check-manpage-drift-test _ci-check-diagnostic-prefix _ci-check-diagnostic-prefix-test _ci-worktree-setup-test _ci-gate-status-test _ci-check-tools-test _ci-enums-check _ci-enums-codegen-drift _ci-enums-codegen-drift-test _ci-enums-codegen-drift-test _ci-self-scan _ci-self-scan-headroom _ci-cargo-pipeline _ci-py-fmt-check _ci-py-lint _ci-py-typecheck _ci-py-test _ci-py-stubtest # Default target help: @@ -143,6 +143,7 @@ help: @echo " check-publish-metadata Assert every publishable crate carries the metadata crates.io needs" @echo " check-publish-metadata-test Self-tests for the check-publish-metadata gate" @echo " check-manpage-assets Assert every bca-*.1 man page is in deb+rpm asset lists" + @echo " check-manpage-drift-test Self-tests for the man-page drift gate" @echo " check-diagnostic-prefix Block capitalised Warning:/Error:/Note: literals" @echo " check-diagnostic-prefix-test Self-tests for the diagnostic-prefix gate" @echo " worktree-setup-test Self-tests for the worktree-setup submodule classifier" @@ -498,17 +499,30 @@ manpages: # job in `.github/workflows/ci.yml`, this recipe runs `cargo xtask` # (which rewrites `man/*.1` in place — the same side effect CI # accepts on its ephemeral runners) and then fails if the resulting -# tree differs from the index. Used by `_ci-manpages` and -# `_pc-manpages`. Locally, contributors with hand-edited `.1` files -# will see those edits overwritten; man pages are generated -# artifacts and should not be hand-edited. +# tree differs from what is committed — modified, deleted, *and* +# added pages (#1249; `git diff` alone cannot see an untracked new +# page). Used by `_ci-manpages` and `_pc-manpages`. Locally, +# contributors with hand-edited `.1` files will see those edits +# overwritten; man pages are generated artifacts and should not be +# hand-edited. +# +# The assertion itself lives in `utils/check-manpage-drift.py`, which +# the CI job's "Verify no drift" step calls too, so the two sites +# cannot drift apart. Its self-tests are `check-manpage-drift-test`. manpages-check: @echo "Checking man pages match clap schema..." @cargo xtask - @if ! git diff --exit-code -- man/; then \ - echo "ERROR: man pages drift from the clap schema. The regenerated files are already in your working tree — run 'git add man/' and commit alongside the clap change."; \ - exit 1; \ - fi + @python3 $(BASE_DIR)utils/check-manpage-drift.py + +# Self-tests for the man-page drift gate. Kept separate from +# `manpages-check` (matching the check-versions-test pattern) so the +# gate stays a regenerate-then-assert pair and the test failures get +# their own clean parallel-arm output in the pre-commit/CI DAG. The +# tests run against scratch repositories only — never the real `man/`, +# which `_pc-manpages` rewrites concurrently. +check-manpage-drift-test: + @echo "Running manpage-drift self-tests..." + @(cd $(BASE_DIR) && python3 -m unittest -q utils/check-manpage-drift-test.py) # Lockstep-version invariant: every owned crate and every internal # `=` dep pin must equal `[workspace.package].version`. See @@ -1308,7 +1322,7 @@ lint: $(MAKE) -j --output-sync=target \ _ci-clippy \ _ci-shellcheck _ci-markdown-lint _ci-toml-lint _ci-makefile-check \ - _ci-actionlint _ci-snapshot-anchors _ci-snapshot-anchors-test _ci-rustfmt-bail _ci-rustfmt-bail-test _ci-grammar-marker-sync _ci-grammar-marker-sync-test _ci-check-versions _ci-check-versions-test _ci-check-grammar-crate-test _ci-check-excluded-manifests _ci-check-excluded-manifests-test _ci-check-ruff-lockstep _ci-check-ruff-lockstep-test _ci-check-publish-metadata _ci-check-publish-metadata-test _ci-check-manpage-assets _ci-check-diagnostic-prefix _ci-check-diagnostic-prefix-test _ci-worktree-setup-test _ci-gate-status-test _ci-check-tools-test _ci-enums-check _ci-enums-codegen-drift _ci-enums-codegen-drift-test + _ci-actionlint _ci-snapshot-anchors _ci-snapshot-anchors-test _ci-rustfmt-bail _ci-rustfmt-bail-test _ci-grammar-marker-sync _ci-grammar-marker-sync-test _ci-check-versions _ci-check-versions-test _ci-check-grammar-crate-test _ci-check-excluded-manifests _ci-check-excluded-manifests-test _ci-check-ruff-lockstep _ci-check-ruff-lockstep-test _ci-check-publish-metadata _ci-check-publish-metadata-test _ci-check-manpage-assets _ci-check-manpage-drift-test _ci-check-diagnostic-prefix _ci-check-diagnostic-prefix-test _ci-worktree-setup-test _ci-gate-status-test _ci-check-tools-test _ci-enums-check _ci-enums-codegen-drift _ci-enums-codegen-drift-test # --------------------------------------------------------------------------- # Maintenance @@ -1473,7 +1487,7 @@ _pc-all: $(MAKE) -j --output-sync=target \ _pc-test \ _pc-shellcheck _pc-markdown-lint _pc-toml-lint _pc-makefile-check \ - _pc-actionlint _pc-snapshot-anchors _pc-snapshot-anchors-test _pc-rustfmt-bail _pc-rustfmt-bail-test _pc-grammar-marker-sync _pc-grammar-marker-sync-test _pc-check-versions _pc-check-versions-test _pc-check-grammar-crate-test _pc-check-excluded-manifests _pc-check-excluded-manifests-test _pc-check-ruff-lockstep _pc-check-ruff-lockstep-test _pc-check-publish-metadata _pc-check-publish-metadata-test _pc-check-manpage-assets _pc-check-diagnostic-prefix _pc-check-diagnostic-prefix-test _pc-worktree-setup-test _pc-gate-status-test _pc-check-tools-test _pc-enums-check _pc-enums-codegen-drift _pc-enums-codegen-drift-test \ + _pc-actionlint _pc-snapshot-anchors _pc-snapshot-anchors-test _pc-rustfmt-bail _pc-rustfmt-bail-test _pc-grammar-marker-sync _pc-grammar-marker-sync-test _pc-check-versions _pc-check-versions-test _pc-check-grammar-crate-test _pc-check-excluded-manifests _pc-check-excluded-manifests-test _pc-check-ruff-lockstep _pc-check-ruff-lockstep-test _pc-check-publish-metadata _pc-check-publish-metadata-test _pc-check-manpage-assets _pc-check-manpage-drift-test _pc-check-diagnostic-prefix _pc-check-diagnostic-prefix-test _pc-worktree-setup-test _pc-gate-status-test _pc-check-tools-test _pc-enums-check _pc-enums-codegen-drift _pc-enums-codegen-drift-test \ _pc-manpages \ _pc-self-scan _pc-self-scan-headroom \ _pc-py-fmt _pc-py-typecheck _pc-py-test _pc-py-stubtest @@ -1483,7 +1497,7 @@ _ci-all: $(MAKE) -j --output-sync=target \ _ci-cargo-pipeline \ _ci-shellcheck _ci-markdown-lint _ci-toml-lint _ci-makefile-check \ - _ci-actionlint _ci-snapshot-anchors _ci-snapshot-anchors-test _ci-rustfmt-bail _ci-rustfmt-bail-test _ci-grammar-marker-sync _ci-grammar-marker-sync-test _ci-check-versions _ci-check-versions-test _ci-check-grammar-crate-test _ci-check-excluded-manifests _ci-check-excluded-manifests-test _ci-check-ruff-lockstep _ci-check-ruff-lockstep-test _ci-check-publish-metadata _ci-check-publish-metadata-test _ci-check-manpage-assets _ci-check-diagnostic-prefix _ci-check-diagnostic-prefix-test _ci-worktree-setup-test _ci-gate-status-test _ci-check-tools-test _ci-enums-check _ci-enums-codegen-drift _ci-enums-codegen-drift-test \ + _ci-actionlint _ci-snapshot-anchors _ci-snapshot-anchors-test _ci-rustfmt-bail _ci-rustfmt-bail-test _ci-grammar-marker-sync _ci-grammar-marker-sync-test _ci-check-versions _ci-check-versions-test _ci-check-grammar-crate-test _ci-check-excluded-manifests _ci-check-excluded-manifests-test _ci-check-ruff-lockstep _ci-check-ruff-lockstep-test _ci-check-publish-metadata _ci-check-publish-metadata-test _ci-check-manpage-assets _ci-check-manpage-drift-test _ci-check-diagnostic-prefix _ci-check-diagnostic-prefix-test _ci-worktree-setup-test _ci-gate-status-test _ci-check-tools-test _ci-enums-check _ci-enums-codegen-drift _ci-enums-codegen-drift-test \ _ci-py-fmt-check _ci-py-lint _ci-py-typecheck _ci-py-test _ci-py-stubtest # --------------------------------------------------------------------------- @@ -1616,6 +1630,9 @@ _pc-check-versions-test: _pc-fmt _pc-check-grammar-crate-test: _pc-fmt $(MAKE) check-grammar-crate-test +_pc-check-manpage-drift-test: _pc-fmt + $(MAKE) check-manpage-drift-test + _pc-check-excluded-manifests: _pc-fmt $(MAKE) check-excluded-manifests @@ -1810,6 +1827,9 @@ _ci-check-versions-test: _ci-check-grammar-crate-test: $(MAKE) check-grammar-crate-test +_ci-check-manpage-drift-test: + $(MAKE) check-manpage-drift-test + _ci-check-excluded-manifests: $(MAKE) check-excluded-manifests diff --git a/STABILITY.md b/STABILITY.md index 4683ef265..afa3c3f29 100644 --- a/STABILITY.md +++ b/STABILITY.md @@ -1,12 +1,11 @@ # Stability policy -`big-code-analysis` is on the `2.x` line (currently `2.1.0`). The -crate graduated from the rapid-iteration `0.x` window into a written -stability contract and has now made its first major (`2.0`) break: -callers can pin a minor version and expect a working tree against any -subsequent patch or minor bump, without code edits. This document -spells out exactly what that contract covers across the `2.x` line, and -the narrow seams where it deliberately does not reach. +`big-code-analysis` is on the `2.x` line (currently `2.1.0`). This +document is the public-API stability contract: what is stable, what +counts as an additive change, what is reserved for a major bump, and +the narrow seams where the contract deliberately does not reach. +Within `2.x`, callers can pin a minor version and expect a working +tree against any subsequent patch or minor bump, without code edits. ## Versioning scheme @@ -74,7 +73,7 @@ section. `MetricsOptions::default()` plus `with_*` setters; the contract is the builder methods, not the field representation (#533). The parser-generic `metrics` / `metrics_with_options` functions and the - `MetricsCfg` builder were removed at `2.0` in favour of `analyze` / + `MetricsCfg` builder were removed at `2.0` in favor of `analyze` / `Ast`. - `Metric`, `MetricSet` in `src/metric_set.rs`: `Metric` carries `#[non_exhaustive]`, so adding variants is additive. `MetricSet` @@ -113,7 +112,7 @@ section. functions (`function`, `find`, `count`, `rm_comments`) are `pub(crate)`, not part of the public surface, and the parser-generic `operands_and_operators` function was removed in - favour of `Ast::ops`. `Ops::operators` and `Ops::operands` are + favor of `Ast::ops`. `Ops::operators` and `Ops::operands` are sorted in byte-lexicographic order **of the rendered strings** as of the `2.1` line; before that they were documented as arbitrary and did in fact vary run to run. The distinction @@ -128,7 +127,7 @@ section. budget shared with a dedicated producer thread and `run` spawned `max(2, num_jobs) - 1` consumers; since #1114 dispatch happens on the calling thread and all `num_jobs` go to consumers. The signature - did not change, so this is a behavioural change, not a source break: + did not change, so this is a behavioral change, not a source break: a caller passing `n` gets one more worker than before. `without_path_verification` (added #1114) opts out of the per-path `is_file()` check for a caller whose own traversal already @@ -253,7 +252,7 @@ rendered projections** (like the AST `bca report` output): the page round-trip format; do not parse them. The 2.0 line restructured the AST report's presentation deliberately: hotspot section titles now follow one ` hotspots (top N by )` template (#677), the WMC table is -labelled "Types" (#687), and the HTML section anchors derive from a stable +labeled "Types" (#687), and the HTML section anchors derive from a stable ` hotspots` slug independent of `--top`, so a deep link minted against a `1.x` report may need re-fragmenting. The VCS report's `(total)` column headers became `(long)` (#592). These are presentation changes only; @@ -284,7 +283,7 @@ surface: `big_code_analysis::vcs::{PerFunctionBlame, BlameSession, LineSpan}`, the `vcs::Error::Blame` variant, the `bca metrics --vcs-per-function` flag, and the `CodeMetrics::vcs` field now being populated on nested function -spaces (not only the file space). The per-function block shares +spaces (not just the file space). The per-function block shares `wire::Vcs`'s shape and the same `risk_score` / `*_version` contract above, but its numbers are a `git blame` *current snapshot* (`churn` is surviving-line count, not the file-level added+deleted churn), so values @@ -357,7 +356,7 @@ by `bca vcs --no-cache` / `--clear-cache` / `--cache-dir` (and reused transparently by `bca metrics --vcs` / `bca report --vcs`), the `POST /vcs` `no_cache` / `cache_dir` fields, and the `vcs_metrics(no_cache=…, cache_dir=…)` parameters. The contract is purely -behavioural (a cache hit produces output **bit-identical** to an uncached +behavioral (a cache hit produces output **bit-identical** to an uncached `build_history_index` at the same reference time), so the cache never changes the `wire::Vcs` shape or any metric value. The **on-disk file format** (the `HistoryCache` / `CommitEvent` JSON, versioned by @@ -588,7 +587,7 @@ reach the raw tree-sitter surface. the same value-not-stable sense as the re-export above. The former `Parser::from_tree` / `metrics_from_tree` entry points were removed at `2.0` (`Parser::from_tree` demoted to `pub(crate)` along with - `Parser`; the parser-generic `metrics_from_tree` deleted) in favour + `Parser`; the parser-generic `metrics_from_tree` deleted) in favor of this single explicit-name seam. - **`Ast`** is the parse-once seam and, as of `2.0`, the single public analysis entry point. `Ast::as_tree_sitter` exposes the held @@ -607,7 +606,7 @@ reach the raw tree-sitter surface. is exposed. At `2.0` the path-positional `get_function_spaces*` / `metrics_from_tree` / `get_ops` shims, the parser-generic `metrics` / `metrics_with_options` / `operands_and_operators` functions, and the - `action` / `Callback` dispatch were all removed in favour of these + `action` / `Callback` dispatch were all removed in favor of these explicit-name `Source` / `Ast` seams (the old path-positional forms derived identity from a lossy path). - **`#[doc(hidden)]` items** are part of the macro / internal @@ -655,7 +654,7 @@ mis-matching. Recent transitions: sole case matching consults it. Elsewhere the field re-rendered on every edit above the function, churning diffs and conflicting on every merge. v2–v5 baselines read unchanged: a recorded line is - still honoured exactly as before. A v6 file read by a pre-v6 bca + still honored exactly as before. A v6 file read by a pre-v6 bca fails to parse (a required field is missing) rather than mis-matching; upgrade bca or regenerate with `--write-baseline`. The optionality reaches four rendered surfaces, so an entry with no @@ -671,11 +670,11 @@ mis-matching. Recent transitions: optional `body_hash` field backs `--baseline-fuzzy-match`. v2/v3 baselines are still read: their bare `function` name maps to `qualified` via a serde alias and matching degrades to bare-name + - tolerance (equivalent to the old behaviour, now line-drift-tolerant) + tolerance (equivalent to the old behavior, now line-drift-tolerant) until the file is refreshed with `--write-baseline`. No re-analysis is performed at load time. - **v2 → v3** ([#376](https://github.com/dekobon/big-code-analysis/issues/376)): - path keys are canonicalised relative to the baseline file's own + path keys are canonicalized relative to the baseline file's own directory. ASCII-clean v2 baselines migrate transparently with a one-time stderr notice; v2 baselines with pre-encoded non-ASCII paths may need a `--write-baseline` refresh. @@ -714,7 +713,7 @@ at the crate root from `src/output/`: | `write_csv` | `(space: &FuncSpace, source_path: &Path, writer: W) -> io::Result<()>` | | `write_csv_aggregate` | `(spaces: impl IntoIterator, writer: W) -> io::Result<()>`; one shared header, then every tree's rows | | `write_sarif` | `(offenders: &[OffenderRecord], writer: W) -> io::Result<()>` | -| `write_sarif_with_suppressed` | `(active, suppressed: &[OffenderRecord], writer: W) -> io::Result<()>` | +| `write_sarif_with_suppressed` | `(active, in_source, baseline: &[OffenderRecord], writer: W) -> io::Result<()>`. The two suppressed slices stay separate because they render different `suppressions[].kind` values: `in_source` gives `"inSource"`, `baseline` gives `"external"` | | `write_checkstyle` | `(offenders: &[OffenderRecord], writer: W) -> io::Result<()>` | | `write_clang_warning` | `(offenders: &[OffenderRecord], …, writer: W) -> io::Result<()>` | | `write_msvc_warning` | `(offenders: &[OffenderRecord], writer: W) -> io::Result<()>` | @@ -724,8 +723,9 @@ The shared types they consume (`OffenderRecord`, `Severity` (see *Offender / catalog enums*), `TOOL_ID` (the `"big-code-analysis"` tool name), `CSV_HEADER`, and `CSV_EXTENSION` (`".csv"`)) are part of the same surface. The AST dump entry points (`dump_node`, `dump_root`, -`dump_ops`, and the `Dump` / `DumpCfg` config types) are likewise -re-exported and shape-stable. Each terminal dump entry point also has a +and `dump_ops`, which take no config) are likewise re-exported and +shape-stable, as is the `AstCfg` config type that `Ast::dump` +consumes. Each terminal dump entry point also has a `*_with_color` sibling (`dump_node_with_color`, `dump_root_with_color`, `dump_ops_with_color`, `dump_function_spans_with_color`) that takes a `ColorMode` (`Auto` / `Always` / `Never`); the bare forms retain their @@ -801,7 +801,9 @@ canonical URI for the standard is `https://docs.oasis-open.org/sarif/sarif/v2.1.0/cs01/schemas/sarif-schema-2.1.0.json`. The pinned schema URI and version are stable within `2.x`. The `tool.driver.name` is `TOOL_ID` (`"big-code-analysis"`) and -`tool.driver.version` is the CLI package version. +`tool.driver.version` is the `big-code-analysis` library crate version +(currently in lockstep with the CLI's). The writer is also reached from +the Python bindings' `to_sarif`, where no CLI is involved. #### code-climate @@ -977,16 +979,15 @@ list, how the two are combined depends on the *meaning* of the list - **`--no-config` ignores the manifest entirely.** With it set the manifest contributes nothing, so excludes come only from the CLI. -The negative-filter union is a behaviour change from the pre-2.0 +The negative-filter union is a behavior change from the pre-2.0 contract, where CLI excludes *replaced* the manifest list. It landed as part of the 2.0 line. ## Python bindings `big-code-analysis-py` is the PyO3 wrapper published from this -workspace. It is not yet on PyPI; the names below are the contract -the **first** stable publish locks in, because none of them can -change afterward without breaking every consumer's imports. +workspace to PyPI. The names below are locked: none of them can +change without breaking every consumer's imports. - **Distribution name** `big-code-analysis` (the `pip install` target / PyPI project name). @@ -1133,7 +1134,7 @@ contract points are: special-cases the field names per endpoint. This shape-changes the published `Span` library type. - **Unknown fields.** Every request body and query string rejects an - unrecognised field with a `400` whose `error` names the offending key + unrecognized field with a `400` whose `error` names the offending key and whose `error_kind` is `unknown_field` (#633). A typo can no longer silently change analysis semantics; clients probing for feature support use the `GET /v1` route index (#643) instead. This is a @@ -1165,11 +1166,13 @@ contract points are: `invalid_scope_flag`, `vcs_mode_conflict`, `payload_too_large`, `read_error`, `internal_error`, `parse_timeout`, `parse_pool_saturated`, `ast_build_failed`, `metrics_failed`, - `not_found`, `method_not_allowed`, `unsupported_media_type`, and the + `not_found`, `method_not_allowed`, `unsupported_media_type`, + `not_acceptable`, `serialize_failed`, and the per-cause vcs tokens `vcs_not_a_repository`, `vcs_invalid_revision`, `vcs_invalid_bot_pattern`, `vcs_invalid_window`, `vcs_invalid_timestamp`, `vcs_invalid_formula`, `vcs_invalid_file_type_scope`, `vcs_invalid_bus_factor_threshold`, + `vcs_invalid_author_hash_key`, `vcs_invalid_trend`, `vcs_invalid_diff`, and the catch-all `vcs_internal_error`. A `file_name` that maps to no supported language is a `422 Unprocessable Entity` carrying @@ -1179,13 +1182,20 @@ contract points are: per-vcs-cause tokens replace the former single kitchen-sink `/vcs` 400 message at `2.0` (#631): a bad window now answers `vcs_invalid_window` with the specific cause, not a sentence listing - every possible parameter. + every possible parameter. `vcs_invalid_author_hash_key` is new after + `2.1.0`, which does not carry it: a misused `author_hash_key` already + answered `400` there, but reported the catch-all `vcs_internal_error`, + so a client branching on the token saw its own mistake as a server + fault (#1245). Adding a token is additive, and correcting one that + named the wrong cause is a bug fix rather than a rename — but a client + that has to interoperate with both must treat `vcs_internal_error` on + a `400` as the older spelling of this case. - **`/vcs`-family defaults.** The web defaults match the CLI's bounded defaults so the same logical invocation returns the same-sized result on either surface (#636): `top` defaults to 50, `top_deltas` to 10, and `points` to 12 (formerly hard-required). An explicit `top: 0` / `top_deltas: 0` still returns all (the #602 `0 = all` escape). - Aligning these from the former unbounded "all" default is a behavioural + Aligning these from the former unbounded "all" default is a behavioral `2.0`-line break: payloads omitting these fields get a smaller result. - **Introspection.** `GET /v1/version` reports the server and library versions; `GET /v1/languages` reports the supported diff --git a/big-code-analysis-book/src/commands/rest.md b/big-code-analysis-book/src/commands/rest.md index 616b6ab87..9ee3da73f 100644 --- a/big-code-analysis-book/src/commands/rest.md +++ b/big-code-analysis-book/src/commands/rest.md @@ -637,7 +637,8 @@ id}` body (its `error_kind` tokens are the `vcs_*` family — e.g. `vcs_not_a_repository`, `vcs_invalid_window`). A client mistake — `repo_path` does not exist or is not a git working tree (both carry `vcs_not_a_repository`), an unresolvable -`ref`/`commit`, a malformed or non-diff `diff`, or a malformed window / +`ref`/`commit`, a malformed or non-diff `diff`, an unusable +`author_hash_key` (`vcs_invalid_author_hash_key`), or a malformed window / timestamp / formula / file-type / threshold / trend parameter — is a `400`; a failure of the history walk itself is a `500`. A nonexistent `repo_path` is a typo — the most common client error here — so it answers diff --git a/big-code-analysis-book/src/python/sarif.md b/big-code-analysis-book/src/python/sarif.md index df9f94e93..40b68a6c8 100644 --- a/big-code-analysis-book/src/python/sarif.md +++ b/big-code-analysis-book/src/python/sarif.md @@ -3,12 +3,17 @@ `bca.to_sarif(result, *, thresholds=None)` renders an analysis result (or an iterable of them) into a [SARIF 2.1.0](https://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html) -JSON document, ready for upload to GitHub Code Scanning or any +JSON document, ready for upload to [GitHub Code +Scanning](https://docs.github.com/en/code-security/code-scanning) or any other SARIF consumer. The output is produced by the same Rust writer that backs `bca check --report-format sarif`, so the schema URL, tool driver name / version, and rule descriptions match the CLI byte-for-byte. +Examples on this page import the package as `bca` +(`import big_code_analysis as bca`). A bare `bca` in a shell command is +the CLI binary. + ```python {{#include ../../../big-code-analysis-py/examples/sarif_output.py:17:33}} ``` @@ -17,10 +22,10 @@ byte-for-byte. * A single `dict` returned by `bca.analyze` or `bca.analyze_source`. -* Any iterable yielding such dicts and / or `bca.AnalysisFailure` +* Any iterable yielding such dicts or `bca.AnalysisFailure` instances (the natural shape of `bca.analyze_batch`'s return value). `AnalysisFailure` entries are skipped silently — they - represent files that could not be analysed, not findings. + represent files that could not be analyzed, not findings. ## Thresholds @@ -41,8 +46,9 @@ run. `thresholds=None` (the default) and `thresholds={}` both produce a well-formed SARIF document with empty `results` and `rules` -arrays. This matches the CLI's posture: there are **no built-in -default thresholds**; every check run supplies its own limits. +arrays. This matches `bca check`, which applies **no implicit +limits**: every run supplies its own, from `--threshold` or a +`bca.toml` (which `bca init` scaffolds with a starting table). ## Upload to GitHub Code Scanning @@ -68,7 +74,7 @@ The upload action is documented under The bindings produce one SARIF run per call; the action handles the upload to the repository's Code Scanning alerts. -## What "Unit" findings mean +## Which spaces produce findings `to_sarif` emits a finding at every space — the file unit, each container, and each leaf function or closure — whose **own** value @@ -80,14 +86,13 @@ The four subtree-aggregate metrics — `cyclomatic`, their per-space `value` field instead, so it reports an interior breach (for example a function whose own complexity breaches even though a nested closure's does not) without being fooled by the larger -aggregate. Before the `value` field existed the binding could read only -the aggregate and so emitted these four only at leaf spaces, missing -genuine interior breaches the CLI reports (#958). +aggregate. Unit findings carry `logicalLocations: [{"fullyQualifiedName": -""}]`. Nameless non-unit spaces (rare parse-failure case) -carry `""` — both matching the CLI's `function_token` -placeholders. +""}]`. Every other space carries its qualified symbol. Within +that symbol, a closure/lambda (the `` name every grammar +emits) and the `None`-name parse-failure case both collapse to +``, matching the CLI's `space_segment`. ## See also diff --git a/big-code-analysis-book/src/recipes/ci.md b/big-code-analysis-book/src/recipes/ci.md index e0a423481..f78407bc4 100644 --- a/big-code-analysis-book/src/recipes/ci.md +++ b/big-code-analysis-book/src/recipes/ci.md @@ -464,9 +464,18 @@ digest from above): * Adoption guide: https://dekobon.github.io/big-code-analysis/recipes/baselines.html ``` -The refresh invocation mirrors the gate's resolved `--paths`, -`--exclude`, `--exclude-from`, `--config`, and `--baseline` so a -first-time reader of a failing CI log can copy-paste it verbatim. +The refresh invocation mirrors every flag the gate ran with that +decides what a baseline records: the walk scope (`--paths`, +`--include`, `--exclude`, `--exclude-from`, `--paths-from`), the +walker tuning (`--language`, `--exclude-tests`, +`--cyclomatic-count-try`, `--no-ignore`, `--no-skip-generated`, +`--no-config`), and the gate's own inputs (`--config`, `--threshold`, +`--tier`, `--no-suppress`, `--check-exclude`, `--check-exclude-from`, +`--baseline-fuzzy-match`, `--baseline`). So a first-time reader of a +failing CI log can copy-paste it verbatim. Flags that change only +reporting or the exit code — `--jobs`, `--report-format`, +`--output`, `--no-fail`, `--exit-codes` — are left off, since none +can move an entry into or out of the file. The artifact URL is derived from `$GITHUB_REPOSITORY` and `$GITHUB_RUN_ID` when both are present (always true in GHA); local runs — where there is no upload to point at — instead suggest diff --git a/big-code-analysis-book/src/recipes/exporting-data.md b/big-code-analysis-book/src/recipes/exporting-data.md index 376f5beb7..7bf4876d3 100644 --- a/big-code-analysis-book/src/recipes/exporting-data.md +++ b/big-code-analysis-book/src/recipes/exporting-data.md @@ -1,9 +1,11 @@ # Exporting metric data The `metrics`, `ops`, and `preproc` subcommands all support structured -output formats meant for machine consumption. Pair them with a JSON -processor like [`jq`](https://jqlang.github.io/jq/) for ad-hoc -analysis, or feed them into a database or dashboard. +output formats meant for machine consumption. This page covers writing +those documents, diffing two runs, discovering the metric catalog at +runtime, and stripping comments for downstream tools. Pair the output +with a JSON processor like [`jq`](https://jqlang.github.io/jq/) for +ad-hoc analysis, or feed it into a database or dashboard. ## Export per-file metrics as JSON @@ -32,6 +34,11 @@ JSON array of the per-file results): bca metrics -p src/ -O json --output /tmp/metrics.json ``` +Its elements are ordered by file path regardless of the order the +parallel workers finished in, so the document is byte-identical between +runs over unchanged sources. `bca ops --output ` orders its +aggregate the same way. + CBOR (`-O cbor`) is the most compact format; it is binary and so requires a destination (`--output` or `--output-dir`). JSON, TOML, and YAML can all be streamed to stdout when no destination is given, which @@ -99,15 +106,15 @@ bca diff --since main /path/to/other-checkout -p src/ ``` `--since` materializes the ref's tree into a temporary directory (via -`git archive`), runs the same metric walk against it, then diffs — the -temp tree is removed automatically, including on error. The same -`-p/--paths`, `-I/--include`, and `-X/--exclude` selection applies to -both sides so they analyze the same file set. Selection paths are -**repo-root-relative** and must be relative: the working-tree side is -anchored at the repository root (matching the whole-tree `git archive` -of the ref), so `bca diff --since` produces the same result from any -subdirectory. An absolute `--paths` is rejected (exit 1) — it cannot -address the extracted ref tree. +`git ls-tree` + `git cat-file`), runs the same metric walk against it, +then diffs — the temp tree is removed automatically, including on +error. The same `-p/--paths`, `-I/--include`, and `-X/--exclude` +selection applies to both sides so they analyze the same file set. +Selection paths are **repo-root-relative** and must be relative: the +working-tree side is anchored at the repository root (matching the +whole-tree materialization of the ref), so `bca diff --since` produces +the same result from any subdirectory. An absolute `--paths` is +rejected (exit 1) — it cannot address the extracted ref tree. Unlike `bca check --since` (best-effort), `bca diff --since` is an explicit request: an unresolvable ref, a missing `git`, or a non-git @@ -117,9 +124,6 @@ positional (the after side); passing two is an error. `bca diff` exits 0 on success — it is informational, not a gate — unless the opt-in `--exit-code` flag is passed, which exits 2 when the filtered diff is non-empty. -It replaces the former `json-minimal-tests` + `split-minimal-tests.py` -chain used to validate that a grammar bump did not regress metrics; the -`utils/check-grammar-crate.py` helper now calls `bca diff` internally. ## Pull a single metric across an entire tree @@ -150,8 +154,9 @@ tab-completion. ## Extract operands and operators (Halstead) `ops` emits the raw operand and operator lists per file, which is the -input to Halstead-style metric calculations beyond what the built-in -report shows: +input to +[Halstead](https://en.wikipedia.org/wiki/Halstead_complexity_measures)-style +metric calculations beyond what the built-in report shows: ```bash bca ops \ diff --git a/big-code-analysis-cli/src/check_format.rs b/big-code-analysis-cli/src/check_format.rs index 9d5c46dc0..92ab21c6a 100644 --- a/big-code-analysis-cli/src/check_format.rs +++ b/big-code-analysis-cli/src/check_format.rs @@ -1245,7 +1245,7 @@ mod tests { let dir = tempfile::tempdir().expect("tempdir"); let path = dir.path().join("step-summary.md"); let pairs = vec![coverage_pair(violation_for("src/a.rs", "f", "cyclomatic"))]; - let remediation = "\n--- next steps ---\n* Detailed reports: bca-reports artifact\n* To refresh baseline: bca --paths . check --write-baseline .bca-baseline.toml\n"; + let remediation = "\n--- next steps ---\n* Detailed reports: bca-reports artifact\n* To refresh baseline: bca check --paths . --write-baseline .bca-baseline.toml\n"; write_step_summary(&path, &pairs, Some(remediation)).expect("write 1"); let after_first = std::fs::read_to_string(&path).expect("read 1"); // 4-backtick fence opens AND closes inside the marker pair. diff --git a/big-code-analysis-cli/src/commands/analyze.rs b/big-code-analysis-cli/src/commands/analyze.rs index f29fb3347..c8db2af77 100644 --- a/big-code-analysis-cli/src/commands/analyze.rs +++ b/big-code-analysis-cli/src/commands/analyze.rs @@ -210,22 +210,36 @@ pub(crate) fn enforce_explicit_unrecognized( /// `resolve_structured_output`. pub(crate) fn write_aggregate( fmt: Option, - items: Vec, + mut items: Vec, out: &Path, pretty: bool, ) { let Some(fmt) = fmt else { return; }; + // Workers finish out of order, so items reach here in completion + // order — different on every run at `--jobs > 1`, and no longer the + // order `walk_directory_seed` sorted the file list into. Restore it + // before serializing so two runs over an unchanged tree write a + // byte-identical document (#1244). This is the crate's standing + // rule for a channel-collected result set — `run_check_walk` sorts + // violations, `collect_marker_rows` sorts exemption rows, #1091 + // sorted the `Ops` vocabularies — so do not drop it as redundant + // with the walk's own sort, which does not survive the workers. + // + // Keyed on the emitted `PathBuf` rather than a string: path + // ordering is total and requires no UTF-8 validity, so a non-UTF-8 + // path cannot reorder under a lossy rendering of itself. + items.sort_by(|a, b| a.emitted_path().cmp(b.emitted_path())); // `metrics` streams `FuncSpace`; `ops` streams `Ops`. The two never // mix in one run (separate subcommands), so the first item's variant // determines the element type for the whole aggregate. - let is_ops = matches!(items.first(), Some(AggregateItem::Ops(_))); + let is_ops = matches!(items.first(), Some(AggregateItem::Ops(..))); let result = if is_ops { let ops: Vec = items .into_iter() .filter_map(|item| match item { - AggregateItem::Ops(o) => Some(*o), + AggregateItem::Ops(o, _) => Some(*o), AggregateItem::Metrics(..) => None, }) .collect(); @@ -244,7 +258,7 @@ pub(crate) fn write_aggregate( .into_iter() .filter_map(|item| match item { AggregateItem::Metrics(space, path) => Some((*space, path)), - AggregateItem::Ops(_) => None, + AggregateItem::Ops(..) => None, }) .collect(); match fmt.dispatch() { diff --git a/big-code-analysis-cli/src/commands/check.rs b/big-code-analysis-cli/src/commands/check.rs index 001e33ba6..860e3d081 100644 --- a/big-code-analysis-cli/src/commands/check.rs +++ b/big-code-analysis-cli/src/commands/check.rs @@ -138,7 +138,7 @@ pub(crate) fn run_check( // there is no baseline to refresh and no artifact worth pointing // at. Suppressed debt is informational and never remediated here. let remediation = if any_violations { - format_remediation_block(&globals_for_remediation, &args) + format_remediation_block(&globals_for_remediation, &args, tier) } else { None }; diff --git a/big-code-analysis-cli/src/commands/check/remediation.rs b/big-code-analysis-cli/src/commands/check/remediation.rs index f01b847e6..0c2b20897 100644 --- a/big-code-analysis-cli/src/commands/check/remediation.rs +++ b/big-code-analysis-cli/src/commands/check/remediation.rs @@ -3,7 +3,11 @@ use super::super::*; use super::*; -pub(crate) fn format_remediation_block(globals: &GlobalOpts, args: &CheckArgs) -> Option { +pub(crate) fn format_remediation_block( + globals: &GlobalOpts, + args: &CheckArgs, + tier: TierSpec, +) -> Option { use std::fmt::Write as _; if args.no_remediation { return None; @@ -13,25 +17,40 @@ pub(crate) fn format_remediation_block(globals: &GlobalOpts, args: &CheckArgs) - let _ = writeln!( out, "* To refresh baseline: {}", - refresh_baseline_command(globals, args) - ); - // The refresh command mirrors path filters (`--paths`, - // `--exclude`, `--exclude-from`, `--config`, `--baseline`) but - // intentionally omits selectors that don't affect baseline - // composition (`--num-jobs`) and ones that would bloat the - // common-case command (`--include`, `--language`, - // `--paths-from`, `--exclude-tests`). Surface the omission so a - // user with a non-trivial scope re-adds them rather than - // assuming the printed command is complete. - out.push_str( - " (mirrors path filters only — re-add any --include / --language / --exclude-tests / --paths-from flags as needed)\n", + refresh_baseline_command(globals, args, tier) ); + // The command now mirrors every flag that changes what + // `--write-baseline` records (#1243), so the enumerated + // "re-add any --include / --language / …" caveat this line used + // to carry is gone — each of those flags is emitted. What no + // printed command can reproduce is a list that came from stdin, + // so that one caveat stays, conditional on it applying. + if any_list_flag_read_stdin(globals, args) { + out.push_str( + " (a `-` list flag read stdin — point it at the file those patterns came from)\n", + ); + } out.push_str( "* Adoption guide: https://dekobon.github.io/big-code-analysis/recipes/baselines.html\n", ); Some(out) } +/// Whether any of the three list-from-file flags the refresh command +/// mirrors was given as `-` (stdin). Every other flag re-runs +/// identically; a stdin list cannot, because the pipe that fed the +/// gate is gone by the time anyone reads the log. +fn any_list_flag_read_stdin(globals: &GlobalOpts, args: &CheckArgs) -> bool { + [ + globals.paths_from.as_deref(), + globals.exclude_from.as_deref(), + args.check_exclude_from.as_deref(), + ] + .into_iter() + .flatten() + .any(|p| p == Path::new("-")) +} + pub(crate) fn artifact_link() -> String { artifact_link_for( std::env::var(check_format::GITHUB_REPOSITORY_ENV).ok(), @@ -63,66 +82,199 @@ pub(crate) fn artifact_link_for(repo: Option, run_id: Option) -> } } -pub(crate) fn refresh_baseline_command(globals: &GlobalOpts, args: &CheckArgs) -> String { - let mut cmd = String::from("bca"); - let paths: Vec<&Path> = if globals.paths.is_empty() { +/// The copy-paste baseline-refresh invocation, rendered for a POSIX +/// shell. Every element of [`refresh_baseline_argv`] is quoted, so a +/// value containing a space, a glob metacharacter, or a quote survives +/// the paste as one argument. +pub(crate) fn refresh_baseline_command( + globals: &GlobalOpts, + args: &CheckArgs, + tier: TierSpec, +) -> String { + refresh_baseline_argv(globals, args, tier) + .iter() + .map(|arg| shell_quote(arg)) + .collect::>() + .join(" ") +} + +/// The baseline-refresh invocation as argv, `bca` at index 0 and the +/// `check` subcommand at index 1. +/// +/// **The subcommand comes before the flags.** `--paths` / `--exclude` / +/// `--exclude-from` have been subcommand-scoped since #597; the +/// pre-#597 `bca check` shape this used to emit is a clap +/// usage error, so every failing gate printed a next-steps command that +/// exited 1 when run (#1243). +/// +/// Built as argv rather than as a string so the flag list and its shell +/// rendering are separable: the tests feed this straight to +/// `Cli::try_parse_from` and compare the parsed flags against the run +/// that produced them. A string-only builder can only be *string*- +/// matched, which is how the broken order survived eight assertions. +/// +/// # What is mirrored, and why +/// +/// The criterion is baseline *composition*: a flag that changes which +/// violations `--write-baseline` records must be echoed, or the +/// suggested refresh writes a different baseline than the gate +/// measured. That covers the walk scope (which files), the walker +/// tuning (what the metrics come out as), and the gate's own inputs +/// (which offenders survive to the writer) — `apply_check_exclude` +/// runs before `write_check_baseline` (#378), so `--check-exclude` +/// belongs here as much as `--exclude` does, and a `--threshold` +/// override decides the offender set outright. +/// +/// Deliberately omitted: `--jobs` (throughput only); the reporting and +/// exit-code flags (`--report-format`, `--output`, `--no-fail`, +/// `--exit-codes`, `--summary-file`, `--github-annotations`, +/// `--no-summary`, `--no-remediation`), none of which reach the +/// writer; `--baseline-line-tolerance`, which is read-side matching; +/// and `--since` / `--changed-only`, which clap forbids alongside +/// `--write-baseline` on purpose — a diff-scoped baseline would look +/// like a complete snapshot to the next run. +/// +/// Manifest-supplied settings need no echoing: the refresh runs from +/// the same directory and rediscovers the same `bca.toml`. The one +/// exception is `--no-config`, which is mirrored precisely so the +/// refresh keeps ignoring it. +pub(crate) fn refresh_baseline_argv( + globals: &GlobalOpts, + args: &CheckArgs, + tier: TierSpec, +) -> Vec { + let mut argv = vec!["bca".to_owned(), "check".to_owned()]; + push_walk_args(&mut argv, globals); + push_gate_args(&mut argv, args, tier); + argv +} + +/// Mirror the walk: which files are visited and how their metrics are +/// computed. Sourced from [`GlobalOpts`], which by this point carries +/// the CLI flags with any `bca.toml` values already merged in. +fn push_walk_args(argv: &mut Vec, globals: &GlobalOpts) { + if globals.paths.is_empty() { // Default-when-absent — mirror the walker's `expand_seed_paths` // `.` fallback (#596) so the printed refresh command behaves // identically to the pathless invocation that produced it. - vec![Path::new(".")] - } else { - globals.paths.iter().map(PathBuf::as_path).collect() - }; - for p in &paths { - cmd.push_str(" --paths "); - cmd.push_str(&shell_quote_path(p)); - } - for ex in &globals.exclude { - cmd.push_str(" --exclude "); - cmd.push_str(&shell_quote(ex)); + push_pair(argv, "--paths", "."); } - if let Some(p) = &globals.exclude_from { - cmd.push_str(" --exclude-from "); - cmd.push_str(&shell_quote_path(p)); + for p in &globals.paths { + push_pair(argv, "--paths", &path_arg(p)); } - cmd.push_str(" check"); - if let Some(p) = &args.config { - cmd.push_str(" --config "); - cmd.push_str(&shell_quote_path(p)); + push_repeated(argv, "--include", &globals.include); + push_repeated(argv, "--exclude", &globals.exclude); + push_path(argv, "--paths-from", globals.paths_from.as_deref()); + push_path(argv, "--exclude-from", globals.exclude_from.as_deref()); + push_value(argv, "--language", globals.language.as_deref()); + push_path(argv, "--preproc-data", globals.preproc_data.as_deref()); + push_switch(argv, "--no-skip-generated", globals.no_skip_generated); + push_switch(argv, "--no-ignore", globals.no_ignore); + push_switch(argv, "--no-config", globals.no_config); + push_switch(argv, "--exclude-tests", globals.exclude_tests); + push_equals(argv, "--cyclomatic-count-try", globals.count_cyclomatic_try); +} + +/// Mirror the gate: which of the walk's violations reach the baseline +/// writer, and where the file lands. `tier` is the *resolved* tier — +/// `args.tier` alone loses a deprecated `--headroom `, and +/// re-resolving here would emit that flag's deprecation warning a +/// second time. +fn push_gate_args(argv: &mut Vec, args: &CheckArgs, tier: TierSpec) { + push_path(argv, "--config", args.config.as_deref()); + for (metric, limit) in &args.thresholds { + push_pair(argv, "--threshold", &format!("{metric}={limit}")); } + push_equals(argv, "--tier", tier_value(tier)); + push_switch(argv, "--no-suppress", args.no_suppress); + push_repeated(argv, "--check-exclude", &args.check_exclude); + push_path( + argv, + "--check-exclude-from", + args.check_exclude_from.as_deref(), + ); + // Not just a read-side matcher: a fuzzy write populates each + // entry's `body_hash`, so the flag changes the file's content. + push_equals(argv, "--baseline-fuzzy-match", args.baseline_fuzzy_match); // `--baseline` and `--write-baseline` conflict in clap, so we // prefer the user's baseline path if they ran with it (the // refresh writes back to the same file). Fall back to the // documented default `.bca-baseline.toml`. - cmd.push_str(" --write-baseline "); - match args.baseline.as_deref() { - Some(p) => cmd.push_str(&shell_quote_path(p)), - None => cmd.push_str(&shell_quote(".bca-baseline.toml")), + let target = args + .baseline + .as_deref() + .map_or_else(|| DEFAULT_BASELINE_FILE.to_owned(), path_arg); + push_pair(argv, "--write-baseline", &target); +} + +/// The `--tier` value for the resolved tier, or `None` at the `hard` +/// default, which needs no flag at all. +fn tier_value(tier: TierSpec) -> Option { + match tier { + TierSpec::Hard => None, + TierSpec::Soft(None) => Some("soft".to_owned()), + TierSpec::Soft(Some(ratio)) => Some(format!("soft={ratio}")), } - cmd -} - -fn shell_quote_path(p: &Path) -> String { - // The printed command is an *identifier* in the user's shell — - // running it must reach the same file `bca` walked. Non-UTF-8 - // paths cannot be expressed as a shell argument verbatim, so - // surface them as a clearly-broken placeholder rather than emit - // a `to_string_lossy` form that silently points at the wrong - // file (AGENTS.md: identifier paths use `to_str()` with explicit - // non-UTF-8 handling, not `path.display()`). - // - // The placeholder contains `<`, `>` and spaces, which force - // `shell_quote`'s slow path → single-quoted literal. Combining - // the to_str + quote here (instead of leaving callers to chain - // them) makes the discipline structural: a future caller can't - // accidentally `eprintln!("{}", path_for_shell(p))` and emit an - // unquoted `` that bash would parse as input - // redirection. - let raw = p.to_str().map_or_else( +} + +fn push_pair(argv: &mut Vec, flag: &str, value: &str) { + argv.push(flag.to_owned()); + argv.push(value.to_owned()); +} + +fn push_repeated(argv: &mut Vec, flag: &str, values: &[String]) { + for value in values { + push_pair(argv, flag, value); + } +} + +fn push_value(argv: &mut Vec, flag: &str, value: Option<&str>) { + if let Some(value) = value { + push_pair(argv, flag, value); + } +} + +fn push_path(argv: &mut Vec, flag: &str, value: Option<&Path>) { + if let Some(value) = value { + push_pair(argv, flag, &path_arg(value)); + } +} + +fn push_switch(argv: &mut Vec, flag: &str, enabled: bool) { + if enabled { + argv.push(flag.to_owned()); + } +} + +/// Push a flag whose clap definition sets `require_equals`, so name and +/// value must arrive as one argv element (`--tier=soft=0.9`). Splitting +/// them across two elements is a usage error, which is exactly the +/// class of mistake the argv round-trip test exists to catch. +fn push_equals(argv: &mut Vec, flag: &str, value: Option) { + if let Some(value) = value { + argv.push(format!("{flag}={value}")); + } +} + +/// A path as one argv element. The refresh command is an *identifier* +/// in the user's shell — running it must reach the same file `bca` +/// walked. Non-UTF-8 paths cannot be expressed as a shell argument +/// verbatim, so surface them as a clearly-broken placeholder rather +/// than emit a `to_string_lossy` form that silently points at the +/// wrong file (AGENTS.md: identifier paths use `to_str()` with +/// explicit non-UTF-8 handling, not `path.display()`). +/// +/// The placeholder contains `<`, `>` and spaces, which force +/// [`shell_quote`]'s slow path → single-quoted literal. Nothing here +/// quotes: every element of the argv is rendered through +/// `shell_quote` by [`refresh_baseline_command`], so an unquoted +/// `` that a shell would read as input redirection +/// is unreachable by construction. +fn path_arg(p: &Path) -> String { + p.to_str().map_or_else( || format!("", p.display()), str::to_string, - ); - shell_quote(&raw) + ) } /// Shell-quote `s` for inclusion in the remediation block's diff --git a/big-code-analysis-cli/src/commands/diff_cmd.rs b/big-code-analysis-cli/src/commands/diff_cmd.rs index f667fd423..e7a4c0544 100644 --- a/big-code-analysis-cli/src/commands/diff_cmd.rs +++ b/big-code-analysis-cli/src/commands/diff_cmd.rs @@ -119,9 +119,9 @@ pub(crate) fn compute_since_diff( // creating any temp state, so nothing needs cleaning up on this path. diff::validate_since_ref(since_ref).unwrap_or_else(|reason| die(reason)); - // Both sides are rooted at their own tree top (a `git archive` - // extraction of `` for the before side, the repo root for the - // after side) and pair on root-relative keys. Selection — `--paths` + // Both sides are rooted at their own tree top (the materialized + // `` tree for the before side, the repo root for the after + // side) and pair on root-relative keys. Selection — `--paths` // and the optional positional scope — must therefore be *relative*: // an absolute path addresses the live filesystem, not the extracted // tree, so it would walk the current tree for both sides and @@ -143,11 +143,11 @@ pub(crate) fn compute_since_diff( // out of the `/tmp/…` extraction of `` (reaching unrelated host // files or nothing), while on the after side it climbs out of the // repo root — so the two sides walk different trees and the diff - // silently mis-pairs. `git archive` extractions never contain `..` - // path components, so any `..` in a selector cannot address real - // tracked files on the before side regardless. Reject it with the - // same clear message rather than produce a bogus all-zero / partial - // diff. + // silently mis-pairs. `materialize_tree` routes every path it + // materializes through `bytes_to_rel_path`, which rejects `..` and + // absolute components, so no `..` selector addresses a real tracked + // file on the before side regardless. Reject it with the same clear + // message rather than produce a bogus all-zero / partial diff. let escaping_selector = selectors().find(|p| escapes_root(p)); if let Some(esc) = escaping_selector { die(format_args!( @@ -184,8 +184,8 @@ pub(crate) fn compute_since_diff( // After side: always the working tree, rooted at the *git repo root* // (not the process CWD) so its root-relative keys line up with the - // before side — a `git archive` of the whole ref tree, always rooted - // at the repo top. This lets `bca diff --since` run from any + // before side — a materialization of the whole ref tree, always + // rooted at the repo top. This lets `bca diff --since` run from any // subdirectory. The optional positional is a relative *scope* folded // into both sides by `side_globals` (#497), never an alternate root, // so a subtree positional (`bca diff --since HEAD src`) selects the @@ -232,7 +232,7 @@ pub(crate) fn side_globals(globals: &GlobalOpts, scope: Option<&Path>) -> Global // The `--since` positional is a relative path *scope*, not a tree // root: merge it into `--paths` so both sides walk the same subtree // and pair on the same keys (#497). Both sides are rooted at their - // own tree top (the `git archive` extraction / the repo root), so a + // own tree top (the materialized `` tree / the repo root), so a // scope of `src` selects `src/…` on each — never re-roots one side. if let Some(scope) = scope { side.paths.push(scope.to_path_buf()); diff --git a/big-code-analysis-cli/src/commands_tests.rs b/big-code-analysis-cli/src/commands_tests.rs index 9860d50ce..cb276224d 100644 --- a/big-code-analysis-cli/src/commands_tests.rs +++ b/big-code-analysis-cli/src/commands_tests.rs @@ -386,18 +386,34 @@ fn globals_for_remediation( } } +/// Parse a rendered refresh argv back through the real clap surface, +/// returning the `check` args it produced. Any usage error — a flag in +/// a position clap no longer accepts, a value spelling the parser +/// rejects — fails here rather than being string-matched away (#1243). +fn parse_refresh_argv(argv: &[String]) -> CheckArgs { + use clap::Parser; + let parsed = crate::Cli::try_parse_from(argv).unwrap_or_else(|e| { + panic!("emitted refresh command must parse; got:\n{e}\nargv: {argv:?}") + }); + match parsed.command { + crate::Command::Check(args) => *args, + other => panic!("expected Command::Check, got {other:?}"), + } +} + #[test] fn format_remediation_block_returns_none_when_suppressed() { let globals = globals_for_remediation(&["."], &[], None); let args = check_args_for_remediation(None, None, true); - assert!(format_remediation_block(&globals, &args).is_none()); + assert!(format_remediation_block(&globals, &args, crate::TierSpec::Hard).is_none()); } #[test] fn format_remediation_block_contains_three_bullet_points() { let globals = globals_for_remediation(&["."], &[], None); let args = check_args_for_remediation(Some("bca-thresholds.toml"), None, false); - let out = format_remediation_block(&globals, &args).expect("remediation present"); + let out = format_remediation_block(&globals, &args, crate::TierSpec::Hard) + .expect("remediation present"); assert!(out.contains("--- next steps ---")); assert!( out.contains("* Detailed reports:"), @@ -416,11 +432,62 @@ fn format_remediation_block_contains_three_bullet_points() { } #[test] -fn refresh_baseline_command_mirrors_resolved_args() { - // The copy-paste invocation must reproduce the gate's --paths / - // --exclude / --exclude-from / --config so the user can run it - // verbatim. Hard-coding `--paths .` would be wrong for repos - // that scope scans differently. +fn remediation_block_prints_the_command_the_builder_rendered() { + // The block and the builder were each tested, but nothing pinned + // the seam between them: every other test in this section calls + // `refresh_baseline_argv` / `refresh_baseline_command` directly. + // Hard-coding `TierSpec::Hard` at the block's call site — the + // exact mistake threading the resolved tier through exists to + // prevent — left the whole crate suite green. + let globals = GlobalOpts { + paths: vec![PathBuf::from("src")], + exclude_tests: true, + ..GlobalOpts::default() + }; + let args = CheckArgs { + no_suppress: true, + ..base_check_args() + }; + let tier = crate::TierSpec::Soft(Some(0.9)); + let out = format_remediation_block(&globals, &args, tier).expect("remediation present"); + let refresh: Vec<&str> = out + .lines() + .filter(|l| l.starts_with("* To refresh baseline: ")) + .collect(); + assert_eq!(refresh.len(), 1, "expected one refresh bullet, got:\n{out}"); + assert_eq!( + refresh[0], + format!( + "* To refresh baseline: {}", + refresh_baseline_command(&globals, &args, tier) + ), + "the block must print the builder's own rendering, got:\n{out}" + ); + // The three arguments the block forwards, each with a value the + // default cannot produce, so dropping any one of them shows up + // here rather than in a builder-level test that never runs + // through `format_remediation_block`. + for expected in [ + "--paths src", + "--exclude-tests", + "--no-suppress", + "--tier=soft=0.9", + ] { + assert!( + refresh[0].contains(expected), + "missing {expected} in the printed command: {}", + refresh[0] + ); + } +} + +#[test] +fn refresh_baseline_command_names_the_subcommand_before_its_flags() { + // The walk flags are subcommand-scoped (#597), so `check` must + // come first. This is the *shape* half of the #1243 guard; the + // half that would have caught the regression is + // `refresh_baseline_argv_round_trips_through_the_parser` below, + // which runs the string past the parser instead of past `contains`. let globals = globals_for_remediation(&["src", "tests"], &["target", "vendor"], Some(".bcaignore")); let args = check_args_for_remediation( @@ -428,73 +495,300 @@ fn refresh_baseline_command_mirrors_resolved_args() { Some(".bca-baseline.toml"), false, ); - let cmd = refresh_baseline_command(&globals, &args); + let cmd = refresh_baseline_command(&globals, &args, crate::TierSpec::Hard); assert!( - cmd.starts_with("bca "), - "must start with `bca `, got: {cmd}" + cmd.starts_with("bca check --"), + "walk flags must follow the subcommand, got: {cmd}" ); - assert!( - cmd.contains("--paths src"), - "missing --paths src, got: {cmd}" +} + +#[test] +fn refresh_baseline_argv_round_trips_through_the_parser() { + // The regression guard for #1243. Every flag the gate ran with + // must come back out of `Cli::try_parse_from` carrying the value + // it went in with — not merely appear somewhere in the string. + // + // Fixture values are deliberately asymmetric (two paths but one + // include, distinct globs per flag, two thresholds with different + // numeric shapes) so a flag emitted with the wrong name or the + // wrong value lands on its own failure rather than on a value + // that happens to match. The four boolean switches are all `true` + // here, which cannot tell them apart — + // `refresh_baseline_argv_pairs_each_switch_with_its_own_flag` + // covers that wiring. + let globals = GlobalOpts { + paths: vec![PathBuf::from("src"), PathBuf::from("tests")], + include: vec!["*.rs".to_string()], + exclude: vec!["target/**".to_string(), "vendor/**".to_string()], + language: Some("rust".to_string()), + no_skip_generated: true, + preproc_data: Some(PathBuf::from("preproc.json")), + paths_from: Some(PathBuf::from("paths.txt")), + exclude_from: Some(PathBuf::from(".bcaignore")), + no_ignore: true, + exclude_tests: true, + count_cyclomatic_try: Some(false), + no_config: true, + ..GlobalOpts::default() + }; + let args = CheckArgs { + thresholds: vec![ + ("cyclomatic".to_string(), 15.0), + ("halstead.effort".to_string(), 12.5), + ], + config: Some(PathBuf::from("bca-thresholds.toml")), + no_suppress: true, + baseline: Some(PathBuf::from("custom-baseline.toml")), + // `false`, not `true`. `--baseline-fuzzy-match` declares + // `default_missing_value = "true"`, so a `true` fixture also + // passes when the flag is emitted as *two* argv elements: + // clap reads the bare flag as `true` and swallows the value + // as a stray positional path. Only `false` can distinguish + // that from the `require_equals` spelling `push_equals` + // produces. + baseline_fuzzy_match: Some(false), + check_exclude: vec!["fixtures/**".to_string()], + check_exclude_from: Some(PathBuf::from(".bcacheckignore")), + ..base_check_args() + }; + let tier = crate::TierSpec::Soft(Some(0.9)); + + let argv = refresh_baseline_argv(&globals, &args, tier); + let back = parse_refresh_argv(&argv); + + assert_eq!( + back.selection.paths, + vec![PathBuf::from("src"), PathBuf::from("tests")], ); - assert!( - cmd.contains("--paths tests"), - "missing --paths tests, got: {cmd}" + assert_eq!(back.selection.include, vec!["*.rs".to_string()]); + assert_eq!( + back.selection.exclude, + vec!["target/**".to_string(), "vendor/**".to_string()], ); - assert!( - cmd.contains("--exclude target"), - "missing --exclude target, got: {cmd}" + assert_eq!(back.selection.language.as_deref(), Some("rust")); + assert!(back.selection.no_skip_generated); + assert_eq!( + back.selection.paths_from.as_deref(), + Some(Path::new("paths.txt")), ); - assert!( - cmd.contains("--exclude vendor"), - "missing --exclude vendor, got: {cmd}" + assert_eq!( + back.selection.exclude_from.as_deref(), + Some(Path::new(".bcaignore")), ); - assert!( - cmd.contains("--exclude-from .bcaignore"), - "missing --exclude-from, got: {cmd}" + assert!(back.selection.no_ignore); + assert!(back.selection.no_config); + assert_eq!( + back.preproc.preproc_data.as_deref(), + Some(Path::new("preproc.json")), ); - assert!( - cmd.contains("check"), - "missing `check` subcommand, got: {cmd}" + assert!(back.tuning.exclude_tests); + assert_eq!(back.tuning.cyclomatic_count_try, Some(false)); + assert_eq!(back.thresholds, args.thresholds); + assert_eq!( + back.config.as_deref(), + Some(Path::new("bca-thresholds.toml")), ); - assert!( - cmd.contains("--config bca-thresholds.toml"), - "missing --config, got: {cmd}" + assert_eq!(back.tier, tier); + assert!(back.no_suppress); + assert_eq!(back.baseline_fuzzy_match, Some(false)); + // The refresh *writes* the baseline the gate read, so `--baseline` + // becomes `--write-baseline` (the two conflict in clap). + assert_eq!( + back.write_baseline, + Some(Some(PathBuf::from("custom-baseline.toml"))), ); + assert_eq!(back.baseline, None); + // Nothing may land in the trailing positional `[PATHS]`. A + // `require_equals` flag emitted as two elements parses cleanly + // and dumps its value here, which every assertion above would + // otherwise miss. assert!( - cmd.contains("--write-baseline .bca-baseline.toml"), - "missing --write-baseline, got: {cmd}" + back.positional.positional_paths.is_empty(), + "stray positional argument in {argv:?}" + ); +} + +#[test] +fn refresh_baseline_argv_mirrors_the_gate_only_excludes() { + // `apply_check_exclude` runs before `write_check_baseline` (#378), + // so a refresh that drops `--check-exclude` records exactly the + // violations the gate deliberately exempted. Asserted apart from + // the full round-trip so losing this mirroring fails a test that + // names it. + let globals = globals_for_remediation(&["."], &[], None); + let args = CheckArgs { + check_exclude: vec!["fixtures/**".to_string(), "generated/**".to_string()], + check_exclude_from: Some(PathBuf::from(".bcacheckignore")), + ..base_check_args() + }; + let back = parse_refresh_argv(&refresh_baseline_argv( + &globals, + &args, + crate::TierSpec::Hard, + )); + assert_eq!(back.check_exclude, args.check_exclude); + assert_eq!( + back.check_exclude_from.as_deref(), + Some(Path::new(".bcacheckignore")), ); } #[test] -fn refresh_baseline_command_defaults_paths_when_unset() { +fn refresh_baseline_argv_defaults_paths_and_baseline_when_unset() { // `--paths .` is the walker's implicit default; the remediation // block must print it explicitly so the user can copy-paste // without thinking about which directory `bca` would have walked - // by default. + // by default. With no `--baseline`, the write target is the + // documented default file. let globals = globals_for_remediation(&[], &[], None); let args = check_args_for_remediation(None, None, false); - let cmd = refresh_baseline_command(&globals, &args); - assert!( - cmd.contains("--paths ."), - "missing default --paths, got: {cmd}" + let argv = refresh_baseline_argv(&globals, &args, crate::TierSpec::Hard); + let back = parse_refresh_argv(&argv); + assert_eq!(back.selection.paths, vec![PathBuf::from(".")]); + assert_eq!( + back.write_baseline, + Some(Some(PathBuf::from(".bca-baseline.toml"))), ); - // And the default baseline target is `.bca-baseline.toml`. - assert!( - cmd.contains("--write-baseline .bca-baseline.toml"), - "missing default baseline path, got: {cmd}" + // Flags the gate never set stay off the command rather than being + // emitted at their defaults. This has to be asserted on the argv: + // clap re-materialises every default on the way back, so + // `back.tier == Hard` holds whether or not a redundant + // `--tier=hard` was printed. + // + // Asserted as the *whole* argv, not as a deny-list of names. A + // deny-list only guards the flags someone thought to list — with + // four of the sixteen conditionally-emitted flags named, pinning + // `--cyclomatic-count-try=false` and `--baseline-fuzzy-match=true` + // onto a run that set neither passed. Any flag emitted + // unconditionally now fails here, including one added later. + let rendered: Vec<&str> = argv.iter().map(String::as_str).collect(); + assert_eq!( + rendered, + vec![ + "bca", + "check", + "--paths", + ".", + "--write-baseline", + ".bca-baseline.toml", + ], ); } #[test] -fn refresh_baseline_command_shell_quotes_paths_with_spaces() { - // A `--paths` arg containing a space must be quoted so the - // copy-paste command shells correctly. The simple identifier - // path takes the fast no-quote branch. - let globals = globals_for_remediation(&["dir with space", "src"], &[], None); +fn refresh_baseline_argv_renders_every_tier_spelling() { + // `--tier` is the one mirrored flag whose argv form is not a + // straight copy of a field: the hard default prints nothing, a + // bare soft tier prints `soft`, and a pinned ratio prints + // `soft=`. A refresh that drops the tier silently writes a + // hard-tier baseline for a soft-tier gate; one that mis-spells it + // fails to parse, which is #1243 again. + let globals = globals_for_remediation(&["."], &[], None); + let args = check_args_for_remediation(None, None, false); + let cases = [ + (crate::TierSpec::Hard, None), + (crate::TierSpec::Soft(None), Some("--tier=soft")), + (crate::TierSpec::Soft(Some(0.9)), Some("--tier=soft=0.9")), + ]; + for (tier, expected) in cases { + let argv = refresh_baseline_argv(&globals, &args, tier); + let emitted = argv.iter().find(|a| a.starts_with("--tier")); + assert_eq!( + emitted.map(String::as_str), + expected, + "wrong --tier element for {tier:?}, got: {argv:?}" + ); + assert_eq!( + parse_refresh_argv(&argv).tier, + tier, + "{tier:?} did not survive the parser" + ); + } +} + +#[test] +fn refresh_baseline_argv_pairs_each_switch_with_its_own_flag() { + // Four presence-only walk switches emit four distinct flags. A + // fixture that sets them all alike cannot tell the pairings + // apart: transposing two `push_switch` value arguments produces + // byte-identical argv. Turning exactly one on at a time makes any + // transposition fail on both of the flags involved. + let base = globals_for_remediation(&["."], &[], None); + let args = check_args_for_remediation(None, None, false); + let cases = [ + ( + "--no-skip-generated", + GlobalOpts { + no_skip_generated: true, + ..base.clone() + }, + ), + ( + "--no-ignore", + GlobalOpts { + no_ignore: true, + ..base.clone() + }, + ), + ( + "--no-config", + GlobalOpts { + no_config: true, + ..base.clone() + }, + ), + ( + "--exclude-tests", + GlobalOpts { + exclude_tests: true, + ..base + }, + ), + ]; + let every_switch: Vec<&str> = cases.iter().map(|(flag, _)| *flag).collect(); + for (flag, globals) in &cases { + let argv = refresh_baseline_argv(globals, &args, crate::TierSpec::Hard); + let emitted: Vec<&str> = argv + .iter() + .map(String::as_str) + .filter(|a| every_switch.contains(a)) + .collect(); + assert_eq!( + emitted, + vec![*flag], + "setting only {flag} must emit only {flag}, got: {argv:?}" + ); + } +} + +#[test] +fn refresh_baseline_command_shell_quotes_values_needing_it() { + // A value containing a space, a glob metacharacter, or a quote + // must survive the paste as one argument; a plain identifier + // takes the fast no-quote branch so the common case reads + // naturally. + // + // Each `--include` value carries exactly one metacharacter, alone. + // Every other fixture here pairs its metacharacter with a space or + // a `*` that forces the slow path by itself, so widening the + // fast-path allow-list to admit `$`, `~` or `&` changed no test — + // a value must isolate the character to test it. The empty string + // is the same gap in the other direction: it takes the fast path + // only because of an explicit `!s.is_empty()` guard, and unquoted + // it vanishes from the command entirely. + let globals = GlobalOpts { + paths: vec![PathBuf::from("dir with space"), PathBuf::from("src")], + exclude: vec!["sub/**".to_string(), "it's/**".to_string()], + include: vec![ + "$HOME".to_string(), + "~".to_string(), + "a&b".to_string(), + String::new(), + ], + ..GlobalOpts::default() + }; let args = check_args_for_remediation(None, None, false); - let cmd = refresh_baseline_command(&globals, &args); + let cmd = refresh_baseline_command(&globals, &args, crate::TierSpec::Hard); assert!( cmd.contains("--paths 'dir with space'"), "expected single-quoted spaced path, got: {cmd}" @@ -503,6 +797,162 @@ fn refresh_baseline_command_shell_quotes_paths_with_spaces() { cmd.contains("--paths src"), "expected unquoted simple path, got: {cmd}" ); + assert!( + cmd.contains("--exclude 'sub/**'"), + "a glob must be quoted so the shell does not expand it, got: {cmd}" + ); + assert!( + cmd.contains(r"--exclude 'it'\''s/**'"), + "an embedded quote must be escaped POSIX-style, got: {cmd}" + ); + for expected in [ + "--include '$HOME'", + "--include '~'", + "--include 'a&b'", + "--include ''", + ] { + assert!(cmd.contains(expected), "expected {expected}, got: {cmd}"); + } +} + +/// A path `bca` walked but cannot spell as UTF-8 becomes a visibly +/// broken placeholder rather than a `to_string_lossy` form that points +/// at some other file. The placeholder contains `<` and spaces, so it +/// must reach the reader *quoted* — unquoted, a shell parses it as +/// input redirection and the command fails in a way that reads as a +/// `bca` bug. Unix-only: `OsStr` is byte-addressable only there. +#[cfg(unix)] +#[test] +fn refresh_baseline_command_quotes_the_non_utf8_path_placeholder() { + use std::os::unix::ffi::OsStrExt as _; + let globals = GlobalOpts { + paths: vec![PathBuf::from(std::ffi::OsStr::from_bytes(b"lib\xffdir"))], + ..GlobalOpts::default() + }; + let args = check_args_for_remediation(None, None, false); + let argv = refresh_baseline_argv(&globals, &args, crate::TierSpec::Hard); + assert!( + argv.iter().any(|a| a.starts_with(" = String::from_utf8(out.stdout) + .expect("utf8") + .lines() + .map(str::to_string) + .collect(); + assert_eq!( + argv, + refresh_baseline_argv(&globals, &args, crate::TierSpec::Hard), + "the shell must recover exactly the argv the builder produced", + ); + + let back = parse_refresh_argv(&argv); + assert_eq!(back.selection.paths, vec![PathBuf::from("dir with space")]); + assert_eq!( + back.selection.exclude, + vec!["sub/**".to_string(), "it's/**".to_string()], + ); + assert_eq!( + back.selection.include, + vec![ + "$(echo pwned)".to_string(), + "$HOME".to_string(), + String::new(), + ], + ); +} + +#[test] +fn remediation_block_flags_a_stdin_list_it_cannot_replay() { + // A `-` list flag consumed a pipe that is gone by the time anyone + // reads the log, so the printed command needs a caveat no other + // flag does. All three sources are covered: dropping any one of + // them from `any_list_flag_read_stdin` leaves that flag's users + // with an unreplayable command and no warning. + let caveat = "read stdin"; + let args = check_args_for_remediation(None, None, false); + let base = globals_for_remediation(&["."], &[], Some(".bcaignore")); + + let mut from_paths = base.clone(); + from_paths.paths_from = Some(PathBuf::from("-")); + let mut from_exclude = base.clone(); + from_exclude.exclude_from = Some(PathBuf::from("-")); + let piped_check_exclude = CheckArgs { + check_exclude_from: Some(PathBuf::from("-")), + ..check_args_for_remediation(None, None, false) + }; + + for (label, globals, args) in [ + ("--paths-from", &from_paths, &args), + ("--exclude-from", &from_exclude, &args), + ("--check-exclude-from", &base, &piped_check_exclude), + ] { + let out = format_remediation_block(globals, args, crate::TierSpec::Hard) + .expect("remediation present"); + assert!( + out.contains(caveat), + "{label} - must be called out, got:\n{out}" + ); + } + + // The same fixture with every list flag file-backed must not warn. + let quiet = + format_remediation_block(&base, &args, crate::TierSpec::Hard).expect("remediation present"); + assert!( + !quiet.contains(caveat), + "a file-backed list replays fine and needs no caveat, got:\n{quiet}" + ); } #[test] @@ -1237,3 +1687,187 @@ fn into_preproc_data_degrades_on_shared_arc() { "shared-Arc recovery must surface the accumulated file entries" ); } + +// --------------------------------------------------------------------- +// Aggregate `--output` element ordering (#1244) +// --------------------------------------------------------------------- + +/// A trivial parsed Rust document carrying `name`. +/// +/// The tests below hand the same fixture a *path* that disagrees with +/// its `name`, so an assertion on the emitted order can only hold if +/// `write_aggregate` keys on the emitted path — sorting by `name`, or +/// not sorting at all, produces a different sequence in every case. +fn aggregate_ast(name: &str) -> big_code_analysis::Ast { + use big_code_analysis::{Ast, LANG, Source}; + + Ast::parse(Source::new(LANG::Rust, b"fn f() { let x = 1; }\n").with_name(Some(name.to_owned()))) + .expect("fixture parses") +} + +/// The `metrics` element of an [`aggregate_ast`] document. +fn aggregate_space(name: &str) -> FuncSpace { + aggregate_ast(name) + .metrics(big_code_analysis::MetricsOptions::default()) + .expect("fixture yields metrics") +} + +/// The `ops` element of an [`aggregate_ast`] document. +fn aggregate_ops(name: &str) -> Ops { + aggregate_ast(name).ops().expect("fixture yields ops") +} + +/// `(emitted path, document name)` in a deliberately scrambled source +/// order. The path order (`alpha`, `beta`, `middle`, `zebra`) is not +/// the source order, not the name order, and — because `beta` and +/// `middle` carry deliberately anti-correlated names — not the reversed +/// name order either. +const SCRAMBLED: [(&str, &str); 4] = [ + ("src/zebra.rs", "name-aaa"), + ("src/alpha.rs", "name-zzz"), + ("src/middle.rs", "name-yyy"), + ("src/beta.rs", "name-mmm"), +]; + +/// The names of [`SCRAMBLED`] in emitted-path order — hand-pinned, and +/// the answer to no other ordering rule: not source order +/// (`aaa, zzz, yyy, mmm`), not name-ascending (`aaa, mmm, yyy, zzz`), +/// not name-descending (`zzz, yyy, mmm, aaa`), not path-descending +/// (`aaa, yyy, mmm, zzz`). +const PATH_ORDERED_NAMES: [&str; 4] = ["name-zzz", "name-mmm", "name-yyy", "name-aaa"]; + +/// Read the `name` of every element of a JSON aggregate, positionally. +fn aggregate_names(out: &Path) -> Vec { + let doc: serde_json::Value = + serde_json::from_slice(&std::fs::read(out).expect("aggregate written")) + .expect("aggregate is valid JSON"); + doc.as_array() + .expect("aggregate is a top-level array") + .iter() + .map(|elem| { + elem.get("name") + .and_then(serde_json::Value::as_str) + .expect("every element carries a name") + .to_owned() + }) + .collect() +} + +/// Workers stream their results in completion order, so `metrics +/// --output ` used to emit a differently-ordered document on +/// every run over an unchanged tree (#1244). The aggregate is sorted by +/// emitted path before serialization. +#[test] +fn write_aggregate_orders_metrics_elements_by_emitted_path() { + let dir = tempfile::tempdir().expect("tempdir"); + let out = dir.path().join("metrics.json"); + let items = SCRAMBLED + .iter() + .map(|(path, name)| { + AggregateItem::Metrics(Box::new(aggregate_space(name)), PathBuf::from(path)) + }) + .collect(); + + write_aggregate(Some(MetricsFormat::Json), items, &out, false); + + assert_eq!( + aggregate_names(&out), + PATH_ORDERED_NAMES, + "metrics aggregate elements must be emitted in sorted emitted-path order" + ); +} + +/// The `ops` arm of the same contract. `Ops` carries no path of its +/// own, which is why `AggregateItem::Ops` ships one alongside it. +#[test] +fn write_aggregate_orders_ops_elements_by_emitted_path() { + let dir = tempfile::tempdir().expect("tempdir"); + let out = dir.path().join("ops.json"); + let items = SCRAMBLED + .iter() + .map(|(path, name)| AggregateItem::Ops(Box::new(aggregate_ops(name)), PathBuf::from(path))) + .collect(); + + write_aggregate(Some(MetricsFormat::Json), items, &out, false); + + assert_eq!( + aggregate_names(&out), + PATH_ORDERED_NAMES, + "ops aggregate elements must be emitted in sorted emitted-path order" + ); +} + +/// The CSV arm builds its rows from the same vector, keyed by the +/// emitted path in the first column, so it inherits the same order. +#[test] +fn write_aggregate_orders_csv_rows_by_emitted_path() { + let dir = tempfile::tempdir().expect("tempdir"); + let out = dir.path().join("metrics.csv"); + let items = SCRAMBLED + .iter() + .map(|(path, name)| { + AggregateItem::Metrics(Box::new(aggregate_space(name)), PathBuf::from(path)) + }) + .collect(); + + write_aggregate(Some(MetricsFormat::Csv), items, &out, false); + + let text = std::fs::read_to_string(&out).expect("aggregate written"); + let mut lines = text.lines(); + assert!( + lines.next().is_some_and(|h| h.starts_with("path,")), + "expected the shared CSV header first: {text}" + ); + // One file contributes several rows (one per space), so collapse + // runs of the same path rather than asserting row-for-row. + let mut paths: Vec<&str> = Vec::new(); + for line in lines { + let path = line.split(',').next().expect("split yields one field"); + if paths.last() != Some(&path) { + paths.push(path); + } + } + assert_eq!( + paths, + [ + "src/alpha.rs", + "src/beta.rs", + "src/middle.rs", + "src/zebra.rs" + ], + "CSV aggregate rows must be grouped in sorted emitted-path order" + ); +} + +/// The degenerate sizes: sorting must not turn an empty run into a +/// missing document, nor a one-file run into an empty array. Both are +/// real `--output` invocations (a tree with no analyzable file; a +/// single explicit path). +#[test] +fn write_aggregate_handles_empty_and_single_element_runs() { + let dir = tempfile::tempdir().expect("tempdir"); + + let empty = dir.path().join("empty.json"); + write_aggregate(Some(MetricsFormat::Json), Vec::new(), &empty, false); + assert_eq!( + std::fs::read_to_string(&empty).expect("empty aggregate written"), + "[]", + "an empty run still writes an empty aggregate array" + ); + + let single = dir.path().join("single.json"); + write_aggregate( + Some(MetricsFormat::Json), + vec![AggregateItem::Metrics( + Box::new(aggregate_space("name-only")), + PathBuf::from("src/only.rs"), + )], + &single, + false, + ); + assert_eq!( + aggregate_names(&single), + ["name-only"], + "a single-element run emits exactly that element" + ); +} diff --git a/big-code-analysis-cli/src/diff.rs b/big-code-analysis-cli/src/diff.rs index dc9efd2a4..6cb9dc780 100644 --- a/big-code-analysis-cli/src/diff.rs +++ b/big-code-analysis-cli/src/diff.rs @@ -405,10 +405,10 @@ pub(crate) fn canonicalize_for_match(path: &Path) -> PathBuf { } /// Subcommand for `git rev-parse --verify ^{tree}`, used to confirm -/// a `--since` ref resolves to a tree object before we try to archive -/// it. The `^{tree}` peel rejects refs that exist but do not name a -/// commit/tree (e.g. a blob SHA), giving a precise diagnostic instead of -/// a confusing `git archive` failure downstream. +/// a `--since` ref resolves to a tree object before we try to +/// materialize it. The `^{tree}` peel rejects refs that exist but do not +/// name a commit/tree (e.g. a blob SHA), giving a precise diagnostic +/// instead of a confusing `ls-tree` failure downstream. const TREEISH_SUFFIX: &str = "^{tree}"; /// Validate that `since_ref` can serve as the "before" side of diff --git a/big-code-analysis-cli/src/dispatch.rs b/big-code-analysis-cli/src/dispatch.rs index 98a95bc73..478bfa603 100644 --- a/big-code-analysis-cli/src/dispatch.rs +++ b/big-code-analysis-cli/src/dispatch.rs @@ -355,7 +355,7 @@ fn dispatch_ops( // Single-file aggregate mode (`--output `, #669): stream // the ops tree to the post-walk collector. if let Some(tx) = &cfg.aggregate_tx { - let _ = tx.send(crate::AggregateItem::Ops(Box::new(ops))); + let _ = tx.send(crate::AggregateItem::Ops(Box::new(ops), path)); return Ok(()); } // CSV is rejected upstream in `run()` for the Ops command, diff --git a/big-code-analysis-cli/src/metric_diff.rs b/big-code-analysis-cli/src/metric_diff.rs index e1d60bc47..35066f7a7 100644 --- a/big-code-analysis-cli/src/metric_diff.rs +++ b/big-code-analysis-cli/src/metric_diff.rs @@ -61,7 +61,7 @@ const NAME_KEY: &str = "name"; /// interchangeable at the [`crate::walk_metric_set`] signature. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) enum DiffSide { - /// The `git archive` extraction of ``. + /// The materialized `` tree (see `diff::materialize_tree`). Before, /// The working tree (or the explicit directory positional). After, diff --git a/big-code-analysis-cli/src/walk.rs b/big-code-analysis-cli/src/walk.rs index 2adae079f..0c13e4e6a 100644 --- a/big-code-analysis-cli/src/walk.rs +++ b/big-code-analysis-cli/src/walk.rs @@ -14,10 +14,24 @@ use super::*; /// channel item stays small (a `FuncSpace` tree is large). pub(crate) enum AggregateItem { /// A `metrics` file-level space plus its emitted path (the path is - /// needed for the CSV aggregate, whose rows are keyed by file). + /// needed for the CSV aggregate, whose rows are keyed by file, and + /// as the aggregate document's ordering key). Metrics(Box, PathBuf), - /// An `ops` operator/operand tree. - Ops(Box), + /// An `ops` operator/operand tree plus its emitted path. `Ops` + /// carries only a `name`, which for non-UTF-8 input is a *lossy* + /// rendering of that path, so the path rides alongside it as the + /// ordering key (see `write_aggregate`). + Ops(Box, PathBuf), +} + +impl AggregateItem { + /// The emitted path of the file this result came from — the + /// aggregate document's ordering key (#1244). + pub(crate) fn emitted_path(&self) -> &Path { + match self { + Self::Metrics(_, path) | Self::Ops(_, path) => path, + } + } } /// Assemble a runtime [`GlobalOpts`] from a subcommand's flag groups. diff --git a/big-code-analysis-cli/tests/diff/diff_since.rs b/big-code-analysis-cli/tests/diff/diff_since.rs index ac60dcb08..ea55cad3e 100644 --- a/big-code-analysis-cli/tests/diff/diff_since.rs +++ b/big-code-analysis-cli/tests/diff/diff_since.rs @@ -290,7 +290,7 @@ fn since_leaves_no_temp_trees_behind() { #[test] fn since_from_subdir_pairs_against_repo_root() { // Regression: `bca diff --since` must pair files correctly when run - // from a subdirectory. The before side is a `git archive` of the + // from a subdirectory. The before side is a materialization of the // whole ref tree (rooted at the repo top), so the working-tree after // side must also anchor at the repo root — not the process CWD — // else the keys (`src/work.rs` vs a CWD-relative form) never match diff --git a/big-code-analysis-cli/tests/output/output_unification.rs b/big-code-analysis-cli/tests/output/output_unification.rs index 2701bab73..8b1bf71d5 100644 --- a/big-code-analysis-cli/tests/output/output_unification.rs +++ b/big-code-analysis-cli/tests/output/output_unification.rs @@ -11,6 +11,9 @@ //! - #1115: every file destination is written through a buffer that is //! explicitly flushed, so a document larger than the buffer is //! complete on disk rather than missing its tail. +//! - #1244: the aggregate document's elements are emitted in sorted +//! emitted-path order rather than worker-completion order, so two +//! runs over an unchanged tree produce byte-identical files. use assert_cmd::Command; use predicates::prelude::*; @@ -183,6 +186,129 @@ fn output_writes_single_aggregate_file() { "each element is a metrics record: {elem}" ); } + // The elements are ordered by emitted path, not by whichever worker + // finished first (#1244). Two files can pair up in the arrival order + // by chance, so the discriminating case is the many-file fixture in + // `aggregate_elements_are_ordered_by_path_not_worker_arrival`; this + // is the cheap guard on the shape everyone reads. + assert_eq!( + aggregate_names(&doc), + [ + dir.path().join("a.rs").to_str().unwrap(), + dir.path().join("b.rs").to_str().unwrap(), + ], + "aggregate elements must be emitted in sorted emitted-path order" + ); +} + +/// Read the `name` of every element of a JSON aggregate, positionally. +/// Deliberately order-preserving: the order *is* the property under +/// test in the #1244 assertions below. +fn aggregate_names(doc: &serde_json::Value) -> Vec<&str> { + doc.as_array() + .expect("aggregate is a top-level array") + .iter() + .map(|elem| { + elem.get("name") + .and_then(serde_json::Value::as_str) + .expect("every element carries a name") + }) + .collect() +} + +/// Write `FILES` one-function sources under `dir` and return their +/// paths in the order the aggregate must emit them. +/// +/// The expectation is sorted as `PathBuf`s, not as strings: `Path` +/// compares component-wise (`a/b` sorts before `a-x/c`, the reverse of +/// the bytewise answer), and production sorts paths. A string-sorted +/// expectation agrees only as long as every fixture file sits in one +/// flat directory, and would fail spuriously the day one does not. +fn write_ordering_fixture(dir: &std::path::Path, files: usize) -> Vec { + let mut expected: Vec = Vec::with_capacity(files); + for i in 0..files { + // `m10.rs` sorts before `m2.rs`, so the numeric and lexicographic + // orders disagree — an assertion that accidentally described + // creation order would not match. + let file = dir.join(format!("m{i}.rs")); + std::fs::write(&file, format!("fn f{i}() {{ let x = {i}; }}\n")).unwrap(); + expected.push(file); + } + expected.sort_unstable(); + expected + .iter() + .map(|p| p.to_str().unwrap().to_owned()) + .collect() +} + +/// Run `command --output ` as JSON over `dir` at `--jobs 8` and +/// return the emitted document. +fn aggregate_at_eight_jobs( + dir: &std::path::Path, + command: &str, + out: &std::path::Path, +) -> serde_json::Value { + cli() + .args([ + command, + "--paths", + dir.to_str().unwrap(), + "-O", + "json", + "--output", + out.to_str().unwrap(), + "--jobs", + "8", + ]) + .assert() + .success(); + + serde_json::from_slice(&std::fs::read(out).unwrap()).expect("aggregate is valid JSON") +} + +/// Enough files that worker completion order reliably diverges from the +/// walk's sorted file list at `--jobs 8`. +const ORDERING_FIXTURE_FILES: usize = 24; + +/// Workers finish out of order, so before #1244 this fixture emitted a +/// different permutation on every run — twelve measured runs gave +/// twelve distinct orders and not one of them sorted. The aggregate is +/// now sorted by emitted path before serialization, which makes a +/// single run's order the whole assertion: no repeat-and-compare, which +/// could pass on scheduling luck. +#[test] +fn aggregate_elements_are_ordered_by_path_not_worker_arrival() { + let dir = TempDir::new().unwrap(); + let expected = write_ordering_fixture(dir.path(), ORDERING_FIXTURE_FILES); + let out = dir.path().join("agg.json"); + + let doc = aggregate_at_eight_jobs(dir.path(), "metrics", &out); + + assert_eq!( + aggregate_names(&doc), + expected, + "aggregate elements must be emitted in sorted emitted-path order" + ); +} + +/// The `ops` half of the same contract, end to end. `AggregateItem::Ops` +/// carries a `PathBuf` for one reason — it is the sort key — and only a +/// multi-file `ops --output` run exercises the wiring that puts it +/// there: with the walk's path dropped at the send site, every unit test +/// above still passes because they construct the items by hand. +#[test] +fn ops_aggregate_elements_are_ordered_by_path_not_worker_arrival() { + let dir = TempDir::new().unwrap(); + let expected = write_ordering_fixture(dir.path(), ORDERING_FIXTURE_FILES); + let out = dir.path().join("ops.json"); + + let doc = aggregate_at_eight_jobs(dir.path(), "ops", &out); + + assert_eq!( + aggregate_names(&doc), + expected, + "ops aggregate elements must be emitted in sorted emitted-path order" + ); } /// `--output-dir ` writes the per-file tree (the historical diff --git a/big-code-analysis-py/README.md b/big-code-analysis-py/README.md index 022edf99c..1989648fa 100644 --- a/big-code-analysis-py/README.md +++ b/big-code-analysis-py/README.md @@ -3,30 +3,29 @@ Python bindings for the [`big-code-analysis`](https://github.com/dekobon/big-code-analysis) Rust library — compute maintainability metrics for source code in -~20 languages using the same tree-sitter parsers the Rust crate -ships with. +~20 languages using the same +[tree-sitter](https://tree-sitter.github.io/tree-sitter/) parsers the +Rust crate ships with. **Full documentation:** the book's [Python Bindings chapter](https://dekobon.github.io/big-code-analysis/python/index.html) -covers the install matrix, batch / async / SARIF recipes, and the -full error taxonomy. The README below is the quick reference -shown on PyPI. - -All nine phases of the Python bindings work (issues #265–#273; -parent #103) have landed. The crate now ships single-file -analysis, the never-raise batch entry point, the `flatten_spaces` -flat-record iterator, explicit metric selection (`metrics=`), -SARIF 2.1.0 rendering (`to_sarif`), the strict ruff / mypy / -pyright tooling gate, manylinux wheel CI on Linux x86_64 + -aarch64, the book's "Python Bindings" chapter, and the end-user -example set covered below. See the -[CHANGELOG](../CHANGELOG.md) for the per-phase changes. +covers the install matrix, batch / async / [SARIF +2.1.0](https://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html) +recipes, and the full error taxonomy. The README below is the quick +reference shown on PyPI. + +The bindings provide single-file analysis, a never-raise batch entry +point, the `flatten_spaces` flat-record iterator, explicit metric +selection (`metrics=`), SARIF rendering (`to_sarif`), and manylinux +wheels for Linux x86_64 and aarch64. See the +[CHANGELOG](https://github.com/dekobon/big-code-analysis/blob/main/CHANGELOG.md) +for what changed between releases. ## Runnable examples `big-code-analysis-py/examples/` is the canonical collection of -copy-paste recipes. Every file is executed under CI either via -`tests/test_book_examples.py` (the `.py` examples) or via +copy-paste recipes. CI executes every file, either through +`tests/test_book_examples.py` (the `.py` examples) or through `jupyter nbconvert --execute` (the notebook), so a renamed kwarg or removed function fails CI before the example can rot in the docs. @@ -36,7 +35,7 @@ docs. | `quick_start.py` | Single-file analysis + headline metric. Embedded by the book's *Quick start*. | | `batch_processing.py` | `analyze_batch` + the `AnalysisFailure` discriminator. Embedded by *Batch processing*. | | `flat_records.py` | `flatten_spaces` → sqlite for one file. Embedded by *Flat-record iteration*. | -| `metric_selection.py` | `metrics=` kwarg + dependency-pull behaviour. Embedded by *Metric selection*. | +| `metric_selection.py` | `metrics=` kwarg + dependency-pull behavior. Embedded by *Metric selection*. | | `sarif_output.py` | Minimal SARIF rendering. Embedded by *SARIF output*. | | `errors_taxonomy.py` | The full exception map across the entry points. Embedded by *Error handling*. | | `async_patterns.py` | `asyncio.to_thread` (canonical) vs the in-loop anti-pattern. Embedded by *Async patterns*. | @@ -50,7 +49,8 @@ docs. The package is published on PyPI: the distribution is **`big-code-analysis`** and the import name **`big_code_analysis`** — names locked by the stability contract (see -[`STABILITY.md`](../STABILITY.md), *Python bindings*). +[`STABILITY.md`](https://github.com/dekobon/big-code-analysis/blob/main/STABILITY.md), +*Python bindings*). ```bash pip install big-code-analysis @@ -80,15 +80,15 @@ Install `uv` via or `pipx install uv`. Alternative provisioning paths (`mise install`, direct `pipx install ruff/mypy/pyright/maturin`, plain `pip install -e ".[dev]"`) still work — see -[CONTRIBUTING.md](../CONTRIBUTING.md) — but only the uv-managed path -is pinned to `uv.lock`. +[CONTRIBUTING.md](https://github.com/dekobon/big-code-analysis/blob/main/CONTRIBUTING.md) +— but only the uv-managed path is pinned to `uv.lock`. ## Usage ```python import big_code_analysis as bca -# Analyse a file by path. The returned dict matches the JSON +# Analyze a file by path. The returned dict matches the JSON # emitted by `bca metrics --format json` for the same # file at the `FuncSpace` boundary — same field order, same # numeric formatting, same shape. Language detection mirrors the @@ -104,20 +104,20 @@ result = bca.analyze("src/main.rs") if result is not None: print(result["metrics"]["cognitive"]["sum"]) -# Analyse a Rust file with `#[test]` subtrees pruned out — same +# Analyze a Rust file with `#[test]` subtrees pruned out — same # result as `bca metrics --exclude-tests --format json`. prod_only = bca.analyze("src/main.rs", exclude_tests=True) # Non-UTF-8 paths raise `ValueError` by default so the `name` # field is always a round-trippable identifier. Pass # `allow_lossy_path=True` to opt into the CLI's U+FFFD -# substitution behaviour (see `bca.analyze.__doc__` and #316). +# substitution behavior (see `bca.analyze.__doc__` and #316). lossy = bca.analyze(weird_path, allow_lossy_path=True) # Force analysis of files marked `@generated` (default skips them). forced = bca.analyze("third_party/generated.pb.go", skip_generated=False) -# Analyse an in-memory snippet (str, bytes, or bytearray accepted). +# Analyze an in-memory snippet (str, bytes, or bytearray accepted). metrics = bca.analyze_source("fn main() {}\n", "rust") # Language detection helpers. `language_for_file` reads the file @@ -154,7 +154,7 @@ assert "py" in bca.language_extensions(Lang.PYTHON) Pass `metrics=[…]` to compute only a subset of the metric suite. `metrics=None` (the default) preserves today's "compute everything" -behaviour. Unrequested metrics are **absent** from the result dict +behavior. Unrequested metrics are **absent** from the result dict (not present with `None` placeholders). ```python @@ -179,7 +179,7 @@ assert "halstead" in bca.METRIC_NAMES assert bca.MetricName.HALSTEAD == "halstead" ``` -The same kwarg is honoured by `bca.analyze_source` and +The same kwarg is honored by `bca.analyze_source` and `bca.analyze_batch` — the latter applies the selection uniformly to every file in the batch. Validation runs *before* any file I/O: an empty list or unknown name raises `ValueError` immediately and never @@ -205,7 +205,7 @@ Duplicates are silently collapsed. result (or an iterable of them) into a SARIF 2.1.0 JSON document suitable for upload to GitHub Code Scanning or any other SARIF consumer. The output is produced by the same Rust writer that -backs `bca check -O sarif`, so the schema URL, tool driver name / +backs `bca check --report-format sarif`, so the schema URL, tool driver name / version, and rule descriptions match the CLI byte-for-byte. ```python @@ -221,7 +221,7 @@ with open("metrics.sarif", "w", encoding="utf-8") as fh: fh.write(sarif) # Batch input — AnalysisFailure entries are skipped silently because -# they represent files we couldn't analyse, not findings. +# they represent files we couldn't analyze, not findings. batch = bca.analyze_batch(["src/a.py", "src/b.rs", "src/c.cpp"]) sarif = bca.to_sarif(batch, thresholds={"cognitive": 20}) ``` @@ -239,8 +239,9 @@ typo fails fast instead of silently producing an empty SARIF run. `thresholds=None` (the default) and `thresholds={}` both produce a well-formed SARIF document with empty `results` and `rules` -arrays. This matches the CLI's posture: there are **no built-in -default thresholds**; every check run supplies its own limits. +arrays. This matches `bca check`, which applies **no implicit +limits**: every run supplies its own, from `--threshold` or a +`bca.toml` (which `bca init` scaffolds with a starting table). **Unit-level findings.** `to_sarif` emits a finding at every space whose **own** value breaches its limit, exactly matching @@ -251,9 +252,10 @@ subtree-aggregate metrics (`cyclomatic`, `cyclomatic.modified`, `cognitive`, `abc`) read the per-space `value` field rather than the rolled-up aggregate (#958, #969). Unit findings carry `logicalLocations: [{"fullyQualifiedName": -""}]`; nameless non-unit spaces (rare parse-failure case) -carry `""` — both matching the CLI's `function_token` -placeholders. See the book's +""}]`; every other space carries its qualified symbol. Within +that symbol, a closure/lambda (the `` name every grammar +emits) and the `None`-name parse-failure case both collapse to +``, matching the CLI's `space_segment`. See the book's [SARIF output](https://dekobon.github.io/big-code-analysis/python/sarif.html) page for the full contract. @@ -286,13 +288,13 @@ order, so `zip(inputs, results)` lines up by index **when no path is skipped**. `analyze_batch` accepts the same keyword-only options as `analyze` — `exclude_tests`, `allow_lossy_path`, `skip_generated` (default `True`), `metrics` — so migrating between the two is -behaviour-preserving. +behavior-preserving. With the default `skip_generated=True`, a generated file is *skipped* and produces no result element (matching single-file `analyze`, which returns `None`), so the result list can be shorter than the input. This default flipped at 2.0 — the pre-2.0 `analyze_batch` always -analysed generated files (`skip_generated=False`); pass that +analyzed generated files (`skip_generated=False`); pass that explicitly to restore one-element-per-input. ```python @@ -308,9 +310,9 @@ for path, result in zip(paths, results): ``` The pattern above keeps `paths` and `results` as separate -materialised sequences. If you want to drive `analyze_batch` from +materialized sequences. If you want to drive `analyze_batch` from a generator (e.g. `glob.iglob('**/*.py')`) for memory efficiency, -materialise it into a list first — otherwise +materialize it into a list first — otherwise `zip(generator, analyze_batch(generator))` yields nothing because `analyze_batch` exhausts the generator before `zip` re-iterates it: @@ -345,7 +347,7 @@ parallel single-file calls. With the 2.0 default `skip_generated=True`, `analyze_batch` applies the CLI's `is_generated` walker filter, so a generated file is skipped and contributes no result element — the result list can be shorter than the input. Pass `skip_generated=False` -to analyse every file and guarantee one `dict`-or-`AnalysisFailure` +to analyze every file and guarantee one `dict`-or-`AnalysisFailure` element per input. ## Flatten to records @@ -389,7 +391,7 @@ with sqlite3.connect("metrics.db") as db: ``` The iterator is lazy and single-use: it walks the input once -without materialising the whole list, and a second iteration is +without materializing the whole list, and a second iteration is empty. Records always carry `path` (the analyzed file, or `None` for `analyze_source`), `name`, `kind`, `start_line`, `end_line`, `parent_name`, and `depth`. Anonymous spaces (Rust closures, JS @@ -422,7 +424,7 @@ per-file failures — see the Batch processing section above). Exception types raised by `bca.analyze` / `bca.analyze_source`: - `bca.UnsupportedLanguageError` (subclass of `ValueError`) — - raised when a file extension is unrecognised, or when + raised when a file extension is unrecognized, or when `analyze_source(..., language="...")` is passed an unknown language name. - `bca.ParseError` (subclass of `ValueError`) — raised when the @@ -469,7 +471,7 @@ Returned by `bca.analyze_batch` inside the result list: `error_kind` is a closed taxonomy: ``"IoError"`` covers both filesystem failures and the non-UTF-8 path case (kept at three kinds per the API contract); ``"ParseError"`` similarly covers - internal JSON-serialisation failures of the resulting + internal JSON-serialization failures of the resulting `FuncSpace` (rare; reserved upstream). The OS `errno` is preserved in the `error` string via Rust's ``" (os error )"`` default formatting — parse with regex diff --git a/big-code-analysis-py/python/big_code_analysis/_native.pyi b/big-code-analysis-py/python/big_code_analysis/_native.pyi index da223b51a..d1fadda32 100644 --- a/big-code-analysis-py/python/big_code_analysis/_native.pyi +++ b/big-code-analysis-py/python/big_code_analysis/_native.pyi @@ -974,8 +974,10 @@ def to_sarif( ``abc``) read the per-space ``value`` field rather than the rolled-up aggregate (#958, #969). Unit-level findings carry ``logicalLocations: [{"fullyQualifiedName": - ""}]``; nameless non-unit spaces carry ``""`` — - matching the CLI's ``function_token`` placeholder. + ""}]``; every other space carries its qualified symbol. Within + that symbol, a closure/lambda (the ```` name every grammar + emits) and the ``None``-name parse-failure case both collapse to + ````, matching the CLI's ``space_segment``. Raises ------ diff --git a/big-code-analysis-py/src/lib.rs b/big-code-analysis-py/src/lib.rs index 1dad18dd7..fd80b5742 100644 --- a/big-code-analysis-py/src/lib.rs +++ b/big-code-analysis-py/src/lib.rs @@ -806,7 +806,7 @@ fn vcs_commit( } /// Score an arbitrary unified diff for partial just-in-time risk (issue -/// #580). The programmatic analogue of `bca vcs jit --diff`; lives under +/// #580). The programmatic analogue of `bca vcs commit --diff`; lives under /// `big_code_analysis.vcs.score_diff`. /// /// `diff` is a unified diff string. A bare diff has no author / parent / @@ -896,7 +896,7 @@ fn _native(m: &Bound<'_, PyModule>) -> PyResult<()> { /// `vcs.rank` / `vcs.trend` / `vcs.commit` / `vcs.score_diff` plus the /// shared `vcs.Options` object. The entry-point names mirror the `bca vcs` /// CLI subcommands (`bca vcs` → `rank`, `vcs trend` → `trend`, -/// `vcs commit` → `commit`, `vcs jit --diff` → `score_diff`). +/// `vcs commit` → `commit`, `vcs commit --diff` → `score_diff`). /// /// The submodule is registered with the dotted `module` path on each item /// so `repr()` / pickling report `big_code_analysis.vcs.*`, and inserted diff --git a/big-code-analysis-py/src/sarif.rs b/big-code-analysis-py/src/sarif.rs index 0ded0de73..37a34716a 100644 --- a/big-code-analysis-py/src/sarif.rs +++ b/big-code-analysis-py/src/sarif.rs @@ -441,7 +441,7 @@ fn extract_space_fields(space: &Bound<'_, PyDict>) -> PyResult { // This space's own `::`-segment in the qualified-symbol chain. // Mirrors the CLI's `space_segment` - // (`big-code-analysis-cli/src/thresholds.rs`): a named space + // (`big-code-analysis-cli/src/qualified_name.rs`): a named space // contributes its AST name; an anonymous space — the literal // `` every grammar emits for closures/lambdas, plus the // `None`-name parse-failure case — collapses to `` diff --git a/big-code-analysis-py/tests/test_sarif.py b/big-code-analysis-py/tests/test_sarif.py index 5b09e3b44..536c99df2 100644 --- a/big-code-analysis-py/tests/test_sarif.py +++ b/big-code-analysis-py/tests/test_sarif.py @@ -957,7 +957,7 @@ def test_to_sarif_unit_space_emits_file_placeholder() -> None: """A unit-level finding (for a File-scoped metric, #969) emits ``logicalLocations: [{fullyQualifiedName: ''}]`` rather than duplicating the path that already appears in - ``artifactLocation.uri``. Matches the CLI's ``function_token``. + ``artifactLocation.uri``. Matches the CLI's ``qualified_symbol``. """ code = "class A:\n def m(self): pass\n" result = bca.analyze_source(code, "python") diff --git a/big-code-analysis-py/tests/test_vcs.py b/big-code-analysis-py/tests/test_vcs.py index 8a39ba26e..813908b3c 100644 --- a/big-code-analysis-py/tests/test_vcs.py +++ b/big-code-analysis-py/tests/test_vcs.py @@ -486,9 +486,9 @@ def test_vcs_metrics_no_cache_writes_nothing(tmp_path: Path) -> None: def test_vcs_jit_commit_returns_report(tmp_path: Path) -> None: - """vcs_jit(repo, commit) returns the full commit JIT report (issue + """vcs.commit(repo, commit) returns the full commit JIT report (issue #331). The commit-mode report carries the score plus every feature - group, mirroring ``bca vcs jit``.""" + group, mirroring ``bca vcs commit``.""" repo = _build_repo(tmp_path) report = bca_vcs.commit(repo, commit="HEAD") assert report["jit_schema_version"] == 3 @@ -522,7 +522,7 @@ def test_vcs_jit_commit_returns_report(tmp_path: Path) -> None: def test_vcs_jit_diff_mode_marks_unavailable_groups() -> None: - """vcs_jit(diff=...) scores an arbitrary diff (issue #580). Only size and + """vcs.score_diff(diff) scores an arbitrary diff (issue #580). Only size and diffusion are computable, so the report is marked ``source == "diff"`` and the unavailable groups are ABSENT (not present as zero) — a consumer cannot read a missing group as "low risk".""" diff --git a/big-code-analysis-web/src/web/server/errors.rs b/big-code-analysis-web/src/web/server/errors.rs index 9c966d63d..3b33c7556 100644 --- a/big-code-analysis-web/src/web/server/errors.rs +++ b/big-code-analysis-web/src/web/server/errors.rs @@ -225,8 +225,9 @@ pub(crate) fn not_acceptable_message() -> String { /// cause. The set is governed by `STABILITY.md`; adding a token is an /// additive change, renaming or removing one is a break. Clients branch /// on these tokens instead of string-matching the free-form `error` -/// prose. The vcs-family mapping lives in [`vcs_error_kind`], keyed off -/// `vcs::Error`'s own variants so a new variant forces a token decision. +/// prose. The vcs-family mapping lives in [`vcs_error_kind`]; see there +/// for why a new `vcs::Error` variant cannot be *compile*-forced to pick +/// a token, and which test stands in for the compiler (#1245). pub(crate) mod error_kind { /// `file_name` (and content sniffing) mapped to no supported language. pub(crate) const UNSUPPORTED_LANGUAGE: &str = "unsupported_language"; @@ -277,6 +278,9 @@ pub(crate) mod error_kind { pub(crate) const VCS_INVALID_FILE_TYPE_SCOPE: &str = "vcs_invalid_file_type_scope"; /// The bus-factor threshold was out of range. pub(crate) const VCS_INVALID_BUS_FACTOR_THRESHOLD: &str = "vcs_invalid_bus_factor_threshold"; + /// The `author_hash_key` was empty, or was supplied without + /// `emit_author_details` (which it has no effect without) — #956. + pub(crate) const VCS_INVALID_AUTHOR_HASH_KEY: &str = "vcs_invalid_author_hash_key"; /// A trend point-count / span option was malformed. pub(crate) const VCS_INVALID_TREND: &str = "vcs_invalid_trend"; /// The unified diff passed to `/vcs/jit` was malformed. @@ -298,12 +302,25 @@ pub(crate) mod error_kind { /// Stable `error_kind` token for a [`VcsError`] (#631). /// -/// Keyed exhaustively off the library `vcs::Error` variants (no wildcard -/// arm) so adding a variant is a compile error here until a token is -/// chosen — the same forcing function `is_client_input` uses. Every -/// environment / backend failure collapses onto +/// Every environment / backend failure collapses onto /// [`error_kind::VCS_INTERNAL_ERROR`]; each client-input variant gets its /// own specific token. +/// +/// **The wildcard is not a safety net, and the compiler cannot help +/// here.** `vcs::Error` is `#[non_exhaustive]`, so this cross-crate match +/// is *required* to carry a wildcard — an omitted client-input variant +/// silently reaches it and is reported to the client as a server fault. +/// That is not hypothetical: `InvalidAuthorHashKey` did exactly that from +/// #956 until #1245, answering `400` (which `is_client_input` gets right +/// on its own) with `vcs_internal_error`. `is_client_input` is the +/// forcing function for *status*, and only for status; it forces nothing +/// about this table. +/// +/// The guard for this table is therefore a test, not the compiler: +/// `vcs_error_kind_maps_each_client_variant_to_a_distinct_token` iterates +/// `vcs::Error::client_input_samples()`, which the library generates from +/// the same list as `is_client_input`'s arms. Adding a client-input +/// variant there fails that test until a token is added below. pub(crate) fn vcs_error_kind(error: &VcsError) -> &'static str { match error { VcsError::NotARepository(_) => error_kind::VCS_NOT_A_REPOSITORY, @@ -314,14 +331,14 @@ pub(crate) fn vcs_error_kind(error: &VcsError) -> &'static str { VcsError::InvalidFormula(_) => error_kind::VCS_INVALID_FORMULA, VcsError::InvalidFileTypeScope(_) => error_kind::VCS_INVALID_FILE_TYPE_SCOPE, VcsError::InvalidBusFactorThreshold(_) => error_kind::VCS_INVALID_BUS_FACTOR_THRESHOLD, + VcsError::InvalidAuthorHashKey(_) => error_kind::VCS_INVALID_AUTHOR_HASH_KEY, VcsError::InvalidTrend(_) => error_kind::VCS_INVALID_TREND, VcsError::InvalidDiff(_) => error_kind::VCS_INVALID_DIFF, - // Every environment / backend failure (`OpenRepository`, `Walk`, - // `Diff`, `Mailmap`, `Blame`, `Cache`) and any future - // `#[non_exhaustive]` variant collapses onto the generic internal - // token; `is_client_input` already owns the exhaustive - // client-vs-environment forcing function, so this wildcard cannot - // silently mis-classify a client error. + // Reached by every environment / backend failure + // (`OpenRepository`, `Walk`, `Diff`, `Mailmap`, `Blame`, `Cache`), + // which is what it is for, and by any future client-input variant + // whose token is still missing, which is what the samples-driven + // guard test above exists to catch. _ => error_kind::VCS_INTERNAL_ERROR, } } diff --git a/big-code-analysis-web/src/web/server_tests.rs b/big-code-analysis-web/src/web/server_tests.rs index 5a956db8b..b0f366ae7 100644 --- a/big-code-analysis-web/src/web/server_tests.rs +++ b/big-code-analysis-web/src/web/server_tests.rs @@ -2883,75 +2883,90 @@ async fn vcs_error_response_maps_classification_to_status() { async fn vcs_error_kind_maps_each_client_variant_to_a_distinct_token() { // Locks the closed #631 token vocabulary: every `is_client_input` // variant gets its own distinct, non-internal token. `is_client_input` - // (#641) owns 400-vs-500; `vcs_error_kind` owns which 400 token — this - // pins all ten so a future variant or a renamed token cannot silently - // collapse onto the generic internal token or collide with a sibling. + // (#641) owns 400-vs-500; `vcs_error_kind` owns which 400 token. + // + // The case list is *derived from the enum*, not hand-written. The + // hand-written version of this test pinned ten variants and read as a + // completeness guard while `InvalidAuthorHashKey` — added to + // `is_client_input` by #956 — collapsed onto `vcs_internal_error` for + // the whole of its life (#1245). `client_input_samples()` is generated + // from the same list as `is_client_input`'s arms, so a variant cannot + // be classified as client input without appearing here. use std::collections::HashSet; - use std::path::PathBuf; - let cases: [(VcsError, &str); 10] = [ - ( - VcsError::NotARepository(PathBuf::from("/x")), - error_kind::VCS_NOT_A_REPOSITORY, - ), - ( - VcsError::ResolveRef { - reference: "HEAD".to_owned(), - reason: "gone".to_owned(), - }, - error_kind::VCS_INVALID_REVISION, - ), - ( - VcsError::InvalidBotPattern("[".to_owned()), - error_kind::VCS_INVALID_BOT_PATTERN, - ), - ( - VcsError::InvalidWindow("x".to_owned()), - error_kind::VCS_INVALID_WINDOW, - ), - ( - VcsError::InvalidTimestamp("x".to_owned()), - error_kind::VCS_INVALID_TIMESTAMP, - ), - ( - VcsError::InvalidFormula("x".to_owned()), - error_kind::VCS_INVALID_FORMULA, - ), - ( - VcsError::InvalidFileTypeScope("x".to_owned()), - error_kind::VCS_INVALID_FILE_TYPE_SCOPE, - ), - ( - VcsError::InvalidBusFactorThreshold("x".to_owned()), - error_kind::VCS_INVALID_BUS_FACTOR_THRESHOLD, - ), - ( - VcsError::InvalidTrend("x".to_owned()), - error_kind::VCS_INVALID_TREND, - ), - ( - VcsError::InvalidDiff("x".to_owned()), - error_kind::VCS_INVALID_DIFF, - ), - ]; + // Expected tokens are spelled as *literals*, not as `error_kind::` + // constants. The vocabulary is a closed contract in `STABILITY.md`, so + // a changed constant value has to fail here; the previous version + // compared each constant against itself and pinned no wire string at + // all. The `None` arm is the completeness catch: `vcs::Error` is + // `#[non_exhaustive]`, so this match must carry a wildcard, and a + // sample reaching it is a client-input variant with no agreed token. + fn expected_token(error: &VcsError) -> Option<&'static str> { + match error { + VcsError::NotARepository(_) => Some("vcs_not_a_repository"), + VcsError::ResolveRef { .. } => Some("vcs_invalid_revision"), + VcsError::InvalidBotPattern(_) => Some("vcs_invalid_bot_pattern"), + VcsError::InvalidWindow(_) => Some("vcs_invalid_window"), + VcsError::InvalidTimestamp(_) => Some("vcs_invalid_timestamp"), + VcsError::InvalidFormula(_) => Some("vcs_invalid_formula"), + VcsError::InvalidFileTypeScope(_) => Some("vcs_invalid_file_type_scope"), + VcsError::InvalidBusFactorThreshold(_) => Some("vcs_invalid_bus_factor_threshold"), + VcsError::InvalidAuthorHashKey(_) => Some("vcs_invalid_author_hash_key"), + VcsError::InvalidTrend(_) => Some("vcs_invalid_trend"), + VcsError::InvalidDiff(_) => Some("vcs_invalid_diff"), + _ => None, + } + } + + // Every assertion below iterates `samples`, so a sample set that lost + // an entry satisfies all of them. Pin the size, and pin the #1245 + // subject by name: the count catches any variant leaving the + // client-input group, the `any` catches this one specifically. + let samples = VcsError::client_input_samples(); + assert_eq!( + samples.len(), + 11, + "the client-input variant set changed size; every entry needs a \ + token in `vcs_error_kind` and a case in `expected_token` above", + ); + assert!( + samples + .iter() + .any(|error| matches!(error, VcsError::InvalidAuthorHashKey(_))), + "the #1245 regression subject must stay in the sampled client-input set", + ); + let mut seen = HashSet::new(); - for (err, expected) in &cases { + for error in &samples { assert!( - err.is_client_input(), - "{err:?} must classify as client input" + error.is_client_input(), + "{error:?} is sampled as client input but classifies as environment", ); - let token = vcs_error_kind(err); - assert_eq!(token, *expected, "wrong token for {err:?}"); + let token = vcs_error_kind(error); + let Some(expected) = expected_token(error) else { + panic!( + "client-input variant {error:?} has no agreed error_kind token; \ + add one to `vcs_error_kind`, to `expected_token` above, and to \ + the vocabulary lists in STABILITY.md and the book (got `{token}`)" + ); + }; + assert_eq!(token, expected, "wrong token for {error:?}"); + // Production can no longer reach this — the `assert_eq!` above + // fires first. It guards the *expectation* table instead: it + // stops a future maintainer from silencing the `else` panic by + // writing `Some("vcs_internal_error")` into `expected_token`, + // which would re-bless the #1245 behaviour as intended. assert_ne!( token, error_kind::VCS_INTERNAL_ERROR, - "client variant {err:?} must not collapse onto the internal token", + "client variant {error:?} must not collapse onto the internal token", ); assert!( seen.insert(token), "token `{token}` is not distinct across client variants" ); } + // An environment/backend variant collapses onto the internal token. assert!(!VcsError::Walk("boom".to_owned()).is_client_input()); assert_eq!( @@ -2960,6 +2975,113 @@ async fn vcs_error_kind_maps_each_client_variant_to_a_distinct_token() { ); } +/// POSTs `payload` to `uri` through the production routing table and +/// returns the response status alongside the decoded JSON body. +/// +/// [`post_raw_json`] does almost the same thing and is the better helper +/// when only the body matters; this one keeps the status, because an +/// `error_kind` assertion is only half the contract without it. +async fn post_vcs_json(uri: &str, payload: Value) -> (StatusCode, Value) { + let app = test::init_service( + App::new() + .app_data(test_config()) + .configure(configure_routes), + ) + .await; + let req = test::TestRequest::post() + .uri(uri) + .insert_header(ContentType::json()) + .set_json(&payload) + .to_request(); + let resp = test::call_service(&app, req).await; + let status = resp.status(); + (status, test::read_body_json(resp).await) +} + +#[actix_rt::test] +async fn vcs_author_hash_key_without_emit_is_400_with_its_own_token() { + // #1245: the status was always right (`is_client_input` covers + // `InvalidAuthorHashKey`), but the machine token was + // `vcs_internal_error` — telling a client its own mistake was a server + // fault. Assert all three fields: a status-only assertion, which is + // what the sibling `*_is_400` tests do, is exactly what let this ship. + // + // A bare temp dir is enough for all three of these — the handler's + // `repo_path_must_exist` is a `Path::exists` call and `options_from` + // rejects the key before any history walk. Drop the validation and + // the request fails on the missing repository instead, which the + // `error_kind` assertion still catches, so the real git repository + // the sibling vcs tests build would buy nothing but four subprocesses. + let dir = tempfile::tempdir().expect("tempdir"); + let (status, body) = post_vcs_json( + "/v1/vcs", + json!({ + "id": "req-ahk", + "repo_path": dir.path().to_str().unwrap(), + "author_hash_key": "k", + }), + ) + .await; + assert_eq!(status, StatusCode::BAD_REQUEST); + assert_eq!(body["error_kind"], json!("vcs_invalid_author_hash_key")); + assert_eq!( + body["error"], + json!("invalid author-hash key: author_hash_key requires emit_author_details"), + ); + assert_eq!(body["id"], json!("req-ahk")); +} + +#[actix_rt::test] +async fn vcs_trend_author_hash_key_without_emit_is_400_with_its_own_token() { + // `/vcs/trend` reaches the same `options_from`, so it carried the same + // mislabelling (#1245). Distinct `id` and endpoint from the `/vcs` + // case so a copy-paste that points both tests at one route shows up. + let dir = tempfile::tempdir().expect("tempdir"); + let (status, body) = post_vcs_json( + "/v1/vcs/trend", + json!({ + "id": "req-ahk-trend", + "repo_path": dir.path().to_str().unwrap(), + "author_hash_key": "k", + }), + ) + .await; + assert_eq!(status, StatusCode::BAD_REQUEST); + assert_eq!(body["error_kind"], json!("vcs_invalid_author_hash_key")); + assert_eq!( + body["error"], + json!("invalid author-hash key: author_hash_key requires emit_author_details"), + ); + assert_eq!(body["id"], json!("req-ahk-trend")); +} + +#[actix_rt::test] +async fn vcs_empty_author_hash_key_is_400_with_its_own_token() { + // The second trigger: `AuthorHashKey::new` rejects an empty key even + // when `emit_author_details` is set (an unset environment variable + // expanding to `""`, #956). It reaches `InvalidAuthorHashKey` down a + // different path in `options_from`, and its prose differs from the + // requires-emit case above — so the two cannot pass for each other. + let dir = tempfile::tempdir().expect("tempdir"); + let (status, body) = post_vcs_json( + "/v1/vcs", + json!({ + "id": "req-ahk-empty", + "repo_path": dir.path().to_str().unwrap(), + "emit_author_details": true, + "author_hash_key": "", + }), + ) + .await; + assert_eq!(status, StatusCode::BAD_REQUEST); + assert_eq!(body["error_kind"], json!("vcs_invalid_author_hash_key")); + assert_eq!( + body["error"], + json!("invalid author-hash key: the key is empty"), + ); + assert_eq!(body["id"], json!("req-ahk-empty")); +} + #[actix_rt::test] async fn test_web_vcs_outside_repo_is_400() { let dir = tempfile::tempdir().expect("tempdir"); diff --git a/enums/tests/dispatch.rs b/enums/tests/dispatch.rs index 2a38bf937..952454b9f 100644 --- a/enums/tests/dispatch.rs +++ b/enums/tests/dispatch.rs @@ -81,12 +81,13 @@ fn lang_irules_resolves_to_tree_sitter_irules() { } // The Cpp -> mozcpp swap is the original drift bug this test file -// guards against (issue #344). Asserting against `tree_sitter_mozcpp` -// (not `tree_sitter_cpp`) is the load-bearing detail here. +// guards against (issue #344). Since #720 `Lang::Cpp` is backed by the +// upstream community grammar and the Mozilla fork moved to +// `Lang::Mozcpp`, asserting against `tree_sitter_cpp` (not +// `tree_sitter_mozcpp`) is the load-bearing detail here; +// `lang_mozcpp_resolves_to_vendored_mozcpp` below pins the fork. #[test] fn lang_cpp_resolves_to_upstream_tree_sitter_cpp() { - // Since #720 `Lang::Cpp` is backed by the upstream community - // grammar; the Mozilla fork moved to `Lang::Mozcpp` below. assert_dispatch(&Lang::Cpp, "Cpp", &tree_sitter_cpp::LANGUAGE.into()); } diff --git a/src/checker.rs b/src/checker.rs index ade91bea9..2c2f4bcd4 100644 --- a/src/checker.rs +++ b/src/checker.rs @@ -1426,6 +1426,196 @@ mod tests { assert!(GroovyCode::is_call(&chain), "command_chain must be a call"); } + // ===== C-family `is_call` regression tests (issue #1254) ===== + + // Every C-family grammar here references `preproc_call_expression` + // only under an `alias(…, $.call_expression)` — zero bare references + // in `grammar.json`, and no entry in `node-types.json` — so the + // unsuffixed `CallExpression` never reaches `kind_id()` while the + // aliased `CallExpression2` carries every real call. + // + // The two halves do different jobs. The first keeps the fixture an + // honest witness: a grammar bump that stops emitting the alias must + // fail here rather than silently weaken the counts below. The second + // cannot fail against the current parse tables — `ts_symbol_map` + // rewrites the unsuffixed symbol onto the aliased one for *every* + // input — so it is a drift marker, not a live guard: it fires only if + // a bump renumbers the enum or stops aliasing the rule, which is + // exactly what justifies keeping the unsuffixed arm defensively + // (the grammar-dispatch §2 technique, applied to an alias-mapped + // rule rather than a hidden one). + #[track_caller] + fn assert_call_kinds_are_aliased( + parser: &P, + lang: &str, + aliased: u16, + unsuffixed: u16, + ) { + assert!( + ast_has_kind_id(parser, aliased), + "{lang}: fixture must emit the aliased call_expression kind" + ); + assert!( + !ast_has_kind_id(parser, unsuffixed), + "{lang}: the unsuffixed CallExpression is the aliased-away \ + preproc_call_expression and must not surface" + ); + } + + // Six calls — statement position, initializer, `if` condition, both + // binary operands, and the `return` operand. The other parenthesised + // shapes are deliberate negatives: the `int f(int);` prototype's + // `function_declarator`, the `sizeof(int)` operand, and the `(int)x` + // cast. Six differs from the C++ fixture's five, so a test that + // reaches for the wrong parser cannot pass on the right number. + const C_CALL_SHAPES: &str = "int f(int);\n\ + int main(void) {\n\ + f(1);\n\ + int x = g(2);\n\ + if (h(3)) { }\n\ + int y = a(4) + b(5);\n\ + int z = sizeof(int) + (int)x + y;\n\ + return c(z);\n\ + }\n"; + + // Five calls — member (`t.m()`), arrow member (`p->m()`), qualified + // (`ns::f()`), and both function-pointer spellings (`fp(2)`, + // `(*fp)(3)`). `T t(1)` and `new T(4)` are the negatives that matter: + // object creation is an ABC concern, not a call site, matching Groovy / + // Java / C# (#430). Functional-style `T(4)` is deliberately absent + // from the fixture — the grammar emits it as a plain `call_expression`, + // so it *is* counted, and including it here would blur what the five + // is pinning. + const CPP_CALL_SHAPES: &str = "struct T { T(int); int m(); };\n\ + namespace ns { int f(); }\n\ + int main() {\n\ + T t(1);\n\ + t.m();\n\ + T *p = &t;\n\ + p->m();\n\ + ns::f();\n\ + int (*fp)(int) = nullptr;\n\ + fp(2);\n\ + (*fp)(3);\n\ + T *q = new T(4);\n\ + delete q;\n\ + return sizeof(T);\n\ + }\n"; + + /// Every `call_expression` the pinned C grammar emits carries the + /// *aliased* `CallExpression2` kind_id. The unsuffixed variant is the + /// grammar's `preproc_call_expression`, referenced only under an + /// `alias(…, $.call_expression)` — it is absent from `node-types.json` + /// and never reaches `kind_id()`. Matching it alone left `is_call` + /// dead: `bca count -t call` reported 0 on ordinary C (#1254). + #[test] + fn c_is_call_matches_the_aliased_call_expression() { + let parser = CParser::new( + C_CALL_SHAPES.as_bytes().to_vec(), + &PathBuf::from("test.c"), + None, + ); + + assert_call_kinds_are_aliased( + &parser, + "C", + C::CallExpression2 as u16, + C::CallExpression as u16, + ); + + assert_eq!( + count(&parser, &["call".to_string()]).0, + 6, + "is_call must match every call_expression the C grammar emits" + ); + + // Parenthesised non-call shapes stay out. The exact count above is + // the sharper guard — these rows document the intended boundary + // and catch a future edit that widens the match to them. + for (kind, label) in [ + (C::SizeofExpression as u16, "sizeof_expression"), + (C::CastExpression as u16, "cast_expression"), + (C::FunctionDeclarator as u16, "function_declarator"), + ] { + let node = find_first_kind(&parser, kind).expect(label); + assert!(!CCode::is_call(&node), "{label} must not be a call"); + } + } + + /// C++ counterpart of [`c_is_call_matches_the_aliased_call_expression`]. + /// Member, qualified and function-pointer calls all arrive as the same + /// aliased `CallExpression2`; object construction does not (#1254). + #[test] + fn cpp_is_call_matches_the_aliased_call_expression() { + let parser = CppParser::new( + CPP_CALL_SHAPES.as_bytes().to_vec(), + &PathBuf::from("test.cpp"), + None, + ); + + assert_call_kinds_are_aliased( + &parser, + "Cpp", + Cpp::CallExpression2 as u16, + Cpp::CallExpression as u16, + ); + + assert_eq!( + count(&parser, &["call".to_string()]).0, + 5, + "is_call must match member, qualified and function-pointer \ + calls, and only those" + ); + + // `new T(4)` and `sizeof(T)` are call-shaped but not call sites. + for (kind, label) in [ + (Cpp::NewExpression as u16, "new_expression"), + (Cpp::SizeofExpression as u16, "sizeof_expression"), + (Cpp::InitDeclarator as u16, "init_declarator"), + ] { + let node = find_first_kind(&parser, kind).expect(label); + assert!(!CppCode::is_call(&node), "{label} must not be a call"); + } + } + + /// `Mozcpp` owns no file extension, so nothing routes to it and it + /// gets no integration-snapshot coverage. Pin it against its + /// extension-owning sibling instead: the Mozilla fork inherits + /// upstream C++'s aliasing, so the same source must yield the same + /// calls (grammar-dispatch, "sweep the rest"). + #[test] + fn mozcpp_is_call_agrees_with_cpp() { + let source = CPP_CALL_SHAPES.as_bytes().to_vec(); + let mozcpp = MozcppParser::new(source.clone(), &PathBuf::from("test.cpp"), None); + let cpp = CppParser::new(source, &PathBuf::from("test.cpp"), None); + + assert_call_kinds_are_aliased( + &mozcpp, + "Mozcpp", + Mozcpp::CallExpression2 as u16, + Mozcpp::CallExpression as u16, + ); + + let mozcpp_calls = count(&mozcpp, &["call".to_string()]).0; + assert_eq!( + mozcpp_calls, 5, + "Mozcpp must count the same five calls as C++" + ); + assert_eq!( + mozcpp_calls, + count(&cpp, &["call".to_string()]).0, + "Mozcpp is the Mozilla C++ fork; is_call must not diverge \ + from CppCode on shared source" + ); + + let new_expr = + find_first_kind(&mozcpp, Mozcpp::NewExpression as u16).expect("new_expression"); + assert!( + !MozcppCode::is_call(&new_expr), + "new_expression must not be a call" + ); + } + fn parse_python(src: &str) -> PythonParser { PythonParser::new(src.as_bytes().to_vec(), &PathBuf::from("test.py"), None) } diff --git a/src/checker/c.rs b/src/checker/c.rs index f376602f1..171625867 100644 --- a/src/checker/c.rs +++ b/src/checker/c.rs @@ -40,8 +40,22 @@ impl Checker for CCode { false } + // Every `call_expression` the pinned grammar emits carries the + // aliased `CallExpression2` kind_id: the unsuffixed symbol is the + // grammar's `preproc_call_expression`, which is only ever referenced + // under an `alias(…, $.call_expression)`, so it never reaches + // `kind_id()` (absent from `node-types.json` too). Matching the + // unsuffixed variant alone left this predicate dead — `bca count -t + // call` reported 0 on ordinary C (#1254). Keep it listed defensively: + // it is the symbol a grammar bump is most likely to start emitting + // (grammar-dispatch §1). Note this is an alias-mapped *visible* rule, + // not the `_`-prefixed hidden rule of §2 / lesson #34 — different + // mechanism, same symptom, same defensive treatment. fn is_call(node: &Node) -> bool { - node.kind_id() == C::CallExpression + matches!( + node.kind_id().into(), + C::CallExpression | C::CallExpression2 + ) } // C's `(void)` marker, shared with C++, Mozcpp and Objective-C. diff --git a/src/checker/cpp.rs b/src/checker/cpp.rs index 38a9cac25..cfbbe3e73 100644 --- a/src/checker/cpp.rs +++ b/src/checker/cpp.rs @@ -51,8 +51,24 @@ impl Checker for CppCode { node.kind_id() == Cpp::LambdaExpression } + // See `CCode::is_call` (#1254): the unsuffixed variant is the + // grammar's always-aliased `preproc_call_expression` and never + // reaches `kind_id()`, so every real call — free, member (`o.m()` / + // `p->m()`), qualified (`ns::f()`) and through a function pointer — + // arrives as `CallExpression2`. Kept listed defensively. + // + // `new_expression` is deliberately not listed: object creation is an + // ABC concern rather than a call site, matching Groovy / Java / C# + // (#430) — note the ABC walker in `src/metrics/abc/cpp.rs` *does* + // count it. This excludes `new T(4)` and declaration-position + // `T t(1)` only. Functional-style construction (`return T(4)`) is + // still counted, because tree-sitter-cpp emits it as an ordinary + // `call_expression` and nothing in the tree distinguishes it. fn is_call(node: &Node) -> bool { - node.kind_id() == Cpp::CallExpression + matches!( + node.kind_id().into(), + Cpp::CallExpression | Cpp::CallExpression2 + ) } // C's `(void)` marker, shared with C++, Mozcpp and Objective-C. diff --git a/src/checker/mozcpp.rs b/src/checker/mozcpp.rs index ef7259479..858b2bd6f 100644 --- a/src/checker/mozcpp.rs +++ b/src/checker/mozcpp.rs @@ -51,8 +51,16 @@ impl Checker for MozcppCode { node.kind_id() == Mozcpp::LambdaExpression } + // See `CCode::is_call` (#1254). The Mozilla fork inherits upstream + // C++'s always-aliased `preproc_call_expression`, so the unsuffixed + // variant never reaches `kind_id()` here either. Kept listed + // defensively; `new_expression` deliberately not listed, with the same + // scope and the same functional-style caveat as `CppCode::is_call`. fn is_call(node: &Node) -> bool { - node.kind_id() == Mozcpp::CallExpression + matches!( + node.kind_id().into(), + Mozcpp::CallExpression | Mozcpp::CallExpression2 + ) } // C's `(void)` marker, shared with C++, Mozcpp and Objective-C. diff --git a/src/getter/csharp.rs b/src/getter/csharp.rs index 90f20cc5e..08ee3b533 100644 --- a/src/getter/csharp.rs +++ b/src/getter/csharp.rs @@ -45,7 +45,7 @@ impl Getter for CsharpCode { } } - fn get_op_type<'a>(node: &Node<'a>, _ancestors: Ancestors<'a, '_>) -> HalsteadType { + fn get_op_type<'a>(node: &Node<'a>, ancestors: Ancestors<'a, '_>) -> HalsteadType { use Csharp::*; match node.kind_id().into() { @@ -79,9 +79,26 @@ impl Getter for CsharpCode { // Predefined / primitive types | PredefinedType => HalsteadType::Operator, - // Operands: identifiers and literals. + // `boolean_literal: choice('true', 'false')` wraps the + // keyword leaf, so a literal reaches the walker twice; + // listing both kinds inflated `N2` by one per literal while + // the text-keyed `n2` hid it (#1253). The leaf cannot simply + // be dropped: the overloadable-operator list emits a bare + // `true` / `false` under `operator_declaration` — the + // grammar's only unwrapped position — and suppressing that + // would leave two such declarations with identical Halstead + // vocabularies whenever their bodies match. That position + // keeps its pre-#1253 `Operand` classification here; whether + // an overloaded operator's *name* is better counted as an + // operator, as `operator +` already is, is #1296. + True | False => match ancestors.parent(node).map(|p| p.kind_id().into()) { + Some(BooleanLiteral) => HalsteadType::Unknown, + _ => HalsteadType::Operand, + }, + // Operands: identifiers and literals. `NullLiteral` is a + // childless leaf, so it needs no such guard. Identifier | GenericName | QualifiedName | AliasQualifiedName - | IntegerLiteral | RealLiteral | BooleanLiteral | NullLiteral | True | False + | IntegerLiteral | RealLiteral | BooleanLiteral | NullLiteral | CharacterLiteral | StringLiteral | VerbatimStringLiteral | RawStringLiteral => HalsteadType::Operand, // `$"..."` counts as one operand when inert. When it carries diff --git a/src/getter/elixir.rs b/src/getter/elixir.rs index 6659f5275..60a633b07 100644 --- a/src/getter/elixir.rs +++ b/src/getter/elixir.rs @@ -206,13 +206,23 @@ impl Getter for ElixirCode { // (`~r`, the trailing `i`/`u` flags) stay as operands even // for interpolated sigils — they are distinct tokens with // their own text. + // + // `boolean: choice("true", "false")` and `nil: "nil"` each + // wrap a keyword leaf, so a literal reaches the walker + // twice; listing the wrapper and the leaf both inflated `N2` + // by one per literal while the text-keyed `n2` hid it + // (#1253). Only the wrappers count, mirroring Ruby's `Nil` / + // `Nil2` split. Unlike C#, the leaves need no parent guard: + // those two rules are the grammar's only producers of them, + // and the one other position accepting the reserved words — + // the right-hand side of a remote dot, `Foo.nil` — aliases + // them to `identifier`. E::Identifier | E::Alias | E::OperatorIdentifier | E::SigilName | E::SigilName2 | E::SigilModifiers | E::Keyword | E::Keyword2 | E::QuotedKeyword | E::Integer | E::Float | E::Char | E::Atom | E::Atom2 | E::QuotedAtom - | E::Boolean | E::True | E::False - | E::Nil | E::Nil2 + | E::Boolean | E::Nil => HalsteadType::Operand, _ => HalsteadType::Unknown, diff --git a/src/getter/php.rs b/src/getter/php.rs index 4866a63de..86df40cd0 100644 --- a/src/getter/php.rs +++ b/src/getter/php.rs @@ -28,7 +28,7 @@ impl Getter for PhpCode { } } - fn get_op_type<'a>(node: &Node<'a>, _ancestors: Ancestors<'a, '_>) -> HalsteadType { + fn get_op_type<'a>(node: &Node<'a>, ancestors: Ancestors<'a, '_>) -> HalsteadType { use Php::*; match node.kind_id().into() { // Operator: control-flow keywords @@ -89,12 +89,56 @@ impl Getter for PhpCode { | DOT => HalsteadType::Operator, - // Operands: identifiers and literals. + // `name` is a genuine operand almost everywhere — function, + // class, const, member (`->prop`) and namespace-component + // identifiers are all this kind. But it is ALSO the identifier + // leaf of every variable reference: `$x` parses as a + // `variable_name` wrapping `name`, and `"${x}"` as a + // `dynamic_variable_name` wrapping it directly. Both wrappers + // are operands in their own right below (keyed with the `$` + // sigil, the way Bash and iRules key theirs), so counting the + // leaf too double-counts every variable reference — inflating + // N2 and adding a spurious sigil-less twin to n2 (#1259). + // Exclude it exactly in those two positions, the same + // parent-scoped guard Bash applies to `variable_name` under + // `simple_expansion` and iRules to `Id` under + // `variable_substitution`. `Name2` is the hidden `_name` + // supertype the parser never emits — pinned by + // `php_name2_hidden_rule_drift_marker` in `src/metrics/abc.rs`. + // It rides along defensively so a grammar bump that promotes + // it inherits the guard rather than the bug. + Name | Name2 => match ancestors.parent(node).map(|p| p.kind_id().into()) { + Some(VariableName | DynamicVariableName) => HalsteadType::Unknown, + _ => HalsteadType::Operand, + }, + + // Variable-variable syntax nests these wrappers: `$$x` is a + // `dynamic_variable_name` around a `variable_name`, `${$x}` + // the same, and `$$$x` a `dynamic_variable_name` around + // another `dynamic_variable_name`. Each level's text span + // contains the one below it, so only the outermost may count; + // suppressing both kinds under a `dynamic_variable_name` + // parent handles arbitrary depth (#1259). An inner reference + // reached through a real expression (`${$x . 'y'}`, whose + // `$x` hangs off a `binary_expression`) is unaffected. + VariableName | DynamicVariableName => { + match ancestors.parent(node).map(|p| p.kind_id().into()) { + Some(DynamicVariableName) => HalsteadType::Unknown, + _ => HalsteadType::Operand, + } + } + + // Operands: literals, type expressions, and the compound + // name forms. FIXME(#1293): the type and name wrappers here + // still double-count the leaves they contain — `?int` scores + // 3 (`optional_type` + `primitive_type` + the `int` token) + // and `Foo\Bar\Baz` scores 5. Same class as the guards + // above, but which node should carry the operand is a + // per-construct call, so it is tracked apart from #1259. // `String`/`String2`/`String3` (single-quoted) and // `Nowdoc` never interpolate and are always counted as // one operand each. - Name | Name2 | VariableName | DynamicVariableName - | Integer | Float | Float2 + Integer | Float | Float2 | String | String2 | String3 | Nowdoc | Boolean | Null | Null2 diff --git a/src/metrics/abc.rs b/src/metrics/abc.rs index 3c25e11ea..e9a6e3ada 100644 --- a/src/metrics/abc.rs +++ b/src/metrics/abc.rs @@ -456,6 +456,20 @@ mod tests { // the same space), so the func-space shim carries Cyclomatic too. check_func_space_only_shim!(check_func_space, Abc, Cyclomatic); + // Recurse to the innermost function space and assert the invariant + // named above there. `conditions()` is that one space's own count, + // so it pairs with `cyclomatic()` and never `cyclomatic_sum()`, + // which folds in a base of 1 per nested space. + fn assert_deepest_conditions_match_cyclomatic(space: &crate::FuncSpace, expected: u64) { + let mut deepest = space; + while let Some(child) = deepest.spaces.last() { + deepest = child; + } + let decisions = deepest.metrics.cyclomatic.cyclomatic() - 1; + assert_eq!(decisions, expected); + assert_eq!(deepest.metrics.abc.conditions(), decisions); + } + // Walk the AST and return true iff any node has `kind_id == target`. // Used as a drift marker for hidden-rule kind ids: a passing // `!ast_has_kind_id(...)` assertion proves the kind is unreachable @@ -1395,6 +1409,94 @@ mod tests { ); } + // Issue #1274: a generic *declaration* is type syntax, not a + // decision. `java_count_token_condition` denied only + // `type_arguments`, so `class Gen` and ` U ident(U x)` — both + // `type_parameters` — each scored two conditions. The fix counts + // `<` / `>` only under `binary_expression`, the polarity C / C++ / + // Rust / Go already use; a `grammar.json` sweep proves the token is + // emitted from exactly three productions, so the two shapes agree on + // every well-formed input. + // + // One fixture per production the arm must ignore: the class-level + // `type_parameters` (`Gen`), its `extends` bound's nested + // `type_arguments` (`Comparable`), the method-level + // `type_parameters` (``), and a nested generic + // (`Map>`) whose two closing `>` lex as separate + // tokens under separate `type_arguments`. Pre-fix this file scored + // 4 conditions — two per `type_parameters` bracket pair; expected 0, + // the file contains no conditional construct at all. + #[test] + fn java_generic_declarations_are_not_conditions() { + check_metrics::( + "class Gen> { + U ident(U x) { return x; } + List pick(Map> m) { return m.get(\"k\"); } + }", + "foo.java", + |metric| { + assert_eq!(metric.abc.conditions_sum(), 0); + // Non-vacuity guard: 0 is also what an unparsed file + // scores, so pin a value only a walked body can produce + // — the `m.get("k")` invocation. + assert_eq!(metric.abc.branches_sum(), 1); + }, + ); + } + + // Second half of #1274, same premise and same match: a wildcard type + // argument's `?` is type syntax, not a ternary. tree-sitter-java emits + // a bare `?` from exactly two productions — `ternary_expression` and + // `wildcard` — so the `QMARK` arm carries the same allowlist gate the + // `<` / `>` arm does. + // + // The body carries a real ternary over a real comparison so the + // expected total is 2, not 0. Asserting 0 on a wildcard-only fixture + // would have been vacuous — an unparsable file scores 0 too, so the + // test could not tell "the wildcard was ignored" from "the fixture + // stopped being recognised". Each failure mode now lands on its own + // number: 4 if the wildcard `?` counts again (the pre-fix value), 3 + // if the gate is aimed at `Wildcard` instead, 1 if it swallows the + // genuine ternary, 0 if parsing breaks. Two wildcards against one + // ternary is deliberate — with one of each, aiming the gate at the + // wrong one of the two productions still totals 2 and the test + // cannot see the difference. + #[test] + fn java_generic_wildcard_is_not_a_condition() { + check_metrics::( + "class A { + int m(List xs, Map ys, + int a, int b) { return a < b ? 1 : 2; } + }", + "foo.java", + |metric| { + assert_eq!(metric.abc.conditions_sum(), 2); + }, + ); + } + + // The other half of #1274: narrowing the `<` / `>` arm must not + // swallow real comparisons. `List` in the signature keeps a + // generic in scope so the fixture proves both directions at once, + // and the assertion is the grammar-dispatch §8 pin — on the + // method's own space the ABC condition count must equal the + // cyclomatic decision count (`cyclomatic()` minus the per-space + // base of 1). Both are 2, one per `if`. + #[test] + fn java_comparison_operators_still_count_alongside_generics() { + check_func_space::( + "class A { + int m(List xs, int a, int b) { + if (a < b) { return 1; } + if (a > b) { return 2; } + return 0; + } + }", + "foo.java", + |space| assert_deepest_conditions_match_cyclomatic(&space, 2), + ); + } + #[test] fn groovy_no_abc() { // Comment-only file has no executable code → all-zero ABC. @@ -1590,6 +1692,89 @@ mod tests { ); } + // Groovy half of #1274 — see `java_generic_declarations_are_not_conditions`. + // The class-level `type_parameters` and its bound's nested + // `type_arguments` mirror the Java fixture; pre-fix this file scored + // 2 conditions, expected 0. + #[test] + fn groovy_generic_declarations_are_not_conditions() { + check_metrics::( + "class Gen> { + List pick(Map> m) { m.get(\"k\") } + }", + "foo.groovy", + |metric| { + assert_eq!(metric.abc.conditions_sum(), 0); + // Non-vacuity guard — see the Java counterpart. + assert_eq!(metric.abc.branches_sum(), 1); + }, + ); + } + + // Groovy counterpart of `java_generic_wildcard_is_not_a_condition`, + // including its choice of a non-zero expected total and its + // two-wildcards-one-ternary shape; the dekobon grammar emits the + // same `wildcard` node. Pre-fix: 4. + #[test] + fn groovy_generic_wildcard_is_not_a_condition() { + check_metrics::( + "class A { + int m(List xs, Map ys, + int a, int b) { a < b ? 1 : 2 } + }", + "foo.groovy", + |metric| { + assert_eq!(metric.abc.conditions_sum(), 2); + }, + ); + } + + // A generic *method* gets its own fixture because the dekobon + // grammar gives it its own production: `def U ident(U x)` emits + // `method_type_parameters`, not the `type_parameters` the class form + // above uses. It is the fourth and last production from which that + // grammar emits a bare `<` / `>` (the others being + // `binary_expression`, `type_arguments` and `type_parameters`), and + // the one a denylist extended only with `TypeParameters` — the fix + // this issue's own plan proposed — would still miss. Revert-verified + // in both directions: under that denylist this test still fails + // while `groovy_generic_declarations_are_not_conditions` passes. + // + // The body carries a ternary over a comparison for the same + // non-vacuity reason as the wildcard tests: expected 2, pre-fix 4 + // (the `` bracket pair), 0 if the fixture stops parsing. + #[test] + fn groovy_method_type_parameters_are_not_conditions() { + check_metrics::( + "class A { + def U ident(U x, int a, int b) { a < b ? x : null } + }", + "foo.groovy", + |metric| { + assert_eq!(metric.abc.conditions_sum(), 2); + }, + ); + } + + // Groovy counterpart of + // `java_comparison_operators_still_count_alongside_generics`: the + // narrowed arm keeps counting real comparisons, pinned against the + // cyclomatic decision count on the method's own space (§8). + #[test] + fn groovy_comparison_operators_still_count_alongside_generics() { + check_func_space::( + "class A { + int m(List xs, int a, int b) { + if (a < b) { return 1 } + if (a > b) { return 2 } + return 0 + } + }", + "foo.groovy", + |space| assert_deepest_conditions_match_cyclomatic(&space, 2), + ); + } + #[test] fn groovy_if_multiple_conditions() { // Mirrors `java_if_multiple_conditions`: `&&` / `||` chains @@ -2361,19 +2546,6 @@ mod tests { // #469 ABC `conditions()` was 3 here while cyclomatic stayed at 2. #[test] fn java_csharp_cpp_switch_default_cyclomatic_parity() { - // Recurse to the deepest function space (the method holding the - // switch) and assert per-space cyclomatic decisions == ABC - // conditions. - fn assert_deepest(space: &crate::FuncSpace) { - if let Some(child) = space.spaces.last() { - assert_deepest(child); - return; - } - // Per-space cyclomatic base is 1; the two case arms add 2. - let decisions = space.metrics.cyclomatic.cyclomatic() - 1; - assert_eq!(decisions, 2); - assert_eq!(space.metrics.abc.conditions(), decisions); - } check_func_space::( "class A { int m(int x) { @@ -2381,7 +2553,7 @@ mod tests { } }", "foo.java", - |space| assert_deepest(&space), + |space| assert_deepest_conditions_match_cyclomatic(&space, 2), ); check_func_space::( "class A { @@ -2390,14 +2562,14 @@ mod tests { } }", "foo.cs", - |space| assert_deepest(&space), + |space| assert_deepest_conditions_match_cyclomatic(&space, 2), ); check_func_space::( "void f(int x) { switch (x) { case 1: return; case 2: return; default: return; } }", "foo.cpp", - |space| assert_deepest(&space), + |space| assert_deepest_conditions_match_cyclomatic(&space, 2), ); } @@ -2420,18 +2592,7 @@ mod tests { } }", "foo.php", - |space| { - fn assert_deepest(space: &crate::FuncSpace) { - if let Some(child) = space.spaces.last() { - assert_deepest(child); - return; - } - let decisions = space.metrics.cyclomatic.cyclomatic() - 1; - assert_eq!(decisions, 2); - assert_eq!(space.metrics.abc.conditions(), decisions); - } - assert_deepest(&space); - }, + |space| assert_deepest_conditions_match_cyclomatic(&space, 2), ); } @@ -2453,18 +2614,7 @@ mod tests { }; }", "foo.php", - |space| { - fn assert_deepest(space: &crate::FuncSpace) { - if let Some(child) = space.spaces.last() { - assert_deepest(child); - return; - } - let decisions = space.metrics.cyclomatic.cyclomatic() - 1; - assert_eq!(decisions, 2); - assert_eq!(space.metrics.abc.conditions(), decisions); - } - assert_deepest(&space); - }, + |space| assert_deepest_conditions_match_cyclomatic(&space, 2), ); } diff --git a/src/metrics/abc/groovy.rs b/src/metrics/abc/groovy.rs index 5a266dd5b..0b0dd18ae 100644 --- a/src/metrics/abc/groovy.rs +++ b/src/metrics/abc/groovy.rs @@ -164,13 +164,40 @@ fn groovy_count_token_condition<'a>( ) -> bool { use Groovy::*; match node.kind_id().into() { - GTEQ | LTEQ | EQEQ | BANGEQ | Else | Case | QMARK | Try | Catch => { + GTEQ | LTEQ | EQEQ | BANGEQ | Else | Case | Try | Catch => { stats.conditions += 1.; } - // Excludes `<` / `>` used for generic types (e.g. `List`). + // As in Java: a bare `?` is either a ternary head or the head of + // a `wildcard` type argument (`List`), and only the + // first is a decision (#1274). + QMARK => { + if ancestors + .parent(node) + .is_some_and(|parent| matches!(parent.kind_id().into(), TernaryExpression)) + { + stats.conditions += 1.; + } + } + // Counts `<` / `>` only as the operator token of a + // `binary_expression` — see `java_count_token_condition` for + // the polarity rationale. The dekobon Groovy grammar emits a + // bare `<` / `>` from four productions: `binary_expression`, + // `type_arguments`, `type_parameters`, and — unlike + // tree-sitter-java — a separate `method_type_parameters` for + // `def U m(U x)`. The previous denylist named only + // `type_arguments`, leaving both generic-declaration forms + // worth two conditions (#1274). The positive form also copes + // with a construct this grammar cannot parse: an explicit type + // witness (`Collections.emptyList()`) puts its `<` + // under an `ERROR` node that no denylist can name, so only the + // trailing `>` — which error recovery does hang off a + // `binary_expression` — is still counted. Groovy's `<=>` is a + // `spaceship_expression` carrying its own token, so it never + // reaches this arm. GT | LT => { - if let Some(parent) = ancestors.parent(node) - && !matches!(parent.kind_id().into(), TypeArguments) + if ancestors + .parent(node) + .is_some_and(|parent| matches!(parent.kind_id().into(), BinaryExpression)) { stats.conditions += 1.; } diff --git a/src/metrics/abc/java.rs b/src/metrics/abc/java.rs index 215a2d69d..b32d676bd 100644 --- a/src/metrics/abc/java.rs +++ b/src/metrics/abc/java.rs @@ -179,7 +179,9 @@ fn java_count_token_branch(node: &Node, stats: &mut Stats) -> bool { } // Counts condition tokens: comparison operators, control-flow keywords, -// and `<` / `>` outside generic-type contexts. The `default` arm of a +// and the two tokens Java's generic syntax shares with an operator — +// `<` / `>` count only in comparison position and `?` only as a ternary +// head, each gated on its parent kind. The `default` arm of a // `switch` is excluded: it is the unconditional fallthrough, so // cyclomatic counts only the `Case` arms (issue #469). Java's classic // statement switch (`default:`) and arrow switch (`default ->`) both @@ -192,13 +194,40 @@ fn java_count_token_condition<'a>( ) -> bool { use Java::*; match node.kind_id().into() { - GTEQ | LTEQ | EQEQ | BANGEQ | Else | Case | QMARK | Try | Catch => { + GTEQ | LTEQ | EQEQ | BANGEQ | Else | Case | Try | Catch => { stats.conditions += 1.; } - // Excludes `<` / `>` used for generic types (`Box`). + // `?` opens a ternary, but tree-sitter-java also emits it bare + // as the head of a `wildcard` type argument + // (`List`), which is type syntax and no more a + // decision than the `<` / `>` around it (#1274). Those two + // productions are the only ones that emit a bare `?`, so the + // same allowlist polarity used below settles it. + QMARK => { + if ancestors + .parent(node) + .is_some_and(|parent| matches!(parent.kind_id().into(), TernaryExpression)) + { + stats.conditions += 1.; + } + } + // Counts `<` / `>` only as the operator token of a + // `binary_expression` — the polarity C / C++ / Rust / Go use. + // tree-sitter-java emits a bare `<` / `>` from exactly three + // productions (`binary_expression`, `type_arguments`, + // `type_parameters`), so this is the inverse of denying the two + // generic-type contexts, and it does not have to be revisited + // when a grammar bump adds a fourth type-syntax one. The + // previous denylist named only `type_arguments`, leaving every + // generic *declaration* — `class Gen`, ` void m()`, both + // `type_parameters` — worth two conditions (#1274). A nested + // generic (`Map>`) closes with two separate `>` + // tokens under their own `type_arguments`, not one `>>`; the + // shifts are distinct tokens and never reach this arm. GT | LT => { - if let Some(parent) = ancestors.parent(node) - && !matches!(parent.kind_id().into(), TypeArguments) + if ancestors + .parent(node) + .is_some_and(|parent| matches!(parent.kind_id().into(), BinaryExpression)) { stats.conditions += 1.; } diff --git a/src/metrics/halstead.rs b/src/metrics/halstead.rs index d057db971..376de40fb 100644 --- a/src/metrics/halstead.rs +++ b/src/metrics/halstead.rs @@ -2065,6 +2065,15 @@ mod tests { // to 14 (5 non-primitive operators + 9 distinct primitives); // total operators (N1) is unchanged because the same nodes are // still counted, just keyed by lexeme. + // + // N2 dropped 23 → 21 with issue #1253: `true` and `false` each + // reached the walker twice — once as `boolean_literal`, once as + // the keyword leaf under it — so each added one spurious + // occurrence. n2 is unchanged at 21 because operands are keyed + // by source text, so the duplicate collapsed into the existing + // vocabulary entry; that is exactly why the inflation was + // invisible in n2. Every operand here is distinct, so + // N2 == n2 == 21 after the fix. check_metrics::( "public class Prims { byte a = 1; @@ -2083,12 +2092,68 @@ mod tests { assert_eq!(metric.halstead.unique_operators(), 14); assert_eq!(metric.halstead.total_operators(), 33); assert_eq!(metric.halstead.unique_operands(), 21); - assert_eq!(metric.halstead.total_operands(), 23); + assert_eq!(metric.halstead.total_operands(), 21); insta::assert_json_snapshot!(metric.halstead); }, ); } + #[test] + fn csharp_boolean_literal_counts_once() { + // Regression: issue #1253. `boolean_literal: choice('true', + // 'false')` wraps the keyword leaf, and both kinds sat in the + // operand arm, so every `true` / `false` occurrence added +1 to + // N2. Operands are keyed by source text, so the duplicate + // collapsed into the same vocabulary entry and n2 stayed + // correct — which is why nothing caught it. + // + // Source repeats `true` so N2 exceeds n2 and the assertions can + // tell "counted once per occurrence" from "deduplicated into + // the vocabulary". + // + // Operands by text key: `A`, `M`, `a`, `b`, `c`, `d`, `true` × 2, + // `false`, `null` ⇒ n2 = 9, N2 = 10. Before the fix the keyword + // leaves added one occurrence per boolean ⇒ N2 = 13. + check_metrics::( + "class A {\n void M() {\n bool a = true;\n bool b = false;\n bool c = true;\n object d = null;\n }\n}\n", + "foo.cs", + |metric| { + assert_eq!(metric.halstead.unique_operands(), 9); + assert_eq!(metric.halstead.total_operands(), 10); + }, + ); + } + + #[test] + fn csharp_boolean_keyword_outside_a_literal_still_counts() { + // Companion to the test above (#1253): the suppression fires on + // the *parent* kind, never on `True` / `False` alone. C#'s + // overloadable-operator list emits a bare `true` / `false` token + // with no `boolean_literal` wrapper — `operator_declaration` is + // the grammar's only such position — so a blanket exclusion + // would drop the operand that is the sole difference between + // `operator true` and `operator false`, leaving two such + // declarations with identical Halstead vocabularies whenever + // their bodies match. + // + // Each declaration names one boolean and returns the other, so + // the fixture exercises both the guarded and the unguarded + // position for each keyword. + // + // Operands: `A` × 3 (class name, two parameter types), `a` × 2, + // `true` × 2 (operator name + literal), `false` × 2 (likewise) + // ⇒ n2 = 4, N2 = 9. A blanket exclusion gives N2 = 7; no guard + // at all restores the double count at N2 = 11. + check_metrics::( + "class A {\n public static bool operator true(A a) => false;\n public static bool operator false(A a) => true;\n}\n", + "foo.cs", + |metric| { + assert_eq!(metric.halstead.unique_operands(), 4); + assert_eq!(metric.halstead.total_operands(), 9); + }, + ); + } + #[test] fn csharp_predefined_types_keyed_by_lexeme() { // Regression: issue #286. The C# grammar emits one `PredefinedType` @@ -3059,6 +3124,65 @@ f() { }); } + #[test] + fn elixir_boolean_and_nil_literals_count_once() { + // Regression: issue #1253. `boolean: choice("true", "false")` + // and `nil: "nil"` each wrap a keyword leaf, and both the + // wrapper and the leaf sat in the operand arm — so every + // literal occurrence added +1 to N2. Operands are keyed by + // source text, so the duplicate collapsed into the same + // vocabulary entry and n2 stayed correct, which is why nothing + // caught it. + // + // Source is the issue's reproducer plus a repeat of `true` and + // `nil`, so N2 exceeds n2 and the assertions can tell "counted + // once per occurrence" from "deduplicated into the vocabulary". + // All three keywords appear, so restoring any one of `True`, + // `False`, or `Nil2` to the operand arm trips this test. + // + // Operands by text key: `x`, `y`, `z`, `w`, `v`, `true` × 2, + // `nil` × 2, `false` ⇒ n2 = 8, N2 = 10. Before the fix each of + // the five literals counted twice ⇒ N2 = 15. + // + // This also guards the drift in the other direction. Elixir + // classifies the wrapper and drops the leaf outright rather + // than parent-guarding it, so a grammar bump that stopped + // emitting `boolean` / `nil` would leave the leaves unclassified + // and the literals would vanish from N2 entirely (⇒ 5 / 5) + // rather than merely being miscounted. + check_metrics::( + "x = true\ny = nil\nz = false\nw = true\nv = nil\n", + "foo.ex", + |metric| { + assert_eq!(metric.halstead.unique_operands(), 8); + assert_eq!(metric.halstead.total_operands(), 10); + }, + ); + } + + #[test] + fn elixir_reserved_word_after_a_dot_stays_an_operand() { + // Companion to the test above (#1253). Elixir drops `True` / + // `False` / `Nil2` from the operand arm outright, which is only + // safe because the one grammar position that accepts a reserved + // word outside the `boolean` / `nil` wrapper — the right-hand + // side of a remote dot — aliases it to `identifier`. This pins + // that alias: if a grammar bump emitted the bare keyword there + // instead, `Foo.nil` and `Foo.true` would silently stop + // contributing an operand. + // + // Source: a = Foo.nil / b = Foo.true / c = nil + // + // Operands by text key: `a`, `Foo` × 2, `nil` × 2 (the aliased + // identifier and the real literal, which share a text key), + // `b`, `true`, `c` ⇒ n2 = 6, N2 = 8. Losing the alias drops the + // two dotted references ⇒ N2 = 6. + check_metrics::("a = Foo.nil\nb = Foo.true\nc = nil\n", "foo.ex", |metric| { + assert_eq!(metric.halstead.unique_operands(), 6); + assert_eq!(metric.halstead.total_operands(), 8); + }); + } + #[test] fn elixir_interpolated_sigil_no_double_count() { // Sigils mirror strings under #180. For `~r/foo#{name}/`, the @@ -3329,11 +3453,18 @@ f() { // After #695 only the opening delimiters count: `()` and // `{}` fold to one operator each per balanced pair, so the // former `)`/`}` closers no longer inflate n1/N1 (was - // 11 unique / 15 total). Operands are unchanged. + // 11 unique / 15 total). + // + // Operands after #1259, tallied by `get_id` (source bytes): + // `avg` × 1, `int` × 8 (the `primitive_type` wrapper and + // its `int` keyword child, at all four type positions), + // `$a` / `$b` / `$c` × 2 each, `3` × 1 ⇒ n2 = 6, N2 = 16. + // Before the fix each `$v` also contributed its sigil-less + // `name` leaf ⇒ 9 / 22. assert_eq!(metric.halstead.unique_operators(), 9); assert_eq!(metric.halstead.total_operators(), 12); - assert_eq!(metric.halstead.unique_operands(), 9); - assert_eq!(metric.halstead.total_operands(), 22); + assert_eq!(metric.halstead.unique_operands(), 6); + assert_eq!(metric.halstead.total_operands(), 16); insta::assert_json_snapshot!(metric.halstead); }, ); @@ -3348,15 +3479,104 @@ f() { |metric| { // After #695 only opening delimiters count: the `)`/`}` // closers no longer add operators (was 9 unique / 9 total). + // + // Operands after #1259: `inc` × 1, `int` × 4 (the + // `primitive_type` wrapper plus its `int` keyword child, at + // both type positions), `$x` × 2, `1` × 1 ⇒ n2 = 4, N2 = 8. + // Before the fix `$x` also contributed its `x` leaf twice + // ⇒ 5 / 10. assert_eq!(metric.halstead.unique_operators(), 7); assert_eq!(metric.halstead.total_operators(), 7); - assert_eq!(metric.halstead.unique_operands(), 5); - assert_eq!(metric.halstead.total_operands(), 10); + assert_eq!(metric.halstead.unique_operands(), 4); + assert_eq!(metric.halstead.total_operands(), 8); insta::assert_json_snapshot!(metric.halstead); }, ); } + #[test] + fn php_variable_reference_counts_once() { + // Regression: issue #1259. `$x` parses as a `variable_name` + // wrapping a `name` leaf, and both kinds were in the operand + // arm — so every variable reference contributed twice to N2 and + // planted a sigil-less twin (`x` beside `$x`) in the n2 + // vocabulary. Since `$var` is the most common token class in + // PHP, that roughly doubled N2 for real files. + // + // Source: the issue's reproducer plus a re-reference of `$a` and + // `$b`, so N2 exceeds n2 and the assertions can tell "counted + // once per occurrence" from "deduplicated into the vocabulary". + // ( + "( + "( + "( "( "prop!"` text adds one - // more unique operand and one more occurrence ⇒ 6 / 11. + // more unique operand and one more occurrence ⇒ 5 / 8. check_metrics::( "prop = \"x\"; echo \"Hi $obj->prop!\";", "foo.php", |metric| { - assert_eq!(metric.halstead.unique_operands(), 5); - assert_eq!(metric.halstead.total_operands(), 10); + assert_eq!(metric.halstead.unique_operands(), 4); + assert_eq!(metric.halstead.total_operands(), 7); }, ); } @@ -3500,15 +3724,17 @@ f() { // ( "("( "(declaration: &Node<'a>) -> Option bool { +// +// A `function_definition` child is accepted outright rather than +// recursed into: it *is* a function, whatever declarator it carries. +// Recursing would miss a conversion operator, whose declarator is an +// `operator_cast` and not a `function_declarator` (#1258). +// +// The arm is inert at the `Npa` call sites, which only ever pass a +// `field_declaration`: per both grammars' `node-types.json`, neither +// `field_declaration` nor the `pointer_declarator` / +// `reference_declarator` wrappers this recurses through can carry a +// `function_definition` or a `declaration`, so the new arm is +// unreachable from there at any depth. It fires only for `Npm`'s +// `template_declaration`, whose child set does list `function_definition`. +pub(crate) fn cpp_declares_function(node: &Node) -> bool { node.children().any(|child| match child.kind() { - "function_declarator" => true, + "function_declarator" | "function_definition" => true, // Recurse through declarator wrappers that can sit above the // function_declarator (`Foo* operator->()`, // `template<...> T fn();`, constructor / destructor // `declaration`s inside a class body). "pointer_declarator" | "reference_declarator" | "declaration" => { - cpp_has_function_declarator(&child) + cpp_declares_function(&child) } _ => false, }) diff --git a/src/metrics/npm.rs b/src/metrics/npm.rs index f9383deb6..0203d8483 100644 --- a/src/metrics/npm.rs +++ b/src/metrics/npm.rs @@ -560,6 +560,12 @@ mod tests { check_metrics_only_shim!(check_metrics, Npm); check_func_space_only_shim!(check_func_space, Npm); + // `Npm` alongside the two metrics that count the same C++ members + // through independent walks: `Nom` (one per function space) and + // `Wmc` (each member's cyclomatic, rolled into the class). #1258 + // was invisible to the `Npm`-only shim precisely because nothing + // asserted the three agree. + check_metrics_only_shim!(check_metrics_with_nom_wmc, Npm, Nom, Wmc); #[test] fn java_constructors() { @@ -3453,6 +3459,160 @@ class C { ); } + // The C++ source shared by the `cpp_*` and `mozcpp_*` halves of the + // #1258 regression pair. `.mozcpp` owns no file extension, so the + // fork gets no integration-snapshot coverage and its clone of the + // `TemplateDeclaration` arm can only be pinned against its + // extension-owning sibling (grammar-dispatch, "sweep the rest"). + const TEMPLATE_METHOD_WITH_BODY: &str = "class C {\n\ + public:\n\ + template T get() { return T{}; }\n\ + int plain() { return 1; }\n\ + };"; + + // Every `template_declaration` payload the C++ grammar admits in + // class scope that is *not* a member function, per both grammars' + // `node-types.json`: a nested templated class (`type_specifier`), + // an `alias_declaration`, a templated static data member + // (`declaration` with no function declarator), and a + // `friend_declaration` — whose function is a free function the + // class merely grants access to, not a member of it. + // + // `real()` and `Nested::hidden()` are present so the expected + // totals are 2/1 rather than 0/0: a fixture that stopped parsing + // scores the default on every field, and an all-zero expectation + // cannot tell that apart from the payloads being correctly + // ignored. That the two differ also exercises the visibility flag, + // which an all-public fixture would leave pinned. + // + // `Nested` deliberately carries a *private* method rather than + // being empty. Its own class space contributes 1/0 to the subtree + // sums, so the expectation is 2/1 — and a helper that descended + // through `class_specifier` *and* `field_declaration_list` into the + // nested body would count the outer `template_declaration` as well + // and reach 3/2. (Both arms are needed to break it; adding either + // alone leaves the recursion one level short. Verified by + // perturbation.) An empty `Nested` would leave that descent + // untested in either direction. + const NON_METHOD_TEMPLATE_PAYLOADS: &str = "class C {\n\ + public:\n\ + template class Nested { void hidden() {} };\n\ + template using Alias = T;\n\ + template static T value;\n\ + template friend void amigo() {}\n\ + template T real() { return T{}; }\n\ + };"; + + #[test] + fn cpp_template_method_with_inline_body_counts() { + // A templated member *with a body* parses as + // `template_declaration > function_definition`, not the + // `template_declaration > declaration` shape that + // `cpp_template_methods_count` above pins. Before #1258 the + // guard could only reach a `function_declarator`, so `get()` + // scored zero: `npm` said the class had one method while `nom` + // opened two function spaces and `wmc` weighted two. + check_metrics_with_nom_wmc::(TEMPLATE_METHOD_WITH_BODY, "foo.cpp", |metric| { + assert_eq!(metric.npm.class_nm_sum(), 2); + assert_eq!(metric.npm.class_npm_sum(), 2); + // Both members carry a body, so all three walks must + // land on 2. Each method's cyclomatic is 1. + assert_eq!(metric.nom.functions_sum(), 2); + assert_eq!(metric.wmc.class_wmc_sum(), 2); + }); + } + + #[test] + fn cpp_template_method_with_inline_body_respects_visibility() { + // Deliberately asymmetric — 2 public, 1 private. A template arm + // that counted methods but ignored `current_is_public` lands on + // 3/3, and one that never reached the private section lands on + // 2/2; only the correct arm produces 3/2. + check_metrics_with_nom_wmc::( + "class C {\n\ + public:\n\ + template T a() { return T{}; }\n\ + template T b() { return T{}; }\n\ + private:\n\ + template T c() { return T{}; }\n\ + };", + "foo.cpp", + |metric| { + assert_eq!(metric.npm.class_nm_sum(), 3); + assert_eq!(metric.npm.class_npm_sum(), 2); + assert_eq!(metric.nom.functions_sum(), 3); + }, + ); + } + + #[test] + fn cpp_template_conversion_operator_with_body_counts() { + // A conversion operator's declarator is an `operator_cast`, so + // there is no `function_declarator` anywhere in this subtree. + // This is why `cpp_declares_function` accepts a + // `function_definition` child outright rather than recursing + // into it — the recursion the #1258 plan proposed would score + // this member zero. + check_metrics_with_nom_wmc::( + "class C {\n\ + public:\n\ + template operator T() { return T{}; }\n\ + };", + "foo.cpp", + |metric| { + assert_eq!(metric.npm.class_nm_sum(), 1); + assert_eq!(metric.npm.class_npm_sum(), 1); + assert_eq!(metric.nom.functions_sum(), 1); + }, + ); + } + + #[test] + fn cpp_non_method_template_payloads_are_not_counted() { + check_metrics_with_nom_wmc::( + NON_METHOD_TEMPLATE_PAYLOADS, + "foo.cpp", + |metric| { + // `real()` (public, in C) plus `Nested::hidden()` + // (private, in its own class space). + assert_eq!(metric.npm.class_nm_sum(), 2); + assert_eq!(metric.npm.class_npm_sum(), 1); + // `nom` additionally counts the friend's body, which is + // a free function the class merely grants access to and + // not a member — an arm that leaked through + // `friend_declaration` would push `class_nm_sum` to 3. + assert_eq!(metric.nom.functions_sum(), 3); + }, + ); + } + + #[test] + fn mozcpp_template_method_with_inline_body_counts() { + check_metrics_with_nom_wmc::( + TEMPLATE_METHOD_WITH_BODY, + "foo.cpp", + |metric| { + assert_eq!(metric.npm.class_nm_sum(), 2); + assert_eq!(metric.npm.class_npm_sum(), 2); + assert_eq!(metric.nom.functions_sum(), 2); + assert_eq!(metric.wmc.class_wmc_sum(), 2); + }, + ); + } + + #[test] + fn mozcpp_non_method_template_payloads_are_not_counted() { + check_metrics_with_nom_wmc::( + NON_METHOD_TEMPLATE_PAYLOADS, + "foo.cpp", + |metric| { + assert_eq!(metric.npm.class_nm_sum(), 2); + assert_eq!(metric.npm.class_npm_sum(), 1); + assert_eq!(metric.nom.functions_sum(), 3); + }, + ); + } + #[test] fn javascript_empty_unit_no_methods() { check_metrics::("", "empty.js", |metric| { diff --git a/src/metrics/npm/cpp.rs b/src/metrics/npm/cpp.rs index 2c06986de..5935a2920 100644 --- a/src/metrics/npm/cpp.rs +++ b/src/metrics/npm/cpp.rs @@ -50,8 +50,9 @@ impl Npm for CppCode { stats.class_npm += 1; } } - // Declaration-only member function. The wrapping node - // varies by shape: + // Member function reached through a wrapper node + // rather than surfacing as a `function_definition` + // directly. The wrapper varies by shape: // - `field_declaration > function_declarator` for // ordinary forward-declared methods (incl. pure // virtual `= 0` and `Foo* operator->()` wrapped in @@ -59,16 +60,30 @@ impl Npm for CppCode { // - `declaration > function_declarator` for // constructors / destructors (no return type). // - `template_declaration > declaration > - // function_declarator` for templated member fns. + // function_declarator` for a templated member fn + // declared without a body. + // - `template_declaration > function_definition` for a + // templated member fn *with* an inline body. This + // shape was missed until #1258, leaving `npm` + // disagreeing with `nom` and `wmc` on the same class. // - // The shared `cpp_has_function_declarator` helper walks - // the declarator subtree (including `declaration` - // wrappers) so all three shapes collapse into one arm; - // the guard avoids counting non-method declarations - // (e.g. nested type aliases) under the same parent. + // The shared `cpp_declares_function` helper walks the + // declarator subtree (including `declaration` wrappers) + // so all four shapes collapse into one arm. The guard + // is what keeps the `template_declaration` payloads + // that are *not* member functions out: a nested + // templated class (a `type_specifier` the helper does + // not descend into), an `alias_declaration`, a + // templated static data member (a `declaration` with + // no function declarator), and a `friend_declaration`, + // whose function is not a member of this class. The + // list is not closed: both grammars also admit a + // nested `template_declaration` as a child, which the + // helper declines because the two-header form it would + // spell is not valid C++ at class scope. FieldDeclaration | Declaration | Declaration2 | Declaration3 | Declaration4 | TemplateDeclaration - if super::npa::cpp_has_function_declarator(&child) => + if super::npa::cpp_declares_function(&child) => { stats.class_nm += 1; if current_is_public { diff --git a/src/metrics/npm/mozcpp.rs b/src/metrics/npm/mozcpp.rs index 4be8f600f..cff974b0b 100644 --- a/src/metrics/npm/mozcpp.rs +++ b/src/metrics/npm/mozcpp.rs @@ -50,8 +50,9 @@ impl Npm for MozcppCode { stats.class_npm += 1; } } - // Declaration-only member function. The wrapping node - // varies by shape: + // Member function reached through a wrapper node + // rather than surfacing as a `function_definition` + // directly. The wrapper varies by shape: // - `field_declaration > function_declarator` for // ordinary forward-declared methods (incl. pure // virtual `= 0` and `Foo* operator->()` wrapped in @@ -59,16 +60,30 @@ impl Npm for MozcppCode { // - `declaration > function_declarator` for // constructors / destructors (no return type). // - `template_declaration > declaration > - // function_declarator` for templated member fns. + // function_declarator` for a templated member fn + // declared without a body. + // - `template_declaration > function_definition` for a + // templated member fn *with* an inline body. This + // shape was missed until #1258, leaving `npm` + // disagreeing with `nom` and `wmc` on the same class. // - // The shared `cpp_has_function_declarator` helper walks - // the declarator subtree (including `declaration` - // wrappers) so all three shapes collapse into one arm; - // the guard avoids counting non-method declarations - // (e.g. nested type aliases) under the same parent. + // The shared `cpp_declares_function` helper walks the + // declarator subtree (including `declaration` wrappers) + // so all four shapes collapse into one arm. The guard + // is what keeps the `template_declaration` payloads + // that are *not* member functions out: a nested + // templated class (a `type_specifier` the helper does + // not descend into), an `alias_declaration`, a + // templated static data member (a `declaration` with + // no function declarator), and a `friend_declaration`, + // whose function is not a member of this class. The + // list is not closed: both grammars also admit a + // nested `template_declaration` as a child, which the + // helper declines because the two-header form it would + // spell is not valid C++ at class scope. FieldDeclaration | Declaration | Declaration2 | Declaration3 | Declaration4 | TemplateDeclaration - if super::npa::cpp_has_function_declarator(&child) => + if super::npa::cpp_declares_function(&child) => { stats.class_nm += 1; if current_is_public { diff --git a/src/metrics/snapshots/big_code_analysis__metrics__halstead__tests__csharp_primitive_types_and_booleans.snap b/src/metrics/snapshots/big_code_analysis__metrics__halstead__tests__csharp_primitive_types_and_booleans.snap index 55b3b032a..7e60b7bd3 100644 --- a/src/metrics/snapshots/big_code_analysis__metrics__halstead__tests__csharp_primitive_types_and_booleans.snap +++ b/src/metrics/snapshots/big_code_analysis__metrics__halstead__tests__csharp_primitive_types_and_booleans.snap @@ -6,15 +6,15 @@ expression: metric.halstead "unique_operators": 14, "total_operators": 33, "unique_operands": 21, - "total_operands": 23, - "length": 56, + "total_operands": 21, + "length": 54, "estimated_program_length": 145.54163478716043, - "purity_ratio": 2.5989577640564363, + "purity_ratio": 2.6952154590214894, "vocabulary": 35, - "volume": 287.2398489489181, - "difficulty": 7.666666666666667, - "level": 0.13043478260869565, - "effort": 2202.172175275039, - "time": 122.34289862639106, - "bugs": 0.05642171193174631 + "volume": 276.9812829150282, + "difficulty": 7.0, + "level": 0.14285714285714285, + "effort": 1938.8689804051974, + "time": 107.7149433558443, + "bugs": 0.05182958366687233 } diff --git a/src/metrics/snapshots/big_code_analysis__metrics__halstead__tests__php_operators_and_operands.snap b/src/metrics/snapshots/big_code_analysis__metrics__halstead__tests__php_operators_and_operands.snap index 8c58d1f3d..3776aa448 100644 --- a/src/metrics/snapshots/big_code_analysis__metrics__halstead__tests__php_operators_and_operands.snap +++ b/src/metrics/snapshots/big_code_analysis__metrics__halstead__tests__php_operators_and_operands.snap @@ -5,16 +5,16 @@ expression: metric.halstead { "unique_operators": 9, "total_operators": 12, - "unique_operands": 9, - "total_operands": 22, - "length": 34, - "estimated_program_length": 57.05865002596162, - "purity_ratio": 1.6781955889988711, - "vocabulary": 18, - "volume": 141.7774500490386, - "difficulty": 11.0, - "level": 0.09090909090909091, - "effort": 1559.5519505394248, - "time": 86.64177502996804, - "bugs": 0.044827581562788044 + "unique_operands": 6, + "total_operands": 16, + "length": 28, + "estimated_program_length": 44.039100017307746, + "purity_ratio": 1.5728250006181337, + "vocabulary": 15, + "volume": 109.39293667703852, + "difficulty": 12.0, + "level": 0.08333333333333333, + "effort": 1312.7152401244623, + "time": 72.92862445135901, + "bugs": 0.03996309328541862 } diff --git a/src/metrics/snapshots/big_code_analysis__metrics__halstead__tests__php_simple_function.snap b/src/metrics/snapshots/big_code_analysis__metrics__halstead__tests__php_simple_function.snap index 6dbe5b770..e1e607f2f 100644 --- a/src/metrics/snapshots/big_code_analysis__metrics__halstead__tests__php_simple_function.snap +++ b/src/metrics/snapshots/big_code_analysis__metrics__halstead__tests__php_simple_function.snap @@ -5,16 +5,16 @@ expression: metric.halstead { "unique_operators": 7, "total_operators": 7, - "unique_operands": 5, - "total_operands": 10, - "length": 17, - "estimated_program_length": 31.26112492884004, - "purity_ratio": 1.838889701696473, - "vocabulary": 12, - "volume": 60.94436251225965, + "unique_operands": 4, + "total_operands": 8, + "length": 15, + "estimated_program_length": 27.651484454403228, + "purity_ratio": 1.8434322969602153, + "vocabulary": 11, + "volume": 51.89147427955946, "difficulty": 7.0, "level": 0.14285714285714285, - "effort": 426.61053758581755, - "time": 23.700585421434308, - "bugs": 0.018890051025563473 + "effort": 363.2403199569162, + "time": 20.180017775384233, + "bugs": 0.016969723977179437 } diff --git a/src/vcs/error.rs b/src/vcs/error.rs index 4dbda63f0..8cc0c7f22 100644 --- a/src/vcs/error.rs +++ b/src/vcs/error.rs @@ -79,44 +79,124 @@ pub enum Error { Cache(String), } -impl Error { - /// Whether this error was caused by client-supplied input (a bad - /// path, revision, window, timestamp, formula, pattern, threshold, - /// trend parameter, file-type scope, or diff) as opposed to an - /// environment or backend failure (opening the repository, walking - /// history, diffing, `.mailmap`, blame, or the persistent cache). - /// - /// Front ends use this to choose a status: a web boundary maps - /// client-input errors to `400 Bad Request` and the rest to - /// `500 Internal Server Error` (see `vcs_error_response` in the web - /// crate). - /// - /// The match is intentionally exhaustive (no wildcard arm): adding a - /// new [`Error`] variant is a compile error here until it is - /// classified, which prevents the silent fall-through that twice - /// mis-mapped client-input variants to `500` (`InvalidFileTypeScope`, - /// `InvalidDiff`; see issue #641). - #[must_use] - pub fn is_client_input(&self) -> bool { - match self { - Self::NotARepository(_) - | Self::ResolveRef { .. } - | Self::InvalidBotPattern(_) - | Self::InvalidWindow(_) - | Self::InvalidTimestamp(_) - | Self::InvalidFormula(_) - | Self::InvalidFileTypeScope(_) - | Self::InvalidBusFactorThreshold(_) - | Self::InvalidAuthorHashKey(_) - | Self::InvalidTrend(_) - | Self::InvalidDiff(_) => true, - Self::OpenRepository(_) - | Self::Walk(_) - | Self::Diff(_) - | Self::Mailmap(_) - | Self::Blame(_) - | Self::Cache(_) => false, +/// Emits [`Error::is_client_input`] and [`Error::client_input_samples`] +/// from a single variant list. +/// +/// Both halves are generated from the same `client_input` group, so they +/// cannot drift. That coupling is the point. `is_client_input` alone is +/// already compile-forced — its generated match has no wildcard arm, so +/// adding an [`Error`] variant fails to build until it is listed in one +/// group or the other (issue #641). But a front end that turns a +/// client-input variant into its own machine-readable token cannot be +/// compile-forced in the same way: [`Error`] is `#[non_exhaustive]`, so a +/// match in any *other* crate must carry a wildcard and the compiler has +/// nothing to complain about. `InvalidAuthorHashKey` reached the web +/// crate's `vcs_error_kind` wildcard for exactly that reason and was +/// reported to clients as an internal server fault (issue #1245). +/// +/// Listing a variant under `client_input` therefore also yields a +/// constructed sample of it, and the web crate's token guard iterates +/// those samples rather than a hand-written copy of the list. Adding a +/// twelfth client-input variant is a compile error here; fixing that +/// compile error is what makes the token guard fail until a token is +/// chosen. +/// +/// `client_input` takes `pat_param`, not `pat`, so that one entry cannot +/// quietly stand for two variants. `pat` admits a top-level or-pattern, +/// and `Self::InvalidWindow(_) | Self::InvalidTimestamp(_) => ` would keep the match exhaustive and correct while yielding a +/// sample for only the first — reopening #1245 for the second. That is +/// the shape a maintainer reaches for, because it is how the arms were +/// written before this macro existed. `pat_param` rejects the `|` +/// outright ("no rules expected `|`"). The `environment` group keeps +/// `pat` because it produces no samples and nothing depends on its +/// entries being one-to-one. +macro_rules! classify_error_variants { + ( + client_input { $($client:pat_param => $sample:expr),+ $(,)? } + environment { $($environment:pat),+ $(,)? } + ) => { + impl Error { + /// Whether this error was caused by client-supplied input (a bad + /// path, revision, window, timestamp, formula, pattern, threshold, + /// author-hash key, trend parameter, file-type scope, or diff) as + /// opposed to an environment or backend failure (opening the + /// repository, walking history, diffing, `.mailmap`, blame, or the + /// persistent cache). + /// + /// Front ends use this to choose a status: a web boundary maps + /// client-input errors to `400 Bad Request` and the rest to + /// `500 Internal Server Error` (see `vcs_error_response` in the web + /// crate). + /// + /// The match is intentionally exhaustive (no wildcard arm): adding a + /// new [`Error`] variant is a compile error here until it is + /// classified, which prevents the silent fall-through that twice + /// mis-mapped client-input variants to `500` (`InvalidFileTypeScope`, + /// `InvalidDiff`; see issue #641). + #[must_use] + pub fn is_client_input(&self) -> bool { + match self { + $($client => true,)+ + $($environment => false,)+ + } + } + + /// One constructed sample per client-input variant, in + /// declaration order. + /// + /// Test support for front ends that must map every + /// client-input variant onto something of their own — a + /// machine-readable error token, a help string, an exit code. + /// `#[non_exhaustive]` denies them an exhaustive match, so + /// iterating these samples is the only way such a mapping can + /// be checked for completeness (issue #1245). + /// + /// Not part of the stability contract: `#[doc(hidden)]`, and + /// the payloads are placeholders whose exact text may change + /// at any time. Assert on the *variant*, never on a sample's + /// rendered message. + #[doc(hidden)] + #[must_use] + pub fn client_input_samples() -> Vec { + vec![$($sample),+] + } } + }; +} + +// Adding an entry to `client_input` is a cross-crate obligation, not a +// local one: every client-input variant owes the web surface its own +// `error_kind` token (`vcs_error_kind` in `big-code-analysis-web`), plus +// a line in the vocabulary lists in `STABILITY.md` and the book. Nothing +// here can enforce that — `Error` is `#[non_exhaustive]`, so the web +// match must carry a wildcard — which is why the sample below exists for +// the guard test to iterate. One entry per variant; the samples are +// throwaway payloads, deliberately unlike any real message. +classify_error_variants! { + client_input { + Self::NotARepository(_) => Self::NotARepository(PathBuf::from("/not-a-repository")), + Self::ResolveRef { .. } => Self::ResolveRef { + reference: "HEAD".to_owned(), + reason: "unborn branch".to_owned(), + }, + Self::InvalidBotPattern(_) => Self::InvalidBotPattern("[".to_owned()), + Self::InvalidWindow(_) => Self::InvalidWindow("banana".to_owned()), + Self::InvalidTimestamp(_) => Self::InvalidTimestamp("yesterday".to_owned()), + Self::InvalidFormula(_) => Self::InvalidFormula("astrology".to_owned()), + Self::InvalidFileTypeScope(_) => Self::InvalidFileTypeScope(String::new()), + Self::InvalidBusFactorThreshold(_) => Self::InvalidBusFactorThreshold("1.5".to_owned()), + Self::InvalidAuthorHashKey(_) => Self::InvalidAuthorHashKey("hunter2".to_owned()), + Self::InvalidTrend(_) => Self::InvalidTrend("1".to_owned()), + Self::InvalidDiff(_) => Self::InvalidDiff("not a unified diff".to_owned()), + } + environment { + Self::OpenRepository(_), + Self::Walk(_), + Self::Diff(_), + Self::Mailmap(_), + Self::Blame(_), + Self::Cache(_), } } diff --git a/src/vcs/error_tests.rs b/src/vcs/error_tests.rs index bd0bdd1c7..afa3999a5 100644 --- a/src/vcs/error_tests.rs +++ b/src/vcs/error_tests.rs @@ -6,6 +6,10 @@ use std::path::PathBuf; // variant's wording so a refactor cannot silently degrade them. #[test] fn display_covers_every_variant() { + // One case per `Error` variant. The count is asserted below because + // nothing else notices an omission: this list sat at 16 of 17 from + // #956 until #1245, leaving `InvalidAuthorHashKey`'s wording — the + // `error` prose of a `400` — pinned by nothing. let cases: Vec<(Error, &str)> = vec![ ( Error::NotARepository(PathBuf::from("/tmp/x")), @@ -55,6 +59,10 @@ fn display_covers_every_variant() { Error::InvalidBusFactorThreshold("1.5".to_owned()), "invalid bus-factor threshold: 1.5", ), + ( + Error::InvalidAuthorHashKey("the key is empty".to_owned()), + "invalid author-hash key: the key is empty", + ), ( Error::InvalidTrend("one point".to_owned()), "invalid trend parameters: one point", @@ -72,6 +80,22 @@ fn display_covers_every_variant() { "history cache error: disk full", ), ]; + // Completeness, for the half of the enum that can be enumerated. + // Comparing discriminants ignores the payloads, which differ between + // the samples and the cases below. The environment variants have no + // generated list and stay hand-checked — nothing can enumerate them, + // so a new one silently goes unpinned here. + let covered: Vec<_> = cases + .iter() + .map(|(err, _)| std::mem::discriminant(err)) + .collect(); + for sample in Error::client_input_samples() { + assert!( + covered.contains(&std::mem::discriminant(&sample)), + "client-input variant {sample:?} has no Display case; its wording \ + is the `error` prose of a 400 and nothing else pins it", + ); + } for (err, expected) in cases { let rendered = err.to_string(); assert!( @@ -105,31 +129,40 @@ fn invalid_formula_lists_the_accepted_names() { assert!(rendered.contains("percentile"), "{rendered:?}"); } +/// Size of the `client_input` group of `classify_error_variants!`. +/// +/// Hand-maintained on purpose, and compared against +/// `Error::client_input_samples()` — a production-derived list — so it +/// fires when a variant *moves* between the two groups. That move is +/// otherwise invisible: it shrinks one list and grows the other, which +/// every per-variant assertion in this file survives. +/// +/// There is deliberately no matching `ENVIRONMENT_COUNT`. The +/// environment cases below are a vec literal, so asserting its length +/// would compare a literal against a constant and could not fail for +/// any change to `Error`. +const CLIENT_INPUT_COUNT: usize = 11; + // Pin the client-input vs environment/backend classification of every // variant (issue #641). The web boundary maps `is_client_input()` to -// `400`/`500`; a silent re-classification here would silently change the -// HTTP contract, so each variant is asserted explicitly. Mirrors the -// exhaustive match in `Error::is_client_input` — when a variant is added, -// `is_client_input` fails to compile (forcing a decision) and this test -// must gain a corresponding case. +// `400`/`500`, so a silent re-classification changes the HTTP contract. #[test] fn is_client_input_classifies_every_variant() { + // The client-input half comes from `client_input_samples()`, which + // `classify_error_variants!` generates from the same list as + // `is_client_input`'s own arms, so it cannot fall behind the enum. + // The hand-written vec it replaces did exactly that: it held ten + // entries from #956 until #1245, silently omitting + // `InvalidAuthorHashKey` while claiming to cover every variant. + // + // The counts are the load-bearing part, and the reason they are not + // derived. Deriving them would defeat them: *moving* a variant from + // `client_input` to `environment` shrinks the sample set in step, so + // every per-variant assertion below still passes while a client + // mistake starts answering `500` — the inverse of the bug #1245 + // fixed, and just as quiet. let s = || "x".to_owned(); - let client_input: Vec = vec![ - Error::NotARepository(PathBuf::from("/tmp/x")), - Error::ResolveRef { - reference: s(), - reason: s(), - }, - Error::InvalidBotPattern(s()), - Error::InvalidWindow(s()), - Error::InvalidTimestamp(s()), - Error::InvalidFormula(s()), - Error::InvalidFileTypeScope(s()), - Error::InvalidBusFactorThreshold(s()), - Error::InvalidTrend(s()), - Error::InvalidDiff(s()), - ]; + let client_input = Error::client_input_samples(); let environment: Vec = vec![ Error::OpenRepository(s()), Error::Walk(s()), @@ -138,6 +171,12 @@ fn is_client_input_classifies_every_variant() { Error::Blame(s()), Error::Cache(s()), ]; + assert_eq!( + client_input.len(), + CLIENT_INPUT_COUNT, + "the client-input group changed size; if that is deliberate, every \ + new variant also needs an `error_kind` token in the web crate", + ); for err in client_input { assert!(err.is_client_input(), "{err:?} should be client input"); } diff --git a/src/vcs/jit.rs b/src/vcs/jit.rs index 3fd63629e..29c947190 100644 --- a/src/vcs/jit.rs +++ b/src/vcs/jit.rs @@ -58,7 +58,7 @@ //! # Scope //! //! [`score`] / [`JitReport`] cover a real commit (all five groups). -//! Scoring an arbitrary unprovenanced diff (`bca vcs jit --diff `) +//! Scoring an arbitrary unprovenanced diff (`bca vcs commit --diff `) //! is supported as a deliberately *partial* path (issue #580): a bare //! diff carries no author, parent, or file history, so only the size and //! diffusion groups are computable. That path produces a distinct diff --git a/tests/repositories/big-code-analysis-output b/tests/repositories/big-code-analysis-output index 63e0923d7..77c111348 160000 --- a/tests/repositories/big-code-analysis-output +++ b/tests/repositories/big-code-analysis-output @@ -1 +1 @@ -Subproject commit 63e0923d7165e0da78ab97ceaf75905f9460f1aa +Subproject commit 77c1113483e6b079fa4998ca7c74845786884cf9 diff --git a/utils/check-manpage-drift-test.py b/utils/check-manpage-drift-test.py new file mode 100644 index 000000000..9c0ef4452 --- /dev/null +++ b/utils/check-manpage-drift-test.py @@ -0,0 +1,470 @@ +#!/usr/bin/env python3 +"""Tests for check-manpage-drift.py. + +The gate this exercises had gone green for two years over the one state +it most needed to catch — a man page ``cargo xtask`` had just written +and nobody had committed (#1249). A gate that silently stops gating +looks exactly like a clean tree, which is why every direction it claims +to cover is pinned here against a scratch repository. + +The table below is the measured behaviour of the three candidate git +mechanisms; the tests are that table, and swapping the implementation +to either rejected alternative turns one of them red: + +=========================== ========= ================== =========== +state git diff ls-files --others porcelain +=========================== ========= ================== =========== +clean pass pass pass +new page, untracked **pass** fail fail +new page, staged pass pass **fail** +tracked page modified fail pass fail +tracked page deleted fail pass fail +page removed from index **pass** fail fail +=========================== ========= ================== =========== + +There is deliberately **no** smoke test against the real repository. +``man/*.1`` is rewritten in place by the ``_pc-manpages`` stage, which +``make pre-commit`` runs concurrently with this test's own stage, so an +assertion about the real tree's cleanliness would be a race rather than +an invariant. + +Run with: + python3 -m unittest -q utils/check-manpage-drift-test.py +""" + +from __future__ import annotations + +import contextlib +import importlib.util +import io +import os +import pathlib +import subprocess +import tempfile +import unittest +import unittest.mock + +# The gate under test is a sibling in `utils/`. +UTILS_DIR = pathlib.Path(__file__).resolve().parent +SCRIPT_SRC = UTILS_DIR / "check-manpage-drift.py" + + +def _load_module(): # type: ignore[no-untyped-def] + spec = importlib.util.spec_from_file_location( + "check_manpage_drift", SCRIPT_SRC + ) + assert spec is not None and spec.loader is not None + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +DRIFT = _load_module() + +# Identity for the fixture commits, passed per invocation rather than +# written into any config. +GIT_ENV_ARGS = [ + "-c", + "user.email=test@example.invalid", + "-c", + "user.name=manpage-drift test", +] + +# Host config must not reach the fixtures. `GIT_CONFIG_*` alone is not +# enough: git falls back to `$XDG_CONFIG_HOME/git/ignore` (defaulting to +# `$HOME/.config/git/ignore`) for excludes, which no config variable +# suppresses, so a contributor whose global ignore covers `*.1` would +# otherwise silently change what the untracked assertions measure. +# `_isolated_env` re-points both at the per-test tempdir. +BASE_GIT_ENV = { + "GIT_CONFIG_GLOBAL": os.devnull, + "GIT_CONFIG_SYSTEM": os.devnull, +} + + +def run_git(cwd: pathlib.Path, *args: str) -> str: + return subprocess.run( + ["git", *GIT_ENV_ARGS, *args], + cwd=cwd, + check=True, + capture_output=True, + text=True, + ).stdout + + +class ManDirFixture(unittest.TestCase): + """A scratch repo with a committed `man/` tree. + + Pages are named after real ones so a failure message reads like + the situation it models. The `.gitignore` is realism, not a lever: + the untracked half never passes `--exclude-standard`, so the gate's + tolerance for a dropping under `man/` comes from the `man/*.1` + pathspec alone. Emptying this file changes no verdict — the + ignore-rule interaction is pinned by + `test_gitignored_new_page_still_fails` instead. + """ + + PAGES = ("bca.1", "bca-check.1", "bca-web.1") + + def setUp(self) -> None: + self._tmp = tempfile.TemporaryDirectory() + self.addCleanup(self._tmp.cleanup) + self.root = pathlib.Path(self._tmp.name) / "repo" + (self.root / "man").mkdir(parents=True) + + home = pathlib.Path(self._tmp.name) / "home" + home.mkdir() + env = unittest.mock.patch.dict( + os.environ, + {**BASE_GIT_ENV, "HOME": str(home), "XDG_CONFIG_HOME": str(home)}, + ) + env.start() + self.addCleanup(env.stop) + + run_git(self.root, "init", "-q", "-b", "main") + (self.root / ".gitignore").write_text("man/*.log\n", encoding="utf-8") + for page in self.PAGES: + self.page(page).write_text(f".TH {page}\n", encoding="utf-8") + run_git(self.root, "add", "-A") + run_git(self.root, "commit", "-qm", "man pages") + + def page(self, name: str) -> pathlib.Path: + return self.root / "man" / name + + def case_sensitive(self) -> bool: + """True when the fixture's filesystem distinguishes `a` from `A`. + + `git init` sets `core.ignorecase=true` on APFS and NTFS, where + a case-only rename produces a different tree entirely. Probed + rather than inferred from `sys.platform`: a case-sensitive + volume on macOS and a case-insensitive one on Linux both exist. + """ + probe = self.root / "man" / ".case-probe" + probe.write_text("", encoding="utf-8") + try: + return not (self.root / "man" / ".CASE-PROBE").exists() + finally: + probe.unlink() + + def gate(self) -> tuple[int, str, str]: + """Run the real `main()` against the fixture; (code, out, err).""" + out, err = io.StringIO(), io.StringIO() + with unittest.mock.patch.object(DRIFT, "REPO_ROOT", self.root): + with contextlib.redirect_stdout(out), contextlib.redirect_stderr( + err + ): + code = DRIFT.main() + return code, out.getvalue(), err.getvalue() + + def assertPasses(self, why: str) -> None: + code, out, err = self.gate() + self.assertEqual(code, 0, f"{why}: expected pass, got {code}\n{err}") + self.assertIn("manpage-drift: OK", out) + + def assertFails(self, why: str, *named: str) -> None: + code, _out, err = self.gate() + self.assertEqual(code, 1, f"{why}: expected failure, got {code}") + for name in named: + self.assertIn(name, err, f"{why}: report did not name {name}") + + +class CleanTreeTest(ManDirFixture): + def test_committed_tree_passes(self) -> None: + self.assertPasses("a freshly committed man/ tree") + + def test_non_page_dropping_is_not_drift(self) -> None: + # The added-page half is scoped to `man/*.1`, so an editor + # dropping under man/ is not drift — which is what buys the + # right to ignore the ignore rules (see the next test). + (self.root / "man" / "scratch.log").write_text("x\n", encoding="utf-8") + self.assertPasses("a non-page file under man/") + + def test_changes_outside_man_are_not_drift(self) -> None: + # Pins the pathspec scope. Widening either half to the whole + # repository turns `make manpages-check` red on any dirty + # tree — and it runs inside `make pre-commit`, where an + # untracked file elsewhere is the normal state. + (self.root / "untracked.txt").write_text("x\n", encoding="utf-8") + (self.root / "src").mkdir() + (self.root / "src" / "main.rs").write_text("fn main() {}\n") + run_git(self.root, "add", "src/main.rs") + run_git(self.root, "commit", "-qm", "src") + (self.root / "src" / "main.rs").write_text("fn main() {;}\n") + self.assertPasses("changes outside man/") + + +class AddedPageTest(ManDirFixture): + """#1249: the direction `git diff` cannot see.""" + + def test_untracked_new_page_fails(self) -> None: + # Exactly what `cargo xtask` leaves behind for a subcommand + # added to the clap tree but never committed. + self.page("bca-newsub.1").write_text(".TH new\n", encoding="utf-8") + self.assertFails("an untracked new page", "man/bca-newsub.1") + + def test_git_diff_alone_would_have_passed(self) -> None: + # Pins the bug rather than only the fix: if a future edit drops + # the `ls-files` half, this states plainly what is lost. + self.page("bca-newsub.1").write_text(".TH new\n", encoding="utf-8") + self.assertEqual(DRIFT.tracked_drift(self.root), "") + self.assertEqual( + DRIFT.untracked_pages(self.root), ["man/bca-newsub.1"] + ) + + def test_gitignored_new_page_still_fails(self) -> None: + # An ignore rule must not be able to hide a generated page. + # `--exclude-standard` would consult the host's global ignore + # too, where a `*.1` line silently reinstates #1249 and no + # in-repo config can override it. Written to `info/exclude` + # because that is the one exclude source a test can set + # without depending on the host at all. + (self.root / ".git" / "info" / "exclude").write_text( + "man/bca-newsub.1\n", encoding="utf-8" + ) + self.page("bca-newsub.1").write_text(".TH new\n", encoding="utf-8") + self.assertEqual( + run_git( + self.root, "status", "--porcelain", "--", "man/" + ).strip(), + "", + "fixture no longer models an ignored page", + ) + self.assertFails("a gitignored new page", "man/bca-newsub.1") + + def test_staged_new_page_passes(self) -> None: + # The mechanism decision. `git status --porcelain` reports + # `A man/bca-newsub.1` here and would fail — but staging is + # the remedy the gate's own message prescribes, and the + # `pre-commit` framework runs this gate against the staged + # tree, so rejecting it would break every man-page commit. + self.page("bca-newsub.1").write_text(".TH new\n", encoding="utf-8") + run_git(self.root, "add", "man/bca-newsub.1") + self.assertIn( + "A man/bca-newsub.1", + run_git(self.root, "status", "--porcelain", "--", "man/"), + "fixture no longer models the staged-new-page state", + ) + self.assertPasses("a staged but uncommitted new page") + + +class ModifiedPageTest(ManDirFixture): + def test_modified_tracked_page_fails(self) -> None: + self.page("bca-check.1").write_text(".TH changed\n", encoding="utf-8") + self.assertFails("a modified tracked page", "drift from the clap") + + def test_modified_page_diff_is_printed(self) -> None: + # The old gate printed `git diff`'s output; keep that, so a + # reviewer sees which lines moved without re-running anything. + self.page("bca-check.1").write_text(".TH changed\n", encoding="utf-8") + code, out, _err = self.gate() + self.assertEqual(code, 1) + self.assertIn("man/bca-check.1", out) + self.assertIn("+.TH changed", out) + + @unittest.skipUnless( + os.path.exists("/bin/true"), "needs a silent POSIX diff driver" + ) + def test_external_diff_driver_cannot_blank_the_report(self) -> None: + # This pins `--no-ext-diff` specifically. A configured + # `diff.external` / GIT_EXTERNAL_DIFF may print nothing at all, + # and without the flag the failure would name no lines. It does + # NOT pin the verdict rule — with the flag in place git never + # consults the driver, so the run is byte-identical to an + # ordinary diff. `EmptyDiffOutputTest` covers that half. + self.page("bca-check.1").write_text(".TH changed\n", encoding="utf-8") + with unittest.mock.patch.dict( + os.environ, {"GIT_EXTERNAL_DIFF": "/bin/true"} + ): + code, out, err = self.gate() + self.assertEqual(code, 1) + self.assertIn("drift", err) + self.assertIn("+.TH changed", out) + + def test_staged_modification_passes(self) -> None: + self.page("bca-check.1").write_text(".TH changed\n", encoding="utf-8") + run_git(self.root, "add", "man/bca-check.1") + self.assertPasses("a staged modification") + + +class EmptyDiffOutputTest(unittest.TestCase): + """`tracked_drift` reads git's exit status, never stdout emptiness. + + No filesystem state can produce "status 1, no output" while + `--no-ext-diff` is passed, so the only way to cover the rule — and + the fallback string that keeps it from collapsing back into a clean + verdict — is to drive `_git` directly. + """ + + @staticmethod + def _proc(returncode: int, stdout: str) -> subprocess.CompletedProcess: + return subprocess.CompletedProcess( + args=["git"], returncode=returncode, stdout=stdout, stderr="" + ) + + def test_status_one_without_output_is_still_drift(self) -> None: + with unittest.mock.patch.object( + DRIFT, "_git", lambda *_a, **_k: self._proc(1, "") + ): + self.assertNotEqual(DRIFT.tracked_drift(pathlib.Path(".")), "") + + def test_status_zero_with_output_is_not_drift(self) -> None: + # The inverse: a verdict taken from stdout would call this + # drift. Status 0 means git found no differences, whatever it + # chose to print. + with unittest.mock.patch.object( + DRIFT, "_git", lambda *_a, **_k: self._proc(0, "noise\n") + ): + self.assertEqual(DRIFT.tracked_drift(pathlib.Path(".")), "") + + +class RemovedPageTest(ManDirFixture): + def test_page_deleted_from_worktree_fails(self) -> None: + # What `xtask`'s `sweep_orphans` does to a retired subcommand. + self.page("bca-check.1").unlink() + self.assertFails("a page deleted from the worktree", "drift") + + def test_page_removed_from_index_only_fails(self) -> None: + # `git rm --cached` leaves the file on disk and untracked, so + # `git diff` is clean and only the `ls-files` half reports it. + run_git(self.root, "rm", "-q", "--cached", "man/bca-check.1") + self.assertEqual(DRIFT.tracked_drift(self.root), "") + self.assertFails("a page removed from the index", "man/bca-check.1") + + +class CaseOnlyRenameTest(ManDirFixture): + """A case-only command rename, on a case-sensitive filesystem. + + `xtask` writes `bca-Check.1` and `sweep_orphans` — which compares + names case-sensitively — deletes the now-unexpected `bca-check.1`. + Both directions fire at once, and the old gate saw only the + deletion, so its report never named the page that had to be added. + + On a case-*insensitive* filesystem the same rename makes + `sweep_orphans` delete the page it just wrote; that is #1250, a + bug in the sweep rather than in this gate. + """ + + def test_report_names_both_the_removal_and_the_addition(self) -> None: + if not self.case_sensitive(): + self.skipTest("needs a case-sensitive filesystem") + self.page("bca-check.1").unlink() + self.page("bca-Check.1").write_text(".TH Check\n", encoding="utf-8") + self.assertFails( + "a case-only rename", + "man/bca-Check.1", # the added half — invisible before #1249 + "drift from the clap", # the removed half + ) + + +class GitFailureTest(unittest.TestCase): + """A git error must not read as a clean tree. + + Each half is covered on its own: `main()` calls `tracked_drift` + first, so a test that only checks the overall exit code passes + while the *other* half silently swallows everything. + """ + + def setUp(self) -> None: + self._tmp = tempfile.TemporaryDirectory() + self.addCleanup(self._tmp.cleanup) + self.root = pathlib.Path(self._tmp.name) / "not-a-repo" + self.root.mkdir() + + # Without a ceiling, git walks up from $TMPDIR and can discover + # an ancestor `.git` — on a host whose tempdir lives inside a + # checkout, or with GIT_DIR exported, both halves then succeed + # and the gate reports a clean tree. Measured: that turns these + # tests into `0 != 2` failures with no hint of the cause. + env = unittest.mock.patch.dict( + os.environ, + {**BASE_GIT_ENV, "GIT_CEILING_DIRECTORIES": self._tmp.name}, + ) + env.start() + self.addCleanup(env.stop) + for leaked in ("GIT_DIR", "GIT_WORK_TREE"): + os.environ.pop(leaked, None) + + # Assert the precondition rather than assuming it: everything + # below is vacuous if this directory is inside a repository. + probe = subprocess.run( + ["git", "rev-parse", "--show-toplevel"], + cwd=self.root, + capture_output=True, + text=True, + check=False, + ) + self.assertNotEqual( + probe.returncode, 0, "fixture directory is inside a git repo" + ) + + def _run(self) -> tuple[int, str]: + err = io.StringIO() + with unittest.mock.patch.object(DRIFT, "REPO_ROOT", self.root): + with contextlib.redirect_stdout(io.StringIO()): + with contextlib.redirect_stderr(err): + code = DRIFT.main() + return code, err.getvalue() + + def test_tracked_half_failing_alone_exits_two(self) -> None: + with unittest.mock.patch.object( + DRIFT, "untracked_pages", lambda _root: [] + ): + code, err = self._run() + self.assertEqual(code, 2) + self.assertIn("diff", err) + + def test_untracked_half_failing_alone_exits_two(self) -> None: + with unittest.mock.patch.object( + DRIFT, "tracked_drift", lambda _root: "" + ): + code, err = self._run() + self.assertEqual(code, 2) + self.assertIn("ls-files", err) + + def test_both_halves_failing_exits_two(self) -> None: + code, err = self._run() + self.assertEqual(code, 2) + self.assertIn("error:", err) + + +class AnnotationTest(ManDirFixture): + """CI keeps the `::error::` annotation the inline step used to emit.""" + + def _run_with_actions(self, value: str | None) -> str: + """Run the gate over a failing tree; return stdout. + + Asserts the run actually failed, so a "no annotation" case + cannot pass by the gate having found nothing to report. + """ + env = {} if value is None else {"GITHUB_ACTIONS": value} + with unittest.mock.patch.dict(os.environ, env, clear=False): + if value is None: + os.environ.pop("GITHUB_ACTIONS", None) + code, out, _err = self.gate() + self.assertEqual(code, 1, "fixture did not produce a failure") + return out + + def test_annotation_emitted_for_an_added_page(self) -> None: + self.page("bca-newsub.1").write_text(".TH new\n", encoding="utf-8") + self.assertIn("::error::", self._run_with_actions("true")) + + def test_annotation_emitted_for_a_modified_page(self) -> None: + # Both failure branches must reach `_annotate`. Moving the call + # under either `if` is invisible from the other branch's test. + self.page("bca-check.1").write_text(".TH changed\n", encoding="utf-8") + self.assertIn("::error::", self._run_with_actions("true")) + + def test_no_annotation_when_unset(self) -> None: + self.page("bca-newsub.1").write_text(".TH new\n", encoding="utf-8") + self.assertNotIn("::error::", self._run_with_actions(None)) + + def test_no_annotation_when_not_a_runner(self) -> None: + # `== "true"`, not `is not None`: Actions sets the variable to + # the literal string, and popping it alone cannot tell the two + # predicates apart. + self.page("bca-newsub.1").write_text(".TH new\n", encoding="utf-8") + self.assertNotIn("::error::", self._run_with_actions("false")) + + +if __name__ == "__main__": + unittest.main() diff --git a/utils/check-manpage-drift.py b/utils/check-manpage-drift.py new file mode 100644 index 000000000..ddbaf60e1 --- /dev/null +++ b/utils/check-manpage-drift.py @@ -0,0 +1,225 @@ +#!/usr/bin/env python3 +"""check-manpage-drift + +Assert the committed ``man/`` tree matches what ``cargo xtask`` just +regenerated, in **all three** directions: modified, deleted, and added. + +This is the assertion half of the man-page drift gate. Callers run +``cargo xtask`` first (it rewrites ``man/*.1`` in place and sweeps +orphan pages), then run this script: + +* ``make manpages-check`` — the local / ``make pre-commit`` flavour. +* the ``manpage`` job in ``.github/workflows/ci.yml``. + +Both sites call this one script rather than each carrying their own +copy of the check, so the two cannot drift apart (the previous +arrangement was two hand-mirrored shell blocks kept in step by a +comment). + +Why not ``git diff --exit-code -- man/`` alone (#1249) +------------------------------------------------------ + +``git diff`` compares tracked content only, so the page ``cargo xtask`` +writes for a brand-new subcommand — untracked, never committed — is +invisible to it. The gate went green and the page silently never +shipped. Measured verdicts for every state that reaches this gate: + +=========================== ========= ================== =========== +state git diff ls-files --others porcelain +=========================== ========= ================== =========== +clean pass pass pass +new page, untracked **pass** fail fail +new page, staged pass pass **fail** +tracked page modified fail pass fail +tracked page deleted fail pass fail +page removed from index **pass** fail fail +gitignored new page **pass** see below **pass** +=========================== ========= ================== =========== + +The last row is why this script does *not* pass ``--exclude-standard``; +see "Two scoping decisions" below. ``git status --porcelain`` cannot be +made to cover it at all. + +``git diff`` plus ``git ls-files --others`` covers every row. Plain +``git status --porcelain`` also covers every row, but it fails the +"new page, staged" one *by design*, and that state is the remedy this +gate's own error message prescribes (``git add man/``, then commit): +the ``pre-commit`` framework runs its ``manpages`` hook against the +staged tree, so a porcelain check would reject every legitimate +man-page commit. ``git add -N`` is likewise rejected — it mutates the +contributor's index behind their back. + +Note the "page removed from index" row: ``git rm --cached`` leaves the +file on disk and untracked, so ``ls-files --others`` reports it and +``git diff`` does not. That is also the cheapest faithful reproduction +of the added-page state for manual probing, because a fabricated +``man/zz-probe.1`` does *not* work — ``xtask``'s ``sweep_orphans`` +deletes any unexpected ``.1`` before this script ever runs. + +Two scoping decisions worth stating, because both are load-bearing: + +* The untracked half runs **without** ``--exclude-standard``, and is + scoped to ``man/*.1`` to afford that. An ignore rule that covered + a page would otherwise reinstate #1249 exactly — measured: a global + ``~/.config/git/ignore`` containing ``*.1`` makes + ``ls-files --others --exclude-standard`` return nothing over an + uncommitted page, and neither ``GIT_CONFIG_GLOBAL`` nor the repo's + own ``.gitignore`` has any say in that. ``xtask`` writes nothing but + ``.1`` files and ``sweep_orphans`` removes nothing else, so + ``man/*.1`` is exactly the generated surface; narrowing to it means + dropping the ignore rules cannot turn an editor dropping + (``.swp``, ``~``, ``.DS_Store``) into a gate failure. +* The tracked half keeps the old ``man/`` scope, so a tracked non-page + file living there is still covered. Anything tracked is there + deliberately. + +``--exit-code`` is likewise not decoration: a configured +``diff.external`` / ``GIT_EXTERNAL_DIFF`` can print nothing at all, so +treating empty stdout as "clean" reports a modified page as clean — +measured with ``GIT_EXTERNAL_DIFF=/bin/true``. The verdict is git's +exit status. ``--no-ext-diff`` then keeps the *printed* evidence git's +own, so a real failure still shows which lines moved. + +See AGENTS.md "Validation gates" for the policy this enforces. +""" + +from __future__ import annotations + +import os +import pathlib +import subprocess +import sys + +# `parents[1]`, not `parent`: these gates live in `utils/` but every +# path they read is anchored at the repository root. +REPO_ROOT = pathlib.Path(__file__).resolve().parents[1] + +# Everything tracked under `man/` is covered by the modified/deleted +# half; only the generated pages are covered by the added half. See the +# module docstring for why the two scopes differ. +MAN_PATHSPEC = "man/" +GENERATED_PAGE_PATHSPEC = "man/*.1" + + +class GitError(Exception): + """A git invocation failed; carries the command and its stderr.""" + + +def _git( + args: list[str], root: pathlib.Path, allowed: tuple[int, ...] = (0,) +) -> subprocess.CompletedProcess[str]: + """Run ``git `` in *root* and return the completed process. + + Raises :class:`GitError` for any status outside *allowed*, rather + than letting a failure read as an empty (== clean) result — a gate + that reports "no drift" because git could not run is the failure + mode this whole script exists to prevent. + """ + proc = subprocess.run( + ["git", *args], + cwd=root, + capture_output=True, + text=True, + check=False, + ) + if proc.returncode not in allowed: + raise GitError(f"git {' '.join(args)}: {proc.stderr.strip()}") + return proc + + +def tracked_drift(root: pathlib.Path) -> str: + """Return the worktree-vs-index diff for ``man/`` ("" when clean). + + Covers the modified and deleted directions. Deliberately compares + against the *index*, not ``HEAD``: staged-but-uncommitted pages are + the state the gate's remedy produces and must read as clean. + + The verdict is ``--exit-code``'s status, not stdout emptiness: with + ``diff.external`` configured, a diff driver that prints nothing + would otherwise read as a clean tree. ``--no-ext-diff`` is the + second half of that — it keeps the returned text git's own, so the + failure report still shows which lines moved. + """ + proc = _git( + ["diff", "--no-ext-diff", "--exit-code", "--", MAN_PATHSPEC], + root, + allowed=(0, 1), + ) + # `--exit-code` reports 1 for "differences found"; anything else is + # a real git failure and already raised above. The fallback keeps + # "differences found" from collapsing back into a clean verdict if + # git ever reports 1 while printing nothing. + if proc.returncode != 1: + return "" + return proc.stdout or "(git reports differences under man/)\n" + + +def untracked_pages(root: pathlib.Path) -> list[str]: + """Return untracked ``man/*.1`` paths, ignore rules notwithstanding. + + Covers the added direction, which ``git diff`` cannot see (#1249). + """ + out = _git( + ["ls-files", "--others", "--", GENERATED_PAGE_PATHSPEC], root + ).stdout + return [line for line in out.splitlines() if line] + + +def _annotate(summary: str) -> None: + """Emit a GitHub Actions ``::error::`` annotation when on a runner. + + Preserves the annotation the CI step used to spell inline, so a + drift failure still surfaces in the workflow summary rather than + only in the raw log. Annotations are read from stdout. + """ + if os.environ.get("GITHUB_ACTIONS") == "true": + print(f"::error::{summary}") + + +def main() -> int: + try: + diff = tracked_drift(REPO_ROOT) + untracked = untracked_pages(REPO_ROOT) + except GitError as exc: + sys.stderr.write(f"error: {exc}\n") + return 2 + + if not diff and not untracked: + print("manpage-drift: OK (man/ matches the clap schema)") + return 0 + + if diff: + # Same output the old `git diff --exit-code` printed, so a + # reviewer can still see exactly which lines moved. + sys.stdout.write(diff) + sys.stderr.write( + "error: committed man pages drift from the clap schema\n" + " The regenerated files are already in your working " + "tree — run 'git add man/'\n" + " and commit them alongside the clap change.\n" + ) + if untracked: + sys.stderr.write( + "error: newly generated man page(s) are not committed:\n" + ) + for path in untracked: + sys.stderr.write(f" {path}\n") + sys.stderr.write( + " 'cargo xtask' writes one page per (sub)command, so a " + "new subcommand's\n" + " page starts out untracked and 'git diff' cannot see " + "it. Run 'git add man/'\n" + " and commit it alongside the clap change (or delete " + "the file if it is\n" + " not a generated page).\n" + ) + + _annotate( + "Committed man pages drift from the clap schema. Run 'cargo xtask' " + "locally and commit the result (including any newly added page)." + ) + return 1 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/xtask/src/main.rs b/xtask/src/main.rs index 69667d39d..8406e9235 100644 --- a/xtask/src/main.rs +++ b/xtask/src/main.rs @@ -2,9 +2,10 @@ //! //! `cargo xtask` (no args) regenerates the man pages for `bca` and //! `bca-web` under `man/` at the repo root, one `.1` per top-level -//! binary plus one per `bca` subcommand. CI gates a `git diff -//! --exit-code -- man/` against the output, so adding a flag without -//! re-running `cargo xtask` fails the manpage job. +//! binary plus one per `bca` subcommand. CI gates the output with +//! `utils/check-manpage-drift.py` — modified, deleted, *and* newly +//! added pages — so adding a flag or a subcommand without re-running +//! `cargo xtask` and committing the result fails the manpage job. #![allow(missing_docs)] #![allow(clippy::pedantic)] // Production-only `unwrap()` ban. See `[workspace.lints.clippy]` in the @@ -74,15 +75,96 @@ fn run_manpages(workspace_root: &Path) -> io::Result<()> { )?; // Sweep orphan `.1` files (renamed/removed subcommands) so the CI - // `git diff --exit-code -- man/` gate flips red on stale pages - // instead of silently shipping them. + // man-page drift gate flips red on stale pages instead of silently + // shipping them. The deletion shows up in `git diff`; the page the + // rename *adds* needs the gate's untracked half (#1249). A + // *case-only* rename is the one shape the sweep refuses to resolve + // on its own and reports as an error (#1250). sweep_orphans(&out_dir, &expected)?; println!("Wrote man pages to {}", out_dir.display()); Ok(()) } +/// Whether two `*.1` filenames can name the same physical file. +/// +/// The one definition of the equivalence relation both halves of the +/// man-page mechanism apply: `render_man_page`'s collision guard and +/// `sweep_orphans`' classification. They spelled it separately once, +/// disagreed, and the sweep deleted the page the guard had just +/// written (#1250) — sharing it makes that divergence unrepresentable. +/// +/// Only ASCII case is folded, so a non-ASCII pair such as `café.1` / +/// `CAFÉ.1` is *not* matched — on APFS those name one file, making that +/// pair #1250 unfixed. What keeps the gap unreachable is that no clap +/// command in this workspace is non-ASCII. `render_man_page`'s +/// `debug_assert` is a tripwire for that fact rather than a guard on +/// it: it panics instead of returning an error, and is compiled out +/// under `--release` (every invocation today is a debug build). +/// Adopting a Unicode-folding comparison (e.g. `unicase`) is the +/// prerequisite for lifting the restriction. +fn names_same_file(a: &str, b: &str) -> bool { + a.eq_ignore_ascii_case(b) +} + +/// Delete `man/*.1` files that no command in `expected` accounts for. +/// +/// A directory entry has *three* verdicts against `expected`, not two, +/// so sharing the write guard's relation is not simply a matter of +/// swapping in a case-insensitive comparison (#1250): +/// +/// * byte-equal to an expected name — the page just written; keep. +/// * no [`names_same_file`] match — a renamed or removed command's +/// stale page; remove. +/// * a [`names_same_file`] match that is not byte-equal — a case-only +/// command rename; error, because neither other verdict is right. +/// *Removing* destroys the page `render_man_page` just wrote: on +/// APFS / NTFS the write for `BCA.1` lands in the existing `bca.1` +/// directory entry, since those filesystems are case-*preserving* +/// and opening a file under a different spelling does not rename it. +/// *Keeping* strands a stale page on ext4 / btrfs, where the two +/// files are distinct. Measured against +/// `utils/check-manpage-drift.py` (#1249): the untracked `BCA.1` +/// makes the gate red once and `git add man/` clears it, but the +/// rename never touches `bca.1`'s bytes, so that page is invisible +/// to both halves of the gate from the outset and simply stays +/// tracked — after which a case-insensitive sweep can never remove +/// it and it ships forever. +/// +/// Erroring is the only verdict *derivable from the filenames alone* +/// that reads the same on every filesystem, which is what the guard's +/// comment says this crate normalises for, and it keeps the two sites' +/// *polarity* aligned: both reject a case-only collision rather than +/// one rejecting and the other silently retaining. The sweep could +/// instead ask the filesystem whether the two spellings are one file +/// (`st_dev` / `st_ino`, or `file_index` / +/// `volume_serial_number` on Windows) and resolve it with no human +/// step. That is two platform-gated code paths for an event that has +/// happened zero times; it is the escape hatch if case-only renames +/// ever become routine. fn sweep_orphans(out_dir: &Path, expected: &[String]) -> io::Result<()> { + // Load-bearing for the single `find` below, not merely for its + // cost: if `expected` held both `bca.1` and `BCA.1`, `find` would + // answer with whichever came first and the entry `bca.1` would be + // classified by list order — kept or rejected. `render_man_page` + // rules that out by refusing to push a second name that matches an + // earlier one, so the candidate is unique when it exists. + debug_assert!( + !expected + .iter() + .enumerate() + .any(|(i, a)| expected[..i].iter().any(|b| names_same_file(a, b))), + "render_man_page must reject case-insensitively equal page names before they reach \ + the sweep; got {expected:?}", + ); + // Classify every entry before unlinking any of it. `fs::read_dir` + // order is unspecified, so removing as we go and bailing on the + // first conflict would delete a different subset of the orphans on + // each run, and would report only the first of several renames. + // Two phases make "the error path removes nothing" a postcondition + // a test can assert. + let mut orphans = Vec::::new(); + let mut conflicts = Vec::::new(); for entry in fs::read_dir(out_dir)? { let entry = entry?; let path = entry.path(); @@ -94,15 +176,43 @@ fn sweep_orphans(out_dir: &Path, expected: &[String]) -> io::Result<()> { // Only sweep .1 files — leave any future README / .gitkeep // committed alongside untouched. Skip real directories so a // stray `foo.1/` doesn't error out the whole sweep. - if !file_type.is_dir() - && path.extension().is_some_and(|e| e == "1") - && let Some(name) = path.file_name().and_then(|n| n.to_str()) - && !expected.iter().any(|n| n == name) - { - fs::remove_file(&path)?; - println!("Removed orphan {}", path.display()); + if file_type.is_dir() || path.extension().is_none_or(|e| e != "1") { + continue; + } + let Some(name) = path.file_name().and_then(|n| n.to_str()) else { + continue; + }; + // At most one candidate, per the assertion above, so + // byte-equality alone decides between the keep and error arms. + match expected.iter().find(|n| names_same_file(n, name)) { + // The page just written. + Some(want) if want == name => {} + Some(want) => conflicts.push(format!( + " `{name}` differs from expected `{want}` only in ASCII case — `rm man/{name}`" + )), + None => orphans.push(path), } } + + if !conflicts.is_empty() { + return Err(io::Error::new( + io::ErrorKind::AlreadyExists, + format!( + "a case-only man-page rename cannot be swept safely — on a case-insensitive \ + filesystem the stale spelling *is* the page just written, so neither keeping \ + nor removing it is right everywhere. No orphan pages were removed. Delete the \ + file(s) below and re-run `cargo xtask`:\n{}\n(plain `rm`, not `git rm`: on a \ + case-insensitive filesystem the file now holds the freshly rendered content, \ + and `git rm` refuses a path with local modifications)", + conflicts.join("\n"), + ), + )); + } + + for path in orphans { + fs::remove_file(&path)?; + println!("Removed orphan {}", path.display()); + } Ok(()) } @@ -178,7 +288,11 @@ fn render_man_page( // case-insensitive filesystems while case-sensitive ext4/btrfs // see two distinct files. Normalising to ASCII case here makes // the gate behave identically on every developer workstation. - if expected.iter().any(|n| n.eq_ignore_ascii_case(&filename)) { + // + // The relation lives in `names_same_file` because `sweep_orphans` + // must apply the same one; the two spelling it separately, and + // diverging, is #1250. + if expected.iter().any(|n| names_same_file(n, &filename)) { return Err(io::Error::new( io::ErrorKind::AlreadyExists, format!("duplicate man page filename `{filename}` for command `{name}`"), @@ -258,6 +372,198 @@ mod tests { ); } + // A case-only command rename (`bca` -> `BCA`) as it presents on a + // case-insensitive filesystem (APFS, NTFS): `render_man_page` + // wrote `BCA.1`, the write landed in the pre-existing `bca.1` + // directory entry, and that single entry is what the sweep sees. + // + // Both competing designs fail this: the case-*sensitive* original + // deletes the file and returns `Ok`, and a plain + // `eq_ignore_ascii_case` keep returns `Ok` without naming the + // conflict. + #[test] + fn sweep_errors_on_case_only_rename_with_one_entry() { + let tmp = TempDir::new().expect("tempdir"); + fs::write(tmp.path().join("bca.1"), b"freshly rendered BCA page") + .expect("write fixture file"); + + let err = sweep_orphans(tmp.path(), &["BCA.1".to_string()]) + .expect_err("case-only rename must not be swept silently"); + + assert_eq!(err.kind(), io::ErrorKind::AlreadyExists); + let msg = err.to_string(); + assert!( + msg.contains("bca.1") && msg.contains("BCA.1"), + "error must name both spellings, got: {msg}" + ); + assert!( + tmp.path().join("bca.1").exists(), + "the page just written must survive the sweep" + ); + assert_eq!( + fs::read(tmp.path().join("bca.1")).expect("read page"), + b"freshly rendered BCA page", + "surviving page contents must be untouched", + ); + } + + // The same rename as it presents on a case-*sensitive* filesystem + // (ext4, btrfs): `BCA.1` is a new file and the stale `bca.1` + // remains. The verdict must be the error raised above — that + // identical-on-every-filesystem property is the whole reason the + // third arm exists, and it is what a plain `eq_ignore_ascii_case` + // keep gives up (there the stale page survives, is committed by the + // drift gate's own `git add man/` remedy, and is then permanently + // unsweepable). + // + // On a case-insensitive filesystem the fixture collapses: the second + // write lands in the first file's directory entry, so there is one + // page holding the fresh bytes rather than two. The verdict and the + // remedy are asserted in both worlds; only the claim about the stale + // page's *contents* is specific to the two-file case, and it is + // branched on below rather than assumed. + #[test] + fn sweep_errors_on_case_only_rename_with_both_entries() { + let tmp = TempDir::new().expect("tempdir"); + let two_files = dir_is_case_sensitive(tmp.path()); + fs::write(tmp.path().join("bca.1"), b"stale lowercase page").expect("write stale page"); + fs::write(tmp.path().join("BCA.1"), b"freshly rendered BCA page") + .expect("write fresh page"); + + let err = sweep_orphans(tmp.path(), &["BCA.1".to_string()]) + .expect_err("case-only rename must not be swept silently"); + + assert_eq!(err.kind(), io::ErrorKind::AlreadyExists); + // The remedy must name the *stale* spelling. An implementation + // that interpolated the expected name here would tell the + // contributor to delete the page just generated, and every + // `exists()` assertion below would still pass, so this string + // is what catches the transposition. It holds on either + // filesystem: the on-disk entry is spelled `bca.1` in both. + assert!( + err.to_string().contains("`rm man/bca.1`"), + "remedy must name the stale spelling, got: {err}" + ); + assert_eq!( + fs::read(tmp.path().join("BCA.1")).expect("read expected page"), + b"freshly rendered BCA page", + "the expected page must survive with its contents intact", + ); + + if two_files { + assert_eq!( + fs::read(tmp.path().join("bca.1")).expect("read stale page"), + b"stale lowercase page", + "the stale page must be left for the contributor to remove, not silently deleted", + ); + } else { + // The hazard #1250 exists for, observed directly: one entry, + // still spelled with the stale case, now holding the freshly + // rendered bytes. Asserting the count is what makes this a + // real check rather than a restatement of the assertion + // above — the two paths resolve to the same file here. + let pages = man_page_names(tmp.path()); + assert_eq!( + pages, + vec!["bca.1".to_string()], + "a case-insensitive filesystem must leave exactly the stale entry", + ); + } + } + + /// Whether `dir` resolves filenames case-sensitively. + /// + /// Probed rather than inferred from the target OS. `cfg!(unix)` is + /// true on macOS, whose default APFS is case-*insensitive*, and a + /// Linux checkout can sit on a case-insensitive mount — so a `cfg` + /// gate answers a different question than the one that decides + /// whether two spellings are two files. + fn dir_is_case_sensitive(dir: &std::path::Path) -> bool { + let probe = dir.join("zz-case-probe.tmp"); + fs::write(&probe, b"probe").expect("write case probe"); + let distinct = !dir.join("ZZ-CASE-PROBE.TMP").exists(); + fs::remove_file(&probe).expect("remove case probe"); + distinct + } + + /// The `.1` entries in `dir`, sorted, as the filesystem spells them. + fn man_page_names(dir: &std::path::Path) -> Vec { + let mut names: Vec = fs::read_dir(dir) + .expect("read_dir") + .map(|e| e.expect("dir entry").file_name()) + .filter_map(|n| n.into_string().ok()) + .filter(|n| n.ends_with(".1")) + .collect(); + names.sort(); + names + } + + // The error path is a refusal, not a partial sweep: `fs::read_dir` + // order is unspecified, so a sweep that unlinked as it classified + // would remove a different subset of the genuine orphans on each + // run before bailing. `zz-orphan.1` here is a true orphan that the + // same call would delete were it not for the conflict. + #[test] + fn sweep_removes_nothing_when_a_case_only_rename_is_detected() { + let tmp = TempDir::new().expect("tempdir"); + touch(tmp.path(), "bca.1"); + touch(tmp.path(), "zz-orphan.1"); + + sweep_orphans(tmp.path(), &["BCA.1".to_string()]) + .expect_err("case-only rename must not be swept silently"); + + assert!( + tmp.path().join("zz-orphan.1").exists(), + "the error path must not have unlinked a genuine orphan" + ); + } + + // Classifying before unlinking also means every conflict is known + // by the time the error is built, so a contributor renaming two + // commands at once fixes both in one pass instead of rediscovering + // the second on the next run. Order is `fs::read_dir` order and so + // unspecified — assert presence, never position. + #[test] + fn sweep_error_names_every_case_only_rename() { + let tmp = TempDir::new().expect("tempdir"); + touch(tmp.path(), "bca.1"); + touch(tmp.path(), "bca-web.1"); + + let err = sweep_orphans(tmp.path(), &["BCA.1".to_string(), "BCA-WEB.1".to_string()]) + .expect_err("case-only renames must not be swept silently"); + + let msg = err.to_string(); + for stale in ["bca.1", "bca-web.1"] { + assert!( + msg.contains(&format!("`rm man/{stale}`")), + "error must name every conflict; `{stale}` missing from: {msg}" + ); + } + } + + // The shared relation is `eq_ignore_ascii_case`, which folds only + // ASCII — so a non-ASCII case difference is *not* a match and the + // entry is swept as an ordinary orphan. That limitation is the one + // `render_man_page`'s `debug_assert` exists to keep unreachable + // (no clap command in this workspace is non-ASCII). Pinning it here + // makes a future switch to Unicode case folding a deliberate change + // rather than a silent one, and discriminates the ASCII relation + // from a full-folding `to_lowercase()` comparison, which would + // instead error. + #[test] + fn sweep_treats_non_ascii_case_difference_as_an_orphan() { + let tmp = TempDir::new().expect("tempdir"); + touch(tmp.path(), "café.1"); + + sweep_orphans(tmp.path(), &["CAFÉ.1".to_string()]) + .expect("non-ASCII case difference is not a case-insensitive match"); + + assert!( + !tmp.path().join("café.1").exists(), + "eq_ignore_ascii_case does not fold `é`, so the entry is an orphan" + ); + } + #[test] fn sweep_keeps_subdirectory() { let tmp = TempDir::new().expect("tempdir");