"[Object: null prototype]" spam from util.inspect

[mvduin]

Commit 90014b6 changed util.inspect as a fix for #22141. While it may be important to emphasize null-prototype objects for that very particular use-case, for general debugging it is just annoying spam that can make dumps of complex objects substantially less readable. This should not be the default behaviour, but an option specifically enabled by assert.deepStrictEqual's use of inspect.

[mvduin]

The commit itself also seems overly complicated to me. Why is it checking for a null prototype on an object which is a Set, Map, Array, or TypedArray? All of those are mutually exclusive with having a null-prototype.

[addaleax]

All of those are mutually exclusive with having a null-prototype.

It’s not exclusive:

> a = new Map([[1,2],[3,4]]); Object.setPrototypeOf(a, null)
[Map: null prototype] { 1 => 2, 3 => 4 }
> a.set
undefined

That being said – it’s unlikely to happen in the real world. I don’t have strong opinions about whether or how to include this information in the util.inspect() output.

/cc @BridgeAR

[mvduin]

Yikes, sometimes I forget how horrifying javascript can be. But yeah, noone would do this in real-world code (I'd hope!), as opposed to Object.create(null) which is a fairly common thing to do (and is even used for various settings objects of util.inspect itself).

[devsnek]

it only adds extra output to things that actually have a null prototype, and as you say, its rather rare, so perhaps not a huge issue?

[mvduin]

I said no such thing, and if I didn't consider it an issue then I wouldn't have opened one.

Using a null-prototype for objects is common good practice in certain cases (basically whenever it is used kind of like a Map, but using an actual Map would just be more cumbersome and less programmer-friendly than using an object). The lib directory of nodejs (v10.x branch) itself contains 43 occurrences of Object.create(null). More importantly (to me), they are all over my codebase.

For example, this update turned this debug dump in a test script:

/: keydb =
        { host: { key: h`159355cb40dc15c987e7830c77c98fd47d437fdae9ad51787333f0da97e38a7d` },
          product:
           { key: h`4b038d142de2a5453e1ea15e558115077dc208477a5447508f3990e839073f44`,
             authority: 'product' },
          root:
           { key: h`a7a4b0c777a880e7d77a715d07fefad75bbfd194e51735bc2c247ba52de9390d`,
             authority: 'root' },
          test:
           { key: h`a7299d4c99ec31474513fc39f407521ff388ca5d830a1f2a7a1aed71d302f7bd`,
             authority: 'root' },
          'u:test':
           { key: h`e78299c54a9e3c2b55adc2fff0f794d447f8737b181a9f57fecbd7ba8d715aa7`,
             authority: 'user' } }

into

/: keydb =
        [Object: null prototype] {
          host:
           [Object: null prototype] { key: h`159355cb40dc15c987e7830c77c98fd47d437fdae9ad51787333f0da97e38a7d` },
          product:
           [Object: null prototype] {
             key: h`4b038d142de2a5453e1ea15e558115077dc208477a5447508f3990e839073f44`,
             authority: 'product' },
          root:
           [Object: null prototype] {
             key: h`a7a4b0c777a880e7d77a715d07fefad75bbfd194e51735bc2c247ba52de9390d`,
             authority: 'root' },
          test:
           [Object: null prototype] {
             key: h`a7299d4c99ec31474513fc39f407521ff388ca5d830a1f2a7a1aed71d302f7bd`,
             authority: 'root' },
          'u:test':
           [Object: null prototype] {
             key: h`e78299c54a9e3c2b55adc2fff0f794d447f8737b181a9f57fecbd7ba8d715aa7`,
             authority: 'user' } }

This is simply an eyesore.

(In contrast, creating a Set/Map/Array/TypedArray and then manually changing the prototype to null is a bizarre thing to do, hopefully doesn't happen in any real-world code, and I don't think util.inspect should bother to waste any time on considering these cases.)

[BridgeAR]

@mvduin we provide the prototype for all objects. Doing the same for the null prototype allows to distinguish those objects. Since this utility function is meant for debugging it's also important to have as much information as possible. Without that, you could miss subtle bugs. If you can come up with an alternative that is intuitive and allows distinguishing "regular" objects from null prototype ones, please let me know.

Examples how the output behaves for different prototypes:

class Foo {}
const a = {}
Object.setPrototypeOf(a, Foo.prototype)

console.log(a)
// Foo {}

I personally suggest to give the compact option a try (a relative recent addition). It changes the output formatting and should be better in case of readability. Using compact: 3 with your example results in:

> util.inspect.replDefaults.compact = 3
3
> keydb
{
  host: [Object: null prototype] {
    key: '159355cb40dc15c987e7830c77c98fd47d437fdae9ad51787333f0da97e38a7d'
  },
  product: [Object: null prototype] {
    key: '4b038d142de2a5453e1ea15e558115077dc208477a5447508f3990e839073f44',
    authority: 'product'
  },
  root: [Object: null prototype] {
    key: 'a7a4b0c777a880e7d77a715d07fefad75bbfd194e51735bc2c247ba52de9390d',
    authority: 'root'
  },
  test: [Object: null prototype] {
    key: 'a7299d4c99ec31474513fc39f407521ff388ca5d830a1f2a7a1aed71d302f7bd',
    authority: 'root'
  },
  'u:test': [Object: null prototype] {
    key: 'e78299c54a9e3c2b55adc2fff0f794d447f8737b181a9f57fecbd7ba8d715aa7',
    authority: 'user'
  }
}

I don't think util.inspect should bother to waste any time on considering these cases

util.inspect() should provide useful information and inspection no matter how bad the input was tampered with. It also does not "waste" any time as it's no real overhead to distinguish these cases. The performance bottlenecks lay elsewhere and util.inspect() is actually quite fast.

---

As a personal request: it would be nice if you would try to provide your feedback in a more constructive way that does not decrease the value of other persons work.

[benjamingr]

As a personal request: it would be nice if you would try to provide your feedback in a more constructive way that does not decrease the value of other persons work.

👍 yes please.

@mvduin hey 👋 Please be more considerate when you interact here.

Please consider self moderating the below:

1. no sane person would do this - calling people's sanity into question is not acceptable conduct in the issue tracker.
2. The commit itself also seems really bizarre - snarky comments about the people working on Node.js are not acceptable conduct in the issue tracker.

In addition, I would recommend considering toning down the criticism in your messages and focus on objective technical aspects of the issue.

Please refer to https://github.com/nodejs/admin/blob/master/CODE_OF_CONDUCT.md for a brief guide on interaction in the org.

If you would like to discuss this in private feel free to reach out via report@nodejs.org or moderation@nodejs.org

Thanks for the issue, I think this discussion is worth having and I'd like us to figure out a way to avoid [Prototype null] in the logs or make it less verbose in "the common case".

[othiym23]

(wearing my moderator hat)

In addition, I would recommend considering toning down the criticism in your messages and focus on objective technical aspects of the issue.

To echo and amplify a little, yes, there are definitely warts to JavaScript, especially around prototype handling, but #22141 is an issue that can be solved a number of ways, none of which are obviously "most" correct. It seems like this conversation is headed towards an OK resolution, but still – can we keep the conversation focused on pros and cons, and spend less time talking about aesthetics, which are inherently subjective?

[bnoordhuis]

I'm tagging this 'feature request'. I think the consensus is that in general it's a good change and unlikely to simply be reverted. (FWIW, I feel the same way.)

If someone doesn't come up with a good counterproposal in the next few days, I'll close this out.

[GrosSacASac]

for null prototype:
[object]
for regular prototype:
[object Object]

[mvduin]

Apologies for my ill chosen words. I just got really annoyed when, after applying some updates to have the latest security fixes, I was suddenly faced with this rather verbose output in my previously lean and clean debug output. Posting while in a state of annoyance is rarely a good idea.

Please consider self moderating the below

Done. I've edited my posts to tone them down a bit.

util.inspect() should provide useful information and inspection no matter how bad the input was tampered with.

But there are limits to what it can do regardless, and it's still easy to find cases where assert.deepStrictEqual fails on arguments which are rendered identically by util.inspect. I don't really care about how null-prototype non-Objects are presented either way, I was just surprised effort was put into those without any known or expected use case.

The new output also seems rather inconsistent to me. In general util.inspect uses the format name { properties } where name is the name of constructor property (provided that it's a function) of the value inspected, regardless of its prototype.

> { __proto__: {} }
{}
> { __proto__: { constructor: { name: "Hello" }} }
{}
> { __proto__: { constructor: function Hello(){} } }
Hello {}
> { constructor: function Hello(){} }
Hello { constructor: [Function: Hello] }
> { __proto__: Foo.prototype, constructor: function Hello(){} }
Hello { constructor: [Function: Hello] }

In a rare few cases it also emphasizes that an object is an instance of some specific base class that is regarded as being important (e.g. Promise or Set), when it doesn't match the constructor name:

> { __proto__: Set.prototype, constructor: function Hello(){} }
Hello [Set] { constructor: [Function: Hello] }

null-prototype objects don't really match any of these cases, but if it is felt the need exists to render them differently from "normal" objects, I think null { ... } would be perfectly adequate.

[ljharb]

What would you suggest for a null-prototype Map or Set or other object? (I’m going to be making these same changes in https://npmjs.com/object-inspect, to follow whatever the conclusion is here)

[mvduin]

What would you suggest for a null-prototype Map or Set or other object?

I honestly don't think they are a relevant case to consider, and I'm not sure it even makes sense to consider these to be Maps or Sets since they are not instances of these classes, even if they have the relevant hidden slots. I'd be fine with them being rendered as null {}. If you do want to render them as sets, then null [Set] { ...elements... } probably?

I was curious to see how util.inspect currently renders an instance of Set with its prototype changed to a random other class, and the answer is it doesn't:

> Object.setPrototypeOf(new Set, (class Foo {}).prototype)
TypeError: clazz is not a constructor
    at noPrototypeIterator (internal/util/inspect.js:429:14)
    at formatRaw (internal/util/inspect.js:688:31)
    at formatValue (internal/util/inspect.js:508:10)
    at Object.inspect (internal/util/inspect.js:191:10)

[ljharb]

The slots are entirely what makes them maps or sets, not their prototype chain :-) you’ve found what i consider a bug in util.inspect tho!

So if a set is normally Set { }, a null set would be null [Set] { }?

[mvduin]

The slots are entirely what makes them maps or sets, not their prototype chain :-)

I mean, technically yes, but for most practical purposes that's an implementation detail. The only way the average javascript programmer can test whether a value is a Set is using x instanceof Set.

Also, using the hidden slots rather than prototype chain seems inconsistent with other behaviour of util.inspect, e.g.:

> { __proto__: Set.prototype, constructor: class Foo{} }
Foo [Set] { constructor: [Function: Foo] }

Here it's highlighting this object as being a Set purely based on its prototype chain, even though it lacks the hidden slots. This kind of suggests that [Set] should be added only if the object is actually an instance of Set, and the hidden slots should only be used to render the body (since there you don't really have an alternative).

So if a set is normally Set { }, a null set would be null [Set] { }?

Either that or actually just null {} but with the contents rendered like a set (i.e. elements followed by properties). It depends a bit on whether my example would have been rendered as Foo [Set] {} or Foo {} if it didn't break inspect.

Mostly though, I'd just go with whatever is the least effort, since I don't think it matters how these abominations are rendered ;-)

[BridgeAR]

@mvduin

I've edited my posts to tone them down a bit.

Thank you.

it's still easy to find cases where assert.deepStrictEqual fails

This is not about assert. It is just a side effect that it relies on the output for some things.

I was just surprised effort was put into those without any known or expected use case

It is about consistency and to prevent tampering. This is especially important in JS where it's possible to manipulate everything. So there's definitely a use case.

In a rare few cases it also emphasizes that an object is an instance of some specific base class that is regarded as being important

That is not correct. It just prints the Symbol.toStringTag in your mentioned examples.

I was curious to see how util.inspect currently renders an instance of Set with its prototype changed to a random other class, and the answer is it doesn't

That is outdated. On master it prints Foo {}. Ideally it would print something like Foo [Set] {}.

I honestly don't think they are a relevant case to consider

I definitely want to know about tampered objects in my logs. It is not about how I personally create objects, it's about input in general, no matter from what source.

I'd be fine with them being rendered as null {}

This is not intuitive for me (and I expect to others as well but I can only speak for myself). As a reader, I would expect this output to be a bug and not that it's about the null-prototype.

The new output also seems rather inconsistent to me.

It is a best effort for most inputs. I personally think your examples all come out fine but there is no absolute right or wrong. So far all additions to util.inspect were about fine-tuning the output. There's definitely room for improvement and distinguishing more cases and making it more intuitive has always been the goal.

@GrosSacASac

for regular prototype: [object Object]

That would cause "regular" objects to be more verbose than they are currently.

for null prototype: [object]

I personally would not know what this stands for. And how should it look like for other classes?

[BridgeAR]

@mvduin

The only way the average javascript programmer can test whether a value is a Set is using x instanceof Set.

That is not correct util.types.isSet. That is tampering proof and should always be used instead of instanceof checks.

just null {} but with the contents rendered like a set

That would not be distinguishable from a lot of other types.

I'd just go with whatever is the least effort

The current implementation is more complex for null prototypes so the default case won't slow down. The direct implementation would be trivial. I wanted to remove some special casing that actually makes things more difficult to handle a couple of times but that would result in up to 10% performance degradation for other types and that is not acceptable.

[mvduin]

That is not correct util.types.isSet.

Ah, I stand corrected. I looked for it in util since I remembered it had a bunch of these tests, and had not realized that these tests were arbitrarily split between util and util.types.

That is tampering proof

You've used the word "tampering" before but I have no idea what you're referring to. Who is tampering with what, and what are they doing in my nodejs application? Every npm module I use is (unfortunately) implicitly trusted anyway, since they can execute arbitrary code as part of being installed by npm. I have trouble imagining what kind of tampering would involve passing me objects with an oddball prototype.

should always be used instead of instanceof checks.

Either should be fine if you're not doing anything with multiple contexts. This is probably more a worry when doing web programming, yet in that case util.types.isSet is not available. (Why does the ECMAScript spec not provide functions like these anyway, analogous to Array.isArray ?)

That would not be distinguishable from a lot of other types.

I know, but I very much doubt it matters in this case. There are numerous ways anyway in which objects are not necessarily distinguishable based on util.inspect output. Consistency in output format is also important, to be able to interpret it correctly. Excessive verbosity can also decrease the usefulness of the output by making it harder to extract the information you're interested in due to the noise present.

[mvduin]

This is not intuitive for me (and I expect to others as well but I can only speak for myself). As a reader, I would expect this output to be a bug and not that it's about the null-prototype.

But you'd figure it out quickly. The current [Object: null prototype] feels flat out wrong since they are not an instance of Object. At the very least it ought to be shortened to just [null prototype].

Anyway, it seems that using a null-prototype object as prototype (instead of null itself) is an effective workaround:

> Object.create( Object.create(null) )
{}

so I guess I'll just start using that instead of Object.create(null). (Or maybe replace Object.create to automatically do so globally... hmm, disgusting but still somewhat tempting...)

[benjamingr]

You've used the word "tampering" before but I have no idea what you're referring to. Who is tampering with what, and what are they doing in my nodejs application? Every npm module I use is (unfortunately) implicitly trusted anyway, since they can execute arbitrary code as part of being installed by npm. I have trouble imagining what kind of tampering would involve passing me objects with an oddball prototype.

Let's say you write your code on Node latest and deploy it on Node 8 (LTS). You tested it and everything works. Unfortunately one of your npm dependencies has a dependency that detects Node's version and shims some newer Node feature (let's say: worker_threads, some http2 change or a new crypto method).

That shim for compatibility replaces Error (or another builtin) in order to provide some feature (for example better stack traces, instrumented errors, spec compliance for the feature etc).

Now, I wish polyfills wouldn't do this but it's also a pretty common things. We've had polyfills (like for WeakMap) that altered Object.prototype.

Now as Node core and an open source library maintainer this is something we care about a lot since we use "the same JavaScript" too. I've had bugs related to some user changing some builtin in virtually any open source library I maintain.

Now, you might say that for the average app developer it's not a huge deal and that proper tests would catch the above example and you would be right. I'm on the fence with regards to how we should log prototypes and I'd like to see better behaviour than what we currently do.

Why does the ECMAScript spec not provide functions like these anyway, analogous to Array.isArray ?

I think this was discussed a few times (detecting the same object type across realms). I think @ljharb even had a proposal for isError at some point (what happened to that?).

Most of the work on those proposal is blocked on actually doing the work, lobbying and reaching consensus in tc39. I encourage you to reach out to specification bodies (we're lucky to have several tc39 members around) and ask how you can get involved.

Excessive verbosity can also decrease the usefulness of the output by making it harder to extract the information you're interested in due to the noise present.

Absolutely, echoing @bnoordhuis I think what we need right now is a counter-proposal that maintains correctness but has less verbose output.

[ljharb]

Error.isError was rejected; the committee isn’t fond of brand-checking, by and large, and Error remains the only builtin that doesn’t provide one - the stacks proposal, however, will provide one.

Brand checks remain very important, and i have a number of packages in npm that work in both browsers and node to perform them.

[BridgeAR]

@mvduin

I [...] had not realized that these tests were arbitrarily split between util and util.types.

They are not. Only use the ones exposed by util.types. The ones exposed on util are mostly deprecated and might use instanceof.

But you'd figure it out quickly.

That's not what I call a good API. Things should feel natural without having to look up what things stand for.

The current [Object: null prototype] feels flat out wrong since they are not an instance of Object. At the very least it ought to be shortened to just [null prototype].

It is still an object, even if it has a null prototype. It's also a big difference if the prototype is set on e.g., a Set or a "plain" Object. It is possible to do things in JS that do not seem "right" but a lot of code contains things that "do not seem right". It's therefore important to tell the user what builtin was used before the prototype was set to null.

it seems that using a null-prototype object as prototype (instead of null itself) is an effective workaround

Newer Node.js versions log the prototype in these cases as well, so I recommend against it.

Or maybe replace Object.create to automatically do so globally... hmm, disgusting but still somewhat tempting...
You've used the word "tampering" before but I have no idea what you're referring to. [...] I have trouble imagining what kind of tampering would involve passing me objects with an oddball prototype.
I very much doubt it matters [to distinguish different built-ins with null prototype]

That is what I call "tampering" with code: any developer in a team or a module might do things on the prototype or with what ever objects. It could be a team mate or a bug that causes the prototype to be manipulated as security vector while parsing incoming JSON. What ever reason there might be that someone manipulated a prototype, it is always good to know what data you are actually looking at. Setting the prototype to null would cause all for of loops to fail while it would still be possible to inspect the set (with an indirection). It could therefore show up as valid set and you would have to invest a lot of time figuring out what's actually wrong. I would not want to be in that situation.

verbosity can also decrease the usefulness of the output by making it harder to extract the information you're interested

Visualizing the prototype for special cases should not be a main factor for log output.

Why does the ECMAScript spec not provide functions like [...] Array.isArray

That seems out of scope for this issue.

[BridgeAR]

@benjamingr

I think what we need right now is a counter-proposal that maintains correctness but has less verbose output

Did you struggle with the output so far and have you tried the compact option set to false or better 2 or 3 before (depending on what version of Node.js you use)?

It is not less verbose but the formatting is improved.

I personally like the current intuitive null prototype description and I personally can not think of an alternative that would be as good. Thus I am hesitant about changing something in that direction.

[benjamingr]

Did you struggle with the output so far and have you tried the compact option set to false or better 2 or 3 before (depending on what version of Node.js you use)?

Honestly I'm not too bothered by it since I haven't run into an issue with the current method "organically" myself. I can of course create a case where it is more verbose but 🤷‍♂️

I personally like the current intuitive null prototype description and I personally can not think of an alternative that would be as good. Thus I am hesitant about changing something in that direction.

I can't think of one either - that's not to say there isn't a better alternative we just haven't thought of yet.

I was mostly saying this to reiterate that a better proposal is what's currently blocking improving this (rather than say... code).

---

@ljharb

Error.isError was rejected; the committee isn’t fond of brand-checking

that's a shame, thank you for fighting the good fight and trying to promote these things :) 🙇

[BridgeAR]

As @bnoordhuis pointed out most people seem to actually agree with the change and there was no further comment for quite a while. Thus, I'll close this issue.

If there's another good alternative, please feel free to comment so that this can be reopened.