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

Tracking issue for #[wasm_import_module] #52090

Closed
alexcrichton opened this issue Jul 6, 2018 · 23 comments
Closed

Tracking issue for #[wasm_import_module] #52090

alexcrichton opened this issue Jul 6, 2018 · 23 comments
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC O-wasm Target: WASM (WebAssembly), http://webassembly.org/ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

alexcrichton commented Jul 6, 2018

This is a tracking issue for the #[wasm_import_module] attribute and the wasm_import_module feature. This attribute is applied to extern { ... } blocks like so:

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

The WebAssembly specification requires that all imported values from the host environment have a two-level namespace:

All imports include two opaque names: a module name and an import name, which are required to be valid UTF-8. The interpretation of these names is up to the host environment but designed to allow a host environments, like the Web, to support a two-level namespace.

Additionally only globals (like static variables), functions, memories, and tables can be imported. The extern { ... } language block is used to import globals and functions, memories and tables cannot currently be manually imported.

Each field of the wasm import needs to be configurable by Rust as both fields have semantic meaning. Typically the first field of the import is the "module" interpreted as an ES module, and the second field is what to import from that module.

Currently LLVM and LLD will default imports to the "env" module. This means that if you compile this code:

extern {
    fn foo();
}

it will generate a wasm file that imports the function "foo" from the module "env". By using #[wasm_import_module] we can customize what happens here:

#[wasm_import_module = "somewhere else"]
extern {
    fn foo();
}

This attribute must be of the form #[wasm_import_module = "string"], no other forms are accepted. It can only be attached to an extern block. All items in the block are considered to come from the same module. Through the usage of #[link_name] we can then configure both fields of WebAssembly imports arbitrarily:

#[wasm_import_module = "somewhere else"]
extern {
    #[link_name = "some_other_name"]
    fn foo();
}

The #[wasm_import_module] is accepted on non-wasm platforms but has no effect, it is simply an ignored attribute.

Helpful links:

Open questions and TODO

  • Documentation in the reference
  • Should the attribute instead be #[link(wasm_import_module = "...")]?

cc rustwasm/team#82

@alexcrichton alexcrichton added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. B-unstable Blocker: Implemented in the nightly compiler and unstable. O-wasm Target: WASM (WebAssembly), http://webassembly.org/ labels Jul 6, 2018
@alexcrichton
Copy link
Member Author

The WebAssembly target doesn't actually have any concept of a single-level import, so in that sense this doesn't necessarily make sense in wasm:

extern {
    fn foo();
}

We're currently relying on LLVM's defacto behavior to set the import module as "env", but we could also specify it ourselves to do that. Additionally we could also forbid this at compile time and simply require that all extern blocks have a specified module. I would personally lean towards the latter of these interpretations.

@alexcrichton alexcrichton added the C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC label Jul 6, 2018
@alexcrichton
Copy link
Member Author

I'd like to propose that this feature be put up for stabilization but I'm not currently a member of the compiler team to do so, would someone from @rust-lang/compiler like to help me champion this issue and FCP it for stabilization?

Also cc @rust-lang/wg-wasm

@Mark-Simulacrum
Copy link
Member

Is it well-defined for the attribute to have a space as in the tracking issue #[wasm_import_section = "somewhere else"]? That seems counter intuitive to me; I haven't looked, but if not we should have the compiler itself deny invalid identifiers there.

@alexcrichton
Copy link
Member Author

The WebAssembly specification only has the requirement of "required to be valid UTF-8", and beyond that there is no restriction on what the import module can be. We wouldn't want to restrict it to only Rust identifiers because it's frequently an ES module path like ./foo or foo/bar or @foo/bar/baz.

alexcrichton added a commit to alexcrichton/rust that referenced this issue Jul 6, 2018
kennytm added a commit to kennytm/rust that referenced this issue Jul 6, 2018
rustc: Update tracking issue for wasm_import_module

It's now rust-lang#52090
@nagisa
Copy link
Member

nagisa commented Jul 6, 2018

@rfcbot merge

Seems fairly uncontroversial change to me. @alexcrichton summarized this better than I could above, so read that.

AFAIK env is also the most common "section" in use, so this attribute would not see too much of an use, but when some library uses a section other than env, this becomes critical for interop with that library.

@rfcbot
Copy link

rfcbot commented Jul 6, 2018

Team member @nagisa has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jul 6, 2018
@nagisa
Copy link
Member

nagisa commented Jul 6, 2018

This feature will documentation in the book/reference/etc. before being finally stabilised. Please add a TODO to that effect to the text of the issue.

@nagisa
Copy link
Member

nagisa commented Jul 6, 2018

Also a naming bikeshed: couldn’t we reuse #[link] and add a #[link(wasm_section = "foobarbaz")] or something like that instead of coining a new attribute?

@tschneidereit
Copy link

@alexcrichton, I assume the mentions of wasm_import_section should be wasm_import_module?

@Mark-Simulacrum, @nagisa, as @alexcrichton mentioned above, this will be used to import from ES module paths. Once the ES module integration is standardized, this will in fact be used a lot: whenever a Wasm module is loaded as an ES module instead of via the JS API (WebAssembly.instantiateStreaming etc).

@alexcrichton my only concern is that this is somewhat verbose. E.g. compare this JS import

import {
    some_other_name as foo,
    bar
} from "somewhere else";

to this Rust one:

#[wasm_import_section = "somewhere else"]
extern {
    #[link_name = "some_other_name"]
    fn foo();
    fn bar();
}

I suppose this is something where sugar could be added later if it turns out to really be too verbose in practice, though.

@Pauan
Copy link

Pauan commented Jul 6, 2018

I am not a member of the Rust Compiler team (I am a member of the Rust WebAssembly team), however one concern I have is with using wasm_import_module to import a relative ES6 module (which will be a common use-case).

Consider this Rust code:

// path/to/some/crate/foo.rs
#[wasm_import_module = "./bar.js"]
extern {
    ...
}

In this case we have a file foo.rs which is importing a file bar.js (which is in the same folder).

My expectation is that this will work. However, it doesn't work, because after the crate is compiled into .wasm, the path to foo.rs is lost, so it ends up looking for ./bar.js in a completely different (and wrong) folder.

I'm not sure how to fix this issue.

Perhaps it can be fixed entirely by third-party tools like wasm-bindgen and wasm-pack (can it?).

But I suspect it might require some changes to the wasm_import_module attribute, such as saving the path to the .rs file somewhere, so that way any downstream tools (like wasm-pack) can recreate the correct relative path.

@alexcrichton
Copy link
Member Author

@nagisa it's true yeah, going off #[link] would also be fine by me! Although @tschneidereit is right in that wasm_import_section was a mistake above (corrected now), so I think it'd look like:

#[link(wasm_import_module = "...")]
extern {
    // ...
}

@tschneidereit ah thanks for pointing out my mistake! I definitely meant wasm_import_module the whole time! Also it's true yeah that this is a bit wordy, but what I'm hoping for here is the low-level support in rustc as opposed to a high-level attribute. Basically providing the building blocks for tools like wasm-bindgen. With wasm-bindgen these attributes aren't actually ever declared by a user, but rather they're purely internal implementation details.

@Pauan my hope with this attribute (and #[wasm_custom_section]) is to provide the low-level support necessary to take advantage of these features of the wasm spec. I would expect higher level tools to actually process JS files and paths and such, but they need the ability to simply at the lowest level control what rustc does. This is similar to SIMD stabilization where we didn't stabilized an ergonomic interface to SIMD (as that takes quite some time) but rather just the low level tools needed to work with SIMD on stable.

@Pauan
Copy link

Pauan commented Jul 6, 2018

my hope with this attribute (and #[wasm_custom_section]) is to provide the low-level support necessary to take advantage of these features of the wasm spec. I would expect higher level tools to actually process JS files and paths and such, but they need the ability to simply at the lowest level control what rustc does.

Sure, I agree. My concern is this:

  1. You are writing a Rust crate.

  2. Your Rust crate has a dependency on another Rust crate.

  3. That other Rust crate uses wasm_import_module to import a .js file which is local to that crate.

I think that situation will be difficult for wasm-bindgen / wasm-pack to handle, because the wasm_import_module (and the .js file) are in a completely different crate.

Given that, it may be necessary for wasm_import_module to be extended with additional functionality (such as storing the path to the .rs file), but that might be a breaking change, which is why I'm concerned about stabilizing it.

So as long as we have some way of fixing the above problem without requiring breaking changes to wasm_import_module, then I'm okay with stabilizing wasm_import_module.

But I don't see any obvious ways to do that, do you have any ideas?

@alexcrichton
Copy link
Member Author

@Pauan yes I think it'll be difficult to handle but I don't think it'll be a problem in practice. It sounds similar to "what if a crate uses unsafe incorrectly?" or "what if a crate depends on a libc symbol I haven't bound?" Crate authors themselves have to be proactive about what they're using and authors would basically need to understand that using #[wasm_import_module] to import from a local JS file basically doesn't work, they'd need a different mechanism (like what wasm-bindgen/wasm-pack would provide).

I see #[wasm_import_module], like SIMD, as a building block for tool authors and framework authors, not all developers. Not all developers are writing SIMD intrinsics by hand, nor will they by writing custom wasm imports by hand. Rather centralized tools (like the faster crate or wasm-bindgen) will emit these primitives.

@Pauan
Copy link

Pauan commented Jul 7, 2018

@alexcrichton So, what you're saying is that users generally won't use wasm_import_module, instead tools such as wasm-bindgen / wasm-pack / etc. will automatically insert wasm_import_module (after doing the appropriate relative path resolution)?

@alexcrichton
Copy link
Member Author

Indeed yes, that may likely be the case

@Pauan
Copy link

Pauan commented Jul 7, 2018

@alexcrichton Alright, in that case I don't have any concerns.

@eddyb
Copy link
Member

eddyb commented Jul 12, 2018

Don't we have #[link(name = "...")] or something for pretty much this purpose exactly?

@alexcrichton
Copy link
Member Author

@eddyb unfortunately no, this is configuring a two-level namespace which we don't have a configuration for right now for other platforms. Something like #[link(name = "...")] is selecting the name of the library to link

@eddyb
Copy link
Member

eddyb commented Jul 12, 2018

@alexcrichton But isn't the first of the two level-namespace equivalent to the library half of the pair of "library and symbol" from, e.g. Windows?

@alexcrichton
Copy link
Member Author

@eddyb maybe? In that sense wasm has three namespaces because it'll still have archives of compiled C objects which need to be identified with #[link(name = "...")]

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jul 12, 2018
@alexcrichton alexcrichton reopened this Jul 12, 2018
@eddyb
Copy link
Member

eddyb commented Jul 12, 2018

@alexcrichton Ah, this makes more sense, sorry about the confusion!
Any overlap with maybe sections or using commas somewhere? I just expect that the more specific to wasm a mechanism, the more we'll regret it in the future.

@alexcrichton
Copy link
Member Author

Currently no in the sense that wasm doesn't dictate any meaning with the first level of the namespaced import, it's up to bundlers and module instantiators to interpret that

alexcrichton added a commit to alexcrichton/rust that referenced this issue Jul 16, 2018
This commit stabilizes the `#[wasm_import_module]` attribute. Tracked by rust-lang#52090
this issue can be attached to foreign modules (`extern { ... }` blocks) and is
used to configured the module name that the imports are listed with. The
WebAssembly specification indicates two utf-8 names are associated with all
imported items, one for the module the item comes from and one for the item
itself. The item itself is configurable in Rust via its identifier or
`#[link_name = "..."]`, but the module name was previously not configurable and
defaulted to `"env"`. This commit ensures that this is also configurable.

Closes rust-lang#52090
@alexcrichton
Copy link
Member Author

I've submitted #52445 which folds this into the #[link] attribute, which if merged should close this issue.

alexcrichton added a commit to alexcrichton/rust that referenced this issue Jul 18, 2018
This commit stabilizes the `#[wasm_import_module]` attribute as
`#[link(wasm_import_module = "...")]`. Tracked by rust-lang#52090 this new directive in
the `#[link]` attribute is used to configured the module name that the imports
are listed with. The WebAssembly specification indicates two utf-8 names are
associated with all imported items, one for the module the item comes from and
one for the item itself. The item itself is configurable in Rust via its
identifier or `#[link_name = "..."]`, but the module name was previously not
configurable and defaulted to `"env"`. This commit ensures that this is also
configurable.

Closes rust-lang#52090
bors added a commit that referenced this issue Jul 20, 2018
rustc: Stabilize #[wasm_import_module] as #[link(...)]

This commit stabilizes the `#[wasm_import_module]` attribute as
`#[link(wasm_import_module = "...")]`. Tracked by #52090 this new directive in
the `#[link]` attribute is used to configured the module name that the imports
are listed with. The WebAssembly specification indicates two utf-8 names are
associated with all imported items, one for the module the item comes from and
one for the item itself. The item itself is configurable in Rust via its
identifier or `#[link_name = "..."]`, but the module name was previously not
configurable and defaulted to `"env"`. This commit ensures that this is also
configurable.

Closes #52090
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC O-wasm Target: WASM (WebAssembly), http://webassembly.org/ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants