From 2bb58a39ef62918d26fe8a0e8380a53ef2e94eeb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Tue, 11 Aug 2026 11:28:36 +0200 Subject: [PATCH 1/4] perf(codegen,runtime): let a declared `string` pick the concat lowering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `"shape:" + this.tag` — a string literal plus a field declared `string` — lowered to `js_dynamic_string_or_number_add`: a RuntimeHandleScope, four root_nanbox_f64s and two ToPrimitive calls spent rediscovering what the declaration already stated. 88.8 ns per concatenation; 29.4 ns after. Four changes: 1. `js_string_concat_box` forwards a non-string operand to `js_dynamic_string_or_number_add` instead of treating it as the empty string (`"ab" + 42` used to render as `"ab"`). This is a standalone silent-wrong-answer fix, and it is what makes (2) unable to change any program's output. 2. A new `is_declared_string_expr`, kept SEPARATE from `is_definitely_string_expr` because an annotation is evidence, not proof (#7831). Its only consumer is the two-operand concat, which emits `js_string_concat_box` — so after (1) the declaration selects a lowering and never an answer. The one-sided arm, the N-way chain fold and the Map string-key paths all keep the strict predicate; each could otherwise change a result. 3. `static_type_of` resolves `type X = { ... }` property types, as it already did for `interface X { ... }`. An object-type alias is structurally the same declaration; only the filing cabinet differed (`module.type_aliases` vs `module.interfaces`). 4. `class_dynamic_prop_root_store` takes `&str` and updates an existing key in place. Codegen emits `js_class_register_static_field` after every `Expr::StaticFieldSet`, so `Shape.made = Shape.made + 1` in a constructor allocated and dropped a `String` once per construction. Quiet M1 mini, best-of-7, outputs byte-identical to node 26.5.1 with exit 0: concat probe 0.2742 -> 0.1553, pipeline 0.5164 -> 0.4847, shapes 0.1894 -> 0.1833. No corpus regression; iso_miss canary clean under PERRY_GC_SCHEDULE_RATE=1, PERRY_GC_PROTECT_FROMSPACE and PERRY_GC_VERIFY_EVACUATION. Claude-Session: https://claude.ai/code/session_012B8z92S82sCfqCrVqrFgS2 --- ...ed-string-concat-and-static-field-store.md | 55 ++++++ crates/perry-codegen/src/expr/binary.rs | 18 +- crates/perry-codegen/src/type_analysis.rs | 6 +- .../src/type_analysis/predicates.rs | 24 +++ .../src/type_analysis/strings.rs | 47 +++++ .../src/type_analysis/strings/tests.rs | 163 ++++++++++++++++++ .../src/object/class_registry/gc_roots.rs | 2 +- .../class_registry/prototype_methods.rs | 5 +- .../src/object/class_registry/state.rs | 108 +++++++++++- .../src/object/field_set_by_name.rs | 2 +- .../object/field_set_by_name/write_helpers.rs | 2 +- .../src/object/object_ops/define_property.rs | 2 +- crates/perry-runtime/src/string/concat.rs | 28 ++- crates/perry-runtime/src/string/tests.rs | 48 ++++++ crates/perry-runtime/src/symbol/properties.rs | 2 +- 15 files changed, 491 insertions(+), 21 deletions(-) create mode 100644 changelog.d/7832-declared-string-concat-and-static-field-store.md create mode 100644 crates/perry-codegen/src/type_analysis/strings/tests.rs diff --git a/changelog.d/7832-declared-string-concat-and-static-field-store.md b/changelog.d/7832-declared-string-concat-and-static-field-store.md new file mode 100644 index 0000000000..ed0a59f413 --- /dev/null +++ b/changelog.d/7832-declared-string-concat-and-static-field-store.md @@ -0,0 +1,55 @@ +### 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"`. + +That was a live silent-wrong-answer hazard, and it was also the reason the concat fast path could never be selected from a type annotation: `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**. + +#### 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()); From 631f0fdb380f73cf8986d4073ea619c8204ab277 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Tue, 11 Aug 2026 11:31:33 +0200 Subject: [PATCH 2/4] chore: name the changelog fragment for PR #7835 Claude-Session: https://claude.ai/code/session_012B8z92S82sCfqCrVqrFgS2 --- ...e.md => 7835-declared-string-concat-and-static-field-store.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog.d/{7832-declared-string-concat-and-static-field-store.md => 7835-declared-string-concat-and-static-field-store.md} (100%) diff --git a/changelog.d/7832-declared-string-concat-and-static-field-store.md b/changelog.d/7835-declared-string-concat-and-static-field-store.md similarity index 100% rename from changelog.d/7832-declared-string-concat-and-static-field-store.md rename to changelog.d/7835-declared-string-concat-and-static-field-store.md From 37519b01ae774cc9e69c788b70400211368f760c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Tue, 11 Aug 2026 11:53:06 +0200 Subject: [PATCH 3/4] docs: the concat_box wrong answer is latent on main, not live MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codegen only selected js_string_concat_box when both operands satisfied the strict is_definitely_string_expr, so reaching it with a non-string required a lying `string`-declared local. A declared field and a (string, number) parameter pair both route elsewhere and answer correctly on 1ee158d27. The fix is still required — widening the operand test to accept a declaration is what would make the wrong answer reachable — but describing it as a live bug overclaimed. Claude-Session: https://claude.ai/code/session_012B8z92S82sCfqCrVqrFgS2 --- .../7835-declared-string-concat-and-static-field-store.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 index ed0a59f413..f54c1f2e38 100644 --- a/changelog.d/7835-declared-string-concat-and-static-field-store.md +++ b/changelog.d/7835-declared-string-concat-and-static-field-store.md @@ -18,7 +18,9 @@ No corpus regression: `churn` 0.4218→0.4242, `churn_alloc` 0.3733→0.3758, `p 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"`. -That was a live silent-wrong-answer hazard, and it was also the reason the concat fast path could never be selected from a type annotation: `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**. +**This is latent on the pre-change compiler, not a user-visible bug.** Codegen only selected this helper when both operands satisfied the strict `is_definitely_string_expr`, so a `"ab" + 42` reaching it required a lying *`string`-declared local* — the one arm of that predicate which is already a declaration rather than a structural proof. `o.t + 7` on a `string`-declared field, and a `(string, number)` parameter pair, both routed elsewhere and answered correctly. What made the fix necessary is change (2): widening the operand test to accept a declaration is exactly what would have made the wrong answer reachable. + +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**. #### 2. `+` accepts a declared `string`, but only where (1) makes that free From b3380b2a4a7fe63d8ef8ba44ed7055192fe46139 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Tue, 11 Aug 2026 11:57:01 +0200 Subject: [PATCH 4/4] docs: the concat_box wrong answer IS live; scope #7835 against #7837 Reverts the previous commit's framing. The empty-string result is reachable on 1ee158d27 through is_definitely_string_expr's LocalGet arm, which already trusts a declared type: const t: string = (99 as any); console.log(t + "x"); // node "99x", perry "x" Verified on a clean 1ee158d27 build and on this branch. A declared FIELD and a (string, number) parameter pair both route elsewhere and were already correct, which is why three negative probes read as absence. #7837 records two defects of the same premise. This PR fixes the dropped operand (defect 2) and NOT the wrong-operator selection (defect 1, `s + 7` printing 427 instead of 49), which lives in the one-sided arm and needs #7831-style guarded lowering rather than a runtime delegation. Claude-Session: https://claude.ai/code/session_012B8z92S82sCfqCrVqrFgS2 --- ...5-declared-string-concat-and-static-field-store.md | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) 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 index f54c1f2e38..06aaf10213 100644 --- a/changelog.d/7835-declared-string-concat-and-static-field-store.md +++ b/changelog.d/7835-declared-string-concat-and-static-field-store.md @@ -18,10 +18,19 @@ No corpus regression: `churn` 0.4218→0.4242, `churn_alloc` 0.3733→0.3758, `p 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 latent on the pre-change compiler, not a user-visible bug.** Codegen only selected this helper when both operands satisfied the strict `is_definitely_string_expr`, so a `"ab" + 42` reaching it required a lying *`string`-declared local* — the one arm of that predicate which is already a declaration rather than a structural proof. `o.t + 7` on a `string`-declared field, and a `(string, number)` parameter pair, both routed elsewhere and answered correctly. What made the fix necessary is change (2): widening the operand test to accept a declaration is exactly what would have made the wrong answer reachable. +**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.