feat(node): Filled in most util/types is* checks#540
feat(node): Filled in most util/types is* checks#540MendyBerger wants to merge 1 commit intounjs:mainfrom
Conversation
📝 WalkthroughWalkthroughThis PR replaces many placeholder stubs in Node.js runtime type utilities with concrete, strongly-typed implementations using Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/node/internal/util/types.ts`:
- Around line 103-105: The isSharedArrayBuffer type guard directly uses the
global SharedArrayBuffer which can be undefined in some runtimes; update the
guard (and the other direct SharedArrayBuffer references in this file) to first
check typeof SharedArrayBuffer !== 'undefined' before using instanceof so the
function returns false instead of throwing when SharedArrayBuffer is not
available (i.e., change isSharedArrayBuffer and the other checks to
short-circuit with a typeof guard).
- Around line 138-152: The isTypedArray implementation currently checks
instanceof Float16Array which can throw ReferenceError in runtimes without that
constructor; update the isTypedArray function to avoid direct constructor checks
and instead return ArrayBuffer.isView(val) &&
Object.prototype.toString.call(val) !== "[object DataView]". Locate the
isTypedArray export and replace the instanceof chain (including Float16Array)
with this safe detection logic so typed arrays are recognized across
environments that lack certain constructors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 27ba8a0b-b434-4b29-af22-c5ff4cc729aa
📒 Files selected for processing (1)
src/runtime/node/internal/util/types.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/node/internal/util/types.ts`:
- Around line 44-52: The isAsyncFunction and isGeneratorFunction helpers are
vulnerable to constructor spoofing via checking val.constructor?.name; change
detection to use Object.prototype.toString.call(val) to compare against "[object
AsyncFunction]" and "[object GeneratorFunction]" (while still ensuring typeof
val === "function") so spoofed constructor objects can't produce false
positives; update the exported functions isAsyncFunction and isGeneratorFunction
to use this robust check while preserving their TypeScript typings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b77b447-dd71-4413-9ac7-fb96f1dd7514
📒 Files selected for processing (1)
src/runtime/node/internal/util/types.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/node/internal/util/types.ts`:
- Around line 16-35: Replace the unsafe instanceof checks in isBigIntObject,
isBooleanObject, isNumberObject, isStringObject, isSymbolObject (and analogous
boxed-primitive checks later) with an internal-slot detection that calls the
primitive's valueOf to verify it's a true boxed primitive; e.g., for
isStringObject use a guarded call like try { return typeof
Object.prototype.valueOf.call(val) === 'string'; } catch { return false; }
(analogous checks: 'number', 'boolean', 'bigint', 'symbol') so prototype-forged
objects (Object.create(String.prototype)) are rejected; update isBoxedPrimitive
to use those new helpers.
- Around line 67-81: The isMap and isSet functions currently use instanceof
which misclassifies cross-realm objects and prototype fakes; replace those
checks with realm-safe internal tag checks using
Object.prototype.toString.call(val) === '[object Map]' for isMap and ===
'[object Set]' for isSet (keep the same exported names isMap and isSet and type
signatures), so cross-realm Map/Set values are accepted and objects merely
inheriting Map.prototype are rejected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a06c599e-ae29-43d1-98da-30fac3148f8f
📒 Files selected for processing (1)
src/runtime/node/internal/util/types.ts
| export const isBigIntObject: typeof nodeUtilTypes.isBigIntObject = ( | ||
| val: unknown, | ||
| ): val is BigInt => val instanceof BigInt; | ||
|
|
||
| export const isBooleanObject: typeof nodeUtilTypes.isBooleanObject = ( | ||
| val, | ||
| ): val is boolean => typeof val === "boolean"; | ||
| val: unknown, | ||
| ): val is Boolean => val instanceof Boolean; | ||
|
|
||
| export const isNumberObject: typeof nodeUtilTypes.isNumberObject = ( | ||
| val, | ||
| ): val is number => typeof val === "number"; | ||
| val: unknown, | ||
| ): val is Number => val instanceof Number; | ||
|
|
||
| export const isStringObject: typeof nodeUtilTypes.isStringObject = ( | ||
| val, | ||
| ): val is string => typeof val === "string"; | ||
| val: unknown, | ||
| ): val is String => val instanceof String; | ||
|
|
||
| export const isSymbolObject: typeof nodeUtilTypes.isSymbolObject = ( | ||
| val, | ||
| ): val is symbol => typeof val === "symbol"; | ||
| val: unknown, | ||
| ): val is Symbol => val instanceof Symbol; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/runtime/node/internal/util/types.ts | head -40Repository: unjs/unenv
Length of output: 1654
🏁 Script executed:
cat -n src/runtime/node/internal/util/types.ts | sed -n '125,140p'Repository: unjs/unenv
Length of output: 703
🏁 Script executed:
node <<'NODE'
const { types } = require('node:util');
const fakeString = Object.create(String.prototype);
console.log('node util.types.isStringObject(fakeString):', types.isStringObject(fakeString));
console.log('fakeString instanceof String:', fakeString instanceof String);
const fakeNumber = Object.create(Number.prototype);
console.log('node util.types.isNumberObject(fakeNumber):', types.isNumberObject(fakeNumber));
console.log('fakeNumber instanceof Number:', fakeNumber instanceof Number);
const fakeBigInt = Object.create(BigInt.prototype);
console.log('node util.types.isBigIntObject(fakeBigInt):', types.isBigIntObject(fakeBigInt));
console.log('fakeBigInt instanceof BigInt:', fakeBigInt instanceof BigInt);
const fakeBoolean = Object.create(Boolean.prototype);
console.log('node util.types.isBooleanObject(fakeBoolean):', types.isBooleanObject(fakeBoolean));
console.log('fakeBoolean instanceof Boolean:', fakeBoolean instanceof Boolean);
const fakeSymbol = Object.create(Symbol.prototype);
console.log('node util.types.isSymbolObject(fakeSymbol):', types.isSymbolObject(fakeSymbol));
console.log('fakeSymbol instanceof Symbol:', fakeSymbol instanceof Symbol);
NODERepository: unjs/unenv
Length of output: 482
Replace instanceof checks with internal-slot detection for boxed primitives.
Lines 18, 22, 26, 30, 34 use instanceof which accepts prototype-forged objects (e.g., Object.create(String.prototype)), while Node's util.types correctly rejects them. This causes false positives in isBigIntObject, isBooleanObject, isNumberObject, isStringObject, isSymbolObject, and isBoxedPrimitive.
Detect internal slots by calling valueOf instead:
Suggested fix
+const hasBoxedSlot = (
+ val: unknown,
+ valueOf: (this: unknown) => unknown,
+): boolean => {
+ if (typeof val !== "object" || val === null) {
+ return false;
+ }
+ try {
+ valueOf.call(val);
+ return true;
+ } catch {
+ return false;
+ }
+};
export const isBigIntObject: typeof nodeUtilTypes.isBigIntObject = (
val: unknown,
-): val is BigInt => val instanceof BigInt;
+): val is BigInt => hasBoxedSlot(val, BigInt.prototype.valueOf);
export const isBooleanObject: typeof nodeUtilTypes.isBooleanObject = (
val: unknown,
-): val is Boolean => val instanceof Boolean;
+): val is Boolean => hasBoxedSlot(val, Boolean.prototype.valueOf);
export const isNumberObject: typeof nodeUtilTypes.isNumberObject = (
val: unknown,
-): val is Number => val instanceof Number;
+): val is Number => hasBoxedSlot(val, Number.prototype.valueOf);
export const isStringObject: typeof nodeUtilTypes.isStringObject = (
val: unknown,
-): val is String => val instanceof String;
+): val is String => hasBoxedSlot(val, String.prototype.valueOf);
export const isSymbolObject: typeof nodeUtilTypes.isSymbolObject = (
val: unknown,
-): val is Symbol => val instanceof Symbol;
+): val is Symbol => hasBoxedSlot(val, Symbol.prototype.valueOf);
export const isBoxedPrimitive: typeof nodeUtilTypes.isBoxedPrimitive = (
val: unknown,
): val is String | Number | Boolean | BigInt | Symbol => {
- return val instanceof String ||
- val instanceof Number ||
- val instanceof BigInt ||
- val instanceof Boolean ||
- val instanceof Symbol;
+ return isStringObject(val) ||
+ isNumberObject(val) ||
+ isBigIntObject(val) ||
+ isBooleanObject(val) ||
+ isSymbolObject(val);
};Also applies to lines 128–136.
🧰 Tools
🪛 ESLint
[error] 18-18: Prefer using the primitive bigint as a type name, rather than the upper-cased BigInt.
(@typescript-eslint/no-wrapper-object-types)
[error] 18-18: Avoid using instanceof for type checking as it can lead to unreliable results.
(unicorn/no-instanceof-builtins)
[error] 22-22: Prefer using the primitive boolean as a type name, rather than the upper-cased Boolean.
(@typescript-eslint/no-wrapper-object-types)
[error] 22-22: Avoid using instanceof for type checking as it can lead to unreliable results.
(unicorn/no-instanceof-builtins)
[error] 26-26: Prefer using the primitive number as a type name, rather than the upper-cased Number.
(@typescript-eslint/no-wrapper-object-types)
[error] 26-26: Avoid using instanceof for type checking as it can lead to unreliable results.
(unicorn/no-instanceof-builtins)
[error] 30-30: Prefer using the primitive string as a type name, rather than the upper-cased String.
(@typescript-eslint/no-wrapper-object-types)
[error] 30-30: Avoid using instanceof for type checking as it can lead to unreliable results.
(unicorn/no-instanceof-builtins)
[error] 34-34: Prefer using the primitive symbol as a type name, rather than the upper-cased Symbol.
(@typescript-eslint/no-wrapper-object-types)
[error] 34-34: Avoid using instanceof for type checking as it can lead to unreliable results.
(unicorn/no-instanceof-builtins)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/node/internal/util/types.ts` around lines 16 - 35, Replace the
unsafe instanceof checks in isBigIntObject, isBooleanObject, isNumberObject,
isStringObject, isSymbolObject (and analogous boxed-primitive checks later) with
an internal-slot detection that calls the primitive's valueOf to verify it's a
true boxed primitive; e.g., for isStringObject use a guarded call like try {
return typeof Object.prototype.valueOf.call(val) === 'string'; } catch { return
false; } (analogous checks: 'number', 'boolean', 'bigint', 'symbol') so
prototype-forged objects (Object.create(String.prototype)) are rejected; update
isBoxedPrimitive to use those new helpers.
| export const isMap: typeof nodeUtilTypes.isMap = <T>( | ||
| val: unknown, | ||
| ): val is T extends ReadonlyMap<any, any> | ||
| ? (unknown extends T ? never : ReadonlyMap<any, any>) | ||
| : Map<unknown, unknown> => { | ||
| return val instanceof Map; | ||
| }; | ||
|
|
||
| // @ts-ignore | ||
| export const isSet: typeof nodeUtilTypes.isSet = (val): val is Set => | ||
| val instanceof Set; | ||
| export const isSet: typeof nodeUtilTypes.isSet = <T>( | ||
| val: unknown, | ||
| ): val is T extends ReadonlySet<any> | ||
| ? unknown extends T ? never : ReadonlySet<any> | ||
| : Set<unknown> => { | ||
| return val instanceof Set; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/runtime/node/internal/util/types.ts | head -90Repository: unjs/unenv
Length of output: 3597
🏁 Script executed:
node <<'NODE'
const vm = require('node:vm');
const { types } = require('node:util');
const crossRealmMap = vm.runInNewContext('new Map([["k",1]])');
console.log('node util.types.isMap(crossRealmMap):', types.isMap(crossRealmMap));
console.log('crossRealmMap instanceof Map:', crossRealmMap instanceof Map);
const fakeMap = Object.create(Map.prototype);
console.log('node util.types.isMap(fakeMap):', types.isMap(fakeMap));
console.log('fakeMap instanceof Map:', fakeMap instanceof Map);
const crossRealmSet = vm.runInNewContext('new Set([1])');
console.log('node util.types.isSet(crossRealmSet):', types.isSet(crossRealmSet));
console.log('crossRealmSet instanceof Set:', crossRealmSet instanceof Set);
const fakeSet = Object.create(Set.prototype);
console.log('node util.types.isSet(fakeSet):', types.isSet(fakeSet));
console.log('fakeSet instanceof Set:', fakeSet instanceof Set);
NODERepository: unjs/unenv
Length of output: 345
🌐 Web query:
Node.js util.types.isMap isSet implementation C++ internal type checking
💡 Result:
In Node.js, util.types.isMap() / util.types.isSet() are not implemented as JS instanceof checks. They’re exposed from a C++ internal binding that delegates to V8’s internal type predicates.
-
The JS glue is
lib/internal/util/types.js, which exports (among other helpers)...internalBinding('types')—i.e., it re-exports the C++ “types” binding directly. [1] -
The C++ binding is registered in
src/node_types.ccviaNODE_BINDING_CONTEXT_AWARE_INTERNAL(types, node::InitializeTypes). [2] -
In that file, Node defines functions like
IsMap/IsSetthat simply do:args.GetReturnValue().Set(args[0]->IsMap());args.GetReturnValue().Set(args[0]->IsSet());
meaning the decision is made by V8’s
v8::Value::IsMap()/IsSet()checks (and Node also wires “Fast API” versions viaSetFastMethodNoSideEffect(...)). [2]
From the public API side, Node’s docs describe util.types.isMap(value) as returning true “if the value is a built-in Map instance” (same idea for isSet). [3]
Sources:
[1] lib/internal/util/types.js (Node.js) re-exporting internalBinding('types')
[2] src/node_types.cc (Node.js) implementing IsMap / IsSet via value->IsMap() / value->IsSet() and registering the binding
[3] Node.js util documentation for util.types.isMap()
Replace instanceof with realm-safe type checks to match Node.js behavior.
The current implementation at lines 72 and 80 using instanceof Map and instanceof Set fails to achieve Node parity:
- False negative: cross-realm Maps/Sets (created in different contexts via
vm.runInNewContext) are incorrectly rejected - False positive: fake objects created with
Object.create(Map.prototype)are incorrectly accepted
Node.js's util.types.isMap() and util.types.isSet() use V8's internal type predicates which correctly handle both cases. The current implementation needs realm-safe checking to match this behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/node/internal/util/types.ts` around lines 67 - 81, The isMap and
isSet functions currently use instanceof which misclassifies cross-realm objects
and prototype fakes; replace those checks with realm-safe internal tag checks
using Object.prototype.toString.call(val) === '[object Map]' for isMap and ===
'[object Set]' for isSet (keep the same exported names isMap and isSet and type
signatures), so cross-realm Map/Set values are accepted and objects merely
inheriting Map.prototype are rejected.
|
If there's any human looking at this, are we actually concerned about the increasingly far-fetched suggestions the llm keeps making here? I'm happy to do the changes it wants, but for each code change I accept, it finds an even deeper edge case. I don't think the rest of this codebase has this level of scrutiny applied to it. Thoughts? |
1 participant
Not closing an issue as it's mostly just implementing a couple of functions that are
notImplemented.I also just did a general cleanup on the file and made the checks looks more alike.
While I was there I noticed a bug with the
is[Primitive]Objectimplementations. They were checking withtypeof, buttypeof new Stringis actually"object"so I changed them to useinstanceof.Summary by CodeRabbit
Bug Fixes
Refactor
New Features