Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Normative: IntegerIndexedElementSet should always indicate success #2210

Merged
merged 1 commit into from
Dec 16, 2020
Merged

Normative: IntegerIndexedElementSet should always indicate success #2210

merged 1 commit into from
Dec 16, 2020

Conversation

rkirsling
Copy link
Member

@rkirsling rkirsling commented Oct 21, 2020 •
edited
Loading

Resolves #2208.

This behavior is necessitated by web reality and tied to #2164 (although the OOB path is technically separate).

@syg and I chatted about whether the (non-normative) description of [[Set]] in Table 6 needs to be updated and concluded "no" for two reasons:

  1. It is extremely difficult to update this verbiage to be exhaustive without explicit mention of typed arrays.
  2. The meaning of the verb "set" is already ambiguous thanks to userland setters. In other words, it can be argued that the property value was "set" just by virtue of having entered IntegerIndexedElementSet.

@rkirsling rkirsling added the editor call to be discussed in the next editor call label Oct 21, 2020
@bakkot
Copy link
Contributor

bakkot commented Oct 21, 2020

I note that this operation can no longer return any normal completion except true. I wonder if it should be refactored to not return anything.

Also, this probably merits a NOTE to the effect of "This operation always appears to succeed, although it does not have any effect when writing past the end of the TypedArray or to a TypedArray which is backed by a detached ArrayBuffer".

@rkirsling
Copy link
Member Author

@bakkot I definitely agree about the note, but I'm not certain that the return value refactor is clearer. I've added it here as a third commit, so we can discuss whether we want to keep it or not.

@rkirsling rkirsling changed the title IntegerIndexedElementSet should always return true IntegerIndexedElementSet should always indicate success Oct 21, 2020
@rkirsling rkirsling changed the title IntegerIndexedElementSet should always indicate success Normative: IntegerIndexedElementSet should always indicate success Nov 3, 2020
@syg syg removed the editor call to be discussed in the next editor call label Nov 4, 2020
caiolima pushed a commit to caiolima/webkit that referenced this pull request Nov 12, 2020
Align %TypedArray% behavior with recent spec adjustments
5d29948 
https://bugs.webkit.org/show_bug.cgi?id=218776

Reviewed by Yusuke Suzuki.

JSTests:

* stress/reflect-set.js:
* stress/typedarray-functions-with-neutered.js:
* stress/typedarray-includes.js:
* stress/typedarray-indexOf.js:
* stress/typedarray-join.js: Added.
* stress/typedarray-lastIndexOf.js:
Update tests.

* test262/expectations.yaml:
Mark a handful of test cases as temporarily failing.
These will disappear in a future test262 update.

Source/JavaScriptCore:

The recent spec changes for typed arrays with detached buffers had certain ripple effects,
namely the following two PRs which will be presented in next week's TC39 meeting.
Since no controversy is expected, this patch addresses them now, though test262 adjustments are forthcoming.

1. tc39/ecma262#2210
   It is correct that `ta[i] = n` doesn't throw when `ta` has a detached buffer or `i` is otherwise OOB,
   but by not throwing, Reflect.set(ta, i, n) is obliged to return true.

2. tc39/ecma262#2221
   Until now, %TypedArray%.prototype.{includes, indexOf, join, lastIndexOf} lacked a rigorous specification;
   in particular, each has a parameter that may detach the buffer upon valueOf or toString, and the expected
   behavior was not made clear. It seems most sensible to do what the corresponding Array methods do upon
   `array.length = 0`: make use of the cached length but don't access indices, such that indexOf/lastIndexOf
   return -1 while includes/join act as if the elements were all `undefined`.

* runtime/JSGenericTypedArrayViewInlines.h:
(JSC::JSGenericTypedArrayView<Adaptor>::put):
(JSC::JSGenericTypedArrayView<Adaptor>::defineOwnProperty):
(JSC::JSGenericTypedArrayView<Adaptor>::putByIndex):
* runtime/JSGenericTypedArrayViewPrototypeFunctions.h:
(JSC::genericTypedArrayViewProtoFuncIncludes):
(JSC::genericTypedArrayViewProtoFuncIndexOf):
(JSC::genericTypedArrayViewProtoFuncJoin):
(JSC::genericTypedArrayViewProtoFuncLastIndexOf):


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@269670 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@bakkot bakkot added has consensus This has committee consensus. needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 labels Nov 16, 2020
@bakkot
Copy link
Contributor

bakkot commented Nov 16, 2020

@rkirsling This got consensus, but I'm not sure if there's tests for it yet. Do you know?

@rkirsling
Copy link
Member Author

@rwaldron's been helping me with the tests for #2164 and its various ripple effects. Rick, would you have time to look at this (and #2221)? Lemme know if it's getting to be too much. 😅

rwaldron added a commit to tc39/test262 that referenced this pull request Nov 25, 2020
@rwaldron
IntegerIndexedElementSet should always indicate success. Ref: tc39/ec…
f5d0bf1 
…ma262/pull/2210
@bakkot bakkot mentioned this pull request Nov 28, 2020
Merged
rwaldron added a commit to tc39/test262 that referenced this pull request Dec 2, 2020
@rwaldron
IntegerIndexedElementSet should always indicate success. Ref: tc39/ec…
939811a 
…ma262/pull/2210
rwaldron added a commit to tc39/test262 that referenced this pull request Dec 2, 2020
@rwaldron
IntegerIndexedElementSet should always indicate success. Ref: tc39/ec…
96aff62 
…ma262/pull/2210
@rkirsling rkirsling added has test262 tests and removed needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 labels Dec 8, 2020
@rkirsling rkirsling force-pushed the integerindexedelementset-must-return-true branch from f5e7bb9 to 869005a Compare December 10, 2020 20:23
@ljharb ljharb requested review from michaelficarra, syg and a team December 12, 2020 07:40
syg
syg approved these changes Dec 16, 2020
View reviewed changes
Copy link
Contributor

@syg syg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks!

@bakkot bakkot added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Dec 16, 2020
@ljharb ljharb force-pushed the integerindexedelementset-must-return-true branch from 869005a to ba09250 Compare December 16, 2020 04:52
@ljharb ljharb merged commit ba09250 into tc39:master Dec 16, 2020
@rkirsling rkirsling deleted the integerindexedelementset-must-return-true branch December 16, 2020 05:45
ryanhaddad pushed a commit to WebKit/WebKit that referenced this pull request Dec 22, 2020
@rkirsling
Align %TypedArray% behavior with recent spec adjustments
e6ac510 
https://bugs.webkit.org/show_bug.cgi?id=218776

Reviewed by Yusuke Suzuki.

JSTests:

* stress/reflect-set.js:
* stress/typedarray-functions-with-neutered.js:
* stress/typedarray-includes.js:
* stress/typedarray-indexOf.js:
* stress/typedarray-join.js: Added.
* stress/typedarray-lastIndexOf.js:
Update tests.

* test262/expectations.yaml:
Mark a handful of test cases as temporarily failing.
These will disappear in a future test262 update.

Source/JavaScriptCore:

The recent spec changes for typed arrays with detached buffers had certain ripple effects,
namely the following two PRs which will be presented in next week's TC39 meeting.
Since no controversy is expected, this patch addresses them now, though test262 adjustments are forthcoming.

1. tc39/ecma262#2210
   It is correct that `ta[i] = n` doesn't throw when `ta` has a detached buffer or `i` is otherwise OOB,
   but by not throwing, Reflect.set(ta, i, n) is obliged to return true.

2. tc39/ecma262#2221
   Until now, %TypedArray%.prototype.{includes, indexOf, join, lastIndexOf} lacked a rigorous specification;
   in particular, each has a parameter that may detach the buffer upon valueOf or toString, and the expected
   behavior was not made clear. It seems most sensible to do what the corresponding Array methods do upon
   `array.length = 0`: make use of the cached length but don't access indices, such that indexOf/lastIndexOf
   return -1 while includes/join act as if the elements were all `undefined`.

* runtime/JSGenericTypedArrayViewInlines.h:
(JSC::JSGenericTypedArrayView<Adaptor>::put):
(JSC::JSGenericTypedArrayView<Adaptor>::defineOwnProperty):
(JSC::JSGenericTypedArrayView<Adaptor>::putByIndex):
* runtime/JSGenericTypedArrayViewPrototypeFunctions.h:
(JSC::genericTypedArrayViewProtoFuncIncludes):
(JSC::genericTypedArrayViewProtoFuncIndexOf):
(JSC::genericTypedArrayViewProtoFuncJoin):
(JSC::genericTypedArrayViewProtoFuncLastIndexOf):


Canonical link: https://commits.webkit.org/231449@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@269670 268f45cc-cd09-0410-ab3c-d52691b4dbfc
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Dec 22, 2020
@anba
Bug 1672862 - Part 1: Change TypedArray indexed properties to report …
be0125b 
…as configurable. r=tcampbell

Implements the changes from <tc39/ecma262#2164> and
<tc39/ecma262#2210> for [[GetOwnProperty]]. Part 3
will update [[DefineOwnProperty]] and part 5 updates [[Set]].

A side-effect of this change is that non-extensible, non-empty TypedArrays are
no longer considered as sealed by `Object.isSealed()`.

A later patch in this series will update test262 to enable more currently
skipped TypedArray tests.

Differential Revision: https://phabricator.services.mozilla.com/D99380
sidvishnoi pushed a commit to sidvishnoi/gecko-webmonetization that referenced this pull request Dec 24, 2020
@anba
Bug 1672862 - Part 1: Change TypedArray indexed properties to report …
ebd9e7d 
…as configurable. r=tcampbell

Implements the changes from <tc39/ecma262#2164> and
<tc39/ecma262#2210> for [[GetOwnProperty]]. Part 3
will update [[DefineOwnProperty]] and part 5 updates [[Set]].

A side-effect of this change is that non-extensible, non-empty TypedArrays are
no longer considered as sealed by `Object.isSealed()`.

A later patch in this series will update test262 to enable more currently
skipped TypedArray tests.

Differential Revision: https://phabricator.services.mozilla.com/D99380
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Jan 1, 2021
@marco-c
Bug 1672862 - Part 1: Change TypedArray indexed properties to report …
6e752ba 
…as configurable. r=tcampbell

Implements the changes from <tc39/ecma262#2164> and
<tc39/ecma262#2210> for [[GetOwnProperty]]. Part 3
will update [[DefineOwnProperty]] and part 5 updates [[Set]].

A side-effect of this change is that non-extensible, non-empty TypedArrays are
no longer considered as sealed by `Object.isSealed()`.

A later patch in this series will update test262 to enable more currently
skipped TypedArray tests.

Differential Revision: https://phabricator.services.mozilla.com/D99380

UltraBlame original commit: efd88e38955fc4722c6a882c175fdb48d6ff9e10
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Jan 1, 2021
@marco-c
Bug 1672862 - Part 1: Change TypedArray indexed properties to report …
0044543 
…as configurable. r=tcampbell

Implements the changes from <tc39/ecma262#2164> and
<tc39/ecma262#2210> for [[GetOwnProperty]]. Part 3
will update [[DefineOwnProperty]] and part 5 updates [[Set]].

A side-effect of this change is that non-extensible, non-empty TypedArrays are
no longer considered as sealed by `Object.isSealed()`.

A later patch in this series will update test262 to enable more currently
skipped TypedArray tests.

Differential Revision: https://phabricator.services.mozilla.com/D99380

UltraBlame original commit: efd88e38955fc4722c6a882c175fdb48d6ff9e10
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Jan 1, 2021
@marco-c
Bug 1672862 - Part 1: Change TypedArray indexed properties to report …
64adff8 
…as configurable. r=tcampbell

Implements the changes from <tc39/ecma262#2164> and
<tc39/ecma262#2210> for [[GetOwnProperty]]. Part 3
will update [[DefineOwnProperty]] and part 5 updates [[Set]].

A side-effect of this change is that non-extensible, non-empty TypedArrays are
no longer considered as sealed by `Object.isSealed()`.

A later patch in this series will update test262 to enable more currently
skipped TypedArray tests.

Differential Revision: https://phabricator.services.mozilla.com/D99380

UltraBlame original commit: efd88e38955fc4722c6a882c175fdb48d6ff9e10
JonWBedard pushed a commit to WebKit/WebKit that referenced this pull request Jan 6, 2021
@rkirsling
Align %TypedArray% behavior with recent spec adjustments
21da394 
https://bugs.webkit.org/show_bug.cgi?id=218776

Reviewed by Yusuke Suzuki.

JSTests:

* stress/reflect-set.js:
* stress/typedarray-functions-with-neutered.js:
* stress/typedarray-includes.js:
* stress/typedarray-indexOf.js:
* stress/typedarray-join.js: Added.
* stress/typedarray-lastIndexOf.js:
Update tests.

* test262/expectations.yaml:
Mark a handful of test cases as temporarily failing.
These will disappear in a future test262 update.

Source/JavaScriptCore:

The recent spec changes for typed arrays with detached buffers had certain ripple effects,
namely the following two PRs which will be presented in next week's TC39 meeting.
Since no controversy is expected, this patch addresses them now, though test262 adjustments are forthcoming.

1. tc39/ecma262#2210
   It is correct that `ta[i] = n` doesn't throw when `ta` has a detached buffer or `i` is otherwise OOB,
   but by not throwing, Reflect.set(ta, i, n) is obliged to return true.

2. tc39/ecma262#2221
   Until now, %TypedArray%.prototype.{includes, indexOf, join, lastIndexOf} lacked a rigorous specification;
   in particular, each has a parameter that may detach the buffer upon valueOf or toString, and the expected
   behavior was not made clear. It seems most sensible to do what the corresponding Array methods do upon
   `array.length = 0`: make use of the cached length but don't access indices, such that indexOf/lastIndexOf
   return -1 while includes/join act as if the elements were all `undefined`.

* runtime/JSGenericTypedArrayViewInlines.h:
(JSC::JSGenericTypedArrayView<Adaptor>::put):
(JSC::JSGenericTypedArrayView<Adaptor>::defineOwnProperty):
(JSC::JSGenericTypedArrayView<Adaptor>::putByIndex):
* runtime/JSGenericTypedArrayViewPrototypeFunctions.h:
(JSC::genericTypedArrayViewProtoFuncIncludes):
(JSC::genericTypedArrayViewProtoFuncIndexOf):
(JSC::genericTypedArrayViewProtoFuncJoin):
(JSC::genericTypedArrayViewProtoFuncLastIndexOf):


Canonical link: https://commits.webkit.org/231449@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@269670 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@evilpie evilpie mentioned this pull request Feb 26, 2021
Closed
@bakkot bakkot mentioned this pull request Mar 15, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bakkot bakkot bakkot approved these changes

@syg syg syg approved these changes

@ljharb ljharb ljharb approved these changes

@michaelficarra michaelficarra Awaiting requested review from michaelficarra

Assignees
No one assigned
Labels
has consensus This has committee consensus. has test262 tests ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Is IntegerIndexedElementSet returning false for OOB access web-compatible?
4 participants
@rkirsling @bakkot @ljharb @syg