Editorial: Refactor index checking for Integer-Indexed exotic objects

[syg]

This refactor was prompted by the confusing -0 check in Integer-Indexed exotic objects' [[DefineOwnProperty]] trap. Usually the index validity checking is done in IntegerIndexedElementGet and IntegerIndexedElementSet, but that trap does some extra checks because it might not fall through to IntegerIndexedElementSet due to a non-data property descriptor.

[anba]

The index check in [[DefineOwnProperty]] must be kept at the same position, otherwise this is a normative change, because the ToNumber/ToBigInt conversion in IntegerIndexedElementSet is observable.

[syg]

The asserts are still valid indeed, I'll reinstate them.

…

On Thu, Oct 24, 2019, 14:58 Jordan Harband ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In spec.html <#1752 (comment)>: > <emu-clause id="sec-integerindexedelementget" aoid="IntegerIndexedElementGet"> <h1>IntegerIndexedElementGet ( _O_, _index_ )</h1> <p>The abstract operation IntegerIndexedElementGet with arguments _O_ and _index_ performs the following steps:</p> <emu-alg> - 1. Assert: Type(_index_) is Number. why is this assert no longer valid? (ideally all argument types are asserted at the top of every abstract operation) ------------------------------ In spec.html <#1752 (comment)>: > @@ -9505,16 +9507,12 @@ <h1>IntegerIndexedElementGet ( _O_, _index_ )</h1> <h1>IntegerIndexedElementSet ( _O_, _index_, _value_ )</h1> <p>The abstract operation IntegerIndexedElementSet with arguments _O_, _index_, and _value_ performs the following steps:</p> <emu-alg> - 1. Assert: Type(_index_) is Number. why is this assert no longer valid? ------------------------------ In spec.html <#1752 (comment)>: > 1. If IsAccessorDescriptor(_Desc_) is *true*, return *false*. 1. If _Desc_ has a [[Configurable]] field and if _Desc_.[[Configurable]] is *true*, return *false*. 1. If _Desc_ has an [[Enumerable]] field and if _Desc_.[[Enumerable]] is *false*, return *false*. 1. If _Desc_ has a [[Writable]] field and if _Desc_.[[Writable]] is *false*, return *false*. 1. If _Desc_ has a [[Value]] field, then 1. Let _value_ be _Desc_.[[Value]]. 1. Return ? IntegerIndexedElementSet(_O_, _numericIndex_, _value_). + 1. If IsIntegerIndexValid(_O_, _numericIndex_) is *false*, return *false*. per @anba <https://github.com/anba>'s comment, i think this needs to move up to line 9401? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1752?email_source=notifications&email_token=AAAXLUICCMVEUTM6DWIK3P3QQIK63A5CNFSM4JENFEBKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCJFL2CA#pullrequestreview-306887944>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAXLUKJOPVCU6VKBVN3ZDTQQIK63ANCNFSM4JENFEBA> .

[syg]

Addressed review.

[jmdyck]

The copy-paste error in the attributes of the new <emu-clause> tag needs to be fixed. The other stuff is optional.

It looks like a correct refactoring to me.

[jmdyck]

I think it would make more sense to have the parameter-asserts in the same order as the parameters. (Despite that IntegerIndexedElementGet/Set have them in the above order.)

[syg]

I'll reorder all the relevant asserts.

[ljharb]

Suggested change

	1. If IsInteger(_index_) is *false*, return *false*.
	1. If ! IsInteger(_index_) is *false*, return *false*.

[ljharb]

Suggested change

	1. If IsValidIntegerIndex(_O_, _index_) is *false*, return *undefined*.
	1. If ! IsValidIntegerIndex(_O_, _index_) is *false*, return *undefined*.

[ljharb]

Suggested change

	1. If IsValidIntegerIndex(_O_, _index_) is *false*, return *false*.
	1. If ! IsValidIntegerIndex(_O_, _index_) is *false*, return *false*.

[ljharb]

Suggested change

	1. If IsValidIntegerIndex(_O_, _numericIndex_) is *false*, return *false*.
	1. If ! IsValidIntegerIndex(_O_, _numericIndex_) is *false*, return *false*.

[ljharb]

Suggested change

	1. If IsValidIntegerIndex(_O_, _numericIndex_) is *false*, return *false*.
	1. If ! IsValidIntegerIndex(_O_, _numericIndex_) is *false*, return *false*.

[syg]

@ljharb You know, in general I don't agree with the editorial direction that ! be prefixed on invocations of AOs that cannot throw because they do not return completion values. ! and ? telegraph to me that the AO returns a completion value, since those are shorthands for unwrapping, and I'd prefer to use ! only for an AO that can throw, but will not for that particular invocation.

That said, there is so much prior use of ! in the way I disagree with, I'll incorporate your desired changes here.

[ljharb]

Every abstract op always returns a completion value, implicitly.

The drive for using ! or ? at callsites is that, otherwise, you can't possibly know if an abstract op call "can throw" or not, so the ! gives you the only info you need: either that it can't throw, or that it won't throw with these arguments. Whether it can throw in other cases isn't helpful information.