diff --git a/changelog.d/7835-declared-string-concat-and-static-field-store.md b/changelog.d/7835-declared-string-concat-and-static-field-store.md new file mode 100644 index 0000000000..06aaf10213 --- /dev/null +++ b/changelog.d/7835-declared-string-concat-and-static-field-store.md @@ -0,0 +1,66 @@ +### perf(codegen, runtime): a declared `string` is enough to pick the concat lowering, and a `static` counter stops re-interning its own key + +Three changes to the same theme — Perry re-deriving at runtime facts it already had at compile time — plus the runtime guarantee that makes the first one safe. + +Measured on the quiet M1 mini, best-of-5, against a clean build of `1ee158d27` (perry 0.5.1463). Both arms built with `-p perry -p perry-runtime-static -p perry-stdlib-static`; every corpus program's output verified byte-identical to `node --experimental-strip-types` (Node 26.5.1) with exit 0 before timing. + +| bench | before | after | | +|---|--:|--:|--:| +| `concat_field` (new probe) | 0.2739 | **0.1556** | −43.2% | +| `concat_field_base` (its subtrahend) | 0.0969 | 0.0966 | — | +| **ns per concatenation** | **88.5** | **29.5** | **−67%** | +| `gc-handoff/apps/pipeline.ts` | 0.5163 | **0.4846** | −6.1% | +| `gc-handoff/apps/shapes.ts` | 0.1900 | **0.1833** | −3.5% | + +No corpus regression: `churn` 0.4218→0.4242, `churn_alloc` 0.3733→0.3758, `push_cls` 0.3691→0.3693, `push_num` 0.1533→0.1520, `churn_read` 0.0242→0.0230, `cycles` 0.1953→0.1951, `deeplist` 0.1242→0.1244, `tree` 1.6455→1.6437, `tree_wide` 2.1170→2.1166, `retain` 0.3607→0.3603, `retain1` 0.1498→0.1477, `retain_wide` 0.4714→0.4711, `fib40` 0.4063→0.4065, `asyncpipe` 0.1329→0.1331, `interp` 1.5162→1.5159, `iso_miss` 1.9456→1.9433 (`checksum 437840 misses 0`). The six `*_real` globalThis-bootstrap arms stay within ±2% of their base arms, as they were. + +#### 1. `js_string_concat_box` delegates a non-string operand instead of dropping it + +It decoded both operands with `str_bytes_from_jsvalue(...).unwrap_or((null, 0))` — so an operand that was not a string became the **empty string**. `"ab" + 42` through this helper rendered as `"ab"`. + +**This is reachable on `1ee158d27` today — see #7837.** `is_definitely_string_expr`'s `LocalGet` arm already trusts a declared type, so a `string`-declared local holding a non-string selects this helper: + +```ts +const t: string = (99 as any); +console.log(t + "x"); // node: "99x" perry before: "x" <- the 99 is dropped +``` + +Measured on a clean `1ee158d27` build and on this branch; a `string`-declared *field* (`o.t + 7`) and a `(string, number)` parameter pair both route elsewhere and were already correct, which is why the defect survived casual probing. + +It was also the reason the concat fast path could never be selected from a type annotation in the first place: `lower_string_concat.rs`'s self-append lowering carries a whole `dother`/`cold` arm whose comment says, in as many words, that a lie has to be routed around this helper. It now forwards any non-string pair to `js_dynamic_string_or_number_add`, which is the full spec `+`. String+string is unchanged (including the ≤5-byte SSO result encoding); string+number concatenates with the number's decimal form; number+number **adds and returns a number**. + +#7837 records a **second, separate** defect of the same premise that this PR does **not** fix: `s + 7` on a `string`-declared local holding `42` prints `427` instead of `49`, because the one-sided `l ^ r` arm picks the *operator* from the annotation. That arm lowers through `js_string_concat_value`, which takes an already-unboxed `StringHeader*` and cannot detect the lie, so it needs the guarded-diamond treatment #7831 is giving the numeric side — not this PR's runtime delegation. Deciding it is deliberately left to #7837. + +#### 2. `+` accepts a declared `string`, but only where (1) makes that free + +`"shape:" + this.tag` — a string literal plus a field declared `string` — lowered to `js_dynamic_string_or_number_add`: a `RuntimeHandleScope`, four `root_nanbox_f64`s and two `ToPrimitive` calls, spent rediscovering what the declaration already stated. `is_string_expr` has trusted that same declaration for string *method* dispatch since #655; the concat path did not. + +The new predicate is `is_declared_string_expr`, and it is deliberately **separate** from `is_definitely_string_expr` rather than an extension of it. Perry does not enforce annotations at runtime — the exact gap #7831 is closing on the numeric side — so a declaration is evidence, not proof. It therefore has exactly one consumer: the two-operand concat, which emits `js_string_concat_box`. After (1) that helper produces the dynamic-path answer for every combination of runtime values, so **the declaration selects a lowering and can never select an answer.** + +Three neighbours deliberately keep the strict predicate, each because it *would* be able to change an answer: +- the one-sided `l ^ r` arm lowers through `js_string_concat_value` / `js_value_concat_string`, which take an already-unboxed `StringHeader*` and cannot tell a lie from a string; +- the N-way chain fold formats every part as a string, so an all-declared chain of numbers would concatenate where the spec adds; +- the `Map` string-key fast paths in `expr::math_simple` key a lookup on the claim. + +#### 3. `type X = { … }` resolves its property types, like `interface X { … }` already did + +`lower_type_alias_decl` files aliases in `module.type_aliases` while `lower_interface_decl` files interfaces in `module.interfaces`, and `static_type_of` only ever consulted the latter. An object-type alias is structurally interchangeable with an interface in TypeScript — same declaration, same runtime layout, same absence of any layout guarantee — and `type` is the form most application code reaches for. So `type Record = { kind: string; … }` proved **nothing** about `r.kind`, purely because the author wrote `type` instead of `interface`. Only a non-generic alias whose right-hand side is a closed object type answers; an alias to a `Named`/`Generic` type is left alone. + +This is what makes `pipeline.ts`'s `makeTagger` (`prefix + r.kind`, 360,000 calls) reach the concat at all. + +#### 4. `class_dynamic_prop_root_store` stops allocating a key it already has + +Codegen emits `js_class_register_static_field` after every `Expr::StaticFieldSet`, so `Shape.made = Shape.made + 1` inside a constructor runs it **once per construction** — 144,000 times in `shapes.ts`. Each call did `str::from_utf8(…).to_string()` (a heap allocation, immediately dropped, because `HashMap::insert` keeps the *original* key), a `CLASS_DELETED_KEYS` probe, and an `entry().or_insert_with().insert()`. + +The signature is now `&str` — every caller already had a borrowed value — and a store whose key exists updates the slot in place. The in-place arm also skips the deleted-keys probe, but only while **nothing has ever been deleted**; once anything has, the original sequence runs verbatim, so the pre-existing conflation between a deleted prototype key and a same-named static field (`class C { m() {} static m = 1 }`, both under one class_id) keeps whatever behaviour it had. + +#### Tests + +- `perry-runtime`: `concat_box_delegates_a_non_string_operand_to_the_dynamic_add` pins all four operand combinations plus the SSO result encoding; `repeated_store_updates_in_place_and_stays_readable` and `store_after_delete_clears_the_deleted_mark` pin both arms of the static-field store. +- `perry-codegen`: `alias_declared_string_field_takes_the_static_concat`, `field_absent_from_the_alias_keeps_the_dynamic_add`, and `alias_declared_number_field_is_deliberately_not_routed` — the last one asserts the numeric side is *unchanged*, so a future widening there has to be a deliberate edit to this test. + +All in-crate unit tests, so they run in the per-PR `cargo-test` job rather than the nightly-only integration suites (#5960). + +#### Probes + +`gc-handoff/bench/concat_field.ts` and `concat_field_base.ts` are the ns/concat pair: identical programs, 2,000,000 `"shape:" + this.tag` concatenations, differing only in whether `describe()` concatenates or returns a same-length constant. diff --git a/crates/perry-codegen/src/expr/binary.rs b/crates/perry-codegen/src/expr/binary.rs index f00a2bea78..72a3e3758a 100644 --- a/crates/perry-codegen/src/expr/binary.rs +++ b/crates/perry-codegen/src/expr/binary.rs @@ -460,7 +460,23 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { } } - if l_is_str && r_is_str { + // The pairwise concat — and ONLY the pairwise concat — accepts a + // DECLARED `string` as well as a structurally-proven one, so + // `"shape:" + this.tag` and `prefix + r.kind` stop paying + // `js_dynamic_string_or_number_add`'s scope + four roots + two + // `ToPrimitive`s to rediscover what the declarations said. + // + // Sound for a LYING declaration, not merely unlikely to meet + // one: this arm emits `js_string_concat_box`, which + // tag-dispatches both operands and forwards any non-string pair + // to `js_dynamic_string_or_number_add` — so string+string, + // string+number and number+number all return exactly what the + // dynamic path returns. See `is_declared_string_expr` for why + // the chain fold above and the one-sided arm below must keep + // the strict predicate. + if crate::type_analysis::is_declared_string_expr(ctx, left) + && crate::type_analysis::is_declared_string_expr(ctx, right) + { return lower_string_concat(ctx, left, right); } if l_is_str || r_is_str { diff --git a/crates/perry-codegen/src/type_analysis.rs b/crates/perry-codegen/src/type_analysis.rs index 34840e3ac0..e253305cbf 100644 --- a/crates/perry-codegen/src/type_analysis.rs +++ b/crates/perry-codegen/src/type_analysis.rs @@ -55,9 +55,9 @@ pub(crate) use refine::{ is_process_namespace_version_property, refine_type_from_init, }; pub(crate) use strings::{ - class_name_extends_url_search_params, is_definitely_string_expr, is_map_expr, is_set_expr, - is_string_expr, is_url_search_params_expr, is_url_search_params_subclass_expr, - map_static_type_args, set_static_type_args, + class_name_extends_url_search_params, is_declared_string_expr, is_definitely_string_expr, + is_map_expr, is_set_expr, is_string_expr, is_url_search_params_expr, + is_url_search_params_subclass_expr, map_static_type_args, set_static_type_args, }; #[cfg(test)] diff --git a/crates/perry-codegen/src/type_analysis/predicates.rs b/crates/perry-codegen/src/type_analysis/predicates.rs index 741531e3ba..b6d63fa643 100644 --- a/crates/perry-codegen/src/type_analysis/predicates.rs +++ b/crates/perry-codegen/src/type_analysis/predicates.rs @@ -626,6 +626,30 @@ pub(crate) fn static_type_of(ctx: &FnCtx<'_>, e: &Expr) -> Option { } } } + // The `type X = { … }` half of #655. A TS object-type ALIAS is + // structurally interchangeable with an `interface` — the same + // declaration, the same runtime layout (a plain object), the + // same absence of any layout guarantee — and `type` is the form + // most application code reaches for. But `lower_type_alias_decl` + // files aliases in `module.type_aliases` while + // `lower_interface_decl` files interfaces in + // `module.interfaces`, and only the latter was consulted here. + // So `type Record = { kind: string; amount: number }` proved + // NOTHING about `r.kind` / `r.amount`: `"t:" + r.kind` lowered + // to `js_dynamic_string_or_number_add` and `r.amount + r.id` + // picked up `js_number_coerce` on both sides, purely because + // the author wrote `type` instead of `interface`. + // + // Only a non-generic alias whose right-hand side is a closed + // object type answers here. An alias to a `Named`/`Generic` + // type is left alone — resolving those would need the + // cycle-guarded chain walk the class path has, and they are not + // what this is for. + if let Some(HirType::Object(obj)) = ctx.type_aliases.get(&receiver_class) { + if let Some(p) = obj.properties.get(property) { + return Some(p.ty.clone()); + } + } } hir_inferred_static_type(ctx, e) } diff --git a/crates/perry-codegen/src/type_analysis/strings.rs b/crates/perry-codegen/src/type_analysis/strings.rs index 19d0b4a5b2..b1e9e3c001 100644 --- a/crates/perry-codegen/src/type_analysis/strings.rs +++ b/crates/perry-codegen/src/type_analysis/strings.rs @@ -301,6 +301,50 @@ pub(crate) fn is_definitely_string_expr(ctx: &FnCtx<'_>, e: &Expr) -> bool { } } +/// True when a DECLARATION says this expression is a `string`: a `string` +/// field on a class, interface or object-type alias (`this.tag`, `r.kind`), a +/// method whose declared return type is `string`, an element of a `string[]`. +/// +/// This is deliberately NOT folded into [`is_definitely_string_expr`], and the +/// distinction is the whole point. Perry does not enforce annotations at +/// runtime, so a declaration is evidence, not proof — the same gap #7831 is +/// closing on the numeric side, where a declared-`number` field that actually +/// held a string made `fadd` propagate the string's NaN payload and +/// `typeof (v * 2)` answer `"string"`. +/// +/// So this predicate has exactly ONE consumer: the two-operand +/// `a + b` concat in `expr::binary`, which lowers to `js_string_concat_box`. +/// That helper tag-dispatches both operands and hands ANY non-string pair +/// straight to `js_dynamic_string_or_number_add`, so every combination of +/// runtime values — string+string, string+number, number+number — produces +/// exactly the result the dynamic path would have produced. The declaration +/// therefore selects a *lowering*, never an *answer*. +/// +/// The places that must NOT use it, and why: +/// - the `l ^ r` arm of `+` lowers through `js_string_concat_value` / +/// `js_value_concat_string`, which take an already-unboxed `StringHeader*` +/// and cannot tell a lie from a string; +/// - the N-way chain fold formats each part as a string, so an all-declared +/// chain of numbers would concatenate where the spec adds; +/// - the `Map` string-key fast paths in `expr::math_simple` key a lookup on +/// the claim. +/// +/// Each of those keeps [`is_definitely_string_expr`], which answers only for +/// expressions whose string-ness is structural (a literal, `String(x)`, +/// `.toString()`, `JSON.stringify`, …). +pub(crate) fn is_declared_string_expr(ctx: &FnCtx<'_>, e: &Expr) -> bool { + if is_definitely_string_expr(ctx, e) { + return true; + } + matches!( + e, + Expr::PropertyGet { .. } | Expr::Call { .. } | Expr::IndexGet { .. } + ) && matches!( + static_type_of(ctx, e), + Some(HirType::String) | Some(HirType::StringLiteral(_)) + ) +} + /// Resolve the declared type of `.` when `object` is a /// known user class or interface that declares (or inherits) a field /// named `field`. Returns `None` when the receiver isn't a tracked @@ -543,3 +587,6 @@ pub(crate) fn is_string_expr(ctx: &FnCtx<'_>, e: &Expr) -> bool { _ => false, } } + +#[cfg(test)] +mod tests; diff --git a/crates/perry-codegen/src/type_analysis/strings/tests.rs b/crates/perry-codegen/src/type_analysis/strings/tests.rs new file mode 100644 index 0000000000..4cccf1b5ac --- /dev/null +++ b/crates/perry-codegen/src/type_analysis/strings/tests.rs @@ -0,0 +1,163 @@ +//! cargo-test-visible coverage for the declaration-based string proof in +//! `is_definitely_string_expr` and the `type X = { … }` arm of +//! `static_type_of`. +//! +//! `"lit" + r.field` where `field` is DECLARED `string` must lower to the +//! static concat, not to `js_dynamic_string_or_number_add` — that helper opens +//! a `RuntimeHandleScope`, roots four operands and runs `ToPrimitive` on both +//! sides to rediscover what the declaration already stated. A field the +//! declaration does NOT describe must keep the dynamic helper. + +use crate::{compile_module, CompileOptions}; +use perry_hir::types::{ObjectType, PropertyInfo, Type}; +use perry_hir::{BinaryOp, Expr, Function, Module, ModuleInitKind, Param}; +use std::collections::HashMap; + +fn rec_alias() -> HashMap { + let mut properties = HashMap::new(); + properties.insert( + "kind".to_string(), + PropertyInfo { + ty: Type::String, + optional: false, + readonly: false, + }, + ); + properties.insert( + "amount".to_string(), + PropertyInfo { + ty: Type::Number, + optional: false, + readonly: false, + }, + ); + let mut aliases = HashMap::new(); + aliases.insert( + "Rec".to_string(), + Type::Object(ObjectType { + name: Some("Rec".to_string()), + properties, + property_order: None, + index_signature: None, + }), + ); + aliases +} + +/// `function probe(r: Rec): string { return "t:" + r.; }` +fn concat_probe_ir(property: &str) -> String { + let module = Module { + name: "alias_string_concat.ts".to_string(), + imports: Vec::new(), + exports: Vec::new(), + classes: Vec::new(), + interfaces: Vec::new(), + type_aliases: Vec::new(), + enums: Vec::new(), + globals: Vec::new(), + functions: vec![Function { + id: 1, + name: "probe".to_string(), + type_params: Vec::new(), + params: vec![Param { + id: 1, + name: "r".to_string(), + ty: Type::Named("Rec".to_string()), + default: None, + decorators: Vec::new(), + is_rest: false, + arguments_object: None, + }], + return_type: Type::String, + body: vec![perry_hir::Stmt::Return(Some(Expr::Binary { + op: BinaryOp::Add, + left: Box::new(Expr::String("t:".to_string())), + right: Box::new(Expr::PropertyGet { + object: Box::new(Expr::LocalGet(1)), + property: property.to_string(), + byte_offset: 0, + }), + }))], + is_async: false, + is_generator: false, + is_strict: false, + is_exported: false, + captures: Vec::new(), + decorators: Vec::new(), + was_plain_async: false, + was_unrolled: false, + }], + script_global_functions: Vec::new(), + references_global_this: false, + annexb_global_undefined_names: Vec::new(), + init: Vec::new(), + exported_native_instances: Vec::new(), + exported_func_return_native_instances: Vec::new(), + exported_objects: Vec::new(), + exported_functions: Vec::new(), + widgets: Vec::new(), + uses_fetch: false, + uses_webassembly: false, + extern_funcs: Vec::new(), + init_was_unrolled: false, + has_top_level_await: false, + init_kind: ModuleInitKind::Eager, + async_step_closures: std::collections::HashSet::new(), + closure_display_names: HashMap::new(), + class_display_names: HashMap::new(), + closure_source_text: HashMap::new(), + async_generator_funcs: std::collections::HashSet::new(), + gen_param_prologue_len: HashMap::new(), + }; + let opts = CompileOptions { + emit_ir_only: true, + output_type: "executable".to_string(), + type_aliases: rec_alias(), + ..Default::default() + }; + String::from_utf8(compile_module(&module, opts).unwrap()).expect("LLVM IR should be UTF-8") +} + +#[test] +fn alias_declared_string_field_takes_the_static_concat() { + let ir = concat_probe_ir("kind"); + assert!( + !ir.contains("call double @js_dynamic_string_or_number_add"), + "`\"t:\" + r.kind` with `kind` declared `string` on the object-type \ + alias must not re-derive the operand types at runtime:\n{ir}" + ); + assert!( + ir.contains("@js_string_concat"), + "it must lower to a string concat instead:\n{ir}" + ); +} + +#[test] +fn alias_declared_number_field_is_deliberately_not_routed() { + // The alias resolution added here is consumed by the STRING side only. + // `"t:" + r.amount` lands in the one-sided arm, which keeps the strict + // `is_definitely_string_expr` on the left and asks `is_numeric_expr` about + // the right — and `is_numeric_expr`'s `PropertyGet` arm answers from + // `ctx.classes` alone, on purpose: a `true` there means "this lowers to a + // REAL double", and the guarded class-field diamond's cold arm can hand + // back a NaN-boxed value that `fadd` would propagate rather than add + // (#7831). Widening the numeric side needs that PR's guarded lowering, + // not this one's runtime delegation, so it stays dynamic here. + let ir = concat_probe_ir("amount"); + assert!( + ir.contains("call double @js_dynamic_string_or_number_add"), + "the numeric side must NOT be widened by this change:\n{ir}" + ); +} + +#[test] +fn field_absent_from_the_alias_keeps_the_dynamic_add() { + // The proof is the DECLARATION, not the receiver's shape. A property the + // alias says nothing about is still type-unknown and must keep the + // spec-complete helper. + let ir = concat_probe_ir("undeclared"); + assert!( + ir.contains("call double @js_dynamic_string_or_number_add"), + "an undeclared property proves nothing and must stay dynamic:\n{ir}" + ); +} diff --git a/crates/perry-runtime/src/object/class_registry/gc_roots.rs b/crates/perry-runtime/src/object/class_registry/gc_roots.rs index 6d1b60d90f..c322e57906 100644 --- a/crates/perry-runtime/src/object/class_registry/gc_roots.rs +++ b/crates/perry-runtime/src/object/class_registry/gc_roots.rs @@ -516,7 +516,7 @@ pub(crate) fn test_clear_class_side_table_roots() { #[cfg(test)] pub(crate) fn test_seed_class_dynamic_prop_root(class_id: u32, name: &str, value_bits: u64) { - class_dynamic_prop_root_store(class_id, name.to_string(), f64::from_bits(value_bits)); + class_dynamic_prop_root_store(class_id, name, f64::from_bits(value_bits)); } #[cfg(test)] diff --git a/crates/perry-runtime/src/object/class_registry/prototype_methods.rs b/crates/perry-runtime/src/object/class_registry/prototype_methods.rs index f772fbf395..a4b67dd2b3 100644 --- a/crates/perry-runtime/src/object/class_registry/prototype_methods.rs +++ b/crates/perry-runtime/src/object/class_registry/prototype_methods.rs @@ -18,9 +18,8 @@ pub unsafe extern "C" fn js_class_register_static_field( if class_id == 0 || name_ptr.is_null() || name_len == 0 { return; } - let name = match std::str::from_utf8(std::slice::from_raw_parts(name_ptr, name_len)) { - Ok(s) => s.to_string(), - Err(_) => return, + let Ok(name) = std::str::from_utf8(std::slice::from_raw_parts(name_ptr, name_len)) else { + return; }; class_dynamic_prop_root_store(class_id, name, value); } diff --git a/crates/perry-runtime/src/object/class_registry/state.rs b/crates/perry-runtime/src/object/class_registry/state.rs index 85d065c7da..12c942c527 100644 --- a/crates/perry-runtime/src/object/class_registry/state.rs +++ b/crates/perry-runtime/src/object/class_registry/state.rs @@ -54,17 +54,57 @@ pub(crate) fn class_is_key_deleted(class_id: u32, key: &str) -> bool { }) } -pub(crate) fn class_dynamic_prop_root_store(class_id: u32, name: String, value: f64) { - CLASS_DELETED_KEYS.with(|m| { - if let Some(keys) = m.borrow_mut().get_mut(&class_id) { - keys.remove(&name); +/// Record `C. = value` in the class-ref side table that dynamic reads +/// (`const K: any = C; K.name`, `Object.keys(C)`, `getOwnPropertyDescriptor`) +/// consult, and shade the stored value for the incremental marker. +/// +/// Takes `&str`, not `String`: every caller had a value it did not own, so the +/// old signature forced an allocation on EVERY store — and then threw it away, +/// because `HashMap::insert` keeps the original key when one is already +/// present. That is the shape of a `static` counter: codegen emits this call +/// after each `Expr::StaticFieldSet`, so `Shape.made = Shape.made + 1` inside a +/// constructor runs it once per construction (144,000 times in +/// gc-handoff/apps/shapes.ts) and the key exists after the first. +/// +/// The in-place update also skips the `CLASS_DELETED_KEYS` probe — but only +/// when NO class key has ever been deleted, which is the state of essentially +/// every program (`delete C.x` on a class constructor is vanishingly rare). +/// Once anything has been deleted the original sequence runs verbatim, so the +/// interaction between a deleted PROTOTYPE key and a same-named static field +/// (`class C { m() {} static m = 1 }` — both land under one class_id) keeps +/// whatever behaviour it had. +pub(crate) fn class_dynamic_prop_root_store(class_id: u32, name: &str, value: f64) { + let nothing_deleted = CLASS_DELETED_KEYS.with(|m| m.borrow().is_empty()); + if nothing_deleted { + let updated = CLASS_DYNAMIC_PROPS.with(|m| { + match m + .borrow_mut() + .get_mut(&class_id) + .and_then(|props| props.get_mut(name)) + { + Some(slot) => { + *slot = value; + true + } + None => false, + } + }); + if updated { + crate::gc::runtime_write_barrier_root_nanbox(value.to_bits()); + return; } - }); + } else { + CLASS_DELETED_KEYS.with(|m| { + if let Some(keys) = m.borrow_mut().get_mut(&class_id) { + keys.remove(name); + } + }); + } CLASS_DYNAMIC_PROPS.with(|m| { m.borrow_mut() .entry(class_id) - .or_insert_with(std::collections::HashMap::new) - .insert(name, value); + .or_default() + .insert(name.to_string(), value); }); crate::gc::runtime_write_barrier_root_nanbox(value.to_bits()); } @@ -663,3 +703,57 @@ pub(crate) fn global_object_prototype_bits() -> Option { None } } + +#[cfg(test)] +mod class_dynamic_prop_store_tests { + use super::*; + + fn stored(class_id: u32, name: &str) -> Option { + class_own_static_field_value(class_id, name) + } + + /// The in-place update arm must be observationally identical to the + /// insert arm — same table, same value, same key set. This is the shape + /// `Shape.made = Shape.made + 1` produces once per construction. + #[test] + fn repeated_store_updates_in_place_and_stays_readable() { + let cid = 0x7c01_0001; + for i in 0..5u32 { + class_dynamic_prop_root_store(cid, "made", f64::from(i)); + assert_eq!(stored(cid, "made"), Some(f64::from(i))); + } + // A second key on the same class still inserts. + class_dynamic_prop_root_store(cid, "other", 9.0); + assert_eq!(stored(cid, "other"), Some(9.0)); + assert_eq!(stored(cid, "made"), Some(4.0)); + let mut keys = class_own_enumerable_field_names(cid); + keys.sort(); + assert_eq!(keys, vec!["made".to_string(), "other".to_string()]); + } + + /// The fast path is gated on "nothing has ever been deleted". Once a key + /// IS deleted, a re-store must still clear it from the deleted set — the + /// behaviour the unconditional probe used to provide. + #[test] + fn store_after_delete_clears_the_deleted_mark() { + let cid = 0x7c01_0002; + class_dynamic_prop_root_store(cid, "k", 1.0); + // Delete the way `delete C.k` does: drop the value AND mark the key. + class_delete_own_dynamic_prop(cid, "k"); + class_mark_key_deleted(cid, "k"); + assert!(class_is_key_deleted(cid, "k")); + assert_eq!(stored(cid, "k"), None); + + class_dynamic_prop_root_store(cid, "k", 2.0); + assert!( + !class_is_key_deleted(cid, "k"), + "re-storing a deleted static key must un-delete it" + ); + assert_eq!(stored(cid, "k"), Some(2.0)); + + // And a subsequent store, now on the slow arm (the deleted-keys map + // is non-empty for the whole process), still updates the value. + class_dynamic_prop_root_store(cid, "k", 3.0); + assert_eq!(stored(cid, "k"), Some(3.0)); + } +} diff --git a/crates/perry-runtime/src/object/field_set_by_name.rs b/crates/perry-runtime/src/object/field_set_by_name.rs index ca93ae6901..812798cffe 100644 --- a/crates/perry-runtime/src/object/field_set_by_name.rs +++ b/crates/perry-runtime/src/object/field_set_by_name.rs @@ -423,7 +423,7 @@ pub extern "C" fn js_object_set_field_by_name( "ERR_INVALID_ARG_TYPE", ); } - class_dynamic_prop_root_store(class_id, name, value); + class_dynamic_prop_root_store(class_id, &name, value); } } return; diff --git a/crates/perry-runtime/src/object/field_set_by_name/write_helpers.rs b/crates/perry-runtime/src/object/field_set_by_name/write_helpers.rs index 9257e77841..ceaa4ddffe 100644 --- a/crates/perry-runtime/src/object/field_set_by_name/write_helpers.rs +++ b/crates/perry-runtime/src/object/field_set_by_name/write_helpers.rs @@ -78,7 +78,7 @@ pub(super) unsafe fn mirror_class_object_static_write( let name_len = (*key).byte_len as usize; if let Ok(name) = std::str::from_utf8(std::slice::from_raw_parts(name_ptr, name_len)) { if !name.is_empty() && !name.starts_with("__perry_") { - class_dynamic_prop_root_store((*obj).class_id, name.to_string(), value); + class_dynamic_prop_root_store((*obj).class_id, name, value); } } } diff --git a/crates/perry-runtime/src/object/object_ops/define_property.rs b/crates/perry-runtime/src/object/object_ops/define_property.rs index fb76647dee..5a0d32d02d 100644 --- a/crates/perry-runtime/src/object/object_ops/define_property.rs +++ b/crates/perry-runtime/src/object/object_ops/define_property.rs @@ -540,7 +540,7 @@ pub extern "C" fn js_object_define_property( // lookup. super::super::class_registry::class_dynamic_prop_root_store( target_cid, - name.clone(), + &name, f64::from_bits(value_field.bits()), ); // A data descriptor is non-enumerable unless it diff --git a/crates/perry-runtime/src/string/concat.rs b/crates/perry-runtime/src/string/concat.rs index c4608f0d15..05410e536c 100644 --- a/crates/perry-runtime/src/string/concat.rs +++ b/crates/perry-runtime/src/string/concat.rs @@ -115,12 +115,36 @@ fn bytes_all_ascii(data: *const u8, len: u32) -> bool { .all(|&b| b < 0x80) } +/// SSO-aware pairwise `a + b` for two operands the codegen believes are +/// strings. Both operands arrive NaN-boxed so an SSO operand stays inline, and +/// the result is NaN-boxed too — SSO when the total fits five ASCII bytes, a +/// heap `StringHeader` otherwise. +/// +/// **A non-string operand is delegated, not treated as empty.** Perry does not +/// validate declared types at runtime, so "the codegen believes these are +/// strings" is a claim about an annotation, not about the bits. This function +/// used to `unwrap_or((null, 0))` such an operand, which silently rendered +/// `"ab" + 42` as `"ab"`; the codegen then had to route every possibly-lying +/// operand around it (see the `dother`/`cold` arms of the self-append lowering +/// in `lower_string_concat.rs`, whose comment says exactly that). Handing the +/// pair to [`js_dynamic_string_or_number_add`] instead gives a lie the full +/// spec answer — `ToPrimitive`, string concat when either side really is a +/// string, numeric add when neither is — so a static string proof is now a +/// PERFORMANCE claim that cannot change a program's output. +/// +/// [`js_dynamic_string_or_number_add`]: crate::value::js_dynamic_string_or_number_add #[no_mangle] pub extern "C" fn js_string_concat_box(l_value: f64, r_value: f64) -> f64 { let mut scratch_l = [0u8; crate::value::SHORT_STRING_MAX_LEN]; let mut scratch_r = [0u8; crate::value::SHORT_STRING_MAX_LEN]; - let l = str_bytes_from_jsvalue(l_value, &mut scratch_l).unwrap_or((std::ptr::null(), 0)); - let r = str_bytes_from_jsvalue(r_value, &mut scratch_r).unwrap_or((std::ptr::null(), 0)); + let (Some(l), Some(r)) = ( + str_bytes_from_jsvalue(l_value, &mut scratch_l), + str_bytes_from_jsvalue(r_value, &mut scratch_r), + ) else { + // `str_bytes_from_jsvalue` returns `None` for exactly the non-string + // values, so this is the annotation-lie arm and nothing else. + return unsafe { crate::value::js_dynamic_string_or_number_add(l_value, r_value) }; + }; let total_blen = l.1 + r.1; // SSO encodes its length tag as the JS `.length`, so it is only sound for diff --git a/crates/perry-runtime/src/string/tests.rs b/crates/perry-runtime/src/string/tests.rs index e5cf1abe55..72f7bf468e 100644 --- a/crates/perry-runtime/src/string/tests.rs +++ b/crates/perry-runtime/src/string/tests.rs @@ -525,3 +525,51 @@ fn string_compare_value_heap_and_sso_mixes() { assert_eq!(js_string_compare_value(heap("x"), undef), 1); assert_eq!(js_string_compare_value(undef, undef), 0); } + +// ── js_string_concat_box's non-string operand delegates ──────────────────── + +/// A `string`-declared operand that holds something else at runtime must get +/// the full dynamic `+`, not silently vanish. +/// +/// Perry does not validate declared types at runtime, so the codegen's +/// static string proof (`is_definitely_string_expr`) is a claim about an +/// ANNOTATION. This helper used to treat an operand it could not decode as +/// the empty string, which made `"ab" + 42` render as `"ab"` — a silent wrong +/// answer, and the reason the concat fast path had to be withheld from every +/// declaration-based proof. Delegating instead is what lets the proof be a +/// performance decision that cannot change a program's output. +#[test] +fn concat_box_delegates_a_non_string_operand_to_the_dynamic_add() { + let heap = |s: &str| { + let p = js_string_from_bytes(s.as_ptr(), s.len() as u32); + f64::from_bits(crate::value::JSValue::string_ptr(p).bits()) + }; + let text = |v: f64| { + let p = unsafe { crate::value::js_jsvalue_to_string(v) }; + let bytes = unsafe { std::slice::from_raw_parts(string_data(p), (*p).byte_len as usize) }; + String::from_utf8(bytes.to_vec()).expect("ascii") + }; + + // string + number → concatenation with the number's decimal form. + assert_eq!(text(js_string_concat_box(heap("ab"), 42.0)), "ab42"); + // number + string → same, other order. + assert_eq!(text(js_string_concat_box(42.0, heap("ab"))), "42ab"); + // Both operands lying: `+` is then plain numeric addition, and the result + // is a NUMBER, not a string. This is the arm the old `unwrap_or` answered + // with the empty string. + assert_eq!(js_string_concat_box(40.0, 2.0), 42.0); + // An int32-tagged operand must decode to its value, not to its boxed bits. + let int42 = f64::from_bits(crate::value::JSValue::int32(42).bits()); + assert_eq!(text(js_string_concat_box(heap("n="), int42)), "n=42"); + // undefined / null keep their ToString forms. + let undef = f64::from_bits(crate::value::JSValue::undefined().bits()); + assert_eq!(text(js_string_concat_box(heap("v:"), undef)), "v:undefined"); + + // The all-strings path is untouched, including the SSO result encoding. + let sso_result = js_string_concat_box(heap("a"), heap("b")); + assert_eq!(text(sso_result), "ab"); + assert!( + crate::value::JSValue::from_bits(sso_result.to_bits()).is_short_string(), + "a 2-byte ASCII result must still be assembled inline as SSO" + ); +} diff --git a/crates/perry-runtime/src/symbol/properties.rs b/crates/perry-runtime/src/symbol/properties.rs index 59a1a7d19d..32c967cb58 100644 --- a/crates/perry-runtime/src/symbol/properties.rs +++ b/crates/perry-runtime/src/symbol/properties.rs @@ -553,7 +553,7 @@ pub unsafe extern "C" fn js_class_register_static_symbol(class_id: u32, sym: f64 crate::value::JSValue::pointer(err as *const u8).bits(), )); } - crate::object::class_dynamic_prop_root_store(class_id, name.to_string(), value); + crate::object::class_dynamic_prop_root_store(class_id, name, value); return; } store_class_static_symbol_root(class_id, sym_key, value.to_bits());