Implement lpil suggestions after review.

Rename all terms related to variables: variable / functions, Var,
identifiers to value(s) in code, comment and CHANGELOG.md

Rename the new field in `Error::UnknownVariable` from
`imported_modules_with_same_public_variable_name` to `possible_modules`.
The field name was too long.

Change the fold pattern with immutable accumulator to a for loop
mutating a variable.

Rewrite the compiler message to a "Did you mean" sentence.

Author: realraphrous

compiler-core/src/error.rs

@@ -2402,7 +2402,7 @@ but no type in scope with that name."
                         discarded_location,
                         name,
                         type_with_name_in_scope,
-                        imported_modules_with_same_public_variable_name,
+                        possible_modules,
                     } => {
                         let title = String::from("Unknown variable");
                         if let Some(ignored_location) = discarded_location {
@@ -2443,20 +2443,15 @@ but no type in scope with that name."
                                     wrap_format!("The name `{name}` is not in scope here.")
                                 };
 
-                                // If there are some suggestions about variables in imported modules
-                                // put a "consider" text after the main message
-                                if !imported_modules_with_same_public_variable_name.is_empty() {
-                                    let consider_text = imported_modules_with_same_public_variable_name
-                                        .iter()
-                                        .fold(
-                                            String::from("Consider using one of these variables:\n\n"),
-                                            |mut acc, module_name| {
-                                                acc.push_str(&wrap_format!("    {module_name}.{name}\n"));
-                                                acc
-                                            }
-                                        );
+                                // If there are some suggestions about public values in imported
+                                // modules put a "did you mean" text after the main message
+                                if !possible_modules.is_empty() {
+                                    let mut did_you_mean_text = String::from("Did you mean one of these:\n\n");
+                                    for module_name in possible_modules {
+                                        did_you_mean_text.push_str(&format!("  - {module_name}.{name}\n"));
+                                    }
                                     text.push('\n');
-                                    text.push_str(&consider_text);
+                                    text.push_str(&did_you_mean_text);
                                 }
 
                                 text

compiler-core/src/type_.rs

@@ -1103,6 +1103,15 @@ impl ModuleInterface {
         }
     }
 
+    pub fn get_public_function(&self, name: &str, arity: usize) -> Option<&ValueConstructor> {
+        self.get_public_value(name).filter(|value_constructor| {
+            match value_constructor.type_.fn_types() {
+                Some((fn_arguments_type, _)) => fn_arguments_type.len() == arity,
+                None => false,
+            }
+        })
+    }
+
     pub fn get_public_type(&self, name: &str) -> Option<&TypeConstructor> {
         let type_ = self.types.get(name)?;
         if type_.publicity.is_importable() {
@@ -1680,14 +1689,11 @@ pub enum FieldAccessUsage {
     Other,
 }
 
-/// This is used to know when a variable is used as a value or as a call:
-/// function call, a pipeline or a custom type variant constructor
+/// This is used to know when a value is used as a call or not.
 #[derive(Debug, Copy, Clone, PartialEq, Eq)]
-pub enum VarUsage {
-    /// Used as `call()` or `left |> right`
+pub enum ValueUsage {
+    /// Used as `call(..)`, `Type(..)`, `left |> right` or `left |> right(..)`
     Call { arity: usize },
-    /// Used as `left |> right(..)`
-    PipelineCall,
     /// Used as `variable`
     Other,
 }

compiler-core/src/type_/environment.rs

@@ -488,7 +488,7 @@ impl Environment<'_> {
                     name: name.clone(),
                     variables: self.local_value_names(),
                     type_with_name_in_scope: self.module_types.keys().any(|typ| typ == name),
-                    imported_modules_with_same_public_variable_name: self
+                    possible_modules: self
                         .imported_modules
                         .iter()
                         .filter_map(|(module_name, (_, module))| {

compiler-core/src/type_/error.rs

@@ -167,7 +167,7 @@ pub enum Error {
         type_with_name_in_scope: bool,
         /// Filled with the name of imported modules when the module has public value
         /// with the same name as this variable
-        imported_modules_with_same_public_variable_name: Vec<EcoString>,
+        possible_modules: Vec<EcoString>,
     },
 
     UnknownType {
@@ -1317,7 +1317,7 @@ pub enum UnknownValueConstructorError {
         name: EcoString,
         variables: Vec<EcoString>,
         type_with_name_in_scope: bool,
-        imported_modules_with_same_public_variable_name: Vec<EcoString>,
+        possible_modules: Vec<EcoString>,
     },
 
     Module {
@@ -1343,14 +1343,14 @@ pub fn convert_get_value_constructor_error(
             name,
             variables,
             type_with_name_in_scope,
-            imported_modules_with_same_public_variable_name,
+            possible_modules,
         } => Error::UnknownVariable {
             location,
             name,
             variables,
             discarded_location: None,
             type_with_name_in_scope,
-            imported_modules_with_same_public_variable_name,
+            possible_modules,
         },
 
         UnknownValueConstructorError::Module { name, suggestions } => Error::UnknownModule {

compiler-core/src/type_/expression.rs

@@ -435,7 +435,7 @@ impl<'a, 'b> ExprTyper<'a, 'b> {
             UntypedExpr::Var { location, name, .. } => self.infer_var(
                 name,
                 location,
-                VarUsage::Other,
+                ValueUsage::Other,
                 ReferenceRegistration::RegisterReferences,
             ),
 
@@ -727,7 +727,7 @@ impl<'a, 'b> ExprTyper<'a, 'b> {
     }
 
     // Helper to create a new error expr.
-    pub(crate) fn error_expr(&mut self, location: SrcSpan) -> TypedExpr {
+    fn error_expr(&mut self, location: SrcSpan) -> TypedExpr {
         TypedExpr::Invalid {
             location,
             type_: self.new_unbound_var(),
@@ -1246,11 +1246,11 @@ impl<'a, 'b> ExprTyper<'a, 'b> {
         }
     }
 
-    pub(crate) fn infer_var(
+    fn infer_var(
         &mut self,
         name: EcoString,
         location: SrcSpan,
-        var_usage: VarUsage,
+        var_usage: ValueUsage,
         register_reference: ReferenceRegistration,
     ) -> Result<TypedExpr, Error> {
         let constructor = self.do_infer_value_constructor(
@@ -1354,7 +1354,7 @@ impl<'a, 'b> ExprTyper<'a, 'b> {
             UntypedExpr::Var { location, name } => self.infer_var(
                 name,
                 location,
-                VarUsage::Other,
+                ValueUsage::Other,
                 ReferenceRegistration::DoNotRegisterReferences,
             ),
             _ => self.infer_or_error(container),
@@ -2320,7 +2320,7 @@ impl<'a, 'b> ExprTyper<'a, 'b> {
         match guard {
             ClauseGuard::Var { location, name, .. } => {
                 let constructor =
-                    self.infer_value_constructor(&None, &name, &location, VarUsage::Other)?;
+                    self.infer_value_constructor(&None, &name, &location, ValueUsage::Other)?;
 
                 // We cannot support all values in guard expressions as the BEAM does not
                 let definition_location = match &constructor.variant {
@@ -3483,7 +3483,7 @@ impl<'a, 'b> ExprTyper<'a, 'b> {
         module: &Option<(EcoString, SrcSpan)>,
         name: &EcoString,
         location: &SrcSpan,
-        var_usage: VarUsage,
+        var_usage: ValueUsage,
     ) -> Result<ValueConstructor, Error> {
         self.do_infer_value_constructor(
             module,
@@ -3499,7 +3499,7 @@ impl<'a, 'b> ExprTyper<'a, 'b> {
         module: &Option<(EcoString, SrcSpan)>,
         name: &EcoString,
         location: &SrcSpan,
-        var_usage: VarUsage,
+        var_usage: ValueUsage,
         register_reference: ReferenceRegistration,
     ) -> Result<ValueConstructor, Error> {
         let constructor = match module {
@@ -3640,7 +3640,7 @@ impl<'a, 'b> ExprTyper<'a, 'b> {
         &mut self,
         name: &EcoString,
         location: &SrcSpan,
-        var_usage: VarUsage,
+        var_usage: ValueUsage,
     ) -> Error {
         // First try to see if this is a module alias:
         // `import gleam/io`
@@ -3653,47 +3653,21 @@ impl<'a, 'b> ExprTyper<'a, 'b> {
                 name: name.clone(),
             },
             None => {
-                let imported_modules_with_same_public_variable_name = match var_usage {
+                let possible_modules = match var_usage {
                     // This is a function call, we need to suggest a public
-                    // variable which is a function with the correct arity
-                    VarUsage::Call { arity } => self
+                    // value which is a function with the correct arity
+                    ValueUsage::Call { arity } => self
                         .environment
                         .imported_modules
                         .iter()
                         .filter_map(|(module_name, (_, module))| {
-                            module
-                                .get_public_value(name)
-                                .filter(|value_constructor| {
-                                    match value_constructor.type_.fn_types() {
-                                        Some((fn_arguments_type, _)) => {
-                                            fn_arguments_type.len() == arity
-                                        }
-                                        None => false,
-                                    }
-                                })
-                                .map(|_| module_name)
-                        })
-                        .cloned()
-                        .collect_vec(),
-                    // This is a `Call` into a `Pipeline`. This is hard to make
-                    // good suggestions because of the way it can be desugared.
-                    // In this case, return every functions with the same name
-                    // even if they have wrong arity.
-                    VarUsage::PipelineCall => self
-                        .environment
-                        .imported_modules
-                        .iter()
-                        .filter_map(|(module_name, (_, module))| {
-                            module
-                                .get_public_value(name)
-                                .filter(|value_constructor| value_constructor.type_.is_fun())
-                                .map(|_| module_name)
+                            module.get_public_function(name, arity).map(|_| module_name)
                         })
                         .cloned()
                         .collect_vec(),
                     // This is a reference to a variable, we need to suggest
-                    // public variables of any type
-                    VarUsage::Other => self
+                    // a public value of any type
+                    ValueUsage::Other => self
                         .environment
                         .imported_modules
                         .iter()
@@ -3718,7 +3692,7 @@ impl<'a, 'b> ExprTyper<'a, 'b> {
                         .module_types
                         .keys()
                         .any(|typ| typ == name),
-                    imported_modules_with_same_public_variable_name,
+                    possible_modules,
                 }
             }
         }
@@ -3779,7 +3753,7 @@ impl<'a, 'b> ExprTyper<'a, 'b> {
             } if arguments.is_empty() => {
                 // Type check the record constructor
                 let constructor =
-                    self.infer_value_constructor(&module, &name, &location, VarUsage::Other)?;
+                    self.infer_value_constructor(&module, &name, &location, ValueUsage::Other)?;
 
                 let (tag, field_map) = match &constructor.variant {
                     ValueConstructorVariant::Record {
@@ -3822,7 +3796,7 @@ impl<'a, 'b> ExprTyper<'a, 'b> {
                     &module,
                     &name,
                     &location,
-                    VarUsage::Call {
+                    ValueUsage::Call {
                         arity: arguments.len(),
                     },
                 )?;
@@ -3968,7 +3942,7 @@ impl<'a, 'b> ExprTyper<'a, 'b> {
             } => {
                 // Infer the type of this constant
                 let constructor =
-                    self.infer_value_constructor(&module, &name, &location, VarUsage::Other)?;
+                    self.infer_value_constructor(&module, &name, &location, ValueUsage::Other)?;
                 match constructor.variant {
                     ValueConstructorVariant::ModuleConstant { .. }
                     | ValueConstructorVariant::LocalConstant { .. }
@@ -4146,27 +4120,7 @@ impl<'a, 'b> ExprTyper<'a, 'b> {
     ) -> (TypedExpr, Vec<TypedCallArg>, Arc<Type>) {
         let fun = match fun {
             UntypedExpr::Var { location, name } => {
-                // Because we are not calling infer / infer_or_error directly,
-                // we do not warn if there was a previous panic. Check for that
-                // here. Not perfect, but works.
-                if self.previous_panics {
-                    self.warn_for_unreachable_code(location, PanicPosition::PreviousExpression);
-                }
-
-                match self.infer_var(
-                    name,
-                    location,
-                    VarUsage::Call {
-                        arity: arguments.len(),
-                    },
-                    ReferenceRegistration::RegisterReferences,
-                ) {
-                    Ok(typed_expr) => typed_expr,
-                    Err(error) => {
-                        self.problems.error(error);
-                        self.error_expr(location)
-                    }
-                }
+                self.infer_called_var(name, location, arguments.len())
             }
 
             UntypedExpr::FieldAccess {
@@ -4206,6 +4160,26 @@ impl<'a, 'b> ExprTyper<'a, 'b> {
         (fun, arguments, type_)
     }
 
+    pub fn infer_called_var(
+        &mut self,
+        name: EcoString,
+        location: SrcSpan,
+        arity: usize,
+    ) -> TypedExpr {
+        match self.infer_var(
+            name,
+            location,
+            ValueUsage::Call { arity },
+            ReferenceRegistration::RegisterReferences,
+        ) {
+            Ok(typed_expr) => typed_expr,
+            Err(error) => {
+                self.problems.error(error);
+                self.error_expr(location)
+            }
+        }
+    }
+
     fn infer_fn_with_call_context(
         &mut self,
         arguments: Vec<UntypedArg>,
@@ -4768,7 +4742,7 @@ fn is_trusted_pure_module(environment: &Environment<'_>) -> bool {
 }
 
 #[derive(Debug, Clone, Copy)]
-pub(crate) enum ReferenceRegistration {
+enum ReferenceRegistration {
     RegisterReferences,
     DoNotRegisterReferences,
 }

compiler-core/src/type_/pattern.rs

@@ -1259,7 +1259,7 @@ impl<'a, 'b> PatternTyper<'a, 'b> {
                                     .module_types
                                     .keys()
                                     .any(|type_| type_ == &name),
-                                imported_modules_with_same_public_variable_name: self
+                                possible_modules: self
                                     .environment
                                     .imported_modules
                                     .iter()