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.

Sign up for GitHub

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

Jump to bottom

Update proposal #14

Merged
merged 6 commits into from
Jan 16, 2024
Merged

Update proposal #14

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e1a47ee
Rename concatenate to concat
eqrion Dec 7, 2023
64219d6
Editorial changes
eqrion Dec 29, 2023
e5d650f
Tighten up definitions of function builtins
eqrion Dec 29, 2023
feb2cef
Rework support for WTF-8
eqrion Dec 29, 2023
440a0f6
Review comments
eqrion Jan 9, 2024
121bd8c
Update substring range checking
eqrion Jan 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 58 additions & 19 deletions proposals/js-string-builtins/Overview.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,6 @@ The following internal helpers are defined in wasm and used by the below definit

```wasm
(module
(type $array_i16 (array i16))
(type $array_i16_mut (array (mut i16)))

(func (export "trap")
Expand All @@ -163,10 +162,10 @@ The following internal helpers are defined in wasm and used by the below definit
local.get 0
array.len
)
(func (export "array_i16_get") (param (ref $array_i16) i32) (result i32)
(func (export "array_i16_mut_get") (param (ref $array_i16_mut) i32) (result i32)
local.get 0
local.get 1
array.get_u $array_i16
array.get_u $array_i16_mut
)
(func (export "array_i16_mut_set") (param (ref $array_i16_mut) i32 i32)
local.get 0
Expand All @@ -183,6 +182,8 @@ The following internal helpers are defined in wasm and used by the below definit
func cast(
string: externref
) -> (ref extern) {
// Technically a partially redundant test, but want to be clear the null is
// not allowed.
if (string === null ||
eqrion marked this conversation as resolved.
Show resolved Hide resolved
typeof string !== "string")
trap();
Expand All @@ -197,6 +198,8 @@ func cast(
func test(
string: externref
) -> i32 {
// Technically a partially redundant test, but want to be clear the null is
// not allowed.
if (string === null ||
typeof string !== "string")
return 0;
Expand All @@ -207,19 +210,19 @@ func test(
### "wasm:js-string" "fromCharCodeArray"

```
/// Convert the specified range of an immutable i16 array into a String,
/// Convert the specified range of a mutable i16 array into a String,
/// treating each i16 as an unsigned 16-bit char code.
///
/// The range is given by [start, end). This function traps if the range is
/// outside the bounds of the array.
///
/// NOTE: This function only takes an immutable i16 array defined in its own
/// NOTE: This function only takes a mutable i16 array defined in its own
/// recursion group.
///
/// If this is an issue for toolchains, we can look into how to relax the
/// function type while still maintaining good performance.
func fromCharCodeArray(
array: (ref null (array i16)),
array: (ref null (array (mut i16))),
start: i32,
end: i32
) -> (ref extern)
Expand All @@ -239,14 +242,14 @@ func fromCharCodeArray(

let result = "";
for(let i = start; i < end; i++) {
let charCode = array_i16_get(array, i);
let charCode = array_i16_mut_get(array, i);
result += String.fromCharCode(charCode);
}
return result;
}
```

### "wasm:js-string" "copyToCharCodeArray"
### "wasm:js-string" "intoCharCodeArray"

```
/// Copy a string into a pre-allocated mutable i16 array at `start` index.
Expand All @@ -255,7 +258,7 @@ func fromCharCodeArray(
/// the string.
///
/// Traps if the string doesn't fit into the array.
func copyToCharCodeArray(
func intoCharCodeArray(
string: externref,
array: (ref null (array (mut i16))),
start: i32
Expand All @@ -268,6 +271,8 @@ func copyToCharCodeArray(
if (array === null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that the previous version of this function's spec would also have trapped when the array is null: implicitly as part of the array.length load.

I'm a little concerned that specifying a specific order of checks/traps might become annoying in the future. For now it's probably fine, but it would sure be unfortunate if N years from now some engine couldn't build the fancy optimization they just thought of because it would observably reorder, say, bounds check and null check traps compared to the previous version of that engine and/or other established engines. Maybe there's just no way to categorically avoid that risk when an operation needs to check more than one thing...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking that doing the upfront bounds check would allow for the most flexibility in implementation by making sure any re-ordering of the loop wouldn't need to preserve the proper access order. But I'm open to filing an issue to discuss this more.

trap();

// Technically a partially redundant test, but want to be clear the null is
// not allowed.
if (string === null ||
typeof string !== "string")
trap();
Expand Down Expand Up @@ -332,6 +337,8 @@ func charCodeAt(
// a JS value using standard conversions. Reinterpret as unsigned here.
index >>>= 0;

// Technically a partially redundant test, but want to be clear the null is
// not allowed.
if (string === null ||
typeof string !== "string")
trap();
Expand All @@ -355,6 +362,8 @@ func codePointAt(
// a JS value using standard conversions. Reinterpret as unsigned here.
index >>>= 0;

// Technically a partially redundant test, but want to be clear the null is
// not allowed.
if (string === null ||
typeof string !== "string")
trap();
Expand All @@ -370,6 +379,8 @@ func codePointAt(

```
func length(string: externref) -> i32 {
// Technically a partially redundant test, but want to be clear the null is
// not allowed.
if (string === null ||
typeof string !== "string")
trap();
Expand Down Expand Up @@ -412,14 +423,15 @@ func substring(
start >>>= 0;
end >>>= 0;

// Technically a partially redundant test, but want to be clear the null is
// not allowed.
if (string === null ||
typeof string !== "string")
trap();

// Ensure the range is ordered and within bounds to avoid the complex
// behavior that `substring` performs when that is not the case.
if (start > end ||
end > string.length)
// Ensure the range is within bounds to avoid the complex behavior that
// `substring` performs when that is not the case.
if (start > string.length)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think checking for start <= end (i.e. taking the return "" path when start > end) was useful, let's keep that.
(I also didn't mean to express strong opposition to what you had here before. If you'd like to return "" for end > string.length, I'm fine with that. I was just pointing out that handling the start < string.length < end case by capping end is rather simple, if we want to allow it.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I reverted the change.

return "";

// [1]
Expand Down Expand Up @@ -483,7 +495,7 @@ The following internal helpers are defined in wasm and used by the below definit
(type $array_i8 (array i8))
(type $array_i8_mut (array (mut i8)))

(func (export "trap")
(func (export "unreachable")
unreachable
)
(func (export "array_len") (param arrayref) (result i32)
Expand All @@ -508,6 +520,25 @@ The following internal helpers are defined in wasm and used by the below definit
)
```

```js
// Triggers a wasm trap, which will generate a WebAssembly.RuntimeError that is
// uncatchable to WebAssembly with an implementation defined message.
function trap() {
// Directly constructing and throwing a WebAssembly.RuntimeError will yield
// an exception that is catchable by the WebAssembly exception-handling
// proposal. Workaround this by executing an unreachable trap and
// modifying it. The final spec will probably use a non-polyfillable
// intrinsic to get this exactly right.
try {
unreachable();
} catch (err) {
// Wasm trap error messages are not defined by the JS-API spec currently.
err.message = IMPL_DEFINED;
throw err;
}
}
```

### "wasm:text-decoder" "decodeStringFromUTF8Array"

```
Expand Down Expand Up @@ -562,7 +593,7 @@ func decodeStringFromUTF8Array(
```
/// Returns the number of bytes string would take when encoded as UTF-8.
///
/// Traps if the string doesn't fit into the array.
/// Traps if the length of the UTF-8 encoded string doesn't fit into an i32
func measureStringAsUTF8(
string: externref,
) -> i32
Expand All @@ -571,9 +602,8 @@ func measureStringAsUTF8(
// to a JS value using standard conversions. Reinterpret as unsigned here.
start >>>= 0;

if (array === null)
trap();

// Technically a partially redundant test, but want to be clear the null is
// not allowed.
if (string === null ||
typeof string !== "string")
trap();
Expand All @@ -594,7 +624,9 @@ func measureStringAsUTF8(

```
/// Encode a string into a pre-allocated mutable i8 array at `start` index using
/// the UTF-8 encoding.
/// the UTF-8 encoding. This uses the replacement character for unpaired
/// surrogates and so it doesn't support lossless round-tripping with
/// `decodeStringFromUTF8Array`.
///
/// Returns the number of bytes written.
///
Expand All @@ -612,6 +644,8 @@ func encodeStringIntoUTF8Array(
if (array === null)
trap();

// Technically a partially redundant test, but want to be clear the null is
// not allowed.
if (string === null ||
typeof string !== "string")
trap();
Expand All @@ -637,10 +671,15 @@ func encodeStringIntoUTF8Array(

```
/// Encode a string into a new mutable i8 array using UTF-8.
eqrion marked this conversation as resolved.
Show resolved Hide resolved
////
/// This uses the replacement character for unpaired surrogates and so it
/// doesn't support lossless round-tripping with `decodeStringFromUTF8Array`.
func encodeStringToUTF8Array(
string: externref
) -> (ref (array (mut i8)))
{
// Technically a partially redundant test, but want to be clear the null is
// not allowed.
if (string === null ||
typeof string !== "string")
trap();
Expand Down