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

util: fix circular ref for Set and Map #14775

Closed
wants to merge 1 commit into from
Closed

util: fix circular ref for Set and Map #14775

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions lib/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -738,7 +738,9 @@ function formatSet(ctx, value, recurseTimes, visibleKeys, keys) {
var output = [];
value.forEach(function(v) {
var nextRecurseTimes = recurseTimes === null ? null : recurseTimes - 1;
var str = formatValue(ctx, v, nextRecurseTimes);
var str = ctx.seen.includes(v) ?
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

See #14775 (comment)

ctx.stylize('[Circular]', 'special') :
formatValue(ctx, v, nextRecurseTimes);
Copy link
Member

Choose a reason for hiding this comment

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

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');)?

Copy link
Member

Choose a reason for hiding this comment

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

If not: Can we DRY this up a bit, maybe add a function for it or something?

Copy link
Member

Choose a reason for hiding this comment

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

@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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@addaleax That would be a semver-major change as the ctx is passed to custom inspection functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I’ll just open a PR and we can discuss from there.

output.push(str);
});
for (var n = 0; n < keys.length; n++) {
Expand All @@ -753,9 +755,13 @@ function formatMap(ctx, value, recurseTimes, visibleKeys, keys) {
var output = [];
value.forEach(function(v, k) {
var nextRecurseTimes = recurseTimes === null ? null : recurseTimes - 1;
var str = formatValue(ctx, k, nextRecurseTimes);
var str = ctx.seen.includes(k) ?
ctx.stylize('[Circular]', 'special') :
formatValue(ctx, k, nextRecurseTimes);
str += ' => ';
str += formatValue(ctx, v, nextRecurseTimes);
str += ctx.seen.includes(v) ?
ctx.stylize('[Circular]', 'special') :
formatValue(ctx, v, nextRecurseTimes);
output.push(str);
});
for (var n = 0; n < keys.length; n++) {
Expand Down
19 changes: 19 additions & 0 deletions test/parallel/test-util-inspect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -783,6 +783,13 @@ if (typeof Symbol !== 'undefined') {
);
}

// Test circular Set
{
const set = new Set();
set.add(set);
assert.strictEqual(util.inspect(set), 'Set { [Circular] }');
}

// test Map
{
assert.strictEqual(util.inspect(new Map()), 'Map {}');
Expand All @@ -794,6 +801,18 @@ if (typeof Symbol !== 'undefined') {
'Map { \'foo\' => null, [size]: 1, bar: 42 }');
}

// Test circular Map
{
const map = new Map();
map.set(map, 'map');
assert.strictEqual(util.inspect(map), "Map { [Circular] => 'map' }");
map.set(map, map);
assert.strictEqual(util.inspect(map), 'Map { [Circular] => [Circular] }');
map.delete(map);
map.set('map', map);
assert.strictEqual(util.inspect(map), "Map { 'map' => [Circular] }");
}

// test Promise
{
const resolved = Promise.resolve(3);
Expand Down