Improve error messages involving ignored variables

Fix #3987

crates/ast/src/lang/env.rs

@@ -121,16 +121,18 @@ impl<'a> Env<'a> {
 
                             Ok(symbol)
                         }
-                        None => Err(RuntimeError::LookupNotInScope(
+                        None => Err(RuntimeError::LookupNotInScope {
-                            Loc {
+                            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) {

crates/ast/src/lang/scope.rs

@@ -203,13 +203,14 @@ impl Scope {
     pub fn lookup(&mut self, ident: &Ident, region: Region) -> Result<Symbol, RuntimeError> {
         match self.idents.get(ident) {
             Some((symbol, _)) => Ok(*symbol),
-            None => Err(RuntimeError::LookupNotInScope(
+            None => Err(RuntimeError::LookupNotInScope {
-                Loc {
+                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,
+            }),
         }
     }
 

crates/compiler/can/src/env.rs

@@ -124,18 +124,19 @@ impl<'a> Env<'a> {
                     Ok(symbol)
                 }
                 None => {
-                    let error = RuntimeError::LookupNotInScope(
+                    let error = RuntimeError::LookupNotInScope {
-                        Loc {
+                        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)
                 }
             }

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

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) => {

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<String, Region>,
 }
 
 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(
+                // identifier not found
-                    Loc {
+
+                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<Region> {
+        self.ignored_locals.get(&ident.to_owned()).copied()
+    }
 }
 
 pub fn create_alias(

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 { .. })
             )
         }));
     }

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

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<Region>,
+    },
     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

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<Ident>, MutSet<Box<str>>),
+    LookupNotInScope {
+        loc_name: Loc<Ident>,
+        /// All of the names in scope (for the error message)
+        suggestion_options: MutSet<Box<str>>,
+        /// 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<Region>,
+    },
     OpaqueNotDefined {
         usage: Loc<Ident>,
         opaques_in_scope: MutSet<Box<str>>,

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

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

crates/reporting/src/error/canonicalize.rs

@@ -1254,19 +1254,49 @@ fn to_bad_ident_expr_report<'b>(
             ])
         }
 
-        Underscore(pos) => {
+        UnderscoreAlone(_pos) => {
-            let region = Region::new(surroundings.start(), 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(
+                alloc.region(lines.convert_region(surroundings)),
-                    lines.convert_region(surroundings),
-                    lines.convert_region(region),
-                ),
                 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) => {
+        RuntimeError::LookupNotInScope {
-            doc = not_found(alloc, lines, loc_name.region, &loc_name.value, options);
+            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<Box<str>>,
+    underscored_suggestion_region: Option<Region>,
 ) -> 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() {

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!
+                An underscore can be used to ignore a value when pattern matching, but
-"###,
+                it cannot be used as a variable.
-                "  " // TODO make the reporter not insert extraneous spaces here in the first place!
+                "###
+            ),
+        )
+    );
+
+    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!
+                "###
             ),
         )
     );