Skip to content

assert: move assertion part in extra file #20486

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 3 commits into from
Closed

assert: move assertion part in extra file #20486

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5b0038a
doc: update assert documentation
BridgeAR May 2, 2018
9fce095
assert: move AssertionError into own file
BridgeAR May 2, 2018
4c9c77d
errors: move functions to error code
BridgeAR May 2, 2018
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
errors: move functions to error code 
This makes sure the functions are actually directly beneat the
specification of an error code.
That way it is not necessary to jump around when looking at the
functionality.
  • Loading branch information
@BridgeAR
BridgeAR committed May 7, 2018
commit 4c9c77d9954d74541f597fb39afc3776985a971c
189 changes: 92 additions & 97 deletions lib/internal/errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,26 @@ function isStackOverflowError(err) {
err.message === maxStack_ErrorMessage;
}

function oneOf(expected, thing) {
assert(typeof thing === 'string', '`thing` has to be of type string');
if (Array.isArray(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)}`;
}
}

module.exports = {
dnsException,
errnoException,
Expand Down Expand Up @@ -441,7 +461,15 @@ E('ERR_ARG_NOT_ITERABLE', '%s must be iterable', TypeError);
E('ERR_ASSERTION', '%s', Error);
E('ERR_ASYNC_CALLBACK', '%s must be a function', TypeError);
E('ERR_ASYNC_TYPE', 'Invalid name for async "type": %s', TypeError);
E('ERR_BUFFER_OUT_OF_BOUNDS', bufferOutOfBounds, RangeError);
E('ERR_BUFFER_OUT_OF_BOUNDS',
// Using a default argument here is important so the argument is not counted
// towards `Function#length`.
(name = undefined) => {
if (name) {
return `"${name}" is outside of buffer bounds`;
}
return 'Attempt to write outside buffer bounds';
}, RangeError);
E('ERR_BUFFER_TOO_LARGE',
`Cannot create a Buffer larger than 0x${kMaxLength.toString(16)} bytes`,
RangeError);
Expand Down Expand Up @@ -579,7 +607,32 @@ E('ERR_INSPECTOR_CLOSED', 'Session was closed', Error);
E('ERR_INSPECTOR_NOT_AVAILABLE', 'Inspector is not available', Error);
E('ERR_INSPECTOR_NOT_CONNECTED', 'Session is not connected', Error);
E('ERR_INVALID_ADDRESS_FAMILY', 'Invalid address family: %s', RangeError);
E('ERR_INVALID_ARG_TYPE', invalidArgType, TypeError);
E('ERR_INVALID_ARG_TYPE',
(name, expected, actual) => {
assert(typeof name === 'string', "'name' must be a string");
Copy link
Copy Markdown
Member

@joyeecheung joyeecheung May 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

In general I think it's pretty odd to use assert in internal/errors.js. My impression is that assert in our own code is used to guard against potential user-land monkey-patching gone wrong, but the internal errors are not open to monkey-patching in any way, and most of the internal error code should be simple enough to debug without assertions (even if they are not that simple now, at least they are supposed to be, since internal/errors is required by almost every module). It is especially odd in cases like this where the formatter of ERR_INVALID_ARG_TYPE is effectively guarding against a ERR_INVALID_ARG_TYPE. Even if we want to guard against our own code, we should probably be using DCHECK macros that are only available in debug mode instead of asserts

Copy link
Copy Markdown
Member Author

@BridgeAR BridgeAR May 8, 2018

Choose a reason for hiding this comment

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

I did not add anything new here. Before it was just about using internalAssert instead. In this specific case I do not think it provides a lot of benefit to check this (especially since we do not check input types in any other error). There are a few cases in here that make it much easier to implement the error codes though. I found a few cases that were wrong when validating the passed through arguments and I definitely would like to keep that.

That aside: when I worked on #20567 I also tried to remove assert. As far as I see it is partially used in places where we would now use other errors from in here and partially to prevent malicious values getting through to the C++ layer (you could call it "monkey-patching gone wrong" ;-) ).

Using CHECK and DCHECK for these things would definitely be one way to deal with these. There is just one problem: using e.g., DCHECK means our regular builds while coding will not show any errors and we would only see the issues in the CI. I am not sure if that is fine.

Copy link
Copy Markdown
Member Author

@BridgeAR BridgeAR May 8, 2018

Choose a reason for hiding this comment

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

In general: I guess this discussion is not really about the specific code change?


// determiner: 'must be' or 'must not be'
let determiner;
if (typeof expected === 'string' && expected.startsWith('not ')) {
determiner = 'must not be';
expected = expected.replace(/^not /, '');
} else {
determiner = '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')}`;
}

// TODO(BridgeAR): Improve the output by showing `null` and similar.
msg += `. Received type ${typeof actual}`;
return msg;
}, TypeError);
E('ERR_INVALID_ARG_VALUE', (name, value, reason = 'is invalid') => {
let inspected = util.inspect(value);
if (inspected.length > 128) {
Expand All @@ -595,7 +648,16 @@ E('ERR_INVALID_ASYNC_ID', 'Invalid %s value: %s', RangeError);
E('ERR_INVALID_BUFFER_SIZE',
'Buffer size must be a multiple of %s', RangeError);
E('ERR_INVALID_CALLBACK', 'Callback must be a function', TypeError);
E('ERR_INVALID_CHAR', invalidChar, TypeError);
E('ERR_INVALID_CHAR',
// Using a default argument here is important so the argument is not counted
// towards `Function#length`.
(name, field = undefined) => {
let msg = `Invalid character in ${name}`;
if (field !== undefined) {
msg += ` ["${field}"]`;
}
return msg;
}, TypeError);
E('ERR_INVALID_CURSOR_POS',
'Cannot set cursor row without setting its column', TypeError);
E('ERR_INVALID_FD',
Expand Down Expand Up @@ -644,7 +706,26 @@ E('ERR_IPC_DISCONNECTED', 'IPC channel is already disconnected', Error);
E('ERR_IPC_ONE_PIPE', 'Child process can have only one IPC pipe', Error);
E('ERR_IPC_SYNC_FORK', 'IPC cannot be used with synchronous forks', Error);
E('ERR_METHOD_NOT_IMPLEMENTED', 'The %s method is not implemented', Error);
E('ERR_MISSING_ARGS', missingArgs, TypeError);
E('ERR_MISSING_ARGS',
(...args) => {
assert(args.length > 0, 'At least one arg needs to be specified');
let msg = 'The ';
const len = args.length;
args = args.map((a) => `"${a}"`);
switch (len) {
case 1:
msg += `${args[0]} argument`;
break;
case 2:
msg += `${args[0]} and ${args[1]} arguments`;
break;
default:
msg += args.slice(0, len - 1).join(', ');
msg += `, and ${args[len - 1]} arguments`;
break;
}
return `${msg} must be specified`;
}, TypeError);
E('ERR_MISSING_MODULE', 'Cannot find module %s', Error);
E('ERR_MODULE_RESOLUTION_LEGACY',
'%s not found by import in %s.' +
Expand All @@ -665,7 +746,13 @@ E('ERR_NO_CRYPTO',
E('ERR_NO_ICU',
'%s is not supported on Node.js compiled without ICU', TypeError);
E('ERR_NO_LONGER_SUPPORTED', '%s is no longer supported', Error);
E('ERR_OUT_OF_RANGE', outOfRange, RangeError);
E('ERR_OUT_OF_RANGE',
(name, range, value) => {
let msg = `The value of "${name}" is out of range.`;
if (range !== undefined) msg += ` It must be ${range}.`;
msg += ` Received ${value}`;
return msg;
}, RangeError);
E('ERR_REQUIRE_ESM', 'Must use import to load ES Module: %s', Error);
E('ERR_SCRIPT_EXECUTION_INTERRUPTED',
'Script execution was interrupted by `SIGINT`', Error);
Expand Down Expand Up @@ -762,95 +849,3 @@ E('ERR_VM_MODULE_NOT_MODULE',
'Provided module is not an instance of Module', Error);
E('ERR_VM_MODULE_STATUS', 'Module status %s', Error);
E('ERR_ZLIB_INITIALIZATION_FAILED', 'Initialization failed', Error);

function invalidArgType(name, expected, actual) {
assert(typeof name === 'string', "'name' must be a string");

// determiner: 'must be' or 'must not be'
let determiner;
if (typeof expected === 'string' && expected.startsWith('not ')) {
determiner = 'must not be';
expected = expected.replace(/^not /, '');
} else {
determiner = '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')}`;
}

// TODO(BridgeAR): Improve the output by showing `null` and similar.
msg += `. Received type ${typeof actual}`;
return msg;
}

function missingArgs(...args) {
assert(args.length > 0, 'At least one arg needs to be specified');
let msg = 'The ';
const len = args.length;
args = args.map((a) => `"${a}"`);
switch (len) {
case 1:
msg += `${args[0]} argument`;
break;
case 2:
msg += `${args[0]} and ${args[1]} arguments`;
break;
default:
msg += args.slice(0, len - 1).join(', ');
msg += `, and ${args[len - 1]} arguments`;
break;
}
return `${msg} must be specified`;
}

function oneOf(expected, thing) {
assert(typeof thing === 'string', '`thing` has to be of type string');
if (Array.isArray(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)}`;
}
}

// Using a default argument here is important so the argument is not counted
// towards `Function#length`.
function bufferOutOfBounds(name = undefined) {
if (name) {
return `"${name}" is outside of buffer bounds`;
}
return 'Attempt to write outside buffer bounds';
}

// Using a default argument here is important so the argument is not counted
// towards `Function#length`.
function invalidChar(name, field = undefined) {
let msg = `Invalid character in ${name}`;
if (field !== undefined) {
msg += ` ["${field}"]`;
}
return msg;
}

function outOfRange(name, range, value) {
let msg = `The value of "${name}" is out of range.`;
if (range !== undefined) msg += ` It must be ${range}.`;
msg += ` Received ${value}`;
return msg;
}