fix(codegen): forward rest-param spread through native method overrides and super method calls#5529
Conversation
|
Caution Review failedPull request was closed or merged during review 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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new HIR variant Changessuper.method(...spread) end-to-end implementation
Sequence Diagram(s)sequenceDiagram
participant HIR as HIR Lowering
participant Codegen as Codegen (lower_expr)
participant SuperMethod as super_method::lower
participant Runtime as js_super_method_call_dynamic_apply
participant Dynamic as js_super_method_call_dynamic
HIR->>Codegen: Expr::SuperMethodCallSpread { method, args }
Codegen->>SuperMethod: route via super_method::lower
SuperMethod->>SuperMethod: resolve class id, materialize this_box
SuperMethod->>SuperMethod: build args_array (push plain, concat spread)
SuperMethod->>Runtime: cid, name_ptr, name_len, this_box, args_array
Runtime->>Runtime: flatten NaN-boxed array to Vec<f64>
Runtime->>Dynamic: cid, name_ptr, name_len, this_value, args_ptr, args_len
Dynamic-->>Runtime: result f64
Runtime-->>SuperMethod: result f64
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…super calls When a class method with a rest parameter is invoked and the receiver has an instance-level (own-property) override of that method — as happens for an EventEmitter subclass whose native emit/on surface is installed on the instance — the override branch of the runtime override check was handed the STATIC ABI's rest-bundled argument list. It then forwarded the rest array as a single positional argument to the dynamic override (js_native_call_value), so super.emit(event, ...args) delivered [payload] to listeners instead of payload (and lost trailing args entirely for the multi-arg case). Fix the override branch to receive the flat, un-rest-bundled user arguments; the static branch keeps the bundled arguments unchanged. Separately, the SuperMethodCall lowering discarded the spread marker entirely, so the static super-dispatch path passed a ...rest spread as one array argument even for an ordinary fixed-arity parent. Add a SuperMethodCallSpread HIR variant (mirroring the existing constructor SuperCallSpread) plus a runtime apply helper that flattens the codegen-built args array (regular + spread-expanded) before forwarding through the shared super-method dispatch. Adds a standalone regression fixture covering 0/1/2-arg native-override forwarding, fixed-arity JS-parent spread, and rest-param instance overrides.
6fc45e8 to
0f160fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-hir/src/stable_hash/expr.rs`:
- Line 100: The tag function call in the Expr::SuperMethodCallSpread variant
uses tag value 12241, which is already used by the Expr::RegisterClassCaptures
variant, causing a stable-hash collision. Replace the tag value 12241 in the
SuperMethodCallSpread match arm with a unique tag number that is not already
used by any other Expr variant in this stable hash implementation. Verify the
new tag value does not conflict with any other tags used in the file before
committing the change.
🪄 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: 7d51461f-9fdb-4584-961d-cef0144e4a85
📒 Files selected for processing (14)
crates/perry-codegen/src/collectors/this_as_value.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/super_method.rscrates/perry-codegen/src/lower_call/method_override.rscrates/perry-codegen/src/lower_call/property_get.rscrates/perry-codegen/src/runtime_decls/strings.rscrates/perry-hir/src/analysis/uses_this.rscrates/perry-hir/src/ir/expr.rscrates/perry-hir/src/lower/expr_call/mod.rscrates/perry-hir/src/stable_hash/expr.rscrates/perry-hir/src/walker/expr_mut.rscrates/perry-hir/src/walker/expr_ref.rscrates/perry-runtime/src/object/class_constructors.rstest-files/test_super_rest_spread_native_override.ts
The new Expr::SuperMethodCallSpread variant reused tag 12241, already held by Expr::RegisterClassCaptures, tripping the expr_variant_stable_hash_tags_are_unique guard and risking incorrect incremental-cache reuse. Give it 12509 (next free tag after 12508). Also applies cargo fmt to super_method.rs (CI fmt gate). Resolves the CodeRabbit review comment on expr.rs:100. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Problem
A subclass that overrides a method and forwards a rest param via
super.method(event, ...args)delivered the spread as a single array argument when the resolved super target was a native / bound base (e.g.EventEmitter):This breaks the common
emit/ event-forwarding override pattern.Root cause — two distinct latent bugs
Native-override branch (
lower_call/property_get.rs,lower_call/method_override.rs): a method with a rest param rest-bundles its args into[this, event, [rest]]for the static ABI. The own-method-override runtime branch (which invokes the native base method installed on the instance) was handed that bundled list and forwarded the rest array as one positional arg. Fixed by giving the override branch the flat, un-bundled user args while the static branch keeps the bundled form.Static super-method dispatch (
perry-hir/.../expr_call/mod.rs):super.method(...args)lowered toExpr::SuperMethodCall { args: Vec<Expr> }, discarding the spread marker (only the constructorSuperCallSpreadpath preserved it) — so even a fixed-arity JS parent received the spread as one array. Added aSuperMethodCallSpreadHIR variant + ajs_super_method_call_dynamic_applyruntime helper that flattens the codegen-built args array (regular + spread-expanded) before dispatch.Verification
"A", "B"; zero-arg fires;super.emit(e)works.this.methodoverride of a rest method, non-rest override.cargo test -p perry-runtime --lib --test-threads=1: 1067 passed, 0 failed.cargo checkclean.Repro:
test-files/test_super_rest_spread_native_override.ts.Summary by CodeRabbit
New Features
super.method(...spread)so spread arguments are correctly flattened and forwarded duringsupermethod dispatch.Bug Fixes
superhandling for spread calls:this-usage detection and override dispatch now receive the original argument shape (avoids rest being bundled incorrectly), producing correct argument counts for native/event-style methods.Tests
super.<method>(event, ...rest)forwarding across instance overrides and static dispatch paths, ensuring flat argument delivery.