stepancheg / rust-protobuf

Rust implementation of Google protocol buffers
MIT License
2.81k stars 382 forks source link

feat: add support for repeated fields in constants #740

Closed plusvic closed 1 month ago

plusvic commented 2 months ago

This adds support for repeated fields in constants. Until now the following was failing with a parsing error:

optional uint64 my_field = 1 [ (my_option) = { my_option_repeated_field: ["foo", "bar"] } ];

This is valid for protoc and it's covered in the specification, but the parser wasn't accepting the ["foo", "bar"] syntax for assigning a value to the repeated field.

Some refactoring was required to implement this. Particularly, the functions option_value_field_to_unknown_value and option_value_message_to_unknown_value, which returned UnknownValue, now receive a mutable UnknownFields and add new fields to it. These function where renamed to more appropriate names.

Additionally, this fixes an issue while parsing constant messages that use commas or semicolons as field separators. For instance, all the following variants that are accepted by the official compilerprotoc were being rejected by the parser

{foo: 1,bar: 2,baz: 3,}
{foo: 1;bar: 2;baz: 3;}
{foo: 1 bar: 2 baz: 3}
{foo: 1,bar: 2;baz: 3}
{foo: 1,bar: 2 baz: 3}
plusvic commented 1 month ago

@stepancheg Any thoughts on this? I would like to merge this upstream. I've been using my modified branch for a while without any issues in my project (https://github.com/VirusTotal/yara-x), but I can't publish new versions of my crate that uses this feature until a release of rust-protobuf includes these changes.

stepancheg commented 1 month ago

Hi @plusvic,

I merged this change. Looks good, but I didn't read it thoroughly.

Some context: rust-protobuf project will be shut down eventually, because maintaining this project is too much for me, and Google is working on official version of protobuf support for Rust. As far as I understand, what they have seems to work, but it is not as feature-rich as this project yet.

plusvic commented 1 month ago

Oh, I didn't know that Google was working on an official implementation. I've been investigating and it looks that this official implementation is not pure-Rust, it's a wrapper around the C++ implementation or μpb (https://github.com/protocolbuffers/upb), that also also probably means that it relies on protoc for generating the Rust code.

So, I still see great value in rust-protobuf, as it offers some niceties that the official implementation won't have. I'll probably keep using it even if I had to maintain my own fork. Are you open to transfer the project to new maintainers? I think I could volunteer on that.

stepancheg commented 1 month ago

I've been investigating and it looks that this official implementation is not pure-Rust, it's a wrapper around the C++ implementation or μpb

AFAIU it is a wrapper around C implementation. So it should be easy to compile. And that implementations seems to do better at memory management: they seem to borrow at parsing, which is better than what rust-protobuf is doing from performance perspective.

it relies on protoc for generating the Rust code.

Of that I'm certain. Although that should not be an issue. I also have this crate https://docs.rs/protoc-bin-vendored/ which provides prebuilt binaries as crates. (If I came up with this idea earlier, I would probably not have started pure rust codegen.)

it offers some niceties that the official implementation won't have

I hope they will develop necessary features eventually. And some things likely can be built on top of that.

Are you open to transfer the project to new maintainers?

It's complicated. I actually already have transferred "protobuf" crate ownership to them, and they publish new versions of rust-protobuf these days (long story). So the best I can offer is ask for your help maintaining 3.x branch in this repository or post a link from readme if you decide to publish a fork.

plusvic commented 1 month ago

Thank you very much for the context. For the time being I'll wait and see how the official implementation looks like once it is released.