diff --git a/core/src/eval/mod.rs b/core/src/eval/mod.rs index 6ebd951db6..10d167ca5d 100644 --- a/core/src/eval/mod.rs +++ b/core/src/eval/mod.rs @@ -935,6 +935,53 @@ impl VirtualMachine { }) } } + + /// Evaluate a term, but attempt to continue on errors. + /// + /// This differs from `VirtualMachine::eval_full` in 3 ways: + /// - We try to accumulate errors instead of bailing out. When recursing into record + /// fields and array elements, we keep evaluating subsequent elements even if one + /// fails. + /// - We ignore missing field errors. It would be nice not to ignore them, but it's hard + /// to tell when they're appropriate: the term might intentionally be a partial configuration. + /// - We only return the accumulated errors; we don't return the eval'ed term. + pub fn eval_permissive(&mut self, rt: RichTerm) -> Vec { + fn inner( + slf: &mut VirtualMachine, + acc: &mut Vec, + rt: RichTerm, + ) { + match slf.eval(rt) { + Err(e) => acc.push(e), + Ok(t) => match t.as_ref() { + Term::Array(ts, _) => { + for t in ts.iter() { + // After eval_closure, all the array elements are + // closurized already, so we don't need to do any tracking + // of the env. + inner(slf, acc, t.clone()); + } + } + Term::Record(data) => { + for field in data.fields.values() { + if let Some(v) = &field.value { + let value_with_ctr = RuntimeContract::apply_all( + v.clone(), + field.pending_contracts.iter().cloned(), + v.pos, + ); + inner(slf, acc, value_with_ctr); + } + } + } + _ => {} + }, + } + } + let mut ret = Vec::new(); + inner(self, &mut ret, rt); + ret + } } impl VirtualMachine { diff --git a/lsp/nls/src/background.rs b/lsp/nls/src/background.rs index 5bfcb7a85a..71e6e98ab2 100644 --- a/lsp/nls/src/background.rs +++ b/lsp/nls/src/background.rs @@ -10,13 +10,8 @@ use ipc_channel::ipc::{IpcOneShotServer, IpcReceiver, IpcSender}; use log::warn; use lsp_types::Url; use nickel_lang_core::{ - cache::{ImportResolver, SourcePath}, - error::EvalError, - eval::{ - cache::{Cache, CacheImpl}, - VirtualMachine, - }, - term::{RichTerm, RuntimeContract, Term}, + cache::SourcePath, + eval::{cache::CacheImpl, VirtualMachine}, }; use serde::{Deserialize, Serialize}; @@ -69,47 +64,6 @@ fn drain_ready Deserialize<'de> + Serialize>(rx: &IpcReceiver, bu } } -// Evaluate `rt` and collect errors. -// -// This differs from `VirtualMachine::eval_full` in 2 ways: -// - We try to accumulate errors instead of bailing out. When recursing into record -// fields and array elements, we keep evaluating subsequent elements even if one -// fails. -// - We ignore missing field errors. It would be nice not to ignore them, but it's hard -// to tell when they're appropriate: the term might intentionally be a partial configuration. -fn eval_permissive( - vm: &mut VirtualMachine, - errors: &mut Vec, - rt: RichTerm, -) { - match vm.eval(rt) { - Err(e) => errors.push(e), - Ok(t) => match t.as_ref() { - Term::Array(ts, _) => { - for t in ts.iter() { - // After eval_closure, all the array elements are - // closurized already, so we don't need to do any tracking - // of the env. - eval_permissive(vm, errors, t.clone()); - } - } - Term::Record(data) => { - for field in data.fields.values() { - if let Some(v) = &field.value { - let value_with_ctr = RuntimeContract::apply_all( - v.clone(), - field.pending_contracts.iter().cloned(), - v.pos, - ); - eval_permissive(vm, errors, value_with_ctr); - } - } - } - _ => {} - }, - } -} - // Returning an Option, just to let us use `?` when channels disconnect. fn worker(cmd_rx: IpcReceiver, response_tx: IpcSender) -> Option<()> { let mut evals = Vec::new(); @@ -167,8 +121,7 @@ fn worker(cmd_rx: IpcReceiver, response_tx: IpcSender) -> Opt // We've already checked that parsing and typechecking are successful, so we // don't expect further errors. let rt = vm.prepare_eval(file_id).unwrap(); - let mut errors = Vec::new(); - eval_permissive(&mut vm, &mut errors, rt); + let errors = vm.eval_permissive(rt); diagnostics.extend( errors .into_iter() @@ -281,6 +234,8 @@ impl SupervisorState { } } + // The Result return type is only there to allow the ? shortcuts. In fact, + // this only ever returns errors. fn run_one(&mut self) -> Result<(), SupervisorError> { loop { let mut select = Select::new(); @@ -334,6 +289,7 @@ impl SupervisorState { fn run(&mut self) { loop { match self.run_one() { + // unreachable because run_one only returns on error Ok(_) => unreachable!(), Err(SupervisorError::MainExited) => break, Err(SupervisorError::WorkerExited) => { diff --git a/lsp/nls/src/command.rs b/lsp/nls/src/command.rs index 0e6ebbef88..7a0a9d19fb 100644 --- a/lsp/nls/src/command.rs +++ b/lsp/nls/src/command.rs @@ -23,10 +23,10 @@ pub fn handle_command( } fn eval(server: &mut Server, uri: &Url) -> Result<(), Error> { - let cache = &mut server.world.cache; - if let Some(file_id) = cache.file_id(uri)? { + if let Some(file_id) = server.world.cache.file_id(uri)? { // TODO: avoid cloning the cache. Maybe we can have a VM with a &mut Cache? - let mut vm = VirtualMachine::<_, CacheImpl>::new(cache.clone(), std::io::stderr()); + let mut vm = + VirtualMachine::<_, CacheImpl>::new(server.world.cache.clone(), std::io::stderr()); let rt = vm.prepare_eval(file_id)?; if let Err(e) = vm.eval_full(rt) { let diags = server.world.lsp_diagnostics(file_id, e);