console,util: revert #23162

[BridgeAR]

Edit by @Fishrock123: issue description at #29592 (comment)

---

This reverts #23162 so that util.format() works as before.

Both TSC members (@addaleax and @jasnell) who gave their LG to the original PR seem to be in favor of reverting to the old behavior. I therefore went ahead and opened this PR.

I requested the review of all people being involved in #29539 to take a look at this.

Refs: #29539

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

[jasnell]

To be certain, I'm +1 on reverting in 12.x, I think there's still a discussion to be had about whether this should be reverted in master. Either way, the original should have been considered semver-major Scratch that, getting my issues mixed up lol ... this was originally semver-major :-) ... looking back over the history, yeah, this should be reverted

[silverwind]

I will not approve this. To me, the old behaviour was broken and inconsistent. Thought should be given if the revert itself is a semver-major given that 12.0.0 released five months ago with this change.

[jasnell]

I think considering this revert a semver-major may be appropriate

[silverwind]

Either way, the original should have been considered semver-major

You seem rather ignorant here based on the fact that this was all done orderly in a semver-major and released in 12.0.0. Your statement is just plain wrong.

[jasnell]

You seem rather ignorant here based on the fact that this was all done orderly in a semver-major and released in 12.0.0. Your statement is just plain wrong.

Excuse me, but that's rather rude. Please see my corrected comment.

[sam-github]

@silverwind, please be more patient. This issue is confusing, and not helped by having a mixture of incomplete and incorrect comments in the related discussions.

I did some digging, this is my understanding of what happened.

If the first argument is a string, its is treated as a format string, and any specifiers in it may consume some number of values which are formatted as specified. This is obviously as-expected.

The surprise is how string values that are not consumed by the format string are treated. They can be unconsumed for two reasons:

1. The first argument wasn't a string. Since it isn't a string, it doesn't have format specifiers, and consumes no values.
2. The first argument was a string, but there are more values than are consumed by the format specifiers.

How unconsumed arg were formatted used to depend on whether the first parameter was a string:

core/build (master $% u=) % nvm use 8; node -e "console.log(Symbol('not fmt'), 'unconsumed')"; node -e "console.log('fmt %s', 5, 'unconsumed')"
Now using node v8.16.1 (npm v6.4.1)
Symbol(not fmt) 'unconsumed'
fmt 5 unconsumed
core/build (master $% u=) % nvm use 10; node -e "console.log(Symbol('not fmt'), 'unconsumed')"; node -e "console.log('fmt %s', 5, 'unconsumed')"
Now using node v10.16.3 (npm v6.9.0)
Symbol(not fmt) 'unconsumed'
fmt 5 unconsumed
core/build (master $% u=) % nvm use 12; node -e "console.log(Symbol('not fmt'), 'unconsumed')"; node -e "console.log('fmt %s', 5, 'unconsumed')"
Now using node v12.10.0 (npm v6.10.3)
Symbol(not fmt) unconsumed
fmt 5 unconsumed

#23162 removed this inconsistency in how unconsumed string arguments were treated in 12.x, making them be treated uniformly in the two cases, by not inspecting them.

12 is also more consistent wrt. how unconsumed non-string values are treated (I'm not sure if this was because of #23162). 12 seems to convert to a string (String(unconsumed)) only when explicitly requested with a '%s' format, whereas 10 would do it based on whether the first arg was a format string or not:

core/build (master $% u=) % nvm use 10; node -e "console.log(console.log)"; node -e "console.log(Symbol('sym'), console.log)"; node -e "console.log('fmt %s', console.log)"; node -e "console.log('unconsumed', console.log)";
Now using node v10.16.3 (npm v6.9.0)
[Function: bound consoleCall]
Symbol(sym) [Function: bound consoleCall]
fmt function () { [native code] }
unconsumed function () { [native code] }

core/build (master $% u=) % nvm use 12; node -e "console.log(console.log)"; node -e "console.log(Symbol('sym'), console.log)"; node -e "console.log('fmt %s', console.log)"; node -e "console.log('unconsumed', console.log)";
Now using node v12.10.0 (npm v6.10.3)
[Function: bound consoleCall]
Symbol(sym) [Function: bound consoleCall]
fmt function () { [native code] }
unconsumed [Function: bound consoleCall]

I'm guessing that people using console.log() for debugging are expecting to see more "inspect"-like output (strings quoted), whereas people using console.log() for CLI output want to see more direct string printing (no added quotes). #23162 kept things the same for this second use case:

core/build (master $% u=) % nvm i 10; node -e "console.log('hello', 'my', 'friend')"
v10.16.3 is already installed.
Now using node v10.16.3 (npm v6.9.0)
hello my friend
core/build (master $% u=) % nvm i 12; node -e "console.log('hello', 'my', 'friend')"
v12.10.0 is already installed.
Now using node v12.10.0 (npm v6.10.3)
hello my friend

Summary: I think there are only three options for the next major release line

1. Unconsumed args are treated differently depending on whether a format string didn't consume them, or there was no format string to consume them (10.x and before does this)
2. Uniformly inspect them (no release line does this)
3. Uniformly print the (uninspected) value of the string (12.x does this)

[BridgeAR]

Rebased due to conflicts. PTAL

[nodejs-github-bot]

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

[Trott]

I interpret @silverwind's initial comment as blocking opposition to this, so I'm going to remove the author ready label.

@silverwind: If I'm wrong and you're OK with this (just perhaps not willing to approve it yourself) then please comment to that effect. Thanks!

[BridgeAR]

@refack PTAL since you also approved the original PR.

[BridgeAR]

@nodejs/tsc PTAL

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[Trott]

Adding tsc-agenda because it seems like a TSC vote will be required to decide whether this should land and be in the 13.0.0 release (over @silverwind's objection) or whether 13.0.0 should go out with the existing behavior.

@nodejs/tsc In the hopes of getting a quick resolution (since the release will happen in less than 3 weeks), please leave a reaction here. I think the four options cover all the likely possibilities, but if not, leave a comment!

🎉 = revert (land this)
❤️ = do not revert (close this without landing)
👀 = abstaining
😞 = I don't know what my vote is yet, will have a better idea after TSC talks about it

[mcollina]

LGTM

[Fishrock123]

FWIW Web browsers disagree on this behavior entirely.

• Chrome: acts like our old behavior
• Firefox: acts like our new behavior
• Safari: acts completely differently
  • Symbol(not fmt) – "unconsumed"
  • fmt 5 – "unconsumed"
• Edge: internal error on inspecting a Symbol, otherwise like our new behavior

I think the new behavior is more consistent. If it should be changed, I think the unconsumed case where a formatter is present should still inspect (like Safari). I suspect that would not be widely liked though, and so I think the present v12 behavior of non-quoting strings is better, if different.

ALSO: This was intentionally changed in a major 5 months ago which will be LTS. Reverting this in the following major seems like a mistake to me. I would suggest reverting in 12 but I bet the ship's sailed on that.

Edit, that being said, one might expect the following to work, although it does not:

console.log('fmt %s', 5, 'fmt2 %s', 6)

[Fishrock123]

Additionally, this issue is not particularly clear, so here is an explanation. The issue is what should happen to the second (string) argument in e.g. console.log(Symbol('not fmt'), 'unconsumed').

The old behavior ran it through util.inspect, where as the new prints the string directly:

Symbol(not fmt) 'unconsumed' // old
Symbol(not fmt) unconsumed   // new

Note that when this happens with a formatter, it always directly prints the string (with or without the change):

> console.log('fmt %s', 5, 'unconsumed')
fmt 5 unconsumed

[targos]

Thanks for the explanation @Fishrock123.
I think we should keep the current behavior. It is more consistent and predictable than the previous one.

[Trott]

(Rebased and force-pushed to resolve a pair of conflicts in the docs. Just formatting/lint stuff.)

[apapirovski]

Reverting this makes sense to me. Reverting as a semver-major, given that 12 is an LTS line, doesn’t. I think more of this discussion needs to cover that point.

I'm really torn on this. Going to remove my approval on the change

[sam-github]

After discussion, I'm at:

• The current 12.x behaviour is more consistent
• It breaks with 11.x (as our policy allows), and while I'm not sure the break was worth it, its too late now to change this in 12.x, without an extraordinary break of our process (we'd have to land a semver major in 12.x just before it goes LTS, in violation of our policy)
• Changing it back for 13.x just means 10.x would be one way, 12.x would be a different way, and 13.x/14.x would go back to 10.x's way.... but even if we are concerned that the 12.x change was unncessary, flipflopping back and forth doesn't seem helpful to anybody.

So, I'm arriving at do not land for this -- lets keep 12.x's behaviour now and forever.

[Trott]

Consensus at the TSC meeting is to not revert. This can remain open if @BridgeAR or anyone else wants to continue to make the case, but this won't land in 13.0.0. It will likely have to land in 14.0.0 if at all.

[jasnell]

Ping @BridgeAR ... what do you want to do with this one?

[jasnell]

Closing due to lack of continued activity on this one. Can reopen if it needs to be revisited