Support for Unknown Fields (Rebase of #574)

[blizzardfinnegan]

For more information see Issue #2

This PR is largely similar to #574 , rebased to main, and with edits suggested by the maintainer on said PR implemented to the best of my ability.

[blizzardfinnegan]

@caspermeijn
If there are any objections or additional comments on this, I would love to know. This is an important feature, and one the company I work with is dependent on.

Unfortunately, Github doesn't have a clear way to request reviews on a PR for someone outside the org and/or not a direct developer on the project, otherwise I would have done so. Apologies for the ping if that isn't the right channel.

[alebar42]

@blizzardfinnegan AFAIK you could remove or comment out the ignored tests here in order to verify the implementation: https://github.com/blizzardfinnegan/prost/blob/feature/unknown_fields_support/conformance/failing_tests.txt

[blizzardfinnegan]

@alebar42 Thank you for the correction and clarification!
I made the relevant changes, and pushed them to my branch. It doesn't seem to have passed through to the PR yet? Might be a CI issue... 🤷

Either way, the tests do in fact pass (locally, at least).

Edit: Unclear what was the issue, but said changes are now in the PR, which is the important part.

[blizzardfinnegan]

@caspermeijn
Any chance there's some movement on this?

[aidandj]

Ran into an issue using pbjson_build with this. Because when you generate unknown fields, pbjson_build doesn't know what to do with them and it errors:

error[E0063]: missing field `unknown_fields` in initializer of `google::protobuf::generated_code_info::Annotation`
    --> /out/pbjson/google.protobuf.serde.rs:3929:20
     |
3929 |                 Ok(super::tonic::google::protobuf::generated_code_info::Annotation {

[caspermeijn]

Thank you for taking this forward. It is a large PR, so it takes some time for me to respond.

I miss user facing documentation. Could you add a section to the README that describes this new functionality?

[blizzardfinnegan]

Thank you for the review! I pretty much did as clean of a direct uplift as I could with the original MR, without changing too much. I'll go through and add docs and resolve the issues you pointed out.

Apologies for the repeated pings. It's definitely a bear of a PR, I wasn't sure if you had seen it or not.

[aidandj]

Random thought. Protobuf naming guide recommends not using underscores as the initial character. Although it's not enforced, suggesting or using a default struct field name with a underscore prefix would be much more unlikely to have naming conflicts.

https://protobuf.dev/programming-guides/style/

[blizzardfinnegan]

Protobuf naming guide recommends not using underscores as the initial character.

This makes sense; Protobuf is language-agnostic, and some languages use underscores to denote private fields (python(?)), so useful to encourage people to not use it. Thank you for the information! I've updated accordingly.

[caspermeijn]

I like the progress. I would like to see some small changes.

[blizzardfinnegan]

@caspermeijn Just checking in on the status of this MR; I know the past however long has been holidays, so projects and whatnot have probably taken a bit of a break, wanted to make sure it's still on the radar.

[caspermeijn]

@caspermeijn Just checking in on the status of this MR; I know the past however long has been holidays, so projects and whatnot have probably taken a bit of a break, wanted to make sure it's still on the radar.

Yes, I see your progress. Thank you for that. It is difficult for me to find the time to review this large PR, as it needs a long block of time to fully understand the context. Please hang in there!

I did move one of your fixes of the bench to a seperate small PR ;-) #1404

[blizzardfinnegan]

Entirely understandable! Big, fundamental PRs like this take a ton of energy.

Thank you for catching that unrelated-to-this-PR change, I can make changes to pull that out of this specific PR in the next week or so; I think clippy may have auto-applied it and I didn't catch it in a git add.

Apologies for the repeated pings; this PR is the biggest hold-up for a decently-sized RIIR effort at work, and other devs at work are eager to play with it instead of using the google rust wrapper crate thingy. Please don't take that as a request for urgency though, I want to make sure it's done right (even if it is a first pass that needs more refinement and/or additional features added later).

[blizzardfinnegan]

CI failures seem to be largely due to Github internal issues; will force a rerun tomorrow, hoping they (Github/Microslop) get their issues resolved.

[blizzardfinnegan]

@caspermeijn
Been a while, just checking back in on the progress for this MR.