Skip to content
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

Jump to bottom

internal/errors: improve invalid arg type #14057

Closed
wants to merge 4 commits into from
Closed

internal/errors: improve invalid arg type #14057

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b62c7d3
lib: fix expected and actual args
BridgeAR Jun 21, 2017
8c67f24
lib: always include the actual value
BridgeAR Jun 21, 2017
c5b1ba5
internal/errors: improve error messages
BridgeAR Jun 20, 2017
1717cb9
test: fix tests for changed error messages
BridgeAR Jun 21, 2017
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
23 changes: 15 additions & 8 deletions lib/dgram.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,19 +262,22 @@ Socket.prototype.sendto = function(buffer,
address,
callback) {
if (typeof offset !== 'number') {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'offset', 'number');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'offset', 'number',
offset);
}

if (typeof length !== 'number') {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'length', 'number');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'length', 'number',
length);
}

if (typeof port !== 'number') {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'port', 'number');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'port', 'number', port);
}

if (typeof address !== 'string') {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'address', 'string');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'address', 'string',
address);
}

this.send(buffer, offset, length, port, address, callback);
Expand All @@ -287,7 +290,8 @@ function sliceBuffer(buffer, offset, length) {
} else if (!isUint8Array(buffer)) {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'buffer',
['Buffer', 'Uint8Array', 'string']);
['Buffer', 'Uint8Array', 'string'],
buffer);
}

offset = offset >>> 0;
Expand Down Expand Up @@ -380,14 +384,16 @@ Socket.prototype.send = function(buffer,
} else if (!isUint8Array(buffer)) {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'buffer',
['Buffer', 'Uint8Array', 'string']);
['Buffer', 'Uint8Array', 'string'],
buffer);
} else {
list = [ buffer ];
}
} else if (!(list = fixBufferList(buffer))) {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'buffer list arguments',
['Buffer', 'string']);
['Buffer', 'string'],
buffer);
}

port = port >>> 0;
Expand All @@ -405,7 +411,8 @@ Socket.prototype.send = function(buffer,
} else if (address && typeof address !== 'string') {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'address',
['string', 'falsy']);
['string', 'falsy'],
address);
}

this._healthCheck();
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/child_process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ ChildProcess.prototype.spawn = function(options) {
options.envPairs = [];
else if (!Array.isArray(options.envPairs)) {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options.envPairs',
'array', options.envPairs);
'Array', options.envPairs);
}

options.envPairs.push('NODE_CHANNEL_FD=' + ipcFd);
Expand All @@ -301,7 +301,7 @@ ChildProcess.prototype.spawn = function(options) {
else if (options.args === undefined)
this.spawnargs = [];
else
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options.args', 'array',
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options.args', 'Array',
options.args);

var err = this._handle.spawn(options);
Expand Down
37 changes: 26 additions & 11 deletions lib/internal/errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ function makeNodeError(Base) {
class AssertionError extends Error {
constructor(options) {
if (typeof options !== 'object' || options === null) {
throw new exports.TypeError('ERR_INVALID_ARG_TYPE', 'options', 'object');
throw new exports.TypeError('ERR_INVALID_ARG_TYPE', 'options', 'object',
options);
}
const util = lazyUtil();
const message = options.message ||
Expand Down Expand Up @@ -150,7 +151,7 @@ E('ERR_INVALID_URL', 'Invalid URL: %s');
E('ERR_INVALID_URL_SCHEME',
(expected) => {
lazyAssert();
return `The URL must be ${oneOf(expected, 'scheme')}`;
return `The URL must be ${oneOf(expected, 'of scheme')}`;
});
E('ERR_IPC_CHANNEL_CLOSED', 'Channel closed');
E('ERR_IPC_DISCONNECTED', 'IPC channel is already disconnected');
Expand Down Expand Up @@ -183,12 +184,26 @@ E('ERR_V8BREAKITERATOR', 'Full ICU data not installed. ' +
function invalidArgType(name, expected, actual) {
const assert = lazyAssert();
assert(name, 'name is required');
const type = name.includes('.') ? 'property' : 'argument';
var msg = `The "${name}" ${type} must be ${oneOf(expected, 'type')}`;
if (arguments.length >= 3) {
msg += `. Received type ${actual !== null ? typeof actual : 'null'}`;
assert.strictEqual(arguments.length, 3, 'arguments length mismatch');
const argType = name.includes('.') ? 'property' : 'argument';
// Only check the first entry
const expectedType = Array.isArray(expected) ? expected[0] : expected;
// A-Z = 65-90, a-z = 97-122
const type = expectedType.charCodeAt(0) > 96 ? 'of type' : 'instance of';
var received;
if (actual == null) {
received = String(actual);
} else if (typeof actual === 'object') {
if (actual.constructor != null) {
received = `instance of ${actual.constructor.name}`;
Copy link

Choose a reason for hiding this comment

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

Provided that actual is an arbitrary user-provided object which we are validating, after this patch, exceptions can be raised on any of:

  • actual.constructor access.
  • actual.constructor.name access.
  • String concatenation with actual.constructor.name (if it's a Symbol or String(actual.constructor.name) throws)

Copy link

Choose a reason for hiding this comment

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

This is an uncommon code path, so wrapping in try-catch should be fine?

Copy link
Member Author

@BridgeAR BridgeAR Jul 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I do not agree that it's useful to guard against a constructor or name property set to a getter that throws or being set to a symbol.
If this would ever result in an error, nothing bad would happen besides showing the "wrong" error.

} else {
received = 'a plain object';
}
} else {
received = `type ${typeof actual}`;
}
return msg;
return `The "${name}" ${argType} must be ${oneOf(expected, type)}. ` +
`Received ${received}`;
}

function missingArgs(...args) {
Expand Down Expand Up @@ -220,14 +235,14 @@ function oneOf(expected, thing) {
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 ` +
return `${thing} ${expected.slice(0, len - 1).join(', ')}, or ` +
expected[len - 1];
} else if (len === 2) {
return `one of ${thing} ${expected[0]} or ${expected[1]}`;
return `${thing} ${expected[0]} or ${expected[1]}`;
} else {
return `of ${thing} ${expected[0]}`;
return `${thing} ${expected[0]}`;
}
} else {
return `of ${thing} ${String(expected)}`;
return `${thing} ${String(expected)}`;
}
}
8 changes: 5 additions & 3 deletions lib/internal/process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,14 @@ function setup_cpuUsage() {
if (prevValue) {
if (!previousValueIsValid(prevValue.user)) {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'preValue.user', 'Number');
'prevValue.user', 'number', prevValue.user);
}

if (!previousValueIsValid(prevValue.system)) {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'preValue.system', 'Number');
'prevValue.system',
'number',
prevValue.system);
}
}

Expand Down Expand Up @@ -170,7 +172,7 @@ function setupKillAndExit() {

// eslint-disable-next-line eqeqeq
if (pid != (pid | 0)) {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'pid', 'Number');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'pid', 'number', pid);
}

// preserve null signal
Expand Down
10 changes: 7 additions & 3 deletions lib/internal/process/warning.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,9 +130,11 @@ function setupProcessWarnings() {
code = undefined;
}
if (code !== undefined && typeof code !== 'string')
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'code', 'string');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'code', 'string',
code);
if (type !== undefined && typeof type !== 'string')
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'type', 'string');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'type', 'string',
type);
if (warning === undefined || typeof warning === 'string') {
warning = new Error(warning);
warning.name = String(type || 'Warning');
Expand All @@ -142,7 +144,9 @@ function setupProcessWarnings() {
}
if (!(warning instanceof Error)) {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'warning', ['Error', 'string']);
'warning',
['Error', 'of type string'],
warning);
}
if (warning.name === 'DeprecationWarning') {
if (process.noDeprecation)
Expand Down
3 changes: 2 additions & 1 deletion lib/internal/url.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,8 @@ Object.defineProperties(URL.prototype, {
// eslint-disable-next-line func-name-matching
value: function format(options) {
if (options && typeof options !== 'object')
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options', 'object');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options', 'object',
options);
options = Object.assign({
fragment: true,
unicode: false,
Expand Down
5 changes: 3 additions & 2 deletions lib/internal/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ function deprecate(fn, msg, code) {
}

if (code !== undefined && typeof code !== 'string')
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'code', 'string');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'code', 'string', code);

var warned = false;
function deprecated(...args) {
Expand Down Expand Up @@ -204,7 +204,8 @@ const kCustomPromisifyArgsSymbol = Symbol('customPromisifyArgs');
function promisify(orig) {
if (typeof orig !== 'function') {
const errors = require('internal/errors');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'original', 'function');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'original', 'function',
orig);
}

if (orig[kCustomPromisifiedSymbol]) {
Expand Down
10 changes: 5 additions & 5 deletions lib/path.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ const errors = require('internal/errors');

function assertPath(path) {
if (typeof path !== 'string') {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'path', 'string');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'path', 'string', path);
}
}

Expand Down Expand Up @@ -816,7 +816,7 @@ const win32 = {

basename: function basename(path, ext) {
if (ext !== undefined && typeof ext !== 'string')
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'ext', 'string');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'ext', 'string', ext);
assertPath(path);
var start = 0;
var end = -1;
Expand Down Expand Up @@ -959,7 +959,7 @@ const win32 = {

format: function format(pathObject) {
if (pathObject === null || typeof pathObject !== 'object') {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'pathObject', 'Object',
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'pathObject', 'object',
pathObject);
}
return _format('\\', pathObject);
Expand Down Expand Up @@ -1371,7 +1371,7 @@ const posix = {

basename: function basename(path, ext) {
if (ext !== undefined && typeof ext !== 'string')
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'ext', 'string');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'ext', 'string', ext);
assertPath(path);

var start = 0;
Expand Down Expand Up @@ -1502,7 +1502,7 @@ const posix = {

format: function format(pathObject) {
if (pathObject === null || typeof pathObject !== 'object') {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'pathObject', 'Object',
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'pathObject', 'object',
pathObject);
}
return _format('/', pathObject);
Expand Down
2 changes: 1 addition & 1 deletion lib/repl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1052,7 +1052,7 @@ REPLServer.prototype.defineCommand = function(keyword, cmd) {
cmd = {action: cmd};
} else if (typeof cmd.action !== 'function') {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'action', 'function', cmd.action);
'cmd.action', 'function', cmd.action);
}
this.commands[keyword] = cmd;
};
Expand Down
24 changes: 17 additions & 7 deletions lib/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,11 @@ Object.defineProperty(inspect, 'defaultOptions', {
},
set: function(options) {
if (options === null || typeof options !== 'object') {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options', 'object');
throw new errors.TypeError(
'ERR_INVALID_ARG_TYPE',
'options',
'object',
options);
}
Object.assign(inspectDefaultOptions, options);
return inspectDefaultOptions;
Expand Down Expand Up @@ -957,14 +961,18 @@ exports.log = function() {
exports.inherits = function(ctor, superCtor) {

if (ctor === undefined || ctor === null)
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'ctor', 'function');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'ctor', 'function',
ctor);

if (superCtor === undefined || superCtor === null)
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'superCtor', 'function');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'superCtor', 'function',
superCtor);

if (superCtor.prototype === undefined) {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'superCtor.prototype',
'function');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'superCtor.prototype',
'function',
superCtor.prototype);
}
ctor.super_ = superCtor;
Object.setPrototypeOf(ctor.prototype, superCtor.prototype);
Expand Down Expand Up @@ -1077,7 +1085,8 @@ function callbackify(original) {
throw new errors.TypeError(
'ERR_INVALID_ARG_TYPE',
'original',
'function');
'function',
original);
}

// We DO NOT return the promise as it gives the user a false sense that
Expand All @@ -1089,7 +1098,8 @@ function callbackify(original) {
throw new errors.TypeError(
'ERR_INVALID_ARG_TYPE',
'last argument',
'function');
'function',
maybeCb);
}
const cb = (...args) => { Reflect.apply(maybeCb, this, args); };
// In true node style we process the callback on `nextTick` with all the
Expand Down
13 changes: 10 additions & 3 deletions test/parallel/test-assert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -676,7 +676,14 @@ try {
{
// Verify that throws() and doesNotThrow() throw on non-function block
function typeName(value) {
return value === null ? 'null' : typeof value;
if (value == null) {
return value;
}
const type = typeof value;
if (type !== 'object') {
return `type ${type}`;
}
return `instance of ${value.constructor.name}`;
}

const testBlockTypeError = (method, block) => {
Expand All @@ -690,7 +697,7 @@ try {
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: 'The "block" argument must be of type function. Received ' +
'type ' + typeName(block)
typeName(block)
})(e);
}

Expand Down Expand Up @@ -732,7 +739,7 @@ assert.throws(() => {
{
// bad args to AssertionError constructor should throw TypeError
const args = [1, true, false, '', null, Infinity, Symbol('test'), undefined];
const re = /^The "options" argument must be of type object$/;
const re = /^The "options" argument must be of type object\. Received /;
args.forEach((input) => {
assert.throws(
() => new assert.AssertionError(input),
Expand Down
Loading