Fix an issue with the name resolution changes in the previous release (…

…#127) * Fixed an issue with the name resolution changes in the previous release, causing some valid files to be rejected
andrewhickman · Sep 8, 2024 · f90733b · f90733b
1 parent 4c8c56b
commit f90733b
Show file tree

Hide file tree

Showing 14 changed files with 209 additions and 43 deletions.
diff --git 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-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);
         }

diff --git 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)

diff --git 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
@@ -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<FileIndex>,
         names: &'a HashMap<Box<str>, 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,52 +314,67 @@ fn to_json_name(name: &str) -> String {
     result
 }
 
-fn resolve_name<'a, 'b, 'c>(
+fn resolve_name<'a, 'b>(
     dependencies: &HashSet<FileIndex>,
     names: &'a HashMap<Box<str>, 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),
         None => resolve_relative_name(dependencies, names, scope, name, filter),
     }
 }
 
-fn resolve_relative_name<'a, 'b, 'c>(
+fn resolve_relative_name<'a, 'b>(
     dependencies: &HashSet<FileIndex>,
     names: &'a HashMap<Box<str>, 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
@@ -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<String>,
@@ -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}' resolves 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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' resolves 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
@@ -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
@@ -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