Repository navigation
Conversation
validators.rs hand-formatted `The "x" property must be of type T, got <typeof>`. Route it through the shared renderer so it produces Node's `The "x" argument must be of type T. Received <specific type>`, and make the shared renderer say "property" for dotted names like Node does. Also: validateArray/validateUnion use Node's templates, the string and boolean array validators report the offending element instead of the array, path.join() names its argument "path" and path.basename() names its second argument "suffix", both as in Node.
|
Warning Review limit reached
Next review available in: 20 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 (11)
Comment |
|
Status: reproduced and fixed. Reproduced on Bun 1.4.0 and main by diffing the Overlaps with section 1 of the draft #35423 (same message-shape fix, plus more); see the PR description for what this PR additionally covers. |
There was a problem hiding this comment.
I reviewed this and didn't find any bugs. Since it changes the shared throw_invalid_argument_type_value* renderers used by ~100 call sites across node/webcore/socket/zlib/crypto, a human look (and green CI) would still be worthwhile before landing.
What was reviewed:
InvalidArgTypeNamematchesBun::Message::addParameterin ErrorCode.cpp exactly (argumentsuffix → verbatim, dotted → property, else argument).- Grepped for stale assertions on the old
, got <type>wording and on"options.X" argument— the remaining hits go throughthrow_invalid_property_typeor hand-formatted sqlite messages, neither touched here. path.joinhot-loop simplification is fine: the removed cold-path helper existed to avoid buildingformat_args!("paths[{i}]")per iteration; the new name is a static&strso that concern is gone, andthrow_err_invalid_arg_typeis still#[cold].- Array-element fix (
iteminstead ofvalue) andget_type_namedeletion look correct.
Extended reasoning...
Overview
The PR routes the Rust validators.rs error paths (throw_err_invalid_arg_type, validate_array, validate_string_enum, and the string/boolean array validators) through the shared JSGlobalObject::throw_invalid_argument_type_value{,2,_one_of} renderers, and teaches those three renderers Node's argument-vs-property naming rule via a new InvalidArgTypeName Display type. It also fixes three argument-name mismatches in node:path (join → "path", basename suffix → "suffix"), reorders OptionValueType::VALUES_INFO to match Node's validateUnion order, makes the array validators report the offending element rather than the whole array, and deletes the now-dead get_type_name. Seven test files add or tighten exact-message assertions.
Security risks
None. All changes are on cold error-message paths; no parsing, allocation, or control-flow changes on the happy path.
Level of scrutiny
Medium. The logic is small (~50 net source lines), but throw_invalid_argument_type_value* has ~100 call sites across node_fs, SocketAddress, zlib, crypto, h2_frame_parser, socket_body, ffi_body, etc. Every caller passing a dotted name now says "property" instead of "argument", which is Node-correct and matches the existing C++ addParameter, but any missed exact-message test assertion in the vendored Node suite or Bun's own tests would surface as a CI failure. I spot-checked: the surviving "options.X" argument must be a ... assertions are hand-formatted in NodeSqlite.cpp (different template, unaffected), and the surviving must be of type ..., got assertions go through throw_invalid_property_type (explicitly left alone per the PR description). handle.fd in node_cluster_binding.rs will flip to "property", which is Node-correct.
Other factors
The path.join change removes an explicit #[cold] #[inline(never)] cold helper. That helper's stated purpose was avoiding a 48-byte fmt::Arguments build per iteration for format_args!("paths[{i}]"); with a static "path" the concern disappears and throw_err_invalid_arg_type remains #[cold], so this is a justified simplification rather than a perf regression. CI (#96508) was still building when I reviewed; given the breadth of callers, waiting for green CI plus a human glance seems prudent. Test coverage is thorough (path × posix/win32 × 14 fns × 7 values, plus parseArgs/fs/scrypt/SocketAddress) and the PR description documents byte-equality with Node v26.3.0.
Problem
path.join(1)throwsThe "paths[0]" property must be of type string, got number; Node v26.3.0 throwsThe "path" argument must be of type string. Received type number (1). Thecode(ERR_INVALID_ARG_TYPE) is right, only the message differs.throw_err_invalid_arg_typeinsrc/runtime/node/util/validators.rsformats the message by hand ("property",, got <typeof>) instead of using the shared renderer. It is behindvalidate_string,validate_int32,validate_uint32,validate_boolean,validate_object, the string/boolean array validators, and themodeerrors offs.chmod/fs.fchmod, sonode:path,util.parseArgs,net.BlockList,crypto.scryptand others all produce the Bun wording.validate_array(must be an instance of Array, got ...) andvalidate_string_enum(options.x.type must be one of: boolean|string) hand-format their messages the same way.path.join()names its argumentpaths[i](Node'sjoin()usespath; onlyresolve()usespaths[i]), andpath.basename()names its second argumentext(Node:suffix).JSGlobalObject::throw_invalid_argument_type_value) always saysargument. Node sayspropertywhen the name is dotted, sofs.rmSync(p, { maxRetries: "a" })andnew SocketAddress({ address: 1 })saidThe "options.maxRetries" argument ...where Node saysproperty.Fix
validators.rs:throw_err_invalid_arg_typedelegates tothrow_invalid_argument_type_value;validate_arrayandvalidate_string_enumgo throughthrow_invalid_argument_type_value2with Node's phrases (an instance of Array,('string|boolean')); the array validators report the element;get_type_name(the only remaining user of the old wording) is deleted.OptionValueType::VALUES_INFOis reordered tostring|boolean, the order Node passes tovalidateUnion.JSGlobalObject.rs: the threethrow_invalid_argument_type_value*renderers format the name through a smallDisplaytype that applies Node's rule ("x" argument,"a.b" property, a name ending inargumentverbatim). This is the same ruleBun::Message::addParameteralready applies for C++ callers, so Rust and C++ errors now agree.path.rs:join()validates each argument aspath;basename()validates the suffix assuffix.process.versions.nodereports). A 196-call matrix overpath.{posix,win32}x 14 functions x 7 bad values, plus theparseArgs,fs,scrypt,BlockListandSocketAddresscases below, diffs clean against a realnodebinary; 192 of the 196 path lines differed before.JSGlobalObject::throw_invalid_property_type(theJSValueoption getters behindfs.mkdir'srecursiveand Bun's own option bags) still uses the old wording; that is a different helper with mostly Bun-API callers and needs dotted-name plumbing (options.recursive) to match Node.parseArgs'sERR_INVALID_ARG_VALUEmessages andfs.rm's hand-writtenrecursive/forcemessages are different templates and are not touched. Section 1 of the draft node: restore 9 concealed-gap tests to verbatim upstream (arg-type messages, win32 paths, parseEnv) #35423 (and the renderer change in node: Node-accurate ERR_INVALID_ARG_TYPE / ERR_INVALID_THIS / ERR_MISSING_ARGS; resync test/common and 117 vendored tests with v26.3.0 #35429) covers the message shape part of this; this PR is the focused version and also fixes the array element,join,basenameandvalidateUnionmismatches, which those drafts do not.test/js/node/path/path.test.js: exact messages for everypathfunction on both namespaces (path,paths[i],from/to,suffix,pathObject).test/js/node/path/parse-format.test.js,resolve.test.js: existing assertions on the old wording updated (the former carried a TODO describing this divergence).test/js/node/util/parse_args/parse-args.test.mjs:validate_array,validate_boolean,validate_object,validate_string,validate_string_enum, the array element validators, and argument vs property naming.test/js/node/fs/fs.test.ts:chmodSync/fchmodSyncwithout a mode (directthrow_err_invalid_arg_typecallers);rmSyncretry options (dotted name through the shared renderer).test/js/node/crypto/scrypt.test.ts:validate_int32(keylen) andvalidate_uint32(N,cost).test/js/node/net/socketaddress.spec.ts: dotted name from a direct caller of the shared renderer.grepfor the oldmust be of type ..., gotwording finds no remaining assertions that this change affects (the leftovers are thethrow_invalid_property_typecallers above).Background
ERR_INVALID_ARG_TYPE(name, expected, actual)in Node'slib/internal/errors.jsrendersThe <subject> must be <expected>. Received <determineSpecificType(actual)>. The subject is"name" argument, or"name" propertywhen the name contains a.(it is then an option path likeoptions.foo).expectedrenders asof type stringfor a primitive type name,an instance of Arrayfor a class name, and verbatim for anything else (validateUnionpasses('string|boolean')).determineSpecificTypeis Node's description of the received value:type number (1),type string ('x'),an instance of Object,null,undefined. Bun implements it once in C++ (ErrorCode.cpp) and exposes it to Rust asJSGlobalObject::determine_specific_type, which the shared renderers already used.src/runtime/node/util/validators.rsis the Rust port of Node'slib/internal/validators.js, used by the natively implemented parts ofnode:path,util.parseArgs,net.BlockList,crypto.scrypt,dgramandzlib.Before / after for a few of the calls (right column is also Node v26.3.0's output)
path.join(1)The "paths[0]" property must be of type string, got numberThe "path" argument must be of type string. Received type number (1)path.join("a", 2n)The "paths[1]" property must be of type string, got bigintThe "path" argument must be of type string. Received type bigint (2n)path.resolve("a", null)The "paths[1]" property must be of type string, got objectThe "paths[1]" argument must be of type string. Received nullpath.basename("a", 1)The "ext" property must be of type string, got numberThe "suffix" argument must be of type string. Received type number (1)path.format([])The "pathObject" property must be of type object, got arrayThe "pathObject" argument must be of type object. Received an instance of ArrayparseArgs({ args: "x" })The "args" property must be an instance of Array, got stringThe "args" argument must be an instance of Array. Received type string ('x')parseArgs(... default: ["a", 1])The "options.alpha.default[1]" property must be of type string, got arrayThe "options.alpha.default[1]" property must be of type string. Received type number (1)parseArgs(... type: "nope")options.alpha.type must be one of: boolean|stringThe "options.alpha.type" property must be ('string|boolean'). Received type string ('nope')fs.chmodSync(p)The "mode" property must be of type number, got undefinedThe "mode" argument must be of type number. Received undefinedfs.rmSync(p, { maxRetries: "a" })The "options.maxRetries" argument must be of type number. Received type string ('a')The "options.maxRetries" property must be of type number. Received type string ('a')new BlockList().addSubnet("1.1.1.1", "x")The "prefix" property must be of type number, got stringThe "prefix" argument must be of type number. Received type string ('x')scryptSync("p", "s", "x")The "keylen" property must be of type number, got stringThe "keylen" argument must be of type number. Received type string ('x')