Repository: tweag/nickel
Pull request #814 (2022-08-31 10:38:25)

Toggle error tolerance on/off in Cache.
Related to #799 and #808.

Not entirely happy with this. See comment below. What do you think, @yannham?


Patch:
diff --git a/benches/stdlib.rs b/benches/stdlib.rs
index 21ed10a0..62f0739a 100644
--- a/benches/stdlib.rs
+++ b/benches/stdlib.rs
@@ -1,10 +1,10 @@
 use criterion::{criterion_group, criterion_main, Criterion};
 use pprof::criterion::{Output, PProfProfiler};
 
-use nickel_lang::cache::Cache;
+use nickel_lang::cache::{Cache, ErrorTolerance};
 
 pub fn typecheck_stdlib(c: &mut Criterion) {
-    let mut cache = Cache::new();
+    let mut cache = Cache::new(ErrorTolerance::Strict);
     cache.load_stdlib().unwrap();
     let type_env = cache.mk_type_env().unwrap();
     c.bench_function("typecheck stdlib", |b| {
diff --git a/lsp/nls/src/files.rs b/lsp/nls/src/files.rs
index e9ed6693..ce692bc7 100644
--- a/lsp/nls/src/files.rs
+++ b/lsp/nls/src/files.rs
@@ -82,7 +82,7 @@ fn typecheck(server: &mut Server, file_id: FileId) -> Result<CacheOp<()>, Vec<Di
 fn parse_and_typecheck(server: &mut Server, uri: Url, file_id: FileId) -> Result<()> {
     let diagnostics = server
         .cache
-        .parse_lax(file_id)
+        .parse(file_id)
         .map_err(|parse_err| parse_err.to_diagnostic(server.cache.files_mut(), None))
         .map(|parse_errs| {
             // Parse errors are not fatal
diff --git a/lsp/nls/src/server.rs b/lsp/nls/src/server.rs
index f3de4d0b..358e91ec 100644
--- a/lsp/nls/src/server.rs
+++ b/lsp/nls/src/server.rs
@@ -16,7 +16,7 @@ use lsp_types::{
     TextDocumentSyncOptions, WorkDoneProgressOptions,
 };
 
-use nickel_lang::cache::Cache;
+use nickel_lang::cache::{Cache, ErrorTolerance};
 use nickel_lang::typecheck::Environment;
 
 use crate::{
@@ -59,7 +59,7 @@ impl Server {
     }
 
     pub fn new(connection: Connection) -> Server {
-        let mut cache = Cache::new();
+        let mut cache = Cache::new(ErrorTolerance::Tolerant);
         cache.load_stdlib().unwrap();
         let initial_env = cache.mk_type_env().unwrap();
         let lin_cache = HashMap::new();
diff --git a/src/cache.rs b/src/cache.rs
index 20d6fbe2..712de9eb 100644
--- a/src/cache.rs
+++ b/src/cache.rs
@@ -70,12 +70,22 @@ pub struct Cache {
     stdlib_ids: Option<Vec<FileId>>,
     /// The inferred type of wildcards for each `FileId`.
     wildcards: HashMap<FileId, Wildcards>,
+    /// Whether processing should try to continue even in case of errors. Needed by the NLS.
+    error_tolerance: ErrorTolerance,
 
     #[cfg(debug_assertions)]
     /// Skip loading the stdlib, used for debugging purpose
     pub skip_stdlib: bool,
 }
 
+/// The error tolerance mode used by the parser. The NLS needs to try to
+/// continue even in case of errors.
+#[derive(Debug, Clone)]
+pub enum ErrorTolerance {
+    Tolerant,
+    Strict,
+}
+
 /// wrapping eval environment with typing environment
 #[derive(Debug, Clone)]
 pub struct Envs {
@@ -202,7 +212,7 @@ pub enum ResolvedTerm {
 }
 
 impl Cache {
-    pub fn new() -> Self {
+    pub fn new(error_tolerance: ErrorTolerance) -> Self {
         Cache {
             files: Files::new(),
             file_ids: HashMap::new(),
@@ -210,6 +220,7 @@ impl Cache {
             wildcards: HashMap::new(),
             imports: HashMap::new(),
             stdlib_ids: None,
+            error_tolerance,
 
             #[cfg(debug_assertions)]
             skip_stdlib: false,
@@ -343,7 +354,7 @@ impl Cache {
     /// corresponding error messages are collected and returned.
     ///
     /// The `Err` part of the result corresponds to non-recoverable errors.
-    pub fn parse_lax(&mut self, file_id: FileId) -> Result<CacheOp<ParseErrors>, ParseError> {
+    fn parse_lax(&mut self, file_id: FileId) -> Result<CacheOp<ParseErrors>, ParseError> {
         if let Some(CachedTerm { parse_errs, .. }) = self.terms.get(&file_id) {
             Ok(CacheOp::Cached(parse_errs.clone()))
         } else {
@@ -356,24 +367,31 @@ impl Cache {
                     parse_errs: parse_errs.clone(),
                 },
             );
+
             Ok(CacheOp::Done(parse_errs))
         }
     }
 
-    /// Parse a source and populate the corresponding entry in the cache, or do nothing if the
-    /// entry has already been parsed. This function is not error tolerant and returns `Err` if at
-    /// least one parse error has been encountered.
-    pub fn parse(&mut self, file_id: FileId) -> Result<CacheOp<()>, ParseErrors> {
-        match self.parse_lax(file_id)? {
-            CacheOp::Done(e) | CacheOp::Cached(e) if !e.no_errors() => Err(e),
-            CacheOp::Done(_) => Ok(CacheOp::Done(())),
-            CacheOp::Cached(_) => Ok(CacheOp::Cached(())),
+    /// Parse a source and populate the corresponding entry in the cache, or do
+    /// nothing if the entry has already been parsed. This function is error
+    /// tolerant if self.error_tolerant = true.
+    pub fn parse(&mut self, file_id: FileId) -> Result<CacheOp<ParseErrors>, ParseErrors> {
+        let result = self.parse_lax(file_id);
+
+        match self.error_tolerance {
+            ErrorTolerance::Tolerant => result.map_err(|err| err.into()),
+            ErrorTolerance::Strict => match result? {
+                CacheOp::Done(e) | CacheOp::Cached(e) if !e.no_errors() => Err(e),
+                CacheOp::Done(_) => Ok(CacheOp::Done(ParseErrors::none())),
+                CacheOp::Cached(_) => Ok(CacheOp::Cached(ParseErrors::none())),
+            },
         }
     }
 
-    /// Parse a source and populate the corresponding entry in the cache, or do nothing if the
-    /// entry has already been parsed. Support multiple formats.
-    pub fn parse_multi(
+    /// Parse a source and populate the corresponding entry in the cache, or do
+    /// nothing if the entry has already been parsed. Support multiple formats.
+    /// This function is error tolerant.
+    fn parse_multi_lax(
         &mut self,
         file_id: FileId,
         format: InputFormat,
@@ -394,6 +412,26 @@ impl Cache {
         }
     }
 
+    /// Parse a source and populate the corresponding entry in the cache, or do
+    /// nothing if the entry has already been parsed. Support multiple formats.
+    /// This function is error tolerant if self.error_tolerant = true.
+    pub fn parse_multi(
+        &mut self,
+        file_id: FileId,
+        format: InputFormat,
+    ) -> Result<CacheOp<ParseErrors>, ParseErrors> {
+        let result = self.parse_multi_lax(file_id, format);
+
+        match self.error_tolerance {
+            ErrorTolerance::Tolerant => result.map_err(|err| err.into()),
+            ErrorTolerance::Strict => match result? {
+                CacheOp::Done(e) | CacheOp::Cached(e) if !e.no_errors() => Err(e),
+                CacheOp::Done(_) => Ok(CacheOp::Done(ParseErrors::none())),
+                CacheOp::Cached(_) => Ok(CacheOp::Cached(ParseErrors::none())),
+            },
+        }
+    }
+
     /// Parse a source without querying nor populating the cache.
     pub fn parse_nocache(&self, file_id: FileId) -> Result<(RichTerm, ParseErrors), ParseError> {
         self.parse_nocache_multi(file_id, InputFormat::Nickel)
@@ -410,6 +448,7 @@ impl Cache {
         match format {
             InputFormat::Nickel => {
                 let (t, parse_errs) =
+                    // TODO: Should this really be parse_term if self.error_tolerant = false?
                     parser::grammar::TermParser::new().parse_term_lax(file_id, Lexer::new(buf))?;
 
                 Ok((t, parse_errs))
@@ -642,7 +681,7 @@ impl Cache {
     ) -> Result<CacheOp<()>, Error> {
         let mut result = CacheOp::Cached(());
 
-        if self.parse(file_id)? == CacheOp::Done(()) {
+        if let CacheOp::Done(_) = self.parse(file_id)? {
             result = CacheOp::Done(());
         }
 
@@ -986,7 +1025,6 @@ impl ImportResolver for Cache {
             }
         };
 
-        // We ignore non fatal parse errors while importing.
         self.parse_multi(file_id, format)
             .map_err(|err| ImportError::ParseErrors(err.into(), *pos))?;
 
diff --git a/src/program.rs b/src/program.rs
index 21b9c1f2..30a369ec 100644
--- a/src/program.rs
+++ b/src/program.rs
@@ -50,7 +50,7 @@ impl Program {
     }
 
     pub fn new_from_file(path: impl Into<OsString>) -> std::io::Result<Program> {
-        let mut cache = Cache::new();
+        let mut cache = Cache::new(ErrorTolerance::Strict);
         let main_id = cache.add_file(path)?;
 
         Ok(Program { main_id, cache })
@@ -62,7 +62,7 @@ impl Program {
         T: Read,
         S: Into<OsString> + Clone,
     {
-        let mut cache = Cache::new();
+        let mut cache = Cache::new(ErrorTolerance::Strict);
         let main_id = cache.add_source(source_name, source)?;
 
         Ok(Program { main_id, cache })
diff --git a/src/repl/mod.rs b/src/repl/mod.rs
index 6ba6c85d..c7f0af36 100644
--- a/src/repl/mod.rs
+++ b/src/repl/mod.rs
@@ -6,7 +6,7 @@
 //! Dually, the frontend is the user-facing part, which may be a CLI, a web application, a
 //! jupyter-kernel (which is not exactly user-facing, but still manages input/output and
 //! formatting), etc.
-use crate::cache::{Cache, Envs};
+use crate::cache::{Cache, Envs, ErrorTolerance};
 use crate::error::{Error, EvalError, IOError, ParseError, ParseErrors, ReplError};
 use crate::identifier::Ident;
 use crate::parser::{grammar, lexer, ExtendedTerm};
@@ -83,7 +83,7 @@ impl ReplImpl {
     /// Create a new empty REPL.
     pub fn new() -> Self {
         ReplImpl {
-            cache: Cache::new(),
+            cache: Cache::new(ErrorTolerance::Strict),
             parser: grammar::ExtendedTermParser::new(),
             env: Envs::new(),
             initial_type_env: typecheck::Environment::new(),
diff --git a/utilities/src/lib.rs b/utilities/src/lib.rs
index 2aff59bd..4daac592 100644
--- a/utilities/src/lib.rs
+++ b/utilities/src/lib.rs
@@ -2,7 +2,7 @@
 use codespan::Files;
 use criterion::Criterion;
 use nickel_lang::{
-    cache::Cache,
+    cache::{Cache, Envs, ErrorTolerance},
     error::{Error, ParseError},
     eval,
     parser::{grammar, lexer},
@@ -117,9 +117,7 @@ impl<'b> Bench<'b> {
 }
 
 pub fn bench_terms<'r>(rts: Vec<Bench<'r>>) -> Box<dyn Fn(&mut Criterion) + 'r> {
-    use nickel_lang::cache::Envs;
-
-    let mut cache = Cache::new();
+    let mut cache = Cache::new(ErrorTolerance::Strict);
     let Envs { eval_env, type_env } = cache.prepare_stdlib().unwrap();
     Box::new(move |c: &mut Criterion| {
         rts.iter().for_each(|bench| {


Test patch:
diff --git a/tests/imports.rs b/tests/imports.rs
index b208da39..6d5d7618 100644
--- a/tests/imports.rs
+++ b/tests/imports.rs
@@ -1,5 +1,5 @@
 use assert_matches::assert_matches;
-use nickel_lang::error::{Error, EvalError, ParseError, TypecheckError};
+use nickel_lang::error::{Error, EvalError, ImportError, TypecheckError};
 use nickel_lang::program::Program;
 use nickel_lang::term::Term;
 use std::io::BufReader;
@@ -122,9 +122,7 @@ fn import_unexpected_token_fail() {
     .unwrap();
     assert_matches!(
         prog.eval(),
-        Err(Error::EvalError(EvalError::ParseError(
-            ParseError::UnexpectedToken(..)
-        )))
+        Err(Error::ImportError(ImportError::ParseErrors(..)))
     );
 }
 
@@ -143,8 +141,25 @@ fn import_unexpected_token_in_record_fail() {
     .unwrap();
     assert_matches!(
         prog.eval(),
-        Err(Error::EvalError(EvalError::ParseError(
-            ParseError::UnexpectedToken(..)
-        )))
+        Err(Error::ImportError(ImportError::ParseErrors(..)))
+    );
+}
+
+#[test]
+fn import_unexpected_token_buried_fail() {
+    let mut prog = Program::new_from_source(
+        BufReader::new(
+            format!(
+                "let _ign = {} in 1",
+                mk_import("unexpected_token_buried.ncl")
+            )
+            .as_bytes(),
+        ),
+        "should_fail",
+    )
+    .unwrap();
+    assert_matches!(
+        prog.eval(),
+        Err(Error::ImportError(ImportError::ParseErrors(..)))
     );
 }
diff --git a/tests/imports/unexpected_token_buried.ncl b/tests/imports/unexpected_token_buried.ncl
new file mode 100644
index 00000000..e589efda
--- /dev/null
+++ b/tests/imports/unexpected_token_buried.ncl
@@ -0,0 +1,6 @@
+{ 
+  foo = {
+    bar = [2 @ ],
+  }, 
+  baz = 2 
+}
\ No newline at end of file
diff --git a/tests/parse_fail.rs b/tests/parse_fail.rs
index b65ee8b3..6539309a 100644
--- a/tests/parse_fail.rs
+++ b/tests/parse_fail.rs
@@ -12,3 +12,11 @@ fn unexpected_token() {
 fn unexpected_token_in_record() {
     assert_matches!(eval("{ name = \"Nickel\",, }"), Err(Error::ParseErrors(..)));
 }
+
+#[test]
+fn unexpected_token_buried() {
+    assert_matches!(
+        eval("{ foo = {bar = [2 @ ],}, baz = 2 }"),
+        Err(Error::ParseErrors(..))
+    );
+}
diff --git a/tests/stdlib_typecheck.rs b/tests/stdlib_typecheck.rs
index 2ed9894e..3c0e9a75 100644
--- a/tests/stdlib_typecheck.rs
+++ b/tests/stdlib_typecheck.rs
@@ -1,9 +1,9 @@
 use assert_matches::assert_matches;
-use nickel_lang::cache::Cache;
+use nickel_lang::cache::{Cache, ErrorTolerance};
 
 #[test]
 fn stdlib_typecheck() {
-    let mut cache = Cache::new();
+    let mut cache = Cache::new(ErrorTolerance::Strict);
     assert_matches!(cache.load_stdlib(), Ok(_));
     assert_matches!(cache.typecheck_stdlib(), Ok(_));
 }