Skip to content

Fixes NoUnusedVariablesRule false positive #133

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion Sources/GraphQL/Language/AST.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion Sources/GraphQL/Language/Kinds.swift
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
public enum Kind {
public enum Kind: CaseIterable {
case name
case document
case operationDefinition
Expand Down
10 changes: 5 additions & 5 deletions Sources/GraphQL/Language/Visitor.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
}
Expand All @@ -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
// }
}
Expand Down
30 changes: 7 additions & 23 deletions Sources/GraphQL/Validation/Rules/NoUnusedVariablesRule.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -47,7 +32,6 @@ func NoUnusedVariablesRule(context: ValidationContext) -> Visitor {
)
}
}

return .continue
}
)
Expand Down
9 changes: 4 additions & 5 deletions Sources/GraphQL/Validation/Validate.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"."#
)
}
}