util: remove instance check on empty object

[baylesa-dev]

Remove a useless check on Object descriptor. It seems like js browsers engine dont do this check.

Fixes: #35730

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

[aduh95]

Hi @baylesa-dev 👋 Thanks for opening this! Could you add a test to ensure the related issue doesn't happen again if we make other changes to this code in the future?

[nodejs-github-bot]

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

[codecov-io]

Codecov Report

Merging #35754 into master will decrease coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #35754      +/-   ##
==========================================
- Coverage   87.92%   87.85%   -0.07%     
==========================================
  Files         477      477              
  Lines      113090   113088       -2     
  Branches    24632    24606      -26     
==========================================
- Hits        99431    99356      -75     
- Misses       7948     8020      +72     
- Partials     5711     5712       +1     

Impacted Files	Coverage Δ
lib/internal/util/inspect.js	93.41% <100.00%> (-2.26%)	⬇️
src/connect_wrap.h	25.00% <0.00%> (-75.00%)	⬇️
lib/internal/repl/history.js	88.16% <0.00%> (-4.15%)	⬇️
src/inspector_agent.cc	83.88% <0.00%> (-0.86%)	⬇️
lib/internal/encoding.js	84.06% <0.00%> (-0.53%)	⬇️
src/node_http_parser.cc	80.75% <0.00%> (-0.45%)	⬇️
lib/internal/url.js	96.12% <0.00%> (-0.41%)	⬇️
lib/_http_server.js	97.83% <0.00%> (-0.11%)	⬇️
src/module_wrap.cc	70.84% <0.00%> (+0.24%)	⬆️

[baylesa-dev]

@aduh95 Do you know why some checks weren't successful ? What could I do to resolve them?

[BridgeAR]

@baylesa-dev the check is not useless. It's a safeguard to find the correct prototype. To fix the issue, we'll have to wrap the instanceof check in a try / catch and return false in the error case.

This should be a full fix:

diff --git a/lib/internal/util/inspect.js b/lib/internal/util/inspect.js
index e6787760fe..c281d36630 100644
--- a/lib/internal/util/inspect.js
+++ b/lib/internal/util/inspect.js
@@ -534,6 +534,14 @@ function getEmptyFormatArray() {
   return [];
 }
 
+function isInstanceof(object, proto) {
+  try {
+    return object instanceof proto;
+  } catch {
+    return false;
+  }
+}
+
 function getConstructorName(obj, ctx, recurseTimes, protoProps) {
   let firstProto;
   const tmp = obj;
@@ -542,7 +550,7 @@ function getConstructorName(obj, ctx, recurseTimes, protoProps) {
     if (descriptor !== undefined &&
         typeof descriptor.value === 'function' &&
         descriptor.value.name !== '' &&
-        tmp instanceof descriptor.value) {
+        isInstanceof(tmp, descriptor.value)) {
       if (protoProps !== undefined &&
          (firstProto !== obj ||
          !builtInObjects.has(descriptor.value.name))) {
diff --git a/test/parallel/test-util-inspect.js b/test/parallel/test-util-inspect.js
index 70f2ff6432..a8caf6f891 100644
--- a/test/parallel/test-util-inspect.js
+++ b/test/parallel/test-util-inspect.js
@@ -2866,6 +2866,17 @@ assert.strictEqual(
   );
 }
 
+// Regression test for https://github.com/nodejs/node/issues/35730
+{
+  function Func() {}
+  Func.prototype = null;
+  const object = {};
+  object.constructor = Func;
+
+  assert.strictEqual(util.inspect(object), '{ constructor: [Function: Func] }');
+}
+
 // Test changing util.inspect.colors colors and aliases.
 {
   const colors = util.inspect.colors;

[ExE-Boss]

This change broke tests inspecting the prototype object, which is not an instance of its constructor.

[Trott]

Should this PR be closed or is work ongoing?

[ExE-Boss]

This PR has been superseded by #36178, which has now been merged.

Wrong button