Normalize TypedArray, ArrayBuffer and DataView constructors

[leobalter]

Ref #265

On these changes, the offset arguments have a consistent check to throw for values < 0, and length will pass through the ToLength.

[anba]

I'd avoid adding explicit tests to check whether or not an optional parameter is present, if normal type conversions through ToInteger/ToLength are sufficient. And I still think it's important to find out why the current parameter validation steps were added. And if the validation steps in the DataView constructor are changed, we may also want to apply similar changes to GetViewValue/SetViewValue.

[leobalter]

I'd avoid adding explicit tests to check whether or not an optional parameter is present, if normal type conversions through ToInteger/ToLength are sufficient.

I tried to follow other parts handling arguments that are not present, even when they get the undefined value.

And I still think it's important to find out why the current parameter validation steps were added.

I'm on the same boat.

we may also want to apply similar changes to GetViewValue/SetViewValue.

+1, I'll include these

[leobalter]

@anba the new commits are addressing some extras, including the TypedArray (length) constructor.

It still does not answer the parameter validation question. I'll check if I find anything.

[anba]

I tried to follow other parts handling arguments that are not present, even when they get the undefined value.

I guess it depends on the built-ins, for example Array.prototype.copyWithin, Array.prototype.fill, Array.prototype.includes, Array.prototype.indexOf and %TypedArray%.prototype.set all use implicit conversion instead of explicit checks for non-present parameters.

[leobalter]

On the last commit I removed the "not present" steps, but I wanted to keep this one as it still bugs me here.

[leobalter]

@anba ^^

[leobalter]

@littledan I tried to address #468 on 0157b3f.

I reused this same PR as it already touches the same steps. I can split it to a new PR if it works better.

Note that for consistency this PR also removes the RangeError on SameValueZero(numberLength, elementLength). This check is necessary and we might need to use to ToNumber instead of ToInteger if we want to throw for float values as well. I'll do it if necessary.

[littledan]

These changes look good to me, and have the effect of making many things more web-compatible, as per #265. I am not sure, but it still feels like the resulting spec has potential web compatibility concerns, though I'm not nearly as worried. @allenwb What do you think?

[leobalter]

@littledan 361930a

[littledan]

@leobalter the fix-up looks good, modulo web compat concerns. Looks like a faithful implementation of @allenwb's suggestion, and I'm in favor of the spec change given that the compat issue is preexisting.

[jorendorff]

Looks like this is missing a ?.

[leobalter]

I tried to address the last concerns pointed by @jorendorff but I'm afraid I might be bike shedding on the behaviors. I'll try to list them here:

• throw RangeError for non integer or negative lengths
  • _TypedArray_ ( length )
  • _TypedArray_ ( buffer, byteoffset, length )
  • ArrayBuffer( length )
  • DataView( buffer, byteOffset, byteLength )
• take non-present or undefined length argument as 0.
  • _TypedArray_ ( length )
  • _TypedArray_ ( buffer, byteoffset, length )
  • ArrayBuffer( length )
  • DataView( buffer, byteOffset, byteLength )
• length becomes an optional argument
  • _TypedArray_ ( buffer, byteoffset, length )
  • ArrayBuffer( length )
  • DataView( buffer, byteOffset, byteLength )
• accept string values to represent length, as "0"
  • _TypedArray_ ( length )
  • _TypedArray_ ( buffer, byteoffset, length )
  • ArrayBuffer( length )
  • DataView( buffer, byteOffset, byteLength )

[leobalter]

@littledan @bterlson should I add this to the agenda? It might need consensus.

[bterlson]

@leobalter Yeah probably a good idea. I'll add the label.

[zloirock]

@leebyron

throw RangeError for non integer or negative lengths

I already wrote - ws -> socket.io compatibility. You break NodeJS.

[bterlson]

I believe this was discussed at tc39. @leobalter can you summarize the outcome of the discussion? I imagine we are free to fix #265?

[leobalter]

I've got new up-to-date tables including the expected results from the specs (same on ES6, ES7, and draft) and the expected results with the current PR.

The changes on this PR are now updated to reflect this table.

TypedArray( length )

Results for new Int8Array( x ).length:

	0	1.1	0.1	-1	-0	undefined	null	Infinity	NaN
v8	0	1	0	RangeError	0	0	TypeError	RangeError	0
sm	0	TypeError	TypeError	TypeError	0	TypeError	TypeError	TypeError	TypeError
jsc*	0	TypeError	TypeError	RangeError	0	TypeError	TypeError	TypeError	TypeError
edge	0	RangeError	RangeError	RangeError	0	TypeError	TypeError	RangeError	RangeError
specs	0	RangeError	RangeError	RangeError	0	TypeError	0	RangeError	RangeError
PR	0	1	0	RangeError	0	0	0	RangeError	0

* - used MacOS Safari 9.0

TypedArray( buffer, byteOffset, length )

Results for new Int8Array( new ArrayBuffer(128), 0, x ).length:

	n/a	0	1.1	0.1	-1	-0	undefined	null	Infinity	NaN
v8	128	0	1	0	RangeError	0	128	0	RangeError	0
sm	128	0	1	0	RangeError	0	0	0	0	0
jsc*	128	0	1	0	RangeError	0	0	0	0	0
edge	128	0	1	0	RangeError	0	128	0	0	0
specs	128	0	1	0	0	0	128	0	2 ** 53 - 1	0
PR	128	0	1	0	RangeError	0	128	0	RangeError	0

Note: as mentioned at #410 (comment) Infinity length may cause a RangeError at step 14.c:

14. Else,
    a. Let newLength be ? ToLength(length).
    b. Let newByteLength be newLength × elementSize.
    c. If offset+newByteLength > bufferByteLength, throw a RangeError exception.

ArrayBuffer( length )

Results for new ArrayBuffer( x ).byteLength:

	n/a	0	1.1	0.1	-1	-0	undefined	null	Infinity	NaN
v8	0	0	1	0	RangeError	0	0	0	RangeError	0
sm	0	0	1	0	RangeError	0	0	0	0	0
jsc*	0	0	1	0	Error	0	0	0	0	0
edge	0	0	1	0	RangeError	0	0	0	0	0
specs	RangeError	RangeError	RangeError	RangeError	RangeError	0	RangeError	0	RangeError	RangeError
PR	0	0	1	0	RangeError	0	0	0	RangeError	0

DataView( buffer, byteOffset, byteLength )

Results for new DataView( new ArrayBuffer(128), 0, x ).byteLength:

	n/a	0	1.1	0.1	-1	-0	undefined	null	Infinity	NaN
v8	128	0	1	0	0	0	128	0	RangeError	0
sm	128	0	1	0	RangeError	0	128	0	0	0
jsc*	128	0	1	0	RangeError	0	0	0	0	0
edge	128	0	1	0	0	0	128	0	RangeError	0
draft	128	0	1	0	0	0	128	0	2 ** 53 - 1	0
PR	128	0	1	0	RangeError	0	128	0	RangeError	0

Note: as mentioned at #410 (comment) Infinity length may cause a RangeError at step 11.b:

11. Else,
   a. Let viewByteLength be ? ToLength(byteLength).
   b. If offset+viewByteLength > bufferByteLength, throw a RangeError exception.

[littledan]

Excellent data. I'm happy with these semantics; they seem to be good with respect to both consistency and probable web compatibility (at least from an intersection semantics point of view). It's nice that it's shown here that new exceptions can be thrown in some cases where the ES2017 draft spec currently doesn't throw.

@allenwb Have you found more information which you mentioned about previous motivation to not throw errors in some of these cases? Or are you skeptical of some of the checks which are removed here?

[leobalter]

I've also browsed at the Khronos spec, it wasn't helpful to improve my data: https://www.khronos.org/registry/typedarray/specs/latest/ Maybe I should seek for another info from Khronos to find some historical background.

[allenwb]

@leobalter
In your table for: new Int8Array( new ArrayBuffer(128), 0, x ).length
you say the spec. produces 2**53-1 when x is +∞. My reading of both the ES2015 spec. and the current draft is that RangeError should occur. See Step 14.c.
Similar issue for the DateView table. See Ste3p 11.b.
What are you interpreting differently from me?

Regarding the Khronos spec. I think the original author (Ken) just used WebIDL conventions to describe the signatures To understand them you have to look at the Khronos signatures and then look at the conversion rules for the WebIDL types used in the WebIDL spec that was contemporary with the Khronos spec. For example, x in the signature applicable to the above table is specified to be "unsigned long" so the this conversion applies to it.

@littledan Regarding "previous motivations", I was thinking about things like the following. But I didn't finde anything that is immediately relevant. There are also probably closed issues bugs.ecmascript.org that may be relevant.

From: Allen Wirfs-Brock <[email protected]>
Subject: Re: double-checking on typed array indices
Date: February 28, 2014 at 9:34:46 AM PST
To: Luke Wagner <[email protected]>
Cc: Brendan Eich <[email protected]>


On Feb 28, 2014, at 7:36 AM, Luke Wagner wrote:

In
 https://bugzilla.mozilla.org/show_bug.cgi?id=695438#c25
Andre Bargull pointed out that the current definition of what is an indexed property of a typed array in the spec is still pending changes:
 https://bugs.ecmascript.org/show_bug.cgi?id=2049

I think the intended changes sound good, but I just wanted to double check:
- negative integers will unconditionally be undefined (this one is critical to maintain)
- super-large integers will now also be unconditionally undefined (instead of own properties)

Less, that's correct.  Actually any integer >= the length of the typed array unconditionally yields undefined.

There is still a remaining compatibility issue we should probably consider.

It would be nice if using +/-Infinity as an index also yielded undefined.

However, assuming that legacy TypedArray implementations applied the ToUint32 conversion to the index, infinities would be equivalent to 0 as an index.

I can easily imagine that there are buggy programs whose behavior depend upon that behavior and hence we probably need to preserve that behavior.

What do you think? What happens with infinities in your fast paths?

Allen

[leobalter]

In your table for: new Int8Array( new ArrayBuffer(128), 0, x ).length
you say the spec. produces 2**53-1 when x is +∞. My reading of both the ES2015 spec. and the current draft is that RangeError should occur. See Step 14.c.
Similar issue for the DateView table. See Step 11.b.
What are you interpreting differently from me?

That's my bad on merging two different tables I've had made. The new Int8Array( new ArrayBuffer(128), 0, x ).length reflects only the tests for the runtimes. For the specs + pr, the results I presented are expected on the length conversion. I should have considered Step 14.c but I missed that ignoring anything beyond 14.a for the TypedArray and 11.a for the DataView.

On this case, my data is wrong, but the RangeError would be preserved on the spec.

[leobalter]

Regarding the Khronos spec. I think the original author (Ken) just used WebIDL conventions to describe the signatures To understand them you have to look at the Khronos signatures and then look at the conversion rules for the WebIDL types used in the WebIDL spec that was contemporary with the Khronos spec. For example, x in the signature applicable to the above table is specified to be "unsigned long" so the this conversion applies to it.

That's very helpful, I cay update the table with this info if it helps, but I believe these changes are close to that spec, with a few minor differences.

e.g.:

• similar behaviors:
  • non-integer numbers are cast as integers;
  • throwing for negative numbers, Infinity or excessive numbers
• different behaviors:
  • Throwing RangeErrors instead of TypeErrors
  • Casting NaN to 0 instead of throwing a RangeError (I'm not sure about this one, I'm slightly inclined to change it to throw a RangeError too, but I'm following implemenations mostly)
  • The max index is 2 ** 53 - 1 instead of 2 ** 32 - 1.

infinities would be equivalent to 0 as an index

We are not doing this on the constructor, although a few implementations are casting Infinity to 0 as you can notice from the table. That's not reflected on the specs.

[zloirock]

@leobalter sorry, I missed your comment. Sure, conversion floats to integers without error for TypedArray should prevent this problem.

[leobalter]

Thanks, @zloirock!

@allenwb I added notes on #410 (comment) to mention the Infinity values may cause the RangeError.

I also believe it might not trigger the RangeError all the time, assuming offset + newByteLength > bufferByteLength can be false as in: 0 + (2 ** 53 - 1) > (2 ** 53 - 1).

edit: the ToLength will convert +Infinity to 2 ** 53 -1 and the element size can be 1 on *int8* values. and that's happening on steps 14.a and 11.a

[leobalter]

This reached consensus in the last meeting of May 2016.

@bterlson can we merge this? 👍