-
Notifications
You must be signed in to change notification settings - Fork 30.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
util: fix inspecting of proxy objects #6465
Changes from all commits
86daa71
ec16e91
b2ebb7b
f630602
6115df5
842a8e4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
'use strict'; | ||
|
||
const util = require('util'); | ||
const common = require('../common.js'); | ||
|
||
const bench = common.createBenchmark(main, { | ||
v: [1, 2], | ||
n: [1e6] | ||
}); | ||
|
||
function twoDifferentProxies(n) { | ||
// This one should be slower between we're looking up multiple proxies. | ||
const proxyA = new Proxy({}, {get: () => {}}); | ||
const proxyB = new Proxy({}, {get: () => {}}); | ||
bench.start(); | ||
for (var i = 0; i < n; i += 1) | ||
util.inspect({a: proxyA, b: proxyB}, {showProxy: true}); | ||
bench.end(n); | ||
} | ||
|
||
function oneProxy(n) { | ||
// This one should be a bit faster because of the internal caching. | ||
const proxy = new Proxy({}, {get: () => {}}); | ||
bench.start(); | ||
for (var i = 0; i < n; i += 1) | ||
util.inspect({a: proxy, b: proxy}, {showProxy: true}); | ||
bench.end(n); | ||
} | ||
|
||
function main(conf) { | ||
const n = conf.n | 0; | ||
const v = conf.v | 0; | ||
|
||
switch (v) { | ||
case 1: | ||
oneProxy(n); | ||
break; | ||
case 2: | ||
twoDifferentProxies(n); | ||
break; | ||
default: | ||
throw new Error('Should not get to here'); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -139,6 +139,7 @@ function inspect(obj, opts) { | |
if (ctx.depth === undefined) ctx.depth = 2; | ||
if (ctx.colors === undefined) ctx.colors = false; | ||
if (ctx.customInspect === undefined) ctx.customInspect = true; | ||
if (ctx.showProxy === undefined) ctx.showProxy = false; | ||
if (ctx.colors) ctx.stylize = stylizeWithColor; | ||
return formatValue(ctx, obj, ctx.depth); | ||
} | ||
|
@@ -241,6 +242,46 @@ function inspectPromise(p) { | |
|
||
|
||
function formatValue(ctx, value, recurseTimes) { | ||
if (ctx.showProxy && | ||
((typeof value === 'object' && value !== null) || | ||
typeof value === 'function')) { | ||
var proxy = undefined; | ||
var proxyCache = ctx.proxyCache; | ||
if (!proxyCache) | ||
proxyCache = ctx.proxyCache = new Map(); | ||
// Determine if we've already seen this object and have | ||
// determined that it either is or is not a proxy. | ||
if (proxyCache.has(value)) { | ||
// We've seen it, if the value is not undefined, it's a Proxy. | ||
proxy = proxyCache.get(value); | ||
} else { | ||
// Haven't seen it. Need to check. | ||
// If it's not a Proxy, this will return undefined. | ||
// Otherwise, it'll return an array. The first item | ||
// is the target, the second item is the handler. | ||
// We ignore (and do not return) the Proxy isRevoked property. | ||
proxy = binding.getProxyDetails(value); | ||
if (proxy) { | ||
// We know for a fact that this isn't a Proxy. | ||
// Mark it as having already been evaluated. | ||
// We do this because this object is passed | ||
// recursively to formatValue below in order | ||
// for it to get proper formatting, and because | ||
// the target and handle objects also might be | ||
// proxies... it's unfortunate but necessary. | ||
proxyCache.set(proxy, undefined); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand the logic but I don't think the 'necessary' is accurate; if you skipped adding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Necessary in the sense of avoiding an extraneous call to getProxyDetails... Which I would consider to be a bug since the point is to only check when we don't know for sure. ;) |
||
} | ||
// If the object is not a Proxy, then this stores undefined. | ||
// This tells the code above that we've already checked and | ||
// ruled it out. If the object is a proxy, this caches the | ||
// results of the getProxyDetails call. | ||
proxyCache.set(value, proxy); | ||
} | ||
if (proxy) { | ||
return 'Proxy ' + formatValue(ctx, proxy, recurseTimes); | ||
} | ||
} | ||
|
||
// Provide a hook for user-specified inspect functions. | ||
// Check that value is an object with an inspect function on it | ||
if (ctx.customInspect && | ||
|
@@ -785,7 +826,6 @@ exports.isPrimitive = isPrimitive; | |
|
||
exports.isBuffer = Buffer.isBuffer; | ||
|
||
|
||
function pad(n) { | ||
return n < 10 ? '0' + n.toString(10) : n.toString(10); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
'use strict'; | ||
|
||
require('../common'); | ||
const assert = require('assert'); | ||
const util = require('util'); | ||
const processUtil = process.binding('util'); | ||
const opts = {showProxy: true}; | ||
|
||
const target = {}; | ||
const handler = { | ||
get: function() { throw new Error('Getter should not be called'); } | ||
}; | ||
const proxyObj = new Proxy(target, handler); | ||
|
||
// Inspecting the proxy should not actually walk it's properties | ||
assert.doesNotThrow(() => util.inspect(proxyObj, opts)); | ||
|
||
// getProxyDetails is an internal method, not intended for public use. | ||
// This is here to test that the internals are working correctly. | ||
const details = processUtil.getProxyDetails(proxyObj); | ||
assert.strictEqual(target, details[0]); | ||
assert.strictEqual(handler, details[1]); | ||
|
||
assert.strictEqual(util.inspect(proxyObj, opts), | ||
'Proxy [ {}, { get: [Function] } ]'); | ||
|
||
// Using getProxyDetails with non-proxy returns undefined | ||
assert.strictEqual(processUtil.getProxyDetails({}), undefined); | ||
|
||
// This will throw because the showProxy option is not used | ||
// and the get function on the handler object defined above | ||
// is actually invoked. | ||
assert.throws( | ||
() => util.inspect(proxyObj) | ||
); | ||
|
||
// Yo dawg, I heard you liked Proxy so I put a Proxy | ||
// inside your Proxy that proxies your Proxy's Proxy. | ||
const proxy1 = new Proxy({}, {}); | ||
const proxy2 = new Proxy(proxy1, {}); | ||
const proxy3 = new Proxy(proxy2, proxy1); | ||
const proxy4 = new Proxy(proxy1, proxy2); | ||
const proxy5 = new Proxy(proxy3, proxy4); | ||
const proxy6 = new Proxy(proxy5, proxy5); | ||
const expected0 = '{}'; | ||
const expected1 = 'Proxy [ {}, {} ]'; | ||
const expected2 = 'Proxy [ Proxy [ {}, {} ], {} ]'; | ||
const expected3 = 'Proxy [ Proxy [ Proxy [ {}, {} ], {} ], Proxy [ {}, {} ] ]'; | ||
const expected4 = 'Proxy [ Proxy [ {}, {} ], Proxy [ Proxy [ {}, {} ], {} ] ]'; | ||
const expected5 = 'Proxy [ Proxy [ Proxy [ Proxy [Object], {} ],' + | ||
' Proxy [ {}, {} ] ],\n Proxy [ Proxy [ {}, {} ]' + | ||
', Proxy [ Proxy [Object], {} ] ] ]'; | ||
const expected6 = 'Proxy [ Proxy [ Proxy [ Proxy [Object], Proxy [Object]' + | ||
' ],\n Proxy [ Proxy [Object], Proxy [Object] ] ],\n' + | ||
' Proxy [ Proxy [ Proxy [Object], Proxy [Object] ],\n' + | ||
' Proxy [ Proxy [Object], Proxy [Object] ] ] ]'; | ||
assert.strictEqual(util.inspect(proxy1, opts), expected1); | ||
assert.strictEqual(util.inspect(proxy2, opts), expected2); | ||
assert.strictEqual(util.inspect(proxy3, opts), expected3); | ||
assert.strictEqual(util.inspect(proxy4, opts), expected4); | ||
assert.strictEqual(util.inspect(proxy5, opts), expected5); | ||
assert.strictEqual(util.inspect(proxy6, opts), expected6); | ||
assert.strictEqual(util.inspect(proxy1), expected0); | ||
assert.strictEqual(util.inspect(proxy2), expected0); | ||
assert.strictEqual(util.inspect(proxy3), expected0); | ||
assert.strictEqual(util.inspect(proxy4), expected0); | ||
assert.strictEqual(util.inspect(proxy5), expected0); | ||
assert.strictEqual(util.inspect(proxy6), expected0); | ||
|
||
// Just for fun, let's create a Proxy using Arrays. | ||
const proxy7 = new Proxy([], []); | ||
const expected7 = 'Proxy [ [], [] ]'; | ||
assert.strictEqual(util.inspect(proxy7, opts), expected7); | ||
assert.strictEqual(util.inspect(proxy7), '[]'); | ||
|
||
// Now we're just getting silly, right? | ||
const proxy8 = new Proxy(Date, []); | ||
const proxy9 = new Proxy(Date, String); | ||
const expected8 = 'Proxy [ [Function: Date], [] ]'; | ||
const expected9 = 'Proxy [ [Function: Date], [Function: String] ]'; | ||
assert.strictEqual(util.inspect(proxy8, opts), expected8); | ||
assert.strictEqual(util.inspect(proxy9, opts), expected9); | ||
assert.strictEqual(util.inspect(proxy8), '[Function: Date]'); | ||
assert.strictEqual(util.inspect(proxy9), '[Function: Date]'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you storing the array that .getProxyDetails() returns as a key? I would expect this to be
proxyCache.set(value, proxy)
. The comment seems wrong as well. This wholeif
statement seems like it could be dropped, actually.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look at lines 273 and 276.