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

Json.Decoder.int Bug / Ints in Elm #337

Closed
agrafix opened this Issue Aug 6, 2015 · 5 comments

Comments

Projects
None yet
3 participants
@agrafix

agrafix commented Aug 6, 2015

The javascript spec allows numbers up to 2^53-1 (9007199254740991) and Elm generally is fine with these large integers, too. The json decoder fails for inputs larger than 2147483647.

I've found the error in the Native.Json part:
https://github.com/elm-lang/core/blob/2a8dcb522f71541c299a8a465bb39bc17bb9ec67/src/Native/Json.js#L53

It checks that (value|0) === value, but this check will fail for any input larger than 2147483647 because binary operators only work on 32bit ints.

I did not send a pull request (yet?) because I'm not sure what the correct step is. The Bitwise module allows parameters of type Int, and this will produce broken results iff the number is larger. Should elm distinguish between Int (32 Bit) and LargeInt (53 Bit)? Or should we just fix the json parser?

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Aug 6, 2015

Member

PR #153 was open with a potential fix, but we closed it so @ajhager or @krisajenkins could do a PR with the faster version of the fix.

Member

evancz commented Aug 6, 2015

PR #153 was open with a potential fix, but we closed it so @ajhager or @krisajenkins could do a PR with the faster version of the fix.

@ajhager

This comment has been minimized.

Show comment
Hide comment
@ajhager

ajhager Aug 6, 2015

Contributor

And the new PR is on its way. :)

Contributor

ajhager commented Aug 6, 2015

And the new PR is on its way. :)

@agrafix

This comment has been minimized.

Show comment
Hide comment
@agrafix

agrafix Aug 7, 2015

Very cool, looking forward to it! :-) @ajhager

agrafix commented Aug 7, 2015

Very cool, looking forward to it! :-) @ajhager

@agrafix

This comment has been minimized.

Show comment
Hide comment
@agrafix

agrafix Aug 7, 2015

@evancz what do you think about the behavior of the bitwise module? maybe there should at lease be a warning that is only works for 32 Bits Ints?

agrafix commented Aug 7, 2015

@evancz what do you think about the behavior of the bitwise module? maybe there should at lease be a warning that is only works for 32 Bits Ints?

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Sep 22, 2016

Member

Not sure what happened here, so I am going to close. Someone please reopen with fresh information if something else needs to happen.

Member

evancz commented Sep 22, 2016

Not sure what happened here, so I am going to close. Someone please reopen with fresh information if something else needs to happen.

@evancz evancz closed this Sep 22, 2016

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