Repository: extendr/extendr
Pull request #488 (2023-02-24 21:23:54)

Add `use_rng` option to `extendr`
Restarted #476 PR as a rebase went really badly. 

multimeric: I think this PR is good, but we maybe need to think about whether it needs to live in the `extendr` macro or not. It seems like RNG has a "scope" (I don't really understand why, but I'm sure you do). So maybe `with_rng!{}` could be a macro that the user can put inside their extendr function? I'm a bit wary of overloading the `extendr` macro with things that don't relate to the wrapped function itself or the way in which it is converted to R.
CGMossa: I think it should live in `#[extendr]`. 

Honestly, it should be a feature in ExtendR from day one;

In principle, I think we could also provide a macro that provides this, but panics would result in lack of `PutRNGState` call to back to R. Which would be very bad. So I'd be doing the same capturing that is done now for extendr-binded-functions for this macro. Unless the `try!`-block feature is stable one day, it could be a mixture of two.
CGMossa: `#[extendr]` is a little overloaded right now. It will loose an argument soon in `use_try_from`.
But I still vote that the rng-thing stays in a prominent place.
multimeric: Oh okay, are you saying that you need a finalizer that runs in case of errors? And that we can't separate this from the function-level error handler because [`try-blocks`](https://doc.rust-lang.org/unstable-book/language-features/try-blocks.html) is still unstable? I agree that supporting the `?` operator is imperative. The only way to do it as I suggest would be to pull in a macro from another crate: https://docs.rs/tryvial/latest/tryvial/macro.try_block.html. I guess editing the `extendr` macro is the best solution at the moment, but I'd like to see what @Ilia-Kosenkov thinks.
CGMossa: I don't know what I'm saying, sorry I'm still under the weather.
The problem is that no matter what happens on the Rust-side, the `PutRngState` has to be called.
The way we are doing it right now by capturing panics, makes it fit with this. 

Making a macro-block would have to handle panics as well; But a first step would be just to support errors, i.e.
put rng state back even when an error is encountered. And that I don't even know how to do as a macro.
Besides the macro would be verbose, and I would personally hate to work within a macro;

Ilia-Kosenkov: I need more information on this. Can we enable this globally? What's the overhead for calling it for each method (an equivalent of having `use_rng = true` all the time)? 
CGMossa: This shouldn't be enabled globally.
I think there are substantial performance costs, that we wouldn't be nice to spur on users.
Rcpp does it this way:
```cpp
// [[Rcpp::export(rng = false)]]
double myFunction(double input) {
// ...code that never uses the
// R random number generation...
}
```
[source](https://cran.r-project.org/web/packages/Rcpp/vignettes/Rcpp-attributes.pdf)

Frankly, I think it should be opt-in, and not opt-out, but any agreement we make here is fair by me.
Ilia-Kosenkov: Well, we could go the Rcpp way for sure (but I agree with opt-in).
multimeric: Isn't the way that Mossa proposed exactly the same as Rcpp?
CGMossa: @multimeric the review on this one is also waiting to be resolved.
I think this is as done as can be. 
multimeric: I would kind of like some reassurance that this correctly handles a panic, but I can see that would be difficult. At least the macro compiles correctly.
Ilia-Kosenkov: This one has a lot of merge conflicts, @CGMossa can you resolve it & then merge?
CGMossa: This is a little less trivial to merge now. 
CGMossa: Apologies. I've merged, and changed a lot of stuff, and now I would appreciate another review on this, just to make sure that it isn't completely insane.
CGMossa: Ehh.. I checked it several times and did several merges... Can you tell me what you see that is wrong please?


CGMossa: ```
kevin@pop-os:~/GitHub/extendr$ git merge origin/master
Already up to date.
```

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index e9a75874e..94350db8c 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,8 @@
 
 - [**either**] `TryFrom<&Robj> for Either<T, R>` and `From<Either<T, R>> for Robj` if `T` and `R` are themselves implement these traits. This unblocks scenarios like accepting any numeric vector from R via `Either<Integers, Doubles>` without extra memory allocation [[#480]](https://github.com/extendr/extendr/pull/480)
 - `PartialOrd` trait implementation for `Rfloat`, `Rint` and `Rbool`. `Rfloat` and `Rint` gained `min()` and `max()` methods [[#573]](https://github.com/extendr/extendr/pull/573)
+- Added `use_rng` option for the `extendr` attribute macro, which enables the use of
+random number sampling methods from R, e.g. `#[extendr(use_rng = true)`. [[#476]](https://github.com/extendr/extendr/pull/476)
 
 ### Fixed
 
diff --git a/Cargo.toml b/Cargo.toml
index 6a2829eb1..6c5bda2d9 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -1,11 +1,10 @@
 [workspace]
 resolver = "2"
 
-members = [
-	"extendr-api",
-	"extendr-engine",
-	"extendr-macros",
-]
+members = ["extendr-api", "extendr-engine", "extendr-macros"]
+
+[workspace.dependencies]
+libR-sys = "0.6.0"
 
 [patch.crates-io]
 # When uncommenting this, do not forget to uncomment the same line in
diff --git a/extendr-api/Cargo.toml b/extendr-api/Cargo.toml
index 00bc186d2..c6ddf54dc 100644
--- a/extendr-api/Cargo.toml
+++ b/extendr-api/Cargo.toml
@@ -17,12 +17,11 @@ repository = "https://github.com/extendr/extendr"
 rust-version = "1.60"
 
 [dependencies]
-libR-sys = "0.6.0"
-extendr-macros = { path = "../extendr-macros", version = "0.4.0" }
-extendr-engine = { path = "../extendr-engine", version = "0.4.0" }
+libR-sys = { workspace = true }
+extendr-macros = { path = "../extendr-macros", version="0.4.0" }
+extendr-engine = { path = "../extendr-engine", version="0.4.0" }
 lazy_static = "1.4"
 paste = "1.0.5"
-
 either = { version = "1.8.1", optional = true }
 libc = { version = "0.2", optional = true }
 ndarray = { version = "0.15.3", optional = true }
diff --git a/extendr-api/src/robj/mod.rs b/extendr-api/src/robj/mod.rs
index d9c82f875..1ca621409 100644
--- a/extendr-api/src/robj/mod.rs
+++ b/extendr-api/src/robj/mod.rs
@@ -845,7 +845,6 @@ pub trait Attributes: Types + Length {
     /// ```
     /// use extendr_api::prelude::*;
     /// test! {
-    ///
     ///    let mut robj = r!("hello").set_attrib(sym!(xyz), 1)?;
     ///    assert_eq!(robj.get_attrib(sym!(xyz)), Some(r!(1)));
     /// }
diff --git a/extendr-api/src/wrapper/altrep.rs b/extendr-api/src/wrapper/altrep.rs
index 49ff3e026..66988eb0e 100644
--- a/extendr-api/src/wrapper/altrep.rs
+++ b/extendr-api/src/wrapper/altrep.rs
@@ -496,9 +496,9 @@ impl Altrep {
             }
 
             let ptr: *mut StateType = Box::into_raw(Box::new(state));
-            let tag = r!(());
-            let prot = r!(());
-            let state = R_MakeExternalPtr(ptr as *mut c_void, tag.get(), prot.get());
+            let tag = R_NilValue;
+            let prot = R_NilValue;
+            let state = R_MakeExternalPtr(ptr as *mut c_void, tag, prot);
             R_RegisterCFinalizer(state, Some(finalizer::<StateType>));
 
             let class_ptr = R_altrep_class_t { ptr: class.get() };
diff --git a/extendr-api/src/wrapper/externalptr.rs b/extendr-api/src/wrapper/externalptr.rs
index 619bb29ee..b45e42a50 100644
--- a/extendr-api/src/wrapper/externalptr.rs
+++ b/extendr-api/src/wrapper/externalptr.rs
@@ -94,9 +94,15 @@ impl<T: Any + Debug> ExternalPtr<T> {
                 unsafe {
                     let ptr = R_ExternalPtrAddr(x) as *mut T;
 
+                    // Free the `tag`, which is the type-name
+                    R_SetExternalPtrTag(x, R_NilValue);
+
                     // Convert the pointer to a box and drop it implictly.
                     // This frees up the memory we have used and calls the "T::drop" method if there is one.
                     drop(Box::from_raw(ptr));
+
+                    // Now set the pointer in ExternalPTR to C `NULL`
+                    R_ClearExternalPtr(x);
                 }
             }
 
diff --git a/extendr-macros/Cargo.toml b/extendr-macros/Cargo.toml
index 466146302..cfb901a9c 100644
--- a/extendr-macros/Cargo.toml
+++ b/extendr-macros/Cargo.toml
@@ -7,7 +7,7 @@ authors = [
     "Mossa Merhi Reimert <mossa@sund.ku.dk>",
     "Claus O. Wilke <wilke@austin.utexas.edu>",
     "Hiroaki Yutani",
-    "Ilia A. Kosenkov <ilia.kosenkov@outlook.com>"
+    "Ilia A. Kosenkov <ilia.kosenkov@outlook.com>",
 ]
 edition = "2021"
 description = "Generate bindings from R to Rust."
@@ -25,3 +25,4 @@ proc-macro2 = { version = "1.0" }
 [dev-dependencies]
 extendr-api = { path = "../extendr-api" }
 extendr-engine = { path = "../extendr-engine" }
+libR-sys = { workspace = true }
diff --git a/extendr-macros/src/extendr_function.rs b/extendr-macros/src/extendr_function.rs
index 25ba3e0ab..2dd5c3775 100644
--- a/extendr-macros/src/extendr_function.rs
+++ b/extendr-macros/src/extendr_function.rs
@@ -10,7 +10,6 @@ pub fn extendr_function(args: Vec<syn::NestedMeta>, mut func: ItemFn) -> TokenSt
     for arg in &args {
         parse_options(&mut opts, arg);
     }
-
     let mut wrappers: Vec<ItemFn> = Vec::new();
     wrappers::make_function_wrappers(&opts, &mut wrappers, "", &func.attrs, &mut func.sig, None);
 
@@ -21,12 +20,19 @@ pub fn extendr_function(args: Vec<syn::NestedMeta>, mut func: ItemFn) -> TokenSt
     })
 }
 
-/// Parse a set of attribute arguments for #[extendr(opts...)]
+/// Parse a set of attribute arguments for `#[extendr(opts...)]`
+///
+/// Supported options:
+///
+/// - `use_try_from = bool` which uses `TryFrom<Robj>` for argument conversions.
+/// - `r_name = "name"` which specifies the name of the wrapper on the R-side.
+/// - `use_rng = bool` ensures the RNG-state is pulled and pushed
+///
 pub fn parse_options(opts: &mut wrappers::ExtendrOptions, arg: &syn::NestedMeta) {
     use syn::{Lit, LitBool, Meta, MetaNameValue, NestedMeta};
 
     fn help_message() -> ! {
-        panic!("expected #[extendr(use_try_from=bool, r_name=\"name\")]");
+        panic!("expected #[extendr(use_try_from = bool, r_name = \"name\", mod_name = \"r_mod_name\", use_rng = bool)]");
     }
 
     match arg {
@@ -53,6 +59,12 @@ pub fn parse_options(opts: &mut wrappers::ExtendrOptions, arg: &syn::NestedMeta)
                 } else {
                     help_message();
                 }
+            } else if path.is_ident("use_rng") {
+                if let Lit::Bool(LitBool { value, .. }) = lit {
+                    opts.use_rng = *value;
+                } else {
+                    help_message();
+                }
             } else {
                 help_message();
             }
diff --git a/extendr-macros/src/wrappers.rs b/extendr-macros/src/wrappers.rs
index bc7576615..a295fe902 100644
--- a/extendr-macros/src/wrappers.rs
+++ b/extendr-macros/src/wrappers.rs
@@ -10,6 +10,7 @@ pub struct ExtendrOptions {
     pub use_try_from: bool,
     pub r_name: Option<String>,
     pub mod_name: Option<String>,
+    pub use_rng: bool,
 }
 
 // Generate wrappers for a specific function.
@@ -100,12 +101,31 @@ pub fn make_function_wrappers(
     //     }
     // }
     // ```
-    //
+    let rng_start = opts
+        .use_rng
+        .then(|| {
+            quote!(unsafe {
+                libR_sys::GetRNGstate();
+            })
+        })
+        .unwrap_or_default();
+    let rng_end = opts
+        .use_rng
+        .then(|| {
+            quote!(unsafe {
+                libR_sys::PutRNGstate();
+            })
+        })
+        .unwrap_or_default();
     wrappers.push(parse_quote!(
         #[no_mangle]
         #[allow(non_snake_case, clippy::not_unsafe_ptr_arg_deref)]
         pub extern "C" fn #wrap_name(#formal_args) -> extendr_api::SEXP {
             use extendr_api::robj::*;
+
+            // pull RNG state before evaluation
+            #rng_start
+
             let wrap_result_state: std::result::Result<
                 std::result::Result<Robj, extendr_api::Error>,
                 Box<dyn std::any::Any + Send>
@@ -115,9 +135,11 @@ pub fn make_function_wrappers(
                     Ok(extendr_api::Robj::from(#call_name(#actual_args)))
                 })
             };
-            // any obj created in above unsafe scope, which are not moved into wrap_result_state are now dropped
 
+            // return RNG state back to r after evaluation
+            #rng_end
 
+            // any obj created in above unsafe scope, which are not moved into wrap_result_state are now dropped
             match wrap_result_state {
                 Ok(Ok(zz)) => {
                     return unsafe { zz.get() };


Test patch:
diff --git a/extendr-api/tests/rng_tests.rs b/extendr-api/tests/rng_tests.rs
new file mode 100644
index 000000000..8cf2f15cd
--- /dev/null
+++ b/extendr-api/tests/rng_tests.rs
@@ -0,0 +1,40 @@
+//! Tests the ability to use R to generate random numbers.
+//!
+//! The `extendr` attribute macro now has a `use_rng` argument.
+//! If this is used, then a `GetRNGstate()` is invoked before the function is called,
+//! and a `PutRNGstate()` is invoked after, even if said method panics.
+//!
+//! These tests has to check two things:
+//!
+//! * That the seed is set and affects the sampling functions on the rust-side
+//! in the same way, as on the r-side.
+//! * That the resulting sampled vector is not equal 0, as it would be that
+//! if `GetRNGstate()` is not called prior to sampling.
+//!
+//!
+use extendr_api::prelude::*;
+
+#[extendr(use_rng = true)]
+fn generate_big_random_vec() -> Vec<f64> {
+    let n = 100;
+    let param = 10e3;
+    (0..n)
+        .map(|_| unsafe { libR_sys::R_unif_index(param) })
+        .collect()
+}
+
+#[test]
+fn test_extendr_rng() {
+    test! {
+        R!(r#"set.seed(20230205)"#).unwrap();
+        let x = generate_big_random_vec();
+        R!(r#"set.seed(20230205)"#).unwrap();
+        let y = generate_big_random_vec();
+        assert_eq!(y, x);
+        // if the rng state isn't retrieved, then distribution is
+        // 0 always
+        assert!(
+            !x.iter().all(|&x| x == 0f64)
+        );
+    }
+}