[THRIFT-5392] Thrift Enums should generate forward compatible enum like code

• Type: Improvement
• Status: Open
• Priority: Major
• Resolution: Unresolved
• Component/s: Rust - Compiler, Rust - Library
• Labels: improvement rust
• Assignee: Allen George
• Reporter: Samuele Maci
• Created: 08/Apr/21 09:11
• Updated: 01/Aug/21 15:53

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

# Fully made up enum to use as example
enum HttpStatusCode {
  Ok = 200,
  Created = 201,
  Accepted = 202,
} 

is currently rendered as

// 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) }
}

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

  let enum_value: HttpStatusCode = foo();
  match enum_value {
    HttpStatusCode::Ok => do_ok(),
    HttpStatusCode::Created => do_ok(),
    // HttpStatusCode::Accepted => ...  // This is intentionally left out
  }

• we allow creating un-existing enums from code

  let enum_value = HttpStatusCode(1234);  // I would have expected an error

  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)

  let enum_value = HttpStatusCode::try_from("Ok")?;  // I would have expected to be possible

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)

#[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());
}

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.

Issue Links

• supercedes: THRIFT-5314 Enum forward compatibility