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

wasm_bindgen(constructor) doesn't compile with cfg_attr #1191

Closed
attente opened this issue Jan 16, 2019 · 5 comments · Fixed by #1208
Closed

wasm_bindgen(constructor) doesn't compile with cfg_attr #1191

attente opened this issue Jan 16, 2019 · 5 comments · Fixed by #1208

Comments

@attente
Copy link

attente commented Jan 16, 2019

Hi, it seems that constructors can't be conditionally tagged when using cfg_attr in combination with wasm_bindgen(constructor), for example:

#[cfg_attr(feature = "bindings", wasm_bindgen(constructor))]

The error message I get from attempting this is:

error: expected one of `(`, `async`, `const`, `default`, `existential`, `extern`, `fn`, `type`, or `unsafe`, found `static`
  --> src/lib.rs:19:38
   |
19 |     #[cfg_attr(feature = "bindings", wasm_bindgen(constructor))]
   |                                      ^^^^^^^^^^^^^^^^^^^^^^^^^
@alexcrichton
Copy link
Contributor

Oh dear this looks bad! It looks like we're possibly generating bad code and this may not be related to #[cfg_attr] though. Can you gist the code in question to help us reproduce?

attente added a commit to attente/wasm-bindgen-1191 that referenced this issue Jan 17, 2019
@attente
Copy link
Author

attente commented Jan 17, 2019

Sorry for the delay, I made a repo if you'd like to clone and try it out:
https://github.com/attente/wasm-bindgen-1191

I built it with cargo build --release --target wasm32-unknown-unknown --features bindings.

@alexcrichton
Copy link
Contributor

Hm ok thanks for the reproduction! Something is happening here with procedural macros we weren't expecting which is causing the issue. Regardless though this is a use case we want to support! I'll work on getting this fixed hopefully

@alexcrichton
Copy link
Contributor

Ok I think I know how we can solve this. I'm gonna jot some thoughts down so I don't forget!

The problem is that in this code:

#[foo]
impl Foo {
    #[cfg_attr(a, foo)]
    pub fn new() {}
}

the expansion of the impl doesn't actually see the #[foo] on new because the #[cfg] expansion is deferred until later. In wasm-bindgen we try to do all the expansions immediately, but they don't work out in this case because we can't expand the sub-methods as we don't know whether they'll be included or not.

To solve this I think what we should do is expand the above to:

impl Foo {
    #[cfg_attr(a, foo)]
    #[wasm_bindgen_method_of = Foo]
    pub fn new() {}
}

// other goop generated by `impl Foo { ... }`

basically we rely on recursive macro expansion. We added a synthetic attribute on all methods (at the end). This synthetic attribute will simply return its input if #[wasm_bindgen] isn't present, but otherwise #[wasm_bindgen] will remove the internal attribute.

That way when expanding a method we still know whether it's part of a struct or not, and we should properly handle #[cfg]!

This will be a somewhat involved fix, so it may take some time to implement

alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this issue Jan 25, 2019
Reported in rustwasm#1191 the fix requires us to get a bit creative I think. The
general gist is that a block like this:

    #[wasm_bindgen]
    impl Foo {
        pub fn foo() {}
    }

was previously expanded all in one go. Now, however, it's expanded into:

    impl Foo {
        #[__wasm_bindgen_class_marker(Foo = "Foo")]
        pub fn foo() {}
    }

    // goop generated by orginal #[wasm_bindgen]

This method of expansion takes advantage of rustc's recursive expansion
feature. It also allows us to expand `impl` blocks and allow inner items
to not be fully expanded yet, such as still having `#[cfg]` attributes
(like in the original bug report).

We use theinternal `__wasm_bindgen_class_marker` to indicate that we're
parsing an `ImplItemMethod` unconditionally, and then generation
proceeds as usual. The only final catch is that when we're expanding in
an `impl` block we have to generate tokens for the `Program`
(wasm-bindgen injected goop like the custom section) inside the body
of the function itself instead of next to it. Otherwise we'd get syntax
errors inside of impl blocks!

Closes rustwasm#1191
@alexcrichton
Copy link
Contributor

Ok a fix should be at #1208!

alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this issue Jan 28, 2019
Reported in rustwasm#1191 the fix requires us to get a bit creative I think. The
general gist is that a block like this:

    #[wasm_bindgen]
    impl Foo {
        pub fn foo() {}
    }

was previously expanded all in one go. Now, however, it's expanded into:

    impl Foo {
        #[__wasm_bindgen_class_marker(Foo = "Foo")]
        pub fn foo() {}
    }

    // goop generated by orginal #[wasm_bindgen]

This method of expansion takes advantage of rustc's recursive expansion
feature. It also allows us to expand `impl` blocks and allow inner items
to not be fully expanded yet, such as still having `#[cfg]` attributes
(like in the original bug report).

We use theinternal `__wasm_bindgen_class_marker` to indicate that we're
parsing an `ImplItemMethod` unconditionally, and then generation
proceeds as usual. The only final catch is that when we're expanding in
an `impl` block we have to generate tokens for the `Program`
(wasm-bindgen injected goop like the custom section) inside the body
of the function itself instead of next to it. Otherwise we'd get syntax
errors inside of impl blocks!

Closes rustwasm#1191
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 a pull request may close this issue.

2 participants