[jira] [Comment Edited] (THRIFT-5392) Thrift Enums should generate forward compatible enum like code

["Allen George (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/THRIFT-5392?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17317107#comment-17317107 ] 

Allen George edited comment on THRIFT-5392 at 4/8/21, 11:28 AM:
----------------------------------------------------------------

I considered using a sentinel value, but note that it would have to be something prefixed specially - for example: {{Type::__Unknown(actual_unknown_value)}} - to avoid name clashes. In the end, I took a close look at the Flatbuffers issue linked in the ticket and decided to use their approach.

As you've noted, given the churn in the enum space (this is the 3rd time I've had to change the enum generation) I'm incredibly reluctant to change the output unless I have wider community input.


was (Author: allengeorge):
I considered using a sentinel value, but note that it would have to be something prefixed specially - for example: {{Type::__Unknown}} - to avoid name clashes. In the end, I took a close look at the Flatbuffers issue linked in the ticket and decided to use their approach.

As you've noted, given the churn in the enum space (this is the 3rd time I've had to change the enum generation) I'm incredibly reluctant to change the output unless I have wider community input.

> Thrift Enums should generate forward compatible enum like code
> --------------------------------------------------------------
>
>                 Key: THRIFT-5392
>                 URL: https://issues.apache.org/jira/browse/THRIFT-5392
>             Project: Thrift
>          Issue Type: Improvement
>          Components: Rust - Compiler, Rust - Library
>            Reporter: Samuele Maci
>            Priority: Major
>              Labels: improvement, rust
>
> I'm starting using thrift interfaces in rust and I've been surprised to discover that thrift enums are not generated as rust enums.
> The following thrift enum
> {code:java}
> # Fully made up enum to use as example
> enum HttpStatusCode {
>   Ok = 200,
>   Created = 201,
>   Accepted = 202,
> } 
> {code}
> is currently rendered as
> {code:java}
> // The generated code has been shrank a bit for readability
> #[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
> pub struct HttpStatusCode(pub i32);
> impl HttpStatusCode {
>     pub const Ok: Self = HttpStatusCode(200);
>     pub const Created: Self = HttpStatusCode(201);
>     pub const Accepted: Self = HttpStatusCode(202);
> }
> impl ::fbthrift::ThriftEnum for HttpStatusCode {
>     fn enumerate() -> &'static [(HttpStatusCode, &'static str)] {
>         &[(Self::Ok, "Ok"), (Self::Created, "Created"), (Self::Accepted, "Accepted")]
>     }
>     fn variants() -> &'static [&'static str] {
>         &["Ok", "Created", "Accepted"]
>     }
>     fn variant_values() -> &'static [HttpStatusCode] {
>         &[Self::Ok, Self::Created, Self::Accepted]
>     }
> }
> impl Default for HttpStatusCode {
>     fn default() -> Self { HttpStatusCode(::fbthrift::__UNKNOWN_ID) }
> }
> impl<'a> From<&'a HttpStatusCode> for i32 {
>     fn from(x: &'a HttpStatusCode) -> Self { x.0 }
> }
> impl From<HttpStatusCode> for i32 {
>     fn from(x: HttpStatusCode) -> Self { x.0 }
> }
> impl From<i32> for HttpStatusCode {
>     fn from(x: i32) -> Self { Self(x) }
> }
> {code}
> The generated code poses, in my view, some limitations as well as it does not use the powerful rust compiler capabilities:
>  * having a struct instead of enum removes the capability of the compiler to verify for exhaustive matching. The code below would compile
> {code:java}
> let enum_value: HttpStatusCode = foo();
> match enum_value {
>   HttpStatusCode::Ok => do_ok(),
>   HttpStatusCode::Created => do_ok(),
>   // HttpStatusCode::Accepted => ...  // This is intentionally left out
> }{code}
>  * we allow creating un-existing enums from code
> {code:java}
> let enum_value = HttpStatusCode(1234);  // I would have expected an error{code}
> I would have expected that TryFrom<i32> was implemented and not the infallible form From<i32>
>  * we do not allow creating enums from variant names (but we do it from enum "binary" value)
> {code:java}
> let enum_value = HttpStatusCode::try_from("Ok")?;  // I would have expected to be possible{code}
> I do see that the conversion from rust enum to rust struct was done in THRIFT-5314 for forward-compatibility but I'm wondering if that is the best way forward to ensure that we can levarage what the rust ecosystem can provide us.
>  This is mostly meant to lift developers from some tedious and error-prone checks which the compiler knows how to do.
>  NOTE: Working of a wide code-base and depending on thrift definition of thrid part services makes very hard to keep track of all the changes and having the compiler reporting the issues is a non-negligible advantage.
> *How could we move forward without impacting way too much on the current experience?*
>  My idea would be to have back enum variants and in order to have backward/forward capabilities I would suggest the introduction of a sentinel enum variant (as using Result<HttpStatusCode, ::fbthrift::Error> does not look appealing to most)
> {code:java}
> #[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)]
> pub enum HttpStatusCode {
>   Unknown,  // Maybe the variant name could be configurable
>   Ok, Created, Accepted,
> }
> impl ::fbthrift::ThriftEnum for HttpStatusCode {
>     // As current 
>     ...
> }
> impl Default for HttpStatusCode {
>     fn default() -> Self { Self::Unknown }
> }
> impl<'a> From<&'a HttpStatusCode> for i32 {
>     fn from(x: &'a HttpStatusCode) -> Self {
>         match x {
>             HttpStatusCode::Unknown => ::fbthrift::__UNKNOWN_ID,
>             HttpStatusCode::Ok => 200,
>             HttpStatusCode::Created => 201,
>             HttpStatusCode::Accepted => 202,
>         }
>     }
> }
> impl From<HttpStatusCode> for i32 {
>     fn from(x: HttpStatusCode) -> Self { Self::from(&x) }
> }
> impl TryFrom<i32> for HttpStatusCode {
>     type Error = ::fbthrift::ProtocolError;
>     fn try_from(x: i32) -> Result<Self, Self::Error> {
>         match x {
>             200 => Ok(Self::Ok),
>             201 => Ok(Self::Created),
>             202 => Ok(Self::Accepted),
>             _ => Err(::fbthrift::ProtocolError::InvalidValue),
>         }
>     }
> }
> impl HttpStatusCode {
>     // Not From trait because it conflicts with From implementation from TryFrom
>     fn from(x: i32) -> Self {
>         Self::try_from(x).unwrap_or_default()
>     }
> }
> impl<'a> std::convert::TryFrom<&'a str> for HttpStatusCode {
>     type Error = ::fbthrift::ProtocolError;
>     fn try_from(x: &'a str) -> Result<Self, Self::Error> {
>         match x {
>             "Ok" => Ok(Self::Ok),
>             "Created" => Ok(Self::Created),
>             "Accepted" => Ok(Self::Accepted),
>             _ => Err(::fbthrift::ProtocolError::InvalidValue),
>         }
>     }
> }
> #[test]
> fn dummy_test() {
>     assert_eq!(HttpStatusCode::Ok, HttpStatusCode::from(200));
>     assert_eq!(HttpStatusCode::Unknown, HttpStatusCode::from(300));
>     assert!(HttpStatusCode::try_from(200).is_ok());
>     assert!(HttpStatusCode::try_from(300).is_err());
> }
> {code}
> Before eventually moving forward with updating the compiler to support the new schema (and I can be doing so) I would like to have a sort of discussion in order to avoid investing time toward a non-acceptable/non-ideal direction.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)