Skip to content

errors: improve ERR_INVALID_ARG_TYPE #29675

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
BridgeAR wants to merge 4 commits into from
Closed

errors: improve ERR_INVALID_ARG_TYPE #29675

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c6e586b
buffer: improve .from() error details
BridgeAR Sep 23, 2019
14309b2
errors: improve ERR_INVALID_ARG_TYPE
BridgeAR Sep 23, 2019
b033324
fixup: use `object` instead of `Object` is most cases
BridgeAR Sep 26, 2019
0e7b9ff
fixup: errors: improve ERR_INVALID_ARG_TYPE
BridgeAR Dec 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
errors: improve ERR_INVALID_ARG_TYPE 
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.
  • Loading branch information
@BridgeAR
BridgeAR committed Dec 19, 2019
commit 14309b2b01ca47a3496a54cb1dd8d997d9e94154
2 changes: 1 addition & 1 deletion lib/_http_server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@ function Server(options, requestListener) {
} else if (options == null || typeof options === 'object') {
options = { ...options };
} else {
throw new ERR_INVALID_ARG_TYPE('options', 'object', options);
throw new ERR_INVALID_ARG_TYPE('options', 'Object', options);
Copy link
Copy Markdown
Member

@Trott Trott Sep 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the justification for changing object to Object? Here are the things that come to mind as arguments against it:

  • typeof {} === 'object' and not 'Object'
  • Sure, you can do new Object() but you can also do new String() or new Number() and we don't capitalize those.
  • "Everything is lowercased" is an easy rule to remember. "Some things get an initial capital letter and other things don't" is more prone to human error.
  • No added value to the end user of saying Object instead of object.
  • Not doing that change will greatly reduce the diff size, making this easier to review as well as backport. It also reduces the risk of conflicts with other PRs.

Copy link
Copy Markdown
Member Author

@BridgeAR BridgeAR Sep 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The justification was that there are already multiple entries written as Object and the documentation uses Object instead of object. I personally actually favor object a tiny bit above Object. I just roughly remembered that there where comments about this in a PR quite a while ago (I do not remember which one though), so I thought I'll go for that.

This actually also applies to the type function. It is mostly written as Function.

@vsemozhetbyt I think you where involved in this discussion. Do you by any chance remember any details?

Copy link
Copy Markdown
Contributor

@vsemozhetbyt vsemozhetbyt Sep 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I cannot remember. I can only say that we use lower case for primitives and upper case for other types in doc parts that are based on tools/doc/type-parser.js, otherwise I cannot decide what is better for code and error messages.

Copy link
Copy Markdown
Member Author

@BridgeAR BridgeAR Sep 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Trott I am fine to convert all Function and Object entries to lower cased versions, if you think that's a good way to handle it. I personally favor that.

Copy link
Copy Markdown
Member

@Trott Trott Sep 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Trott I am fine to convert all Function and Object entries to lower cased versions, if you think that's a good way to handle it. I personally favor that.

I'm fine either way.

Copy link
Copy Markdown
Member

@addaleax addaleax Sep 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when talking about the type I expect it to correspond to typeof input === 'object'. When Object is used I would expect the direct prototype to be `Object

I might be misunderstanding you, but those seem like arguments for keeping lowercase object here, because it is about typeof and not the prototype constructor name?

Copy link
Copy Markdown
Member Author

@BridgeAR BridgeAR Sep 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's correct that we use typeof input === 'object' in (probably) all of these cases. But when we do, we normally mean the object should have Object as prototype (or a null prototype as in Object.create(null)).

The example above is more intuitive for me if it's written as:
The "error" argument must be of type function or an instance of Object, Error, or RegExp. That way I would immediately expect the input to be allowed to look like:

let input = {} // Valid
input = /a/ // Valid
input = new TypeError() // Valid
input = new Map() // Invalid

Copy link
Copy Markdown
Member Author

@BridgeAR BridgeAR Sep 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax what do you think would be best here? The PR should overall reduce ambiguity and seems clearer than before.

Copy link
Copy Markdown
Member

@addaleax addaleax Sep 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But when we do, we normally mean the object should have Object as prototype (or a null prototype as in Object.create(null)).

I don’t think that’s true, and the way that JS objects work, the Map instance in your example is valid because we can attach regular properties to it, too.

So I’d personally stick with the current formatting of lowercase object unless we actually perform some kind of instanceof-style check.

Copy link
Copy Markdown
Member Author

@BridgeAR BridgeAR Sep 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct that it would be possible to use any object if we do the typecheck only. What we normally do is a check for a specific class, than another one and in the end we accept any object as long as it seems to fit.

However, we do mostly not accept null and that is also of type object.

I decided to go for a middle ground and hope that is good for everyone: if no class is detected, we'll always use type in combination with object. If we detect a class and object as accepted values, we'll declare it as instance of Object. This condition is pretty rare and I could only find few errors where this would apply too. That way we'll keep the distinction between different object types and use type of object in almost all other cases.

Is that fine for you?

}

this[kIncomingMessage] = options.IncomingMessage || IncomingMessage;
Expand Down
4 changes: 2 additions & 2 deletions lib/child_process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,7 +412,7 @@ function normalizeSpawnArguments(file, args, options) {
} else if (args == null) {
args = [];
} else if (typeof args !== 'object') {
throw new ERR_INVALID_ARG_TYPE('args', 'object', args);
throw new ERR_INVALID_ARG_TYPE('args', 'Object', args);
} else {
options = args;
args = [];
Expand All @@ -421,7 +421,7 @@ function normalizeSpawnArguments(file, args, options) {
if (options === undefined)
options = {};
else if (options === null || typeof options !== 'object')
throw new ERR_INVALID_ARG_TYPE('options', 'object', options);
throw new ERR_INVALID_ARG_TYPE('options', 'Object', options);

// Validate the cwd, if present.
if (options.cwd != null &&
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/child_process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -707,7 +707,7 @@ function setupChannel(target, channel, serializationMode) {
typeof message !== 'number' &&
typeof message !== 'boolean') {
throw new ERR_INVALID_ARG_TYPE(
'message', ['string', 'object', 'number', 'boolean'], message);
'message', ['string', 'Object', 'number', 'boolean'], message);
}

// Support legacy function signature
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/console/constructor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ function Console(options /* or: stdout, stderr, ignoreErrors = true */) {
}
optionsMap.set(this, inspectOptions);
} else if (inspectOptions !== undefined) {
throw new ERR_INVALID_ARG_TYPE('inspectOptions', 'object', inspectOptions);
throw new ERR_INVALID_ARG_TYPE('inspectOptions', 'Object', inspectOptions);
}

// Bind the prototype functions to this Console instance
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/crypto/keygen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,11 +134,11 @@ function check(type, options, callback) {
let cipher, passphrase, publicType, publicFormat, privateType, privateFormat;

if (options !== undefined && typeof options !== 'object')
throw new ERR_INVALID_ARG_TYPE('options', 'object', options);
throw new ERR_INVALID_ARG_TYPE('options', 'Object', options);

function needOptions() {
if (options == null)
throw new ERR_INVALID_ARG_TYPE('options', 'object', options);
throw new ERR_INVALID_ARG_TYPE('options', 'Object', options);
return options;
}

Expand Down
4 changes: 2 additions & 2 deletions lib/internal/crypto/keys.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class KeyObject {
if (type !== 'secret' && type !== 'public' && type !== 'private')
throw new ERR_INVALID_ARG_VALUE('type', type);
if (typeof handle !== 'object')
throw new ERR_INVALID_ARG_TYPE('handle', 'object', handle);
throw new ERR_INVALID_ARG_TYPE('handle', 'Object', handle);

this[kKeyType] = type;

Expand Down Expand Up @@ -178,7 +178,7 @@ function isStringOrBuffer(val) {

function parseKeyEncoding(enc, keyType, isPublic, objName) {
if (enc === null || typeof enc !== 'object')
throw new ERR_INVALID_ARG_TYPE('options', 'object', enc);
throw new ERR_INVALID_ARG_TYPE('options', 'Object', enc);

const isInput = keyType === undefined;

Expand Down
138 changes: 105 additions & 33 deletions lib/internal/errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ const {
const messages = new Map();
const codes = {};

const classRegExp = /^([A-Z][a-z0-9]*)+$/;
const kTypes = [
'string', 'boolean', 'number', 'symbol', 'bigint', 'function'
];

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

const MainContextError = Error;
Expand Down Expand Up @@ -610,26 +615,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 +911,106 @@ 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);
} else if (value === 'Function') { // 'Function' should be handled as type
types.push('function');
} else if (classRegExp.test(value)) {
instances.push(value);
} else {
assert(value !== 'object',
'The value "object" should be written as "Object"');
other.push(value);
}
}

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 ';
}

// TODO(BridgeAR): Improve the output by showing `null` and similar.
msg += `. Received type ${typeof actual}`;
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 +1098,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
2 changes: 1 addition & 1 deletion lib/internal/fs/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -580,7 +580,7 @@ const validateRmdirOptions = hideStackFrames((options) => {
if (options === undefined)
return defaultRmdirOptions;
if (options === null || typeof options !== 'object')
throw new ERR_INVALID_ARG_TYPE('options', 'object', options);
throw new ERR_INVALID_ARG_TYPE('options', 'Object', options);

options = { ...defaultRmdirOptions, ...options };

Expand Down
2 changes: 1 addition & 1 deletion lib/internal/http2/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1505,7 +1505,7 @@ class ServerHttp2Session extends Http2Session {
// be invalid.
if (typeof origin !== 'string') {
throw new ERR_INVALID_ARG_TYPE('originOrStream',
['string', 'number', 'URL', 'object'],
['string', 'number', 'URL', 'Object'],
originOrStream);
} else if (origin === 'null' || origin.length === 0) {
throw new ERR_HTTP2_ALTSVC_INVALID_ORIGIN();
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/process/per_thread.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ function wrapProcessMethods(binding) {
if (prevValue) {
if (!previousValueIsValid(prevValue.user)) {
if (typeof prevValue !== 'object')
throw new ERR_INVALID_ARG_TYPE('prevValue', 'object', prevValue);
throw new ERR_INVALID_ARG_TYPE('prevValue', 'Object', prevValue);

if (typeof prevValue.user !== 'number') {
throw new ERR_INVALID_ARG_TYPE('prevValue.user',
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/streams/end-of-stream.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ function eos(stream, opts, callback) {
} else if (opts == null) {
opts = {};
} else if (typeof opts !== 'object') {
throw new ERR_INVALID_ARG_TYPE('opts', 'object', opts);
throw new ERR_INVALID_ARG_TYPE('opts', 'Object', opts);
}
if (typeof callback !== 'function') {
throw new ERR_INVALID_ARG_TYPE('callback', 'function', callback);
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/worker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ class Worker extends EventEmitter {
} else if (options.env !== SHARE_ENV) {
throw new ERR_INVALID_ARG_TYPE(
'options.env',
['object', 'undefined', 'null', 'worker_threads.SHARE_ENV'],
['Object', 'undefined', 'null', 'worker_threads.SHARE_ENV'],
options.env);
}

Expand Down
2 changes: 1 addition & 1 deletion lib/trace_events.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ class Tracing {

function createTracing(options) {
if (typeof options !== 'object' || options === null)
throw new ERR_INVALID_ARG_TYPE('options', 'object', options);
throw new ERR_INVALID_ARG_TYPE('options', 'Object', options);

if (!ArrayIsArray(options.categories)) {
throw new ERR_INVALID_ARG_TYPE('options.categories', 'string[]',
Expand Down
2 changes: 1 addition & 1 deletion lib/vm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ function compileFunction(code, params, options = {}) {
if (typeof extension !== 'object') {
throw new ERR_INVALID_ARG_TYPE(
`options.contextExtensions[${i}]`,
'object',
'Object',
extension
);
}
Expand Down
20 changes: 20 additions & 0 deletions test/common/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -717,6 +717,25 @@ 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,
Expand All @@ -735,6 +754,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
View file
Open in desktop
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')"
Copy link
Copy Markdown
Member

@Trott Trott Dec 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Micro-nit, but in messages like this one, I think it's more idiomatic for strings to be in double-quotes. Of course, both are fine in JavaScript so feel free to ignore this. People will know what is meant. (Only noticed it myself because job on the line above is in double quotes, but that's not even a string variable value, it's an identifier, so
¯\(ツ)/¯ but I did have to stop and wonder why one was in double-quotes and another in single-quotes.)

Copy link
Copy Markdown
Member Author

@BridgeAR BridgeAR Dec 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rely upon .inspect() in this case and it's not possible to configure the quotation in that case. Therefore I'd stick to this for now (even though it would be possible to special handle strings).

}
);

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
View file
Open in desktop
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
Loading