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

Ungate start functions, and restrict args/returns. #297

Closed

Conversation

sunfishcode
Copy link
Member

Remove the restriction on component start functions calling imports. This allows start functions to run artbitrary user code.

And, adjust the 🪙 to include start functions in the MVP, and add a restriction that start functions cannot currently have arguments or return values, for now.

And add some words describing the order that start functions are called in.

@lukewagner
Copy link
Member

This makes sense to me. Originally, I had thought the restriction enabled new kinds of AOT snapshotting and avoided some weird initialization-time circularities regarding parent/child initialization but, with the benefit of the intervening two years and tons more experience, I don't think it's actually buying us anything.

The only question is: independent of removing the trap, do we want to go ahead and add the component-level start function now? If Preview 2 implementations are happy to add it Real Soon, that sounds good to me.

@sunfishcode
Copy link
Member Author

This is also aligning with existing implementation in Wasmtime, where it's already possible to have an inner core module with a core-wasm start function, which runs at instantiation time, and it can call imports at that time.

@lukewagner
Copy link
Member

So for next steps: I think we're all agreed on simply deleting runtime invariant 3 (bringing the spec in-line with current implementations); if there was a PR with just that, I think we could merge it today. But it's still a question of whether we want or need to un-gate (zero-arity) component-level start functions. For example, maybe we should consider that a feature of 0.2.1. Also, iiuc, toolchain supports exists in various places to achieve the effect we need using just the core wasm start function (which is also nullary). So my suggestion is: maybe we scope this PR down to just deleting runtime invariant 3 and consider/prioritize component-level start functions separately?

@sunfishcode
Copy link
Member Author

Ok, I've now filed #300 to just remove invariant 3, and we can consider/prioritize the rest separately.

Adjust the 🪙 to include start functions in the MVP, and add a
restriction that start functions cannot currently have arguments or
return values, for now.

And add some words describing the order that start functions are
called in.
@sunfishcode sunfishcode changed the title Allow start functions to call imports, and restrict args/returns. Ungate start functions, and restrict args/returns. Feb 7, 2024
@lukewagner
Copy link
Member

I think the criteria for this being ungated is the feature being implemented and included in some release, so I'll close this PR for now (since we don't have PRs open for all gated features). When it is time to ungate, we can reopen (and rebase, since #336 will add more cases) or just file a new PR since deleting emojis is easy.

@lukewagner lukewagner closed this Apr 30, 2024
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.

4 participants