Index conversion in ArrayAccumulation

[GeorgNeis]

ArrayAccumulation computes the array index via ToString(ToUint32(nextIndex+padding)). This means the index may become 2^32-1. But that's not a valid array index anymore since it is the max value of the length property.

[littledan]

What should the behavior be here? I imagine it would be pretty confusing if an Array actually had more than 2^32 elements and the property definitions wrapped around (rather than just overflowing as ordinary property definitions that happen to have property keys which look like numbers, and don't affect the length).

At the same time, I imagine many ECMAScript implementations run into resource limitations when processing such long literals or spreads (this seems to be the case with a version of V8 and SpiderMonkey that I tested long spreads with; maybe that's why you're filing this issue).

I wonder if we should instead throw an SyntaxError when Array literals are longer than 2^32-1, and a RangeError when spreads make an Array literal longer than that length at runtime. The change is likely to be web-compatible due to the resource limitations of these engines.

[GeorgNeis]

I'm very much in favor of throwing once the index reaches 2^32-1. But for simplicity I'd prefer to always throw a RangeError. The distinction you made is not as clear as it sounds.

[littledan]

OK, we already have parse-time ReferenceErrors, so I don't see the problem with a parse-time RangeError. Does anyone else? Any hesitation about restricting the length of array literals this way?

[ljharb]

One thing i like about this approach is that it would necessitate a test262 test that ensures implementations do parse literals that are of 2^32 - 1 items (along with one that ensures it throws at 2^32).

[littledan]

I don't see why it would necessitate that (any more than the current specification does). Resource limitations are outside of the scope of the specification, before and after this change. This change would just add an upper bound.

[allenwb]

The only way this would happen in the real world would be by spreading a run-away iterator. But if it does occur, what happens is fully-specified by exotic array's
[[DefineOwnProperty]].

Note that the behavior would be the same as if Array.from was used with the same iterator. (Except that Array.from imposes a MAX_SAFE_INTERGER limit, to accommodate array subclasses that don't have the 2³²-1 limit).

We discussed such design issues during ES6 development and the consensus was to preserve legacy wrapper behavior for built-in array objects. You get the same results from spending an iterator that produces 2³³ zeros and a writing a for loop that uses an assignment to set the elements of an array from the same iterator.

[littledan]

@allenwb If the current semantics are by design, I'm curious what you think of the bug at the top of the thread here. Do we really intend to wrap around at one greater than the maximum Array index? I'd imagine either not wrapping (and creating arbitrarily many post-Array properties) or wrapping after exceeding 2^31-2 (so no property ever fails to update length).

[allenwb]

OK, I think I see what happened. There was a bug in the ES2015 spec. which was made worse by a subsequent attempt to fix the bug.

Let's start with three axioms:

1. numeric keys ≥ 2³²-1 are not recognized as "array indexes". So when a property is defined on an exotic array object with such a key an ordinary (non-array element) property is created and the value of the length property is not automatically updated.
2. Prior to ES2015 Array.prototype methods, don't (directly) do ToUInt32 conversions on array indices. Any necessary conversions happen in the internal methods of Array exotic objects. ES2015 and subsequent editions didn't change this. (That is fine, it's not the bug.) That means that when these Array methods are forced to create array indices ≥ 2³²-1 on an exotic array yhey will create ordinary properties.
3. The intend of ArrayAccumulation for spread operators is that it should operate similarly to an equivalent operation using a for loop which is also similar to how the built-in array methods operate.

So now, the bug: For axiom 3 to work, the actual computed index (possibly ≥ 2³²-1) must be passed into [[Set]] or [[DefineOwnProperty]] without performing ToUint32 conversion. Just like in the legacy array methods. In the ES2015 spec. that is indeed the case for the SpreadElement : ... AssignmentExpression ArrayAccumulation algorithm. However, the other ElementList : Elision_opt AssignmentExpression and ElementList : ElementList , Elision_opt AssignmentExpression ArrayAccumulation algorithms both did an explicit ToUint32 conversion on the array index before calling CreateDataProperty. That's original ES2015 bug.

Apparently at some point subsequent to ES2015, somebody notices this inconsistency and fixed it by adding a ToUint32 to the SpreadElement : ... AssignmentExpression ArrayAccumulation algorithm. That was the wrong fix. The correct fix would have been to remove the ToUnit32 conversions there were the original bug.

[GeorgNeis]

That makes sense. I think there's still an issue with how the "length" property is set, though, because the new length is also calculated using ToUint32. See https://tc39.github.io/ecma262/#sec-array-initializer-runtime-semantics-evaluation. Perhaps these Set operations should be skipped if the raw value is greater than 2^32-1?

[littledan]

It sounds like the suggested fix is to remove all the ToUint32s in the evaluation of Array literals, and change the length calculation to simply saturate out at 2**32-1. Is that right? What do others on this thread think of that change?

[anba]

Apparently at some point subsequent to ES2015, somebody notices this inconsistency and fixed it by adding a ToUint32 to the SpreadElement : ... AssignmentExpression ArrayAccumulation algorithm. That was the wrong fix. The correct fix would have been to remove the ToUnit32 conversions there were the original bug.

See #153 for background why additional ToUint32 calls were added to ArrayAccumulation and Array Evaluation.

It sounds like the suggested fix is to remove all the ToUint32s in the evaluation of Array literals, and change the length calculation to simply saturate out at 2**32-1.

Replacing all ToUint32 calls with max(<value>, 2**32-1) should work without regressing #153.