fix(vm): handle AsyncGeneratorAwaitReturn PromiseResolve abrupt completion (#709)#958
fix(vm): handle AsyncGeneratorAwaitReturn PromiseResolve abrupt completion (#709)#958replygirl wants to merge 3 commits intotrynova:mainfrom
Conversation
aapoalas
left a comment
aapoalas
left a comment
There was a problem hiding this comment.
Alright, nice! This is a meaningful change that fixes some issues, very very nice indeed!
Some issues remain though, let's fix those before merging this <3
| /// | ||
| /// Internal variant for call sites that must handle abrupt completion from | ||
| /// `PromiseResolve(%Promise%, x)`. | ||
| pub fn resolve_maybe_abrupt( |
There was a problem hiding this comment.
issue: Okay, reading this with a little more thought, the only difference between this and the existing fn resolve(...) is that this actually performs the same_value check.
There is no reason to add a new method for that; we can just use the existing fn resolve and, optionally, add the same_value check path there, although it is currently meaningless as we never have any Promise subclasses.
So: this method should be removed and you should instead just call the normal fn resolve function.
If you want to add the same_value path in the fn resolve, let's do that in a separate PR again. There is an issue regarding the GC lifetime of x in this implementation that we'd need to fix.
| "staging/source-phase-imports/import-source-source-text-module.js": "FAIL", | ||
| "staging/top-level-await/tla-hang-entry.js": "FAIL" | ||
| } No newline at end of file | ||
| } |
There was a problem hiding this comment.
thought: I think this'll lead to an issue with the test runner, unfortunately :(
| "built-ins/AsyncGeneratorFunction/proto-from-ctor-realm-prototype.js": "FAIL", | ||
| "built-ins/AsyncGeneratorFunction/proto-from-ctor-realm.js": "FAIL", | ||
| "built-ins/AsyncGeneratorFunction/prototype/constructor.js": "FAIL", | ||
| "built-ins/AsyncGeneratorPrototype/return/return-state-completed-broken-promise.js": "FAIL", |
| // a. Perform AsyncGeneratorCompleteStep(generator, promiseCompletion, true). | ||
| // b. Perform AsyncGeneratorDrainQueue(generator). | ||
| // c. Return unused. | ||
| let return_value = value.unbind().bind(gc.nogc()); |
There was a problem hiding this comment.
nitpick (blocking): No reason to do this line; just pass in value.unbind() as a parameter on the next line.
| // b. Perform AsyncGeneratorDrainQueue(generator). | ||
| // c. Return unused. | ||
| let return_value = value.unbind().bind(gc.nogc()); | ||
| let promise_completion = Promise::resolve_maybe_abrupt(agent, return_value.unbind(), gc.reborrow()) |
There was a problem hiding this comment.
issue: Instead of the map and map_err function calls, just do .unbind().bind(gc.nogc()) after the call.
| let promise_completion = Promise::resolve_maybe_abrupt(agent, return_value.unbind(), gc.reborrow()) | |
| let promise_completion = Promise::resolve_maybe_abrupt(agent, return_value.unbind(), gc.reborrow()).unbind().bind(gc.nogc()); |
| async_generator_drain_queue(agent, scoped_generator, gc); | ||
| return; | ||
| } | ||
| Ok(promise) => promise.unbind().bind(gc.nogc()), |
There was a problem hiding this comment.
nitpick: No need to rebind here
| Ok(promise) => promise.unbind().bind(gc.nogc()), | |
| Ok(promise) => promise, |
| }; | ||
| // 7. Let promiseCompletion be Completion(PromiseResolve(%Promise%, completion.[[Value]])). | ||
| // 8. If promiseCompletion is an abrupt completion, then | ||
| // a. Perform AsyncGeneratorCompleteStep(generator, promiseCompletion, true). |
There was a problem hiding this comment.
nitpick: Move these spec coments into the Err(err) => {} branch below.
| unreachable!() | ||
| }; | ||
| // 7. Let promiseCompletion be Completion(PromiseResolve(%Promise%, completion.[[Value]])). | ||
| // 8. If promiseCompletion is an abrupt completion, then |
There was a problem hiding this comment.
nitpick: Move this spec comment just before the match promise_completion line.
i'm dead but will come back around in the next few days :) |
replygirl
changed the title
|
Too much simplification. |
LMAO i'm too tired |
replygirl
force-pushed
the
fix/vm-709-await-return-promise-abrupt
branch
from
68a5f07 to
13f2caa
Compare
replygirl
changed the title
2 participants
Split from #950
Fixes #709
Refs #950