From 05f42519b53575ad3235b5e0a0cd7d71f04076a5 Mon Sep 17 00:00:00 2001 From: Adam Joseph Date: Sat, 9 Sep 2023 22:02:56 -0700 Subject: [PATCH] fix(tvix/eval): fix b/281 by adding Value::Catchable This commit makes catchable errors a variant of Value. The main downside of this approach is that we lose the ability to use Rust's `?` syntax for propagating catchable errors. Change-Id: Ibe89438d8a70dcec29e016df692b5bf88a5cad13 Reviewed-on: https://cl.tvl.fyi/c/depot/+/9289 Reviewed-by: tazjin Autosubmit: Adam Joseph Tested-by: BuildkiteCI --- tvix/cli/src/derivation.rs | 116 ++++++++++++++++++++----------- tvix/eval/src/builtins/impure.rs | 57 ++++++++------- tvix/eval/src/builtins/mod.rs | 57 ++++++++++----- tvix/eval/src/builtins/to_xml.rs | 4 ++ tvix/eval/src/compiler/import.rs | 12 ++-- tvix/eval/src/compiler/mod.rs | 6 +- tvix/eval/src/errors.rs | 61 +++++++--------- tvix/eval/src/lib.rs | 2 +- tvix/eval/src/nix_search_path.rs | 39 ++++++----- tvix/eval/src/tests/mod.rs | 13 ++-- tvix/eval/src/value/attrs.rs | 2 +- tvix/eval/src/value/json.rs | 20 ++++-- tvix/eval/src/value/mod.rs | 57 +++++++++++++-- tvix/eval/src/vm/generators.rs | 28 ++++---- tvix/eval/src/vm/mod.rs | 92 +++++++----------------- tvix/serde/src/de.rs | 1 + 16 files changed, 320 insertions(+), 247 deletions(-) diff --git a/tvix/cli/src/derivation.rs b/tvix/cli/src/derivation.rs index de6d58a13..76cc3f60f 100644 --- a/tvix/cli/src/derivation.rs +++ b/tvix/cli/src/derivation.rs @@ -6,7 +6,9 @@ use std::collections::{btree_map, BTreeSet}; use std::rc::Rc; use tvix_eval::builtin_macros::builtins; use tvix_eval::generators::{self, emit_warning_kind, GenCo}; -use tvix_eval::{AddContext, CoercionKind, ErrorKind, NixAttrs, NixList, Value, WarningKind}; +use tvix_eval::{ + AddContext, CatchableErrorKind, CoercionKind, ErrorKind, NixAttrs, NixList, Value, WarningKind, +}; use crate::errors::Error; use crate::known_paths::{KnownPaths, PathKind, PathName}; @@ -150,19 +152,22 @@ async fn handle_derivation_parameters( name: &str, value: &Value, val_str: &str, -) -> Result { +) -> Result, ErrorKind> { match name { - IGNORE_NULLS => return Ok(false), + IGNORE_NULLS => return Ok(Ok(false)), // Command line arguments to the builder. "args" => { let args = value.to_list()?; for arg in args { - drv.arguments.push(strong_coerce_to_string(co, arg).await?); + match strong_coerce_to_string(co, arg).await? { + Err(cek) => return Ok(Err(cek)), + Ok(s) => drv.arguments.push(s), + } } // The arguments do not appear in the environment. - return Ok(false); + return Ok(Ok(false)); } // Explicitly specified drv outputs (instead of default [ "out" ]) @@ -185,14 +190,18 @@ async fn handle_derivation_parameters( _ => {} } - Ok(true) + Ok(Ok(true)) } -async fn strong_coerce_to_string(co: &GenCo, val: Value) -> Result { +async fn strong_coerce_to_string( + co: &GenCo, + val: Value, +) -> Result, ErrorKind> { let val = generators::request_force(co, val).await; - let val_str = generators::request_string_coerce(co, val, CoercionKind::Strong).await; - - Ok(val_str.as_str().to_string()) + match generators::request_string_coerce(co, val, CoercionKind::Strong).await { + Err(cek) => Ok(Err(cek)), + Ok(val_str) => Ok(Ok(val_str.as_str().to_string())), + } } #[builtins(state = "Rc>")] @@ -256,12 +265,15 @@ mod derivation_builtins { co: &GenCo, attrs: &NixAttrs, key: &str, - ) -> Result, ErrorKind> { + ) -> Result, CatchableErrorKind>, ErrorKind> { if let Some(attr) = attrs.select(key) { - return Ok(Some(strong_coerce_to_string(co, attr.clone()).await?)); + match strong_coerce_to_string(co, attr.clone()).await? { + Err(cek) => return Ok(Err(cek)), + Ok(str) => return Ok(Ok(Some(str))), + } } - Ok(None) + Ok(Ok(None)) } for (name, value) in input.clone().into_iter_sorted() { @@ -270,38 +282,60 @@ mod derivation_builtins { continue; } - let val_str = strong_coerce_to_string(&co, value.clone()).await?; + match strong_coerce_to_string(&co, value.clone()).await? { + Err(cek) => return Ok(Value::Catchable(cek)), + Ok(val_str) => { + // handle_derivation_parameters tells us whether the + // argument should be added to the environment; continue + // to the next one otherwise + match handle_derivation_parameters( + &mut drv, + &co, + name.as_str(), + &value, + &val_str, + ) + .await? + { + Err(cek) => return Ok(Value::Catchable(cek)), + Ok(false) => continue, + _ => (), + } - // handle_derivation_parameters tells us whether the - // argument should be added to the environment; continue - // to the next one otherwise - if !handle_derivation_parameters(&mut drv, &co, name.as_str(), &value, &val_str).await? - { - continue; - } - - // Most of these are also added to the builder's environment in "raw" form. - if drv - .environment - .insert(name.as_str().to_string(), val_str.into()) - .is_some() - { - return Err(Error::DuplicateEnvVar(name.as_str().to_string()).into()); + // Most of these are also added to the builder's environment in "raw" form. + if drv + .environment + .insert(name.as_str().to_string(), val_str.into()) + .is_some() + { + return Err(Error::DuplicateEnvVar(name.as_str().to_string()).into()); + } + } } } - populate_output_configuration( - &mut drv, - select_string(&co, &input, "outputHash") - .await - .context("evaluating the `outputHash` parameter")?, - select_string(&co, &input, "outputHashAlgo") - .await - .context("evaluating the `outputHashAlgo` parameter")?, - select_string(&co, &input, "outputHashMode") - .await - .context("evaluating the `outputHashMode` parameter")?, - )?; + let output_hash = match select_string(&co, &input, "outputHash") + .await + .context("evaluating the `outputHash` parameter")? + { + Err(cek) => return Ok(Value::Catchable(cek)), + Ok(s) => s, + }; + let output_hash_algo = match select_string(&co, &input, "outputHashAlgo") + .await + .context("evaluating the `outputHashAlgo` parameter")? + { + Err(cek) => return Ok(Value::Catchable(cek)), + Ok(s) => s, + }; + let output_hash_mode = match select_string(&co, &input, "outputHashMode") + .await + .context("evaluating the `outputHashMode` parameter")? + { + Err(cek) => return Ok(Value::Catchable(cek)), + Ok(s) => s, + }; + populate_output_configuration(&mut drv, output_hash, output_hash_algo, output_hash_mode)?; // Scan references in relevant attributes to detect any build-references. let references = { diff --git a/tvix/eval/src/builtins/impure.rs b/tvix/eval/src/builtins/impure.rs index 26d1ba494..5324fd565 100644 --- a/tvix/eval/src/builtins/impure.rs +++ b/tvix/eval/src/builtins/impure.rs @@ -27,40 +27,47 @@ mod impure_builtins { #[builtin("pathExists")] async fn builtin_path_exists(co: GenCo, path: Value) -> Result { - let path = coerce_value_to_path(&co, path).await?; - Ok(generators::request_path_exists(&co, path).await) + match coerce_value_to_path(&co, path).await? { + Err(cek) => Ok(Value::Catchable(cek)), + Ok(path) => Ok(generators::request_path_exists(&co, path).await), + } } #[builtin("readDir")] async fn builtin_read_dir(co: GenCo, path: Value) -> Result { - let path = coerce_value_to_path(&co, path).await?; + match coerce_value_to_path(&co, path).await? { + Err(cek) => Ok(Value::Catchable(cek)), + Ok(path) => { + let dir = generators::request_read_dir(&co, path).await; + let res = dir.into_iter().map(|(name, ftype)| { + ( + // TODO: propagate Vec or bytes::Bytes into NixString. + NixString::from( + String::from_utf8(name.to_vec()).expect("parsing file name as string"), + ), + Value::String( + match ftype { + FileType::Directory => "directory", + FileType::Regular => "regular", + FileType::Symlink => "symlink", + FileType::Unknown => "unknown", + } + .into(), + ), + ) + }); - let dir = generators::request_read_dir(&co, path).await; - let res = dir.into_iter().map(|(name, ftype)| { - ( - // TODO: propagate Vec or bytes::Bytes into NixString. - NixString::from( - String::from_utf8(name.to_vec()).expect("parsing file name as string"), - ), - Value::String( - match ftype { - FileType::Directory => "directory", - FileType::Regular => "regular", - FileType::Symlink => "symlink", - FileType::Unknown => "unknown", - } - .into(), - ), - ) - }); - - Ok(Value::attrs(NixAttrs::from_iter(res))) + Ok(Value::attrs(NixAttrs::from_iter(res))) + } + } } #[builtin("readFile")] async fn builtin_read_file(co: GenCo, path: Value) -> Result { - let path = coerce_value_to_path(&co, path).await?; - Ok(generators::request_read_to_string(&co, path).await) + match coerce_value_to_path(&co, path).await? { + Err(cek) => Ok(Value::Catchable(cek)), + Ok(path) => Ok(generators::request_read_to_string(&co, path).await), + } } } diff --git a/tvix/eval/src/builtins/mod.rs b/tvix/eval/src/builtins/mod.rs index 322f91710..13355fa1c 100644 --- a/tvix/eval/src/builtins/mod.rs +++ b/tvix/eval/src/builtins/mod.rs @@ -40,18 +40,25 @@ pub const CURRENT_PLATFORM: &str = env!("TVIX_CURRENT_SYSTEM"); /// builtin. This coercion can _never_ be performed in a Nix program /// without using builtins (i.e. the trick `path: /. + path` to /// convert from a string to a path wouldn't hit this code). -pub async fn coerce_value_to_path(co: &GenCo, v: Value) -> Result { +pub async fn coerce_value_to_path( + co: &GenCo, + v: Value, +) -> Result, ErrorKind> { let value = generators::request_force(co, v).await; if let Value::Path(p) = value { - return Ok(*p); + return Ok(Ok(*p)); } - let vs = generators::request_string_coerce(co, value, CoercionKind::Weak).await; - let path = PathBuf::from(vs.as_str()); - if path.is_absolute() { - Ok(path) - } else { - Err(ErrorKind::NotAnAbsolutePath(path)) + match generators::request_string_coerce(co, value, CoercionKind::Weak).await { + Ok(vs) => { + let path = PathBuf::from(vs.as_str()); + if path.is_absolute() { + Ok(Ok(path)) + } else { + Err(ErrorKind::NotAnAbsolutePath(path)) + } + } + Err(cek) => Ok(Err(cek)), } } @@ -218,8 +225,10 @@ mod pure_builtins { if i != 0 { res.push_str(&separator); } - let s = generators::request_string_coerce(&co, val, CoercionKind::Weak).await; - res.push_str(s.as_str()); + match generators::request_string_coerce(&co, val, CoercionKind::Weak).await { + Ok(s) => res.push_str(s.as_str()), + Err(c) => return Ok(Value::Catchable(c)), + } } Ok(res.into()) } @@ -313,6 +322,9 @@ mod pure_builtins { // and our tests for foldl'. nul = generators::request_call_with(&co, op.clone(), [nul, val]).await; nul = generators::request_force(&co, nul).await; + if let c @ Value::Catchable(_) = nul { + return Ok(c); + } } Ok(nul) @@ -340,9 +352,13 @@ mod pure_builtins { #[builtin("toJSON")] async fn builtin_to_json(co: GenCo, val: Value) -> Result { - let json_value = val.to_json(&co).await?; - let json_str = serde_json::to_string(&json_value)?; - Ok(json_str.into()) + match val.to_json(&co).await? { + Err(cek) => Ok(Value::Catchable(cek)), + Ok(json_value) => { + let json_str = serde_json::to_string(&json_value)?; + Ok(json_str.into()) + } + } } #[builtin("fromTOML")] @@ -893,7 +909,7 @@ mod pure_builtins { #[builtin("throw")] async fn builtin_throw(co: GenCo, message: Value) -> Result { - Err(ErrorKind::CatchableErrorKind(CatchableErrorKind::Throw( + Ok(Value::Catchable(CatchableErrorKind::Throw( message.to_str()?.to_string(), ))) } @@ -929,15 +945,20 @@ mod pure_builtins { #[builtin("toPath")] async fn builtin_to_path(co: GenCo, s: Value) -> Result { - let path: Value = crate::value::canon_path(coerce_value_to_path(&co, s).await?).into(); - Ok(path.coerce_to_string(co, CoercionKind::Weak).await?) + match coerce_value_to_path(&co, s).await? { + Err(cek) => return Ok(Value::Catchable(cek)), + Ok(path) => { + let path: Value = crate::value::canon_path(path).into(); + Ok(path.coerce_to_string(co, CoercionKind::Weak).await?) + } + } } #[builtin("tryEval")] async fn builtin_try_eval(co: GenCo, #[lazy] e: Value) -> Result { let res = match generators::request_try_force(&co, e).await { - Some(value) => [("value", value), ("success", true.into())], - None => [("value", false.into()), ("success", false.into())], + Value::Catchable(_) => [("value", false.into()), ("success", false.into())], + value => [("value", value), ("success", true.into())], }; Ok(Value::attrs(NixAttrs::from_iter(res.into_iter()))) diff --git a/tvix/eval/src/builtins/to_xml.rs b/tvix/eval/src/builtins/to_xml.rs index 9fb6bb5f5..6d486d356 100644 --- a/tvix/eval/src/builtins/to_xml.rs +++ b/tvix/eval/src/builtins/to_xml.rs @@ -134,6 +134,10 @@ fn value_variant_to_xml(w: &mut EventWriter, value: &Value) -> Resu metadata: Some(Rc::new(value.clone())), }) } + + Value::Catchable(_) => { + panic!("tvix bug: value_to_xml() called on a value which had not been deep-forced") + } }?; Ok(()) diff --git a/tvix/eval/src/compiler/import.rs b/tvix/eval/src/compiler/import.rs index 59e3d92e8..94a6602e9 100644 --- a/tvix/eval/src/compiler/import.rs +++ b/tvix/eval/src/compiler/import.rs @@ -25,7 +25,10 @@ async fn import_impl( mut args: Vec, ) -> Result { // TODO(sterni): canon_path()? - let mut path = coerce_value_to_path(&co, args.pop().unwrap()).await?; + let mut path = match coerce_value_to_path(&co, args.pop().unwrap()).await? { + Err(cek) => return Ok(Value::Catchable(cek)), + Ok(path) => path, + }; if path.is_dir() { path.push("default.nix"); @@ -36,11 +39,8 @@ async fn import_impl( } // TODO(tazjin): make this return a string directly instead - let contents = generators::request_read_to_string(&co, path.clone()) - .await - .to_str()? - .as_str() - .to_string(); + let contents: Value = generators::request_read_to_string(&co, path.clone()).await; + let contents = contents.to_str()?.as_str().to_string(); let parsed = rnix::ast::Root::parse(&contents); let errors = parsed.errors(); diff --git a/tvix/eval/src/compiler/mod.rs b/tvix/eval/src/compiler/mod.rs index 5c2825507..cfd50bec5 100644 --- a/tvix/eval/src/compiler/mod.rs +++ b/tvix/eval/src/compiler/mod.rs @@ -396,11 +396,11 @@ impl Compiler<'_> { } else if raw_path.starts_with('<') { // TODO: decide what to do with findFile if raw_path.len() == 2 { - return self.emit_error( - node, - ErrorKind::CatchableErrorKind(CatchableErrorKind::NixPathResolution( + return self.emit_constant( + Value::Catchable(CatchableErrorKind::NixPathResolution( "Empty <> path not allowed".into(), )), + node, ); } let path = &raw_path[1..(raw_path.len() - 1)]; diff --git a/tvix/eval/src/errors.rs b/tvix/eval/src/errors.rs index 54def8d33..a61d55aa2 100644 --- a/tvix/eval/src/errors.rs +++ b/tvix/eval/src/errors.rs @@ -16,7 +16,28 @@ use crate::spans::ToSpan; use crate::value::{CoercionKind, NixString}; use crate::{SourceCode, Value}; -/// "CatchableErrorKind" errors -- those which can be detected by `builtins.tryEval`. +/// "CatchableErrorKind" errors -- those which can be detected by +/// `builtins.tryEval`. +/// +/// Note: this type is deliberately *not* incorporated as a variant +/// of ErrorKind, because then Result would have +/// redundant representations for catchable errors, which would make +/// it too easy to handle errors incorrectly: +/// +/// - Ok(Value::Catchable(cek)) +/// - Err(ErrorKind::ThisVariantDoesNotExist(cek)) +/// +/// Because CatchableErrorKind is not a variant of ErrorKind, you +/// will often see functions which return a type like: +/// +/// Result,ErrorKind> +/// +/// ... where T is any type other than Value. This is unfortunate, +/// because Rust's magic `?`-syntax does not work on nested Result +/// values like this. +/// +/// TODO(amjoseph): investigate result> +/// #[derive(Clone, Debug)] pub enum CatchableErrorKind { Throw(String), @@ -180,14 +201,6 @@ pub enum ErrorKind { context: String, underlying: Box, }, - - CatchableErrorKind(CatchableErrorKind), -} - -impl From for ErrorKind { - fn from(c: CatchableErrorKind) -> ErrorKind { - ErrorKind::CatchableErrorKind(c) - } } impl error::Error for Error { @@ -243,17 +256,6 @@ impl From for ErrorKind { } } -impl ErrorKind { - /// Returns `true` if this error can be caught by `builtins.tryEval` - pub fn is_catchable(&self) -> bool { - match self { - Self::CatchableErrorKind(_) => true, - Self::NativeError { err, .. } | Self::BytecodeError(err) => err.kind.is_catchable(), - _ => false, - } - } -} - impl From for ErrorKind { fn from(err: serde_json::Error) -> Self { // Can't just put the `serde_json::Error` in the ErrorKind since it doesn't impl `Clone` @@ -297,13 +299,7 @@ impl Error { impl Display for ErrorKind { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match &self { - ErrorKind::CatchableErrorKind(CatchableErrorKind::Throw(msg)) => { - write!(f, "error thrown: {}", msg) - } ErrorKind::Abort(msg) => write!(f, "evaluation aborted: {}", msg), - ErrorKind::CatchableErrorKind(CatchableErrorKind::AssertionFailed) => { - write!(f, "assertion failed") - } ErrorKind::DivisionByZero => write!(f, "division by zero"), @@ -340,8 +336,7 @@ impl Display for ErrorKind { write!(f, "can not compare a {} with a {}", lhs, rhs) } - ErrorKind::CatchableErrorKind(CatchableErrorKind::NixPathResolution(err)) - | ErrorKind::RelativePathResolution(err) => { + ErrorKind::RelativePathResolution(err) => { write!(f, "could not resolve path: {}", err) } @@ -741,15 +736,12 @@ impl Error { let label = match &self.kind { ErrorKind::DuplicateAttrsKey { .. } => "in this attribute set", ErrorKind::InvalidAttributeName(_) => "in this attribute set", - ErrorKind::CatchableErrorKind(CatchableErrorKind::NixPathResolution(_)) - | ErrorKind::RelativePathResolution(_) => "in this path literal", + ErrorKind::RelativePathResolution(_) => "in this path literal", ErrorKind::UnexpectedArgument { .. } => "in this function call", // The spans for some errors don't have any more descriptive stuff // in them, or we don't utilise it yet. - ErrorKind::CatchableErrorKind(CatchableErrorKind::Throw(_)) - | ErrorKind::Abort(_) - | ErrorKind::CatchableErrorKind(CatchableErrorKind::AssertionFailed) + ErrorKind::Abort(_) | ErrorKind::AttributeNotFound { .. } | ErrorKind::IndexOutOfBounds { .. } | ErrorKind::TailEmptyList @@ -790,14 +782,11 @@ impl Error { /// used to refer users to documentation. fn code(&self) -> &'static str { match self.kind { - ErrorKind::CatchableErrorKind(CatchableErrorKind::Throw(_)) => "E001", ErrorKind::Abort(_) => "E002", - ErrorKind::CatchableErrorKind(CatchableErrorKind::AssertionFailed) => "E003", ErrorKind::InvalidAttributeName { .. } => "E004", ErrorKind::AttributeNotFound { .. } => "E005", ErrorKind::TypeError { .. } => "E006", ErrorKind::Incomparable { .. } => "E007", - ErrorKind::CatchableErrorKind(CatchableErrorKind::NixPathResolution(_)) => "E008", ErrorKind::DynamicKeyInScope(_) => "E009", ErrorKind::UnknownStaticVariable => "E010", ErrorKind::UnknownDynamicVariable(_) => "E011", diff --git a/tvix/eval/src/lib.rs b/tvix/eval/src/lib.rs index 3ce98bc47..4227014c2 100644 --- a/tvix/eval/src/lib.rs +++ b/tvix/eval/src/lib.rs @@ -48,7 +48,7 @@ use crate::vm::run_lambda; // Re-export the public interface used by other crates. pub use crate::compiler::{compile, prepare_globals, CompilationOutput}; -pub use crate::errors::{AddContext, Error, ErrorKind, EvalResult}; +pub use crate::errors::{AddContext, CatchableErrorKind, Error, ErrorKind, EvalResult}; pub use crate::io::{DummyIO, EvalIO, FileType}; pub use crate::pretty_ast::pretty_print_expr; pub use crate::source::SourceCode; diff --git a/tvix/eval/src/nix_search_path.rs b/tvix/eval/src/nix_search_path.rs index 3e553e62b..f86cffd6d 100644 --- a/tvix/eval/src/nix_search_path.rs +++ b/tvix/eval/src/nix_search_path.rs @@ -124,22 +124,24 @@ pub struct NixSearchPath { impl NixSearchPath { /// Attempt to resolve the given `path` within this [`NixSearchPath`] using the /// path resolution rules for `<...>`-style paths - pub fn resolve

(&self, io: &mut dyn EvalIO, path: P) -> Result + pub fn resolve

( + &self, + io: &mut dyn EvalIO, + path: P, + ) -> Result, ErrorKind> where P: AsRef, { let path = path.as_ref(); for entry in &self.entries { if let Some(p) = entry.resolve(io, path)? { - return Ok(p); + return Ok(Ok(p)); } } - Err(ErrorKind::CatchableErrorKind( - CatchableErrorKind::NixPathResolution(format!( - "path '{}' was not found in the Nix search path", - path.display() - )), - )) + Ok(Err(CatchableErrorKind::NixPathResolution(format!( + "path '{}' was not found in the Nix search path", + path.display() + )))) } } @@ -204,19 +206,19 @@ mod tests { let nix_search_path = NixSearchPath::from_str("./.").unwrap(); let mut io = StdIO {}; let res = nix_search_path.resolve(&mut io, "src").unwrap(); - assert_eq!(res, current_dir().unwrap().join("src").clean()); + assert_eq!( + res.unwrap().to_path_buf(), + current_dir().unwrap().join("src").clean() + ); } #[test] fn failed_resolution() { let nix_search_path = NixSearchPath::from_str("./.").unwrap(); let mut io = StdIO {}; - let err = nix_search_path.resolve(&mut io, "nope").unwrap_err(); + let err = nix_search_path.resolve(&mut io, "nope").unwrap(); assert!( - matches!( - err, - ErrorKind::CatchableErrorKind(CatchableErrorKind::NixPathResolution(..)) - ), + matches!(err, Err(CatchableErrorKind::NixPathResolution(..))), "err = {err:?}" ); } @@ -226,7 +228,7 @@ mod tests { let nix_search_path = NixSearchPath::from_str("./.:/").unwrap(); let mut io = StdIO {}; let res = nix_search_path.resolve(&mut io, "etc").unwrap(); - assert_eq!(res, Path::new("/etc")); + assert_eq!(res.unwrap().to_path_buf(), Path::new("/etc")); } #[test] @@ -234,7 +236,10 @@ mod tests { let nix_search_path = NixSearchPath::from_str("/:tvix=.").unwrap(); let mut io = StdIO {}; let res = nix_search_path.resolve(&mut io, "tvix/src").unwrap(); - assert_eq!(res, current_dir().unwrap().join("src").clean()); + assert_eq!( + res.unwrap().to_path_buf(), + current_dir().unwrap().join("src").clean() + ); } #[test] @@ -242,7 +247,7 @@ mod tests { let nix_search_path = NixSearchPath::from_str("/:tvix=.").unwrap(); let mut io = StdIO {}; let res = nix_search_path.resolve(&mut io, "tvix").unwrap(); - assert_eq!(res, current_dir().unwrap().clean()); + assert_eq!(res.unwrap().to_path_buf(), current_dir().unwrap().clean()); } } } diff --git a/tvix/eval/src/tests/mod.rs b/tvix/eval/src/tests/mod.rs index 02227a7e9..b7bea6094 100644 --- a/tvix/eval/src/tests/mod.rs +++ b/tvix/eval/src/tests/mod.rs @@ -1,3 +1,4 @@ +use crate::value::Value; use builtin_macros::builtins; use pretty_assertions::assert_eq; use test_generator::test_resources; @@ -57,19 +58,23 @@ fn eval_test(code_path: &str, expect_success: bool) { eval.builtins.extend(mock_builtins::builtins()); let result = eval.evaluate(); - - if expect_success && !result.errors.is_empty() { + let failed = match result.value { + Some(Value::Catchable(_)) => true, + _ => !result.errors.is_empty(), + }; + if expect_success && failed { panic!( "{code_path}: evaluation of eval-okay test should succeed, but failed with {:?}", result.errors, ); } - if !expect_success && !result.errors.is_empty() { + if !expect_success && failed { return; } - let result_str = result.value.unwrap().to_string(); + let value = result.value.unwrap(); + let result_str = value.to_string(); if let Ok(exp) = std::fs::read_to_string(exp_path) { if expect_success { diff --git a/tvix/eval/src/value/attrs.rs b/tvix/eval/src/value/attrs.rs index e4840cc88..15a709730 100644 --- a/tvix/eval/src/value/attrs.rs +++ b/tvix/eval/src/value/attrs.rs @@ -393,7 +393,7 @@ impl NixAttrs { // /another/ set with a __toString attr. let s = generators::request_string_coerce(co, result, kind).await; - return Some(s); + return Some(s.ok()?); } None diff --git a/tvix/eval/src/value/json.rs b/tvix/eval/src/value/json.rs index 38496f8f3..1290cce14 100644 --- a/tvix/eval/src/value/json.rs +++ b/tvix/eval/src/value/json.rs @@ -4,15 +4,18 @@ /// as there is internal Nix logic that must happen within the /// serialisation methods. use super::{CoercionKind, Value}; +use crate::errors::{CatchableErrorKind, ErrorKind}; use crate::generators::{self, GenCo}; -use crate::ErrorKind; use serde_json::value::to_value; use serde_json::Value as Json; // name clash with *our* `Value` use serde_json::{Map, Number}; impl Value { - pub(crate) async fn to_json(self, co: &GenCo) -> Result { + pub(crate) async fn to_json( + self, + co: &GenCo, + ) -> Result, ErrorKind> { let self_forced = generators::request_force(co, self).await; let value = match self_forced { @@ -42,14 +45,14 @@ impl Value { // serialise to the string-coerced version of the result of // calling that. if let Some(s) = attrs.try_to_string(co, CoercionKind::Weak).await { - return Ok(Json::String(s.as_str().to_string())); + return Ok(Ok(Json::String(s.as_str().to_string()))); } // Attribute sets with an `outPath` attribute // serialise to a JSON serialisation of that inner // value (regardless of what it is!). if let Some(out_path) = attrs.select("outPath") { - return Ok(generators::request_to_json(co, out_path.clone()).await); + return Ok(Ok(generators::request_to_json(co, out_path.clone()).await)); } let mut out = Map::with_capacity(attrs.len()); @@ -63,6 +66,8 @@ impl Value { Json::Object(out) } + Value::Catchable(c) => return Ok(Err(c)), + val @ Value::Closure(_) | val @ Value::Thunk(_) | val @ Value::Builtin(_) @@ -76,12 +81,15 @@ impl Value { } }; - Ok(value) + Ok(Ok(value)) } /// Generator version of the above, which wraps responses in /// Value::Json. pub(crate) async fn to_json_generator(self, co: GenCo) -> Result { - Ok(Value::Json(self.to_json(&co).await?)) + match self.to_json(&co).await? { + Err(cek) => Ok(Value::Catchable(cek)), + Ok(json) => Ok(Value::Json(json)), + } } } diff --git a/tvix/eval/src/value/mod.rs b/tvix/eval/src/value/mod.rs index 254bbbc09..f5b373e3c 100644 --- a/tvix/eval/src/value/mod.rs +++ b/tvix/eval/src/value/mod.rs @@ -20,7 +20,7 @@ mod path; mod string; mod thunk; -use crate::errors::ErrorKind; +use crate::errors::{CatchableErrorKind, ErrorKind}; use crate::opcode::StackIdx; use crate::spans::LightSpan; use crate::vm::generators::{self, GenCo}; @@ -81,6 +81,24 @@ pub enum Value { #[serde(skip)] FinaliseRequest(bool), + + #[serde(skip)] + Catchable(CatchableErrorKind), +} + +impl From for Value { + fn from(c: CatchableErrorKind) -> Value { + Value::Catchable(c) + } +} + +impl From> for Value +where + Value: From, +{ + fn from(v: Result) -> Value { + v.map_or_else(|cek| Value::Catchable(cek), |v| v.into()) + } } lazy_static! { @@ -222,18 +240,28 @@ impl Value { Value::List(list) => { for val in list { - generators::request_deep_force(&co, val.clone(), thunk_set.clone()).await; + if let c @ Value::Catchable(_) = + generators::request_deep_force(&co, val.clone(), thunk_set.clone()).await + { + return Ok(c); + } } } Value::Attrs(attrs) => { for (_, val) in attrs.iter() { - generators::request_deep_force(&co, val.clone(), thunk_set.clone()).await; + if let c @ Value::Catchable(_) = + generators::request_deep_force(&co, val.clone(), thunk_set.clone()).await + { + return Ok(c); + } } } Value::Thunk(_) => panic!("Tvix bug: force_value() returned a thunk"), + Value::Catchable(_) => return Ok(value), + Value::AttrNotFound | Value::Blueprint(_) | Value::DeferredUpvalue(_) @@ -279,8 +307,12 @@ impl Value { } if let Some(out_path) = attrs.select("outPath") { - let s = generators::request_string_coerce(&co, out_path.clone(), kind).await; - return Ok(Value::String(s)); + return match generators::request_string_coerce(&co, out_path.clone(), kind) + .await + { + Ok(s) => Ok(Value::String(s)), + Err(c) => Ok(Value::Catchable(c)), + }; } Err(ErrorKind::NotCoercibleToString { from: "set", kind }) @@ -308,8 +340,10 @@ impl Value { out.push(' '); } - let s = generators::request_string_coerce(&co, elem, kind).await; - out.push_str(s.as_str()); + match generators::request_string_coerce(&co, elem, kind).await { + Ok(s) => out.push_str(s.as_str()), + Err(c) => return Ok(Value::Catchable(c)), + } } Ok(Value::String(out.into())) @@ -328,6 +362,8 @@ impl Value { kind, }), + (c @ Value::Catchable(_), _) => return Ok(c), + (Value::AttrNotFound, _) | (Value::Blueprint(_), _) | (Value::DeferredUpvalue(_), _) @@ -384,6 +420,8 @@ impl Value { let result = match (a, b) { // Trivial comparisons + (c @ Value::Catchable(_), _) => return Ok(c), + (_, c @ Value::Catchable(_)) => return Ok(c), (Value::Null, Value::Null) => true, (Value::Bool(b1), Value::Bool(b2)) => b1 == b2, (Value::String(s1), Value::String(s2)) => s1 == s2, @@ -526,6 +564,7 @@ impl Value { Value::UnresolvedPath(_) => "internal[unresolved_path]", Value::Json(_) => "internal[json]", Value::FinaliseRequest(_) => "internal[finaliser_sentinel]", + Value::Catchable(_) => "internal[catchable]", } } @@ -533,6 +572,7 @@ impl Value { gen_cast!(as_int, i64, "int", Value::Integer(x), *x); gen_cast!(as_float, f64, "float", Value::Float(x), *x); gen_cast!(to_str, NixString, "string", Value::String(s), s.clone()); + gen_cast!(to_path, Box, "path", Value::Path(p), p.clone()); gen_cast!(to_attrs, Box, "set", Value::Attrs(a), a.clone()); gen_cast!(to_list, NixList, "list", Value::List(l), l.clone()); gen_cast!( @@ -660,6 +700,8 @@ impl Value { // TODO: handle suspended thunks with a different explanation instead of panicking Value::Thunk(t) => t.value().explain(), + Value::Catchable(_) => "a catchable failure".into(), + Value::AttrNotFound | Value::Blueprint(_) | Value::DeferredUpvalue(_) @@ -785,6 +827,7 @@ impl TotalDisplay for Value { // Delegate thunk display to the type, as it must handle // the case of already evaluated or cyclic thunks. Value::Thunk(t) => t.total_fmt(f, set), + Value::Catchable(_) => panic!("total_fmt() called on a CatchableErrorKind"), } } } diff --git a/tvix/eval/src/vm/generators.rs b/tvix/eval/src/vm/generators.rs index a195dac24..f9c5786d8 100644 --- a/tvix/eval/src/vm/generators.rs +++ b/tvix/eval/src/vm/generators.rs @@ -190,10 +190,6 @@ pub enum VMResponse { /// VM response with a span to use at the current point. Span(LightSpan), - - /// Message returned by the VM when a catchable error is encountered during - /// the evaluation of `builtins.tryEval`. - ForceError, } impl Display for VMResponse { @@ -204,7 +200,6 @@ impl Display for VMResponse { VMResponse::Path(p) => write!(f, "path({})", p.to_string_lossy()), VMResponse::Directory(d) => write!(f, "dir(len = {})", d.len()), VMResponse::Span(_) => write!(f, "span"), - VMResponse::ForceError => write!(f, "force_error"), } } } @@ -539,20 +534,18 @@ pub async fn request_force(co: &GenCo, val: Value) -> Value { } } -/// Force a value, but inform the caller (by returning `None`) if a catchable -/// error occured. -pub(crate) async fn request_try_force(co: &GenCo, val: Value) -> Option { +/// Force a value +pub(crate) async fn request_try_force(co: &GenCo, val: Value) -> Value { if let Value::Thunk(_) = val { match co.yield_(VMRequest::TryForce(val)).await { - VMResponse::Value(value) => Some(value), - VMResponse::ForceError => None, + VMResponse::Value(value) => value, msg => panic!( "Tvix bug: VM responded with incorrect generator message: {}", msg ), } } else { - Some(val) + val } } @@ -592,13 +585,18 @@ where callable } -pub async fn request_string_coerce(co: &GenCo, val: Value, kind: CoercionKind) -> NixString { +pub async fn request_string_coerce( + co: &GenCo, + val: Value, + kind: CoercionKind, +) -> Result { match val { - Value::String(s) => s, + Value::String(s) => Ok(s), _ => match co.yield_(VMRequest::StringCoerce(val, kind)).await { - VMResponse::Value(value) => value + VMResponse::Value(Value::Catchable(c)) => Err(c), + VMResponse::Value(value) => Ok(value .to_str() - .expect("coerce_to_string always returns a string"), + .expect("coerce_to_string always returns a string")), msg => panic!( "Tvix bug: VM responded with incorrect generator message: {}", msg diff --git a/tvix/eval/src/vm/mod.rs b/tvix/eval/src/vm/mod.rs index 07d3725fd..d8f38718c 100644 --- a/tvix/eval/src/vm/mod.rs +++ b/tvix/eval/src/vm/mod.rs @@ -84,13 +84,6 @@ impl WithSpan for Result { Err(kind) => { let mut error = Error::new(kind, top_span.get_span()); - // Short-circuit the wrapping if we're dealing with tryEval, in - // which case the error is hidden and does not need to be - // exhaustive. - if !vm.try_eval_frames.is_empty() && error.kind.is_catchable() { - return Err(error); - } - // Wrap the top-level error in chaining errors for each element // of the frame stack. for frame in vm.frames.iter().rev() { @@ -360,8 +353,6 @@ impl<'o> VM<'o> { /// Run the VM's primary (outer) execution loop, continuing execution based /// on the current frame at the top of the frame stack. fn execute(mut self) -> EvalResult { - let mut catchable_error_occurred = false; - while let Some(frame) = self.frames.pop() { self.reasonable_span = frame.span(); let frame_id = self.frames.len(); @@ -377,21 +368,7 @@ impl<'o> VM<'o> { .observer .observe_suspend_call_frame(frame_id, &self.stack), - Err(err) => { - if let Some(catching_frame_idx) = self.try_eval_frames.pop() { - if err.kind.is_catchable() { - self.observer.observe_exit_call_frame(frame_id, &self.stack); - catchable_error_occurred = true; - - // truncate the frame stack back to the - // frame that can catch this error - self.frames.truncate(/* len = */ catching_frame_idx + 1); - continue; - } - } - - return Err(err); - } + Err(err) => return Err(err), }; } @@ -406,14 +383,7 @@ impl<'o> VM<'o> { self.observer .observe_enter_generator(frame_id, name, &self.stack); - let initial_msg = if catchable_error_occurred { - catchable_error_occurred = false; - Some(VMResponse::ForceError) - } else { - None - }; - - match self.run_generator(name, span, frame_id, state, generator, initial_msg) { + match self.run_generator(name, span, frame_id, state, generator, None) { Ok(true) => { self.observer .observe_exit_generator(frame_id, name, &self.stack) @@ -423,25 +393,7 @@ impl<'o> VM<'o> { .observe_suspend_generator(frame_id, name, &self.stack) } - Err(err) => { - if let Some(catching_frame_idx) = self.try_eval_frames.pop() { - if err.kind.is_catchable() { - self.observer.observe_exit_generator( - frame_id, - name, - &self.stack, - ); - catchable_error_occurred = true; - - // truncate the frame stack back to the - // frame that can catch this error - self.frames.truncate(/* len = */ catching_frame_idx + 1); - continue; - } - } - - return Err(err); - } + Err(err) => return Err(err), }; } } @@ -449,12 +401,12 @@ impl<'o> VM<'o> { // Once no more frames are present, return the stack's top value as the // result. + let value = self + .stack + .pop() + .expect("tvix bug: runtime stack empty after execution"); Ok(RuntimeResult { - value: self - .stack - .pop() - .expect("tvix bug: runtime stack empty after execution"), - + value: value, warnings: self.warnings, }) } @@ -925,10 +877,8 @@ impl<'o> VM<'o> { } OpCode::OpAssertFail => { - frame.error( - self, - ErrorKind::CatchableErrorKind(CatchableErrorKind::AssertionFailed), - )?; + self.stack + .push(Value::Catchable(CatchableErrorKind::AssertionFailed)); } // Data-carrying operands should never be executed, @@ -1214,18 +1164,26 @@ async fn add_values(co: GenCo, a: Value, b: Value) -> Result { let result = match (a, b) { (Value::Path(p), v) => { let mut path = p.to_string_lossy().into_owned(); - let vs = generators::request_string_coerce(&co, v, CoercionKind::Weak).await; - path.push_str(vs.as_str()); - crate::value::canon_path(PathBuf::from(path)).into() + match generators::request_string_coerce(&co, v, CoercionKind::Weak).await { + Ok(vs) => { + path.push_str(vs.as_str()); + crate::value::canon_path(PathBuf::from(path)).into() + } + Err(c) => Value::Catchable(c), + } } (Value::String(s1), Value::String(s2)) => Value::String(s1.concat(&s2)), (Value::String(s1), v) => Value::String( - s1.concat(&generators::request_string_coerce(&co, v, CoercionKind::Weak).await), + match generators::request_string_coerce(&co, v, CoercionKind::Weak).await { + Ok(s2) => s1.concat(&s2), + Err(c) => return Ok(Value::Catchable(c)), + }, ), (v, Value::String(s2)) => Value::String( - generators::request_string_coerce(&co, v, CoercionKind::Weak) - .await - .concat(&s2), + match generators::request_string_coerce(&co, v, CoercionKind::Weak).await { + Ok(s1) => s1.concat(&s2), + Err(c) => return Ok(Value::Catchable(c)), + }, ), (a, b) => arithmetic_op!(&a, &b, +)?, }; diff --git a/tvix/serde/src/de.rs b/tvix/serde/src/de.rs index 59a72b741..2e8a9618e 100644 --- a/tvix/serde/src/de.rs +++ b/tvix/serde/src/de.rs @@ -109,6 +109,7 @@ impl<'de> de::Deserializer<'de> for NixDeserializer { | Value::DeferredUpvalue(_) | Value::UnresolvedPath(_) | Value::Json(_) + | Value::Catchable(_) | Value::FinaliseRequest(_) => Err(Error::Unserializable { value_type: self.value.type_of(), }),