fix(hir): spread call of a native crypto method must not collapse the spread - #6670
Conversation
… spread `crypto.hkdf(...args, cb)` (dotted) and `hkdf(...args, cb)` (named import) lowered through the crypto fast-path, which collapses the call into a flat `Expr::Call` / `Expr::NativeMethodCall` whose args are the lowered AST arguments verbatim — a spread operand is passed as the array itself, not expanded. The codegen fast-path (`arm_crypto_hkdf_async_alg`) then saw too few positional args and returned `undefined` without dispatching, so the callback never fired: a silent no-op. Gate the crypto passthrough (dotted form, `module_static.rs`) and the crypto named-import path (`globals.rs`) on `!has_spread`. A spread call now falls through to the generic `CallSpread` lowering, which routes through `js_closure_call_apply_with_spread` -> `dispatch_bound_method` -> `js_native_call_method` — the same bound-native dispatch the value-read form (`const f = crypto.hkdf; f(...)`) already uses, and which the runtime crypto dispatcher handles for every callable export by name. Adds `test-files/test_crypto_hkdf_spread_6668.ts`, byte-identical to `node --experimental-strip-types` across dotted / interleaved / named / sync spread forms plus a non-spread regression guard. Fixes #6668
|
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 (2)
📝 WalkthroughWalkthroughCrypto call lowering detects spread arguments and bypasses specialized native-module and crypto passthrough paths. A regression test covers synchronous and asynchronous ChangesCrypto spread-call dispatch
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/lower/expr_call/module_static.rs`:
- Around line 972-987: Update the crypto handling in the module-static call
lowering block so any has_spread call bypasses the method_name passthrough check
and returns through the generic spread-call path. Align this guard with the
crypto behavior in globals.rs, ensuring non-passthrough methods such as sha256
and md5 do not enter the method_name match and receive the spread array as a
normal argument; preserve existing handling for non-spread calls.
In `@test-files/test_crypto_hkdf_spread_6668.ts`:
- Around line 19-20: Update the argument arrays sargs, a1, mid, and a3 used by
the crypto.hkdfSync and hkdf spread calls to be inferred as tuples, using const
assertions or explicit tuple types, so the spread operands satisfy the expected
parameter signatures.
🪄 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: ad2ce898-8594-4716-819f-87f9e5a5f7f5
📒 Files selected for processing (3)
crates/perry-hir/src/lower/expr_call/globals.rscrates/perry-hir/src/lower/expr_call/module_static.rstest-files/test_crypto_hkdf_spread_6668.ts
Address CodeRabbit review on #6670. module_static.rs previously gated only `is_passthrough_method` on `!has_spread`, so a spread call to a non-passthrough crypto method (`crypto.sha256(...args)`, `crypto.md5(...args)`, `crypto.getRandomValues(...args)`) still fell into the manual `match method_name` arms and had its spread array extracted as a single argument. Bail out of the entire crypto block when `has_spread`, mirroring the crypto guard already in globals.rs, so every crypto spread call routes through the generic CallSpread / bound-native dispatch path. Also make the spread arg lists in the regression test tuples (`as const`) so the spreads satisfy the parameter signatures under `tsc` (TS2556); runtime output is unchanged and still byte-identical to node.
Summary
Fixes #6668 — a spread call of a native
cryptomethod(
crypto.hkdf(...args, cb)) silently no-ops: no dispatch, no throw, thecallback never fires, and the call evaluates to
undefined.Root cause
The bug is in HIR lowering, not the runtime spread bridge the issue
suspected. The crypto fast-path (
lower_crypto_passthrough, reached from boththe dotted
crypto.hkdf(...)form inmodule_static.rsand the named-importimport { hkdf } from "crypto"form inglobals.rs) collapses the call into aflat
Expr::Call/Expr::NativeMethodCallwhose args are the lowered ASTarguments verbatim — a spread operand (
...args) is passed as the arrayitself, not expanded. Codegen's
arm_crypto_hkdf_async_algthen seesargs.len() < 6and returnsundefinedwithout ever dispatching, so thecallback never runs.
Confirmed by dumping HIR:
crypto.hkdf(...args, cb)lowered toCall { callee: PropertyGet{NativeModuleRef("crypto"), "hkdf"}, args: [<args array>, cb] }— 2 positional args instead of 6.
The value-read form (
const f = crypto.hkdf; f(...)) already works because itdispatches through the bound-native path
(
js_native_call_value→dispatch_bound_method→js_native_call_method).Fix
When a spread argument is present, the crypto fast-paths decline so the
generic lowering tail builds an
Expr::CallSpreadinstead. That CallSpreadroutes through
js_closure_call_apply_with_spread→dispatch_bound_method("hkdf")→js_native_call_method— the exactbound-native dispatch the value-read form uses (and which the runtime crypto
dispatcher already handles for every callable export by name).
module_static.rs— gate the dotted-form crypto passthrough on!has_spread.globals.rs— decline named-import crypto method calls that carry a spread.Both are one-line guards; no runtime changes. Scoped to
crypto(the reportedmodule), whose runtime dispatcher resolves every callable export by name.
Verification
test-files/test_crypto_hkdf_spread_6668.ts(new) — byte-identical tonode --experimental-strip-typesacross five forms:Before the fix,
dotted-spread,dotted-interleaved, andnamed-spreadweremissing entirely (silent no-op).
Summary by CodeRabbit
crypto.hkdf/hkdfSyncbehavior when invoked with spread arguments, preventing incorrect simplification.crypto.hkdfspread-call variants to verify deterministic synchronous output and reliable asynchronous callback execution.