fix(bun:test): constructing a mock function must return an object#31381
fix(bun:test): constructing a mock function must return an object#31381robobun wants to merge 1 commit into
Conversation
Mock functions registered the call callback as their construct callback, so `new mockFn()` / `Reflect.construct(mockFn, [])` could produce a non-object value (e.g. undefined), violating the [[Construct]] contract and failing the isCell assertion in JSC when the result was used. Add a dedicated construct callback that creates an instance from newTarget's prototype, runs the mock implementation with it as this, and returns the implementation result only when it is an object.
|
Warning Review limit reached
Your plan includes 5 reviews of capacity. Refill in 30 minutes and 25 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 2:37 AM PT - May 25th, 2026
❌ @robobun, your commit 6fbf94d has 2 failures in
🧪 To try this PR locally: bunx bun-pr 31381That installs a local version of the PR into your bun-31381 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Closing as a duplicate of #30212, which fixes this same assertion the same way: separate #30212 is further along — it has been through review, its latest CI run is green, and it already carries regression coverage for this exact entry point (constructing a spy created by |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| JSObject* thisObject = prototype | ||
| ? JSC::constructEmptyObject(lexicalGlobalObject, prototype) | ||
| : JSC::constructEmptyObject(lexicalGlobalObject); | ||
|
|
||
| JSValue result = JSValue::decode(jsMockFunctionCallImpl(lexicalGlobalObject, callframe, thisObject)); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
|
|
||
| // A [[Construct]] call must always produce an object. If the mock implementation returned a | ||
| // primitive, return the newly created instance instead, matching ordinary constructor semantics. | ||
| if (result && result.isObject()) | ||
| return JSValue::encode(result); | ||
|
|
||
| return JSValue::encode(thisObject); |
There was a problem hiding this comment.
🟣 Not blocking, and pre-existing, but worth a follow-up: mock.instances is still never populated — fn->instances is only declared, cleared, lazily created empty, and GC-visited, never written to — so even with the new construct path, new fn(); fn.mock.instances is [] where Jest gives [<instance>]. Since this PR now creates the very thisObject that mock.instances is meant to record, it'd be a natural place to push it (and, to fully match Jest, push thisValue on every call alongside the contexts push in jsMockFunctionCallImpl).
Extended reasoning...
What this is
JSMockFunction has an instances field (line 271) that backs mockFn.mock.instances, but nothing in the file ever writes to it. Grepping confirms it appears only at:
- line 271 — the
WriteBarrier<JSArray>declaration, - line 350 —
clear()clears it, - lines 420–425 —
getInstances()lazily creates an empty array, - line 483 — GC visit,
- line 757 — the
"instances"slot in the mock object structure.
There is no instances->push(...) or initializeIndex call anywhere, so fn.mock.instances is always an empty array — both before and after this PR. In Jest, mock.instances records the this value for every invocation (parallel to mock.contexts), and is most useful exactly for the new fn() case this PR is adding.
Why mention it on this PR
This is a pre-existing gap — instances has never been populated in Bun, and this PR does not regress it. The refutation correctly notes that the PR's stated scope is the isCell() crash fix and that "matching Jest's behavior" in the description refers to the primitive-return-replaced-by-this semantics, not full mock.instances parity. That's all fair, and this should not block the PR.
It is still worth a non-blocking note because the new jsMockFunctionConstruct is the first place in this file that actually creates the instance object that mock.instances is supposed to hold. Before this PR there was no obvious hook; now there is, and it's two lines away from where the value would be pushed. Flagging it as a "while you're here" follow-up is reasonable; landing it separately is equally fine.
Step-by-step proof
const fn = jest.fn();const inst = new fn();— entersjsMockFunctionConstruct(line 989), allocatesthisObject(lines 1003–1005), callsjsMockFunctionCallImplwiththisValue = thisObject.jsMockFunctionCallImplpushes ontofn->calls,fn->contexts,fn->invocationCallOrder, andfn->returnValues(lines 854–910). It never touchesfn->instances.jsMockFunctionConstructreturnsthisObject(line 1015).fn->instancesis still unset.fn.mock.instancestriggersgetInstances()(line 420), which lazily creates an empty array. Result:[].- In Jest the same code yields
[inst].
Suggested follow-up fix
In jsMockFunctionCallImpl, right next to the existing contexts push (lines 868–880), add an analogous block that pushes thisValue onto fn->instances. That matches Jest exactly: Jest records this into mock.instances on every call (for plain calls it's the receiver, for new it's the freshly created instance), so doing it only in jsMockFunctionConstruct would still diverge for non-construct calls. Doing it in the shared impl covers both, and the new construct path already threads the right thisObject through as thisValue.
1 participant
What does this PR do?
Fixes a crash found by fuzzing (fingerprint
JSCJSValue.h(1043),ASSERTION FAILED: isCell()).JSMockFunction(the object behindjest.fn()/mock()/spyOn()inbun:test) registered its call callback as its construct callback too. When a mock was constructed and its implementation produced a non-object (e.g. no implementation at all, ormockReturnValue(5)), the construct callback returned that primitive. A[[Construct]]call must always produce an object, so paths that go throughJSC::construct()—Reflect.construct(mockFn, []),super()on a mocked class, proxies, etc. — ended up callingasObject()on a non-cell value. That trips theisCell()assertion in debug builds and produces a bogus "object" pointer in release builds. Plainnew mockFn()was also wrong: it evaluated toundefined.This PR gives
JSMockFunctiona dedicated construct callback that:newTarget.prototype(falling back toObject.prototype),this(so calls/contexts/results are still recorded andmockReturnThis()returns the instance),How did you verify your code works?
mock() > are constructabletest totest/js/bun/test/mock-fn.test.jscoveringnew mockFn()/Reflect.constructwith no implementation, primitive return values, object return values, and throwing implementations. The file keeps its Jest/Vitest interop, and these assertions match Jest's behavior.bun bd test test/js/bun/test/mock-fn.test.js,mock-disposable.test.ts,spyMatchers.test.ts,expect-toHaveReturnedWith.test.js,expect/toHaveReturnedWith.test.ts, andtest/regression/issue/issue-1825-jest-mock-functions.test.tsall pass.