From c1b9be53c89a7ac11c01905a4477785a6154512b Mon Sep 17 00:00:00 2001
From: Roman Reiss <me@silverwind.io>
Date: Sat, 29 Sep 2018 14:41:36 +0200
Subject: [PATCH] util: treat format arguments equally

Two changes here which bring us closer to the console standard:

- Arguments to `util.format` are no longer formatted differently
  depending on their order, with format strings being an exception.
- Format specifier formatting is now only triggered if the string
  actually contains a format string.

Under the hood, we now use a single shared function to format the given
arguments which will make the code easier to read and modify.

PR-URL: https://github.com/nodejs/node/pull/23162
Fixes: https://github.com/nodejs/node/issues/23137
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
---
 doc/api/util.md                   |  50 +++++----
 lib/util.js                       | 179 ++++++++++++++++--------------
 test/parallel/test-util-format.js |  16 +++
 3 files changed, 144 insertions(+), 101 deletions(-)

diff --git a/doc/api/util.md b/doc/api/util.md
index c1fc606a517..3def79546b6 100644
--- a/doc/api/util.md
+++ b/doc/api/util.md
@@ -183,6 +183,17 @@ property take precedence over `--trace-deprecation` and
 <!-- YAML
 added: v0.5.3
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/23162
+    description: The `format` argument is now only taken as such if it actually
+                 contains format specifiers.
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/23162
+    description: If the `format` argument is not a format string, the output
+                 string's formatting is no longer dependent on the type of the
+                 first argument. This change removes previously present quotes
+                 from strings that were being output when the first argument
+                 was not a string.
   - version: REPLACEME
     pr-url: https://github.com/nodejs/node/pull/17907
     description: The `%o` specifier's `depth` option will now fall back to the
@@ -195,11 +206,9 @@ changes:
 * `format` {string} A `printf`-like format string.
 
 The `util.format()` method returns a formatted string using the first argument
-as a `printf`-like format.
-
-The first argument is a string containing zero or more *placeholder* tokens.
-Each placeholder token is replaced with the converted value from the
-corresponding argument. Supported placeholders are:
+as a `printf`-like format string which can contain zero or more format
+specifiers. Each specifier is replaced with the converted value from the
+corresponding argument. Supported specifiers are:
 
 * `%s` - `String`.
 * `%d` - `Number` (integer or floating point value) or `BigInt`.
@@ -218,37 +227,40 @@ contains circular references.
 * `%%` - single percent sign (`'%'`). This does not consume an argument.
 * Returns: {string} The formatted string
 
-If the placeholder does not have a corresponding argument, the placeholder is
-not replaced.
+If a specifier does not have a corresponding argument, it is not replaced:
 
 ```js
 util.format('%s:%s', 'foo');
 // Returns: 'foo:%s'
 ```
 
-If there are more arguments passed to the `util.format()` method than the number
-of placeholders, the extra arguments are coerced into strings then concatenated
-to the returned string, each delimited by a space. Excessive arguments whose
-`typeof` is `'object'` or `'symbol'` (except `null`) will be transformed by
-`util.inspect()`.
+Values that are not part of the format string are formatted using
+`util.inspect()` if their type is either `'object'`, `'symbol'`, `'function'`
+or `'number'` and using `String()` in all other cases.
+
+If there are more arguments passed to the `util.format()` method than the
+number of specifiers, the extra arguments are concatenated to the returned
+string, separated by spaces:
 
 ```js
-util.format('%s:%s', 'foo', 'bar', 'baz'); // 'foo:bar baz'
+util.format('%s:%s', 'foo', 'bar', 'baz');
+// Returns: 'foo:bar baz'
 ```
 
-If the first argument is not a string then `util.format()` returns
-a string that is the concatenation of all arguments separated by spaces.
-Each argument is converted to a string using `util.inspect()`.
+If the first argument does not contain a valid format specifier, `util.format()`
+returns a string that is the concatenation of all arguments separated by spaces:
 
 ```js
-util.format(1, 2, 3); // '1 2 3'
+util.format(1, 2, 3);
+// Returns: '1 2 3'
 ```
 
 If only one argument is passed to `util.format()`, it is returned as it is
-without any formatting.
+without any formatting:
 
 ```js
-util.format('%% %s'); // '%% %s'
+util.format('%% %s');
+// Returns: '%% %s'
 ```
 
 Please note that `util.format()` is a synchronous method that is mainly
diff --git a/lib/util.js b/lib/util.js
index 2e42beb58b2..22c2b260daf 100644
--- a/lib/util.js
+++ b/lib/util.js
@@ -72,98 +72,113 @@ function format(...args) {
   return formatWithOptions(emptyOptions, ...args);
 }
 
-function formatWithOptions(inspectOptions, f) {
-  let i, tempStr;
-  if (typeof f !== 'string') {
-    if (arguments.length === 1) return '';
-    let res = '';
-    for (i = 1; i < arguments.length - 1; i++) {
-      res += inspect(arguments[i], inspectOptions);
-      res += ' ';
-    }
-    res += inspect(arguments[i], inspectOptions);
-    return res;
+function formatValue(val, inspectOptions) {
+  const inspectTypes = ['object', 'symbol', 'function', 'number'];
+
+  if (inspectTypes.includes(typeof val)) {
+    return inspect(val, inspectOptions);
+  } else {
+    return String(val);
+  }
+}
+
+function formatWithOptions(inspectOptions, ...args) {
+  const first = args[0];
+  const parts = [];
+
+  const firstIsString = typeof first === 'string';
+
+  if (firstIsString && args.length === 1) {
+    return first;
   }
 
-  if (arguments.length === 2) return f;
+  if (firstIsString && /%[sjdOoif%]/.test(first)) {
+    let i, tempStr;
+    let str = '';
+    let a = 1;
+    let lastPos = 0;
 
-  let str = '';
-  let a = 2;
-  let lastPos = 0;
-  for (i = 0; i < f.length - 1; i++) {
-    if (f.charCodeAt(i) === 37) { // '%'
-      const nextChar = f.charCodeAt(++i);
-      if (a !== arguments.length) {
-        switch (nextChar) {
-          case 115: // 's'
-            tempStr = String(arguments[a++]);
-            break;
-          case 106: // 'j'
-            tempStr = tryStringify(arguments[a++]);
-            break;
-          case 100: // 'd'
-            const tempNum = arguments[a++];
-            // eslint-disable-next-line valid-typeof
-            if (typeof tempNum === 'bigint') {
-              tempStr = `${tempNum}n`;
-            } else {
-              tempStr = `${Number(tempNum)}`;
+    for (i = 0; i < first.length - 1; i++) {
+      if (first.charCodeAt(i) === 37) { // '%'
+        const nextChar = first.charCodeAt(++i);
+        if (a !== args.length) {
+          switch (nextChar) {
+            case 115: // 's'
+              tempStr = String(args[a++]);
+              break;
+            case 106: // 'j'
+              tempStr = tryStringify(args[a++]);
+              break;
+            case 100: // 'd'
+              const tempNum = args[a++];
+              // eslint-disable-next-line valid-typeof
+              if (typeof tempNum === 'bigint') {
+                tempStr = `${tempNum}n`;
+              } else {
+                tempStr = `${Number(tempNum)}`;
+              }
+              break;
+            case 79: // 'O'
+              tempStr = inspect(args[a++], inspectOptions);
+              break;
+            case 111: // 'o'
+            {
+              const opts = Object.assign({}, inspectOptions, {
+                showHidden: true,
+                showProxy: true,
+                depth: 4
+              });
+              tempStr = inspect(args[a++], opts);
+              break;
             }
-            break;
-          case 79: // 'O'
-            tempStr = inspect(arguments[a++], inspectOptions);
-            break;
-          case 111: // 'o'
-          {
-            const opts = Object.assign({}, inspectOptions, {
-              showHidden: true,
-              showProxy: true
-            });
-            tempStr = inspect(arguments[a++], opts);
-            break;
+            case 105: // 'i'
+              const tempInteger = args[a++];
+              // eslint-disable-next-line valid-typeof
+              if (typeof tempInteger === 'bigint') {
+                tempStr = `${tempInteger}n`;
+              } else {
+                tempStr = `${parseInt(tempInteger)}`;
+              }
+              break;
+            case 102: // 'f'
+              tempStr = `${parseFloat(args[a++])}`;
+              break;
+            case 37: // '%'
+              str += first.slice(lastPos, i);
+              lastPos = i + 1;
+              continue;
+            default: // any other character is not a correct placeholder
+              continue;
           }
-          case 105: // 'i'
-            const tempInteger = arguments[a++];
-            // eslint-disable-next-line valid-typeof
-            if (typeof tempInteger === 'bigint') {
-              tempStr = `${tempInteger}n`;
-            } else {
-              tempStr = `${parseInt(tempInteger)}`;
-            }
-            break;
-          case 102: // 'f'
-            tempStr = `${parseFloat(arguments[a++])}`;
-            break;
-          case 37: // '%'
-            str += f.slice(lastPos, i);
-            lastPos = i + 1;
-            continue;
-          default: // any other character is not a correct placeholder
-            continue;
+          if (lastPos !== i - 1) {
+            str += first.slice(lastPos, i - 1);
+          }
+          str += tempStr;
+          lastPos = i + 1;
+        } else if (nextChar === 37) {
+          str += first.slice(lastPos, i);
+          lastPos = i + 1;
         }
-        if (lastPos !== i - 1)
-          str += f.slice(lastPos, i - 1);
-        str += tempStr;
-        lastPos = i + 1;
-      } else if (nextChar === 37) {
-        str += f.slice(lastPos, i);
-        lastPos = i + 1;
       }
     }
-  }
-  if (lastPos === 0)
-    str = f;
-  else if (lastPos < f.length)
-    str += f.slice(lastPos);
-  while (a < arguments.length) {
-    const x = arguments[a++];
-    if ((typeof x !== 'object' && typeof x !== 'symbol') || x === null) {
-      str += ` ${x}`;
-    } else {
-      str += ` ${inspect(x, inspectOptions)}`;
+    if (lastPos === 0) {
+      str = first;
+    } else if (lastPos < first.length) {
+      str += first.slice(lastPos);
+    }
+
+    parts.push(str);
+    while (a < args.length) {
+      parts.push(formatValue(args[a], inspectOptions));
+      a++;
+    }
+  } else {
+    for (const arg of args) {
+      parts.push(formatValue(arg, inspectOptions));
     }
   }
-  return str;
+
+  return parts.join(' ');
 }
 
 const debugs = {};
diff --git a/test/parallel/test-util-format.js b/test/parallel/test-util-format.js
index 0c4ba82fec8..2ca8e0857f3 100644
--- a/test/parallel/test-util-format.js
+++ b/test/parallel/test-util-format.js
@@ -273,6 +273,10 @@ assert.strictEqual(util.format('percent: %d%, fraction: %d', 10, 0.1),
                    'percent: 10%, fraction: 0.1');
 assert.strictEqual(util.format('abc%', 1), 'abc% 1');
 
+// Additional arguments after format specifiers
+assert.strictEqual(util.format('%i', 1, 'number'), '1 number');
+assert.strictEqual(util.format('%i', 1, () => {}), '1 [Function]');
+
 {
   const o = {};
   o.o = o;
@@ -315,3 +319,15 @@ function BadCustomError(msg) {
 util.inherits(BadCustomError, Error);
 assert.strictEqual(util.format(new BadCustomError('foo')),
                    '[BadCustomError: foo]');
+
+// The format of arguments should not depend on type of the first argument
+assert.strictEqual(util.format('1', '1'), '1 1');
+assert.strictEqual(util.format(1, '1'), '1 1');
+assert.strictEqual(util.format('1', 1), '1 1');
+assert.strictEqual(util.format(1, 1), '1 1');
+assert.strictEqual(util.format('1', () => {}), '1 [Function]');
+assert.strictEqual(util.format(1, () => {}), '1 [Function]');
+assert.strictEqual(util.format('1', "'"), "1 '");
+assert.strictEqual(util.format(1, "'"), "1 '");
+assert.strictEqual(util.format('1', 'number'), '1 number');
+assert.strictEqual(util.format(1, 'number'), '1 number');