-
Notifications
You must be signed in to change notification settings - Fork 35
Prepare for integration with String Builtins #95
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
base: main
Are you sure you want to change the base?
Conversation
b1da11a
to
a49d71c
Compare
As written this throws a link error at compile time. We have a few options here:
Thoughts / feedback welcome what might be most suitable // cc @eqrion |
For now, I've changed the error to be a Compile error per (2) above. |
There is actually a benefit to doing this error late in that source phase imports can still support polyfilling the |
Double checking my understanding here. So if you have On a browser that doesn't support js-string builtins.
On a browser that does support js-string builtins.
Is that accurate? |
Yes, that's a correct summary, with the same rules applying to any future builtins. |
We might want to let them override it in the supporting js-string browser. That way if they have some bug in their polyfill they keep getting the polyfill. On the other hand they could still get that wrong as folks do in JS. Since wasm isn't a source language, I would expect folks are less likely to camp on names the committee would want though... Ideally, there would be some magic key that sites have to have to get the built-in version and that magic key is only picked/published once a browser actually ships the feature. That way browsers can turn on the built-in and sites have to redeploy to actually start using it. I don't know how we would do that though... but maybe this comment will inspire someone smarter than me to figure it out! |
Yes, we have the possibility to hard-code a value here under the I'll also post up a new web platform test to ensure this behaviour is captured in the ESM Integration WPT. Would be great to get a review / approval before I go ahead further though if someone can take a look. |
I think if an override behaviour is desired, then that should be supported in the custom instantiation workflows by allowing importObj to override instead of having string builtins always take precedence. But that would be a normative change in the string builtins to avoid polyfill upgrade issues on the esm integration path. Other options might be to have a new setting And of course the import attributes option for setting compile options can also apply in due course. But the first two items above are probably the best bet. @eqrion do you think the concern of broken polyfill upgrade paths / permitting builtin virtualization is worth considering a feature like this for? |
So this results in |
@annevk currently today, string builtins are never exposed to the module system and behave as implicit bindings more analogous to JS globals ( This PR explicitly adds a new feature to the ESM integration to make it stricter about handling other That is Today anyone can write a JS file containing:
And an import map containing:
And polyfill eg Node.js builtins. The Wasm scheme as defined here is actually more strict than JS, and that makes sense because it is strictly not an import scheme but a lower level implicit compile time import binding scheme. The discussions here have then been about polyfill paths for unknown builtins in the WebAssembly.instantiate(mod, { 'wasm:unknown-builtin': { ... } }) would be useful just like we can do for Does that help clarify your understanding of the situation? |
I think so, but this makes |
@annevk I'm unclear on how this makes If the error is the issue we could change this PR to no longer throw for the |
I've added a commit here to remove the @eqrion @kmiller68 @lukewagner I would highly value an approval to be able to move this forward and get the Node.js implementation merged. |
One nice property of the JS string builtins, as they are currently specced, is that they are polyfillable. Not only as a whole today, but also partially in the future. If a later proposal adds new functions in the existing string builtins module, it is possible to polyfill only the new functions when they are not implemented yet, while benefitting from the fast native implementation of the (older) existing builtins. Moreover, when an engine adds support for a new function, we automatically get the better implementation. Is it possible to preserve these very nice properties in the ESM integration? |
Despite the |
One way we can retain polyfillability while not making the This way, any future compile time imports remain implementable and thus polyfillable, in both the evaluation and source phases (via import maps or custom Wasm instantiation respectively), and we don't have any matching rule on imports for the This makes the tradeoff that adding new builtins in future may be seen as a breaking change of sorts, but that might be possible if we explicitly ensure Wasm toolchains never use the The opt-out clause here then might be to allow import attributes to customize the explicit compile-time imports instead of relying on the default - For now I've updated to this approach, let's discuss the tradeoffs as part of the meeting agenda further, but we do need to come to a resolution on this soon for implementations to ship. |
@lukewagner wouldn't it be the case that if a |
IIUC, this allows new separate modules to be polyfilled (like a hypothetical |
I'm not familiar with all the subtleties here, so I may be missing something. Who would standardize a URL scheme for |
It was pointed out to me that I might have been misled by |
@annevk as far as I'm interpreting the feedback, are you saying that the handling here that explicitly excludes the known compile time builtin string list is preferable to an approach that filters for a In both cases, these special import names are specific to Wasm and entirely internal to Wasm though - everything happens at compilation such that that they aren't exposed to the ECMA module system at all ( @sjrd I think polyfilling individual functions is a question for the string builtins proposal itself, it would be great to see that discussion tracked as an issue in that repo if you are able to follow-up on this? |
This was already discussed, and is already explained in the explainer, starting here: |
For what it's worth, a TAG review was done of the original js-string-builtins proposal [1]. It was closed though with a comment that the TAG lacked the right 'domain expertise' for this area. The original proposal also didn't integrate with ESM yet, so that part was lacking. Wasm builtins have a smaller scope than the original built-in modules proposal (AFAICT). They're a space for operations on JS primitives or JS builtin objects that are around the size of a wasm instruction. It's not intended for general Web API's or even most of the JS builtin API's. @guybedford From reading the discussion here, I'm remembering some of the subtleties around polyfilling in the JS-API part of the proposal. While it'd be nice to reserve the 'wasm:' prefix here for flexibility in future evolution, that seems like it would make polyfilling not work unless users were to ship different import names for different versions of browsers (which is a probably a non-starter). Reserving just specifiers that a browser currently implements (like 'wasm:js-string'), would prevent polyfiling new additions to an existing specifier (as @sjrd points out). Both of those use-cases work with the Wasm JS-API because (1) the users opts-in to the exact set of builtin collections (e.g. js-string) they want. Anything not requested falls back to normal instantiation. And (2) if a user imports a function from a builtin collection that is not supported by the browser, we fall back to normal instantiation for that individual import. The Wasm JS-API design does have the risk that when a wasm engine implements a new builtin, it could break a polyfill that has a bug/quirk that the user is relying on as their imports are overridden automatically by the engine (as @kmiller68 points out). I don't remember if this was discussed or if there is a good alternative to avoid this. For the JS-API this only happens in the case where an engine adds a new builtin to an existing collection, as that's the case where you automatically get the builtin to override anything you provided. Maybe that's acceptable? I wonder if the following design for ESM integration would work:
This would give us a better polyfilling story than if we had reserved everything under e.g. for the issue in #95 around what name to use for the string constants feature, the CG will have multiple options for names to pick in the future even if the whole 'wasm:' prefix is not reserved. The important bit for string constants is just that we don't have a good name to use today because we have binary size concerns. So I'd prefer to just not pick anything right away. |
@eqrion these are good suggestions and I agree. For (1), I've created an issue to follow up on the attribute in #99, and I hope to have a PR up shortly. For (2) I've followed your suggestion as described by changing the integration to explicitly use the find a builtin algorithm and noting this semantic only works when fully merged with the string builtins spec. Note, unless I'm misinterpreting the spec, I believe this also requires the upstream fix WebAssembly/js-string-builtins#46 to work as you describe. |
@annevk Sorry to butt in a bit, but I want to clarify that when you see It is true that if some other entity decides to specify a |
Yes, that is the concern. |
This prepares for the integration with string builtins, without explicitly rebasing to it yet.
We add a note to all spec steps that apply on the merge path:
builtins: ['js-string']
and not providing a string import module for now