Skip to content

merge immutable-arraybuffer tests #4445

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

phoddie
Copy link
Contributor

@phoddie phoddie commented Apr 1, 2025

Suite of test262 tests for the Immutable ArrayBuffer proposal.

@phoddie phoddie requested a review from a team as a code owner April 1, 2025 20:44
Copy link
Contributor

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

@phoddie This is pretty noisy right now; please fix the lint issues:

  • Add necessary front matter fields
  • Update features.txt
  • Fix the copyrights to a single year rather than a range
  • etc.

@phoddie
Copy link
Contributor Author

phoddie commented Apr 4, 2025

Sure, will do. FWIW – "Fix the copyrights to a single year rather than a range" feels artificial.

@ljharb
Copy link
Member

ljharb commented Apr 4, 2025

It's not; it ensures that the range doesn't need to be updated every year.

@gibson042
Copy link
Contributor

Rebasing on main will also get the annotation improvements from #4449.

@phoddie
Copy link
Contributor Author

phoddie commented Apr 4, 2025

It's not; it ensures that the range doesn't need to be updated every year

??? The range only needs to be updated in years when the tests are modified, not when the calendar advances. Anyway. I will change the tests to conform with this peculiar requirement.

@phoddie phoddie requested a review from a team as a code owner April 4, 2025 17:54
@phoddie
Copy link
Contributor Author

phoddie commented Apr 4, 2025

The linter is now happy and I've attempted to resolve all comments.

Copy link
Contributor

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

I am a champion of the proposal and will not formally approve this PR would appreciate attention from other maintainers, but I have reviewed it and confirmed consistency with the proposal. Followups are expected to cover error handling and ArrayBuffer.prototype.sliceToImmutable, but this LGTM.

Comment on lines +9 to +13
var immutable = Object.getOwnPropertyDescriptor(
ArrayBuffer.prototype, "immutable"
);

var getter = immutable.get;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:

Suggested change
var immutable = Object.getOwnPropertyDescriptor(
ArrayBuffer.prototype, "immutable"
);
var getter = immutable.get;
var getter = Object.getOwnPropertyDescriptor(
ArrayBuffer.prototype, "immutable"
).get;

Comment on lines +9 to +13
var immutable = Object.getOwnPropertyDescriptor(
ArrayBuffer.prototype, "immutable"
);

var getter = immutable.get;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:

Suggested change
var immutable = Object.getOwnPropertyDescriptor(
ArrayBuffer.prototype, "immutable"
);
var getter = immutable.get;
var getter = Object.getOwnPropertyDescriptor(
ArrayBuffer.prototype, "immutable"
).get;

Copy link
Contributor

@anba anba left a comment

Choose a reason for hiding this comment

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

There is still a bit of missing coverage:

ArrayBuffer.prototype.sliceToImmutable

  • Missing coverage.

Atomics

  • Missing coverage.

ArrayBuffer.prototype.resize
ArrayBuffer.prototype.slice
ArrayBuffer.prototype.transfer
ArrayBuffer.prototype.transferToFixedLength
ArrayBuffer.prototype.transferToImmutable

  • Ensure throws with immutable ArrayBuffer.

ArrayBuffer.prototype.byteLength
ArrayBuffer.prototype.detached
ArrayBuffer.prototype.maxByteLength
ArrayBuffer.prototype.resizable

  • Explicit tests calling with immutable ArrayBuffer instead of just as part of other tests?

DataView:

  • Use throwing conversion to ensure TypeError is thrown before coercing index parameter.

%TypedArray%.prototype.filter
%TypedArray%.prototype.map
%TypedArray%.prototype.slice
%TypedArray%.prototype.subarray

  • Species create returning TypedArray with immutable ArrayBuffer.

%TypedArray%.from
%TypedArray%.of

  • Constructor returns TypedArray with immutable ArrayBuffer.

%TypedArray%.prototype.subarray

  • Throws for immutable TypedArray before coercing offset parameter.

TypedArray [[Set]] coverage:

  • Add Reflect.set coverage.
  • Ensure no value coercion for immutable TypedArrays.
  • Ensure correct behaviour when receiver isn't the current object.
  • Test out-bounds indices.

var sample = new DataView(buffer.transferToImmutable(), 0);

assert.throws(TypeError, function() {
sample.setFloat16(0, 0n);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
sample.setFloat16(0, 0n);
sample.setFloat16(0, 0);

var sample = new DataView(buffer.transferToImmutable(), 0);

assert.throws(TypeError, function() {
sample.setFloat32(0, 0n);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
sample.setFloat32(0, 0n);
sample.setFloat32(0, 0);

var sample = new DataView(buffer.transferToImmutable(), 0);

assert.throws(TypeError, function() {
sample.setFloat64(0, 0n);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
sample.setFloat64(0, 0n);
sample.setFloat64(0, 0);

var sample = new DataView(buffer.transferToImmutable(), 0);

assert.throws(TypeError, function() {
sample.setInt16(0, 0n);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
sample.setInt16(0, 0n);
sample.setInt16(0, 0);

var sample = new DataView(buffer.transferToImmutable(), 0);

assert.throws(TypeError, function() {
sample.setInt32(0, 0n);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
sample.setInt32(0, 0n);
sample.setInt32(0, 0);

var buffer = new ArrayBuffer(42 * TA.BYTES_PER_ELEMENT);
var sample = new TA(buffer.transferToImmutable());
assert.throws(TypeError, function() {
Reflect.defineProperty(sample, "0", desc);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Reflect.defineProperty(sample, "0", desc);
Object.defineProperty(sample, "0", desc);

Because Reflect.defineProperty returns false on failure. But also add a separate test for Reflect.defineProperty. And add coverage when desc has the current property descriptor attributes. For example:

js> ta = new Int8Array(new ArrayBuffer(1).transferToImmutable())
({0:0})
js> Reflect.defineProperty(ta, 0, {})
true
js> Reflect.defineProperty(ta, 0, {writable: false})
true
js> Reflect.defineProperty(ta, 0, {writable: true})
false
js> Reflect.defineProperty(ta, 0, {value: 1})
false
js> Reflect.defineProperty(ta, 0, {value: 0})
true
js> Reflect.defineProperty(ta, 0, {value: 0, writable: false, enumerable: true, configurable: false})
true
js> Reflect.defineProperty(ta, 0, {value: 1, writable: false, enumerable: true, configurable: false})
false

And probably also for floating point special cases NaN or -0 to ensure SameValue semantics are used:

js> ta = new Float64Array(new Float64Array([NaN]).buffer.transferToImmutable())
({0:NaN})
js> Reflect.defineProperty(ta, 0, {value: NaN})
true

js> ta = new Float64Array(new Float64Array([-0]).buffer.transferToImmutable())
({0:-0})
js> Reflect.defineProperty(ta, 0, {value: -0})
true
js> Reflect.defineProperty(ta, 0, {value: +0})
false

// Copyright (C) 2025 Moddable Tech, Inc. All rights reserved.
// This code is governed by the BSD license found in the LICENSE file.
/*---
description: Reflect.defineProperty throws on indexed property if buffer is immutable
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
description: Reflect.defineProperty throws on indexed property if buffer is immutable
description: Object.defineProperty throws on indexed property if buffer is immutable

description: Reflect.defineProperty throws on indexed property if buffer is immutable
esid: sec-integer-indexed-exotic-objects-defineownproperty-p-desc
includes: [testTypedArray.js]
features: [Reflect, TypedArray, immutable-arraybuffer]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
features: [Reflect, TypedArray, immutable-arraybuffer]
features: [TypedArray, immutable-arraybuffer]

description: setting indexed property throws if buffer is immutable
esid: sec-integer-indexed-exotic-objects-set-p-v-receiver
includes: [testTypedArray.js]
features: [TypedArray, immutable-arraybuffer]
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing flags: [onlyStrict]. But please also add a flags: [noStrict] test which ensure no TypeError is thrown.

var sample = new TA(buffer.transferToImmutable());
assert.throws(TypeError, function() {
sample[0] = 1;
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Also assert assert.sameValue(sample[0], 0) to ensure no modification took place.

@ptomato
Copy link
Contributor

ptomato commented May 7, 2025

Thanks for the review @anba. This is very helpful.

I'd prefer not to make this PR any bigger so let's not fix the missing coverage here, instead please consider opening an issue with a testing plan.

@phoddie
Copy link
Contributor Author

phoddie commented May 14, 2025

The helpful feedback from @anba have been applied.

As per discussion with @gibson042, we'll break this up into multiple PRs based on the test plan, when available.

lando-prod-mozilla bot pushed a commit to mozilla-firefox/firefox that referenced this pull request May 27, 2025
…spidermonkey-reviewers,dminor

Test262 PR: <tc39/test262#4445>.

Roughly half of the spec proposal is still missing coverage, see the review
notes at <tc39/test262#4445 (review)>.

Differential Revision: https://phabricator.services.mozilla.com/D249415
lando-prod-mozilla bot pushed a commit to mozilla-firefox/firefox that referenced this pull request May 27, 2025
…spidermonkey-reviewers,dminor

Test262 PR: <tc39/test262#4445>.

Roughly half of the spec proposal is still missing coverage, see the review
notes at <tc39/test262#4445 (review)>.

Differential Revision: https://phabricator.services.mozilla.com/D249415
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request May 27, 2025
…spidermonkey-reviewers,dminor

Test262 PR: <tc39/test262#4445>.

Roughly half of the spec proposal is still missing coverage, see the review
notes at <tc39/test262#4445 (review)>.

Differential Revision: https://phabricator.services.mozilla.com/D249415
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request May 27, 2025
…spidermonkey-reviewers,dminor

Test262 PR: <tc39/test262#4445>.

Roughly half of the spec proposal is still missing coverage, see the review
notes at <tc39/test262#4445 (review)>.

Differential Revision: https://phabricator.services.mozilla.com/D249415
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request May 28, 2025
…spidermonkey-reviewers,dminor

Test262 PR: <tc39/test262#4445>.

Roughly half of the spec proposal is still missing coverage, see the review
notes at <tc39/test262#4445 (review)>.

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

UltraBlame original commit: a2125b80c39422a420f5eae283f045d8d98d13ee
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request May 28, 2025
…spidermonkey-reviewers,dminor

Test262 PR: <tc39/test262#4445>.

Roughly half of the spec proposal is still missing coverage, see the review
notes at <tc39/test262#4445 (review)>.

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

UltraBlame original commit: 8b6221835a28784ac50521fe790e6521aea5572e
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request May 28, 2025
…spidermonkey-reviewers,dminor

Test262 PR: <tc39/test262#4445>.

Roughly half of the spec proposal is still missing coverage, see the review
notes at <tc39/test262#4445 (review)>.

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

UltraBlame original commit: a2125b80c39422a420f5eae283f045d8d98d13ee
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request May 28, 2025
…spidermonkey-reviewers,dminor

Test262 PR: <tc39/test262#4445>.

Roughly half of the spec proposal is still missing coverage, see the review
notes at <tc39/test262#4445 (review)>.

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

UltraBlame original commit: 8b6221835a28784ac50521fe790e6521aea5572e
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request May 28, 2025
…spidermonkey-reviewers,dminor

Test262 PR: <tc39/test262#4445>.

Roughly half of the spec proposal is still missing coverage, see the review
notes at <tc39/test262#4445 (review)>.

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

UltraBlame original commit: a2125b80c39422a420f5eae283f045d8d98d13ee
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request May 28, 2025
…spidermonkey-reviewers,dminor

Test262 PR: <tc39/test262#4445>.

Roughly half of the spec proposal is still missing coverage, see the review
notes at <tc39/test262#4445 (review)>.

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

UltraBlame original commit: 8b6221835a28784ac50521fe790e6521aea5572e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants