Bug: type attributes are not deduplicated, leading to invalid generated code

[poliorcetics]

I used the following:

# Cargo.toml
[package]
name = "prost-repro"
version = "0.1.0"
edition = "2021"

[dependencies]
prost = "0.11.6"

[build-dependencies]
prost-build = "0.11.6"

// src/test.proto
syntax = "proto3";

package test;

message Test {
    enum TestEnum {
        TEST_ENUM_VALUE_0 = 0;
    }

    TestEnum member = 1;
}

// build.rs
fn main() -> std::io::Result<()> {
    prost_build::Config::new()
        .type_attribute(".test.Test", "#[derive(Copy)]")
        .compile_protos(&["src/test.proto"], &["src/"])?;
    Ok(())
}

include!(concat!(env!("OUT_DIR"), "/test.rs"));

fn main() {
    println!("{}", env!("OUT_DIR"));
    println!("Hello, world!");
}

This produces the following code in OUT_DIR/test.rs:

#[derive(Copy)]
#[allow(clippy::derive_partial_eq_without_eq)]
#[derive(Clone, PartialEq, ::prost::Message)]
pub struct Test {
    #[prost(enumeration = "test::TestEnum", tag = "1")]
    pub member: i32,
}
/// Nested message and enum types in `Test`.
pub mod test {
    #[derive(Copy)]
    #[derive(
        Clone,
        Copy,
        Debug,
        PartialEq,
        Eq,
        Hash,
        PartialOrd,
        Ord,
        ::prost::Enumeration
    )]
    #[repr(i32)]
    pub enum TestEnum {
        Value0 = 0,
    }
    impl TestEnum {
        /// 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 {
                TestEnum::Value0 => "TEST_ENUM_VALUE_0",
            }
        }
        /// Creates an enum from field names used in the ProtoBuf definition.
        pub fn from_str_name(value: &str) -> ::core::option::Option<Self> {
            match value {
                "TEST_ENUM_VALUE_0" => Some(Self::Value0),
                _ => None,
            }
        }
    }
}

[LucioFranco]

If Copy is already derived then why do you need to also add it to type attributes?

[poliorcetics]

I only asked for it in message Test, not in the TestEnum

[LucioFranco]

So deduping the strings will be complicated and a quick solution will not work. I think the real work around in this case is to not have an enum named the same as the message. I think that is the clear conflict here. That said, in this case can you just not add copy and use clone instead?

[poliorcetics]

syntax = "proto3";

package test;

message Test {
    enum NamedDifferently {
        TEST_ENUM_VALUE_0 = 0;
    }

    NamedDifferently member = 1;
}

Still has the bug.

That said, in this case can you just not add copy and use clone instead?

Here yes, but what about deriving Ord, Eq or Hash ? Those have the exact same problem and are often necessary (especially Hash, to stick some messages in a HashSet for example)

[LucioFranco]

In your example there what is the code that is generated?

[poliorcetics]

Asking for derive(Hash) instead of Copy:

#[derive(Hash)]
#[allow(clippy::derive_partial_eq_without_eq)]
#[derive(Clone, PartialEq, ::prost::Message)]
pub struct Test {
    #[prost(enumeration = "test::NamedDifferently", tag = "1")]
    pub member: i32,
}
/// Nested message and enum types in `Test`.
pub mod test {
    #[derive(Hash)]
    #[derive(
        Clone,
        Copy,
        Debug,
        PartialEq,
        Eq,
        Hash,
        PartialOrd,
        Ord,
        ::prost::Enumeration
    )]
    #[repr(i32)]
    pub enum NamedDifferently {
        TestEnumValue0 = 0,
    }
    impl NamedDifferently {
        /// 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 {
                NamedDifferently::TestEnumValue0 => "TEST_ENUM_VALUE_0",
            }
        }
        /// Creates an enum from field names used in the ProtoBuf definition.
        pub fn from_str_name(value: &str) -> ::core::option::Option<Self> {
            match value {
                "TEST_ENUM_VALUE_0" => Some(Self::TestEnumValue0),
                _ => None,
            }
        }
    }
}

[LucioFranco]

Is the type_attribute the same? I am not seeing why it would apply it to test.Test and then apply it to NamedDifferently.

[poliorcetics]

Yes it's the same. I think it applies because the NamedDifferently enum is nested inside the Test message

[LucioFranco]

So actually, I believe the implementation is correct, I tried this locally but if you move the enum out of the message it works. I know sometimes you can't change the proto but in this case for the implementation we have in prost I think what we have now makes sense and the correct solution is to move the enum out from under that name space.

Now one small idea would be to have some sort of path syntax that isn't basicallytest.Test.* but I am not sure how to approach that in a way that makes me happy.

[poliorcetics]

the correct solution is to move the enum out from under that name space

I can't do that in my case. The example here could yes, but the bug would still be there and at $work I don't control the protos that much. For now I can impl Copy for <type> after the include!() call but it's spotty at best.

[poliorcetics]

If you have guidance on how to fix this, I can certainly work on it, expecting you to fix everything for free is unreasonable

[LucioFranco]

I unfortunately don't know a fix that would personally make me happy since I think the current and simple approach works. I think working around it like what you are doing is likely the best approach and then either trying to convince upstream to change their proto.

[poliorcetics]

A (breaking) approach would be to not apply attributes to inner messages and enums and instead ask for them.

I don't know how willing the project is to make breaking changes though. That one would probably be easy but long to fix for users too.