Skip to content

Commit

Permalink
#[extendr]-impl: Return the right reference (extendr#726)
Browse files Browse the repository at this point in the history
* extendrtests: added an integration test

* add test where other is of type `&Self`. why not?

* use `cast` instead of `as`

* added a (private) symbol from libR-sys necessary in
`#[extendr]`-impl.

* Apply suggestions from code review

Co-authored-by: Ilia Kosenkov <ilia.kosenkov.at.gm@gmail.com>

* split off externalptr tests instead of submodule

---------

Co-authored-by: Ilia Kosenkov <ilia.kosenkov.at.gm@gmail.com>
  • Loading branch information
CGMossa and Ilia-Kosenkov authored May 27, 2024
1 parent 13e4e4b commit 9d548d3
Show file tree
Hide file tree
Showing 15 changed files with 2,442 additions and 1,429 deletions.
4 changes: 4 additions & 0 deletions extendr-api/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ pub enum Error {

ExpectedExternalPtrType(Robj, String),
ExpectedExternalNonNullPtr(Robj),
ExpectedExternalPtrReference,
Other(String),

#[cfg(feature = "ndarray")]
Expand Down Expand Up @@ -163,6 +164,9 @@ impl std::fmt::Display for Error {
robj
)
}
Error::ExpectedExternalPtrReference => {
write!(f, "It is only possible to return a reference to self.")
}
Error::NoGraphicsDevices(_robj) => write!(f, "No graphics devices active."),
Error::Other(str) => write!(f, "{}", str),

Expand Down
5 changes: 5 additions & 0 deletions extendr-api/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -422,9 +422,14 @@ pub const NA_LOGICAL: Rbool = Rbool::na_value();
#[doc(hidden)]
pub use std::collections::HashMap;

/// This is needed for the generation of wrappers.
#[doc(hidden)]
pub use libR_sys::DllInfo;

/// This is necessary for `#[extendr]`-impl
#[doc(hidden)]
pub use libR_sys::R_ExternalPtrAddr;

#[doc(hidden)]
pub use libR_sys::SEXP;

Expand Down
5 changes: 2 additions & 3 deletions extendr-api/src/robj/rinternals.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use crate::*;
use std::os::raw;

///////////////////////////////////////////////////////////////
/// The following impls wrap specific Rinternals.h functions.
Expand Down Expand Up @@ -67,7 +66,7 @@ pub trait Rinternals: Types + Conversions {

/// Get the source ref.
fn get_current_srcref(val: i32) -> Robj {
unsafe { Robj::from_sexp(R_GetCurrentSrcref(val as raw::c_int)) }
unsafe { Robj::from_sexp(R_GetCurrentSrcref(val as std::ffi::c_int)) }
}

/// Get the source filename.
Expand Down Expand Up @@ -253,7 +252,7 @@ pub trait Rinternals: Types + Conversions {
/// Internal function used to implement `#[extendr]` impl
#[doc(hidden)]
unsafe fn external_ptr_addr<T>(&self) -> *mut T {
R_ExternalPtrAddr(self.get()) as *mut T
R_ExternalPtrAddr(self.get()).cast()
}

/// Internal function used to implement `#[extendr]` impl
Expand Down
2 changes: 1 addition & 1 deletion extendr-api/src/wrapper/externalptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ impl<T> ExternalPtr<T> {

extern "C" fn finalizer<T>(x: SEXP) {
unsafe {
let ptr = R_ExternalPtrAddr(x) as *mut T;
let ptr = R_ExternalPtrAddr(x).cast::<T>();

// Free the `tag`, which is the type-name
R_SetExternalPtrTag(x, R_NilValue);
Expand Down
32 changes: 28 additions & 4 deletions extendr-macros/src/wrappers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,11 +91,27 @@ pub(crate) fn make_function_wrappers(
quote! { #rust_name }
};

// arguments for the wrapper with type being `SEXP`
let formal_args = inputs
.iter()
.map(|input| translate_formal(input, self_ty))
.collect::<syn::Result<Punctuated<FnArg, Token![,]>>>()?;

// extract the names of the arguments only (`mut` are ignored in `formal_args` already)
let sexp_args = formal_args
.clone()
.into_iter()
.map(|x| match x {
// the wrapper doesn't use `self` arguments
FnArg::Receiver(_) => unreachable!(),
FnArg::Typed(ref typed) => match typed.pat.as_ref() {
syn::Pat::Ident(ref pat_ident) => pat_ident.ident.clone(),
_ => unreachable!(),
},
})
.collect::<Vec<Ident>>();

// arguments from R (`SEXP`s) are converted to `Robj`
let convert_args: Vec<syn::Stmt> = inputs
.iter()
.map(translate_to_robj)
Expand Down Expand Up @@ -170,9 +186,16 @@ pub(crate) fn make_function_wrappers(
// instead of converting &Self / &mut Self, pass on the passed
// ExternalPtr<Self>
quote!(
let _return_ref_to_self = #call_name(#actual_args);
//FIXME: find a less hardcoded way to write `_self_robj`
Ok(_self_robj)
let return_ref_to_self = #call_name(#actual_args);

#(
if std::ptr::addr_eq(
extendr_api::R_ExternalPtrAddr(#sexp_args),
std::ptr::from_ref(return_ref_to_self)) {
return Ok(extendr_api::Robj::from_sexp(#sexp_args))
}
)*
Err(Error::ExpectedExternalPtrReference)
)
} else {
quote!(Ok(extendr_api::Robj::from(#call_name(#actual_args))))
Expand Down Expand Up @@ -219,7 +242,7 @@ pub(crate) fn make_function_wrappers(
// included in panic. The advantage would be the panic cause could be included
// in the R terminal error message and not only via std-err.
// but it should be handled in a separate function and not in-lined here.
let err_string = format!("user function panicked: {}",#r_name_str);
let err_string = format!("User function panicked: {}", #r_name_str);
// cannot use throw_r_error here for some reason.
// handle_panic() exports err string differently than throw_r_error.
extendr_api::handle_panic(err_string.as_str(), || panic!());
Expand Down Expand Up @@ -445,6 +468,7 @@ fn translate_to_robj(input: &FnArg) -> syn::Result<syn::Stmt> {
}
}
FnArg::Receiver(_) => {
// this is `mut`, in case of a mutable reference
Ok(parse_quote! { let mut _self_robj = extendr_api::robj::Robj::from_sexp(_self); })
}
}
Expand Down
3 changes: 3 additions & 0 deletions tests/extendrtests/NAMESPACE
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,16 @@ S3method("$","__MyClass")
S3method("$",MyClass)
S3method("$",MyClassUnexported)
S3method("$",MySubmoduleClass)
S3method("$",Wrapper)
S3method("[[","__MyClass")
S3method("[[",MyClass)
S3method("[[",MyClassUnexported)
S3method("[[",MySubmoduleClass)
S3method("[[",Wrapper)
export("__00__special_function_name")
export(MyClass)
export(MySubmoduleClass)
export(Wrapper)
export(euclidean_dist)
export(false)
export(hello_submodule)
Expand Down
45 changes: 37 additions & 8 deletions tests/extendrtests/R/extendr-wrappers.R
Original file line number Diff line number Diff line change
Expand Up @@ -214,23 +214,52 @@ MySubmoduleClass$set_a <- function(x) invisible(.Call(wrap__MySubmoduleClass__se

MySubmoduleClass$a <- function() .Call(wrap__MySubmoduleClass__a, self)

MySubmoduleClass$me_owned <- function() .Call(wrap__MySubmoduleClass__me_owned, self)
#' @rdname MySubmoduleClass
#' @usage NULL
#' @export
`$.MySubmoduleClass` <- function (self, name) { func <- MySubmoduleClass[[name]]; environment(func) <- environment(); func }

#' @export
`[[.MySubmoduleClass` <- `$.MySubmoduleClass`

#' Class for testing (exported)
#' @examples
#' x <- Wrapper$new()
#' x$a()
#' x$set_a(10)
#' x$a()
#' @export
Wrapper <- new.env(parent = emptyenv())

MySubmoduleClass$me_ref <- function() .Call(wrap__MySubmoduleClass__me_ref, self)
Wrapper$new <- function() .Call(wrap__Wrapper__new)

MySubmoduleClass$me_mut <- function() .Call(wrap__MySubmoduleClass__me_mut, self)
Wrapper$set_a <- function(x) invisible(.Call(wrap__Wrapper__set_a, self, x))

MySubmoduleClass$me_explicit_ref <- function() .Call(wrap__MySubmoduleClass__me_explicit_ref, self)
Wrapper$a <- function() .Call(wrap__Wrapper__a, self)

MySubmoduleClass$me_explicit_mut <- function() .Call(wrap__MySubmoduleClass__me_explicit_mut, self)
Wrapper$me_owned <- function() .Call(wrap__Wrapper__me_owned, self)

#' @rdname MySubmoduleClass
Wrapper$me_ref <- function() .Call(wrap__Wrapper__me_ref, self)

Wrapper$me_mut <- function() .Call(wrap__Wrapper__me_mut, self)

Wrapper$me_explicit_ref <- function() .Call(wrap__Wrapper__me_explicit_ref, self)

Wrapper$me_explicit_mut <- function() .Call(wrap__Wrapper__me_explicit_mut, self)

Wrapper$max_ref <- function(other) .Call(wrap__Wrapper__max_ref, self, other)

Wrapper$max_ref_offset <- function(other, `_offset`) .Call(wrap__Wrapper__max_ref_offset, self, other, `_offset`)

Wrapper$max_ref2 <- function(other) .Call(wrap__Wrapper__max_ref2, self, other)

#' @rdname Wrapper
#' @usage NULL
#' @export
`$.MySubmoduleClass` <- function (self, name) { func <- MySubmoduleClass[[name]]; environment(func) <- environment(); func }
`$.Wrapper` <- function (self, name) { func <- Wrapper[[name]]; environment(func) <- environment(); func }

#' @export
`[[.MySubmoduleClass` <- `$.MySubmoduleClass`
`[[.Wrapper` <- `$.Wrapper`


# nolint end
2 changes: 1 addition & 1 deletion tests/extendrtests/man/MySubmoduleClass.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

23 changes: 23 additions & 0 deletions tests/extendrtests/man/Wrapper.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

96 changes: 96 additions & 0 deletions tests/extendrtests/src/rust/src/externalptr.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
use extendr_api::prelude::*;

// Class for testing
#[derive(Default, Debug, Clone, Copy)]
struct Wrapper {
a: i32,
}

/// Class for testing (exported)
/// @examples
/// x <- Wrapper$new()
/// x$a()
/// x$set_a(10)
/// x$a()
/// @export
#[extendr]
impl Wrapper {
/// Method for making a new object.
fn new() -> Self {
Self { a: 0 }
}

/// Method for setting stuff.
/// @param x a number
fn set_a(&mut self, x: i32) {
self.a = x;
}

/// Method for getting stuff.
fn a(&self) -> i32 {
self.a
}

// NOTE: Cannot move ownership, as that concept is incompatible with bridging
// data from R to Rust
// fn myself(self) -> Self {
// self
// }

/// Method for getting one's (by way of a copy) self.
fn me_owned(&self) -> Self {
// only possible due to `derive(Clone, Copy)`
*self
}

/// Method for getting one's (ref) self.
fn me_ref(&self) -> &Self {
self
}

/// Method for getting one's (ref mut) self.
fn me_mut(&mut self) -> &mut Self {
self
}

/// Method for getting one's ref (explicit) self.
fn me_explicit_ref(&self) -> &Wrapper {
self
}

/// Method for getting one's ref mut (explicit) self.
fn me_explicit_mut(&mut self) -> &mut Wrapper {
self
}

fn max_ref(&self, other: &'static Wrapper) -> &Self {
if self.a > other.a {
self
} else {
other
}
}

/// `offset` does nothing.
fn max_ref_offset(&self, other: &'static Wrapper, _offset: i32) -> &Self {
if self.a > other.a {
self
} else {
other
}
}

fn max_ref2(&self, other: &'static Self) -> &Self {
if self.a > other.a {
self
} else {
other
}
}
}

// Macro to generate exports
extendr_module! {
mod externalptr;
impl Wrapper;
}
3 changes: 2 additions & 1 deletion tests/extendrtests/src/rust/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use extendr_api::{graphics::*, prelude::*};
mod altrep;
mod attributes;
mod dataframe;
mod externalptr;
mod graphic_device;
mod memory_leaks;
mod optional_either;
Expand Down Expand Up @@ -350,5 +351,5 @@ extendr_module! {
use optional_faer;
use raw_identifiers;
use submodule;

use externalptr;
}
32 changes: 0 additions & 32 deletions tests/extendrtests/src/rust/src/submodule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,38 +37,6 @@ impl MySubmoduleClass {
fn a(&self) -> i32 {
self.a
}

// NOTE: Cannot move ownership, as that concept is incompatible with bridging
// data from R to Rust
// fn myself(self) -> Self {
// self
// }

/// Method for getting one's (by way of a copy) self.
fn me_owned(&self) -> Self {
// only possible due to `derive(Clone, Copy)`
*self
}

/// Method for getting one's (ref) self.
fn me_ref(&self) -> &Self {
self
}

/// Method for getting one's (ref mut) self.
fn me_mut(&mut self) -> &mut Self {
self
}

/// Method for getting one's ref (explicit) self.
fn me_explicit_ref(&self) -> &MySubmoduleClass {
self
}

/// Method for getting one's ref mut (explicit) self.
fn me_explicit_mut(&mut self) -> &mut MySubmoduleClass {
self
}
}

// Macro to generate exports
Expand Down
Loading

0 comments on commit 9d548d3

Please sign in to comment.