From 4aa1137d8e3d16a64ad1158fcb94dca65bf6332b Mon Sep 17 00:00:00 2001 From: Axel Karjalainen Date: Fri, 1 Aug 2025 18:01:23 +0300 Subject: [PATCH] refactor(snix/eval,snix/glue): add snix_eval::try_cek! macros Fixes #146 Change-Id: I971fac0d9d18e4ea73a527e499ac7ac213658477 Reviewed-on: https://cl.snix.dev/c/snix/+/30638 Reviewed-by: Florian Klink Autosubmit: Axel Karjalainen Tested-by: besadii --- snix/eval/src/builtins/impure.rs | 74 ++++++------- snix/eval/src/builtins/mod.rs | 151 ++++++++++++--------------- snix/eval/src/compiler/import.rs | 6 +- snix/eval/src/errors.rs | 75 +++++++++++++ snix/glue/src/builtins/derivation.rs | 95 +++++++---------- snix/glue/src/builtins/fetchers.rs | 29 ++--- snix/glue/src/builtins/utils.rs | 7 +- 7 files changed, 227 insertions(+), 210 deletions(-) diff --git a/snix/eval/src/builtins/impure.rs b/snix/eval/src/builtins/impure.rs index 0cac27dfc..8386066ed 100644 --- a/snix/eval/src/builtins/impure.rs +++ b/snix/eval/src/builtins/impure.rs @@ -16,7 +16,10 @@ mod impure_builtins { use std::os::unix::ffi::OsStrExt; use super::*; - use crate::builtins::{coerce_value_to_path, hash::hash_nix_string}; + use crate::{ + builtins::{coerce_value_to_path, hash::hash_nix_string}, + try_cek_to_value, + }; #[builtin("getEnv")] async fn builtin_get_env(co: GenCo, var: Value) -> Result { @@ -28,67 +31,52 @@ mod impure_builtins { #[builtin("hashFile")] async fn builtin_hash_file(co: GenCo, algo: Value, path: Value) -> Result { - let path = match coerce_value_to_path(&co, path).await? { - Err(cek) => return Ok(Value::from(cek)), - Ok(p) => p, - }; + let path = try_cek_to_value!(coerce_value_to_path(&co, path).await?); let r = generators::request_open_file(&co, path).await; hash_nix_string(algo.to_str()?, r).map(Value::from) } #[builtin("pathExists")] async fn builtin_path_exists(co: GenCo, path: Value) -> Result { - match coerce_value_to_path(&co, path).await? { - Err(cek) => Ok(Value::from(cek)), - Ok(path) => Ok(generators::request_path_exists(&co, path).await), - } + let path = try_cek_to_value!(coerce_value_to_path(&co, path).await?); + Ok(generators::request_path_exists(&co, path).await) } #[builtin("readDir")] async fn builtin_read_dir(co: GenCo, path: Value) -> Result { - match coerce_value_to_path(&co, path).await? { - Err(cek) => Ok(Value::from(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::from(ftype.to_string()), - ) - }); + let path = try_cek_to_value!(coerce_value_to_path(&co, path).await?); + 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::from(ftype.to_string()), + ) + }); - 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 { - match coerce_value_to_path(&co, path).await? { - Err(cek) => Ok(Value::from(cek)), - Ok(path) => { - let mut buf = Vec::new(); - generators::request_open_file(&co, path) - .await - .read_to_end(&mut buf)?; - Ok(Value::from(buf)) - } - } + let path = try_cek_to_value!(coerce_value_to_path(&co, path).await?); + let mut buf = Vec::new(); + generators::request_open_file(&co, path) + .await + .read_to_end(&mut buf)?; + Ok(Value::from(buf)) } #[builtin("readFileType")] async fn builtin_read_file_type(co: GenCo, path: Value) -> Result { - match coerce_value_to_path(&co, path).await? { - Err(cek) => Ok(Value::from(cek)), - Ok(path) => Ok(Value::from( - generators::request_read_file_type(&co, path) - .await - .to_string(), - )), - } + let path = try_cek_to_value!(coerce_value_to_path(&co, path).await?); + Ok(Value::from( + generators::request_read_file_type(&co, path) + .await + .to_string(), + )) } } diff --git a/snix/eval/src/builtins/mod.rs b/snix/eval/src/builtins/mod.rs index 552c544b2..6633131f5 100644 --- a/snix/eval/src/builtins/mod.rs +++ b/snix/eval/src/builtins/mod.rs @@ -14,7 +14,6 @@ use std::collections::VecDeque; use std::path::PathBuf; use std::sync::{Mutex, OnceLock}; -use crate::arithmetic_op; use crate::value::PointerEquality; use crate::vm::generators::{self, GenCo}; use crate::warnings::WarningKind; @@ -24,6 +23,7 @@ use crate::{ errors::{CatchableErrorKind, ErrorKind}, value::{CoercionKind, NixAttrs, NixList, NixString, Thunk, Value}, }; +use crate::{arithmetic_op, try_cek}; use self::versions::{VersionPart, VersionPartsIter}; @@ -59,25 +59,23 @@ pub async fn coerce_value_to_path( return Ok(Ok(*p)); } - match generators::request_string_coerce( - co, - value, - CoercionKind { - strong: false, - import_paths: false, - }, - ) - .await - { - Ok(vs) => { - let path = vs.to_path()?.to_owned(); - if path.is_absolute() { - Ok(Ok(path)) - } else { - Err(ErrorKind::NotAnAbsolutePath(path)) - } - } - Err(cek) => Ok(Err(cek)), + let vs = try_cek!( + generators::request_string_coerce( + co, + value, + CoercionKind { + strong: false, + import_paths: false, + }, + ) + .await + ); + + let path = vs.to_path()?.to_owned(); + if path.is_absolute() { + Ok(Ok(path)) + } else { + Err(ErrorKind::NotAnAbsolutePath(path)) } } @@ -106,7 +104,9 @@ mod pure_builtins { use os_str_bytes::OsStringBytes; use rustc_hash::{FxHashMap, FxHashSet}; - use crate::{AddContext, NixContext, NixContextElement, value::PointerEquality}; + use crate::{ + AddContext, NixContext, NixContextElement, try_cek_to_value, value::PointerEquality, + }; use super::*; @@ -302,23 +302,20 @@ mod pure_builtins { if i != 0 { res.push_str(&separator); } - match generators::request_string_coerce( - &co, - val, - CoercionKind { - strong: false, - import_paths: true, - }, - ) - .await - { - Ok(mut s) => { - res.push_str(&s); - if let Some(other_context) = s.take_context() { - context.extend(other_context.into_iter()); - } - } - Err(c) => return Ok(Value::Catchable(Box::new(c))), + let mut s = try_cek_to_value!( + generators::request_string_coerce( + &co, + val, + CoercionKind { + strong: false, + import_paths: true, + }, + ) + .await + ); + res.push_str(&s); + if let Some(other_context) = s.take_context() { + context.extend(other_context.into_iter()); } } // FIXME: pass immediately the string res. @@ -372,11 +369,11 @@ mod pure_builtins { #[builtin("elem")] async fn builtin_elem(co: GenCo, x: Value, xs: Value) -> Result { for val in xs.to_list()? { - match generators::check_equality(&co, x.clone(), val, PointerEquality::AllowAll).await? - { - Ok(true) => return Ok(true.into()), - Ok(false) => continue, - Err(cek) => return Ok(Value::from(cek)), + match try_cek_to_value!( + generators::check_equality(&co, x.clone(), val, PointerEquality::AllowAll).await? + ) { + true => return Ok(true.into()), + false => continue, } } Ok(false.into()) @@ -504,13 +501,10 @@ mod pure_builtins { let attrs = val.to_attrs()?; let key = attrs.select_required("key")?; - let value_missing = bgc_insert_key(&co, key.clone(), &mut done_keys).await?; + let value_missing = + try_cek_to_value!(bgc_insert_key(&co, key.clone(), &mut done_keys).await?); - if let Err(cek) = value_missing { - return Ok(Value::Catchable(Box::new(cek))); - } - - if let Ok(false) = value_missing { + if !value_missing { continue; } @@ -909,10 +903,9 @@ mod pure_builtins { #[builtin("lessThan")] async fn builtin_less_than(co: GenCo, x: Value, y: Value) -> Result { let span = generators::request_span(&co).await; - match x.nix_cmp_ordering(y, co, span).await? { - Err(cek) => Ok(Value::from(cek)), - Ok(Ordering::Less) => Ok(Value::Bool(true)), - Ok(_) => Ok(Value::Bool(false)), + match try_cek_to_value!(x.nix_cmp_ordering(y, co, span).await?) { + Ordering::Less => Ok(Value::Bool(true)), + _ => Ok(Value::Bool(false)), } } @@ -1474,23 +1467,18 @@ mod pure_builtins { return Ok(s); } - match coerce_value_to_path(&co, s).await? { - Err(cek) => Ok(Value::from(cek)), - Ok(path) => { - let path: Value = crate::value::canon_path(path).into(); - let span = generators::request_span(&co).await; - Ok(path - .coerce_to_string( - co, - CoercionKind { - strong: false, - import_paths: false, - }, - span, - ) - .await?) - } - } + let path = try_cek_to_value!(coerce_value_to_path(&co, s).await?); + let path: Value = crate::value::canon_path(path).into(); + let span = generators::request_span(&co).await; + path.coerce_to_string( + co, + CoercionKind { + strong: false, + import_paths: false, + }, + span, + ) + .await } #[builtin("tryEval")] @@ -1521,18 +1509,17 @@ async fn bgc_insert_key( done: &mut Vec, ) -> Result, ErrorKind> { for existing in done.iter() { - match generators::check_equality( - co, - existing.clone(), - key.clone(), - // TODO(tazjin): not actually sure which semantics apply here - PointerEquality::ForbidAll, - ) - .await? - { - Ok(true) => return Ok(Ok(false)), - Ok(false) => (), - Err(cek) => return Ok(Err(cek)), + if try_cek!( + generators::check_equality( + co, + existing.clone(), + key.clone(), + // TODO(tazjin): not actually sure which semantics apply here + PointerEquality::ForbidAll, + ) + .await? + ) { + return Ok(Ok(false)); } } diff --git a/snix/eval/src/compiler/import.rs b/snix/eval/src/compiler/import.rs index f76fa094b..abda62d82 100644 --- a/snix/eval/src/compiler/import.rs +++ b/snix/eval/src/compiler/import.rs @@ -14,6 +14,7 @@ use crate::{ builtins::coerce_value_to_path, generators::pin_generator, observer::NoOpObserver, + try_cek_to_value, value::{Builtin, Thunk}, vm::generators::{self, GenCo}, }; @@ -25,10 +26,7 @@ async fn import_impl( mut args: Vec, ) -> Result { // TODO(sterni): canon_path()? - let mut path = match coerce_value_to_path(&co, args.pop().unwrap()).await? { - Err(cek) => return Ok(Value::Catchable(Box::new(cek))), - Ok(path) => path, - }; + let mut path = try_cek_to_value!(coerce_value_to_path(&co, args.pop().unwrap()).await?); if path.is_dir() { path.push("default.nix"); diff --git a/snix/eval/src/errors.rs b/snix/eval/src/errors.rs index 4f3c40b4f..6e2b6658b 100644 --- a/snix/eval/src/errors.rs +++ b/snix/eval/src/errors.rs @@ -16,6 +16,81 @@ use crate::spans::ToSpan; use crate::value::{CoercionKind, NixString}; use crate::{SourceCode, Value}; +/// Propagates a [catchable error](CatchableErrorKind) as `Ok(Err(_))` or returns the unwrapped value in `Ok`. +/// +/// You should use the try operator (`?`) to propagate uncatchable errors before passing catchable +/// errors to `try_cek`. +/// +/// **Input type:** `Result` +/// +/// **Output type:** `T` +/// +/// **Return type of containing function:** `Result, _>` +/// +/// # Example +/// +/// ``` +/// use snix_eval::{try_cek, CatchableErrorKind}; +/// +/// # #[derive(Debug)] struct MyUncatchableError; +/// # fn example() -> Result, MyUncatchableError> { +/// fn my_fn() -> Result, MyUncatchableError> { +/// Ok(Ok(42)) +/// } +/// +/// let value: i32 = try_cek!(my_fn()?); +/// assert_eq!(value, 42); +/// +/// fn my_other_fn() -> Result, MyUncatchableError> { +/// Ok(Err(CatchableErrorKind::AssertionFailed)) +/// } +/// +/// try_cek!(my_other_fn()?); // results in `return Ok(Err(cek))` +/// unreachable!(); +/// +/// # Ok(Ok(())) +/// # } +/// # fn main() { +/// # pretty_assertions::assert_matches!(example().unwrap(), Err(CatchableErrorKind::AssertionFailed)); +/// # } +/// ``` +#[macro_export] +macro_rules! try_cek { + ($result:expr) => { + match $result { + Ok(s) => s, + Err(cek) => { + // Type-check to avoid accidental misuse + let cek: $crate::CatchableErrorKind = cek; + return Ok(Err(cek)); + } + } + }; +} + +/// Propagates a [catchable error](CatchableErrorKind) as `Ok(Value::Catchable(_))` or returns the unwrapped value in `Ok`. +/// +/// **Input type:** `Result` +/// +/// **Output type:** `T` +/// +/// **Return type of containing function:** `Result` +/// +/// See [`try_cek!`]'s documentation for more. +#[macro_export] +macro_rules! try_cek_to_value { + ($result:expr) => { + match $result { + Ok(s) => s, + Err(cek) => { + // Type-check to avoid accidental misuse + let cek: $crate::CatchableErrorKind = cek; + return Ok(Value::Catchable(Box::new(cek))); + } + } + }; +} + /// "CatchableErrorKind" errors -- those which can be detected by /// `builtins.tryEval`. /// diff --git a/snix/glue/src/builtins/derivation.rs b/snix/glue/src/builtins/derivation.rs index cc2e32a47..6586c5ba8 100644 --- a/snix/glue/src/builtins/derivation.rs +++ b/snix/glue/src/builtins/derivation.rs @@ -176,7 +176,7 @@ pub(crate) mod derivation_builtins { use nix_compat::store_path::hash_placeholder; use snix_eval::generators::Gen; - use snix_eval::{NixContext, NixContextElement, NixString}; + use snix_eval::{NixContext, NixContextElement, NixString, try_cek_to_value}; use crate::builtins::utils::{select_string, strong_importing_coerce_to_string}; use crate::fetchurl::fetchurl_derivation_to_fetch; @@ -279,13 +279,10 @@ pub(crate) mod derivation_builtins { // These are only set in drv.arguments. "args" => { for arg in value.to_list()? { - match strong_importing_coerce_to_string(&co, arg).await { - Err(cek) => return Ok(Value::from(cek)), - Ok(s) => { - input_context.mimic(&s); - drv.arguments.push(s.to_str()?.to_owned()) - } - } + let s = + try_cek_to_value!(strong_importing_coerce_to_string(&co, arg).await); + input_context.mimic(&s); + drv.arguments.push(s.to_str()?.to_owned()) } } @@ -338,28 +335,23 @@ pub(crate) mod derivation_builtins { // handle builder and system. "builder" | "system" => { - match strong_importing_coerce_to_string(&co, value).await { - Err(cek) => return Ok(Value::from(cek)), - Ok(val_str) => { - input_context.mimic(&val_str); + let val_str = + try_cek_to_value!(strong_importing_coerce_to_string(&co, value).await); + input_context.mimic(&val_str); - if arg_name == "builder" { - val_str.to_str()?.clone_into(&mut drv.builder); - } else { - val_str.to_str()?.clone_into(&mut drv.system); - } + if arg_name == "builder" { + val_str.to_str()?.clone_into(&mut drv.builder); + } else { + val_str.to_str()?.clone_into(&mut drv.system); + } - // Either populate drv.environment or structured_attrs. - if let Some(ref mut structured_attrs) = structured_attrs { - // No need to check for dups, we only iterate over every attribute name once - structured_attrs.insert( - arg_name.to_owned(), - val_str.to_str()?.to_owned().into(), - ); - } else { - insert_env(&mut drv, arg_name, val_str.as_bytes().into())?; - } - } + // Either populate drv.environment or structured_attrs. + if let Some(ref mut structured_attrs) = structured_attrs { + // No need to check for dups, we only iterate over every attribute name once + structured_attrs + .insert(arg_name.to_owned(), val_str.to_str()?.to_owned().into()); + } else { + insert_env(&mut drv, arg_name, val_str.as_bytes().into())?; } } @@ -384,14 +376,11 @@ pub(crate) mod derivation_builtins { // No need to check for dups, we only iterate over every attribute name once structured_attrs.insert(arg_name.to_owned(), val_json); } else { - match strong_importing_coerce_to_string(&co, value).await { - Err(cek) => return Ok(Value::from(cek)), - Ok(val_str) => { - input_context.mimic(&val_str); + let val_str = + try_cek_to_value!(strong_importing_coerce_to_string(&co, value).await); + input_context.mimic(&val_str); - insert_env(&mut drv, arg_name, val_str.as_bytes().into())?; - } - } + insert_env(&mut drv, arg_name, val_str.as_bytes().into())?; } } } @@ -400,27 +389,21 @@ pub(crate) mod derivation_builtins { // Configure fixed-output derivations if required. { - let output_hash = match select_string(&co, &input, "outputHash") - .await - .context("evaluating the `outputHash` parameter")? - { - Err(cek) => return Ok(Value::from(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::from(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::from(cek)), - Ok(s) => s, - }; + let output_hash = try_cek_to_value!( + select_string(&co, &input, "outputHash") + .await + .context("evaluating the `outputHash` parameter")? + ); + let output_hash_algo = try_cek_to_value!( + select_string(&co, &input, "outputHashAlgo") + .await + .context("evaluating the `outputHashAlgo` parameter")? + ); + let output_hash_mode = try_cek_to_value!( + select_string(&co, &input, "outputHashMode") + .await + .context("evaluating the `outputHashMode` parameter")? + ); if let Some(warning) = handle_fixed_output(&mut drv, output_hash, output_hash_algo, output_hash_mode)? diff --git a/snix/glue/src/builtins/fetchers.rs b/snix/glue/src/builtins/fetchers.rs index e129cd014..8b9a6c9cf 100644 --- a/snix/glue/src/builtins/fetchers.rs +++ b/snix/glue/src/builtins/fetchers.rs @@ -9,7 +9,7 @@ use nix_compat::nixhash::{HashAlgo, NixHash}; use snix_eval::builtin_macros::builtins; use snix_eval::generators::Gen; use snix_eval::generators::GenCo; -use snix_eval::{CatchableErrorKind, ErrorKind, Value}; +use snix_eval::{CatchableErrorKind, ErrorKind, Value, try_cek}; use std::rc::Rc; use url::Url; @@ -56,18 +56,10 @@ async fn extract_fetch_args( )); } - let url_str = match select_string(co, &attrs, "url").await? { - Ok(s) => s.ok_or_else(|| ErrorKind::AttributeNotFound { name: "url".into() })?, - Err(cek) => return Ok(Err(cek)), - }; - let name = match select_string(co, &attrs, "name").await? { - Ok(s) => s, - Err(cek) => return Ok(Err(cek)), - }; - let sha256_str = match select_string(co, &attrs, "sha256").await? { - Ok(s) => s, - Err(cek) => return Ok(Err(cek)), - }; + let url_str = try_cek!(select_string(co, &attrs, "url").await?) + .ok_or_else(|| ErrorKind::AttributeNotFound { name: "url".into() })?; + let name = try_cek!(select_string(co, &attrs, "name").await?); + let sha256_str = try_cek!(select_string(co, &attrs, "sha256").await?); Ok(Ok(NixFetchArgs { url: Url::parse(&url_str).map_err(|e| ErrorKind::SnixError(Rc::new(e)))?, @@ -89,6 +81,7 @@ async fn extract_fetch_args( pub(crate) mod fetcher_builtins { use bstr::ByteSlice; use nix_compat::{flakeref, nixhash::NixHash}; + use snix_eval::try_cek_to_value; use std::collections::BTreeMap; use super::*; @@ -137,10 +130,7 @@ pub(crate) mod fetcher_builtins { co: GenCo, args: Value, ) -> Result { - let args = match extract_fetch_args(&co, args).await? { - Ok(args) => args, - Err(cek) => return Ok(Value::from(cek)), - }; + let args = try_cek_to_value!(extract_fetch_args(&co, args).await?); // Derive the name from the URL basename if not set explicitly. let name = args @@ -163,10 +153,7 @@ pub(crate) mod fetcher_builtins { co: GenCo, args: Value, ) -> Result { - let args = match extract_fetch_args(&co, args).await? { - Ok(args) => args, - Err(cek) => return Ok(Value::from(cek)), - }; + let args = try_cek_to_value!(extract_fetch_args(&co, args).await?); // Name defaults to "source" if not set explicitly. const DEFAULT_NAME_FETCH_TARBALL: &str = "source"; diff --git a/snix/glue/src/builtins/utils.rs b/snix/glue/src/builtins/utils.rs index a2d60855e..8505ce659 100644 --- a/snix/glue/src/builtins/utils.rs +++ b/snix/glue/src/builtins/utils.rs @@ -2,6 +2,7 @@ use bstr::ByteSlice; use snix_eval::{ CatchableErrorKind, CoercionKind, ErrorKind, NixAttrs, NixString, Value, generators::{self, GenCo}, + try_cek, }; pub(super) async fn strong_importing_coerce_to_string( @@ -26,10 +27,8 @@ pub(super) async fn select_string( key: &str, ) -> Result, CatchableErrorKind>, ErrorKind> { if let Some(attr) = attrs.select(key) { - match strong_importing_coerce_to_string(co, attr.clone()).await { - Err(cek) => return Ok(Err(cek)), - Ok(str) => return Ok(Ok(Some(str.to_str()?.to_owned()))), - } + let str = try_cek!(strong_importing_coerce_to_string(co, attr.clone()).await); + return Ok(Ok(Some(str.to_str()?.to_owned()))); } Ok(Ok(None))