[jira] [Updated] (THRIFT-5392) Thrift Enums should generate enum like code

["Samuele Maci (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/THRIFT-5392?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Samuele Maci updated THRIFT-5392:
---------------------------------
    Component/s: Rust - Library

> Thrift Enums should generate 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)