From a439654047c3414e5a8617d5aa8a39a4e3c23f5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Tue, 18 Aug 2026 07:49:48 +0200 Subject: [PATCH] fix(codegen): address object-write review feedback --- changelog.d/6821-review-followups.md | 1 + .../perry-codegen/src/expr/proxy_reflect.rs | 10 +++- crates/perry-codegen/src/stmt/let_stmt.rs | 25 +++++----- crates/perry-codegen/src/stmt/loops.rs | 21 ++------- .../tests/native_proof_regressions.rs | 46 +++++++++++++++++++ 5 files changed, 72 insertions(+), 31 deletions(-) create mode 100644 changelog.d/6821-review-followups.md diff --git a/changelog.d/6821-review-followups.md b/changelog.d/6821-review-followups.md new file mode 100644 index 0000000000..048d12b3c3 --- /dev/null +++ b/changelog.d/6821-review-followups.md @@ -0,0 +1 @@ +fix(codegen): preserve immutable literal key and loop-bound metadata when a local uses module-global or boxed storage, and share static property-key resolution across object-write optimizers. diff --git a/crates/perry-codegen/src/expr/proxy_reflect.rs b/crates/perry-codegen/src/expr/proxy_reflect.rs index 98d0dbea6c..90de6564b9 100644 --- a/crates/perry-codegen/src/expr/proxy_reflect.rs +++ b/crates/perry-codegen/src/expr/proxy_reflect.rs @@ -1081,10 +1081,16 @@ fn lower_put_value_dyn_ic_inline( Ok(result) } -fn static_write_key(ctx: &FnCtx<'_>, key: &Expr) -> Option { +pub(crate) fn static_string_write_key(ctx: &FnCtx<'_>, key: &Expr) -> Option { match key { Expr::String(property) => Some(property.clone()), Expr::LocalGet(id) => ctx.const_string_locals.get(id).cloned(), + _ => None, + } +} + +pub(crate) fn static_write_key(ctx: &FnCtx<'_>, key: &Expr) -> Option { + static_string_write_key(ctx, key).or_else(|| match key { // #6812 (w13): `o[7] = v` — a constant integer key is the canonical // numeric-string property key ("7"; i64 formatting is canonical for // every integer, including negatives). Real arrays never take the IC @@ -1093,7 +1099,7 @@ fn static_write_key(ctx: &FnCtx<'_>, key: &Expr) -> Option { // generic write, which performs the element store. Expr::Integer(n) => Some(n.to_string()), _ => None, - } + }) } fn put_value_rhs_is_safepoint_free(ctx: &FnCtx<'_>, expr: &Expr) -> bool { diff --git a/crates/perry-codegen/src/stmt/let_stmt.rs b/crates/perry-codegen/src/stmt/let_stmt.rs index 850225195c..bc2ee522a7 100644 --- a/crates/perry-codegen/src/stmt/let_stmt.rs +++ b/crates/perry-codegen/src/stmt/let_stmt.rs @@ -93,8 +93,21 @@ pub(crate) fn lower_let( // class-alias chain resolution below (and any other site // that needs id → name) can use it. ctx.local_id_to_name.insert(id, name.to_string()); + // Record immutable literal metadata before the module-global and boxed + // storage paths return. The loop/PIC matchers reason from the HIR local id, + // so the storage representation does not change the const proof. if !mutable { if let Some(init_expr) = init { + if let perry_hir::Expr::String(value) = init_expr { + ctx.const_string_locals.insert(id, value.clone()); + } + if let Some(value) = match init_expr { + perry_hir::Expr::Integer(value) => Some(*value as f64), + perry_hir::Expr::Number(value) if value.is_finite() => Some(*value), + _ => None, + } { + ctx.const_number_locals.insert(id, value); + } if let Some(props) = crate::lower_call::extract_options_fields(ctx, init_expr) { ctx.option_object_locals.insert(id, props); } @@ -1965,17 +1978,5 @@ pub(crate) fn lower_let( let lit = crate::nanbox::double_literal(*cv); ctx.block().store(DOUBLE, &lit, &slot); } - if !mutable { - if let Some(perry_hir::Expr::String(value)) = init { - ctx.const_string_locals.insert(id, value.clone()); - } - if let Some(value) = init.and_then(|expr| match expr { - perry_hir::Expr::Integer(value) => Some(*value as f64), - perry_hir::Expr::Number(value) if value.is_finite() => Some(*value), - _ => None, - }) { - ctx.const_number_locals.insert(id, value); - } - } Ok(()) } diff --git a/crates/perry-codegen/src/stmt/loops.rs b/crates/perry-codegen/src/stmt/loops.rs index b115f911ee..9495d6da49 100644 --- a/crates/perry-codegen/src/stmt/loops.rs +++ b/crates/perry-codegen/src/stmt/loops.rs @@ -2740,18 +2740,7 @@ fn match_object_array_write_loop( // v1: a table-driven lane must be the group's only store. return None; } - let property = match key.as_ref() { - Expr::String(property) => property.clone(), - Expr::LocalGet(id) => ctx.const_string_locals.get(id).cloned()?, - // #6812 (w13): `o[7] = v` — a constant integer key IS the - // canonical numeric-string property ("7") on a plain - // object. Receivers that are real arrays at runtime are - // safe: the preflight guard type-checks every element as - // GC_TYPE_OBJECT and rejects the nest, and the per-write - // fallback handles element writes generically. - Expr::Integer(n) => n.to_string(), - _ => return None, - }; + let property = crate::expr::proxy_reflect::static_write_key(ctx, key.as_ref())?; let value = match_object_array_write_number(value, outer_counter_id, inner_counter_id, &temps)?; // Same size budget as the temps: a value combining several @@ -3702,11 +3691,9 @@ fn match_class_field_versioned_loop( if t != r { return None; } - let prop = match key.as_ref() { - Expr::String(prop) => prop.clone(), - Expr::LocalGet(id) => ctx.const_string_locals.get(id).cloned()?, - _ => return None, - }; + // Keep this class-field clone's existing string-only contract; + // integer keys are handled by the general object-write matcher. + let prop = crate::expr::proxy_reflect::static_string_write_key(ctx, key.as_ref())?; recv = Some(*t); if !class_field_loop_pure_expr_collect(ctx, value, counter_id, &mut recv, &mut props) { return None; diff --git a/crates/perry-codegen/tests/native_proof_regressions.rs b/crates/perry-codegen/tests/native_proof_regressions.rs index 16e48411e0..d18958165e 100644 --- a/crates/perry-codegen/tests/native_proof_regressions.rs +++ b/crates/perry-codegen/tests/native_proof_regressions.rs @@ -14484,6 +14484,52 @@ fn immutable_string_key_reuses_static_write_pic() { ); } +#[test] +fn module_global_immutable_string_key_reuses_static_write_pic() { + let key = 2u32; + let object = 3u32; + let mut module = module_with_classes_and_params( + "module_global_immutable_string_key_write_pic", + Vec::new(), + Vec::new(), + Type::String, + vec![Stmt::Return(Some(Expr::LocalGet(key)))], + ); + module.init = vec![ + Stmt::Let { + id: key, + name: "key".to_string(), + ty: Type::String, + mutable: false, + init: Some(Expr::String("x".to_string())), + }, + Stmt::Let { + id: object, + name: "object".to_string(), + ty: Type::Any, + mutable: false, + init: Some(Expr::Object(vec![("x".to_string(), Expr::Integer(0))])), + }, + Stmt::Expr(Expr::PutValueSet { + target: Box::new(Expr::LocalGet(object)), + key: Box::new(Expr::LocalGet(key)), + value: Box::new(Expr::Integer(1)), + receiver: Box::new(Expr::LocalGet(object)), + strict: false, + }), + ]; + + let ir = compile_ir_for_module_with_opts(module, empty_opts()).unwrap(); + assert!( + ir.contains("call double @js_put_value_set_ic_miss"), + "an immutable string key promoted to module-global storage should retain its static-key metadata:\n{ir}" + ); + assert!( + !ir.contains("call double @js_put_value_set_dyn_ic("), + "the module-global immutable literal key should not fall through to dynamic PutValue:\n{ir}" + ); +} + #[test] fn mutable_string_key_rejects_static_write_pic() { let object = 1u32;