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

Feature/multiple return values #90

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

hendrik-s-debruin
Copy link
Contributor

DO NOT MERGE THIS

This is a first exploration on the discussion here. I have not made much attempt at making the code elegant at this point.

I have added a new pattern, the AugmentedFunction. Currently, this pattern can be used to return multiple return values, but theoretically it could be updated to also handle fallible functions. The goal would be to eventually add something similar for the service pattern, but I have left this as out of scope for the current exploration.

I have not attempted to write the back-end writers for cpython and C# yet, as I'm not experienced with creating bindings there. I have for now only shown how the C back-end would be modified.

I am having some difficulty understanding how to run all the tests. For now, I have done some basic testing inside of the hello_world example by calling cargo test there, and inspecting the generated C header file that gets generated.

I've been using cargo expand inside of the hello_world example to inspect what the proc macros are generating. In a nutshell, the idea is that clients write:

/// Function using the type.
#[ffi_function]
#[no_mangle]
pub extern "C" fn my_function3(input: Vec2) -> (i32, i32) {
    (1, 2)
}

This function will then get renamed by the macro to my_function3_interoptopus_internal. The macro will also generate another new function with the original name my_function3 that contains OUT parameters. The output of of this macro, obtained by cargo expand, for this example is currently:

/// Function using the type.
#[no_mangle]
pub extern "C" fn my_function3_interoptopus_internal(input: Vec2) -> (i32, i32) {
    (1, 2)
}

/// TODO put the documentation strings here
#[no_mangle]
pub extern "C" fn my_function3(
    input: Vec2,
    out_param_0: *mut i32,
    out_param_1: *mut i32,
) {
    let (return_value_0, return_value_1) = my_function3_interoptopus_internal(input);
    unsafe {
        *out_param_0 = return_value_0;
        *out_param_1 = return_value_1;
    }
}

For now I have not attempted to add user-defined names for the OUT parameters. This could be done by adding arguments to the ffi_function attribute macro, but has been left as an open point for now.

The header file that is generated contains the following:

void my_function3(vec2 input, int32_t* out_param_0, int32_t* out_param_1);

The function my_function3_interoptopus_internal is not exposed in the header function.

The idea is that the backend writers realise that they are dealing with an AugmentedFunction and translate the OUT parameters into something that is idiomatic for them. For instance, Python could return a tuple directly.

Note that, since the macro works with tokens and not type information, the user cannot write something like

type MultipleOutput = (f32, i32);
#[no_mangle]
pub extern "C" fn foo() -> MultipleOutput {
    todo!()
}

The macro needs to see the tuple in the token stream in order to realise that this is an augmented function with multiple return values.

At this stage I am not 100% sure whether the AugmentedFunction pattern has been hooked up properly to the writers. The function is still exposed via the function! macro. I'm not sure if this function will have to be edited, or whether the definition of FunctionInfo should be changed such that AugmentedFunction will be received by the backend writers.

Finally, fallible functions could be added to this AugmentedFunction by adding another OUT parameter representing the error type. The function would then return an enum for the success status of the function call.

@ralfbiedert ralfbiedert added c-core Interoptopus Core Crate c-proc Proc Macros needs-discussion Something rather fuzzy, input wanted. enhancement Make existing things better. labels Jun 22, 2023
@ralfbiedert
Copy link
Owner

Sorry for delay, few days off, will look into this once back.

@ralfbiedert
Copy link
Owner

Alright, I managed to have a quick look, some thoughts:

  • Having a transform for pub extern "C" fn my_function3(input: Vec2) -> (i32, i32) looks great!
  • That is works with a vanilla #[ffi_function] is a bit too magic IMO. I think a plain ffi_function should only enable a function to be exported. To transform output to input parameters should probably be handled by another annotation.
  • I'm not sure yet what's the best way for annotating that would be. For example ffi_service is a 'pattern annotation' (and as such it's implied that it may generate or transform methods), while ffi_function so far is only a marker annotation. That said this distinction might only exist in my head, and from a UX perspective maybe ffi_function should 'just work' ... What do you think?
  • Once implemented for real this probably needs a whole bunch of unit tests (e.g., using generics, lifetimes in return types, custom opaque pointer returns, ...).
  • I'm not sure this pattern is a TypePattern ... I mean, it sort of is, but also the actual pattern is also a function transformation, which in turn would open the can of worms of having a new pattern type ...

About your comments:

I am having some difficulty understanding how to run all the tests

Yeah ... it's a bit of a mess. Ideally you have dotnet and python installed, then cargo test should actually run the tests. In addition there are a few tests that rely on .expected files which, if changed, can be regenerated with the scripts. Feel free to reach out if you need help.

I've been using cargo expand inside of the hello_world example to inspect what the proc macros are generating

Many attributes (should be all, but I might not have come around) should have a debug flag to help with that, although the formatting often gets messy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-core Interoptopus Core Crate c-proc Proc Macros enhancement Make existing things better. needs-discussion Something rather fuzzy, input wanted.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants