diff --git a/prost-reflect/src/descriptor/build/mod.rs b/prost-reflect/src/descriptor/build/mod.rs index 2f1c3372..19d5c54b 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> { +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, names: &'a HashMap, 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,13 +314,13 @@ fn to_json_name(name: &str) -> String { result } -fn resolve_name<'a, 'b>( +fn resolve_name<'a, 'b, 'c>( dependencies: &HashSet, names: &'a HashMap, 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), @@ -304,38 +328,47 @@ fn resolve_name<'a, 'b>( } } -fn resolve_relative_name<'a>( +fn resolve_relative_name<'a, 'b, 'c>( dependencies: &HashSet, names: &'a HashMap, 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> + '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 { + iter::once(scope) + .chain(scope.rmatch_indices('.').map(move |(i, _)| &scope[..i])) + .chain(iter::once("")) } fn join_path(path1: &[i32], path2: &[i32]) -> Box<[i32]> { diff --git a/prost-reflect/src/descriptor/error.rs b/prost-reflect/src/descriptor/error.rs index 94695e34..5c2383cb 100644 --- a/prost-reflect/src/descriptor/error.rs +++ b/prost-reflect/src/descriptor/error.rs @@ -83,6 +83,12 @@ pub(super) enum DescriptorErrorKind { #[cfg_attr(not(feature = "miette"), allow(dead_code))] help: Option, }, + NameShadowed { + name: String, + found: Label, + #[cfg_attr(not(feature = "miette"), allow(dead_code))] + help: Option, + }, 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 { 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, .. } => { diff --git a/prost-reflect/src/descriptor/mod.rs b/prost-reflect/src/descriptor/mod.rs index 642ab4ed..09479cfc 100644 --- a/prost-reflect/src/descriptor/mod.rs +++ b/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), diff --git a/prost-reflect/src/descriptor/tests.rs b/prost-reflect/src/descriptor/tests.rs index 391c4c8a..c2495844 100644 --- a/prost-reflect/src/descriptor/tests.rs +++ b/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] diff --git a/prost-reflect/tests/data/enum_field_invalid_default3.yml b/prost-reflect/tests/data/enum_field_invalid_default3.yml index b2db4925..20c494f7 100644 --- a/prost-reflect/tests/data/enum_field_invalid_default3.yml +++ b/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 diff --git a/prost-reflect/tests/data/name_resolution4.yml b/prost-reflect/tests/data/name_resolution4.yml new file mode 100644 index 00000000..5e80a2c0 --- /dev/null +++ b/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 diff --git a/prost-reflect/tests/main.rs b/prost-reflect/tests/main.rs index 9308e10a..15fec9da 100644 --- a/prost-reflect/tests/main.rs +++ b/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); diff --git a/prost-reflect/tests/snapshots/main__name_resolution4.snap b/prost-reflect/tests/snapshots/main__name_resolution4.snap new file mode 100644 index 00000000..6e840f05 --- /dev/null +++ b/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