Limitation for Long type finally removed: support for integer types #171
Conversation
### What's done: - removed limitation for integers to be long - TOML spec: Arbitrary 64-bit signed integers (from −2^63 to 2^63−1) should be accepted and handled losslessly. If an integer cannot be represented losslessly, an error must be thrown - so right now we support Byte, Short and Int. But if there is an overflow, we throw an exception
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
ktlint found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.
### What's done: - removed limitation for integers to be long - TOML spec: Arbitrary 64-bit signed integers (from −2^63 to 2^63−1) should be accepted and handled losslessly. If an integer cannot be represented losslessly, an error must be thrown - so right now we support Byte, Short and Int. But if there is an overflow, we throw an exception
…7/ktoml into feature/integer-values
### What's done: - removed limitation for integers to be long - TOML spec: Arbitrary 64-bit signed integers (from −2^63 to 2^63−1) should be accepted and handled losslessly. If an integer cannot be represented losslessly, an error must be thrown - so right now we support Byte, Short and Int. But if there is an overflow, we throw an exception
|
@aSemy @NightEule5 @BOOMeranGG @nulls could you please have a look? |
### What's done: - removed limitation for integers to be long - TOML spec: Arbitrary 64-bit signed integers (from −2^63 to 2^63−1) should be accepted and handled losslessly. If an integer cannot be represented losslessly, an error must be thrown - so right now we support Byte, Short and Int. But if there is an overflow, we throw an exception
| // Unsigned values are not supported now, and I think | ||
| // that will not be supported, because TOML spec says the following: | ||
| // Arbitrary 64-bit signed integers (from −2^63 to 2^63−1) should be accepted and handled losslessly. |
There was a problem hiding this comment.
Would you mind elaborating on why unsigneds can't be supported?
I'd recommend making a new GitHub issue 'support unsigned ints' (I could make the issue if you like) and adding the link in the source code, then it's easier to track and follow progress.
There was a problem hiding this comment.
I don't mind, but the main problem is related to the kotlinx.serialization. I cannot find decodeUInt in their library...
I will write it to the comments
There was a problem hiding this comment.
Yeah, it's a little bit odd how KxS handles unsigneds.
KxS JSON handles them natively though https://github.com/Kotlin/kotlinx.serialization/blob/v1.4.1/docs/value-classes.md#unsigned-types-support-json-only
I suspect that they should 'just work' though, since they have serializers https://github.com/Kotlin/kotlinx.serialization/blob/v1.4.1/core/commonMain/src/kotlinx/serialization/internal/ValueClasses.kt
How KxS JSON handles them is
- unsigned numbers are value classes, so their
SerialDescriptors are 'inline' - it keeps a list of the 'SerialDescriptors' for all unsigned numbers
https://github.com/Kotlin/kotlinx.serialization/blob/dc950f5098162a3f8dd742d04b95f9a0405470e1/formats/json/commonMain/src/kotlinx/serialization/json/internal/StreamingJsonEncoder.kt#L15-L24 - there's a specific function
AbstractDecoder.decodeInline(...), that will handle the unsigned numbers - then there's a specific decoder for the unsigned types https://github.com/Kotlin/kotlinx.serialization/blob/d7e58c2eeb02e1da87ad36d41ef25523fafb6935/formats/json/commonMain/src/kotlinx/serialization/json/internal/StreamingJsonDecoder.kt#L367-L379
There was a problem hiding this comment.
We will need to add it then also... Yes, let's support it then :)
Looks like a workaround in KxS, looks like they forgot about unsigned in the very beginning
| ): T = if (content in limits.min..limits.max) { | ||
| conversion(content) | ||
| } else { |
There was a problem hiding this comment.
I think this code is based on my PR which used the MIN/MAX boundaries, but I checked the Kotlinx Serialization JSON decoder and I think it has a better way.
- decode to Long
- convert to
value.toInt() - if the result is different, then throw an exception
It's less code, and I think easier to understand because it's inline, the logic isn't split up or abstracted
There was a problem hiding this comment.
The only problem I see here is that comparing operation with limits can be easier than making a toInt() and than comparing... But anyway probably your variant is good enough.
There was a problem hiding this comment.
speaking about the idea - I actually asked a question on stack overflow about that and as well as I and you thought - everyone suggests to check boundaries: https://stackoverflow.com/questions/74510016/how-to-understand-if-there-was-an-integer-overflow-during-parsing-of-string-with :D
So probably, it is a good idea to have the most obvious solution
There was a problem hiding this comment.
Personally I prefer the KxS JSON way with .toInt(), but the result is the same pick whatever you prefer :)
There was a problem hiding this comment.
Also funny thing that @NightEule5 suggested same solution as you initially here: #153 (comment) :D
ktoml-core/src/commonMain/kotlin/com/akuleshov7/ktoml/decoders/TomlAbstractDecoder.kt
Outdated
Show resolved
Hide resolved
### What's done: - removed limitation for integers to be long - TOML spec: Arbitrary 64-bit signed integers (from −2^63 to 2^63−1) should be accepted and handled losslessly. If an integer cannot be represented losslessly, an error must be thrown - so right now we support Byte, Short and Int. But if there is an overflow, we throw an exception
What's done:
CastExceptionremoved and now unified withIllegalTypeException