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: make code property mutable #15694

Closed
wants to merge 2 commits into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Sep 30, 2017

Userland code can break if it depends on a mutable code property for
errors. Allow users to change the code property but do not propagate
changes to the error name.

Additionally, make message and name consistent with Error object
(non-enumerable). Test that console.log() and .toString() calls on
internal Error objects with mutated properties have analogous results
with the standard ECMAScript Error objects.

Fixes: #15658

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)

errors

@nodejs-github-bot nodejs-github-bot added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Sep 30, 2017
@Trott
Copy link
Member Author

Trott commented Sep 30, 2017

ping @jasnell @jdalton @evanlucas

@Trott
Copy link
Member Author

Trott commented Sep 30, 2017

Is this semver-major?

@Trott
Copy link
Member Author

Trott commented Sep 30, 2017

Also I couldn't locate user-facing documentation explaining that code was immutable, but maybe I didn't look in the right place. Did I miss it? @jasnell

@jdalton
Copy link
Member

jdalton commented Sep 30, 2017

Here's my take on it:

function createClass(Super) {
  return class NodeError extends Super {
    constructor(key, ...args) {
      super(getMessage(key, args))
      this[codeSym] = key
    }

    get code() {
      return this[codeSym]
    }

    set code(value) {
      setProperty(this, "code", { value })
    }

    get name() {
      return super.name + " [" + this[codeSym] + "]"
    }

    set name(value) {
      setProperty(this, "name", { value })
    }
  }
}

The setProperty helper codifies to:

Object.defineProperty(this, "code", {
  configurable: true,
  enumerable: true,
  value,
  writable: true
})

This way they're still getters until they are overwritten.

@Trott
Copy link
Member Author

Trott commented Sep 30, 2017

This way they're still getters until they are overwritten.

Naïve question perhaps, but what's the benefit of keeping it as a getter?

@jdalton
Copy link
Member

jdalton commented Sep 30, 2017

Naïve question perhaps, but what's the benefit of keeping it as a getter?

Keeps the existing dynamic behavior. I don't have the background on why the getter was used in the first place though.

@Trott
Copy link
Member Author

Trott commented Sep 30, 2017

@jdalton
Copy link
Member

jdalton commented Sep 30, 2017

Shouldn't name be made mutable too?

@Trott
Copy link
Member Author

Trott commented Sep 30, 2017

Shouldn't name be made mutable too?

I suppose so. And probably add a test to confirm that when name and message have been modified, the results of e.toString() and console.log(e) are analogous to modifying those properties on an Error object and calling e.toString() and console.log(e) in the browser (assuming the console.log() behavior is reasonably consistent across browsers). Specifically, on Chrome at least, I see that toString() uses the new over-ridden values, but console.log() uses the original values.

@Trott
Copy link
Member Author

Trott commented Sep 30, 2017

@targos
Copy link
Member

targos commented Sep 30, 2017

it looks like we can get rid of the symbol now

enumerable: false,
value: `${super.name} [${this[kCode]}]`,
writable: true
});
}
Copy link
Member

@jdalton jdalton Sep 30, 2017

Choose a reason for hiding this comment

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

If you moved the name assignment to the prototype it would align with builtin errors
(no-own name prop on error objects).

nix that
Object.defineProperty(NodeError.prototype, 'name', {
  configurable: true,
  enumerable: false,
  value: `${Base.prototype.name} [${this[kCode]}]`,
  writable: true
});

return NodeError

Ah, this is why the getter and symbols where used!

Copy link
Member

@jdalton jdalton Oct 1, 2017

Choose a reason for hiding this comment

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

Following up on my previous comment.
It looks like getters and a symbol was used to mimic built-in errors which have non-enumerable inherited properties (no-own properties).

So the tweaks needed to the existing errors would be adding setters for code and name to assign a value with enumerable, configurable, and writable of true. To avoid the current chattier console.log the kCode symbol assignment could be done with Object.defineProperty and made enumerable of false.

@Trott If you're up for it I could send a PR to your PR, since I'm kind of walking through the code as is and I've never had a commit merged to Node. 😋

Copy link
Member

Choose a reason for hiding this comment

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

It looks like getters and a symbol was used to mimic built-in errors which have non-enumerable inherited properties (no-own properties).

Yep. That's exactly why

Copy link
Member Author

Choose a reason for hiding this comment

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

@Trott If you're up for it I could send a PR to your PR, since I'm kind of walking through the code as is and I've never had a commit merged to Node.

@jdalton Sure, you can do that or if you'd rather just take it over, you can open your own PR and I can close this one. Either way is fine by me.

@refack
Copy link
Contributor

refack commented Oct 1, 2017

@jdalton are you suggesting to make both .name and .code overridable? and in that case it will be possible to completely hide the preassigned code.

makeNodeErr((e) => {
  e.name = 'g';
  e.code = 'f';
  e.message = 'h';
  e.toString() === 'g: h';
});

How about keeping e[kCode] in some way part of the toString() output?
Or add a .nodeCode() accessor?

@refack
Copy link
Contributor

refack commented Oct 1, 2017

it looks like we can get rid of the symbol now

As mentioned in #15694 (comment), pristine NodeErrors should have .name and .code as prototype getters so that instances of the class will only have .stack as an own property. In that case assigning the code to a symbol field still makes sense.

@jdalton
Copy link
Member

jdalton commented Oct 1, 2017

@refack

are you suggesting to make both .name and .code overridable? and in that case it will be possible to completely hide the preassigned code.

Yep. Error objects are created with certain info then thrown to user-land where folks can do what they want with them.

How about keeping e[kCode] in some way part of the toString() output?
Or add a .nodeCode() accessor?

I'm not a fan of obstructing user-land here. My take is that Node creates an error that walks like an error and talks like an error (no own-enumerable properties, standard toString behavior, etc.) and then throws it out. After that it's no longer Node's concern. Folks can catch it, augment it, decorate it, or shallow it. The tweaks to makeNodeError are minimal and the kCode symbol will still exist on the error object so folks can restore the original getters/behavior of a modified error if desired.

@Trott
Copy link
Member Author

Trott commented Oct 1, 2017

@Trott
Copy link
Member Author

Trott commented Oct 2, 2017

@jdalton I have to make one significant change in addition to a few unimportant linting changes.

The significant change is to define the name property with enumerable: false to be consistent with standard ES Error objects. The test fails otherwise (which is why CI is red red red). I'm assuming that I should do that, but just in case that's totally wrong in your view or anyone else's, I'm mentioning it here...

I'm leaving code as enumerable.

@Trott Trott force-pushed the code-writable branch 2 times, most recently from da94494 to aa33f44 Compare October 2, 2017 05:10
@Trott
Copy link
Member Author

Trott commented Oct 2, 2017

@jdalton
Copy link
Member

jdalton commented Oct 2, 2017

@Trott

The significant change is to define the name property with enumerable: false to be consistent with standard ES Error objects.

That's handled by the getter being defined in the class definition. It's non-enumerable by default.
When you set an own property however it will become enumerable:

var e = new Error("x")
console.log(e.name) // "Error"
console.log(Object.keys(e)) // []
e.name = "foo"
console.log(Object.keys(e)) // ["name"]

@targos
Copy link
Member

targos commented Oct 2, 2017

+1 to make the name property enumerable after the setter is used.

@jdalton
Copy link
Member

jdalton commented Oct 2, 2017

@Trott As mentioned in my previous comment, making the name property non-enumerable when assigned, myError.name = "foo", is the wrong behavior (doesn't match built-in). If you need to restore the original getter for unit tests you can simple delete the own property:

console.log(myError.name); // "foo"
delete myError.name
console.log(myError.name) // "Error [ERR_TLS_HANDSHAKE_TIMEOUT]"

That removes the own property and goes back to the prototypal property.
For reference I've done this in my own tests.

@refack
Copy link
Contributor

refack commented Oct 2, 2017

I'm not a fan of obstructing user-land here. My take is that Node creates an error that walks like an error and talks like an error (no own-enumerable properties, standard toString behavior, etc.) and then throws it

My only concern is 3rd party unintentional clobbering. I'd rather we make it a tiny bit hard for libraries to completely clobber the new behaviour. So can we think of a way to get to the code even if .code and .name are overridden?

set name(value) {
defineProperty(this, 'name', {
configurable: true,
enumerable: false,
Copy link
Member

Choose a reason for hiding this comment

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

Likely should be true after setting.

@Trott Trott added the console Issues and PRs related to the console subsystem. label Oct 4, 2017
@jdalton
Copy link
Member

jdalton commented Oct 4, 2017

It kinda looks like you're in a bad testing state somehow @Trott. Could the test be running with showHidden turned on by accident? That would explain why you're seeing braces for untouched errors which have no enumerable properties but have the kCode as a non-enumerable property.

@Trott
Copy link
Member Author

Trott commented Oct 4, 2017

I'm not seeing that with this PR.

@jdalton I'm not seeing it anymore either even after I roll back the last commit I just added. I must have gotten confused about what binary corresponded to what code changes and been running a binary against an earlier version of this PR. Sorry about the unproductive detour.

@Trott
Copy link
Member Author

Trott commented Oct 4, 2017

@jdalton I'll split out the console.log()/util.inspect() changes into a separate PR so this one can land without an extended debate as to whether such things are breaking changes or not.

For internal errors, make `code` and `name` settable while keeping them
non-own properties by default.

Fixes: nodejs#15658
@Trott
Copy link
Member Author

Trott commented Oct 4, 2017

@Trott
Copy link
Member Author

Trott commented Oct 4, 2017

CI looks good. (OS X failure is infra issues and unrelated.)

Trott added a commit to Trott/io.js that referenced this pull request Oct 4, 2017
Userland code can break if it depends on a mutable `code` property for
errors. Allow users to change the `code` property but do not propagate
changes to the error `name`.

Additionally, make `message` and `name` consistent with `Error` object
(non-enumerable). Test that `console.log()` and `.toString()` calls on
internal `Error` objects with mutated properties have analogous results
with the standard ECMAScript `Error` objects.

PR-URL: nodejs#15694
Fixes: nodejs#15658
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Trott pushed a commit to Trott/io.js that referenced this pull request Oct 4, 2017
For internal errors, make `code` and `name` settable while keeping them
non-own properties by default.

PR-URL: nodejs#15694
Fixes: nodejs#15658
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@Trott
Copy link
Member Author

Trott commented Oct 4, 2017

Landed in 6e172be and 42a2a9b . Thanks, @jdalton! 🎉

@Trott Trott closed this Oct 4, 2017
@MylesBorins
Copy link
Contributor

This is not landing cleanly on 8.x, could you please send a backport.

@Trott
Copy link
Member Author

Trott commented Oct 8, 2017

Added the dont-land labels for v6.x and v4.x because the new error system does not exist in those release lines. Will backport for v8.x.

@Trott
Copy link
Member Author

Trott commented Oct 8, 2017

@MylesBorins Backport in #16078

addaleax pushed a commit to addaleax/ayo that referenced this pull request Oct 12, 2017
Userland code can break if it depends on a mutable `code` property for
errors. Allow users to change the `code` property but do not propagate
changes to the error `name`.

Additionally, make `message` and `name` consistent with `Error` object
(non-enumerable). Test that `console.log()` and `.toString()` calls on
internal `Error` objects with mutated properties have analogous results
with the standard ECMAScript `Error` objects.

PR-URL: nodejs/node#15694
Fixes: nodejs/node#15658
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Oct 12, 2017
For internal errors, make `code` and `name` settable while keeping them
non-own properties by default.

PR-URL: nodejs/node#15694
Fixes: nodejs/node#15658
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Trott added a commit to Trott/io.js that referenced this pull request Oct 12, 2017
Userland code can break if it depends on a mutable `code` property for
errors. Allow users to change the `code` property but do not propagate
changes to the error `name`.

Additionally, make `message` and `name` consistent with `Error` object
(non-enumerable). Test that `console.log()` and `.toString()` calls on
internal `Error` objects with mutated properties have analogous results
with the standard ECMAScript `Error` objects.

PR-URL: nodejs#15694
Fixes: nodejs#15658
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Trott pushed a commit to Trott/io.js that referenced this pull request Oct 12, 2017
For internal errors, make `code` and `name` settable while keeping them
non-own properties by default.

PR-URL: nodejs#15694
Fixes: nodejs#15658
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
evanlucas pushed a commit that referenced this pull request Oct 23, 2017
Userland code can break if it depends on a mutable `code` property for
errors. Allow users to change the `code` property but do not propagate
changes to the error `name`.

Additionally, make `message` and `name` consistent with `Error` object
(non-enumerable). Test that `console.log()` and `.toString()` calls on
internal `Error` objects with mutated properties have analogous results
with the standard ECMAScript `Error` objects.

Backport-PR-URL: #16078
PR-URL: #15694
Fixes: #15658
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
evanlucas pushed a commit that referenced this pull request Oct 23, 2017
For internal errors, make `code` and `name` settable while keeping them
non-own properties by default.

Backport-PR-URL: #16078
PR-URL: #15694
Fixes: #15658
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@Trott Trott deleted the code-writable branch January 13, 2022 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
console Issues and PRs related to the console subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Readonly error.code has ecosystem impact.
10 participants