Merge pull request tweag#808 from tweag/fix/issue-799

Handle and report parse errors that cause trouble during evaluation.

src/error.rs

@@ -52,6 +52,8 @@ pub enum EvalError {
         /* position of the original unevaluated expression */ TermPos,
         /* evaluated expression */ RichTerm,
     ),
+    /// Tried to evaluate a term which wasn't parsed correctly.
+    ParseError(ParseError),
     /// A term which is not a function has been applied to an argument.
     NotAFunc(
         /* term */ RichTerm,
@@ -1036,6 +1038,7 @@ impl ToDiagnostic<FileId> for EvalError {
                     .with_labels(labels)
                     .with_notes(vec![msg.clone()])]
             }
+            EvalError::ParseError(parse_error) => parse_error.to_diagnostic(files, contract_id),
             EvalError::NotAFunc(t, arg, pos_opt) => vec![Diagnostic::error()
                 .with_message("not a function")
                 .with_labels(vec![

src/eval/mod.rs

@@ -632,6 +632,9 @@ where
                     env: local_env,
                 }
             }
+            Term::ParseError(parse_error) => {
+                return Err(EvalError::ParseError(parse_error.clone()));
+            }
             // Continuation of operations and thunk update
             _ if stack.is_top_thunk() || stack.is_top_cont() => {
                 clos = Closure {
@@ -701,7 +704,7 @@ pub fn subst(rt: RichTerm, initial_env: &Environment, env: &Environment) -> Rich
             })
             .unwrap_or_else(|| RichTerm::new(Term::Var(id), pos)),
         v @ Term::Null
-        | v @ Term::ParseError
+        | v @ Term::ParseError(_)
         | v @ Term::Bool(_)
         | v @ Term::Num(_)
         | v @ Term::Str(_)

src/parser/grammar.lalrpop

@@ -219,9 +219,11 @@ UniTerm: UniTerm = {
         UniTerm::from(mk_app!(Term::Op1(UnaryOp::Ite(), cond), t1, t2)),
     <l: @L> <t: !> <r: @R> => {
         let pos = mk_pos(src_id, l, r);
-        errors.push(t);
+        errors.push(t.clone());
 
-        UniTerm::from(RichTerm::new(Term::ParseError, pos))
+        UniTerm::from(RichTerm::new(Term::ParseError(
+            crate::error::ParseError::from_lalrpop(t.error, src_id)
+        ), pos))
     },
 };
 

src/pretty.rs

@@ -684,7 +684,7 @@ where
                 .append(allocator.space())
                 .append(allocator.as_string(f.to_string_lossy()).double_quotes()),
             ResolvedImport(id) => allocator.text(format!("import <file_id: {:?}>", id)),
-            ParseError => allocator
+            ParseError(_) => allocator
                 .text("#<PARSE ERROR!>")
                 .append(allocator.hardline()),
         }

src/term.rs

@@ -17,6 +17,7 @@
 //! the term level, and together with [crate::eval::merge], they allow for flexible and modular
 //! definitions of contracts, record and metadata all together.
 use crate::destruct::Destruct;
+use crate::error::ParseError;
 use crate::identifier::Ident;
 use crate::label::Label;
 use crate::match_sharedterm;
@@ -157,7 +158,7 @@ pub enum Term {
     #[serde(skip)]
     ResolvedImport(FileId),
     #[serde(skip)]
-    ParseError,
+    ParseError(ParseError),
 }
 
 pub type SealingKey = i32;
@@ -361,7 +362,7 @@ impl Term {
     {
         use self::Term::*;
         match self {
-            Null | ParseError => (),
+            Null | ParseError(_) => (),
             Switch(ref mut t, ref mut cases, ref mut def) => {
                 cases.iter_mut().for_each(|c| {
                     let (_, t) = c;
@@ -448,7 +449,7 @@ impl Term {
             | Term::Import(_)
             | Term::ResolvedImport(_)
             | Term::StrChunks(_)
-            | Term::ParseError => None,
+            | Term::ParseError(_) => None,
         }
         .map(String::from)
     }
@@ -512,7 +513,7 @@ impl Term {
                 format!("<{}{}={}>", content, value_label, value)
             }
             Term::Var(id) => id.to_string(),
-            Term::ParseError => String::from("<parse error>"),
+            Term::ParseError(_) => String::from("<parse error>"),
             Term::Let(..)
             | Term::LetPattern(..)
             | Term::App(_, _)
@@ -580,7 +581,7 @@ impl Term {
             | Term::ResolvedImport(_)
             | Term::StrChunks(_)
             | Term::RecRecord(..)
-            | Term::ParseError => false,
+            | Term::ParseError(_) => false,
         }
     }
 
@@ -620,7 +621,7 @@ impl Term {
             | Term::ResolvedImport(_)
             | Term::StrChunks(_)
             | Term::RecRecord(..)
-            | Term::ParseError => false,
+            | Term::ParseError(_) => false,
         }
     }
 
@@ -652,7 +653,7 @@ impl Term {
             | Term::MetaValue(..)
             | Term::Import(..)
             | Term::ResolvedImport(..)
-            | Term::ParseError => false,
+            | Term::ParseError(_) => false,
         }
     }
 }

src/transform/free_vars.rs

@@ -24,7 +24,7 @@ fn collect_free_vars(rt: &mut RichTerm, free_vars: &mut HashSet<Ident>) {
         Term::Var(id) => {
             free_vars.insert(id.clone());
         }
-        Term::ParseError
+        Term::ParseError(_)
         | Term::Null
         | Term::Bool(_)
         | Term::Num(_)

src/typecheck/mod.rs

@@ -225,7 +225,7 @@ fn walk<L: Linearizer>(
     );
 
     match t.as_ref() {
-        Term::ParseError
+        Term::ParseError(_)
         | Term::Null
         | Term::Bool(_)
         | Term::Num(_)
@@ -463,7 +463,7 @@ fn type_check_<L: Linearizer>(
     linearizer.add_term(lin, t, *pos, ty.clone());
 
     match t.as_ref() {
-        Term::ParseError => Ok(()),
+        Term::ParseError(_) => Ok(()),
         // null is inferred to be of type Dyn
         Term::Null => unify(state, ty, mk_typewrapper::dynamic())
             .map_err(|err| err.into_typecheck_err(state, rt.pos)),

tests/imports.rs

@@ -1,5 +1,5 @@
 use assert_matches::assert_matches;
-use nickel_lang::error::{Error, EvalError, TypecheckError};
+use nickel_lang::error::{Error, EvalError, ParseError, TypecheckError};
 use nickel_lang::program::Program;
 use nickel_lang::term::Term;
 use std::io::BufReader;
@@ -112,3 +112,39 @@ fn circular_imports_fail() {
         Ok(Term::RecRecord(..)) | Ok(Term::Record(..))
     );
 }
+
+#[test]
+fn import_unexpected_token_fail() {
+    let mut prog = Program::new_from_source(
+        BufReader::new(mk_import("unexpected_token.ncl").as_bytes()),
+        "should_fail",
+    )
+    .unwrap();
+    assert_matches!(
+        prog.eval(),
+        Err(Error::EvalError(EvalError::ParseError(
+            ParseError::UnexpectedToken(..)
+        )))
+    );
+}
+
+#[test]
+fn import_unexpected_token_in_record_fail() {
+    let mut prog = Program::new_from_source(
+        BufReader::new(
+            format!(
+                "let x = {} in \"Hello, \" ++ x.name",
+                mk_import("unexpected_token_in_record.ncl")
+            )
+            .as_bytes(),
+        ),
+        "should_fail",
+    )
+    .unwrap();
+    assert_matches!(
+        prog.eval(),
+        Err(Error::EvalError(EvalError::ParseError(
+            ParseError::UnexpectedToken(..)
+        )))
+    );
+}

tests/imports/unexpected_token.ncl

@@ -0,0 +1 @@
+"Nickel"$

tests/imports/unexpected_token_in_record.ncl

@@ -0,0 +1,3 @@
+{
+  name = "Nickel",,
+}

tests/parse_fail.rs

@@ -0,0 +1,14 @@
+use assert_matches::assert_matches;
+use nickel_lang::error::Error;
+
+use nickel_lang_utilities::eval;
+
+#[test]
+fn unexpected_token() {
+    assert_matches!(eval("\"Nickel\"$"), Err(Error::ParseErrors(..)));
+}
+
+#[test]
+fn unexpected_token_in_record() {
+    assert_matches!(eval("{ name = \"Nickel\",, }"), Err(Error::ParseErrors(..)));
+}