Inconsistency in %ArrayIteratorPrototype%.next for TypedArrays

[littledan]

In %ArrayIteratorPrototype%.next, step 8.a., if the Array being iterated over is a TypedArray, the length used is the TypedArray's [[ArrayLength]]. However, if the TypedArray's ArrayBuffer is detached, then the length accessor would've returned zero (see spec). For consistency, should we do the same here?

One fix would be to make step 8.a check for a detached ArrayBuffer and use 0 for the length in that case. I'd like this one, as it leaves in the special allowance to make TypedArrays a bit more implementation-friendly by using the underlying length. Another possibility would be to always use the length accessor.

Thanks to @caitp and @bmeurer for raising this issue to me and discussing possible fixes.

[caitp]

fwiw: the behaviour that was in v8 before the CL in which Benedikt found this is basically what people would end up with if they polyfilled %ArrayIteratorPrototype%.next, and I believe some discussion in the jslang channel on irc.mozilla.org suggested that SpiderMonkey behaves the same way, so maybe there's some weight behind fixing this inconsistency.

[caitp]

fwiw: the behaviour that was in v8 before the CL in which Benedikt found this is basically what people would end up with if they polyfilled %ArrayIteratorPrototype%.next, and I believe some discussion in the jslang channel on irc.mozilla.org suggested that SpiderMonkey behaves the same way, so maybe there's some weight behind fixing this inconsistency.

[bmeurer]

I agree with @caitp, it makes sense to match the polyfill(able) behavior. So +1 for a spec fix.

[bmeurer]

I agree with @caitp, it makes sense to match the polyfill(able) behavior. So +1 for a spec fix.

[allenwb]

In general, the iterative methods of %TypedArrayPrototype% are specified to throw when applied to a typed array instances whose buffer is detached. This throw occurs, because of the detached check in IntegerIndexedElementGet and IntegerIndexedElementSet combined with that fact that the iterative methods directly access [[ArrayLength]] rather than self invoking their length method.

%ArrayIteratorPrototype%.next, as current specified, is consistent with the behavior of the iterative methods. Changing the specification as suggest above would create a new inconsistency.

[allenwb]

In general, the iterative methods of %TypedArrayPrototype% are specified to throw when applied to a typed array instances whose buffer is detached. This throw occurs, because of the detached check in IntegerIndexedElementGet and IntegerIndexedElementSet combined with that fact that the iterative methods directly access [[ArrayLength]] rather than self invoking their length method.

%ArrayIteratorPrototype%.next, as current specified, is consistent with the behavior of the iterative methods. Changing the specification as suggest above would create a new inconsistency.

[caitp]

I think that soec-side inconsistency is less weird for users than the current one

[caitp]

I think that soec-side inconsistency is less weird for users than the current one

[caitp]

s/soec/spec (mobile github, sorry!)

[caitp]

s/soec/spec (mobile github, sorry!)

[allenwb]

Also, there was a reason why we explicitly make the %ArrayPrototype% methods use [[ArrayLength]] rather than self invoking length. I wish I could remember the exact reasoning, maybe it's somewhere in the meeting notes. But, I think it was a legacy compat issue and/or concerns about fact that length property can be redefined.

[allenwb]

Also, there was a reason why we explicitly make the %ArrayPrototype% methods use [[ArrayLength]] rather than self invoking length. I wish I could remember the exact reasoning, maybe it's somewhere in the meeting notes. But, I think it was a legacy compat issue and/or concerns about fact that length property can be redefined.

[caitp]

it's not clear why "length" is configurable, but you have a point. I'm still in favor of treating length as 0 in the detached case, though.

[caitp]

it's not clear why "length" is configurable, but you have a point. I'm still in favor of treating length as 0 in the detached case, though.

[allenwb]

length is configurable because (unlike regular arrays) it is just an accessor property on %TypedArrayPrototype%. Even if it was non-configurable, it could still be over-ridden at the instance level.

More generally, this bug and the meeting note it references is probably relevant to this thread.

[allenwb]

length is configurable because (unlike regular arrays) it is just an accessor property on %TypedArrayPrototype%. Even if it was non-configurable, it could still be over-ridden at the instance level.

More generally, this bug and the meeting note it references is probably relevant to this thread.

[caitp]

I'm not arguing with any of that, but I am saying it's unexpected for

var array = new Uint8Array([1, 2, 3]);
DetachArrayBuffer(array.buffer);
[...A.p.keys.call(array)] // [0, 1, 2]
array.length // 0
[...A.p.entries.call(array)] // TypeError

This is just weird, IMO

// edited for clarity

[caitp]

I'm not arguing with any of that, but I am saying it's unexpected for

var array = new Uint8Array([1, 2, 3]);
DetachArrayBuffer(array.buffer);
[...A.p.keys.call(array)] // [0, 1, 2]
array.length // 0
[...A.p.entries.call(array)] // TypeError

This is just weird, IMO

// edited for clarity

[allenwb]

@caitp I agree the keys example is weird.

How about throwing in 8.a if buffer is detatched? Then keys and values would behave the same.

[allenwb]

@caitp I agree the keys example is weird.

How about throwing in 8.a if buffer is detatched? Then keys and values would behave the same.

[caitp]

I'd vouch for either of the following:

8. If a has a [[TypedArrayName]] internal slot, then
  a. Perform ? ValidateTypedArray(a).
  b. Let len be a.[[ArrayLength]].
9. Else,
  a. Let len be ? ToLength(? Get(a, "length")).

OR

8. If a has a [[TypedArrayName]] internal slot, then
  a. Let buffer be a.[[ViewedArrayBuffer]]
  b. If IsDetachedBuffer(buffer), let len be 0.
  c. Else, let len be a.[[ArrayLength]].
9. Else,
  a. Let len be ? ToLength(? Get(a, "length")).

The latter is what's been implemented for a long time, but it's possible that this isn't observable anywhere so it probably wouldn't break anything to change it. I'm okay with either one.

[caitp]

I'd vouch for either of the following:

8. If a has a [[TypedArrayName]] internal slot, then
  a. Perform ? ValidateTypedArray(a).
  b. Let len be a.[[ArrayLength]].
9. Else,
  a. Let len be ? ToLength(? Get(a, "length")).

OR

8. If a has a [[TypedArrayName]] internal slot, then
  a. Let buffer be a.[[ViewedArrayBuffer]]
  b. If IsDetachedBuffer(buffer), let len be 0.
  c. Else, let len be a.[[ArrayLength]].
9. Else,
  a. Let len be ? ToLength(? Get(a, "length")).

The latter is what's been implemented for a long time, but it's possible that this isn't observable anywhere so it probably wouldn't break anything to change it. I'm okay with either one.

[allenwb]

Well, I vote for a. ValidateTypedArray does some tests that are redundant in this situation, but I assume that implementations will be smart enough to skip those.

Whether or not this is a safe change: what does Safari and Edge do for this case. If any of them follow the current spec. this won't be a change to web interoperable behavior.

[allenwb]

Well, I vote for a. ValidateTypedArray does some tests that are redundant in this situation, but I assume that implementations will be smart enough to skip those.

Whether or not this is a safe change: what does Safari and Edge do for this case. If any of them follow the current spec. this won't be a change to web interoperable behavior.

[caitp]

Whether or not this is a safe change: what does Safari and Edge do for this case. If any of them follow the current spec. this won't be a change to web interoperable behavior.

I doubt it's a web breaking change whether they follow the current spec or not, it seems incredibly unlikely that anyone is actually reaching this branch of spec text in code on the web today

[caitp]

Whether or not this is a safe change: what does Safari and Edge do for this case. If any of them follow the current spec. this won't be a change to web interoperable behavior.

I doubt it's a web breaking change whether they follow the current spec or not, it seems incredibly unlikely that anyone is actually reaching this branch of spec text in code on the web today

[leobalter]

Well, I vote for a. ValidateTypedArray does some tests that are redundant in this situation, but I assume that implementations will be smart enough to skip those.

I'm +1 to it.

I'm also not against the other option if proven necessary.

[leobalter]

Well, I vote for a. ValidateTypedArray does some tests that are redundant in this situation, but I assume that implementations will be smart enough to skip those.

I'm +1 to it.

I'm also not against the other option if proven necessary.

[bterlson]

+1 on ValidateTypedArray. I also highly doubt this will break anyone.

[bterlson]

+1 on ValidateTypedArray. I also highly doubt this will break anyone.

[littledan]

OK, ValidateTypedArray sounds good to me. I wrote a spec change and a test262 test (which fails on current V8). Thoughts?

[littledan]

OK, ValidateTypedArray sounds good to me. I wrote a spec change and a test262 test (which fails on current V8). Thoughts?

[caitp]

Works for me.

[caitp]

Works for me.

[bterlson]

I've added needs-consensus to #724, but would be good to get a look from @jswalden and @msaboff.

[bterlson]

I've added needs-consensus to #724, but would be good to get a look from @jswalden and @msaboff.

[jswalden]

Throwing for during-iteration detaching seems fine to me. I'd just write out the single step of checking for detachment rather than reuse the ill-fitting ValidateTypedArray (and commented to that effect on the PR), but semantically they're the same.

[jswalden]

Throwing for during-iteration detaching seems fine to me. I'd just write out the single step of checking for detachment rather than reuse the ill-fitting ValidateTypedArray (and commented to that effect on the PR), but semantically they're the same.

[littledan]

This pull request gained consensus at the November 2016 TC39 meeting.

[littledan]

This pull request gained consensus at the November 2016 TC39 meeting.