util: add prototype support for function inspection

[BridgeAR]

This prevents function names being printed if the function is part
of an object and the property it's attached to has the same name as
the function.

This adds prototype support for functions when using util.inspect() and marks anonymous functions explicitly as such.

Checklist

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

[mscdex]

Doesn't this make them appear identical to anonymous functions now?

[BridgeAR]

@mscdex that is correct. The same will almost always be assigned and anonymous functions are rare for things to inspect.

What about highlighting anonymous functions with either a different color or by explicitly mentioning that it's anonymous? That way the average would be less verbose and less duplication.

If anyone has some further suggestions how to improve it further by reducing the overhead, that would be great! :-)

[BridgeAR]

@nodejs/util PTAL

[BridgeAR]

This could use some reviews.

[BridgeAR]

@nodejs/util @mscdex @joyeecheung I thought about distinguishing anonymous functions and other functions again and the best I could think of is switching the brackets. We could just use <Function> instead of [Function], document the difference and use [Anonymous function] for all anonymous functions. That way it's explicit that a function is anonymous and it's possible to see the difference between removed function names in case it's a property.

Any opinions on that?

[mscdex]

To me it seems like anonymous functions as property values would be more common (especially when considering object literal declarations), so keeping things the way they are now is more ideal IMO.

[BridgeAR]

@mscdex anonymous functions as property values are normally using the property name as function name:

> { a() {}, b: () => {}, c: function() {} }
{ a: [Function: a], b: [Function: b], c: [Function: c] }

[joyeecheung]

Can we use (anonymous) like how it’s used in the inspector?

[BridgeAR]

I updated the branch with my suggestions. That is now a somewhat more involved change than before, since I also added null prototype support and made functions more tampering proof besides just marking anonymous functions as such.

@joyeecheung PTAL. Using (anonymous) alone would not be sufficient, since there are different types of functions (async and generator ones). If you suggested to change it to: [Function: (anonymous)], that would be a possibility while it would be possible to set that as name with defineProperty (even though it would be unlikely).

[BridgeAR]

The first commit is the original one. I did not change that during the rebase. The first two commits should be squashed later but I thought I keep it around for easier reviews.

[joyeecheung]

If you suggested to change it to: [Function: (anonymous)], that would be a possibility while it would be possible to set that as name with defineProperty (even though it would be unlikely).

Yes, that’s what I meant, because the first part is supposed to be the type isn’t it?

I think it’s fine to use it even though the name can be set using defineProperty, Chrome still does this way so this is a format people are familiar with. With the Anonymous prefix and the brackets it’s less obvious without docs.

[mscdex]

I also agree that putting Anonymous in front is less consistent. Instead of [Function: (anonymous)] you could leave out the :, that would avoid ambiguity if someone named their function (anonymous).

[BridgeAR]

Thanks for your review! I updated it to the suggestion.

[addaleax]

This is definitely semver-major, btw. We’ve even had issues when the function name display changed during a semver-minor V8 upgrade in the past.

[addaleax]

I don’t think people get a good intuition for what the difference between <…> and […] mean, respectively, when inspecting values.

I also don’t personally see the issue with always printing the function name, even if that is a bit verbose/redundant – it seems more consistent than what this PR does, and consistency in util.inspect() is important for intuitive understanding.

[BridgeAR]

@addaleax

We’ve even had issues when the function name display changed during a semver-minor V8 upgrade in the past.

But that was about String(function), not about util.inspect() which does something very different.

This is definitely semver-major

I agree due to the change of the brackets. Without that, I don't think that it would have to be semver-major.

I don’t think people get a good intuition for what the difference between <…> and […] mean, respectively, when inspecting values.

Since I also added anonymous in case a function is anonymous, I guess we could also stick to the square brackets as before. I was reluctant to change the brackets first due to the non-intuitive character but I think as soon as people saw the difference a couple of times, it would be something good. Especially to differentiate it with older Node.js versions (that is my main concern about keeping the square brackets).
Do you have a suggestion how to improve it further? I personally think this change became pretty decent.

[addaleax]

We’ve even had issues when the function name display changed during a semver-minor V8 upgrade in the past.

But that was about String(function), not about util.inspect() which does something very different.

No, I was talking about util.inspect() – iirc, when V8 started inferring function names for properties, it showed up in util.inspect() output that people were testing against.

This is definitely semver-major

I agree due to the change of the brackets. Without that, I don't think that it would have to be semver-major.

I think this is semver-major either way, it changes util.inspect() output rather significantly (the number of tests that had to be changed for that is a pretty good indicator), and while we discourage people from using it programatically, I think we should err on the side of caution. That also gives people a longer time to figure out whether this is a change that improves util.inspect() before we release it.

I don’t think people get a good intuition for what the difference between <…> and […] mean, respectively, when inspecting values.

Since I also added anonymous in case a function is anonymous, I guess we could also stick to the square brackets as before

I would prefer that, yes.

Do you have a suggestion how to improve it further? I personally think this change became pretty decent.

I mean, still, in the end it seems like this PR removes redundancy in one way and adds it in another way, and gives up consistency during that? I don’t have strong opinions about this besides that I find the different brackets confusing, so I’d defer to others…

[BridgeAR]

No, I was talking about util.inspect() – iirc, when V8 started inferring function names for properties

I just looked through a lot of issues but I could not find anything related. Do you have a reference for it for me?

the number of tests that had to be changed for that is a pretty good indicator

This is actually a combination of multiple minor changes. Should I maybe just move a few changes out in a separate PR? For example everything besides the anonymous part and the property name part?

Since I also added anonymous in case a function is anonymous, I guess we could also stick to the square brackets as before

I would prefer that, yes.

OK, but this will conflict with older Node.js versions in that case. So if we have another solution / idea, I would rather do that.

I mean, still, in the end it seems like this PR removes redundancy in one way and adds it in another way, and gives up consistency during that? I don’t have strong opinions about this besides that I find the different brackets confusing, so I’d defer to others…

It adds the redundancy for anonymous functions but that should be less common than having a function as property name. It is also more explicit in case of anonymous functions what I personally prefer.

---

CITGM https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1830/ ✔️

[addaleax]

@BridgeAR 9940766 is essentially what I was referring to, but I also remember something this impacting outside projects. (I vaguely remember it being one of @bcoe’s projects that was affected by util.inspect() changes but I could be wrong).

It adds the redundancy for anonymous functions but that should be less common than having a function as property name.

Fwiw, I’d think that anonymous function are generally more common than objects with enumerable function properties.

[BridgeAR]

9940766 is essentially what I was referring to

I understand what change you meant but I can't find any issues or comments about trouble due to this change.

Fwiw, I’d think that anonymous function are generally more common than objects with enumerable function properties.

I guess in general that might be correct but those are normally not logged, are they? (I can mostly think of functions logged as object value or in case the function is assigned directly to a variable. Both of these cases infer the function name in almost all cases.)

[thefourtheye]

Instead of [Function: (anonymous)] you could leave out the :, that would avoid ambiguity if someone named their function (anonymous)

Isn't naming a function (anonymous) syntactically wrong?

[BridgeAR]

@thefourtheye

Isn't naming a function (anonymous) syntactically wrong?

It's not possible to name a function like that when creating them but it's possible to change the name later on by using Object.defineProperty.

[BridgeAR]

Landed in d0667e8 🎉

@addaleax since we try to backport semver-major PRs in non-breaking ways: I'll open a backport for v12 that backports the prototype part but not the anonymous part. Does that sound fine to you?

[addaleax]

@BridgeAR Yes, I think so.

[targos]

@BridgeAR please do :) This is blocking #27522.

[BridgeAR]

@targos I opened a backport (#27570) but the backport will not resolve that conflict. That has to be done manually.

[BridgeAR]

I just added the backport for #27522 to the same PR.