From 43f5974254df580ffa5eb1cfc50eb3d9b5c276fb Mon Sep 17 00:00:00 2001 From: Giacomo Cavalieri Date: Mon, 16 Jun 2025 21:24:27 +0200 Subject: [PATCH 1/5] improve error message for variable not in scope when there's discarded variables --- compiler-core/src/analyse.rs | 22 ++++-- compiler-core/src/error.rs | 41 +++++++++--- compiler-core/src/type_/environment.rs | 20 +++++- compiler-core/src/type_/error.rs | 4 +- compiler-core/src/type_/expression.rs | 11 +++ compiler-core/src/type_/pattern.rs | 19 ++++-- compiler-core/src/type_/tests/errors.rs | 67 +++++++++++++++++++ ...red_variable_outside_of_current_scope.snap | 22 ++++++ ...__tests__errors__shadowed_fn_argument.snap | 23 +++++++ ...s__errors__shadowed_function_argument.snap | 21 ++++++ ..._tests__errors__shadowed_let_variable.snap | 22 ++++++ ...ts__errors__shadowed_pattern_variable.snap | 27 ++++++++ 12 files changed, 277 insertions(+), 22 deletions(-) create mode 100644 compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__do_not_suggest_ignored_variable_outside_of_current_scope.snap create mode 100644 compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_fn_argument.snap create mode 100644 compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_function_argument.snap create mode 100644 compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_let_variable.snap create mode 100644 compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_pattern_variable.snap diff --git a/compiler-core/src/analyse.rs b/compiler-core/src/analyse.rs index e6aff28a7eb..a7480560e34 100644 --- a/compiler-core/src/analyse.rs +++ b/compiler-core/src/analyse.rs @@ -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 diff --git a/compiler-core/src/error.rs b/compiler-core/src/error.rs index fd997a28573..8a3361a5bd3 100644 --- a/compiler-core/src/error.rs +++ b/compiler-core/src/error.rs @@ -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}; @@ -2388,6 +2388,7 @@ but no type in scope with that name." TypeError::UnknownVariable { location, variables, + ignored_variables, name, type_with_name_in_scope, } => { @@ -2402,12 +2403,28 @@ 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, @@ -2415,7 +2432,15 @@ but no type in scope with that name." path: path.clone(), src: src.clone(), extra_labels: vec![], - }), + } + }; + + Diagnostic { + title: "Unknown variable".into(), + text, + hint: None, + level: Level::Error, + location: Some(location), } } diff --git a/compiler-core/src/type_/environment.rs b/compiler-core/src/type_/environment.rs index 9aab3eef2c9..e0b315bdf6b 100644 --- a/compiler-core/src/type_/environment.rs +++ b/compiler-core/src/type_/environment.rs @@ -56,6 +56,10 @@ pub struct Environment<'a> { /// Values defined in the current function (or the prelude) pub scope: im::HashMap, + // The names of all the ignored variables and arguments in scope: + // `let _var = 10` `pub fn main(_var) { todo }`. + pub ignored_names: HashMap, + /// Types defined in the current module (or the prelude) pub module_types: HashMap, @@ -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()], @@ -148,6 +153,7 @@ impl<'a> Environment<'a> { #[derive(Debug)] pub struct ScopeResetData { local_values: im::HashMap, + ignored_names: HashMap, } impl Environment<'_> { @@ -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( @@ -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() @@ -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 { diff --git a/compiler-core/src/type_/error.rs b/compiler-core/src/type_/error.rs index 63aa60ad032..07aefbae72b 100644 --- a/compiler-core/src/type_/error.rs +++ b/compiler-core/src/type_/error.rs @@ -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. /// @@ -167,6 +167,7 @@ pub enum Error { location: SrcSpan, name: EcoString, variables: Vec, + ignored_variables: HashMap, type_with_name_in_scope: bool, }, @@ -1297,6 +1298,7 @@ pub fn convert_get_value_constructor_error( location, name, variables, + ignored_variables: HashMap::new(), type_with_name_in_scope, }, diff --git a/compiler-core/src/type_/expression.rs b/compiler-core/src/type_/expression.rs index 146e6d2ba27..1c10a777a4b 100644 --- a/compiler-core/src/type_/expression.rs +++ b/compiler-core/src/type_/expression.rs @@ -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 @@ -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 diff --git a/compiler-core/src/type_/pattern.rs b/compiler-core/src/type_/pattern.rs index dd11aba74f9..4886b977749 100644 --- a/compiler-core/src/type_/pattern.rs +++ b/compiler-core/src/type_/pattern.rs @@ -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, @@ -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( @@ -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 @@ -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); } }; diff --git a/compiler-core/src/type_/tests/errors.rs b/compiler-core/src/type_/tests/errors.rs index 6c5509406af..2524d137f96 100644 --- a/compiler-core/src/type_/tests/errors.rs +++ b/compiler-core/src/type_/tests/errors.rs @@ -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 +} +" + ); +} diff --git a/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__do_not_suggest_ignored_variable_outside_of_current_scope.snap b/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__do_not_suggest_ignored_variable_outside_of_current_scope.snap new file mode 100644 index 00000000000..46afe2a50d8 --- /dev/null +++ b/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__do_not_suggest_ignored_variable_outside_of_current_scope.snap @@ -0,0 +1,22 @@ +--- +source: compiler-core/src/type_/tests/errors.rs +expression: "\npub fn go() {\n let _ = {\n let _y = 1 // <- this shouldn't be highlighted!\n }\n y\n}\n" +--- +----- SOURCE CODE + +pub fn go() { + let _ = { + let _y = 1 // <- this shouldn't be highlighted! + } + y +} + + +----- ERROR +error: Unknown variable + ┌─ /src/one/two.gleam:6:3 + │ +6 │ y + │ ^ + +The name `y` is not in scope here. diff --git a/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_fn_argument.snap b/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_fn_argument.snap new file mode 100644 index 00000000000..c4be27a16ee --- /dev/null +++ b/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_fn_argument.snap @@ -0,0 +1,23 @@ +--- +source: compiler-core/src/type_/tests/errors.rs +expression: "\npub fn go(x) {\n fn(_y) {\n y + x\n }\n}\n" +--- +----- SOURCE CODE + +pub fn go(x) { + fn(_y) { + y + x + } +} + + +----- ERROR +error: Unknown variable + ┌─ /src/one/two.gleam:4:5 + │ +3 │ fn(_y) { + │ -- Did you mean to use this ignored variable? +4 │ y + x + │ ^ + +The name `y` is not in scope here. diff --git a/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_function_argument.snap b/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_function_argument.snap new file mode 100644 index 00000000000..1b4899dcde6 --- /dev/null +++ b/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_function_argument.snap @@ -0,0 +1,21 @@ +--- +source: compiler-core/src/type_/tests/errors.rs +expression: "\npub fn go(_x) {\n x + 1\n}\n" +--- +----- SOURCE CODE + +pub fn go(_x) { + x + 1 +} + + +----- ERROR +error: Unknown variable + ┌─ /src/one/two.gleam:3:3 + │ +2 │ pub fn go(_x) { + │ -- Did you mean to use this ignored variable? +3 │ x + 1 + │ ^ + +The name `x` is not in scope here. diff --git a/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_let_variable.snap b/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_let_variable.snap new file mode 100644 index 00000000000..1ca13534f0f --- /dev/null +++ b/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_let_variable.snap @@ -0,0 +1,22 @@ +--- +source: compiler-core/src/type_/tests/errors.rs +expression: "\npub fn go() {\n let _x = 1\n x + 1\n}\n" +--- +----- SOURCE CODE + +pub fn go() { + let _x = 1 + x + 1 +} + + +----- 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. diff --git a/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_pattern_variable.snap b/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_pattern_variable.snap new file mode 100644 index 00000000000..f25f058dfe8 --- /dev/null +++ b/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_pattern_variable.snap @@ -0,0 +1,27 @@ +--- +source: compiler-core/src/type_/tests/errors.rs +expression: "\npub type Wibble {\n Wibble(Int)\n}\n\npub fn go(x) {\n case x {\n Wibble(_y) -> y + 1\n }\n}\n" +--- +----- SOURCE CODE + +pub type Wibble { + Wibble(Int) +} + +pub fn go(x) { + case x { + Wibble(_y) -> y + 1 + } +} + + +----- ERROR +error: Unknown variable + ┌─ /src/one/two.gleam:8:19 + │ +8 │ Wibble(_y) -> y + 1 + │ -- ^ + │ │ + │ Did you mean to use this ignored variable? + +The name `y` is not in scope here. From c8d17b5264853a767f1d65b4e0888c124fba7eb7 Mon Sep 17 00:00:00 2001 From: Giacomo Cavalieri Date: Mon, 16 Jun 2025 21:27:54 +0200 Subject: [PATCH 2/5] fix changelog --- CHANGELOG.md | 9 --------- 1 file changed, 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 82c7734db0f..d1361f8920f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 From 49fd5b4f2a146f53abd74bd5cad4aba5873f9908 Mon Sep 17 00:00:00 2001 From: Giacomo Cavalieri Date: Mon, 16 Jun 2025 21:30:15 +0200 Subject: [PATCH 3/5] changelog --- CHANGELOG.md | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d1361f8920f..6129239032b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,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 From bea3eb8d6034a4100acb922b1c412b91512dc887 Mon Sep 17 00:00:00 2001 From: Giacomo Cavalieri Date: Tue, 17 Jun 2025 21:53:26 +0200 Subject: [PATCH 4/5] use immutable hash map and carry less data around --- compiler-core/src/analyse.rs | 2 +- compiler-core/src/error.rs | 7 +++---- compiler-core/src/type_/environment.rs | 14 +++++++------- compiler-core/src/type_/error.rs | 8 +++++--- compiler-core/src/type_/expression.rs | 9 +++++++-- compiler-core/src/type_/pattern.rs | 11 ++++++++--- 6 files changed, 31 insertions(+), 20 deletions(-) diff --git a/compiler-core/src/analyse.rs b/compiler-core/src/analyse.rs index a7480560e34..9da13191f42 100644 --- a/compiler-core/src/analyse.rs +++ b/compiler-core/src/analyse.rs @@ -561,7 +561,7 @@ impl<'a, A> ModuleAnalyzer<'a, A> { name_location: location, .. } => { - let _ = environment.ignored_names.insert(name.clone(), *location); + let _ = environment.discarded_names.insert(name.clone(), *location); } } diff --git a/compiler-core/src/error.rs b/compiler-core/src/error.rs index 8a3361a5bd3..1689248f56d 100644 --- a/compiler-core/src/error.rs +++ b/compiler-core/src/error.rs @@ -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, eco_format}; +use ecow::EcoString; use itertools::Itertools; use pubgrub::Package; use pubgrub::{DerivationTree, VersionSet}; @@ -2388,7 +2388,7 @@ but no type in scope with that name." TypeError::UnknownVariable { location, variables, - ignored_variables, + discarded_location, name, type_with_name_in_scope, } => { @@ -2404,8 +2404,7 @@ but no type in scope with that name." } }; - let ignored_variable = ignored_variables.get(&eco_format!("_{name}")); - let location = if let Some(ignored_location) = ignored_variable { + let location = if let Some(ignored_location) = discarded_location { Location { label: Label { text: None, diff --git a/compiler-core/src/type_/environment.rs b/compiler-core/src/type_/environment.rs index e0b315bdf6b..a974dd55e27 100644 --- a/compiler-core/src/type_/environment.rs +++ b/compiler-core/src/type_/environment.rs @@ -58,7 +58,7 @@ pub struct Environment<'a> { // The names of all the ignored variables and arguments in scope: // `let _var = 10` `pub fn main(_var) { todo }`. - pub ignored_names: HashMap, + pub discarded_names: im::HashMap, /// Types defined in the current module (or the prelude) pub module_types: HashMap, @@ -137,7 +137,7 @@ impl<'a> Environment<'a> { unqualified_imported_types: HashMap::new(), accessors: prelude.accessors.clone(), scope: prelude.values.clone().into(), - ignored_names: HashMap::new(), + discarded_names: im::HashMap::new(), importable_modules, current_module, local_variable_usages: vec![HashMap::new()], @@ -153,7 +153,7 @@ impl<'a> Environment<'a> { #[derive(Debug)] pub struct ScopeResetData { local_values: im::HashMap, - ignored_names: HashMap, + discarded_names: im::HashMap, } impl Environment<'_> { @@ -176,11 +176,11 @@ impl Environment<'_> { pub fn open_new_scope(&mut self) -> ScopeResetData { let local_values = self.scope.clone(); - let ignored_names = self.ignored_names.clone(); + let discarded_names = self.discarded_names.clone(); self.local_variable_usages.push(HashMap::new()); ScopeResetData { local_values, - ignored_names, + discarded_names, } } @@ -192,7 +192,7 @@ impl Environment<'_> { ) { let ScopeResetData { local_values, - ignored_names, + discarded_names, } = data; let unused = self @@ -208,7 +208,7 @@ impl Environment<'_> { self.handle_unused_variables(unused, problems); } self.scope = local_values; - self.ignored_names = ignored_names; + self.discarded_names = discarded_names; } pub fn next_uid(&mut self) -> u64 { diff --git a/compiler-core/src/type_/error.rs b/compiler-core/src/type_/error.rs index 07aefbae72b..5aee5a5bf0c 100644 --- a/compiler-core/src/type_/error.rs +++ b/compiler-core/src/type_/error.rs @@ -14,7 +14,7 @@ use hexpm::version::Version; use num_bigint::BigInt; #[cfg(test)] use pretty_assertions::assert_eq; -use std::{collections::HashMap, sync::Arc}; +use std::sync::Arc; /// Errors and warnings discovered when compiling a module. /// @@ -167,7 +167,9 @@ pub enum Error { location: SrcSpan, name: EcoString, variables: Vec, - ignored_variables: HashMap, + /// If there's a discarded variable with the same name in the same scope + /// this will contain its location. + discarded_location: Option, type_with_name_in_scope: bool, }, @@ -1298,7 +1300,7 @@ pub fn convert_get_value_constructor_error( location, name, variables, - ignored_variables: HashMap::new(), + discarded_location: None, type_with_name_in_scope, }, diff --git a/compiler-core/src/type_/expression.rs b/compiler-core/src/type_/expression.rs index 1c10a777a4b..44b02431915 100644 --- a/compiler-core/src/type_/expression.rs +++ b/compiler-core/src/type_/expression.rs @@ -18,6 +18,7 @@ use crate::{ parse::PatternPosition, reference::ReferenceKind, }; +use ecow::eco_format; use hexpm::version::{LowestVersion, Version}; use im::hashmap; use itertools::Itertools; @@ -1013,7 +1014,7 @@ impl<'a, 'b> ExprTyper<'a, 'b> { ArgNames::Discard { name, .. } | ArgNames::LabelledDiscard { name, .. } => { let _ = self .environment - .ignored_names + .discarded_names .insert(name.clone(), location); } } @@ -3479,7 +3480,11 @@ impl<'a, 'b> ExprTyper<'a, 'b> { location: *location, name: name.clone(), variables: self.environment.local_value_names(), - ignored_variables: self.environment.ignored_names.clone(), + discarded_location: self + .environment + .discarded_names + .get(&eco_format!("_{name}")) + .cloned(), type_with_name_in_scope: self .environment .module_types diff --git a/compiler-core/src/type_/pattern.rs b/compiler-core/src/type_/pattern.rs index 4886b977749..2892f93f895 100644 --- a/compiler-core/src/type_/pattern.rs +++ b/compiler-core/src/type_/pattern.rs @@ -1,3 +1,4 @@ +use ecow::eco_format; use hexpm::version::{LowestVersion, Version}; use im::hashmap; use itertools::Itertools; @@ -588,7 +589,7 @@ impl<'a, 'b> PatternTyper<'a, 'b> { self.check_name_case(location, &name, Named::Discard); let _ = self .environment - .ignored_names + .discarded_names .insert(name.clone(), location); Pattern::Discard { type_, @@ -635,7 +636,11 @@ impl<'a, 'b> PatternTyper<'a, 'b> { location, name: name.clone(), variables: self.environment.local_value_names(), - ignored_variables: self.environment.ignored_names.clone(), + discarded_location: self + .environment + .discarded_names + .get(&eco_format!("_{name}")) + .cloned(), type_with_name_in_scope: self .environment .module_types @@ -703,7 +708,7 @@ impl<'a, 'b> PatternTyper<'a, 'b> { AssignName::Discard(right) => { let _ = self .environment - .ignored_names + .discarded_names .insert(right.clone(), right_location); self.check_name_case(right_location, right, Named::Discard); } From 2b813e53998b841fb4c8c351ba45839b33055661 Mon Sep 17 00:00:00 2001 From: Giacomo Cavalieri Date: Tue, 17 Jun 2025 22:00:39 +0200 Subject: [PATCH 5/5] improve error message --- CHANGELOG.md | 6 +- compiler-core/src/error.rs | 71 +++++++++++-------- ...__tests__errors__shadowed_fn_argument.snap | 6 +- ...s__errors__shadowed_function_argument.snap | 6 +- ..._tests__errors__shadowed_let_variable.snap | 6 +- ...ts__errors__shadowed_pattern_variable.snap | 6 +- 6 files changed, 55 insertions(+), 46 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6129239032b..2877ade616b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -66,11 +66,11 @@ ┌─ /src/one/two.gleam:4:3 │ 3 │ let _x = 1 - │ -- Did you mean to use this ignored variable? + │ -- This value is discarded 4 │ x + 1 - │ ^ + │ ^ So it is not in scope here. - The name `x` is not in scope here. + Hint: Change `_x` to `x` or reference another variable ``` ([Giacomo Cavalieri](https://github.com/giacomocavalieri)) diff --git a/compiler-core/src/error.rs b/compiler-core/src/error.rs index 1689248f56d..214a937b1ad 100644 --- a/compiler-core/src/error.rs +++ b/compiler-core/src/error.rs @@ -2392,22 +2392,12 @@ but no type in scope with that name." name, type_with_name_in_scope, } => { - let text = if *type_with_name_in_scope { - wrap_format!("`{name}` is a type, it cannot be used as a value.") - } else { - let is_first_char_uppercase = name.chars().next().is_some_and(char::is_uppercase); + let title = String::from("Unknown variable"); - if is_first_char_uppercase { - wrap_format!("The custom type variant constructor `{name}` is not in scope here.") - } else { - wrap_format!("The name `{name}` is not in scope here.") - } - }; - - let location = if let Some(ignored_location) = discarded_location { - Location { + if let Some(ignored_location) = discarded_location { + let location = Location { label: Label { - text: None, + text: Some("So this is not in scope".into()), span: *location, }, path: path.clone(), @@ -2416,30 +2406,49 @@ but no type in scope with that name." ExtraLabel { src_info: None, label: Label { - text: Some("Did you mean to use this ignored variable?".into()), + text: Some("This value is discarded".into()), span: *ignored_location, } } ], + }; + Diagnostic { + title, + text: "".into(), + hint: Some(wrap_format!( + "Change `_{name}` to `{name}` or reference another variable", + )), + level: Level::Error, + location: Some(location), } } else { - Location { - label: Label { - text: did_you_mean(name, variables), - span: *location, - }, - path: path.clone(), - src: src.clone(), - extra_labels: vec![], - } - }; + let text = if *type_with_name_in_scope { + wrap_format!("`{name}` is a type, it cannot be used as a value.") + } else { + let is_first_char_uppercase = name.chars().next().is_some_and(char::is_uppercase); - Diagnostic { - title: "Unknown variable".into(), - text, - hint: None, - level: Level::Error, - location: Some(location), + if is_first_char_uppercase { + wrap_format!("The custom type variant constructor `{name}` is not in scope here.") + } else { + wrap_format!("The name `{name}` is not in scope here.") + } + }; + + Diagnostic { + title, + text, + hint: None, + level: Level::Error, + location: Some(Location { + label: Label { + text: did_you_mean(name, variables), + span: *location, + }, + path: path.clone(), + src: src.clone(), + extra_labels: vec![], + }), + } } } diff --git a/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_fn_argument.snap b/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_fn_argument.snap index c4be27a16ee..72f9b286eb6 100644 --- a/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_fn_argument.snap +++ b/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_fn_argument.snap @@ -16,8 +16,8 @@ error: Unknown variable ┌─ /src/one/two.gleam:4:5 │ 3 │ fn(_y) { - │ -- Did you mean to use this ignored variable? + │ -- This value is discarded 4 │ y + x - │ ^ + │ ^ So this is not in scope -The name `y` is not in scope here. +Hint: Change `_y` to `y` or reference another variable diff --git a/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_function_argument.snap b/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_function_argument.snap index 1b4899dcde6..27b632829b9 100644 --- a/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_function_argument.snap +++ b/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_function_argument.snap @@ -14,8 +14,8 @@ error: Unknown variable ┌─ /src/one/two.gleam:3:3 │ 2 │ pub fn go(_x) { - │ -- Did you mean to use this ignored variable? + │ -- This value is discarded 3 │ x + 1 - │ ^ + │ ^ So this is not in scope -The name `x` is not in scope here. +Hint: Change `_x` to `x` or reference another variable diff --git a/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_let_variable.snap b/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_let_variable.snap index 1ca13534f0f..d48099fdde6 100644 --- a/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_let_variable.snap +++ b/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_let_variable.snap @@ -15,8 +15,8 @@ error: Unknown variable ┌─ /src/one/two.gleam:4:3 │ 3 │ let _x = 1 - │ -- Did you mean to use this ignored variable? + │ -- This value is discarded 4 │ x + 1 - │ ^ + │ ^ So this is not in scope -The name `x` is not in scope here. +Hint: Change `_x` to `x` or reference another variable diff --git a/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_pattern_variable.snap b/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_pattern_variable.snap index f25f058dfe8..a0cb5b4e89c 100644 --- a/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_pattern_variable.snap +++ b/compiler-core/src/type_/tests/snapshots/gleam_core__type___tests__errors__shadowed_pattern_variable.snap @@ -20,8 +20,8 @@ error: Unknown variable ┌─ /src/one/two.gleam:8:19 │ 8 │ Wibble(_y) -> y + 1 - │ -- ^ + │ -- ^ So this is not in scope │ │ - │ Did you mean to use this ignored variable? + │ This value is discarded -The name `y` is not in scope here. +Hint: Change `_y` to `y` or reference another variable