From b171fa253007d20f985ea9504adcffbc73db1373 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Fri, 25 May 2018 12:05:39 +0200 Subject: [PATCH] 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 Reviewed-By: Anna Henningsen --- lib/console.js | 9 ++- lib/util.js | 98 ++++++++++++++++-------------- src/node_util.cc | 3 + test/parallel/test-util-inspect.js | 49 ++++++++------- 4 files changed, 89 insertions(+), 70 deletions(-) diff --git a/lib/console.js b/lib/console.js index 98ab0c35712d97..7fef9e0a7297e8 100644 --- a/lib/console.js +++ b/lib/console.js @@ -360,8 +360,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 = []; @@ -391,7 +394,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 8160d802b0332f..b427895ebd5c44 100644 --- a/lib/util.js +++ b/lib/util.js @@ -115,6 +115,10 @@ const meta = [ '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '\\\\' ]; +// Constants to map the iterator state. +const kWeak = 0; +const kIterator = 1; +const kMapEntries = 2; const escapeFn = (str) => meta[str.charCodeAt(0)]; @@ -982,77 +986,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 9f31786b32557f..41b1307bb4912c 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -57,6 +57,9 @@ static void PreviewEntries(const FunctionCallbackInfo& args) { Local entries; if (!args[0].As()->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 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 708eccee273478..183fbb16e39c9e 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. @@ -965,18 +967,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. @@ -992,7 +995,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. @@ -1413,17 +1416,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 @@ -1437,17 +1440,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.