fix(npm): decide npm/npa emission by space kind alone - #1206
Merged
Conversation
#1197 declared one emission rule for the object-oriented member metrics -- containers and the file unit carry the block, a function space never does -- but enforced it only for the ten languages it routed through `opens_member_scope`. The other seven kept enabling from their own grammar node kinds and disagreed with it both ways: a Go or Rust `struct` declared inside a function body put the block on that function space (38 of them in the serde corpus alone, mostly `#[test]` functions), while a C++ namespace, and any file root whose only container sat inside a function, carried none. In that last case the counts reached the root's `_sum` fields and nothing emitted them, so suppressing the function-space block on its own would have deleted them from output rather than relocating them. Replace `is_class_space` with the space's own kind, recorded once per space by the walker beside `compute_wmc`, and delete the eighteen `enable_for_member_scope` call sites, the twelve direct enables and `opens_member_scope` itself. There is no per-language surface left to deviate on. `Npm::HAS_MEMBERS` / `Npa::HAS_MEMBERS` keep a grammar with no class-shaped construct anywhere (C, Bash, Lua, Perl, Tcl, iRules) out of the output entirely, rather than giving every shell script an all-zero block -- the file unit is a member scope like any other. `wmc` reaches the same place via a no-op `compute` that records no kind; the const is the equivalent for a metric whose kind comes from the walker. No metric value changed: the counts always rolled up through every enclosing space regardless of which one serialized them. Thresholds are unaffected -- `bca check` reads `metrics.npm` through `MetricScope`, which never consulted the emission gate. Go keeps no container `SpaceKind`. Giving it one would create new nested spaces in every Go file and re-partition every per-space metric, which is a major-version change to the space tree rather than a bug fix. Its consequence is now documented instead: since `npm` / `npa` are reported only at a Go file's root and `bca check` gates them on container spaces, no `npm` or `npa` limit can fire on Go source. Fixes #1203
Follow-ups from the simplify-rust, rust-optimize and review passes over 91ac421. No behaviour change. - `set_space_kind`'s doc still described the first draft, which seeded the kind in `FuncSpace::new`; it is called from the walker's finalize step beside `compute_wmc`. - Share one `implement_metric_trait!` arm between the `Npm` and `Npa` no-op impls instead of two identical bodies, mirroring the existing `@code_and_chain_taking` helper. - Drop `rust_count_struct_attrs`'s `if attrs > 0` guard and the comment justifying it. It existed to stop a fieldless marker struct enabling the block; with emission decided by kind, adding zero is already a no-op. - Cover Mozcpp in `container_scope_tests`. It was the one language with a real `Npm` / `Npa` impl left out, and the one that owns no file extension, so nothing else reaches it — `cpp_mozcpp_parity` compares metric values and would not see it lose both blocks. - Rename `container_counts_survive_the_narrowed_enable`, which was named for a mechanism this change deleted. - Replace the Go fixture's `\t` escapes with spaces. - Record in the changelog that the Python `to_sarif` binding was silently dropping namespace-scoped `npm` / `npa` offenders that the CLI gated, because it reads the JSON and the key was absent.
`each_elixir_module_counts_only_its_own_members` contradicted itself: the name promised a module's own members, while its first assertion pinned `class_nm_sum() == 3` — a figure the test's own comment says includes the nested `Inner`'s method through the roll-up. What the value actually establishes is that no module absorbs a nested one's methods twice; a double count reads 4. Name it that way.
`a_language_with_no_member_construct_emits_neither_block` and `a_type_declared_inside_a_function_reaches_the_root_rollup` build a case list whose every entry is feature-gated, then call `assert_fixtures_present` on it. Under a feature set that enables no listed language the list is empty and that helper fires, so the tests failed as false positives on `--no-default-features --features rust,typescript` — the minimal-langs CI leg. `make pre-commit` runs `--all-features` and structurally cannot see this. Gate each test on the union of its own fixtures' features, so it is hidden rather than failing. Verified by removing the gate and re-running that exact feature set. Also correct the module doc: the space kind is recorded by the walker's finalize step, not in `FuncSpace::new` as an earlier draft did.
The blast-radius figures came from a census over serde plus DeepSpeech/native_client, but the sentence holding them claims the integration corpora as a whole. Re-measured from the committed snapshot delta itself: 39 `npa` blocks removed across 10 serde files (was 38 in 9), 1,214 file roots gaining one (was 1,252), 1,332 C++ namespaces (was 1,337). Three claims that were false rather than imprecise: - `STABILITY.md` still carried #1197's "outside Rust and Go" caveat, so the shape contract described a rule this change had already made unconditional. - The book called Go "the one supported language whose space tree has no container kind". Bash, C, Lua, Perl, Tcl and iRules have none either; Go is the one that emits `npm` / `npa` without having one. - A new docstring in `test_metrics_select.py` claimed `wmc` is elided from a class-less unit root. It is not — `class_interface_compute` records `space_kind = Unit`, and the web fixture shows `wmc` present there. And a stale comment: `@code_and_chain_taking` no longer serves `Npa` / `Npm`, which route through `@code_and_chain_taking_memberless` for `HAS_MEMBERS`. Reaching for the old arm would silently restore the all-zero file-root block.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1206 +/- ##
==========================================
+ Coverage 98.26% 98.27% +0.01%
==========================================
Files 276 276
Lines 71583 71501 -82
Branches 71153 71071 -82
==========================================
- Hits 70339 70266 -73
+ Misses 823 820 -3
+ Partials 421 415 -6
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.
Fixes #1203.
What was wrong
#1197 declared one emission rule for the object-oriented member
metrics — containers and the file
unitroot carry thenpm/npablock, a function space never does — but enforced it only for the ten
languages it routed through
opens_member_scope. The other seven keptenabling from their own grammar node kinds and disagreed with it in
both directions.
A kind × emission census over
serde+DeepSpeech+pdf.js(14,438 files) before this change:
Four different
unitrows is the clearest single sign the rule was aconvention rather than an invariant.
Two corrections to the issue's resolution plan
The plan's load-bearing finding was half right.
is_class_spacedoes gate emission only — read in exactly one place,
is_disabled()→wire.rs:354-356, with neithermergenorcompute_sumconsultingit. But the conclusion drawn from that, that clearing the flag on a
function space keeps the parent roll-up, is false for the shapes the
issue reports:
Go enables
npmat the root only for a file with a directMethodDeclarationchild, and Rustnpaonly for a module-scopestruct. In 4 of the 9 affectedserdefiles the root carries noblock, so a plain clear would have deleted the counts from output
rather than relocating them.
The plan also recommended patching Go and Rust only. That would
have left the namespace and file-root under-emission in place.
What this does instead
Emission is a pure function of the space's kind, for every language.
is_class_spacebecomesspace_kind: SpaceKind, recorded once perspace by the walker beside
compute_wmc;opens_member_scope, bothenable_for_member_scopemethods, 18 call sites and 12 per-languageenables are deleted. Net −106 lines of non-comment production code.
After — one row per kind:
Npm::HAS_MEMBERS/Npa::HAS_MEMBERSis the one addition beyond theplan, and it was necessary: a file unit is a member scope like any
other, so kind-driven emission alone gave every shell script and
.cfile an all-zero block. Those 527 units are the non-OO languages held
out by it.
wmcreaches the same place through a no-opcomputethatrecords no kind.
No metric value changed — the counts always rolled up through every
enclosing space regardless of which one serialized them. Thresholds are
unaffected:
bca checkreadsmetrics.npmthroughMetricScope,which never consulted the gate.
Scope decision: Go container
SpaceKind— declinedAdding
type … struct/type … interfacetoGoCode::is_func_spaceand
get_space_kindcreates new nested spaces in every Go file,re-partitioning every per-space metric and changing the tree shape for
every consumer. That is a major-version design change, not a bug fix.
Documented instead, in the metrics guide and the thresholds recipe: a
Go file's
npm/npalive on the unit root,bca checkgates themon container spaces, therefore no
npmornpathreshold can fireon Go source.
A pre-existing front-end divergence closes as a side effect
MetricScope::ContaineradmitsNamespace, sobca checkhas alwaysflagged namespace-scoped offenders (verified:
N: npm = 4 (limit 3)).But the Python
to_sarifbinding walks serialized JSON and skips ametric whose key is absent (
sarif.rs:508) — and no namespace carriedthe key, so it silently dropped every one, contradicting its own claim
to match the CLI gate. SARIF output may now gain namespace-scoped
findings. Noted in the changelog.
Verification
make pre-commitgreen.container_scope_testsextended from 10 languages to all 17,including Mozcpp — which owns no file extension and so had no
coverage at all.
.claude/rules/testing.md;reverting is unavailable because the fix deletes the old path.
Reinstating the defect fails 3 tests, deleting the
npaseed fails4, flipping
HAS_MEMBERSfails exactly 1.a count rather than a key — confirmed inert by running a
pre-change binary on the new fixture:
Outer3/2/1,Inner1/0,Sibling1, bit-identical.added keys are exactly
npm/npaand their sub-keys, removalsexactly 39 whole
npablocks on Rust function spaces, no othermetric key touched on either side. Submodule bumped to
big-code-analysis-output@a23ea6a8in the same commit as the fix.Review passes
simplify-rust,rust-optimize,reviewandaudit-testseach ranover the diff; their fixes are the follow-up commits. The last of them
caught a CI break the others missed: two new tests called
assert_fixtures_presenton feature-gated case lists that go emptyunder
--no-default-features --features rust,typescript, theminimal-langs leg.
make pre-commitruns--all-featuresandstructurally cannot see that class of bug.