Skip to content

Commit

Permalink
errors: improve ERR_INVALID_ARG_TYPE
Browse files Browse the repository at this point in the history
ERR_INVALID_ARG_TYPE is the most common error used throughout the
code base. This improves the error message by providing more details
to the user and by indicating more precisely which values are allowed
ones and which ones are not.

It adds the actual input to the error message in case it's a primitive.
If it's a class instance, it'll print the class name instead of
"object" and "falsy" or similar entries are not named "type" anymore.

PR-URL: #29675
Reviewed-By: Rich Trott <[email protected]>
  • Loading branch information
BridgeAR committed Jan 3, 2020
1 parent 4df3652 commit 33c5dbe
Show file tree
Hide file tree
Showing 127 changed files with 664 additions and 533 deletions.
156 changes: 123 additions & 33 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,21 @@ const {
const messages = new Map();
const codes = {};

const classRegExp = /^([A-Z][a-z0-9]*)+$/;
// Sorted by a rough estimate on most frequently used entries.
const kTypes = [
'string',
'function',
'number',
'object',
// Accept 'Function' and 'Object' as alternative to the lower cased version.
'Function',
'Object',
'boolean',
'bigint',
'symbol'
];

const { kMaxLength } = internalBinding('buffer');

const MainContextError = Error;
Expand Down Expand Up @@ -610,26 +625,6 @@ function isStackOverflowError(err) {
err.message === maxStack_ErrorMessage;
}

function oneOf(expected, thing) {
assert(typeof thing === 'string', '`thing` has to be of type string');
if (ArrayIsArray(expected)) {
const len = expected.length;
assert(len > 0,
'At least one expected value needs to be specified');
expected = expected.map((i) => String(i));
if (len > 2) {
return `one of ${thing} ${expected.slice(0, len - 1).join(', ')}, or ` +
expected[len - 1];
} else if (len === 2) {
return `one of ${thing} ${expected[0]} or ${expected[1]}`;
} else {
return `of ${thing} ${expected[0]}`;
}
} else {
return `of ${thing} ${String(expected)}`;
}
}

// Only use this for integers! Decimal numbers do not work with this function.
function addNumericalSeparator(val) {
let res = '';
Expand Down Expand Up @@ -926,27 +921,114 @@ E('ERR_INVALID_ADDRESS_FAMILY', function(addressType, host, port) {
E('ERR_INVALID_ARG_TYPE',
(name, expected, actual) => {
assert(typeof name === 'string', "'name' must be a string");
if (!ArrayIsArray(expected)) {
expected = [expected];
}

let msg = 'The ';
if (name.endsWith(' argument')) {
// For cases like 'first argument'
msg += `${name} `;
} else {
const type = name.includes('.') ? 'property' : 'argument';
msg += `"${name}" ${type} `;
}

// determiner: 'must be' or 'must not be'
let determiner;
if (typeof expected === 'string' && expected.startsWith('not ')) {
determiner = 'must not be';
msg += 'must not be ';
expected = expected.replace(/^not /, '');
} else {
determiner = 'must be';
msg += 'must be ';
}

let msg;
if (name.endsWith(' argument')) {
// For cases like 'first argument'
msg = `The ${name} ${determiner} ${oneOf(expected, 'type')}`;
} else {
const type = name.includes('.') ? 'property' : 'argument';
msg = `The "${name}" ${type} ${determiner} ${oneOf(expected, 'type')}`;
const types = [];
const instances = [];
const other = [];

for (const value of expected) {
assert(typeof value === 'string',
'All expected entries have to be of type string');
if (kTypes.includes(value)) {
types.push(value.toLowerCase());
} else if (classRegExp.test(value)) {
instances.push(value);
} else {
assert(value !== 'object',
'The value "object" should be written as "Object"');
other.push(value);
}
}

// TODO(BridgeAR): Improve the output by showing `null` and similar.
msg += `. Received type ${typeof actual}`;
// Special handle `object` in case other instances are allowed to outline
// the differences between each other.
if (instances.length > 0) {
const pos = types.indexOf('object');
if (pos !== -1) {
types.splice(pos, 1);
instances.push('Object');
}
}

if (types.length > 0) {
if (types.length > 2) {
const last = types.pop();
msg += `one of type ${types.join(', ')}, or ${last}`;
} else if (types.length === 2) {
msg += `one of type ${types[0]} or ${types[1]}`;
} else {
msg += `of type ${types[0]}`;
}
if (instances.length > 0 || other.length > 0)
msg += ' or ';
}

if (instances.length > 0) {
if (instances.length > 2) {
const last = instances.pop();
msg += `an instance of ${instances.join(', ')}, or ${last}`;
} else {
msg += `an instance of ${instances[0]}`;
if (instances.length === 2) {
msg += ` or ${instances[1]}`;
}
}
if (other.length > 0)
msg += ' or ';
}

if (other.length > 0) {
if (other.length > 2) {
const last = other.pop();
msg += `one of ${other.join(', ')}, or ${last}`;
} else if (other.length === 2) {
msg += `one of ${other[0]} or ${other[1]}`;
} else {
if (other[0].toLowerCase() !== other[0])
msg += 'an ';
msg += `${other[0]}`;
}
}

if (actual == null) {
msg += `. Received ${actual}`;
} else if (typeof actual === 'function' && actual.name) {
msg += `. Received function ${actual.name}`;
} else if (typeof actual === 'object') {
if (actual.constructor && actual.constructor.name) {
msg += `. Received an instance of ${actual.constructor.name}`;
} else {
const inspected = lazyInternalUtilInspect()
.inspect(actual, { depth: -1 });
msg += `. Received ${inspected}`;
}
} else {
let inspected = lazyInternalUtilInspect()
.inspect(actual, { colors: false });
if (inspected.length > 25)
inspected = `${inspected.slice(0, 25)}...`;
msg += `. Received type ${typeof actual} (${inspected})`;
}
return msg;
}, TypeError);
E('ERR_INVALID_ARG_VALUE', (name, value, reason = 'is invalid') => {
Expand Down Expand Up @@ -1034,7 +1116,15 @@ E('ERR_INVALID_URL', function(input) {
return `Invalid URL: ${input}`;
}, TypeError);
E('ERR_INVALID_URL_SCHEME',
(expected) => `The URL must be ${oneOf(expected, 'scheme')}`, TypeError);
(expected) => {
if (typeof expected === 'string')
expected = [expected];
assert(expected.length <= 2);
const res = expected.length === 2 ?
`one of scheme ${expected[0]} or ${expected[1]}` :
`of scheme ${expected[0]}`;
return `The URL must be ${res}`;
}, TypeError);
E('ERR_IPC_CHANNEL_CLOSED', 'Channel closed', Error);
E('ERR_IPC_DISCONNECTED', 'IPC channel is already disconnected', Error);
E('ERR_IPC_ONE_PIPE', 'Child process can have only one IPC pipe', Error);
Expand Down
21 changes: 21 additions & 0 deletions test/common/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -718,6 +718,26 @@ function runWithInvalidFD(func) {
printSkipMessage('Could not generate an invalid fd');
}

// A helper function to simplify checking for ERR_INVALID_ARG_TYPE output.
function invalidArgTypeHelper(input) {
if (input == null) {
return ` Received ${input}`;
}
if (typeof input === 'function' && input.name) {
return ` Received function ${input.name}`;
}
if (typeof input === 'object') {
if (input.constructor && input.constructor.name) {
return ` Received an instance of ${input.constructor.name}`;
}
return ` Received ${util.inspect(input, { depth: -1 })}`;
}
let inspected = util.inspect(input, { colors: false });
if (inspected.length > 25)
inspected = `${inspected.slice(0, 25)}...`;
return ` Received type ${typeof input} (${inspected})`;
}

module.exports = {
allowGlobals,
buildType,
Expand All @@ -735,6 +755,7 @@ module.exports = {
hasIntl,
hasCrypto,
hasMultiLocalhost,
invalidArgTypeHelper,
isAIX,
isAlive,
isFreeBSD,
Expand Down
13 changes: 8 additions & 5 deletions test/es-module/test-esm-loader-modulemap.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ common.expectsError(
{
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: 'The "url" argument must be of type string. Received type number'
message: 'The "url" argument must be of type string. Received type number' +
' (1)'
}
);

Expand All @@ -34,7 +35,8 @@ common.expectsError(
{
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: 'The "url" argument must be of type string. Received type number'
message: 'The "url" argument must be of type string. Received type number' +
' (1)'
}
);

Expand All @@ -43,8 +45,8 @@ common.expectsError(
{
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: 'The "job" argument must be of type ModuleJob. ' +
'Received type string'
message: 'The "job" argument must be an instance of ModuleJob. ' +
"Received type string ('notamodulejob')"
}
);

Expand All @@ -53,6 +55,7 @@ common.expectsError(
{
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: 'The "url" argument must be of type string. Received type number'
message: 'The "url" argument must be of type string. Received type number' +
' (1)'
}
);
2 changes: 1 addition & 1 deletion test/internet/test-dns-promises-resolve.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ const dnsPromises = require('dns').promises;
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: 'The "rrtype" argument must be of type string. ' +
`Received type ${typeof rrtype}`
`Received type ${typeof rrtype} (${rrtype})`
}
);
}
Expand Down
8 changes: 4 additions & 4 deletions test/parallel/test-assert-async.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,8 @@ promises.push(assert.rejects(
assert.rejects('fail', {}),
{
code: 'ERR_INVALID_ARG_TYPE',
message: 'The "promiseFn" argument must be one of type ' +
'Function or Promise. Received type string'
message: 'The "promiseFn" argument must be of type function or an ' +
"instance of Promise. Received type string ('fail')"
}
));

Expand Down Expand Up @@ -225,8 +225,8 @@ promises.push(assert.rejects(
assert.doesNotReject(123),
{
code: 'ERR_INVALID_ARG_TYPE',
message: 'The "promiseFn" argument must be one of type ' +
'Function or Promise. Received type number'
message: 'The "promiseFn" argument must be of type ' +
'function or an instance of Promise. Received type number (123)'
}
));
/* eslint-enable no-restricted-syntax */
Expand Down
22 changes: 12 additions & 10 deletions test/parallel/test-assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -414,8 +414,8 @@ assert.throws(
{
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: 'The "fn" argument must be of type Function. Received ' +
`type ${typeof fn}`
message: 'The "fn" argument must be of type function.' +
common.invalidArgTypeHelper(fn)
}
);
};
Expand Down Expand Up @@ -484,8 +484,8 @@ assert.throws(() => {
{
code: 'ERR_INVALID_ARG_TYPE',
name: 'TypeError',
message: 'The "options" argument must be of type Object. ' +
`Received type ${typeof input}`
message: 'The "options" argument must be of type object.' +
common.invalidArgTypeHelper(input)
});
});
}
Expand Down Expand Up @@ -931,8 +931,9 @@ common.expectsError(
{
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: 'The "error" argument must be one of type Object, Error, ' +
'Function, or RegExp. Received type string'
message: 'The "error" argument must be of type function or ' +
'an instance of Error, RegExp, or Object. Received type string ' +
"('Error message')"
}
);

Expand All @@ -945,8 +946,9 @@ common.expectsError(
() => assert.throws(() => {}, input),
{
code: 'ERR_INVALID_ARG_TYPE',
message: 'The "error" argument must be one of type Object, Error, ' +
`Function, or RegExp. Received type ${typeof input}`
message: 'The "error" argument must be of type function or ' +
'an instance of Error, RegExp, or Object.' +
common.invalidArgTypeHelper(input)
}
);
});
Expand Down Expand Up @@ -1024,8 +1026,8 @@ common.expectsError(
{
type: TypeError,
code: 'ERR_INVALID_ARG_TYPE',
message: 'The "expected" argument must be one of type Function or ' +
'RegExp. Received type object'
message: 'The "expected" argument must be of type function or an ' +
'instance of RegExp. Received an instance of Object'
}
);

Expand Down
12 changes: 6 additions & 6 deletions test/parallel/test-buffer-alloc.js
Original file line number Diff line number Diff line change
Expand Up @@ -967,19 +967,19 @@ common.expectsError(
{
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: 'The "target" argument must be one of type Buffer or Uint8Array.' +
' Received type undefined'
message: 'The "target" argument must be an instance of Buffer or ' +
'Uint8Array. Received undefined'
});

assert.throws(() => Buffer.from(), {
name: 'TypeError',
message: 'The first argument must be one of type string, Buffer, ' +
'ArrayBuffer, Array, or Array-like Object. Received type undefined'
message: 'The first argument must be of type string or an instance of ' +
'Buffer, ArrayBuffer, or Array or an Array-like Object. Received undefined'
});
assert.throws(() => Buffer.from(null), {
name: 'TypeError',
message: 'The first argument must be one of type string, Buffer, ' +
'ArrayBuffer, Array, or Array-like Object. Received type object'
message: 'The first argument must be of type string or an instance of ' +
'Buffer, ArrayBuffer, or Array or an Array-like Object. Received null'
});

// Test prototype getters don't throw
Expand Down
5 changes: 3 additions & 2 deletions test/parallel/test-buffer-arraybuffer.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,9 @@ assert.throws(function() {
}, {
code: 'ERR_INVALID_ARG_TYPE',
name: 'TypeError',
message: 'The first argument must be one of type string, Buffer,' +
' ArrayBuffer, Array, or Array-like Object. Received type object'
message: 'The first argument must be of type string or an instance of ' +
'Buffer, ArrayBuffer, or Array or an Array-like Object. Received ' +
'an instance of AB'
});

// Test the byteOffset and length arguments
Expand Down
Loading

0 comments on commit 33c5dbe

Please sign in to comment.