[
https://issues.apache.org/jira/browse/THRIFT-5392?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Samuele Maci updated THRIFT-5392:
---------------------------------
Description:
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.
was:
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.
> 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)
