Skip to content

Better error message for unknown variable named as an ignored variable #4686

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 18, 2025
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 27 additions & 9 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,6 @@

## Unreleased

### Build tool

- Generated JavaScript functions, constants, and custom type constructors now
include any doc comment as a JSDoc comment, making it easier to use the
generated code and browse its documentation from JavaScript.
([Giacomo Cavalieri](https://github.com/giacomocavalieri))

## v1.11.0-rc1 - 2025-05-15

### Compiler

- Generated JavaScript functions, constants, and custom type constructors now
Expand Down Expand Up @@ -57,6 +48,33 @@
shadows an imported name in the current module.
([Aayush Tripathi](https://github.com/aayush-tripathi))

- The compiler can now tell when an unknown variable might be referring to an
ignored variable and provide an helpful error message highlighting it. For
example, this piece of code:

```gleam
pub fn go() {
let _x = 1
x + 1
}
```

Now results in the following error:

```
error: Unknown variable
┌─ /src/one/two.gleam:4:3
3 │ let _x = 1
│ -- Did you mean to use this ignored variable?
4 │ x + 1
│ ^

The name `x` is not in scope here.
```

([Giacomo Cavalieri](https://github.com/giacomocavalieri))

### Build tool

- `gleam update`, `gleam deps update`, and `gleam deps download` will now print
Expand Down
22 changes: 17 additions & 5 deletions compiler-core/src/analyse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -550,11 +550,23 @@ impl<'a, A> ModuleAnalyzer<'a, A> {
has_javascript_external: external_javascript.is_some(),
};

let typed_args = arguments
.into_iter()
.zip(&prereg_args_types)
.map(|(a, t)| a.set_type(t.clone()))
.collect_vec();
let mut typed_args = Vec::with_capacity(arguments.len());
for (argument, type_) in arguments.into_iter().zip(&prereg_args_types) {
let argument = argument.set_type(type_.clone());
match &argument.names {
ast::ArgNames::Named { .. } | ast::ArgNames::NamedLabelled { .. } => (),
ast::ArgNames::Discard { name, location }
| ast::ArgNames::LabelledDiscard {
name,
name_location: location,
..
} => {
let _ = environment.ignored_names.insert(name.clone(), *location);
}
}

typed_args.push(argument);
}

// We have already registered the function in the `register_value_from_function`
// method, but here we must set this as the current function again, so that anything
Expand Down
41 changes: 33 additions & 8 deletions compiler-core/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use crate::type_::printer::{Names, Printer};
use crate::type_::{FieldAccessUsage, error::PatternMatchKind};
use crate::{ast::BinOp, parse::error::ParseErrorType, type_::Type};
use crate::{bit_array, diagnostic::Level, javascript, type_::UnifyErrorSituation};
use ecow::EcoString;
use ecow::{EcoString, eco_format};
use itertools::Itertools;
use pubgrub::Package;
use pubgrub::{DerivationTree, VersionSet};
Expand Down Expand Up @@ -2388,6 +2388,7 @@ but no type in scope with that name."
TypeError::UnknownVariable {
location,
variables,
ignored_variables,
name,
type_with_name_in_scope,
} => {
Expand All @@ -2402,20 +2403,44 @@ but no type in scope with that name."
wrap_format!("The name `{name}` is not in scope here.")
}
};
Diagnostic {
title: "Unknown variable".into(),
text,
hint: None,
level: Level::Error,
location: Some(Location {

let ignored_variable = ignored_variables.get(&eco_format!("_{name}"));
let location = if let Some(ignored_location) = ignored_variable {
Location {
label: Label {
text: None,
span: *location,
},
path: path.clone(),
src: src.clone(),
extra_labels: vec![
ExtraLabel {
src_info: None,
label: Label {
text: Some("Did you mean to use this ignored variable?".into()),
span: *ignored_location,
}
}
],
}
} else {
Location {
label: Label {
text: did_you_mean(name, variables),
span: *location,
},
path: path.clone(),
src: src.clone(),
extra_labels: vec![],
}),
}
};

Diagnostic {
title: "Unknown variable".into(),
text,
hint: None,
level: Level::Error,
location: Some(location),
}
}

Expand Down
20 changes: 18 additions & 2 deletions compiler-core/src/type_/environment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ pub struct Environment<'a> {
/// Values defined in the current function (or the prelude)
pub scope: im::HashMap<EcoString, ValueConstructor>,

// The names of all the ignored variables and arguments in scope:
// `let _var = 10` `pub fn main(_var) { todo }`.
pub ignored_names: HashMap<EcoString, SrcSpan>,

/// Types defined in the current module (or the prelude)
pub module_types: HashMap<EcoString, TypeConstructor>,

Expand Down Expand Up @@ -133,6 +137,7 @@ impl<'a> Environment<'a> {
unqualified_imported_types: HashMap::new(),
accessors: prelude.accessors.clone(),
scope: prelude.values.clone().into(),
ignored_names: HashMap::new(),
importable_modules,
current_module,
local_variable_usages: vec![HashMap::new()],
Expand All @@ -148,6 +153,7 @@ impl<'a> Environment<'a> {
#[derive(Debug)]
pub struct ScopeResetData {
local_values: im::HashMap<EcoString, ValueConstructor>,
ignored_names: HashMap<EcoString, SrcSpan>,
}

impl Environment<'_> {
Expand All @@ -170,8 +176,12 @@ impl Environment<'_> {

pub fn open_new_scope(&mut self) -> ScopeResetData {
let local_values = self.scope.clone();
let ignored_names = self.ignored_names.clone();
self.local_variable_usages.push(HashMap::new());
ScopeResetData { local_values }
ScopeResetData {
local_values,
ignored_names,
}
}

pub fn close_scope(
Expand All @@ -180,6 +190,11 @@ impl Environment<'_> {
was_successful: bool,
problems: &mut Problems,
) {
let ScopeResetData {
local_values,
ignored_names,
} = data;

let unused = self
.local_variable_usages
.pop()
Expand All @@ -192,7 +207,8 @@ impl Environment<'_> {
if was_successful {
self.handle_unused_variables(unused, problems);
}
self.scope = data.local_values;
self.scope = local_values;
self.ignored_names = ignored_names;
}

pub fn next_uid(&mut self) -> u64 {
Expand Down
4 changes: 3 additions & 1 deletion compiler-core/src/type_/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use hexpm::version::Version;
use num_bigint::BigInt;
#[cfg(test)]
use pretty_assertions::assert_eq;
use std::sync::Arc;
use std::{collections::HashMap, sync::Arc};

/// Errors and warnings discovered when compiling a module.
///
Expand Down Expand Up @@ -167,6 +167,7 @@ pub enum Error {
location: SrcSpan,
name: EcoString,
variables: Vec<EcoString>,
ignored_variables: HashMap<EcoString, SrcSpan>,
type_with_name_in_scope: bool,
},

Expand Down Expand Up @@ -1297,6 +1298,7 @@ pub fn convert_get_value_constructor_error(
location,
name,
variables,
ignored_variables: HashMap::new(),
type_with_name_in_scope,
},

Expand Down
11 changes: 11 additions & 0 deletions compiler-core/src/type_/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1008,6 +1008,16 @@ impl<'a, 'b> ExprTyper<'a, 'b> {
.map(|type_| self.type_from_ast(&type_))
.unwrap_or_else(|| Ok(self.new_unbound_var()))?;

match &names {
ArgNames::Named { .. } | ArgNames::NamedLabelled { .. } => (),
ArgNames::Discard { name, .. } | ArgNames::LabelledDiscard { name, .. } => {
let _ = self
.environment
.ignored_names
.insert(name.clone(), location);
}
}

// If we know the expected type of the argument from its contextual
// usage then unify the newly constructed type with the expected type.
// We do this here because then there is more type information for the
Expand Down Expand Up @@ -3469,6 +3479,7 @@ impl<'a, 'b> ExprTyper<'a, 'b> {
location: *location,
name: name.clone(),
variables: self.environment.local_value_names(),
ignored_variables: self.environment.ignored_names.clone(),
type_with_name_in_scope: self
.environment
.module_types
Expand Down
19 changes: 13 additions & 6 deletions compiler-core/src/type_/pattern.rs
Original file line number Diff line number Diff line change
Expand Up @@ -586,6 +586,10 @@ impl<'a, 'b> PatternTyper<'a, 'b> {
match pattern {
Pattern::Discard { name, location, .. } => {
self.check_name_case(location, &name, Named::Discard);
let _ = self
.environment
.ignored_names
.insert(name.clone(), location);
Pattern::Discard {
type_,
name,
Expand Down Expand Up @@ -613,8 +617,8 @@ impl<'a, 'b> PatternTyper<'a, 'b> {

Pattern::VarUsage { name, location, .. } => {
let constructor = match self.variables.get_mut(&name) {
// If we've bound a variable in the current bit array pattern, we
// want to use that.
// If we've bound a variable in the current bit array pattern,
// we want to use that.
Some(variable) if variable.in_scope() => {
variable.usage = Usage::UsedInPattern;
ValueConstructor::local_variable(
Expand All @@ -631,6 +635,7 @@ impl<'a, 'b> PatternTyper<'a, 'b> {
location,
name: name.clone(),
variables: self.environment.local_value_names(),
ignored_variables: self.environment.ignored_names.clone(),
type_with_name_in_scope: self
.environment
.module_types
Expand Down Expand Up @@ -695,10 +700,12 @@ impl<'a, 'b> PatternTyper<'a, 'b> {
},
);
}
AssignName::Discard(_) => {
if let AssignName::Discard(right) = &right_side_assignment {
self.check_name_case(right_location, right, Named::Discard);
}
AssignName::Discard(right) => {
let _ = self
.environment
.ignored_names
.insert(right.clone(), right_location);
self.check_name_case(right_location, right, Named::Discard);
}
};

Expand Down
67 changes: 67 additions & 0 deletions compiler-core/src/type_/tests/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3125,3 +3125,70 @@ fn bit_array_using_pattern_variables_from_other_bit_array() {
fn non_utf8_string_assignment() {
assert_error!(r#"let assert <<"Hello" as message:utf16>> = <<>>"#);
}

#[test]
fn shadowed_function_argument() {
assert_module_error!(
"
pub fn go(_x) {
x + 1
}
"
);
}

#[test]
fn shadowed_fn_argument() {
assert_module_error!(
"
pub fn go(x) {
fn(_y) {
y + x
}
}
"
);
}

#[test]
fn shadowed_let_variable() {
assert_module_error!(
"
pub fn go() {
let _x = 1
x + 1
}
"
);
}

#[test]
fn shadowed_pattern_variable() {
assert_module_error!(
"
pub type Wibble {
Wibble(Int)
}

pub fn go(x) {
case x {
Wibble(_y) -> y + 1
}
}
"
);
}

#[test]
fn do_not_suggest_ignored_variable_outside_of_current_scope() {
assert_module_error!(
"
pub fn go() {
let _ = {
let _y = 1 // <- this shouldn't be highlighted!
}
y
}
"
);
}
Loading
Loading