Repository: apollographql/apollo-rs
Pull request #494 (2023-03-27 09:34:17)

feat(compiler): validate indirectly self-referential directives
Ref #248 

When defining a directive, that directive may not also be used on any type or directive references by the new directive. We already checked simple things like:
```graphql
directive @selfReference(arg: Int @selfReference) on ARGUMENT_DEFINITION
```

This PR recurses into types and directives referenced by a directive definition.

Examples of new error cases:

**Indirect recursion through directives** (reports one error per directive definition)
```graphql
directive @loopA(arg: Boolean @loopB) on ARGUMENT_DEFINITION
directive @loopB(arg: Boolean @loopC) on ARGUMENT_DEFINITION
directive @loopC(arg: Boolean @loopA) on ARGUMENT_DEFINITION
```
![image](https://user-images.githubusercontent.com/1006268/227903126-854b5afe-bb7c-4523-8cff-c00f2f124393.png)


**Indirect recursion through a type extension**
```graphql
extend scalar String @deprecatedType(reason: "use OurCustomString instead")
directive @deprecatedType(reason: String!) on OBJECT | INTERFACE | ENUM | SCALAR | UNION
```
![image](https://user-images.githubusercontent.com/1006268/227920241-b078530e-5547-4e59-9e21-2d8d5eb045cf.png)


**Recursion through a field in an input object**
```graphql
enum Enum { NON_RECURSIVE }
extend enum Enum { RECURSIVE @wrong }
input Name { value: String!, type: Enum }
input InputObject { name: Name }
directive @wrong(input: InputObject) on INPUT_FIELD_DEFINITION
````
![image](https://user-images.githubusercontent.com/1006268/227903171-4028cd94-681d-46e7-8b5e-90a68c5da632.png)
(in the future we could trace it through all the types that make up the recursion & point to each step)

lrlna: ah, you too are having issues with circle ci! i had to log out of circle, log back in, and then push a commit again. than the config.yaml "magically" appeared

Patch:
diff --git a/crates/apollo-compiler/src/database/hir.rs b/crates/apollo-compiler/src/database/hir.rs
index be43c8d..1fefb8a 100644
--- a/crates/apollo-compiler/src/database/hir.rs
+++ b/crates/apollo-compiler/src/database/hir.rs
@@ -197,6 +197,7 @@ impl TypeDefinition {
             Self::ScalarTypeDefinition(def) => def.loc(),
         }
     }
+
     /// Returns `true` if the type definition is [`ScalarTypeDefinition`].
     ///
     /// [`ScalarTypeDefinition`]: TypeDefinition::ScalarTypeDefinition
@@ -844,6 +845,19 @@ impl DirectiveDefinition {
         self.loc
     }
 
+    /// Get the location information for the "head" of the directive definition, namely the
+    /// `directive` keyword and the name.
+    pub(crate) fn head_loc(&self) -> HirNodeLocation {
+        self.name_src()
+            .loc()
+            .map(|name_loc| HirNodeLocation {
+                // Adjust the node length to include the name
+                node_len: name_loc.end_offset() - self.loc.offset(),
+                ..self.loc
+            })
+            .unwrap_or(self.loc)
+    }
+
     /// Checks if current directive is one of built-in directives - `@skip`,
     /// `@include`, `@deprecated`, `@specifiedBy`.
     pub fn is_built_in(&self) -> bool {
@@ -2201,7 +2215,7 @@ impl InputValueDefinition {
         self.description.as_deref()
     }
 
-    /// Get a reference to input value definition's directives.
+    /// Return the directives used on this input value definition.
     pub fn directives(&self) -> &[Directive] {
         self.directives.as_ref()
     }
@@ -3350,6 +3364,11 @@ impl HirNodeLocation {
         self.offset
     }
 
+    /// Get the source offset of the end of the current node.
+    pub fn end_offset(&self) -> usize {
+        self.offset + self.node_len
+    }
+
     /// Get node length.
     pub fn node_len(&self) -> usize {
         self.node_len
diff --git a/crates/apollo-compiler/src/validation/directive.rs b/crates/apollo-compiler/src/validation/directive.rs
index 7105500..874d70c 100644
--- a/crates/apollo-compiler/src/validation/directive.rs
+++ b/crates/apollo-compiler/src/validation/directive.rs
@@ -7,6 +7,130 @@ use crate::{
     ValidationDatabase,
 };
 
+/// Track used names in a recursive function.
+///
+/// Pass the result of `stack.push(name)` to recursive calls. Use `stack.contains(name)` to check
+/// if the name was used somewhere up the call stack.
+struct RecursionStack<'a>(&'a mut Vec<String>);
+impl RecursionStack<'_> {
+    fn push(&mut self, name: String) -> RecursionStack<'_> {
+        self.0.push(name);
+        RecursionStack(self.0)
+    }
+    fn contains(&self, name: &str) -> bool {
+        self.0.iter().any(|seen| seen == name)
+    }
+    fn first(&self) -> Option<&str> {
+        self.0.get(0).map(|s| s.as_str())
+    }
+}
+impl Drop for RecursionStack<'_> {
+    fn drop(&mut self) {
+        self.0.pop();
+    }
+}
+
+/// This struct just groups functions that are used to find self-referential directives.
+/// The way to use it is to call `FindRecursiveDirective::check`.
+struct FindRecursiveDirective<'a> {
+    db: &'a dyn ValidationDatabase,
+}
+impl FindRecursiveDirective<'_> {
+    fn type_definition(
+        &self,
+        seen: &mut RecursionStack<'_>,
+        def: &hir::TypeDefinition,
+    ) -> Result<(), hir::Directive> {
+        for directive in def.directives() {
+            self.directive(seen, directive)?;
+        }
+
+        match def {
+            hir::TypeDefinition::InputObjectTypeDefinition(input_type_definition) => {
+                for input_value in input_type_definition.fields() {
+                    self.input_value(seen, input_value)?;
+                }
+            }
+            hir::TypeDefinition::EnumTypeDefinition(enum_type_definition) => {
+                for enum_value in enum_type_definition.values() {
+                    self.enum_value(seen, enum_value)?;
+                }
+            }
+            _ => (),
+        }
+
+        Ok(())
+    }
+
+    fn input_value(
+        &self,
+        seen: &mut RecursionStack<'_>,
+        input_value: &hir::InputValueDefinition,
+    ) -> Result<(), hir::Directive> {
+        for directive in input_value.directives() {
+            self.directive(seen, directive)?;
+        }
+        if let Some(type_def) = input_value.ty().type_def(self.db.upcast()) {
+            self.type_definition(seen, &type_def)?;
+        }
+
+        Ok(())
+    }
+
+    fn enum_value(
+        &self,
+        seen: &mut RecursionStack<'_>,
+        enum_value: &hir::EnumValueDefinition,
+    ) -> Result<(), hir::Directive> {
+        for directive in enum_value.directives() {
+            self.directive(seen, directive)?;
+        }
+
+        Ok(())
+    }
+
+    fn directive(
+        &self,
+        seen: &mut RecursionStack<'_>,
+        directive: &hir::Directive,
+    ) -> Result<(), hir::Directive> {
+        if !seen.contains(directive.name()) {
+            if let Some(def) = directive.directive(self.db.upcast()) {
+                self.directive_definition(seen.push(directive.name().to_string()), &def)?;
+            }
+        } else if seen.first() == Some(directive.name()) {
+            // Only report an error & bail out early if this is the *initial* directive.
+            // This prevents raising confusing errors when a directive `@b` which is not
+            // self-referential uses a directive `@a` that *is*. The error with `@a` should
+            // only be reported on its definition, not on `@b`'s.
+            return Err(directive.clone());
+        }
+
+        Ok(())
+    }
+
+    fn directive_definition(
+        &self,
+        mut seen: RecursionStack<'_>,
+        def: &hir::DirectiveDefinition,
+    ) -> Result<(), hir::Directive> {
+        let mut guard = seen.push(def.name().to_string());
+        for input_value in def.arguments().input_values() {
+            self.input_value(&mut guard, input_value)?;
+        }
+
+        Ok(())
+    }
+
+    fn check(
+        db: &dyn ValidationDatabase,
+        directive_def: &hir::DirectiveDefinition,
+    ) -> Result<(), hir::Directive> {
+        FindRecursiveDirective { db }
+            .directive_definition(RecursionStack(&mut vec![]), directive_def)
+    }
+}
+
 pub fn validate_directive_definitions(db: &dyn ValidationDatabase) -> Vec<ApolloDiagnostic> {
     let mut diagnostics = Vec::new();
 
@@ -15,23 +139,19 @@ pub fn validate_directive_definitions(db: &dyn ValidationDatabase) -> Vec<Apollo
     //
     // Returns Recursive Definition error.
     for (name, directive_def) in db.directive_definitions().iter() {
-        for input_values in directive_def.arguments().input_values() {
-            for directive in input_values.directives().iter() {
-                let directive_name = directive.name();
-                if name == directive_name {
-                    diagnostics.push(
-                        ApolloDiagnostic::new(
-                            db,
-                            directive.loc().into(),
-                            DiagnosticData::RecursiveDefinition { name: name.clone() },
-                        )
-                        .label(Label::new(
-                            directive.loc(),
-                            "recursive directive definition",
-                        )),
-                    );
-                }
-            }
+        if let Err(directive) = FindRecursiveDirective::check(db, directive_def) {
+            diagnostics.push(
+                ApolloDiagnostic::new(
+                    db,
+                    directive_def.loc().into(),
+                    DiagnosticData::RecursiveDefinition { name: name.clone() },
+                )
+                .label(Label::new(
+                    directive_def.head_loc(),
+                    "recursive directive definition",
+                ))
+                .label(Label::new(directive.loc(), "refers to itself here")),
+            );
         }
 
         // Validate directive definitions' arguments


Test patch:
diff --git a/crates/apollo-compiler/test_data/diagnostics/0070_self_referential_directive_definition.graphql b/crates/apollo-compiler/test_data/diagnostics/0070_self_referential_directive_definition.graphql
index 395c78e..3abb5b0 100644
--- a/crates/apollo-compiler/test_data/diagnostics/0070_self_referential_directive_definition.graphql
+++ b/crates/apollo-compiler/test_data/diagnostics/0070_self_referential_directive_definition.graphql
@@ -3,4 +3,21 @@ type Query {
   field(arg: Int @invalidExample): String
 }
 
-directive @invalidExample(arg: String @invalidExample) on ARGUMENT_DEFINITION
\ No newline at end of file
+# Simple, direct self-reference
+directive @invalidExample(arg: String @invalidExample) on ARGUMENT_DEFINITION
+
+# Some more nested self-references
+extend scalar String @deprecatedType(reason: "use OurCustomString instead")
+directive @deprecatedType(reason: String!) on OBJECT | INTERFACE | ENUM | SCALAR | UNION
+
+directive @loopA(arg: Boolean @loopB) on ARGUMENT_DEFINITION
+directive @loopB(arg: Boolean @loopC) on ARGUMENT_DEFINITION
+directive @loopC(arg: Boolean @loopA) on ARGUMENT_DEFINITION
+
+enum Enum { NON_RECURSIVE }
+extend enum Enum { RECURSIVE @wrong }
+input Name { value: String!, type: Enum }
+input InputObject { name: Name }
+directive @wrong(input: InputObject) on INPUT_FIELD_DEFINITION
+
+directive @thisOneShouldNotError(arg: Boolean @loopC) on FIELD
diff --git a/crates/apollo-compiler/test_data/diagnostics/0070_self_referential_directive_definition.txt b/crates/apollo-compiler/test_data/diagnostics/0070_self_referential_directive_definition.txt
index 038c8d2..8475d5e 100644
--- a/crates/apollo-compiler/test_data/diagnostics/0070_self_referential_directive_definition.txt
+++ b/crates/apollo-compiler/test_data/diagnostics/0070_self_referential_directive_definition.txt
@@ -8,8 +8,8 @@
             file_id: FileId {
                 id: 70,
             },
-            offset: 106,
-            length: 15,
+            offset: 100,
+            length: 77,
         },
         labels: [
             Label {
@@ -17,15 +17,220 @@
                     file_id: FileId {
                         id: 70,
                     },
-                    offset: 106,
-                    length: 15,
+                    offset: 100,
+                    length: 25,
                 },
                 text: "recursive directive definition",
             },
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 70,
+                    },
+                    offset: 138,
+                    length: 15,
+                },
+                text: "refers to itself here",
+            },
         ],
         help: None,
         data: RecursiveDefinition {
             name: "invalidExample",
         },
     },
+    ApolloDiagnostic {
+        cache: {
+            0: "built_in_types.graphql",
+            70: "0070_self_referential_directive_definition.graphql",
+        },
+        location: DiagnosticLocation {
+            file_id: FileId {
+                id: 70,
+            },
+            offset: 290,
+            length: 88,
+        },
+        labels: [
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 70,
+                    },
+                    offset: 290,
+                    length: 25,
+                },
+                text: "recursive directive definition",
+            },
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 70,
+                    },
+                    offset: 235,
+                    length: 54,
+                },
+                text: "refers to itself here",
+            },
+        ],
+        help: None,
+        data: RecursiveDefinition {
+            name: "deprecatedType",
+        },
+    },
+    ApolloDiagnostic {
+        cache: {
+            0: "built_in_types.graphql",
+            70: "0070_self_referential_directive_definition.graphql",
+        },
+        location: DiagnosticLocation {
+            file_id: FileId {
+                id: 70,
+            },
+            offset: 380,
+            length: 60,
+        },
+        labels: [
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 70,
+                    },
+                    offset: 380,
+                    length: 16,
+                },
+                text: "recursive directive definition",
+            },
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 70,
+                    },
+                    offset: 532,
+                    length: 6,
+                },
+                text: "refers to itself here",
+            },
+        ],
+        help: None,
+        data: RecursiveDefinition {
+            name: "loopA",
+        },
+    },
+    ApolloDiagnostic {
+        cache: {
+            0: "built_in_types.graphql",
+            70: "0070_self_referential_directive_definition.graphql",
+        },
+        location: DiagnosticLocation {
+            file_id: FileId {
+                id: 70,
+            },
+            offset: 441,
+            length: 60,
+        },
+        labels: [
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 70,
+                    },
+                    offset: 441,
+                    length: 16,
+                },
+                text: "recursive directive definition",
+            },
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 70,
+                    },
+                    offset: 410,
+                    length: 6,
+                },
+                text: "refers to itself here",
+            },
+        ],
+        help: None,
+        data: RecursiveDefinition {
+            name: "loopB",
+        },
+    },
+    ApolloDiagnostic {
+        cache: {
+            0: "built_in_types.graphql",
+            70: "0070_self_referential_directive_definition.graphql",
+        },
+        location: DiagnosticLocation {
+            file_id: FileId {
+                id: 70,
+            },
+            offset: 502,
+            length: 60,
+        },
+        labels: [
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 70,
+                    },
+                    offset: 502,
+                    length: 16,
+                },
+                text: "recursive directive definition",
+            },
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 70,
+                    },
+                    offset: 471,
+                    length: 6,
+                },
+                text: "refers to itself here",
+            },
+        ],
+        help: None,
+        data: RecursiveDefinition {
+            name: "loopC",
+        },
+    },
+    ApolloDiagnostic {
+        cache: {
+            0: "built_in_types.graphql",
+            70: "0070_self_referential_directive_definition.graphql",
+        },
+        location: DiagnosticLocation {
+            file_id: FileId {
+                id: 70,
+            },
+            offset: 705,
+            length: 62,
+        },
+        labels: [
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 70,
+                    },
+                    offset: 705,
+                    length: 16,
+                },
+                text: "recursive directive definition",
+            },
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 70,
+                    },
+                    offset: 621,
+                    length: 6,
+                },
+                text: "refers to itself here",
+            },
+        ],
+        help: None,
+        data: RecursiveDefinition {
+            name: "wrong",
+        },
+    },
 ]