Skip to content
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

Removing a Double restriction: float support #175

Merged
merged 2 commits into from
Jan 1, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ package com.akuleshov7.ktoml.decoders

import com.akuleshov7.ktoml.exceptions.IllegalTypeException
import com.akuleshov7.ktoml.tree.nodes.TomlKeyValue
import com.akuleshov7.ktoml.tree.nodes.pairs.values.TomlDouble
import com.akuleshov7.ktoml.tree.nodes.pairs.values.TomlLong
import com.akuleshov7.ktoml.utils.FloatingPointLimitsEnum
import com.akuleshov7.ktoml.utils.FloatingPointLimitsEnum.*
import com.akuleshov7.ktoml.utils.IntegerLimitsEnum
import com.akuleshov7.ktoml.utils.IntegerLimitsEnum.*
import kotlinx.datetime.Instant
Expand All @@ -26,7 +29,7 @@ public abstract class TomlAbstractDecoder : AbstractDecoder() {
override fun decodeByte(): Byte = decodePrimitiveType()
override fun decodeShort(): Short = decodePrimitiveType()
override fun decodeInt(): Int = decodePrimitiveType()
override fun decodeFloat(): Float = invalidType("Float", "Double")
override fun decodeFloat(): Float = decodePrimitiveType()
override fun decodeChar(): Char = invalidType("Char", "String")

// Valid Toml types that should be properly decoded
Expand All @@ -36,18 +39,19 @@ public abstract class TomlAbstractDecoder : AbstractDecoder() {
override fun decodeString(): String = decodePrimitiveType()

protected fun DeserializationStrategy<*>.isDateTime(): Boolean =
descriptor == instantSerializer.descriptor ||
descriptor == localDateTimeSerializer.descriptor ||
descriptor == localDateSerializer.descriptor
descriptor == instantSerializer.descriptor ||
descriptor == localDateTimeSerializer.descriptor ||
descriptor == localDateSerializer.descriptor

// Cases for date-time types
@Suppress("UNCHECKED_CAST")
override fun <T> decodeSerializableValue(deserializer: DeserializationStrategy<T>): T = when (deserializer.descriptor) {
instantSerializer.descriptor -> decodePrimitiveType<Instant>() as T
localDateTimeSerializer.descriptor -> decodePrimitiveType<LocalDateTime>() as T
localDateSerializer.descriptor -> decodePrimitiveType<LocalDate>() as T
else -> super.decodeSerializableValue(deserializer)
}
override fun <T> decodeSerializableValue(deserializer: DeserializationStrategy<T>): T =
when (deserializer.descriptor) {
instantSerializer.descriptor -> decodePrimitiveType<Instant>() as T
localDateTimeSerializer.descriptor -> decodePrimitiveType<LocalDateTime>() as T
localDateSerializer.descriptor -> decodePrimitiveType<LocalDate>() as T
else -> super.decodeSerializableValue(deserializer)
}

internal abstract fun decodeKeyValue(): TomlKeyValue

Expand All @@ -64,6 +68,7 @@ public abstract class TomlAbstractDecoder : AbstractDecoder() {
try {
return when (val value = keyValue.value) {
is TomlLong -> decodeInteger(value.content as Long, keyValue.lineNo)
is TomlDouble -> decodeFloatingPoint(value.content as Double, keyValue.lineNo)
else -> keyValue.value.content as T
}
} catch (e: ClassCastException) {
Expand All @@ -75,36 +80,70 @@ public abstract class TomlAbstractDecoder : AbstractDecoder() {
}
}

private inline fun <reified T> decodeFloatingPoint(content: Double, lineNo: Int): T = when (T::class) {
Float::class -> validateAndConvertFloatingPoint(content, lineNo, FLOAT) { num: Double -> num.toFloat() as T }
Double::class -> validateAndConvertFloatingPoint(content, lineNo, DOUBLE) { num: Double -> num as T }
else -> invalidType(T::class.toString(), "Signed Type")
}

/**
* ktoml parser treats all integer literals as Long and all floating-point literals as Double,
* so here we should be checking that there is no overflow with smaller types like Byte, Short and Int.
*/
private inline fun <reified T> validateAndConvertFloatingPoint(
content: Double,
lineNo: Int,
limits: FloatingPointLimitsEnum,
conversion: (Double) -> T,
): T = if (content in limits.min..limits.max) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's about suggestion to convert to lower type and then back and check that value is the same?

Copy link
Owner Author

@orchestr7 orchestr7 Jan 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nulls , such suggestions valid, however looks a little bit confusing for integer types. But for floating-point types it can cause an error.

Floating-point arithmetics in Kotlin compiler is partially implemented using C++ interop. And in C++ standard it is explicitly told that floating point numbers cannot store all values exactly (not due to limits, but due to precision), so there can potentially be cases when doubleNumber.toFloat().toDouble() != doubleNumber due to loss of bits even when the number is inside Float bounds (-MAX_FLOAT..MAX_FLOAT).

So it is a big question for me if I really should rely on this hack and have potential (but with low probability) case when we corrupt the number and throw an exception (and with a higher complexity algorithm) instead of a simple solution to check of bounds.

As you know, I am not a risky guy, so I think that I would better use low risk option here (at least with floating point numbers). I am sure you would have done the same in this situation... 😄

But If you or @aSemy have very strong objections - I can rework it in the future...

conversion(content)
} else {
throw IllegalTypeException(
"The floating point literal, that you have provided is <$content>, " +
"but the type for deserialization is <${T::class}>. You will get an overflow, " +
"so we advise you to check the data or use other type for deserialization (Long, for example)",
lineNo
)
}

/**
* After a lot of discussions (https://github.com/akuleshov7/ktoml/pull/153#discussion_r1003114861 and
* https://github.com/akuleshov7/ktoml/issues/163), we have finally decided to allow to use Integer types and not only Long.
* This method does simple validation of integer values to avoid overflow. For example, you really want to use byte,
* we will check here, that your byte value does not exceed 127 and so on.
*/
private inline fun <reified T> decodeInteger(content: Long, lineNo: Int): T = when (T::class) {
Byte::class -> validateAndConvertInt(content, lineNo, BYTE) { num: Long -> num.toByte() as T }
Short::class -> validateAndConvertInt(content, lineNo, SHORT) { num: Long -> num.toShort() as T }
Int::class -> validateAndConvertInt(content, lineNo, INT) { num: Long -> num.toInt() as T }
Long::class -> validateAndConvertInt(content, lineNo, LONG) { num: Long -> num as T }
Double::class, Float::class -> throw IllegalTypeException(
"Expected floating-point number, but received integer literal: <$content>. " +
"Deserialized floating-point number should have a dot: <$content.0>",
lineNo
)
else -> invalidType(T::class.toString(), "Signed Type")
}
private inline fun <reified T> decodeInteger(content: Long, lineNo: Int): T =
when (T::class) {
Byte::class -> validateAndConvertInteger(content, lineNo, BYTE) { num: Long -> num.toByte() as T }
Short::class -> validateAndConvertInteger(content, lineNo, SHORT) { num: Long -> num.toShort() as T }
Int::class -> validateAndConvertInteger(content, lineNo, INT) { num: Long -> num.toInt() as T }
Long::class -> validateAndConvertInteger(content, lineNo, LONG) { num: Long -> num as T }
Double::class, Float::class -> throw IllegalTypeException(
"Expected floating-point number, but received integer literal: <$content>. " +
"Deserialized floating-point number should have a dot: <$content.0>",
lineNo
)
else -> invalidType(T::class.toString(), "Signed Type")
}

private inline fun <reified T> validateAndConvertInt(
/**
* ktoml parser treats all integer literals as Long and all floating-point literals as Double,
* so here we should be checking that there is no overflow with smaller types like Byte, Short and Int.
*/
private inline fun <reified T> validateAndConvertInteger(
content: Long,
lineNo: Int,
limits: IntegerLimitsEnum,
conversion: (Long) -> T,
): T = if (content in limits.min..limits.max) {
conversion(content)
} else {
throw IllegalTypeException("The integer literal, that you have provided is <$content>, " +
"but the type for deserialization is <${T::class}>. You will get an overflow, " +
"so we advise you to check the data or use other type for deserialization (Long, for example)", lineNo)
throw IllegalTypeException(
"The integer literal, that you have provided is <$content>, " +
"but the type for deserialization is <${T::class}>. You will get an overflow, " +
"so we advise you to check the data or use other type for deserialization (Long, for example)",
lineNo
)
}

private fun invalidType(typeName: String, requiredType: String): Nothing {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public enum class IntegerLimitsEnum(public val min: Long, public val max: Long)
// 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.
// If an integer cannot be represented losslessly, an error must be thrown. <- FixMe: this is still not supported
// If an integer cannot be represented losslessly, an error must be thrown.
// U_BYTE(UByte.MIN_VALUE.toLong(), UByte.MAX_VALUE.toLong()),
// U_SHORT(UShort.MIN_VALUE.toLong(), UShort.MAX_VALUE.toLong()),
// U_INT(UInt.MIN_VALUE.toLong(), UInt.MAX_VALUE.toLong()),
Expand All @@ -28,8 +28,8 @@ public enum class IntegerLimitsEnum(public val min: Long, public val max: Long)
* @property min
* @property max
*/
public enum class FloatLimitsEnums(public val min: Double, public val max: Double) {
DOUBLE(Double.MIN_VALUE, Double.MAX_VALUE),
FLOAT(Float.MIN_VALUE.toDouble(), Float.MAX_VALUE.toDouble()),
public enum class FloatingPointLimitsEnum(public val min: Double, public val max: Double) {
DOUBLE(-Double.MAX_VALUE, Double.MAX_VALUE),
FLOAT(-Float.MAX_VALUE.toDouble(), Float.MAX_VALUE.toDouble()),
;
}
Original file line number Diff line number Diff line change
Expand Up @@ -192,17 +192,15 @@ class PrimitivesDecoderTest {
@Serializable
data class Data(val value: Float)

assertFailsWith<IllegalTypeException> {
val data = Toml.decodeFromString<Data>(toml)
assertEquals(expected, data.value)
}
val data = Toml.decodeFromString<Data>(toml)
assertEquals(expected, data.value)
}

test(0f, "0")
test(1f, "1")
test(-1f, "-1")
test(-128f, "-128")
test(127f, "127")
test(0f, "0.0")
test(1f, "1.0")
test(-1f, "-1.0")
test(-128f, "-128.0")
test(127f, "127.0")
}

@Test
Expand All @@ -218,6 +216,7 @@ class PrimitivesDecoderTest {
}
}

testFails((-Double.MAX_VALUE).toString())
testFails("-129")
testFails("128")
}
Expand Down