Disallow missing commas between fields (#2287 #2520)

[alisa101rs]

Fixes #2287 and #2520, makes sure that comma between key-value pairs is required in Json.decodeFromString

[alisa101rs]

Hi @sandwwraith, could you please take a look at failed build? I believe it's not caused by this PR and I shouldn't/can't fix it in this PR

[sandwwraith]

Sure, I'll restart the build

[sandwwraith]

Hi! Thanks for the great PR! I love the idea overall, but I have several comments on implementation.

[sandwwraith]

Just a quick update from me: I've reviewed the PR and I'm mostly satisfied with the changes. However, I also ran the decoding benchmarks and unfortunately, scores for them dropped for about 6-8%:

develop
====

Benchmark                                                   Mode  Cnt     Score    Error  Units
TwitterFeedBenchmark.decodeMicroTwitter                    thrpt   14   962.120 ± 34.369  ops/s
TwitterFeedBenchmark.decodeMicroTwitterNoAltNames          thrpt   14  1005.670 ± 43.361  ops/s
TwitterFeedBenchmark.decodeMicroTwitterWithNamingStrategy  thrpt   14   915.170 ± 10.127  ops/s
TwitterFeedBenchmark.decodeTwitter                         thrpt   14  1073.797 ± 51.738  ops/s
TwitterFeedBenchmark.decodeTwitterNoAltNames               thrpt   14  1090.153 ±  3.413  ops/s
TwitterFeedBenchmark.decodeTwitterWithNamingStrategy       thrpt   14   928.378 ±  3.961  ops/s


this PR
====
Benchmark                                                   Mode  Cnt     Score    Error  Units
TwitterFeedBenchmark.decodeMicroTwitter                    thrpt   14   885.924 ± 27.029  ops/s
TwitterFeedBenchmark.decodeMicroTwitterNoAltNames          thrpt   14   968.884 ±  7.501  ops/s
TwitterFeedBenchmark.decodeMicroTwitterWithNamingStrategy  thrpt   14   865.704 ± 17.647  ops/s
TwitterFeedBenchmark.decodeTwitter                         thrpt   14  1013.502 ± 14.582  ops/s
TwitterFeedBenchmark.decodeTwitterNoAltNames               thrpt   14   983.525 ± 48.605  ops/s
TwitterFeedBenchmark.decodeTwitterWithNamingStrategy       thrpt   14   871.941 ±  9.046  ops/s

I will try to investigate whether it is possible to restructure some changes to optimize it further or if we have to accept this performance hit for the sake of correctness.

[alisa101rs]

Yeah I think performance hit comes from this change

https://github.com/Kotlin/kotlinx.serialization/pull/2889/files#diff-dc3d58b34387712e1b644a549a44bc8ad853ee2d1944f0da168b4c9e6ad28fc6L113-R115

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

Lines 113 to 117 in f9f160a

	else -> if (mode == newMode && json.configuration.explicitNulls) {
	this
	} else {
	StreamingJsonDecoder(json, newMode, lexer, descriptor, discriminatorHolder)
	}

https://github.com/alisa101rs/kotlinx.serialization/blob/719acbb385679a51743801bb114a6f5da0fd3aef/formats/json/commonMain/src/kotlinx/serialization/json/internal/StreamingJsonDecoder.kt#L115