CBOR unsigned number encoding is wrong

[JesusMcCloud]

Describe the bug

Direct byte encoding/decoding of Kotlin unsigned primitives is currently two’s-complement via signed primitive, which round-trips within the library, but doesn’t match the CBOR representation. CBOR has native signed and native unsigned Integer types and those should be used

Context:
While working on structured CBOR and pondering extensions/shorthands for numbers other than Long I noticed that unsigned primitives must not take the detour through signed types, because CBOR represents signed and unsigned types with a different header/mask. This is perfectly fine for the newly introducedCborInteger, because it stores a numbers signum and magnitude separately.

Problem:
When rebasing onto dev I noticed range checks got introduced to the CBOR decoder and it clicked:

Kotlin’s built-in serializers for UByte/UShort/UInt/ULong are inline wrappers over the signed primitives (Byte/Short/Int/Long) and literally call encodeByte(value.toByte()), decodeByte().toUByte(), etc. (core/commonMain/src/kotlinx/serialization/internal/ValueClasses.kt).

• CBOR’s “unsigned integer” is major type 0 (0..2⁶⁴-1). So for example UByte(200) “should” be encoded as CBOR unsigned integer 200.
• With the range checks (pulled from dev) in CborReader.decodeByte/decodeShort/decodeInt, decoding a CBOR unsigned integer 200 into a Byte now throws (since 200 is not within [-128..127]). That means UByte decoding via the standard serializer also fails for values > 127 even though an unsigned CBOR Int up to 255 would be within range.
• Encoding is also flawed: UByte(200).toByte() is -56, so the default encodeByte path emits a CBOR negative integer, not an unsigned 200.

To Reproduce
See Testcase that always fails.

Expected behavior
Unsigned Kotlin number types should always be encoded as Major Type 0

Environment

• Kotlin version: any
• Library version: any
• Kotlin platforms: all
• Gradle version: any
• Other relevant context: Was always there and the range checks made it even worse, which helped discover the issue

[JesusMcCloud]

I have decided against a separate PR, but instead included the fix in #3036 , because while I maintain this must be rectified, it is also a behavioural breaking change

[fzhinkin]

Nice catch, thanks for reporting!

I could argue about Kotlin unsigned numbers representation and CBOR major types (mis)alignment, but unsigned numbers decoding should not cause an exception, that's for sure.

[JesusMcCloud]

I thought it a bit of a pickle at first, given that unsigned numbers in Kotlin are just different interpretations of signed numbers, but the bits remain the same. However, this is not the case with CBOR where they do differ.

Still, if someone wants an unsigned number, I'd argue the intent is clear, and if the format allows for preserving that intent, then different semantics of the same bits should be encoded into actually different bits. you can then always toInt(), etc. in Kotlin and get back the signed interpretation.
I think other JetBrains in-house formats should be scanned for issues with range checks, regardless of whether signed types are supported:
Anything that has VarInt or unsigned integer types may run into sublte overflow cases, depending on the encoding. We have separate code paths for unsigned 64 bit integers and signed 64 bit integers in ASN.1 and manually unrolled the encoding/decoding loops to take care of it.

I don't know how relevant this is, but depending on the tool and model, it's probably good to do that by hand, because I caught this bug, trying to think myself back into the decoder while I had Codex double-check for bugs after the rebase. Codex did not notice this issue.