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.

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

Improve coverage for reference type in various contexts #4216

Merged
merged 1 commit into from
Sep 20, 2024

Conversation

anba
Copy link
Contributor

@anba anba commented Sep 2, 2024

Missing coverage encountered while implementing
tc39/ecma262#3307 in SpiderMonkey.

Ensure environment lookups are performed in the correct order:

  • keyed-destructuring-property-reference-target-evaluation-order-with-bindings.js

Ensure delete super[elem] steps are correctly performed:

  • delete/super-property-topropertykey.js
  • delete/super-property-uninitialized-this.js

Ensure ToPropertyKey for computed property names in object literals correctly performed:

  • object/computed-property-name-topropertykey-before-value-evaluation.js

Ensure GetSuperBase is executed before ToPropertKey:

  • super/prop-expr-getsuperbase-before-topropertykey-*

Ensure GetThisBinding is executed first:

  • super/prop-expr-uninitialized-this-*

@anba anba requested a review from a team as a code owner September 2, 2024 15:13
@anba
Copy link
Contributor Author

anba commented Sep 4, 2024

@linusg Do you need these flags because your implementation doesn't support these features? I'm asking because other files in the same directories are already omitting some flags. Unless it's necessary for some actual implementation, I'd prefer to omit flags for language features which were added years ago.

@linusg
Copy link
Member

linusg commented Sep 4, 2024

Personally I've never used the flags, IMO evergreen engines don't need them. A failure is a failure and filtering doesn't change that.

The comments are just for completeness, feel free to disregard them if the maintainers agree that we don't need to be strict about flags 👍

@ljharb
Copy link
Member

ljharb commented Sep 4, 2024

We in fact do want every test to have flags for every feature, even ones added many years ago (we also want to avoid newer syntax when it's not strictly necessary for the test)

@anba
Copy link
Contributor Author

anba commented Sep 4, 2024

No, that's not what's documented in features.txt.

@ljharb
Copy link
Member

ljharb commented Sep 4, 2024

ok, but that's the current policy of the maintainers (and the existence of class and other <= ES2015 features should make it clear that the sentence at the top is nonexhaustive). I'll add to my list to update the wording in that document.

@anba
Copy link
Contributor Author

anba commented Sep 4, 2024

Is this documented anywhere? For example #3196 (comment) and #3665 (comment) don't state that it's a strict requirement to name each and every feature.

@ljharb
Copy link
Member

ljharb commented Sep 4, 2024

It's indeed not a strict requirement - but we still want everything to have them.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Sep 5, 2024
…ermonkey-reviewers,jandem

The spec PR reordered the `ToPropertyKey` call to happen in `GetValue` resp.
`PutValue`, which means it has to happen after `GetSuperBase`.

Tests are included in <tc39/test262#4216>.

Differential Revision: https://phabricator.services.mozilla.com/D220825
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Sep 5, 2024
…ing destructured element. r=arai

`BytecodeEmitter::emitDestructuringLHSRef` needs to call `NameOpEmitter::prepareForRhs`
to emit the necessary steps to resolve environment bindings. To avoid adding a new
`skipPrepareForRhs` method to `NameOpEmitter`, instead pass `DestructuringLHSRef&`
to `emitDestructuringLHSRef`, where `DestructuringLHSRef` holds the emitter for the
destructuring left-hand side reference.

Tests are in <tc39/test262#4216>.

Differential Revision: https://phabricator.services.mozilla.com/D220828
@ptomato
Copy link
Contributor

ptomato commented Sep 5, 2024

I should nuance my position on the policy - All other things being equal I'd prefer to have exhaustive flags, but I also don't think we should be putting that burden on people who contribute tests, so I'd much rather pursue that via codemods.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Sep 5, 2024
…ermonkey-reviewers,jandem

The spec PR reordered the `ToPropertyKey` call to happen in `GetValue` resp.
`PutValue`, which means it has to happen after `GetSuperBase`.

Tests are included in <tc39/test262#4216>.

Differential Revision: https://phabricator.services.mozilla.com/D220825
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Sep 5, 2024
…ing destructured element. r=arai

`BytecodeEmitter::emitDestructuringLHSRef` needs to call `NameOpEmitter::prepareForRhs`
to emit the necessary steps to resolve environment bindings. To avoid adding a new
`skipPrepareForRhs` method to `NameOpEmitter`, instead pass `DestructuringLHSRef&`
to `emitDestructuringLHSRef`, where `DestructuringLHSRef` holds the emitter for the
destructuring left-hand side reference.

Tests are in <tc39/test262#4216>.

Differential Revision: https://phabricator.services.mozilla.com/D220828
aosmond pushed a commit to aosmond/gecko that referenced this pull request Sep 5, 2024
…ermonkey-reviewers,jandem

The spec PR reordered the `ToPropertyKey` call to happen in `GetValue` resp.
`PutValue`, which means it has to happen after `GetSuperBase`.

Tests are included in <tc39/test262#4216>.

Differential Revision: https://phabricator.services.mozilla.com/D220825
aosmond pushed a commit to aosmond/gecko that referenced this pull request Sep 5, 2024
…ing destructured element. r=arai

`BytecodeEmitter::emitDestructuringLHSRef` needs to call `NameOpEmitter::prepareForRhs`
to emit the necessary steps to resolve environment bindings. To avoid adding a new
`skipPrepareForRhs` method to `NameOpEmitter`, instead pass `DestructuringLHSRef&`
to `emitDestructuringLHSRef`, where `DestructuringLHSRef` holds the emitter for the
destructuring left-hand side reference.

Tests are in <tc39/test262#4216>.

Differential Revision: https://phabricator.services.mozilla.com/D220828
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Sep 6, 2024
…ermonkey-reviewers,jandem

The spec PR reordered the `ToPropertyKey` call to happen in `GetValue` resp.
`PutValue`, which means it has to happen after `GetSuperBase`.

Tests are included in <tc39/test262#4216>.

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

UltraBlame original commit: c4fbbb9b19cc19b8836de939b5561964b4058030
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Sep 6, 2024
…ing destructured element. r=arai

`BytecodeEmitter::emitDestructuringLHSRef` needs to call `NameOpEmitter::prepareForRhs`
to emit the necessary steps to resolve environment bindings. To avoid adding a new
`skipPrepareForRhs` method to `NameOpEmitter`, instead pass `DestructuringLHSRef&`
to `emitDestructuringLHSRef`, where `DestructuringLHSRef` holds the emitter for the
destructuring left-hand side reference.

Tests are in <tc39/test262#4216>.

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

UltraBlame original commit: 5a9e48dab16e9706ff963d6155df153acb6a0354
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Sep 6, 2024
…ermonkey-reviewers,jandem

The spec PR reordered the `ToPropertyKey` call to happen in `GetValue` resp.
`PutValue`, which means it has to happen after `GetSuperBase`.

Tests are included in <tc39/test262#4216>.

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

UltraBlame original commit: e7e456e896ff809e6883f37dc5ba8c6a30e0c3d6
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Sep 6, 2024
…ing destructured element. r=arai

`BytecodeEmitter::emitDestructuringLHSRef` needs to call `NameOpEmitter::prepareForRhs`
to emit the necessary steps to resolve environment bindings. To avoid adding a new
`skipPrepareForRhs` method to `NameOpEmitter`, instead pass `DestructuringLHSRef&`
to `emitDestructuringLHSRef`, where `DestructuringLHSRef` holds the emitter for the
destructuring left-hand side reference.

Tests are in <tc39/test262#4216>.

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

UltraBlame original commit: 4c26da6ff32617ca7b277a16949419a7ab28ac5b
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Sep 6, 2024
…ermonkey-reviewers,jandem

The spec PR reordered the `ToPropertyKey` call to happen in `GetValue` resp.
`PutValue`, which means it has to happen after `GetSuperBase`.

Tests are included in <tc39/test262#4216>.

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

UltraBlame original commit: c4fbbb9b19cc19b8836de939b5561964b4058030
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Sep 6, 2024
…ing destructured element. r=arai

`BytecodeEmitter::emitDestructuringLHSRef` needs to call `NameOpEmitter::prepareForRhs`
to emit the necessary steps to resolve environment bindings. To avoid adding a new
`skipPrepareForRhs` method to `NameOpEmitter`, instead pass `DestructuringLHSRef&`
to `emitDestructuringLHSRef`, where `DestructuringLHSRef` holds the emitter for the
destructuring left-hand side reference.

Tests are in <tc39/test262#4216>.

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

UltraBlame original commit: 5a9e48dab16e9706ff963d6155df153acb6a0354
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Sep 6, 2024
…ermonkey-reviewers,jandem

The spec PR reordered the `ToPropertyKey` call to happen in `GetValue` resp.
`PutValue`, which means it has to happen after `GetSuperBase`.

Tests are included in <tc39/test262#4216>.

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

UltraBlame original commit: e7e456e896ff809e6883f37dc5ba8c6a30e0c3d6
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Sep 6, 2024
…ing destructured element. r=arai

`BytecodeEmitter::emitDestructuringLHSRef` needs to call `NameOpEmitter::prepareForRhs`
to emit the necessary steps to resolve environment bindings. To avoid adding a new
`skipPrepareForRhs` method to `NameOpEmitter`, instead pass `DestructuringLHSRef&`
to `emitDestructuringLHSRef`, where `DestructuringLHSRef` holds the emitter for the
destructuring left-hand side reference.

Tests are in <tc39/test262#4216>.

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

UltraBlame original commit: 4c26da6ff32617ca7b277a16949419a7ab28ac5b
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this pull request Sep 6, 2024
…ermonkey-reviewers,jandem

The spec PR reordered the `ToPropertyKey` call to happen in `GetValue` resp.
`PutValue`, which means it has to happen after `GetSuperBase`.

Tests are included in <tc39/test262#4216>.

Differential Revision: https://phabricator.services.mozilla.com/D220825
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this pull request Sep 6, 2024
…ing destructured element. r=arai

`BytecodeEmitter::emitDestructuringLHSRef` needs to call `NameOpEmitter::prepareForRhs`
to emit the necessary steps to resolve environment bindings. To avoid adding a new
`skipPrepareForRhs` method to `NameOpEmitter`, instead pass `DestructuringLHSRef&`
to `emitDestructuringLHSRef`, where `DestructuringLHSRef` holds the emitter for the
destructuring left-hand side reference.

Tests are in <tc39/test262#4216>.

Differential Revision: https://phabricator.services.mozilla.com/D220828
@Ms2ger Ms2ger self-assigned this Sep 13, 2024
Copy link
Contributor

@Ms2ger Ms2ger left a comment

Choose a reason for hiding this comment

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

These are nasty. Thanks.


var key = {
toString() {
Object.setPrototypeOf(obj, proto2);
Copy link
Contributor

Choose a reason for hiding this comment

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

One might assert that this method is called only once

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'd prefer to not assert this, because otherwise V8 will have to skip this test, because V8 evaluates obj[key] += value as obj[key] = obj[key] + value, which evaluates ToPropertyKey(key) twice. And if we don't assert how many times ToPropertyKey(key) is evaluated, V8 is more likely to not skip this test, so there's maybe more incentive for V8 to fix their implementation to evaluate GetSuperBase before ToPropertyKey.

@Ms2ger Ms2ger enabled auto-merge (rebase) September 13, 2024 14:51
Missing coverage encountered while implementing
<tc39/ecma262#3307> in SpiderMonkey.

Ensure environment lookups are performed in the correct order:
- keyed-destructuring-property-reference-target-evaluation-order-with-bindings.js

Ensure `delete super[elem]` steps are correctly performed:
- delete/super-property-topropertykey.js
- delete/super-property-uninitialized-this.js

Ensure ToPropertyKey for computed property names in object literals
correctly performed:
- object/computed-property-name-topropertykey-before-value-evaluation.js

Ensure `GetSuperBase` is executed before `ToPropertKey`:
- super/prop-expr-getsuperbase-before-topropertykey-*

Ensure `GetThisBinding` is executed first:
- super/prop-expr-uninitialized-this-*
@Ms2ger Ms2ger merged commit 18ae34d into tc39:main Sep 20, 2024
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants