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.

Sign up for GitHub

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

Jump to bottom

Incorrect inspection of Set iterator for set.entries() #24629

Closed
targos opened this issue Nov 24, 2018 · 9 comments
Closed

Incorrect inspection of Set iterator for set.entries() #24629

targos opened this issue Nov 24, 2018 · 9 comments
Labels
confirmed-bug Issues with confirmed bugs. util Issues and PRs related to the built-in util module. v8 engine Issues and PRs related to the V8 dependency.

Comments

@targos
Copy link
Member

targos commented Nov 24, 2018

  • Version: 10, 11, master
  • Platform: Linux
  • Subsystem: util
var set = new Set([1, 2])
console.log(set.entries())

In Node 6 and 8, it outputs:

SetIterator { [ 1, 1 ], [ 2, 2 ] }

In Node 10, 11 and master:

[Set Iterator] { 1, 2 }
@targos targos added the util Issues and PRs related to the built-in util module. label Nov 24, 2018
@devsnek devsnek added v8 engine Issues and PRs related to the V8 dependency. confirmed-bug Issues with confirmed bugs. and removed v8 engine Issues and PRs related to the V8 dependency. labels Nov 24, 2018
@devsnek
Copy link
Member

devsnek commented Nov 24, 2018 •
edited
Loading

Object::PreviewEntries reports unkeyed and gives the flat array.

@nodejs/v8

@targos
Copy link
Member Author

targos commented Nov 24, 2018

Yeah, Chrome has the same behavior:

image

@Trott
Copy link
Member

Trott commented Nov 24, 2018 •
edited
Loading

/ping @BridgeAR

@BridgeAR
Copy link
Member

AFAIC this works as intended. With the change to using Object::PreviewEntries this difference has been discussed as well and we decided to follow chrome.
The set entries only contain the values, so there is no need to display it as an array with the value duplicated.

@devsnek
Copy link
Member

devsnek commented Nov 24, 2018

@BridgeAR its not an accurate representation of the actual iterator results though...

> new Set([2, 4]).entries()
[Set Iterator] { 2, 4 }
> // oh ok its just some numbers
> new Set([2, 4]).entries().next()
{ value: [ 2, 2 ], done: false }
> // wait value is an array now?
>

@targos
Copy link
Member Author

targos commented Nov 24, 2018

It's true that they contain the values but they are still in arrays, so IMHO it is confusing to not see that in the preview.

@BridgeAR
Copy link
Member

I just fixed Object::PreviewEntries to work as intended locally and I'll raise a PR to V8 as soon as I set up the whole machinery for V8 PRs and I'll backport that as soon as it lands.

ruben@BridgeAR-T450s:~/repos/node/node$ ./node
> a = new Set([1,2,3])
Set { 1, 2, 3 }
> a.values()
[Set Iterator] { 1, 2, 3 }
> a.keys()
[Set Iterator] { 1, 2, 3 }
> a.entries()
[Set Iterator] { [ 1, 1 ], [ 2, 2 ], [ 3, 3 ] }
> process.versions
{ http_parser: '2.8.0',
  node: '12.0.0-pre',
  v8: '7.0.276.38-node.13',

Refs: #20831 (the original PR that changed the behavior)

@targos
Copy link
Member Author

targos commented Dec 16, 2018

@BridgeAR what's the status?

@BridgeAR
Copy link
Member

@targos I have to fix the Node.js test expectation first in another PR for V8 as those tests will now fail (Which is intended). I am working on that.

The actual CL to fix the preview: https://chromium-review.googlesource.com/c/v8/v8/+/1350790

joyeecheung pushed a commit to joyeecheung/v8 that referenced this issue Feb 4, 2019
@BridgeAR
Fix preview of set entries
74571c8 
Set entries return an array with the value as first and second entry.
As such these are considered key value pairs to align with maps
entries iterator.
So far the return value was identical to the values iterator and that
is misleading.

This also adds tests to verify the results and improves the coverage
a tiny bit by testing different iterators.

Refs: nodejs/node#24629

[email protected]

Change-Id: I669a724bb4afaf5a713e468b1f51691d22c25253
Reviewed-on: https://chromium-review.googlesource.com/c/1350790
Commit-Queue: Yang Guo <[email protected]>
Reviewed-by: Benedikt Meurer <[email protected]>
Reviewed-by: Jakob Gruber <[email protected]>
Reviewed-by: Yang Guo <[email protected]>
Cr-Commit-Position: refs/heads/master@{#59311}
@BridgeAR BridgeAR mentioned this issue Feb 5, 2019
Closed
4 tasks
BridgeAR added a commit to BridgeAR/node that referenced this issue Feb 16, 2019
@BridgeAR
deps: V8: backport 74571c8
0a0ef6e 
Original commit message:

    Fix preview of set entries

    Set entries return an array with the value as first and second entry.
    As such these are considered key value pairs to align with maps
    entries iterator.
    So far the return value was identical to the values iterator and that
    is misleading.

    This also adds tests to verify the results and improves the coverage
    a tiny bit by testing different iterators.

    Refs: nodejs#24629

    [email protected]

    Change-Id: I669a724bb4afaf5a713e468b1f51691d22c25253
    Reviewed-on: https://chromium-review.googlesource.com/c/1350790
    Commit-Queue: Yang Guo <[email protected]>
    Reviewed-by: Benedikt Meurer <[email protected]>
    Reviewed-by: Jakob Gruber <[email protected]>
    Reviewed-by: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#59311}

Refs: v8/v8@74571c8
pull bot pushed a commit to Rachelmorrell/node that referenced this issue Feb 20, 2019
@BridgeAR
util: update set iterator entries inspection
3d62d0c 
The inspection output for Set#entries() was wrong so far as it did
not return an array as it should have. That was a bug in V8 that is
now fixed and the code in Node.js has to be updated accordingly.

PR-URL: nodejs#25941
Fixes: nodejs#24629
Reviewed-By: Michaël Zasso <[email protected]>
addaleax pushed a commit that referenced this issue Feb 21, 2019
@BridgeAR @addaleax
deps: V8: backport 74571c8
a09fb30 
Original commit message:

    Fix preview of set entries

    Set entries return an array with the value as first and second entry.
    As such these are considered key value pairs to align with maps
    entries iterator.
    So far the return value was identical to the values iterator and that
    is misleading.

    This also adds tests to verify the results and improves the coverage
    a tiny bit by testing different iterators.

    Refs: #24629

    [email protected]

    Change-Id: I669a724bb4afaf5a713e468b1f51691d22c25253
    Reviewed-on: https://chromium-review.googlesource.com/c/1350790
    Commit-Queue: Yang Guo <[email protected]>
    Reviewed-by: Benedikt Meurer <[email protected]>
    Reviewed-by: Jakob Gruber <[email protected]>
    Reviewed-by: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#59311}

Refs: v8/v8@74571c8

PR-URL: #25941
Fixes: #24629
Reviewed-By: Michaël Zasso <[email protected]>
addaleax pushed a commit that referenced this issue Feb 21, 2019
@BridgeAR @addaleax
util: update set iterator entries inspection
f32477a 
The inspection output for Set#entries() was wrong so far as it did
not return an array as it should have. That was a bug in V8 that is
now fixed and the code in Node.js has to be updated accordingly.

PR-URL: #25941
Fixes: #24629
Reviewed-By: Michaël Zasso <[email protected]>
rvagg pushed a commit that referenced this issue Feb 28, 2019
@BridgeAR @rvagg
deps: V8: backport 74571c8
206e4b0 
Original commit message:

    Fix preview of set entries

    Set entries return an array with the value as first and second entry.
    As such these are considered key value pairs to align with maps
    entries iterator.
    So far the return value was identical to the values iterator and that
    is misleading.

    This also adds tests to verify the results and improves the coverage
    a tiny bit by testing different iterators.

    Refs: #24629

    [email protected]

    Change-Id: I669a724bb4afaf5a713e468b1f51691d22c25253
    Reviewed-on: https://chromium-review.googlesource.com/c/1350790
    Commit-Queue: Yang Guo <[email protected]>
    Reviewed-by: Benedikt Meurer <[email protected]>
    Reviewed-by: Jakob Gruber <[email protected]>
    Reviewed-by: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#59311}

Refs: v8/v8@74571c8

PR-URL: #25941
Fixes: #24629
Reviewed-By: Michaël Zasso <[email protected]>
rvagg pushed a commit that referenced this issue Feb 28, 2019
@BridgeAR @rvagg
util: update set iterator entries inspection
4bf58ac 
The inspection output for Set#entries() was wrong so far as it did
not return an array as it should have. That was a bug in V8 that is
now fixed and the code in Node.js has to be updated accordingly.

PR-URL: #25941
Fixes: #24629
Reviewed-By: Michaël Zasso <[email protected]>
cjihrig pushed a commit to cjihrig/node that referenced this issue Mar 12, 2019
@targos @cjihrig
deps: V8: cherry-pick 74571c8
11df998 
Original commit message:

    Fix preview of set entries

    Set entries return an array with the value as first and second entry.
    As such these are considered key value pairs to align with maps
    entries iterator.
    So far the return value was identical to the values iterator and that
    is misleading.

    This also adds tests to verify the results and improves the coverage
    a tiny bit by testing different iterators.

    Refs: nodejs#24629

    [email protected]

    Change-Id: I669a724bb4afaf5a713e468b1f51691d22c25253
    Reviewed-on: https://chromium-review.googlesource.com/c/1350790
    Commit-Queue: Yang Guo <[email protected]>
    Reviewed-by: Benedikt Meurer <[email protected]>
    Reviewed-by: Jakob Gruber <[email protected]>
    Reviewed-by: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#59311}

Refs: v8/v8@74571c8
targos added a commit to targos/node that referenced this issue Mar 12, 2019
@targos
deps: V8: cherry-pick 74571c8
10f045d 
Original commit message:

    Fix preview of set entries

    Set entries return an array with the value as first and second entry.
    As such these are considered key value pairs to align with maps
    entries iterator.
    So far the return value was identical to the values iterator and that
    is misleading.

    This also adds tests to verify the results and improves the coverage
    a tiny bit by testing different iterators.

    Refs: nodejs#24629

    [email protected]

    Change-Id: I669a724bb4afaf5a713e468b1f51691d22c25253
    Reviewed-on: https://chromium-review.googlesource.com/c/1350790
    Commit-Queue: Yang Guo <[email protected]>
    Reviewed-by: Benedikt Meurer <[email protected]>
    Reviewed-by: Jakob Gruber <[email protected]>
    Reviewed-by: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#59311}

Refs: v8/v8@74571c8
targos added a commit that referenced this issue Mar 14, 2019
@targos
deps: V8: cherry-pick 74571c8
cf649c9 
Original commit message:

    Fix preview of set entries

    Set entries return an array with the value as first and second entry.
    As such these are considered key value pairs to align with maps
    entries iterator.
    So far the return value was identical to the values iterator and that
    is misleading.

    This also adds tests to verify the results and improves the coverage
    a tiny bit by testing different iterators.

    Refs: #24629

    [email protected]

    Change-Id: I669a724bb4afaf5a713e468b1f51691d22c25253
    Reviewed-on: https://chromium-review.googlesource.com/c/1350790
    Commit-Queue: Yang Guo <[email protected]>
    Reviewed-by: Benedikt Meurer <[email protected]>
    Reviewed-by: Jakob Gruber <[email protected]>
    Reviewed-by: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#59311}

Refs: v8/v8@74571c8

PR-URL: #25852
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
confirmed-bug Issues with confirmed bugs. util Issues and PRs related to the built-in util module. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@Trott @targos @devsnek @BridgeAR