New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integer Indexed Exotic Objects inconsistent wrt Reflect.get/set/has #347

Closed
verwaest opened this Issue Feb 2, 2016 · 3 comments

Comments

Projects
None yet
5 participants
@verwaest

verwaest commented Feb 2, 2016

According to the spec, [[get]] (9.4.5.4) and [[set]] (9.4.5.5) only ignore CanonicalNumericIndexStrings if SameValue(O, Receiver). [[HasProperty]] however does not have such a check since it doesn't even get passed in the Receiver.

var o = {__proto__: Int32Array(100)};
Object.prototype[1.3] = 100;
// In 9.4.5.4 we don't go down IntegerIndexedElementGet since
// SameValue(O, Receiver) is false.
assertEquals(100, o[1.3])
// In 9.4.5.2 we do return false since the CanonicalNumericIndexString
// branch isn't guarded by SameValue(O, Receiver).
assertFalse(Reflect.has(o, 1.3));

This is inconsistent. I see two options to make this more consistent:

  1. Optionally pass in Receiver to Reflect.has, or
  2. remove SameValue(O, Receiver) from [[get]] and [[set]].

I personally prefer the second, since we can't pre-check whether target is receiver in the context of Reflect.get. You can easily do Reflect.get(target, <CanonicalNumericIndexString>, typed_array_in_prototype_chain_of_target), causing the [[get]] to be blocked once we reach the typed array.

@allenwb

This comment has been minimized.

Show comment
Hide comment
@allenwb

allenwb Feb 2, 2016

Member

I think the second alternative is probably a good idea.

The receiver check is enforcing that typed array element access do not occur when a typed array is access via prototype lookup.

When this was specified we were trying to clean up array access corner cases and implementation differences of typed arrays. Apparently at the time I thought that inheriting access to typed array elements was one of the things we wanted to precluded.

However, It isn't clear to me now why this case is important and it does create the inconsistency you have identified. I suspect that at least some implementation have not implemented it which case it is probably pretty safe to change. However, somebody should test the current status of implementions WRT the receiver check.

Member

allenwb commented Feb 2, 2016

I think the second alternative is probably a good idea.

The receiver check is enforcing that typed array element access do not occur when a typed array is access via prototype lookup.

When this was specified we were trying to clean up array access corner cases and implementation differences of typed arrays. Apparently at the time I thought that inheriting access to typed array elements was one of the things we wanted to precluded.

However, It isn't clear to me now why this case is important and it does create the inconsistency you have identified. I suspect that at least some implementation have not implemented it which case it is probably pretty safe to change. However, somebody should test the current status of implementions WRT the receiver check.

@rossberg

This comment has been minimized.

Show comment
Hide comment
@rossberg

rossberg Feb 2, 2016

Member
Member

rossberg commented Feb 2, 2016

kisg pushed a commit to paul99/v8mips that referenced this issue Feb 3, 2016

[runtime] Remove receiver==holder check in IntegerIndexedExotic lookup
This was inconsistent in the spec in case of has vs get, set. Removing
receiver==holder simplifies the lookup; so tentatively removing this
additional check which was broken until yesterday anyway. See
tc39/ecma262#347 for more information.

Review URL: https://codereview.chromium.org/1660903002

Cr-Commit-Position: refs/heads/master@{#33701}
@evilpie

This comment has been minimized.

Show comment
Hide comment
@evilpie

evilpie Feb 20, 2016

Contributor

SpiderMonkey never had this check: https://bugzilla.mozilla.org/show_bug.cgi?id=1216621

Contributor

evilpie commented Feb 20, 2016

SpiderMonkey never had this check: https://bugzilla.mozilla.org/show_bug.cgi?id=1216621

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment