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: avoid duplicate SetFunctionName in {async,}generator methods #2463

Merged
merged 1 commit into from
Aug 7, 2021

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented Jul 17, 2021

#1668 introduced a DefineMethodProperty method for use when creating plain methods, generator methods, async methods, and async generator methods. In that PR it included a call to SetFunctionName, which was correspondingly factored out of the evaluation semantics of plain methods and async methods, but not generator methods or async generator methods. In the evaluation semantics for generator (and async generator) methods, there's another property (.prototype) which is added between the call to SetFunctionName and DefineMethodProperty, and since property creation order is observable dropping the first call and just letting it happen in DefineMethodProperty would be an observable change.

Instead, just remove SetFunctionName from DefineMethodProperty, and restore it to the evaluation semantics for plain and async methods. It doesn't really belong in DefineMethodProperty.

(I'm pretty sure I introduced this bug by accident rebasing #1668.)

This was definitely unintentional, and fails an assert (in SetFunctionName). So I'm calling it editorial.

Thanks to @devsnek for the catch.

@bakkot bakkot added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Aug 4, 2021
@ljharb ljharb force-pushed the generator-method-setfunctionname branch from 7daa47b to fcabfe0 Compare August 7, 2021 05:44
@ljharb ljharb merged commit fcabfe0 into master Aug 7, 2021
@ljharb ljharb deleted the generator-method-setfunctionname branch August 7, 2021 05:49
mathiasbynens pushed a commit to mathiasbynens/ecma262 that referenced this pull request Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Editors believe this PR needs no further reviews, and is ready to land. spec bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants