diff --git a/.bca-baseline.toml b/.bca-baseline.toml index 5d5ff545c..4b762bb86 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 = 52227.844233305164 +value = 51274.96613103099 [[entry]] path = "src/metrics/npa/python.rs" diff --git a/CHANGELOG.md b/CHANGELOG.md index a4f3034ae..03192f5b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -766,6 +766,41 @@ for historical reference. ### Fixed +- **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..61867b5cb 100644 --- a/STABILITY.md +++ b/STABILITY.md @@ -1074,8 +1074,13 @@ 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, 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 (#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 df61b70f4..dfe738a54 100644 --- a/big-code-analysis-book/src/metrics.md +++ b/big-code-analysis-book/src/metrics.md @@ -917,6 +917,31 @@ 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". +### 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 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 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 +[Threshold scope](commands/check.md#threshold-scope)). + ### How to read it NPA is a *direct* measure of encapsulation. Every public attribute @@ -946,7 +971,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`. +`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 57e0bad55..812c9f723 100644 --- a/big-code-analysis-py/src/types_codegen.rs +++ b/big-code-analysis-py/src/types_codegen.rs @@ -1056,19 +1056,30 @@ 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. 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 / npm / npa on a class. + // 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", ); 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..13b10d10b 100644 --- a/big-code-analysis-py/tests/test_metrics_select.py +++ b/big-code-analysis-py/tests/test_metrics_select.py @@ -81,6 +81,11 @@ def test_metric_names_round_trip_through_analyze() -> None: 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"} for name in bca.METRIC_NAMES: @@ -514,3 +519,62 @@ 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. + + 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_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" + + 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), ( + f"function space {space['name']!r} must not carry npa/npm; got {sorted(keys)}" + ) + + +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] + 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 new file mode 100644 index 000000000..ddf6c6c53 --- /dev/null +++ b/src/metrics/container_scope_tests.rs @@ -0,0 +1,447 @@ +//! 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 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::{assert_fixtures_present, space_verbatim}; +use crate::{LANG, MetricsOptions}; + +/// A serialized space, flattened to what these tests assert on. +struct Emitted { + kind: SpaceKind, + /// `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, +} + +/// 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"); + // `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(kind), + name: value["name"].as_str().map(str::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_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 +/// 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, +} + +/// 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 +} + +/// 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] = &[ + #[cfg(feature = "kotlin")] + 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 } +} +", + }, + #[cfg(feature = "java")] + 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; } +} +", + }, + #[cfg(feature = "groovy")] + 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 } +} +", + }, + #[cfg(feature = "javascript")] + Fixture { + lang: LANG::Javascript, + containers: &["C"], + source: "\ +class C { + a = 1; + static { this.b = 2; } + q() { return this.a; } +} +function top(x) { return x; } +", + }, + #[cfg(feature = "mozjs")] + Fixture { + lang: LANG::Mozjs, + containers: &["C"], + source: "\ +class C { + a = 1; + static { this.b = 2; } + q() { return this.a; } +} +function top(x) { return x; } +", + }, + #[cfg(feature = "typescript")] + 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; } +", + }, + #[cfg(feature = "typescript")] + 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; } +", + }, + #[cfg(feature = "csharp")] + Fixture { + lang: LANG::Csharp, + containers: &["C", "I"], + source: "\ +interface I { + int Q(); +} +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"], + source: "\ +a; } +} +function top($x) { return $x; } +", + }, + #[cfg(feature = "ruby")] + 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() { + assert_fixtures_present(FIXTURES); + for fixture in FIXTURES { + 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 + ), + "{:?}: {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 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, as are C#'s +/// expression-bodied property and indexer. +#[test] +fn function_spaces_emit_neither() { + assert_fixtures_present(FIXTURES); + for fixture in FIXTURES { + let spaces = emitted_spaces(fixture.lang, fixture.source); + let functions: Vec<&Emitted> = spaces + .iter() + .filter(|s| s.kind == SpaceKind::Function) + .collect(); + // A fixture whose functions all failed to open a space would make + // every assertion below vacuous. + assert!( + !functions.is_empty(), + "{:?}: expected at least one function space, got {:?}", + fixture.lang, + summary(&spaces) + ); + for space in functions { + 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 + ); + } + } +} + +/// 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. +/// +/// [`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] +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 { + let space = only_space(*lang, &spaces, name); + assert_eq!(space.kind, SpaceKind::Function, "{lang:?}: {name:?}"); + assert!( + !space.has_npm && !space.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] +#[cfg(feature = "java")] +fn container_counts_survive_the_narrowed_enable() { + let space = space_verbatim( + LANG::Java, + fixture_source(LANG::Java).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 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. + 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..1a6b8af0f 100644 --- a/src/metrics/mod.rs +++ b/src/metrics/mod.rs @@ -4,6 +4,10 @@ //! 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. @@ -31,6 +35,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 +73,41 @@ pub mod wmc; 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 74783d486..4a9c5b66c 100644 --- a/src/metrics/npa.rs +++ b/src/metrics/npa.rs @@ -20,8 +20,10 @@ 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::*; @@ -29,6 +31,16 @@ 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. #[derive(Clone, Debug, Default, PartialEq)] #[non_exhaustive] pub struct Stats { @@ -212,6 +224,27 @@ impl Stats { pub(crate) fn is_disabled(&self) -> bool { !self.is_class_space } + + /// 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. + #[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; + } + } } // Computes an accessibility ratio (public members / total members), @@ -250,15 +283,18 @@ where fn compute<'a>( node: &Node<'a>, code: &'a [u8], - _ancestors: Ancestors<'a, '_>, + ancestors: Ancestors<'a, '_>, stats: &mut Stats, ); } -// 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 @@ -281,15 +317,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::*; - if Self::is_func_space(node) && stats.is_disabled() { - stats.is_class_space = true; - } + stats.enable_for_member_scope::(node, code, ancestors); match node.kind_id().into() { ClassBody | EnumBodyDeclarations => { @@ -370,15 +404,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::*; - if Self::is_func_space(node) && stats.is_disabled() { - stats.is_class_space = true; - } + stats.enable_for_member_scope::(node, code, ancestors); match node.kind_id().into() { ClassBody => { @@ -492,15 +524,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::*; - if Self::is_func_space(node) && stats.is_disabled() { - stats.is_class_space = true; - } + 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 899d2e826..9452c0d42 100644 --- a/src/metrics/npa/csharp.rs +++ b/src/metrics/npa/csharp.rs @@ -11,15 +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::*; - if Self::is_func_space(node) && stats.is_disabled() { - stats.is_class_space = true; - } + 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 0237ae5b7..5bfbb4488 100644 --- a/src/metrics/npa/groovy.rs +++ b/src/metrics/npa/groovy.rs @@ -21,15 +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::*; - if Self::is_func_space(node) && stats.is_disabled() { - stats.is_class_space = true; - } + 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 c902ddb91..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,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 Self::is_func_space(node) && stats.is_disabled() { - stats.is_class_space = true; - } + 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 8fea1c97c..0052d12fb 100644 --- a/src/metrics/npa/php.rs +++ b/src/metrics/npa/php.rs @@ -11,16 +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::*; - // Enables the `Npa` metric if computing stats of a class-like space. - if Self::is_func_space(node) && stats.is_disabled() { - stats.is_class_space = true; - } + 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 04d44b117..957fda7e6 100644 --- a/src/metrics/npa/ruby.rs +++ b/src/metrics/npa/ruby.rs @@ -17,9 +17,7 @@ impl Npa for RubyCode { ) { use Ruby::*; - if Self::is_func_space(node) && stats.is_disabled() { - stats.is_class_space = true; - } + 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 2db3a9a52..b29276273 100644 --- a/src/metrics/npm.rs +++ b/src/metrics/npm.rs @@ -20,9 +20,11 @@ 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::*; @@ -30,6 +32,16 @@ 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. #[derive(Clone, Debug, Default, PartialEq)] #[non_exhaustive] pub struct Stats { @@ -213,6 +225,27 @@ impl Stats { pub(crate) fn is_disabled(&self) -> bool { !self.is_class_space } + + /// 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. + #[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; + } + } } /// The direct children of `node` that `C` classifies as functions. @@ -255,15 +288,18 @@ where fn compute<'a>( node: &Node<'a>, code: &'a [u8], - _ancestors: Ancestors<'a, '_>, + ancestors: Ancestors<'a, '_>, stats: &mut Stats, ); } -// 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 @@ -279,15 +315,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::*; - if Self::is_func_space(node) && stats.is_disabled() { - stats.is_class_space = true; - } + stats.enable_for_member_scope::(node, code, ancestors); match node.kind_id().into() { ClassBody | EnumBodyDeclarations => { @@ -354,15 +388,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::*; - if Self::is_func_space(node) && stats.is_disabled() { - stats.is_class_space = true; - } + stats.enable_for_member_scope::(node, code, ancestors); match node.kind_id().into() { ClassBody => { @@ -432,15 +464,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::*; - if Self::is_func_space(node) && stats.is_disabled() { - stats.is_class_space = true; - } + 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 88d66e555..63fd20e91 100644 --- a/src/metrics/npm/csharp.rs +++ b/src/metrics/npm/csharp.rs @@ -53,15 +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::*; - if Self::is_func_space(node) && stats.is_disabled() { - stats.is_class_space = true; - } + 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 cf973a05f..a04cfec34 100644 --- a/src/metrics/npm/groovy.rs +++ b/src/metrics/npm/groovy.rs @@ -18,16 +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::*; - if Self::is_func_space(node) && stats.is_disabled() { - stats.is_class_space = true; - } + 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 ce949164d..ab2532a6b 100644 --- a/src/metrics/npm/kotlin.rs +++ b/src/metrics/npm/kotlin.rs @@ -17,16 +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::*; - // Enables the `Npm` metric for any class-like func_space. - if Self::is_func_space(node) && stats.is_disabled() { - stats.is_class_space = true; - } + 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 57c7da2d2..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,9 +24,7 @@ impl Npm for PhpCode { // sibling `Abc::compute` carries a marker here. use Php::*; - if Self::is_func_space(node) && stats.is_disabled() { - stats.is_class_space = true; - } + 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 8c5f7e13b..0e3e3f7aa 100644 --- a/src/metrics/npm/ruby.rs +++ b/src/metrics/npm/ruby.rs @@ -28,9 +28,7 @@ impl Npm for RubyCode { ) { use Ruby::*; - if Self::is_func_space(node) && stats.is_disabled() { - stats.is_class_space = true; - } + 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 7a9c5ec8f..f75791311 160000 --- a/tests/repositories/big-code-analysis-output +++ b/tests/repositories/big-code-analysis-output @@ -1 +1 @@ -Subproject commit 7a9c5ec8fb06d185d09588e8ca7af9468104410a +Subproject commit f757913110dc8c9c9784deb26accc8a52c029d51