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

Iterator Helpers #2818

Merged
merged 68 commits into from
Jun 15, 2023
Merged

Iterator Helpers #2818

merged 68 commits into from
Jun 15, 2023

Conversation

rwaldron
Copy link
Contributor

@rwaldron rwaldron commented Sep 24, 2020 •
edited by Ms2ger
Loading

@rwaldron rwaldron force-pushed the iterator-helpers branch 5 times, most recently from 3000eb9 to 20bac2c Compare September 29, 2020 15:10
@rwaldron
Copy link
Contributor Author

@jugglinmike this will likely be relevant to your efforts in 2021. Please feel free to revise in any way that you see fit.

@rwaldron
Copy link
Contributor Author

rwaldron commented Dec 2, 2021

Dusting this off.

Today I was able to run these tests with SpiderMonkey's iterator helpers feature enabled. Here's how:

  1. Run esvu for latest JS engines
  2. In the root of this repo, run: test262-harness -t 16 --hostType=sm --hostPath=$(which sm) --hostArgs='--enable-iterator-helpers' 'test/built-ins/AsyncIterator/**/*.js'

My current results:

λ test262-harness -t 16 --hostType=sm --hostPath=$(which sm) --hostArgs='--enable-iterator-helpers' 'test/built-ins/AsyncIterator/**/*.js'
FAIL test/built-ins/AsyncIterator/prop-desc.js (strict mode)
  Expected no error, got ReferenceError: AsyncIterator is not defined

FAIL test/built-ins/AsyncIterator/prop-desc.js (default)
  Expected no error, got ReferenceError: AsyncIterator is not defined

Ran 192 tests
190 passed
2 failed
test262-harness -t 16 --hostType=sm --hostPath=$(which sm)    8.56s user 2.99s system 222% cpu 5.191 total

@benjamingr benjamingr mentioned this pull request Jan 30, 2022
Merged
@Ms2ger Ms2ger added the awaiting stage 2.7 Supports a "Stage 2" feature label Nov 8, 2022
@michaelficarra
Copy link
Member

@Ms2ger the proposal is now stage 3.

Would anyone like to pick up this PR and update it to match the current state of the proposal?

@Ms2ger
Copy link
Contributor

Ms2ger commented Dec 1, 2022

Do we want to use #3728?

@michaelficarra
Copy link
Member

I've updated the tests in this branch to cover just the sync iterator helpers, as the async iterator helpers have been split into their own proposal (and demoted to stage 2). Next, I'll review the existing tests for completeness and update/add tests as necessary, then I'll add tests to cover the remaining helpers that were not yet covered.

ljharb
ljharb requested changes Mar 22, 2023
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

These changes are for the validation order change that got consensus yesterday.

test/built-ins/Iterator/prototype/drop/argument-effect-order.js Outdated Show resolved Hide resolved
test/built-ins/Iterator/prototype/every/argument-effect-order.js Outdated Show resolved Hide resolved
test/built-ins/Iterator/prototype/filter/argument-effect-order.js Outdated Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented Mar 22, 2023

Does https://github.com/tc39/test262/blob/main/test/built-ins/Object/prototype/toString/symbol-tag-non-str-builtin.js#L29 need to be updated as well, since Iterator.prototype[Symbol.toStringTag] will now exist?

@ljharb ljharb mentioned this pull request Mar 24, 2023
Merged
@ljharb ljharb force-pushed the iterator-helpers branch 2 times, most recently from aa33100 to 018f993 Compare March 28, 2023 23:34
ptomato
ptomato reviewed Apr 1, 2023
View reviewed changes
Copy link
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

👋 Thanks for working on this, I'm glad to see it's being picked up! I didn't have time to go through the entire PR but here are some comments that occurred to me as I started reading at the top.

test/built-ins/Iterator/length.js Outdated Show resolved Hide resolved
test/built-ins/Iterator/newtarget-or-active-function-object.js Outdated Show resolved Hide resolved
test/built-ins/Iterator/prototype/drop/limit-rangeerror.js Show resolved Hide resolved
test/built-ins/Iterator/prototype/drop/limit-tonumber-throws.js Outdated Show resolved Hide resolved
test/built-ins/Iterator/prototype/drop/result-proto.js Outdated Show resolved Hide resolved
test/built-ins/Iterator/prototype/drop/return-is-forwarded.js Outdated Show resolved Hide resolved
@michaelficarra michaelficarra removed awaiting stage 2.7 Supports a "Stage 2" feature needs follow-up labels Apr 25, 2023
@michaelficarra
Copy link
Member

Addressed feedback from @syg and rebased.

ljharb
ljharb approved these changes Jun 1, 2023
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

tests look good, all pass on my polyfill written close-to-spec

@ljharb ljharb requested review from ptomato, a team, bakkot and benjamingr June 1, 2023 17:21
@michaelficarra
Copy link
Member

@tc39/test262-maintainers Any idea what the review process/timeline is going to look like for this? Implementations are already well underway, and we have approvals from both @syg and @ljharb.

ptomato
ptomato approved these changes Jun 12, 2023
View reviewed changes
Copy link
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

I'm definitely not going to have time to review this in the near future, but with positive reviews from Shu and Jordan I'm confident that we could merge it.

Before merging it would be good to figure out the ESMeta job's failure. I looked at the test it's complaining about and left a suggestion, but I'm not sure if that's really the cause.

test/built-ins/Iterator/prototype/Symbol.iterator/name.js Outdated Show resolved Hide resolved
@michaelficarra
Copy link
Member

@ptomato Tests are passing.

@ptomato ptomato merged commit dfc7ce4 into main Jun 15, 2023
@ptomato ptomato deleted the iterator-helpers branch June 15, 2023 15:07
@michaelficarra michaelficarra mentioned this pull request Aug 24, 2023
Open
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@syg syg syg approved these changes

@michaelficarra michaelficarra michaelficarra left review comments

@ljharb ljharb ljharb approved these changes

@ptomato ptomato ptomato approved these changes

@bakkot bakkot Awaiting requested review from bakkot

@benjamingr benjamingr Awaiting requested review from benjamingr

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@rwaldron @michaelficarra @Ms2ger @ljharb @syg @ptomato @benjamingr @bakkot