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

support for rust fns #4

Merged
merged 10 commits into from
Jul 29, 2022
Merged

support for rust fns #4

merged 10 commits into from
Jul 29, 2022

Conversation

dr-bonez
Copy link
Contributor

@dr-bonez dr-bonez commented Jun 3, 2020

Adds a serde backend for duktape that allows for serializing rust types onto the stack, and deserializing them off of the stack.

Adds an attribute macro duktape_fn which can be applied to a rust function which will generate a wrapper function that can be imported into a duktape context. All arguments must implement serde::Deserialize and the return type must implement serde::Serialize.

Adds a function-like macro add_global_fn! which takes a context and a function that has the duktape_fn attribute, and adds it to the global object.

@dr-bonez dr-bonez marked this pull request as draft June 3, 2020 22:02
@dr-bonez
Copy link
Contributor Author

dr-bonez commented Jun 3, 2020

Marked as draft, since many more tests need to be added. However, actual functionality is complete.

src/lib.rs Outdated Show resolved Hide resolved
@dr-bonez dr-bonez marked this pull request as ready for review June 9, 2020 20:23
format!("test {}", input)
}

#[cfg_attr(feature = "derive", duktape_fn)]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it surprising that this produces two items as the proc macro output. Could the value be transformed into static test_rust_complex_fn: DukFunction = unsafe { DukFunction::new(1, |...| { ... }) } or something that is actually still one item? Could it be expressed using a trait instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can make it one item, where the function being annotated is replaced with the derived version. This would prevent the fn being usable in a non-duktape scenario however. Is this desired?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you can still impl<F> Fn for DukFunction<F> where F: Fn and so on, to make it callable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

implementing Fn is unstable. The solution @samsartor came up with, that I implemented, I think is our best bet.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM!

src/lib.rs Show resolved Hide resolved
@dr-bonez dr-bonez requested a review from dflemstr June 22, 2020 22:00
@dr-bonez
Copy link
Contributor Author

So what I did here was add a module with the same name as the function. Turns out rust allows this, and will decide which to use depending on context. The module exports a struct that implements an unsafe trait called DukFunction which has everything we need to add it to the duktape context.

@dr-bonez
Copy link
Contributor Author

@dflemstr

src/ser.rs Show resolved Hide resolved
duk-derive/src/lib.rs Outdated Show resolved Hide resolved
@dflemstr
Copy link
Owner

I've been slow to respond to this PR, let me know if this is still relevant for you and I will take another pass at the review!

@dr-bonez
Copy link
Contributor Author

yes this is still relevant. would be a nice feature to have :)

Copy link
Owner

@dflemstr dflemstr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more comments, mostly concerning soundness, but again I'm happy to merge this if you think this has been taking too long already :)

duk-derive/src/lib.rs Outdated Show resolved Hide resolved
duk-derive/src/lib.rs Show resolved Hide resolved
format!("test {}", input)
}

#[cfg_attr(feature = "derive", duktape_fn)]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM!

duk-derive/src/lib.rs Outdated Show resolved Hide resolved
@dr-bonez
Copy link
Contributor Author

dr-bonez commented Mar 3, 2022

alright it's finally time to start using this. if we can get it merged, that would be cool. if not, I can just publish and maintain a fork

@dr-bonez
Copy link
Contributor Author

dr-bonez commented Mar 4, 2022

alright, pretty sure all comments are addressed

@dflemstr dflemstr merged commit 465cb44 into dflemstr:master Jul 29, 2022
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

Successfully merging this pull request may close these issues.

None yet

3 participants