fix: batch of fifteen issues (#1180-#1182, #1184-#1192, #1194-#1196) - #1198
Merged
Conversation
`utils/check-snapshot-anchors.py` classified comment and string spans before deciding whether a snapshot call was anchored, but never lexed char literals. A `b'"'` or `'"'` read as an unpaired double quote opened a string span running to the next `"` anywhere later in the file, so every `insta::assert_json_snapshot!` in between became invisible and the gate reported a clean file. That is the exact "reads as clean" failure the gate exists to prevent, so it must not be its own failure mode. Ports `char_literal_end()` from `check-rustfmt-bail.py`, which hit the identical defect in #1136. Returning `None` for a lifetime is the half that matters: `'a`, `'_` and `'outer:` have no closing quote, and a greedy variant that treats one as a literal swallows the rest of the file in the other direction. Three further gaps found while fixing it, all the same failure mode: - `find_macro_call_end` is a second, independent lexer in the same file and carried both defects. A char literal in the argument list ran the body scan past the real closing paren and into the next call, whose inline `@"…"` the bare call then claimed as its own anchor. - The raw-string branch tested for `r` only, so `br"…"` / `br#"…"#` were unrecognised openers. Extracted `raw_string_end` / `regular_string_end` so both lexers share one spelling. - `default_targets()` used a non-recursive `glob`, so the 126 files under `src/metrics/{abc,cognitive,cyclomatic,loc,npa,npm}/` created by the #969 split were never scanned at all. Now `rglob`. Latent, not live: every live per-file count is unchanged, confirming the issue's assessment. Zero counts are no longer written to the baseline — an unlisted file is already allowed zero via `load_baseline`'s `.get(rel, 0)`, so the recursive scan adds no noise and the baseline stays a list of debt. That made the `if not baseline` guard wrong for a tree with no debt anywhere, which now tests existence instead. Also ports `baseline_key()`, so naming a file outside the repository reports a count rather than a `relative_to` traceback. Adds `utils/check-snapshot-anchors-test.py` (15 tests) and wires it into `make snapshot-anchors-test`, the pre-commit/CI DAGs, the `lint` aggregate, and an explicit CI step, matching the rustfmt-bail pair. Each end-to-end fixture was verified by perturbation: removing the char branch, making it greedy, and removing the byte-raw branch each fail the test that names the behaviour, and nothing else. Three first-draft fixtures passed against the bug and were replaced — an even number of lifetimes ahead of the call pairs off harmlessly, and a raw string with balanced quotes lexes the same either way. Fixes #1192
`test_analyze_vcs_true_releases_gil_for_walk` failed intermittently on `assert progressed > 0` — once during `make pre-commit`, then 5/5 clean on an unchanged tree. The worker signalled `started.set()` before calling `analyze()`, so the main thread could wake, be descheduled, and find the walk already over. The fixture repo has one commit, so the walk is sub-millisecond. "The worker finished before I looked" and "the GIL was held so I could not run" are indistinguishable from the main thread's side with this design, so the assertion samples a race rather than measuring the property. Takes direction 3 from the issue: drop `progressed`, rename to `test_analyze_vcs_true_completes_off_the_main_thread`, and keep the claim a worker thread can actually support — the walk completes rather than deadlocking the interpreter. The equivalence the old docstring asserted in prose but never checked is now a real assertion: the threaded block must equal a serial call's. Two secondary defects fixed while here. The worker's `assert out is not None` was swallowed by the thread and resurfaced in the main thread as a KeyError on `result`, hiding the cause; it is now captured and re-raised. And `worker.join(timeout=30.0)` was dead code, because the polling loop only exited once the thread was already gone — `join` is now the wait, so the timeout is load-bearing and a genuine deadlock fails rather than hangs the suite. Evidence: 25 runs of the new test under full CPU saturation, 0 failures. The old form also survived 25 such runs, so the flake was not reproduced on demand — but the race window is demonstrable directly. Stalling the main thread 50 ms after `started.wait()`, which is what a loaded machine does to it, yields `progressed == 0` against a healthy GIL-releasing walk, while a 0 ms stall yields 7. The assertion measured scheduling luck, not the contract. Fixes #1191
`spaces::line_span` kept a `child_count() == 0 -> (0, 0)` case for a
childless root, so a whitespace-only file reported `unit 0..0` where
every other space is 1-based. `tests/parity/space_span_containment.rs`
asserts the file-level unit spans `(1, line_count)`; no fixture reached
the case, so the test passed while the invariant it states was false.
The measured cause is not what the guard's comment claimed. A childless
root is not zero-extent: tree-sitter collapses it to a *point at the end*
of the whitespace, so `start_row` is the last row, not row 0. The general
rule would therefore have returned an inverted `3..2` for `"\n\n"`, not
the `1..2` the issue assumed — clamping the start is the fix, not
widening the guard's condition.
A control test written to pin that the carve-out did not leak into
ordinary files then failed, surfacing a second half the issue does not
mention: tree-sitter starts the root at the first *token*, so any file
opening with blank lines reported a unit that omitted them.
`"\n\n\nfn a() {}\n\n\n"` gave `4..6` for a 6-line file. A leading
comment was always fine, since comments are in the tree. Fixing only the
childless case would have left the invariant false for this input, so
both are fixed together: the unit's start is now anchored at 1 rather
than measured, which is what "the file-level unit" means.
An empty file keeps `0..0`. It is the one input with no lines at all, so
`(1, line_count)` would demand the inverted `1..0`; that carve-out is
pinned as a unit test and called out in the parity test's doc.
Both callers — `FuncSpace::new` and `Ops::new` — go through this, so the
metrics and ops walks move together and their parity is preserved.
Tests: four unit tests beside the fix using `space_verbatim` (the
ordinary harnesses append a trailing newline and cannot reach these
inputs), and a whitespace-only sweep added to the parity suite so the
`(1, line_count)` rule is exercised in every language rather than only
against real code fixtures. Each was verified by perturbation: restoring
the old guard fails the whitespace and leading-blank-line tests, dropping
the empty-file carve-out fails the empty-file test, and anchoring every
kind rather than only `Unit` fails the nested-space control.
Three CSV snapshots move, all in one field: their `r#"` fixtures open
with a newline, so the unit row's start goes 2 -> 1 with every other
value byte-identical. The integration-snapshot submodule is unchanged.
Fixes #1195
A comment inside a ternary changed its ABC conditions in thirteen languages. tree-sitter counts a comment among a node's children, so it is both the operand's previous sibling and a shift in every positional index — one cause, two opposite symptoms: - **Over-count** (C, C++, Objective-C, Mozcpp, PHP, Perl, JavaScript, TypeScript, TSX, MozJS). The boolean-context seed asked "is my previous sibling `?` or `:`". A comment answers no, flipping the seed on for a *branch* slot: `a ? /*n*/ (b) : c` scored 3 where `a ? (b) : c` scores 2. This is the defect #1181 describes. - **Under-count** (Java, C#, Groovy). Not mentioned in the issue, and found by a control test written to check the first fix had not leaked. These walk branches by `child(2)` / `child(4)`, so the comment *is* child 2 and the real operand is never inspected: `a ? /*n*/ !b : c` scored 2 where `a ? !b : c` scores 3. Both slots now resolve through `child_by_field_name`, testing identity against the `condition` field rather than negating a neighbour-token test. All three grammars in the second group already expose `condition` / `consequence` / `alternative`, so the positional form was never necessary. This also closes the inverts-on-failure weakness the issue gives as its real reason: an unmatched sibling meant "boolean context", so a future extra token id would have turned every parenthesised alternative into a condition. The issue scopes this to three modules; it is seven, covering thirteen languages — `cpp_inspect_container` is shared by the C, Objective-C and Mozcpp impls, and `js_family` by four. Ruby was already on the field form (#1161) and is unchanged; its comment describing the C-family defect as live has been updated. Java, C# and Groovy needed both halves in one change: fixing the branch walk alone would have fed correctly-located operands to a seed that still flipped on for them, converting their under-count into the C family's over-count. Tests: `ternary_comment_invariance` in `abc.rs` sweeps every enabled ternary language and asserts a comment changes nothing, for both a parenthesised and a negated branch operand — the two inputs that discriminate the two defects, neither of which existing fixtures use. A companion test pins the absolute values 2 and 3 so a regression moving both sides equally still fails. Verified by perturbation: restoring the C++ seed fails the over-count direction and restoring Java's positional walk fails the under-count direction, each naming its own language. Removes `Node::previous_sibling_under` and its test. The fix deleted its last seven callers, and its doc said it existed for "every ABC condition walker" — with none left it was dead code. `Node::previous_sibling` and `Ancestors::previous_sibling` keep their `checker.rs` callers. No corpus snapshot moves: a comment between `?` and a parenthesised or negated operand does not occur in the integration corpora. Fixes #1181
Ruby's and Perl's ABC unary-conditional walkers tested only the `!`
token, so the two spellings of one negation scored differently:
if not b 0 vs if !b 1
x = a ? (not b) : (not c) 2 vs x = a ? (!b) : (!c) 4
`not` and `!` differ in precedence but not in meaning, and ABC counts
the negation rather than the parse. Both now read the grammar's
`operator` field — present on `unary` / `unary_expression` in both
grammars — and accept either token, matching the field-based addressing
the ternary slots moved to in #1181.
Sibling sweep, per grammar-dispatch item 7. The issue nominates Perl and
Lua; measured, it is Perl only:
- **Perl** — same gap, same fix. Its `operator` field types are
`! + ++ - -- and not ~`. The hidden `_unary_not` supertype
(`P::UnaryNot`) is deliberately not matched: the parser never emits it.
- **Lua** — already correct. `not` is its *only* negation keyword and was
already the token being tested, so there was nothing to widen.
- **Elixir** — not named in the issue, checked because it has a `not`
keyword and a `Not` variant. Already correct; it reaches the same
count by another path.
- **Python** — counts `not` through its own dispatcher arm and has no
`!` spelling to compare against.
Every one of those is exercised by the new test rather than only noted,
so a future edit cannot regress the already-correct three silently.
Tests: `keyword_negation_parity` in `abc.rs` asserts the two spellings
score identically across the guard, both-branch-ternary and
single-branch-ternary shapes, plus a companion pinning the absolute
values 1 and 4 so a regression moving both spellings equally still
fails. Verified by perturbation: narrowing either language back to
`BANG` alone fails two tests naming that language.
No corpus snapshot moves.
Fixes #1182
Tcl and iRules had none of the Phase 2B slot routing, so a bare-truthy
predicate scored zero and a ternary scored only its `?` marker. Both now
match the value every other language reports for the same expression:
if {$a} 0 -> 1 (C's `if (a)` is 1)
if {!$a} 0 -> 1
while {$a} 0 -> 1
if/elseif/else 2 -> 4 (C++ is 4)
expr {$a ? !$b : !$c} 1 -> 4 (C++ is 4)
The fix the issue prescribes does not transfer. Its model is #1102/#1161's
`child_by_field_name("condition")`, but `ternary_expr` in both grammars
exposes **no fields at all**, and `while` has no `condition` field either
(`seq('while', $.expr, $._word)`). Slots are therefore located relative
to the `?` and `:` tokens, never by index: `_expr` inlines
`seq('(', $._expr, ')')`, so `($a) ? !$b : !$c` puts anonymous parens
directly under `ternary_expr` and shifts every operand right.
Three further gaps had to close for the routing to work at all, none of
them ternary-specific:
- The seed listed one parent kind (`BinopExpr`), so only a `&&` / `||`
chain ever established boolean context. It now also admits the
`if` / `elseif` / `while` predicate and a ternary's condition slot.
- `has_boolean_content` was not `mut` and lacked the
`if !has_boolean_content && is_not` line every sibling carries, so a
`!` outside a chain could never establish context for itself.
- There was no wrapper peel. The `expr` node is this grammar's
`condition_clause`; peeling it is what reaches the operand.
Grammar-dispatch item 1 bites twice here and both traps are commented at
the point of use: `Expr2` is the `expr` *command keyword* and `Expr3` the
hidden `_expr` supertype, and both render to the same name as the
`Expr` node the code wants. `If`/`While`/`Elseif` have the same
node-vs-keyword split. Item 9 does *not* bite for `if`/`while` — contrary
to the issue's warning, both have dedicated productions rather than
parsing as a generic `command`.
A predicate written as a command substitution is a truthy test of that
command's result, so `if {[somecmd]}` reports 1 exactly as `if {$a}`
does. The redundant `if {[expr {…}]}` idiom therefore reports the chain
plus one, which is what moves `tcl_if_multiple_conditions` 7 -> 9 and
`tcl_while_conditions` 4 -> 6.
Scope: this delivers the issue's three numbered Scope items. The argument
and `return` slots named in its *Where* section stay unrouted, and the
book row now says so precisely rather than claiming the whole of Phase 2B
is missing. Wiring them needs a decision the issue does not make — in
these grammars a negation is reachable only through a standalone
`expr {…}` command, and routing that double-counts against the
command-substitution treatment above.
Tests: `irules_abc_bare_truthy_reports_zero` pinned the *absence* of this
routing and is rewritten as `..._counts_one_condition`; six new tests
cover the negated bare predicate, the ternary slots, the #1161
comparison control (`$a > 0 ? 1 : 0`, which must not move and does not),
and a parenthesised condition in both languages. That last one matters:
a first pass without it left the whole fixed-index revert passing every
test, since indices 0/2/4 are accidentally right for an unparenthesised
ternary. Verified by perturbation — reverting the seed fails two tests,
reverting either language's slot location fails exactly that language's
parenthesised test.
Book: the Tcl and iRules deviation rows are rewritten, and both languages
join the ternary-slots row.
No corpus snapshot moves; three in-tree Tcl snapshots move by value only.
Fixes #1180
A manifest `exclude` glob is written against the `bca.toml` directory,
but `WalkFilters::passes` matched it against the *walk root*. The two
coincide only for the canonical `paths = ["."]`, so a directory seed
silently stopped excluding anything:
$ bca metrics -O json | jq -r .name # ./sub/kept.rs — correct
$ bca metrics -p sub -O json | jq -r .name
sub/kept.rs
sub/skipme/a.rs # leaks
The same leak appears from inside the subdirectory (`cd sub && bca
metrics -p .`), where the walk root, the working directory and the
manifest root are three different places. Both are fixed.
Pre-existing, not a #1164 regression: that change anchored the
explicitly-named-file path and the `bca check` gate, and left this one
as it was.
The rule — *a path is excluded when the CLI set matches at the working
directory, or the manifest set matches at the manifest directory* — was
spelled three times with three different answers about anchoring. It now
lives once, in `walk_seed::AnchoredExcludes`, which `WalkFilters::passes`,
`WalkFilters::warn_exclude_overridden` and `CheckExcludes::exempts` all
delegate to, so the walker and the gate cannot drift about which files a
manifest glob describes. The one deliberate exception — an explicitly
named file seed bypasses excludes entirely (#726) — stays a separate
method on the caller (`WalkFilters::includes`) and is documented on the
shared type, rather than hiding as a divergence inside one match
expression.
Cost, which the issue flags as the thing to check: anchoring allocates
and `passes` runs for every walked file, so the working directory is
resolved once per walk rather than per entry, and the manifest branch is
guarded by `is_empty()` — a run with no manifest excludes pays nothing,
and one with them pays the normalising allocation only after the CLI set
declines. The issue's claim that `manifest_match_path` costs a
`current_dir()` syscall per call is not accurate: `ManifestAnchor`
already carried a caller-resolved `cwd`. The real cost is the join,
lexical-normalise and strip.
Tests: three added to `explicit_path_excludes.rs`, which had no
directory-seed coverage at all — a subdirectory seed from the manifest
root, the same from inside it, and a control pinning that the canonical
root walk (always correct) stays correct. Verified by perturbation:
matching the manifest set at the walk root again fails both new
directory-seed tests and two pre-existing #1164 tests, while the control
passes, which is exactly the asymmetry the bug had.
Fixes #1189
`--print-effective-config` reported one flattened `check_exclude` array, so a reader could not tell which globs resolve against which root. After #1164 that is what decides whether a glob matches at all: a CLI glob resolves against the caller's working directory, a manifest glob against the `bca.toml` directory. This is the surface consulted to answer "which exemptions are in effect?", which is the question #1164 exists to make answerable. Takes option 1 — additive sibling keys naming the manifest-origin subset, leaving the resolved arrays where they are. The walker's `exclude` surface gets the same treatment as the gate's, since it flattens two anchors identically; the issue names only the gate. check_exclude = ["tests/**", "./generated/**"] manifest_check_exclude = ["./generated/**"] manifest = "/repo/bca.toml" The anchor is the `manifest` key's directory, which is populated exactly when the new keys are. The round-trip constraint the issue says shapes the fix turns out not to bind: `--config` deserializes `ThresholdConfig`, which reads only the `[thresholds]` table and has no `deny_unknown_fields`, so `[check]` is parsed and discarded — and `check_print_effective_config_toml_roundtrips_through_config` asserts only two `[thresholds]` substrings. Verified by round-tripping a captured config through `--config` with the new keys present. One correction to the issue. It reports `display_globs_from` as dropping the manifest's `exclude_from` file when a CLI `--exclude-from` is also set. The file is not dropped from the report — it is not in effect: a CLI `--exclude-from` *replaces* the manifest's rather than unioning with it, by the documented `replaced_by` rule in `Manifest::merge` ("the inline list unions, the file does not"). Reporting it would assert something false. `manifest_exclude_from` is therefore present exactly when the manifest's file is the one in force, which is the provenance question for that key. Tests: `print_effective_config_unions_the_manifest_check_exclude` gains the new-shape assertions the issue nominates it for, plus a check that an unconfigured surface emits no empty provenance key. A new test pins the replace-not-union rule from both sides — the manifest's file goes absent while its inline glob stays — so the two rules cannot be collapsed into a single "CLI wins" story by a later edit. Book: the `check` page documents both key pairs and the union-vs-replace difference between the glob lists and the files. Fixes #1194
`write_language_section` existed twice, and its format-independent half — the `Source` match, the `cc_note` select split, and the `fully_suppressed_count` call — was line-for-line identical. Both copies carried a comment claiming the two formats could not diverge, which a comment cannot enforce. That half moves into `markdown_report::hotspot` as `select_for`, returning a `SpecOutcome` of either rows (with the CC stats a `cc_note` spec needs) or a fully-suppressed count. Each renderer's loop becomes a two-arm match over its own four emit functions, which really are the only format-specific part. The issue's sketch names two types that do not exist: there is no `Row` (rows are `Vec<&FunctionSummary>` throughout) and no `CcStats` (it is `CyclomaticStats`). The signature here is written against the real ones. Per-format rationale stays at the call sites, per `.claude/rules/macro-comments.md`: why HTML wraps the MI note in a styled `<p>` while Markdown has a dedicated emitter, and why the suppressed caption takes the spec in one format and the rendered title in the other. What moved into `select_for` is the shared reasoning — select-first, and why an empty table is not the same as a suppressed one. Folds in the second duplicate the issue flags: `partition_by_kind` and `split_units_and_functions` were the same function with the same signature and two implementations (one-pass with `with_capacity` vs two filter passes). The one-pass version survives, now shared. Tests: `both_formats_select_the_same_sections` is new, and it is the point of the change. The existing cross-format guard checks column membership only, so it could not have noticed the two selection copies disagreeing — a structural guard should not rest on a property nothing covers. It sweeps `SPECS` asserting the formats agree on both which sections render a table and which are fully suppressed, with a fixture carrying both outcomes and `seen_table > 0` / `seen_suppressed > 0` so neither arm can pass vacuously. Verified by perturbation: dropping the fully-suppressed arm from the HTML renderer alone fails it, naming the section the formats disagreed about. Behaviour-preserving: all 5105 tests pass unchanged, including the rich report snapshots. Fixes #1190
A lambda written without the optional parentheses reported zero
arguments:
Function<Integer,Integer> f = x -> x + 1; // nargs 0, should be 1
Function<Integer,Integer> g = (x) -> x + 1; // nargs 1
The two spellings are the same lambda — the parens are optional in the
grammar and carry no meaning — so this breaks the same
"byte-equivalent constructs score identically" contract the book states
for cognitive.
`compute_args` walks the children of the `parameters` field. Java's
`lambda_expression` puts a list there for `(x) -> …` and `() -> …`, but
for the bare form it puts a childless `identifier`, so the walk yields
nothing. The singular-`parameter` fallback beside it would have handled
exactly this, but it is unreachable: the `if let` already matched.
Sibling sweep, per grammar-dispatch item 7. The issue nominates Groovy
and Kotlin; measured, it is **C#** — which the issue does not mention.
Its bare `x => x + 1` puts a childless `implicit_parameter` in the same
field, with the same result. Groovy has no `lambda_expression` node at
all and Kotlin routes lambdas through `compute_kotlin_lambda_args`; both
were already correct, and both are now covered by the test so they
cannot regress silently. The JS family reaches the right answer through
the singular `parameter` field.
The fix is a gated `Checker::is_bare_param` hook (default `false`, so no
other language changes) rather than an ungated `child_count() == 0`.
Honest caveat: no test distinguishes the two — the ungated form passes
the whole suite — because the difference only appears on a MISSING or
zero-width ERROR node under `parameters`, which no fixture produces. The
gate is chosen so a broken parse cannot silently conjure an argument,
and so as not to repeat the not-gated-in-code shape the issue complains
about in the existing fallback.
A Java `NArgs::compute` override was not an option: the trait doc makes
`compute` and `params_owner` mutually exclusive, and `JavaCode` already
overrides `params_owner` for #1160's compact constructor.
Tests: `lambda_parenthesisation_parity` asserts the two spellings agree
across Java, C# and the JS family, plus the absolute value 1 so a
regression zeroing both spellings still fails, `() -> 1` staying 0 (the
new branch must not mistake an empty list for a parameter), and
`(x, y) -> …` staying 2 (the plural path undisturbed). A companion pins
that the argument is billed to `closure_args`, per language rather than
globally — the JS arrow's channel depends on binding, which is #1188.
Verified by perturbation: removing the branch fails both tests.
Two C# corpus snapshots move, `closure_args` 0 -> 1 with its derived
aggregates and no `fn_args` field touched. Submodule bumped and pushed
in this commit.
Fixes #1185
Kotlin property accessors (`get()` / `set()`), Kotlin `init { … }`, Java
and Groovy `static { … }`, and the JS family's ES2022 `class_static_block`
were each referenced nowhere outside the generated language enum. None
opened a `FuncSpace`, so its control flow was charged to the enclosing
class space, and `bca check` could never flag one however complex it got.
Each now appears in `Checker::is_func_space` and `Getter::get_space_kind`,
and is named by a new `get_func_space_name` override: `<get>`, `<set>`,
`<init>`, `<static-init>`. `get_func_space_name` returns `Option<&'a str>`
borrowed from `code`, so a `&'static str` is the only synthesised name
possible — a per-property `<get-foo>` would need a signature change. The
angle brackets follow the existing `<anonymous>` convention and cannot
collide with a real identifier in any of these grammars.
Deliberately **not** in `is_func`. None is a callable named at a call
site — in Kotlin you write `p.foo`, not `p.getFoo()` — and putting an
accessor there would have `Npm` bill the same property once as an
attribute and again as a method, skewing the NPA/NPM ratio the OOP
metrics exist to report. This is the split the JS family already uses
for `FunctionExpression`.
That choice has a consequence the issue does not anticipate, confirmed
by measurement and settled deliberately: `Nom` keys on `is_func`, so an
accessor-only Kotlin file still reports `nom.functions == 0` — the
symptom the issue calls "worst of the set". What the missing *space*
caused is fixed (own metric scope, complexity off the class, flaggable
by `bca check`); the `nom` count is not, and the book and the tests now
say so rather than leaving it to be rediscovered.
WMC does move: it keys on `SpaceKind::Function` rather than on `is_func`,
so an initialiser's complexity now rolls into its class's WMC. That is
correct — WMC is the sum of a class's method-body complexities and an
initialiser body is real class complexity — but it is a published-metric
change. `kotlin_init_block` is updated from 1 to 2 with the reasoning
inline.
Prerequisite, and the reason this lands before the rest of the group:
`functions_metrics_parity` asserted that every *named* `SpaceKind::Function`
space is reported by `Ast::functions()`, with the literal `"<anonymous>"`
as its only escape hatch. A named space that is `is_func_space` but not
`is_func` fails that. The hatch is now a predicate over the
angle-bracket convention, and `get_func_space_name`'s default is
extracted as `default_func_space_name` so an override can special-case a
few kinds and delegate the rest.
Two grammar-dispatch traps, both commented at the point of use:
`src/getter/kotlin.rs` glob-imports `Kotlin::*` inside the function while
the `Getter` *trait* is in scope from `use super::*`, so the `Getter` /
`Setter` variants are written fully qualified; `src/getter/groovy.rs`
imports an explicit list, so a bare `StaticInitializer` parses as a
binding that matches everything.
Tests: four in `nameless_construct_spaces`, each asserting on **both**
the new space and the space it took the value from — asserting only the
new one passes even if the parent kept a duplicate count, which is what
#1160 found in the first draft of the equivalent Java fix. One pins that
two `static { }` blocks in a class produce two identically-named spaces,
so the collision reads as a decision rather than an oversight.
No corpus snapshot moves: the integration corpora contain no `.kt`,
`.java` or `.groovy` files and no JS class static block.
Refs #1184
`GeneratorFunction` and `GeneratorFunctionDeclaration` were
`SpaceKind::Function` to the `Getter` and *closures* to the `Checker`,
and every metric keying on one or the other inherited the disagreement.
`function* g() {}` is lexically a function declaration — the `*` is a
modifier, not a different binding form — so the Getter was right.
function outer(a,b){ if(a){ if(b){ function* gen(c){ if(c){…} } }}}
// cognitive 3 -> 2, matching the byte-equivalent non-generator
// nom: functions 0 / closures 1 -> functions 1 / closures 0
// nargs: the parameter moves from closure_args to fn_args
The two kinds are **not** symmetrical, which the issue's "move both"
framing misses and which the grammar settles:
`generator_function_declaration` has a *required* `name` field and
mirrors `function_declaration`, so it is unconditionally a function;
`generator_function` has an *optional* one and mirrors
`function_expression`, so it routes through `check_if_func!`. Making it
unconditional would classify `run(function*(){})` as a function while
`run(function(){})` stays a closure — a new inconsistency in place of
the old one.
Both cognitive halves move together, as the issue's comment requires,
because they are independent:
- the **boundary arm** decides whether the generator resets its own
inherited nesting (`outer2`'s `gen`: 3 -> 2);
- the **`stops` list** decides whether a plain `function` nested *inside*
a generator gets a depth surcharge (`wrapper`'s `inner`: 1 -> 2).
Fixing only one leaves the other reproducer wrong. The 14-line comment
describing generators as a knowing omission is replaced with what the
arm now does and why the two kinds are gated differently.
This also repairs a latent parity violation: `function* g(){}` opened a
Function space named `g` while `is_func` said false, so `bca functions`
and `bca find --type function` did not report it. No fixture reached it,
which is the only reason `functions_metrics_parity` was green — the JS
fixture now carries a generator, and reverting the classification fails
that test by name.
Tests: `check_js_function_boundary` gains generator cases in both its
loops, so all four instantiating languages cover them — #1159's tests
covered none, which the issue's comment calls out. Verified by
perturbation: restoring the old classification fails all four
per-language boundary tests plus the parity test.
No corpus snapshot moves, and worth stating precisely rather than as a
bare claim: exactly one file across all three corpora contains a
generator (`pdf.js/src/core/xfa/template.js`), and it is in
`pdf_js_test.rs`'s exclude list under the #84 parse-error FIXME. Its
checked-in snapshot is therefore stale with respect to this change but
is never compared.
Fixes #1186
`nesting.lambda` was reset only by the JS macro, so every other language
carried an enclosing closure's surcharge into a function *declared
inside* it. The same body scored differently depending on whether
something two levels up happened to be a closure:
fn outer(a: bool, b: bool) {
let f = || { if a { fn g(b: bool) { if b { … } } g(b); } };
f();
}
// `g` scored 3 inside the closure, 2 outside it
Measured at 3-vs-2 in Rust, Java, C++, PHP — the four the issue names —
and in **C#**, which it does not, and where a `LocalFunctionStatement`
inside a lambda is idiomatic rather than contrived.
A function declaration opens a fresh lexical scope whatever encloses it,
so the reset belongs to every boundary. It moves into
`enter_function_boundary`, which is what stops a language opting out by
accident — the way the gap arose. The `js_cognitive!` arm becomes a
plain caller and its 8-line justification for spelling the statements
out longhand is deleted rather than edited: it is now false.
Audited all 18 callers. No language is made wrong and none double-fixes:
in every module the lambda-increment kind set and the boundary kind set
are disjoint, so a single node can only take one arm and the order of
operations inside the helper cannot matter. Three near-misses worth
naming because the identifiers are close but the `kind_id`s differ —
Lua's `FunctionDefinition` (lambda) vs `FunctionDeclaration*` (boundary),
Kotlin's `AnonymousFunction` vs `FunctionDeclaration`, PHP's
`AnonymousFunction`/`ArrowFunction` vs `FunctionDefinition`/`MethodDeclaration`.
Elixir is the one boundary that cannot use the helper — it takes the
resets without the depth bump, for the documented `Call`-ancestor
reason — so it gains the line by hand, with a note that staying in step
is now its own responsibility.
The JS tail from #1159 lands here too: `ArrowFunction` joins the `stops`
list, so `(function(){ function g(){…} })()` and
`(() => { function g(){…} })()` both charge `g` a depth of 1 where the
arrow form charged 0. It cannot double-charge, since the sole caller
resets `lambda` first.
`javascript_function_depth_and_lambda_are_distinguishable` moves 1 -> 2
and its rationale is re-derived: the doubled arrow is still load-bearing
for its original parity reason, and the test still discriminates a field
transposition — writing `function_depth = 0` in place of `lambda = 0`
scores 4 against the expected 2. `javascript_arrow_contributes_lambda_nesting`
is unaffected. Python's boundary comment claimed only the JS macro
carried the extra line; rewritten.
Tests: `a_function_declared_inside_a_closure_scores_the_same_as_outside`
in the cross-language parity suite pins the rule once rather than per
module, asserting the paired fixtures agree *and* that the baseline is
still 2 — an equality alone would survive a regression that moved both
halves.
One corpus snapshot moves, from the arrow-in-`stops` half:
`accessibility_spec.js`'s `getSpans` sits inside a
`describe(..., () => {…})`, so it gains depth 1 and the arrow nested in
it inherits it. Cognitive fields only. Submodule bumped and pushed here.
Fixes #1187
`check_if_func!` and `check_if_arrow_func!` both answer "is this
expression bound to a name, or used positionally?", and they answered it
with two different ancestor walks. The issue reports one divergence;
there are four, each measured:
1. **`$stop`**: `Arguments` for functions, `CallExpression` for arrows —
different tree levels, since `call_expression > arguments >
function_expression`. An IIFE's chain is
`parenthesized_expression -> call_expression` with no `arguments` node
on it, so the function walk ran past the call and reached the binding:
`(function(c){})(1)` was a closure and `const v = (function(c){})(1)`
a function. The same construct, classified by what happened one level
up past the call. This is the reported bug.
2. **`$up`**: `Pair` in the function list only, so
`({ "k": function(){} })` was a function and `({ "k": () => {} })` a
closure. The arrow agreed for a *bare* key alone, and by a different
mechanism — `$extra`'s `property_identifier` sibling — which a string,
number or computed key does not provide.
3. **`$extra`**: `has_sibling(PropertyIdentifier)` in the arrow only, the
exact mirror: `class C { p = () => {}; }` was a function and
`class C { p = function(){}; }` a closure.
4. **`$extra`**: `is_child(Identifier)` in the function only — and this
one **must stay**. For a function expression the only possible
`identifier` child is its optional name, so `run(function g(){})` is
rightly a function; an arrow stores its un-parenthesised single
parameter in exactly that position, so unifying it would make
`run(x => x)` a function. That is the commonest callback shape in any
JS corpus.
The first three are unified, which makes an IIFE a closure in both
spellings — matching the arrow behaviour, and matching how a reader reads
it. `$up` and `$stop` are now token-identical; only `$extra` differs, for
the reason in (4), recorded at the macros.
Knock-on, taken deliberately: the cognitive boundary arm is gated on
`Self::is_func`, so a reclassified IIFE no longer resets its own
`nesting.conditional`. That coupling is the documented invariant — the
Checker and cognitive disagreeing would be the larger bug — so it stays.
Tests: `check_js_binding_site_parity` runs in all four instantiating
languages (the macros are shared but the token ids are not) and asserts
the two spellings agree for each divergence, plus both directions of the
one that must not unify. Verified by perturbation: reverting each of the
four independently fails the case that names it, in every language.
Reinstating `is_child(Identifier)` on the arrow fails
`run(x => x)` — the guard against a future over-eager unification.
`javascript_nom` / `mozjs_nom` move: their fixture is the D1 case
(`var bar = (function(){…})()`), now two functions and two closures
rather than three and one. Four pdf.js corpus files move, all pure
reclassification — `functions + closures` constant in every pair, no
field outside nom / nargs / cognitive touched. Submodule bumped and
pushed here.
Fixes #1188
The five nameless constructs open a `FuncSpace` since #1184a, but reached no cognitive boundary arm, so each inherited the enclosing conditional nesting instead of restarting it. A Kotlin property accessor nested two `if`s deep scored 7 where the ordinary method beside it — same body, same position — scored 5. This is the second half of #1160's pattern: `is_func` / `get_space_kind` fix the space, the cognitive arm and the `stops` list fix the score. Held until last in this group because it edits the same `js_cognitive!` region #1186, #1187 and #1188 rewrote. Kotlin, Java and Groovy match kinds directly, so the constructs join the existing arm and its `stops` list. JavaScript needs its own *ungated* arm: the class static block is deliberately outside `is_func` (#1184a), and the JS boundary arm is gated on `Self::is_func`, so adding the kind there would never fire. It is in `stops` for the same reason a `function_expression` is — a `function` declared inside a `static { … }` really is nested in one. Tests: `nameless_construct_boundaries` compares each construct against a *sibling method* with a byte-identical body in the same position, so the assertion states the property rather than a number that moves with any unrelated re-tuning; the absolute 5 is pinned alongside so a regression moving both equally still fails. Two levels of `if` are load-bearing, and the issue's own checklist says so. A first draft used one level and reported the same number with and without the fix — it could not discriminate, and I only noticed because the perturbation run came back green. A second false negative followed: the first perturbation attempt silently matched nothing, because rustfmt had collapsed the arm onto one line after I wrote the pattern. Both are worth recording, since each looked exactly like a passing verification. With the perturbation actually applied, the Kotlin accessor scores 7 against the method's 5 and the test names it. Fixes #1184
#1183 proposed converging this repository's `nargs` limit from 7 onto the shipped default of 5, and prescribed sampling a dozen offenders before committing to the refactors. Measured all 76 that a limit of 5 would newly gate, and the result is a third answer — neither "threaded context" nor "genuine god-functions": | | count | |---|---| | newly gated at `nargs = 5` | 76 | | with **6+ parameters of their own** | **17** | | carrying closure args | 59 | | with four or fewer own parameters | 44 | The gate extracts `nargs.total()`, which sums a function's own parameters with every nested closure's. `Parser<T>::filters` has one parameter and five from closures in its body; `write_top_offenders` has three and scores 6 through a sort comparator and a format closure. Converging would therefore pressure contributors to delete idiomatic inline closures for 59 of the 76, which is not what the metric is for. That is a property of the extractor rather than of this codebase, so it is not a case for the per-language override in #1141 either. Filed as #1196, since every adopting project inherits the same semantics through the shipped default, and #1140's per-language corpus figures were derived from the same summed value. Replaces the previous rationale, which framed the choice as "stay at 7 or do the real 6 -> 5 work" — the evidence supersedes both halves. Refs #1183, #1196
`bca check` over the batch's changed files reported four offenders that
are mine. Each is resolved on its own terms rather than by baselining
the lot.
**`get_func_space_name`, cognitive 16 (limit 15), four JS-family
modules.** #1184a added an early return and pushed it over. Genuinely
flattened instead: the `Pair` and `VariableDeclarator` arms differed
only in *which field* carries the name, so they collapse to one lookup,
which removes a nesting level and the duplication together. The
`if let … else { if let … { match … } }` staircase becomes two early
returns and a `map_or`.
**`EffectiveConfig::from_resolved`, halstead.effort 52857 (limit
50000).** #1194 added four fields to a 27-field struct literal. The
effort is entirely operand vocabulary — 71 unique operands, one per
field name, and no decisions at all — so the number is an artifact of
flat initialisation rather than of anything a reader must follow. Split
on the boundary a reader would already draw: the function built two
independent types, and `EffectiveCheck` now has its own constructor.
**`tcl_inspect_container` / `irules_inspect_container`, cognitive 19.**
Not refactored. These are the same wrapper-peeling state machine as
`cpp_inspect_container`, which already carries `bca:
suppress(cognitive)` with the rationale that the boolean-context flag
must be readable at every step so any split has to thread it back out.
#1180 pushed the two siblings over the limit by giving them the seed and
the wrapper peel every other language already had. They get the same
marker, pointing at the same reasoning.
Behaviour is unchanged throughout: 5117 tests pass, including the 450
getter / nom / cognitive tests that cover the flattened function.
The remaining offender is `markdown_report/hotspot.rs`'s file-level
`loc.ploc`, which is a baseline refresh rather than a code question —
handled separately with the rest of the baseline.
Four read-only reviewers (correctness over the library, over the CLI and tooling, a test audit, and a reuse/modernisation pass) went over `main..HEAD`. Their findings, resolved. **Two regressions this batch introduced, both caught by review rather than by its tests, both now pinned.** `!($a)` scored zero in Tcl and iRules. #1180's negation branch kept a fixed `child(1)`, and `_expr` inlines `( … )` as anonymous children, so the index landed on `(` and the walk stopped. The bare form counted and the parenthesised one did not — an inconsistency the fix itself created, since neither counted before it. Both branches now take the first *named* child. The parenthesised fixtures added with #1180 covered the ternary *condition* slot only, which is why this passed. `function () {…}.bind(this)` became a function. #1188's fix for the class-field divergence used `has_sibling(PropertyIdentifier)`, which tests every sibling rather than a binding position, so a member expression's property name supplied one — re-creating the func/arrow divergence #1188 exists to remove, in a new shape, and moving real pdf.js counts. The binding site is now named structurally: the field-definition kind joins `$up`, threaded per language because JS and TS spell it differently (`FieldDefinition` / `PublicFieldDefinition`). Two corpus snapshots return to their pre-batch values exactly. **A gate this branch was failing.** `make rustfmt-bail` was red: five comments sat *inside* a match pattern, which makes rustfmt emit the enclosing match verbatim while `cargo fmt --check` still exits 0 — the exact defect `.claude/rules/formatting.md` documents, introduced while adding the #1184 arms. Hoisted above the arms, as the rule requires. **Documentation that contradicted the code.** `GlobalOpts::exclude`'s clap doc still described the #1189 bug as current behaviour, and clap renders it into `--help` and 14 man pages; corrected and `cargo xtask` re-run in this commit. The `js_cognitive!` rationale still said `ArrowFunction` was "knowingly absent" from `stops` two lines above the slice containing it. `bca.toml` still offered "do the real 6 -> 5 work" as an option the paragraph below it rules out. `benchmarking.md` named `Node::previous_sibling_under` in the present tense after #1181 removed it. **CHANGELOG.** `STABILITY.md` requires every metric-value drift to be called out; nothing was recorded. `[Unreleased]` now carries all eight drifts and the two additive changes. **A performance regression in #1189.** `relative_tail` canonicalised the manifest root per call, and the exclude match runs per *walked file* — so a project reached through a symlink, or `bca diff`'s before-side (which moves the cwd away from the manifest), paid two syscalls per file. The root is fixed for a run, so it is canonicalised once and passed down; the per-file `path.canonicalize()` remains only as the last resort it always was. **Test quality.** `check_js_binding_site_parity` asserted only the function count, so a regression dropping a construct from `is_func_space` entirely — zero functions *and* zero closures — passed every negative row; it now asserts the pair. `is_synthesised_name` matched any `<…>`-shaped name, which would have exempted Ruby's `def <=>`; the five names are enumerated. Two assertions that could not fail independently (`assertIn("1", …)` against a header containing "1192", and `kept_offender` as a substring of `nested_kept_offender`) are replaced with ones that can. Not fixed here, filed instead: the #1184 spaces emit `npm`/`npa` blocks their sibling methods do not (#1197). The enable predicate reads `is_func_space` as "is a container", and correcting it needs the same per-language threading as above — not a rushed edit at the end of a large batch.
`make pre-commit` was failing at `_pc-self-scan`: `markdown_report/hotspot.rs` grew past its recorded `loc.ploc` of 650 to 696 when #1190 moved the shared selection half into it from the two renderers. `AGENTS.md` requires the refresh in the same change as the metric move, and it was outstanding. Refreshed with the headroom variant, so functions in the 95-100% band are recorded and the soft tier does not re-fire on untouched files. The file shrinks: 183 entries to 181. Several recorded values drop because this batch's refactors genuinely simplified their subjects — `EffectiveConfig::from_resolved`'s nargs 12 to 7 after the `EffectiveCheck` constructor was split out, and two `loc.ploc` entries by 23 and 9 lines. Three entries are new, and all three are the cost of an extraction rather than new complexity: `EffectiveCheck::from_resolved` (nargs 10), `select_for` (7) and `relative_tail_with` (5). Extracting a helper moves a parameter list into existence — the file-level artifact `AGENTS.md` warns about under "Responding to bca metric feedback" — and in each case the alternative was leaving the duplication the reviews asked to remove. `select_for` is the clearest instance: seven parameters exist because it owns the whole format-independent half that both renderers previously spelled out.
`bca check --threshold nargs=N` read `nargs.total()`, which is `function_args_sum() + closure_args_sum()` — subtree sums — so a function was gated on its own parameters *plus every nested closure's*. `write_top_offenders` declares three parameters and was reported at 6, because a sort comparator and a format closure contributed three more. The remediation the number implied, fewer parameters, was not the one that would clear it. Measured on this repository while working #1183: of the 76 functions a limit of 5 would have newly gated, only **17** had six or more parameters of their own. 59 carried closure args and 44 had four or fewer own parameters; the extreme was `Parser<T>::filters` — one parameter, plus five from the closures in its body. Converging the limit would have pressured contributors to delete idiomatic inline closures rather than simplify signatures. The survey on #1196 found no comparable tool doing this. RuboCop `Metrics/ParameterLists` (verified in source: `on_args` returns early for lambdas and procs), ESLint `max-params`, Clippy `too_many_arguments`, lizard, SonarQube S107 and Pylint `R0913` all count one callable's own formal parameters. Two of those are the anchors `default_thresholds.rs` derives the shipped limit of 5 from, so the default was calibrated against a different quantity than the gate enforced. The book's own NArgs definition — "arguments declared by *a* function, method, or closure" — already agreed with the precedent rather than with the gate. Nothing escapes the narrower rule. In the ten grammars whose closures open their own space a closure is gated on its own offender row, which is where its fix belongs — verified: a six-parameter Rust closure inside a one-parameter function is flagged as `small::<anon@L2>`, not as `small`. In Python, Java, Kotlin and C++/Mozcpp a lambda opens no space, so its arguments can only be attributed to the enclosing function; there the row now carries the split, `nargs = 8 (1 own + 7 lambda)`, so the reader can tell whether the lever is the signature or the lambda. The split is appended rather than substituted, so the value stays where tooling parsing the row expects it, and it is omitted when there is no lambda term to disclose. `NargsSplit` is a struct rather than a `(u64, u64)` because the two fields are same-typed, not interchangeable, and transposing them would print a fluent lie. Effect on this project: refreshing the baseline retired 45 of its 61 recorded `nargs` entries (181 entries to 137 overall). Those were never real debt. Tests: four end-to-end cases in `check_thresholds.rs` covering own-args gating, the closure-on-its-own-row guarantee, the split where a lambda opens no space, and its absence for a plain parameter list. Verified by perturbation — restoring `total()` fails the two that assert gating, while the two that assert rendering correctly do not move. Note that the old behaviour had **no** test at all: the whole suite passed unchanged when the extractor was first swapped. Docs: the book's NArgs section gains a "what the threshold gate measures" subsection distinguishing the gate's reading from the serialized subtree sums; the threshold recipe keeps its #1143 cluster-trap narrative but records that those figures predate this change and re-measures the same tree (7 -> 5 now costs 17 new hard-tier offenders rather than 77). The `MetricScope::Function` doc no longer claims every metric in that scope includes nested closures — `nargs` now does not, while `halstead.*`, `nexits` and `tokens` still do. Scope note: #1196 and my report on it both said the #1140 per-language percentile table would need re-deriving. It would not — that table carries `cognitive`, `cyclomatic`, `abc`, `halstead.effort` and `loc.ploc`, and no `nargs` column. The one corpus-derived `nargs` claim in the book is annotated instead, since re-measuring needs the 20-language corpus that is not in this repository; the claim's direction of change (down) only strengthens its conclusion. Fixes #1196
Two behaviour bugs, both introduced by this branch, both reproduced against its own binary before fixing. **`bca report` was not moved with the gate.** #1196 changed the threshold extractor to own-parameters and left `FunctionSummary.nargs` on `m.nargs.total()`. Both surfaces read the same `[thresholds] nargs` key — the report through `AdvisoryThresholds::from_manifest_hard` — so on one fixture `bca check --threshold nargs=4` exited 0 while `bca report` flagged the same function under "Many parameters", for a number the gate no longer enforced. Aligned. **`check_if_arrow_func!` never received the field-definition kind.** #1188's third divergence was fixed for `check_if_func!` only, so the class-field split survived wherever the field name is not a bare identifier: `class C { ["k"] = function(){} }` was a function and its arrow spelling a closure, likewise for `"s" =` and `#p =`, in all four JS-family languages. The comment above the macros claimed `$up` and `$stop` were already token-identical; they were not. The parity test exercised `p = …`, the one spelling that worked. Both macros now take the kind, and the test covers all four spellings. **The `own > 0` guard hid the split where it mattered most.** A function declaring no parameters at all rendered a bare `nargs = 7` when a spaceless lambda put it over — the misleading row #1196 exists to remove — while its one-parameter sibling got the annotation, making the omission look deliberate. The counts cannot separate that from a closure's own space, which is shaped identically (`0` own, `N` lambda) and correctly wants no split; the subject can, since a closure space carries no name. **Documentation that asserted things the code does not do.** The `--exclude` help I rewrote for #1189 claimed CLI globs resolve against the working directory and that manifest globs "anchor the same way" — measured, `bca check nested -X './nested/skipme/**'` excludes nothing while `'./skipme/**'` does, because the CLI half still matches the walk-root form. Rewritten to describe both anchors accurately, with `man/` regenerated. `bca.toml` still described #1196 as unsettled after the commit that settled it. The spaceless-closure language list omitted Groovy (verified: `K::small: nargs = 7 (1 own + 6 lambda)`). The accessor-convention block still filed `nargs` under subtree sums, three lines above the extractor that no longer reads one. Kotlin's `is_func_space` rationale implied a `nom.functions` fix that #1184 deliberately did not make. A C# comment still described the positional ternary reads #1181 replaced. **A real pre-existing lexer defect.** `find_macro_call_end` skipped line comments, strings and char literals but not block comments, in both directions: a `"` inside one opened a runaway span so the body ran into the next call and stole its `@"…"` anchor (under-count, 0 for 1), and a `)` inside one truncated the body before its own anchor (over-count, a spurious CI failure). `scan_ignore_spans` always handled them. Also `char_literal_end` missed `\xNN`, and the comment asserting `\u{}` was "the only multi-char escape" was wrong — benign for the count, but that comment is what would let the next reader skip it, this lexer having already been copied once. **Dead code that read as coverage.** The Tcl/iRules boolean-context seed carried a ternary-condition disjunct that can never fire: the flag is read only after a peel, `Expr` cannot sit under `ternary_expr`, and a `!`-unary sets the flag itself. Verified by deletion. It also cost two full child scans per ternary to produce `false`. Removed, with the reason it is live in the C family recorded. **Tests.** `seen_table` could not fail as intended — a fully-suppressed section emits the same `### {title}` heading, so a fixture exercising only that arm satisfied the guard. Two ABC baseline tests lacked the `checked > 0` guard their siblings carry and asserted nothing on a minimal-features build. `irules_abc_parenthesised_…` compared two values with no absolute anchor. `test_byte_raw_string_…`'s docstring claimed an end-to-end discrimination it does not have — measured, the `b` branch changes no count anywhere in the workspace — now stated honestly. Added: the four field-name spellings, the zero-own split, a closure's own row, both block-comment directions, `regular_string_end`'s escape branch and `scan_ignore_spans`' comment nesting (the last two extracted by #1192 and untested until now), and `\xNN`. Every new test was verified by perturbation. Note one of those perturbations produced a syntactically invalid file, so the module failed to import and unittest printed no failure line — which reads exactly like a non-discriminating test. Confirming the perturbation applied *and* still parses has to come before reading the result. Baseline refreshed; 137 entries, unchanged in count.
Closes the step #1183 opened. It was declined twice before and both refusals were right at the time: #1143 measured 7 -> 6 against a hard-tier count and missed that the proportional soft tier would park 74 functions permanently in the band, and #1183 measured 6 -> 5 and found the offenders were mostly not parameter-heavy at all — the gate summed a function's own parameters with every nested closure's, so 59 of 76 would-be offenders were artifacts. #1196 removed that, and with it the reason to stay. Staying would also have left a limit that caught nothing: at 7 there were six offenders and all six were already baselined, which is the "cannot catch anything" argument the book uses against the old shipped default. Absorption ratio, in AGENTS.md's order of preference: **4 fixed, 0 suppressed, 129 baselined.** That is overwhelmingly baseline, and per #1183's own procedure the ratio is the finding rather than a footnote — see below. Fixed: the four HTML section writers took `(out, headings, id_prefix)` as their first three arguments. Those jointly answer "which language's section am I appending to", no caller holds one without the others, and an `HtmlSection` bundle is the refactor the parameter count was pointing at rather than a split invented to move a number. Behaviour-preserving; the report snapshots are untouched. Not fixed, deliberately: ten `dispatch_*` functions share `(language, source, path, pr)` — `parse_ast` takes exactly those four, so a bundle is clearly right there too — but only three of the ten breach. Bundling three would leave the router inconsistent, and bundling ten is a subcommand-router refactor that does not belong inside a threshold change. It is the obvious follow-up. The baseline roughly doubles, 137 -> 247 entries, 126 of them `nargs`. One correction to the reasoning recorded in #1143 and repeated in the book, found by measuring rather than reasoning: the cluster effect is **structural for every integer metric**, not a property of landing on a population's mode. A function sitting exactly at limit L is always above 0.95 x L, so the ten functions at exactly 7 were in the soft band before this change just as the ninety-one at exactly 5 are after it. The limit choice controls only how many sit there. #1143's objection was sound because the hard-tier gain was zero, not because 6 was near a mode. The corollary is that the proportional soft tier carries little information for a small-integer metric: at `halstead.effort` it is a real band (50000 vs 47500), at `nargs` it is 5 vs 4.75 and flags every compliant function at the ceiling by construction. Most of the 110 new soft-tier entries are that artifact rather than debt. Recorded in the book; worth taking back to #1140. Refs #1183, #1196
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1198 +/- ##
==========================================
+ Coverage 98.17% 98.25% +0.08%
==========================================
Files 276 276
Lines 70896 71570 +674
Branches 70466 71140 +674
==========================================
+ Hits 69600 70322 +722
+ Misses 875 827 -48
Partials 421 421
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
`promotes_to_func_space_with_code` spelled out the whole of `is_func_space_with_code` a second time rather than calling it. The two bodies were identical, and only the override was reachable from the walk, so no test could observe the copy it was not using — the pair could drift apart silently. Forwarding to `is_func_space_with_code` keeps the single-lookup saving the override exists for: the default is `is_func_with_code || is_func_space_with_code`, and the second predicate already subsumes the first (its final clause *is* `is_func_with_code`, since a method macro outside a `quote` block satisfies both), so one `elixir_call_keyword` call still answers the combined question. Verified live by perturbation: making `is_func_space_with_code` return `false` unconditionally now fails the Elixir ABC and space tests. Before this change the same perturbation failed nothing.
The `Display` impl was entirely unexercised, yet it is the only rendering of these diagnostics a user sees — the CLI writes it to stderr and `bca-web` may surface it verbatim. Each variant's text is asserted whole rather than probed with `contains`, which is what catches a dropped path interpolation; a substring check passes against both. The cycle case uses three unsorted members, one containing a space, so it can distinguish "prints every member" from "prints the first" and pins the quoting that exists to keep whitespace visible. Records without changing the shipped `Warning:` / `warning:` split across the five variants, so normalising it later is a reviewed diff rather than an accident. Verified by perturbation: dropping the `file.display()` interpolation from `SelfInclusion` fails the test.
`is_transient_object_miss` decides whether the post-blame commit lookup is retried, and was not executed by any test. Getting it wrong is silent in both directions: too narrow and the #579 ODB race resurfaces as a hard failure, too wide and a genuinely-absent or wrong-kind object burns the whole retry budget before reporting what was actually wrong. The predicate is a two-level `matches!`, so supplying only the retryable value would pass against `|_| true`. Each rejectable shape is constructed too, one per arm the pattern can reject: the sibling `Find` variant for the inner arm and `Convert` for the outer. Verified by perturbation in both directions — the test fails against a predicate hardwired to `true` and against one hardwired to `false`.
`preproc_diagnostic_display_renders_each_variant` asserted four of the five variants — the multi-line `IncludeCycle` needs a different assertion shape and lives in the sibling test — while its doc claimed each variant was pinned. A name that overstates coverage is how a gap gets read as a guard. Renamed to `…_renders_each_single_line_variant`, with the doc pointing at the sibling test and scoping the `Warning:` / `warning:` note to all five variants taken together, which is where that three-versus-two count actually holds. Name and comment only; both tests are unchanged and still pass.
`PreprocDiagnostic` shipped two spellings: `SelfInclusion`,
`IncludeCycle` and `NotPreprocessed` capitalised `Warning:` while the
two non-UTF-8 variants did not. `bca preproc` prints all five verbatim
via `eprintln!("{diagnostic}")`, so both reached users from one command.
The capitalised three also matched no other CLI diagnostic: `warn()`
has rendered lowercase `warning:` since #609, alongside `note:`.
Lowercase is therefore the convention, not a coin toss.
Chosen downward rather than upward on purpose. `warn(msg)` expands to
`eprintln!("warning: {msg}")`, so with the prefix lowercased the
current bare `eprintln!` and a future `warn()` call emit identical
bytes — moving the prefix out of `Display` and onto the CLI's severity
ladder becomes a pure refactor with no output diff. Flipping upward
would have made these five the only capitalised warnings in the CLI and
forced the same decision again later.
Not a breaking change: STABILITY.md holds `Display` impls stable but
explicitly excludes their exact wording. `output/csv.rs` and
`output/offenders.rs` still self-prefix capitalised; that and the
`warn()` routing are left to the follow-up issue.
dekobon
added a commit
that referenced
this pull request
Aug 6, 2026
Review follow-ups on #1199: - The gate flagged capitalised literals inside multi-line raw-string fixtures, where neither `diag-prefix-ok` position is reachable — both land inside the fixture, so the marker would alter the text a metric test measures. Skip those interiors. Verified across all 559 tracked files that the skip hides zero would-be hits. - The naive raw-string opener read `strip_suffix(b"\r")` and `Ruby::R => "r",` as opening a raw string and skipped to the next quote — a false *clean*, the outcome the gate exists to prevent. Excluding `"` and `\` from the lookbehind cuts candidate multi-line opens from 99 to 27, all genuine fixtures. Both shapes are pinned. - `re.search` reported one offender per line, so a two-offender line was under-reported and the header undercounted. Use `finditer`. - An escaped inner quote (`"he said \"Error: no\""`) was flagged, contradicting the documented prose exemption. Rejected by lookbehind. `walk.rs`'s two explicit-path notices carried the `bca: warning:` double prefix that #609 removed elsewhere — named as "the old redundant double prefix" in `paths_discovery.rs:466` — which made this PR's own AGENTS.md rule false on arrival. Migrated onto `diag::warn`, with the three assertions updated and a guard pinning its absence. The CHANGELOG carried #1198's entry claiming a lowercase prefix and byte-compatibility that this change supersedes; both would have shipped in one release. Folded into one entry stating the end state.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes fifteen issues (#1180–#1182, #1184–#1192, #1194, #1195), plus
#1196, which came out of working #1183.
make pre-commitis green on the tip. Coverage rose on every measure bycovered count (lines 86,828 → 87,623; functions 9,398 → 9,479; regions
99,831 → 100,677).
The issues were often wrong, and measuring first was the point
Nine of the fifteen had their framing corrected. The ones worth knowing
about before reviewing:
child_by_field_name("condition")ternary_exprexposes no fields, andwhilehas noconditionfield. Slots are located relative to the?/:tokens. Three further gaps blocked the routing entirely.nom.functions == 0is the worst symptomis_func_space-not-is_funcdesign does not fix that number — documented rather than papered overidentifierchild is its parameter, so unifying it would reclassify every single-argument callback(1, line_count)#1183 → #1196
#1183 proposed converging this repo's
nargslimit 7 → 5. Measuring all76 would-be offenders showed the gate summed a function's own parameters
with every nested closure's — only 17 had six or more of their own,
and one had a single parameter plus five from closures in its body.
That is a property of the extractor every adopting project inherits, so
it became #1196: the gate now counts a callable's own parameters, which
is what RuboCop, ESLint, Clippy, lizard, SonarQube and Pylint all
measure — and what this limit's own rationale was derived from. Nothing
escapes: a closure that opens its own space is gated on its own row, and
where a lambda opens none the row shows the split,
nargs = 8 (1 own + 7 lambda).With that settled, the convergence landed. Absorption ratio: 4 fixed,
0 suppressed, 129 baselined — overwhelmingly baseline, which #1183's
own procedure calls a finding rather than a footnote.
Review found two regressions this branch introduced
Both reproduced against the branch's own binary, both now fixed with
tests verified by perturbation:
bca reportwas not moved with the gate. Same file, samebca.toml:bca check --threshold nargs=4exited 0 whilebca reportflagged the same function.
check_if_arrow_func!never received the field-definition kind, sofix(js): an IIFE is a function or a closure depending on whether its result is bound #1188's class-field divergence survived for every non-identifier field
name (
["k"],"s",#p). The comment claimed the two macros werealready token-identical; they were not.
Review also caught a gate this branch was failing (
rustfmt-bail, fromcomments inside match patterns), help text that shipped the #1189 bug's
description into 14 man pages, and a genuine pre-existing lexer defect in
find_macro_call_endcausing both a silent under-count and a spurious CIfailure.
Two corrections to reasoning recorded in the repo
property of landing on a population's mode. A function at exactly
Lis always above
0.95 × L, so the ten functions at exactly 7 were inthe soft band before this change just as the ninety-one at exactly 5
are after. chore(self-scan): converge repo thresholds onto the shipped defaults #1143's objection was sound because the hard-tier gain was
zero, not because 6 was near a mode. The corollary: the proportional
soft tier carries little information for small-integer metrics, so
most of the 110 new soft entries are that artifact rather than debt.
cognitive,cyclomatic,abc,halstead.effortandloc.ploc, andno
nargscolumn. question(nargs): the threshold gate sums a function's closures' parameters into its own #1196's body claimed otherwise. The onecorpus-derived
nargsclaim is annotated instead, since re-measuringneeds a corpus not in this repository.
Metric drift
Every value change is recorded in
CHANGELOG.mdperSTABILITY.md.Summary: ABC conditions (#1180–#1182),
nom/nargsfor generators andIIFEs (#1186, #1188),
nargsfor Java/C# lambdas (#1185), cognitiveacross five languages (#1187), Kotlin WMC (#1184), the file-level unit
span (#1195), and the
nargsgate semantics (#1196).Four corpus snapshots move, all pure reclassification. The submodule is
bumped and pushed; the recorded SHA is reachable on its remote.
Follow-up: three unexercised production paths
Added after reading this PR's Codecov report. Codecov shows 98.20%, but
46,199 of its 71,578 instrumented lines are inline
#[cfg(test)]bodies, which are 99.43% covered by construction. Production-only
coverage is 95.98%. A further 130 of the gap lines are
#[ignore]dGroovy tests blocked on upstream grammar limitations — the two largest
"gaps" in the whole report, and permanently uncoverable.
Three real gaps came out of that, each verified by perturbation:
refactor(elixir)—promotes_to_func_space_with_codespelled outthe whole of
is_func_space_with_codea second time rather thancalling it. Only the override was reachable from the walk, so no test
could observe the copy it was not using and the two could drift apart
silently. Now a delegation, which keeps the single-lookup saving the
override exists for. Breaking
is_func_space_with_codenow fails theElixir suite; before this it failed nothing.
test(preproc)— thePreprocDiagnosticDisplayimpl wasentirely unexercised, yet it is the only rendering of these
diagnostics a user sees. Each variant's text is now asserted whole
rather than probed with
contains, which is what catches a droppedpath interpolation.
test(vcs)—is_transient_object_missdecides whether thepost-blame commit lookup is retried, and was executed by no test.
Both rejectable shapes are constructed, so the test fails against a
predicate hardwired to
trueand against one hardwired tofalse.Covered lines:
checker/elixir.rs37 → 44,preproc.rs328 → 346,vcs/git/blame.rs78 → 82. Notechecker/elixir.rs's percentage fell(63.79% → 61.97%) while its covered count rose — the delegation made a
previously-uninstantiated function live, so its lines joined the
denominator. Judge that file by covered count, not by the column.
Deliberately skipped: covering Elixir's
is_call, which would mean atest that re-asserts a one-line
kind_id ==comparison. It moves thenumber and guards nothing.
Left open deliberately
npm/npablocks their siblingmethods do not. Needs the same per-language threading as fix(js): an IIFE is a function or a closure depending on whether its result is bound #1188.
returnslots — outside fix(abc): wire Phase 2B slot routing for Tcl and iRules #1180's numberedscope, and wiring them needs a double-count decision the issue does not
make. The book row states precisely what is and is not routed.
dispatch_*bundle — ten functions share(language, source, path, pr), which is exactlyparse_ast'ssignature, but only three breach. Bundling three would leave the router
inconsistent; bundling ten is its own change.
book.
warn()— filed as refactor(diag): unify warning prefixes onto the warn() severity ladder #1199. ThePreprocDiagnosticcasing split is fixed here (all five variantsnow render lowercase
warning:, matchingdiag.rs's style(cli): unify diagnostic prefixes across the binary #609 ladder;STABILITY.md:158excludesDisplaywording from the stabilitycontract). What is deferred is moving the prefix out of
Displayand onto
warn(), plus the two capitalised stragglers inoutput/csv.rsandoutput/offenders.rs. The flip was madedownward precisely so that follow-up is byte-identical:
warn(msg)renders
warning: {msg}, so it becomes a pure refactor with nosecond user-visible change.