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 support for unstable WebIDL #1997

Merged
merged 6 commits into from
Feb 26, 2020
Merged

Conversation

grovesNL
Copy link
Contributor

@grovesNL grovesNL commented Feb 12, 2020

As a follow-up to #1896, #1950, and #1972, I started to look into supporting unstable WebIDL through RUSTFLAGS=--cfg=web_sys_unstable_apis as mentioned by @alexcrichton

In this PR the idea is to have an attribute like [WebSysUnstableApi] on all experimental WebIDL definitions. During parsing, this attribute can be recognized, and we can attach #[cfg(web_sys_unstable_apis)] on any blocks which reference these unstable definitions.

For example, the definition of:

[WebSysUnstableApi]
interface GPUBindGroup

becomes something like:

#[cfg(web_sys_unstable_apis)]
pub struct GpuBindGroupBinding

This PR tries to cover the common types of WebIDL definitions (enums, interfaces, partial interfaces, etc.). It doesn't currently handle exports used internally, such as

pub extern "C" fn __wbindgen_describe___widl_f_copy_texture_to_texture_with_gpu_extent_3d_dict_GPUCommandEncoder

(I believe these exports are harmless, but we could hide these too) Edit: looks like this hiding these is necessary

Some possible pros/cons to this approach:

  • We can mark certain parts of an API as stable or unstable which could be useful to extend existing APIs
  • It's difficult to know whether this PR updates all parts of the parser and code-gen that we need, so this might be error-prone (i.e. we might accidentally leak some experimental API). The changes in general feel a bit invasive, so I'd be interested if there's a better way to approach these changes
  • Annotating the WebIDL takes a bit of effort and could be error-prone, compared to moving it into a special "experimental" directory instead of "enabled"
    • Moving WebIDL into a separate "experimental" directory has some other challenges though – currently we parse all WebIDL files as a single String and splitting them apart might have some subtle effects (i.e. de-duplicating types which are shared)

I'd be interested in any feedback, especially whether there's a better solution or even a way to simplify passing these unstable_api flags around. In general this approach seems to work fine for me so far, but I'd be happy to rework this PR entirely.

@Pauan
Copy link
Contributor

Pauan commented Feb 12, 2020

As an alternative, what if we kept the experimental WebIDL in a separate experimental folder, and then added a new argument to wasm_bindgen_webidl::compile, so it would be like this:

fn compile(webidl_source: &str, experimental_source: &str, allowed_types: Option<&[&str]>) -> Result<String>

Now it can generate a single string as usual, but there is a separation between stable and unstable without needing a new WebSysUnstableApi attribute.

@Pauan
Copy link
Contributor

Pauan commented Feb 12, 2020

P.S. Thanks a lot for working on this! This has been bugging me for quite a while now.

@grovesNL
Copy link
Contributor Author

As an alternative, what if we kept the experimental WebIDL in a separate experimental folder

I looked into this briefly, but I wasn't sure how to split both sources in parse and later combine their ASTs (Programs) back together. Maybe this change wouldn't be too difficult though.

For example, any time we see partial interfaces like Navigator, we would probably need to keep them separate during parsing, because it's possible for the interface to be stable but have some unstable attributes attached to it (readonly attribute GPU gpu)

@Pauan Pauan mentioned this pull request Feb 13, 2020
@alexcrichton
Copy link
Contributor

Thanks for this! I would agree that it would be best if we could avoid editing the *.webidl that much (such as adding all the custom attributes). Would @Pauan's suggestion be workable? For inclusions like the fields and such, could mixins be edited into the *.webidl file?

@grovesNL
Copy link
Contributor Author

Alright sounds good, I'll try to make the separate directory work

@grovesNL
Copy link
Contributor Author

Ok it seems reasonably straightforward to move these into a separate directory and remove the attribute from the WebIDL definitions. I'll update this PR again once I work through a couple remaining issues

@grovesNL
Copy link
Contributor Author

Updated the PR again to use an unstable directory – let me know what you think.

The nightly failures seem unrelated to this PR (https://dev.azure.com/rustwasm/wasm-bindgen/_build/results?buildId=2042&view=logs&j=aef2040c-a7c2-5c59-fd4c-9a8df33dc1b6&t=529981cf-1b81-530f-0edf-a26fa1e08bdd&l=42)

@alexcrichton
Copy link
Contributor

Looks great to me, thanks!

I think that lots of WebSysUnstableApi annotations are still present though? Could the *.webidl be directly imported now except for perhaps a small edit to include an extra mixin to make sure the global property is available?

@grovesNL
Copy link
Contributor Author

Thanks for taking a look! The annotations were left in by mistake while rebasing but should be fixed now.

@alexcrichton
Copy link
Contributor

Ok you can disregard my previous comment about editing webidl files, this looks great!

A few final asks before merging:

  • Can you be sure to add tests to CI that we build web-sys with this feature flag?
  • Can you configure docs.rs to pass this flag when generating documentation?
  • Can all unstable APIs be documented that they need this --cfg flag to get passed when compiling? This could probably reference the documentation in the next bullet.
  • Additionally, can you add documentation about the feature flag? For example how to pass it, when to pass it, how to use RUSTFLAGS, etc.

@grovesNL grovesNL force-pushed the cfg-unstable branch 2 times, most recently from 24cc294 to 0942bf9 Compare February 26, 2020 14:33
@grovesNL
Copy link
Contributor Author

@alexcrichton could you check the 4 latest commits to see if they address the points in your comment? I'm not sure if we can verify the docs.rs change until it's published

@alexcrichton
Copy link
Contributor

Looks great, thanks!

Mind also updating the doc building to document unstable APIs?

@grovesNL
Copy link
Contributor Author

Thanks! I've fixed the typo and updated the docs CI step

@alexcrichton alexcrichton merged commit 99c59a7 into rustwasm:master Feb 26, 2020
@alexcrichton
Copy link
Contributor

Thanks so much!

@grovesNL grovesNL changed the title [WIP] Add support for unstable WebIDL Add support for unstable WebIDL Feb 26, 2020
@grovesNL grovesNL deleted the cfg-unstable branch February 26, 2020 23:39
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.

3 participants