diff --git a/changelog.d/8607-pipeline-null-default-add.md b/changelog.d/8607-pipeline-null-default-add.md new file mode 100644 index 0000000000..36fdb7afde --- /dev/null +++ b/changelog.d/8607-pipeline-null-default-add.md @@ -0,0 +1,6 @@ +### Performance + +- Speed up generic registries and record-processing pipelines by caching stable + array fields on proven-contained receivers and using a guarded numeric fast + path for null-defaulted counters, while preserving dynamic JavaScript + semantics on aliased and non-number fallback paths. diff --git a/crates/perry-codegen/src/codegen/artifacts.rs b/crates/perry-codegen/src/codegen/artifacts.rs index 7fb46bb014..ed9c2a86fb 100644 --- a/crates/perry-codegen/src/codegen/artifacts.rs +++ b/crates/perry-codegen/src/codegen/artifacts.rs @@ -362,6 +362,7 @@ pub(super) fn emit_module_artifacts(c: ModuleArtifactsCtx<'_>) -> Result<()> { false, None, Some(nonnegative_index_params), + false, ) .with_context(|| { format!( @@ -395,6 +396,7 @@ pub(super) fn emit_module_artifacts(c: ModuleArtifactsCtx<'_>) -> Result<()> { .contains_key(&(class.name.clone(), method.name.clone())), None, None, + false, ) .with_context(|| format!("lowering method '{}::{}'", class.name, method.name))?; // Representation-selection Phase 5a: the additive `internal` @@ -431,6 +433,7 @@ pub(super) fn emit_module_artifacts(c: ModuleArtifactsCtx<'_>) -> Result<()> { false, Some(fact.clone()), None, + false, ) .with_context(|| { format!( @@ -438,6 +441,48 @@ pub(super) fn emit_module_artifacts(c: ModuleArtifactsCtx<'_>) -> Result<()> { class.name, method.name ) })?; + + // #8607: a second, stricter clone for the Phase 3b + // provenance+containment route. Its synthetic immutable + // aliases keep stable array-valued fields in local slots, so + // existing local-array loop optimizations can see through + // repeated `this.field` uses. It is never selected by the + // guarded or dispatch-tower `$pshape` routes. + if let Some(cached_method) = + crate::collectors::ptr_array_cached_method(class, method) + { + compile_method( + llmod, + class, + &cached_method, + func_names, + strings, + class_table, + method_names, + module_globals, + module_global_types, + opts.import_function_prefixes, + enum_table, + static_field_globals, + class_ids, + func_signatures, + func_synthetic_arguments, + module_boxed_vars, + closure_rest_params, + cross_module, + None, + false, + Some(fact.clone()), + None, + true, + ) + .with_context(|| { + format!( + "lowering contained-receiver array-cache clone of method '{}::{}'", + class.name, method.name + ) + })?; + } } } for member in class @@ -468,6 +513,7 @@ pub(super) fn emit_module_artifacts(c: ModuleArtifactsCtx<'_>) -> Result<()> { false, None, None, + false, ) .with_context(|| { format!( @@ -534,6 +580,7 @@ pub(super) fn emit_module_artifacts(c: ModuleArtifactsCtx<'_>) -> Result<()> { false, None, None, + false, ) .with_context(|| format!("lowering getter '{}::{}'", class.name, prop))?; } @@ -588,6 +635,7 @@ pub(super) fn emit_module_artifacts(c: ModuleArtifactsCtx<'_>) -> Result<()> { false, None, None, + false, ) .with_context(|| format!("lowering setter '{}::{}'", class.name, prop))?; } @@ -684,6 +732,7 @@ pub(super) fn emit_module_artifacts(c: ModuleArtifactsCtx<'_>) -> Result<()> { false, None, None, + false, ) .with_context(|| format!("lowering constructor for '{}'", class.name))?; } diff --git a/crates/perry-codegen/src/codegen/method.rs b/crates/perry-codegen/src/codegen/method.rs index 436e7d2bef..5d9b9c799c 100644 --- a/crates/perry-codegen/src/codegen/method.rs +++ b/crates/perry-codegen/src/codegen/method.rs @@ -263,6 +263,7 @@ pub(super) fn compile_method( force_generic_body: bool, proven_this: Option, nonnegative_index_params: Option<&[u32]>, + ptr_array_cache_clone: bool, ) -> Result<()> { let public_llvm_name = methods .get(&(class.name.clone(), method.name.clone())) @@ -282,8 +283,11 @@ pub(super) fn compile_method( let is_pshape_clone = proven_this.is_some(); let is_index_clone = nonnegative_index_params.is_some(); debug_assert!(!(is_pshape_clone && is_index_clone)); + debug_assert!(!ptr_array_cache_clone || is_pshape_clone); let llvm_name = if let Some(params) = nonnegative_index_params { crate::codegen::nonnegative_index_method_name(&public_llvm_name, params) + } else if ptr_array_cache_clone { + crate::collectors::ptr_array_cache_method_name(&public_llvm_name) } else if is_pshape_clone { crate::collectors::pshape_method_name(&public_llvm_name) } else if typed_public_trampoline.is_some() || force_generic_body { diff --git a/crates/perry-codegen/src/collectors/mod.rs b/crates/perry-codegen/src/collectors/mod.rs index 010c4a513f..97d96591c9 100644 --- a/crates/perry-codegen/src/collectors/mod.rs +++ b/crates/perry-codegen/src/collectors/mod.rs @@ -84,7 +84,8 @@ pub(crate) use number_by_construction::collect_number_by_construction_locals; pub(crate) use param_ranges::{collect_param_int_ranges, ParamIntRanges}; pub(crate) use pointer_locals::collect_pointer_typed_locals; pub(crate) use proven_this::{ - method_proven_this, prune_unregistered_clones, pshape_method_name, + method_proven_this, prune_unregistered_clones, pshape_method_name, ptr_array_cache_fields, + ptr_array_cache_method_name, ptr_array_cached_method, tower_route_profitable as pshape_tower_route_profitable, }; pub(crate) use ptr_numarray::{NumArrayDensity, NumArrayLocal}; diff --git a/crates/perry-codegen/src/collectors/proven_this.rs b/crates/perry-codegen/src/collectors/proven_this.rs index 4d40ec03ba..9d732d4166 100644 --- a/crates/perry-codegen/src/collectors/proven_this.rs +++ b/crates/perry-codegen/src/collectors/proven_this.rs @@ -113,7 +113,8 @@ use std::collections::{HashMap, HashSet}; -use perry_hir::{Class, Expr, Function}; +use perry_hir::types::Type; +use perry_hir::{Class, Expr, Function, Stmt}; use super::ptr_shape::{ chain_admissible, chain_classes, chain_field_names, chain_method_map, ptr_shape_locals_enabled, @@ -154,6 +155,337 @@ pub(crate) fn pshape_method_name(public_name: &str) -> String { format!("{public_name}$pshape") } +/// The contained-receiver-only array-field-cache clone. +/// +/// Unlike [`pshape_method_name`], this clone must never be selected merely +/// because a call site has re-checked an aliased receiver's shape. Its body +/// keeps array-valued `this.field` loads in locals across calls in the method, +/// which requires Phase 3b's stronger provenance + containment proof. The +/// sole routing site is therefore the guard-free `Ptr` local arm in +/// `lower_call/property_get/dynamic_dispatch.rs`. +pub(crate) fn ptr_array_cache_method_name(public_name: &str) -> String { + format!("{public_name}$ptr_arrays") +} + +/// Build a method body that snapshots stable, array-valued fields of a +/// contained receiver into immutable locals at entry. +/// +/// This is deliberately a separate clone rather than a change to `$pshape`: +/// exact shape alone fixes field *offsets*, not field *values*. An aliased +/// receiver could have one of its array slots replaced by a callback while a +/// method is running. A Phase 3b local has the extra containment proof that +/// rules that alias out, and the restrictions in [`ptr_array_cache_fields`] +/// reject direct slot replacement and internally-dispatched `this` calls. +pub(crate) fn ptr_array_cached_method(class: &Class, method: &Function) -> Option { + let fields = ptr_array_cache_fields(class, method); + if fields.is_empty() { + return None; + } + + let mut used_ids = HashSet::new(); + super::collect_let_ids(&method.body, &mut used_ids); + super::collect_ref_ids_in_stmts(&method.body, &mut used_ids); + used_ids.extend(method.params.iter().map(|p| p.id)); + used_ids.extend(method.captures.iter().copied()); + let mut next_id = used_ids.iter().copied().max().unwrap_or(0); + + let mut aliases: HashMap = HashMap::new(); + for (name, ty) in fields { + loop { + next_id = next_id.checked_add(1)?; + if used_ids.insert(next_id) { + break; + } + } + aliases.insert(name, (next_id, ty)); + } + + let mut cached = method.clone(); + rewrite_array_field_reads_in_stmts(&mut cached.body, &aliases); + + // Preserve class declaration order. Apart from making generated IR stable, + // this matches the order in which the source-level equivalent would bind + // the aliases. + let mut prefix = Vec::with_capacity(aliases.len()); + for field in &class.fields { + let Some((id, ty)) = aliases.get(&field.name) else { + continue; + }; + prefix.push(Stmt::Let { + id: *id, + name: format!("__perry_ptr_array_{}", field.name), + ty: ty.clone(), + mutable: false, + init: Some(Expr::PropertyGet { + object: Box::new(Expr::This), + property: field.name.clone(), + byte_offset: 0, + }), + }); + } + prefix.append(&mut cached.body); + cached.body = prefix; + Some(cached) +} + +/// Array fields worth caching for [`ptr_array_cached_method`]. Empty means no +/// clone may be emitted or routed to. +/// +/// The optimization is intentionally narrow: +/// +/// * the method contains a loop (otherwise the extra roots/code size do not +/// amortize); +/// * the field is an own, statically-named `Array` initialized by an array +/// literal and is read through `this` in the method; +/// * the method neither replaces/deletes that field nor invokes another +/// method with the same `this` (which could replace it transitively). +/// +/// Array *contents* may still change. The alias holds the same array object, +/// so pushes and indexed stores remain observable exactly as before. +pub(crate) fn ptr_array_cache_fields(class: &Class, method: &Function) -> Vec<(String, Type)> { + if !stmts_contain_loop(&method.body) { + return Vec::new(); + } + + let candidates: HashMap<&str, &Type> = class + .fields + .iter() + .filter(|field| { + field.key_expr.is_none() + && matches!(field.ty, Type::Array(_)) + && matches!(field.init, Some(Expr::Array(_) | Expr::ArraySpread(_))) + }) + .map(|field| (field.name.as_str(), &field.ty)) + .collect(); + if candidates.is_empty() { + return Vec::new(); + } + + let mut reads = HashSet::new(); + let mut unsafe_rebind = false; + super::scalar_method_dispatch::for_each_expr_in_stmts(&method.body, &mut |expr| { + match expr { + Expr::PropertyGet { + object, property, .. + } if matches!(object.as_ref(), Expr::This) + && candidates.contains_key(property.as_str()) => + { + reads.insert(property.clone()); + } + Expr::PropertySet { + object, property, .. + } + | Expr::PropertyUpdate { + object, property, .. + } if matches!(object.as_ref(), Expr::This) + && candidates.contains_key(property.as_str()) => + { + unsafe_rebind = true; + } + // A computed own-property write could name any candidate field. + Expr::IndexSet { object, .. } | Expr::IndexUpdate { object, .. } + if matches!(object.as_ref(), Expr::This) => + { + unsafe_rebind = true; + } + Expr::PutValueSet { + target, receiver, .. + } if matches!(target.as_ref(), Expr::This) + || matches!(receiver.as_ref(), Expr::This) => + { + unsafe_rebind = true; + } + Expr::Delete(inner) + if matches!( + inner.as_ref(), + Expr::PropertyGet { object, .. } | Expr::IndexGet { object, .. } + if matches!(object.as_ref(), Expr::This) + ) => + { + unsafe_rebind = true; + } + // `this.m()` is safe for the shape proof because the callee is + // vetted transitively, but its stores could replace a cached + // array slot. Keep this local transform independent of that + // transitive analysis and decline the clone. + Expr::Call { callee, .. } + if matches!( + callee.as_ref(), + Expr::PropertyGet { object, .. } + if matches!(object.as_ref(), Expr::This) + ) => + { + unsafe_rebind = true; + } + Expr::SuperPropertySet { .. } + | Expr::SuperMethodCall { .. } + | Expr::SuperMethodCallSpread { .. } => { + unsafe_rebind = true; + } + _ => {} + } + }); + if unsafe_rebind { + return Vec::new(); + } + + class + .fields + .iter() + .filter(|field| reads.contains(field.name.as_str())) + .filter_map(|field| { + candidates + .get(field.name.as_str()) + .map(|ty| (field.name.clone(), (*ty).clone())) + }) + .collect() +} + +fn stmts_contain_loop(stmts: &[Stmt]) -> bool { + stmts.iter().any(|stmt| match stmt { + Stmt::While { .. } | Stmt::DoWhile { .. } | Stmt::For { .. } => true, + Stmt::If { + then_branch, + else_branch, + .. + } => { + stmts_contain_loop(then_branch) + || else_branch.as_deref().is_some_and(stmts_contain_loop) + } + Stmt::Try { + body, + catch, + finally, + } => { + stmts_contain_loop(body) + || catch + .as_ref() + .is_some_and(|catch| stmts_contain_loop(&catch.body)) + || finally.as_deref().is_some_and(stmts_contain_loop) + } + Stmt::Switch { cases, .. } => cases.iter().any(|case| stmts_contain_loop(&case.body)), + Stmt::Labeled { body, .. } => stmts_contain_loop(std::slice::from_ref(body.as_ref())), + _ => false, + }) +} + +fn rewrite_array_field_reads_in_stmts(stmts: &mut [Stmt], aliases: &HashMap) { + for stmt in stmts { + match stmt { + Stmt::Let { init, .. } => { + if let Some(init) = init { + rewrite_array_field_reads(init, aliases); + } + } + Stmt::Expr(expr) | Stmt::Throw(expr) => { + rewrite_array_field_reads(expr, aliases); + } + Stmt::Return(expr) => { + if let Some(expr) = expr { + rewrite_array_field_reads(expr, aliases); + } + } + Stmt::If { + condition, + then_branch, + else_branch, + } => { + rewrite_array_field_reads(condition, aliases); + rewrite_array_field_reads_in_stmts(then_branch, aliases); + if let Some(else_branch) = else_branch { + rewrite_array_field_reads_in_stmts(else_branch, aliases); + } + } + Stmt::While { condition, body } => { + rewrite_array_field_reads(condition, aliases); + rewrite_array_field_reads_in_stmts(body, aliases); + } + Stmt::DoWhile { body, condition } => { + rewrite_array_field_reads_in_stmts(body, aliases); + rewrite_array_field_reads(condition, aliases); + } + Stmt::For { + init, + condition, + update, + body, + } => { + if let Some(init) = init { + rewrite_array_field_reads_in_stmts( + std::slice::from_mut(init.as_mut()), + aliases, + ); + } + if let Some(condition) = condition { + rewrite_array_field_reads(condition, aliases); + } + if let Some(update) = update { + rewrite_array_field_reads(update, aliases); + } + rewrite_array_field_reads_in_stmts(body, aliases); + } + Stmt::Try { + body, + catch, + finally, + } => { + rewrite_array_field_reads_in_stmts(body, aliases); + if let Some(catch) = catch { + rewrite_array_field_reads_in_stmts(&mut catch.body, aliases); + } + if let Some(finally) = finally { + rewrite_array_field_reads_in_stmts(finally, aliases); + } + } + Stmt::Switch { + discriminant, + cases, + } => { + rewrite_array_field_reads(discriminant, aliases); + for case in cases { + if let Some(test) = &mut case.test { + rewrite_array_field_reads(test, aliases); + } + rewrite_array_field_reads_in_stmts(&mut case.body, aliases); + } + } + Stmt::Labeled { body, .. } => { + rewrite_array_field_reads_in_stmts(std::slice::from_mut(body.as_mut()), aliases) + } + Stmt::Break + | Stmt::Continue + | Stmt::LabeledBreak(_) + | Stmt::LabeledContinue(_) + | Stmt::PreallocateBoxes(_) + | Stmt::PreallocateTdzBoxes(_) + | Stmt::ReleaseBoxes(_) => {} + } + } +} + +fn rewrite_array_field_reads(expr: &mut Expr, aliases: &HashMap) { + if let Expr::PropertyGet { + object, property, .. + } = expr + { + if matches!(object.as_ref(), Expr::This) { + if let Some((id, _)) = aliases.get(property) { + *expr = Expr::LocalGet(*id); + return; + } + } + } + // A nested ordinary function has its own dynamic `this`. A lexical arrow + // that mentioned the method receiver was already rejected by + // `method_proven_this`, so no eligible reference is lost here. + if matches!(expr, Expr::Closure { .. }) { + return; + } + perry_hir::walker::walk_expr_children_mut(expr, &mut |child| { + rewrite_array_field_reads(child, aliases) + }); +} + /// Drop any proven-`this` clone whose method pair never made it into the /// method registry: a pair with no registered public symbol could never have /// been emitted, and a routing site consulting `pshape_methods` must never diff --git a/crates/perry-codegen/src/collectors/proven_this_routing_tests.rs b/crates/perry-codegen/src/collectors/proven_this_routing_tests.rs index 3bb92d985e..b29a159c2a 100644 --- a/crates/perry-codegen/src/collectors/proven_this_routing_tests.rs +++ b/crates/perry-codegen/src/collectors/proven_this_routing_tests.rs @@ -102,6 +102,18 @@ fn field(name: &str, ty: Type) -> ClassField { } } +fn array_field(name: &str) -> ClassField { + ClassField { + name: name.to_string(), + key_expr: None, + ty: Type::Array(Box::new(Type::Number)), + init: Some(Expr::Array(Vec::new())), + is_private: false, + is_readonly: false, + decorators: Vec::new(), + } +} + fn param(id: u32, name: &str, ty: Type) -> Param { Param { id, @@ -400,6 +412,129 @@ fn ptr_shape_local_module() -> Module { m } +/// A small Registry-shaped class for #8607. Repeated `this.keys` / +/// `this.vals` reads in the loop are the shape whose guarded array accesses +/// become ordinary local-array accesses in the contained-receiver clone. +fn array_registry_class() -> Class { + let scan = func( + 110, + "scan", + Vec::new(), + Type::Number, + vec![ + Stmt::Let { + id: 111, + name: "sum".to_string(), + ty: Type::Number, + mutable: true, + init: Some(Expr::Number(0.0)), + }, + Stmt::For { + init: Some(Box::new(Stmt::Let { + id: 112, + name: "i".to_string(), + ty: Type::Number, + mutable: true, + init: Some(Expr::Number(0.0)), + })), + condition: Some(Expr::Compare { + op: perry_hir::CompareOp::Lt, + left: Box::new(Expr::LocalGet(112)), + right: Box::new(Expr::PropertyGet { + object: Box::new(this_get("keys")), + property: "length".to_string(), + byte_offset: 0, + }), + }), + update: Some(Expr::Update { + id: 112, + op: perry_hir::UpdateOp::Increment, + prefix: false, + }), + body: vec![Stmt::Expr(Expr::LocalSet( + 111, + Box::new(Expr::Binary { + op: BinaryOp::Add, + left: Box::new(Expr::LocalGet(111)), + right: Box::new(Expr::IndexGet { + object: Box::new(this_get("vals")), + index: Box::new(Expr::LocalGet(112)), + }), + }), + ))], + }, + Stmt::Return(Some(Expr::LocalGet(111))), + ], + ); + class( + 109, + "ArrayRegistry", + vec![array_field("keys"), array_field("vals")], + vec![scan], + ) +} + +fn ptr_array_cache_module() -> Module { + let mut m = Module::new("ptr_array_cache.ts"); + m.classes = vec![array_registry_class()]; + m.functions = vec![ + // Phase 3b: provenance + containment. This is the ONLY caller allowed + // to select `$ptr_arrays`. + func( + 120, + "contained", + Vec::new(), + Type::Number, + vec![ + Stmt::Let { + id: 121, + name: "result".to_string(), + ty: Type::Number, + mutable: true, + init: Some(Expr::Number(0.0)), + }, + Stmt::Let { + id: 122, + name: "registry".to_string(), + ty: Type::Named("ArrayRegistry".to_string()), + mutable: false, + init: Some(Expr::New { + class_name: "ArrayRegistry".to_string(), + args: Vec::new(), + type_args: Vec::new(), + byte_offset: 0, + cap_args_appended: 0, + }), + }, + Stmt::Expr(Expr::LocalSet( + 121, + Box::new(call(Expr::LocalGet(122), "scan", Vec::new())), + )), + Stmt::Return(Some(Expr::LocalGet(121))), + ], + ), + // A typed parameter is aliased by construction. Exact-shape guards + // may route it to `$pshape`, but never to the cached-value clone. + func( + 123, + "aliased", + vec![param( + 124, + "registry", + Type::Named("ArrayRegistry".to_string()), + )], + Type::Number, + vec![Stmt::Return(Some(call( + Expr::LocalGet(124), + "scan", + Vec::new(), + )))], + ), + ]; + m.init_kind = ModuleInitKind::Eager; + m +} + /// A module whose `probe(c: Counter)` calls both methods on a statically-typed /// parameter. A typed parameter is not a Phase 3b shape-proven local (no /// provenance, no containment), so both calls go through the *guarded* site: @@ -642,6 +777,73 @@ fn ptr_shape_local_typed_fallback_routes_to_proven_this_clone() { ); } +/// #8607: array-field value caching requires the Phase 3b containment proof, +/// not merely an exact shape. Pin both sides so widening a `$pshape` route to +/// this clone cannot silently make an aliased callback-induced slot rebind +/// stale. +#[test] +fn array_field_cache_clone_routes_only_from_contained_receivers() { + let ir = emit(&ptr_array_cache_module(), false); + let cached = ir + .lines() + .find(|line| line.starts_with("define") && line.contains("__scan$ptr_arrays(")) + .and_then(|line| { + let at = line.find('@')?; + let paren = line[at..].find('(')?; + Some(line[at + 1..at + paren].to_string()) + }) + .unwrap_or_else(|| panic!("no contained-receiver array-cache clone emitted:\n{ir}")); + + let contained = function_body(&ir, "__contained("); + assert!( + contained.contains(&format!("call double @{cached}(")), + "the contained receiver must call its array-cache clone:\n{contained}" + ); + let aliased = function_body(&ir, "__aliased("); + assert!( + !aliased.contains("$ptr_arrays"), + "an aliased receiver must never reach a cached-field-value clone:\n{aliased}" + ); + assert!( + aliased.contains("$pshape"), + "the existing exact-shape clone should remain available to the aliased route:\n{aliased}" + ); +} + +#[test] +fn array_field_cache_declines_direct_and_super_slot_rebinding() { + let class = array_registry_class(); + let scan = &class.methods[0]; + assert_eq!( + crate::collectors::ptr_array_cache_fields(&class, scan).len(), + 2, + "the control Registry method should cache both array fields" + ); + + let mut direct = scan.clone(); + direct.body.push(Stmt::Expr(Expr::PropertySet { + object: Box::new(Expr::This), + property: "keys".to_string(), + value: Box::new(Expr::Array(Vec::new())), + })); + assert!( + crate::collectors::ptr_array_cache_fields(&class, &direct).is_empty(), + "a direct slot replacement would make an entry snapshot stale" + ); + + let mut through_super = scan.clone(); + through_super.body.push(Stmt::Expr(Expr::SuperPropertySet { + parent_class_id: 1, + parent_class_name: Some("Parent".to_string()), + key: Box::new(Expr::String("keys".to_string())), + value: Box::new(Expr::Array(Vec::new())), + })); + assert!( + crate::collectors::ptr_array_cache_fields(&class, &through_super).is_empty(), + "a super property set still writes with receiver=this" + ); +} + /// The routed call must still hand the callee a shadow-bound receiver slot. /// /// #6925 kept the clone's `(double this, …)` ABI and its shadow-bound, diff --git a/crates/perry-codegen/src/expr/binary.rs b/crates/perry-codegen/src/expr/binary.rs index 5c868ab396..c289ae6c08 100644 --- a/crates/perry-codegen/src/expr/binary.rs +++ b/crates/perry-codegen/src/expr/binary.rs @@ -6,7 +6,7 @@ use anyhow::Result; use perry_hir::types::Type as HirType; -use perry_hir::{BinaryOp, Expr, LogicalOp}; +use perry_hir::{BinaryOp, CompareOp, Expr, LogicalOp}; use crate::lower_string_concat::{ flatten_string_add_chain, lower_string_coerce_concat, lower_string_concat, @@ -128,8 +128,13 @@ fn lower_checked_i32_modulo(ctx: &mut FnCtx<'_>, left: &str, right: &str) -> Str ) } -/// `+` where both operands are statically numeric but at least one of them is -/// numeric only because a DECLARED type said so (#7773, #7776). +/// A `+` tree whose numeric interpretation needs a runtime confirmation. +/// +/// Most callers arrive here because both operands are statically numeric but +/// at least one is numeric only because a DECLARED type said so (#7773, +/// #7776). The other caller is the null-defaulted dynamic value recognized by +/// [`is_null_defaulted_local_plus_numeric_literal`] (#8607): its common value +/// is numeric, but its generic source left the HIR type as `Any`. /// /// Nothing enforces annotations at runtime, so a `x: number` slot reached /// through `as any` really can hold a string — and then the spec says `+` is @@ -178,10 +183,8 @@ fn lower_checked_i32_modulo(ctx: &mut FnCtx<'_>, left: &str, right: &str) -> Str /// The residual cost lands where it is already small: every read that reaches /// here is one the compiler could NOT prove, so it pays an inline header /// precheck or a `js_typed_feedback_class_field_get_guard` call for its shape -/// check regardless. The proven tiers (element-shape / class-field loop facts, -/// `Ptr` numeric fields, scalar replacement, POD records, typed arrays) -/// never get here at all — `numeric_proof_is_declared_only` answers `false`. -fn lower_declared_only_numeric_add(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { +/// check regardless. Proven raw-f64 tiers need no guard at all. +fn lower_guarded_numeric_add(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { let mut leaves = Vec::new(); add_tree_leaves(expr, &mut leaves); let needs_test: Vec = leaves @@ -201,22 +204,18 @@ fn lower_declared_only_numeric_add(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result is_num, }); } - // The caller only routes here when a leaf is declared-only, and every - // such leaf is a field / element / local read — none of which - // `expr_produces_canonical_raw_f64` vouches for. So there is always at - // least one test; an empty condition would mean the two predicates had - // drifted apart, which is worth a hard error rather than a silent - // unguarded `fadd`. + // Both callers guarantee at least one value that a static raw-f64 + // proof cannot vouch for: either a declared-only read or the dynamic + // arm of a null-defaulted conditional. Keep that contract explicit so + // future predicate drift cannot silently turn this into an unguarded + // `fadd`. let Some(all_num) = cond else { - anyhow::bail!( - "declared-only `+` tree has no testable leaf: \ - numeric_proof_is_declared_only and expr_produces_canonical_raw_f64 disagree" - ); + anyhow::bail!("guarded `+` tree has no testable leaf"); }; - let fast_idx = ctx.new_block("declared_add.numeric"); - let slow_idx = ctx.new_block("declared_add.dynamic"); - let merge_idx = ctx.new_block("declared_add.merge"); + let fast_idx = ctx.new_block("guarded_add.numeric"); + let slow_idx = ctx.new_block("guarded_add.dynamic"); + let merge_idx = ctx.new_block("guarded_add.merge"); let fast_label = ctx.block_label(fast_idx); let slow_label = ctx.block_label(slow_idx); let merge_label = ctx.block_label(merge_idx); @@ -239,6 +238,55 @@ fn lower_declared_only_numeric_add(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result bool { + (is_strict_null_defaulted_local(left) && is_numeric_literal(right)) + || (is_numeric_literal(left) && is_strict_null_defaulted_local(right)) +} + +fn is_numeric_literal(expr: &Expr) -> bool { + matches!(expr, Expr::Integer(_) | Expr::Number(_)) +} + +fn is_strict_null_defaulted_local(expr: &Expr) -> bool { + let Expr::Conditional { + condition, + then_expr, + else_expr, + } = expr + else { + return false; + }; + if !is_numeric_literal(then_expr) { + return false; + } + let Expr::Compare { + op: CompareOp::Eq, + left, + right, + } = condition.as_ref() + else { + return false; + }; + let compared_local = match (left.as_ref(), right.as_ref()) { + (Expr::LocalGet(id), Expr::Null) | (Expr::Null, Expr::LocalGet(id)) => *id, + _ => return false, + }; + matches!(else_expr.as_ref(), Expr::LocalGet(id) if *id == compared_local) +} + /// The `+` tree's operand leaves, in evaluation order — a left-to-right walk, /// so `with_operands_rooted` lowers them in the order JS evaluates them. fn add_tree_leaves<'a>(expr: &'a Expr, out: &mut Vec<&'a Expr>) { @@ -420,7 +468,7 @@ fn operand_needs_residual_coerce(ctx: &FnCtx<'_>, expr: &Expr, fallback_coerced: // string unchanged. Every non-`+` arithmetic operator is a plain // `ToNumber` on its operands, so a coerce is the whole fix here; // `+` needs the concat dispatch and gets it from - // `lower_declared_only_numeric_add`. Proven raw-f64 tiers answer + // `lower_guarded_numeric_add`. Proven raw-f64 tiers answer // false here, so asking about every expression keeps them exempt. || numeric_proof_is_declared_only(ctx, expr)) } @@ -819,6 +867,15 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // → string concat, BIGINT → bigint add, otherwise numeric. let both_numeric = crate::type_analysis::is_numeric_expr(ctx, left) && crate::type_analysis::is_numeric_expr(ctx, right); + // #8607: a generic registry lookup leaves the counter value + // typed as `Any`, so `(prev === null ? 0 : prev) + 1` used to + // call the full JS add dispatcher on every pipeline stage. + // Keep its dynamic semantics in a cold arm and let the common + // numeric value take the same guarded fadd used for violable + // declared-number reads. + if is_null_defaulted_local_plus_numeric_literal(left, right) { + return lower_guarded_numeric_add(ctx, expr); + } // `+` is the one arithmetic operator that must distinguish // numeric addition from string concatenation before lowering // its operands. Admit native-i1 Booleans only when the other @@ -849,7 +906,7 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { if numeric_proof_is_declared_only(ctx, left) || numeric_proof_is_declared_only(ctx, right) { - return lower_declared_only_numeric_add(ctx, expr); + return lower_guarded_numeric_add(ctx, expr); } } // BigInt arithmetic fast path. NaN-tagged bigints compare diff --git a/crates/perry-codegen/src/expr/mod.rs b/crates/perry-codegen/src/expr/mod.rs index a488357fb3..9f23392719 100644 --- a/crates/perry-codegen/src/expr/mod.rs +++ b/crates/perry-codegen/src/expr/mod.rs @@ -2132,6 +2132,8 @@ mod index_get; #[cfg(test)] mod index_get_claim_tests; mod masked_window; +#[cfg(test)] +mod null_default_numeric_add_tests; mod ptr_numarray_access; mod ta_param_f64_read; pub(crate) use index_get::{ diff --git a/crates/perry-codegen/src/expr/null_default_numeric_add_tests.rs b/crates/perry-codegen/src/expr/null_default_numeric_add_tests.rs new file mode 100644 index 0000000000..a10460fa70 --- /dev/null +++ b/crates/perry-codegen/src/expr/null_default_numeric_add_tests.rs @@ -0,0 +1,86 @@ +//! #8607: IR coverage for null-defaulted dynamic counter increments. + +use perry_hir::types::Type; +use perry_hir::{BinaryOp, CompareOp, Expr, Stmt}; + +use crate::temp_root_coverage::main_ir_for as ir_for; + +const VALUE: u32 = 1; +const RESULT: u32 = 2; + +fn value(init: Expr) -> Stmt { + Stmt::Let { + id: VALUE, + name: "value".to_string(), + ty: Type::Any, + mutable: false, + init: Some(init), + } +} + +fn result(init: Expr) -> Stmt { + Stmt::Let { + id: RESULT, + name: "result".to_string(), + ty: Type::Any, + mutable: false, + init: Some(init), + } +} + +fn increment(expr: Expr) -> Expr { + Expr::Binary { + op: BinaryOp::Add, + left: Box::new(expr), + right: Box::new(Expr::Integer(1)), + } +} + +#[test] +fn null_defaulted_dynamic_increment_has_guarded_numeric_fast_path() { + let ir = ir_for( + "null_defaulted_dynamic_increment", + vec![ + value(Expr::String("4".to_string())), + result(increment(Expr::Conditional { + condition: Box::new(Expr::Compare { + op: CompareOp::Eq, + left: Box::new(Expr::LocalGet(VALUE)), + right: Box::new(Expr::Null), + }), + then_expr: Box::new(Expr::Integer(0)), + else_expr: Box::new(Expr::LocalGet(VALUE)), + })), + ], + ); + + assert!( + ir.contains("guarded_add.numeric") && ir.contains("fadd double"), + "the numeric value must reach the guarded inline add:\n{ir}" + ); + assert!( + ir.contains("guarded_add.dynamic") + && ir.contains("call double @js_dynamic_string_or_number_add("), + "a non-number must retain JavaScript concatenation semantics:\n{ir}" + ); +} + +#[test] +fn arbitrary_dynamic_increment_stays_on_dynamic_dispatch() { + let ir = ir_for( + "arbitrary_dynamic_increment", + vec![ + value(Expr::Undefined), + result(increment(Expr::LocalGet(VALUE))), + ], + ); + + assert!( + !ir.contains("guarded_add.numeric") && !ir.contains("fadd double"), + "an unguarded Any value must not be assumed numeric:\n{ir}" + ); + assert!( + ir.contains("call double @js_dynamic_string_or_number_add("), + "an unguarded Any value must keep JavaScript `+` dispatch:\n{ir}" + ); +} diff --git a/crates/perry-codegen/src/lower_call/property_get/dynamic_dispatch.rs b/crates/perry-codegen/src/lower_call/property_get/dynamic_dispatch.rs index c7af6082e5..b7f903579b 100644 --- a/crates/perry-codegen/src/lower_call/property_get/dynamic_dispatch.rs +++ b/crates/perry-codegen/src/lower_call/property_get/dynamic_dispatch.rs @@ -1379,8 +1379,21 @@ pub(crate) fn try_lower_instance_method_call( .pshape_methods .contains_key(&(class_name.clone(), property.to_string())) .then(|| crate::collectors::pshape_method_name(&fallback_fn)); + // #8607: containment is stronger than the exact-shape + // proof used by the other `$pshape` routes. Only here can + // a method safely keep array-valued receiver fields in + // locals across calls: no alias exists that could replace + // a slot while the method runs. Emission and routing use + // the same structural eligibility predicate. + let ptr_array_cache_target = pshape_target.as_ref().and_then(|_| { + let class = ctx.classes.get(&class_name)?; + let method = class.methods.iter().find(|m| m.name == property)?; + (!crate::collectors::ptr_array_cache_fields(class, method).is_empty()) + .then(|| crate::collectors::ptr_array_cache_method_name(&fallback_fn)) + }); let generic_target = nonnegative_index_direct_name .as_deref() + .or(ptr_array_cache_target.as_deref()) .or(pshape_target.as_deref()) .unwrap_or(fallback_fn.as_str()); // Prefer the typed-receiver clone (bare gep+load field diff --git a/test-files/test_issue_8607_null_default_numeric_add.ts b/test-files/test_issue_8607_null_default_numeric_add.ts new file mode 100644 index 0000000000..5e11646072 --- /dev/null +++ b/test-files/test_issue_8607_null_default_numeric_add.ts @@ -0,0 +1,50 @@ +class Registry { + private keys: K[] = []; + private values: V[] = []; + + get(key: K): V | null { + for (let i = 0; i < this.keys.length; i++) { + if (this.keys[i] === key) return this.values[i]; + } + return null; + } + + set(key: K, value: V): void { + for (let i = 0; i < this.keys.length; i++) { + if (this.keys[i] === key) { + this.values[i] = value; + return; + } + } + this.keys.push(key); + this.values.push(value); + } +} + +function increment(registry: Registry, key: string): any { + const previous = registry.get(key); + const next = (previous === null ? 0 : previous) + 1; + registry.set(key, next); + return next; +} + +// Keep this Registry local and unaliased so codegen may select the +// contained-receiver array-field-cache clone for get/set. The calls above use +// an aliased parameter and therefore exercise the ordinary guarded clone. +function containedCounter(): number | null { + const counts = new Registry(); + for (let i = 0; i < 4; i++) { + const previous = counts.get("count"); + counts.set("count", (previous === null ? 0 : previous) + 1); + } + return counts.get("count"); +} + +const missing = new Registry(); +console.log(increment(missing, "count")); +console.log(increment(missing, "count")); + +const stringValue = new Registry(); +stringValue.set("count", "4"); +console.log(increment(stringValue, "count")); +console.log(containedCounter());