raise warning for function arguments used only in recursive calls

Author: giacomocavalieri

CHANGELOG.md

@@ -63,6 +63,39 @@
 
   ([Giacomo Cavalieri](https://github.com/giacomocavalieri))
 
+- The compiler now raises a warning when a function's argument is only passed
+  along in a recursive call but not actually used for anything. For example:
+
+  ```gleam
+  import gleam/io
+
+  pub fn greet(x, times) {
+    case times {
+      0 -> Nil
+      _ -> {
+        io.println("Hello, Joe!")
+        greet(x, times - 1)
+      }
+    }
+  }
+  ```
+
+  In this piece of code the `x` argument is actually never used, and the
+  compiler will raise the following warning:
+
+  ```text
+  warning: Unused function argument
+    ┌─ /Users/giacomocavalieri/Desktop/prova/src/prova.gleam:3:14
+    │
+  3 │ pub fn greet(x, times) {
+    │              ^ This argument is unused
+
+  This argument is passed to the function when recursing, but it's never used
+  for anything.
+  ```
+
+  ([Giacomo Cavalieri](https://github.com/giacomocavalieri))
+
 - The compiler now emits a better error message for private types marked as
   opaque. For example, the following piece of code:
 

compiler-core/src/analyse.rs

@@ -599,6 +599,7 @@ impl<'a, A> ModuleAnalyzer<'a, A> {
                 .expect("Could not find hydrator for fn");
 
             let (arguments, body) = expr_typer.infer_fn_with_known_types(
+                Some(name.clone()),
                 typed_arguments.clone(),
                 body,
                 Some(prereg_return_type.clone()),

compiler-core/src/ast/typed.rs

@@ -1033,6 +1033,13 @@ impl TypedExpr {
         }
     }
 
+    pub fn var_constructor(&self) -> Option<&ValueConstructor> {
+        match self {
+            TypedExpr::Var { constructor, .. } => Some(constructor),
+            _ => None,
+        }
+    }
+
     #[must_use]
     pub(crate) fn is_panic(&self) -> bool {
         match self {

compiler-core/src/language_server/code_action.rs

@@ -6236,7 +6236,7 @@ impl<'ast> ast::visit::Visit<'ast> for InlineVariable<'ast> {
             VariableDeclaration::LetPattern => {}
             VariableDeclaration::UsePattern
             | VariableDeclaration::ClausePattern
-            | VariableDeclaration::FunctionParameter
+            | VariableDeclaration::FunctionParameter { .. }
             | VariableDeclaration::Generated => return,
         }
 
@@ -6256,7 +6256,7 @@ impl<'ast> ast::visit::Visit<'ast> for InlineVariable<'ast> {
             VariableDeclaration::LetPattern => {}
             VariableDeclaration::UsePattern
             | VariableDeclaration::ClausePattern
-            | VariableDeclaration::FunctionParameter
+            | VariableDeclaration::FunctionParameter { .. }
             | VariableDeclaration::Generated => return,
         }
 

compiler-core/src/type_.rs

@@ -877,6 +877,49 @@ impl ValueConstructorVariant {
             ValueConstructorVariant::Record { field_map, .. } => field_map.as_ref(),
         }
     }
+
+    /// If this is the constructor of a module function, returns the function's
+    /// module and name.
+    ///
+    fn function_module_and_name(&self) -> Option<(EcoString, EcoString)> {
+        match self {
+            ValueConstructorVariant::ModuleFn { name, module, .. } => {
+                Some((module.clone(), name.clone()))
+            }
+
+            ValueConstructorVariant::ModuleConstant { .. }
+            | ValueConstructorVariant::LocalConstant { .. }
+            | ValueConstructorVariant::LocalVariable { .. }
+            | ValueConstructorVariant::Record { .. } => None,
+        }
+    }
+
+    /// If this is a constructor for an argument, returns the name of the
+    /// function where it is defined and its index in the function's parameter
+    /// list.
+    ///
+    fn argument_function_and_index(&self) -> Option<(EcoString, usize)> {
+        match self {
+            ValueConstructorVariant::LocalVariable {
+                location: _,
+                origin:
+                    VariableOrigin {
+                        syntax: _,
+                        declaration:
+                            VariableDeclaration::FunctionParameter {
+                                function_name,
+                                index,
+                            },
+                    },
+            } => Some((function_name.clone()?, *index)),
+
+            ValueConstructorVariant::LocalVariable { .. }
+            | ValueConstructorVariant::ModuleConstant { .. }
+            | ValueConstructorVariant::LocalConstant { .. }
+            | ValueConstructorVariant::ModuleFn { .. }
+            | ValueConstructorVariant::Record { .. } => None,
+        }
+    }
 }
 
 #[derive(Debug, Clone, PartialEq, Eq)]

compiler-core/src/type_/environment.rs

@@ -78,8 +78,17 @@ 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 bool in the tuple here tracks if the variable has been used
-    pub local_variable_usages: Vec<HashMap<EcoString, (VariableOrigin, SrcSpan, bool)>>,
+    ///
+    /// 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>>,
 
     /// Used to determine if all functions/constants need to support the current
     /// compilation target.
@@ -134,6 +143,7 @@ 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(),
@@ -658,15 +668,15 @@ impl Environment<'_> {
         location: SrcSpan,
         problems: &mut Problems,
     ) {
-        if let Some((origin, location, false)) = self
+        if let Some((origin, location, 0)) = self
             .local_variable_usages
             .last_mut()
             .expect("Attempted to access non-existent entity usages scope")
-            .insert(name.clone(), (origin, location, false))
+            .insert(name.clone(), (origin, location, 0))
         {
             // 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, false));
+            let _ = unused.insert(name, (origin, location, 0));
             self.handle_unused_variables(unused, problems);
         }
     }
@@ -681,7 +691,7 @@ impl Environment<'_> {
             .rev()
             .find_map(|scope| scope.get_mut(&name))
         {
-            *used = true;
+            *used += 1;
         }
     }
 
@@ -795,13 +805,25 @@ impl Environment<'_> {
 
     fn handle_unused_variables(
         &mut self,
-        unused: HashMap<EcoString, (VariableOrigin, SrcSpan, bool)>,
+        unused: HashMap<EcoString, (VariableOrigin, SrcSpan, usize)>,
         problems: &mut Problems,
     ) {
-        for (origin, location, used) in unused.into_values() {
-            if !used {
+        for (origin, location, usages) 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(),
+                });
+            }
         }
     }
 

compiler-core/src/type_/error.rs

@@ -761,13 +761,21 @@ pub enum VariableSyntax {
     Generated,
 }
 
-#[derive(Debug, Clone, Copy, PartialEq, Eq, serde::Serialize, serde::Deserialize)]
+#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize)]
 /// The source of a variable, such as a `let` assignment, or function parameter.
 pub enum VariableDeclaration {
     LetPattern,
     UsePattern,
     ClausePattern,
-    FunctionParameter,
+    FunctionParameter {
+        /// The name of the function defining the parameter. This will be None
+        /// for parameters introduced by anoynmous functions: `fn(a) { a }`
+        ///
+        function_name: Option<EcoString>,
+        /// The index of the parameter in the function's parameter list.
+        ///
+        index: usize,
+    },
     Generated,
 }
 
@@ -791,6 +799,17 @@ impl VariableOrigin {
             declaration: VariableDeclaration::Generated,
         }
     }
+
+    pub fn is_function_parameter(&self) -> bool {
+        match self.declaration {
+            VariableDeclaration::FunctionParameter { .. } => true,
+
+            VariableDeclaration::LetPattern
+            | VariableDeclaration::UsePattern
+            | VariableDeclaration::ClausePattern
+            | VariableDeclaration::Generated => false,
+        }
+    }
 }
 
 #[derive(Debug, Eq, PartialEq, Clone, serde::Serialize, serde::Deserialize)]
@@ -1075,6 +1094,24 @@ pub enum Warning {
         location: SrcSpan,
         outcome: ComparisonOutcome,
     },
+    /// When a function's argument is only ever used unchanged in recursive
+    /// calls. For example:
+    ///
+    /// ```gleam
+    /// pub fn wibble(x, n) {
+    /// //            ^ This argument is not needed,
+    ///   case n {
+    ///     0 -> Nil
+    ///     _ -> wibble(x, n - 1)
+    /// //              ^ It's only used in recursive calls!
+    ///   }
+    /// }
+    /// ```
+    ///
+    UnusedRecursiveArgument {
+        location: SrcSpan,
+        usages: Vec<SrcSpan>,
+    },
 }
 
 #[derive(Debug, Eq, PartialEq, Clone, serde::Serialize, serde::Deserialize)]
@@ -1333,7 +1370,8 @@ impl Warning {
             | Warning::ModuleImportedTwice {
                 second: location, ..
             }
-            | Warning::RedundantComparison { location, .. } => *location,
+            | Warning::RedundantComparison { location, .. }
+            | Warning::UnusedRecursiveArgument { location, .. } => *location,
         }
     }