Merge pull request #133 from NeedleInAJayStack/fix/no-unused-variables-rule

Fixes NoUnusedVariablesRule false positive

Author: NeedleInAJayStack

Sources/GraphQL/Language/AST.swift

@@ -301,7 +301,6 @@ public func == (lhs: Definition, rhs: Definition) -> Bool {
 public enum OperationType: String {
     case query
     case mutation
-    // Note: subscription is an experimental non-spec addition.
     case subscription
 }
 
@@ -949,6 +948,15 @@ public final class ObjectValue {
         self.loc = loc
         self.fields = fields
     }
+
+    public func get(key: String) -> NodeResult? {
+        switch key {
+        case "fields":
+            return .array(fields)
+        default:
+            return nil
+        }
+    }
 }
 
 extension ObjectValue: Equatable {
@@ -968,6 +976,17 @@ public final class ObjectField {
         self.name = name
         self.value = value
     }
+
+    public func get(key: String) -> NodeResult? {
+        switch key {
+        case "name":
+            return .node(name)
+        case "value":
+            return .node(value)
+        default:
+            return nil
+        }
+    }
 }
 
 extension ObjectField: Equatable {

Sources/GraphQL/Language/Kinds.swift

@@ -1,4 +1,4 @@
-public enum Kind {
+public enum Kind: CaseIterable {
     case name
     case document
     case operationDefinition

Sources/GraphQL/Language/Visitor.swift

@@ -268,7 +268,7 @@ final class Stack {
  * If a prior visitor edits a node, no following visitors will see that node.
  */
 func visitInParallel(visitors: [Visitor]) -> Visitor {
-    var skipping = [Node?](repeating: nil, count: visitors.count)
+    var skipping = [VisitResult?](repeating: nil, count: visitors.count)
 
     return Visitor(
         enter: { node, key, parent, path, ancestors in
@@ -283,9 +283,9 @@ func visitInParallel(visitors: [Visitor]) -> Visitor {
                     )
 
                     if case .skip = result {
-                        skipping[i] = node
+                        skipping[i] = .node(node)
                     } else if case .break = result {
-//                        skipping[i] = BREAK
+                        skipping[i] = .break
                     } else if case .node = result {
                         return result
                     }
@@ -306,11 +306,11 @@ func visitInParallel(visitors: [Visitor]) -> Visitor {
                     )
 
                     if case .break = result {
-//                        skipping[i] = BREAK
+                        skipping[i] = .break
                     } else if case .node = result {
                         return result
                     }
-                } // else if skipping[i] == node {
+                } // else if case let .node(skippedNode) = skipping[i], skippedNode == node {
 //                    skipping[i] = nil
 //                }
             }

Sources/GraphQL/Validation/Rules/NoUnusedVariablesRule.swift

@@ -5,38 +5,23 @@
  * are used, either directly or within a spread fragment.
  */
 func NoUnusedVariablesRule(context: ValidationContext) -> Visitor {
-    var variableDefs: [VariableDefinition] = []
-
     return Visitor(
-        enter: { node, _, _, _, _ in
-            if node is OperationDefinition {
-                variableDefs = []
-                return .continue
-            }
-
-            if let def = node as? VariableDefinition {
-                variableDefs.append(def)
-                return .continue
-            }
-
-            return .continue
+        enter: { _, _, _, _, _ in
+            .continue
         },
         leave: { node, _, _, _, _ -> VisitResult in
             guard let operation = node as? OperationDefinition else {
                 return .continue
             }
 
-            var variableNameUsed: [String: Bool] = [:]
             let usages = context.getRecursiveVariableUsages(operation: operation)
+            let variableNameUsed = Set(usages.map { usage in
+                usage.node.name.value
+            })
 
-            for usage in usages {
-                variableNameUsed[usage.node.name.value] = true
-            }
-
-            for variableDef in variableDefs {
+            for variableDef in operation.variableDefinitions {
                 let variableName = variableDef.variable.name.value
-
-                if variableNameUsed[variableName] != true {
+                if !variableNameUsed.contains(variableName) {
                     context.report(
                         error: GraphQLError(
                             message: operation.name.map {
@@ -47,7 +32,6 @@ func NoUnusedVariablesRule(context: ValidationContext) -> Visitor {
                     )
                 }
             }
-
             return .continue
         }
     )

Sources/GraphQL/Validation/Validate.swift

@@ -240,11 +240,10 @@ public final class ValidationContext {
                     }
 
                     if let variable = node as? Variable {
-                        usages
-                            .append(VariableUsage(
-                                node: variable,
-                                type: typeInfo.inputType
-                            ))
+                        usages.append(VariableUsage(
+                            node: variable,
+                            type: typeInfo.inputType
+                        ))
                     }
 
                     return .continue

Tests/GraphQLTests/ValidationTests/NoUnusedVariablesRuleTests.swift

@@ -256,4 +256,30 @@ class NoUnusedVariablesRuleTests: ValidationTestCase {
             message: #"Variable "$a" is never used in operation "Bar"."#
         )
     }
+
+    func testVariableUsedInsideObject() throws {
+        try assertValid(
+            """
+            query Foo($a: String) {
+              field(object: { a: $a })
+            }
+            """
+        )
+    }
+
+    func testVariableUnusedInsideObject() throws {
+        let errors = try assertInvalid(
+            errorCount: 1,
+            query: """
+            query Foo($a: String, $b: String) {
+              field(object: { a: $a })
+            }
+            """
+        )
+
+        try assertValidationError(
+            error: errors[0], line: 1, column: 23,
+            message: #"Variable "$b" is never used in operation "Foo"."#
+        )
+    }
 }