Skip to content

fix(bun:test): constructing a mock whose implementation returns a primitive now yields an object #31362

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 1 commit into from
Closed

fix(bun:test): constructing a mock whose implementation returns a primitive now yields an object #31362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 30 additions & 1 deletion src/jsc/bindings/JSMockFunction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@
}

JSC_DECLARE_HOST_FUNCTION(jsMockFunctionCall);
JSC_DECLARE_HOST_FUNCTION(jsMockFunctionConstruct);
JSC_DECLARE_CUSTOM_GETTER(jsMockFunctionGetter_protoImpl);
JSC_DECLARE_CUSTOM_GETTER(jsMockFunctionGetter_mock);
JSC_DECLARE_HOST_FUNCTION(jsMockFunctionGetter_mockGetLastCall);
Expand Down Expand Up @@ -462,7 +463,7 @@
}

JSMockFunction(JSC::VM& vm, JSC::Structure* structure, CallbackKind wrapKind)
: Base(vm, structure, jsMockFunctionCall, jsMockFunctionCall)
: Base(vm, structure, jsMockFunctionCall, jsMockFunctionConstruct)
{
initMock();
}
Expand Down Expand Up @@ -981,6 +982,34 @@
return JSValue::encode(jsUndefined());
}

// A native constructor must return an object, but a mock implementation can produce any value.
// Follow ordinary [[Construct]] semantics: if the implementation does not return an object,
// return a fresh object created from newTarget's prototype instead.
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));
}

Check warning on line 1011 in src/jsc/bindings/JSMockFunction.cpp

View check run for this annotation

Claude / Claude Code Review

Construct path: this-binding and mock.instances still diverge from Jest 

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}
Comment on lines +988 to +1011

@claude claude Bot May 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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:

  1. jsMockFunctionCall(globalObject, callframe) — which reads thisValue = callframe->thisValue() and invokes the implementation via Bun::call(..., thisValue, args) as a regular call
  2. only after that, allocates a fresh object from newTarget.prototype and 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 fresh this from fn.prototype, the wrapper does impl.apply(this, args), this.value = 42 lands on the fresh object, the wrapper returns undefined, [[Construct]] returns the mutated this. inst.value === 42, and fn.mock.contexts[0] === inst.
  • Bun after this PR: jsMockFunctionCall runs the implementation with callframe->thisValue() (the native construct frame's this, not a fresh instance), so this.value = 42 lands on whatever that is. The implementation returns undefined, so jsMockFunctionConstruct then allocates a brand-new empty object at lines 1007-1009 and returns it. inst.value === undefined, and mock.contexts[0] is not inst.

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.


void JSMockFunctionPrototype::finishCreation(JSC::VM& vm, JSC::JSGlobalObject* globalObject)
{
Base::finishCreation(vm);
Expand Down
24 changes: 24 additions & 0 deletions test/js/bun/test/mock-fn.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -794,6 +794,22 @@ describe("mock()", () => {

expect(bar()()).toBe(true);
});

test("constructing a mock returns an object even when the implementation returns a primitive", () => {
const fn = jest.fn();
expect(typeof new fn()).toBe("object");
expect(typeof Reflect.construct(fn, [])).toBe("object");

const returnsPrimitive = jest.fn().mockReturnValue(42);
expect(typeof new returnsPrimitive()).toBe("object");
expect(typeof Reflect.construct(returnsPrimitive, [])).toBe("object");
expect(returnsPrimitive).toHaveBeenCalledTimes(2);

const instance = { a: 1 };
const returnsObject = jest.fn(() => instance);
expect(new returnsObject()).toBe(instance);
expect(Reflect.construct(returnsObject, [])).toBe(instance);
});
});

describe("spyOn", () => {
Expand Down Expand Up @@ -1011,6 +1027,14 @@ describe("spyOn", () => {
expect(arr[14]()).toBe(456);
expect(fn).not.toHaveBeenCalled();
});

test("constructing a spy on a missing property does not crash", () => {
const target = {};
const spy = spyOn(target, "doesNotExist");
expect(typeof Reflect.construct(spy, [])).toBe("object");
expect(spy).toHaveBeenCalledTimes(1);
spy.mockRestore();
});
}

// spyOn does not work with getters/setters yet.
Expand Down
Loading