fix(runtime): #5591 — JSON/String test262 compliance fixes - #5818
Closed
proggeramlug wants to merge 3 commits into
Closed
fix(runtime): #5591 — JSON/String test262 compliance fixes#5818proggeramlug wants to merge 3 commits into
proggeramlug wants to merge 3 commits into
Conversation
… body to its global completion value Residual of the #5579 regression batch: the `language/eval-code/indirect` completion-value cluster. Indirect eval `(0, eval)('<const>')` runs as global code, but Perry's general indirect-eval path deferred every valid body to the runtime global-`eval` thunk, which only models the `this`/`globalThis` idiom and returns `undefined` for everything else. So `(0,eval)('x = 1')` yielded undefined instead of 1 and never mutated the global `x` — diverging from the script-mode Node oracle (`vm.runInThisContext`, #5346/#5511). Perry already models a constant *direct* eval body with a scope-capturing completion IIFE (#1679). For indirect eval that IIFE is only sound where the captured enclosing scope already IS the global scope: module top level (`scope_depth == 0`, no enclosing class / `with` / ESM) under `PERRY_GLOBAL_SCRIPT_THIS`, where module-top `var`s and `this` are the global bindings (#5608/#5609). There `try_indirect_eval_general` now folds the body via the shared `build_eval_completion_iife` (sloppy unless the body opens with its own Use Strict Directive — indirect eval never inherits the caller's strictness). The fold is restricted to *declaration-free* bodies. A scope-capturing IIFE places any `var`/`function`/`class`/`let`/`const` the body declares inside the wrapper, but real global eval routes `var`/`function` to the global var environment and `let`/`const`/`class` to the eval's own fresh lexical environment — and Perry additionally registers class names at module scope, so a folded `class C {}` would leak `C` to the top level (breaking `indirect/lex-env-distinct-cls`). A declaration-free body has no such binding to misplace; it only reads/assigns the globals it names. A new recursive `eval_body_declares_bindings` scan (through blocks, loops, `try`, `switch`, `with`, labeled and `if` statements where `var`/`function` hoist) gates this, so any declaration defers to the runtime thunk as before. Test262 `language/eval-code/indirect`: +2 (`cptn-nrml-expr-prim`, `cptn-nrml-expr-obj`) with zero regressions across `language/eval-code` and `annexB/language/eval-code` (declaration-bearing bodies, incl. the annexB function-hoisting set, are unchanged). Outside global-script mode (the default, and every standalone build) behavior is byte-for-byte unchanged. Regression test: crates/perry/tests/issue_5579_indirect_eval_global_completion.rs — completion values + global mutation, object identity, always-sloppy `with`/ undeclared-assignment side effects, and that a nested-scope indirect eval does NOT capture the enclosing function's locals. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- stmt_declares_binding: replace the `_ => false` catch-all with explicit no-declaration arms (Expr/Empty/Debugger/Return/Break/Continue/Throw) so a future ast::Stmt variant that can nest a declaration is a compile error here rather than a silent miss in the eval-fold safety gate. - indirect_eval_nested_does_not_capture_locals: add a positive `typeof: undefined` + `DONE` assertion (not just the negative `!number` check) and align the doc comment with the clean-exit expectation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Six spec-accuracy fixes from the test262 builtins-tail sweep:
1. JSON.stringify(Symbol()) → undefined (ECMA-262 §25.5.2.2 step 5).
Previously fell through to the object/pointer serializer and emitted
an empty string or "{}" instead of returning undefined.
2. JSON space string spacer truncated to 10 characters (§25.5.2.1 step 6b).
The STRING_TAG branch was writing the full string; `.chars().take(10)`
now caps it. SSO paths (≤5 bytes) are already within the limit.
3. Replacer function returning a boxed-primitive wrapper (new Boolean/Number/
String) → emits the unwrapped value, not "{}". Added an early
`boxed_primitive_json_value` check at the top of
`dispatch_pointer_with_replacer` so the unwrapping happens after the
replacer has been applied (§25.5.2.2 step 4).
4. new String(Symbol()) → TypeError (§22.1.1.1 step 2b). ToString of a
Symbol is a TypeError; the `new` form now checks `js_is_symbol` before
allocating the wrapper object. The bare-call form `String(symbol)` is
unaffected (already returns SymbolDescriptiveString before reaching this
function).
5. String.prototype.toString() brand-checks `this` (§22.1.3.3 thisStringValue).
Added `string_proto_to_string_thunk` and `string_receiver_or_throw` in
`primitive_proto_thunks.rs`. Accepts string primitives, new String(…)
wrappers, and String.prototype itself (returns ""). Throws TypeError for
everything else (number, boolean, null, undefined, Symbol, plain object).
6. String.prototype.valueOf() same brand-check (same §22.1.3.3 requirement).
Shares `string_receiver_or_throw`; installed via `string_proto_value_of_thunk`.
Both thunks are wired up in the "String" arm of
`populate_builtin_prototype_methods` (global_this.rs), overwriting the
previous no-op `toString`/`valueOf` entries that were installed by
`install_noop_proto_methods`. The `primitive_proto_method_value` fast-path
(used by collection_iter.rs for folded prototype-method reads) is also
updated with ("String", "toString") and ("String", "valueOf") entries.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughNarrows indirect-eval completion-value folding in ChangesIndirect eval completion folding
String.prototype brand-checked thunks
JSON.stringify spec fixes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/perry/tests/issue_5579_indirect_eval_global_completion.rs (1)
154-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for declaration-containing eval bodies.
The production change hinges on
eval_body_declares_bindings(&body_stmts), but these regressions only exercise declaration-free folding and nested-scope deferral. Please add one global-script indirect eval containing a declaration, e.g.classorvar, to pin that it is not completion-IIFE folded/leaked as described by the safety gate.🤖 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/tests/issue_5579_indirect_eval_global_completion.rs` around lines 154 - 184, The current regression tests cover declaration-free indirect eval and nested-scope deferral, but they do not pin the safety gate driven by eval_body_declares_bindings(&body_stmts). Add a new global-script test near indirect_eval_nested_does_not_capture_locals that uses an indirect eval body containing a declaration such as var or class, and assert it does not get completion-IIFE folded or leak bindings into the surrounding scope. Use the existing compile_and_run pattern and reference the same eval/global-script setup so the test exercises the declaration-containing path directly.crates/perry-runtime/src/builtins/formatting/boxed_primitives.rs (1)
278-285: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid the hardcoded byte length.
41matches the message length today, but a hardcoded count is fragile under future edits. Derive it from the literal instead.♻️ Proposed change
if unsafe { crate::symbol::js_is_symbol(value) } != 0 { - let msg = crate::string::js_string_from_bytes( - b"Cannot convert a Symbol value to a string".as_ptr(), - 41, - ); + let bytes = b"Cannot convert a Symbol value to a string"; + let msg = crate::string::js_string_from_bytes(bytes.as_ptr(), bytes.len() as u32); let err = crate::error::js_typeerror_new(msg); crate::exception::js_throw(crate::value::js_nanbox_pointer(err as i64)) }🤖 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/builtins/formatting/boxed_primitives.rs` around lines 278 - 285, The Symbol-to-string TypeError in boxed_primitives::js_string_from_bytes uses a hardcoded byte length that can drift from the message text. Update the message construction in the Symbol check branch to derive the length from the literal itself rather than passing a fixed count, keeping the js_symbol path and js_typeerror_new/js_throw flow unchanged.
🤖 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-runtime/src/json/replacer.rs`:
- Around line 160-164: The boxed-primitive unwrapping in
`json::replacer::write_replaced` is incorrectly sending boxed BigInt values into
`write_replaced_scalar`, which serializes them as digits instead of rejecting
them. Update the `boxed_primitive_json_value` handling so `Object(1n)` is
excluded from this scalar path and instead follows the BigInt error behavior
used by `JSON.stringify`, while keeping the existing unwrapping for Boolean,
Number, and String wrappers.
- Around line 1167-1169: Move the symbol early-return below the root replacer
handling in js_json_stringify_full so the root value still goes through toJSON
and the function replacer first. The current js_is_symbol(value) check
short-circuits before the root replacer can run, which prevents
JSON.stringify(Symbol(), () => 1) from returning the replacer result. Keep the
symbol-to-undefined behavior only after the root replacer path has been applied,
using the existing js_json_stringify_full and js_is_symbol logic to locate the
change.
- Around line 1223-1225: The spacer truncation in the JSON replacer is using
chars().take(10), which counts Unicode scalar values instead of UTF-16 code
units and can violate the JSON.stringify spec for non-BMP characters. Update the
truncation logic in the indent/spacer handling in replacer.rs to clamp by UTF-16
code units, keeping the existing ECMA-262 step 6b behavior and ensuring astral
symbols are counted correctly.
---
Nitpick comments:
In `@crates/perry-runtime/src/builtins/formatting/boxed_primitives.rs`:
- Around line 278-285: The Symbol-to-string TypeError in
boxed_primitives::js_string_from_bytes uses a hardcoded byte length that can
drift from the message text. Update the message construction in the Symbol check
branch to derive the length from the literal itself rather than passing a fixed
count, keeping the js_symbol path and js_typeerror_new/js_throw flow unchanged.
In `@crates/perry/tests/issue_5579_indirect_eval_global_completion.rs`:
- Around line 154-184: The current regression tests cover declaration-free
indirect eval and nested-scope deferral, but they do not pin the safety gate
driven by eval_body_declares_bindings(&body_stmts). Add a new global-script test
near indirect_eval_nested_does_not_capture_locals that uses an indirect eval
body containing a declaration such as var or class, and assert it does not get
completion-IIFE folded or leak bindings into the surrounding scope. Use the
existing compile_and_run pattern and reference the same eval/global-script setup
so the test exercises the declaration-containing path directly.
🪄 Autofix (Beta)
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: 3a504b71-e9ea-4b2b-9b0f-e78f5ae91617
📒 Files selected for processing (6)
crates/perry-hir/src/lower/const_fold_fn.rscrates/perry-runtime/src/builtins/formatting/boxed_primitives.rscrates/perry-runtime/src/json/replacer.rscrates/perry-runtime/src/object/global_this.rscrates/perry-runtime/src/object/primitive_proto_thunks.rscrates/perry/tests/issue_5579_indirect_eval_global_completion.rs
Comment on lines
+160
to
+164
| // ECMA-262 §25.5.2.2 step 4: unwrap boxed primitive wrappers after the | ||
| // replacer has been applied. `new Boolean(true)` → "true", etc. Must come | ||
| // before the GC-type dispatch so the empty-keys fallback doesn't emit "{}". | ||
| if let Some(prim) = crate::builtins::boxed_primitive_json_value(replaced) { | ||
| write_replaced_scalar(buf, prim); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and locate the helper and scalar writer.
ast-grep outline crates/perry-runtime/src/json/replacer.rs --view expanded
printf '\n--- boxed primitive helper refs ---\n'
rg -n "boxed_primitive_json_value|write_replaced_scalar|BIGINT_TAG|TypeError|BigInt" crates/perry-runtime/src/json/replacer.rs crates/perry-runtime/src -g '!**/target/**'
printf '\n--- relevant slices ---\n'
sed -n '130,220p' crates/perry-runtime/src/json/replacer.rs
printf '\n--- helper definition if present ---\n'
rg -n "fn boxed_primitive_json_value|pub fn boxed_primitive_json_value" crates/perry-runtime/src -g '!**/target/**'Repository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '90,180p' crates/perry-runtime/src/json/replacer.rs
printf '\n---\n'
sed -n '560,610p' crates/perry-runtime/src/json/replacer.rsRepository: PerryTS/perry
Length of output: 6305
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '200,240p' crates/perry-runtime/src/builtins/formatting/boxed_primitives.rs
printf '\n---\n'
sed -n '1,140p' crates/perry-runtime/src/json/replacer.rs
printf '\n---\n'
sed -n '330,390p' crates/perry-runtime/src/json/replacer.rs
printf '\n---\n'
sed -n '1150,1215p' crates/perry-runtime/src/json/replacer.rsRepository: PerryTS/perry
Length of output: 13349
Don’t route boxed BigInt through the scalar writer.
boxed_primitive_json_value includes Object(1n), and this branch unwraps it before write_replaced_scalar, which then emits BIGINT_TAG as digits. JSON.stringify should throw on BigInt instead of producing JSON text here.
🤖 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/json/replacer.rs` around lines 160 - 164, The
boxed-primitive unwrapping in `json::replacer::write_replaced` is incorrectly
sending boxed BigInt values into `write_replaced_scalar`, which serializes them
as digits instead of rejecting them. Update the `boxed_primitive_json_value`
handling so `Object(1n)` is excluded from this scalar path and instead follows
the BigInt error behavior used by `JSON.stringify`, while keeping the existing
unwrapping for Boolean, Number, and String wrappers.
Comment on lines
+1167
to
+1169
| // JSON.stringify(symbol) returns undefined per spec (ECMA-262 §25.5.2.2 step 5) | ||
| if crate::symbol::js_is_symbol(value) != 0 { | ||
| return TAG_UNDEFINED as i64; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='crates/perry-runtime/src/json/replacer.rs'
echo '== outline =='
ast-grep outline "$FILE" --view expanded || true
echo
echo '== relevant ranges =='
rg -n "js_json_stringify_full|js_is_symbol|replacer|TAG_UNDEFINED|root" "$FILE"
echo
echo '== surrounding lines 1120-1205 =='
sed -n '1120,1205p' "$FILE"
echo
echo '== search callers =='
rg -n "js_json_stringify_full\\(" crates/perry-runtime/srcRepository: PerryTS/perry
Length of output: 15559
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1288,1348p' crates/perry-runtime/src/json/replacer.rsRepository: PerryTS/perry
Length of output: 2790
Move the symbol check below the root replacer call. js_json_stringify_full already applies toJSON and the function replacer to the root value, but this early js_is_symbol(value) return skips that path, so JSON.stringify(Symbol(), () => 1) still returns undefined instead of the replacer result.
🤖 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/json/replacer.rs` around lines 1167 - 1169, Move the
symbol early-return below the root replacer handling in js_json_stringify_full
so the root value still goes through toJSON and the function replacer first. The
current js_is_symbol(value) check short-circuits before the root replacer can
run, which prevents JSON.stringify(Symbol(), () => 1) from returning the
replacer result. Keep the symbol-to-undefined behavior only after the root
replacer path has been applied, using the existing js_json_stringify_full and
js_is_symbol logic to locate the change.
Comment on lines
+1223
to
+1225
| // ECMA-262 §25.5.2.1 step 6b: truncate string spacer to 10 characters. | ||
| let full = str_from_header(sp_ptr).unwrap_or(""); | ||
| indent_str = full.chars().take(10).collect(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file first.
ast-grep outline crates/perry-runtime/src/json/replacer.rs --view expanded || true
# Locate the spacer truncation and nearby helpers/tests.
rg -n "take\\(10\\)|spacer|indent_str|str_from_header|JSON.stringify|ECMA-262" crates/perry-runtime/src/json/replacer.rs crates/perry-runtime/src -g '!**/target/**' || true
# Read the relevant section with line numbers.
sed -n '1180,1260p' crates/perry-runtime/src/json/replacer.rsRepository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find any tests or helper utilities that exercise spacer truncation semantics.
rg -n "indent_str|spacer|JSON.stringify|utf16|len_utf16|chars\\(\\)\\.take\\(10\\)" crates/perry-runtime -g '!**/target/**' || trueRepository: PerryTS/perry
Length of output: 33165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the string header accessor used by replacer.
sed -n '470,510p' crates/perry-runtime/src/json/mod.rs
# Show the UTF-16-based string helpers already used elsewhere in the runtime.
sed -n '200,230p' crates/perry-runtime/src/string/mod.rs
# Tiny semantic probe: JS string length is UTF-16 code units, not Unicode scalar values.
node - <<'JS'
const samples = [
"😀".repeat(10),
"😀".repeat(9) + "a",
"a😀".repeat(5),
];
for (const s of samples) {
console.log(JSON.stringify({
sample: s,
chars: [...s].length,
codeUnits: s.length,
take10: [...s].slice(0, 10).join(""),
take10Units: s.slice(0, 10),
}));
}
JSRepository: PerryTS/perry
Length of output: 2742
Truncate the spacer by UTF-16 code units
chars().take(10) counts Unicode scalar values, so astral symbols can exceed the JS JSON.stringify 10-unit limit. Use a UTF-16-aware clamp here so non-BMP spacer strings match spec.
🤖 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/json/replacer.rs` around lines 1223 - 1225, The
spacer truncation in the JSON replacer is using chars().take(10), which counts
Unicode scalar values instead of UTF-16 code units and can violate the
JSON.stringify spec for non-BMP characters. Update the truncation logic in the
indent/spacer handling in replacer.rs to clamp by UTF-16 code units, keeping the
existing ECMA-262 step 6b behavior and ensuring astral symbols are counted
correctly.
Contributor
Author
|
Closing: all 5 fixes in this PR (#5591 — JSON symbol→undefined, space truncation, boxed-primitive in replacer, new String(Symbol)→TypeError, String.prototype brand-checks) were independently shipped in #5803 which has already merged to main. This branch was based 178 commits behind origin/main (at 911a979) so CI was also failing due to stale code unrelated to these changes. |
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.
Closes #5591 (partial — targets the String and JSON categories from the 133-case sweep).
Summary
Six spec-accuracy fixes for test262 built-ins:
JSON.stringify(Symbol()) →
undefined— ECMA-262 §25.5.2.2 step 5. Previously fell through the pointer serializer and emitted{}or an empty string instead of returningundefined.JSON string spacer truncated to 10 chars — §25.5.2.1 step 6b. The
STRING_TAGspacer branch was writing the full string;.chars().take(10)now caps it. SSO short-string paths (≤5 bytes) were already within the limit.Replacer returning
new Boolean/Number/String(…)→ emits primitive, not{}— §25.5.2.2 step 4. Addedboxed_primitive_json_valueunwrapping at the top ofdispatch_pointer_with_replacer, after the replacer has been applied and before the GC-type dispatch.new String(Symbol())→TypeError— §22.1.1.1 step 2b.ToString(symbol)throws; thenewform now checksjs_is_symbolbefore allocating the wrapper. BareString(symbol)(SymbolDescriptiveString) is unaffected.String.prototype.toStringbrand-checksthis— §22.1.3.3thisStringValue. Addedstring_proto_to_string_thunk+string_receiver_or_throwinprimitive_proto_thunks.rs. Accepts string primitives,new String(…)wrappers, andString.prototypeitself (returns""); throwsTypeErrorfor everything else.String.prototype.valueOfsame brand-check — same §22.1.3.3 requirement. Sharesstring_receiver_or_throw; installed alongsidetoStringviastring_proto_value_of_thunk.Files changed
perry-runtime/src/builtins/formatting/boxed_primitives.rsnew String(Symbol())→ TypeErrorperry-runtime/src/json/replacer.rsperry-runtime/src/object/global_this.rsperry-runtime/src/object/primitive_proto_thunks.rsstring_receiver_or_throw, thunks, method-value entriesTest plan
cargo check -p perry-runtime— no errorscargo test --release --workspace— CI./scripts/run_gap_tests.sh) — CI🤖 Generated with Claude Code
Summary by CodeRabbit
evalbehavior in global-script mode so indirectevalnow matches expected completion values and global binding behavior.JSON.stringifyto returnundefinedfor symbols, unwrap boxed primitives correctly, and limit string indentation to 10 characters.Stringobject behavior, including proper handling ofnew String(symbol)and brand-checkedtoString/valueOfresults.