Repository: tweag/nickel (rust)

Issue:
The REPL doesn't show properly parse error anymore
**Describe the bug**
If we enter something that fails to parse inside the REPL, we don't get any meaningful error reporting, but rather a `<parse error>`. If we try to add contracts or type annotations, it's even worse as we just get random contract violations

**To Reproduce**
```
nickel>
let protocol : <http, ftp, sftp> = `http in
  switch {
    http -> 1,
    ftp -> 2,
    sftp -> 3
  } protocol

<parse error>

nickel>(let protocol : <http, ftp, sftp> = `http in
  switch {
    http -> 1,
    ftp -> 2,
    sftp -> 3
  } protocol) : Num

error: Blame error: contract broken by a value.
  ┌─ :1:1
  │
1 │ Num
  │ --- expected type
  │
  ┌─ repl-input-22:2:3
  │
2 │ ╭   switch {
3 │ │     http -> 1,
4 │ │     ftp -> 2,
5 │ │     sftp -> 3
6 │ │   } protocol) : Num
  │ ╰────────────' evaluated to this expression

note:
  ┌─ repl-input-22:6:17
  │
6 │   } protocol) : Num
  │                 ^^^ bound here

```

**Expected behavior**
I expect parse errors to be reported nicely via an error diagnostic, as they used to be.

**Additional context**
It is most probably caused by PR #456. For whatever reason the REPL now considers parse errors as acceptable, while it shouldn't.

cc @Avi-D-coder 


Pull request:
Fix #487
Also adds wrapper functions around parse variants so it's easier to use.

Patch:
diff --git a/src/cache.rs b/src/cache.rs
index 8d023f40..81ceb679 100644
--- a/src/cache.rs
+++ b/src/cache.rs
@@ -362,16 +362,8 @@ impl Cache {
 
         match format {
             InputFormat::Nickel => {
-                let mut parse_err_rec = Vec::new();
-                let t = parser::grammar::TermParser::new()
-                    .parse(file_id, &mut parse_err_rec, Lexer::new(buf))
-                    .map_err(|err| (ParseError::from_lalrpop(err, file_id)))?;
-
-                let parse_errs: ParseErrors = parse_err_rec
-                    .into_iter()
-                    .map(|e| ParseError::from_lalrpop(e.error, file_id))
-                    .collect::<Vec<_>>()
-                    .into();
+                let (t, parse_errs) = parser::grammar::TermParser::new()
+                    .parse_term_tolerant(file_id, Lexer::new(&buf))?;
 
                 Ok((t, parse_errs))
             }
@@ -1007,8 +999,7 @@ pub mod resolvers {
             if let hash_map::Entry::Vacant(e) = self.term_cache.entry(file_id) {
                 let buf = self.files.source(file_id);
                 let term = parser::grammar::TermParser::new()
-                    .parse(file_id, &mut Vec::new(), Lexer::new(buf))
-                    .map_err(|e| ParseError::from_lalrpop(e, file_id))
+                    .parse_term(file_id, Lexer::new(&buf))
                     .map_err(|e| ImportError::ParseErrors(e.into(), *pos))?;
                 e.insert(term);
                 Ok((
diff --git a/src/error.rs b/src/error.rs
index 2c138013..1febab08 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -6,17 +6,19 @@ use std::fmt::Write;
 
 use codespan::{FileId, Files};
 use codespan_reporting::diagnostic::{Diagnostic, Label, LabelStyle};
+use lalrpop_util::ErrorRecovery;
 
 use crate::eval::{CallStack, StackElem};
 use crate::identifier::Ident;
 use crate::label::ty_path;
 use crate::parser::error::{LexicalError, ParseError as InternalParseError};
+use crate::parser::lexer::Token;
 use crate::parser::utils::mk_span;
 use crate::position::{RawSpan, TermPos};
 use crate::serialize::ExportFormat;
 use crate::term::RichTerm;
 use crate::types::Types;
-use crate::{label, repl};
+use crate::{label, parser, repl};
 
 /// A general error occurring during either parsing or evaluation.
 #[derive(Debug, Clone, PartialEq)]
@@ -191,7 +193,7 @@ pub enum TypecheckError {
 
 #[derive(Debug, PartialEq, Clone, Default)]
 pub struct ParseErrors {
-    errors: Vec<ParseError>,
+    pub errors: Vec<ParseError>,
 }
 
 impl ParseErrors {
@@ -213,6 +215,18 @@ impl ParseErrors {
     pub const fn none() -> ParseErrors {
         ParseErrors { errors: Vec::new() }
     }
+
+    pub fn from_recoverable<'a>(
+        errs: Vec<ErrorRecovery<usize, Token<'a>, parser::error::ParseError>>,
+        file_id: FileId,
+    ) -> Self {
+        ParseErrors {
+            errors: errs
+                .into_iter()
+                .map(|e| ParseError::from_lalrpop(e.error, file_id))
+                .collect(),
+        }
+    }
 }
 
 impl From<ParseError> for ParseErrors {
diff --git a/src/eval.rs b/src/eval.rs
index 9798679d..13565a37 100644
--- a/src/eval.rs
+++ b/src/eval.rs
@@ -1124,7 +1124,7 @@ mod tests {
         let id = Files::new().add("<test>", String::from(s));
 
         grammar::TermParser::new()
-            .parse(id, &mut Vec::new(), lexer::Lexer::new(&s))
+            .parse_term(id, lexer::Lexer::new(&s))
             .map(|mut t| {
                 t.clean_pos();
                 t
@@ -1445,11 +1445,12 @@ mod tests {
                 .unwrap()
         );
 
-        let t = parse("switch {x => [1, glob1], y => loc2, z => {id = true, other = glob3}} loc1")
-            .unwrap();
+        let t =
+            parse("switch {`x => [1, glob1], `y => loc2, `z => {id = true, other = glob3}} loc1")
+                .unwrap();
         assert_eq!(
             subst(t, &global_env, &env),
-            parse("switch {x => [1, 1], y => (if false then 1 else \"Glob2\"), z => {id = true, other = false}} true").unwrap()
+            parse("switch {`x => [1, 1], `y => (if false then 1 else \"Glob2\"), `z => {id = true, other = false}} true").unwrap()
         );
     }
 }
diff --git a/src/parser/mod.rs b/src/parser/mod.rs
index f6048c36..7ffbf90f 100644
--- a/src/parser/mod.rs
+++ b/src/parser/mod.rs
@@ -1,5 +1,7 @@
+use crate::error::{ParseError, ParseErrors};
 use crate::identifier::Ident;
 use crate::term::RichTerm;
+use codespan::FileId;
 use lalrpop_util::lalrpop_mod;
 
 lalrpop_mod!(
@@ -18,3 +20,65 @@ pub enum ExtendedTerm {
     RichTerm(RichTerm),
     ToplevelLet(Ident, RichTerm),
 }
+
+impl grammar::ExtendedTermParser {
+    pub fn parse_term_tolerant(
+        &self,
+        file_id: FileId,
+        lexer: lexer::Lexer,
+    ) -> Result<(ExtendedTerm, ParseErrors), ParseError> {
+        let mut parse_errors = Vec::new();
+        let result = self
+            .parse(file_id, &mut parse_errors, lexer)
+            .map_err(|err| ParseError::from_lalrpop(err, file_id));
+
+        let parse_errors = ParseErrors::from_recoverable(parse_errors, file_id);
+        match result {
+            Ok(t) => Ok((t, parse_errors)),
+            Err(e) => Err(e),
+        }
+    }
+
+    pub fn parse_term(
+        &self,
+        file_id: FileId,
+        lexer: lexer::Lexer,
+    ) -> Result<ExtendedTerm, ParseErrors> {
+        match self.parse_term_tolerant(file_id, lexer) {
+            Ok((t, e)) if e.no_errors() => Ok(t),
+            Ok((_, e)) => Err(e),
+            Err(e) => Err(e.into()),
+        }
+    }
+}
+
+impl grammar::TermParser {
+    pub fn parse_term_tolerant(
+        &self,
+        file_id: FileId,
+        lexer: lexer::Lexer,
+    ) -> Result<(RichTerm, ParseErrors), ParseError> {
+        let mut parse_errors = Vec::new();
+        let result = self
+            .parse(file_id, &mut parse_errors, lexer)
+            .map_err(|err| ParseError::from_lalrpop(err, file_id));
+
+        let parse_errors = ParseErrors::from_recoverable(parse_errors, file_id);
+        match result {
+            Ok(t) => Ok((t, parse_errors)),
+            Err(e) => Err(e),
+        }
+    }
+
+    pub fn parse_term(
+        &self,
+        file_id: FileId,
+        lexer: lexer::Lexer,
+    ) -> Result<RichTerm, ParseErrors> {
+        match self.parse_term_tolerant(file_id, lexer) {
+            Ok((t, e)) if e.no_errors() => Ok(t),
+            Ok((_, e)) => Err(e),
+            Err(e) => Err(e.into()),
+        }
+    }
+}
diff --git a/src/program.rs b/src/program.rs
index 1b3b2dae..e94e92eb 100644
--- a/src/program.rs
+++ b/src/program.rs
@@ -22,7 +22,7 @@
 //! [`mk_global_env`](./struct.Program.html#method.mk_global_env)).  Each such value is added to
 //! the global environment before the evaluation of the program.
 use crate::cache::*;
-use crate::error::{Error, ParseError, ToDiagnostic};
+use crate::error::{Error, ToDiagnostic};
 use crate::identifier::Ident;
 use crate::parser::lexer::Lexer;
 use crate::term::{RichTerm, Term};
@@ -159,9 +159,8 @@ pub fn query(
         // errors.
         let source = format!("x.{}", p);
         let query_file_id = cache.add_tmp("<query>", source.clone());
-        let new_term = parser::grammar::TermParser::new()
-            .parse(query_file_id, &mut Vec::new(), Lexer::new(&source))
-            .map_err(|err| ParseError::from_lalrpop(err, query_file_id))?;
+        let new_term =
+            parser::grammar::TermParser::new().parse_term(query_file_id, Lexer::new(&source))?;
 
         // Substituting `y` for `t`
         let mut env = eval::Environment::new();
@@ -219,7 +218,7 @@ mod tests {
         let id = Files::new().add("<test>", String::from(s));
 
         grammar::TermParser::new()
-            .parse(id, &mut Vec::new(), lexer::Lexer::new(&s))
+            .parse_term(id, lexer::Lexer::new(&s))
             .map(|mut t| {
                 t.clean_pos();
                 t
diff --git a/src/repl/mod.rs b/src/repl/mod.rs
index 08d234e3..2ebb77b9 100644
--- a/src/repl/mod.rs
+++ b/src/repl/mod.rs
@@ -7,13 +7,14 @@
 //! jupyter-kernel (which is not exactly user-facing, but still manages input/output and
 //! formatting), etc.
 use crate::cache::Cache;
-use crate::error::{Error, EvalError, IOError, ParseError, REPLError};
+use crate::error::{Error, EvalError, IOError, ParseError, ParseErrors, REPLError};
 use crate::identifier::Ident;
 use crate::parser::{grammar, lexer, ExtendedTerm};
 use crate::term::{RichTerm, Term};
 use crate::types::Types;
 use crate::{eval, transformations, typecheck};
 use codespan::FileId;
+use md5::digest::generic_array::functional::FunctionalSequence;
 use simple_counter::*;
 use std::ffi::{OsStr, OsString};
 use std::io::Write;
@@ -117,11 +118,15 @@ impl REPLImpl {
             String::from(exp),
         );
 
-        match self
+        let (term, parse_errs) = self
             .parser
-            .parse(file_id, &mut Vec::new(), lexer::Lexer::new(exp))
-            .map_err(|err| ParseError::from_lalrpop(err, file_id))?
-        {
+            .parse_term_tolerant(file_id, lexer::Lexer::new(exp))?;
+
+        if !parse_errs.no_errors() {
+            return Err(parse_errs.into());
+        }
+
+        match term {
             // Because we don't use the cache for input, we have to perform recursive import
             // resolution/typechecking/transformation by oursleves.
             ExtendedTerm::RichTerm(t) => {
@@ -260,7 +265,7 @@ pub enum InputStatus {
     Complete(ExtendedTerm),
     Partial,
     Command,
-    Failed(ParseError),
+    Failed(ParseErrors),
 }
 
 /// Validator enabling multiline input.
@@ -303,15 +308,21 @@ impl InputParser {
 
         let result = self
             .parser
-            .parse(self.file_id, &mut Vec::new(), lexer::Lexer::new(input))
-            .map_err(|err| ParseError::from_lalrpop(err, self.file_id));
+            .parse_term_tolerant(self.file_id, lexer::Lexer::new(input));
+
+        let partial = |pe| {
+            matches!(
+                pe,
+                &ParseError::UnexpectedEOF(..) | &ParseError::UnmatchedCloseBrace(..)
+            )
+        };
 
         match result {
-            Ok(t) => InputStatus::Complete(t),
-            Err(ParseError::UnexpectedEOF(..)) | Err(ParseError::UnmatchedCloseBrace(..)) => {
-                InputStatus::Partial
-            }
-            Err(err) => InputStatus::Failed(err),
+            Ok((t, e)) if e.no_errors() => InputStatus::Complete(t),
+            Ok((_, e)) if e.errors.iter().all(|e| partial(e)) => InputStatus::Partial,
+            Ok((_, e)) => InputStatus::Failed(e),
+            Err(e) if partial(&e) => InputStatus::Partial,
+            Err(err) => InputStatus::Failed(err.into()),
         }
     }
 }
diff --git a/src/types.rs b/src/types.rs
index 9a3860ee..2c736526 100644
--- a/src/types.rs
+++ b/src/types.rs
@@ -430,7 +430,7 @@ mod test {
         let id = Files::new().add("<test>", wrapper.clone());
 
         let rt = TermParser::new()
-            .parse(id, &mut Vec::new(), Lexer::new(&wrapper))
+            .parse_term(id, Lexer::new(&wrapper))
             .unwrap();
 
         match *rt.term {


Test patch:
diff --git a/src/parser/tests.rs b/src/parser/tests.rs
index cd724296..e2a5a6c6 100644
--- a/src/parser/tests.rs
+++ b/src/parser/tests.rs
@@ -13,8 +13,8 @@ fn parse(s: &str) -> Result<RichTerm, ParseError> {
     let id = Files::new().add("<test>", String::from(s));
 
     super::grammar::TermParser::new()
-        .parse(id, &mut Vec::new(), Lexer::new(&s))
-        .map_err(|err| ParseError::from_lalrpop(err, id))
+        .parse_term(id, Lexer::new(&s))
+        .map_err(|errs| errs.errors.first().unwrap().clone())
 }
 
 fn parse_without_pos(s: &str) -> RichTerm {
diff --git a/tests/typecheck_fail.rs b/tests/typecheck_fail.rs
index 4d744b94..0b3fad33 100644
--- a/tests/typecheck_fail.rs
+++ b/tests/typecheck_fail.rs
@@ -16,7 +16,7 @@ fn type_check_expr(s: impl std::string::ToString) -> Result<Types, TypecheckErro
     let id = Files::new().add("<test>", s.clone());
     type_check(
         &grammar::TermParser::new()
-            .parse(id, &mut Vec::new(), lexer::Lexer::new(&s))
+            .parse_term(id, lexer::Lexer::new(&s))
             .unwrap(),
     )
 }