util: fix circular ref for Set and Map #14775
Conversation
nodejs-github-bot
added
the
util
| var str = formatValue(ctx, v, nextRecurseTimes); | ||
| var str = ctx.seen.includes(v) ? | ||
| ctx.stylize('[Circular]', 'special') : | ||
| formatValue(ctx, v, nextRecurseTimes); |
There was a problem hiding this comment.
I don’t know if you tried that, but wouldn’t it be better to do this type of check/early return in formatValue, before the ctx.seen.push() call (i.e. if (ctx.seen.includes(v)) return ctx.stylize('[Circular]', 'special');)?
There was a problem hiding this comment.
If not: Can we DRY this up a bit, maybe add a function for it or something?
There was a problem hiding this comment.
@not-an-aardvark @BridgeAR @aqrln This is what I was having in mind:
diff in the fold
diff --git a/lib/util.js b/lib/util.js
index a9f267bd416d..9b8e8395ffac 100644
--- a/lib/util.js
+++ b/lib/util.js
@@ -216,7 +216,7 @@ function debuglog(set) {
function inspect(obj, opts) {
// default options
var ctx = {
- seen: [],
+ seen: new Set(),
stylize: stylizeNoColor
};
// legacy...
@@ -613,11 +613,16 @@ function formatValue(ctx, value, recurseTimes) {
}
}
- ctx.seen.push(value);
+ var output;
+ if (ctx.seen.has(value)) {
+ output = ctx.stylize('[Circular]', 'special');
+ } else {
+ ctx.seen.push(value);
- var output = formatter(ctx, value, recurseTimes, visibleKeys, keys);
+ output = formatter(ctx, value, recurseTimes, visibleKeys, keys);
- ctx.seen.pop();
+ ctx.seen.pop();
+ }
return reduceToSingleString(output, base, braces, ctx.breakLength);
}
@@ -822,21 +827,17 @@ function formatProperty(ctx, value, recurseTimes, visibleKeys, key, array) {
}
}
if (!str) {
- if (ctx.seen.indexOf(desc.value) < 0) {
- if (recurseTimes === null) {
- str = formatValue(ctx, desc.value, null);
+ if (recurseTimes === null) {
+ str = formatValue(ctx, desc.value, null);
+ } else {
+ str = formatValue(ctx, desc.value, recurseTimes - 1);
+ }
+ if (str.indexOf('\n') > -1) {
+ if (array) {
+ str = str.replace(/\n/g, '\n ');
} else {
- str = formatValue(ctx, desc.value, recurseTimes - 1);
+ str = str.replace(/^|\n/g, '\n ');
}
- if (str.indexOf('\n') > -1) {
- if (array) {
- str = str.replace(/\n/g, '\n ');
- } else {
- str = str.replace(/^|\n/g, '\n ');
- }
- }
- } else {
- str = ctx.stylize('[Circular]', 'special');
}
}
if (name === undefined) {WDYT?
There was a problem hiding this comment.
I am aware of that (ref #14492 (comment)) but I would rather keep the concerns separated.
And I am actually working on improving the inspect performance in general at the moment and I think it would be best to keep the performance changes in that one but I did not yet have enough time to finish it.
There was a problem hiding this comment.
@addaleax That would be a semver-major change as the ctx is passed to custom inspection functions.
There was a problem hiding this comment.
One non-breaking workaround would be to continue to make context.seen an array and create a new Set internally, and use the Set for all membership checks. That way we would sill avoid linear-time membership checks.
There was a problem hiding this comment.
I’ll just open a PR and we can discuss from there.
| value.forEach(function(v) { | ||
| var nextRecurseTimes = recurseTimes === null ? null : recurseTimes - 1; | ||
| var str = formatValue(ctx, v, nextRecurseTimes); | ||
| var str = ctx.seen.includes(v) ? |
There was a problem hiding this comment.
This isn't directly related to this PR, but why is ctx.seen an array? Would it be better to use a Set/WeakSet to avoid linear-time includes checks?
|
Closing in favor of #14790 |
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>
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>
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>
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>
6 participants
Fixes #14758
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
util