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

LibJS/Bytecode: Implement async generators #19934

Merged
merged 8 commits into from
Jul 14, 2023

Conversation

Lubrsi
Copy link
Member

@Lubrsi Lubrsi commented Jul 10, 2023

Bytecode diff:

Summary:
    Diff Tests:
        +3960 ✅ (8.018%) +32 ❌ (0.064%) -3992 📝 (8.082%)

Tests passing: 47122/49388 (95.4%)

AST diff:

Summary:
    Diff Tests:
        +3964 ✅ (8.026%) +27 ❌ (0.056%) +1 💥️ (0.002%) -3992 📝 (8.082%)

Tests passing: 47322/49388 (95.8%)

https://www.youtube.com/watch?v=_ovdm2yX4MA

@Lubrsi Lubrsi requested a review from linusg as a code owner July 10, 2023 23:02
@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Jul 10, 2023
@trflynn89
Copy link
Member

trflynn89 commented Jul 11, 2023

Couple of high level questions without getting into the diff:
Is this really a 1 commit change?
test-js tests?
Any idea what the new crashes are?

Copy link
Member

@alimpfard alimpfard left a comment

Choose a reason for hiding this comment

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

bytecode changes mostly LGTM, just one comment about a comment:

Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp Outdated Show resolved Hide resolved
@trflynn89
Copy link
Member

Any idea what the new crashes are?

Seems like a spec bug to me. In bytecode mode, the crashes are:

test/built-ins/AsyncGeneratorPrototype/return/return-state-completed-broken-promise.js 📝 -> 💥
test/built-ins/AsyncGeneratorPrototype/return/return-suspendedStart-broken-promise.js  📝 -> 💥

These are asserting on infallible invocations to AsyncGeneratorAwaitReturn:

// ii. Perform ! AsyncGeneratorAwaitReturn(generator).
MUST(await_return());

However this operation is trivially fallible in step 6:

// 6. Let promise be ? PromiseResolve(%Promise%, completion.[[Value]]).
auto* promise = TRY(promise_resolve(vm, realm.intrinsics().promise_constructor(), completion.value().value()));

The current form of this AO was written here. Before that, abrupt completions from PromiseResolve were specifically handled to reject the promise and prevent further throwing.

@trflynn89
Copy link
Member

Ah there's a spec PR open for this already: tc39/ecma262#2683

@Lubrsi
Copy link
Member Author

Lubrsi commented Jul 14, 2023

All comments addressed except the 1 AST crash, which is:

💥️ /home/lukew/libjs-test262/test262/test/built-ins/AsyncGeneratorPrototype/next/request-queue-await-order.js                                                                                                                                                                     
                                                                                                                                                                                                                                                                                  
/home/lukew/Desktop/serenity-project/serenity/Userland/Libraries/LibJS/Runtime/Completion.cpp:117: JS::ThrowCompletionOr<JS::Value> JS::await(VM&, Value): Assertion `success.has_value()' failed.

Though I don't know if it's worth fixing this given that AST's await is very ad-hoc and AST is on it's way out.

@linusg linusg merged commit e86d7ca into SerenityOS:master Jul 14, 2023
@github-actions github-actions bot removed the 👀 pr-needs-review PR needs review from a maintainer or community member label Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants