diff --git a/changelog.d/8015-logical-return-shapes.md b/changelog.d/8015-logical-return-shapes.md new file mode 100644 index 0000000000..4397344250 --- /dev/null +++ b/changelog.d/8015-logical-return-shapes.md @@ -0,0 +1,13 @@ +### Representation selection: prove fresh logical return shapes (#7170 R2) + +Functions and CJS-wrapped closure producers that return `&&` / `||` +expressions can now issue a `Ptr` return fact when every value that can +escape the complete short-circuit expression is a fresh allocation of the same +admissible class. This includes nested fallback forms such as +`(flag && new C()) || new C()`; caller bindings reuse the existing guard-free +fixed-offset field-access path with no new pointer position or ABI change. + +Primitive or unknown escape paths, disagreeing reachable classes, and nullish +coalescing remain fail-closed. `--opt-report` marks only allocations that can +actually become the logical result as served, leaving consumed short-circuit +operands out of that population. 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 86a558a62e..9ef3d2d055 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 @@ -745,9 +745,10 @@ fn a_region_lowered_twice_still_collapses_and_says_how_much() { // producer, `Tier::Served` with it. // // 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. +// VALUE. #7170 R2 additionally consumes fresh, agreeing conditional and +// logical result allocations. It still says nothing about a conditional's +// condition, a consumed short-circuit operand, an `await` or a member base +// that happens to sit inside the returned expression. // // 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 @@ -839,9 +840,12 @@ fn a_conditional_condition_is_not_a_return_shape_source() { ); } -/// `return flag && new C()` — a binary operand. +/// `return true && new C()` — the right operand is the only value this +/// expression can return, so the logical-return proof consumes it. It keeps +/// the honest operand-position label rather than masquerading as a bare +/// return. #[test] -fn a_logical_operand_under_a_return_is_not_a_return_position() { +fn a_logical_result_operand_is_reported_as_served_by_return_shape() { let c = c_classes(); let mut classes = HashMap::new(); classes.insert("C".to_string(), &c); @@ -855,7 +859,48 @@ fn a_logical_operand_under_a_return_is_not_a_return_position() { 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)); + assert_eq!(rows[0].tier, Some(crate::opt_report::Tier::Served)); +} + +/// A fresh left operand is always truthy. In `new C() || new D()` it is the +/// returned value and the right allocation is consumed by short-circuiting. +/// Marking both served would put a structurally unreachable site back into the +/// population R0 corrected. +#[test] +fn a_consumed_logical_operand_is_not_a_return_shape_source() { + let c = c_classes(); + let d = class_with_fields("D", &["x"]); + let mut classes = HashMap::new(); + classes.insert("C".to_string(), &c); + classes.insert("D".to_string(), &d); + 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 stmts = vec![Stmt::Return(Some(Expr::Logical { + op: perry_hir::LogicalOp::Or, + left: Box::new(new_c()), + right: Box::new(new_d), + }))]; + + let entries = run_as_producer(&stmts, &classes); + let rows = alloc_rows(&entries); + assert_eq!(rows.len(), 2); + assert_eq!( + rows.iter() + .filter(|e| e.tier == Some(crate::opt_report::Tier::Served)) + .count(), + 1, + "only the allocation that can become the logical result is served" + ); + let served = rows + .iter() + .find(|e| e.tier == Some(crate::opt_report::Tier::Served)) + .expect("one served allocation"); + assert!(served.name.contains("C")); } /// `return await new C()` — the awaited operand is not the returned value diff --git a/crates/perry-codegen/src/collectors/ptr_shape_report.rs b/crates/perry-codegen/src/collectors/ptr_shape_report.rs index 78cab85302..3e3517d369 100644 --- a/crates/perry-codegen/src/collectors/ptr_shape_report.rs +++ b/crates/perry-codegen/src/collectors/ptr_shape_report.rs @@ -549,10 +549,10 @@ fn walk_lets(stmts: &[Stmt], depth: u32, f: &mut impl FnMut(u32, &str, u32)) { /// 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. #7170 R2 consumes conditional *result* arms as return-shape -/// sources, but they remain in this syntactic bucket rather than being -/// mislabeled as direct returns. +/// value — a conditional arm, a logical operand, an awaited operand, a member +/// access base. #7170 R2 consumes conditional and logical *result* allocations +/// 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 @@ -601,9 +601,10 @@ pub(super) struct NewSite { /// see [`crate::opt_report::Entry::alloc_ordinal`]. pub ordinal: u32, /// 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. + /// either the direct expression of a `Stmt::Return`, or a possible result + /// allocation of a returned conditional / logical expression (#7170 R2). + /// An allocation in a condition, a constructor argument, or a consumed + /// short-circuit operand is not a source. /// /// Set only by [`scan_return`] and its result-arm walker. Deriving /// servedness from the context label instead would be wrong: conditional @@ -747,41 +748,26 @@ fn scan_stmts( /// Scan the expression of a `Stmt::Return`. /// /// 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. +/// R2 additionally proves possible result allocations of conditional and +/// short-circuiting logical expressions. Conditions and every non-result +/// nested expression remain ordinary operands. /// /// This is the only entry into the served-source classification. 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); - } - } - 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), - } + let sources = super::ptr_shape_returns::possible_return_shape_new_sources(e); + scan_return_result(e, depth, true, &sources, 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) { +/// Scan a returned expression while preserving which nested allocations can +/// actually become its result. `direct` controls only the position label; the +/// source bit comes from the producer proof's own outcome analysis. +fn scan_return_result( + e: &Expr, + depth: u32, + direct: bool, + sources: &[&Expr], + out: &mut Vec, +) { match e { Expr::New { class_name, @@ -789,7 +775,15 @@ fn scan_conditional_result(e: &Expr, depth: u32, out: &mut Vec) { byte_offset, .. } => { - push_new_site(out, class_name, RETURNED_OPERAND, depth, *byte_offset, true); + let is_source = sources.iter().any(|source| std::ptr::eq(*source, e)); + push_new_site( + out, + class_name, + if direct { RETURN } else { RETURNED_OPERAND }, + depth, + *byte_offset, + is_source, + ); let arg_ctx = arg_context(class_name); for a in args { scan_expr(a, depth, arg_ctx, out); @@ -801,8 +795,12 @@ fn scan_conditional_result(e: &Expr, depth: u32, out: &mut Vec) { else_expr, } => { scan_expr(condition, depth, RETURNED_OPERAND, out); - scan_conditional_result(then_expr, depth, out); - scan_conditional_result(else_expr, depth, out); + scan_return_result(then_expr, depth, false, sources, out); + scan_return_result(else_expr, depth, false, sources, out); + } + Expr::Logical { left, right, .. } => { + scan_return_result(left, depth, false, sources, out); + scan_return_result(right, depth, false, sources, out); } _ => scan_expr(e, depth, RETURNED_OPERAND, out), } diff --git a/crates/perry-codegen/src/collectors/ptr_shape_returns.rs b/crates/perry-codegen/src/collectors/ptr_shape_returns.rs index b2906b8cee..8e4e72ad82 100644 --- a/crates/perry-codegen/src/collectors/ptr_shape_returns.rs +++ b/crates/perry-codegen/src/collectors/ptr_shape_returns.rs @@ -56,9 +56,15 @@ //! 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. +//! * A short-circuiting `&&` / `||` expression when every value that can +//! actually escape the expression is fresh. The proof tracks truthy and +//! falsy outcomes separately: `flag && new C()` is refused because `flag` +//! can escape on the falsy path, while `(flag && new C()) || new C()` is +//! admitted because the outer fallback replaces that path with a fresh `C`. //! //! Anything else — `return CACHE`, `return this.field`, `return mk()`, -//! or a conditional with a non-fresh/disagreeing arm — yields no fact. +//! `return flag && new C()`, or an expression with a non-fresh/disagreeing +//! result — yields no fact. //! //! ## Why the producer must not fall off its end //! @@ -94,7 +100,7 @@ use std::collections::{HashMap, HashSet}; use perry_hir::types::Type; -use perry_hir::{Class, Expr, Module, Stmt}; +use perry_hir::{Class, Expr, LogicalOp, Module, Stmt}; use super::ptr_shape::{chain_admissible, ptr_shape_locals_enabled}; use super::ptr_shape_report as report; @@ -328,21 +334,24 @@ fn producer_return_class( return None; } - // 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. + // Every return must agree on one class, and each possible result must be + // fresh. #7170 R2 models conditionals and short-circuiting logical + // expressions as the set of values they can actually return, recursively. let mut sources = Vec::new(); for r in &returns { - if !collect_fresh_return_sources(r, f.body, true, &mut sources) { + let outcomes = collect_return_outcomes(r, f.body, true); + if !outcomes.is_all_fresh() { return None; } + sources.extend(outcomes.sources); } let mut class_name: Option<&str> = None; let mut needs_body_proof: Vec = Vec::new(); - for (name, local) in sources { + for source in sources { + let (name, local) = match source { + FreshReturnSource::New { class_name, .. } => (class_name, None), + FreshReturnSource::Local { id, class_name } => (class_name, Some(id)), + }; match class_name { None => class_name = Some(name), Some(prev) if prev == name => {} @@ -412,12 +421,81 @@ fn producer_return_class( Some(class_name.to_string()) } -/// Flatten one returned expression into the fresh values it may produce. +#[derive(Clone, Copy)] +enum FreshReturnSource<'a> { + New { expr: &'a Expr, class_name: &'a str }, + Local { id: u32, class_name: &'a str }, +} + +/// Abstract result set of one returned expression. +/// +/// Fresh allocations are always truthy. Non-fresh results are split by +/// truthiness because a surrounding logical expression may consume one half +/// rather than return it: `||` consumes falsy left results, while `&&` +/// consumes truthy left results. +struct ReturnOutcomes<'a> { + sources: Vec>, + non_fresh_truthy: bool, + non_fresh_falsy: bool, +} + +impl<'a> ReturnOutcomes<'a> { + fn fresh(source: FreshReturnSource<'a>) -> Self { + Self { + sources: vec![source], + non_fresh_truthy: false, + non_fresh_falsy: false, + } + } + + fn non_fresh(truthy: bool) -> Self { + Self { + sources: Vec::new(), + non_fresh_truthy: truthy, + non_fresh_falsy: !truthy, + } + } + + fn unknown() -> Self { + Self { + sources: Vec::new(), + non_fresh_truthy: true, + non_fresh_falsy: true, + } + } + + fn may_be_truthy(&self) -> bool { + !self.sources.is_empty() || self.non_fresh_truthy + } + + fn may_be_falsy(&self) -> bool { + self.non_fresh_falsy + } + + fn is_all_fresh(&self) -> bool { + !self.sources.is_empty() && !self.non_fresh_truthy && !self.non_fresh_falsy + } + + fn merge(mut self, other: Self) -> Self { + self.sources.extend(other.sources); + self.non_fresh_truthy |= other.non_fresh_truthy; + self.non_fresh_falsy |= other.non_fresh_falsy; + self + } +} + +/// Flatten one returned expression into the 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: +/// Logical operators preserve their JavaScript operand-value semantics: +/// +/// * `left && right` returns a falsy `left`, otherwise `right`; +/// * `left || right` returns a truthy `left`, otherwise `right`. +/// +/// This makes the proof path-sensitive without trusting erased TypeScript +/// types. Each fresh leaf keeps the existing freshness obligation: /// /// * `New` is fresh by construction; /// * `LocalGet` is accepted only when it is the direct return expression, then @@ -425,37 +503,95 @@ fn producer_return_class( /// `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>( +/// Every unsupported expression is conservatively both truthy and falsy. `??` +/// remains unsupported because it needs a separate nullish outcome partition. +fn collect_return_outcomes<'a>( expr: &'a Expr, body: &'a [Stmt], is_direct_return: bool, - out: &mut Vec<(&'a str, Option)>, -) -> bool { +) -> ReturnOutcomes<'a> { match expr { - Expr::New { class_name, .. } => { - out.push((class_name.as_str(), None)); - true - } + Expr::New { class_name, .. } => ReturnOutcomes::fresh(FreshReturnSource::New { + expr, + class_name: class_name.as_str(), + }), Expr::LocalGet(id) if is_direct_return => { let Some(class_name) = seeded_class_of_local(body, *id) else { - return false; + return ReturnOutcomes::unknown(); }; - out.push((class_name, Some(*id))); - true + ReturnOutcomes::fresh(FreshReturnSource::Local { + id: *id, + class_name, + }) } Expr::Conditional { then_expr, else_expr, .. - } => { - collect_fresh_return_sources(then_expr, body, false, out) - && collect_fresh_return_sources(else_expr, body, false, out) + } => collect_return_outcomes(then_expr, body, false) + .merge(collect_return_outcomes(else_expr, body, false)), + Expr::Logical { op, left, right } => { + let left = collect_return_outcomes(left, body, false); + match op { + LogicalOp::And => { + let left_may_be_truthy = left.may_be_truthy(); + if !left_may_be_truthy { + return left; + } + let right = collect_return_outcomes(right, body, false); + ReturnOutcomes { + // A truthy left operand is consumed by `&&`; only the + // right operand can supply a fresh result. + sources: right.sources, + non_fresh_truthy: right.non_fresh_truthy, + non_fresh_falsy: left.non_fresh_falsy || right.non_fresh_falsy, + } + } + LogicalOp::Or => { + let left_may_be_falsy = left.may_be_falsy(); + if !left_may_be_falsy { + return left; + } + let right = collect_return_outcomes(right, body, false); + let mut sources = left.sources; + sources.extend(right.sources); + ReturnOutcomes { + // A falsy left operand is consumed by `||`; a truthy + // fresh left allocation remains a possible result. + sources, + non_fresh_truthy: left.non_fresh_truthy || right.non_fresh_truthy, + non_fresh_falsy: right.non_fresh_falsy, + } + } + LogicalOp::Coalesce => ReturnOutcomes::unknown(), + } } - _ => false, + Expr::Undefined | Expr::Null => ReturnOutcomes::non_fresh(false), + Expr::Bool(value) => ReturnOutcomes::non_fresh(*value), + Expr::Integer(value) => ReturnOutcomes::non_fresh(*value != 0), + Expr::Number(value) => ReturnOutcomes::non_fresh(*value != 0.0 && !value.is_nan()), + Expr::String(value) => ReturnOutcomes::non_fresh(!value.is_empty()), + _ => ReturnOutcomes::unknown(), } } +/// Fresh allocation nodes that may be the final value of this expression. +/// +/// The optimization report uses this structural view only after the enclosing +/// region is known to carry a return-shape fact. Keeping the source selection +/// here prevents its logical-expression accounting from drifting away from +/// the producer proof above. +pub(super) fn possible_return_shape_new_sources(expr: &Expr) -> Vec<&Expr> { + collect_return_outcomes(expr, &[], true) + .sources + .into_iter() + .filter_map(|source| match source { + FreshReturnSource::New { expr, .. } => Some(expr), + FreshReturnSource::Local { .. } => None, + }) + .collect() +} + /// 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 2da27fc89f..de609b31a7 100644 --- a/crates/perry-codegen/src/collectors/ptr_shape_returns_tests.rs +++ b/crates/perry-codegen/src/collectors/ptr_shape_returns_tests.rs @@ -402,6 +402,151 @@ fn disagreeing_conditional_return_classes_get_no_fact() { assert_eq!(control.return_shape_class(33), Some("C")); } +/// #7170 R2: `&&` and `||` return operand values, not booleans. A statically +/// truthy left side of `&&` and a statically falsy left side of `||` both make +/// the fresh right allocation the only observable result. +/// +/// Sabotage: remove the `Expr::Logical` arm from +/// `collect_return_outcomes` and both producer assertions fail. +#[test] +fn deterministic_logical_return_is_a_fact_and_seeds_its_caller() { + let logical = |op, left| Expr::Logical { + op, + left: Box::new(left), + right: Box::new(new_c()), + }; + let (facts, c) = facts_for(vec![ + function( + 34, + "and_fresh", + vec![Stmt::Return(Some(logical( + perry_hir::LogicalOp::And, + Expr::Bool(true), + )))], + ), + function( + 35, + "or_fresh", + vec![Stmt::Return(Some(logical( + perry_hir::LogicalOp::Or, + Expr::Bool(false), + )))], + ), + ]); + assert_eq!(facts.return_shape_class(34), Some("C")); + assert_eq!(facts.return_shape_class(35), Some("C")); + + let classes = classes_of(&c); + let caller = call_and_store(36, Expr::FuncRef(34)); + assert!( + promote(&caller, &classes, &facts).contains_key(&36), + "a logical-return fact must reach the caller-side seed" + ); +} + +/// A logical expression may filter a non-object intermediate without letting +/// it escape. `(flag && new C()) || new C()` returns the inner allocation when +/// `flag` is truthy and the fallback allocation otherwise, so every complete +/// path is fresh even though neither `flag && new C()` nor `flag || new C()` +/// is independently a return-shape producer. +/// +/// Sabotage: flatten logical operands like conditional arms instead of +/// tracking truthiness and the positive assertion fails. +#[test] +fn nested_logical_fallback_returns_only_fresh_objects() { + let flag = Expr::LocalGet(999); + let inner = Expr::Logical { + op: perry_hir::LogicalOp::And, + left: Box::new(flag.clone()), + right: Box::new(new_c()), + }; + let with_fallback = Expr::Logical { + op: perry_hir::LogicalOp::Or, + left: Box::new(inner), + right: Box::new(new_c()), + }; + let (facts, _) = facts_for(vec![ + function(36, "with_fallback", vec![Stmt::Return(Some(with_fallback))]), + function( + 37, + "and_without_fallback", + vec![Stmt::Return(Some(Expr::Logical { + op: perry_hir::LogicalOp::And, + left: Box::new(flag.clone()), + right: Box::new(new_c()), + }))], + ), + function( + 38, + "or_without_guard", + vec![Stmt::Return(Some(Expr::Logical { + op: perry_hir::LogicalOp::Or, + left: Box::new(flag), + right: Box::new(new_c()), + }))], + ), + ]); + assert_eq!(facts.return_shape_class(36), Some("C")); + assert_eq!(facts.return_shape_class(37), None); + assert_eq!(facts.return_shape_class(38), None); +} + +/// A fresh object is always truthy. Therefore `new D() && new C()` returns +/// only `C`, while `new C() || new D()` also returns only `C`. The consumed +/// allocation must not make the classes appear to disagree. +#[test] +fn consumed_logical_allocation_does_not_set_the_return_class() { + 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 (facts, _) = facts_for_classes( + vec![class_d()], + vec![ + function( + 39, + "and_consumes_left", + vec![Stmt::Return(Some(Expr::Logical { + op: perry_hir::LogicalOp::And, + left: Box::new(new_d.clone()), + right: Box::new(new_c()), + }))], + ), + function( + 40, + "or_consumes_right", + vec![Stmt::Return(Some(Expr::Logical { + op: perry_hir::LogicalOp::Or, + left: Box::new(new_c()), + right: Box::new(new_d), + }))], + ), + ], + ); + assert_eq!(facts.return_shape_class(39), Some("C")); + assert_eq!(facts.return_shape_class(40), Some("C")); +} + +/// `??` branches on nullishness rather than truthiness and deliberately stays +/// outside this increment. Treating it as `||` would be wrong for `0`, `false` +/// and the empty string. +#[test] +fn nullish_coalescing_return_remains_fail_closed() { + let (facts, _) = facts_for(vec![function( + 41, + "coalesce", + vec![Stmt::Return(Some(Expr::Logical { + op: perry_hir::LogicalOp::Coalesce, + left: Box::new(Expr::Null), + right: Box::new(new_c()), + }))], + )]); + assert_eq!(facts.return_shape_class(41), None); +} + /// 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. ///