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

@tniessen tniessen commented Jun 17, 2017

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.
@@ -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');

This comment has been minimized.

@lpinca

lpinca Jun 18, 2017
Member

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

This comment has been minimized.

@tniessen

tniessen Jun 18, 2017
Author Member

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?

This comment has been minimized.

@BridgeAR

BridgeAR Jun 18, 2017
Member

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];

This comment has been minimized.

@tniessen

tniessen Jun 18, 2017
Author Member

@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.

@lpinca
lpinca approved these changes Jun 18, 2017
@tniessen
Copy link
Member Author

@tniessen tniessen commented Jun 18, 2017

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

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,

This comment has been minimized.

@cjihrig

cjihrig Jun 18, 2017
Contributor

This might be better as a RangeError.

This comment has been minimized.

@tniessen

tniessen Jun 18, 2017
Author Member

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.

This comment has been minimized.

@TimothyGu

TimothyGu Jun 19, 2017
Member

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

@tniessen tniessen commented Jun 20, 2017

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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.