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

correctness fixes to get Envoy xDS protos compiling #30

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 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
9 changes: 9 additions & 0 deletions pbjson-build/src/escape.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,12 @@ pub fn escape_ident(mut ident: String) -> String {
};
ident
}

/// Converts a `snake_case` identifier to an `UpperCamel` case Rust type identifier.
pub fn escape_camel_case(mut ident: String) -> String {
bpowers marked this conversation as resolved.
Show resolved Hide resolved
// Suffix an underscore for the `Self` Rust keyword as it is not allowed as raw identifier.
if ident == "Self" {
ident += "_";
}
ident
}
11 changes: 10 additions & 1 deletion pbjson-build/src/generator/enumeration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,16 @@
//! An enumeration should be decode-able from the full string variant name
//! or its integer tag number, and should encode to the string representation

use std::collections::HashSet;
use std::io::{Result, Write};

use super::{
write_deserialize_end, write_deserialize_start, write_serialize_end, write_serialize_start,
Indent,
};
use crate::descriptor::{EnumDescriptor, TypePath};
use crate::generator::write_fields_array;
use crate::resolver::Resolver;
use std::io::{Result, Write};

pub fn generate_enum<W: Write>(
resolver: &Resolver<'_>,
Expand All @@ -21,9 +23,16 @@ pub fn generate_enum<W: Write>(
) -> Result<()> {
let rust_type = resolver.rust_type(path);

let mut numbers = HashSet::new();

let variants: Vec<_> = descriptor
.values
.iter()
.filter(|variant| {
// Skip duplicate enum values. Protobuf allows this when the
// 'allow_alias' option is set.
numbers.insert(variant.number())
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we could get a test of this added to pbjson-test?

})
.map(|variant| {
let variant_name = variant.name.clone().unwrap();
let rust_variant = resolver.rust_variant(path, &variant_name);
Expand Down
30 changes: 18 additions & 12 deletions pbjson-build/src/generator/message.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ use super::{
Indent,
};
use crate::descriptor::TypePath;
use crate::escape::escape_camel_case;
use crate::generator::write_fields_array;
use crate::resolver::Resolver;

Expand Down Expand Up @@ -440,7 +441,7 @@ fn write_deserialize_message<W: Write>(
{indent} formatter.write_str("struct {name}")
{indent} }}

{indent} fn visit_map<V>(self, mut map: V) -> Result<{rust_type}, V::Error>
{indent} fn visit_map<V>(self, mut pbjson_map: V) -> Result<{rust_type}, V::Error>
{indent} where
{indent} V: serde::de::MapAccess<'de>,
{indent} {{"#,
Expand Down Expand Up @@ -470,7 +471,7 @@ fn write_deserialize_message<W: Write>(
if !message.fields.is_empty() || !message.one_ofs.is_empty() {
writeln!(
writer,
"{}while let Some(k) = map.next_key()? {{",
"{}while let Some(k) = pbjson_map.next_key()? {{",
Indent(indent + 2)
)?;

Expand All @@ -491,7 +492,7 @@ fn write_deserialize_message<W: Write>(
} else {
writeln!(
writer,
"{}while map.next_key::<GeneratedField>()?.is_some() {{}}",
"{}while pbjson_map.next_key::<GeneratedField>()?.is_some() {{}}",
Indent(indent + 2)
)?;
}
Expand Down Expand Up @@ -591,7 +592,7 @@ fn write_deserialize_field_name<W: Write>(
"{}\"{}\" => Ok(GeneratedField::{}),",
Indent(indent + 5),
json_name,
type_name
escape_camel_case(type_name.to_string()),
)?;
}
writeln!(
Expand Down Expand Up @@ -631,7 +632,12 @@ fn write_fields_enum<'a, W: Write, I: Iterator<Item = &'a str>>(
)?;
writeln!(writer, "{}enum GeneratedField {{", Indent(indent))?;
for type_name in fields {
writeln!(writer, "{}{},", Indent(indent + 1), type_name)?;
writeln!(
writer,
"{}{},",
Indent(indent + 1),
escape_camel_case(type_name.to_string())
)?;
}
writeln!(writer, "{}}}", Indent(indent))
}
Expand Down Expand Up @@ -689,14 +695,14 @@ fn write_deserialize_field<W: Write>(
FieldModifier::Repeated => {
write!(
writer,
"map.next_value::<Vec<{}>>()?.into_iter().map(|x| x as i32).collect()",
"pbjson_map.next_value::<Vec<{}>>()?.into_iter().map(|x| x as i32).collect()",
resolver.rust_type(path)
)?;
}
_ => {
write!(
writer,
"map.next_value::<{}>()? as i32",
"pbjson_map.next_value::<{}>()? as i32",
resolver.rust_type(path)
)?;
}
Expand All @@ -705,7 +711,7 @@ fn write_deserialize_field<W: Write>(
writeln!(writer)?;
write!(
writer,
"{}map.next_value::<std::collections::HashMap<",
"{}pbjson_map.next_value::<std::collections::HashMap<",
Indent(indent + 2),
)?;

Expand Down Expand Up @@ -764,7 +770,7 @@ fn write_deserialize_field<W: Write>(
write!(writer, "{}", Indent(indent + 1))?;
}
_ => {
write!(writer, "map.next_value()?",)?;
write!(writer, "pbjson_map.next_value()?",)?;
}
};

Expand All @@ -785,7 +791,7 @@ fn write_encode_scalar_field<W: Write>(
let deserializer = match scalar {
ScalarType::Bytes => "BytesDeserialize",
_ if scalar.is_numeric() => "NumberDeserialize",
_ => return write!(writer, "map.next_value()?",),
_ => return write!(writer, "pbjson_map.next_value()?",),
};

writeln!(writer)?;
Expand All @@ -794,7 +800,7 @@ fn write_encode_scalar_field<W: Write>(
FieldModifier::Repeated => {
writeln!(
writer,
"{}map.next_value::<Vec<::pbjson::private::{}<_>>>()?",
"{}pbjson_map.next_value::<Vec<::pbjson::private::{}<_>>>()?",
Indent(indent + 1),
deserializer
)?;
Expand All @@ -807,7 +813,7 @@ fn write_encode_scalar_field<W: Write>(
_ => {
writeln!(
writer,
"{}map.next_value::<::pbjson::private::{}<_>>()?.0",
"{}pbjson_map.next_value::<::pbjson::private::{}<_>>()?.0",
Indent(indent + 1),
deserializer
)?;
Expand Down
4 changes: 2 additions & 2 deletions pbjson-build/src/message.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use prost_types::{
};

use crate::descriptor::{Descriptor, DescriptorSet, MessageDescriptor, Syntax, TypeName, TypePath};
use crate::escape::escape_ident;
use crate::escape::{escape_camel_case, escape_ident};

#[derive(Debug, Clone, Copy)]
pub enum ScalarType {
Expand Down Expand Up @@ -86,7 +86,7 @@ pub struct Field {
impl Field {
pub fn rust_type_name(&self) -> String {
use heck::CamelCase;
self.name.to_camel_case()
escape_camel_case(self.name.to_camel_case())
}

pub fn rust_field_name(&self) -> String {
Expand Down
21 changes: 18 additions & 3 deletions pbjson-build/src/resolver.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use crate::descriptor::{Package, TypePath};
use crate::escape::{escape_camel_case, escape_ident};

#[derive(Debug)]
pub struct Resolver<'a> {
Expand Down Expand Up @@ -57,7 +58,7 @@ impl<'a> Resolver<'a> {
.path()
.iter()
.zip(path.path())
.filter(|(a, b)| a == b)
.take_while(|(a, b)| a == b)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 👌

.count();

let super_count = self.package.path().len() - shared_prefix;
Expand All @@ -75,11 +76,11 @@ impl<'a> Resolver<'a> {
while let Some(i) = iter.next() {
match iter.peek() {
Some(_) => {
ret.push_str(i.to_snake_case().as_str());
ret.push_str(escape_ident(i.to_snake_case()).as_str());
ret.push_str("::");
}
None => {
ret.push_str(i.to_camel_case().as_str());
ret.push_str(escape_camel_case(i.to_camel_case()).as_str());
}
}
}
Expand Down Expand Up @@ -110,6 +111,20 @@ mod tests {
use super::*;
use crate::descriptor::TypeName;

#[test]
fn test_complicated_resolver() {
let resolver_package = Package::new("envoy.service.health.v3");
let resolver = Resolver::new(&[], &resolver_package, false);

let to_resolve = TypePath::new(Package::new("envoy.config.core.v3"))
.child(TypeName::new("HealthStatus"));

assert_eq!(
"super::super::super::config::core::v3::HealthStatus",
resolver.rust_type(&to_resolve)
);
}

#[test]
fn test_resolver() {
let resolver_package = Package::new("test.syntax3");
Expand Down