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

util: use proper circular reference checking #14790

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

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

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

var output = formatter(ctx, value, recurseTimes, visibleKeys, keys);
var output = formatter(ctx, value, recurseTimes, visibleKeys, keys);
Copy link
Contributor

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

Choose a reason for hiding this comment

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

Done!

@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
@@ -613,13 +613,18 @@ function formatValue(ctx, value, recurseTimes) {
}
}

ctx.seen.push(value);
// TODO(addaleax): Make `seen` a Set to avoid linear-time lookup.
if (ctx.seen.includes(value)) {
Copy link
Member

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

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
// TODO(addaleax): Make `seen` a Set to avoid linear-time lookup.
if (ctx.seen.includes(value)) {
return ctx.stylize('[Circular]', 'special');
} else {
Copy link
Contributor

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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@BridgeAR @addaleax
test: check util.inspect circular Set and Map refs
98c0a47 
Ref: nodejs#14775
PR-URL: nodejs#14790
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins
Copy link
Contributor

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

@refack refack refack approved these changes

@jasnell jasnell jasnell approved these changes

@evanlucas evanlucas evanlucas approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@cjihrig cjihrig cjihrig approved these changes

@tniessen tniessen tniessen approved these changes

@hiroppy hiroppy hiroppy approved these changes

@aqrln aqrln aqrln approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

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.