Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add support for repeated fields in constants #740

Merged
merged 4 commits into from
Oct 15, 2024

Conversation

plusvic
Copy link
Contributor

@plusvic plusvic commented Sep 5, 2024

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}

According to the Protobuf Specification the fields in a constant message can be separated by spaces, commas or semicolons. All the following variants are accepted by the official compiler`protoc`.

```
{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}
```
This adds support for repeated fields in constants. Until know the following was failing with a parsing error:

```protobuf
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.
plusvic added a commit to VirusTotal/yara-x that referenced this pull request Sep 5, 2024
The fixes were sent for review to the original repository:

stepancheg/rust-protobuf#740

Until these fixes are merged and published in a new release, we'll depend on my own version.
@plusvic
Copy link
Contributor Author

plusvic commented Oct 15, 2024

@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 stepancheg merged commit a1306cc into stepancheg:master Oct 15, 2024
10 checks passed
@stepancheg
Copy link
Owner

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
Copy link
Contributor Author

plusvic commented Oct 15, 2024

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
Copy link
Owner

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
Copy link
Contributor Author

plusvic commented Oct 15, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants