Fix ProtoBuf packing for kotlin unsigned types#3079
Conversation
…s can be packed. inline popTag -> popTagOrDefault is needed, because packed serializer will use MISSING_TAG for entries, popTag fails on that.
|
Before moving further with this PR (for the part where inline value classes support is fixed) we need to address the questions from #3045 (comment) |
|
Okay, I'm not a Kotlin maintainer or anything, but here are my thoughts:
As long as value classes use their default serializer, I think, they should be inlined completely: 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.
The way I edited the
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. |
|
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 |
formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/internal/Helpers.kt
Outdated
Show resolved
Hide resolved
formats/protobuf/commonTest/src/kotlinx/serialization/protobuf/PackedArraySerializerTest.kt
Show resolved
Hide resolved
sandwwraith
left a comment
There was a problem hiding this comment.
Big thanks for your fix! It slipped out of my attention last month, but I will include it in the final 1.10.0
ProtoBuf's
ProtoPackedannotation only worked for the original signed primitives and ByteArray, and did not work for unsigned variants.