perf(codegen,runtime): let a declared string pick the concat lowering - #7835
Merged
Conversation
added 2 commits
August 11, 2026 11:28
`"shape:" + this.tag` — a string literal plus a field declared `string` — lowered to `js_dynamic_string_or_number_add`: a RuntimeHandleScope, four root_nanbox_f64s and two ToPrimitive calls spent rediscovering what the declaration already stated. 88.8 ns per concatenation; 29.4 ns after. Four changes: 1. `js_string_concat_box` forwards a non-string operand to `js_dynamic_string_or_number_add` instead of treating it as the empty string (`"ab" + 42` used to render as `"ab"`). This is a standalone silent-wrong-answer fix, and it is what makes (2) unable to change any program's output. 2. A new `is_declared_string_expr`, kept SEPARATE from `is_definitely_string_expr` because an annotation is evidence, not proof (#7831). Its only consumer is the two-operand concat, which emits `js_string_concat_box` — so after (1) the declaration selects a lowering and never an answer. The one-sided arm, the N-way chain fold and the Map string-key paths all keep the strict predicate; each could otherwise change a result. 3. `static_type_of` resolves `type X = { ... }` property types, as it already did for `interface X { ... }`. An object-type alias is structurally the same declaration; only the filing cabinet differed (`module.type_aliases` vs `module.interfaces`). 4. `class_dynamic_prop_root_store` takes `&str` and updates an existing key in place. Codegen emits `js_class_register_static_field` after every `Expr::StaticFieldSet`, so `Shape.made = Shape.made + 1` in a constructor allocated and dropped a `String` once per construction. Quiet M1 mini, best-of-7, outputs byte-identical to node 26.5.1 with exit 0: concat probe 0.2742 -> 0.1553, pipeline 0.5164 -> 0.4847, shapes 0.1894 -> 0.1833. No corpus regression; iso_miss canary clean under PERRY_GC_SCHEDULE_RATE=1, PERRY_GC_PROTECT_FROMSPACE and PERRY_GC_VERIFY_EVACUATION. Claude-Session: https://claude.ai/code/session_012B8z92S82sCfqCrVqrFgS2
📝 WalkthroughWalkthroughThe PR adds declaration-based string-concatenation lowering, dynamic coercion for non-string operands, object-type-alias property resolution, and in-place static-field updates with deleted-key handling. ChangesString Concatenation and Type Analysis
Runtime Concatenation
Static Field Storage
Estimated code review effort: 4 (Complex) | ~45 minutes 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 |
Codegen only selected js_string_concat_box when both operands satisfied the strict is_definitely_string_expr, so reaching it with a non-string required a lying `string`-declared local. A declared field and a (string, number) parameter pair both route elsewhere and answer correctly on 1ee158d. The fix is still required — widening the operand test to accept a declaration is what would make the wrong answer reachable — but describing it as a live bug overclaimed. Claude-Session: https://claude.ai/code/session_012B8z92S82sCfqCrVqrFgS2
Reverts the previous commit's framing. The empty-string result is reachable on 1ee158d through is_definitely_string_expr's LocalGet arm, which already trusts a declared type: const t: string = (99 as any); console.log(t + "x"); // node "99x", perry "x" Verified on a clean 1ee158d build and on this branch. A declared FIELD and a (string, number) parameter pair both route elsewhere and were already correct, which is why three negative probes read as absence. #7837 records two defects of the same premise. This PR fixes the dropped operand (defect 2) and NOT the wrong-operator selection (defect 1, `s + 7` printing 427 instead of 49), which lives in the one-sided arm and needs #7831-style guarded lowering rather than a runtime delegation. Claude-Session: https://claude.ai/code/session_012B8z92S82sCfqCrVqrFgS2
proggeramlug
marked this pull request as ready for review
August 11, 2026 10:20
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/perry-runtime/src/object/class_registry/state.rs (1)
77-102: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winScope the deleted-key fallback to
class_id.Line 77 disables the update fast path for every class after any class key is deleted.
CLASS_DELETED_KEYSis keyed byclass_id, and the required deleted-key interaction is only within the same class. Keep the global no-deletions fast path, but when it is false, check and clean deleted keys for the currentclass_idbefore selecting the slow path. This preserves deleted prototype/static-name handling without permanently degrading unrelated static-field updates.🤖 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/object/class_registry/state.rs` around lines 77 - 102, The deleted-key check in the class update logic must be scoped to the current class instead of treating any entry in CLASS_DELETED_KEYS as disabling the fast path globally. Preserve the global empty-map fast path, but when entries exist, inspect the deleted-key set for class_id and remove name only from that set before deciding whether to update CLASS_DYNAMIC_PROPS or use the slow path. Ensure unrelated classes retain the fast update behavior.changelog.d/7835-declared-string-concat-and-static-field-store.md (1)
1-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCondense this fragment into one release-note entry.
Keep the shipped behavior and user impact. Remove benchmark harness details, internal lowering names, and the separate unresolved-defect narrative. These details can become stale or conflict when release notes combine fragments.
Based on learnings, changelog fragments must “describe the final shipped behavior as one coherent release-note entry.”
🤖 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/7835-declared-string-concat-and-static-field-store.md` around lines 1 - 66, Condense the changelog entry into one coherent release note describing the shipped behavior and user impact: correct non-string handling for string concatenation, compile-time use of declared string types where safe, object-type alias property resolution, and more efficient static-field updates. Remove benchmark data, probe details, internal lowering/runtime symbol names, test implementation details, and the separate unresolved-defect discussion.Source: Learnings
🤖 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 `@crates/perry-codegen/src/type_analysis/strings/tests.rs`:
- Around line 1-3: Update the module documentation to refer to the
declaration-based predicate is_declared_string_expr instead of
is_definitely_string_expr, while leaving the static_type_of reference unchanged.
In `@crates/perry-runtime/src/string/concat.rs`:
- Around line 137-146: Update js_string_concat_box to root both l_value and
r_value in a RuntimeHandleScope before string_storage_alloc, then reload the
rooted handles and call str_bytes_from_jsvalue again after allocation before
reading lengths or copying bytes. Ensure no byte views obtained before
allocation are dereferenced afterward, and add a forced-GC test covering
concatenation of heap-backed strings.
---
Nitpick comments:
In `@changelog.d/7835-declared-string-concat-and-static-field-store.md`:
- Around line 1-66: Condense the changelog entry into one coherent release note
describing the shipped behavior and user impact: correct non-string handling for
string concatenation, compile-time use of declared string types where safe,
object-type alias property resolution, and more efficient static-field updates.
Remove benchmark data, probe details, internal lowering/runtime symbol names,
test implementation details, and the separate unresolved-defect discussion.
In `@crates/perry-runtime/src/object/class_registry/state.rs`:
- Around line 77-102: The deleted-key check in the class update logic must be
scoped to the current class instead of treating any entry in CLASS_DELETED_KEYS
as disabling the fast path globally. Preserve the global empty-map fast path,
but when entries exist, inspect the deleted-key set for class_id and remove name
only from that set before deciding whether to update CLASS_DYNAMIC_PROPS or use
the slow path. Ensure unrelated classes retain the fast update behavior.
🪄 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: 54e61f80-b57c-4fea-8f41-235ce6a7cf07
📒 Files selected for processing (15)
changelog.d/7835-declared-string-concat-and-static-field-store.mdcrates/perry-codegen/src/expr/binary.rscrates/perry-codegen/src/type_analysis.rscrates/perry-codegen/src/type_analysis/predicates.rscrates/perry-codegen/src/type_analysis/strings.rscrates/perry-codegen/src/type_analysis/strings/tests.rscrates/perry-runtime/src/object/class_registry/gc_roots.rscrates/perry-runtime/src/object/class_registry/prototype_methods.rscrates/perry-runtime/src/object/class_registry/state.rscrates/perry-runtime/src/object/field_set_by_name.rscrates/perry-runtime/src/object/field_set_by_name/write_helpers.rscrates/perry-runtime/src/object/object_ops/define_property.rscrates/perry-runtime/src/string/concat.rscrates/perry-runtime/src/string/tests.rscrates/perry-runtime/src/symbol/properties.rs
Comment on lines
+1
to
+3
| //! cargo-test-visible coverage for the declaration-based string proof in | ||
| //! `is_definitely_string_expr` and the `type X = { … }` arm of | ||
| //! `static_type_of`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the predicate name in the module documentation.
The declaration-based predicate is is_declared_string_expr, not is_definitely_string_expr. The current text conflicts with the implementation and can direct future changes to the unsafe predicate.
Proposed fix
-//! `is_definitely_string_expr` and the `type X = { … }` arm of
+//! `is_declared_string_expr` and the `type X = { … }` arm of📝 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.
Suggested change
| //! cargo-test-visible coverage for the declaration-based string proof in | |
| //! `is_definitely_string_expr` and the `type X = { … }` arm of | |
| //! `static_type_of`. | |
| //! cargo-test-visible coverage for the declaration-based string proof in | |
| //! `is_declared_string_expr` and the `type X = { … }` arm of | |
| //! `static_type_of`. |
🤖 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/type_analysis/strings/tests.rs` around lines 1 - 3,
Update the module documentation to refer to the declaration-based predicate
is_declared_string_expr instead of is_definitely_string_expr, while leaving the
static_type_of reference unchanged.
Comment on lines
137
to
+146
| pub extern "C" fn js_string_concat_box(l_value: f64, r_value: f64) -> f64 { | ||
| let mut scratch_l = [0u8; crate::value::SHORT_STRING_MAX_LEN]; | ||
| let mut scratch_r = [0u8; crate::value::SHORT_STRING_MAX_LEN]; | ||
| let l = str_bytes_from_jsvalue(l_value, &mut scratch_l).unwrap_or((std::ptr::null(), 0)); | ||
| let r = str_bytes_from_jsvalue(r_value, &mut scratch_r).unwrap_or((std::ptr::null(), 0)); | ||
| let (Some(l), Some(r)) = ( | ||
| str_bytes_from_jsvalue(l_value, &mut scratch_l), | ||
| str_bytes_from_jsvalue(r_value, &mut scratch_r), | ||
| ) else { | ||
| // `str_bytes_from_jsvalue` returns `None` for exactly the non-string | ||
| // values, so this is the annotation-lie arm and nothing else. | ||
| return unsafe { crate::value::js_dynamic_string_or_number_add(l_value, r_value) }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Root both operands across string_storage_alloc.
str_bytes_from_jsvalue returns byte views backed by movable heap strings. string_storage_alloc at Line 176 can collect. The reads from l.0 and r.0 at Lines 183-223 can then dereference moved storage.
Root l_value and r_value in a RuntimeHandleScope before allocation. After allocation, reload both handles and decode the byte views again before computing lengths or copying bytes. Add a forced-GC test with heap strings.
Based on learnings, a byte view from str_bytes_from_jsvalue is invalid across allocation or GC for non-SSO strings.
🤖 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/concat.rs` around lines 137 - 146, Update
js_string_concat_box to root both l_value and r_value in a RuntimeHandleScope
before string_storage_alloc, then reload the rooted handles and call
str_bytes_from_jsvalue again after allocation before reading lengths or copying
bytes. Ensure no byte views obtained before allocation are dereferenced
afterward, and add a forced-GC test covering concatenation of heap-backed
strings.
Source: Learnings
Contributor
Author
|
Heads-up from #7842 (closing #7837): this PR's I did not intend to duplicate it — #7837's brief told me it was yours and not to redo it — but the acceptance criterion for #7842 is that Whichever of us lands second should drop their copy — it is a ~20-line One correction to this PR's framing, now that #7837 has been fully enumerated. The doc comment on
#7842 adds No conflict otherwise: your |
proggeramlug
pushed a commit
that referenced
this pull request
Aug 11, 2026
…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
proggeramlug
pushed a commit
that referenced
this pull request
Aug 11, 2026
…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
proggeramlug
pushed a commit
that referenced
this pull request
Aug 11, 2026
…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
proggeramlug
pushed a commit
that referenced
this pull request
Aug 11, 2026
…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
proggeramlug
added a commit
that referenced
this pull request
Aug 11, 2026
…ot pick the `+` operator (#7842) `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 Co-authored-by: Ralph Küpper <ralph@skelpo.com>
This was referenced Aug 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The finding
gc-handoff/apps/shapes.ts'sdescribe()is"shape:" + this.tag— a string literal plus a field declaredstring. It lowered tojs_dynamic_string_or_number_add: aRuntimeHandleScope, fourroot_nanbox_f64s and twoToPrimitivecalls, spent rediscovering at runtime what the declaration already stated.is_string_exprhas trusted that same declaration for string method dispatch since #655. The concat path did not.88.8 ns per concatenation → 29.4 ns. (
gc-handoff/bench/concat_field.tsminusconcat_field_base.ts, 2,000,000 concatenations, quiet M1 mini, best-of-7.)Why widening the predicate is safe, and why it is scoped so tightly
Perry does not enforce annotations at runtime — the exact gap #7831 is closing on the numeric side, where a declared-
numberfield holding a string madefaddpropagate the string's NaN payload andtypeof (v * 2)answer"string". A declaration is evidence, not proof.So this PR does not widen
is_definitely_string_expr. It adds a separateis_declared_string_exprwith exactly one consumer: the two-operanda + bconcat, which emitsjs_string_concat_box.And it first fixes that helper.
js_string_concat_boxdecoded operands withstr_bytes_from_jsvalue(...).unwrap_or((null, 0))— an operand that was not a string became the empty string, so"ab" + 42through this helper rendered as"ab". This is reachable on1ee158d27today — tracked as #7837.is_definitely_string_expr'sLocalGetarm already trusts a declared type, so astring-declared local holding a non-string selects this helper and the operand disappears:Measured on a clean
1ee158d27build and on this branch. Astring-declared field (o.t + 7) and a(string, number)parameter pair both route elsewhere and were already correct on both builds, which is why the defect survives casual probing — three negative probes of the wrong shapes do not establish absence.It is also why the concat fast path could never be selected from an annotation in the first place:
lower_string_concat.rs's self-append lowering carries a wholedother/coldarm whose comment says a lie has to be routed around this helper. It now forwards any non-string pair tojs_dynamic_string_or_number_add.Scope against #7837 — this PR fixes one of the two defects
#7837 records two silent wrong answers from the same premise (an erased TS annotation treated as a runtime proof — the string mirror of #7831). Do not read this PR as closing both.
t + "x", operand dropped99xx99x— fixed heres + 7, wrong operator chosen49427427— not fixed hereDefect 1 lives in the one-sided
l ^ rarm, which lowers throughjs_string_concat_value/js_value_concat_string. Those take an already-unboxedStringHeader*and cannot detect the lie, so fixing it needs the guarded-diamond treatment #7831 is applying to the numeric side, not this PR's runtime delegation. That arm is deliberately left on the strict predicate here and is untouched; the decision belongs to #7837.Worth noting for that decision: scoping
is_declared_string_exprto the two-operand path is what keeps this PR from widening defect 1. Had the declaration been accepted in the one-sided arm,o.t + 7— correct at14on both builds today — would have become"77".After that,
js_string_concat_boxreturns the dynamic-path answer for every combination of runtime values — string+string, string+number, and number+number (which adds and returns a number). The declaration therefore selects a lowering and can never select an answer.Three neighbours deliberately keep the strict predicate, each because it would be able to change an answer:
l ^ rarm lowers throughjs_string_concat_value/js_value_concat_string, which take an already-unboxedStringHeader*and cannot tell a lie from a string;Mapstring-key fast paths inexpr::math_simplekey a lookup on the claim.type X = { … }was worth less thaninterface X { … }lower_type_alias_declfiles aliases inmodule.type_aliases;lower_interface_declfiles interfaces inmodule.interfaces.static_type_ofonly ever consulted the latter. Sotype Record = { kind: string; amount: number }proved nothing aboutr.kind— not because an alias is weaker evidence than an interface (structurally they are the same declaration, the same runtime layout, the same absence of any layout guarantee), but because of which cabinet it was filed in. Only a non-generic alias whose right-hand side is a closed object type answers.This is what lets
pipeline.ts'smakeTagger(prefix + r.kind, 360,000 calls) reach the concat at all.A
staticcounter re-interned its own key, once per constructionCodegen emits
js_class_register_static_fieldafter everyExpr::StaticFieldSet, soShape.made = Shape.made + 1inside a constructor runs it once per construction — 144,000 times inshapes.ts. Each call didstr::from_utf8(…).to_string()(a heap allocation, immediately dropped, becauseHashMap::insertkeeps the original key), aCLASS_DELETED_KEYSprobe, and anentry().or_insert_with().insert().The signature is now
&str— every caller already had a borrowed value — and a store whose key exists updates the slot in place. The in-place arm skips the deleted-keys probe only while nothing has ever been deleted; once anything has, the original sequence runs verbatim, so the pre-existing conflation between a deleted prototype key and a same-named static field (class C { m() {} static m = 1 }, both under one class_id) keeps whatever behaviour it had.Measurements
Quiet M1 mini, load ~1.6, best-of-7 (best-of-5 for the wider set), against a clean build of
1ee158d27built with the same-p perry -p perry-runtime-static -p perry-stdlib-staticpackage set and an isolatedCARGO_TARGET_DIR. Every program's output verified byte-identical tonode --experimental-strip-types(26.5.1) with exit 0 before timing; the harness records an exit code per cell.concat_field(new probe)concat_field_base(its subtrahend)apps/pipeline.tsapps/shapes.tsNo corpus regression (best-of-5):
churn0.4216→0.4234,churn_alloc0.3729→0.3752,churn_read0.0228→0.0224,push_num0.1507→0.1511,push_cls0.3658→0.3681,cycles0.1938→0.1938,deeplist0.1239→0.1232,tree1.6419→1.6425,tree_wide2.1132→2.1097,retain0.3477→0.3464,retain10.1367→0.1359,retain_wide0.4552→0.4569,fib400.3997→0.3948,asyncpipe0.1345→0.1349,interp1.4976→1.4971,iso_miss1.9256→1.9260.The
iso_misscanary printschecksum 437840 misses 0and exits 0 on the miss counter, including underPERRY_GC_SCHEDULE_RATE=1,PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800, andPERRY_GC_VERIFY_EVACUATION=1.shapes,pipelineandconcat_fieldare also byte-correct underPERRY_GC_SCHEDULE_RATE=1 PERRY_GC_VERIFY_EVACUATION=1.Tests
All in-crate unit tests, so they run in the per-PR
cargo-testjob rather than the nightly-only integration suites (#5960).perry-runtime:concat_box_delegates_a_non_string_operand_to_the_dynamic_addpins all four operand combinations (including number+number returning a number) plus the SSO result encoding;repeated_store_updates_in_place_and_stays_readableandstore_after_delete_clears_the_deleted_markpin both arms of the static-field store.perry-codegen:alias_declared_string_field_takes_the_static_concat,field_absent_from_the_alias_keeps_the_dynamic_add, andalias_declared_number_field_is_deliberately_not_routed— the last asserts the numeric side is unchanged, so widening it later has to be a deliberate edit to this test.Local runs on this branch's build:
perry-runtime --lib2108 passed / 0 failed (RUST_TEST_THREADS=1),perry-codegen --lib858 passed / 0 failed after the test correction. Gap suite in progress locally at time of writing; through 290/535 on the pre-narrowing build the only failures were the 8 already recorded intest-parity/gap_snapshot.json(2parity_fail, 6node_fail) — zero new.Probes added
gc-handoff/bench/concat_field.ts/concat_field_base.ts(the ns/concat pair) live in the out-of-repo handoff tree, not in this PR.https://claude.ai/code/session_012B8z92S82sCfqCrVqrFgS2
Summary by CodeRabbit
Bug Fixes
undefined, or other non-string values.Tests