fix(codegen): a declared numeric type is not a proof that the value is a number

changelog.d/7831-declared-numeric-type-is-not-a-proof.md

@@ -0,0 +1,48 @@
+### Fixed
+
+- **A declared numeric type is no longer treated as proof that the value is a
+  number** (#7773, #7776). Perry does not enforce annotations at runtime, but
+  codegen answered `is_numeric_expr` = `true` on the strength of one and then
+  emitted bare f64 arithmetic on whatever the slot actually held.
+
+  That is worse than producing a `NaN`, because arithmetic on a NaN-BOXED value
+  is not a no-op: `fadd`/`fmul` propagate the input NaN's payload, so a
+  NaN-boxed string comes back out of the instruction still tagged as that
+  string and flows on as if nothing happened — `typeof (v * 2)` answered
+  `"string"`. Four divergences from Node, all silent: `o.x + 1` gave `NaN`
+  where Node concatenates; `const v = o.x; v + 1` looked as though the `+ 1`
+  had evaporated; `v * 2` returned the string; and summing a `P[]` with one
+  `as any`-stored `Q` element gave `NaN`.
+
+  A new `numeric_proof_is_declared_only` separates "an annotation said so" from
+  a real proof. It is deliberately narrower than
+  `expr_may_return_boxed_value_from_raw_f64_fallback` (which answers "is there
+  a raw-f64 tier worth trying" and stays true for reads with no boxed fallback
+  at all): element-shape and class-field loop facts, `Ptr<Shape>` numeric
+  fields, scalar replacement, POD records and typed arrays all answer `false`
+  and keep their bare loads. `+` then lowers through an inline NaN-box tag test
+  — `fadd` on the fast arm, `js_dynamic_string_or_number_add` on the cold one —
+  because the spec's `+` dispatches on the runtime value; every other
+  arithmetic operator is a plain `ToNumber` and only needed the existing
+  residual-coerce rule taught to see a refined LOCAL.
+
+  `expr/mod.rs::lower_numeric_binary_value` turned out to be a second
+  arithmetic tier that bypasses `binary::lower` entirely and emits bare
+  `fadd`/`fmul` with no residual coerce at all; it was the path both
+  refined-local shapes took, and it now hands declared-only operands down the
+  same way its two existing `Mod` cases do.
+
+  Two details are load-bearing and are pinned by the test. The diamond covers
+  the whole `+` **tree**, not one node each: per-node diamonds make the outer
+  add of `s += o.x + 1` consume a phi that LLVM cannot prove is a canonical
+  double, which killed the `fadd` in the loop (+38% before fusing, +8.6%
+  after). And every leaf is tested except those `expr_produces_canonical_raw_f64`
+  vouches for — testing only the declared-only leaves skips the ACCUMULATOR,
+  which holds a string the moment this lowering's own cold arm concatenates,
+  and summed `16zw1113151719` down to `16zw`.
+
+  Measured on the quiet M1 mini, same runtime in both arms: element-shape clone
+  218 → 217 ms (−0.5%, untouched), `this.v + 1` in a method 70 → 76 ms (+8.6%),
+  `s += p.x + p.y` with an escaped receiver 196 → 263 ms (+34.2%). The cost
+  falls only on reads nothing could prove, which already pay an inline header
+  precheck or a `js_typed_feedback_class_field_get_guard` call.

crates/perry-codegen/src/codegen/closure.rs

@@ -978,6 +978,7 @@ pub(super) fn compile_closure(
         temp_roots: crate::rooting::TempRootPool::default(),
         shadow_slot_map,
         persistent_shadow_slots: std::collections::HashSet::new(),
+        declared_only_numeric_locals: std::collections::HashSet::new(),
         shadow_slot_clears_after_stmt,
         arena_state_slot: None,
         class_keys_slots: HashMap::new(),

crates/perry-codegen/src/codegen/entry.rs

@@ -816,6 +816,7 @@ pub(super) fn compile_module_entry(
             temp_roots: crate::rooting::TempRootPool::default(),
             shadow_slot_map: main_shadow_slot_map,
             persistent_shadow_slots: std::collections::HashSet::new(),
+            declared_only_numeric_locals: std::collections::HashSet::new(),
             shadow_slot_clears_after_stmt: main_shadow_slot_clears_after_stmt,
             arena_state_slot: None,
             class_keys_slots: HashMap::new(),
@@ -1485,6 +1486,7 @@ pub(super) fn compile_module_entry(
             temp_roots: crate::rooting::TempRootPool::default(),
             shadow_slot_map: init_shadow_slot_map,
             persistent_shadow_slots: std::collections::HashSet::new(),
+            declared_only_numeric_locals: std::collections::HashSet::new(),
             shadow_slot_clears_after_stmt: init_shadow_slot_clears_after_stmt,
             arena_state_slot: None,
             class_keys_slots: HashMap::new(),

crates/perry-codegen/src/codegen/function.rs

@@ -770,6 +770,7 @@ pub(super) fn compile_function(
         unsigned_i32_locals: native_facts.unsigned_i32_locals(),
         shadow_slot_map,
         persistent_shadow_slots: std::collections::HashSet::new(),
+        declared_only_numeric_locals: std::collections::HashSet::new(),
         shadow_slot_clears_after_stmt,
         shadow_slots_bound: bound_param_slots,
         temp_roots: crate::rooting::TempRootPool::default(),

crates/perry-codegen/src/codegen/method.rs

@@ -509,6 +509,7 @@ pub(super) fn compile_method(
         temp_roots: crate::rooting::TempRootPool::default(),
         shadow_slot_map,
         persistent_shadow_slots: std::collections::HashSet::new(),
+        declared_only_numeric_locals: std::collections::HashSet::new(),
         shadow_slot_clears_after_stmt,
         arena_state_slot: None,
         class_keys_slots: HashMap::new(),
@@ -1572,6 +1573,7 @@ pub(super) fn compile_static_method(
         temp_roots: crate::rooting::TempRootPool::default(),
         shadow_slot_map,
         persistent_shadow_slots: std::collections::HashSet::new(),
+        declared_only_numeric_locals: std::collections::HashSet::new(),
         shadow_slot_clears_after_stmt,
         arena_state_slot: None,
         class_keys_slots: HashMap::new(),

crates/perry-codegen/src/expr/binary.rs

@@ -18,7 +18,7 @@ use crate::native_value::{
 use crate::type_analysis::{
     add_operands_have_pod_materialization_hazard,
     expr_may_return_boxed_value_from_raw_f64_fallback, is_bigint_expr, is_bool_expr,
-    is_numeric_expr,
+    is_numeric_expr, numeric_proof_is_declared_only,
 };
 use crate::types::{DOUBLE, I1, I128, I32, I64};
 
@@ -51,6 +51,166 @@ fn lower_rooted_dynamic_binary(
     })
 }
 
+/// `+` where both operands are statically numeric but at least one of them is
+/// numeric only because a DECLARED type said so (#7773, #7776).
+///
+/// Nothing enforces annotations at runtime, so a `x: number` slot reached
+/// through `as any` really can hold a string — and then the spec says `+` is
+/// string concatenation, which is what Node does. Trusting the annotation cost
+/// two different wrong answers, both silent:
+///
+/// * `o.x + 1` produced `NaN`, because the number-context read's cold arm
+///   `js_number_coerce`s unconditionally; Node prints `s1`.
+/// * through a refined local the add did not even coerce — `fadd` on a
+///   NaN-BOXED value propagates the input payload on both AArch64 and x86-64,
+///   so the string came back out of the add still a string, and the `+ 1`
+///   looked like it had evaporated.
+///
+/// So re-check at runtime instead of assuming. The fast arm keeps the inline
+/// `fadd`; only a value that is not a canonical double reaches the dynamic
+/// helper, which is the one that implements the spec's `+`.
+///
+/// **The whole `+` TREE becomes one diamond, not one per node.** That is a
+/// correctness-neutral but performance-critical detail, and doing it the
+/// obvious way first is what showed why. Per-node diamonds make the outer add
+/// of `s += o.x + 1` consume a PHI, and LLVM cannot prove a phi over
+/// (`fadd`, runtime call) is a canonical double — so the outer test never
+/// folded, its cold arm stayed live in the loop, and `Acc.run`'s hot loop lost
+/// its `fadd` to an unconditional call. Measured on the bench mini that shape
+/// went 86 ms -> 119 ms. Fusing the tree removes the phi entirely: one test
+/// over the tree's violable LEAVES, one branch, then either all-`fadd` or
+/// all-`js_dynamic_string_or_number_add`.
+///
+/// Associativity is preserved rather than assumed away: both arms rebuild the
+/// ORIGINAL tree shape. `1 + (2 + "x")` is `"12x"` and `(1 + 2) + "x"` is
+/// `"3x"`, so a flattened re-association would be a wrong answer — the leaves
+/// are collected in evaluation order for rooting, but the arms are rebuilt
+/// node-for-node.
+///
+/// Every leaf is tested EXCEPT those that `expr_produces_canonical_raw_f64`
+/// vouches for (literals, `Math.*`, an explicit coerce, non-`+` arithmetic).
+/// Testing only the declared-only leaves is not enough, and the accumulator is
+/// the counter-example: `let s = 0; s += r.x + r.y` types `s` as `Number`, but
+/// the moment this very lowering's cold arm concatenates, `s` HOLDS A STRING
+/// while its static type still says otherwise. Skipping it summed
+/// `16zw1113151719` down to `16zw` — the fast arm `fadd`ed a NaN-boxed string
+/// and passed it through unchanged, which is the original bug reintroduced one
+/// level up. `expr_produces_canonical_raw_f64` declines to vouch for a
+/// `LocalGet` precisely because a local is a slot somebody can store into.
+///
+/// The residual cost lands where it is already small: every read that reaches
+/// here is one the compiler could NOT prove, so it pays an inline header
+/// precheck or a `js_typed_feedback_class_field_get_guard` call for its shape
+/// check regardless. The proven tiers (element-shape / class-field loop facts,
+/// `Ptr<Shape>` numeric fields, scalar replacement, POD records, typed arrays)
+/// never get here at all — `numeric_proof_is_declared_only` answers `false`.
+fn lower_declared_only_numeric_add(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
+    let mut leaves = Vec::new();
+    add_tree_leaves(expr, &mut leaves);
+    let needs_test: Vec<bool> = leaves
+        .iter()
+        .map(|leaf| !crate::type_analysis::expr_produces_canonical_raw_f64(ctx, leaf))
+        .collect();
+
+    with_operands_rooted(ctx, &leaves, |ctx, values| {
+        let mut cond: Option<String> = None;
+        for (value, is_tested) in values.iter().zip(needs_test.iter()) {
+            if !is_tested {
+                continue;
+            }
+            let is_num = crate::stmt::emit_js_value_is_number(ctx, value);
+            cond = Some(match cond {
+                Some(prev) => ctx.block().and(I1, &prev, &is_num),
+                None => is_num,
+            });
+        }
+        // The caller only routes here when a leaf is declared-only, and every
+        // such leaf is a field / element / local read — none of which
+        // `expr_produces_canonical_raw_f64` vouches for. So there is always at
+        // least one test; an empty condition would mean the two predicates had
+        // drifted apart, which is worth a hard error rather than a silent
+        // unguarded `fadd`.
+        let Some(all_num) = cond else {
+            anyhow::bail!(
+                "declared-only `+` tree has no testable leaf: \
+                 numeric_proof_is_declared_only and expr_produces_canonical_raw_f64 disagree"
+            );
+        };
+
+        let fast_idx = ctx.new_block("declared_add.numeric");
+        let slow_idx = ctx.new_block("declared_add.dynamic");
+        let merge_idx = ctx.new_block("declared_add.merge");
+        let fast_label = ctx.block_label(fast_idx);
+        let slow_label = ctx.block_label(slow_idx);
+        let merge_label = ctx.block_label(merge_idx);
+        ctx.block().cond_br(&all_num, &fast_label, &slow_label);
+
+        ctx.current_block = fast_idx;
+        let fast_val = rebuild_add_tree(ctx, expr, values, &mut 0, true);
+        let fast_end = ctx.block().label.clone();
+        ctx.block().br(&merge_label);
+
+        ctx.current_block = slow_idx;
+        let slow_val = rebuild_add_tree(ctx, expr, values, &mut 0, false);
+        let slow_end = ctx.block().label.clone();
+        ctx.block().br(&merge_label);
+
+        ctx.current_block = merge_idx;
+        Ok(ctx
+            .block()
+            .phi(DOUBLE, &[(&fast_val, &fast_end), (&slow_val, &slow_end)]))
+    })
+}
+
+/// The `+` tree's operand leaves, in evaluation order — a left-to-right walk,
+/// so `with_operands_rooted` lowers them in the order JS evaluates them.
+fn add_tree_leaves<'a>(expr: &'a Expr, out: &mut Vec<&'a Expr>) {
+    if let Expr::Binary {
+        op: BinaryOp::Add,
+        left,
+        right,
+    } = expr
+    {
+        add_tree_leaves(left, out);
+        add_tree_leaves(right, out);
+    } else {
+        out.push(expr);
+    }
+}
+
+/// Rebuild the `+` tree over already-lowered leaf values, node for node, so the
+/// original associativity survives. `fast` picks the inline `fadd`; otherwise
+/// every node goes through the spec-`+` helper.
+fn rebuild_add_tree(
+    ctx: &mut FnCtx<'_>,
+    expr: &Expr,
+    values: &[String],
+    next_leaf: &mut usize,
+    fast: bool,
+) -> String {
+    if let Expr::Binary {
+        op: BinaryOp::Add,
+        left,
+        right,
+    } = expr
+    {
+        let l = rebuild_add_tree(ctx, left, values, next_leaf, fast);
+        let r = rebuild_add_tree(ctx, right, values, next_leaf, fast);
+        return if fast {
+            ctx.block().fadd(&l, &r)
+        } else {
+            ctx.block().call(
+                DOUBLE,
+                "js_dynamic_string_or_number_add",
+                &[(DOUBLE, &l), (DOUBLE, &r)],
+            )
+        };
+    }
+    let value = values[*next_leaf].clone();
+    *next_leaf += 1;
+    value
+}
+
 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`
@@ -145,7 +305,17 @@ fn lower_arithmetic_operand(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<(String,
 fn operand_needs_residual_coerce(ctx: &FnCtx<'_>, expr: &Expr, fallback_coerced: bool) -> bool {
     !fallback_coerced
         && (!is_numeric_expr(ctx, expr)
-            || expr_may_return_boxed_value_from_raw_f64_fallback(ctx, expr))
+            || expr_may_return_boxed_value_from_raw_f64_fallback(ctx, expr)
+            // #7773: a local REFINED to `Number` from a declared field/element
+            // type is `is_numeric_expr`, but the hazard predicate above only
+            // knows how to look at reads, so `const v = o.x; v * 2` emitted a
+            // bare `fmul`. Arithmetic on a NaN-box preserves the payload, so
+            // that multiply returned the string unchanged — `typeof (v * 2)`
+            // answered `"string"`. Every non-`+` arithmetic operator is a plain
+            // `ToNumber` on its operands, so a coerce is the whole fix here;
+            // `+` needs the concat dispatch and gets it from
+            // `lower_declared_only_numeric_add`.
+            || matches!(expr, Expr::LocalGet(_)) && numeric_proof_is_declared_only(ctx, expr))
 }
 
 /// Lower an operand in number context: route through
@@ -519,6 +689,15 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
                         right,
                     );
                 }
+                // Both sides are statically numeric — but "statically" can mean
+                // "an annotation said so", and annotations are not enforced
+                // (#7773, #7776). Re-check the tag at runtime rather than
+                // emitting a bare `fadd` on a value that may be NaN-boxed.
+                if numeric_proof_is_declared_only(ctx, left)
+                    || numeric_proof_is_declared_only(ctx, right)
+                {
+                    return lower_declared_only_numeric_add(ctx, expr);
+                }
             }
             // BigInt arithmetic fast path. NaN-tagged bigints compare
             // unordered under `fadd`/`fsub`/`fmul`/`fdiv`/`frem` (the

crates/perry-codegen/src/expr/mod.rs

@@ -744,6 +744,23 @@ pub(crate) struct FnCtx<'a> {
     /// protected temporary this function lowers.
     pub temp_roots: crate::rooting::TempRootPool,
 
+    /// #7773: LocalIds whose `Number`/`Int32` type was REFINED from a read
+    /// whose own numeric answer is only a declared type — `const v = o.x` on a
+    /// `x: number` field, or `const e = arr[i]` on a `number[]`.
+    ///
+    /// The refinement is load-bearing (an un-annotated `const` is `Any` in the
+    /// HIR, so without it every ordinary field read loses the numeric fast
+    /// path), but it copies an annotation rather than proving anything. The
+    /// local then reads as `is_numeric_expr`, which licenses a bare `fadd` /
+    /// `fmul` on whatever the slot holds — and arithmetic on a NaN-boxed value
+    /// PRESERVES ITS PAYLOAD, so a string laundered in through `as any` came
+    /// back out of a multiply still tagged as a string (`typeof (v * 2)` was
+    /// `"string"`).
+    ///
+    /// Consumed by `type_analysis::numeric_proof_is_declared_only`, which turns
+    /// the trust into a four-instruction runtime tag test instead.
+    pub declared_only_numeric_locals: std::collections::HashSet<u32>,
+
     /// Cached pointer to this function's `InlineArenaState` slot —
     /// allocated lazily on the first `new ClassName()` site that uses
     /// the inline bump-allocator path. The slot lives in the function
@@ -2393,6 +2410,26 @@ fn lower_numeric_binary_value(
         return Ok(None);
     }
 
+    // #7773: `is_numeric_expr` answering `true` is not always a PROOF — for a
+    // class-field read, an array element, or a local refined from one, it is
+    // just the declared type repeated back, and nothing enforces declared types
+    // at runtime. This tier emits a bare `fadd`/`fmul` with no residual coerce
+    // at all, and arithmetic on a NaN-BOXED value propagates the payload
+    // instead of producing NaN — so a string laundered into a `x: number` slot
+    // came back out of `v * 2` still a string (`typeof` said `"string"`).
+    //
+    // Hand those to `binary::lower`, which has both remedies: the runtime tag
+    // test that keeps `+` on the spec's string-concat dispatch, and the
+    // residual `js_number_coerce` that gives every other operator its
+    // `ToNumber`. Same hand-off shape as the two Mod cases below, and for the
+    // same reason — it must run before operand lowering so an `Ok(None)` emits
+    // no dead loads or duplicate records.
+    if crate::type_analysis::numeric_proof_is_declared_only(ctx, left)
+        || crate::type_analysis::numeric_proof_is_declared_only(ctx, right)
+    {
+        return Ok(None);
+    }
+
     // Hand this proven shape to `binary::lower`, which owns the existing
     // integer remainder and negative-zero repair. This must run before operand
     // lowering so returning `None` emits no dead loads or duplicate records.

crates/perry-codegen/src/stmt/let_stmt.rs

@@ -290,6 +290,27 @@ pub(crate) fn lower_let(
         ty.clone()
     };
 
+    // #7773: the refinement above copies a DECLARED type — `const v = o.x` on a
+    // `x: number` field answers `Number` because the annotation says so, not
+    // because anything proved it. Nothing enforces annotations at runtime, so
+    // record the local as violable; `numeric_proof_is_declared_only` then makes
+    // arithmetic on it re-check the tag instead of trusting the type outright.
+    //
+    // Only the Any → numeric direction matters. A local the user DECLARED
+    // `number` is equally unenforced, but it is also the shape every honest
+    // program is made of; the refined case is the one where codegen invented
+    // the numeric claim itself, and it is the one both reported shapes need.
+    if matches!(ty, perry_hir::types::Type::Any)
+        && matches!(
+            refined_ty,
+            perry_hir::types::Type::Number | perry_hir::types::Type::Int32
+        )
+    {
+        if init.is_some_and(|e| crate::type_analysis::numeric_proof_is_declared_only(ctx, e)) {
+            ctx.declared_only_numeric_locals.insert(id);
+        }
+    }
+
     // Track closure func_id → local_id mapping so the closure
     // call site in lower_call can look up rest param info.
     if let Some(perry_hir::Expr::Closure {