Skip to content

Fix crash when using Reflect.construct on a mock function#29790

Closed
robobun wants to merge 2 commits into
mainfrom
farm/b694b77c/mock-fn-reflect-construct
Closed

Fix crash when using Reflect.construct on a mock function#29790
robobun wants to merge 2 commits into
mainfrom
farm/b694b77c/mock-fn-reflect-construct

Fix crash when using Reflect.construct on a mock function

cdac378
Select commit
Loading
Failed to load commit list.
Claude / Claude Code Review completed Apr 27, 2026 in 20m 21s

Code review found 2 potential issues

Found 4 candidates, confirmed 2. See review comments for details.

Details

Severity Count
🔴 Important 0
🟡 Nit 2
🟣 Pre-existing 0
Severity File:Line Issue
🟡 Nit src/bun.js/bindings/JSMockFunction.cpp:997 Construct fallback ignores newTarget.prototype
🟡 Nit test/js/bun/test/mock-fn.test.js:809-810 spyOn non-function property breaks Jest/Vitest compat

Annotations

Check warning on line 997 in src/bun.js/bindings/JSMockFunction.cpp

See this annotation in the file changed.

@claude claude / Claude Code Review

Construct fallback ignores newTarget.prototype

Nit: the fallback uses `constructEmptyObject(lexicalGlobalObject)`, which always inherits from `Object.prototype` and ignores `callframe->newTarget()`. So `Reflect.construct(jest.fn(), [], SomeClass)` returns an object that is *not* `instanceof SomeClass`, whereas Jest (ordinary `[[Construct]]`) would honor `newTarget.prototype`. Not blocking — the crash fix is a strict improvement — but you could use `InternalFunction::createSubclassStructure` (as in `NapiClass.cpp` / `JSDOMFile.cpp`) to derive

Check warning on line 810 in test/js/bun/test/mock-fn.test.js

See this annotation in the file changed.

@claude claude / Claude Code Review

spyOn non-function property breaks Jest/Vitest compat

nit: `spyOn({ foo: 42 }, "foo")` on a non-function property is Bun-specific — Jest throws `Cannot spy the foo property because it is not a function`. Since this file is meant to be runnable in Jest/Vitest (per the header), these two lines should be wrapped in `if (isBun) { ... }`, matching the existing `spyOn works on object` test which is guarded for the same reason.