fix: batch of six metric, performance, and correctness fixes by dekobon · Pull Request #1089 · dekobon/big-code-analysis

dekobon · 2026-07-27T02:29:40Z

Fixes #1059
Fixes #1066
Fixes #1067
Fixes #1069
Fixes #1070
Fixes #1084

Six issues fixed on one integration branch, each through investigate →
fix → simplify → review → validate. One Fixes keyword per issue, since
a combined Fixes #a #b trailer only closes the first.

What's here

Issue	Change
#1084	The metric walk carries its ancestor chain down the traversal instead of rediscovering it with `Node::parent`
#1067	`Sloc` derives its row count from the span's end column, not an `is_unit` proxy
#1069	Fast integer hasher for `loc`'s line sets; `NestingMap` pre-sized from the node count
#1066	Shared `#[cfg(test)]` helpers moved to a test-only `src/test_support.rs`
#1070	`python_comprehension_clause_nesting` takes `Nesting` rather than three positional `usize`s
#1059	`node_text`'s safety rationale corrected; guarded/unguarded asymmetry resolved as deliberate

Two things to look at first

#1067 changes metric values. This is the only behaviour-changing item.

All languages: library callers passing bytes with no trailing newline
through Source / Ast::parse — including the Python bindings'
Ast.parse, which does not normalise — now get the real row count.
A one-line unterminated file reported sloc == 0, which short-circuited
all three MI formulas to 0.0. The CLI, web server, and
analyze / analyze_source read through read_file_with_eol and are
unaffected on this axis.
Any grammar ending a func-space node at column 0: the same rule
corrects the opposite error. Perl's and Bash's last function absorbed
the trailing newline and was inflated by one row — two checked-in
snapshots recorded a per-function sloc larger than the whole file's,
an impossible value that had been passing. This drift does reach the CLI,
web server, and Python bindings.

Both are stated in CHANGELOG.md. No submodule bump is needed: the
integration corpora contain no Perl or Bash files, and a clean run leaves
no .snap.new.

#1084 is metric-neutral but wide (64 files). Ancestors wraps either a
known chain — where the parent is chain.last() and each ancestor carries
its own sub-chain — or unknown(), which climbs with Node::parent for
callers that reached a node some other way, so unconverted sites stay
correct at the old cost. The walker's truncate(depth) / push(node)
bookkeeping is pinned by a debug-only Ancestors::checked.

Evidence

make pre-commit green end to end (MAKE_EXIT=0): cargo trio, doc
warnings, udeps, markdown/TOML/shell/Makefile/Actions lints, man-page
drift, both self-scan tiers, and the Python ruff/mypy/pyright/pytest/
stubtest stages.
make bench-scaling: all 8 probes within bound. The three perf(metrics): Node::parent lookups make three walks quadratic in nesting depth #1084 probes
moved from the quadratic bound to the linear one — measured exponents
1.97 / 1.95 / 2.01 → 1.10 / 0.95 / 1.07. QUADRATIC_BOUND no longer
exists in the harness.
4718 tests pass; no snapshot churn beyond the two corrected Perl values.
Baseline: net one changed value (metrics_inner halstead.effort
123702 → 124910) plus refactor(tools): move cfg(test) helpers out of the gated production file #1066 retiring src/tools.rs's loc.sloc entry.

Deferred, with issues filed

refactor(cognitive): thread Nesting through increase_nesting #1086 — increase_nesting has the same positional-usize shape at
43 call sites across 21 cognitive modules.
fix(metrics/loc): whitespace-only files break #1067's newline-independence invariant #1087 — whitespace-only files violate fix(metrics/loc): sloc is 0 for a one-line unterminated file #1067's new
newline-independence invariant; upstream tree-sitter span behaviour.
perf(metrics): thread the ancestor chain through the four remaining Node::parent climbs #1088 — the Node::parent climbs outside the probed paths, left on
Ancestors::unknown(). Now includes increment_function_depth, which
is mechanical (its call sites already have ancestors in scope) but was
scoped out to bound perf(metrics): Node::parent lookups make three walks quadratic in nesting depth #1084.

Also included

Lessons 82–84 and an amendment to .claude/rules/testing.md covering the
compile-time-only refactor case, where test-via-revert leaves the new test
uncompilable and you must perturb instead.

`node_text` takes `code: &[u8]`, so slicing it has no char-boundary precondition -- the doc's "off a UTF-8 char boundary" panic cannot occur, and its "incremental reparse, VCS per-function re-slice" hazards are not paths this crate has (`Ast` is documented as a snapshot with `InputEdit` out of scope). Restate the two guards as the unrelated failure modes they actually are: `from_utf8` rejects non-UTF-8 bytes, reachable from ordinary use because `Ast::parse` accepts arbitrary bytes; `code.get` bounds-checks the range, reachable only by violating the same-parse precondition. Resolve the guarded/unguarded asymmetry as deliberate rather than adopting `code.get` at the sibling slice sites. Guarding is free in `node_text` because `Option` is already its contract and a `None` degrades to an unnamed space. The siblings (`get_operand_id`, `is_useful_comment`, `get_text_span`, `write_node_snippet`) return infallible types feeding metric arithmetic or rendered output, where the only fallback would be a fabricated empty value that silently corrupts a count -- worse than upholding a documented precondition. State that precondition on the `Getter` trait, matching how #795 resolved the same question for `dump_node`. Also correct `Ast::from_tree_sitter`, which claimed a tree/code mismatch merely "yields nonsensical metric values"; a tree built from longer source than `code` panics on an out-of-bounds index. Add a `node_text` test pinning the non-UTF-8 path, which had no coverage distinct from the out-of-range path. Fixes #1059

Review of the #1059 fix found the replacement doc introduced two new claims the code does not support. `get_func_space_name` returns `Some("<anonymous>")` for a node with no `name` field -- it never returns `None` there, and `node_text` is only reached when a name child exists. The graceful degradation comes from the walker storing a space name as `Option<String>`. `node_text` is not reached solely through the default `get_func_space_name`; ten per-language modules call it from their `get_func_name` overrides, 25 call sites in total.

The five `#[cfg(test)]` helpers in `src/tools.rs` (check_func_space, check_metrics, metrics_verbatim, assert_child_space_kind, child_space) are used by every per-metric test module, but they lived in a production file that the self-scan gate walks. Their doc comments and `#[cfg(test)]` attribute lines are not pruned by `exclude_tests`, so each new shared helper cost gated `loc.sloc` budget and another `.bca-baseline.toml` ratchet. Move all five into a new `#[cfg(test)] mod test_support`, excluded from the walk via `.bcaignore`. `src/tools.rs` drops from 845 to 749 physical lines and from 779 to 748 gated `loc.sloc` — under the 760 soft limit, so its `<file> loc.sloc` baseline entry is retired rather than ratcheted. `assert_child_space_kind` now delegates to `child_space` instead of repeating the lookup, and both are `#[track_caller]` so a failure points at the test rather than the helper. The crate-root `pub(crate) use crate::tools::check_func_space` re-export had a single consumer and is gone. Also correct `bca.toml`'s `exclude_tests` comment: it claimed the option does not lower `loc.sloc`, which #722 fixed. Measured on `src/tools.rs` before this change: 779 with the manifest, 845 with `--no-config`, root span `1..845` either way. The pruned node's rows are subtracted; its attribute line, doc comment, and surrounding blanks still count. Fixes #1066

Drop the redundant `-> ()` on `check_metrics`' `fn` pointer parameter and normalise the new `.bcaignore` entry to the `./**/` form the neighbouring `*_tests.rs` line already uses. The two `.clone()` calls in the moved helpers were checked and are load-bearing: `FuncSpace` implements `Drop`, so `.metrics` cannot be moved out of it (E0509).

Commit 67e3504 left `check_metrics` with a hand-wrapped parameter list that rustfmt collapses to one line, so `cargo fmt --check` (and therefore `make pre-commit`) was red at HEAD on this branch before any further change. Incidental to #1070; fixed here so the gate is green.

`python_comprehension_clause_nesting` took three bare positional `usize` parameters (`nesting`, `depth`, `lambda`) — the shape AGENTS.md's newtype rule warns against. #1062 introduced the `Nesting` struct and threaded it through the map's read and write sites but stopped at this signature, so the one place that writes three independent nesting fields into per-clause map slots still took them positionally. Take `inherited: Nesting` instead. The two clause arms now share a single `Nesting { conditional: inherited.conditional + for_count, ..inherited }` construction, which both removes the duplicated insert and makes `function_depth` / `lambda` pass through by name rather than by position. No behaviour change: every metric value is identical. The accompanying unit test drives the helper directly and asserts each clause's map slot field-by-field. That is the only way to pin this assignment — every downstream consumer folds the three fields into one sum (`stats.nesting = conditional + function_depth + lambda`), so a transposition is invisible to metric-level assertions. Verified by perturbation: swapping `function_depth` and `lambda` in the helper fails this test and no other, with the remaining 332 cognitive tests still green. `increase_nesting` has the same positional-usize shape at 43 call sites across 21 language modules; it is scoped out here and tracked in #1086, because converting it means reshaping every per-language `compute` body and its transposition is provably unobservable (both operands only ever enter a symmetric sum). Fixes #1070

`Sloc::sloc()` picked its row count from an `is_unit` flag: the unit span used `end - start`, everything else `end - start + 1`. Both are proxies for the real question — does the span's end position land at column 0, meaning the final row contributes no characters? The proxy was wrong in both directions. tree-sitter's root node runs to end-of-input, so the unit ends at column 0 only when the file is newline-terminated; source that stops mid-line lost its last row and a one-line unterminated file reported `sloc == 0`. That in turn made `mi::inputs_are_empty` short-circuit MI to 0.0, and broke the `cloc + ploc <= sloc` invariant for `b"fn f(){}\n/// x"`. In the other direction, tree-sitter-perl's `function_definition` swallows the newline after the closing brace of a file's last `sub`, so the unconditional `+ 1` credited a row that sub does not occupy. Replace the flag with the span's end column and a single `span_rows` rule shared by `sloc()` and the `exclude_tests` prune accounting. `Loc::compute`'s `is_unit` parameter and `NodeFacts::unit` become dead and are dropped, which also confines the walker's `get_space_kind_with_code` lookup to nodes that open a space (the #522 workaround is no longer needed for the Loc path). Metric drift, both user-visible: - Library callers passing bytes with no trailing newline through `Source` / `Ast::parse` now get the real row count. The CLI, web server and Python bindings read files through `read_file_with_eol`, which appends a newline, so they are unaffected. - The last `sub` in every Perl file loses the phantom row: its `sloc` drops by one, along with the file's `sloc_max` / `blank_max` where that sub was the maximum. Tests use `metrics_verbatim` / the new `space_verbatim`, never `check_metrics` — the latter re-appends a trailing newline and would make the whole input class unreachable. Fixes #1067

`Ploc`/`Cloc`'s line-number sets ran SipHash-1-3 on keys this crate produces itself, once per AST node in the catch-all arm of every `Loc::compute`, and `Loc` is default-selected plus an MI input. Move `spaces::NodeIdHasher` to `int_hash::IntKeyHasher` (it is no longer node-id specific; the algorithm is unchanged) and key all three sets with it. `NestingMap` was grown a doubling at a time even though every real `Cognitive::compute` ends by writing its own node's slot, so its final size is one entry per visited node and `Node::descendant_count()` reports that in O(1). Reserve it up front, gated on `Cognitive` being selected and on the new `Cognitive::SEEDS_NESTING`, which is `false` for the two macro-generated no-op impls (`Preproc`, `Ccomment`) so their map stays unallocated. Behaviour is unchanged: hashing and capacity are not observable, no metric value moves. Measured with 20 interleaved rounds of the #1068 harness, both binaries alternated on one pinned core, median paired ratio plus a sign test. `corpus/walk/loc` -7.1% (19/20 rounds, p < 0.0001), `shape/walk/loc/nested-while` -8.7% (19/20, p < 0.0001), `shape/walk/cognitive/nested-while` -12.2% (17/20, p = 0.0026), against controls spanning +0.7% to +6.6% — that control spread is cross-build code layout and bounds the precision at roughly +/-7 points. The nesting reserve does *not* resolve on the corpus slice, whose median file is ~5 KiB; that reading is unresolved, not zero. `docs/development/benchmarking.md` records the interleaved protocol: the sequential `--baseline` recipe it documented reported 23% and 19% swings on untouched benchmarks on this host. Fixes #1069

`tree_sitter` stores no parent pointer, so `Node::parent` resolves by descending from the root and costs `O(depth)`. Three metric walks asked a node for an ancestor once per node and were therefore quadratic in nesting depth, however few steps each took: `Checker::is_else_if` (13 languages), `Node::count_specific_ancestors` from `Loc` (8 languages) and `cognitive/python`, and `elixir_is_inside_quote_block` from `Nom`. Give the walker an explicit ancestor chain and hand it down, the upward counterpart of the downward flag propagation in #1052 / #1062. `Ancestors` (src/node.rs) wraps either a known chain — where the parent is `chain.last()` and each ancestor's own chain is the prefix before it — or `unknown()`, which climbs with `Node::parent` for callers that reached a node some other way. Predicates keep their original logic and only change where the ancestor comes from, so every metric value is unchanged across all 23 languages. `metrics_inner` maintains the chain with a `depth` field on `Walk`, truncating on arrival and pushing after the per-node computes. `Loc::compute`, `Cognitive::compute`, `Nom::compute`, the `_with_code` `Checker` / `Getter` predicates, and `Checker::is_else_if` take it. All three probes in `big-code-analysis-bench` move from `QUADRATIC_BOUND` to `LINEAR_BOUND` (and to `LINEAR_DEPTHS`), which leaves `LINEAR_BOUND` the only bound in the set. Measured on an idle host: `cognitive/nested-if` 1.97 -> 1.14, `loc/nested-declaration` 1.95 -> 1.12, `nom/nested-quote` 2.01 -> 1.02. At depth 1000, `nom` on nested Elixir `quote` blocks drops from ~260 ms to ~2 ms and `loc` on nested C declarations from ~62 ms to ~1 ms. Four `Node::parent` climbs outside these paths stay on `unknown()` — correct, just unaccelerated — because reaching them means widening `is_func` / `Npm` / `Npa` / `get_func_space_name` across every language. They are enumerated in #1088. Fixes #1084

#1084's parity test proves the truncate/push rule on a replica walker in node.rs, so it cannot see metrics_inner itself desynchronising -- a `chain.push` moved ahead of the per-node computes, or a `continue` inserted above the truncate. `Ancestors::parent` trusts `chain.last()` unvalidated, so such a drift feeds every predicate a wrong ancestor silently. Add `Ancestors::checked`, which debug-asserts the chain against `Node::parent` before wrapping it, and construct through it in the walker. `Ancestors::known` stays unchecked for the callers that deliberately pair a chain with a foreign node -- the `previous_sibling` fallback test is one. Debug-only: `Node::parent` is the O(depth) lookup #1084 removes and must not run in release. Verified by perturbing the truncate, which fires the assertion with the offending node kind. Baseline: `metrics_inner` halstead.effort 123702 -> 124910, the one extra operand at the call site.

Record the six issues fixed on this integration branch: #1059, #1066, #1067, #1069, #1070, #1084. The #1067 and #1084 entries state their metric drift and measured complexity change explicitly; the #1068 harness entry is updated now that its three quadratic findings are fixed.

Code review of the batch branch found three factual errors in the notes I wrote, each verified against the corpus. The Python bindings are affected after all: `PyAst::parse` (big-code-analysis-py/src/ast.rs) hands bytes to `Source::from_bytes` verbatim, unlike `analyze_source`, which calls `normalize_eol`. Name it in the drift paragraph. The per-function drift is not Perl-only. Any grammar whose func-space node ends at column 0 loses the phantom row -- Bash does too, which a three-row function reproduces: it measures 3, so the old unconditional `+ 1` gave it 4, more rows than the file that contains it. Generalise the wording and record the Bash case. `benchmarking.md` undercounted what still climbs with `Node::parent`. Enumerate it accurately rather than saying "four". Also correct the `UNTERMINATED_ONE_LINERS` exclusion rationale: `Preproc` and `Ccomment` are not exempt from #1067. Their `Loc` impl is a no-op, but `sloc` is not node-accumulated -- the synthetic Unit root seeds a real span -- so they drift too. They are held out of that sweep only because its `mi != 0` assertion cannot hold with `ploc == 0`, and now get their own regression test.

82: when several predicates need an ancestor, propagate the chain rather than a derived flag each. The same O(depth) `Node::parent` cause was fixed three times (#1052, #1062, #1084) before the general form appeared, and a chain type with an unknown/fallback variant is what keeps the blast radius a choice. 83: a categorical proxy for a positional property is wrong in both directions. `Sloc` keying on `is_unit` rather than on the span's end column lost a row for unterminated files and gained one for Perl's last `sub` -- the latter recorded a per-function `sloc` larger than the whole file's in two checked-in snapshots (#1067). 84: a factual claim in prose is untested code. Eight wrong claims shipped or nearly shipped in one batch, none caught by any gate (#1059, #1066, #1067, #1084). Also amend .claude/rules/testing.md for the case its test-via-revert procedure cannot cover: a compile-time-only refactor, where reverting leaves the new test uncompilable and the perturbation must be run against the whole suite to show it is the only failure. Every count in these entries was measured against the tree rather than copied from the issue text -- #1084's "13 languages" for `is_else_if` counts only the two macro families and misses the hand-written impls in go/python/php, so the ancestor-consuming total is 16.

codecov · 2026-07-27T02:38:14Z

Codecov Report

❌ Patch coverage is 98.28221% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 98.06%. Comparing base (0c4d213) to head (0fc93c8).

Files with missing lines	Patch %	Lines
src/node.rs	96.19%	6 Missing and 1 partial ⚠️
src/metrics/loc.rs	97.27%	3 Missing ⚠️
src/int_hash.rs	98.61%	1 Missing ⚠️
src/metrics/cognitive.rs	98.79%	1 Missing ⚠️
src/metrics/cognitive/python.rs	98.52%	1 Missing ⚠️
src/test_support.rs	97.43%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1089      +/-   ##
==========================================
- Coverage   98.06%   98.06%   -0.01%     
==========================================
  Files         268      270       +2     
  Lines       66586    67117     +531     
  Branches    66156    66687     +531     
==========================================
+ Hits        65299    65816     +517     
- Misses        846      860      +14     
  Partials      441      441

Flag	Coverage Δ
rust	`98.04% <98.28%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/checker.rs	`96.32% <100.00%> (+0.76%)`	⬆️
src/checker/elixir.rs	`87.93% <100.00%> (+7.93%)`	⬆️
src/checker/go.rs	`76.92% <100.00%> (ø)`
src/checker/php.rs	`87.09% <100.00%> (ø)`
src/checker/python.rs	`100.00% <100.00%> (ø)`
src/getter.rs	`92.77% <100.00%> (+1.34%)`	⬆️
src/getter/elixir.rs	`93.22% <100.00%> (-1.02%)`	⬇️
src/macros/mod.rs	`92.46% <100.00%> (+0.32%)`	⬆️
src/metrics/abc.rs	`99.61% <ø> (ø)`
src/metrics/cognitive/bash.rs	`100.00% <100.00%> (ø)`
... and 67 more

... and 6 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…cost Codecov flagged 16 uncovered lines on #1089. Most are doc comments, braces, and assert!-message arguments that only evaluate on failure. Two were real. `Checker::is_else_if`'s trait default and Elixir's override both answer a flat `false`, and neither was executed by any test: `Preproc` and `Ccomment` (the only two grammars using the default) have no `if` construct, and Elixir branches with `cond do` or nested `if/else`. #1084 changed both signatures to take `Ancestors`, exactly the edit that can invert a one-line predicate unnoticed. Pin the contract directly; verified by perturbing each to `true` independently. The third, `cognitive/python.rs`'s boolean-ancestor stop set, turned out not to be a test gap. Deleting three of its four arms -- or the `ExpressionList` arm alone -- leaves the whole lib suite green, and no fixture I could build discriminates them: a lambda body is a single expression, so a lambda can never enclose an `if`/`for`/`while` statement and there is no outer lambda for a missing stop to over-count. Filed as #1090 rather than papered over with a test that cannot fail. The new Python test pins the scores it does cover and says in its doc that it does not discriminate the arms. The remaining flagged line, `int_hash.rs`'s `usize::try_from` guard, is unreachable on a 64-bit target by design.

dekobon added 13 commits July 26, 2026 13:15

dekobon mentioned this pull request Jul 27, 2026

metrics/cognitive: Python boolean-ancestor stop set is unobservable — dead or untested? #1090

Open

dekobon merged commit edbad0a into main Jul 27, 2026
39 checks passed

dekobon deleted the fix/batch-2026-07-26 branch July 27, 2026 05:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: batch of six metric, performance, and correctness fixes#1089

fix: batch of six metric, performance, and correctness fixes#1089
dekobon merged 14 commits into
mainfrom
fix/batch-2026-07-26

dekobon commented Jul 27, 2026

Uh oh!

codecov Bot commented Jul 27, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

dekobon commented Jul 27, 2026

What's here

Two things to look at first

Evidence

Deferred, with issues filed

Also included

Uh oh!

codecov Bot commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

codecov Bot commented Jul 27, 2026 •

edited

Loading