[
https://issues.apache.org/jira/browse/THRIFT-5392?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17317108#comment-17317108
]
Allen George commented on THRIFT-5392:
--------------------------------------
Link to the comment where I considered it:
https://issues.apache.org/jira/browse/THRIFT-5314?focusedCommentId=17235560&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17235560
The downsides were that each enum has to have a magic unknown value that
everyone has to check for.
> 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)
