Repository: extendr/extendr
Pull request #222 (2021-07-14 17:07:41)

Add option to use try from in extendr macro
This PR adds an option:

```
#[extendr(use_try_from = true)]
fn test_i32(val: i32) -> i32 {
    val
}
```

This is a prelude to supporting the try_from error handling formally.

It should have no effect on the main branch and so, we may merge this to enable others to contribute
to writing conversions.

We may need the failing conversion to give a useful error message by using to_string() on the returned
error.

We should then write integration tests to check these messages in R.

Note that Option<T> gives None if T is a NA value, not NULL.

There is another wrapper `Nullable` which handles NULLs.


brendanrbrown: looks nice -- i've commented on one test that snuck away this time, but adding it could be work for others as you mentioned in #219.

Patch:
diff --git a/extendr-api/src/error.rs b/extendr-api/src/error.rs
index a7020d3fa..9dc817806 100644
--- a/extendr-api/src/error.rs
+++ b/extendr-api/src/error.rs
@@ -1,25 +1,27 @@
 //! Error handling in Rust called from R.
 
-use crate::*;
-use libR_sys::*;
-use std::os::raw;
-
-static mut R_ERROR_BUF: Vec<u8> = Vec::new();
+use crate::{throw_r_error, Robj};
 
 /// Throw an R error if a result is an error.
 #[doc(hidden)]
 pub fn unwrap_or_throw<T>(r: std::result::Result<T, &'static str>) -> T {
-    unsafe {
-        match r {
-            Err(e) => {
-                R_ERROR_BUF.clear();
-                R_ERROR_BUF.extend(e.bytes());
-                R_ERROR_BUF.push(0);
-                Rf_error(R_ERROR_BUF.as_slice().as_ptr() as *mut raw::c_char);
-                unreachable!("");
-            }
-            Ok(v) => v,
+    match r {
+        Err(e) => {
+            throw_r_error(e.to_string());
+            unreachable!("");
+        }
+        Ok(v) => v,
+    }
+}
+
+#[doc(hidden)]
+pub fn unwrap_or_throw_error<T>(r: std::result::Result<T, Error>) -> T {
+    match r {
+        Err(e) => {
+            throw_r_error(e.to_string());
+            unreachable!("");
         }
+        Ok(v) => v,
     }
 }
 
diff --git a/extendr-api/src/robj/mod.rs b/extendr-api/src/robj/mod.rs
index 6ccd292e5..1317d385f 100644
--- a/extendr-api/src/robj/mod.rs
+++ b/extendr-api/src/robj/mod.rs
@@ -1,6 +1,6 @@
 //! R object handling.
 //!
-//! See. https://cran.r-project.org/doc/manuals/R-exts.html
+//! See. <https://cran.r-project.org/doc/manuals/R-exts.html>
 //!
 //! Fundamental principals:
 //!
diff --git a/extendr-api/src/robj/rinternals.rs b/extendr-api/src/robj/rinternals.rs
index 73a79be2f..90e8fb0f7 100644
--- a/extendr-api/src/robj/rinternals.rs
+++ b/extendr-api/src/robj/rinternals.rs
@@ -257,7 +257,7 @@ impl Robj {
     }
 
     /// Copy a vector and resize it.
-    /// See. https://github.com/hadley/r-internals/blob/master/vectors.md
+    /// See. <https://github.com/hadley/r-internals/blob/master/vectors.md>
     pub fn xlengthgets(&self, new_len: usize) -> Result<Robj> {
         unsafe {
             if self.is_vector() {
diff --git a/extendr-api/src/robj/try_from_robj.rs b/extendr-api/src/robj/try_from_robj.rs
index 8aa793c7b..7f280124e 100644
--- a/extendr-api/src/robj/try_from_robj.rs
+++ b/extendr-api/src/robj/try_from_robj.rs
@@ -158,7 +158,15 @@ macro_rules! impl_option {
     };
 }
 
+impl_option!(u8);
+impl_option!(u16);
+impl_option!(u32);
+impl_option!(u64);
+impl_option!(i8);
+impl_option!(i16);
 impl_option!(i32);
+impl_option!(i64);
+impl_option!(f32);
 impl_option!(f64);
 impl_option!(Bool);
 impl_option!(bool);
diff --git a/extendr-api/src/thread_safety.rs b/extendr-api/src/thread_safety.rs
index eafdecda1..f2b6b8c71 100644
--- a/extendr-api/src/thread_safety.rs
+++ b/extendr-api/src/thread_safety.rs
@@ -86,11 +86,13 @@ where
     }
 }
 
+static mut R_ERROR_BUF: Option<std::ffi::CString> = None;
+
 pub fn throw_r_error<S: AsRef<str>>(s: S) {
     let s = s.as_ref();
     unsafe {
-        let cstring = std::ffi::CString::new(s).unwrap();
-        libR_sys::Rf_error(cstring.as_ptr());
+        R_ERROR_BUF = Some(std::ffi::CString::new(s).unwrap());
+        libR_sys::Rf_error(R_ERROR_BUF.as_ref().unwrap().as_ptr());
         unreachable!("Rf_error does not return");
     };
 }
diff --git a/extendr-engine/src/lib.rs b/extendr-engine/src/lib.rs
index 067a6a18a..d841cca8b 100644
--- a/extendr-engine/src/lib.rs
+++ b/extendr-engine/src/lib.rs
@@ -4,7 +4,7 @@
 //!
 //! Its principal use is for testing.
 //!
-//! See https://github.com/wch/r-source/blob/trunk/src/unix/Rembedded.c
+//! See <https://github.com/wch/r-source/blob/trunk/src/unix/Rembedded.c>
 
 use libR_sys::*;
 use std::os::raw;
diff --git a/extendr-macros/src/extendr_function.rs b/extendr-macros/src/extendr_function.rs
index 68909c624..86c00367f 100644
--- a/extendr-macros/src/extendr_function.rs
+++ b/extendr-macros/src/extendr_function.rs
@@ -5,7 +5,7 @@ use syn::ItemFn;
 
 /// Generate bindings for a single function.
 pub fn extendr_function(args: Vec<syn::NestedMeta>, func: ItemFn) -> TokenStream {
-    let mut opts = wrappers::ExtendrOptions {};
+    let mut opts = wrappers::ExtendrOptions::default();
 
     for arg in &args {
         parse_options(&mut opts, arg);
@@ -22,16 +22,23 @@ pub fn extendr_function(args: Vec<syn::NestedMeta>, func: ItemFn) -> TokenStream
 }
 
 /// Parse a set of attribute arguments for #[extendr(opts...)]
-pub fn parse_options(_opts: &mut wrappers::ExtendrOptions, _arg: &syn::NestedMeta) {
-    /*use syn::{Lit, Meta, MetaNameValue, NestedMeta};
+pub fn parse_options(opts: &mut wrappers::ExtendrOptions, arg: &syn::NestedMeta) {
+    use syn::{Lit, LitBool, Meta, MetaNameValue, NestedMeta};
 
     match arg {
         NestedMeta::Meta(Meta::NameValue(MetaNameValue {
             ref path,
             eq_token: _,
-            lit: Lit::Str(ref _lit_str),
+            lit,
         })) => {
+            if path.is_ident("use_try_from") {
+                if let Lit::Bool(LitBool { value, .. }) = lit {
+                    opts.use_try_from = *value;
+                }
+            } else {
+                panic!("expected use_try_from");
+            }
         }
         _ => panic!("expected #[extendr(opt = \"string\", ...)]"),
-    }*/
+    }
 }
diff --git a/extendr-macros/src/extendr_impl.rs b/extendr-macros/src/extendr_impl.rs
index 73f244704..52604aa19 100644
--- a/extendr-macros/src/extendr_impl.rs
+++ b/extendr-macros/src/extendr_impl.rs
@@ -62,7 +62,7 @@ pub fn extendr_impl(mut item_impl: ItemImpl) -> TokenStream {
         return quote! { compile_error!("where clause not allowed in #[extendr] impl"); }.into();
     }
 
-    let opts = wrappers::ExtendrOptions {};
+    let opts = wrappers::ExtendrOptions::default();
     let self_ty = item_impl.self_ty.as_ref();
     let self_ty_name = wrappers::type_name(self_ty);
     let prefix = format!("{}__", self_ty_name);
diff --git a/extendr-macros/src/wrappers.rs b/extendr-macros/src/wrappers.rs
index de184055c..5db064be3 100644
--- a/extendr-macros/src/wrappers.rs
+++ b/extendr-macros/src/wrappers.rs
@@ -5,11 +5,21 @@ pub const META_PREFIX: &str = "meta__";
 pub const WRAP_PREFIX: &str = "wrap__";
 
 #[derive(Debug)]
-pub struct ExtendrOptions {}
+pub struct ExtendrOptions {
+    pub use_try_from: bool,
+}
+
+impl Default for ExtendrOptions {
+    fn default() -> Self {
+        ExtendrOptions {
+            use_try_from: false,
+        }
+    }
+}
 
 // Generate wrappers for a specific function.
 pub fn make_function_wrappers(
-    _opts: &ExtendrOptions,
+    opts: &ExtendrOptions,
     wrappers: &mut Vec<ItemFn>,
     prefix: &str,
     attrs: &[syn::Attribute],
@@ -66,7 +76,7 @@ pub fn make_function_wrappers(
 
     let actual_args: Punctuated<Expr, Token![,]> = inputs
         .iter()
-        .filter_map(|input| translate_actual(input))
+        .filter_map(|input| translate_actual(opts, input))
         .collect();
 
     let meta_args: Vec<Expr> = inputs
@@ -267,14 +277,18 @@ fn translate_to_robj(input: &FnArg) -> syn::Stmt {
 }
 
 // Generate actual argument list for the call (ie. a list of conversions).
-fn translate_actual(input: &FnArg) -> Option<Expr> {
+fn translate_actual(opts: &ExtendrOptions, input: &FnArg) -> Option<Expr> {
     match input {
         FnArg::Typed(ref pattype) => {
             let pat = &pattype.pat.as_ref();
             let ty = &pattype.ty.as_ref();
             if let syn::Pat::Ident(ref ident) = pat {
                 let varname = format_ident!("_{}_robj", ident.ident);
-                Some(parse_quote! { extendr_api::unwrap_or_throw(<#ty>::from_robj(&#varname)) })
+                if opts.use_try_from {
+                    Some(parse_quote! { extendr_api::unwrap_or_throw_error(#varname.try_into()) })
+                } else {
+                    Some(parse_quote! { extendr_api::unwrap_or_throw(<#ty>::from_robj(&#varname)) })
+                }
             } else {
                 None
             }


Test patch:
diff --git a/extendr-api/tests/extendr_macro.rs b/extendr-api/tests/extendr_macro.rs
new file mode 100644
index 000000000..828ce7f2f
--- /dev/null
+++ b/extendr-api/tests/extendr_macro.rs
@@ -0,0 +1,108 @@
+use extendr_api::prelude::*;
+
+#[extendr(use_try_from = true)]
+fn test_i32(val: i32) -> i32 {
+    val
+}
+
+#[extendr(use_try_from = true)]
+fn test_i16(val: i16) -> i16 {
+    val
+}
+
+#[extendr(use_try_from = true)]
+fn test_option_i32(val: Option<i32>) -> i32 {
+    if let Some(i) = val {
+        i
+    } else {
+        -1
+    }
+}
+
+#[extendr(use_try_from = true)]
+fn test_option_f64(val: Option<f64>) -> f64 {
+    if let Some(i) = val {
+        i
+    } else {
+        -1.0
+    }
+}
+
+#[extendr(use_try_from = true)]
+fn test_option_i16(val: Option<i16>) -> i16 {
+    if let Some(i) = val {
+        i
+    } else {
+        -1
+    }
+}
+
+#[test]
+fn tests_with_successful_outcomes() {
+    unsafe {
+        test! {
+            // Matching integer.
+            assert_eq!(new_owned(wrap__test_i32(r!(1).get())), r!(1));
+
+            // i32 takes any numeric.
+            assert_eq!(new_owned(wrap__test_i32(r!(1.0).get())), r!(1));
+
+
+            // Matching integer.
+            assert_eq!(new_owned(wrap__test_option_i32(r!(1).get())), r!(1));
+
+            // Option<i32> takes any numeric.
+            assert_eq!(new_owned(wrap__test_option_i32(r!(1.0).get())), r!(1));
+
+            // NA input.
+            assert_eq!(new_owned(wrap__test_option_i32(r!(NA_REAL).get())), r!(-1));
+
+            // NA input.
+            assert_eq!(new_owned(wrap__test_option_i32(r!(NA_INTEGER).get())), r!(-1));
+
+
+            // Matching integer.
+            assert_eq!(new_owned(wrap__test_option_i16(r!(1).get())), r!(1));
+
+            // Option<i16> takes any numeric.
+            assert_eq!(new_owned(wrap__test_option_i16(r!(1.0).get())), r!(1));
+
+            // NA input.
+            assert_eq!(new_owned(wrap__test_option_i16(r!(NA_REAL).get())), r!(-1));
+
+            // NA input.
+            assert_eq!(new_owned(wrap__test_option_i16(r!(NA_INTEGER).get())), r!(-1));
+
+
+            // Matching integer.
+            assert_eq!(new_owned(wrap__test_option_f64(r!(1).get())), r!(1.0));
+
+            // Option<f64> takes any numeric.
+            assert_eq!(new_owned(wrap__test_option_f64(r!(1.0).get())), r!(1.0));
+
+            // NA input.
+            assert_eq!(new_owned(wrap__test_option_f64(r!(NA_REAL).get())), r!(-1.0));
+
+            // NA input.
+            assert_eq!(new_owned(wrap__test_option_f64(r!(NA_INTEGER).get())), r!(-1.0));
+        }
+    }
+}
+
+// Win32 does not support catch_unwind.
+#[cfg(not(target_arch = "x86"))]
+#[test]
+fn tests_with_unsuccessful_outcomes() {
+    unsafe {
+        test! {
+            // These should throw R errors.
+            // They may cause stack traces, but this is harmless.
+            assert!(catch_r_error(|| wrap__test_i32(r!("xyz").get())).is_err());
+            assert!(catch_r_error(|| wrap__test_i32(r!(pairlist!(x=1)).get())).is_err());
+            assert!(catch_r_error(|| wrap__test_i32(r!(list!(1, 2, 3)).get())).is_err());
+
+            // TODO: check for overflow.
+            // assert!(catch_r_error(|| wrap__test_i16(r!(1234567890).get())).is_err());
+        }
+    }
+}