Repository navigation
fix(abc): ABC condition-counting batch - #1524
Merged
Merged
Conversation
C++ and Mozcpp counted `<=` `>=` `==` `!=` `<=>` in an ungated arm, so declaring `operator<=`, `operator>=`, `operator==`, `operator!=` or `operator<=>` scored one spurious ABC condition per overload, while `<` / `>` were already gated on a `binary_expression` parent. This is the C++ instance of the C# defect fixed in #1420. All seven tokens now share the `binary_expression` allowlist. A grammar.json sweep of tree-sitter-cpp 0.23.4 and the vendored mozcpp grammar is recorded at the arm: `#if A <= B` is aliased onto `binary_expression` and keeps counting; `operator_name` and fold expressions do not count, consistent with `<` / `>`. C and ObjC emit these tokens only from comparisons and have no overloading, so their arms stay as they are, with a note saying why. Corpus snapshots move only in openfst operator==/operator!= definitions and declarations, all downward. Fixes #1448
The #1448 gate dropped comparison folds to zero. A fold applies its comparison, so `(a <= ...)` and `(a < ...)` now score one condition, and the binary fold `(0 == ... == a)` also scores one: the token must be the `fold_expression`'s `operator` field, which names only one of its two spellings. The applied-comparison test moves into `cpp_comparison_is_applied`, shared with Mozcpp and keyed on kind names like the other shared C++ helpers, which also keeps `compute` under its halstead.effort soft limit. The overload fixture gains a defaulted C++20 `operator==`.
A subject-less `case` lists ordinary boolean expressions, evaluated as `if` predicates, so a comparison in a `when` clause is already an ABC condition through the token arm. The `When` arm added a blanket one on top: `case; when x > 5 then 1; else 0; end` scored 3 against its `if` analogue's 2. Parenthesised and negated comparisons, `&&` / `||` chains, `(x in T)` pattern tests and ternaries double-counted the same way. A subject-less clause now pays for its decision only when no other arm already did, decided by `ruby_condition_scores_itself`, which peels `(...)` and `!` / `not` and asks whether a comparison, chain, pattern test or ternary remains. A subject-ful clause keeps its per-clause count, since its `pattern === subject` comparison is written nowhere. The #1421 template routed the condition through the `if` slot instead. Measured here, that slot scores 0 for `Foo::Bar`, `self`, `-x`, `defined?(x)`, `(y = x)`, `*xs` and `x + 1`, so delegating would have dropped each of those clauses from 1 to 0. That gap in the `if` slot is filed as #1520. A clause listing several patterns pays unless any pattern scored itself, so `when a, x > 1` and `when x > 1, a` agree. `when x > 1, x < -1` reads 2 against cyclomatic's single decision per clause, as Kotlin's multi-alternative entries do. The comparison token list moves into `ruby_comparison_kinds!` so the token arm and the new predicate cannot disagree. Fixes #1453
A subject-less `when a, b` is an implicit `||`, but the clause paid one condition unless any pattern had already scored itself, so `when a, b` and `when a, x > 1` read 1 against their `if a || b` / `if a || x > 1` analogues' 2, while `when x > 1, x < -1` matched its analogue at 2. Each pattern now scores once unless its own arm already counted it, which matches all three analogues and stays independent of pattern order. Subject-ful clauses are unchanged.
A bare `/re/` or `m{re}` matches the implicit `$_`, a relational
operator with no operator token. It reached ABC only through
`perl_bool_terminal_kinds!()`, which walkers consult inside a boolean
slot, so `my $r = /^#/` scored 0 against `my $r = ($x =~ /^#/)`'s 1.
Move both kinds out of the terminal set onto a gated arm that counts
the pattern wherever it is written, except where it is bound by
`=~` / `!~` (that token already counts) or is `split`'s first
argument, a delimiter rather than a match. In-slot shapes such as
`if (/x/)` and `/x/ && $y` keep their scores. `s///` and `tr///`
stay out of scope (#1475).
Fixes #1467
Six languages spelled their wrapper set twice: once in the `<lang>_inspect_container` peel loop and again in `<lang>_count_condition`, which decides what to hand that peel. Kotlin, Groovy and C# each had the two lists drift apart and score a valid predicate zero (#1459, #1466, #1463). Ruby, Go, Rust, Python, Lua and Java now carry a `<lang>_wrapper_operand` that returns one peel step, and both the loop and the condition slot ask it. Python's returns a bare node: it peels parentheses only, so it has no proves-boolean flag to report. Step semantics are unchanged, including the positional `child(1)` reads. PHP, Perl and C++ restated nothing (their slot already falls through to the peel) and are left alone. A 337-cell wrapper matrix (`b`, `(b)`, `!b`, `!!b`, `(!b)`, `!(b)`, `((b))`, `-b`, `(-b)`, plus `not` forms, across each language's if, loop, chain, ternary and guard slots) measured identical before and after. The new tests pin routed, declined and proves-boolean answers per language; a perturbation sweep of each half fails them. The Java `java_inspect_container` cognitive baseline entry is dropped, as the function no longer breaches. Fixes #1470
C++ spells `!`, `&&`, `||` and `!=` equally as `not`, `and`, `or` and `not_eq` ([lex.digraph]), and tree-sitter-cpp and the vendored tree-sitter-mozcpp give each alternative its own token kind. ABC matched only the symbols, so `if (not b)` scored 0 against 1 for `if (!b)`, `b and c` scored 0 against 2 for `b && c`, and `b not_eq c` scored 0 against 1 for `b != c`. The shared peel now accepts `not` beside `!`, and the Cpp and Mozcpp dispatchers add `And` / `Or` to the Rule 9 chain arm and `NotEq` to the comparison arm. Each alternative occurs in a subset of its symbol's grammar productions, so the existing parent gates decide it unchanged. C and Objective-C have no such tokens (`not` is an <iso646.h> macro the parser reads as an identifier), so their scores do not move. Cyclomatic, cognitive and Halstead have the same gap; that is left to a follow-up.
`ruby_condition_scores_itself` restated the `(...)` / `!` / `not` peel that `ruby_wrapper_operand` now owns. It asks the peel instead, so the two cannot drift apart. Behaviour is unchanged.
tree-sitter counts a comment among a node's children and marks it
named, so every ABC slot read by fixed child index or as "the first
named child" could land on a comment and score the condition zero:
`if (/*c*/ b)`, `if /*c*/ (b)`, `do {} while /*c*/ (b);`,
`return /*c*/ !b;`, `(( /*c*/ b))`, `! /*c*/ b`. Two read sites were
affected in nearly every language.
Slot reads now go through the grammar's field (`condition`, `value`,
`right`, `body`, Elixir's `operator`), or, where the grammar names
none, the new `wrapped_operand` helper: the first named child that is
not an `extra`. That covers C, C++, Mozcpp, ObjC, C#, Java, Groovy,
the JS family, PHP, Python, Rust, Go, Lua and Perl. The field reads
also fix two preambles: a C++ init-statement (`if (int y = g(); b)`)
and a Rust loop label (`'a: while b`) both scored zero.
Peel steps through `(...)` and negations take the operand the same
way in every language, which also repairs Ruby's subject-less `when`
(`when ( # c⏎ x > 1)` counted twice), Perl's `split( # sep⏎ /,/, $s)`
delimiter test, Perl's last-operand read of a trailing comment, and
Elixir's `=` read behind a comment (an assignment).
Two tests that pinned the old zero for C# and Groovy now assert the
fixed value. Corpus snapshots move only `abc.conditions`, upward: two
pdf.js `if (/* … */ x)` functions, and serde's `while try!(…)`, whose
2015-edition macro parses to an ERROR at child(1) so the field read
now finds the real condition. The self-scan baseline is refreshed:
Go's and ObjC's `compute` effort fell.
Fixes #1455
Ruby's paren peel read the first statement of `(x; y)`, but the sequence evaluates to its last; it now reads the last operand, as Perl's list peel does, so `if (x; x > 1)` scores 1 and `if (x > 1; x)` 2. Docs: the book gains a Ruby subject-less `when` row and notes the C++ overload, fold and alternative-token rules; a test-module doc no longer lists Perl's bare matches in the terminal set; the CHANGELOG names the kenlm snapshots and the Elixir assignment.
C#'s declarator arm, Ruby's statement-sequence peel and Perl's list peel each spelled "last child that is named and not an extra". They now share `last_operand` beside `wrapped_operand`. Also separates the Lua and Python peel comments from the module overview and fixes a member count in a Perl test comment.
`ruby_condition_scores_itself` treated every `&&` / `||` chain as already counted, but the Rule 9 walker counts plain operands only, so `case; when -a && -b` and `when (a = 1) && (b = 2)` scored 0 against 1 on main and against `when -a`'s 1. A chain now counts as paid only if some operand, at any depth, is a comparison, pattern test, ternary or a plain operand the walker counts. The walk is a worklist so a long left-nested chain cannot recurse deeply.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1524 +/- ##
==========================================
+ Coverage 98.01% 98.08% +0.06%
==========================================
Files 359 359
Lines 96210 97148 +938
Branches 95779 96717 +938
==========================================
+ Hits 94303 95283 +980
+ Misses 1222 1210 -12
+ Partials 685 655 -30
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
The wrapper peels spelled a grammar-required child as `child?`, and
the C++, JS and PHP peel loops had a second `let ... else { break }`
after the non-wrapper exit, so each carried a `None` branch no valid
tree reaches, reported as uncovered patch lines. The peels now map
the optional child (`wrapped_operand(node).map(...)`, guards via
`is_some_and`), the three loops end through one exit, and two
table-driven tests filter disabled languages instead of
`continue`-ing past them. Behaviour is unchanged.
This branch was successfully deployed
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.
Five ABC
conditionsfixes insrc/metrics/abc/, batched because they share the per-languagecount_condition/ peel helpers. Onlyabc.*values move; cyclomatic, cognitive and Halstead are unchanged. TheCHANGELOG.md[Unreleased]section has the user-facing wording.Issues
</>score a condition #1448, C++ / Mozcpp: comparison-operator overloads (operator<=,==,!=,>=,<=>) no longer score a condition, because all seven comparison tokens now count only where applied. A comparison fold counts once, through itsoperatorfield.#if A <= Bstill counts.casedouble-counts its comparison #1453, Ruby: a subject-lesscaseno longer counts awhencomparison twice. Each pattern is scored as one operand of an implicit||, sowhen a, b=if a || b. A subject-fulcase x; when 1is unchanged./re//m{re}match against$_scores by use. A pattern bound by=~/!~andsplit's delimiter are excluded.<lang>_wrapper_operand) which kinds it unwraps, instead of restating the list. No metric change.not/and/or/not_eqlike!/&&/||/!=.ifwith an init-statement, a labelled Rustwhile, and an Elixir=behind a comment.#1468 was already fixed by #1476. I re-measured it and closed it separately.
Fixes #1448
Fixes #1453
Fixes #1455
Fixes #1467
Fixes #1470
Corpus snapshot drift
tests/repositories/big-code-analysis-outputmoves062ae343 → 2e83ae93:</>score a condition #1448: 61 openfst / kenlm entries. Every comparison-overload definition or declaration drops by one condition.if (/* … */ …)×2 gain one each. serdewhile try!(…).is_some()gains one, because the 2015-editiontry!parses to anERRORat child 1, which the field read now skips.Review
Each fix went through simplify-rust, rust-optimize, review and audit-tests. Every fixed arm was perturbation-verified, meaning each perturbation fails the intended tests. A fresh-context whole-branch review and
/code-review highfollowed; their findings are fixed in278880fa,f4b63250and09283fc1.Follow-ups filed
ifslot scores 0 forFoo::Bar,-x, …switchdouble count, the fix(abc/ruby): subject-lesscasedouble-counts its comparison #1453 sibling.Test plan
make pre-commit:BCA_GATE: pass (gate=pre-commit).--all-features: 5,801 tests pass.mainis 99% (1,476 / 1,482), against a 97.31% project figure. Every uncovered line is either abreakon a grammar-required child or a disabled-languagecontinue..bca-baseline.tomlrefreshed where a value moved.