Editorial: Move attachedness check into bounds chk

[jugglinmike]

@syg This is an attempt to implement the change you suggested here.

One aspect of this that I was not expecting is the effect on the IntegerIndexObjectLength operation. With this change in place, that operation also verifies buffer attachedness. This has two implications which feel somewhat unideal to me:

• the resulting semantics are a little indirect: the "length" of a integer-indexed object of a detached buffer will be reported as zero. For example, at first glance:
  • [[OwnPropertyKeys]] may appear to be iterating over keys regardless of the buffer's attachedness
  • IsValidIntegerIndex may not appear to be checking boundary conditions
• I don't think we can completely avoid redundant attachedness checks. It seems to me that the following abstract operations will need to explicitly check for attachedness before performing the out-of-bounds check (and implicitly checking for attachedness a second time):
  • SetTypedArrayFromTypedArray
  • InitializeTypedArrayFromTypedArray
  • ValidateAtomicAccess (the existing attachedness checks are present elsewhere in ECMA262, but I presume refactoring that would be more invasive than you'd like)

What do you think?

[syg]

I think this is correct, thanks for the refactoring.

I agree with your indirectness concern, since the detach check is now under IsIntegerIndexedObjectOutOfBounds. Maybe we should rename it to IsIntegerIndexedObjectDetachedOrOutOfBounds, WDYT?

[syg]

Please add an assert and a note:

1. Assert: IsDetachedBuffer(_O_.[[ViewedArrayBuffer]]) is *false*.
1. NOTE: When the buffer is detached, IntegerIndexedObjectLength returns 0.

[syg]

I don't think we can completely avoid redundant attachedness checks. It seems to me that the following abstract operations will need to explicitly check for attachedness before performing the out-of-bounds check (and implicitly checking for attachedness a second time):

Some redundancy isn't the end of the world if the alternative is really unreadable. I'd have to see the alternatives side by side to judge, though, so happy to defer to your judgment here for now.

[jugglinmike]

I think this is correct, thanks for the refactoring.

My pleasure!

Please add an assert and a note:

1. Assert: IsDetachedBuffer(_O_.[[ViewedArrayBuffer]]) is *false*.
1. NOTE: When the buffer is detached, IntegerIndexedObjectLength returns 0.

Done

I agree with your indirectness concern, since the detach check is now under IsIntegerIndexedObjectOutOfBounds. Maybe we should rename it to IsIntegerIndexedObjectDetachedOrOutOfBounds, WDYT?

I dunno. Despite its length, the name IsIntegerIndexedObjectDetachedOrOutOfBounds is still somewhat imprecise: the integer-indexed object isn't detached, after all.

It's not the out-of-bounds check that's bugging me, though. I think that the concept of attachedness fits reasonably well inside the concept of bounds checking. "Any typed array or view becomes out of bounds once its buffer is detached." I think people reading this text would presume that.

I'm not confident that a reader, seeing that an array reports a length of 0, would necessarily recognize that the attachedness of its buffer had been consulted at all.

What if IntegerIndexedObjectLength returned null for detached and out-of-bounds arrays? Many call sites would immediately convert null to 0 which, while more verbose, would surface the deail we've agreed is "indirect." More importantly, handling the null value in the other call sites is a more natural way to call this out:

• it will be clear that [[OwnPropertyKeys]] is sensitive to out-of-bounds arrays
• it will be clear that IsValidIntegerIndex is sensitive to out-of-bounds arrays (maybe even clear enough to obviate the assertion and Note that you've recommended)

Here's a patch based on top of this branch.

[jugglinmike]

Ah, but then there's this:

• Uses of [[ArrayLength]] on Integer-Indexed exotic objects are replaced with calls to IntegerIndexedObjectLength.

That makes the idea of explicit "null to zero" conversions a little harder to justify...

[syg]

What if IntegerIndexedObjectLength returned null for detached and out-of-bounds arrays?

I like this idea. The only suggestion I'd make is to use a sentinel like ~detached-buffer~ instead of null.

That makes the idea of explicit "null to zero" conversions a little harder to justify...

Oh don't worry about that, the explicitness seems worth it for readability.

[jugglinmike]

What if IntegerIndexedObjectLength returned null for detached and out-of-bounds arrays?

I like this idea. The only suggestion I'd make is to use a sentinel like ~detached-buffer~ instead of null.

Totally! A sentinel is much easier to search for, and it makes the meaning of this condition even more clear. Though I'd like to suggest ~out-of-bounds~ instead.

For IntegerIndexedObjectLength to report a detached buffer specifically, it would need to duplicate the IsDetachedBuffer check that we're putting in IsIntegerIndexedObjectOutOfBounds. Instead, my latest commit reports the more general condition, ~out-of-bounds~. Doing so avoids duplicating the check, and it reinforces the relationship we've been discussing between detachedness and out-of-bounds-ness.

While that feels more cohesive to me, it also underscores the fact that we're not explaining how detachedness relates to being "out of bounds." Not only that, but by my reading, the current definition is incorrect:

The abstract operation IsIntegerIndexedObjectOutOfBounds takes arguments O and getBufferByteLength. It checks if any part of the underlying viewed buffer is out of bounds. It performs the following steps when called:

That seems to describe the inverse of the true behavior. After all, it's legal for a buffer to extend beyond the boundary of the array. Whether my reading is correct or not, a more explicit definition might avoid confusion and alert readers to the relevance of detached buffers:

The abstract operation IsIntegerIndexedObjectOutOfBounds takes arguments O and getBufferByteLength. It checks if any of the object's numeric properties describe an undefined sequence of byte data block values. It performs the following steps when called:

Does that seem like an improvement to you? If so, should we add something similar to IsViewOutOfBounds?

That makes the idea of explicit "null to zero" conversions a little harder to justify...

Oh don't worry about that, the explicitness seems worth it for readability.

Great!

[jugglinmike]

Any thoughts on this, @syg?

[syg]

Thanks for the ping, this had fallen off my radar.

[syg]

I see the confusion about the wording for the IsIntegerIndexedObjectOutOfBounds AO.

It checks if any of the object's numeric properties describe an undefined sequence of byte data block values.

Hm, I don't think "describe an undefined sequence of byte data block values" is that helpful for the reader. Undefined in what sense? Maybe it's not wholly out of bounds, and the reader might expect that part of the sequence should be defined.

Maybe something like "It checks if any of the object's numeric properties reference a value at an index out of the underlying data block's bounds"?

[syg]

This is my fault since I forgot about this PR for so long, but could you please rebase? Thank you.

[jugglinmike]

I see the confusion about the wording for the IsIntegerIndexedObjectOutOfBounds AO.

It checks if any of the object's numeric properties describe an undefined sequence of byte data block values.

Hm, I don't think "describe an undefined sequence of byte data block values" is that helpful for the reader. Undefined in what sense? Maybe it's not wholly out of bounds, and the reader might expect that part of the sequence should be defined.

Maybe something like "It checks if any of the object's numeric properties reference a value at an index out of the underlying data block's bounds"?

Could we get just a little farther from the phrase "out of bounds"? How do you feel about: "It checks if any of the object's numeric properties reference a value at an index not contained within the underlying data block's bounds."

I can tell I'm getting pedantic because I've also started to question what it means to have a property. Do you think anyone would argue that, due to the internal methods of the indexed-integer exotic objects, typed arrays don't actually have numeric properties?

This is my fault since I forgot about this PR for so long, but could you please rebase? Thank you.

Sure; the conflicts came from gh-62. In the interest of transparency (and under the assumption that you'll be "squash merging" this patch), I've resolved the conflicts with a merge commit. I'm happy to do the squashing and force-push the result to this branch at your request.

[ljharb]

Typed arrays observably have numeric properties; the internal implementation details aren’t relevant.

[syg]

"It checks if any of the object's numeric properties reference a value at an index not contained within the underlying data block's bounds."

Sure, that sounds fine to me.

I can tell I'm getting pedantic because I've also started to question what it means to have a property. Do you think anyone would argue that, due to the internal methods of the indexed-integer exotic objects, typed arrays don't actually have numeric properties?

No, I don't think anyone would argue that. This is a sentence describing at a high-level what the AO does, and it doesn't need to exactly describe what the AO does. For that, readers can read the actual algorithm steps.