From 4e4b38d573f90f7229ed0ce68b4da12c2bce595d Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 9 Aug 2026 12:03:16 -0700 Subject: [PATCH 01/16] fix(abc/ruby): gate LT/GT comparisons on a binary parent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The superclass clause of a class definition spells its `<` with the same LT token as a comparison, so every subclass declaration scored a phantom ABC condition. Count LT/GT only under a `binary` parent — the positive polarity the other guarded languages use, and forward-proof against future non-comparison `<` uses. It also stops counting the `<` that names an operator method. Fixes #1280 --- CHANGELOG.md | 10 ++++++++++ src/metrics/abc.rs | 32 ++++++++++++++++++++++++++++++++ src/metrics/abc/ruby.rs | 22 +++++++++++++++++++--- 3 files changed, 61 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c8c3911fe..91f53f0a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -96,6 +96,16 @@ for historical reference. ### Fixed +- Ruby ABC counted the `<` of a superclass clause as a comparison + condition, so every subclass declaration scored a phantom condition — + `class Foo < Bar` with a single assignment inside reported + `conditions = 1` for a file with no conditional in it. `LT` / `GT` are + now counted only under a `binary` parent, the positive polarity Rust, + Go, C, C++, Java, Groovy, C#, Kotlin and the JS family already use. + The same gate stops counting the `<` that names an operator method + (`def <(other)`). Real comparisons, `<=>`, the `<<` shovel and heredoc + openers are unaffected. Ruby `abc` values drop by one per subclass + declaration and per operator-method definition (#1280). - `fix_includes` returned its diagnostics in a different order on every run for identical input, because both producers push while iterating a `HashMap` — `files` in `build_include_graph`, `nodes` in diff --git a/src/metrics/abc.rs b/src/metrics/abc.rs index e9a6e3ada..dcd7ac816 100644 --- a/src/metrics/abc.rs +++ b/src/metrics/abc.rs @@ -4734,6 +4734,38 @@ function f(int $a, int $b): int { ); } + #[test] + fn ruby_superclass_clause_is_not_a_condition() { + // Regression for #1280: a superclass clause spells its `<` with the + // same `LT` token as a comparison, but parents it under + // `superclass` rather than `binary`, so the parent gate excludes + // it. Before the gate every subclass declaration scored a phantom + // condition. + // expected: 0 conditions — the file contains no conditional at all; + // the single assignment is `x = 1`. + check_metrics::( + "class Foo < Bar\n def plain\n x = 1\n end\nend\n", + "foo.rb", + |metric| { + assert_eq!(metric.abc.conditions_sum(), 0); + assert_eq!(metric.abc.assignments_sum(), 1); + }, + ); + } + + #[test] + fn ruby_operator_method_name_is_not_a_condition() { + // The `<` naming an operator method parents under `operator`, which + // the `binary` gate likewise excludes (#1280). The body carries a + // real comparison so the expected value is not the all-zero default: + // 1 discriminates "only the name token is excluded" from both "the + // gate excludes everything" (0) and "nothing is gated" (2). + // expected: 1 condition — the `@v < other` comparison, not the `def <`. + check_metrics::("def <(other)\n @v < other\nend\n", "foo.rb", |metric| { + assert_eq!(metric.abc.conditions_sum(), 1); + }); + } + #[test] fn ruby_case_match_in_arms_are_conditions() { // Regression for #977: each non-wildcard `case … in` arm is one diff --git a/src/metrics/abc/ruby.rs b/src/metrics/abc/ruby.rs index 5a87b9243..431251d93 100644 --- a/src/metrics/abc/ruby.rs +++ b/src/metrics/abc/ruby.rs @@ -230,9 +230,25 @@ impl Abc for RubyCode { Call | Call2 | Call3 | Call4 | Super | Yield | Yield2 => { stats.branches += 1.; } - EQEQ | BANGEQ | EQEQEQ | LT | GT | LTEQ | GTEQ | LTEQGT | EQTILDE | BANGTILDE - | Else | Elsif | When | QMARK | Rescue | RescueModifier | RescueModifier2 - | RescueModifier3 => { + EQEQ | BANGEQ | EQEQEQ | LTEQ | GTEQ | LTEQGT | EQTILDE | BANGTILDE | Else | Elsif + | When | QMARK | Rescue | RescueModifier | RescueModifier2 | RescueModifier3 => { + stats.conditions += 1.; + } + // `<` and `>` are comparisons only inside a `binary` node. The + // same `<` token also spells a class's superclass clause + // (`class Foo < Bar`, parent `superclass`) and an operator + // method's name (`def <(other)`, parent `operator`), neither of + // which is a condition — the positive `binary` guard is the + // polarity Rust / Go / C / C++ already use, and stays correct + // for any future non-comparison `<` (#1280). `a < b` emits the + // `Binary2` alias; the two siblings are listed defensively per + // grammar-dispatch §1. `<<` is the distinct `LTLT` token and + // heredoc openers are their own tokens, so neither is affected. + LT | GT + if ancestors + .parent(node) + .is_some_and(|p| matches!(p.kind_id().into(), Binary | Binary2 | Binary3)) => + { stats.conditions += 1.; } // A `case … in` pattern-match arm is a branch condition exactly From d284110b9196b546bce457f1a4dd27f579edb733 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 9 Aug 2026 12:07:09 -0700 Subject: [PATCH 02/16] fix(abc): count constructor delegation as a branch super(...)/this(...) in Java, : base(...)/: this(...) in C#, and : super(...) in Kotlin each parse as their own production rather than the call kind those branch counters matched, so a delegation scored zero where Groovy scored one for identical source. None wraps a call node for the delegation itself, so no double count arises. Fixes #1279 --- CHANGELOG.md | 11 +++++ src/metrics/abc.rs | 87 +++++++++++++++++++++++++++++++++++++++ src/metrics/abc/csharp.rs | 7 ++++ src/metrics/abc/java.rs | 14 ++++++- src/metrics/abc/kotlin.rs | 6 ++- 5 files changed, 122 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 91f53f0a0..b325f3799 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -96,6 +96,17 @@ for historical reference. ### Fixed +- Constructor delegation scored zero ABC branches in Java, C# and + Kotlin. `super(…)` / `this(…)` parses as `explicit_constructor_invocation` + in Java, `: base(…)` / `: this(…)` as `constructor_initializer` in C#, + and `: super(…)` as `constructor_delegation_call` in Kotlin — none of + which is the call kind each language's branch counter matched, so a + delegation contributed nothing where Groovy scored one for identical + source. Each is now one branch, per Fitzpatrick's "one per function + call". Calls in a delegation's argument list are separate nodes and + still count on their own, so `super(f())` is two branches, not three. + Java, C# and Kotlin `abc` values rise by one per delegating + constructor (#1279). - Ruby ABC counted the `<` of a superclass clause as a comparison condition, so every subclass declaration scored a phantom condition — `class Foo < Bar` with a single assignment inside reported diff --git a/src/metrics/abc.rs b/src/metrics/abc.rs index dcd7ac816..86463dbbd 100644 --- a/src/metrics/abc.rs +++ b/src/metrics/abc.rs @@ -1497,6 +1497,93 @@ mod tests { ); } + #[test] + fn java_constructor_delegation_is_a_branch() { + // Regression for #1279: `super(…)` / `this(…)` parse as + // `explicit_constructor_invocation`, not `method_invocation`, so + // each scored zero branches while Groovy scored one for identical + // source. Both constructors delegate, and neither body contains any + // other call. + // expected: 2 branches — one delegation each. + check_metrics::( + "class Sub extends Base { + Sub() { super(1); } + Sub(int a) { this(); } + }", + "foo.java", + |metric| { + assert_eq!(metric.abc.branches_sum(), 2); + }, + ); + } + + #[test] + fn java_constructor_delegation_does_not_double_count_arguments() { + // The delegation node does not wrap a `method_invocation` for the + // call itself, so `super(f())` is exactly two branches — the + // delegation and the argument call — not three (#1279). + // expected: 2 branches. + check_metrics::( + "class Sub extends Base { + Sub() { super(f()); } + }", + "foo.java", + |metric| { + assert_eq!(metric.abc.branches_sum(), 2); + }, + ); + } + + #[test] + fn csharp_constructor_initializer_is_a_branch() { + // C# spells the same delegation as a `constructor_initializer` + // (`: base(…)` / `: this(…)`), which likewise scored zero (#1279). + // expected: 2 branches — one initializer each, no other calls. + check_metrics::( + "class Sub : Base { + Sub() : base(1) { } + Sub(int a) : this() { } + }", + "foo.cs", + |metric| { + assert_eq!(metric.abc.branches_sum(), 2); + }, + ); + } + + #[test] + fn kotlin_constructor_delegation_is_a_branch() { + // Kotlin's secondary-constructor delegation is a + // `constructor_delegation_call`, a distinct production from + // `CallExpression`, so it scored zero for the same reason (#1279). + // expected: 1 branch — the `: super(x)` delegation. + check_metrics::( + "class Sub : Base { + constructor(x: Int) : super(x) { } + }", + "foo.kt", + |metric| { + assert_eq!(metric.abc.branches_sum(), 1); + }, + ); + } + + #[test] + fn groovy_constructor_delegation_is_a_branch() { + // Groovy already counted this shape before #1279; the assertion + // pins the JVM-family parity the Java and Kotlin fixes restore. + // expected: 1 branch — the `super(1)` delegation. + check_metrics::( + "class Sub extends Base { + Sub() { super(1) } + }", + "foo.groovy", + |metric| { + assert_eq!(metric.abc.branches_sum(), 1); + }, + ); + } + #[test] fn groovy_no_abc() { // Comment-only file has no executable code → all-zero ABC. diff --git a/src/metrics/abc/csharp.rs b/src/metrics/abc/csharp.rs index e3e1ccf33..e908c1285 100644 --- a/src/metrics/abc/csharp.rs +++ b/src/metrics/abc/csharp.rs @@ -148,6 +148,12 @@ fn csharp_count_token_assignment(node: &Node, stats: &mut Stats) -> bool { true } +// `ConstructorInitializer` is the `: base(…)` / `: this(…)` delegation on a +// constructor — a call by Fitzpatrick's rule, and the C# spelling of the +// shape Java and Groovy count (#1279). Unlike the invocation kinds it +// carries no numeric-suffix aliases, and it does not wrap an +// `InvocationExpression` for the delegation itself, so no double count +// arises; calls in its argument list are separate nodes counted on their own. fn csharp_count_token_branch(node: &Node, stats: &mut Stats) -> bool { use Csharp::*; if matches!( @@ -156,6 +162,7 @@ fn csharp_count_token_branch(node: &Node, stats: &mut Stats) -> bool { | crate::Csharp::InvocationExpression2 | crate::Csharp::InvocationExpression3 | ObjectCreationExpression + | ConstructorInitializer ) { stats.branches += 1.; return true; diff --git a/src/metrics/abc/java.rs b/src/metrics/abc/java.rs index b32d676bd..60724153e 100644 --- a/src/metrics/abc/java.rs +++ b/src/metrics/abc/java.rs @@ -168,10 +168,20 @@ fn java_count_token_assignment(node: &Node, stats: &mut Stats) -> bool { true } -// Counts branch tokens: every method call or `new` allocation. +// Counts branch tokens: every method call, `new` allocation, and +// constructor delegation. +// `ExplicitConstructorInvocation` is the `super(…)` / `this(…)` delegation +// at the head of a constructor — a call by Fitzpatrick's rule, and one +// Groovy already counted for identical source (#1279). It is a distinct +// production that does not wrap a `MethodInvocation` for the delegation +// itself, so listing it adds no double count; calls in its argument list +// are separate nodes and still count on their own. fn java_count_token_branch(node: &Node, stats: &mut Stats) -> bool { use Java::*; - if matches!(node.kind_id().into(), MethodInvocation | New) { + if matches!( + node.kind_id().into(), + MethodInvocation | New | ExplicitConstructorInvocation + ) { stats.branches += 1.; return true; } diff --git a/src/metrics/abc/kotlin.rs b/src/metrics/abc/kotlin.rs index 1b6183bea..632ef7f67 100644 --- a/src/metrics/abc/kotlin.rs +++ b/src/metrics/abc/kotlin.rs @@ -190,7 +190,11 @@ impl Abc for KotlinCode { // access (`arr[i]`) is NOT a branch (it's an operator on a // sequence), matching the Java rule of "method invocation // only". - CallExpression => { + // `ConstructorDelegationCall` is a secondary constructor's + // `: super(…)` / `: this(…)`. It is a distinct production + // rather than a `CallExpression`, so it scored zero while the + // same source in Java, C# and Groovy scores one (#1279). + CallExpression | ConstructorDelegationCall => { stats.branches += 1.; } // Conditions: comparison operators, identity equality, From b2fd9a5b5ca474c034486c8db9ae2eb64e6709b0 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 9 Aug 2026 12:10:41 -0700 Subject: [PATCH 03/16] fix(abc/go): count var declarations that carry an initializer var x = 5 and x := 5 are the same binding spelled two ways, but only the latter matched an assignment arm. Count a var_spec with a value field, which covers both var x = 5 and var y int = 6 without matching an uninitialized var z int. Matching the spec rather than the declaration counts each initialized line of a grouped var block. Fixes #1278 --- CHANGELOG.md | 9 +++ src/metrics/abc.rs | 60 +++++++++++++++---- src/metrics/abc/go.rs | 15 +++++ ...t_plain_compound_short_var_and_incdec.snap | 8 +-- ...cs__abc__tests__go_calls_are_branches.snap | 8 +-- ...__abc__tests__go_complex_function_abc.snap | 8 +-- 6 files changed, 86 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b325f3799..6d5e24699 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -96,6 +96,15 @@ for historical reference. ### Fixed +- Go ABC scored zero assignments for a `var` declaration with an + initializer. `var x = 5` and `x := 5` are the same binding spelled two + ways, but only the latter is a `short_var_declaration`; the former is a + `var_spec`, which no arm matched. An initialized `var_spec` — typed or + not, standalone or inside a grouped `var ( … )` block — is now one + assignment, matching what Rust, Java, C, C++, C#, JS, Lua and PHP + already count for the same construct (each measured). `const` stays + excluded, as does an uninitialized `var z int`. Go `abc` values rise + by one per initialized `var` spec (#1278). - Constructor delegation scored zero ABC branches in Java, C# and Kotlin. `super(…)` / `this(…)` parses as `explicit_constructor_invocation` in Java, `: base(…)` / `: this(…)` as `constructor_initializer` in C#, diff --git a/src/metrics/abc.rs b/src/metrics/abc.rs index 86463dbbd..d21e08c97 100644 --- a/src/metrics/abc.rs +++ b/src/metrics/abc.rs @@ -6275,16 +6275,18 @@ function f(int $a, int $b): int { #[test] fn go_assignments_count_plain_compound_short_var_and_incdec() { // `x := 0` (short var decl), `x = 5` and `x = 7` (plain `=`), - // `x += 2` (compound), `x++` (inc) → A = 5. `var y = 1` is a - // declaration — its `=` is not counted (matches the Rust/Java - // rule for `let` / `int y = 1`). + // `x += 2` (compound), `x++` (inc), and the initialized + // declaration `var y = 1` — which is counted, matching the Rust + // and Java rules for `let y = 1` / `int y = 1` (both measured at + // one assignment each). The comment here previously claimed the + // opposite and pinned Go at 6 (#1278). check_metrics::( "package main\nfunc f() { var y = 1; _ = y; x := 0; x = 5; x += 2; x = 7; x++ }\n", "foo.go", |metric| { // `_ = y` is itself an assignment_statement → +1. - // x:= + x=5 + x+=2 + x=7 + x++ + _=y → 6 - assert_eq!(metric.abc.assignments_sum(), 6); + // var y=1 + _=y + x:= + x=5 + x+=2 + x=7 + x++ → 7 + assert_eq!(metric.abc.assignments_sum(), 7); assert_eq!(metric.abc.branches_sum(), 0); assert_eq!(metric.abc.conditions_sum(), 0); insta::assert_json_snapshot!(metric.abc); @@ -6292,6 +6294,44 @@ function f(int $a, int $b): int { ); } + #[test] + fn go_var_declarations_count_only_when_initialized() { + // Regression for #1278: a `var` declaration with an initializer is + // a `var_spec` carrying a `value` field, not an + // `assignment_statement` or `short_var_declaration`, so it scored + // zero — `var x = 5` and `x := 5` are the same binding spelled two + // ways. Both typed and untyped initializers count; an + // uninitialized `var z int` and a `const` do not. + // expected: 3 assignments — `var x = 5`, `var y int = 6`, `z := 7`; + // `var w int` and `const c = 1` contribute nothing. + check_metrics::( + "package main\nfunc f() int {\n\tvar x = 5\n\tvar y int = 6\n\tvar w int\n\tconst c = 1\n\tz := 7\n\treturn x + y + w + c + z\n}\n", + "foo.go", + |metric| { + assert_eq!(metric.abc.assignments_sum(), 3); + assert_eq!(metric.abc.branches_sum(), 0); + }, + ); + } + + #[test] + fn go_grouped_var_block_counts_each_initialized_spec() { + // A grouped `var ( … )` block is one `var_declaration` holding one + // `var_spec` per line, so matching the spec counts each initialized + // line on its own. A multi-name spec is still one binding + // statement, matching `p, q := 1, 2` (#1278). + // expected: 3 assignments — `a = 1`, `p, q = 1, 2`, and `r := 0`; + // the uninitialized `b int` contributes nothing. + check_metrics::( + "package main\nfunc f() {\n\tvar (\n\t\ta = 1\n\t\tb int\n\t)\n\tvar p, q = 1, 2\n\tr := 0\n\t_ = a + b + p + q + r\n}\n", + "foo.go", + |metric| { + // The trailing `_ = …` is itself an assignment_statement. + assert_eq!(metric.abc.assignments_sum(), 4); + }, + ); + } + #[test] fn go_calls_are_branches() { // Three calls: free function `g()`, method call `r.Inc()`, and @@ -6403,10 +6443,10 @@ function f(int $a, int $b): int { #[test] fn go_complex_function_abc() { // Mixed shape, verified by hand: - // - Assignments: `_ = x` (after `var`), `n := 0`, - // `n = n + 1`, `n += 2`, `n++`, `_ = len(s)` → A = 6. - // `var x = 10` is a declaration, not counted. Every `_ = ...` - // IS counted as an assignment_statement. + // - Assignments: `var x = 10` (an initialized declaration, #1278), + // `_ = x`, `n := 0`, `n = n + 1`, `n += 2`, `n++`, + // `_ = len(s)` → A = 7. Every `_ = ...` IS counted as an + // assignment_statement. // - Branches: `len(s)` → B = 1. // - Conditions: `n < 10` → 1, `else` → 1, switch arms `case 0:` // and `case 1:` (default excluded) → 2 → total C = 4. @@ -6426,7 +6466,7 @@ function f(int $a, int $b): int { }\n", "foo.go", |metric| { - assert_eq!(metric.abc.assignments_sum(), 6); + assert_eq!(metric.abc.assignments_sum(), 7); assert_eq!(metric.abc.branches_sum(), 1); assert_eq!(metric.abc.conditions_sum(), 4); insta::assert_json_snapshot!(metric.abc); diff --git a/src/metrics/abc/go.rs b/src/metrics/abc/go.rs index 8d48e6376..c52605474 100644 --- a/src/metrics/abc/go.rs +++ b/src/metrics/abc/go.rs @@ -109,6 +109,21 @@ impl Abc for GoCode { G::AssignmentStatement | G::ShortVarDeclaration | G::IncStatement | G::DecStatement => { stats.assignments += 1.; } + // `var x = 5` and `x := 5` are the same binding spelled two + // ways, and the cross-language policy counts an initialized + // declaration: Java the `EQ` in `int x = 5;`, C++ an + // `InitDeclarator` carrying `EQ`, Rust a `LetDeclaration` with + // a `value` field, JS a `let` / `var` initializer (#1278). The + // `value` field is what distinguishes `var z int` (no + // initializer, no assignment) position-independently across + // both `var x = 5` and `var y int = 6`. Matching the spec + // rather than the declaration counts each line of a grouped + // `var ( a = 1 \n b int )` block on its own; a multi-name + // `var p, q = 1, 2` is one spec and so scores 1, matching + // `p, q := 1, 2`. `const` stays excluded, as everywhere else. + G::VarSpec if node.child_by_field_name("value").is_some() => { + stats.assignments += 1.; + } // Every call expression — including method calls // (`r.Method()` parses as `call_expression` whose callee is // a `selector_expression`) — contributes one branch. diff --git a/src/metrics/snapshots/big_code_analysis__metrics__abc__tests__go_assignments_count_plain_compound_short_var_and_incdec.snap b/src/metrics/snapshots/big_code_analysis__metrics__abc__tests__go_assignments_count_plain_compound_short_var_and_incdec.snap index 33bcbe120..649231b09 100644 --- a/src/metrics/snapshots/big_code_analysis__metrics__abc__tests__go_assignments_count_plain_compound_short_var_and_incdec.snap +++ b/src/metrics/snapshots/big_code_analysis__metrics__abc__tests__go_assignments_count_plain_compound_short_var_and_incdec.snap @@ -3,16 +3,16 @@ source: src/metrics/abc.rs expression: metric.abc --- { - "assignments": 6, + "assignments": 7, "branches": 0, "conditions": 0, - "magnitude": 6.0, + "magnitude": 7.0, "value": 0.0, - "assignments_average": 3.0, + "assignments_average": 3.5, "branches_average": 0.0, "conditions_average": 0.0, "assignments_min": 0, - "assignments_max": 6, + "assignments_max": 7, "branches_min": 0, "branches_max": 0, "conditions_min": 0, diff --git a/src/metrics/snapshots/big_code_analysis__metrics__abc__tests__go_calls_are_branches.snap b/src/metrics/snapshots/big_code_analysis__metrics__abc__tests__go_calls_are_branches.snap index b75cf1223..40043530c 100644 --- a/src/metrics/snapshots/big_code_analysis__metrics__abc__tests__go_calls_are_branches.snap +++ b/src/metrics/snapshots/big_code_analysis__metrics__abc__tests__go_calls_are_branches.snap @@ -3,16 +3,16 @@ source: src/metrics/abc.rs expression: metric.abc --- { - "assignments": 1, + "assignments": 2, "branches": 3, "conditions": 0, - "magnitude": 3.1622776601683795, + "magnitude": 3.605551275463989, "value": 0.0, - "assignments_average": 0.25, + "assignments_average": 0.5, "branches_average": 0.75, "conditions_average": 0.0, "assignments_min": 0, - "assignments_max": 1, + "assignments_max": 2, "branches_min": 0, "branches_max": 3, "conditions_min": 0, diff --git a/src/metrics/snapshots/big_code_analysis__metrics__abc__tests__go_complex_function_abc.snap b/src/metrics/snapshots/big_code_analysis__metrics__abc__tests__go_complex_function_abc.snap index 536af5b2d..e793ed722 100644 --- a/src/metrics/snapshots/big_code_analysis__metrics__abc__tests__go_complex_function_abc.snap +++ b/src/metrics/snapshots/big_code_analysis__metrics__abc__tests__go_complex_function_abc.snap @@ -3,16 +3,16 @@ source: src/metrics/abc.rs expression: metric.abc --- { - "assignments": 6, + "assignments": 7, "branches": 1, "conditions": 4, - "magnitude": 7.280109889280518, + "magnitude": 8.12403840463596, "value": 0.0, - "assignments_average": 3.0, + "assignments_average": 3.5, "branches_average": 0.5, "conditions_average": 2.0, "assignments_min": 0, - "assignments_max": 6, + "assignments_max": 7, "branches_min": 0, "branches_max": 1, "conditions_min": 0, From dacf3ded949627916a40549c44d3c61878ddde2b Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 9 Aug 2026 12:16:42 -0700 Subject: [PATCH 04/16] fix(metrics/bash): count the arithmetic ternary as a decision Bash's only ternary form scored zero cyclomatic and zero cognitive while every sibling with a ternary counts one. Add it to both cyclomatic tiers and to the cognitive nesting arm, matching the C-family ConditionalExpression treatment. A drift-marker test pins the defensively-listed TernaryExpression2 alias as unreachable at the pinned grammar. Fixes #1268 --- CHANGELOG.md | 10 ++++ src/metrics/cognitive.rs | 59 +++++++++++++++++++++ src/metrics/cognitive/bash.rs | 12 ++++- src/metrics/cyclomatic.rs | 95 ++++++++++++++++++++++++++++++++++ src/metrics/cyclomatic/bash.rs | 12 ++++- 5 files changed, 186 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6d5e24699..9a3e54fc2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -96,6 +96,16 @@ for historical reference. ### Fixed +- A Bash arithmetic ternary — the only ternary form Bash has — + contributed nothing to cyclomatic or cognitive complexity, so + `local m=$(( a > b ? a : b ))` reported cyclomatic 1 (base only) and + cognitive 0, where the same construct scores 2 / 1 in every sibling + that has a ternary. It is now a decision point in both cyclomatic + tiers and a nesting construct for cognitive, matching the C-family + `ConditionalExpression`. Both arithmetic contexts are covered — the + `$(( … ))` expansion and the bare `(( … ))` statement. Bash + `cyclomatic`, `cognitive` and the `mi` family derived from them shift + for files using arithmetic ternaries (#1268). - Go ABC scored zero assignments for a `var` declaration with an initializer. `var x = 5` and `x := 5` are the same binding spelled two ways, but only the latter is a `short_var_declaration`; the former is a diff --git a/src/metrics/cognitive.rs b/src/metrics/cognitive.rs index 6b4f4ba75..b66c2565c 100644 --- a/src/metrics/cognitive.rs +++ b/src/metrics/cognitive.rs @@ -6080,6 +6080,65 @@ mod tests { ); } + #[test] + fn bash_arithmetic_ternary_increases_nesting() { + // Regression for #1268: Bash's only ternary form scored zero + // cognitive complexity. It nests like the C-family + // `ConditionalExpression`. Both arithmetic contexts are covered — + // the `$(( … ))` expansion and the bare `(( … ))` statement. + check_metrics::( + "f() { + local m=$(( a > b ? a : b )) # +1 ternary + } + g() { + (( x = a ? b : c )) # +1 ternary + }", + "foo.sh", + |metric| { + // One increment per function, at nesting 0 in each. + insta::assert_json_snapshot!( + metric.cognitive, + @r#" + { + "sum": 2, + "value": 0, + "average": 1.0, + "min": 0, + "max": 1 + } + "# + ); + }, + ); + } + + #[test] + fn bash_nested_arithmetic_ternary_charges_the_inner_one_twice() { + // A ternary inside a ternary is +1 for the outer and +2 for the + // inner (one increment plus one nesting level), the same charge + // every nesting construct carries (#1268). + check_metrics::( + "h() { + local n=$(( a ? b : c ? d : e )) # +1 outer, +2 inner + }", + "foo.sh", + |metric| { + insta::assert_json_snapshot!( + metric.cognitive, + @r#" + { + "sum": 3, + "value": 0, + "average": 3.0, + "min": 0, + "max": 3 + } + "# + ); + }, + ); + } + #[test] fn bash_boolean_sequence() { // First if: a chain of `&&` is one boolean increment regardless of diff --git a/src/metrics/cognitive/bash.rs b/src/metrics/cognitive/bash.rs index de728eefc..82a05d431 100644 --- a/src/metrics/cognitive/bash.rs +++ b/src/metrics/cognitive/bash.rs @@ -30,7 +30,17 @@ impl Cognitive for BashCode { // covers both `for` and `select`. `CStyleForStatement` is the // `for ((…))` arithmetic form. `ElifClause` is a dedicated node, // not a nested `if`, so no `is_else_if` check is needed. - IfStatement | WhileStatement | ForStatement | CStyleForStatement | CaseStatement => { + // `TernaryExpression` is the arithmetic ternary inside + // `(( … ))` / `$(( … ))`, Bash's only ternary form. It nests + // like the C-family `ConditionalExpression`, so a ternary + // inside a ternary charges the inner one at +2 (#1268). The + // pinned grammar emits only kind 223; the + // `TernaryExpression2` alias is listed defensively per + // grammar-dispatch §1 and pinned unreachable by + // `bash_ternary_expression_alias_is_unreachable` in the + // cyclomatic test module. + IfStatement | WhileStatement | ForStatement | CStyleForStatement | CaseStatement + | TernaryExpression | TernaryExpression2 => { increase_nesting(stats, &mut nesting); } ElifClause | ElseClause => { diff --git a/src/metrics/cyclomatic.rs b/src/metrics/cyclomatic.rs index 4bdd72411..5598c5f80 100644 --- a/src/metrics/cyclomatic.rs +++ b/src/metrics/cyclomatic.rs @@ -4649,6 +4649,101 @@ f() { ); } + #[test] + fn bash_arithmetic_ternary_is_a_decision() { + // Regression for #1268: the arithmetic ternary is the only ternary + // form Bash has, and it contributed nothing to either cyclomatic + // tier while every sibling with a ternary counts one. Both + // arithmetic contexts are covered — the `$(( … ))` expansion and + // the bare `(( … ))` statement — since the grammar admits the + // construct in both and only a fixture in each shows both reach + // the arm. + check_metrics::( + "#!/bin/bash +f() { + local m=$(( a > b ? a : b )) +} +g() { + (( x = a ? b : c )) +}", + "foo.sh", + |metric| { + // unit(1) + f(base 1 + ternary 1) + g(base 1 + ternary 1) + // = sum 5, max 2. Both tiers move: a ternary is a decision + // point in the modified count too. + assert_eq!(metric.cyclomatic.cyclomatic_sum(), 5); + assert_eq!(metric.cyclomatic.cyclomatic_max(), 2); + assert_eq!(metric.cyclomatic.cyclomatic_modified_sum(), 5); + }, + ); + } + + #[test] + fn bash_ternary_expression_alias_is_unreachable() { + // Drift marker for the defensive `TernaryExpression2` arm (lesson + // 34 / grammar-dispatch §1). The Bash enum carries two kinds + // mapping to `"ternary_expression"`, but at the pinned grammar + // only 223 is ever emitted — verified across every arithmetic + // context the grammar admits: `$(( … ))` expansion, bare + // `(( … ))` statement, a c-style `for ((…))` header, `let`, an + // array subscript, an `if (( … ))` condition, and a `declare -i` + // initializer. Both cyclomatic and cognitive list the alias + // anyway; if a grammar bump starts emitting it, this assertion + // fails loudly rather than the metric silently doubling. + let src = "for ((i = a ? b : c; i < 10; i++)); do :; done\n\ + let \"y = a ? b : c\"\n\ + arr[$(( a ? b : c ))]=1\n\ + if (( a ? b : c )); then :; fi\n\ + declare -i z=$(( a ? b : c ))\n\ + w=$(( a ? b : c ))\n\ + (( v = a ? b : c ))\n"; + let parser = BashParser::new( + src.as_bytes().to_vec(), + &std::path::PathBuf::from("foo.sh"), + None, + ); + assert!(ast_has_kind_id(&parser, Bash::TernaryExpression as u16)); + assert!(!ast_has_kind_id(&parser, Bash::TernaryExpression2 as u16)); + } + + // Walk the AST and return true iff any node has `kind_id == target`. + // Used as a drift marker for an alias kind id: a passing + // `!ast_has_kind_id(...)` assertion proves the kind is unreachable at + // the pinned grammar version, so a future grammar bump that starts + // emitting it fails loudly instead of silently changing the metric + // (lesson 34). + fn ast_has_kind_id(parser: &P, target: u16) -> bool { + let mut stack = vec![parser.root()]; + while let Some(node) = stack.pop() { + if node.kind_id() == target { + return true; + } + for i in (0..node.child_count()).rev() { + if let Some(c) = node.child(i) { + stack.push(c); + } + } + } + false + } + + #[test] + fn bash_nested_arithmetic_ternary_counts_each_occurrence() { + // Each ternary is its own decision point (#1268). + check_metrics::( + "#!/bin/bash +h() { + local n=$(( a ? b : c ? d : e )) +}", + "foo.sh", + |metric| { + // unit(1) + h(base 1 + two ternaries) = sum 4, max 3. + assert_eq!(metric.cyclomatic.cyclomatic_sum(), 4); + assert_eq!(metric.cyclomatic.cyclomatic_max(), 3); + }, + ); + } + #[test] fn bash_simple_function() { check_metrics::( diff --git a/src/metrics/cyclomatic/bash.rs b/src/metrics/cyclomatic/bash.rs index 031155d0c..ebfeda70f 100644 --- a/src/metrics/cyclomatic/bash.rs +++ b/src/metrics/cyclomatic/bash.rs @@ -30,12 +30,22 @@ impl Cyclomatic for BashCode { Bash::CaseStatement => { stats.cyclomatic_modified += 1.; } - // Both standard and modified. + // Both standard and modified. `TernaryExpression` is the + // arithmetic ternary inside `(( … ))` / `$(( … ))` — the only + // ternary form Bash has, and a decision point in every sibling + // that has one (#1268). The pinned grammar emits only kind 223 + // in every arithmetic context; the `TernaryExpression2` alias + // is listed defensively per grammar-dispatch §1, following the + // ObjC `BinaryExpression | BinaryExpression2` precedent, and + // pinned unreachable by + // `bash_ternary_expression_alias_is_unreachable`. Bash::IfStatement | Bash::ElifClause | Bash::ForStatement | Bash::CStyleForStatement | Bash::WhileStatement + | Bash::TernaryExpression + | Bash::TernaryExpression2 | Bash::AMPAMP | Bash::PIPEPIPE => { stats.cyclomatic += 1.; From c5f4d7fcc5fa427730ee07d373c4ed712ef98381 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 9 Aug 2026 12:20:36 -0700 Subject: [PATCH 05/16] fix(vcs): single-source the saturating window boundary build_cached and BlameWalk::new computed their window boundaries with a plain subtraction while four sibling sites saturated, each carrying its own copy of the hazard comment. Both operands are attacker- adjacent, so the plain form panics in a debug build and wraps to a future-dated boundary in release. Route all six through one window_boundary helper that documents the invariant once and is directly unit-testable at the i64 extremes. days_between carried the same hazard under a .max(0) clamp a wrapped delta defeats. Fixes #1271 --- CHANGELOG.md | 14 ++++++++++++++ src/vcs/git/blame.rs | 6 +++--- src/vcs/git/cached.rs | 4 ++-- src/vcs/git/history.rs | 8 ++------ src/vcs/git/jit.rs | 9 +++------ src/vcs/options.rs | 20 ++++++++++++++++++++ src/vcs/options_tests.rs | 27 +++++++++++++++++++++++++++ src/vcs/replay.rs | 9 +++------ src/vcs/stats.rs | 11 +++++++---- src/vcs/stats_tests.rs | 15 +++++++++++++++ 10 files changed, 96 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a3e54fc2..68a320049 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -96,6 +96,20 @@ for historical reference. ### Fixed +- The VCS window-boundary computation was non-saturating in two of its + six sites — `build_cached`, the *default* path for `bca vcs`, and + `BlameWalk`'s constructor. Both operands are attacker-adjacent: the + reference time is an `--as-of` value or a committer timestamp read out + of an object header, and the window length is user-supplied up to + roughly `i64::MAX` through `--long-window`. A plain `-` therefore + panicked in a debug build and, in release, wrapped to a boundary in + the far *future*, silently reversing the pure-hit check, the candidate + filter, `prune_and_dedup`, and the `walk_long_boundary` persisted for + future runs. All six sites now route through one + `vcs::options::window_boundary` helper that documents and pins the + saturation invariant once. `days_between` carried the same hazard + under a `.max(0)` clamp that a wrapped (positive) delta defeats, and + saturates too (#1271). - A Bash arithmetic ternary — the only ternary form Bash has — contributed nothing to cyclomatic or cognitive complexity, so `local m=$(( a > b ? a : b ))` reported cyclomatic 1 (base only) and diff --git a/src/vcs/git/blame.rs b/src/vcs/git/blame.rs index cba4b4e84..557c3c579 100644 --- a/src/vcs/git/blame.rs +++ b/src/vcs/git/blame.rs @@ -86,7 +86,7 @@ use super::repo; use crate::vcs::classify::{self, Classification}; use crate::vcs::error::Error; use crate::vcs::identity::{AuthorId, BotFilter}; -use crate::vcs::options::Options; +use crate::vcs::options::{Options, window_boundary}; use crate::vcs::stats::{Accumulator, ChangeRecord, Stats}; /// A function's 1-based, inclusive line span at the target ref, as taken @@ -291,8 +291,8 @@ impl PerFunctionBlame { .transpose()?; let now = options.as_of.unwrap_or_else(super::current_unix_seconds); - let long_boundary = now - options.long_window_secs; - let recent_boundary = now - options.recent_window_secs; + let long_boundary = window_boundary(now, options.long_window_secs); + let recent_boundary = window_boundary(now, options.recent_window_secs); Ok(Self { repo: repo.into_sync(), diff --git a/src/vcs/git/cached.rs b/src/vcs/git/cached.rs index 7bd1ab059..4afbfb424 100644 --- a/src/vcs/git/cached.rs +++ b/src/vcs/git/cached.rs @@ -24,7 +24,7 @@ use crate::diag::warn; use crate::vcs::HistoryIndex; use crate::vcs::cache::{self, CacheConfig, CommitEvent, HistoryCache}; use crate::vcs::error::Error; -use crate::vcs::options::Options; +use crate::vcs::options::{Options, window_boundary}; use crate::vcs::replay; use crate::vcs::score::RISK_SCORE_VERSION; use crate::vcs::stats::VCS_SCHEMA_VERSION; @@ -102,7 +102,7 @@ pub(crate) fn build_cached( }; let fingerprint = cache::fingerprint(options); - let long_boundary = now - options.long_window_secs; + let long_boundary = window_boundary(now, options.long_window_secs); // Pure hit: an exact, compatible entry that reaches back far enough. let exact_path = cache::entry_path(&repo_dir, &head_sha); diff --git a/src/vcs/git/history.rs b/src/vcs/git/history.rs index 2acae28ff..df007d728 100644 --- a/src/vcs/git/history.rs +++ b/src/vcs/git/history.rs @@ -26,7 +26,7 @@ use crate::vcs::cache::CommitEvent; use crate::vcs::classify::{self, Classification}; use crate::vcs::error::Error; use crate::vcs::identity::{AuthorId, BotFilter}; -use crate::vcs::options::Options; +use crate::vcs::options::{Options, window_boundary}; /// Rename edges (`source → destination`) a commit introduced. type RenameEdges = Vec<(PathBuf, PathBuf)>; @@ -53,11 +53,7 @@ pub(crate) fn collect_events( now: i64, stop_oids: &HashSet, ) -> Result<(Vec, Option), Error> { - // Saturating: a garbage extreme `--as-of` (e.g. i64::MIN) must not - // overflow the cutoff subtraction. Saturating to i64::MIN yields an - // unbounded-past boundary — every commit timestamp is included, the - // safe total behavior for a degenerate clock. - let long_boundary = now.saturating_sub(options.long_window_secs); + let long_boundary = window_boundary(now, options.long_window_secs); let mailmap = repo.open_mailmap(); let bots = if options.exclude_bots { diff --git a/src/vcs/git/jit.rs b/src/vcs/git/jit.rs index 47e202063..b88d9a708 100644 --- a/src/vcs/git/jit.rs +++ b/src/vcs/git/jit.rs @@ -30,7 +30,7 @@ use crate::vcs::jit::{ JIT_SCHEMA_VERSION, JIT_SCORE_VERSION, JitCommit, JitDiffusion, JitExperience, JitFeatures, JitHistory, JitPurpose, JitReport, JitSize, JitSource, score, }; -use crate::vcs::options::{Options, RiskFormula}; +use crate::vcs::options::{Options, RiskFormula, window_boundary}; /// One text file the scored commit touched. Shared with the /// [`diff_parse`](super::diff_parse) sibling, which builds the same shape @@ -483,11 +483,8 @@ fn experience_features( options: &Options, reference_time: i64, ) -> Result { - // Saturating window cutoffs: an extreme `reference_time` must not - // overflow the subtraction; saturating to i64::MIN yields an - // unbounded-past boundary (include all history). See history.rs. - let long_boundary = reference_time.saturating_sub(options.long_window_secs); - let recent_boundary = reference_time.saturating_sub(options.recent_window_secs); + let long_boundary = window_boundary(reference_time, options.long_window_secs); + let recent_boundary = window_boundary(reference_time, options.recent_window_secs); // Same participant identity as the file-level priors: author + co-authors, // bot-filtered when `--exclude-bots`. diff --git a/src/vcs/options.rs b/src/vcs/options.rs index 5af6be10e..a7a0cfbba 100644 --- a/src/vcs/options.rs +++ b/src/vcs/options.rs @@ -295,6 +295,26 @@ impl Options { } } +/// The oldest timestamp a window of `window_secs` reaches back to from +/// `reference`. +/// +/// Saturating, and that is the whole point of the function existing. Both +/// operands are attacker-adjacent: `reference` is an `--as-of` value or a +/// committer timestamp read straight out of an object header, and +/// `window_secs` is user-supplied up to roughly `i64::MAX` through +/// [`parse_window`]. A plain `-` therefore panics in a debug build and, in +/// release, wraps to a boundary in the far *future* — which silently +/// reverses the meaning of every comparison downstream. Saturating to +/// `i64::MIN` instead yields an unbounded-past boundary: every commit is +/// inside the window, the safe total behaviour for a degenerate clock. +/// +/// Every window boundary in the subsystem goes through here so the walk, +/// the replay, and the cache all agree on the same value at the extremes +/// (#1271). +pub(crate) fn window_boundary(reference: i64, window_secs: i64) -> i64 { + reference.saturating_sub(window_secs) +} + /// Round a second count to the nearest whole day, saturating into /// `u32`. Window lengths never approach `u32::MAX` days in practice, /// but the saturation keeps the conversion total and lint-clean. diff --git a/src/vcs/options_tests.rs b/src/vcs/options_tests.rs index 823252696..4566832dc 100644 --- a/src/vcs/options_tests.rs +++ b/src/vcs/options_tests.rs @@ -310,3 +310,30 @@ fn custom_scope_matches_only_listed_extensions_case_insensitively() { // An extension-less file never matches a custom list. assert!(!scope.includes(Path::new("Makefile"))); } + +#[test] +fn window_boundary_saturates_at_both_extremes() { + // The ordinary case: the boundary is `window_secs` before the + // reference, exactly. + assert_eq!(window_boundary(1_000_000, 90 * DAY), 1_000_000 - 90 * DAY); + + // A degenerate clock. `--as-of` accepts any i64 and a committer + // timestamp is whatever an object header says, while `parse_window` + // admits a window up to roughly i64::MAX ("292471208677y" passes its + // checked multiply). A plain `-` panics here in a debug build and, in + // release, wraps to a boundary in the far *future* — reversing every + // comparison downstream. Saturating pins it at i64::MIN, an + // unbounded-past boundary that includes all history (#1271). + assert_eq!(window_boundary(i64::MIN, 1), i64::MIN); + assert_eq!(window_boundary(i64::MIN, i64::MAX), i64::MIN); + assert_eq!(window_boundary(-1, i64::MAX), i64::MIN); + + // The mirror extreme: subtracting a negative window from a large + // positive reference saturates upward rather than wrapping negative, + // which would otherwise read as "unbounded past" for a nonsense input. + assert_eq!(window_boundary(i64::MAX, -1), i64::MAX); + assert_eq!(window_boundary(i64::MAX, i64::MIN), i64::MAX); + + // A zero window reaches back nowhere. + assert_eq!(window_boundary(0, 0), 0); +} diff --git a/src/vcs/replay.rs b/src/vcs/replay.rs index d1fa73084..73786167e 100644 --- a/src/vcs/replay.rs +++ b/src/vcs/replay.rs @@ -19,7 +19,7 @@ use super::cache::CommitEvent; use super::classify::Classification; use super::entropy::{self, CochangeGraph}; use super::identity::AuthorId; -use super::options::{Options, RiskFormula}; +use super::options::{Options, RiskFormula, window_boundary}; use super::score; use super::stats::{Accumulator, ChangeRecord, Stats}; @@ -172,11 +172,8 @@ pub(crate) fn replay( .into_iter() .map(|(path, sloc)| (path, Accumulator::new(sloc))) .collect(); - // Saturating window cutoffs: a garbage extreme `now` (i64::MIN) must - // not overflow; saturating to i64::MIN means "no lower bound", i.e. - // include all history — the safe total behavior. See history.rs. - let long_boundary = now.saturating_sub(options.long_window_secs); - let recent_boundary = now.saturating_sub(options.recent_window_secs); + let long_boundary = window_boundary(now, options.long_window_secs); + let recent_boundary = window_boundary(now, options.recent_window_secs); let mut graph = CochangeGraph::new(); // Rebuilt newest-first across the whole log, exactly as the live walk // builds it — so a rename in a newer (possibly incremental) commit diff --git a/src/vcs/stats.rs b/src/vcs/stats.rs index 198958087..5a5e56d94 100644 --- a/src/vcs/stats.rs +++ b/src/vcs/stats.rs @@ -335,11 +335,14 @@ impl Accumulator { } } -/// Whole days between an earlier timestamp and `now`, clamped at zero so -/// a future-dated commit (clock skew) reads as "today" rather than a -/// negative age. fn days_between(earlier: i64, now: i64) -> u32 { - let delta = (now - earlier).max(0); + // Saturating for the same reason the window boundaries are + // (`options::window_boundary`, #1271): `earlier` is a committer + // timestamp read out of an object header, so a crafted extreme makes + // the plain subtraction overflow — a panic in a debug build, and in + // release a wrapped delta that the `.max(0)` cannot repair because it + // wraps to a *positive* value. Saturating keeps the clamp meaningful. + let delta = now.saturating_sub(earlier).max(0); u32::try_from(delta / SECONDS_PER_DAY).unwrap_or(u32::MAX) } diff --git a/src/vcs/stats_tests.rs b/src/vcs/stats_tests.rs index d760bc53e..d6a9d1af9 100644 --- a/src/vcs/stats_tests.rs +++ b/src/vcs/stats_tests.rs @@ -319,3 +319,18 @@ fn total_edits_sum_saturates_across_authors() { assert!(stats.ownership_top_share.is_finite()); assert_eq!(stats.authors_long, 2); } + +#[test] +fn days_between_saturates_on_an_extreme_timestamp() { + // `earlier` is a committer timestamp straight out of an object header, + // so the subtraction must be total. The plain `-` panics in a debug + // build on these inputs and, in release, wraps to a *positive* delta + // that the `.max(0)` clamp cannot repair (#1271). + assert_eq!(days_between(i64::MIN, i64::MAX), u32::MAX); + assert_eq!(days_between(i64::MIN, 0), u32::MAX); + // The mirror direction: a future-dated commit still clamps to today. + assert_eq!(days_between(i64::MAX, i64::MIN), 0); + // The ordinary case is unchanged. + assert_eq!(days_between(0, 3 * 86_400), 3); + assert_eq!(days_between(1_000, 1_000), 0); +} From d31161c07e6d946e621da2f240e99a300971489b Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 9 Aug 2026 12:25:23 -0700 Subject: [PATCH 06/16] fix(vcs/identity): match the bot pattern case-insensitively DEFAULT_BOT_PATTERN's doc has promised case-insensitive matching since the option shipped, but BotFilter::new compiled a plain Regex and push_if_human matches raw signature bytes, so a capitalised bot identity was counted as a human author. Compile with RegexBuilder::case_insensitive, leaving (?-i) as the opt-out. Bump CACHE_SCHEMA_VERSION: the fingerprint hashes only the pattern string, so a cached event log would otherwise replay under the old semantics. Fixes #1265 --- CHANGELOG.md | 13 ++++++++ big-code-analysis-book/src/commands/vcs.md | 6 +++- big-code-analysis-cli/src/cli_args/vcs.rs | 4 ++- man/bca-vcs.1 | 2 +- src/vcs/cache.rs | 14 ++++++-- src/vcs/git/identity_tests.rs | 39 ++++++++++++++++++++++ src/vcs/identity.rs | 15 +++++++-- src/vcs/identity_tests.rs | 32 ++++++++++++++++++ src/vcs/options_tests.rs | 6 +++- 9 files changed, 122 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 68a320049..ca30ba870 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -96,6 +96,19 @@ for historical reference. ### Fixed +- The VCS bot-author filter was case-sensitive despite + `DEFAULT_BOT_PATTERN`'s doc promising the opposite since the option + shipped. `BotFilter::new` compiled with a plain `Regex`, and + `push_if_human` matches the raw signature bytes before any + lowercasing, so `Dependabot[bot]` was counted as a human author while + its lowercase twin was excluded — diluting ownership and raising file + risk on the same repository depending only on how a bot capitalised + itself. A user pattern behaves as written now too: + `--bot-pattern renovate` matches `Renovate Bot`. Prefix `(?-i)` to + restore exact matching. `CACHE_SCHEMA_VERSION` moves 1 → 2, since the + cache fingerprint hashes only the pattern string and an event log + walked under the old matcher would otherwise replay with its excluded + set intact; the bump costs one cold walk (#1265). - The VCS window-boundary computation was non-saturating in two of its six sites — `build_cached`, the *default* path for `bca vcs`, and `BlameWalk`'s constructor. Both operands are attacker-adjacent: the diff --git a/big-code-analysis-book/src/commands/vcs.md b/big-code-analysis-book/src/commands/vcs.md index bf089f2c0..6566793cc 100644 --- a/big-code-analysis-book/src/commands/vcs.md +++ b/big-code-analysis-book/src/commands/vcs.md @@ -108,7 +108,11 @@ standalone page, pass [`bca report --vcs`](report.md), which appends a Author identities are canonicalised through the repository `.mailmap` and counted by lowercased email; `Co-authored-by:` trailers add participants. Bot identities (`dependabot[bot]`, `renovate[bot]`, -`github-actions[bot]`, …) are excluded by default. Binary files and +`github-actions[bot]`, …) are excluded by default. The pattern — +the built-in one or your own `--bot-pattern` — is matched +case-insensitively as a substring against both the author name and the +email, so `--bot-pattern renovate` catches `Renovate Bot`; prefix +`(?-i)` to match case exactly. Binary files and symlinks are skipped; an untracked file has no record at all (distinct from a tracked file with zero in-window activity). diff --git a/big-code-analysis-cli/src/cli_args/vcs.rs b/big-code-analysis-cli/src/cli_args/vcs.rs index 572a1aced..31dcb709c 100644 --- a/big-code-analysis-cli/src/cli_args/vcs.rs +++ b/big-code-analysis-cli/src/cli_args/vcs.rs @@ -129,7 +129,9 @@ pub(crate) struct VcsArgs { /// Do not exclude bot author identities. #[clap(long, global = true)] pub(crate) no_exclude_bots: bool, - /// Override the bot-author exclusion regex. + /// Override the bot-author exclusion regex. Matched + /// case-insensitively against both the author name and the email; + /// prefix `(?-i)` to match case exactly. #[clap(long, global = true)] pub(crate) bot_pattern: Option, /// Reference "now" for reproducible runs (RFC 3339, `@unix`, or any diff --git a/man/bca-vcs.1 b/man/bca-vcs.1 index 09df9b142..cdd6cd6b2 100644 --- a/man/bca-vcs.1 +++ b/man/bca-vcs.1 @@ -68,7 +68,7 @@ Do not follow file renames across history Do not exclude bot author identities .TP \fB\-\-bot\-pattern\fR \fI\fR -Override the bot\-author exclusion regex +Override the bot\-author exclusion regex. Matched case\-insensitively against both the author name and the email; prefix `(?\-i)` to match case exactly .TP \fB\-\-as\-of\fR \fI\fR Reference "now" for reproducible runs (RFC 3339, `@unix`, or any git date spelling). Defaults to wall\-clock time diff --git a/src/vcs/cache.rs b/src/vcs/cache.rs index 85ff76683..ca199330d 100644 --- a/src/vcs/cache.rs +++ b/src/vcs/cache.rs @@ -70,9 +70,17 @@ use super::score::RISK_SCORE_VERSION; use super::stats::VCS_SCHEMA_VERSION; /// On-disk format version for the cache. Bump on any change to the -/// `HistoryCache` / `CommitEvent` shape; an older entry is then -/// ignored rather than mis-parsed. -pub const CACHE_SCHEMA_VERSION: u32 = 1; +/// `HistoryCache` / `CommitEvent` shape — and on any change to what a +/// recorded event *means*, since an older entry is otherwise replayed +/// under semantics it was not walked with. +/// +/// Version 2 (#1265): [`BotFilter`](super::identity::BotFilter) became +/// case-insensitive. The fingerprint hashes only the pattern *string*, +/// so an event log walked under the case-sensitive matcher would +/// otherwise replay with its excluded set intact — the same input +/// answering differently depending on whether the cache was warm. The +/// bump forces one benign cold walk. +pub const CACHE_SCHEMA_VERSION: u32 = 2; /// Front-end control over the persistent cache for one build. /// diff --git a/src/vcs/git/identity_tests.rs b/src/vcs/git/identity_tests.rs index 85e7dba5c..76161864b 100644 --- a/src/vcs/git/identity_tests.rs +++ b/src/vcs/git/identity_tests.rs @@ -135,6 +135,45 @@ mod push_if_human { ); } + /// Regression for #1265. `push_if_human` calls `is_bot` on the **raw** + /// signature bytes, before the lowercasing `AuthorId::new` applies + /// later — so the filter's own case sensitivity is what decides, and + /// it was case-sensitive against a doc contract promising otherwise. + /// Two commits by the same bot differing only in capitalisation are + /// both excluded now; before the fix the capitalised one survived as + /// a phantom human author. + #[test] + fn bot_authors_are_excluded_regardless_of_case() { + let snapshot: &'static gix::mailmap::Snapshot = + Box::leak(Box::new(gix::mailmap::Snapshot::default())); + // Leaked for the same reason the snapshot is: `ParticipantResolver` + // borrows both for `'a`, and the process exits after the test. + let bots: &'static crate::vcs::identity::BotFilter = Box::leak(Box::new( + crate::vcs::identity::BotFilter::new(crate::vcs::options::DEFAULT_BOT_PATTERN) + .expect("default pattern compiles"), + )); + let resolver = ParticipantResolver::new(snapshot, Some(bots)); + let mut out = Vec::new(); + resolver.push_if_human( + &mut out, + b"dependabot[bot]", + b"dependabot[bot]@users.noreply.github.com", + ); + resolver.push_if_human( + &mut out, + b"Dependabot[bot]", + b"Dependabot[bot]@users.noreply.github.com", + ); + assert!( + out.is_empty(), + "neither casing of a bot identity may anchor a participant: {out:?}" + ); + // The filter still admits humans, so the emptiness above is the + // bot rule firing rather than the resolver dropping everything. + resolver.push_if_human(&mut out, b"Ada Lovelace", b"ada@example.com"); + assert_eq!(out.len(), 1, "a human author is still kept: {out:?}"); + } + #[test] fn named_and_name_only_authors_are_kept() { let resolver = resolver(); diff --git a/src/vcs/identity.rs b/src/vcs/identity.rs index f5dd73dde..267b5f524 100644 --- a/src/vcs/identity.rs +++ b/src/vcs/identity.rs @@ -28,7 +28,7 @@ use hmac::{Hmac, KeyInit, Mac}; use sha2::{Digest, Sha256}; -use regex::Regex; +use regex::{Regex, RegexBuilder}; use super::error::Error; @@ -252,12 +252,23 @@ pub struct BotFilter { impl BotFilter { /// Compile a bot-exclusion pattern. /// + /// Matching is **case-insensitive**, which is what + /// [`DEFAULT_BOT_PATTERN`](crate::vcs::DEFAULT_BOT_PATTERN) has always + /// documented and what the substring-style patterns users write + /// expect: `--bot-pattern renovate` matches `Renovate Bot`. The flag + /// is only the *default* for the pattern, so an expert can restore + /// case sensitivity for all or part of it with an inline `(?-i)` — + /// `(?-i)Foo` does not match `foo`. + /// /// # Errors /// /// Returns [`Error::InvalidBotPattern`] when `pattern` is not a /// valid regular expression. pub fn new(pattern: &str) -> Result { - let pattern = Regex::new(pattern).map_err(|e| Error::InvalidBotPattern(e.to_string()))?; + let pattern = RegexBuilder::new(pattern) + .case_insensitive(true) + .build() + .map_err(|e| Error::InvalidBotPattern(e.to_string()))?; Ok(Self { pattern }) } diff --git a/src/vcs/identity_tests.rs b/src/vcs/identity_tests.rs index d11a1a30c..4ddddff25 100644 --- a/src/vcs/identity_tests.rs +++ b/src/vcs/identity_tests.rs @@ -180,6 +180,38 @@ fn default_bot_pattern_matches_known_bots() { assert!(!filter.is_bot(b"Ada Lovelace", b"ada@example.com")); } +#[test] +fn bot_pattern_matching_is_case_insensitive() { + // Regression for #1265: `DEFAULT_BOT_PATTERN`'s doc has promised + // case-insensitive matching since the option shipped, but the filter + // compiled with a plain `Regex`. A capitalised bot identity was + // counted as a human author — diluting ownership and raising file + // risk — while its lowercase twin was excluded. + let filter = BotFilter::new(DEFAULT_BOT_PATTERN).expect("default pattern compiles"); + assert!(filter.is_bot( + b"Dependabot[bot]", + b"Dependabot[bot]@users.noreply.github.com" + )); + assert!(filter.is_bot(b"RENOVATE[BOT]", b"")); + assert!(filter.is_bot(b"", b"GitHub-Actions[Bot]@users.noreply.github.com")); + // A user-supplied substring pattern behaves the way its author reads + // it — this is the case the doc contract exists for. + let custom = BotFilter::new("renovate").expect("custom pattern compiles"); + assert!(custom.is_bot(b"Renovate Bot", b"bot@renovateapp.com")); + // A human is still not a bot under either. + assert!(!filter.is_bot(b"Ada Lovelace", b"ada@example.com")); + assert!(!custom.is_bot(b"Ada Lovelace", b"ada@example.com")); +} + +#[test] +fn bot_pattern_can_opt_back_into_case_sensitivity() { + // `case_insensitive(true)` sets only the *default* flag, so an inline + // `(?-i)` restores exact matching for a user who needs it (#1265). + let sensitive = BotFilter::new("(?-i)renovate").expect("inline flag compiles"); + assert!(sensitive.is_bot(b"renovate[bot]", b"")); + assert!(!sensitive.is_bot(b"Renovate Bot", b"")); +} + #[test] fn invalid_bot_pattern_is_rejected() { assert!(matches!( diff --git a/src/vcs/options_tests.rs b/src/vcs/options_tests.rs index 4566832dc..07fa27e95 100644 --- a/src/vcs/options_tests.rs +++ b/src/vcs/options_tests.rs @@ -90,7 +90,11 @@ fn defaults_match_the_issue_sample() { #[test] fn default_bot_pattern_is_a_valid_regex() { // Guards the `expect` documented in `Options::default` / `BotFilter`. - assert!(regex::Regex::new(DEFAULT_BOT_PATTERN).is_ok()); + // Compiled through `BotFilter::new` rather than `Regex::new` directly, + // so it exercises the production constructor — including the + // case-insensitive flag it sets (#1265) — instead of re-verifying that + // the regex crate parses a constant. + assert!(super::super::identity::BotFilter::new(DEFAULT_BOT_PATTERN).is_ok()); } #[test] From 6ccc83a9fcc171012269958f90855a04f3e88792 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 9 Aug 2026 12:31:08 -0700 Subject: [PATCH 07/16] test(vcs/error): pin variant coverage with a compile-forced witness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two exhaustiveness-claiming tests were plain Vecs asking to be kept in step, and #956 showed they are not. #1245 fixed the client-input half by generating its list from production, but left the environment half hand-checked with nothing able to enumerate it — the file's own comment said so. Add a test-only wildcard-free variant_index match: adding a variant now fails to compile the file that must gain the new cases. Both lists route through a coverage bitmap over it, so an unlisted variant of either kind fails. VARIANT_COUNT is guarded in both directions. Fixes #1269 --- src/vcs/error_tests.rs | 106 ++++++++++++++++++++++++++++++++++------- 1 file changed, 88 insertions(+), 18 deletions(-) diff --git a/src/vcs/error_tests.rs b/src/vcs/error_tests.rs index afa3999a5..612caf567 100644 --- a/src/vcs/error_tests.rs +++ b/src/vcs/error_tests.rs @@ -1,6 +1,75 @@ use super::*; use std::path::PathBuf; +/// A dense index per [`Error`] variant, from a **wildcard-free** match. +/// +/// This is the structural half of the contract that `is_client_input`'s +/// own compile-forcing cannot reach. `Error` is `#[non_exhaustive]`, but +/// that only constrains *other* crates — a match here, inside the +/// defining crate, must still be exhaustive. So adding an eighteenth +/// variant fails to **compile this file**, which is the file that must +/// gain the new cases. Before #1269 the two lists below were plain +/// `Vec`s asking politely to be kept in step, and #956 showed they are +/// not: `InvalidAuthorHashKey` went unlisted in both until #1245. +/// +/// [`VARIANT_COUNT`] cannot go stale in either direction. Too small and +/// the bitmap indexing below panics out of bounds; too large and the +/// unreachable slot is never covered, failing the completeness +/// assertion. +fn variant_index(e: &Error) -> usize { + match e { + Error::NotARepository(_) => 0, + Error::OpenRepository(_) => 1, + Error::ResolveRef { .. } => 2, + Error::Walk(_) => 3, + Error::Diff(_) => 4, + Error::Mailmap(_) => 5, + Error::InvalidBotPattern(_) => 6, + Error::InvalidWindow(_) => 7, + Error::InvalidTimestamp(_) => 8, + Error::InvalidFormula(_) => 9, + Error::InvalidFileTypeScope(_) => 10, + Error::InvalidBusFactorThreshold(_) => 11, + Error::InvalidAuthorHashKey(_) => 12, + Error::InvalidTrend(_) => 13, + Error::Blame(_) => 14, + Error::InvalidDiff(_) => 15, + Error::Cache(_) => 16, + } +} + +/// Number of [`Error`] variants — see [`variant_index`], which guards it. +const VARIANT_COUNT: usize = 17; + +/// Assert `errors` names every variant exactly once. +/// +/// `what` is spliced into the failure so the message says which list is +/// short, and the panic names the missing slots by index rather than +/// only reporting a count mismatch. +fn assert_covers_every_variant<'a>(errors: impl IntoIterator, what: &str) { + let mut seen = [false; VARIANT_COUNT]; + for err in errors { + let i = variant_index(err); + assert!( + !seen[i], + "{what}: two entries name the same variant ({err:?}); each must \ + stand for exactly one, or a later variant hides behind it", + ); + seen[i] = true; + } + let missing: Vec = seen + .iter() + .enumerate() + .filter_map(|(i, hit)| (!hit).then_some(i)) + .collect(); + assert!( + missing.is_empty(), + "{what}: {} of {VARIANT_COUNT} variants covered; missing the arms of \ + `variant_index` at {missing:?}", + VARIANT_COUNT - missing.len(), + ); +} + // The Display strings are user-facing diagnostics surfaced by the CLI, // the `POST /vcs` error body, and the Python `ValueError`. Pin each // variant's wording so a refactor cannot silently degrade them. @@ -80,22 +149,12 @@ fn display_covers_every_variant() { "history cache error: disk full", ), ]; - // Completeness, for the half of the enum that can be enumerated. - // Comparing discriminants ignores the payloads, which differ between - // the samples and the cases below. The environment variants have no - // generated list and stay hand-checked — nothing can enumerate them, - // so a new one silently goes unpinned here. - let covered: Vec<_> = cases - .iter() - .map(|(err, _)| std::mem::discriminant(err)) - .collect(); - for sample in Error::client_input_samples() { - assert!( - covered.contains(&std::mem::discriminant(&sample)), - "client-input variant {sample:?} has no Display case; its wording \ - is the `error` prose of a 400 and nothing else pins it", - ); - } + // Completeness over the *whole* enum, not just the half with a + // generated list. `variant_index` is wildcard-free, so an eighteenth + // variant breaks the build here rather than sliding through with its + // wording — the `error` prose of a 400 or a 500 — pinned by nothing + // (#1269). + assert_covers_every_variant(cases.iter().map(|(err, _)| err), "Display cases"); for (err, expected) in cases { let rendered = err.to_string(); assert!( @@ -137,10 +196,12 @@ fn invalid_formula_lists_the_accepted_names() { /// otherwise invisible: it shrinks one list and grows the other, which /// every per-variant assertion in this file survives. /// -/// There is deliberately no matching `ENVIRONMENT_COUNT`. The +/// There is deliberately no matching `ENVIRONMENT_COUNT`: the /// environment cases below are a vec literal, so asserting its length /// would compare a literal against a constant and could not fail for -/// any change to `Error`. +/// any change to `Error`. What does hold the environment half honest is +/// [`assert_covers_every_variant`], which needs the two groups to be +/// jointly exhaustive over a compile-forced witness (#1269). const CLIENT_INPUT_COUNT: usize = 11; // Pin the client-input vs environment/backend classification of every @@ -177,6 +238,15 @@ fn is_client_input_classifies_every_variant() { "the client-input group changed size; if that is deliberate, every \ new variant also needs an `error_kind` token in the web crate", ); + // Jointly exhaustive and disjoint over every variant. This is what + // stops a new *environment* variant from going unclassified: the + // generated `client_input_samples` cannot cover it, and the vec above + // is hand-written, so without this the pair could silently classify + // 17 of 18 (#1269). + assert_covers_every_variant( + client_input.iter().chain(environment.iter()), + "classification", + ); for err in client_input { assert!(err.is_client_input(), "{err:?} should be client input"); } From f5482868dd7018b3cdf4972d58dc3f944bef31ea Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 9 Aug 2026 12:34:46 -0700 Subject: [PATCH 08/16] fix(cli/check): render mi.* baseline regressions with a valid tag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit format_regressed_tag computed its percentage in the higher-is-worse direction and prefixed a literal +, so a lower-is-worse mi.* metric — where the classifier emits Regressed on a drop — rendered the double-signed [regr +-29%]. Measure the magnitude in the metric's own direction so the tag keeps one greppable shape, and correct the two comments and two book passages that described the old behaviour. Fixes #1242 --- CHANGELOG.md | 10 ++++++ .../src/recipes/baselines.md | 6 +++- big-code-analysis-book/src/recipes/ci.md | 11 +++--- big-code-analysis-cli/src/baseline.rs | 14 +++++--- big-code-analysis-cli/src/thresholds.rs | 31 +++++++++++++---- big-code-analysis-cli/src/thresholds_tests.rs | 34 +++++++++++++++++++ 6 files changed, 90 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ca30ba870..1ac55d7af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -96,6 +96,16 @@ for historical reference. ### Fixed +- `bca check --baseline` rendered a malformed, double-signed + `[regr +-29%]` tag for every `mi.*` regression. `Baseline::classify` + has been direction-aware since #827 — for the lower-is-worse `mi.*` + family a *drop* below the recorded value is the regression — but the + tag formatter still computed `(value - recorded) / recorded` and + prefixed a literal `+`, so the percentage carried its own minus sign. + Anything grepping the documented `[regr +N%]` shape mis-parsed it. The + magnitude is now measured in the metric's own direction, keeping one + tag shape across every metric; the higher-is-worse rendering is + unchanged (#1242). - The VCS bot-author filter was case-sensitive despite `DEFAULT_BOT_PATTERN`'s doc promising the opposite since the option shipped. `BotFilter::new` compiled with a plain `Regex`, and diff --git a/big-code-analysis-book/src/recipes/baselines.md b/big-code-analysis-book/src/recipes/baselines.md index a7cd0818a..62b436d17 100644 --- a/big-code-analysis-book/src/recipes/baselines.md +++ b/big-code-analysis-book/src/recipes/baselines.md @@ -199,7 +199,11 @@ Tag prefixes: written. See [Matching](#how-matching-works) for the resolution order. - `[regr +N%]` — the baseline contains a recorded value and the - current value is `N%` higher. Cases: + current value is `N%` worse. "Worse" is measured in the metric's own + direction, so for the lower-is-worse `mi.*` family a *drop* of `N%` + below the recorded value reads `+N%` like any other regression; the + tag has one shape across every metric, which is what makes it safe to + grep for. Cases: - `[regr from 0]` when the recorded value is `0.0` and a non-zero percentage would divide by zero. - `[regr +>9999%]` caps once the regression exceeds 100× the diff --git a/big-code-analysis-book/src/recipes/ci.md b/big-code-analysis-book/src/recipes/ci.md index f78407bc4..9f8663bbe 100644 --- a/big-code-analysis-book/src/recipes/ci.md +++ b/big-code-analysis-book/src/recipes/ci.md @@ -302,10 +302,13 @@ so a developer can tell at a glance whether they are looking at a brand-new offender or a known one that has worsened: - `[new]` — no baseline entry for this function / metric. -- `[regr +N%]` — current value exceeds the recorded baseline by `N` - percent. Special forms: `[regr from 0]` when the baseline value - was zero, `[regr +>9999%]` when the regression exceeds 100× the - baseline, `[regr NaN]` when the current value is NaN. +- `[regr +N%]` — current value is `N` percent worse than the recorded + baseline, measured in the metric's own direction (for the + lower-is-worse `mi.*` family that is a drop, still rendered `+N%`, so + the tag has one greppable shape). Special forms: `[regr from 0]` when + the baseline value was zero, `[regr +>9999%]` when the regression + exceeds 100× the baseline, `[regr NaN]` when the current value is + NaN. The violation rows go to stdout; everything below goes to stderr. See [Which stream](../commands/check.md#which-stream) for the full diff --git a/big-code-analysis-cli/src/baseline.rs b/big-code-analysis-cli/src/baseline.rs index 910b0f9aa..02bd585de 100644 --- a/big-code-analysis-cli/src/baseline.rs +++ b/big-code-analysis-cli/src/baseline.rs @@ -489,11 +489,15 @@ impl Baseline { // parser also rejects nan/inf). Negative is the same idea // for the rendered-tag path: `format_regressed_tag` divides // by `recorded` to compute `+N%`, and a negative `recorded` - // produces a double-signed `[regr +-N%]` tag. Metric - // extractors never emit negative values, so the only way - // an entry can be negative is hand-editing — silently - // drop and treat the entry as if missing (the violation - // will surface as `New`). + // flips the quotient's sign, producing a double-signed + // `[regr +-N%]` tag. Metric extractors never emit negative + // values, so the only way an entry can be negative is + // hand-editing — silently drop and treat the entry as if + // missing (the violation will surface as `New`). This filter + // was never the only route to that malformed tag, though: + // until #1242 an ordinary `mi.*` regression produced it from + // a perfectly well-formed baseline, because the numerator + // was computed in the higher-is-worse direction. // // `is_sign_negative()` catches `-0.0` too (which `< 0.0` // misses under IEEE 754: `-0.0 < 0.0` is false). `-0.0` diff --git a/big-code-analysis-cli/src/thresholds.rs b/big-code-analysis-cli/src/thresholds.rs index fa7440763..cc01e0194 100644 --- a/big-code-analysis-cli/src/thresholds.rs +++ b/big-code-analysis-cli/src/thresholds.rs @@ -695,7 +695,10 @@ pub(crate) fn render_violation_line(v: &Violation, tag: Option<&Coverage>) -> St None | Some(Coverage::Covered { .. }) => format!("{v}"), Some(Coverage::New) => format!("[new] {v}"), Some(Coverage::Regressed { recorded }) => { - format!("{} {v}", format_regressed_tag(*recorded, v.value)) + format!( + "{} {v}", + format_regressed_tag(*recorded, v.value, v.lower_is_worse) + ) } } } @@ -707,23 +710,39 @@ pub(crate) fn render_violation_line(v: &Violation, tag: Option<&Coverage>) -> St /// - `pct > 9999` → `[regr +>9999%]` (cap; 100× the baseline is /// already screaming-loud, exact number adds nothing). /// - else → `[regr +N%]` with N rounded to nearest integer. -fn format_regressed_tag(recorded: f64, value: f64) -> String { +/// +/// The magnitude is measured in the metric's own direction, so the tag +/// always reads "worse by N%" and log parsers see one shape across every +/// metric. For the lower-is-worse `mi.*` family that means inverting the +/// difference, matching how [`Violation::ratio`] already inverts. Before +/// #1242 this function assumed the higher-is-worse direction and printed +/// a literal `+` in front of what was then a negative percentage, so an +/// ordinary `mi.original` regression rendered the double-signed +/// `[regr +-29%]`. +fn format_regressed_tag(recorded: f64, value: f64, lower_is_worse: bool) -> String { if value.is_nan() { return "[regr NaN]".to_string(); } if recorded == 0.0 { return "[regr from 0]".to_string(); } - let pct = ((value - recorded) / recorded * 100.0).round(); + let worse_by = if lower_is_worse { + recorded - value + } else { + value - recorded + }; + let pct = (worse_by / recorded * 100.0).round(); if pct > 9999.0 { return "[regr +>9999%]".to_string(); } // `{:.0}` formats the rounded float with zero decimal digits, so // we avoid an f64-to-int cast that clippy flags as possibly // truncating. `pct` is finite (caller filtered NaN and zero - // `recorded`), bounded above by 9999 here, and bounded below by 0 - // because the classifier only emits Regressed when - // `value > recorded`. + // `recorded`), bounded above by 9999 here, and non-negative because + // the classifier only emits Regressed when the value is worse than + // `recorded` *in the metric's direction* — which is the direction + // `worse_by` is now measured in. A regression too small to round to + // one percent renders `[regr +0%]`, as it always has. format!("[regr +{pct:.0}%]") } diff --git a/big-code-analysis-cli/src/thresholds_tests.rs b/big-code-analysis-cli/src/thresholds_tests.rs index 560cf3335..ca4695abe 100644 --- a/big-code-analysis-cli/src/thresholds_tests.rs +++ b/big-code-analysis-cli/src/thresholds_tests.rs @@ -725,6 +725,40 @@ fn render_regressed_integer_percent() { assert!(out.starts_with("[regr +50%] "), "got: {out}"); } +#[test] +fn render_regressed_lower_is_worse_tag_is_well_formed() { + // Regression for #1242. `Baseline::classify` is direction-aware, so + // for the lower-is-worse `mi.*` family it emits `Regressed` when the + // value *drops* below the recorded one. The tag formatter assumed + // the opposite direction and prefixed a literal `+` to what was then + // a negative percentage, rendering `[regr +-29%]` — a shape no log + // parser anchored on `[regr +N%]` can read. The magnitude is now + // measured in the metric's own direction, so it stays "worse by N%". + // Values are the issue's reproducer: `mi.original` recorded at + // 142.75 dropping to 101.03 is (142.75 - 101.03) / 142.75 ≈ 29%. + let mut v = sample_violation(); + v.metric = "mi.original"; + v.lower_is_worse = true; + v.value = 101.03; + v.limit = 160.0; + let out = render_violation_line(&v, Some(&Coverage::Regressed { recorded: 142.75 })); + assert!(out.starts_with("[regr +29%] "), "got: {out}"); + assert!(!out.contains("+-"), "double-signed percentage: {out}"); +} + +#[test] +fn render_regressed_direction_is_not_a_blanket_sign_flip() { + // The companion to the test above: fixing the `mi.*` direction must + // not invert the higher-is-worse one. A cyclomatic rise from 20 to + // 30 is still `+50%`, not `-50%` — pinned here because a sign flip + // in the wrong branch would satisfy the test above on its own + // (#1242). + let v = sample_violation(); + assert!(!v.lower_is_worse, "the fixture must be higher-is-worse"); + let out = render_violation_line(&v, Some(&Coverage::Regressed { recorded: 20.0 })); + assert!(out.starts_with("[regr +50%] "), "got: {out}"); +} + #[test] fn render_regressed_rounds_half_to_nearest_even_or_away() { // Halstead can produce values that round to a nearby integer; From 968a42ae78bd073a80792b4a1318be231e63c8b7 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 9 Aug 2026 12:35:48 -0700 Subject: [PATCH 09/16] refactor(enums): drop the dead escape flag from sanitize_string MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No production caller passed escape=true: #862 established the JSON generator was the last one and was double-escaping by mistake. Removing the flag deletes the dead double-backslash branch, its test, the JSON_TOKEN_ESCAPE constant that existed only to document why the answer must be false, and a boolean from every call site — making the #862 bug unrepresentable rather than one flipped boolean away. Codegen output is byte-identical. Fixes #1241 --- enums/src/common.rs | 76 +++++++++++++++++---------------------------- enums/src/go.rs | 2 +- enums/src/json.rs | 11 ++----- enums/src/rust.rs | 2 +- 4 files changed, 33 insertions(+), 58 deletions(-) diff --git a/enums/src/common.rs b/enums/src/common.rs index 40ff789e3..309346f06 100644 --- a/enums/src/common.rs +++ b/enums/src/common.rs @@ -95,35 +95,25 @@ pub fn sanitize_identifier(name: &str) -> String { /// Escapes a tree-sitter node-kind string for embedding in a string /// literal. /// -/// `escape` selects how many rounds of literal interpretation the value -/// must survive: `false` emits the single-backslash form for a literal -/// parsed once (Rust source, JSON — see issue #862), `true` the -/// double-backslash form for a value that is re-interpreted a second -/// time. +/// Emits the single-backslash form, for a literal that is parsed exactly +/// once. Every generator here wants that: the Rust, Go and JSON outputs +/// each interpolate the value into one string literal and no output +/// format has a second source-string interpretation layer. A +/// double-backslash variant existed behind an `escape: bool` until +/// #1241; its last caller was the JSON generator, and #862 established +/// that caller was double-escaping by mistake. The flag is gone so the +/// #862 bug is unrepresentable rather than one flipped boolean away. #[must_use] -pub fn sanitize_string(name: &str, escape: bool) -> String { +pub fn sanitize_string(name: &str) -> String { let mut result = String::with_capacity(name.len()); - if escape { - for c in name.chars() { - match c { - '\"' => result += "\\\\\\\"", - '\\' => result += "\\\\\\\\", - '\t' => result += "\\\\t", - '\n' => result += "\\\\n", - '\r' => result += "\\\\r", - _ => result.push(c), - } - } - } else { - for c in name.chars() { - match c { - '\"' => result += "\\\"", - '\\' => result += "\\\\", - '\t' => result += "\\t", - '\n' => result += "\\n", - '\r' => result += "\\r", - _ => result.push(c), - } + for c in name.chars() { + match c { + '\"' => result += "\\\"", + '\\' => result += "\\\\", + '\t' => result += "\\t", + '\n' => result += "\\n", + '\r' => result += "\\r", + _ => result.push(c), } } result @@ -159,8 +149,7 @@ pub fn camel_case(name: &str) -> String { /// before anonymous ones, so a named kind keeps the unsuffixed name and /// an anonymous namesake takes the suffix; that is a naming priority, /// not the order of the returned `Vec`, which stays keyed on the id so -/// the generated enum's discriminants match tree-sitter's. `escape` is -/// forwarded to [`sanitize_string`]. +/// the generated enum's discriminants match tree-sitter's. /// /// # Panics /// @@ -171,7 +160,7 @@ pub fn camel_case(name: &str) -> String { /// shipped library code, so failing the run loudly is the cheaper /// outcome. #[must_use] -pub fn get_token_names(language: &Language, escape: bool) -> Vec<(String, bool, String)> { +pub fn get_token_names(language: &Language) -> Vec<(String, bool, String)> { let count = language.node_kind_count(); let mut names = BTreeMap::default(); let mut name_count = HashMap::new(); @@ -195,7 +184,7 @@ pub fn get_token_names(language: &Language, escape: bool) -> Vec<(String, bool, let kind = language .node_kind_for_id(id) .expect("id < node_kind_count is a valid node kind"); - let ts_name = sanitize_string(kind, escape); + let ts_name = sanitize_string(kind); let mut name = camel_case(&sanitize_identifier(kind)); if name.is_empty() { name = format!("Anon{i}"); @@ -296,25 +285,18 @@ mod tests { assert_eq!(sanitize_identifier("Self"), "SELF"); } - // escape=false emits the single-backslash (Rust source) form. + // The single-backslash form, for a literal parsed exactly once. This + // is now the only form `sanitize_string` can emit; the + // double-backslash variant and its `escape=true` test went with the + // flag in #1241. #[test] - fn sanitize_string_no_escape() { + fn sanitize_string_escapes_for_a_single_parse() { assert_eq!( - sanitize_string("a\"b\\c\td\ne\rf", false), + sanitize_string("a\"b\\c\td\ne\rf"), "a\\\"b\\\\c\\td\\ne\\rf" ); } - // escape=true emits the double-backslash form (the value survives a - // second round of source-string interpretation). - #[test] - fn sanitize_string_escape() { - assert_eq!( - sanitize_string("a\"b\\c\td\ne\rf", true), - "a\\\\\\\"b\\\\\\\\c\\\\td\\\\ne\\\\rf" - ); - } - #[test] fn camel_case_simple() { assert_eq!(camel_case("foo_bar"), "FooBar"); @@ -338,7 +320,7 @@ mod tests { #[test] fn get_token_names_appends_error_sentinel_last() { let language: Language = tree_sitter_rust::LANGUAGE.into(); - let names = get_token_names(&language, false); + let names = get_token_names(&language); let (rust_name, renamed, ts_name) = names.last().expect("non-empty token list"); assert_eq!(ts_name, "ERROR"); assert_eq!(rust_name, "Error"); @@ -348,7 +330,7 @@ mod tests { #[test] fn get_token_names_have_unique_rust_names() { let language: Language = tree_sitter_rust::LANGUAGE.into(); - let names = get_token_names(&language, false); + let names = get_token_names(&language); let mut seen = std::collections::HashSet::new(); for (rust_name, _, _) in &names { assert!( @@ -367,7 +349,7 @@ mod tests { #[test] fn get_token_names_renamed_flag_marks_deduplicated_entries() { let language: Language = tree_sitter_rust::LANGUAGE.into(); - let names = get_token_names(&language, false); + let names = get_token_names(&language); let originals: Vec<&str> = names .iter() .filter(|(_, renamed, _)| !renamed) diff --git a/enums/src/go.rs b/enums/src/go.rs index 3c0a00c5b..0aef9e072 100644 --- a/enums/src/go.rs +++ b/enums/src/go.rs @@ -31,7 +31,7 @@ pub fn generate_go(output: &Path, file_template: &str) -> std::io::Result<()> { let path = output.join(file_name); let mut file = File::create(path)?; - let names = get_token_names(&language, false); + let names = get_token_names(&language); // `unwrap_or(0)` is the identity, not a swallowed error: with no // names the `map` below yields nothing, so the padding width is // never read. The empty case is unreachable — `get_token_names` diff --git a/enums/src/json.rs b/enums/src/json.rs index 6f91d8369..79d088004 100644 --- a/enums/src/json.rs +++ b/enums/src/json.rs @@ -12,13 +12,6 @@ struct JsonTemplate { names: Vec<(String, bool, String)>, } -// The token text is interpolated into a JSON string literal that is parsed -// exactly once, so use the single-backslash escape form (escape=false), -// matching the Rust and Go generators. The double-backslash form -// (escape=true) is only correct for values that survive a second -// source-string interpretation layer (issue #862). -const JSON_TOKEN_ESCAPE: bool = false; - /// Writes one JSON token-kind table per [`Lang`] into `output`. /// /// `file_template` is the file stem with `$` standing in for the @@ -41,7 +34,7 @@ pub fn generate_json(output: &Path, file_template: &str) -> std::io::Result<()> let path = output.join(file_name); let mut file = File::create(path)?; - let names = get_token_names(&language, JSON_TOKEN_ESCAPE); + let names = get_token_names(&language); let args = JsonTemplate { names }; @@ -89,7 +82,7 @@ mod tests { // Render a single ["name", "ts_name"] row exactly as `generate_json` // would, then extract the quoted ts_name back out of the rendered JSON. fn rendered_ts_name(token: &str) -> String { - let ts_name = sanitize_string(token, JSON_TOKEN_ESCAPE); + let ts_name = sanitize_string(token); let args = JsonTemplate { names: vec![("Tok".to_string(), false, ts_name)], }; diff --git a/enums/src/rust.rs b/enums/src/rust.rs index 772757bab..60a81fdee 100644 --- a/enums/src/rust.rs +++ b/enums/src/rust.rs @@ -45,7 +45,7 @@ pub fn generate_rust(output: &Path, file_template: &str) -> std::io::Result<()> fn build_rust_template(lang: &Lang) -> RustTemplate { let c_name = camel_case(get_language_name(lang)); - let names = get_token_names(&get_language(lang), false); + let names = get_token_names(&get_language(lang)); // `get_token_names` always appends the tree-sitter ERROR sentinel last // (pinned by its `get_token_names_appends_error_sentinel_last` test), so the From 609d473875e25b1746753b2c5696facc30391a3a Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 9 Aug 2026 12:38:04 -0700 Subject: [PATCH 10/16] fix(enums): collision-check minted dedup names get_token_names minted a suffixed name (Foo -> Foo2) and returned it without registering it, so a mint could silently duplicate a node kind whose own sanitized name is literally base + digits. tree-sitter-php is one aliased rule away: it already emits cast_type_token1 through cast_type_token12, so a single new id carrying cast_type_token1 would mint a second CastTypeToken12. The generator would exit 0 and the duplicate would surface as a rustc error in the parent crate during a grammar bump. Route both mint sites through a claim_name helper that probes upward past a squatter and registers what it hands out, and widen the uniqueness test from Rust alone to every linked grammar. Codegen output is byte-identical. Fixes #1237 --- enums/src/common.rs | 151 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 118 insertions(+), 33 deletions(-) diff --git a/enums/src/common.rs b/enums/src/common.rs index 309346f06..047dde5a4 100644 --- a/enums/src/common.rs +++ b/enums/src/common.rs @@ -1,5 +1,5 @@ use std::collections::BTreeMap; -use std::collections::hash_map::{Entry, HashMap}; +use std::collections::HashMap; use tree_sitter::Language; /// Lifts an `askama` render failure onto the `io::Result` channel every @@ -140,6 +140,48 @@ pub fn camel_case(name: &str) -> String { result } +/// Claims a unique Rust name for `base`, returning it and whether a +/// collision-breaking suffix had to be minted. +/// +/// `name_count` doubles as the taken-name set and the per-base counter: +/// every returned name is registered as a key, so a name minted here can +/// never be handed out twice and can never be claimed later by a literal +/// namesake either. +/// +/// Registering the mint is the whole point (#1237). The previous code +/// incremented only the *base*'s counter and returned `base + counter` +/// without inserting it, so a minted name could silently duplicate a +/// node kind whose own sanitized name is literally `base + digits` — +/// and `tree-sitter-php` is one aliased rule away from exactly that: it +/// already emits `cast_type_token1` … `cast_type_token12`, so a single +/// new id carrying the kind string `cast_type_token1` would mint a +/// second `CastTypeToken12`. The generator would still exit `0` and the +/// duplicate would surface as a rustc duplicate-variant error in the +/// *parent* crate during a grammar bump, reading as upstream breakage +/// rather than a generator bug. Probing upward past a squatter makes the +/// name unique by construction instead. +/// +/// The base counter advances to the suffix actually used, so a later +/// claim of the same base resumes above it and the walk stays linear +/// over repeated collisions rather than re-probing from the start. +fn claim_name(name_count: &mut HashMap, base: String) -> (String, bool) { + let Some(&last) = name_count.get(&base) else { + name_count.insert(base.clone(), 1); + return (base, false); + }; + let mut n = last; + let candidate = loop { + n += 1; + let candidate = format!("{base}{n}"); + if !name_count.contains_key(&candidate) { + break candidate; + } + }; + name_count.insert(base, n); + name_count.insert(candidate.clone(), 1); + (candidate, true) +} + /// Enumerates a grammar's node kinds as `(rust_name, renamed, ts_name)` /// triples in node-kind id order, with the tree-sitter ERROR sentinel /// appended last. @@ -189,33 +231,20 @@ pub fn get_token_names(language: &Language) -> Vec<(String, bool, String)> { if name.is_empty() { name = format!("Anon{i}"); } - let e = match name_count.entry(name.clone()) { - Entry::Occupied(mut e) => { - *e.get_mut() += 1; - (format!("{}{}", name, e.get()), true, ts_name) - } - Entry::Vacant(e) => { - e.insert(1); - (name, false, ts_name) - } - }; - names.insert(i, e); + let (name, renamed) = claim_name(&mut name_count, name); + names.insert(i, (name, renamed, ts_name)); } } let mut names: Vec<_> = names.values().cloned().collect(); // The tree-sitter ERROR sentinel is appended last. If the grammar already - // defines an "error" keyword that camel-cased to "Error", increment the - // counter so this sentinel gets a unique name (e.g. "Error2"). - let error_name = match name_count.entry("Error".to_string()) { - Entry::Occupied(mut e) => { - *e.get_mut() += 1; - format!("Error{}", e.get()) - } - Entry::Vacant(e) => { - e.insert(1); - "Error".to_string() - } - }; + // defines an "error" keyword that camel-cased to "Error", `claim_name` + // mints a unique name for the sentinel (e.g. "Error2"), probing past any + // literal kind already squatting that suffix (#1237). The sentinel's + // `renamed` flag stays `false` whatever name it lands on: the flag marks + // an entry the reverse string->kind mappings must drop, and the sentinel + // is excluded from those for its own reasons — pinned by + // `get_token_names_appends_error_sentinel_last`. + let (error_name, _minted) = claim_name(&mut name_count, "Error".to_string()); names.push((error_name, false, "ERROR".to_string())); names @@ -327,17 +356,73 @@ mod tests { assert!(!renamed, "the ERROR sentinel is not a deduplicated entry"); } + // Every grammar the crate links, not just Rust. A duplicate name is a + // rustc duplicate-variant error in the *parent* crate, surfacing + // during a grammar bump where it reads as upstream breakage; catching + // it here names the generator instead (#1237). Rust alone could not: + // it has no reachable collision, so the assertion was vacuous for the + // hazard it is written against. The live near-misses are elsewhere — + // Rust's own literal `Expr2021`, and PHP's `cast_type_token1` … + // `cast_type_token12`, which are one aliased rule away from a real + // collision. #[test] - fn get_token_names_have_unique_rust_names() { - let language: Language = tree_sitter_rust::LANGUAGE.into(); - let names = get_token_names(&language); - let mut seen = std::collections::HashSet::new(); - for (rust_name, _, _) in &names { - assert!( - seen.insert(rust_name.clone()), - "duplicate Rust name emitted: {rust_name}" - ); + fn get_token_names_have_unique_rust_names_in_every_grammar() { + let mut checked = 0_usize; + for lang in crate::languages::Lang::into_enum_iter() { + let names = get_token_names(&crate::languages::get_language(&lang)); + let mut seen = std::collections::HashSet::new(); + for (rust_name, _, _) in &names { + assert!( + seen.insert(rust_name.clone()), + "duplicate Rust name emitted for {lang:?}: {rust_name}" + ); + } + checked += 1; } + assert!(checked > 1, "the sweep must cover more than one grammar"); + } + + // The PHP shape, reproduced synthetically because no current grammar + // reaches it: a literal kind already occupies the suffix the mint + // would hand out. The mint must probe past the squatter rather than + // duplicate it (#1237). Before the fix the third claim returned + // `Foo2` — the name the second row already holds. + #[test] + fn claim_name_probes_past_a_literal_squatting_the_mint_suffix() { + let mut name_count = HashMap::new(); + assert_eq!( + claim_name(&mut name_count, "Foo".to_string()), + ("Foo".to_string(), false) + ); + // A *literal* kind whose own sanitized name is the base plus a + // digit — `Foo2` here, `CastTypeToken12` in tree-sitter-php. + assert_eq!( + claim_name(&mut name_count, "Foo2".to_string()), + ("Foo2".to_string(), false) + ); + // The second claim of `Foo` must not mint `Foo2` again. + assert_eq!( + claim_name(&mut name_count, "Foo".to_string()), + ("Foo3".to_string(), true) + ); + // And a claim of the squatter itself dedups against its own + // registration rather than colliding. + assert_eq!( + claim_name(&mut name_count, "Foo2".to_string()), + ("Foo22".to_string(), true) + ); + } + + // Repeated collisions on one base advance the counter monotonically, + // so the mint stays deterministic and linear rather than re-probing + // from the start each time (#1237). + #[test] + fn claim_name_resumes_above_the_last_suffix_it_used() { + let mut name_count = HashMap::new(); + let claims: Vec = (0..4) + .map(|_| claim_name(&mut name_count, "Bar".to_string()).0) + .collect(); + assert_eq!(claims, ["Bar", "Bar2", "Bar3", "Bar4"]); } // The bool flag marks entries whose Rust name was suffixed with a From 1817f0eda5b23c1cec83c64fee265c7301315bce Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 9 Aug 2026 12:46:01 -0700 Subject: [PATCH 11/16] chore(baseline): refresh after the quick-fix batch Four movements, all consequences of this branch: the Go ABC var_spec arm raises GoCode::compute's cyclomatic and halstead.effort, the direction-aware regression tag adds eight ploc to thresholds.rs, and build_cached got cheaper now that its boundary arithmetic is a helper call. The fifth is an identity shift, not a regression: removing four comment lines from history.rs renamed the anonymous space diff_collect:: to at an unchanged nexits of 6. --- .bca-baseline.toml | 10 +++++----- CHANGELOG.md | 20 ++++++++++++++++++++ src/vcs/identity.rs | 3 ++- 3 files changed, 27 insertions(+), 6 deletions(-) diff --git a/.bca-baseline.toml b/.bca-baseline.toml index fffe27890..7ada049e9 100644 --- a/.bca-baseline.toml +++ b/.bca-baseline.toml @@ -339,7 +339,7 @@ value = 66737.04848699087 path = "big-code-analysis-cli/src/thresholds.rs" qualified = "" metric = "loc.ploc" -value = 554.0 +value = 562.0 [[entry]] path = "big-code-analysis-cli/src/thresholds.rs" @@ -855,13 +855,13 @@ value = 15.0 path = "src/metrics/abc/go.rs" qualified = "GoCode::compute" metric = "cyclomatic" -value = 15.0 +value = 17.0 [[entry]] path = "src/metrics/abc/go.rs" qualified = "GoCode::compute" metric = "halstead.effort" -value = 50933.14135135399 +value = 55577.39635901512 [[entry]] path = "src/metrics/abc/groovy.rs" @@ -1257,7 +1257,7 @@ value = 6.0 path = "src/vcs/git/cached.rs" qualified = "build_cached" metric = "halstead.effort" -value = 76740.04063247392 +value = 73179.83359417088 [[entry]] path = "src/vcs/git/cached.rs" @@ -1315,7 +1315,7 @@ value = 8.0 [[entry]] path = "src/vcs/git/history.rs" -qualified = "diff_collect::" +qualified = "diff_collect::" metric = "nexits" value = 6.0 diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ac55d7af..f45311911 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -96,6 +96,18 @@ for historical reference. ### Fixed +- The `enums` code generator minted collision-breaking names + (`Foo` → `Foo2`) without registering them, so a minted suffix could + silently duplicate a node kind whose own sanitized name is literally + `base` + digits. `tree-sitter-php` is one aliased rule away from + exactly that — it already emits `cast_type_token1` through + `cast_type_token12` — and the generator would have exited `0`, leaving + the duplicate to surface as a rustc duplicate-variant error in the + parent crate during a grammar bump, where it reads as upstream + breakage. Minted names are now probed against, and registered in, the + taken-name set. No shipped artifact changes: no current grammar + reaches the collision, so every generated file is byte-identical + (#1237). - `bca check --baseline` rendered a malformed, double-signed `[regr +-29%]` tag for every `mi.*` regression. `Baseline::classify` has been direction-aware since #827 — for the lower-is-worse `mi.*` @@ -338,6 +350,14 @@ for historical reference. ### Changed +- The `enums` generator's `sanitize_string` / `get_token_names` lost their + `escape: bool` parameter. No production caller passed `true`: #862 + established that the JSON generator, the last one, was double-escaping + by mistake. Removing the flag deletes the dead double-backslash branch, + its test, and the `JSON_TOKEN_ESCAPE` constant that existed only to + document why the answer must be `false` — making the #862 bug + unrepresentable rather than one flipped boolean away. Internal build + tool only; generated output is byte-identical (#1241). - The workspace-excluded `enums` crate declares the workspace lint posture (`clippy::pedantic`, `missing_docs`) in its own manifest. `[workspace.lints]` reaches members only, so `make enums-check` had been diff --git a/src/vcs/identity.rs b/src/vcs/identity.rs index 267b5f524..c88e2053b 100644 --- a/src/vcs/identity.rs +++ b/src/vcs/identity.rs @@ -253,7 +253,8 @@ impl BotFilter { /// Compile a bot-exclusion pattern. /// /// Matching is **case-insensitive**, which is what - /// [`DEFAULT_BOT_PATTERN`](crate::vcs::DEFAULT_BOT_PATTERN) has always + /// [`DEFAULT_BOT_PATTERN`](crate::vcs::options::DEFAULT_BOT_PATTERN) + /// has always /// documented and what the substring-style patterns users write /// expect: `--bot-pattern renovate` matches `Renovate Bot`. The flag /// is only the *default* for the pattern, so an expert can restore From 5e7b44bcdecd2fdeb3441096221ab0becafb7228 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 9 Aug 2026 12:52:54 -0700 Subject: [PATCH 12/16] docs(vcs/identity): rewrap the BotFilter case-insensitivity note --- src/vcs/identity.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/vcs/identity.rs b/src/vcs/identity.rs index c88e2053b..c9a2d1612 100644 --- a/src/vcs/identity.rs +++ b/src/vcs/identity.rs @@ -254,9 +254,9 @@ impl BotFilter { /// /// Matching is **case-insensitive**, which is what /// [`DEFAULT_BOT_PATTERN`](crate::vcs::options::DEFAULT_BOT_PATTERN) - /// has always - /// documented and what the substring-style patterns users write - /// expect: `--bot-pattern renovate` matches `Renovate Bot`. The flag + /// has always documented, and what the substring-style patterns + /// users write expect: `--bot-pattern renovate` matches + /// `Renovate Bot`. The flag /// is only the *default* for the pattern, so an expert can restore /// case sensitivity for all or part of it with an inline `(?-i)` — /// `(?-i)Foo` does not match `foo`. From 2a97598abfe5fa136d70ae5994090113c8bd7925 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 9 Aug 2026 14:45:02 -0700 Subject: [PATCH 13/16] fix(abc): gate Bash redirects and widen Ruby's comparison gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code review found the #1280 polarity applies in two more places. Bash spells an I/O redirection with the same < and > tokens as a comparison, parented under file_redirect, so every redirect in a script scored a condition — the Bash instance of the bug #1280 fixed for Ruby, missed by that issue's cross-language sweep. Both tokens now require a binary_expression parent. Ruby's gate covered only LT/GT, but every comparison and equality token is equally definable as an operator method, so def ==(other) kept the phantom condition def <(other) lost. The gate now covers all eight. Bash ABC also scored zero for the arithmetic ternary while #1268 had already brought it into cyclomatic and cognitive; ABC now matches the C-family, so Bash and C agree at 2 for the same expression. --- CHANGELOG.md | 20 ++++++++- src/metrics/abc.rs | 89 +++++++++++++++++++++++++++++++++++++++++ src/metrics/abc/bash.rs | 38 +++++++++++++++--- src/metrics/abc/ruby.rs | 37 +++++++++-------- 4 files changed, 162 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f45311911..26ad269c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -184,7 +184,25 @@ for historical reference. The same gate stops counting the `<` that names an operator method (`def <(other)`). Real comparisons, `<=>`, the `<<` shovel and heredoc openers are unaffected. Ruby `abc` values drop by one per subclass - declaration and per operator-method definition (#1280). + declaration and per operator-method definition (#1280). The gate now + covers the sibling comparison and equality tokens too — `==`, `!=`, + `===`, `<=`, `>=`, `<=>`, `=~`, `!~` are each equally definable as an + operator method, so `def ==(other)` scored the same phantom condition + `def <(other)` did. +- Bash ABC counted every I/O redirection as a condition. `>` and `<` + spell a redirect as well as a comparison, and the grammar parents the + redirect under `file_redirect`, so `echo hi > out.txt` reported + `conditions = 1` for a line with no test in it — the Bash instance of + the #1280 polarity, missed by that issue's cross-language sweep. Both + tokens now require a `binary_expression` parent; comparisons inside + `[[ … ]]` and `(( … ))` are unaffected. Bash `abc` values drop by one + per redirection (#1280). +- Bash ABC scored no condition for the arithmetic ternary, so + `local m=$(( a > b ? a : b ))` reported 1 where the equivalent C + `int m = a > b ? a : b;` reports 2. #1268 brought Bash's only ternary + form into cyclomatic and cognitive; ABC now counts it too, matching + the C-family `ConditionalExpression`. Bash `abc` values rise by one + per arithmetic ternary (#1268). - `fix_includes` returned its diagnostics in a different order on every run for identical input, because both producers push while iterating a `HashMap` — `files` in `build_include_graph`, `nodes` in diff --git a/src/metrics/abc.rs b/src/metrics/abc.rs index d21e08c97..8e2533c41 100644 --- a/src/metrics/abc.rs +++ b/src/metrics/abc.rs @@ -1322,6 +1322,59 @@ mod tests { ); } + #[test] + fn bash_redirection_is_not_a_condition() { + // `>` and `<` spell an I/O redirection as well as a comparison, and + // the grammar parents the redirection under `file_redirect` rather + // than `binary_expression`. Ungated, every redirect in a script + // scored a condition: this fixture measured 2 before the parent + // gate — the Bash instance of #1280's positive-parent polarity. + // expected: 0 conditions — no test, no `if`, no comparison. + check_metrics::( + "f() {\n echo hi > out.txt\n read x < in.txt\n}\n", + "foo.sh", + |metric| { + assert_eq!(metric.abc.conditions_sum(), 0); + // The two `echo` / `read` commands still count as branches, + // so the zero above is the gate firing rather than the walk + // skipping the function body. + assert_eq!(metric.abc.branches_sum(), 2); + }, + ); + } + + #[test] + fn bash_comparison_inside_an_arithmetic_or_test_context_is_a_condition() { + // The positive control for the gate above: the same tokens under a + // `binary_expression` are real comparisons in both the `[[ … ]]` + // test form and the `(( … ))` arithmetic form, and still count. + // expected: 4 — one `if` control-flow condition and one `>` per + // function. + check_metrics::( + "f() {\n if [[ $a > $b ]]; then :; fi\n}\ng() {\n if (( a > b )); then :; fi\n}\n", + "foo.sh", + |metric| { + assert_eq!(metric.abc.conditions_sum(), 4); + }, + ); + } + + #[test] + fn bash_arithmetic_ternary_is_a_condition() { + // The ABC half of #1268. Cyclomatic and cognitive both count Bash's + // only ternary form; ABC did not, so the identical construct scored + // 1 here against the C family's 2. + // expected: 2 — the `>` comparison and the ternary itself, matching + // `int m = a > b ? a : b;` in C. + check_metrics::( + "f() {\n local m=$(( a > b ? a : b ))\n}\n", + "foo.sh", + |metric| { + assert_eq!(metric.abc.conditions_sum(), 2); + }, + ); + } + #[test] fn bash_magnitude() { // Combined assignments + branches + conditions. The single `if` @@ -4853,6 +4906,42 @@ function f(int $a, int $b): int { }); } + #[test] + fn ruby_every_comparison_operator_method_name_is_not_a_condition() { + // The sibling half of #1280. `<` is not special: every comparison + // and equality token Ruby lets you `def` parents under `operator` + // in that position, so gating only `LT` / `GT` left `def ==`, + // `def <=`, `def >=`, `def <=>`, `def !=` and `def =~` each + // scoring a phantom condition — measured at 1 apiece for a body + // containing no conditional at all. + // expected: 0 conditions per definition; only the name token is on + // the line, so a single non-zero total localises the regression. + check_metrics::( + "def ==(o)\n 1\nend\n\ + def !=(o)\n 1\nend\n\ + def <=(o)\n 1\nend\n\ + def >=(o)\n 1\nend\n\ + def <=>(o)\n 1\nend\n\ + def =~(o)\n 1\nend\n\ + def <(o)\n 1\nend\n\ + def >(o)\n 1\nend\n", + "foo.rb", + |metric| { + assert_eq!(metric.abc.conditions_sum(), 0); + }, + ); + // The positive control: the same tokens inside a `binary` are real + // comparisons and still count, so the gate is not blanket + // suppression. + check_metrics::( + "def cmp(a, b)\n a == b || a <= b || a <=> b\nend\n", + "foo.rb", + |metric| { + assert_eq!(metric.abc.conditions_sum(), 3); + }, + ); + } + #[test] fn ruby_case_match_in_arms_are_conditions() { // Regression for #977: each non-wildcard `case … in` arm is one diff --git a/src/metrics/abc/bash.rs b/src/metrics/abc/bash.rs index 8a6ae1014..d99e1079d 100644 --- a/src/metrics/abc/bash.rs +++ b/src/metrics/abc/bash.rs @@ -16,7 +16,7 @@ impl Abc for BashCode { fn compute<'a>( node: &Node<'a>, code: &'a [u8], - _ancestors: Ancestors<'a, '_>, + ancestors: Ancestors<'a, '_>, stats: &mut Stats, ) { match node.kind_id().into() { @@ -33,7 +33,7 @@ impl Abc for BashCode { Bash::Command => { stats.branches += 1.; } - // Two condition signals share this arm: + // Three condition signals share this arm: // // - Comparison operators inside `[[ … ]]` and `(( … ))`, plus // the prefix test operators `-z`, `-n`, `-eq`, `-lt`, … which @@ -43,17 +43,45 @@ impl Abc for BashCode { // condition signal. These branch keywords mirror the matching // Bash cyclomatic decisions (`if`/`elif`/`while`; not `for`/ // `&&`/`||`), lifting ABC off 0 for `if cmd; then … elif … fi`. + // - The arithmetic ternary, Bash's only ternary form, matching + // the C-family `ConditionalExpression` and the Bash + // cyclomatic / cognitive arms that count it (#1268). + // `TernaryExpression2` is listed defensively per + // grammar-dispatch §1, as it is in those two siblings. Bash::EQEQ | Bash::BANGEQ - | Bash::LT - | Bash::GT | Bash::LTEQ | Bash::GTEQ | Bash::EQTILDE | Bash::TestOperator | Bash::IfStatement | Bash::ElifClause - | Bash::WhileStatement => { + | Bash::WhileStatement + | Bash::TernaryExpression + | Bash::TernaryExpression2 => { + stats.conditions += 1.; + } + // `<` and `>` are comparisons only inside a `binary_expression`. + // The same two tokens spell an I/O redirection (`cmd > out`, + // `read x < in`), which the grammar parents under + // `file_redirect` — so ungated they scored every redirect in a + // script as a condition. This is the Bash instance of the + // positive-parent gate #1280 applied to Ruby, and the polarity + // Rust / Go / C / C++ already use. At tree-sitter-bash 0.25.1 + // only `BinaryExpression3` wraps a comparison; the four + // siblings are listed defensively per grammar-dispatch §1. + Bash::LT | Bash::GT + if ancestors.parent(node).is_some_and(|p| { + matches!( + p.kind_id().into(), + Bash::BinaryExpression + | Bash::BinaryExpression2 + | Bash::BinaryExpression3 + | Bash::BinaryExpression4 + | Bash::BinaryExpression5 + ) + }) => + { stats.conditions += 1.; } // Case arms are conditions too, but counted per-arm like diff --git a/src/metrics/abc/ruby.rs b/src/metrics/abc/ruby.rs index 431251d93..e77b7dc1f 100644 --- a/src/metrics/abc/ruby.rs +++ b/src/metrics/abc/ruby.rs @@ -26,7 +26,10 @@ use crate::*; // `Call3` nodes and are counted as branches like any other call. // - Conditions: comparison and equality operator tokens emitted inside // `binary` (`==`, `!=`, `===`, `<`, `>`, `<=`, `>=`, `<=>`, -// `=~`, `!~`), plus the control-flow arms that the Fitzpatrick rules +// `=~`, `!~`) — the `binary` parent is a gate, not a description, since +// 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 // `?` ternary marker, plus `Rescue` (the rescue clause) and rescue // modifiers. `if` / `unless` themselves are not counted (the head @@ -230,27 +233,29 @@ impl Abc for RubyCode { Call | Call2 | Call3 | Call4 | Super | Yield | Yield2 => { stats.branches += 1.; } - EQEQ | BANGEQ | EQEQEQ | LTEQ | GTEQ | LTEQGT | EQTILDE | BANGTILDE | Else | Elsif - | When | QMARK | Rescue | RescueModifier | RescueModifier2 | RescueModifier3 => { - stats.conditions += 1.; - } - // `<` and `>` are comparisons only inside a `binary` node. The - // same `<` token also spells a class's superclass clause - // (`class Foo < Bar`, parent `superclass`) and an operator - // method's name (`def <(other)`, parent `operator`), neither of - // which is a condition — the positive `binary` guard is the - // polarity Rust / Go / C / C++ already use, and stays correct - // for any future non-comparison `<` (#1280). `a < b` emits the - // `Binary2` alias; the two siblings are listed defensively per - // grammar-dispatch §1. `<<` is the distinct `LTLT` token and - // heredoc openers are their own tokens, so neither is affected. - LT | GT + // A comparison / equality token is a condition only inside a + // `binary` node. Every one of them doubles as a *method name* + // in a `def` (`def <(other)`, `def ==(other)`, …), where the + // grammar parents it under `operator`; `<` additionally spells + // a class's superclass clause (`class Foo < Bar`, parent + // `superclass`). Neither is a condition — the positive + // `binary` guard is the polarity Rust / Go / C / C++ already + // use, and stays correct for any future non-comparison + // spelling (#1280). `a < b` emits the `Binary2` alias; the two + // 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 if ancestors .parent(node) .is_some_and(|p| matches!(p.kind_id().into(), Binary | Binary2 | Binary3)) => { stats.conditions += 1.; } + Else | Elsif | When | QMARK | Rescue | RescueModifier | RescueModifier2 + | RescueModifier3 => { + stats.conditions += 1.; + } // 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 b3a48a9a9dcb111b365826afbe711f0932662c26 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 9 Aug 2026 14:45:02 -0700 Subject: [PATCH 14/16] docs: restore two doc comments lost during the batch replace_symbol_body dropped days_between's /// contract (whole days, clamped at zero so a clock-skewed commit reads as today), leaving only the implementation note about saturation. The C# branch counter lost its lead line the same way, so an arm-specific note read as the function's description while Java's sibling kept its own. --- src/metrics/abc/csharp.rs | 2 ++ src/vcs/stats.rs | 3 +++ 2 files changed, 5 insertions(+) diff --git a/src/metrics/abc/csharp.rs b/src/metrics/abc/csharp.rs index e908c1285..5ad14bb07 100644 --- a/src/metrics/abc/csharp.rs +++ b/src/metrics/abc/csharp.rs @@ -148,6 +148,8 @@ fn csharp_count_token_assignment(node: &Node, stats: &mut Stats) -> bool { true } +// Counts branch tokens: every invocation, `new` allocation, and +// constructor delegation. // `ConstructorInitializer` is the `: base(…)` / `: this(…)` delegation on a // constructor — a call by Fitzpatrick's rule, and the C# spelling of the // shape Java and Groovy count (#1279). Unlike the invocation kinds it diff --git a/src/vcs/stats.rs b/src/vcs/stats.rs index 5a5e56d94..b06acdf6f 100644 --- a/src/vcs/stats.rs +++ b/src/vcs/stats.rs @@ -335,6 +335,9 @@ impl Accumulator { } } +/// Whole days between an earlier timestamp and `now`, clamped at zero so +/// a future-dated commit (clock skew) reads as "today" rather than a +/// negative age. fn days_between(earlier: i64, now: i64) -> u32 { // Saturating for the same reason the window boundaries are // (`options::window_boundary`, #1271): `earlier` is a committer From 1de8a3b1c73fd27ff5244e3657907c662e0523b9 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 9 Aug 2026 14:45:02 -0700 Subject: [PATCH 15/16] docs(lessons): merge four findings into lessons 34, 59, 84, 89 Four merges, no new entries and no renumbering: the polarity of a grammar-role gate (#1274, #1280) into 89, a report's site list not being a census (#1271) into 59, a false claim about a sibling language holding a wrong expected value (#1278) into 84, and an alias that is unreachable without being hidden (#1268) into 34. --- docs/development/lessons_learned.md | 53 ++++++++++++++++++++++++++--- 1 file changed, 49 insertions(+), 4 deletions(-) diff --git a/docs/development/lessons_learned.md b/docs/development/lessons_learned.md index 3a782a5e8..c9a3bad7f 100644 --- a/docs/development/lessons_learned.md +++ b/docs/development/lessons_learned.md @@ -1339,7 +1339,11 @@ a drift-marker assertion (`!ast_has_kind_id(&parser, Lang::HiddenVariant as u16)`) whose message names the hidden rule and demands replacement on drift. A hidden-rule variant without a drift marker is an invisible promise: it looks like -coverage and protects nothing observable today. +coverage and protects nothing observable today. The underscore +identifies a hidden rule; it does not establish that a non-underscore +alias is *reachable*. When the name is ordinary, probe every context the +construct appears in before concluding the variant is live — the remedy +is the same drift marker either way. The parser flattens hidden rules away, so a defensive arm listing one is dead code today and a correctness promise *if* a future grammar revision @@ -1358,6 +1362,16 @@ the same), and `Php::String3` (the `_string` hidden supertype). Each maps to a name beginning with `_`, confirming the heuristic. The remediation keeps the arm and pins the absence. +**An alias with an ordinary name that the grammar never emits** (#1268). +`Bash::TernaryExpression2` maps to `"ternary_expression"` with no leading +underscore, so the heuristic above answers "not hidden". Across all eight +arithmetic contexts the grammar admits — `$(( … ))`, a bare `(( … ))` +statement, a c-style `for ((…))` header, `let`, an array subscript, an +`if (( … ))` condition, a `declare -i` initializer, and a plain +assignment — the parser emits only the unsuffixed kind. The defensive arm +stays and carries the same `ast_has_kind_id` marker; what changed is that +identifying the case took a probe rather than a grep. + --- ## 35. Two predicates classifying the same node must agree @@ -2159,7 +2173,10 @@ the bug.** A site that arrives at the same wrong answer by different code does not match the pattern you are grepping for, and consolidating the sites that *do* match converts one surface's bug into a divergence between surfaces — a worse failure, and one no existing parity test -covers. +covers. A report's list of affected sites is a starting point, not a +census: #1271 named five window-boundary subtractions, and a sweep for +the quantity found a sixth in `BlameWalk::new` and a seventh in +`days_between`, whose shape matched no search for the reported one. The cost is not the duplication but *omission by default*: a newly added language — or a sibling cloned from a template predating the rule — @@ -3066,6 +3083,15 @@ climbs remained; the real figure was higher. `loc.sloc` cap that gates the build, and anyone budgeting a file against that cap from the comment would get it wrong. +**A claim about a sibling language held a wrong number in place** +(#1278). A Go ABC test asserted that an initialized `var` declaration is +not counted, "matching the Rust/Java rule for `let` / `int y = 1`". +Measured: Rust and Java each score one assignment for exactly that form, +so the rationale was backwards — and load-bearing, because the expected +value it justified was wrong too and had been for as long as the comment +stood. Prose explaining *why* a number is what it is has to be checked +together with the number, not after it. + --- ## 85. Coverage measures execution, not discrimination @@ -3295,8 +3321,12 @@ everything else, so the inputs that move are exactly the ones no one enumerated and no test covers. Before consolidating, enumerate the node kinds the container can actually hold, and decide the leftovers deliberately: adopting the shared answer is usually right, but it is a -decision, not a refactor. (cf. lesson 59 for why you are consolidating, -and lesson 65 for the structural inverse.) +decision, not a refactor. The same choice exists when you author the +filter rather than consolidate one: gating a token that serves several +grammatical roles *positively* — require the one parent that means what +you are counting — is closed and fails safe, while denying the roles you +thought of is open and admits the ones you did not. (cf. lesson 59 for +why you are consolidating, and lesson 65 for the structural inverse.) The direction of the change is what hides it. A positive filter is closed — a grammar that starts emitting a new kind silently scores zero, @@ -3323,4 +3353,19 @@ through the same helper — `int f(int a,,)` gives 2 and answering differently. Recording that in the arm's comment is what stops the next reader from filing it as a regression. +**One token, several grammatical roles — a denylist admits the role +nobody listed** (#1274, #1280). A bare `<` is a comparison, and in Java +and Groovy it also delimits generic type arguments and type +*parameters*. The gate denied `type_arguments` only, so every generic +*declaration* — `class Gen`, ` void m()` — scored two phantom ABC +conditions, and Groovy has a third form, `method_type_parameters`, that +a denylist must name separately. Both were rewritten to require a +`binary_expression` parent. Ruby then arrived with two roles nobody +would have enumerated in advance — a superclass clause +(`class Foo < Bar`) and an operator-method name (`def <(other)`) — and +the positive gate covered both without being told about either. **C#, +Kotlin and the JS family still carry the denylist form**, so they are +correct only for the roles someone thought of: the state Java and Groovy +were in before #1274. + --- From 3427ea1e87ef4d69b82c3b62df9b3ffd3cd0ae31 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 9 Aug 2026 15:05:08 -0700 Subject: [PATCH 16/16] test: share one ast_has_kind_id built on preorder Codecov flagged the PR's only patch gap as a partial branch in cyclomatic.rs: the drift-marker helper paired node.child(i) with an `if let Some(..)` whose None arm is unreachable for i < child_count(). Three modules each carried a byte-identical copy of that walk, so the same unreachable arm was measured three times. Replace all three with one test_support helper over Node::preorder, which has no Option to discriminate. Net -43 lines and the partial is gone by construction. Behaviour is unchanged: preorder visits the same nodes, and `any` short-circuits exactly where the early `return true` did. --- src/checker.rs | 20 +------------------- src/metrics/abc.rs | 23 +---------------------- src/metrics/cyclomatic.rs | 23 +---------------------- src/test_support.rs | 17 +++++++++++++++++ 4 files changed, 20 insertions(+), 63 deletions(-) diff --git a/src/checker.rs b/src/checker.rs index 2c2f4bcd4..fe917aa59 100644 --- a/src/checker.rs +++ b/src/checker.rs @@ -780,7 +780,7 @@ mod tests { BashParser, JavascriptCode, JavascriptParser, MozjsParser, PhpParser, TsxParser, TypescriptParser, }; - use crate::test_support::for_each_node_with_chain; + use crate::test_support::{ast_has_kind_id, for_each_node_with_chain}; use std::fmt::Write as _; use std::path::PathBuf; @@ -917,24 +917,6 @@ mod tests { // ===== JS-family `is_string` regression tests (issue #283) ===== - // Walk the AST and return true iff any node has `kind_id == target`. - // Used to confirm an alias kind actually surfaces in a real parse - // before asserting it routes through `is_string`. - fn ast_has_kind_id(parser: &P, target: u16) -> bool { - let mut stack = vec![parser.root()]; - while let Some(node) = stack.pop() { - if node.kind_id() == target { - return true; - } - for i in (0..node.child_count()).rev() { - if let Some(c) = node.child(i) { - stack.push(c); - } - } - } - false - } - // For each language, count nodes whose kind_id is exactly `target` // *and* simultaneously match `is_string`. A non-zero result proves // both that the alias appears in the parse and that the checker diff --git a/src/metrics/abc.rs b/src/metrics/abc.rs index 8e2533c41..d095dd90e 100644 --- a/src/metrics/abc.rs +++ b/src/metrics/abc.rs @@ -444,7 +444,7 @@ implement_metric_trait!(Abc, PreprocCode, CcommentCode); )] mod tests { use crate::test_support::{ - check_func_space_only_shim, check_metrics_only_shim, metrics_verbatim, + ast_has_kind_id, check_func_space_only_shim, check_metrics_only_shim, metrics_verbatim, }; use crate::traits::ParserTrait; @@ -470,27 +470,6 @@ mod tests { assert_eq!(deepest.metrics.abc.conditions(), decisions); } - // Walk the AST and return true iff any node has `kind_id == target`. - // Used as a drift marker for hidden-rule kind ids: a passing - // `!ast_has_kind_id(...)` assertion proves the kind is unreachable - // at the pinned grammar version, so a future grammar bump that - // promotes the hidden rule to a concrete emitted node will fail - // loudly instead of silently changing the metric (lesson 34). - fn ast_has_kind_id(parser: &P, target: u16) -> bool { - let mut stack = vec![parser.root()]; - while let Some(node) = stack.pop() { - if node.kind_id() == target { - return true; - } - for i in (0..node.child_count()).rev() { - if let Some(c) = node.child(i) { - stack.push(c); - } - } - } - false - } - /// Regression for #227: a `Stats::default()` that never sees an /// observation must not leak the `f64::MAX` sentinel for /// `assignments_min`, `branches_min`, or `conditions_min`. All diff --git a/src/metrics/cyclomatic.rs b/src/metrics/cyclomatic.rs index 5598c5f80..a0236bed4 100644 --- a/src/metrics/cyclomatic.rs +++ b/src/metrics/cyclomatic.rs @@ -571,7 +571,7 @@ mod typescript; clippy::too_many_lines )] mod tests { - use crate::test_support::check_metrics_only_shim; + use crate::test_support::{ast_has_kind_id, check_metrics_only_shim}; use super::*; @@ -4706,27 +4706,6 @@ g() { assert!(!ast_has_kind_id(&parser, Bash::TernaryExpression2 as u16)); } - // Walk the AST and return true iff any node has `kind_id == target`. - // Used as a drift marker for an alias kind id: a passing - // `!ast_has_kind_id(...)` assertion proves the kind is unreachable at - // the pinned grammar version, so a future grammar bump that starts - // emitting it fails loudly instead of silently changing the metric - // (lesson 34). - fn ast_has_kind_id(parser: &P, target: u16) -> bool { - let mut stack = vec![parser.root()]; - while let Some(node) = stack.pop() { - if node.kind_id() == target { - return true; - } - for i in (0..node.child_count()).rev() { - if let Some(c) = node.child(i) { - stack.push(c); - } - } - } - false - } - #[test] fn bash_nested_arithmetic_ternary_counts_each_occurrence() { // Each ternary is its own decision point (#1268). diff --git a/src/test_support.rs b/src/test_support.rs index 695174130..574867784 100644 --- a/src/test_support.rs +++ b/src/test_support.rs @@ -307,3 +307,20 @@ pub(crate) fn for_each_node_with_chain( } visited } + +/// Walks `parser`'s tree and reports whether any node has `kind_id == +/// target`. +/// +/// The drift marker behind lesson 34. A passing `!ast_has_kind_id(…)` +/// proves an enum variant is unreachable at the pinned grammar, so a +/// defensive dispatch arm listing it is an explicit promise rather than +/// silent dead code; a bump that starts emitting the kind fails the +/// assertion instead of quietly changing a metric. +/// +/// `preorder` rather than a hand-rolled index walk: three modules each +/// carried their own copy pairing `node.child(i)` with an `if let +/// Some(..)` whose `None` arm is unreachable for `i < child_count()`, +/// which Codecov reported as a partial branch in each of them. +pub(crate) fn ast_has_kind_id(parser: &P, target: u16) -> bool { + parser.root().preorder().any(|n| n.kind_id() == target) +}