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.

Sign up for GitHub

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

util: format specifier %d use parseInt #23321

Closed
wants to merge 2 commits into from
Closed

util: format specifier %d use parseInt #23321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7f649ae
util: format specifier `%d` use parseInt
Oct 7, 2018
cec2e69
test: fix %d specifier for BigInt
Oct 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion doc/api/util.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ 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`.
* `%d` - Integer or `BigInt`.
* `%i` - Integer or `BigInt`.
* `%f` - Floating point value.
* `%j` - JSON. Replaced with the string `'[Circular]'` if the argument
Expand Down
10 changes: 1 addition & 9 deletions lib/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,15 +109,6 @@ function formatWithOptions(inspectOptions, ...args) {
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;
Expand All @@ -131,6 +122,7 @@ function formatWithOptions(inspectOptions, ...args) {
tempStr = inspect(args[a++], opts);
break;
}
case 100: // 'd'
case 105: // 'i'
const tempInteger = args[a++];
// eslint-disable-next-line valid-typeof
Expand Down
12 changes: 6 additions & 6 deletions test/parallel/test-util-format.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,20 +57,20 @@ assert.throws(
}
);

// Number format specifier
// Integer format specifier
assert.strictEqual(util.format('%d'), '%d');
assert.strictEqual(util.format('%d', 42.0), '42');
assert.strictEqual(util.format('%d', 42), '42');
assert.strictEqual(util.format('%d', '42'), '42');
assert.strictEqual(util.format('%d', '42.0'), '42');
assert.strictEqual(util.format('%d', 1.5), '1.5');
assert.strictEqual(util.format('%d', -0.5), '-0.5');
assert.strictEqual(util.format('%d', ''), '0');
assert.strictEqual(util.format('%d', 1.5), '1');
assert.strictEqual(util.format('%d', -0.5), '0');
assert.strictEqual(util.format('%d', ''), 'NaN');
assert.strictEqual(util.format('%d %d', 42, 43), '42 43');
assert.strictEqual(util.format('%d %d', 42), '42 %d');
assert.strictEqual(
util.format('%d', 1180591620717411303424),
'1.1805916207174113e+21'
'1'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m a bit surprised by this one, tbh … at least for me locally, `${parseInt('1180591620717411303424')}` === '1.1805916207174113e+21'? Do you know why there’s a discrepancy?

Interestingly, Firefox seems to provide 0 here, and Chrome the exponential notation…

Copy link
Member

@targos targos Oct 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens here is not ${parseInt('1180591620717411303424')} but ${parseInt(1180591620717411303424)} which is equivalent to ${parseInt('1.1805916207174113e+21')} and returns 1.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So … just to make sure, these are browser bugs?

Copy link
Member

@targos targos Oct 17, 2018 •
edited by evanlucas
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure... It could also be a spec bug because no browser has the "correct" behavior.
To summarize, with console.log('%d', 1180591620717411303424):

  • Edge and Chrome: 1.1805916207174113e+21
  • Firefox: 0
  • Safari: 1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me it seems like the browsers deviate from the spec in this case. However, it does seem to be a a good idea to follow Chrome and Edge in this case. Especially, since the spec does not seem to follow any browser implementation in this case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Safari logs 1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 should be correct according to the latest parseInt spec. NumberToString has to convert the number to expotential notation and then the following spec text applies:

If S contains a code unit that is not a radix-R digit, let Z be the substring of S consisting of all code units before the first such code unit

Let mathInt be the mathematical integer value that is represented by Z

. is not a radix-R digit, so parsing stops there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in summary for console.log('%d', number):

  • Chrome uses Number
  • Edge uses Number
  • Safari uses parseInt (this is spec-compliant)
  • Firefox uses neither Number nor parseInt

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chrome may not use Number for %d.
at least chrome 70(stable) on my macbook, I think %d is not equal Number.
the results are:
console.log('%d', 1.2): 1
Number(1.2): 1.2

);
assert.strictEqual(
util.format('%d', 1180591620717411303424n),
Expand Down Expand Up @@ -270,7 +270,7 @@ assert.strictEqual(util.format('o: %O, a: %O'), 'o: %O, a: %O');
// Invalid format specifiers
assert.strictEqual(util.format('a% b', 'x'), 'a% b x');
assert.strictEqual(util.format('percent: %d%, fraction: %d', 10, 0.1),
'percent: 10%, fraction: 0.1');
'percent: 10%, fraction: 0');
assert.strictEqual(util.format('abc%', 1), 'abc% 1');

// Additional arguments after format specifiers
Expand Down