util: change sparse arrays inspection format #11576

aqrln · 2017-02-27T07:35:31Z

Missing elements in sparse arrays used to be serialized to empty placeholders delimited with commas by util.inspect() and in some cases the result was a syntactically correct representation of a JavaScript array with shorter length than the original one. This PR implements @TimothyGu's suggestion to change the way util.inspect() formats sparse arrays to something similar to how Firefox shows them.

Fixes: #11570

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

util

aqrln · 2017-02-27T07:36:54Z

Existing tests are updated, but I'll add a couple of new ones.

UPD: already added.

TimothyGu · 2017-02-27T07:50:18Z

lib/util.js

+      index++;
+    }
+    if (emptyElements > 0) {
+      output.push(ctx.stylize(`[${emptyElements} empty]`, 'special'));


I still stand by my opinion of using 'undefined' as the color, though I'm not going to block this PR if that's what others prefer.

Well, let's wait and see what others think about it. If there's no specific preference, I'll change it to 'undefined'.

TimothyGu · 2017-02-27T07:50:21Z

test/parallel/test-util-inspect.js

@@ -832,12 +832,12 @@ checkAlignment(new Map(big_array.map(function(y) { return [y, null]; })));
 // Do not backport to v5/v4 unless all of
 // https://github.com/nodejs/node/pull/6334 is backported.
 {
-  const x = Array(101);
+  const x = ','.repeat(100).split(',');  // array of 101 empty strings


new Array(101).fill() might be more semantically correct.

Right, thanks.

aqrln · 2017-02-27T10:40:26Z

@TimothyGu I've updated the PR to use the 'undefined' color instead of the 'special' one. It indeed seems better.

evanlucas · 2017-02-27T11:30:32Z

I'm on the fence on this one...leaning slightly towards keep existing behavior. I'm not sure that this is in fact more readable... going ahead and marking as semver-major though

aqrln · 2017-02-27T12:14:26Z

@evanlucas yes, certainly semver-major. An alternative solution that is at least similar to existing behavior yet solves the issue is to add an extra comma if the last element of a sparse array is missing.

Fishrock123 · 2017-02-27T16:24:54Z

I agree with @evanlucas. I think something a bit more clear would be desired e.g. [3 empty indices skipped]. Not sure that's saving anything then though so...

jasnell · 2017-02-27T19:08:38Z

I don't feel strongly about it one way or another but one fairly compact approach is to use one . for each omitted index... e.g.

[1, 2, {...}, 6, 7] // three index positions skipped
[1, 2, {..}, 5, 6, 7] // two index positions skipped
[1, 2, {..........}, 13, 14] // ten index positions skipped

aqrln · 2017-02-27T19:18:18Z

@Fishrock123 changed it to be

> new Array(3)
[ [3 uninitialized elements] ]
> [1, 2, , , , 3, 4, 5]
[ 1, 2, [3 uninitialized elements], 3, 4, 5 ]
> [1, 2, , , , 3, 4, 5, , 2]
[ 1,
  2,
  [3 uninitialized elements],
  3,
  4,
  5,
  [1 uninitialized element],
  2 ]

aqrln · 2017-02-27T19:18:23Z

@TimothyGu looks like this is somewhat controversial 😄 As I already suggested above, a better solution might be to leave the current format with a small addition: if a sparse array doesn't have its last element, just append an extra dangling comma. This way the output of util.format() will not be contradicting JavaScript syntax and #11570 will be fixed, though the other way. What do you think about it?

addaleax · 2017-02-27T19:23:07Z

lib/util.js

  }
+  const remaining = value.length - index;


Shouldn’t this be value.length - visisbleLength?

No, it shouldn't. visibleLength is the count of... well, printed elements, i.e., 3 in case of [ 1, [10000 uninitialized elements], 2 ] (maybe the variable name isn't that great, though). remaining, on the other hand, is the count of real array indices which values were not printed.

aqrln · 2017-02-27T19:23:29Z

@jasnell well, this would be hard to format with respect to inspectOptions.maxArrayLength and inspectOptions.breakLength.

addaleax

LGTM

(edit: +1 to doing this and +1 to “uninitialized”)

TimothyGu · 2017-02-27T21:00:22Z

if a sparse array doesn't have its last element, just append an extra dangling comma

No, though it's now "correct", it wouldn't be less confusing.

The goal for the message is to be 1) clear/unambiguous, and 2) concise.

Having an extra dangling comma at the end would indeed be unambiguous in the sense of ES compliance, but it would be confusing considering chances are people might have already gotten used to this quirk in Node.js.

That applies also to the suggestion to use {..} as well: it's simply not beginner-friendly.

At the same time, uninitialized seems to convey the same information as, only in a less concise fashion than empty.

aqrln · 2017-02-28T13:06:27Z

Having an extra dangling comma at the end would indeed be unambiguous in the sense of ES compliance, but it would be confusing considering chances are people might have already gotten used to this quirk in Node.js.

+1, good point.

So what about the placeholder message? Considering browser examples provided in #11570, I don't really like the way Chromium formats sparse arrays (it isn't less confusing when [undefined] and [undefined × 1] mean two completely different things), the format Firefox uses is more appealing to me, though the word "slots" is rather misleading. Speaking about the message we currently have in this PR, first of all, I think I can change the word element(s) to item(s) because not only it is shorter but it is also the term used in ... N more items message.

Hence the current variants:

"empty items"
"uninitialized items"

It would be great if anyone has better suggestions. @TimothyGu, @addaleax?

addaleax · 2017-02-28T13:14:32Z

I like “uninitialized” because it tells the user how that hole in the array actually came to be – it was simply not initialized to any particular value (except for when somebody does delete arr[i], but I’d say that doesn’t happen very often).

I’d be fine with any of {“empty”, “uninitialized”, “missing”} × {“item(s)”, “element(s)”}, though.

addaleax · 2017-03-01T16:31:05Z

@nodejs/ctc This needs another review from a CTC member. Also, is anybody objecting to the current format ([ 'foo', [1 uninitialized element], 'baz' ])?

cjihrig · 2017-03-01T16:34:14Z

The square bracket use makes it a bit harder to parse. At a quick glance, it looks like a nested array.

jasnell · 2017-03-01T16:34:59Z

I'm good with the [ bracket but a { could work also. Either way I can live with it

cjihrig · 2017-03-01T16:36:52Z

That would read like an object literal. Maybe < and >. Maybe I'm reading into it too much :-)

aqrln · 2017-03-01T16:37:23Z

@cjihrig, just wanted to suggest it :)

aqrln · 2017-03-01T16:38:32Z

Also, what about s/element(s)/item(s) to be consistent with an existing "... N more items" message?

Fishrock123 · 2017-03-01T16:38:33Z

I think it is a bit excessively long but I don't see good and descriptive options otherwise.

Well, I suppose empty is probably better than uninitialized?

TimothyGu

LGTM other than the two nits. Thanks for working on this!

TimothyGu · 2017-03-01T17:15:37Z

test/parallel/test-util-inspect.js

  assert(!/1 more item/.test(util.inspect(x, {maxArrayLength: 101})));
 }

 {
+  const x = new Array(101).fill();
+  assert(/^\[ ... 101 more items ]$/.test(
+      util.inspect(x, {maxArrayLength: 0})));


nit: I know the regex is there already, but it doesn't seem to be needed, since assert.strictEqual(util.inspect(x, {maxArrayLength: 0}), '[ ... 101 more items ]') should accomplish the same.

I think it's better to open a new PR and change it for the whole group of tests here so that they look consistent and this PR doesn't modify the tests which are not related to its changes.

Other that these two and analogous Uint8Array test (line 873), there are tests where a regex can be reduced to a simple .endsWith() check.

TimothyGu · 2017-03-01T17:15:45Z

test/parallel/test-util-inspect.js

+      util.inspect(x, {maxArrayLength: 0})));
+}
+
+{
  const x = Array(101);
  assert(/^\[ ... 101 more items ]$/.test(
      util.inspect(x, {maxArrayLength: 0})));


addaleax · 2017-03-04T18:41:21Z

CI: https://ci.nodejs.org/job/node-test-commit/8267/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/620/

aqrln · 2017-03-04T23:14:19Z

@addaleax I guess CITGM failures aren't related to this patch

@TimothyGu

Missing elements in sparse arrays used to be serialized to empty placeholders delimited with commas by util.inspect() and in some cases the result was a syntactically correct representation of a JavaScript array with shorter length than the original one. This commit implements @TimothyGu's suggestion to change the way util.inspect() formats sparse arrays to something similar to how Firefox shows them. Fixes: nodejs#11570

aqrln · 2017-03-07T17:59:01Z

Squashed the commits into one.

aqrln · 2017-03-09T16:50:55Z

@addaleax ping

addaleax · 2017-03-09T16:53:46Z

@aqrln Thanks for the ping, I’m busy but I’ll land this later today if nobody beats me to it

addaleax · 2017-03-10T00:17:24Z

Landed in ec2f098, thanks for the PR!

@TimothyGu

Missing elements in sparse arrays used to be serialized to empty placeholders delimited with commas by util.inspect() and in some cases the result was a syntactically correct representation of a JavaScript array with shorter length than the original one. This commit implements @TimothyGu's suggestion to change the way util.inspect() formats sparse arrays to something similar to how Firefox shows them. Fixes: #11570 PR-URL: #11576 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

* Enclose tests that used to introduce module-level variables into their own scopes. * Replace ES5 anonymous functions with arrow functions where it makes sense. * And make one arrow function a regular function thus fixing a bug in a getter inside an object created in "Array with dynamic properties" test. This getter has never been invoked though, so the test hasn't been failing. * Convert snake_case identifiers to camelCase. * Make some variable names more readable. * Replace regular expressions in maxArrayLength tests with simple assert.strictEquals() and assert(...endsWith()) checks, as suggested in <nodejs#11576 (comment)>.

* Enclose tests that used to introduce module-level variables into their own scopes. * Replace ES5 anonymous functions with arrow functions where it makes sense. * And make one arrow function a regular function thus fixing a bug in a getter inside an object created in "Array with dynamic properties" test. This getter has never been invoked though, so the test hasn't been failing. * Convert snake_case identifiers to camelCase. * Make some variable names more readable. * Replace regular expressions in maxArrayLength tests with simple assert.strictEquals() and assert(...endsWith()) checks, as suggested in <#11576 (comment)>. PR-URL: #11779 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

@TimothyGu

Missing elements in sparse arrays used to be serialized to empty placeholders delimited with commas by util.inspect() and in some cases the result was a syntactically correct representation of a JavaScript array with shorter length than the original one. This commit implements @TimothyGu's suggestion to change the way util.inspect() formats sparse arrays to something similar to how Firefox shows them. Fixes: nodejs#11570 PR-URL: nodejs#11576 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

* Enclose tests that used to introduce module-level variables into their own scopes. * Replace ES5 anonymous functions with arrow functions where it makes sense. * And make one arrow function a regular function thus fixing a bug in a getter inside an object created in "Array with dynamic properties" test. This getter has never been invoked though, so the test hasn't been failing. * Convert snake_case identifiers to camelCase. * Make some variable names more readable. * Replace regular expressions in maxArrayLength tests with simple assert.strictEquals() and assert(...endsWith()) checks, as suggested in <nodejs#11576 (comment)>. PR-URL: nodejs#11779 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

* Enclose tests that used to introduce module-level variables into their own scopes. * Replace ES5 anonymous functions with arrow functions where it makes sense. * And make one arrow function a regular function thus fixing a bug in a getter inside an object created in "Array with dynamic properties" test. This getter has never been invoked though, so the test hasn't been failing. * Convert snake_case identifiers to camelCase. * Make some variable names more readable. * Replace regular expressions in maxArrayLength tests with simple assert.strictEquals() and assert(...endsWith()) checks, as suggested in <#11576 (comment)>. PR-URL: #11779 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Feb 27, 2017

aqrln force-pushed the inspect-sparse-arrays branch from 61b0999 to 3fe8d6a Compare February 27, 2017 07:41

TimothyGu reviewed Feb 27, 2017

View reviewed changes

aqrln force-pushed the inspect-sparse-arrays branch 2 times, most recently from b692e86 to 72ecc70 Compare February 27, 2017 10:28

aqrln force-pushed the inspect-sparse-arrays branch from 72ecc70 to 8993097 Compare February 27, 2017 11:26

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

aqrln force-pushed the inspect-sparse-arrays branch from 8993097 to e923dbb Compare February 27, 2017 12:22

addaleax reviewed Feb 27, 2017

View reviewed changes

addaleax approved these changes Feb 27, 2017

View reviewed changes

jasnell approved these changes Mar 1, 2017

View reviewed changes

addaleax approved these changes Mar 1, 2017

View reviewed changes

TimothyGu approved these changes Mar 1, 2017

View reviewed changes

cjihrig approved these changes Mar 1, 2017

View reviewed changes

targos approved these changes Mar 2, 2017

View reviewed changes

evanlucas approved these changes Mar 2, 2017

View reviewed changes

addaleax added this to the 8.0.0 milestone Mar 4, 2017

aqrln force-pushed the inspect-sparse-arrays branch from 2cb034c to fb4fe0d Compare March 7, 2017 17:50

addaleax closed this Mar 10, 2017

aqrln deleted the inspect-sparse-arrays branch March 10, 2017 00:19

aqrln mentioned this pull request Mar 10, 2017

test: refactor test-util-inspect.js #11779

Closed

2 tasks

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

vsemozhetbyt mentioned this pull request Jul 25, 2017

util.inspect is slow for large sparse arrays #14487

Closed

ocombe mentioned this pull request Jan 18, 2018

Update i18n locale data extraction script angular/angular#21626

Closed

util: change sparse arrays inspection format #11576

util: change sparse arrays inspection format #11576

Conversation

aqrln commented Feb 27, 2017 • edited Loading

Checklist

Affected core subsystem(s)

aqrln commented Feb 27, 2017 • edited Loading

TimothyGu Feb 27, 2017

Choose a reason for hiding this comment

aqrln Feb 27, 2017 • edited Loading

Choose a reason for hiding this comment

TimothyGu Feb 27, 2017

Choose a reason for hiding this comment

aqrln Feb 27, 2017

Choose a reason for hiding this comment

aqrln commented Feb 27, 2017

evanlucas commented Feb 27, 2017

aqrln commented Feb 27, 2017

Fishrock123 commented Feb 27, 2017

jasnell commented Feb 27, 2017

aqrln commented Feb 27, 2017

aqrln commented Feb 27, 2017 • edited Loading

addaleax Feb 27, 2017

Choose a reason for hiding this comment

aqrln Feb 27, 2017

Choose a reason for hiding this comment

aqrln commented Feb 27, 2017

addaleax left a comment • edited Loading

Choose a reason for hiding this comment

TimothyGu commented Feb 27, 2017

aqrln commented Feb 28, 2017

addaleax commented Feb 28, 2017

addaleax commented Mar 1, 2017

cjihrig commented Mar 1, 2017

jasnell commented Mar 1, 2017

cjihrig commented Mar 1, 2017

aqrln commented Mar 1, 2017

aqrln commented Mar 1, 2017

Fishrock123 commented Mar 1, 2017

TimothyGu left a comment

Choose a reason for hiding this comment

TimothyGu Mar 1, 2017

Choose a reason for hiding this comment

aqrln Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

aqrln Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

TimothyGu Mar 1, 2017

Choose a reason for hiding this comment

addaleax commented Mar 4, 2017

aqrln commented Mar 4, 2017

aqrln commented Mar 7, 2017

aqrln commented Mar 9, 2017

addaleax commented Mar 9, 2017

addaleax commented Mar 10, 2017

aqrln commented Feb 27, 2017 •

edited

Loading

aqrln commented Feb 27, 2017 •

edited

Loading

aqrln Feb 27, 2017 •

edited

Loading

aqrln commented Feb 27, 2017 •

edited

Loading

addaleax left a comment •

edited

Loading

aqrln Mar 1, 2017 •

edited

Loading

aqrln Mar 1, 2017 •

edited

Loading