fix(check): Typecheck a module even if macro expansion fails

If an imported module does not exist or has a type error we still want to get type errors from the module that called `import!`.
gluon-lang · Nov 15, 2017 · 5f78fec · 5f78fec
1 parent 5347278
commit 5f78fec
Show file tree

Hide file tree

Showing 16 changed files with 191 additions and 105 deletions.
diff --git a/base/src/ast.rs b/base/src/ast.rs
@@ -269,7 +269,10 @@ pub enum Expr<Id> {
     /// A group of sequenced expressions
     Block(Vec<SpannedExpr<Id>>),
     /// An invalid expression
-    Error,
+    Error(
+        /// Provides a hint of what type the expression would have, if any
+        Option<ArcType<Id>>,
+    ),
 }
 
 #[derive(Clone, PartialEq, Debug)]
@@ -406,7 +409,7 @@ pub fn walk_mut_expr<V: ?Sized + MutVisitor>(v: &mut V, e: &mut SpannedExpr<V::I
         }
         Expr::TypeBindings(_, ref mut expr) => v.visit_expr(expr),
         Expr::Ident(ref mut id) => v.visit_ident(id),
-        Expr::Literal(..) | Expr::Error => (),
+        Expr::Literal(..) | Expr::Error(..) => (),
     }
 }
 
@@ -529,7 +532,7 @@ pub fn walk_expr<'a, V: ?Sized + Visitor<'a>>(v: &mut V, e: &'a SpannedExpr<V::I
         }
         Expr::TypeBindings(_, ref expr) => v.visit_expr(expr),
         Expr::Ident(ref id) => v.visit_typ(&id.typ),
-        Expr::Literal(..) | Expr::Error => (),
+        Expr::Literal(..) | Expr::Error(..) => (),
     }
 }
 
@@ -624,7 +627,7 @@ impl Typed for Expr<Symbol> {
             Expr::Array(ref array) => array.typ.clone(),
             Expr::Lambda(ref lambda) => lambda.id.typ.clone(),
             Expr::Block(ref exprs) => exprs.last().expect("Expr in block").env_type_of(env),
-            Expr::Error => Type::hole(),
+            Expr::Error(ref typ) => typ.clone().unwrap_or_else(|| Type::hole()),
         }
     }
 }
@@ -673,9 +676,8 @@ fn get_return_type(env: &TypeEnv, alias_type: &ArcType, arg_count: usize) -> Arc
             )
         });
 
-        env.find_type_info(alias_ident).unwrap_or_else(|| {
-            ice!("Unexpected type {} is not a function", alias_type)
-        })
+        env.find_type_info(alias_ident)
+            .unwrap_or_else(|| ice!("Unexpected type {} is not a function", alias_type))
     };
 
     let typ = types::walk_move_type(alias.typ().into_owned(), &mut |typ| {

diff --git a/benches/check.rs b/benches/check.rs
@@ -25,7 +25,7 @@ fn typecheck_prelude(b: &mut Bencher) {
             .read_to_string(&mut text)
             .unwrap();
         text.expand_macro(&mut compiler, &vm, "std.prelude")
-            .unwrap_or_else(|err| panic!("{}", err))
+            .unwrap_or_else(|(_, err)| panic!("{}", err))
     };
     b.iter(|| {
         let result = MacroValue { expr: expr.clone() }.typecheck(&mut compiler, &vm, "<top>", "");

diff --git a/check/src/typecheck.rs b/check/src/typecheck.rs
@@ -57,8 +57,6 @@ pub enum TypeError<I> {
     UndefinedRecord { fields: Vec<I> },
     /// Found a case expression without any alternatives
     EmptyCase,
-    /// An `Error` ast node was found indicating an invalid parse
-    ErrorAst(&'static str),
     Message(String),
 }
 
@@ -151,7 +149,6 @@ impl<I: fmt::Display + AsRef<str>> fmt::Display for TypeError<I> {
                 Ok(())
             }
             EmptyCase => write!(f, "`case` expression with no alternatives"),
-            ErrorAst(typ) => write!(f, "`Error` {} found during typechecking", typ),
             Message(ref msg) => write!(f, "{}", msg),
         }
     }
@@ -489,7 +486,6 @@ impl<'a> Typecheck<'a> {
                 DuplicateField(_) |
                 UndefinedRecord { .. } |
                 EmptyCase |
-                ErrorAst(_) |
                 Rename(_) |
                 Message(_) => (),
                 KindError(_, ref mut typ) |
@@ -949,7 +945,9 @@ impl<'a> Typecheck<'a> {
                 }
                 Ok(TailCall::Type(self.typecheck_opt(last, expected_type)))
             }
-            Expr::Error => Err(TypeError::ErrorAst("expression")),
+            Expr::Error(ref typ) => Ok(TailCall::Type(
+                typ.clone().unwrap_or_else(|| self.subs.new_var()),
+            )),
         }
     }
 
@@ -1162,7 +1160,7 @@ impl<'a> Typecheck<'a> {
                 id.typ = match_type.clone();
                 match_type
             }
-            Pattern::Error => self.error(span, TypeError::ErrorAst("pattern")),
+            Pattern::Error => self.subs.new_var(),
         }
     }
 
@@ -1223,9 +1221,11 @@ impl<'a> Typecheck<'a> {
                     Err(err) => self.error(ast_type.span(), err),
                 }
             }
-            _ => types::translate_type_with(type_cache, ast_type, |typ| {
-                self.translate_ast_type(type_cache, typ)
-            }),
+            _ => types::translate_type_with(
+                type_cache,
+                ast_type,
+                |typ| self.translate_ast_type(type_cache, typ),
+            ),
         }
     }
 
@@ -1921,10 +1921,12 @@ impl<'a> Typecheck<'a> {
                 self.environment
                     .find_type_info(new_id)
                     .map(|alias| alias.clone().into_type())
-                    .or_else(|| if id == new_id {
-                        None
-                    } else {
-                        Some(Type::ident(new_id.clone()))
+                    .or_else(|| {
+                        if id == new_id {
+                            None
+                        } else {
+                            Some(Type::ident(new_id.clone()))
+                        }
                     })
             }
             Type::Variant(ref row) => {

diff --git a/completion/src/lib.rs b/completion/src/lib.rs
@@ -473,7 +473,7 @@ where
             } else {
                 self.visit_one(exprs)
             },
-            Expr::Error => (),
+            Expr::Error(..) => (),
         }
     }
 }

diff --git a/format/src/pretty_print.rs b/format/src/pretty_print.rs
@@ -288,7 +288,7 @@ impl<'a: 'e, 'e> Printer<'a, 'e> {
                     self.pretty_expr_(binds.last().unwrap().alias.span.end, body)
                 ].group()
                 }
-                Expr::Error => arena.text("<error>"),
+                Expr::Error(_) => arena.text("<error>"),
             };
         comments.append(doc)
     }

diff --git a/parser/src/grammar.lalrpop b/parser/src/grammar.lalrpop
@@ -371,15 +371,15 @@ Alternative: Alternative<Id> = {
         let span = pos::Span::new(pat.span.end, end);
         Alternative {
             pattern: pat,
-            expr: pos::spanned(span, Expr::Error),
+            expr: pos::spanned(span, Expr::Error(None)),
         }
     },
     "|" <start: @R> <end: @L> <err: !> => {
         errors.push(err.error);
         let span = pos::Span::new(start, end);
         Alternative {
             pattern: pos::spanned(span, Pattern::Error),
-            expr: pos::spanned(span, Expr::Error),
+            expr: pos::spanned(span, Expr::Error(None)),
         }
     },
 };
@@ -551,7 +551,7 @@ Expr: Expr<Id> = {
 
     ! => {
         errors.push(<>.error);
-        Expr::Error
+        Expr::Error(None)
     }
 };
 

diff --git a/parser/src/lib.rs b/parser/src/lib.rs
@@ -74,7 +74,7 @@ fn shrink_hidden_spans<Id>(mut expr: SpannedExpr<Id>) -> SpannedExpr<Id> {
         Expr::Array(_) |
         Expr::Record { .. } |
         Expr::Tuple { .. } |
-        Expr::Error => (),
+        Expr::Error(..) => (),
     }
     expr
 }

diff --git a/parser/tests/support/mod.rs b/parser/tests/support/mod.rs
@@ -222,7 +222,7 @@ pub fn array(fields: Vec<SpExpr>) -> SpExpr {
 }
 
 pub fn error() -> SpExpr {
-    no_loc(Expr::Error)
+    no_loc(Expr::Error(None))
 }
 
 pub fn alias<Id>(

diff --git a/repl/src/repl.rs b/repl/src/repl.rs
@@ -109,7 +109,8 @@ fn complete(thread: &Thread, name: &str, fileinput: &str, pos: usize) -> GluonRe
             Err((Some(expr), err)) => (expr, Err(err.into())),
         };
 
-    expr.expand_macro(&mut compiler, thread, &name)?;
+    expr.expand_macro(&mut compiler, thread, &name)
+        .map_err(|(_, err)| err)?;
 
     // Only need the typechecker to fill infer the types as best it can regardless of errors
     let _ = compiler.typecheck_expr(thread, &name, fileinput, &mut expr);

diff --git a/src/compiler_pipeline.rs b/src/compiler_pipeline.rs
@@ -8,14 +8,13 @@
 //! difficult to forget a stage.
 
 use std::borrow::{Borrow, BorrowMut};
-#[cfg(feature = "serde")]
 use std::result::Result as StdResult;
 
 #[cfg(feature = "serde")]
 use either::Either;
 
 use base::ast::SpannedExpr;
-use base::error::InFile;
+use base::error::{Errors, InFile};
 use base::metadata::Metadata;
 use base::types::{ArcType, Type};
 use base::source::Source;
@@ -29,6 +28,8 @@ use vm::thread::{RootedValue, Thread, ThreadInternal};
 
 use {Compiler, Error, Result};
 
+pub type SalvageResult<T> = StdResult<T, (Option<T>, Error)>;
+
 /// Result type of successful macro expansion
 pub struct MacroValue<E> {
     pub expr: E,
@@ -42,13 +43,15 @@ pub trait MacroExpandable {
         compiler: &mut Compiler,
         thread: &Thread,
         file: &str,
-    ) -> Result<MacroValue<Self::Expr>>
+    ) -> SalvageResult<MacroValue<Self::Expr>>
     where
         Self: Sized,
     {
         let mut macros = MacroExpander::new(thread);
         let expr = self.expand_macro_with(compiler, &mut macros, file)?;
-        macros.finish()?;
+        if let Err(err) = macros.finish() {
+            return Err((Some(expr), err.into()));
+        }
         Ok(expr)
     }
 
@@ -57,7 +60,7 @@ pub trait MacroExpandable {
         compiler: &mut Compiler,
         macros: &mut MacroExpander,
         file: &str,
-    ) -> Result<MacroValue<Self::Expr>>;
+    ) -> SalvageResult<MacroValue<Self::Expr>>;
 }
 
 impl<'s> MacroExpandable for &'s str {
@@ -68,13 +71,18 @@ impl<'s> MacroExpandable for &'s str {
         compiler: &mut Compiler,
         macros: &mut MacroExpander,
         file: &str,
-    ) -> Result<MacroValue<Self::Expr>> {
+    ) -> SalvageResult<MacroValue<Self::Expr>> {
         compiler
             .parse_expr(macros.vm.global_env().type_cache(), file, self)
-            .map_err(From::from)
+            .map_err(|err| (None, err.into()))
             .and_then(|mut expr| {
-                expr.expand_macro_with(compiler, macros, file)?;
-                Ok(MacroValue { expr: expr })
+                let result = expr.expand_macro_with(compiler, macros, file)
+                    .map(|_| ())
+                    .map_err(|(value, err)| (value.map(|_| ()), err));
+                if let Err((value, err)) = result {
+                    return Err((value.map(|_| MacroValue { expr }), err));
+                }
+                Ok(MacroValue { expr })
             })
     }
 }
@@ -87,7 +95,7 @@ impl<'s> MacroExpandable for &'s mut SpannedExpr<Symbol> {
         compiler: &mut Compiler,
         macros: &mut MacroExpander,
         file: &str,
-    ) -> Result<MacroValue<Self::Expr>> {
+    ) -> SalvageResult<MacroValue<Self::Expr>> {
         if compiler.implicit_prelude {
             compiler.include_implicit_prelude(macros.vm.global_env().type_cache(), file, self);
         }
@@ -138,9 +146,24 @@ where
         expr_str: &str,
         expected_type: Option<&ArcType>,
     ) -> Result<TypecheckValue<Self::Expr>> {
-        self.expand_macro(compiler, thread, file).and_then(|expr| {
-            expr.typecheck_expected(compiler, thread, file, expr_str, expected_type)
-        })
+        let mut macro_error = None;
+        let expr = match self.expand_macro(compiler, thread, file) {
+            Ok(expr) => expr,
+            Err((Some(expr), err)) => {
+                macro_error = Some(err);
+                expr
+            }
+            Err((None, err)) => return Err(err),
+        };
+        match expr.typecheck_expected(compiler, thread, file, expr_str, expected_type) {
+            Ok(value) => match macro_error {
+                Some(err) => return Err(err),
+                None => Ok(value),
+            },
+            Err(err) => Err(
+                Errors::from(macro_error.into_iter().chain(Some(err)).collect::<Vec<_>>()).into(),
+            ),
+        }
     }
 }