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

Add first-class support for binary crates #1843

Merged
merged 5 commits into from Nov 4, 2019

Conversation

boringcactus
Copy link
Contributor

@boringcactus boringcactus commented Oct 29, 2019

fixes #1630

Essentially, "if nothing was tagged #[wasm_bindgen(start)] and there is an exported fn main(i32, i32) -> i32 then process it as though it were tagged that way."

open questions before i'm confident this implementation is ready:

  • should it refuse to autodiscover a main that is already tagged for wasm_bindgen in some other way?
  • can the shim_idx field on the function descriptor be safely set to 0?
  • can the arg names be justifiably set to argc and argv?
  • should there be an automated test for this behavior somewhere? if so, how and where should that test be written?

this allows for first-class support of binary crates
@alexcrichton
Copy link
Contributor

@alexcrichton alexcrichton commented Oct 30, 2019

Thanks for this! I agree that pattern matching and using inference here is the way to go. I think though what we need to do here is to perhaps inject a new function to be start which calls main with zero'd out arguments?

Currently this I believe will export a binding for main but we want to be sure to hook it up to all the start infrastructure to make sure it's started automatically

@boringcactus
Copy link
Contributor Author

@boringcactus boringcactus commented Oct 31, 2019

considering that #[wasm_bindgen(start)] emits a scary warning if applied to a function with arguments, hacking the planet to get the same effect attached to a function with two arguments is probably not ideal.

if i manually write #[wasm_bindgen(start)] pub fn start() { main(); } in Rust and then ask walrus what its body is, i get

function Id { idx: 22 } (Some("start"))
  GlobalGet(GlobalGet { global: Id { idx: 0 } })
  LocalSet(LocalSet { local: Id { idx: 182 } })
  Const(Const { value: I32(16) })
  LocalSet(LocalSet { local: Id { idx: 183 } })
  LocalGet(LocalGet { local: Id { idx: 182 } })
  LocalGet(LocalGet { local: Id { idx: 183 } })
  Binop(Binop { op: I32Sub })
  LocalSet(LocalSet { local: Id { idx: 184 } })
  LocalGet(LocalGet { local: Id { idx: 184 } })
  GlobalSet(GlobalSet { global: Id { idx: 0 } })
  Call(Id { idx: 21 } (Some("bin_crate_without_a_bundler::start::h2a2091f59a0e039a")))
  Call(Id { idx: 888 } (Some("<T as wasm_bindgen::convert::traits::ReturnWasmAbi>::return_abi::h4fdec286c4868b5e")))
  Const(Const { value: I32(16) })
  LocalSet(LocalSet { local: Id { idx: 185 } })
  LocalGet(LocalGet { local: Id { idx: 184 } })
  LocalGet(LocalGet { local: Id { idx: 185 } })
  Binop(Binop { op: I32Add })
  LocalSet(LocalSet { local: Id { idx: 186 } })
  LocalGet(LocalGet { local: Id { idx: 186 } })
  GlobalSet(GlobalSet { global: Id { idx: 0 } })
  Return(Return)
function Id { idx: 21 } (Some("bin_crate_without_a_bundler::start::h2a2091f59a0e039a"))
  Call(Id { idx: 20 } (Some("bin_crate_without_a_bundler::main::ha40db6ddc3c404a3")))
  Return(Return)

so hopefully replicating that verbatim is not actually necessary (especially bc a few pieces would be difficult or impossible to reliably replicate). zeroing out the arguments and dropping the return value is, unsurprisingly, far easier.

@alexcrichton
Copy link
Contributor

@alexcrichton alexcrichton commented Nov 1, 2019

I think you'll want to avoid calling self.program here and synthesize a new function to place in the start function with walrus, and I think that'd help cut down on the verbosity here as well?

Additionally can you be sure to add some tests for this change?

@boringcactus
Copy link
Contributor Author

@boringcactus boringcactus commented Nov 1, 2019

I'm already synthesizing a replacement with walrus, but per your suggestion I've skipped program and gone straight to export since that's the only part of program that actually runs anyway.

i wasn't quite sure where to write tests, but

//! Assertions about errors in `wasm-bindgen` or assertions about the output of
//! `wasm-bindgen` should all be placed in this test suite, however. Currently
pointed me in the right direction.

@alexcrichton
Copy link
Contributor

@alexcrichton alexcrichton commented Nov 4, 2019

Oh right sorry good point! I think though that none of export or program should be needed here, it should be possible to just directly modify the state of the local context to reflect that the main function was found? Either that or this could share infrastructure with the detection of #[wasm_bindgen(start)]?

And yeah that tests directory should be good to house tests!

.functions()
.find(|x| x.name.as_ref().map_or(false, |x| x == "main"))
.map(|x| (x.ty(), x.id()));
if let Some((main_type, main_id)) = main_info {
Copy link
Contributor

@alexcrichton alexcrichton Nov 4, 2019

Choose a reason for hiding this comment

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

Could this use a match with an early return to de-indent the rest of this body?

Copy link
Contributor Author

@boringcactus boringcactus Nov 4, 2019

Choose a reason for hiding this comment

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

it could indeed!

@boringcactus
Copy link
Contributor Author

@boringcactus boringcactus commented Nov 4, 2019

export is the detection of #[wasm_bindgen(start)], but it turns out only one line from export is actually needed for the new test to pass, so just using that one line cleans up a lot of other stuff.

@alexcrichton
Copy link
Contributor

@alexcrichton alexcrichton commented Nov 4, 2019

Perfect, thanks!

@alexcrichton alexcrichton merged commit 79cf4f6 into rustwasm:master Nov 4, 2019
20 checks passed
@boringcactus boringcactus deleted the first-class-bins branch Nov 4, 2019
bors bot added a commit to grovesNL/glow that referenced this issue May 14, 2020
105: Simplify the example a bit r=grovesNL a=17cupsofcoffee

Here's a few tweaks that make the example a bit simpler/up-to-date:

* A seperate `wasm_main` is no longer required due to rustwasm/wasm-bindgen#1843.
* The prelude import wasn't actually being used anywhere.
* Nightly is no longer required to build this project, so I removed that from the instructions.
* `--no-modules` has been replaced with `--target web`, which doesn't rely on global variables and allows support for [JS snippets](https://rustwasm.github.io/docs/wasm-bindgen/reference/js-snippets.html).
    * The only caveat to this is that the generates JS is a ES module instead of a basic JS file - but any browser that supports WASM will also support ES modules, so I don't think this is an issue in practice.

Co-authored-by: Joe Clay <27cupsofcoffee@gmail.com>
bors bot added a commit to grovesNL/glow that referenced this issue May 21, 2020
105: Simplify the example a bit r=grovesNL a=17cupsofcoffee

Here's a few tweaks that make the example a bit simpler/up-to-date:

* A seperate `wasm_main` is no longer required due to rustwasm/wasm-bindgen#1843.
* The prelude import wasn't actually being used anywhere.
* Nightly is no longer required to build this project, so I removed that from the instructions.
* `--no-modules` has been replaced with `--target web`, which doesn't rely on global variables and allows support for [JS snippets](https://rustwasm.github.io/docs/wasm-bindgen/reference/js-snippets.html).
    * The only caveat to this is that the generates JS is a ES module instead of a basic JS file - but any browser that supports WASM will also support ES modules, so I don't think this is an issue in practice.

Co-authored-by: Joe Clay <27cupsofcoffee@gmail.com>
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