Unknown fields

[wildarch]

As proposed in #256.

This adds an unknown_fields flag to prost-build that adds unknown field support to specific proto messages. In the structs for these message it adds an extra field:

struct MyMessage {
  // ...
  protobuf_unknown_fields: Vec<UnknownField>,
}

This works both with deserialization and serialization: Any fields not recognized while parsing are put in the unknown fields, and any fields added here will be included when serializing.

I think the main use case for this feature would be to support custom options by enabling this flag for things like FieldOptions.

There is still some work to be done on this PR in terms of tests and documentation, but I'm hoping to get @danburkert's blessing before finishing that 😄.

[wildarch]

CC @tiziano88

[tiziano88]

Could you add a test in which the protobuf message has an actual field called protobuf_unknown_fields, of whatever type? I think we should clarify what behaviour we expect in that case, which I guess will boil down to one of the following:

• fail early with a friendly error message
• somehow try to escape the field name so that it does not conflict with this; I expect there must be a list of identifiers to avoid (e.g. Rust keywords) that are already escaped by prost -- or does it just ignore them?
• give this field a name that cannot possibly clash with any generated field, e.g. _protobuf_unknown_fields or similar.

[wildarch]

I did a quick test, and it seems prost removes leading underscores in protobuf field names, so I think we could name the field _unknown_fields?

[wildarch]

As a sidenote, if you define a message like this:

message Test {
  optional string _weird_field = 1;
  optional string weird_field = 2;
}

Prost fails with:

4 |     pub weird_field: ::std::option::Option<std::string::String>,
  |     ----------------------------------------------------------- `weird_field` first declared here
5 |     #[prost(string, optional, tag="2")]
6 |     pub weird_field: ::std::option::Option<std::string::String>,
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ field already declared

[Frando]

I was testing this today as a step towards custom options (#256). I noted that there's a bug currently for varint fields. They end up as empty because the len that is put into the unknown field value is always 0. The current code parses the varint, which as I read the code always advances the read buffer. I couldn't see a way to read the varint without advancing the read buffer (which is what we'd want here), so instead I went for reencoding the varint in-place if the return value should be captured for the unknown_fields.

I fixed it here. I don't know though how to "PR against this PR" - @wildarch, can you merge it manually into this PR, or what's the recommended way?

[wildarch]

Thanks for fixing that @Frando! I can pull your changes in 😄. But before I do that and spend more time on this PR I would like some feedback from the maintainers of the crate to see if they are interested in merging this eventually.

[Frando]

@danburkert how do you feel about this PR, are there things that should be changed for this to be accepted? I'm using this in hrpc-build to specify service and method IDs through options. I'd like to publish it on crates.io sooner or later, thus would like to help getting this PR merged (plus adding unkown fields to to the descriptors in this commit).

[Frando]

Hei @danburkert, do you think this can move forward, or do you recommend another approach?

[teburd]

I'm really interested in seeing this move forward, as I'd like to customize codegen based on protobuf options

[yusdacra]

This would be really useful, what's needed to move this forwards?

[Frando]

@danburkert do you have a comment on the future for this PR? Is there a way to get it merged, do you want to see further changes? I'm using it in hrpc and would like to release on crates.io soonish, so I'm thinking if I will publish a fork of prost on crates.io or if this can be included in mainline (which I'd prefer of course).

[hollinwilkins]

What would need to happen to get this merged? Does someone need to go in and resolve conflicts? This would be a very welcome feature update :)