diff --git a/crates/ast/src/lang/env.rs b/crates/ast/src/lang/env.rs index a4fef56079..c86b212c9a 100644 --- a/crates/ast/src/lang/env.rs +++ b/crates/ast/src/lang/env.rs @@ -121,16 +121,18 @@ impl<'a> Env<'a> { Ok(symbol) } - None => Err(RuntimeError::LookupNotInScope( - Loc { + None => Err(RuntimeError::LookupNotInScope { + loc_name: Loc { value: Ident::from(ident), region, }, - self.ident_ids + suggestion_options: self + .ident_ids .ident_strs() .map(|(_, string)| string.into()) .collect(), - )), + underscored_suggestion_region: None, + }), } } else { match self.dep_idents.get(&module_id) { diff --git a/crates/ast/src/lang/scope.rs b/crates/ast/src/lang/scope.rs index d3ca1ba577..db2c23f3f5 100644 --- a/crates/ast/src/lang/scope.rs +++ b/crates/ast/src/lang/scope.rs @@ -203,13 +203,14 @@ impl Scope { pub fn lookup(&mut self, ident: &Ident, region: Region) -> Result { match self.idents.get(ident) { Some((symbol, _)) => Ok(*symbol), - None => Err(RuntimeError::LookupNotInScope( - Loc { + None => Err(RuntimeError::LookupNotInScope { + loc_name: Loc { region, value: ident.clone().into(), }, - self.idents.keys().map(|v| v.as_ref().into()).collect(), - )), + suggestion_options: self.idents.keys().map(|v| v.as_ref().into()).collect(), + underscored_suggestion_region: None, + }), } } diff --git a/crates/compiler/can/src/env.rs b/crates/compiler/can/src/env.rs index 685e10c046..5c052d2f07 100644 --- a/crates/compiler/can/src/env.rs +++ b/crates/compiler/can/src/env.rs @@ -124,18 +124,19 @@ impl<'a> Env<'a> { Ok(symbol) } None => { - let error = RuntimeError::LookupNotInScope( - Loc { + let error = RuntimeError::LookupNotInScope { + loc_name: Loc { value: Ident::from(ident), region, }, - scope + suggestion_options: scope .locals .ident_ids .ident_strs() .map(|(_, string)| string.into()) .collect(), - ); + underscored_suggestion_region: None, + }; Err(error) } } diff --git a/crates/compiler/can/src/expr.rs b/crates/compiler/can/src/expr.rs index 9e9b10d0f5..62ee13504b 100644 --- a/crates/compiler/can/src/expr.rs +++ b/crates/compiler/can/src/expr.rs @@ -1019,9 +1019,18 @@ pub fn canonicalize_expr<'a>( } ast::Expr::Underscore(name) => { // we parse underscores, but they are not valid expression syntax + let problem = roc_problem::can::RuntimeError::MalformedIdentifier( (*name).into(), - roc_parse::ident::BadIdent::Underscore(region.start()), + if name.is_empty() { + roc_parse::ident::BadIdent::UnderscoreAlone(region.start()) + } else { + roc_parse::ident::BadIdent::UnderscoreAtStart { + position: region.start(), + // Check if there's an ignored identifier with this name in scope (for better error messages) + declaration_region: scope.lookup_ignored_local(name), + } + }, region, ); diff --git a/crates/compiler/can/src/pattern.rs b/crates/compiler/can/src/pattern.rs index d881ad4257..4e96e604b2 100644 --- a/crates/compiler/can/src/pattern.rs +++ b/crates/compiler/can/src/pattern.rs @@ -379,6 +379,12 @@ pub fn canonicalize_pattern<'a>( Err(pattern) => pattern, } } + Underscore(name) => { + // An underscored identifier can't be used, but we'll still add it to the scope + // for better error messages if someone tries to use it. + scope.introduce_ignored_local(name, region); + Pattern::Underscore + } Tag(name) => { // Canonicalize the tag's name. Pattern::AppliedTag { @@ -479,8 +485,6 @@ pub fn canonicalize_pattern<'a>( ptype => unsupported_pattern(env, ptype, region), }, - Underscore(_) => Pattern::Underscore, - &NumLiteral(str) => match pattern_type { WhenBranch => match finish_parsing_num(str) { Err(_error) => { diff --git a/crates/compiler/can/src/scope.rs b/crates/compiler/can/src/scope.rs index d1f4656ba7..88a139a45d 100644 --- a/crates/compiler/can/src/scope.rs +++ b/crates/compiler/can/src/scope.rs @@ -41,6 +41,10 @@ pub struct Scope { /// Identifiers that are in scope, and defined in the current module pub locals: ScopedIdentIds, + + /// Ignored variables (variables that start with an underscore). + /// We won't intern them because they're only used during canonicalization for error reporting. + ignored_locals: VecMap, } impl Scope { @@ -65,6 +69,7 @@ impl Scope { abilities_store: starting_abilities_store, shadows: VecMap::default(), imports: default_imports, + ignored_locals: VecMap::default(), } } @@ -89,13 +94,17 @@ impl Scope { match self.scope_contains_ident(ident) { InScope(symbol, _) => Ok(symbol), NotInScope(_) | NotPresent => { - let error = RuntimeError::LookupNotInScope( - Loc { + // identifier not found + + let error = RuntimeError::LookupNotInScope { + loc_name: Loc { region, value: Ident::from(ident), }, - self.idents_in_scope().map(|v| v.as_ref().into()).collect(), - ); + suggestion_options: self.idents_in_scope().map(|v| v.as_ref().into()).collect(), + // Check if the user just forgot to remove an underscore from an ignored identifier + underscored_suggestion_region: self.lookup_ignored_local(ident), + }; Err(error) } @@ -418,11 +427,13 @@ impl Scope { // - exposed_ident_count: unchanged // - home: unchanged let aliases_count = self.aliases.len(); + let ignored_locals_count = self.ignored_locals.len(); let locals_snapshot = self.locals.in_scope.len(); let result = f(self); self.aliases.truncate(aliases_count); + self.ignored_locals.truncate(ignored_locals_count); // anything added in the inner scope is no longer in scope now for i in locals_snapshot..self.locals.in_scope.len() { @@ -444,6 +455,19 @@ impl Scope { pub fn gen_unique_symbol(&mut self) -> Symbol { Symbol::new(self.home, self.locals.gen_unique()) } + + /// Introduce a new ignored variable (variable starting with an underscore). + /// The underscore itself should not be included in `ident`. + pub fn introduce_ignored_local(&mut self, ident: &str, region: Region) { + self.ignored_locals.insert(ident.to_owned(), region); + } + + /// Lookup an ignored variable (variable starting with an underscore). + /// The underscore itself should not be included in `ident`. + /// Returns the source code region of the ignored variable if it's found. + pub fn lookup_ignored_local(&self, ident: &str) -> Option { + self.ignored_locals.get(&ident.to_owned()).copied() + } } pub fn create_alias( diff --git a/crates/compiler/can/tests/test_can.rs b/crates/compiler/can/tests/test_can.rs index bab2c3e447..7086931de0 100644 --- a/crates/compiler/can/tests/test_can.rs +++ b/crates/compiler/can/tests/test_can.rs @@ -332,7 +332,7 @@ mod test_can { matches!( problem, Problem::SignatureDefMismatch { .. } - | Problem::RuntimeError(RuntimeError::LookupNotInScope(_, _)) + | Problem::RuntimeError(RuntimeError::LookupNotInScope { .. }) ) })); } @@ -360,7 +360,7 @@ mod test_can { matches!( problem, Problem::SignatureDefMismatch { .. } - | Problem::RuntimeError(RuntimeError::LookupNotInScope(_, _)) + | Problem::RuntimeError(RuntimeError::LookupNotInScope { .. }) ) })); } diff --git a/crates/compiler/fmt/src/spaces.rs b/crates/compiler/fmt/src/spaces.rs index 5408814c09..84d8c36531 100644 --- a/crates/compiler/fmt/src/spaces.rs +++ b/crates/compiler/fmt/src/spaces.rs @@ -761,7 +761,15 @@ fn remove_spaces_bad_ident(ident: BadIdent) -> BadIdent { match ident { BadIdent::Start(_) => BadIdent::Start(Position::zero()), BadIdent::Space(e, _) => BadIdent::Space(e, Position::zero()), - BadIdent::Underscore(_) => BadIdent::Underscore(Position::zero()), + BadIdent::UnderscoreAlone(_) => BadIdent::UnderscoreAlone(Position::zero()), + BadIdent::UnderscoreInMiddle(_) => BadIdent::UnderscoreInMiddle(Position::zero()), + BadIdent::UnderscoreAtStart { + position: _, + declaration_region, + } => BadIdent::UnderscoreAtStart { + position: Position::zero(), + declaration_region, + }, BadIdent::QualifiedTag(_) => BadIdent::QualifiedTag(Position::zero()), BadIdent::WeirdAccessor(_) => BadIdent::WeirdAccessor(Position::zero()), BadIdent::WeirdDotAccess(_) => BadIdent::WeirdDotAccess(Position::zero()), diff --git a/crates/compiler/parse/src/ident.rs b/crates/compiler/parse/src/ident.rs index 59a04b3824..efd4758e26 100644 --- a/crates/compiler/parse/src/ident.rs +++ b/crates/compiler/parse/src/ident.rs @@ -3,7 +3,7 @@ use crate::parser::{BadInputError, EExpr, ParseResult, Parser}; use crate::state::State; use bumpalo::collections::vec::Vec; use bumpalo::Bump; -use roc_region::all::Position; +use roc_region::all::{Position, Region}; /// A tag, for example. Must start with an uppercase letter /// and then contain only letters and numbers afterwards - no dots allowed! @@ -254,7 +254,14 @@ pub enum BadIdent { Start(Position), Space(BadInputError, Position), - Underscore(Position), + UnderscoreAlone(Position), + UnderscoreInMiddle(Position), + UnderscoreAtStart { + position: Position, + /// If this variable was already declared in a pattern (e.g. \_x -> _x), + /// then this is where it was declared. + declaration_region: Option, + }, QualifiedTag(Position), WeirdAccessor(Position), WeirdDotAccess(Position), @@ -529,7 +536,7 @@ fn chomp_identifier_chain<'a>( // to give good error messages for this case Err(( chomped as u32 + 1, - BadIdent::Underscore(pos.bump_column(chomped as u32 + 1)), + BadIdent::UnderscoreInMiddle(pos.bump_column(chomped as u32 + 1)), )) } else if first_is_uppercase { // just one segment, starting with an uppercase letter; that's a tag diff --git a/crates/compiler/problem/src/can.rs b/crates/compiler/problem/src/can.rs index a81784b239..fdeb251f33 100644 --- a/crates/compiler/problem/src/can.rs +++ b/crates/compiler/problem/src/can.rs @@ -335,7 +335,11 @@ impl Problem { }) | Problem::RuntimeError(RuntimeError::UnsupportedPattern(region)) | Problem::RuntimeError(RuntimeError::MalformedPattern(_, region)) - | Problem::RuntimeError(RuntimeError::LookupNotInScope(Loc { region, .. }, _)) + | Problem::RuntimeError(RuntimeError::LookupNotInScope { + loc_name: Loc { region, .. }, + suggestion_options: _, + underscored_suggestion_region: _, + }) | Problem::RuntimeError(RuntimeError::OpaqueNotDefined { usage: Loc { region, .. }, .. @@ -505,7 +509,14 @@ pub enum RuntimeError { UnresolvedTypeVar, ErroneousType, - LookupNotInScope(Loc, MutSet>), + LookupNotInScope { + loc_name: Loc, + /// All of the names in scope (for the error message) + suggestion_options: MutSet>, + /// If the unfound variable is `name` and there's an ignored variable called `_name`, + /// this is the region where `_name` is defined (for the error message) + underscored_suggestion_region: Option, + }, OpaqueNotDefined { usage: Loc, opaques_in_scope: MutSet>, diff --git a/crates/compiler/test_syntax/tests/snapshots/malformed/malformed_ident_due_to_underscore.expr.result-ast b/crates/compiler/test_syntax/tests/snapshots/malformed/malformed_ident_due_to_underscore.expr.result-ast index b776a10113..1544a84253 100644 --- a/crates/compiler/test_syntax/tests/snapshots/malformed/malformed_ident_due_to_underscore.expr.result-ast +++ b/crates/compiler/test_syntax/tests/snapshots/malformed/malformed_ident_due_to_underscore.expr.result-ast @@ -2,7 +2,7 @@ Closure( [ @1-11 MalformedIdent( "the_answer", - Underscore( + UnderscoreInMiddle( @5, ), ), diff --git a/crates/compiler/test_syntax/tests/snapshots/malformed/underscore_expr_in_def.expr.result-ast b/crates/compiler/test_syntax/tests/snapshots/malformed/underscore_expr_in_def.expr.result-ast index 83779d17ea..d567cb848d 100644 --- a/crates/compiler/test_syntax/tests/snapshots/malformed/underscore_expr_in_def.expr.result-ast +++ b/crates/compiler/test_syntax/tests/snapshots/malformed/underscore_expr_in_def.expr.result-ast @@ -31,7 +31,7 @@ Defs( @5-8 SpaceBefore( MalformedIdent( "n_p", - Underscore( + UnderscoreInMiddle( @7, ), ), diff --git a/crates/reporting/src/error/canonicalize.rs b/crates/reporting/src/error/canonicalize.rs index 30f9e58aae..c0be61e751 100644 --- a/crates/reporting/src/error/canonicalize.rs +++ b/crates/reporting/src/error/canonicalize.rs @@ -1254,19 +1254,49 @@ fn to_bad_ident_expr_report<'b>( ]) } - Underscore(pos) => { - let region = Region::new(surroundings.start(), pos); + UnderscoreAlone(_pos) => { + alloc.stack([ + alloc.reflow("An underscore is being used as a variable here:"), + alloc.region(lines.convert_region(surroundings)), + alloc.concat([alloc + .reflow(r"An underscore can be used to ignore a value when pattern matching, but it cannot be used as a variable.")]), + ]) + } + + UnderscoreInMiddle(_pos) => { alloc.stack([ alloc.reflow("Underscores are not allowed in identifier names:"), - alloc.region_with_subregion( - lines.convert_region(surroundings), - lines.convert_region(region), - ), + alloc.region(lines.convert_region(surroundings)), alloc.concat([alloc .reflow(r"I recommend using camelCase. It's the standard style in Roc code!")]), ]) } + UnderscoreAtStart { + position: _pos, + declaration_region, + } => { + let line = "This variable's name starts with an underscore:"; + alloc.stack([ + match declaration_region { + None => alloc.reflow(line), + Some(declaration_region) => alloc.stack([ + alloc.reflow(line), + alloc.region(lines.convert_region(declaration_region)), + alloc.reflow("But then it is used here:"), + ]) + }, + alloc.region(lines.convert_region(surroundings)), + alloc.concat([ + alloc.reflow(r"A variable's name can only start with an underscore if the variable is unused. "), + match declaration_region { + None => alloc.reflow(r"But it looks like the variable is being used here!"), + Some(_) => alloc.reflow(r"Since you are using this variable, you could remove the underscore from its name in both places."), + } + ]), + ]) + } + BadOpaqueRef(pos) => { use BadIdentNext::*; let kind = "an opaque reference"; @@ -1409,7 +1439,13 @@ fn to_bad_ident_pattern_report<'b>( ]) } - Underscore(pos) => { + UnderscoreAlone(..) | UnderscoreAtStart { .. } => { + unreachable!( + "it's fine to have an underscore at the beginning of an identifier in a pattern" + ) + } + + UnderscoreInMiddle(pos) => { let region = Region::from_pos(pos.sub(1)); alloc.stack([ @@ -1580,8 +1616,19 @@ fn pretty_runtime_error<'b>( (title, doc) = report_shadowing(alloc, lines, original_region, shadow, kind); } - RuntimeError::LookupNotInScope(loc_name, options) => { - doc = not_found(alloc, lines, loc_name.region, &loc_name.value, options); + RuntimeError::LookupNotInScope { + loc_name, + suggestion_options: options, + underscored_suggestion_region, + } => { + doc = not_found( + alloc, + lines, + loc_name.region, + &loc_name.value, + options, + underscored_suggestion_region, + ); title = UNRECOGNIZED_NAME; } RuntimeError::CircularDef(entries) => { @@ -2219,6 +2266,7 @@ fn not_found<'b>( region: roc_region::all::Region, name: &Ident, options: MutSet>, + underscored_suggestion_region: Option, ) -> RocDocBuilder<'b> { let mut suggestions = suggest::sort( name.as_inline_str().as_str(), @@ -2234,7 +2282,15 @@ fn not_found<'b>( alloc.reflow(" missing up-top"), ]); - let default_yes = alloc.reflow("Did you mean one of these?"); + let default_yes = match underscored_suggestion_region { + Some(underscored_region) => alloc.stack([ + alloc.reflow("There is an ignored identifier of a similar name here:"), + alloc.region(lines.convert_region(underscored_region)), + alloc.reflow("Did you mean to remove the leading underscore?"), + alloc.reflow("If not, did you mean one of these?"), + ]), + None => alloc.reflow("Did you mean one of these?"), + }; let to_details = |no_suggestion_details, yes_suggestion_details| { if suggestions.is_empty() { diff --git a/crates/reporting/tests/test_reporting.rs b/crates/reporting/tests/test_reporting.rs index 9096c1bf1b..e4db5bd4df 100644 --- a/crates/reporting/tests/test_reporting.rs +++ b/crates/reporting/tests/test_reporting.rs @@ -10151,6 +10151,41 @@ In roc, functions are always written as a lambda, like{} "### ); + test_report!( + forgot_to_remove_underscore, + indoc!( + r#" + \_foo -> foo + "# + ), + |golden| pretty_assertions::assert_eq!( + golden, + indoc!( + r###"── UNRECOGNIZED NAME ───────────────────────────────────── /code/proj/Main.roc ─ + + Nothing is named `foo` in this scope. + + 4│ \_foo -> foo + ^^^ + + There is an ignored identifier of a similar name here: + + 4│ \_foo -> foo + ^^^^ + + Did you mean to remove the leading underscore? + + If not, did you mean one of these? + + Box + Bool + U8 + F64 + "### + ), + ) + ); + test_report!( call_with_underscore_identifier, indoc!( @@ -10162,17 +10197,102 @@ In roc, functions are always written as a lambda, like{} ), |golden| pretty_assertions::assert_eq!( golden, - &format!( + indoc!( r###"── SYNTAX PROBLEM ──────────────────────────────────────── /code/proj/Main.roc ─ -Underscores are not allowed in identifier names: + An underscore is being used as a variable here: -6│ f 1 _ 1 -{} + 6│ f 1 _ 1 + ^ -I recommend using camelCase. It's the standard style in Roc code! -"###, - " " // TODO make the reporter not insert extraneous spaces here in the first place! + An underscore can be used to ignore a value when pattern matching, but + it cannot be used as a variable. + "### + ), + ) + ); + + test_report!( + call_with_declared_identifier_starting_with_underscore, + indoc!( + r#" + f = \x, y, z -> x + y + z + + \a, _b -> f a _b 1 + "# + ), + |golden| pretty_assertions::assert_eq!( + golden, + indoc!( + r###"── SYNTAX PROBLEM ──────────────────────────────────────── /code/proj/Main.roc ─ + + This variable's name starts with an underscore: + + 6│ \a, _b -> f a _b 1 + ^^ + + But then it is used here: + + 6│ \a, _b -> f a _b 1 + ^^ + + A variable's name can only start with an underscore if the variable is + unused. Since you are using this variable, you could remove the + underscore from its name in both places. + "### + ), + ) + ); + + test_report!( + call_with_undeclared_identifier_starting_with_underscore, + indoc!( + r#" + f = \x, y, z -> x + y + z + + \a, _b -> f a _r 1 + "# + ), + |golden| pretty_assertions::assert_eq!( + golden, + indoc!( + r###" + ── SYNTAX PROBLEM ──────────────────────────────────────── /code/proj/Main.roc ─ + + This variable's name starts with an underscore: + + 6│ \a, _b -> f a _r 1 + ^^ + + A variable's name can only start with an underscore if the variable is + unused. But it looks like the variable is being used here! + "### + ), + ) + ); + + test_report!( + underscore_in_middle_of_identifier, + indoc!( + r#" + f = \x, y, z -> x + y + z + + \a, _b -> f a var_name 1 + "# + ), + |golden| pretty_assertions::assert_eq!( + golden, + indoc!( + r###" + ── SYNTAX PROBLEM ──────────────────────────────────────── /code/proj/Main.roc ─ + + Underscores are not allowed in identifier names: + + 6│ \a, _b -> f a var_name 1 + ^^^^^^^^ + + I recommend using camelCase. It's the standard style in Roc code! + "### ), ) );