util: fix nested proxy inspection

[BridgeAR]

Fixes: #61061

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.54%. Comparing base (dc97b50) to head (809774e).
⚠️ Report is 29 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #61077      +/-   ##
==========================================
+ Coverage   88.53%   88.54%   +0.01%     
==========================================
  Files         703      703              
  Lines      208551   208602      +51     
  Branches    40226    40235       +9     
==========================================
+ Hits       184633   184708      +75     
+ Misses      15939    15904      -35     
- Partials     7979     7990      +11     

Files with missing lines	Coverage Δ
lib/internal/util/inspect.js	99.63% <100.00%> (+<0.01%)	⬆️

... and 37 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[LiviaMedeiros]

The code LGTM, thanks!

[LiviaMedeiros]

Just for the record (not sure if solvable within this PR), it seems that there is a similar issue in util.format('%s', proxifiedProxy).

util.format('%s', proxyObj); // not nested: works
util.inspect(new Proxy(proxyObj, {}), { depth: 0, colors: false, compact: 3 }); // nested inspect: throws on main, works with this PR
util.format('%s', new Proxy(proxyObj, {})); // nested format: throws by triggering get trap

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[BridgeAR]

@LiviaMedeiros that access is in hasBuiltInToString() and it has to do almost the same as here: in case a proxy is detected, do a recursion, if the value is a proxy (it should be verified, if the proxy check is slower than just always doing the recursion directly and using the getProxyDetails method as isProxy). Shall I push that to this code or open a separate PR? I am not on the computer, so I can't change it now.

[LiviaMedeiros]

Yep, it seems that

diff --git a/lib/internal/util/inspect.js b/lib/internal/util/inspect.js
index e7b0063318c..2858dd0ea17 100644
--- a/lib/internal/util/inspect.js
+++ b/lib/internal/util/inspect.js
@@ -2685,6 +2692,9 @@ function hasBuiltInToString(value) {
     if (proxyTarget === null) {
       return true;
     }
+    if (isProxy(proxyTarget)) {
+      return hasBuiltInToString(proxyTarget);
+    }
     value = proxyTarget;
   }

fixes the issue for util.format().
I'll run benchmark locally to compare this with unconditional recursion; it probably should be faster for rare case of actual nested proxies but same or slightly slower than isProxy() for other cases.

upd: doesn't look like there's noticeable difference

                                                                      confidence improvement accuracy (*)   (**)  (***)
util/inspect-array.js type='denseArray_showHidden' len=100 n=5000                    -0.38 %       ±1.24% ±1.65% ±2.15%
util/inspect-array.js type='denseArray_showHidden' len=100000 n=5000           *      1.19 %       ±1.02% ±1.36% ±1.77%
util/inspect-array.js type='denseArray' len=100 n=5000                               -0.13 %       ±1.11% ±1.48% ±1.92%
util/inspect-array.js type='denseArray' len=100000 n=5000                             0.05 %       ±0.63% ±0.83% ±1.09%
util/inspect-array.js type='mixedArray' len=100 n=5000                               -0.05 %       ±0.46% ±0.61% ±0.79%
util/inspect-array.js type='mixedArray' len=100000 n=5000                            -0.49 %       ±1.26% ±1.67% ±2.18%
util/inspect-array.js type='sparseArray' len=100 n=5000                              -0.82 %       ±2.44% ±3.25% ±4.23%
util/inspect-array.js type='sparseArray' len=100000 n=5000                            0.91 %       ±0.93% ±1.24% ±1.63%
util/inspect-proxy.js isProxy=0 showProxy=0 n=100000                                  0.02 %       ±1.70% ±2.26% ±2.95%
util/inspect-proxy.js isProxy=0 showProxy=1 n=100000                                 -0.61 %       ±1.47% ±1.95% ±2.55%
util/inspect-proxy.js isProxy=1 showProxy=0 n=100000                                  0.24 %       ±1.18% ±1.57% ±2.05%
util/inspect-proxy.js isProxy=1 showProxy=1 n=100000                                  0.09 %       ±1.25% ±1.66% ±2.17%
util/inspect.js option='colors' method='Array' n=80000                                0.23 %       ±1.77% ±2.36% ±3.07%
util/inspect.js option='colors' method='Date' n=80000                                -0.84 %       ±1.61% ±2.14% ±2.79%
util/inspect.js option='colors' method='Error' n=80000                               -0.13 %       ±0.75% ±1.00% ±1.30%
util/inspect.js option='colors' method='Number' n=80000                              -0.03 %       ±1.83% ±2.43% ±3.17%
util/inspect.js option='colors' method='Object_deep_ln' n=80000                      -0.28 %       ±0.68% ±0.91% ±1.18%
util/inspect.js option='colors' method='Object_empty' n=80000                         3.02 %       ±3.27% ±4.35% ±5.67%
util/inspect.js option='colors' method='Object' n=80000                              -0.11 %       ±0.73% ±0.97% ±1.27%
util/inspect.js option='colors' method='Set' n=80000                                  0.03 %       ±1.45% ±1.93% ±2.51%
util/inspect.js option='colors' method='String_boxed' n=80000                        -0.08 %       ±1.33% ±1.77% ±2.30%
util/inspect.js option='colors' method='String_complex' n=80000                       0.48 %       ±0.93% ±1.24% ±1.62%
util/inspect.js option='colors' method='String' n=80000                               1.28 %       ±1.44% ±1.92% ±2.50%
util/inspect.js option='colors' method='TypedArray_extra' n=80000                     0.42 %       ±1.59% ±2.12% ±2.76%
util/inspect.js option='colors' method='TypedArray' n=80000                          -0.03 %       ±1.50% ±2.00% ±2.60%
util/inspect.js option='none' method='Array' n=80000                                  0.12 %       ±0.78% ±1.04% ±1.35%
util/inspect.js option='none' method='Date' n=80000                                  -0.96 %       ±1.65% ±2.20% ±2.86%
util/inspect.js option='none' method='Error' n=80000                                 -0.23 %       ±1.05% ±1.40% ±1.82%
util/inspect.js option='none' method='Number' n=80000                                 1.06 %       ±4.02% ±5.35% ±6.97%
util/inspect.js option='none' method='Object_deep_ln' n=80000                        -0.35 %       ±0.54% ±0.72% ±0.94%
util/inspect.js option='none' method='Object_empty' n=80000                           0.10 %       ±3.60% ±4.79% ±6.23%
util/inspect.js option='none' method='Object' n=80000                                -0.55 %       ±0.83% ±1.10% ±1.44%
util/inspect.js option='none' method='Set' n=80000                                   -1.13 %       ±1.74% ±2.31% ±3.01%
util/inspect.js option='none' method='String_boxed' n=80000                           0.21 %       ±1.24% ±1.65% ±2.15%
util/inspect.js option='none' method='String_complex' n=80000                         0.65 %       ±1.42% ±1.88% ±2.46%
util/inspect.js option='none' method='String' n=80000                                 0.38 %       ±2.44% ±3.25% ±4.23%
util/inspect.js option='none' method='TypedArray_extra' n=80000                      -0.29 %       ±0.57% ±0.75% ±0.98%
util/inspect.js option='none' method='TypedArray' n=80000                             0.50 %       ±0.54% ±0.72% ±0.93%
util/inspect.js option='showHidden' method='Array' n=80000                            0.10 %       ±0.42% ±0.56% ±0.74%
util/inspect.js option='showHidden' method='Date' n=80000                             0.19 %       ±1.24% ±1.65% ±2.15%
util/inspect.js option='showHidden' method='Error' n=80000                           -0.18 %       ±0.97% ±1.29% ±1.68%
util/inspect.js option='showHidden' method='Number' n=80000                          -0.26 %       ±3.84% ±5.11% ±6.65%
util/inspect.js option='showHidden' method='Object_deep_ln' n=80000                  -0.17 %       ±0.63% ±0.84% ±1.09%
util/inspect.js option='showHidden' method='Object_empty' n=80000                    -0.64 %       ±2.66% ±3.54% ±4.61%
util/inspect.js option='showHidden' method='Object' n=80000                           0.42 %       ±0.91% ±1.21% ±1.57%
util/inspect.js option='showHidden' method='Set' n=80000                              1.01 %       ±1.56% ±2.07% ±2.70%
util/inspect.js option='showHidden' method='String_boxed' n=80000                    -0.13 %       ±0.72% ±0.95% ±1.24%
util/inspect.js option='showHidden' method='String_complex' n=80000                   0.22 %       ±0.95% ±1.26% ±1.64%
util/inspect.js option='showHidden' method='String' n=80000                           1.44 %       ±2.49% ±3.31% ±4.31%
util/inspect.js option='showHidden' method='TypedArray_extra' n=80000                -0.04 %       ±0.80% ±1.07% ±1.39%
util/inspect.js option='showHidden' method='TypedArray' n=80000                       0.26 %       ±0.76% ±1.01% ±1.31%

As for adding it to this PR or making a follow-up one, I think whether is fine.

[BridgeAR]

I found some issues with this fix. The context would not be identical anymore for custom inspect functions. Now this is a very nuances issue and it is actually not completely clear what an ideal behavior is when it comes to this. I will investigate a bit more.

[BridgeAR]

@LiviaMedeiros I made a small microbenchmark. Calling it directly was faster. The inspect benchmark was not the right one for this code path :)

I fixed the other parts and also changed that multiple nested ones will also show up by default.

[nodejs-github-bot]

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

[gurgunday]

lgtm

[nodejs-github-bot]

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

[LiviaMedeiros]

Thanks! LGTM with or without the nit.

If you wanna, i can go ahead and open backport PRs to v20.x~v25.x once this lands, since the expectations in test rely on #61029 which is semver-major PRs that contain breaking changes and should be released in the next major version.

[BridgeAR]

@LiviaMedeiros that would be great, if you could open the manual backports for these, thank you!

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 9120924