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.

Sign up for GitHub

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

Jump to bottom

src: add error code helpers to env #19465

Closed
wants to merge 1 commit into from
Closed

src: add error code helpers to env #19465

wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Mar 20, 2018 •
edited
Loading

This commit adds env->THROW_ERR_* and env->ERR_* helpers in
the C++ land to quickly assign error codes to
existing C++ errors. It also converts several errors in the
C++ land that can be assigned either obvious existing error codes,
or error codes close to existing ones. The plan is to first
assign more error codes before v10, then either migrate the errors
into JS land or improve the error messages during v10 without
having to go semver-major since they already have error codes
assigned.

The following new error codes are added:

  • ERR_INVALID_CONSTRUCTOR_CALL
  • ERR_SCRIPT_EXECUTION_TIMEOUT (close to
    ERR_SCRIPT_EXECUTION_INTERRUPTED)
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 20, 2018
@joyeecheung joyeecheung added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 20, 2018
@joyeecheung
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/13752/

cc @targos @jasnell

@joyeecheung
Copy link
Member Author

I have left out some argument checking errors in node_crypto.cc and node.cc since I believe when they are converted into JS land they will have different errors thrown anyway - for example, a lot of ERR_MISSING_ARGS are usually just replaced by individual ERR_INVALID_ARG_TYPE, and we usually just ignore the extra arguments instead of throwing an error.

devsnek
devsnek approved these changes Mar 20, 2018
View reviewed changes
Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

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

changes look good overall. i'm a bit curious how many of these can be removed (e.g. the constructor check in ModuleWrap::Resolve seems unneeded) or moved to js

vsemozhetbyt
vsemozhetbyt reviewed Mar 20, 2018
View reviewed changes
doc/api/errors.md
<a id="ERR_INVALID_CONSTRUCTOR_CALL"></a>
### ERR_INVALID_CONSTRUCTOR_CALL

A function that should not be invoked as a construcor was invoked with `new`
Copy link
Contributor

Choose a reason for hiding this comment

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

construcor -> constructor

jasnell
jasnell reviewed Mar 20, 2018
View reviewed changes
src/inspector_js_api.cc Outdated
@@ -96,7 +97,7 @@ class JSBindingsConnection : public AsyncWrap {
static void New(const FunctionCallbackInfo<Value>& info) {
Environment* env = Environment::GetCurrent(info);
if (!info[0]->IsFunction()) {
env->ThrowTypeError("Message callback is required");
env->THROW_ERR_INVALID_CALLBACK("Message callback is required");
Copy link
Member

Choose a reason for hiding this comment

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

These should really use standard messages that could be specified in the macro definitions... e.g. this should be env->THROW_ERR_INVALID_CALLBACK() and use the same error message text as the javascript equivalent.

Copy link
Member Author

@joyeecheung joyeecheung Mar 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@jasnell Yes, but this PR is more about assigning the codes in case we don't have the time to migrate them all before v10. Most of the errors touched here can be thrown in JS land anyway so I've left most of the messages as-is since if we are going to change them, might as well just do it in JS. But if we don't manage to do a full migration to them all before v10 and don't even assign these codes in C++, then we'll have yet another bunch of semver-major error changes after v10. With this at least we should be free to improve the messages during v10.

Copy link
Member Author

@joyeecheung joyeecheung Mar 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@jasnell I can leave these more obvious errors untouched if you think we can get them migrated properly before v10, the ones that I don't really think we'll have time to finish migrating to JS before v10 are the ones thrown in macros, like THROW_AND_RETURN_IF_OOB

Copy link
Member

Choose a reason for hiding this comment

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

Ok, that makes sense. I just wouldn't want it to fall off the radar... that is, I don't want folks to think that just because the code is assigned, there's nothing left to do on these.

@joyeecheung joyeecheung added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Mar 20, 2018
@joyeecheung
src: add error code helpers to env
03a190f 
This commit adds env->THROW_ERR_* and env->ERR_* helpers in
the C++ land to quickly assign error codes to
existing C++ errors. It also converts several errors in the
C++ land that can be assigned either obvious existing error codes,
or error codes close to existing ones. The plan is to first
assign more error codes before v10, then either migrate the errors
into JS land or improve the error messages during v10 without
having to go semver-major since they already have error codes
assigned.

The following new error codes are added:

- ERR_INVALID_CONSTRUCTOR_CALL
- ERR_SCRIPT_EXECUTION_TIMEOUT (related to
  ERR_SCRIPT_EXECUTION_INTERRUPTED)
@joyeecheung joyeecheung mentioned this pull request Apr 2, 2018
Closed
4 tasks
@joyeecheung joyeecheung added the blocked PRs that are blocked by other issues or PRs. label Apr 4, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Apr 9, 2018

@joyeecheung what is this blocked by at the moment? I think this is a good idea to get the codes in soon.

It needs a rebase at the moment.

@BridgeAR
Copy link
Member

Ping @joyeecheung

@joyeecheung
Copy link
Member Author

@BridgeAR Part of this PR is replaced by the first commit of #19739 I will either try to rebase and use the helpers introduced there, or just reopen a new one..

@joyeecheung joyeecheung mentioned this pull request Apr 18, 2018
Closed
4 tasks
@joyeecheung
Copy link
Member Author

Closed now that #20121 landed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@BridgeAR BridgeAR BridgeAR approved these changes

@devsnek devsnek devsnek approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
blocked PRs that are blocked by other issues or PRs. c++ Issues and PRs that require attention from people who are familiar with C++. errors Issues and PRs related to JavaScript errors originated in Node.js core. lib / src Issues and PRs related to general changes in the lib or src directory. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@joyeecheung @BridgeAR @jasnell @devsnek @vsemozhetbyt @nodejs-github-bot