fix(core): Log customInspect implementation errors

[Soremwar]

Right now, the customInspect function in core swallows all errors that are thrown in user defined inspection functions. I don't think there is a reason for this behavior to be inforced (in my particular use case this is swallowing Permission errors).

With this PR, a warning will be appended to the inspected item showing the error thrown in the the inspect function

[nayeemrmn]

This was introduced in #3226. It can be troublesome if a broken customInspect prevents logging the object or any object that contains it. Instead, the string should indicate that it threw:

> console.log(headers);
[Custom inspect: Error: message] Headers {}

Logging should be throw-safe.

[Soremwar]

> ReadableStream.prototype
[Custom inspect: TypeError: Invalid ReadableStream.]
ReadableStream {
  [Symbol(Deno.customInspect)]: [Function: [Deno.customInspect]],
  [Symbol(Symbol.asyncIterator)]: [Function: [Symbol.asyncIterator]]
}

Is this a desirable output?

[nayeemrmn]

I'd omit the line break before the fallback output, it looks better for a nested property.

[Soremwar]

Hmm I discovered something interesting while messing around with this.

Turns out you can skip the inspect error pass by defining Deno.customInspect with a getter

Is this desired behavior based on what we just discussed?

const x = {}

Object.defineProperty(x, Deno.customInspect, {
  enumerable: false,
  configurable: false,
  get: function(){
    return Deno.env.toObject();
  }
})

console.log(x)

[Soremwar]

Mmm gotcha. I didn't know you could assign something other than a function to customInspect. This might cover my use case after all

[Soremwar]

Doesn't look so great on variables. I think I'm gonna leave the line break there

[Custom inspect: PermissionDenied: access to environment variables, run again with the --allow-env flag] [Function: x]

[Soremwar]

@00ff0000red You are right. However it can be polyfilled easily with Symbol.for("Deno.customInspect") instead of Deno.customInspect, so any Deno bundler like Bundler can make this code usable just as well in any environment

[ghost]

Actually, what I said was irrelevant anyway, since, in a browser, you wouldn't exactly have Deno.customInspect in the first place.

[Soremwar]

Ready for review

[ry]

What's the behavior in Node for this?

It seems like we should just throw the exception rather than trying to log it. I agree silently swallowing the error is not great. But why try to catch it at all?

[caspervonb]

Node throws

const util = require("util");

const foo = {
  [util.inspect.custom]() {
      console.log("before");
      throw new Error("error");
      console.log("after");
   },
 };
 
util.inspect(foo);
console.log("done");

before
/home/caspervonb/test.js:6
    throw new Error("error");
    ^

Error: error
    at Object.[nodejs.util.inspect.custom] (/home/caspervonb/test.js:6:11)
    at formatValue (node:internal/util/inspect:735:19)
    at Object.inspect (node:internal/util/inspect:309:10)
    at Object.<anonymous> (/home/caspervonb/test.js:11:6)
    at Module._compile (node:internal/modules/cjs/loader:1108:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1137:10)
    at Module.load (node:internal/modules/cjs/loader:973:32)
    at Function.Module._load (node:internal/modules/cjs/loader:813:14)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:76:12)
    at node:internal/main/run_main_module:17:47

I don't get why we'd do anything special here at all really.
I'd prefer to let this fail loud and clear.

[Soremwar]

Yeah I'm not crazy about this swallowing errors either (customInspect is an user implementation after all, it's not out of their control to fix the error).

I'll revert to the original commit and look for customInspect implementations in Deno code that require manual error handling (like Headers)

[Soremwar]

I'll look for customInspect implementations in Deno code that require manual error handling (like Headers)

On second though since errors like trying to read ReadableStream.prototype are really debug only there is no reason to change this implementations.

[nayeemrmn]

It surprises me that Node throws for this, because it doesn't when evaluating getters with getters: true. I don't see what the difference would be. I agree with @kevinkassimo's original PR to make logging (as an exception) throw safe.

[ry]

LGTM - this is more explicit, easier to debug. Thanks @Soremwar