From 172f75daee08fae9d6aa544d1fff61b3e897bc3c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= <renee.kooi@apollographql.com>
Date: Fri, 19 Jul 2024 17:50:21 +0200
Subject: [PATCH] chore(compiler): make unused variable validation more
 efficient

The validation used a hash map and two hash sets. We can do the exact
same validation with only a hash map, by first putting all declared variables
in the hash map, and removing variables as we see them used. Then at the end we
only have unused variables in the map.
---
 .../src/validation/variable.rs                | 30 ++++++++++++-------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/crates/apollo-compiler/src/validation/variable.rs b/crates/apollo-compiler/src/validation/variable.rs
index 5cf0d0cd6..0ce12b5e5 100644
--- a/crates/apollo-compiler/src/validation/variable.rs
+++ b/crates/apollo-compiler/src/validation/variable.rs
@@ -1,6 +1,5 @@
 use crate::ast;
 use crate::collections::HashMap;
-use crate::collections::HashSet;
 use crate::executable;
 use crate::validation::diagnostics::DiagnosticData;
 use crate::validation::DiagnosticList;
@@ -167,8 +166,8 @@ pub(crate) fn validate_unused_variables(
     document: &ExecutableDocument,
     operation: &executable::Operation,
 ) {
-    let defined_vars: HashSet<_> = operation.variables.iter().map(|var| &var.name).collect();
-    let locations: HashMap<_, _> = operation
+    // Start off by considering all variables unused: names are removed from this as we find them.
+    let mut unused_vars: HashMap<_, _> = operation
         .variables
         .iter()
         .map(|var| {
@@ -178,23 +177,32 @@ pub(crate) fn validate_unused_variables(
             )
         })
         .collect();
-    let mut used_vars = HashSet::default();
     let walked = walk_selections(
         document,
         &operation.selection_set,
         |selection| match selection {
             executable::Selection::Field(field) => {
-                used_vars.extend(variables_in_directives(&field.directives));
-                used_vars.extend(variables_in_arguments(&field.arguments));
+                for used in variables_in_directives(&field.directives) {
+                    unused_vars.remove(used);
+                }
+                for used in variables_in_arguments(&field.arguments) {
+                    unused_vars.remove(used);
+                }
             }
             executable::Selection::FragmentSpread(fragment) => {
                 if let Some(fragment_def) = document.fragments.get(&fragment.fragment_name) {
-                    used_vars.extend(variables_in_directives(&fragment_def.directives));
+                    for used in variables_in_directives(&fragment_def.directives) {
+                        unused_vars.remove(used);
+                    }
+                }
+                for used in variables_in_directives(&fragment.directives) {
+                    unused_vars.remove(used);
                 }
-                used_vars.extend(variables_in_directives(&fragment.directives));
             }
             executable::Selection::InlineFragment(fragment) => {
-                used_vars.extend(variables_in_directives(&fragment.directives));
+                for used in variables_in_directives(&fragment.directives) {
+                    unused_vars.remove(used);
+                }
             }
         },
     );
@@ -203,9 +211,9 @@ pub(crate) fn validate_unused_variables(
         return;
     }
 
-    for &unused_var in defined_vars.difference(&used_vars) {
+    for (unused_var, location) in unused_vars {
         diagnostics.push(
-            locations[unused_var],
+            location,
             DiagnosticData::UnusedVariable {
                 name: unused_var.clone(),
             },