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.

Sign up for GitHub

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

Jump to bottom

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

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

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0288841
generate associated constants in enums representing protobuf enum ali…
ousado Apr 23, 2023
09f2e99
rustfmt; Handle collisions of generated names between aliases and exi…
ousado Apr 23, 2023
4fe05fc
accomodate funny (comment aware) test
ousado Apr 23, 2023
0378107
always refer to first variant with a given value in case of multiple …
ousado Apr 24, 2023
93d039b
fix comment, move loop into if block
ousado Apr 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
50 changes: 43 additions & 7 deletions prost-build/src/code_generator.rs
View file
Open in desktop
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,22 @@ 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");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Three slashes is for rustdoc comments. This is not a doc comment, right?

Copy link
Author

Choose a reason for hiding this comment

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

right

}
for variant in &aliases {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Personally, I would place the for loop inside the if-block

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 +733,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 +1159,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 +1169,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 +1191,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