check for namespace shadowing (#126)

* check for namespace shadowing

protoc enforces that the name space resolution stops when it found a relative name
which starts with a package fragment contained by the current package

e.g:
if package is com.foo
The type foo.Bar relative name resolution will never reach root path, it will stop searching at com.foo.Bar

* add test for name resolution shadowing

* resolve_message_name_conflict_with_field_name is now a Shadow case

we change the test as this is not a proper case

prost-reflect/src/descriptor/build/mod.rs

@@ -38,7 +38,7 @@ enum ResolveNameFilter {
     FieldType,
 }
 
-enum ResolveNameResult<'a, 'b> {
+enum ResolveNameResult<'a, 'b, 'c> {
     Found {
         name: Cow<'b, str>,
         def: &'a Definition,
@@ -52,6 +52,10 @@ enum ResolveNameResult<'a, 'b> {
         name: Cow<'b, str>,
         file: FileIndex,
     },
+    Shadowed {
+        name: Cow<'b, str>,
+        shadowed_name: Cow<'c, str>,
+    },
     NotFound,
 }
 
@@ -168,7 +172,7 @@ impl fmt::Display for ResolveNameFilter {
     }
 }
 
-impl<'a, 'b> ResolveNameResult<'a, 'b> {
+impl<'a, 'b, 'c> ResolveNameResult<'a, 'b, 'c> {
     fn new(
         dependencies: &HashSet<FileIndex>,
         names: &'a HashMap<Box<str>, Definition>,
@@ -192,7 +196,7 @@ impl<'a, 'b> ResolveNameResult<'a, 'b> {
         }
     }
 
-    fn into_owned(self) -> ResolveNameResult<'a, 'static> {
+    fn into_owned(self) -> ResolveNameResult<'a, 'static, 'static> {
         match self {
             ResolveNameResult::Found { name, def } => ResolveNameResult::Found {
                 name: Cow::Owned(name.into_owned()),
@@ -209,6 +213,13 @@ impl<'a, 'b> ResolveNameResult<'a, 'b> {
                 name: Cow::Owned(name.into_owned()),
                 file,
             },
+            ResolveNameResult::Shadowed {
+                name,
+                shadowed_name,
+            } => ResolveNameResult::Shadowed {
+                name: Cow::Owned(name.into_owned()),
+                shadowed_name: Cow::Owned(shadowed_name.into_owned()),
+            },
             ResolveNameResult::NotFound => ResolveNameResult::NotFound,
         }
     }
@@ -268,6 +279,19 @@ impl<'a, 'b> ResolveNameResult<'a, 'b> {
                 ),
                 help: None,
             }),
+            ResolveNameResult::Shadowed { name, shadowed_name } => Err(DescriptorErrorKind::NameShadowed {
+                found: Label::new(
+                    files,
+                    "found here",
+                    found_file,
+                    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
+                )),
+                name: name.into_owned(),
+            }),
         }
     }
 }
@@ -290,52 +314,61 @@ fn to_json_name(name: &str) -> String {
     result
 }
 
-fn resolve_name<'a, 'b>(
+fn resolve_name<'a, 'b, 'c>(
     dependencies: &HashSet<FileIndex>,
     names: &'a HashMap<Box<str>, Definition>,
-    scope: &str,
+    scope: &'c str,
     name: &'b str,
     filter: ResolveNameFilter,
-) -> ResolveNameResult<'a, 'b> {
+) -> ResolveNameResult<'a, 'b, 'c> {
     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>(
+fn resolve_relative_name<'a, 'b, 'c>(
     dependencies: &HashSet<FileIndex>,
     names: &'a HashMap<Box<str>, Definition>,
-    scope: &str,
-    relative_name: &str,
+    scope: &'c str,
+    relative_name: &'b str,
     filter: ResolveNameFilter,
-) -> ResolveNameResult<'a, 'static> {
+) -> ResolveNameResult<'a, 'b, 'c> {
     let mut err = ResolveNameResult::NotFound;
-
-    for candidate in resolve_relative_name_candidates(scope, relative_name) {
+    let relative_first_part = relative_name.split('.').next();
+    let mut last_candidate_last_part = None;
+    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)),
+        };
         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),
+            };
+        }
+        last_candidate_last_part = candidate_parent.rsplit('.').next();
     }
 
     err.into_owned()
 }
 
-fn resolve_relative_name_candidates<'b: 'c, 'c>(
-    scope: &'c str,
-    relative_name: &'b str,
-) -> impl Iterator<Item = Cow<'b, str>> + 'c {
-    iter::once(Cow::Owned(format!("{scope}.{relative_name}")))
-        .chain(
-            scope
-                .rmatch_indices('.')
-                .map(move |(i, _)| Cow::Owned(format!("{}.{relative_name}", &scope[..i]))),
-        )
-        .chain(iter::once(Cow::Borrowed(relative_name)))
+fn resolve_relative_candidate_parents(scope: &str) -> impl Iterator<Item = &str> {
+    iter::once(scope)
+        .chain(scope.rmatch_indices('.').map(move |(i, _)| &scope[..i]))
+        .chain(iter::once(""))
 }
 
 fn join_path(path1: &[i32], path2: &[i32]) -> Box<[i32]> {

prost-reflect/src/descriptor/error.rs

@@ -83,6 +83,12 @@ pub(super) enum DescriptorErrorKind {
         #[cfg_attr(not(feature = "miette"), allow(dead_code))]
         help: Option<String>,
     },
+    NameShadowed {
+        name: String,
+        found: Label,
+        #[cfg_attr(not(feature = "miette"), allow(dead_code))]
+        help: Option<String>,
+    },
     InvalidType {
         name: String,
         expected: String,
@@ -316,6 +322,7 @@ impl DescriptorErrorKind {
             DescriptorErrorKind::FieldNumberInExtensionRange { found, .. } => Some(found),
             DescriptorErrorKind::ExtensionNumberOutOfRange { found, .. } => Some(found),
             DescriptorErrorKind::NameNotFound { found, .. } => Some(found),
+            DescriptorErrorKind::NameShadowed { found, .. } => Some(found),
             DescriptorErrorKind::InvalidType { found, .. } => Some(found),
             DescriptorErrorKind::InvalidFieldDefault { found, .. } => Some(found),
             DescriptorErrorKind::EmptyEnum { found } => Some(found),
@@ -380,6 +387,9 @@ impl DescriptorErrorKind {
             DescriptorErrorKind::NameNotFound { found, .. } => {
                 found.resolve_span(file, source);
             }
+            DescriptorErrorKind::NameShadowed { found, .. } => {
+                found.resolve_span(file, source);
+            }
             DescriptorErrorKind::InvalidType { found, defined, .. } => {
                 found.resolve_span(file, source);
                 defined.resolve_span(file, source);
@@ -522,6 +532,9 @@ 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::InvalidType { name, expected, .. } => {
                 write!(f, "'{}' is not {}", name, expected)
             }
@@ -634,7 +647,8 @@ impl miette::Diagnostic for DescriptorErrorKind {
             DescriptorErrorKind::FieldNumberInExtensionRange { .. } => None,
             DescriptorErrorKind::DuplicateFieldJsonName { .. } => None,
             DescriptorErrorKind::DuplicateFieldCamelCaseName { .. } => None,
-            DescriptorErrorKind::NameNotFound { help, .. } => help
+            DescriptorErrorKind::NameNotFound { help, .. }
+            | DescriptorErrorKind::NameShadowed { help, .. } => help
                 .as_ref()
                 .map(|h| -> Box<dyn fmt::Display> { Box::new(h.clone()) }),
             DescriptorErrorKind::InvalidType { .. } => None,
@@ -685,7 +699,8 @@ impl miette::Diagnostic for DescriptorErrorKind {
                 spans.extend(first.to_span());
                 spans.extend(second.to_span());
             }
-            DescriptorErrorKind::NameNotFound { found, .. } => {
+            DescriptorErrorKind::NameNotFound { found, .. }
+            | DescriptorErrorKind::NameShadowed { found, .. } => {
                 spans.extend(found.to_span());
             }
             DescriptorErrorKind::InvalidFieldNumber { found, .. } => {

prost-reflect/src/descriptor/mod.rs

@@ -154,14 +154,14 @@ struct Identity {
     name_index: usize,
 }
 
-#[derive(Clone)]
+#[derive(Clone, Debug)]
 struct Definition {
     file: FileIndex,
     path: Box<[i32]>,
     kind: DefinitionKind,
 }
 
-#[derive(Copy, Clone)]
+#[derive(Copy, Clone, Debug)]
 enum DefinitionKind {
     Package,
     Message(MessageIndex),

prost-reflect/src/descriptor/tests.rs

@@ -146,15 +146,8 @@ fn resolve_message_name_conflict_with_field_name() {
         }],
     };
 
-    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"
-    );
+    let e = DescriptorPool::from_file_descriptor_set(file_descriptor_set).unwrap_err();
+    assert_eq!(e.to_string(), "name 'MyMessage' is shadowed");
 }
 
 #[test]

prost-reflect/tests/data/enum_field_invalid_default3.yml

@@ -12,7 +12,7 @@ file:
   messageType:
   - name: Bar
     field:
-    - name: foo
+    - name: foo_field
       number: 1
       label: LABEL_OPTIONAL
       typeName: foo.Foo

prost-reflect/tests/data/name_resolution4.yml

@@ -0,0 +1,18 @@
+file:
+- name: dep.proto
+  package: foo.bar
+  messageType:
+  - name: FooBar
+- name: root.proto
+  package: com.foo.bar
+  dependency:
+  - dep.proto
+  messageType:
+  - name: Foo
+    field:
+    - name: foobar
+      number: 1
+      label: LABEL_OPTIONAL
+      typeName: foo.FooBar
+      jsonName: foobar
+  syntax: proto3

prost-reflect/tests/main.rs

@@ -132,6 +132,7 @@ check_err!(invalid_service_type3);
 check_err!(name_resolution1);
 check_err!(name_resolution2);
 check_ok!(name_resolution3);
+check_err!(name_resolution4);
 check_err!(name_collision1);
 check_err!(name_collision2);
 check_err!(name_collision3);

prost-reflect/tests/snapshots/main__name_resolution4.snap

@@ -0,0 +1,11 @@
+---
+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."
+labels: []
+message: "name 'foo.FooBar' is shadowed"
+related: []
+severity: error