perf(repsel): admit conditional return-shape producers (#7170 R2)

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<Shape>` 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.

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() {

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<NewSite>) {
     match e {
         Expr::New {
@@ -764,6 +766,44 @@ fn scan_return(e: &Expr, depth: u32, out: &mut Vec<NewSite>) {
                 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<NewSite>) {
+    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,15 +814,15 @@ 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),
         context,
         loop_depth: depth,
         ordinal: out.len() as u32,
         byte_offset,
-        is_return_position,
+        is_return_shape_source,
     });
 }
 

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<u32> = 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<u32>)>,
+) -> 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> {