util: correctly inspect Map/Set Iterators

Previously, a MapIterator or SetIterator would
not be inspected properly. This change makes it possible
to inspect them by creating a Debug Mirror and previewing
the iterators to not consume the actual iterator that
we are trying to inspect.

This change also adds a node_util binding that uses
v8's Value::IsSetIterator and Value::IsMapIterator
to verify that the values passed in are actual iterators.

Fixes: https://github.com/nodejs/node/issues/3107
PR-URL: https://github.com/nodejs/node/pull/3119
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

4 files changed, 89 insertions, 5 deletions

diff --git a/lib/util.js b/lib/util.js
index 401a0ed3c7..41c5b445cb 100644
--- a/lib/util.js
+++ b/lib/util.js
@@ -3,6 +3,7 @@
 const uv = process.binding('uv');
 const Buffer = require('buffer').Buffer;
 const internalUtil = require('internal/util');
+const binding = process.binding('util');
 
 var Debug;
 var ObjectIsPromise;
@@ -323,11 +324,23 @@ function formatValue(ctx, value, recurseTimes) {
       braces = ['{', '}'];
       formatter = formatPromise;
     } else {
-      if (constructor === Object)
-        constructor = null;
-      braces = ['{', '}'];
-      empty = true;  // No other data than keys.
-      formatter = formatObject;
+      if (binding.isMapIterator(value)) {
+        constructor = { name: 'MapIterator' };
+        braces = ['{', '}'];
+        empty = false;
+        formatter = formatCollectionIterator;
+      } else if (binding.isSetIterator(value)) {
+        constructor = { name: 'SetIterator' };
+        braces = ['{', '}'];
+        empty = false;
+        formatter = formatCollectionIterator;
+      } else {
+        if (constructor === Object)
+          constructor = null;
+        braces = ['{', '}'];
+        empty = true;  // No other data than keys.
+        formatter = formatObject;
+      }
     }
   }
 
@@ -501,6 +514,18 @@ function formatMap(ctx, value, recurseTimes, visibleKeys, keys) {
   return output;
 }
 
+function formatCollectionIterator(ctx, value, recurseTimes, visibleKeys, keys) {
+  ensureDebugIsInitialized();
+  const mirror = Debug.MakeMirror(value, true);
+  var nextRecurseTimes = recurseTimes === null ? null : recurseTimes - 1;
+  var vals = mirror.preview();
+  var output = [];
+  for (let o of vals) {
+    output.push(formatValue(ctx, o, nextRecurseTimes));
+  }
+  return output;
+}
+
 function formatPromise(ctx, value, recurseTimes, visibleKeys, keys) {
   var output = [];
   var internals = inspectPromise(value);
diff --git a/node.gyp b/node.gyp
index 9575f4cbcd..ece9eeec2c 100644
--- a/node.gyp
+++ b/node.gyp
@@ -115,6 +115,7 @@
         'src/node_javascript.cc',
         'src/node_main.cc',
         'src/node_os.cc',
+        'src/node_util.cc',
         'src/node_v8.cc',
         'src/node_stat_watcher.cc',
         'src/node_watchdog.cc',
diff --git a/src/node_util.cc b/src/node_util.cc
new file mode 100644
index 0000000000..cc86478e09
--- /dev/null
+++ b/src/node_util.cc
@@ -0,0 +1,38 @@
+#include "node.h"
+#include "v8.h"
+#include "env.h"
+#include "env-inl.h"
+
+namespace node {
+namespace util {
+
+using v8::Context;
+using v8::FunctionCallbackInfo;
+using v8::Local;
+using v8::Object;
+using v8::Value;
+
+static void IsMapIterator(const FunctionCallbackInfo<Value>& args) {
+  CHECK_EQ(1, args.Length());
+  args.GetReturnValue().Set(args[0]->IsMapIterator());
+}
+
+
+static void IsSetIterator(const FunctionCallbackInfo<Value>& args) {
+  CHECK_EQ(1, args.Length());
+  args.GetReturnValue().Set(args[0]->IsSetIterator());
+}
+
+
+void Initialize(Local<Object> target,
+                Local<Value> unused,
+                Local<Context> context) {
+  Environment* env = Environment::GetCurrent(context);
+  env->SetMethod(target, "isMapIterator", IsMapIterator);
+  env->SetMethod(target, "isSetIterator", IsSetIterator);
+}
+
+}  // namespace util
+}  // namespace node
+
+NODE_MODULE_CONTEXT_AWARE_BUILTIN(util, node::util::Initialize)
diff --git a/test/parallel/test-util-inspect.js b/test/parallel/test-util-inspect.js
index aa6c764491..a706f49599 100644
--- a/test/parallel/test-util-inspect.js
+++ b/test/parallel/test-util-inspect.js
@@ -287,6 +287,26 @@ global.Promise = function() { this.bar = 42; };
 assert.equal(util.inspect(new Promise()), '{ bar: 42 }');
 global.Promise = oldPromise;
 
+// Map/Set Iterators
+var m = new Map([['foo', 'bar']]);
+assert.strictEqual(util.inspect(m.keys()), 'MapIterator { \'foo\' }');
+assert.strictEqual(util.inspect(m.values()), 'MapIterator { \'bar\' }');
+assert.strictEqual(util.inspect(m.entries()),
+                   'MapIterator { [ \'foo\', \'bar\' ] }');
+// make sure the iterator doesn't get consumed
+var keys = m.keys();
+assert.strictEqual(util.inspect(keys), 'MapIterator { \'foo\' }');
+assert.strictEqual(util.inspect(keys), 'MapIterator { \'foo\' }');
+
+var s = new Set([1, 3]);
+assert.strictEqual(util.inspect(s.keys()), 'SetIterator { 1, 3 }');
+assert.strictEqual(util.inspect(s.values()), 'SetIterator { 1, 3 }');
+assert.strictEqual(util.inspect(s.entries()),
+                   'SetIterator { [ 1, 1 ], [ 3, 3 ] }');
+// make sure the iterator doesn't get consumed
+keys = s.keys();
+assert.strictEqual(util.inspect(keys), 'SetIterator { 1, 3 }');
+assert.strictEqual(util.inspect(keys), 'SetIterator { 1, 3 }');
 
 // Test alignment of items in container
 // Assumes that the first numeric character is the start of an item.