Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generalize the possibility of putting struct or enum in the input or output argument #739

Open
Yunuuuu opened this issue Apr 15, 2024 · 5 comments

Comments

@Yunuuuu
Copy link

Yunuuuu commented Apr 15, 2024

In current implement, we can only put Struct or Enum output in impl but not fn (except we also use extendr to export the impl of the same Struct or Enum).

Hope to add something like this, in this way, we can just put [derive(Sexp)] followed by the creation of Struct or Enum, we can then put the object in function input or output. Following code is copied directly from extendr, it would be better to allow users control which object (struct or enum) can be put in the input argument or output argument (Current implement only allow output or input struct or enum whose impl has been exported by extendr).

use proc_macro::TokenStream;
use quote::{quote, ToTokens};
use syn::Fields;

#[proc_macro_derive(Sexp)]
pub fn sexp(input: TokenStream) -> TokenStream {
    // Construct a representation of Rust code as a syntax tree
    // that we can manipulate
    let ast = syn::parse(input).unwrap();
    impl_sexp_macro(ast)
}

fn impl_sexp_macro(ast: syn::DeriveInput) -> TokenStream {
    let item = syn::Item::from(ast);
    let ident = match item {
        syn::Item::Enum(item_enum) => item_enum.ident,
        syn::Item::Struct(item_struct) => item_struct.ident,
        _ => panic!("Can only support Struct and Enum object"),
    };
    let class_str = ident.to_string();
    let finalizer_ident = quote::format_ident!("__finalize__{}", ident);
    let expanded = quote! {
        // https://github.com/extendr/extendr/blob/master/extendr-macros/src/extendr_impl.rs#L223

        // Input conversion function for this type.
        impl<'a> extendr_api::FromRobj<'a> for &#ident {
            fn from_robj(robj: &'a Robj) -> std::result::Result<Self, &'static str> {
                if extendr_api::robj::Rinternals::check_external_ptr_type::<#ident>(robj) {
                    #[allow(clippy::transmute_ptr_to_ref)]
                    Ok(unsafe { std::mem::transmute(extendr_api::robj::Rinternals::external_ptr_addr::<#ident>(robj)) })
                } else {
                    Err(concat!("expected ", #class_str))
                }
            }
        }

        // Input conversion function for a reference to this type.
        impl<'a> extendr_api::FromRobj<'a> for &mut #ident {
            fn from_robj(robj: &'a Robj) -> std::result::Result<Self, &'static str> {
                if extendr_api::robj::Rinternals::check_external_ptr_type::<#ident>(robj) {
                    #[allow(clippy::transmute_ptr_to_ref)]
                    Ok(unsafe { std::mem::transmute(extendr_api::robj::Rinternals::external_ptr_addr::<#ident>(robj)) })
                } else {
                    Err(concat!("expected ", #class_str))
                }
            }
        }

        // Function to free memory for this type.
        #[allow(non_snake_case)]
        extern "C" fn #finalizer_ident (sexp: extendr_api::SEXP) {
            unsafe {
                let robj = extendr_api::robj::Robj::from_sexp(sexp);
                if extendr_api::robj::Rinternals::check_external_ptr_type::<#ident>(&robj) {
                    //eprintln!("finalize {}", #class);
                    let ptr = extendr_api::robj::Rinternals::external_ptr_addr::<#ident>(&robj);
                    drop(Box::from_raw(ptr));
                }
            }
        }

        use extendr_api::robj::Attributes; // used by set_attrib

        // Output conversion function for this type.
        impl std::convert::From<#ident> for extendr_api::Robj {
            fn from(value: #ident) -> Self {
                unsafe {
                    let ptr = Box::into_raw(Box::new(value));
                    let mut res = extendr_api::Robj::make_external_ptr(ptr, extendr_api::Robj::from(()));
                    res.set_attrib(extendr_api::prelude::class_symbol(), #class_str).unwrap();
                    res.register_c_finalizer(Some(#finalizer_ident));
                    res
                }
            }
        }

        // Output conversion function for this type.
        impl<'a> std::convert::From<&'a #ident> for extendr_api::Robj {
            fn from(value: &'a #ident) -> Self {
                unsafe {
                    let ptr = Box::into_raw(Box::new(value));
                    let mut res = extendr_api::Robj::make_external_ptr(ptr, extendr_api::Robj::from(()));
                    res.set_attrib(extendr_api::prelude::class_symbol(), #class_str).unwrap();
                    res.register_c_finalizer(Some(#finalizer_ident));
                    res
                }
            }
        }
    };
    TokenStream::from(expanded)
}
@CGMossa
Copy link
Member

CGMossa commented Apr 15, 2024

Let me guess what this is about: You'd like to put #[extendr] on struct or enum, so that they may be made passable to R.

There is a limitation of proc-macro that we are facing here. You cannot put #[extendr] on both a struct and its impl-block:

#[extendr]
struct Model([u8]);

#[extendr]
impl Model {
// ...
}

The issue is, the macro cannot detect if you only put an #[extendr] on the struct and its impl-block or both.
So another request (that I cannot do right now) is allow the ability to split up the impl-block into two, or in two files. That's not possible because the proc-macro doesn't know.

At the moment, if you add #[extendr] on struct, that doesn't do anything, so there is no issue.

If you want to send rust data to R, you can use ExternalPtr<T>. That's what powers #[extendr]-impl anyway. The changes haven't been merged yet, but we will be making all the #[extendr]-impl macro use ExternalPtr<T> completely.

@Yunuuuu
Copy link
Author

Yunuuuu commented Apr 15, 2024

Thanks for your details, I think I have mis-understand the usage of #[extendr] and extendr_module.

After several days of using extendr and your explanations, maybe extendr works in this way (Please correct me if I'm wrong):

  1. Struct or Enum: won't do anything. (I mis-understood extendr will register methods to transform them into Robj, but extendr don't so I'm here to request).
  2. fn: transform this function directly into C function, it means we must manually transform the input or ouput argument into a extendr_api::Robj (or define the traits: extendr_api::FromRobj and std::convert::From for extendr_api::Robj ).
  3. impl: transform all methods into the C function for R usage, and also automatically define the traits: extendr_api::FromRobj and std::convert::From for extendr_api::Robj for Self(which should be the Struct or Enum). This means we can also directly export rust function involved in these Struct or Enum without manually transforming between extendr_api::Robj (since these methods have been implemented when we export impl for these Struct or Enum).

The different implement details for fn and impl confused me (since I thought impl will just iterate with fn method) when I first got touch with extendr. Maybe it's better to split impl implement (take the traits: extendr_api::FromRobj and std::convert::From for extendr_api::Robj into Struct and Enum), and instead just return them as it, it's better to leave user know they cannot be used in other object.

other_item => TokenStream::from(quote! {#other_item}),

It seems savvy get these well.
https://yutannihilation.github.io/savvy/guide/savvy_macro.html
image

@yutannihilation
Copy link
Contributor

Yeah, I ended up requiring always putting #[savvy] macro both on struct and on impl. The reasons are

  • I feel it's simpler not only as a user interface but also as the internal implementation.
  • An expert user might want to implement the conversion trait (in my framework's case, TryFrom) by themselves.

That said, for enum output, it's not a big deal to wrap an enum with a struct. So, I think it's also reasonable not to support enum output. For this reason, savvy doesn't support an enum with fields.

cf. https://yutannihilation.github.io/savvy/guide/enum.html#limitation

@CGMossa
Copy link
Member

CGMossa commented Apr 15, 2024

I find requiring #[extendr] on impl-block and struct to be an okay idea, but right now, I don't see an immediate use for it. I believe @JosiahParry is also in favour of splitting this purpose. So that's two votes already!

If you want structs to be embeddable in R via ExternalPtr<T> you may use an empty impl-block:

#[extendr]
impl Model {}

On the other hand, there are multiple ways one can embed structs to R. One may embed it as a List, if
all fields implements Robj: From<T>. And viceversa. We have derive-macros for that already.
One may also view plain enums as R factor. I've got a prototype implementation of that here https://github.com/cgmossa/extendr, see https://github.com/CGMossa/extendr/blob/master/extendr-macros/src/extendr_enum.rs for a little preview.

As I said before, we have things in the works for this area; One particular thing I've investigated is how we return &T/&mut T (without cloning) to R, from an #[extendr]-block. I hold the opinion that one cannot create an ExternalPtr from &T/&mut T, instead I think we should employ a pass-through strategy, which is implemented in a PR that's "open" right now.

@Yunuuuu
Copy link
Author

Yunuuuu commented Apr 16, 2024

Thanks, this is a good way to do the same work now.

#[extendr]
impl Model {}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants