Validate that all referenced types are included in dependency files (#58

)

Fixes #57

CHANGELOG.md

@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Changed
 
 - The minimum supported rust version is now **1.64.0**.
+- When adding files to a `DescriptorPool`, the library now validates that all referenced types are contained within the dependency files (including files imported using `import public`). Fixes #57.
 
 ## [0.11.5] - 2023-08-29
 

prost-reflect/doc/intro.md

@@ -3,7 +3,7 @@ protobuf type definition is not known ahead of time.
 
 The main entry points into the API of this crate are:
 
-- [`DescriptorPool`] wraps a [`FileDescriptorSet`][prost_types::FileDescriptorSet] output by 
+- [`DescriptorPool`] wraps a [`FileDescriptorSet`][prost_types::FileDescriptorSet] output by
   the protobuf compiler to provide an API for inspecting type definitions.
-- [`DynamicMessage`] provides encoding, decoding and reflection of an arbitrary protobuf 
+- [`DynamicMessage`] provides encoding, decoding and reflection of an arbitrary protobuf
   message definition described by a [`MessageDescriptor`].

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

@@ -3,12 +3,16 @@ mod options;
 mod resolve;
 mod visit;
 
-use std::{borrow::Cow, collections::HashMap, sync::Arc};
+use std::{
+    borrow::Cow,
+    collections::{HashMap, HashSet},
+    sync::Arc,
+};
 
 use crate::{
     descriptor::{
-        to_index, types::FileDescriptorProto, Definition, DefinitionKind, DescriptorPoolInner,
-        EnumIndex, ExtensionIndex, FileIndex, MessageIndex, ServiceIndex,
+        error::NameNotFoundHelp, to_index, types::FileDescriptorProto, Definition, DefinitionKind,
+        DescriptorPoolInner, EnumIndex, ExtensionIndex, FileIndex, MessageIndex, ServiceIndex,
     },
     DescriptorError, DescriptorPool,
 };
@@ -130,44 +134,70 @@ fn to_json_name(name: &str) -> String {
 }
 
 fn resolve_name<'a, 'b>(
+    dependencies: &HashSet<FileIndex>,
     names: &'a HashMap<Box<str>, Definition>,
     scope: &str,
     name: &'b str,
-) -> Option<(Cow<'b, str>, &'a Definition)> {
+) -> Result<(Cow<'b, str>, &'a Definition), NameNotFoundHelp> {
     match name.strip_prefix('.') {
-        Some(full_name) => names.get(full_name).map(|def| (Cow::Borrowed(name), def)),
-        None => resolve_relative_name(names, scope, name)
-            .map(|(resolved_name, def)| (Cow::Owned(resolved_name), def)),
+        Some(full_name) => match names.get(full_name) {
+            Some(def) => {
+                if dependencies.contains(&def.file) {
+                    Ok((Cow::Borrowed(full_name), def))
+                } else {
+                    Err(vec![(full_name.to_owned(), def.file)])
+                }
+            }
+            None => Err(vec![]),
+        },
+        None => {
+            let (resolved_name, def) = resolve_relative_name(dependencies, names, scope, name)?;
+            Ok((Cow::Owned(resolved_name), def))
+        }
     }
 }
 
 fn resolve_relative_name<'a>(
+    dependencies: &HashSet<FileIndex>,
     names: &'a HashMap<Box<str>, Definition>,
     scope: &str,
     relative_name: &str,
-) -> Option<(String, &'a Definition)> {
+) -> Result<(String, &'a Definition), NameNotFoundHelp> {
     let mut buf = format!(".{}.{}", scope, relative_name);
+    let mut help = Vec::new();
 
     if let Some(def) = names.get(&buf[1..]) {
-        return Some((buf, def));
+        if dependencies.contains(&def.file) {
+            return Ok((buf, def));
+        } else {
+            help.push((buf[1..].to_owned(), def.file));
+        }
     }
 
     for (i, _) in scope.rmatch_indices('.') {
         buf.truncate(i + 2);
         buf.push_str(relative_name);
 
         if let Some(def) = names.get(&buf[1..]) {
-            return Some((buf, def));
+            if dependencies.contains(&def.file) {
+                return Ok((buf, def));
+            } else {
+                help.push((buf[1..].to_owned(), def.file));
+            }
         }
     }
 
     buf.truncate(1);
     buf.push_str(relative_name);
     if let Some(def) = names.get(&buf[1..]) {
-        return Some((buf, def));
+        if dependencies.contains(&def.file) {
+            return Ok((buf, def));
+        } else {
+            help.push((buf[1..].to_owned(), def.file));
+        }
     }
 
-    None
+    Err(help)
 }
 
 fn join_path(path1: &[i32], path2: &[i32]) -> Box<[i32]> {

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

@@ -1,4 +1,4 @@
-use std::collections::{hash_map, BTreeMap, HashMap};
+use std::collections::{hash_map, BTreeMap, HashMap, HashSet};
 
 use crate::{
     descriptor::{
@@ -83,6 +83,7 @@ impl<'a> Visitor for NameVisitor<'a> {
             raw: file.clone(),
             prost: Default::default(), // the prost descriptor is initialized from the internal descriptor once resolution is complete, to avoid needing to duplicate all modifications
             dependencies: Vec::with_capacity(file.dependency.len()),
+            transitive_dependencies: HashSet::default(),
         });
 
         if !file.package().is_empty() {

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

@@ -9,7 +9,7 @@ use crate::{
             visit::{visit, Visitor},
             DescriptorPoolOffsets,
         },
-        error::{DescriptorErrorKind, Label},
+        error::{fmt_name_not_found_help, DescriptorErrorKind, Label},
         tag,
         types::{
             uninterpreted_option, DescriptorProto, EnumDescriptorProto, EnumValueDescriptorProto,
@@ -357,53 +357,40 @@ impl<'a> OptionsVisitor<'a> {
 
             let desc = message.descriptor();
             if part.is_extension {
-                match self.find_extension(scope, &part.name_part) {
-                    Some(extension_desc) => {
-                        resolved_path.push(extension_desc.number() as i32);
-
-                        if is_last {
-                            if extension_desc.cardinality() != Cardinality::Repeated
-                                && message.has_extension(&extension_desc)
-                            {
-                                return Err(DescriptorErrorKind::DuplicateOption {
-                                    name: fmt_option_name(&option.name),
-                                    found: Label::new(
-                                        &self.pool.inner.files,
-                                        "found here",
-                                        file,
-                                        path,
-                                    ),
-                                });
-                            } else {
-                                self.set_field_value(
-                                    message.get_extension_mut(&extension_desc),
-                                    &mut resolved_path,
-                                    &extension_desc,
-                                    option,
-                                    file,
-                                    &path,
-                                )?;
-                            }
-                        } else if let Value::Message(submessage) =
-                            message.get_extension_mut(&extension_desc)
-                        {
-                            message = submessage;
-                        } else {
-                            return Err(DescriptorErrorKind::InvalidOptionType {
-                                name: fmt_option_name(&option.name[..i + 1]),
-                                ty: fmt_field_ty(&extension_desc),
-                                value: fmt_value(option),
-                                is_last,
-                                found: Label::new(&self.pool.inner.files, "found here", file, path),
-                            });
-                        }
-                    }
-                    None => {
-                        return Err(DescriptorErrorKind::OptionNotFound {
-                            name: fmt_option_name(&option.name[..i + 1]),
+                let extension_desc =
+                    self.find_extension(scope, &part.name_part, file, &path, &desc)?;
+                resolved_path.push(extension_desc.number() as i32);
+
+                if is_last {
+                    if extension_desc.cardinality() != Cardinality::Repeated
+                        && message.has_extension(&extension_desc)
+                    {
+                        return Err(DescriptorErrorKind::DuplicateOption {
+                            name: fmt_option_name(&option.name),
                             found: Label::new(&self.pool.inner.files, "found here", file, path),
-                        })
+                        });
+                    } else {
+                        self.set_field_value(
+                            message.get_extension_mut(&extension_desc),
+                            &mut resolved_path,
+                            &extension_desc,
+                            option,
+                            file,
+                            &path,
+                        )?;
                     }
+                } else if let Value::Message(submessage) =
+                    message.get_extension_mut(&extension_desc)
+                {
+                    message = submessage;
+                } else {
+                    return Err(DescriptorErrorKind::InvalidOptionType {
+                        name: fmt_option_name(&option.name[..i + 1]),
+                        ty: fmt_field_ty(&extension_desc),
+                        value: fmt_value(option),
+                        is_last,
+                        found: Label::new(&self.pool.inner.files, "found here", file, path),
+                    });
                 }
             } else {
                 match desc.get_field_by_name(&part.name_part) {
@@ -526,19 +513,60 @@ impl<'a> OptionsVisitor<'a> {
         Ok(())
     }
 
-    fn find_extension(&self, scope: &str, name: &str) -> Option<ExtensionDescriptor> {
-        match resolve_name(&self.pool.inner.names, scope, name) {
-            Some((
+    #[allow(clippy::result_large_err)]
+    fn find_extension(
+        &self,
+        scope: &str,
+        name: &str,
+        file: FileIndex,
+        path: &[i32],
+        extendee: &MessageDescriptor,
+    ) -> Result<ExtensionDescriptor, DescriptorErrorKind> {
+        match resolve_name(
+            &self.pool.inner.files[file as usize].transitive_dependencies,
+            &self.pool.inner.names,
+            scope,
+            name,
+        ) {
+            Ok((
                 _,
                 &Definition {
                     kind: DefinitionKind::Extension(index),
                     ..
                 },
-            )) => Some(ExtensionDescriptor {
-                pool: self.pool.clone(),
-                index,
+            )) => {
+                let desc = ExtensionDescriptor {
+                    pool: self.pool.clone(),
+                    index,
+                };
+
+                if desc.containing_message() == *extendee {
+                    Ok(desc)
+                } else {
+                    Err(DescriptorErrorKind::InvalidOptionExtendee {
+                        name: desc.full_name().to_owned(),
+                        expected_extendee: extendee.full_name().to_owned(),
+                        actual_extendee: desc.containing_message().full_name().to_owned(),
+                        found: Label::new(&self.pool.inner.files, "found here", file, path.into()),
+                    })
+                }
+            }
+            Ok((_, def)) => {
+                let def_file = def.file;
+                let def_path = def.path.clone();
+
+                Err(DescriptorErrorKind::InvalidType {
+                    name: name.to_owned(),
+                    expected: "an extension".to_owned(),
+                    found: Label::new(&self.pool.inner.files, "found here", file, path.into()),
+                    defined: Label::new(&self.pool.inner.files, "defined here", def_file, def_path),
+                })
+            }
+            Err(help) => Err(DescriptorErrorKind::NameNotFound {
+                name: name.to_owned(),
+                found: Label::new(&self.pool.inner.files, "found here", file, path.into()),
+                help: fmt_name_not_found_help(&self.pool.inner.files, file, help),
             }),
-            _ => None,
         }
     }
 }