From a77ac806723260ce06d7c5151fb7f220e141d96c Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 13 Sep 2026 18:43:38 -0700 Subject: [PATCH 01/11] fix(abc/csharp): gate all six comparison operators MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A C# comparison-operator overload declares a name; it applies nothing. #1297 established that for `<` and `>`, but C# overloads six operators and spells the other four with distinct tokens — `<=` `>=` `==` `!=` — which reached a different, ungated arm of `csharp_count_token_condition`. Each still scored one spurious ABC condition per declaration: measured 1 apiece against `<` / `>`'s 0, on a class where `cyclomatic()` is 1 for all six members. The four tokens are merged onto the `binary_expression` parent allowlist `<` / `>` already carried, which subsumes #1383's `relational_pattern` denial for `<=` / `>=` and fails closed on a grammar bump (§1). `Else`, `Case`, `Try` and `Catch` are split out of the old shared arm and stay ungated: each comes from one production, so there is nothing to gate on. Two claims the previous comments made are corrected rather than inherited, both by measurement. `BinaryExpression2` was described as listed defensively and unreachable, on the grounds that C#'s preprocessor admits no bare `<` / `>`. That is true of `<` / `>` and irrelevant to `==` / `!=`, which the preprocessor does admit. The reason the alias entry is not load-bearing is different: `#if A == B` parses as `BinaryExpression` (369), so the preprocessor spelling counts through the first entry. `csharp_preproc_equality_counts_through_the_binary_expression_alias` pins both halves, since the gate is now the only thing keeping `#if` equality a condition. `Case` comes from two productions, not one — `switch_section` and `goto_statement` — so `goto case 2;` scores a condition without being an arm. It is left counting deliberately: C# cyclomatic counts the same token, so gating it here alone would break the §8 parity `conditions == cyclomatic() - 1` that a two-arm-plus-`goto case` fixture currently satisfies. Filed separately rather than changed here. The new test gives each token its own member. A partial fix then names the spelling it missed, where the file total cannot: that total reads 4 before and 0 after, so any two-of-four fix halves it and still looks like movement. Method `n` is the §11 control — the same four tokens applied, inside `binary_expression` parents, which must keep scoring one each. No corpus snapshot moves: neither C# corpus declares a comparison-operator overload. Fixes #1420 --- big-code-analysis-book/src/metrics.md | 2 +- src/metrics/abc.rs | 149 ++++++++++++++++++++-- src/metrics/abc/csharp.rs | 171 ++++++++++++++------------ 3 files changed, 233 insertions(+), 89 deletions(-) diff --git a/big-code-analysis-book/src/metrics.md b/big-code-analysis-book/src/metrics.md index f1c88a92..d9d1c218 100644 --- a/big-code-analysis-book/src/metrics.md +++ b/big-code-analysis-book/src/metrics.md @@ -151,7 +151,7 @@ application would over-count. | Ruby | Bare-predicate `if` / `unless` / `while` / `until` (block and modifier forms) count one condition | Idiomatic Ruby favours bare predicates (`if flag`, `x if flag`); counting the condition slot keeps ABC conditions at or above Ruby's cyclomatic decision count (the alignment enforced across the other languages). A comparison (`if a == b`) or `&&` / `\|\|` chain in the predicate is counted by its own operator / walker arm and is not double-counted. | | Bash | `if` / `elif` / `while` and each non-wildcard `case` arm count one condition | A Bash predicate is a *command*, so the branch keyword itself — not an embedded boolean expression — is the condition signal. Each matches a Bash cyclomatic decision; the bare `*)` case arm (the analogue of `default:`) is excluded, mirroring the cyclomatic standard count. The arithmetic ternary `$(( a ? b : c ))` therefore contributes nothing: it carries no branch keyword, so it falls outside the rule set rather than through a gap in it. | | Kotlin | `try` counts a condition alongside `catch` | Fitzpatrick counts both keywords, and Java / C# / C++ / Groovy already count both; Kotlin previously counted only the catch block. | -| C++, Objective-C, Mozcpp, Rust, Go, Java, Groovy, C#, Kotlin, TypeScript, TSX, JavaScript, Mozjs, Lua, Perl, Ruby, Bash, Elixir | A `<` or `>` that is not a comparison is not a condition | Every one of these grammars spells at least one non-comparison construct with the same bare `<` / `>` token a comparison uses, so the comparison rule is gated on the token's parent. What that excludes, per family: template and generic brackets in C++, Objective-C, Mozcpp, Rust, Go, Java, Groovy, C#, Kotlin, TypeScript and TSX (#1274); JSX tag delimiters in TypeScript, TSX, JavaScript and Mozjs; Lua 5.4 variable attributes (`local x = 1`); a C# comparison-operator overload's declared name (`operator <`); Kotlin's qualified super call (`super.g()`) (#1297); Perl's filehandle and lexical-handle readlines (``, `<$fh>` — but not ``, which the grammar lexes as one token); Ruby's superclass clause (`class Foo < Bar`) and comparison-operator method names (`def <(other)`), and Bash I/O redirection (`cmd > out`) (#1280); Elixir's sigil delimiters (`~s`) (#1256). C# additionally excludes the operator of a relational pattern (`x is > 0`, and the `> 5 =>` arm of a switch expression): the arm or `if` condition slot that owns the pattern already scores the decision, so counting the operator too charged a relational arm twice what the constant arm `5 => 1` scores (#1383). Its `>=` / `<=` spelling is excluded by the same rule through a separate token. The exclusion holds wherever the pattern sits, so a pattern no arm or condition slot owns (`bool b = x is > 0;`, `return x is > 0;`, a `when` guard) scores 0, one below the equivalent `x > 0`. PHP, Python, Tcl and iRules emit a bare `<` / `>` from no non-comparison production; C carries the same gate as its C-family siblings although, having no templates, it has nothing to exclude. The gate is a claim about the grammar's productions, not about every parse: where a grammar resolves a generic *call* into nested `binary_expression` nodes, as tree-sitter-kotlin-ng does for `id(a)`, no polarity can exclude it (#1394). | +| C++, Objective-C, Mozcpp, Rust, Go, Java, Groovy, C#, Kotlin, TypeScript, TSX, JavaScript, Mozjs, Lua, Perl, Ruby, Bash, Elixir | A `<` or `>` that is not a comparison is not a condition | Every one of these grammars spells at least one non-comparison construct with the same bare `<` / `>` token a comparison uses, so the comparison rule is gated on the token's parent. What that excludes, per family: template and generic brackets in C++, Objective-C, Mozcpp, Rust, Go, Java, Groovy, C#, Kotlin, TypeScript and TSX (#1274); JSX tag delimiters in TypeScript, TSX, JavaScript and Mozjs; Lua 5.4 variable attributes (`local x = 1`); a C# comparison-operator overload's declared name (`operator <`); Kotlin's qualified super call (`super.g()`) (#1297); Perl's filehandle and lexical-handle readlines (``, `<$fh>` — but not ``, which the grammar lexes as one token); Ruby's superclass clause (`class Foo < Bar`) and comparison-operator method names (`def <(other)`), and Bash I/O redirection (`cmd > out`) (#1280); Elixir's sigil delimiters (`~s`) (#1256). C# additionally excludes the operator of a relational pattern (`x is > 0`, and the `> 5 =>` arm of a switch expression): the arm or `if` condition slot that owns the pattern already scores the decision, so counting the operator too charged a relational arm twice what the constant arm `5 => 1` scores (#1383). Its `>=` / `<=` spelling is excluded by the same rule through a separate token. The declared name of an operator overload is excluded on every spelling too: C# overloads six comparison operators and gives each a distinct token, so `operator <=`, `operator >=`, `operator ==` and `operator !=` join `operator <` and `operator >`, which alone were excluded before #1420. The exclusion holds wherever the pattern sits, so a pattern no arm or condition slot owns (`bool b = x is > 0;`, `return x is > 0;`, a `when` guard) scores 0, one below the equivalent `x > 0`. PHP, Python, Tcl and iRules emit a bare `<` / `>` from no non-comparison production; C carries the same gate as its C-family siblings although, having no templates, it has nothing to exclude. The gate is a claim about the grammar's productions, not about every parse: where a grammar resolves a generic *call* into nested `binary_expression` nodes, as tree-sitter-kotlin-ng does for `id(a)`, no polarity can exclude it (#1394). | | Java, Groovy, C#, TypeScript, TSX | A `?` used as type syntax is not a ternary | In each of these grammars the ternary `?` and the type-syntax `?` are the *same* anonymous token, so the ternary rule above is gated on the token's parent. Java and Groovy exclude the wildcard bound `List` (#1274); C# excludes the nullable type `int? x` and the constraint `where T : class?`; TypeScript and TSX exclude optional parameters, properties, methods, class fields and tuple elements, and conditional types (`T extends U ? X : Y`, which the type checker resolves and erases before runtime, so it is no more a branch than the `<` / `>` already excluded) (#1275). Safe navigation is untouched: C#'s `a?.b` shares the same token and still counts, while the other languages spell theirs as a distinct one. | #### Worked example diff --git a/src/metrics/abc.rs b/src/metrics/abc.rs index fbdcd611..eee35991 100644 --- a/src/metrics/abc.rs +++ b/src/metrics/abc.rs @@ -3789,6 +3789,131 @@ mod tests { ); } + // #1420, the other four spellings of the test above. C# overloads six + // comparison operators and #1297 gated two: `<=` `>=` `==` `!=` are + // distinct tokens that reach a different arm of + // `csharp_count_token_condition` and each still scored one spurious + // condition on its declaration — measured 1 apiece against `<` / `>`'s + // 0, on a class where `cyclomatic()` is 1 for all six. + // + // Each token gets its own member so a partial fix names the spelling + // it missed: gating `EQEQ | BANGEQ` and forgetting `GTEQ | LTEQ` fails + // here by operator, where the file total cannot tell the two apart — + // it reads 4 before the fix and 0 after, so any two-of-four fix halves + // it and still looks like movement. + // + // `n` is the deliberate control (§11): the same four tokens *applied*, + // inside `binary_expression` parents, which must keep scoring one + // condition each. Without it the gate could be satisfied by refusing + // to count these tokens at all. + #[test] + fn csharp_comparison_operator_overloads_are_not_conditions() { + let src = "class V { + public static bool operator <=(V a, V b) { return true; } + public static bool operator >=(V a, V b) { return true; } + public static bool operator ==(V a, V b) { return true; } + public static bool operator !=(V a, V b) { return true; } + int n(int a, int b) { + if (a <= b) { return 1; } + if (a >= b) { return 2; } + if (a == b) { return 3; } + if (a != b) { return 4; } + return 0; + } + }"; + // Two of each token: one declaring, one applying. Trimming either + // half out collapses this test into the other half's control and + // is what this anchor exists to catch. + assert_csharp_fixture_spells( + src, + &[ + ( + Csharp::OperatorDeclaration as u16, + 4, + "operator declarations", + ), + (Csharp::LTEQ as u16, 2, "`<=`"), + (Csharp::GTEQ as u16, 2, "`>=`"), + (Csharp::EQEQ as u16, 2, "`==`"), + (Csharp::BANGEQ as u16, 2, "`!=`"), + ], + ); + check_func_space::(src, "foo.cs", |space| { + let class = &space.spaces[0]; + assert_eq!(class.spaces.len(), 5, "four overloads plus `n`"); + for (member, spelling) in class.spaces.iter().zip(["<=", ">=", "==", "!="]) { + assert_eq!( + member.metrics.abc.conditions(), + 0, + "`operator {spelling}` declares an operator, it does not apply one" + ); + assert_eq!( + member.metrics.abc.conditions(), + member.metrics.cyclomatic.cyclomatic() - 1, + "`operator {spelling}`: §8 parity with the cyclomatic decision count" + ); + } + // The control, found by name so a reordered fixture cannot + // silently re-point it at an overload. 0 here would mean the + // gate swallowed the applying form along with the declaring + // one. + let control = &class.spaces[4]; + assert_eq!(control.name.as_deref(), Some("n")); + assert_eq!( + control.metrics.abc.conditions(), + 4, + "`n` applies all four tokens: one condition each" + ); + }); + } + + // The allowlist #1420 installed is the only thing keeping `#if A == B` + // a condition, and it keeps it for a reason no arm states: the grammar + // aliases `preproc_binary_expression` onto `binary_expression`, so the + // preprocessor's `==` arrives with kind id `BinaryExpression` (369) + // and not with one of its own. Before #1420 the token was ungated and + // no fact about its parent mattered; now a grammar bump that gave the + // alias a distinct id would stop counting every `#if` equality with + // nothing red. This is that pin. + // + // `BinaryExpression2` (469) is the pre-alias symbol the arm also + // lists, defensively, per `.claude/rules/grammar-dispatch.md` §1. §2 + // asks that such an entry be kept *and* have its unreachability + // asserted, so that a pin promoting the symbol changes behaviour + // loudly rather than invisibly. + #[test] + fn csharp_preproc_equality_counts_through_the_binary_expression_alias() { + let src = "class P { + #if A == B + int x; + #endif + #if C != D + int y; + #endif + }"; + let parser = CsharpParser::new( + src.as_bytes().to_vec(), + std::path::Path::new("foo.cs"), + None, + ); + assert!( + ast_has_kind_id(&parser, Csharp::BinaryExpression as u16), + "the `#if` expressions must parse as `binary_expression` — \ + the alias is what the arm's allowlist matches" + ); + assert!( + !ast_has_kind_id(&parser, Csharp::BinaryExpression2 as u16), + "`BinaryExpression2` is the pre-alias symbol and must stay \ + unreachable; if the grammar starts emitting it, re-derive the \ + allowlist rather than trusting this arm" + ); + // 2, one per `#if` expression. The two directives carry no + // declarations the walk would score, so nothing else contributes. + check_metrics::(src, "foo.cs", |metric| { + assert_eq!(metric.abc.conditions_sum(), 2); + }); + } + // #1383: a `relational_pattern`'s operator is not a condition of // its own — the `switch_expression_arm` that owns it already scores // the decision, exactly as it does for the constant arm `5 => 1`. @@ -3802,21 +3927,23 @@ mod tests { // `>=` / `<=` in the file, so a readmitted `RelationalPattern` // parent is the only thing that can lift the count. // - // Both spellings are covered because they reach two different arms: - // `n`'s `>` / `<` are gated by the `GT | LT` parent allowlist, - // `g`'s `>=` / `<=` by their own `RelationalPattern` denial. Fixing - // one arm and not the other leaves the *other method* at 4, which - // is why this asserts per member and not through a total: + // Both spellings are covered, and asserted per member rather than + // through a total, because they reached two different arms when this + // was written: `n`'s `>` / `<` were gated by a `GT | LT` parent + // allowlist and `g`'s `>=` / `<=` by a separate `RelationalPattern` + // denial, so fixing one and not the other left the *other method* at + // 4. #1420 merged all six comparison tokens onto the one allowlist, + // which removes that particular way to get it half right — but the + // per-member shape still earns its keep, since a total of 6 is what + // both `{2, 2, 2}` and a regressed `{4, 2, 0}` report: // // | state | `n` | `g` | `c` | // |---|---|---|---| - // | both halves gated (shipped) | 2 | 2 | 2 | - // | only `GT \| LT` gated | 2 | 4 | 2 | - // | only `GTEQ \| LTEQ` gated | 4 | 2 | 2 | - // | neither | 4 | 4 | 2 | + // | `relational_pattern` excluded (shipped) | 2 | 2 | 2 | + // | readmitted to the allowlist | 4 | 4 | 2 | // - // `c` is the constant-pattern control and reads 2 in every column: - // it is what the relational methods are supposed to agree with. + // `c` is the constant-pattern control and reads 2 in both rows: it is + // what the relational methods are supposed to agree with. #[test] fn csharp_relational_pattern_does_not_double_count_its_arm() { let src = "class A { diff --git a/src/metrics/abc/csharp.rs b/src/metrics/abc/csharp.rs index 892004ab..61c0b6c2 100644 --- a/src/metrics/abc/csharp.rs +++ b/src/metrics/abc/csharp.rs @@ -107,10 +107,12 @@ fn csharp_count_unary_conditions(list_node: &Node, conditions: &mut f64) { // ABC token-level helpers for C#. Mirror of Java's helper layout with // C#-specific deltas: every aliased kind id is matched via the // `csharp_*_kinds!()` macros (lesson #2); `ObjectCreationExpression` -// joins `InvocationExpression*` as a branch; each of the four tokens a -// `relational_pattern` can spell (`<` `>` `<=` `>=`) excludes that -// parent, because a C# pattern's operator belongs to the arm that owns -// it rather than scoring on its own (#1383); +// joins `InvocationExpression*` as a branch; all six comparison tokens +// share one arm allowlisting a `binary_expression` parent, which excludes +// type syntax (#1275), the declared name of an operator overload (#1297, +// #1420) and a `relational_pattern`'s operator — the last because a C# +// pattern's operator belongs to the arm that owns it rather than scoring +// on its own (#1383); // `ConditionalExpression` replaces Java's `TernaryExpression`; // `for_statement` exposes its condition via the named `condition` // field rather than positional index. @@ -222,32 +224,98 @@ fn csharp_count_token_condition<'a>( // arrow `default ->` forms) is the unconditional fallthrough and // is excluded, mirroring cyclomatic's `Case`-only count and the // expression-arm discard rule below (issues #456, #469). - EQEQ | BANGEQ | Else | Case | Try | Catch => { + // + // These four stay ungated; `EQEQ` / `BANGEQ` shared the arm until + // #1420 and moved to the gated one below. `Else`, `Try` and + // `Catch` come from one production each (`if_statement`, + // `try_statement`, `catch_clause`), so there is nothing to gate on. + // `Case` comes from two — `switch_section` and `goto_statement` — + // and the second is over-counted: FIXME(#1450), `goto case 2;` + // scores a condition without being an arm. Left alone rather than + // missed: C# cyclomatic counts the same token + // (`src/metrics/cyclomatic/csharp.rs`), so gating it here alone + // would break the §8 parity `conditions == cyclomatic() - 1` that + // a two-arm-plus-`goto case` method currently satisfies at + // 3 == 4 - 1. Both arms have to move together. `goto default;` + // costs nothing already, because `Default` is excluded as the + // switch's unconditional fallthrough. + Else | Case | Try | Catch => { stats.conditions += 1.; } - // The other half of #1383. A `relational_pattern` spells its - // operator as any of `<` `>` `<=` `>=`, and the last two are - // distinct tokens that never reach the `GT | LT` arm below, so - // fixing only that arm would leave `x switch { >= 10 => … }` - // double-counted while `x switch { > 10 => … }` was not — the - // same bug surviving under a different token. The arm that owns - // the pattern is the decision; see the `GT | LT` comment. + // All six C# comparison tokens, counted only where they *apply* + // an operator. A `grammar.json` sweep of tree-sitter-c-sharp + // 0.23.5 finds each one in these productions, and only + // `binary_expression` (`a < b`) qualifies: + // + // `<` / `>` — `binary_expression`, `relational_pattern`, + // `operator_declaration`, `type_argument_list`, + // `type_parameter_list`, `function_pointer_type` + // `<=` / `>=` — `binary_expression`, `relational_pattern`, + // `operator_declaration` + // `==` / `!=` — `binary_expression`, `preproc_binary_expression` + // (`#if A == B`), `operator_declaration` + // + // Three reasons to exclude, one per production family: + // + // `type_argument_list` (`Dictionary`), `type_parameter_list` + // (`class Foo`) and `function_pointer_type` (`delegate*`) are type syntax, no more a decision than the `?` of a + // nullable type (#1275). + // + // `operator_declaration` — `public static bool operator <(V a, + // V b)` — names the operator being *defined* rather than applying + // it. #1297 found this for `<` / `>`, whose denylist at the time + // named the three type-syntax parents and not this one, so every + // comparison-operator overload scored a condition. C# overloads + // six operators, though, and the other four are distinct tokens + // that reached a different arm: `<=` `>=` `==` `!=` each still + // scored 1 against `<` / `>`'s 0 on a class overloading all six, + // every member of which has `cyclomatic()` 1 (#1420). + // + // `relational_pattern` is excluded for a third reason (#1383): a + // pattern's comparison operator is not its own decision, since the + // enclosing `switch_expression_arm` or `if` condition slot already + // scores one. Counting the operator too charged + // `x switch { > 5 => … }` twice what the constant arm + // `x switch { 5 => … }` scores, and twice C#'s own cyclomatic + // decision count. + // + // Allowlist polarity, matching Java's #1274 fix and unlike the + // `QMARK` arm below: one decision parent against six excluded + // productions, and a grammar bump that grows a seventh should fail + // closed (`.claude/rules/grammar-dispatch.md` §1). #1383 landed + // half of this as a denylist naming `relational_pattern` alone, + // which is what let `operator_declaration` through for four of the + // six tokens; the allowlist subsumes that denial. The `QMARK` arm + // takes the opposite polarity for a reason specific to that token + // — see its comment. // - // Denylist polarity here rather than the allowlist the `GT` / - // `LT` arm uses, because these two tokens have no type syntax - // to fail closed against: `<=` and `>=` reach this arm from - // `binary_expression`, `relational_pattern` and - // `operator_declaration` only. That third parent is the - // `>=` / `<=` spelling of #1297's operator-overload bug, which - // that fix reached only through the `GT` / `LT` allowlist and - // which `==` / `!=` share. + // `BinaryExpression2` is the second enum id carrying the + // `binary_expression` kind string, listed per §1. Measured at this + // pin it is unreachable from here: the `preproc_binary_expression` + // of `#if A == B` parses as `BinaryExpression` (369), so the + // preprocessor spelling counts through the first entry, and C#'s + // preprocessor admits only `== != && || !` in any case. // - // FIXME(#1420): `operator <=` / `>=` / `==` / `!=` still score - // a spurious condition each, measured. Left alone here so this - // commit changes one behaviour; the fix is to give all four - // tokens the `BinaryExpression` allowlist polarity, which - // subsumes the denial below. - GTEQ | LTEQ if !ancestors.parent_has_kind(node, RelationalPattern as u16) => { + // A failed guard returns `false` and falls through to + // `csharp_walk_for_conditions`, which matches none of these six + // token kinds — so the fall-through is a no-op. + // + // Failing closed also covers error recovery, where the sweep + // above says nothing: a token the parser reparents under `{ERROR}` + // stops counting. `<` / `>` have behaved that way since #1297 and + // the other four now agree, valid input is unaffected, and no C# + // corpus file parses with an `{ERROR}` node — measured. Nothing + // pins it, because a fixture the language rejects would make the + // grammar's present recovery the contract (§6). + GT | LT | GTEQ | LTEQ | EQEQ | BANGEQ + if ancestors.parent(node).is_some_and(|parent| { + matches!( + parent.kind_id().into(), + BinaryExpression | BinaryExpression2 + ) + }) => + { stats.conditions += 1.; } // tree-sitter-c-sharp emits a bare `?` from exactly four @@ -312,57 +380,6 @@ fn csharp_count_token_condition<'a>( { stats.conditions += 1.; } - // Counts `<` / `>` only where they score a decision of their - // own. A `grammar.json` sweep of tree-sitter-c-sharp 0.23.5 - // finds a bare `<` / `>` in exactly six productions, and only - // `binary_expression` (`a < b`) qualifies. Four are type - // syntax — `type_argument_list` (`Dictionary`), - // `type_parameter_list` (`class Foo`), `function_pointer_type` - // (`delegate*`) and `operator_declaration` - // (`public static bool operator <(V a, V b)`), whose `<` names - // the operator being *defined* rather than applying it. The - // sixth is `relational_pattern`, excluded for a different - // reason — see below. - // - // The previous denylist named three of those four and not - // `operator_declaration`, so every comparison-operator overload - // scored a condition per declaration (#1297). Allowlist - // polarity, matching Java's #1274 fix and unlike the `QMARK` - // arm above: `<` / `>` have one decision parent against five - // excluded productions, and a grammar bump that grows a seventh - // should fail closed - // (`.claude/rules/grammar-dispatch.md` §1). The `QMARK` arm - // takes the opposite polarity for a reason specific to that - // token — see its comment. - // - // `relational_pattern` is deliberately *not* in the allowlist - // (#1383). A pattern's comparison operator is not its own - // decision: the enclosing `switch_expression_arm` or `if` - // condition slot already scores one, so counting the operator - // too charged `x switch { > 5 => … }` twice what the constant - // arm `x switch { 5 => … }` scores and twice C#'s own - // cyclomatic decision count. #1297 kept the entry to avoid an - // unmeasured behaviour change riding along with it; this is - // that measured change. - // - // `BinaryExpression2` is the id the grammar aliases - // `preproc_binary_expression` to, and it is listed defensively - // per §1 rather than because it is reachable: C#'s preprocessor - // admits only `== != && || !`, so no bare `<` / `>` can have - // that parent at this pin. It is genuinely reachable in the - // C / C++ arms this one mirrors, where `#if A < B` is legal. - // `<=` / `>=` and the shifts are distinct tokens and never - // reach this arm. - GT | LT - if ancestors.parent(node).is_some_and(|parent| { - matches!( - parent.kind_id().into(), - BinaryExpression | BinaryExpression2 - ) - }) => - { - stats.conditions += 1.; - } _ => return false, } true From 1f668be9f9e374fee2e9d844b4fca8481bd7fe08 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 13 Sep 2026 19:15:24 -0700 Subject: [PATCH 02/11] fix(abc/csharp): count primary-constructor base calls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A C# 12 primary constructor passes arguments to its base in the declaration header — `class Sub(int x) : Base(x)` — which invokes the base constructor exactly as the `: base(x)` initializer #1279 added does. It scored zero. Measured before: `class Sub1(int x) : Base(x)`, `record R1(int x) : Base(x);` and `record class R2(int x) : Base(x);` all 0 branches, against 1 for the `: base(x)` spelling beside them. tree-sitter-c-sharp 0.23.5 spells the two declaration families differently, so there is no single node to match — verified by `bca dump`, not inferred. A record nests the arguments under a `primary_constructor_base_type`; a class hangs them straight off the `base_list`. Matching only the former covers records and misses every class, so the two are independent paths (§11) and both are listed. Both kinds are gated on a `base_list` parent. The gate is load-bearing on `ArgumentList`, which is otherwise the argument list of every call in the file: ungated, 19 existing C# tests fail and each call bills two branches instead of one. This is §5's container-plus-containable shape — a `primary_constructor_base_type` holds an `argument_list` — and the gate is what makes it safe rather than a double count: in the record spelling the inner list's parent is the wrapper, not the base list, so the two are never both matched. `BaseList` and `BaseList2` both render to "base_list" and only 252 is observed; both are listed per §1, with a zero-count drift marker pinning that 246 is unemitted. An argument-less base type still costs nothing — `struct S(int x) : IBase`, `record R(int x) : Base`, `enum E : byte`, `interface I : IBase` emit neither node. Attribute, indexer and type argument lists are distinct kind ids and never occur under a base list. Matching `ArgumentList` makes `compute` return before `csharp_walk_for_conditions`, so a base list's argument list no longer reaches that function's `ArgumentList` arm. Measured harmless: the arm is dead for every argument list, because an `argument_list`'s children are `argument` wrappers `csharp_inspect_container` rejects on the first iteration — `Helper(!b)` and `Helper((b))` each score zero conditions before this change as well as after. Repairing it means revisiting that exclusion, not this arm. Three grammar-reachable shapes are not valid C# and now score 1 (`interface I : IBase(x)`, `enum E : Base(x)`, a base call on a class with no primary constructor). No valid program distinguishes the behaviours, so per §6 the gap is documented in the arm and left untested rather than pinned. Java and Groovy were swept and need no sibling fix: neither language has a primary constructor, their `superclass` node carries no argument list, and a base call is an `explicit_constructor_invocation` / `method_invocation` already counted. Kotlin was fixed in #1384. No corpus snapshot moves — no C# corpus file uses the construct. Fixes #1406 --- src/metrics/abc.rs | 177 +++++++++++++++++++++++++++++++++++++- src/metrics/abc/csharp.rs | 89 ++++++++++++++++--- 2 files changed, 253 insertions(+), 13 deletions(-) diff --git a/src/metrics/abc.rs b/src/metrics/abc.rs index eee35991..adc54969 100644 --- a/src/metrics/abc.rs +++ b/src/metrics/abc.rs @@ -455,7 +455,8 @@ implement_metric_trait!(Abc, PreprocCode, CcommentCode); )] mod tests { use crate::test_support::{ - ast_has_kind_id, check_func_space_only_shim, check_metrics_only_shim, metrics_verbatim, + ast_has_kind_id, check_func_space_only_shim, check_metrics_only_shim, child_space, + metrics_verbatim, }; use crate::traits::ParserTrait; @@ -1765,6 +1766,180 @@ mod tests { ); } + #[test] + fn csharp_primary_constructor_base_call_is_a_branch() { + // The C# 12 primary-constructor superclass call invokes the base + // constructor exactly as the `: base(…)` above it does, and scored + // zero until #1406 — the C# sibling of Kotlin's #1384. + // + // tree-sitter-c-sharp 0.23.5 spells the `record` and `class` + // families differently, so per `.claude/rules/grammar-dispatch.md` + // §11 they are two independent paths and each needs its own + // fixture: the record form nests the arguments under a + // `primary_constructor_base_type`, the class form hangs them + // straight off the `base_list`. A record-only fixture says nothing + // about the class arm, and vice versa. + // + // `S` and `R3` are the negatives: a base type with no argument + // list is not a call, and neither spelling emits an + // `argument_list` for it. + let src = "class Sub(int x) : Base(x) { } + record R1(int x) : Base(x); + record class R2(int x) : Base(x); + class G(int x) : GBase(x) where T : class { } + class Multi(int x) : Base(x), IBase { } + struct S(int x) : IBase { } + record R3(int x) : Base;"; + // Anchors every spelling against a fixture edit: with presence + // checks only, deleting `(x)` from the class form leaves the + // record form satisfying the arm and this test green. The last + // two counts anchor the declarations that would otherwise decay + // into duplicates of `Sub` — `Multi` owns the fixture's only + // comma (`Base(x), IBase`) and `G` its only type argument list + // (`GBase`), so trimming either back fails here by name. + assert_csharp_fixture_spells( + src, + &[ + ( + Csharp::PrimaryConstructorBaseType as u16, + 2, + "`record` base calls", + ), + (Csharp::ArgumentList as u16, 5, "base-call argument lists"), + (Csharp::BaseList2 as u16, 7, "base lists"), + // The §2 drift marker for the arm's other half. Both ids + // render to `"base_list"`; at this pin every declaration + // family above emits 252 and none emits 246, so + // `BaseList` is defensive and this zero says so. A + // grammar bump that starts emitting it fails here rather + // than silently changing which arm does the work. + (Csharp::BaseList as u16, 0, "unaliased `base_list` nodes"), + (Csharp::COMMA as u16, 1, "`Multi`'s trailing interface"), + (Csharp::TypeArgumentList as u16, 1, "`G`'s generic base"), + ], + ); + check_func_space::(src, "foo.cs", |space| { + for name in ["Sub", "R1", "R2", "G", "Multi"] { + assert_eq!( + child_space(&space, name).metrics.abc.branches(), + 1, + "{name}: the primary-constructor base call is one branch" + ); + } + for name in ["S", "R3"] { + assert_eq!( + child_space(&space, name).metrics.abc.branches(), + 0, + "{name}: a base type with no argument list is not a call" + ); + } + }); + } + + #[test] + fn csharp_base_list_gate_excludes_other_argument_lists() { + // The `base_list` parent gate on the `ArgumentList` arm is + // load-bearing, not decoration: ungated it bills a branch for the + // argument list of every call and allocation in the file, on top + // of the `InvocationExpression` / `ObjectCreationExpression` the + // arm already counts — doubling both (#1406). + // + // The fixture pairs one real base call with the four other + // argument-carrying productions C# spells nearby: an attribute + // (`attribute_argument_list`), an invocation, an object creation, + // and an indexer (`bracketed_argument_list`) alongside a generic + // (`type_argument_list`). Those last three are distinct kind ids + // rather than `argument_list`, so they pin the §1 neighbour set + // as much as the gate. + let src = "[System.Obsolete(\"why\")] + class A(int x) : Base(x) { + void M(int y) { Helper(y); } + object N() { return new Foo(1); } + int P(System.Collections.Generic.List l, int i) { return l[i]; } + }"; + assert_csharp_fixture_spells( + src, + &[ + ( + Csharp::AttributeArgumentList as u16, + 1, + "attribute argument lists", + ), + ( + Csharp::BracketedArgumentList as u16, + 1, + "bracketed argument lists", + ), + (Csharp::TypeArgumentList as u16, 1, "type argument lists"), + (Csharp::ArgumentList as u16, 3, "argument lists"), + ], + ); + check_func_space::(src, "foo.cs", |space| { + let class = child_space(&space, "A"); + assert_eq!( + class.metrics.abc.branches(), + 1, + "`: Base(x)` alone — the attribute's arguments are not a call" + ); + // The controls. 2 apiece would mean the gate let the + // argument list through beside the call that owns it. + assert_eq!( + child_space(class, "M").metrics.abc.branches(), + 1, + "`Helper(y)` is one branch, not one per argument list" + ); + assert_eq!( + child_space(class, "N").metrics.abc.branches(), + 1, + "`new Foo(1)` is one branch, not one per argument list" + ); + // `P` carries the indexer and the generic deliberately, but + // gets no assertion: `bracketed_argument_list` and + // `type_argument_list` are distinct kind ids, so `P` reads 0 + // with the gate, without it, and without this change + // altogether. The census above is what actually pins them — + // an assertion here would read as coverage it cannot provide. + }); + } + + #[test] + fn csharp_base_call_and_constructor_initializer_do_not_double_count() { + // A class can spell *both* delegations at once: the primary + // constructor's `: Base(x)` and a secondary constructor's + // `: this(a)`. They are separate nodes in separate spaces, so the + // file scores 2 — one each. 3 would mean the #1406 arm also fired + // on something `ConstructorInitializer` already owned; 1 would + // mean one of the two stopped counting. + // + // Java has the equivalent guard + // (`java_constructor_delegation_does_not_double_count_arguments`); + // C# now has two delegation spellings and this is the pairing + // test for them. + let src = "class Both(int x) : Base(x) { + public Both(int a, int b) : this(a) { } + }"; + assert_csharp_fixture_spells( + src, + &[ + (Csharp::ConstructorInitializer as u16, 1, "`: this(a)`"), + (Csharp::BaseList2 as u16, 1, "the primary-constructor base"), + ], + ); + check_func_space::(src, "foo.cs", |space| { + let class = child_space(&space, "Both"); + assert_eq!( + class.metrics.abc.branches(), + 1, + "the class space owns `: Base(x)` and nothing else" + ); + assert_eq!( + child_space(class, "Both").metrics.abc.branches(), + 1, + "the secondary constructor owns `: this(a)` and nothing else" + ); + }); + } + #[test] fn kotlin_constructor_delegation_is_a_branch() { // Kotlin's secondary-constructor delegation is a diff --git a/src/metrics/abc/csharp.rs b/src/metrics/abc/csharp.rs index 61c0b6c2..bbbe13ff 100644 --- a/src/metrics/abc/csharp.rs +++ b/src/metrics/abc/csharp.rs @@ -189,27 +189,84 @@ fn csharp_count_token_assignment<'a>( } // Counts branch tokens: every invocation, `new` allocation, and -// constructor delegation. +// constructor delegation in either of its two spellings. // `ConstructorInitializer` is the `: base(…)` / `: this(…)` delegation on a // constructor — a call by Fitzpatrick's rule, and the C# spelling of the // shape Java and Groovy count (#1279). Unlike the invocation kinds it // carries no numeric-suffix aliases, and it does not wrap an // `InvocationExpression` for the delegation itself, so no double count // arises; calls in its argument list are separate nodes counted on their own. -fn csharp_count_token_branch(node: &Node, stats: &mut Stats) -> bool { +fn csharp_count_token_branch<'a>( + node: &Node<'a>, + ancestors: Ancestors<'a, '_>, + stats: &mut Stats, +) -> bool { use Csharp::*; - if matches!( - node.kind_id().into(), + let is_branch = match node.kind_id().into() { + // Spelled out rather than reaching for `csharp_invocation_expr_kinds!()` + // (`big-code-analysis-ast/src/macros/kind_sets.rs`), which carries + // exactly these three: the macro *is* an or-pattern, so combining it + // with further alternatives here trips `clippy::unnested_or_patterns` + // at `-D warnings`. `Checker::is_csharp_call` can use it because the + // set is that predicate's whole answer. The §1 obligation is met by + // listing all three aliases, not by the macro. crate::Csharp::InvocationExpression - | crate::Csharp::InvocationExpression2 - | crate::Csharp::InvocationExpression3 - | ObjectCreationExpression - | ConstructorInitializer - ) { + | crate::Csharp::InvocationExpression2 + | crate::Csharp::InvocationExpression3 + | ObjectCreationExpression + | ConstructorInitializer => true, + // The C# 12 primary-constructor superclass call — `class + // Sub(int x) : Base(x)` — invokes the base constructor exactly as + // `: base(x)` does, and scored zero until #1406. Kotlin's + // equivalent was fixed in #1384; this is the C# sibling. + // + // tree-sitter-c-sharp 0.23.5 spells the two declaration families + // differently, so there is no single node to match (verified with + // `bca dump`, not inferred): + // + // `record R(int x) : Base(x);` base_list > primary_constructor_ + // base_type > argument_list + // `class C(int x) : Base(x) {}` base_list > argument_list (flat) + // + // Matching both kinds under a `base_list` parent therefore covers + // both families. This is §5's container-plus-containable shape — + // a `primary_constructor_base_type` *holds* an `argument_list` — + // and the parent gate is what makes it safe rather than a double + // count: in the record spelling the inner `argument_list`'s parent + // is the `primary_constructor_base_type`, not the `base_list`, so + // only the outer node fires. The two are never siblings, so + // neither spelling ever presents both. `record R(int x) : Base;` + // and `struct S(int x) : IBase {}` emit no `argument_list` and no + // `primary_constructor_base_type` at all, so an argument-less base + // type still costs nothing, and `enum E : byte` / `interface I : + // IBase` cost nothing for the same reason. + // + // Three grammar-reachable shapes are not valid C# and score 1 + // here: `interface I : IBase(x)`, `enum E : Base(x)`, and a + // `class NoCtor : Base(x)` with no primary constructor. Each + // parses cleanly — the grammar is more permissive than the + // language — and no valid program can tell the behaviours apart, + // so per §6 the gap is documented and left untested rather than + // pinned, which would make the grammar's present permissiveness + // the contract. + // + // The parent gate is load-bearing on `ArgumentList`, which is + // otherwise the argument list of every call in the file. It is + // also what keeps the sibling `attribute_argument_list` / + // `bracketed_argument_list` / `type_argument_list` kinds out — + // they are distinct kind ids, and none of them occurs under a + // `base_list`. `BaseList` and `BaseList2` both render to + // `"base_list"` and only the second is observed here; both are + // listed per `.claude/rules/grammar-dispatch.md` §1. + PrimaryConstructorBaseType | ArgumentList => ancestors + .parent(node) + .is_some_and(|parent| matches!(parent.kind_id().into(), BaseList | BaseList2)), + _ => false, + }; + if is_branch { stats.branches += 1.; - return true; } - false + is_branch } fn csharp_count_token_condition<'a>( @@ -398,6 +455,14 @@ fn csharp_walk_for_conditions<'a>( csharp_count_unary_conditions(&parent, conds); } } + // `compute` returns as soon as `csharp_count_token_branch` fires, + // so since #1406 an `argument_list` under a `base_list` no longer + // reaches this arm. Measured harmless: the arm is dead for *every* + // argument list, because an `argument_list`'s children are + // `argument` wrappers that `csharp_inspect_container` rejects on + // the first iteration — `Helper(!b)` and `Helper((b))` both score + // zero conditions today. Repairing it means revisiting that + // exclusion, not just this arm. ArgumentList => csharp_count_unary_conditions(node, conds), // tree-sitter-c-sharp `if_statement` / `while_statement` shape: // [`if`/`while`, `(`, condition, `)`, body, …]. The parens are @@ -484,7 +549,7 @@ impl Abc for CsharpCode { if csharp_count_token_assignment(node, ancestors, stats) { return; } - if csharp_count_token_branch(node, stats) { + if csharp_count_token_branch(node, ancestors, stats) { return; } if csharp_count_token_condition(node, ancestors, stats) { From efe0c8534a4ff33f9021b26bf6e781415e3dd6ab Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 13 Sep 2026 19:50:31 -0700 Subject: [PATCH 03/11] fix(csharp): count `when` guards as decisions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A C# `when` guard is a branch and neither metric modelled it. Cyclomatic had no arm for either spelling — `when_clause`, shared by `switch_expression_arm` and `switch_section`, and `catch_filter_clause` on `catch (E e) when (…)` — so a guarded arm contributed one decision where it fails two ways: the pattern does not match, or it matches and the guard is false. ABC counted whatever operator token happened to sit inside the guard, so three semantically identical guards produced two numbers: `when x % 2 == 0` and `when x > 2` scored one condition via the comparison-token arm, `when IsEven(x)` scored none. Cyclomatic now counts both clause kinds toward the standard and modified tiers, matching Rust, whose match guard already counts. ABC models the guard as a condition slot rather than suppressing its operator: every spelling contributes exactly one, and a compound guard (`when a > 1 && b < 2`) keeps its sub-structure instead of collapsing. Suppression would have reached the same internal agreement one count below C#'s own decision count. Two grammar details the slot turns on. The clause nodes, not the `when` keyword they share: `When` is also a `_reserved_identifier` at this pin, so `int when = 1;` emits it under an `identifier` and the token would have scored a decision per mention of the variable. And every named child, not the first: tree-sitter extras are named nodes, so a leading comment (`when /*c*/ g`) would otherwise hand the slot a `comment` and silently restore the spelling-dependence this removes. Costs one corpus snapshot. `control_flow.cs` gains a cyclomatic decision on `Bucket` (switch guard) and `SafeDivide` (catch filter), rippling into `mi`, `wmc` and the file aggregates; ABC is unchanged there because both corpus guards are operator-shaped. Fixes #1422 --- src/metrics/abc.rs | 415 +++++++++++++++++--- src/metrics/abc/csharp.rs | 56 ++- src/metrics/cyclomatic.rs | 30 +- src/metrics/cyclomatic/csharp.rs | 27 ++ tests/repositories/big-code-analysis-output | 2 +- 5 files changed, 465 insertions(+), 65 deletions(-) diff --git a/src/metrics/abc.rs b/src/metrics/abc.rs index adc54969..8ceaca20 100644 --- a/src/metrics/abc.rs +++ b/src/metrics/abc.rs @@ -530,6 +530,40 @@ mod tests { } } + // The second sibling, for a fixture whose point is that a construct + // moves a member *off* the value its unguarded control scores. + // `assert_every_member_scores` cannot serve those: it takes one + // expected value for the whole container, and every #1422 guard + // fixture deliberately pairs guarded members with an unguarded + // control one lower — which is the comparison the test exists to + // make. Both tiers per member, because a guard is a decision in each + // and a fix that moved only one of them would satisfy neither claim + // on its own. + fn assert_csharp_members_score(container: &crate::FuncSpace, expected: &[(&str, u64, u64)]) { + assert_eq!( + container.spaces.len(), + expected.len(), + "member count changed — the fixture moved, not the metric" + ); + for (name, conditions, cyclomatic) in expected { + let member = container + .spaces + .iter() + .find(|s| s.name.as_deref() == Some(*name)) + .unwrap_or_else(|| panic!("fixture lost `{name}`")); + assert_eq!( + member.metrics.abc.conditions(), + *conditions, + "{name}: abc.conditions" + ); + assert_eq!( + member.metrics.cyclomatic.cyclomatic(), + *cyclomatic, + "{name}: cyclomatic" + ); + } + } + // #1383's fix makes a relational pattern's operator score *zero*, so // its tests have no second axis to anchor on: trimming `> 5` down to // `5` leaves every assertion satisfied and the construct under test @@ -4252,71 +4286,346 @@ mod tests { // guard too. // // A relational *pattern* in the guard (`g`) is the other side of that - // line: it scores nothing, though no slot pays for it, because the - // gate is on the operator's parent and a guard is a decision slot - // neither metric models. It is the outside-slot trade - // `csharp_relational_pattern_outside_a_decision_slot_scores_zero` - // records, and it leaves `when n is > 5` one below `when n > 5`; - // modelling the guard as a slot would score the `is` test once and - // close the gap (#1422). + // line: the operator itself still scores nothing, because the gate is + // on the operator's parent — but since #1422 the guard is a condition + // slot, and an `is` test is one of `csharp_bool_terminal_kinds!()`, so + // the slot pays for it. That is what closed the gap this test used to + // carry a `FIXME(#1422)` for: `when n is > 5` now reads level with + // `when n > 5` instead of one below it. // - // It is also where §8 does not hold, and the reason is worth - // stating precisely, because the obvious reading is wrong. Neither - // metric models the guard as a branch — C# cyclomatic has no - // `when_clause` arm, and ABC has no guard rule either. ABC's extra - // count is simply the `==` *token*, which happens to sit inside the - // guard. Measured: + // §8 holds throughout, which it did not before #1422. The old table + // here recorded the defect: neither metric modelled the guard, so + // ABC's count was whatever operator token happened to sit inside it + // and cyclomatic's was nothing at all. Both tiers measured, before + // and after: // // | guard | `conditions()` | `cyclomatic() - 1` | // |---|---|---| - // | `when x % 2 == 0` (this fixture) | 3 | 2 | - // | `when x > 2` | 3 | 2 | - // | `when IsEven(x)` | 2 | 2 | - // - // So the gap is not "ABC models guards better"; it is that a - // call-shaped guard restores parity while an operator-shaped one - // does not. That inconsistency is real, predates #1383, and is - // filed rather than changed here — see #1422. + // | `when x % 2 == 0` (this fixture) | 3 → 3 | 2 → 3 | + // | `when x > 2` | 3 → 3 | 2 → 3 | + // | `when IsEven(x)` | 2 → 3 | 2 → 3 | // // The `==` is the guard's own operator and the only `==` in the - // file, so trimming the `when` clause out of the fixture drops the - // count to 2 rather than leaving the assertion satisfied by - // something else. + // file, so trimming the `when` clause out of `w` drops its count to + // 2 rather than leaving the assertion satisfied by something else; + // `assert_csharp_fixture_spells` pins both guards by kind against + // the same decay. #[test] fn csharp_switch_arm_guard_operator_still_counts() { - check_func_space::( - "class A { + let src = "class A { int w(int x) => x switch { > 0 when x % 2 == 0 => 1, > 0 => 2, _ => 3 }; int g(int x) => x switch { int n when n is > 5 => 1, _ => 0 }; - }", - "foo.cs", - |space| { - let m = &space.spaces[0].spaces[0]; - assert_eq!(m.name.as_deref(), Some("w")); - // 4 if the `when_clause` itself started counting, 2 if - // the guard's `==` were suppressed along with the - // pattern operators — both are live regressions. - assert_eq!( - m.metrics.abc.conditions(), - 3, - "two arms plus the guard's `==`" - ); - assert_eq!(m.metrics.cyclomatic.cyclomatic(), 3); - - let g = &space.spaces[0].spaces[1]; - assert_eq!(g.name.as_deref(), Some("g")); - // FIXME(#1422): the one non-discard arm alone. The guard's - // pattern `>` scores nothing where `when n > 5` would add - // one; a guard modelled as a condition slot would score the - // `is` test instead, taking this to 2. - assert_eq!( - g.metrics.abc.conditions(), + }"; + assert_csharp_fixture_spells( + src, + &[ + (Csharp::WhenClause as u16, 2, "`when` guards"), + (Csharp::EQEQ as u16, 1, "the guard's `==`"), + (Csharp::RelationalPattern as u16, 3, "relational patterns"), + ], + ); + check_func_space::(src, "foo.cs", |space| { + let m = &space.spaces[0].spaces[0]; + assert_eq!(m.name.as_deref(), Some("w")); + // 4 if the guard slot started counting a `binary_expression` + // on top of its `==`, 2 if the `==` were suppressed along + // with the pattern operators — both are live regressions. + assert_eq!( + m.metrics.abc.conditions(), + 3, + "two arms plus the guard's `==`" + ); + assert_eq!(m.metrics.cyclomatic.cyclomatic(), 4); + + let g = &space.spaces[0].spaces[1]; + assert_eq!(g.name.as_deref(), Some("g")); + assert_eq!( + g.metrics.abc.conditions(), + 2, + "one arm plus the guard slot, which scores the `is` test" + ); + assert_eq!(g.metrics.cyclomatic.cyclomatic(), 3); + }); + } + + // #1422's headline: four spellings of one guard, previously worth + // two different ABC numbers because nothing modelled the guard and + // ABC counted whatever operator token happened to sit inside it — + // `when x % 2 == 0` and `when x > 2` scored 1 through the + // comparison-token arm, `when IsEven(x)` scored 0. Modelled as a + // condition slot each contributes exactly 1, and the cyclomatic + // `WhenClause` arm lifts the decision count to match. + // + // `none` is the unguarded control and the anchor: a two-arm switch + // over the same relational patterns carrying no guard at all, so + // every member here is exactly one guard away from it. Without that + // row a fix that scored the *arms* differently would satisfy the + // four guarded rows alike. Its arms run `> 2` before `> 0` because + // the reverse order is a C# compile error (CS8510, the later arm + // subsumed) — the guarded members escape that only because a guard + // can fail, which is what makes their repeated `> 0` legal. + // + // `paren` is not a fourth spelling for its own sake — it is the only + // member reaching the `WhenClause` seed added to + // `csharp_inspect_container`, since `when_clause` wraps a + // parenthesised guard in a real `parenthesized_expression` rather + // than the anonymous parens `catch_filter_clause` uses. + #[test] + fn csharp_switch_arm_guard_scores_one_condition_however_spelled() { + let src = "class A { + static bool IsEven(int x) { return true; } + int tok(int x) => x switch { > 0 when x % 2 == 0 => 1, > 0 => 2, _ => 3 }; + int cmp(int x) => x switch { > 0 when x > 2 => 1, > 0 => 2, _ => 3 }; + int call(int x) => x switch { > 0 when IsEven(x) => 1, > 0 => 2, _ => 3 }; + int paren(int x) => x switch { > 0 when (IsEven(x)) => 1, > 0 => 2, _ => 3 }; + int none(int x) => x switch { > 2 => 1, > 0 => 2, _ => 3 }; + }"; + assert_csharp_fixture_spells( + src, + &[ + (Csharp::WhenClause as u16, 4, "`when` guards"), + (Csharp::EQEQ as u16, 1, "`tok`'s `==`"), + ( + Csharp::ParenthesizedExpression as u16, 1, - "one arm; the guard's relational pattern scores nothing" - ); - assert_eq!(g.metrics.cyclomatic.cyclomatic(), 2); - }, + "`paren`'s parenthesised guard", + ), + // By the call's *argument list*, which carries no + // numeric-suffix alias, rather than by + // `invocation_expression`, which has three + // (`InvocationExpression` / `2` / `3`) and would pin this + // anchor to whichever one the pinned grammar happens to + // emit. `call` and `paren` hold the only two argument + // lists in the fixture — `IsEven(int x)` is a + // `parameter_list`. + (Csharp::ArgumentList as u16, 2, "the `IsEven` calls"), + ], ); + check_func_space::(src, "foo.cs", |space| { + assert_csharp_members_score( + &space.spaces[0], + &[ + ("IsEven", 0, 1), + ("tok", 3, 4), + ("cmp", 3, 4), + ("call", 3, 4), + ("paren", 3, 4), + ("none", 2, 3), + ], + ); + }); + } + + // The statement `switch` reaches the same `when_clause` kind through + // a different parent — `switch_section` rather than + // `switch_expression_arm` — which makes it an independent path in + // the sense of `.claude/rules/grammar-dispatch.md` §11: the arms + // themselves are counted by two different owners (a `Case` token + // here, the `SwitchExpressionArm` node there), so a guard rule + // written against one shape could be dead for the other and every + // expression-form fixture would still read correct. + #[test] + fn csharp_statement_switch_section_guard_counts() { + let src = "class A { + static bool IsEven(int x) { return true; } + int guarded(int x) { + switch (x) { + case > 0 when IsEven(x): return 1; + case > 0: return 2; + default: return 3; + } + } + int plain(int x) { + switch (x) { + case > 2: return 1; + case > 0: return 2; + default: return 3; + } + } + }"; + assert_csharp_fixture_spells( + src, + &[ + (Csharp::WhenClause as u16, 1, "the `case … when` guard"), + (Csharp::SwitchSection as u16, 6, "switch sections"), + ( + Csharp::SwitchExpressionArm as u16, + 0, + "expression arms (this fixture is the statement form)", + ), + ], + ); + check_func_space::(src, "foo.cs", |space| { + assert_csharp_members_score( + &space.spaces[0], + &[("IsEven", 0, 1), ("guarded", 3, 4), ("plain", 2, 3)], + ); + }); + } + + // The sibling #1422's issue body does not mention, measured to have + // the identical defect: `catch (E e) when (…)` is a second guard + // spelling on its own `catch_filter_clause` kind, and it scored + // conditions 3 / 2 / 2 for the comparison / call spellings and the + // unfiltered control while `cyclomatic()` sat at 2 for all three. + // + // ABC runs one above `cyclomatic() - 1` on every row, guarded and + // unguarded alike, and that offset is not this fix's: C# ABC counts + // the `Try` token as a condition (Fitzpatrick Rule 5, the same + // family as `else`) where cyclomatic counts only the `catch`. The + // guard moves both tiers by exactly one, which is what `none` + // anchors — the offset is a constant here, not something a guard + // introduces. + // + // `paren` carries the *double* parenthesis on purpose. The grammar + // spells `catch_filter_clause`'s own parentheses as anonymous tokens + // the way `if_statement` does, so `when (IsEven(x))` hands the slot a + // bare `invocation_expression` and never reaches the + // `CatchFilterClause` seed in `csharp_inspect_container`; only a + // second pair of parentheses produces a `parenthesized_expression` + // there. + #[test] + fn csharp_catch_filter_guard_scores_one_condition_however_spelled() { + let src = "class A { + static bool IsEven(int x) { return true; } + int tok(int x) { try { return 1; } catch (System.Exception e) when (x % 2 == 0) { return 2; } } + int cmp(int x) { try { return 1; } catch (System.Exception e) when (x > 0) { return 2; } } + int call(int x) { try { return 1; } catch (System.Exception e) when (IsEven(x)) { return 2; } } + int paren(int x) { try { return 1; } catch (System.Exception e) when ((IsEven(x))) { return 2; } } + int none(int x) { try { return 1; } catch (System.Exception e) { return 2; } } + }"; + assert_csharp_fixture_spells( + src, + &[ + ( + Csharp::CatchFilterClause as u16, + 4, + "`catch … when` filters", + ), + (Csharp::CatchClause as u16, 5, "catch clauses"), + (Csharp::WhenClause as u16, 0, "switch guards (none here)"), + ( + Csharp::ParenthesizedExpression as u16, + 1, + "`paren`'s inner parentheses", + ), + ], + ); + check_func_space::(src, "foo.cs", |space| { + assert_csharp_members_score( + &space.spaces[0], + &[ + ("IsEven", 0, 1), + ("tok", 3, 3), + ("cmp", 3, 3), + ("call", 3, 3), + ("paren", 3, 3), + ("none", 2, 2), + ], + ); + }); + } + + // A guarded discard is where the two rules meet. `_ when g =>` is + // already exempt from the `default:` exclusion — + // `csharp_switch_expression_arm_is_bare_discard` looks for a + // `when_clause` for exactly this reason — so after #1422 it scores + // twice: once as an arm that is no longer unconditional, once for + // the guard that makes it conditional. `bare` is the control that + // keeps the exclusion itself pinned: drop the guard and the arm goes + // back to costing nothing. + #[test] + fn csharp_guarded_discard_arm_scores_arm_and_guard() { + let src = "class A { + static bool IsEven(int x) { return true; } + int guarded(int x) => x switch { > 0 => 1, _ when IsEven(x) => 2, _ => 3 }; + int bare(int x) => x switch { > 0 => 1, _ => 3 }; + }"; + assert_csharp_fixture_spells( + src, + &[ + (Csharp::WhenClause as u16, 1, "the guarded discard"), + (Csharp::Discard as u16, 3, "discard patterns"), + ], + ); + check_func_space::(src, "foo.cs", |space| { + assert_csharp_members_score( + &space.spaces[0], + &[("IsEven", 0, 1), ("guarded", 3, 4), ("bare", 1, 2)], + ); + }); + } + + // A comment inside the guard must not cost it its condition. + // tree-sitter `extra`s are *named* nodes, so `comment` can be the + // first named child of either guard clause — and a slot that read + // only the first named child scored `when /*c*/ IsEven(x)` as 1 + // where `when IsEven(x)` scores 2, silently reinstating the very + // spelling-dependence #1422 removes and breaking the §8 parity the + // fix establishes. Found in review of #1422 before it shipped. + // + // This is the C# instance of the class #1181 fixed for the ternary, + // where a comment between a token and its operand shifted every + // positional read. The two commented members are asserted equal to + // their uncommented twins rather than to a literal, so the pair + // cannot drift apart silently; the literals are pinned by + // `csharp_switch_arm_guard_scores_one_condition_however_spelled` and + // its catch-filter sibling. + #[test] + fn csharp_guard_keeps_its_condition_across_a_comment() { + let src = "class A { + static bool IsEven(int x) => true; + int plain(int x) => x switch { > 0 when IsEven(x) => 1, _ => 0 }; + int cmt(int x) => x switch { > 0 when /*c*/ IsEven(x) => 1, _ => 0 }; + int cplain(int x) { try { return 1; } catch (System.Exception e) when (IsEven(x)) { return 2; } } + int ccmt(int x) { try { return 1; } catch (System.Exception e) when (/*c*/ IsEven(x)) { return 2; } } + }"; + assert_csharp_fixture_spells( + src, + &[ + (Csharp::Comment as u16, 2, "the in-guard comments"), + (Csharp::WhenClause as u16, 2, "switch guards"), + (Csharp::CatchFilterClause as u16, 2, "catch filters"), + ], + ); + check_func_space::(src, "foo.cs", |space| { + assert_csharp_members_score( + &space.spaces[0], + &[ + ("IsEven", 0, 1), + ("plain", 2, 3), + ("cmt", 2, 3), + ("cplain", 3, 3), + ("ccmt", 3, 3), + ], + ); + }); + } + + // Why the guard is a condition *slot* and not a flat +1: a compound + // guard keeps its sub-structure. `when a > 1 && b < 2` scores the + // two comparisons through the token arm and nothing extra from the + // slot, because a `binary_expression` is not one of + // `csharp_bool_terminal_kinds!()` — so it reads one above `single`'s + // lone comparison rather than collapsing to the same number, and + // cyclomatic agrees because it counts the `&&`. + #[test] + fn csharp_compound_guard_keeps_its_sub_structure() { + let src = "class A { + int compound(int a, int b) => a switch { > 0 when a > 1 && b < 2 => 1, _ => 0 }; + int single(int a, int b) => a switch { > 0 when a > 1 => 1, _ => 0 }; + }"; + assert_csharp_fixture_spells( + src, + &[ + (Csharp::WhenClause as u16, 2, "`when` guards"), + (Csharp::AMPAMP as u16, 1, "`compound`'s `&&`"), + (Csharp::LT as u16, 1, "`compound`'s `<`"), + ], + ); + check_func_space::(src, "foo.cs", |space| { + assert_csharp_members_score(&space.spaces[0], &[("compound", 3, 4), ("single", 2, 3)]); + }); } // #1275: tree-sitter-c-sharp spells `int?`, `where T : class?`, the diff --git a/src/metrics/abc/csharp.rs b/src/metrics/abc/csharp.rs index bbbe13ff..71f1ad8a 100644 --- a/src/metrics/abc/csharp.rs +++ b/src/metrics/abc/csharp.rs @@ -22,10 +22,14 @@ fn csharp_inspect_container(container_node: &Node, parent: &Node, conditions: &m let mut node_kind = node.kind_id().into(); // Seed the boolean-context flag from the parent: known-boolean - // contexts (loop / if / binary expression) imply the contained - // expression evaluates as a condition. + // contexts (loop / if / guard / binary expression) imply the + // contained expression evaluates as a condition. The two guard + // clauses joined this list with #1422 — a `when` guard is a boolean + // slot exactly as an `if` condition is, so `when (b)` and + // `catch (E e) when ((b))` count their parenthesised operand. let mut has_boolean_content = match parent.kind_id().into() { - BinaryExpression | IfStatement | WhileStatement | DoStatement | ForStatement => true, + BinaryExpression | IfStatement | WhileStatement | DoStatement | ForStatement + | WhenClause | CatchFilterClause => true, ConditionalExpression => parent .child_by_field_name("condition") .is_some_and(|condition| condition.id() == node.id()), @@ -486,6 +490,52 @@ fn csharp_walk_for_conditions<'a>( csharp_count_condition(&condition, node, conds); } } + // C#'s two guard spellings, each modelled as a condition slot + // exactly like the `if` / `while` / `do` slots above (#1422). + // Before this, a guard scored whatever operator happened to sit + // inside it: `when x % 2 == 0` and `when x > 2` counted one via + // the comparison-token arm while `when IsEven(x)` counted zero, + // so three semantically identical guards produced two different + // numbers. As a slot every spelling contributes exactly one — + // a call / `is` test / bare identifier through + // `csharp_bool_terminal_kinds!()`, a comparison through the + // token arm that already owned it — and a compound guard + // (`when a > 1 && b < 2`) keeps its sub-structure rather than + // collapsing to one. + // + // Suppressing the guard's operator instead would have reached + // the same internal agreement one count *below* C#'s own + // cyclomatic decision count, which #1422 fixes upward in + // `src/metrics/cyclomatic/csharp.rs`. + // + // By role, not index (`.claude/rules/grammar-dispatch.md` §3): + // the two clauses carry their guard as the only *expression* + // child but disagree on where it sits, because + // `catch_filter_clause` spells its parentheses as anonymous + // tokens (`when`, `(`, expr, `)`) the way `if_statement` does, + // while `when_clause` has none (`when`, expr). Neither exposes + // a field for the slot. + // + // Every named child, not the first: tree-sitter `extra`s are + // named nodes and may precede the expression, so `when /*c*/ g` + // hands a `comment` to a first-child read and silently restores + // the spelling-dependence this fix removes. C#'s extras at this + // pin are `comment` plus ten `preproc_*` kinds — none of them a + // `csharp_bool_terminal_kinds!()` member, and none a paren or + // `!`-prefix wrapper — so passing them through the slot adds + // nothing and the loop cannot double count a clause that holds + // one expression by construction. + // + // The sibling `if` / `while` / `do` slots read a fixed child + // index and so still lose their condition to a leading comment + // (`if (/*c*/ g)` scores 0). That is the same class of bug and + // predates this arm; it is left to its own change rather than + // widened into here. + WhenClause | CatchFilterClause => { + for guard in node.children().filter(Node::is_named) { + csharp_count_condition(&guard, node, conds); + } + } // `return value;` — child(1) is the value expression. ReturnStatement => csharp_inspect_child(node, 1, conds), // Child 2: declarator / assignment RHS, lambda body diff --git a/src/metrics/cyclomatic.rs b/src/metrics/cyclomatic.rs index c2b2c350..2c2b34de 100644 --- a/src/metrics/cyclomatic.rs +++ b/src/metrics/cyclomatic.rs @@ -1924,6 +1924,15 @@ mod tests { /// bare wildcard — the `when` guard adds a non-trivial decision — /// so the arm still contributes one standard decision, mirroring /// Rust's `_ if g` rule. + /// + /// Since #1422 the guard contributes a second decision of its own + /// (`WhenClause`), so the arm is worth 2: the pattern can match and + /// the guard still fail, which is a distinct way to fall through to + /// the next arm. The two counts have different owners — + /// `csharp_switch_expression_arm_is_bare_discard` keeps the arm out + /// of the `default:` exclusion, the `WhenClause` arm scores the + /// guard — so this test would still fail if #282's exclusion were + /// reintroduced, at sum 5 / max 3 against the asserted 6 / 4. #[test] fn csharp_switch_expression_guarded_discard_still_counts() { check_metrics::( @@ -1938,10 +1947,10 @@ mod tests { "foo.cs", |metric| { // expected: unit(1) + class(1) + fn(base 1 + 1 explicit + - // 1 guarded discard; bare `_ =>` skipped) = 5, - // max 3. - assert_eq!(metric.cyclomatic.cyclomatic_sum(), 5); - assert_eq!(metric.cyclomatic.cyclomatic_max(), 3); + // 1 guarded discard + 1 `when` guard (#1422); + // bare `_ =>` skipped) = 6, max 4. + assert_eq!(metric.cyclomatic.cyclomatic_sum(), 6); + assert_eq!(metric.cyclomatic.cyclomatic_max(), 4); }, ); } @@ -1979,6 +1988,11 @@ mod tests { /// so the arm still contributes one standard decision. Exercises /// the `DeclarationPattern` arm of `classify_pattern` combined /// with the post-pattern `WhenClause` sweep. + /// + /// As with `csharp_switch_expression_guarded_discard_still_counts`, + /// the guard has scored a decision of its own since #1422, so the + /// arm is worth 2. Reintroducing #303's exclusion would read sum 5 / + /// max 3 against the asserted 6 / 4. #[test] fn csharp_switch_expression_guarded_var_underscore_still_counts() { check_metrics::( @@ -1993,10 +2007,10 @@ mod tests { "foo.cs", |metric| { // expected: unit(1) + class(1) + fn(base 1 + 1 explicit `1` + - // 1 guarded `var _`; bare `_ =>` skipped) = 5, - // max 3. - assert_eq!(metric.cyclomatic.cyclomatic_sum(), 5); - assert_eq!(metric.cyclomatic.cyclomatic_max(), 3); + // 1 guarded `var _` + 1 `when` guard (#1422); + // bare `_ =>` skipped) = 6, max 4. + assert_eq!(metric.cyclomatic.cyclomatic_sum(), 6); + assert_eq!(metric.cyclomatic.cyclomatic_max(), 4); }, ); } diff --git a/src/metrics/cyclomatic/csharp.rs b/src/metrics/cyclomatic/csharp.rs index 20537232..cd798979 100644 --- a/src/metrics/cyclomatic/csharp.rs +++ b/src/metrics/cyclomatic/csharp.rs @@ -39,12 +39,39 @@ impl Cyclomatic for CsharpCode { stats.cyclomatic_modified += 1.; } // Both standard and modified. + // + // `WhenClause` and `CatchFilterClause` are C#'s two guard + // spellings, and each is a decision the enclosing construct + // does not already pay for (#1422): a guarded arm fails two + // ways — the pattern does not match, or it matches and the + // guard is false — while contributing one decision, and + // `catch (E e) when (c)` tests the filter after the type. + // Rust already counts a match guard — `rust.rs`'s `If` arm + // catches the guard's own `if` token — so C# was the + // outlier, not the convention. + // + // The clause *nodes*, not the `when` keyword they share: + // `When` (119) is also a `_reserved_identifier` at this pin, + // so `int when = 1;` emits it under an `identifier` and the + // token would score a decision per mention of the variable + // (`bca dump`, not inferred). One `when_clause` serves both + // the `switch_expression_arm` and `switch_section` guards; + // neither kind carries a numeric-suffix alias. + // + // A guarded discard (`_ when g => …`) therefore scores two, + // and that is the intended reading: the guard is already why + // `csharp_switch_expression_arm_is_bare_discard` keeps the + // arm out of the `default:` exclusion, so the arm is no + // longer an unconditional fallthrough and the guard that + // makes it conditional is its own decision. IfStatement | ForStatement | ForeachStatement | WhileStatement | DoStatement | CatchClause + | WhenClause + | CatchFilterClause | ConditionalExpression | ConditionalAccessExpression | AMPAMP diff --git a/tests/repositories/big-code-analysis-output b/tests/repositories/big-code-analysis-output index 98c48d2c..cf1ef381 160000 --- a/tests/repositories/big-code-analysis-output +++ b/tests/repositories/big-code-analysis-output @@ -1 +1 @@ -Subproject commit 98c48d2cb9e1209edc6204ec868eef1e068d0e2f +Subproject commit cf1ef3810d9e759acd346ae98fde3f630ace0c29 From 97b0641220a11f0c30d723143da20dd5fc0a5082 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 13 Sep 2026 20:24:20 -0700 Subject: [PATCH 04/11] fix(abc/kotlin): score subject-less when arms once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A subject-less `when` arm's condition is an ordinary boolean expression, compiled exactly as an `if` predicate, so the comparison operator inside it is already an ABC condition through the token arms. The `WhenEntry` arm added a blanket one on top, and `when { x > 5 -> 1; x < 0 -> 2; else -> 0 }` reported 4 conditions against a cyclomatic decision count of 2. All six comparison spellings were affected, not the two `<` / `>` the issue names: `>=` and `==` reach the count through a different arm and scored 2 against 1. A subject-less entry now goes through the same condition slot `if` uses, and a subject-ful one keeps its per-entry count — its arm lists a pattern, not an independent boolean expression, so the implicit `subject == pattern` is a decision nothing in the source spells and the entry must pay for it. The operator is not suppressed instead, which would have collapsed a compound condition `when { a > 1 && b < 2 -> … }` to one count, a decision below Kotlin's own. Deferring to the slot exposed a second gap it would otherwise have regressed: `is_expression` and `in_expression` are the two relational forms the grammar spells as their own production rather than as a `binary_expression`, so no comparison-token arm sees them and nothing counted them at all. Listing both in `kotlin_bool_terminal_kinds!()` reaches the predicate slot, the `&&` / `||` walker and the paren / `!` unwrapper together, and fixes `if (a is String)` and `if (a in 1..2)`, which scored zero against a decision count of one. `WhenEntry` joins the walker's boolean-context set so a parenthesised condition counts; a negated one already did. Two divergences from `conditions == cyclomatic() - 1` remain and are correct. A comparison nested inside a comparison is two ABC conditions and one branch, so `when (x) { y > 5 -> … }` reads 2 against 1 exactly as `if (x == (y > 5))` always has. And a multi-alternative entry, `when { x > 5, y < 0 -> … }`, reads 2 against 1 because cyclomatic scores the whole entry as one decision; it was 3 before, and the residue is cyclomatic's model rather than a double count here. There is no Kotlin corpus — one unsnapshotted `.kt` file repo-wide — so every number above was measured on hand fixtures against a live build, before and after, and each changed path was verified by perturbation. Fixes #1421 --- big-code-analysis-ast/src/macros/kind_sets.rs | 13 + src/metrics/abc.rs | 331 +++++++++++++++++- src/metrics/abc/kotlin.rs | 107 +++++- 3 files changed, 423 insertions(+), 28 deletions(-) diff --git a/big-code-analysis-ast/src/macros/kind_sets.rs b/big-code-analysis-ast/src/macros/kind_sets.rs index 187fc95c..953674d8 100644 --- a/big-code-analysis-ast/src/macros/kind_sets.rs +++ b/big-code-analysis-ast/src/macros/kind_sets.rs @@ -574,6 +574,17 @@ macro_rules! tsx_bool_terminal_kinds { // (`arr[0]`), and `this_expression`. Comparison operands (`x > 0`) are // themselves `binary_expression` nodes, so they are absent from this set // and contribute nothing — matching the paper's "only unary conditions". +// +// `is_expression` (`a is String`, `a !is String`) and `in_expression` +// (`a in 1..2`, `a !in 1..2`) are the two relational forms the grammar +// spells as their own production rather than as a `binary_expression`, +// so the comparison-token arms never see them and nothing else in the +// Kotlin impl counts them. They are terminal for every consumer of this +// set — neither carries a nested chain link, and neither's own operator +// token (`is` / `!is` / `in` / `!in`) is counted anywhere — so listing +// them here scores each exactly once, as Fitzpatrick Rule 5 scores any +// other relational operator. Before #1421 `if (a is String)` scored +// zero conditions against a cyclomatic decision of one. #[macro_export] #[doc(hidden)] macro_rules! kotlin_bool_terminal_kinds { @@ -583,6 +594,8 @@ macro_rules! kotlin_bool_terminal_kinds { | $crate::Kotlin::NavigationExpression | $crate::Kotlin::IndexExpression | $crate::Kotlin::ThisExpression + | $crate::Kotlin::IsExpression + | $crate::Kotlin::InExpression }; } diff --git a/src/metrics/abc.rs b/src/metrics/abc.rs index 8ceaca20..a831764d 100644 --- a/src/metrics/abc.rs +++ b/src/metrics/abc.rs @@ -538,8 +538,11 @@ mod tests { // control one lower — which is the comparison the test exists to // make. Both tiers per member, because a guard is a decision in each // and a fix that moved only one of them would satisfy neither claim - // on its own. - fn assert_csharp_members_score(container: &crate::FuncSpace, expected: &[(&str, u64, u64)]) { + // on its own, and because a member may legitimately sit *above* its + // decision count — an `else` arm, or a comparison nested inside + // another comparison, is an ABC condition with no cyclomatic + // decision behind it (#1421). + fn assert_members_score(container: &crate::FuncSpace, expected: &[(&str, u64, u64)]) { assert_eq!( container.spaces.len(), expected.len(), @@ -579,12 +582,8 @@ mod tests { // of the control. Measured — with presence-only anchoring, rewriting // `if (x is > 0)` to `if (x > 0)` in one method of five failed // nothing. - fn assert_csharp_fixture_spells(src: &str, kinds: &[(u16, usize, &str)]) { - let parser = CsharpParser::new( - src.as_bytes().to_vec(), - std::path::Path::new("foo.cs"), - None, - ); + fn assert_fixture_spells(src: &str, path: &str, kinds: &[(u16, usize, &str)]) { + let parser = P::new(src.as_bytes().to_vec(), std::path::Path::new(path), None); for (kind, want, spelling) in kinds { let found = parser .root() @@ -598,6 +597,13 @@ mod tests { } } + // The C# binding of the anchor above. Sixteen callers pass a `foo.cs` + // fixture, so the parser and path are fixed here rather than repeated + // at each one. + fn assert_csharp_fixture_spells(src: &str, kinds: &[(u16, usize, &str)]) { + assert_fixture_spells::(src, "foo.cs", kinds); + } + /// Regression for #227: a `Stats::default()` that never sees an /// observation must not leak the `f64::MAX` sentinel for /// `assignments_min`, `branches_min`, or `conditions_min`. All @@ -4402,7 +4408,7 @@ mod tests { ], ); check_func_space::(src, "foo.cs", |space| { - assert_csharp_members_score( + assert_members_score( &space.spaces[0], &[ ("IsEven", 0, 1), @@ -4456,7 +4462,7 @@ mod tests { ], ); check_func_space::(src, "foo.cs", |space| { - assert_csharp_members_score( + assert_members_score( &space.spaces[0], &[("IsEven", 0, 1), ("guarded", 3, 4), ("plain", 2, 3)], ); @@ -4512,7 +4518,7 @@ mod tests { ], ); check_func_space::(src, "foo.cs", |space| { - assert_csharp_members_score( + assert_members_score( &space.spaces[0], &[ ("IsEven", 0, 1), @@ -4549,7 +4555,7 @@ mod tests { ], ); check_func_space::(src, "foo.cs", |space| { - assert_csharp_members_score( + assert_members_score( &space.spaces[0], &[("IsEven", 0, 1), ("guarded", 3, 4), ("bare", 1, 2)], ); @@ -4589,7 +4595,7 @@ mod tests { ], ); check_func_space::(src, "foo.cs", |space| { - assert_csharp_members_score( + assert_members_score( &space.spaces[0], &[ ("IsEven", 0, 1), @@ -4624,7 +4630,7 @@ mod tests { ], ); check_func_space::(src, "foo.cs", |space| { - assert_csharp_members_score(&space.spaces[0], &[("compound", 3, 4), ("single", 2, 3)]); + assert_members_score(&space.spaces[0], &[("compound", 3, 4), ("single", 2, 3)]); }); } @@ -5448,6 +5454,303 @@ function f(int $a, int $b): int { ); } + // The Kotlin binding of the two assertions every #1421 fixture needs, + // coupled on purpose. A per-member score table is worth only as much + // as the anchor proving the fixture still spells what it claims to + // (`.claude/rules/testing.md`, "Perturb the fixture as well as the + // production line"), and the `when_subject` count is the anchor no + // score can stand in for: a `when` that grew a subject scores its + // entries the pre-#1421 way with every other row still satisfied. + // Each fixture is one class, so the members are `spaces[0]`'s. + fn assert_kotlin_class_members( + src: &str, + kinds: &[(u16, usize, &str)], + expected: &[(&str, u64, u64)], + ) { + assert_fixture_spells::(src, "foo.kt", kinds); + check_func_space::(src, "foo.kt", |space| { + assert_members_score(&space.spaces[0], expected); + }); + } + + // #1421's headline. A subject-less `when` arm's condition is an + // ordinary boolean expression, so its comparison operator is already + // an ABC condition through the token arms; the `WhenEntry` arm added + // a second one on top, and `two` reported 4 against a decision count + // of 2. + // + // `bare` is the control that isolates the defect rather than merely + // observing it: same arm count, same entry shape, no comparison + // operator anywhere — so it scored the right 2 before the fix and + // after it. A fix that changed both members alike would be attacking + // the entry count, which was never wrong. + // + // `ge` and `eq` exist because the issue named `<` and `>` only. All + // six comparison spellings share the defect: `>=` and `==` reach + // `stats.conditions` through the `LTEQ | GTEQ | EQEQ | …` arm rather + // than the parent-gated `LT | GT` arm, and both scored 2 against a + // decision count of 1. + // + // `iff` is the member that legitimately sits *above* its decision + // count, and it is here so the table cannot be read as "§8 parity + // everywhere". ABC counts the `else` (Fitzpatrick Rule 5) and + // cyclomatic does not, so 2 conditions against 1 decision is correct + // and unchanged by this fix. + // + // The `when_subject` anchor is the load-bearing one: the whole test + // turns on these `when`s having no subject, and nothing in the + // measured numbers would notice a fixture that grew one — a + // subject-ful `when (x) { … }` scores its entries the old way and + // `two` would read 4 again with every other row still satisfied. + #[test] + fn kotlin_subjectless_when_arm_counts_its_condition_once() { + let src = "class K { + fun f(): Boolean = true + fun two(x: Int): Int = when { x > 5 -> 1; x < 0 -> 2; else -> 0 } + fun bare(x: Boolean, y: Boolean): Int = when { x -> 1; y -> 2; else -> 0 } + fun ge(x: Int): Int = when { x >= 5 -> 1; else -> 0 } + fun eq(x: Int): Int = when { x == 5 -> 1; else -> 0 } + fun andd(a: Int, b: Int): Int = when { a > 1 && b < 2 -> 1; else -> 0 } + fun call(): Int = when { f() -> 1; else -> 0 } + fun iff(x: Int): Int = if (x > 5) 1 else 0 + }"; + assert_kotlin_class_members( + src, + &[ + (Kotlin::WhenSubject as u16, 0, "`when` subjects"), + (Kotlin::WhenEntry as u16, 14, "`when` entries"), + ( + Kotlin::BinaryExpression as u16, + 8, + "comparison / chain expressions", + ), + (Kotlin::AMPAMP as u16, 1, "`andd`'s `&&`"), + ], + &[ + // (member, abc.conditions, cyclomatic) + ("f", 0, 1), + // Was 4: two entries, two comparisons, counted twice. + ("two", 2, 3), + // The no-comparison control — 2 before and after. + ("bare", 2, 3), + // Was 2 each: the `GTEQ` / `EQEQ` spellings. + ("ge", 1, 2), + ("eq", 1, 2), + // Was 3. A compound condition keeps both of its + // comparisons — suppressing the operators instead + // would have collapsed this one to 1. + ("andd", 2, 3), + // The condition slot scores a call directly, so a + // spelling with no operator in it still reads 1. + ("call", 1, 2), + // Above its decision count on purpose: ABC counts + // the `else`, cyclomatic does not. + ("iff", 2, 2), + ], + ); + } + + // The other half of #1421: a subject-ful entry must not move. Its + // condition is a *pattern* matched against the subject, not an + // independent boolean expression, so the implicit `subject == + // pattern` is the decision and the entry itself pays for it — + // `range_test`, `type_test` and a bare constant carry no token the + // comparison arms would count. + // + // `cmp` is the interesting row and the reason `nested` sits beside + // it. `when (x) { y > 5 -> … }` is legal when `x` is `Boolean`, and + // it reads 2 conditions against 1 decision — the entry's implicit + // equality plus the `>` inside the operand. That is not the #1421 + // double-count reappearing: `nested` spells the same thing as an + // `if` (`x == (y > 5)`) and ABC has always scored it 2 against the + // same 1 decision. A comparison nested inside a comparison is two + // comparisons; only one of them is a branch. + #[test] + fn kotlin_subjectful_when_arms_keep_the_per_entry_count() { + let src = "class K { + fun inn(a: Int): Int = when (a) { in 1..2 -> 1; else -> 0 } + fun notin(a: Int): Int = when (a) { !in 1..2 -> 1; else -> 0 } + fun iss(a: Any): Int = when (a) { is String -> 1; else -> 0 } + fun konst(a: Int): Int = when (a) { 1 -> 10; 2 -> 20; else -> 0 } + fun cmp(x: Boolean, y: Int): Int = when (x) { y > 5 -> 1; else -> 0 } + fun nested(x: Boolean, y: Int): Int { if (x == (y > 5)) { return 1 }; return 0 } + }"; + assert_kotlin_class_members( + src, + &[ + (Kotlin::WhenSubject as u16, 5, "`when` subjects"), + (Kotlin::WhenEntry as u16, 11, "`when` entries"), + (Kotlin::RangeTest as u16, 2, "`in` / `!in` patterns"), + (Kotlin::TypeTest as u16, 1, "the `is` pattern"), + ( + Kotlin::BinaryExpression as u16, + 3, + "`cmp`'s and `nested`'s comparisons", + ), + ], + &[ + ("inn", 1, 2), + ("notin", 1, 2), + ("iss", 1, 2), + ("konst", 2, 3), + // Above its decision count, like `nested` below it + // and for the same reason. + ("cmp", 2, 2), + ("nested", 2, 2), + ], + ); + } + + // A subject-less condition reaches the slot through the same + // `kotlin_inspect_container` unwrapping an `if` predicate does, which + // is what separates this design from suppressing the entry's + // operator. `notted` and `paren` unwrap to a bare terminal and score + // through the slot; `notCmp` and `parenCmp` unwrap to a + // `binary_expression`, score nothing from the slot, and take their + // one condition from the `>` the token arm already owned. Both pairs + // land on 1, which no blanket `+1` can produce for the second pair — + // they read 2 before the fix. + // + // `paren` is the member that reaches the `WhenEntry` seed added to + // `kotlin_inspect_container`'s boolean-context set; without it the + // parenthesised operand is not in a slot the walker calls boolean and + // `paren` drops to 0. `notted` cannot stand in for it — a `!` + // operator proves boolean content on its own. + #[test] + fn kotlin_subjectless_when_condition_wrappers_count_once() { + let src = "class K { + fun notted(a: Boolean): Int = when { !a -> 1; else -> 0 } + fun paren(a: Boolean): Int = when { (a) -> 1; else -> 0 } + fun notCmp(a: Int): Int = when { !(a > 1) -> 1; else -> 0 } + fun parenCmp(a: Int): Int = when { (a > 1) -> 1; else -> 0 } + fun nest(x: Int, y: Int): Int = + when { x > 1 -> when { y > 2 -> 1; else -> 2 }; else -> 0 } + fun blockBody(x: Int): Int { when { x > 5 -> { return 1 } }; return 0 } + }"; + assert_kotlin_class_members( + src, + &[ + // Paired, not bare: "no subjects" alone is satisfied by a + // fixture that lost its `when`s altogether. + (Kotlin::WhenSubject as u16, 0, "`when` subjects"), + (Kotlin::WhenEntry as u16, 13, "`when` entries"), + (Kotlin::UnaryExpression as u16, 2, "the two `!` operands"), + ( + Kotlin::ParenthesizedExpression as u16, + 3, + "the three parenthesised operands", + ), + (Kotlin::BinaryExpression as u16, 5, "comparisons"), + ], + &[ + ("notted", 1, 2), + ("paren", 1, 2), + // Both were 2: the entry's blanket count plus the `>`. + ("notCmp", 1, 2), + ("parenCmp", 1, 2), + // Was 4 — one entry and one comparison per `when`, + // each counted twice. + ("nest", 2, 3), + // The `when`-as-statement shape: a `block` body and no + // `else` arm at all. Reading the condition from the + // `condition` field rather than by position is what keeps + // the block out of the slot; it was 2 before the fix. + ("blockBody", 1, 2), + ], + ); + } + + // The one place this design deliberately does not reach parity, put + // under test so the choice cannot drift unremarked. A `when` entry + // may list alternatives (`when { a, b -> … }`, an implicit `or`), and + // the `condition` field is `multiple` — but cyclomatic scores the + // whole entry as one decision, so `bareAlts` is at parity only + // because the slot reads the *first* alternative and stops. Routing + // every alternative through the slot would push it to 2. + // + // `alts` is the row that cannot be brought to parity from the ABC + // side at all: the comparison-token arms count `>` and `<` wherever + // they appear, so it reads 2 against 1 decision no matter what the + // entry slot does. It was 3 before #1421 and the remaining gap is + // cyclomatic's single count per multi-alternative entry, not a + // double-count here. + // + // `subjAlts` is the subject-ful control, unchanged and at parity: its + // alternatives are constants carrying no token to count. + #[test] + fn kotlin_subjectless_when_multi_alternative_entry() { + let src = "class K { + fun alts(x: Int, y: Int): Int = when { x > 5, y < 0 -> 1; else -> 0 } + fun bareAlts(x: Boolean, y: Boolean): Int = when { x, y -> 1; else -> 0 } + fun subjAlts(a: Int): Int = when (a) { 1, 2 -> 10; else -> 0 } + }"; + assert_kotlin_class_members( + src, + &[ + (Kotlin::WhenSubject as u16, 1, "`subjAlts`'s subject"), + (Kotlin::WhenEntry as u16, 6, "`when` entries"), + ( + Kotlin::BinaryExpression as u16, + 2, + "`alts`'s two comparisons", + ), + ], + &[ + // Above its decision count: cyclomatic scores the entry + // once, the token arms score both comparisons. Was 3. + ("alts", 2, 2), + ("bareAlts", 1, 2), + ("subjAlts", 1, 2), + ], + ); + } + + // `is_expression` and `in_expression` are the two relational forms + // tree-sitter-kotlin-ng spells as their own production rather than as + // a `binary_expression`, so no comparison-token arm sees them and + // nothing counted them: `if (a is String)` scored 0 against a + // decision count of 1, and the same test in a subject-less `when` + // scored 1 only because the entry's blanket `+1` happened to cover + // it. Removing that blanket without listing these two in + // `kotlin_bool_terminal_kinds!()` would have regressed `whenIs` and + // `whenIn` to 0, and `isAnd` — where the `is` test is one operand of + // a `&&` chain and so reaches the walker rather than the slot — from + // 2 to 1. + // + // The `if` members are here because the fix is in the shared + // terminal-kind set, not in the `when` arm: they are the call sites + // that prove it reaches the `if` / `while` predicate slot too. + #[test] + fn kotlin_is_and_in_expressions_are_unary_conditions() { + let src = "class K { + fun ifIs(a: Any): Int { if (a is String) { return 1 }; return 0 } + fun ifIn(a: Int): Int { if (a in 1..2) { return 1 }; return 0 } + fun whenIs(a: Any): Int = when { a is String -> 1; else -> 0 } + fun whenIn(a: Int): Int = when { a in 1..2 -> 1; else -> 0 } + fun isAnd(a: Any, b: Boolean): Int = when { a is String && b -> 1; else -> 0 } + }"; + assert_kotlin_class_members( + src, + &[ + // Paired, not bare — see the wrappers test above. + (Kotlin::WhenSubject as u16, 0, "`when` subjects"), + (Kotlin::WhenEntry as u16, 6, "`when` entries"), + (Kotlin::IsExpression as u16, 3, "`is` tests"), + (Kotlin::InExpression as u16, 2, "`in` tests"), + (Kotlin::AMPAMP as u16, 1, "`isAnd`'s `&&`"), + ], + &[ + // Both were 0 — the predicate slot saw a kind it did + // not classify and the token arms saw no operator. + ("ifIs", 1, 2), + ("ifIn", 1, 2), + ("whenIs", 1, 2), + ("whenIn", 1, 2), + ("isAnd", 2, 3), + ], + ); + } + #[test] fn kotlin_catch_block_counts() { check_metrics::( diff --git a/src/metrics/abc/kotlin.rs b/src/metrics/abc/kotlin.rs index eeed3af6..6d2cfbb1 100644 --- a/src/metrics/abc/kotlin.rs +++ b/src/metrics/abc/kotlin.rs @@ -62,10 +62,18 @@ fn kotlin_inspect_container(container_node: &Node, parent: &Node, conditions: &m // A parenthesised / negated operand only contributes when it sits in // a boolean-evaluating slot. The chain wrapper (`binary_expression`) // and the control-flow headers all qualify; a `!`-operator anywhere - // also proves the operand is boolean (`if (!x)`). + // also proves the operand is boolean (`if (!x)`). `WhenEntry` joined + // the list with #1421: a subject-less `when` arm's condition is an + // ordinary boolean expression, so `when { (a) -> … }` is the same + // slot as `if ((a))`. let mut has_boolean_content = matches!( parent.kind_id().into(), - BinaryExpression | IfExpression | WhileStatement | DoWhileStatement | ForStatement + BinaryExpression + | IfExpression + | WhileStatement + | DoWhileStatement + | ForStatement + | WhenEntry ); loop { @@ -96,12 +104,14 @@ fn kotlin_inspect_container(container_node: &Node, parent: &Node, conditions: &m } } -// Counts each non-comparison operand of a Kotlin `&&` / `||` chain once. -// Mirrors `java_count_unary_conditions`: comparison operands are nested -// `binary_expression` nodes (absent from `kotlin_bool_terminal_kinds!()`) -// and so contribute nothing, while bare identifiers / calls / member -// accesses each add one. Inner chain links and `!` / paren wrappers are -// routed through `kotlin_inspect_container`. +// Counts each operand of a Kotlin `&&` / `||` chain that no token arm +// already owns. Mirrors `java_count_unary_conditions`: a comparison +// operand is a nested `binary_expression`, absent from +// `kotlin_bool_terminal_kinds!()`, and contributes nothing here because +// its operator token was counted directly; bare identifiers / calls / +// member accesses, and the `is` / `in` tests no token arm sees, each add +// one. Inner chain links and `!` / paren wrappers are routed through +// `kotlin_inspect_container`. fn kotlin_count_unary_conditions(list_node: &Node, conditions: &mut f64) { use Kotlin::*; @@ -133,10 +143,13 @@ fn kotlin_count_unary_conditions(list_node: &Node, conditions: &mut f64) { // mirroring `ruby_count_condition` / `rust_count_condition`. // tree-sitter-kotlin-ng exposes the predicate via the `condition` field on // `if_expression`, `while_statement`, and `do_while_statement`, so the -// field lookup is position-independent across all three forms. A bare -// terminal (`if (flag)`) counts directly; a comparison or boolean chain -// (`if (a == b)`, `if (a && b)`) is a nested `binary_expression` already -// counted by the comparison-token and `&&`/`||` walker arms, so it adds +// field lookup is position-independent across all three forms. Since +// #1421 it also scores a subject-less `when` entry's condition, which is +// an `if` predicate in all but spelling. A bare terminal (`if (flag)`) +// counts directly, as does an `is` / `in` test that no token arm sees; a +// comparison or boolean chain (`if (a == b)`, `if (a && b)`) is a nested +// `binary_expression` already counted by the comparison-token and +// `&&`/`||` walker arms, so it adds // nothing here. A parenthesised or negated predicate (`if ((flag))`, // `if (!flag)`) is unwrapped by `kotlin_inspect_container`. Without this // arm, idiomatic Kotlin bare predicates reported 0 ABC conditions while @@ -152,6 +165,68 @@ fn kotlin_count_condition(condition: &Node, parent: &Node, conditions: &mut f64) } } +// Returns true when the `when_expression` enclosing `entry` carries a +// subject — `when (x) { … }` rather than `when { … }`. The distinction +// decides how the entry's condition is scored (#1421): a subject-ful +// entry lists a *pattern* compared against the subject, a subject-less +// one lists an ordinary boolean expression. +// +// A `when_entry`'s parent IS the `when_expression`, and the grammar +// exposes no field for the subject (`when_expression` has an empty +// `fields` map in node-types.json), so the membership test is a scan of +// the parent's children rather than a `child_by_field_name`. Scanning +// rather than reading a fixed index also keeps a leading `extra` — a +// comment between `when` and `(x)` — from displacing the answer. +fn kotlin_enclosing_when_has_subject<'a>(entry: &Node<'a>, ancestors: Ancestors<'a, '_>) -> bool { + ancestors.parent(entry).is_some_and(|when_expression| { + when_expression + .children() + .any(|child| child.kind_id() == Kotlin::WhenSubject) + }) +} + +// Scores one non-`else` `when` entry. Both `when` shapes contribute the +// same single decision that cyclomatic counts, but they pay for it in +// different places, and #1421 is the case where ABC charged for it twice. +// +// A subject-ful entry (`when (x) { in 1..2 -> … }`) lists a *pattern*, +// not an independent boolean expression: the decision is the implicit +// `x == pattern`, which nothing in the source spells, so the entry +// itself contributes the condition. A `range_test`, `type_test` or bare +// constant carries no token the comparison arms would count, and #1383's +// logic applies unchanged. +// +// A subject-less entry (`when { x > 5 -> … }`) lists an ordinary boolean +// expression — textually identical to an `if` predicate, and compiled as +// one — so it goes through the same slot `IfExpression` uses. Before +// this the entry added a blanket 1 *on top of* the comparison operator +// its condition already scored through the token arms, so +// `when { x > 5 -> 1; x < 0 -> 2; else -> 0 }` reported 4 conditions +// against a cyclomatic decision count of 2. The slot scores a bare +// terminal (`when { x -> … }`, `when { a is String -> … }`) directly and +// leaves a comparison or `&&` / `||` chain to the arms that already own +// it. Suppressing the operator instead would have been the wrong half to +// give way: a compound condition `when { a > 1 && b < 2 -> … }` needs +// both its comparisons, and suppression collapses it to one. +// +// The `condition` field is `multiple` — `when { a, b -> … }` lists +// alternatives — and only the first reaches the slot. That is +// deliberate: cyclomatic scores a multi-alternative entry as one +// decision, so routing every alternative through the slot would move +// `when { x, y -> … }` off that count rather than onto it. Alternatives +// after the first are still seen by the token arms. +fn kotlin_count_when_entry<'a>( + entry: &Node<'a>, + ancestors: Ancestors<'a, '_>, + conditions: &mut f64, +) { + if kotlin_enclosing_when_has_subject(entry, ancestors) { + *conditions += 1.; + } else if let Some(condition) = entry.child_by_field_name("condition") { + kotlin_count_condition(&condition, entry, conditions); + } +} + impl Abc for KotlinCode { fn compute<'a>( node: &Node<'a>, @@ -252,9 +327,13 @@ impl Abc for KotlinCode { // fallback arm, which is the analogue of C-family `default:` // and Rust's wildcard `_ =>`. Cyclomatic already excludes it // (`WhenEntry if !kotlin_when_entry_is_else`); ABC must track - // the same decision count (issue #456, lesson 11). + // the same decision count (issue #456, lesson 11). Sharing + // that predicate is what keeps the two metrics from drifting. + // `kotlin_count_when_entry` decides where the decision is + // paid for — the entry, or the operators of its condition + // (#1421). WhenEntry if !crate::metrics::cyclomatic::kotlin_when_entry_is_else(node) => { - stats.conditions += 1.; + kotlin_count_when_entry(node, ancestors, &mut stats.conditions); } // `else` is a keyword token used in both `if_expression`'s // else-clause and `when`'s `else ->` entry. Only count it From 46e77ad57147453a61569b34bfe0df1cf9a0ef27 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 13 Sep 2026 20:43:04 -0700 Subject: [PATCH 05/11] fix(abc): count enum constants with arguments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An enum constant carrying constructor arguments — `A(1)` — is an object construction under Fitzpatrick's "function invocation or object construction" rule, but scored zero in every JVM-family language. #1279 and #1384 settled the two sibling delegation forms (`super(…)` / `this(…)` and `class Sub : Base(1, 2)`) and left this one behind, because both looked at superclass delegation only. Adds one arm per language, gated on the entry's argument-list child so the childless spelling does not regress (grammar-dispatch §6): Kotlin `enum_entry` + `value_arguments`, Java `enum_constant` + `argument_list`, Groovy `enum_constant` + `argument_list`. Java's gate names `argument_list` specifically, so an annotated constant cannot satisfy it via the distinct `annotation_argument_list` production. The rationale, including the counter-argument that an enum constant is a declaration rather than a call site a reader navigates to, is recorded on each arm. Java and Groovy's branch functions move from `if matches!` to the `match` + `is_branch` shape #1406 established in the C# sibling; neither needed widening to take `Ancestors`, since the gate reads a child. Cost is one branch per argument-carrying constant, and +2 cyclomatic on `KotlinCode::compute` (18 -> 20), whose baseline entry is refreshed here. The baseline regeneration also lowers `src/spaces/compute.rs:: metrics_inner`'s `halstead.effort` from 119147.75 to 116715.61. That entry is stale on `main` and is not this change's doing: it is a decrease, so it never made the gate red and no earlier commit was forced to refresh it. It rides along because the baseline is regenerated wholesale rather than edited by hand. Measured with `bca dump` / `bca metrics` against a live build: Kotlin `enum class E(val v: Int) { A(1), B(2), C(3) }` 0 -> 3, Java `enum E { A(1), B, C(2); }` 0 -> 2, Groovy `enum E { A(1), B, C(2) }` 0 -> 2; every bare and annotated-bare spelling stays at 0, and `A(f())` scores 2 rather than 1. No corpus or snapshot movement. Fixes #1407 --- .bca-baseline.toml | 2 +- src/metrics/abc.rs | 127 ++++++++++++++++++++++++++++++++++++++ src/metrics/abc/groovy.rs | 37 ++++++++++- src/metrics/abc/java.rs | 39 ++++++++++-- src/metrics/abc/kotlin.rs | 29 +++++++++ 5 files changed, 224 insertions(+), 10 deletions(-) diff --git a/.bca-baseline.toml b/.bca-baseline.toml index 86835f9e..d62bd2ea 100644 --- a/.bca-baseline.toml +++ b/.bca-baseline.toml @@ -915,7 +915,7 @@ value = 15.0 path = "src/metrics/abc/kotlin.rs" qualified = "KotlinCode::compute" metric = "cyclomatic" -value = 18.0 +value = 20.0 [[entry]] path = "src/metrics/abc/objc.rs" diff --git a/src/metrics/abc.rs b/src/metrics/abc.rs index a831764d..0bd84f95 100644 --- a/src/metrics/abc.rs +++ b/src/metrics/abc.rs @@ -2097,6 +2097,133 @@ mod tests { ); } + // The three tests below are one fix seen from three grammars (#1407): + // an enum constant carrying constructor arguments is an object + // construction and scored zero in every JVM-family language, which + // #1279 and #1384 left behind because both looked at superclass + // delegation only. + // + // Each fixture pairs an argument-carrying constant with a bare one in + // the *same* enum, because that is the only shape that shows the gate + // working: a fixture of only `A(1)` scores the same whether the arm is + // gated on the argument list or matches every constant + // (`.claude/rules/grammar-dispatch.md` §6). Each also carries a + // constant whose argument is itself a call, which must score 2 rather + // than 1 (§5). + // + // `branches_sum()` alone cannot notice the bare constant being trimmed + // out — it contributes to no axis — so every fixture is anchored on a + // node census that fails by name when a spelling is edited away + // (`.claude/rules/testing.md`, "Perturb the fixture as well as the + // production line"). The two are coupled in one helper for the same + // reason `assert_kotlin_class_members` couples its pair: a branch + // total added without the census reads as coverage and is not. + + fn assert_enum_branches( + src: &str, + path: &str, + kinds: &[(u16, usize, &str)], + branches: u64, + ) { + assert_fixture_spells::

(src, path, kinds); + check_func_space::(src, path, |space| { + assert_eq!(space.metrics.abc.branches_sum(), branches); + }); + } + + #[test] + fn java_enum_constant_with_arguments_is_a_branch() { + // `@SuppressWarnings("x") B` is the discriminating case for the + // gate's *kind*, not just its presence: the annotation carries an + // argument list of its own, so an arm gated on "has some argument + // list anywhere below" would score it. It cannot satisfy this one + // twice over — `annotation_argument_list` is a distinct kind id + // from `argument_list`, and it hangs off the constant's `modifiers` + // child rather than the constant itself. Both verified with `bca + // dump`. + // + // expected: 4 branches — `A(1)`, `K(2)`, `D(…)`, and the `f()` + // inside `D`'s argument list. `B` scores 0. + let src = "class Outer { + static int f() { return 1; } + enum E { + A(1), + @SuppressWarnings(\"x\") B, + K(2), + D(f()); + E(int v) {} + } + }"; + assert_enum_branches::( + src, + "foo.java", + &[ + (Java::EnumConstant as u16, 4, "enum constants"), + (Java::ArgumentList as u16, 4, "argument lists"), + ( + Java::AnnotationArgumentList as u16, + 1, + "`@SuppressWarnings`'s argument list", + ), + (Java::MethodInvocation as u16, 1, "`D`'s nested `f()` call"), + ], + 4, + ); + } + + #[test] + fn kotlin_enum_entry_with_arguments_is_a_branch() { + // Kotlin needs a defaulted primary-constructor parameter to spell + // both cases in one enum: `enum class E(val v: Int)` obliges every + // entry to pass an argument, so `= 0` is what lets the bare `B` + // sit beside `A(1)`. `enum class P` covers the other shape the gate + // has to leave alone — an enum with no constructor at all, whose + // entries can never carry an argument list. + // + // expected: 3 branches — `A(1)`, `C(…)`, and the `g()` inside `C`'s + // argument list. `B`, `X` and `Y` score 0. + let src = "fun g(): Int = 1 + enum class E(val v: Int = 0) { A(1), B, C(g()) } + enum class P { X, Y }"; + assert_enum_branches::( + src, + "foo.kt", + &[ + (Kotlin::EnumEntry as u16, 5, "enum entries"), + (Kotlin::ValueArguments as u16, 3, "argument lists"), + (Kotlin::CallExpression as u16, 1, "`C`'s nested `g()` call"), + ], + 3, + ); + } + + #[test] + fn groovy_enum_constant_with_arguments_is_a_branch() { + // Groovy's enum has no annotated-constant case to cover: the + // dekobon grammar cannot parse `@Deprecated A(1)` inside an enum + // body and recovers into `ERROR` nodes, emitting no `enum_constant` + // at all. The remaining two cases are the ones that matter. + // + // expected: 3 branches — `A(1)`, `D(…)`, and the `Outer.f()` inside + // `D`'s argument list. `B` scores 0. + let src = "class Outer { static int f() { 1 } } + enum E { A(1), B, D(Outer.f()) }"; + assert_enum_branches::( + src, + "foo.groovy", + &[ + (Groovy::EnumConstant as u16, 3, "enum constants"), + (Groovy::ArgumentList as u16, 3, "argument lists"), + ( + Groovy::MethodInvocation as u16, + 1, + "`D`'s nested `Outer.f()` call", + ), + ], + 3, + ); + } + #[test] fn groovy_no_abc() { // Comment-only file has no executable code → all-zero ABC. diff --git a/src/metrics/abc/groovy.rs b/src/metrics/abc/groovy.rs index f419816b..74d51b0c 100644 --- a/src/metrics/abc/groovy.rs +++ b/src/metrics/abc/groovy.rs @@ -162,11 +162,42 @@ fn groovy_count_token_assignment<'a>( fn groovy_count_token_branch(node: &Node, stats: &mut Stats) -> bool { use Groovy::*; - if matches!(node.kind_id().into(), MethodInvocation | CommandChain | New) { + let is_branch = match node.kind_id().into() { + MethodInvocation | CommandChain | New => true, + // An enum constant carrying constructor arguments — `A(1)` in + // `enum E { A(1), B, C(2) }` — invokes the enum's constructor, so + // it is an object construction under Fitzpatrick's "function + // invocation or object construction" rule. It scored zero until + // #1407, which is the inconsistent position once #1279 decided + // Java's `super(…)` / `this(…)` — already counted here through + // `MethodInvocation` — and #1384 decided Kotlin's `class Sub : + // Base(1, 2)`: all three are a constructor call the source spells + // out. + // + // The counter-argument is that an enum constant is a declaration, + // not a call site a reader navigates to. It loses because the same + // is true of a superclass delegation, and because the arguments + // still have to be understood as a constructor's, which is the + // effort ABC is measuring. + // + // The child gate is what makes this a §6 narrowing rather than a + // new node: a bare `B` is `enum_constant > identifier` with no + // `argument_list` child and must stay at zero. Verified with `bca + // dump`: the dekobon Groovy grammar gives the constant an + // `identifier` plus an optional `argument_list`. Java's annotated + // spelling has no analogue to guard against here — this grammar + // cannot parse `@Deprecated A(1)` inside an enum body at all and + // recovers into `ERROR` nodes, so no `enum_constant` is produced. + // An argument that is itself a call (`A(f())`) scores 2: + // `argument_list` is not a branch node, so the inner + // `method_invocation` is the only other node counted (§5). + EnumConstant => node.is_child(ArgumentList as u16), + _ => false, + }; + if is_branch { stats.branches += 1.; - return true; } - false + is_branch } // The `default` arm of a `switch` is excluded (issue #469): it is the diff --git a/src/metrics/abc/java.rs b/src/metrics/abc/java.rs index 4bb69017..70570aad 100644 --- a/src/metrics/abc/java.rs +++ b/src/metrics/abc/java.rs @@ -196,14 +196,41 @@ fn java_count_token_assignment<'a>( // are separate nodes and still count on their own. fn java_count_token_branch(node: &Node, stats: &mut Stats) -> bool { use Java::*; - if matches!( - node.kind_id().into(), - MethodInvocation | New | ExplicitConstructorInvocation - ) { + let is_branch = match node.kind_id().into() { + MethodInvocation | New | ExplicitConstructorInvocation => true, + // An enum constant carrying constructor arguments — `A(1)` in + // `enum E { A(1), B, C(2); }` — invokes the enum's constructor, so + // it is an object construction under Fitzpatrick's "function + // invocation or object construction" rule. It scored zero until + // #1407, which is the inconsistent position once #1279 decided + // `super(…)` / `this(…)` above: both are a constructor call the + // source spells out, and Kotlin's `class Sub : Base(1, 2)` was + // settled the same way in #1384. + // + // The counter-argument is that an enum constant is a declaration, + // not a call site a reader navigates to. It loses because the same + // is true of the delegation forms already counted, and because the + // arguments still have to be understood as a constructor's, which + // is the effort ABC is measuring. + // + // The child gate is what makes this a §6 narrowing rather than a + // new node: a bare `B` is `enum_constant > identifier` with no + // `argument_list` child and must stay at zero. The gate is + // specifically on `argument_list`, so an annotated constant + // (`@Deprecated A` / `@Foo(1) A`) cannot satisfy it — an + // annotation's arguments are a distinct `annotation_argument_list` + // production, and they hang off the constant's `modifiers` child + // rather than off the constant directly. Both verified with `bca + // dump`. An argument that is itself a call (`A(f())`) scores 2: + // `argument_list` is not a branch node, so the inner + // `method_invocation` is the only other node counted (§5). + EnumConstant => node.is_child(ArgumentList as u16), + _ => false, + }; + if is_branch { stats.branches += 1.; - return true; } - false + is_branch } // Counts condition tokens: comparison operators, control-flow keywords, diff --git a/src/metrics/abc/kotlin.rs b/src/metrics/abc/kotlin.rs index 6d2cfbb1..50b8745c 100644 --- a/src/metrics/abc/kotlin.rs +++ b/src/metrics/abc/kotlin.rs @@ -293,6 +293,35 @@ impl Abc for KotlinCode { { stats.branches += 1.; } + // An enum entry carrying constructor arguments — `A(1)` in + // `enum class E(val v: Int) { A(1), B(2) }` — invokes the enum's + // constructor, so it is an object construction under + // Fitzpatrick's "function invocation or object construction" + // rule. It scored zero until #1407, which is the inconsistent + // position once #1279 and #1384 decided the two sibling + // delegation forms (`: this(…)` and `class Sub : Base(1, 2)`): + // all three are a constructor call the source spells out. + // + // The counter-argument is that an enum entry is a declaration, + // not a call site a reader navigates to. It loses because the + // same is true of `class Sub : Base(1, 2)` — also a declaration + // — and because the arguments still have to be understood as a + // constructor's, which is the effort ABC is measuring. + // + // The child gate is what makes this a §6 narrowing rather than + // a new node: a bare `B` is `enum_entry > identifier` with no + // `value_arguments` child and must stay at zero, as must every + // entry of an enum with no constructor at all (`enum class E { + // A, B }`). Verified with `bca dump`. `value_arguments` is the + // only argument-list production the entry can carry, and a + // Kotlin `annotation` is a `constructor_invocation` above, not + // an entry, so nothing else satisfies the gate. An argument + // that is itself a call (`A(f())`) scores 2: `value_arguments` + // is not a branch node, so the inner `call_expression` is the + // only other node counted (no double count, §5). + EnumEntry if node.is_child(ValueArguments as u16) => { + stats.branches += 1.; + } // Conditions: comparison operators, identity equality, // ternary-elvis (`?:`), `as?` safe-cast, and the arms of // control-flow constructs (`else`, `catch`, `when` entries). From 333e258b841dc3fbb9871243ab3d200db493e360 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Mon, 14 Sep 2026 13:53:10 -0700 Subject: [PATCH 06/11] chore(self-scan): refresh a stale baseline entry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `src/spaces/compute.rs::metrics_inner` recorded a `halstead.effort` of 119147.75 against a live measurement of 116715.61. Nothing on this branch touches `src/spaces/`; the entry went stale on `main`, in `d4479cdf` (PR #1446's review-findings commit), which changed `compute.rs` after the last baseline refresh. It went unnoticed because the value *fell*. The baseline filter suppresses a violation only while the live measurement stays at or below the recorded value, so a decrease never reddens the gate and nothing forces a refresh — the recorded number simply stops describing the tree, and the gate runs looser than intended until someone regenerates for an unrelated reason. Here that reason was #1407 refreshing a Kotlin entry, since the write target regenerates the file wholesale. Recorded separately from that fix so the metric commits carry only their own baseline movement. The staleness mechanism is filed as #1465. --- .bca-baseline.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.bca-baseline.toml b/.bca-baseline.toml index d62bd2ea..8e184ad8 100644 --- a/.bca-baseline.toml +++ b/.bca-baseline.toml @@ -1203,7 +1203,7 @@ value = 7.0 path = "src/spaces/compute.rs" qualified = "metrics_inner" metric = "halstead.effort" -value = 119147.7514530567 +value = 116715.6053686016 [[entry]] path = "src/spaces/compute.rs" From e98fbafa17fd3fb109b55acbf2d9eaddc677611f Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 13 Sep 2026 21:12:07 -0700 Subject: [PATCH 07/11] fix(abc): count numeric operands in six languages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PHP, Groovy and the name-keyed C / C++ / Mozcpp / Objective-C terminal-bool sets named no numeric literal kind, so a truthy number in a boolean operand slot scored no condition. `$a && 1` scored one against `$a && $b`'s two, and in the C family the gap was visible *within* one language: `if (true)` scored one condition and `if (1)` none, because `"true"` was in the set and `number_literal` was not. The operand kind fell through the terminal gate, the walker recursed one level, and the recursion then failed the list-kind gate. Nothing warned. Adds `Php::Integer` / `Php::Float`, `Groovy::NumberLiteral`, and the name-keyed `"number_literal"` / `"char_literal"` arms. `char_literal` is included because a character literal has integral type (`int` in C, `char` in C++) and is truthy for the same reason a number is; omitting it would reproduce the same asymmetry one kind narrower. `Php::Float2` is deliberately excluded: it renders to the same `"float"` string but is the type keyword, not a value. C++'s `user_defined_literal` and Objective-C's `version_number` are excluded for the reasons recorded on the set. Every spelling was re-derived with `bca dump` per language rather than by an alias sweep, which is what #1379's first cut got wrong: PHP folds every radix, separator and exponent into `integer` / `float`; Groovy and all four C-family grammars emit one `number_literal` each. #1379 deferred these six on the stated grounds that all three sets had integration-corpus files. That held only for the C family. PHP's six corpus files carry no truthy numeric operand, and no corpus carries a `.groovy` file at all — the 16 `.gradle` files in DeepSpeech sit outside every test glob. Both stale rationales are corrected rather than dropped. Costs seven DeepSpeech C/C++ snapshots, all `abc.conditions` and its derived `magnitude` / `value` / per-space min-max-average and no other metric, every move traceable to a `while (1)` or `do { … } while (0)`. Fixes #1410 --- big-code-analysis-ast/src/macros/kind_sets.rs | 103 +++++++++++++----- src/metrics/abc.rs | 61 ++++++++++- tests/repositories/big-code-analysis-output | 2 +- 3 files changed, 134 insertions(+), 32 deletions(-) diff --git a/big-code-analysis-ast/src/macros/kind_sets.rs b/big-code-analysis-ast/src/macros/kind_sets.rs index 953674d8..457c7a4c 100644 --- a/big-code-analysis-ast/src/macros/kind_sets.rs +++ b/big-code-analysis-ast/src/macros/kind_sets.rs @@ -142,10 +142,23 @@ macro_rules! java_bool_terminal_kinds { // the dekobon Groovy grammar has no `await` or `array_access` // analogues, so those collapse out of the C# set. // -// FIXME(#1410): Groovy truth makes every non-zero number truthy, so this -// set is missing the numeric literal kinds — `a && 1` scores one -// condition where `a && b` scores two. Deferred out of #1379 because the -// integration corpora carry Groovy files and the fix moves snapshots. +// Groovy truth makes every non-zero number truthy, so `NumberLiteral` +// is a unary condition here for the same reason Python's `Integer` / +// `Float` are. Without it `a && 1` scored one condition against +// `a && b`'s two (#1410). +// +// One kind covers every spelling: the dekobon grammar consolidated the +// per-radix rules the prior one split (`getter/groovy.rs`), so `0x1f`, +// `0b101`, `017`, `1_000`, `1e3` and every type suffix (`1L`, `1.5f`, +// `1G`, `1I`, `1.5d`, `1.2g`) all lex as `number_literal` — verified by +// `bca dump`, not read off the grammar, because #1379's misses were +// sibling rules under a hidden choice rather than aliases of one rule. +// +// #1379 deferred this on the stated grounds that "the integration +// corpora carry Groovy files"; they do not. No corpus carries a +// `.groovy` file at all, and DeepSpeech's 16 `.gradle` files are +// outside every test glob (`tests/corpus/deepspeech_test.rs` globs +// `*.cc` / `*.cpp` / `*.h` / `*.hh`), so this fix moves no snapshot. #[macro_export] #[doc(hidden)] macro_rules! groovy_bool_terminal_kinds { @@ -154,6 +167,7 @@ macro_rules! groovy_bool_terminal_kinds { | $crate::Groovy::CommandChain | $crate::Groovy::Identifier | $crate::Groovy::BooleanLiteral + | $crate::Groovy::NumberLiteral | $crate::Groovy::FieldAccess | $crate::Groovy::CastExpression | $crate::Groovy::ParenthesizedTypeCast @@ -210,13 +224,33 @@ macro_rules! go_bool_terminal_kinds { }; } -// FIXME(#1410): C and C++ are integer-truthy, so this set is missing the -// numeric literal kinds — `if (1)` scores no condition where `if (true)` -// scores one, within the same language. Name-keyed, so C, C++, Mozcpp and -// Objective-C are all affected, which makes this the largest of the three -// sets #1379 left behind (see `perl_bool_terminal_kinds!` below for the -// measurement). Deferred out of #1379 because the DeepSpeech corpus is -// C/C++ and the fix moves snapshots. +// The C family is integer-truthy — `if (1)`, `while (1)`, +// `do { … } while (0)` and `a && 1` are all legal and idiomatic — so +// `number_literal` and `char_literal` belong here for the same reason +// `"true"` does. Omitting them made the gap visible *within* one +// language: `if (true)` scored one condition and `if (1)` none (#1410). +// +// One `number_literal` arm covers every spelling in all four grammars. +// `0x1f`, `017`, `0b101`, `1u`, `1L`, `1ULL`, `1.5f`, `1e3` and C++'s +// `1'000` digit separator all lex to it, verified by `bca dump` per +// language rather than read off the grammar — #1379's misses were +// sibling rules under a hidden choice, which an alias sweep cannot see. +// +// `char_literal` is here because a character literal has integral type +// (`int` in C, `char` in C++) and is contextually convertible to bool +// exactly as a number is; leaving it out would reproduce the same +// within-language asymmetry one kind narrower. It is also the correct +// half of the wrapper/leaf pair (grammar-dispatch §5): an operand slot +// always holds the `char_literal`, never its inner `character` / +// `escape_sequence` child, so the wrapper is the only node reachable +// here and there is nothing to double-count. +// +// Two neighbouring kinds are deliberately absent. C++'s +// `user_defined_literal` (`1.0_km`) wraps a `number_literal` but +// evaluates to whatever `operator""` returns, which need not be +// numeric or contextually boolean. Objective-C's `version_number` is +// the `@available(iOS 13.0, *)` token, not a literal, and never +// occupies an operand slot. #[macro_export] #[doc(hidden)] macro_rules! cpp_bool_terminal_kinds { @@ -240,6 +274,8 @@ macro_rules! cpp_bool_terminal_kinds { "identifier" | "true" | "false" + | "number_literal" + | "char_literal" | "call_expression" | "message_expression" | "field_expression" @@ -249,10 +285,23 @@ macro_rules! cpp_bool_terminal_kinds { }; } -// FIXME(#1410): PHP treats every non-zero number as truthy, so this set -// is missing the numeric literal kinds — `$a && 1` scores one condition -// where `$a && $b` scores two. Deferred out of #1379 because the -// integration corpora carry PHP files and the fix moves snapshots. +// PHP treats every non-zero number as truthy, so `Integer` and `Float` +// are unary conditions here for the same reason Python's `Integer` / +// `Float` are. Naming neither scored `$a && 1` one condition against +// `$a && $b`'s two, and `if ($a && 1.0)` one against two (#1410). +// +// The unit to check is the supertype, not the alias list (#1379), and +// PHP has neither to miss: every radix prefix, `_` separator and +// exponent folds into one of those two kinds — `0x1f`, `0b101`, `017`, +// `0o17` and `1_000` all lex as `integer`, and `1e3`, `1.5e10`, `.5` +// and `1.` as `float` (verified by `bca dump`). +// +// `Float2` (52) is **not** a third numeric kind despite rendering to +// the same `"float"` string: it is the `float` *type* keyword of a +// parameter type or a `(float)` cast, which `Getter::get_op_type` +// groups with `Int` / `Bool` / `String2` rather than with the `Integer` +// / `Float` value operands (`getter/php.rs`). Listing it would be the +// `Number2` mistake `typescript_bool_terminal_kinds!` records below. #[macro_export] #[doc(hidden)] macro_rules! php_bool_terminal_kinds { @@ -271,6 +320,8 @@ macro_rules! php_bool_terminal_kinds { | $crate::Php::Name2 | $crate::Php::VariableName | $crate::Php::Boolean + | $crate::Php::Integer + | $crate::Php::Float | $crate::Php::FunctionCallExpression | $crate::Php::MemberCallExpression | $crate::Php::ScopedCallExpression @@ -348,20 +399,16 @@ macro_rules! python_bool_terminal_kinds { // those five, so there is nothing to count. // // That rationale does **not** extend to the C family, which an earlier -// revision of this comment wrongly grouped with them. C and C++ are -// integer-truthy — `if (1)`, `while (1)`, `do { … } while (0)` and -// `a && 1` are all legal and idiomatic — so they carry the same gap PHP -// and Groovy do. `cpp_bool_terminal_kinds!` is name-keyed and shared by -// C, C++, Mozcpp and Objective-C, so all four are affected, and the -// omission is visible *within* one language: `if (true)` scores one -// condition and `if (1)` scores none, because `"true"` is in the set -// and `number_literal` is not. +// revision of this comment wrongly grouped with them: C and C++ are +// integer-truthy, so they carried the same gap PHP and Groovy did. +// #1410 closed all six — PHP, Groovy and the name-keyed C, C++, Mozcpp +// and Objective-C set — and each carries its own rationale above. // -// PHP, Groovy and the four C-family languages are all tracked in #1410. -// They are deferred rather than deliberate, and for a scheduling reason -// only: each has integration-corpus files (the DeepSpeech `native_client` -// snapshots are C/C++), so fixing them moves snapshots and wants its own -// measurement pass. +// #1379 deferred those six on the grounds that all three sets had +// integration-corpus files. That held only for the C family (the +// DeepSpeech `native_client` tree, seven snapshots): PHP's corpus files +// carry no truthy numeric operand, and no corpus carries a `.groovy` +// file at all. #[macro_export] #[doc(hidden)] macro_rules! perl_bool_terminal_kinds { diff --git a/src/metrics/abc.rs b/src/metrics/abc.rs index 0bd84f95..729575b5 100644 --- a/src/metrics/abc.rs +++ b/src/metrics/abc.rs @@ -12922,6 +12922,12 @@ mod keyword_negation_parity { /// scored one. Python had both kinds since #772 and is the control the /// other three were brought level with. /// +/// #1410 closed the six sets #1379 deferred: PHP, Groovy, and the +/// name-keyed C / C++ / Mozcpp / Objective-C set, where the omission +/// was visible *within* one language — `if (true)` scored one condition +/// and `if (1)` none, because `"true"` was in the set and +/// `number_literal` was not. +/// /// The headline claim of each case is a *comparison*: a numeric operand /// must score exactly what an identifier operand scores in the same slot. /// That is what discriminates the defect — with the kind missing from the @@ -12958,7 +12964,13 @@ mod keyword_negation_parity { feature = "perl", feature = "python", feature = "lua", - feature = "javascript" + feature = "javascript", + feature = "php", + feature = "groovy", + feature = "c", + feature = "cpp", + feature = "mozcpp", + feature = "objc" ))] mod numeric_bool_operands { use crate::test_support::metrics_verbatim; @@ -12970,8 +12982,10 @@ mod numeric_bool_operands { type Slot = (&'static str, u64, u64); /// A language's two slots, its identifier baseline operand, the - /// numeric operands that must score the same, and how many of those - /// there should be. + /// truthy literal operands that must score the same, and how many of + /// those there should be. Every row but the C family's lists numeric + /// literals only; that one adds `char_literal`, which is a second + /// integral-literal kind rather than a second numeric spelling. /// /// The count is not bookkeeping. `for_each_case` counts *languages*, /// so trimming a row's operand list back to `&["1"]` — which is @@ -13015,6 +13029,20 @@ mod numeric_bool_operands { /// lexes as `integer`, and the `octal` kind is unreachable. /// - Elixir: `integer` / `float` / `char`. `?a` is the codepoint 97, /// a numeric literal wearing a sigil. + /// - PHP: `integer` / `float`. Every radix prefix, `_` separator and + /// exponent folds into those two, and the `float` *type* keyword + /// `Float2` is a third id rendering to the same name that must + /// stay out of the set (#1410). + /// - Groovy: one consolidated `number_literal`, type suffixes + /// (`1L`, `1.5f`, `1G`) included. + /// - C, C++, Mozcpp and Objective-C: one consolidated + /// `number_literal`, plus `char_literal` — not a numeric spelling + /// but a second integral-literal *kind*, since `'c'` has type + /// `int` in C and `char` in C++ and is truthy for the same reason. + /// All four share one name-keyed set and so run the same fixture, + /// which is the only coverage `Mozcpp` can have: it owns no file + /// extension, and `metrics_verbatim` is the one entry point that + /// reaches a `LANG` without going through one. /// /// Python, Lua and JavaScript were already correct and ride along as /// controls — they are what the first three were measured against, @@ -13077,6 +13105,33 @@ mod numeric_bool_operands { &["1", "1.0", "0xff"], 3, ), + LANG::Php => ( + [ + (" ( + [ + ("def f(a) {\n return a && {}\n}\n", 2, 3), + ("def f() {\n if ({}) { return 1 }\n}\n", 1, 3), + ], + "b", + &["1"], + 1, + ), + LANG::C | LANG::Cpp | LANG::Mozcpp | LANG::Objc => ( + [ + ("int f(int a) {\n return a && {};\n}\n", 2, 3), + ("int f() {\n if ({}) { return 1; }\n}\n", 1, 3), + ], + "b", + &["1", "'c'"], + 2, + ), _ => return None, }) } diff --git a/tests/repositories/big-code-analysis-output b/tests/repositories/big-code-analysis-output index cf1ef381..ee1bd859 160000 --- a/tests/repositories/big-code-analysis-output +++ b/tests/repositories/big-code-analysis-output @@ -1 +1 @@ -Subproject commit cf1ef3810d9e759acd346ae98fde3f630ace0c29 +Subproject commit ee1bd8591b0d66ec83377a522291caf4a0d8db7b From 6ed40b01665cc9c00e91396fba23370267dba3ff Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 13 Sep 2026 21:12:44 -0700 Subject: [PATCH 08/11] docs(changelog): consolidate entries from batch fix Six ABC and cyclomatic fixes landed on this branch with their changelog entries held back to avoid conflicting edits to one file. Each records its metric drift, since all six move published numbers. --- CHANGELOG.md | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9da18865..137f58bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -135,6 +135,98 @@ for historical reference. ### Fixed +- **C# comparison-operator overloads no longer score a spurious + condition** (#1420). C# overloads six comparison operators and #1297 + gated only `<` and `>`; the other four are distinct tokens that + reached a different arm, so `operator <=`, `operator >=`, + `operator ==` and `operator !=` each scored one ABC condition on their + *declaration*, where the token names the operator being defined rather + than applying it. All six now share one arm gated on a + `binary_expression` parent, which also subsumes #1383's + `relational_pattern` denial and fails closed on a grammar bump. + **Metric drift:** `abc.conditions` falls by one per comparison-operator + overload declaration in C#. + +- **C# ABC counts a primary-constructor base call** (#1406). C# 12 lets a + class, struct or record declare its constructor in the header and pass + arguments to its base there — `class Sub(int x) : Base(x)` — which + invokes the base constructor exactly as the `: base(x)` initializer + added in #1279 does, but scored nothing. It is now one branch, in both + spellings tree-sitter-c-sharp uses: the record form nests the arguments + under a `primary_constructor_base_type`, the class form hangs them + straight off the `base_list`, so one node would have covered only half + the declarations. The arm is gated on that `base_list` parent, since an + `argument_list` is otherwise the argument list of every call in the + file. A base type passing no arguments (`struct S(int x) : IBase`) is + unchanged at zero. This is the C# sibling of Kotlin's #1384. + **Metric drift:** C# `abc.branches` rises by one per type passing + arguments to its base from a primary constructor. + +- **A C# `when` guard now counts as a decision in both cyclomatic + complexity and ABC** (#1422). Cyclomatic had no arm for either guard + spelling — `when_clause` on a switch arm or `case` section, and + `catch_filter_clause` on `catch (E e) when (…)` — so a guarded arm + scored one decision where it has two ways to fail: the pattern does not + match, or it matches and the guard is false. ABC counted whatever + operator happened to sit inside the guard, so `when x > 2` scored one + condition while the equivalent `when IsEven(x)` scored none. The guard + is now a condition slot like an `if` condition, so every spelling + scores exactly one and a compound guard keeps its sub-structure. + **Metric drift:** C# `cyclomatic` (standard and modified) gains one per + guard, and `abc.conditions` gains one for any guard not already + operator-shaped. + +- **Kotlin no longer double-counts a subject-less `when` arm's comparison + operator** (#1421). `when { x > 5 -> 1; x < 0 -> 2; else -> 0 }` + reported 4 conditions against a cyclomatic decision count of 2, because + the arm added a blanket one on top of the comparison the token arms + already scored; all six comparison spellings were affected, not only + `<` and `>`. A subject-less arm now scores its condition through the + same slot an `if` predicate uses, while a subject-ful arm keeps its + per-entry count — its pattern is not an independent boolean expression, + so the implicit `subject == pattern` is a decision nothing in the + source spells. As part of the same fix, Kotlin's `is` and `in` tests + count as conditions wherever a boolean is evaluated: `if (a is String)` + and `if (a in 1..2)` previously scored zero against a decision count of + one. **Metric drift:** Kotlin `abc.conditions` falls for subject-less + `when` arms carrying a comparison, and rises for any `is` / `in` test + in a boolean slot. + +- **An enum constant carrying constructor arguments is counted as a + branch** (#1407). `A(1)` in `enum E { A(1), B, C(2); }` invokes the + enum's constructor, so it is an object construction under + Fitzpatrick's rule — the same as the `super(…)` / `this(…)` and + `class Sub : Base(1, 2)` forms counted since #1279 and #1384 — and + scored zero in Kotlin, Java and Groovy alike. Each arm is gated on the + entry's argument-list child, so a constant with no arguments (`B`), and + every constant of an enum with no constructor, stays at zero; Java's + gate names `argument_list` specifically, so an annotated constant + cannot satisfy it through the distinct `annotation_argument_list` + production. **Metric drift:** `abc.branches` rises by one per + argument-carrying enum constant in Kotlin, Java and Groovy. + +- **A truthy numeric literal in a boolean operand slot now scores a unary + condition in PHP, Groovy and the C family** (#1410). `abc.conditions` + keys on a per-language terminal-bool kind set, and six of them named no + numeric kind — the last deferrals from #1379. `$a && 1` scored one + condition against `$a && $b`'s two; `def f(a) { a && 1 }` the same in + Groovy; and in C, C++, Mozcpp and Objective-C the omission showed + *within* one language, `if (true)` scoring one condition and `if (1)` + none, because `"true"` was in the set and `number_literal` was not. + Each set now names every numeric kind its grammar emits — `integer` / + `float` for PHP, one consolidated `number_literal` for Groovy and for + each C-family grammar — verified by `bca dump` per language rather than + by an alias sweep, which is the check #1379's first cut skipped. The + C-family set also gains `char_literal`: `'c'` has integral type and is + truthy exactly as a number is. PHP's `float` *type* keyword, C++'s + `user_defined_literal` and Objective-C's `version_number` are + deliberately excluded — none is a truthy value literal. **Metric + drift:** `abc.conditions` and `abc.magnitude` rise for PHP, Groovy, C, + C++, Mozcpp and Objective-C code using a numeric (or, in the C family, + character) literal as a bare `&&` / `||` operand, an `if` / `while` / + `do`-`while` / `for` condition, or a ternary condition — `while (1)` + and `do { … } while (0)` being the common idioms. + - **Single-language feature subsets build their tests again, and CI gates them** (#1426). Five of the twenty-two single-language subsets of `-p big-code-analysis --all-targets` failed, all in From cb149e27585b315b117765005774e9e5f8bb019a Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 13 Sep 2026 21:34:23 -0700 Subject: [PATCH 09/11] fix(abc): close two gaps found reviewing the batch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both were invisible per-fix and visible only reading the branch as one diff: each fix looked correct alone while contradicting another. C# listed `IsPatternExpression` (392) and not `IsExpression` (391) in `csharp_bool_terminal_kinds!()`. They are distinct kinds, not aliases — the grammar emits the second only once a pattern is involved (`x is int y`, `x is null`) and the first for the bare type test — so two spellings of one test disagreed: `if (x is int)` scored zero conditions against a cyclomatic decision of one while `if (x is int y)` scored one. That also falsified the `when` guard fix's own claim to score one however spelled, measured `when x is int` 1 against `when IsEven(x)` 2, and it is the C# half of the gap the Kotlin subject-less `when` fix closed by adding `IsExpression | InExpression` there. Neither fix swept toward the other. Kotlin spells boolean `and` / `or` / `xor` as infix *functions*, so `a and b` parses as `infix_expression` — not a `binary_expression`, and matched by no token arm. The blanket per-entry count the subject-less `when` fix removed had been covering it, so `when { a and b -> … }` fell from one condition to zero while the `if (a and b)` it is supposed to agree with still scored one. That contradiction is the whole premise of that fix inverted, which is what made it diagnosable. `InfixExpression` joins `kotlin_bool_terminal_kinds!()` for the same reason `CallExpression` is there: `a and b` is `a.and(b)`. Neither arm double-counts (§5): no arm counts the `is` token, and the infix operator is an identifier the walker never reaches. Removing either entry fails exactly its own test. The book's C# row said a relational pattern in a `when` guard "scores 0, one below the equivalent `x > 0`". The guard became a condition slot earlier on this branch, which falsified it; corrected here, along with two missing rows in the ABC per-language deviations table. No corpus snapshot moves — no C# corpus file uses a bare `is` type test, and there is no Kotlin corpus. --- CHANGELOG.md | 29 ++- big-code-analysis-ast/src/macros/kind_sets.rs | 29 ++- big-code-analysis-book/src/metrics.md | 6 +- src/metrics/abc.rs | 168 ++++++++++++++++-- src/metrics/abc/csharp.rs | 12 +- src/metrics/abc/kotlin.rs | 13 ++ src/metrics/cyclomatic/csharp.rs | 7 + 7 files changed, 237 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 137f58bf..a3dce857 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -144,8 +144,11 @@ for historical reference. than applying it. All six now share one arm gated on a `binary_expression` parent, which also subsumes #1383's `relational_pattern` denial and fails closed on a grammar bump. - **Metric drift:** `abc.conditions` falls by one per comparison-operator - overload declaration in C#. + **Metric drift:** C# `abc.conditions` falls by one for each + `operator <=` / `>=` / `==` / `!=` declaration. `operator <` and + `operator >` already scored zero under #1297's allowlist, so a type + overloading all six — which C# requires to be declared in pairs — + falls by four, not six. - **C# ABC counts a primary-constructor base call** (#1406). C# 12 lets a class, struct or record declare its constructor in the header and pass @@ -171,10 +174,19 @@ for historical reference. operator happened to sit inside the guard, so `when x > 2` scored one condition while the equivalent `when IsEven(x)` scored none. The guard is now a condition slot like an `if` condition, so every spelling - scores exactly one and a compound guard keeps its sub-structure. + scores exactly one and a compound guard keeps its sub-structure. The + same change adds the bare type test `x is int` to C#'s terminal-bool + operand set: it is `is_expression`, a distinct kind from the + `is_pattern_expression` of `x is int y`, and only the latter was + listed — so the two spellings of one test disagreed, `if (x is int)` + scoring zero conditions against a cyclomatic decision of one. **Metric drift:** C# `cyclomatic` (standard and modified) gains one per - guard, and `abc.conditions` gains one for any guard not already - operator-shaped. + guard; `abc.conditions` gains one for any guard not already + operator-shaped, and one for every bare `is` type test in a boolean + slot. `wmc` and `mi` are derived from cyclomatic and move with it — the + C# corpus snapshot records `class_wmc_sum` 27 → 29 and a matching fall + in all three `mi` variants — so a `wmc` or `mi` threshold can newly + fire on an unedited C# file carrying guarded arms. - **Kotlin no longer double-counts a subject-less `when` arm's comparison operator** (#1421). `when { x > 5 -> 1; x < 0 -> 2; else -> 0 }` @@ -188,9 +200,12 @@ for historical reference. source spells. As part of the same fix, Kotlin's `is` and `in` tests count as conditions wherever a boolean is evaluated: `if (a is String)` and `if (a in 1..2)` previously scored zero against a decision count of - one. **Metric drift:** Kotlin `abc.conditions` falls for subject-less + one, as did `a and b` — Kotlin spells boolean `and` / `or` / `xor` as + infix *functions*, so they parse as `infix_expression` rather than as + a binary expression and no token arm ever saw them either. + **Metric drift:** Kotlin `abc.conditions` falls for subject-less `when` arms carrying a comparison, and rises for any `is` / `in` test - in a boolean slot. + and any infix boolean call in a boolean slot. - **An enum constant carrying constructor arguments is counted as a branch** (#1407). `A(1)` in `enum E { A(1), B, C(2); }` invokes the diff --git a/big-code-analysis-ast/src/macros/kind_sets.rs b/big-code-analysis-ast/src/macros/kind_sets.rs index 457c7a4c..4e02cabb 100644 --- a/big-code-analysis-ast/src/macros/kind_sets.rs +++ b/big-code-analysis-ast/src/macros/kind_sets.rs @@ -53,7 +53,7 @@ macro_rules! csharp_prefix_unary_expr_kinds { // the C# grammar. Anything in this set, when it appears in a known- // boolean context (if / while / do / for / ternary / binary), counts // as one condition. The set bundles `csharp_invocation_expr_kinds!()` -// with the bare `Identifier` / `BooleanLiteral` leaves *and* the five +// with the bare `Identifier` / `BooleanLiteral` leaves *and* the six // expression kinds whose evaluated value is implicitly boolean in any // idiomatic codebase: // @@ -61,11 +61,25 @@ macro_rules! csharp_prefix_unary_expr_kinds { // - `AwaitExpression` — `await CheckAsync()` // - `CastExpression` — `(bool)v`, `(IDisposable)x is not null` // - `IsPatternExpression` — `x is null`, `x is not Foo f` +// - `IsExpression` — `x is int`, the bare type test // - `ElementAccessExpression` — `flags[0]`, `dict["key"]` // // Before #372 only the first three (invocation / identifier / // boolean) were recognised, so all five kinds above silently scored // zero conditions in `if` / `while` / `do` / ternary contexts. +// +// `IsExpression` (391) and `IsPatternExpression` (392) are distinct +// kinds, not aliases: the grammar emits the first for a bare type test +// (`x is int`) and the second only once a pattern is involved +// (`x is int y`, `x is null`, `x is not Foo`). Listing only the second +// scored `if (x is int)` zero conditions against a cyclomatic decision +// of one, while `if (x is int y)` scored one — an asymmetry between two +// spellings of the same test, and the C# half of the gap #1421 closed +// for Kotlin by adding `IsExpression | InExpression` there. It also +// left #1422's guard slot spelling-dependent in the one case that fix +// claims to have fixed: `when x is int` scored 1 where +// `when IsEven(x)` scored 2. No arm counts the `is` token itself, so +// there is nothing to double-count (§5). #[macro_export] #[doc(hidden)] macro_rules! csharp_bool_terminal_kinds { @@ -79,6 +93,7 @@ macro_rules! csharp_bool_terminal_kinds { | $crate::Csharp::AwaitExpression | $crate::Csharp::CastExpression | $crate::Csharp::IsPatternExpression + | $crate::Csharp::IsExpression | $crate::Csharp::ElementAccessExpression }; } @@ -622,6 +637,17 @@ macro_rules! tsx_bool_terminal_kinds { // themselves `binary_expression` nodes, so they are absent from this set // and contribute nothing — matching the paper's "only unary conditions". // +// `infix_expression` is a call to an infix function, and Kotlin spells +// boolean `and` / `or` / `xor` that way — `a and b` is `a.and(b)`. It +// belongs here for the same reason `call_expression` does, and its +// absence was a regression of #1421 rather than a pre-existing gap: the +// blanket per-entry count that fix removed had been covering it, so +// `when { a and b -> … }` fell from one condition to zero while the +// `if (a and b)` it is supposed to agree with still scored one. The set +// does not discriminate on return type — `f()` counts in a boolean slot +// whatever it returns — so a non-boolean infix call in a boolean slot is +// out of scope here for the same reason. +// // `is_expression` (`a is String`, `a !is String`) and `in_expression` // (`a in 1..2`, `a !in 1..2`) are the two relational forms the grammar // spells as their own production rather than as a `binary_expression`, @@ -643,6 +669,7 @@ macro_rules! kotlin_bool_terminal_kinds { | $crate::Kotlin::ThisExpression | $crate::Kotlin::IsExpression | $crate::Kotlin::InExpression + | $crate::Kotlin::InfixExpression }; } diff --git a/big-code-analysis-book/src/metrics.md b/big-code-analysis-book/src/metrics.md index d9d1c218..a0c866ea 100644 --- a/big-code-analysis-book/src/metrics.md +++ b/big-code-analysis-book/src/metrics.md @@ -145,13 +145,15 @@ application would over-count. | All languages | `default` / `_` wildcard arm excluded from the condition set | Fitzpatrick's Figure 2 lists `default`, but it falls through unconditionally — counting it would inflate `C` on every `switch` / `match` regardless of body. big-code-analysis omits it for every language (the Rust `_ =>` and Java `default:` arms included). | | Tcl | Chain-operand, bare-truthy and ternary slots wired; argument and `return` slots are not | Each operand of a `&&` / `\|\|` chain inside `expr {…}` counts as one condition, so `if {$a && $b}` reports two. `if` / `elseif` / `while` route their `expr {…}` predicate, so a bare-truthy `if {$a}` reports one and `if {!$a}` likewise — matching C's `if (a)` (#1180). A predicate written as a command substitution is a truthy test of that command's result, so `if {[somecmd]}` also reports one, and the redundant `if {[expr {$a \|\| $b}]}` idiom reports three (the chain's two operands plus the substitution). The argument and `return` slots remain unrouted: a negation reached only through a standalone `expr {…}` command still reports zero. | | iRules | Chain-operand, bare-truthy and ternary slots wired; argument and `return` slots are not | Each operand of a `&&` / `\|\|` / `and` / `or` chain counts as one condition (Rule 9), so `if {!$a && !$b}` reports two. iRules also recognises the word-form string-match comparators (`contains`, `starts_with`, `ends_with`, `equals`, `matches`, …) that Tcl lacks (Tcl's `eq` / `ne` / `in` / `ni` are shared). Bare-truthy and ternary slot routing matches its Tcl sibling exactly (#1180) — see that row for the shared detail, including the argument and `return` slots that remain unrouted. | -| All Phase 2 languages (Java, Groovy, C#, Rust, Go, JavaScript, TypeScript, TSX, Mozjs, PHP, C, C++, Objective-C, Mozcpp, Python, Perl, Lua) | `if (true) {}`, `m(!a, !b)`, `return !x` count their operand(s) | Phase 2B routes `if` / `while` / `do-while` / argument-list / `return` slots through the same walker, so the rule applies uniformly across decision-bearing positions. A bare `return x` continues to report zero — Fitzpatrick treats an identifier in a return slot as a value, not a unary conditional. | +| All Phase 2 languages (Java, Groovy, C#, Rust, Go, JavaScript, TypeScript, TSX, Mozjs, PHP, C, C++, Objective-C, Mozcpp, Python, Perl, Lua) | `if (true) {}`, `m(!a, !b)`, `return !x` count their operand(s) | Phase 2B routes `if` / `while` / `do-while` / argument-list / `return` slots through the same walker, so the rule applies uniformly across decision-bearing positions. A bare `return x` continues to report zero — Fitzpatrick treats an identifier in a return slot as a value, not a unary conditional. C# is the one exception to the argument-list half of the rule: its grammar wraps each argument in an `argument` node that the walker does not descend, so `M(!a, !b)` reports zero where every other language in this row reports two. `if (true)` and `return !x` behave as stated. | | Ternary slots: Java, Groovy, C#, C, C++, Objective-C, Mozcpp, JavaScript, TypeScript, TSX, Mozjs, PHP, Perl, Ruby, Python, Tcl, iRules | `a ? !b : !c` counts its condition and both branch operands | The same walker also runs over a ternary's three operand slots, so `a ? !b : !c` reports 4 (the `?` plus three unary conditions) rather than 1. Tcl and iRules reach the same 4 without grammar fields: their `ternary_expr` exposes none, and `_expr` inlines `( … )`, so the slots are located relative to the `?` and `:` tokens instead of by index (#1180). Python arrives at the same total by a different route: `not` operands are counted by the `NotOperator` rule wherever they appear, so only the condition slot is routed through the walker and `(not b) if a else (not c)` likewise reports 4. Languages with no ternary (Rust, Go, Kotlin, Lua, Elixir) are unaffected. | | `for` header condition slot: Java, Groovy, C#, C, C++, Objective-C, Mozcpp, JavaScript, TypeScript, TSX, Mozjs, PHP, Go, Perl | `for (; a; )` counts one condition; `for (;;)` counts none | The loop header's condition is a decision-bearing slot like an `if` predicate, so `for (; a; )`, `for (; !a; )` and `for (; (a); )` each report one. A comparison-shaped condition (`i < n`) is already counted by its own operator arm and is not double-counted. Every one of these grammars except Go exposes a `condition` field on `for_statement` (Perl's sits on its C-style `for_statement_1`), so the slot is read by field name rather than by child index; Go's field sits on the nested `for_clause`, and its outer header slot is located structurally (first named child that is neither the body nor a comment). An **empty** condition counts zero in every language: Fitzpatrick's rules count conditional operators and unary conditions that are present, and an omitted test is not a decision. Java and Groovy previously scored `for (;;)` as one vacuously-true condition and no longer do. Tcl, iRules and Bash are command-dispatched grammars whose loop headers carry no boolean-expression slot to route (grammar-dispatch §9) (#1276). | | Ruby | Bare-predicate `if` / `unless` / `while` / `until` (block and modifier forms) count one condition | Idiomatic Ruby favours bare predicates (`if flag`, `x if flag`); counting the condition slot keeps ABC conditions at or above Ruby's cyclomatic decision count (the alignment enforced across the other languages). A comparison (`if a == b`) or `&&` / `\|\|` chain in the predicate is counted by its own operator / walker arm and is not double-counted. | | Bash | `if` / `elif` / `while` and each non-wildcard `case` arm count one condition | A Bash predicate is a *command*, so the branch keyword itself — not an embedded boolean expression — is the condition signal. Each matches a Bash cyclomatic decision; the bare `*)` case arm (the analogue of `default:`) is excluded, mirroring the cyclomatic standard count. The arithmetic ternary `$(( a ? b : c ))` therefore contributes nothing: it carries no branch keyword, so it falls outside the rule set rather than through a gap in it. | | Kotlin | `try` counts a condition alongside `catch` | Fitzpatrick counts both keywords, and Java / C# / C++ / Groovy already count both; Kotlin previously counted only the catch block. | -| C++, Objective-C, Mozcpp, Rust, Go, Java, Groovy, C#, Kotlin, TypeScript, TSX, JavaScript, Mozjs, Lua, Perl, Ruby, Bash, Elixir | A `<` or `>` that is not a comparison is not a condition | Every one of these grammars spells at least one non-comparison construct with the same bare `<` / `>` token a comparison uses, so the comparison rule is gated on the token's parent. What that excludes, per family: template and generic brackets in C++, Objective-C, Mozcpp, Rust, Go, Java, Groovy, C#, Kotlin, TypeScript and TSX (#1274); JSX tag delimiters in TypeScript, TSX, JavaScript and Mozjs; Lua 5.4 variable attributes (`local x = 1`); a C# comparison-operator overload's declared name (`operator <`); Kotlin's qualified super call (`super.g()`) (#1297); Perl's filehandle and lexical-handle readlines (``, `<$fh>` — but not ``, which the grammar lexes as one token); Ruby's superclass clause (`class Foo < Bar`) and comparison-operator method names (`def <(other)`), and Bash I/O redirection (`cmd > out`) (#1280); Elixir's sigil delimiters (`~s`) (#1256). C# additionally excludes the operator of a relational pattern (`x is > 0`, and the `> 5 =>` arm of a switch expression): the arm or `if` condition slot that owns the pattern already scores the decision, so counting the operator too charged a relational arm twice what the constant arm `5 => 1` scores (#1383). Its `>=` / `<=` spelling is excluded by the same rule through a separate token. The declared name of an operator overload is excluded on every spelling too: C# overloads six comparison operators and gives each a distinct token, so `operator <=`, `operator >=`, `operator ==` and `operator !=` join `operator <` and `operator >`, which alone were excluded before #1420. The exclusion holds wherever the pattern sits, so a pattern no arm or condition slot owns (`bool b = x is > 0;`, `return x is > 0;`, a `when` guard) scores 0, one below the equivalent `x > 0`. PHP, Python, Tcl and iRules emit a bare `<` / `>` from no non-comparison production; C carries the same gate as its C-family siblings although, having no templates, it has nothing to exclude. The gate is a claim about the grammar's productions, not about every parse: where a grammar resolves a generic *call* into nested `binary_expression` nodes, as tree-sitter-kotlin-ng does for `id(a)`, no polarity can exclude it (#1394). | +| C# | A `when` guard is a condition slot, scoring one however it is spelled | A guard on a switch arm (`when_clause`, shared by `switch_expression_arm` and `switch_section`) or on a `catch` (`catch_filter_clause`) is a branch: the pattern can match while the guard fails. Neither metric modelled it, and ABC scored whatever operator happened to sit inside, so `when x > 2` counted one and the equivalent `when IsEven(x)` counted none. The guard's expression is now scored exactly as an `if` condition is — one for a call, `is` test or bare identifier, one via the operator arm for a comparison — so every spelling agrees, and a compound guard (`when a > 1 && b < 2`) keeps its sub-structure rather than collapsing to one (#1422). | +| Kotlin | A subject-less `when` arm scores through the predicate slot; a subject-ful arm scores per entry | A subject-less arm's condition (`when { x > 5 -> … }`) is an ordinary boolean expression, compiled as an `if` predicate, so the comparison inside it is already counted by the operator arms and a blanket per-entry increment double-counted it. A subject-ful arm (`when (x) { in 1..2 -> … }`) lists a pattern rather than an independent boolean expression, so the implicit `subject == pattern` is a decision nothing in the source spells and the entry itself pays for it (#1421). | +| C++, Objective-C, Mozcpp, Rust, Go, Java, Groovy, C#, Kotlin, TypeScript, TSX, JavaScript, Mozjs, Lua, Perl, Ruby, Bash, Elixir | A `<` or `>` that is not a comparison is not a condition | Every one of these grammars spells at least one non-comparison construct with the same bare `<` / `>` token a comparison uses, so the comparison rule is gated on the token's parent. What that excludes, per family: template and generic brackets in C++, Objective-C, Mozcpp, Rust, Go, Java, Groovy, C#, Kotlin, TypeScript and TSX (#1274); JSX tag delimiters in TypeScript, TSX, JavaScript and Mozjs; Lua 5.4 variable attributes (`local x = 1`); a C# comparison-operator overload's declared name (`operator <`); Kotlin's qualified super call (`super.g()`) (#1297); Perl's filehandle and lexical-handle readlines (``, `<$fh>` — but not ``, which the grammar lexes as one token); Ruby's superclass clause (`class Foo < Bar`) and comparison-operator method names (`def <(other)`), and Bash I/O redirection (`cmd > out`) (#1280); Elixir's sigil delimiters (`~s`) (#1256). C# additionally excludes the operator of a relational pattern (`x is > 0`, and the `> 5 =>` arm of a switch expression): the arm or `if` condition slot that owns the pattern already scores the decision, so counting the operator too charged a relational arm twice what the constant arm `5 => 1` scores (#1383). Its `>=` / `<=` spelling is excluded by the same rule through a separate token. The declared name of an operator overload is excluded on every spelling too: C# overloads six comparison operators and gives each a distinct token, so `operator <=`, `operator >=`, `operator ==` and `operator !=` join `operator <` and `operator >`, which alone were excluded before #1420. The exclusion holds wherever the pattern sits, so a pattern no arm or condition slot owns (`bool b = x is > 0;`, `return x is > 0;`) scores 0, one below the equivalent `x > 0`. A `when` guard is no longer such a place: since #1422 the guard is itself a condition slot, so `when n is > 5` reads level with `when n > 5` rather than one below it. PHP, Python, Tcl and iRules emit a bare `<` / `>` from no non-comparison production; C carries the same gate as its C-family siblings although, having no templates, it has nothing to exclude. The gate is a claim about the grammar's productions, not about every parse: where a grammar resolves a generic *call* into nested `binary_expression` nodes, as tree-sitter-kotlin-ng does for `id(a)`, no polarity can exclude it (#1394). | | Java, Groovy, C#, TypeScript, TSX | A `?` used as type syntax is not a ternary | In each of these grammars the ternary `?` and the type-syntax `?` are the *same* anonymous token, so the ternary rule above is gated on the token's parent. Java and Groovy exclude the wildcard bound `List` (#1274); C# excludes the nullable type `int? x` and the constraint `where T : class?`; TypeScript and TSX exclude optional parameters, properties, methods, class fields and tuple elements, and conditional types (`T extends U ? X : Y`, which the type checker resolves and erases before runtime, so it is no more a branch than the `<` / `>` already excluded) (#1275). Safe navigation is untouched: C#'s `a?.b` shares the same token and still counts, while the other languages spell theirs as a distinct one. | #### Worked example diff --git a/src/metrics/abc.rs b/src/metrics/abc.rs index 729575b5..e167ed80 100644 --- a/src/metrics/abc.rs +++ b/src/metrics/abc.rs @@ -542,6 +542,7 @@ mod tests { // decision count — an `else` arm, or a comparison nested inside // another comparison, is an ABC condition with no cyclomatic // decision behind it (#1421). + #[track_caller] fn assert_members_score(container: &crate::FuncSpace, expected: &[(&str, u64, u64)]) { assert_eq!( container.spaces.len(), @@ -549,11 +550,7 @@ mod tests { "member count changed — the fixture moved, not the metric" ); for (name, conditions, cyclomatic) in expected { - let member = container - .spaces - .iter() - .find(|s| s.name.as_deref() == Some(*name)) - .unwrap_or_else(|| panic!("fixture lost `{name}`")); + let member = child_space(container, name); assert_eq!( member.metrics.abc.conditions(), *conditions, @@ -582,6 +579,7 @@ mod tests { // of the control. Measured — with presence-only anchoring, rewriting // `if (x is > 0)` to `if (x > 0)` in one method of five failed // nothing. + #[track_caller] fn assert_fixture_spells(src: &str, path: &str, kinds: &[(u16, usize, &str)]) { let parser = P::new(src.as_bytes().to_vec(), std::path::Path::new(path), None); for (kind, want, spelling) in kinds { @@ -597,9 +595,10 @@ mod tests { } } - // The C# binding of the anchor above. Sixteen callers pass a `foo.cs` + // The C# binding of the anchor above. Fourteen callers pass a `foo.cs` // fixture, so the parser and path are fixed here rather than repeated // at each one. + #[track_caller] fn assert_csharp_fixture_spells(src: &str, kinds: &[(u16, usize, &str)]) { assert_fixture_spells::(src, "foo.cs", kinds); } @@ -4199,8 +4198,7 @@ mod tests { // silently re-point it at an overload. 0 here would mean the // gate swallowed the applying form along with the declaring // one. - let control = &class.spaces[4]; - assert_eq!(control.name.as_deref(), Some("n")); + let control = child_space(class, "n"); assert_eq!( control.metrics.abc.conditions(), 4, @@ -4549,6 +4547,68 @@ mod tests { }); } + // The bare type test `x is int` is `is_expression` (391); only once + // a pattern is involved (`x is int y`, `x is null`) does the grammar + // emit `is_pattern_expression` (392). They are distinct kinds, and + // `csharp_bool_terminal_kinds!()` listed only the second, so the two + // spellings of one test disagreed: `if (x is int)` scored zero + // conditions against a cyclomatic decision of one while + // `if (x is int y)` scored one. + // + // Found by the whole-branch review of this batch, and it falsified + // #1422's own claim in the arm above that a guard "scores one + // however it is spelled" — `when x is int` read 1 where + // `when IsEven(x)` read 2, which is the spelling-dependence that + // fix exists to remove. It is the C# half of the gap #1421 closed + // for Kotlin by adding `IsExpression | InExpression` there. + // + // The plain `if` members are the control that keeps this honest: a + // guard-only fixture could be satisfied by a guard-specific rule, + // and the defect was never guard-specific. `q` and `iq` are the + // already-correct `is_pattern_expression` twins, so a regression + // that reintroduced the asymmetry fails on the pair rather than on + // an absolute number. + #[test] + fn csharp_bare_is_type_test_scores_one_condition() { + let src = "class A { + static bool IsEven(int x) { return true; } + int g(object x) => x switch { object o when x is int => 1, _ => 0 }; + int gq(object x) => x switch { object o when x is int y => 1, _ => 0 }; + int gc(object x) => x switch { object o when IsEven(1) => 1, _ => 0 }; + int p(object x) { if (x is int) { return 1; } return 0; } + int q(object x) { if (x is int y) { return 1; } return 0; } + }"; + assert_csharp_fixture_spells( + src, + &[ + (Csharp::IsExpression as u16, 2, "the bare `is` type tests"), + ( + Csharp::IsPatternExpression as u16, + 2, + "the designation-pattern twins", + ), + (Csharp::WhenClause as u16, 3, "`when` guards"), + ], + ); + check_func_space::(src, "foo.cs", |space| { + assert_members_score( + &space.spaces[0], + &[ + ("IsEven", 0, 1), + // Level with `gq` and `gc`: every guard spelling + // scores one, which is what #1422 claims. + ("g", 2, 3), + ("gq", 2, 3), + ("gc", 2, 3), + // Level with `q`: the two spellings of one type + // test agree outside a guard too. + ("p", 1, 2), + ("q", 1, 2), + ], + ); + }); + } + // The statement `switch` reaches the same `when_clause` kind through // a different parent — `switch_section` rather than // `switch_expression_arm` — which makes it an independent path in @@ -4699,9 +4759,10 @@ mod tests { // // This is the C# instance of the class #1181 fixed for the ternary, // where a comment between a token and its operand shifted every - // positional read. The two commented members are asserted equal to - // their uncommented twins rather than to a literal, so the pair - // cannot drift apart silently; the literals are pinned by + // positional read. Each commented member is asserted against the + // same literal pair as its uncommented twin in the same table, so a + // regression that moved only the commented spelling fails here; the + // literals themselves are pinned by // `csharp_switch_arm_guard_scores_one_condition_however_spelled` and // its catch-filter sibling. #[test] @@ -5589,6 +5650,7 @@ function f(int $a, int $b): int { // score can stand in for: a `when` that grew a subject scores its // entries the pre-#1421 way with every other row still satisfied. // Each fixture is one class, so the members are `spaces[0]`'s. + #[track_caller] fn assert_kotlin_class_members( src: &str, kinds: &[(u16, usize, &str)], @@ -5728,6 +5790,38 @@ function f(int $a, int $b): int { ); } + // `kotlin_enclosing_when_has_subject` stops scanning at the `{`, so + // that a subject-less `when` does not walk all N arms once per arm. + // The bound has to clear the header first, and the header can carry + // an `extra`: tree-sitter hangs `when /*c*/ (x)`'s comment off the + // `when_expression` between the keyword and the subject, so a bound + // one child tighter — `child(1)`, or a stop at the first *named* + // child — reads `cmtSubject` as subject-less. It would then score + // its `1` arm through the slot, where a `number_literal` is not a + // Kotlin terminal, and report 0 conditions instead of 1. That is + // what discriminates here; `cmtNone` is the other direction, where + // the brace is all the bound has to stop on. + // + // The `block_comment` census is the fixture anchor: delete either + // comment and the members still read 1, because a plain `when (x)` + // and a plain `when {` both do. The count fails by name instead. + #[test] + fn kotlin_when_subject_scan_clears_a_comment_before_the_brace() { + let src = "class K { + fun cmtSubject(x: Int): Int = when /*c*/ (x) { 1 -> 1; else -> 0 } + fun cmtNone(a: Boolean): Int = when /*c*/ { a -> 1; else -> 0 } + }"; + assert_kotlin_class_members( + src, + &[ + (Kotlin::BlockComment as u16, 2, "the two header comments"), + (Kotlin::WhenSubject as u16, 1, "`cmtSubject`'s subject"), + (Kotlin::WhenEntry as u16, 4, "`when` entries"), + ], + &[("cmtSubject", 1, 2), ("cmtNone", 1, 2)], + ); + } + // A subject-less condition reaches the slot through the same // `kotlin_inspect_container` unwrapping an `if` predicate does, which // is what separates this design from suppressing the entry's @@ -5878,6 +5972,58 @@ function f(int $a, int $b): int { ); } + // Kotlin spells boolean `and` / `or` / `xor` as infix *functions*, so + // `a and b` parses as `infix_expression` — not `binary_expression`, + // and not any token arm. It belongs in + // `kotlin_bool_terminal_kinds!()` for the same reason + // `call_expression` does: `a and b` is `a.and(b)`. + // + // This was a regression of #1421, not a pre-existing gap, and it is + // the third shape the blanket per-entry count turned out to be + // propping up — after `is` / `in` and bare parens, which that fix + // caught. Found by the whole-branch review. Measured on both sides: + // `when { a and b -> … }` scored 1 before and 0 after, while the + // `if (a and b)` it is supposed to agree with still scored 1. + // + // The `if` members are the anchor rather than a second `when`: the + // whole premise of #1421 is that a subject-less arm scores like an + // `if` predicate, so a fixture where the two disagree is the + // statement of the bug. `andAmp` holds the pair level against the + // `&&` spelling, which never regressed and would otherwise be the + // only form under test. + #[test] + fn kotlin_infix_boolean_functions_are_unary_conditions() { + let src = "class K { + fun whenAnd(a: Boolean, b: Boolean): Int = when { a and b -> 1; else -> 0 } + fun whenOr(a: Boolean, b: Boolean): Int = when { a or b -> 1; else -> 0 } + fun whenXor(a: Boolean, b: Boolean): Int = when { a xor b -> 1; else -> 0 } + fun whenAmp(a: Boolean, b: Boolean): Int = when { a && b -> 1; else -> 0 } + fun ifAnd(a: Boolean, b: Boolean): Int { if (a and b) { return 1 }; return 0 } + }"; + assert_kotlin_class_members( + src, + &[ + (Kotlin::WhenSubject as u16, 0, "`when` subjects"), + (Kotlin::InfixExpression as u16, 4, "the infix calls"), + (Kotlin::AMPAMP as u16, 1, "`whenAmp`'s `&&`"), + ], + &[ + // 0 before this fix: the predicate slot saw a kind it + // did not classify and no token arm matched. + ("whenAnd", 1, 2), + ("whenOr", 1, 2), + ("whenXor", 1, 2), + // Never regressed — `&&` is a `binary_expression` whose + // operator the token arm owns. Holds the others level. + ("whenAmp", 2, 3), + // The agreement #1421 exists to establish: a + // subject-less arm scores what the equivalent `if` + // predicate scores. + ("ifAnd", 1, 2), + ], + ); + } + #[test] fn kotlin_catch_block_counts() { check_metrics::( diff --git a/src/metrics/abc/csharp.rs b/src/metrics/abc/csharp.rs index 71f1ad8a..c4017909 100644 --- a/src/metrics/abc/csharp.rs +++ b/src/metrics/abc/csharp.rs @@ -520,17 +520,17 @@ fn csharp_walk_for_conditions<'a>( // named nodes and may precede the expression, so `when /*c*/ g` // hands a `comment` to a first-child read and silently restores // the spelling-dependence this fix removes. C#'s extras at this - // pin are `comment` plus ten `preproc_*` kinds — none of them a + // pin are `comment` plus nine `preproc_*` kinds — none of them a // `csharp_bool_terminal_kinds!()` member, and none a paren or // `!`-prefix wrapper — so passing them through the slot adds // nothing and the loop cannot double count a clause that holds // one expression by construction. // - // The sibling `if` / `while` / `do` slots read a fixed child - // index and so still lose their condition to a leading comment - // (`if (/*c*/ g)` scores 0). That is the same class of bug and - // predates this arm; it is left to its own change rather than - // widened into here. + // FIXME(#1455): the sibling `if` / `while` / `do` slots read a + // fixed child index and so still lose their condition to a + // leading comment (`if (/*c*/ g)` scores 0). That is the same + // class of bug and predates this arm; it is left to its own + // change rather than widened into here. WhenClause | CatchFilterClause => { for guard in node.children().filter(Node::is_named) { csharp_count_condition(&guard, node, conds); diff --git a/src/metrics/abc/kotlin.rs b/src/metrics/abc/kotlin.rs index 50b8745c..8837f91e 100644 --- a/src/metrics/abc/kotlin.rs +++ b/src/metrics/abc/kotlin.rs @@ -177,10 +177,23 @@ fn kotlin_count_condition(condition: &Node, parent: &Node, conditions: &mut f64) // the parent's children rather than a `child_by_field_name`. Scanning // rather than reading a fixed index also keeps a leading `extra` — a // comment between `when` and `(x)` — from displacing the answer. +// +// The scan stops at the opening brace, which is what keeps it `O(1)`. +// The subject can only sit in the header, between `when` and `{`, so +// everything past the brace is arms — and an unbounded `any()` over a +// *subject-less* `when` never short-circuits, so it walks every arm +// once per arm. That is quadratic in the arm count, and measurable: +// before the bound, a generated 8,000-arm subject-less `when` took +// 2.5 s against 0.03 s for the same-size subject-ful control. The +// bound preserves the `extra` tolerance above — `when /*c*/ (x) {` +// still finds the subject, `when /*c*/ {` still stops at the brace — +// and degrades to the old whole-child scan only when error recovery +// leaves the brace out entirely, where the answer is unchanged. fn kotlin_enclosing_when_has_subject<'a>(entry: &Node<'a>, ancestors: Ancestors<'a, '_>) -> bool { ancestors.parent(entry).is_some_and(|when_expression| { when_expression .children() + .take_while(|child| child.kind_id() != Kotlin::LBRACE) .any(|child| child.kind_id() == Kotlin::WhenSubject) }) } diff --git a/src/metrics/cyclomatic/csharp.rs b/src/metrics/cyclomatic/csharp.rs index cd798979..85912d9f 100644 --- a/src/metrics/cyclomatic/csharp.rs +++ b/src/metrics/cyclomatic/csharp.rs @@ -21,6 +21,13 @@ impl Cyclomatic for CsharpCode { // Standard-only: individual switch statement arms. The `case` // keyword token is what is matched here; `default:` uses a // distinct `Default` token and is correctly excluded. + // + // FIXME(#1450): `goto case 2;` spells the same token, so it + // scores a decision without being an arm. The ABC half of + // this is the matching arm in `src/metrics/abc/csharp.rs`; + // the two have to move together or the §8 parity + // `conditions == cyclomatic() - 1` breaks, which is why + // neither has been gated on its own. Case => { stats.cyclomatic += 1.; } From 22eec7e8ce74f2c1b3f0d2ea8328ef9fc0b06f30 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Mon, 14 Sep 2026 12:12:24 -0700 Subject: [PATCH 10/11] fix(abc): count own-production bool constructs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A boolean test the grammar gives its own production, rather than spelling as a `binary_expression`, reaches no comparison-token arm and was absent from its `_bool_terminal_kinds!()` set, so it scored zero conditions where a bare identifier in the same slot scores one. Both walker paths were short: the `if` predicate and the operands of a `&&` chain each measured one below their identifier control at equal cyclomatic. Eight constructs across three languages, each taking exactly one of the two routes so nothing is counted twice: - Groovy `a in l` / `a !in l` joins the terminal set. A token arm cannot serve it: `in` is shared with `for_in_statement`, so an ungated arm would score every `for (x in l)` header, and `!in` emits no operator token at all — `token(seq("!in", …))` makes it invisible to tree-sitter, so no gate could reach the negated spelling. - Groovy `===` / `!==` / `=~` / `==~` become condition tokens, so they also score outside a boolean slot as `==` already did, closing a within-language asymmetry between two equality operators. A grammar.json sweep gives each token exactly one producing rule, so no gate is needed. - Perl `/^#/` and `m{^#}` — sibling rules, not aliases — join the set. A bound `$x =~ /^#/` still scores one, not two. - Ruby `a in Integer` joins the set; `match_pattern` stays out, since `expr => pat` raises rather than yielding a bool. Rust's `matches!(…)` measures short the same way and is deliberately left out. Its fix is `macro_invocation`, which also catches `cfg!` / `dbg!` / `todo!`, and the one corpus snapshot it moves is a `cfg!` rather than a `matches!` — so the arm's first effect here is precisely the breadth #1461 item 2 exists to decide. Settling that by inclusion would decide the issue by accident. Groovy, Perl and Ruby have no snapshotted corpus, so nothing moves. `own_production_bool_constructs` pins both slots per language against an identifier control plus the absolute conditions/cyclomatic pair, and pins the two Perl spellings as distinct grammar kinds so neither terminal-set entry can go dead unnoticed. Each arm was verified by removal, confirming the module fails naming its own language. Fixes #1449 --- big-code-analysis-ast/src/macros/kind_sets.rs | 74 +++++ src/metrics/abc.rs | 311 ++++++++++++++++++ src/metrics/abc/groovy.rs | 32 +- 3 files changed, 416 insertions(+), 1 deletion(-) diff --git a/big-code-analysis-ast/src/macros/kind_sets.rs b/big-code-analysis-ast/src/macros/kind_sets.rs index 4e02cabb..3de830fb 100644 --- a/big-code-analysis-ast/src/macros/kind_sets.rs +++ b/big-code-analysis-ast/src/macros/kind_sets.rs @@ -174,6 +174,37 @@ macro_rules! java_bool_terminal_kinds { // `.groovy` file at all, and DeepSpeech's 16 `.gradle` files are // outside every test glob (`tests/corpus/deepspeech_test.rs` globs // `*.cc` / `*.cpp` / `*.h` / `*.hh`), so this fix moves no snapshot. +// +// `membership_expression` (`a in l`, `a !in l`) is the Groovy spelling +// of Kotlin's `in_expression`, and joins the set for the same reason +// #1421 added that one: the grammar gives membership its own +// production rather than a `binary_expression`, so no comparison-token +// arm ever sees it and `if (a in l)` scored zero conditions against a +// cyclomatic decision of one. +// +// It is the one Groovy relational form that has to come through the +// terminal set rather than through `groovy_count_token_condition`'s +// token arm, and a `grammar.json` sweep of dekobon-tree-sitter-groovy +// 0.2.2 says why on both halves: the `in` token is shared with +// `for_in_statement`, so an ungated token arm would score every +// `for (x in list)` header, and the `!in` spelling emits **no operator +// token at all** — `bca dump` shows `membership_expression` with two +// `identifier` children and nothing between them — so a token arm +// could not reach the negated form however it were gated. Listing the +// wrapper covers both spellings at once and double-counts neither, +// since neither token is counted anywhere (§5). +// +// The sibling relational productions `identity_expression` (`===`, +// `!==`) and `regex_find_expression` / `regex_match_expression` (`=~`, +// `==~`) are deliberately **absent** here: each emits an operator +// token that the same sweep finds in that one production and nowhere +// else, so they are counted as plain comparison tokens beside `==` / +// `!=` in `groovy_count_token_condition` — the spelling every other +// language in the workspace uses for those operators (Kotlin, JS, +// PHP, Elixir for `===`; Perl, Ruby, Bash for `=~`), and the one that +// also scores them outside a boolean slot, where `def r = (a == b)` +// already scores and `def r = (a === b)` did not. Listing them in +// both places would score each twice (§5). #[macro_export] #[doc(hidden)] macro_rules! groovy_bool_terminal_kinds { @@ -187,6 +218,7 @@ macro_rules! groovy_bool_terminal_kinds { | $crate::Groovy::CastExpression | $crate::Groovy::ParenthesizedTypeCast | $crate::Groovy::InstanceofExpression + | $crate::Groovy::MembershipExpression }; } @@ -207,6 +239,7 @@ macro_rules! rust_bool_terminal_kinds { // the C# fix in #372 (lesson 19), which closed the same gap // for `CastExpression`, `MemberAccessExpression`, and // `AwaitExpression` on the C# side. + // () => { $crate::Rust::Identifier | $crate::Rust::BooleanLiteral @@ -424,6 +457,31 @@ macro_rules! python_bool_terminal_kinds { // DeepSpeech `native_client` tree, seven snapshots): PHP's corpus files // carry no truthy numeric operand, and no corpus carries a `.groovy` // file at all. +// +// `pattern_matcher` (`/^#/`) and `pattern_matcher_m` (`m{^#}`) are the +// two spellings of a match against the implicit `$_`. They are +// **sibling rules, not aliases** — ids 337 and 336, the distinction +// this file's own numeric-literal note warns is invisible to an alias +// sweep — so both have to be listed or the `m{}` half stays at zero. +// A bound match (`$x =~ /^#/`) is a `binary_expression` whose `=~` +// token the dispatcher already counts; the bare form carries no +// operator token at all, which is why `if (/^#/)` scored zero +// conditions against `if ($x)`'s one. +// +// Listing them double-counts nothing (§5). In `$x =~ /^#/` the +// pattern is a child of the `binary_expression`, and every walker that +// consumes this set either breaks on that `binary_expression` +// (`perl_inspect_container`, `perl_count_condition`) or requires the +// list node itself to be the `&&`-chain parent +// (`perl_count_unary_conditions`), so the pattern node is never +// reached alongside its own `=~`. +// +// Three further rules in the same grammar family are deliberately +// absent because whether they are boolean *tests* is a judgement call, +// not a dispatch gap: `substitution_pattern_s` (`s///`) and +// `transliteration_tr_or_y` (`tr///`) each evaluate to a count rather +// than a bool, and `regex_pattern_qr` (`qr//`) to a compiled-pattern +// object that is always true. All three measure zero conditions today. #[macro_export] #[doc(hidden)] macro_rules! perl_bool_terminal_kinds { @@ -450,6 +508,8 @@ macro_rules! perl_bool_terminal_kinds { | $crate::Perl::CallExpressionRecursive | $crate::Perl::CallExpressionWithBareword | $crate::Perl::MethodInvocation + | $crate::Perl::PatternMatcher + | $crate::Perl::PatternMatcherM }; } @@ -709,6 +769,19 @@ macro_rules! kotlin_bool_terminal_kinds { // None of the four kinds has a numeric-suffix alias in tree-sitter-ruby // 0.23.1; `_int_or_float` (`Ruby::IntOrFloat`) is a hidden supertype the // parser never emits (grammar-dispatch §2). +// +// `test_pattern` is Ruby 3.0's one-line pattern test (`a in Integer`), +// which evaluates to a boolean. The grammar gives it its own +// production, so the comparison-token arm in `metrics/abc/ruby.rs` +// never sees it — that arm is gated on a `binary` parent and lists no +// `in` token — and `if a in Integer` scored zero conditions against a +// cyclomatic decision of one. Nothing counts the `in` token itself, so +// listing the wrapper scores it exactly once (§5). +// +// Its neighbour `match_pattern` (`expr => pat`, id 252) is **not** +// here and must not be added: that spelling raises `NoMatchingPattern` +// on failure rather than yielding a boolean, so it is a destructuring +// assignment, not a condition. #[macro_export] #[doc(hidden)] macro_rules! ruby_bool_terminal_kinds { @@ -730,6 +803,7 @@ macro_rules! ruby_bool_terminal_kinds { | $crate::Ruby::Float | $crate::Ruby::Rational | $crate::Ruby::Complex + | $crate::Ruby::TestPattern }; } diff --git a/src/metrics/abc.rs b/src/metrics/abc.rs index e167ed80..626f13a7 100644 --- a/src/metrics/abc.rs +++ b/src/metrics/abc.rs @@ -13378,3 +13378,314 @@ mod numeric_bool_operands { }); } } + +/// A relational construct with its own grammar production must score +/// like an identifier in the same boolean slot (#1449, #1461). +/// +/// Every language here spells at least one boolean test as a dedicated +/// node rather than as a `binary_expression`, so no comparison-token +/// arm ever sees it. A kind missing from `_bool_terminal_kinds!()` +/// scores **zero**, silently, and zero is indistinguishable from a +/// construct that legitimately scores zero — which is why the headline +/// claim of each case is a *comparison* against an identifier control +/// in the identical slot rather than an absolute number. +/// +/// The constructs, and the route each fix took: +/// +/// - **Groovy** `a in l` / `a !in l` (`membership_expression`) joins +/// the terminal set. The other four — `a === b` / `a !== b` +/// (`identity_expression`) and `s =~ /p/` / `s ==~ /p/` +/// (`regex_find_expression`, `regex_match_expression`) — are counted +/// as operator tokens in `groovy_count_token_condition` instead, so +/// they also score outside a boolean slot as `==` already did. Both +/// macro and arm carry the reasoning; the rule that matters here is +/// that a construct takes exactly one of the two routes, never both +/// (`.claude/rules/grammar-dispatch.md` §5). +/// - **Perl** `/^#/` (`pattern_matcher`) and `m{^#}` +/// (`pattern_matcher_m`), the two spellings of a match against the +/// implicit `$_`. They are sibling rules, not aliases, so both are +/// listed — a fixture carrying only the first would have left the +/// `m{}` half at zero and read as covered. +/// - **Ruby** `a in Integer` (`test_pattern`), the one-line pattern +/// test. Decided against `match_pattern` (`expr => pat`), which +/// raises rather than yielding a boolean. +/// +/// Rust's `matches!(…)` measures short in the same way, and is +/// deliberately **not** here: its fix is `macro_invocation`, which also +/// catches `cfg!` / `dbg!` / `todo!`, and the one corpus snapshot it +/// moves is a `cfg!` rather than a `matches!`. That breadth is what +/// #1461 item 2 exists to decide, so it is left to that issue rather +/// than settled by inclusion here. +/// +/// Two slots per language, because the sets feed two independent walker +/// paths (§11) and every construct above measured short in **both**: the +/// operands of a `&&` chain, and the predicate of an `if`. A fixture of +/// only one leaves the other untested. +/// +/// Each row also carries the construct's negated spelling. That is not +/// redundancy: `_inspect_container` is a *third* consumer of every +/// terminal set, reached only after a `(…)` or `!…` wrapper is peeled, +/// and it is the consumer that decides whether the peeled operand sits +/// in boolean context at all. Without a negated row, Groovy's and +/// Rust's `inspect_container` arms were the site no fixture here +/// exercised. +/// +/// Even with it, no language exercises all three consumers through +/// these two slots, and which one is missed differs by language — +/// Perl's `perl_count_condition` (reached only from a ternary or a +/// C-style `for` header) and Ruby's `ruby_count_unary_conditions` +/// (unreachable here because `in` binds looser than `&&`, so the chain +/// slot must parenthesise and routes through `inspect_container` +/// instead). Both are covered elsewhere in this file; the gap is +/// recorded rather than papered over, because a reader comparing the +/// slot count to the consumer count will otherwise assume it is three. +/// +/// The `cyclomatic` half of `every_construct_scores_its_recorded_values` +/// is what rules out a regression that moved both metrics together: +/// cyclomatic must not move when only the operand spelling does, which +/// is what makes a `conditions` move unambiguously ABC's. The recorded +/// figures differ per slot (Rust's chain slot carries the extra `&&` +/// decision), so each slot pins its own pair rather than sharing one. +// Gated on the union of the four languages with rows, so a build +// enabling none of them drops the module instead of failing its +// guards (`.claude/rules/testing.md`, #1286 / #1411). +#[cfg(test)] +#[cfg(any(feature = "groovy", feature = "perl", feature = "ruby"))] +mod own_production_bool_constructs { + use crate::test_support::metrics_verbatim; + use crate::{LANG, MetricsOptions}; + + /// One fixture shape: a source template with a `{}` slot for the + /// whole boolean expression, and the `abc.conditions_sum` / + /// `cyclomatic_sum` every spelling in that slot must produce. + type Slot = (&'static str, u64, u64); + + /// A language's two slots, its identifier control, the constructs + /// that must score the same, and how many of those there should be. + /// + /// The count is not bookkeeping. `for_each_case` counts *languages*, + /// so trimming Groovy's list back to a single construct — or + /// dropping Perl's `m{}` half, which is a separate grammar rule from + /// its `//` half — leaves the module green with that coverage + /// deleted. Pinning the length makes such a trim a deliberate + /// two-line edit instead of a silent one. + type Case = ([Slot; 2], &'static str, &'static [&'static str], usize); + + fn conditions(lang: LANG, source: &str) -> u64 { + metrics_verbatim(lang, source.as_bytes(), MetricsOptions::default()) + .abc + .conditions_sum() + } + + fn cyclomatic_sum(lang: LANG, source: &str) -> u64 { + metrics_verbatim(lang, source.as_bytes(), MetricsOptions::default()) + .cyclomatic + .cyclomatic_sum() + } + + /// `([chain_slot, predicate_slot], identifier, constructs)` per + /// language. + /// + /// `{}` is the left-hand operand of a two-operand short-circuit + /// chain in the first slot and the whole `if` predicate in the + /// second, so every fixture differs from its own control in exactly + /// the construct under test. + /// + /// Ruby's chain slot parenthesises the slot because `in` binds + /// looser than `&&` there; the control is parenthesised identically, + /// so the parens cancel out of the comparison and + /// `ruby_inspect_container` unwraps them for both sides alike. + fn cases(lang: LANG) -> Option { + Some(match lang { + LANG::Groovy => ( + [ + ("def f(a, b, l, s) {\n return {} && b\n}\n", 2, 3), + ("def f(a, b, l, s) {\n if ({}) { return 1 }\n}\n", 1, 3), + ], + "b", + &[ + "a in l", + "a !in l", + "a === b", + "a !== b", + "s =~ /p/", + "s ==~ /p/", + "!(a in l)", + ], + 7, + ), + LANG::Perl => ( + [ + ("sub f {\n my $x = {} && $b;\n}\n", 2, 3), + ("sub f {\n if ({}) { 1; }\n}\n", 1, 3), + ], + "$a", + &["/^#/", "m{^#}", "!/^#/"], + 3, + ), + LANG::Ruby => ( + [ + ("def f(a)\n ({}) && b\nend\n", 2, 3), + ("def f(a)\n if {}\n 1\n end\nend\n", 1, 3), + ], + "a", + &["a in Integer", "!(a in Integer)"], + 2, + ), + _ => return None, + }) + } + + /// Runs `check` once per enabled language that has a case, having + /// first established that the case can still assert something. + /// + /// The three guards mirror `numeric_bool_operands::for_each_case`, + /// where each was added only after a measured perturbation of it + /// left that module green: `checked > 0` for the runtime half of + /// the feature gate, the recorded construct count so a trimmed row + /// cannot vanish silently, and the `{}` slot so `str::replace` does + /// not degenerate into comparing a string with itself. + fn for_each_case(check: impl Fn(LANG, Case)) { + let mut checked = 0; + for lang in LANG::into_enum_iter() { + if !lang.is_enabled() { + continue; + } + let Some(case @ (slots, _, constructs, expected_constructs)) = cases(lang) else { + continue; + }; + // Both halves are needed, and the second does not imply the + // first. The length check alone only asserts that the list + // and its recorded count *agree*, so emptying a row and + // setting its count to 0 — the shape a careless "the test + // failed, fix the number" edit takes — deleted that + // language's coverage with both tests still green when + // measured. `numeric_bool_operands` above has the same + // weakness and is left alone here, being out of this + // change's scope. + assert!( + !constructs.is_empty(), + "{lang:?}: the construct list is empty; this language asserted nothing" + ); + assert_eq!( + constructs.len(), + expected_constructs, + "{lang:?}: the construct list no longer covers every own-production \ + boolean spelling this language was fixed for" + ); + for (template, _, _) in slots { + assert!( + template.contains("{}"), + "{lang:?}: template lost its `{{}}` slot: {template}" + ); + } + check(lang, case); + checked += 1; + } + assert!( + checked > 0, + "no language with an own-production boolean construct enabled; \ + this test asserted nothing" + ); + } + + #[test] + fn an_own_production_construct_scores_like_an_identifier() { + for_each_case(|lang, (slots, identifier, constructs, _)| { + for (template, _, _) in slots { + let baseline = conditions(lang, &template.replace("{}", identifier)); + for construct in constructs { + let source = template.replace("{}", construct); + let scored = conditions(lang, &source); + assert_eq!( + scored, baseline, + "{lang:?}: `{construct}` scored {scored} conditions against \ + `{identifier}`'s {baseline}\n source: {source}" + ); + } + } + }); + } + + /// The two Perl spellings must remain two distinct grammar kinds. + /// + /// `perl_bool_terminal_kinds!()` lists `PatternMatcher` **and** + /// `PatternMatcherM` on the stated grounds that `/^#/` and `m{^#}` + /// are sibling rules rather than aliases of one. Nothing above pins + /// that: if a grammar bump collapsed `m{}` onto `pattern_matcher`, + /// both fixtures would keep scoring through the surviving entry, + /// the `PatternMatcherM` arm would become dead code, and every test + /// in this module would stay green — the silent-drift shape lesson + /// 34 and grammar-dispatch §2 exist to catch. + /// + /// Asserting the ids differ is not enough on its own, since two + /// distinct enum variants prove nothing about what the parser + /// emits. So each spelling is parsed and required to produce its + /// own kind and not the other's. + #[test] + #[cfg(feature = "perl")] + fn the_two_perl_match_spellings_are_distinct_kinds() { + use crate::{ParserTrait, Perl, PerlParser}; + use big_code_analysis_ast::test_support::ast_has_kind_id; + + assert_ne!( + Perl::PatternMatcher as u16, + Perl::PatternMatcherM as u16, + "the two spellings share one kind id; one terminal-set entry is dead" + ); + + for (src, present, absent) in [ + ( + "sub f { if (/^#/) { 1; } }", + ("pattern_matcher", Perl::PatternMatcher as u16), + ("pattern_matcher_m", Perl::PatternMatcherM as u16), + ), + ( + "sub f { if (m{^#}) { 1; } }", + ("pattern_matcher_m", Perl::PatternMatcherM as u16), + ("pattern_matcher", Perl::PatternMatcher as u16), + ), + ] { + let parser = PerlParser::new( + src.as_bytes().to_vec(), + &std::path::PathBuf::from("f.pl"), + None, + ); + let (present_name, present_id) = present; + let (absent_name, absent_id) = absent; + assert!( + ast_has_kind_id(&parser, present_id), + "`{src}` no longer emits `{present_name}`" + ); + assert!( + !ast_has_kind_id(&parser, absent_id), + "`{src}` now also emits `{absent_name}`; the two spellings have \ + collapsed and one `perl_bool_terminal_kinds!()` entry is dead" + ); + } + } + + /// The absolute anchor under the comparison above: every spelling + /// must produce the slot's recorded `conditions`, and must leave + /// `cyclomatic` alone. + #[test] + fn every_construct_scores_its_recorded_values() { + for_each_case(|lang, (slots, identifier, constructs, _)| { + for (template, expected_conditions, expected_cyclomatic) in slots { + for spelling in std::iter::once(identifier).chain(constructs.iter().copied()) { + let source = template.replace("{}", spelling); + assert_eq!( + conditions(lang, &source), + expected_conditions, + "{lang:?}: `{spelling}` conditions\n source: {source}" + ); + assert_eq!( + cyclomatic_sum(lang, &source), + expected_cyclomatic, + "{lang:?}: `{spelling}` cyclomatic_sum\n source: {source}" + ); + } + } + }); + } +} diff --git a/src/metrics/abc/groovy.rs b/src/metrics/abc/groovy.rs index 74d51b0c..2e1f2f1f 100644 --- a/src/metrics/abc/groovy.rs +++ b/src/metrics/abc/groovy.rs @@ -220,7 +220,37 @@ fn groovy_count_token_condition<'a>( // C / PHP short-ternary reading that also walks the left operand // as a unary condition — it keeps `abc.conditions` equal to // `cyclomatic() - 1` on the chain (grammar-dispatch §8). - GTEQ | LTEQ | EQEQ | BANGEQ | Else | Case | Try | Catch | QMARKCOLON => { + // `EQEQEQ` / `BANGEQEQ` (`===`, `!==`) and `EQTILDE` / + // `EQEQTILDE` (`=~`, `==~`) are Groovy's identity and regex + // comparisons. They sit in this ungated arm beside `==` / `!=` + // for the reason those do, and a `grammar.json` sweep of + // dekobon-tree-sitter-groovy 0.2.2 is what makes ungating + // safe: each of the four tokens is emitted by exactly one + // production — `identity_expression` for the first pair, + // `regex_find_expression` / `regex_match_expression` for the + // second — so unlike `GT` / `LT` there is no type-argument or + // loop-header spelling to exclude. + // + // Their own expression kinds are therefore **not** in + // `groovy_bool_terminal_kinds!()`; counting both would score + // each twice (§5). The token is the better half of that choice + // because it scores outside a boolean slot as well, where + // `def r = (a == b)` already scored 1 and `def r = (a === b)` + // scored 0 — a within-language asymmetry between two + // equality operators. It is also how every other language here + // spells these: `EQEQEQ | BANGEQEQ` are token arms in Kotlin, + // the JS family, PHP and Elixir, and `EQTILDE` in Perl, Ruby + // and Bash. `getter/groovy.rs` already classifies all four as + // Halstead operators. + // + // Groovy's membership (`in` / `!in`) is the one relational + // form that cannot come through here — see + // `groovy_bool_terminal_kinds!()` for why. The spaceship `<=>` + // is deliberately still absent: it yields -1 / 0 / 1 rather + // than a boolean, so whether it is a condition at all is the + // open question in FIXME(#1461) item 4, not an oversight here. + GTEQ | LTEQ | EQEQ | BANGEQ | EQEQEQ | BANGEQEQ | EQTILDE | EQEQTILDE | Else | Case + | Try | Catch | QMARKCOLON => { stats.conditions += 1.; } // As in Java: a bare `?` is either a ternary head or the head of From 672125647983ac6458f0eb1cf85b650b343887c1 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Mon, 14 Sep 2026 12:53:27 -0700 Subject: [PATCH 11/11] fix(abc): close the condition-slot gaps from PR review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both findings from the code review of this PR. Same root cause: a condition slot delegates to a per-language helper whose third outcome is silence, so a valid boolean expression outside its terminal / paren / unary allowlist contributes nothing. The blanket counts the `when`-arm and guard fixes removed had been covering those shapes. Kotlin's peel understood only a prefix `!` and parentheses. `unary_expression` is one kind for both unary spellings, so the slot routed a postfix `a!!` in as handled while the peel's positional read found the `!!` token and stopped; an `as` cast was not a wrapper at all. Both regressed from 1 to 0 when the subject-less `when` fix moved payment from the entry to this slot. The peel now reads its operand by grammar field, and `kotlin_count_condition` derives its wrapper set from the peel rather than restating it — those two lists disagreeing is the defect, so there is one source of truth. The safe cast `as?` stays excluded: its `AsQMARK` token is already a condition, and peeling it too scores `a as? T` twice. C# `??` was a cyclomatic decision and no ABC condition, so a `when b ?? false` guard scored zero where every other spelling scored one. Counting the token levels the spellings without touching the slot, which keeps a compound guard's sub-structure. The review asked for an unconditional slot increment instead; measured, that takes `when x > 2` from 3 to 4 — the double count the guard fix removed — and suppression inside the guard flattens `when a > 1 && b < 2` from 4 to 3, which that fix preserves deliberately. The JS family has always counted `??`, so this closes a divergence rather than opening one. It narrows the ABC-versus-cyclomatic gap on a guard from two to one without closing it, since cyclomatic counts `??` in addition to the guard clause while ABC's condition substitutes for the slot; away from a guard, parity is exact. Also here, from the whole-branch review of the same change: the Groovy terminal set's comment claimed the grammar has no `array_access` analogue, which is false — `subscript_expression`, `safe_navigation_expression` and three siblings all exist, are legal in a boolean slot, and are unlisted, so `if (l[0])` and `if (a?.b)` score zero. Filed separately; the sentence is corrected because a comment asserting the analogue does not exist is what stops the next reader looking. `kotlin_chain_operands_peel_null_assertions_and_casts` gained a kind census per row — the bare `a && b` control scores the same 2, so trimming the `!!` out of a fixture had left it green with its subject removed. And `assert_fixture_spells` now refuses an empty kind list, which had made every following assertion in a caller vacuous. Costs one corpus snapshot: `linq.cs` is the only C# corpus file with a `??`, and its file-level conditions go 10 to 12. Only abc keys move. --- CHANGELOG.md | 37 +++ big-code-analysis-ast/src/macros/kind_sets.rs | 18 +- src/metrics/abc.rs | 248 +++++++++++++++++- src/metrics/abc/csharp.rs | 38 ++- src/metrics/abc/kotlin.rs | 133 +++++++--- tests/repositories/big-code-analysis-output | 2 +- 6 files changed, 422 insertions(+), 54 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a3dce857..f99adf46 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -135,6 +135,43 @@ for historical reference. ### Fixed +- **ABC now counts a boolean test that the grammar gives its own + production** (#1449). A construct spelled as a dedicated node rather + than a `binary_expression` reaches no comparison-token arm, so Groovy's + `in` / `!in` / `===` / `!==` / `=~` / `==~`, Perl's bare `//` and `m{}` + matches, and Ruby's `a in Integer` each scored zero conditions in an + `if` predicate or a `&&` operand where a bare identifier scores one. + Groovy's `===` / `!==` / `=~` / `==~` are counted as comparison + operator tokens rather than terminal kinds, so they now also score + outside a boolean slot, as `==` already did; `in` / `!in` cannot take + that route, because `in` is shared with `for_in_statement` and `!in` + emits no operator token at all. **Metric drift:** `abc.conditions` + rises by one per occurrence of these constructs in a boolean slot, and + by one per `===` / `!==` / `=~` / `==~` anywhere in Groovy. + +- **Kotlin ABC counts a null-asserted or cast condition, and C# ABC + counts `??`** (#1459). Both languages model a condition slot that + delegates to a per-language helper, and both helpers contributed + nothing for a valid boolean expression outside their allowlist. In + Kotlin, `when { a!! -> … }` and `when { a as Boolean -> … }` scored + zero against a decision count of one: `unary_expression` is a single + kind for both the prefix `!x` and the postfix `x!!`, so the slot routed + the null assertion in as handled while the peel looked for the operand + on the wrong side of the token, and an `as` cast was not recognised at + all. The peel now reads its operand through the grammar's fields and + unwraps parentheses, `!`, `!!` and `as` in any combination; the safe + cast `as?` is deliberately left to the `AsQMARK` token that already + counts it, so the two cast spellings come out level rather than the + safe one scoring twice. In C#, `??` was a cyclomatic decision that no + ABC arm counted, so `when b ?? false` scored one fewer condition than + `when x > 2`, `when E(x)` or `when x is int` — the spelling-dependence + #1422 exists to remove. It joins the condition tokens, matching the + JS/TS family, which has always counted it. `??=` is unchanged and + remains an assignment, as in the JS family. **Metric drift:** Kotlin + `abc.conditions` rises by one per `!!` or `as` cast standing as a + predicate or a `&&` / `||` operand; C# `abc.conditions` rises by one + per `??` anywhere. + - **C# comparison-operator overloads no longer score a spurious condition** (#1420). C# overloads six comparison operators and #1297 gated only `<` and `>`; the other four are distinct tokens that diff --git a/big-code-analysis-ast/src/macros/kind_sets.rs b/big-code-analysis-ast/src/macros/kind_sets.rs index 3de830fb..09493e1d 100644 --- a/big-code-analysis-ast/src/macros/kind_sets.rs +++ b/big-code-analysis-ast/src/macros/kind_sets.rs @@ -154,8 +154,22 @@ macro_rules! java_bool_terminal_kinds { // `cast_expression` inside `parenthesized_expression`). The set bundles // the bool-evaluating terminals added by #372 (`FieldAccess`, // `CastExpression`, `ParenthesizedTypeCast`, `InstanceofExpression`); -// the dekobon Groovy grammar has no `await` or `array_access` -// analogues, so those collapse out of the C# set. +// the dekobon Groovy grammar has no `await` analogue, so that one +// collapses out of the C# set. +// +// It DOES have an indexing analogue, and four navigation kinds beside +// it — `subscript_expression`, `safe_subscript_expression`, +// `safe_navigation_expression`, `safe_chain_dot_expression` and +// `direct_field_access_expression`, all alternatives of `_expression` +// and all legal in a boolean slot. None is listed, so `if (l[0])`, +// `if (a?.b)` and `if (a.@b)` score zero where `if (a)` scores one, +// while C# scores `l[0]` through `ElementAccessExpression` and Kotlin +// scores both through `IndexExpression` / `NavigationExpression`. +// `a?.b` is the worst of them: Groovy cyclomatic counts `?.` as a +// decision, so ABC sits two below its own decision count on an +// idiomatic predicate. Tracked separately — an earlier revision of +// this comment claimed the analogue did not exist, which is the sort +// of claim that stops the next reader looking. // // Groovy truth makes every non-zero number truthy, so `NumberLiteral` // is a unary condition here for the same reason Python's `Integer` / diff --git a/src/metrics/abc.rs b/src/metrics/abc.rs index 626f13a7..936edd44 100644 --- a/src/metrics/abc.rs +++ b/src/metrics/abc.rs @@ -581,6 +581,9 @@ mod tests { // nothing. #[track_caller] fn assert_fixture_spells(src: &str, path: &str, kinds: &[(u16, usize, &str)]) { + // An empty list would make every following assertion vacuous, + // which is the anchor's own failure mode rather than a caller's. + assert!(!kinds.is_empty(), "anchor asserted nothing"); let parser = P::new(src.as_bytes().to_vec(), std::path::Path::new(path), None); for (kind, want, spelling) in kinds { let found = parser @@ -4501,6 +4504,23 @@ mod tests { // `csharp_inspect_container`, since `when_clause` wraps a // parenthesised guard in a real `parenthesized_expression` rather // than the anonymous parens `catch_filter_clause` uses. + // + // `nullc` is #1459's row and the counter-example that falsified the + // claim above when it was first written. A `??` guard is a + // `binary_expression`, which the slot declines — it leaves an + // operator guard to the arm that already counts the operator — and + // no ABC arm counted `??`, so this one spelling read 2 where the + // other four read 3. Counting the token levels it without touching + // the slot; an unconditional `+1` in the slot instead would have + // taken `cmp` to 4, re-creating the double count #1422 removed. + // + // It is the one guarded member whose cyclomatic is not 4, and that + // is not a discrepancy to fix: C# cyclomatic counts `??` as a + // decision *in addition to* the guard clause, so the arm below + // narrows the ABC-versus-cyclomatic gap from two to one rather than + // closing it. The remaining one is the slot's standing policy of + // leaving a `binary_expression` to its operators, and `cmp` pays it + // too — it just happens to break even there. #[test] fn csharp_switch_arm_guard_scores_one_condition_however_spelled() { let src = "class A { @@ -4509,13 +4529,15 @@ mod tests { int cmp(int x) => x switch { > 0 when x > 2 => 1, > 0 => 2, _ => 3 }; int call(int x) => x switch { > 0 when IsEven(x) => 1, > 0 => 2, _ => 3 }; int paren(int x) => x switch { > 0 when (IsEven(x)) => 1, > 0 => 2, _ => 3 }; + int nullc(int x, bool? b) => x switch { > 0 when b ?? false => 1, > 0 => 2, _ => 3 }; int none(int x) => x switch { > 2 => 1, > 0 => 2, _ => 3 }; }"; assert_csharp_fixture_spells( src, &[ - (Csharp::WhenClause as u16, 4, "`when` guards"), + (Csharp::WhenClause as u16, 5, "`when` guards"), (Csharp::EQEQ as u16, 1, "`tok`'s `==`"), + (Csharp::QMARKQMARK as u16, 1, "`nullc`'s `??`"), ( Csharp::ParenthesizedExpression as u16, 1, @@ -4541,6 +4563,10 @@ mod tests { ("cmp", 3, 4), ("call", 3, 4), ("paren", 3, 4), + // Was 2 — the guard spelling nothing counted. + // Cyclomatic 5 rather than 4 because `??` is a + // decision there on top of the guard clause. + ("nullc", 3, 5), ("none", 2, 3), ], ); @@ -4861,24 +4887,55 @@ mod tests { // // This test is what stops a later "make all three languages // consistent" pass from flipping C# to a `ConditionalExpression` - // allowlist: that would silently drop both counts here to 0 while - // every other ABC test still passed. `??` is deliberately absent - // from the expected total — C# ABC does not list `QMARKQMARK` as a - // condition (it does in the TS family), which is pre-existing and - // out of scope for #1275. + // allowlist: that would silently drop both `?` counts here while + // every other ABC test still passed. The third condition is the + // `??`, which #1459 added to close the gap this comment used to + // record as out of scope for #1275 — so a flipped `QMARK` polarity + // now reads 1 rather than 0, and the discrimination is unchanged. #[test] fn csharp_conditional_access_still_counts_as_a_condition() { - check_metrics::( - "class A { + let src = "class A { object M(string s, int[] xs) { return s?.Length ?? xs?[0]; } - }", - "foo.cs", - |metric| { - assert_eq!(metric.abc.conditions_sum(), 2); - }, + }"; + // The total is now fed by two token arms rather than one, so it + // carries the per-source claim only alongside these counts: 3 is + // two bare `?` plus one `??`, and without them a `QMARK` arm + // that had stopped counting `a?[0]` while `??` gained a second + // count elsewhere would still read 3. + assert_csharp_fixture_spells( + src, + &[ + (Csharp::QMARK as u16, 2, "the `?.` and `?[` operators"), + (Csharp::QMARKQMARK as u16, 1, "the `??`"), + ], ); + check_metrics::(src, "foo.cs", |metric| { + assert_eq!(metric.abc.conditions_sum(), 3); + }); + } + + // The `QMARKQMARK` arm away from any guard, where nothing else can + // supply the count (#1459). `coalesce`'s body holds no comparison, + // no call and no control-flow keyword, so its one condition is the + // `??` and nothing else; it read 0 before the arm while C# + // cyclomatic scored the decision, which is the direction ABC is not + // allowed to sit on. + // + // `plain` is the control that keeps this from being an assertion + // about `return` or about the method shape: same body without the + // operator, 0 conditions and 1 decision. + #[test] + fn csharp_null_coalescing_is_a_condition() { + let src = "class A { + int coalesce(int? x) { return x ?? 0; } + int plain(int x) { return x; } + }"; + assert_csharp_fixture_spells(src, &[(Csharp::QMARKQMARK as u16, 1, "the `??`")]); + check_func_space::(src, "foo.cs", |space| { + assert_members_score(&space.spaces[0], &[("coalesce", 1, 2), ("plain", 0, 1)]); + }); } // The other direction of #1275's C# gate, isolated: narrowing @@ -5739,6 +5796,113 @@ function f(int $a, int $b): int { ); } + // #1459, the regression #1421 left behind. Turning the `when` + // entry's blanket count into a condition slot moved the payment from + // the entry to the condition — so every shape the slot cannot + // classify went from 1 to 0, silently. Two shapes did: + // + // - `a!!`. `unary_expression` is one kind for *both* Kotlin unary + // spellings, and the slot routed it to the peel, so this reads as + // handled at every call site. The peel then looked for the operand + // at child(1) — right for the prefix `!x`, wrong for a postfix + // `x!!`, whose child(1) is the `!!` token — and stopped. + // - `a as Boolean`. `as_expression` was not a wrapper the peel knew + // at all, so the slot fell off the end of its `else if`. + // + // The controls are `bare` / `cmp` / `call`, one per surviving path + // (terminal, comparison token, invocation): a fix that moved the + // entry count back would take those to 2 and fail here, which is + // what distinguishes repairing the peel from reverting #1421. + // + // `safe` is the §5 row and the one measurement decided rather than + // taste. `as?` is already a condition *token* + // (`src/metrics/abc/kotlin.rs`), so a peel that treated `as` and + // `as?` alike would score it 2 — the double count that arm's own + // fix was avoiding. `kotlin_wrapper_operand` excludes `as?` for + // that reason, and this member is what fails if the exclusion goes: + // `(a as? Boolean)!!` is valid Kotlin whose peel reaches the cast + // through two other wrappers, so it needs no invalid fixture to + // discriminate. + // + // `iff` carries the same `a!!` through the `if` slot. That half is + // not #1421 fallout — `if (a!!)` has read 0 since the slot landed in + // #773 — but it is the same peel and the same edit, and a fix + // narrowed to `when` entries would leave it at 0. + // + // Every member is valid Kotlin: `a!!` on a `Boolean?`, `as` on an + // `Any`, and `(a as? Boolean)!!` back to a `Boolean`. The kind + // anchors are what stop a later edit from trimming a spelling out + // and turning its member into a silent copy of `bare`. + #[test] + fn kotlin_condition_slot_peels_null_assertions_and_casts() { + let src = "class K { + fun g(x: Boolean): Boolean = true + fun bare(a: Boolean): Int = when { a -> 1; else -> 0 } + fun cmp(a: Int): Int = when { a > 5 -> 1; else -> 0 } + fun call(a: Boolean): Int = when { g(a) -> 1; else -> 0 } + fun bang(a: Boolean?): Int = when { a!! -> 1; else -> 0 } + fun paren(a: Boolean?): Int = when { (a!!) -> 1; else -> 0 } + fun cast(a: Any): Int = when { a as Boolean -> 1; else -> 0 } + fun chain(a: Any?): Int = when { a!! as Boolean -> 1; else -> 0 } + fun safe(a: Any): Int = when { (a as? Boolean)!! -> 1; else -> 0 } + fun cmt(a: Boolean): Int = when { ! /*c*/ a -> 1; else -> 0 } + fun iff(a: Boolean?): Int { if (a!!) { return 1 }; return 0 } + }"; + assert_kotlin_class_members( + src, + &[ + (Kotlin::WhenSubject as u16, 0, "`when` subjects"), + ( + Kotlin::BANGBANG as u16, + 5, + "the `!!` null assertions of `bang` / `paren` / `chain` / `safe` / `iff`", + ), + ( + Kotlin::AsExpression as u16, + 3, + "the casts of `cast` / `chain` / `safe`", + ), + (Kotlin::AsQMARK as u16, 1, "`safe`'s safe cast"), + ( + Kotlin::BlockComment as u16, + 1, + "`cmt`'s interposed comment, spaces and all", + ), + ( + Kotlin::ParenthesizedExpression as u16, + 2, + "the parenthesised operands of `paren` / `safe`", + ), + ], + &[ + // (member, abc.conditions, cyclomatic) + ("g", 0, 1), + // The three controls, unchanged by this fix. + ("bare", 1, 2), + ("cmp", 1, 2), + ("call", 1, 2), + // Each was 0: the peel stopped on the wrapper. + ("bang", 1, 2), + ("paren", 1, 2), + ("cast", 1, 2), + ("chain", 1, 2), + // 1, not 2 — the `as?` token owns this one, and the + // peel declines to charge for it a second time. + ("safe", 1, 2), + // The peel reads its operand through the `operator` / + // `argument` fields, so an `extra` between the two no + // longer displaces it. The spacing is load-bearing and + // must not be tidied away: `!/*c*/a` unspaced parses + // with the comment somewhere a `child(1)` read still + // skips, so only the spaced spelling can tell the field + // read from the positional one. Measured both ways. + ("cmt", 1, 2), + // The `if` slot, which has read 0 since #773. + ("iff", 1, 2), + ], + ); + } + // The other half of #1421: a subject-ful entry must not move. Its // condition is a *pattern* matched against the subject, not an // independent boolean expression, so the implicit `subject == @@ -6277,6 +6441,62 @@ function f(int $a, int $b): int { ); } + // `kotlin_inspect_container` has two callers, and #1459's fixture + // members all reach it through the *slot* + // (`kotlin_count_condition`). This is the other one: the `&&` / `||` + // walker, which routes any named non-terminal operand through the + // same peel. The two wrappers #1459 taught it therefore have a + // second, structurally independent path into the count, and a + // fixture covering only the slot leaves it untested + // (`.claude/rules/grammar-dispatch.md` §11). + // + // Each chain is one identifier plus one wrapped operand, so 2 is + // "the walker peeled the wrapper" and 1 is "it gave up on it" — + // which is what both lines scored before #1459. The bare `a && b` + // control is `kotlin_unary_conditions_in_chain`'s shape at 2, so a + // regression cannot be read as the chain itself changing. + #[test] + fn kotlin_chain_operands_peel_null_assertions_and_casts() { + // Anchored per row, because the assertion alone cannot tell the + // wrapper from its operand: `a && b` scores the same 2 / 3 as + // every row here, so trimming the `!!` or the cast out of a + // fixture would leave this green with its subject gone + // (`.claude/rules/testing.md`, "Perturb the fixture as well as + // the production line"). + for (chain, spelling, anchors) in [ + ( + "a && b!!", + "null assertion", + &[(Kotlin::BANGBANG as u16, 1usize, "the `!!`")][..], + ), + ( + "a && b as Boolean", + "cast", + &[(Kotlin::AsExpression as u16, 1, "the `as` cast")][..], + ), + ( + "a && (b!!)", + "parenthesised null assertion", + &[ + (Kotlin::BANGBANG as u16, 1, "the `!!`"), + (Kotlin::ParenthesizedExpression as u16, 1, "the parens"), + ][..], + ), + ] { + let src = format!("fun f(a: Boolean, b: Any) {{ if ({chain}) {{ println(\"x\") }} }}"); + assert_fixture_spells::(&src, "foo.kt", anchors); + check_func_space::(&src, "foo.kt", |space| { + let f = child_space(&space, "f"); + assert_eq!( + f.metrics.abc.conditions(), + 2, + "`{chain}`: the identifier plus the {spelling} operand" + ); + assert_eq!(f.metrics.cyclomatic.cyclomatic(), 3, "`{chain}`: decisions"); + }); + } + } + #[test] fn kotlin_bare_if_predicate_is_one_condition() { // Issue #773: a bare-boolean `if` predicate (`if (flag)`) is one @@ -13446,7 +13666,7 @@ mod numeric_bool_operands { /// is what makes a `conditions` move unambiguously ABC's. The recorded /// figures differ per slot (Rust's chain slot carries the extra `&&` /// decision), so each slot pins its own pair rather than sharing one. -// Gated on the union of the four languages with rows, so a build +// Gated on the union of the three languages with rows, so a build // enabling none of them drops the module instead of failing its // guards (`.claude/rules/testing.md`, #1286 / #1411). #[cfg(test)] diff --git a/src/metrics/abc/csharp.rs b/src/metrics/abc/csharp.rs index c4017909..36dbbed4 100644 --- a/src/metrics/abc/csharp.rs +++ b/src/metrics/abc/csharp.rs @@ -300,7 +300,30 @@ fn csharp_count_token_condition<'a>( // 3 == 4 - 1. Both arms have to move together. `goto default;` // costs nothing already, because `Default` is excluded as the // switch's unconditional fallthrough. - Else | Case | Try | Catch => { + // + // `QMARKQMARK` joined them in #1459 and is ungated for the same + // reason: `??` comes from `binary_expression` alone. It is a + // decision — `a ?? b` evaluates `b` only when `a` is null, the + // `a != null ? a : b` the language lets you not spell — and C# + // cyclomatic has always counted it + // (`src/metrics/cyclomatic/csharp.rs`), so ABC sat one *below* + // C#'s own decision count wherever a `??` appeared. + // + // #1422 made that gap visible rather than merely present. Its + // guard slot claims every spelling of a `when` guard is worth + // one condition, and `when b ?? false` was worth zero: the slot + // sees a `binary_expression`, which it leaves to the operator + // arms because a comparison guard's operator is already counted + // here — and for `??` there was nothing to leave it to. Counting + // the token is what levels the spellings *and* keeps a compound + // guard (`when a > 1 && b < 2`) at its two conditions; a blanket + // `+1` on the slot would have done neither. + // + // No double count (§5): `??` is one token, distinct from the + // bare `QMARK` below and from the `??=` compound assignment + // (`QMARKQMARKEQ`, counted as an assignment), and every + // condition slot declines a `binary_expression` outright. + Else | Case | Try | Catch | QMARKQMARK => { stats.conditions += 1.; } // All six C# comparison tokens, counted only where they *apply* @@ -399,10 +422,15 @@ fn csharp_count_token_condition<'a>( // ABC *below* C#'s own cyclomatic decision count on a safe- // navigation chain. Denying the two type-syntax parents keeps // that count without an allowlist entry a later "consistency" - // pass could drop. It is agreement on this one token, not on - // the metric: `??` is a C# cyclomatic decision (`QMARKQMARK` - // there) and no ABC condition here, a pre-existing gap the - // JS-family arms do not share. + // pass could drop. The sibling `??` gap this comment used to + // record — a C# cyclomatic decision that was no ABC condition — + // is closed for `??` by `QMARKQMARK` joining the ungated + // condition-token arm at the top of this match (#1459). It is + // *not* closed for `??=`: `QMARKQMARKEQ` is a cyclomatic + // decision and stays an ABC assignment only, so `a ??= b` still + // sits one below C#'s decision count. That is deliberate rather + // than missed — the JS family scores `??=` the same way — but it + // is a live divergence, not a closed one. // // The agreement being protected is C#-internal, not cross- // language: `a?.b?.c` scores ABC conditions 2 in C# and 0 in diff --git a/src/metrics/abc/kotlin.rs b/src/metrics/abc/kotlin.rs index 8837f91e..e7856cc2 100644 --- a/src/metrics/abc/kotlin.rs +++ b/src/metrics/abc/kotlin.rs @@ -20,8 +20,15 @@ use crate::*; // `else` / `when`-entry / `catch` arms. Compared with the Java impl we // stay token-level (matching the leaf kind_ids) rather than walking // `Modifiers` children; the Kotlin grammar exposes the relevant -// operators directly as token nodes inside `binary_expression`, -// `assignment`, `prefix_expression`, and `postfix_expression`. +// operators directly as token nodes inside `binary_expression` and +// `assignment`. +// +// It does *not* have `prefix_expression` / `postfix_expression`, which +// this comment named until #1459. tree-sitter-kotlin-ng spells both +// unary positions as one `unary_expression` told apart only by its +// `operator` field — and believing the two-production version is how +// `kotlin_inspect_container` came to handle the prefix `!` and drop the +// postfix `!!` while reading as though it covered both. // Returns true when this `=` token initialises an *immutable* (`val`) // binding, whose initialiser is part of the declaration and therefore not @@ -47,18 +54,89 @@ fn kotlin_eq_initializes_immutable_binding<'a>( parent.children().any(|child| child.kind_id() == Val) } +// The operand a transparent wrapper wraps, and whether the wrapper +// itself proves that operand is boolean. `None` means the node is not a +// wrapper and the peel stops there. +// +// Only the prefix `!` proves booleanness: `!x` is boolean whatever `x` +// is, while parentheses, a null assertion and a cast are all +// type-preserving (`a!!` and `a as T` are boolean exactly when the slot +// they sit in is). So they inherit the caller's verdict rather than +// setting it. +// +// Two of the four shapes were not here from the start, and both were +// #1459 fallout from #1421 turning the `when` entry's blanket count +// into a condition slot: before that, `when { a!! -> … }` was paid for +// by the entry, and after it by this peel, which scored zero. +// +// - `unary_expression` is one kind for *both* Kotlin unary spellings, +// and the peel recognised only the prefix one. A postfix `a!!` stores +// its operand *before* the token, so the positional read for `!x` +// found the `!!` and stopped on a node the slot had already routed +// here as handled. +// - `as_expression` was not recognised at all, so a cast fell off the +// end of the slot's `else if`. +// +// By field, not index (`.claude/rules/grammar-dispatch.md` §3): both +// kinds name their parts (`operator` / `argument`, `left` / `right`), so +// one read serves the prefix and postfix spellings alike instead of the +// per-spelling index that produced the defect, and it survives a grammar +// re-order. It also survives an interposed `extra`: `when { ! /*c*/ a }` +// scores its condition, where the positional read scored zero. +// +// `parenthesized_expression` names nothing — its only child in +// node-types.json is the unlabelled inner `expression` — so it keeps the +// positional read it has always had, and with it the comment bug: +// `when { ( /*c*/ a) -> … }` still scores zero, because child(1) is the +// comment. Measured, not assumed. That is the same class as the C# +// `if` / `while` / `do` slots tracked in #1455 and left to their own +// change there; it predates #1459 and is recorded here rather than +// widened into it. +fn kotlin_wrapper_operand<'a>(node: &Node<'a>) -> Option<(Node<'a>, bool)> { + use Kotlin::*; + + match node.kind_id().into() { + // `(expr)` — the inner expression follows the `(` token. + ParenthesizedExpression => Some((node.child(1)?, false)), + UnaryExpression => { + let operand = node.child_by_field_name("argument")?; + match node.child_by_field_name("operator")?.kind_id().into() { + BANG => Some((operand, true)), + BANGBANG => Some((operand, false)), + // `-x`, `x++` and friends: arithmetic, never a boolean + // slot's operand, so the peel declines rather than + // reaching a bare `identifier` and counting it. + _ => None, + } + } + // `x as T` — `left` is the operand, `right` the target type. + // + // The *safe* cast `x as? T` is excluded on purpose (§5, one kind + // per operator): `AsQMARK` is already a condition token below, so + // peeling through it too would score `when { a as? T -> … }` two + // where every other spelling scores one. The plain `as` has no + // such token — `As` is not in that arm — so the peel is the only + // thing that can count it, and the two spellings come out level. + AsExpression if !node.is_child(AsQMARK as u16) => { + Some((node.child_by_field_name("left")?, false)) + } + _ => None, + } +} + // Kotlin ABC unary-conditional walker (Fitzpatrick Rule 9; issue #557). // tree-sitter-kotlin-ng parses `a && b || c` as a left-nested chain of // flat `binary_expression` nodes carrying `&&` / `||` operator tokens, -// the same shape as the Java template. Negation surfaces as -// `unary_expression` whose child(0) is the `!` token; the condition slot -// may also be wrapped in `parenthesized_expression`. Both are unwrapped -// by `kotlin_inspect_container` to reach the inner bare operand. +// the same shape as the Java template. Negation surfaces as a +// `unary_expression` whose `operator` field is the `!` token; the +// condition slot may also be wrapped in `parenthesized_expression`, a +// postfix `!!` null assertion, or an `as` cast. All are unwrapped by +// `kotlin_inspect_container` to reach the inner bare operand, and they +// chain: `(a!! as Boolean)` peels three wrappers to one `identifier`. fn kotlin_inspect_container(container_node: &Node, parent: &Node, conditions: &mut f64) { use Kotlin::*; let mut node = *container_node; - let mut node_kind = node.kind_id().into(); // A parenthesised / negated operand only contributes when it sits in // a boolean-evaluating slot. The chain wrapper (`binary_expression`) // and the control-flow headers all qualify; a `!`-operator anywhere @@ -76,26 +154,11 @@ fn kotlin_inspect_container(container_node: &Node, parent: &Node, conditions: &m | WhenEntry ); - loop { - let is_parens = matches!(node_kind, ParenthesizedExpression); - let is_not = matches!(node_kind, UnaryExpression) - && node.child(0).is_some_and(|c| c.kind_id() == BANG as u16); - - if !is_parens && !is_not { - break; - } - if !has_boolean_content && is_not { - has_boolean_content = true; - } + while let Some((operand, proves_boolean)) = kotlin_wrapper_operand(&node) { + has_boolean_content |= proves_boolean; + node = operand; - // Parenthesised expressions wrap their inner expression at child - // index 1 (after the `(` token); a `!` unary stores its operand - // at index 1 (after the `!` token). - let Some(child) = node.child(1) else { break }; - node = child; - node_kind = node.kind_id().into(); - - if matches!(node_kind, kotlin_bool_terminal_kinds!()) { + if matches!(node.kind_id().into(), kotlin_bool_terminal_kinds!()) { if has_boolean_content { *conditions += 1.; } @@ -150,17 +213,23 @@ fn kotlin_count_unary_conditions(list_node: &Node, conditions: &mut f64) { // comparison or boolean chain (`if (a == b)`, `if (a && b)`) is a nested // `binary_expression` already counted by the comparison-token and // `&&`/`||` walker arms, so it adds -// nothing here. A parenthesised or negated predicate (`if ((flag))`, -// `if (!flag)`) is unwrapped by `kotlin_inspect_container`. Without this +// nothing here. A predicate wrapped in parentheses, a negation, a +// postfix `!!` null assertion or an `as` cast (`if ((flag))`, +// `if (!flag)`, `if (flag!!)`, `if (v as Boolean)`) is unwrapped by +// `kotlin_inspect_container`. Without this // arm, idiomatic Kotlin bare predicates reported 0 ABC conditions while // Kotlin's own cyclomatic counted the decision, breaking the // conditions >= decisions invariant (#469/#473/#456/#696); issue #773. fn kotlin_count_condition(condition: &Node, parent: &Node, conditions: &mut f64) { - use Kotlin::*; - let kind = condition.kind_id().into(); - if matches!(kind, kotlin_bool_terminal_kinds!()) { + if matches!(condition.kind_id().into(), kotlin_bool_terminal_kinds!()) { *conditions += 1.; - } else if matches!(kind, ParenthesizedExpression | UnaryExpression) { + } else if kotlin_wrapper_operand(condition).is_some() { + // Asking the peel itself which kinds it unwraps, rather than + // restating the list here. The two spelled the list separately + // until #1459, and that is how `unary_expression` came to be + // routed here while the peel handled only half of it — the slot + // read as covering a shape the peel dropped on the floor + // (`.claude/rules/grammar-dispatch.md` §7). kotlin_inspect_container(condition, parent, conditions); } } diff --git a/tests/repositories/big-code-analysis-output b/tests/repositories/big-code-analysis-output index ee1bd859..81abf593 160000 --- a/tests/repositories/big-code-analysis-output +++ b/tests/repositories/big-code-analysis-output @@ -1 +1 @@ -Subproject commit ee1bd8591b0d66ec83377a522291caf4a0d8db7b +Subproject commit 81abf5931c9d54dfc43a109149209d14cdf60cac