util: treat format arguments equally, add format string detection

[silverwind]

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

Previously, a string first argument was treated as a format string even if it did not actually contain format specifiers. Now it will only be treated as a format string when it contains format specifiers which is in line with the Console Standard.

Additionally, this eliminates output inconsistencies in the non-format-string case where elements were output differently depending on their position in the argument list.

[nodejs-github-bot]

@silverwind build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1061/pipeline

[vsemozhetbyt]

Should we update this note with something like this:

If the first argument is not a string then... ->
If the first argument is not a string or it does not contain any format specifiers then...?

[silverwind]

@vsemozhetbyt thanks, updated docs quite a bit to be more accurate and also renamed placeholders to format specifiers.
@cjihrig you sure about this semver-major label? All our current tests pass and I think the only real change is the escaping of ' in strings.

[cjihrig]

I think it's an edge case, but still needs to be treated as semver major just to be safe. Our own test suite has not been a great measuring stick in the past for these things. If others feel strongly that it's not semver major, feel free to remove the label.

[silverwind]

On second thought, I'm fine with keeping it major.

[silverwind]

citgm: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1566/

[vsemozhetbyt]

From citgm, something changes independently of the first string parameter content:

 !!! Invalid input or expected stdout
 ---INPUT---
 {
     var o = {
         n: NaN
     };
     console.log(o.n == o.n, o.n === o.n, o.n != o.n, o.n !== o.n, typeof o.n);
 }
 ---EXPECTED STDOUT---
 false false true true 'number'
 ---ACTUAL STDOUT---
 false false true true number

[silverwind]

Another one:

 ---INPUT---
 {
     var a = "32";
     !function() {
         var b = a + 1;
         !function(a) {
             console.log(a++, b);
         }(0);
     }();
 }
 ---EXPECTED STDOUT---
 0 '321'
 ---ACTUAL STDOUT---
 0 321

That may be because I use inspect instead of String on numbers, which I recall was necessary to get some of our own tests to pass. Will check.

[silverwind]

Looks like the old implementation did use inspect on all arguments if the first argument is not a string. When first was a string, String was used on types other than objects and symbols.

Guess I have to reimplement this inconsistency for the sake of compatibilty.

[silverwind]

I cannot fix above errors without re-introducing the issue that this was meant to fix.

Our current implementation of util.format outputs types other then object and symbol using String() when the first argument is a string. When the first argument is not a string, inspect is used on all arguments, which results in inconsistent output like this:

My implementation removes this inconsistency by always formatting arguments the same, regardless of their position in the argument list:

I think this is the best compromise we can do and it's worth the breaking change in the console output.

A even better way could be to run all types through util.inspect, but that would mean we'd have to remove string quotes and all strings in console output would become colored, which would probably break a ton of things.

[silverwind]

Added a second changelog entry to clarify the details on the formatting change. If we disregard color, the only real difference in console/format output is on the arguments [nonstring, string] where previously, string would have been logged with quotes which are now gone.

[addaleax]

Looks like this needs a rebase? :/

[refack]

Maybe move this to the next paragraph, that explains what happens to those.

[silverwind]

Yeah makes sense, I moved it after the code block.

[silverwind]

Rebased and addressed @refack's comment on docs. Also, new commit message.

[silverwind]

Thanks, Landed in c1b9be5.