closure-compiler icon indicating copy to clipboard operation
closure-compiler copied to clipboard

I think parseInt radix should be optional

Open jimmywarting opened this issue 6 years ago • 12 comments
trafficstars

I understand why this is. But if feels like a very old legacy issue...

https://github.com/google/closure-compiler/blob/b29d09f56b56e5b3ea78ba0317ae579b837fd821/externs/es3.js#L289-L300

Nowdays all browser use radix 10 as default and has done so for a long time. And the likelihood of someone starting the number with 0 and using IE8 is very slim. Many have stop supporting old browsers

* @param {number=} base 

jimmywarting avatar Feb 12 '19 22:02 jimmywarting

Created Google internal issue http://b/124375924

EatingW avatar Feb 13 '19 18:02 EatingW

parseInt still infers the radix from the string contents when one is not provided.

In Chrome:

parseInt('0x20') === 32
parseInt('0x20', 10) === 0

This is a huge footgun if you are parsing user provided content. The only behavior that has changed is we no longer have to worry about an inferred octal radix.

ChadKillingsworth avatar Feb 16 '19 12:02 ChadKillingsworth

Thought it would only be a footgun for things like padded zeros like 010 0x91 is clearly a hex value. I probably would always would want to do both octal or hex parsing then.

How about this:

If you really want to parse octal or hex based on the leader, then pass undefined as the base

if the 2nd argument is undefined can the compiler then remove it instead of changing it to void 0?

Still would prefer it to be optional

jimmywarting avatar Feb 17 '19 10:02 jimmywarting

0x91 is clearly a hex value

Yes it is - and it's 100% clear when it's a string literal like that. The problem comes when it's not a string literal:

const value = document.getElementById('myinput').value;
console.log(parseInt(value)); // What's my radix?

ChadKillingsworth avatar Feb 18 '19 17:02 ChadKillingsworth

That would not be a problem if the input type is a number cuz that prevents the user from enter a x as for a number input I would also have done input.valueAsNumber cuz 1e2 could be valid number too. and parsing 1e2 is not the same as parseInt('1e2') then you need to use Number('1e2')

<input id="myinput" type="number" min="0" max="100" step="1">
const value = document.getElementById('myinput').valueAsNumber;
console.log(value); // What's my radix?

And when the input is invalid like 1e the input value is empty "" and valueAsNumber is NaN

And if you want to you can always use the pattern attribute to restrict it even further

It would also be less error prone since you can first validate the hole form or just the input before proceeding

jimmywarting avatar Feb 18 '19 17:02 jimmywarting

I realize it feels annoying to be forced to supply the second argument to parseInt(), but it avoids a footgun and leads to clearer (more explicit about what it's doing) code.

I don't think we're likely to change this anytime soon.

It is a barrier to compiling code not written with closure-compiler in mind, though, and longer term we'd like to remove such barriers. Of those barriers, this isn't the most important one, so again, probably not changing soon. Sorry.

brad4d avatar Feb 20 '19 02:02 brad4d

Just noticed this issue, had created one at https://github.com/google/closure-compiler/issues/3548.

I realize it feels annoying to be forced to supply the second argument to parseInt()

In our case, it is not as much the need to supply a second argument to parseInt(), but that when we do pass parseInt(..., undefined);, it results in Closure not optimizing this second argument away, but it generates code parseInt(..., void 0); in the output. Closure insists on adhering to a convention, but then does a sloppy job of minifying code when one does adopt that convention. Now that is what feels annoying, if something.

juj avatar Feb 15 '20 12:02 juj

Maybe some interested party could add a Peephole pass to remove an unnecessary second argument to parseInt. I don't think our core team is likely to prioritize doing that, though.

brad4d avatar Feb 20 '20 17:02 brad4d

Thinking more general, would it be a sensible optimization to always collapse ‘foo(1,2,undefined,undefined)’ into ‘foo(1,2)’? (there may be a need to disable such optimization in some cases, e.g. WebGL teximage2D function, but most of the time that feels like a safe optimization?)

juj avatar Feb 20 '20 20:02 juj

Thinking more general, would it be a sensible optimization to always collapse ‘foo(1,2,undefined,undefined)’ into ‘foo(1,2)’? (there may be a need to disable such optimization in some cases, e.g. WebGL teximage2D function, but most of the time that feels like a safe optimization?)

Many non-native functions and methods distinguish the two cases; also the Number function

> Number(undefined)
< NaN
> Number()
< 0

the Math.max function

> Math.max(1, 2, 3, undefined)
< NaN
> Math.max(1, 2, 3)
< 3

and the Array.prototype.reduce method

> [].reduce(function () {}, undefined)
< undefined
> [].reduce(function () {})
< Uncaught TypeError: Reduce of empty array with no initial value

Andrew-Cottrell avatar May 02 '20 13:05 Andrew-Cottrell

It seems like radix is now optional: https://github.com/google/closure-compiler/blob/71785a7e3e04be489510d4695870c7491af17c3f/externs/es6.js#L1942-L1953

I'm not sure when/how it changed though. Maybe this can be closed now?

sbc100 avatar Feb 28 '25 00:02 sbc100

No, it's still required. If it were optional, the declaration would look like this.

* @param {number=} radix

brad4d avatar Feb 28 '25 00:02 brad4d