diff --git a/benchmarks/repsel_census/baseline.json b/benchmarks/repsel_census/baseline.json index 69275442a3..bc9d321501 100644 --- a/benchmarks/repsel_census/baseline.json +++ b/benchmarks/repsel_census/baseline.json @@ -28,7 +28,9 @@ "ptr_shape_get_number": 2, "ptr_shape_set": 1, "ptr_shape_method": 1 - } + }, + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "fixture_ptr_shape_sites", @@ -59,7 +61,9 @@ "ptr_shape_set": 1, "ptr_shape_update": 1, "ptr_shape_method": 1 - } + }, + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "fixture_ptr_shape_elements", @@ -89,6 +93,47 @@ "ptr_shape_method": 1, "class_field_get.shape_proven_load": 1, "ptr_shape_set": 2 + }, + "alloc_contexts": {}, + "alloc_buckets": {} + }, + { + "name": "fixture_alloc_buckets", + "role": "liveness", + "source": "benchmarks/repsel_census/fixtures/fixture_alloc_buckets.ts", + "floors": { + "ptr-shape": 3, + "ptr-shape-consumed": 0, + "ptr-numarray": 0, + "canonical-i32": 0, + "canonical-u32": 0, + "canonical-str": 0, + "int-valued-ta": 0, + "spec-abi-entry": 0, + "spec-abi-taptr-slot": 0 + }, + "candidates": { + "ptr-shape": 10, + "ptr-numarray": 0, + "canonical-slot": 0, + "int-valued-ta": 0, + "spec-abi": 0 + }, + "unconsumed_mechanisms": { + "scalar_replaced": 3 + }, + "consumption_sites": {}, + "alloc_contexts": { + "return": 1, + "constructor argument": 1, + "object literal property value": 1 + }, + "alloc_buckets": { + "ptr-shape | return | rule 1 (provenance) \u2014 already served by return-shape": 1, + "ptr-shape | constructor argument | rule 1 (provenance)": 1, + "ptr-shape | initializer | rule 1 (provenance)": 2, + "ptr-shape | object literal property value | rule 1 (provenance)": 1, + "ptr-shape | returned expression operand | rule 1 (provenance)": 2 } }, { @@ -114,7 +159,9 @@ "spec-abi": 2 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "fixture_canonical_slots", @@ -139,7 +186,9 @@ "spec-abi": 3 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "fixture_int_valued_ta", @@ -164,7 +213,9 @@ "spec-abi": 0 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "fixture_spec_abi_taptr", @@ -189,7 +240,9 @@ "spec-abi": 1 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "fixture_loop_bounded_i32", @@ -214,7 +267,9 @@ "spec-abi": 5 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "fixture_module_init_canonical", @@ -239,7 +294,9 @@ "spec-abi": 0 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "batch", @@ -270,6 +327,12 @@ "ptr_shape_get_number": 1, "ptr_shape_set": 2, "class_field_get.shape_proven_load": 2 + }, + "alloc_contexts": { + "return": 1 + }, + "alloc_buckets": { + "ptr-shape | return | rule 1 (provenance)": 1 } }, { @@ -295,7 +358,9 @@ "spec-abi": 0 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_02_loop_overhead", @@ -320,7 +385,9 @@ "spec-abi": 0 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_03_array_write", @@ -345,7 +412,9 @@ "spec-abi": 0 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_04_array_read", @@ -370,7 +439,9 @@ "spec-abi": 0 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_05_fibonacci", @@ -395,7 +466,9 @@ "spec-abi": 1 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_06_math_intensive", @@ -420,7 +493,9 @@ "spec-abi": 0 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_07_object_create", @@ -447,7 +522,9 @@ "unconsumed_mechanisms": { "scalar_replaced": 1 }, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_08_string_concat", @@ -472,7 +549,9 @@ "spec-abi": 0 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_09_method_calls", @@ -503,7 +582,9 @@ "class_field_get.shape_proven_load": 1, "ptr_shape_get_number": 1, "ptr_shape_set": 1 - } + }, + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_10_nested_loops", @@ -528,7 +609,9 @@ "spec-abi": 0 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_11_prime_sieve", @@ -553,7 +636,9 @@ "spec-abi": 0 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_12_binary_trees", @@ -580,7 +665,9 @@ "unconsumed_mechanisms": { "scalar_replaced": 1 }, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_13_factorial", @@ -605,7 +692,9 @@ "spec-abi": 0 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_14_closure", @@ -630,7 +719,9 @@ "spec-abi": 0 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_15_mandelbrot", @@ -655,7 +746,9 @@ "spec-abi": 0 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_16_matrix_multiply", @@ -680,7 +773,9 @@ "spec-abi": 1 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {}, + "alloc_buckets": {} }, { "name": "suite_17_loop_data_dependent", @@ -705,8 +800,10 @@ "spec-abi": 0 }, "unconsumed_mechanisms": {}, - "consumption_sites": {} + "consumption_sites": {}, + "alloc_contexts": {}, + "alloc_buckets": {} } ], - "generated_at": "2026-07-31T21:18:40.428173Z" + "generated_at": "2026-08-01T09:57:46.100718Z" } diff --git a/benchmarks/repsel_census/fixtures/fixture_alloc_buckets.ts b/benchmarks/repsel_census/fixtures/fixture_alloc_buckets.ts new file mode 100644 index 0000000000..317031e565 --- /dev/null +++ b/benchmarks/repsel_census/fixtures/fixture_alloc_buckets.ts @@ -0,0 +1,97 @@ +// Liveness fixture for the `Ptr` ALLOC-SITE BUCKETS (#7170 R0). +// +// The other `ptr_shape` fixtures assert that a promotion can be observed. This +// one asserts something the census could not see at all before R0: that the +// three *denial* buckets the report files unbound allocations into are still +// being told apart. +// +// They were not. #7170 measured two merges, each of which made a +// scheduler-facing number mean something other than what it looked like: +// +// * `constructor argument` conflated a genuine `new C(arg)` (2.0% of sites +// on the dependency corpus) with the property values of anonymous-shape +// object literals (24.5%) — because `{a: {b: 1}}` lowers to +// `new __AnonShape_N()`, so the inner literal really is an +// argument of a `new`, just not of a constructor anybody wrote. +// * the `return` bucket counted syntactic sites rather than opportunities: +// `collectors/ptr_shape_returns.rs` (#7107) already admits a bare +// `return new C(...)` as a return-shape producer, but `deny_alloc_site` +// fires before any seeding and could not know. +// +// Each of the three shapes below lands in exactly one bucket, and +// `ALLOC_BUCKET_FLOORS` / `ALLOC_RULE_FLOORS` require all three to be +// reported. A classification arm that stops firing re-merges its bucket into a +// neighbour, the count for that bucket goes to zero, and the census goes red — +// instead of the merge being invisible, which is the state R0 found. +// +// This is also the only layer that exercises the WIRING. The unit tests set the +// report scope by hand, so `codegen/function.rs` could pass `false` for every +// function's return-shape fact and every one of them would still pass. Here the +// fact has to travel from `collect_return_shape_functions` through +// `ModuleDispatchFacts` into the region scope for `makePoint`'s row to carry +// the served rule. +// +// Do not "tidy" this file: +// * giving `makePoint` a second return of a different class, or an early +// `return;`, revokes its return-shape fact and takes the served bucket to +// zero; +// * binding the inner literal of `nested` to its own `const` moves it out of +// the anonymous-shape-component bucket entirely (it becomes rule 1's own +// seed); +// * adding `Object.defineProperty` / `delete` / `setPrototypeOf` / `Proxy` +// anywhere arms the rule-5 module barrier, which kills every return-shape +// fact in the module and silently empties the served bucket. + +class Point { + x: number; + y: number; + constructor(x: number, y: number) { + this.x = x; + this.y = y; + } +} + +class Boxed { + v: Point; + constructor(v: Point) { + this.v = v; + } +} + +// Bucket 1 — `return`, in a function that DOES carry a return-shape fact: +// every return is a bare fresh `new` of one declared, admissible class, the +// body cannot fall off its end, and the declared return type is a pointer. +// So this row must be filed under the served rule, not the rule-1 wall. +export function makePoint(i: number): Point { + return new Point(i, i + 1); +} + +// Bucket 2 — a genuine `new C(arg)` constructor argument. `Boxed` is a class +// the developer declared, so `new Point(1, 2)` here is what the label says. +const boxed = new Boxed(new Point(1, 2)); + +// Bucket 3 — an anonymous-shape component. `{ inner: ..., k: 3 }` is a closed +// shape, so it lowers to `new __AnonShape_N(, 3)` and the inner +// literal arrives as an argument of that synthetic constructor. It is a field +// value of a parent allocation that is itself unbound: proving its shape +// licenses nothing on its own, which is exactly why it must not share a bucket +// with the line above. +const nested = { inner: { a: 1, b: 2 }, k: 3 }; + +// Bucket 4 — a returned expression OPERAND. What this function returns is the +// conditional, not either allocation, so #7107's return-shape fact covers +// neither and `pickPoint` gets no fact at all (`producer_return_class` admits +// only a bare `Expr::New` or a proven local as a return). Before the #7176 +// review both arms inherited the `return` label from `Stmt::Return` and were +// counted as return positions — which is what over-stated the `return` bucket +// published on #7170 as R1's ceiling. +export function pickPoint(flag: boolean): Point { + return flag ? new Point(5, 6) : new Point(7, 8); +} + +const p = makePoint(3); +const q = pickPoint(true); +console.log( + "alloc_buckets:" + + (boxed.v.x + nested.inner.a + nested.k + p.x + p.y + q.x + q.y), +); diff --git a/changelog.d/7176-repsel-alloc-site-buckets.md b/changelog.d/7176-repsel-alloc-site-buckets.md new file mode 100644 index 0000000000..c1801d961a --- /dev/null +++ b/changelog.d/7176-repsel-alloc-site-buckets.md @@ -0,0 +1,67 @@ +**`--opt-report`: the `Ptr` allocation-site buckets now mean what they +are read as (#7170 R0).** Report-only — emitted LLVM IR is identical across the +**29 dependency modules that emit IR** out of 30 sampled (the 30th fails to +compile identically in both arms), each with a same-compiler control run, both +arms run *under* `--opt-report` — the only mode in which any of this code +executes — and both built from one pinned commit. Four defects, each of which +made a scheduler-facing number say something other than what it looked like: + +- **De-duplication was masking rows, by a factor of 2.2.** Every anonymous + object literal renders as `object literal { ... }` and carries + `byte_offset: 0`, so every unbound literal in a function collapsed onto one + `Entry::dedup_key`. Over 196 real `__esModule` dependency modules the + `ptr-shape` allocation-site denial count goes **837 → 1842**. A new + `alloc_ordinal` (the site's index in the region's deterministic walk) + discriminates them; a function lowered twice walks the same body, produces + the same ordinals, and still collapses. What the drain *did* collapse is now + reported as `summary.masked_by_dedup` instead of left implicit. +- **`constructor argument` conflated two mechanisms.** A closed-shape literal + lowers to `new __AnonShape_N(v0, …)` whose constructor arguments *are* its + property values, so `{a: {b: 1}}` filed its inner literal as a constructor + argument. Split into `constructor argument` (5 on the corpus) and + `object literal property value` (418) — the old label was 98.8 % the latter, + and the two need different fixes: a nested literal is a field value of a + parent allocation that is itself unbound, so proving its shape licenses + nothing on its own. +- **The `return` bucket counted syntactic sites, not opportunities.** + `collectors/ptr_shape_returns.rs` (#7107) already admits a bare + `return new C(...)` as a producer, but `deny_alloc_site` fires before any + seeding. Those sites now carry their own rule string and a new `Tier::Served` + so they leave the rule-1 bucket rather than inflating it. +- **`return` was a *label propagated down a subtree*, not a position.** It was + set once at `Stmt::Return` and inherited by everything nested inside the + returned expression, so `return cond ? {a} : {b}`, `return flag && {a}`, + `return await {a}` and `return {a}.x` all counted as return positions. + `scan_return` is now the only producer of a return position, servedness reads + `NewSite::is_return_position` rather than comparing a label (so renaming a + bucket cannot silently disable it), and nested allocations get their own + bucket, `returned expression operand`. + +The buckets are computed by the compiler (`summary.by_rule`, +`summary.by_alloc_context`) instead of being reconstructed downstream, which is +what made the second and third defects invisible: a `jq` reduction over +`entries[]` cannot see that one label means two mechanisms. + +**The re-derived dependency-JS rule-1 wall is 1838 unserved allocation sites of +2028 `Ptr` candidates (90.6 %)**, replacing #7152's `506 / 746` and +#7170's `963 / 1352`. The correction #7170 §5.2 expected to be material is not +— served returns are **4** sites — because only 62 of 1842 unbound allocations +are in a `function` region at all; 1711 (92.9 %) are in closures, which cannot +carry a return-shape fact. The instrument's real distortion was the dedup, and +it ran the other way: the wall is larger than reported, not smaller. + +Census: `alloc_buckets` per workload keyed by the full `(analysis, allocation +position, denial rule)` tuple — the same identity the renderer uses, so the two +cannot describe different things — recorded as context and floored in code via +`ALLOC_BUCKET_FLOORS`, with `fixture_alloc_buckets.ts` written to land one +allocation in each bucket. The served row is the only layer that can catch the +wiring dying: it lives in `codegen/function.rs`, and every compiler unit test +sets the report scope by hand. No existing census floor moved and no existing +workload's candidate count moved: the distortion is invisible on hand-written +benchmarks and doubles the row count on real dependency JS, which is why it +survived. + +Still open, measured but deliberately not fixed here: `statement` is the +largest bucket (455 of 1842, 24.7 %) and is a **catch-all** — the report walk's +default context, holding property stores, reassignments, conditional branches +and default parameters indistinguishably. diff --git a/crates/perry-codegen/src/codegen/function.rs b/crates/perry-codegen/src/codegen/function.rs index a48c35b494..b755bf5b10 100644 --- a/crates/perry-codegen/src/codegen/function.rs +++ b/crates/perry-codegen/src/codegen/function.rs @@ -594,8 +594,18 @@ pub(super) fn compile_function( .unwrap_or_default(); // `--opt-report` (#6952): attribute every representation decision the // collectors below make to this function. No-op when the report is off. - let _opt_report_scope = - crate::opt_report::enter_region(&f.name, crate::opt_report::RegionKind::Function); + // + // #7170 R0: this is the only region opener holding both the `FuncId` and + // `ModuleDispatchFacts`, so it is the only one that can say whether this + // body's `return new C(...)` sites are already served by #7107's + // return-shape mechanism. Read by nothing but the report. + let _opt_report_scope = crate::opt_report::enter_function_region( + &f.name, + cross_module + .module_dispatch + .return_shape_class(f.id) + .is_some(), + ); let native_facts = crate::collectors::collect_native_region_fact_graph_with_spec_lens( &f.body, &f.params, diff --git a/crates/perry-codegen/src/collectors/cjs_scaffolding.rs b/crates/perry-codegen/src/collectors/cjs_scaffolding.rs index 9fe1855c37..cb0f89538d 100644 --- a/crates/perry-codegen/src/collectors/cjs_scaffolding.rs +++ b/crates/perry-codegen/src/collectors/cjs_scaffolding.rs @@ -1325,8 +1325,15 @@ mod tests { let base = super::super::ptr_shape_report::unbound_new_sites(®ion, &CjsPreamble::default()); let contexts: Vec<&str> = base.iter().map(|s| s.context).collect(); + // #7170 §5.1 / R0 renamed this bucket, and the rename is measured on + // exactly this statement: `const __cjs_module = { exports: {} }` is an + // anonymous-shape allocation whose constructor arguments ARE its + // property values, so the inner `{}` is a *component of a literal*, not + // an argument of a `new C(...)`. It was 188 of the 194 rows the old + // `constructor argument` label carried (PR #7171 §1) — which is why + // that label could not be read as "constructor arguments". assert!( - contexts.contains(&"constructor argument"), + contexts.contains(&"object literal property value"), "the `{{ exports: {{}} }}` inner literal is no longer reported \ unsuppressed; this test's premise is gone: {contexts:?}" ); diff --git a/crates/perry-codegen/src/collectors/ptr_shape_opt_report_tests.rs b/crates/perry-codegen/src/collectors/ptr_shape_opt_report_tests.rs index fcfb1d6350..2e399ac711 100644 --- a/crates/perry-codegen/src/collectors/ptr_shape_opt_report_tests.rs +++ b/crates/perry-codegen/src/collectors/ptr_shape_opt_report_tests.rs @@ -332,3 +332,444 @@ fn nothing_is_recorded_when_the_report_is_off() { "the sink must stay empty when the report is off" ); } + +// ── #7170 R0: what the alloc-site buckets MEAN ───────────────────────────── +// +// Three defects, measured on 197 dependency modules, each of which made a +// scheduler-facing number say something other than what it looked like. The +// tests below are grouped by defect and each one is written so that reverting +// its arm in the compiler takes it — and only it — red. + +fn anon_shape(args: Vec) -> Expr { + Expr::New { + class_name: "__AnonShape_1f2e".to_string(), + args, + type_args: Vec::new(), + byte_offset: 0, + cap_args_appended: 0, + } +} + +fn alloc_rows(entries: &[crate::opt_report::Entry]) -> Vec<&crate::opt_report::Entry> { + entries + .iter() + .filter(|e| e.position == Position::AllocSite) + .collect() +} + +/// Defect 2 (#7170 §5.1). A closed-shape object literal lowers to +/// `new __AnonShape_N(v0, …)` whose constructor arguments ARE its property +/// values, so `{a: {b: 1}}` filed its inner literal under +/// `constructor argument` — the label for a genuine `new C(arg)`. On the +/// dependency corpus the two are 24.5% and 2.0% of sites: one bucket was 92% +/// the other thing. +#[test] +fn a_nested_object_literal_is_not_filed_as_a_constructor_argument() { + let classes = HashMap::new(); + let stmts = vec![Stmt::Let { + id: 1, + name: "outer".to_string(), + ty: Type::Any, + mutable: false, + init: Some(anon_shape(vec![anon_shape(Vec::new())])), + }]; + + let session = Session::start(); + let _ = run(&stmts, &classes); + let entries = session.entries(); + + let rows = alloc_rows(&entries); + assert_eq!(rows.len(), 1, "the inner literal is the only unbound site"); + assert_eq!( + rows[0].alloc_context.as_deref(), + Some("object literal property value"), + "a property value of an anonymous shape is not a constructor argument" + ); +} + +/// The other direction: a genuine `new C(arg)` argument must KEEP the +/// `constructor argument` bucket. Without this the split is satisfiable by a +/// rule that simply renames everything. +#[test] +fn a_real_constructor_argument_keeps_its_own_bucket() { + let c = class_with_fields("C", &["x"]); + let boxc = class_with_fields("Box", &["v"]); + let mut classes = HashMap::new(); + classes.insert("C".to_string(), &c); + classes.insert("Box".to_string(), &boxc); + let stmts = vec![Stmt::Let { + id: 1, + name: "b".to_string(), + ty: Type::Any, + mutable: false, + init: Some(Expr::New { + class_name: "Box".to_string(), + args: vec![new_c()], + type_args: Vec::new(), + byte_offset: 0, + cap_args_appended: 0, + }), + }]; + + let session = Session::start(); + let _ = run(&stmts, &classes); + let entries = session.entries(); + + let rows = alloc_rows(&entries); + assert_eq!(rows.len(), 1); + assert_eq!( + rows[0].alloc_context.as_deref(), + Some("constructor argument"), + "`new Box(new C())` really is a constructor argument" + ); +} + +/// Defect 3 (#7170 §5.2). `deny_alloc_site` fires unconditionally at the top +/// of the collector, before any seeding, so a `return new C(...)` in a +/// function that ALREADY carries a return-shape fact (#7107) was counted as a +/// rule-1 miss. It is not one: the class reaches every +/// `const r = producer(...)` call site. +#[test] +fn a_return_in_a_return_shape_producer_is_reported_as_served() { + let c = class_with_fields("C", &["x"]); + let mut classes = HashMap::new(); + classes.insert("C".to_string(), &c); + let stmts = vec![Stmt::Return(Some(new_c()))]; + + let session = Session::start(); + let _guard = crate::opt_report::enter_function_region("mk", true); + let _ = run(&stmts, &classes); + drop(_guard); + let entries = session.entries(); + + let rows = alloc_rows(&entries); + assert_eq!(rows.len(), 1); + assert_eq!( + rows[0].rule.as_deref(), + Some("rule 1 (provenance) — already served by return-shape"), + "a served return must leave the rule-1 bucket, not merely annotate it" + ); + assert_eq!(rows[0].tier, Some(crate::opt_report::Tier::Served)); +} + +/// The same site in a function WITHOUT a return-shape fact stays an ordinary +/// rule-1 denial. This is the anti-vacuity half: a classifier that answers +/// "served" unconditionally passes the test above and fails this one. +#[test] +fn a_return_in_a_plain_function_is_still_a_rule_1_denial() { + let c = class_with_fields("C", &["x"]); + let mut classes = HashMap::new(); + classes.insert("C".to_string(), &c); + let stmts = vec![Stmt::Return(Some(new_c()))]; + + let session = Session::start(); + let _guard = crate::opt_report::enter_function_region("mk", false); + let _ = run(&stmts, &classes); + drop(_guard); + let entries = session.entries(); + + let rows = alloc_rows(&entries); + assert_eq!(rows.len(), 1); + assert_eq!(rows[0].rule.as_deref(), Some("rule 1 (provenance)")); + assert_eq!( + rows[0].tier, + Some(crate::opt_report::Tier::CompilerLimitation) + ); +} + +/// #7170 §6, as a test rather than a comment: **91.6% of dependency-JS +/// allocation sites are in closure regions, and none of them is served.** +/// `collect_return_shape_functions` issues facts only for `hir.functions` +/// entries and the caller-side seed only fires on a bare `Expr::FuncRef` +/// callee, which a closure call never is. A classifier that keyed servedness +/// on the syntax of the returns alone — rather than on the fact — would mark +/// this whole population served and delete the wall it is supposed to measure. +#[test] +fn a_closure_region_never_reports_a_served_return() { + let c = class_with_fields("C", &["x"]); + let mut classes = HashMap::new(); + classes.insert("C".to_string(), &c); + let stmts = vec![Stmt::Return(Some(new_c()))]; + + let session = Session::start(); + let _guard = crate::opt_report::enter_closure("mk", 7); + let _ = run(&stmts, &classes); + drop(_guard); + let entries = session.entries(); + + let rows = alloc_rows(&entries); + assert_eq!(rows.len(), 1); + assert_eq!( + rows[0].rule.as_deref(), + Some("rule 1 (provenance)"), + "a closure body cannot carry a return-shape fact" + ); +} + +/// Only the RETURN position is served, across both nesting shapes a `return` +/// can put an allocation in: a constructor argument of the returned `new`, and +/// an argument of a returned *call*. +/// +/// Both are in the body. The doc used to cite `return f(...)` while the body +/// built only `return new C(new C())` — a different code path, since the +/// `Expr::New` arm overrides the context and the `Expr::Call` arm is what +/// handles the other. +#[test] +fn only_the_return_position_is_marked_served() { + let c = class_with_fields("C", &["x"]); + let mut classes = HashMap::new(); + classes.insert("C".to_string(), &c); + // `return new C(new C())` — outer is the return, inner is an argument. + let stmts = vec![Stmt::Return(Some(Expr::New { + class_name: "C".to_string(), + args: vec![new_c()], + type_args: Vec::new(), + byte_offset: 0, + cap_args_appended: 0, + }))]; + + let session = Session::start(); + let _guard = crate::opt_report::enter_function_region("mk", true); + let _ = run(&stmts, &classes); + drop(_guard); + let entries = session.entries(); + + let rows = alloc_rows(&entries); + assert_eq!(rows.len(), 2, "outer return plus inner argument"); + let served: Vec<_> = rows + .iter() + .filter(|e| e.tier == Some(crate::opt_report::Tier::Served)) + .collect(); + assert_eq!(served.len(), 1, "exactly the return position is served"); + assert_eq!(served[0].alloc_context.as_deref(), Some("return")); + + // `return f(new C())` — the allocation is a CALL ARGUMENT. Nothing here is + // a return position, so nothing is served even in a producer region. + // + // Reuses the SAME `session`: `Session::start` takes a process-global, + // non-reentrant lock, so opening a second one while the first is alive + // deadlocks. `entries()` has already drained the sink, so the second phase + // starts clean. + let called = vec![Stmt::Return(Some(Expr::Call { + callee: Box::new(Expr::FuncRef(9)), + args: vec![new_c()], + type_args: Vec::new(), + byte_offset: 0, + }))]; + + let _guard = crate::opt_report::enter_function_region("mk", true); + let _ = run(&called, &classes); + drop(_guard); + let entries = session.entries(); + + let rows = alloc_rows(&entries); + assert_eq!(rows.len(), 1); + assert_eq!(rows[0].alloc_context.as_deref(), Some("call argument")); + assert_ne!(rows[0].tier, Some(crate::opt_report::Tier::Served)); +} + +/// Defect 1 (PR #7171 §3). Every anonymous literal renders as +/// `object literal { ... }` and carries `byte_offset: 0`, so +/// `(module, function, name, position, analysis, outcome, rule)` was the same +/// tuple for every unbound literal in a function and all but one vanished. +/// PR #7171 measured the consequence directly: suppressing 379 scaffolding +/// rows REVEALED 62 real user rows the dedup had been hiding. +#[test] +fn two_unbound_literals_in_one_function_are_two_rows() { + let classes = HashMap::new(); + let stmts = vec![ + Stmt::Expr(anon_shape(Vec::new())), + Stmt::Expr(anon_shape(Vec::new())), + ]; + + let session = Session::start(); + let _ = run(&stmts, &classes); + let entries = session.entries(); + + let rows = alloc_rows(&entries); + assert_eq!( + rows.len(), + 2, + "two syntactic allocations are two rows, not one: {rows:#?}" + ); + let mut ordinals: Vec<_> = rows.iter().filter_map(|e| e.alloc_ordinal).collect(); + ordinals.sort_unstable(); + assert_eq!(ordinals, vec![0, 1]); +} + +/// ...and the de-duplication that motivated the key still works: a function +/// lowered twice (a boxed entry plus a typed clone) walks the same body and +/// produces the same ordinals, so its rows still collapse — and the count of +/// what collapsed is reported rather than left implicit. +#[test] +fn a_region_lowered_twice_still_collapses_and_says_how_much() { + let classes = HashMap::new(); + let stmts = vec![ + Stmt::Expr(anon_shape(Vec::new())), + Stmt::Expr(anon_shape(Vec::new())), + ]; + + let session = Session::start(); + let _guard = crate::opt_report::enter_function_region("twice", false); + let _ = run(&stmts, &classes); + let _ = run(&stmts, &classes); + drop(_guard); + let entries = session.entries(); + + assert_eq!( + alloc_rows(&entries).len(), + 2, + "the second lowering collapses" + ); + assert_eq!( + crate::opt_report::masked_by_dedup(), + 2, + "and the report says how many rows it folded away" + ); +} + +// ── #7176 review, Major 1: `return` is a POSITION, not "anywhere under a +// returned expression" ───────────────────────────────────────────────── +// +// `RETURN` is set once, at `Stmt::Return(Some(e))`, and `scan_expr` propagates +// `ctx` unchanged through its fallback arm — only `New`, `Call` arguments and +// `Array` override it. So every allocation nested inside a returned expression +// inherited the `return` label and, in a region flagged as a return-shape +// producer, `Tier::Served` with it. +// +// The return-shape fact covers the function's RETURN VALUE. It says nothing +// about an operand of a conditional, a `&&`, an `await` or a member base that +// happens to sit inside the returned expression. Two consequences, both live: +// +// * the `return` bucket over-counts — and 323 of it was published as R1's +// ceiling on #7170; +// * servedness is decided by a string that means the wrong thing. Today no +// production region can hit that (`producer_return_class` admits only a +// bare `Expr::New` or `Expr::LocalGet` return, so a ternary/await/binary +// return yields no fact at all) — but that is a distant invariant in +// another file, and R2 widening the producer side would silently turn this +// into a wrong `Served` row. These tests force the producer flag on so the +// classifier is tested on its own terms rather than on that invariant. + +fn ternary(a: Expr, b: Expr) -> Expr { + Expr::Conditional { + condition: Box::new(Expr::Bool(true)), + then_expr: Box::new(a), + else_expr: Box::new(b), + } +} + +/// Run with the region flagged as a return-shape producer, which is the only +/// state in which the served classification can fire at all. +fn run_as_producer( + stmts: &[Stmt], + classes: &HashMap, +) -> Vec { + let session = Session::start(); + let guard = crate::opt_report::enter_function_region("mk", true); + let _ = run(stmts, classes); + drop(guard); + session.entries() +} + +fn c_classes() -> Class { + class_with_fields("C", &["x"]) +} + +/// `return cond ? new C() : new C()` — two operands of a conditional, neither +/// of which is the function's return value. +#[test] +fn a_conditional_arm_under_a_return_is_not_a_return_position() { + let c = c_classes(); + let mut classes = HashMap::new(); + classes.insert("C".to_string(), &c); + let stmts = vec![Stmt::Return(Some(ternary(new_c(), new_c())))]; + + let entries = run_as_producer(&stmts, &classes); + let rows = alloc_rows(&entries); + assert_eq!(rows.len(), 2); + for e in &rows { + assert_ne!( + e.alloc_context.as_deref(), + Some("return"), + "a conditional arm is not the returned value" + ); + assert_ne!( + e.tier, + Some(crate::opt_report::Tier::Served), + "the return-shape fact does not cover a conditional arm" + ); + } +} + +/// `return flag && new C()` — a binary operand. +#[test] +fn a_logical_operand_under_a_return_is_not_a_return_position() { + let c = c_classes(); + let mut classes = HashMap::new(); + classes.insert("C".to_string(), &c); + let stmts = vec![Stmt::Return(Some(Expr::Logical { + op: perry_hir::LogicalOp::And, + left: Box::new(Expr::Bool(true)), + right: Box::new(new_c()), + }))]; + + let entries = run_as_producer(&stmts, &classes); + let rows = alloc_rows(&entries); + assert_eq!(rows.len(), 1); + assert_ne!(rows[0].alloc_context.as_deref(), Some("return")); + assert_ne!(rows[0].tier, Some(crate::opt_report::Tier::Served)); +} + +/// `return await new C()` — the awaited operand is not the returned value +/// either; the promise's resolution is. +#[test] +fn an_awaited_operand_under_a_return_is_not_a_return_position() { + let c = c_classes(); + let mut classes = HashMap::new(); + classes.insert("C".to_string(), &c); + let stmts = vec![Stmt::Return(Some(Expr::Await(Box::new(new_c()))))]; + + let entries = run_as_producer(&stmts, &classes); + let rows = alloc_rows(&entries); + assert_eq!(rows.len(), 1); + assert_ne!(rows[0].alloc_context.as_deref(), Some("return")); + assert_ne!(rows[0].tier, Some(crate::opt_report::Tier::Served)); +} + +/// `return new C().x` — the allocation is a member-access BASE. What is +/// returned is a field of it, which is not even an object. +#[test] +fn a_member_base_under_a_return_is_not_a_return_position() { + let c = c_classes(); + let mut classes = HashMap::new(); + classes.insert("C".to_string(), &c); + let stmts = vec![Stmt::Return(Some(Expr::PropertyGet { + object: Box::new(new_c()), + property: "x".to_string(), + byte_offset: 0, + }))]; + + let entries = run_as_producer(&stmts, &classes); + let rows = alloc_rows(&entries); + assert_eq!(rows.len(), 1); + assert_ne!(rows[0].alloc_context.as_deref(), Some("return")); + assert_ne!(rows[0].tier, Some(crate::opt_report::Tier::Served)); +} + +/// The anti-vacuity half: a BARE `return new C()` — the one shape the +/// return-shape fact actually covers — must still be `return` and still be +/// served. Without this, "never mark anything served" passes all four above. +#[test] +fn a_bare_return_is_still_a_return_position_and_still_served() { + let c = c_classes(); + let mut classes = HashMap::new(); + classes.insert("C".to_string(), &c); + let stmts = vec![Stmt::Return(Some(new_c()))]; + + let entries = run_as_producer(&stmts, &classes); + let rows = alloc_rows(&entries); + assert_eq!(rows.len(), 1); + assert_eq!(rows[0].alloc_context.as_deref(), Some("return")); + assert_eq!(rows[0].tier, Some(crate::opt_report::Tier::Served)); +} diff --git a/crates/perry-codegen/src/collectors/ptr_shape_report.rs b/crates/perry-codegen/src/collectors/ptr_shape_report.rs index 628fbae0c8..46546380e1 100644 --- a/crates/perry-codegen/src/collectors/ptr_shape_report.rs +++ b/crates/perry-codegen/src/collectors/ptr_shape_report.rs @@ -46,6 +46,11 @@ pub(super) struct ShapeDenial { } const RULE1: &str = "rule 1 (provenance)"; +/// #7170 R0. A **separate rule string**, not a flavour of [`RULE1`]: the rule +/// name is what every scheduler-facing tally buckets on (#7152 and #7170 both +/// counted `rule 1` rows straight off the report), so a site that an existing +/// mechanism already serves has to leave that bucket, not merely annotate it. +const RULE1_SERVED: &str = "rule 1 (provenance) — already served by return-shape"; const RULE2: &str = "rule 2 (containment)"; const RULE3: &str = "rule 3 (this-flow containment)"; const RULE4: &str = "rule 4 (dispatch stability)"; @@ -64,6 +69,39 @@ pub(super) const UNBOUND_ALLOC: ShapeDenial = ShapeDenial { issue: Some("#7034 §4 (return-shape facts)"), }; +/// #7170 §5.2, R0: the same syntactic site as [`UNBOUND_ALLOC`], in the return +/// position of a function that **already carries a return-shape fact**. +/// +/// `report::deny_alloc_site` runs at the top of +/// `collect_shape_proven_ptr_locals`, before any seeding — it cannot see that +/// `collectors/ptr_shape_returns.rs` admits a bare `return new C(...)` with no +/// local at all as a producer, so the class does reach every +/// `const r = producer(…)` caller. Left under [`UNBOUND_ALLOC`] these rows +/// counted as rule-1 misses. +/// +/// **How big this correction actually is: 4 sites of 1842**, measured over 196 +/// real dependency modules. #7170 §5.2 expected it to be a material share of +/// the rule-1 wall and it is not — because only 62 of those 1842 allocations +/// are in a `function` region at all (1711 are in closures, which cannot carry +/// a return-shape fact; #7170 §6). The bucket is separated anyway: 4 is the +/// honest number only once the classification exists, and "small" is a result +/// rather than a reason to leave two mechanisms merged. +/// +/// **What this asserts, precisely:** the fact was *issued* for the enclosing +/// function. Whether any caller consumed it is a different question with a +/// different counter (`Outcome::Consumed`), and this row makes no claim about +/// it. +pub(super) const UNBOUND_ALLOC_SERVED_RETURN: ShapeDenial = ShapeDenial { + rule: RULE1_SERVED, + reason: "returned from a function that carries a return-shape fact, so its \ + class already reaches every `const r = producer(...)` call site \ + (#7107). The allocation itself is still unbound — rule 1 cannot \ + anchor to it — but this is NOT an unserved position, and counting \ + it as one over-states the rule-1 wall.", + tier: Tier::Served, + issue: Some("#7107 (return-shape facts, shipped)"), +}; + pub(super) const LET_INIT_NOT_NEW: ShapeDenial = ShapeDenial { rule: RULE1, reason: "the binding is re-declared with an initializer that is not the \ @@ -365,23 +403,44 @@ pub(super) fn deny_local( } /// Record an allocation site that never became a candidate (rule 1). +/// +/// #7170 R0 changed two things here and nothing else: +/// +/// 1. The denial is [`UNBOUND_ALLOC_SERVED_RETURN`] when the site is in return +/// position of a function that already carries a return-shape fact, so the +/// served population leaves the rule-1 bucket instead of inflating it. +/// 2. The syntactic position and the site's walk ordinal are recorded as +/// first-class fields, which is what lets `Entry::dedup_key` tell two +/// allocations in one function apart. Every object literal renders as +/// `object literal { ... }` and carries `byte_offset: 0`, so before this +/// they collapsed onto one row. pub(super) fn deny_alloc_site(site: &NewSite) { if !opt_report::enabled() || suppressed() { return; } - opt_report::deny(Denial { - position: Position::AllocSite, - name: &site.display, - local_id: None, - analysis: Analysis::PtrShape, - rule: UNBOUND_ALLOC.rule, - reason: UNBOUND_ALLOC.reason, - tier: UNBOUND_ALLOC.tier, - issue: UNBOUND_ALLOC.issue, - loop_depth: site.loop_depth, - detail: Some(format!("allocation position: {}", site.context)), - byte_offset: (site.byte_offset != 0).then_some(site.byte_offset), - }); + let served = site.is_return_position && opt_report::region_is_return_shape_producer(); + let d = if served { + UNBOUND_ALLOC_SERVED_RETURN + } else { + UNBOUND_ALLOC + }; + opt_report::deny_alloc( + Denial { + position: Position::AllocSite, + name: &site.display, + local_id: None, + analysis: Analysis::PtrShape, + rule: d.rule, + reason: d.reason, + tier: d.tier, + issue: d.issue, + loop_depth: site.loop_depth, + detail: Some(format!("allocation position: {}", site.context)), + byte_offset: (site.byte_offset != 0).then_some(site.byte_offset), + }, + site.context, + site.ordinal, + ); } // ── Auxiliary walks (only run under the report flag) ─────────────────────── @@ -456,6 +515,56 @@ fn walk_lets(stmts: &[Stmt], depth: u32, f: &mut impl FnMut(u32, &str, u32)) { } } +// ── Allocation-position vocabulary ───────────────────────────────────────── +// +// Named constants rather than inline literals, because #7170 §5.1 is a bug +// about exactly one of these strings meaning two different things. +// +// None of these strings is load-bearing: servedness is decided by +// [`NewSite::is_return_position`], set at the one site that knows it. A label +// here can be renamed without silently disabling a classification. + +/// The allocation IS the function's return value: `return new C(...)`. +const RETURN: &str = "return"; +/// The allocation sits *inside* a returned expression but is not the returned +/// value — a conditional arm, a `&&` operand, an awaited operand, a member +/// access base. +/// +/// Split out in review of #7176. `RETURN` was set once, at +/// `Stmt::Return(Some(e))`, and `scan_expr` propagates its context unchanged +/// through the fallback arm, so `return cond ? new C() : new D()` filed both +/// arms as return positions. That over-counted the `return` bucket — 323 of +/// which was published on #7170 as R1's ceiling — and would have handed +/// `Tier::Served` to operands the return-shape fact does not cover the moment +/// the producer side widened. +const RETURNED_OPERAND: &str = "returned expression operand"; +/// A genuine `new C(arg)` argument — the developer wrote a constructor call. +const CTOR_ARG: &str = "constructor argument"; +/// A property value of an anonymous-shape object literal. +/// +/// #7170 §5.1: a closed-shape literal lowers to +/// `new __AnonShape_N(v0, v1, …)` whose constructor arguments *are* its +/// property values (`perry-hir/src/lower/expr_object.rs`), so `{a: {b: 1}}` +/// filed its inner literal under [`CTOR_ARG`]. Measured on 197 dependency +/// modules the two are 2.0% and 24.5% of sites respectively — one bucket was +/// 92% the other thing. They are not the same opportunity: a nested literal is +/// a field value of a parent allocation that is itself unbound, so proving its +/// shape licenses nothing on its own. +const ANON_SHAPE_COMPONENT: &str = "object literal property value"; + +/// The allocation-position label for the arguments of `class_name`. +fn arg_context(class_name: &str) -> &'static str { + if is_anon_shape(class_name) { + ANON_SHAPE_COMPONENT + } else { + CTOR_ARG + } +} + +fn is_anon_shape(class_name: &str) -> bool { + class_name.starts_with("__AnonShape") +} + /// An object allocation that is not the initializer of a `Stmt::Let`. pub(super) struct NewSite { /// `new Row(...)` / `{ key, value }` — what the developer wrote. @@ -463,6 +572,18 @@ pub(super) struct NewSite { /// Where it sits: `return`, `call argument`, `array element`, … pub context: &'static str, pub loop_depth: u32, + /// Index of this site in the region's walk. A de-duplication discriminant; + /// see [`crate::opt_report::Entry::alloc_ordinal`]. + pub ordinal: u32, + /// This allocation **is** the expression of a `Stmt::Return` — the value the + /// function hands back — rather than something nested inside it. + /// + /// The only input to the served-return classification, and set in exactly + /// one place ([`scan_return`]) together with the `context` label, so a + /// sabotage cannot kill one without the other. Deriving servedness from the + /// label string instead would make a cosmetic rename of a report bucket + /// silently disable it. + pub is_return_position: bool, /// Byte offset of the `new` expression in its module's source. `Expr::New` /// is the one HIR node that already carries a source position (#5253, /// captured for constructor TypeErrors), so allocation sites — which have @@ -503,11 +624,14 @@ fn scan_stmts( // The Let init IS the provenance site rule 1 accepts; skip it and // scan only its arguments. Stmt::Let { - init: Some(Expr::New { args, .. }), + init: Some(Expr::New { + class_name, args, .. + }), .. } => { + let ctx = arg_context(class_name); for a in args { - scan_expr(a, depth, "constructor argument", out); + scan_expr(a, depth, ctx, out); } } Stmt::Let { init, .. } => { @@ -517,7 +641,7 @@ fn scan_stmts( } Stmt::Expr(e) => scan_expr(e, depth, ctx, out), Stmt::Throw(e) => scan_expr(e, depth, "throw", out), - Stmt::Return(Some(e)) => scan_expr(e, depth, "return", out), + Stmt::Return(Some(e)) => scan_return(e, depth, out), Stmt::Return(None) => {} Stmt::If { condition, @@ -594,6 +718,52 @@ fn scan_stmts( } } +/// Scan the expression of a `Stmt::Return`. +/// +/// **The direct expression of a `return` is the function's return value; +/// anything nested inside it is an operand.** `return cond ? new C() : new D()` +/// returns the *conditional*, not either allocation, and #7107's return-shape +/// fact says nothing about them. +/// +/// This is the only place `RETURN` and [`NewSite::is_return_position`] are set, +/// and they are set together, so no sabotage can leave the label and the +/// classification disagreeing. +fn scan_return(e: &Expr, depth: u32, out: &mut Vec) { + match e { + Expr::New { + class_name, + args, + byte_offset, + .. + } => { + push_new_site(out, class_name, RETURN, depth, *byte_offset, true); + let arg_ctx = arg_context(class_name); + for a in args { + scan_expr(a, depth, arg_ctx, out); + } + } + _ => scan_expr(e, depth, RETURNED_OPERAND, out), + } +} + +fn push_new_site( + out: &mut Vec, + class_name: &str, + context: &'static str, + depth: u32, + byte_offset: u32, + is_return_position: bool, +) { + out.push(NewSite { + display: display_class(class_name), + context, + loop_depth: depth, + ordinal: out.len() as u32, + byte_offset, + is_return_position, + }); +} + fn scan_expr(e: &Expr, depth: u32, ctx: &'static str, out: &mut Vec) { match e { // A closure body is its own lowering region; it reports separately. @@ -604,14 +774,12 @@ fn scan_expr(e: &Expr, depth: u32, ctx: &'static str, out: &mut Vec) { byte_offset, .. } => { - out.push(NewSite { - display: display_class(class_name), - context: ctx, - loop_depth: depth, - byte_offset: *byte_offset, - }); + // Never a return position: `scan_return` handles the one expression + // that is, and it does not route through here. + push_new_site(out, class_name, ctx, depth, *byte_offset, false); + let arg_ctx = arg_context(class_name); for a in args { - scan_expr(a, depth, "constructor argument", out); + scan_expr(a, depth, arg_ctx, out); } } Expr::Call { callee, args, .. } => { @@ -633,7 +801,7 @@ fn scan_expr(e: &Expr, depth: u32, ctx: &'static str, out: &mut Vec) { /// "__AnonShape_…" }`; render them as the object literals the developer /// actually wrote rather than leaking the synthetic class name. fn display_class(class_name: &str) -> String { - if class_name.starts_with("__AnonShape") { + if is_anon_shape(class_name) { String::from("object literal { ... }") } else { format!("new {class_name}(...)") diff --git a/crates/perry-codegen/src/opt_report/mod.rs b/crates/perry-codegen/src/opt_report/mod.rs index 9b5a08442b..771f958358 100644 --- a/crates/perry-codegen/src/opt_report/mod.rs +++ b/crates/perry-codegen/src/opt_report/mod.rs @@ -100,6 +100,9 @@ pub(crate) mod test_support { .unwrap_or_else(|e| e.into_inner()); FORCED.store(true, Ordering::Relaxed); let _ = super::take_entries(); + // The drain above may itself have collapsed leftovers; zero the + // counter so a hand-built render test never reads a neighbour's. + super::MASKED_BY_DEDUP.store(0, Ordering::Relaxed); Session { _guard: guard } } @@ -112,6 +115,7 @@ pub(crate) mod test_support { .unwrap_or_else(|e| e.into_inner()); FORCED.store(false, Ordering::Relaxed); let _ = super::take_entries(); + super::MASKED_BY_DEDUP.store(0, Ordering::Relaxed); Session { _guard: guard } } @@ -299,14 +303,43 @@ pub enum Tier { /// Perry's limitation, not the program's. This tier doubles as a roadmap /// generator: `issue` names the tracking issue where one exists. CompilerLimitation, + /// The value is denied *at this position* but a different, already-shipped + /// mechanism carries its shape onward, so it is **not** a missed + /// opportunity and must not be counted as one (#7170 R0). + /// + /// This tier exists because the other three all answer "who should fix + /// it?", and for these rows the answer is "nobody — it is already fixed + /// elsewhere". Filing them under `CompilerLimitation` (which is where a + /// rule-1 alloc-site denial lands by default) put them in the roadmap + /// bucket a scheduler reads, and #7152/#7170 were both scheduled off a + /// number inflated by exactly that. + /// + /// The explicit rename keeps the serialized spelling identical to + /// [`Tier::as_str`], for the reason `Analysis::PtrNumArray` documents: + /// serde's kebab-case derive would spell this `served` while every + /// heading and text row says `served-elsewhere`, so one tier would appear + /// under two names in a single document and a consumer keying on either + /// would see half the rows. + #[serde(rename = "served-elsewhere")] + Served, } impl Tier { + /// Every tier, in report order. The text renderer enumerates *this* rather + /// than a hand-written list, so a new tier cannot be silently unrendered. + pub const ALL: [Tier; 4] = [ + Tier::Fixable, + Tier::CompilerLimitation, + Tier::Served, + Tier::InherentlyPolymorphic, + ]; + pub fn as_str(self) -> &'static str { match self { Tier::Fixable => "fixable", Tier::InherentlyPolymorphic => "inherently-polymorphic", Tier::CompilerLimitation => "compiler-limitation", + Tier::Served => "served-elsewhere", } } @@ -315,6 +348,7 @@ impl Tier { Tier::Fixable => "Fixable in your source", Tier::InherentlyPolymorphic => "Inherently polymorphic (correctly boxed — no action)", Tier::CompilerLimitation => "Perry limitation (not your code)", + Tier::Served => "Already served by another mechanism (not a missed opportunity)", } } } @@ -396,6 +430,31 @@ pub struct Entry { /// it is a site nobody has ever watched work. Two of the six were in /// exactly that state when this field was added. pub site: Option, + /// For a [`Position::AllocSite`] entry: the syntactic position the + /// allocation sits in (`return`, `constructor argument`, `object literal + /// property value`, …). + /// + /// A first-class field rather than prose inside `detail`, for the same + /// reason `site` is one: this is the bucket a scheduler counts. #7170 §5.1 + /// found the `constructor argument` label conflating genuine `new C(arg)` + /// arguments (2.0% of sites) with the property values of anonymous-shape + /// object literals (24.5%) — two different opportunities needing two + /// different fixes, merged into one number by the label alone. + pub alloc_context: Option, + /// For a [`Position::AllocSite`] entry: the site's index in its region's + /// deterministic report walk. + /// + /// **A de-duplication discriminant, not a source location.** Anonymous + /// object literals all render as `object literal { ... }` and all carry + /// `byte_offset: 0` (`perry-hir/src/lower/expr_object.rs` never populates + /// one for them), so before this field every unbound literal in a function + /// collapsed onto a single row in [`Entry::dedup_key`] — PR #7171 measured + /// suppressing 379 scaffolding rows *revealing* 62 previously-masked real + /// ones, and suppressing a single record moving its row between buckets + /// while changing the total by zero. The walk is a plain `Vec` traversal + /// with no map iteration, so a function lowered twice (a boxed entry plus + /// a typed clone) produces the same ordinals and still de-duplicates. + pub alloc_ordinal: Option, } impl Entry { @@ -438,6 +497,17 @@ impl Entry { /// It is kept because a dedup key should carry the full discriminant: the /// cost is one tuple element, and the failure mode if a future rule name /// collides is silent data loss in the report that feeds the census. + /// - **`alloc_ordinal`** separates the several unbound allocation sites of + /// one function from each other. It is `None` for every other position, + /// so no other tally moves. See [`Entry::alloc_ordinal`] for why the + /// pre-existing key could not tell them apart, and + /// [`masked_by_dedup`] for what is still being collapsed. + /// + /// `alloc_context` is deliberately **not** in this key. It is a function + /// of the site, so the ordinal already implies it, and a second + /// enforcement point that no sabotage can kill is how #7171 ended up + /// with four green holes in its first pass. One discriminant, one test. + #[allow(clippy::type_complexity)] fn dedup_key( &self, ) -> ( @@ -449,6 +519,7 @@ impl Entry { Outcome, Option, Option, + Option, ) { ( self.module.clone(), @@ -468,6 +539,7 @@ impl Entry { // and selection tallies (and therefore the baseline's `candidates` // column) are unchanged. self.site.clone(), + self.alloc_ordinal, ) } } @@ -480,6 +552,20 @@ struct Scope { function: String, region: RegionKind, invoked_per_element: Option, + /// #7170 R0: this region is a function that carries a **return-shape fact** + /// (`collectors/ptr_shape_returns.rs`, #7107), so its `return new C(...)` + /// sites already feed an existing mechanism. + /// + /// Set by exactly one caller — [`enter_function_region`], from + /// `codegen/function.rs`, which is the only place that holds both the + /// `FuncId` and `ModuleDispatchFacts`. Every other region (method, closure, + /// module-init) leaves it `false`, and that is CORRECT rather than + /// conservative: `collect_return_shape_functions` issues facts only for + /// `hir.functions` entries, and the caller-side seed + /// (`find_return_shape_candidates`) only fires on a bare `Expr::FuncRef` + /// callee — which a closure call never is. #7170 §6 is precisely the + /// measurement that closures are *not* served. + return_shape_producer: bool, } thread_local! { @@ -523,6 +609,48 @@ pub(crate) fn enter_region(function: &str, region: RegionKind) -> ScopeGuard { enter(&module, function, region) } +/// [`enter_region`] for a top-level function body, additionally recording +/// whether this function carries a **return-shape fact** (#7107). +/// +/// Report-only: the flag is read by nothing but +/// [`region_is_return_shape_producer`], which only the `Ptr` alloc-site +/// recorder consults. Like every other entry point here it is inert — and +/// allocation-free — when the report is off. +pub(crate) fn enter_function_region(function: &str, return_shape_producer: bool) -> ScopeGuard { + if !enabled() { + return ScopeGuard { + previous: None, + active: false, + }; + } + let scope = Scope { + module: current_module(), + function: function.to_string(), + region: RegionKind::Function, + invoked_per_element: None, + return_shape_producer, + }; + let previous = SCOPE.with(|s| s.borrow_mut().replace(scope)); + ScopeGuard { + previous, + active: true, + } +} + +/// Whether the region currently being lowered carries a return-shape fact. +/// +/// `false` with no scope at all, which is the safe direction: an unattributed +/// site is reported as an ordinary rule-1 denial rather than silently written +/// off as served. +pub(crate) fn region_is_return_shape_producer() -> bool { + SCOPE.with(|s| { + s.borrow() + .as_ref() + .map(|sc| sc.return_shape_producer) + .unwrap_or(false) + }) +} + fn current_module() -> String { SCOPE.with(|s| { s.borrow() @@ -564,6 +692,7 @@ pub(crate) fn enter(module: &str, function: &str, region: RegionKind) -> ScopeGu function: function.to_string(), region, invoked_per_element: None, + return_shape_producer: false, }; let previous = SCOPE.with(|s| s.borrow_mut().replace(scope)); ScopeGuard { @@ -587,6 +716,10 @@ pub(crate) fn enter_closure(function: &str, func_id: u32) -> ScopeGuard { function: function.to_string(), region: RegionKind::Closure, invoked_per_element: per_element_role(Some(func_id)), + // #7170 §6: a closure is never a return-shape producer — the caller-side + // seed requires a bare `Expr::FuncRef` callee. This `false` is the + // measurement, not a default. + return_shape_producer: false, }; let previous = SCOPE.with(|s| s.borrow_mut().replace(scope)); ScopeGuard { @@ -666,6 +799,27 @@ pub(crate) struct Denial<'a> { /// `detail` costs anything: the arguments are evaluated before this function /// can early-return. pub(crate) fn deny(d: Denial<'_>) { + deny_in_scope(d, None, None); +} + +/// [`deny`] for a [`Position::AllocSite`] row, carrying the two fields that +/// make an unbound allocation identifiable: the syntactic position it sits in +/// and its ordinal in the region's report walk. +/// +/// A separate entry point rather than two more fields on [`Denial`], because +/// every other denial site would then have to spell `None` twice for a pair of +/// fields only this one can populate. +pub(crate) fn deny_alloc(d: Denial<'_>, alloc_context: &str, alloc_ordinal: u32) { + if !enabled() { + return; + } + deny_in_scope(d, Some(alloc_context.to_string()), Some(alloc_ordinal)); +} + +/// The one body behind [`deny`] and [`deny_alloc`]: build a `Denied` entry from +/// the ambient scope. Kept single so the two entry points cannot drift in what +/// they attribute. +fn deny_in_scope(d: Denial<'_>, alloc_context: Option, alloc_ordinal: Option) { if !enabled() { return; } @@ -702,6 +856,8 @@ pub(crate) fn deny(d: Denial<'_>) { detail: d.detail, byte_offset: d.byte_offset, site: None, + alloc_context, + alloc_ordinal, }); } @@ -732,6 +888,8 @@ pub(crate) fn deny_named(function: &str, region: RegionKind, d: Denial<'_>) { detail: d.detail, byte_offset: d.byte_offset, site: None, + alloc_context: None, + alloc_ordinal: None, }); } @@ -790,6 +948,8 @@ pub(crate) fn select( detail, byte_offset: None, site: None, + alloc_context: None, + alloc_ordinal: None, }); } @@ -826,6 +986,8 @@ pub(crate) fn select_explicit( detail: None, byte_offset: None, site: None, + alloc_context: None, + alloc_ordinal: None, }); } @@ -895,6 +1057,8 @@ pub(crate) fn consume( detail: Some(format!("consumed at {site}")), byte_offset: None, site: Some(site.to_string()), + alloc_context: None, + alloc_ordinal: None, }); } @@ -943,9 +1107,29 @@ pub(crate) fn unconsumed(u: Unconsumed<'_>) { detail: u.detail, byte_offset: None, site: None, + alloc_context: None, + alloc_ordinal: None, }); } +/// How many entries the most recent [`take_entries`] collapsed. +/// +/// #7170 R0: de-duplication is not free information hygiene — it is a lossy +/// step between "what the compiler recorded" and "what a scheduler counts", +/// and PR #7171 measured it changing which bucket a row lands in while leaving +/// the total at zero. The count is therefore reported rather than left +/// implicit, so a reader can see how much of the population is still being +/// folded away instead of having to assume it is none. +/// +/// Set (not accumulated) by each drain; `0` before the first one. +static MASKED_BY_DEDUP: std::sync::atomic::AtomicUsize = std::sync::atomic::AtomicUsize::new(0); + +/// Entries collapsed by the most recent [`take_entries`]. See +/// [`MASKED_BY_DEDUP`]. +pub fn masked_by_dedup() -> usize { + MASKED_BY_DEDUP.load(std::sync::atomic::Ordering::Relaxed) +} + /// Drain every recorded entry, de-duplicated and ranked. Called once by the /// CLI after module codegen finishes. pub fn take_entries() -> Vec { @@ -953,8 +1137,10 @@ pub fn take_entries() -> Vec { Ok(mut v) => std::mem::take(&mut *v), Err(_) => Vec::new(), }; + let before = entries.len(); let mut seen = std::collections::HashSet::new(); entries.retain(|e| seen.insert(e.dedup_key())); + MASKED_BY_DEDUP.store(before - entries.len(), std::sync::atomic::Ordering::Relaxed); entries.sort_by(|a, b| a.rank().cmp(&b.rank())); entries } @@ -983,6 +1169,8 @@ mod tests { detail: None, byte_offset: None, site: None, + alloc_context: None, + alloc_ordinal: None, } } @@ -1020,6 +1208,38 @@ mod tests { } } + /// One tier, one spelling — the same trap `serialized_analysis_matches_as_str` + /// guards for analyses. A tier whose serde name differs from its `as_str` + /// name appears twice in one document and a consumer keying on either sees + /// half the rows. + #[test] + fn serialized_tier_matches_as_str() { + for tier in Tier::ALL { + let serialized = serde_json::to_string(&tier).unwrap(); + assert_eq!( + serialized.trim_matches('"'), + tier.as_str(), + "serde and as_str disagree for {tier:?}" + ); + } + } + + /// `Tier::ALL` must actually enumerate the enum. The text renderer loops + /// over it, so a tier missing here is a tier that is in the JSON and not in + /// the human report. + #[test] + fn tier_all_lists_every_variant() { + for tier in [ + Tier::Fixable, + Tier::InherentlyPolymorphic, + Tier::CompilerLimitation, + Tier::Served, + ] { + assert!(Tier::ALL.contains(&tier), "{tier:?} missing from Tier::ALL"); + } + assert_eq!(Tier::ALL.len(), 4); + } + #[test] fn dedup_key_collapses_a_function_lowered_twice() { let a = entry("f", 0, None); diff --git a/crates/perry-codegen/src/opt_report/render.rs b/crates/perry-codegen/src/opt_report/render.rs index 2347217a10..84d0d9f0f7 100644 --- a/crates/perry-codegen/src/opt_report/render.rs +++ b/crates/perry-codegen/src/opt_report/render.rs @@ -73,7 +73,18 @@ fn hotness(e: &Entry) -> String { } /// Human-readable report. +/// +/// Reads the process-global de-duplication counter. **Tests must use +/// [`render_text_with`]**: the renderers take a snapshot (`entries`) while that +/// counter is live, so a test asserting on the "duplicate row(s) collapsed" +/// line without holding a `test_support::Session` would observe whatever a +/// concurrently-running collector test last stored. pub fn render_text(entries: &[Entry]) -> String { + render_text_with(entries, super::masked_by_dedup()) +} + +/// [`render_text`] with the de-duplication count supplied explicitly. +pub fn render_text_with(entries: &[Entry], masked: usize) -> String { let mut out = String::new(); let tallies = tally(entries); @@ -134,8 +145,36 @@ pub fn render_text(entries: &[Entry]) -> String { " {:<16} {total_selected} values unboxed, {total_denied} left Boxed", "TOTAL", ); + if masked > 0 { + let _ = writeln!( + out, + " {:<16} {masked} duplicate row(s) collapsed by de-duplication", + "", + ); + } out.push('\n'); + // ── Unbound allocation sites, by position ────────────────────────────── + // #7170 R0. These rows are the majority of the `Ptr` denial + // population on real code, and their positions are what a scheduler reads + // to decide which escape to open next. Printed as an explicit table so + // that reading is not a `jq` reduction over `entries[]` whose bucket + // definitions live nowhere. + let alloc = alloc_buckets(entries); + if !alloc.is_empty() { + let total: usize = alloc.iter().map(|b| b.denied).sum(); + let _ = writeln!(out, "Unbound allocation sites by position ({total})"); + out.push_str("-------------------------------------\n"); + for b in &alloc { + let _ = writeln!( + out, + " {:<34} {:>5} {} / {}", + b.context, b.denied, b.analysis, b.rule, + ); + } + out.push('\n'); + } + // ── Headline: call out any representation that promoted nothing ──────── // This is the #7034 §0 finding, stated without having to read IR. let mut zeros: Vec<(&Analysis, &AnalysisTally)> = tallies @@ -218,11 +257,7 @@ pub fn render_text(entries: &[Entry]) -> String { ); } - for tier in [ - Tier::Fixable, - Tier::CompilerLimitation, - Tier::InherentlyPolymorphic, - ] { + for tier in Tier::ALL { let rows: Vec<&&Entry> = denials .iter() .filter(|e| e.tier == Some(tier)) @@ -256,6 +291,9 @@ pub fn render_text(entries: &[Entry]) -> String { if let Some(detail) = &e.detail { let _ = writeln!(out, " {detail}"); } + if let Some(ordinal) = e.alloc_ordinal { + let _ = writeln!(out, " allocation site #{ordinal} in this region"); + } if let Some(offset) = e.byte_offset { let _ = writeln!(out, " source byte offset {offset}"); } @@ -320,13 +358,96 @@ struct JsonAnalysis<'a> { records_consumption: bool, } +/// One `(analysis, rule)` denial bucket. +/// +/// #7170 R0: the bucket counts are computed **here**, by the compiler that +/// owns the rule vocabulary, rather than reconstructed downstream with `jq` +/// over `entries[]`. Every scheduling decision in the #7152/#7170 chain was +/// taken off a hand-rolled reduction of those rows, and two of the three +/// defects R0 fixes were invisible in that reduction by construction: it could +/// not see that one label meant two mechanisms, nor that a bucket contained +/// already-served sites. +#[derive(Debug, serde::Serialize)] +struct JsonRuleBucket<'a> { + analysis: &'a str, + rule: &'a str, + tier: Option, + denied: usize, +} + +/// One `(allocation position, rule)` bucket, over `alloc-site` denials. +/// +/// Separate from [`JsonRuleBucket`] because the position is the discriminant +/// that #7170 §5.1 found merged: `constructor argument` and `object literal +/// property value` share a rule and are different opportunities. +#[derive(Debug, serde::Serialize)] +struct JsonAllocBucket<'a> { + analysis: &'a str, + context: &'a str, + rule: &'a str, + denied: usize, +} + #[derive(Debug, serde::Serialize)] struct JsonSummary<'a> { selected: usize, denied: usize, consumed: usize, unconsumed: usize, + /// Entries the drain collapsed. See + /// [`crate::opt_report::masked_by_dedup`] — a nonzero value here is the + /// count of rows a consumer is NOT seeing. + masked_by_dedup: usize, by_analysis: Vec>, + by_rule: Vec>, + by_alloc_context: Vec>, +} + +fn rule_buckets(entries: &[Entry]) -> Vec> { + let mut out: BTreeMap<(&str, &str), (Option, usize)> = BTreeMap::new(); + for e in entries.iter().filter(|e| e.outcome == Outcome::Denied) { + let rule = e.rule.as_deref().unwrap_or(""); + let slot = out + .entry((e.analysis.as_str(), rule)) + .or_insert((e.tier, 0)); + // Rule-to-tier is 1:1 because both come from one `ShapeDenial` + // constant — but nothing HERE enforces that, and silently keeping the + // first tier seen would let a scheduler-facing bucket mean two things + // at once, which is the defect this PR exists to remove. Assert the + // invariant rather than resolving it. + debug_assert_eq!( + slot.0, e.tier, + "rule {rule} carries two tiers; the bucket would report only one" + ); + slot.1 += 1; + } + out.into_iter() + .map(|((analysis, rule), (tier, denied))| JsonRuleBucket { + analysis, + rule, + tier, + denied, + }) + .collect() +} + +fn alloc_buckets(entries: &[Entry]) -> Vec> { + let mut out: BTreeMap<(&str, &str, &str), usize> = BTreeMap::new(); + for e in entries.iter().filter(|e| e.outcome == Outcome::Denied) { + let Some(context) = e.alloc_context.as_deref() else { + continue; + }; + let rule = e.rule.as_deref().unwrap_or(""); + *out.entry((e.analysis.as_str(), context, rule)).or_insert(0) += 1; + } + out.into_iter() + .map(|((analysis, context, rule), denied)| JsonAllocBucket { + analysis, + context, + rule, + denied, + }) + .collect() } #[derive(Debug, serde::Serialize)] @@ -340,6 +461,12 @@ struct JsonReport<'a> { /// builds and catch a representation regression (a `selected` count silently /// going to zero — exactly what #7034 found by hand). pub fn render_json(entries: &[Entry]) -> String { + render_json_with(entries, super::masked_by_dedup()) +} + +/// [`render_json`] with the de-duplication count supplied explicitly. See +/// [`render_text_with`] for why tests use this form. +pub fn render_json_with(entries: &[Entry], masked: usize) -> String { let tallies = tally(entries); let report = JsonReport { schema_version: SCHEMA_VERSION, @@ -348,6 +475,7 @@ pub fn render_json(entries: &[Entry]) -> String { denied: tallies.values().map(|t| t.denied).sum(), consumed: tallies.values().map(|t| t.consumed).sum(), unconsumed: tallies.values().map(|t| t.unconsumed).sum(), + masked_by_dedup: masked, // Enumerate `Analysis::ALL`, not the analyses that happen to have // entries: an analysis that recorded nothing must appear with an // explicit `0`. A consumer cannot tell an absent key from a zero @@ -369,6 +497,8 @@ pub fn render_json(entries: &[Entry]) -> String { } }) .collect(), + by_rule: rule_buckets(entries), + by_alloc_context: alloc_buckets(entries), }, entries, }; @@ -400,6 +530,8 @@ mod tests { detail: None, byte_offset: None, site: None, + alloc_context: None, + alloc_ordinal: None, } } @@ -423,6 +555,8 @@ mod tests { detail: None, byte_offset: None, site: None, + alloc_context: None, + alloc_ordinal: None, } } @@ -722,3 +856,209 @@ mod tests { ); } } + +#[cfg(test)] +mod r0_bucket_tests { + use super::*; + use crate::opt_report::{Position, RegionKind}; + + fn alloc_site(context: &str, rule: &str, tier: Tier, ordinal: u32) -> Entry { + Entry { + module: "dep.js".into(), + function: "mk".into(), + region: RegionKind::Function, + position: Position::AllocSite, + name: "object literal { ... }".into(), + local_id: None, + analysis: Analysis::PtrShape, + outcome: Outcome::Denied, + rep: "Boxed".into(), + rule: Some(rule.into()), + reason: Some("unbound".into()), + tier: Some(tier), + issue: None, + loop_depth: 0, + invoked_per_element: None, + detail: Some(format!("allocation position: {context}")), + byte_offset: None, + site: None, + alloc_context: Some(context.into()), + alloc_ordinal: Some(ordinal), + } + } + + const RULE1: &str = "rule 1 (provenance)"; + const SERVED: &str = "rule 1 (provenance) — already served by return-shape"; + + /// #7170 R0's primary deliverable, at the schema level: the bucket counts a + /// scheduler reads are computed by the compiler that owns the rule + /// vocabulary, not reconstructed with `jq` over `entries[]`. Two of the + /// three defects R0 fixes were invisible to such a reduction by + /// construction — it could not see that one label meant two mechanisms. + #[test] + fn json_reports_the_alloc_buckets_separately() { + let entries = vec![ + alloc_site("constructor argument", RULE1, Tier::CompilerLimitation, 0), + alloc_site( + "object literal property value", + RULE1, + Tier::CompilerLimitation, + 1, + ), + alloc_site( + "object literal property value", + RULE1, + Tier::CompilerLimitation, + 2, + ), + alloc_site("return", SERVED, Tier::Served, 3), + ]; + let json: serde_json::Value = serde_json::from_str(&render_json_with(&entries, 0)).unwrap(); + let buckets = json["summary"]["by_alloc_context"].as_array().unwrap(); + let find = |ctx: &str| { + buckets + .iter() + .find(|b| b["context"] == ctx) + .unwrap_or_else(|| panic!("missing {ctx} bucket in {buckets:#?}")) + .clone() + }; + assert_eq!(find("constructor argument")["denied"], 1); + assert_eq!(find("object literal property value")["denied"], 2); + assert_eq!(find("return")["denied"], 1); + assert_eq!(find("return")["rule"], SERVED); + } + + /// The wall number itself: `by_rule` must separate the served population + /// from the rule-1 one, because "506 of 963" was a count of the merged + /// bucket and every scheduling decision in the #7152/#7170 chain was taken + /// off it. + #[test] + fn json_by_rule_separates_served_sites_from_the_rule_1_wall() { + let entries = vec![ + alloc_site("return", RULE1, Tier::CompilerLimitation, 0), + alloc_site("return", SERVED, Tier::Served, 1), + alloc_site("return", SERVED, Tier::Served, 2), + ]; + let json: serde_json::Value = serde_json::from_str(&render_json_with(&entries, 0)).unwrap(); + let rules = json["summary"]["by_rule"].as_array().unwrap(); + let row = |rule: &str| { + rules + .iter() + .find(|r| r["rule"] == rule) + .unwrap_or_else(|| panic!("missing rule {rule} in {rules:#?}")) + .clone() + }; + assert_eq!(row(RULE1)["denied"], 1, "the wall is 1, not 3"); + assert_eq!(row(SERVED)["denied"], 2); + assert_eq!(row(SERVED)["tier"], "served-elsewhere"); + } + + /// A served row must not be filed under the roadmap tier. That heading is + /// what a scheduler reads as "Perry's outstanding work", and #7170 R0 + /// exists because already-shipped mechanisms were being counted there. + #[test] + fn a_served_site_renders_under_its_own_heading_not_the_roadmap_one() { + let text = render_text_with(&[alloc_site("return", SERVED, Tier::Served, 0)], 0); + assert!( + text.contains("Already served by another mechanism"), + "got:\n{text}" + ); + assert!( + !text.contains("Perry limitation (not your code)"), + "a served row must not appear in the roadmap tier; got:\n{text}" + ); + } + + /// Every tier must be renderable. The loop enumerates `Tier::ALL`, so a + /// future tier cannot be added and silently left unprinted — which is how + /// a row disappears from the human report while still being in the JSON. + #[test] + fn every_tier_has_a_heading_and_is_rendered() { + for (i, tier) in Tier::ALL.into_iter().enumerate() { + let text = render_text_with(&[alloc_site("return", RULE1, tier, i as u32)], 0); + assert!( + text.contains(tier.heading()), + "{:?} is not rendered; got:\n{text}", + tier + ); + } + } + + /// The allocation-position table is the human-readable half of the same + /// fix, and it must state the position rather than leaving it to prose. + #[test] + fn text_reports_the_allocation_positions_as_a_table() { + let text = render_text_with( + &[ + alloc_site("constructor argument", RULE1, Tier::CompilerLimitation, 0), + alloc_site( + "object literal property value", + RULE1, + Tier::CompilerLimitation, + 1, + ), + ], + 0, + ); + assert!( + text.contains("Unbound allocation sites by position (2)"), + "got:\n{text}" + ); + assert!( + text.contains("object literal property value"), + "got:\n{text}" + ); + } + + /// The de-duplication count is reported only when it is nonzero, and it is + /// supplied explicitly here rather than read from the process-global that + /// `render_text` consults — otherwise this ABSENCE assertion would observe + /// whatever a concurrently-running collector test last stored. + #[test] + fn the_masked_row_count_is_printed_only_when_nonzero() { + let rows = [alloc_site("return", RULE1, Tier::CompilerLimitation, 0)]; + assert!( + !render_text_with(&rows, 0).contains("collapsed by de-duplication"), + "a zero must not print a line" + ); + let text = render_text_with(&rows, 7); + assert!( + text.contains("7 duplicate row(s) collapsed by de-duplication"), + "got:\n{text}" + ); + let json: serde_json::Value = serde_json::from_str(&render_json_with(&rows, 7)).unwrap(); + assert_eq!(json["summary"]["masked_by_dedup"], 7); + } + + /// The rule-to-tier invariant is asserted, not resolved silently — and the + /// assertion is exercised, so it is not a guard nobody has watched fire. + /// + /// `debug_assert` is compiled out in release, which is the right trade for + /// a report-only invariant: the cost of being wrong is one mislabelled + /// bucket, not a miscompile. This test runs in the debug profile where the + /// assertion is live. + #[test] + #[should_panic(expected = "carries two tiers")] + #[cfg(debug_assertions)] + fn one_rule_carrying_two_tiers_is_a_hard_error() { + let a = alloc_site("return", RULE1, Tier::CompilerLimitation, 0); + let b = alloc_site("return", RULE1, Tier::Fixable, 1); + let _ = render_json_with(&[a, b], 0); + } + + /// An entry that is not an allocation site contributes to `by_rule` and to + /// nothing else — the alloc table must not grow rows for ordinary locals. + #[test] + fn a_local_denial_does_not_appear_in_the_alloc_table() { + let mut local = alloc_site("return", RULE1, Tier::Fixable, 0); + local.position = Position::Local; + local.alloc_context = None; + local.alloc_ordinal = None; + let json: serde_json::Value = serde_json::from_str(&render_json_with(&[local], 0)).unwrap(); + assert!(json["summary"]["by_alloc_context"] + .as_array() + .unwrap() + .is_empty()); + assert_eq!(json["summary"]["by_rule"].as_array().unwrap().len(), 1); + } +} diff --git a/scripts/compiler_output_harness/repsel_census.py b/scripts/compiler_output_harness/repsel_census.py index 7799bc8913..d63c12c9b8 100644 --- a/scripts/compiler_output_harness/repsel_census.py +++ b/scripts/compiler_output_harness/repsel_census.py @@ -278,6 +278,72 @@ } +#: Minimum number of unbound allocations each **allocation bucket** must be +#: seen in, per workload. **Held in code, never in the baseline**, for the same +#: reason as [`LIVENESS_FLOORS`], and gated separately. +#: +#: The key is the FULL bucket identity — `(analysis, allocation position, +#: denial rule)` — and not the position and the rule counted independently. +#: That was the first shape of this table and CodeRabbit was right to reject it +#: (#7176 review): with two separate maps, a report that filed a `return` under +#: the unserved rule *and* emitted the served rule on some other position +#: satisfied both floors while describing a compiler that had stopped +#: classifying anything correctly. The renderer already defines an allocation +#: bucket by all three (`opt_report::render::alloc_buckets`); the census now +#: floors the same tuple, so the two cannot describe different things. +#: +#: #7170 R0 split one alloc-site label into buckets that mean different things: +#: +#: * `constructor argument` — a genuine `new C(arg)` argument +#: * `object literal property value` — a property value of an anonymous-shape +#: literal, i.e. `{a: {b: 1}}` +#: * `return` — the allocation IS the returned value, +#: with the RULE saying whether the +#: enclosing function's return-shape fact +#: already serves it +#: * `returned expression operand` — nested inside a returned expression +#: (a conditional arm, a `&&` operand, an +#: awaited operand, a member base). Added +#: in review: these were being filed as +#: return positions, which over-counted +#: the `return` bucket published on #7170 +#: as R1's ceiling. +#: +#: Every one of those classifications can fail silently to **zero**, and a zero +#: bucket is indistinguishable from "the classifier is dead" — CLAUDE.md failure +#: mode 4, in the instrument that R0 exists to repair. `fixture_alloc_buckets` +#: is written to put at least one allocation in each, so a classification arm +#: that stops firing takes this check red instead of quietly re-merging the +#: buckets it separated. +#: +#: The served row is also the only end-to-end assertion on the WIRING: hard- +#: coding `false` in `codegen/function.rs`, or dropping the fact conjunct in +#: `deny_alloc_site`, takes it red while every compiler unit test still passes, +#: because those set the report scope by hand. +ALLOC_BUCKET_FLOORS: dict[str, dict[tuple[str, str, str], int]] = { + "fixture_alloc_buckets": { + ("ptr-shape", "constructor argument", "rule 1 (provenance)"): 1, + ("ptr-shape", "object literal property value", "rule 1 (provenance)"): 1, + ("ptr-shape", "returned expression operand", "rule 1 (provenance)"): 1, + ( + "ptr-shape", + "return", + "rule 1 (provenance) — already served by return-shape", + ): 1, + }, +} + + +def alloc_bucket_key(analysis: str, context: str, rule: str) -> str: + """The one spelling of an allocation bucket's identity. + + A string rather than a tuple because the observed map is serialized into the + baseline JSON as context. Built in exactly one place so the extractor and + the floors cannot key on different things. + """ + return f"{analysis} | {context} | {rule}" + + #: Workloads allowed to produce **zero candidates** — no analysis considered any #: value in them. Held in code for the same reason as [`LIVENESS_FLOORS`]: it is #: an assertion about the compiler, and `--update` must not be able to widen it. @@ -408,6 +474,12 @@ def census_from_report(report: dict[str, Any]) -> dict[str, Any]: consumed_receiver = 0 unconsumed_mechanisms: dict[str, int] = {} denial_rules: dict[str, int] = {} + #: #7170 R0. Unbound-allocation denials keyed by the FULL bucket identity — + #: analysis, allocation position and denial rule together, via + #: [`alloc_bucket_key`]. Not three independent tallies: two of the buckets + #: R0 separated share a rule and differ only by position, and two share a + #: position and differ only by rule, so only the tuple identifies a bucket. + alloc_buckets: dict[str, int] = {} # Keyed by analysis as well as by rule. `unconsumed_mechanisms` alone is # rule-keyed, and with a second instrumented analysis a gap in analysis A # would be excused by a mechanism recorded for analysis B. @@ -460,6 +532,14 @@ def census_from_report(report: dict[str, Any]) -> dict[str, Any]: denial_rules[str(entry.get("rule") or "")] = ( denial_rules.get(str(entry.get("rule") or ""), 0) + 1 ) + context = entry.get("alloc_context") + if context is not None: + key = alloc_bucket_key( + str(analysis), + str(context), + str(entry.get("rule") or ""), + ) + alloc_buckets[key] = alloc_buckets.get(key, 0) + 1 if unknown_sites: raise HarnessError( f"--opt-report recorded consumption at unregistered site(s) " @@ -494,6 +574,7 @@ def census_from_report(report: dict[str, Any]) -> dict[str, Any]: "candidates": candidates, "unconsumed_mechanisms": unconsumed_mechanisms, "denial_rules": denial_rules, + "alloc_buckets": alloc_buckets, "unconsumed_by_analysis": unconsumed_by_analysis, "consumed_receiver": consumed_receiver, "consumption_sites": consumption_sites, @@ -757,6 +838,47 @@ def check_refusal_floors(observed: dict[str, dict[str, Any]]) -> list[str]: return failures +def check_alloc_bucket_floors(observed: dict[str, dict[str, Any]]) -> list[str]: + """Every alloc-site position bucket must still be observed somewhere. + + Independent of the baseline file on purpose — see [`ALLOC_BUCKET_FLOORS`]. + + This is the third layer of #7104's pattern for the #7170 R0 classifications: + unit tests prove the classifier picks the right bucket, this proves the + classifier is still *wired into a real compile*, and the sabotage matrix + proves each arm can go red. Layer 2 is the one the unit tests cannot give: + `codegen/function.rs` could pass `false` for every function and every unit + test would still pass, because they set the scope by hand. + """ + failures: list[str] = [] + for name, minimums in ALLOC_BUCKET_FLOORS.items(): + if name not in observed: + failures.append( + f"alloc-bucket workload {name!r} did not run; the census cannot " + "claim to observe a bucket it never measured" + ) + continue + buckets = observed[name].get("alloc_buckets", {}) + for (analysis, context, rule), minimum in minimums.items(): + key = alloc_bucket_key(analysis, context, rule) + seen = int(buckets.get(key, 0)) + if seen < minimum: + failures.append( + f"{name}: allocation bucket {key!r} was reported {seen} " + f"time(s), and must be reported at least {minimum}. The " + "position and the rule are floored as ONE tuple on purpose: " + "either collectors/ptr_shape_report.rs stopped telling this " + "bucket apart from a neighbour (#7170 §5.1), or the " + "served-return classification stopped reaching a real " + "compile — codegen/function.rs no longer passing the " + "return-shape fact into the report scope, or deny_alloc_site " + "no longer consulting it. Every compiler unit test stays " + "green for that last one, because they set the scope by " + f"hand. Observed buckets: {sorted(buckets)}" + ) + return failures + + def check_instrument_liveness(observed: dict[str, dict[str, Any]]) -> list[str]: """No census key may read zero across the ENTIRE corpus. @@ -1086,6 +1208,7 @@ def census(args: argparse.Namespace) -> int: improvements += imp liveness = check_liveness_fixtures(observed) if not partial else [] refusals = check_refusal_floors(observed) if not partial else [] + alloc_buckets = check_alloc_bucket_floors(observed) if not partial else [] dead = check_instrument_liveness(observed) if not partial else [] unreached = check_analysis_reach(observed) if not partial else [] # Always checked, even for a --workload subset: it is an internal @@ -1111,6 +1234,7 @@ def census(args: argparse.Namespace) -> int: ("REGRESSION", regressions), ("DEAD INSTRUMENT", liveness + dead), ("REFUSAL NO LONGER FIRING", refusals), + ("ALLOC-SITE BUCKET NO LONGER DISTINGUISHED", alloc_buckets), ("UNREACHED BY EVERY ANALYSIS", unreached), ("CONSUMPTION COUNTER IS INCOHERENT", invariant), ("WASTED PROMOTION WITH NO NAMED MECHANISM", unexplained), @@ -1169,6 +1293,13 @@ def _update( # Context, never gated: which mechanism ate each wasted promotion. workload["unconsumed_mechanisms"] = observed[name].get("unconsumed_mechanisms", {}) workload["consumption_sites"] = observed[name].get("consumption_sites", {}) + # #7170 R0, also context and also never gated: where this workload's + # unbound allocations sit. Recorded per workload so the rule-1 wall can + # be re-derived from a checked-in artifact instead of from a `jq` line + # in an issue comment — which is how the 506/963 headline outlived the + # measurement it came from. The gated assertion is + # ALLOC_BUCKET_FLOORS, held in code. + workload["alloc_buckets"] = observed[name].get("alloc_buckets", {}) baseline["generated_at"] = utc_now() path.write_text(json.dumps(baseline, indent=2) + "\n", encoding="utf-8") print(f"Wrote {path.relative_to(REPO_ROOT)} ({len(workloads)} workload(s)).") @@ -1233,6 +1364,115 @@ def self_test(_args: argparse.Namespace) -> int: dead = check_instrument_liveness({"w": {"counts": counts}}) assert any("ptr-shape" in d for d in dead), dead + # #7170 R0: the alloc-site bucket extraction and its floors. Asserted in + # the FAILING direction first, for the reason stated at the top of this + # function — and once per defect, because the classifications can each die + # on their own. + bucket_rows = ( + ("constructor argument", "rule 1 (provenance)"), + ("object literal property value", "rule 1 (provenance)"), + ("returned expression operand", "rule 1 (provenance)"), + ("return", "rule 1 (provenance) — already served by return-shape"), + ) + bucket_report = { + "schema_version": 2, + "summary": { + "selected": 0, + "denied": len(bucket_rows), + "by_analysis": [ + { + "analysis": a, + "target_rep": a, + "rule_source": "x", + "selected": 0, + "denied": len(bucket_rows) if a == "ptr-shape" else 0, + "consumed": 0, + "unconsumed": 0, + } + for a in EXPECTED_ANALYSES + ], + }, + "entries": [ + { + "analysis": "ptr-shape", + "outcome": "denied", + "rep": "Boxed", + "position": "alloc-site", + "rule": rule_name, + "alloc_context": context, + } + for context, rule_name in bucket_rows + ], + } + buckets = census_from_report(bucket_report) + assert buckets["alloc_buckets"] == { + alloc_bucket_key("ptr-shape", c, r): 1 for c, r in bucket_rows + }, buckets["alloc_buckets"] + + fixture = next(iter(ALLOC_BUCKET_FLOORS)) + good = {fixture: {"counts": buckets["counts"], **buckets}} + assert not check_alloc_bucket_floors(good), check_alloc_bucket_floors(good) + + # Re-merging the two literal buckets — the #7170 §5.1 defect — is red. + merged = json.loads(json.dumps(good)) + merged[fixture]["alloc_buckets"] = { + alloc_bucket_key("ptr-shape", "constructor argument", "rule 1 (provenance)"): 2, + alloc_bucket_key( + "ptr-shape", + "returned expression operand", + "rule 1 (provenance)", + ): 1, + alloc_bucket_key( + "ptr-shape", + "return", + "rule 1 (provenance) — already served by return-shape", + ): 1, + } + failures = check_alloc_bucket_floors(merged) + assert any("object literal property value" in f for f in failures), failures + + # Filing a returned-expression OPERAND as a return position — the review + # defect this bucket was split to catch — is red. + operand_as_return = json.loads(json.dumps(good)) + operand_as_return[fixture]["alloc_buckets"] = { + alloc_bucket_key("ptr-shape", c, r): 1 + for c, r in bucket_rows + if c != "returned expression operand" + } + operand_as_return[fixture]["alloc_buckets"][ + alloc_bucket_key("ptr-shape", "return", "rule 1 (provenance)") + ] = 1 + failures = check_alloc_bucket_floors(operand_as_return) + assert any("returned expression operand" in f for f in failures), failures + + # ★ The hole CodeRabbit found in the first shape of this table: the served + # RULE present, but on the wrong POSITION. With the rule and the context + # floored independently this passed; keyed as one tuple it cannot. + swapped = json.loads(json.dumps(good)) + swapped[fixture]["alloc_buckets"] = { + alloc_bucket_key("ptr-shape", c, r): 1 + for c, r in bucket_rows + if c != "return" + } + swapped[fixture]["alloc_buckets"][ + alloc_bucket_key( + "ptr-shape", + "call argument", + "rule 1 (provenance) — already served by return-shape", + ) + ] = 1 + swapped[fixture]["denial_rules"] = { + "rule 1 (provenance) — already served by return-shape": 1 + } + failures = check_alloc_bucket_floors(swapped) + assert any( + "already served by return-shape" in f and "return" in f for f in failures + ), failures + + # And a workload that did not run at all cannot pass by being absent. + failures = check_alloc_bucket_floors({}) + assert failures, "a missing alloc-bucket fixture must be a failure" + # #7128: the refusal check must go red when the rule stops firing, and # green only when it fires at least as often as it was measured to. target = next(iter(REFUSAL_FLOORS))