node errors: render -0 and constructor-less objects in determineSpecificType like node - #38642
node errors: render -0 and constructor-less objects in determineSpecificType like node#38642robobun wants to merge 1 commit into
Conversation
…ficType like node
The number arm now prints "type number (-0)" for negative zero instead of
dropping the sign. The object arm is ported as written in node: it only
says "an instance of X" when the constructor has a name property, throws
the `in` operator's TypeError when the constructor is a truthy primitive,
and otherwise renders the value with util.inspect(value, { depth: -1 })
instead of the native formatter at full depth.
|
Warning Review limit reached
Next review available in: 15 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: reproduced on bun 1.4.0 with |
There was a problem hiding this comment.
LGTM — focused Node compat fix for determineSpecificType with thorough test coverage.
What was reviewed:
- Exception-scope discipline in
appendSpecificTypeOfObject: every fallible call (get,getIfPropertyExists,toString,view,utilInspectFunction,profiledCall) is followed byRETURN_IF_EXCEPTION;toBooleancorrectly needs no check. - The
util.inspectinvocation matches the established pattern inJSURLSearchParams.cpp/WebStreamsInspectCustom.cpp(defaultGlobalObject→utilInspectFunction→MarkedArgumentBuffer→profiledCall). - The removed fallthrough to
JSValueToStringSafefor objects with a falsy constructor is now handled inside the helper via thedepth: -1inspect fallback — matches node and covered by the null-prototype /constructor: nulltest rows. -0detection viad == 0 && std::signbit(d)is placed after the NaN/±Infinity checks so+0still falls through to the numeric formatter.
Extended reasoning...
Overview
This PR fixes two divergences between Bun's determineSpecificType() (in src/jsc/bindings/ErrorCode.cpp, which renders the Received ... suffix of ERR_INVALID_ARG_TYPE and ERR_INVALID_THIS) and Node's reference implementation in lib/internal/errors.js:
- The number arm now special-cases
-0to render astype number (-0)instead oftype number (0). - The object arm is extracted into a static helper
appendSpecificTypeOfObject()that faithfully ports node's logic: check'name' in value.constructorviagetIfPropertyExists, throw the V8-wordedin-operator TypeError when the constructor is a truthy primitive, and fall back toutil.inspect(value, { depth: -1 })for constructor-less / nameless-constructor objects.
A comprehensive new test file exercises 4 entry points (C++ validator, JS builtin $ERR_INVALID_ARG_TYPE, Rust determine_specific_type, and generated-class createInvalidThisError) against 18 rendering cases, 5 primitive-constructor throws, and 4 exception-propagation scenarios, all cross-checked against node v26.3.0.
Security risks
None. This is error-message formatting only; it does not touch auth, filesystem, network, or any privileged path. The new call into util.inspect runs user-observable code (custom inspect symbols, Proxy traps, getters), but that is exactly node's behavior and any exception is propagated cleanly rather than swallowed.
Level of scrutiny
Moderate. The change is in C++ JSC bindings with ThrowScope discipline, which is the most-blocked review category in this repo. However:
- The helper declares its own
DECLARE_THROW_SCOPE(correct, since it callsthrowTypeError), while the caller keeps its existingDECLARE_TOP_EXCEPTION_SCOPEand checks withRETURN_IF_EXCEPTIONimmediately after — the contract towards existing callers is unchanged. - Every call that can enter JS or throw (
object->get,getIfPropertyExists,toWTFString,toString,view,utilInspectFunction,profiledCall) is followed byRETURN_IF_EXCEPTION.toBooleanandconstructEmptyObjectare correctly not checked (matching neighboring code and JSC semantics). - The
util.inspectcall pattern is copied verbatim from existing sites (JSURLSearchParams.cpp:246,WebStreamsInspectCustom.cpp:57,JSBroadcastChannel.cpp:216). - The PR description states the change was verified under
BUN_JSC_validateExceptionChecks=1.
Other factors
- The old code fell through to
JSValueToStringSafe(the native single-line formatter at unlimited depth) whenconstructorwas falsy; the new code always returns from within the helper via thedepth: -1inspect fallback. This is an intentional behavior change that brings Bun in line with node, and the test's[Object: null prototype]and[Map]rows cover it. - The
-0check is ordered correctly: it comes after the NaN and ±Infinity checks (which use!=and==on the double) and only fires whend == 0with the sign bit set, so+0and all other numbers still hit the generalbuilder.append(d)path — confirmed by thezeroandnegative fractiontest rows. - Test quality is high: exact-message assertions (not
toContain), covers the negative contract (unchanged renderings for plain objects, arrays, anonymous classes, inherited names), exercises Proxyhastraps and getter/inspect exceptions, and usesdescribe.eachover the four entry points so a divergence in any one path fails independently. - The PR notes a pre-existing debug-only assert in
ErrorCodeCache::createErrorwhen a primitive is thrown during rendering on thecreateInvalidThisErrorpath; the test deliberately throwsRangeErrorobjects rather than primitives so it does not trip that unrelated issue.
Problem
determineSpecificType()insrc/jsc/bindings/ErrorCode.cpprenders theReceived ...part of everyERR_INVALID_ARG_TYPEmessage (C++ validators, the JS builtins'$ERR_INVALID_ARG_TYPE, Rust'sdetermine_specific_type, and thebut received ...suffix of theERR_INVALID_THISthat native classes throw). Two arms of it diverge from node'slib/internal/errors.jsdetermineSpecificType():ErrorCode.cpp:389):process.chdir(-0)saysReceived type number (0); node saystype number (-0).StringBuilder::append(double)drops the sign likeNumber#toStringdoes, and node special-cases it.ErrorCode.cpp:492): bun only checks thatvalue.constructoris truthy and then prints.constructor.name, node checks'name' in value.constructorand otherwise falls back toutil.inspect(value, { depth: -1 }). Measured on bun 1.4.0 vs node v26.3.0 throughprocess.chdir(v):{ constructor: {} }: bunan instance of undefined, node[Object]{ constructor: null }: bun{}, node[Object](bun's fallback was the native formatter at unlimited depth)Object.assign(Object.create(null), { a: 1 }): bun[Object: null prototype] { a: 1 }, node[Object: null prototype]{ constructor: 1 }: bunan instance of undefined, node throwsTypeError: Cannot use 'in' operator to search for 'name' in 1(theinon a primitive propagates out of the message builder)Fix
-0renders astype number (-0).appendSpecificTypeOfObject(), a line by line port of node's arm:getIfPropertyExists(name)is the'name' in ctorcheck plus the read (it goes through the same[[HasProperty]]walk, so Proxyhastraps and inherited names behave as in node); a truthy primitive constructor throws a TypeError with the text node prints for it; everything else callsutil.inspect(value, { depth: -1 })the way the BroadcastChannel / URLSearchParams / web streams inspect code already calls it from C++. Bun'sutil.inspectis a port of node's, so the collapsed renderings ([Object],[Foo],[Map],[Object: null prototype], custom inspect functions called with depth -1) come out identical; the fallback runs only for values whose constructor is missing or nameless, so the commonan instance of Xpath is unchanged apart from usinggetIfPropertyExists.ThrowScopebecause it throws;determineSpecificType()keeps itsTopExceptionScope, so its contract towards callers (exception left pending, callers check) is unchanged. The same shape already happens today when aconstructorgetter throws, which is why every caller, includingcreateInvalidThisError, already copes with it.test/js/node/errors/invalid-arg-type-received.test.ts: 4 entry points (C++process.chdir, JSEventEmitter#on, RustSocketAddress.parse, generated-classCryptoHasher#updateinvalidthis) x 18 renderings, the 5 primitive-constructor throws, and propagation of exceptions thrown from the constructor getter, ahastrap, the name getter, and a custom inspect function. All 12 tests fail on bun 1.4.0 (only the intended rows differ, see below) and pass on this build, also withBUN_JSC_validateExceptionChecks=1.ERR_INVALID_ARG_TYPEentry points.Background
determineSpecificType(nodelib/internal/errors.js): turns the offending value into theReceived ...text ofERR_INVALID_ARG_TYPE: primitives astype number (5), objects asan instance of <constructor name>, and anything without a usable constructor name throughutil.inspectwithdepth: -1, which prints a value as just its bracketed constructor name instead of its contents.'name' in x:[[HasProperty]], i.e. it looks up the prototype chain and asks a Proxy'shastrap, and it throws a TypeError whenxis a primitive. JSC'sJSObject::getIfPropertyExistsdoes that lookup and returns the value, or an emptyJSValuewhen the property does not exist.ThrowScope; aTopExceptionScope(whatdetermineSpecificTypeuses) can only observe exceptions that callees threw. UnderBUN_JSC_validateExceptionChecks=1(CI's ASAN lanes) every call that may throw has to be followed by a check, hence theRETURN_IF_EXCEPTIONafter the new helper.utilInspectFunction()on the global object lazily loadsnode:utiland returns itsinspect, the same function users call;defaultGlobalObject()maps a non-Bun global (anode:vmcontext) to the Bun global that owns it.constructorgetter that doesthrow 7),ErrorCodeCache::createErrorasserts in debug builds on thecreateInvalidThisErrorpath. It reproduces on main without this change and is being fixed separately.Rows that differ on bun 1.4.0 (process.chdir entry point; the other three entry points differ identically)
Node v26.3.0 replay of the test's data (excerpt, 0 mismatches over 81 checks)