Add support to decode numeric literals containing an exponent #2227

xBaank · 2023-03-12T13:26:38Z

Closes #2078

sandwwraith · 2023-04-20T12:52:50Z

Hi, thanks for your PR, and sorry it has taken so long to get to it. I see you have some other commits beside yours, did you start your branch off master? Please rebase it on dev.

Regarding contents: this is really nice addition IMO, as json.org spec indeed permits exponent numbers. However, to fully support them, you also need to consider JsonElement parsing — e.g. conversion from JsonElement to a serializable class. (decodeFromJsonElement<SomeData>(parseToJsonElement(string))) As far as I see, it uses simple String.toLong() conversion and will also fail on numbers with exponent. To test behavior in all available modes, you may use parameterizedTest function from JsonTestBase — see its usages. Unfortunately we can't accept your PR without supporting both these modes, as it will make Json parsing inconsistent.

sandwwraith

.

xBaank · 2023-04-20T13:18:40Z

did you start your branch off master? Please rebase it on dev.

I actually started it on dev but synced my branch from github and pulled the master changes accidentally

However, to fully support them, you also need to consider JsonElement parsing — e.g. conversion from JsonElement to a serializable class. (decodeFromJsonElement(parseToJsonElement(string))) As far as I see, it uses simple String.toLong() conversion and will also fail on numbers with exponent.

I will take a look on decoding from JsonElement to Type

xBaank · 2023-04-20T15:47:42Z

I see you have some other commits beside yours, did you start your branch off master? Please rebase it on dev.

you mean creating another pull request to the dev branch with the master changes? or just rebase master into my branch with newest changes?

sandwwraith · 2023-04-20T16:49:06Z

Your branch (with your-only commits) should start from the latest dev. You can force-push it to use the same pull request

sandwwraith · 2023-04-20T17:25:13Z

You can drop unnecessary commits during interactive rebase

xBaank · 2023-04-20T18:11:00Z

You can drop unnecessary commits during interactive rebase

ty, I didn't know

formats/json-tests/commonTest/src/kotlinx/serialization/json/JsonExponentTest.kt

xBaank · 2023-04-24T08:25:18Z

formats/json/commonMain/src/kotlinx/serialization/json/internal/StringOps.kt

+private val INT_NUMBERS_CHARS = arrayOf('1', '2', '3', '4', '5', '6', '7', '8', '9', '0', 'e', 'E', '+', '-')
+internal fun String.toLongJson(): Long {
+    return toLongOrNull() ?: toDouble()
+        .takeIf { all { it in INT_NUMBERS_CHARS } }


Should cases like this one be supported when decoding into a long? and if so, should it be truncated, rounded...?

Same here

formats/json/commonMain/src/kotlinx/serialization/json/JsonElementSerializers.kt

sandwwraith · 2023-05-01T13:19:22Z

formats/json/commonMain/src/kotlinx/serialization/json/internal/StringOps.kt

+}
+internal fun String.toLongJsonOrNull(): Long? {
+    return toLongOrNull() ?: toDoubleOrNull()
+        ?.takeIf { all { it in INT_NUMBERS_CHARS } }


I think such code poses hidden problem: all iterates all symbols and it in iterates all chars in the INT_NUMBERS_CHARS array, so it results in a quadratic complexity. Moreover, it is just hard to read. I see several possibilities here:

To simplify condition, I think it is possible just to check '.' !in it. All other non-digit and non-E signs should be prohibited by toDoubleOrNull(), I guess?

A harder, but more correct change would be extract parsing logic from consumeNumericLiteral or to create StringJsonLexer in place so we wouldn't have two different code paths doing the same thing.

I think it is possible to also extract these takeIfs to a separate function since it doesn't matter if you use toDouble or toDoubleOrNull given that exception is anyway thrown at the end if needed.

I think StringJsonLexer is the best solution

sandwwraith · 2023-05-01T13:19:50Z

formats/json/commonMain/src/kotlinx/serialization/json/internal/lexer/AbstractJsonLexer.kt

+            true -> 10.0.pow(exponentAccumulator.toDouble())
+        }
+
+        if(hasExponent) {


[nit] formatting: space before opening brace (check other places too)

sandwwraith · 2023-05-09T15:06:41Z

formats/json/commonMain/src/kotlinx/serialization/json/JsonElement.kt

+private inline fun <T> mapExceptionsToNull(f : () -> T) : T? {
+    return try { f()}
+    catch (e : JsonDecodingException) { null }
+    catch (e : IndexOutOfBoundsException) { null }


I doubt IIOBE can happen here, because there is special EOF handling in the consumeNumericLiteral. (and generally, lexer is not allowed to throw it, because it must throw only JsonDecodingException). Do you have an example when it happens?

Yes, this test was failing on TREE mode, hasChar on consumeNumericLiteral doesn't update when the char is - | + | e | E

kotlinx.serialization/formats/json-tests/commonTest/src/kotlinx/serialization/json/JsonParserFailureModesTest.kt

Lines 134 to 144 in cfce5d8

@Test

fun testInvalidNumber() = parametrizedTest {

assertFailsWithSerial("JsonDecodingException") { default.decodeFromString<Holder>("""{"id":-}""", it) }

assertFailsWithSerial("JsonDecodingException") { default.decodeFromString<Holder>("""{"id":+}""", it) }

assertFailsWithSerial("JsonDecodingException") { default.decodeFromString<Holder>("""{"id":--}""", it) }

assertFailsWithSerial("JsonDecodingException") { default.decodeFromString<Holder>("""{"id":1-1}""", it) }

assertFailsWithSerial("JsonDecodingException") { default.decodeFromString<Holder>("""{"id":0-1}""", it) }

assertFailsWithSerial("JsonDecodingException") { default.decodeFromString<Holder>("""{"id":0-}""", it) }

assertFailsWithSerial("JsonDecodingException") { default.decodeFromString<Holder>("""{"id":a}""", it) }

assertFailsWithSerial("JsonDecodingException") { default.decodeFromString<Holder>("""{"id":-a}""", it) }

}

sandwwraith · 2023-05-09T15:07:13Z

formats/json/commonMain/src/kotlinx/serialization/json/JsonElement.kt

@@ -315,6 +321,18 @@ public val JsonPrimitive.contentOrNull: String? get() = if (this is JsonNull) nu
 private fun JsonElement.error(element: String): Nothing =
    throw IllegalArgumentException("Element ${this::class} is not a $element")

+private inline fun <T> mapExceptionsToNull(f : () -> T) : T? {
+    return try { f()}


formatting: space before brace.

I recommend using IntelliJ IDEA auto-formatter, as it honors Kotlin's style guide

formats/json/commonMain/src/kotlinx/serialization/json/JsonElementSerializers.kt

sandwwraith · 2023-05-09T15:12:39Z

formats/json/commonMain/src/kotlinx/serialization/json/internal/lexer/AbstractJsonLexer.kt

+
+        if (hasExponent) {
+            val doubleAccumulator  = accumulator.toDouble() * calculateExponent(exponentAccumulator, isExponentPositive)
+            if(doubleAccumulator > Long.MAX_VALUE || doubleAccumulator < Long.MIN_VALUE) fail("Numeric value overflow")


fornatting: space after if

sandwwraith · 2023-05-09T15:24:01Z

formats/json-tests/commonTest/src/kotlinx/serialization/json/JsonExponentTest.kt

+
+    @Test
+    fun testExponentDecodingTruncatedDecimal() = parametrizedTest {
+        val decoded = Json.decodeFromString<SomeData>("""{ "count": -1E-1 }""", it)


I've just realized that it is incorrect to truncate such values. That's why the problem with 4.9E-324 happens in the first place - it is too small. We should not allow such values to be parsed to Long, or any value with non-zero fractional part after applying exponent. I think the easiest way to check it would be with doubleAccumulator.floor() == doubleAccumulator, as in here:

kotlinx.serialization/formats/json/jsMain/src/kotlinx/serialization/json/internal/DynamicDecoders.kt

Line 154 in e2092a4

val canBeConverted = number.isFinite() && floor(number) == number

…mentSerializers.kt Co-authored-by: Leonid Startsev <[email protected]>

…mbers

sandwwraith · 2023-05-15T16:51:53Z

formats/json/commonMain/src/kotlinx/serialization/json/JsonElement.kt

-public val JsonPrimitive.intOrNull: Int? get() = content.toIntOrNull()
+public val JsonPrimitive.intOrNull: Int?
+    get() =
+        mapExceptionsToNull { StringJsonLexer(content).consumeNumericLiteral().toInt().toLong().toInt() }


Why there's .toInt().toLong().toInt()?

I don't know why I did that

sandwwraith

Great job, thank you!

Fixes #2078

…#2227) Fixes Kotlin#2078

sandwwraith requested review from qwwdfsad and sandwwraith April 19, 2023 16:56

sandwwraith requested changes Apr 20, 2023

View reviewed changes

sandwwraith added the waiting for clarification label Apr 20, 2023

sandwwraith removed the waiting for clarification label Apr 20, 2023

xBaank added 2 commits April 20, 2023 19:30

Support for exponential numbers

cd9eae6

Fix for js legacy tests

f14085a

xBaank force-pushed the exponentNumbers branch from abcced6 to ce0cf49 Compare April 20, 2023 17:31

xBaank added 7 commits April 20, 2023 20:07

More tests

b4179e8

Cleanup

9b83269

Remove allocation + tests

0a182fd

Cleanup

4d8435c

Remove unused import

a3a9475

use *

637a1cf

Cleanup

e43d624

xBaank force-pushed the exponentNumbers branch from ce0cf49 to e43d624 Compare April 20, 2023 18:09

xBaank added 3 commits April 20, 2023 23:38

still fails + need some refactor

bac754b

Make it simpler

e2ca3c7

Cleanup

a1ed53b

xBaank commented Apr 24, 2023

View reviewed changes

formats/json-tests/commonTest/src/kotlinx/serialization/json/JsonExponentTest.kt Show resolved Hide resolved

xBaank commented Apr 24, 2023

View reviewed changes

formats/json/commonMain/src/kotlinx/serialization/json/JsonElementSerializers.kt Show resolved Hide resolved

xBaank requested a review from sandwwraith April 26, 2023 08:33

sandwwraith reviewed May 1, 2023

View reviewed changes

Use StringJsonLexer

2d649d2

sandwwraith requested changes May 9, 2023

View reviewed changes

xBaank and others added 5 commits May 9, 2023 22:56

Fix IOBE

2996732

Update formats/json/commonMain/src/kotlinx/serialization/json/JsonEle…

6251545

…mentSerializers.kt Co-authored-by: Leonid Startsev <[email protected]>

Update formats/json/commonMain/src/kotlinx/serialization/json/JsonEle…

45391a0

…mentSerializers.kt Co-authored-by: Leonid Startsev <[email protected]>

fail when number has fractional part

f823393

Merge remote-tracking branch 'origin/exponentNumbers' into exponentNu…

3bb3932

…mbers

sandwwraith reviewed May 15, 2023

View reviewed changes

Remove toInt().toLong().toInt()

4167039

sandwwraith approved these changes May 17, 2023

View reviewed changes

sandwwraith merged commit f76d073 into Kotlin:dev May 17, 2023

LeonidSandal pushed a commit that referenced this pull request Jun 29, 2023

Add support to decode numeric literals containing an exponent (#2227)

dd836f3

Fixes #2078

LeonidSandal pushed a commit that referenced this pull request Jun 29, 2023

Add support to decode numeric literals containing an exponent (#2227)

25f3fa7

Fixes #2078

pdvrieze pushed a commit to pdvrieze/kotlinx.serialization that referenced this pull request Jul 4, 2023

Add support to decode numeric literals containing an exponent (Kotlin…

31ea356

…#2227) Fixes Kotlin#2078

JesusMcCloud pushed a commit to a-sit-plus/kotlinx.serialization that referenced this pull request Jul 5, 2023

Add support to decode numeric literals containing an exponent (Kotlin…

124aa5d

…#2227) Fixes Kotlin#2078

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support to decode numeric literals containing an exponent #2227

Add support to decode numeric literals containing an exponent #2227

xBaank commented Mar 12, 2023

sandwwraith commented Apr 20, 2023 •

edited

Loading

sandwwraith left a comment

xBaank commented Apr 20, 2023

xBaank commented Apr 20, 2023

sandwwraith commented Apr 20, 2023

sandwwraith commented Apr 20, 2023

xBaank commented Apr 20, 2023 •

edited

Loading

xBaank Apr 24, 2023 •

edited

Loading

sandwwraith May 1, 2023

xBaank May 3, 2023

sandwwraith May 1, 2023 •

edited

Loading

sandwwraith May 9, 2023 •

edited

Loading

xBaank May 9, 2023

sandwwraith May 9, 2023

sandwwraith May 9, 2023

sandwwraith May 9, 2023

sandwwraith May 15, 2023

xBaank May 16, 2023

sandwwraith left a comment

	@Test
	fun testInvalidNumber() = parametrizedTest {
	assertFailsWithSerial("JsonDecodingException") { default.decodeFromString<Holder>("""{"id":-}""", it) }
	assertFailsWithSerial("JsonDecodingException") { default.decodeFromString<Holder>("""{"id":+}""", it) }
	assertFailsWithSerial("JsonDecodingException") { default.decodeFromString<Holder>("""{"id":--}""", it) }
	assertFailsWithSerial("JsonDecodingException") { default.decodeFromString<Holder>("""{"id":1-1}""", it) }
	assertFailsWithSerial("JsonDecodingException") { default.decodeFromString<Holder>("""{"id":0-1}""", it) }
	assertFailsWithSerial("JsonDecodingException") { default.decodeFromString<Holder>("""{"id":0-}""", it) }
	assertFailsWithSerial("JsonDecodingException") { default.decodeFromString<Holder>("""{"id":a}""", it) }
	assertFailsWithSerial("JsonDecodingException") { default.decodeFromString<Holder>("""{"id":-a}""", it) }
	}

Add support to decode numeric literals containing an exponent #2227

Add support to decode numeric literals containing an exponent #2227

Conversation

xBaank commented Mar 12, 2023

sandwwraith commented Apr 20, 2023 • edited Loading

sandwwraith left a comment

Choose a reason for hiding this comment

xBaank commented Apr 20, 2023

xBaank commented Apr 20, 2023

sandwwraith commented Apr 20, 2023

sandwwraith commented Apr 20, 2023

xBaank commented Apr 20, 2023 • edited Loading

xBaank Apr 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandwwraith May 1, 2023 • edited Loading

Choose a reason for hiding this comment

sandwwraith May 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandwwraith left a comment

Choose a reason for hiding this comment

sandwwraith commented Apr 20, 2023 •

edited

Loading

xBaank commented Apr 20, 2023 •

edited

Loading

xBaank Apr 24, 2023 •

edited

Loading

sandwwraith May 1, 2023 •

edited

Loading

sandwwraith May 9, 2023 •

edited

Loading