From d3bfc598d6aecad942e1d5ab44e49e5616a90b28 Mon Sep 17 00:00:00 2001 From: Markus Westerlind Date: Sun, 27 Dec 2020 12:16:09 +0100 Subject: [PATCH] fix: Provide the type of imported modules with errors --- Cargo.lock | 1 + Cargo.toml | 3 +- base/src/error.rs | 60 ++++++++++++++++++++++++++++++++++ check/tests/support/mod.rs | 9 +++-- src/compiler_pipeline.rs | 67 +++++--------------------------------- src/import.rs | 52 +++++++++++++++++++++-------- src/lib.rs | 5 ++- src/query.rs | 53 ++++++++++++++++++++++++++---- tests/error.rs | 41 +++++++++++++++++++++++ vm/src/macros.rs | 21 +++++++----- 10 files changed, 221 insertions(+), 91 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9251269444..d16314161c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1104,6 +1104,7 @@ dependencies = [ "criterion", "either", "env_logger 0.7.1", + "expect-test", "futures 0.3.5", "gluon-salsa", "gluon_base", diff --git a/Cargo.toml b/Cargo.toml index 54042ad593..fa4f50f404 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -75,10 +75,11 @@ little-skeptic = { version = "0.15.0", optional = true } walkdir = "2" [dev-dependencies] +anyhow = "1" criterion = "0.3" collect-mac = "0.1.0" env_logger = "0.7" -anyhow = "1" +expect-test = "1" thiserror = "1" insta = "0.16" pretty_assertions = "0.6" diff --git a/base/src/error.rs b/base/src/error.rs index 6914bf9f31..47b1d28358 100644 --- a/base/src/error.rs +++ b/base/src/error.rs @@ -374,3 +374,63 @@ impl AsDiagnostic for Box { Diagnostic::error().with_message(self.to_string()) } } + +pub type SalvageResult = std::result::Result>; + +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct Salvage { + pub value: Option, + pub error: E, +} + +impl fmt::Display for Salvage +where + E: fmt::Display, +{ + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "{}", self.error) + } +} + +impl Salvage { + pub fn map(self, f: impl FnOnce(T) -> U) -> Salvage { + Salvage { + value: self.value.map(f), + error: self.error, + } + } + + pub fn map_err(self, f: impl FnOnce(E) -> U) -> Salvage { + Salvage { + value: self.value, + error: f(self.error), + } + } + + pub fn get_value(self) -> std::result::Result { + self.value.ok_or(self.error) + } + + pub fn err_into(self) -> Salvage + where + F: From, + { + let Salvage { value, error } = self; + Salvage { + value, + error: error.into(), + } + } +} + +impl From for Salvage { + fn from(error: E) -> Self { + Salvage { value: None, error } + } +} + +impl From>> for InFile { + fn from(s: Salvage>) -> Self { + s.error + } +} diff --git a/check/tests/support/mod.rs b/check/tests/support/mod.rs index 419e5f6dd1..ad5f09289b 100644 --- a/check/tests/support/mod.rs +++ b/check/tests/support/mod.rs @@ -439,13 +439,18 @@ macro_rules! test_check { } macro_rules! test_check_err { - ($name:ident, $source:expr, $($id: pat),+) => { + ($name:ident, $source:expr, $($id: pat),+ $(,)? $(=> $ty: expr)?) => { #[test] fn $name() { let _ = env_logger::try_init(); let text = $source; - let result = support::typecheck(text); + let (expr, result) = support::typecheck_expr(text); assert_err!([$source] result, $($id),+); + $( + use crate::base::ast::Typed; + assert_eq!(expr.expr().env_type_of(&crate::base::ast::EmptyEnv::default()).to_string(), $ty); + )? + let _ = expr; } }; } diff --git a/src/compiler_pipeline.rs b/src/compiler_pipeline.rs index a9fd7adf75..bf9a4b6fc0 100644 --- a/src/compiler_pipeline.rs +++ b/src/compiler_pipeline.rs @@ -9,7 +9,6 @@ use std::{ borrow::{Borrow, BorrowMut, Cow}, - fmt, result::Result as StdResult, sync::Arc, }; @@ -44,58 +43,9 @@ use crate::{ pub type BoxFuture<'vm, T, E> = std::pin::Pin> + Send + 'vm>>; -pub type SalvageResult = StdResult>; +pub type SalvageResult = crate::base::error::SalvageResult; -#[derive(Clone, Debug, Eq, PartialEq)] -pub struct Salvage { - pub value: Option, - pub error: E, -} - -impl fmt::Display for Salvage -where - E: fmt::Display, -{ - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "{}", self.error) - } -} - -impl Salvage { - pub fn map(self, f: impl FnOnce(T) -> U) -> Salvage { - Salvage { - value: self.value.map(f), - error: self.error, - } - } - - pub fn get_value(self) -> std::result::Result { - self.value.ok_or(self.error) - } - - pub fn err_into(self) -> Salvage - where - F: From, - { - let Salvage { value, error } = self; - Salvage { - value, - error: error.into(), - } - } -} - -impl From for Salvage { - fn from(error: E) -> Self { - Salvage { value: None, error } - } -} - -impl From>> for InFile { - fn from(s: Salvage>) -> Self { - s.error - } -} +pub use crate::base::error::Salvage; impl From> for Error { fn from(s: Salvage) -> Self { @@ -648,19 +598,20 @@ where ) { Ok(typ) => typ, Err(error) => { + let typ = expr + .borrow_mut() + .expr() + .try_type_of(&env(&*compiler.database)) + .unwrap_or_else(|_| thread.global_env().type_cache().error()); return Err(Salvage { value: Some(TypecheckValue { - typ: expr - .borrow_mut() - .expr() - .try_type_of(&env(&*compiler.database)) - .unwrap_or_else(|_| thread.global_env().type_cache().error()), + typ, expr, metadata_map, metadata, }), error, - }) + }); } }; diff --git a/src/import.rs b/src/import.rs index c0e249a57e..957b5a591d 100644 --- a/src/import.rs +++ b/src/import.rs @@ -32,14 +32,14 @@ use crate::base::{ use crate::vm::{ self, gc::Trace, - macros::{Error as MacroError, Macro, MacroExpander, MacroFuture}, + macros::{Error as MacroError, LazyMacroResult, Macro, MacroExpander, MacroFuture}, thread::{RootedThread, Thread}, vm::VmEnv, ExternLoader, ExternModule, }; use crate::{ - compiler_pipeline::SalvageResult, + compiler_pipeline::{Salvage, SalvageResult}, query::{AsyncCompilation, Compilation, CompilerDatabase}, IoError, ModuleCompiler, ThreadExt, }; @@ -99,7 +99,17 @@ impl Importer for DefaultImporter { _vm: &Thread, modulename: &str, ) -> SalvageResult { - let value = compiler.database.global(modulename.to_string()).await?; + let result = compiler.database.global(modulename.to_string()).await; + // Forcibly load module_type so we can salvage a type for the error if necessary + let _ = compiler + .database + .module_type(modulename.to_string(), None) + .await; + + let value = result.map_err(|error| { + let value = compiler.database.peek_module_type(modulename); + Salvage { value, error } + })?; Ok(value.typ) } } @@ -534,14 +544,18 @@ where let result = std::panic::AssertUnwindSafe(db.import(modulename)) .catch_unwind() .await - .map(|r| r.map_err(|err| MacroError::message(err.to_string()))) + .map(|r| { + r.map_err(|salvage| { + salvage.map_err(|err| MacroError::message(err.to_string())) + }) + }) .unwrap_or_else(|err| { - Err(MacroError::message( + Err(Salvage::from(MacroError::message( err.downcast::() .map(|s| *s) .or_else(|e| e.downcast::<&str>().map(|s| String::from(&s[..]))) .unwrap_or_else(|_| "Unknown panic".to_string()), - )) + ))) }); // Drop the database before sending the result, otherwise the forker may drop before the forked database drop(db); @@ -549,23 +563,35 @@ where })) .unwrap(); return Box::pin(async move { - Ok(From::from(move || { - rx.map(move |r| { - r.unwrap_or_else(|err| Err(MacroError::new(Error::String(err.to_string())))) - .map(move |id| pos::spanned(span, Expr::Ident(id)).into()) - }) + Ok(LazyMacroResult::from(move || { + async move { + rx.await + .unwrap_or_else(|err| { + Err(Salvage::from(MacroError::new(Error::String( + err.to_string(), + )))) + }) + .map(|id| pos::spanned(span, Expr::Ident(id))) + .map_err(|salvage| { + salvage.map(|id| pos::spanned(span, Expr::Ident(id))) + }) + } .boxed() })) }); } Box::pin(async move { - Ok(From::from(move || { + Ok(LazyMacroResult::from(move || { async move { let result = db .import(modulename) .await - .map_err(|err| MacroError::message(err.to_string())) + .map_err(|salvage| { + salvage + .map(|id| pos::spanned(span, Expr::Ident(id))) + .map_err(|err| MacroError::message(err.to_string())) + }) .map(move |id| pos::spanned(span, Expr::Ident(id))); drop(db); result diff --git a/src/lib.rs b/src/lib.rs index a244db7726..6e20b746a9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -651,7 +651,10 @@ pub trait ThreadExt: Send + Sync { } let mut db = vm.get_database(); - db.import(module_name).await.map(|_| ()) + db.import(module_name) + .await + .map(|_| ()) + .map_err(|err| err.error) } /// Loads `filename` and compiles and runs its input by calling `load_script` diff --git a/src/query.rs b/src/query.rs index b9b41b65a1..2f9f7dadd5 100644 --- a/src/query.rs +++ b/src/query.rs @@ -222,21 +222,30 @@ impl crate::query::CompilationBase for CompilerDatabase { TypecheckedSourceModuleQuery .in_db(self) .peek(&(key.into(), None)) - .and_then(|r| r.ok()) + .and_then(|r| match r { + Ok(t) => Some(t), + Err(salvage) => salvage.value, + }) } fn peek_module_type(&self, key: &str) -> Option { ModuleTypeQuery .in_db(self) .peek(&(key.into(), None)) - .and_then(|r| r.ok()) + .and_then(|r| match r { + Ok(t) => Some(t), + Err(salvage) => salvage.value, + }) } fn peek_module_metadata(&self, key: &str) -> Option> { ModuleMetadataQuery .in_db(self) .peek(&(key.into(), None)) - .and_then(|r| r.ok()) + .and_then(|r| match r { + Ok(t) => Some(t), + Err(salvage) => salvage.value, + }) } fn peek_core_expr(&self, key: &str) -> Option> { @@ -421,8 +430,8 @@ pub trait Compilation: CompilationBase { expected_type: Option, ) -> StdResult>, Error>; - #[salsa::cycle(recover_cycle)] - async fn import(&self, module: String) -> StdResult, Error>; + #[salsa::cycle(recover_cycle_salvage)] + async fn import(&self, module: String) -> SalvageResult, Error>; #[doc(hidden)] #[salsa::cycle(recover_cycle)] @@ -474,6 +483,31 @@ fn recover_cycle( .into()) } +fn recover_cycle_salvage( + _db: &dyn Compilation, + cycle: &[String], + module: &String, +) -> SalvageResult { + let mut cycle: Vec<_> = cycle + .iter() + .filter(|k| k.starts_with("import(")) + .map(|k| { + k.trim_matches(|c: char| c != '"') + .trim_matches('"') + .trim_start_matches('@') + .to_string() + }) + .collect(); + cycle.pop(); + Err( + Error::from(macros::Error::new(crate::import::Error::CyclicDependency( + module.to_string(), + cycle, + ))) + .into(), + ) +} + fn get_extern_global(db: &dyn Compilation, name: &str) -> Option { if db.compiler().state().extern_globals.contains(name) { unsafe { @@ -646,7 +680,7 @@ async fn compiled_module( async fn import( db: &mut OwnedDb<'_, dyn Compilation + '_>, modulename: String, -) -> StdResult, Error> { +) -> SalvageResult, Error> { assert!(!modulename.starts_with('@')); let thread = db.thread().root_thread(); @@ -658,7 +692,12 @@ async fn import( let compiler = db.compiler(); compiler.collect_garbage(); - let typ = result?; + let typ = result.map_err(|salvage| { + salvage.map(|typ| TypedIdent { + name: name.clone(), + typ, + }) + })?; Ok(TypedIdent { name, typ }) } diff --git a/tests/error.rs b/tests/error.rs index 45beafe836..3c08c30f19 100644 --- a/tests/error.rs +++ b/tests/error.rs @@ -1,3 +1,5 @@ +use expect_test::expect; + use gluon::{ base, check::typecheck::TypeError, compiler_pipeline::*, parser, vm::Error as VMError, Error, ThreadExt, @@ -95,3 +97,42 @@ fn undefined_infix() { _ => panic!(), } } + +#[test] +fn module_imports_provide_a_type_despite_internal_errors() { + let _ = ::env_logger::try_init(); + let vm = support::make_vm(); + let text = r#" + type Test = () + let error : () = "" + { Test } + "#; + let _ = vm.load_script("test", text); + let result = vm.run_expr::( + "test2", + r#" + let { Test } = import! test + 3.14 + "#, + ); + expect![[r#" + error: error: Expected the following types to be equal + Expected: () + Found: String + 1 errors were found during unification: + Types do not match: + Expected: () + Found: String + ┌─ test:3:26 + │ + 3 │ let error : () = "" + │ ^^ + + + ┌─ test2:2:24 + │ + 2 │ let { Test } = import! test + │ ^^^^^^^^^^^^ + + "#]].assert_eq(&result.unwrap_err().to_string()); +} diff --git a/vm/src/macros.rs b/vm/src/macros.rs index 25d3a16f0c..e82b36f0cc 100644 --- a/vm/src/macros.rs +++ b/vm/src/macros.rs @@ -17,7 +17,7 @@ use gluon_codegen::Trace; use crate::base::{ ast::{self, Expr, MutVisitor, SpannedExpr}, - error::{AsDiagnostic, Errors as BaseErrors}, + error::{AsDiagnostic, Errors as BaseErrors, Salvage, SalvageResult}, fnv::FnvMap, pos, pos::{BytePos, Spanned}, @@ -34,20 +34,22 @@ pub type SpannedError = Spanned; pub type Errors = BaseErrors; pub type MacroResult<'ast> = Result, Error>; +pub type SalvageMacroResult<'ast> = SalvageResult, Error>; pub enum LazyMacroResult<'ast> { Done(SpannedExpr<'ast, Symbol>), Lazy( Box< - dyn for<'a> FnOnce() -> Pin> + Send + 'ast>> - + Send + dyn for<'a> FnOnce() -> Pin< + Box> + Send + 'ast>, + > + Send + 'ast, >, ), } impl<'ast> LazyMacroResult<'ast> { - async fn compute(self) -> MacroResult<'ast> { + async fn compute(self) -> SalvageMacroResult<'ast> { match self { Self::Done(r) => Ok(r), Self::Lazy(f) => f().await, @@ -63,8 +65,9 @@ impl<'ast> From> for LazyMacroResult<'ast> { impl<'ast, F> From for LazyMacroResult<'ast> where - for<'a> F: - FnOnce() -> Pin> + Send + 'ast>> + Send + 'ast, + for<'a> F: FnOnce() -> Pin> + Send + 'ast>> + + Send + + 'ast, { fn from(r: F) -> Self { Self::Lazy(Box::new(r)) @@ -495,9 +498,9 @@ impl<'a> MacroExpander<'a> { let expr = { expr }; let new_expr = match result { Ok(replacement) => replacement.value, - Err(err) => { - self.errors.push(pos::spanned(expr.span, err)); - Expr::Error(None) + Err(Salvage { error, value }) => { + self.errors.push(pos::spanned(expr.span, error)); + value.map_or_else(|| Expr::Error(None), |e| e.value) } };