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

Editorial: Add Yield, CreateIteratorFromClosure, CreateAsyncIteratorFromClosure abs op. #2045

Merged
merged 1 commit into from
Jan 6, 2021
Merged

Conversation

Jack-Works
Copy link
Member

@Jack-Works Jack-Works commented Jun 14, 2020

Introduce Yield and CreateIteratorFromClosure, CreateAsyncIteratorFromClosure in the spec to make it easier to define builtin generators.

Diff: https://arai-a.github.io/ecma262-compare/?pr=2045

@ljharb
Copy link
Member

ljharb commented Jun 14, 2020

How is this normative? It seems purely editorial.

@Jack-Works Jack-Works changed the title Normative: Add Yield abstract operation, CreateBuiltinGeneratorFunction abs op. Editorial: Add Yield abstract operation, CreateBuiltinGeneratorFunction abs op. Jun 14, 2020
@ljharb
Copy link
Member

ljharb commented Jun 14, 2020

Is this meant to be useful for existing iterator-producing methods and the iterator helpers proposal?

@Jack-Works
Copy link
Member Author

I did a quick check, the current @@iterator on Array isn't a GeneratorFunction.
If we use it to replace the current iterator producing methods, it might be a breaking change (change @@iterator from a normal function to a GeneratorFunction)

Yes, it's for the iterator helpers (and my range proposal)

@ljharb
Copy link
Member

ljharb commented Jun 14, 2020

The plan for iterator helpers is to not have an actual generator function exposed to users, and i assume the same would be true for range. The intention is for each thing to be a normal function (just like all the existing iterator producers) that internally calls an abstract operation that’s implemented with generator spec mechanics.

@Jack-Works
Copy link
Member Author

@ljharb rewrited

@ljharb
Copy link
Member

ljharb commented Jun 15, 2020

I'll be most interested to see this technique used to non-observably improve the way, for example, String.prototype.matchAll is specified.

@Jack-Works
Copy link
Member Author

I'll be most interested to see this technique used to non-observably improve the way, for example, String.prototype.matchAll is specified.

Can the current PR do that?

@ljharb
Copy link
Member

ljharb commented Jun 15, 2020

I feel pretty strongly that the current PR must do that, since we don't generally add abstract operations that have zero uses :-)

@Jack-Works
Copy link
Member Author

I feel pretty strongly that the current PR must do that, since we don't generally add abstract operations that have zero uses :-)

I'll try to convert some existing algorithms later to verify my idea

@Jack-Works
Copy link
Member Author

I found it is not easy to move to CreateBuiltinGeneratorFunction cause they have their own prototype, the next method that does the brand checking.

@Jack-Works Jack-Works requested a review from ljharb June 16, 2020 04:10
@Jack-Works
Copy link
Member Author

I have rewritten the String@@iterator with this new abstract op @ljharb

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.

This is very interesting, I think I'm going to like it a lot.

It'd be great to convert a few more builtins to get a feel for how that would work - perhaps matchAll, Array, and Map or Set?

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@ljharb ljharb added the editor call to be discussed in the next editor call label Jun 16, 2020
@Jack-Works
Copy link
Member Author

how's that going? 🙈 @ljharb

@ljharb
Copy link
Member

ljharb commented Jun 23, 2020

@Jack-Works it needs further editor call discussion; hopefully we can update this week.

@ljharb
Copy link
Member

ljharb commented Jun 24, 2020

@Jack-Works hi! we discussed this, and we'd love to see more examples of iterator-producers converted to use this spec text. In particular, https://tc39.es/ecma262/#sec-string.prototype.matchall, https://tc39.es/ecma262/#sec-array.prototype.keys, and https://tc39.es/ecma262/#sec-map.prototype.keys.

@ljharb ljharb added editorial change and removed editor call to be discussed in the next editor call labels Jun 24, 2020
@devsnek
Copy link
Member

devsnek commented Jun 25, 2020

This isn't the design we're planning to use for iterator helpers, so I'm not sure how useful it would be to merge it in its current form.

@Jack-Works
Copy link
Member Author

This isn't the design we're planning to use for iterator helpers, so I'm not sure how useful it would be to merge it in its current form.

The design in this PR is learned from the iterator helper's current spec. I try to formalize words like

X is a built-in generator function which, when called, performs the following prelude steps:

Set lastValue to Yield(mapped).

and I need to support the current built-in generator-like methods behavior too.

My design work in the following way:

  • A built-in generator can have prelude steps (to do the parameter check) before they're getting iterated.
  • A built-in generator can declare its returning iterator to have a [[IteratorKind]] slot. This is used for the brand check, therefore the %StringIterator%.next.call(_array_iterator_) can be banned (to keep the current behavior).

If there is any design not compatible with the iterator helper, please point out and I'll try to fix that, thanks! @devsnek

@Jack-Works
Copy link
Member Author

@Jack-Works hi! we discussed this, and we'd love to see more examples of iterator-producers converted to use this spec text. In particular, https://tc39.es/ecma262/#sec-string.prototype.matchall, https://tc39.es/ecma262/#sec-array.prototype.keys, and https://tc39.es/ecma262/#sec-map.prototype.keys.

done! 👀

@devsnek
Copy link
Member

devsnek commented Jun 25, 2020

@Jack-Works we're not using prelude and body steps anymore. Here's an example of what we have atm, though its not final:

@Jack-Works
Copy link
Member Author

Is there specific reason to use the new form you screenshotted instead of the old form?

@devsnek
Copy link
Member

devsnek commented Jun 25, 2020

@Jack-Works we had pushback against exposing the functions directly and pushback against the "prelude"/"body" pattern.

@Jack-Works
Copy link
Member Author

@Jack-Works we had pushback against exposing the functions directly and pushback against the "prelude"/"body" pattern.

LGTM, I'll adjust the design to match the style you posted later.

@Jack-Works
Copy link
Member Author

Jack-Works commented Jun 30, 2020

[2020-06-30T08:45:52.451Z] Linting...

Error: expecting ordered list, got whitespace

    at Parser.parseAlgorithm (/home/travis/build/tc39/ecma262/node_modules/ecmarkdown/dist/parser.js:41:19)

Why the linter didn't tell me it failed at which line instead of this confusing message... 🤦‍♂️

Oh okay I have a syntax error here.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@bakkot
Copy link
Contributor

bakkot commented Dec 26, 2020

Looks pretty good! There's a couple of small correctness things and a couple of small editorial issues which should get fixed before landing, and it also needs a rebase.

@Jack-Works
Copy link
Member Author

has updated to the mainstream by merge. do I still need a rebase?

spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
@bakkot
Copy link
Contributor

bakkot commented Dec 26, 2020

has updated to the mainstream by merge. do I still need a rebase?

No, that's fine.

spec.html Show resolved Hide resolved
Copy link
Contributor

@bakkot bakkot left a comment

Choose a reason for hiding this comment

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

Needs a couple of last formatting tweaks. Otherwise LGTM.

spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented Dec 26, 2020

@Jack-Works this PR will need a rebase, as well - it has a merge conflict.

@Jack-Works

This comment has been minimized.

spec.html Outdated Show resolved Hide resolved
jmdyck
jmdyck previously requested changes Dec 29, 2020
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@ljharb ljharb requested a review from jmdyck January 6, 2021 22:40
@ljharb ljharb added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Jan 6, 2021
@ljharb ljharb merged commit 5f4be6e into tc39:master Jan 6, 2021
@Jack-Works
Copy link
Member Author

Cool

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial change ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.