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: worksutil.inspect(newProxy(proxyObj,{}),{depth: 0,colors: false,compact: 3});// nested inspect: throws on main, works with this PRutil.format('%s',newProxy(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

confidenceimprovement accuracy (*) (**) (***)
util/inspect-array.jstype='denseArray_showHidden'len=100n=5000-0.38 % ±1.24% ±1.65% ±2.15%
util/inspect-array.jstype='denseArray_showHidden'len=100000n=5000*1.19 % ±1.02% ±1.36% ±1.77%
util/inspect-array.jstype='denseArray'len=100n=5000-0.13 % ±1.11% ±1.48% ±1.92%
util/inspect-array.jstype='denseArray'len=100000n=50000.05 % ±0.63% ±0.83% ±1.09%
util/inspect-array.jstype='mixedArray'len=100n=5000-0.05 % ±0.46% ±0.61% ±0.79%
util/inspect-array.jstype='mixedArray'len=100000n=5000-0.49 % ±1.26% ±1.67% ±2.18%
util/inspect-array.jstype='sparseArray'len=100n=5000-0.82 % ±2.44% ±3.25% ±4.23%
util/inspect-array.jstype='sparseArray'len=100000n=50000.91 % ±0.93% ±1.24% ±1.63%
util/inspect-proxy.jsisProxy=0showProxy=0n=1000000.02 % ±1.70% ±2.26% ±2.95%
util/inspect-proxy.jsisProxy=0showProxy=1n=100000-0.61 % ±1.47% ±1.95% ±2.55%
util/inspect-proxy.jsisProxy=1showProxy=0n=1000000.24 % ±1.18% ±1.57% ±2.05%
util/inspect-proxy.jsisProxy=1showProxy=1n=1000000.09 % ±1.25% ±1.66% ±2.17%
util/inspect.jsoption='colors'method='Array'n=800000.23 % ±1.77% ±2.36% ±3.07%
util/inspect.jsoption='colors'method='Date'n=80000-0.84 % ±1.61% ±2.14% ±2.79%
util/inspect.jsoption='colors'method='Error'n=80000-0.13 % ±0.75% ±1.00% ±1.30%
util/inspect.jsoption='colors'method='Number'n=80000-0.03 % ±1.83% ±2.43% ±3.17%
util/inspect.jsoption='colors'method='Object_deep_ln'n=80000-0.28 % ±0.68% ±0.91% ±1.18%
util/inspect.jsoption='colors'method='Object_empty'n=800003.02 % ±3.27% ±4.35% ±5.67%
util/inspect.jsoption='colors'method='Object'n=80000-0.11 % ±0.73% ±0.97% ±1.27%
util/inspect.jsoption='colors'method='Set'n=800000.03 % ±1.45% ±1.93% ±2.51%
util/inspect.jsoption='colors'method='String_boxed'n=80000-0.08 % ±1.33% ±1.77% ±2.30%
util/inspect.jsoption='colors'method='String_complex'n=800000.48 % ±0.93% ±1.24% ±1.62%
util/inspect.jsoption='colors'method='String'n=800001.28 % ±1.44% ±1.92% ±2.50%
util/inspect.jsoption='colors'method='TypedArray_extra'n=800000.42 % ±1.59% ±2.12% ±2.76%
util/inspect.jsoption='colors'method='TypedArray'n=80000-0.03 % ±1.50% ±2.00% ±2.60%
util/inspect.jsoption='none'method='Array'n=800000.12 % ±0.78% ±1.04% ±1.35%
util/inspect.jsoption='none'method='Date'n=80000-0.96 % ±1.65% ±2.20% ±2.86%
util/inspect.jsoption='none'method='Error'n=80000-0.23 % ±1.05% ±1.40% ±1.82%
util/inspect.jsoption='none'method='Number'n=800001.06 % ±4.02% ±5.35% ±6.97%
util/inspect.jsoption='none'method='Object_deep_ln'n=80000-0.35 % ±0.54% ±0.72% ±0.94%
util/inspect.jsoption='none'method='Object_empty'n=800000.10 % ±3.60% ±4.79% ±6.23%
util/inspect.jsoption='none'method='Object'n=80000-0.55 % ±0.83% ±1.10% ±1.44%
util/inspect.jsoption='none'method='Set'n=80000-1.13 % ±1.74% ±2.31% ±3.01%
util/inspect.jsoption='none'method='String_boxed'n=800000.21 % ±1.24% ±1.65% ±2.15%
util/inspect.jsoption='none'method='String_complex'n=800000.65 % ±1.42% ±1.88% ±2.46%
util/inspect.jsoption='none'method='String'n=800000.38 % ±2.44% ±3.25% ±4.23%
util/inspect.jsoption='none'method='TypedArray_extra'n=80000-0.29 % ±0.57% ±0.75% ±0.98%
util/inspect.jsoption='none'method='TypedArray'n=800000.50 % ±0.54% ±0.72% ±0.93%
util/inspect.jsoption='showHidden'method='Array'n=800000.10 % ±0.42% ±0.56% ±0.74%
util/inspect.jsoption='showHidden'method='Date'n=800000.19 % ±1.24% ±1.65% ±2.15%
util/inspect.jsoption='showHidden'method='Error'n=80000-0.18 % ±0.97% ±1.29% ±1.68%
util/inspect.jsoption='showHidden'method='Number'n=80000-0.26 % ±3.84% ±5.11% ±6.65%
util/inspect.jsoption='showHidden'method='Object_deep_ln'n=80000-0.17 % ±0.63% ±0.84% ±1.09%
util/inspect.jsoption='showHidden'method='Object_empty'n=80000-0.64 % ±2.66% ±3.54% ±4.61%
util/inspect.jsoption='showHidden'method='Object'n=800000.42 % ±0.91% ±1.21% ±1.57%
util/inspect.jsoption='showHidden'method='Set'n=800001.01 % ±1.56% ±2.07% ±2.70%
util/inspect.jsoption='showHidden'method='String_boxed'n=80000-0.13 % ±0.72% ±0.95% ±1.24%
util/inspect.jsoption='showHidden'method='String_complex'n=800000.22 % ±0.95% ±1.26% ±1.64%
util/inspect.jsoption='showHidden'method='String'n=800001.44 % ±2.49% ±3.31% ±4.31%
util/inspect.jsoption='showHidden'method='TypedArray_extra'n=80000-0.04 % ±0.80% ±1.07% ±1.39%
util/inspect.jsoption='showHidden'method='TypedArray'n=800000.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-majorPRs 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