-
Notifications
You must be signed in to change notification settings - Fork 478
Merge js-string-builtins into wasm-3.0 #1943
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: wasm-3.0
Are you sure you want to change the base?
Conversation
* Adopts builtin modules approach * Adds section of polyfilling * Adds section on feature detection * Adds cast/test builtins * Adds future extension ideas for - binding memory - utf8/wtf8 - evolving the type signatures
Update proposal
- Eliminate usage of 'builtin module' in description. This is not essential to the proposal and causes confusion around a similarly named JS proposal, which had different goals. - Clarify some minor points. - Make JS-API changes to WebIDL comprehensive. - Reword feature detection section to actually propose change to WebAssembly.validate method
- Function builtin behaviors is defined using 'create a host function' - Clarify behavior around monkey patching using standard language - Clarify edge cases around nullability - Clarify edge cases around unsigned/signed integers - Restrict 'substring' behavior to normal cases - Use wasm helpers for when wasm instructions are needed
The existing WTF-8 operation in this proposal violated one of the goals of the proposal: "don't create substantial new functionality" by introducing WTF-8 transcoding support to the web platform without prior precedent. The WTF-8 operation is removed because of this. The naming for WTF-16 operations is reworked to refer to 'charCodes' instead as that is what the JS String interface uses. We could support UTF-8 transcoding by referring to the TextEncoder/TextDecoder interfaces, so this commit adds support for that.
Explain that users should validate modules that deliberately produce link errors to test for support for particular builtins.
This commit adds a basic suite of tests for the js-string-builtins. This is done by defining a polyfill module matching the overview, and then comparing the host provided builtins against the polyfill on representative inputs.
- Syntax highlighting - Added links - Code font - Consistency - Grammer fixes
Needs more detail to properly integrate with GC array ops.
This avoids having to refer to actual Wasm instructions, since after all this is a host function.
Equals specifically allows null inputs. Update the JS API tests and the polyfill to match.
I'll let @Ms2ger review thia, since it only touches the JS specs. |
Thanks - no obvious issues jump out at me while skimming; I'll try to take a slightly more thorough look by next week. Should the tests still be marked as tentative? |
Good catch, updated. |
The WG voted to advance js-string-builtins to phase 5 today, so I don't think we are blocked on that anymore. |
I opened eqrion#1 to clarify that the function types for each of the builtins must also be in their own rec groups, etc. V8 is currently shipping a bug where we accept any function type that has the correct signature, but we'll try to fix it. It would be good to add tests for this as well. |
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.
Some formatting comments. Thanks!
1. Let |importExternType| be |import|[2] | ||
1. Let |stringExternType| be `global const (ref extern)` | ||
1. If [=match_externtype=](|stringExternType|, |importExternType|) is false, return false |
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.
1. Let |importExternType| be |import|[2] | |
1. Let |stringExternType| be `global const (ref extern)` | |
1. If [=match_externtype=](|stringExternType|, |importExternType|) is false, return false | |
1. Let |importExternType| be |import|[2]. | |
1. Let |stringExternType| be `global const (ref extern)`. | |
1. If [=match_externtype=](|stringExternType|, |importExternType|) is false, return false. |
@@ -384,53 +398,104 @@ Note: | |||
</div> | |||
|
|||
<div algorithm> | |||
The <dfn method for="WebAssembly">validate(|bytes|)</dfn> method, when invoked, performs the following steps: | |||
To <dfn>validate builtins and imported string for a WebAssembly module</dfn> from module |module|, enabled builtins |builtinSetNames|, and |importedStringModule|, perform the following steps: |
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.
For new algorithms, please follow the formatting of "retrieving a host value", that is, empty line after <div>
and before </div>
, an empty line between the header and the list, and start on column 0. (I haven't made the spec consistent yet because of diff churn, but I probably should after this batch of proposals.)
1. Let |builtinSetNames| be |options|["builtins"] | ||
1. Let |importedStringModule| be |options|["importedStringConstants"] |
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.
1. Let |builtinSetNames| be |options|["builtins"] | |
1. Let |importedStringModule| be |options|["importedStringConstants"] | |
1. Let |builtinSetNames| be |options|["builtins"]. | |
1. Let |importedStringModule| be |options|["importedStringConstants"]. |
1. Let |builtinSetNames| be |options|["builtins"] | ||
1. Let |importedStringModule| be |options|["importedStringConstants"] |
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.
1. Let |builtinSetNames| be |options|["builtins"] | |
1. Let |importedStringModule| be |options|["importedStringConstants"] | |
1. Let |builtinSetNames| be |options|["builtins"]. | |
1. Let |importedStringModule| be |options|["importedStringConstants"]. |
<div algorithm> | ||
To <dfn>instantiate imported strings</dfn> with module |module| and |importedStringModule|, perform the following steps: | ||
1. Assert: |importedStringModule| is not null. |
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.
<div algorithm> | |
To <dfn>instantiate imported strings</dfn> with module |module| and |importedStringModule|, perform the following steps: | |
1. Assert: |importedStringModule| is not null. | |
<div algorithm> | |
To <dfn>instantiate imported strings</dfn> with module |module| and |importedStringModule|, perform the following steps: | |
1. Assert: |importedStringModule| is not null. |
<div algorithm> | ||
To <dfn>get the builtins for a builtin set</dfn> with |builtinSetName|, perform the following steps: |
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.
<div algorithm> | |
To <dfn>get the builtins for a builtin set</dfn> with |builtinSetName|, perform the following steps: | |
<div algorithm> | |
To <dfn>get the builtins for a builtin set</dfn> with |builtinSetName|, perform the following steps: |
and below
To <dfn>find a builtin</dfn> with |import| and enabled builtins |builtinSetNames|, perform the following steps: | ||
|
||
1. Assert: [=validate builtin set names=] |builtinSetNames| is true. | ||
1. Let |importModuleName| be |import|[0] |
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.
1. Let |importModuleName| be |import|[0] | |
1. Let |importModuleName| be |import|[0]. |
Please check the rest of this section.
<div algorithm> | ||
To <dfn>create a builtin function</dfn> from type |funcType| and execution steps |steps|, perform the following steps: | ||
|
||
1. Let |stored settings| be the <a spec=HTML>incumbent settings object</a>. |
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.
stored settings is unused
|
||
The JS-API defines sets of builtin functions which can be imported through {{WebAssemblyCompileOptions|options}} when compiling a module. WebAssembly builtin functions mirror existing JavaScript builtins, but adapt them to be useable directly as WebAssembly functions with minimal overhead. | ||
|
||
All builtin functions are grouped into sets. Every builtin set has a |name| that is used in {{WebAssemblyCompileOptions}}, and a |qualified name| with a `wasm:` prefix that [=read the imports|is used during import lookup=]. |
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.
name and qualified name should be definitions (with for=builtin-set
or similar) and be cross-referenced as such below
|
||
<div algorithm> | ||
|
||
The <dfn abstract-op lt="CharCodeAt ">CharCodeAt(|string|, |index|)</dfn> abstract operation, when invoked, performs the following steps: |
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.
The <dfn abstract-op lt="CharCodeAt ">CharCodeAt(|string|, |index|)</dfn> abstract operation, when invoked, performs the following steps: | |
The <dfn abstract-op lt="CharCodeAt">CharCodeAt(|string|, |index|)</dfn> abstract operation, when invoked, performs the following steps: |
There are no substantive open issues on the js-string-builtins repo right now, this has shipped in two browsers now (Chrome and Firefox), and so I'd like to start the process of merging this into the spec. I originally based this off the wasm-3.0 branch, so that's what I'm basing this PR onto.
From my read of the process document, js-string-builtins needs to be phase 5 before merging into the spec repo. So I've filed an agenda item for advancing js-string-builtins to phase 5 here. I think we could start review of this before then to catch any potential issues that may need to be discussed with the WG. If not, I'll just leave this here until after the next WG meeting.
cc' @rossberg @Ms2ger as editors.