Skip to content

fix(jsruntime): Effect.pipe(map) chain composition#1000

Merged
proggeramlug merged 1 commit into
mainfrom
worktree-agent-a62dcf3da9a808c6a
May 18, 2026
Merged

fix(jsruntime): Effect.pipe(map) chain composition#1000
proggeramlug merged 1 commit into
mainfrom
worktree-agent-a62dcf3da9a808c6a

Conversation

@proggeramlug
Copy link
Copy Markdown
Contributor

Summary

  • Effect.runSync(Effect.succeed(42).pipe(Effect.map(x => x + 1))) returned undefined (TypeError: f is not a function) instead of 43. PR feat(jsruntime): V8 named-import static-method dispatch for Effect.succeed #992 fixed Effect.succeed(42) but the follow-on Effect.map(fn) lost its closure arg at the V8 boundary.
  • Two-layer fix: extend HIR js_transform to wrap Closure args of StaticMethodCall on V8-imported classes in JsCreateCallback, and add a GC_TYPE_CLOSURE arm to native_object_to_v8 that wraps the closure pointer as a real v8::Function (covers the LocalGet/FuncRef fallback path).
  • Bumps to 0.5.1002; fixture at test-files/test_effect_pipe_map.ts.

Root cause

StaticMethodCall { class_name: "Effect", method_name: "map", args: [Closure {...}] } flowed through emit_v8_member_method_call -> js_call_v8_member_method. Each arg was packed via native_to_v8(scope, fixup_native_for_v8(a)). The closure's raw *const ClosureHeader had heap-pointer bits (0x0000_0001_xxxx_xxxx), so fixup_native_for_v8 tagged it POINTER_TAG. native_to_v8 then dispatched to native_object_to_v8, which had arms for GC_TYPE_PROMISE / ARRAY / OBJECT but not CLOSURE — fallback string/array heuristics misidentified the closure and surfaced as a proxy object. Effect's pipeline tried f = transformer._op_action_fn, got a non-function, threw inside runSync.

Fix

  1. crates/perry-hir/src/js_transform.rsStaticMethodCall arm now checks extern_func_to_js.contains_key(class_name) and rewrites any Closure arg into JsCreateCallback { closure, param_count }, marking the closure's params as JS values on a cloned tracker (mirrors the existing JsCallMethod arm).

  2. crates/perry-jsruntime/src/bridge.rs:

    • New perry_closure_v8_trampoline + native_closure_to_v8: build a v8::Function whose data slot holds the closure's raw *const ClosureHeader (wrapped in v8::External). On invocation the trampoline marshals args via v8_to_native, stashes the scope for nested FFI (stash_trampoline_scope), calls js_closure_call_array(closure_env, args_ptr, args_len), and returns through native_to_v8.
    • native_object_to_v8 gains a GC_TYPE_CLOSURE arm gated on CLOSURE_MAGIC at offset 12 (same tag js_value_typeof uses), routed to native_closure_to_v8. Defense-in-depth for the LocalGet / FuncRef fallback path that the HIR transform can't see.

Test plan

  • Effect.runSync(Effect.succeed(42).pipe(Effect.map((x: number) => x + 1))) returns 43 (was: undefined with internal f is not a function).
  • const fn = (x) => x + 1; Effect.runSync(Effect.succeed(42).pipe(Effect.map(fn))) returns 43 (named local case — exercises the new bridge GC_TYPE_CLOSURE arm).
  • Gap suite: 30/36 pass — same set as pre-fix; no regressions.
  • All four test_issue_effect_* fixtures still pass (arguments_length_returned_fn, factory_static_dispatch, tag_deeper, tag_undefined).
  • Fixture test-files/test_effect_pipe_map.ts exercises the chain.

`Effect.runSync(Effect.succeed(42).pipe(Effect.map(x => x + 1)))` returned
`undefined` (TypeError: f is not a function inside Effect's pipeline)
instead of `43`. PR #992 wired up `Effect.succeed(42)` to
`js_call_v8_member_method`, but the follow-on `Effect.map(fn)` still
failed because the Perry closure argument never crossed into V8 as a
real `v8::Function`.

Two-layer fix:

1. HIR `js_transform`: extend the closure -> `JsCreateCallback` rewrite
   to `StaticMethodCall` args when the class name is a JS-imported value
   (mirrors the existing `JsCallMethod` / `JsCallFunction` arms). This
   catches inline-Closure arg patterns like `Effect.map((x) => x + 1)`.

2. Bridge `native_object_to_v8`: add a `GC_TYPE_CLOSURE` arm gated on
   `CLOSURE_MAGIC` that wraps the closure in a `v8::Function` via a new
   `perry_closure_v8_trampoline` (closure pointer stashed in
   `v8::External`, body invokes `js_closure_call_array`). This catches
   the LocalGet / FuncRef path where the HIR transform only sees the
   variable, not the closure literal.

Bumps version to 0.5.1002. Detailed root cause and validation in
CHANGELOG.

Fixture: test-files/test_effect_pipe_map.ts.
@proggeramlug proggeramlug merged commit 409aead into main May 18, 2026
6 of 9 checks passed
@proggeramlug proggeramlug deleted the worktree-agent-a62dcf3da9a808c6a branch May 18, 2026 07:39
proggeramlug added a commit that referenced this pull request May 18, 2026
…1009)

PR #973 lowered bare built-in idents (`Promise`, `Array`, `Date`, ...)
as `PropertyGet { GlobalGet(0), name }` so they route through the
globalThis singleton closure path. Two codegen call sites that
specialize `.then()` dispatch were still pattern-matching the legacy
`Expr::GlobalGet(_)` shape only:

- `type_analysis::is_promise_expr` for `Promise.resolve/reject/all/race/
  allSettled/any(...)` and `Array.fromAsync(...)`.
- `lower_call.rs`'s fused `Promise.resolve(x).then(cb)` fast path that
  routes to `js_promise_resolved_then`.

When `is_promise_expr` returned false, `.then(cb)` fell through to the
generic native method dispatch which doesn't enqueue the callback —
microtask-02..07 and edge-promises went silent in compile-smoke's
Native no-fallback gate, and on Linux V8 surfaced the same shape as
`TypeError: then is not a function`. The Native gate had been red on
every PR since #973 (admin-bypassed on #997 / #1000 / #1003 / #1004).

Extract `type_analysis::is_global_builtin_named(expr, name)` that
matches both shapes (legacy `GlobalGet(_)` and the post-#973
`PropertyGet { GlobalGet(0), name }`) and route both call sites
through it.

Validation: `scripts/run_native_no_fallback_tests.sh` — 35 passed, 0
failed (was 28/7 pre-fix).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant