fix(codegen,runtime): a declared string is not a proof, so it may not pick the + operator

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.

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<Param>, 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<Param>, 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}"
+    );
+}

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;

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<String> {
                 // 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);
                         }
                     }