Skip to content

util: use proper circular reference checking #14790

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
Closed
wants to merge 2 commits into from
Closed

util: use proper circular reference checking #14790

wants to merge 2 commits into from

Conversation

@addaleax
Copy link
Member

@addaleax addaleax commented Aug 12, 2017

Circular references are conceptually nothing that should be checked for objects (or Sets or Maps) only, but applies to recursive structures in general, so move the seen checks into a position where it is part of the recursion itself.

Fixes: #14758

Tests are taken from #14775

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

@BridgeAR @TimothyGu @not-an-aardvark @aqrln

@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Aug 12, 2017
@addaleax
Copy link
Member Author

addaleax commented Aug 12, 2017

CI: https://ci.nodejs.org/job/node-test-commit/11725/

aqrln
aqrln approved these changes Aug 12, 2017
View reviewed changes
lib/util.js Outdated
Copy link
Contributor

@aqrln aqrln Aug 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny nit: const since the line is being changed anyway.

Copy link
Member Author

@addaleax addaleax Aug 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@addaleax addaleax force-pushed the util-inspect-map-recurse branch from f281467 to 013eca8 Compare August 12, 2017 19:28
@BridgeAR BridgeAR mentioned this pull request Aug 13, 2017
Closed
4 tasks
TimothyGu
TimothyGu approved these changes Aug 13, 2017
View reviewed changes
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a question.

lib/util.js Outdated
Copy link
Member

@TimothyGu TimothyGu Aug 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume value can only ever be an object here?

Copy link
Member Author

@addaleax addaleax Aug 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Or maybe that depends on what you mean by “object”; at least it’s not a primitive.

evanlucas
evanlucas approved these changes Aug 14, 2017
View reviewed changes
Copy link
Contributor

@evanlucas evanlucas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a nit (take it or leave it) :]

lib/util.js Outdated
Copy link
Contributor

@evanlucas evanlucas Aug 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason to add the else here? The diff would be smaller without it. Just a nit though

@BridgeAR BridgeAR mentioned this pull request Aug 17, 2017
Closed
4 tasks
addaleax and others added 2 commits August 17, 2017 20:17
@addaleax
util: use proper circular reference checking
0d019fa 
Circular references are conceptually nothing that should be checked
for objects (or Sets or Maps) only, but applies to recursive structures
in general, so move the `seen` checks into a position where it is part
of the recursion itself.

Fixes: nodejs#14758
PR-URL: nodejs#14790
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben.bridgewater@fintura.de>
@BridgeAR @addaleax
test: check util.inspect circular Set and Map refs
98c0a47 
Ref: nodejs#14775
PR-URL: nodejs#14790
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@addaleax addaleax force-pushed the util-inspect-map-recurse branch from 013eca8 to 98c0a47 Compare August 17, 2017 18:19
@addaleax
Copy link
Member Author

addaleax commented Aug 17, 2017 •
edited
Loading

Landed in 0d019fa, 98c0a47 859a228, f853baf

@addaleax addaleax closed this Aug 17, 2017
@addaleax addaleax deleted the util-inspect-map-recurse branch August 17, 2017 18:21
addaleax added a commit that referenced this pull request Aug 17, 2017
@addaleax
util: use proper circular reference checking
859a228 
Circular references are conceptually nothing that should be checked
for objects (or Sets or Maps) only, but applies to recursive structures
in general, so move the `seen` checks into a position where it is part
of the recursion itself.

Fixes: #14758
PR-URL: #14790
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben.bridgewater@fintura.de>
addaleax pushed a commit that referenced this pull request Aug 17, 2017
@BridgeAR @addaleax
test: check util.inspect circular Set and Map refs
f853baf 
Ref: #14775
PR-URL: #14790
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
@addaleax @MylesBorins
util: use proper circular reference checking
e97f7f3 
Circular references are conceptually nothing that should be checked
for objects (or Sets or Maps) only, but applies to recursive structures
in general, so move the `seen` checks into a position where it is part
of the recursion itself.

Fixes: #14758
PR-URL: #14790
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben.bridgewater@fintura.de>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
@BridgeAR @MylesBorins
test: check util.inspect circular Set and Map refs
c0e7344 
Ref: #14775
PR-URL: #14790
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
@addaleax @MylesBorins
util: use proper circular reference checking
c3c6cb1 
Circular references are conceptually nothing that should be checked
for objects (or Sets or Maps) only, but applies to recursive structures
in general, so move the `seen` checks into a position where it is part
of the recursion itself.

Fixes: #14758
PR-URL: #14790
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben.bridgewater@fintura.de>
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
@BridgeAR @MylesBorins
test: check util.inspect circular Set and Map refs
4f49ae5 
Ref: #14775
PR-URL: #14790
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins
Copy link
Contributor

MylesBorins commented Sep 20, 2017

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

@tniessen tniessen tniessen approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

+5 more reviewers

@refack refack refack approved these changes

@evanlucas evanlucas evanlucas approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@hiroppy hiroppy hiroppy approved these changes

@aqrln aqrln aqrln approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

util Issues and PRs related to the built-in util module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.