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

errors: add internal/errors.js #11220

Closed
wants to merge 1 commit into from
Closed

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Feb 7, 2017

Add the internal/errors.js core mechanism.

This was extracted from #9265 with some updates. It adds the basic mechanism for moving to the internal errors module without modifying any of the actual error instances within core. Transitioning to the use of this module should be done incrementally in small chunks over time.

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

errors

@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 7, 2017
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Feb 7, 2017

## Adding new errors

New static error codes are adding by modifying the `internal/errors.js` file
Copy link
Member

Choose a reason for hiding this comment

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

s/adding/added/

E('EXAMPLE_KEY2', (a, b) => return `${a} ${b}`);
```

The first argument passed to `E()` is the static identifier. The second
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, the name of this function being so short is to echo the existing convention for error names like ENOTFOUND? But this convention is no longer followed in this new error code system?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is short to make it as brief as possible. It is intended to be used only within this file. It is exported only to facilitate testing.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but this is a guide so I think this would be read by contributors, especially those who are new? And they might need to change that file and call E to register new errors.

### Class: errors.Error(key[, args...])

* `key` {String} The static error identifier
* `args... {Any} Zero or more optional arguments
Copy link
Member

Choose a reason for hiding this comment

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

Are the arguments left undocumented because they are non-standard? Probably an explanation would help readers to understand what they are(e.g. a link to MDN)

Copy link
Member Author

Choose a reason for hiding this comment

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

The arguments are arbitrary and dependent on the specific error message (essentially the same as util.format(). Specifically, the error message for a key may be something like 'Foo %s %d', so that errors.Error('KEY', 's', '1') would produce the error message 'Foo s 1'

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation, but does this built-in errors.Error() method have a formatting function defined? It seems to just pass the arguments to the Error base class, so we already know what the format would be(at least for V8)?

Copy link
Member

Choose a reason for hiding this comment

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

Or, do we allow users to customize the format of these builtin errors?

Copy link
Member Author

Choose a reason for hiding this comment

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

Look at the message() function. When a key is registered using the E() function, it's associated value is either a string with optional util.format tokens or a function that returns a string. Inside message(), the key lookup returns this value. If the value is a string, it is passed through util.format(), otherwise the arguments are passed to the function, allowing more customized formatting for the error.

Again, this is not a user facing API. This is for use only within core and would be used anywhere we throw an internal error.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, yes, but "users" I meant "consumers of this API, i.e. contributors". I think my questions have been answered, thanks.

On a side note, I think I was having questions because the formatting behavior is explained near the bottom of this document, so by the time I read this, I was not sure how the args are going to be consumed. A reference to the formatting process might help, or maybe the formatting process can be explained first, but maybe that's just me :).

class NodeError extends Error {
constructor(key /**, ...args **/) {
lazyAssert().strictEqual(typeof key, 'string');
const args = Array(arguments.length - 1);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can just use the rest operator? Probably faster in newer versions of V8.

Copy link
Member Author

Choose a reason for hiding this comment

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

rest parameters are not yet fully optimized. There is a benchmark already to test those. Once they have been optimized we can switch.

Copy link
Member

Choose a reason for hiding this comment

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

Do you refer to benchmark/misc/console.js? I just run it with the latest master and it seems the usage with arguments is already optimized?

./node benchmark/run.js --filter console misc

misc/console.js
misc/console.js n=1000000 concat=1 method="restAndSpread": 499,544.4494311414
misc/console.js n=1000000 concat=0 method="restAndSpread": 513,992.8268692781
misc/console.js n=1000000 concat=1 method="argumentsAndApply": 619,756.4629049919
misc/console.js n=1000000 concat=0 method="argumentsAndApply": 624,507.957596242
misc/console.js n=1000000 concat=1 method="restAndApply": 688,473.7438723565
misc/console.js n=1000000 concat=0 method="restAndApply": 681,323.2961384911
misc/console.js n=1000000 concat=1 method="restAndConcat": 895,355.2343246405

Copy link
Member Author

Choose a reason for hiding this comment

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

It's possible that things have improved with the most recent v8 drops. I'm running the benchmark/es/restparams-bench.js right now to compare

Copy link
Member

Choose a reason for hiding this comment

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

Oh, yes, those are the right benchmarks.

Also I discovered something interesting. If I leave the forced optimization there, the results are being consistent with:

es/restparams-bench.js
es/restparams-bench.js millions=100 method="copy": 30.148206272683915
es/restparams-bench.js millions=100 method="rest": 22.696521524914942
es/restparams-bench.js millions=100 method="arguments": 55.12597317932212

But if I comment out the forced optimization:

es/restparams-bench.js
es/restparams-bench.js millions=100 method="copy": 30.754392030574046
es/restparams-bench.js millions=100 method="rest": 33.315344080305636
es/restparams-bench.js millions=100 method="arguments": 57.77862383401849

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, very nice, these results are MUCH better than they were just a couple of months ago. Switching to rest params!!

                                                        improvement confidence   p.value
 es/restparams-bench.js millions=100 method="arguments"     -0.17 %            0.9399783
 es/restparams-bench.js millions=100 method="copy"           0.02 %            0.9923623
 es/restparams-bench.js millions=100 method="rest"           4.06 %            0.1729710


class NodeError extends Error {
constructor(key /**, ...args **/) {
lazyAssert().strictEqual(typeof key, 'string');
Copy link
Member

Choose a reason for hiding this comment

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

A more informative message would be helpful here.

Copy link
Member Author

Choose a reason for hiding this comment

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

This assert is not intended to be user facing. This is simply to make sure proper use of the internal API and is here only to make sure that node itself is passing in the right kind of value.

function message(key, args) {
const assert = lazyAssert();
const util = lazyUtil();
const msg = exports[symbolFor(key)];
Copy link
Member

Choose a reason for hiding this comment

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

If I am reading this right, the second time a key is passed into this, it will be in the map and this assertion won't fail, even though the error is not properly registered with E?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, right, good point.

assert.strictEqual(err4.message, 'abc xyz');
assert.strictEqual(err4.code, 'TEST_ERROR_2');

assert.throws(
Copy link
Member

Choose a reason for hiding this comment

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

What happens if this block is executed twice?(TEST_FOO_KEY gets passed twice)

The `internal/errors` module exposes three custom `Error` classes as a
`message` function.

### Class: errors.Error(key[, args...])
Copy link
Member

Choose a reason for hiding this comment

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

Since we now have rest parameters, I’d be in favour of using ...args in these places if that’s possible?

### Class: errors.RangeError(key[, args...])

* `key` {String} The static error identifier
* `args... {Any} Zero or more optional arguments
Copy link
Member

Choose a reason for hiding this comment

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

There’s a missing backtick on all copies of this line. :D

return util;
}

class NodeError extends Error {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this can be a mixin to avoid code duplication?

Copy link
Member Author

Choose a reason for hiding this comment

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

Possibly. I did it this way in order to preserve instanceof checks (e.g. (new error.TypeError()) instanceof TypeError) but may be able to find a way around that.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure, would that really be much of a problem? What I had in mind was something like this:

const makeNodeError = (Base) => class NodeError extends Base {
  [ current code, except only once ]
  static get name() { return `Node${super.name}`; } /* optional */
};

const NodeError = makeNodeError(Error);
const NodeRangeError = makeNodeError(RangeError);
const NodeTypeError = makeNodeError(TypeError);

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah right :-) .. that works too

@jasnell
Copy link
Member Author

jasnell commented Feb 7, 2017

Updated! PTAL

@jasnell
Copy link
Member Author

jasnell commented Feb 7, 2017

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Looking good, thanks for (re-)starting this!

And by the way, I don’t think this really counts semver-minor if it doesn’t touch the public API. (Probably makes no difference, though. 😄)

file:

```js
E('FOO', 'Expected string received %s);
Copy link
Member

Choose a reason for hiding this comment

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

Missing quote ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

grr.. lol ...

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, the linter doesn't lint code inside doc/guides?

return msg;
args.unshift(msg);
}
return String(fmt.apply(null, args));
Copy link
Member

Choose a reason for hiding this comment

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

fmt(...args);? Not sure whether that was part of your conversation with @joyeecheung or not

Copy link
Member

Choose a reason for hiding this comment

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

That conversation is about rest operator and this is spread operator...not sure about the performance of spread though? From the results of benchmark/misc/console.js with method="restAndSpread" compared method="restAndApply" I think the spread operator is not ready yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't look like we have a benchmark for the spread operator so I'll open a PR to add one

kSymbolsMap.set(name, sym);
}
return sym;
}
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure I understand why this file uses symbol properties of exports… wouldn’t const messages = new Map(); as a key → message map be a bit easier?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah doesn't really make much sense now. This was originally done to make it easier to discover/enumerate these but that originated in a much older version

@jasnell
Copy link
Member Author

jasnell commented Feb 7, 2017

@jasnell
Copy link
Member Author

jasnell commented Feb 7, 2017

Failure in CI is unrelated.

@evanlucas
Copy link
Contributor

So, one concern I have is how will we backport these? Will we set the error codes on master and then just backport? If so, what happens if a new error is added to just a release line, but not to master?

@jasnell
Copy link
Member Author

jasnell commented Feb 8, 2017

This would likely need to land in Node.js 8.0.0 and not be backported. The PRs to port existing errors to this mechanism would be semver-major initially so would no backport to older majors. Only changes after porting would be semver-minor or patch.

As much as possible we should try to avoid adding new types of errors (new error codes). On the off chance that we are forced to land something in a release line and not master, those would land in that release line and should be added to the documentation in master but not necessarily be used in master. Hopefully that made sense.

@evanlucas
Copy link
Contributor

Thanks @jasnell! That makes sense to me. Just wanted to make sure it had been thought through. :]

static, permanent error code and an optionally parameterized message. The
intent of the module is to make errors thrown by Node.js core to be more
consistent and to allow changes to error messages to be made without requiring
a semver-major label.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: semver-major label is kind of our internal jargon and people aren't going to understand (without an explanation) why we consider error message changes as semver major anyway.

Maybe just embrace completeness as this is a guide and go with something more like this?:

...parameterized message. [END OF PARAGRAPH]

The intent of the module is to allow errors provided by Node.js to be assigned a
permanent identifier. Without a permanent identifier, userland code may need to inspect
error messages to distinguish one error from another. An unfortunate result
of that practice is that changes to error messages result in broken code in the ecosystem.
For that reason, Node.js has considered error message changes to be breaking changes.
By providing a permanent identifier for a specific error, we reduce the need for userland
code to inspect error messages.

@Trott
Copy link
Member

Trott commented Feb 9, 2017

@jasnell This feature is long overdue. Thanks for doing all the work to get us to the point where we can do this!

Add the internal/errors.js core mechanism.
@jasnell
Copy link
Member Author

jasnell commented Feb 9, 2017

@Trott, updated with your suggestion in the guide

@jasnell
Copy link
Member Author

jasnell commented Feb 9, 2017

New CI before landing: https://ci.nodejs.org/job/node-test-pull-request/6315/

jasnell added a commit that referenced this pull request Feb 9, 2017
Add the internal/errors.js core mechanism.

PR-URL: #11220
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Feb 9, 2017

Landed in 159749d

@jasnell jasnell closed this Feb 9, 2017
italoacasas pushed a commit that referenced this pull request Feb 13, 2017
Add the internal/errors.js core mechanism.

PR-URL: #11220
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@@ -616,3 +616,18 @@ exports.WPT = {
assert.fail(undefined, undefined, `Reached unreachable code: ${desc}`);
}
};

// Useful for testing expected internal/error objects
exports.expectsError = function expectsError(code, type, message) {
Copy link
Member

Choose a reason for hiding this comment

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

This function was added to common.js but not documented in the test/README.md.

@jasnell
Copy link
Member Author

jasnell commented Feb 14, 2017

Why? None of those existing errors or codes are affected by this in any way. The code property was selected precisely because it is already used.

@jasnell
Copy link
Member Author

jasnell commented Feb 14, 2017

To be certain, this mechanism does not change or override any of the existing core uses of .code, does not interfere with any known public uses of .code, and would be used only to add .code to errors generated by core that do not already have a .code property.

@joyeecheung
Copy link
Member

Hmm, so for now we just leave the err.code untouched for those modules that already set this, and update all the other modules to set err.code with a new convention? That sounds good to me, though we should document about this inconsistency, otherwise it just causes more confusions to the users IMO.

@jasnell
Copy link
Member Author

jasnell commented Feb 14, 2017

For now, yes. Eventually we should work on making all of the errors consistent, but for those errors that already have a .code, there's no immediate need to change those.

}

get code() {
return this[kCode];
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, so if a PR updates a module to throw the new errors, since code is read only here, any code setting .code later would not have effect, then this could break existing behavior unless we check carefully enough before landing those PRs? Can we add a setter that do some kinds of assertion here?

Copy link
Member

@joyeecheung joyeecheung Feb 14, 2017

Choose a reason for hiding this comment

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

For example:

set code(options) {
  if (typeof options === 'object' && options.override === true) {
    this[kCode] = options.code;
    return;
  }
  assert.fail('This error can not be migrated for now');
}

Copy link
Member

Choose a reason for hiding this comment

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

EDIT: forgot return ^

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'd rather just leave it read only. If someone really wanted to override the value of .code, they can easily use Object.defineProperty() to do so.

Copy link
Member

Choose a reason for hiding this comment

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

then just raise the assertion then? This is just to prevent we accidently migrate an error that already have its .code set by existing code (but it could be hard to catch if that code happens much later than its creation, like in a callback), and by throwing this new kind of error, that code can't actually set .code.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are no cases that I'm aware of where we set the code later. And in the case a user attempts to set the code, I'd rather they not see an assertion error. We can best catch these cases in code review

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that can not be let loose in the user land. Anyway thanks for bearing with me :P

@evanlucas
Copy link
Contributor

I think @Trott brings up a good point...

I'm not sure how I feel about error messages changing from TypeError to TypeError[ERR_INVALID_ARG_TYPE]. I didn't realize this changed the output of the error message :[

@Trott
Copy link
Member

Trott commented Feb 14, 2017

I'm not sure how I feel about error messages changing from TypeError to TypeError[ERR_INVALID_ARG_TYPE]. I didn't realize this changed the output of the error message

I'm still not sure where I stand on this. I'm trying to work out the cost/benefit on this.

Like, if I thought that changing the error message to that would mostly-eliminate full-message-text-sniffing and therefore allow us to treat message changes as semver-patch, I'd probably be all for it.

But I'm not sure I can convince myself that will happen.

And there's a potential of a lot of adverse effects on the ecosystem from this.

@jasnell
Copy link
Member Author

jasnell commented Feb 14, 2017

And there's a potential of a lot of adverse effects on the ecosystem from this

Serious question: Like what?

The code is included in the .name property (and thus printed when the Error is output to the console) in order to give users come static identifier they can copy and google for as easily as possible, even if the message itself changes (or, at some point down the road, is translated).

The .code property allows this to be easily acquired programatically for the same effect.

The actual .message property is not touched, except when going through and normalizing the error messages that Node.js is producing. The error code is omitted from the message property specifically to discourage users from parsing the message to get that information. Any regexp checks on the message will only be impacted by the actual changes to the error messages and not by the inclusion of the code in the name.

For each of these errors, err instanceof Error, err instanceof TypeError and err instanceof RangeError still works as expected. util.isError(err) returns true. Error.prototype.toString(err) returns Error.

@Trott
Copy link
Member

Trott commented Feb 14, 2017

This is a total nit, but the lack of space before the opening square bracket suggests it's an index or property operator and not for human consumption. I wonder if it would be better to use parentheses and to use a space. Maybe put it on the other side of the colon while we're at it:

TypeError: (ERR_INVALID_ARG_TYPE) The "warning" argument must be one of type Error, or string

@jasnell
Copy link
Member Author

jasnell commented Feb 14, 2017

Putting it on the other side of the colon implies that we're either customizing the toString() output or prepending it to the message, neither of which is something that I'd like to do as it becomes a much more invasive change. We could wrap within parens and put a space in front.

@Trott
Copy link
Member

Trott commented Feb 14, 2017

Serious question: Like what?

Anything that uses assert.throws() with a regular expression could be susceptible. Anything that matches on /TypeError: foo/ now needs to match on /TypeError[ERR_CODE]: foo/. (This problem is actually how I first noticed this change. A previously-passing assert.throws() was failing in a PR because the regexp did not have the [ERR_CODE] part.)

You raise some good points I didn't realize that mitigate things. I guess CITGM runs on the various PRs can help provide some data about how widespread issues might be.

@jasnell
Copy link
Member Author

jasnell commented Feb 14, 2017

Yep, uses of assert.throws() can be impacted just like any error message change, which is why moving to this new mechanism requires a semver-major label. It would have no more significant impact than making any change to an error message (add a period to your /TypeError: foo\./ and you also run into issues). This is not unique to this change and is why the change is opt in, and why common.expectsError() is added for our own tests.

@Trott
Copy link
Member

Trott commented Feb 15, 2017

Yep, uses of assert.throws() can be impacted just like any error message change, which is why moving to this new mechanism requires a semver-major label. It would have no more significant impact than making any change to an error message (add a period to your /TypeError: foo./ and you also run into issues). This is not unique to this change and is why the change is opt in, and why common.expectsError() is added for our own tests.

I think these two things are very different:

  • "we changed a handful of error messages in this next major version and it's going to break a few people's tests here and there"

  • "we've changed every/nearly-every/most/half/whatever error message emitted by Node.js core"

@jasnell
Copy link
Member Author

jasnell commented Feb 15, 2017

Yep, which is why changes need to happen incrementally. The initial version of this pr included changes for everything, all at once. I backed off that so that individual changes can be looked at case by case

@Trott
Copy link
Member

Trott commented Feb 15, 2017

BTW, I'm not arguing against this. I'm still trying to assess it.

The fact that this will change pretty much all (or at least a very large number of) core error messages was a surprise to me. Judging from his comments, it surprised @evanlucas as well. Small data set, yes. It's entirely possible this is totally the way to go.
¯\(ツ)

italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 25, 2017
Add the internal/errors.js core mechanism.

PR-URL: nodejs#11220
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@italoacasas italoacasas mentioned this pull request Feb 25, 2017
krydos pushed a commit to krydos/node that referenced this pull request Feb 25, 2017
Add the internal/errors.js core mechanism.

PR-URL: nodejs#11220
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. errors Issues and PRs related to JavaScript errors originated in Node.js core. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants