From 727e051bde6ea93d37cf7710f5b44fa58cf0f7ba Mon Sep 17 00:00:00 2001 From: Andrew Hickman Date: Sun, 8 Sep 2024 20:37:08 +0100 Subject: [PATCH 1/6] Fixed an issue with the name resolution changes in the previous release, causing some valid files to be rejected --- CHANGELOG.md | 5 ++ prost-reflect-tests/src/decode.rs | 2 - prost-reflect/src/descriptor/build/mod.rs | 79 +++++++++++-------- prost-reflect/src/descriptor/error.rs | 12 ++- prost-reflect/src/descriptor/mod.rs | 16 ++++ prost-reflect/src/descriptor/tests.rs | 11 ++- prost-reflect/tests/data/name_resolution5.yml | 34 ++++++++ prost-reflect/tests/data/name_resolution6.yml | 20 +++++ prost-reflect/tests/main.rs | 2 + .../snapshots/main__name_resolution4.snap | 5 +- .../snapshots/main__name_resolution5.snap | 38 +++++++++ .../snapshots/main__name_resolution6.snap | 24 ++++++ 12 files changed, 207 insertions(+), 41 deletions(-) create mode 100644 prost-reflect/tests/data/name_resolution5.yml create mode 100644 prost-reflect/tests/data/name_resolution6.yml create mode 100644 prost-reflect/tests/snapshots/main__name_resolution5.snap create mode 100644 prost-reflect/tests/snapshots/main__name_resolution6.snap diff --git a/CHANGELOG.md b/CHANGELOG.md index e9f9c1f2..f557d8cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Fixed an issue with the name resolution changes in the previous release, causing some valid files to be rejected (protox#86) + ## [0.14.1] - 2024-09-02 ### Fixed @@ -422,3 +426,4 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#41]: https://github.com/andrewhickman/prost-reflect/pull/41 [#99]: https://github.com/andrewhickman/prost-reflect/issues/99 [protox#82]: https://github.com/andrewhickman/protox/issues/82 +[protox#86]: https://github.com/andrewhickman/protox/issues/86 diff --git a/prost-reflect-tests/src/decode.rs b/prost-reflect-tests/src/decode.rs index 93e4dbe2..7d42a856 100644 --- a/prost-reflect-tests/src/decode.rs +++ b/prost-reflect-tests/src/decode.rs @@ -1,5 +1,3 @@ -#![cfg(test)] - use std::{ collections::{BTreeMap, HashMap}, fmt::Debug, diff --git a/prost-reflect/src/descriptor/build/mod.rs b/prost-reflect/src/descriptor/build/mod.rs index 19d5c54b..52ae4ebd 100644 --- a/prost-reflect/src/descriptor/build/mod.rs +++ b/prost-reflect/src/descriptor/build/mod.rs @@ -38,7 +38,7 @@ enum ResolveNameFilter { FieldType, } -enum ResolveNameResult<'a, 'b, 'c> { +enum ResolveNameResult<'a, 'b> { Found { name: Cow<'b, str>, def: &'a Definition, @@ -54,7 +54,7 @@ enum ResolveNameResult<'a, 'b, 'c> { }, Shadowed { name: Cow<'b, str>, - shadowed_name: Cow<'c, str>, + shadowed_name: Cow<'b, str>, }, NotFound, } @@ -172,7 +172,7 @@ impl fmt::Display for ResolveNameFilter { } } -impl<'a, 'b, 'c> ResolveNameResult<'a, 'b, 'c> { +impl<'a, 'b> ResolveNameResult<'a, 'b> { fn new( dependencies: &HashSet, names: &'a HashMap, Definition>, @@ -196,7 +196,7 @@ impl<'a, 'b, 'c> ResolveNameResult<'a, 'b, 'c> { } } - fn into_owned(self) -> ResolveNameResult<'a, 'static, 'static> { + fn into_owned(self) -> ResolveNameResult<'a, 'static> { match self { ResolveNameResult::Found { name, def } => ResolveNameResult::Found { name: Cow::Owned(name.into_owned()), @@ -287,10 +287,10 @@ impl<'a, 'b, 'c> ResolveNameResult<'a, 'b, 'c> { join_path(found_path1, found_path2), ), help: Some(format!( - "'{}' is is resolved to '{}', which is not defined. The innermost scope is searched first in name resolution. Consider using a leading '.'(i.e., '.{}') to start from the outermost scope.", - name, shadowed_name, name + "The innermost scope is searched first in name resolution. Consider using a leading '.'(i.e., '.{name}') to start from the outermost scope.", )), name: name.into_owned(), + shadowed_name: shadowed_name.into_owned(), }), } } @@ -314,13 +314,13 @@ fn to_json_name(name: &str) -> String { result } -fn resolve_name<'a, 'b, 'c>( +fn resolve_name<'a, 'b>( dependencies: &HashSet, names: &'a HashMap, Definition>, - scope: &'c str, + scope: &str, name: &'b str, filter: ResolveNameFilter, -) -> ResolveNameResult<'a, 'b, 'c> { +) -> ResolveNameResult<'a, 'b> { match name.strip_prefix('.') { Some(full_name) => ResolveNameResult::new(dependencies, names, full_name, filter), None if scope.is_empty() => ResolveNameResult::new(dependencies, names, name, filter), @@ -328,38 +328,53 @@ fn resolve_name<'a, 'b, 'c>( } } -fn resolve_relative_name<'a, 'b, 'c>( +fn resolve_relative_name<'a, 'b>( dependencies: &HashSet, names: &'a HashMap, Definition>, - scope: &'c str, + scope: &str, relative_name: &'b str, filter: ResolveNameFilter, -) -> ResolveNameResult<'a, 'b, 'c> { +) -> ResolveNameResult<'a, 'b> { let mut err = ResolveNameResult::NotFound; - let relative_first_part = relative_name.split('.').next(); - let mut last_candidate_last_part = None; + let relative_first_part = relative_name.split('.').next().unwrap_or_default(); + for candidate_parent in resolve_relative_candidate_parents(scope) { let candidate = match candidate_parent { - "" => Cow::Borrowed(relative_name), - _ => Cow::Owned(format!("{}.{}", candidate_parent, relative_name)), + "" => Cow::Borrowed(relative_first_part), + _ => Cow::Owned(format!("{}.{}", candidate_parent, relative_first_part)), }; - let res = ResolveNameResult::new(dependencies, names, candidate, filter); - if res.is_found() { - return res.into_owned(); - } else if matches!(err, ResolveNameResult::NotFound) { - err = res; - } - // Check if the name is shadowed by last parent scope, but this is not the last candidate - if Some(relative_first_part) == Some(last_candidate_last_part) - && !candidate_parent.is_empty() - { - let shadowed_name = format!("{}.{}", candidate_parent, relative_name); - return ResolveNameResult::Shadowed { - name: Cow::Borrowed(relative_name), - shadowed_name: Cow::Owned(shadowed_name), - }; + + if relative_first_part.len() == relative_name.len() { + // Looking up a simple name e.g. `Foo` + let res = ResolveNameResult::new(dependencies, names, candidate, filter); + if res.is_found() { + return res.into_owned(); + } else if matches!(err, ResolveNameResult::NotFound) { + err = res; + } + } else { + // Looking up a name including a namespace e.g. `foo.Foo`. First determine the scope using the first component of the name. + match names.get(candidate.as_ref()) { + Some(def) if def.kind.is_parent() => { + let candidate_full = match candidate_parent { + "" => Cow::Borrowed(relative_name), + _ => Cow::Owned(format!("{}.{}", candidate_parent, relative_name)), + }; + + let res = + ResolveNameResult::new(dependencies, names, candidate_full.clone(), filter); + if matches!(res, ResolveNameResult::NotFound) { + return ResolveNameResult::Shadowed { + name: Cow::Borrowed(relative_name), + shadowed_name: candidate_full, + }; + } else { + return res; + } + } + _ => continue, + } } - last_candidate_last_part = candidate_parent.rsplit('.').next(); } err.into_owned() diff --git a/prost-reflect/src/descriptor/error.rs b/prost-reflect/src/descriptor/error.rs index 5c2383cb..74efbc6d 100644 --- a/prost-reflect/src/descriptor/error.rs +++ b/prost-reflect/src/descriptor/error.rs @@ -85,6 +85,7 @@ pub(super) enum DescriptorErrorKind { }, NameShadowed { name: String, + shadowed_name: String, found: Label, #[cfg_attr(not(feature = "miette"), allow(dead_code))] help: Option, @@ -532,8 +533,15 @@ impl fmt::Display for DescriptorErrorKind { DescriptorErrorKind::NameNotFound { name, .. } => { write!(f, "name '{}' is not defined", name) } - DescriptorErrorKind::NameShadowed { name, .. } => { - write!(f, "name '{}' is shadowed", name) + DescriptorErrorKind::NameShadowed { + name, + shadowed_name, + .. + } => { + write!( + f, + "'{name}' is is resolved to '{shadowed_name}', which is not defined. ", + ) } DescriptorErrorKind::InvalidType { name, expected, .. } => { write!(f, "'{}' is not {}", name, expected) diff --git a/prost-reflect/src/descriptor/mod.rs b/prost-reflect/src/descriptor/mod.rs index 09479cfc..6018135f 100644 --- a/prost-reflect/src/descriptor/mod.rs +++ b/prost-reflect/src/descriptor/mod.rs @@ -393,6 +393,22 @@ impl fmt::Debug for KindIndex { } } +impl DefinitionKind { + fn is_parent(&self) -> bool { + match self { + DefinitionKind::Package => true, + DefinitionKind::Message(_) => true, + DefinitionKind::Field(_) => false, + DefinitionKind::Oneof(_) => false, + DefinitionKind::Service(_) => true, + DefinitionKind::Method(_) => false, + DefinitionKind::Enum(_) => true, + DefinitionKind::EnumValue(_) => false, + DefinitionKind::Extension(_) => false, + } + } +} + impl DescriptorPoolInner { fn get_by_name(&self, name: &str) -> Option<&Definition> { let name = name.strip_prefix('.').unwrap_or(name); diff --git a/prost-reflect/src/descriptor/tests.rs b/prost-reflect/src/descriptor/tests.rs index c2495844..391c4c8a 100644 --- a/prost-reflect/src/descriptor/tests.rs +++ b/prost-reflect/src/descriptor/tests.rs @@ -146,8 +146,15 @@ fn resolve_message_name_conflict_with_field_name() { }], }; - let e = DescriptorPool::from_file_descriptor_set(file_descriptor_set).unwrap_err(); - assert_eq!(e.to_string(), "name 'MyMessage' is shadowed"); + let descriptor_pool = DescriptorPool::from_file_descriptor_set(file_descriptor_set).unwrap(); + let message = descriptor_pool + .get_message_by_name("my.package.MyMessage") + .unwrap(); + let field = message.get_field_by_name("MyMessage").unwrap(); + assert_eq!( + field.kind().as_message().unwrap().full_name(), + "my.package.MyMessage" + ); } #[test] diff --git a/prost-reflect/tests/data/name_resolution5.yml b/prost-reflect/tests/data/name_resolution5.yml new file mode 100644 index 00000000..dd44226a --- /dev/null +++ b/prost-reflect/tests/data/name_resolution5.yml @@ -0,0 +1,34 @@ +file: +- name: root.proto + messageType: + - name: Parent + field: + - name: ChildMessage + number: 1 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + typeName: ChildMessage + jsonName: ChildMessage + - name: ChildEnum + number: 2 + label: LABEL_OPTIONAL + type: TYPE_ENUM + typeName: ChildEnum + jsonName: ChildEnum + - name: ChildMessage + field: + - name: field + number: 1 + label: LABEL_OPTIONAL + type: TYPE_STRING + jsonName: field + enumType: + - name: ChildEnum + value: + - name: UNKNOWN + number: 0 + - name: A + number: 1 + - name: B + number: 2 + syntax: proto3 diff --git a/prost-reflect/tests/data/name_resolution6.yml b/prost-reflect/tests/data/name_resolution6.yml new file mode 100644 index 00000000..a84cd507 --- /dev/null +++ b/prost-reflect/tests/data/name_resolution6.yml @@ -0,0 +1,20 @@ +file: +- name: dep.proto + package: foo + messageType: + - name: Foo + syntax: proto3 +- name: root.proto + package: sample + dependency: + - dep.proto + messageType: + - name: Sample + field: + - name: foo + number: 2 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + typeName: foo.Foo + jsonName: foo + syntax: proto3 diff --git a/prost-reflect/tests/main.rs b/prost-reflect/tests/main.rs index 15fec9da..e3057ee5 100644 --- a/prost-reflect/tests/main.rs +++ b/prost-reflect/tests/main.rs @@ -133,6 +133,8 @@ check_err!(name_resolution1); check_err!(name_resolution2); check_ok!(name_resolution3); check_err!(name_resolution4); +check_ok!(name_resolution5); +check_ok!(name_resolution6); check_err!(name_collision1); check_err!(name_collision2); check_err!(name_collision3); diff --git a/prost-reflect/tests/snapshots/main__name_resolution4.snap b/prost-reflect/tests/snapshots/main__name_resolution4.snap index 6e840f05..4643a4c9 100644 --- a/prost-reflect/tests/snapshots/main__name_resolution4.snap +++ b/prost-reflect/tests/snapshots/main__name_resolution4.snap @@ -1,11 +1,10 @@ --- source: prost-reflect/tests/main.rs -assertion_line: 79 expression: actual --- causes: [] -help: "'foo.FooBar' is is resolved to 'com.foo.FooBar', which is not defined. The innermost scope is searched first in name resolution. Consider using a leading '.'(i.e., '.foo.FooBar') to start from the outermost scope." +help: "The innermost scope is searched first in name resolution. Consider using a leading '.'(i.e., '.foo.FooBar') to start from the outermost scope." labels: [] -message: "name 'foo.FooBar' is shadowed" +message: "'foo.FooBar' is is resolved to 'com.foo.FooBar', which is not defined. " related: [] severity: error diff --git a/prost-reflect/tests/snapshots/main__name_resolution5.snap b/prost-reflect/tests/snapshots/main__name_resolution5.snap new file mode 100644 index 00000000..7ee3579f --- /dev/null +++ b/prost-reflect/tests/snapshots/main__name_resolution5.snap @@ -0,0 +1,38 @@ +--- +source: prost-reflect/tests/main.rs +expression: actual +--- +file: + - enumType: + - name: ChildEnum + value: + - name: UNKNOWN + number: 0 + - name: A + number: 1 + - name: B + number: 2 + messageType: + - field: + - jsonName: ChildMessage + label: LABEL_OPTIONAL + name: ChildMessage + number: 1 + type: TYPE_MESSAGE + typeName: ".ChildMessage" + - jsonName: ChildEnum + label: LABEL_OPTIONAL + name: ChildEnum + number: 2 + type: TYPE_ENUM + typeName: ".ChildEnum" + name: Parent + - field: + - jsonName: field + label: LABEL_OPTIONAL + name: field + number: 1 + type: TYPE_STRING + name: ChildMessage + name: root.proto + syntax: proto3 diff --git a/prost-reflect/tests/snapshots/main__name_resolution6.snap b/prost-reflect/tests/snapshots/main__name_resolution6.snap new file mode 100644 index 00000000..3b4413b9 --- /dev/null +++ b/prost-reflect/tests/snapshots/main__name_resolution6.snap @@ -0,0 +1,24 @@ +--- +source: prost-reflect/tests/main.rs +expression: actual +--- +file: + - messageType: + - name: Foo + name: dep.proto + package: foo + syntax: proto3 + - dependency: + - dep.proto + messageType: + - field: + - jsonName: foo + label: LABEL_OPTIONAL + name: foo + number: 2 + type: TYPE_MESSAGE + typeName: ".foo.Foo" + name: Sample + name: root.proto + package: sample + syntax: proto3 From c093c29bb20f9d4b68309b4d6a1aef37572507fe Mon Sep 17 00:00:00 2001 From: Andrew Hickman Date: Sun, 8 Sep 2024 20:41:06 +0100 Subject: [PATCH 2/6] Error formatting --- prost-reflect/src/descriptor/error.rs | 2 +- prost-reflect/tests/snapshots/main__name_resolution4.snap | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/prost-reflect/src/descriptor/error.rs b/prost-reflect/src/descriptor/error.rs index 74efbc6d..c92ff28d 100644 --- a/prost-reflect/src/descriptor/error.rs +++ b/prost-reflect/src/descriptor/error.rs @@ -540,7 +540,7 @@ impl fmt::Display for DescriptorErrorKind { } => { write!( f, - "'{name}' is is resolved to '{shadowed_name}', which is not defined. ", + "'{name}' is is resolved to '{shadowed_name}', which is not defined", ) } DescriptorErrorKind::InvalidType { name, expected, .. } => { diff --git a/prost-reflect/tests/snapshots/main__name_resolution4.snap b/prost-reflect/tests/snapshots/main__name_resolution4.snap index 4643a4c9..5f73e74e 100644 --- a/prost-reflect/tests/snapshots/main__name_resolution4.snap +++ b/prost-reflect/tests/snapshots/main__name_resolution4.snap @@ -5,6 +5,6 @@ expression: actual causes: [] help: "The innermost scope is searched first in name resolution. Consider using a leading '.'(i.e., '.foo.FooBar') to start from the outermost scope." labels: [] -message: "'foo.FooBar' is is resolved to 'com.foo.FooBar', which is not defined. " +message: "'foo.FooBar' is is resolved to 'com.foo.FooBar', which is not defined" related: [] severity: error From 0aac0bda94f47b132523148382bbc9be1f874c82 Mon Sep 17 00:00:00 2001 From: Andrew Hickman Date: Sun, 8 Sep 2024 20:42:52 +0100 Subject: [PATCH 3/6] Nicer grammar --- prost-reflect/src/descriptor/error.rs | 2 +- prost-reflect/tests/snapshots/main__name_resolution4.snap | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/prost-reflect/src/descriptor/error.rs b/prost-reflect/src/descriptor/error.rs index c92ff28d..56b6aaee 100644 --- a/prost-reflect/src/descriptor/error.rs +++ b/prost-reflect/src/descriptor/error.rs @@ -540,7 +540,7 @@ impl fmt::Display for DescriptorErrorKind { } => { write!( f, - "'{name}' is is resolved to '{shadowed_name}', which is not defined", + "'{name}' resolves to '{shadowed_name}', which is not defined", ) } DescriptorErrorKind::InvalidType { name, expected, .. } => { diff --git a/prost-reflect/tests/snapshots/main__name_resolution4.snap b/prost-reflect/tests/snapshots/main__name_resolution4.snap index 5f73e74e..da78ab3b 100644 --- a/prost-reflect/tests/snapshots/main__name_resolution4.snap +++ b/prost-reflect/tests/snapshots/main__name_resolution4.snap @@ -5,6 +5,6 @@ expression: actual causes: [] help: "The innermost scope is searched first in name resolution. Consider using a leading '.'(i.e., '.foo.FooBar') to start from the outermost scope." labels: [] -message: "'foo.FooBar' is is resolved to 'com.foo.FooBar', which is not defined" +message: "'foo.FooBar' resolves to 'com.foo.FooBar', which is not defined" related: [] severity: error From d90ba20df39ace2db7bce3a1897e2304e874cd1d Mon Sep 17 00:00:00 2001 From: Andrew Hickman Date: Sun, 8 Sep 2024 20:45:30 +0100 Subject: [PATCH 4/6] Typo --- prost-reflect/src/descriptor/build/mod.rs | 2 +- prost-reflect/tests/snapshots/main__name_resolution4.snap | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/prost-reflect/src/descriptor/build/mod.rs b/prost-reflect/src/descriptor/build/mod.rs index 52ae4ebd..844c0a25 100644 --- a/prost-reflect/src/descriptor/build/mod.rs +++ b/prost-reflect/src/descriptor/build/mod.rs @@ -287,7 +287,7 @@ impl<'a, 'b> ResolveNameResult<'a, 'b> { join_path(found_path1, found_path2), ), help: Some(format!( - "The innermost scope is searched first in name resolution. Consider using a leading '.'(i.e., '.{name}') to start from the outermost scope.", + "The innermost scope is searched first in name resolution. Consider using a leading '.' (i.e., '.{name}') to start from the outermost scope.", )), name: name.into_owned(), shadowed_name: shadowed_name.into_owned(), diff --git a/prost-reflect/tests/snapshots/main__name_resolution4.snap b/prost-reflect/tests/snapshots/main__name_resolution4.snap index da78ab3b..954abb89 100644 --- a/prost-reflect/tests/snapshots/main__name_resolution4.snap +++ b/prost-reflect/tests/snapshots/main__name_resolution4.snap @@ -3,7 +3,7 @@ source: prost-reflect/tests/main.rs expression: actual --- causes: [] -help: "The innermost scope is searched first in name resolution. Consider using a leading '.'(i.e., '.foo.FooBar') to start from the outermost scope." +help: "The innermost scope is searched first in name resolution. Consider using a leading '.' (i.e., '.foo.FooBar') to start from the outermost scope." labels: [] message: "'foo.FooBar' resolves to 'com.foo.FooBar', which is not defined" related: [] From 5e82ba6ba5fbfc4898a32a2956060ce71e1ceca9 Mon Sep 17 00:00:00 2001 From: Andrew Hickman Date: Sun, 8 Sep 2024 20:48:09 +0100 Subject: [PATCH 5/6] Lints --- prost-reflect-build/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prost-reflect-build/src/lib.rs b/prost-reflect-build/src/lib.rs index fda83428..0345fcdd 100644 --- a/prost-reflect-build/src/lib.rs +++ b/prost-reflect-build/src/lib.rs @@ -173,7 +173,7 @@ impl Builder { .type_attribute(full_name, "#[derive(::prost_reflect::ReflectMessage)]") .type_attribute( full_name, - &format!(r#"#[prost_reflect(message_name = "{}")]"#, full_name,), + format!(r#"#[prost_reflect(message_name = "{}")]"#, full_name,), ) .type_attribute(full_name, &pool_attribute); } From 94707a3f732a18bb67cc04574e486de182d73ea1 Mon Sep 17 00:00:00 2001 From: Andrew Hickman Date: Sun, 8 Sep 2024 20:50:29 +0100 Subject: [PATCH 6/6] Lints --- prost-reflect-conformance-tests/build.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prost-reflect-conformance-tests/build.rs b/prost-reflect-conformance-tests/build.rs index b5270a84..7225eac2 100644 --- a/prost-reflect-conformance-tests/build.rs +++ b/prost-reflect-conformance-tests/build.rs @@ -115,7 +115,7 @@ fn install_conformance_test_runner(src_dir: &Path, prefix_dir: &Path) -> Result< .arg("-GNinja") .arg("cmake/") .arg("-DCMAKE_BUILD_TYPE=DEBUG") - .arg(&format!("-DCMAKE_INSTALL_PREFIX={}", prefix_dir.display())) + .arg(format!("-DCMAKE_INSTALL_PREFIX={}", prefix_dir.display())) .arg("-Dprotobuf_BUILD_CONFORMANCE=ON") .arg("-Dprotobuf_BUILD_TESTS=OFF") .current_dir(src_dir)