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

re Number.parseInt cleanup #972

Open
lars-t-hansen opened this Issue Aug 9, 2017 · 17 comments

Comments

Projects
None yet
5 participants
@lars-t-hansen
Contributor

lars-t-hansen commented Aug 9, 2017

I see from the meeting notes that @BrendanEich is proposing to clean up parseInt wrt to NaN, undefined, and null, since parsing them with sufficiently high radices will have surprising results.

Does this problem also not pertain to true and false values? I ask because nothing was recorded about those values in the minutes.

In the SpiderMonkey shell,

js> Number.parseInt(true)
NaN

js> Number.parseInt(true, 36)
1389110
@bterlson

This comment has been minimized.

Show comment
Hide comment
@bterlson

bterlson Aug 9, 2017

Member

I think so.

If we're cleaning things up, I also can't come up with a scenario where the behavior of Number.parseInt(1000, radixOtherThan10) makes sense.

Member

bterlson commented Aug 9, 2017

I think so.

If we're cleaning things up, I also can't come up with a scenario where the behavior of Number.parseInt(1000, radixOtherThan10) makes sense.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Aug 9, 2017

Member

"1000" might be in base 2, for example - I think being able to change the radix is important.

Member

ljharb commented Aug 9, 2017

"1000" might be in base 2, for example - I think being able to change the radix is important.

@bterlson

This comment has been minimized.

Show comment
Hide comment
@bterlson

bterlson Aug 9, 2017

Member

@ljharb 1000 (not "1000") is always in base 10. I think it's strange to want to convert a base 10 number into a string and parse it as a different radix (as opposed to, say, converting to a different radix via Number#toString(radix)). When would you do the former?

Member

bterlson commented Aug 9, 2017

@ljharb 1000 (not "1000") is always in base 10. I think it's strange to want to convert a base 10 number into a string and parse it as a different radix (as opposed to, say, converting to a different radix via Number#toString(radix)). When would you do the former?

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Aug 9, 2017

Member

ahhhh, i see what you mean - you're talking about passing a non-string in with a non-decimal radix.

I agree that's unlikely, but parseInt takes only strings, and ToStrings everything - it would be weird for it to accept both number and string input and behave differently.

Member

ljharb commented Aug 9, 2017

ahhhh, i see what you mean - you're talking about passing a non-string in with a non-decimal radix.

I agree that's unlikely, but parseInt takes only strings, and ToStrings everything - it would be weird for it to accept both number and string input and behave differently.

@bakkot

This comment has been minimized.

Show comment
Hide comment
@bakkot

bakkot Aug 9, 2017

Contributor

Any chance we could make it throw (or at least unconditionally return NaN) when given any primitive which is not a string?

Contributor

bakkot commented Aug 9, 2017

Any chance we could make it throw (or at least unconditionally return NaN) when given any primitive which is not a string?

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Aug 9, 2017

Member

I'd be very surprised if that was web compatible - and I think it'd definitely need to throw, not return NaN.

The convention though is that parse methods coerce to strings, and Math methods coerce to numbers; I'm not sure what the value of changing that here is.

Member

ljharb commented Aug 9, 2017

I'd be very surprised if that was web compatible - and I think it'd definitely need to throw, not return NaN.

The convention though is that parse methods coerce to strings, and Math methods coerce to numbers; I'm not sure what the value of changing that here is.

@bakkot

This comment has been minimized.

Show comment
Hide comment
@bakkot

bakkot Aug 9, 2017

Contributor

@ljharb, I thought changing that was what this whole proposal was about?

Anyway, I doubt Number.parseInt is seeing much use, especially in these edge cases.

Contributor

bakkot commented Aug 9, 2017

@ljharb, I thought changing that was what this whole proposal was about?

Anyway, I doubt Number.parseInt is seeing much use, especially in these edge cases.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Aug 9, 2017

Member

@bakkot changing what happens for specific primitives with specific radix ranges is what the proposal was about; parseInt(1000, x) works fine for any radix already.

Number.parseInt === parseInt, by spec, so it's always been a safe 1:1 conversion - and while I think there won't likely be any use of these edge cases, I think there absolutely will be a ton of usages of it that pass in non-string values.

Member

ljharb commented Aug 9, 2017

@bakkot changing what happens for specific primitives with specific radix ranges is what the proposal was about; parseInt(1000, x) works fine for any radix already.

Number.parseInt === parseInt, by spec, so it's always been a safe 1:1 conversion - and while I think there won't likely be any use of these edge cases, I think there absolutely will be a ton of usages of it that pass in non-string values.

@bakkot

This comment has been minimized.

Show comment
Hide comment
@bakkot

bakkot Aug 9, 2017

Contributor

Hm. If I understood right, we were discussing changing only Number.parseInt, thereby breaking that function identity and allowing us to make more significant changes.

I would be surprised to see a lot of uses of Number.parseInt precisely because of that identity.

Contributor

bakkot commented Aug 9, 2017

Hm. If I understood right, we were discussing changing only Number.parseInt, thereby breaking that function identity and allowing us to make more significant changes.

I would be surprised to see a lot of uses of Number.parseInt precisely because of that identity.

@bakkot

This comment has been minimized.

Show comment
Hide comment
@bakkot

bakkot Aug 9, 2017

Contributor

Anyway, revised proposal: Any chance we could make Number.parseInt throw (or at least unconditionally return NaN) when given both any primitive which is not a string and a radix?

Contributor

bakkot commented Aug 9, 2017

Anyway, revised proposal: Any chance we could make Number.parseInt throw (or at least unconditionally return NaN) when given both any primitive which is not a string and a radix?

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Aug 9, 2017

Member

Number.parseInt(numberOrString) is all over the airbnb codebase.

Member

ljharb commented Aug 9, 2017

Number.parseInt(numberOrString) is all over the airbnb codebase.

@bakkot

This comment has been minimized.

Show comment
Hide comment
@bakkot

bakkot Aug 9, 2017

Contributor

whyyyyyyyy

That is to say, why are you passing it a number? When would that ever come up?

Anyway, are you ever doing Number.parseInt(numberOrString, radix)?

Contributor

bakkot commented Aug 9, 2017

whyyyyyyyy

That is to say, why are you passing it a number? When would that ever come up?

Anyway, are you ever doing Number.parseInt(numberOrString, radix)?

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Aug 9, 2017

Member

Personally, I'm not, but I'd think it's a common thing.

As to "why", because sometimes the values come from developers typing in number literals (and passing them through a call stack that eventually gets run through parseInt), and sometimes the values come from the API or the DOM, where they're always strings.

Brendan's proposal is about the specific edge cases of "passing non-string primitives into parseInt with a radix and getting surprising non-NaN results" - I don't think it's a good idea to scope-creep it into "let's tighten up the type signature of parseInt" :-p

Member

ljharb commented Aug 9, 2017

Personally, I'm not, but I'd think it's a common thing.

As to "why", because sometimes the values come from developers typing in number literals (and passing them through a call stack that eventually gets run through parseInt), and sometimes the values come from the API or the DOM, where they're always strings.

Brendan's proposal is about the specific edge cases of "passing non-string primitives into parseInt with a radix and getting surprising non-NaN results" - I don't think it's a good idea to scope-creep it into "let's tighten up the type signature of parseInt" :-p

@bterlson

This comment has been minimized.

Show comment
Hide comment
@bterlson

bterlson Aug 10, 2017

Member

I would be very surprised if making non-string values throw or return NaN would be web compatible. However, it may be possible to do if radix != 10. In those cases, parseInt of a number value is most likely a bug? Not that I'm super eager to tighten this down more.

Member

bterlson commented Aug 10, 2017

I would be very surprised if making non-string values throw or return NaN would be web compatible. However, it may be possible to do if radix != 10. In those cases, parseInt of a number value is most likely a bug? Not that I'm super eager to tighten this down more.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Aug 10, 2017

Member

Relying on parseInt(1000, 2) === parseInt('1000', 2) doesn't seem like a bug to me.

Member

ljharb commented Aug 10, 2017

Relying on parseInt(1000, 2) === parseInt('1000', 2) doesn't seem like a bug to me.

@claudepache

This comment has been minimized.

Show comment
Hide comment
@claudepache

claudepache Aug 10, 2017

Contributor
parseInt(0b1111101000, 2) // 8

o_O

Contributor

claudepache commented Aug 10, 2017

parseInt(0b1111101000, 2) // 8

o_O

@bterlson

This comment has been minimized.

Show comment
Hide comment
@bterlson

bterlson Aug 10, 2017

Member

@ljharb That identity is only true for numbers that happen to be "binary" in base 10. Any other number value is going to behave very strangely. This is what I mean as likely a bug - the only case where you can know this is ok is if you're dealing with a literal, and in that case why are you using parseInt?

Member

bterlson commented Aug 10, 2017

@ljharb That identity is only true for numbers that happen to be "binary" in base 10. Any other number value is going to behave very strangely. This is what I mean as likely a bug - the only case where you can know this is ok is if you're dealing with a literal, and in that case why are you using parseInt?

@ljharb ljharb referenced this issue Aug 14, 2017

Closed

Infinity #79

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment