Skip to content

Editorial: Unify style of abrupt completion and non-LoopContinues result handling in ForIn/OfBodyEvaluation #3595

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 1 commit into
base: main
Choose a base branch
from

Conversation

aapoalas
Copy link

@aapoalas aapoalas commented May 11, 2025

A personal favourite of mine: the order of checking/comparing iterationKind and iteratorKind is different between two otherwise nearly identical blocks handling abrupt completions (exceptions) in the loop header binding, and handling non-loop-continuing results of the body evaluation.

The difference makes the two blocks really hard to compare, even though they're 100% the same except for the UpdateEmpty call.

@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. needs data This PR needs more information; such as web compatibility data, “web reality” (what all engines do)… needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 labels May 11, 2025
@aapoalas
Copy link
Author

@michaelficarra This doesn't have any effect on behaviour of source text evaluation. It's strictly only a reordering of a few lines.

Copy link
Member

@linusg linusg left a comment

Choose a reason for hiding this comment

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

I like it :)

@linusg linusg added editorial change and removed 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. needs data This PR needs more information; such as web compatibility data, “web reality” (what all engines do)… needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 labels May 14, 2025
Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

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

Ah I see, this is relying on the fact that there's no call site for iterationKind of ~enumerate~ and iteratorKind of ~async~. This could probably be stated explicitly somewhere.

@aapoalas
Copy link
Author

Ah I see, this is relying on the fact that there's no call site for iterationKind of ~enumerate~ and iteratorKind of ~async~. This could probably be stated explicitly somewhere.

I can add asserts in the ~enumerate~ branch for iteratorKind being ~sync~ if you'd want?

@michaelficarra
Copy link
Member

@aapoalas I had something else in mind #3475 (comment)

@michaelficarra michaelficarra force-pushed the editorial/for-in-of-body-iterator-close-unification branch from 9816e74 to 9d2b0a9 Compare May 22, 2025 14:18
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.

3 participants