fix(npm): emit npm/npa on member scopes, not every space - #1205
Merged
Conversation
`Npm` and `Npa` enabled themselves from `Checker::is_func_space`, which answers "does this node open a space", not "is this a container that owns methods and attributes". Ten languages emitted an all-zero block on the whole-file unit root, and C#, JavaScript, MozJS, TypeScript, TSX, PHP and Ruby emitted one on every ordinary method too. #1184 then gave Kotlin `get()`/`set()`/`init {}`, Java and Groovy `static {}` and the JS-family `class_static_block` their own function spaces, so those grew a block the plain method beside them did not have -- the symptom this issue reports. The enable now goes through `metrics::opens_container_space`, which reuses `MetricScope::Container` rather than re-deriving it. That is the same set of space kinds `bca check` and the SARIF export already gate these metrics on (#969), so emission and thresholding are one rule. Only the ten `is_func_space`-shaped sites move. Python, Rust, C, C++, Mozcpp, Go, Objective-C and Elixir already gate on node kind; their gates are deliberate and documented in place. No metric value changes: `is_class_space` gates serialization only, and `merge` sums the roll-up regardless. 3,765 all-zero blocks disappear from 278 integration snapshots, none of them C/C++/Rust. The new tests serialize a whole `FuncSpace` rather than reading `space.metrics`, because both existing surfaces are blind to this gate -- `check_metrics` hands back the ungated struct and the `insta` snapshots go through `serialize_via_wire!`. Perturbing the helper back to `is_func_space` fails exactly the two new negative tests and nothing else in the 3,312-test lib suite, which is why the defect shipped. `src/metrics/npa/php.rs`'s baselined `halstead.effort` moves +0.7% on the call-site rename; the baseline is refreshed in the same commit. Fixes #1197
The three-line enable block was repeated verbatim at 16 sites across `npm` and `npa`. `Stats::enable_for_container` names the idempotence invariant once instead of restating `&& stats.is_disabled()` at every call, and checks the cheap flag before the two predicate calls rather than after. Fewer tokens in those bodies also moves three baselined metrics the right way: `npa/php.rs`'s `halstead.effort` drops 52615 -> 49514, back under the 50000 limit, and `npa/csharp.rs` and `npa/kotlin.rs` fall off the baseline entirely as their `cognitive` clears 15. Test fixes in the same pass: the space name is modelled as `Option` rather than defaulted, since the verbatim root legitimately has none and a defaulted `""` would match nothing silently; and the #1184 fixtures are looked up by language instead of by index into `FIXTURES`.
`opens_container_space` drops from `pub(crate)` to private: every caller is a descendant of `crate::metrics`. (`average` beside it keeps `pub(crate)` — `wire::metrics` and `output::funcspace_row` do reach it from outside.) In the container-scope tests, `only_space` and `summary` replace three copies of the "every space as (kind, name)" diagnostic and two copies of the exactly-one-match assertion. The `kind != Unit` filter goes with them: it guarded a collision that became impossible once the space name was modelled as `Option`, since the verbatim root has none. cargo clippy -W clippy::pedantic reports zero warnings for the crate, so there was no lint triage to apply.
Two independent reviewers caught that the first cut of #1197 narrowed too far. `MetricScope::Container` excludes `SpaceKind::Unit`, so gating on it deleted the whole-file `npm`/`npa` roll-up as well as the all-zero function-space noise -- 400 non-zero values across the integration corpus, e.g. `php/traits_enums.php`'s root `class_npm_sum: 7`. The CHANGELOG claimed nothing but zeros had been removed; that was wrong, and it was the sentence a reader would use to size the upgrade. It also left the three OO metrics disagreeing about the same root: `wmc` is `MetricScope::Container` too, but its rule is `!matches!(kind, Function | Unknown)` -- it drops function spaces and keeps the roll-up. That rule is the one #1197 actually asks for, since the reported symptom is a `<get>` space carrying a block its sibling method does not. `SpaceKind::is_member_scope` now states it once and `wmc`, `npm` and `npa` all read it, so they cannot drift apart again. Also from review: - `opens_member_scope` classifies through `is_func_space_with_code` / `get_space_kind_with_code`, the pair the walker itself uses. The byte-less forms disagree for Elixir, whose `defmodule` is a `Class` only to the source-aware getter, so routing Elixir through the helper would have silently emitted nothing for every module. - The test file is feature-gated per language and calls `assert_fixtures_present`. It previously panicked under the `minimal-langs` CI feature set instead of compiling out. - New coverage: the file-root roll-up in both directions (a container-only rule fails it), C#'s expression-bodied property and accessor-less indexer, and the interface-side counters. - Corrected doc claims that were false for Go and Rust, which set the flag from their own node kinds and still enable a file root the shared rule would not; the book's claim that CSV writes zeros (it writes the real values); and a doc that undercounted the affected languages. - STABILITY.md now states that which space carries which block is not part of the shape contract. - The Python binding test reads the root again and gained a companion asserting the absence on function spaces. `npa/php.rs`'s baselined `halstead.effort` moves 49514 -> 51275 on the two threaded arguments; baseline refreshed in the same commit.
`test_class_only_metrics_skip_function_spaces` used the `Hello.java` fixture, but Java's `is_func_space` never listed `method_declaration` -- a Java method carried no npm/npa block even before #1197, so asserting its absence held either way. The test passed for the wrong reason. JavaScript's `is_func_space` did list every function form, so the three function spaces in the new inline fixture -- a class method, a static block, and a top-level function -- each really carried an all-zero block in 2.0.x. Verified by rebuilding the extension with the pre-fix predicate: the test fails on `<static-init>`, and passes once restored. Also asserts the class space still carries both blocks, and pins the space counts, so a fixture that stopped parsing cannot make the loop vacuous.
Review found the Go/Rust caveat stated backwards. Probed: for both
languages a `struct` declared inside a function puts the block on that
*function* space and leaves the unit root without one — the opposite of
what the shared rule predicts, and the opposite of what the docs said.
Corrected in the book, the two `Stats` doc comments and STABILITY.md,
which each carried an unqualified "never on a function space".
`{#oop-emission-scope}` sat on a paragraph, where mdBook renders it as
literal text and leaves the `[above](#oop-emission-scope)` link with no
target. It is now a real heading, matching the book's convention that
every explicit anchor sits on one.
Also repoints a doc link at the renamed `function_spaces_emit_neither`
— `#[cfg(test)]` keeps rustdoc's `-D warnings` from catching it — and
merges two split `use crate::node::…` lines.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1205 +/- ##
==========================================
- Coverage 98.26% 98.26% -0.01%
==========================================
Files 276 276
Lines 71584 71583 -1
Branches 71154 71153 -1
==========================================
- Hits 70340 70339 -1
Misses 823 823
Partials 421 421
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
NpmandNpadecided whether to emit their serialized block by askingChecker::is_func_space(node)— "does this node open a space" — ratherthan "is this a scope that owns methods and attributes". The two
questions had drifted apart, and #1184 made the gap visible.
Fixes #1197.
The defect is wider than the issue reports
Measured with
bca metricsbefore the fix:C.kt/C.java/C.groovyC.js/C.ts/C.cs/C.rb/C.phpC#, JavaScript, MozJS, TypeScript, TSX, PHP and Ruby list ordinary
methods in
is_func_space, so every method already carried an all-zeroblock. In the JS family
class_static_blockwas therefore not a newinconsistency — just one more instance of a pre-existing one. The
inconsistency the issue shows is real for Kotlin, Java and Groovy, whose
methods are
is_funconly.Ten
Npm/Npaimpls used theis_func_spaceshape and all ten moved.Python, C++, Mozcpp, Objective-C and Elixir gate on their own node kinds
and are correct; Rust and Go also gate on their own kinds but are not
correct — see the follow-up below.
MetricScope::Containeris the wrong predicateThe issue proposes gating on "is this a container space", and
MetricScope::Containerlooks ready-made. It is not: it excludesSpaceKind::Unit, so gating on it also deletes the whole-fileroll-up — 400 non-zero values across the integration corpus, including
php/traits_enums.php's rootclass_npm_sum: 7.It would also have left the three OO metrics disagreeing about the same
root, because
wmccarries the sameMetricScope::Containerbut gateson
!matches!(kind, Function | Unknown)— dropping function spaces andkeeping the roll-up. That is the rule #1197 actually wants, since the
reported symptom is a function space carrying a block its sibling method
does not.
The first two commits here implement the container-only version; review
caught it and
042a258ccorrects it. The rule now lives once, asSpaceKind::is_member_scope, read bywmc,npmandnpaalike.Result
6,974 blocks removed across 213 integration snapshots. 6,969 were
entirely zero. The five that were not belong to a function lexically
containing a class — a PHP
new class { … }, a JavaScript class insidea callback — where the block was that nested class's roll-up; it remains
on the class's own space and in the file-root total, the position
wmchas always omitted.
No metric value changed:
is_class_spacefeeds onlywire::CodeMetrics::from, nevermerge.Notes for the reviewer
metrics::opens_member_scopeclassifies through the source-awareis_func_space_with_code/get_space_kind_with_code, the pair thewalker itself uses. The byte-less forms disagree for Elixir, whose
defmoduleis aClassonly to the source-aware getter — routingElixir through the helper with the plain forms would silently emit
nothing for every module, with no test failure.
FuncSpace, because both existingsurfaces are blind to this gate by construction:
check_metricshandsback the ungated
Stats, and theinstasnapshots go throughserialize_via_wire!, which bypasses theOption. That blindness iswhy the defect shipped — perturbing the predicate back fails only the
new tests out of 3,313.
the_file_root_keeps_its_rollup, widening back tois_func_spacefails
function_spaces_emit_neither.dekobon/big-code-analysis-output@f7579131,already pushed.
.bca-baseline.tomlmoves one entry:npa/php.rs'shalstead.efforton the two threaded arguments. Two other entries dropped off entirely
when the duplicated enable block was folded into a method.
Follow-ups filed, not folded in
SpaceKindat all, so it enablesnpmonthe file root and, via
StructType/InterfaceType, on functionspaces; Rust
npadoes the same for astructinside a function.Fixing needs a decision about what a Go
npmmeans before it needscode.
.claude/rules/grammar-dispatch.md§7 does not list theis_func_space↔get_space_kindpair, which now decides serializedshape, nor the
_with_codehazard above.