From c28d6b9411cd817529bb3f0894e6b6ca837505fb Mon Sep 17 00:00:00 2001 From: Jared Ramirez Date: Sun, 29 Dec 2024 13:21:09 -0800 Subject: [PATCH] Make ListStart branch less brittle; Add ListEnd branch --- crates/compiler/load/tests/test_reporting.rs | 89 +++++++++++++++----- crates/reporting/src/error/parse.rs | 80 ++++++++++-------- 2 files changed, 110 insertions(+), 59 deletions(-) diff --git a/crates/compiler/load/tests/test_reporting.rs b/crates/compiler/load/tests/test_reporting.rs index 424642432d6..cc9e6a8d203 100644 --- a/crates/compiler/load/tests/test_reporting.rs +++ b/crates/compiler/load/tests/test_reporting.rs @@ -6433,59 +6433,102 @@ All branches in an `if` must have the same type! } #[test] - fn invalid_app_name() { + fn exposes_start() { report_header_problem_as( indoc!( r" - app foobar - exposes [main, @Foo] - imports [pf.Task, Base64] + module foobar [] " ), indoc!( r#" - ── WEIRD APP NAME in /code/proj/Main.roc ─────────────────────────────────────── + ── WEIRD EXPOSES in /code/proj/Main.roc ──────────────────────────────────────── - I am partway through parsing a header, but got stuck here: + I am partway through parsing a header, but I got stuck here: - 1│ app foobar - ^ + 1│ module foobar [] + ^ - I am expecting an application name next, like app "main" or - app "editor". App names are surrounded by quotation marks. + I was expecting an `exposes` list like + + [Animal, default, tame] "# ), ) } #[test] - fn unneeded_module_name() { + fn exposes_missing_comma() { report_header_problem_as( indoc!( r" - module foobar [] + module [value func] " ), indoc!( r#" - ── WEIRD MODULE NAME in /code/proj/Main.roc ──────────────────────────────────── + ── WEIRD EXPOSES in /code/proj/Main.roc ──────────────────────────────────────── - I am partway through parsing a header, but I got stuck here: + I am partway through parsing an `exposes` list, but I got stuck here: - 1│ module foobar [] - ^ + 1│ module [value func] + ^ - I am expecting a list of exports like + I was expecting a type name, value name or function name next, like - module [func, value] + [Animal, default, tame] + "# + ), + ) + } - or module params like + #[test] + fn exposes_end() { + report_header_problem_as( + indoc!( + r" + module [value + " + ), + indoc!( + r#" + ── WEIRD EXPOSES in /code/proj/Main.roc ──────────────────────────────────────── + + I am partway through parsing an `exposes` list, but I got stuck here: + + 1│ module [value + 2│ + ^ + + I was expecting a type name, value name or function name next, like - module { echo } -> [func, value] + [Animal, default, tame] + "# + ), + ) + } - If you're trying to specify a module name, recall that unlike - application names, module names are not specified in the header. - Instead, they are derived from the name of the module's filename. + #[test] + fn invalid_app_name() { + report_header_problem_as( + indoc!( + r" + app foobar + exposes [main, @Foo] + imports [pf.Task, Base64] + " + ), + indoc!( + r#" + ── WEIRD APP NAME in /code/proj/Main.roc ─────────────────────────────────────── + + I am partway through parsing a header, but got stuck here: + + 1│ app foobar + ^ + + I am expecting an application name next, like app "main" or + app "editor". App names are surrounded by quotation marks. "# ), ) diff --git a/crates/reporting/src/error/parse.rs b/crates/reporting/src/error/parse.rs index 75c069fd4cc..bae405748af 100644 --- a/crates/reporting/src/error/parse.rs +++ b/crates/reporting/src/error/parse.rs @@ -4049,20 +4049,24 @@ fn to_exposes_report<'a>( let severity = Severity::RuntimeError; match *parse_problem { - EExposes::ListEnd(pos) | // TODO: give this its own error message - EExposes::Identifier(pos) => { + // TODO: It would be nice to give ListEnd it's own error message. + // However with how parsing error are currently reported, ListEnd and + // Identifier are tightly coupled and Identifier is basically never + // "thrown". Splitting these up to make more specific error messages + // would require tweaking how parsing works. + EExposes::ListEnd(pos) | EExposes::Identifier(pos) => { let surroundings = Region::new(start, pos); let region = LineColumnRegion::from_pos(lines.convert_pos(pos)); let doc = alloc.stack([ - alloc.reflow(r"I am partway through parsing an `exposes` list, but I got stuck here:"), + alloc.concat([ + alloc.reflow("I am partway through parsing an "), + alloc.keyword("exposes"), + alloc.reflow(" list, but I got stuck here:"), + ]), alloc.region_with_subregion(lines.convert_region(surroundings), region, severity), - alloc.concat([alloc.reflow( - "I was expecting a type name, value name or function name next, like", - )]), - alloc - .parser_suggestion("[Animal, default, tame]") - .indent(4), + alloc.reflow("I was expecting a type name, value name or function name next, like"), + alloc.parser_suggestion("[Animal, default, tame]").indent(4), ]); Report { @@ -4085,9 +4089,7 @@ fn to_exposes_report<'a>( alloc.keyword("exposes"), alloc.reflow(" keyword next, like"), ]), - alloc - .parser_suggestion("[Animal, default, tame]") - .indent(4), + alloc.parser_suggestion("[Animal, default, tame]").indent(4), ]); Report { @@ -4100,7 +4102,20 @@ fn to_exposes_report<'a>( EExposes::Space(error, pos) => to_space_report(alloc, lines, filename, &error, pos), - EExposes::ListStart(pos@Position { offset: 7 }) => { + // TODO: Depending on the type of header we're parsing, it would be + // great to add more context to this error message. For example, if + // we're parsing a `module` then saying something like: + // + // I was expecting an exposes list like + // + // ... + // + // or module params like + // ... + // + // would be great. But currently we don't capture the type of header in + // the parse problem data type, so this isn't possible + EExposes::ListStart(pos) => { let surroundings = Region::new(start, pos); let region = LineColumnRegion::from_pos(lines.convert_pos(pos)); @@ -4108,40 +4123,33 @@ fn to_exposes_report<'a>( alloc.reflow(r"I am partway through parsing a header, but I got stuck here:"), alloc.region_with_subregion(lines.convert_region(surroundings), region, severity), alloc.concat([ - alloc.reflow("I am expecting a list of exports like"), - ]), - alloc - .parser_suggestion("module [func, value]") - .indent(4), - alloc.concat([ - alloc.reflow("or module params like"), - ]), - alloc - .parser_suggestion("module { echo } -> [func, value]") - .indent(4), - alloc.concat([ - alloc.reflow("If you're trying to specify a module name, recall that unlike "), - alloc.reflow("application names, module names are not specified in the header. "), - alloc.reflow("Instead, they are derived from the name of the module's filename."), + alloc.reflow("I was expecting an "), + alloc.keyword("exposes"), + alloc.reflow(" list like"), ]), + alloc.parser_suggestion("[Animal, default, tame]").indent(4), ]); - Report { filename, doc, - title: "WEIRD MODULE NAME".to_string(), + title: "WEIRD EXPOSES".to_string(), severity, } - }, + } // If you're adding or changing syntax, please handle the case with a // good error message above instead of adding more unhandled cases below. - EExposes::Open(pos) | - EExposes::IndentExposes(pos) | - EExposes::IndentListStart(pos) | - EExposes::ListStart(pos) => - to_unhandled_parse_error_report(alloc, lines, filename, format!("{:?}", parse_problem), pos, start) + EExposes::Open(pos) | EExposes::IndentExposes(pos) | EExposes::IndentListStart(pos) => { + to_unhandled_parse_error_report( + alloc, + lines, + filename, + format!("{:?}", parse_problem), + pos, + start, + ) + } } }