fix(bun:test): return an object when a mock function is constructed#31620
fix(bun:test): return an object when a mock function is constructed#31620robobun wants to merge 2 commits into
Conversation
JSMockFunction registered jsMockFunctionCall as both the call and the construct handler. A native construct callback must return an object — Interpreter::executeConstruct passes the result straight through asObject() — but jsMockFunctionCall can return undefined, a number, or any other primitive, so constructing a mock (e.g. Reflect.construct on jest.fn()) crashed on a non-object cell. Split the body into jsMockFunctionCallImpl parameterized on the `this` value and add a dedicated jsMockFunctionConstruct handler. It creates `this` from newTarget's prototype, runs the mock against it, and returns the mock's result only when that result is an object, otherwise the freshly created instance — matching `new` on an ordinary function. A construct-specific handler is used rather than checking callframe->newTarget() inside the shared handler, because newTarget() aliases thisValue() in JSC and would misfire on ordinary calls that happen to have a non-undefined receiver.
|
Updated 5:13 PM PT - May 30th, 2026
❌ @robobun, your commit 6346360 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 31620That installs a local version of the PR into your bun-31620 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughMock functions now support being used as constructors. The implementation refactors the existing call logic into a shared helper that accepts a parameterized ChangesMock function constructor support
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
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 `@src/jsc/bindings/JSMockFunction.cpp`:
- Around line 989-1012: jsMockFunctionConstruct creates thisObject for
constructor calls but never records it in the mock's instances array; update
jsMockFunctionConstruct to push the constructed thisObject onto the mock's
instances list (fn.mock.instances) so constructor invocations are tracked.
Locate jsMockFunctionConstruct and, after thisObject is created (and before
returning), retrieve the mock function object (via the CallFrame/callee or
existing mock function value used by jsMockFunctionCallImpl) and append
thisObject to its 'mock.instances' array, handling any JS exceptions
consistently with the existing RETURN_IF_EXCEPTION usage so the original return
behavior is preserved.
🪄 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: 066d6d0e-cc88-4f75-b5b4-612f2e567d81
📒 Files selected for processing (2)
src/jsc/bindings/JSMockFunction.cpptest/js/bun/test/mock-fn.test.js
Populate fn->instances alongside fn->contexts in the shared call helper so `new`/Reflect.construct invocations appear in mock.instances, mirroring jest (which records each call's `this`). Verified identical output against jest for plain construct, object-returning impl, this-mutating impl, and regular non-construct calls.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Closing as a duplicate of #31386, which fixes the same crash with the identical |
What
Constructing a Bun mock function (
jest.fn()/Bun.mock()) with a non-object return value crashed the process with aSIGSEGV(anASSERTION FAILED: isCell()in debug builds).JSMockFunctionregistered the same host function (jsMockFunctionCall) for both[[Call]]and[[Construct]]:Since it isn't
callHostFunctionAsConstructor,InternalFunction::getConstructDataexposes it as a native constructor. A native construct callback must return an object —Interpreter::executeConstructfeeds the result straight throughasObject()— butjsMockFunctionCallcan returnundefined, a number, or any other primitive (no implementation →jsUndefined(),mockReturnValue(42)→42,mockReturnThis()→ the receiver). Feeding a primitive toasObject()dereferences a non-object cell → flaky crash.(
new fn()from JS bytecode happened to survive becauseop_constructapplies the "non-object return → usethis" rule itself;Reflect.constructand other native-construct paths do not.)Fix
Split the call body into
jsMockFunctionCallImpl, parameterized on thethisvalue, and add a dedicatedjsMockFunctionConstructhandler. It createsthisfromnewTarget's prototype, runs the mock against it, and returns the mock's result only when that result is an object — otherwise the freshly created instance, matchingnewon an ordinary JS function. This also makesmock.contexts/mock.callsobserve the constructed instance and honorsnewTarget.prototype(soReflect.construct(fn, [], Base)yields aBaseinstance).A construct-specific handler is used rather than checking
callframe->newTarget()inside the shared handler:CallFrame::newTarget()aliasesthisValue()in JSC, so that check would misfire on ordinary calls with a non-undefined receiver (e.g.obj.method()wheremethodis amockReturnValue(42)mock would wrongly returnobj).Test
Added
mock() > are constructable with newtotest/js/bun/test/mock-fn.test.js, covering: no-implementationnew,Reflect.construct,this-mutating implementations, object-returning implementations, ignored primitive returns,newTarget.prototype,Reflect.constructwith an explicit subclass target, and a guard that a normal call with a non-undefinedthisstill returns the mock's value.Found by Fuzzilli.