util: restore all information in inspect

The former implementation lacked symbols on the iterator objects without prototype. This is now fixed. The special handling for overriding `Symbol.iterator` was removed as it's very difficult to deal with this properly. Manipulating the symbols is just not supported. PR-URL: #22437 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
nodejs · Sep 3, 2018 · 5b14066 · 5b14066
1 parent fcf059a
commit 5b14066
Show file tree

Hide file tree

Showing 2 changed files with 45 additions and 59 deletions.
diff --git a/lib/util.js b/lib/util.js
@@ -462,13 +462,6 @@ function getPrefix(constructor, tag, fallback) {
   return '';
 }
 
-function addExtraKeys(source, target, keys) {
-  for (const key of keys) {
-    target[key] = source[key];
-  }
-  return target;
-}
-
 function findTypedConstructor(value) {
   for (const [check, clazz] of [
     [isUint8Array, Uint8Array],
@@ -484,14 +477,36 @@ function findTypedConstructor(value) {
     [isBigUint64Array, BigUint64Array]
   ]) {
     if (check(value)) {
-      return new clazz(value);
+      return clazz;
     }
   }
-  return value;
 }
 
 const getBoxedValue = formatPrimitive.bind(null, stylizeNoColor);
 
+function noPrototypeIterator(ctx, value, recurseTimes) {
+  let newVal;
+  // TODO: Create a Subclass in case there's no prototype and show
+  // `null-prototype`.
+  if (isSet(value)) {
+    const clazz = Object.getPrototypeOf(value) || Set;
+    newVal = new clazz(setValues(value));
+  } else if (isMap(value)) {
+    const clazz = Object.getPrototypeOf(value) || Map;
+    newVal = new clazz(mapEntries(value));
+  } else if (Array.isArray(value)) {
+    const clazz = Object.getPrototypeOf(value) || Array;
+    newVal = new clazz(value.length || 0);
+  } else if (isTypedArray(value)) {
+    const clazz = findTypedConstructor(value) || Uint8Array;
+    newVal = new clazz(value);
+  }
+  if (newVal) {
+    Object.defineProperties(newVal, Object.getOwnPropertyDescriptors(value));
+    return formatValue(ctx, newVal, recurseTimes);
+  }
+}
+
 // Note: using `formatValue` directly requires the indentation level to be
 // corrected by setting `ctx.indentationLvL += diff` and then to decrease the
 // value afterwards again.
@@ -757,39 +772,25 @@ function formatValue(ctx, value, recurseTimes) {
       braces = ['{', '}'];
     // The input prototype got manipulated. Special handle these.
     // We have to rebuild the information so we are able to display everything.
-    } else if (isSet(value)) {
-      const newVal = addExtraKeys(value, new Set(setValues(value)), keys);
-      return formatValue(ctx, newVal, recurseTimes);
-    } else if (isMap(value)) {
-      const newVal = addExtraKeys(value, new Map(mapEntries(value)), keys);
-      return formatValue(ctx, newVal, recurseTimes);
-    } else if (Array.isArray(value)) {
-      // The prefix is not always possible to fully reconstruct.
-      const prefix = getPrefix(constructor, tag);
-      braces = [`${prefix === 'Array ' ? '' : prefix}[`, ']'];
-      formatter = formatArray;
-      const newValue = [];
-      newValue.length = value.length;
-      value = addExtraKeys(value, newValue, keys);
-    } else if (isTypedArray(value)) {
-      const newValue = findTypedConstructor(value);
-      value = addExtraKeys(value, newValue, keys.slice(newValue.length));
-      // The prefix is not always possible to fully reconstruct.
-      braces = [`${getPrefix(getConstructorName(value), tag)}[`, ']'];
-      formatter = formatTypedArray;
-    } else if (isMapIterator(value)) {
-      braces = [`[${tag || 'Map Iterator'}] {`, '}'];
-      formatter = formatMapIterator;
-    } else if (isSetIterator(value)) {
-      braces = [`[${tag || 'Set Iterator'}] {`, '}'];
-      formatter = formatSetIterator;
-    // Handle other regular objects again.
-    } else if (keyLength === 0) {
-      if (isExternal(value))
-        return ctx.stylize('[External]', 'special');
-      return `${getPrefix(constructor, tag)}{}`;
     } else {
-      braces[0] = `${getPrefix(constructor, tag)}{`;
+      const specialIterator = noPrototypeIterator(ctx, value, recurseTimes);
+      if (specialIterator) {
+        return specialIterator;
+      }
+      if (isMapIterator(value)) {
+        braces = [`[${tag || 'Map Iterator'}] {`, '}'];
+        formatter = formatMapIterator;
+      } else if (isSetIterator(value)) {
+        braces = [`[${tag || 'Set Iterator'}] {`, '}'];
+        formatter = formatSetIterator;
+      // Handle other regular objects again.
+      } else if (keyLength === 0) {
+        if (isExternal(value))
+          return ctx.stylize('[External]', 'special');
+        return `${getPrefix(constructor, tag)}{}`;
+      } else {
+        braces[0] = `${getPrefix(constructor, tag)}{`;
+      }
     }
   }
 

diff --git a/test/parallel/test-util-inspect.js b/test/parallel/test-util-inspect.js
@@ -1600,24 +1600,6 @@ util.inspect(process);
     'prematurely. Maximum call stack size exceeded.]'));
 }
 
-// Manipulating the Symbol.iterator should still produce nice results.
-[
-  [[1, 2], '[ 1, 2 ]'],
-  [[, , 5, , , , ], '[ <2 empty items>, 5, <3 empty items> ]'],
-  [new Set([1, 2]), 'Set { 1, 2 }'],
-  [new Map([[1, 2]]), 'Map { 1 => 2 }'],
-  [new Uint8Array(2), 'Uint8Array [ 0, 0 ]'],
-  // It seems like the following can not be fully restored :(
-  [new Set([1, 2]).entries(), 'Object [Set Iterator] {}'],
-  [new Map([[1, 2]]).keys(), 'Object [Map Iterator] {}'],
-].forEach(([value, expected]) => {
-  // "Remove the Symbol.iterator"
-  Object.defineProperty(value, Symbol.iterator, {
-    value: false
-  });
-  assert.strictEqual(util.inspect(value), expected);
-});
-
 // Verify the output in case the value has no prototype.
 // Sadly, these cases can not be fully inspected :(
 [
@@ -1673,6 +1655,9 @@ util.inspect(process);
   );
   value.foo = 'bar';
   assert.notStrictEqual(util.inspect(value), expected);
+  delete value.foo;
+  value[Symbol('foo')] = 'yeah';
+  assert.notStrictEqual(util.inspect(value), expected);
 });
 
 assert.strictEqual(inspect(1n), '1n');