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

Confusing inspection for sparse arrays #11570

Closed
TimothyGu opened this issue Feb 26, 2017 · 5 comments
Closed

Confusing inspection for sparse arrays #11570

TimothyGu opened this issue Feb 26, 2017 · 5 comments
Labels
util Issues and PRs related to the built-in util module.

Comments

@TimothyGu
Copy link
Member

  • Version: all supported versions (oldest tested branch is v4.x (4.8.0))
  • Platform: all
  • Subsystem: util

In the node REPL, sparse elements of an array are shown as "empty":

> new Array(3)
[ , ,  ]

However, when one copy the resulting [ , , ] and reenter it into the prompt, a different array is shown, thus confusing the developer:

> [ , ,  ]
[ ,  ]

How the Chrome inspector deals with this situation is explicitly make the inspected result not compliant JS:

> new Array(3)
[undefined × 3]
> [undefined, , undefined] // to differentiate between a hole and `undefined`
[undefined, undefined × 1, undefined]

Firefox is more upfront:

> new Array(3)
Array [ <3 empty slots> ]
> [undefined, , undefined] // to differentiate between a hole and `undefined`
Array [ undefined, <1 empty slot>, undefined ]
@TimothyGu TimothyGu added the util Issues and PRs related to the built-in util module. label Feb 26, 2017
@bnoordhuis
Copy link
Member

I agree it's confusing but I think it's correct. Due to the trailing comma, [ , , ] is actually a two-element array, not three:

> new Array(3).length
3
> [ , , ].length
2

@TimothyGu
Copy link
Member Author

Yeah, the REPL is doing its job alright. It's just the inspection is a bit counter-intuitive.

Personally I'm leaning more towards Firefox's approach. It's clear and unambiguous, and I'm worried the Unicode × character used in Chrome's version might not be friendly towards Windows terminals. Substitutes like the letter X might be confusing in themselves.

@aqrln
Copy link
Contributor

aqrln commented Feb 27, 2017

@TimothyGu @bnoordhuis what about something like this?

> new Array(3)
[ [3 empty] ]
> [1, 2, , , , 3, 4, 5]
[ 1, 2, [3 empty], 3, 4, 5 ]
> [1, 2, , , , 3, 4, 5, , 2]
[ 1, 2, [3 empty], 3, 4, 5, [1 empty], 2 ]
Screenshot

@TimothyGu
Copy link
Member Author

I'd be okay with that. Personally I'd prefer the [3 empty] to have the same color as undefined just to be consistent, though that's bikeshedding.

@aqrln
Copy link
Contributor

aqrln commented Feb 27, 2017

@TimothyGu IMO it's more appropriate to mark it with special style, but ¯\_(ツ)_/¯

aqrln added a commit to aqrln/node that referenced this issue Mar 7, 2017
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
jungx098 pushed a commit to jungx098/node that referenced this issue Mar 21, 2017
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]>
Niek added a commit to Niek/node that referenced this issue Mar 16, 2021
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
victorgomes pushed a commit to v8/node that referenced this issue Mar 24, 2021
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
victorgomes pushed a commit to v8/node that referenced this issue Apr 6, 2021
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
pthier pushed a commit to v8/node that referenced this issue May 12, 2021
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
victorgomes pushed a commit to v8/node that referenced this issue Jun 8, 2021
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
pthier pushed a commit to v8/node that referenced this issue Jul 5, 2021
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
victorgomes pushed a commit to v8/node that referenced this issue Aug 9, 2021
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
pthier pushed a commit to v8/node that referenced this issue Sep 13, 2021
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
victorgomes pushed a commit to v8/node that referenced this issue Oct 12, 2021
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
pthier pushed a commit to v8/node that referenced this issue Nov 19, 2021
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
pthier pushed a commit to v8/node that referenced this issue Dec 9, 2021
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
victorgomes pushed a commit to v8/node that referenced this issue Jan 5, 2022
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
pthier pushed a commit to v8/node that referenced this issue Feb 15, 2022
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
victorgomes pushed a commit to v8/node that referenced this issue Mar 8, 2022
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
victorgomes pushed a commit to v8/node that referenced this issue Apr 8, 2022
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
pthier pushed a commit to v8/node that referenced this issue May 12, 2022
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
pthier pushed a commit to v8/node that referenced this issue Jun 14, 2022
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
pthier pushed a commit to v8/node that referenced this issue Jul 20, 2022
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
victorgomes pushed a commit to v8/node that referenced this issue Aug 11, 2022
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
pthier pushed a commit to v8/node that referenced this issue Sep 22, 2022
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
victorgomes pushed a commit to v8/node that referenced this issue Oct 21, 2022
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
pthier pushed a commit to v8/node that referenced this issue Nov 16, 2022
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
pthier pushed a commit to v8/node that referenced this issue Feb 6, 2023
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
victorgomes pushed a commit to v8/node that referenced this issue Mar 16, 2023
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
pthier pushed a commit to v8/node that referenced this issue Apr 26, 2023
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
victorgomes pushed a commit to v8/node that referenced this issue May 22, 2023
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
pthier pushed a commit to v8/node that referenced this issue Jul 4, 2023
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
pthier pushed a commit to v8/node that referenced this issue Aug 24, 2023
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
victorgomes pushed a commit to v8/node that referenced this issue Sep 12, 2023
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
victorgomes pushed a commit to v8/node that referenced this issue Sep 12, 2023
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
victorgomes pushed a commit to v8/node that referenced this issue Nov 8, 2023
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
pthier pushed a commit to v8/node that referenced this issue Jan 17, 2024
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
victorgomes pushed a commit to v8/node that referenced this issue Feb 14, 2024
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
victorgomes pushed a commit to v8/node that referenced this issue Mar 4, 2024
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
pthier pushed a commit to v8/node that referenced this issue Mar 21, 2024
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
victorgomes pushed a commit to v8/node that referenced this issue Apr 23, 2024
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
victorgomes pushed a commit to v8/node that referenced this issue Jun 3, 2024
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
pthier pushed a commit to v8/node that referenced this issue Jul 15, 2024
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
victorgomes pushed a commit to v8/node that referenced this issue Aug 22, 2024
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
pthier pushed a commit to v8/node that referenced this issue Oct 2, 2024
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
victorgomes pushed a commit to v8/node that referenced this issue Nov 18, 2024
See: https://bugs.chromium.org/p/v8/issues/detail?id=11570

This PR disables the Proxy test in test/parallel/test-v8-serdes.js while https://chromium-review.googlesource.com/c/v8/v8/+/2739980 is being merged.

Will do another PR to re-enabled the (fixed) test once the 2739980 has landed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants