console: fix console.dir crash on a revoked proxy

[daeyeon]

Fixes: #43095

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

[BridgeAR]

Thanks for the PR! I am going to ignore the implementation details for now to clarify the visualization first:

I am wondering how we want to visualize the revoked proxy with showProxy === false. It's not null but we do not know about the value either. We could just print Revoked Proxy [] or Proxy [ null, null ]. This would however expose the information that it's a proxy which is a hidden fact for not-revoked proxies.

The situation for showProxy === true is simpler: just either use Revoked Proxy [] or Proxy [ null, null ]. I personally like the Revoked Proxy [] a bit better. Any other opinions @nodejs/util?

[daeyeon]

Thanks for starting the clarification!

FAYI, this PR's current visualization is the following:

let r = Proxy.revocable({}, {});
console.log(1, inspect(r.proxy));
console.log(2, inspect(r.proxy, { showProxy: true }));

r.revoke();

console.log(3, inspect(r.proxy));
console.log(4, inspect(r.proxy, { showProxy: true }));

1 {}
2 Proxy [ {}, {} ]
3 Proxy []
4 Proxy [ null, null ]

(3) was the part I was most confused of.

[BridgeAR]

@daeyeon what do you think about:

1 {}
2 Proxy [ {}, {} ]
3 <Revoked Proxy>
4 <Revoked Proxy>

3 + 4 using special for formatting (ctx.stylize('<Revoked Proxy>', 'special')).

[BridgeAR]

It is possible to simplify the code by using this diff (line numbers must be ignored as they are not identical), if we do not care to distinguish visualization with the showProxy option:

@@ -759,6 +758,9 @@ function formatValue(ctx, value, recurseTimes, typedArray) {
   // any proxy handlers.
   const proxy = getProxyDetails(value, !!ctx.showProxy);
   if (proxy !== undefined) {
+    if (proxy === null) {
+      return ctx.stylize('<Revoked Proxy>', 'special');
+    }
     if (ctx.showProxy) {
       return formatProxy(ctx, proxy, recurseTimes);
     }
@@ -1967,6 +1960,9 @@ function hasBuiltInToString(value) {
   const getFullProxy = false;
   const proxyTarget = getProxyDetails(value, getFullProxy);
   if (proxyTarget !== undefined) {
+    if (proxyTarget === null) {
+      return true;
+    }
     value = proxyTarget;
   }

The last change moves the extra check to only be executed for proxies and no other entries (value?.toString has to run the check for all entries).

[daeyeon]

@BridgeAR Thanks for the review.

I also think that <Revoked Proxy> looks good and provides enough information for a revoked proxy. It can make the code simpler indeed. I applied your suggestion. PTAL.

The last change moves the extra check to only be executed for proxies and no other entries (value?.toString has to run the check for all entries).

Also fixed.

[BridgeAR]

LGTM!

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/44100/

[nodejs-github-bot]

Landed in f7cd3f6