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

generate assoc. consts in enums representing protobuf enum aliases, resolves #792 #849

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 44 additions & 7 deletions prost-build/src/code_generator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -662,7 +662,12 @@ impl<'a> CodeGenerator<'a> {

self.depth += 1;
self.path.push(2);
let mut aliases = vec![];
for variant in variant_mappings.iter() {
if variant.alias_of.is_some() {
aliases.push(variant);
continue;
}
self.path.push(variant.path_idx as i32);

self.append_doc(&fq_proto_enum_name, Some(variant.proto_name));
Expand All @@ -688,7 +693,23 @@ impl<'a> CodeGenerator<'a> {
self.buf.push_str(" {\n");
self.depth += 1;
self.path.push(2);

if aliases.len() > 0 {
self.push_indent();
self.buf.push_str("// Aliases.\n");

for variant in &aliases {
self.push_indent();
self.buf.push_str("#[allow(non_upper_case_globals)]");
self.push_indent();
self.buf.push_str(&format!(
"pub const {}: {} = {}::{};\n",
variant.generated_variant_name,
enum_name,
enum_name,
variant.alias_of.as_ref().unwrap()
));
}
}
self.push_indent();
self.buf.push_str(
"/// String value of the enum field names used in the ProtoBuf definition.\n",
Expand All @@ -713,6 +734,9 @@ impl<'a> CodeGenerator<'a> {
self.depth += 1;

for variant in variant_mappings.iter() {
if variant.alias_of.is_some() {
continue;
}
self.push_indent();
self.buf.push_str(&enum_name);
self.buf.push_str("::");
Expand Down Expand Up @@ -1136,6 +1160,7 @@ struct EnumVariantMapping<'a> {
proto_name: &'a str,
proto_number: i32,
generated_variant_name: String,
alias_of: Option<String>,
}

fn build_enum_value_mappings<'a>(
Expand All @@ -1145,15 +1170,20 @@ fn build_enum_value_mappings<'a>(
) -> Vec<EnumVariantMapping<'a>> {
let mut numbers = HashSet::new();
let mut generated_names = HashMap::new();
let mut mappings = Vec::new();
let mut mappings: Vec<EnumVariantMapping<'a>> = Vec::new();

for (idx, value) in enum_values.iter().enumerate() {
// Skip duplicate enum values. Protobuf allows this when the
// Remember name for duplicate enum values. Protobuf allows this when the
// 'allow_alias' option is set.
let mut alias_of = None;
if !numbers.insert(value.number()) {
continue;
for m in &mappings {
if m.proto_number == value.number() {
Comment on lines 1179 to +1181
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

genereated_names can be used to do this lookup in one step, right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how, that would require a lookup by value instead of name, no?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. The HashMap is the other way around. Please disregard my comment.

alias_of = Some(m.generated_variant_name.clone());
caspermeijn marked this conversation as resolved.
Show resolved Hide resolved
break;
}
}
}

let mut generated_variant_name = to_upper_camel(value.name());
if do_strip_enum_prefix {
generated_variant_name =
Expand All @@ -1162,15 +1192,22 @@ fn build_enum_value_mappings<'a>(

if let Some(old_v) = generated_names.insert(generated_variant_name.to_owned(), value.name())
{
panic!("Generated enum variant names overlap: `{}` variant name to be used both by `{}` and `{}` ProtoBuf enum values",
generated_variant_name, old_v, value.name());
// if alias ends up being a duplicate, we don't need it, and can skip it.
// TODO: check if enum values are actually the same
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this TODO needs to be solved

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like duplicates should always be disallowed. Even if they are an alias, then overlap is not disirable.

if alias_of.is_some() {
continue;
} else {
panic!("Generated enum variant names overlap: `{}` variant name to be used both by `{}` and `{}` ProtoBuf enum values",
generated_variant_name, old_v, value.name());
}
}

mappings.push(EnumVariantMapping {
path_idx: idx,
proto_name: value.name(),
proto_number: value.number(),
generated_variant_name,
alias_of,
})
}
mappings
Expand Down
Loading