util: improve display of iterators and weak entries

This adds the number of not visible elements when inspecting iterators
while exceeding `maxArrayLength`.
It also fixes a edge case with `maxArrayLength` and the map.entries()
iterator. Now the whole entry will be visible instead of only the key
but not the value of the first entry.
Besides that it uses a slighly better algorithm that improves the
performance by skipping unnecessary steps.

PR-URL: https://github.com/nodejs/node/pull/20961
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

4 files changed, 89 insertions, 70 deletions

diff --git a/lib/console.js b/lib/console.js
index 665a2a6a23..a5b2265f58 100644
--- a/lib/console.js
+++ b/lib/console.js
@@ -367,8 +367,11 @@ Console.prototype.table = function(tabularData, properties) {
   const mapIter = isMapIterator(tabularData);
   let isKeyValue = false;
   let i = 0;
-  if (mapIter)
-    [ tabularData, isKeyValue ] = previewEntries(tabularData);
+  if (mapIter) {
+    const res = previewEntries(tabularData, true);
+    tabularData = res[0];
+    isKeyValue = res[1];
+  }
 
   if (isKeyValue || isMap(tabularData)) {
     const keys = [];
@@ -398,7 +401,7 @@ Console.prototype.table = function(tabularData, properties) {
 
   const setIter = isSetIterator(tabularData);
   if (setIter)
-    [ tabularData ] = previewEntries(tabularData);
+    tabularData = previewEntries(tabularData);
 
   const setlike = setIter || (mapIter && !isKeyValue) || isSet(tabularData);
   if (setlike) {
diff --git a/lib/util.js b/lib/util.js
index c56b8429f4..3c4fdbb128 100644
--- a/lib/util.js
+++ b/lib/util.js
@@ -118,6 +118,10 @@ const meta = [
   '', '', '', '', '', '', '', '', '', '',
   '', '', '', '', '', '', '', '\\\\'
 ];
+// Constants to map the iterator state.
+const kWeak = 0;
+const kIterator = 1;
+const kMapEntries = 2;
 
 function addQuotes(str, quotes) {
   if (quotes === -1) {
@@ -1017,77 +1021,83 @@ function formatMap(ctx, value, recurseTimes, keys) {
   return output;
 }
 
-function formatWeakSet(ctx, value, recurseTimes, keys) {
+function formatSetIterInner(ctx, value, recurseTimes, keys, entries, state) {
   const maxArrayLength = Math.max(ctx.maxArrayLength, 0);
-  const [ entries ] = previewEntries(value).slice(0, maxArrayLength + 1);
   const maxLength = Math.min(maxArrayLength, entries.length);
   let output = new Array(maxLength);
   for (var i = 0; i < maxLength; ++i)
     output[i] = formatValue(ctx, entries[i], recurseTimes);
-  // Sort all entries to have a halfway reliable output (if more entries than
-  // retrieved ones exist, we can not reliably return the same output).
-  output = output.sort();
-  if (entries.length > maxArrayLength)
-    output.push('... more items');
+  if (state === kWeak) {
+    // Sort all entries to have a halfway reliable output (if more entries than
+    // retrieved ones exist, we can not reliably return the same output).
+    output = output.sort();
+  }
+  const remaining = entries.length - maxLength;
+  if (remaining > 0) {
+    output.push(`... ${remaining} more item${remaining > 1 ? 's' : ''}`);
+  }
   for (i = 0; i < keys.length; i++)
     output.push(formatProperty(ctx, value, recurseTimes, keys[i], 0));
   return output;
 }
 
-function formatWeakMap(ctx, value, recurseTimes, keys) {
+function formatMapIterInner(ctx, value, recurseTimes, keys, entries, state) {
   const maxArrayLength = Math.max(ctx.maxArrayLength, 0);
-  const [ entries ] = previewEntries(value).slice(0, (maxArrayLength + 1) * 2);
   // Entries exist as [key1, val1, key2, val2, ...]
-  const remainder = entries.length / 2 > maxArrayLength;
-  const len = entries.length / 2 - (remainder ? 1 : 0);
+  const len = entries.length / 2;
+  const remaining = len - maxArrayLength;
   const maxLength = Math.min(maxArrayLength, len);
   let output = new Array(maxLength);
-  for (var i = 0; i < maxLength; i++) {
+  let start = '';
+  let end = '';
+  let middle = ' => ';
+  let i = 0;
+  if (state === kMapEntries) {
+    start = '[ ';
+    end = ' ]';
+    middle = ', ';
+  }
+  for (; i < maxLength; i++) {
     const pos = i * 2;
-    output[i] = `${formatValue(ctx, entries[pos], recurseTimes)} => ` +
-      formatValue(ctx, entries[pos + 1], recurseTimes);
+    output[i] = `${start}${formatValue(ctx, entries[pos], recurseTimes)}` +
+      `${middle}${formatValue(ctx, entries[pos + 1], recurseTimes)}${end}`;
+  }
+  if (state === kWeak) {
+    // Sort all entries to have a halfway reliable output (if more entries
+    // than retrieved ones exist, we can not reliably return the same output).
+    output = output.sort();
+  }
+  if (remaining > 0) {
+    output.push(`... ${remaining} more item${remaining > 1 ? 's' : ''}`);
   }
-  // Sort all entries to have a halfway reliable output (if more entries than
-  // retrieved ones exist, we can not reliably return the same output).
-  output = output.sort();
-  if (remainder > 0)
-    output.push('... more items');
   for (i = 0; i < keys.length; i++)
     output.push(formatProperty(ctx, value, recurseTimes, keys[i], 0));
   return output;
 }
 
-function zip2(list) {
-  const ret = Array(list.length / 2);
-  for (var i = 0; i < ret.length; ++i)
-    ret[i] = [list[2 * i], list[2 * i + 1]];
-  return ret;
+function formatWeakSet(ctx, value, recurseTimes, keys) {
+  const entries = previewEntries(value);
+  return formatSetIterInner(ctx, value, recurseTimes, keys, entries, kWeak);
 }
 
-function formatCollectionIterator(ctx, value, recurseTimes, keys) {
-  const output = [];
-  var [ entries, isKeyValue ] = previewEntries(value);
-  if (isKeyValue)
-    entries = zip2(entries);
-  for (const entry of entries) {
-    if (ctx.maxArrayLength === output.length) {
-      output.push('... more items');
-      break;
-    }
-    output.push(formatValue(ctx, entry, recurseTimes));
-  }
-  for (var n = 0; n < keys.length; n++) {
-    output.push(formatProperty(ctx, value, recurseTimes, keys[n], 0));
-  }
-  return output;
+function formatWeakMap(ctx, value, recurseTimes, keys) {
+  const entries = previewEntries(value);
+  return formatMapIterInner(ctx, value, recurseTimes, keys, entries, kWeak);
 }
 
-function formatMapIterator(ctx, value, recurseTimes, keys) {
-  return formatCollectionIterator(ctx, value, recurseTimes, keys);
+function formatSetIterator(ctx, value, recurseTimes, keys) {
+  const entries = previewEntries(value);
+  return formatSetIterInner(ctx, value, recurseTimes, keys, entries, kIterator);
 }
 
-function formatSetIterator(ctx, value, recurseTimes, keys) {
-  return formatCollectionIterator(ctx, value, recurseTimes, keys);
+function formatMapIterator(ctx, value, recurseTimes, keys) {
+  const [entries, isKeyValue] = previewEntries(value, true);
+  if (isKeyValue) {
+    return formatMapIterInner(
+      ctx, value, recurseTimes, keys, entries, kMapEntries);
+  }
+
+  return formatSetIterInner(ctx, value, recurseTimes, keys, entries, kIterator);
 }
 
 function formatPromise(ctx, value, recurseTimes, keys) {
diff --git a/src/node_util.cc b/src/node_util.cc
index 9f31786b32..41b1307bb4 100644
--- a/src/node_util.cc
+++ b/src/node_util.cc
@@ -57,6 +57,9 @@ static void PreviewEntries(const FunctionCallbackInfo<Value>& args) {
   Local<Array> entries;
   if (!args[0].As<Object>()->PreviewEntries(&is_key_value).ToLocal(&entries))
     return;
+  // Fast path for WeakMap, WeakSet and Set iterators.
+  if (args.Length() == 1)
+    return args.GetReturnValue().Set(entries);
   Local<Array> ret = Array::New(env->isolate(), 2);
   ret->Set(env->context(), 0, entries).FromJust();
   ret->Set(env->context(), 1, v8::Boolean::New(env->isolate(), is_key_value))
diff --git a/test/parallel/test-util-inspect.js b/test/parallel/test-util-inspect.js
index 0dc3f11bfe..c2b26fd5a4 100644
--- a/test/parallel/test-util-inspect.js
+++ b/test/parallel/test-util-inspect.js
@@ -447,13 +447,15 @@ assert.strictEqual(util.inspect(-5e-324), '-5e-324');
 {
   const map = new Map();
   map.set(1, 2);
-  const [ vals ] = previewEntries(map.entries());
-  const valsOutput = [];
-  for (const o of vals) {
-    valsOutput.push(o);
-  }
-
-  assert.strictEqual(util.inspect(valsOutput), '[ 1, 2 ]');
+  // Passing only a single argument to indicate a set iterator.
+  const valsSetIterator = previewEntries(map.entries());
+  // Passing through true to indicate a map iterator.
+  const valsMapIterEntries = previewEntries(map.entries(), true);
+  const valsMapIterKeys = previewEntries(map.keys(), true);
+
+  assert.strictEqual(util.inspect(valsSetIterator), '[ 1, 2 ]');
+  assert.strictEqual(util.inspect(valsMapIterEntries), '[ [ 1, 2 ], true ]');
+  assert.strictEqual(util.inspect(valsMapIterKeys), '[ [ 1 ], false ]');
 }
 
 // Test for other constructors in different context.
@@ -913,18 +915,19 @@ if (typeof Symbol !== 'undefined') {
 // Test Map iterators.
 {
   const map = new Map([['foo', 'bar']]);
-  assert.strictEqual(util.inspect(map.keys()), "[Map Iterator] { 'foo' }");
-  assert.strictEqual(util.inspect(map.values()), "[Map Iterator] { 'bar' }");
-  assert.strictEqual(util.inspect(map.entries()),
-                     "[Map Iterator] { [ 'foo', 'bar' ] }");
+  assert.strictEqual(util.inspect(map.keys()), '[Map Iterator] { \'foo\' }');
+  assert.strictEqual(util.inspect(map.values()), '[Map Iterator] { \'bar\' }');
+  map.set('A', 'B!');
+  assert.strictEqual(util.inspect(map.entries(), { maxArrayLength: 1 }),
+                     "[Map Iterator] { [ 'foo', 'bar' ], ... 1 more item }");
   // Make sure the iterator doesn't get consumed.
   const keys = map.keys();
-  assert.strictEqual(util.inspect(keys), "[Map Iterator] { 'foo' }");
-  assert.strictEqual(util.inspect(keys), "[Map Iterator] { 'foo' }");
+  assert.strictEqual(util.inspect(keys), "[Map Iterator] { 'foo', 'A' }");
+  assert.strictEqual(util.inspect(keys), "[Map Iterator] { 'foo', 'A' }");
   keys.extra = true;
   assert.strictEqual(
     util.inspect(keys, { maxArrayLength: 0 }),
-    '[Map Iterator] { ... more items, extra: true }');
+    '[Map Iterator] { ... 2 more items, extra: true }');
 }
 
 // Test Set iterators.
@@ -940,7 +943,7 @@ if (typeof Symbol !== 'undefined') {
   keys.extra = true;
   assert.strictEqual(
     util.inspect(keys, { maxArrayLength: 1 }),
-    '[Set Iterator] { 1, ... more items, extra: true }');
+    '[Set Iterator] { 1, ... 1 more item, extra: true }');
 }
 
 // Test alignment of items in container.
@@ -1364,17 +1367,17 @@ util.inspect(process);
   assert.strictEqual(out, expect);
 
   out = util.inspect(weakMap, { maxArrayLength: 0, showHidden: true });
-  expect = 'WeakMap { ... more items }';
+  expect = 'WeakMap { ... 2 more items }';
   assert.strictEqual(out, expect);
 
   weakMap.extra = true;
   out = util.inspect(weakMap, { maxArrayLength: 1, showHidden: true });
   // It is not possible to determine the output reliable.
-  expect = 'WeakMap { [ [length]: 0 ] => {}, ... more items, extra: true }';
-  const expectAlt = 'WeakMap { {} => [ [length]: 0 ], ... more items, ' +
+  expect = 'WeakMap { [ [length]: 0 ] => {}, ... 1 more item, extra: true }';
+  const expectAlt = 'WeakMap { {} => [ [length]: 0 ], ... 1 more item, ' +
                     'extra: true }';
   assert(out === expect || out === expectAlt,
-         `Found "${out}" rather than "${expect}" or "${expectAlt}"`);
+         `Found: "${out}"\nrather than: "${expect}"\nor: "${expectAlt}"`);
 }
 
 { // Test WeakSet
@@ -1388,17 +1391,17 @@ util.inspect(process);
   assert.strictEqual(out, expect);
 
   out = util.inspect(weakSet, { maxArrayLength: -2, showHidden: true });
-  expect = 'WeakSet { ... more items }';
+  expect = 'WeakSet { ... 2 more items }';
   assert.strictEqual(out, expect);
 
   weakSet.extra = true;
   out = util.inspect(weakSet, { maxArrayLength: 1, showHidden: true });
   // It is not possible to determine the output reliable.
-  expect = 'WeakSet { {}, ... more items, extra: true }';
-  const expectAlt = 'WeakSet { [ 1, [length]: 1 ], ... more items, ' +
+  expect = 'WeakSet { {}, ... 1 more item, extra: true }';
+  const expectAlt = 'WeakSet { [ 1, [length]: 1 ], ... 1 more item, ' +
                     'extra: true }';
   assert(out === expect || out === expectAlt,
-         `Found "${out}" rather than "${expect}" or "${expectAlt}"`);
+         `Found: "${out}"\nrather than: "${expect}"\nor: "${expectAlt}"`);
 }
 
 { // Test argument objects.