-
Notifications
You must be signed in to change notification settings - Fork 35
Support direct global bindings #104
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
Conversation
d13bd74
to
1fd242e
Compare
I've significantly simplified the approach here by removing the extra spec work added for resolving cyclic bindings, to instead discuss any possible cycle support separately. |
1834063
to
e46cfcc
Compare
e46cfcc
to
fd68140
Compare
document/js-api/index.bs
Outdated
1. Let |resolutionInstance| be |resolution|.\[[Module]].\[[Instance]]. | ||
1. If |resolutionInstance| is ~empty~ then, | ||
1. Throw a {LinkError} exception. | ||
1. Let |resolutionModule| be |resolution|.\[[Module]].\[[ModuleSource]].\[[Module]]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(this is confusing, I thought [[Module]] was the Module Record corresponding to [[ModuleSource]] rather than a WebAssembly module, but I don't think we can improve it anyway)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it's unfortunate we have [[Module]]
on both a resolve binding record and a WebAssembly.Module module object. Perhaps resolve binding records could refactor at some point to use [[ResolvedModule]]
or something like that to ensure clarity in this kind of scenario.
I've split out resolutionModule
and resolutionName
and also added some assertions to make the relationships clearer.
document/js-api/index.bs
Outdated
1. [=list/Append=] |externval| to |imports|. | ||
1. Otherwise, | ||
1. Let |env| be |resolution|.\[[Module]].\[[Environment]]. | ||
1. Let |v| be [=?=] |env|.GetBindingValue(|resolution|.\[[BindingName]], true). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if [[BindingName]] is NAMESPACE? e.g. if I import foo
from a module that does export * as foo from "./bar.js"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are reading directly from the environment record here, so NAMESPACE
cannot be a value in an environment record in ECMA-262, you'd get the namespace object representation instead.
1. Note: The condition above leaves unsupported JS values as uninitialized in TDZ and therefore as a reference error on | ||
access. When integrating with shared globals, they may be excluded here similarly to v128 above. | ||
1. Perform [=!=] |record|.\[[Environment]].InitializeBinding(|name|, [=ToJSValue=](|global_value|)). | ||
1. If |mut| is [=var=], then associate all future mutations of |globaladdr| with the ECMA-262 binding record for |name| in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fwiw, https://tc39.es/ecma262/#sec-createimportbinding is similarly vague.
1. Note: The condition above leaves unsupported JS values as uninitialized in TDZ and therefore as a reference error on | ||
access. When integrating with shared globals, they may be excluded here similarly to v128 above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we do this instead of simply not exposing them, making it a linking error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great point and thanks for spotting this. The reason is that we need these bindings to work when Wasm modules import other Wasm modules. That is, while these values are not expressible in JS, they should still be exposed as exports to other Wasm. TDZ is therefore a useful way to represent this property.
Co-authored-by: Andreas Rossberg <[email protected]>
Instance lookup function for Wasm Namespaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spec text looks good
This updates the specification to provide the unwrapping of
WebAssembly.Global
values in the exports of WebAssembly module instances, while fully retaining the existing support of Wasm to Wasm global binding and ensuring constant and mutable globals are only ever fulfilled by their same corresponding type.This was originally discussed in the early stages of this proposal, and then brought up recently again in #103.
With this change,
import { exportVal } from './mod.wasm'
wheremod.wasm
exports a mutable global, can reflect the live direct JS value forexportVal
in JS of that mutable global without requiring theexportVal.value
accessor normally needed for instances ofWebAssembly.Global
. We effectively get live direct bindings on exports, and also for imports to other Wasm modules, while continuing to have snapshotted bindings on non-Wasm imports.Live bindings for JS modules are supported for Wasm mutable globals due the infallible nature of
ToJSValue()
allowing us to create an association between the JS environment record and the Wasm global.The way we handle Wasm to Wasm imports is to store the
[[Instance]]
on the WebAssembly module record, so that we can access the original global address from the instance when resolving to another WebAssembly Module record. All other types retain their existing handling. Since Wasm to Wasm imports no longer have the wrapping and unwrapping through JS which relied on implicit subtyping, we instead directly perform the same extern subtype checks of the WebAssembly value imports, using the corresponding core definition for this.This PR was raised at the WasmCG, and obtained 6 votes for, 15 neutral and 1 against.
Concern from the 1 vote against, was subsequently resolved in #108 which was also landed into this PR after discussion in #107, allowing for a new
WebAssembly.namespaceInstance
function for obtaining the instance from a given namespace:As a WebIDL function taking a ModuleNamespace, this is also refactored with an associated WebIDL PR in whatwg/webidl#1483.