From 20833656aee4aaefdd83e7beb141a5e03f8c956d Mon Sep 17 00:00:00 2001
From: Vincent Ambo <tazjin@tvl.su>
Date: Tue, 20 Feb 2024 18:09:12 +0700
Subject: fix(tvix/eval): propagate catchable errors at the top of an eval

(Re-)Adds an error variant that wraps a catchable error kind, which is
used for returning the result of an evaluation.

Previously this would return the internal catchable value, which would
lead to panics if users tried to use these. Somehow this was missed; I
think we need error output tests.

Change-Id: Id6e24aa2ce4ea4358a29b2e1cf4a6749986baf8c
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10991
Tested-by: BuildkiteCI
Autosubmit: tazjin <tazjin@tvl.su>
Reviewed-by: flokli <flokli@flokli.de>
---
 tvix/eval/src/errors.rs    | 40 +++++++++++++++++++++++++++++++++++-----
 tvix/eval/src/lib.rs       | 11 ++++++++++-
 tvix/eval/src/value/mod.rs |  1 +
 3 files changed, 46 insertions(+), 6 deletions(-)

(limited to 'tvix/eval/src')

diff --git a/tvix/eval/src/errors.rs b/tvix/eval/src/errors.rs
index 2627570211..db02093b8d 100644
--- a/tvix/eval/src/errors.rs
+++ b/tvix/eval/src/errors.rs
@@ -35,9 +35,7 @@ use crate::{SourceCode, Value};
 /// ... 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>>
-///
+// TODO(amjoseph): investigate result<T,Either<CatchableErrorKind,ErrorKind>>
 #[derive(Clone, Debug)]
 pub enum CatchableErrorKind {
     Throw(Box<str>),
@@ -47,6 +45,21 @@ pub enum CatchableErrorKind {
     NixPathResolution(Box<str>),
 }
 
+impl Display for CatchableErrorKind {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            CatchableErrorKind::Throw(s) => write!(f, "error thrown: {}", s),
+            CatchableErrorKind::AssertionFailed => write!(f, "assertion failed"),
+            CatchableErrorKind::UnimplementedFeature(s) => {
+                write!(f, "feature {} is not implemented yet", s)
+            }
+            CatchableErrorKind::NixPathResolution(s) => {
+                write!(f, "Nix path entry could not be resolved: {}", s)
+            }
+        }
+    }
+}
+
 #[derive(Clone, Debug)]
 pub enum ErrorKind {
     /// These are user-generated errors through builtins.
@@ -208,6 +221,14 @@ pub enum ErrorKind {
 
     /// Unexpected context string
     UnexpectedContext,
+
+    /// Top-level evaluation result was a catchable Nix error, and
+    /// should fail the evaluation.
+    ///
+    /// This variant **must** only be used at the top-level of
+    /// tvix-eval when returning a result to the user, never inside of
+    /// eval code.
+    CatchableError(CatchableErrorKind),
 }
 
 impl error::Error for Error {
@@ -508,6 +529,10 @@ to a missing value in the attribute set(s) included via `with`."#,
             ErrorKind::UnexpectedContext => {
                 write!(f, "unexpected context string")
             }
+
+            ErrorKind::CatchableError(inner) => {
+                write!(f, "{}", inner)
+            }
         }
     }
 }
@@ -795,7 +820,8 @@ impl Error {
             | ErrorKind::TvixError(_)
             | ErrorKind::TvixBug { .. }
             | ErrorKind::NotImplemented(_)
-            | ErrorKind::WithContext { .. } => return None,
+            | ErrorKind::WithContext { .. }
+            | ErrorKind::CatchableError(_) => return None,
         };
 
         Some(label.into())
@@ -805,11 +831,14 @@ impl Error {
     /// used to refer users to documentation.
     fn code(&self) -> &'static str {
         match self.kind {
+            ErrorKind::CatchableError(CatchableErrorKind::Throw(_)) => "E001",
             ErrorKind::Abort(_) => "E002",
+            ErrorKind::CatchableError(CatchableErrorKind::AssertionFailed) => "E003",
             ErrorKind::InvalidAttributeName { .. } => "E004",
             ErrorKind::AttributeNotFound { .. } => "E005",
             ErrorKind::TypeError { .. } => "E006",
             ErrorKind::Incomparable { .. } => "E007",
+            ErrorKind::CatchableError(CatchableErrorKind::NixPathResolution(_)) => "E008",
             ErrorKind::DynamicKeyInScope(_) => "E009",
             ErrorKind::UnknownStaticVariable => "E010",
             ErrorKind::UnknownDynamicVariable(_) => "E011",
@@ -848,7 +877,8 @@ impl Error {
             ErrorKind::TvixBug { .. } => "E998",
 
             // Placeholder error while Tvix is under construction.
-            ErrorKind::NotImplemented(_) => "E999",
+            ErrorKind::CatchableError(CatchableErrorKind::UnimplementedFeature(_))
+            | ErrorKind::NotImplemented(_) => "E999",
 
             // Chained errors should yield the code of the innermost
             // error.
diff --git a/tvix/eval/src/lib.rs b/tvix/eval/src/lib.rs
index 8ce37de427..845964cb7e 100644
--- a/tvix/eval/src/lib.rs
+++ b/tvix/eval/src/lib.rs
@@ -301,7 +301,7 @@ where
             nix_path,
             self.io_handle,
             runtime_observer,
-            source,
+            source.clone(),
             globals,
             lambda,
             self.strict,
@@ -310,6 +310,15 @@ where
         match vm_result {
             Ok(mut runtime_result) => {
                 result.warnings.append(&mut runtime_result.warnings);
+                if let Value::Catchable(inner) = runtime_result.value {
+                    result.errors.push(Error::new(
+                        ErrorKind::CatchableError(*inner),
+                        file.span,
+                        source,
+                    ));
+                    return result;
+                }
+
                 result.value = Some(runtime_result.value);
             }
             Err(err) => {
diff --git a/tvix/eval/src/value/mod.rs b/tvix/eval/src/value/mod.rs
index 165bdac597..095bc269d4 100644
--- a/tvix/eval/src/value/mod.rs
+++ b/tvix/eval/src/value/mod.rs
@@ -84,6 +84,7 @@ pub enum Value {
     FinaliseRequest(bool),
 
     #[serde(skip)]
+    // TODO(tazjin): why is this in a Box?
     Catchable(Box<CatchableErrorKind>),
 }
 
-- 
cgit 1.4.1