From 8ca55da71208a62c09bfacfef9e9c152690dc680 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic <490392+dekobon@users.noreply.github.com> Date: Wed, 7 Oct 2026 21:51:03 -0700 Subject: [PATCH 01/17] fix(abc/cpp): gate all comparison tokens on parent C++ and Mozcpp counted `<=` `>=` `==` `!=` `<=>` in an ungated arm, so declaring `operator<=`, `operator>=`, `operator==`, `operator!=` or `operator<=>` scored one spurious ABC condition per overload, while `<` / `>` were already gated on a `binary_expression` parent. This is the C++ instance of the C# defect fixed in #1420. All seven tokens now share the `binary_expression` allowlist. A grammar.json sweep of tree-sitter-cpp 0.23.4 and the vendored mozcpp grammar is recorded at the arm: `#if A <= B` is aliased onto `binary_expression` and keeps counting; `operator_name` and fold expressions do not count, consistent with `<` / `>`. C and ObjC emit these tokens only from comparisons and have no overloading, so their arms stay as they are, with a note saying why. Corpus snapshots move only in openfst operator==/operator!= definitions and declarations, all downward. Fixes #1448 --- src/metrics/abc.rs | 180 ++++++++++++++++++++ src/metrics/abc/c.rs | 6 + src/metrics/abc/cpp.rs | 53 ++++-- src/metrics/abc/mozcpp.rs | 22 +-- src/metrics/abc/objc.rs | 5 + tests/repositories/big-code-analysis-output | 2 +- 6 files changed, 241 insertions(+), 27 deletions(-) diff --git a/src/metrics/abc.rs b/src/metrics/abc.rs index 5ce5838f0..deff0429a 100644 --- a/src/metrics/abc.rs +++ b/src/metrics/abc.rs @@ -4533,6 +4533,186 @@ mod tests { }); } + // #1448, the C++ / Mozcpp instance of #1420. C++ overloads seven + // comparison operators; before the fix only `<` / `>` were gated on a + // `binary_expression` parent, so `operator<=` `>=` `==` `!=` and the + // spaceship `operator<=>` each scored one spurious condition on a + // struct where every member has `cyclomatic()` 1. + // + // Members are looked up by name, so a partial fix fails on the + // spelling it missed rather than on a total that merely moved. `n` + // is the §11 control: the same seven tokens applied, which must + // keep scoring one condition each. The census anchors both halves — + // trimming an overload or an application out of the fixture fails + // by name instead of decaying into its counterpart. + #[cfg(any(feature = "cpp", feature = "mozcpp"))] + const CPP_COMPARISON_OVERLOADS: &str = "struct V { + bool operator<(const V& o) const { return true; } + bool operator>(const V& o) const { return true; } + bool operator<=(const V& o) const { return true; } + bool operator>=(const V& o) const { return true; } + bool operator==(const V& o) const { return true; } + bool operator!=(const V& o) const { return true; } + auto operator<=>(const V& o) const { return 0; } + }; + int n(int a, int b) { + return (a < b) + (a > b) + (a <= b) + (a >= b) + (a == b) + (a != b) + (a <=> b); + }"; + + // `operator_name` id, then the seven token ids in source order, for + // whichever of the two grammars `P` parses. + #[cfg(any(feature = "cpp", feature = "mozcpp"))] + fn assert_cpp_comparison_overloads_are_not_conditions( + operator_name: u16, + tokens: [u16; 7], + ) { + #[cfg(any(feature = "cpp", feature = "mozcpp"))] + const SPELLINGS: [&str; 7] = ["<", ">", "<=", ">=", "==", "!=", "<=>"]; + let mut census = vec![(operator_name, 7, "operator names")]; + // Two of each token: one declaring, one applying. + census.extend(tokens.iter().zip(SPELLINGS).map(|(&id, s)| (id, 2, s))); + assert_fixture_spells::

(CPP_COMPARISON_OVERLOADS, "foo.cpp", &census); + check_func_space::(CPP_COMPARISON_OVERLOADS, "foo.cpp", |space| { + let class = child_space(&space, "V"); + assert_eq!(class.spaces.len(), 7, "seven overloads"); + for spelling in SPELLINGS { + let member = child_space(class, &format!("operator{spelling}")); + 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" + ); + } + assert_eq!( + child_space(&space, "n").metrics.abc.conditions(), + 7, + "`n` applies all seven tokens: one condition each" + ); + }); + } + + #[cfg(feature = "cpp")] + #[test] + fn cpp_comparison_operator_overloads_are_not_conditions() { + assert_cpp_comparison_overloads_are_not_conditions::( + Cpp::OperatorName as u16, + [ + Cpp::LT, + Cpp::GT, + Cpp::LTEQ, + Cpp::GTEQ, + Cpp::EQEQ, + Cpp::BANGEQ, + Cpp::LTEQGT, + ] + .map(|k| k as u16), + ); + } + + // Mozcpp owns no file extension, so this is its only coverage of the + // arm: no integration snapshot routes to it. + #[cfg(feature = "mozcpp")] + #[test] + fn mozcpp_comparison_operator_overloads_are_not_conditions() { + use crate::Mozcpp; + assert_cpp_comparison_overloads_are_not_conditions::( + Mozcpp::OperatorName as u16, + [ + Mozcpp::LT, + Mozcpp::GT, + Mozcpp::LTEQ, + Mozcpp::GTEQ, + Mozcpp::EQEQ, + Mozcpp::BANGEQ, + Mozcpp::LTEQGT, + ] + .map(|k| k as u16), + ); + } + + // The two other productions #1448's allowlist decides, alongside the + // operator overloads above. + // + // `#if A <= B` is a real decision and must keep counting: the grammar + // aliases `preproc_binary_expression` onto `binary_expression`, and + // the allowlist is the only thing that admits it now that `<=` / `!=` + // are gated. The fixture's two directives are the root's only + // conditions. + // + // A fold over a comparison operator does not count, for all six + // foldable tokens alike (`<` / `>` never did): before the fix `le` + // scored 1 and the binary fold `eq` scored 2, one per spelling of its + // single operator, against `lt`'s 0. `idiom` is the control — its + // `==` sits in a `binary_expression` inside the fold and counts. + // + // The census also pins §2: every `binary_expression` here carries the + // `BinaryExpression2` id, and `BinaryExpression` — the pre-alias + // symbol the arm lists defensively — is never emitted. + #[cfg(any(feature = "cpp", feature = "mozcpp"))] + fn assert_cpp_comparison_folds_and_preproc_operands( + binary_expression: u16, + binary_expression2: u16, + fold_expression: u16, + ) { + let src = "#if A <= B + #endif + #if C != D + #endif + template bool lt(T... a) { return (a < ...); } + template bool le(T... a) { return (a <= ...); } + template bool eq(T... a) { return (0 == ... == a); } + template bool idiom(T... a) { return ((a == 0) && ...); }"; + assert_fixture_spells::

( + src, + "foo.cpp", + &[ + (fold_expression, 4, "fold expressions"), + (binary_expression2, 3, "binary expressions"), + (binary_expression, 0, "pre-alias binary expressions"), + ], + ); + check_func_space::(src, "foo.cpp", |space| { + for (name, want) in [("lt", 0), ("le", 0), ("eq", 0), ("idiom", 1)] { + assert_eq!( + child_space(&space, name).metrics.abc.conditions(), + want, + "fold in `{name}`" + ); + } + assert_eq!( + space.metrics.abc.conditions(), + 2, + "one condition per `#if` comparison" + ); + }); + } + + #[cfg(feature = "cpp")] + #[test] + fn cpp_comparison_folds_and_preproc_operands() { + assert_cpp_comparison_folds_and_preproc_operands::( + Cpp::BinaryExpression as u16, + Cpp::BinaryExpression2 as u16, + Cpp::FoldExpression as u16, + ); + } + + #[cfg(feature = "mozcpp")] + #[test] + fn mozcpp_comparison_folds_and_preproc_operands() { + use crate::Mozcpp; + assert_cpp_comparison_folds_and_preproc_operands::( + Mozcpp::BinaryExpression as u16, + Mozcpp::BinaryExpression2 as u16, + Mozcpp::FoldExpression as u16, + ); + } + // #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`. diff --git a/src/metrics/abc/c.rs b/src/metrics/abc/c.rs index aa311ec6a..f156b7341 100644 --- a/src/metrics/abc/c.rs +++ b/src/metrics/abc/c.rs @@ -68,6 +68,12 @@ impl Abc for CCode { // counterpart is Rule 9). See the module-level `Stats` // doc-comment for the cross-language policy (issue // #395, walker tracked in #403). + // + // Unlike C++ (#1448), `<=` `>=` `==` `!=` need no parent + // gate: tree-sitter-c 0.24.2's `grammar.json` emits them + // only from `binary_expression` and + // `preproc_binary_expression`, both comparisons, and C has + // no operator overloading to declare them elsewhere. LTEQ | GTEQ | EQEQ | BANGEQ | Else | Case | QMARK => { stats.conditions += 1.; } diff --git a/src/metrics/abc/cpp.rs b/src/metrics/abc/cpp.rs index f7983815e..4f22d8f60 100644 --- a/src/metrics/abc/cpp.rs +++ b/src/metrics/abc/cpp.rs @@ -234,9 +234,6 @@ impl Abc for CppCode { CallExpression | CallExpression2 | NewExpression => { stats.branches += 1.; } - // Comparison operators emitted as token children of a - // `binary_expression`. The C++20 spaceship `<=>` (`LTEQGT`) - // is a comparison operator and counts once per use. // `else` opens an alternative branch path; `case` // (non-default) adds one per switch arm; `?` opens a // ternary; `try` / `catch` count per Fitzpatrick (and @@ -249,18 +246,48 @@ impl Abc for CppCode { // counterpart is Rule 9). See the module-level `Stats` // doc-comment for the cross-language policy (issue // #395, walker tracked in #403). - LTEQ | GTEQ | EQEQ | BANGEQ | LTEQGT | Else | Case | QMARK | Try | Try2 | Catch => { + Else | Case | QMARK | Try | Try2 | Catch => { stats.conditions += 1.; } - // Plain `<` / `>` doubles as template-argument and - // template-parameter delimiter (`std::vector`, - // `template `). The `binary_expression` parent - // check disambiguates without inspecting siblings — only - // comparison uses of `<` / `>` count. Both kind-id aliases - // (`BinaryExpression`, `BinaryExpression2`) are accepted - // because the C++ grammar emits the same node under two - // production-rule paths. - LT | GT + // The seven comparison tokens, counted only when applied — + // their parent is a `binary_expression`. A `grammar.json` + // sweep of tree-sitter-cpp 0.23.4 (and the vendored + // tree-sitter-mozcpp) finds each of them in these + // productions: + // + // - `binary_expression` — a comparison; counts. + // - `preproc_binary_expression` — `#if A <= B`, legal C++ + // and a real decision. The grammar aliases it onto + // `binary_expression`, so it counts through the same + // allowlist. + // - `operator_name` — `bool operator<=(…)` *declares* the + // operator rather than applying it. Each of the seven + // overloads has `cyclomatic()` 1; before #1448 only + // `<` / `>` were gated, so the other five scored one + // condition each (#1420 is the C# instance). + // - `_fold_operator` / `_binary_fold_operator` (`<` `>` `<=` + // `>=` `==` `!=` only) — the parent is `fold_expression`. + // Not counted, as `<` / `>` never were: a binary fold + // `(0 == ... == a)` spells its one operator twice, and a + // comparison fold is a chained comparison + // (`a1 < (a2 < a3)`). The idiomatic spelling + // `((a == 0) && ...)` keeps its `==` inside a + // `binary_expression` and still counts. + // - `template_argument_list` / `template_parameter_list` / + // `system_lib_string` (`<` `>` only) — delimiters + // (`std::vector`, `#include `). + // + // Allowlist polarity so a grammar bump adding a production + // fails closed (`.claude/rules/grammar-dispatch.md` §1). A + // token reparented under `{ERROR}` by recovery stops + // counting too, as `<` / `>` always have. + // + // At this pin every `binary_expression`, `#if` operand or + // not, carries kind id `BinaryExpression2` (341); + // `BinaryExpression` (253) is the pre-alias symbol and never + // emitted, listed defensively per §1 and pinned unreachable + // by `cpp_comparison_folds_and_preproc_operands` (§2). + LT | GT | LTEQ | GTEQ | EQEQ | BANGEQ | LTEQGT if ancestors.parent(node).is_some_and(|p| { matches!(p.kind_id().into(), BinaryExpression | BinaryExpression2) }) => diff --git a/src/metrics/abc/mozcpp.rs b/src/metrics/abc/mozcpp.rs index 5141955a5..643f1564a 100644 --- a/src/metrics/abc/mozcpp.rs +++ b/src/metrics/abc/mozcpp.rs @@ -70,9 +70,6 @@ impl Abc for MozcppCode { CallExpression | CallExpression2 | NewExpression => { stats.branches += 1.; } - // Comparison operators emitted as token children of a - // `binary_expression`. The C++20 spaceship `<=>` (`LTEQGT`) - // is a comparison operator and counts once per use. // `else` opens an alternative branch path; `case` // (non-default) adds one per switch arm; `?` opens a // ternary; `try` / `catch` count per Fitzpatrick (and @@ -85,18 +82,17 @@ impl Abc for MozcppCode { // counterpart is Rule 9). See the module-level `Stats` // doc-comment for the cross-language policy (issue // #395, walker tracked in #403). - LTEQ | GTEQ | EQEQ | BANGEQ | LTEQGT | Else | Case | QMARK | Try | Try2 | Catch => { + Else | Case | QMARK | Try | Try2 | Catch => { stats.conditions += 1.; } - // Plain `<` / `>` doubles as template-argument and - // template-parameter delimiter (`std::vector`, - // `template `). The `binary_expression` parent - // check disambiguates without inspecting siblings — only - // comparison uses of `<` / `>` count. Both kind-id aliases - // (`BinaryExpression`, `BinaryExpression2`) are accepted - // because the C++ grammar emits the same node under two - // production-rule paths. - LT | GT + // The seven comparison tokens count only when applied. The + // vendored grammar has the same productions for them as + // tree-sitter-cpp; the `grammar.json` sweep, the fold and + // `#if` decisions, and the operator-overload defect (#1448) + // are recorded on the matching arm of `CppCode::compute`. + // Here `BinaryExpression2` (430) is the emitted id and + // `BinaryExpression` (342) the never-emitted pre-alias one. + LT | GT | LTEQ | GTEQ | EQEQ | BANGEQ | LTEQGT if ancestors.parent(node).is_some_and(|p| { matches!(p.kind_id().into(), BinaryExpression | BinaryExpression2) }) => diff --git a/src/metrics/abc/objc.rs b/src/metrics/abc/objc.rs index f27868a56..14874146f 100644 --- a/src/metrics/abc/objc.rs +++ b/src/metrics/abc/objc.rs @@ -67,6 +67,11 @@ impl Abc for ObjcCode { // and the `@try` / `@catch` exception conditions. `&&` / `||` // are deliberately excluded (Fitzpatrick Rule 7; the // unary-conditional counterpart is Rule 9, handled below). + // `<=` `>=` `==` `!=` need no parent gate, unlike C++ + // (#1448): tree-sitter-objc 3.0.2's `grammar.json` emits + // them only from `binary_expression` and + // `preproc_binary_expression`, and ObjC has no operator + // overloading. LTEQ | GTEQ | EQEQ | BANGEQ | Else | Case | QMARK | ATtry | CatchClause => { stats.conditions += 1.; } diff --git a/tests/repositories/big-code-analysis-output b/tests/repositories/big-code-analysis-output index 062ae3439..b22e69aad 160000 --- a/tests/repositories/big-code-analysis-output +++ b/tests/repositories/big-code-analysis-output @@ -1 +1 @@ -Subproject commit 062ae3439ee405c20152a9845e8d7f27d994f632 +Subproject commit b22e69aad9a9d9bbee0234557ffcd7748350b1d7 From 0ac3f36e2e3a789da631a23b5756013ac76deb49 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic <490392+dekobon@users.noreply.github.com> Date: Wed, 7 Oct 2026 22:03:15 -0700 Subject: [PATCH 02/17] fix(abc/cpp): count a comparison fold once The #1448 gate dropped comparison folds to zero. A fold applies its comparison, so `(a <= ...)` and `(a < ...)` now score one condition, and the binary fold `(0 == ... == a)` also scores one: the token must be the `fold_expression`'s `operator` field, which names only one of its two spellings. The applied-comparison test moves into `cpp_comparison_is_applied`, shared with Mozcpp and keyed on kind names like the other shared C++ helpers, which also keeps `compute` under its halstead.effort soft limit. The overload fixture gains a defaulted C++20 `operator==`. --- src/metrics/abc.rs | 43 +++++++++++++++++++++++++++++---------- src/metrics/abc/cpp.rs | 42 +++++++++++++++++++++++++------------- src/metrics/abc/mozcpp.rs | 12 +++++------ 3 files changed, 65 insertions(+), 32 deletions(-) diff --git a/src/metrics/abc.rs b/src/metrics/abc.rs index deff0429a..b9c26e9d5 100644 --- a/src/metrics/abc.rs +++ b/src/metrics/abc.rs @@ -4555,6 +4555,10 @@ mod tests { bool operator!=(const V& o) const { return true; } auto operator<=>(const V& o) const { return 0; } }; + struct W { + int n; + bool operator==(const W&) const = default; + }; int n(int a, int b) { return (a < b) + (a > b) + (a <= b) + (a >= b) + (a == b) + (a != b) + (a <=> b); }"; @@ -4568,9 +4572,15 @@ mod tests { ) { #[cfg(any(feature = "cpp", feature = "mozcpp"))] const SPELLINGS: [&str; 7] = ["<", ">", "<=", ">=", "==", "!=", "<=>"]; - let mut census = vec![(operator_name, 7, "operator names")]; - // Two of each token: one declaring, one applying. - census.extend(tokens.iter().zip(SPELLINGS).map(|(&id, s)| (id, 2, s))); + let mut census = vec![(operator_name, 8, "operator names")]; + // Two of each token, one declaring and one applying, plus the + // third `==` that `W`'s defaulted comparison declares. + census.extend( + tokens + .iter() + .zip(SPELLINGS) + .map(|(&id, s)| (id, if s == "==" { 3 } else { 2 }, s)), + ); assert_fixture_spells::

(CPP_COMPARISON_OVERLOADS, "foo.cpp", &census); check_func_space::(CPP_COMPARISON_OVERLOADS, "foo.cpp", |space| { let class = child_space(&space, "V"); @@ -4593,6 +4603,14 @@ mod tests { 7, "`n` applies all seven tokens: one condition each" ); + // A defaulted C++20 comparison declares the operator with no + // body; it is the common modern spelling and must score + // nothing anywhere in the file. + assert_eq!( + space.metrics.abc.conditions_sum(), + 7, + "only `n`'s seven applications are conditions" + ); }); } @@ -4644,15 +4662,18 @@ mod tests { // are gated. The fixture's two directives are the root's only // conditions. // - // A fold over a comparison operator does not count, for all six - // foldable tokens alike (`<` / `>` never did): before the fix `le` - // scored 1 and the binary fold `eq` scored 2, one per spelling of its - // single operator, against `lt`'s 0. `idiom` is the control — its - // `==` sits in a `binary_expression` inside the fold and counts. + // A fold over a comparison operator counts once, through the fold's + // `operator` field: before the fix `lt` scored 0, `le` 1 and the + // binary fold `eq` 2 (one per spelling of its single operator). + // `idiom` is the control — its `==` sits in a `binary_expression` + // inside the fold and counts there, while the fold's own `&&` is not + // a comparison. // // The census also pins §2: every `binary_expression` here carries the - // `BinaryExpression2` id, and `BinaryExpression` — the pre-alias - // symbol the arm lists defensively — is never emitted. + // `BinaryExpression2` id and the pre-alias `BinaryExpression` is never + // emitted. `cpp_comparison_is_applied` matches the kind *name*, which + // covers both, so a grammar that starts emitting the pre-alias id + // changes nothing — this census is what would notice it. #[cfg(any(feature = "cpp", feature = "mozcpp"))] fn assert_cpp_comparison_folds_and_preproc_operands( binary_expression: u16, @@ -4677,7 +4698,7 @@ mod tests { ], ); check_func_space::(src, "foo.cpp", |space| { - for (name, want) in [("lt", 0), ("le", 0), ("eq", 0), ("idiom", 1)] { + for (name, want) in [("lt", 1), ("le", 1), ("eq", 1), ("idiom", 1)] { assert_eq!( child_space(&space, name).metrics.abc.conditions(), want, diff --git a/src/metrics/abc/cpp.rs b/src/metrics/abc/cpp.rs index 4f22d8f60..3a8cfcf4c 100644 --- a/src/metrics/abc/cpp.rs +++ b/src/metrics/abc/cpp.rs @@ -13,6 +13,24 @@ use super::{Abc, Stats}; use crate::macros::cpp_bool_terminal_kinds; use crate::*; +// Whether a comparison token under `parent` applies its operator, as +// opposed to declaring an overload (`operator_name`) or delimiting a +// template. Compared by kind *name*, like `cpp_inspect_container`, so +// one body serves both grammars: at the pinned grammars every +// `binary_expression` (an `#if` operand included) carries the aliased +// id `BinaryExpression2`, and the name covers the never-emitted +// pre-alias id too. A fold counts once, through its `operator` field, +// because a binary fold `(0 == ... == a)` spells its operator twice. +pub(super) fn cpp_comparison_is_applied(node: &Node, parent: &Node) -> bool { + match parent.kind() { + "binary_expression" => true, + "fold_expression" => parent + .child_by_field_name("operator") + .is_some_and(|op| op.id() == node.id()), + _ => false, + } +} + // C++ ABC unary-conditional walker (Fitzpatrick Rule 9 in Figure 3; // see `rust_inspect_container` for the cross-language rationale). // Matches on node-kind NAMES so the helper is correct for every C-family @@ -267,12 +285,12 @@ impl Abc for CppCode { // condition each (#1420 is the C# instance). // - `_fold_operator` / `_binary_fold_operator` (`<` `>` `<=` // `>=` `==` `!=` only) — the parent is `fold_expression`. - // Not counted, as `<` / `>` never were: a binary fold - // `(0 == ... == a)` spells its one operator twice, and a - // comparison fold is a chained comparison - // (`a1 < (a2 < a3)`). The idiomatic spelling - // `((a == 0) && ...)` keeps its `==` inside a - // `binary_expression` and still counts. + // A fold applies its comparison, so it counts once per + // fold: the token must be the fold's `operator` field. A + // binary fold `(0 == ... == a)` spells its one operator + // twice, and the field names only one of the two + // spellings. The idiomatic `((a == 0) && ...)` keeps its + // `==` inside a `binary_expression` and counts there. // - `template_argument_list` / `template_parameter_list` / // `system_lib_string` (`<` `>` only) — delimiters // (`std::vector`, `#include `). @@ -282,15 +300,11 @@ impl Abc for CppCode { // token reparented under `{ERROR}` by recovery stops // counting too, as `<` / `>` always have. // - // At this pin every `binary_expression`, `#if` operand or - // not, carries kind id `BinaryExpression2` (341); - // `BinaryExpression` (253) is the pre-alias symbol and never - // emitted, listed defensively per §1 and pinned unreachable - // by `cpp_comparison_folds_and_preproc_operands` (§2). + // The test is `cpp_comparison_is_applied`, shared with Mozcpp. LT | GT | LTEQ | GTEQ | EQEQ | BANGEQ | LTEQGT - if ancestors.parent(node).is_some_and(|p| { - matches!(p.kind_id().into(), BinaryExpression | BinaryExpression2) - }) => + if ancestors + .parent(node) + .is_some_and(|parent| cpp_comparison_is_applied(node, &parent)) => { stats.conditions += 1.; } diff --git a/src/metrics/abc/mozcpp.rs b/src/metrics/abc/mozcpp.rs index 643f1564a..2820aaa65 100644 --- a/src/metrics/abc/mozcpp.rs +++ b/src/metrics/abc/mozcpp.rs @@ -10,8 +10,8 @@ )] use super::cpp::{ - cpp_count_unary_conditions, cpp_inspect_child, cpp_inspect_container, cpp_walk_for_statement, - cpp_walk_ternary, + cpp_comparison_is_applied, cpp_count_unary_conditions, cpp_inspect_child, + cpp_inspect_container, cpp_walk_for_statement, cpp_walk_ternary, }; use super::{Abc, Stats}; use crate::*; @@ -90,12 +90,10 @@ impl Abc for MozcppCode { // tree-sitter-cpp; the `grammar.json` sweep, the fold and // `#if` decisions, and the operator-overload defect (#1448) // are recorded on the matching arm of `CppCode::compute`. - // Here `BinaryExpression2` (430) is the emitted id and - // `BinaryExpression` (342) the never-emitted pre-alias one. LT | GT | LTEQ | GTEQ | EQEQ | BANGEQ | LTEQGT - if ancestors.parent(node).is_some_and(|p| { - matches!(p.kind_id().into(), BinaryExpression | BinaryExpression2) - }) => + if ancestors + .parent(node) + .is_some_and(|parent| cpp_comparison_is_applied(node, &parent)) => { stats.conditions += 1.; } From 98c9bf8b1efad922e6583b67c7d534fdab269096 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic <490392+dekobon@users.noreply.github.com> Date: Wed, 7 Oct 2026 22:24:45 -0700 Subject: [PATCH 03/17] fix(abc/ruby): score subject-less when clauses once A subject-less `case` lists ordinary boolean expressions, evaluated as `if` predicates, so a comparison in a `when` clause is already an ABC condition through the token arm. The `When` arm added a blanket one on top: `case; when x > 5 then 1; else 0; end` scored 3 against its `if` analogue's 2. Parenthesised and negated comparisons, `&&` / `||` chains, `(x in T)` pattern tests and ternaries double-counted the same way. A subject-less clause now pays for its decision only when no other arm already did, decided by `ruby_condition_scores_itself`, which peels `(...)` and `!` / `not` and asks whether a comparison, chain, pattern test or ternary remains. A subject-ful clause keeps its per-clause count, since its `pattern === subject` comparison is written nowhere. The #1421 template routed the condition through the `if` slot instead. Measured here, that slot scores 0 for `Foo::Bar`, `self`, `-x`, `defined?(x)`, `(y = x)`, `*xs` and `x + 1`, so delegating would have dropped each of those clauses from 1 to 0. That gap in the `if` slot is filed as #1520. A clause listing several patterns pays unless any pattern scored itself, so `when a, x > 1` and `when x > 1, a` agree. `when x > 1, x < -1` reads 2 against cyclomatic's single decision per clause, as Kotlin's multi-alternative entries do. The comparison token list moves into `ruby_comparison_kinds!` so the token arm and the new predicate cannot disagree. Fixes #1453 --- src/metrics/abc.rs | 333 ++++++++++++++++++++++++++++++++++++++++ src/metrics/abc/ruby.rs | 119 +++++++++++++- 2 files changed, 448 insertions(+), 4 deletions(-) diff --git a/src/metrics/abc.rs b/src/metrics/abc.rs index b9c26e9d5..cdeb06a2a 100644 --- a/src/metrics/abc.rs +++ b/src/metrics/abc.rs @@ -8727,6 +8727,339 @@ function f(int $a, int $b): int { ); } + // The two anchors every #1453 fixture needs, coupled on purpose as + // `assert_kotlin_class_members` couples them for #1421. `subjects` is + // the load-bearing one: the whole distinction is whether a `case` + // carries a `value` field, no node kind spells that, and a fixture + // that grew a subject would score its clauses the pre-#1453 way with + // every other row still satisfied. Each fixture is top-level `def`s, + // so the members are the unit space's. + #[cfg(feature = "ruby")] + fn assert_ruby_case_members( + src: &str, + subjects: usize, + kinds: &[(u16, usize, &str)], + expected: &[(&str, u64, u64)], + ) { + let parser = RubyParser::new( + src.as_bytes().to_vec(), + std::path::Path::new("foo.rb"), + None, + ); + let found = parser + .root() + .preorder() + .filter(|n| n.kind_id() == Ruby::Case && n.child_by_field_name("value").is_some()) + .count(); + assert_eq!(found, subjects, "`case` subjects — the fixture moved"); + assert_fixture_spells::(src, "foo.rb", kinds); + check_func_space::(src, "foo.rb", |space| { + assert_members_score(&space, expected); + }); + } + + // #1453's headline. A subject-less `when` clause's condition is an + // ordinary boolean expression, so a comparison in it is already an + // ABC condition through the token arm; the clause added a blanket one + // on top, and `gt` scored 3 against its `if` analogue `iff`'s 2. + // + // `bare` and `call` are the controls that isolate the defect: same + // clause shape, no operator for any other arm to count, so the clause + // must still pay for its decision — 2 and 1 before the fix and after. + // A fix that moved them would be attacking the clause count, which + // was never wrong. + // + // `gt`, `bare` and `iff` sit one above cyclomatic's decision count + // because ABC counts the `else` (Fitzpatrick Rule 5) and cyclomatic + // does not; every other row is at `conditions == cyclomatic - 1`. + // `cmt` puts a comment where the subject would go: an extra is not the + // `value` field, so the `case` stays subject-less. + #[cfg(feature = "ruby")] + #[test] + fn ruby_subjectless_case_when_counts_its_condition_once() { + let src = "def gt(x) + case + when x > 5 then 1 + else 0 + end +end +def iff(x) + if x > 5 then 1 else 0 end +end +def two(x) + case + when x > 5 then 1 + when x < 0 then 2 + end +end +def ge(x) + case + when x >= 5 then 1 + end +end +def andd(a, b) + case + when a > 1 && b < 2 then 1 + end +end +def bare(b) + case + when b then 1 + else 0 + end +end +def call(x) + case + when x.even? then 1 + end +end +def cmt(x) + case # no subject + when x > 5 then 1 + end +end +"; + assert_ruby_case_members( + src, + 0, + &[ + (Ruby::When as u16, 8, "`when` clauses"), + (Ruby::GT as u16, 5, "`>` comparisons"), + (Ruby::AMPAMP as u16, 1, "`andd`'s `&&`"), + (Ruby::Comment as u16, 1, "`cmt`'s comment"), + ], + &[ + // (member, abc.conditions, cyclomatic) + // Was 3: the `>` and the clause, for one decision. + ("gt", 2, 2), + ("iff", 2, 2), + // Was 4. + ("two", 2, 3), + // Was 2: `>=` reaches the same token arm. + ("ge", 1, 2), + // Was 3. The compound condition keeps both comparisons — + // suppressing the operators instead would collapse it. + ("andd", 2, 3), + ("bare", 2, 2), + ("call", 1, 2), + // Was 2. + ("cmt", 1, 2), + ], + ); + } + + // Wrappers and the shapes no other arm owns. `not_cmp`, `paren_cmp`, + // `tern` and `test_pat` peel to an expression another arm already + // scored, and each read one above parity before the fix. `paren` and + // `notted` peel to a bare operand nothing else counts, so the clause + // pays exactly as an `if` predicate would. + // + // `scope`, `neg`, `splat` and `arith` are why the clause asks "is this + // already paid for" rather than routing through `ruby_count_condition`: + // that `if` slot scores each of these 0, so delegating to it would + // have dropped all four from 1 to 0 against an unchanged decision. + #[cfg(feature = "ruby")] + #[test] + fn ruby_subjectless_case_when_condition_wrappers_count_once() { + let src = "def paren(b) + case + when (b) then 1 + end +end +def notted(b) + case + when !b then 1 + end +end +def not_cmp(x) + case + when not(x > 1) then 1 + end +end +def paren_cmp(x) + case + when (x > 1) then 1 + end +end +def tern(x, a, b) + case + when x ? a : b then 1 + end +end +def test_pat(x) + case + when (x in Integer) then 1 + end +end +def scope + case + when Foo::Bar then 1 + end +end +def neg(x) + case + when -x then 1 + end +end +def splat(xs) + case + when *xs then 1 + end +end +def arith(x) + case + when x + 1 then 1 + end +end +"; + assert_ruby_case_members( + src, + 0, + &[ + (Ruby::When as u16, 10, "`when` clauses"), + ( + Ruby::ParenthesizedStatements as u16, + 4, + "parenthesised operands", + ), + (Ruby::Conditional as u16, 1, "`tern`'s ternary"), + (Ruby::TestPattern as u16, 1, "`test_pat`'s pattern test"), + (Ruby::ScopeResolution2 as u16, 1, "`scope`'s `Foo::Bar`"), + (Ruby::SplatArgument as u16, 1, "`splat`'s `*xs`"), + ], + &[ + ("paren", 1, 2), + ("notted", 1, 2), + // Each was 2. + ("not_cmp", 1, 2), + ("paren_cmp", 1, 2), + ("test_pat", 1, 2), + // Was 3. The ternary's own `?` and its condition `x` are + // its two conditions, as in `if x ? a : b`. + ("tern", 2, 3), + ("scope", 1, 2), + ("neg", 1, 2), + ("splat", 1, 2), + ("arith", 1, 2), + ], + ); + } + + // A clause may list several patterns, an implicit `or` cyclomatic + // scores as one decision. The clause pays unless any pattern already + // scored itself, so `bare_first` and `cmp_first` agree although only + // their pattern order differs — reading the first pattern alone, as + // Kotlin's `kotlin_count_when_entry` does, would score them 2 and 1. + // + // `alts` is the row that cannot reach parity from the ABC side: both + // comparisons are real conditions wherever they are written, and the + // remaining gap is cyclomatic's one-per-clause model (as for Kotlin's + // `when { x > 5, y < 0 -> … }`). It was 3. + #[cfg(feature = "ruby")] + #[test] + fn ruby_subjectless_case_when_multiple_patterns() { + let src = "def bare_alts(a, b) + case + when a, b then 1 + end +end +def alts(x) + case + when x > 1, x < -1 then 1 + end +end +def bare_first(a, x) + case + when a, x > 1 then 1 + end +end +def cmp_first(a, x) + case + when x > 1, a then 1 + end +end +"; + assert_ruby_case_members( + src, + 0, + &[ + (Ruby::When as u16, 4, "`when` clauses"), + (Ruby::Pattern as u16, 8, "two patterns per clause"), + ], + &[ + ("bare_alts", 1, 2), + ("alts", 2, 2), + // Both were 2. + ("bare_first", 1, 2), + ("cmp_first", 1, 2), + ], + ); + } + + // The other half of #1453: a subject-ful clause must not move. It + // lists a pattern matched with `pattern === subject`, a comparison + // written nowhere, so the clause itself pays for the decision. + // `subj_cmp` is the subject-ful counterpart of `gt` above and keeps + // the clause's count *and* its `>`: `y > 5` is a value compared + // against `x`, two comparisons for one branch. `in_arms` (the + // `InClause` arm), `unl` and `elsif_` share no code with `When` and + // are here to pin that the fix left them alone. + #[cfg(feature = "ruby")] + #[test] + fn ruby_subjectful_case_when_keeps_the_per_clause_count() { + let src = "def konst(x) + case x + when 1 then 1 + when 2 then 2 + else 0 + end +end +def subj_alts(x) + case x + when 1, 2 then 1 + else 0 + end +end +def subj_cmp(x, y) + case x + when y > 5 then 1 + end +end +def in_arms(x) + case x + in Integer then 1 + in String then 2 + else 0 + end +end +def unl(x) + unless x > 5 then 1 else 0 end +end +def elsif_(x) + if x > 5 then 1 elsif x < 0 then 2 else 0 end +end +"; + // Three: `in_arms` is a `case_match`, a separate production. + assert_ruby_case_members( + src, + 3, + &[ + (Ruby::CaseMatch as u16, 1, "`in_arms`'s `case`"), + (Ruby::When as u16, 4, "`when` clauses"), + (Ruby::InClause as u16, 2, "`in_arms`'s clauses"), + (Ruby::GT as u16, 3, "`>` comparisons"), + ], + &[ + ("konst", 3, 3), + ("subj_alts", 2, 2), + ("subj_cmp", 2, 2), + ("in_arms", 3, 3), + ("unl", 2, 2), + ("elsif_", 4, 3), + ], + ); + } + // --------------------------------------------------------------- // Default-impl placeholder smoke tests (audited in #188). // diff --git a/src/metrics/abc/ruby.rs b/src/metrics/abc/ruby.rs index 08e664b38..a652be679 100644 --- a/src/metrics/abc/ruby.rs +++ b/src/metrics/abc/ruby.rs @@ -30,7 +30,8 @@ use crate::*; // every one of those tokens also names an operator method in a `def` // and `<` also spells a superclass clause (#1280) — plus the // control-flow arms that the Fitzpatrick rules -// list — the named clause nodes `Else` / `Elsif` / `When` and the +// list — the named clause nodes `Else` / `Elsif` / `When` (see +// `ruby_count_when` for a subject-less `case`) and the // `?` ternary marker, plus `Rescue` (the rescue clause) and rescue // modifiers. `if` / `unless` themselves are not counted (the head // condition appears as the inner comparison); the `Then` clause is @@ -178,6 +179,113 @@ fn ruby_count_condition(condition: &Node, parent: &Node, conditions: &mut f64) { } } +// The comparison / equality tokens that are one condition each inside a +// `binary`. Shared by the token arm in `compute` and by +// `ruby_condition_scores_itself`, which must agree with that arm on +// exactly which operators it charges. +macro_rules! ruby_comparison_kinds { + () => { + Ruby::EQEQ + | Ruby::BANGEQ + | Ruby::EQEQEQ + | Ruby::LT + | Ruby::GT + | Ruby::LTEQ + | Ruby::GTEQ + | Ruby::LTEQGT + | Ruby::EQTILDE + | Ruby::BANGTILDE + }; +} + +// Whether another arm of `compute` already charges `expr` as a condition, +// looking through `(…)` and `!` / `not` layers, which add no decision of +// their own. True for a comparison (the token arm), an `&&` / `||` / +// `and` / `or` chain (the Rule 9 walker), a one-line pattern test (the +// `TestPattern` arm) and a ternary (the `?` arm plus `ruby_walk_ternary`) +// — exactly the expressions the `if` predicate slot leaves to those arms. +// +// It answers "is the decision already paid for", not "is this a boolean +// slot", which is why a subject-less `when` asks this rather than routing +// through `ruby_count_condition`. That slot scores zero for any shape +// outside `ruby_bool_terminal_kinds!()` — `Foo::Bar`, `self`, `-x`, +// `defined?(x)`, `(y = x)`, `*xs`, `1..2` — so delegating to it would +// have moved each of those `when`s from 1 to 0 against an unchanged +// cyclomatic decision (measured for #1453). The `if` forms of those +// shapes do score 0 today; that is the slot's gap (#1520), not this +// arm's to copy. +fn ruby_condition_scores_itself(expr: &Node) -> bool { + use Ruby::*; + + let mut node = *expr; + loop { + let next = match node.kind_id().into() { + Binary | Binary2 | Binary3 => { + return node.child_by_field_name("operator").is_some_and(|op| { + let op = op.kind_id().into(); + matches!(op, ruby_comparison_kinds!()) + || matches!(op, AMPAMP | PIPEPIPE | And | Or) + }); + } + TestPattern | Conditional => return true, + ParenthesizedStatements => node.children().find(Node::is_named), + Unary | Unary2 | Unary3 | Unary4 | Unary5 + if node + .child_by_field_name("operator") + .is_some_and(|op| matches!(op.kind_id().into(), BANG | Not)) => + { + node.child_by_field_name("operand") + } + _ => return false, + }; + let Some(child) = next else { return false }; + node = child; + } +} + +// Scores one `when` clause (#1453, transferring #1421's Kotlin rule). +// Both `case` shapes contribute the single decision cyclomatic counts per +// clause, but they pay for it in different places. +// +// A subject-ful clause (`case x; when 1`) lists a *pattern* matched with +// `pattern === x`: that comparison is written nowhere in the source, so +// the clause itself is the condition. +// +// A subject-less clause (`case; when x > 5`) lists an ordinary boolean +// expression, evaluated exactly as an `if` predicate. When that +// expression is a comparison, chain, pattern test or ternary, its own arm +// has already counted it, and the clause's former blanket +1 counted the +// same decision twice: `case; when x > 5 then 1; else 0; end` scored 3 +// against its `if` analogue's 2. Otherwise — a bare `when b`, `(b)`, +// `!b`, `x.even?` — nothing else sees it and the clause still pays. +// +// A clause may list several patterns (`when a, b`), an implicit `or` that +// cyclomatic scores as one decision. The clause pays unless *any* pattern +// already scored itself, so `when a, b` stays at 1 and the result does not +// depend on the order the patterns are written in. `when x > 1, x < -1` +// reads 2 against that one decision: both comparisons are real ABC +// conditions wherever they appear, and the gap is cyclomatic's +// one-per-clause model, as for Kotlin's multi-alternative entries. +// +// A `when` whose parent is not a `case` occurs only under error +// recovery; it keeps the per-clause count rather than guessing. +fn ruby_count_when<'a>(when: &Node<'a>, ancestors: Ancestors<'a, '_>, conditions: &mut f64) { + let subject_less = ancestors.parent(when).is_some_and(|case| { + case.kind_id() == Ruby::Case && case.child_by_field_name("value").is_none() + }); + // `pattern` wraps exactly one expression, so an extra (a comment) can + // sit beside it under the `when` but never inside it. + let owned = subject_less + && when + .children() + .filter(|child| child.kind_id() == Ruby::Pattern) + .filter_map(|pattern| pattern.child(0)) + .any(|expr| ruby_condition_scores_itself(&expr)); + if !owned { + *conditions += 1.; + } +} + // Phase-2B (issues #403 / #1102 / #1161): a Ruby ternary's condition and // its two branch operands are each a Fitzpatrick Rule 9 unary condition, // exactly as `cpp_walk_ternary` counts them for the C family. Without @@ -275,7 +383,7 @@ impl Abc for RubyCode { // siblings are listed defensively per grammar-dispatch §1. // `<<` is the distinct `LTLT` token and heredoc openers are // their own tokens, so neither is affected. - EQEQ | BANGEQ | EQEQEQ | LT | GT | LTEQ | GTEQ | LTEQGT | EQTILDE | BANGTILDE + ruby_comparison_kinds!() if ancestors .parent(node) .is_some_and(|p| matches!(p.kind_id().into(), Binary | Binary2 | Binary3)) => @@ -301,10 +409,13 @@ impl Abc for RubyCode { // the arm's meaning is "this node is a condition, with // nothing to gate on" — as true of a production as of a // token. - Else | Elsif | When | QMARK | Rescue | RescueModifier | RescueModifier2 - | RescueModifier3 | TestPattern => { + Else | Elsif | QMARK | Rescue | RescueModifier | RescueModifier2 | RescueModifier3 + | TestPattern => { stats.conditions += 1.; } + // A subject-less `case` can already have paid for its clause + // through the clause's own operators (#1453). + When => ruby_count_when(node, ancestors, &mut stats.conditions), // A `case … in` pattern-match arm is a branch condition exactly // when it counts toward cyclomatic — a non-wildcard pattern or // a guarded arm. The bare `in _` default arm is filtered out, From 20c33876d6a6de5e91305fa80b87ba1312886e89 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic <490392+dekobon@users.noreply.github.com> Date: Wed, 7 Oct 2026 22:31:06 -0700 Subject: [PATCH 04/17] fix(abc/ruby): score each when pattern as an || operand A subject-less `when a, b` is an implicit `||`, but the clause paid one condition unless any pattern had already scored itself, so `when a, b` and `when a, x > 1` read 1 against their `if a || b` / `if a || x > 1` analogues' 2, while `when x > 1, x < -1` matched its analogue at 2. Each pattern now scores once unless its own arm already counted it, which matches all three analogues and stays independent of pattern order. Subject-ful clauses are unchanged. --- src/metrics/abc.rs | 39 +++++++++++++++++++++++++-------------- src/metrics/abc/ruby.rs | 34 +++++++++++++++++++--------------- 2 files changed, 44 insertions(+), 29 deletions(-) diff --git a/src/metrics/abc.rs b/src/metrics/abc.rs index cdeb06a2a..607c96b3a 100644 --- a/src/metrics/abc.rs +++ b/src/metrics/abc.rs @@ -8945,16 +8945,14 @@ end ); } - // A clause may list several patterns, an implicit `or` cyclomatic - // scores as one decision. The clause pays unless any pattern already - // scored itself, so `bare_first` and `cmp_first` agree although only - // their pattern order differs — reading the first pattern alone, as - // Kotlin's `kotlin_count_when_entry` does, would score them 2 and 1. - // - // `alts` is the row that cannot reach parity from the ABC side: both - // comparisons are real conditions wherever they are written, and the - // remaining gap is cyclomatic's one-per-clause model (as for Kotlin's - // `when { x > 5, y < 0 -> … }`). It was 3. + // A clause may list several patterns: an implicit `||`, so each is + // one operand of that chain and scores once unless its own arm already + // counted it — every row here must equal its `if … || …` twin in the + // `or_*` defs. `bare_first` and `cmp_first` differ only in pattern + // order and must agree; reading the first pattern alone, as Kotlin's + // `kotlin_count_when_entry` does, would score them 2 and 1. None can + // reach `conditions == cyclomatic - 1`: cyclomatic scores the clause + // as one decision, and the `||` twins sit just as far above it. #[cfg(feature = "ruby")] #[test] fn ruby_subjectless_case_when_multiple_patterns() { @@ -8978,6 +8976,15 @@ def cmp_first(a, x) when x > 1, a then 1 end end +def or_bare(a, b) + if a || b then 1 end +end +def or_cmp(x) + if x > 1 || x < -1 then 1 end +end +def or_mixed(a, x) + if a || x > 1 then 1 end +end "; assert_ruby_case_members( src, @@ -8987,11 +8994,15 @@ end (Ruby::Pattern as u16, 8, "two patterns per clause"), ], &[ - ("bare_alts", 1, 2), + // Was 1: one pattern went unscored. + ("bare_alts", 2, 2), + ("or_bare", 2, 3), + // Was 3. ("alts", 2, 2), - // Both were 2. - ("bare_first", 1, 2), - ("cmp_first", 1, 2), + ("or_cmp", 2, 3), + ("bare_first", 2, 2), + ("cmp_first", 2, 2), + ("or_mixed", 2, 3), ], ); } diff --git a/src/metrics/abc/ruby.rs b/src/metrics/abc/ruby.rs index a652be679..78030902a 100644 --- a/src/metrics/abc/ruby.rs +++ b/src/metrics/abc/ruby.rs @@ -259,13 +259,14 @@ fn ruby_condition_scores_itself(expr: &Node) -> bool { // against its `if` analogue's 2. Otherwise — a bare `when b`, `(b)`, // `!b`, `x.even?` — nothing else sees it and the clause still pays. // -// A clause may list several patterns (`when a, b`), an implicit `or` that -// cyclomatic scores as one decision. The clause pays unless *any* pattern -// already scored itself, so `when a, b` stays at 1 and the result does not -// depend on the order the patterns are written in. `when x > 1, x < -1` -// reads 2 against that one decision: both comparisons are real ABC -// conditions wherever they appear, and the gap is cyclomatic's -// one-per-clause model, as for Kotlin's multi-alternative entries. +// A clause may list several patterns (`when a, b`): an implicit `||`, so +// each pattern is one operand of that chain and is scored as Rule 9 +// scores an `||` operand — once, unless its own arm already counted it. +// `when a, b` / `when a, x > 1` / `when x > 1, x < -1` therefore each +// read 2, exactly their `if a || b` analogues, and the order the patterns +// are written in cannot matter. Cyclomatic scores the clause as one +// decision, so these sit above `conditions == cyclomatic - 1`, as the +// `||` analogues do. // // A `when` whose parent is not a `case` occurs only under error // recovery; it keeps the per-clause count rather than guessing. @@ -273,16 +274,19 @@ fn ruby_count_when<'a>(when: &Node<'a>, ancestors: Ancestors<'a, '_>, conditions let subject_less = ancestors.parent(when).is_some_and(|case| { case.kind_id() == Ruby::Case && case.child_by_field_name("value").is_none() }); + if !subject_less { + *conditions += 1.; + return; + } // `pattern` wraps exactly one expression, so an extra (a comment) can // sit beside it under the `when` but never inside it. - let owned = subject_less - && when - .children() - .filter(|child| child.kind_id() == Ruby::Pattern) - .filter_map(|pattern| pattern.child(0)) - .any(|expr| ruby_condition_scores_itself(&expr)); - if !owned { - *conditions += 1.; + for pattern in when.children().filter(|c| c.kind_id() == Ruby::Pattern) { + if pattern + .child(0) + .is_some_and(|expr| !ruby_condition_scores_itself(&expr)) + { + *conditions += 1.; + } } } From 2d8678863a02adaa75e3ff546deda04957469266 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic <490392+dekobon@users.noreply.github.com> Date: Wed, 7 Oct 2026 22:33:48 -0700 Subject: [PATCH 05/17] test(abc/ruby): cover || and or when clauses --- src/metrics/abc.rs | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/metrics/abc.rs b/src/metrics/abc.rs index 607c96b3a..bcb0125db 100644 --- a/src/metrics/abc.rs +++ b/src/metrics/abc.rs @@ -8802,6 +8802,16 @@ def andd(a, b) when a > 1 && b < 2 then 1 end end +def orr(a, b) + case + when a || b then 1 + end +end +def kw_or(a, b) + case + when a or b then 1 + end +end def bare(b) case when b then 1 @@ -8823,9 +8833,11 @@ end src, 0, &[ - (Ruby::When as u16, 8, "`when` clauses"), + (Ruby::When as u16, 10, "`when` clauses"), (Ruby::GT as u16, 5, "`>` comparisons"), (Ruby::AMPAMP as u16, 1, "`andd`'s `&&`"), + (Ruby::PIPEPIPE as u16, 1, "`orr`'s `||`"), + (Ruby::Or as u16, 1, "`kw_or`'s `or`"), (Ruby::Comment as u16, 1, "`cmt`'s comment"), ], &[ @@ -8840,6 +8852,10 @@ end // Was 3. The compound condition keeps both comparisons — // suppressing the operators instead would collapse it. ("andd", 2, 3), + // The Rule 9 walker scores both operands of a bare chain, + // in both spellings; the clause adds nothing on top. + ("orr", 2, 3), + ("kw_or", 2, 3), ("bare", 2, 2), ("call", 1, 2), // Was 2. From 61e65b7c401ed89b044bbda53a24107b6d926827 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic <490392+dekobon@users.noreply.github.com> Date: Wed, 7 Oct 2026 22:53:07 -0700 Subject: [PATCH 06/17] fix(abc/perl): score a bare match by use A bare `/re/` or `m{re}` matches the implicit `$_`, a relational operator with no operator token. It reached ABC only through `perl_bool_terminal_kinds!()`, which walkers consult inside a boolean slot, so `my $r = /^#/` scored 0 against `my $r = ($x =~ /^#/)`'s 1. Move both kinds out of the terminal set onto a gated arm that counts the pattern wherever it is written, except where it is bound by `=~` / `!~` (that token already counts) or is `split`'s first argument, a delimiter rather than a match. In-slot shapes such as `if (/x/)` and `/x/ && $y` keep their scores. `s///` and `tr///` stay out of scope (#1475). Fixes #1467 --- big-code-analysis-ast/src/macros/kind_sets.rs | 24 +-- big-code-analysis-book/src/metrics.md | 2 +- src/metrics/abc.rs | 153 +++++++++++++++++- src/metrics/abc/perl.rs | 77 ++++++++- 4 files changed, 226 insertions(+), 30 deletions(-) diff --git a/big-code-analysis-ast/src/macros/kind_sets.rs b/big-code-analysis-ast/src/macros/kind_sets.rs index 12cdf9f3f..e37c09507 100644 --- a/big-code-analysis-ast/src/macros/kind_sets.rs +++ b/big-code-analysis-ast/src/macros/kind_sets.rs @@ -732,23 +732,11 @@ macro_rules! python_bool_terminal_kinds { // 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 `=~`. +// `pattern_matcher` (`/^#/`) and `pattern_matcher_m` (`m{^#}`), the +// two spellings of a match against the implicit `$_`, are absent: a +// match is a relational operator, so it scores by use through a gated +// arm in `PerlCode::compute` (#1467), not only where a slot reads it. +// Listing them here as well would score `if (/^#/)` twice (§5). // // `substitution_pattern_s` (`s///`) and `transliteration_tr_or_y` // (`tr///`) are deliberately absent: both edit `$_` and evaluate to a @@ -805,8 +793,6 @@ macro_rules! perl_bool_terminal_kinds { | $crate::Perl::CallExpressionRecursive | $crate::Perl::CallExpressionWithBareword | $crate::Perl::MethodInvocation - | $crate::Perl::PatternMatcher - | $crate::Perl::PatternMatcherM | $crate::Perl::StringSingleQuoted | $crate::Perl::StringQQuoted | $crate::Perl::StringDoubleQuoted diff --git a/big-code-analysis-book/src/metrics.md b/big-code-analysis-book/src/metrics.md index 69c2962db..5deec1604 100644 --- a/big-code-analysis-book/src/metrics.md +++ b/big-code-analysis-book/src/metrics.md @@ -155,7 +155,7 @@ application would over-count. | 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 TSX, JavaScript and Mozjs — TypeScript carries the same gate through the shared macro, but the `.ts` dialect of tree-sitter-typescript has no JSX production, so it has nothing to exclude there; 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 is on the operator alone, not on the test that encloses it: since #1461 the `is` test itself is a condition wherever it is written, so `bool b = x is > 0;` and `return x is > 0;` each score 1 — level with the `x > 0` they are sugar for, and the same 1 a `when n is > 5` guard has scored since #1422 made the guard a condition slot. Counting the pattern's operator as well would make a relational arm worth twice the constant arm `5 => 1`. 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). The exclusion is ABC's alone: Halstead counts vocabulary rather than decisions, so `bca ops` still bills every one of these `<` / `>` as an operator, and the two metrics are meant to disagree here (#1395) — see [the Halstead delimiter rule](#halstead). | | 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. | -| C#, Java, Groovy, Kotlin, Ruby, Elixir | A relational operator scores by use; a value-bearing operand scores in a boolean slot | Five of these grammars spell at least one relational construct as its own production rather than as a binary expression with an operator token: C#'s `x is int` and `x is null`, Java's and Groovy's `x instanceof T`, Groovy's `a in l`, Kotlin's `a is T` and `a in 1..2`, and Ruby's one-line `a in Integer`. Having no token to count, each was reached only through the language's terminal-operand set, which the walker consults inside an `if` / `while` / ternary / `&&`-operand slot and nowhere else — so `var b = x == 1;` scored 1 while `var b = x is int;` scored 0. Fitzpatrick's Rule 5 counts a relational operator wherever it appears, so each now counts wherever it appears and has left the operand set; being in both would score it twice. Groovy's spaceship `<=>` counts on the same rule, as it already did in Ruby, PHP, C++ and Mozcpp, although its result is an integer rather than a boolean — what Rule 5 measures is the comparison, not its type. Elixir reached the same 0 by the other route: its membership and type tests (`a in [1, 2]`, `rescue e in RuntimeError`) *do* carry an operator token, and simply had no arm matching it. They count by use on the same rule, gated on the token's parent so that an operator merely *named* (`&in/2`) stays excluded alongside `<` and `>`. The converse still holds for a construct whose *value* fills the slot: a cast, a Go type assertion, a Rust `matches!` / `cfg!` macro and a Python walrus are all operands and count only where a slot reads them as a predicate (#1461). | +| C#, Java, Groovy, Kotlin, Ruby, Elixir, Perl | A relational operator scores by use; a value-bearing operand scores in a boolean slot | Five of these grammars spell at least one relational construct as its own production rather than as a binary expression with an operator token: C#'s `x is int` and `x is null`, Java's and Groovy's `x instanceof T`, Groovy's `a in l`, Kotlin's `a is T` and `a in 1..2`, and Ruby's one-line `a in Integer`. Having no token to count, each was reached only through the language's terminal-operand set, which the walker consults inside an `if` / `while` / ternary / `&&`-operand slot and nowhere else — so `var b = x == 1;` scored 1 while `var b = x is int;` scored 0. Fitzpatrick's Rule 5 counts a relational operator wherever it appears, so each now counts wherever it appears and has left the operand set; being in both would score it twice. Groovy's spaceship `<=>` counts on the same rule, as it already did in Ruby, PHP, C++ and Mozcpp, although its result is an integer rather than a boolean — what Rule 5 measures is the comparison, not its type. Elixir reached the same 0 by the other route: its membership and type tests (`a in [1, 2]`, `rescue e in RuntimeError`) *do* carry an operator token, and simply had no arm matching it. They count by use on the same rule, gated on the token's parent so that an operator merely *named* (`&in/2`) stays excluded alongside `<` and `>`. Perl's bare pattern match (`my $r = /^#/;`, or `m{^#}`) matches the implicit `$_` with no operator token at all, and scores by use on the same rule, level with `$x =~ /^#/` (#1467). Two patterns are excluded: the one bound by `=~` / `!~`, whose operator already counts, and the one `split` takes first (`split /,/, $s`), which is a delimiter rather than a match. The converse still holds for a construct whose *value* fills the slot: a cast, a Go type assertion, a Rust `matches!` / `cfg!` macro and a Python walrus are all operands and count only where a slot reads them as a predicate (#1461). | #### Where a constructor call lands {#abc-constructor-attribution} diff --git a/src/metrics/abc.rs b/src/metrics/abc.rs index bcb0125db..5c7776fd8 100644 --- a/src/metrics/abc.rs +++ b/src/metrics/abc.rs @@ -587,6 +587,7 @@ mod tests { feature = "groovy", feature = "java", feature = "kotlin", + feature = "perl", feature = "python", feature = "ruby", feature = "rust", @@ -13381,6 +13382,150 @@ end ); } + // A bare `/re/` or `m{re}` is a match against `$_`, a relational + // operator spelled with no operator token, so it scores by use + // (#1467, the rule #1461 applied to five other languages). Each + // `*_out` member and `ret` / `notm` / `grp` / `arg` / `smart` was 0 + // before the arm: only a boolean slot reached the pattern, through + // `perl_bool_terminal_kinds!()`. `mbare_out` is the `m{}` spelling, a + // sibling kind rather than an alias. `smart` levels a smartmatch + // with its `=~` sibling: no arm counts `~~`, so the pattern does. + // + // The in-slot members are the §5 check that the slot stopped + // counting the pattern in the change that gave it an arm: each + // reads what it read before. `bound_out` / `nbound` pin the gate on + // `=~` / `!~` — the operator token scores, the pattern under it does + // not — and `ctrl_out` is the `==` control the rest level with. + // `s///`, `tr///` and `qr//` are not matches and stay at 0; whether + // the first two should score is #1475. + #[cfg(feature = "perl")] + #[test] + fn perl_bare_match_is_a_condition_by_use() { + check_func_space::( + "sub bare_out { my $r = /^#/; return $r; }\n\ + sub mbare_out { my $r = m{^#}; return $r; }\n\ + sub ret { return /x/; }\n\ + sub notm { my $r = !/x/; return $r; }\n\ + sub grp { my @a = @_; return grep { /x/ } @a; }\n\ + sub arg { foo(/x/); }\n\ + sub smart { my ($x) = @_; my $r = $x ~~ /re/; return $r; }\n\ + sub bare_in { if (/^#/) { return 1; } return 0; }\n\ + sub mbare_in { unless (m{^#}) { return 1; } return 0; }\n\ + sub modif { return 0 unless /x/; return 1; }\n\ + sub andy { my ($y) = @_; return /x/ && $y; }\n\ + sub oror { return /a/ || /b/; }\n\ + sub tern { return /x/ ? 1 : 0; }\n\ + sub forc { for (my $i = 0; /x/; $i++) { } }\n\ + sub bound_out { my ($x) = @_; my $r = ($x =~ /^#/); return $r; }\n\ + sub nbound { my ($x) = @_; my $r = ($x !~ /^#/); return $r; }\n\ + sub ctrl_out { my ($x) = @_; my $r = ($x == 1); return $r; }\n\ + sub subst { my $n = s/x/y/; return $n; }\n\ + sub tr1 { my $n = tr/a/b/; return $n; }\n\ + sub qrx { my $q = qr/x/; return $q; }\n", + "foo.pl", + |space| { + assert_members_score( + &space, + &[ + ("bare_out", 1, 1), + ("mbare_out", 1, 1), + ("ret", 1, 1), + // `!` adds nothing outside a slot; the match is + // the one condition. + ("notm", 1, 1), + ("grp", 1, 1), + ("arg", 1, 1), + ("smart", 1, 1), + ("bare_in", 1, 2), + ("mbare_in", 1, 2), + ("modif", 1, 2), + // One per `&&` / `||` operand, the match included. + ("andy", 2, 2), + ("oror", 2, 2), + // The ternary node plus its condition. + ("tern", 2, 2), + ("forc", 1, 2), + ("bound_out", 1, 1), + ("nbound", 1, 1), + ("ctrl_out", 1, 1), + ("subst", 0, 1), + ("tr1", 0, 1), + ("qrx", 0, 1), + ], + ); + }, + ); + } + + // The pattern `split` takes first is its delimiter, not a match + // against `$_`, so it is no condition (#1467) in any spelling: the + // spaced and parenthesised call forms, the `m{}` pattern, and the + // `CORE::` qualifier. Two discriminating members keep the exclusion + // narrow: `spl2` puts the pattern in `split`'s *second* slot, where + // it is an ordinary match whose result `split` receives, `grpe` + // puts one first in a call that is not `split`, and `tsplit` puts + // one first in a list whose grandparent holds a bare `split` without + // being that call — the ternary node plus `$c` plus the match. + #[cfg(feature = "perl")] + #[test] + fn perl_split_delimiter_is_not_a_condition() { + use crate::{ParserTrait, Perl}; + + let src = "sub spl { my ($s) = @_; my @p = split /,/, $s; return @p; }\n\ + sub splp { my ($s) = @_; my @p = split(/,/, $s); return @p; }\n\ + sub splm { my ($s) = @_; my @p = split m{,}, $s; return @p; }\n\ + sub splc { my ($s) = @_; my @p = CORE::split(/,/, $s); return @p; }\n\ + sub spl2 { my @p = split ' ', /x/; return @p; }\n\ + sub grpe { my @a = @_; return grep /x/, @a; }\n\ + sub tsplit { my ($c) = @_; my @r = $c ? split : (/x/); return @r; }\n"; + + // Fixture census: each member but `tsplit` holds exactly one + // pattern, as a direct argument of its call — so a zero below + // is the exclusion at work and not a pattern the fixture lost. + let parser = PerlParser::new( + src.as_bytes().to_vec(), + &std::path::PathBuf::from("foo.pl"), + None, + ); + let call_arguments = parser + .root() + .preorder() + .filter(|n| { + matches!( + n.kind_id().into(), + Perl::PatternMatcher | Perl::PatternMatcherM + ) + }) + .filter(|n| { + n.parent() + .and_then(|list| list.parent()) + .is_some_and(|call| { + matches!( + call.kind_id().into(), + Perl::CallExpressionWithSpacedArgs + | Perl::CallExpressionWithArgsWithBrackets + ) + }) + }) + .count(); + assert_eq!(call_arguments, 6, "one call-argument pattern per member"); + + check_func_space::(src, "foo.pl", |space| { + assert_members_score( + &space, + &[ + ("spl", 0, 1), + ("splp", 0, 1), + ("splm", 0, 1), + ("splc", 0, 1), + ("spl2", 1, 1), + ("grpe", 1, 1), + ("tsplit", 3, 2), + ], + ); + }); + } + // ---------- Lua ABC tests ---------- #[cfg(feature = "lua")] @@ -16575,8 +16720,8 @@ mod own_production_bool_constructs { /// 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{^#}` + /// The bare-match arm of `PerlCode::compute` 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, @@ -16597,7 +16742,7 @@ mod own_production_bool_constructs { assert_ne!( Perl::PatternMatcher as u16, Perl::PatternMatcherM as u16, - "the two spellings share one kind id; one terminal-set entry is dead" + "the two spellings share one kind id; one bare-match arm entry is dead" ); for (src, present, absent) in [ @@ -16626,7 +16771,7 @@ mod own_production_bool_constructs { 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" + collapsed and one bare-match arm entry is dead" ); } } diff --git a/src/metrics/abc/perl.rs b/src/metrics/abc/perl.rs index fc00077f9..275837060 100644 --- a/src/metrics/abc/perl.rs +++ b/src/metrics/abc/perl.rs @@ -30,7 +30,8 @@ use crate::*; // already contributed the branch. // - Conditions: numeric and string comparison operators (`==`, `!=`, // `<`, `>`, `<=`, `>=`, `<=>`, `eq`, `ne`, `lt`, `gt`, `le`, `ge`, -// `cmp`, `=~`, `!~`), the ternary operator (`TernaryExpression`), +// `cmp`, `=~`, `!~`), a bare `/re/` / `m{re}` match against `$_`, +// the ternary operator (`TernaryExpression`), // and each `elsif` / `else` clause of an `if` / `unless` // statement. Bare predicates that have no comparison (e.g. // `if ($x)`) are not separately counted; we let the comparison @@ -317,6 +318,57 @@ fn perl_walk_statement_modifier(node: &Node, conditions: &mut f64) { } } +// Whether a pattern (`/re/` or `m{re}`) is one of the two shapes that +// are not a bare match the ABC walk should count on its own (#1467): +// +// - **Bound**: the pattern is the right operand of `$x =~ /re/` or +// `$x !~ /re/`, whose operator token is already a condition. Read +// through the grammar's `operator` field, so `$x ~~ /re/` — a +// smartmatch, which no arm counts — still scores the match once. +// - **`split`'s delimiter**: `split /,/, $s` and `split(/,/, $s)` hand +// the pattern to `split` as a separator; nothing is matched against +// `$_`, so it is no condition. Only the *first* argument is a +// delimiter: a pattern elsewhere in the list is an ordinary match +// whose result `split` receives. The callee is identified by its +// `function_name` bytes (grammar-dispatch §10) — the package +// qualifier is ignored so `CORE::split` is covered too. +fn perl_pattern_is_bound_or_delimiter(pattern: &Node, code: &[u8], ancestors: Ancestors) -> bool { + use Perl as P; + + let mut climb = ancestors.iter(pattern).map(|(ancestor, _)| ancestor); + climb + .next() + .is_some_and(|parent| match parent.kind_id().into() { + P::BinaryExpression => parent + .child_by_field_name("operator") + .is_some_and(|op| matches!(op.kind_id().into(), P::EQTILDE | P::BANGTILDE)), + P::Arguments | P::Array => { + parent + .children() + .find(Node::is_named) + .is_some_and(|first| first.id() == pattern.id()) + && climb + .next() + .is_some_and(|call| perl_call_is_split(&call, code)) + } + _ => false, + }) +} + +// Whether `call` is a `split` call. Every named child of the two call +// wrappers that take an argument list is the callee or an `args` +// field, so a list whose grandparent is one of them is its arguments. +fn perl_call_is_split(call: &Node, code: &[u8]) -> bool { + matches!( + call.kind_id().into(), + Perl::CallExpressionWithSpacedArgs | Perl::CallExpressionWithArgsWithBrackets + ) && call + .children() + .find(|child| child.kind_id() == Perl::CallExpressionWithBareword as u16) + .and_then(|callee| callee.child_by_field_name("function_name")) + .is_some_and(|name| code.get(name.start_byte()..name.end_byte()) == Some(b"split")) +} + fn perl_is_call_argument_parent(parent: Node) -> bool { use Perl as P; matches!( @@ -359,7 +411,7 @@ fn perl_count_unary_conditions(list_node: &Node, conditions: &mut f64) { impl Abc for PerlCode { fn compute<'a>( node: &Node<'a>, - _code: &'a [u8], + code: &'a [u8], ancestors: Ancestors<'a, '_>, stats: &mut Stats, ) { @@ -374,10 +426,10 @@ impl Abc for PerlCode { // and the cyclomatic count is the number of node kinds the // grammar can hand us, neither being reasoning a reader must // do. Adding the guarded `<` / `>` arm for #1297 took the - // count from 14 to 15, and the statement-modifier arm for - // #1464 from 15 to 16; each arm is independent and - // self-describing like every other, and there is no semantic - // boundary to split this lookup on. + // count from 14 to 15, the statement-modifier arm for #1464 + // to 16, and the bare-match arm for #1467 to 17; each arm is + // independent and self-describing like every other, and there + // is no semantic boundary to split this lookup on. use Perl as P; match node.kind_id().into() { @@ -478,6 +530,19 @@ impl Abc for PerlCode { P::LT | P::GT if ancestors.parent_has_kind(node, P::BinaryExpression as u16) => { stats.conditions += 1.; } + // A bare `/re/` or `m{re}` (sibling kinds, not aliases) + // matches the implicit `$_`: a relational operator with no + // operator token, so it scores by use wherever it is + // written (#1467) — `my $r = /^#/` levels with + // `my $r = ($x =~ /^#/)`. It is not in + // `perl_bool_terminal_kinds!()`, or every slot would score + // it a second time. `s///` and `tr///` stay out: they edit + // `$_` and yield a count, a policy question left to #1475. + P::PatternMatcher | P::PatternMatcherM + if !perl_pattern_is_bound_or_delimiter(node, code, ancestors) => + { + stats.conditions += 1.; + } // Fitzpatrick Rule 9 walker: each operand of a Perl // short-circuit / low-precedence logical chain is one // condition (issue #403). Covers `&&`, `||`, `//`, From 4b7028f827a591d879fcef1fceb22c20265c71aa Mon Sep 17 00:00:00 2001 From: Elijah Zupancic <490392+dekobon@users.noreply.github.com> Date: Wed, 7 Oct 2026 23:13:55 -0700 Subject: [PATCH 07/17] refactor(abc): ask the peel which kinds are wrappers Six languages spelled their wrapper set twice: once in the `_inspect_container` peel loop and again in `_count_condition`, which decides what to hand that peel. Kotlin, Groovy and C# each had the two lists drift apart and score a valid predicate zero (#1459, #1466, #1463). Ruby, Go, Rust, Python, Lua and Java now carry a `_wrapper_operand` that returns one peel step, and both the loop and the condition slot ask it. Python's returns a bare node: it peels parentheses only, so it has no proves-boolean flag to report. Step semantics are unchanged, including the positional `child(1)` reads. PHP, Perl and C++ restated nothing (their slot already falls through to the peel) and are left alone. A 337-cell wrapper matrix (`b`, `(b)`, `!b`, `!!b`, `(!b)`, `!(b)`, `((b))`, `-b`, `(-b)`, plus `not` forms, across each language's if, loop, chain, ternary and guard slots) measured identical before and after. The new tests pin routed, declined and proves-boolean answers per language; a perturbation sweep of each half fails them. The Java `java_inspect_container` cognitive baseline entry is dropped, as the function no longer breaches. Fixes #1470 --- .bca-baseline.toml | 6 -- src/metrics/abc.rs | 189 ++++++++++++++++++++++++++++++++++++++ src/metrics/abc/go.rs | 53 ++++++----- src/metrics/abc/java.rs | 74 ++++++++------- src/metrics/abc/lua.rs | 58 +++++++----- src/metrics/abc/python.rs | 42 ++++++--- src/metrics/abc/ruby.rs | 97 +++++++++---------- src/metrics/abc/rust.rs | 50 +++++----- 8 files changed, 398 insertions(+), 171 deletions(-) diff --git a/.bca-baseline.toml b/.bca-baseline.toml index 8169e0da6..abb57103a 100644 --- a/.bca-baseline.toml +++ b/.bca-baseline.toml @@ -899,12 +899,6 @@ qualified = "GoCode::compute" metric = "halstead.effort" value = 49999.43426005393 -[[entry]] -path = "src/metrics/abc/java.rs" -qualified = "java_inspect_container" -metric = "cognitive" -value = 15.0 - [[entry]] path = "src/metrics/abc/kotlin.rs" qualified = "KotlinCode::compute" diff --git a/src/metrics/abc.rs b/src/metrics/abc.rs index 5c7776fd8..63be76c91 100644 --- a/src/metrics/abc.rs +++ b/src/metrics/abc.rs @@ -17602,3 +17602,192 @@ mod hidden_literal_supertypes { ); } } + +/// The condition slot and the wrapper peel agree on which kinds are +/// wrappers, in each language whose slot used to restate the list +/// (#1470). +/// +/// `_count_condition` decides which slot expressions to hand the +/// peel, and `_inspect_container` decides which it can descend. +/// Spelled as two lists they drifted three times — Kotlin (#1459), +/// Groovy (#1466) and C# (#1463) — each time scoring a valid predicate +/// zero. The six languages here now ask `_wrapper_operand` +/// instead, which removed the duplication without moving a number: the +/// wrapper matrix measured identical before and after, so a revert +/// cannot fail these tests. What they pin is the *answer* both halves +/// must keep giving: +/// +/// - **Routed.** Every wrapper the peel accepts scores its slot once. +/// A slot that stops asking the peel scores these zero. +/// - **Declined.** A unary the peel does not accept (`-b`) scores +/// nothing, so a peel that starts accepting arithmetic operators by +/// accident is caught here rather than in a corpus snapshot. +/// - **Proves boolean.** A negation counts its operand outside any +/// boolean slot and a parenthesis does not — the second half of the +/// peel's answer, which the slot never sees. +/// +/// Each template's `@` is the operand under test. +#[cfg(test)] +#[cfg(any( + feature = "rust", + feature = "go", + feature = "java", + feature = "python", + feature = "ruby", + feature = "lua" +))] +mod wrapper_peel_routing { + use crate::test_support::metrics_verbatim; + use crate::{LANG, MetricsOptions}; + + fn conditions(lang: LANG, source: &str) -> u64 { + metrics_verbatim(lang, source.as_bytes(), MetricsOptions::default()) + .abc + .conditions_sum() + } + + /// Asserts `conditions` for every `(operand, expected)` row of one + /// template. + fn assert_operands(lang: LANG, template: &str, rows: &[(&str, u64)]) { + assert!(template.contains('@'), "template has no operand slot"); + for &(operand, expected) in rows { + let source = template.replace('@', operand); + assert_eq!( + conditions(lang, &source), + expected, + "`{operand}` in {lang:?}\n source: {source}" + ); + } + } + + #[test] + #[cfg(feature = "rust")] + fn rust_condition_slot_routes_what_the_peel_accepts() { + assert_operands( + LANG::Rust, + "fn f(b: bool) { if @ {} }\n", + &[ + ("b", 1), + ("(b)", 1), + ("!b", 1), + ("!(b)", 1), + ("-b", 0), + ("(-b)", 0), + ], + ); + assert_operands( + LANG::Rust, + "fn f(b: bool) { g(@); }\n", + &[("!b", 1), ("(b)", 0), ("-b", 0)], + ); + } + + #[test] + #[cfg(feature = "go")] + fn go_condition_slot_routes_what_the_peel_accepts() { + assert_operands( + LANG::Go, + "package p\nfunc f(b bool) { if @ {} }\n", + &[ + ("b", 1), + ("(b)", 1), + ("!b", 1), + ("!(b)", 1), + ("-b", 0), + ("(-b)", 0), + ], + ); + assert_operands( + LANG::Go, + "package p\nfunc f(b bool) { g(@) }\n", + &[("!b", 1), ("(b)", 0), ("-b", 0)], + ); + } + + /// Java's `if` hands the peel its `condition` directly; the ternary + /// condition is the slot that goes through `java_count_condition`, + /// so it is the one under test. The `?` itself is the base 1. + #[test] + #[cfg(feature = "java")] + fn java_condition_slot_routes_what_the_peel_accepts() { + assert_operands( + LANG::Java, + "class A { int f(boolean b) { return @ ? 1 : 2; } }\n", + &[ + ("b", 2), + ("(b)", 2), + ("!b", 2), + ("!(b)", 2), + ("-b", 1), + ("(-b)", 1), + ], + ); + assert_operands( + LANG::Java, + "class A { void f(boolean b) { g(@); } }\n", + &[("!b", 1), ("(b)", 0), ("-b", 0)], + ); + } + + /// Python peels parentheses only: `not` is counted by its own + /// `NotOperator` arm, so `python_wrapper_operand` carries no + /// proves-boolean flag and there is no outside-a-slot half to pin. + #[test] + #[cfg(feature = "python")] + fn python_condition_slot_routes_what_the_peel_accepts() { + assert_operands( + LANG::Python, + "def f(b):\n if @:\n pass\n", + &[("b", 1), ("(b)", 1), ("((b))", 1), ("-b", 0), ("(-b)", 0)], + ); + } + + /// Ruby has no counted call-argument slot, so the outside-a-slot + /// half reads a ternary *branch*, which is type-free: only a + /// negation there proves its operand boolean. + #[test] + #[cfg(feature = "ruby")] + fn ruby_condition_slot_routes_what_the_peel_accepts() { + assert_operands( + LANG::Ruby, + "def f(b)\n if @ then 1 end\nend\n", + &[ + ("b", 1), + ("(b)", 1), + ("!b", 1), + ("not b", 1), + ("!(b)", 1), + ("-b", 0), + ("(-b)", 0), + ], + ); + assert_operands( + LANG::Ruby, + "def f(b, c)\n x = c ? 1 : @\nend\n", + &[("!b", 3), ("not b", 3), ("(b)", 2), ("-b", 2)], + ); + } + + #[test] + #[cfg(feature = "lua")] + fn lua_condition_slot_routes_what_the_peel_accepts() { + assert_operands( + LANG::Lua, + "function f(b)\n if @ then end\nend\n", + &[ + ("b", 1), + ("(b)", 1), + ("not b", 1), + ("not (b)", 1), + ("-b", 0), + ("#b", 0), + ("(-b)", 0), + ], + ); + assert_operands( + LANG::Lua, + "function f(b)\n g(@)\nend\n", + &[("not b", 1), ("(b)", 0), ("-b", 0)], + ); + } +} diff --git a/src/metrics/abc/go.rs b/src/metrics/abc/go.rs index 13890714c..af78ccaee 100644 --- a/src/metrics/abc/go.rs +++ b/src/metrics/abc/go.rs @@ -13,6 +13,28 @@ use super::{Abc, Stats, for_each_named_child}; use crate::macros::go_bool_terminal_kinds; use crate::*; +// One step of the `(...)` / `!` peel: the operand a wrapper wraps, and +// whether the wrapper itself proves that operand boolean. `None` for +// anything this peel does not descend, which is also the answer +// `go_count_condition` asks for, so the slot and the peel cannot +// disagree about which kinds are wrappers (#1470; the Kotlin, Groovy +// and C# instances of that disagreement were #1459, #1466 and #1463). +// Both wrappers store their operand at child(1), after the `(` or the +// operator token. A `unary_expression` spelled with any other operator +// (`-x`, `^x`, `*p`, `&v`, `<-ch`) is never a boolean slot's operand, +// so the peel declines it. +fn go_wrapper_operand<'a>(node: &Node<'a>) -> Option<(Node<'a>, bool)> { + use Go as G; + + match node.kind_id().into() { + G::ParenthesizedExpression => Some((node.child(1)?, false)), + G::UnaryExpression if node.child(0)?.kind_id() == G::BANG as u16 => { + Some((node.child(1)?, true)) + } + _ => None, + } +} + // Go ABC unary-conditional walker (issue #403; see `rust_inspect_container` // for the cross-language rationale). Terminal-bool kinds include calls, // selector access (`r.Field`), index access (`xs[i]`), and type @@ -22,29 +44,16 @@ fn go_inspect_container(container_node: &Node, parent: &Node, conditions: &mut f use Go as G; let mut node = *container_node; - let mut node_kind = node.kind_id().into(); let mut has_boolean_content = matches!( parent.kind_id().into(), G::BinaryExpression | G::IfStatement | G::ForStatement ); - loop { - let is_parens = matches!(node_kind, G::ParenthesizedExpression); - let is_not = matches!(node_kind, G::UnaryExpression) - && node.child(0).is_some_and(|c| c.kind_id() == G::BANG as u16); + while let Some((operand, proves_boolean)) = go_wrapper_operand(&node) { + has_boolean_content |= proves_boolean; + node = operand; - if !is_parens && !is_not { - break; - } - if !has_boolean_content && is_not { - has_boolean_content = true; - } - - let Some(child) = node.child(1) else { break }; - node = child; - node_kind = node.kind_id().into(); - - if matches!(node_kind, go_bool_terminal_kinds!()) { + if matches!(node.kind_id().into(), go_bool_terminal_kinds!()) { if has_boolean_content { *conditions += 1.; } @@ -55,11 +64,13 @@ fn go_inspect_container(container_node: &Node, parent: &Node, conditions: &mut f // Phase-2B (issue #403): condition-slot dispatcher for Go. fn go_count_condition(condition: &Node, parent: &Node, conditions: &mut f64) { - use Go as G; - let kind = condition.kind_id().into(); - if matches!(kind, go_bool_terminal_kinds!()) { + if matches!(condition.kind_id().into(), go_bool_terminal_kinds!()) { *conditions += 1.; - } else if matches!(kind, G::ParenthesizedExpression | G::UnaryExpression) { + } else if go_wrapper_operand(condition).is_some() { + // Asking the peel itself which kinds it unwraps, rather than + // restating the list here (#1470): a restated list that gained a + // kind the peel lacked would read as covering a shape the peel + // then dropped (`.claude/rules/grammar-dispatch.md` §7). go_inspect_container(condition, parent, conditions); } } diff --git a/src/metrics/abc/java.rs b/src/metrics/abc/java.rs index 174647e2d..f9fa1d494 100644 --- a/src/metrics/abc/java.rs +++ b/src/metrics/abc/java.rs @@ -13,13 +13,36 @@ use super::{Abc, Stats}; use crate::macros::java_bool_terminal_kinds; use crate::*; +// One step of the `(...)` / `!` peel: the operand a wrapper wraps, and +// whether the wrapper itself proves that operand boolean (`return (!x);` +// is boolean whatever `x` is; a parenthesis preserves the type). `None` +// for anything this peel does not descend, which is also the answer +// `java_count_condition` asks for, so the slot and the peel cannot +// disagree about which kinds are wrappers (#1470; the Kotlin, Groovy +// and C# instances of that disagreement were #1459, #1466 and #1463). +// A `unary_expression` spelled `-x`, `+x` or `~x` is never a boolean +// slot's operand, so the peel declines it. +// +// Both wrappers store their expression at child index one, after the +// `(` or the operator token: +// https://github.com/tree-sitter/tree-sitter-java/blob/master/src/grammar.json#L2472 +// https://github.com/tree-sitter/tree-sitter-java/blob/master/src/grammar.json#L2150 +fn java_wrapper_operand<'a>(node: &Node<'a>) -> Option<(Node<'a>, bool)> { + use Java::*; + + match node.kind_id().into() { + ParenthesizedExpression => Some((node.child(1)?, false)), + UnaryExpression if node.child(0)?.kind_id() == BANG as u16 => Some((node.child(1)?, true)), + _ => None, + } +} + // Inspects the content of Java parenthesized expressions // and `Not` operators to find unary conditional expressions fn java_inspect_container(container_node: &Node, parent: &Node, conditions: &mut f64) { use Java::*; let mut node = *container_node; - let mut node_kind = node.kind_id().into(); // Initializes the flag to true if the container is known to contain a boolean value // `Guard` joined this list with #1454: a `case … when g ->` guard is @@ -37,35 +60,9 @@ fn java_inspect_container(container_node: &Node, parent: &Node, conditions: &mut }; // Looks inside parenthesized expressions and `Not` operators to find what they contain - loop { - // Checks if the node is a parenthesized expression or a `Not` operator - // The child node of index 0 contains the unary expression operator (we look for the `!` operator) - let is_parenthesised_exp = matches!(node_kind, ParenthesizedExpression); - let is_not_operator = matches!(node_kind, UnaryExpression) - && node - .child(0) - .is_some_and(|c| matches!(c.kind_id().into(), BANG)); - - // Stops the exploration if the node is neither - // a parenthesized expression nor a `Not` operator - if !is_parenthesised_exp && !is_not_operator { - break; - } - - // Sets the flag to true if a `Not` operator is found - // This is used to prove if a variable or a value returned by a method is actually boolean - // e.g. `return (!x);` - if !has_boolean_content && is_not_operator { - has_boolean_content = true; - } - - // Parenthesized expressions and `Not` operators nodes - // always store their expressions in the children nodes of index one - // https://github.com/tree-sitter/tree-sitter-java/blob/master/src/grammar.json#L2472 - // https://github.com/tree-sitter/tree-sitter-java/blob/master/src/grammar.json#L2150 - let Some(child) = node.child(1) else { break }; - node = child; - node_kind = node.kind_id().into(); + while let Some((operand, proves_boolean)) = java_wrapper_operand(&node) { + has_boolean_content |= proves_boolean; + node = operand; // Stops the exploration when the content is found. The terminal // set includes `FieldAccess` (`obj.flag`), `CastExpression` @@ -75,7 +72,7 @@ fn java_inspect_container(container_node: &Node, parent: &Node, conditions: &mut // `InstanceofExpression` was a fifth until #1461 moved it to // an unconditional arm: it is an operator, so it scores by use // rather than only where this walker looks. - if matches!(node_kind, java_bool_terminal_kinds!()) { + if matches!(node.kind_id().into(), java_bool_terminal_kinds!()) { if has_boolean_content { *conditions += 1.; } @@ -409,13 +406,14 @@ fn java_walk_ternary(node: &Node, stats: &mut Stats) { // Mirrors // `csharp_count_condition` / `groovy_count_condition`. fn java_count_condition(condition: &Node, parent: &Node, conditions: &mut f64) { - use Java::*; - match condition.kind_id().into() { - java_bool_terminal_kinds!() => *conditions += 1., - ParenthesizedExpression | UnaryExpression => { - java_inspect_container(condition, parent, conditions); - } - _ => {} + if matches!(condition.kind_id().into(), java_bool_terminal_kinds!()) { + *conditions += 1.; + } else if java_wrapper_operand(condition).is_some() { + // Asking the peel itself which kinds it unwraps, rather than + // restating the list here (#1470): a restated list that gained a + // kind the peel lacked would read as covering a shape the peel + // then dropped (`.claude/rules/grammar-dispatch.md` §7). + java_inspect_container(condition, parent, conditions); } } diff --git a/src/metrics/abc/lua.rs b/src/metrics/abc/lua.rs index 8bdecc8b0..63b0f0fe7 100644 --- a/src/metrics/abc/lua.rs +++ b/src/metrics/abc/lua.rs @@ -30,6 +30,30 @@ use crate::*; // operators `and` / `or` are deliberately NOT counted; see the // module-level `Stats` doc-comment for the cross-language policy // (issue #395, walker tracked in #403). +// One step of the `(...)` / `not` peel: the operand a wrapper wraps, and +// whether the wrapper itself proves that operand boolean. `None` for +// anything this peel does not descend, which is also the answer +// `lua_count_condition` asks for, so the slot and the peel cannot +// disagree about which kinds are wrappers (#1470; the Kotlin, Groovy +// and C# instances of that disagreement were #1459, #1466 and #1463). +// Both wrappers store their operand at child(1), after the `(` or the +// `not` keyword. A `unary_expression` spelled `-x`, `#t` or `~x` is +// arithmetic, length or bitwise — never a boolean slot's operand — so +// the peel declines it. +// +// A `not` proves the operand boolean even when the parent context does +// not — matching the JS / Java pattern. Without that, `m(not a)` and +// other call-argument contexts would never set the walker's flag. +fn lua_wrapper_operand<'a>(node: &Node<'a>) -> Option<(Node<'a>, bool)> { + match node.kind_id().into() { + Lua::ParenthesizedExpression => Some((node.child(1)?, false)), + Lua::UnaryExpression if node.child(0)?.kind_id() == Lua::Not as u16 => { + Some((node.child(1)?, true)) + } + _ => None, + } +} + // Lua ABC unary-conditional walker (Fitzpatrick Rule 9; issue #403). // Lua's logical operators are keyword tokens (`and` / `or`) inside a // `binary_expression`; `not x` is a `unary_expression` whose first @@ -38,33 +62,16 @@ use crate::*; // every call / indexing form. fn lua_inspect_container(container_node: &Node, parent: &Node, conditions: &mut f64) { let mut node = *container_node; - let mut node_kind = node.kind_id().into(); let mut has_boolean_content = matches!( parent.kind_id().into(), Lua::BinaryExpression | Lua::IfStatement | Lua::WhileStatement | Lua::RepeatStatement ); - loop { - let is_parens = matches!(node_kind, Lua::ParenthesizedExpression); - let is_not = matches!(node_kind, Lua::UnaryExpression) - && node - .child(0) - .is_some_and(|c| c.kind_id() == Lua::Not as u16); - - if !is_parens && !is_not { - break; - } - // A `not` wrapper proves the operand is boolean even when - // the parent context didn't — matches the JS/Java/etc. - // pattern. Without this, `m(not a)` and other call-argument - // contexts would never propagate has_boolean_content. - has_boolean_content |= is_not; - - let Some(child) = node.child(1) else { break }; - node = child; - node_kind = node.kind_id().into(); + while let Some((operand, proves_boolean)) = lua_wrapper_operand(&node) { + has_boolean_content |= proves_boolean; + node = operand; - if matches!(node_kind, lua_bool_terminal_kinds!()) { + if matches!(node.kind_id().into(), lua_bool_terminal_kinds!()) { if has_boolean_content { *conditions += 1.; } @@ -79,10 +86,13 @@ fn lua_inspect_container(container_node: &Node, parent: &Node, conditions: &mut // FunctionCall, etc.) count at the top level; `(...)` / `not ...` // route through `lua_inspect_container`. fn lua_count_condition(condition: &Node, parent: &Node, conditions: &mut f64) { - let kind = condition.kind_id().into(); - if matches!(kind, lua_bool_terminal_kinds!()) { + if matches!(condition.kind_id().into(), lua_bool_terminal_kinds!()) { *conditions += 1.; - } else if matches!(kind, Lua::ParenthesizedExpression | Lua::UnaryExpression) { + } else if lua_wrapper_operand(condition).is_some() { + // Asking the peel itself which kinds it unwraps, rather than + // restating the list here (#1470): a restated list that gained a + // kind the peel lacked would read as covering a shape the peel + // then dropped (`.claude/rules/grammar-dispatch.md` §7). lua_inspect_container(condition, parent, conditions); } } diff --git a/src/metrics/abc/python.rs b/src/metrics/abc/python.rs index dfdad6f6d..6a2b85af5 100644 --- a/src/metrics/abc/python.rs +++ b/src/metrics/abc/python.rs @@ -41,6 +41,25 @@ use crate::*; // paper's "unary conditional expression". See the module-level // `Stats` doc-comment for the cross-language `&&` / `||` policy // (issue #395, walker tracked in #403). +// One step of the `(...)` peel: the expression a parenthesis wraps, at +// child(1) after the `(` token. `None` for anything else, which is also +// the answer `python_count_condition` asks for, so the slot and the peel +// cannot disagree about which kinds are wrappers (#1470; the Kotlin, +// Groovy and C# instances of that disagreement were #1459, #1466 and +// #1463). +// +// Unlike its siblings this returns no "proves boolean" flag, because no +// wrapper it accepts can prove one: `not` is deliberately not peeled — +// the `NotOperator` arm counts it (see the walker below) — and a +// parenthesis preserves the type of what it wraps. +fn python_wrapper_operand<'a>(node: &Node<'a>) -> Option> { + if matches!(node.kind_id().into(), Python::ParenthesizedExpression) { + node.child(1) + } else { + None + } +} + // Python ABC unary-conditional walker (Fitzpatrick Rule 9 / Figure 4; // issue #403). Python's `a and b` parses as `boolean_operator` (NOT // `binary_operator`), so the walker triggers on the `And` / `Or` @@ -59,7 +78,6 @@ fn python_inspect_container(container_node: &Node, parent: &Node, conditions: &m use Python::*; let mut node = *container_node; - let mut node_kind = node.kind_id().into(); // `IfClause` joined this list with #1454: a `case … if g:` guard is // a boolean slot exactly as an `if` condition is, so a parenthesised // guard operand (`case n if (b):`) counts where the bare @@ -69,16 +87,10 @@ fn python_inspect_container(container_node: &Node, parent: &Node, conditions: &m BooleanOperator | IfStatement | WhileStatement | ConditionalExpression | IfClause ); - loop { - if !matches!(node_kind, ParenthesizedExpression) { - break; - } + while let Some(operand) = python_wrapper_operand(&node) { + node = operand; - let Some(child) = node.child(1) else { break }; - node = child; - node_kind = node.kind_id().into(); - - if matches!(node_kind, python_bool_terminal_kinds!()) { + if matches!(node.kind_id().into(), python_bool_terminal_kinds!()) { if has_boolean_content { *conditions += 1.; } @@ -98,11 +110,13 @@ fn python_inspect_container(container_node: &Node, parent: &Node, conditions: &m // (the `Or`/`And` keyword walker, the `NotOperator` arm, and // the `ComparisonOperator` arm at lines `~1334-1390`). fn python_count_condition(condition: &Node, parent: &Node, conditions: &mut f64) { - use Python::*; - let kind = condition.kind_id().into(); - if matches!(kind, python_bool_terminal_kinds!()) { + if matches!(condition.kind_id().into(), python_bool_terminal_kinds!()) { *conditions += 1.; - } else if matches!(kind, ParenthesizedExpression) { + } else if python_wrapper_operand(condition).is_some() { + // Asking the peel itself which kinds it unwraps, rather than + // restating the list here (#1470): a restated list that gained a + // kind the peel lacked would read as covering a shape the peel + // then dropped (`.claude/rules/grammar-dispatch.md` §7). python_inspect_container(condition, parent, conditions); } } diff --git a/src/metrics/abc/ruby.rs b/src/metrics/abc/ruby.rs index 78030902a..b2c3ee46c 100644 --- a/src/metrics/abc/ruby.rs +++ b/src/metrics/abc/ruby.rs @@ -37,22 +37,53 @@ use crate::*; // condition appears as the inner comparison); the `Then` clause is // an implicit grammar wrapper around every `if` / `elsif` body and // is NOT counted as a separate arm. +// One step of the `(...)` / negation peel: the operand a wrapper wraps, +// and whether the wrapper itself proves that operand boolean. `None` +// for anything that is not a wrapper this peel descends, which is also +// the answer `ruby_count_condition` asks for — it routes exactly the +// kinds this function accepts, so the two cannot disagree (#1470; the +// Kotlin, Groovy and C# instances of that disagreement were #1459, +// #1466 and #1463). +// +// Both spellings of the same negation. `not` and `!` differ in +// precedence but not in meaning, and ABC counts the negation, not the +// parse — testing `BANG` alone scored `if not b` as 0 where `if !b` +// scores 1, and a `not` ternary as 2 where the `!` form scores 4 +// (#1182). The operator is read through the grammar's `operator` field +// rather than child(0), matching the ternary slots (#1181). The other +// `unary` operators (`-b`, `~b`, `defined?`) are arithmetic or +// introspection, never a boolean slot's operand, so the peel declines. +// +// A negation stores its operand at child index 1, after the operator +// token. `parenthesized_statements` wraps its body in named children; +// the first named child carries the expression. +fn ruby_wrapper_operand<'a>(node: &Node<'a>) -> Option<(Node<'a>, bool)> { + use Ruby::*; + + match node.kind_id().into() { + ParenthesizedStatements => Some((node.children().find(Node::is_named)?, false)), + Unary | Unary2 | Unary3 | Unary4 | Unary5 => { + match node.child_by_field_name("operator")?.kind_id().into() { + BANG | Not => Some((node.child(1)?, true)), + _ => None, + } + } + _ => None, + } +} + // Ruby ABC unary-conditional walker (Fitzpatrick Rule 9; issue #557). // tree-sitter-ruby parses `a && b || c` as a left-nested chain of // `binary` nodes carrying `&&` / `||` / `and` / `or` operator tokens // (the `binary` kind is aliased `Binary`..`Binary3` per lesson #2, so // every alias must be matched). Negation surfaces as `unary` -// (`Unary`..`Unary5`) whose child(0) is the `!` token; the condition -// slot may be wrapped in `parenthesized_statements`. Both are unwrapped -// by `ruby_inspect_container`. +// (`Unary`..`Unary5`); the condition slot may be wrapped in +// `parenthesized_statements`. Both are unwrapped one layer at a time by +// `ruby_wrapper_operand`. fn ruby_inspect_container(container_node: &Node, parent: &Node, conditions: &mut f64) { - // bca: suppress(cognitive) — wrapper-peeling state machine, clearest whole - // See `cpp_inspect_container` for the shared rationale: one loop peels - // `(...)` / `!...` layers while carrying a single boolean-context flag. use Ruby::*; let mut node = *container_node; - let mut node_kind = node.kind_id().into(); let parent_kind = parent.kind_id().into(); // A ternary seeds boolean context for its condition slot alone: the // two branch operands are type-free, so an unnegated branch must @@ -81,40 +112,11 @@ fn ruby_inspect_container(container_node: &Node, parent: &Node, conditions: &mut .child_by_field_name("condition") .is_some_and(|condition| condition.id() == node.id())); - loop { - let is_parens = matches!(node_kind, ParenthesizedStatements); - // Both spellings of the same negation. `not` and `!` differ in - // precedence but not in meaning, and ABC counts the negation, - // not the parse — testing `BANG` alone scored `if not b` as 0 - // where `if !b` scores 1, and a `not` ternary as 2 where the `!` - // form scores 4 (#1182). Read through the grammar's `operator` - // field rather than child(0), matching the ternary slots (#1181). - let is_not = matches!(node_kind, Unary | Unary2 | Unary3 | Unary4 | Unary5) - && node - .child_by_field_name("operator") - .is_some_and(|op| matches!(op.kind_id().into(), BANG | Not)); + while let Some((operand, proves_boolean)) = ruby_wrapper_operand(&node) { + has_boolean_content |= proves_boolean; + node = operand; - if !is_parens && !is_not { - break; - } - if !has_boolean_content && is_not { - has_boolean_content = true; - } - - // A `!` unary stores its operand at child index 1 (after the `!` - // token). `parenthesized_statements` wraps its body in named - // children; descend through the first named child carrying the - // expression. - let next = if is_not { - node.child(1) - } else { - node.children().find(Node::is_named) - }; - let Some(child) = next else { break }; - node = child; - node_kind = node.kind_id().into(); - - if matches!(node_kind, ruby_bool_terminal_kinds!()) { + if matches!(node.kind_id().into(), ruby_bool_terminal_kinds!()) { if has_boolean_content { *conditions += 1.; } @@ -167,14 +169,15 @@ fn ruby_count_unary_conditions(list_node: &Node, conditions: &mut f64) { // counted them, breaking the conditions >= decisions invariant // (#469/#473/#456); issue #696. fn ruby_count_condition(condition: &Node, parent: &Node, conditions: &mut f64) { - use Ruby::*; - let kind = condition.kind_id().into(); - if matches!(kind, ruby_bool_terminal_kinds!()) { + if matches!(condition.kind_id().into(), ruby_bool_terminal_kinds!()) { *conditions += 1.; - } else if matches!( - kind, - ParenthesizedStatements | Unary | Unary2 | Unary3 | Unary4 | Unary5 - ) { + } else if ruby_wrapper_operand(condition).is_some() { + // Asking the peel itself which kinds it unwraps, rather than + // restating the list here (#1470). A restated list that gained + // a kind the peel lacked would read as covering a shape the + // peel then dropped on the floor (`.claude/rules/ + // grammar-dispatch.md` §7) — the Kotlin and Groovy defects of + // #1459 / #1466. ruby_inspect_container(condition, parent, conditions); } } diff --git a/src/metrics/abc/rust.rs b/src/metrics/abc/rust.rs index a789f52c4..fdfaf6790 100644 --- a/src/metrics/abc/rust.rs +++ b/src/metrics/abc/rust.rs @@ -13,6 +13,25 @@ use super::{Abc, Stats}; use crate::macros::rust_bool_terminal_kinds; use crate::*; +// One step of the `(...)` / `!` peel: the operand a wrapper wraps, and +// whether the wrapper itself proves that operand boolean. `None` for +// anything this peel does not descend, which is also the answer +// `rust_count_condition` asks for, so the slot and the peel cannot +// disagree about which kinds are wrappers (#1470; the Kotlin, Groovy +// and C# instances of that disagreement were #1459, #1466 and #1463). +// Both wrappers store their operand at child(1), after the `(` or the +// operator token. A `unary_expression` spelled `-x` or `*p` is never a +// boolean slot's operand, so the peel declines it. +fn rust_wrapper_operand<'a>(node: &Node<'a>) -> Option<(Node<'a>, bool)> { + use Rust::*; + + match node.kind_id().into() { + ParenthesizedExpression => Some((node.child(1)?, false)), + UnaryExpression if node.child(0)?.kind_id() == BANG as u16 => Some((node.child(1)?, true)), + _ => None, + } +} + // Rust ABC unary-conditional walker (Fitzpatrick Rule 7 / Listing 2). // // On every `&&` / `||` token, we walk the parent `binary_expression` @@ -34,7 +53,6 @@ fn rust_inspect_container(container_node: &Node, parent: &Node, conditions: &mut use Rust::*; let mut node = *container_node; - let mut node_kind = node.kind_id().into(); // `MatchPattern` joined this list with #1454: a match guard // (`n if g =>`) is a boolean slot exactly as an `if` condition is, // so a parenthesised guard operand (`n if (b)`) counts where the @@ -44,23 +62,11 @@ fn rust_inspect_container(container_node: &Node, parent: &Node, conditions: &mut BinaryExpression | IfExpression | WhileExpression | LetChain | LetChain2 | MatchPattern ); - 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); + while let Some((operand, proves_boolean)) = rust_wrapper_operand(&node) { + has_boolean_content |= proves_boolean; + node = operand; - if !is_parens && !is_not { - break; - } - if !has_boolean_content && is_not { - has_boolean_content = true; - } - - let Some(child) = node.child(1) else { break }; - node = child; - node_kind = node.kind_id().into(); - - if matches!(node_kind, rust_bool_terminal_kinds!()) { + if matches!(node.kind_id().into(), rust_bool_terminal_kinds!()) { if has_boolean_content { *conditions += 1.; } @@ -78,11 +84,13 @@ fn rust_inspect_container(container_node: &Node, parent: &Node, conditions: &mut // the `java_count_condition` / `java_inspect_child` helper pair used // by `java_walk_ternary` / `java_walk_for_statement`. fn rust_count_condition(condition: &Node, parent: &Node, conditions: &mut f64) { - use Rust::*; - let kind = condition.kind_id().into(); - if matches!(kind, rust_bool_terminal_kinds!()) { + if matches!(condition.kind_id().into(), rust_bool_terminal_kinds!()) { *conditions += 1.; - } else if matches!(kind, ParenthesizedExpression | UnaryExpression) { + } else if rust_wrapper_operand(condition).is_some() { + // Asking the peel itself which kinds it unwraps, rather than + // restating the list here (#1470): a restated list that gained a + // kind the peel lacked would read as covering a shape the peel + // then dropped (`.claude/rules/grammar-dispatch.md` §7). rust_inspect_container(condition, parent, conditions); } } From 272bc9c32bf3a63d5b6614c1d18bbbeadd4f0ca1 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic <490392+dekobon@users.noreply.github.com> Date: Wed, 7 Oct 2026 23:20:51 -0700 Subject: [PATCH 08/17] fix(abc/cpp): score ISO alternative operator tokens C++ spells `!`, `&&`, `||` and `!=` equally as `not`, `and`, `or` and `not_eq` ([lex.digraph]), and tree-sitter-cpp and the vendored tree-sitter-mozcpp give each alternative its own token kind. ABC matched only the symbols, so `if (not b)` scored 0 against 1 for `if (!b)`, `b and c` scored 0 against 2 for `b && c`, and `b not_eq c` scored 0 against 1 for `b != c`. The shared peel now accepts `not` beside `!`, and the Cpp and Mozcpp dispatchers add `And` / `Or` to the Rule 9 chain arm and `NotEq` to the comparison arm. Each alternative occurs in a subset of its symbol's grammar productions, so the existing parent gates decide it unchanged. C and Objective-C have no such tokens (`not` is an macro the parser reads as an identifier), so their scores do not move. Cyclomatic, cognitive and Halstead have the same gap; that is left to a follow-up. --- src/metrics/abc.rs | 57 +++++++++++++++++++++++++++++++++++++++ src/metrics/abc/cpp.rs | 24 +++++++++++++---- src/metrics/abc/mozcpp.rs | 8 +++--- 3 files changed, 81 insertions(+), 8 deletions(-) diff --git a/src/metrics/abc.rs b/src/metrics/abc.rs index 63be76c91..bedcf66d9 100644 --- a/src/metrics/abc.rs +++ b/src/metrics/abc.rs @@ -4654,6 +4654,63 @@ mod tests { ); } + // ISO C++ spells `!`, `&&`, `||` and `!=` equally as `not`, `and`, + // `or` and `not_eq` ([lex.digraph]), and the C++ grammars give each + // alternative token a kind of its own. ABC tested only the symbols, + // so `if (not b)` scored 0 against `if (!b)`'s 1, `b and c` scored 0 + // against `b && c`'s 2, and `b not_eq c` scored 0 against `b != c`'s + // 1. Each row pairs a symbolic spelling with its alternative and the + // value both must give; the census proves the alternative fixture + // really carries the alternative token rather than parsing it as an + // identifier, which is what tree-sitter-c does with the same text. + // + // Arguments: the `not`, `and`, `or` and `not_eq` ids of `P`'s grammar. + #[cfg(any(feature = "cpp", feature = "mozcpp"))] + fn assert_cpp_alternative_tokens_score_like_symbols(alt: [u16; 4]) { + let [not, and, or, not_eq] = alt; + let rows = [ + ("if (!b) {}", "if (not b) {}", not, 1), + ("bool x = !b && c;", "bool x = not b && c;", not, 2), + ("bool x = c ? !b : c;", "bool x = c ? not b : c;", not, 3), + ("bool x = b && c;", "bool x = b and c;", and, 2), + ("if (b || c) {}", "if (b or c) {}", or, 2), + ("if (b != c) {}", "if (b not_eq c) {}", not_eq, 1), + ]; + for (symbolic, alternative, token, expected) in rows { + let alt_source = format!("void f(bool b, bool c) {{\n {alternative}\n}}\n"); + assert_fixture_spells::

(&alt_source, "foo.cpp", &[(token, 1, alternative)]); + for source in [symbolic, alternative] { + let source = format!("void f(bool b, bool c) {{\n {source}\n}}\n"); + check_func_space::(&source, "foo.cpp", |space| { + assert_eq!( + space.metrics.abc.conditions_sum(), + expected, + "`{source}` must score like its symbolic spelling `{symbolic}`" + ); + }); + } + } + } + + #[cfg(feature = "cpp")] + #[test] + fn cpp_alternative_tokens_score_like_symbols() { + assert_cpp_alternative_tokens_score_like_symbols::( + [Cpp::Not, Cpp::And, Cpp::Or, Cpp::NotEq].map(|k| k as u16), + ); + } + + // Mozcpp owns no file extension, so this is its only coverage of the + // alternative tokens: no integration snapshot routes to it. + #[cfg(feature = "mozcpp")] + #[test] + fn mozcpp_alternative_tokens_score_like_symbols() { + use crate::Mozcpp; + assert_cpp_alternative_tokens_score_like_symbols::( + [Mozcpp::Not, Mozcpp::And, Mozcpp::Or, Mozcpp::NotEq].map(|k| k as u16), + ); + } + // The two other productions #1448's allowlist decides, alongside the // operator overloads above. // diff --git a/src/metrics/abc/cpp.rs b/src/metrics/abc/cpp.rs index 3a8cfcf4c..b40c1e5d4 100644 --- a/src/metrics/abc/cpp.rs +++ b/src/metrics/abc/cpp.rs @@ -68,8 +68,16 @@ pub(super) fn cpp_inspect_container(container_node: &Node, parent: &Node, condit // same way at child(1). `do { ... } while (...)`'s trailing // condition is a plain `parenthesized_expression`. let is_parens = matches!(node_kind, "parenthesized_expression" | "condition_clause"); - let is_not = - node_kind == "unary_expression" && node.child(0).is_some_and(|c| c.kind() == "!"); + // `not` is the ISO C++ alternative token for `!` ([lex.digraph]); + // the C++ grammars give it a kind of its own, so testing `!` + // alone scored `if (not b)` zero where `if (!b)` scores one. + // tree-sitter-c and tree-sitter-objc have no such token — there + // `not` is an `` macro the parser sees as an + // identifier — so the extra name is inert for them. + let is_not = node_kind == "unary_expression" + && node + .child(0) + .is_some_and(|c| matches!(c.kind(), "!" | "not")); if !is_parens && !is_not { break; @@ -301,7 +309,11 @@ impl Abc for CppCode { // counting too, as `<` / `>` always have. // // The test is `cpp_comparison_is_applied`, shared with Mozcpp. - LT | GT | LTEQ | GTEQ | EQEQ | BANGEQ | LTEQGT + // + // `NotEq` is `not_eq`, the ISO alternative token for `!=`. It + // occurs in exactly the productions `!=` does bar the + // preprocessor one, so the same allowlist decides it. + LT | GT | LTEQ | GTEQ | EQEQ | BANGEQ | NotEq | LTEQGT if ancestors .parent(node) .is_some_and(|parent| cpp_comparison_is_applied(node, &parent)) => @@ -309,8 +321,10 @@ impl Abc for CppCode { stats.conditions += 1.; } // Fitzpatrick Rule 9 (C++ in Figure 3): each operand of a - // `&&` / `||` chain is one condition (issue #403). - AMPAMP | PIPEPIPE => { + // `&&` / `||` chain is one condition (issue #403). `and` / + // `or` are the same operators spelled as ISO alternative + // tokens, which the grammar gives kinds of their own. + AMPAMP | PIPEPIPE | And | Or => { if let Some(parent) = ancestors.parent(node) { cpp_count_unary_conditions(&parent, &mut stats.conditions); } diff --git a/src/metrics/abc/mozcpp.rs b/src/metrics/abc/mozcpp.rs index 2820aaa65..9069d19d3 100644 --- a/src/metrics/abc/mozcpp.rs +++ b/src/metrics/abc/mozcpp.rs @@ -90,7 +90,7 @@ impl Abc for MozcppCode { // tree-sitter-cpp; the `grammar.json` sweep, the fold and // `#if` decisions, and the operator-overload defect (#1448) // are recorded on the matching arm of `CppCode::compute`. - LT | GT | LTEQ | GTEQ | EQEQ | BANGEQ | LTEQGT + LT | GT | LTEQ | GTEQ | EQEQ | BANGEQ | NotEq | LTEQGT if ancestors .parent(node) .is_some_and(|parent| cpp_comparison_is_applied(node, &parent)) => @@ -98,8 +98,10 @@ impl Abc for MozcppCode { stats.conditions += 1.; } // Fitzpatrick Rule 9 (C++ in Figure 3): each operand of a - // `&&` / `||` chain is one condition (issue #403). - AMPAMP | PIPEPIPE => { + // `&&` / `||` chain is one condition (issue #403). `and` / + // `or` are the same operators spelled as ISO alternative + // tokens, which the grammar gives kinds of their own. + AMPAMP | PIPEPIPE | And | Or => { if let Some(parent) = ancestors.parent(node) { cpp_count_unary_conditions(&parent, &mut stats.conditions); } From e34162827bd18e3960d06f0016cf73653ddaa81a Mon Sep 17 00:00:00 2001 From: Elijah Zupancic <490392+dekobon@users.noreply.github.com> Date: Wed, 7 Oct 2026 23:22:15 -0700 Subject: [PATCH 09/17] chore(baseline): refresh kind_sets.rs sloc after #1467 --- .bca-baseline.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.bca-baseline.toml b/.bca-baseline.toml index abb57103a..a1ed9af31 100644 --- a/.bca-baseline.toml +++ b/.bca-baseline.toml @@ -267,7 +267,7 @@ value = 120805.48245794396 path = "big-code-analysis-ast/src/macros/kind_sets.rs" qualified = "" metric = "loc.sloc" -value = 1357.0 +value = 1343.0 [[entry]] path = "big-code-analysis-ast/src/node.rs" From 6e53476addb3dfcfb914449c7a21d6a6f802524e Mon Sep 17 00:00:00 2001 From: Elijah Zupancic <490392+dekobon@users.noreply.github.com> Date: Wed, 7 Oct 2026 23:23:53 -0700 Subject: [PATCH 10/17] refactor(abc/ruby): reuse the peel in the when test `ruby_condition_scores_itself` restated the `(...)` / `!` / `not` peel that `ruby_wrapper_operand` now owns. It asks the peel instead, so the two cannot drift apart. Behaviour is unchanged. --- src/metrics/abc/ruby.rs | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/metrics/abc/ruby.rs b/src/metrics/abc/ruby.rs index b2c3ee46c..64dfc1025 100644 --- a/src/metrics/abc/ruby.rs +++ b/src/metrics/abc/ruby.rs @@ -37,6 +37,7 @@ use crate::*; // condition appears as the inner comparison); the `Then` clause is // an implicit grammar wrapper around every `if` / `elsif` body and // is NOT counted as a separate arm. + // One step of the `(...)` / negation peel: the operand a wrapper wraps, // and whether the wrapper itself proves that operand boolean. `None` // for anything that is not a wrapper this peel descends, which is also @@ -222,7 +223,7 @@ fn ruby_condition_scores_itself(expr: &Node) -> bool { let mut node = *expr; loop { - let next = match node.kind_id().into() { + match node.kind_id().into() { Binary | Binary2 | Binary3 => { return node.child_by_field_name("operator").is_some_and(|op| { let op = op.kind_id().into(); @@ -231,18 +232,13 @@ fn ruby_condition_scores_itself(expr: &Node) -> bool { }); } TestPattern | Conditional => return true, - ParenthesizedStatements => node.children().find(Node::is_named), - Unary | Unary2 | Unary3 | Unary4 | Unary5 - if node - .child_by_field_name("operator") - .is_some_and(|op| matches!(op.kind_id().into(), BANG | Not)) => - { - node.child_by_field_name("operand") - } - _ => return false, + _ => {} + } + // The same wrappers the condition peel descends, by construction. + let Some((operand, _)) = ruby_wrapper_operand(&node) else { + return false; }; - let Some(child) = next else { return false }; - node = child; + node = operand; } } From 432f19fe05b0abc3ce317ccc65be364ce8de2acf Mon Sep 17 00:00:00 2001 From: Elijah Zupancic <490392+dekobon@users.noreply.github.com> Date: Thu, 8 Oct 2026 00:04:40 -0700 Subject: [PATCH 11/17] fix(abc): read slots past interposed comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit tree-sitter counts a comment among a node's children and marks it named, so every ABC slot read by fixed child index or as "the first named child" could land on a comment and score the condition zero: `if (/*c*/ b)`, `if /*c*/ (b)`, `do {} while /*c*/ (b);`, `return /*c*/ !b;`, `(( /*c*/ b))`, `! /*c*/ b`. Two read sites were affected in nearly every language. Slot reads now go through the grammar's field (`condition`, `value`, `right`, `body`, Elixir's `operator`), or, where the grammar names none, the new `wrapped_operand` helper: the first named child that is not an `extra`. That covers C, C++, Mozcpp, ObjC, C#, Java, Groovy, the JS family, PHP, Python, Rust, Go, Lua and Perl. The field reads also fix two preambles: a C++ init-statement (`if (int y = g(); b)`) and a Rust loop label (`'a: while b`) both scored zero. Peel steps through `(...)` and negations take the operand the same way in every language, which also repairs Ruby's subject-less `when` (`when ( # c⏎ x > 1)` counted twice), Perl's `split( # sep⏎ /,/, $s)` delimiter test, Perl's last-operand read of a trailing comment, and Elixir's `=` read behind a comment (an assignment). Two tests that pinned the old zero for C# and Groovy now assert the fixed value. Corpus snapshots move only `abc.conditions`, upward: two pdf.js `if (/* … */ x)` functions, and serde's `while try!(…)`, whose 2015-edition macro parses to an ERROR at child(1) so the field read now finds the real condition. The self-scan baseline is refreshed: Go's and ObjC's `compute` effort fell. Fixes #1455 --- .bca-baseline.toml | 8 +- src/metrics/abc.rs | 488 +++++++++++++++++++- src/metrics/abc/c.rs | 42 +- src/metrics/abc/cpp.rs | 69 +-- src/metrics/abc/csharp.rs | 85 ++-- src/metrics/abc/elixir.rs | 30 +- src/metrics/abc/go.rs | 20 +- src/metrics/abc/groovy.rs | 53 +-- src/metrics/abc/java.rs | 47 +- src/metrics/abc/js_family.rs | 47 +- src/metrics/abc/kotlin.rs | 15 +- src/metrics/abc/lua.rs | 22 +- src/metrics/abc/mozcpp.rs | 46 +- src/metrics/abc/objc.rs | 23 +- src/metrics/abc/perl.rs | 94 ++-- src/metrics/abc/php.rs | 37 +- src/metrics/abc/python.rs | 26 +- src/metrics/abc/ruby.rs | 14 +- src/metrics/abc/rust.rs | 40 +- tests/repositories/big-code-analysis-output | 2 +- 20 files changed, 824 insertions(+), 384 deletions(-) diff --git a/.bca-baseline.toml b/.bca-baseline.toml index a1ed9af31..a92b9b0bf 100644 --- a/.bca-baseline.toml +++ b/.bca-baseline.toml @@ -897,7 +897,7 @@ value = 16.0 path = "src/metrics/abc/go.rs" qualified = "GoCode::compute" metric = "halstead.effort" -value = 49999.43426005393 +value = 49998.590597403396 [[entry]] path = "src/metrics/abc/kotlin.rs" @@ -905,12 +905,6 @@ qualified = "KotlinCode::compute" metric = "cyclomatic" value = 20.0 -[[entry]] -path = "src/metrics/abc/objc.rs" -qualified = "ObjcCode::compute" -metric = "halstead.effort" -value = 50110.01872952996 - [[entry]] path = "src/metrics/abc/rust.rs" qualified = "RustCode::compute" diff --git a/src/metrics/abc.rs b/src/metrics/abc.rs index bedcf66d9..3b467d139 100644 --- a/src/metrics/abc.rs +++ b/src/metrics/abc.rs @@ -432,6 +432,24 @@ pub(super) fn for_each_named_child( } } +// The operand a single-operand wrapper holds — `(…)`, a negation, a +// `return` — read as its first named child that is not a tree-sitter +// `extra`. A positional `child(1)` read assumes nothing sits between the +// opening token and the operand, and a comment can sit anywhere: `(/*c*/ +// b)` handed the comment to the peel, which declined it, and the +// condition scored zero (#1455). `is_named` alone is not enough either, +// because extras are named. +pub(super) fn wrapped_operand<'a>(node: &Node<'a>) -> Option> { + node.children().find(is_operand) +} + +// Whether `child` can occupy an operand slot: named, and not an `extra`. +// The `extra` flag is read off the tree-sitter node because the `Node` +// wrapper exposes no accessor for it. +pub(super) fn is_operand(child: &Node) -> bool { + child.is_named() && !child.as_tree_sitter().is_extra() +} + // Default no-op `Abc` impls. Audited in #188; the matrix below // records the rationale for every entry so the no-op default is a // deliberate choice, not scaffolding leftover. @@ -5267,15 +5285,13 @@ mod tests { // between it and the operator. The operator is identified by // membership rather than by index for the same reason. // - // The other two wrappers have no such luck and are pinned here as a - // *measured* gap rather than left to be discovered: the C# grammar - // gives `parenthesized_expression`, `prefix_unary_expression` and + // The other two wrappers name no field either — the C# grammar gives + // `parenthesized_expression`, `prefix_unary_expression` and // `postfix_unary_expression` an empty `fields` map in - // node-types.json, so the field read Kotlin's and Groovy's - // equivalents use is unavailable and `child(1)` is a comment - // whenever one is written there. That is #1455, which predates - // #1463 and is a separate change; what this test adds is that the - // new arm does not join it. + // node-types.json — so `child(1)` was a comment whenever one was + // written there, and `nc` / `rc` scored zero. They read their + // operand as the first one that is not an extra since #1455, so all + // three pairs agree with their controls. #[cfg(feature = "csharp")] #[test] fn csharp_null_forgiving_operand_survives_an_interposed_comment() { @@ -5309,17 +5325,15 @@ mod tests { &space.spaces[0], &[ ("p", 1, 2), - // The row #1463 fixes and the only one of the three - // pairs that agrees with its control. + // The row #1463 fixes. ("pc", 1, 2), ("n", 1, 2), // #1455: `child(1)` of the prefix wrapper is the - // comment. Delete this expectation, not the row, - // when that issue is fixed. - ("nc", 0, 2), + // comment. + ("nc", 1, 2), ("r", 1, 2), // #1455 again, through the paren wrapper. - ("rc", 0, 2), + ("rc", 1, 2), ], ); }); @@ -16725,10 +16739,8 @@ mod own_production_bool_constructs { /// the peel rewrite. /// /// `parenthesized_expression` names nothing in node-types.json, so - /// it keeps the positional read and keeps the bug; that half is - /// pinned here as a *measured* gap rather than left to be discovered - /// as a surprise. Kotlin records the identical pair - /// (`kotlin_wrapper_operand`). + /// it kept the positional read, and the bug, until #1455 read it as + /// the first operand that is not an extra. #[test] #[cfg(feature = "groovy")] fn groovy_negation_operand_survives_an_interposed_comment() { @@ -16744,9 +16756,9 @@ mod own_production_bool_constructs { assert_eq!(conditions(LANG::Groovy, &template.replace("{}", "(a)")), 1); assert_eq!( conditions(LANG::Groovy, &template.replace("{}", "( /*c*/ a)")), - 0, - "`parenthesized_expression` now survives an interposed comment — if the \ - grammar gained a field for its inner expression, read it and delete this" + 1, + "the parenthesised operand is being read positionally again; a comment \ + displaces it" ); } @@ -17848,3 +17860,437 @@ mod wrapper_peel_routing { ); } } + +/// A comment inside a condition or value slot changes nothing (#1455). +/// +/// Every walker read its slots by fixed child index, and every peel +/// stepped into a `(…)` / negation with `child(1)` or the first named +/// child. tree-sitter counts a comment among a node's children and +/// marks it *named*, so `if (/*c*/ b)` handed the comment to the slot, +/// which declined it, and the condition scored zero. Each pair below is +/// one spelling with and without a comment; the two are asserted equal +/// to each other *and* to the expected literal, so a regression that +/// moved both sides alike — to zero, say — still fails. +/// +/// The forms, per slot and per comment spelling of the language: a +/// comment before the operand, inside a nested parenthesis, before a +/// negation, between a negation and its operand, and — where the +/// language's syntax puts parentheses around the slot — before those +/// parentheses. Value slots (`return`, declarator, assignment, lambda +/// body) take the negated forms only, since a bare operand there is no +/// condition, and only the first comment spelling: a line comment after +/// `return` ends the statement in Go. +#[cfg(test)] +#[cfg(any( + feature = "c", + feature = "cpp", + feature = "csharp", + feature = "elixir", + feature = "go", + feature = "groovy", + feature = "java", + feature = "javascript", + feature = "kotlin", + feature = "lua", + feature = "mozcpp", + feature = "mozjs", + feature = "objc", + feature = "perl", + feature = "php", + feature = "python", + feature = "ruby", + feature = "rust", + feature = "typescript" +))] +mod slot_comment_invariance { + use crate::test_support::metrics_verbatim; + use crate::{LANG, MetricsOptions}; + + fn conditions(lang: LANG, source: &str) -> u64 { + metrics_verbatim(lang, source.as_bytes(), MetricsOptions::default()) + .abc + .conditions_sum() + } + + /// How one language spells the slots under test. + struct Spelling { + /// Comment spellings, each carrying the whitespace — a newline + /// for a line comment — the operand after it needs. + comments: &'static [&'static str], + operand: &'static str, + /// `None` where the peel does not descend the negation: Python + /// counts `not` through its own arm, and tree-sitter-python + /// fails to parse the valid `(not # c⏎ b)` at the pinned grammar. + negation: Option<&'static str>, + /// The slot is written inside parentheses: either the syntax + /// requires them (`if (…)`), or a line comment needs them to + /// stay inside the expression (Python, Ruby, Elixir). + parenthesised: bool, + /// The parentheses belong to the slot's syntax, so a comment + /// may also sit before them (`if /*c*/ (b)`). + syntactic_parens: bool, + /// `(template, expected)`; `@` marks a condition slot. + conditions: &'static [(&'static str, u64)], + /// Templates whose `@` is a value slot; a negation there scores 1. + values: &'static [&'static str], + } + + const C_COMMENTS: &[&str] = &["/*c*/ ", "//c\n "]; + + const C_FAMILY: Spelling = Spelling { + comments: C_COMMENTS, + operand: "b", + negation: Some("!"), + parenthesised: true, + syntactic_parens: true, + conditions: &[ + ("int f(){ if @ { x(); } }", 1), + ("int f(){ while @ { x(); } }", 1), + ("int f(){ do { x(); } while @; }", 1), + ], + values: &["int f(){ return @; }"], + }; + + const JS_FAMILY: Spelling = Spelling { + comments: C_COMMENTS, + operand: "b", + negation: Some("!"), + parenthesised: true, + syntactic_parens: true, + conditions: &[ + ("function f(){ if @ { x(); } }", 1), + ("function f(){ while @ { x(); } }", 1), + ("function f(){ do { x(); } while @; }", 1), + ], + values: &["function f(){ return @; }"], + }; + + fn spelling(lang: LANG) -> Option { + Some(match lang { + LANG::C | LANG::Cpp | LANG::Mozcpp | LANG::Objc => C_FAMILY, + LANG::Javascript | LANG::Mozjs | LANG::Typescript | LANG::Tsx => JS_FAMILY, + LANG::Java => Spelling { + conditions: &[ + ("class K{ void f(){ if @ { x(); } } }", 1), + ("class K{ void f(){ while @ { x(); } } }", 1), + ("class K{ void f(){ do { x(); } while @; } }", 1), + ], + values: &[ + "class K{ boolean f(){ return @; } }", + "class K{ void f(){ boolean y = @; } }", + "class K{ void f(){ y = @; } }", + "class K{ void f(){ Supplier s = () -> @; } }", + ], + ..C_FAMILY + }, + LANG::Csharp => Spelling { + conditions: &[ + ("class K{ void f(){ if @ { x(); } } }", 1), + ("class K{ void f(){ while @ { x(); } } }", 1), + ("class K{ void f(){ do { x(); } while @; } }", 1), + ], + values: &[ + "class K{ bool f(){ return @; } }", + "class K{ void f(){ var y = @; } }", + "class K{ void f(){ y = @; } }", + "class K{ void f(){ Func s = () => @; } }", + ], + ..C_FAMILY + }, + LANG::Groovy => Spelling { + conditions: &[ + ("def f(){ if @ { x() } }", 1), + ("def f(){ while @ { x() } }", 1), + ("def f(){ do { x() } while @ }", 1), + ], + values: &[ + "def f(){ return @ }", + "def f(){ def y = @ }", + "def f(){ y = @ }", + ], + ..C_FAMILY + }, + LANG::Php => Spelling { + comments: &["/*c*/ ", "//c\n ", "#c\n "], + operand: "$b", + conditions: &[ + (" Spelling { + conditions: &[ + ("fun f(){ if @ { x() } }", 1), + ("fun f(){ while @ { x() } }", 1), + ("fun f(){ do { x() } while @ }", 1), + ], + values: &[], + ..C_FAMILY + }, + LANG::Perl => Spelling { + comments: &["# c\n "], + operand: "$b", + conditions: &[ + ("sub f { if @ { x(); } }", 1), + ("sub f { unless @ { x(); } }", 1), + ("sub f { while @ { x(); } }", 1), + ("sub f { until @ { x(); } }", 1), + ], + values: &["sub f { return @; }"], + ..C_FAMILY + }, + LANG::Rust => Spelling { + parenthesised: false, + syntactic_parens: false, + conditions: &[ + ("fn f(){ if @ { x(); } }", 1), + ("fn f(){ while @ { x(); } }", 1), + ], + values: &["fn f() -> bool { return @; }"], + ..C_FAMILY + }, + LANG::Go => Spelling { + parenthesised: false, + syntactic_parens: false, + conditions: &[ + ("package p\nfunc f(){ if @ { x() } }", 1), + ("package p\nfunc f(){ for @ { x() } }", 1), + ], + values: &["package p\nfunc f() bool { return @ }"], + ..C_FAMILY + }, + LANG::Lua => Spelling { + comments: &["--[[c]] ", "--c\n "], + negation: Some("not "), + parenthesised: false, + syntactic_parens: false, + conditions: &[ + ("function f() if @ then x() end end", 1), + ("function f() while @ do x() end end", 1), + ("function f() repeat x() until @ end", 1), + ], + values: &["function f() return @ end"], + ..C_FAMILY + }, + LANG::Python => Spelling { + comments: &["# c\n "], + negation: None, + syntactic_parens: false, + conditions: &[ + ("def f():\n if @:\n x()\n", 1), + ("def f():\n while @:\n x()\n", 1), + ], + values: &[], + ..C_FAMILY + }, + LANG::Ruby => Spelling { + comments: &["# c\n "], + syntactic_parens: false, + conditions: &[ + ("def f\n if @\n x\n end\nend\n", 1), + ("def f\n while @\n x\n end\nend\n", 1), + ("def f\n case\n when @\n x\n end\nend\n", 1), + ], + values: &[], + ..C_FAMILY + }, + // Elixir's `if` slot scored its operand whatever the peel + // returned, so the defect showed only where the peel decides: + // an operand of a `&&` chain, which counts each operand. + LANG::Elixir => Spelling { + comments: &["# c\n "], + syntactic_parens: false, + conditions: &[ + ( + "defmodule M do\n def f do\n if @ do\n x()\n end\n end\nend\n", + 1, + ), + ("defmodule M do\n def f do\n y = a && @\n end\nend\n", 2), + ], + values: &[], + ..C_FAMILY + }, + _ => return None, + }) + } + + /// `(without, with, expected)` for every form of every slot. + fn cases(s: &Spelling) -> Vec<(String, String, u64)> { + let slot = |x: String| { + if s.parenthesised { format!("({x})") } else { x } + }; + let b = s.operand; + let mut out = Vec::new(); + for &(template, expected) in s.conditions { + let fill = |x: String| template.replace('@', &x); + for c in s.comments { + let mut push = |plain: String, commented: String| { + out.push((fill(plain), fill(commented), expected)); + }; + push(slot(b.into()), slot(format!("{c}{b}"))); + push(slot(format!("({b})")), slot(format!("({c}{b})"))); + if let Some(n) = s.negation { + push(slot(format!("{n}{b}")), slot(format!("{c}{n}{b}"))); + push(slot(format!("{n}{b}")), slot(format!("{n}{c}{b}"))); + } + if s.syntactic_parens { + push(slot(b.into()), format!("{c}{}", slot(b.into()))); + } + } + } + if let (Some(n), Some(c)) = (s.negation, s.comments.first()) { + for template in s.values { + let fill = |x: String| template.replace('@', &x); + out.push((fill(format!("{n}{b}")), fill(format!("{c}{n}{b}")), 1)); + out.push((fill(format!("{n}{b}")), fill(format!("{n}{c}{b}")), 1)); + } + } + out + } + + #[test] + fn a_comment_in_a_slot_changes_nothing() { + let mut checked = 0; + for lang in LANG::into_enum_iter() { + if !lang.is_enabled() { + continue; + } + let Some(spelling) = spelling(lang) else { + continue; + }; + for (plain, commented, expected) in cases(&spelling) { + let without = conditions(lang, &plain); + assert_eq!( + conditions(lang, &commented), + without, + "{lang:?}: a comment changed the ABC conditions\n \ + without: {plain}\n with: {commented}" + ); + assert_eq!(without, expected, "{lang:?}: {plain}"); + checked += 1; + } + } + assert!( + checked > 0, + "no slot language enabled; this test asserted nothing" + ); + } +} + +/// Slot occupants that no fixed index or first-named-child read finds +/// (#1455): a preamble before the slot, a comment *after* the operand +/// of a last-operand read, and the non-condition reads that were +/// positional too. Each row pairs a spelling with its plain twin. +#[cfg(test)] +#[cfg(any( + feature = "cpp", + feature = "elixir", + feature = "mozcpp", + feature = "perl", + feature = "ruby", + feature = "rust" +))] +mod slot_role_reads { + use crate::test_support::metrics_verbatim; + use crate::{LANG, MetricsOptions}; + + /// `(conditions, assignments)`. + fn abc(lang: LANG, source: &str) -> (u64, u64) { + let abc = metrics_verbatim(lang, source.as_bytes(), MetricsOptions::default()).abc; + (abc.conditions_sum(), abc.assignments_sum()) + } + + /// `(language, plain, variant, (conditions, assignments))`. + const ROWS: &[(LANG, &str, &str, (u64, u64))] = &[ + // A C++ `condition_clause` holds its condition in the `value` + // field; an init-statement sits at child(1) before it. + ( + LANG::Cpp, + "int f(){ int y = g(); if (b) { x(); } }", + "int f(){ if (int y = g(); b) { x(); } }", + (1, 1), + ), + ( + LANG::Mozcpp, + "int f(){ int y = g(); if (b) { x(); } }", + "int f(){ if (int y = g(); b) { x(); } }", + (1, 1), + ), + ( + LANG::Cpp, + "int f(){ if constexpr (b) { x(); } }", + "int f(){ if constexpr (/*c*/ b) { x(); } }", + (1, 0), + ), + // A loop label is the `while` expression's first child. + ( + LANG::Rust, + "fn f(){ while b { x(); } }", + "fn f(){ 'a: while b { x(); } }", + (1, 0), + ), + // Perl's `(…)` evaluates to its last element, so the peel takes + // the last operand — never a trailing comment. + ( + LANG::Perl, + "sub f { if ($b) { x(); } }", + "sub f { if ($b # c\n) { x(); } }", + (1, 0), + ), + ( + LANG::Perl, + "sub f { x() if $b; }", + "sub f { x() if $b # c\n; }", + (1, 0), + ), + // `split`'s delimiter is its first *operand*; a comment before it + // made the pattern read as an ordinary match. + ( + LANG::Perl, + "sub f { my @a = split(/,/, $s); }", + "sub f { my @a = split( # sep\n /,/, $s); }", + (0, 1), + ), + // A subject-less `when` scores its operand once; the comment + // made the peel return it, so `x > 1` counted twice. + ( + LANG::Ruby, + "def f\n case\n when (x > 1)\n y\n end\nend\n", + "def f\n case\n when ( # c\n x > 1)\n y\n end\nend\n", + (1, 0), + ), + // Elixir's match operator, read at child(1), lost to a comment + // before the `=` — an assignment rather than a condition. + ( + LANG::Elixir, + "defmodule M do\n def f do\n a = 1\n end\nend\n", + "defmodule M do\n def f do\n a # c\n = 1\n end\nend\n", + (0, 1), + ), + ]; + + #[test] + fn a_slot_is_read_by_role() { + let mut checked = 0; + for &(lang, plain, variant, expected) in ROWS { + if !lang.is_enabled() { + continue; + } + let without = abc(lang, plain); + assert_eq!( + abc(lang, variant), + without, + "{lang:?}: the variant scored differently\n plain: {plain}\n variant: {variant}" + ); + assert_eq!(without, expected, "{lang:?}: {plain}"); + checked += 1; + } + assert!( + checked > 0, + "no row language enabled; this test asserted nothing" + ); + } +} diff --git a/src/metrics/abc/c.rs b/src/metrics/abc/c.rs index f156b7341..10681d089 100644 --- a/src/metrics/abc/c.rs +++ b/src/metrics/abc/c.rs @@ -10,10 +10,9 @@ )] use super::cpp::{ - cpp_count_unary_conditions, cpp_inspect_child, cpp_inspect_container, cpp_walk_for_statement, - cpp_walk_ternary, + cpp_count_unary_conditions, cpp_inspect_slot, cpp_walk_for_statement, cpp_walk_ternary, }; -use super::{Abc, Stats}; +use super::{Abc, Stats, wrapped_operand}; use crate::*; impl Abc for CCode { @@ -98,29 +97,24 @@ impl Abc for CCode { cpp_count_unary_conditions(&parent, &mut stats.conditions); } } - // Phase-2B (issue #403): condition slots. C wraps every - // `if (...)` / `while (...)` / `do {…} while (...)` / - // `return value` in a paren / parenthesized expression - // (return is unparenthesized but its child(1) is the - // expression). `cpp_inspect_container` handles the - // `(...)` / `!...` unwrap so `if (true)` and `return !x` - // each count one condition; bare `return x` reports zero. - // Use `child_by_field_name("condition")` for if/while (C has - // no `if constexpr`, so child(1) is always the - // condition_clause). Return uses positional child(1) — its - // value field is always at index 1. - IfStatement | WhileStatement => { - if let Some(cond) = node.child_by_field_name("condition") { - cpp_inspect_container(&cond, node, &mut stats.conditions); - } + // Phase-2B (issue #403): condition slots. `if (...)` / + // `while (...)` / `do {…} while (...)` wrap their condition + // in a `parenthesized_expression` that + // `cpp_inspect_container` unwraps, so `if (true)` and + // `return !x` each count one condition; bare `return x` + // reports zero. Every slot is read by role, never by index: + // a comment shifts every later child (`do {} while /*c*/ + // (b);`, `return /*c*/ !b;` — #1455). `return` names no + // field; its value is its only operand. + IfStatement | WhileStatement | DoStatement => { + cpp_inspect_slot( + node.child_by_field_name("condition"), + node, + &mut stats.conditions, + ); } ReturnStatement => { - cpp_inspect_child(node, 1, &mut stats.conditions); - } - // `do { ... } while (cond);` — children: `do`, body, - // `while`, condition (parenthesized). Condition at child(3). - DoStatement => { - cpp_inspect_child(node, 3, &mut stats.conditions); + cpp_inspect_slot(wrapped_operand(node), node, &mut stats.conditions); } // `f(!a, !b)` — argument list walker. Two aliases — // `argument_list` is emitted as ArgumentList or diff --git a/src/metrics/abc/cpp.rs b/src/metrics/abc/cpp.rs index b40c1e5d4..c03b63874 100644 --- a/src/metrics/abc/cpp.rs +++ b/src/metrics/abc/cpp.rs @@ -9,7 +9,7 @@ clippy::cast_sign_loss )] -use super::{Abc, Stats}; +use super::{Abc, Stats, wrapped_operand}; use crate::macros::cpp_bool_terminal_kinds; use crate::*; @@ -63,11 +63,12 @@ pub(super) fn cpp_inspect_container(container_node: &Node, parent: &Node, condit loop { // `condition_clause` is the C++-grammar wrapper around an - // `if (...)` / `while (...)` head — same `(`, content, `)` - // shape as `parenthesized_expression`, so it unwraps the - // same way at child(1). `do { ... } while (...)`'s trailing - // condition is a plain `parenthesized_expression`. - let is_parens = matches!(node_kind, "parenthesized_expression" | "condition_clause"); + // `if (...)` / `while (...)` head — the same `(`, content, `)` + // shape as `parenthesized_expression`, plus an optional + // init-statement before the content. `do { ... } while (...)`'s + // trailing condition is a plain `parenthesized_expression`. + let is_clause = node_kind == "condition_clause"; + let is_parens = is_clause || node_kind == "parenthesized_expression"; // `not` is the ISO C++ alternative token for `!` ([lex.digraph]); // the C++ grammars give it a kind of its own, so testing `!` // alone scored `if (not b)` zero where `if (!b)` scores one. @@ -86,7 +87,15 @@ pub(super) fn cpp_inspect_container(container_node: &Node, parent: &Node, condit has_boolean_content = true; } - let Some(child) = node.child(1) else { break }; + // By role, never at child(1): a comment may sit there + // (`if (/*c*/ b)`, #1455), and so may a clause's init-statement + // (`if (int y = f(); b)`), whose condition is the `value` field. + let operand = if is_clause { + node.child_by_field_name("value") + } else { + wrapped_operand(&node) + }; + let Some(child) = operand else { break }; node = child; node_kind = node.kind(); @@ -105,9 +114,9 @@ pub(super) fn cpp_inspect_container(container_node: &Node, parent: &Node, condit // `(...)` / `!...` unwrap chain and the boolean-context seed from // the parent kind. No top-level terminal counter is needed because // the paren wrapper provides the unwrap step. -pub(super) fn cpp_inspect_child(node: &Node, idx: usize, conditions: &mut f64) { - if let Some(child) = node.child(idx) { - cpp_inspect_container(&child, node, conditions); +pub(super) fn cpp_inspect_slot(slot: Option, parent: &Node, conditions: &mut f64) { + if let Some(child) = slot { + cpp_inspect_container(&child, parent, conditions); } } @@ -329,31 +338,25 @@ impl Abc for CppCode { cpp_count_unary_conditions(&parent, &mut stats.conditions); } } - // Phase-2B (issue #403): condition slots. C++ wraps every - // `if (...)` / `while (...)` / `do {…} while (...)` / - // `return value` in a paren / parenthesized expression - // (return is unparenthesized but its child(1) is the - // expression). `cpp_inspect_container` handles the - // `(...)` / `!...` unwrap so `if (true)` and `return !x` - // each count one condition; bare `return x` reports zero. - // Use `child_by_field_name("condition")` for if/while so - // the `if constexpr (cond)` form (where child(1) is the - // `constexpr` keyword, not the condition_clause) is - // handled correctly. Return uses positional child(1) - // — its value field is always at index 1, no optional - // attribute precedes it. - IfStatement | WhileStatement => { - if let Some(cond) = node.child_by_field_name("condition") { - cpp_inspect_container(&cond, node, &mut stats.conditions); - } + // Phase-2B (issue #403): condition slots. `if (...)` / + // `while (...)` / `do {…} while (...)` wrap their condition + // in a paren / `condition_clause` node that + // `cpp_inspect_container` unwraps, so `if (true)` and + // `return !x` each count one condition; bare `return x` + // reports zero. Every slot is read by role, never by index: + // `if constexpr (cond)` puts the keyword at child(1), and a + // comment shifts every later child (`do {} while /*c*/ (b);`, + // `return /*c*/ !b;` — #1455). `return` names no field; its + // value is its only operand. + IfStatement | WhileStatement | DoStatement => { + cpp_inspect_slot( + node.child_by_field_name("condition"), + node, + &mut stats.conditions, + ); } ReturnStatement => { - cpp_inspect_child(node, 1, &mut stats.conditions); - } - // `do { ... } while (cond);` — children: `do`, body, - // `while`, condition (parenthesized). Condition at child(3). - DoStatement => { - cpp_inspect_child(node, 3, &mut stats.conditions); + cpp_inspect_slot(wrapped_operand(node), node, &mut stats.conditions); } // `f(!a, !b)` — argument list walker. Two aliases — // `argument_list` is emitted as ArgumentList or diff --git a/src/metrics/abc/csharp.rs b/src/metrics/abc/csharp.rs index c432a803c..b85e2094e 100644 --- a/src/metrics/abc/csharp.rs +++ b/src/metrics/abc/csharp.rs @@ -9,7 +9,7 @@ clippy::cast_sign_loss )] -use super::{Abc, Stats}; +use super::{Abc, Stats, is_operand, wrapped_operand}; use crate::macros::{ csharp_bool_terminal_kinds, csharp_paren_expr_kinds, csharp_prefix_unary_expr_kinds, }; @@ -57,18 +57,16 @@ use crate::*; // (`b /*c*/ !`); the operand is `child(0)`, which no extra can precede // because the node starts there. // -// The paren and prefix arms keep the positional reads they have always -// had. The C# grammar names nothing here — `parenthesized_expression`, +// The paren and prefix arms read their operand with `wrapped_operand`. +// The C# grammar names nothing here — `parenthesized_expression`, // `prefix_unary_expression` and `postfix_unary_expression` all carry an // empty `fields` map in node-types.json — so the field read that -// Kotlin's and Groovy's equivalents use is unavailable, and with it the -// comment bug those reads dodge: `if (( /*c*/ b))` and `if (! /*c*/ b)` -// still score zero, because `child(1)` is the comment. Measured, not -// assumed. That is #1455, which predates this change and is recorded -// here rather than widened into it; the new arm adds no instance of it. +// Kotlin's and Groovy's equivalents use is unavailable, and the +// positional `child(1)` it replaced lost the operand to a comment: +// `if (( /*c*/ b))` and `if (! /*c*/ b)` scored zero (#1455). // // Every `?` below is infallible for well-formed C# — each wrapper is -// the operator token plus its operand, so `child(0)` and `child(1)` +// the operator token plus its operand, so `child(0)` and an operand // both exist — and is spelled as an `Option` because `AGENTS.md` bans // `expect` outside tests. Do not try to cover the `None` arms: only // error recovery reaches them (`bool b = !;` parses to a one-child @@ -80,13 +78,13 @@ fn csharp_wrapper_operand<'a>(node: &Node<'a>) -> Option<(Node<'a>, bool)> { match node.kind_id().into() { // `(expr)` — the inner expression follows the `(` token. - csharp_paren_expr_kinds!() => Some((node.child(1)?, false)), + csharp_paren_expr_kinds!() => Some((wrapped_operand(node)?, false)), // `!expr` — the operand follows the operator token. Seven other // prefix operators (`++ -- + - ~ & ^`) share this kind, as does // the `*` of a pointer indirection the grammar aliases onto it; // none is a boolean slot's operand. csharp_prefix_unary_expr_kinds!() => match node.child(0)?.kind_id().into() { - BANG => Some((node.child(1)?, true)), + BANG => Some((wrapped_operand(node)?, true)), _ => None, }, // `expr!` — the null-forgiving operator. One kind id at the @@ -634,25 +632,14 @@ fn csharp_walk_for_conditions<'a>( // 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 - // anonymous string children, NOT a wrapping - // `parenthesized_expression` as in tree-sitter-java — so the - // condition lives at child(2). Targeting child(1) (the literal - // `(` token) was the #370 bug: every unary / bare-identifier - // condition silently scored 0. See issue #370. - IfStatement | WhileStatement => { - if let Some(condition) = node.child(2) { - csharp_count_condition(&condition, node, conds); - } - } - // tree-sitter-c-sharp `do_statement` shape: - // [`do`, body, `while`, `(`, condition, `)`, `;`]. The - // condition lives at child(4), not child(3) (which is the - // literal `(` token). Targeting child(3) was the second half - // of the #370 bug. - DoStatement => { - if let Some(condition) = node.child(4) { + // tree-sitter-c-sharp spells the parens of `if` / `while` / + // `do … while` as anonymous tokens, NOT a wrapping + // `parenthesized_expression` as in tree-sitter-java, so the + // slot holds the bare condition. Read by grammar field: a fixed + // index landed on the `(` token (#370), and after that fix on a + // comment, since `if (/*c*/ b)` puts one at child(2) (#1455). + IfStatement | WhileStatement | DoStatement => { + if let Some(condition) = node.child_by_field_name("condition") { csharp_count_condition(&condition, node, conds); } } @@ -692,25 +679,23 @@ fn csharp_walk_for_conditions<'a>( // `!`-prefix wrapper — so passing them through the slot adds // nothing and the loop cannot double count a clause that holds // one expression by construction. - // - // 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); } } - // `return value;` — child(1) is the value expression. - ReturnStatement => csharp_inspect_child(node, 1, conds), - // Child 2: declarator / assignment RHS, lambda body - // (`params => body`). - crate::Csharp::VariableDeclarator - | crate::Csharp::VariableDeclarator2 - | AssignmentExpression - | LambdaExpression => csharp_inspect_child(node, 2, conds), + // The value slots, none of which a fixed index can address: a + // comment may precede the value (`return /*c*/ !b;`), and a + // lambda's optional `async` / attribute preamble shifts its body. + // `return` and a declarator name no field for the value, so the + // first and last operand stand in: a `return` holds only its + // value, and a declarator's initialiser follows its name. + ReturnStatement => csharp_inspect_slot(wrapped_operand(node), node, conds), + crate::Csharp::VariableDeclarator | crate::Csharp::VariableDeclarator2 => { + csharp_inspect_slot(node.children().filter(is_operand).last(), node, conds); + } + AssignmentExpression => csharp_inspect_slot(node.child_by_field_name("right"), node, conds), + LambdaExpression => csharp_inspect_slot(node.child_by_field_name("body"), node, conds), ConditionalExpression => csharp_walk_conditional(node, stats), ForStatement => csharp_walk_for_statement(node, stats), _ => {} @@ -776,12 +761,12 @@ impl Abc for CsharpCode { } } -// C# mirror of `java_inspect_child` / `groovy_inspect_child`: passes -// `node.child(idx)` to `csharp_inspect_container`, which is a no-op on -// every kind `csharp_wrapper_operand` declines. -fn csharp_inspect_child(node: &Node, idx: usize, conditions: &mut f64) { - if let Some(child) = node.child(idx) { - csharp_inspect_container(&child, node, conditions); +// C# mirror of `java_inspect_slot` / `groovy_inspect_slot`: passes a +// value slot's occupant to `csharp_inspect_container`, which is a no-op +// on every kind `csharp_wrapper_operand` declines. +fn csharp_inspect_slot(slot: Option, parent: &Node, conditions: &mut f64) { + if let Some(child) = slot { + csharp_inspect_container(&child, parent, conditions); } } diff --git a/src/metrics/abc/elixir.rs b/src/metrics/abc/elixir.rs index 7650d5069..208cc277c 100644 --- a/src/metrics/abc/elixir.rs +++ b/src/metrics/abc/elixir.rs @@ -9,7 +9,7 @@ clippy::cast_sign_loss )] -use super::{Abc, Stats}; +use super::{Abc, Stats, wrapped_operand}; use crate::lang_helpers::elixir::elixir_call_keyword; use crate::macros::elixir_bool_terminal_kinds; use crate::*; @@ -64,13 +64,15 @@ fn elixir_inspect_container(container_node: &Node, parent: &Node, conditions: &m has_boolean_content = true; } - // A `!` unary stores its operand at child index 1 (after the `!` - // token); a parenthesised `block` carries its inner expression as - // the first named child. + // A negation names its `operand`; a parenthesised `block` + // carries its inner expression as its only operand. Neither is + // read positionally or as the first named child, because a + // comment is named and may sit before the operand + // (`a && ! # c⏎ b`, #1455). let next = if is_not { - node.child(1) + node.child_by_field_name("operand") } else { - node.children().find(Node::is_named) + wrapped_operand(&node) }; let Some(child) = next else { break }; node = child; @@ -235,15 +237,15 @@ impl Abc for ElixirCode { match node.kind_id().into() { // A `BinaryOperator` whose operator token is `EQ` is a - // pattern-match assignment. The grammar shape is - // `(left, operator, right)`, so the operator token is - // always at child index 1 — looking it up directly is - // O(1) vs. an `any()` scan of all children. This arm - // fires on every Elixir binary op (comparisons, pipes, - // boolean ops, arithmetic) so the constant-time check - // matters. + // pattern-match assignment. Read through the `operator` + // field rather than at child index 1, where a comment + // before the operator (`a # c⏎ = 1`) sits instead (#1455); + // a field read also avoids an `any()` scan of all children + // on an arm that fires on every Elixir binary op. E::BinaryOperator | E::BinaryOperator2 | E::BinaryOperator3 - if node.child(1).is_some_and(|c| c.kind_id() == E::EQ as u16) => + if node + .child_by_field_name("operator") + .is_some_and(|c| c.kind_id() == E::EQ as u16) => { stats.assignments += 1.; } diff --git a/src/metrics/abc/go.rs b/src/metrics/abc/go.rs index af78ccaee..56d9299f5 100644 --- a/src/metrics/abc/go.rs +++ b/src/metrics/abc/go.rs @@ -9,7 +9,7 @@ clippy::cast_sign_loss )] -use super::{Abc, Stats, for_each_named_child}; +use super::{Abc, Stats, for_each_named_child, wrapped_operand}; use crate::macros::go_bool_terminal_kinds; use crate::*; @@ -19,17 +19,19 @@ use crate::*; // `go_count_condition` asks for, so the slot and the peel cannot // disagree about which kinds are wrappers (#1470; the Kotlin, Groovy // and C# instances of that disagreement were #1459, #1466 and #1463). -// Both wrappers store their operand at child(1), after the `(` or the -// operator token. A `unary_expression` spelled with any other operator +// Each operand is read by role rather than at child(1) after the `(` or +// the operator token, where a comment may sit (`if ! /*c*/ b`, #1455): +// `unary_expression` names its `operand`, and a parenthesis holds only +// its operand. A `unary_expression` spelled with any other operator // (`-x`, `^x`, `*p`, `&v`, `<-ch`) is never a boolean slot's operand, // so the peel declines it. fn go_wrapper_operand<'a>(node: &Node<'a>) -> Option<(Node<'a>, bool)> { use Go as G; match node.kind_id().into() { - G::ParenthesizedExpression => Some((node.child(1)?, false)), + G::ParenthesizedExpression => Some((wrapped_operand(node)?, false)), G::UnaryExpression if node.child(0)?.kind_id() == G::BANG as u16 => { - Some((node.child(1)?, true)) + Some((node.child_by_field_name("operand")?, true)) } _ => None, } @@ -239,16 +241,18 @@ impl Abc for GoCode { go_walk_for_statement(node, &mut stats.conditions); } // `return value` — Go wraps the return values in an - // `expression_list` at child(1). Iterate the list's + // `expression_list`, the statement's only operand (not + // child(1), where `return /*c*/ !x` puts a comment — + // #1455). Iterate the list's // children and route each through `inspect_container` // (NOT the terminal-at-top form): `return !x` counts // the wrapped Identifier once, while `return x` (bare // identifier in the return slot) reports zero // conditions. Matches Java's policy in // `java_return_without_conditions`. Bare `return` - // (no values) has no child(1). + // (no values) has no operand. G::ReturnStatement => { - if let Some(expr_list) = node.child(1) { + if let Some(expr_list) = wrapped_operand(node) { for_each_named_child(&expr_list, &mut stats.conditions, go_inspect_container); } } diff --git a/src/metrics/abc/groovy.rs b/src/metrics/abc/groovy.rs index 8df727377..f7ce71052 100644 --- a/src/metrics/abc/groovy.rs +++ b/src/metrics/abc/groovy.rs @@ -9,7 +9,7 @@ clippy::cast_sign_loss )] -use super::{Abc, Stats}; +use super::{Abc, Stats, wrapped_operand}; use crate::macros::groovy_bool_terminal_kinds; use crate::*; @@ -29,23 +29,22 @@ use crate::*; // (`if (! /*c*/ a)` scores, where the positional `child(1)` read scored // the comment). `parenthesized_expression` names nothing — its only // child in node-types.json is the unlabelled inner `_expression` — so it -// keeps the positional read, and with it the same comment bug Kotlin -// records: `if ( /*c*/ a)` still scores zero. Measured, not assumed. +// takes the first operand that is not an extra; the positional read it +// replaced scored `if ( /*c*/ a)` zero (#1455). // // Kotlin, C# and Groovy now share this peel's *shape* and nothing else, // deliberately. A common helper would have to be parameterised by the // wrapper kind set, a per-wrapper operand accessor, a per-wrapper // proves-boolean flag and the parent seed — every line of the body — // to save a five-line `while let`, so the reuse worth having is the -// `Option<(Node, bool)>` signature, not a generic function. C#'s -// positional read over its aliased wrapper kinds is #1455's, not this -// change's. +// `Option<(Node, bool)>` signature and the shared `wrapped_operand` +// read, not a generic function. // // Every `?` below is infallible at the pinned grammar and is spelled // that way because `AGENTS.md` bans `expect` outside tests — do not try // to cover the `None` arms. `unary_expression` declares `operand` and // `operator` as required fields, and a `parenthesized_expression` is -// `(` expr `)`, so `child(1)` exists. Only error recovery on invalid +// `(` expr `)`, so an operand exists. Only error recovery on invalid // Groovy can produce a shorter node, and pinning that would make the // grammar's present over-permissiveness the contract (§6). fn groovy_wrapper_operand<'a>(node: &Node<'a>) -> Option<(Node<'a>, bool)> { @@ -53,7 +52,7 @@ fn groovy_wrapper_operand<'a>(node: &Node<'a>) -> Option<(Node<'a>, bool)> { match node.kind_id().into() { // `(expr)` — the inner expression follows the `(` token. - ParenthesizedExpression => Some((node.child(1)?, false)), + ParenthesizedExpression => Some((wrapped_operand(node)?, false)), UnaryExpression => { let operand = node.child_by_field_name("operand")?; match node.child_by_field_name("operator")?.kind_id().into() { @@ -135,15 +134,15 @@ fn groovy_count_unary_conditions(list_node: &Node, conditions: &mut f64) { // Java's `DoStatement`; no `LambdaExpression` (Groovy closures take // block bodies, no implicit-return arm); and `if (…)` / `while (…)` / // `do { … } while (…)` parens inlined as token children rather than -// wrapped in `parenthesized_expression`, so the condition sits at a -// different child index and goes through `groovy_count_condition`. +// wrapped in `parenthesized_expression`, so the condition slot holds +// the bare expression and goes through `groovy_count_condition`. -// Groovy mirror of `java_inspect_child`: passes `node.child(idx)` to +// Groovy mirror of `java_inspect_slot`: passes a slot's occupant to // `groovy_inspect_container`, which is a no-op on kinds other than // `ParenthesizedExpression` / `!`-prefixed `UnaryExpression`. -fn groovy_inspect_child(node: &Node, idx: usize, conditions: &mut f64) { - if let Some(child) = node.child(idx) { - groovy_inspect_container(&child, node, conditions); +fn groovy_inspect_slot(slot: Option, parent: &Node, conditions: &mut f64) { + if let Some(child) = slot { + groovy_inspect_container(&child, parent, conditions); } } @@ -373,24 +372,22 @@ fn groovy_walk_for_conditions<'a>( } } ArgumentList => groovy_count_unary_conditions(node, conds), - VariableDeclarator | AssignmentExpression => groovy_inspect_child(node, 2, conds), - // dekobon `if_statement` / `while_statement` shape: - // [keyword, `(`, condition, `)`, body, …]. Condition lives at - // child index 2 (not 1 as under tree-sitter-java, where parens - // wrap the condition in a `parenthesized_expression`). - IfStatement | WhileStatement => { - if let Some(condition) = node.child(2) { - groovy_count_condition(&condition, node, conds); - } + VariableDeclarator => groovy_inspect_slot(node.child_by_field_name("value"), node, conds), + AssignmentExpression => { + groovy_inspect_slot(node.child_by_field_name("right"), node, conds); } - // dekobon shape: [`do`, body, `while`, `(`, condition, `)`]. - // Condition is at child index 4. - DoWhileStatement => { - if let Some(condition) = node.child(4) { + // The dekobon grammar inlines the parens of `if` / `while` / + // `do … while` as anonymous tokens (tree-sitter-java wraps them + // in a `parenthesized_expression`), so the slot holds the bare + // condition. Read by grammar field: the fixed index it replaced + // landed on a comment in `if (/*c*/ b)` (#1455). + IfStatement | WhileStatement | DoWhileStatement => { + if let Some(condition) = node.child_by_field_name("condition") { groovy_count_condition(&condition, node, conds); } } - ReturnStatement => groovy_inspect_child(node, 1, conds), + // `return value` names no field; the value is its only operand. + ReturnStatement => groovy_inspect_slot(wrapped_operand(node), node, conds), TernaryExpression => groovy_walk_ternary(node, stats), ForStatement => groovy_walk_for_statement(node, stats), _ => {} diff --git a/src/metrics/abc/java.rs b/src/metrics/abc/java.rs index f9fa1d494..b33d0a32c 100644 --- a/src/metrics/abc/java.rs +++ b/src/metrics/abc/java.rs @@ -9,7 +9,7 @@ clippy::cast_sign_loss )] -use super::{Abc, Stats}; +use super::{Abc, Stats, wrapped_operand}; use crate::macros::java_bool_terminal_kinds; use crate::*; @@ -23,16 +23,19 @@ use crate::*; // A `unary_expression` spelled `-x`, `+x` or `~x` is never a boolean // slot's operand, so the peel declines it. // -// Both wrappers store their expression at child index one, after the -// `(` or the operator token: -// https://github.com/tree-sitter/tree-sitter-java/blob/master/src/grammar.json#L2472 -// https://github.com/tree-sitter/tree-sitter-java/blob/master/src/grammar.json#L2150 +// The operand is read by role, not at child index one after the `(` or +// the operator token: a comment may sit there (`(/*c*/ b)`, `! /*c*/ b`) +// and scored the condition zero (#1455). `unary_expression` names its +// `operand`; `parenthesized_expression` names nothing, so it takes the +// first operand that is not an extra. fn java_wrapper_operand<'a>(node: &Node<'a>) -> Option<(Node<'a>, bool)> { use Java::*; match node.kind_id().into() { - ParenthesizedExpression => Some((node.child(1)?, false)), - UnaryExpression if node.child(0)?.kind_id() == BANG as u16 => Some((node.child(1)?, true)), + ParenthesizedExpression => Some((wrapped_operand(node)?, false)), + UnaryExpression if node.child(0)?.kind_id() == BANG as u16 => { + Some((node.child_by_field_name("operand")?, true)) + } _ => None, } } @@ -128,13 +131,13 @@ fn java_count_unary_conditions(list_node: &Node, conditions: &mut f64) { // mutually exclusive in the source language so a short-circuit chain // reproduces the original `match` semantics bit-for-bit. -// Shared helper: passes `node.child(idx)` to `java_inspect_container`. +// Shared helper: passes a slot's occupant to `java_inspect_container`. // The container helper is a no-op on kinds other than // `ParenthesizedExpression` / `!`-prefixed `UnaryExpression`, so no // `matches!` guard is needed at the call site. -fn java_inspect_child(node: &Node, idx: usize, conditions: &mut f64) { - if let Some(child) = node.child(idx) { - java_inspect_container(&child, node, conditions); +fn java_inspect_slot(slot: Option, parent: &Node, conditions: &mut f64) { + if let Some(child) = slot { + java_inspect_container(&child, parent, conditions); } } @@ -328,8 +331,14 @@ fn java_walk_for_conditions<'a>(node: &Node<'a>, ancestors: Ancestors<'a, '_>, s } // Unary conditions among method arguments. ArgumentList => java_count_unary_conditions(node, conds), - // Child 1: `if (cond) ...`, `while (cond) ...`, `return value;`. - IfStatement | WhileStatement | ReturnStatement => java_inspect_child(node, 1, conds), + // `if (cond)`, `while (cond)`, `do … while (cond);`, by grammar + // field: a fixed index lands on a comment before the slot + // (`if /*c*/ (b)`) and scores the condition zero (#1455). + IfStatement | WhileStatement | DoStatement => { + java_inspect_slot(node.child_by_field_name("condition"), node, conds); + } + // `return value;` names no field; the value is its only operand. + ReturnStatement => java_inspect_slot(wrapped_operand(node), node, conds), // The Java 21 pattern-switch guard (`case Integer i when g ->`), // modelled as a condition slot exactly like the `if` / `while` // slots above (#1454, transferring #1422's C# rule). Before @@ -363,13 +372,11 @@ fn java_walk_for_conditions<'a>(node: &Node<'a>, ancestors: Ancestors<'a, '_>, s java_count_condition(&guard, node, conds); } } - // Child 2: assignment / declarator RHS, lambda body - // (`params -> body`). - VariableDeclarator | AssignmentExpression | LambdaExpression => { - java_inspect_child(node, 2, conds); - } - // Child 3: the `while (cond)` condition of `do { ... } while (...);`. - DoStatement => java_inspect_child(node, 3, conds), + // Declarator / assignment RHS and lambda body (`params -> body`), + // by field for the same reason as the condition slots above. + VariableDeclarator => java_inspect_slot(node.child_by_field_name("value"), node, conds), + AssignmentExpression => java_inspect_slot(node.child_by_field_name("right"), node, conds), + LambdaExpression => java_inspect_slot(node.child_by_field_name("body"), node, conds), TernaryExpression => java_walk_ternary(node, stats), ForStatement => java_walk_for_statement(node, stats), _ => {} diff --git a/src/metrics/abc/js_family.rs b/src/metrics/abc/js_family.rs index 0180b9957..32485edff 100644 --- a/src/metrics/abc/js_family.rs +++ b/src/metrics/abc/js_family.rs @@ -9,7 +9,7 @@ clippy::cast_sign_loss )] -use super::{Abc, Stats}; +use super::{Abc, Stats, wrapped_operand}; use crate::macros::{ javascript_bool_terminal_kinds, mozjs_bool_terminal_kinds, tsx_bool_terminal_kinds, typescript_bool_terminal_kinds, @@ -64,7 +64,11 @@ macro_rules! impl_js_family_unary_walker { has_boolean_content = true; } - let Some(child) = node.child(1) else { break }; + // The wrapper's only operand, not child(1): a comment may + // sit there (`(/*c*/ b)`, `! /*c*/ b` — #1455). + let Some(child) = wrapped_operand(&node) else { + break; + }; node = child; node_kind = node.kind_id().into(); @@ -429,26 +433,18 @@ macro_rules! ts_abc_compute { // (...)` in `parenthesized_expression`, so // `_inspect_container`'s paren-unwrap handles // the boolean-literal case (`if (true)` counts 1). - // The condition sits at child(1) for if and while. - // For `do_statement`, the condition is at child(3) - // (children: `do`(0), body(1), `while`(2), - // parenthesized condition(3), `;`(4)). - IfStatement | WhileStatement => { - if let Some(cond) = node.child(1) { - $inspect_container(&cond, node, &mut stats.conditions); - } - } - DoStatement => { - // children: `do`(0), body(1), `while`(2), - // parenthesized condition(3), `;`(4). - if let Some(cond) = node.child(3) { + // Read by grammar field, not index: a comment before + // the slot (`if /*c*/ (b)`) shifted every positional + // read onto it (#1455). + IfStatement | WhileStatement | DoStatement => { + if let Some(cond) = node.child_by_field_name("condition") { $inspect_container(&cond, node, &mut stats.conditions); } } - // `return value;` — value at child(1). The bare - // `return;` (no value) form has no child(1). + // `return value;` names no field; the value is its only + // operand. The bare `return;` form has none. ReturnStatement => { - if let Some(value) = node.child(1) { + if let Some(value) = wrapped_operand(node) { $inspect_container(&value, node, &mut stats.conditions); } } @@ -573,21 +569,14 @@ macro_rules! js_abc_compute { } // Phase-2B (issue #403): condition slots. Same shape // as the TypeScript impl above — see that macro's - // arm-block for the per-child-index rationale. - IfStatement | WhileStatement => { - if let Some(cond) = node.child(1) { - $inspect_container(&cond, node, &mut stats.conditions); - } - } - DoStatement => { - // children: `do`(0), body(1), `while`(2), - // parenthesized condition(3), `;`(4). - if let Some(cond) = node.child(3) { + // arm-block for why each slot is read by role. + IfStatement | WhileStatement | DoStatement => { + if let Some(cond) = node.child_by_field_name("condition") { $inspect_container(&cond, node, &mut stats.conditions); } } ReturnStatement => { - if let Some(value) = node.child(1) { + if let Some(value) = wrapped_operand(node) { $inspect_container(&value, node, &mut stats.conditions); } } diff --git a/src/metrics/abc/kotlin.rs b/src/metrics/abc/kotlin.rs index d7c8439db..056f53262 100644 --- a/src/metrics/abc/kotlin.rs +++ b/src/metrics/abc/kotlin.rs @@ -9,7 +9,7 @@ clippy::cast_sign_loss )] -use super::{Abc, Stats}; +use super::{Abc, Stats, wrapped_operand}; use crate::macros::kotlin_bool_terminal_kinds; use crate::*; @@ -85,19 +85,16 @@ fn kotlin_eq_initializes_immutable_binding<'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. +// node-types.json is the unlabelled inner `expression` — so it takes the +// first operand that is not an extra. The positional `child(1)` it +// replaced scored `when { ( /*c*/ a) -> … }` zero, because that child is +// the comment (#1455). 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)), + ParenthesizedExpression => Some((wrapped_operand(node)?, false)), UnaryExpression => { let operand = node.child_by_field_name("argument")?; match node.child_by_field_name("operator")?.kind_id().into() { diff --git a/src/metrics/abc/lua.rs b/src/metrics/abc/lua.rs index 63b0f0fe7..e9340f8b0 100644 --- a/src/metrics/abc/lua.rs +++ b/src/metrics/abc/lua.rs @@ -9,7 +9,7 @@ clippy::cast_sign_loss )] -use super::{Abc, Stats, for_each_named_child}; +use super::{Abc, Stats, for_each_named_child, wrapped_operand}; use crate::macros::lua_bool_terminal_kinds; use crate::*; @@ -36,8 +36,10 @@ use crate::*; // `lua_count_condition` asks for, so the slot and the peel cannot // disagree about which kinds are wrappers (#1470; the Kotlin, Groovy // and C# instances of that disagreement were #1459, #1466 and #1463). -// Both wrappers store their operand at child(1), after the `(` or the -// `not` keyword. A `unary_expression` spelled `-x`, `#t` or `~x` is +// Each operand is read by role rather than at child(1) after the `(` or +// the `not` keyword, where a comment may sit (`not --[[c]] b`, #1455): +// `unary_expression` names its `operand`, and a parenthesis holds only +// its operand. A `unary_expression` spelled `-x`, `#t` or `~x` is // arithmetic, length or bitwise — never a boolean slot's operand — so // the peel declines it. // @@ -46,9 +48,9 @@ use crate::*; // other call-argument contexts would never set the walker's flag. fn lua_wrapper_operand<'a>(node: &Node<'a>) -> Option<(Node<'a>, bool)> { match node.kind_id().into() { - Lua::ParenthesizedExpression => Some((node.child(1)?, false)), + Lua::ParenthesizedExpression => Some((wrapped_operand(node)?, false)), Lua::UnaryExpression if node.child(0)?.kind_id() == Lua::Not as u16 => { - Some((node.child(1)?, true)) + Some((node.child_by_field_name("operand")?, true)) } _ => None, } @@ -180,14 +182,16 @@ impl Abc for LuaCode { lua_count_condition(&cond, node, &mut stats.conditions); } } - // `return value` — Lua wraps return values in - // `expression_list` at child(1). Route each named child + // `return value` — Lua wraps return values in an + // `expression_list`, the statement's only operand (not + // child(1), where `return --[[c]] not x` puts a comment — + // #1455). Route each named child // through `inspect_container` (no top-level terminal // count) so `return not x` counts the unary unwrap once // while `return x` (bare) reports zero. Bare `return` - // (no values) has no child(1). + // (no values) has no operand. Lua::ReturnStatement => { - if let Some(expr_list) = node.child(1) { + if let Some(expr_list) = wrapped_operand(node) { for_each_named_child(&expr_list, &mut stats.conditions, lua_inspect_container); } } diff --git a/src/metrics/abc/mozcpp.rs b/src/metrics/abc/mozcpp.rs index 9069d19d3..2f872fbb7 100644 --- a/src/metrics/abc/mozcpp.rs +++ b/src/metrics/abc/mozcpp.rs @@ -10,10 +10,10 @@ )] use super::cpp::{ - cpp_comparison_is_applied, cpp_count_unary_conditions, cpp_inspect_child, - cpp_inspect_container, cpp_walk_for_statement, cpp_walk_ternary, + cpp_comparison_is_applied, cpp_count_unary_conditions, cpp_inspect_slot, + cpp_walk_for_statement, cpp_walk_ternary, }; -use super::{Abc, Stats}; +use super::{Abc, Stats, wrapped_operand}; use crate::*; impl Abc for MozcppCode { @@ -106,31 +106,25 @@ impl Abc for MozcppCode { cpp_count_unary_conditions(&parent, &mut stats.conditions); } } - // Phase-2B (issue #403): condition slots. C++ wraps every - // `if (...)` / `while (...)` / `do {…} while (...)` / - // `return value` in a paren / parenthesized expression - // (return is unparenthesized but its child(1) is the - // expression). `cpp_inspect_container` handles the - // `(...)` / `!...` unwrap so `if (true)` and `return !x` - // each count one condition; bare `return x` reports zero. - // Use `child_by_field_name("condition")` for if/while so - // the `if constexpr (cond)` form (where child(1) is the - // `constexpr` keyword, not the condition_clause) is - // handled correctly. Return uses positional child(1) - // — its value field is always at index 1, no optional - // attribute precedes it. - IfStatement | WhileStatement => { - if let Some(cond) = node.child_by_field_name("condition") { - cpp_inspect_container(&cond, node, &mut stats.conditions); - } + // Phase-2B (issue #403): condition slots. `if (...)` / + // `while (...)` / `do {…} while (...)` wrap their condition + // in a paren / `condition_clause` node that + // `cpp_inspect_container` unwraps, so `if (true)` and + // `return !x` each count one condition; bare `return x` + // reports zero. Every slot is read by role, never by index: + // `if constexpr (cond)` puts the keyword at child(1), and a + // comment shifts every later child (`do {} while /*c*/ (b);`, + // `return /*c*/ !b;` — #1455). `return` names no field; its + // value is its only operand. + IfStatement | WhileStatement | DoStatement => { + cpp_inspect_slot( + node.child_by_field_name("condition"), + node, + &mut stats.conditions, + ); } ReturnStatement => { - cpp_inspect_child(node, 1, &mut stats.conditions); - } - // `do { ... } while (cond);` — children: `do`, body, - // `while`, condition (parenthesized). Condition at child(3). - DoStatement => { - cpp_inspect_child(node, 3, &mut stats.conditions); + cpp_inspect_slot(wrapped_operand(node), node, &mut stats.conditions); } // `f(!a, !b)` — argument list walker. Two aliases — // `argument_list` is emitted as ArgumentList or diff --git a/src/metrics/abc/objc.rs b/src/metrics/abc/objc.rs index 14874146f..3e12fab06 100644 --- a/src/metrics/abc/objc.rs +++ b/src/metrics/abc/objc.rs @@ -10,15 +10,14 @@ )] use super::cpp::{ - cpp_count_unary_conditions, cpp_inspect_child, cpp_inspect_container, cpp_walk_for_statement, - cpp_walk_ternary, + cpp_count_unary_conditions, cpp_inspect_slot, cpp_walk_for_statement, cpp_walk_ternary, }; -use super::{Abc, Stats}; +use super::{Abc, Stats, wrapped_operand}; use crate::*; // Objective-C ABC. ObjC is C plus message sends and `@`-directives, so // the walker is the C/C++ shape (it reuses the grammar-agnostic -// `cpp_inspect_container` / `cpp_inspect_child` / `cpp_count_unary_conditions` +// `cpp_inspect_container` / `cpp_inspect_slot` / `cpp_count_unary_conditions` // helpers, which match on node-kind strings) with two ObjC additions: // * the `B` (branch) dimension counts `message_expression` // (`[obj msg:x]`) alongside C `call_expression`s — a message send is @@ -95,16 +94,16 @@ impl Abc for ObjcCode { cpp_count_unary_conditions(&parent, &mut stats.conditions); } } - IfStatement | WhileStatement => { - if let Some(cond) = node.child_by_field_name("condition") { - cpp_inspect_container(&cond, node, &mut stats.conditions); - } + // By role, never by index — see `impl Abc for CppCode` (#1455). + IfStatement | WhileStatement | DoStatement => { + cpp_inspect_slot( + node.child_by_field_name("condition"), + node, + &mut stats.conditions, + ); } ReturnStatement => { - cpp_inspect_child(node, 1, &mut stats.conditions); - } - DoStatement => { - cpp_inspect_child(node, 3, &mut stats.conditions); + cpp_inspect_slot(wrapped_operand(node), node, &mut stats.conditions); } ArgumentList | ArgumentList2 => { cpp_count_unary_conditions(node, &mut stats.conditions); diff --git a/src/metrics/abc/perl.rs b/src/metrics/abc/perl.rs index 275837060..7f2154769 100644 --- a/src/metrics/abc/perl.rs +++ b/src/metrics/abc/perl.rs @@ -9,7 +9,7 @@ clippy::cast_sign_loss )] -use super::{Abc, Stats}; +use super::{Abc, Stats, is_operand, wrapped_operand}; use crate::macros::perl_bool_terminal_kinds; use crate::*; @@ -102,8 +102,8 @@ fn perl_inspect_container(container_node: &Node, parent: &Node, conditions: &mut // child gives the semantically correct operand for both // shapes: `($a)` → `$a`, `($x, $y)` → `$y`, `if ($a)` → // `$a`. `ParenthesizedArgument` (the other paren-wrap kind) - // has only one inner expression, so child(1) and last-named - // are equivalent. + // has only one inner expression, so the first and last + // operand are the same node. let is_parens = matches!(node_kind, P::ParenthesizedArgument | P::Array); // Both spellings of the same negation — see `ruby_inspect_container` // for the rationale; Perl has the identical gap (#1182). Read @@ -123,13 +123,14 @@ fn perl_inspect_container(container_node: &Node, parent: &Node, conditions: &mut has_boolean_content = true; } - // Descend through the wrapper to the value. Array uses - // last-named-child (Perl scalar-context value); other - // wrappers store their inner expression at child(1). + // Descend through the wrapper to the value. Array uses the + // last operand (Perl scalar-context value); the other wrappers + // hold one operand. Neither is read by index: a comment may sit + // before or after the operand (`(# c` / `! # c`, #1455). let next = if matches!(node_kind, P::Array) { - perl_last_named_child(&node) + perl_last_operand(&node) } else { - node.child(1) + wrapped_operand(&node) }; let Some(child) = next else { break }; node = child; @@ -144,14 +145,14 @@ fn perl_inspect_container(container_node: &Node, parent: &Node, conditions: &mut } } -// Phase-2B (issue #403): pass `node.child(idx)` through +// Phase-2B (issue #403): pass a slot's occupant through // `perl_inspect_container`. Perl wraps `if (cond)` / `while (cond)` / // `unless (cond)` / `until (cond)` conditions in a // `parenthesized_argument`, so the paren unwrap handles the // boolean-literal case. -fn perl_inspect_child(node: &Node, idx: usize, conditions: &mut f64) { - if let Some(child) = node.child(idx) { - perl_inspect_container(&child, node, conditions); +fn perl_inspect_slot(slot: Option, parent: &Node, conditions: &mut f64) { + if let Some(child) = slot { + perl_inspect_container(&child, parent, conditions); } } @@ -162,28 +163,16 @@ fn perl_inspect_child(node: &Node, idx: usize, conditions: &mut f64) { // `perl_count_unary_conditions`; condition-slot Arrays are // already unwrapped by `perl_inspect_container`. This predicate // disambiguates by checking the parent kind. -// Returns the last named child of a node, or None if there are no -// named children. Used by `perl_inspect_container` to descend through -// the `Array` `(...)` wrapper: for a single-element grouping -// `($a)` the last named child is `$a`; for a multi-element list -// literal `($x, $y)` the last named child is `$y` (the value the -// expression evaluates to in Perl's scalar context, which is the -// only context the walker operates in). -fn perl_last_named_child<'a>(node: &Node<'a>) -> Option> { - let mut cursor = node.cursor(); - let mut last_named = None; - if cursor.goto_first_child() { - loop { - let child = cursor.node(); - if child.is_named() { - last_named = Some(child); - } - if !cursor.goto_next_sibling() { - break; - } - } - } - last_named +// Returns the last operand of a node — its last named child that is +// not an extra — or None if there is none. Used by +// `perl_inspect_container` to descend through the `Array` `(...)` +// wrapper: for a single-element grouping `($a)` the last operand is +// `$a`; for a multi-element list literal `($x, $y)` it is `$y` (the +// value the expression evaluates to in Perl's scalar context, which is +// the only context the walker operates in). A trailing comment +// (`($a # c⏎)`) is skipped rather than returned (#1455). +fn perl_last_operand<'a>(node: &Node<'a>) -> Option> { + node.children().filter(is_operand).last() } // Phase-2B (issues #403 / #1102): a ternary's condition and its two @@ -283,7 +272,7 @@ fn perl_walk_for_statement(node: &Node, conditions: &mut f64) { // The `condition` field holds an `_argument_choice`: either a // `parenthesized_argument`, which `perl_inspect_container` already // peels, or a bare `arguments` wrapper, which it does not. Peel the -// `arguments` layer here — via the same last-named-child rule the +// `arguments` layer here — via the same last-operand rule the // `Array` `(...)` wrapper uses, since a Perl comma list evaluates to // its last element in the scalar context a condition imposes — and // hand what it holds to the shared condition classifier. @@ -301,15 +290,15 @@ fn perl_walk_for_statement(node: &Node, conditions: &mut f64) { // wrapping an `array`). An empty wrapper peels to nothing, so that // arm and a bare `None` score alike — measured by perturbation, not // assumed. A test would pin a value neither branch decides. -// - `perl_last_named_child` returns `None` only for an `arguments` -// node with no named child, which the grammar's comma-separated +// - `perl_last_operand` returns `None` only for an `arguments` +// node with no operand, which the grammar's comma-separated // one-or-more list cannot produce. fn perl_walk_statement_modifier(node: &Node, conditions: &mut f64) { let Some(condition) = node.child_by_field_name("condition") else { return; }; let slot = if matches!(condition.kind_id().into(), Perl::Arguments) { - perl_last_named_child(&condition) + perl_last_operand(&condition) } else { Some(condition) }; @@ -343,9 +332,12 @@ fn perl_pattern_is_bound_or_delimiter(pattern: &Node, code: &[u8], ancestors: An .child_by_field_name("operator") .is_some_and(|op| matches!(op.kind_id().into(), P::EQTILDE | P::BANGTILDE)), P::Arguments | P::Array => { + // The first operand, not the first named child: a + // comment is named, so `split( # sep⏎ /,/, $s)` read it + // as the first argument (#1455). parent .children() - .find(Node::is_named) + .find(is_operand) .is_some_and(|first| first.id() == pattern.id()) && climb .next() @@ -557,16 +549,20 @@ impl Abc for PerlCode { // `until (cond)` in the `Array` `(...)` shape (the // grammar's name for parenthesized expressions in // statement-modifier slots) — the paren unwrap handles - // boolean-literal cases. Condition sits at child(1) - // (child(0) is the `if` / `while` keyword). - // `return value`'s value also sits at child(1); merged - // into the same arm body to satisfy `match_same_arms`. - P::IfStatement - | P::UnlessStatement - | P::WhileStatement - | P::UntilStatement - | P::ReturnExpression => { - perl_inspect_child(node, 1, &mut stats.conditions); + // boolean-literal cases. Read by grammar field, not at + // child(1): a comment may sit before the slot (`if # c⏎ + // ($b)`), which the fixed index scored zero (#1455). + // `return value` names no field; its value is its only + // operand. + P::IfStatement | P::UnlessStatement | P::WhileStatement | P::UntilStatement => { + perl_inspect_slot( + node.child_by_field_name("condition"), + node, + &mut stats.conditions, + ); + } + P::ReturnExpression => { + perl_inspect_slot(wrapped_operand(node), node, &mut stats.conditions); } // `call(!$a, !$b)` — argument list walker. Perl wraps // call-argument lists in an `Array` node (same kind name diff --git a/src/metrics/abc/php.rs b/src/metrics/abc/php.rs index 0c1b1419a..2ade4b7fb 100644 --- a/src/metrics/abc/php.rs +++ b/src/metrics/abc/php.rs @@ -9,7 +9,7 @@ clippy::cast_sign_loss )] -use super::{Abc, Stats}; +use super::{Abc, Stats, wrapped_operand}; use crate::macros::php_bool_terminal_kinds; use crate::*; @@ -49,7 +49,11 @@ fn php_inspect_container(container_node: &Node, parent: &Node, conditions: &mut has_boolean_content = true; } - let Some(child) = node.child(1) else { break }; + // The wrapper's only operand, not child(1): a comment may sit + // there (`(/*c*/ $b)`, `! /*c*/ $b` — #1455). + let Some(child) = wrapped_operand(&node) else { + break; + }; node = child; node_kind = node.kind_id().into(); @@ -62,13 +66,13 @@ fn php_inspect_container(container_node: &Node, parent: &Node, conditions: &mut } } -// Phase-2B helper (issue #403): pass `node.child(idx)` through +// Phase-2B helper (issue #403): pass a slot's occupant through // `php_inspect_container`. PHP wraps `if (...)` / `while (...)` / // `do {…} while (...)` in `parenthesized_expression`, so the paren // unwrap handles the boolean-literal case (`if (true)` counts 1). -fn php_inspect_child(node: &Node, idx: usize, conditions: &mut f64) { - if let Some(child) = node.child(idx) { - php_inspect_container(&child, node, conditions); +fn php_inspect_slot(slot: Option, parent: &Node, conditions: &mut f64) { + if let Some(child) = slot { + php_inspect_container(&child, parent, conditions); } } @@ -280,15 +284,20 @@ impl Abc for PhpCode { } } // Phase-2B (issue #403): condition slots. PHP wraps - // `if (...)` / `while (...)` in `parenthesized_expression` - // at child(1); `return value;` exposes the value at the - // same index. `do {…} while (...)` has the parenthesized - // condition at child(3). - IfStatement | WhileStatement | ReturnStatement => { - php_inspect_child(node, 1, &mut stats.conditions); + // `if (...)` / `while (...)` / `do {…} while (...)` in a + // `parenthesized_expression`, read by grammar field: a + // comment before it (`if /*c*/ ($b)`) shifted the fixed + // index it replaced (#1455). `return value;` names no + // field; its value is its only operand. + IfStatement | WhileStatement | DoStatement => { + php_inspect_slot( + node.child_by_field_name("condition"), + node, + &mut stats.conditions, + ); } - DoStatement => { - php_inspect_child(node, 3, &mut stats.conditions); + ReturnStatement => { + php_inspect_slot(wrapped_operand(node), node, &mut stats.conditions); } // `f(!$a, !$b)` — argument list walker. Arguments => { diff --git a/src/metrics/abc/python.rs b/src/metrics/abc/python.rs index 6a2b85af5..3f85574e3 100644 --- a/src/metrics/abc/python.rs +++ b/src/metrics/abc/python.rs @@ -9,7 +9,7 @@ clippy::cast_sign_loss )] -use super::{Abc, Stats}; +use super::{Abc, Stats, wrapped_operand}; use crate::macros::python_bool_terminal_kinds; use crate::*; @@ -41,8 +41,9 @@ use crate::*; // paper's "unary conditional expression". See the module-level // `Stats` doc-comment for the cross-language `&&` / `||` policy // (issue #395, walker tracked in #403). -// One step of the `(...)` peel: the expression a parenthesis wraps, at -// child(1) after the `(` token. `None` for anything else, which is also +// One step of the `(...)` peel: the expression a parenthesis wraps — +// its only operand, not child(1) after the `(` token, which is where a +// comment sits in `if ( # c` (#1455). `None` for anything else, which is also // the answer `python_count_condition` asks for, so the slot and the peel // cannot disagree about which kinds are wrappers (#1470; the Kotlin, // Groovy and C# instances of that disagreement were #1459, #1466 and @@ -54,7 +55,7 @@ use crate::*; // parenthesis preserves the type of what it wraps. fn python_wrapper_operand<'a>(node: &Node<'a>) -> Option> { if matches!(node.kind_id().into(), Python::ParenthesizedExpression) { - node.child(1) + wrapped_operand(node) } else { None } @@ -200,9 +201,9 @@ fn python_count_case_guard(case_clause: &Node, conditions: &mut f64) { } } -fn python_inspect_child(node: &Node, idx: usize, conditions: &mut f64) { - if let Some(child) = node.child(idx) { - python_count_condition(&child, node, conditions); +fn python_count_slot(slot: Option, parent: &Node, conditions: &mut f64) { + if let Some(condition) = slot { + python_count_condition(&condition, parent, conditions); } } @@ -321,8 +322,17 @@ impl Abc for PythonCode { // ArgumentList do not need walker arms because every // unary-conditional content node (NotOperator, // ComparisonOperator) already has its own top-level arm. + // + // Read by grammar field (grammar-dispatch §3). Unlike the + // other languages' slots (#1455) a fixed child(1) gave the + // same answer: no valid Python puts a comment between `if` + // and its condition, so no test can tell the two apart. IfStatement | WhileStatement => { - python_inspect_child(node, 1, &mut stats.conditions); + python_count_slot( + node.child_by_field_name("condition"), + node, + &mut stats.conditions, + ); } // `a if c() else b` — the conditional expression node itself // is one condition, as the `?` token is in every C-family diff --git a/src/metrics/abc/ruby.rs b/src/metrics/abc/ruby.rs index 64dfc1025..6f521c231 100644 --- a/src/metrics/abc/ruby.rs +++ b/src/metrics/abc/ruby.rs @@ -9,7 +9,7 @@ clippy::cast_sign_loss )] -use super::{Abc, Stats}; +use super::{Abc, Stats, wrapped_operand}; use crate::macros::ruby_bool_terminal_kinds; use crate::*; @@ -55,17 +55,19 @@ use crate::*; // `unary` operators (`-b`, `~b`, `defined?`) are arithmetic or // introspection, never a boolean slot's operand, so the peel declines. // -// A negation stores its operand at child index 1, after the operator -// token. `parenthesized_statements` wraps its body in named children; -// the first named child carries the expression. +// A negation names its `operand`; `parenthesized_statements` wraps its +// body in named children, the first of which that is not an extra +// carries the expression. Both are read by role because a comment may +// sit before the operand — `(# c` or `! # c` — and a positional or +// first-named-child read handed the comment to the slot (#1455). fn ruby_wrapper_operand<'a>(node: &Node<'a>) -> Option<(Node<'a>, bool)> { use Ruby::*; match node.kind_id().into() { - ParenthesizedStatements => Some((node.children().find(Node::is_named)?, false)), + ParenthesizedStatements => Some((wrapped_operand(node)?, false)), Unary | Unary2 | Unary3 | Unary4 | Unary5 => { match node.child_by_field_name("operator")?.kind_id().into() { - BANG | Not => Some((node.child(1)?, true)), + BANG | Not => Some((node.child_by_field_name("operand")?, true)), _ => None, } } diff --git a/src/metrics/abc/rust.rs b/src/metrics/abc/rust.rs index fdfaf6790..878803adb 100644 --- a/src/metrics/abc/rust.rs +++ b/src/metrics/abc/rust.rs @@ -9,7 +9,7 @@ clippy::cast_sign_loss )] -use super::{Abc, Stats}; +use super::{Abc, Stats, wrapped_operand}; use crate::macros::rust_bool_terminal_kinds; use crate::*; @@ -19,15 +19,19 @@ use crate::*; // `rust_count_condition` asks for, so the slot and the peel cannot // disagree about which kinds are wrappers (#1470; the Kotlin, Groovy // and C# instances of that disagreement were #1459, #1466 and #1463). -// Both wrappers store their operand at child(1), after the `(` or the -// operator token. A `unary_expression` spelled `-x` or `*p` is never a +// Each wrapper holds one operand, read by role rather than at child(1) +// after the `(` or the operator token, where a comment may sit +// (`if ! /*c*/ b`, #1455) — neither wrapper names a field. A +// `unary_expression` spelled `-x` or `*p` is never a // boolean slot's operand, so the peel declines it. fn rust_wrapper_operand<'a>(node: &Node<'a>) -> Option<(Node<'a>, bool)> { use Rust::*; match node.kind_id().into() { - ParenthesizedExpression => Some((node.child(1)?, false)), - UnaryExpression if node.child(0)?.kind_id() == BANG as u16 => Some((node.child(1)?, true)), + ParenthesizedExpression => Some((wrapped_operand(node)?, false)), + UnaryExpression if node.child(0)?.kind_id() == BANG as u16 => { + Some((wrapped_operand(node)?, true)) + } _ => None, } } @@ -81,7 +85,7 @@ fn rust_inspect_container(container_node: &Node, parent: &Node, conditions: &mut // condition itself is a terminal-bool kind (`if true {}`, `if a {}`), // it counts as one condition; if wrapped in `(...)` or `!...`, // `rust_inspect_container` unwraps until a terminal is found. Mirrors -// the `java_count_condition` / `java_inspect_child` helper pair used +// the `java_count_condition` / `java_inspect_slot` helper pair used // by `java_walk_ternary` / `java_walk_for_statement`. fn rust_count_condition(condition: &Node, parent: &Node, conditions: &mut f64) { if matches!(condition.kind_id().into(), rust_bool_terminal_kinds!()) { @@ -141,9 +145,9 @@ fn rust_count_match_arm(node: &Node, conditions: &mut f64) { } } -fn rust_inspect_child(node: &Node, idx: usize, conditions: &mut f64) { - if let Some(child) = node.child(idx) { - rust_count_condition(&child, node, conditions); +fn rust_count_slot(slot: Option, parent: &Node, conditions: &mut f64) { + if let Some(condition) = slot { + rust_count_condition(&condition, parent, conditions); } } @@ -277,21 +281,25 @@ impl Abc for RustCode { // Phase-2B (issue #403): Fitzpatrick Rule 6 / 7 condition // slots. `if true {}` / `if !a {}` count their condition // once via `rust_count_condition` (terminal-at-top or - // paren / unary unwrap). The condition sits at child(1) - // for `if_expression` and `while_expression` (child(0) - // is the keyword). Rust has no `do_statement`, no - // ternary, and no for-condition slot. + // paren / unary unwrap). Read by grammar field: a fixed + // child(1) landed on a comment (`if /*c*/ b`) and on the + // label of `'a: while b` (#1455). Rust has no + // `do_statement`, no ternary, and no for-condition slot. IfExpression | WhileExpression => { - rust_inspect_child(node, 1, &mut stats.conditions); + rust_count_slot( + node.child_by_field_name("condition"), + node, + &mut stats.conditions, + ); } - // `return value;` — `value` sits at child(1). Use the + // `return value;` — the value is the only operand. Use the // bare `inspect_container` path (no top-level terminal // count) so that `return x` reports zero conditions // while `return !x` reports one. Matches Java's policy // (`java_return_without_conditions`): a bare identifier // in the return slot is not a unary conditional. ReturnExpression => { - if let Some(value) = node.child(1) { + if let Some(value) = wrapped_operand(node) { rust_inspect_container(&value, node, &mut stats.conditions); } } diff --git a/tests/repositories/big-code-analysis-output b/tests/repositories/big-code-analysis-output index b22e69aad..2e83ae93b 160000 --- a/tests/repositories/big-code-analysis-output +++ b/tests/repositories/big-code-analysis-output @@ -1 +1 @@ -Subproject commit b22e69aad9a9d9bbee0234557ffcd7748350b1d7 +Subproject commit 2e83ae93b57894255497da8416cfd4df9fa7ec9f From 016ed8481ca2128523ff09bf0e693fe65f20302e Mon Sep 17 00:00:00 2001 From: Elijah Zupancic <490392+dekobon@users.noreply.github.com> Date: Thu, 8 Oct 2026 00:11:13 -0700 Subject: [PATCH 12/17] docs(changelog): consolidate ABC batch entries Record the metric changes from #1448, #1453, #1455, #1467 and #1470 under [Unreleased] Fixed. --- CHANGELOG.md | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d477d500b..86dd1efea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,53 @@ for historical reference. ## [Unreleased] +Every entry under **Fixed** changes ABC `conditions` for valid input. +Only `abc.*` values move; cyclomatic, cognitive and Halstead are +unchanged. + +### Fixed + +- **A C++ or Mozcpp comparison-operator overload no longer scores a + condition** (#1448). `operator<=`, `operator>=`, `operator==`, + `operator!=` and `operator<=>` each declared one, while `operator<` + and `operator>` were already excluded; all seven comparison tokens + now count only where they are applied. A fold over a comparison + (`(a < ...)`, `(0 == ... == a)`) now counts once, through the fold's + `operator` field, where `<` / `>` folds scored 0 and a binary fold + scored 2. `#if A <= B` still counts. In the openfst corpus every + `operator==` / `operator!=` definition and declaration drops by one. +- **C++ and Mozcpp score the ISO alternative tokens like their + symbols** (#1470). `not b`, `b and c`, `b or c` and `b not_eq c` scored + 0 where `!b`, `b && c`, `b || c` and `b != c` score 1, 2, 2 and 1. + Cyclomatic, cognitive and Halstead do not yet recognise them (#1522). +- **A Ruby subject-less `case` no longer counts a comparison twice** + (#1453). `case; when x > 5 … else …` scored 3 against its `if` + analogue's 2. A subject-less `when` is evaluated like an `if` + predicate, so each of its patterns is one operand of an implicit `||` + and scores once unless its own operator already counted it: + `when a, b` and `when a, x > 1` now score 2, level with `if a || b`. + A subject-ful `case x; when 1` still scores one per clause. +- **A Perl bare pattern match scores by use** (#1467). `/^#/` and + `m{^#}` match the implicit `$_`, and scored a condition only inside a + boolean slot, so `my $r = /^#/;` read 0 against `$x =~ /^#/`'s 1. They + now count wherever they are written. The pattern bound by `=~` / `!~` + is not counted again, and `split`'s delimiter (`split /,/, $s`) is not + a match and scores 0. In-slot spellings such as `if (/x/)` are + unchanged. +- **A comment inside or before a condition no longer zeroes it** + (#1455). Condition and value slots were read by child index, and the + `(…)` / `!` peels stepped to child 1 or to the first named child, so a + comment (`if (/*c*/ b)`, `if /*c*/ (b)`, `return /*c*/ !b`, + `((/*c*/ b))`, `! /*c*/ b`) took the operand's place and the condition + scored 0. Every slot is now read by grammar field or as the first + operand that is not a comment, in C, C++, C#, Elixir, Go, Groovy, + Java, JavaScript, Kotlin, Lua, Mozcpp, Mozjs, Objective-C, Perl, PHP, + Python, Ruby, Rust, TSX and TypeScript. The same change fixes a C++ + `if` with an init-statement (`if (int y = f(); b)`), a labelled Rust + `while`, and an Elixir `=` behind a comment, which scored no + assignment. In the corpora, two pdf.js commented conditions and one + serde `while try!(…)` loop gain one condition each. + ## [2.3.0] - 2026-10-07 This release adds a ninth published crate, `big-code-analysis-ast`, From 278880fa1902cd2a7758d4f186dd0ef57b169426 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic <490392+dekobon@users.noreply.github.com> Date: Thu, 8 Oct 2026 00:29:27 -0700 Subject: [PATCH 13/17] fix(abc): address whole-branch review findings Ruby's paren peel read the first statement of `(x; y)`, but the sequence evaluates to its last; it now reads the last operand, as Perl's list peel does, so `if (x; x > 1)` scores 1 and `if (x > 1; x)` 2. Docs: the book gains a Ruby subject-less `when` row and notes the C++ overload, fold and alternative-token rules; a test-module doc no longer lists Perl's bare matches in the terminal set; the CHANGELOG names the kenlm snapshots and the Elixir assignment. --- CHANGELOG.md | 16 ++++++---- big-code-analysis-book/src/metrics.md | 3 +- src/metrics/abc.rs | 45 ++++++++++++++++++--------- src/metrics/abc/ruby.rs | 11 ++++--- 4 files changed, 49 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 86dd1efea..67f72a182 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,9 +24,9 @@ for historical reference. ## [Unreleased] -Every entry under **Fixed** changes ABC `conditions` for valid input. -Only `abc.*` values move; cyclomatic, cognitive and Halstead are -unchanged. +Every entry under **Fixed** changes ABC values for valid input, mostly +`conditions` (one Elixir case changes `assignments`). Only `abc.*` +values move; cyclomatic, cognitive and Halstead are unchanged. ### Fixed @@ -37,8 +37,9 @@ unchanged. now count only where they are applied. A fold over a comparison (`(a < ...)`, `(0 == ... == a)`) now counts once, through the fold's `operator` field, where `<` / `>` folds scored 0 and a binary fold - scored 2. `#if A <= B` still counts. In the openfst corpus every - `operator==` / `operator!=` definition and declaration drops by one. + scored 2. `#if A <= B` still counts. In the openfst and kenlm corpora each + comparison-overload definition and declaration (`operator==`, + `operator!=`, `operator>=`) drops by one. - **C++ and Mozcpp score the ISO alternative tokens like their symbols** (#1470). `not b`, `b and c`, `b or c` and `b not_eq c` scored 0 where `!b`, `b && c`, `b || c` and `b != c` score 1, 2, 2 and 1. @@ -49,7 +50,10 @@ unchanged. predicate, so each of its patterns is one operand of an implicit `||` and scores once unless its own operator already counted it: `when a, b` and `when a, x > 1` now score 2, level with `if a || b`. - A subject-ful `case x; when 1` still scores one per clause. + A subject-ful `case x; when 1` still scores one per clause. A + parenthesised statement sequence in a condition (`if (x; x > 1)`) is + now read by its last statement, the value Ruby evaluates it to, where + the first was read before. - **A Perl bare pattern match scores by use** (#1467). `/^#/` and `m{^#}` match the implicit `$_`, and scored a condition only inside a boolean slot, so `my $r = /^#/;` read 0 against `$x =~ /^#/`'s 1. They diff --git a/big-code-analysis-book/src/metrics.md b/big-code-analysis-book/src/metrics.md index 5deec1604..30b266bf8 100644 --- a/big-code-analysis-book/src/metrics.md +++ b/big-code-analysis-book/src/metrics.md @@ -153,7 +153,8 @@ application would over-count. | 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#, Java, Rust, Python, Ruby, Elixir | A pattern-match guard is a condition slot, scoring one however it is spelled | A guard is a branch: the pattern can match while the guard fails. The guard's expression is scored exactly as an `if` condition is — one for a call, type test, attribute 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. Before this, ABC scored whatever operator happened to sit inside, so `when x > 2` counted one and the equivalent `when IsEven(x)` counted none. The spellings, per language: C# `when_clause` on a switch arm or `case` section and `catch_filter_clause` on a `catch` (#1422); Java 21's `guard` on a pattern-switch label, in both the arrow and colon forms; Rust's `match_pattern` guard; Python's `case … if g:`; Ruby's `if_guard` / `unless_guard` on a `case … in` arm; Elixir's `when` operator on a `stab_clause` head or a `def` / `defguard` head (#1454). The same change made the guard a **cyclomatic** decision wherever it was not already one — Java, Ruby and Elixir — and excluded Elixir's typespec `when` (`@spec f(a) :: a when a: integer`), which spells the same token as a guard but is type syntax. Groovy and Kotlin are absent because neither pinned grammar has a guard production at all: Kotlin 2.1 guard syntax does not parse, so there is nothing to classify (#1454). | | 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 TSX, JavaScript and Mozjs — TypeScript carries the same gate through the shared macro, but the `.ts` dialect of tree-sitter-typescript has no JSX production, so it has nothing to exclude there; 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 is on the operator alone, not on the test that encloses it: since #1461 the `is` test itself is a condition wherever it is written, so `bool b = x is > 0;` and `return x is > 0;` each score 1 — level with the `x > 0` they are sugar for, and the same 1 a `when n is > 5` guard has scored since #1422 made the guard a condition slot. Counting the pattern's operator as well would make a relational arm worth twice the constant arm `5 => 1`. 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). The exclusion is ABC's alone: Halstead counts vocabulary rather than decisions, so `bca ops` still bills every one of these `<` / `>` as an operator, and the two metrics are meant to disagree here (#1395) — see [the Halstead delimiter rule](#halstead). | +| Ruby | A subject-less `case` scores each `when` pattern as an `\|\|` operand; a subject-ful `case` scores per clause | A subject-less clause (`case; when x > 5`) lists ordinary boolean expressions, evaluated like an `if` predicate, and several patterns (`when a, b`) are an implicit `\|\|`. Each pattern therefore scores once unless its own operator already counted it, so `when x > 5` scores 1 like `if x > 5`, and `when a, b` scores 2 like `if a \|\| b`. Before this, a blanket per-clause increment counted a comparison twice. A subject-ful clause (`case x; when 1`) matches each pattern with an implicit `pattern === x` that nothing in the source spells, so the clause pays once (#1453). | +| 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 TSX, JavaScript and Mozjs — TypeScript carries the same gate through the shared macro, but the `.ts` dialect of tree-sitter-typescript has no JSX production, so it has nothing to exclude there; 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. C++ and Mozcpp do the same for their seven, `operator<=>` included, and also exclude the operator a comparison *fold* spells twice: `(0 == ... == a)` counts once, like `(a == ...)` (#1448). Their ISO alternative tokens `not_eq`, `not`, `and` and `or` count exactly as `!=`, `!`, `&&` and `\|\|` do (#1470). The exclusion is on the operator alone, not on the test that encloses it: since #1461 the `is` test itself is a condition wherever it is written, so `bool b = x is > 0;` and `return x is > 0;` each score 1 — level with the `x > 0` they are sugar for, and the same 1 a `when n is > 5` guard has scored since #1422 made the guard a condition slot. Counting the pattern's operator as well would make a relational arm worth twice the constant arm `5 => 1`. 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). The exclusion is ABC's alone: Halstead counts vocabulary rather than decisions, so `bca ops` still bills every one of these `<` / `>` as an operator, and the two metrics are meant to disagree here (#1395) — see [the Halstead delimiter rule](#halstead). | | 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. | | C#, Java, Groovy, Kotlin, Ruby, Elixir, Perl | A relational operator scores by use; a value-bearing operand scores in a boolean slot | Five of these grammars spell at least one relational construct as its own production rather than as a binary expression with an operator token: C#'s `x is int` and `x is null`, Java's and Groovy's `x instanceof T`, Groovy's `a in l`, Kotlin's `a is T` and `a in 1..2`, and Ruby's one-line `a in Integer`. Having no token to count, each was reached only through the language's terminal-operand set, which the walker consults inside an `if` / `while` / ternary / `&&`-operand slot and nowhere else — so `var b = x == 1;` scored 1 while `var b = x is int;` scored 0. Fitzpatrick's Rule 5 counts a relational operator wherever it appears, so each now counts wherever it appears and has left the operand set; being in both would score it twice. Groovy's spaceship `<=>` counts on the same rule, as it already did in Ruby, PHP, C++ and Mozcpp, although its result is an integer rather than a boolean — what Rule 5 measures is the comparison, not its type. Elixir reached the same 0 by the other route: its membership and type tests (`a in [1, 2]`, `rescue e in RuntimeError`) *do* carry an operator token, and simply had no arm matching it. They count by use on the same rule, gated on the token's parent so that an operator merely *named* (`&in/2`) stays excluded alongside `<` and `>`. Perl's bare pattern match (`my $r = /^#/;`, or `m{^#}`) matches the implicit `$_` with no operator token at all, and scores by use on the same rule, level with `$x =~ /^#/` (#1467). Two patterns are excluded: the one bound by `=~` / `!~`, whose operator already counts, and the one `split` takes first (`split /,/, $s`), which is a delimiter rather than a match. The converse still holds for a construct whose *value* fills the slot: a cast, a Go type assertion, a Rust `matches!` / `cfg!` macro and a Python walrus are all operands and count only where a slot reads them as a predicate (#1461). | diff --git a/src/metrics/abc.rs b/src/metrics/abc.rs index 3b467d139..a48f64dda 100644 --- a/src/metrics/abc.rs +++ b/src/metrics/abc.rs @@ -16428,8 +16428,10 @@ mod numeric_bool_operands { /// - **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. +/// covered — a fixture carrying only the first would have left the +/// `m{}` half at zero and read as covered. Since #1467 they take the +/// other route: a gated arm in `PerlCode::compute` counts them by use, +/// and they have left `perl_bool_terminal_kinds!()`. /// - **Ruby** `a in Integer` (`test_pattern`), the one-line pattern /// test. Decided against `match_pattern` (`expr => pat`), which /// raises rather than yielding a boolean. @@ -16463,12 +16465,13 @@ mod numeric_bool_operands { /// 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` +/// these two slots, and which one is missed differs by language. Perl +/// is no longer one of them: since #1467 its rows score through the +/// by-use arm, not through any consumer of the terminal set. Ruby's +/// `ruby_count_unary_conditions` is the remaining one /// (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 +/// instead). It is 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. /// @@ -16582,14 +16585,15 @@ mod own_production_bool_constructs { &["a in Integer", "!(a in Integer)"], 2, ), - // Objective-C's `@available` is not relational like the three - // above, but it reaches the terminal set by the same route: a - // dedicated `available_expression` production whose value is a - // boolean, seen by no comparison-token arm (#1457). Both - // spellings are listed because they are alternatives of one - // rule's leading token rather than two rules — so unlike - // Perl's `m{}` above they cannot drift apart, and the second - // row is here to keep that claim measured rather than assumed. + // Objective-C's `@available` is not relational like the + // constructs above, but it reaches the terminal set by the + // route Groovy's `in` takes: a dedicated `available_expression` + // production whose value is a boolean, seen by no + // comparison-token arm (#1457). Both spellings are listed + // because they are alternatives of one rule's leading token + // rather than two rules — so unlike Perl's `m{}` above they + // cannot drift apart, and the second row is here to keep that + // claim measured rather than assumed. LANG::Objc => ( [ ("int f(int a) {\n return {} && b;\n}\n", 2, 3), @@ -17835,6 +17839,19 @@ mod wrapper_peel_routing { "def f(b, c)\n x = c ? 1 : @\nend\n", &[("!b", 3), ("not b", 3), ("(b)", 2), ("-b", 2)], ); + // A statement sequence evaluates to its last statement, so that is + // the operand the slot reads: `(b; b > 1)` is the comparison, + // already counted by its own `>`, and `(b > 1; b)` is the bare `b`, + // which the slot counts beside that `>`. Reading the first + // statement inverted both. The subject-less `when` goes through + // the same peel. + let seqs: &[(&str, u64)] = &[("(b; b > 1)", 1), ("(b > 1; b)", 2)]; + assert_operands(LANG::Ruby, "def f(b)\n if @ then 1 end\nend\n", seqs); + assert_operands( + LANG::Ruby, + "def f(b)\n case\n when @ then 1\n end\nend\n", + seqs, + ); } #[test] diff --git a/src/metrics/abc/ruby.rs b/src/metrics/abc/ruby.rs index 6f521c231..8ed5b7cee 100644 --- a/src/metrics/abc/ruby.rs +++ b/src/metrics/abc/ruby.rs @@ -9,7 +9,7 @@ clippy::cast_sign_loss )] -use super::{Abc, Stats, wrapped_operand}; +use super::{Abc, Stats, is_operand}; use crate::macros::ruby_bool_terminal_kinds; use crate::*; @@ -55,16 +55,17 @@ use crate::*; // `unary` operators (`-b`, `~b`, `defined?`) are arithmetic or // introspection, never a boolean slot's operand, so the peel declines. // -// A negation names its `operand`; `parenthesized_statements` wraps its -// body in named children, the first of which that is not an extra -// carries the expression. Both are read by role because a comment may +// A negation names its `operand`; `parenthesized_statements` wraps a +// statement sequence and evaluates to its *last* statement (`(x; y)` is +// `y`), so the peel reads the last child that is not an extra, as +// Perl's `perl_last_operand` does for a list. Both are read by role because a comment may // sit before the operand — `(# c` or `! # c` — and a positional or // first-named-child read handed the comment to the slot (#1455). fn ruby_wrapper_operand<'a>(node: &Node<'a>) -> Option<(Node<'a>, bool)> { use Ruby::*; match node.kind_id().into() { - ParenthesizedStatements => Some((wrapped_operand(node)?, false)), + ParenthesizedStatements => Some((node.children().filter(is_operand).last()?, false)), Unary | Unary2 | Unary3 | Unary4 | Unary5 => { match node.child_by_field_name("operator")?.kind_id().into() { BANG | Not => Some((node.child_by_field_name("operand")?, true)), From f4b632502b5eb5af1736c2a6b9f7640bb3932a41 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic <490392+dekobon@users.noreply.github.com> Date: Thu, 8 Oct 2026 03:34:53 -0700 Subject: [PATCH 14/17] refactor(abc): share the last-operand read C#'s declarator arm, Ruby's statement-sequence peel and Perl's list peel each spelled "last child that is named and not an extra". They now share `last_operand` beside `wrapped_operand`. Also separates the Lua and Python peel comments from the module overview and fixes a member count in a Perl test comment. --- src/metrics/abc.rs | 10 +++++++++- src/metrics/abc/csharp.rs | 4 ++-- src/metrics/abc/lua.rs | 1 + src/metrics/abc/perl.rs | 20 ++++---------------- src/metrics/abc/python.rs | 11 ++++++----- src/metrics/abc/ruby.rs | 12 ++++++------ 6 files changed, 28 insertions(+), 30 deletions(-) diff --git a/src/metrics/abc.rs b/src/metrics/abc.rs index a48f64dda..a678cdaa1 100644 --- a/src/metrics/abc.rs +++ b/src/metrics/abc.rs @@ -443,6 +443,14 @@ pub(super) fn wrapped_operand<'a>(node: &Node<'a>) -> Option> { node.children().find(is_operand) } +// The last operand of `node`, by the same rule as `wrapped_operand`: for +// a wrapper that evaluates to its final element (a Perl list in scalar +// context, a Ruby statement sequence) and for a slot that follows an +// unnamed one (a C# declarator's initialiser after its name). +pub(super) fn last_operand<'a>(node: &Node<'a>) -> Option> { + node.children().filter(is_operand).last() +} + // Whether `child` can occupy an operand slot: named, and not an `extra`. // The `extra` flag is read off the tree-sitter node because the `Node` // wrapper exposes no accessor for it. @@ -13531,7 +13539,7 @@ end // The pattern `split` takes first is its delimiter, not a match // against `$_`, so it is no condition (#1467) in any spelling: the // spaced and parenthesised call forms, the `m{}` pattern, and the - // `CORE::` qualifier. Two discriminating members keep the exclusion + // `CORE::` qualifier. Three discriminating members keep the exclusion // narrow: `spl2` puts the pattern in `split`'s *second* slot, where // it is an ordinary match whose result `split` receives, `grpe` // puts one first in a call that is not `split`, and `tsplit` puts diff --git a/src/metrics/abc/csharp.rs b/src/metrics/abc/csharp.rs index b85e2094e..fafd9cf41 100644 --- a/src/metrics/abc/csharp.rs +++ b/src/metrics/abc/csharp.rs @@ -9,7 +9,7 @@ clippy::cast_sign_loss )] -use super::{Abc, Stats, is_operand, wrapped_operand}; +use super::{Abc, Stats, last_operand, wrapped_operand}; use crate::macros::{ csharp_bool_terminal_kinds, csharp_paren_expr_kinds, csharp_prefix_unary_expr_kinds, }; @@ -692,7 +692,7 @@ fn csharp_walk_for_conditions<'a>( // value, and a declarator's initialiser follows its name. ReturnStatement => csharp_inspect_slot(wrapped_operand(node), node, conds), crate::Csharp::VariableDeclarator | crate::Csharp::VariableDeclarator2 => { - csharp_inspect_slot(node.children().filter(is_operand).last(), node, conds); + csharp_inspect_slot(last_operand(node), node, conds); } AssignmentExpression => csharp_inspect_slot(node.child_by_field_name("right"), node, conds), LambdaExpression => csharp_inspect_slot(node.child_by_field_name("body"), node, conds), diff --git a/src/metrics/abc/lua.rs b/src/metrics/abc/lua.rs index e9340f8b0..f70cb937e 100644 --- a/src/metrics/abc/lua.rs +++ b/src/metrics/abc/lua.rs @@ -30,6 +30,7 @@ use crate::*; // operators `and` / `or` are deliberately NOT counted; see the // module-level `Stats` doc-comment for the cross-language policy // (issue #395, walker tracked in #403). + // One step of the `(...)` / `not` peel: the operand a wrapper wraps, and // whether the wrapper itself proves that operand boolean. `None` for // anything this peel does not descend, which is also the answer diff --git a/src/metrics/abc/perl.rs b/src/metrics/abc/perl.rs index 7f2154769..92ab3e20f 100644 --- a/src/metrics/abc/perl.rs +++ b/src/metrics/abc/perl.rs @@ -9,7 +9,7 @@ clippy::cast_sign_loss )] -use super::{Abc, Stats, is_operand, wrapped_operand}; +use super::{Abc, Stats, is_operand, last_operand, wrapped_operand}; use crate::macros::perl_bool_terminal_kinds; use crate::*; @@ -128,7 +128,7 @@ fn perl_inspect_container(container_node: &Node, parent: &Node, conditions: &mut // hold one operand. Neither is read by index: a comment may sit // before or after the operand (`(# c` / `! # c`, #1455). let next = if matches!(node_kind, P::Array) { - perl_last_operand(&node) + last_operand(&node) } else { wrapped_operand(&node) }; @@ -163,18 +163,6 @@ fn perl_inspect_slot(slot: Option, parent: &Node, conditions: &mut f64) { // `perl_count_unary_conditions`; condition-slot Arrays are // already unwrapped by `perl_inspect_container`. This predicate // disambiguates by checking the parent kind. -// Returns the last operand of a node — its last named child that is -// not an extra — or None if there is none. Used by -// `perl_inspect_container` to descend through the `Array` `(...)` -// wrapper: for a single-element grouping `($a)` the last operand is -// `$a`; for a multi-element list literal `($x, $y)` it is `$y` (the -// value the expression evaluates to in Perl's scalar context, which is -// the only context the walker operates in). A trailing comment -// (`($a # c⏎)`) is skipped rather than returned (#1455). -fn perl_last_operand<'a>(node: &Node<'a>) -> Option> { - node.children().filter(is_operand).last() -} - // Phase-2B (issues #403 / #1102): a ternary's condition and its two // branch operands are each a Fitzpatrick Rule 9 unary condition, exactly // as `java_walk_ternary` already counts them. Without this Perl scored @@ -290,7 +278,7 @@ fn perl_walk_for_statement(node: &Node, conditions: &mut f64) { // wrapping an `array`). An empty wrapper peels to nothing, so that // arm and a bare `None` score alike — measured by perturbation, not // assumed. A test would pin a value neither branch decides. -// - `perl_last_operand` returns `None` only for an `arguments` +// - `last_operand` returns `None` only for an `arguments` // node with no operand, which the grammar's comma-separated // one-or-more list cannot produce. fn perl_walk_statement_modifier(node: &Node, conditions: &mut f64) { @@ -298,7 +286,7 @@ fn perl_walk_statement_modifier(node: &Node, conditions: &mut f64) { return; }; let slot = if matches!(condition.kind_id().into(), Perl::Arguments) { - perl_last_operand(&condition) + last_operand(&condition) } else { Some(condition) }; diff --git a/src/metrics/abc/python.rs b/src/metrics/abc/python.rs index 3f85574e3..ea7047f62 100644 --- a/src/metrics/abc/python.rs +++ b/src/metrics/abc/python.rs @@ -41,13 +41,14 @@ use crate::*; // paper's "unary conditional expression". See the module-level // `Stats` doc-comment for the cross-language `&&` / `||` policy // (issue #395, walker tracked in #403). + // One step of the `(...)` peel: the expression a parenthesis wraps — // its only operand, not child(1) after the `(` token, which is where a -// comment sits in `if ( # c` (#1455). `None` for anything else, which is also -// the answer `python_count_condition` asks for, so the slot and the peel -// cannot disagree about which kinds are wrappers (#1470; the Kotlin, -// Groovy and C# instances of that disagreement were #1459, #1466 and -// #1463). +// comment sits in `if ( # c` (#1455). `None` for anything else, which +// is also the answer `python_count_condition` asks for, so the slot and +// the peel cannot disagree about which kinds are wrappers (#1470; the +// Kotlin, Groovy and C# instances of that disagreement were #1459, +// #1466 and #1463). // // Unlike its siblings this returns no "proves boolean" flag, because no // wrapper it accepts can prove one: `not` is deliberately not peeled — diff --git a/src/metrics/abc/ruby.rs b/src/metrics/abc/ruby.rs index 8ed5b7cee..4715a19db 100644 --- a/src/metrics/abc/ruby.rs +++ b/src/metrics/abc/ruby.rs @@ -9,7 +9,7 @@ clippy::cast_sign_loss )] -use super::{Abc, Stats, is_operand}; +use super::{Abc, Stats, last_operand}; use crate::macros::ruby_bool_terminal_kinds; use crate::*; @@ -57,15 +57,15 @@ use crate::*; // // A negation names its `operand`; `parenthesized_statements` wraps a // statement sequence and evaluates to its *last* statement (`(x; y)` is -// `y`), so the peel reads the last child that is not an extra, as -// Perl's `perl_last_operand` does for a list. Both are read by role because a comment may -// sit before the operand — `(# c` or `! # c` — and a positional or -// first-named-child read handed the comment to the slot (#1455). +// `y`), so the peel reads its `last_operand`, as Perl's peel does for +// a list. Both are read by role because a comment may sit before the +// operand — `(# c` or `! # c` — and a positional or first-named-child +// read handed the comment to the slot (#1455). fn ruby_wrapper_operand<'a>(node: &Node<'a>) -> Option<(Node<'a>, bool)> { use Ruby::*; match node.kind_id().into() { - ParenthesizedStatements => Some((node.children().filter(is_operand).last()?, false)), + ParenthesizedStatements => Some((last_operand(node)?, false)), Unary | Unary2 | Unary3 | Unary4 | Unary5 => { match node.child_by_field_name("operator")?.kind_id().into() { BANG | Not => Some((node.child_by_field_name("operand")?, true)), From 09283fc10b6d372b9dfc203f2184dd0b00c34058 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic <490392+dekobon@users.noreply.github.com> Date: Thu, 8 Oct 2026 03:41:49 -0700 Subject: [PATCH 15/17] fix(abc/ruby): pay a when chain with no counted operand `ruby_condition_scores_itself` treated every `&&` / `||` chain as already counted, but the Rule 9 walker counts plain operands only, so `case; when -a && -b` and `when (a = 1) && (b = 2)` scored 0 against 1 on main and against `when -a`'s 1. A chain now counts as paid only if some operand, at any depth, is a comparison, pattern test, ternary or a plain operand the walker counts. The walk is a worklist so a long left-nested chain cannot recurse deeply. --- src/metrics/abc.rs | 27 +++++++++++++++++++++++ src/metrics/abc/ruby.rs | 49 +++++++++++++++++++++++++++-------------- 2 files changed, 60 insertions(+), 16 deletions(-) diff --git a/src/metrics/abc.rs b/src/metrics/abc.rs index a678cdaa1..aa9ed4004 100644 --- a/src/metrics/abc.rs +++ b/src/metrics/abc.rs @@ -17862,6 +17862,33 @@ mod wrapper_peel_routing { ); } + /// A subject-less `when` whose pattern is a chain owes its clause + /// condition unless some operand of the chain is counted by another + /// arm. `-a && -b` and `(a = 1) && (b = 2)` have no such operand — + /// the Rule 9 walker counts plain operands only — so the clause pays, + /// as it does for a bare `when -a`; treating every chain as paid + /// scored them 0 (found by review of #1453). The other rows keep the + /// double count #1453 removed out: a counted operand anywhere in the + /// chain, at any depth, pays for it. + #[test] + #[cfg(feature = "ruby")] + fn ruby_subjectless_when_chain_pays_only_through_a_counted_operand() { + assert_operands( + LANG::Ruby, + "def f(a, b, c)\n case\n when @ then 1\n end\nend\n", + &[ + ("-a", 1), + ("-a && -b", 1), + ("(a = 1) && (b = 2)", 1), + ("-a || -b || -c", 1), + ("-a && b", 1), + ("a > 1 && -b", 1), + ("-a && (-b || c)", 1), + ("a && b", 2), + ], + ); + } + #[test] #[cfg(feature = "lua")] fn lua_condition_slot_routes_what_the_peel_accepts() { diff --git a/src/metrics/abc/ruby.rs b/src/metrics/abc/ruby.rs index 4715a19db..a16aebf5b 100644 --- a/src/metrics/abc/ruby.rs +++ b/src/metrics/abc/ruby.rs @@ -207,10 +207,15 @@ macro_rules! ruby_comparison_kinds { // Whether another arm of `compute` already charges `expr` as a condition, // looking through `(…)` and `!` / `not` layers, which add no decision of -// their own. True for a comparison (the token arm), an `&&` / `||` / -// `and` / `or` chain (the Rule 9 walker), a one-line pattern test (the -// `TestPattern` arm) and a ternary (the `?` arm plus `ruby_walk_ternary`) -// — exactly the expressions the `if` predicate slot leaves to those arms. +// their own. True for a comparison (the token arm), a one-line pattern +// test (the `TestPattern` arm) and a ternary (the `?` arm plus +// `ruby_walk_ternary`). An `&&` / `||` / `and` / `or` chain is paid only +// if some operand is: one of those, or a plain operand the Rule 9 +// walker counts. `-a && -b` has neither, so the walker scores it 0 and +// the clause must still pay, exactly as it does for `when -a`. +// +// A worklist rather than recursion: a left-nested chain is as deep as +// it is long, and the input is untrusted source. // // It answers "is the decision already paid for", not "is this a boolean // slot", which is why a subject-less `when` asks this rather than routing @@ -224,25 +229,37 @@ macro_rules! ruby_comparison_kinds { fn ruby_condition_scores_itself(expr: &Node) -> bool { use Ruby::*; - let mut node = *expr; - loop { + // (node, whether it is an operand of an enclosing chain) + let mut pending = vec![(*expr, false)]; + while let Some((mut node, in_chain)) = pending.pop() { + // The same wrappers the condition peel descends, by construction. + while let Some((operand, _)) = ruby_wrapper_operand(&node) { + node = operand; + } match node.kind_id().into() { Binary | Binary2 | Binary3 => { - return node.child_by_field_name("operator").is_some_and(|op| { - let op = op.kind_id().into(); - matches!(op, ruby_comparison_kinds!()) - || matches!(op, AMPAMP | PIPEPIPE | And | Or) - }); + let Some(op) = node.child_by_field_name("operator") else { + continue; + }; + let op = op.kind_id().into(); + if matches!(op, ruby_comparison_kinds!()) { + return true; + } + if matches!(op, AMPAMP | PIPEPIPE | And | Or) { + pending.extend( + ["left", "right"] + .into_iter() + .filter_map(|field| node.child_by_field_name(field)) + .map(|operand| (operand, true)), + ); + } } TestPattern | Conditional => return true, + kind if in_chain && matches!(kind, ruby_bool_terminal_kinds!()) => return true, _ => {} } - // The same wrappers the condition peel descends, by construction. - let Some((operand, _)) = ruby_wrapper_operand(&node) else { - return false; - }; - node = operand; } + false } // Scores one `when` clause (#1453, transferring #1421's Kotlin rule). From b7a2b2368bc63ed29f64eecdf353fae285817537 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic <490392+dekobon@users.noreply.github.com> Date: Thu, 8 Oct 2026 04:16:20 -0700 Subject: [PATCH 16/17] refactor(abc): drop unreachable branches from peels The wrapper peels spelled a grammar-required child as `child?`, and the C++, JS and PHP peel loops had a second `let ... else { break }` after the non-wrapper exit, so each carried a `None` branch no valid tree reaches, reported as uncovered patch lines. The peels now map the optional child (`wrapped_operand(node).map(...)`, guards via `is_some_and`), the three loops end through one exit, and two table-driven tests filter disabled languages instead of `continue`-ing past them. Behaviour is unchanged. --- src/metrics/abc.rs | 10 ++-------- src/metrics/abc/cpp.rs | 14 ++++++-------- src/metrics/abc/csharp.rs | 11 ++++++----- src/metrics/abc/go.rs | 6 +++--- src/metrics/abc/groovy.rs | 2 +- src/metrics/abc/java.rs | 6 +++--- src/metrics/abc/js_family.rs | 17 +++++++++-------- src/metrics/abc/kotlin.rs | 2 +- src/metrics/abc/lua.rs | 10 +++++++--- src/metrics/abc/php.rs | 17 +++++++++-------- src/metrics/abc/ruby.rs | 31 +++++++++++++++---------------- src/metrics/abc/rust.rs | 6 +++--- 12 files changed, 65 insertions(+), 67 deletions(-) diff --git a/src/metrics/abc.rs b/src/metrics/abc.rs index aa9ed4004..0d4f9169f 100644 --- a/src/metrics/abc.rs +++ b/src/metrics/abc.rs @@ -18206,10 +18206,7 @@ mod slot_comment_invariance { #[test] fn a_comment_in_a_slot_changes_nothing() { let mut checked = 0; - for lang in LANG::into_enum_iter() { - if !lang.is_enabled() { - continue; - } + for lang in LANG::into_enum_iter().filter(LANG::is_enabled) { let Some(spelling) = spelling(lang) else { continue; }; @@ -18327,10 +18324,7 @@ mod slot_role_reads { #[test] fn a_slot_is_read_by_role() { let mut checked = 0; - for &(lang, plain, variant, expected) in ROWS { - if !lang.is_enabled() { - continue; - } + for &(lang, plain, variant, expected) in ROWS.iter().filter(|row| row.0.is_enabled()) { let without = abc(lang, plain); assert_eq!( abc(lang, variant), diff --git a/src/metrics/abc/cpp.rs b/src/metrics/abc/cpp.rs index c03b63874..5bd5ad92b 100644 --- a/src/metrics/abc/cpp.rs +++ b/src/metrics/abc/cpp.rs @@ -80,22 +80,20 @@ pub(super) fn cpp_inspect_container(container_node: &Node, parent: &Node, condit .child(0) .is_some_and(|c| matches!(c.kind(), "!" | "not")); - if !is_parens && !is_not { - break; - } - if !has_boolean_content && is_not { - has_boolean_content = true; - } - // By role, never at child(1): a comment may sit there // (`if (/*c*/ b)`, #1455), and so may a clause's init-statement // (`if (int y = f(); b)`), whose condition is the `value` field. + // Anything that is not a wrapper has no operand, which ends the + // peel through the same exit as a missing one. let operand = if is_clause { node.child_by_field_name("value") - } else { + } else if is_parens || is_not { wrapped_operand(&node) + } else { + None }; let Some(child) = operand else { break }; + has_boolean_content |= is_not; node = child; node_kind = node.kind(); diff --git a/src/metrics/abc/csharp.rs b/src/metrics/abc/csharp.rs index fafd9cf41..aa25e9e1f 100644 --- a/src/metrics/abc/csharp.rs +++ b/src/metrics/abc/csharp.rs @@ -78,15 +78,16 @@ fn csharp_wrapper_operand<'a>(node: &Node<'a>) -> Option<(Node<'a>, bool)> { match node.kind_id().into() { // `(expr)` — the inner expression follows the `(` token. - csharp_paren_expr_kinds!() => Some((wrapped_operand(node)?, false)), + csharp_paren_expr_kinds!() => wrapped_operand(node).map(|o| (o, false)), // `!expr` — the operand follows the operator token. Seven other // prefix operators (`++ -- + - ~ & ^`) share this kind, as does // the `*` of a pointer indirection the grammar aliases onto it; // none is a boolean slot's operand. - csharp_prefix_unary_expr_kinds!() => match node.child(0)?.kind_id().into() { - BANG => Some((wrapped_operand(node)?, true)), - _ => None, - }, + csharp_prefix_unary_expr_kinds!() + if node.child(0).is_some_and(|c| c.kind_id() == BANG as u16) => + { + wrapped_operand(node).map(|o| (o, true)) + } // `expr!` — the null-forgiving operator. One kind id at the // pinned `=0.23.5`, no numbered aliases (§1). PostfixUnaryExpression if node.is_child(BANG as u16) => Some((node.child(0)?, false)), diff --git a/src/metrics/abc/go.rs b/src/metrics/abc/go.rs index 56d9299f5..02f2f1dea 100644 --- a/src/metrics/abc/go.rs +++ b/src/metrics/abc/go.rs @@ -29,9 +29,9 @@ fn go_wrapper_operand<'a>(node: &Node<'a>) -> Option<(Node<'a>, bool)> { use Go as G; match node.kind_id().into() { - G::ParenthesizedExpression => Some((wrapped_operand(node)?, false)), - G::UnaryExpression if node.child(0)?.kind_id() == G::BANG as u16 => { - Some((node.child_by_field_name("operand")?, true)) + G::ParenthesizedExpression => wrapped_operand(node).map(|o| (o, false)), + G::UnaryExpression if node.child(0).is_some_and(|c| c.kind_id() == G::BANG as u16) => { + node.child_by_field_name("operand").map(|o| (o, true)) } _ => None, } diff --git a/src/metrics/abc/groovy.rs b/src/metrics/abc/groovy.rs index f7ce71052..9ec7b7e3b 100644 --- a/src/metrics/abc/groovy.rs +++ b/src/metrics/abc/groovy.rs @@ -52,7 +52,7 @@ fn groovy_wrapper_operand<'a>(node: &Node<'a>) -> Option<(Node<'a>, bool)> { match node.kind_id().into() { // `(expr)` — the inner expression follows the `(` token. - ParenthesizedExpression => Some((wrapped_operand(node)?, false)), + ParenthesizedExpression => wrapped_operand(node).map(|o| (o, false)), UnaryExpression => { let operand = node.child_by_field_name("operand")?; match node.child_by_field_name("operator")?.kind_id().into() { diff --git a/src/metrics/abc/java.rs b/src/metrics/abc/java.rs index b33d0a32c..03ac45c2c 100644 --- a/src/metrics/abc/java.rs +++ b/src/metrics/abc/java.rs @@ -32,9 +32,9 @@ fn java_wrapper_operand<'a>(node: &Node<'a>) -> Option<(Node<'a>, bool)> { use Java::*; match node.kind_id().into() { - ParenthesizedExpression => Some((wrapped_operand(node)?, false)), - UnaryExpression if node.child(0)?.kind_id() == BANG as u16 => { - Some((node.child_by_field_name("operand")?, true)) + ParenthesizedExpression => wrapped_operand(node).map(|o| (o, false)), + UnaryExpression if node.child(0).is_some_and(|c| c.kind_id() == BANG as u16) => { + node.child_by_field_name("operand").map(|o| (o, true)) } _ => None, } diff --git a/src/metrics/abc/js_family.rs b/src/metrics/abc/js_family.rs index 32485edff..296bc366d 100644 --- a/src/metrics/abc/js_family.rs +++ b/src/metrics/abc/js_family.rs @@ -57,18 +57,19 @@ macro_rules! impl_js_family_unary_walker { 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; - } - // The wrapper's only operand, not child(1): a comment may // sit there (`(/*c*/ b)`, `! /*c*/ b` — #1455). - let Some(child) = wrapped_operand(&node) else { + // Anything that is not a wrapper has no operand, which ends the + // peel through the same exit as a missing one. + let operand = if is_parens || is_not { + wrapped_operand(&node) + } else { + None + }; + let Some(child) = operand else { break; }; + has_boolean_content |= is_not; node = child; node_kind = node.kind_id().into(); diff --git a/src/metrics/abc/kotlin.rs b/src/metrics/abc/kotlin.rs index 056f53262..c7408a11c 100644 --- a/src/metrics/abc/kotlin.rs +++ b/src/metrics/abc/kotlin.rs @@ -94,7 +94,7 @@ fn kotlin_wrapper_operand<'a>(node: &Node<'a>) -> Option<(Node<'a>, bool)> { match node.kind_id().into() { // `(expr)` — the inner expression follows the `(` token. - ParenthesizedExpression => Some((wrapped_operand(node)?, false)), + ParenthesizedExpression => wrapped_operand(node).map(|o| (o, false)), UnaryExpression => { let operand = node.child_by_field_name("argument")?; match node.child_by_field_name("operator")?.kind_id().into() { diff --git a/src/metrics/abc/lua.rs b/src/metrics/abc/lua.rs index f70cb937e..916443419 100644 --- a/src/metrics/abc/lua.rs +++ b/src/metrics/abc/lua.rs @@ -49,9 +49,13 @@ use crate::*; // other call-argument contexts would never set the walker's flag. fn lua_wrapper_operand<'a>(node: &Node<'a>) -> Option<(Node<'a>, bool)> { match node.kind_id().into() { - Lua::ParenthesizedExpression => Some((wrapped_operand(node)?, false)), - Lua::UnaryExpression if node.child(0)?.kind_id() == Lua::Not as u16 => { - Some((node.child_by_field_name("operand")?, true)) + Lua::ParenthesizedExpression => wrapped_operand(node).map(|o| (o, false)), + Lua::UnaryExpression + if node + .child(0) + .is_some_and(|c| c.kind_id() == Lua::Not as u16) => + { + node.child_by_field_name("operand").map(|o| (o, true)) } _ => None, } diff --git a/src/metrics/abc/php.rs b/src/metrics/abc/php.rs index 2ade4b7fb..9b33f0d5d 100644 --- a/src/metrics/abc/php.rs +++ b/src/metrics/abc/php.rs @@ -42,18 +42,19 @@ fn php_inspect_container(container_node: &Node, parent: &Node, conditions: &mut let is_not = matches!(node_kind, UnaryOpExpression) && 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; - } - // The wrapper's only operand, not child(1): a comment may sit // there (`(/*c*/ $b)`, `! /*c*/ $b` — #1455). - let Some(child) = wrapped_operand(&node) else { + // Anything that is not a wrapper has no operand, which ends the + // peel through the same exit as a missing one. + let operand = if is_parens || is_not { + wrapped_operand(&node) + } else { + None + }; + let Some(child) = operand else { break; }; + has_boolean_content |= is_not; node = child; node_kind = node.kind_id().into(); diff --git a/src/metrics/abc/ruby.rs b/src/metrics/abc/ruby.rs index a16aebf5b..9a8e222de 100644 --- a/src/metrics/abc/ruby.rs +++ b/src/metrics/abc/ruby.rs @@ -65,12 +65,13 @@ fn ruby_wrapper_operand<'a>(node: &Node<'a>) -> Option<(Node<'a>, bool)> { use Ruby::*; match node.kind_id().into() { - ParenthesizedStatements => Some((last_operand(node)?, false)), - Unary | Unary2 | Unary3 | Unary4 | Unary5 => { - match node.child_by_field_name("operator")?.kind_id().into() { - BANG | Not => Some((node.child_by_field_name("operand")?, true)), - _ => None, - } + ParenthesizedStatements => last_operand(node).map(|o| (o, false)), + Unary | Unary2 | Unary3 | Unary4 | Unary5 + if node + .child_by_field_name("operator") + .is_some_and(|op| matches!(op.kind_id().into(), BANG | Not)) => + { + node.child_by_field_name("operand").map(|o| (o, true)) } _ => None, } @@ -238,20 +239,18 @@ fn ruby_condition_scores_itself(expr: &Node) -> bool { } match node.kind_id().into() { Binary | Binary2 | Binary3 => { - let Some(op) = node.child_by_field_name("operator") else { - continue; - }; - let op = op.kind_id().into(); - if matches!(op, ruby_comparison_kinds!()) { - return true; - } - if matches!(op, AMPAMP | PIPEPIPE | And | Or) { - pending.extend( + match node + .child_by_field_name("operator") + .map(|op| op.kind_id().into()) + { + Some(ruby_comparison_kinds!()) => return true, + Some(AMPAMP | PIPEPIPE | And | Or) => pending.extend( ["left", "right"] .into_iter() .filter_map(|field| node.child_by_field_name(field)) .map(|operand| (operand, true)), - ); + ), + _ => {} } } TestPattern | Conditional => return true, diff --git a/src/metrics/abc/rust.rs b/src/metrics/abc/rust.rs index 878803adb..05270de05 100644 --- a/src/metrics/abc/rust.rs +++ b/src/metrics/abc/rust.rs @@ -28,9 +28,9 @@ fn rust_wrapper_operand<'a>(node: &Node<'a>) -> Option<(Node<'a>, bool)> { use Rust::*; match node.kind_id().into() { - ParenthesizedExpression => Some((wrapped_operand(node)?, false)), - UnaryExpression if node.child(0)?.kind_id() == BANG as u16 => { - Some((wrapped_operand(node)?, true)) + ParenthesizedExpression => wrapped_operand(node).map(|o| (o, false)), + UnaryExpression if node.child(0).is_some_and(|c| c.kind_id() == BANG as u16) => { + wrapped_operand(node).map(|o| (o, true)) } _ => None, } From 775582e5a0105e4f5852b28e72cea4eeac0046b2 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic <490392+dekobon@users.noreply.github.com> Date: Thu, 8 Oct 2026 04:26:52 -0700 Subject: [PATCH 17/17] refactor(abc/ruby): chain the negation peel step --- src/metrics/abc/ruby.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/metrics/abc/ruby.rs b/src/metrics/abc/ruby.rs index 9a8e222de..c08b909f8 100644 --- a/src/metrics/abc/ruby.rs +++ b/src/metrics/abc/ruby.rs @@ -66,13 +66,11 @@ fn ruby_wrapper_operand<'a>(node: &Node<'a>) -> Option<(Node<'a>, bool)> { match node.kind_id().into() { ParenthesizedStatements => last_operand(node).map(|o| (o, false)), - Unary | Unary2 | Unary3 | Unary4 | Unary5 - if node - .child_by_field_name("operator") - .is_some_and(|op| matches!(op.kind_id().into(), BANG | Not)) => - { - node.child_by_field_name("operand").map(|o| (o, true)) - } + Unary | Unary2 | Unary3 | Unary4 | Unary5 => node + .child_by_field_name("operator") + .filter(|op| matches!(op.kind_id().into(), BANG | Not)) + .and_then(|_| node.child_by_field_name("operand")) + .map(|o| (o, true)), _ => None, } }