tafia / quick-protobuf

A rust implementation of protobuf parser
MIT License
452 stars 87 forks source link

Parse extensions correctly. Allow options more. #242

Closed sinistersnare closed 1 year ago

sinistersnare commented 1 year ago

the syntax extend MESSAGENAME { ... } is now legal, and parses to an Extend struct. The struct is not used in the compiler.

Also, options are now parsed and ignored in enums and messages.

such as:

message A {
  option an_option = "hello, world!";
  option (an_option_extension) = false;
}

This PR only involves the Parser. It does not compile the extend functionality, only allows it to compile.

I kind of went into this codebase blind to add this, so if I am doing something non-kosher, let me know! I tried not to step on any toes, but I couldn't help myself a couple times. If you want me to revert the couple of style changes, I will happily.

Also, can there be a new release? It seems there has not been one in > 1 year?

sinistersnare commented 1 year ago

Would love some guidance on what is causing the test failures? They seem unrelated to my changes?

snproj commented 1 year ago

What's erroring out

Hi! Just took a quick peek at why it's failing, and from what I can tell it's just because the test cases for enum aliases weren't implemented before, and the test cases are hardcoded to assert that they aren't implemented (more precisely, it asserts that pb-rs should not be able to parse quick-protobuf > tests > rust_protobuf > v2/3 > test_enum_alias_pb.proto).

Codewise, in quick-protobuf > tests > rust_protobuf:

must_fail["v3/test_enum_alias_pb.proto"]="enum alias not implemented"
must_fail["v2/test_enum_alias_pb.proto"]="enum alias not implemented"

What to do

Since pb-rs can parse this file without parsing errors now, we should remove these. However, the actual enum alias functionality still isn't implemented, so we can either:

  1. Find a different way to assert that the enum alias functionality isn't implemented, or
  2. Just remove this assertion test, and instead document somewhere that enum aliases aren't supported

Option 1 makes me think of the trybuild crate, which can be used to show that something doesn't compile. This would be more useful in a case where we want to show that an existing API should not compile; however, our case is more that there does not exist any such API. Therefore, option 2 is the more straightforward option in my opinion.

snproj commented 1 year ago

Oh there are new releases for pb-rs (0.10.0) and quick-protobuf (0.8.1), if that's what you meant

sinistersnare commented 1 year ago

I think Option 2 is the best choice. Currently, the only documentation for missing features I see is #12. Should we add some docs at the crate level about unsupported features?

snproj commented 1 year ago

Hmm, I think that would make sense -- and then perhaps close #12 with a note that users should now reference the docs instead (to keep a single source of truth). This could be a different PR, I think.

For now, I reckon we could just remove the lines in generate.sh completely (not just comment them out), and also add a comment to v2 + 3 > test_enum_alias_pb.proto explaining that the test used to throw a parse error but no longer does?

sinistersnare commented 1 year ago

I made the requested change (hopefully to your agreement). Making official docs and dropping #12 seems like a great idea!