diff --git a/changelog.d/8820-call-return-array-stores.md b/changelog.d/8820-call-return-array-stores.md new file mode 100644 index 0000000000..2e1c50ac40 --- /dev/null +++ b/changelog.d/8820-call-return-array-stores.md @@ -0,0 +1 @@ +Array index assignments whose base is a call expression now preserve the call's statically known Array type and use the typed array-store path while evaluating the base exactly once. Strict writes through that path also honor non-writable and accessor descriptors, read-only length, and non-extensible holes. diff --git a/crates/perry-codegen/src/expr/call_return_array_index_tests.rs b/crates/perry-codegen/src/expr/call_return_array_index_tests.rs new file mode 100644 index 0000000000..68802a99f7 --- /dev/null +++ b/crates/perry-codegen/src/expr/call_return_array_index_tests.rs @@ -0,0 +1,175 @@ +use crate::{compile_module, CompileOptions}; +use perry_hir::types::Type; +use perry_hir::{Class, Expr, Function, Module, Param, Stmt}; + +fn param(id: u32, name: &str, ty: Type) -> Param { + Param { + id, + name: name.to_string(), + ty, + default: None, + decorators: Vec::new(), + is_rest: false, + arguments_object: None, + } +} + +fn function( + id: u32, + name: &str, + params: Vec, + return_type: Type, + body: Vec, +) -> Function { + Function { + id, + name: name.to_string(), + type_params: Vec::new(), + params, + return_type, + body, + is_async: false, + is_generator: false, + is_strict: true, + is_exported: false, + captures: Vec::new(), + decorators: Vec::new(), + was_plain_async: false, + was_unrolled: false, + } +} + +fn call_get_data(selector: i64) -> Expr { + Expr::Call { + callee: Box::new(Expr::PropertyGet { + object: Box::new(Expr::This), + property: "getData".to_string(), + byte_offset: 0, + }), + args: vec![Expr::Integer(selector)], + type_args: Vec::new(), + byte_offset: 0, + } +} + +fn store_class(receiver_selector: i64) -> Class { + let get_data = function( + 2, + "getData", + vec![param(3, "selector", Type::Number)], + Type::Array(Box::new(Type::Any)), + vec![Stmt::Return(Some(Expr::Array(vec![Expr::Number(0.0)])))], + ); + let write = function( + 3, + "write", + vec![ + param(1, "index", Type::Number), + param(2, "value", Type::Any), + ], + Type::Void, + vec![Stmt::Expr(Expr::PutValueSet { + target: Box::new(call_get_data(0)), + key: Box::new(Expr::LocalGet(1)), + value: Box::new(Expr::LocalGet(2)), + receiver: Box::new(call_get_data(receiver_selector)), + strict: true, + })], + ); + Class { + id: 1, + name: "Store".to_string(), + type_params: Vec::new(), + extends: None, + extends_name: None, + native_extends: None, + extends_expr: None, + heritage_lexically_shadowed: false, + fields: Vec::new(), + constructor: None, + methods: vec![get_data, write], + getters: Vec::new(), + setters: Vec::new(), + static_accessor_names: Vec::new(), + static_accessor_fn_ids: Vec::new(), + computed_members: Vec::new(), + static_fields: Vec::new(), + static_methods: Vec::new(), + decorators: Vec::new(), + is_exported: false, + aliases: Vec::new(), + is_nested: false, + alloc_width_hint: 0, + specialized_from: None, + } +} + +fn compile_store_ir(receiver_selector: i64) -> String { + let mut module = Module::new("call_return_array_put_value.ts"); + module.classes.push(store_class(receiver_selector)); + let bytes = compile_module( + &module, + CompileOptions { + emit_ir_only: true, + ..Default::default() + }, + ) + .expect("call-returned array store compiles"); + String::from_utf8(bytes).expect("LLVM IR is UTF-8") +} + +fn write_method_ir(ir: &str) -> &str { + let signature = "define double @perry_method_call_return_array_put_value_ts__Store__write("; + let start = ir.find(signature).expect("write method is present in IR"); + let method_and_rest = &ir[start..]; + let end = method_and_rest + .find("\n}\n") + .expect("write method has a closing brace"); + &method_and_rest[..end + 3] +} + +#[test] +fn same_call_returned_array_uses_array_index_store_and_evaluates_receiver_once() { + let ir = compile_store_ir(0); + let write_ir = write_method_ir(&ir); + + assert!( + write_ir.contains("call i64 @js_typed_feedback_array_set_index_or_string("), + "a call with an Array return type must use the array-index semantic fallback:\n{write_ir}" + ); + assert!( + !write_ir.contains("call double @js_put_value_set_dyn_ic("), + "the proven array receiver must not enter the generic Proxy-compatible PutValue ladder:\n{write_ir}" + ); + assert_eq!( + write_ir + .matches( + "call double @perry_method_call_return_array_put_value_ts__Store__getData(" + ) + .count(), + 1, + "the syntactically duplicated target/receiver call represents one evaluated assignment base" + ); +} + +#[test] +fn distinct_call_receiver_stays_on_explicit_receiver_put_value_path() { + let ir = compile_store_ir(1); + let write_ir = write_method_ir(&ir); + + assert!( + !write_ir.contains("call i64 @js_typed_feedback_array_set_index_or_string("), + "a receiver that differs from the target must not use same-receiver array lowering:\n{write_ir}" + ); + assert!( + write_ir.contains("call double @js_put_value_set("), + "the distinct receiver must be passed to the generic PutValue helper:\n{write_ir}" + ); + assert_eq!( + write_ir + .matches("call double @perry_method_call_return_array_put_value_ts__Store__getData(") + .count(), + 2, + "target and distinct receiver calls are independently evaluated" + ); +} diff --git a/crates/perry-codegen/src/expr/mod.rs b/crates/perry-codegen/src/expr/mod.rs index dbfed78b52..5cb84eb1b5 100644 --- a/crates/perry-codegen/src/expr/mod.rs +++ b/crates/perry-codegen/src/expr/mod.rs @@ -161,6 +161,8 @@ mod write_pic_barrier_tests; // temp alloca through the same shadow-slot emission every named local uses, // and it now lives outside `crate::expr`. #[cfg(test)] +mod call_return_array_index_tests; +#[cfg(test)] mod call_spread_rooting_tests; mod call_spread_short; #[cfg(test)] diff --git a/crates/perry-codegen/src/expr/proxy_reflect.rs b/crates/perry-codegen/src/expr/proxy_reflect.rs index 8e0a0450c5..0d3bca3bc4 100644 --- a/crates/perry-codegen/src/expr/proxy_reflect.rs +++ b/crates/perry-codegen/src/expr/proxy_reflect.rs @@ -1260,7 +1260,15 @@ fn is_numeric_string_key(key: &str) -> bool { } fn put_value_index_fast_path(ctx: &FnCtx<'_>, target: &Expr, key: &Expr, receiver: &Expr) -> bool { - if !same_side_effect_free_receiver(target, receiver) { + // `PutValueSet` stores the assignment base in both `target` and `receiver`; + // those two HIR trees describe one source evaluation, not two evaluations + // that may be coalesced only when pure. Use the same structural-identity + // check as the generic same-receiver PutValue lowering below so expressions + // such as `this.getData()[index] = value` can retain the statically known + // Array type. `IndexSet::lower` evaluates that base once. A genuinely + // distinct receiver (including a call with different arguments) still + // fails closed to the explicit-receiver runtime path. + if !same_put_value_receiver_expr(target, receiver) { return false; } if is_array_expr(ctx, target) { diff --git a/crates/perry-runtime/src/array/indexing.rs b/crates/perry-runtime/src/array/indexing.rs index d6a25b9343..efde64e781 100644 --- a/crates/perry-runtime/src/array/indexing.rs +++ b/crates/perry-runtime/src/array/indexing.rs @@ -1084,10 +1084,11 @@ pub extern "C" fn js_array_set_f64(arr: *mut ArrayHeader, index: u32, value: f64 /// (`index_set` / `index` / `field_set_by_name`) routes here. /// test262 built-ins/Array element/add on frozen|sealed|non-extensible. /// Strict-mode guard for a would-be `arr[index] = v` element write: throws the -/// spec `Set`-with-`Throw` TypeError when `arr` is frozen (existing index → -/// read-only) or non-extensible and the index is new (→ not-extensible). No-op -/// for writable slots, buffers, and typed arrays (which own their store -/// semantics). Shared by the strict element-write entry points. +/// spec `Set`-with-`Throw` TypeError when an own data descriptor is read-only, +/// an accessor has no setter, `length` is read-only and would grow, the array +/// is frozen, or a non-extensible array would gain a new element. No-op for +/// writable slots, buffers, and typed arrays (which own their store semantics). +/// Shared by the strict element-write entry points. #[inline] pub(crate) fn array_strict_index_write_guard(arr: *mut ArrayHeader, index: u32) { let clean = clean_arr_ptr_mut(arr); @@ -1099,12 +1100,49 @@ pub(crate) fn array_strict_index_write_guard(arr: *mut ArrayHeader, index: u32) } let flags = array_object_flags(clean); let length = unsafe { (*clean).length }; + + // A descriptor-bearing array is rare, so keep all key construction and + // side-table probes off the ordinary dense-array path. An accessor with a + // setter remains writable even when the object is frozen; return early and + // let `js_array_set_f64_extend` invoke it. Every other rejected descriptor + // must throw here because that lower-level helper deliberately retains a + // silent contract for internal DefineOwnProperty callers. + if flags & crate::gc::OBJ_FLAG_ARRAY_DESCRIPTORS != 0 { + let key = index.to_string(); + if let Some(accessor) = crate::object::get_accessor_descriptor(clean as usize, &key) { + if accessor.set == 0 { + throw_frozen_array_index_write(index); + } + return; + } + if crate::object::get_property_attrs(clean as usize, &key) + .is_some_and(|attrs| !attrs.writable()) + { + throw_frozen_array_index_write(index); + } + if index >= length + && crate::object::get_property_attrs(clean as usize, "length") + .is_some_and(|attrs| !attrs.writable()) + { + crate::collection_iter::throw_type_error( + "Cannot assign to read only property 'length' of object '[object Array]'", + ); + } + } + if index < length { - // Existing index: only a *frozen* array's data is non-writable; a - // sealed / non-extensible array still permits overwriting it. if flags & crate::gc::OBJ_FLAG_FROZEN != 0 { throw_frozen_array_index_write(index); } + // `length` includes holes. Filling one creates a new own property, so + // sealed/preventExtensions arrays must reject it even though the index + // is numerically in bounds. This probe is confined to the already-cold + // restricted-object branch. + if flags & (crate::gc::OBJ_FLAG_SEALED | crate::gc::OBJ_FLAG_NO_EXTEND) != 0 + && !unsafe { array_has_own_index(clean, index) } + { + throw_array_not_extensible_add(index); + } } else if flags & (crate::gc::OBJ_FLAG_FROZEN | crate::gc::OBJ_FLAG_SEALED | crate::gc::OBJ_FLAG_NO_EXTEND) != 0 diff --git a/crates/perry/tests/call_return_array_index.rs b/crates/perry/tests/call_return_array_index.rs new file mode 100644 index 0000000000..fe5e9732e0 --- /dev/null +++ b/crates/perry/tests/call_return_array_index.rs @@ -0,0 +1,149 @@ +//! Executable semantics for array-index stores whose assignment base is a +//! call expression. The HIR repeats that call as the PutValue target and +//! receiver, but the source evaluates it exactly once. + +use std::path::PathBuf; +use std::process::Command; + +fn perry_bin() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_perry")) +} + +fn compile_and_run(source: &str) -> String { + let dir = tempfile::tempdir().expect("tempdir"); + let entry = dir.path().join("main.ts"); + let output = dir.path().join("main_bin"); + std::fs::write(&entry, source).expect("write entry"); + + let compile = Command::new(perry_bin()) + .current_dir(dir.path()) + .arg("compile") + .arg(&entry) + .arg("-o") + .arg(&output) + .arg("--no-cache") + .output() + .expect("run perry compile"); + assert!( + compile.status.success(), + "perry compile failed\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&compile.stdout), + String::from_utf8_lossy(&compile.stderr) + ); + + let run = Command::new(&output) + .current_dir(dir.path()) + .output() + .expect("run compiled binary"); + assert!( + run.status.success(), + "compiled binary failed\nstatus: {:?}\nstdout:\n{}\nstderr:\n{}", + run.status, + String::from_utf8_lossy(&run.stdout), + String::from_utf8_lossy(&run.stderr) + ); + String::from_utf8_lossy(&run.stdout).into_owned() +} + +#[test] +fn call_returned_array_store_preserves_evaluation_proxy_and_descriptor_semantics() { + let stdout = compile_and_run( + r#" +class Store { + calls = 0; + constructor(public data: any[]) {} + + getData(): any[] { + this.calls++; + return this.data; + } + + write(index: number, value: any): any { + return this.getData()[index] = value; + } +} + +const plain: any[] = [10, 20]; +const store = new Store(plain); +console.log("integer", store.write(1, 41), plain[1], store.calls); +console.log("fractional", store.write(1.5, 77), plain["1.5"], plain.length, store.calls); + +const traps: string[] = []; +const target: any[] = [1, 2]; +const proxy: any[] = new Proxy(target, { + set(t: any, key: any, value: any, receiver: any) { + traps.push(String(key) + ":" + String(value)); + return Reflect.set(t, key, value, receiver); + }, +}); +const proxyStore = new Store(proxy); +console.log("proxy", proxyStore.write(0, 9), target[0], proxyStore.calls, traps.join(",")); + +const locked: any[] = [5]; +Object.defineProperty(locked, "0", { value: 5, writable: false }); +const lockedStore = new Store(locked); +let rejected = false; +try { + lockedStore.write(0, 8); +} catch (_error) { + rejected = true; +} +console.log("locked", rejected, locked[0], lockedStore.calls); + +const getterOnly: any[] = [6]; +Object.defineProperty(getterOnly, "0", { get() { return 6; } }); +const getterStore = new Store(getterOnly); +rejected = false; +try { + getterStore.write(0, 8); +} catch (_error) { + rejected = true; +} +console.log("getter-only", rejected, getterOnly[0], getterStore.calls); + +const accessor: any[] = [4]; +let setterValue = 0; +Object.defineProperty(accessor, "0", { + get() { return setterValue; }, + set(value: any) { setterValue = value; }, +}); +const accessorStore = new Store(accessor); +console.log("setter", accessorStore.write(0, 12), accessor[0], accessorStore.calls); + +const hole: any[] = [1, 2, 3]; +delete hole[1]; +Object.preventExtensions(hole); +const holeStore = new Store(hole); +rejected = false; +try { + holeStore.write(1, 9); +} catch (_error) { + rejected = true; +} +console.log("sealed-hole", rejected, hole[1], holeStore.calls); + +const fixedLength: any[] = [1]; +Object.defineProperty(fixedLength, "length", { writable: false }); +const fixedLengthStore = new Store(fixedLength); +rejected = false; +try { + fixedLengthStore.write(1, 2); +} catch (_error) { + rejected = true; +} +console.log("fixed-length", rejected, fixedLength.length, fixedLengthStore.calls); +"#, + ); + + assert_eq!( + stdout, + "integer 41 41 1\n\ + fractional 77 77 2 2\n\ + proxy 9 9 1 0:9\n\ + locked true 5 1\n\ + getter-only true 6 1\n\ + setter 12 12 1\n\ + sealed-hole true undefined 1\n\ + fixed-length true 1 1\n" + ); +}