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

joyeecheung · 2018-04-18T08:15:50Z

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.

joyeecheung · 2018-04-18T08:17:24Z

src/node_i18n.cc

@@ -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]));


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

joyeecheung · 2018-04-18T08:26:01Z

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

joyeecheung · 2018-04-18T08:27:17Z

Master CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1373/
PR CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1374/

joyeecheung · 2018-04-18T08:28:19Z

cc @jasnell @BridgeAR @devsnek @nodejs/tsc

danbev · 2018-04-18T10:10:56Z

src/node_crypto.cc

@@ -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(


Perhaps remove the node namespace here?

danbev · 2018-04-18T10:11:23Z

src/node_crypto.cc

@@ -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(


Perhaps remove the node namespace here?

danbev · 2018-04-18T10:11:30Z

src/node_crypto.cc

@@ -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);


Perhaps remove the node namespace here?

joyeecheung · 2018-04-18T13:30:31Z

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

mcollina

LGTM

addaleax · 2018-04-18T14:36:19Z

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 · 2018-04-18T14:42:11Z

@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 · 2018-04-18T14:50:17Z

CITGM, master: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1375/
CITGM, this PR: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1376/

jasnell · 2018-04-18T14:56:20Z

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 · 2018-04-18T15:20:31Z

CITGM failures in #20121 (comment) look irrelavent.

On master but not in PR:

citgm.yargs-v11.0.0, test timed out looks irrelevant

On this PR but not on master:

citgm.multer-v1.3.0, tmp file does not exist, showed up in 1366 before
citgm.q-v1.5.1 EPERM when installing the module, showed up in 1361 before
citgm.spawn-wrap-v1.4.2 showed up in 1372 before

joyeecheung · 2018-04-18T15:44:00Z

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 · 2018-04-18T15:46:46Z

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/

jasnell · 2018-04-18T17:47:30Z

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 · 2018-04-18T18:32:19Z

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

jasnell · 2018-04-18T18:36:11Z

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

joyeecheung · 2018-04-19T03:30:08Z

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

joyeecheung · 2018-04-19T04:32:24Z

Landed in 188ed07...fb58cae, thanks!

PR-URL: #20121 Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

- 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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

PR-URL: #20121 Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

- 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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

PR-URL: #20121 Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

joyeecheung added 7 commits April 18, 2018 15:40

src: add THROW_ERR_* helpers

034ddaf

src: migrate ERR_INDEX_OUT_OF_RANGE in C++

495ba98

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_BUFFER_OUT_OF_BOUNDS in node_buffer.cc

185c60d

src: throw ERR_MISSING_MODULE in module_wrap.cc

b2e7ff3

src: throw ERR_INVALID_ARG_VALUE in node_crypto.cc

46935ab

src: throw ERR_MISSING_ARGS in node_crypto.cc

0b4364e

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

joyeecheung commented Apr 18, 2018

View reviewed changes

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

danbev approved these changes Apr 18, 2018

View reviewed changes

addaleax approved these changes Apr 18, 2018

View reviewed changes

mcollina approved these changes Apr 18, 2018

View reviewed changes

jasnell approved these changes Apr 18, 2018

View reviewed changes

Fishrock123 approved these changes Apr 18, 2018

View reviewed changes

joyeecheung added 3 commits April 18, 2018 23:28

fixup! src: throw ERR_MISSING_ARGS in node_crypto.cc

184e8ea

fixup! src: throw ERR_INVALID_ARG_TYPE in C++ argument checks

834764e

fixup! src: throw ERR_INVALID_ARG_VALUE in node_crypto.cc

60a0c36

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

addaleax added this to the 10.0.0 milestone Apr 18, 2018

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 closed this Apr 19, 2018

This was referenced Apr 19, 2018

git-node: suggest -x and --autosquash for git rebase nodejs/node-core-utils#237

Merged

src: add error code helpers to env #19465

Closed

addaleax mentioned this pull request Apr 21, 2018

deps: upgrade npm to 6.1.0 #20190

Closed

2 tasks

yanivfriedensohn mentioned this pull request May 14, 2019

src: add error codes to errors thrown in C++ #27700

Closed

3 tasks

yanivfriedensohn mentioned this pull request Jun 14, 2019

src: add error codes to errors thrown in node_i18n.cc #28221

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

joyeecheung commented Apr 18, 2018 •

edited

Loading

joyeecheung Apr 18, 2018

joyeecheung commented Apr 18, 2018

joyeecheung commented Apr 18, 2018

joyeecheung commented Apr 18, 2018

danbev Apr 18, 2018

danbev Apr 18, 2018

danbev Apr 18, 2018

joyeecheung commented Apr 18, 2018

mcollina left a comment

addaleax commented Apr 18, 2018

joyeecheung commented Apr 18, 2018

addaleax commented Apr 18, 2018

jasnell commented Apr 18, 2018

joyeecheung commented Apr 18, 2018

joyeecheung commented Apr 18, 2018 •

edited

Loading

joyeecheung commented Apr 18, 2018

jasnell commented Apr 18, 2018

MylesBorins commented Apr 18, 2018

jasnell commented Apr 18, 2018

joyeecheung commented Apr 19, 2018

joyeecheung commented Apr 19, 2018

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

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

Conversation

joyeecheung commented Apr 18, 2018 • edited Loading

src: add THROW_ERR_* helpers

src: migrate ERR_INDEX_OUT_OF_RANGE in C++

src: throw ERR_INVALID_ARG_TYPE in C++ argument checks

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

joyeecheung Apr 18, 2018

Choose a reason for hiding this comment

joyeecheung commented Apr 18, 2018

joyeecheung commented Apr 18, 2018

joyeecheung commented Apr 18, 2018

danbev Apr 18, 2018

Choose a reason for hiding this comment

danbev Apr 18, 2018

Choose a reason for hiding this comment

danbev Apr 18, 2018

Choose a reason for hiding this comment

joyeecheung commented Apr 18, 2018

mcollina left a comment

Choose a reason for hiding this comment

addaleax commented Apr 18, 2018

joyeecheung commented Apr 18, 2018

addaleax commented Apr 18, 2018

jasnell commented Apr 18, 2018

joyeecheung commented Apr 18, 2018

joyeecheung commented Apr 18, 2018 • edited Loading

joyeecheung commented Apr 18, 2018

jasnell commented Apr 18, 2018

MylesBorins commented Apr 18, 2018

jasnell commented Apr 18, 2018

joyeecheung commented Apr 19, 2018

joyeecheung commented Apr 19, 2018

joyeecheung commented Apr 18, 2018 •

edited

Loading

joyeecheung commented Apr 18, 2018 •

edited

Loading