fix(codegen,runtime): a declared string is not a proof, so it may not pick the + operator - #7842
Conversation
📝 WalkthroughWalkthroughDeclared ChangesDeclared string addition
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BinaryLowering
participant StringAnalysis
participant RuntimeAddHelpers
BinaryLowering->>StringAnalysis: check runtime string proof
BinaryLowering->>RuntimeAddHelpers: emit boxed addition helper call
RuntimeAddHelpers->>RuntimeAddHelpers: inspect operand runtime tags
RuntimeAddHelpers-->>BinaryLowering: return boxed string or numeric result
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ee67cd4 to
24eb1e1
Compare
…ot pick the `+` operator `is_definitely_string_expr` answered `true` on the strength of an erased TypeScript annotation, and `+` chose string concatenation from it. Perry does not enforce declared types at runtime, so `const s: string = (42 as any)` puts a number in the slot and thirteen shapes came out silently wrong, exit 0: `s + 7` printed "427" instead of 49, and `t + "x"` printed "x" — the operand was decoded as the empty string and vanished. The policy, matching #7831 on the numeric side: a static type may select a lowering, never an answer. Applied where each site can afford it. * `js_string_concat_box` becomes total: a non-string operand is delegated to `js_dynamic_string_or_number_add` instead of `unwrap_or((null, 0))`-ing to the empty string. (Same hunk as #7835; whichever lands second drops its copy.) * The one-sided `l ^ r` arm cannot be repaired that way — codegen unboxes to a `StringHeader*` before the call, so the tag is gone. A declared-only operand is now passed NaN-boxed to `js_string_add_value` / `js_value_add_string`, which test the tag and then run either the identical fused concat or the spec's `+`. * The N-way chain fold requires a proven string in its head pair, since it formats every part as a string and only reproduces the source tree when the first node really concatenates. `string_value_is_runtime_guaranteed` separates the two kinds of evidence the predicate had been mixing. Its whitelist is closed: an unclassified arm answers "claim" and gets guarded, which costs a compare rather than an answer. Compiling all 19 corpus programs with the base and fixed compilers against the same runtime archives yields LLVM IR differing by exactly two lines — the `declare`s for the new helpers. No call site moved. Refs #7837. Claude-Session: https://claude.ai/code/session_012B8z92S82sCfqCrVqrFgS2
24eb1e1 to
0b30df4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/perry-runtime/src/string/tests.rs (1)
590-628: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a non-number, non-string other operand.
The matrix covers heap strings, SSO strings, and numbers. It does not cover a real string on the declared side with
null,undefined, or an object on the other side. That input takesjs_string_concat_value's slow path throughjs_jsvalue_to_string, which can run usertoStringand collect. That path is the highest GC risk in the new helpers and it is currently unexercised through these entry points.Run this suite with
RUST_TEST_THREADS=1, as required forperry-runtimetests that share process-global runtime state. As per coding guidelines: "Runperry-runtimetests single-threaded withRUST_TEST_THREADS=1because they share process-global runtime state."🧪 Proposed additional assertions
assert_eq!( boxed_text(js_value_add_string(boxed_heap("x"), 42.0)), "x42" ); + // The other operand is neither a number nor a string, so the fused + // helper takes its `js_jsvalue_to_string` slow path. + assert_eq!( + boxed_text(js_string_add_value( + boxed_heap("ab"), + crate::value::JSValue::undefined().to_f64() + )), + "abundefined" + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/string/tests.rs` around lines 590 - 628, Add coverage to string_add_value_picks_the_operator_from_the_bits for a real declared-side string combined with a non-number, non-string value such as null, undefined, or an object, exercising the js_string_concat_value slow path through js_jsvalue_to_string and validating the resulting string. Run the perry-runtime test suite with RUST_TEST_THREADS=1.Source: Coding guidelines
crates/perry-codegen/src/codegen/declared_string_add_tests.rs (1)
206-209: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the positive assertion to pin the intended lowering.
This test asserts only that
js_string_add_valueis absent. That assertion also passes if the expression stops using the fused concat path entirely, for example if it routes tojs_dynamic_string_or_number_add. The sibling tests at lines 157-165 and 179-183 assert a positive and a negative together. Match that pattern so a regression that loses the fused concat fails here.♻️ Proposed additional assertion
assert!( + ir.contains("call i64 `@js_string_concat_value`("), + "a string method on a proven receiver must keep the fused \ + single-allocation concat:\n{ir}" + ); + assert!( !ir.contains("call double `@js_string_add_value`("), "a string method on a string literal returns a string:\n{ir}" );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-codegen/src/codegen/declared_string_add_tests.rs` around lines 206 - 209, Update the test around the existing js_string_add_value negative assertion to also positively assert the intended fused string-concatenation lowering, matching the sibling tests’ positive-and-negative pattern. Use the expected concat IR symbol so the test fails if lowering falls back to js_dynamic_string_or_number_add or another path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@changelog.d/7842-declared-string-is-not-a-runtime-proof.md`:
- Line 22: Correct the release-note text in the one-sided operator reference by
replacing the bitwise XOR symbol with the addition symbol; leave the surrounding
explanation unchanged.
---
Nitpick comments:
In `@crates/perry-codegen/src/codegen/declared_string_add_tests.rs`:
- Around line 206-209: Update the test around the existing js_string_add_value
negative assertion to also positively assert the intended fused
string-concatenation lowering, matching the sibling tests’ positive-and-negative
pattern. Use the expected concat IR symbol so the test fails if lowering falls
back to js_dynamic_string_or_number_add or another path.
In `@crates/perry-runtime/src/string/tests.rs`:
- Around line 590-628: Add coverage to
string_add_value_picks_the_operator_from_the_bits for a real declared-side
string combined with a non-number, non-string value such as null, undefined, or
an object, exercising the js_string_concat_value slow path through
js_jsvalue_to_string and validating the resulting string. Run the perry-runtime
test suite with RUST_TEST_THREADS=1.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5bfe58bd-6d7b-46af-9182-6f1f157df5d5
📒 Files selected for processing (12)
changelog.d/7842-declared-string-is-not-a-runtime-proof.mdcrates/perry-codegen/src/codegen/declared_string_add_tests.rscrates/perry-codegen/src/codegen/mod.rscrates/perry-codegen/src/expr/binary.rscrates/perry-codegen/src/lower_string_concat.rscrates/perry-codegen/src/runtime_decls/strings.rscrates/perry-codegen/src/type_analysis.rscrates/perry-codegen/src/type_analysis/strings.rscrates/perry-runtime/src/string/concat.rscrates/perry-runtime/src/string/mod.rscrates/perry-runtime/src/string/tests.rstest-files/test_gap_declared_string_local_holds_number_7837.ts
| 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 `+`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the operator in the release-note text.
The text reads "the one-sided l ^ r arm". ^ is the bitwise XOR operator. This entry describes the + arm. The fragment is assembled verbatim into the release notes, so correct it here.
✏️ Proposed fix
-- **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.
+- **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.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - **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 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 `+`. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@changelog.d/7842-declared-string-is-not-a-runtime-proof.md` at line 22,
Correct the release-note text in the one-sided operator reference by replacing
the bitwise XOR symbol with the addition symbol; leave the surrounding
explanation unchanged.
|
Correction to my own I wrote earlier that Perry's executables are not byte-deterministic. That was wrong, and the way it was wrong is a trap any A/B script walks straight into. Perry's output is byte-deterministic. But the ad-hoc code signature embeds the output basename, and the Mach-O UUID is content-derived over content that includes it. So:
Those 49 bytes are 16 (LC_UUID) + 33 (signature blob). Nothing else in the binary moves. Re-run with both arms written to the same basename in different directories, base compiler vs fixed compiler, same runtime archives: All 19 corpus executables are byte-identical. That is the conclusive direction — a content-derived UUID can manufacture a spurious difference but never a spurious identity — and it is a stronger statement than the IR argument, because it covers the linked machine code and not just the emitted IR. The IR comparison still stands and still explains why: the only delta is two Practical rule: hold the output basename constant across A/B arms. A |
1 participant
Closes #7837.
The bug
is_definitely_string_expransweredtrueon the strength of an erased TypeScript annotation, and+chose string concatenation from it. Perry does not enforce declared types at runtime — CLAUDE.md says so under Known Limitations — soconst s: string = (42 as any)really does put a number in the slot.I enumerated the arms of that predicate rather than probing spellings, and found 13 shapes that were silently wrong on
82f0e9681, exit 0, no diagnostic. #7835 has since landed and fixes 4 of them. Splitting the credit honestly, measured on purpose-built compilers for each of the three commits:82f0e9681ab1bd464b(with #7835)s + 749427427497 + s4974274249s + true4342true42true43a + b + "x"(N-way fold)141x4299x4299x141xa + b + a183429942429942183const u = s; u + 74942742749(c ? s : "q") + 74942742749arr.slice(0) + 71,271,27f(a: string, b: number)via a function value4942742749t + "x"99xx— operand vanished99x99x"x" + tx99xx99x99a + b(both declared)141141141pf(a: string)returninga + "x"99xx99x99xThe last four rows are #7835's — they route through
js_string_concat_box, which it made total. The nine above them are #7837 defect 1 and are still wrong onmaintoday: the wrong OPERATOR, chosen from an annotation.Two of the nine deserve a second look, because they show the premise is wider than "an annotation".
The
.toString()/.slice()/.replace()… arm matches on the METHOD NAME, with no look at the receiver.Array.prototype.slicereturns an array — soarr.slice(0) + 7claimed a string and the array was decoded as the empty string. A name is a guess about the receiver's type, which is evidence of exactly the same quality as an annotation.A
stringPARAMETER is live too. #7837 records a(string, number)parameter pair as already-correct, and it is — when called directly, because the inliner substitutes the argument and the annotation evaporates. Reached through a function value it is not, and that is what the row above measures. Three negative probes did not establish absence; nor did four.The policy
The same one #7831 is applying on the numeric side: a static type may select a lowering, never an answer. It is applied in the place each site can afford it, which is different for each.
Helpers that receive both operands NaN-boxed can be made total, and #7835 did that —
js_string_concat_boxno longer decodes a non-string operand as the empty string. That hunk was in this branch before the rebase and has been dropped: main has it, and it is the right fix for those four rows. Nothing to merge there.The one-sided
l ^ rarm cannot be repaired that way. Codegen unboxes the string operand to aStringHeader*before the call, so by the timejs_string_concat_valueruns there is no tag left to test. When the operand's string-ness is declared-only it is now passed NaN-boxed tojs_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 — once the accumulator is a string every later
+concatenates whatever its part holds. The fold 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 evidenceis_definitely_string_exprhad been mixing. A literal,String(x),JSON.stringify,path.join,os.arch()construct a string; aLocalGetand 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 proved rather than sampled
I did not have to trust a stopwatch for this. Compiling all 19 corpus programs with the base compiler and the fixed compiler against the same runtime archives isolates the codegen delta exactly, and the answer is available statically.
All 19 linked executables are byte-identical.
cmpon the binaries, base compiler vs fixed compiler, same runtime archives, same output basename: 19 identical, 0 differing. Identity is the conclusive direction — the Mach-O UUID is content-derived, so nondeterminism could manufacture a spurious difference but never a spurious identity. Identical bytes mean identical machine code, which means this change cannot have cost those programs a nanosecond.One trap worth recording, because it cost me a wrong conclusion first time round. Perry's output is byte-deterministic, but the ad-hoc code signature embeds the output basename, and the UUID is derived over content that includes it. So building the two arms to
b_churnandf_churn— the obvious thing for an A/B script to do — makes every program differ by exactly 49 bytes (16 UUID + 33 signature) on pure naming, and I briefly concluded from that that Perry's executables were not comparable at all. They are; build both arms to the same basename in different directories.churn.tsandfib40.tsboth rebuild identically under that discipline.The IR comparison (
--trace llvm) agrees and says a little more about why:All 19 programs' IR differs by exactly two lines — the
declares for the two new helpers, which emit no code:Zero call sites moved, zero blocks changed, zero folds lost — the two
declares emit no code, which is why the linked binaries come out identical. 15 of the 19 emit no string-concat call at all; the other four (interp,iso_miss,pipeline,shapes) still call exactly the same helpers they did before, because their concats are led by string literals and a literal is a proof.cat_localadditionally fails to link against the base runtime — undefinedjs_string_add_value— which is independent proof that the corpus never references the new symbols.There is no codegen diamond anywhere in this change, which is the deliberate difference from #7831's shape. On the numeric side the fast arm is a single
fadd, so a guard is expensive beside it and its phi blocks LLVM from proving the result is a canonical double — that is where #7831's +8.6% / +34.2% came from. Here the fast arm is a heap-allocating runtime call in every case, so the guard rides inside a call that was already happening and there is no phi to lose an optimisation to.Timing on the quiet M1 mini (load 1.3–1.9, bench lock held under a unique token and released with the guarded check, best-of-5, exit code recorded per cell, arms alternated within each rep; both arms built from the same worktree with the identical
-p perry -p perry-runtime-static -p perry-stdlib-staticpackage set, each linking its own runtime archives, so this measures the whole change and not just codegen). Base isab1bd464b— the branch has since been rebased twice more (onto0321c6554, which now carries #7831, #7832 and #7839); the A/B is a controlled comparison in which base and fix were built from the same commit, so later main-side speedups do not affect the delta. Every ceiling in the acceptance list is met, most of them by a wide margin now that #7833/#7834 have landed:Every cell exited 0 on both arms. Nothing is outside run-to-run noise, which is what the IR comparison predicted. (The same table measured before the rebase, against
82f0e9681, agreed to within 0.1 points on every row.)The cost, on a probe of the affected shape
Three probes in
gc-handoff/m0810/p7837/, each verified to exercise its subject by inspecting the emitted IR rather than assuming. Best-of-21, same session, lock held:cat_local—const s: string = parts[0]; s + i×2Mstrrecvtag diamond +js_string_concat_valuejs_string_add_valuecat_lit—"id-" + i×2Mjs_string_concat_valuecat_build—s = s + "ab"×2Mjs_string_append+1.05 ns per guarded concatenation is the honest price, and it is paid only where the compiler could prove nothing.
sis read from an array element specifically so constant propagation cannot fold it back to a literal and route the site around the guard;cat_local's loop does nothing but concatenate, so the guard's share of the work is as large as it can be — and it is still ~3%. Worth noting the fix actually removes IR here: thestrrecv.heap/strrecv.coldunbox diamond disappears, replaced by one call.For contrast with #7831, which is the same policy on the numeric side: its guard costs +8.6% on
this.v + 1and +34.2% on an escapeds += p.x + p.y. The asymmetry is structural, not a disagreement — see the note above aboutfaddversus an allocating call.Validation
test-files/test_gap_declared_string_local_holds_number_7837.ts— 41 assertions, byte-identical to Node 26.5.1. 13 of the rows are wrong on82f0e9681and right here. The other 28 are the reason the fix cannot just route everything to the dynamic helper: honest string concatenation, honest arithmetic,typeofon both, the three shapes A lyingstring-declared local silently drops an operand:t + "x"returns "x", ands + 7concatenates instead of adding #7837 lists as already-correct (declared field, object property, parameter pair), class fields, array elements, and six proven-string producers that must keep their fused concat.gc-handoff/m0810/lyinglocal.tsprints49then99x, byte-identical tonode --experimental-strip-types, exit 0.gc-handoff/apps/iso_miss.tsprintschecksum 437840 misses 0.crates/perry-codegen/src/codegen/declared_string_add_tests.rs) asserting on which helper is emitted — each paired with the neighbouring proven shape that must NOT be guarded.js_string_concat_box's totality.cargo test --release -p perry-codegen --no-fail-fast: 1301 passed, 1 failed —large_local_array_push_inbounds_store_emits_precise_slot_barrier, which is pre-existing (asserts onjs_write_barrier_slotplacement insideapush.inbounds.for a 2050-element array push; fix(codegen): a declared numeric type is not a proof that the value is a number #7831 verifiedBASE_EXIT=101on a clean-main build).cargo test --release -p perry-runtime --lib(RUST_TEST_THREADS=1): 2115 passed, 0 failed.cargo fmt --all -- --checkandscripts/check_file_size.shclean.What I could not run: the full gap suite. The dev host was at load 55–120 from other sessions and 20 GB free; a single gap test's compile was taking 3.5 minutes, which puts the 536-test suite at several hours, and the bench mini has 15 GB free — below the floor a parity run wants. I stopped the run at test 31/536 (2 failures, both already listed in
test-parity/gap_snapshot.json:test_gap_2159_defineproperty_class_prototype,test_gap_2514_settracesigint). Saying so plainly rather than implying coverage I do not have.Left for a follow-up, deliberately
s += xon a declaredstringlocal has the same defect from the same premise —let c: string = (42 as any); c += 1gives"421", not43— and it survives this PR. It is a different lowering (lower_string_self_append, reached fromLocalSet, not frombinary::lower), it sits on the load-bearing O(n) string-builder path, and fixing it correctly means hoisting a tag test above aToStringthat has observable side effects. That wants its own change and its own measurement rather than a rider on one that currently measures at zero. Filed as #7841, with the repro, the reason the fix is not a one-liner, and a suggested shape.