From 43df676a0691ee4b9db312fc787046a9c6904c08 Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Tue, 16 Jan 2024 08:41:33 -0500 Subject: [PATCH 1/4] Default to packed repeated primitive fields in proto3. Fixes #704. In proto3, repeated primitive fields should be packed by default: https://protobuf.dev/programming-guides/encoding/#packed --- protobuf-codegen/src/gen/field/mod.rs | 31 ++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/protobuf-codegen/src/gen/field/mod.rs b/protobuf-codegen/src/gen/field/mod.rs index d6a5e44a4..79a16e05e 100644 --- a/protobuf-codegen/src/gen/field/mod.rs +++ b/protobuf-codegen/src/gen/field/mod.rs @@ -188,7 +188,36 @@ impl<'a> FieldGen<'a> { FieldKind::Repeated(RepeatedField { elem, - packed: field.field.proto().options.get_or_default().packed(), + packed: field + .field + .proto() + .options + .get_or_default() + .packed + .unwrap_or(match field.message.scope.file_scope.syntax() { + Syntax::Proto2 => false, + // in proto3, repeated primitive types are packed by default + Syntax::Proto3 => match field.field.proto().type_() { + Type::TYPE_DOUBLE + | Type::TYPE_FLOAT + | Type::TYPE_INT64 + | Type::TYPE_UINT64 + | Type::TYPE_INT32 + | Type::TYPE_FIXED64 + | Type::TYPE_FIXED32 + | Type::TYPE_BOOL + | Type::TYPE_UINT32 + | Type::TYPE_SFIXED32 + | Type::TYPE_SFIXED64 + | Type::TYPE_SINT32 + | Type::TYPE_SINT64 => true, + Type::TYPE_STRING + | Type::TYPE_GROUP + | Type::TYPE_MESSAGE + | Type::TYPE_BYTES + | Type::TYPE_ENUM => false, + }, + }), }) } RuntimeFieldType::Singular(..) => { From d60252982919933735375ddff9ca3cde037cf7d7 Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Wed, 17 Jan 2024 08:10:40 -0500 Subject: [PATCH 2/4] Provide a better error on a packed non-primitive. Fixes #706. --- protobuf-codegen/src/gen/field/mod.rs | 71 ++++++++++++++------------- 1 file changed, 37 insertions(+), 34 deletions(-) diff --git a/protobuf-codegen/src/gen/field/mod.rs b/protobuf-codegen/src/gen/field/mod.rs index 79a16e05e..54e17b843 100644 --- a/protobuf-codegen/src/gen/field/mod.rs +++ b/protobuf-codegen/src/gen/field/mod.rs @@ -185,40 +185,43 @@ impl<'a> FieldGen<'a> { } RuntimeFieldType::Repeated(..) => { let elem = field_elem(&field, root_scope, &customize); - - FieldKind::Repeated(RepeatedField { - elem, - packed: field - .field - .proto() - .options - .get_or_default() - .packed - .unwrap_or(match field.message.scope.file_scope.syntax() { - Syntax::Proto2 => false, - // in proto3, repeated primitive types are packed by default - Syntax::Proto3 => match field.field.proto().type_() { - Type::TYPE_DOUBLE - | Type::TYPE_FLOAT - | Type::TYPE_INT64 - | Type::TYPE_UINT64 - | Type::TYPE_INT32 - | Type::TYPE_FIXED64 - | Type::TYPE_FIXED32 - | Type::TYPE_BOOL - | Type::TYPE_UINT32 - | Type::TYPE_SFIXED32 - | Type::TYPE_SFIXED64 - | Type::TYPE_SINT32 - | Type::TYPE_SINT64 => true, - Type::TYPE_STRING - | Type::TYPE_GROUP - | Type::TYPE_MESSAGE - | Type::TYPE_BYTES - | Type::TYPE_ENUM => false, - }, - }), - }) + let primitive = match field.field.proto().type_() { + Type::TYPE_DOUBLE + | Type::TYPE_FLOAT + | Type::TYPE_INT64 + | Type::TYPE_UINT64 + | Type::TYPE_INT32 + | Type::TYPE_FIXED64 + | Type::TYPE_FIXED32 + | Type::TYPE_BOOL + | Type::TYPE_UINT32 + | Type::TYPE_SFIXED32 + | Type::TYPE_SFIXED64 + | Type::TYPE_SINT32 + | Type::TYPE_SINT64 => true, + Type::TYPE_STRING + | Type::TYPE_GROUP + | Type::TYPE_MESSAGE + | Type::TYPE_BYTES + | Type::TYPE_ENUM => false, + }; + let packed = field + .field + .proto() + .options + .get_or_default() + .packed + .unwrap_or(match field.message.scope.file_scope.syntax() { + Syntax::Proto2 => false, + // in proto3, repeated primitive types are packed by default + Syntax::Proto3 => primitive, + }); + if packed && !primitive { + anyhow::bail!( + "[packed = true] can only be specified for repeated primitive fields" + ); + } + FieldKind::Repeated(RepeatedField { elem, packed }) } RuntimeFieldType::Singular(..) => { let elem = field_elem(&field, root_scope, &customize); From ccfcb535b1e4df6ede6684eedff4ea13b75cd051 Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Thu, 18 Jan 2024 07:46:21 -0500 Subject: [PATCH 3/4] Treat enums as primitive. --- protobuf-codegen/src/gen/field/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/protobuf-codegen/src/gen/field/mod.rs b/protobuf-codegen/src/gen/field/mod.rs index 54e17b843..ac9617289 100644 --- a/protobuf-codegen/src/gen/field/mod.rs +++ b/protobuf-codegen/src/gen/field/mod.rs @@ -198,12 +198,12 @@ impl<'a> FieldGen<'a> { | Type::TYPE_SFIXED32 | Type::TYPE_SFIXED64 | Type::TYPE_SINT32 - | Type::TYPE_SINT64 => true, + | Type::TYPE_SINT64 + | Type::TYPE_ENUM => true, Type::TYPE_STRING | Type::TYPE_GROUP | Type::TYPE_MESSAGE - | Type::TYPE_BYTES - | Type::TYPE_ENUM => false, + | Type::TYPE_BYTES => false, }; let packed = field .field From 8a956a57d72acdf37ec92acf07e4af9c98a29bef Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Sun, 3 Mar 2024 16:40:49 -0500 Subject: [PATCH 4/4] Add test for packed default. --- .../src/common/v2/test_repeated_packed.rs | 15 +++++++++++++++ .../src/common/v2/test_repeated_packed_pb.proto | 4 ++++ 2 files changed, 19 insertions(+) diff --git a/test-crates/protobuf-codegen-protoc-test/src/common/v2/test_repeated_packed.rs b/test-crates/protobuf-codegen-protoc-test/src/common/v2/test_repeated_packed.rs index d8ecbb02a..8e6191aaa 100644 --- a/test-crates/protobuf-codegen-protoc-test/src/common/v2/test_repeated_packed.rs +++ b/test-crates/protobuf-codegen-protoc-test/src/common/v2/test_repeated_packed.rs @@ -1,3 +1,5 @@ +use protobuf::reflect::Syntax; +use protobuf::MessageFull; use protobuf_test_common::*; use super::test_repeated_packed_pb::*; @@ -83,3 +85,16 @@ fn test_issue_281() { test.values = (0..100).collect(); test_serialize_deserialize_no_hex(&test); } + +#[test] +fn test_write_packed_default() { + let mut test = TestPackedDefault::new(); + test.varints = vec![0, 1, 2, 3, 4, 5]; + + // Proto3 packs primitives by default, proto2 does not. + let expected_hex = match TestPackedDefault::descriptor().file_descriptor().syntax() { + Syntax::Proto2 => "08 00 08 01 08 02 08 03 08 04 08 05", + Syntax::Proto3 => "0a 06 00 01 02 03 04 05", + }; + test_serialize_deserialize(expected_hex, &test); +} diff --git a/test-crates/protobuf-codegen-protoc-test/src/common/v2/test_repeated_packed_pb.proto b/test-crates/protobuf-codegen-protoc-test/src/common/v2/test_repeated_packed_pb.proto index c4b543641..76743ea0d 100644 --- a/test-crates/protobuf-codegen-protoc-test/src/common/v2/test_repeated_packed_pb.proto +++ b/test-crates/protobuf-codegen-protoc-test/src/common/v2/test_repeated_packed_pb.proto @@ -13,3 +13,7 @@ message TestUnpacked { message TestIssue281 { repeated fixed32 values = 1 [packed=true]; } + +message TestPackedDefault { + repeated uint32 varints = 1; +}