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 <tazjin@tvl.su>
Autosubmit: Adam Joseph <adam@westernsemico.com>
Tested-by: BuildkiteCI

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<bool, ErrorKind> {
+) -> Result<Result<bool, CatchableErrorKind>, 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<String, ErrorKind> {
+async fn strong_coerce_to_string(
+    co: &GenCo,
+    val: Value,
+) -> Result<Result<String, CatchableErrorKind>, ErrorKind> {
     let val = generators::request_force(co, val).await;
-    let val_str = generators::request_string_coerce(co, val, CoercionKind::Strong).await;
+    match generators::request_string_coerce(co, val, CoercionKind::Strong).await {
-
+        Err(cek) => Ok(Err(cek)),
-    Ok(val_str.as_str().to_string())
+        Ok(val_str) => Ok(Ok(val_str.as_str().to_string())),
+    }
 }
 
 #[builtins(state = "Rc<RefCell<KnownPaths>>")]
@@ -256,12 +265,15 @@ mod derivation_builtins {
             co: &GenCo,
             attrs: &NixAttrs,
             key: &str,
-        ) -> Result<Option<String>, ErrorKind> {
+        ) -> Result<Result<Option<String>, 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,14 +282,24 @@ 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
-            if !handle_derivation_parameters(&mut drv, &co, name.as_str(), &value, &val_str).await?
+                    match handle_derivation_parameters(
+                        &mut drv,
+                        &co,
+                        name.as_str(),
+                        &value,
+                        &val_str,
+                    )
+                    .await?
                     {
-                continue;
+                        Err(cek) => return Ok(Value::Catchable(cek)),
+                        Ok(false) => continue,
+                        _ => (),
                     }
 
                     // Most of these are also added to the builder's environment in "raw" form.
@@ -289,19 +311,31 @@ mod derivation_builtins {
                         return Err(Error::DuplicateEnvVar(name.as_str().to_string()).into());
                     }
                 }
+            }
+        }
 
-        populate_output_configuration(
+        let output_hash = match select_string(&co, &input, "outputHash")
-            &mut drv,
-            select_string(&co, &input, "outputHash")
             .await
-                .context("evaluating the `outputHash` parameter")?,
+            .context("evaluating the `outputHash` parameter")?
-            select_string(&co, &input, "outputHashAlgo")
+        {
+            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")?,
+            .context("evaluating the `outputHashAlgo` parameter")?
-            select_string(&co, &input, "outputHashMode")
+        {
+            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")?,
+            .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 = {

tvix/eval/src/builtins/impure.rs

@@ -27,14 +27,17 @@ mod impure_builtins {
 
     #[builtin("pathExists")]
     async fn builtin_path_exists(co: GenCo, path: Value) -> Result<Value, ErrorKind> {
-        let path = coerce_value_to_path(&co, path).await?;
+        match coerce_value_to_path(&co, path).await? {
-        Ok(generators::request_path_exists(&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<Value, ErrorKind> {
-        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)| {
                     (
@@ -56,11 +59,15 @@ mod impure_builtins {
 
                 Ok(Value::attrs(NixAttrs::from_iter(res)))
             }
+        }
+    }
 
     #[builtin("readFile")]
     async fn builtin_read_file(co: GenCo, path: Value) -> Result<Value, ErrorKind> {
-        let path = coerce_value_to_path(&co, path).await?;
+        match coerce_value_to_path(&co, path).await? {
-        Ok(generators::request_read_to_string(&co, path).await)
+            Err(cek) => Ok(Value::Catchable(cek)),
+            Ok(path) => Ok(generators::request_read_to_string(&co, path).await),
+        }
     }
 }
 

tvix/eval/src/builtins/mod.rs

@@ -40,20 +40,27 @@ 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<PathBuf, ErrorKind> {
+pub async fn coerce_value_to_path(
+    co: &GenCo,
+    v: Value,
+) -> Result<Result<PathBuf, CatchableErrorKind>, 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;
+    match generators::request_string_coerce(co, value, CoercionKind::Weak).await {
+        Ok(vs) => {
             let path = PathBuf::from(vs.as_str());
             if path.is_absolute() {
-        Ok(path)
+                Ok(Ok(path))
             } else {
                 Err(ErrorKind::NotAnAbsolutePath(path))
             }
         }
+        Err(cek) => Ok(Err(cek)),
+    }
+}
 
 #[builtins]
 mod pure_builtins {
@@ -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;
+            match generators::request_string_coerce(&co, val, CoercionKind::Weak).await {
-            res.push_str(s.as_str());
+                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,10 +352,14 @@ mod pure_builtins {
 
     #[builtin("toJSON")]
     async fn builtin_to_json(co: GenCo, val: Value) -> Result<Value, ErrorKind> {
-        let json_value = val.to_json(&co).await?;
+        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")]
     async fn builtin_from_toml(co: GenCo, toml: Value) -> Result<Value, ErrorKind> {
@@ -893,7 +909,7 @@ mod pure_builtins {
 
     #[builtin("throw")]
     async fn builtin_throw(co: GenCo, message: Value) -> Result<Value, ErrorKind> {
-        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<Value, ErrorKind> {
-        let path: Value = crate::value::canon_path(coerce_value_to_path(&co, s).await?).into();
+        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<Value, ErrorKind> {
         let res = match generators::request_try_force(&co, e).await {
-            Some(value) => [("value", value), ("success", true.into())],
+            Value::Catchable(_) => [("value", false.into()), ("success", false.into())],
-            None => [("value", false.into()), ("success", false.into())],
+            value => [("value", value), ("success", true.into())],
         };
 
         Ok(Value::attrs(NixAttrs::from_iter(res.into_iter())))

tvix/eval/src/builtins/to_xml.rs

@@ -134,6 +134,10 @@ fn value_variant_to_xml<W: Write>(w: &mut EventWriter<W>, 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(())

tvix/eval/src/compiler/import.rs

@@ -25,7 +25,10 @@ async fn import_impl(
     mut args: Vec<Value>,
 ) -> Result<Value, ErrorKind> {
     // 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())
+    let contents: Value = generators::request_read_to_string(&co, path.clone()).await;
-        .await
+    let contents = contents.to_str()?.as_str().to_string();
-        .to_str()?
-        .as_str()
-        .to_string();
 
     let parsed = rnix::ast::Root::parse(&contents);
     let errors = parsed.errors();

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(
+                return self.emit_constant(
-                    node,
+                    Value::Catchable(CatchableErrorKind::NixPathResolution(
-                    ErrorKind::CatchableErrorKind(CatchableErrorKind::NixPathResolution(
                         "Empty <> path not allowed".into(),
                     )),
+                    node,
                 );
             }
             let path = &raw_path[1..(raw_path.len() - 1)];

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<Value,ErrorKind> 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<Result<T,CatchableErrorKind>,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<T,Either<CatchableErrorKind,ErrorKind>>
+///
 #[derive(Clone, Debug)]
 pub enum CatchableErrorKind {
     Throw(String),
@@ -180,14 +201,6 @@ pub enum ErrorKind {
         context: String,
         underlying: Box<ErrorKind>,
     },
-
-    CatchableErrorKind(CatchableErrorKind),
-}
-
-impl From<CatchableErrorKind> for ErrorKind {
-    fn from(c: CatchableErrorKind) -> ErrorKind {
-        ErrorKind::CatchableErrorKind(c)
-    }
 }
 
 impl error::Error for Error {
@@ -243,17 +256,6 @@ impl From<io::Error> 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<serde_json::Error> 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::Abort(_)
-            | ErrorKind::CatchableErrorKind(CatchableErrorKind::AssertionFailed)
             | 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",

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;

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<P>(&self, io: &mut dyn EvalIO, path: P) -> Result<PathBuf, ErrorKind>
+    pub fn resolve<P>(
+        &self,
+        io: &mut dyn EvalIO,
+        path: P,
+    ) -> Result<Result<PathBuf, CatchableErrorKind>, ErrorKind>
     where
         P: AsRef<Path>,
     {
         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(
+        Ok(Err(CatchableErrorKind::NixPathResolution(format!(
-            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!(
+                matches!(err, Err(CatchableErrorKind::NixPathResolution(..))),
-                    err,
-                    ErrorKind::CatchableErrorKind(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());
         }
     }
 }

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();
-
+    let failed = match result.value {
-    if expect_success && !result.errors.is_empty() {
+        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 {

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

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<Json, ErrorKind> {
+    pub(crate) async fn to_json(
+        self,
+        co: &GenCo,
+    ) -> Result<Result<Json, CatchableErrorKind>, 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<Value, ErrorKind> {
-        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)),
+        }
     }
 }

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<CatchableErrorKind> for Value {
+    fn from(c: CatchableErrorKind) -> Value {
+        Value::Catchable(c)
+    }
+}
+
+impl<V> From<Result<V, CatchableErrorKind>> for Value
+where
+    Value: From<V>,
+{
+    fn from(v: Result<V, CatchableErrorKind>) -> 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 match generators::request_string_coerce(&co, out_path.clone(), kind)
-                    return Ok(Value::String(s));
+                        .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;
+                    match generators::request_string_coerce(&co, elem, kind).await {
-                    out.push_str(s.as_str());
+                        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<PathBuf>, "path", Value::Path(p), p.clone());
     gen_cast!(to_attrs, Box<NixAttrs>, "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"),
         }
     }
 }

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
+/// Force a value
-/// error occured.
+pub(crate) async fn request_try_force(co: &GenCo, val: Value) -> Value {
-pub(crate) async fn request_try_force(co: &GenCo, val: Value) -> Option<Value> {
     if let Value::Thunk(_) = val {
         match co.yield_(VMRequest::TryForce(val)).await {
-            VMResponse::Value(value) => Some(value),
+            VMResponse::Value(value) => value,
-            VMResponse::ForceError => None,
             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<NixString, CatchableErrorKind> {
     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

tvix/eval/src/vm/mod.rs

@@ -84,13 +84,6 @@ impl<T, S: GetSpan> WithSpan<T, S> for Result<T, ErrorKind> {
             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<RuntimeResult> {
-        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) => {
+                        Err(err) => return 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);
-                        }
                     };
                 }
 
@@ -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 {
+                    match self.run_generator(name, span, frame_id, state, generator, None) {
-                        catchable_error_occurred = false;
-                        Some(VMResponse::ForceError)
-                    } else {
-                        None
-                    };
-
-                    match self.run_generator(name, span, frame_id, state, generator, initial_msg) {
                         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) => {
+                        Err(err) => return 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);
-                        }
                     };
                 }
             }
@@ -449,12 +401,12 @@ impl<'o> VM<'o> {
 
         // Once no more frames are present, return the stack's top value as the
         // result.
-        Ok(RuntimeResult {
+        let value = self
-            value: self
             .stack
             .pop()
-                .expect("tvix bug: runtime stack empty after execution"),
+            .expect("tvix bug: runtime stack empty after execution");
-
+        Ok(RuntimeResult {
+            value: value,
             warnings: self.warnings,
         })
     }
@@ -925,10 +877,8 @@ impl<'o> VM<'o> {
                 }
 
                 OpCode::OpAssertFail => {
-                    frame.error(
+                    self.stack
-                        self,
+                        .push(Value::Catchable(CatchableErrorKind::AssertionFailed));
-                        ErrorKind::CatchableErrorKind(CatchableErrorKind::AssertionFailed),
-                    )?;
                 }
 
                 // Data-carrying operands should never be executed,
@@ -1214,18 +1164,26 @@ async fn add_values(co: GenCo, a: Value, b: Value) -> Result<Value, ErrorKind> {
     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;
+            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)
+            match generators::request_string_coerce(&co, v, CoercionKind::Weak).await {
-                .await
+                Ok(s1) => s1.concat(&s2),
-                .concat(&s2),
+                Err(c) => return Ok(Value::Catchable(c)),
+            },
         ),
         (a, b) => arithmetic_op!(&a, &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(),
             }),