Add newtype wrapper around FragmentDefinition names

Summary: Create a newtype wrapper struct `FragmentDefinitionName` around `FragmentDefinition.name` to distinguish it from other `StringKey`s. Reviewed By: rbalicki2 Differential Revision: D38399922 fbshipit-source-id: c44cebeb720cc859283444a1931ba56483009b3c
facebook · Aug 9, 2022 · 639abe1 · 639abe1
1 parent b90d622
commit 639abe1
Show file tree

Hide file tree

Showing 149 changed files with 3,715 additions and 2,438 deletions.
diff --git a/compiler/crates/dependency-analyzer/src/ast.rs b/compiler/crates/dependency-analyzer/src/ast.rs
@@ -5,8 +5,10 @@
  * LICENSE file in the root directory of this source tree.
  */
 
+use graphql_ir::FragmentDefinitionName;
+use graphql_ir::FragmentDefinitionNameMap;
+use graphql_ir::FragmentDefinitionNameSet;
 use graphql_syntax::*;
-use intern::string_key::StringKey;
 use intern::string_key::StringKeyMap;
 use intern::string_key::StringKeySet;
 
@@ -16,7 +18,7 @@ pub struct ReachableAst {
     /// reachable from the project definitions.
     pub definitions: Vec<ExecutableDefinition>,
     /// The fragment names added from the base definitions.
-    pub base_fragment_names: StringKeySet,
+    pub base_fragment_names: FragmentDefinitionNameSet,
 }
 
 /// Get all definitions that reachable from project defintions
@@ -32,14 +34,15 @@ pub fn get_reachable_ast(
     }
 
     let mut reachable_base_asts = Default::default();
-    let mut base_definitions_map: StringKeyMap<ExecutableDefinition> = Default::default();
+    let mut base_definitions_map: FragmentDefinitionNameMap<ExecutableDefinition> =
+        Default::default();
 
     // Preprocess all base fragment definitions
     // Skipping operations because project definitions can't reference base operations
     for base_definition in base_definitions {
         match &base_definition {
             ExecutableDefinition::Fragment(fragment) => {
-                let name = fragment.name.value;
+                let name = FragmentDefinitionName(fragment.name.value);
                 assert!(
                     base_definitions_map.insert(name, base_definition).is_none(),
                     "get_reachable_ast called on graph with duplicate definition of `{}`",
@@ -111,19 +114,20 @@ pub fn get_definition_references<'a>(
 }
 
 fn visit_selections(
-    base_definitions_map: &StringKeyMap<ExecutableDefinition>,
-    reachable_base_asts: &mut StringKeySet,
+    base_definitions_map: &FragmentDefinitionNameMap<ExecutableDefinition>,
+    reachable_base_asts: &mut FragmentDefinitionNameSet,
     selections: &List<Selection>,
     is_base: bool,
 ) {
     for selection in &selections.items {
         match selection {
             Selection::FragmentSpread(selection) => {
-                if is_base || base_definitions_map.contains_key(&selection.name.value) {
+                let fragment_name = FragmentDefinitionName(selection.name.value);
+                if is_base || base_definitions_map.contains_key(&fragment_name) {
                     traverse_base_ast_definition(
                         base_definitions_map,
                         reachable_base_asts,
-                        selection.name.value,
+                        fragment_name,
                     )
                 }
             }
@@ -145,9 +149,9 @@ fn visit_selections(
 }
 
 fn traverse_base_ast_definition(
-    base_definitions_map: &StringKeyMap<ExecutableDefinition>,
-    reachable_base_asts: &mut StringKeySet,
-    key: StringKey,
+    base_definitions_map: &FragmentDefinitionNameMap<ExecutableDefinition>,
+    reachable_base_asts: &mut FragmentDefinitionNameSet,
+    key: FragmentDefinitionName,
 ) {
     if reachable_base_asts.contains(&key) {
         return;

diff --git a/compiler/crates/dependency-analyzer/src/ir.rs b/compiler/crates/dependency-analyzer/src/ir.rs
@@ -90,7 +90,7 @@ fn build_dependency_graph(
     for definition in definitions.into_iter() {
         let name = match &definition {
             ExecutableDefinition::Operation(operation) => operation.name.item.0,
-            ExecutableDefinition::Fragment(fragment) => fragment.name.item,
+            ExecutableDefinition::Fragment(fragment) => fragment.name.item.0,
         };
 
         // Visit the selections of the IR to build it's `children`
@@ -171,7 +171,7 @@ fn visit_selections(
     for selection in selections {
         match selection {
             Selection::FragmentSpread(node) => {
-                let current_node = node.fragment.item;
+                let current_node = node.fragment.item.0;
                 update_dependecy_graph(current_node, parent_name, dependency_graph, children);
             }
             Selection::InlineFragment(node) => {
@@ -187,7 +187,12 @@ fn visit_selections(
                 if let Some(fragment_name) =
                     get_resolver_fragment_name(schema.field(linked_field.definition.item))
                 {
-                    update_dependecy_graph(fragment_name, parent_name, dependency_graph, children);
+                    update_dependecy_graph(
+                        fragment_name.0,
+                        parent_name,
+                        dependency_graph,
+                        children,
+                    );
                 }
                 visit_selections(
                     schema,
@@ -201,7 +206,12 @@ fn visit_selections(
                 if let Some(fragment_name) =
                     get_resolver_fragment_name(schema.field(scalar_field.definition.item))
                 {
-                    update_dependecy_graph(fragment_name, parent_name, dependency_graph, children);
+                    update_dependecy_graph(
+                        fragment_name.0,
+                        parent_name,
+                        dependency_graph,
+                        children,
+                    );
                 }
             }
             Selection::Condition(node) => {

diff --git a/compiler/crates/dependency-analyzer/tests/ast/mod.rs b/compiler/crates/dependency-analyzer/tests/ast/mod.rs
@@ -34,7 +34,7 @@ pub fn transform_fixture(fixture: &Fixture<'_>) -> Result<String, String> {
     texts.push("========== Base definitions ==========".to_string());
     let mut defs = base_fragment_names
         .iter()
-        .map(|key| key.lookup())
+        .map(|key| key.0.lookup())
         .collect::<Vec<_>>();
     defs.sort_unstable();
     texts.push(defs.join(", "));

diff --git a/compiler/crates/graphql-ir-validations/src/validate_selection_conflict.rs b/compiler/crates/graphql-ir-validations/src/validate_selection_conflict.rs
@@ -23,6 +23,7 @@ use graphql_ir::node_identifier::LocationAgnosticBehavior;
 use graphql_ir::Argument;
 use graphql_ir::Field as IRField;
 use graphql_ir::FragmentDefinition;
+use graphql_ir::FragmentDefinitionName;
 use graphql_ir::LinkedField;
 use graphql_ir::OperationDefinition;
 use graphql_ir::Program;
@@ -87,13 +88,15 @@ impl<'s, B: LocationAgnosticBehavior + Sync> ValidateSelectionConflict<'s, B> {
 
     fn prewarm_fragments(&self, program: &'s Program) {
         // Validate the fragments in topology order.
-        let mut unclaimed_fragment_queue: VecDeque<StringKey> = VecDeque::new();
+        let mut unclaimed_fragment_queue: VecDeque<FragmentDefinitionName> = VecDeque::new();
 
         // Construct the dependency graph, which is represented by two maps:
         // DAG1: fragment K -> Used by: {Fragment v_1, v_2, v_3, ...}
         // DAG2: fragment K -> Spreading: {Fragment v_1, v_2, v_3, ...}
-        let mut dag_used_by: HashMap<StringKey, HashSet<StringKey>> = HashMap::new();
-        let mut dag_spreading: HashMap<StringKey, HashSet<StringKey>> = HashMap::new();
+        let mut dag_used_by: HashMap<FragmentDefinitionName, HashSet<FragmentDefinitionName>> =
+            HashMap::new();
+        let mut dag_spreading: HashMap<FragmentDefinitionName, HashSet<FragmentDefinitionName>> =
+            HashMap::new();
         for fragment in program.fragments() {
             let fragment_ = fragment.name.item;
             let spreads = fragment
@@ -180,12 +183,12 @@ impl<'s, B: LocationAgnosticBehavior + Sync> ValidateSelectionConflict<'s, B> {
         &self,
         fragment: &'s FragmentDefinition,
     ) -> DiagnosticsResult<Arc<Fields<'s>>> {
-        if let Some(cached) = self.fragment_cache.get(&fragment.name.item) {
+        if let Some(cached) = self.fragment_cache.get(&fragment.name.item.0) {
             return Ok(Arc::clone(&cached));
         }
         let fields = Arc::new(self.validate_selections(&fragment.selections)?);
         self.fragment_cache
-            .insert(fragment.name.item, Arc::clone(&fields));
+            .insert(fragment.name.item.0, Arc::clone(&fields));
         Ok(fields)
     }
 

diff --git a/compiler/crates/graphql-ir/Cargo.toml b/compiler/crates/graphql-ir/Cargo.toml
@@ -27,6 +27,7 @@ intern = { path = "../intern" }
 lazy_static = "1.4"
 once_cell = "1.12"
 schema = { path = "../schema" }
+serde = { version = "1.0.136", features = ["derive", "rc"] }
 thiserror = "1.0.30"
 
 [dev-dependencies]

diff --git a/compiler/crates/graphql-ir/src/build.rs b/compiler/crates/graphql-ir/src/build.rs
@@ -290,7 +290,7 @@ impl<'schema, 'signatures, 'options> Builder<'schema, 'signatures, 'options> {
     ) -> DiagnosticsResult<FragmentDefinition> {
         let signature = self
             .signatures
-            .get(&fragment.name.value)
+            .get(&FragmentDefinitionName(fragment.name.value))
             .expect("Expected signature to be created");
         let fragment_type = TypeReference::Named(signature.type_condition);
 
@@ -707,12 +707,17 @@ impl<'schema, 'signatures, 'options> Builder<'schema, 'signatures, 'options> {
         spread: &graphql_syntax::FragmentSpread,
         parent_types: &[TypeReference],
     ) -> DiagnosticsResult<FragmentSpread> {
-        let spread_name_with_location = spread
-            .name
-            .name_with_location(self.location.source_location());
+        let spread_name_with_location = WithLocation::from_span(
+            self.location.source_location(),
+            spread.name.span,
+            FragmentDefinitionName(spread.name.value),
+        );
 
         // Exit early if the fragment does not exist
-        let signature = match self.signatures.get(&spread.name.value) {
+        let signature = match self
+            .signatures
+            .get(&FragmentDefinitionName(spread.name.value))
+        {
             Some(fragment) => fragment,
             None if self.options.allow_undefined_fragment_spreads => {
                 let directives = if self.options.relay_mode.is_some() {
@@ -740,13 +745,13 @@ impl<'schema, 'signatures, 'options> Builder<'schema, 'signatures, 'options> {
                         self.find_conflicting_parent_type(parent_types, signature.type_condition)
                             .is_none()
                     })
-                    .map(|signature| signature.name.item)
+                    .map(|signature| signature.name.item.0)
                     .collect::<Vec<StringKey>>();
                 let suggestions =
                     suggestion_list::suggestion_list(spread.name.value, &fragment_names, 5);
                 return Err(vec![Diagnostic::error_with_data(
                     ValidationMessageWithData::UndefinedFragment {
-                        fragment_name: spread.name.value,
+                        fragment_name: FragmentDefinitionName(spread.name.value),
                         suggestions,
                     },
                     self.location.with_span(spread.name.span),
@@ -760,7 +765,7 @@ impl<'schema, 'signatures, 'options> Builder<'schema, 'signatures, 'options> {
             // no possible overlap
             return Err(vec![Diagnostic::error(
                 ValidationMessage::InvalidFragmentSpreadType {
-                    fragment_name: spread.name.value,
+                    fragment_name: FragmentDefinitionName(spread.name.value),
                     parent_type: self.schema.get_type_name(parent_type.inner()),
                     type_condition: self.schema.get_type_name(signature.type_condition),
                 },

diff --git a/compiler/crates/graphql-ir/src/errors.rs b/compiler/crates/graphql-ir/src/errors.rs
@@ -15,6 +15,8 @@ use schema::Type;
 use schema::TypeReference;
 use thiserror::Error;
 
+use crate::ir::FragmentDefinitionName;
+
 /// Fixed set of validation errors with custom display messages
 #[derive(Clone, Debug, Error, Eq, PartialEq, Ord, PartialOrd, Hash)]
 pub enum ValidationMessage {
@@ -140,7 +142,7 @@ pub enum ValidationMessage {
         "Invalid fragment spread '{fragment_name}', the type of this fragment ('{type_condition}') can never occur for parent type '{parent_type}'"
     )]
     InvalidFragmentSpreadType {
-        fragment_name: StringKey,
+        fragment_name: FragmentDefinitionName,
         parent_type: StringKey,
         type_condition: StringKey,
     },
@@ -356,15 +358,15 @@ pub enum ValidationMessage {
     )]
     UnusedFragmentVariable {
         variable_name: StringKey,
-        fragment_name: StringKey,
+        fragment_name: FragmentDefinitionName,
     },
 
     #[error(
         "Variable `${variable_name}` of fragment `{fragment_name}` is marked as unused using `unusedLocalVariable_DEPRECATED: true`, but is actually used. `unusedLocalVariable_DEPRECATED: true` should be removed."
     )]
     UselessUnusedFragmentVariableAnnotation {
         variable_name: StringKey,
-        fragment_name: StringKey,
+        fragment_name: FragmentDefinitionName,
     },
 
     #[error(
@@ -436,7 +438,9 @@ pub enum ValidationMessage {
     #[error(
         "The `raw_response_type` argument should be set to `true` for the @no_inline fragment `{fragment_name}` used in the query with @raw_response_type."
     )]
-    RequiredRawResponseTypeOnNoInline { fragment_name: StringKey },
+    RequiredRawResponseTypeOnNoInline {
+        fragment_name: FragmentDefinitionName,
+    },
 
     #[error("No fields can have an alias that start with two underscores.")]
     NoDoubleUnderscoreAlias,
@@ -465,7 +469,7 @@ pub enum ValidationMessageWithData {
 
     #[error("Undefined fragment '{fragment_name}'.{suggestions}", suggestions = did_you_mean(suggestions))]
     UndefinedFragment {
-        fragment_name: StringKey,
+        fragment_name: FragmentDefinitionName,
         suggestions: Vec<StringKey>,
     },
 

diff --git a/compiler/crates/graphql-ir/src/ir.rs b/compiler/crates/graphql-ir/src/ir.rs
@@ -5,8 +5,11 @@
  * LICENSE file in the root directory of this source tree.
  */
 
+use std::collections::HashMap;
+use std::collections::HashSet;
 use std::fmt;
 use std::fmt::Display;
+use std::fmt::Formatter;
 use std::hash::Hash;
 use std::sync::Arc;
 
@@ -17,11 +20,13 @@ use common::WithLocation;
 use graphql_syntax::FloatValue;
 use graphql_syntax::OperationKind;
 use intern::string_key::StringKey;
+use intern::BuildIdHasher;
 use schema::FieldID;
 use schema::SDLSchema;
 use schema::Schema;
 use schema::Type;
 use schema::TypeReference;
+use serde::Serialize;
 
 use crate::AssociatedData;
 // Definitions
@@ -48,10 +53,8 @@ impl ExecutableDefinition {
 
     pub fn name_with_location(&self) -> WithLocation<StringKey> {
         match self {
-            ExecutableDefinition::Operation(node) => {
-                WithLocation::new(node.name.location, node.name.item.0)
-            }
-            ExecutableDefinition::Fragment(node) => node.name,
+            ExecutableDefinition::Operation(node) => node.name.map(|x| x.0),
+            ExecutableDefinition::Fragment(node) => node.name.map(|x| x.0),
         }
     }
 }
@@ -93,10 +96,23 @@ impl OperationDefinition {
     }
 }
 
+/// A newtype wrapper around StringKey to represent a FragmentDefinition's name
+#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd, Serialize)]
+pub struct FragmentDefinitionName(pub StringKey);
+
+impl fmt::Display for FragmentDefinitionName {
+    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
+        self.0.fmt(f)
+    }
+}
+
+pub type FragmentDefinitionNameMap<V> = HashMap<FragmentDefinitionName, V, BuildIdHasher<u32>>;
+pub type FragmentDefinitionNameSet = HashSet<FragmentDefinitionName, BuildIdHasher<u32>>;
+
 /// A fully-typed fragment definition
 #[derive(Clone, Debug, Eq, PartialEq)]
 pub struct FragmentDefinition {
-    pub name: WithLocation<StringKey>,
+    pub name: WithLocation<FragmentDefinitionName>,
     pub variable_definitions: Vec<VariableDefinition>,
     pub used_global_variables: Vec<VariableDefinition>,
     pub type_condition: Type,
@@ -238,7 +254,7 @@ impl fmt::Debug for Selection {
 /// ... Name
 #[derive(Clone, Debug, Eq, PartialEq)]
 pub struct FragmentSpread {
-    pub fragment: WithLocation<StringKey>,
+    pub fragment: WithLocation<FragmentDefinitionName>,
     pub arguments: Vec<Argument>,
     pub directives: Vec<Directive>,
 }