fix(metrics): eleven ABC, cyclomatic and self-scan fixes - #1476
Merged
Merged
Conversation
`super` is an operator only as a wildcard type bound (`List<? super T>`), where it denotes no value and mirrors `? extends T`'s `extends`. In receiver position it names a value and is an operand. Groovy listed `Super` in an ungated operator alternation; Java has carried the parent gate since #1380. No metric moves. The pinned dekobon grammar spells `'super'` in the `wildcard` production and nowhere else, so every super-reference already parsed as a plain `identifier` and was already an operand. The gate removes that grammar accident: a bump routing a reference to the `super` token now classifies it correctly instead of billing it as an operator. The gate's operand branch is unreachable under the pin and left untested — only error recovery on invalid Groovy (`List<? super>`) detaches the token from its `wildcard`, and pinning that shape would make the grammar's over-permissiveness the contract. The existing node census is the drift marker for it. Splitting `Super` out of the shared alternation adds two arms to a match that already bails rustfmt on in-pattern section comments, so the rustfmt-bail baseline is ratcheted 5 -> 7. Fixes #1419
`Baseline::classify` answers `Covered` across the whole non-breaching side of a recorded value, so an offender that measured *better* than its record is indistinguishable from one sitting exactly on it. The entry keeps suppressing violations up to a value the tree no longer produces — gate headroom nobody chose, in which a later regression hides — and nothing reddens until someone regenerates the file for an unrelated reason. `filter_by_baseline` now folds every `Covered` classification into a `StaleTally` and emits one aggregated stderr warning naming the count and the worst entry by relative drift. Direction comes from `breaches_limit` with its arguments swapped, so the tally and the ratchet cannot disagree about which way `mi.*` improves. Exit codes and the kept set are unchanged. This closes only half the class: a function that stopped breaching its limit altogether produces no violation, so it never reaches the matcher and its entry stays invisible here. Catching that needs a scheduled regeneration diffed against the committed file. Fixes #1465
Objective-C's runtime OS-version check parses to a dedicated `available_expression` node that reaches no comparison-token arm, so `if (@available(iOS 13.0, *))` scored zero ABC conditions where `if (a)` scores one. The same shortfall applied to a `&&` operand, a `while` / `for` condition, a ternary condition and a negated check. The node joins `cpp_bool_terminal_kinds!()`, the name-keyed set the C-family ABC walkers share. The wrapper is the entry rather than any child: one grammar rule spans both the `@available` and `__builtin_available` spellings and makes the `version` child optional, so only the wrapper is present for every form. The neighbouring `version_number` exclusion stands unchanged -- it is a fragment of this node's interior, never an operand. No C, C++ or Mozcpp grammar emits a node by that name, so the addition is inert for the other three languages sharing the set. That inertness was previously assumed; it is now pinned by a test that also asserts Objective-C still emits the kind, so the negative half cannot pass vacuously. Metric drift: Objective-C `abc.conditions` rises by one per `@available` / `__builtin_available` check in a boolean slot. The integration corpora contain no Objective-C, so no snapshot moves. Fixes #1457
Five alternatives of the grammar's `_expression` rule were absent from `groovy_bool_terminal_kinds!()`, so `if (l[0])`, `if (l?[0])`, `if (a?.b)`, `if (a??.b)` and `if (a.@b)` scored zero ABC conditions where `if (a)` scores one, in both the `if` predicate slot and the `&&` / `||` chain slot. C# counts its `element_access_expression` and Kotlin its `index_expression` / `navigation_expression`, so this was a per-language asymmetry, not a policy difference. `a?.b` was the worst: Groovy cyclomatic counts `?.` as a decision, so ABC sat two below its own decision count on an idiomatic predicate. Counting the wrapper double-counts no token: ABC's condition-token arm lists no navigation operator, and `?[` is its own token rather than a bare `?` the ternary-gated arm could see. The condition slot also routed every `unary_expression` to a peel that handled only the `!` spelling, so it claimed `~a` / `-a` / `+a` and dropped them. It now asks `groovy_wrapper_operand` which wrappers the peel unwraps rather than restating the list, the same divergence #1459 fixed in Kotlin, and the peel reads its operand by grammar field, so `if (! /*c*/ a)` scores like `if (!a)` instead of reading the comment. Metric drift: Groovy `abc.conditions` and `abc.magnitude` rise by one per such expression in a boolean slot. No integration snapshot moves; no corpus carries a Groovy file. Fixes #1466
`postfix_unary_expression` was in neither `csharp_bool_terminal_kinds!()` nor the wrapper peel, so `if (b!)` scored zero ABC conditions where `if (b)` scores one, and likewise `if ((b!))`, `if (!b!)`, `if (b!!)` and every `&&` / `||` operand spelled with the suffix. The condition slot recognised the shape and silently scored nothing for it — the fifth instance of that class after Kotlin's `is` / `in`, bare parentheses, Kotlin's infix `and` and its postfix `!!` / `as`. With nullable reference types enabled the suffix is ordinary notation, so ABC sat one below C#'s own cyclomatic decision count on idiomatic predicates. The suffix is type-preserving, so it inherits the slot's verdict rather than proving booleanness, and the wrappers chain. `b++` and `b--` share the grammar production and stay excluded as arithmetic; their tokens are already ABC assignments. No arm counts the `!` itself, so nothing is double-counted. The peel moves into `csharp_wrapper_operand` in the shape Kotlin and Groovy already use, and `csharp_count_condition` asks it which wrappers it unwraps instead of restating the kind list — the divergence #1459 and #1466 fixed in those two, where the slot claimed a kind the peel then dropped. The operator is identified by membership and the operand read at `child(0)`, which no `extra` can precede because the node starts there, so `b /*c*/ !` scores like `b!`. The paren and prefix arms keep their positional reads: the C# grammar gives all three kinds an empty `fields` map, so `if (! /*c*/ b)` and `if (( /*c*/ b))` still score zero. That is #1455, partially addressed here only in that the new arm adds no instance of it; the two existing ones are now pinned as measured gaps. Metric drift: C# `abc.conditions` and `abc.magnitude` rise by one per null-forgiving expression standing as a predicate or a `&&` / `||` operand. No integration snapshot moves — every `postfix_unary_expression` in the corpus is an `i++` or an `n--`. Fixes #1463
tree-sitter-c-sharp emits the `case` keyword token (id 114) from two productions: `switch_section`, a real arm, and `goto_statement`, where `goto case 2;` is an unconditional jump to one. Both C# ABC and C# cyclomatic counted the bare token, so a method whose only difference from a control was a `goto case` read one condition and one decision higher while having exactly the same arms. Both arms now gate on a `switch_section` parent through one shared predicate, in allowlist polarity: a `grammar.json` sweep of the pinned 0.23.5 finds `"case"` in exactly those two rules, and naming the one decision parent means a grammar bump that grows a third fails closed. Neither `Case` nor `SwitchSection` carries a numeric-suffix alias, and `switch_section` is not a hidden rule. `goto default;` needed no equivalent gate: `Default` is a distinct token neither metric counts, being the switch's unconditional fallthrough. Cognitive is unchanged — it models the construct on the `goto_statement` node as an unstructured jump, so a `goto case` remains a jump there and merely stops also being an arm. The two arms move in one commit because they measure the same token, not because a global law binds them: `conditions == cyclomatic() - 1` is an opt-in fixture property, and gating either side alone broke none of the 3,429 library tests, since no fixture outside the cognitive suite spelled `goto case` at all. `assert_fixture_spells` and its C# binding move from the `abc` test module to `test_support` so the cyclomatic test can anchor on the same kind-id counts. Metric drift: C# `abc.conditions`, `abc.magnitude` and `cyclomatic` each fall by one per `goto case`. No integration snapshot moves — the 39-file C# corpus contains no `goto case`. Fixes #1450 Fixes #1451
`return 1 if $x;` scored abc.conditions 0 where the block form
`if ($x) { return 1; }` scores 1, and likewise for `unless`, `while`
and `until`. The dispatcher had arms for the four block statements and
none for the `*_simple_statement` nodes the grammar emits for the
modifier spelling — the idiomatic Perl one, so the undercount was
systematic on real code.
Perl's cyclomatic dispatcher already counted all six modifier kinds,
so this was a straight ABC undercount against Perl's own decision
count rather than a disagreement between the two metrics.
The slot is read by the grammar's `condition` field and routed through
the shared condition classifier, so a compound predicate keeps its
sub-structure: `return 1 if $x && $y;` scores 2, as `if ($x && $y)`
already did. The `for` / `foreach` modifier is excluded — it iterates
a list and has no boolean test, which the grammar records by naming
that slot `list` rather than `condition`.
Metric drift: Perl abc.conditions and abc.magnitude rise by one per
`if` / `unless` / `while` / `until` statement modifier.
Fixes #1464
#1422 made a C# `when` guard a cyclomatic decision and an ABC condition slot, and argued from Rust that C# was the outlier. That was half true: Rust's cyclomatic saw the guard through the `if` keyword token, but no sibling modelled the ABC half and Java had neither. Each language now scores a guard as a slot, so every spelling contributes exactly one and a compound guard keeps its sub-structure. Java 21's `guard` was referenced by neither metric; Rust's `match_pattern` guard and Python's `case … if g:` had the decision but not the condition; Ruby's `if_guard` / `unless_guard` had neither. Elixir is the inverse — it counted the `when` token as a condition with no cyclomatic arm behind it — and its arm is gated on the guard's position, because the language has no guard production and a typespec's binding clause spells the same token. Both metrics share that gate, so the typespec stops scoring a condition too. Also closes the opposite-direction gap: a bare guard (`_ if b`) scored nothing, one below the arm's own decision count. Groovy and Kotlin are unchanged and untested: neither pinned grammar has a guard production, and Kotlin 2.1 guard syntax does not parse, so pinning its numbers would make that limitation the contract. Metric drift: Java, Ruby and Elixir cyclomatic gains one per guard, and wmc / mi move with it; abc.conditions gains one per non-operator guard in Java, Rust, Python and Ruby, and falls by one per Elixir typespec `when`. Three serde snapshots move. Fixes #1454
A literal in a boolean operand slot scored no ABC condition unless it
was numeric: `x || "default"` reported conditions 1 against `x || y`'s
2, and `if ("s")` reported 0 against `if (b)`'s 1. #1410 closed the
numeric half; the non-numeric literals were missing from every set the
numerics were added to.
Adds, each measured short against an identifier control in both walker
paths (the chain operand and the `if` predicate) before landing:
- JavaScript / Mozjs / TypeScript / Tsx: string, template_string,
regex, null, undefined, object, array
- Python: string, concatenated_string, none, list, set, tuple,
dictionary, ellipsis
- Lua: string, table_constructor
- PHP: string, encapsed_string, heredoc, nowdoc,
array_creation_expression, null, shell_command_expression, and
cast_expression — PHP was the only set in the Java / C# / Groovy /
PHP group naming no cast kind
- Groovy: string_literal, null_literal, list_literal, map_literal
A type keyword rendering to the same node-kind name as its literal
stays out, extending the rule PHP's `float` keyword established.
C#, Java, Kotlin, Rust and Go are unchanged: a bare literal in a
boolean slot is a compile error there. The C family carries the same
gap for string_literal and is deferred, being the one integer-truthy
group with corpus exposure.
The tests assert the kind_id each spelling parses to, not only the
resulting count — a set naming the wrong alias of a multi-id kind
keeps reporting the unfixed number, which no conditions comparison
can see.
Metric drift: abc.conditions / magnitude / value rise by one per
non-numeric literal operand in a boolean slot. 85 of 384 pdf.js
snapshots move, conditions-family only and all upward; no other
corpus moves.
Fixes #1462
A construct a grammar spells as its own production -- C#'s two `is` tests, Java's and Groovy's `instanceof`, Groovy's `in`, Kotlin's `is` / `in`, Ruby's one-line `in` -- had no operator token to count, so it reached `stats.conditions` only through `<lang>_bool_terminal_kinds!()`. Every walker consults that set inside an `if` / `while` / ternary / `&&`-operand slot and nowhere else, while the comparison token beside it carried no such gate: `var b = x == 1;` scored 1 and `var b = x is int;` scored 0. Fitzpatrick Rule 5 scores a relational operator by use. The seven constructs move to their language's unconditional condition arm and leave the operand sets, which hold values rather than operators; listing them in both would score them twice. Groovy's spaceship `<=>` joins as a condition token, the spelling Ruby, PHP, C++ and Mozcpp already use for it. Two operands join the sets on the converse rule, counting in a boolean slot only: Rust's `macro_invocation` (`if matches!(x, Some(_))`, and `cfg!` alike -- the breadth is the decision, since these sets have never discriminated on return type) and Python's `named_expression`. The walrus is the one construct in the survey scoring on two ABC axes, which is correct: it binds a name and decides a branch, and the axes are independent measurements rather than a partition. C#'s `x is > 5` now reads level with the `x > 5` it is sugar for, closing the asymmetry #1383 recorded as a deliberate exception it had no way to remove. The pattern's own `>` still scores nothing, so the test is worth one condition rather than two. Measured before and after on every construct, inside and outside a boolean slot: no score inside a slot moves, so nothing is counted twice, and a `for (x in xs)` header stays at zero in all three languages whose membership keyword it shares. One of the 1,610 integration snapshots moves -- serde's `serde_derive/src/dummy.rs`, on `if cfg!(no_underscore_consts)`. The five structurally changed languages have no corpus exposure. Fixes #1461
A whole-branch review of the batch found that Elixir did not get the condition slot #1454 gave Java, Rust, Python, Ruby and C#. Its `when` arm added a flat one for the token on top of whatever the guard's sub-structure already scored, so `when n > 5` cost two where `when is_integer(n)` and `when n` cost one -- the spelling-dependence #1454 exists to remove, reproduced in one of the languages it fixed, and a grammar-dispatch section 5 double count with the `>` token arm. The guard expression now routes through `elixir_count_condition`, the sibling of `java_count_condition` / `ruby_count_condition`: a value-bearing guard scores in the slot, an operator-spelled one scores through the operator's own arm. All nine spellings the grammar allows land on one, and Elixir reads level with Java on every shape of the cross-language fixture. The typespec gate is untouched. Two arms the slot presumes existed did not, and both would have scored zero rather than one once the slot declined an operator: * `in` / `not in` are relational operators with no arm. #1461 moved exactly this class onto an unconditional arm in five languages and did not reach Elixir, so `a in b` scored zero against `a == b`'s one. They join the `<` / `>` parent gate, which already excludes an operator merely named (`&in/2`, `Kernel.in(a, b)`). * the keyword `not` was not recognised as a negation, only `!`, so `a && not b` scored one against `a && !b`'s two. `elixir_when_is_guard`'s doc called alternative guards left-associative; they are right-associative, the outermost holding the anchor. The conclusion it drew was right and is unchanged -- the construct scores one -- and `elixir_count_guard` peels the same nesting to reach the alternative that occupies the slot. Metric drift: Elixir abc.conditions falls by one per operator-spelled guard, and rises by one per `in` / `not in` and per `not`-negated boolean operand. Cyclomatic is unaffected. No corpus carries an Elixir file, and no integration snapshot moves; one anchored unit snapshot does.
A whole-branch review of the batch found #1462's sweep incomplete while its own comment called it complete. It added strings, `null` and collection literals to eight sets and disclaimed only C#/Java/Kotlin/ Rust/Go (a bare literal is a compile error there) and the C family (deferred for corpus exposure). Perl, Ruby and Elixir are truthy-valued, appeared in neither list, and already carried `False` / `Nil` -- the precedent that same comment cites -- yet none got a string, collection or symbol literal. All three reproduced the gap the CHANGELOG describes: `a && "s"` scored 1 against `a && b`'s 2, and `if ("s")` 0 against `if (b)`'s 1. Each kind was measured a condition short of an identifier control in both walker paths before being listed, with its id read off `bca dump`: * Perl: the four string productions the grammar keeps separate, `heredoc_initializer`, `qx()` and backticks, `qw()` / `[...]` / `{...}`, `qr//`, and `special_literal`. `s///` and `tr///` stay out -- they edit `$_` and yield a count, so they are operations, not literals. `qr//` had been deferred beside them as "always true", which is the argument for counting it once the question became whether a literal fills the slot. * Ruby: `string`, `chained_string`, `heredoc_beginning`, `subshell`, `array` / `hash` / `%w[]` / `%i[]`, `regex`, `?a`, and both symbol productions. * Elixir: `string`, `charlist`, `sigil`, `quoted_atom` -- a separate production from `atom`, so `:"q a"` scored 0 where `:atom` scored 1 -- and `list` / `tuple` / `map` / `bitstring`. Tcl and iRules are the remaining truthy-valued languages and needed nothing: `quoted_word`, `braced_word_simple` and `number` already cover every literal an `expr {...}` operand can hold, measured rather than assumed. `literal_bool_operands` gains a row per language and the three features join its `cfg(any(...))` union, so a Perl-, Ruby- or Elixir-only build no longer compiles the module away and silently asserts nothing about them. A Perl or Ruby heredoc cannot live in that table -- its body follows the statement, so substituting one into a single-line template yields an unterminated literal -- and has its own test. The completeness claim in `kind_sets.rs` and the CHANGELOG now say eleven languages and name what each set gained. The Groovy back-reference that #1462 left pointing at a list it had emptied is corrected too. Metric drift: abc.conditions rises by one per non-numeric literal in a boolean operand slot in the three languages. Cyclomatic is unaffected. No corpus carries a .pl, .rb or .ex file and no integration snapshot moves. `kind_sets.rs` crosses the loc.sloc backstop on documentation volume alone (ploc 468 against a limit of 550) and takes a baseline entry.
Extend lessons 23, 50 and 84 with sub-examples from the 2026-09-14 batch rather than adding new entries. Each is an instance of a mechanism those entries already name, and a new number would be a breaking change for the roughly sixty files that cite lessons by number. 23 gains the case where the compensation is not a constant but a helper that pins each value absolutely while asserting no relation between them, so a wrong value that is individually plausible reads exactly like a right one. 50 gains the audit owed prospectively when a change removes one of two paths summing into a field. 84 gains an in-source FIXME whose conclusion held but whose stated reason did not survive measurement.
Three additions to the testing rules, all from the 2026-09-14 batch. Classify a build failure by `error[` or `could not compile`: cargo prints `error: test failed` on an ordinary red run, so a driver matching bare `error` reads every discriminating perturbation as a build break. A perturbation that passes is a finding, not a null result. Check whether the behaviour is enforced by the mechanism or merely absent from a list before strengthening anything, and correct the test's comment rather than inventing a fixture to justify it. A feature-gate check needs `-p big-code-analysis`. A workspace-wide run does not isolate features at all, because the CLI and web members pull the library with default features and cargo unifies them. Also wraps a line beginning `#1431`, which rumdl parsed as a heading.
The module's two tests are gated on `php` and `groovy` respectively, but the module itself was gated only on `test`, so a build enabling neither feature left `ast_has_kind_id` and the `crate::*` glob unused. CI sets `RUSTFLAGS: -D warnings` workflow-wide, so that is a hard compile failure on six feature-matrix legs: no-default-features, minimal-langs, lang-c, lang-go, lang-python and lang-tcl. `make pre-commit` cannot see this — it builds the default and all-features flavours only, and both enable php and groovy.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1476 +/- ##
==========================================
+ Coverage 98.00% 98.03% +0.02%
==========================================
Files 359 359
Lines 94399 95916 +1517
Branches 93968 95485 +1517
==========================================
+ Hits 92520 94031 +1511
+ Misses 1212 1205 -7
- Partials 667 680 +13
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
dekobon
commented
Sep 15, 2026
dekobon
commented
Sep 15, 2026
dekobon
commented
Sep 15, 2026
dekobon
commented
Sep 15, 2026
dekobon
left a comment
dekobon
left a comment
Owner
Author
There was a problem hiding this comment.
Code Review: fix(metrics): eleven ABC, cyclomatic and self-scan fixes
Verdict: REQUEST CHANGES
| Severity | Count |
|---|---|
| HIGH | 1 |
| MEDIUM | 3 |
Findings
| # | Severity | File:Line | Title | Category |
|---|---|---|---|---|
| 1 | HIGH | src/metrics/npa/shared.rs:682 |
Repeated Elixir guards lose decision alternatives | correctness |
| 2 | MEDIUM | src/metrics/abc.rs:14954 |
New fixture locks in the Groovy safe-index undercount | test-gap |
| 3 | MEDIUM | src/metrics/abc.rs:15868 |
Empty literal rows still pass the new tests | test-gap |
| 4 | MEDIUM | CHANGELOG.md:174 |
ABC drift is described as linear for non-linear fields | documentation |
Review passes executed
- Correctness and grammar-dispatch logic
- Security and attacker-reachability assessment (+ Semgrep)
- Architecture, API contract, and traversal performance
- Test discrimination and feature-gate coverage
- Baseline freshness tooling
- Documentation, snapshots, and generated-artifact consistency
- Independent Sol review
Files reviewed: 37
Semgrep ran 97 Rust/multilang rules over 23 changed production files with zero findings. Focused/all-workspace tests passed in review passes, and all current GitHub checks are green. The findings are semantic and test-contract gaps not covered by those gates.
`def f(n) when is_integer(n) when is_float(n)` read identically to the single-guard `when is_integer(n)` on both axes, while the semantically equivalent `when is_integer(n) or is_float(n)` read one higher on each. Elixir defines repeated guards as an or-chain: each `when` expression is tried in turn and evaluation moves to the next when the previous is false or raises, so the two spellings carry the same decisions. The guards nest right-associatively (`head when (a when (b when c))`), and only the outermost sat on the anchor `elixir_when_is_guard` tests, so every nested `when` answered "not a guard" and contributed no decision. ABC then peeled the nesting and scored the last alternative alone. `elixir_when_is_guard` now climbs the enclosing `when` operators before asking the position question, which makes every token in the chain a guard; the climb stops at the first non-`when` ancestor, so a single guard pays no extra step. The shared `elixir_when_alternative` names the one alternative each token introduces — the nested operator's `left`, or its own `right` at the end of the chain — so the ABC slot stays one-per-token instead of counting the whole chain once per token (grammar-dispatch section 5). Both metrics therefore move together and the section 8 parity holds at every chain length. The typespec exclusion is unaffected: a `@spec f(a) :: a when a: integer` binding clause nests no `when`, so the climb is a no-op and the same anchor rejects it, with multiple bindings arriving as one `keywords` node. Refs PR #1476.
`?[` short-circuits on a null receiver exactly as `?.` and `??.` do, but Groovy cyclomatic listed only the two navigation tokens, so `l?[0]` read level with the unconditional `l[0]` while `l?.get(0)` read one higher. The arm matches the `?[` token rather than the `safe_subscript_expression` wrapper, the same granularity and the same reason as its two siblings: a chain nests one wrapper inside another, so the token counts each operator once where the wrapper would score `l?[0]?[1]` as one. `QMARKLBRACK` has a single kind id at the pin, and the wrapper node reaches no cyclomatic arm, so there is no double count. `own_production_bool_constructs` listed `l?[0]` among the constructs whose contract is "scores the control's cyclomatic". That row passed only because of this gap, so it would have rejected the fix; it moves to `groovy_safe_navigation_closes_the_two_below_gap` beside `a?.b` and `a??.b`, which assert the offset explicitly, and the row's recorded construct count follows it down. Fixes #1471. Refs PR #1476.
`literal_bool_operands::for_each_case` documented three guards against the table decaying into asserting nothing, but only two were written. A row with `literals: &[]` and `expected_kinds: 0` satisfies the length check, runs its inner loops zero times, and still increments `checked` — which counts languages, not spellings — so that language asserted nothing while the non-vacuity guard read satisfied. Measured: emptying the Elixir row left all four tests in the module green. `numeric_bool_operands` carries the same gap, and its own doc comment names emptying as one of the shapes it catches, so both drivers get the assertion rather than only the one that was reported. Refs PR #1476.
Seven Unreleased entries said `abc.conditions` and `abc.magnitude` rise or fall "by one". Only the conditions count does: magnitude and value are sqrt(A^2 + B^2 + C^2) over the whole vector, so their deltas depend on what the vector already held — the Elixir snapshot this branch moved went conditions 2 -> 1 with magnitude 2.236 -> 1.414. Two of the seven also called `abc.conditions` and `abc.magnitude` gated threshold metrics. Neither is a threshold name: `bca check` accepts `abc`, which extracts the per-space magnitude. Refs PR #1476.
Owner
Author
|
All four findings fixed and confirmed. Three had inline threads, now replied to and resolved; finding 3 appeared only in the summary table with no thread to reply on, so it is covered here.
Every finding reproduced before being fixed. No pushback on any of them. Finding 3 — and it was worse than reported
Two things made it sharper than the title suggests:
Perturbation, on the Elixir row set to Finding 1 — a note on the shape of the fixThe obvious repair — classify every alternative from each token, once the climb makes nested tokens guards — would have been a §5 double count giving N² conditions for an N-alternative chain, because ABC's Verification
Also closed
|
`elixir_when_is_guard` accepts a definition `Call`'s `arguments` anchor when the keyword is a method macro *or* one of `defguard` / `defguardp`. `elixir_is_method_macro` spells only `def` / `defp` / `defmacro` / `defmacrop`, so the second disjunct is its own path, and no fixture in the suite spelled `defguard` — deleting the `matches!` failed zero of the 3,460 lib tests. `defguard` is where a guard is most plainly a decision, so an ungated head would have scored it zero on both axes while the `def f(x) when g` spelling it expands into scores one. Asserted on the module rather than a member, since `defguard` opens no function space; the guard bodies are calls rather than comparisons so neither can supply a condition of its own, and `plain` pins the module's rows to the two guards and nothing else. Raises patch coverage on PR #1476.
Every `None` arm named here is an `Option` on a lookup the pinned grammar cannot fail: a required field, or a wrapper's operator-plus- operand child pair. Each is spelled `?` or `let ... else` rather than `expect` because AGENTS.md bans the latter outside tests, which leaves a branch no fixture can take and nothing on the page saying so. Say so, with the evidence and the reason not to chase it: - csharp/groovy wrapper operands: `unary_expression` declares `operand` and `operator` required, and a parenthesized expression is `(` expr `)`. Only error recovery reaches the arms (`bool b = !;` parses to a one-child `prefix_unary_expression`), and that is invalid source, so pinning its numbers would make the grammar's present over-permissiveness the contract. - elixir: `binary_operator` declares `right` required; and the `When` arm's parent lookup cannot miss, because the gate it sits behind already walked that ancestor chain and returns false when it is empty. Not re-plumbed out of the predicate, whose job is to be one boolean the Abc and Cyclomatic impls share. - npa/shared: a `when` token's chain always holds its own operator, and that operator always sits under something, because the Elixir root is `source`. - perl: `condition` is required on all five modifier productions, and an `arguments` node cannot be childless. The `else` arm takes a `parenthesized_argument`, which the parser emits in this slot only for the empty spelling `EXPR if ();` — valid Perl, but an empty wrapper peels to nothing, so that arm and a bare `None` score alike. Measured by perturbation: no test moves either way. A test there would pin a value neither branch decides. Comment-only; no behaviour change. Documents the uncoverable tail of PR #1476's patch coverage.
`patch.default.target` was `auto`, which targets the project figure and is unreachable by construction for a grammar-dispatch patch. `AGENTS.md` bans `unwrap` / `expect` in non-test code, so every infallible node lookup is spelled `?` and each mints a `None` region no valid input can reach; partials were 12 of the 19 misses on PR #1476. The comment also records a second trap: Codecov drops a file whose diff GitHub's API refuses to send, so a heavily-edited file is absent from the patch rather than counted as missing. That removed the largest changed file from the denominator on #1476, which is why the published patch figure covered 303 lines and not the ~1,840 the branch touches.
The `feature-matrix` step comment explains why it runs clippy rather than `cargo check` but says nothing about the suite, so readers have assumed the legs exercise behaviour. They verify compilation only. Running the suite there is not currently possible: per-language tests are not feature-gated (30 of 3,482 `#[test]` fns in `src/`), and `Node` panics by contract on a grammar that is not compiled in, so `--no-default-features --features rust` fails 2,642 of 3,238 tests. Refs #1472
`.claude/rules/testing.md` requires a test whose case list is built from `#[cfg(feature = …)]` rows to carry `#[cfg(any(feature = …))]` naming the union, so a build enabling none of them drops the subject instead of tripping its own `checked > 0` guard. `cargo clippy --all-targets` over a partial feature set catches the compile half of that. It cannot see the runtime half: a subject whose union gate is present but defeated — by a non-feature disjunct, or by a gate that does not reach the item — is a valid build that warns about nothing and fails only when that leg runs the test, which is how #1220 and PR #1221 shipped. `utils/check-feature-gates.py` scans `src/` for `mod` and `#[test] fn` items carrying such a gate (39 today, 34 of them test-bearing), resolves the leg's feature closure from the root manifest, and asks `cargo nextest list` whether any subject disjoint from that closure is still in the build. It runs as a step in each `feature-matrix` leg, where the flags it needs are already to hand. It is deliberately out of `make pre-commit`: it needs a test-binary build per feature set, and the question is only interesting for a set that enables none of some subject's rows. `make check-feature-gates FLAGS=…` is the manual spelling; the self-tests, which stub the cargo call, do run in the local gate. Refs #1472
The script forwards every unrecognised argument to `cargo nextest list`, which builds the test binaries. Without `--locked` it was the only cargo invocation in the job permitted to rewrite `Cargo.lock` rather than fail on a stale one; the clippy step directly above it already carried the flag.
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.
Eleven issues, one branch. Ten are ABC/cyclomatic metric defects; one is a
self-scan tooling gap. Most share a root shape: a condition slot delegates
to a per-language helper that scores a known terminal kind, recurses into a
known wrapper, and silently scores nothing for everything else.
What landed
Supergated on itsWildcardparent7f2a1febbca checkwarns when a baseline entry has measured past its recordaa649899@availablescores a conditionbf6252bca??.b)41e107fcb!, via a peel the slot queries17ee482ecasegated on aswitch_sectionparent, both metricsb6c8747ee82fc38e04cefe5c61a28712747b0b3cPlus two remediation commits from a whole-branch review, two documentation
commits, and one CI fix — see below.
Every issue that made a measurable claim under-reported it
Each fix was measured before it was written rather than taken from the issue
body. That changed the scope of most of them:
@availablescores no condition #1457 documented one failing shape; five more had the same zero(
&&operands,forconditions, ternaries,!@available, and the__builtin_availablesynonym).b!scores no condition #1463 documented one; five more (b!!,arr[0]!,C()!, aforcondition,
b /*c*/ !).a??.b, found by walking all 39_expressionalternatives rather than the five the issue listed.
mainmeasured 2, because the language had no cyclomatic guard arm at all. Its
claim that
metrics.mdand the fix(csharp): awhenguard on a switch arm is a branch neither metric counts #1422 changelog entry "do not scope theguard rule to C#" is also false — both scope it explicitly.
goto casecounts as a decision #1450's FIXMEs justified "both arms must move together" by citing aparity invariant. Gating either side alone breaks zero of 3,429 lib
tests: that parity is opt-in fixture policy, and no fixture outside the
cognitive tests spelled
goto case. The conclusion held; the reasonnever did.
#1450and#1451are the same defect filed twice, so one commit closesboth.
The whole-branch review found two defects everything else missed
Nine per-issue pipelines, a green gate, 5,766 tests and 99.5% patch
coverage all passed over these. Both are visible only across commits:
61216676).fix(metrics): match guards are under-counted in Java, Rust and Python #1454 gave five languages a condition slot; Elixir got a flat
+1on topof existing sub-structure, so
when y > 5scored 3 where every siblingscored 2 — the spelling-dependence the change existed to remove. Three
artefacts asserted the opposite, and the test recorded
("tok", 3, 3)where parity requires 2. Fixing it surfaced two further latent gaps the
+1had masked: Elixir had noin/not inarm in either metric, andonly
!counted as negation, not keywordnot.a9100f3b).Perl, Ruby and Elixir already carried
False/Nil/Nullbut got nostring or collection literal, so
if ("s")scored 0 againstif (b)'s 1.A CI-only failure the local gate cannot see
0e15c49cgatesmod hidden_literal_supertypeson the union of the twofeatures its tests name. Both tests are gated on
php/groovy; themodule was gated only on
test, leaving two imports unused. WithRUSTFLAGS: -D warningsset workflow-wide this is a hard compile failure onsix feature-matrix legs (
no-default-features,minimal-langs,lang-c,lang-go,lang-python,lang-tcl), all verified locally before and after.make pre-commitstructurally cannot catch this: it builds the defaultand
--all-featuresflavours, and both enablephpandgroovy.Metric drift
Threshold-gated metrics move in several languages. Each commit carries its
own drift paragraph in
CHANGELOG.md. Corpus movement: 86 of 1,610snapshots — 85 pdf.js (#1462) and 1 serde (#1461, an
if cfg!(...), not thematches!the issue predicted). The submodule advanced81abf593 → 4b0a998e → 42650711 → 062ae343, linearly, with each SHArecorded in the same parent commit as its fix.
Four of the five languages #1461 changes have no corpus file at all, so most
of that small delta is "not exercised" rather than "not affected" — the
per-language before/after tables on the issue are the evidence there.
Verification
make pre-commit→BCA_GATE: passadded lines are all documented unreachable-at-this-pin defensive arms
-D warningsFollow-ups filed
#1467 (Perl
PatternMatcher, split out by #1461), #1468 (Elixirelixir_inspect_containeris dead code — 0 hits across the suite), #1469(the C family's literal gap — the one integer-truthy group with 1,042
snapshots of exposure), #1470 (nine languages still restate the wrapper set
instead of asking their peel; that shape produced a real bug three times out
of three), #1471 (Groovy
?[uncounted in cyclomatic), #1472 (the ungatedabc.rstest module), #1473 (a scheduled baseline-freshness job — thewarning in #1465 cannot close the class on its own), #1474 (PHP
is_stringmatches the
stringtype keyword), #1475 (nine constructs measuring short,each a judgement call).
Fixes #1419
Fixes #1450
Fixes #1451
Fixes #1454
Fixes #1457
Fixes #1461
Fixes #1462
Fixes #1463
Fixes #1464
Fixes #1465
Fixes #1466