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

util: add support for error subclassing and null prototype #23852

Closed
wants to merge 2 commits into from

Conversation

antsmartian
Copy link
Contributor

@antsmartian antsmartian commented Oct 24, 2018

Handle error subclassing and null prototypes on error.

/cc @BridgeAR

  • 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

@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Oct 24, 2018
@antsmartian antsmartian changed the title util: add support for error subclassing and null handling util: add support for error subclassing and null prototype Oct 24, 2018
lib/internal/util/inspect.js Outdated Show resolved Hide resolved
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

This is a good start but I am a bit afraid about manipulating manually created error stacks.

Would you be so kind and add a test for a case similar to this:

class CustomError extends TypeError {
  constructor(message) {
    super(message);
    const stack = this.stack.split('\n');
    stack.shift();
    this.stack = `Awesome stack: message\n${stack.join('\n')}`;
  }
}
const err = new CustomError('foobar');
...

It is very hard to tell what the user intention was or was not. Therefore I suggest to check for if the original start is the name of one of the classes this error inherited from. That way we are safe to replace things we are certain about.

Something similar about the null prototype: I would use a regular expression to add the null prototype information instead of using indexOf(). The user could definitely have used some special information. I would probably go for /^[a-z_-]+: /i but maybe we want to be more conservative and go for /^[a-z_-]*Error: /i.

lib/internal/util/inspect.js Outdated Show resolved Hide resolved
lib/internal/util/inspect.js Outdated Show resolved Hide resolved
lib/internal/util/inspect.js Outdated Show resolved Hide resolved
@BridgeAR BridgeAR added the wip Issues and PRs that are still a work in progress. label Oct 25, 2018
@antsmartian
Copy link
Contributor Author

antsmartian commented Oct 27, 2018

@BridgeAR : Quick question, for the given example:

class CustomError extends TypeError {
  constructor(message) {
    super(message);
    const stack = this.stack.split('\n');
    stack.shift();
    this.stack = `Awesome stack: message\n${stack.join('\n')}`;
  }
}
const err = new CustomError('foobar');

If we do our check with parent error type, then we would keep the stack as it is like:

Awesome stack: message
    at repl:1:13

but I think, we need to print:

CustomError: foobar
   at repl:1:13

What do you think? Or is it ok, for the user to show them what they need?

@antsmartian
Copy link
Contributor Author

@BridgeAR Ping :)

@antsmartian
Copy link
Contributor Author

@BridgeAR Friendly remainder 🔉

@antsmartian
Copy link
Contributor Author

@BridgeAR friendly ping again...

@antsmartian
Copy link
Contributor Author

antsmartian commented Dec 12, 2018

@BridgeAR Sorry, but one more very gentle ping.. :)

Object.setPrototypeOf(customErr, null);
assert.ok(util.inspect(customErr)
.includes('[Error: null prototype]'));
}
Copy link
Member

Choose a reason for hiding this comment

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

This case is a duplicate of the test above.

@BridgeAR
Copy link
Member

BridgeAR commented Dec 12, 2018

I have to give this one a bit more thought. Sadly, there are lots and lots of edge cases that could happen with errors.

@BridgeAR
Copy link
Member

BridgeAR commented Apr 1, 2019

Superseded by #26923. It could still be expanded but I would not want to do that before Node.js v13.

@BridgeAR BridgeAR closed this Apr 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
util Issues and PRs related to the built-in util module. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants