bun:ffi: throw validation errors from viewSource and JSCallback instead of returning them

[robobun]

What does this PR do?

viewSource from bun:ffi returned a TypeError instead of throwing it when the descriptor was invalid:

import { viewSource } from "bun:ffi";

const result = viewSource({ myFunc: 42 });
result instanceof TypeError;  // true, returned not thrown
result.message;               // 'Expected an object for key "myFunc"'

JSCallback has the same bug: its constructor destructured the returned error, so validation failures produced a useless instance instead of throwing:

import { JSCallback } from "bun:ffi";

const cb = new JSCallback(() => {}, 42);   // options must be an object
cb.ptr;                                    // undefined, no error surfaced

Both happen for every validation failure the native implementation reports: non-object descriptors, unknown FFI type names, a non-object options argument, and (for JSCallback) a non-callable callback.

Cause

The native host functions on Bun.FFI return an Error instance as their value when validation fails; the bun:ffi module is expected to turn that into a throw. dlopen, cc and linkSymbols already do this with if (Error.isError(result)) throw result;, but viewSource was exported as a direct alias of the native function and JSCallback destructured the native result without checking it.

Fix

Wrap viewSource and guard the JSCallback constructor the same way as the three existing siblings. The declared return type of viewSource in packages/bun-types/ffi.d.ts is string[] | string, which this now matches.

Why this is correct

Matching dlopen/cc/linkSymbols keeps the bun:ffi module internally consistent, and it matches the typed contract. The low-level Bun.FFI object is left unchanged (it is not part of the typed public surface and Bun.FFI.dlopen/Bun.FFI.linkSymbols also return errors).

How did you verify your code works?

Rewrote test/js/bun/ffi/ffi-viewSource-non-object.test.ts to assert the error is actually thrown (the old assertions only passed because expect().toThrow() currently accepts a returned Error, which #34392 tightens). Added test.each cases for viewSource (non-object descriptor, unknown type, non-object options, callback descriptor, valid input) and JSCallback (non-object options, non-callable callback, unknown type, valid input).

$ bun bd test test/js/bun/ffi/ffi-viewSource-non-object.test.ts
 24 pass  0 fail

22 of the 24 cases fail on an unfixed build (the two valid-input cases pass as expected). The full test/js/bun/ffi/ suite is green and clean under BUN_JSC_validateExceptionChecks=1.

Related

#34392 changes toThrow() to stop accepting a returned Error and rewrites this test to assert on the returned value as a stopgap. This PR is the actual fix; whichever lands second will need the test file reconciled.

#33200 touches FFI::print/FFI::callback for a different bug (propagating exceptions from descriptor getters) and still returns the validation error as a value, so the wrappers here cover both before and after that change.

---

no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/ffi/ffi-viewSource-non-object.test.ts

[robobun]

Reproduced with import { viewSource } from "bun:ffi"; viewSource({ myFunc: 42 }) returning a TypeError instead of throwing; new JSCallback(() => {}, 42) has the same problem and silently constructs with ptr === undefined. Fix wraps both with the same Error.isError guard that dlopen/cc/linkSymbols already use.

24 tests pass locally (22 fail on an unfixed build). The FFI test file is green on every CI lane across two runs; remaining reds are unrelated pre-existing ASAN failures on the Debian 13 x64-asan lane (test-worker-message-port-transfer-terminate.js, tracked in #34095, and timer-heap-race.test.ts, a ConcurrentTask leak during worker VM teardown) plus Windows flakes that passed on retry. Ready for review.

[robobun]

^{Updated 2:11 PM PT - Jul 16th, 2026}

❌ @robobun, your commit 811e48c has 1 failures in Build #74045 (All Failures):

• test/js/web/timers/timer-heap-race.test.ts - pid 6405 sigabrt on 🐧 13 x64-asan

---

🧪   To try this PR locally:

bunx bun-pr 34396

That installs a local version of the PR into your bun-34396 executable, so you can run:

bun-34396 --bun

[coderabbitai]

Walkthrough

Changes

FFI viewSource behavior

Layer / File(s)	Summary
Wrap native viewSource results src/js/bun/ffi.ts	The exported viewSource wrapper delegates to the native implementation, throws returned errors, and returns successful results.
Test thrown errors and successful output test/js/bun/ffi/ffi-viewSource-non-object.test.ts	Tests now inspect caught TypeError values, cover invalid inputs, and verify normal and callback source generation.

Possibly related PRs

• oven-sh/bun#34392: Adjusts error assertions for cases where Error values are returned instead of thrown.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title matches the main change: viewSource now throws validation errors, and JSCallback is also fixed.
Description check	✅ Passed	The PR description includes both required sections and covers the change, cause, fix, and verification details.

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

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 `@test/js/bun/ffi/ffi-viewSource-non-object.test.ts`:
- Around line 25-29: Replace the input loop in the viewSource invalid-value
tests with describe.each() parameterized cases, and apply the same conversion to
the additional test matrices around the referenced cases. Keep each value as a
separately reported case while preserving the existing TypeError assertions and
expected message checks.

🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3e5208c5-2c99-4c8b-967b-f6dbdb2a2c31

📥 Commits

Reviewing files that changed from the base of the PR and between 6ad467f and 2900eed.

📒 Files selected for processing (2)

• src/js/bun/ffi.ts
• test/js/bun/ffi/ffi-viewSource-non-object.test.ts

[claude]

LGTM — straightforward fix that wraps viewSource with the same Error.isError(result) → throw guard already used by dlopen/cc/linkSymbols.

What was reviewed:

• Confirmed the wrapper matches the sibling pattern in src/js/bun/ffi.ts and that always passing isCallback (even as undefined) is harmless — FFI::print in ffi_body.rs treats Some(undefined) the same as None via to_boolean().
• Checked the zero-arg edge: eat_required no longer sees an empty slot, but print still rejects via is_empty_or_undefined_or_null, so it still throws.
• Tests use an explicit try/catch helper so they distinguish thrown vs returned regardless of toThrow() semantics, and cover all four validation paths plus the happy path.

Extended reasoning...

Overview

Two files touched. src/js/bun/ffi.ts renames the direct ffi.viewSource alias to nativeViewSource and adds a 3-line wrapper that throws when the native call returns an Error — byte-for-byte the same pattern as dlopen (line ~444), cc (line ~519), and linkSymbols (line ~531) in the same file. test/js/bun/ffi/ffi-viewSource-non-object.test.ts is expanded from 3 assertions to 5 test cases covering non-object descriptors, unknown FFI types, non-object options, non-object callback descriptors, and the valid-input return shape.

Security risks

None. This is a JS-side wrapper that converts a returned error value into a thrown one; no new native code, no new inputs reach native, no auth/crypto/permissions surface.

Level of scrutiny

Low. The fix is mechanical pattern-matching against three existing siblings in the same file. I traced the one potential behavior delta — the wrapper now always passes two arguments to the native function — through FFIObject.rs::view_source → arguments_old::<2> → next_eat → FfiImpl::print. The native side branches on is_callback.to_boolean(), so Some(undefined) and None both fall through to the non-callback path. The only observable change beyond the intended one is that viewSource() with zero args now throws the invalid_options_arg TypeError instead of the eat_required "Missing argument" error — still a thrown TypeError, and not part of any documented contract.

Other factors

The rewritten test uses an explicit thrown() helper (try/catch) rather than expect().toThrow(), which the PR description notes is deliberate to avoid depending on toThrow()'s current lenient handling of returned errors (being tightened in #34392). Tests assert toBeInstanceOf(TypeError) and check message content, and include a positive case asserting the documented string[] / string return types. No CODEOWNERS entry covers these paths. No outstanding review comments.

[claude]

LGTM — mechanical fix that mirrors the existing Error.isError(result) guard already used by dlopen/cc/linkSymbols in the same file.

What was reviewed:

• Confirmed the native FFI::print and FFI::callback return validation errors as values via to_invalid_arguments (ffi_body.rs:1268/1272/1345/1421), so the JS-side throw is the correct place for this.
• Checked the arity change in the viewSource wrapper (now always forwards 2 args): undefined for isCallback is falsy in print, and undefined for symbols still yields a thrown TypeError via invalid_options_arg — no behavior regression.
• Test error-message assertions match the native strings ("Expected object", "Expected callback function", "JSFunctionCall", "my_callback_function").
• CodeRabbit's describe.each nit is stale — commit 6533ebb converted to test.each, which is what the repo guidelines prescribe.

Extended reasoning...

Overview

Two small changes to src/js/bun/ffi.ts:

1. viewSource is no longer a direct alias of ffi.viewSource; it's a 4-line wrapper that throws when the native result is an Error, matching the identical guard on dlopen, cc, and linkSymbols a few lines away.
2. The JSCallback constructor checks nativeCallback's result before destructuring {ctx, ptr} and throws if it's an Error.

The test file is rewritten with test.each matrices covering non-object descriptors, unknown FFI type names, non-object options, non-callable callbacks, and valid-input positive cases for both viewSource and JSCallback. It uses an explicit thrown() helper so the assertions distinguish thrown vs returned Errors regardless of toThrow() semantics (relevant given #34392).

Security risks

None. This only changes whether an already-constructed TypeError is returned vs thrown from a validation path in bun:ffi. No new inputs are accepted, no parsing changes, no auth/crypto/permission surface.

Level of scrutiny

Low. The pattern is copied byte-for-byte from three sibling functions in the same file. I traced the native side (src/runtime/ffi/ffi_body.rs) to confirm FFI::print and FFI::callback do use to_invalid_arguments (returns an Error as a value, not thrown) on every validation failure, so the wrapper is the right place to convert to a throw. I also checked that the wrapper always forwarding two args doesn't change the non-callback path: arguments_old::<2> yields Some(undefined) for the second arg, to_boolean() is false, and it falls through as before. The zero-arg case now surfaces "Expected an options object with symbol names" instead of "Missing argument", which is an improvement and is what the test asserts.

Other factors

• Test assertions on error messages match the exact native strings ("Expected object", "Expected callback function") and generated-source markers ("JSFunctionCall", "my_callback_function") I found in ffi_body.rs / host_fns.rs.
• The Error.isError call is not primordial-safe, but it's the exact idiom the three neighboring functions already use — not a new concern introduced here.
• CodeRabbit left a stale nit about converting loops to describe.each; the current revision already uses test.each, which is what CLAUDE.md prescribes for matrices. Nothing outstanding.
• No CODEOWNERS entries cover these paths.
• The PR description notes 22/24 test cases fail on an unfixed build, satisfying the "fails for the right reason" bar.