Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

console, doc, util: console.log() and util.format() are wrongly documented and may be inconsistent #13908

Closed
vsemozhetbyt opened this issue Jun 25, 2017 · 12 comments
Labels
console Issues and PRs related to the console subsystem. doc Issues and PRs related to the documentations. util Issues and PRs related to the built-in util module.

Comments

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Jun 25, 2017

This is an issue that superseded #6341, Previously I thought this was a pure doc issue, now it seems to me that it is a more complicated problem that needs more discussion.

The both docs fragments (here and here) are wrong: util.inspect() is not called on each argument if the first argument is a non-format string. This is true if the first argument is not a string at all (see this path in code). If the first argument is a non-format string, util.inspect() is called only for arguments whose typeof is 'object' or 'symbol' (except null) — see this path in code.

Currently, I've found out that this impacts the output with String and Function arguments (watch out for quotes in the output for strings and absolutely different output for functions):

> console.log(1, 'str'); // .inspect()
1 'str'
> console.log('str', 'str'); // no .inspect()
str str

> console.log(1, () => true); // .inspect()
1 [Function]
> console.log('str', () => true); // no .inspect()
str () => true

Maybe there are other diferences.

Possible solutions:

  1. Document this difference.
  2. As it seems to be a confusing behavior, maybe we need to unify this with a semver-major fix.

UPD: This fragment is more correct:

> console.log(1, 'str2'); // .inspect()
1 'str2'
> console.log('str %s', 'str1', 'str2'); // no .inspect()
str str1 str2

> console.log(1, () => true); // .inspect()
1 [Function]
> console.log('str %s', 'str1', () => true); // no .inspect()
str str1 () => true

As you can see, util.inspect() is not called for excessive String and Function arguments here.

However, this doc fragment can also be improved, as functions are objects (maybe typeof should be mentioned).

@vsemozhetbyt vsemozhetbyt added console Issues and PRs related to the console subsystem. doc Issues and PRs related to the documentations. util Issues and PRs related to the built-in util module. labels Jun 25, 2017
@vsemozhetbyt vsemozhetbyt changed the title console.log and util.format with non-format string console, doc, util: console.log() and util.format() are wrongly documented Jun 25, 2017
@vsemozhetbyt vsemozhetbyt changed the title console, doc, util: console.log() and util.format() are wrongly documented console, doc, util: console.log() and util.format() are wrongly documented and may be inconsistent Jun 25, 2017
@nattelog
Copy link
Contributor

nattelog commented Jun 25, 2017

IMO if the first argument is not a format string, i.e. it could be a string type but with no %-characters (unless you \-escape them), all arguments should be called with util.inspect() for consistency purpose.

@vsemozhetbyt
Copy link
Contributor Author

I am really not sure if this should also be applied for all excessive arguments after a format string. Both ways can be considered somehow inconsistent.

@nattelog
Copy link
Contributor

@vsemozhetbyt What would be the option? To throw an error?

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Jun 25, 2017

I think throwing would be needlessly obstructive. Maybe this case should be just documented more clearly. Because if we apply inspect-all-format-not-respective-arguments here, this could be more confusing:

> console.log('foo %s', 'str', 'str');
foo str 'str' // instead of current: foo str str

@nattelog
Copy link
Contributor

I think it seems consistent. If you for any case would want to write that string you simply write console.log('foo %s str', 'str')?

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Jun 25, 2017

Or console.log('foo %s %s', 'str', 'str');. Well, maybe this makes sense.

However, this should be widely discussed, as these ones are rather heavily used functions across the userland.

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Jun 27, 2017

I do not know how this can be worded in good English with not so confusing examples, but this is some dry summation.

Various sets of arguments with different behavior and results.

  1. String with N placeholders + N arguments = string where all placeholders are replaced with converted arguments.

  2. String with N placeholders + zero or less than N arguments = string where placeholders without corresponding arguments are not replaced.

  3. String with N placeholders (including zero) + more than N arguments = string where placeholders are replaced with converted corresponding arguments + space + extra arguments coerced into strings and concatenated with spaces (util.inspect() is used for arguments whose typeof is 'object' (except null) or 'symbol').

  4. Non-string argument + zero or more arguments = string where all arguments coerced into strings and concatenated with spaces (each argument is converted to a string using util.inspect())

  5. No arguments = empty string (or '\n' from console.log()). The doc for util.format() may also be corrected here, as the first argument is not mandatory (this is a useless case, nevertheless, this may be clarified).

const f = () => {};
const s = 'str';
const o = new Array(3);

console.log('%j %s %j');
console.log('%j %s %j', f);

console.log('%j %s %j', f, s, o);

console.log('%j %s', f, s, o);
console.log('', f, s, o);

console.log(f, s, o);

console.log();
%j %s %j
undefined %s %j

undefined str [null,null,null]

undefined str [ <3 empty items> ]
 () => {} str [ <3 empty items> ]

[Function: f] 'str' [ <3 empty items> ]

< '\n' >
const f = () => {};
const s = 'str';
const o = new Array(3);

> util.format('%j %s %j');
'%j %s %j'
> util.format('%j %s %j', f);
'undefined %s %j'

> util.format('%j %s %j', f, s, o);
'undefined str [null,null,null]'

> util.format('%j %s', f, s, o);
'undefined str [ <3 empty items> ]'
> util.format('', f, s, o);
' () => {} str [ <3 empty items> ]'

> util.format(f, s, o);
'[Function: f] \'str\' [ <3 empty items> ]'

> util.format();
''

cc @nodejs/documentation, @bnoordhuis, @cjihrig, @evanlucas

@nattelog
Copy link
Contributor

Would it be of interest if I submitted a PR with this proposal realised?

@vsemozhetbyt
Copy link
Contributor Author

@nattelog I hope a doc PR would draw some more attention at least)

@nattelog
Copy link
Contributor

@vsemozhetbyt And by doc PR you mean one where I document the util.format() function?

@nattelog
Copy link
Contributor

@vsemozhetbyt I added a small check to this if statement:

if (typeof f !== 'string' || !/%\w/.test(f))

To guard against non-format strings. However, it makes many tests fail. See this for instance:

=== release test-cli-eval ===                            
Path: parallel/test-cli-eval
assert.js:60
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: '\'start\'\n\'beforeExit\'\n\'exit\'\n' === 'start\nbeforeExit\nexit\n'
    at Object.<anonymous> (/Users/nattelog/Projekt/node/test/parallel/test-cli-eval.js:174:10)
    at Module._compile (module.js:569:30)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:503:32)
    at tryModuleLoad (module.js:466:12)
    at Function.Module._load (module.js:458:3)
    at Function.Module.runMain (module.js:605:10)
    at startup (bootstrap_node.js:158:16)
    at bootstrap_node.js:575:3
Command: out/Release/node /Users/nattelog/Projekt/node/test/parallel/test-cli-eval.js

Since util.inspect() will be called on every argument passed now, there will be more cases of util.format() outputs with an extra '-character around strings which will cause assertion errors like the one in the example.

If this change will be implemented, many test cases must be changed as well.

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Jun 30, 2017

@nattelog I think the doc for util.format() (and maybe some cleanup and hint in the doc for console.log()) would be a good start and a base for backports to docs for all actual versions.

If you like to unify the API behavior, it would be better to raise a separate PR for this.

Yes, this change will be a drastic one and many tests will need fixing, so this will be some time-consuming project. And the guard also should be more careful (currently it would erroneously consider strings like ' %%s ' or ' %_wrong ' as format strings ).

So maybe we should get some +1 from CTC members before somebody dares to undertake this fix.

nattelog added a commit to nattelog/node that referenced this issue Jul 1, 2017
Add clarification to the documentation on util.format() and console.log()
regarding how excessive arguments are treated when the first argument is
a non-format string compared to when it is not a string at all.

Fixes: nodejs#13908
nattelog added a commit to nattelog/node that referenced this issue Jul 1, 2017
Rephrased how excessive arguments to the format string are concatenated.

Fixes: nodejs#13908
addaleax pushed a commit that referenced this issue Jul 11, 2017
Add clarification to the documentation on util.format()
and console.log() regarding how excessive arguments are treated
when the first argument is a non-format string
compared to when it is not a string at all.

PR-URL: #14027
Fixes: #13908
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
addaleax pushed a commit that referenced this issue Jul 18, 2017
Add clarification to the documentation on util.format()
and console.log() regarding how excessive arguments are treated
when the first argument is a non-format string
compared to when it is not a string at all.

PR-URL: #14027
Fixes: #13908
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Fishrock123 pushed a commit that referenced this issue Jul 19, 2017
Add clarification to the documentation on util.format()
and console.log() regarding how excessive arguments are treated
when the first argument is a non-format string
compared to when it is not a string at all.

PR-URL: #14027
Fixes: #13908
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 15, 2017
Add clarification to the documentation on util.format()
and console.log() regarding how excessive arguments are treated
when the first argument is a non-format string
compared to when it is not a string at all.

PR-URL: #14027
Fixes: #13908
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 15, 2017
Add clarification to the documentation on util.format()
and console.log() regarding how excessive arguments are treated
when the first argument is a non-format string
compared to when it is not a string at all.

PR-URL: #14027
Fixes: #13908
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 15, 2017
Add clarification to the documentation on util.format()
and console.log() regarding how excessive arguments are treated
when the first argument is a non-format string
compared to when it is not a string at all.

PR-URL: #14027
Fixes: #13908
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 16, 2017
Add clarification to the documentation on util.format()
and console.log() regarding how excessive arguments are treated
when the first argument is a non-format string
compared to when it is not a string at all.

PR-URL: #14027
Fixes: #13908
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
This was referenced Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
console Issues and PRs related to the console subsystem. doc Issues and PRs related to the documentations. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants