Skip to content

First refactor for WebIDL bindings #1566

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

Merged
merged 26 commits into from
Jun 5, 2019

Conversation

alexcrichton
Copy link
Contributor

This commit starts the wasm-bindgen CLI tool down the road to being a
true polyfill for WebIDL bindings. This refactor is probably the first
of a few, but is hopefully the largest and most sprawling and everything
will be a bit more targeted from here on out.

The goal of this refactoring is to separate out the massive
crates/cli-support/src/js/mod.rs into a number of separate pieces of
functionality. It currently takes care of basically everything
including:

  • Binding intrinsics
  • Handling anyref transformations
  • Generating all JS for imports/exports
  • All the logic for how to import and how to name imports
  • Execution and management of wasm-bindgen closures

Many of these are separable concerns and most overlap with WebIDL
bindings. The internal refactoring here is intended to make it more
clear who's responsible for what as well as making some existing
operations much more straightforward. At a high-level, the following
changes are done:

  1. A src/webidl.rs module is introduced. The purpose of this module is
    to take all of the raw wasm-bindgen custom sections from the module
    and transform them into a WebIDL bindings section.

This module has a placeholder WebidlCustomSection which is nowhere
near the actual custom section but if you squint is in theory very
similar. It's hoped that this will eventually become the true WebIDL
custom section, currently being developed in an external crate.

Currently, however, the WebIDL bindings custom section only covers a
subset of the functionality we export to wasm-bindgen users. To avoid
leaving them high and dry this module also contains an auxiliary
custom section named WasmBindgenAux. This custom section isn't
intended to have a binary format, but is intended to represent a
theoretical custom section necessary to couple with WebIDL bindings to
achieve all our desired functionality in wasm-bindgen. It'll never
be standardized, but it'll also never be serialized :)

  1. The src/webidl.rs module now takes over quite a bit of
    functionality from src/js/mod.rs. Namely it handles synthesis of an
    export_map and an import_map mapping export/import IDs to exactly
    what's expected to be hooked up there. This does not include type
    information (as that's in the bindings section) but rather includes
    things like "this is the method of class A" or "this import is from
    module foo" and things like that. These could arguably be subsumed
    by future JS features as well, but that's for another time!

  2. All handling of wasm-bindgen "descriptor functions" now happens in a
    dedicated src/descriptors.rs module. The output of this module is
    its own custom section (intended to be immediately consumed by the
    WebIDL module) which is in theory what we want to ourselves emit one
    day but rustc isn't capable of doing so right now.

  3. Invocations and generations of imports are completely overhauled.
    Using the import_map generated in the WebIDL step all imports are
    now handled much more precisely in one location rather than
    haphazardly throughout the module. This means we have precise
    information about each import of the module and we only modify
    exactly what we're looking at. This also vastly simplifies intrinsic
    generation since it's all simply a codegen part of the rust2js.rs
    module now.

  4. Handling of direct imports which don't have a JS shim generated is
    slightly different from before and is intended to be
    future-compatible with WebIDL bindings in its full glory, but we'll
    need to update it to handle cases for constructors and method calls
    eventually as well.

  5. Intrinsic definitions now live in their own file (src/intrinsic.rs)
    and have a separated definition for their symbol name and signature.
    The actual implementation of each intrinsic lives in rust2js.rs

There's a number of TODO items to finish before this merges. This
includes reimplementing the anyref pass and actually implementing import
maps for other targets. Those will come soon in follow-up commits, but
the entire tests/wasm/main.rs suite is currently passing and this
seems like a good checkpoint.

@@ -158,7 +158,7 @@ fn switch_methods() {

assert!(!switch_methods_called());
SwitchMethods::new().b();
assert!(switch_methods_called());
assert!(!switch_methods_called());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this is indicative of that there are actually a few subtle behavior changes in this PR. Some things aren't quite as "final" as they used to be, although I suspect that this is ok in practice basically everywhere.

@alexcrichton alexcrichton force-pushed the webidl-bindings-refactor-1 branch from ff6b4c4 to 8e34b8d Compare June 3, 2019 18:38
@alexcrichton
Copy link
Contributor Author

Ok! I've put this PR through its paces and I think this is ready to go. I suspect that there's at least one bug still lurking but I'm pretty confident there's nothing overly glaring at least.

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Ok, here is my review for the first commit; going to get to the rest of them tomorrow :)

let import_id = module
.imports
.iter()
.find(|i| i.name == import_name)
Copy link
Member

Choose a reason for hiding this comment

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

Want to add a find_by_name method to walrus::ModuleImports? Or file a good-first-issue.

Edit: filed rustwasm/walrus#84

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 filed rustwasm/walrus#86 for this as well

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Ok all the rest of those commits were indeed much more manageable to go through, and I don't really have much to say :)

Looks good!

/// `HashMap` might affect the generated JS bindings. We want to ensure that the
/// generated output is deterministic and we do so by ensuring that iteration of
/// hash maps is consistently sorted.
fn sorted_iter<K, V>(map: &HashMap<K, V>) -> impl Iterator<Item = (&K, &V)>
Copy link
Member

Choose a reason for hiding this comment

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

Is this better than using BTreeMap instead of HashMap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eh I think it's sort of taste. I personally like to use HashMap as much as possible until proven otherwise, and then when it affects output either use a temporary BTreeMap or sort it like this somehow. It really could go either way though and I don't have a preference one way or another.

alexcrichton and others added 21 commits June 5, 2019 07:52
This commit starts the `wasm-bindgen` CLI tool down the road to being a
true polyfill for WebIDL bindings. This refactor is probably the first
of a few, but is hopefully the largest and most sprawling and everything
will be a bit more targeted from here on out.

The goal of this refactoring is to separate out the massive
`crates/cli-support/src/js/mod.rs` into a number of separate pieces of
functionality. It currently takes care of basically everything
including:

* Binding intrinsics
* Handling anyref transformations
* Generating all JS for imports/exports
* All the logic for how to import and how to name imports
* Execution and management of wasm-bindgen closures

Many of these are separable concerns and most overlap with WebIDL
bindings. The internal refactoring here is intended to make it more
clear who's responsible for what as well as making some existing
operations much more straightforward. At a high-level, the following
changes are done:

1. A `src/webidl.rs` module is introduced. The purpose of this module is
   to take all of the raw wasm-bindgen custom sections from the module
   and transform them into a WebIDL bindings section.

  This module has a placeholder `WebidlCustomSection` which is nowhere
  near the actual custom section but if you squint is in theory very
  similar. It's hoped that this will eventually become the true WebIDL
  custom section, currently being developed in an external crate.

  Currently, however, the WebIDL bindings custom section only covers a
  subset of the functionality we export to wasm-bindgen users. To avoid
  leaving them high and dry this module also contains an auxiliary
  custom section named `WasmBindgenAux`. This custom section isn't
  intended to have a binary format, but is intended to represent a
  theoretical custom section necessary to couple with WebIDL bindings to
  achieve all our desired functionality in `wasm-bindgen`. It'll never
  be standardized, but it'll also never be serialized :)

2. The `src/webidl.rs` module now takes over quite a bit of
   functionality from `src/js/mod.rs`. Namely it handles synthesis of an
   `export_map` and an `import_map` mapping export/import IDs to exactly
   what's expected to be hooked up there. This does not include type
   information (as that's in the bindings section) but rather includes
   things like "this is the method of class A" or "this import is from
   module `foo`" and things like that. These could arguably be subsumed
   by future JS features as well, but that's for another time!

3. All handling of wasm-bindgen "descriptor functions" now happens in a
   dedicated `src/descriptors.rs` module. The output of this module is
   its own custom section (intended to be immediately consumed by the
   WebIDL module) which is in theory what we want to ourselves emit one
   day but rustc isn't capable of doing so right now.

4. Invocations and generations of imports are completely overhauled.
   Using the `import_map` generated in the WebIDL step all imports are
   now handled much more precisely in one location rather than
   haphazardly throughout the module. This means we have precise
   information about each import of the module and we only modify
   exactly what we're looking at. This also vastly simplifies intrinsic
   generation since it's all simply a codegen part of the `rust2js.rs`
   module now.

5. Handling of direct imports which don't have a JS shim generated is
   slightly different from before and is intended to be
   future-compatible with WebIDL bindings in its full glory, but we'll
   need to update it to handle cases for constructors and method calls
   eventually as well.

6. Intrinsic definitions now live in their own file (`src/intrinsic.rs`)
   and have a separated definition for their symbol name and signature.
   The actual implementation of each intrinsic lives in `rust2js.rs`

There's a number of TODO items to finish before this merges. This
includes reimplementing the anyref pass and actually implementing import
maps for other targets. Those will come soon in follow-up commits, but
the entire `tests/wasm/main.rs` suite is currently passing and this
seems like a good checkpoint.
Make sure the wasm import definition map is hooked up correctly!
This is no longe rneeded now that we precisely track what needs to be
exported for an imported item, so all the imports are hooked up
correctly elsewhere without the need for the `__exports` map.
Make sure to explicitly parse strings to numbers
Need to make sure we update the import itself and configure the value on
the import object!
This is supposed to be `===`, not `==`.
This commit reimplements the `anyref` transformation pass tasked with
taking raw rustc output and enhancing the module to use `anyref`. This
was disabled in the previous commits during refactoring, and now the
pass is re-enabled in the manner originally intended.

Instead of being tangled up in the `js/mod.rs` pass, the anyref
transformation now happens locally within one module,
`cli-support/src/anyref.rs`, which exclusively uses the output of the
`webidl` module which produces a WebIDL bindings section as well as an
auxiliary wasm-bindgen specific section. This makes the anyref transform
much more straightforward and local, ensuring that it doesn't propagate
elsewhere and can be a largely local concern during the transformation.

The main addition needed to support this pass was detailed knowledge of
the ABI of a `Descriptor`. This knowledge is already implicitly
hardcoded in `js2rust.rs` and `rust2js.rs` through the ABI shims
generated. This was previously used for the anyref transformation to
piggy-back what was already there, but as a separate pass we are unable
to reuse the knowledge in the binding generator.

Instead `Descriptor` now has two dedicated methods describing the
various ABI properties of a type. This is then asserted to be correct
(all the time) when processing bindings, ensuring that the two are kept
in sync.
* Catch all closures by walking all `Descriptor` values and looking for
  either `Function` or `Closure`.
* Update the correct arguments for wasm by ensuring that the closure
  modifications skip the first two arguments.
Don't delay processing until `finalize`, but instead process it as soon
as it's requested to avoid doing too much logic in `finalize`.
This allows using WebIDL bindings types to describe both of them instead
of having a custom ABI, allowing for more direct and rich bindings
eventually!
Iteration order of hash maps is nondeterministic, so add a `sorted_iter`
function and then use that throughout whenever iteration order of a hash
map would affect the generated JS.
These can have similar optimizations as importing a value directly.
This was once required due to flavorful management of the `WeakRef`
proposal but nowadays it's simple enough that we don't need to refactor
it out here.
@alexcrichton alexcrichton force-pushed the webidl-bindings-refactor-1 branch 8 times, most recently from 8010bf3 to 0aa1aff Compare June 5, 2019 18:03
@alexcrichton alexcrichton force-pushed the webidl-bindings-refactor-1 branch 4 times, most recently from 68fc2b4 to c4d9f41 Compare June 5, 2019 19:14
@alexcrichton alexcrichton force-pushed the webidl-bindings-refactor-1 branch from c4d9f41 to e24c031 Compare June 5, 2019 19:26
@alexcrichton alexcrichton merged commit cf2a42c into rustwasm:master Jun 5, 2019
@alexcrichton alexcrichton deleted the webidl-bindings-refactor-1 branch June 5, 2019 20:05
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