lib/src: refactoring out of range index and and migrate error for consistency #11296

larissayvette · 2017-02-10T17:28:26Z

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

Affected core subsystem(s)

lib, src
Changing where RangeError was out of range index to Index out of range

jasnell

If we were to do this, I'd prefer to make it part of the migration to using internal/errors.js. See #11273.

Changing error messages like this is a semver-major change and has a very real chance of breaking user code. Accordingly, since we'll be moving this code over to using internal/errors.js anyway, we might as well do both at the same time.

Trott · 2017-02-10T18:03:54Z

@jasnell wrote:

If we were to do this

I don't think there's any question we should do this. There is no reason for writeInt8() to return RangeError: Index out of range but writeFloatLE() to return RangeError: out of range index in the code below.

let buf = Buffer.alloc(9);
try {
  buf.writeInt8(1, -1); 
} catch (e) {
  console.log(e.toString()); // RangeError: Index out of range
}
try {
  buf.writeFloatLE(1, -1); 
} catch (e) {
  console.log(e.toString()); // RangeError: out of range index
}

@jasnell continued:

...I'd prefer to make it part of the migration to using internal/errors.js

Would you be OK with that being a subsequent PR? I'd prefer we separate out changes to error messages from migrations to internal/error.js rather than bundling those adjacent-but-not-integrally-related things. I can make sure that second PR happens immediately after this one lands if that affects your spider-sense on this one.

jasnell · 2017-02-10T18:15:19Z

As long as the two land closely together, or perhaps even as two commits in the same PR, that's fine.

Trott

LGTM if CI is green. Might be good to edit the commit message to make it clear that this is not changing an error message just to change it, but rather taking two error messages that are used in essentially identical situations and making them consistent.

jasnell · 2017-02-10T18:35:21Z

I will clear my "changes requested" review once I get a chance to review the other PR.

Trott · 2017-02-14T00:53:23Z

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

Trott · 2017-02-17T15:42:13Z

@jasnell It looks like sometimes this RangeError is thrown in JS-land (in which case we can migrate to the internal error system) and sometimes it is thrown by C++ (in which case we cannot, right? Maybe I'm wrong?).

For the sake of consistency, I wonder if it makes sense to not migrate the JS-land errors to the new system until we can do errors thrown in C++ as well. This way, you don't get very dissimilar RangeError objects depending on whether you are trying to read a float or an int. What do you think?

jasnell · 2017-02-17T15:55:00Z

We're always going to have at least some inconsistency given that errors thrown from V8 will not have this information and neither will userland code. I will work on providing an equivalent mechanism for the errors thrown from C/C++ but I think it makes sense (and is best) to incrementally move forward. If we try to hold off and do them all at once, we're going to end up with too much of a moving target (I've already tried it). I think userland will be willing to forgive some short term inconsistency if it means we're making progress towards something better.

Trott · 2017-02-19T06:02:57Z

test/parallel/test-buffer-write-noassert.js

    invalidArgs[1] = -1;
    assert.throws(
      () => Buffer.alloc(9)[funx](...invalidArgs),
-      /^RangeError: (?:Index )?out of range(?: index)?$/
+      //(err) => err instanceof RangeError && err.message === "Index out of range"


Nit: Should this commented-out code be removed?

ok, fixing it

Trott · 2017-02-19T06:04:44Z

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

TimothyGu · 2017-02-20T22:55:36Z

There was a force push right after last CI, so it didn't go through.

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

TimothyGu · 2017-02-20T22:57:01Z

Heh, seems like there is a merge conflict: https://ci.nodejs.org/job/node-test-commit/8037/console

@larissayvette can you git rebase master?

larissayvette · 2017-02-21T01:55:57Z

@TimothyGu alright

TimothyGu · 2017-02-21T02:30:35Z

Seems to be working now: https://ci.nodejs.org/job/node-test-pull-request/6521/

Trott · 2017-02-21T06:43:10Z

Tests are passing (hooray!) but linter is failing because a number of lines in test-buffer-fill.js are longer than 80 characters.

Easiest solution is probably to change things like this:

assert.throws(() =>
             Buffer.allocUnsafe(8).fill('a', -1),
              /^RangeError(?:\[ERR_INDEX_OUT_OF_RANGE\])?: Index out of range$/);

...to something more like this:

assert.throws(
  () => Buffer.allocUnsafe(8).fill('a', -1),
  /^RangeError(?:\[ERR_INDEX_OUT_OF_RANGE\])?: Index out of range$/
);

jasnell · 2017-02-21T10:17:46Z

Is there a problem using common.expectsError()?

larissayvette · 2017-02-21T13:14:02Z

@jasnell when common.expectsError() is used it produces this error
'undefined === ERR_INDEX_OUT_OF_RANGE`

larissayvette · 2017-02-21T14:21:48Z

@Trott I think the lint is correct now

Trott · 2017-02-21T17:19:34Z

On closer inspection, @jasnell is right to suggest that the regular expression is unnecessary in most of the places it's used here. It only needs to be used in places where the throw might be C++ or JS. That's probably only going to happen in test-buffer-write-noassert and/or test-buffer-read.

Trott · 2017-02-21T17:21:25Z

test/parallel/test-buffer-alloc.js

@@ -956,7 +956,7 @@ assert.throws(() => {
  const a = Buffer.alloc(1);
  const b = Buffer.alloc(1);
  a.copy(b, 0, 0x100000000, 0x100000001);
-}, /out of range index/);
+}, /^RangeError(?:\[ERR_INDEX_OUT_OF_RANGE\])?: Index out of range$/);


This regexp can be replaced with common.expectsError(undefined, RangeError, 'Index out of range')

Trott · 2017-02-21T17:24:46Z

test/parallel/test-buffer-compare-offset.js

@@ -57,7 +57,7 @@ assert.strictEqual(1, a.compare(b, Infinity, -Infinity));
 // zero length target because default for targetEnd <= targetSource
 assert.strictEqual(1, a.compare(b, '0xff'));

-const oor = /out of range index/;
+const oor = /^RangeError(?:\[ERR_INDEX_OUT_OF_RANGE\])?: Index out of range$/;


This line can be replaced with:

const oor = common.expectsError('ERR_INDEX_OUT_OF_RANGE');

Will need to change the require('../common'); to const common = require('../common'); too.

Trott · 2017-02-21T17:28:14Z

test/parallel/test-buffer-fill.js

-             buf1.fill('a', 0, 0, 'foo'), /^TypeError: Unknown encoding: foo$/);
+assert.throws(
+  () => buf1.fill(0, -1),
+  /^RangeError(?:\[ERR_INDEX_OUT_OF_RANGE\])?: Index out of range$/);


This one (and likely the others in this file) can be replaced with common.expectsError('ERR_INDEX_OUT_OF_RANGE'));. (Will need to add const common = require... at top of file.)

From line 187 to 196 , test passes only when it is regular expression. and for the subsequent error message it is common.expectsError(undefined, RangeError, 'Index out of range')); that works and not common.expectsError('ERR_INDEX_OUT_OF_RANGE'));

Trott · 2017-02-21T17:29:39Z

test/parallel/test-buffer-write-noassert.js

  {
    const invalidArgs = Array.from(args);
    invalidArgs[1] = -1;
    assert.throws(
      () => Buffer.alloc(9)[funx](...invalidArgs),
-      /^RangeError: (?:Index )?out of range(?: index)?$/
+      /^RangeError(?:\[ERR_INDEX_OUT_OF_RANGE\])?: Index out of range$/


I think this is the only regexp that should stay because we don't know ahead of time whether the error is coming from JS (if writing an int, for example) or C++ (if writing a float, for example).

+1... we should probably look at fixing up common.expectsError() to account for this, however. Will have to think on that for a bit

@jasnell If we really wanted to use common.expectsError() and not a RegExp, we could do something like the following (untested) code:

let expectedError; if (['writeFloatBE', 'writeFloatLE'].includes(funx)) { expectedError = common.expectsError(undefined, RangeError, 'Index out of range'); } else { expectedError = common.expectsError('ERR_INDEX_OUT_OF_RANGE', RangeError, 'Index out of range'); } assert.throws(() => { Buffer.alloc(9)[funx](...invalidArgs) }, expectedError);

Definitely more complicated, though, so I'm fine with the RegExp approach. Either way.

bleh... let's keep it with the regex for now

jasnell · 2017-02-22T15:10:44Z

Keep in mind also that eventually, even the C/C++ errors thrown by Node.js will include these codes and the augmented name. We'll still need to be careful about errors thrown by V8 or Chakra

jasnell

Generally LGTM with some nits and @Trott's comments addressed. Thank you for this!

jasnell · 2017-02-22T15:12:44Z

doc/api/errors.md

+  <a id="ERR_INDEX_OUT_OF_RANGE"></a>
+  ### ERR_INDEX_OUT_OF_RANGE
+
+The `'ERR_INDEX_OUT_OF_RANGE'` error code is used to alert user that index parsed is out of range


I would suggest rewording just a bit here...

The `'ERR_INDEX_OUT_OF_RANGE'` error code is used when a given index is out of the accepted range.

I will make the change

jasnell · 2017-02-22T15:15:37Z

test/parallel/test-buffer-fill.js

+  /^RangeError(?:\[ERR_INDEX_OUT_OF_RANGE\])?: Index out of range$/);
+assert.throws(
+  () => buf1.fill(0, 0, buf1.length + 1),
+  /^RangeError(?:\[ERR_INDEX_OUT_OF_RANGE\])?: Index out of range$/);


For the cases where the regex must still be provided, it would be easiest to change later on if the regex was defined once and assigned then used multiple times... e.g.

const errRegex = /^RangeError .../; assert.throws(() => { /* ... */ }, errRegex); assert.throws(() => { /* ... */ }, errRegex);

Trott · 2017-02-24T17:11:56Z

test/parallel/test-buffer-fill.js

 const assert = require('assert');
+const errors = require('../../lib/internal/errors');


Can this be require('internal/errors'); along with a // Flags: --expose-internals comment at the top of the file instead?

Trott · 2017-02-24T17:14:30Z

test/parallel/test-buffer-fill.js

-              /^TypeError: encoding must be a string$/);
-assert.throws(() =>
-             buf1.fill('a', 0, 0, 'foo'), /^TypeError: Unknown encoding: foo$/);
+const errRegex =


I don't think we need errRegex in this file. I think all instances of it can be replaced with common.expectsError().

Trott · 2017-02-24T17:53:12Z

test/parallel/test-buffer-fill.js

 const assert = require('assert');
+const errors = require('../../lib/internal/errors');// Flags: --expose-internals


I think the comment needs to be the first line of the file. The idea of using it is to be able to change ../../lib/internal/errors on this line to internal/errors. This way, the test runs based on version of node that is compiled and not the contents of lib in the source tree at the time the test is run.

(Also, it means that in order to run the test from the command line, you need to either use tools/test.py or else pass the --expose-internals flag to node. But lots of tests are like that already, so that should be fine.)

Indeed. The // Flags: --expose-internals should be the first line of the test file,
The import must also be require('internal/errors'), without the ../../lib/ part.

Trott · 2017-03-03T17:09:09Z

@jasnell Two things: Can you give this another look since it has changed a bit? Also, are we not landing these new internal error things on master yet? What needs to happen before we can start doing that?

Trott · 2017-03-03T17:10:46Z

Oh, it looks like this needs to be updated for the new common.expectsError() signature.

Instead of common.expectsError(code, type, msg), it's now common.expectsError({code, type, message: msg}).

Trott · 2017-03-07T18:26:39Z

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

Trott · 2017-05-28T03:25:39Z

Rebased and force pushed.

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

Trott · 2017-05-28T04:13:09Z

Landed in 234353a.
Thanks for the contribution, @larissayvette! 🎉

PR-URL: #11296 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Feb 10, 2017

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 10, 2017

jasnell requested changes Feb 10, 2017

View reviewed changes

Trott approved these changes Feb 10, 2017

View reviewed changes

jasnell approved these changes Feb 13, 2017

View reviewed changes

larissayvette force-pushed the range branch from 1fd588b to b44b9ec Compare February 14, 2017 17:53

larissayvette force-pushed the range branch from 193e668 to a3902d2 Compare February 17, 2017 16:02

Trott reviewed Feb 19, 2017

View reviewed changes

larissayvette force-pushed the range branch from 7ac8d38 to 1b0d734 Compare February 20, 2017 21:52

larissayvette force-pushed the range branch from 1b0d734 to 6262ef5 Compare February 21, 2017 01:56

larissayvette force-pushed the range branch from 6262ef5 to 96f6e7a Compare February 21, 2017 14:18

larissayvette changed the title ~~lib/src: refactoring out of range index~~ lib/src: refactoring out of range index and and migrate error for consistency Feb 21, 2017

Trott reviewed Feb 21, 2017

View reviewed changes

jasnell approved these changes Feb 22, 2017

View reviewed changes

larissayvette force-pushed the range branch from 96f6e7a to c11b933 Compare February 22, 2017 16:57

Trott reviewed Feb 24, 2017

View reviewed changes

larissayvette force-pushed the range branch from c11b933 to 7284189 Compare February 24, 2017 17:50

Trott reviewed Feb 24, 2017

View reviewed changes

larissayvette force-pushed the range branch from 7284189 to 9a2d0e7 Compare March 2, 2017 21:59

larissayvette force-pushed the range branch 2 times, most recently from 36226c2 to a47c1d1 Compare March 7, 2017 18:22

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

larissayvette added 3 commits May 27, 2017 16:39

lib/src: refactoring out of range index

41ba54f

lib,doc,test: creating error code for index out of range

ee453f3

test: updating error message

c12569a

Trott force-pushed the range branch from a47c1d1 to c12569a Compare May 28, 2017 03:23

Trott closed this May 28, 2017

Trott pushed a commit that referenced this pull request May 28, 2017

lib,src: refactor buffer out of range index

234353a

PR-URL: #11296 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

		const assert = require('assert');
		const errors = require('../../lib/internal/errors');

lib/src: refactoring out of range index and and migrate error for consistency #11296

lib/src: refactoring out of range index and and migrate error for consistency #11296

Conversation

larissayvette commented Feb 10, 2017

Checklist

Affected core subsystem(s)

jasnell left a comment

Choose a reason for hiding this comment

Trott commented Feb 10, 2017

jasnell commented Feb 10, 2017

Trott left a comment • edited Loading

Choose a reason for hiding this comment

jasnell commented Feb 10, 2017

Trott commented Feb 14, 2017

Trott commented Feb 17, 2017

jasnell commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Feb 19, 2017

TimothyGu commented Feb 20, 2017

TimothyGu commented Feb 20, 2017

larissayvette commented Feb 21, 2017

TimothyGu commented Feb 21, 2017 • edited Loading

Trott commented Feb 21, 2017

jasnell commented Feb 21, 2017

larissayvette commented Feb 21, 2017

larissayvette commented Feb 21, 2017

Trott commented Feb 21, 2017

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

Trott Feb 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Feb 22, 2017

jasnell 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

Trott commented Mar 3, 2017

Trott commented Mar 3, 2017 • edited by TimothyGu Loading

Trott commented Mar 7, 2017

Trott commented May 28, 2017

Trott commented May 28, 2017

Trott left a comment •

edited

Loading

TimothyGu commented Feb 21, 2017 •

edited

Loading

Trott Feb 22, 2017 •

edited

Loading

Trott commented Mar 3, 2017 •

edited by TimothyGu

Loading