[
https://issues.apache.org/jira/browse/THRIFT-5392?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17317119#comment-17317119
]
Samuele Maci edited comment on THRIFT-5392 at 4/8/21, 2:27 PM:
---------------------------------------------------------------
I was writing specifically about the comment and trying to provide context, but
got caught in a call.
The impression I've got by reading the other task's comments is that it was
decided to opt for C-style enums because it was eventually easier and because
others do it and more importantly
{quote}
If the C++ and Java implementations work in a given way, I think it makes sense
to align newer implementations with them
{quote}
does not look like a strong argument to me especially as it would lead to
allowing common features only (ignoring that different platforms might provide
different benefits).
Deciding to use C-style enums fully remove one of the most appealing features
that Rust provides around enums which is exhaustive matching and this is a
feature that not all languages have (definitely not C++ and Java).
What I see is that on flatbuffers the main motivation to decide to use C-style
instead of rust-style enums is caused by the fact that type conversion is not
needed because the enum is a type-wrapping around an int type.
Additionally all the discussion was mostly focused on using Result<_, _> to
handle forward compatibility which is very idiomatic but has some memory costs
(due to the size of Err instances).
The question here is:
* what are the tread-offs that lead thrift-rust to not leverage rust-enum
capabilities?
* is it intentional to not have pattern matching?
* do we really want to allow creation of enums with any value to propagate into
the rust codebase?
I guess, that before any discussion there should be a clear statement of the
status quo.
---
*Edit*: forgot to mention in the issue description. The decision of using
int-type-wrapping strategy does also have an impact on debug-ability as Debug
trait is not defined, and even if it would we would have an output like
"HttpStatusCode(200)" instead of the expected "HttpStatusCode::Ok".
Sure, we could generate the code implementing Debug (and eventually Display)
traits but it requires generation and still is open the issue of "how to handle
not-supported enum values"
was (Author: macisamuele):
I was writing specifically about the comment and trying to provide context, but
got caught in a call.
The impression I've got by reading the other task's comments is that it was
decided to opt for C-style enums because it was eventually easier and because
others do it and more importantly
{quote}
If the C++ and Java implementations work in a given way, I think it makes sense
to align newer implementations with them
{quote}
does not look like a strong argument to me especially as it would lead to
allowing common features only (ignoring that different platforms might provide
different benefits).
Deciding to use C-style enums fully remove one of the most appealing features
that Rust provides around enums which is exhaustive matching and this is a
feature that not all languages have (definitely not C++ and Java).
What I see is that on flatbuffers the main motivation to decide to use C-style
instead of rust-style enums is caused by the fact that type conversion is not
needed because the enum is a type-wrapping around an int type.
Additionally all the discussion was mostly focused on using Result<_, _> to
handle forward compatibility which is very idiomatic but has some memory costs
(due to the size of Err instances).
The question here is:
* what are the tread-offs that lead thrift-rust to not leverage rust-enum
capabilities?
* is it intentional to not have pattern matching?
* do we really want to allow creation of enums with any value to propagate into
the rust codebase?
I guess, that before any discussion there should be a clear statement of the
status quo.
> 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)
