Error.name should not contain display elements

[Trott]

• Version: 10.0.0-pre (master)
• Platform: n/a
• Subsystem: error

Remainder of this is a comment by @felixfbecker in #13937 (comment) that I think deserves its own issue:

I agree that adding the code to the name is weird. For me name is a programmatically-inspectable property, just like code, and not for "human consumption" like message. Having pretty formatting in there with a space and brackets goes against that.
Browser errors have no code property, they always use name, so that's where this intuition is coming from. For example, for fetch you would have to check if err.name === 'AbortError'. The only other option is using instanceof, but that is bad practice (assert on interfaces, not implementations).

Personally I see code as a subcategory of name, e.g. name can be TypeError and code ERR_SOCKET_BAD_TYPE. It's both useful to be able to make assertions about the broader category of and error as well as the exact error reason. For example, in an Express error handler, I might want to never return any TypeErrors to the client because they are programming errors. Or take p-retry as an example (emphasis mine):

Returns a Promise that is fulfilled when calling input returns a fulfilled promise. If calling input returns a rejected promise, input is called again until the max retries are reached, it then rejects with the last rejection reason.
It doesn't retry on TypeError as that's a user error.

Checking code here wouldn't be helpful.

I would prefer if both name and code are for programmatic inspection without fancy formatting, and Error.prototype.toString() could be modified to include both error.name and error.code in the output, as toString() is unarguably purely for human consumption. Doing this for all errors, including user errors, would be a great feature and encourage users to make use of error codes too. Alternatively it could only be an internal base error class.

[Trott]

(Aside: There's no nodejs/error team. Wonder if there should be.)

[joyeecheung]

I sometimes find that formatting odd too. I guess the choice is to make the error message look like ErrorName [ERR_CODE]: message and overriding the name seems to be the best way to accomplish this without tampering with the prototype. EDIT: wait but if we override toString() in custom classes we could accomplish the same thing?

I am not sure about overriding Error.prototype.toString() though since it is quite intrusive for users. I believe we can make it work by just overriding the toString() methods in our classes extending Error?

cc @jasnell

[joyeecheung]

Also this may create quite a churn if we change the name because there are now a lot of code using assert.throws to match the names formatted this way. Would be tricky if it's semver-major. To be honest I prefer to use common.expectsError instead of assert.throws to test these errors because the name formatting looked odd to me and it seemed odder to write tests depending on this.

[felixfbecker]

As a user I wouldn't see it as "overriding" as the whole Error class is something that the NodeJS runtime provides me with, and there are no guarantees about what toString() returns except that it returns some human-readable representation of the Error. It would result in the same output if the error has no code property.

But yeah, Node could also choose to only do it for internal errors. I just think users will quickly think "wait, this is very helpful, why can't I have this for all my errors".

[joyeecheung]

@felixfbecker Well I think it's useful but I would not say all users would agree. It would be better if this is opt-in, since I believe there will be quite a few users depending on the current behavior of Error.prototype.string, especially people using them to analyze logs. The benefit of a new format does not seem to worth the breakage.

[felixfbecker]

Wouldn't that argument apply just as well to built-in errors (analyzing errors in logs)?

[joyeecheung]

@felixfbecker I would say not since our errors are not instances of native errors. They are extended.

[joyeecheung]

As a user I wouldn't see it as "overriding" as the whole Error class is something that the NodeJS runtime provides me with, and there are no guarantees about what toString() returns except that it returns some human-readable representation of the Error.

The current behavior is actually spec'd, see https://tc39.github.io/ecma262/#sec-error.prototype.tostring

19.5.3.4Error.prototype.toString ( )

The following steps are taken:

Let O be the this value.
If Type(O) is not Object, throw a TypeError exception.
Let name be ? Get(O, "name").
If name is undefined, let name be "Error"; otherwise let name be ? ToString(name).
Let msg be ? Get(O, "message").
If msg is undefined, let msg be the empty String; otherwise let msg be ? ToString(msg).
If name is the empty String, return msg.
If msg is the empty String, return name.
Return the string-concatenation of name, the code unit 0x003A (COLON), the code unit 0x0020 (SPACE), and msg.

[felixfbecker]

The current behavior is actually spec'd

Got it, wasn't aware of that. Frankly in that case I would prefer having no code printed in toString() at all over modifying name.

The code will still show up if logged with console.log()/console.error()/util.inspect() as it has always been:

> console.error(Object.assign(new Error('test'), { code: 'ERR_THIS_IS_A_TEST' }))
{ Error: test
    at repl:1:29
    at Script.runInThisContext (vm.js:65:33)
    at REPLServer.defaultEval (repl.js:246:29)
    at bound (domain.js:375:14)
    at REPLServer.runBound [as eval] (domain.js:388:12)
    at REPLServer.onLine (repl.js:497:10)
    at REPLServer.emit (events.js:132:15)
    at REPLServer.emit (domain.js:421:20)
    at REPLServer.Interface._onLine (readline.js:285:10)
    at REPLServer.Interface._line (readline.js:638:8) code: 'ERR_THIS_IS_A_TEST' }
undefined
> try { require('child_process').execSync('asdkljasd', { encoding: 'utf-8' }) } catch (err) { console.error(err) }
{ Error: Command failed: asdkljasd
/bin/sh: asdkljasd: command not found

    at checkExecSyncError (child_process.js:574:11)
    at Object.execSync (child_process.js:611:13)
    at repl:1:32
    at Script.runInThisContext (vm.js:65:33)
    at REPLServer.defaultEval (repl.js:246:29)
    at bound (domain.js:375:14)
    at REPLServer.runBound [as eval] (domain.js:388:12)
    at REPLServer.onLine (repl.js:497:10)
    at REPLServer.emit (events.js:132:15)
    at REPLServer.emit (domain.js:421:20)
  status: 127,
  signal: null,
  output: [ null, '', '/bin/sh: asdkljasd: command not found\n' ],
  pid: 56111,
  stdout: '',
  stderr: '/bin/sh: asdkljasd: command not found\n' }

Maybe it would be an improvement to make the default uncaught exception handler use console.error(err) instead of only printing error.toString()?

[joyeecheung]

@felixfbecker It's not so much that the repl prints error.toString(), it actually prints error.stack. The repl filters out all the internal frames so for an error created with only code in the repl, the only thing left will be the first line of the stack, which is the same as error.toString() in the case of V8.

I think it would be useful to log other properties out in the repl, since I often find myself catching the error and then console.log() it when I am in repl. Not sure if there will be collaborators against it though. You may want to open a PR to find out.

[felixfbecker]

I wasn't specifically talking about the REPL, just used it as a demonstration. Here is the same with node -e:

$ node -e "throw Object.assign(new Error('test'), { code: 'ERR_TEST' })"                                                   
[eval]:1
throw Object.assign(new Error('test'), { code: 'ERR_TEST' })
^

Error: test
    at [eval]:1:21
    at Script.runInThisContext (vm.js:65:33)
    at Object.runInThisContext (vm.js:199:38)
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (module.js:662:30)
    at evalScript (bootstrap_node.js:522:27)
    at startup (bootstrap_node.js:169:9)
    at bootstrap_node.js:665:3
✘-1 ~
$ node -e "console.error(Object.assign(new Error('test'), { code: 'ERR_TEST' }))"
{ Error: test
    at [eval]:1:29
    at Script.runInThisContext (vm.js:65:33)
    at Object.runInThisContext (vm.js:199:38)
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (module.js:662:30)
    at evalScript (bootstrap_node.js:522:27)
    at startup (bootstrap_node.js:169:9)
    at bootstrap_node.js:665:3 code: 'ERR_TEST' }

I am proposing this as an alternative solution to the problem "we want to show users the error codes of the node core errors when they are printed", because modifying name for this is not a good solution imo for the reasons outlined above.

This solution has the added benefit of also exposing other helpful metadata that errors have plenty of as shown in my previous example (e.g. child_process errors have stderr, exit code, signal...). And the amount of information that is printed can already be easily configured.

[joyeecheung]

I personally like the idea of logging out other properties of an error in those handlers, but I would not think of this as an alternative to put the code in the results of error.toString() (by modifying name or not) since if the code is not in error.toString() it does not stand out as much.

[felixfbecker]

I don't have a problem with Node using an internal errors base class that overrides toString() to print the error code too. The internal error class can even take the code as a required parameter to enforce all internal errors have error codes.

[joyeecheung]

@felixfbecker That is enforced by the JS linter actually. (there are still 100+ C++ errors without .code though)

[BridgeAR]

I just had a look at this and played around how this could be fixed and it turns out to be quite a difficult task at hand.

We use util.inspect for everything that we log and when inspecting errors, we return the error stack (plus enumerable properties on the error).

Manipulating the stack is not an option because the stack itself is also manipulated throughout the code after creating the error with e.g., Error.captureStackTrace. So manipulating error.toString() does not work (also for another reason: even if adding a specific toString() to each error it does not mean that this function would ever be called as we could likely use Error.prototype.toString.call(error) instead).

I guess the best way to solve this is to just adding the properties that we want to show directly on the error as enumerable properties. That way these properties would always be visible when inspecting them (no matter if in the repl or elsewhere).

[BridgeAR]

The repl would probably need some extra love but that should be fine.

Here an example how it would look like when changing the name and printing code as well:

{ TypeError: The "fd" argument must be of type number. Received type undefined
    at Object.fs.write (fs.js:630:3)
    at repl:1:17
    at Script.runInThisContext (vm.js:89:20) code: 'ERR_INVALID_ARG_TYPE' }

[felixfbecker]

So manipulating error.toString() does not work (also for another reason: even if add a specific toString()

I don't understand why - wouldn't it result in the same output that is output right now in v10 with the modified name?

In any case, I think going through util.inspect() is better because it can print name and code in an easy to understand way (that these are properties I can introspect).
Maybe there could be some work on util.inspect() to not jam code on the last line of the stack trace to make it more visible.

[BridgeAR]

@felixfbecker toString() is never called when inspecting or logging (that uses inspect internally). That is the reason why it would not work. And if you add a individual toString() function and use the prototype toString function, it would still not be called. As in:

const err = new Error('foo')
err.toString = () => console.log('called')
Error.prototype.toString.call(err)
> 'Error: foo'
err == 123
> called
> false

[BridgeAR]

Maybe there could be some work on util.inspect() to not jam code on the last line of the stack trace to make it more visible.

👍 I am going to look into it.

[BridgeAR]

I might have found a solution. Trying it out right now.

[BridgeAR]

I tried it out and my approach works relatively well in most cases but as I feared above not in all cases. Therefore I still see no other way.

[jasnell]

what's the current status on this? Will this be a semver major or no?

[BridgeAR]

@jasnell there was no PR that changed the behavior.

And I am not sure how we can address this properly. The only working way for me was to set the name in the constructor, access the stack trace (so it generates the one with the code) and reset the name to the proper one without code. This works relatively well. However, in case the application exits due to the error the stack is generated again (seems like C++ code does that). Therefore the error code is not visible in those cases anymore. If someone knows how to prevent the stack trace from being generated again, we could try this workaround.

[joyeecheung]

Why do we have to keep the error code in err.name in the first place? Can't we just put it in err.message (after the colon)? I doubt if anybody is relying on any of these two being exactly the same as before since the whole point of these errors is to rely on err.code instead, so I don't think we would break anybody if we just do a semver-major bump and change err.name as well as err.message?

[joyeecheung]

Also, formatting err.name with Name [${err.code}] seems to break Web compatibility in our Web API implementations (at least it breaks WPT and has to be worked around with #22556).

This was brought up in #11299 @jasnell

The only significant difference is the addition of the code property which would just need to be documented as a Node.js specific extension. instanceof Error / instanceof TypeError still work for these as expected.

But WPT does not use instanceof because exceptions on the Web can come from different realms (so do Node.js exceptions thrown from different vms), and in addition to err.code, err.name is currently another significant difference from the simple exceptions.

(So..maybe #11299 should be reopened?)

[BridgeAR]

I would also prefer to have a "clean" name property instead of adding the code to it but it's not a strong opinion.

[joyeecheung]

If we want to move the code from err.name to err.message, should we do this before v11, or should we wait and do this in the v11 cycle (pre-v12)? (I prefer to do this before v11 is cut, assuming we manage to revamp our tests in time, but I'll defer to @jasnell )

(Also, we can use help from the upcoming code-and-learn to fix the Error [CODE] in tests, currently there are 200+ instances in 68 files)

[Trott]

(Also, we can use help from the upcoming code-and-learn to fix the Error [CODE] in tests, currently there are 200+ instances in 68 files)

Do we know what we'd want it replaced with? Checking err.code for sure, but also.... instanceof check on the original Error type (RangeError, TypeError, etc.)? Or something else?

[joyeecheung]

@Trott common.expectsError({type: TypeError, code: 'ERR_CODE'}) as what we used to do. We can change common.expectsError to check err.name matches type.constructor.name instead of using instanceof later or now (that's what the WPT harness does in essence to work around errors in different realms)

[Trott]

@Trott common.expectsError({type: TypeError, code: 'ERR_CODE'}) as what we used to do.

I can live with that. Ideally, I would prefer to avoid the magic/special type property in common.expectsError() because it won't be available when we move (back) to assert.throws() (which is what I believe @BridgeAR and I have both been imagining we'd do after 8.x is end-of-life).

EDIT: Although I'm sure we can figure out some way to tell assert.throws() to check it. assert.throws(block, object, options)? This is getting off-topic for this issue. I'll have this conversation elsewhere... :-D

[jasnell]

I'm strongly -1 on moving the code to err.message. There is far more code out in the wild that parses through the error message than pays attention to the error name and moving the code to the message carries a very high risk of breaking code, which would force us to treat changing error messages as semver-major again, which takes us in the wrong direction. Including the code in the name was not a snap decision, it was well thought out as being the least disruptive approach.

What I do think is a viable approach long term is to make a proposal to TC-39 to add code as a standard property along with a specification that ensures it's value is always serialized with the error message and stack.

[jasnell]

I'm going to take this issue off the 11.0.0 milestone as it would require (a) having a PR, (b) having consensus on the approach, and (c) having TSC sign off to get pulled in to 11 at this point.

[joyeecheung]

There is far more code out in the wild that parses through the error message than pays attention to the error name and moving the code to the message carries a very high risk of breaking code, which would force us to treat changing error messages as semver-major again

If we still cannot move err.code out of err.name because people parse err.message (in the stacktrace or just as err.message), what difference does it make to have a err.code or not? Aren't we still stuck at the semver-major situation if we cannot move err.code out of err.name simply because that will change err.message/the stacktrace?

It's hard to imagine that someone would rely on Error [ERR_CODE] being exactly the same format knowing that ERR_CODE is already available as err.code and is much more reliable (at least the format of error.name is not documented while error.code is in https://nodejs.org/api/errors.html). How would the code relying on this behavior look like?

const match = err.name.match(/(\w+) \[(.+)\]/);
//or
const match = err.stack.match(/(\w+) \[(.+)\]:/);
const [ _, name, code] = match;

instead of

const {name, code} = err;

?

[joyeecheung]

@jasnell IIUC, you might be talking about places like

const match = err.message.match(someRe);
// expects there is no `[ERR_CODE]` suddenly showing up at the start

? But I believe for errors like those we don't even touch the err.name anyway? (e.g. uvException, errnoException, .etc) They are just the most simple Error with "Error" as name right now, so the change does not effect them anyway. The errors that do have their names formatted as Error [ERR_CODE] (i.e. the errors formatted with the E() function in internal/errors.js ) are currently all free to change their messages without going semver-major, so adding [ERR_CODE] to their error message doesn't change our policy.