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.

Sign up for GitHub

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

Jump to bottom

Disable closures as host functions for now + docs + tests #1841

Merged
merged 5 commits into from
Nov 30, 2020
Merged

Disable closures as host functions for now + docs + tests #1841

merged 5 commits into from
Nov 30, 2020

Conversation

MarkMcCaskey
Copy link
Contributor

@MarkMcCaskey MarkMcCaskey commented Nov 24, 2020 •
edited
Loading

Resolves #1811 for now

Part of #1840

Review

  • Add a short description of the the change to the CHANGELOG.md file

@MarkMcCaskey MarkMcCaskey force-pushed the fix/explicitly-disable-closures-as-host-fns branch from 3ea8817 to 29d50a5 Compare November 24, 2020 20:44
@jubianchi jubianchi added 1.0 Wasmer at 1.0 🎉 enhancement New feature! bug Something isn't working labels Nov 25, 2020
@Hywan
Copy link
Contributor

Hywan commented Nov 26, 2020

It's going to break wasmer-python, and probably wasmer-c-api (didn't check for that one). Is it temporary?

@syrusakbary
Copy link
Member

We don't support native closures with Wasmer... are they used in wasmer-python ?

@webmaster128
Copy link
Contributor

webmaster128 commented Nov 26, 2020 •
edited
Loading

We use this feature for a year or so without any issues. In our case, the api is copied into the closure: https://github.com/CosmWasm/cosmwasm/blob/upgrade-to-wasmer-reborn/packages/vm/src/instance.rs#L124-L130. For some reason this still compiles (EDIT: and tests pass) using Wasmer at 82fc008. What am I missing here? Maybe I don't know what a "closure" is? Maybe copies are okay? Is the move making a difference?

@webmaster128
Copy link
Contributor

In https://doc.rust-lang.org/book/ch13-01-closures.html#capturing-the-environment-with-closures there is a good explanation regarding naming and the different capture types. In Rust all anonymous functions are closures (no "lambda" as used in other languages). Then there are different types of environment capturing. And it turns out some of those captures (all captured data owned by the function) are still possible with this PR.

@Hywan
Copy link
Contributor

Hywan commented Nov 27, 2020

We don't support native closures with Wasmer... are they used in wasmer-python ?

Yes they are: https://github.com/wasmerio/wasmer-python/blob/99bd88d7f8a7cc4083625767d0cac77052456cb1/packages/api/src/externals/function.rs#L145-L193

It captures the result_types variable. Not that we can still store them in the environment 🤔.

Hywan added a commit to Hywan/python-ext-wasm that referenced this pull request Nov 27, 2020
@Hywan
feat(api) Avoid using a closure to declare host function.
e57f02d 
Closures as host functions are going to be disabled for a moment in
Wasmer, see wasmerio/wasmer#1841. In the
`wasmer` Python package, we were passing a closure to
`Function::new_with_env`. In order to get a regular function, this
patch stores the previously captured varibles into the function
environment. The only concerned variable is `result_types`.
@Hywan Hywan mentioned this pull request Nov 27, 2020
Merged
@webmaster128
Copy link
Contributor

webmaster128 commented Nov 27, 2020 •
edited
Loading

I found why my code is not affected by this change. We use Function::new_with_env, but this PR is limited to Function::new_native and friends. Is this a missing check or should only "native" functions have the new restriction? This means wasmerio/wasmer-python#420 was not needed.

webmaster128
webmaster128 reviewed Nov 27, 2020
View reviewed changes
tests/compilers/native_functions.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
syrusakbary and others added 2 commits November 30, 2020 14:09
@syrusakbary @webmaster128
Update tests/compilers/native_functions.rs
081af64 
Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com>
@syrusakbary @webmaster128
Update CHANGELOG.md
f583263 
Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com>
@syrusakbary
Copy link
Member

This means wasmerio/wasmer-python#420 was not needed.

Yeah, you are right @webmaster128

@syrusakbary
Copy link
Member

bors r+

bors bot added a commit that referenced this pull request Nov 30, 2020
@bors @syrusakbary
Merge #1841
9f0eb24 
1841: Disable closures as host functions for now + docs + tests r=syrusakbary a=MarkMcCaskey

Resolves #1811 for now

Part of #1840 

# Review

- [x] Add a short description of the the change to the CHANGELOG.md file


Co-authored-by: Mark McCaskey <mark@wasmer.io>
Co-authored-by: Syrus Akbary <me@syrusakbary.com>
@bors
Copy link
Contributor

bors bot commented Nov 30, 2020

Build failed:

@MarkMcCaskey
Copy link
Contributor Author

bors r+

@bors
Copy link
Contributor

bors bot commented Nov 30, 2020

Build succeeded:

@bors bors bot merged commit 5fe3929 into master Nov 30, 2020
@bors bors bot deleted the fix/explicitly-disable-closures-as-host-fns branch November 30, 2020 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@webmaster128 webmaster128 webmaster128 left review comments

@jubianchi jubianchi jubianchi approved these changes

@syrusakbary syrusakbary syrusakbary approved these changes

@Hywan Hywan Awaiting requested review from Hywan

Assignees
No one assigned
Labels
1.0 Wasmer at 1.0 bug Something isn't working 🎉 enhancement New feature!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Function::new_native support for closures with captured environments
5 participants
@MarkMcCaskey @Hywan @syrusakbary @webmaster128 @jubianchi