Parse extensions correctly. Allow options more. #242

sinistersnare · 2022-12-28T18:10:44Z

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 · 2022-12-28T20:07:03Z

pb-rs/src/parser.rs

        },
    )(input)
 }

 fn option_ignore(input: &str) -> IResult<&str, ()> {
-    value((), delimited(tag("option"), take_until(";"), tag(";")))(input)
+    value((), delimited(pair(tag("option"), many1(br)), take_until(";"), tag(";")))(input)


I changed this rule to ensure that this parser only matches option ... and not optional ...

Nice catch!

sinistersnare · 2022-12-28T20:08:04Z

pb-rs/src/parser.rs

@@ -823,8 +985,10 @@ mod test {
        }
    }

-    #[test]
-    fn test_missing_tokens() {
+    mod missing_tokens {


I changed this from a function to an inner-module... This seems like a much better idea than having functions. This way, there is the organization, without the foot-gun of not calling functions!

Sounds good to me! We can also apply this to test_which_names_can_be_qualified().

sinistersnare · 2022-12-28T20:09:19Z

pb-rs/src/types.rs

+#[derive(Debug, Clone, Default)]
+pub struct Extend {
+    /// The message being extended.
+    pub name: String,
+    /// All fields that are being added to the extended message.
+    pub fields: Vec<Field>,
+}
+
+impl Extend {}


I created this extend struct, which hopefully will be incorporated into the compiler eventually (possibly by me), but we may want to switch to #235 first.

sinistersnare · 2022-12-28T20:10:49Z

pb-rs/src/types.rs

+#[derive(Debug, Clone, Default)]
+pub struct Extensions {
+    pub from: i32,
+    /// Max number is 536,870,911 (2^29 - 1), as defined in the
+    /// protobuf docs
+    pub to: i32,
+}
+
+impl Extensions {
+    /// The max field number that can be used as an extension.
+    pub fn max() -> i32 {
+        536870911
+    }
+
+}


To be clear this is related, but orthogonal functionality to

extend X { ... }

This is the functionality for

message A { ... extensions 500 to max } message B { ... extensions 39 to 339 }

sinistersnare · 2022-12-28T20:15:32Z

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

snproj · 2023-01-06T08:39:48Z

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:

Find a different way to assert that the enum alias functionality isn't implemented, or
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 · 2023-01-06T09:15:55Z

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

sinistersnare · 2023-01-06T12:57:02Z

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 · 2023-01-06T18:11:22Z

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?

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; }

sinistersnare · 2023-01-07T02:08:32Z

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

sinistersnare force-pushed the feature/message-extensions branch from 9981195 to b5b9f11 Compare December 28, 2022 20:06

sinistersnare commented Dec 28, 2022

View reviewed changes

sinistersnare force-pushed the feature/message-extensions branch from b5b9f11 to 94caca7 Compare January 6, 2023 12:58

sinistersnare force-pushed the feature/message-extensions branch from 94caca7 to a92c9f3 Compare January 7, 2023 02:07

snproj merged commit 21cd0d2 into tafia:master Jan 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse extensions correctly. Allow options more. #242

Parse extensions correctly. Allow options more. #242

sinistersnare commented Dec 28, 2022 •

edited

Loading

sinistersnare Dec 28, 2022

snproj Jan 6, 2023

sinistersnare Dec 28, 2022

snproj Jan 6, 2023

sinistersnare Dec 28, 2022

sinistersnare Dec 28, 2022

sinistersnare commented Dec 28, 2022

snproj commented Jan 6, 2023 •

edited

Loading

snproj commented Jan 6, 2023

sinistersnare commented Jan 6, 2023

snproj commented Jan 6, 2023

sinistersnare commented Jan 7, 2023

Parse extensions correctly. Allow options more. #242

Parse extensions correctly. Allow options more. #242

Conversation

sinistersnare commented Dec 28, 2022 • edited Loading

sinistersnare Dec 28, 2022

Choose a reason for hiding this comment

snproj Jan 6, 2023

Choose a reason for hiding this comment

sinistersnare Dec 28, 2022

Choose a reason for hiding this comment

snproj Jan 6, 2023

Choose a reason for hiding this comment

sinistersnare Dec 28, 2022

Choose a reason for hiding this comment

sinistersnare Dec 28, 2022

Choose a reason for hiding this comment

sinistersnare commented Dec 28, 2022

snproj commented Jan 6, 2023 • edited Loading

What's erroring out

What to do

snproj commented Jan 6, 2023

sinistersnare commented Jan 6, 2023

snproj commented Jan 6, 2023

sinistersnare commented Jan 7, 2023

sinistersnare commented Dec 28, 2022 •

edited

Loading

snproj commented Jan 6, 2023 •

edited

Loading