A more lightweight, informative error value in `TryFrom<i32>` for enums

tokio-rs / prost

PROST! a Protocol Buffers implementation for the Rust Language

Apache License 2.0

3.87k stars 501 forks source link

A more lightweight, informative error value in `TryFrom<i32>` for enums #1010

Closed mzabaluev closed 4 months ago

mzabaluev commented 6 months ago

DecodeError as the error type of the generated TryFrom<i32> conversions for enum values is problematic:

It's overweight for the call sites converting a field value to its representative domain-safe enum, as it's generally meant to describe a decoding failure on a wire message. Construction of a DecodeError value makes an allocation even with a static message.
It erases information about the raw value, both programmatically (there's only a cause string) and as currently generated, from the displayed message (could use format! in the conversion impls, but that'll cost another allocation).

Add a new UnknownEnumValue error type to prost. Change the TryFrom<i32> conversions generated for enums to use that as the associated Error type. The error value carries the original integer as the public inner member.

As this is a breaking change, it's not meant for 0.12, but for a future version.

mzabaluev commented 5 months ago

I think the TryFrom impl generated for an enum is a separate case from the protobuf message decoding error. It arises in a different context: the consumer tries to convert a field of an already decoded message to its nominal enum type. DecodeError, for its part, has all the data to represent a failure arising when a message is decoded.

caspermeijn commented 5 months ago

I think the TryFrom impl generated for an enum is a separate case from the protobuf message decoding error. It arises in a different context: the consumer tries to convert a field of an already decoded message to its nominal enum type. DecodeError, for its part, has all the data to represent a failure arising when a message is decoded.

That makes sense. I do think that DecodeError should be changed to a non_exthaustive enum instead of a string, but that is an issue for another day.

caspermeijn commented 4 months ago

@mzabaluev The main prost library is moved to a subdirectory. Could you solve the merge conflict?

mzabaluev commented 4 months ago

The main prost library is moved to a subdirectory. Could you solve the merge conflict?

It's easy to do, but I will wait on #1068 to make sure tests pass.

caspermeijn commented 4 months ago

It's easy to do, but I will wait on #1068 to make sure tests pass.

That PR is merged now

mzabaluev commented 4 months ago

@mzabaluev The main prost library is moved to a subdirectory. Could you solve the merge conflict?

Done against 683091ae47c84545e22ee70ebbb1bbead9a8990e