kotlinx.serialization icon indicating copy to clipboard operation
kotlinx.serialization copied to clipboard

Fix ProtoBuf packing for kotlin unsigned types

Open KosmX opened this issue 3 months ago • 4 comments

ProtoBuf's ProtoPacked annotation only worked for the original signed primitives and ByteArray, and did not work for unsigned variants.

KosmX avatar Sep 12 '25 23:09 KosmX

@KosmX, thanks for opening the PR!

Related issue: https://github.com/Kotlin/kotlinx.serialization/issues/3045

fzhinkin avatar Sep 15 '25 21:09 fzhinkin

Before moving further with this PR (for the part where inline value classes support is fixed) we need to address the questions from https://github.com/Kotlin/kotlinx.serialization/issues/3045#issuecomment-3211256099

fzhinkin avatar Sep 15 '25 21:09 fzhinkin

Okay, I'm not a Kotlin maintainer or anything, but here are my thoughts: I'll start with the last, that's probably the easiest or most important

Kotlin might start supporting multi-field value classes at some point, it's worth considering them when addressing single-field value classes issue

As long as value classes use their default serializer, I think, they should be inlined completely:
For example I have a value class Complex(val real: Double, val imaginary: Double), I want it to be extremely compact in a huge array. And that's the same for serialization, especially for binary formats like protobuf.

And, if for some reason, I do not want to serialize the value class this way, I can still go the long way, and write a custom serializer, or maybe we can add an annotation for serializable value classes (like DontInlineSerialize)

If we want to match standards/specifications exactly, don't bend protobuf's rules, maybe do the inlining the opposite way: inline only for unsigned primitives, and for explicit SerializeInline annotation.

Currently we try to inline things in all formats.

potentially, there might be the same issues for inline values classes that are fields of regular classes, we have to figure out something there too

The way I edited the isPackable getter, it is recursive on nested value classes. I should write a test-case for that.

we need to figure out how to deal with Proto<Something>-annotated fields of inline value classes

If the value class is inlined, I would look for these annotations recursively, from the deepest.

On the implementation question, I have some ideas, but nothing precise-enough, and I think it would be better to have the complete specification first, maybe involve more people, ask what their thoughts are.

KosmX avatar Sep 15 '25 22:09 KosmX

Hi @KosmX! I think we can merge this without solving all the issues mentioned. Do you want to continue working with this PR? If so, I'll submit my portion of comments

sandwwraith avatar Nov 24 '25 11:11 sandwwraith