Crystal: JSON::PullParser should output Int32 instead of Int64

[HCLarsen]

While writing a custom converter for JSON::Mapping, I kept getting type errors because the PullParser outputs integers as Int64 instead of Int32. Since the rest of the standard library (ie Object#to_i) defaults to Int32, wouldn't it be more logical for PullParser to do the same?

[straight-shoota]

The parser is supposed to parse a JSON number as a Crystal integer. The integer type should be big enough to hold JSON numbers. While the JSON spec does not specifcy a size limit, a practical limit is 53-bit integer which is the soft limit for JavaScript integers (above that, integer precision cannot be guaranteed).
Int32 wouldn't be big enough for that and thus it shouldn't be used by default.

[asterite]

Also, you just need to do read_int.to_i32. We could add read_i32. Or we could redefine read_int to return Int32 and also have read_int64. But JSON::PullParser is quite low level and not very flexible, so I personally don't mind having to write read_int.to_i32, and take into consideration that JSON::Type is a union that has an Int64 as a type.

[HCLarsen]

@asterite It's true that I can just do read_int.to_i (since to_i outputs an Int32). The question is, is that the best way to do it. As I said above, the majority of the core library defaults integer conversions to 32-bit integers, so this is a big variation from the norm of how Crystal works.

@straight-shoota I do see logic in that. However, how often do we actually come across numbers that big when parsing JSON data. I rather like the idea of having read_int default to 32-bits and having a separate read_int64 when you expect to get larger numbers.

However, I will say that if we don't change the default size of read_int, we must add some documentation that indicates the output type of read_int. When I had this issue today, I had to go all the way into the source code to figure out why I was getting a 64 bit integer.

[RX14]

read_int should probably theoretically return a BigInt for unlimited size. Not very practical though. It might even be worth implementing an i128 read_int.

How about just read_int(Int32), then you can get whatever type you want. The default type can be Int32.

[stakach]

Since most of us use 64 bit platforms wouldn't it make more sense to have crystal lang default to Int64?
Is there any reason that 32bit values are the default?

[HCLarsen]

Not being part of the core team, I can only speculate, but it seems wasteful to use 64 bit integers as the default. How often do you work with numbers with greater magnitude than 2 billion?

[ysbaddaden]

Int64 numbers can happen, and if it happens and we default to an 32-bit, you have a bug which can have disastrous effects —it happened to Facebook API many years ago, because user ids suddenly crossed the maximum int32, which silently overflowed in many clients... Outch.

The maximum integers in a JSON are larger than an Int32 can hold, thus we must ALWAYS use an Int64. However how wasteful it may feel for the usual case.

[stakach]

Also operations that output to a 32-bit subregister, in the CPU, are automatically zero-extended to the entire 64-bit register.
So outside of memory usage, which shouldn't be much of an issue if you compare a crystal app to a java app for instance, I can't see any real negatives with defaulting to 64bits?

[HCLarsen]

The negative, as I already stated, is that when you try to assign the output of this method to an Int32, the program crashes. Thus, my earlier statement that if we must output an Int64 with this method, then we need some documentation to indicate what the output from this method is.

[RX14]

no, the program doesn't crash, you get a compiler error. Which is far nicer.

The documentation for the return type can be annotated easily.

[ysbaddaden]

Closing, since we won't change the return type.