From 4d10c22442fb574dc5f6237e39f5c5391f01fab3 Mon Sep 17 00:00:00 2001 From: Emi Simpson Date: Thu, 24 Feb 2022 20:37:16 -0500 Subject: [PATCH 1/4] =?UTF-8?q?=F0=9F=90=9B=EF=B8=8F=20Handle=20unimported?= =?UTF-8?q?=20modules=20properly?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit helpful error, not panic! Closes #2422 --- ast/src/lang/env.rs | 18 ++++++---- compiler/can/src/annotation.rs | 4 ++- compiler/can/src/env.rs | 18 ++++++---- compiler/load/src/file.rs | 2 ++ compiler/mono/src/ir.rs | 5 +++ compiler/problem/src/can.rs | 27 +++++++++++++++ reporting/src/error/canonicalize.rs | 51 ++++++++++++++++++----------- 7 files changed, 93 insertions(+), 32 deletions(-) diff --git a/ast/src/lang/env.rs b/ast/src/lang/env.rs index 3f9e17e3d5..1b04f417d2 100644 --- a/ast/src/lang/env.rs +++ b/ast/src/lang/env.rs @@ -160,12 +160,17 @@ impl<'a> Env<'a> { }) } }, - None => { - panic!( - "Module {} exists, but is not recorded in dep_idents", - module_name - ) - } + None => Err(RuntimeError::ModuleNotImported { + module_name, + imported_modules: self + .dep_idents + .keys() + .filter_map(|module_id| self.module_ids.get_name(*module_id)) + .map(|module_name| module_name.as_ref().into()) + .collect(), + region, + module_exists: true, + }), } } } @@ -177,6 +182,7 @@ impl<'a> Env<'a> { .map(|string| string.as_ref().into()) .collect(), region, + module_exists: false, }), } } diff --git a/compiler/can/src/annotation.rs b/compiler/can/src/annotation.rs index 5cd1b22164..593c0f1189 100644 --- a/compiler/can/src/annotation.rs +++ b/compiler/can/src/annotation.rs @@ -130,7 +130,9 @@ fn make_apply_symbol( // it was imported but it doesn't expose this ident. env.problem(roc_problem::can::Problem::RuntimeError(problem)); - Err(Type::Erroneous(Problem::UnrecognizedIdent((*ident).into()))) + // A failed import should have already been reported through + // roc_can::env::Env::qualified_lookup's checks + Err(Type::Erroneous(Problem::SolvedTypeError)) } } } diff --git a/compiler/can/src/env.rs b/compiler/can/src/env.rs index 46f627d7d9..e1216c42eb 100644 --- a/compiler/can/src/env.rs +++ b/compiler/can/src/env.rs @@ -124,12 +124,17 @@ impl<'a> Env<'a> { }) } }, - None => { - panic!( - "Module {} exists, but is not recorded in dep_idents", - module_name - ) - } + None => Err(RuntimeError::ModuleNotImported { + module_name, + imported_modules: self + .dep_idents + .keys() + .filter_map(|module_id| self.module_ids.get_name(*module_id)) + .map(|module_name| module_name.as_ref().into()) + .collect(), + region, + module_exists: true, + }), } } } @@ -141,6 +146,7 @@ impl<'a> Env<'a> { .map(|string| string.as_ref().into()) .collect(), region, + module_exists: false, }), } } diff --git a/compiler/load/src/file.rs b/compiler/load/src/file.rs index 0a119ecb23..e06150e997 100644 --- a/compiler/load/src/file.rs +++ b/compiler/load/src/file.rs @@ -3227,6 +3227,8 @@ fn canonicalize_and_constrain<'a>( module_timing.canonicalize = canonicalize_end.duration_since(canonicalize_start).unwrap(); + use roc_can::module::ModuleOutput; + use std::process; match canonicalized { Ok(module_output) => { // Generate documentation information diff --git a/compiler/mono/src/ir.rs b/compiler/mono/src/ir.rs index 0e0c39019a..fdadd5f2f7 100644 --- a/compiler/mono/src/ir.rs +++ b/compiler/mono/src/ir.rs @@ -8629,6 +8629,11 @@ fn match_on_lambda_set<'a>( env.arena.alloc(result), ) } + Layout::Struct([]) => { + // This is a lambda set with no associated lambdas, often produced as a result + // of a runtime error at another point in the code. + Stmt::RuntimeError("Cannot have a lambda set with zero variants") + } Layout::Struct(fields) => { let function_symbol = lambda_set.set[0].0; diff --git a/compiler/problem/src/can.rs b/compiler/problem/src/can.rs index a4eeddebe7..eb7a9c22aa 100644 --- a/compiler/problem/src/can.rs +++ b/compiler/problem/src/can.rs @@ -161,10 +161,37 @@ pub enum RuntimeError { region: Region, exposed_values: Vec, }, + /// A module was referenced, but hasn't been imported anywhere in the program + /// + /// An example would be: + /// ```roc + /// app "hello" + /// packages { pf: "platform" } + /// imports [ pf.Stdout] + /// provides [ main ] to pf + /// + /// main : Task.Task {} [] // Task isn't imported! + /// main = Stdout.line "I'm a Roc application!" + /// ``` ModuleNotImported { + /// The name of the module that was referenced module_name: ModuleName, + /// A list of modules which *have* been imported imported_modules: MutSet>, + /// Where the problem occured region: Region, + /// Whether or not the module exists at all + /// + /// This is used to suggest that the user import the module, as opposed to fix a + /// typo in the spelling. For example, if the user typed `Task`, and the platform + /// exposes a `Task` module that hasn't been imported, we can sugguest that they + /// add the import statement. + /// + /// On the other hand, if the user typed `Tesk`, they might want to check their + /// spelling. + /// + /// If unsure, this should be set to `false` + module_exists: bool, }, InvalidPrecedence(PrecedenceProblem, Region), MalformedIdentifier(Box, roc_parse::ident::BadIdent, Region), diff --git a/reporting/src/error/canonicalize.rs b/reporting/src/error/canonicalize.rs index 3759c26afa..e20e451b9e 100644 --- a/reporting/src/error/canonicalize.rs +++ b/reporting/src/error/canonicalize.rs @@ -999,8 +999,16 @@ fn pretty_runtime_error<'b>( module_name, imported_modules, region, + module_exists, } => { - doc = module_not_found(alloc, lines, region, &module_name, imported_modules); + doc = module_not_found( + alloc, + lines, + region, + &module_name, + imported_modules, + module_exists, + ); title = MODULE_NOT_IMPORTED; } @@ -1414,34 +1422,39 @@ fn not_found<'b>( ]) } +/// Generate a message informing the user that a module was referenced, but not found +/// +/// See [`roc_problem::can::ModuleNotImported`] fn module_not_found<'b>( alloc: &'b RocDocAllocator<'b>, lines: &LineInfo, region: roc_region::all::Region, name: &ModuleName, options: MutSet>, + module_exists: bool, ) -> RocDocBuilder<'b> { - let mut suggestions = - suggest::sort(name.as_str(), options.iter().map(|v| v.as_ref()).collect()); - suggestions.truncate(4); + // If the module exists, sugguest that the user import it + let details = if module_exists { + // TODO: Maybe give an example of how to do that + alloc.reflow("Did you mean to import it?") + } else { + // If the module might not exist, sugguest that it's a typo + let mut suggestions = + suggest::sort(name.as_str(), options.iter().map(|v| v.as_ref()).collect()); + suggestions.truncate(4); - let default_no = alloc.concat(vec![ - alloc.reflow("Is there an "), - alloc.keyword("import"), - alloc.reflow(" or "), - alloc.keyword("exposing"), - alloc.reflow(" missing up-top"), - ]); - - let default_yes = alloc - .reflow("Is there an import missing? Perhaps there is a typo. Did you mean one of these?"); - - let to_details = |no_suggestion_details, yes_suggestion_details| { if suggestions.is_empty() { - no_suggestion_details + // We don't have any recommended spelling corrections + alloc.concat(vec![ + alloc.reflow("Is there an "), + alloc.keyword("import"), + alloc.reflow(" or "), + alloc.keyword("exposing"), + alloc.reflow(" missing up-top"), + ]) } else { alloc.stack(vec![ - yes_suggestion_details, + alloc.reflow("Is there an import missing? Perhaps there is a typo. Did you mean one of these?"), alloc .vcat(suggestions.into_iter().map(|v| alloc.string(v.to_string()))) .indent(4), @@ -1456,6 +1469,6 @@ fn module_not_found<'b>( alloc.reflow("` module is not imported:"), ]), alloc.region(lines.convert_region(region)), - to_details(default_no, default_yes), + details, ]) } From 7db55e4662b3f394de7b8a00d34442a9bf0392cb Mon Sep 17 00:00:00 2001 From: Emi Simpson Date: Fri, 25 Feb 2022 11:41:47 -0500 Subject: [PATCH 2/4] =?UTF-8?q?=F0=9F=93=9D=EF=B8=8F=20Add=20a=20test=20fo?= =?UTF-8?q?r=20#2422?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- reporting/tests/test_reporting.rs | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/reporting/tests/test_reporting.rs b/reporting/tests/test_reporting.rs index 356a123373..c16690ec0d 100644 --- a/reporting/tests/test_reporting.rs +++ b/reporting/tests/test_reporting.rs @@ -8274,4 +8274,35 @@ I need all branches in an `if` to have the same type! ), ) } + + #[test] + fn unimported_modules_reported() { + report_problem_as( + indoc!( + r#" + main : Task.Task {} [] + main = "whatever man you don't even know my type" + main + "# + ), + indoc!( + r#" + ── MODULE NOT IMPORTED ───────────────────────────────────────────────────────── + + The `Task` module is not imported: + + 1│ main : Task.Task {} [] + ^^^^^^^^^^^^^^^ + + Is there an import missing? Perhaps there is a typo. Did you mean one + of these? + + Test + List + Num + Set + "# + ), + ) + } } From 10f665f6c5fe35f13f308af56b4778c8ec0288bd Mon Sep 17 00:00:00 2001 From: Emi Simpson Date: Fri, 25 Feb 2022 11:46:46 -0500 Subject: [PATCH 3/4] =?UTF-8?q?Amend=204d10c22:=20=F0=9F=90=9B=20Fix=20a?= =?UTF-8?q?=20typo=20in=20doccomment=20for=20ModuleNotImported?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit blammed by the spellchecker! how embarrassing --- compiler/problem/src/can.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/problem/src/can.rs b/compiler/problem/src/can.rs index 76bf36ef2d..1adac511cb 100644 --- a/compiler/problem/src/can.rs +++ b/compiler/problem/src/can.rs @@ -188,7 +188,7 @@ pub enum RuntimeError { module_name: ModuleName, /// A list of modules which *have* been imported imported_modules: MutSet>, - /// Where the problem occured + /// Where the problem occurred region: Region, /// Whether or not the module exists at all /// From 043923c62bdf0cb248ccc2e6698f545e286960c9 Mon Sep 17 00:00:00 2001 From: Emi Simpson Date: Fri, 25 Feb 2022 13:14:45 -0500 Subject: [PATCH 4/4] =?UTF-8?q?=F0=9F=A7=B9=20Clean=20up=20unused=20import?= =?UTF-8?q?s?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- compiler/load/src/file.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/compiler/load/src/file.rs b/compiler/load/src/file.rs index f870dfed71..0cfa433b12 100644 --- a/compiler/load/src/file.rs +++ b/compiler/load/src/file.rs @@ -3227,8 +3227,6 @@ fn canonicalize_and_constrain<'a>( module_timing.canonicalize = canonicalize_end.duration_since(canonicalize_start).unwrap(); - use roc_can::module::ModuleOutput; - use std::process; match canonicalized { Ok(module_output) => { // Generate documentation information