`OpenEnum`: an enum to represent protobuf's enumeration field values #1079

mzabaluev · 2024-05-28T11:35:31Z

Another solution for #276, amenable to destructuring.

This changes the representation of enum fields in message structs to this generic wrapper, parameterized over the generated known enum type:

pub enum OpenEnum<T> {
    /// A known value defined in the proto
    Known(T),
    /// Unknown value as decoded from the wire
    Unknown(i32),
}

In an improvement over #1061, this allows convenient matching of enum field values as part of the message. There are also convenience methods to fallibly extract the known value in an Option or Result.

The wrapper to represent field of enumerated types with the possibility of unknown values.

Replace i32 with the type-checked wrapper.

caspermeijn · 2024-05-28T14:10:35Z

Please add a description that explains your proposed change. That will make reviewing easier.

Forgot to re-enable tempdir cleanup disabled for debugging.

QuentinPerez · 2024-05-28T23:20:48Z

Do you think that this logic could be applied to the oneof field ?

mzabaluev · 2024-05-29T12:51:38Z

Do you think that this logic could be applied to the oneof field ?

I believe it works differently for oneofs: if an unknown field number is encountered in a message and it's not a known oneof variant or a regular field, the field is ignored. So the oneof field that would get the value if the variant field were described in the proto would get None instead.

caspermeijn · 2024-05-31T08:47:52Z

Will this break compatibility with proto2/closed enums? Personally, I don't use proto2, so I am not sure whether it is properly supported at all.

caspermeijn · 2024-05-31T08:48:10Z

I think it makes sense to provide a migration guide.

caspermeijn · 2024-05-31T08:48:26Z

What is the error message for a i32 field with #[prost(enumeration)]? Can we detect that scenario and print a nice error message?

caspermeijn · 2024-05-31T08:48:40Z

I would like to see some tests for OpenEnum.

caspermeijn · 2024-05-31T08:01:51Z

prost-derive/src/field/map.rs

+ self.#ident.get(#take_ref key).cloned().and_then(|x| { x.known() })
 }
 #[doc=#insert_doc]
 pub fn #insert(&mut self, key: #key_ty, value: #ty) -> ::core::option::Option<#ty> {
- self.#ident.insert(key, value as i32).and_then(|x| {
- let result: ::core::result::Result<#ty, _> = ::core::convert::TryFrom::try_from(x);
- result.ok()
- })
+ self.#ident.insert(key, value.into()).and_then(|x| { x.known() })


Is it beneficial to provide a get and insert function with the new wrapper? I feel like the map can be used directly with the helper functions provided by OpenEnum.

As can be seen in the generated code itself, these methods provide considerable convenience for the most common use cases.

Perhaps the insert method could be changed to return the possible old value as Option<OpenEnum<#ty>> so the caller can choose the fallback behavior for unknown values, but that API would be inconsistent with the get method. If it's to be changed so, the only added convenience there would be the .into() conversion call.

caspermeijn · 2024-05-31T08:06:45Z

prost-derive/src/field/scalar.rs

 pub fn #get(&self) -> #ty {
- ::core::convert::TryFrom::try_from(self.#ident).unwrap_or(#default)
+ self.#ident.unwrap_or(#default)
 }

 #[doc=#set_doc]
 pub fn #set(&mut self, value: #ty) {
- self.#ident = value as i32;
+ self.#ident = value.into();
 }


Is it still beneficial to provide a get, set and push functions with the new wrapper? I feel like the field can be used directly with the helper functions provided by OpenEnum.

There is some convenience in the setter method:

msg.set_kind(Kind::Foo)

is nicer and more type-ahead friendly than

msg.kind = Kind::Foo.into()

The same applies to the push method for repeated fields.

The getter hides a bit much opinionated behavior, in my opinion, so it might be better to leave it to OpenEnum helper methods to explicitly decide on how unknown values should be treated.

Another thing to consider here is porting the existing code. If these methods get used instead of direct access to fields, which I assume happens a lot now because the fields are just i32 or Option<i32>, updating to the version that generates OpenEnum would not require changes.

I think the better API is to have some sort of set operation on OpenEnum. Maybe something like:

msg.kind.set(Kind::Foo)

Well, this change will break existing code in many ways, so I think we should directly go for the best API

caspermeijn · 2024-05-31T08:08:23Z

prost-derive/src/field/scalar.rs

@@ -555,13 +550,13 @@ impl Ty {
 Ty::Bool => quote!(bool),
 Ty::String => quote!(&str),
 Ty::Bytes(..) => quote!(&[u8]),
- Ty::Enumeration(..) => quote!(i32),
+ Ty::Enumeration(..) => unreachable!("an enum should never be queried for its ref type"),


Can you explain me why it should never be queried by ref?

caspermeijn · 2024-05-31T08:10:32Z

prost-derive/src/field/scalar.rs

 }
 }

 pub fn module(&self) -> Ident {
 match *self {
- Ty::Enumeration(..) => Ident::new("int32", Span::call_site()),
+ Ty::Enumeration(..) => Ident::new("enumeration", Span::call_site()),


Does it make sense to use self.as_str() for enumerations as well?

caspermeijn · 2024-05-31T08:31:34Z

prost/src/encoding.rs

+ for value in values {
+ encode(tag, value, buf);
+ }


Could int32::encode_repeated be reused here?

No, unless int32::encode_repeated is generalized to take an impl IntoIterator that produces i32. Which might be a good idea.

caspermeijn · 2024-05-31T08:31:50Z

prost/src/encoding.rs

+ if values.is_empty() {
+ return;
+ }
+
+ encode_key(tag, WireType::LengthDelimited, buf);
+ let len: usize = values
+ .iter()
+ .map(|value| encoded_len_varint(value.to_raw() as u64))
+ .sum();
+ encode_varint(len as u64, buf);
+
+ for value in values {
+ encode_varint(value.to_raw() as u64, buf);
+ }


Could int32::encode_packed be reused here?

caspermeijn · 2024-05-31T08:32:56Z

prost/src/encoding.rs

+ if wire_type == WireType::LengthDelimited {
+ // Packed.
+ merge_loop(values, buf, ctx, |values, buf, ctx| {
+ let mut value = Default::default();
+ merge(WireType::Varint, &mut value, buf, ctx)?;
+ values.push(value);
+ Ok(())
+ })
+ } else {
+ // Unpacked.
+ check_wire_type(WireType::Varint, wire_type)?;
+ let mut value = Default::default();
+ merge(wire_type, &mut value, buf, ctx)?;
+ values.push(value);
+ Ok(())
+ }


Could int32::merge_repeated be reused here?

caspermeijn · 2024-05-31T08:33:28Z

prost/src/encoding.rs

+ key_len(tag) * values.len()
+ + values
+ .iter()
+ .map(|value| encoded_len_varint(value.to_raw() as u64))
+ .sum::<usize>()


Could int32::encoded_len_repeated be reused here?

caspermeijn · 2024-05-31T08:34:07Z

prost/src/encoding.rs

+ if values.is_empty() {
+ 0
+ } else {
+ let len = values
+ .iter()
+ .map(|value| encoded_len_varint(value.to_raw() as u64))
+ .sum::<usize>();
+ key_len(tag) + encoded_len_varint(len as u64) + len
+ }


Could int32::encoded_len_packed be reused here?

caspermeijn · 2024-05-31T08:41:32Z

prost/src/open_enum.rs

+ let mut raw = 0;
+ <i32 as Message>::merge_field(&mut raw, tag, wire_type, buf, ctx)?;
+ *self = OpenEnum::from_raw(raw);
+ Ok(())


This is the same function as encoding::enumeration::merge, right?

mzabaluev · 2024-06-02T06:54:05Z

Will this break compatibility with proto2/closed enums? Personally, I don't use proto2, so I am not sure whether it is properly supported at all.

I believe prost has never been conformant with closed enums: the raw values have been left in the message as is.
I think we could try to support closed enums in two ways:

Always represent an enum field as OpenEnum, but in the closed enum case, decode unknown values as Known(Default::default()). This leaves the possibility of producing unknown values on encoding.
Represent closed enums with their generated Rust enum type. Decode unknown values as Default::default().

I prefer option 2, even though it requires more work. Protobuf edition 2023 has closed enums as a feature, so they need to be supported even if we ignore proto2.

mzabaluev · 2024-06-02T07:14:43Z

I think it makes sense to provide a migration guide.

What would be a good place for it? I can add a section to the README.

mzabaluev · 2024-06-02T07:32:47Z

I would like to see some tests for OpenEnum.

I'm going to add at least one good example/doctest on the type.

caspermeijn · 2024-06-07T11:55:39Z

Have thought some more about this PR: I don't want to break users in this way. At least not now.

I suggest making this an option in prost-build so that we can experiment with the API without breaking existing users. That way, interested users can opt in, and we don't have to create a perfect OpenEnum API on the first try.

Once we feel good about the new API, we can think about changing the default.

Please look at bytes for a good example of changing the generated data type.

ArjixWasTaken · 2024-06-28T19:42:19Z

Instead of introducing a new type, a simple Result<T, i32> sounds more logical to me.
enum-repr-derive follows this approach when parsing an enum from i32

mzabaluev · 2024-06-29T13:41:40Z

Instead of introducing a new type, a simple Result<T, i32> sounds more logical to me.

It's weird to have a Result as a struct field. The names of variants and methods of Result are less than intuitively applicable: it's not necessarily an error to receive an unknown enum value from the wire, so we should give the API users a speed bump to decide how to deal with them. There are convenience methods and the TryInto impl to convert the OpenEnum value to a Result if that's the chosen approach.

Another benefit of introducing a new type is for the add-on macros and code generators that derive something on structs generated by prost-build. These could deal with the OpenEnum type in some specific way, even without access to the proto descriptor data for the field. Doing the same (e.g. defining generic trait impls) for Result would feel like too much overloading.

caspermeijn · 2024-08-09T06:01:14Z

How are default values handles in this solution? Especially default values set for a specific field in the proto file.

mzabaluev added 3 commits May 28, 2024 13:14

prost: add OpenEnum

4beb811

The wrapper to represent field of enumerated types with the possibility of unknown values.

prost: encoding module for OpenEnum

faa74d1

Use OpenEnum in generated code

8bd451d

Replace i32 with the type-checked wrapper.

mzabaluev mentioned this pull request May 28, 2024

OpenEnum: type-safe wrapper for enum field values #1061

Closed

mzabaluev added 3 commits May 28, 2024 23:50

prost: document OpenEnum

a163f69

Restore cleanup in tests

1702c54

Forgot to re-enable tempdir cleanup disabled for debugging.

prost-derive: streamline Debug impl of enums

9ec7c5d

mzabaluev marked this pull request as ready for review May 28, 2024 21:08

caspermeijn requested changes May 31, 2024

View reviewed changes

caspermeijn added the breaking change label May 31, 2024

caspermeijn mentioned this pull request May 31, 2024

getter/setter generated for optional enum fields should use Option #1027

Open

ArjixWasTaken mentioned this pull request Jun 28, 2024

A more ergonomic enumeration representation #276

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`OpenEnum`: an enum to represent protobuf's enumeration field values #1079

`OpenEnum`: an enum to represent protobuf's enumeration field values #1079

mzabaluev commented May 28, 2024 •

edited

Loading

caspermeijn commented May 28, 2024

QuentinPerez commented May 28, 2024

mzabaluev commented May 29, 2024 •

edited

Loading

caspermeijn commented May 31, 2024

caspermeijn commented May 31, 2024

caspermeijn commented May 31, 2024

caspermeijn commented May 31, 2024

caspermeijn May 31, 2024

mzabaluev Jun 2, 2024

mzabaluev Jun 2, 2024 •

edited

Loading

caspermeijn May 31, 2024

mzabaluev Jun 2, 2024

mzabaluev Jun 2, 2024

caspermeijn Jun 7, 2024

caspermeijn May 31, 2024

caspermeijn May 31, 2024

caspermeijn May 31, 2024

mzabaluev Jun 2, 2024

caspermeijn May 31, 2024

caspermeijn May 31, 2024

caspermeijn May 31, 2024

caspermeijn May 31, 2024

caspermeijn May 31, 2024

mzabaluev commented Jun 2, 2024

mzabaluev commented Jun 2, 2024

mzabaluev commented Jun 2, 2024

caspermeijn commented Jun 7, 2024

ArjixWasTaken commented Jun 28, 2024 •

edited

Loading

mzabaluev commented Jun 29, 2024

caspermeijn commented Aug 9, 2024

OpenEnum: an enum to represent protobuf's enumeration field values #1079

Are you sure you want to change the base?

OpenEnum: an enum to represent protobuf's enumeration field values #1079

Conversation

mzabaluev commented May 28, 2024 • edited Loading

caspermeijn commented May 28, 2024

QuentinPerez commented May 28, 2024

mzabaluev commented May 29, 2024 • edited Loading

caspermeijn commented May 31, 2024

caspermeijn commented May 31, 2024

caspermeijn commented May 31, 2024

caspermeijn commented May 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzabaluev Jun 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzabaluev commented Jun 2, 2024

mzabaluev commented Jun 2, 2024

mzabaluev commented Jun 2, 2024

caspermeijn commented Jun 7, 2024

ArjixWasTaken commented Jun 28, 2024 • edited Loading

mzabaluev commented Jun 29, 2024

caspermeijn commented Aug 9, 2024

`OpenEnum`: an enum to represent protobuf's enumeration field values #1079

`OpenEnum`: an enum to represent protobuf's enumeration field values #1079

mzabaluev commented May 28, 2024 •

edited

Loading

mzabaluev commented May 29, 2024 •

edited

Loading

mzabaluev Jun 2, 2024 •

edited

Loading

ArjixWasTaken commented Jun 28, 2024 •

edited

Loading