fix(bun:test): constructing a mock whose implementation returns a primitive now yields an object#31362
fix(bun:test): constructing a mock whose implementation returns a primitive now yields an object#31362robobun wants to merge 1 commit into
Conversation
…entation returns a primitive JSMockFunction registered jsMockFunctionCall as both its call and construct callback. A native construct callback must return a JSObject, but a mock's implementation can produce any value (undefined for a bare jest.fn() or a spy on a missing property, any primitive via mockReturnValue, etc.). Constructing such a mock through Reflect.construct, a Proxy, or super() funnels through JSC::construct(), which asserts isCell()/isObject() on the result in debug builds and reinterprets the primitive as an object pointer in release builds. Give JSMockFunction a dedicated construct callback that runs the same mock logic and, when the implementation result is not an object, follows ordinary [[Construct]] semantics by returning a fresh object created from newTarget.prototype (falling back to Object.prototype). This also matches Jest, where mocks are plain JS functions and 'new mock()' yields an instance when the implementation returns a primitive.
|
Updated 11:06 AM PT - May 24th, 2026
❌ @robobun, your commit 855241d has 2 failures in
🧪 To try this PR locally: bunx bun-pr 31362That installs a local version of the PR into your bun-31362 --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)
WalkthroughThis PR adds ChangesMock Function Construct Support
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Closing as a duplicate of #30212, which fixes the same |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| JSC_DEFINE_HOST_FUNCTION(jsMockFunctionConstruct, (JSGlobalObject * globalObject, CallFrame* callframe)) | ||
| { | ||
| auto& vm = JSC::getVM(globalObject); | ||
| auto scope = DECLARE_THROW_SCOPE(vm); | ||
|
|
||
| JSValue returnValue = JSValue::decode(jsMockFunctionCall(globalObject, callframe)); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
|
|
||
| if (returnValue && returnValue.isObject()) { | ||
| return JSValue::encode(returnValue); | ||
| } | ||
|
|
||
| JSValue newTarget = callframe->newTarget(); | ||
| JSValue prototype = jsUndefined(); | ||
| if (newTarget && newTarget.isObject()) { | ||
| prototype = asObject(newTarget)->get(globalObject, vm.propertyNames->prototype); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| } | ||
|
|
||
| JSObject* thisObject = prototype.isObject() | ||
| ? JSC::constructEmptyObject(globalObject, asObject(prototype)) | ||
| : JSC::constructEmptyObject(globalObject); | ||
| RELEASE_AND_RETURN(scope, JSValue::encode(thisObject)); | ||
| } |
There was a problem hiding this comment.
🟡 This fixes the crash, but the comment claiming "ordinary [[Construct]] semantics" overstates what the code does: ordinary [[Construct]] allocates the receiver from newTarget.prototype before the body runs and binds it as this, whereas here the implementation runs first (via jsMockFunctionCall, with callframe->thisValue()) and the fresh object is created only afterward. So const fn = jest.fn(function () { this.value = 42 }); new fn() returns an empty object in Bun but {value: 42} in Jest, and mock.contexts records the wrong receiver. Not a regression — strictly better than the prior crash/undefined — so fine as a follow-up; relatedly (and pre-existing), nothing ever pushes onto fn->instances, so mock.instances stays [] after new fn() even though there is now an instance to record.
Extended reasoning...
What the gap is
jsMockFunctionConstruct does:
jsMockFunctionCall(globalObject, callframe)— which readsthisValue = callframe->thisValue()and invokes the implementation viaBun::call(..., thisValue, args)as a regular call- only after that, allocates a fresh object from
newTarget.prototypeand returns it when the implementation result was not an object
Ordinary ECMA-262 [[Construct]] does the opposite: it creates the receiver from newTarget.prototype before evaluating the function body, binds it as this, runs the body, and returns that receiver if the body returned a non-object. Jest's mocks are plain JS functions (the wrapper does impl.apply(this, args)), so they get this for free.
Concrete walkthrough
const fn = jest.fn(function () { this.value = 42; });
const inst = new fn();- Jest:
new fn()creates a freshthisfromfn.prototype, the wrapper doesimpl.apply(this, args),this.value = 42lands on the fresh object, the wrapper returnsundefined,[[Construct]]returns the mutatedthis.inst.value === 42, andfn.mock.contexts[0] === inst. - Bun after this PR:
jsMockFunctionCallruns the implementation withcallframe->thisValue()(the native construct frame's this, not a fresh instance), sothis.value = 42lands on whatever that is. The implementation returnsundefined, sojsMockFunctionConstructthen allocates a brand-new empty object at lines 1007-1009 and returns it.inst.value === undefined, andmock.contexts[0]is notinst.
The new tests don't catch this because they only assert typeof result === "object" or check an implementation that explicitly returns an object — none exercise an implementation that mutates this without returning.
Why this is not a regression
Before this PR the same input either returned undefined (new fn() bytecode path) or crashed with the isCell() assertion (Reflect.construct path). The implementation has always been invoked via Bun::call with callframe->thisValue(), so this-mutation was never preserved and mock.contexts already recorded the same value it does now. Returning an empty object is strictly better than crashing or returning a primitive; this is an incomplete fix, not a new bug.
Adjacent pre-existing gap: mock.instances
Grepping the file, fn->instances is declared, cleared, lazily created, GC-visited and exposed on the mock object — but nothing ever pushes onto it, in either jsMockFunctionCall or the new jsMockFunctionConstruct. So fn.mock.instances is always [] in Bun, whereas in Jest new fn() records the instance. This was equally true before the PR (it's a long-standing stub), so it's out of scope for a crash fix; mentioning it only because the construct path now actually produces an instance object that could be recorded, and the fix below naturally gives you the value to push.
On the refutation
One verifier argued the mock.instances part should be dropped entirely as a pre-existing missing feature with nothing in the diff to comment on. That's fair — it is folded in here as a one-line aside on the same construct callback rather than a standalone finding, and is explicitly flagged as pre-existing / follow-up rather than something to change in this PR.
Suggested follow-up fix
To fully match Jest, jsMockFunctionConstruct should allocate the fresh object from newTarget.prototype first, then invoke the mock with that object as thisValue (this requires either threading a thisValue into jsMockFunctionCall or extracting a helper that takes one). That same object can then be pushed onto mock.contexts and mock.instances. None of that needs to block this PR — the crash fix is correct as-is — but the "ordinary [[Construct]] semantics" comment should probably be softened in the meantime.
1 participant
Problem
JSMockFunctionregisteredjsMockFunctionCallas both its call and construct callback. A native construct callback must return aJSObject—Interpreter::executeConstructcallsasObject()on the result unconditionally — but a mock's implementation can produce any value:undefinedfor a barejest.fn()or a spy on a missing property, or any primitive viamockReturnValue(...).Constructing such a mock through a path that goes via
JSC::construct()(Reflect.construct, Proxy construct,super()) hitsASSERTION FAILED: isCell()in debug builds and reinterprets the primitive's bits as an object pointer in release builds. Found by fuzzing (fingerprintJSCJSValue.h(1043)):Through the bytecode
new mock()path the primitive silently escaped to JS instead, sonew (jest.fn())()evaluated toundefined, which also diverges from Jest.Fix
Give
JSMockFunctiona dedicated construct callback. It runs the existing mock logic (calls are still recorded, results still tracked) and, when the implementation result is not an object, follows ordinary[[Construct]]semantics: return a fresh object created fromnewTarget.prototype, falling back toObject.prototype. Implementations that return an object keep returning that object. This matches Jest, where mocks are plain JS functions andnew mock()yields an instance when the implementation returns a primitive.Tests
Added to
test/js/bun/test/mock-fn.test.js(file is runnable under Jest/Vitest/Bun; the new assertions match Jest behavior):jest.fn()/jest.fn().mockReturnValue(42)vianewandReflect.constructreturns an object, and the calls are recordedBefore this change the new tests fail with
Received: "undefined"on release builds and crash with theisCell()assertion on debug builds; the original fuzzer reproducer now exits cleanly.