diff --git a/CHANGELOG.md b/CHANGELOG.md index 03192f5b6..302018a36 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -766,6 +766,53 @@ for historical reference. ### Fixed +- **Serialized shape.** `npm` and `npa` emission is now decided by the + space's kind alone, for every language (#1203). #1197 declared the rule + — containers and the file `unit` root carry the block, a function space + never does — but enforced it only for the ten languages it routed + through a shared predicate. The other seven kept enabling from their + own grammar node kinds and disagreed with it in both directions: + + - A Go or Rust `struct` declared inside a function body put the block + on that **function** space. Across the `serde` corpus that was 39 + function spaces in 10 files, most of them `#[test]` functions + declaring a local `struct`. + - A C++ `namespace`, and any file root whose only container sat inside + a function, carried **no** block. In the last case the counts were + serialized nowhere at all — they reached the root's `_sum` fields, + which nothing emitted — so merely suppressing the function-space + block would have deleted them from output rather than relocating + them. + + The space kind is now the only input, recorded once per space by the + walker, so there is no per-language surface left to deviate on. + Practically: every container space and every file root of a language + with class-shaped constructs carries both blocks, and no function space + does. In this repository's integration corpora that adds a block to + 1,214 file roots and 1,332 C++ namespaces, and removes one from 39 Rust + function spaces. + + **No metric value changed.** The counts always rolled up through every + enclosing space regardless of which one serialized them; this moves + which space reports them. Thresholds are unaffected — `bca check` reads + `metrics.npm` directly through `MetricScope`, which never consulted the + emission gate. `STABILITY.md` already places which space carries which + block outside the shape contract. + + Languages with no class-shaped construct at all — Bash, C, Lua, Perl, + Tcl, iRules — still emit neither block, rather than gaining an all-zero + one on every file root. Go remains the one language whose `npm` / `npa` + appear only on the root, because its space tree has no container kind; + since `bca check` gates both on container spaces, no `npm` or `npa` + limit can fire on Go source. Both are documented in the metrics guide. + + One consequence reaches a front-end. The Python `to_sarif` binding + walks serialized JSON and skips a metric whose key is absent, so it + silently dropped every `npm` / `npa` offender on a C++ `namespace` — + a kind `MetricScope::Container` admits and the CLI has always gated, + reading the struct rather than the JSON. The two front-ends now agree, + and SARIF output may gain namespace-scoped findings it was missing. + - **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", diff --git a/STABILITY.md b/STABILITY.md index 61867b5cb..53a10ea46 100644 --- a/STABILITY.md +++ b/STABILITY.md @@ -1076,8 +1076,9 @@ 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, and — outside Rust and Go, which gate `npm` / `npa` -on their own node kinds — not on a function space (#1197). A language +on the file root, and never on a function space (#1197, #1203). Since +the latter, the space's own kind is the sole input for every language, +so no grammar deviates from that rule in either direction. 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 dfe738a54..b8a3f6edb 100644 --- a/big-code-analysis-book/src/metrics.md +++ b/big-code-analysis-book/src/metrics.md @@ -927,20 +927,35 @@ 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. +accessor / `init` / `static` spaces. In the same release Go, Rust, +Python, C++, Objective-C and Elixir went the other way: they decided +from their own grammar node kinds, so a `struct` declared inside a +function put the block on that **function** space, while a `namespace` +or a file root with no container at file scope carried none. Both +deviations are gone — the space's kind is now the only input, for every +language. -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. +That rule governs the *block*, not the numbers behind it: the counts +roll up through every enclosing space regardless. So a type declared +inside a function body is reported by the nearest enclosing container, +or by the file root when there is none. + +Two things read differently. A Go file's NPA and NPM live on the `unit` +root and nowhere else, because Go is the one language that emits them +without having a container kind in its space tree — `type … struct` and +`type … interface` open no space of their own. (Bash, C, Lua, Perl, +Tcl and iRules have no container kind either, but they emit neither +block anywhere, so the question does not arise.) 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)). +[Threshold scope](commands/check.md#threshold-scope)). Taken with the +paragraph above, that has a consequence worth stating outright: since a +Go file's NPA and NPM are only ever reported at the root, **no `npa` or +`npm` threshold can fire on Go source**. ### How to read it diff --git a/big-code-analysis-book/src/recipes/thresholds.md b/big-code-analysis-book/src/recipes/thresholds.md index f6bb59aa2..2b428797b 100644 --- a/big-code-analysis-book/src/recipes/thresholds.md +++ b/big-code-analysis-book/src/recipes/thresholds.md @@ -183,7 +183,10 @@ is therefore sparse on codebases predating `use v5.36`, and one anonymous-sub fo because the grammar misparses its signature. `wmc`, `npm`, and `npa` are only produced for languages with a class-like container. They are absent -from Bash, C, Go, Lua, Perl, and Tcl output. +from Bash, C, Lua, Perl, and Tcl output. Go is a half case: it has no container space at all, so +`wmc` is absent and `npm` / `npa` are reported only on the file `unit` root. `bca check` gates both +on container spaces and never on the root, so **no `npm` or `npa` limit can fire on Go source** — see +[Which spaces carry NPA, NPM and WMC](../metrics.md#oop-emission-scope). A per-language override cannot fix any of these. Bash `nargs` is `0` for every function, so no limit — not even `0`, which fires only on a value *above* it — can make the gate say anything. The diff --git a/big-code-analysis-py/tests/test_metrics_select.py b/big-code-analysis-py/tests/test_metrics_select.py index 13b10d10b..84825adfb 100644 --- a/big-code-analysis-py/tests/test_metrics_select.py +++ b/big-code-analysis-py/tests/test_metrics_select.py @@ -78,12 +78,14 @@ 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. + languages with no class-like construct. The unit root of a + language that has one carries all three even when the file + declares no class (#1203), so the Java fixture is no longer + required for the key to appear — it is kept so the three are + exercised on a root that merged a real container's counts. They are also elided from every *function* space, which is where - the unit root differs from a nested one (#1197); the assertion + the unit root differs from a nested one (#1197, #1203); the assertion below reads the root, and ``test_class_only_metrics_skip_function_spaces`` pins the other half. """ diff --git a/big-code-analysis-web/src/web/server_tests.rs b/big-code-analysis-web/src/web/server_tests.rs index 430f7a270..00ddcb736 100644 --- a/big-code-analysis-web/src/web/server_tests.rs +++ b/big-code-analysis-web/src/web/server_tests.rs @@ -639,7 +639,9 @@ async fn test_web_metrics_json() { "sei": 176.151_664_509_301_33, "visual_studio": 81.856_334_244_533_39}, "abc": {"assignments": 0, "branches": 0, "conditions": 0, "magnitude": 0.0, "value": 0.0, "assignments_average": 0.0, "branches_average": 0.0, "conditions_average": 0.0, "assignments_min": 0, "assignments_max": 0, "branches_min": 0, "branches_max": 0, "conditions_min": 0, "conditions_max": 0}, - "wmc": {"class_wmc_sum": 0, "interface_wmc_sum": 0, "total": 0}}, + "wmc": {"class_wmc_sum": 0, "interface_wmc_sum": 0, "total": 0}, + "npm": {"class_npm_sum": 0, "interface_npm_sum": 0, "class_methods": 0, "interface_methods": 0, "class_coa": 0.0, "interface_coa": 0.0, "total": 0, "total_methods": 0, "coa": 0.0}, + "npa": {"class_npa_sum": 0, "interface_npa_sum": 0, "class_attributes": 0, "interface_attributes": 0, "class_cda": 0.0, "interface_cda": 0.0, "total": 0, "total_attributes": 0, "cda": 0.0}}, "name": "test.py", "spaces": [{"kind": "function", "start_line": 3, @@ -730,7 +732,9 @@ async fn test_web_metrics_json_unit() { "sei": 142.643_061_717_489_76, "visual_studio": 88.422_991_744_574_97}, "abc": {"assignments": 0, "branches": 0, "conditions": 0, "magnitude": 0.0, "value": 0.0, "assignments_average": 0.0, "branches_average": 0.0, "conditions_average": 0.0, "assignments_min": 0, "assignments_max": 0, "branches_min": 0, "branches_max": 0, "conditions_min": 0, "conditions_max": 0}, - "wmc": {"class_wmc_sum": 0, "interface_wmc_sum": 0, "total": 0}}, + "wmc": {"class_wmc_sum": 0, "interface_wmc_sum": 0, "total": 0}, + "npm": {"class_npm_sum": 0, "interface_npm_sum": 0, "class_methods": 0, "interface_methods": 0, "class_coa": 0.0, "interface_coa": 0.0, "total": 0, "total_methods": 0, "coa": 0.0}, + "npa": {"class_npa_sum": 0, "interface_npa_sum": 0, "class_attributes": 0, "interface_attributes": 0, "class_cda": 0.0, "interface_cda": 0.0, "total": 0, "total_attributes": 0, "cda": 0.0}}, "name": "test.py", "spaces": []} }); @@ -834,7 +838,9 @@ async fn test_web_metrics_plain() { "sei": 142.643_061_717_489_76, "visual_studio": 88.422_991_744_574_97}, "abc": {"assignments": 0, "branches": 0, "conditions": 0, "magnitude": 0.0, "value": 0.0, "assignments_average": 0.0, "branches_average": 0.0, "conditions_average": 0.0, "assignments_min": 0, "assignments_max": 0, "branches_min": 0, "branches_max": 0, "conditions_min": 0, "conditions_max": 0}, - "wmc": {"class_wmc_sum": 0, "interface_wmc_sum": 0, "total": 0}}, + "wmc": {"class_wmc_sum": 0, "interface_wmc_sum": 0, "total": 0}, + "npm": {"class_npm_sum": 0, "interface_npm_sum": 0, "class_methods": 0, "interface_methods": 0, "class_coa": 0.0, "interface_coa": 0.0, "total": 0, "total_methods": 0, "coa": 0.0}, + "npa": {"class_npa_sum": 0, "interface_npa_sum": 0, "class_attributes": 0, "interface_attributes": 0, "class_cda": 0.0, "interface_cda": 0.0, "total": 0, "total_attributes": 0, "cda": 0.0}}, "name": "test.py", "spaces": [{"kind": "function", "start_line": 1, diff --git a/src/macros/mod.rs b/src/macros/mod.rs index 8fd59e9f4..0311a9981 100644 --- a/src/macros/mod.rs +++ b/src/macros/mod.rs @@ -70,8 +70,12 @@ macro_rules! implement_metric_trait { ); // Internal helper: shared no-op body for traits whose `compute` // signature is `<'a>(&Node<'a>, &'a [u8], Ancestors<'a, '_>, - // &mut Stats)` (Abc, Cyclomatic, Npa, Npm). Public arms below - // delegate here so the body is written once. + // &mut Stats)` (Abc, Cyclomatic). Public arms below delegate here + // so the body is written once. `Npa` and `Npm` share the signature + // but need `HAS_MEMBERS = false` as well, so they route through + // `@code_and_chain_taking_memberless` instead — reaching for this + // arm for a new no-op `Npa` / `Npm` impl would silently restore + // the all-zero file-root block #1203 removed. (@code_and_chain_taking $trait:ident, $($code:ident),+) => ( $( impl $trait for $code { @@ -96,11 +100,32 @@ macro_rules! implement_metric_trait { (Cyclomatic, $($code:ident),+) => ( implement_metric_trait!(@code_and_chain_taking Cyclomatic, $($code),+); ); + // `Npa` and `Npm` take the same shape as the arm above plus one + // thing: the no-op impl must also opt the language out of + // *emitting* the block, which `HAS_MEMBERS` does. Without it a shell + // script would report `class_npa_sum: 0`, because the file unit is a + // member scope like any other and the walker would record its kind + // (#1203). `wmc` reaches the same place by different means — its + // no-op `compute` simply never records a kind. + (@code_and_chain_taking_memberless $trait:ident, $($code:ident),+) => ( + $( + impl $trait for $code { + const HAS_MEMBERS: bool = false; + + fn compute<'a>( + _node: &Node<'a>, + _code: &'a [u8], + _ancestors: crate::Ancestors<'a, '_>, + _stats: &mut Stats, + ) {} + } + )+ + ); (Npa, $($code:ident),+) => ( - implement_metric_trait!(@code_and_chain_taking Npa, $($code),+); + implement_metric_trait!(@code_and_chain_taking_memberless Npa, $($code),+); ); (Npm, $($code:ident),+) => ( - implement_metric_trait!(@code_and_chain_taking Npm, $($code),+); + implement_metric_trait!(@code_and_chain_taking_memberless Npm, $($code),+); ); (Loc, $($code:ident),+) => ( $( diff --git a/src/metrics/container_scope_tests.rs b/src/metrics/container_scope_tests.rs index ddf6c6c53..351899b1f 100644 --- a/src/metrics/container_scope_tests.rs +++ b/src/metrics/container_scope_tests.rs @@ -15,11 +15,24 @@ //! #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. +//! The rule 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. +//! +//! #1197 left the rule a convention: it routed ten languages through a +//! shared predicate and let the other seven keep enabling from their own +//! node kinds. Those seven disagreed with it in both directions — a Go or +//! Rust `struct` declared inside a function put the block on that +//! *function* space, and a file whose only container sat inside a +//! function left the root without one, so the counts were serialized +//! nowhere. #1203 removed the choice: the space's own kind is now the +//! only input, recorded once per space by the walker's finalize step +//! (`spaces::compute::note_member_scope`). Every language is +//! covered below for that reason — the point is no longer that ten obey +//! a predicate, but that the rule has no per-language surface left to +//! deviate on. use serde_json::Value; @@ -74,8 +87,7 @@ fn flatten(value: &Value, out: &mut Vec) { } } -/// One fixture per language whose `Npm` / `Npa` impl enables on -/// [`opens_member_scope`](super::opens_member_scope). +/// One fixture per language that has an `Npm` / `Npa` impl. /// /// Each carries a container with one public method and one public /// attribute, at least one ordinary method, and — where the grammar has @@ -84,6 +96,13 @@ fn flatten(value: &Value, out: &mut Vec) { struct Fixture { lang: LANG, /// Names of the container spaces that must carry both blocks. + /// + /// Empty for Go alone, whose `Getter` has no container `SpaceKind` — + /// `type … struct` and `type … interface` open no space, so a Go + /// file's `npm` / `npa` live on the unit root and nowhere else. + /// [`containers_emit_npm_and_npa`] names Go rather than skipping an + /// empty list quietly, since a list that silently became empty for + /// any other language would make that test vacuous for it. containers: &'static [&'static str], source: &'static str, } @@ -281,6 +300,164 @@ module M end end end +", + }, + // The seven below gated on their own node kinds until #1203. Each + // fixture therefore declares a container *inside a function body* as + // well as at file scope: that is the shape whose block used to land + // on the function space, and — where the language had no other + // container — the shape whose counts reached no serialized block at + // all. + #[cfg(feature = "rust")] + Fixture { + lang: LANG::Rust, + containers: &["T", "S"], + source: "\ +pub struct S { + pub a: u8, + b: u8, +} + +pub trait T { + fn q(&self) -> u8; +} + +impl S { + pub fn m(&self) -> u8 { self.a } +} + +fn top() -> u8 { + struct Inner { pub x: u8 } + Inner { x: 1 }.x +} +", + }, + // Go is the one language with no container `SpaceKind` at all; see + // `Fixture::containers`. + #[cfg(feature = "go")] + Fixture { + lang: LANG::Go, + containers: &[], + source: "\ +package main + +type S struct { + Pub int + priv int +} + +type I interface { + Speak() string +} + +func (s S) Method() int { return s.Pub } + +func Outer() int { + type inner struct { + X int + } + return inner{X: 1}.X +} +", + }, + #[cfg(feature = "python")] + Fixture { + lang: LANG::Python, + containers: &["C", "Inner"], + source: "\ +class C: + a = 1 + + def q(self): + return self.a + +def top(x): + class Inner: + b = 2 + return Inner +", + }, + #[cfg(feature = "cpp")] + Fixture { + lang: LANG::Cpp, + containers: &["N", "C"], + source: "\ +namespace N { +class C { +public: + int a; + int q() { return a; } +}; +} + +int top() { return 0; } +", + }, + // Mozcpp owns no file extension, so nothing routes to it and it gets + // no integration-snapshot coverage at all — the case + // `.claude/rules/grammar-dispatch.md` says to pin against its + // extension-owning sibling. `cpp_mozcpp_parity` does not cover this: + // it compares metric *values* through `metric_sums`, so it would not + // notice Mozcpp losing both blocks to a stray `HAS_MEMBERS = false` + // on its impl — verified by perturbation, which fails here and + // nowhere else. (Adding Mozcpp to the `implement_metric_trait!` + // no-op list is *not* the hazard: it collides with the real impl and + // fails to compile.) Same source as the Cpp fixture, deliberately, + // so a divergence reads as one. + #[cfg(feature = "mozcpp")] + Fixture { + lang: LANG::Mozcpp, + containers: &["N", "C"], + source: "\ +namespace N { +class C { +public: + int a; + int q() { return a; } +}; +} + +int top() { return 0; } +", + }, + #[cfg(feature = "objc")] + Fixture { + lang: LANG::Objc, + // Distinct names deliberately: an `@interface C` and its + // `@implementation C` open two spaces with the *same* name, which + // `only_space` rejects. A `@protocol` carries the interface half + // instead. + containers: &["P", "C"], + source: "\ +@protocol P +- (int)r; +@end + +@implementation C { + int a; +} +- (int)q { return a; } +@end +", + }, + #[cfg(feature = "elixir")] + Fixture { + lang: LANG::Elixir, + containers: &["Outer", "Inner", "Sibling"], + source: "\ +defmodule Outer do + defstruct [:a] + def q, do: 1 + defp r, do: 2 + + defmodule Inner do + def s, do: 3 + end +end + +defmodule Sibling do + def t, do: 4 +end ", }, ]; @@ -293,13 +470,27 @@ end fn containers_emit_npm_and_npa() { assert_fixtures_present(FIXTURES); for fixture in FIXTURES { + // An empty list means every following assertion is skipped, so + // name the one language that is allowed to have one rather than + // letting a fixture go quiet by accident. + assert_eq!( + fixture.containers.is_empty(), + fixture.lang == LANG::Go, + "{:?}: only Go has no container SpaceKind", + fixture.lang + ); let spaces = emitted_spaces(fixture.lang, fixture.source); for want in fixture.containers { let space = only_space(fixture.lang, &spaces, want); assert!( matches!( space.kind, - SpaceKind::Class | SpaceKind::Interface | SpaceKind::Namespace + SpaceKind::Class + | SpaceKind::Interface + | SpaceKind::Namespace + | SpaceKind::Struct + | SpaceKind::Trait + | SpaceKind::Impl ), "{:?}: {want:?} should be a container kind, is {:?}", fixture.lang, @@ -419,12 +610,13 @@ fn the_1184_constructs_open_quiet_function_spaces() { /// 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. +/// `_sum` fields without consulting the space kind — 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] #[cfg(feature = "java")] -fn container_counts_survive_the_narrowed_enable() { +fn container_counts_are_independent_of_the_emission_gate() { let space = space_verbatim( LANG::Java, fixture_source(LANG::Java).as_bytes(), @@ -439,9 +631,197 @@ fn container_counts_survive_the_narrowed_enable() { 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. + // The roll-up reaches the root — 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); } + +/// A type declared *inside a function body* leaves that function quiet +/// and is reported by the file root instead (#1203). +/// +/// This is the shape Go and Rust got wrong. Neither language opens a +/// space for a `struct`, so its counts landed on whichever space enclosed +/// it — a `Function` space for a type declared in a function body, which +/// then serialized a block that #1197 had already ruled out everywhere +/// else. +/// +/// The roll-up half is not a formality. Go enables `npm` at the root only +/// for a file with a direct `MethodDeclaration` child, and Rust `npa` +/// only for a module-scope `struct`, so for a file whose only container +/// sits inside a function, merely *clearing* the function's block would +/// have serialized the counts nowhere at all — they survive in the root's +/// `_sum` fields either way, which is exactly why an absence-only test +/// could not tell the two outcomes apart. Both fixtures below therefore +/// assert the root's sums include the nested declaration's members. +// Gated for the same reason as +// `a_language_with_no_member_construct_emits_neither_block` below: a +// two-language case list makes `assert_fixtures_present` a false +// failure under a feature set that enables neither. +#[test] +#[cfg(any(feature = "go", feature = "rust"))] +fn a_type_declared_inside_a_function_reaches_the_root_rollup() { + // (language, the function holding the declaration, the root's + // `class_na_sum` / `class_npa_sum` once the nested type is folded in) + let cases: &[(LANG, &str, u64, u64)] = &[ + // `S{Pub, priv}` + `inner{X}` = 3 attributes, of which `Pub` and + // `X` are exported by Go's leading-uppercase rule. + #[cfg(feature = "go")] + (LANG::Go, "Outer", 3, 2), + // `S{a, b}` + `Inner{x}` = 3 fields, of which `pub a` and + // `pub x` are public. + #[cfg(feature = "rust")] + (LANG::Rust, "top", 3, 2), + ]; + assert_fixtures_present(cases); + for (lang, holder, na_sum, npa_sum) in cases { + let source = fixture_source(*lang); + + let spaces = emitted_spaces(*lang, source); + let function = only_space(*lang, &spaces, holder); + assert_eq!(function.kind, SpaceKind::Function, "{lang:?}: {holder:?}"); + assert!( + !function.has_npm && !function.has_npa, + "{lang:?}: {holder:?} holds a nested type but must not carry a block \ + (npm={}, npa={})", + function.has_npm, + function.has_npa + ); + + let root = spaces.first().expect("the root space is always emitted"); + assert!( + root.has_npm && root.has_npa, + "{lang:?}: the unit root must carry the roll-up (npm={}, npa={})", + root.has_npm, + root.has_npa + ); + + // The values behind that block, so a rule that emitted an + // all-zero root would fail here rather than pass the key check + // above. + let space = space_verbatim(*lang, source.as_bytes(), MetricsOptions::default()); + assert_eq!( + space.metrics.npa.class_na_sum(), + *na_sum, + "{lang:?}: root attributes, including the type declared in {holder:?}" + ); + assert_eq!( + space.metrics.npa.class_npa_sum(), + *npa_sum, + "{lang:?}: root public attributes, including the type declared in {holder:?}" + ); + } +} + +/// A C++ `namespace` carries both blocks. +/// +/// Namespaces are the largest population the #1203 rule moved — 1,337 of +/// them in this repository's own integration corpora, none of which +/// serialized either block before, because C++ enabled from +/// `ClassSpecifier` / `StructSpecifier` and a namespace is neither. +/// `SpaceKind::Namespace` is asserted directly rather than through +/// [`containers_emit_npm_and_npa`]'s any-container-kind check, which +/// would still pass if the grammar started reporting `N` as a class. +#[test] +#[cfg(feature = "cpp")] +fn a_cpp_namespace_is_a_member_scope() { + let spaces = emitted_spaces(LANG::Cpp, fixture_source(LANG::Cpp)); + let namespace = only_space(LANG::Cpp, &spaces, "N"); + assert_eq!(namespace.kind, SpaceKind::Namespace); + assert!( + namespace.has_npm && namespace.has_npa, + "a namespace rolls its classes up and must carry both blocks \ + (npm={}, npa={})", + namespace.has_npm, + namespace.has_npa + ); +} + +/// A language with no class-shaped construct emits no block at all. +/// +/// The file unit is a member scope like any other, so making emission +/// depend on the space kind alone would have given a shell script a +/// `class_npa_sum: 0` block on every file — noise for a grammar that +/// cannot produce anything else. `Npm::HAS_MEMBERS` / `Npa::HAS_MEMBERS` +/// keep those languages out, the way `wmc`'s no-op `compute` does by +/// never recording a kind. Asserted on the root because it is the only +/// space these fixtures have that is a member scope. +// Gated on the fixtures' own features rather than left ungated: the +// case list is three languages wide, so a feature set that enables +// others but none of these — `--no-default-features --features +// rust,typescript`, the canonical minimal-langs CI configuration — +// would trip `assert_fixtures_present` and read as a defect in +// whatever was being changed. +#[test] +#[cfg(any(feature = "bash", feature = "lua", feature = "c"))] +fn a_language_with_no_member_construct_emits_neither_block() { + let cases: &[(LANG, &str)] = &[ + #[cfg(feature = "bash")] + (LANG::Bash, "foo() { echo hi; }\nfoo\n"), + #[cfg(feature = "lua")] + (LANG::Lua, "function f(a) return a end\n"), + #[cfg(feature = "c")] + (LANG::C, "int add(int a, int b) { return a + b; }\n"), + ]; + assert_fixtures_present(cases); + for (lang, source) in cases { + let spaces = emitted_spaces(*lang, source); + let root = spaces.first().expect("the root space is always emitted"); + assert_eq!(root.kind, SpaceKind::Unit, "{lang:?}: root kind"); + assert!( + !root.has_npm && !root.has_npa, + "{lang:?}: a grammar with no member construct must emit neither \ + block (npm={}, npa={})", + root.has_npm, + root.has_npa + ); + } +} + +/// No Elixir `defmodule` counts a nested module's members twice. +/// +/// Until #1203 the Elixir impls opened with `if !stats.is_disabled() || +/// … { return; }` — a first-wins guard reusing the emission flag, which +/// went away with the flag. It was inert, because the walker pushes a +/// `defmodule`'s space before running any metric against it, so a nested +/// module never reaches its parent's stats. "Was inert" is a claim about +/// walk order rather than about this file, so it is pinned here: were the +/// guard load-bearing, `Outer` would absorb `Inner`'s members twice. +#[test] +#[cfg(feature = "elixir")] +fn nested_elixir_modules_are_not_double_counted() { + use crate::test_support::child_space; + + let root = space_verbatim( + LANG::Elixir, + fixture_source(LANG::Elixir).as_bytes(), + MetricsOptions::default(), + ); + + // `def q` + `defp r`, plus `Inner`'s `def s` once through the + // roll-up — 4 would mean `Outer` counted `s` directly as well. + // `defp` is private, so two of the three are public. + let outer = child_space(&root, "Outer"); + assert_eq!(outer.metrics.npm.class_nm_sum(), 3, "Outer: q, r, Inner::s"); + assert_eq!(outer.metrics.npm.class_npm_sum(), 2, "Outer: q, Inner::s"); + // `defstruct [:a]`, and Elixir struct fields are all public. + assert_eq!(outer.metrics.npa.class_na_sum(), 1, "Outer: defstruct :a"); + + let inner = child_space(outer, "Inner"); + assert_eq!(inner.metrics.npm.class_nm_sum(), 1, "Inner: s"); + assert_eq!( + inner.metrics.npa.class_na_sum(), + 0, + "Inner has no defstruct" + ); + + // A sibling module at file scope, which a guard that fired once per + // *file* rather than once per space would have silenced. + let sibling = child_space(&root, "Sibling"); + assert_eq!(sibling.metrics.npm.class_nm_sum(), 1, "Sibling: t"); + + // Four methods across three modules, three of them public. + assert_eq!(root.metrics.npm.class_nm_sum(), 4, "q, r, Inner::s, t"); + assert_eq!(root.metrics.npm.class_npm_sum(), 3, "q, Inner::s, t"); +} diff --git a/src/metrics/mod.rs b/src/metrics/mod.rs index 1a6b8af0f..2cd2ac6a6 100644 --- a/src/metrics/mod.rs +++ b/src/metrics/mod.rs @@ -4,10 +4,6 @@ //! 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::node::{Ancestors, Node}; - /// Assignment / Branch / Condition counts. pub mod abc; /// Cognitive complexity. @@ -73,41 +69,3 @@ mod container_scope_tests; pub(crate) fn average(sum: f64, count: usize) -> f64 { sum / count.max(1) as f64 } - -/// 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 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]). -/// -/// 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 `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. -/// -/// [#1184]: https://github.com/dekobon/big-code-analysis/issues/1184 -/// [#1197]: https://github.com/dekobon/big-code-analysis/issues/1197 -#[inline] -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 4a9c5b66c..de9e3c8c2 100644 --- a/src/metrics/npa.rs +++ b/src/metrics/npa.rs @@ -20,10 +20,8 @@ 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_member_scope; use crate::node::Node; use crate::*; @@ -32,15 +30,24 @@ use crate::*; /// This metric counts the number of public attributes /// of classes/interfaces. /// -/// 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 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. +/// Emitted on container spaces — [`SpaceKind::Class`], `Struct`, +/// `Trait`, `Impl`, `Namespace`, `Interface` — and on the +/// [`SpaceKind::Unit`] file root that rolls them up. Never on a +/// [`SpaceKind::Function`] space, which owns no members of its own. +/// +/// Since [#1203] that holds by construction rather than by convention: +/// the space's own kind is the only input, so no language can disagree +/// with it in either direction. [`Wmc`](crate::wmc::Stats) decides the +/// same way. A language with no class-shaped construct at all — C, Bash, +/// Lua, Perl, Tcl — emits no block anywhere rather than an all-zero one +/// on each file root. +/// +/// The rule governs the *block*, not the counts behind it: those roll up +/// through every enclosing space regardless, so a type declared inside a +/// function body is reported by the nearest enclosing container, or by +/// the file root when there is none. +/// +/// [#1203]: https://github.com/dekobon/big-code-analysis/issues/1203 #[derive(Clone, Debug, Default, PartialEq)] #[non_exhaustive] pub struct Stats { @@ -52,7 +59,7 @@ pub struct Stats { interface_npa_sum: usize, class_na_sum: usize, interface_na_sum: usize, - is_class_space: bool, + space_kind: SpaceKind, } impl fmt::Display for Stats { @@ -219,31 +226,21 @@ impl Stats { self.interface_na_sum += self.interface_na; } - // Checks if the `Npa` metric is disabled + /// Records the kind of the space these stats describe, which is the + /// sole input to [`Self::is_disabled`]. + /// + /// Called once per space from the walker's finalize step, beside the + /// equivalent `wmc` call. Left unset — and so reported disabled — for + /// a language whose `HAS_MEMBERS` is `false`. #[inline] - pub(crate) fn is_disabled(&self) -> bool { - !self.is_class_space + pub(crate) fn set_space_kind(&mut self, kind: SpaceKind) { + self.space_kind = kind; } - /// 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 - /// 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. + // Checks if the `Npa` metric is disabled #[inline] - 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; - } + pub(crate) fn is_disabled(&self) -> bool { + !self.space_kind.is_member_scope() } } @@ -263,6 +260,17 @@ pub(crate) trait Npa where Self: Checker, { + /// Whether this language has any construct that owns members. + /// + /// `false` only for the no-op impls — grammars with no class-shaped + /// construct at all (C, Bash, Perl, Lua, Tcl, iRules, and the two + /// comment/preprocessor grammars), where the metric could report + /// nothing but zeros. The walker consults it before recording a + /// space kind, so those languages emit no block rather than an + /// all-zero one on every file root (#1203). `wmc` gets the same + /// outcome from its no-op `compute`, which never records a kind. + const HAS_MEMBERS: bool = true; + /// Walk `node` and update `stats` with this metric for the language /// implementing the trait. /// @@ -317,14 +325,12 @@ 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_member_scope::(node, code, ancestors); - match node.kind_id().into() { ClassBody | EnumBodyDeclarations => { for declaration in node @@ -404,14 +410,12 @@ 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_member_scope::(node, code, ancestors); - match node.kind_id().into() { ClassBody => { for member in node.children() { @@ -524,14 +528,12 @@ 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_member_scope::(node, code, ancestors); - if !matches!(node.kind_id().into(), ClassBody) { return; } diff --git a/src/metrics/npa/cpp.rs b/src/metrics/npa/cpp.rs index 6254f3bcb..b694319f7 100644 --- a/src/metrics/npa/cpp.rs +++ b/src/metrics/npa/cpp.rs @@ -17,13 +17,6 @@ impl Npa for CppCode { ) { use Cpp::*; - // Mark class / struct spaces as class spaces so the metric is - // emitted on them. - if matches!(node.kind_id().into(), ClassSpecifier | StructSpecifier) && stats.is_disabled() - { - stats.is_class_space = true; - } - if !matches!(node.kind_id().into(), FieldDeclarationList) { return; } diff --git a/src/metrics/npa/csharp.rs b/src/metrics/npa/csharp.rs index 9452c0d42..391dc75ac 100644 --- a/src/metrics/npa/csharp.rs +++ b/src/metrics/npa/csharp.rs @@ -11,14 +11,12 @@ 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_member_scope::(node, code, ancestors); - // Class / struct / record / interface bodies all share // `DeclarationList`; the parent kind disambiguates. if !matches!(node.kind_id().into(), DeclarationList) { diff --git a/src/metrics/npa/elixir.rs b/src/metrics/npa/elixir.rs index d52693861..50e27305f 100644 --- a/src/metrics/npa/elixir.rs +++ b/src/metrics/npa/elixir.rs @@ -31,6 +31,11 @@ impl Npa for ElixirCode { ) { use crate::metrics::cognitive::{elixir_call_keyword, elixir_do_block_call_children}; + // The space-opening node for a `defmodule` Call is the node + // itself, and the walker pushes that space before running any + // metric against it, so a nested `defmodule` is counted by its + // own module and never reaches this one's stats. + // // `is_func_space_with_code` is not consulted: it is implied. // `elixir_is_class_macro` is exactly `kw == "defmodule"`, so it // answers `true` for every node this check lets through, and for @@ -39,12 +44,10 @@ impl Npa for ElixirCode { // rejects the node anyway. Calling it cost a source-text keyword // scan per node plus, before #1088, an `O(depth)` climb, and its // answer was discarded either way (#1088). - if !stats.is_disabled() || !matches!(elixir_call_keyword(node, code), Some("defmodule")) { + if !matches!(elixir_call_keyword(node, code), Some("defmodule")) { return; } - stats.is_class_space = true; - for stmt in elixir_do_block_call_children(node) { if matches!(elixir_call_keyword(&stmt, code), Some("defstruct")) { let fields = count_defstruct_fields(&stmt); diff --git a/src/metrics/npa/go.rs b/src/metrics/npa/go.rs index a3fc27ea4..7d9a14881 100644 --- a/src/metrics/npa/go.rs +++ b/src/metrics/npa/go.rs @@ -109,13 +109,6 @@ impl Npa for GoCode { public += exported; } - if total == 0 { - return; - } - - if stats.is_disabled() { - stats.is_class_space = true; - } stats.class_na += total; stats.class_npa += public; } diff --git a/src/metrics/npa/groovy.rs b/src/metrics/npa/groovy.rs index 5bfbb4488..a1bf46e37 100644 --- a/src/metrics/npa/groovy.rs +++ b/src/metrics/npa/groovy.rs @@ -21,14 +21,12 @@ 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_member_scope::(node, code, ancestors); - match node.kind_id().into() { ClassBody | EnumBody => { let is_interface_like = groovy_body_is_interface_like(node, ancestors); diff --git a/src/metrics/npa/kotlin.rs b/src/metrics/npa/kotlin.rs index feb2ef16d..5bd8094a2 100644 --- a/src/metrics/npa/kotlin.rs +++ b/src/metrics/npa/kotlin.rs @@ -32,16 +32,12 @@ 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, ) { use Kotlin::*; - // Enables the `Npa` metric for both class and interface spaces - // (and `object` singletons, which `Getter` reports as `Class`). - stats.enable_for_member_scope::(node, code, ancestors); - match node.kind_id().into() { // A `ClassParameter` carrying `val` / `var` is a Kotlin // primary-constructor parameter property — counts once toward diff --git a/src/metrics/npa/mozcpp.rs b/src/metrics/npa/mozcpp.rs index 1b8421442..ff833d21a 100644 --- a/src/metrics/npa/mozcpp.rs +++ b/src/metrics/npa/mozcpp.rs @@ -17,13 +17,6 @@ impl Npa for MozcppCode { ) { use Mozcpp::*; - // Mark class / struct spaces as class spaces so the metric is - // emitted on them. - if matches!(node.kind_id().into(), ClassSpecifier | StructSpecifier) && stats.is_disabled() - { - stats.is_class_space = true; - } - if !matches!(node.kind_id().into(), FieldDeclarationList) { return; } diff --git a/src/metrics/npa/objc.rs b/src/metrics/npa/objc.rs index 82c143c28..f6af819e5 100644 --- a/src/metrics/npa/objc.rs +++ b/src/metrics/npa/objc.rs @@ -51,8 +51,7 @@ fn objc_count_instance_variables(block: &Node) -> (usize, usize) { // Members are direct children of the class node (a `@protocol` nests its // post-`@required` / `@optional` members under a // `qualified_protocol_interface_declaration`), so we walk them when the -// class node itself is visited — the same point `is_class_space` is -// marked (mirroring the C++ impl's marking step). +// class node itself is visited, where `stats` is already the class space. impl Npa for ObjcCode { fn compute<'a>( node: &Node<'a>, @@ -66,9 +65,6 @@ impl Npa for ObjcCode { if !is_interface && node.kind_id() != ClassImplementation as u16 { return; } - if stats.is_disabled() { - stats.is_class_space = true; - } let (mut attributes, mut public) = (0usize, 0usize); for child in node.children() { match child.kind_id().into() { diff --git a/src/metrics/npa/php.rs b/src/metrics/npa/php.rs index 0052d12fb..baf0f1b96 100644 --- a/src/metrics/npa/php.rs +++ b/src/metrics/npa/php.rs @@ -11,14 +11,12 @@ 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_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/python.rs b/src/metrics/npa/python.rs index 8bbff4c4d..7c1a3ebd5 100644 --- a/src/metrics/npa/python.rs +++ b/src/metrics/npa/python.rs @@ -48,12 +48,6 @@ impl Npa for PythonCode { return; } - // Mark the current space as a class space so the metric is - // emitted (otherwise it is suppressed by `is_disabled`). - if stats.is_disabled() { - stats.is_class_space = true; - } - let Some(body) = python_class_body(node) else { return; }; diff --git a/src/metrics/npa/ruby.rs b/src/metrics/npa/ruby.rs index 957fda7e6..d8e8be8e7 100644 --- a/src/metrics/npa/ruby.rs +++ b/src/metrics/npa/ruby.rs @@ -17,8 +17,6 @@ impl Npa for RubyCode { ) { use Ruby::*; - stats.enable_for_member_scope::(node, code, ancestors); - if !matches!(node.kind_id().into(), BodyStatement | BodyStatement2) { return; } diff --git a/src/metrics/npa/rust.rs b/src/metrics/npa/rust.rs index a01bcea87..a16195415 100644 --- a/src/metrics/npa/rust.rs +++ b/src/metrics/npa/rust.rs @@ -57,12 +57,6 @@ impl Npa for RustCode { ) { use Rust::*; - // Mark Impl / Trait spaces as class spaces so the metric is - // emitted on them. - if matches!(node.kind_id().into(), ImplItem | TraitItem) && stats.is_disabled() { - stats.is_class_space = true; - } - match node.kind_id().into() { // Counted on the StructItem so each struct's fields are // tallied exactly once. The enclosing func_space (Unit or @@ -83,9 +77,10 @@ impl Npa for RustCode { } // Counts the fields of a Rust `struct_item` and records them on the -// enclosing func_space. Empty structs (`attrs == 0`) record nothing and -// do not mark the space as a class space, so a fieldless marker struct -// never emits a spurious npa metric. +// enclosing space. A struct declared inside a function body lands on +// that function's stats and reaches the nearest enclosing container — +// or the file root — through the usual merge; which space *serializes* +// the block is decided later, by its kind alone (#1203). fn rust_count_struct_attrs(node: &Node, stats: &mut Stats) { use Rust::*; @@ -123,13 +118,8 @@ fn rust_count_struct_attrs(node: &Node, stats: &mut Stats) { _ => {} } } - if attrs > 0 { - if stats.is_disabled() { - stats.is_class_space = true; - } - stats.class_na += attrs; - stats.class_npa += public_attrs; - } + stats.class_na += attrs; + stats.class_npa += public_attrs; } // Counts an associated `const`/`static` declared directly in an `impl` diff --git a/src/metrics/npm.rs b/src/metrics/npm.rs index b29276273..f9383deb6 100644 --- a/src/metrics/npm.rs +++ b/src/metrics/npm.rs @@ -20,11 +20,9 @@ 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}; -use crate::metrics::opens_member_scope; use crate::node::Node; use crate::*; @@ -33,15 +31,24 @@ use crate::*; /// This metric counts the number of public methods /// of classes/interfaces. /// -/// 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 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. +/// Emitted on container spaces — [`SpaceKind::Class`], `Struct`, +/// `Trait`, `Impl`, `Namespace`, `Interface` — and on the +/// [`SpaceKind::Unit`] file root that rolls them up. Never on a +/// [`SpaceKind::Function`] space, which owns no members of its own. +/// +/// Since [#1203] that holds by construction rather than by convention: +/// the space's own kind is the only input, so no language can disagree +/// with it in either direction. [`Wmc`](crate::wmc::Stats) decides the +/// same way. A language with no class-shaped construct at all — C, Bash, +/// Lua, Perl, Tcl — emits no block anywhere rather than an all-zero one +/// on each file root. +/// +/// The rule governs the *block*, not the counts behind it: those roll up +/// through every enclosing space regardless, so a type declared inside a +/// function body is reported by the nearest enclosing container, or by +/// the file root when there is none. +/// +/// [#1203]: https://github.com/dekobon/big-code-analysis/issues/1203 #[derive(Clone, Debug, Default, PartialEq)] #[non_exhaustive] pub struct Stats { @@ -53,7 +60,7 @@ pub struct Stats { interface_npm_sum: usize, class_nm_sum: usize, interface_nm_sum: usize, - is_class_space: bool, + space_kind: SpaceKind, } impl fmt::Display for Stats { @@ -220,31 +227,21 @@ impl Stats { self.interface_nm_sum += self.interface_nm; } - // Checks if the `Npm` metric is disabled + /// Records the kind of the space these stats describe, which is the + /// sole input to [`Self::is_disabled`]. + /// + /// Called once per space from the walker's finalize step, beside the + /// equivalent `wmc` call. Left unset — and so reported disabled — for + /// a language whose `HAS_MEMBERS` is `false`. #[inline] - pub(crate) fn is_disabled(&self) -> bool { - !self.is_class_space + pub(crate) fn set_space_kind(&mut self, kind: SpaceKind) { + self.space_kind = kind; } - /// 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 - /// 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. + // Checks if the `Npm` metric is disabled #[inline] - 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; - } + pub(crate) fn is_disabled(&self) -> bool { + !self.space_kind.is_member_scope() } } @@ -268,6 +265,17 @@ pub(crate) trait Npm where Self: Checker, { + /// Whether this language has any construct that owns members. + /// + /// `false` only for the no-op impls — grammars with no class-shaped + /// construct at all (C, Bash, Perl, Lua, Tcl, iRules, and the two + /// comment/preprocessor grammars), where the metric could report + /// nothing but zeros. The walker consults it before recording a + /// space kind, so those languages emit no block rather than an + /// all-zero one on every file root (#1203). `wmc` gets the same + /// outcome from its no-op `compute`, which never records a kind. + const HAS_MEMBERS: bool = true; + /// Walk `node` and update `stats` with this metric for the language /// implementing the trait. /// @@ -315,14 +323,12 @@ 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_member_scope::(node, code, ancestors); - match node.kind_id().into() { ClassBody | EnumBodyDeclarations => { for method in direct_child_funcs::(node) { @@ -388,14 +394,12 @@ 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_member_scope::(node, code, ancestors); - match node.kind_id().into() { ClassBody => { for member in node.children() { @@ -464,14 +468,12 @@ 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_member_scope::(node, code, ancestors); - if !matches!(node.kind_id().into(), ClassBody) { return; } diff --git a/src/metrics/npm/cpp.rs b/src/metrics/npm/cpp.rs index 66279c57d..2c06986de 100644 --- a/src/metrics/npm/cpp.rs +++ b/src/metrics/npm/cpp.rs @@ -17,13 +17,6 @@ impl Npm for CppCode { ) { use Cpp::*; - // Mark class / struct spaces as class spaces so the metric is - // emitted on them. - if matches!(node.kind_id().into(), ClassSpecifier | StructSpecifier) && stats.is_disabled() - { - stats.is_class_space = true; - } - if !matches!(node.kind_id().into(), FieldDeclarationList) { return; } diff --git a/src/metrics/npm/csharp.rs b/src/metrics/npm/csharp.rs index 63fd20e91..7d5c7bae2 100644 --- a/src/metrics/npm/csharp.rs +++ b/src/metrics/npm/csharp.rs @@ -53,14 +53,12 @@ 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_member_scope::(node, code, ancestors); - if !matches!(node.kind_id().into(), DeclarationList) { return; } diff --git a/src/metrics/npm/elixir.rs b/src/metrics/npm/elixir.rs index f0ca35ec4..1463aa0e2 100644 --- a/src/metrics/npm/elixir.rs +++ b/src/metrics/npm/elixir.rs @@ -26,7 +26,9 @@ impl Npm for ElixirCode { use crate::metrics::cognitive::{elixir_call_keyword, elixir_do_block_call_children}; // The space-opening node for a `defmodule` Call is the node - // itself, so this triggers exactly once per Class. + // itself, and the walker pushes that space before running any + // metric against it, so a nested `defmodule` is counted by its + // own module and never reaches this one's stats. // // `is_func_space_with_code` is not consulted: it is implied. // `elixir_is_class_macro` is exactly `kw == "defmodule"`, so it @@ -36,12 +38,10 @@ impl Npm for ElixirCode { // rejects the node anyway. Calling it cost a source-text keyword // scan per node plus, before #1088, an `O(depth)` climb, and its // answer was discarded either way (#1088). - if !stats.is_disabled() || !matches!(elixir_call_keyword(node, code), Some("defmodule")) { + if !matches!(elixir_call_keyword(node, code), Some("defmodule")) { return; } - stats.is_class_space = true; - // Direct-child method Calls of the module's do_block. We do // not descend deeper — methods nested inside another // `defmodule` are attributed to that inner module via its own diff --git a/src/metrics/npm/go.rs b/src/metrics/npm/go.rs index c7fa95cf9..df6507884 100644 --- a/src/metrics/npm/go.rs +++ b/src/metrics/npm/go.rs @@ -18,23 +18,11 @@ impl Npm for GoCode { use Go as G; match node.kind_id().into() { - // First-visit pass on the file root: enable npm output if - // the file declares any receiver methods. Walks only the - // direct children — Go always declares methods at file - // scope, so deeper recursion is unnecessary. - G::SourceFile - if stats.is_disabled() - && node - .children() - .any(|c| matches!(c.kind_id().into(), G::MethodDeclaration)) => - { - stats.is_class_space = true; - } // Each receiver method contributes one to the per-space // count; `compute_sum` rolls it into `class_nm_sum`, and - // the parent's merge bubbles it up to the Unit. The - // method's own space is left unmarked so its - // per-function npm block stays suppressed. + // the parent's merge bubbles it up to the Unit, which is + // where a Go file's npm is reported — Go has no container + // space to attach it to. G::MethodDeclaration => { stats.class_nm += 1; // Go's export rule is lexical (issue #458): the method @@ -83,12 +71,6 @@ impl Npm for GoCode { exported += 1; } } - if methods == 0 { - return; - } - if stats.is_disabled() { - stats.is_class_space = true; - } stats.interface_nm += methods; stats.interface_npm += exported; } diff --git a/src/metrics/npm/groovy.rs b/src/metrics/npm/groovy.rs index a04cfec34..049417bcd 100644 --- a/src/metrics/npm/groovy.rs +++ b/src/metrics/npm/groovy.rs @@ -18,15 +18,13 @@ 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_member_scope::(node, code, ancestors); - match node.kind_id().into() { ClassBody | EnumBody => { let is_interface_like = groovy_body_is_interface_like(node, ancestors); diff --git a/src/metrics/npm/kotlin.rs b/src/metrics/npm/kotlin.rs index ab2532a6b..e7c2d5225 100644 --- a/src/metrics/npm/kotlin.rs +++ b/src/metrics/npm/kotlin.rs @@ -17,14 +17,12 @@ 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_member_scope::(node, code, ancestors); - // Each `ClassBody` contributes its direct `FunctionDeclaration` // and `SecondaryConstructor` children to whichever func_space is // currently on the stack. Companion objects (not a func_space) diff --git a/src/metrics/npm/mozcpp.rs b/src/metrics/npm/mozcpp.rs index dddb7120a..4be8f600f 100644 --- a/src/metrics/npm/mozcpp.rs +++ b/src/metrics/npm/mozcpp.rs @@ -17,13 +17,6 @@ impl Npm for MozcppCode { ) { use Mozcpp::*; - // Mark class / struct spaces as class spaces so the metric is - // emitted on them. - if matches!(node.kind_id().into(), ClassSpecifier | StructSpecifier) && stats.is_disabled() - { - stats.is_class_space = true; - } - if !matches!(node.kind_id().into(), FieldDeclarationList) { return; } diff --git a/src/metrics/npm/objc.rs b/src/metrics/npm/objc.rs index 39b26ebf8..6aeb85438 100644 --- a/src/metrics/npm/objc.rs +++ b/src/metrics/npm/objc.rs @@ -15,9 +15,8 @@ use super::*; // interface declaration. There is no per-node visibility marker to read, // so every method counts as public — `npm == nm`. The members are direct // children of the class node (not a `field_declaration_list`), so we walk -// them when the class node itself is visited (where `stats` is already the -// class space, the same point `is_class_space` is marked — mirroring the -// C++ impl's marking step). `@property` accessors are auto-generated and +// them when the class node itself is visited, where `stats` is already the +// class space. `@property` accessors are auto-generated and // carry no `method_declaration` node, so they are not counted here. impl Npm for ObjcCode { fn compute<'a>( @@ -32,9 +31,6 @@ impl Npm for ObjcCode { if !is_interface && node.kind_id() != ClassImplementation as u16 { return; } - if stats.is_disabled() { - stats.is_class_space = true; - } let mut methods = 0; for child in node.children() { match child.kind_id().into() { diff --git a/src/metrics/npm/php.rs b/src/metrics/npm/php.rs index bc4be253e..06bad216c 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,8 +24,6 @@ impl Npm for PhpCode { // sibling `Abc::compute` carries a marker here. use Php::*; - stats.enable_for_member_scope::(node, code, ancestors); - match node.kind_id().into() { DeclarationList => { let Some(parent_kind) = ancestors.parent(node).map(|p| p.kind_id().into()) else { diff --git a/src/metrics/npm/python.rs b/src/metrics/npm/python.rs index 1425e54a8..6f76036d7 100644 --- a/src/metrics/npm/python.rs +++ b/src/metrics/npm/python.rs @@ -37,16 +37,12 @@ impl Npm for PythonCode { ) { use Python::*; - // Gate on `ClassDefinition` specifically so the flag is not - // set on plain function or module spaces. + // Only a `ClassDefinition` owns methods, so nothing else can + // contribute to the count. if !matches!(node.kind_id().into(), ClassDefinition) { return; } - if stats.is_disabled() { - stats.is_class_space = true; - } - // The class body is the `block` child; route through // `python_is_block` so both aliased kind_ids (`Block` / // `Block2`) are accepted at one normalization point (#419). diff --git a/src/metrics/npm/ruby.rs b/src/metrics/npm/ruby.rs index 0e3e3f7aa..47a344019 100644 --- a/src/metrics/npm/ruby.rs +++ b/src/metrics/npm/ruby.rs @@ -28,8 +28,6 @@ impl Npm for RubyCode { ) { use Ruby::*; - stats.enable_for_member_scope::(node, code, ancestors); - if !matches!(node.kind_id().into(), BodyStatement | BodyStatement2) { return; } diff --git a/src/metrics/npm/rust.rs b/src/metrics/npm/rust.rs index 8b5e362a5..33040380b 100644 --- a/src/metrics/npm/rust.rs +++ b/src/metrics/npm/rust.rs @@ -33,11 +33,6 @@ impl Npm for RustCode { ) { use Rust::*; - // Mark Impl / Trait spaces as class spaces so npm emits. - if matches!(node.kind_id().into(), ImplItem | TraitItem) && stats.is_disabled() { - stats.is_class_space = true; - } - // A method is a `function_item` or `function_signature_item` // whose parent is the `declaration_list` of an `impl` or // `trait`. Gating on the kind, parent, and grandparent keeps diff --git a/src/spaces/compute.rs b/src/spaces/compute.rs index 0f0058130..61a8f0055 100644 --- a/src/spaces/compute.rs +++ b/src/spaces/compute.rs @@ -60,6 +60,33 @@ fn compute_wmc(state: &mut State, selected: MetricSet) { } } +/// Records the space kind that decides whether `npm` / `npa` are +/// serialized on this space. +/// +/// Both are emitted only on a member scope — a container, or the file +/// unit that rolls its containers up — and since #1203 the space's own +/// kind is the whole of that decision. Doing it here rather than letting +/// each language raise a flag from its own grammar node kinds is what +/// makes the rule hold for every language, including one added later: +/// there is no per-language surface left to deviate on, in either +/// direction. +/// +/// `HAS_MEMBERS` is the one exception, and it is a language-level opt +/// out rather than a per-space one: a grammar with no class-shaped +/// construct anywhere (C, Bash, Lua, …) would otherwise report an +/// all-zero block on every file root, since a unit is a member scope +/// like any other. +#[inline] +fn note_member_scope(state: &mut State, selected: MetricSet) { + let kind = state.space.kind; + if selected.contains(Metric::Npm) && ::HAS_MEMBERS { + state.space.metrics.npm.set_space_kind(kind); + } + if selected.contains(Metric::Npa) && ::HAS_MEMBERS { + state.space.metrics.npa.set_space_kind(kind); + } +} + #[inline] fn compute_averages(state: &mut State, selected: MetricSet) { // The per-function averages for Cognitive, Exit, and NArgs divide @@ -152,6 +179,7 @@ fn finalize_state(state: &mut State, selected: MetricSet) { compute_sum(state, selected); compute_halstead_and_mi::(state, selected); compute_wmc::(state, selected); + note_member_scope::(state, selected); compute_averages(state, selected); } diff --git a/tests/repositories/big-code-analysis-output b/tests/repositories/big-code-analysis-output index f75791311..a23ea6a8c 160000 --- a/tests/repositories/big-code-analysis-output +++ b/tests/repositories/big-code-analysis-output @@ -1 +1 @@ -Subproject commit f757913110dc8c9c9784deb26accc8a52c029d51 +Subproject commit a23ea6a8c04f128d5617a3d75489143f95f10412