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.

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

process: fix error message of hrtime() #13739

Closed
wants to merge 3 commits into from

Conversation

tniessen
Copy link
Member

process.hrtime() incorrectly passes the function name to errors.TypeError instead of the name of the argument.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

process


I briefly discussed this with @addaleax. We assume that process.hrtime was passed to errors.TypeError due to the fact that it is easier to understand than the name of the parameter (time). I would still opt for consistency within our codebase and error messages, therefore this PR.

To be honest, most error messages of this type are relatively useless without the stack as we never include the function name in the message itself. We could add an argument to ERR_INVALID_ARG_TYPE which allows to specify the function, but that would make the error messages longer.

As always, needs to be approved by @nodejs/ctc.

process.hrtime() incorrectly passes the function name to
errors.TypeError instead of the name of the argument.
@tniessen tniessen added errors Issues and PRs related to JavaScript errors originated in Node.js core. process Issues and PRs related to the process subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Jun 17, 2017
@tniessen tniessen self-assigned this Jun 17, 2017
@nodejs-github-bot nodejs-github-bot added the process Issues and PRs related to the process subsystem. label Jun 17, 2017
@tniessen
Copy link
Member Author

@@ -84,8 +84,7 @@ function setup_hrtime() {
const needsBorrow = nsec < 0;
return [needsBorrow ? sec - 1 : sec, needsBorrow ? nsec + 1e9 : nsec];
}
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'process.hrtime()', 'Array');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'time', 'Array');
Copy link
Member

Choose a reason for hiding this comment

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

Should we add the actual argument now that we are here?

Copy link
Member Author

@tniessen tniessen Jun 18, 2017

Choose a reason for hiding this comment

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

This comes with a problem. After adding the actual argument, the error message looks like 'The "time" argument must be of type Array. Received type object' for arrays with an invalid size. The correct message would be something like 'The "time" array must have a length of 2. Received 3'. Any ideas for a solution? Maybe a new error code ERR_INVALID_ARRAY_LENGTH?

Copy link
Member

Choose a reason for hiding this comment

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

This could be improved by rewriting the statements to something like:

if (!Array.isArray(time)) {
  throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'time', 'Array', time);
}
if (time.length !== 2) {
  throw new errors.TypeError('ERR_TYPE_X', 'time', 2, time);
}
const sec = (hrValues[0] * 0x100000000 + hrValues[1]) - time[0];
const nsec = hrValues[2] - time[1];
const needsBorrow = nsec < 0;
return [needsBorrow ? sec - 1 : sec, needsBorrow ? nsec + 1e9 : nsec];

Copy link
Member Author

Choose a reason for hiding this comment

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

@BridgeAR I know how it can be rewritten, that is not the point (I am actually already running tests against a fix). This is more of a design decision whether we want a code ERR_INVALID_ARRAY_LENGTH or such.

@tniessen
Copy link
Member Author

I pushed a commit proposing a new error code ERR_INVALID_ARRAY_LENGTH and would like some feedback on it.

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM with a comment.

throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'process.hrtime()', 'Array');
if (time.length !== 2) {
throw new errors.TypeError('ERR_INVALID_ARRAY_LENGTH', 'time', 2,
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be better as a RangeError.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't sure about this either. RangeError sounds logical at first considering that the length is out of the valid range (even though that range only includes a single value), but on the other hand, the array is used like a mathematical tuple and I would say that for tuples, the number of elements is part of the type. IMO RangeError is better when it is either about a numeric value passed to a function or if there is actually a specific range of allowed implicit values such as the array length (which permits more than a single value). Both TypeError and RangeError are fine by me, but I would like some other opinions on this.

Copy link
Member

Choose a reason for hiding this comment

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

RangeError is defined in ES as

Indicates a value that is not in the set or range of allowable values.

Here, it's not that time is not in the set or range of allowable values, but rather a property of it, so TypeError should be more appropriate.

The definition also implies that RangeError can also be used for strings, such as in String.prototype.normalize.

tniessen added a commit that referenced this pull request Jun 20, 2017
process.hrtime() incorrectly passed the function name to
errors.TypeError instead of the name of the argument.
Additionally, the type of the actual argument was added to the error
message and a new error code ERR_INVALID_ARRAY_LENGTH was added.

PR-URL: #13739
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@tniessen
Copy link
Member Author

Landed in a0f7284. Quick CI against master: https://ci.nodejs.org/job/node-test-commit/10689/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. process Issues and PRs related to the process subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants