fix(runtime,codegen): expose the Web Streams globals; resolve instanceof against a built-in held in a variable#6335
Conversation
…eof against a built-in held in a variable Two gaps that both surface the moment a bundler minifies. 1. `ReadableStream` / `WritableStream` / `TransformStream` were missing from `globalThis`. Perry implements all three — codegen lowers `new ReadableStream(…)` and `x instanceof ReadableStream`, and each has a class id — but the NAMES were never registered, so a bare `ReadableStream` identifier resolved to nothing: `typeof ReadableStream === "undefined"` and `"ReadableStream" in globalThis` was false, where Node exposes all three as functions. Libraries feature-detect exactly that (`typeof ReadableStream !== "undefined" ? … : …`) when deciding how to consume a `fetch()` body, so they silently took the wrong branch. 2. `js_instanceof_dynamic` returned `false` for every built-in reached through a *variable* rather than its bare name. `x instanceof Response` worked; the equivalent after `const R = Response` did not — and minifiers alias constructors as a matter of course, so in a bundle this was broadly broken. The dynamic path only understood user class objects; a built-in constructor is a `ClosureHeader` thunk, which it did not map back to a class id. It now recovers the id from the thunk's recorded name (ReadableStream / WritableStream / TransformStream / Response / Request / Headers / Blob) and defers to `js_instanceof`. Validation: node-vs-perry differential — `typeof` and `in globalThis` for the three stream constructors, `instanceof` against both the bare name and an alias, and `Response`/`Headers`/`Blob` through aliases — byte-identical to Node, where 9 of the 10 assertions previously disagreed. perry-runtime 1264 passed / 0 failed; perry-codegen 159 passed / 0 failed.
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughWeb Streams constructors are added to Perry’s globalThis builtin name and constructor tables. Dynamic ChangesWeb Streams builtin support
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![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
🤖 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/object/instanceof.rs`:
- Line 110: Update the pointer extraction in the instanceof logic to use the
canonical js_nanbox_get_pointer helper instead of JSValue::as_pointer when
decoding the NaN-boxed type_ref value, preserving the existing ClosureHeader
pointer handling.
- Around line 119-120: Update the string conversion flow around
str_bytes_from_jsvalue to check ptr.is_null() before calling
slice::from_raw_parts, returning None for a null pointer (including the
zero-length case); preserve the existing UTF-8 validation for non-null pointers.
- Around line 117-129: Update the constructor-to-class-ID logic around the
closure name lookup to resolve the constructor by identity against the current
globalThis builtins, rather than trusting the closure’s `.name` value. Only map
`ReadableStream`, `WritableStream`, `TransformStream`, `Response`, `Request`,
`Headers`, and `Blob` when the constructor is the corresponding native builtin;
otherwise return None, preserving the existing class-ID mappings.
🪄 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: 1f58df21-8a09-42c1-97ea-cd7ca040f57e
📒 Files selected for processing (3)
crates/perry-codegen/src/expr/helpers.rscrates/perry-runtime/src/object/global_this_tables.rscrates/perry-runtime/src/object/instanceof.rs
| if !jv.is_pointer() { | ||
| return None; | ||
| } | ||
| let closure = jv.as_pointer::<crate::closure::ClosureHeader>(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the canonical NaN-box pointer extractor.
type_ref is a NaN-boxed value, but Line [110] uses JSValue::as_pointer. Use js_nanbox_get_pointer for pointer payload extraction so this path follows the runtime’s canonical NaN-box decoding contract.
🤖 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/instanceof.rs` at line 110, Update the
pointer extraction in the instanceof logic to use the canonical
js_nanbox_get_pointer helper instead of JSValue::as_pointer when decoding the
NaN-boxed type_ref value, preserving the existing ClosureHeader pointer
handling.
Source: Coding guidelines
| let name_value = crate::closure::closure_get_dynamic_prop(closure as usize, "name"); | ||
| let mut scratch = [0u8; crate::value::SHORT_STRING_MAX_LEN]; | ||
| let (ptr, len) = crate::string::str_bytes_from_jsvalue(name_value, &mut scratch)?; | ||
| let name = std::str::from_utf8(unsafe { std::slice::from_raw_parts(ptr, len as usize) }).ok()?; | ||
| Some(match name { | ||
| "ReadableStream" => 0xFFFF_0060, | ||
| "WritableStream" => 0xFFFF_0061, | ||
| "TransformStream" => 0xFFFF_0062, | ||
| "Response" => 0xFFFF_0028, | ||
| "Request" => 0xFFFF_0029, | ||
| "Headers" => 0xFFFF_002A, | ||
| "Blob" => 0xFFFF_0026, | ||
| _ => return None, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Resolve constructor identity, not only .name.
This treats any closure named Response, ReadableStream, etc. as the native builtin. A user-defined function Response(){} can therefore make actualResponse instanceof Fake return true despite an unrelated prototype. Resolve the constructor by identity against the current globalThis builtin (or an unforgeable builtin marker), then map it to the class ID.
🤖 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/instanceof.rs` around lines 117 - 129, Update
the constructor-to-class-ID logic around the closure name lookup to resolve the
constructor by identity against the current globalThis builtins, rather than
trusting the closure’s `.name` value. Only map `ReadableStream`,
`WritableStream`, `TransformStream`, `Response`, `Request`, `Headers`, and
`Blob` when the constructor is the corresponding native builtin; otherwise
return None, preserving the existing class-ID mappings.
| let (ptr, len) = crate::string::str_bytes_from_jsvalue(name_value, &mut scratch)?; | ||
| let name = std::str::from_utf8(unsafe { std::slice::from_raw_parts(ptr, len as usize) }).ok()?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the nullable string pointer before from_raw_parts.
str_bytes_from_jsvalue explicitly permits Some((null, 0)); Line [120] then creates a slice from that null pointer, which is undefined behavior even for zero length. Return None when ptr.is_null() or change the helper contract.
🤖 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/instanceof.rs` around lines 119 - 120, Update
the string conversion flow around str_bytes_from_jsvalue to check ptr.is_null()
before calling slice::from_raw_parts, returning None for a null pointer
(including the zero-length case); preserve the existing UTF-8 validation for
non-null pointers.
1 participant
Two gaps that both surface the moment a bundler minifies.
1. The Web Streams globals were missing
ReadableStream/WritableStream/TransformStreamwere absent fromglobalThis. Perry implements all three — codegen lowersnew ReadableStream(…)andx instanceof ReadableStream, and each has a class id — but the NAMES were never registered, so a bareReadableStreamidentifier resolved to nothing:Libraries feature-detect exactly that (
typeof ReadableStream !== "undefined" ? … : …) when deciding how to consume afetch()body, so they silently took the wrong branch.2.
instanceoffailed for a built-in held in a variablejs_instanceof_dynamicreturnedfalsefor every built-in reached through a variable rather than its bare name:Minifiers alias constructors as a matter of course, so in a bundle this was broadly broken. The dynamic path only understood user class objects; a built-in constructor is a
ClosureHeaderthunk, which it did not map back to a class id. It now recovers the id from the thunk's recorded name (ReadableStream / WritableStream / TransformStream / Response / Request / Headers / Blob) and defers tojs_instanceof.Validation
node-vs-perry differential —
typeofandin globalThisfor the three stream constructors,instanceofagainst both the bare name and an alias, andResponse/Headers/Blobthrough aliases — byte-identical to Node, where 9 of the 10 assertions previously disagreed.perry-runtime1264 passed / 0 failed;perry-codegen159 passed / 0 failed.Summary by CodeRabbit
ReadableStream,WritableStream, andTransformStream.globalThis.instanceofbehavior when built-in constructors such as Web Streams,Request,Response,Headers, andBlobare stored in variables.