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

Normative: re-use IteratorResult objects in some iterator helpers #3489

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

michaelficarra
Copy link
Member

This re-uses IteratorResult objects when it's possible to just pass the object through. There are currently 3 cases of this: take/drop/filter. See related PR for the iterator sequencing proposal, which also has this opportunity: tc39/proposal-iterator-sequencing#18

I actually don't like doing this for filter since filter observes the value, which would mean triggering a value getter both for filtering and by the eventual consumer.

@michaelficarra michaelficarra added normative change Affects behavior required to correctly evaluate some ECMAScript source text needs consensus This needs committee consensus before it can be eligible to be merged. labels Nov 20, 2024
@nicolo-ribaudo
Copy link
Member

which would mean triggering a value getter both for filtering and by the eventual consumer.

Why do you consider this to only be bad for the .value getter and not for the .done getter, which would be double-triggered by all those methods?

@michaelficarra
Copy link
Member Author

I think an expensive value getter is more likely than an expensive done getter.

@michaelficarra
Copy link
Member Author

michaelficarra commented Nov 21, 2024

@nicolo-ribaudo Also, someone might be surprised that the resulting iterator yields values for which the predicate would return false. But we might just want to say 🤷‍♂️ getters admit weirdness.

edit: I've added a slide to the upcoming presentation covering interactions with getters.

@dminor
Copy link

dminor commented Nov 21, 2024

@anba Do you have an opinion on this change?

@michaelficarra michaelficarra added the needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 label Nov 21, 2024
@anba
Copy link
Contributor

anba commented Nov 22, 2024

This change is possibly difficult to implement for JSC and SM, because both use self-hosted JS for their implementation and the proposed changes can't be easily implemented in JS. (yield* can't be used, because it performs one final IteratorValue when done, which is user-detectable.)

I guess this also applies to tc39/proposal-iterator-sequencing#18.

@michaelficarra
Copy link
Member Author

because both use self-hosted JS for their implementation and the proposed changes can't be easily implemented in JS

Do you mean that they can't be easily implemented in JS at all, or do you mean using generators in JS? It seems manual iteration in JS should work fine.

yield* can't be used, because it performs one final IteratorValue when done, which is user-detectable

I'm open to also requiring the final IteratorValue to fully match a simpler generators/yield* implementation. One extra unnecessary property access at the end seems to be worth the savings of a whole extra object per item yielded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs consensus This needs committee consensus before it can be eligible to be merged. needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 normative change Affects behavior required to correctly evaluate some ECMAScript source text
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants