tokio-rs / prost

PROST! a Protocol Buffers implementation for the Rust Language
Apache License 2.0
3.78k stars 489 forks source link

`proto3` doesn't pack repeated fields by default #976

Open daniel-sherwood opened 7 months ago

daniel-sherwood commented 7 months ago

The code here https://github.com/tokio-rs/prost/blob/63c00248abef84337c1010b6a10f3258b2db8b54/prost-build/src/code_generator.rs#L435 attempts to set the default behaviour for proto3 to be packed, however because options.packed() returns false unless [packed = true] is set on the field, this fails.

daniel-sherwood commented 7 months ago

Just realised this is only the case where the field has other attributes and hence options is not None

caspermeijn commented 6 months ago

I don't understand what your request is. Can you describe your observed behavior and your expected behavior? An example file or project will help with reproducing your problem.

bhollis commented 4 months ago

We're hitting the same bug. Consider a field like this:

  repeated uint64 user_ids = 2 [
    (buf.validate.field).repeated.min_items = 1,
    (buf.validate.field).repeated.max_items = 25
  ];

This generates the following from prost:

#[prost(uint64, repeated, packed="false", tag="2")]
pub user_ids: ::prost::alloc::vec::Vec<u64>,

However if I remove the options, like this:

  repeated uint64 user_ids = 2;

Then the generated code looks like this:

#[prost(uint64, repeated, tag="2")]
pub user_ids: ::prost::alloc::vec::Vec<u64>,

As @daniel-sherwood correctly identified, this is a bug at https://github.com/tokio-rs/prost/blob/63c00248abef84337c1010b6a10f3258b2db8b54/prost-build/src/code_generator.rs#L431-L438

In my first example, there are options on the field so map_or does not use its default and the code falls back to options.packed(). options.packed() appears to be either true or false, with a default of false. But it should default to true if the file is in proto3 syntax.

caspermeijn commented 3 months ago

Thanks, now I understand your problem better. Are you willing to open a PR to fix this?

bhollis commented 3 months ago

Maybe! It'd be my first time trying Rust, so it'd probably take a while.