From f52aa84dbf7e343a8ec269f091c7173a1552b8d0 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Mon, 3 Aug 2026 20:00:09 -0700 Subject: [PATCH 1/6] fix(npm): enable npm/npa on container spaces only `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 --- .bca-baseline.toml | 2 +- CHANGELOG.md | 25 ++ big-code-analysis-book/src/metrics.md | 14 +- big-code-analysis-py/src/types_codegen.rs | 14 +- .../tests/test_metrics_select.py | 24 +- src/metrics/container_scope_tests.rs | 369 ++++++++++++++++++ src/metrics/mod.rs | 40 ++ src/metrics/npa.rs | 23 +- src/metrics/npa/csharp.rs | 2 +- src/metrics/npa/groovy.rs | 2 +- src/metrics/npa/kotlin.rs | 2 +- src/metrics/npa/php.rs | 3 +- src/metrics/npa/ruby.rs | 2 +- src/metrics/npm.rs | 23 +- src/metrics/npm/csharp.rs | 2 +- src/metrics/npm/groovy.rs | 2 +- src/metrics/npm/kotlin.rs | 3 +- src/metrics/npm/php.rs | 2 +- src/metrics/npm/ruby.rs | 2 +- tests/repositories/big-code-analysis-output | 2 +- 20 files changed, 521 insertions(+), 37 deletions(-) create mode 100644 src/metrics/container_scope_tests.rs diff --git a/.bca-baseline.toml b/.bca-baseline.toml index 5d5ff545c..dcacfead1 100644 --- a/.bca-baseline.toml +++ b/.bca-baseline.toml @@ -1053,7 +1053,7 @@ value = 15.0 path = "src/metrics/npa/php.rs" qualified = "PhpCode::compute" metric = "halstead.effort" -value = 52227.844233305164 +value = 52614.71715355187 [[entry]] path = "src/metrics/npa/python.rs" diff --git a/CHANGELOG.md b/CHANGELOG.md index a4f3034ae..9d178829a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -766,6 +766,31 @@ for historical reference. ### Fixed +- **Serialized shape.** `npm` and `npa` are now emitted only on + *container* spaces — class, struct, trait, impl, namespace, interface + (#1197). They were enabled 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 therefore 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 as + well; #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 that the plain method beside + them did not have. The enable now reuses `MetricScope::Container`, 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 + rather than two. Kotlin, Java, Groovy, JavaScript, MozJS, TypeScript, + TSX, C#, PHP and Ruby are affected; Python, Rust, C, C++, Mozcpp, Go, + Objective-C and Elixir already gated on node kind and do not move. + **This changes serialized JSON / YAML / TOML / CBOR shape versus + `2.0.x`** for the seven languages that emitted on methods, and for all + ten at the file root: a consumer reading `metrics.npm` off a function + space or the unit root now finds the key absent. Every removed block + was all zeros, no metric *value* changed, and container counts — + including the `_sum` roll-up the whole tree still accumulates — are + unchanged. The CSV projection is a fixed-column format and is + unaffected: it writes the `npm.*` / `npa.*` columns on every row + regardless. - **Metric drift.** ABC `conditions` moves wherever a comment sits inside a ternary (#1181). Slots are now addressed by grammar field rather than by neighbouring token or fixed index, which fixes two opposite errors diff --git a/big-code-analysis-book/src/metrics.md b/big-code-analysis-book/src/metrics.md index df61b70f4..14b32d6c1 100644 --- a/big-code-analysis-book/src/metrics.md +++ b/big-code-analysis-book/src/metrics.md @@ -917,6 +917,16 @@ decides what counts as "public" (Java `public`, C# `public`, Rust `pub`, Python's "no leading underscore" convention, …) and what counts as "attribute" rather than "method". +NPA and NPM are emitted **only on container spaces** {#oop-emission-scope} +— `class`, `struct`, `trait`, `impl`, `namespace`, `interface`. A +function space and the whole-file `unit` root carry no `npa` / `npm` +block at all, because neither owns attributes or methods. This is the +same set of space kinds a `npa` / `npm` threshold is checked against +(see [Threshold scope](commands/check.md#threshold-scope)), so what +`bca metrics` emits and what `bca check` gates are one rule. The CSV +projection is the exception: it is a fixed-column format and writes the +`npa.*` / `npm.*` columns as zeros on every row, container or not. + ### How to read it NPA is a *direct* measure of encapsulation. Every public attribute @@ -946,7 +956,9 @@ As with NPA, big-code-analysis splits NPM by definition-site kind `interface_npm_sum`, `class_methods` (sum of *all* methods — public or not — across classes), `interface_methods`, `class_coa`, `interface_coa` (operation-accessibility *ratios*, not averages), -`total`, `total_methods`, and `coa`. +`total`, `total_methods`, and `coa`. It is emitted on the same +container spaces, and elided everywhere else, for the same reason +([above](#oop-emission-scope)). The language-specific `Npm` trait decides what counts as public — for example, Rust's `pub`, Python's leading-underscore convention, C++'s `public:` section — and folds together regular methods, diff --git a/big-code-analysis-py/src/types_codegen.rs b/big-code-analysis-py/src/types_codegen.rs index 57e0bad55..85c79652c 100644 --- a/big-code-analysis-py/src/types_codegen.rs +++ b/big-code-analysis-py/src/types_codegen.rs @@ -1056,19 +1056,27 @@ mod tests { /// never appear in the Rust fixture (disabled / not injected), so they /// are checked against a language that emits them and a synthetic /// `vcs` block respectively — otherwise their specs would be untested. + /// + /// `npm` / `npa` are read off the class space rather than the file + /// root: since #1197 they are emitted only on container spaces. #[test] fn class_only_and_vcs_specs_match_wire_json_keys() { use big_code_analysis::wire; - // Java emits wmc / npm / npa on a class. + // Java emits wmc at the file root, but npm / npa only on the + // container space itself (#1197) — read each where it lives. let value = analyze_to_value( "java", "public class Foo {\n public int a;\n public int m() { return 1; }\n}\n", ); let metrics = value["metrics"].as_object().expect("metrics"); assert_keys_match("WmcDict", &metrics["wmc"]); - assert_keys_match("NpmDict", &metrics["npm"]); - assert_keys_match("NpaDict", &metrics["npa"]); + + let class = &value["spaces"][0]; + assert_eq!(class["kind"], "class", "fixture's first child is the class"); + let class_metrics = class["metrics"].as_object().expect("class metrics"); + assert_keys_match("NpmDict", &class_metrics["npm"]); + assert_keys_match("NpaDict", &class_metrics["npa"]); // VcsDict: serialize a default wire::Vcs directly (with the two // optional fields populated so they appear in the key set, which we diff --git a/big-code-analysis-py/tests/test_metrics_select.py b/big-code-analysis-py/tests/test_metrics_select.py index d1eb0add9..af02dc439 100644 --- a/big-code-analysis-py/tests/test_metrics_select.py +++ b/big-code-analysis-py/tests/test_metrics_select.py @@ -41,6 +41,18 @@ def _metrics_keys(result: FuncSpaceDict) -> set[str]: return set(result["metrics"].keys()) +def _all_metrics_keys(result: FuncSpaceDict) -> set[str]: + """Return every metric-family key on the space tree, at any depth. + + ``npm`` and ``npa`` are emitted only on container spaces (#1197), so + a key that exists for a fixture need not exist at its root. + """ + keys = _metrics_keys(result) + for space in result.get("spaces", ()): + keys |= _all_metrics_keys(space) + return keys + + # ───────────────────────────────────────────────────────────────── # METRIC_NAMES module constant # ───────────────────────────────────────────────────────────────── @@ -77,17 +89,19 @@ def test_metric_names_constant_shape() -> None: def test_metric_names_round_trip_through_analyze() -> None: """Every name in ``METRIC_NAMES`` is accepted by ``metrics=``. - Class-only metrics (``npa``, ``npm``, ``wmc``) are elided from - a unit-level space when there is no class to attach them to — - use the Java fixture (which carries a class) for those so the - output key actually appears. + Class-only metrics (``npa``, ``npm``, ``wmc``) need a class to + attach to, so they use the Java fixture. ``npa`` and ``npm`` are + further emitted only on the container space itself (#1197), never + on the file root, so their keys are collected over the whole space + tree rather than off the top-level space. """ class_only = {"npa", "npm", "wmc"} + container_only = {"npa", "npm"} for name in bca.METRIC_NAMES: fixture = FIXTURES / ("Hello.java" if name in class_only else "hello.py") result = bca.analyze(fixture, metrics=[name]) assert result is not None, f"analyze returned None for metrics=[{name!r}]" - keys = _metrics_keys(result) + keys = _all_metrics_keys(result) if name in container_only else _metrics_keys(result) # ``mi`` and ``wmc`` are derived; their *direct* output key # may sit on a nested space (or, for ``wmc``, be ``"wmc"`` # at the unit root after the Java class merge). Their diff --git a/src/metrics/container_scope_tests.rs b/src/metrics/container_scope_tests.rs new file mode 100644 index 000000000..3bbf01f48 --- /dev/null +++ b/src/metrics/container_scope_tests.rs @@ -0,0 +1,369 @@ +//! Which spaces carry an `npm` / `npa` block in serialized output. +//! +//! These assertions cannot be written with `check_metrics`. That helper +//! hands back `spaces::CodeMetrics`, whose `npm` / `npa` fields are plain +//! structs, and the `insta` snapshots in `npm.rs` / `npa.rs` serialize +//! those structs through `serialize_via_wire!`, which bypasses the +//! `Option`. Both surfaces are blind to the emission gate by design — it +//! lives one layer out, in `wire::CodeMetrics::from`, and the only way to +//! observe it is to serialize a whole [`FuncSpace`] and look at the keys. +//! +//! That blindness is exactly how #1197 shipped: `Npm` and `Npa` enabled +//! themselves on `Checker::is_func_space`, which means "opens a space", +//! not "is a container". Ten languages therefore emitted an all-zero +//! block on the file root, and seven of them on every ordinary method +//! too; #1184 then added property accessors and `init` / `static` blocks +//! to the list, next to sibling methods that had none. + +use serde_json::Value; + +use crate::spaces::SpaceKind; +use crate::test_support::space_verbatim; +use crate::{LANG, MetricsOptions}; + +/// A serialized space flattened to the two things these tests assert on. +struct Emitted { + kind: SpaceKind, + name: String, + has_npm: bool, + has_npa: bool, +} + +/// Analyses `source` and flattens every space in the serialized tree, +/// root first. +/// +/// Serializing the [`FuncSpace`](crate::spaces::FuncSpace) rather than +/// reading `space.metrics` is the point: `metrics.npm` is always present +/// as a struct, and only the JSON key is gated. +fn emitted_spaces(lang: LANG, source: &str) -> Vec { + let space = space_verbatim(lang, source.as_bytes(), MetricsOptions::default()); + let value = serde_json::to_value(&space).expect("FuncSpace must serialize"); + let mut out = Vec::new(); + flatten(&value, &mut out); + out +} + +fn flatten(value: &Value, out: &mut Vec) { + let metrics = value["metrics"] + .as_object() + .expect("every space serializes a metrics object"); + out.push(Emitted { + kind: SpaceKind::from_serialized(value["kind"].as_str().unwrap_or_default()), + name: value["name"].as_str().unwrap_or_default().to_owned(), + has_npm: metrics.contains_key("npm"), + has_npa: metrics.contains_key("npa"), + }); + for child in value["spaces"].as_array().into_iter().flatten() { + flatten(child, out); + } +} + +/// One fixture per language whose `Npm` / `Npa` impl enables on +/// [`opens_container_space`](super::opens_container_space). +/// +/// Each carries a container with one public method and one public +/// attribute, at least one ordinary method, and — where the grammar has +/// one — a #1184 construct (`get`/`set`/`init`/`static`), so a single +/// fixture exercises both halves of the defect. +struct Fixture { + lang: LANG, + /// Names of the container spaces that must carry both blocks. + containers: &'static [&'static str], + source: &'static str, +} + +const FIXTURES: &[Fixture] = &[ + Fixture { + lang: LANG::Kotlin, + containers: &["C", "I"], + source: "\ +interface I { + fun q(): Int +} +class C : I { + var p: Int = 0 + get() = field + set(v) { field = v } + init { p = 1 } + override fun q(): Int { return p } +} +", + }, + Fixture { + lang: LANG::Java, + containers: &["C", "I"], + source: "\ +interface I { + int q(); +} +class C implements I { + public int a = 1; + static { System.out.println(\"x\"); } + public int q() { return a; } +} +", + }, + Fixture { + lang: LANG::Groovy, + containers: &["C", "I"], + source: "\ +interface I { + int q() +} +class C implements I { + public int a = 1 + static { println 'x' } + int q() { return a } +} +", + }, + Fixture { + lang: LANG::Javascript, + containers: &["C"], + source: "\ +class C { + a = 1; + static { this.b = 2; } + q() { return this.a; } +} +function top(x) { return x; } +", + }, + Fixture { + lang: LANG::Mozjs, + containers: &["C"], + source: "\ +class C { + a = 1; + static { this.b = 2; } + q() { return this.a; } +} +function top(x) { return x; } +", + }, + Fixture { + lang: LANG::Typescript, + containers: &["C", "I"], + source: "\ +interface I { + q(): number; +} +class C implements I { + public a: number = 1; + static { } + public q(): number { return this.a; } +} +function top(x: number): number { return x; } +", + }, + Fixture { + lang: LANG::Tsx, + containers: &["C", "I"], + source: "\ +interface I { + q(): number; +} +class C implements I { + public a: number = 1; + static { } + public q(): number { return this.a; } +} +function top(x: number): number { return x; } +", + }, + Fixture { + lang: LANG::Csharp, + containers: &["C", "I"], + source: "\ +interface I { + int Q(); +} +class C : I { + public int A = 1; + public int Q() { return A; } +} +", + }, + Fixture { + lang: LANG::Php, + containers: &["C", "I"], + source: "\ +a; } +} +function top($x) { return $x; } +", + }, + Fixture { + lang: LANG::Ruby, + // A Ruby `module` is `SpaceKind::Namespace`, which is a container. + containers: &["M", "C"], + source: "\ +module M + class C + attr_accessor :a + def q + @a + end + end +end +", + }, +]; + +/// The container spaces named by each fixture carry both blocks. +/// +/// The positive half of the contract, and the guard against "fixing" +/// #1197 by disabling the metric everywhere. +#[test] +fn containers_emit_npm_and_npa() { + for fixture in FIXTURES { + let spaces = emitted_spaces(fixture.lang, fixture.source); + for want in fixture.containers { + let found: Vec<&Emitted> = spaces + .iter() + .filter(|s| s.name == *want && s.kind != SpaceKind::Unit) + .collect(); + assert_eq!( + found.len(), + 1, + "{:?}: expected exactly one container space named {want:?}, \ + got {:?}", + fixture.lang, + spaces + .iter() + .map(|s| (s.kind, s.name.as_str())) + .collect::>() + ); + let space = found[0]; + assert!( + matches!( + space.kind, + SpaceKind::Class | SpaceKind::Interface | SpaceKind::Namespace + ), + "{:?}: {want:?} should be a container kind, is {:?}", + fixture.lang, + space.kind + ); + assert!( + space.has_npm && space.has_npa, + "{:?}: container {want:?} must emit npm and npa (npm={}, npa={})", + fixture.lang, + space.has_npm, + space.has_npa + ); + } + } +} + +/// No function space and no file root carries either block. +/// +/// This is the assertion #1197 is about. The `` / `` / +/// `` / `` spaces #1184 introduced are ordinary +/// function spaces here and are covered by the same sweep. +#[test] +fn function_spaces_and_file_roots_emit_neither() { + for fixture in FIXTURES { + let spaces = emitted_spaces(fixture.lang, fixture.source); + let non_containers: Vec<&Emitted> = spaces + .iter() + .filter(|s| matches!(s.kind, SpaceKind::Unit | SpaceKind::Function)) + .collect(); + // A fixture whose functions all failed to open a space would make + // every assertion below vacuous. + assert!( + non_containers.len() >= 2, + "{:?}: expected the unit root plus at least one function space, \ + got {:?}", + fixture.lang, + spaces + .iter() + .map(|s| (s.kind, s.name.as_str())) + .collect::>() + ); + for space in non_containers { + assert!( + !space.has_npm && !space.has_npa, + "{:?}: {:?} space {:?} must not emit npm/npa (npm={}, npa={})", + fixture.lang, + space.kind, + space.name, + space.has_npm, + space.has_npa + ); + } + } +} + +/// Every #1184 construct opens a function space that emits neither +/// block, while a plain method beside it does the same. +/// +/// [`function_spaces_and_file_roots_emit_neither`] would still pass if a +/// grammar stopped opening these spaces at all; naming them pins that +/// they exist *and* stay quiet. +#[test] +fn the_1184_constructs_open_quiet_function_spaces() { + let cases: &[(LANG, &str, &[&str])] = &[ + ( + LANG::Kotlin, + FIXTURES[0].source, + &["", "", ""], + ), + (LANG::Java, FIXTURES[1].source, &[""]), + (LANG::Groovy, FIXTURES[2].source, &[""]), + (LANG::Javascript, FIXTURES[3].source, &[""]), + (LANG::Mozjs, FIXTURES[4].source, &[""]), + (LANG::Typescript, FIXTURES[5].source, &[""]), + (LANG::Tsx, FIXTURES[6].source, &[""]), + ]; + for (lang, source, names) in cases { + let spaces = emitted_spaces(*lang, source); + for name in *names { + let found: Vec<&Emitted> = spaces.iter().filter(|s| s.name == *name).collect(); + assert_eq!( + found.len(), + 1, + "{lang:?}: expected exactly one {name:?} space, got {:?}", + spaces + .iter() + .map(|s| (s.kind, s.name.as_str())) + .collect::>() + ); + assert_eq!(found[0].kind, SpaceKind::Function, "{lang:?}: {name:?}"); + assert!( + !found[0].has_npm && !found[0].has_npa, + "{lang:?}: {name:?} must not emit npm/npa" + ); + } + } +} + +/// Narrowing the enable predicate did not change what the containers +/// count. +/// +/// The emission gate and the counters are independent — `merge` sums the +/// `_sum` fields regardless of `is_class_space` — but that independence +/// is worth pinning rather than assuming, since a wrong predicate could +/// have skipped a `ClassBody` walk instead of just a block. +#[test] +fn container_counts_survive_the_narrowed_enable() { + let space = space_verbatim( + LANG::Java, + FIXTURES[1].source.as_bytes(), + MetricsOptions::default(), + ); + let class = crate::test_support::child_space(&space, "C"); + assert_eq!(class.metrics.npm.class_npm_sum(), 1, "public method `q`"); + assert_eq!(class.metrics.npa.class_npa_sum(), 1, "public attribute `a`"); + + // The roll-up still reaches the root even though the root no longer + // serializes it — the sum is what `bca check` reads at a container, + // and dropping it would be a real regression rather than a shape one. + assert_eq!(space.metrics.npm.class_npm_sum(), 1); + assert_eq!(space.metrics.npa.class_npa_sum(), 1); +} diff --git a/src/metrics/mod.rs b/src/metrics/mod.rs index 3542b8a55..3e6b3dbfb 100644 --- a/src/metrics/mod.rs +++ b/src/metrics/mod.rs @@ -4,6 +4,11 @@ //! traits, and its `Stats` accumulator. See the crate-level docs for an //! overview of the metric suite. +use crate::checker::Checker; +use crate::getter::Getter; +use crate::metric_catalog::MetricScope; +use crate::node::Node; + /// Assignment / Branch / Condition counts. pub mod abc; /// Cognitive complexity. @@ -31,6 +36,10 @@ pub mod tokens; /// Weighted Methods per Class. pub mod wmc; +#[cfg(test)] +#[path = "container_scope_tests.rs"] +mod container_scope_tests; + /// Divides a metric sum by a count, guarding the divisor with `.max(1)`. /// /// Every "average over a count" metric routes through this helper so the @@ -65,3 +74,34 @@ pub mod wmc; pub(crate) fn average(sum: f64, count: usize) -> f64 { sum / count.max(1) as f64 } + +/// Whether `node` opens a *container* space — a class-like scope that owns +/// methods and attributes, and therefore the one scope where the +/// object-oriented `Npm` / `Npa` blocks mean anything. +/// +/// [`Checker::is_func_space`] answers a different, strictly wider question: +/// "does this node open a space at all". The two coincided closely enough +/// to pass for the container test until they did not. In the JS family and +/// C# it has always been true of ordinary methods, and of the file root in +/// every language whose grammar root is listed; [#1184] then added property +/// accessors, `init { … }` and `static { … }` blocks. Each of those spaces +/// grew an all-zero `npm` / `npa` block that the plain method beside it did +/// not have ([#1197]). +/// +/// [`MetricScope::Container`] is reused rather than re-derived: `bca check` +/// and the SARIF export already gate these metrics on exactly that set of +/// space kinds ([#969]), so sharing the definition is what keeps the +/// emitted tree and the threshold scope from drifting apart again. +/// +/// The [`Checker::is_func_space`] conjunct is kept as a precondition, so +/// the flag can still only be set on a node that genuinely opens a space — +/// this predicate is a narrowing of the old one and can never enable a +/// space the old one left disabled. +/// +/// [#969]: https://github.com/dekobon/big-code-analysis/issues/969 +/// [#1184]: https://github.com/dekobon/big-code-analysis/issues/1184 +/// [#1197]: https://github.com/dekobon/big-code-analysis/issues/1197 +#[inline] +pub(crate) fn opens_container_space(node: &Node) -> bool { + L::is_func_space(node) && MetricScope::Container.admits(L::get_space_kind(node)) +} diff --git a/src/metrics/npa.rs b/src/metrics/npa.rs index 74783d486..a53d54bcd 100644 --- a/src/metrics/npa.rs +++ b/src/metrics/npa.rs @@ -22,6 +22,7 @@ use std::fmt; use crate::checker::Checker; use crate::langs::*; use crate::macros::{csharp_var_decl_kinds, csharp_var_declarator_kinds, implement_metric_trait}; +use crate::metrics::opens_container_space; use crate::node::Node; use crate::*; @@ -29,6 +30,11 @@ use crate::*; /// /// This metric counts the number of public attributes /// of classes/interfaces. +/// +/// It is emitted only on *container* spaces — class, struct, trait, +/// impl, namespace and interface — so a serialized function space and +/// the file root carry no `npa` block at all. The enable predicate is +/// `metrics::opens_container_space`. #[derive(Clone, Debug, Default, PartialEq)] #[non_exhaustive] pub struct Stats { @@ -255,10 +261,13 @@ where ); } -// Java and Groovy share their grammar tokens for class/interface -// bodies, so `Npa::compute` differs only by the language enum. -// `impl_npa_java_like!` emits the same body against each enum -// (issue #280). +// `impl_npa_java_like!` was introduced for Java and Groovy, whose +// grammar tokens for class/interface bodies matched closely enough that +// `Npa::compute` differed only by the language enum (issue #280). Groovy +// has since moved to a hand-written impl — the dekobon grammar flattens +// modifiers, see `npa/groovy.rs` — so this expands against Java alone. +// It is kept in macro form because the next Java-shaped grammar can +// reuse it. // // `ClassBody` covers classes and records (records reuse `class_body` // for their explicit declaration body). Record components in @@ -287,7 +296,7 @@ macro_rules! impl_npa_java_like { ) { use $lang::*; - if Self::is_func_space(node) && stats.is_disabled() { + if opens_container_space::(node) && stats.is_disabled() { stats.is_class_space = true; } @@ -376,7 +385,7 @@ macro_rules! ts_npa_compute { ) { use $lang::*; - if Self::is_func_space(node) && stats.is_disabled() { + if opens_container_space::(node) && stats.is_disabled() { stats.is_class_space = true; } @@ -498,7 +507,7 @@ macro_rules! js_npa_compute { ) { use $lang::*; - if Self::is_func_space(node) && stats.is_disabled() { + if opens_container_space::(node) && stats.is_disabled() { stats.is_class_space = true; } diff --git a/src/metrics/npa/csharp.rs b/src/metrics/npa/csharp.rs index 899d2e826..79587325d 100644 --- a/src/metrics/npa/csharp.rs +++ b/src/metrics/npa/csharp.rs @@ -17,7 +17,7 @@ impl Npa for CsharpCode { ) { use Csharp::*; - if Self::is_func_space(node) && stats.is_disabled() { + if opens_container_space::(node) && stats.is_disabled() { stats.is_class_space = true; } diff --git a/src/metrics/npa/groovy.rs b/src/metrics/npa/groovy.rs index 0237ae5b7..01ccf0ae6 100644 --- a/src/metrics/npa/groovy.rs +++ b/src/metrics/npa/groovy.rs @@ -27,7 +27,7 @@ impl Npa for GroovyCode { ) { use Groovy::*; - if Self::is_func_space(node) && stats.is_disabled() { + if opens_container_space::(node) && stats.is_disabled() { stats.is_class_space = true; } diff --git a/src/metrics/npa/kotlin.rs b/src/metrics/npa/kotlin.rs index c902ddb91..b3f50c38a 100644 --- a/src/metrics/npa/kotlin.rs +++ b/src/metrics/npa/kotlin.rs @@ -40,7 +40,7 @@ impl Npa for KotlinCode { // Enables the `Npa` metric for both class and interface spaces // (and `object` singletons, which `Getter` reports as `Class`). - if Self::is_func_space(node) && stats.is_disabled() { + if opens_container_space::(node) && stats.is_disabled() { stats.is_class_space = true; } diff --git a/src/metrics/npa/php.rs b/src/metrics/npa/php.rs index 8fea1c97c..2fedbdfe0 100644 --- a/src/metrics/npa/php.rs +++ b/src/metrics/npa/php.rs @@ -17,8 +17,7 @@ impl Npa for PhpCode { ) { use Php::*; - // Enables the `Npa` metric if computing stats of a class-like space. - if Self::is_func_space(node) && stats.is_disabled() { + if opens_container_space::(node) && stats.is_disabled() { stats.is_class_space = true; } diff --git a/src/metrics/npa/ruby.rs b/src/metrics/npa/ruby.rs index 04d44b117..508e45a61 100644 --- a/src/metrics/npa/ruby.rs +++ b/src/metrics/npa/ruby.rs @@ -17,7 +17,7 @@ impl Npa for RubyCode { ) { use Ruby::*; - if Self::is_func_space(node) && stats.is_disabled() { + if opens_container_space::(node) && stats.is_disabled() { stats.is_class_space = true; } diff --git a/src/metrics/npm.rs b/src/metrics/npm.rs index 2db3a9a52..ddc2a241a 100644 --- a/src/metrics/npm.rs +++ b/src/metrics/npm.rs @@ -23,6 +23,7 @@ use crate::checker::{Checker, csharp_accessor_count}; use crate::langs::*; use crate::macros::implement_metric_trait; use crate::metrics::npa::{accessibility_ratio, python_is_block, ts_member_is_public}; +use crate::metrics::opens_container_space; use crate::node::Node; use crate::*; @@ -30,6 +31,11 @@ use crate::*; /// /// This metric counts the number of public methods /// of classes/interfaces. +/// +/// It is emitted only on *container* spaces — class, struct, trait, +/// impl, namespace and interface — so a serialized function space and +/// the file root carry no `npm` block at all. The enable predicate is +/// `metrics::opens_container_space`. #[derive(Clone, Debug, Default, PartialEq)] #[non_exhaustive] pub struct Stats { @@ -260,10 +266,13 @@ where ); } -// Java and Groovy share their grammar tokens for class / interface -// bodies, so `Npm::compute` differs only by the language enum. -// `impl_npm_java_like!` emits the same body against each enum -// (mirrors `impl_npa_java_like!` in `npa.rs`; issue #280). +// `impl_npm_java_like!` was introduced for Java and Groovy, whose +// grammar tokens for class / interface bodies matched closely enough that +// `Npm::compute` differed only by the language enum (mirrors +// `impl_npa_java_like!` in `npa.rs`; issue #280). Groovy has since moved +// to a hand-written impl — the dekobon grammar flattens modifiers, see +// `npm/groovy.rs` — so this expands against Java alone. It is kept in +// macro form because the next Java-shaped grammar can reuse it. // // `ClassBody` covers class and record explicit bodies; // `EnumBodyDeclarations` is the optional declarations block inside @@ -285,7 +294,7 @@ macro_rules! impl_npm_java_like { ) { use $lang::*; - if Self::is_func_space(node) && stats.is_disabled() { + if opens_container_space::(node) && stats.is_disabled() { stats.is_class_space = true; } @@ -360,7 +369,7 @@ macro_rules! ts_npm_compute { ) { use $lang::*; - if Self::is_func_space(node) && stats.is_disabled() { + if opens_container_space::(node) && stats.is_disabled() { stats.is_class_space = true; } @@ -438,7 +447,7 @@ macro_rules! js_npm_compute { ) { use $lang::*; - if Self::is_func_space(node) && stats.is_disabled() { + if opens_container_space::(node) && stats.is_disabled() { stats.is_class_space = true; } diff --git a/src/metrics/npm/csharp.rs b/src/metrics/npm/csharp.rs index 88d66e555..dc352d83e 100644 --- a/src/metrics/npm/csharp.rs +++ b/src/metrics/npm/csharp.rs @@ -59,7 +59,7 @@ impl Npm for CsharpCode { ) { use Csharp::*; - if Self::is_func_space(node) && stats.is_disabled() { + if opens_container_space::(node) && stats.is_disabled() { stats.is_class_space = true; } diff --git a/src/metrics/npm/groovy.rs b/src/metrics/npm/groovy.rs index cf973a05f..d6c280fc8 100644 --- a/src/metrics/npm/groovy.rs +++ b/src/metrics/npm/groovy.rs @@ -25,7 +25,7 @@ impl Npm for GroovyCode { use crate::metrics::npa::{groovy_body_is_interface_like, groovy_has_explicit_public}; use Groovy::*; - if Self::is_func_space(node) && stats.is_disabled() { + if opens_container_space::(node) && stats.is_disabled() { stats.is_class_space = true; } diff --git a/src/metrics/npm/kotlin.rs b/src/metrics/npm/kotlin.rs index ce949164d..38fce6991 100644 --- a/src/metrics/npm/kotlin.rs +++ b/src/metrics/npm/kotlin.rs @@ -23,8 +23,7 @@ impl Npm for KotlinCode { ) { use Kotlin::*; - // Enables the `Npm` metric for any class-like func_space. - if Self::is_func_space(node) && stats.is_disabled() { + if opens_container_space::(node) && stats.is_disabled() { stats.is_class_space = true; } diff --git a/src/metrics/npm/php.rs b/src/metrics/npm/php.rs index 57c7da2d2..cca2c26a7 100644 --- a/src/metrics/npm/php.rs +++ b/src/metrics/npm/php.rs @@ -24,7 +24,7 @@ impl Npm for PhpCode { // sibling `Abc::compute` carries a marker here. use Php::*; - if Self::is_func_space(node) && stats.is_disabled() { + if opens_container_space::(node) && stats.is_disabled() { stats.is_class_space = true; } diff --git a/src/metrics/npm/ruby.rs b/src/metrics/npm/ruby.rs index 8c5f7e13b..d1751c745 100644 --- a/src/metrics/npm/ruby.rs +++ b/src/metrics/npm/ruby.rs @@ -28,7 +28,7 @@ impl Npm for RubyCode { ) { use Ruby::*; - if Self::is_func_space(node) && stats.is_disabled() { + if opens_container_space::(node) && stats.is_disabled() { stats.is_class_space = true; } diff --git a/tests/repositories/big-code-analysis-output b/tests/repositories/big-code-analysis-output index 7a9c5ec8f..8196e73ae 160000 --- a/tests/repositories/big-code-analysis-output +++ b/tests/repositories/big-code-analysis-output @@ -1 +1 @@ -Subproject commit 7a9c5ec8fb06d185d09588e8ca7af9468104410a +Subproject commit 8196e73aed4e7001ca08cd65b744f21a5d334cbd From e04fb386ff54820c79b6bfddd5f890faedead816 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Mon, 3 Aug 2026 20:07:27 -0700 Subject: [PATCH 2/6] refactor(npm): fold the container enable into a Stats method 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`. --- .bca-baseline.toml | 14 +----- src/metrics/container_scope_tests.rs | 69 ++++++++++++++++++---------- src/metrics/mod.rs | 15 +++--- src/metrics/npa.rs | 28 +++++++---- src/metrics/npa/csharp.rs | 4 +- src/metrics/npa/groovy.rs | 4 +- src/metrics/npa/kotlin.rs | 4 +- src/metrics/npa/php.rs | 4 +- src/metrics/npa/ruby.rs | 4 +- src/metrics/npm.rs | 28 +++++++---- src/metrics/npm/csharp.rs | 4 +- src/metrics/npm/groovy.rs | 4 +- src/metrics/npm/kotlin.rs | 4 +- src/metrics/npm/php.rs | 4 +- src/metrics/npm/ruby.rs | 4 +- 15 files changed, 101 insertions(+), 93 deletions(-) diff --git a/.bca-baseline.toml b/.bca-baseline.toml index dcacfead1..0880699cd 100644 --- a/.bca-baseline.toml +++ b/.bca-baseline.toml @@ -1031,18 +1031,6 @@ qualified = "" metric = "loc.ploc" value = 547.0 -[[entry]] -path = "src/metrics/npa/csharp.rs" -qualified = "CsharpCode::compute" -metric = "cognitive" -value = 15.0 - -[[entry]] -path = "src/metrics/npa/kotlin.rs" -qualified = "KotlinCode::compute" -metric = "cognitive" -value = 15.0 - [[entry]] path = "src/metrics/npa/objc.rs" qualified = "ObjcCode::compute" @@ -1053,7 +1041,7 @@ value = 15.0 path = "src/metrics/npa/php.rs" qualified = "PhpCode::compute" metric = "halstead.effort" -value = 52614.71715355187 +value = 49513.64118505867 [[entry]] path = "src/metrics/npa/python.rs" diff --git a/src/metrics/container_scope_tests.rs b/src/metrics/container_scope_tests.rs index 3bbf01f48..8974e6299 100644 --- a/src/metrics/container_scope_tests.rs +++ b/src/metrics/container_scope_tests.rs @@ -21,10 +21,14 @@ use crate::spaces::SpaceKind; use crate::test_support::space_verbatim; use crate::{LANG, MetricsOptions}; -/// A serialized space flattened to the two things these tests assert on. +/// A serialized space, flattened to what these tests assert on. struct Emitted { kind: SpaceKind, - name: String, + /// `None` for the unit root, which `space_verbatim` analyses without a + /// filename. Modelled as absent rather than defaulted to `""` so a + /// *nested* space that lost its name fails a lookup below instead of + /// quietly matching nothing. + name: Option, has_npm: bool, has_npa: bool, } @@ -47,9 +51,15 @@ fn flatten(value: &Value, out: &mut Vec) { let metrics = value["metrics"] .as_object() .expect("every space serializes a metrics object"); + // `expect` rather than a default on `kind`: a space missing it would + // read as `Unknown`, which no assertion below could tell apart from a + // space these tests are meant to skip. + let kind = value["kind"] + .as_str() + .expect("every space serializes a kind"); out.push(Emitted { - kind: SpaceKind::from_serialized(value["kind"].as_str().unwrap_or_default()), - name: value["name"].as_str().unwrap_or_default().to_owned(), + kind: SpaceKind::from_serialized(kind), + name: value["name"].as_str().map(str::to_owned), has_npm: metrics.contains_key("npm"), has_npa: metrics.contains_key("npa"), }); @@ -72,6 +82,18 @@ struct Fixture { source: &'static str, } +/// The fixture for `lang`. +/// +/// Looked up by language rather than by index, so reordering [`FIXTURES`] +/// cannot silently pair a language with another's source. +fn fixture_source(lang: LANG) -> &'static str { + FIXTURES + .iter() + .find(|f| f.lang == lang) + .unwrap_or_else(|| panic!("no fixture for {lang:?}")) + .source +} + const FIXTURES: &[Fixture] = &[ Fixture { lang: LANG::Kotlin, @@ -227,7 +249,7 @@ fn containers_emit_npm_and_npa() { for want in fixture.containers { let found: Vec<&Emitted> = spaces .iter() - .filter(|s| s.name == *want && s.kind != SpaceKind::Unit) + .filter(|s| s.name.as_deref() == Some(*want) && s.kind != SpaceKind::Unit) .collect(); assert_eq!( found.len(), @@ -237,7 +259,7 @@ fn containers_emit_npm_and_npa() { fixture.lang, spaces .iter() - .map(|s| (s.kind, s.name.as_str())) + .map(|s| (s.kind, s.name.as_deref())) .collect::>() ); let space = found[0]; @@ -283,7 +305,7 @@ fn function_spaces_and_file_roots_emit_neither() { fixture.lang, spaces .iter() - .map(|s| (s.kind, s.name.as_str())) + .map(|s| (s.kind, s.name.as_deref())) .collect::>() ); for space in non_containers { @@ -308,30 +330,29 @@ fn function_spaces_and_file_roots_emit_neither() { /// they exist *and* stay quiet. #[test] fn the_1184_constructs_open_quiet_function_spaces() { - let cases: &[(LANG, &str, &[&str])] = &[ - ( - LANG::Kotlin, - FIXTURES[0].source, - &["", "", ""], - ), - (LANG::Java, FIXTURES[1].source, &[""]), - (LANG::Groovy, FIXTURES[2].source, &[""]), - (LANG::Javascript, FIXTURES[3].source, &[""]), - (LANG::Mozjs, FIXTURES[4].source, &[""]), - (LANG::Typescript, FIXTURES[5].source, &[""]), - (LANG::Tsx, FIXTURES[6].source, &[""]), + let cases: &[(LANG, &[&str])] = &[ + (LANG::Kotlin, &["", "", ""]), + (LANG::Java, &[""]), + (LANG::Groovy, &[""]), + (LANG::Javascript, &[""]), + (LANG::Mozjs, &[""]), + (LANG::Typescript, &[""]), + (LANG::Tsx, &[""]), ]; - for (lang, source, names) in cases { - let spaces = emitted_spaces(*lang, source); + for (lang, names) in cases { + let spaces = emitted_spaces(*lang, fixture_source(*lang)); for name in *names { - let found: Vec<&Emitted> = spaces.iter().filter(|s| s.name == *name).collect(); + let found: Vec<&Emitted> = spaces + .iter() + .filter(|s| s.name.as_deref() == Some(*name)) + .collect(); assert_eq!( found.len(), 1, "{lang:?}: expected exactly one {name:?} space, got {:?}", spaces .iter() - .map(|s| (s.kind, s.name.as_str())) + .map(|s| (s.kind, s.name.as_deref())) .collect::>() ); assert_eq!(found[0].kind, SpaceKind::Function, "{lang:?}: {name:?}"); @@ -354,7 +375,7 @@ fn the_1184_constructs_open_quiet_function_spaces() { fn container_counts_survive_the_narrowed_enable() { let space = space_verbatim( LANG::Java, - FIXTURES[1].source.as_bytes(), + fixture_source(LANG::Java).as_bytes(), MetricsOptions::default(), ); let class = crate::test_support::child_space(&space, "C"); diff --git a/src/metrics/mod.rs b/src/metrics/mod.rs index 3e6b3dbfb..3c76b1822 100644 --- a/src/metrics/mod.rs +++ b/src/metrics/mod.rs @@ -79,14 +79,13 @@ pub(crate) fn average(sum: f64, count: usize) -> f64 { /// methods and attributes, and therefore the one scope where the /// object-oriented `Npm` / `Npa` blocks mean anything. /// -/// [`Checker::is_func_space`] answers a different, strictly wider question: -/// "does this node open a space at all". The two coincided closely enough -/// to pass for the container test until they did not. In the JS family and -/// C# it has always been true of ordinary methods, and of the file root in -/// every language whose grammar root is listed; [#1184] then added property -/// accessors, `init { … }` and `static { … }` blocks. Each of those spaces -/// grew an all-zero `npm` / `npa` block that the plain method beside it did -/// not have ([#1197]). +/// [`Checker::is_func_space`] answers a strictly wider question — "does +/// this node open a space at all" — and stood in for this one until the +/// gap showed. It lists the grammar root in every language, and ordinary +/// methods in the JS family and C#; [#1184] then added property accessors +/// and `init { … }` / `static { … }` blocks. Each of those spaces grew an +/// all-zero `npm` / `npa` block that the plain method beside it did not +/// have ([#1197]). /// /// [`MetricScope::Container`] is reused rather than re-derived: `bca check` /// and the SARIF export already gate these metrics on exactly that set of diff --git a/src/metrics/npa.rs b/src/metrics/npa.rs index a53d54bcd..7b80e3cbf 100644 --- a/src/metrics/npa.rs +++ b/src/metrics/npa.rs @@ -20,6 +20,7 @@ use std::fmt; use crate::checker::Checker; +use crate::getter::Getter; use crate::langs::*; use crate::macros::{csharp_var_decl_kinds, csharp_var_declarator_kinds, implement_metric_trait}; use crate::metrics::opens_container_space; @@ -218,6 +219,21 @@ impl Stats { pub(crate) fn is_disabled(&self) -> bool { !self.is_class_space } + + /// Enables `Npa` on the space `node` opens, if that space is a + /// container (#1197). + /// + /// Idempotent by design: `compute` runs once per node, so the first + /// container node to reach a given space wins and every later call is + /// a no-op. The languages that gate on a bespoke node-kind set — + /// Python, Rust, C++, Mozcpp, Go, Objective-C, Elixir — set the flag + /// themselves and do not route through here. + #[inline] + fn enable_for_container(&mut self, node: &Node) { + if self.is_disabled() && opens_container_space::(node) { + self.is_class_space = true; + } + } } // Computes an accessibility ratio (public members / total members), @@ -296,9 +312,7 @@ macro_rules! impl_npa_java_like { ) { use $lang::*; - if opens_container_space::(node) && stats.is_disabled() { - stats.is_class_space = true; - } + stats.enable_for_container::(node); match node.kind_id().into() { ClassBody | EnumBodyDeclarations => { @@ -385,9 +399,7 @@ macro_rules! ts_npa_compute { ) { use $lang::*; - if opens_container_space::(node) && stats.is_disabled() { - stats.is_class_space = true; - } + stats.enable_for_container::(node); match node.kind_id().into() { ClassBody => { @@ -507,9 +519,7 @@ macro_rules! js_npa_compute { ) { use $lang::*; - if opens_container_space::(node) && stats.is_disabled() { - stats.is_class_space = true; - } + stats.enable_for_container::(node); if !matches!(node.kind_id().into(), ClassBody) { return; diff --git a/src/metrics/npa/csharp.rs b/src/metrics/npa/csharp.rs index 79587325d..13e0ef78d 100644 --- a/src/metrics/npa/csharp.rs +++ b/src/metrics/npa/csharp.rs @@ -17,9 +17,7 @@ impl Npa for CsharpCode { ) { use Csharp::*; - if opens_container_space::(node) && stats.is_disabled() { - stats.is_class_space = true; - } + stats.enable_for_container::(node); // Class / struct / record / interface bodies all share // `DeclarationList`; the parent kind disambiguates. diff --git a/src/metrics/npa/groovy.rs b/src/metrics/npa/groovy.rs index 01ccf0ae6..90d14e167 100644 --- a/src/metrics/npa/groovy.rs +++ b/src/metrics/npa/groovy.rs @@ -27,9 +27,7 @@ impl Npa for GroovyCode { ) { use Groovy::*; - if opens_container_space::(node) && stats.is_disabled() { - stats.is_class_space = true; - } + stats.enable_for_container::(node); match node.kind_id().into() { ClassBody | EnumBody => { diff --git a/src/metrics/npa/kotlin.rs b/src/metrics/npa/kotlin.rs index b3f50c38a..0a394cc7d 100644 --- a/src/metrics/npa/kotlin.rs +++ b/src/metrics/npa/kotlin.rs @@ -40,9 +40,7 @@ impl Npa for KotlinCode { // Enables the `Npa` metric for both class and interface spaces // (and `object` singletons, which `Getter` reports as `Class`). - if opens_container_space::(node) && stats.is_disabled() { - stats.is_class_space = true; - } + stats.enable_for_container::(node); match node.kind_id().into() { // A `ClassParameter` carrying `val` / `var` is a Kotlin diff --git a/src/metrics/npa/php.rs b/src/metrics/npa/php.rs index 2fedbdfe0..45ad3aa5e 100644 --- a/src/metrics/npa/php.rs +++ b/src/metrics/npa/php.rs @@ -17,9 +17,7 @@ impl Npa for PhpCode { ) { use Php::*; - if opens_container_space::(node) && stats.is_disabled() { - stats.is_class_space = true; - } + stats.enable_for_container::(node); // Class / trait / anonymous-class / interface bodies all share // the `DeclarationList` kind; the parent kind disambiguates. diff --git a/src/metrics/npa/ruby.rs b/src/metrics/npa/ruby.rs index 508e45a61..daf96376e 100644 --- a/src/metrics/npa/ruby.rs +++ b/src/metrics/npa/ruby.rs @@ -17,9 +17,7 @@ impl Npa for RubyCode { ) { use Ruby::*; - if opens_container_space::(node) && stats.is_disabled() { - stats.is_class_space = true; - } + stats.enable_for_container::(node); if !matches!(node.kind_id().into(), BodyStatement | BodyStatement2) { return; diff --git a/src/metrics/npm.rs b/src/metrics/npm.rs index ddc2a241a..0421eb3cb 100644 --- a/src/metrics/npm.rs +++ b/src/metrics/npm.rs @@ -20,6 +20,7 @@ use std::fmt; use crate::checker::{Checker, csharp_accessor_count}; +use crate::getter::Getter; use crate::langs::*; use crate::macros::implement_metric_trait; use crate::metrics::npa::{accessibility_ratio, python_is_block, ts_member_is_public}; @@ -219,6 +220,21 @@ impl Stats { pub(crate) fn is_disabled(&self) -> bool { !self.is_class_space } + + /// Enables `Npm` on the space `node` opens, if that space is a + /// container (#1197). + /// + /// Idempotent by design: `compute` runs once per node, so the first + /// container node to reach a given space wins and every later call is + /// a no-op. The languages that gate on a bespoke node-kind set — + /// Python, Rust, C++, Mozcpp, Go, Objective-C, Elixir — set the flag + /// themselves and do not route through here. + #[inline] + fn enable_for_container(&mut self, node: &Node) { + if self.is_disabled() && opens_container_space::(node) { + self.is_class_space = true; + } + } } /// The direct children of `node` that `C` classifies as functions. @@ -294,9 +310,7 @@ macro_rules! impl_npm_java_like { ) { use $lang::*; - if opens_container_space::(node) && stats.is_disabled() { - stats.is_class_space = true; - } + stats.enable_for_container::(node); match node.kind_id().into() { ClassBody | EnumBodyDeclarations => { @@ -369,9 +383,7 @@ macro_rules! ts_npm_compute { ) { use $lang::*; - if opens_container_space::(node) && stats.is_disabled() { - stats.is_class_space = true; - } + stats.enable_for_container::(node); match node.kind_id().into() { ClassBody => { @@ -447,9 +459,7 @@ macro_rules! js_npm_compute { ) { use $lang::*; - if opens_container_space::(node) && stats.is_disabled() { - stats.is_class_space = true; - } + stats.enable_for_container::(node); if !matches!(node.kind_id().into(), ClassBody) { return; diff --git a/src/metrics/npm/csharp.rs b/src/metrics/npm/csharp.rs index dc352d83e..091ca6307 100644 --- a/src/metrics/npm/csharp.rs +++ b/src/metrics/npm/csharp.rs @@ -59,9 +59,7 @@ impl Npm for CsharpCode { ) { use Csharp::*; - if opens_container_space::(node) && stats.is_disabled() { - stats.is_class_space = true; - } + stats.enable_for_container::(node); if !matches!(node.kind_id().into(), DeclarationList) { return; diff --git a/src/metrics/npm/groovy.rs b/src/metrics/npm/groovy.rs index d6c280fc8..6cd27bc46 100644 --- a/src/metrics/npm/groovy.rs +++ b/src/metrics/npm/groovy.rs @@ -25,9 +25,7 @@ impl Npm for GroovyCode { use crate::metrics::npa::{groovy_body_is_interface_like, groovy_has_explicit_public}; use Groovy::*; - if opens_container_space::(node) && stats.is_disabled() { - stats.is_class_space = true; - } + stats.enable_for_container::(node); match node.kind_id().into() { ClassBody | EnumBody => { diff --git a/src/metrics/npm/kotlin.rs b/src/metrics/npm/kotlin.rs index 38fce6991..409ee696b 100644 --- a/src/metrics/npm/kotlin.rs +++ b/src/metrics/npm/kotlin.rs @@ -23,9 +23,7 @@ impl Npm for KotlinCode { ) { use Kotlin::*; - if opens_container_space::(node) && stats.is_disabled() { - stats.is_class_space = true; - } + stats.enable_for_container::(node); // Each `ClassBody` contributes its direct `FunctionDeclaration` // and `SecondaryConstructor` children to whichever func_space is diff --git a/src/metrics/npm/php.rs b/src/metrics/npm/php.rs index cca2c26a7..0242fdf22 100644 --- a/src/metrics/npm/php.rs +++ b/src/metrics/npm/php.rs @@ -24,9 +24,7 @@ impl Npm for PhpCode { // sibling `Abc::compute` carries a marker here. use Php::*; - if opens_container_space::(node) && stats.is_disabled() { - stats.is_class_space = true; - } + stats.enable_for_container::(node); match node.kind_id().into() { DeclarationList => { diff --git a/src/metrics/npm/ruby.rs b/src/metrics/npm/ruby.rs index d1751c745..9d0aafdb2 100644 --- a/src/metrics/npm/ruby.rs +++ b/src/metrics/npm/ruby.rs @@ -28,9 +28,7 @@ impl Npm for RubyCode { ) { use Ruby::*; - if opens_container_space::(node) && stats.is_disabled() { - stats.is_class_space = true; - } + stats.enable_for_container::(node); if !matches!(node.kind_id().into(), BodyStatement | BodyStatement2) { return; From 544b0da8fb2eb6b5864ad3f763efd9bc413cf468 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Mon, 3 Aug 2026 20:13:07 -0700 Subject: [PATCH 3/6] refactor(metrics): narrow visibility and dedupe test lookups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- src/metrics/container_scope_tests.rs | 65 +++++++++++++--------------- src/metrics/mod.rs | 2 +- 2 files changed, 31 insertions(+), 36 deletions(-) diff --git a/src/metrics/container_scope_tests.rs b/src/metrics/container_scope_tests.rs index 8974e6299..a62d26cca 100644 --- a/src/metrics/container_scope_tests.rs +++ b/src/metrics/container_scope_tests.rs @@ -94,6 +94,31 @@ fn fixture_source(lang: LANG) -> &'static str { .source } +/// Every space as `(kind, name)`, for an assertion message. +fn summary(spaces: &[Emitted]) -> Vec<(SpaceKind, Option<&str>)> { + spaces.iter().map(|s| (s.kind, s.name.as_deref())).collect() +} + +/// The one space named `name`, or a failure naming every space there was. +/// +/// Asserting the match count rather than taking the first hit is what +/// stops a rename or a lost space from making the caller's assertions +/// vacuous. +#[track_caller] +fn only_space<'a>(lang: LANG, spaces: &'a [Emitted], name: &str) -> &'a Emitted { + let found: Vec<&Emitted> = spaces + .iter() + .filter(|s| s.name.as_deref() == Some(name)) + .collect(); + assert_eq!( + found.len(), + 1, + "{lang:?}: expected exactly one space named {name:?}, got {:?}", + summary(spaces) + ); + found[0] +} + const FIXTURES: &[Fixture] = &[ Fixture { lang: LANG::Kotlin, @@ -247,22 +272,7 @@ fn containers_emit_npm_and_npa() { for fixture in FIXTURES { let spaces = emitted_spaces(fixture.lang, fixture.source); for want in fixture.containers { - let found: Vec<&Emitted> = spaces - .iter() - .filter(|s| s.name.as_deref() == Some(*want) && s.kind != SpaceKind::Unit) - .collect(); - assert_eq!( - found.len(), - 1, - "{:?}: expected exactly one container space named {want:?}, \ - got {:?}", - fixture.lang, - spaces - .iter() - .map(|s| (s.kind, s.name.as_deref())) - .collect::>() - ); - let space = found[0]; + let space = only_space(fixture.lang, &spaces, want); assert!( matches!( space.kind, @@ -303,10 +313,7 @@ fn function_spaces_and_file_roots_emit_neither() { "{:?}: expected the unit root plus at least one function space, \ got {:?}", fixture.lang, - spaces - .iter() - .map(|s| (s.kind, s.name.as_deref())) - .collect::>() + summary(&spaces) ); for space in non_containers { assert!( @@ -342,22 +349,10 @@ fn the_1184_constructs_open_quiet_function_spaces() { for (lang, names) in cases { let spaces = emitted_spaces(*lang, fixture_source(*lang)); for name in *names { - let found: Vec<&Emitted> = spaces - .iter() - .filter(|s| s.name.as_deref() == Some(*name)) - .collect(); - assert_eq!( - found.len(), - 1, - "{lang:?}: expected exactly one {name:?} space, got {:?}", - spaces - .iter() - .map(|s| (s.kind, s.name.as_deref())) - .collect::>() - ); - assert_eq!(found[0].kind, SpaceKind::Function, "{lang:?}: {name:?}"); + let space = only_space(*lang, &spaces, name); + assert_eq!(space.kind, SpaceKind::Function, "{lang:?}: {name:?}"); assert!( - !found[0].has_npm && !found[0].has_npa, + !space.has_npm && !space.has_npa, "{lang:?}: {name:?} must not emit npm/npa" ); } diff --git a/src/metrics/mod.rs b/src/metrics/mod.rs index 3c76b1822..61a80007f 100644 --- a/src/metrics/mod.rs +++ b/src/metrics/mod.rs @@ -101,6 +101,6 @@ pub(crate) fn average(sum: f64, count: usize) -> f64 { /// [#1184]: https://github.com/dekobon/big-code-analysis/issues/1184 /// [#1197]: https://github.com/dekobon/big-code-analysis/issues/1197 #[inline] -pub(crate) fn opens_container_space(node: &Node) -> bool { +fn opens_container_space(node: &Node) -> bool { L::is_func_space(node) && MetricScope::Container.admits(L::get_space_kind(node)) } From 042a258c121ddbdf68a8f7a57c054a6b3242779f Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Mon, 3 Aug 2026 20:48:51 -0700 Subject: [PATCH 4/6] fix(npm): keep the file-root roll-up, drop only function spaces 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 `` 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. --- .bca-baseline.toml | 2 +- CHANGELOG.md | 60 +++++++----- STABILITY.md | 8 +- big-code-analysis-book/src/metrics.md | 34 ++++--- big-code-analysis-py/src/types_codegen.rs | 9 +- .../tests/test_metrics_select.py | 59 ++++++++---- src/metrics/container_scope_tests.rs | 92 ++++++++++++++++--- src/metrics/mod.rs | 48 ++++++---- src/metrics/npa.rs | 50 ++++++---- src/metrics/npa/csharp.rs | 4 +- src/metrics/npa/groovy.rs | 4 +- src/metrics/npa/kotlin.rs | 4 +- src/metrics/npa/php.rs | 4 +- src/metrics/npa/ruby.rs | 2 +- src/metrics/npm.rs | 50 ++++++---- src/metrics/npm/csharp.rs | 4 +- src/metrics/npm/groovy.rs | 4 +- src/metrics/npm/kotlin.rs | 4 +- src/metrics/npm/php.rs | 4 +- src/metrics/npm/ruby.rs | 2 +- src/metrics/wmc.rs | 2 +- src/spaces/space_kind.rs | 25 +++++ tests/repositories/big-code-analysis-output | 2 +- 23 files changed, 320 insertions(+), 157 deletions(-) diff --git a/.bca-baseline.toml b/.bca-baseline.toml index 0880699cd..4b762bb86 100644 --- a/.bca-baseline.toml +++ b/.bca-baseline.toml @@ -1041,7 +1041,7 @@ value = 15.0 path = "src/metrics/npa/php.rs" qualified = "PhpCode::compute" metric = "halstead.effort" -value = 49513.64118505867 +value = 51274.96613103099 [[entry]] path = "src/metrics/npa/python.rs" diff --git a/CHANGELOG.md b/CHANGELOG.md index 9d178829a..03192f5b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -766,31 +766,41 @@ for historical reference. ### Fixed -- **Serialized shape.** `npm` and `npa` are now emitted only on - *container* spaces — class, struct, trait, impl, namespace, interface - (#1197). They were enabled 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 therefore 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 as - well; #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 that the plain method beside - them did not have. The enable now reuses `MetricScope::Container`, 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 - rather than two. Kotlin, Java, Groovy, JavaScript, MozJS, TypeScript, - TSX, C#, PHP and Ruby are affected; Python, Rust, C, C++, Mozcpp, Go, - Objective-C and Elixir already gated on node kind and do not move. - **This changes serialized JSON / YAML / TOML / CBOR shape versus - `2.0.x`** for the seven languages that emitted on methods, and for all - ten at the file root: a consumer reading `metrics.npm` off a function - space or the unit root now finds the key absent. Every removed block - was all zeros, no metric *value* changed, and container counts — - including the `_sum` roll-up the whole tree still accumulates — are - unchanged. The CSV projection is a fixed-column format and is - unaffected: it writes the `npm.*` / `npa.*` columns on every row - regardless. +- **Serialized shape.** `npm` and `npa` no longer emit an all-zero block + on function spaces (#1197). They enabled themselves from + `Checker::is_func_space`, which answers "does this node open a space", + not "is this a scope that owns methods and attributes". C#, JavaScript, + MozJS, TypeScript, TSX, PHP and Ruby therefore carried a block on every + ordinary method, and #1184 extended that to Kotlin `get()` / `set()` / + `init { … }`, Java and Groovy `static { … }` and the JS-family + `class_static_block` — which is the inconsistency the issue reports: a + `` space carrying OOP metrics while the `m` beside it did not. + Kotlin, Java, Groovy, JavaScript, MozJS, TypeScript, TSX, C#, PHP and + Ruby are affected; Python, Rust, C, C++, Mozcpp, Go, Objective-C and + Elixir gate on their own node kinds and do not move. + + The rule is now `SpaceKind::is_member_scope`, which `wmc` already + followed and which the three metrics share as a single definition: + containers and the whole-file `unit` root carry the block, a function + space never does. **The file-root roll-up is retained** — an earlier + draft of this fix narrowed to containers alone, which would have + deleted the whole-file `class_npm_sum` (7,530 fields across the + integration corpus, 400 of them non-zero) and left `npm` / `npa` + disagreeing with `wmc` about the same root. + + Consumers reading `metrics.npm` off a *function* space now find the key + absent. Across the integration corpus that removes 6,974 blocks, of + which 6,969 were entirely zero. The other five belong to a function + that lexically contains a class — a PHP `new class { … }` or a + JavaScript class inside a callback — where the block was that nested + class's roll-up; it remains available on the class's own space and in + the file-root total, and `wmc` has always omitted it in the same + position. No metric value changed anywhere. + + The CSV projection is a fixed-column format and is unaffected: it + writes the `npm.*` / `npa.*` columns on every row regardless of space + kind, carrying the real accessor values. + - **Metric drift.** ABC `conditions` moves wherever a comment sits inside a ternary (#1181). Slots are now addressed by grammar field rather than by neighbouring token or fixed index, which fixes two opposite errors diff --git a/STABILITY.md b/STABILITY.md index afda85a0b..8a5733ebd 100644 --- a/STABILITY.md +++ b/STABILITY.md @@ -1074,8 +1074,12 @@ drift-gate test, so the Python types cannot diverge from the JSON the CLI emits. The change is stub-only (the runtime values are plain dicts, byte-identical to the CLI output), so it only narrows the static type. Every metric block is `NotRequired` because a `metrics=` -selection can elide blocks; under the default full suite every block -is present. The VCS *report* dicts are now single-sourced and typed too +selection can elide blocks, and because the object-oriented blocks are +scope-gated: `wmc`, `npm` and `npa` are emitted on container spaces and +on the file root, never on a function space (#1197), and a language +with no class-like construct emits them nowhere. Which spaces carry +which block is not part of the shape contract — the `wire` struct +definitions are. The VCS *report* dicts are now single-sourced and typed too (#664): `vcs.rank` returns `VcsReportDict`, `vcs.trend` returns `VcsTrendDict`, `vcs.commit` returns `JitCommitReportDict`, and `vcs.score_diff` returns `JitDiffReportDict`. The report / trend envelope diff --git a/big-code-analysis-book/src/metrics.md b/big-code-analysis-book/src/metrics.md index 14b32d6c1..c9db27be6 100644 --- a/big-code-analysis-book/src/metrics.md +++ b/big-code-analysis-book/src/metrics.md @@ -917,15 +917,26 @@ decides what counts as "public" (Java `public`, C# `public`, Rust `pub`, Python's "no leading underscore" convention, …) and what counts as "attribute" rather than "method". -NPA and NPM are emitted **only on container spaces** {#oop-emission-scope} -— `class`, `struct`, `trait`, `impl`, `namespace`, `interface`. A -function space and the whole-file `unit` root carry no `npa` / `npm` -block at all, because neither owns attributes or methods. This is the -same set of space kinds a `npa` / `npm` threshold is checked against -(see [Threshold scope](commands/check.md#threshold-scope)), so what -`bca metrics` emits and what `bca check` gates are one rule. The CSV -projection is the exception: it is a fixed-column format and writes the -`npa.*` / `npm.*` columns as zeros on every row, container or not. +NPA, NPM and WMC are emitted on **container spaces** {#oop-emission-scope} +— `class`, `struct`, `trait`, `impl`, `namespace`, `interface` — and on +the whole-file `unit` root, which carries the roll-up across every +container in the file. A **function space never carries them**: a method +owns no methods or attributes of its own, so the block would be all +zeros. Before big-code-analysis 2.1.0, NPA and NPM did emit that +all-zero block on function spaces in C#, JavaScript, MozJS, TypeScript, +TSX, PHP and Ruby, and on the Kotlin, Java, Groovy and JS-family +accessor / `init` / `static` spaces. + +Two caveats. Go and Rust set the flag from their own node kinds rather +than the shared rule, so a Go file root or a Rust file root can still +carry a block the rule above would not predict. And the CSV projection +is a fixed-column format: it writes the `npa.*` / `npm.*` columns on +**every** row regardless of space kind, carrying the real accessor +values rather than eliding them. + +Thresholds are narrower still — `bca check` gates `npa` / `npm` on +container spaces only, never the file root (see +[Threshold scope](commands/check.md#threshold-scope)). ### How to read it @@ -956,9 +967,8 @@ As with NPA, big-code-analysis splits NPM by definition-site kind `interface_npm_sum`, `class_methods` (sum of *all* methods — public or not — across classes), `interface_methods`, `class_coa`, `interface_coa` (operation-accessibility *ratios*, not averages), -`total`, `total_methods`, and `coa`. It is emitted on the same -container spaces, and elided everywhere else, for the same reason -([above](#oop-emission-scope)). +`total`, `total_methods`, and `coa`. It follows the same emission rule +as NPA ([above](#oop-emission-scope)). The language-specific `Npm` trait decides what counts as public — for example, Rust's `pub`, Python's leading-underscore convention, C++'s `public:` section — and folds together regular methods, diff --git a/big-code-analysis-py/src/types_codegen.rs b/big-code-analysis-py/src/types_codegen.rs index 85c79652c..812c9f723 100644 --- a/big-code-analysis-py/src/types_codegen.rs +++ b/big-code-analysis-py/src/types_codegen.rs @@ -1058,13 +1058,16 @@ mod tests { /// `vcs` block respectively — otherwise their specs would be untested. /// /// `npm` / `npa` are read off the class space rather than the file - /// root: since #1197 they are emitted only on container spaces. + /// root. Both carry the block, but the class is where the counts are + /// actually produced — the root only rolls them up — so asserting + /// there keeps the spec check independent of the roll-up. #[test] fn class_only_and_vcs_specs_match_wire_json_keys() { use big_code_analysis::wire; - // Java emits wmc at the file root, but npm / npa only on the - // container space itself (#1197) — read each where it lives. + // Java emits all three on both the root and the class; read wmc + // at the root and npm / npa at the class, so each is checked + // where the metric is defined rather than where it aggregates. let value = analyze_to_value( "java", "public class Foo {\n public int a;\n public int m() { return 1; }\n}\n", diff --git a/big-code-analysis-py/tests/test_metrics_select.py b/big-code-analysis-py/tests/test_metrics_select.py index af02dc439..ac95cbb83 100644 --- a/big-code-analysis-py/tests/test_metrics_select.py +++ b/big-code-analysis-py/tests/test_metrics_select.py @@ -41,18 +41,6 @@ def _metrics_keys(result: FuncSpaceDict) -> set[str]: return set(result["metrics"].keys()) -def _all_metrics_keys(result: FuncSpaceDict) -> set[str]: - """Return every metric-family key on the space tree, at any depth. - - ``npm`` and ``npa`` are emitted only on container spaces (#1197), so - a key that exists for a fixture need not exist at its root. - """ - keys = _metrics_keys(result) - for space in result.get("spaces", ()): - keys |= _all_metrics_keys(space) - return keys - - # ───────────────────────────────────────────────────────────────── # METRIC_NAMES module constant # ───────────────────────────────────────────────────────────────── @@ -89,19 +77,22 @@ def test_metric_names_constant_shape() -> None: def test_metric_names_round_trip_through_analyze() -> None: """Every name in ``METRIC_NAMES`` is accepted by ``metrics=``. - Class-only metrics (``npa``, ``npm``, ``wmc``) need a class to - attach to, so they use the Java fixture. ``npa`` and ``npm`` are - further emitted only on the container space itself (#1197), never - on the file root, so their keys are collected over the whole space - tree rather than off the top-level space. + Class-only metrics (``npa``, ``npm``, ``wmc``) are elided from + a unit-level space when there is no class to attach them to — + use the Java fixture (which carries a class) for those so the + output key actually appears. + + They are also elided from every *function* space, which is where + the unit root differs from a nested one (#1197); the assertion + below reads the root, and ``test_class_only_metrics_skip_function_spaces`` + pins the other half. """ class_only = {"npa", "npm", "wmc"} - container_only = {"npa", "npm"} for name in bca.METRIC_NAMES: fixture = FIXTURES / ("Hello.java" if name in class_only else "hello.py") result = bca.analyze(fixture, metrics=[name]) assert result is not None, f"analyze returned None for metrics=[{name!r}]" - keys = _all_metrics_keys(result) if name in container_only else _metrics_keys(result) + keys = _metrics_keys(result) # ``mi`` and ``wmc`` are derived; their *direct* output key # may sit on a nested space (or, for ``wmc``, be ``"wmc"`` # at the unit root after the Java class merge). Their @@ -528,3 +519,33 @@ def test_metrics_does_not_override_skip_generated() -> None: """ result = bca.analyze(FIXTURES / "generated.rs", metrics=["loc"]) assert result is None + + +def test_class_only_metrics_skip_function_spaces() -> None: + """``npa`` / ``npm`` never appear on a function space (#1197). + + The root-level assertion in + ``test_metric_names_round_trip_through_analyze`` cannot see this: + it reads only the top-level space, which legitimately carries the + whole-file roll-up. Before #1197 every Java method space carried an + all-zero block of its own. + """ + result = bca.analyze(FIXTURES / "Hello.java") + assert result is not None + assert {"npa", "npm"} <= _metrics_keys(result), "root keeps its roll-up" + + functions = [s for s in _walk(result) if s["kind"] == "function"] + assert functions, "the Java fixture must contain at least one method" + for space in functions: + keys = set(space["metrics"].keys()) + assert not ({"npa", "npm"} & keys), ( + f"function space {space['name']!r} must not carry npa/npm; got {sorted(keys)}" + ) + + +def _walk(space: FuncSpaceDict) -> list[FuncSpaceDict]: + """Every space in the tree, root first.""" + out = [space] + for child in space.get("spaces", ()): + out.extend(_walk(child)) + return out diff --git a/src/metrics/container_scope_tests.rs b/src/metrics/container_scope_tests.rs index a62d26cca..8f6cdbb1d 100644 --- a/src/metrics/container_scope_tests.rs +++ b/src/metrics/container_scope_tests.rs @@ -10,15 +10,21 @@ //! //! That blindness is exactly how #1197 shipped: `Npm` and `Npa` enabled //! themselves on `Checker::is_func_space`, which means "opens a space", -//! not "is a container". Ten languages therefore emitted an all-zero -//! block on the file root, and seven of them on every ordinary method -//! too; #1184 then added property accessors and `init` / `static` blocks -//! to the list, next to sibling methods that had none. +//! not "is a scope that owns members". Seven of the ten languages +//! therefore emitted an all-zero block on every ordinary method, and +//! #1184 added Kotlin property accessors and `init` / `static` blocks to +//! the list, next to sibling methods that had none. +//! +//! The rule they now follow is [`SpaceKind::is_member_scope`], which +//! `wmc` already used: containers and the file unit carry the block, a +//! function space never does. Both directions are asserted below, +//! because narrowing too far would silently delete the whole-file +//! roll-up rather than the all-zero noise. use serde_json::Value; use crate::spaces::SpaceKind; -use crate::test_support::space_verbatim; +use crate::test_support::{assert_fixtures_present, space_verbatim}; use crate::{LANG, MetricsOptions}; /// A serialized space, flattened to what these tests assert on. @@ -69,7 +75,7 @@ fn flatten(value: &Value, out: &mut Vec) { } /// One fixture per language whose `Npm` / `Npa` impl enables on -/// [`opens_container_space`](super::opens_container_space). +/// [`opens_member_scope`](super::opens_member_scope). /// /// Each carries a container with one public method and one public /// attribute, at least one ordinary method, and — where the grammar has @@ -120,6 +126,7 @@ fn only_space<'a>(lang: LANG, spaces: &'a [Emitted], name: &str) -> &'a Emitted } const FIXTURES: &[Fixture] = &[ + #[cfg(feature = "kotlin")] Fixture { lang: LANG::Kotlin, containers: &["C", "I"], @@ -136,6 +143,7 @@ class C : I { } ", }, + #[cfg(feature = "java")] Fixture { lang: LANG::Java, containers: &["C", "I"], @@ -150,6 +158,7 @@ class C implements I { } ", }, + #[cfg(feature = "groovy")] Fixture { lang: LANG::Groovy, containers: &["C", "I"], @@ -164,6 +173,7 @@ class C implements I { } ", }, + #[cfg(feature = "javascript")] Fixture { lang: LANG::Javascript, containers: &["C"], @@ -176,6 +186,7 @@ class C { function top(x) { return x; } ", }, + #[cfg(feature = "mozjs")] Fixture { lang: LANG::Mozjs, containers: &["C"], @@ -188,6 +199,7 @@ class C { function top(x) { return x; } ", }, + #[cfg(feature = "typescript")] Fixture { lang: LANG::Typescript, containers: &["C", "I"], @@ -203,6 +215,7 @@ class C implements I { function top(x: number): number { return x; } ", }, + #[cfg(feature = "typescript")] Fixture { lang: LANG::Tsx, containers: &["C", "I"], @@ -218,6 +231,7 @@ class C implements I { function top(x: number): number { return x; } ", }, + #[cfg(feature = "csharp")] Fixture { lang: LANG::Csharp, containers: &["C", "I"], @@ -227,10 +241,17 @@ interface I { } class C : I { public int A = 1; + private int[] _v = new int[4]; + // An expression-bodied property and an accessor-less indexer are + // `is_func_space` and `SpaceKind::Function` (#464, #472), so before + // #1197 each carried an all-zero block beside `Q`, which had none. + public int W => A; + public int this[int i] => _v[i]; public int Q() { return A; } } ", }, + #[cfg(feature = "php")] Fixture { lang: LANG::Php, containers: &["C", "I"], @@ -246,6 +267,7 @@ class C implements I { function top($x) { return $x; } ", }, + #[cfg(feature = "ruby")] Fixture { lang: LANG::Ruby, // A Ruby `module` is `SpaceKind::Namespace`, which is a container. @@ -269,6 +291,7 @@ end /// #1197 by disabling the metric everywhere. #[test] fn containers_emit_npm_and_npa() { + assert_fixtures_present(FIXTURES); for fixture in FIXTURES { let spaces = emitted_spaces(fixture.lang, fixture.source); for want in fixture.containers { @@ -293,29 +316,30 @@ fn containers_emit_npm_and_npa() { } } -/// No function space and no file root carries either block. +/// No function space carries either block. /// /// This is the assertion #1197 is about. The `` / `` / /// `` / `` spaces #1184 introduced are ordinary -/// function spaces here and are covered by the same sweep. +/// function spaces here and are covered by the same sweep, as are C#'s +/// expression-bodied property and indexer. #[test] -fn function_spaces_and_file_roots_emit_neither() { +fn function_spaces_emit_neither() { + assert_fixtures_present(FIXTURES); for fixture in FIXTURES { let spaces = emitted_spaces(fixture.lang, fixture.source); - let non_containers: Vec<&Emitted> = spaces + let functions: Vec<&Emitted> = spaces .iter() - .filter(|s| matches!(s.kind, SpaceKind::Unit | SpaceKind::Function)) + .filter(|s| s.kind == SpaceKind::Function) .collect(); // A fixture whose functions all failed to open a space would make // every assertion below vacuous. assert!( - non_containers.len() >= 2, - "{:?}: expected the unit root plus at least one function space, \ - got {:?}", + !functions.is_empty(), + "{:?}: expected at least one function space, got {:?}", fixture.lang, summary(&spaces) ); - for space in non_containers { + for space in functions { assert!( !space.has_npm && !space.has_npa, "{:?}: {:?} space {:?} must not emit npm/npa (npm={}, npa={})", @@ -329,6 +353,30 @@ fn function_spaces_and_file_roots_emit_neither() { } } +/// The whole-file roll-up survives on the unit root, exactly as `wmc`'s +/// does. +/// +/// Narrowing the enable to containers alone would have deleted this — an +/// information loss, not the all-zero-noise removal #1197 asked for, and +/// it would have left `npm` / `npa` disagreeing with `wmc` about a root +/// the three metrics share a [`MetricScope`](crate::metric_catalog::MetricScope). +#[test] +fn the_file_root_keeps_its_rollup() { + assert_fixtures_present(FIXTURES); + for fixture in FIXTURES { + let spaces = emitted_spaces(fixture.lang, fixture.source); + let root = spaces.first().expect("the root space is always emitted"); + assert_eq!(root.kind, SpaceKind::Unit, "{:?}: root kind", fixture.lang); + assert!( + root.has_npm && root.has_npa, + "{:?}: the unit root must keep its npm/npa roll-up (npm={}, npa={})", + fixture.lang, + root.has_npm, + root.has_npa + ); + } +} + /// Every #1184 construct opens a function space that emits neither /// block, while a plain method beside it does the same. /// @@ -338,14 +386,22 @@ fn function_spaces_and_file_roots_emit_neither() { #[test] fn the_1184_constructs_open_quiet_function_spaces() { let cases: &[(LANG, &[&str])] = &[ + #[cfg(feature = "kotlin")] (LANG::Kotlin, &["", "", ""]), + #[cfg(feature = "java")] (LANG::Java, &[""]), + #[cfg(feature = "groovy")] (LANG::Groovy, &[""]), + #[cfg(feature = "javascript")] (LANG::Javascript, &[""]), + #[cfg(feature = "mozjs")] (LANG::Mozjs, &[""]), + #[cfg(feature = "typescript")] (LANG::Typescript, &[""]), + #[cfg(feature = "typescript")] (LANG::Tsx, &[""]), ]; + assert_fixtures_present(cases); for (lang, names) in cases { let spaces = emitted_spaces(*lang, fixture_source(*lang)); for name in *names { @@ -367,6 +423,7 @@ fn the_1184_constructs_open_quiet_function_spaces() { /// is worth pinning rather than assuming, since a wrong predicate could /// have skipped a `ClassBody` walk instead of just a block. #[test] +#[cfg(feature = "java")] fn container_counts_survive_the_narrowed_enable() { let space = space_verbatim( LANG::Java, @@ -377,6 +434,11 @@ fn container_counts_survive_the_narrowed_enable() { assert_eq!(class.metrics.npm.class_npm_sum(), 1, "public method `q`"); assert_eq!(class.metrics.npa.class_npa_sum(), 1, "public attribute `a`"); + // The interface half of the same fixture, which a class-only + // assertion would leave free to regress to zero. + let interface = crate::test_support::child_space(&space, "I"); + assert_eq!(interface.metrics.npm.interface_npm_sum(), 1, "`I::q`"); + // The roll-up still reaches the root even though the root no longer // serializes it — the sum is what `bca check` reads at a container, // and dropping it would be a real regression rather than a shape one. diff --git a/src/metrics/mod.rs b/src/metrics/mod.rs index 61a80007f..81e39cf0c 100644 --- a/src/metrics/mod.rs +++ b/src/metrics/mod.rs @@ -6,7 +6,7 @@ use crate::checker::Checker; use crate::getter::Getter; -use crate::metric_catalog::MetricScope; +use crate::node::Ancestors; use crate::node::Node; /// Assignment / Branch / Condition counts. @@ -75,32 +75,40 @@ pub(crate) fn average(sum: f64, count: usize) -> f64 { sum / count.max(1) as f64 } -/// Whether `node` opens a *container* space — a class-like scope that owns -/// methods and attributes, and therefore the one scope where the -/// object-oriented `Npm` / `Npa` blocks mean anything. +/// Whether `node` opens a space at which the object-oriented member +/// metrics are meaningful — a container, or the file unit that rolls its +/// containers up. See [`SpaceKind::is_member_scope`]. /// /// [`Checker::is_func_space`] answers a strictly wider question — "does /// this node open a space at all" — and stood in for this one until the -/// gap showed. It lists the grammar root in every language, and ordinary -/// methods in the JS family and C#; [#1184] then added property accessors -/// and `init { … }` / `static { … }` blocks. Each of those spaces grew an -/// all-zero `npm` / `npa` block that the plain method beside it did not -/// have ([#1197]). +/// gap showed. It lists ordinary methods in the JS family, C#, PHP and +/// Ruby; [#1184] then added Kotlin property accessors and `init { … }` / +/// `static { … }` blocks. Each of those function spaces grew an all-zero +/// `npm` / `npa` block that the plain method beside it did not have +/// ([#1197]). /// -/// [`MetricScope::Container`] is reused rather than re-derived: `bca check` -/// and the SARIF export already gate these metrics on exactly that set of -/// space kinds ([#969]), so sharing the definition is what keeps the -/// emitted tree and the threshold scope from drifting apart again. +/// Classifies through the **source-aware** [`Checker::is_func_space_with_code`] +/// and [`Getter::get_space_kind_with_code`], because those are what the +/// walker itself uses to build and label the space tree +/// (`spaces::compute`). The byte-less forms disagree for Elixir, whose +/// `defmodule` is a `Class` only to `get_space_kind_with_code` — routing +/// Elixir through here with the plain forms would silently emit nothing +/// for every module, with no test or diagnostic to catch it. /// -/// The [`Checker::is_func_space`] conjunct is kept as a precondition, so -/// the flag can still only be set on a node that genuinely opens a space — -/// this predicate is a narrowing of the old one and can never enable a -/// space the old one left disabled. +/// The `is_func_space_with_code` conjunct is a precondition rather than a +/// live filter: no language currently classifies a member scope on a node +/// that opens no space. It keeps the flag pinned to the space's own root +/// node, so a container kind appearing on a non-space node could never +/// enable the *enclosing* space by accident. /// -/// [#969]: https://github.com/dekobon/big-code-analysis/issues/969 /// [#1184]: https://github.com/dekobon/big-code-analysis/issues/1184 /// [#1197]: https://github.com/dekobon/big-code-analysis/issues/1197 #[inline] -fn opens_container_space(node: &Node) -> bool { - L::is_func_space(node) && MetricScope::Container.admits(L::get_space_kind(node)) +fn opens_member_scope<'a, L: Checker + Getter>( + node: &Node<'a>, + code: &[u8], + ancestors: Ancestors<'a, '_>, +) -> bool { + L::is_func_space_with_code(node, code, ancestors) + && L::get_space_kind_with_code(node, code, ancestors).is_member_scope() } diff --git a/src/metrics/npa.rs b/src/metrics/npa.rs index 7b80e3cbf..1bd3ff3a6 100644 --- a/src/metrics/npa.rs +++ b/src/metrics/npa.rs @@ -23,7 +23,7 @@ use crate::checker::Checker; use crate::getter::Getter; use crate::langs::*; use crate::macros::{csharp_var_decl_kinds, csharp_var_declarator_kinds, implement_metric_trait}; -use crate::metrics::opens_container_space; +use crate::metrics::opens_member_scope; use crate::node::Node; use crate::*; @@ -32,10 +32,14 @@ use crate::*; /// This metric counts the number of public attributes /// of classes/interfaces. /// -/// It is emitted only on *container* spaces — class, struct, trait, -/// impl, namespace and interface — so a serialized function space and -/// the file root carry no `npa` block at all. The enable predicate is -/// `metrics::opens_container_space`. +/// Emitted on container spaces and on the file unit that rolls them up, +/// never on a function space — the rule `wmc` also follows, spelled once +/// as `SpaceKind::is_member_scope`. Each language decides *when* to set +/// the flag: the ten that route through `metrics::opens_member_scope` +/// obey the rule for every node, while Python, Rust, C++, Mozcpp, Go, +/// Objective-C and Elixir gate on their own node kinds and may still +/// enable a space the shared predicate would not (Go's file root, for +/// one). #[derive(Clone, Debug, Default, PartialEq)] #[non_exhaustive] pub struct Stats { @@ -220,17 +224,23 @@ impl Stats { !self.is_class_space } - /// Enables `Npa` on the space `node` opens, if that space is a - /// container (#1197). + /// Enables `Npa` on the space `node` opens, when that space is a + /// member scope — a container, or the file unit that rolls its + /// containers up (#1197). /// /// Idempotent by design: `compute` runs once per node, so the first - /// container node to reach a given space wins and every later call is + /// qualifying node to reach a given space wins and every later call is /// a no-op. The languages that gate on a bespoke node-kind set — /// Python, Rust, C++, Mozcpp, Go, Objective-C, Elixir — set the flag /// themselves and do not route through here. #[inline] - fn enable_for_container(&mut self, node: &Node) { - if self.is_disabled() && opens_container_space::(node) { + fn enable_for_member_scope<'a, L: Checker + Getter>( + &mut self, + node: &Node<'a>, + code: &[u8], + ancestors: Ancestors<'a, '_>, + ) { + if self.is_disabled() && opens_member_scope::(node, code, ancestors) { self.is_class_space = true; } } @@ -272,7 +282,7 @@ where fn compute<'a>( node: &Node<'a>, code: &'a [u8], - _ancestors: Ancestors<'a, '_>, + ancestors: Ancestors<'a, '_>, stats: &mut Stats, ); } @@ -306,13 +316,13 @@ macro_rules! impl_npa_java_like { impl Npa for $code { fn compute<'a>( node: &Node<'a>, - _code: &'a [u8], - _ancestors: Ancestors<'a, '_>, + code: &'a [u8], + ancestors: Ancestors<'a, '_>, stats: &mut Stats, ) { use $lang::*; - stats.enable_for_container::(node); + stats.enable_for_member_scope::(node, code, ancestors); match node.kind_id().into() { ClassBody | EnumBodyDeclarations => { @@ -393,13 +403,13 @@ macro_rules! ts_npa_compute { ($lang:ident) => { fn compute<'a>( node: &Node<'a>, - _code: &'a [u8], - _ancestors: Ancestors<'a, '_>, + code: &'a [u8], + ancestors: Ancestors<'a, '_>, stats: &mut Stats, ) { use $lang::*; - stats.enable_for_container::(node); + stats.enable_for_member_scope::(node, code, ancestors); match node.kind_id().into() { ClassBody => { @@ -513,13 +523,13 @@ macro_rules! js_npa_compute { ($lang:ident) => { fn compute<'a>( node: &Node<'a>, - _code: &'a [u8], - _ancestors: Ancestors<'a, '_>, + code: &'a [u8], + ancestors: Ancestors<'a, '_>, stats: &mut Stats, ) { use $lang::*; - stats.enable_for_container::(node); + stats.enable_for_member_scope::(node, code, ancestors); if !matches!(node.kind_id().into(), ClassBody) { return; diff --git a/src/metrics/npa/csharp.rs b/src/metrics/npa/csharp.rs index 13e0ef78d..9452c0d42 100644 --- a/src/metrics/npa/csharp.rs +++ b/src/metrics/npa/csharp.rs @@ -11,13 +11,13 @@ use super::*; impl Npa for CsharpCode { fn compute<'a>( node: &Node<'a>, - _code: &'a [u8], + code: &'a [u8], ancestors: Ancestors<'a, '_>, stats: &mut Stats, ) { use Csharp::*; - stats.enable_for_container::(node); + stats.enable_for_member_scope::(node, code, ancestors); // Class / struct / record / interface bodies all share // `DeclarationList`; the parent kind disambiguates. diff --git a/src/metrics/npa/groovy.rs b/src/metrics/npa/groovy.rs index 90d14e167..5bfbb4488 100644 --- a/src/metrics/npa/groovy.rs +++ b/src/metrics/npa/groovy.rs @@ -21,13 +21,13 @@ use super::*; impl Npa for GroovyCode { fn compute<'a>( node: &Node<'a>, - _code: &'a [u8], + code: &'a [u8], ancestors: Ancestors<'a, '_>, stats: &mut Stats, ) { use Groovy::*; - stats.enable_for_container::(node); + stats.enable_for_member_scope::(node, code, ancestors); match node.kind_id().into() { ClassBody | EnumBody => { diff --git a/src/metrics/npa/kotlin.rs b/src/metrics/npa/kotlin.rs index 0a394cc7d..feb2ef16d 100644 --- a/src/metrics/npa/kotlin.rs +++ b/src/metrics/npa/kotlin.rs @@ -32,7 +32,7 @@ fn kotlin_count_property_attrs(decl: &Node) -> usize { impl Npa for KotlinCode { fn compute<'a>( node: &Node<'a>, - _code: &'a [u8], + code: &'a [u8], ancestors: Ancestors<'a, '_>, stats: &mut Stats, ) { @@ -40,7 +40,7 @@ impl Npa for KotlinCode { // Enables the `Npa` metric for both class and interface spaces // (and `object` singletons, which `Getter` reports as `Class`). - stats.enable_for_container::(node); + stats.enable_for_member_scope::(node, code, ancestors); match node.kind_id().into() { // A `ClassParameter` carrying `val` / `var` is a Kotlin diff --git a/src/metrics/npa/php.rs b/src/metrics/npa/php.rs index 45ad3aa5e..0052d12fb 100644 --- a/src/metrics/npa/php.rs +++ b/src/metrics/npa/php.rs @@ -11,13 +11,13 @@ use super::*; impl Npa for PhpCode { fn compute<'a>( node: &Node<'a>, - _code: &'a [u8], + code: &'a [u8], ancestors: Ancestors<'a, '_>, stats: &mut Stats, ) { use Php::*; - stats.enable_for_container::(node); + stats.enable_for_member_scope::(node, code, ancestors); // Class / trait / anonymous-class / interface bodies all share // the `DeclarationList` kind; the parent kind disambiguates. diff --git a/src/metrics/npa/ruby.rs b/src/metrics/npa/ruby.rs index daf96376e..957fda7e6 100644 --- a/src/metrics/npa/ruby.rs +++ b/src/metrics/npa/ruby.rs @@ -17,7 +17,7 @@ impl Npa for RubyCode { ) { use Ruby::*; - stats.enable_for_container::(node); + stats.enable_for_member_scope::(node, code, ancestors); if !matches!(node.kind_id().into(), BodyStatement | BodyStatement2) { return; diff --git a/src/metrics/npm.rs b/src/metrics/npm.rs index 0421eb3cb..b02307d6a 100644 --- a/src/metrics/npm.rs +++ b/src/metrics/npm.rs @@ -24,7 +24,7 @@ use crate::getter::Getter; use crate::langs::*; use crate::macros::implement_metric_trait; use crate::metrics::npa::{accessibility_ratio, python_is_block, ts_member_is_public}; -use crate::metrics::opens_container_space; +use crate::metrics::opens_member_scope; use crate::node::Node; use crate::*; @@ -33,10 +33,14 @@ use crate::*; /// This metric counts the number of public methods /// of classes/interfaces. /// -/// It is emitted only on *container* spaces — class, struct, trait, -/// impl, namespace and interface — so a serialized function space and -/// the file root carry no `npm` block at all. The enable predicate is -/// `metrics::opens_container_space`. +/// Emitted on container spaces and on the file unit that rolls them up, +/// never on a function space — the rule `wmc` also follows, spelled once +/// as `SpaceKind::is_member_scope`. Each language decides *when* to set +/// the flag: the ten that route through `metrics::opens_member_scope` +/// obey the rule for every node, while Python, Rust, C++, Mozcpp, Go, +/// Objective-C and Elixir gate on their own node kinds and may still +/// enable a space the shared predicate would not (Go's file root, for +/// one). #[derive(Clone, Debug, Default, PartialEq)] #[non_exhaustive] pub struct Stats { @@ -221,17 +225,23 @@ impl Stats { !self.is_class_space } - /// Enables `Npm` on the space `node` opens, if that space is a - /// container (#1197). + /// Enables `Npm` on the space `node` opens, when that space is a + /// member scope — a container, or the file unit that rolls its + /// containers up (#1197). /// /// Idempotent by design: `compute` runs once per node, so the first - /// container node to reach a given space wins and every later call is + /// qualifying node to reach a given space wins and every later call is /// a no-op. The languages that gate on a bespoke node-kind set — /// Python, Rust, C++, Mozcpp, Go, Objective-C, Elixir — set the flag /// themselves and do not route through here. #[inline] - fn enable_for_container(&mut self, node: &Node) { - if self.is_disabled() && opens_container_space::(node) { + fn enable_for_member_scope<'a, L: Checker + Getter>( + &mut self, + node: &Node<'a>, + code: &[u8], + ancestors: Ancestors<'a, '_>, + ) { + if self.is_disabled() && opens_member_scope::(node, code, ancestors) { self.is_class_space = true; } } @@ -277,7 +287,7 @@ where fn compute<'a>( node: &Node<'a>, code: &'a [u8], - _ancestors: Ancestors<'a, '_>, + ancestors: Ancestors<'a, '_>, stats: &mut Stats, ); } @@ -304,13 +314,13 @@ macro_rules! impl_npm_java_like { impl Npm for $code { fn compute<'a>( node: &Node<'a>, - _code: &'a [u8], - _ancestors: Ancestors<'a, '_>, + code: &'a [u8], + ancestors: Ancestors<'a, '_>, stats: &mut Stats, ) { use $lang::*; - stats.enable_for_container::(node); + stats.enable_for_member_scope::(node, code, ancestors); match node.kind_id().into() { ClassBody | EnumBodyDeclarations => { @@ -377,13 +387,13 @@ macro_rules! ts_npm_compute { ($lang:ident) => { fn compute<'a>( node: &Node<'a>, - _code: &'a [u8], - _ancestors: Ancestors<'a, '_>, + code: &'a [u8], + ancestors: Ancestors<'a, '_>, stats: &mut Stats, ) { use $lang::*; - stats.enable_for_container::(node); + stats.enable_for_member_scope::(node, code, ancestors); match node.kind_id().into() { ClassBody => { @@ -453,13 +463,13 @@ macro_rules! js_npm_compute { ($lang:ident) => { fn compute<'a>( node: &Node<'a>, - _code: &'a [u8], - _ancestors: Ancestors<'a, '_>, + code: &'a [u8], + ancestors: Ancestors<'a, '_>, stats: &mut Stats, ) { use $lang::*; - stats.enable_for_container::(node); + stats.enable_for_member_scope::(node, code, ancestors); if !matches!(node.kind_id().into(), ClassBody) { return; diff --git a/src/metrics/npm/csharp.rs b/src/metrics/npm/csharp.rs index 091ca6307..63fd20e91 100644 --- a/src/metrics/npm/csharp.rs +++ b/src/metrics/npm/csharp.rs @@ -53,13 +53,13 @@ fn csharp_member_public_method_count(member: &Node) -> usize { impl Npm for CsharpCode { fn compute<'a>( node: &Node<'a>, - _code: &'a [u8], + code: &'a [u8], ancestors: Ancestors<'a, '_>, stats: &mut Stats, ) { use Csharp::*; - stats.enable_for_container::(node); + stats.enable_for_member_scope::(node, code, ancestors); if !matches!(node.kind_id().into(), DeclarationList) { return; diff --git a/src/metrics/npm/groovy.rs b/src/metrics/npm/groovy.rs index 6cd27bc46..a04cfec34 100644 --- a/src/metrics/npm/groovy.rs +++ b/src/metrics/npm/groovy.rs @@ -18,14 +18,14 @@ use super::*; impl Npm for GroovyCode { fn compute<'a>( node: &Node<'a>, - _code: &'a [u8], + code: &'a [u8], ancestors: Ancestors<'a, '_>, stats: &mut Stats, ) { use crate::metrics::npa::{groovy_body_is_interface_like, groovy_has_explicit_public}; use Groovy::*; - stats.enable_for_container::(node); + stats.enable_for_member_scope::(node, code, ancestors); match node.kind_id().into() { ClassBody | EnumBody => { diff --git a/src/metrics/npm/kotlin.rs b/src/metrics/npm/kotlin.rs index 409ee696b..ab2532a6b 100644 --- a/src/metrics/npm/kotlin.rs +++ b/src/metrics/npm/kotlin.rs @@ -17,13 +17,13 @@ use super::*; impl Npm for KotlinCode { fn compute<'a>( node: &Node<'a>, - _code: &'a [u8], + code: &'a [u8], ancestors: Ancestors<'a, '_>, stats: &mut Stats, ) { use Kotlin::*; - stats.enable_for_container::(node); + stats.enable_for_member_scope::(node, code, ancestors); // Each `ClassBody` contributes its direct `FunctionDeclaration` // and `SecondaryConstructor` children to whichever func_space is diff --git a/src/metrics/npm/php.rs b/src/metrics/npm/php.rs index 0242fdf22..bc4be253e 100644 --- a/src/metrics/npm/php.rs +++ b/src/metrics/npm/php.rs @@ -11,7 +11,7 @@ use super::*; impl Npm for PhpCode { fn compute<'a>( node: &Node<'a>, - _code: &'a [u8], + code: &'a [u8], ancestors: Ancestors<'a, '_>, stats: &mut Stats, ) { @@ -24,7 +24,7 @@ impl Npm for PhpCode { // sibling `Abc::compute` carries a marker here. use Php::*; - stats.enable_for_container::(node); + stats.enable_for_member_scope::(node, code, ancestors); match node.kind_id().into() { DeclarationList => { diff --git a/src/metrics/npm/ruby.rs b/src/metrics/npm/ruby.rs index 9d0aafdb2..0e3e3f7aa 100644 --- a/src/metrics/npm/ruby.rs +++ b/src/metrics/npm/ruby.rs @@ -28,7 +28,7 @@ impl Npm for RubyCode { ) { use Ruby::*; - stats.enable_for_container::(node); + stats.enable_for_member_scope::(node, code, ancestors); if !matches!(node.kind_id().into(), BodyStatement | BodyStatement2) { return; diff --git a/src/metrics/wmc.rs b/src/metrics/wmc.rs index c60abfc7a..d5d7d4a09 100644 --- a/src/metrics/wmc.rs +++ b/src/metrics/wmc.rs @@ -143,7 +143,7 @@ impl Stats { // Checks if the `Wmc` metric is disabled #[inline] pub(crate) fn is_disabled(&self) -> bool { - matches!(self.space_kind, SpaceKind::Function | SpaceKind::Unknown) + !self.space_kind.is_member_scope() } } diff --git a/src/spaces/space_kind.rs b/src/spaces/space_kind.rs index 0ab09933d..bc922eb62 100644 --- a/src/spaces/space_kind.rs +++ b/src/spaces/space_kind.rs @@ -33,6 +33,31 @@ impl SpaceKind { _ => Self::Unknown, } } + + /// Whether the object-oriented member metrics — `wmc`, `npm`, `npa` — + /// are meaningful on a space of this kind. + /// + /// True for every container (which owns methods and attributes) and + /// for the file [`Unit`](SpaceKind::Unit) (which aggregates its + /// containers' counts into a whole-file roll-up). False for a + /// function space, which owns neither, and for + /// [`Unknown`](SpaceKind::Unknown). + /// + /// This is the single definition the three metrics share. `wmc` + /// carried it alone as an inline `matches!`; `npm` and `npa` enabled + /// themselves from `Checker::is_func_space` instead, which admits + /// function spaces and so gave a Kotlin `` or a JavaScript method + /// an all-zero block its sibling method did not have (#1197). + /// + /// Phrased as an exclusion so that a future [`SpaceKind`] variant — + /// the enum is `#[non_exhaustive]` — defaults to *carrying* the + /// metrics. A new kind is far likelier to be another container than + /// another callable, and an extra roll-up is a milder wrong answer + /// than a silently missing one. + #[must_use] + pub(crate) fn is_member_scope(self) -> bool { + !matches!(self, Self::Function | Self::Unknown) + } } impl fmt::Display for SpaceKind { diff --git a/tests/repositories/big-code-analysis-output b/tests/repositories/big-code-analysis-output index 8196e73ae..f75791311 160000 --- a/tests/repositories/big-code-analysis-output +++ b/tests/repositories/big-code-analysis-output @@ -1 +1 @@ -Subproject commit 8196e73aed4e7001ca08cd65b744f21a5d334cbd +Subproject commit f757913110dc8c9c9784deb26accc8a52c029d51 From 6ae9bba8f78d1fcc49b965bf45cb832a6cdcf0d5 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Mon, 3 Aug 2026 20:55:27 -0700 Subject: [PATCH 5/6] test(py): assert the function-space elision against JavaScript `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 ``, 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. --- .../tests/test_metrics_select.py | 39 ++++++++++++++++--- 1 file changed, 34 insertions(+), 5 deletions(-) diff --git a/big-code-analysis-py/tests/test_metrics_select.py b/big-code-analysis-py/tests/test_metrics_select.py index ac95cbb83..13b10d10b 100644 --- a/big-code-analysis-py/tests/test_metrics_select.py +++ b/big-code-analysis-py/tests/test_metrics_select.py @@ -527,15 +527,39 @@ def test_class_only_metrics_skip_function_spaces() -> None: The root-level assertion in ``test_metric_names_round_trip_through_analyze`` cannot see this: it reads only the top-level space, which legitimately carries the - whole-file roll-up. Before #1197 every Java method space carried an - all-zero block of its own. + whole-file roll-up. + + Uses **JavaScript**, not the ``Hello.java`` fixture. Java's + ``is_func_space`` never listed ``method_declaration``, so a Java + method carried no block even before the fix and asserting its + absence would hold either way. JavaScript's did list every function + form, so each of the three function spaces below — a class method, + a static block, and a top-level function — really did carry an + all-zero ``npm``/``npa`` block in 2.0.x. """ - result = bca.analyze(FIXTURES / "Hello.java") + result = bca.analyze_source( + "class C {\n" + " a = 1;\n" + " static { this.b = 2; }\n" + " m() { return this.a; }\n" + "}\n" + "function top(x) { return x; }\n", + "javascript", + ) assert result is not None assert {"npa", "npm"} <= _metrics_keys(result), "root keeps its roll-up" - functions = [s for s in _walk(result) if s["kind"] == "function"] - assert functions, "the Java fixture must contain at least one method" + spaces = _walk(result) + classes = [s for s in spaces if s["kind"] == "class"] + assert len(classes) == 1, f"expected one class space, got {_shape(spaces)}" + assert {"npa", "npm"} <= set(classes[0]["metrics"].keys()), ( + "the container itself must still carry both blocks" + ) + + functions = [s for s in spaces if s["kind"] == "function"] + assert len(functions) == 3, ( + f"expected m, and top as function spaces, got {_shape(spaces)}" + ) for space in functions: keys = set(space["metrics"].keys()) assert not ({"npa", "npm"} & keys), ( @@ -543,6 +567,11 @@ def test_class_only_metrics_skip_function_spaces() -> None: ) +def _shape(spaces: list[FuncSpaceDict]) -> list[tuple[str, str | None]]: + """``(kind, name)`` per space, for an assertion message.""" + return [(s["kind"], s["name"]) for s in spaces] + + def _walk(space: FuncSpaceDict) -> list[FuncSpaceDict]: """Every space in the tree, root first.""" out = [space] From 2203b194c0a416c0917ce0665ee6f8604daba968 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Tue, 4 Aug 2026 05:01:21 -0700 Subject: [PATCH 6/6] docs(npm): correct the Go and Rust deviation, fix the anchor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- STABILITY.md | 3 ++- big-code-analysis-book/src/metrics.md | 26 +++++++++++++++----------- src/metrics/container_scope_tests.rs | 2 +- src/metrics/mod.rs | 3 +-- src/metrics/npa.rs | 7 ++++--- src/metrics/npm.rs | 7 ++++--- 6 files changed, 27 insertions(+), 21 deletions(-) diff --git a/STABILITY.md b/STABILITY.md index 8a5733ebd..61867b5cb 100644 --- a/STABILITY.md +++ b/STABILITY.md @@ -1076,7 +1076,8 @@ dicts, byte-identical to the CLI output), so it only narrows the static type. Every metric block is `NotRequired` because a `metrics=` selection can elide blocks, and because the object-oriented blocks are scope-gated: `wmc`, `npm` and `npa` are emitted on container spaces and -on the file root, never on a function space (#1197), and a language +on the file root, and — outside Rust and Go, which gate `npm` / `npa` +on their own node kinds — not on a function space (#1197). A language with no class-like construct emits them nowhere. Which spaces carry which block is not part of the shape contract — the `wire` struct definitions are. The VCS *report* dicts are now single-sourced and typed too diff --git a/big-code-analysis-book/src/metrics.md b/big-code-analysis-book/src/metrics.md index c9db27be6..dfe738a54 100644 --- a/big-code-analysis-book/src/metrics.md +++ b/big-code-analysis-book/src/metrics.md @@ -917,22 +917,26 @@ decides what counts as "public" (Java `public`, C# `public`, Rust `pub`, Python's "no leading underscore" convention, …) and what counts as "attribute" rather than "method". -NPA, NPM and WMC are emitted on **container spaces** {#oop-emission-scope} -— `class`, `struct`, `trait`, `impl`, `namespace`, `interface` — and on +### Which spaces carry NPA, NPM and WMC {#oop-emission-scope} + +The three object-oriented blocks are emitted on **container spaces** — +`class`, `struct`, `trait`, `impl`, `namespace`, `interface` — and on the whole-file `unit` root, which carries the roll-up across every -container in the file. A **function space never carries them**: a method -owns no methods or attributes of its own, so the block would be all -zeros. Before big-code-analysis 2.1.0, NPA and NPM did emit that +container in the file. A **function space does not carry them**: a +method owns no methods or attributes of its own, so the block would be +all zeros. Before big-code-analysis 2.1.0, NPA and NPM did emit that all-zero block on function spaces in C#, JavaScript, MozJS, TypeScript, TSX, PHP and Ruby, and on the Kotlin, Java, Groovy and JS-family accessor / `init` / `static` spaces. -Two caveats. Go and Rust set the flag from their own node kinds rather -than the shared rule, so a Go file root or a Rust file root can still -carry a block the rule above would not predict. And the CSV projection -is a fixed-column format: it writes the `npa.*` / `npm.*` columns on -**every** row regardless of space kind, carrying the real accessor -values rather than eliding them. +Two caveats. Go and Rust do not follow that rule: they set the flag +from their own node kinds, on whichever space encloses the `struct` +they are counting. So a `struct` declared inside a function puts the +block on that **function** space, and a file with no `struct` at file +scope leaves the root without one — both the opposite of what the rule +above predicts. And the CSV projection is a fixed-column format: it +writes the `npa.*` / `npm.*` columns on **every** row regardless of +space kind, carrying the real accessor values rather than eliding them. Thresholds are narrower still — `bca check` gates `npa` / `npm` on container spaces only, never the file root (see diff --git a/src/metrics/container_scope_tests.rs b/src/metrics/container_scope_tests.rs index 8f6cdbb1d..ddf6c6c53 100644 --- a/src/metrics/container_scope_tests.rs +++ b/src/metrics/container_scope_tests.rs @@ -380,7 +380,7 @@ fn the_file_root_keeps_its_rollup() { /// Every #1184 construct opens a function space that emits neither /// block, while a plain method beside it does the same. /// -/// [`function_spaces_and_file_roots_emit_neither`] would still pass if a +/// [`function_spaces_emit_neither`] would still pass if a /// grammar stopped opening these spaces at all; naming them pins that /// they exist *and* stay quiet. #[test] diff --git a/src/metrics/mod.rs b/src/metrics/mod.rs index 81e39cf0c..1a6b8af0f 100644 --- a/src/metrics/mod.rs +++ b/src/metrics/mod.rs @@ -6,8 +6,7 @@ use crate::checker::Checker; use crate::getter::Getter; -use crate::node::Ancestors; -use crate::node::Node; +use crate::node::{Ancestors, Node}; /// Assignment / Branch / Condition counts. pub mod abc; diff --git a/src/metrics/npa.rs b/src/metrics/npa.rs index 1bd3ff3a6..4a9c5b66c 100644 --- a/src/metrics/npa.rs +++ b/src/metrics/npa.rs @@ -37,9 +37,10 @@ use crate::*; /// as `SpaceKind::is_member_scope`. Each language decides *when* to set /// the flag: the ten that route through `metrics::opens_member_scope` /// obey the rule for every node, while Python, Rust, C++, Mozcpp, Go, -/// Objective-C and Elixir gate on their own node kinds and may still -/// enable a space the shared predicate would not (Go's file root, for -/// one). +/// Objective-C and Elixir gate on their own node kinds and can still +/// disagree with it in both directions — a Rust or Go `struct` declared +/// inside a function enables the block on that *function* space, and a +/// file with no `struct` at file scope leaves the unit root without one. #[derive(Clone, Debug, Default, PartialEq)] #[non_exhaustive] pub struct Stats { diff --git a/src/metrics/npm.rs b/src/metrics/npm.rs index b02307d6a..b29276273 100644 --- a/src/metrics/npm.rs +++ b/src/metrics/npm.rs @@ -38,9 +38,10 @@ use crate::*; /// as `SpaceKind::is_member_scope`. Each language decides *when* to set /// the flag: the ten that route through `metrics::opens_member_scope` /// obey the rule for every node, while Python, Rust, C++, Mozcpp, Go, -/// Objective-C and Elixir gate on their own node kinds and may still -/// enable a space the shared predicate would not (Go's file root, for -/// one). +/// Objective-C and Elixir gate on their own node kinds and can still +/// disagree with it in both directions — a Rust or Go `struct` declared +/// inside a function enables the block on that *function* space, and a +/// file with no `struct` at file scope leaves the unit root without one. #[derive(Clone, Debug, Default, PartialEq)] #[non_exhaustive] pub struct Stats {