From e61e6ffd308bfc02e4c209ea629e10414f50d1f4 Mon Sep 17 00:00:00 2001 From: Ralph Kuepper Date: Thu, 13 Aug 2026 04:38:58 +0200 Subject: [PATCH 1/2] perf(repsel): admit conditional return-shape producers (#7170) --- .../collectors/ptr_shape_opt_report_tests.rs | 64 +++++++--- .../src/collectors/ptr_shape_report.rs | 88 ++++++++++---- .../src/collectors/ptr_shape_returns.rs | 75 ++++++++++-- .../src/collectors/ptr_shape_returns_tests.rs | 109 ++++++++++++++++++ 4 files changed, 283 insertions(+), 53 deletions(-) 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 8cc82b567f..86a558a62e 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 @@ -744,19 +744,15 @@ fn a_region_lowered_twice_still_collapses_and_says_how_much() { // 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-shape fact originally covered only the function's direct RETURN +// VALUE. #7170 R2 additionally consumes fresh, agreeing conditional result +// arms. It still says nothing about a conditional's condition, a `&&`, an +// `await` or a member base that happens to sit inside the returned expression. // -// * 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. +// R0 separated the syntactic bucket from the servedness bit precisely so R2 +// could make that distinction. These tests force the producer flag on so the +// classifier is tested on its own terms rather than passing only because a +// different producer-side guard happens to reject the region. fn ternary(a: Expr, b: Expr) -> Expr { Expr::Conditional { @@ -783,10 +779,10 @@ 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. +/// `return cond ? new C() : new C()` — the arms retain their honest syntactic +/// bucket, but R2 now consumes both allocations to issue the producer fact. #[test] -fn a_conditional_arm_under_a_return_is_not_a_return_position() { +fn a_conditional_result_arm_is_reported_as_served_by_return_shape() { let c = c_classes(); let mut classes = HashMap::new(); classes.insert("C".to_string(), &c); @@ -801,14 +797,48 @@ fn a_conditional_arm_under_a_return_is_not_a_return_position() { Some("return"), "a conditional arm is not the returned value" ); - assert_ne!( + assert_eq!( e.tier, Some(crate::opt_report::Tier::Served), - "the return-shape fact does not cover a conditional arm" + "R2 consumes each fresh conditional result arm" ); } } +/// The condition is evaluated but is not one of the values returned. Keep its +/// allocation unserved while both result arms are served; otherwise the R2 +/// walker has merely reintroduced R0's "every nested operand is a return" +/// defect under a new boolean. +#[test] +fn a_conditional_condition_is_not_a_return_shape_source() { + let c = c_classes(); + let mut classes = HashMap::new(); + classes.insert("C".to_string(), &c); + let stmts = vec![Stmt::Return(Some(Expr::Conditional { + condition: Box::new(new_c()), + then_expr: Box::new(new_c()), + else_expr: Box::new(new_c()), + }))]; + + let entries = run_as_producer(&stmts, &classes); + let rows = alloc_rows(&entries); + assert_eq!(rows.len(), 3); + assert_eq!( + rows.iter() + .filter(|e| e.tier == Some(crate::opt_report::Tier::Served)) + .count(), + 2, + "only the two result arms feed the return-shape fact" + ); + assert_eq!( + rows.iter() + .filter(|e| e.tier != Some(crate::opt_report::Tier::Served)) + .count(), + 1, + "the conditional's allocation-valued condition stays unserved" + ); +} + /// `return flag && new C()` — a binary operand. #[test] fn a_logical_operand_under_a_return_is_not_a_return_position() { diff --git a/crates/perry-codegen/src/collectors/ptr_shape_report.rs b/crates/perry-codegen/src/collectors/ptr_shape_report.rs index 2e93bf4ff2..78cab85302 100644 --- a/crates/perry-codegen/src/collectors/ptr_shape_report.rs +++ b/crates/perry-codegen/src/collectors/ptr_shape_report.rs @@ -428,8 +428,8 @@ pub(super) fn deny_local( /// /// #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 +/// 1. The denial is [`UNBOUND_ALLOC_SERVED_RETURN`] when the site is a proven +/// source 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 @@ -440,7 +440,7 @@ pub(super) fn deny_alloc_site(site: &NewSite) { if !opt_report::enabled() || suppressed() { return; } - let served = site.is_return_position && opt_report::region_is_return_shape_producer(); + let served = site.is_return_shape_source && opt_report::region_is_return_shape_producer(); let d = if served { UNBOUND_ALLOC_SERVED_RETURN } else { @@ -543,22 +543,25 @@ fn walk_lets(stmts: &[Stmt], depth: u32, f: &mut impl FnMut(u32, &str, u32)) { // 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 +// [`NewSite::is_return_shape_source`], 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. +/// access base. #7170 R2 consumes conditional *result* arms as return-shape +/// sources, but they remain in this syntactic bucket rather than being +/// mislabeled as direct returns. /// /// 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. +/// `Tier::Served` to unrelated operands when the producer side widened. The +/// separate `is_return_shape_source` bit now distinguishes R2's result arms +/// from conditions and every other operand in this same bucket. 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"; @@ -597,15 +600,16 @@ pub(super) struct NewSite { /// 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. + /// This allocation is one of the fresh values a return-shape fact proves: + /// either the direct expression of a `Stmt::Return`, or a result arm of a + /// returned conditional (#7170 R2). An allocation in the condition, a + /// constructor argument, or another nested operand is not a source. /// - /// 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, + /// Set only by [`scan_return`] and its result-arm walker. Deriving + /// servedness from the context label instead would be wrong: conditional + /// arms remain `returned expression operand` positions even when their + /// allocations are inputs to the producer fact. + pub is_return_shape_source: 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 @@ -742,14 +746,12 @@ 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. +/// The direct expression of a `return` is the function's return value. #7170 +/// R2 additionally proves the result arms of a conditional when every leaf is +/// a fresh allocation of one class. The condition and every non-result nested +/// expression remain ordinary operands. /// -/// 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. +/// This is the only entry into the served-source classification. fn scan_return(e: &Expr, depth: u32, out: &mut Vec) { match e { Expr::New { @@ -764,6 +766,44 @@ fn scan_return(e: &Expr, depth: u32, out: &mut Vec) { scan_expr(a, depth, arg_ctx, out); } } + Expr::Conditional { + condition, + then_expr, + else_expr, + } => { + scan_expr(condition, depth, RETURNED_OPERAND, out); + scan_conditional_result(then_expr, depth, out); + scan_conditional_result(else_expr, depth, out); + } + _ => scan_expr(e, depth, RETURNED_OPERAND, out), + } +} + +/// Scan one result arm of a returned conditional. Nested conditionals keep +/// their result leaves in the source set, but their conditions do not. +fn scan_conditional_result(e: &Expr, depth: u32, out: &mut Vec) { + match e { + Expr::New { + class_name, + args, + byte_offset, + .. + } => { + push_new_site(out, class_name, RETURNED_OPERAND, depth, *byte_offset, true); + let arg_ctx = arg_context(class_name); + for a in args { + scan_expr(a, depth, arg_ctx, out); + } + } + Expr::Conditional { + condition, + then_expr, + else_expr, + } => { + scan_expr(condition, depth, RETURNED_OPERAND, out); + scan_conditional_result(then_expr, depth, out); + scan_conditional_result(else_expr, depth, out); + } _ => scan_expr(e, depth, RETURNED_OPERAND, out), } } @@ -774,7 +814,7 @@ fn push_new_site( context: &'static str, depth: u32, byte_offset: u32, - is_return_position: bool, + is_return_shape_source: bool, ) { out.push(NewSite { display: display_class(class_name), @@ -782,7 +822,7 @@ fn push_new_site( loop_depth: depth, ordinal: out.len() as u32, byte_offset, - is_return_position, + is_return_shape_source, }); } diff --git a/crates/perry-codegen/src/collectors/ptr_shape_returns.rs b/crates/perry-codegen/src/collectors/ptr_shape_returns.rs index 768ce13b82..b2906b8cee 100644 --- a/crates/perry-codegen/src/collectors/ptr_shape_returns.rs +++ b/crates/perry-codegen/src/collectors/ptr_shape_returns.rs @@ -50,9 +50,15 @@ //! anywhere but the returned one". It is discharged by re-running //! `collect_shape_proven_ptr_locals` over the producer's body rather than //! by a second, weaker approximation of it. +//! * A conditional expression whose result leaves are fresh `New` allocations +//! and all agree on the same class. The condition itself cannot weaken +//! freshness because a branch-local allocation does not exist until after +//! the condition has run. A `LocalGet` leaf remains fail-closed for now: +//! Phase 3b exempts only a bare `return local`, not one nested in an +//! expression. //! //! Anything else — `return CACHE`, `return this.field`, `return mk()`, -//! `return cond ? a : b` — yields no fact. +//! or a conditional with a non-fresh/disagreeing arm — yields no fact. //! //! ## Why the producer must not fall off its end //! @@ -323,19 +329,20 @@ fn producer_return_class( } // Every return must agree on one class, and each must be a fresh form. + // #7170 R2 treats a conditional as the set of values it can actually + // return, recursively. This is deliberately narrower than a generic + // expression walk: the condition is not a result, and logical operators + // can return their left operand, whose truthiness/type needs a separate + // proof. + let mut sources = Vec::new(); + for r in &returns { + if !collect_fresh_return_sources(r, f.body, true, &mut sources) { + return None; + } + } let mut class_name: Option<&str> = None; let mut needs_body_proof: Vec = Vec::new(); - for r in &returns { - let (name, local) = match r { - Expr::New { class_name: c, .. } => (c.as_str(), None), - Expr::LocalGet(id) => { - // Resolved against the producer's own Phase 3b proof below; - // find its declared class first so disagreement short-circuits. - let c = seeded_class_of_local(f.body, *id)?; - (c, Some(*id)) - } - _ => return None, - }; + for (name, local) in sources { match class_name { None => class_name = Some(name), Some(prev) if prev == name => {} @@ -405,6 +412,50 @@ fn producer_return_class( Some(class_name.to_string()) } +/// Flatten one returned expression into the fresh values it may produce. +/// +/// A conditional is safe exactly when both arms are safe: only one arm runs, +/// but the caller may observe either one. Nested conditionals recurse so the +/// proof is about the complete result set rather than one syntactic layer. +/// Each leaf keeps the existing freshness obligation: +/// +/// * `New` is fresh by construction; +/// * `LocalGet` is accepted only when it is the direct return expression, then +/// discharged by the producer's full Phase 3b containment proof in +/// `producer_return_class`. Phase 3b does not currently exempt a local nested +/// inside a returned expression, so conditional arms stay `New`-only. +/// +/// `false` is fail-closed for every other expression form. +fn collect_fresh_return_sources<'a>( + expr: &'a Expr, + body: &'a [Stmt], + is_direct_return: bool, + out: &mut Vec<(&'a str, Option)>, +) -> bool { + match expr { + Expr::New { class_name, .. } => { + out.push((class_name.as_str(), None)); + true + } + Expr::LocalGet(id) if is_direct_return => { + let Some(class_name) = seeded_class_of_local(body, *id) else { + return false; + }; + out.push((class_name, Some(*id))); + true + } + Expr::Conditional { + then_expr, + else_expr, + .. + } => { + collect_fresh_return_sources(then_expr, body, false, out) + && collect_fresh_return_sources(else_expr, body, false, out) + } + _ => false, + } +} + /// The class of the `new` that a `Stmt::Let` in `stmts` binds to `want`. /// `None` when the id is not bound by exactly one `Let { init: New }` here. fn seeded_class_of_local(stmts: &[Stmt], want: u32) -> Option<&str> { diff --git a/crates/perry-codegen/src/collectors/ptr_shape_returns_tests.rs b/crates/perry-codegen/src/collectors/ptr_shape_returns_tests.rs index aed2300ff4..2da27fc89f 100644 --- a/crates/perry-codegen/src/collectors/ptr_shape_returns_tests.rs +++ b/crates/perry-codegen/src/collectors/ptr_shape_returns_tests.rs @@ -293,6 +293,115 @@ fn direct_new_return_is_a_fact() { assert_eq!(facts.return_shape_class(3), Some("C")); } +/// #7170 R2: both arms of a returned conditional are the complete set of +/// values the caller can observe. When every leaf is fresh and agrees on the +/// class, the producer carries the same fact as a direct `return new C()` and +/// its caller is seeded. +/// +/// The nested arm is deliberate: accepting only one syntactic conditional +/// layer would leave the same proof structurally unreachable after transforms +/// introduce another conditional inside a branch. +/// +/// Sabotage: remove the `Expr::Conditional` arm from +/// `collect_fresh_return_sources` and this fails at the producer assertion. +#[test] +fn agreeing_conditional_return_is_a_fact_and_seeds_its_caller() { + let conditional = Expr::Conditional { + condition: Box::new(Expr::Bool(true)), + then_expr: Box::new(new_c()), + else_expr: Box::new(Expr::Conditional { + condition: Box::new(Expr::Bool(false)), + then_expr: Box::new(new_c()), + else_expr: Box::new(new_c()), + }), + }; + let (facts, c) = facts_for(vec![function( + 30, + "conditional", + vec![Stmt::Return(Some(conditional))], + )]); + assert_eq!( + facts.return_shape_class(30), + Some("C"), + "all conditional result arms are fresh instances of C" + ); + + let classes = classes_of(&c); + let caller = call_and_store(31, Expr::FuncRef(30)); + assert!( + promote(&caller, &classes, &facts).contains_key(&31), + "the conditional-return fact must reach the caller-side seed" + ); +} + +/// A conditional fact is all-arms, not "one object-looking arm is enough". +/// A cached/unknown value on either side may be aliased or may not be an +/// object at all. +/// +/// Sabotage: accept only `then_expr` in `collect_fresh_return_sources` and the +/// first half fails; accept only `else_expr` and the second half fails. +#[test] +fn conditional_return_with_any_non_fresh_arm_gets_no_fact() { + for (fresh_first, then_expr, else_expr) in [ + (true, new_c(), Expr::LocalGet(999)), + (false, Expr::LocalGet(999), new_c()), + ] { + let returned = Expr::Conditional { + condition: Box::new(Expr::Bool(true)), + then_expr: Box::new(then_expr), + else_expr: Box::new(else_expr), + }; + let (facts, _) = facts_for(vec![function( + 31, + "maybe_cached", + vec![Stmt::Return(Some(returned))], + )]); + assert_eq!( + facts.return_shape_class(31), + None, + "the {} conditional arm being fresh cannot license the other arm", + if fresh_first { "first" } else { "second" } + ); + } +} + +/// Freshness is not enough when the arms have different shapes: the caller +/// would use one fixed field-offset table for two dynamic classes. +/// +/// Sabotage: remove the existing class-agreement check after flattening the +/// conditional and this fails. The agreeing control keeps the test live. +#[test] +fn disagreeing_conditional_return_classes_get_no_fact() { + let new_d = Expr::New { + class_name: "D".to_string(), + args: Vec::new(), + type_args: Vec::new(), + byte_offset: 0, + cap_args_appended: 0, + }; + let returned = |else_expr| Expr::Conditional { + condition: Box::new(Expr::Bool(true)), + then_expr: Box::new(new_c()), + else_expr: Box::new(else_expr), + }; + let (facts, _) = facts_for_classes( + vec![class_d()], + vec![function( + 32, + "different", + vec![Stmt::Return(Some(returned(new_d)))], + )], + ); + assert_eq!(facts.return_shape_class(32), None); + + let (control, _) = facts_for(vec![function( + 33, + "same", + vec![Stmt::Return(Some(returned(new_c())))], + )]); + assert_eq!(control.return_shape_class(33), Some("C")); +} + /// A producer that can fall off the end returns `undefined` on that path; a /// caller treating the result as a proven `C` would load a field off it. /// From 72c5c98cb96a4137fc244c04bb1bb2d7e143b49c Mon Sep 17 00:00:00 2001 From: Ralph Kuepper Date: Thu, 13 Aug 2026 04:40:14 +0200 Subject: [PATCH 2/2] docs(changelog): record conditional return shapes --- changelog.d/8007-conditional-return-shapes.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 changelog.d/8007-conditional-return-shapes.md diff --git a/changelog.d/8007-conditional-return-shapes.md b/changelog.d/8007-conditional-return-shapes.md new file mode 100644 index 0000000000..f929c7fb99 --- /dev/null +++ b/changelog.d/8007-conditional-return-shapes.md @@ -0,0 +1,12 @@ +### Representation selection: prove fresh conditional return shapes (#7170 R2) + +Functions and CJS-wrapped closure producers that return conditional expressions +can now issue a `Ptr` return fact when every recursively reachable result +arm is a fresh allocation of the same admissible class. Caller bindings then +reuse the existing guard-free fixed-offset field-access path, with the existing +class-admission, containment, module-barrier, and GC-rooting proofs unchanged. + +Non-fresh or disagreeing arms remain fail-closed, as do logical expressions and +locals nested inside conditional arms. `--opt-report` keeps these allocations in +the honest `returned expression operand` syntax bucket while marking only the +allocations consumed by an issued return-shape fact as served.