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

src: assign error codes to more errors thrown in C++ #20121

Closed
wants to merge 10 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Apr 18, 2018

This is a rework of #19465 , which tries to assign codes to more C++ errors first so we can change their messages later and do proper refactors without having to go semver-major.

Sorry that I don't have much time working on this recently so this might not be able to make it into v10, if that's the case I can try to make it semver-minor (only adding the code property without changing any error message or error types), or just do proper refactors and throw the errors in JS land instead - the ones thrown in macros would require significant work to be refactored properly though.

I have left out the ERR_SCRIPT_EXECUTION_* errors in #19465 because the new mechanism in #19739 use the Maybe version of Set and the returned Maybe is now empty when setting the code property there.

I have also left out some errors related to the constructor or the this because I do not think that's what the errors we will eventually end up with, so there is no point adding them here and replacing them with something else later (like using the V8 signatures to check the this). Some errors in node_crypto.cc are left out because I think they will likely disappear when migrated properly later.

There are no new error codes added this time around.

src: add THROW_ERR_* helpers

src: migrate ERR_INDEX_OUT_OF_RANGE in C++

This patch migrates the "Index out of range" errors in C++
to errors with the code ERR_INDEX_OUT_OF_RANGE which have
equivalents in JavaScript.

src: throw ERR_INVALID_ARG_TYPE in C++ argument checks

  • Moves THROW_AND_RETURN_IF_NOT_BUFFER and
    THROW_AND_RETURN_IF_NOT_STRING from node_crypto.cc to
    node_errors.h so it can be reused.
  • Move THROW_AND_RETURN_UNLESS_BUFFER in util.h to
    node_buffer.cc and call THROW_AND_RETURN_IF_NOT_BUFFER
    there. The only other reference to THROW_AND_RETURN_UNLESS_BUFFER in
    node_i18n.cc can be safely replaced by an assertion since
    the argument will be checked in JS land.
  • Migrate ERR_INVALID_ARG_TYPE errors in C++. We can move the
    checks to JS land if possible later without having to
    go semver-major.

src: throw ERR_BUFFER_OUT_OF_BOUNDS in node_buffer.cc

src: throw ERR_MISSING_MODULE in module_wrap.cc

src: throw ERR_INVALID_ARG_VALUE in node_crypto.cc

src: throw ERR_MISSING_ARGS in node_crypto.cc

Checklist
  • 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

This patch migrates the "Index out of range" errors in C++
to errors with the code `ERR_INDEX_OUT_OF_RANGE` which have
equivalents in JavaScript.
- Moves THROW_AND_RETURN_IF_NOT_BUFFER and
  THROW_AND_RETURN_IF_NOT_STRING from node_crypto.cc to
  node_errors.h so it can be reused.
- Move THROW_AND_RETURN_UNLESS_BUFFER in util.h to
  node_buffer.cc and call THROW_AND_RETURN_IF_NOT_BUFFER
  there. The only other reference to THROW_AND_RETURN_UNLESS_BUFFER in
  node_i18n.cc can be safely replaced by an assertion since
  the argument will be checked in JS land.
- Migrate ERR_INVALID_ARG_TYPE errors in C++. We can move the
  checks to JS land if possible later without having to
  go semver-major.
@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 Apr 18, 2018
@@ -447,7 +448,7 @@ void Transcode(const FunctionCallbackInfo<Value>&args) {
UErrorCode status = U_ZERO_ERROR;
MaybeLocal<Object> result;

THROW_AND_RETURN_UNLESS_BUFFER(env, args[0]);
CHECK(Buffer::HasInstance(args[0]));
Copy link
Member Author

Choose a reason for hiding this comment

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

This is replaced with a CHECK because the type is already checked in JS land

@joyeecheung
Copy link
Member Author

@joyeecheung
Copy link
Member Author

@joyeecheung joyeecheung added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 18, 2018
@joyeecheung
Copy link
Member Author

cc @jasnell @BridgeAR @devsnek @nodejs/tsc

@@ -989,7 +976,8 @@ void SecureContext::SetOptions(const FunctionCallbackInfo<Value>& args) {
ASSIGN_OR_RETURN_UNWRAP(&sc, args.Holder());

if (args.Length() != 1 || !args[0]->IntegerValue()) {
return sc->env()->ThrowTypeError("Options must be an integer value");
return node::THROW_ERR_INVALID_ARG_TYPE(
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps remove the node namespace here?

@@ -1043,8 +1031,8 @@ void SecureContext::SetSessionTimeout(const FunctionCallbackInfo<Value>& args) {
ASSIGN_OR_RETURN_UNWRAP(&sc, args.Holder());

if (args.Length() != 1 || !args[0]->IsInt32()) {
return sc->env()->ThrowTypeError(
"Session timeout must be a 32-bit integer");
return node::THROW_ERR_INVALID_ARG_TYPE(
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps remove the node namespace here?

@@ -4319,7 +4307,7 @@ void DiffieHellman::SetKey(const v8::FunctionCallbackInfo<v8::Value>& args,

if (!Buffer::HasInstance(args[0])) {
snprintf(errmsg, sizeof(errmsg), "%s must be a buffer", what);
return env->ThrowTypeError(errmsg);
return node::THROW_ERR_INVALID_ARG_TYPE(env, errmsg);
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps remove the node namespace here?

@joyeecheung
Copy link
Member Author

CI failed due to infra failures, trying again...: https://ci.nodejs.org/job/node-test-commit/17848/

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@addaleax
Copy link
Member

Just to have this written down:

Since this is only changing type checking error messages and it’s extremely unlikely that they are being checked at all, and this will help making backports during the long v10.x lifecycle easier, I’m good with including this in v10.x as suggested by Joyee in the TSC meeting. I don’t think there were any objections from somebody else, either.

@joyeecheung
Copy link
Member Author

@jasnell Can this go into v10 as is (with @danbev 's nits addressed) with some minor tweaks to error messages and types? They are just trivial errors (mostly type or range checking on arguments) so I do not think there will be users depending on those, IMO it makes sense to lift our current policy (any changes are semver-major) for this PR if it could not make it into v10.0.0. If not I can still try to backport this in a semver-minor manner, but it does not seem to be particularly worth the effort.

@addaleax
Copy link
Member

@jasnell
Copy link
Member

jasnell commented Apr 18, 2018

I'm planning a release candidate build for 10.0.0 for tomorrow. If this lands by then and there are no objections from @nodejs/tsc, then I can pull this in to 10.0.0 assuming CITGM looks good.

@joyeecheung
Copy link
Member Author

CITGM failures in #20121 (comment) look irrelavent.

On master but not in PR:

On this PR but not on master:

@joyeecheung joyeecheung added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 18, 2018
@joyeecheung
Copy link
Member Author

joyeecheung commented Apr 18, 2018

Adding fast-track because of #20121 (comment) and in the TSC meeting where we had quorum no one objected to this landing on v10.

@joyeecheung
Copy link
Member Author

Removed the node:: namespaces in node_crypto.cc as suggested by @danbev. The previous CI still failed on OSX although the infra failure appeared in the one for another PR as well.

New CI: https://ci.nodejs.org/job/node-test-commit/17852/

@addaleax addaleax added this to the 10.0.0 milestone Apr 18, 2018
@jasnell
Copy link
Member

jasnell commented Apr 18, 2018

One thing... this will need explicit @nodejs/tsc approval to fast track as a semver-major, otherwise there's no way it will land in time to make it in. After tomorrow I won't be pulling any semver-major's in at all unless there is explicit go ahead from the TSC (not just a no-objections, and explicit approval)

@MylesBorins
Copy link
Contributor

@jasnell there was no objections to fast track in today's tsc meeting, in which there was quorum. Is that sufficient?

@jasnell
Copy link
Member

jasnell commented Apr 18, 2018

Yep, so long as that's documented here that's perfect. :)

@joyeecheung
Copy link
Member Author

The last CI seems clean, the only error seems to be related to #19903

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 19, 2018
@joyeecheung
Copy link
Member Author

Landed in 188ed07...fb58cae, thanks!

joyeecheung added a commit that referenced this pull request Apr 19, 2018
PR-URL: #20121
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
joyeecheung added a commit that referenced this pull request Apr 19, 2018
This patch migrates the "Index out of range" errors in C++
to errors with the code `ERR_INDEX_OUT_OF_RANGE` which have
equivalents in JavaScript.

PR-URL: #20121
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
joyeecheung added a commit that referenced this pull request Apr 19, 2018
- Moves THROW_AND_RETURN_IF_NOT_BUFFER and
  THROW_AND_RETURN_IF_NOT_STRING from node_crypto.cc to
  node_errors.h so it can be reused.
- Move THROW_AND_RETURN_UNLESS_BUFFER in util.h to
  node_buffer.cc and call THROW_AND_RETURN_IF_NOT_BUFFER
  there. The only other reference to THROW_AND_RETURN_UNLESS_BUFFER in
  node_i18n.cc can be safely replaced by an assertion since
  the argument will be checked in JS land.
- Migrate ERR_INVALID_ARG_TYPE errors in C++. We can move the
  checks to JS land if possible later without having to
  go semver-major.

PR-URL: #20121
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
joyeecheung added a commit that referenced this pull request Apr 19, 2018
PR-URL: #20121
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
joyeecheung added a commit that referenced this pull request Apr 19, 2018
PR-URL: #20121
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
joyeecheung added a commit that referenced this pull request Apr 19, 2018
PR-URL: #20121
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
joyeecheung added a commit that referenced this pull request Apr 19, 2018
PR-URL: #20121
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
jasnell pushed a commit that referenced this pull request Apr 19, 2018
PR-URL: #20121
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
jasnell pushed a commit that referenced this pull request Apr 19, 2018
This patch migrates the "Index out of range" errors in C++
to errors with the code `ERR_INDEX_OUT_OF_RANGE` which have
equivalents in JavaScript.

PR-URL: #20121
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
jasnell pushed a commit that referenced this pull request Apr 19, 2018
- Moves THROW_AND_RETURN_IF_NOT_BUFFER and
  THROW_AND_RETURN_IF_NOT_STRING from node_crypto.cc to
  node_errors.h so it can be reused.
- Move THROW_AND_RETURN_UNLESS_BUFFER in util.h to
  node_buffer.cc and call THROW_AND_RETURN_IF_NOT_BUFFER
  there. The only other reference to THROW_AND_RETURN_UNLESS_BUFFER in
  node_i18n.cc can be safely replaced by an assertion since
  the argument will be checked in JS land.
- Migrate ERR_INVALID_ARG_TYPE errors in C++. We can move the
  checks to JS land if possible later without having to
  go semver-major.

PR-URL: #20121
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
jasnell pushed a commit that referenced this pull request Apr 19, 2018
PR-URL: #20121
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
jasnell pushed a commit that referenced this pull request Apr 19, 2018
PR-URL: #20121
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
jasnell pushed a commit that referenced this pull request Apr 19, 2018
PR-URL: #20121
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
jasnell pushed a commit that referenced this pull request Apr 19, 2018
PR-URL: #20121
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@addaleax addaleax mentioned this pull request Apr 21, 2018
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. fast-track PRs that do not need to wait for 48 hours to land. 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
Development

Successfully merging this pull request may close these issues.

8 participants