util: fix circular ref for Set and Map

[BridgeAR]

Fixes #14758

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

util

[addaleax]

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

[addaleax]

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

[addaleax]

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

[BridgeAR]

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.

[TimothyGu]

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

[not-an-aardvark]

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.

[addaleax]

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

[not-an-aardvark]

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?

[BridgeAR]

See #14775 (comment)

[BridgeAR]

Closing in favor of #14790