Add suggestions for variables with the same name in imported modules when unknown variable #4810

realraphrous · 2025-07-28T11:51:52Z

As stated on this Discord message, this is my first contributions to OSS and this project. I would really appreciate any feedback and improvement on my code, this pull request or commits that I made !

Summary

This PR add more suggestions when an unknown variable is encounter by listing all variables / identifier in imported modules that match the same name. Also, when the variable / identifier is called such as functions, pipeline steps or type constructors, the suggestions are based on the name and number of arguments provided to the call. (ref: #2697 (comment))

Examples

For example, take this invalid program:

import gleam/io

pub fn main() -> Nil {
  println("Hello, World!)
}

The error message reported by the compiler:

error: Unknown variable
  ┌─ /home/raphrous/code/gleam/module_suggestion/src/mylib/mymodule.gleam:4:3
  │
4 │   println("Hello, World!")
  │   ^^^^^^^

The name `println` is not in scope here.
Consider using one of these variables:

    io.println

Also, it works with variables, type constructor, constant etc. Consider this custom module and this program:

// mylib/mymodule
pub const my_const = 8

pub type MyType {
  Some(x: Int, y: Int)
}

// main file
import gleam/option
import mylib/mymodule

pub fn main() -> Nil {
  // option.Some need to be suggested but 
  // not mymodule.Some (cause of arity)
  Some(my_const)
}

With these changes we have new suggestions about variables / identifier in imported modules:

error: Unknown variable
  ┌─ /path/to/project/src/project.gleam:7:3
  │
7 │   Some(my_const)
  │   ^^^^

The custom type variant constructor `Some` is not in scope here.
Consider using one of these variables:

    option.Some


error: Unknown variable
  ┌─ /path/to/project/src/project.gleam:7:8
  │
7 │   Some(my_const)
  │        ^^^^^^^^

The name `my_const` is not in scope here.
Consider using one of these variables:

    mymodule.my_const

Implementations

The current implementation uses a new enumeration VarUsage. This is similar to the enum FieldAccessUsage and it helps the function report_name_error by giving a little more context about the usage of this variable: a function call or something else. This function can now report about imported modules containing a variable with the same name as the unknown variable and, in case of function call, the same arity.

I tried to experiment another implementation where the function report_name_error was not provided with more context. The function was keeped as simple as possible by reporting imported modules containing a variable with the same name, without checking if it was a call or something else. Instead, we were filtering at the call side, inside infer_call function for example, if the variable from the imported module was of type "function-like".
Honestly it does not feel right. We needed to check if an UnknownVariable error was emitted, apply a filtering on imported modules that were reported and re-emit the same error with the filter applied. Also, we had to perform this manipulation at every location where a function call was possible, which likely implies code duplication, errors and omissions.

Shortcomings

Wording

First thing first, the wording. I am not sure about the error message wording:

Consider using one of these variables

For many developers, io.println is a function and uri.empty is a constant, not really variables. I wanted to make the error message like:

Consider using on of these identifiers

But the term identifiers is not used in the compiler, in error messages that I found or the language tour so it does not feel right. As I can see in the compiler, Var refer to what I call identifier so I thought it would be best to write variables.

No type checking on arguments

Because we only check name and arity in the case of calls, the suggestions may be wrong because the type of the function parameters does not match the type of arguments provided with the call. If we want that, we need to give more context, such as arguments types, to the report_name_error function in order to find the right functions to suggest.

Suggestions for pipeline calls like `1 |> add(2)`

Also, I had a problem with function call on pipeline. To better explain the situation, take this program:

pub fn main() -> Nil {
  1 |> add(2)
}

It can be desugared in two ways:

pub fn main() -> Nil {
  // First possibility
  add(1, 2)
  // Second possibility
  add(2)(1)
}

I didn't know how to implement this case. For sure, the compiler can report function with arity of 2 and a function with arity 1 that has a return type that is also a function with arity 1. I just didn't know what was the right choice ...

Tests

To conclude, theses commits does not add tests for now, it might be cool to have some !

Ending

Thank you in advance for reading and helping me on my journey into OSS. Please feel free to give me feedback on all this work! 💛

lpil · 2025-08-07T10:28:50Z

Hi @realraphrous! Thanks for making us your first open source PR 🎉

Sorry for the delay reviewing this, I have been busy with the v1.12 release. Looking now

lpil

Looking great! Very nice work, especially for a first contribution.

I've left a bunch of small notes inline.

Could you add tests for all the different paths through this new feature please 🙏

RE naming: You've been using the words identifier and variable here to refer to public constants and functions in other modules. We don't have a meaning for the word identifier, and a variable is a local variable defined with let, so it's something else. "values" fits generally.

Thanks again! Please un-draft this PR and ping me when you want another review, and do reply to any comments of mine that are unclear or need discussion.

lpil · 2025-08-07T10:41:19Z

CHANGELOG.md

+
+- The compiler now suggest public variables and functions from imported modules
+  when the variable in unknown. These variables and functions are suggested
+  based on name and arity.


Nice, very clear explanation. Will be useful for me when I write the release post too!

Let's say "values" rather than "functions and variables" in this changelog.

lpil · 2025-08-07T10:46:03Z

compiler-core/src/type_/error.rs

        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>,


Descriptive, but a little long! Perhaps module_candidates? possible_modules? Something like that?

lpil · 2025-08-07T10:50:04Z

compiler-core/src/error.rs

+                                    let consider_text = imported_modules_with_same_public_variable_name
+                                        .iter()
+                                        .fold(
+                                            String::from("Consider using one of these variables:\n\n"),


They're not really variables as they're definitions in a module. How about this?

Did you mean one of these? - list.map - dict.map

I think we tend to use the - prefix with lists rendered in this module.

lpil · 2025-08-07T10:52:38Z

compiler-core/src/error.rs

+                                // 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


Little stylistic thing: Can we use a for loop here rather than fold here please 🙏

The accumulator is being mutated, so prefer not to use the immutable accumulator passing pattern of fold.

lpil · 2025-08-07T11:13:29Z

compiler-core/src/error.rs

+                                        .fold(
+                                            String::from("Consider using one of these variables:\n\n"),
+                                            |mut acc, module_name| {
+                                                acc.push_str(&wrap_format!("    {module_name}.{name}\n"));


Do not wrap these items please 🙏

lpil · 2025-08-07T11:24:23Z

compiler-core/src/type_/pipe.rs

+                        UntypedExpr::Var { location, name } => self.expr_typer.infer_var(
+                            name,
+                            location,
+                            VarUsage::PipelineCall,


Rather than have a special case for piped functions, let's use arguments.len() + 1 as the arity of the function, as that's by far the most common use of pipes.

lpil · 2025-08-07T11:27:53Z

compiler-core/src/type_.rs

+/// 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
+#[derive(Debug, Copy, Clone, PartialEq, Eq)]
+pub enum VarUsage {


The name says it's specific to variables, but the definition is a lot more generic! Any expression could could produce a function which is called or not.

Perhaps this is a ValueUsage?

lpil · 2025-08-07T11:29:07Z

compiler-core/src/type_/expression.rs

+                        .iter()
+                        .filter_map(|(module_name, (_, module))| {
+                            module
+                                .get_public_value(name)


Perhaps this is a get_public_function(name, arity)?

lpil · 2025-08-07T11:32:48Z

compiler-core/src/type_/expression.rs

    }

-    fn infer_var(
+    pub(crate) fn infer_var(


I'm not the biggest fan of how this function has become part of the public API. The internal ReferenceRegistration being something that the rest of the code needs to know about is something I'd like to avoid- it's not very clear what it does or how to use it.

Maybe introduce a public infer_called_var function that takes the arity as an argument, leaving the existing functions with the same API as before.

I have done that, infer_var stay private and I have introduced infer_called_var has a public function.

lpil · 2025-08-07T11:41:58Z

compiler-core/src/type_/expression.rs

+            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.


Why do we not need to do this for infer_field_access etc below?

What does the "not perfect bit" mean?

Without this kind of mitigation, the test unreachable_use_after_panic failed, no warning are emitted:

pub fn wibble(_) { 1 } pub fn main() { panic use <- wibble 1 }

I checked, there is no test for a similar program and, when tested, there is no warn for unreachable code:

// define: pub fn wibble(_) { 1 } import mylibrary/mymodule pub fn main() { panic use <- mymodule.wibble 1 }

Basically, I wanted to just pass this test, but it seems that there is a problem with use expression.

Edit: It seems that previous_panics is set to false in do_infer_call_with_known_fun before the warning is registered. We have this call path :

infer_use

infer_call

do_infer_call (depending on expression)
3.1. UntypedExpr::Var -> infer_called_var
3.2. UntypedExpr::FieldAccess -> infer_field_access
3.3. UntypedExpr::Fn -> infer_fn_with_call_context
3.4. Others -> infer -> infer_or_error

do_infer_call_with_known_fun

But, warnings for unreachable code are mainly registered from infer_or_error. With these commits, I added the case for UntypedExpr::Var in do_infer_call and I am not going anymore in infer_or_error. So I broke one test by not giving warning for the first case as stated above. It also explains why I don't have warnings in the second example.

Honnestly, I don't know where to correct that, there is too much possibilities. Did you have advice for that ?

@lpil I don't know if I need to ping you for comment, really sorry if not. I does not know anything about Github ...

realraphrous · 2025-08-15T13:20:18Z

Hi @lpil !
I have implemented all your suggestions in this commit.
Also, I have added tests in this commit.

When a variable is not in the current scope, an `Error::UnknownVariable` is triggered. However the only suggestions were for variables in the scope with a similar name computed by the "did you mean" algorithm. With this commit, we also suggest public variables / identifiers (such as constants, functions or type variant constructor) in imported modules with the same name. For example with the following code: ```gleam import gleam/io pub fn main() -> Nil { println("Hello, World!") } ``` The suggestions are: ``` ┌─ /path/to/project/src/project.gleam:4:3 │ 4 │ println("Hello, World!") │ ^^^^^^^ The name `println` is not in scope here. Consider using one of these variables: io.println ``` However because we are only checking the name of the variable, we could have wrong suggestions, as in this case: ```gleam import gleam/float import gleam/int pub fn main() -> Nil { to_string(3) } ``` Here, it is clear that we want suggestions on a function named `to_string` and accepting one parameter of type `Int`, but this is not the case: ``` ┌─ /path/to/project/src/project.gleam:5:3 │ 5 │ to_string(3) │ ^^^^^^^^^ The name `to_string` is not in scope here. Consider using one of these implementations: float.to_string int.to_string ```

Before this commit, all functions / constructors were suggested even if they do not have the correct arity. With this change, only functions / constructors with the correct arity are suggested. However, even if the arity is correct the type of arguments can mismatch, resulting in incorrect suggestions. ```gleam // fn to_string(Int) -> String import gleam/int // fn to_string(Float) -> String import gleam/float pub fn main() -> Nil { to_string(1) } error: Unknown variable ┌─ /path/to/project/src/project.gleam:8:3 │ 8 │ to_string(1) │ ^^^^^^^^^ The name `to_string` is not in scope here. Consider using one of these variables: int.to_string float.to_string ```

At this stage, pipeline are not considered as `Call` so previous work on `UntypedExpr::Call` does not apply directly to pipeline. With this change, we handle the pipeline case when the function / constructor is not a `Call`. ```gleam 1 |> to_string ``` When there is a call in the pipeline, it is more delicate to address because something like that: ```gleam 1 |> add(2) ``` Can be desugared into two possibilities and so suggestions are not the same. The function `report_name_error` does not have enough context to make good suggestions: ```gleam add(1, 2) add(2)(1) ``` In this case, we just suggest every functions.

In this example, the name `zoo` exist in the module `wibble` which is imported. The compiler correctly suggest to use `wibble.zoo`.

realraphrous · 2025-08-26T08:44:00Z

Really sorry @lpil, did not saw that, with force-push, the review become tough ... I'm using Jujutsu and this is so common to just modify prior commit ...
Last changes were just a rebase on main

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.

…ression. With previous changes, a test was broken when a panic was right before a use expression. The test was like that: ```gleam pub fn wibble(_) { 1 } pub fn main() { panic use <- wibble 1 } ``` This was the case because, before these changes, `UntypedExpr::Var` has not special call path in `do_infer_call` function. `infer` and `infer_or_error` was called has expected and a warning was emitted by `warn_for_unreachable_code` in case of a previous panic expression. Actually, `UntypedExpr::FieldAccess` has a special call path in `do_infer_call` and compiler does not warned about a program like that: ```gleam // Define pub fn wibble(_) { 1 } import mylib/mymod pub fn main() { panic use <- mymod.wibble 1 } ``` With this change, use expression are always warned after a panic expression.

lpil · 2025-08-26T09:47:33Z

Looks like the build is failing!

realraphrous · 2025-08-26T10:52:21Z

This is failling cause of a Deno fetching error : Failed fetching. Retrying in 4000ms...
Is that possible to relaunch the pipeline without a new commit @lpil ?

realraphrous · 2025-08-26T11:29:33Z

@lpil, I amended the last commit without any edit just to relaunch the pipeline. This was a Deno fetching issue for sure, now everything is OK !

GearsDatapacks

This looks good! I've left a couple more notes inline, but this can probably be merged soon.

GearsDatapacks · 2025-08-28T20:44:51Z

CHANGELOG.md

+  The name `println` is not in scope here.
+  Consider using one of these variables:
+
+      io.println


Looks like this entry needs to be updated with the latest wording of the error message

Thanks, had not spotted that !

GearsDatapacks · 2025-08-28T20:45:35Z

compiler-core/src/error.rs

+                                        did_you_mean_text.push_str(&format!("  - {module_name}.{name}\n"));
+                                    }
+                                    text.push('\n');
+                                    text.push_str(&did_you_mean_text);


Any reason you are building did_you_mean_text then pushing to text rather than just appending directly to text here?

No reason at all. I will change that to push to the current text variable.

GearsDatapacks · 2025-08-28T21:02:22Z

compiler-core/src/type_/tests/errors.rs

+    1
+}
+"
+    );


Could we get tests for:

Modules imported using an alias (import some_module as some_alias)

Modules which have multiple segments (import some/module)

GearsDatapacks

This looks great to me! Louis still needs to give it another look over before it's merged.

lpil

Super nice! The approach in the code looks flawless to me!

Couple tiny notes inline RE documentation and extracting a helper, and could you add these final tests please:

Internal values from the same package are suggested
Internal values from a different package are not suggested

lpil · 2025-08-29T11:13:14Z

compiler-core/src/type_/expression.rs

        (fun, arguments, type_)
    }

+    pub fn infer_called_var(


Could you add a detailed documentation comment explaining what this function doe and when it is to be used please 🙏

lpil · 2025-08-29T11:14:52Z

compiler-core/src/type_/expression.rs

+                    ValueUsage::Other => self
+                        .environment
+                        .imported_modules
+                        .iter()
+                        .filter_map(|(module_name, (_, module))| {
+                            module.get_public_value(name).map(|_| module_name)
+                        })
+                        .cloned()
+                        .collect_vec(),


This block of code is repeated a few times. Could you extract it and the block above to a helper function or method please.

Looks like one of the places this block exists is in the environment class, so maybe that's a good place for it.

lpil · 2025-08-29T11:16:36Z

compiler-core/src/type_.rs

+/// This is used to know when a value is used as a call or not.
+#[derive(Debug, Copy, Clone, PartialEq, Eq)]
+pub enum ValueUsage {
+    /// Used as `call(..)`, `Type(..)`, `left |> right` or `left |> right(..)`


Suggested change

/// Used as `call(..)`, `Type(..)`, `left |> right` or `left |> right(..)`

/// Used as `function(..)`, `Record(..)`, `left |> right` or `left |> right(..)`

It's not a type, it's value, specifically a record.

lpil · 2025-08-29T11:17:01Z

compiler-core/src/type_.rs

        }
    }

+    pub fn get_public_function(&self, name: &str, arity: usize) -> Option<&ValueConstructor> {


Document this function please 🙏

Does this return internal values too?

Yes, this function also return internal values. This is based on the function get_public_value that check for importable values, and importable values can be public and internal. I named this function like that to be consistent with other function of the same kind.

Also gonna document this function but get_public_value, on which this function is based, is not documented ...

realraphrous changed the title ~~Better suggestions imported modules~~ Add suggestions for variables with the same name in imported modules when unknown variable Jul 28, 2025

realraphrous force-pushed the better-suggestions-imported-modules branch 3 times, most recently from e292510 to 53c6c08 Compare July 29, 2025 11:45

realraphrous marked this pull request as draft July 29, 2025 13:03

realraphrous marked this pull request as ready for review July 29, 2025 13:22

realraphrous mentioned this pull request Aug 1, 2025

Add the fact that the CHANGELOG.md needs to be modified to reflect changes in CONTRIBUTING.md #4826

Closed

realraphrous force-pushed the better-suggestions-imported-modules branch 2 times, most recently from 68e6ce4 to 8e1b877 Compare August 4, 2025 07:50

lpil reviewed Aug 7, 2025

View reviewed changes

lpil marked this pull request as draft August 7, 2025 11:49

realraphrous force-pushed the better-suggestions-imported-modules branch 4 times, most recently from 4d340b6 to 3e2fc4d Compare August 15, 2025 13:03

realraphrous marked this pull request as ready for review August 15, 2025 13:15

realraphrous added 4 commits August 26, 2025 10:31

Update tests to reflect the previous changes.

0e9d8a8

In this example, the name `zoo` exist in the module `wibble` which is imported. The compiler correctly suggest to use `wibble.zoo`.

realraphrous force-pushed the better-suggestions-imported-modules branch from 3e2fc4d to aa35b39 Compare August 26, 2025 08:34

realraphrous added 3 commits August 26, 2025 10:53

Update of the CHANGELOG.md file.

5f92807

realraphrous force-pushed the better-suggestions-imported-modules branch from aa35b39 to e51319c Compare August 26, 2025 08:53

realraphrous force-pushed the better-suggestions-imported-modules branch from 9edc508 to e51319c Compare August 26, 2025 11:11

Add tests for all possible cases for this feature.

0ea266c

realraphrous force-pushed the better-suggestions-imported-modules branch from 9edc508 to 0ea266c Compare August 26, 2025 11:14

GearsDatapacks reviewed Aug 28, 2025

View reviewed changes

Implements GearsDatapacks suggestions.

8345a55

GearsDatapacks approved these changes Aug 29, 2025

View reviewed changes

lpil reviewed Aug 29, 2025

View reviewed changes

lpil marked this pull request as draft August 29, 2025 11:18

	/// Used as `call(..)`, `Type(..)`, `left \|> right` or `left \|> right(..)`
	/// Used as `function(..)`, `Record(..)`, `left \|> right` or `left \|> right(..)`

Uh oh!

Add suggestions for variables with the same name in imported modules when unknown variable #4810

Are you sure you want to change the base?

Add suggestions for variables with the same name in imported modules when unknown variable #4810

Uh oh!

Conversation

realraphrous commented Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Examples

Implementations

Shortcomings

Wording

No type checking on arguments

Suggestions for pipeline calls like 1 |> add(2)

Tests

Ending

Uh oh!

lpil commented Aug 7, 2025

Uh oh!

lpil left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

realraphrous Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

realraphrous commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

realraphrous commented Aug 26, 2025

Uh oh!

lpil commented Aug 26, 2025

Uh oh!

realraphrous commented Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

realraphrous commented Aug 26, 2025

Uh oh!

GearsDatapacks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GearsDatapacks left a comment

realraphrous commented Jul 28, 2025 •

edited

Loading

Suggestions for pipeline calls like `1 |> add(2)`

lpil left a comment •

edited

Loading

realraphrous Aug 7, 2025 •

edited

Loading

realraphrous commented Aug 15, 2025 •

edited

Loading

realraphrous commented Aug 26, 2025 •

edited

Loading