Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Question] why not TryFrom<i32> instead of from_i32 in derive(prost::Enumeration)? #812

Open
Tudyx opened this issue Feb 6, 2023 · 4 comments

Comments

@Tudyx
Copy link

Tudyx commented Feb 6, 2023

I've noticed that in derive prost::Enumeration we don't implement the std trait TryFrom but instead we use a custom function from_i32. Is there a particular reason for this (for instance MSRV) ?

I'm currently working on a macro that generates the boiler plate conversion functions from Prost generated struct that might contain unwanted Option because proto3 which doesn't support required fields.

For that it would be nice for me if I could have a generic implementation for types that have this function from_i32, that's why it would be nice for me to have it on a trait.
Its currently working for all other protobuf type (nested or not) and I might have found a workaround for this, I was just wondering why the things are done this way.

@LucioFranco
Copy link
Member

That code was written before TryFrom was stable :) I think we can add it now though.

@Leulz
Copy link
Contributor

Leulz commented Apr 21, 2023

That code was written before TryFrom was stable :) I think we can add it now though.

Hey, @LucioFranco, I'm interested in implementing TryFrom for the enums that derive prost::Enumeration. Can you assign this issue to me, please?

Also, should the change make from_i32 deprecated and change whatever uses from_i32 (like this) to use try_from? Or should it just add the option to use TryFrom but keep from_i32?

@messense
Copy link

I'm getting a conflicting implementations of trait TryFrom<i32> for type Type error when trying to upgrade to tonic 0.10.0 for etcd-client, I think it might related to #853?

diff --git a/Cargo.toml b/Cargo.toml
index 1b4737d..b4dbcb8 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -19,8 +19,8 @@ tls-roots = ["tls", "tonic/tls-roots"]
 pub-response-field = ["visible"]

 [dependencies]
-tonic = "0.9.2"
-prost = "0.11.9"
+tonic = "0.10.0"
+prost = "0.12.0"
 tokio = "1.28.0"
 tokio-stream = "0.1.14"
 tower-service = "0.3.2"
@@ -35,7 +35,7 @@ hyper-openssl = { version = "0.9", optional = true }
 tokio = { version = "1.28.0", features = ["full"] }

 [build-dependencies]
-tonic-build = { version = "0.9.2", default-features = false, features = ["prost"] }
+tonic-build = { version = "0.10.0", default-features = false, features = ["prost"] }

 [package.metadata.docs.rs]
 features = ["tls", "tls-roots"]

gives a compile error:

   Compiling tonic v0.10.0
error[E0119]: conflicting implementations of trait `TryFrom<i32>` for type `Type`
  --> /Users/messense/Projects/etcd-client/target/debug/build/etcd-client-a1114818a2a0846b/out/authpb.rs:42:9
   |
42 |         ::prost::Enumeration
   |         ^^^^^^^^^^^^^^^^^^^^
   |
   = note: conflicting implementation in crate `core`:
           - impl<T, U> TryFrom<U> for T
             where U: Into<T>;
   = note: this error originates in the derive macro `::prost::Enumeration` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0119`.

the generated authpb.rs content is

#[allow(clippy::derive_partial_eq_without_eq)]
#[derive(Clone, PartialEq, ::prost::Message)]
pub struct UserAddOptions {
    #[prost(bool, tag = "1")]
    pub no_password: bool,
}
/// User is a single entry in the bucket authUsers
#[allow(clippy::derive_partial_eq_without_eq)]
#[derive(Clone, PartialEq, ::prost::Message)]
pub struct User {
    #[prost(bytes = "vec", tag = "1")]
    pub name: ::prost::alloc::vec::Vec<u8>,
    #[prost(bytes = "vec", tag = "2")]
    pub password: ::prost::alloc::vec::Vec<u8>,
    #[prost(string, repeated, tag = "3")]
    pub roles: ::prost::alloc::vec::Vec<::prost::alloc::string::String>,
    #[prost(message, optional, tag = "4")]
    pub options: ::core::option::Option<UserAddOptions>,
}
/// Permission is a single entity
#[allow(clippy::derive_partial_eq_without_eq)]
#[derive(Clone, PartialEq, ::prost::Message)]
pub struct Permission {
    #[prost(enumeration = "permission::Type", tag = "1")]
    pub perm_type: i32,
    #[prost(bytes = "vec", tag = "2")]
    pub key: ::prost::alloc::vec::Vec<u8>,
    #[prost(bytes = "vec", tag = "3")]
    pub range_end: ::prost::alloc::vec::Vec<u8>,
}
/// Nested message and enum types in `Permission`.
pub mod permission {
    #[derive(
        Clone,
        Copy,
        Debug,
        PartialEq,
        Eq,
        Hash,
        PartialOrd,
        Ord,
        ::prost::Enumeration
    )]
    #[repr(i32)]
    pub enum Type {
        Read = 0,
        Write = 1,
        Readwrite = 2,
    }
    impl Type {
        /// String value of the enum field names used in the ProtoBuf definition.
        ///
        /// The values are not transformed in any way and thus are considered stable
        /// (if the ProtoBuf definition does not change) and safe for programmatic use.
        pub fn as_str_name(&self) -> &'static str {
            match self {
                Type::Read => "READ",
                Type::Write => "WRITE",
                Type::Readwrite => "READWRITE",
            }
        }
        /// Creates an enum from field names used in the ProtoBuf definition.
        pub fn from_str_name(value: &str) -> ::core::option::Option<Self> {
            match value {
                "READ" => Some(Self::Read),
                "WRITE" => Some(Self::Write),
                "READWRITE" => Some(Self::Readwrite),
                _ => None,
            }
        }
    }
}
/// Role is a single entry in the bucket authRoles
#[allow(clippy::derive_partial_eq_without_eq)]
#[derive(Clone, PartialEq, ::prost::Message)]
pub struct Role {
    #[prost(bytes = "vec", tag = "1")]
    pub name: ::prost::alloc::vec::Vec<u8>,
    #[prost(message, repeated, tag = "2")]
    pub key_permission: ::prost::alloc::vec::Vec<Permission>,
}

@LucioFranco
Copy link
Member

@messense this seems like a bug, do you have a minimal repro of the enum issue I can take a look at? We should encode it into a test anyways

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants