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

Support WebIDL namespaces #253

Closed
fitzgen opened this issue Jun 8, 2018 · 15 comments
Closed

Support WebIDL namespaces #253

fitzgen opened this issue Jun 8, 2018 · 15 comments
Labels
frontend:webidl Issues related to the WebIDL frontend to wasm-bindgen help wanted We could use some help fixing this issue!

Comments

@fitzgen
Copy link
Member

fitzgen commented Jun 8, 2018

namespace whatever {
    void double(long x);
}

I think we want to emit a wrapper module around the emitted bindings for these, eg the equivalent of:

pub mod whatever {
    #[wasm_bindgen]
    extern {
        #[wasm_bindgen(js_namespace = whatever)]
        pub fn double(x: i23) -> i32;
}

We will need to add support for this at the backend::ast level.

@fitzgen fitzgen added help wanted We could use some help fixing this issue! frontend:webidl Issues related to the WebIDL frontend to wasm-bindgen labels Jun 8, 2018
@richard-uk1
Copy link
Contributor

I'm looking at this and I don't think the backend::Program supports modules yet. Is this correct, and how would you modify the data structures to incorporate modules? Would you need the ability to nest modules (I would say yes, since you can have namespaces within namespaces)?

@migerh
Copy link

migerh commented Jul 16, 2018

The webidl grammar does not allow nested namespaces. Inside a namespace only readonly attributes and regular operations are allowed.

I had a look at this issue myself recently. Mostly just looking and trying to understand the existing code base. Here's what I got so far: I added support for parsing NonpartialNamespaces and added an AST node to store the namespace name and operations defined in the namespace. After that I added an ToToken impl for the namespace node. Now I'm stuck on how to get the codegen to render the function definitions inside the module. It is already possible to define the module correctly, but I don't understand enough of the TokenStream stuff yet to actually insert the functions into the module.

I also had to introduce additional WebidlParse impls for the Operation nodes so I can add references of the functions to their respective namespace. The existing implementations simply add them to the program and the only connection to their class/interface seems to be the class name as a string.

@richard-uk1
Copy link
Contributor

I had a look too but I didn't get as far as you. Is your code online somewhere?

@migerh
Copy link

migerh commented Jul 16, 2018

Everything I have so far is here: https://github.com/migerh/wasm-bindgen/tree/wip-namespace-and-console

It'll build, but the console test will fail of course. The console module is there, but there are no functions in it.

@richard-uk1
Copy link
Contributor

thankyou :)

@richard-uk1
Copy link
Contributor

I'm glad you posted that. I'd independently duplicated a lot of your code :P. I'm not sure how to generate tokens for the module, but I'm guessing it will be like a struct/interface, in that there are an unknown collection of interior objects (in this case functions) inside an enclosing object (in this case mod). I'll have a look to see if I can finish your work if you like, or i'll leave it to you if you prefer? Or we can both work on it and see who can figure it out :P

@migerh
Copy link

migerh commented Jul 19, 2018

@derekdreery Maybe we can crack this together :) I can now generate something that at least somewhat resembles the suggestion above. Two issues are open atm:

  • In the bindings.rs the js_namespace is enclosed with quotes. wasm-bindgen can't parse that. For now I'm patching this manually by removing the quotes from the generated bindings.rs.
  • The test can't find the functions inside the module although they're there.

@richard-uk1
Copy link
Contributor

richard-uk1 commented Jul 19, 2018 via email

@richard-uk1
Copy link
Contributor

Unimportant observation: I wonder if we should use ImportModule rather than ImportNamespace, since we are generating modules from our ast (this seems to be the pattern the other variants take).

@richard-uk1
Copy link
Contributor

If you want to look at my WIP it's at derekdreery/wasm-bindgen#wip-namespace-and-console.

Something I find helpful is to run rustfmt on the output of the build process, at target/wasm32-unknown-unknown/debug/build/web-sys-xxx/out/bindings.rs (where xxx is the metadata hash for your build). You can then view it as rust code and see if it's correct.

@fitzgen
Copy link
Member Author

fitzgen commented Aug 10, 2018

I think we can use an alternative codegen tactic for namespaces, the equivalent to:

// namespace apollo {
//     bool thirteen(x: unsigned long);
// }
#[wasm_bindgen]
extern {
    type apollo;

    #[wasm_bindgen(static_method_of = apollo)]
    fn thirteen(x: u32) -> bool;
}

Details here: #678 (comment)

@richard-uk1
Copy link
Contributor

Implemented as modules in #678 .

@ohanar
Copy link
Member

ohanar commented Aug 13, 2018

We still need support for namespace attributes, although that shouldn't be much work at this point. I'm going to leave this open for now.

@alexcrichton
Copy link
Contributor

@ohanar perhaps we could open a separate issue for that with some updated instructions?

@fitzgen
Copy link
Member Author

fitzgen commented Aug 23, 2018

Filed #746 for attributes within namespaces. Closing this issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend:webidl Issues related to the WebIDL frontend to wasm-bindgen help wanted We could use some help fixing this issue!
Projects
None yet
Development

No branches or pull requests

5 participants