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

[WIP] whitelist for slice args that do not need to be mutable #1177

Closed
wants to merge 2,129 commits into from

Conversation

chinedufn
Copy link
Contributor

@chinedufn chinedufn commented Jan 14, 2019

Work in progress - Not completed

@alexcrichton thought I'd open an early PR and ask questions as I go along. Should take me some days to get finished as I'll only be making light touches here and there (busy week ahead).

First question: How should I go about writing tests as I go? I started poking around the codebase to better understand what was going on and where things happen but figured it'd be faster to just come and ask for more guidance.

Cheers!


If opening an unfinished PR isn't appropriate in wasm-bindgen just let me know!


#1005

fitzgen and others added 30 commits October 29, 2018 10:45
This commit adds support for running a gc pass over locals in a
function. This will remove dead local declarations for a function
(completely unused) as well as compact existing entries to ensure that
we don't have two local declarations of the same type.

While this was initially intended for some future support of emitting
shims in `wasm-bindgen`, it turns out this pass is firing quite a lot
over existing functions generated by LLVM. Looks like we may see benefit
from this today with slightly smaller wasm binaries!
Add gc support for locals in functions
This commit updates the `wasm-gc` pass of wasm-bindgen to eliminate
duplicate types in the type section, effectively enabling a gc of the
type section itself. The main purpose here is ensure that code generated
by `wasm-bindgen` itself doesn't have to go too far out of its way to
deduplicate at generation time, but rather it can rely on the gc pass to
clean up.

Note that this currently depends on paritytech/parity-wasm#231, but this
can be updated if that ends up not landing.
wasm-gc is in dire need of a better test suite, so I'll work on that
before attempting to de-pessimize this.

Closes rustwasm#994
Eliminate duplicate types in the type section
Assume all data/element/table/memory segments needed
This commit adds a test harness and the beginnings of a test suite for
the crate that performs GC over a wasm module. This crate historically
has had zero tests because it was thought that it would no longer be
used once LLD landed with `--gc-sections`, but `wasm-bindgen` has come
to rely more and more on `wasm-gc` for various purposes.

The last release of `wasm-bindgen` was also released with a bug in the
recently refactored support in the `wasm-gc` crate, providing a perfect
time and motivation to start writing some tests!

All tests added here are `*.wat` files which contain the expected output
after the gc pass is executed. Tests are automatically updated with
`BLESS_TESTS=1` in the environment, which is the expected way to
generate the output for each test.
We statically know which passive segments are actually used, so let's be
sure to gc them!
When a duplicate type is found is should no longer be considered used!
…turn-values

update typescript generation to reflect that Option<T> can be undefined
When a type is renamed in Rust via `js_name` then all method imports
will also need a `js_class` annotation to hook them up correctly.
Add a note about renaming types and `js_class`
Allow defining types which have different names in Rust than they have
in JS! (just like can be done with imported types)

Closes rustwasm#1010
Implement support for `js_class` on exported types
This commit makes the `to_idl_type` infallible, returning a new enum
variant, `UnknownInterface`, in the one location that we still return
`None`. By making this infallible we can ensure that expansion of unions
which have unknown types still generate methods for all the variants
which we actually have all the methods for!
…types instead of returning None when there is at least one unsupported type

For example, the constructor in Response.webidl accepts multiple types. However, one of those types is `ReadableStream` which isn't defined yet, and that causes all constructors for Response to be skipped even though the other argument types could be supported.
This commit updates all examples to not use `path` dependencies but
rather use versioned dependencies like would typically be found in the
wild. This should hopefully make the examples more copy-pastable and
less alien to onlookers!

The development of the examples remains the same where they continue to
use the `wasm-bindgen`, `js-sys`, `web-sys`, etc from in-tree. The
workspace-level `[patch]` section ensures that they use the in-tree
versions instead of the crates.io versions.
Don't use path dependencies in examples
This commit implements the first half of [RFC rustwasm#5] where the `Deref`
trait is implemented for all imported types. The target of `Deref` is
either the first entry of the list of `extends` attribute or `JsValue`.

All examples using `.as_ref()` with various `web-sys` types have been
updated to the more ergonomic deref casts now. Additionally the
`web-sys` generation of the `extends` array has been fixed slightly to
explicitly list implementatoins in the hierarchy order to ensure the
correct target for `Deref` is chosen.

[RFC rustwasm#5]: https://github.com/rustwasm/rfcs/blob/master/text/005-structural-and-deref.md
Previously `arguments` was used to pass around an array of arguments,
but this wasn't actually a `js_sys::Array` but rather a somewhat
esoteric internal object. When switching over `Array` methods to be
`structural` this caused issues because the inherent methods on an
`arguments` object were different than that of `js_sys::Array`.
richard-uk1 and others added 11 commits January 16, 2019 10:16
And fix loads of bugs.
This commit adds two new externs for `JSON.stringify`:
`JSON::stringify_with_replacer` and
`JSON::stringify_with_replacer_and_space`.

Fixes rustwasm#1186
js-sys: JSON::stringify_with_replacer[_and_space]
This commit migrates all our examples to using `wasm-pack build` to
compile their code and run `wasm-bindgen`. This should make it a bit
easier to understand the examples as there's less to follow during the
build step.

Webpack projects are all using `@wasm-tool/wasm-pack-plugin` as well so
the build step is simple `npm run serve`. Other examples which retain
`build.sh` are just using `wasm-pack build` now
@chinedufn
Copy link
Contributor Author

chinedufn commented Jan 17, 2019

Sweet thanks!

First question: How should I go about writing tests as I go?

So I poked around the code base and read up on the docs and it looked like web-sys was the place to add tests. Onwards!

richard-uk1 and others added 5 commits January 17, 2019 21:07
This avoids rust-lang/rust#57628 and also generally
makes sense since wasm-bindgen buils on stable.
Better output from `impl Debug for JsValue`.
@@ -1 +1,3 @@
// intentionally left blank

// QUESTION FOR REVIEWER: WHY? (I'll include this context in the comment)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah this is just because Cargo needs to have something like a library or a binary, so this is just a library to pacify cargo, but the library isn't actually used by any of the tests currently

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh thanks! Even though there's a main.rs file?

@chinedufn chinedufn closed this Jan 22, 2019
@chinedufn chinedufn changed the base branch from master to gh-pages January 22, 2019 00:18
@chinedufn chinedufn changed the base branch from gh-pages to master January 22, 2019 00:18
@chinedufn
Copy link
Contributor Author

Err tried changing branches and changing back since I've run into GitHub diff issues before where that fixed it...

Mot sure how this got closed but I can't re-open it so.. opening a new PR sorry..

@chinedufn
Copy link
Contributor Author

Ahhh my diff was off because I pushed to the wrong remote and didn't notice...

Anyways... moved to #1199 ...

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.