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 name alias on type directly for methods #2124

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

a1trl9
Copy link
Contributor

@a1trl9 a1trl9 commented May 6, 2020

This PR tries to fix #2040.

It could also help to address the following case:

#[wasm_bindgen(module = "my-great-module")]
extern "C" {

    #[wasm_bindgen]
    pub type MModule;

    #[wasm_bindgen(constructor, js_class = "JKModule")]
    pub fn new() -> MModule;

    #[wasm_bindgen(static_method_of = MModule)]
    pub fn init();
}

Currently, for snippet above, wasm-bindgen imports both JKModule and MModule, which I don't think is perfect -- in rust code, both methods actually belong to the same type. Probably just importing JKModule would be clearer (or is it by design?).

Now a global name-alias map will be built before determining imports so that for each import we can check if any name-alias exists and if we should import aliased name instead.

Not sure if this modification follows the initial design of js_class -- for globalThis in js-sys, multiple js_class for type global is actually used.

@a1trl9
Copy link
Contributor Author

a1trl9 commented May 6, 2020

Seems it breaks both web-sys and js-sys and path segments are not handled correctly during parsing. Would re-open when it is solved.

@a1trl9 a1trl9 closed this May 6, 2020
@a1trl9 a1trl9 reopened this May 7, 2020
@alexcrichton
Copy link
Contributor

Sorry for the delay in getting to this, but I'm personally sort of hesitant to merge this. This feels like it's a bit of a complicated interpretation of the attributes and requiring a preprocessing pass doesn't really feel great to me. Does this work if you put js_class on all the attributes? Or are there perhaps other alternatives to getting this working?

@a1trl9
Copy link
Contributor Author

a1trl9 commented May 17, 2020

@alexcrichton Thank you very much for your comment.

Well yeah, it does rely too much on current attributes' behaviour and probably not scalable if we wanna add some other features one day. Also, it isn't very efficient to do a heavy preprocessing just for this one purpose. My bad not thinking it very thoroughly and after a few weeks calming down, I also don't feel it is good enough to be merged.

It feels like in wasm-bindgen some tasks (e.g.encoding, code generation) are done one-way which means we don't always know global information of the whole program. For #2040 it is because we can't tell how js_class was (or will be) handled for type when parsing methods. Issue #1853 may be another case. So probably adding more passes to collect global information in backend would be an acceptable long-term solution?

@alexcrichton
Copy link
Contributor

I'm fine with adding more analysis as necessary in the CLI, the speed isn't really what I'm worried about. I'm mostly worried about the maintenance burden about how there's all these weird interactions between all these attributes. No one ever really designed the whole system here, it was just added piecemeal over time and we have what we have. I'm hesitant to continuously do that because it ends up being an unmaintainable mess.

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.

static_method_of shows unexpected behavior
2 participants