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: GetValue and PutValue do not need to unwrap Completion Records #2842

Merged
merged 3 commits into from
Aug 19, 2022

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented Aug 3, 2022

#2744 removed almost every case where GetValue or PutValue was passed a completion record, instead explicitly unwrapping on the previous line or lines. This removes the final three cases and then updates the definitions of those algorithms so that they do not need to handle being passed Completion Records.

Copy link
Collaborator

@jmdyck jmdyck left a comment

Choose a reason for hiding this comment

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

InitializeReferencedBinding also calls ReturnIfAbrupt on its 2 parameters, so it might make sense to include it in this PR.

@bakkot
Copy link
Contributor Author

bakkot commented Aug 4, 2022

InitializeReferencedBinding also calls ReturnIfAbrupt on its 2 parameters, so it might make sense to include it in this PR.

Done. In the process I marked initialization of let x; and let x = 0; as infallible (in 73f2743), which I am pretty sure is true.

@michaelficarra michaelficarra requested review from syg and a team August 16, 2022 00:35
spec.html Outdated
@@ -22038,7 +22032,7 @@ <h1>
1. Set the running execution context's LexicalEnvironment to _newEnv_.
1. Let _exprRef_ be Completion(Evaluation of _expr_).
1. Set the running execution context's LexicalEnvironment to _oldEnv_.
1. Let _exprValue_ be ? GetValue(_exprRef_).
1. Let _exprValue_ be ? GetValue(? _exprRef_).
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity did ecmarkup find this or was this manually audited?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Manually, alas. ecmarkup does not yet track types except for checking the completion-ness of callsites of AOs, which is purely local.

@ljharb ljharb force-pushed the refactor-for-in-of branch from 5410f2e to 97e2321 Compare August 18, 2022 05:24
Base automatically changed from refactor-for-in-of to main August 18, 2022 05:37
@bakkot bakkot added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Aug 18, 2022
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.

5 participants