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

Implement OptionIntoWasmAbi for Closure references #2768

Merged

Conversation

sinking-point
Copy link
Contributor

See #2767

@sinking-point sinking-point marked this pull request as draft January 13, 2022 14:48
@sinking-point sinking-point marked this pull request as ready for review January 13, 2022 15:17
Copy link
Contributor

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Can you add some tests where None is used? Otherwise I think it would be useful to add a test or double-check that use of the closure from JS after the function has returned throws a well-defined exception

@sinking-point
Copy link
Contributor Author

@alexcrichton Thanks for your feedback.

I meant to uncomment the None test before but it seems I forgot, so I've done that now and tightened it a bit too. Since there are no variations on None, I think this one test is sufficient.

Unlike Fn, FnMut and FnOnce, Closures are not invalidated after the imported function returns. I have instead added a test for the behaviour after .drop()ping the Closure. I manually checked the message of the exception:

Error: closure invoked recursively or destroyed already

This is as expected.

@alexcrichton
Copy link
Contributor

Ah yes, my mistake! Thanks!

@alexcrichton alexcrichton merged commit 3701c9d into rustwasm:main Jan 18, 2022
@sinking-point
Copy link
Contributor Author

No worries. Thanks for merging.

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.

2 participants