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

rustc: Add a #[wasm_import_module] attribute #48992

Closed

Conversation

alexcrichton
Copy link
Member

This commit adds a new attribute to the Rust compiler specific to the wasm
target (and no other targets). The #[wasm_import_module] attribute is used to
specify the module that a name is imported from, and is used like so:

#[wasm_import_module = "./foo.js"]
extern {
    fn some_js_function();
}

Here the import of the symbol some_js_function is tagged with the ./foo.js
module in the wasm output file. Wasm-the-format includes two fields on all
imports, a module and a field. The field is the symbol name (some_js_function
above) and the module has historically unconditionally been "env". I'm not
sure if this "env" convention has asm.js or LLVM roots, but regardless we'd
like the ability to configure it!

The proposed ES module integration with wasm (aka a wasm module is "just another
ES module") requires that the import module of wasm imports is interpreted as an
ES module import, meaning that you'll need to encode paths, NPM packages, etc.
As a result, we'll need this to be something other than "env"!

Unfortunately neither our version of LLVM nor LLD supports custom import modules
(aka anything not "env"). My hope is that by the time LLVM 7 is released both
will have support, but in the meantime this commit adds some primitive
encoding/decoding of wasm files to the compiler. This way rustc postprocesses
the wasm module that LLVM emits to ensure it's got all the imports we'd like to
have in it.

Eventually I'd ideally like to unconditionally require this attribute to be
placed on all extern { ... } blocks. For now though it seemed prudent to add
it as an unstable attribute, so for now it's not required (as that'd force usage
of a feature gate). Hopefully it doesn't take too long to "stabilize" this!

cc rust-lang-nursery/rust-wasm#29

This commit adds a new attribute to the Rust compiler specific to the wasm
target (and no other targets). The `#[wasm_import_module]` attribute is used to
specify the module that a name is imported from, and is used like so:

    #[wasm_import_module = "./foo.js"]
    extern {
        fn some_js_function();
    }

Here the import of the symbol `some_js_function` is tagged with the `./foo.js`
module in the wasm output file. Wasm-the-format includes two fields on all
imports, a module and a field. The field is the symbol name (`some_js_function`
above) and the module has historically unconditionally been `"env"`. I'm not
sure if this `"env"` convention has asm.js or LLVM roots, but regardless we'd
like the ability to configure it!

The proposed ES module integration with wasm (aka a wasm module is "just another
ES module") requires that the import module of wasm imports is interpreted as an
ES module import, meaning that you'll need to encode paths, NPM packages, etc.
As a result, we'll need this to be something other than `"env"`!

Unfortunately neither our version of LLVM nor LLD supports custom import modules
(aka anything not `"env"`). My hope is that by the time LLVM 7 is released both
will have support, but in the meantime this commit adds some primitive
encoding/decoding of wasm files to the compiler. This way rustc postprocesses
the wasm module that LLVM emits to ensure it's got all the imports we'd like to
have in it.

Eventually I'd ideally like to unconditionally require this attribute to be
placed on all `extern { ... }` blocks. For now though it seemed prudent to add
it as an unstable attribute, so for now it's not required (as that'd force usage
of a feature gate). Hopefully it doesn't take too long to "stabilize" this!

cc rust-lang-nursery/rust-wasm#29
@rust-highfive
Copy link
Collaborator

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 13, 2018
@alexcrichton
Copy link
Member Author

Note that this currently doesn't include tests, but I'd like to be sure to include a test once #48883 lands (although I'd prefer to not have too much of a conflict between those two PRs

r? @nikomatsakis

{
self.tcx.sess.span_warn(item.span, "\
must have a #[wasm_import_module = \"...\"] attribute, this \
will become a hard error before too long");
Copy link
Contributor

Choose a reason for hiding this comment

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

you could use a proper forwards compat lint here, but maybe that's overkill?

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps! I didn't turn this on just yet because this attribute is starting off as unstable and figured it'd be a bad experience to force the unstable feture for now at least

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Mar 14, 2018

📌 Commit 8551566 has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 14, 2018
@alexcrichton
Copy link
Member Author

Thanks @nikomatsakis!

I'm gonna try to merge this with #48883 so I can add a test as well, so I'm gonna close this and fold it over there (to avoid rebase conflicts and whatnot)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants