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

[WIP] errors: add internal/errors module #6573

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented May 4, 2016

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

errors (new subsystem)

Description of change

Note: This is a _work in progress. I am opening the PR to begin the discussion about the overall design. I will be iterating based on the feedback. This is far from being ready to land in it's current state._

This is step one to making it so that changes to error messages are not forced to be semver-major changes any more. The internal errors.js module exports three custom error class instances that use an assigned static error code to look up the error message. The message itself can change but the error code would remain the same. Changes to error code would still be considered semver-major, but changes to the messages themselves would not be.

This also updates the Error objects created in lib/internal to use the new mechanism, changing and streamlining several of the error messages in the process. Obviously, this is a semver-major change on it's own.

Several test cases were also modified and a new common.throws method added to test/common.js that understands the new code mechanism. This is added common.js so that the existing assert module api is unchanged.

Update: This PR is purposefully being held back from landing until we are closer to actually releasing v7. Until then, it will need to be periodically updated to pick up any other changes in error handling that land. PRs are welcome. Please label any other PRs that affect error handling with the error-handling label so that those can be easily tracked.

@jasnell jasnell added wip Issues and PRs that are still a work in progress. semver-major PRs that contain breaking changes and should be released in the next major version. discuss Issues opened for discussions and feedbacks. error-handling labels May 4, 2016
assert(msg);
return msg;
},
ENOTIMPLEMENTED: notImplemented,
Copy link
Member

Choose a reason for hiding this comment

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

We should be safe to remove the E prefix on the error codes... anything at Error[Symbol.for('code')] will be an error.

Thoughts on exporting the error codes?

... with all of the ENO codes its hard for me not to think of Brian Eno ... well done :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Error[Symbol.for('code')]

Yeah, considered that possibility also. Definitely makes sense to do it that way.

@Fishrock123
Copy link
Contributor

Somewhat related to #4311

@jasnell jasnell force-pushed the no-major-errors branch 3 times, most recently from 2efc255 to 76bc6e6 Compare May 6, 2016 19:27
@jasnell
Copy link
Member Author

jasnell commented May 9, 2016

@nodejs/ctc ... this now covers all Error/TypeError/RangeError throws we generate from lib/*.js . It does not yet include the errors thrown from src/*. It's a sizable change. To make it easier to review, I'd start here and here. The former being the new lib/internal/errors.js file and the latter being an example of the impact of the change.

Essentially this PR currently consolidates, and makes consistent, all error messages thrown in lib/*.js and associates a stable code with each.

Please review.


class NodeError extends Error {
constructor(key, ...args) {
super(message(key, args));
Copy link
Contributor

@Fishrock123 Fishrock123 May 9, 2016

Choose a reason for hiding this comment

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

does this need to be ...args? whoops, this goes to message()

Copy link
Member Author

Choose a reason for hiding this comment

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

@Fishrock123
Copy link
Contributor

Should we have NodeSyntaxError for invalid arguments? (Since we kinda treat them as that?)


module.exports.Error = NodeError;
module.exports.TypeError = NodeTypeError;
module.exports.RangeError = NodeRangeError;
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for module?

@jasnell
Copy link
Member Author

jasnell commented May 9, 2016

We currently use TypeError for the most part for invalid arguments. There are a few cases where Error is used. If we wanted to switch to using SyntaxError then I would recommend doing that in a separate semver-major PR.

@Fishrock123
Copy link
Contributor

We currently use TypeError for the most part for invalid arguments.

oooh true. that is a better option.


// Utility function for registering the error codes.
function E(sym, val) {
Error[Symbol.for(sym)] = val;
Copy link
Contributor

Choose a reason for hiding this comment

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

Attaching to existing globals makes me a bit hesitant, but I understand it does kinda make sense 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.

I understand the hesitancy. Use of Symbol should make this less of a problem. TBH, however, we're not documenting that we're hanging these off Error so I'm fine changing this part.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to wait to hear others chime in.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we possibly prefix the symbol or some such? e.g. Symbol.for(node.${sym}); to prevent collisions.

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry. i'm missing why this needs to be hung off the global Error?

@Fishrock123
Copy link
Contributor

Overall seems good. Definitely think this is a good approach.

@jasnell
Copy link
Member Author

jasnell commented May 9, 2016

Updated.

@@ -126,7 +126,8 @@ Protocol.prototype.execute = function(d) {
break;

default:
throw new Error('Unknown state');
const errors = require('internal/errors');
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't requires be at the top of the file?

Copy link
Member Author

Choose a reason for hiding this comment

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

When there are only one or two cases in a file, I did it like this so the
require is only called in error conditions
On May 9, 2016 5:27 PM, "Rod Vagg" [email protected] wrote:

In lib/_debugger.js
#6573 (comment):

@@ -126,7 +126,8 @@ Protocol.prototype.execute = function(d) {
break;

 default:
  •  throw new Error('Unknown state');
    
  •  const errors = require('internal/errors');
    

shouldn't requires be at the top of the file?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/nodejs/node/pull/6573/files/56d34d36a841cee62adb657548b90bd27ebce870#r62595987

Copy link
Contributor

Choose a reason for hiding this comment

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

Lazy loading is pretty common in core code. Though I'm not sure how much it actually saves us now.

@jasnell
Copy link
Member Author

jasnell commented Aug 10, 2016

No, the C++ errors can be done separately, I just want to make sure it gets done. The important thing is that there needs to be agreement on syntax.

The only constraint that I can see is that the C++ error messages tend to be far more terse.

@trevnorris
Copy link
Contributor

@jasnell just curious, when we upgrade v8 within a major do we ever verify that none of the error messages changed?

@jasnell
Copy link
Member Author

jasnell commented Aug 11, 2016

Not that I'm aware of.

}

class NodeRangeError extends RangeError {
constructor(key, ... args) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: space in ... args.

Copy link
Contributor

Choose a reason for hiding this comment

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

does the linter not catch this?

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 haven't run the linter on this in a while so not sure. I'll be doing an update on this next week so I'll see for sure what lint issues come up.

Copy link
Member

Choose a reason for hiding this comment

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

The linter will not catch this because we do not have the rest-spread-spacing rule enabled. Sounds like we'd like to enforce no space between the spread operator and the argument, so I'll get a PR for that in Real Soon Now...

@trevnorris
Copy link
Contributor

Other than the fact that this would generate stacks one line shorter than Error.stackTraceLimit I'm not sure what the problem would be with doing something like:

function fixErrStack(stack) {
  const newstring = [];
  const idx0 = stack.search(/[\r\n]+/);
  newstring.push(stack.substr(0, idx0));
  const stack2 = stack.substr(idx0).trim();
  const idx1 = stack2.search(/[\r\n]+/);
  newstring.push(stack2.substr(idx1).trim());
  return newstring.join('\n');  // detect whether to insert \r\n?
}

const e = new Error('hi');
e.stack = fixErrStack(e.stack);

instead of creating two error objects.

@jasnell
Copy link
Member Author

jasnell commented Aug 11, 2016

@trevnorris ... sorry, not following

@trevnorris
Copy link
Contributor

@jasnell

Not that I'm aware of.

Logically I'd say that changes to error messages in v8 would also be a major if we treat them as major in node. Hasn't been a problem to date, but just throwing it out there.

... sorry, not following

How you are using Error.captureStackTrace() in an extended Error class (I believe to remove the top most function call). In order to prevent needing to create two stack traces instead of just one.

@jasnell
Copy link
Member Author

jasnell commented Aug 11, 2016

Re: treating v8 error changes as majors, I don't disagree, just not sure what all we can do about it.
Re: the captureStackTrace()... yeah, that part is irritating. I'll make a note to explore that further.

ChALkeR referenced this pull request in isaacs/node-graceful-fs Aug 16, 2016
node version 7 officially deprecates `fs.read` and `fs.readSync`

This is done using `internal/util`. The unfortunate side affect
being that `graceful-fs v3.x` explodes when running the code in
`vm` as `internal/util` is not accessible from userland.

This commit uses a regular expression to replaces the require of
the specific internal util function with the source of that util
function. As such `graceful-fs v3.x` will no longer explode in
node v7.

One advantage to this approach is that any future deprecation
will not break graceful-fs.
@Trott Trott removed the ctc-agenda label Aug 16, 2016
const ch = expected.codePointAt(0) | 0x20;
const prefix = (ch === 0x61 || ch === 0x65 ||
ch === 0x69 || ch === 0x6f ||
ch === 0x75) ? 'an' : 'a';
Copy link
Member

Choose a reason for hiding this comment

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

I think this returns false positives for e.g. Uint8Array – maybe the message should just read something like must be of type ${expected}?

@jasnell jasnell removed this from the 7.0.0 milestone Aug 26, 2016
@jasnell
Copy link
Member Author

jasnell commented Aug 26, 2016

Pulling this off the v7 milestone. It's not going to get done in time for that.

@jasnell jasnell removed the blocked PRs that are blocked by other issues or PRs. label Aug 29, 2016
@jasnell jasnell changed the title errors: add internal/errors module [WIP] errors: add internal/errors module Sep 27, 2016
@rvagg rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:01
@jasnell
Copy link
Member Author

jasnell commented Oct 24, 2016

Going to close this PR in favor of breaking these changes up into multiple separate PRs that can be done incrementally.

@jasnell jasnell closed this Oct 24, 2016
@jasnell jasnell mentioned this pull request Oct 24, 2016
3 tasks
// portable with the readable-streams module.
function typeError(code, args) {
const assert = require('assert');
var msg;

Choose a reason for hiding this comment

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

Why use var ? I think better to use let.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major PRs that contain breaking changes and should be released in the next major version. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.