change implementation

Author: giacomocavalieri

compiler-core/src/type_.rs

@@ -877,49 +877,6 @@ 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

@@ -99,7 +99,7 @@ pub struct VariableUsage {
     origin: VariableOrigin,
     location: SrcSpan,
     usages: usize,
-    used_recursively: bool,
+    recursive_usages: usize,
 }
 
 impl<'a> Environment<'a> {
@@ -669,7 +669,7 @@ impl Environment<'_> {
             origin,
             location,
             usages: 0,
-            used_recursively,
+            recursive_usages,
         }) = self
             .local_variable_usages
             .last_mut()
@@ -680,7 +680,7 @@ impl Environment<'_> {
                     origin,
                     location,
                     usages: 0,
-                    used_recursively: false,
+                    recursive_usages: 0,
                 },
             )
         {
@@ -692,7 +692,7 @@ impl Environment<'_> {
                     origin,
                     location,
                     usages: 0,
-                    used_recursively,
+                    recursive_usages,
                 },
             );
             self.handle_unused_variables(unused, problems);
@@ -712,16 +712,16 @@ impl Environment<'_> {
     }
 
     /// Marks an argument as being passed recursively to a function call.
-    pub fn set_used_recursively(&mut self, name: &EcoString) {
+    pub fn increment_recursive_usage(&mut self, name: &EcoString) {
         if let Some(VariableUsage {
-            used_recursively, ..
+            recursive_usages, ..
         }) = self
             .local_variable_usages
             .iter_mut()
             .rev()
             .find_map(|scope| scope.get_mut(name))
         {
-            *used_recursively = true;
+            *recursive_usages += 1;
         }
     }
 
@@ -842,7 +842,7 @@ impl Environment<'_> {
             origin,
             location,
             usages,
-            used_recursively,
+            recursive_usages,
         } in unused.into_values()
         {
             if usages == 0 {
@@ -851,7 +851,7 @@ impl Environment<'_> {
             // 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() && used_recursively {
+            else if origin.is_function_parameter() && recursive_usages == usages {
                 problems.warning(Warning::UnusedRecursiveArgument { location });
             }
         }

compiler-core/src/type_/expression.rs

@@ -3586,6 +3586,8 @@ impl<'a, 'b> ExprTyper<'a, 'b> {
             deprecation,
         } = constructor;
 
+        self.check_recursive_argument_usage(name, &variant, &register_reference);
+
         // Emit a warning if the value being used is deprecated.
         if let Deprecation::Deprecated { message } = &deprecation {
             self.problems.warning(Warning::DeprecatedItem {
@@ -3597,20 +3599,22 @@ impl<'a, 'b> ExprTyper<'a, 'b> {
 
         self.narrow_implementations(*location, &variant)?;
 
-        if matches!(
-            register_reference,
+        match register_reference {
+            ReferenceRegistration::DoNotRegisterReferences => (),
+
             ReferenceRegistration::RegisterReferences
-        ) {
-            self.register_value_constructor_reference(
-                name,
-                &variant,
-                *location,
-                if module.is_some() {
-                    ReferenceKind::Qualified
-                } else {
-                    ReferenceKind::Unqualified
-                },
-            );
+            | ReferenceRegistration::VariableArgumentReferences { .. } => {
+                self.register_value_constructor_reference(
+                    name,
+                    &variant,
+                    *location,
+                    if module.is_some() {
+                        ReferenceKind::Qualified
+                    } else {
+                        ReferenceKind::Unqualified
+                    },
+                );
+            }
         }
 
         // Instantiate generic variables into unbound variables for this usage
@@ -3623,6 +3627,43 @@ impl<'a, 'b> ExprTyper<'a, 'b> {
         })
     }
 
+    fn check_recursive_argument_usage(
+        &mut self,
+        name: &EcoString,
+        variant: &ValueConstructorVariant,
+        register_reference: &ReferenceRegistration,
+    ) {
+        // If we are registering references for a call argument
+        let ReferenceRegistration::VariableArgumentReferences {
+            called_function,
+            argument_index,
+        } = register_reference
+        else {
+            return;
+        };
+
+        // If the passed argument is a function's parameter.
+        let ValueConstructorVariant::LocalVariable { origin, .. } = variant else {
+            return;
+        };
+
+        let VariableDeclaration::FunctionParameter {
+            function_name: declaration_function,
+            index: declaration_index,
+        } = &origin.declaration
+        else {
+            return;
+        };
+
+        // If the called function is the same where the argument is defined,
+        // and the argument is passed unchanged.
+        if declaration_function.as_ref() == dbg!(Some(called_function))
+            && declaration_index == argument_index
+        {
+            self.environment.increment_recursive_usage(name);
+        }
+    }
+
     fn register_value_constructor_reference(
         &mut self,
         referenced_name: &EcoString,
@@ -4423,14 +4464,14 @@ impl<'a, 'b> ExprTyper<'a, 'b> {
 
     fn infer_call_argument(
         &mut self,
-        fun: &TypedExpr,
-        value: UntypedExpr,
+        called_function: &TypedExpr,
+        argument: UntypedExpr,
         argument_index: usize,
         type_: Arc<Type>,
         kind: ArgumentKind,
     ) -> TypedExpr {
         let type_ = collapse_links(type_);
-        let value = match (&*type_, value) {
+        let value = match (&*type_, argument) {
             // If the argument is expected to be a function and we are passed a
             // function literal with the correct number of arguments then we
             // have special handling of this argument, passing in information
@@ -4460,20 +4501,13 @@ impl<'a, 'b> ExprTyper<'a, 'b> {
                 location,
             ),
 
-            // Otherwise just perform normal type inference.
-            (_, value) => {
-                let inferred = self.infer(value);
-
-                // 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(name) =
-                    self.argument_used_in_recursive_call(fun, &inferred, argument_index)
-                {
-                    self.environment.set_used_recursively(&name);
-                }
-                inferred
+            // If the argument is a regular var then we specialise.
+            (_, UntypedExpr::Var { location, name }) => {
+                self.infer_variable_call_arg(called_function, name, location, argument_index)
             }
+
+            // Otherwise just perform normal type inference.
+            (_, argument) => self.infer(argument),
         };
 
         if let Err(error) = unify(type_.clone(), value.type_()) {
@@ -4484,39 +4518,41 @@ impl<'a, 'b> ExprTyper<'a, 'b> {
         value
     }
 
-    /// Given a function being called and one of its arguments, this returns the
-    /// argument's definition location if it was defined by that same function.
-    /// For example:
-    ///
-    /// ```gleam
-    /// fn wibble(x) {
-    /// //        ^ Defined by the wibble function
-    ///   wibble(x, 1)
-    /// //       ^ And used in the recursive call unchanged!
-    /// }
-    /// ```
-    ///
-    fn argument_used_in_recursive_call(
-        &self,
+    fn infer_variable_call_arg(
+        &mut self,
         called_function: &TypedExpr,
-        argument: &TypedExpr,
+        argument_name: EcoString,
+        argument_location: SrcSpan,
         argument_index: usize,
-    ) -> 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())?;
-
-        if self.environment.current_module == module
-            && function == function_defining_argument
-            && argument_index == index_in_argument_list
+    ) -> TypedExpr {
+        // If the called function is a function defined in this same module we
+        // pass it along to the `infer_var` function so that we can check if the
+        // argument is being passed recursively to the function that is defining
+        // it.
+        let references = if let TypedExpr::Var {
+            constructor:
+                ValueConstructor {
+                    variant: ValueConstructorVariant::ModuleFn { name, module, .. },
+                    ..
+                },
+            ..
+        } = called_function
+            && *module == self.environment.current_module
         {
-            Some(name.clone())
+            ReferenceRegistration::VariableArgumentReferences {
+                called_function: name.clone(),
+                argument_index,
+            }
         } else {
-            None
+            ReferenceRegistration::RegisterReferences
+        };
+
+        match self.infer_var(argument_name, argument_location, references) {
+            Ok(result) => result,
+            Err(error) => {
+                self.problems.error(error);
+                self.error_expr(argument_location)
+            }
         }
     }
 
@@ -4785,10 +4821,18 @@ fn is_trusted_pure_module(environment: &Environment<'_>) -> bool {
     environment.origin == Origin::Src
 }
 
-#[derive(Debug, Clone, Copy)]
+#[derive(Debug, Clone)]
 enum ReferenceRegistration {
     RegisterReferences,
     DoNotRegisterReferences,
+
+    /// If the variable is being passed to a function that is defined in the
+    /// current module, then this will have the name of the function and the
+    /// argument of the variable being passed as an argument
+    VariableArgumentReferences {
+        called_function: EcoString,
+        argument_index: usize,
+    },
 }
 
 fn extract_typed_use_call_assignments(

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

@@ -14,7 +14,7 @@ warning: Unused function argument
   ┌─ /src/warning/wrn.gleam:2:13
   │
 2 │ pub fn main(x: Int) {
-  │             ^ This argument is unused
+  │             ^ This argument is never used
 
 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

@@ -17,7 +17,7 @@ warning: Unused function argument
   ┌─ /src/warning/wrn.gleam:2:13
   │
 2 │ pub fn main(x, times) {
-  │             ^ This argument is unused
+  │             ^ This argument is never used
 
 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

@@ -15,7 +15,7 @@ warning: Unused function argument
   ┌─ /src/warning/wrn.gleam:2:13
   │
 2 │ pub fn main(x: Int) {
-  │             ^ This argument is unused
+  │             ^ This argument is never used
 
 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

@@ -15,7 +15,7 @@ warning: Unused function argument
   ┌─ /src/warning/wrn.gleam:2:13
   │
 2 │ pub fn main(x: Int) {
-  │             ^ This argument is unused
+  │             ^ This argument is never used
 
 This argument is passed to the function when recursing, but it's never used
 for anything.

compiler-core/src/warning.rs

@@ -731,7 +731,7 @@ but it's never used for anything.",
                         src: src.clone(),
                         path: path.to_path_buf(),
                         label: diagnostic::Label {
-                            text: Some("This argument is unused".into()),
+                            text: Some("This argument is never used".into()),
                             span: *location,
                         },
                         extra_labels: vec![],