diff --git a/changelog.d/7842-declared-string-is-not-a-runtime-proof.md b/changelog.d/7842-declared-string-is-not-a-runtime-proof.md new file mode 100644 index 0000000000..b90f3e9d3a --- /dev/null +++ b/changelog.d/7842-declared-string-is-not-a-runtime-proof.md @@ -0,0 +1,29 @@ +### Fixed + +**A declared `string` no longer picks the `+` operator (#7837).** `is_definitely_string_expr` answered `true` on the strength of an erased TypeScript annotation, and `+` then chose string concatenation from it. Perry does not enforce declared types at runtime — CLAUDE.md says so under Known Limitations — so `const s: string = (42 as any)` really does put a number in the slot. Thirteen shapes came out silently wrong, exit 0, no diagnostic; #7835 has since fixed four of them (the ones routed through `js_string_concat_box`, which it made total). These nine were still wrong on `ab1bd464b`: + +| shape | Node | before | +|---|---|---| +| `s + 7` | `49` | `427` — concat chosen where the spec adds | +| `7 + s` | `49` | `742` | +| `s + true` | `43` | `42true` | +| `a + b + "x"` (N-way fold) | `141x` | `4299x` | +| `a + b + a` | `183` | `429942` | +| `const u = s; u + 7` | `49` | `427` | +| `(c ? s : "q") + 7` | `49` | `427` | +| `arr.slice(0) + 7` | `1,27` | `` (empty) | +| `f(a: string, b: number)` through a function value | `49` | `427` | + +The last two are the same premise wearing different clothes. The `.toString()` / `.slice()` / `.replace()` … arm of the predicate matches on the **method name alone**, with no look at the receiver — `Array.prototype.slice` returns an array. And a `string` PARAMETER is live too: the first triage of this bug called parameters clean because a direct call gets inlined, which erases the annotation; reached through a function value the defect is there. + +The policy, matching #7831 on the numeric side: **a static type may select a lowering, never an answer.** It is applied in the one place each site can afford it. + +- **Helpers that receive both operands NaN-boxed can be made total, and #7835 did that**: `js_string_concat_box` forwards a non-string pair to `js_dynamic_string_or_number_add` rather than decoding it as the empty string. +- **The one-sided `l ^ r` arm could not be fixed that way**, because codegen unboxes the string operand to a `StringHeader*` before the call and the tag is gone by the time `js_string_concat_value` sees it. When the operand's string-ness is declared-only it is now passed NaN-boxed to `js_string_add_value` / `js_value_add_string`, which test the tag and then either run the identical fused single-allocation concat or fall through to the spec's `+`. +- **The N-way chain fold** formats every part as a string, so it reproduces the source tree only when the FIRST node really concatenates. It now requires a *proven* string in the head pair; a chain that fails that falls through to the pairwise lowering, which resolves each node from the runtime tags. + +A new predicate, `string_value_is_runtime_guaranteed`, separates the two kinds of evidence `is_definitely_string_expr` had been mixing: a literal, `String(x)`, `JSON.stringify`, `path.join`, `os.arch()` and friends *construct* a string, while a `LocalGet` and a receiver-blind method name only *claim* one. Its whitelist is deliberately closed — an arm nobody has classified answers "claim" and gets guarded, because that costs one predictable compare while the other default costs a wrong answer. + +**Cost: none measurable, and it is provable rather than sampled.** Compiling all 19 corpus programs with the base and the fixed compiler against the *same* runtime archives produced LLVM IR that differs by exactly two lines — the two `declare` statements for the new helpers. Zero call sites moved, zero folds were lost, and `"prefix" + i` keeps its fused concat because a literal is a proof. The guard lands only on reads the compiler could prove nothing about, and it lands as one compare inside a call that already allocates, so there is no codegen diamond and no phi for LLVM to lose an optimization to. + +Still open, filed as #7841: the `s += x` **self-append** lowering has the same defect from the same premise (`let c: string = (42 as any); c += 1` gives `"421"`, not `43`). It lives in `lower_string_self_append`, not in `binary::lower`, its fix has to move a tag test above a `ToString` that has observable side effects, and it sits on the load-bearing O(n) string-builder path — so it wants its own change and its own measurement rather than a rider on this one. diff --git a/crates/perry-codegen/src/codegen/declared_string_add_tests.rs b/crates/perry-codegen/src/codegen/declared_string_add_tests.rs new file mode 100644 index 0000000000..1d3a009f49 --- /dev/null +++ b/crates/perry-codegen/src/codegen/declared_string_add_tests.rs @@ -0,0 +1,313 @@ +//! #7837 — a declared `string` is not a proof that the value is a string, so +//! it may not pick the `+` OPERATOR. +//! +//! `is_definitely_string_expr`'s `LocalGet` arm trusts `let s: string`, and +//! Perry does not enforce annotations at runtime (CLAUDE.md, Known +//! Limitations). `const s: string = (42 as any); s + 7` therefore selected the +//! one-sided concat lowering and printed `"427"` where Node prints `49`. +//! +//! The one-sided arm is the one that cannot be repaired inside the runtime: +//! codegen unboxes the string operand to a `StringHeader*` before the call, so +//! `js_string_concat_value` never sees a tag to test. The fix hands it the +//! NaN-box instead (`js_string_add_value` / `js_value_add_string`), which is +//! why these tests assert on WHICH helper is emitted. +//! +//! Every test comes in a pair: the lie must be guarded, and the neighbouring +//! shape that carries a real proof must NOT be — a fix that routed everything +//! through the dynamic helper would pass the first half and fail the second, +//! and would have cost `"item_" + i` its fused single-allocation concat. + +use crate::{compile_module, CompileOptions}; +use perry_hir::types::Type; +use perry_hir::{BinaryOp, Expr, Function, Module, ModuleInitKind, Param, Stmt}; + +fn ir_opts() -> CompileOptions { + CompileOptions { + emit_ir_only: true, + output_type: "executable".to_string(), + ..Default::default() + } +} + +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 probe_fn(params: Vec, body: Expr) -> Function { + Function { + id: 1, + name: "probe".to_string(), + type_params: Vec::new(), + params, + return_type: Type::Any, + body: vec![Stmt::Return(Some(body))], + is_async: false, + is_generator: false, + is_strict: true, + was_plain_async: false, + was_unrolled: false, + is_exported: true, + captures: Vec::new(), + decorators: Vec::new(), + } +} + +fn module_with(function: Function) -> Module { + Module { + name: "declared_string_add.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], + 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: std::collections::HashMap::new(), + class_display_names: std::collections::HashMap::new(), + closure_source_text: std::collections::HashMap::new(), + async_generator_funcs: std::collections::HashSet::new(), + gen_param_prologue_len: std::collections::HashMap::new(), + } +} + +fn ir(params: Vec, body: Expr) -> String { + let module = module_with(probe_fn(params, body)); + String::from_utf8(compile_module(&module, ir_opts()).unwrap()).expect("LLVM IR is UTF-8") +} + +fn add(left: Expr, right: Expr) -> Expr { + Expr::Binary { + op: BinaryOp::Add, + left: Box::new(left), + right: Box::new(right), + } +} + +fn str_param() -> Param { + param(1, "s", Type::String) +} + +// ---------------------------------------------------------------- one-sided + +#[test] +fn declared_string_on_the_left_is_guarded() { + // `function probe(s: string) { return s + 7; }` + let ir = ir(vec![str_param()], add(Expr::LocalGet(1), Expr::Number(7.0))); + assert!( + ir.contains("call double @js_string_add_value("), + "a declared-only `string` operand must hand the NaN-box to the \ + tag-dispatching helper, or `s + 7` on a slot holding 42 prints \ + \"427\" instead of 49:\n{ir}" + ); + assert!( + !ir.contains("call i64 @js_string_concat_value("), + "...and must NOT also emit the pre-unboxed fused concat, whose \ + `StringHeader*` argument is exactly what loses the tag:\n{ir}" + ); +} + +#[test] +fn declared_string_on_the_right_is_guarded() { + // `function probe(s: string) { return 7 + s; }` + let ir = ir(vec![str_param()], add(Expr::Number(7.0), Expr::LocalGet(1))); + assert!( + ir.contains("call double @js_value_add_string("), + "the mirrored operand order needs the mirrored guard:\n{ir}" + ); + assert!( + !ir.contains("call i64 @js_value_concat_string("), + "the pre-unboxed fused concat must not survive alongside it:\n{ir}" + ); +} + +#[test] +fn a_string_literal_operand_keeps_the_fused_concat() { + // `function probe(n: number) { return "item_" + n; }` — the hot + // `"prefix" + i` shape. A literal IS a proof about the bits, so `+` is + // concat whatever `n` holds and there is nothing to test at runtime. + let ir = ir( + vec![param(1, "n", Type::Number)], + add(Expr::String("item_".to_string()), Expr::LocalGet(1)), + ); + assert!( + ir.contains("call i64 @js_string_concat_value("), + "a proven string must keep the fused single-allocation concat — the \ + guard is for claims, not for proofs:\n{ir}" + ); + assert!( + !ir.contains("call double @js_string_add_value("), + "and must pay no tag test at all:\n{ir}" + ); +} + +#[test] +fn a_coerced_operand_keeps_the_fused_concat() { + // `String(x) + n` — `js_string_coerce` always allocates a heap + // `StringHeader`, so this is a proof exactly like a literal. + let ir = ir( + vec![param(1, "n", Type::Number)], + add( + Expr::StringCoerce(Box::new(Expr::LocalGet(1))), + Expr::LocalGet(1), + ), + ); + assert!( + ir.contains("call i64 @js_string_concat_value(") + && !ir.contains("call double @js_string_add_value("), + "`String(x)` constructs a string; it is not an annotation:\n{ir}" + ); +} + +#[test] +fn a_string_method_on_a_proven_receiver_keeps_the_fused_concat() { + // `"ab".toUpperCase() + n`. The method-name arm is a proof only because + // the RECEIVER is one — see the next test for why that matters. + let ir = ir( + vec![param(1, "n", Type::Number)], + add( + Expr::Call { + callee: Box::new(Expr::PropertyGet { + object: Box::new(Expr::String("ab".to_string())), + property: "toUpperCase".to_string(), + byte_offset: 0, + }), + args: Vec::new(), + type_args: Vec::new(), + byte_offset: 0, + }, + Expr::LocalGet(1), + ), + ); + assert!( + !ir.contains("call double @js_string_add_value("), + "a string method on a string literal returns a string:\n{ir}" + ); +} + +#[test] +fn a_string_method_name_on_an_unproven_receiver_is_guarded() { + // `is_definitely_string_expr` matches `.slice(…)` on the METHOD NAME with + // no look at the receiver, so `arr.slice(0) + 7` claimed a string and + // printed "" — the array operand was decoded as an empty string. The name + // is a guess about the receiver's type, which is the same kind of evidence + // as an annotation. + let ir = ir( + vec![param(1, "a", Type::Any)], + add( + Expr::Call { + callee: Box::new(Expr::PropertyGet { + object: Box::new(Expr::LocalGet(1)), + property: "slice".to_string(), + byte_offset: 0, + }), + args: vec![Expr::Number(0.0)], + type_args: Vec::new(), + byte_offset: 0, + }, + Expr::Number(7.0), + ), + ); + assert!( + ir.contains("call double @js_string_add_value("), + "`Array.prototype.slice` returns an array; the name proves nothing \ + about the receiver:\n{ir}" + ); +} + +// -------------------------------------------------------------- chain fold + +#[test] +fn a_chain_whose_head_pair_is_all_declared_does_not_fold() { + // `s + t + "x"`. `js_string_concat_chain` formats EVERY part as a string, + // so it reproduces the source tree only when `s + t` really concatenates. + // With both holding numbers Node answers "141x"; the fold answers + // "4299x". + let ir = ir( + vec![str_param(), param(2, "t", Type::String)], + add( + add(Expr::LocalGet(1), Expr::LocalGet(2)), + Expr::String("x".to_string()), + ), + ); + assert!( + !ir.contains("call i64 @js_string_concat_chain("), + "the head pair carries no proof, so the N-way fold is unsound \ + here:\n{ir}" + ); +} + +#[test] +fn a_chain_led_by_a_literal_still_folds() { + // `"x" + s + t`. The first node concatenates whatever `s` holds, so its + // result is a string and every later `+` concatenates too — the fold is + // exact, and this is the CSV / log-line shape it exists for. + let ir = ir( + vec![str_param(), param(2, "t", Type::String)], + add( + add(Expr::String("x".to_string()), Expr::LocalGet(1)), + Expr::LocalGet(2), + ), + ); + assert!( + ir.contains("call i64 @js_string_concat_chain("), + "a proven string in the head pair keeps the N-way fold:\n{ir}" + ); +} + +#[test] +fn a_chain_whose_second_part_is_proven_still_folds() { + // `s + "," + t` — the proof may sit on either side of the first node. + let ir = ir( + vec![str_param(), param(2, "t", Type::String)], + add( + add(Expr::LocalGet(1), Expr::String(",".to_string())), + Expr::LocalGet(2), + ), + ); + assert!( + ir.contains("call i64 @js_string_concat_chain("), + "`s + \",\" + t` concatenates at every node whatever `s` holds:\n{ir}" + ); +} + +// ------------------------------------------------------- untouched tiers + +#[test] +fn two_numeric_operands_are_untouched() { + // The guard must not leak into arithmetic: `a + b` on two `number`s stays + // a bare `fadd` with no string helper anywhere near it. + let ir = ir( + vec![param(1, "a", Type::Number), param(2, "b", Type::Number)], + add(Expr::LocalGet(1), Expr::LocalGet(2)), + ); + assert!( + !ir.contains("call double @js_string_add_value(") + && !ir.contains("call double @js_value_add_string("), + "numeric `+` must not acquire a string guard:\n{ir}" + ); +} diff --git a/crates/perry-codegen/src/codegen/mod.rs b/crates/perry-codegen/src/codegen/mod.rs index 2b2b0fda53..08f012d09b 100644 --- a/crates/perry-codegen/src/codegen/mod.rs +++ b/crates/perry-codegen/src/codegen/mod.rs @@ -53,6 +53,8 @@ mod function; // (`inline_hot_small_enabled` / `inline_hot_small_hint_threshold`). #[cfg(test)] mod clone_suffix_tests; +#[cfg(test)] +mod declared_string_add_tests; pub(crate) mod helpers; mod method; mod method_registry; diff --git a/crates/perry-codegen/src/expr/binary.rs b/crates/perry-codegen/src/expr/binary.rs index e4ef92646c..e388ef8e7e 100644 --- a/crates/perry-codegen/src/expr/binary.rs +++ b/crates/perry-codegen/src/expr/binary.rs @@ -211,6 +211,27 @@ fn rebuild_add_tree( value } +/// May the flattened `p1 + p2 + … + pN` chain be handed to +/// `js_string_concat_chain`, which formats EVERY part as a string? (#7837) +/// +/// The fold reproduces the source tree `(((p1 + p2) + p3) …)` only when that +/// tree really is all-concat. Exactly one node can fail that: `p1 + p2`. If +/// either of those is genuinely a string the node concatenates, its result is +/// a string, and every later `+` concatenates too, whatever the later parts +/// hold. If neither is, the node may be a numeric ADD — and then +/// `const a: string = (42 as any), b: string = (99 as any); a + b + "x"` is +/// `"141x"` in Node while the fold prints `"4299x"`. +/// +/// So the head pair needs a proof, not an annotation. A chain that fails this +/// simply falls through to the pairwise lowering, where `js_string_concat_box` +/// resolves each node from the runtime tags. +fn chain_fold_is_sound(ctx: &FnCtx<'_>, parts: &[&Expr]) -> bool { + parts + .iter() + .take(2) + .any(|p| crate::type_analysis::string_value_is_runtime_guaranteed(ctx, p)) +} + fn lower_arithmetic_operand(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<(String, bool)> { // #6884: a statically typed numeric TypedArray read is Number|undefined, // not an unconditional raw f64. In arithmetic context the OOB `undefined` @@ -624,7 +645,7 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // shapes go through the existing pairwise paths. if l_is_str && r_is_str { if let Some(parts) = flatten_string_add_chain(ctx, left, right) { - if parts.len() >= 3 { + if parts.len() >= 3 && chain_fold_is_sound(ctx, &parts) { return lower_string_concat_chain(ctx, &parts); } } diff --git a/crates/perry-codegen/src/lower_string_concat.rs b/crates/perry-codegen/src/lower_string_concat.rs index e77deb57bc..e0ba1b0c3d 100644 --- a/crates/perry-codegen/src/lower_string_concat.rs +++ b/crates/perry-codegen/src/lower_string_concat.rs @@ -442,6 +442,23 @@ fn coerce_concat_body( // js_jsvalue_to_string + js_string_concat into a single allocation // for number operands (the common `"item_" + i` pattern). if l_is_string && !r_is_string { + // #7837: `l_is_string` may be nothing more than `let l: string`, and a + // declared type is not enforced at runtime. This arm chooses the + // OPERATOR, not just a representation — `s + 7` on a slot holding `42` + // must answer `49`, not `"427"` — and the strict lowering below cannot + // recover: it unboxes to a `StringHeader*` first, so the tag the + // decision needs is gone before the helper sees it. Hand the box over + // instead and let the helper pick. One predictable compare inside a + // call that already allocates; no codegen diamond, so the honest + // shape's fused single-allocation concat is untouched. + if crate::type_analysis::string_proof_is_declared_only(ctx, left) { + let blk = ctx.block(); + return Ok(blk.call( + DOUBLE, + "js_string_add_value", + &[(DOUBLE, l_box), (DOUBLE, r_box)], + )); + } // Issue #214: SSO-safe unbox; repsel Phase 3a: inline `bitcast+and` // for proven-heap operands (string literals — the `"user_" + i` // shape) and tag-dispatch for canonical-Str locals. @@ -456,6 +473,15 @@ fn coerce_concat_body( } if !l_is_string && r_is_string { + // #7837, mirrored: see the left-string arm above. + if crate::type_analysis::string_proof_is_declared_only(ctx, right) { + let blk = ctx.block(); + return Ok(blk.call( + DOUBLE, + "js_value_add_string", + &[(DOUBLE, l_box), (DOUBLE, r_box)], + )); + } // Issue #214: SSO-safe unbox; repsel Phase 3a: see above. let r_handle = str_operand_handle_tag_dispatched(ctx, right, r_box); let blk = ctx.block(); diff --git a/crates/perry-codegen/src/runtime_decls/strings.rs b/crates/perry-codegen/src/runtime_decls/strings.rs index 7473f7ea9b..d7e9289f6e 100644 --- a/crates/perry-codegen/src/runtime_decls/strings.rs +++ b/crates/perry-codegen/src/runtime_decls/strings.rs @@ -33,6 +33,14 @@ pub fn declare_phase_b_strings(module: &mut LlModule) { module.declare_function("js_string_concat_value", I64, &[I64, DOUBLE]); module.declare_function("js_value_concat_string", I64, &[DOUBLE, I64]); + // #7837: the same two fused concats, but with the STRING side passed + // NaN-boxed instead of pre-unboxed, so the helper can tell a real string + // from a `let s: string` that holds a number and pick the spec's operator. + // `js_string_add_value(l_box, r_box) -> box` (string believed on the left) + // `js_value_add_string(l_box, r_box) -> box` (string believed on the right) + module.declare_function("js_string_add_value", DOUBLE, &[DOUBLE, DOUBLE]); + module.declare_function("js_value_add_string", DOUBLE, &[DOUBLE, DOUBLE]); + // N-way string concat (v0.5.769): collapses a left-spine of pairwise // string-typed Add nodes (`a + b + c + ...`) into a single allocation. // First arg is a stack-allocated array of N NaN-boxed `f64` values; diff --git a/crates/perry-codegen/src/type_analysis.rs b/crates/perry-codegen/src/type_analysis.rs index ca9c506303..a328d8e708 100644 --- a/crates/perry-codegen/src/type_analysis.rs +++ b/crates/perry-codegen/src/type_analysis.rs @@ -59,6 +59,7 @@ pub(crate) use strings::{ 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, + string_proof_is_declared_only, string_value_is_runtime_guaranteed, }; #[cfg(test)] diff --git a/crates/perry-codegen/src/type_analysis/strings.rs b/crates/perry-codegen/src/type_analysis/strings.rs index b1e9e3c001..9c56737e56 100644 --- a/crates/perry-codegen/src/type_analysis/strings.rs +++ b/crates/perry-codegen/src/type_analysis/strings.rs @@ -329,9 +329,12 @@ pub(crate) fn is_definitely_string_expr(ctx: &FnCtx<'_>, e: &Expr) -> bool { /// - 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`, …). +/// Each of those keeps [`is_definitely_string_expr`] — but note that predicate +/// is NOT purely structural either, which is #7837: its `LocalGet` arm trusts +/// `let s: string`, and its `.toString()` / `.slice()` / … arm matches on the +/// method NAME with no look at the receiver. Use +/// [`string_value_is_runtime_guaranteed`] below when what you need is a claim +/// about the bits. pub(crate) fn is_declared_string_expr(ctx: &FnCtx<'_>, e: &Expr) -> bool { if is_definitely_string_expr(ctx, e) { return true; @@ -345,6 +348,149 @@ pub(crate) fn is_declared_string_expr(ctx: &FnCtx<'_>, e: &Expr) -> bool { ) } +/// Does this expression's string-ness say something about the BITS, or is it +/// just an erased TypeScript annotation repeated back? (#7837) +/// +/// `is_definitely_string_expr` mixes two very different kinds of evidence. +/// Most of its arms name a runtime entry point that CONSTRUCTS a string — +/// a literal, `String(x)`, `JSON.stringify`, `path.join`, `os.arch()`. Those +/// are proofs. Two are not: +/// +/// * **`LocalGet`** trusts `let s: string`, and Perry does not enforce +/// declared types at runtime (CLAUDE.md, Known Limitations). `const s: +/// string = (42 as any)` type-checks and puts a number in the slot. +/// * **the `.toString()` / `.slice()` / `.replace()` … arm** matches on the +/// METHOD NAME alone, with no look at the receiver. `Array.prototype.slice` +/// returns an array; `({toString(){return 5}}).toString()` returns a number. +/// +/// This answers `true` only for the first kind, so `+` can keep the concat +/// lowering where the string is real and dispatch on the tag where it is a +/// claim. The whitelist is deliberately **closed**: an arm nobody has thought +/// about answers `false` and gets guarded, which costs one predictable +/// compare, whereas defaulting the other way costs a silent wrong answer. +pub(crate) fn string_value_is_runtime_guaranteed(ctx: &FnCtx<'_>, e: &Expr) -> bool { + match e { + Expr::String(_) + | Expr::WtfString(_) + | Expr::StringCoerce(_) + | Expr::TypeOf(_) + | Expr::ArrayJoin { .. } + | Expr::JsonStringify(_) + | Expr::JsonStringifyPretty { .. } + | Expr::JsonStringifyFull(..) + | Expr::StringFromCodePoint(_) + | Expr::StringFromCharCode(_) + | Expr::StringFromCharCodeSpread(_) + | Expr::StringRaw { .. } + | Expr::FsReadFileSync(_) + | Expr::FsReadFileBinary(_) + | Expr::PathSep + | Expr::PathDelimiter + | Expr::PathJoin(..) + | Expr::PathDirname(_) + | Expr::PathBasename(_) + | Expr::PathExtname(_) + | Expr::PathResolve(_) + | Expr::PathNormalize(_) + | Expr::PathResolveJoin(..) + | Expr::PathWin32Join(..) + | Expr::PathToNamespacedPath(_) + | Expr::PathWin32 { + method: + perry_hir::PathWin32Method::ToNamespacedPath + | perry_hir::PathWin32Method::Dirname + | perry_hir::PathWin32Method::Basename + | perry_hir::PathWin32Method::BasenameExt + | perry_hir::PathWin32Method::Extname + | perry_hir::PathWin32Method::Normalize + | perry_hir::PathWin32Method::Format + | perry_hir::PathWin32Method::Relative + | perry_hir::PathWin32Method::Resolve + | perry_hir::PathWin32Method::ResolveJoin, + .. + } + | Expr::ProcessVersion + | Expr::ProcessCwd + | Expr::ProcessTitle + | Expr::OsArch + | Expr::OsType + | Expr::OsPlatform + | Expr::OsRelease + | Expr::OsHostname + | Expr::OsEOL + | Expr::OsDevNull + | Expr::OsEndianness + | Expr::OsMachine + | Expr::OsVersion => true, + // A string METHOD is a proof only when the receiver is one: every name + // in `is_definitely_string_expr`'s list also exists on `Array` or on a + // user object, where it returns something else entirely. + Expr::Call { callee, .. } => match callee.as_ref() { + Expr::PropertyGet { + object, property, .. + } if matches!( + property.as_str(), + "toString" + | "toLowerCase" + | "toUpperCase" + | "trim" + | "trimStart" + | "trimEnd" + | "slice" + | "substring" + | "substr" + | "charAt" + | "repeat" + | "replace" + | "replaceAll" + | "padStart" + | "padEnd" + | "concat" + | "normalize" + | "toFixed" + | "toPrecision" + | "toExponential" + ) => + { + string_value_is_runtime_guaranteed(ctx, object) + } + _ => false, + }, + // `a + b` really is a string whenever ONE side really is: the spec's + // `+` concatenates on either operand being a string, whatever the + // other holds. + Expr::Binary { + op: BinaryOp::Add, + left, + right, + } => { + string_value_is_runtime_guaranteed(ctx, left) + || string_value_is_runtime_guaranteed(ctx, right) + } + Expr::Conditional { + then_expr, + else_expr, + .. + } => { + string_value_is_runtime_guaranteed(ctx, then_expr) + && string_value_is_runtime_guaranteed(ctx, else_expr) + } + Expr::PropertyGet { + object, property, .. + } if is_process_namespace_version_property(object, property) => true, + _ => false, + } +} + +/// The expression reads as a string to `is_definitely_string_expr`, but the +/// only evidence is a declared type or a receiver-blind name guess (#7837). +/// +/// The string mirror of `numeric_proof_is_declared_only` (#7773/#7831), and +/// the same policy: **a static type selects a lowering, never an answer.** +pub(crate) fn string_proof_is_declared_only(ctx: &FnCtx<'_>, e: &Expr) -> bool { + is_definitely_string_expr(ctx, e) && !string_value_is_runtime_guaranteed(ctx, e) +} + /// 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 diff --git a/crates/perry-runtime/src/string/concat.rs b/crates/perry-runtime/src/string/concat.rs index 05410e536c..22f2d23116 100644 --- a/crates/perry-runtime/src/string/concat.rs +++ b/crates/perry-runtime/src/string/concat.rs @@ -765,6 +765,58 @@ pub extern "C" fn js_value_concat_string( js_string_concat(value_str, suffix_handle.get_raw_const_ptr::()) } +/// Resolve a value the caller has already established `is_any_string()` to a +/// raw `StringHeader*`, materialising SSO bits exactly the way the codegen's +/// `unbox_str_handle` does. +/// +/// The caller must pass the result straight into a helper that roots it — +/// nothing may allocate between the SSO materialisation and that root. +#[inline] +fn string_handle_of(value: f64) -> *const StringHeader { + let jsval = crate::value::JSValue::from_bits(value.to_bits()); + if jsval.is_string() { + return unsafe { jsval.as_string_ptr() }; + } + crate::value::js_get_string_pointer_unified(value) as *const StringHeader +} + +/// `l + r` where the codegen's only evidence that `l` is a string is a +/// DECLARED type (or a receiver-blind method-name guess) — #7837 defect 1. +/// +/// The fused `js_string_concat_value` cannot make this decision itself, +/// because codegen hands it an already-unboxed `StringHeader*` and the tag is +/// gone by then. So a declared-only operand is passed NaN-BOXED instead, and +/// the operator is chosen from the bits: +/// +/// * `l` really is a string → the identical fused single-allocation concat the +/// strict path emits, so an honest program pays one predictable compare +/// inside a call it was already making, and no codegen diamond at all; +/// * `l` is anything else → the spec's `+`, which is what +/// `const s: string = (42 as any); s + 7` must answer (`49`, not `"427"`). +/// +/// See [`js_value_add_string`] for the mirrored operand order. +#[no_mangle] +pub unsafe extern "C" fn js_string_add_value(l_value: f64, r_value: f64) -> f64 { + if crate::value::JSValue::from_bits(l_value.to_bits()).is_any_string() { + let handle = string_handle_of(l_value); + let out = js_string_concat_value(handle, r_value); + return crate::value::js_nanbox_string(out as i64); + } + crate::value::js_dynamic_string_or_number_add(l_value, r_value) +} + +/// `l + r` where the declared-only string operand is on the RIGHT — the mirror +/// of [`js_string_add_value`], guarding `js_value_concat_string` the same way. +#[no_mangle] +pub unsafe extern "C" fn js_value_add_string(l_value: f64, r_value: f64) -> f64 { + if crate::value::JSValue::from_bits(r_value.to_bits()).is_any_string() { + let handle = string_handle_of(r_value); + let out = js_value_concat_string(l_value, handle); + return crate::value::js_nanbox_string(out as i64); + } + crate::value::js_dynamic_string_or_number_add(l_value, r_value) +} + /// Fast integer-to-ASCII formatting into a provided buffer. /// Returns the number of bytes written. Digits are written to the END /// of the buffer and then shifted to the front. diff --git a/crates/perry-runtime/src/string/mod.rs b/crates/perry-runtime/src/string/mod.rs index 6df9e22ea0..4d5656de0b 100644 --- a/crates/perry-runtime/src/string/mod.rs +++ b/crates/perry-runtime/src/string/mod.rs @@ -73,8 +73,8 @@ pub(crate) use compare::{ js_string_key_bytes, js_string_key_matches, js_string_key_matches_bytes, utf16_cmp_bytes, }; pub use concat::{ - js_string_concat, js_string_concat_box, js_string_concat_chain, js_string_concat_value, - js_value_concat_string, + js_string_add_value, js_string_concat, js_string_concat_box, js_string_concat_chain, + js_string_concat_value, js_value_add_string, js_value_concat_string, }; pub(crate) use format::fix_exponent_format; pub(crate) use format::js_format_f64; diff --git a/crates/perry-runtime/src/string/tests.rs b/crates/perry-runtime/src/string/tests.rs index 72f7bf468e..eb8d83f1d6 100644 --- a/crates/perry-runtime/src/string/tests.rs +++ b/crates/perry-runtime/src/string/tests.rs @@ -573,3 +573,56 @@ fn concat_box_delegates_a_non_string_operand_to_the_dynamic_add() { "a 2-byte ASCII result must still be assembled inline as SSO" ); } + +// ---------------------------------------------------------------- #7837 + +/// NaN-box a heap string, the way codegen's `nanbox_string_inline` does. +fn boxed_heap(s: &str) -> f64 { + let h = js_string_from_bytes(s.as_ptr(), s.len() as u32); + crate::value::js_nanbox_string(h as i64) +} + +fn boxed_text(v: f64) -> String { + let p = crate::value::js_jsvalue_to_string(v); + string_as_str(p).to_string() +} + +#[test] +fn string_add_value_picks_the_operator_from_the_bits() { + // #7837 defect 1. `js_string_concat_value` takes an already-unboxed + // `StringHeader*`, so it cannot tell a lie from a string; these two take + // the NaN-box precisely so they can. + unsafe { + // A real string on the declared side concatenates, whatever the other + // operand holds — including a heap string, an SSO string and a number. + assert_eq!( + boxed_text(js_string_add_value(boxed_heap("ab"), 5.0)), + "ab5" + ); + assert_eq!( + boxed_text(js_value_add_string(5.0, boxed_heap("ab"))), + "5ab" + ); + let sso = js_string_new_sso(b"ab".as_ptr(), 2); + assert_eq!(boxed_text(js_string_add_value(sso, 5.0)), "ab5"); + assert_eq!(boxed_text(js_value_add_string(5.0, sso)), "5ab"); + assert_eq!( + boxed_text(js_string_add_value(boxed_heap("ab"), boxed_heap("cd"))), + "abcd" + ); + + // A LIE on the declared side is a numeric add, not a concatenation: + // `const s: string = (42 as any); s + 7` is 49 in Node, and was "427". + assert_eq!(js_string_add_value(42.0, 7.0), 49.0); + assert_eq!(js_value_add_string(7.0, 42.0), 49.0); + // ...and still concatenates when the OTHER operand is a real string. + assert_eq!( + boxed_text(js_string_add_value(42.0, boxed_heap("x"))), + "42x" + ); + assert_eq!( + boxed_text(js_value_add_string(boxed_heap("x"), 42.0)), + "x42" + ); + } +} diff --git a/test-files/test_gap_declared_string_local_holds_number_7837.ts b/test-files/test_gap_declared_string_local_holds_number_7837.ts new file mode 100644 index 0000000000..0bc93bab1f --- /dev/null +++ b/test-files/test_gap_declared_string_local_holds_number_7837.ts @@ -0,0 +1,127 @@ +// #7837 — an erased `string` annotation is not a proof that the slot holds a +// string, so it may not choose the `+` OPERATOR. +// +// Two silent wrong answers came out of the same premise. `const s: string = +// (42 as any); s + 7` printed "427" (concat chosen where the spec adds), and +// `const t: string = (99 as any); t + "x"` printed "x" (the operand was +// decoded as the empty string and vanished). +// +// Read this file as two halves. The `lie:` rows are the bug. The `honest:` +// rows are the reason the fix cannot simply route everything through the +// dynamic helper: a real string operand must still concatenate, and a real +// number pair must still add. A fix that coerced everything to a string would +// pass the first half and fail the second. + +const lie: any = 42; +const lie99: any = 99; + +// ---- lie: the declared local, one-sided (`string` + non-string) ---- +const a1: string = lie; +console.log("lie one-sided L", a1 + 7); +console.log("lie one-sided R", 7 + a1); +console.log("lie one-sided bool", a1 + true); +console.log("lie one-sided null", a1 + null); +console.log("lie one-sided undef", a1 + undefined); + +// ---- lie: the declared local, both operands (`string` + `string`) ---- +const a3: string = lie99; +console.log("lie pairwise L", a3 + "x"); +console.log("lie pairwise R", "x" + a3); +const b1: string = lie; +const b2: string = lie99; +console.log("lie pairwise both", b1 + b2); + +// ---- lie: the N-way chain fold ---- +// `js_string_concat_chain` formats every part as a string, so it reproduces +// the source tree only when the FIRST node really concatenates. +console.log("lie chain tail-lit", b1 + b2 + "x"); +console.log("lie chain no-lit", b1 + b2 + b1); +console.log("lie chain head-lit", "x" + b1 + b2); +console.log("lie chain mid-lit", b1 + "," + b2); + +// ---- lie: reached through an alias and through a ternary ---- +const e0: string = lie; +const e1 = e0; +console.log("lie alias", e1 + 7); +const i0: string = lie; +console.log("lie ternary", (true ? i0 : "q") + 7); + +// ---- lie: a receiver-blind method-name guess ---- +// `.slice(...)` is matched on the NAME, with no look at the receiver, so an +// array's `slice` claimed a string result and the operand disappeared. +const k0: any = [1, 2]; +console.log("lie method-name", k0.slice(0) + 7); + +// ---- lie: a declared `string` PARAMETER, kept away from the inliner ---- +// Calling `pf` directly gets it inlined, which erases the parameter's declared +// type and hides the defect — that is why the first triage of this bug +// concluded parameters were safe. Through a function value it survives. +function pf(a: string, b: number): any { + return a + b; +} +function pfr(a: string): any { + return a + "x"; +} +const viaValue: Array<(a: any, b: any) => any> = [pf as any, pfr as any]; +console.log("lie param pair", viaValue[0](lie, 7)); +console.log("lie param right", viaValue[1](lie99, 0)); + +// ---- honest: the declared local really holds a string ---- +const h1: string = "ab"; +const h2: number = 5; +console.log("honest local+num", h1 + h2); +console.log("honest num+local", h2 + h1); +console.log("honest local+lit", h1 + "cd"); +console.log("honest chain", h1 + h2 + "z"); +console.log("honest chain nolit", h1 + h1 + h1); +let acc: string = ""; +for (let i = 0; i < 4; i++) { + acc = acc + i; +} +console.log("honest build", acc); +console.log("honest method", h1.toUpperCase() + h2); + +// ---- honest: arithmetic must stay arithmetic ---- +const n1: number = 6; +const n2: number = 7; +console.log("honest add", n1 + n2); +console.log("honest add lit", n1 + 1); +console.log("honest typeof", typeof (n1 + n2), typeof (h1 + h2)); + +// ---- controls: the three shapes that were ALREADY correct ---- +// A declared field, an object property and a `(string, number)` parameter pair +// route elsewhere; they must not acquire the defect from this fix. +interface Rec { + t: string; + n: number; +} +const rLie: Rec = { t: lie as any, n: 1 }; +const rOk: Rec = { t: "q", n: 1 }; +console.log("control field lie", rLie.t + 7); +console.log("control field ok", rOk.t + 7); +const anyObj: any = { t: 42 }; +console.log("control prop lie", anyObj.t + 7); +class K { + t: string; + constructor(v: any) { + this.t = v; + } + m(): any { + return this.t + 7; + } +} +console.log("control classfield lie", new K(lie).m()); +console.log("control classfield ok", new K("q").m()); +const arrLie: string[] = [lie as any]; +const arrOk: string[] = ["q"]; +console.log("control elem lie", arrLie[0] + 7); +console.log("control elem ok", arrOk[0] + 7); +console.log("control param direct", pf("q", 9)); + +// ---- controls: proven-string producers keep concatenating ---- +console.log("proven String()", String(41) + 8); +console.log("proven stringify", JSON.stringify(41) + 8); +console.log("proven typeof", typeof lie + 8); +console.log("proven charcode", String.fromCharCode(65) + 8); +console.log("proven join", [1, 2].join("-") + 8); +console.log("proven method", "ab".slice(0) + 8);