fix(url): route URLSearchParams subclass instances to their native backing#6738
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughURLSearchParams subclasses now receive hidden native backing storage during construction. Codegen routes inherited properties and methods through URLSearchParams runtime helpers, while runtime reads, mutations, dynamic dispatch, and iteration resolve the backing object. ChangesURLSearchParams subclass support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant URLSearchParamsSubclass
participant js_native_call_method
participant try_url_search_params_dynamic_dispatch
participant URLSearchParamsRuntime
participant HiddenBacking
Caller->>URLSearchParamsSubclass: invoke inherited operation
URLSearchParamsSubclass->>js_native_call_method: dispatch method
js_native_call_method->>try_url_search_params_dynamic_dispatch: pass receiver and arguments
try_url_search_params_dynamic_dispatch->>URLSearchParamsRuntime: resolve subclass receiver
URLSearchParamsRuntime->>HiddenBacking: read or mutate entries
HiddenBacking-->>Caller: return operation result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
7ae88ca to
c671c7a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/perry-runtime/src/url/search_params.rs (1)
330-331: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsistent, correct wiring to the backing resolver — see the standalone comment on lines 35-110 for the shared allocation/GC-safety concern behind
resolve_search_params_receiveritself.Each of these call sites (entries read, set/append/delete/delete2/size/sort) is correctly updated to route through
resolve_search_params_receiver, and each is internally consistent (e.g.js_url_search_params_setresolves before writing_entries, matching the read side).Also applies to: 653-657, 685-689, 712-716, 767-771, 784-784, 869-873
🤖 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/url/search_params.rs` around lines 330 - 331, Ensure all URLSearchParams operations—entries reads, set, append, delete, delete2, size, and sort—resolve the receiver through resolve_search_params_receiver before accessing or mutating the backing _entries data, matching the shown call site and preserving consistent subclass handling.
🤖 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/expr/this_super_call.rs`:
- Around line 330-360: Remove the duplicate early URLSearchParams handling block
in the None arm, including its native initialization and field-initializer
calls, while preserving the later URLSearchParams branch and its
is_builtin_parent_name entry. Ensure the remaining branch continues to handle
URLSearchParams super() calls without changing behavior.
In `@crates/perry-runtime/src/url/search_params.rs`:
- Around line 35-51: Update resolve_search_params_receiver,
url_search_params_backing_of, and js_url_search_params_subclass_init to avoid
keeping raw ObjectHeader pointers live across allocating calls: use a long-lived
interned representation for URL_SEARCH_PARAMS_BACKING_KEY, and root the
receiver/backing values before js_string_from_bytes or
js_url_search_params_new_any, reloading them afterward before field access or
return.
---
Nitpick comments:
In `@crates/perry-runtime/src/url/search_params.rs`:
- Around line 330-331: Ensure all URLSearchParams operations—entries reads, set,
append, delete, delete2, size, and sort—resolve the receiver through
resolve_search_params_receiver before accessing or mutating the backing _entries
data, matching the shown call site and preserving consistent subclass handling.
🪄 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: c988f434-b5ad-410c-a340-4f31817a199d
📒 Files selected for processing (11)
crates/perry-codegen/src/expr/property_get.rscrates/perry-codegen/src/expr/this_super_call.rscrates/perry-codegen/src/lower_call/console_promise.rscrates/perry-codegen/src/lower_call/new.rscrates/perry-codegen/src/lower_call/property_get/number_string.rscrates/perry-codegen/src/runtime_decls/stdlib_ffi/web.rscrates/perry-codegen/src/type_analysis.rscrates/perry-codegen/src/type_analysis/strings.rscrates/perry-runtime/src/object/native_call_method.rscrates/perry-runtime/src/symbol/iterator.rscrates/perry-runtime/src/url/search_params.rs
c671c7a to
2a935b1
Compare
2a935b1 to
065e37a
Compare
…cking `class X extends URLSearchParams` (Next.js's ReadonlyURLSearchParams) instances lost their inherited surface: `.get()`/`.has()`/`.toString()`/iteration/`.size` threw "value is not a function", returned "[object Object]", or read undefined, because the URLSearchParams surface is type-directed lowering rather than real prototype methods — a subclass-typed receiver matches no static arm. Attach a hidden native backing at `super()` and delegate the inherited surface: - runtime: `js_url_search_params_subclass_init` stashes a native URLSearchParams under a hidden key on the instance; `url_search_params_backing_of` + `resolve_search_params_receiver` resolve it, and the read/write runtime methods resolve the backing internally. - super() codegen: emit the init from all three derived-construction paths (explicit `super(init)`, implicit `super(...args)`, no-constructor `new`). - method dispatch: the existing PerryTS#5961 dynamic URLSearchParams block resolves the backing when the native shape probe misses, reusing `url_search_params_dynamic_call` (correct boolean/string/array boxing). This runs before the generic `toString` -> "[object Object]" default. - `.size`, `.toString()`, and default `for...of` (Symbol.iterator) route to the backing via new codegen/runtime carve-outs. A user override (Next's throwing `append`/`delete`/`set`/`sort`) still resolves first via the static tower. Validated byte-identical to `node --experimental-strip-types` across get/getAll/has(name[,value])/set/append/delete/toString/forEach/sort, `.size`, entries()/keys()/values()/for-of, explicit+implicit super, and throwing readonly overrides; no regression to native URLSearchParams, Map/Set subclasses, plain-class or user toString, or number.toString(radix).
065e37a to
369465d
Compare
Problem
A
class X extends URLSearchParamsinstance — most notably Next.js'sReadonlyURLSearchParamsreturned byuseSearchParams()— lost its entireinherited surface:
r.get(k)/r.has(k)threwTypeError: value is not a functionr.toString()returned"[object Object]"r.sizereadundefinedfor (const [k, v] of r)threwnext is not a functionRoot cause: Perry lowers the URLSearchParams method/property surface by the
receiver's static type (
Expr::UrlSearchParams*), not as realURLSearchParams.prototypemethods. A subclass-typed receiver(
ReadonlyURLSearchParams) matches none of those static arms, so every callfell through to generic class dispatch, which has no such member.
This surfaced as a server-side exception on logout in a real Next.js 16 app.
Fix
Attach a hidden native URLSearchParams backing to the subclass instance at
super()and delegate the inherited surface to it:js_url_search_params_subclass_initcreates a nativeURLSearchParams and stashes it under a hidden key on
this;url_search_params_backing_of/resolve_search_params_receiverresolve it.The read/write runtime methods resolve the backing internally, so the
subclass instance stays the observable object.
paths: explicit
super(init), implicitsuper(...args), and theno-constructor
newpath.#5961dynamic-URLSearchParams block injs_native_call_methodresolves the backing when the native shape probemisses, reusing
url_search_params_dynamic_call(correct boolean / string /array boxing). It runs before the generic
toString → "[object Object]"default. A codegen carve-out routes subclass method calls into
js_native_call_methodinstead of the static class tower..size,.toString(), iteration — new codegen/runtime carve-outs routethese to the backing (
.sizeproperty arm,toStringdefers the universalarm,
js_get_iteratoryields[key, value]from the backing).A user override (Next's throwing
append/delete/set/sortonReadonlyURLSearchParams) still resolves first via the static class tower, soread-only semantics are preserved.
Validation
Byte-identical to
node --experimental-strip-typesacross:get/getAll/has(name[, value])/set/append/delete/toString/forEach/sort.size,entries()/keys()/values(), and defaultfor...ofsuper, and throwing read-only overridesNo regression to: native
URLSearchParams(static lowering intact),Map/Setsubclasses, plain-class
toString([object Object]), user-definedtoString, ornumber.toString(radix). Runtimeurl/iterator/search_paramslib tests pass;
cargo fmt --checkclean.Summary by CodeRabbit
URLSearchParams, includingsuper()/newconstruction and native-backed state for derived variants such asReadonlyURLSearchParams..size, method calls (including universal.toString()), and iterator behavior so it no longer falls back to generic/incorrect results.URLSearchParamsmethods update the underlying entries consistently for subclasses.