Fix crash when constructing jest.fn() via Reflect.construct#31164
Fix crash when constructing jest.fn() via Reflect.construct#31164robobun wants to merge 2 commits into
Conversation
JSMockFunction used the same native function for both [[Call]] and [[Construct]]. When used as a constructor, JSC's Interpreter::executeConstruct asserts the result is an object via asObject(). A mock with no implementation (or one that returns a primitive) returned a non-object, tripping the assertion. Split the call/construct entry points. In the construct path, allocate a fresh object for 'this' and return it when the mock result is not an object, matching ordinary [[Construct]] semantics.
|
Updated 10:08 PM PT - May 20th, 2026
❌ @robobun, your commit 271bf5b has 2 failures in
🧪 To try this PR locally: bunx bun-pr 31164That installs a local version of the PR into your bun-31164 --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 constructor support to Jest mock functions in JSC. Mock functions now correctly handle the ChangesJest mock function constructor support in JSC
🚥 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (isConstruct) { | ||
| thisValue = JSC::constructEmptyObject(globalObject); | ||
| } |
There was a problem hiding this comment.
🔴 The synthesized this is created with constructEmptyObject(globalObject), which always uses Object.prototype and ignores callframe->newTarget(). As a result new (jest.fn())() instanceof fn and Reflect.construct(jest.fn(), [], Foo) instanceof Foo are both false, whereas in Jest (and per ordinary [[Construct]]) they are true. Consider deriving the prototype from newTarget (e.g. via InternalFunction::createSubclassStructure / getFunctionRealm, as done in NodeVMScript.cpp and other native constructors here).
Extended reasoning...
What the bug is
In the new construct path, the freshly-allocated this is created with the single-argument overload JSC::constructEmptyObject(globalObject). That overload returns a JSFinalObject whose [[Prototype]] is globalObject->objectPrototype() — i.e. plain Object.prototype. It never consults callframe->newTarget().
Per the ECMAScript [[Construct]] internal method (step OrdinaryCreateFromConstructor), the new object's [[Prototype]] must come from newTarget.prototype. The PR description explicitly says the goal is to "match ordinary [[Construct]] semantics and Jest's behavior", but for the prototype chain it matches neither: in Jest, mock functions are real JS functions, so new fn() goes through ordinary [[Construct]] and the instance inherits from fn.prototype.
Code path that triggers it
if (isConstruct) {
thisValue = JSC::constructEmptyObject(globalObject); // <-- always Object.prototype
}callframe->newTarget() is available here (this is the native-construct entry point) but is never read. Whether the call comes from new fn(), Reflect.construct(fn, []), or Reflect.construct(fn, [], Foo), the resulting object's prototype is the same: Object.prototype.
Why nothing else prevents it
The PR's own "with explicit newTarget" test only asserts typeof result === 'object', which a plain {} satisfies. It would fail if it asserted result instanceof newTarget — which is the observable contract newTarget exists to provide. None of the other added tests check the prototype chain either.
Step-by-step proof
const fn = jest.fn();
const inst = new fn();
Object.getPrototypeOf(inst) === Object.prototype // true (bug) — should be fn.prototype
inst instanceof fn // false (bug) — true in Jest
class Foo {}
const r = Reflect.construct(jest.fn(), [], Foo);
Object.getPrototypeOf(r) === Foo.prototype // false (bug) — should be true
r instanceof Foo // false (bug) — true in Jest
fn.prototype.hello = () => 1;
new fn().hello // undefined (bug) — defined in JestIn each case, constructEmptyObject(globalObject) produced { __proto__: Object.prototype }, so OrdinaryHasInstance walks the chain, never finds fn.prototype / Foo.prototype, and returns false.
Impact
Not a crash and strictly better than the pre-PR behavior (which crashed/returned undefined), but it's an observable correctness divergence in the exact lines this PR adds. Tests that mock a constructor and then assert expect(new Mock()).toBeInstanceOf(Mock) — which pass under Jest — will fail. Methods placed on fn.prototype are unreachable from constructed instances.
How to fix
Use the same pattern this codebase already uses in ~15 native constructors (e.g. NodeVMScript.cpp, JSX509Certificate.cpp, NapiClass.cpp): read callframe->newTarget(), resolve its function realm, derive a structure via InternalFunction::createSubclassStructure (falling back to a default if newTarget.prototype isn't an object), and pass that structure to constructEmptyObject. Then new fn() instanceof fn and Reflect.construct(fn, [], Foo) instanceof Foo will both hold, matching Jest.
| @@ -0,0 +1,72 @@ | |||
| import { describe, expect, jest, spyOn, test } from "bun:test"; | |||
There was a problem hiding this comment.
🟡 nit: Per CLAUDE.md (root and test/CLAUDE.md), tests should be added to the existing file rather than creating a new one. test/js/bun/test/mock-fn.test.js already covers jest.fn() behavior — these construct tests would fit there as a new describe block (and that file is wired up via test-interop.js to run under real Jest/Vitest, which would also validate the parity claim).
Extended reasoning...
The repo's contributor guidelines explicitly call out test placement. Root CLAUDE.md (line 55) says "Default: add your test to the existing test file for the code you're changing. Do not create a new file," and test/CLAUDE.md (line 152) repeats the same rule. This PR creates a brand-new test/js/bun/test/mock-fn-construct.test.ts instead of extending the existing test suite for mock functions.
The existing canonical file is test/js/bun/test/mock-fn.test.js (~1000 lines), which already exercises jest.fn(), mock(), spyOn(), mockReturnValue, mockReturnThis, and friends. The new Reflect.construct cases are squarely in that file's domain — a new describe("Reflect.construct on mock functions") block would slot in naturally alongside the existing coverage.
There's also a concrete benefit beyond convention: mock-fn.test.js is intentionally written to run under Bun, real Jest, and Vitest via test-interop.js. The PR description claims the new construct semantics "match ordinary [[Construct]] semantics and Jest's behavior." Putting the tests in the cross-runner file would actually verify that claim against real Jest, whereas the new standalone file imports from bun:test directly and only ever runs under Bun.
Step-by-step:
- PR adds tests for
jest.fn()construct behavior → falls under "mock function behavior." test/js/bun/test/mock-fn.test.jsexists and is the designated home for that surface area.- CLAUDE.md says: existing file → add there, don't create a new one.
- New file was created instead → convention violation.
The only minor friction is that the new file uses TypeScript annotations (this: { x: number }, as never, Record<string, unknown>) while the existing file is .js. These are trivial to strip — function () { this.x = 1 } works fine without the this annotation, and the casts can be dropped or replaced with // @ts-ignore / plain {}. Not enough to justify a separate file.
Fix: Move the describe("Reflect.construct on mock functions") block into test/js/bun/test/mock-fn.test.js, drop the TS-only syntax, and delete mock-fn-construct.test.ts. Non-blocking — purely organizational.
| if (isConstruct) { | ||
| thisValue = JSC::constructEmptyObject(globalObject); | ||
| } |
There was a problem hiding this comment.
🟣 Pre-existing (non-blocking): mock.instances is still never populated — fn->instances is declared, GC-visited, cleared, and exposed on the mock object, but nothing ever pushes to it. Now that there's a dedicated construct branch with the freshly-allocated thisValue in hand, this would be the natural place to also push it onto fn->instances to match Jest's mock.instances semantics. Fine as a follow-up.
Extended reasoning...
What the issue is
In Jest, mockFn.mock.instances is the array of this values created when the mock is invoked as a constructor (via new or Reflect.construct). In Bun's implementation, the storage for this exists end-to-end — fn->instances is declared as a WriteBarrier<JSArray> on JSMockFunction, lazily created in getInstances(), cleared in clear(), visited by the GC, and wired into the mock object structure at offset 2 — but nothing in JSMockFunction.cpp ever pushes a value into it. As a result, fn.mock.instances is always [] regardless of how the mock is invoked.
Code path
In jsMockFunctionCallOrConstruct, every invocation pushes into fn->calls, fn->contexts, fn->invocationCallOrder, and fn->returnValues. There is no corresponding push into fn->instances. With this PR, the isConstruct branch now explicitly synthesizes thisValue = JSC::constructEmptyObject(globalObject) — which is exactly the value Jest records in mock.instances — but the value is only used as the call receiver and the fallback return; it's never recorded in instances.
Why nothing currently catches this
Existing tests in test/js/bun/test/mock-fn.test.js only assert that mock.instances is empty after mockClear() / mockReset(), which trivially passes since the array is never populated in the first place. No test asserts that constructing a mock adds an entry, so the gap is silent.
Step-by-step proof
const Fn = jest.fn(); const inst = new Fn();- JSC dispatches to
jsMockFunctionConstruct→jsMockFunctionCallOrConstruct(..., /*isConstruct*/ true). thisValueis set to a fresh empty object.- The function pushes to
calls,contexts,invocationCallOrder, andreturnValues, then returnsencodeReturn(jsUndefined())→thisValue. instis the fresh object, butFn.mock.instancesis still[]— in Jest it would be[inst].
Why this is pre-existing
This gap predates the PR entirely. Before this change, jsMockFunctionCall was used for both [[Call]] and [[Construct]] and likewise never touched fn->instances. The PR neither introduces nor worsens the behavior; its scope is fixing a debug-build assertion crash, and it succeeds at that. This comment is raised only because the new explicit construct branch is the obvious and correct place to add the missing push, and the PR description references matching "Jest's behavior" for construct.
Suggested fix (follow-up)
In the if (isConstruct) block (or alongside the other array pushes), push thisValue onto fn->instances, mirroring the pattern used for contexts:
if (isConstruct) {
thisValue = JSC::constructEmptyObject(globalObject);
JSC::JSArray* instances = fn->instances.get();
if (instances) {
instances->push(globalObject, thisValue);
RETURN_IF_EXCEPTION(scope, {});
} else {
JSC::ObjectInitializationScope object(vm);
instances = JSC::JSArray::tryCreateUninitializedRestricted(
object,
globalObject->arrayStructureForIndexingTypeDuringAllocation(JSC::ArrayWithContiguous),
1);
instances->initializeIndex(object, 0, thisValue);
fn->instances.set(vm, fn, instances);
}
}(Note: Jest actually pushes this for every invocation, not just construct calls, so a fuller fix might push unconditionally — but that's a larger compat decision and definitely follow-up territory.)
|
Duplicate of #30212 which has a more complete fix (honors |
1 participant
What
Fixes an assertion failure (
cell->isObjectSlow()inJSObject.h) when a mock function is invoked as a constructor throughReflect.construct(or any native-construct path).Why
JSMockFunctionregistered the same native function for both[[Call]]and[[Construct]]. When invoked as a native constructor, JSC'sInterpreter::executeConstructunconditionally wraps the result withasObject(), which asserts the value is an object. A mock with no implementation (or one returning a primitive /mockReturnValue(primitive)) returnedundefinedor a primitive, tripping the assertion.The
new fn()bytecode path happened not to assert, but still returnedundefined, which is also wrong per[[Construct]]semantics.How
Split the call/construct entry points. In the construct path, allocate a fresh
thisobject up front, pass it to the implementation asthis, and return it when the mock's result is not an object — matching ordinary[[Construct]]semantics and Jest's behavior.Found by Fuzzilli.