remove additional labels from error

Author: giacomocavalieri

compiler-core/src/ast/typed.rs

@@ -1033,9 +1033,11 @@ impl TypedExpr {
         }
     }
 
-    pub fn var_constructor(&self) -> Option<&ValueConstructor> {
+    pub fn var_constructor(&self) -> Option<(&ValueConstructor, &EcoString)> {
         match self {
-            TypedExpr::Var { constructor, .. } => Some(constructor),
+            TypedExpr::Var {
+                constructor, name, ..
+            } => Some((constructor, name)),
             _ => None,
         }
     }

compiler-core/src/type_/environment.rs

@@ -78,17 +78,7 @@ pub struct Environment<'a> {
     /// local_variable_usages is a stack of scopes. When a local variable is created it is
     /// added to the top scope. When a local variable is used we crawl down the scope
     /// stack for a variable with that name and mark it as used.
-    ///
-    /// NOTE: The usize in the tuple here tracks the number of times the variable has
-    ///       been used.
-    pub local_variable_usages: Vec<HashMap<EcoString, (VariableOrigin, SrcSpan, usize)>>,
-
-    /// Used to determine if an argument is only used unchanged in recursive
-    /// calls, and could thus be removed.
-    /// For each of the function's arguments keeps track of the usages in
-    /// recursive calls.
-    ///
-    pub recursive_argument_usages: HashMap<SrcSpan, Vec<SrcSpan>>,
+    pub local_variable_usages: Vec<HashMap<EcoString, VariableUsage>>,
 
     /// Used to determine if all functions/constants need to support the current
     /// compilation target.
@@ -104,6 +94,14 @@ pub struct Environment<'a> {
     pub dev_dependencies: &'a HashSet<EcoString>,
 }
 
+#[derive(Debug)]
+pub struct VariableUsage {
+    origin: VariableOrigin,
+    location: SrcSpan,
+    usages: usize,
+    used_recursively: bool,
+}
+
 impl<'a> Environment<'a> {
     pub fn new(
         EnvironmentArguments {
@@ -143,7 +141,6 @@ impl<'a> Environment<'a> {
             importable_modules,
             current_module,
             local_variable_usages: vec![HashMap::new()],
-            recursive_argument_usages: HashMap::new(),
             target_support,
             names,
             module_type_aliases: HashMap::new(),
@@ -668,30 +665,63 @@ impl Environment<'_> {
         location: SrcSpan,
         problems: &mut Problems,
     ) {
-        if let Some((origin, location, 0)) = self
+        if let Some(VariableUsage {
+            origin,
+            location,
+            usages: 0,
+            used_recursively,
+        }) = self
             .local_variable_usages
             .last_mut()
             .expect("Attempted to access non-existent entity usages scope")
-            .insert(name.clone(), (origin, location, 0))
+            .insert(
+                name.clone(),
+                VariableUsage {
+                    origin,
+                    location,
+                    usages: 0,
+                    used_recursively: false,
+                },
+            )
         {
             // an entity was overwritten in the top most scope without being used
             let mut unused = HashMap::with_capacity(1);
-            let _ = unused.insert(name, (origin, location, 0));
+            let _ = unused.insert(
+                name,
+                VariableUsage {
+                    origin,
+                    location,
+                    usages: 0,
+                    used_recursively,
+                },
+            );
             self.handle_unused_variables(unused, problems);
         }
     }
 
     /// Increments an entity's usage in the current or nearest enclosing scope
     pub fn increment_usage(&mut self, name: &EcoString) {
-        let name = name.clone();
+        if let Some(VariableUsage { usages, .. }) = self
+            .local_variable_usages
+            .iter_mut()
+            .rev()
+            .find_map(|scope| scope.get_mut(name))
+        {
+            *usages += 1;
+        }
+    }
 
-        if let Some((_, _, used)) = self
+    /// Marks an argument as being passed recursively to a function call.
+    pub fn set_used_recursively(&mut self, name: &EcoString) {
+        if let Some(VariableUsage {
+            used_recursively, ..
+        }) = self
             .local_variable_usages
             .iter_mut()
             .rev()
-            .find_map(|scope| scope.get_mut(&name))
+            .find_map(|scope| scope.get_mut(name))
         {
-            *used += 1;
+            *used_recursively = true;
         }
     }
 
@@ -805,24 +835,24 @@ impl Environment<'_> {
 
     fn handle_unused_variables(
         &mut self,
-        unused: HashMap<EcoString, (VariableOrigin, SrcSpan, usize)>,
+        unused: HashMap<EcoString, VariableUsage>,
         problems: &mut Problems,
     ) {
-        for (origin, location, usages) in unused.into_values() {
+        for VariableUsage {
+            origin,
+            location,
+            usages,
+            used_recursively,
+        } in unused.into_values()
+        {
             if usages == 0 {
                 problems.warning(Warning::UnusedVariable { location, origin });
             }
             // If the function parameter is actually used somewhere, but all the
             // usages are just passing it along in a recursive call, then it
             // counts as being unused too.
-            else if origin.is_function_parameter()
-                && let Some(recursive_usages) = self.recursive_argument_usages.get(&location)
-                && recursive_usages.len() == usages
-            {
-                problems.warning(Warning::UnusedRecursiveArgument {
-                    location,
-                    usages: recursive_usages.clone(),
-                });
+            else if origin.is_function_parameter() && used_recursively {
+                problems.warning(Warning::UnusedRecursiveArgument { location });
             }
         }
     }

compiler-core/src/type_/error.rs

@@ -1110,7 +1110,6 @@ pub enum Warning {
     ///
     UnusedRecursiveArgument {
         location: SrcSpan,
-        usages: Vec<SrcSpan>,
     },
 }
 

compiler-core/src/type_/expression.rs

@@ -4467,15 +4467,10 @@ impl<'a, 'b> ExprTyper<'a, 'b> {
                 // If the argument is a simple usage of a parameter defined
                 // by this same function we want to track it, to report unused
                 // parameters that are just passed along the recursive calls.
-                if let Some(definition_location) =
+                if let Some(name) =
                     self.argument_used_in_recursive_call(fun, &inferred, argument_index)
                 {
-                    let _ = self
-                        .environment
-                        .recursive_argument_usages
-                        .entry(definition_location)
-                        .and_modify(|usages| usages.push(inferred.location()))
-                        .or_insert(vec![inferred.location()]);
+                    self.environment.set_used_recursively(&name);
                 }
                 inferred
             }
@@ -4506,20 +4501,20 @@ impl<'a, 'b> ExprTyper<'a, 'b> {
         called_function: &TypedExpr,
         argument: &TypedExpr,
         argument_index: usize,
-    ) -> Option<SrcSpan> {
-        let argument = argument.var_constructor()?;
+    ) -> Option<EcoString> {
+        let (argument, name) = argument.var_constructor()?;
         let (function_defining_argument, index_in_argument_list) =
             argument.variant.argument_function_and_index()?;
 
         let (module, function) = called_function
             .var_constructor()
-            .and_then(|constructor| constructor.variant.function_module_and_name())?;
+            .and_then(|(constructor, _)| constructor.variant.function_module_and_name())?;
 
         if self.environment.current_module == module
             && function == function_defining_argument
             && argument_index == index_in_argument_list
         {
-            Some(argument.definition_location().span)
+            Some(name.clone())
         } else {
             None
         }

compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__warnings__unused_recursive_function_argument.snap

@@ -15,8 +15,6 @@ warning: Unused function argument
   │
 2 │ pub fn main(x: Int) {
   │             ^ This argument is unused
-3 │   main(x)
-  │        -
 
 This argument is passed to the function when recursing, but it's never used
 for anything.

compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__warnings__unused_recursive_function_argument_2.snap

@@ -18,11 +18,6 @@ warning: Unused function argument
   │
 2 │ pub fn main(x, times) {
   │             ^ This argument is unused
-3 │   case times {
-4 │     0 -> main(x, 0)
-  │               -
-5 │     _ -> main(x, times - 1)
-  │               -
 
 This argument is passed to the function when recursing, but it's never used
 for anything.

compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__warnings__unused_recursive_function_inside_anonymous_function.snap

@@ -16,8 +16,6 @@ warning: Unused function argument
   │
 2 │ pub fn main(x: Int) {
   │             ^ This argument is unused
-3 │   let _useless = fn(_) { main(x) }
-  │                               -
 
 This argument is passed to the function when recursing, but it's never used
 for anything.

compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__warnings__unused_recursive_function_with_shadowing.snap

@@ -16,9 +16,6 @@ warning: Unused function argument
   │
 2 │ pub fn main(x: Int) {
   │             ^ This argument is unused
-3 │   let _useless = fn(x) { main(x) }
-4 │   main(x)
-  │        -
 
 This argument is passed to the function when recursing, but it's never used
 for anything.

compiler-core/src/warning.rs

@@ -719,7 +719,7 @@ Hint: You can safely remove it.
                     }),
                 },
 
-                type_::Warning::UnusedRecursiveArgument { location, usages } => Diagnostic {
+                type_::Warning::UnusedRecursiveArgument { location } => Diagnostic {
                     title: "Unused function argument".into(),
                     text: wrap(
                         "This argument is passed to the function when recursing, \
@@ -734,16 +734,7 @@ but it's never used for anything.",
                             text: Some("This argument is unused".into()),
                             span: *location,
                         },
-                        extra_labels: usages
-                            .iter()
-                            .map(|usage| ExtraLabel {
-                                src_info: None,
-                                label: diagnostic::Label {
-                                    text: None,
-                                    span: *usage,
-                                },
-                            })
-                            .collect(),
+                        extra_labels: vec![],
                     }),
                 },