Repository: apollographql/apollo-rs
Pull request #665 (2023-09-27 15:09:31)

Ignore component origin in == and hash, turn some maps into sets
`Component<T>` and `ComponentStr` now compare equal based on their content, without considering anymore whether they come from a definition or an extension.

Because `ExtensionId` is an arbitrary integer, parsing the same SDL twice yields different IDs. This would cause the two `Schema` structs not to compare equal, which made it impossible to use the invariant "serializing and reparsing gives the same tree" for fuzzing.

Additionally, instances of `IndexMap<NodeStr, ComponentOrigin>` have been simplified to the now-equivalent `IndexSet<ComponentStr>`.


Patch:
diff --git a/crates/apollo-compiler/src/schema/component.rs b/crates/apollo-compiler/src/schema/component.rs
index 55498fc..18b1593 100644
--- a/crates/apollo-compiler/src/schema/component.rs
+++ b/crates/apollo-compiler/src/schema/component.rs
@@ -1,6 +1,7 @@
 use crate::Node;
 use crate::NodeLocation;
 use crate::NodeStr;
+use std::hash;
 use std::ops::Deref;
 use std::ops::DerefMut;
 use triomphe::Arc;
@@ -12,7 +13,7 @@ use triomphe::Arc;
 ///
 /// Implements [`Deref`] and [`DerefMut`]
 /// so that methods and fields of `Node<T>` and `T` can be accessed directly.
-#[derive(Debug, Clone, Hash, PartialEq, Eq)]
+#[derive(Debug, Clone)]
 pub struct Component<T> {
     pub origin: ComponentOrigin,
     pub node: Node<T>,
@@ -51,8 +52,8 @@ impl PartialEq for ExtensionId {
     }
 }
 
-impl std::hash::Hash for ExtensionId {
-    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+impl hash::Hash for ExtensionId {
+    fn hash<H: hash::Hasher>(&self, state: &mut H) {
         Arc::as_ptr(&self.arc).hash(state);
     }
 }
@@ -76,6 +77,20 @@ impl<T> Component<T> {
     }
 }
 
+impl<T: hash::Hash> hash::Hash for Component<T> {
+    fn hash<H: hash::Hasher>(&self, state: &mut H) {
+        self.node.hash(state); // ignore `origin`
+    }
+}
+
+impl<T: Eq> Eq for Component<T> {}
+
+impl<T: PartialEq> PartialEq for Component<T> {
+    fn eq(&self, other: &Self) -> bool {
+        self.node == other.node // ignore `origin`
+    }
+}
+
 impl<T> Deref for Component<T> {
     type Target = Node<T>;
 
@@ -109,7 +124,7 @@ impl<T> From<T> for Component<T> {
 ///
 /// Implements [`Deref`]
 /// so that methods and fields of `NodeStr` and [`str`] can be accessed directly.
-#[derive(Debug, Clone, Hash, PartialEq, Eq)]
+#[derive(Debug, Clone)]
 pub struct ComponentStr {
     pub origin: ComponentOrigin,
     pub node: NodeStr,
@@ -126,6 +141,20 @@ impl ComponentStr {
     }
 }
 
+impl hash::Hash for ComponentStr {
+    fn hash<H: hash::Hasher>(&self, state: &mut H) {
+        self.node.hash(state); // ignore `origin`
+    }
+}
+
+impl Eq for ComponentStr {}
+
+impl PartialEq for ComponentStr {
+    fn eq(&self, other: &Self) -> bool {
+        self.node == other.node // ignore `origin`
+    }
+}
+
 impl Deref for ComponentStr {
     type Target = NodeStr;
 
@@ -135,6 +164,18 @@ impl Deref for ComponentStr {
     }
 }
 
+impl std::borrow::Borrow<NodeStr> for ComponentStr {
+    fn borrow(&self) -> &NodeStr {
+        self
+    }
+}
+
+impl std::borrow::Borrow<str> for ComponentStr {
+    fn borrow(&self) -> &str {
+        self
+    }
+}
+
 impl AsRef<str> for ComponentStr {
     fn as_ref(&self) -> &str {
         self
diff --git a/crates/apollo-compiler/src/schema/from_ast.rs b/crates/apollo-compiler/src/schema/from_ast.rs
index 3dfc703..3dd02b6 100644
--- a/crates/apollo-compiler/src/schema/from_ast.rs
+++ b/crates/apollo-compiler/src/schema/from_ast.rs
@@ -422,15 +422,15 @@ impl ObjectType {
         let mut ty = Self {
             name: definition.name.clone(),
             description: definition.description.clone(),
-            implements_interfaces: collect_sticky(
+            implements_interfaces: collect_sticky_set(
                 definition
                     .implements_interfaces
                     .iter()
-                    .map(|name| (name, ComponentOrigin::Definition)),
-                |dup_key, _| {
+                    .map(|name| name.to_component(ComponentOrigin::Definition)),
+                |dup| {
                     errors.push(BuildError::DuplicateImplementsInterface {
                         implementer_name: definition.name.clone(),
-                        interface_name: dup_key.clone(),
+                        interface_name: dup.node,
                     })
                 },
             ),
@@ -472,16 +472,16 @@ impl ObjectType {
                 .iter()
                 .map(|d| d.to_component(origin.clone())),
         );
-        extend_sticky(
+        extend_sticky_set(
             &mut self.implements_interfaces,
             extension
                 .implements_interfaces
                 .iter()
-                .map(|name| (name, origin.clone())),
-            |dup_key, _| {
+                .map(|name| name.to_component(origin.clone())),
+            |dup| {
                 errors.push(BuildError::DuplicateImplementsInterface {
                     implementer_name: extension.name.clone(),
-                    interface_name: dup_key.clone(),
+                    interface_name: dup.node,
                 })
             },
         );
@@ -510,15 +510,15 @@ impl InterfaceType {
         let mut ty = Self {
             name: definition.name.clone(),
             description: definition.description.clone(),
-            implements_interfaces: collect_sticky(
+            implements_interfaces: collect_sticky_set(
                 definition
                     .implements_interfaces
                     .iter()
-                    .map(|name| (name, ComponentOrigin::Definition)),
-                |dup_key, _| {
+                    .map(|name| name.to_component(ComponentOrigin::Definition)),
+                |dup| {
                     errors.push(BuildError::DuplicateImplementsInterface {
                         implementer_name: definition.name.clone(),
-                        interface_name: dup_key.clone(),
+                        interface_name: dup.node,
                     })
                 },
             ),
@@ -560,16 +560,16 @@ impl InterfaceType {
                 .iter()
                 .map(|d| d.to_component(origin.clone())),
         );
-        extend_sticky(
+        extend_sticky_set(
             &mut self.implements_interfaces,
             extension
                 .implements_interfaces
                 .iter()
-                .map(|name| (name, origin.clone())),
-            |dup_key, _| {
+                .map(|name| name.to_component(origin.clone())),
+            |dup| {
                 errors.push(BuildError::DuplicateImplementsInterface {
                     implementer_name: extension.name.clone(),
-                    interface_name: dup_key.clone(),
+                    interface_name: dup.node,
                 })
             },
         );
@@ -603,15 +603,15 @@ impl UnionType {
                 .iter()
                 .map(|d| d.to_component(ComponentOrigin::Definition))
                 .collect(),
-            members: collect_sticky(
+            members: collect_sticky_set(
                 definition
                     .members
                     .iter()
-                    .map(|name| (name, ComponentOrigin::Definition)),
-                |dup_key, _| {
+                    .map(|name| name.to_component(ComponentOrigin::Definition)),
+                |dup| {
                     errors.push(BuildError::UnionMemberNameCollision {
                         union_name: definition.name.clone(),
-                        member: dup_key.clone(),
+                        member: dup.node,
                     })
                 },
             ),
@@ -636,13 +636,16 @@ impl UnionType {
                 .iter()
                 .map(|d| d.to_component(origin.clone())),
         );
-        extend_sticky(
+        extend_sticky_set(
             &mut self.members,
-            extension.members.iter().map(|name| (name, origin.clone())),
-            |dup_key, _| {
+            extension
+                .members
+                .iter()
+                .map(|name| name.to_component(origin.clone())),
+            |dup| {
                 errors.push(BuildError::UnionMemberNameCollision {
                     union_name: extension.name.clone(),
-                    member: dup_key.clone(),
+                    member: dup.node,
                 })
             },
         );
@@ -826,3 +829,25 @@ fn collect_sticky<'a, V>(
     extend_sticky(&mut map, iter, duplicate);
     map
 }
+
+fn extend_sticky_set(
+    set: &mut IndexSet<ComponentStr>,
+    iter: impl IntoIterator<Item = ComponentStr>,
+    mut duplicate: impl FnMut(ComponentStr),
+) {
+    for value in iter.into_iter() {
+        if !set.contains(&value) {
+            set.insert(value);
+        } else {
+            duplicate(value)
+        }
+    }
+}
+fn collect_sticky_set(
+    iter: impl IntoIterator<Item = ComponentStr>,
+    duplicate: impl FnMut(ComponentStr),
+) -> IndexSet<ComponentStr> {
+    let mut set = IndexSet::new();
+    extend_sticky_set(&mut set, iter, duplicate);
+    set
+}
diff --git a/crates/apollo-compiler/src/schema/mod.rs b/crates/apollo-compiler/src/schema/mod.rs
index a4d13a7..be6cf2c 100644
--- a/crates/apollo-compiler/src/schema/mod.rs
+++ b/crates/apollo-compiler/src/schema/mod.rs
@@ -90,7 +90,7 @@ pub struct ScalarType {
 pub struct ObjectType {
     pub name: Name,
     pub description: Option<NodeStr>,
-    pub implements_interfaces: IndexMap<Name, ComponentOrigin>,
+    pub implements_interfaces: IndexSet<ComponentStr>,
     pub directives: Directives,
 
     /// Explicit field definitions.
@@ -108,7 +108,7 @@ pub struct InterfaceType {
     /// * Key: name of an implemented interface
     /// * Value: which interface type extension defined this implementation,
     ///   or `None` for the interface type definition.
-    pub implements_interfaces: IndexMap<Name, ComponentOrigin>,
+    pub implements_interfaces: IndexSet<ComponentStr>,
 
     pub directives: Directives,
 
@@ -128,7 +128,7 @@ pub struct UnionType {
     /// * Key: name of a member object type
     /// * Value: which union type extension defined this implementation,
     ///   or `None` for the union type definition.
-    pub members: IndexMap<NamedType, ComponentOrigin>,
+    pub members: IndexSet<ComponentStr>,
 }
 
 #[derive(Debug, Clone, PartialEq, Eq)]
@@ -369,8 +369,8 @@ impl Schema {
                 | ExtendedType::Enum(_)
                 | ExtendedType::InputObject(_) => continue,
             };
-            for interface in interfaces.keys() {
-                map.entry(interface.clone())
+            for interface in interfaces {
+                map.entry(interface.node.clone())
                     .or_default()
                     .insert(ty_name.clone());
             }
@@ -393,9 +393,9 @@ impl Schema {
                     | ExtendedType::Enum(_)
                     | ExtendedType::InputObject(_) => return false,
                 }
-                .contains_key(abstract_type)
+                .contains(abstract_type)
             }),
-            ExtendedType::Union(def) => def.members.contains_key(maybe_subtype),
+            ExtendedType::Union(def) => def.members.contains(maybe_subtype),
             ExtendedType::Scalar(_)
             | ExtendedType::Object(_)
             | ExtendedType::Enum(_)
@@ -623,8 +623,8 @@ impl ObjectType {
             .flat_map(|dir| dir.origin.extension_id())
             .chain(
                 self.implements_interfaces
-                    .values()
-                    .flat_map(|origin| origin.extension_id()),
+                    .iter()
+                    .flat_map(|component| component.origin.extension_id()),
             )
             .chain(
                 self.fields
@@ -648,8 +648,8 @@ impl InterfaceType {
             .flat_map(|dir| dir.origin.extension_id())
             .chain(
                 self.implements_interfaces
-                    .values()
-                    .flat_map(|origin| origin.extension_id()),
+                    .iter()
+                    .flat_map(|component| component.origin.extension_id()),
             )
             .chain(
                 self.fields
@@ -673,8 +673,8 @@ impl UnionType {
             .flat_map(|dir| dir.origin.extension_id())
             .chain(
                 self.members
-                    .values()
-                    .flat_map(|origin| origin.extension_id()),
+                    .iter()
+                    .flat_map(|component| component.origin.extension_id()),
             )
             .collect()
     }
diff --git a/crates/apollo-compiler/src/schema/serialize.rs b/crates/apollo-compiler/src/schema/serialize.rs
index 752dbd5..8c7cac0 100644
--- a/crates/apollo-compiler/src/schema/serialize.rs
+++ b/crates/apollo-compiler/src/schema/serialize.rs
@@ -219,10 +219,10 @@ fn components<'a, T: 'a>(
         .collect()
 }
 
-fn names(names: &IndexMap<Name, ComponentOrigin>, ext: Option<&ExtensionId>) -> Vec<Name> {
+fn names(names: &IndexSet<ComponentStr>, ext: Option<&ExtensionId>) -> Vec<Name> {
     names
         .iter()
-        .filter(|(_name, origin)| origin.extension_id() == ext)
-        .map(|(name, _origin)| name.clone())
+        .filter(|component| component.origin.extension_id() == ext)
+        .map(|component| component.node.clone())
         .collect()
 }
diff --git a/crates/apollo-compiler/src/validation/fragment.rs b/crates/apollo-compiler/src/validation/fragment.rs
index 75e76f3..d572e04 100644
--- a/crates/apollo-compiler/src/validation/fragment.rs
+++ b/crates/apollo-compiler/src/validation/fragment.rs
@@ -31,7 +31,7 @@ pub fn get_possible_types<'a>(
                         _ => return None,
                     };
 
-                    if implements.contains_key(&intf.name) {
+                    if implements.contains(&intf.name) {
                         Some(name)
                     } else {
                         None
@@ -40,7 +40,11 @@ pub fn get_possible_types<'a>(
                 .collect()
         }
         // 3. If `type` is a union type, return the set of possible types of `type`.
-        Some(schema::ExtendedType::Union(union_)) => union_.members.keys().collect(),
+        Some(schema::ExtendedType::Union(union_)) => union_
+            .members
+            .iter()
+            .map(|component| &component.node)
+            .collect(),
         _ => Default::default(),
     }
 }
diff --git a/crates/apollo-compiler/src/validation/interface.rs b/crates/apollo-compiler/src/validation/interface.rs
index e19fbef..cb54b51 100644
--- a/crates/apollo-compiler/src/validation/interface.rs
+++ b/crates/apollo-compiler/src/validation/interface.rs
@@ -179,7 +179,8 @@ pub fn validate_implements_interfaces(
     let transitive_interfaces = interface_definitions.iter().flat_map(|&(name, interface)| {
         interface
             .implements_interfaces
-            .keys()
+            .iter()
+            .map(|component| &component.node)
             .zip(std::iter::repeat(name))
     });
     for (transitive_interface, via_interface) in transitive_interfaces {
diff --git a/fuzz/fuzz_targets/reparse.rs b/fuzz/fuzz_targets/reparse.rs
index 83ddc48..9faad81 100644
--- a/fuzz/fuzz_targets/reparse.rs
+++ b/fuzz/fuzz_targets/reparse.rs
@@ -8,8 +8,8 @@ use log::debug;
 use log::trace;
 use std::fmt::Debug;
 
-const ENABLE_EXECUTABLE: bool = false;
-const ENABLE_SCHEMA: bool = false;
+const ENABLE_EXECUTABLE: bool = true;
+const ENABLE_SCHEMA: bool = true;
 
 fuzz_target!(|input: &str| {
     let _ = env_logger::try_init();


Test patch:
diff --git a/crates/apollo-compiler/tests/merge_schemas.rs b/crates/apollo-compiler/tests/merge_schemas.rs
index d71cd30..72f8405 100644
--- a/crates/apollo-compiler/tests/merge_schemas.rs
+++ b/crates/apollo-compiler/tests/merge_schemas.rs
@@ -1,11 +1,13 @@
 use apollo_compiler::ast;
 use apollo_compiler::schema;
 use apollo_compiler::schema::Component;
+use apollo_compiler::schema::ComponentStr;
 use apollo_compiler::schema::ExtendedType;
 use apollo_compiler::ApolloCompiler;
 use apollo_compiler::ReprDatabase;
 use apollo_compiler::Schema;
 use indexmap::IndexMap;
+use indexmap::IndexSet;
 
 type MergeError = &'static str;
 
@@ -87,6 +89,14 @@ where
     Ok(())
 }
 
+fn merge_sets(merged: &mut IndexSet<ComponentStr>, new: &IndexSet<ComponentStr>) {
+    for value in new {
+        if !merged.contains(value) {
+            merged.insert(value.clone());
+        }
+    }
+}
+
 fn merge_type_definitions(merged: &mut ExtendedType, new: &ExtendedType) -> Result<(), MergeError> {
     match (merged, new) {
         (ExtendedType::Scalar(merged), ExtendedType::Scalar(new)) => {
@@ -125,11 +135,10 @@ fn merge_object_types(
 ) -> Result<(), &'static str> {
     merge_options(&mut merged.description, &new.description)?;
     merge_vecs(&mut merged.directives, &new.directives)?;
-    merge_maps(
+    merge_sets(
         &mut merged.implements_interfaces,
         &new.implements_interfaces,
-        |_, _| Ok(()), // ignore origin differences
-    )?;
+    );
     merge_maps(&mut merged.fields, &new.fields, merge_fields)
 }
 
@@ -139,11 +148,10 @@ fn merge_interface_types(
 ) -> Result<(), &'static str> {
     merge_options(&mut merged.description, &new.description)?;
     merge_vecs(&mut merged.directives, &new.directives)?;
-    merge_maps(
+    merge_sets(
         &mut merged.implements_interfaces,
         &new.implements_interfaces,
-        |_, _| Ok(()), // ignore origin differences
-    )?;
+    );
     merge_maps(&mut merged.fields, &new.fields, merge_fields)
 }
 
@@ -153,11 +161,8 @@ fn merge_union_types(
 ) -> Result<(), &'static str> {
     merge_options(&mut merged.description, &new.description)?;
     merge_vecs(&mut merged.directives, &new.directives)?;
-    merge_maps(
-        &mut merged.members,
-        &new.members,
-        |_, _| Ok(()), // ignore origin differences
-    )
+    merge_sets(&mut merged.members, &new.members);
+    Ok(())
 }
 
 fn merge_enum_types(