errors: validate input arguments #19924

BridgeAR · 2018-04-11T01:18:03Z

This makes sure the input arguments get validated so implementation
errors will be caught early. It also improves a couple of error
messages by providing more detailed information and fixes errors
detected by the new functionality. Besides that a error type got
simplified and tests got refactored.

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

BridgeAR · 2018-04-11T01:18:43Z

~~CI https://ci.nodejs.org/job/node-test-pull-request/14188/~~
CI https://ci.nodejs.org/job/node-test-pull-request/14190/

AyushG3112 · 2018-04-11T02:02:28Z

Does this remove the need for #19908? If yes, then it can be closed safely.

BridgeAR · 2018-04-11T02:06:26Z

@AyushG3112 I kept one of the two checks that are changed in #19908. Your PR is very likely going to land first, so you can just keep that open.

AyushG3112 · 2018-04-11T02:15:52Z

lib/internal/errors.js

@@ -960,7 +970,6 @@ E('ERR_ZLIB_INITIALIZATION_FAILED', 'Initialization failed', Error);

 function invalidArgType(name, expected, actual) {
  internalAssert(typeof name === 'string');
-  internalAssert(arguments.length === 3);


Shouldn't we keep this check? Without this, the error can be implemented without passing in actual too, so no matter what the user passes as the actual argument, the error message will say undefined

I just moved the check so it will be executed for all errors. That is actually what this PR is all about.

This makes sure the input arguments get validated so implementation errors will be caught early. It also improves a couple of error messages by providing more detailed information and fixes errors detected by the new functionality. Besides that a error type got simplified and tests got refactored.

BridgeAR · 2018-04-12T13:35:38Z

Rebased due to conflicts.

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

mcollina

LGTM with some nits

mcollina · 2018-04-12T13:56:15Z

lib/buffer.js

@@ -920,7 +920,7 @@ Buffer.prototype.write = function write(string, offset, length, encoding) {
      length = remaining;

    if (string.length > 0 && (length < 0 || offset < 0))
-      throw new ERR_BUFFER_OUT_OF_BOUNDS('length', true);
+      throw new ERR_BUFFER_OUT_OF_BOUNDS();


Can you explain this and the change to fs? I didn't expect to see those here.

The length was never used in this case. It was a weird API that I fixed.

Originally the implementation was like:

function a (a, b) { if (b === true) { return 'foo' } return `${a} foo` }

Now it is:

function a(a = undefined) { if (a === undefined) { return 'foo' } return `${a} foo` }

mcollina · 2018-04-12T13:57:07Z

lib/internal/errors.js

@@ -924,7 +934,7 @@ E('ERR_UNCAUGHT_EXCEPTION_CAPTURE_ALREADY_SET',
  Error);
 E('ERR_UNESCAPED_CHARACTERS', '%s contains unescaped characters', TypeError);
 E('ERR_UNHANDLED_ERROR',
-  (err) => {
+  (err = undefined) => {


I don't think this is truly needed. Or is it?

Sadly that is necessary. Default arguments do not count towards function.length as in:

function a(a) {} function b(a = undefined) {} assert.strictEqual(a.length, 1); assert.strictEqual(b.length, 0);

Otherwise it is difficult to validate a couple of functions, but I think it is worth it because it makes sure we make less errors while implementing the errors.

Can you add a comment before the function then? Otherwise it won't be clear to the reader.

mcollina · 2018-04-12T13:57:24Z

lib/internal/errors.js

-  if (isWriting) {
-    return 'Attempt to write outside buffer bounds';
-  } else {
+function bufferOutOfBounds(name = undefined) {


I don't think the default argument is truly needed.

BridgeAR · 2018-04-12T15:06:07Z

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

mcollina

still LGTM

This makes sure the input arguments get validated so implementation errors will be caught early. It also improves a couple of error messages by providing more detailed information and fixes errors detected by the new functionality. Besides that a error type got simplified and tests got refactored. PR-URL: nodejs#19924 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

BridgeAR · 2018-04-13T18:00:46Z

Landed in dca7fb2

This makes sure the input arguments get validated so implementation errors will be caught early. It also improves a couple of error messages by providing more detailed information and fixes errors detected by the new functionality. Besides that a error type got simplified and tests got refactored. PR-URL: #19924 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

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

BridgeAR requested review from jasnell and joyeecheung April 11, 2018 01:18

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Apr 11, 2018

BridgeAR requested a review from a team April 11, 2018 01:18

BridgeAR force-pushed the improve-errors branch from 2a4e78e to 7407009 Compare April 11, 2018 01:34

AyushG3112 reviewed Apr 11, 2018

View reviewed changes

MylesBorins mentioned this pull request Apr 11, 2018

stream: 'readable' have precedence over flowing #18994

Closed

4 tasks

targos approved these changes Apr 12, 2018

View reviewed changes

BridgeAR force-pushed the improve-errors branch from 7407009 to 76a8ce5 Compare April 12, 2018 13:35

mcollina approved these changes Apr 12, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 12, 2018

fixup: add comments

edf2852

mcollina approved these changes Apr 12, 2018

View reviewed changes

jasnell approved these changes Apr 12, 2018

View reviewed changes

BridgeAR closed this Apr 13, 2018

kjin mentioned this pull request Sep 19, 2018

[8.x] Backport http2 changes from 10.x #22850

Closed

4 tasks

BridgeAR deleted the improve-errors branch April 1, 2019 23:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

errors: validate input arguments #19924

errors: validate input arguments #19924

BridgeAR commented Apr 11, 2018

BridgeAR commented Apr 11, 2018 •

edited

Loading

AyushG3112 commented Apr 11, 2018

BridgeAR commented Apr 11, 2018

AyushG3112 Apr 11, 2018

BridgeAR Apr 12, 2018

BridgeAR commented Apr 12, 2018

mcollina left a comment

mcollina Apr 12, 2018

BridgeAR Apr 12, 2018

mcollina Apr 12, 2018

BridgeAR Apr 12, 2018

mcollina Apr 12, 2018

BridgeAR Apr 12, 2018

mcollina Apr 12, 2018

BridgeAR Apr 12, 2018

BridgeAR commented Apr 12, 2018

mcollina left a comment

BridgeAR commented Apr 13, 2018

errors: validate input arguments #19924

errors: validate input arguments #19924

Conversation

BridgeAR commented Apr 11, 2018

Checklist

BridgeAR commented Apr 11, 2018 • edited Loading

AyushG3112 commented Apr 11, 2018

BridgeAR commented Apr 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Apr 12, 2018

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Apr 12, 2018

mcollina left a comment

Choose a reason for hiding this comment

BridgeAR commented Apr 13, 2018

BridgeAR commented Apr 11, 2018 •

edited

Loading