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

chore(prost_build): introduce a typeset FullyQualifiedName #1191

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

chore(prost_build): introduce a typeset FullyQualifiedName #1191

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e8f7997
chore(prost-build): simplify `ExternPaths` usage with parameter generics
gibbz00 Nov 23, 2024
4739ea6
chore(prost_build): introduce a typeset FullyQualifiedName
gibbz00 Nov 23, 2024
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: 7 additions & 2 deletions prost-build/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1079,8 +1079,13 @@ impl Config {
let mut packages = HashMap::new();

let message_graph = MessageGraph::new(requests.iter().map(|x| &x.1), self.boxed.clone());
let extern_paths = ExternPaths::new(&self.extern_paths, self.prost_types)
.map_err(|error| Error::new(ErrorKind::InvalidInput, error))?;
let extern_paths = ExternPaths::new(
self.extern_paths
.iter()
.map(|(a, b)| (a.as_str(), b.as_str())),
self.prost_types,
Comment on lines +1083 to +1086
Copy link
Collaborator

Choose a reason for hiding this comment

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

To be honest, if the goal is to simplify, then that failed here. The previous version was simpler.

I think Config::extern_paths should change type to better match the internals of ExternPaths. If extern_paths is a HashMap, then it can be passed to ExternPaths::new and used without doing a manual copy.

)
.map_err(|error| Error::new(ErrorKind::InvalidInput, error))?;

for (request_module, request_fd) in requests {
// Only record packages that have services
Expand Down
80 changes: 39 additions & 41 deletions prost-build/src/extern_paths.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,48 +23,49 @@ pub struct ExternPaths {
}

impl ExternPaths {
pub fn new(paths: &[(String, String)], prost_types: bool) -> Result<ExternPaths, String> {
pub fn new<'a>(
paths: impl IntoIterator<Item = (&'a str, &'a str)> + 'a,
prost_types: bool,
) -> Result<ExternPaths, String> {
let mut extern_paths = ExternPaths {
extern_paths: HashMap::new(),
};

for (proto_path, rust_path) in paths {
extern_paths.insert(proto_path.clone(), rust_path.clone())?;
extern_paths.insert(proto_path, rust_path)?;
}

if prost_types {
extern_paths.insert(".google.protobuf".to_string(), "::prost_types".to_string())?;
extern_paths.insert(".google.protobuf.BoolValue".to_string(), "bool".to_string())?;
extern_paths.insert(".google.protobuf", "::prost_types")?;
extern_paths.insert(".google.protobuf.BoolValue", "bool")?;
extern_paths.insert(
".google.protobuf.BytesValue".to_string(),
"::prost::alloc::vec::Vec<u8>".to_string(),
".google.protobuf.BytesValue",
"::prost::alloc::vec::Vec<u8>",
)?;
extern_paths.insert(".google.protobuf.DoubleValue", "f64")?;
extern_paths.insert(".google.protobuf.Empty", "()")?;
extern_paths.insert(".google.protobuf.FloatValue", "f32")?;
extern_paths.insert(".google.protobuf.Int32Value", "i32")?;
extern_paths.insert(".google.protobuf.Int64Value", "i64")?;
extern_paths.insert(
".google.protobuf.DoubleValue".to_string(),
"f64".to_string(),
)?;
extern_paths.insert(".google.protobuf.Empty".to_string(), "()".to_string())?;
extern_paths.insert(".google.protobuf.FloatValue".to_string(), "f32".to_string())?;
extern_paths.insert(".google.protobuf.Int32Value".to_string(), "i32".to_string())?;
extern_paths.insert(".google.protobuf.Int64Value".to_string(), "i64".to_string())?;
extern_paths.insert(
".google.protobuf.StringValue".to_string(),
"::prost::alloc::string::String".to_string(),
)?;
extern_paths.insert(
".google.protobuf.UInt32Value".to_string(),
"u32".to_string(),
)?;
extern_paths.insert(
".google.protobuf.UInt64Value".to_string(),
"u64".to_string(),
".google.protobuf.StringValue",
"::prost::alloc::string::String",
)?;
extern_paths.insert(".google.protobuf.UInt32Value", "u32")?;
extern_paths.insert(".google.protobuf.UInt64Value", "u64")?;
}

Ok(extern_paths)
}

fn insert(&mut self, proto_path: String, rust_path: String) -> Result<(), String> {
fn insert(
&mut self,
proto_path: impl Into<String>,
rust_path: impl Into<String>,
Comment on lines +67 to +68
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, this is a great improvement.

) -> Result<(), String> {
let proto_path = proto_path.into();
let rust_path = rust_path.into();

validate_proto_path(&proto_path)?;
match self.extern_paths.entry(proto_path) {
hash_map::Entry::Occupied(occupied) => {
Expand Down Expand Up @@ -124,12 +125,12 @@ mod tests {
#[test]
fn test_extern_paths() {
let paths = ExternPaths::new(
&[
(".foo".to_string(), "::foo1".to_string()),
(".foo.bar".to_string(), "::foo2".to_string()),
(".foo.baz".to_string(), "::foo3".to_string()),
(".foo.Fuzz".to_string(), "::foo4::Fuzz".to_string()),
(".a.b.c.d.e.f".to_string(), "::abc::def".to_string()),
[
(".foo", "::foo1"),
(".foo.bar", "::foo2"),
(".foo.baz", "::foo3"),
(".foo.Fuzz", "::foo4::Fuzz"),
(".a.b.c.d.e.f", "::abc::def"),
],
false,
)
Expand Down Expand Up @@ -157,7 +158,7 @@ mod tests {

#[test]
fn test_well_known_types() {
let paths = ExternPaths::new(&[], true).unwrap();
let paths = ExternPaths::new([], true).unwrap();

let case = |proto_ident: &str, resolved_ident: &str| {
assert_eq!(paths.resolve_ident(proto_ident).unwrap(), resolved_ident);
Expand All @@ -170,8 +171,8 @@ mod tests {

#[test]
fn test_error_fully_qualified() {
let paths = [("foo".to_string(), "bar".to_string())];
let err = ExternPaths::new(&paths, false).unwrap_err();
let paths = [("foo", "bar")];
let err = ExternPaths::new(paths, false).unwrap_err();
assert_eq!(
err.to_string(),
"Protobuf paths must be fully qualified (begin with a leading '.'): foo"
Expand All @@ -180,8 +181,8 @@ mod tests {

#[test]
fn test_error_invalid_path() {
let paths = [(".foo.".to_string(), "bar".to_string())];
let err = ExternPaths::new(&paths, false).unwrap_err();
let paths = [(".foo.", "bar")];
let err = ExternPaths::new(paths, false).unwrap_err();
assert_eq!(
err.to_string(),
"invalid fully-qualified Protobuf path: .foo."
Expand All @@ -190,11 +191,8 @@ mod tests {

#[test]
fn test_error_duplicate() {
let paths = [
(".foo".to_string(), "bar".to_string()),
(".foo".to_string(), "bar".to_string()),
];
let err = ExternPaths::new(&paths, false).unwrap_err();
let paths = [(".foo", "bar"), (".foo", "bar")];
let err = ExternPaths::new(paths, false).unwrap_err();
assert_eq!(err.to_string(), "duplicate extern Protobuf path: .foo")
}
}