Fix crash when constructing a mock function that returns a primitive

[robobun]

What

Fixes a debug assertion (cell->isObjectSlow() in JSObject.h:1345) when a jest.fn() / mock() is invoked as a constructor and its implementation returns a non-object value.

const fn = mock(Symbol);
Reflect.construct(fn, []); // ASSERTION FAILED: cell->isObjectSlow()

Also affected: mock(() => "str"), mock(BigInt), mock().mockReturnValue(42), and mock() with no implementation — any construct path whose result isn't an object.

Why

jsMockFunctionCall was registered as both the [[Call]] and [[Construct]] native for JSMockFunction. Native constructors must return an object; the mock was returning whatever its implementation returned (or jsUndefined()). Reflect.construct then calls asObject() on the result and asserts. In release builds this doesn't crash but new fn() incorrectly returns primitives.

Since CallFrame::newTarget() is just an alias for thisValue(), a single shared native can't distinguish call from construct, so the handler is split into separate jsMockFunctionCall and jsMockFunctionConstruct entry points.

The construct path now:

• allocates a fresh this object using new.target.prototype
• passes it as this to the implementation
• returns it whenever the implementation's result is not an object

This matches ordinary JS [[Construct]] semantics (function F() { return "x"; } new F() → the created object) and how Jest's mock constructor behaves.

Found by Fuzzilli.

[robobun]

^{Updated 1:18 PM PT - May 19th, 2026}

❌ @robobun, your commit b393c56 has 2 failures in Build #56194 (All Failures):

• Failed to create agent for 🪟 aarch64 - build-rust
• scripts/build/ci.ts - build failed on 🍎 x64 - build-cpp

---

🧪   To try this PR locally:

bunx bun-pr 31094

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

bun-31094 --bun

[coderabbitai]

Caution

Review failed

Pull request was closed or merged during review

Walkthrough

Mock functions can now be used as constructors. The implementation adds a separate JSC call path for construct invocations, derives thisValue from newTarget.prototype, and ensures primitive-returning mocks yield constructed objects while object-returning mocks return the provided object. Tests cover return-value handling, new.target prototype behavior, and regression cases.

Changes

Mock Function Constructor Support

Layer / File(s)	Summary
JSC Construct Call Wiring src/jsc/bindings/JSMockFunction.cpp	JSC InternalFunction initialization is updated to route construct calls to the new jsMockFunctionConstruct handler instead of reusing the call path.
Construct-Specific Implementation src/jsc/bindings/JSMockFunction.cpp	Shared helper jsMockFunctionCallOrConstruct accepts an isConstructCall flag, derives thisValue from newTarget.prototype, and uses an encodeReturn lambda that returns constructed thisValue for primitive/undefined returns and the object value for object returns.
Return Value Encoding Unification src/jsc/bindings/JSMockFunction.cpp	All mock result paths (rejected, return-this, return-value) switch from direct JSValue::encode to encodeReturn for consistent construct-call encoding.
Construct Entry Point and Default Returns src/jsc/bindings/JSMockFunction.cpp	Default undefined return uses encodeReturn, and the new jsMockFunctionConstruct host function calls the helper with isConstructCall=true.
Constructor Behavior Tests test/js/bun/test/mock-fn.test.js	New suite validates primitive/object-returning implementations, no-implementation construction, mockReturnValue consistency, new.target prototype assignment, and a Bun-specific non-object-cell regression case with forced GC.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main fix: resolving a crash when constructing mock functions that return primitives.
Description check	✅ Passed	The description covers both required sections: 'What does this PR do?' (detailed explanation of the fix) and 'How did you verify your code works?' (test cases added).
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.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/jsc/bindings/JSMockFunction.cpp`:
- Around line 843-861: The encodeReturn lambda only adjusts the encoded value
seen by callers but does not normalize what gets stored in mock.results or
update mock.instances for constructor calls; update the write sites that push
`"return"` results (the Kind::Call, Kind::ReturnValue, Kind::RejectedValue
branches and the default-undefined branch) to pass the normalized value (use
normalizeReturn(...) instead of the raw value or JSValue::encode(...)) and, when
isConstructCall is true, ensure the constructor path also appends thisValue to
mock.instances so mock.results and mock.instances reflect the actual constructed
object; locate the encodeReturn lambda and the branches handling mock result
pushes in JSMockFunction.cpp and replace the stored values accordingly and add
the thisValue push into the construct path.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 54062884-725b-48fd-b962-e0315a22e0f7

📥 Commits

Reviewing files that changed from the base of the PR and between 184d037 and 8a25957.

📒 Files selected for processing (2)

• src/jsc/bindings/JSMockFunction.cpp
• test/js/bun/test/mock-fn.test.js

[github-actions]

This PR may be a duplicate of:

1. Fix crash constructing jest.fn() via Reflect.construct #30212 - Same fix for crash when jest.fn()/mock() is used as a constructor returning a primitive; both split jsMockFunctionCall into separate call/construct handlers in JSMockFunction.cpp

🤖 Generated with Claude Code

[robobun]

Duplicate of #30212, which already has the same fix. Closing in favor of that one.

One thing this PR had that #30212 doesn't: populating mock.instances on constructor calls (matching jest-mock). Worth carrying over.

[claude]

🟣 Pre-existing nit (not blocking): fn->instances is still never populated, so fn.mock.instances will remain [] after new fn() even though Jest records the constructed this there. Since this PR now synthesizes the correct thisValue for the construct path, it would be a natural place to also push it onto fn->instances (mirroring how calls/contexts/results are pushed) — but that can also land separately.

Extended reasoning...

What's missing

JSMockFunction declares mutable JSC::WriteBarrier<JSC::JSArray> instances;, clears it in clear(), GC-visits it, lazily initializes an empty array in getInstances(), and exposes it on the mock object at offset 2 as the instances property. However, nothing in jsMockFunctionCallOrConstruct (or anywhere else) ever pushes to it. The function pushes to fn->calls, fn->contexts, fn->invocationCallOrder, and fn->returnValues on every invocation, but fn->instances is dead storage that always reads back as [].

Why it's relevant to this PR

In Jest, mockFn.mock.instances is documented to record the this that was bound for each invocation — for a constructor call, that's the freshly-allocated instance. Before this PR, new fn() on a mock that returned a primitive would assert in debug builds and return a primitive in release builds, so the fact that mock.instances stayed empty was largely unobservable (you couldn't really use mocks as constructors anyway). After this PR, new fn() works correctly and the construct path explicitly allocates the right thisValue via constructEmptyObject(globalObject, prototype) — so users who can now construct mocks may reasonably reach for fn.mock.instances and find it empty.

Step-by-step

1. const fn = jest.fn(function () { this.x = 1; });
2. const inst = new fn(); → enters jsMockFunctionConstruct → jsMockFunctionCallOrConstruct(..., true).
3. isConstructCall is true, so thisValue = constructEmptyObject(globalObject, prototype) — this is the instance.
4. The function pushes argumentsArray to fn->calls, thisValue to fn->contexts, the invocation id to fn->invocationCallOrder, and the result record to fn->returnValues. fn->instances is never touched.
5. encodeReturn(returnValue) returns thisValue (the impl returned undefined, a non-object), so inst is the constructed object — correct.
6. fn.mock.instances → getInstances() → lazy-creates and returns an empty array. Expected (Jest): [inst]. Actual: [].

Why nothing prevents it

There's simply no write site. Grepping the file shows instances only at the declaration, clear(), getInstances(), the visitor, and the mock-object structure setup. The existing test suite only asserts fn.mock.instances is empty after mockClear/mockReset, never that it's populated, so no test catches this.

Impact

Low — it's a Jest-compat gap, not a crash or correctness issue in the PR's stated scope (a Fuzzilli-found assertion fix). Users porting Jest tests that assert on mock.instances (e.g. expect(fn.mock.instances[0]).toBe(inst)) will see failures.

Suggested fix (optional, can be a follow-up)

In jsMockFunctionCallOrConstruct, alongside the contexts push, also push thisValue to fn->instances (Jest pushes this for every call, not just constructs — non-construct calls record whatever this was, and plain fn() records undefined):

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);
}

Severity

Pre-existing / nit. This gap predates the PR entirely — instances was never populated for any kind of call. The PR is a targeted crash fix and shouldn't be blocked on this; it's flagged only because the PR is restructuring exactly the construct path that now has the right value to record.