tokio-rs / prost

PROST! a Protocol Buffers implementation for the Rust Language
Apache License 2.0
3.73k stars 482 forks source link

prost automatic naming convention causes build failure on valid protobuf #665

Open gen-xu opened 2 years ago

gen-xu commented 2 years ago

Minimum re-produceable example of protobuf

syntax = "proto3";
package foobar;
enum Fb { 
    Fb1 = 0;
}
enum FB {
    FB2 = 0;
}

Since prost rename FB into Fb and it generates conflicts definitions like following and causes compiling failure in some cases

Is there any way to enforce naming convention in prost? I think it relates with this issue https://github.com/tokio-rs/prost/issues/593

But since this causes compiling failure only in prost but works with other languages with protoc I would deem this as a bug.

#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, ::prost::Enumeration)]
#[repr(i32)]
pub enum Fb {
    Fb1 = 0,
}
#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, ::prost::Enumeration)]
#[repr(i32)]
pub enum Fb {
    Fb2 = 0,
}
LucioFranco commented 2 years ago

This seems like an interesting corner case, I don't see how we can fix this without exposing more fine grained controls which I am not super happy about. That said, I want to say this goes against protobuf naming conventions so I am not sure how much I want to support that.

gen-xu commented 2 years ago

@LucioFranco yes I agree it goes against naming convention. But sometime I think there are needs for that. For example XML, GPIO are very commonly used abbreviations together as all capitals cases. Also I am not sure if this is a bug of heck that does the naming conversion in Prost. As to me I think the upper camel cases applied on FB should still lead into FB.

BatmanAoD commented 3 months ago

Would it be reasonable to support suppressing renaming for messages and enums entirely?

That seems similar to what's requested here, except that request is specific to the members of an enum rather than the enum name itself.

I ask because I have some protobuf that has a fairly odd naming convention that mostly follows the Go style, where initialisms are generally upper-cased, but we have some words in our names that are kind of like "marketing names" where there's a specific capitalization (think eXistenZ but slightly less ludicrous). I think if we were designing our schema today we'd probably treat these as normal words (existenz/Existenz), but at this point it's probably too late to make that change without a lot of unnecessary code churn in projects that use the JSON serialization of the Proto structs.

So it would be very convenient for us to have a way to simply turn off Prost's automatic renaming.

stefanvanburen commented 1 month ago

Have also seen an issue like this crop up with the following code (BSR permalink):

// NOTE: Trimmed out irrelevant context.
message StakeAuthorization {
  oneof validators {
    Validators allow_list = 2;
    Validators deny_list = 3;
  }
  message Validators {
    repeated string address = 1;
  }
}

prost will generate both a pub struct Validators for the nested message, and a pub enum Validators for the oneof, causing a conflict. Both the oneof and nested message names are following the style guide.

caspermeijn commented 3 weeks ago

prost will generate both a pub struct Validators for the nested message, and a pub enum Validators for the oneof, causing a conflict. Both the oneof and nested message names are following the style guide.

I think this is a different and more interesting issue than the original example. I feel like in this case, the enum for oneof should be automatically renamed to prevent this conflict. In the C++ generated code there is no generated type for oneof and therefore it doesn't result in a conflict.

@stefanvanburen Are you willing to work on a PR for solving this name conflict?

stefanvanburen commented 3 weeks ago

@caspermeijn I unfortunately don't have the bandwidth / Rust expertise to contribute a fix at the moment. For the future, do you have a suggestion as to how something like this would be implemented? I imagine prost has to deal with conflicting identifiers in other situations.

caspermeijn commented 3 weeks ago

do you have a suggestion as to how something like this would be implemented? I imagine prost has to deal with conflicting identifiers in other situations.

The current situation is that it doesn't handle those cases. Sometimes it is detected, and a panic is thrown during build. If you follow protobuf naming convention and choose unique names, then there are no problems. This is the first case, that I have seen, where a design decision of prost is troublesome.

caspermeijn commented 1 week ago

The example of @stefanvanburen has an other issue dedicated to it: https://github.com/tokio-rs/prost/issues/505