diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c0aa953..2b7c97f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,12 +3,51 @@ ## [Unreleased] (ReleaseDate) +### New feature + +Like [anyhow], now you can use `?` to propagate any error that implements the +`std::error::Error` trait. + +[anyhow]: https://docs.rs/anyhow/latest/anyhow/index.html + +```rust +#[savvy] +fn no_such_file() -> savvy::Result<()> { + // previously, you had to write .map_err(|e| e.to_string().into()) + let _ = std::fs::read_to_string("no_such_file")?; + Ok(()) +} +``` + +If you want to implement your own error type and the conversion to +`savvy::Error`, please sepcify `use-custom-error` feature to opt-out the +auto-conversion to avoid conflict with `impl From for savvy::Error` + +```toml +savvy = { version = "...", features = ["use-custom-error"] } +``` + +### Breaking change + +By introducing the anyhow-like conversion, savvy loses the error conversion from +a string (e.g. `Err("foo".into())`). Instead, please use `savvy_err!()` macro, +which is a shorthand of `Error::new(format!(...))`. + +```rust +#[savvy] +fn raise_error() -> savvy::Result { + Err(savvy_err!("This is my custom error")) +} +``` + +### Minor improvements + * `NumericScalar::as_usize()` and `NumericSexp::iter_usize()` now rejects numbers larger than `2^32` on 32-bit targets (i.e. webR). Thanks @eitsupi! ## [v0.7.2] (2024-10-27) -### Minor Improvements +### Minor improvements * savvy now generates a reduced number of R functions. @@ -24,7 +63,7 @@ ## [v0.7.0] (2024-10-20) -### Breaking Change +### Breaking change Removed `TryFrom for usize`, so the following code no longer compiles. @@ -44,10 +83,10 @@ by yourself. Also, please be aware you need to handle NA as well. #[savvy] fn foo(x: i32) -> savvy::Result<()> { if x.is_na() { - return Err("cannot convert NA to usize".into())?; + return Err(savvy_err!("cannot convert NA to usize")); } - let x = ::try_from(x).map_err(|e| e.to_string().into()); + let x = ::try_from(x)?; ... } @@ -72,7 +111,7 @@ usize_to_string_scalar(2147483648) ## [v0.6.8] (2024-09-17) -### Minor Improvements +### Minor improvements * `savvy init` now generates * slightly better `configure` script that checks if `cargo` command is available @@ -82,7 +121,7 @@ usize_to_string_scalar(2147483648) ## [v0.6.7] (2024-09-05) -### Minor Improvements +### Minor improvements * Remove the use of non-API call `Rf_findVarInFrame`. @@ -115,7 +154,7 @@ usize_to_string_scalar(2147483648) binary data on Rust's side, your primary option should be to store it in an external pointer (of a struct you define) rather than an R's raw vector. -### Minor Improvements +### Minor improvements * Wrapper environment of a Rust struct or enum now cannot be modified by users. @@ -158,7 +197,7 @@ usize_to_string_scalar(2147483648) * `r_print!()` and `r_eprint!()` now can print strings containing `%`. -### Breaking Change +### Breaking change * The notation for `savvy-cli test` is now changed to `#[cfg(feature = "savvy-test")]` from `#[cfg(savvy_test)]`. This is to avoid the upcoming diff --git a/Cargo.toml b/Cargo.toml index 83495fd8..d1bf09cb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -34,6 +34,12 @@ altrep = ["savvy-ffi/altrep"] # Support logger logger = ["log", "env_logger"] +# By default, savvy provides `impl From for Error`. +# However, this conflicts if the user implements their custom error and the +# conversion from it to savvy::Error. This flag removes the impl to allow such a +# custom error. +use-custom-error = [] + savvy-test = [] [build-dependencies] diff --git a/R-package/R/000-wrappers.R b/R-package/R/000-wrappers.R index c90bebfc..7bc8446a 100644 --- a/R-package/R/000-wrappers.R +++ b/R-package/R/000-wrappers.R @@ -523,6 +523,11 @@ NULL } +`error_conversion` <- function() { + invisible(.Call(savvy_error_conversion__impl)) +} + + `do_call` <- function(`fun`, `args`) { .Call(savvy_do_call__impl, `fun`, `args`) } diff --git a/R-package/src/init.c b/R-package/src/init.c index e83de904..ab783529 100644 --- a/R-package/src/init.c +++ b/R-package/src/init.c @@ -479,6 +479,11 @@ SEXP savvy_safe_warn__impl(void) { return handle_result(res); } +SEXP savvy_error_conversion__impl(void) { + SEXP res = savvy_error_conversion__ffi(); + return handle_result(res); +} + SEXP savvy_do_call__impl(SEXP c_arg__fun, SEXP c_arg__args) { SEXP res = savvy_do_call__ffi(c_arg__fun, c_arg__args); return handle_result(res); @@ -809,6 +814,7 @@ static const R_CallMethodDef CallEntries[] = { {"savvy_raise_error__impl", (DL_FUNC) &savvy_raise_error__impl, 0}, {"savvy_must_panic__impl", (DL_FUNC) &savvy_must_panic__impl, 0}, {"savvy_safe_warn__impl", (DL_FUNC) &savvy_safe_warn__impl, 0}, + {"savvy_error_conversion__impl", (DL_FUNC) &savvy_error_conversion__impl, 0}, {"savvy_do_call__impl", (DL_FUNC) &savvy_do_call__impl, 2}, {"savvy_call_with_args__impl", (DL_FUNC) &savvy_call_with_args__impl, 1}, {"savvy_get_args__impl", (DL_FUNC) &savvy_get_args__impl, 1}, diff --git a/R-package/src/rust/api.h b/R-package/src/rust/api.h index 86aaa471..13c5c522 100644 --- a/R-package/src/rust/api.h +++ b/R-package/src/rust/api.h @@ -87,6 +87,7 @@ SEXP savvy_safe_stop__ffi(void); SEXP savvy_raise_error__ffi(void); SEXP savvy_must_panic__ffi(void); SEXP savvy_safe_warn__ffi(void); +SEXP savvy_error_conversion__ffi(void); SEXP savvy_do_call__ffi(SEXP c_arg__fun, SEXP c_arg__args); SEXP savvy_call_with_args__ffi(SEXP c_arg__fun); SEXP savvy_get_args__ffi(SEXP c_arg__args); diff --git a/R-package/src/rust/src/altrep.rs b/R-package/src/rust/src/altrep.rs index 65c6c4e9..99ae3d40 100644 --- a/R-package/src/rust/src/altrep.rs +++ b/R-package/src/rust/src/altrep.rs @@ -4,8 +4,8 @@ use savvy::altrep::{ register_altstring_class, AltInteger, AltList, AltLogical, AltRaw, AltReal, AltString, }; use savvy::{ - r_println, savvy, savvy_init, IntegerSexp, ListSexp, LogicalSexp, NullSexp, RawSexp, RealSexp, - StringSexp, + r_println, savvy, savvy_err, savvy_init, IntegerSexp, ListSexp, LogicalSexp, NullSexp, RawSexp, + RealSexp, StringSexp, }; // integer @@ -47,7 +47,7 @@ fn print_altint(x: IntegerSexp) -> savvy::Result<()> { return Ok(()); }; - Err("Not a known ALTREP".into()) + Err(savvy_err!("Not a known ALTREP")) } #[savvy] @@ -60,7 +60,7 @@ fn tweak_altint(mut x: IntegerSexp) -> savvy::Result<()> { return Ok(()); }; - Err("Not a known ALTREP".into()) + Err(savvy_err!("Not a known ALTREP")) } // real @@ -102,7 +102,7 @@ fn print_altreal(x: RealSexp) -> savvy::Result<()> { return Ok(()); }; - Err("Not a known ALTREP".into()) + Err(savvy_err!("Not a known ALTREP")) } #[savvy] @@ -115,7 +115,7 @@ fn tweak_altreal(mut x: RealSexp) -> savvy::Result<()> { return Ok(()); }; - Err("Not a known ALTREP".into()) + Err(savvy_err!("Not a known ALTREP")) } // logical @@ -157,7 +157,7 @@ fn print_altlogical(x: LogicalSexp) -> savvy::Result<()> { return Ok(()); }; - Err("Not a known ALTREP".into()) + Err(savvy_err!("Not a known ALTREP")) } #[savvy] @@ -170,7 +170,7 @@ fn tweak_altlogical(mut x: LogicalSexp) -> savvy::Result<()> { return Ok(()); }; - Err("Not a known ALTREP".into()) + Err(savvy_err!("Not a known ALTREP")) } // raw @@ -212,7 +212,7 @@ fn print_altraw(x: RawSexp) -> savvy::Result<()> { return Ok(()); }; - Err("Not a known ALTREP".into()) + Err(savvy_err!("Not a known ALTREP")) } #[savvy] @@ -225,7 +225,7 @@ fn tweak_altraw(mut x: RawSexp) -> savvy::Result<()> { return Ok(()); }; - Err("Not a known ALTREP".into()) + Err(savvy_err!("Not a known ALTREP")) } // string @@ -267,7 +267,7 @@ fn print_altstring(x: StringSexp) -> savvy::Result<()> { return Ok(()); }; - Err("Not a known ALTREP".into()) + Err(savvy_err!("Not a known ALTREP")) } #[savvy] @@ -280,7 +280,7 @@ fn tweak_altstring(mut x: StringSexp) -> savvy::Result<()> { return Ok(()); }; - Err("Not a known ALTREP".into()) + Err(savvy_err!("Not a known ALTREP")) } // list @@ -337,7 +337,7 @@ fn print_altlist(x: ListSexp) -> savvy::Result<()> { return Ok(()); }; - Err("Not a known ALTREP".into()) + Err(savvy_err!("Not a known ALTREP")) } #[savvy] @@ -356,7 +356,7 @@ fn tweak_altlist(mut x: ListSexp) -> savvy::Result<()> { return Ok(()); }; - Err("Not a known ALTREP".into()) + Err(savvy_err!("Not a known ALTREP")) } // initialization @@ -400,6 +400,6 @@ fn print_altint_by_weird_way(x: IntegerSexp) -> savvy::Result<()> { r_println!("{out:?}"); Ok(()) } else { - Err("Not an altint".into()) + Err(savvy_err!("Not an altint")) } } diff --git a/R-package/src/rust/src/environment.rs b/R-package/src/rust/src/environment.rs index bacce9b9..4bd76032 100644 --- a/R-package/src/rust/src/environment.rs +++ b/R-package/src/rust/src/environment.rs @@ -1,10 +1,10 @@ -use savvy::{savvy, EnvironmentSexp, Sexp}; +use savvy::{savvy, savvy_err, EnvironmentSexp, Sexp}; #[savvy] fn get_var_in_env(name: &str, env: Option) -> savvy::Result { let env = env.unwrap_or(EnvironmentSexp::global_env()); let obj = env.get(name)?; - obj.ok_or("Not found".into()) + obj.ok_or(savvy_err!("Not found")) } #[savvy] diff --git a/R-package/src/rust/src/error_handling.rs b/R-package/src/rust/src/error_handling.rs index 0df3e76b..ed8558ac 100644 --- a/R-package/src/rust/src/error_handling.rs +++ b/R-package/src/rust/src/error_handling.rs @@ -1,4 +1,4 @@ -use savvy::{savvy, savvy_init, NullSexp, Sexp}; +use savvy::{savvy, savvy_err, savvy_init, NullSexp, Sexp}; use savvy_ffi::DllInfo; use std::ffi::CString; @@ -8,10 +8,10 @@ static FOO_VALUE: OnceLock> = OnceLock::new(); #[savvy_init] fn init_foo_value(dll: *mut DllInfo) -> savvy::Result<()> { - match FOO_VALUE.set(Mutex::new(-1)) { - Ok(_) => Ok(()), - Err(_) => Err("Failed to set values".into()), - } + FOO_VALUE + .set(Mutex::new(-1)) + .map_err(|_| savvy_err!("Failed to set values"))?; + Ok(()) } struct Foo {} @@ -38,7 +38,7 @@ impl Drop for Foo { fn get_foo_value() -> savvy::Result { match FOO_VALUE.get() { Some(x) => { - let v = *x.lock().unwrap(); + let v = *x.lock()?; v.try_into() } None => NullSexp.into(), @@ -61,7 +61,7 @@ fn safe_stop() -> savvy::Result<()> { #[savvy] fn raise_error() -> savvy::Result { - Err(savvy::Error::new("This is my custom error")) + Err(savvy_err!("This is my custom error")) } #[allow(clippy::out_of_bounds_indexing, unconditional_panic)] @@ -80,3 +80,9 @@ fn safe_warn() -> savvy::Result<()> { Ok(()) } + +#[savvy] +fn error_conversion() -> savvy::Result<()> { + let _ = std::fs::read_to_string("no_such_file")?; + Ok(()) +} diff --git a/book/src/altrep.md b/book/src/altrep.md index 66a18b3c..67102f21 100644 --- a/book/src/altrep.md +++ b/book/src/altrep.md @@ -147,7 +147,7 @@ fn print_altint(x: IntegerSexp) -> savvy::Result<()> { return Ok(()); }; - Err("Not a known ALTREP".into()) + Err(savvy_err!("Not a known ALTREP")) } ``` @@ -183,7 +183,7 @@ fn tweak_altint(mut x: IntegerSexp) -> savvy::Result<()> { return Ok(()); }; - Err("Not a known ALTREP".into()) + Err(savvy_err!("Not a known ALTREP")) } ``` diff --git a/book/src/enum.md b/book/src/enum.md index 3baab9ed..9e284f1f 100644 --- a/book/src/enum.md +++ b/book/src/enum.md @@ -67,7 +67,7 @@ fn plot_line(x: IntegerSexp, y: IntegerSexp, line_type: &str) -> savvy::Result<( ... }, _ => { - return Err("Unsupported line type!".into()); + return Err(savvy_err!("Unsupported line type!")); } } } diff --git a/book/src/error.md b/book/src/error.md index f627e711..77849f98 100644 --- a/book/src/error.md +++ b/book/src/error.md @@ -1,13 +1,15 @@ # Error handling -To propagate your errors to the R session, you can return a `savvy::Error`. You -can easily create it by using `.into()` on a `&str` containing the error message. +To propagate your errors to the R session, you can return a `savvy::Error`. +`savvy_err!()` macro is a shortcut of `savvy::Error::new(format!(...))` to +create a new error. ```rust -/// @export +use savvy::savvy_err; + #[savvy] fn raise_error() -> savvy::Result { - Err("This is my custom error".into()) + Err(savvy_err!("This is my custom error")) } ``` @@ -16,8 +18,29 @@ raise_error() #> Error: This is my custom error ``` -For the implementation details of the internals, please refer to [my blog -post](https://yutani.rbind.io/post/dont-panic-we-can-unwind/#implementation). +Like [anyhow], you can use `?` to easily propagate any error that implements the +`std::error::Error` trait. + +[anyhow]: https://docs.rs/anyhow/latest/anyhow/index.html + +```rust +#[savvy] +fn no_such_file() -> savvy::Result<()> { + let _ = std::fs::read_to_string("no_such_file")?; + Ok(()) +} +``` + +### Custom error + +If you want to implement your own error type and the conversion to +`savvy::Error`, it would conflict with the conversion of `From`. +To avoid an compile error, please sepcify `use-custom-error` feature to opt-out +the conversion. + +```toml +savvy = { version = "...", features = ["use-custom-error"] } +``` ## Show a warning diff --git a/book/src/key_ideas.md b/book/src/key_ideas.md index 7e349885..3c5fb632 100644 --- a/book/src/key_ideas.md +++ b/book/src/key_ideas.md @@ -94,7 +94,7 @@ fn identity_num(x: Sexp) -> savvy::Result { match x.into_typed() { TypedSexp::Integer(i) => identity_int(i), TypedSexp::Real(r) => identity_real(r), - _ => Err("Expected integer or numeric".into()), + _ => Err(savvy_err!("Expected integer or numeric")), } } ``` \ No newline at end of file diff --git a/book/src/matrix.md b/book/src/matrix.md index 5282b862..cfaf6338 100644 --- a/book/src/matrix.md +++ b/book/src/matrix.md @@ -54,7 +54,7 @@ fn nalgebra_input(x: RealSexp) -> savvy::Result<()> { let dim = x.get_dim().ok_or("no dimension found")?; if dim.len() != 2 { - return Err("Input must be matrix!".into()); + return Err(savvy_err!("Input must be matrix!")); } let m = DMatrix::from_vec(dim[0] as _, dim[1] as _, x.to_vec()); @@ -81,7 +81,7 @@ fn glam_input(x: RealSexp) -> savvy::Result<()> { let dim = x.get_dim().ok_or("no dimension found")?; if dim != [3, 3] { - return Err("Input must be 3x3 matrix!".into()); + return Err(savvy_err!("Input must be 3x3 matrix!")); } // As we already check the dimension, this must not fail diff --git a/savvy-bindgen/src/gen/rust.rs b/savvy-bindgen/src/gen/rust.rs index 9dec2c12..61be0193 100644 --- a/savvy-bindgen/src/gen/rust.rs +++ b/savvy-bindgen/src/gen/rust.rs @@ -121,7 +121,7 @@ impl SavvyFn { match result { Ok(orig_result) => orig_result, - Err(_) => Err("panic happened".into()), + Err(_) => Err(savvy::savvy_err!("panic happened")), } }); out.block = Box::new(new_body); diff --git a/savvy-bindgen/src/ir/savvy_enum.rs b/savvy-bindgen/src/ir/savvy_enum.rs index 4eb61e4f..154b6f0b 100644 --- a/savvy-bindgen/src/ir/savvy_enum.rs +++ b/savvy-bindgen/src/ir/savvy_enum.rs @@ -116,7 +116,7 @@ impl SavvyEnum { let i = ::try_from(value)?; match i { #(#match_arms_ref),*, - _ => Err("Unexpected enum variant".into()), + _ => Err(savvy::savvy_err!("Unexpected enum variant")), } } } @@ -129,7 +129,7 @@ impl SavvyEnum { let i = ::try_from(value)?; match i { #(#match_arms),*, - _ => Err("Unexpected enum variant".into()), + _ => Err(savvy::savvy_err!("Unexpected enum variant")), } } } diff --git a/savvy-bindgen/src/ir/savvy_struct.rs b/savvy-bindgen/src/ir/savvy_struct.rs index 63e15aa3..e4e4190a 100644 --- a/savvy-bindgen/src/ir/savvy_struct.rs +++ b/savvy-bindgen/src/ir/savvy_struct.rs @@ -58,7 +58,7 @@ impl SavvyStruct { let x = unsafe { savvy::get_external_pointer_addr(value.0)? as *mut #ty }; let res = unsafe { x.as_ref() }; - res.ok_or("Failed to convert the external pointer to the Rust object".into()) + res.ok_or(savvy::savvy_err!("Failed to convert the external pointer to the Rust object")) } } ); @@ -73,7 +73,7 @@ impl SavvyStruct { let x = unsafe { savvy::get_external_pointer_addr(value.0)? as *mut #ty }; let res = unsafe { x.as_mut() }; - res.ok_or("Failed to convert the external pointer to the Rust object".into()) + res.ok_or(savvy::savvy_err!("Failed to convert the external pointer to the Rust object")) } } ); diff --git a/savvy-bindgen/src/parse_file.rs b/savvy-bindgen/src/parse_file.rs index 54f787ab..9ec337cf 100644 --- a/savvy-bindgen/src/parse_file.rs +++ b/savvy-bindgen/src/parse_file.rs @@ -508,7 +508,7 @@ fn wrap_with_test_function( eprintln!("ok"); Ok(()) } - Err(_) => Err("test failed".into()), + Err(_) => Err(savvy::savvy_err!("test failed")), } } } diff --git a/savvy-macro/src/snapshots/savvy_macro__tests__assert_snapshot_inner-2.snap b/savvy-macro/src/snapshots/savvy_macro__tests__assert_snapshot_inner-2.snap index ff1b6c4a..19950a3d 100644 --- a/savvy-macro/src/snapshots/savvy_macro__tests__assert_snapshot_inner-2.snap +++ b/savvy-macro/src/snapshots/savvy_macro__tests__assert_snapshot_inner-2.snap @@ -9,6 +9,6 @@ expression: lines - " std::panic::set_hook(orig_hook);" - " match result {" - " Ok(orig_result) => orig_result," -- " Err(_) => Err(\"panic happened\".into())," +- " Err(_) => Err(savvy::savvy_err!(\"panic happened\"))," - " }" - "}" diff --git a/savvy-macro/src/snapshots/savvy_macro__tests__assert_snapshot_inner-3.snap b/savvy-macro/src/snapshots/savvy_macro__tests__assert_snapshot_inner-3.snap index b102777a..6bdd2350 100644 --- a/savvy-macro/src/snapshots/savvy_macro__tests__assert_snapshot_inner-3.snap +++ b/savvy-macro/src/snapshots/savvy_macro__tests__assert_snapshot_inner-3.snap @@ -17,6 +17,6 @@ expression: lines - " std::panic::set_hook(orig_hook);" - " match result {" - " Ok(orig_result) => orig_result," -- " Err(_) => Err(\"panic happened\".into())," +- " Err(_) => Err(savvy::savvy_err!(\"panic happened\"))," - " }" - "}" diff --git a/savvy-macro/src/snapshots/savvy_macro__tests__assert_snapshot_inner-4.snap b/savvy-macro/src/snapshots/savvy_macro__tests__assert_snapshot_inner-4.snap index 27101439..23d07854 100644 --- a/savvy-macro/src/snapshots/savvy_macro__tests__assert_snapshot_inner-4.snap +++ b/savvy-macro/src/snapshots/savvy_macro__tests__assert_snapshot_inner-4.snap @@ -12,6 +12,6 @@ expression: lines - " std::panic::set_hook(orig_hook);" - " match result {" - " Ok(orig_result) => orig_result," -- " Err(_) => Err(\"panic happened\".into())," +- " Err(_) => Err(savvy::savvy_err!(\"panic happened\"))," - " }" - "}" diff --git a/savvy-macro/src/snapshots/savvy_macro__tests__assert_snapshot_inner.snap b/savvy-macro/src/snapshots/savvy_macro__tests__assert_snapshot_inner.snap index 4da2b0df..d0fde356 100644 --- a/savvy-macro/src/snapshots/savvy_macro__tests__assert_snapshot_inner.snap +++ b/savvy-macro/src/snapshots/savvy_macro__tests__assert_snapshot_inner.snap @@ -9,6 +9,6 @@ expression: lines - " std::panic::set_hook(orig_hook);" - " match result {" - " Ok(orig_result) => orig_result," -- " Err(_) => Err(\"panic happened\".into())," +- " Err(_) => Err(savvy::savvy_err!(\"panic happened\"))," - " }" - "}" diff --git a/src/altrep/mod.rs b/src/altrep/mod.rs index c1c9f45e..97798d58 100644 --- a/src/altrep/mod.rs +++ b/src/altrep/mod.rs @@ -23,7 +23,7 @@ use savvy_ffi::{ R_NilValue, Rboolean_TRUE, ATTRIB, CADR, CAR, PRINTNAME, SEXP, }; -use crate::{protect::local_protect, sexp::utils::charsxp_to_str, IntoExtPtrSexp}; +use crate::{protect::local_protect, savvy_err, sexp::utils::charsxp_to_str, IntoExtPtrSexp}; /// This stores the ALTREP class objects static ALTREP_CLASS_CATALOGUE: OnceCell>> = @@ -39,9 +39,7 @@ pub(crate) fn create_altrep_instance( let catalogue_mutex = ALTREP_CLASS_CATALOGUE.get().ok_or(crate::Error::new( "ALTREP_CLASS_CATALOGUE is not initialized", ))?; - let catalogue = catalogue_mutex - .lock() - .map_err(|e| crate::Error::new(&e.to_string()))?; + let catalogue = catalogue_mutex.lock()?; let class = catalogue .get(class_name) .ok_or(crate::Error::new("Failed to get the ALTREP class"))?; @@ -64,16 +62,14 @@ fn register_altrep_class( let catalogue_mutex = ALTREP_CLASS_CATALOGUE.get().ok_or(crate::Error::new( "ALTREP_CLASS_CATALOGUE is not initialized", ))?; - let mut catalogue = catalogue_mutex - .lock() - .map_err(|e| crate::Error::new(&e.to_string()))?; + let mut catalogue = catalogue_mutex.lock()?; let existing_entry = catalogue.insert(class_name, class_t); if existing_entry.is_some() { - return Err( - "[WARN] ALTREP class {class_name} is already defined. Something seems wrong.".into(), - ); + return Err(savvy_err!( + "[WARN] ALTREP class {class_name} is already defined. Something seems wrong." + )); } Ok(()) @@ -86,7 +82,7 @@ fn register_altrep_class( /// unexpected might happen. pub unsafe fn get_altrep_class_name(x: SEXP) -> crate::error::Result<&'static str> { if unsafe { ALTREP(x) } != 1 { - return Err("Not an ALTREP".into()); + return Err(savvy_err!("Not an ALTREP")); } let class_name_symbol = unsafe { CAR(ATTRIB(ALTREP_CLASS(x))) }; @@ -100,7 +96,7 @@ pub unsafe fn get_altrep_class_name(x: SEXP) -> crate::error::Result<&'static st /// unexpected might happen. pub unsafe fn get_altrep_package_name(x: SEXP) -> crate::error::Result<&'static str> { if unsafe { ALTREP(x) } != 1 { - return Err("Not an ALTREP".into()); + return Err(savvy_err!("Not an ALTREP")); } let class_name_symbol = unsafe { CADR(ATTRIB(ALTREP_CLASS(x))) }; @@ -114,12 +110,14 @@ pub unsafe fn get_altrep_package_name(x: SEXP) -> crate::error::Result<&'static /// user's responsibility to ensure the underlying pointer is `T`. pub unsafe fn get_altrep_body_mut_unchecked(x: &mut SEXP) -> crate::error::Result<&mut T> { if unsafe { ALTREP(*x) } != 1 { - return Err("Not an ALTREP".into()); + return Err(savvy_err!("Not an ALTREP")); } let ptr = unsafe { crate::get_external_pointer_addr(R_altrep_data1(*x))? as *mut T }; let ref_mut = unsafe { ptr.as_mut() }; - ref_mut.ok_or("Failed to convert the external pointer to the Rust object".into()) + ref_mut.ok_or(savvy_err!( + "Failed to convert the external pointer to the Rust object" + )) } /// Returns the `data1` of an ALTREP object as `&`. @@ -129,12 +127,14 @@ pub unsafe fn get_altrep_body_mut_unchecked(x: &mut SEXP) -> crate::error::Re /// user's responsibility to ensure the underlying pointer is `T`. pub unsafe fn get_altrep_body_ref_unchecked(x: &SEXP) -> crate::error::Result<&T> { if unsafe { ALTREP(*x) } != 1 { - return Err("Not an ALTREP".into()); + return Err(savvy_err!("Not an ALTREP")); } let ptr = unsafe { crate::get_external_pointer_addr(R_altrep_data1(*x))? as *const T }; let ref_mut = unsafe { ptr.as_ref() }; - ref_mut.ok_or("Failed to convert the external pointer to the Rust object".into()) + ref_mut.ok_or(savvy_err!( + "Failed to convert the external pointer to the Rust object" + )) } // Some helpers @@ -144,7 +144,9 @@ pub unsafe fn get_altrep_body_ref_unchecked(x: &SEXP) -> crate::error::Result pub(crate) fn extract_ref_from_altrep(x: &SEXP) -> crate::Result<&T> { let x = unsafe { crate::get_external_pointer_addr(R_altrep_data1(*x)).unwrap() as *mut T }; let self_ = unsafe { x.as_ref() }; - self_.ok_or("Failed to convert the external pointer to the Rust object".into()) + self_.ok_or(savvy_err!( + "Failed to convert the external pointer to the Rust object" + )) } /// Extracts &mut T @@ -153,7 +155,9 @@ pub(crate) fn extract_ref_from_altrep(x: &SEXP) -> crate::Result<&T> { pub(crate) fn extract_mut_from_altrep(x: &mut SEXP) -> crate::Result<&mut T> { let x = unsafe { crate::get_external_pointer_addr(R_altrep_data1(*x)).unwrap() as *mut T }; let self_ = unsafe { x.as_mut() }; - self_.ok_or("Failed to convert the external pointer to the Rust object".into()) + self_.ok_or(savvy_err!( + "Failed to convert the external pointer to the Rust object" + )) } /// Extract T @@ -169,20 +173,17 @@ pub(crate) fn assert_altrep_class(x: SEXP, class_name: &'static str) -> crate::e let catalogue_mutex = ALTREP_CLASS_CATALOGUE.get().ok_or(crate::Error::new( "ALTREP_CLASS_CATALOGUE is not initialized", ))?; - let catalogue = catalogue_mutex - .lock() - .map_err(|e| crate::Error::new(&e.to_string()))?; + let catalogue = catalogue_mutex.lock()?; let class = catalogue .get(class_name) - .ok_or(crate::Error::new("Failed to get the ALTREP class"))?; + .ok_or(savvy_err!("Failed to get the ALTREP class"))?; if unsafe { R_altrep_inherits(x, *class) == Rboolean_TRUE } { Ok(()) } else { - Err(format!( + Err(savvy_err!( "Not an object of the specified ALTREP class ({})", class_name - ) - .into()) + )) } } diff --git a/src/error.rs b/src/error.rs index 4cb36fff..7a166161 100644 --- a/src/error.rs +++ b/src/error.rs @@ -1,3 +1,5 @@ +use std::ops::Deref; + use savvy_ffi::SEXP; #[derive(Debug)] @@ -11,7 +13,7 @@ pub enum Error { } impl Error { - pub fn new(msg: &str) -> Self { + pub fn new(msg: T) -> Self { Self::GeneralError(msg.to_string()) } } @@ -35,18 +37,6 @@ impl std::fmt::Display for Error { } } -impl std::error::Error for Error { - fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - None - } -} - -impl From> for Error { - fn from(e: Box) -> Error { - Error::new(&e.to_string()) - } -} - impl crate::error::Error { pub fn with_arg_name(self, arg_name: &str) -> Self { match self { @@ -64,20 +54,108 @@ impl crate::error::Error { } } -impl From<&str> for Error { - fn from(msg: &str) -> Error { - Error::new(msg) +// To avoid the conflict with `From for savvy::Error` and +// `From for T`, `savvy::Error` cannot implement `std::error::Error` trait +// directly. Instead, it implements `From for Box`. +// This struct is to provide the std::error::Error trait. +// +// This idea is from anyhow crate. cf. https://github.com/dtolnay/anyhow/blob/master/src/error.rs +#[derive(Debug)] +struct ErrorImpl(String); + +impl std::fmt::Display for ErrorImpl { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.0) } } -impl From for Error { - fn from(msg: String) -> Error { - Error::new(&msg) +unsafe impl Send for ErrorImpl {} +unsafe impl Sync for ErrorImpl {} + +impl std::error::Error for ErrorImpl {} + +impl From for Box { + fn from(value: Error) -> Self { + Box::new(ErrorImpl(value.to_string())) + } +} + +impl From for Box { + fn from(value: Error) -> Self { + Box::new(ErrorImpl(value.to_string())) + } +} + +// Note: Unlike anyhow::Error, this doesn't require Send and Sync. This is +// because, +// +// - anyhow preserves the original implementation for std::error::Error by +// accessing vtable directly. +// - anyhow needs to be async-aware (cf. +// https://github.com/dtolnay/anyhow/issues/81) +// +// However, savvy creates a string immediately here (because only a string can +// be propagated to R session), so both won't be a problem. +#[cfg(not(feature = "use-custom-error"))] +impl From for Error +where + E: std::error::Error + 'static, +{ + fn from(value: E) -> Self { + Self::new(value) } } +#[cfg(feature = "use-custom-error")] +impl From> for Error { + fn from(e: Box) -> Error { + Error::new(&e.to_string()) + } +} + +// In the case of no automatic error conversion, provide some common conversion +// for convenience. + +#[cfg(feature = "use-custom-error")] impl From for Error { fn from(value: std::convert::Infallible) -> Self { - Error::new(&value.to_string()) + Self::new(value) + } +} + +#[cfg(feature = "use-custom-error")] +impl From for Error { + fn from(value: std::num::TryFromIntError) -> Self { + Self::new(value) + } +} + +// For CString +#[cfg(feature = "use-custom-error")] +impl From for Error { + fn from(value: std::ffi::NulError) -> Self { + Self::new(value) + } +} + +// For Mutex +#[cfg(feature = "use-custom-error")] +impl From> for Error { + fn from(value: std::sync::PoisonError) -> Self { + Self::new(value) + } +} + +#[cfg(feature = "use-custom-error")] +impl From for Error { + fn from(value: String) -> Self { + Self::new(value) + } +} + +#[cfg(feature = "use-custom-error")] +impl From<&str> for Error { + fn from(value: &str) -> Self { + Self::new(value) } } diff --git a/src/lib.rs b/src/lib.rs index 98ed12e8..42096bd3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -135,23 +135,29 @@ pub fn handle_error(e: crate::error::Error) -> SEXP { #[macro_export] macro_rules! r_print { () => {}; - ($($arg:tt)*) => { savvy::io::r_print(&format!($($arg)*), false); }; + ($($arg:tt)*) => { $crate::io::r_print(&format!($($arg)*), false); }; } #[macro_export] macro_rules! r_eprint { () => {}; - ($($arg:tt)*) => { savvy::io::r_eprint(&format!($($arg)*), false); }; + ($($arg:tt)*) => { $crate::io::r_eprint(&format!($($arg)*), false); }; } #[macro_export] macro_rules! r_println { () => {}; - ($($arg:tt)*) => { savvy::io::r_print(&format!($($arg)*), true); }; + ($($arg:tt)*) => { $crate::io::r_print(&format!($($arg)*), true); }; } #[macro_export] macro_rules! r_eprintln { () => {}; - ($($arg:tt)*) => { savvy::io::r_eprint(&format!($($arg)*), true); }; + ($($arg:tt)*) => { $crate::io::r_eprint(&format!($($arg)*), true); }; +} + +#[macro_export] +macro_rules! savvy_err { + () => {}; + ($($arg:tt)*) => { $crate::Error::new(&format!($($arg)*)) }; } diff --git a/src/sexp/environment.rs b/src/sexp/environment.rs index 161384b4..f39b6d08 100644 --- a/src/sexp/environment.rs +++ b/src/sexp/environment.rs @@ -2,7 +2,7 @@ use std::ffi::CString; use savvy_ffi::{R_GlobalEnv, R_NilValue, R_UnboundValue, Rboolean_TRUE, SEXP}; -use crate::Sexp; +use crate::{savvy_err, Sexp}; use super::utils::str_to_symsxp; @@ -33,7 +33,7 @@ impl EnvironmentSexp { /// words, you should never use this if you don't understand how R's /// protection mechanism works. pub fn get>(&self, name: T) -> crate::error::Result> { - let sym = str_to_symsxp(name)?.ok_or("name must not be empty")?; + let sym = str_to_symsxp(name)?.ok_or(savvy_err!("name must not be empty"))?; // Note: since this SEXP already belongs to an environment, this doesn't // need protection. @@ -58,7 +58,7 @@ impl EnvironmentSexp { /// Returns `true` the specified environment contains the specified /// variable. pub fn contains>(&self, name: T) -> crate::error::Result { - let sym = str_to_symsxp(name)?.ok_or("name must not be empty")?; + let sym = str_to_symsxp(name)?.ok_or(savvy_err!("name must not be empty"))?; let res = unsafe { crate::unwind_protect(|| { @@ -79,10 +79,7 @@ impl EnvironmentSexp { /// Bind the SEXP to the specified environment as the specified name. pub fn set>(&self, name: T, value: Sexp) -> crate::error::Result<()> { - let name_cstr = match CString::new(name.as_ref()) { - Ok(cstr) => cstr, - Err(e) => return Err(crate::error::Error::new(&e.to_string())), - }; + let name_cstr = CString::new(name.as_ref())?; unsafe { crate::unwind_protect(|| { diff --git a/src/sexp/mod.rs b/src/sexp/mod.rs index b6eeb3af..a2337779 100644 --- a/src/sexp/mod.rs +++ b/src/sexp/mod.rs @@ -315,10 +315,7 @@ impl Sexp { /// Returns the specified attribute. pub fn get_attrib(&self, attr: &str) -> crate::error::Result> { - let attr_cstr = match CString::new(attr) { - Ok(cstr) => cstr, - Err(e) => return Err(crate::error::Error::new(&e.to_string())), - }; + let attr_cstr = CString::new(attr)?; let attr_sexp = unsafe { crate::unwind_protect(|| { savvy_ffi::Rf_getAttrib(self.0, savvy_ffi::Rf_install(attr_cstr.as_ptr())) @@ -361,10 +358,7 @@ impl Sexp { /// Set the input value to the specified attribute. pub fn set_attrib(&mut self, attr: &str, value: Sexp) -> crate::error::Result<()> { - let attr_cstr = match CString::new(attr) { - Ok(cstr) => cstr, - Err(e) => return Err(crate::error::Error::new(&e.to_string())), - }; + let attr_cstr = CString::new(attr)?; unsafe { crate::unwind_protect(|| { savvy_ffi::Rf_setAttrib(self.0, savvy_ffi::Rf_install(attr_cstr.as_ptr()), value.0) diff --git a/src/sexp/numeric.rs b/src/sexp/numeric.rs index 28b5f1fd..24b49fcb 100644 --- a/src/sexp/numeric.rs +++ b/src/sexp/numeric.rs @@ -1,6 +1,6 @@ use once_cell::sync::OnceCell; -use crate::{IntegerSexp, NotAvailableValue, RealSexp, Sexp}; +use crate::{savvy_err, IntegerSexp, NotAvailableValue, RealSexp, Sexp}; // --- Utils ------------------------- @@ -24,9 +24,9 @@ fn try_cast_f64_to_i32(f: f64) -> crate::Result { if f.is_na() || f.is_nan() { Ok(i32::na()) } else if f.is_infinite() || !(I32MIN..=I32MAX).contains(&f) { - Err(format!("{f:?} is out of range for integer").into()) + Err(savvy_err!("{f:?} is out of range for integer")) } else if (f - f.round()).abs() > TOLERANCE { - Err(format!("{f:?} is not integer-ish").into()) + Err(savvy_err!("{f:?} is not integer-ish")) } else { Ok(f as i32) } @@ -42,19 +42,21 @@ fn cast_i32_to_f64(i: i32) -> f64 { fn try_cast_i32_to_usize(i: i32) -> crate::error::Result { if i.is_na() { - Err("cannot convert NA to usize".into()) + Err(savvy_err!("cannot convert NA to usize")) } else { - ::try_from(i).map_err(|e| e.to_string().into()) + Ok(::try_from(i)?) } } fn try_cast_f64_to_usize(f: f64) -> crate::Result { if f.is_na() || f.is_nan() { - Err("cannot convert NA or NaN to usize".into()) + Err(savvy_err!("cannot convert NA or NaN to usize")) } else if f.is_infinite() || !(0f64..=F64_MAX_CASTABLE_TO_USIZE).contains(&f) { - Err(format!("{f:?} is out of range that can be safely converted to usize").into()) + Err(savvy_err!( + "{f:?} is out of range that can be safely converted to usize" + )) } else if (f - f.round()).abs() > TOLERANCE { - Err(format!("{f:?} is not integer-ish").into()) + Err(savvy_err!("{f:?} is not integer-ish")) } else { Ok(f as usize) } diff --git a/src/sexp/utils.rs b/src/sexp/utils.rs index 53a400f6..f8d6709e 100644 --- a/src/sexp/utils.rs +++ b/src/sexp/utils.rs @@ -5,14 +5,13 @@ use std::{ use savvy_ffi::{cetype_t_CE_UTF8, Rf_mkCharLenCE, Rf_xlength, R_CHAR, SEXP}; -use crate::NotAvailableValue; +use crate::{savvy_err, NotAvailableValue}; pub(crate) fn assert_len(len: usize, i: usize) -> crate::error::Result<()> { if i >= len { - Err(crate::error::Error::new(&format!( - "index out of bounds: the length is {} but the index is {}", - len, i - ))) + Err(savvy_err!( + "index out of bounds: the length is {len} but the index is {i}", + )) } else { Ok(()) } @@ -59,10 +58,7 @@ pub(crate) fn str_to_symsxp>(name: T) -> crate::error::Result cstr, - Err(e) => return Err(crate::error::Error::new(&e.to_string())), - }; + let name_cstr = CString::new(name)?; let sym = unsafe { crate::unwind_protect(|| savvy_ffi::Rf_install(name_cstr.as_ptr())) }?; Ok(Some(sym))