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: Fixed mutable completion records as immutable #2971

Merged
merged 1 commit into from
Dec 15, 2022

Conversation

jhnaldo
Copy link
Contributor

@jhnaldo jhnaldo commented Dec 13, 2022

In my understanding, the common practice for completion records is to use them as immutable records. For example, 6.2.4.3 UpdateEmpty does not directly modify the [[Value]] field but creates another completion record having the given value in the [[Value]] field.

However, step 7.c.iii.2 in the 15.5.5 Runtime Semantics: Evaluation for <emu-grammar>YieldExpression : `yield` `*` AssignmentExpression</emu-grammar> breaks this common practice. I think this is the only step that treats completion records as mutable in the spec.

In addition, as mentioned in es-meta/esmeta#65, we found that only three Test262 tests related to this issue:

So, I modified step 7.c.iii.2 in the 15.5.5 Runtime Semantics: Evaluation to introduce a new completion record instead of updating its [[Value]] field.

@michaelficarra michaelficarra added establishes editorial conventions editor call to be discussed in the next editor call labels Dec 13, 2022
spec.html Show resolved Hide resolved
@syg syg removed the editor call to be discussed in the next editor call label Dec 14, 2022
spec.html Outdated Show resolved Hide resolved
@bakkot
Copy link
Contributor

bakkot commented Dec 14, 2022

Editors are fine with this, but we do want to say explicitly that mutating record fields in general isn't something we want to get rid of. Completion records are a bit special, though.

@jhnaldo
Copy link
Contributor Author

jhnaldo commented Dec 14, 2022

Editors are fine with this, but we do want to say explicitly that mutating record fields in general isn't something we we ant to get rid of. Completion records are a bit special, though.

I totally agree with you. Mutating record fields is a common practice in general, but completion records are special ones that can be used in every place to represent control flows. I think it is better to disallow mutating completion records to prevent unexpected side effects because of the mutability.

@michaelficarra michaelficarra added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Dec 14, 2022
@ljharb ljharb force-pushed the fix-mutable-completion branch from d8a450a to 578f18f Compare December 15, 2022 05:59
@ljharb ljharb merged commit 578f18f into tc39:main Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
establishes editorial conventions 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.

5 participants