fix(runtime): numeric-key computed calls must read the element, not dispatch by name (#6328)#6344
Open
proggeramlug wants to merge 1 commit into
Open
fix(runtime): numeric-key computed calls must read the element, not dispatch by name (#6328)#6344proggeramlug wants to merge 1 commit into
proggeramlug wants to merge 1 commit into
Conversation
…ispatch by name (#6328) `await Promise.all([...])` silently evaporated — no output, exit code 0 — when the input promises were settled through executor-captured resolvers invoked from a loop: const ps = Array.from({length: 100}, () => new Promise(r => resolvers.push(r))); const all = Promise.all(ps); for (let i = 0; i < 100; i++) resolvers[i](i); const r = await all; // never resumes The promise machinery was not at fault. `Promise.all` is not even required to reproduce: the defect is that `arr[i](x)` — a call whose callee is a computed member with a *numeric* key — was a silent no-op inside an `async` function. Codegen's `try_lower_index_get_call` bails out to the array element-call lowering only when `is_numeric_expr` proves the index numeric; otherwise the call goes to `js_native_call_method_value` (the #321 dynamic-key `this`-binding path). Inside an async function it can never prove it: the async-to-generator transform turns every body local into a boxed mutable capture typed `Any`, so the loop counter reads back untyped. `js_native_call_method_value` then ran the key through `js_to_property_key`, which canonicalizes the index to the string "3", and handed that to the dispatch tower — which resolves by METHOD NAME (own-field scan + prototype/class-id chain). An Array's ELEMENT storage is none of those, so the lookup missed, `undefined` came back, and the call vanished. The loop resolved nothing; `await all` waited on a promise nobody would ever settle; the event loop drained and the process exited 0. The 8-vs-9 threshold in the report is the loop unroller: at a low trip count the indices const-fold to literals, which codegen *can* prove numeric. Fix: per spec `obj[k](...)` is Get(obj, k) then Call — the property read wins. For a numeric key, resolve the element/own value through `js_object_get_index_polymorphic` first and invoke it when the key names something. Fall through to the name tower when it names nothing, so a numerically-named vtable method (`class C { 3() {} }`) keeps its dispatch, and `this` stays bound either way. Fixture asserts exit code as well as stdout — the failure mode is a silent exit 0.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds numeric-key resolution to native method calls and a regression test covering async promise combinators, indexed function calls, ChangesNumeric method calls
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant js_native_call_method_value
participant js_object_get_index_polymorphic
participant js_native_call_value
js_native_call_method_value->>js_object_get_index_polymorphic: read numeric indexed field
js_object_get_index_polymorphic-->>js_native_call_method_value: return indexed value
js_native_call_method_value->>js_native_call_value: invoke callable with implicit this
Possibly related PRs
🚥 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 |
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
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.
Fixes #6328.
TL;DR
The promise machinery was never at fault, and
Promise.allis not needed to reproduce. The real defect is thatarr[i](x)— a call whose callee is a computed member with a numeric key — was a silent no-op inside anasyncfunction. The reportedawait Promise.all([...])hang is a downstream symptom: thefor (…) resolvers[i](i)loop resolved nothing.Root cause
try_lower_index_get_call(perry-codegen/src/lower_call/early_branches.rs) bails out to the array element-call lowering only when it can prove the index numeric:Otherwise the call goes to
js_native_call_method_value, the dynamic-keythis-binding path added for #321 (effect'sthis[(cur)._op](cur)).Inside an
asyncfunction that proof is never available: the async-to-generator transform turns every body local into a boxed mutable capture typedAny, so the loop counter reads back untyped. Confirmed in the HIR — the step closure carriescaptures: [0, 1, 3, …],mutable_captures: [0, 1, 3, …], and the call lowers asCall { callee: IndexGet { object: LocalGet(0), index: LocalGet(3) } }.js_native_call_method_valuethen ran the key throughjs_to_property_key, which canonicalizes the index to the string"3", and handed that tojs_native_call_method— the dispatch tower, which resolves by method name (own-field scan + prototype/class-id chain). An Array's element storage is none of those. The lookup missed,undefinedcame back, and the call evaporated. No throw, no diagnostic.So the loop resolved nothing,
await allwaited on a promise nobody would ever settle, the event loop drained, and the process exited 0 having printed nothing.This also explains every discriminator in the issue:
.then()form worksmainhas noawait→ not transformed → locals stay typed → codegen proves the index numeric → correct loweringsetTimeoutform worksThe 8-vs-9 threshold was the tell — it is an unroll boundary, not an array-capacity or promise-count boundary.
Fix
One runtime change, in
js_native_call_method_value. Per specobj[k](...)isGet(obj, k)thenCall— the property read wins. For a numeric key, resolve the element/own value throughjs_object_get_index_polymorphicfirst and invoke it when the key names something; fall through to the name tower when it names nothing, so a numerically-named vtable method (class C { 3() {} }) keeps its dispatch.thisstays bound on both paths.Deliberately not fixed in codegen: the type information genuinely is not recoverable there (both the array and the index are untyped boxed captures), so the runtime is the only place this can be made correct. The sync fast path is untouched.
Shape matrix vs node 26 (all byte-identical, exit codes match)
Promise.all)Promise.allSettled/race/any, N=100.then()form, N=100setTimeoutawaitalready-resolved plain promiseawaitthrows,catchsees itarr[i](x)with no promise anywhere (the real bug)this-binding via numeric key on a plain object (#321)class C { 3() {} }vtable dispatchValidation
libperry_runtime.astaleness trap is real —perry-runtimeis rlib-only; the archive comes fromperry-runtime-static): base compiler + my archive → fixed; my compiler + base archive → bug returns. The archive is decisively the causal factor.origin/mainbuild (735e894): zero regressions. All 296 shared tests produce identical output. The single sha delta,test_gap_module_const_local_shadow, is already triaged inknown_failures.jsonand reads uninitialised memory — its garbage float drifts on every run of the same binary, on base and on this branch alike.cargo test -p perry-runtime: 1285/1285 green (--test-threads=1). The parallel suite flakes ontest_array_exotic_descriptors_and_global_prototype_identity,stream_constructors_expose_static_method_valuesand twogc::telemetry_verifiertests — pre-existing shared-realm races that reproduce at the same rate on pristinemain.cargo fmt --all -- --checkclean;scripts/check_file_size.shclean.Regression fixture
test-files/test_gap_6328_async_index_call.ts— asserts exit code as well as stdout, since the failure mode is a silent exit 0. Covers the combinator shapes, the underlyingarr[i](x)defect with no promise involved,this-binding, vtable fall-through, and the rejecting-member path.Found while here — separate bug, NOT fixed in this PR
Per-iteration
let/constbinding collapses for closures created in a loop body inside anasyncfunction: every closure sees the last iteration's value.Same root family (the async transform boxes body locals into one shared cell) but a distinct fix in the transform, not the runtime. It is pre-existing and independent — it reproduces on unpatched
mainthrough a call shape that already worked (const f = fns[i]; f(i)), so this PR neither causes nor masks it. Filing separately.Overlap with #6327
None. #6327 (
fix/6084-promise-all-and-freeze-fastpath) rewrites the promise side tables (PROMISE_SETTLE_LISTENERS,PROMISE_OVERFLOW_REACTIONS,PROMISE_ALL_STATES) with an O(1) index — a perf change inpromise/{combinators,reactions,scanners}.rs+ a newkeyed_table.rs. This PR touches onlyobject/native_call_method.rs. No file overlap, no semantic overlap, and #6327 would not have fixed this (the bug is upstream of the promise tables — the promises were never settled at all). Based onorigin/main; merges cleanly in either order.Summary by CodeRabbit
Bug Fixes
thisbinding and class-method dispatch for numeric-key access.Tests