feat(tvix/eval): Use thiserror for ErrorKind and CatchableErrorKind r/8713

thiserror is much more easier to maintain than manually implementing Error and Display.

Change-Id: Ibf13e2d8a96fba69c8acb362b7515274a593dfd6
Reviewed-on: https://cl.tvl.fyi/c/depot/+/12452
Reviewed-by: flokli <flokli@flokli.de>
Autosubmit: Ilan Joselevich <personal@ilanjoselevich.com>
Tested-by: BuildkiteCI

6 files changed, 113 insertions, 250 deletions

diff --git a/tvix/Cargo.lock b/tvix/Cargo.lock
index 7f92e885c3d1..96562e9f2f7a 100644
--- a/tvix/Cargo.lock
+++ b/tvix/Cargo.lock
@@ -4612,6 +4612,7 @@ dependencies = [
  "tabwriter",
  "tempfile",
  "test-strategy",
+ "thiserror",
  "toml 0.6.0",
  "tvix-eval-builtin-macros",
  "vu128",
diff --git a/tvix/Cargo.nix b/tvix/Cargo.nix
index 5671624ceceb..2cc3aa64da37 100644
--- a/tvix/Cargo.nix
+++ b/tvix/Cargo.nix
@@ -15231,6 +15231,10 @@ rec {
             optional = true;
           }
           {
+            name = "thiserror";
+            packageId = "thiserror";
+          }
+          {
             name = "toml";
             packageId = "toml 0.6.0";
           }
diff --git a/tvix/eval/Cargo.toml b/tvix/eval/Cargo.toml
index c99bea4f7125..29862f97d8e4 100644
--- a/tvix/eval/Cargo.toml
+++ b/tvix/eval/Cargo.toml
@@ -36,6 +36,7 @@ data-encoding = { workspace = true }
 rustc-hash = { workspace = true }
 nohash-hasher = { workspace = true }
 vu128 = { workspace = true }
+thiserror.workspace = true
 
 [dev-dependencies]
 criterion = { workspace = true }
diff --git a/tvix/eval/src/errors.rs b/tvix/eval/src/errors.rs
index ee55552c7ac5..9c3383fc5d94 100644
--- a/tvix/eval/src/errors.rs
+++ b/tvix/eval/src/errors.rs
@@ -35,96 +35,109 @@ use crate::{SourceCode, Value};
 /// 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)]
+#[derive(thiserror::Error, Clone, Debug)]
 pub enum CatchableErrorKind {
+    #[error("error thrown: {0}")]
     Throw(Box<str>),
+
+    #[error("assertion failed")]
     AssertionFailed,
+
+    #[error("feature {0} is not implemented yet")]
     UnimplementedFeature(Box<str>),
+
     /// Resolving a user-supplied angle brackets path literal failed in some way.
+    #[error("Nix path entry could not be resolved: {0}")]
     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)]
+#[derive(thiserror::Error, Clone, Debug)]
 pub enum ErrorKind {
     /// These are user-generated errors through builtins.
+    #[error("evaluation aborted: {0}")]
     Abort(String),
 
+    #[error("division by zero")]
     DivisionByZero,
 
-    DuplicateAttrsKey {
-        key: String,
-    },
+    #[error("attribute key '{key}' already defined")]
+    DuplicateAttrsKey { key: String },
 
     /// Attempted to specify an invalid key type (e.g. integer) in a
     /// dynamic attribute name.
+    #[error(
+        "found attribute name '{0}' of type '{}', but attribute names must be strings",
+        .0.type_of()
+    )]
     InvalidAttributeName(Value),
 
-    AttributeNotFound {
-        name: String,
-    },
+    #[error("attribute with name '{name}' could not be found in the set")]
+    AttributeNotFound { name: String },
 
     /// Attempted to index into a list beyond its boundaries.
-    IndexOutOfBounds {
-        index: i64,
-    },
+    #[error("list index '{index}' is out of bounds")]
+    IndexOutOfBounds { index: i64 },
 
     /// Attempted to call `builtins.tail` on an empty list.
+    #[error("'tail' called on an empty list")]
     TailEmptyList,
 
+    #[error("expected value of type '{expected}', but found a '{actual}'")]
     TypeError {
         expected: &'static str,
         actual: &'static str,
     },
 
+    #[error("can not compare a {lhs} with a {rhs}")]
     Incomparable {
         lhs: &'static str,
         rhs: &'static str,
     },
 
     /// Resolving a user-supplied relative or home-relative path literal failed in some way.
+    #[error("could not resolve path: {0}")]
     RelativePathResolution(String),
 
     /// Dynamic keys are not allowed in some scopes.
+    #[error("dynamically evaluated keys are not allowed in {0}")]
     DynamicKeyInScope(&'static str),
 
     /// Unknown variable in statically known scope.
+    #[error("variable not found")]
     UnknownStaticVariable,
 
     /// Unknown variable in dynamic scope (with, rec, ...).
+    #[error(
+        r#"variable '{0}' could not be found
+
+Note that this occured within a `with`-expression. The problem may be related
+to a missing value in the attribute set(s) included via `with`."#
+    )]
     UnknownDynamicVariable(String),
 
     /// User is defining the same variable twice at the same depth.
+    #[error("variable has already been defined")]
     VariableAlreadyDefined(Option<Span>),
 
     /// Attempt to call something that is not callable.
+    #[error("only functions and builtins can be called, but this is a '{0}'")]
     NotCallable(&'static str),
 
     /// Infinite recursion encountered while forcing thunks.
+    #[error("infinite recursion encountered")]
     InfiniteRecursion {
         first_force: Span,
         suspended_at: Option<Span>,
         content_span: Option<Span>,
     },
 
+    // Errors themselves ignored here & handled in Self::spans instead
+    #[error("failed to parse Nix code:")]
     ParseErrors(Vec<rnix::parser::ParseError>),
 
     /// An error occured while executing some native code (e.g. a
     /// builtin), and needs to be chained up.
+    #[error("while evaluating this as native code ({gen_type})")]
     NativeError {
         gen_type: &'static str,
         err: Box<Error>,
@@ -132,31 +145,44 @@ pub enum ErrorKind {
 
     /// An error occured while executing Tvix bytecode, but needs to
     /// be chained up.
+    #[error("while evaluating this Nix code")]
     BytecodeError(Box<Error>),
 
     /// Given type can't be coerced to a string in the respective context
+    #[error("cannot ({}) coerce {from} to a string{}", 
+        (if .kind.strong { "strongly" } else { "weakly" }),
+        (if *.from == "set" {
+            ", missing a `__toString` or `outPath` attribute"
+        } else {
+            ""
+        })
+    )]
     NotCoercibleToString {
         from: &'static str,
         kind: CoercionKind,
     },
 
     /// The given string doesn't represent an absolute path
+    #[error("string '{}' does not represent an absolute path", .0.to_string_lossy())]
     NotAnAbsolutePath(PathBuf),
 
     /// An error occurred when parsing an integer
+    #[error("invalid integer: {0}")]
     ParseIntError(ParseIntError),
 
     // Errors specific to nested attribute sets and merges thereof.
     /// Nested attributes can not be merged with an inherited value.
-    UnmergeableInherit {
-        name: SmolStr,
-    },
+    #[error("cannot merge a nested attribute set into the inherited entry '{name}'")]
+    UnmergeableInherit { name: SmolStr },
 
     /// Nested attributes can not be merged with values that are not
     /// literal attribute sets.
+    #[error("nested attribute sets or keys can only be merged with literal attribute sets")]
     UnmergeableValue,
 
+    // Errors themselves ignored here & handled in Self::spans instead
     /// Parse errors occured while importing a file.
+    #[error("parse errors occured while importing '{}'", .path.to_string_lossy())]
     ImportParseError {
         path: PathBuf,
         file: Arc<File>,
@@ -164,44 +190,70 @@ pub enum ErrorKind {
     },
 
     /// Compilation errors occured while importing a file.
-    ImportCompilerError {
-        path: PathBuf,
-        errors: Vec<Error>,
-    },
+    #[error("compiler errors occured while importing '{}'", .path.to_string_lossy())]
+    ImportCompilerError { path: PathBuf, errors: Vec<Error> },
 
     /// I/O errors
+    #[error("I/O error: {}",
+        ({
+            let mut msg = String::new();
+
+            if let Some(path) = .path {
+                msg.push_str(&format!("{}: ", path.display()));
+            }
+
+            msg.push_str(&.error.to_string());
+
+            msg
+        })
+    )]
     IO {
         path: Option<PathBuf>,
         error: Rc<io::Error>,
     },
 
     /// Errors parsing JSON, or serializing as JSON.
+    #[error("Error converting JSON to a Nix value or back: {0}")]
     JsonError(String),
 
     /// Nix value that can not be serialised to JSON.
+    #[error("a {0} cannot be converted to JSON")]
     NotSerialisableToJson(&'static str),
 
     /// Errors converting TOML to a value
+    #[error("Error converting TOML to a Nix value: {0}")]
     FromTomlError(String),
 
     /// An unexpected argument was supplied to a builtin
+    #[error("Unexpected agrument `{0}` passed to builtin")]
     UnexpectedArgumentBuiltin(NixString),
 
     /// An unexpected argument was supplied to a function that takes formal parameters
-    UnexpectedArgumentFormals {
-        arg: NixString,
-        formals_span: Span,
-    },
+    #[error("Unexpected argument `{arg}` supplied to function")]
+    UnexpectedArgumentFormals { arg: NixString, formals_span: Span },
 
     /// Invalid UTF-8 was encoutered somewhere
+    #[error("Invalid UTF-8 in string")]
     Utf8,
 
     /// Variant for errors that bubble up to eval from other Tvix
     /// components.
+    #[error("{0}")]
     TvixError(Rc<dyn error::Error>),
 
     /// Variant for code paths that are known bugs in Tvix (usually
     /// issues with the compiler/VM interaction).
+    #[error("{}",
+        ({
+            let mut disp = format!("Tvix bug: {}", .msg);
+
+            if let Some(metadata) = .metadata {
+                disp.push_str(&format!("; metadata: {:?}", metadata));
+            }
+
+            disp
+        })
+    )]
     TvixBug {
         msg: &'static str,
         metadata: Option<Rc<dyn Debug>>,
@@ -210,15 +262,18 @@ pub enum ErrorKind {
     /// Tvix internal warning for features triggered by users that are
     /// not actually implemented yet, and without which eval can not
     /// proceed.
+    #[error("feature not yet implemented in Tvix: {0}")]
     NotImplemented(&'static str),
 
     /// Internal variant which should disappear during error construction.
+    #[error("internal ErrorKind::WithContext variant leaked")]
     WithContext {
         context: String,
         underlying: Box<ErrorKind>,
     },
 
     /// Unexpected context string
+    #[error("unexpected context string")]
     UnexpectedContext,
 
     /// Top-level evaluation result was a catchable Nix error, and
@@ -227,10 +282,12 @@ pub enum ErrorKind {
     /// 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.
+    #[error("{0}")]
     CatchableError(CatchableErrorKind),
 
     /// Invalid hash type specified, must be one of "md5", "sha1", "sha256"
     /// or "sha512"
+    #[error("unknown hash type '{0}'")]
     UnknownHashType(String),
 }
 
@@ -334,211 +391,6 @@ impl Error {
     }
 }
 
-impl Display for ErrorKind {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        match &self {
-            ErrorKind::Abort(msg) => write!(f, "evaluation aborted: {}", msg),
-
-            ErrorKind::DivisionByZero => write!(f, "division by zero"),
-
-            ErrorKind::DuplicateAttrsKey { key } => {
-                write!(f, "attribute key '{}' already defined", key)
-            }
-
-            ErrorKind::InvalidAttributeName(val) => write!(
-                f,
-                "found attribute name '{}' of type '{}', but attribute names must be strings",
-                val,
-                val.type_of()
-            ),
-
-            ErrorKind::AttributeNotFound { name } => write!(
-                f,
-                "attribute with name '{}' could not be found in the set",
-                name
-            ),
-
-            ErrorKind::IndexOutOfBounds { index } => {
-                write!(f, "list index '{}' is out of bounds", index)
-            }
-
-            ErrorKind::TailEmptyList => write!(f, "'tail' called on an empty list"),
-
-            ErrorKind::TypeError { expected, actual } => write!(
-                f,
-                "expected value of type '{}', but found a '{}'",
-                expected, actual
-            ),
-
-            ErrorKind::Incomparable { lhs, rhs } => {
-                write!(f, "can not compare a {} with a {}", lhs, rhs)
-            }
-
-            ErrorKind::RelativePathResolution(err) => {
-                write!(f, "could not resolve path: {}", err)
-            }
-
-            ErrorKind::DynamicKeyInScope(scope) => {
-                write!(f, "dynamically evaluated keys are not allowed in {}", scope)
-            }
-
-            ErrorKind::UnknownStaticVariable => write!(f, "variable not found"),
-
-            ErrorKind::UnknownDynamicVariable(name) => write!(
-                f,
-                r#"variable '{}' could not be found
-
-Note that this occured within a `with`-expression. The problem may be related
-to a missing value in the attribute set(s) included via `with`."#,
-                name
-            ),
-
-            ErrorKind::VariableAlreadyDefined(_) => write!(f, "variable has already been defined"),
-
-            ErrorKind::NotCallable(other_type) => {
-                write!(
-                    f,
-                    "only functions and builtins can be called, but this is a '{}'",
-                    other_type
-                )
-            }
-
-            ErrorKind::InfiniteRecursion { .. } => write!(f, "infinite recursion encountered"),
-
-            // Errors themselves ignored here & handled in Self::spans instead
-            ErrorKind::ParseErrors(_) => write!(f, "failed to parse Nix code:"),
-
-            ErrorKind::NativeError { gen_type, .. } => {
-                write!(f, "while evaluating this as native code ({})", gen_type)
-            }
-
-            ErrorKind::BytecodeError(_) => write!(f, "while evaluating this Nix code"),
-
-            ErrorKind::NotCoercibleToString { kind, from } => {
-                let kindly = if kind.strong { "strongly" } else { "weakly" };
-
-                let hint = if *from == "set" {
-                    ", missing a `__toString` or `outPath` attribute"
-                } else {
-                    ""
-                };
-
-                write!(f, "cannot ({kindly}) coerce {from} to a string{hint}")
-            }
-
-            ErrorKind::NotAnAbsolutePath(given) => {
-                write!(
-                    f,
-                    "string '{}' does not represent an absolute path",
-                    given.to_string_lossy()
-                )
-            }
-
-            ErrorKind::ParseIntError(err) => {
-                write!(f, "invalid integer: {}", err)
-            }
-
-            ErrorKind::UnmergeableInherit { name } => {
-                write!(
-                    f,
-                    "cannot merge a nested attribute set into the inherited entry '{}'",
-                    name
-                )
-            }
-
-            ErrorKind::UnmergeableValue => {
-                write!(
-                    f,
-                    "nested attribute sets or keys can only be merged with literal attribute sets"
-                )
-            }
-
-            // Errors themselves ignored here & handled in Self::spans instead
-            ErrorKind::ImportParseError { path, .. } => {
-                write!(
-                    f,
-                    "parse errors occured while importing '{}'",
-                    path.to_string_lossy()
-                )
-            }
-
-            ErrorKind::ImportCompilerError { path, .. } => {
-                writeln!(
-                    f,
-                    "compiler errors occured while importing '{}'",
-                    path.to_string_lossy()
-                )
-            }
-
-            ErrorKind::IO { path, error } => {
-                write!(f, "I/O error: ")?;
-                if let Some(path) = path {
-                    write!(f, "{}: ", path.display())?;
-                }
-                write!(f, "{error}")
-            }
-
-            ErrorKind::JsonError(msg) => {
-                write!(f, "Error converting JSON to a Nix value or back: {msg}")
-            }
-
-            ErrorKind::NotSerialisableToJson(_type) => {
-                write!(f, "a {} cannot be converted to JSON", _type)
-            }
-
-            ErrorKind::FromTomlError(msg) => {
-                write!(f, "Error converting TOML to a Nix value: {msg}")
-            }
-
-            ErrorKind::UnexpectedArgumentBuiltin(arg) => {
-                write!(f, "Unexpected agrument `{arg}` passed to builtin",)
-            }
-
-            ErrorKind::UnexpectedArgumentFormals { arg, .. } => {
-                write!(f, "Unexpected argument `{arg}` supplied to function",)
-            }
-
-            ErrorKind::Utf8 => {
-                write!(f, "Invalid UTF-8 in string")
-            }
-
-            ErrorKind::TvixError(inner_error) => {
-                write!(f, "{inner_error}")
-            }
-
-            ErrorKind::TvixBug { msg, metadata } => {
-                write!(f, "Tvix bug: {}", msg)?;
-
-                if let Some(metadata) = metadata {
-                    write!(f, "; metadata: {:?}", metadata)?;
-                }
-
-                Ok(())
-            }
-
-            ErrorKind::NotImplemented(feature) => {
-                write!(f, "feature not yet implemented in Tvix: {}", feature)
-            }
-
-            ErrorKind::WithContext { .. } => {
-                panic!("internal ErrorKind::WithContext variant leaked")
-            }
-
-            ErrorKind::UnexpectedContext => {
-                write!(f, "unexpected context string")
-            }
-
-            ErrorKind::CatchableError(inner) => {
-                write!(f, "{}", inner)
-            }
-
-            ErrorKind::UnknownHashType(hash_type) => {
-                write!(f, "unknown hash type '{}'", hash_type)
-            }
-        }
-    }
-}
-
 impl Display for Error {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         write!(f, "{}", self.kind)
diff --git a/web/tvixbolt/Cargo.lock b/web/tvixbolt/Cargo.lock
index f67dbb9f9846..301847797107 100644
--- a/web/tvixbolt/Cargo.lock
+++ b/web/tvixbolt/Cargo.lock
@@ -1387,18 +1387,18 @@ checksum = "f18aa187839b2bdb1ad2fa35ead8c4c2976b64e4363c386d45ac0f7ee85c9233"
 
 [[package]]
 name = "thiserror"
-version = "1.0.61"
+version = "1.0.63"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "c546c80d6be4bc6a00c0f01730c08df82eaa7a7a61f11d656526506112cc1709"
+checksum = "c0342370b38b6a11b6cc11d6a805569958d54cfa061a29969c3b5ce2ea405724"
 dependencies = [
  "thiserror-impl",
 ]
 
 [[package]]
 name = "thiserror-impl"
-version = "1.0.61"
+version = "1.0.63"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "46c3384250002a6d5af4d114f2845d37b57521033f30d5c3f46c4d70e1197533"
+checksum = "a4558b58466b9ad7ca0f102865eccc95938dca1a74a856f2b57b6629050da261"
 dependencies = [
  "proc-macro2",
  "quote",
@@ -1536,6 +1536,7 @@ dependencies = [
  "sha2",
  "smol_str",
  "tabwriter",
+ "thiserror",
  "toml",
  "tvix-eval-builtin-macros",
  "vu128",
diff --git a/web/tvixbolt/Cargo.nix b/web/tvixbolt/Cargo.nix
index ceec2d582cca..eefa2b28b478 100644
--- a/web/tvixbolt/Cargo.nix
+++ b/web/tvixbolt/Cargo.nix
@@ -4168,9 +4168,9 @@ rec {
       };
       "thiserror" = rec {
         crateName = "thiserror";
-        version = "1.0.61";
+        version = "1.0.63";
         edition = "2021";
-        sha256 = "028prh962l16cmjivwb1g9xalbpqip0305zhq006mg74dc6whin5";
+        sha256 = "092p83mf4p1vkjb2j6h6z96dan4raq2simhirjv12slbndq26d60";
         authors = [
           "David Tolnay <dtolnay@gmail.com>"
         ];
@@ -4184,9 +4184,9 @@ rec {
       };
       "thiserror-impl" = rec {
         crateName = "thiserror-impl";
-        version = "1.0.61";
+        version = "1.0.63";
         edition = "2021";
-        sha256 = "0cvm37hp0kbcyk1xac1z0chpbd9pbn2g456iyid6sah0a113ihs6";
+        sha256 = "0qd21l2jjrkvnpr5da3l3b58v4wmrkn6aa0h1z5dg6kb8rc8nmd4";
         procMacro = true;
         libName = "thiserror_impl";
         authors = [
@@ -4640,6 +4640,10 @@ rec {
             packageId = "tabwriter";
           }
           {
+            name = "thiserror";
+            packageId = "thiserror";
+          }
+          {
             name = "toml";
             packageId = "toml";
           }