Repository navigation
Conversation
determineSpecificType() built the "function ..." rendering from
Zig::functionName(), the stack-trace heuristic. That returns an empty
string for a callable Proxy and ignores anything the name property says
(bound functions read as "f" instead of "bound f" until .name has been
materialized, redefined names and getters were skipped).
Node renders `function ${value.name}`, so read the name property through
an ordinary [[Get]] and stringify it, letting a throwing getter or trap
propagate the way the object branch already does for .constructor.
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: ready for review (head d6fed24). Reproduced on the released binary (bun 1.4.0) and on main with: try { process.umask(new Proxy(function codegen() {}, {})); } catch (e) { console.log(e.message); }
// bun: The "mask" argument must be of type number. Received function
// node: The "mask" argument must be of type number. Received function codegen
CI: build 95942 (same source change) was green apart from two darwin jobs that expired waiting for an agent. The rerun, build 96344, is red only on three Windows tests this change does not touch: |
There was a problem hiding this comment.
LGTM — focused Node-compat fix that mirrors the existing object-branch pattern in the same function.
What was reviewed:
- The callable branch now does
[[Get]]ofname+toString+viewwithRETURN_IF_EXCEPTIONafter each — identical structure to the.constructorlookup at ErrorCode.cpp:498-510, so callers are already written for a pending exception here. - Checked that
DECLARE_TOP_EXCEPTION_SCOPEis in scope (line 377) and that the object branch already runs user code viavalue.get(... constructor), so this adds no new caller obligation. - Test drives the renderer through all four entry-point families (C++ validator, JS builtin
$ERR_INVALID_ARG_TYPE, Rust FFI,createInvalidThisError) with 16 callable shapes plus throwing-getter/trap/revoked-proxy propagation, and pins the non-callable-Proxy path as unchanged.
Extended reasoning...
Overview
Changes the callable branch of determineSpecificType() in src/jsc/bindings/ErrorCode.cpp from using Zig::functionName() (a stack-trace heuristic that never runs user code and returns "" for proxies) to Node's actual algorithm: value.name via [[Get]], coerced to string. Adds a comprehensive test file exercising the renderer through four distinct entry points.
Security risks
None. This is error-message rendering. The only new behavior is that reading .name on a callable can now run a user getter or Proxy trap — but the object branch of the same function (line 499) has always done exactly this for .constructor, so every caller is already written for it, and the exception-check discipline (RETURN_IF_EXCEPTION after get, toString, and view) is complete. The PR author verified the BUN_JSC_validateExceptionChecks=1 lane is clean.
Level of scrutiny
Low-to-medium. This is a ~12-line change to error-message text formatting, not a hot path or a security boundary. The new code is a near-verbatim copy of the pattern 60 lines below in the same function. The change matches Node's lib/internal/errors.js semantics precisely (function ${value.name}), and the PR description traces the mechanism, cites the Node source, and documents which existing renderings are unchanged.
Other factors
- Test coverage is unusually thorough: 4 entry-point families × 16 callable shapes, plus explicit assertions that a throwing
namegetter/trap and a revoked proxy propagate the user's error rather than being swallowed, plus a negative test that non-callable Proxies still take the object branch. - Verified to fail on
USE_SYSTEM_BUN=1(8/9) and pass on the branch, per the repo's test-validity requirement. - The 59 vendored node parallel tests using
common.invalidArgTypeHelperwere re-run with no new failures. Bun__ErrorCode__determineSpecificType(line 518) andcreateInvalidThisError(line 1811) don't explicitly check for an exception after the call, but they didn't before either — the object branch could already leave one pending — so this PR does not change any caller contract.
|
On the note in the review about the two callers that do not check for an exception right after the call: the Rust entry ( |
|
Updated 10:56 AM PT - Aug 14th, 2026
⏳ @robobun, your commit d6fed24 is still building in
|
There was a problem hiding this comment.
LGTM — the earlier comment-length nit is addressed in d6fed24; the callable branch is now a one-line pointer at node's source.
What was reviewed
- Exception handling:
RETURN_IF_EXCEPTIONafter each of.get()/.toString()/.view()under the function's existingDECLARE_TOP_EXCEPTION_SCOPE, matching the neighboring bigint/string branches; the object branch already runs user code via.constructorso callers are already written for a pending exception here. - Test coverage: 4 entry points (C++ / JS builtins / Rust FFI /
createInvalidThisError) × 16 callable shapes, plus throwing trap/getter/revoked-proxy propagation and a non-callable-Proxy negative case;thrownBythrows if the call doesn't, so no vacuous assertions. - Checked that the anonymous-function rendering (
functionwith trailing space) is unchanged and still pinned by both this test and the existingprocess.umaskassertion.
Extended reasoning...
Overview
An 8-line change to the callable branch of determineSpecificType() in src/jsc/bindings/ErrorCode.cpp: replaces Zig::functionName() (side-effect-free stack-trace helper that returns "" for ProxyObjectType and reads only internal function state) with node's algorithm — [[Get]] of .name, toString, append — with RETURN_IF_EXCEPTION after each step. Plus a 137-line test file exercising the renderer through four distinct entry points.
The prior review round left a nit about the source comment being four lines of change narration; the author trimmed it to a single-line pointer at node's lib/internal/errors.js in d6fed24, matching the style of the string branch's node permalink at line 453. All inline threads (comment-cop ×2, my prior nit) are marked resolved.
Security risks
None material. The change means user code (a .name getter or Proxy get trap) can now run while building an error message for a callable, but (a) the object branch immediately below has always done this for .constructor, so callers already tolerate it, (b) node does the same thing, and (c) the worst outcome is that the user's own exception replaces the ERR_INVALID_ARG_TYPE being built — which the test explicitly asserts across all four entry points including createInvalidThisError.
Level of scrutiny
Low-medium. This is error-message rendering on an already-failing validation path — cold code by definition. The mechanical concern (running user JS mid-C++ with proper exception scoping) is handled: the function already has a DECLARE_TOP_EXCEPTION_SCOPE, the three new fallible calls each have RETURN_IF_EXCEPTION, and the author verified with BUN_JSC_validateExceptionChecks=1. The pattern is byte-for-byte the same as the neighboring bigint branch (lines 405-408).
Other factors
- Test quality is high: table-driven over 4 entry points × 16 value shapes, fresh values per row so JSC's lazy
.namereification is actually exercised, negative case for non-callable proxies, and a throwing-getter/trap/revoked-proxy row per entry point.thrownBythrows if the call doesn't, so assertions can't be silently skipped. 8 of 9 test blocks fail underUSE_SYSTEM_BUN=1. - The PR description enumerates every caller of
determineSpecificTypeand traces exception propagation for each, including thecreateInvalidThisError→ErrorCodeCache::createErrorpath that returns the pending exception's value in place of the new error. - Previous CI run was 177/179 green with the two failures being darwin agent expiry unrelated to the change; commits since then only touched the comment.
|
A second report of this bug arrived, with Buffer.from((function b() {}).bind(null)); // bun: Received function b node: Received function bound b
Buffer.from(new Proxy(function target() {}, {})); // bun: Received function node: Received function targetBoth lines still reproduce on main ( #43005 depends on this fix for one assertion. Its test "send() of a function" in |
Problem
The
Received ...suffix ofERR_INVALID_ARG_TYPErenders a callableProxyasfunctionwith no name. Node prints the target's name:Every Node-style validator shares this rendering (74 of the ~100 entry points I tried, across
process,Buffer,crypto,zlib,url,fs,events,child_process, ...), as does theERR_INVALID_THISmessage native classes build for a wrongthis.Cause: the callable branch of
determineSpecificType()(src/jsc/bindings/ErrorCode.cpp:430) gets the name fromZig::functionName()(src/jsc/bindings/ErrorStackTrace.cpp:547), which returns an empty string forProxyObjectTypeby design and otherwise only looks at internal function state. Node'sdetermineSpecificType(lib/internal/errors.js) isfunction ${value.name}.The same shortcut produces other divergences from node: a bound function renders as
function finstead offunction bound funless something happened to read.namefirst (JSC materializes it lazily), and a redefinedname(data value or getter) is ignored.Fix
[[Get]]ofnameon the value,ToStringit, append. Exceptions are checked after each step and left pending for the caller, which is the contract the object branch (.constructorlookup a few lines below) already has. The C++ message builders andJSBuffer.cppcheck right after the call, the Rust FFI entry is checked bydetermine_specific_typeinsrc/jsc/JSGlobalObject.rs, andcreateInvalidThisErrordoes not check but hands the message toErrorCodeCache::createError, which returns the pending exception's value in place of the new error, so its callers end up throwing the user's exception (the test covers this path).gettrap (or, without one, its target) says,bound fandclass Foocome out as in node, and a throwingnamegetter or trap propagates instead of being swallowed, which node also does. Renderings that were already right are unchanged: named functions, builtins, classes, anonymous functions (function, as before, which the existingprocess.umaskassertion intest/js/node/process/process.test.jspins), and non-callable Proxies still go through the object branch.namegetter or a Proxy trap). The object branch has always done this for.constructor, so callers are already written for it; the exception-check lane (BUN_JSC_validateExceptionChecks=1) is clean on the new test.test/js/node/errors/invalid-arg-type-received-function.test.ts: 9 pass with the fix, 8 fail on the released binary (USE_SYSTEM_BUN=1). It drives one table of values through a C++ validator (process.umask), the JS builtins'$ERR_INVALID_ARG_TYPE(url.format), Rust'sdetermine_specific_type(zlib.crc32) andcreateInvalidThisError(Bun.CryptoHasher#update), and checks that a throwing getter, trap, or revoked proxy surfaces the user's error. Expected strings were taken from node v26.3.0.test/js/node/test/parallelthat usecommon.invalidArgTypeHelper(which expects node'sfunction ${input.name}); no new failures.process.test.jsandutil-promisify.test.jspass apart from a pre-existing$USER-dependent test.Zig::functionNameuse in this file, the[Function: x]arm ofJSValueToStringSafebehindERR_INVALID_ARG_VALUE(bound functions, classes and async functions render as[Function: b],[Function: Foo],[Function: af]where node'sutil.inspectform is[Function: bound b],[class Foo],[AsyncFunction: af]), and two unrelated branches ofdetermineSpecificTypeitself (-0renders astype number (0); aconstructorwithout anameproperty renders asan instance of undefined). node: render native validateOneOf errors like Node and type-check vm microtaskMode #38401, node errors: cut the ERR_INVALID_ARG_VALUE received value at 128 chars like node #38402 and node errors: quote and escape ERR_INVALID_ARG_VALUE string values like util.inspect #38435 edit other parts of this file and do not overlap with this hunk.Background
determineSpecificTypeis bun's port of node's helper of the same name. It appends theReceived ...description of a value (type number (5),an instance of Array,function foo) and is reached from the C++ERR::INVALID_ARG_TYPEfamily and native validators, from the JS builtins via$ERR_INVALID_ARG_TYPE, from Rust viaBun__ErrorCode__determineSpecificType, and fromcreateInvalidThisError.Zig::functionName()is the helper stack-trace formatting uses to name a frame's callee. It deliberately never runs user code, so it skips Proxies and accessors and otherwise reads JSC's internal name fields, which is why it is the wrong source for a message whose node definition is a property read.Proxyis an object of JSC typeProxyObjectTypewrapping a callable target;typeofreports"function"and property reads go through itsgettrap, or straight to the target when there is none.nameown property lazily, on first access. That is why the old rendering of a bound function depended on whether.namehad been read before the error was built.