Skip to content

bun:test: return an object when constructing a mock function#31370

Closed
robobun wants to merge 1 commit into
mainfrom
farm/4e0236f3/fix-mock-construct
Closed

bun:test: return an object when constructing a mock function#31370
robobun wants to merge 1 commit into
mainfrom
farm/4e0236f3/fix-mock-construct

Conversation

@robobun

@robobun robobun commented May 24, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Fixes a fuzzer-found assertion failure (ASSERTION FAILED: isCell() in JSCJSValue.h:1043) triggered by constructing a bun:test mock function:

import { mock } from "bun:test";
const fn = mock();
Reflect.construct(fn, []); // ASSERTION FAILED: isCell() in debug builds

JSMockFunction registered the same host callback (jsMockFunctionCall) for both call and construct, so constructing a mock returned whatever the call path produced — undefined when there is no implementation, or any primitive the implementation returns. JSC requires native construct callbacks to return an object; Interpreter::executeConstruct (the path used by Reflect.construct, proxies, and JSC::construct from C++) does asObject(result) on the returned value, which asserts in debug builds. In release builds new mockFn() silently evaluated to undefined/a primitive, which also breaks Jest's class-mocking pattern (const instance = new MockedClass()).

This PR gives JSMockFunction a dedicated construct callback that follows ordinary [[Construct]] semantics:

  • create this from new.target's prototype (falling back to Object.prototype, like OrdinaryCreateFromConstructor)
  • run the existing mock call logic with that this (so mock.calls/mock.contexts/mock.results are recorded as before, and the implementation sees the new instance as this)
  • return the implementation's result if it is an object, otherwise return the newly created this

This matches how Jest mocks behave when constructed, since Jest's mockConstructor is a plain JS function.

How did you verify your code works?

  • The fuzzer repro and a minimized Reflect.construct(Bun.jest(...).mock(), []) script no longer hit the assertion with a debug (ASAN) build.
  • Added a test to test/js/bun/test/mock-fn.test.js covering new fn(), implementations that return primitives/objects, this handling, and Reflect.construct with a custom new.target. The test fails on current bun (returns undefined from new fn()) and passes with this change; assertions are written to also hold under Jest/Vitest, which this file supports.
  • bun bd test test/js/bun/test/mock-fn.test.js, mock-disposable.test.ts, mock/mock-module.test.ts, spyMatchers.test.ts, and expect-toHaveReturnedWith.test.js all pass, including with BUN_JSC_validateExceptionChecks=1.

JSMockFunction used the same host callback for call and construct, so
`new mockFn()` / `Reflect.construct(mockFn, ...)` could return a
primitive. JSC requires native construct callbacks to return an object,
and Interpreter::executeConstruct asserts on the result, so constructing
a mock with no object-returning implementation hit
`ASSERTION FAILED: isCell()` in debug builds.

Give JSMockFunction a dedicated construct callback that follows ordinary
[[Construct]] semantics: create `this` from new.target's prototype, run
the mock with that `this`, and return the mock's result only when it is
an object.
@robobun

robobun commented May 24, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 4:46 PM PT - May 24th, 2026

@robobun, your commit 50b36e2 has 2 failures in Build #57759 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 31370

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

bun-31370 --bun

@coderabbitai

coderabbitai Bot commented May 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: d9ff2762-1ddc-45e1-b15d-64b26eb6f070

📥 Commits

Reviewing files that changed from the base of the PR and between 2fbfcb9 and 50b36e2.

📒 Files selected for processing (2)
  • src/jsc/bindings/JSMockFunction.cpp
  • test/js/bun/test/mock-fn.test.js

Walkthrough

This PR adds explicit [[Construct]] trap support to mock functions. A new jsMockFunctionConstruct host function is declared and wired into the mock function's base class. The existing call logic is extracted into a shared helper, and both the call wrapper and new construct handler invoke this helper with appropriate thisValue binding. The construct handler creates an object from newTarget, invokes the mock implementation, and returns either the result (if an object) or the created instance (if the result is primitive).

Changes

Mock function constructor support

Layer / File(s) Summary
Declaration and base-class wiring
src/jsc/bindings/JSMockFunction.cpp
Declares jsMockFunctionConstruct host function and updates JSMockFunction base initialization to route [[Construct]] calls to the new handler instead of reusing jsMockFunctionCall.
Call refactoring and construct handler implementation
src/jsc/bindings/JSMockFunction.cpp
Extracts call logic into jsMockFunctionCallImpl(...) helper that accepts explicit thisValue. Implements jsMockFunctionCall as a thin wrapper and adds jsMockFunctionConstruct to create an empty object from newTarget, invoke the implementation with that this, and return the result if it is an object or the created instance otherwise.
Constructor behavior testing
test/js/bun/test/mock-fn.test.js
Adds comprehensive test suite verifying mock functions can be constructed with new, record calls as "return" results, bind this to the instance, handle primitive vs. object return values according to constructor semantics, and preserve prototype chains with alternate newTarget via Reflect.construct.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: adding proper object return handling when constructing mock functions, which directly matches the primary fix in the changeset.
Description check ✅ Passed The PR description comprehensively covers both required sections: detailed explanation of the problem and solution, plus verification through fuzzer repro, test cases, and multiple test runs.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. Fix crash constructing jest.fn() via Reflect.construct #30212 - Both fix the same isCell() assertion failure when constructing mock functions via Reflect.construct/new, by adding a dedicated jsMockFunctionConstruct callback in JSMockFunction.cpp

🤖 Generated with Claude Code

@robobun

robobun commented May 24, 2026

Copy link
Copy Markdown
Collaborator Author

Closing as a duplicate of #30212, which fixes the same isCell() assertion the same way (dedicated jsMockFunctionConstruct that allocates this from new.target's prototype via createSubclassStructure and returns it when the mock's result isn't an object). That PR is older, has broader test coverage (including constructing spies and a user-assigned fn.prototype), has been through review, and has CI history across several rebases — this one adds nothing on top of it. The repro that prompted this PR (Reflect.construct(mock(), []) with no implementation) is already covered by its tests.

@robobun robobun closed this May 24, 2026
@robobun robobun deleted the farm/4e0236f3/fix-mock-construct branch May 24, 2026 23:45
Comment on lines +996 to +1001
JSObject* newTarget = asObject(callframe->newTarget());
JSGlobalObject* functionGlobalObject = getFunctionRealm(lexicalGlobalObject, newTarget);
RETURN_IF_EXCEPTION(scope, {});
Structure* structure = InternalFunction::createSubclassStructure(lexicalGlobalObject, newTarget, functionGlobalObject->objectStructureForObjectConstructor());
RETURN_IF_EXCEPTION(scope, {});
JSObject* thisObject = JSC::constructEmptyObject(vm, structure);

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.

🟡 Now that new fn() returns an object, the common Jest assertion expect(new fn()).toBeInstanceOf(fn) is reachable — but it throws TypeError because JSMockFunction never installs a .prototype own property, so createSubclassStructure falls back to Object.prototype and OrdinaryHasInstance rejects the non-object fn.prototype. The missing .prototype is pre-existing and the construct callback itself is spec-correct, so this can be a follow-up: give each JSMockFunction a fresh .prototype object (as Jest's plain-JS mockConstructor gets automatically) so the default-new.target case wires up the prototype chain.

Extended reasoning...

What the bug is

jsMockFunctionConstruct derives the new instance's structure via InternalFunction::createSubclassStructure(lexicalGlobalObject, newTarget, functionGlobalObject->objectStructureForObjectConstructor()). When newTarget is the mock function itself (the ordinary new fn() case), createSubclassStructure reads newTarget.prototype. But JSMockFunction is an InternalFunction subclass whose create() only calls Base::finishCreation(vm) and sets m_originalName — it never does putDirectWithoutTransition(vm, vm.propertyNames->prototype, ...) the way other Bun InternalFunction constructors (e.g. JSBufferList, NodeVMScript, JSX509Certificate) do, and InternalFunction::finishCreation does not auto-create one. So fn.prototype is undefined.

Code path that triggers it

const fn = jest.fn();
const inst = new fn();
Object.getPrototypeOf(inst) === Object.prototype;   // true in Bun, false in Jest
inst instanceof fn;                                 // TypeError in Bun, true in Jest
expect(inst).toBeInstanceOf(fn);                    // throws in Bun, passes in Jest

Step-by-step:

  1. new fn() enters jsMockFunctionConstruct with callframe->newTarget() === fn.
  2. createSubclassStructure does newTarget->get(..., vm.propertyNames->prototype)undefined (no own .prototype, nothing on JSMockFunctionPrototype / Function.prototype).
  3. Since the result is not an object, it returns the supplied base structure, objectStructureForObjectConstructor(), whose prototype is Object.prototype.
  4. constructEmptyObject produces thisObject with [[Prototype]] = Object.prototype.
  5. Later, inst instanceof fn runs OrdinaryHasInstance: step 4 reads fn.prototypeundefined; step 5 ("If P is not an Object, throw a TypeError") throws.

Why existing code doesn't prevent it

The construct callback correctly implements OrdinaryCreateFromConstructor (fall back to %Object.prototype% when newTarget.prototype is not an object), so it is spec-compliant given its inputs. The gap is upstream: unlike a plain JS function mockConstructor() {} — which is what Jest uses and which gets an auto-created .prototype object — JSMockFunction never installs one. The new test only checks typeof instance === "object" and Reflect.construct(fn5, [], Base) (where Base supplies the prototype), so the default-new.target prototype chain is never asserted.

Impact

The PR description motivates this change with "Jest's class-mocking pattern (const instance = new MockedClass())", but the canonical assertion for that pattern — expect(instance).toBeInstanceOf(MockedClass) — throws in Bun while passing in Jest/Vitest. This is a real Jest-compat gap that becomes reachable for the first time now that new fn() returns an object instead of undefined.

That said, the root cause is pre-existing: jest.fn().prototype was already undefined before this PR, and ({}) instanceof jest.fn() already threw TypeError. This PR strictly improves behavior (assertion crash / undefined → an object), and the construct callback itself is correct.

How to fix

Give each JSMockFunction a fresh .prototype object in JSMockFunction::create (e.g. function->putDirect(vm, vm.propertyNames->prototype, constructEmptyObject(globalObject), PropertyAttribute::DontEnum)), mirroring what JSC does for ordinary JS functions and what other Bun InternalFunction constructors do explicitly. With that in place, createSubclassStructure will pick up fn.prototype, Object.getPrototypeOf(new fn()) === fn.prototype, and instanceof / toBeInstanceOf work as in Jest. This is somewhat orthogonal to the construct callback added here, so it's reasonable as a follow-up rather than a blocker for this crash fix.

Comment on lines +1001 to +1009
JSObject* thisObject = JSC::constructEmptyObject(vm, structure);

JSValue returnValue = JSValue::decode(jsMockFunctionCallImpl(lexicalGlobalObject, callframe, thisObject));
RETURN_IF_EXCEPTION(scope, {});
if (returnValue && returnValue.isObject()) {
return JSValue::encode(returnValue);
}

return JSValue::encode(thisObject);

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.

🟡 Now that new fn() actually works, it might be worth pushing thisObject into fn->instances here so fn.mock.instances[0] is the constructed instance — that's the canonical use case for mock.instances in Jest. This is technically pre-existing (Bun has never populated mock.instances for any call), so feel free to defer to a follow-up, but flagging since the PR description says this matches how Jest mocks behave when constructed.

Extended reasoning...

What's missing

JSMockFunction declares an instances array (exposed as fn.mock.instances), lazily initializes it in getInstances(), clears it in clear(), and visits it for GC — but nothing in the file ever pushes to it. jsMockFunctionCallImpl pushes to calls, contexts, invocationCallOrder, and returnValues, but not instances. The new jsMockFunctionConstruct creates thisObject and runs the mock implementation against it, but likewise never appends thisObject to fn->instances.

How Jest behaves

In Jest, mockConstructor is a plain JS function that pushes this to mock.instances on every invocation (call or construct). The Jest docs describe mock.instances as "an array that contains all the object instances that have been instantiated from this mock function using new". So after const inst = new fn(), Jest guarantees fn.mock.instances[0] === inst. This is the primary documented use case for mock.instances.

Why it surfaces with this PR

This is strictly a pre-existing gap — on main, mock.instances is always an empty array regardless of how the mock is invoked. However, before this PR, new fn() either asserted in debug builds or returned undefined in release, so the construct path was effectively unusable and nobody could meaningfully reach for mock.instances. This PR makes new fn() return a real object for the first time and explicitly states it "matches how Jest mocks behave when constructed", which makes the empty mock.instances array much more visible.

Step-by-step example

With this PR applied:

const fn = jest.fn(function () { this.x = 42; });
const inst = new fn();
  1. jsMockFunctionConstruct runs, allocates thisObject from new.target.prototype.
  2. It calls jsMockFunctionCallImpl(..., thisObject), which pushes [ ] to fn->calls, thisObject to fn->contexts, an id to invocationCallOrder, and a result object to returnValuesbut never touches fn->instances.
  3. The implementation returns undefined (not an object), so jsMockFunctionConstruct returns thisObject.
  4. inst is the constructed object, fn.mock.contexts[0] === inst ✅, but fn.mock.instances is still [] ❌. In Jest, fn.mock.instances[0] === inst.

Impact

Not a regression and not a crash — mock.contexts[0] already captures the same value, and the PR's stated goal (fixing the isCell() assertion) is fully achieved. But users who follow Jest's docs for testing class mocks (expect(fn.mock.instances[0]).toBe(instance)) will find an empty array.

Suggested fix

In jsMockFunctionConstruct, after creating thisObject (or after the call returns), push it to fn->instances the same way jsMockFunctionCallImpl pushes thisValue to fn->contexts. For full Jest parity, jsMockFunctionCallImpl would also push thisValue to instances on every call (Jest pushes this unconditionally), but at minimum the construct path should populate it. Fine as a follow-up if you'd rather keep this PR scoped to the assertion fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant