Implement async generator that support following function
async function *foo() {
let t = await boo('async iteration ');
yield t + 'step-1';
yield t + 'step-2';
}
var f = foo();
f.next().then(({value, done}) => debug(value));
f.next().then(({value, done}) => debug(value));
Comment on attachment 298142[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=298142&action=review
Cool stuff so far. I'd like to look at this more on Friday.
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:39
> + @throwTypeError("Async generator is executing");
Async Generators differ from ordinary generators, in that it's not an error to resume the generator while it's executing.
While it's an unlikely case, the generator is allowed to queue up additional requests while executing, and the queue of requests is drained during resumption (the proposal words this as recursive calls from AsyncGeneratorResolve / AsyncGeneratorReject -> AsyncGeneratorResumeNext)
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:65
> + if (generator.@asyncGeneratorState === @AsyncGeneratorStateAwait) {
the special generator continuation is a nice idea.
Though it doesn't affect JSC just yet due to not being implemented, note that this would _probably_ be observable via the function.sent metaproperty (or at least, it is in the v8 generator implementation without some additional work).
Comment on attachment 298142[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=298142&action=review>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:39
>> + @throwTypeError("Async generator is executing");
>
> Async Generators differ from ordinary generators, in that it's not an error to resume the generator while it's executing.
>
> While it's an unlikely case, the generator is allowed to queue up additional requests while executing, and the queue of requests is drained during resumption (the proposal words this as recursive calls from AsyncGeneratorResolve / AsyncGeneratorReject -> AsyncGeneratorResumeNext)
Sure I'll remove this line.
I've tried to replace queue by chain of promises to allow invoke 'next' many times as well. Also I have not implement&tests methods 'return' & 'throw', not sure that they work correct in this patch.
>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:65
>> + if (generator.@asyncGeneratorState === @AsyncGeneratorStateAwait) {
>
> the special generator continuation is a nice idea.
>
> Though it doesn't affect JSC just yet due to not being implemented, note that this would _probably_ be observable via the function.sent metaproperty (or at least, it is in the v8 generator implementation without some additional work).
Oh Sorry, not sure that I got it.
Do you mean that in case of using chain of promises instead of queue, we will have visible side effects when function.sent would be implemented?
Could you please provide some examples that you faced in v8? I'll try to emulate on current solution, possible current approach is not viable at all.
Also it seems that I need to replace this by:
let wrappedValue = @newPromiseCapability(@Promise);
wrappedValue.@resolve.@call(@undefined, value);
wrappedValue.@promise.@then(
function(result) { @asyncGeneratorResume(generator, promiseCapability, result, @GeneratorResumeModeNormal); },
function(error) { @asyncGeneratorResume(generator, promiseCapability, error, @GeneratorResumeModeThrow); });
Because value is returned by await can be non promise value.
Comment on attachment 298142[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=298142&action=review>>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:65
>>> + if (generator.@asyncGeneratorState === @AsyncGeneratorStateAwait) {
>>
>> the special generator continuation is a nice idea.
>>
>> Though it doesn't affect JSC just yet due to not being implemented, note that this would _probably_ be observable via the function.sent metaproperty (or at least, it is in the v8 generator implementation without some additional work).
>
> Oh Sorry, not sure that I got it.
> Do you mean that in case of using chain of promises instead of queue, we will have visible side effects when function.sent would be implemented?
> Could you please provide some examples that you faced in v8? I'll try to emulate on current solution, possible current approach is not viable at all.
>
> Also it seems that I need to replace this by:
>
> let wrappedValue = @newPromiseCapability(@Promise);
> wrappedValue.@resolve.@call(@undefined, value);
> wrappedValue.@promise.@then(
> function(result) { @asyncGeneratorResume(generator, promiseCapability, result, @GeneratorResumeModeNormal); },
> function(error) { @asyncGeneratorResume(generator, promiseCapability, error, @GeneratorResumeModeThrow); });
> Because value is returned by await can be non promise value.
What I was getting at was about the `function.sent` metaproperty proposal (https://github.com/allenwb/ESideas/blob/master/Generator%20metaproperty.md), which allows the programmer to make `function.sent` an alias to the value the generator was resumed with (eg `generator.next("foo");` makes function.sent === "foo" inside the generator after resuming).
So, with Async Generators and await expressions, the awaited value would alias function.sent rather than the value sent via generator.next(), which observably breaks the metaproperty.
JSC does not implement function.sent yet, so it isn't really noticeable here, but it is something to pay attention to for the future, unless the proposal is withdrawn
Comment on attachment 298142[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=298142&action=review>>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:39
>>> + @throwTypeError("Async generator is executing");
>>
>> Async Generators differ from ordinary generators, in that it's not an error to resume the generator while it's executing.
>>
>> While it's an unlikely case, the generator is allowed to queue up additional requests while executing, and the queue of requests is drained during resumption (the proposal words this as recursive calls from AsyncGeneratorResolve / AsyncGeneratorReject -> AsyncGeneratorResumeNext)
>
> Sure I'll remove this line.
> I've tried to replace queue by chain of promises to allow invoke 'next' many times as well. Also I have not implement&tests methods 'return' & 'throw', not sure that they work correct in this patch.
I read spec one more time and it seems that implementation of 'AsyncGenerator Abstract Operations' with AsyncGeneratorResolve / AsyncGeneratorReject -> AsyncGeneratorResumeNext will require some additional changes in 'generator' and 'async function', so I decided that it should be not be part of this patch, and be part of the separate task - https://bugs.webkit.org/show_bug.cgi?id=166848>>>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:65
>>>> + if (generator.@asyncGeneratorState === @AsyncGeneratorStateAwait) {
>>>
>>> the special generator continuation is a nice idea.
>>>
>>> Though it doesn't affect JSC just yet due to not being implemented, note that this would _probably_ be observable via the function.sent metaproperty (or at least, it is in the v8 generator implementation without some additional work).
>>
>> Oh Sorry, not sure that I got it.
>> Do you mean that in case of using chain of promises instead of queue, we will have visible side effects when function.sent would be implemented?
>> Could you please provide some examples that you faced in v8? I'll try to emulate on current solution, possible current approach is not viable at all.
>>
>> Also it seems that I need to replace this by:
>>
>> let wrappedValue = @newPromiseCapability(@Promise);
>> wrappedValue.@resolve.@call(@undefined, value);
>> wrappedValue.@promise.@then(
>> function(result) { @asyncGeneratorResume(generator, promiseCapability, result, @GeneratorResumeModeNormal); },
>> function(error) { @asyncGeneratorResume(generator, promiseCapability, error, @GeneratorResumeModeThrow); });
>> Because value is returned by await can be non promise value.
>
> What I was getting at was about the `function.sent` metaproperty proposal (https://github.com/allenwb/ESideas/blob/master/Generator%20metaproperty.md), which allows the programmer to make `function.sent` an alias to the value the generator was resumed with (eg `generator.next("foo");` makes function.sent === "foo" inside the generator after resuming).
>
> So, with Async Generators and await expressions, the awaited value would alias function.sent rather than the value sent via generator.next(), which observably breaks the metaproperty.
>
> JSC does not implement function.sent yet, so it isn't really noticeable here, but it is something to pay attention to for the future, unless the proposal is withdrawn
OK. I'll keep in mind. Thanks for information.
Comment on attachment 298373[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=298373&action=review
quick review. I'll review it in detail later.
> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1179
> + dataLogF("Creating async function!\n");
Nit: `async generator function`.
> Source/JavaScriptCore/parser/Parser.cpp:2740
> + bool isAsyncGeneratorMethod = false;
Instead of adding more flags, cleaning up these flags is better.
You can combine `isGenerator`, `isAsync`, `isAsyncGenerator`, `isAsyncMethod`, `isAsyncGeneratorMethod` etc. carefully to one enum, right?
> Source/JavaScriptCore/parser/Parser.cpp:3808
> + bool isAsyncGenerator = false;
> + bool isAsyncGeneratorMethod = false;
Ditto
> Source/JavaScriptCore/runtime/AsyncGeneratorFunctionConstructor.h:36
> + typedef InternalFunction Base;
For the newer code, let's use `using Base = JSFunction;`.
> Source/JavaScriptCore/runtime/AsyncGeneratorFunctionPrototype.h:33
> + typedef JSNonFinalObject Base;
For the newer code, let's use `using Base = JSFunction;`.
> Source/JavaScriptCore/runtime/AsyncGeneratorPrototype.h:36
> + typedef JSNonFinalObject Base;
For the newer code, let's use `using Base = JSFunction;`.
> Source/JavaScriptCore/runtime/AsyncIteratorPrototype.h:34
> + typedef JSNonFinalObject Base;
For the newer code, let's use `using Base = JSFunction;`.
> Source/JavaScriptCore/runtime/JSAsyncGeneratorFunction.h:36
> + typedef JSFunction Base;
For the newer code, let's use `using Base = JSFunction;`.
> Source/JavaScriptCore/runtime/JSFunction.cpp:356
> + // prototype = constructEmptyObject(exec, thisObject->globalObject(vm)->asyncGeneratorPrototype());
This line is not necessary.
> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1223
> + visitor.append(thisObject->m_asyncGeneratorPrototype);
m_asyncIteratorPrototype and m_asyncGeneratorFunctionPrototype need to be visited.
Created attachment 300024[details]
Archive of layout-test-results from ews101 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 300025[details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 300026[details]
Archive of layout-test-results from ews112 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 300027[details]
Archive of layout-test-results from ews124 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
(In reply to comment #10)
> Can you rebase and measure the parser performance by octane closure and
> jquery?
> I'll attempt to review it tomorrow in JST.
I've uploaded rebased version of the patch for Async generator. It builds :-), but there are some already existed tests fail because async generator adds some new entities and allow some new syntax, but not sure that this is important for now, because as for me important to know if approach to implement async generator is correct.
(In reply to comment #10)
> Can you rebase and measure the parser performance by octane closure and
> jquery?
> I'll attempt to review it tomorrow in JST.
Later I'll provide performance test results.
Created attachment 300041[details]
Archive of layout-test-results from ews100 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 300042[details]
Archive of layout-test-results from ews117 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 300043[details]
Archive of layout-test-results from ews122 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 300053[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=300053&action=review
Nice! Could you fix gtk/efl/win build failures?
> Source/JavaScriptCore/ChangeLog:10
> + and async function. This will be fixed in https://bugs.webkit.org/show_bug.cgi?id=166848
Could you explain more detailed design in ChangeLog?
For example, how to implement async iterator's await?
What is the difference between async function and async generator?
What is the difference between async generator and generator?
etc.
It helps so much when we extend / modify / debug the features.
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:30
> +// FIXME: Current implementation is not follow spec in part of
> +// AsyncGenerator Abstract Operations
> +// (https://tc39.github.io/proposal-async-iteration/#sec-asyncgenerator-abstract-operations),
> +// to align implementation with spec created issue
> +// https://bugs.webkit.org/show_bug.cgi?id=166848
What is the observable behavior?
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:54
> +
> + promiseCapability.@resolve({ value, done });
Drop this (see the latter comment).
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:71
> + wrappedValue.@promise.@then(
Is this `@then` use correct?
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:73
> + function(error) { @asyncGeneratorResume(generator, promiseCapability, error, @GeneratorResumeModeThrow); });
return here.
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:78
> + promiseCapability.@reject(error);
return here.
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:79
> + }
Move `promiseCapability.@resolve({ value, done });` here for both state case.
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:89
> + generator.@asyncGeneratorResumePromise.@then(
Is this `@then` correct?
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:545
> + case SourceParseMode::AsyncGeneratorMethodMode:
Let's name AsyncGeneratorWrapperMethodMode.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:546
> + case SourceParseMode::AsyncGeneratorFunctionMode:
Let's name AsyncGeneratorWrapperFunctionMode.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3164
> + case SourceParseMode::AsyncGeneratorFunctionMode:
Let's name AsyncGeneratorWrapperFunctionMode.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3165
> + case SourceParseMode::AsyncGeneratorMethodMode:
Let's name AsyncGeneratorWrapperMethodMode.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3222
> + else if (function->parseMode() == SourceParseMode::AsyncGeneratorFunctionMode)
Let's name AsyncGeneratorWrapperFunctionMode.
> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3471
> + case SourceParseMode::AsyncGeneratorMethodMode:
Let's name AsyncGeneratorWrapperMethodMode.
> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3472
> + case SourceParseMode::AsyncGeneratorFunctionMode: {
Let's name AsyncGeneratorWrapperFunctionMode.
> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6432
> + // TODO: Rename to JSAsyncGenerator
We do not use `TODO:`. Use `FIXME:` instead.
And if you use FIXME, the comment should have the bug URL.
> Source/JavaScriptCore/parser/Parser.cpp:482
> +// TODO: Think about rename to parseAsyncFunctionOrAsyncGeneratorSourceElements
We do not use TODO. Use `FIXME` instead.
And if you use FIXME, the comment should have a bug URL.
> Source/JavaScriptCore/parser/Parser.cpp:1988
> + case SourceParseMode::AsyncGeneratorFunctionMode:
Let's name AsyncGeneratorWrapperFunctionMode.
> Source/JavaScriptCore/parser/Parser.cpp:1991
> + case SourceParseMode::AsyncGeneratorMethodMode:
Let's name AsyncGeneratorWrapperMethodMode.
> Source/JavaScriptCore/parser/Parser.cpp:2270
> + semanticFail("Cannot declare async function named 'await'"); // TODO: Fix text message for async generators
We do not use TODO. Use FIXME instead.
And if you use FIXME, the comment should have a bug URL.
> Source/JavaScriptCore/parser/Parser.cpp:2510
> + parseMode = SourceParseMode::AsyncGeneratorFunctionMode;
Let's name AsyncGeneratorWrapperFunctionMode.
> Source/JavaScriptCore/parser/Parser.cpp:2696
> + ident = m_token.m_data.ident;
I do not think this ident is non null. You need to check it, right?
If so, please add a test to cover this case since the following assert should fire for such a code right now.
> Source/JavaScriptCore/parser/Parser.cpp:2705
> + ? SourceParseMode::AsyncGeneratorMethodMode
Let's name AsyncGeneratorWrapperMethodMode.
> Source/JavaScriptCore/parser/Parser.cpp:2751
> + if (isAsyncMethodParseMode(parseMode)) {
> isConstructor = false;
> - parseMode = SourceParseMode::AsyncMethodMode;
> semanticFailIfTrue(*ident == m_vm->propertyNames->prototype, "Cannot declare an async method named 'prototype'");
> semanticFailIfTrue(*ident == m_vm->propertyNames->constructor, "Cannot declare an async method named 'constructor'");
> - } else if (isGenerator) {
> + } else if (isGeneratorMethodParseMode(parseMode)) {
> isConstructor = false;
> - parseMode = SourceParseMode::GeneratorWrapperFunctionMode;
> semanticFailIfTrue(*ident == m_vm->propertyNames->prototype, "Cannot declare a generator named 'prototype'");
> semanticFailIfTrue(*ident == m_vm->propertyNames->constructor, "Cannot declare a generator named 'constructor'");
> + } else if (isAsyncGeneratorMethodParseMode(parseMode)) {
> + isConstructor = false;
> + semanticFailIfTrue(*ident == m_vm->propertyNames->prototype, "Cannot declare a async generator named 'prototype'");
> + semanticFailIfTrue(*ident == m_vm->propertyNames->constructor, "Cannot declare a async generator named 'constructor'");
> }
Can you clean up these if-else? Only the difference is the error message.
You can unify these code and produce the appropriate error message based on the source parse mode.
Maybe, stringForFunctionMode can be used.
> Source/JavaScriptCore/parser/Parser.cpp:3745
> + isAsyncGenerator = true;
If you found `async *`, it immediately means that this is async generator, correct?
And you need to check whether the next token is identifier.
And could you cover the above case in the test?
> Source/JavaScriptCore/parser/Parser.cpp:3798
> + parseMode = SourceParseMode::AsyncGeneratorMethodMode;
Let's name AsyncGeneratorWrapperMethodMode.
> Source/JavaScriptCore/parser/Parser.cpp:3855
> + ASSERT(parseMode == SourceParseMode::MethodMode || parseMode == SourceParseMode::GeneratorWrapperFunctionMode || parseMode == SourceParseMode::AsyncMethodMode || parseMode == SourceParseMode::AsyncGeneratorMethodMode);
Let's name AsyncGeneratorWrapperMethodMode.
> Source/JavaScriptCore/parser/Parser.cpp:4164
> + parseMode = SourceParseMode::AsyncGeneratorFunctionMode;
Let's name AsyncGeneratorWrapperFunctionMode.
> Source/JavaScriptCore/parser/Parser.h:279
> + case SourceParseMode::AsyncGeneratorMethodMode:
Let's name AsyncGeneratorWrapperMethodMode.
> Source/JavaScriptCore/parser/Parser.h:280
> + case SourceParseMode::AsyncGeneratorFunctionMode:
Let's name AsyncGeneratorWrapperFunctionMode.
> Source/JavaScriptCore/parser/ParserModes.h:62
> + AsyncGeneratorFunctionMode = 0b00000000000000010000000000000000,
Let's name AsyncGeneratorWrapperFunctionMode.
> Source/JavaScriptCore/parser/ParserModes.h:63
> + AsyncGeneratorMethodMode = 0b00000000000000100000000000000000,
Let's name AsyncGeneratorWrapperMethodMode.
> Source/JavaScriptCore/parser/ParserModes.h:110
> + SourceParseMode::AsyncGeneratorFunctionMode,
Let's name AsyncGeneratorWrapperFunctionMode.
> Source/JavaScriptCore/parser/ParserModes.h:111
> + SourceParseMode::AsyncGeneratorMethodMode).contains(parseMode);
Let's name AsyncGeneratorWrappeMethodMode.
> Source/JavaScriptCore/parser/ParserModes.h:117
> + SourceParseMode::AsyncGeneratorFunctionMode,
Let's name AsyncGeneratorWrapperFunctionMode.
> Source/JavaScriptCore/parser/ParserModes.h:119
> + SourceParseMode::AsyncGeneratorMethodMode,
Let's name AsyncGeneratorWrapperMethodMode.
> Source/JavaScriptCore/parser/ParserModes.h:137
> + SourceParseMode::AsyncGeneratorFunctionMode,
Let's name AsyncGeneratorWrapperFunctionMode.
> Source/JavaScriptCore/parser/ParserModes.h:138
> + SourceParseMode::AsyncGeneratorMethodMode).contains(parseMode);
Let's name AsyncGeneratorWrapperMethodMode.
> Source/JavaScriptCore/parser/ParserModes.h:146
> + SourceParseMode::AsyncGeneratorFunctionMode,
Let's name AsyncGeneratorWrapperFunctionMode.
> Source/JavaScriptCore/parser/ParserModes.h:147
> + SourceParseMode::AsyncGeneratorMethodMode,
Let's name AsyncGeneratorWrapperMethodMode.
> Source/JavaScriptCore/parser/ParserModes.h:170
> + // FIXME: GeneratorWrapperFunctionMode is not guaranteed to be a method.
Can you explain this? It seems wrong.
If you need GeneratorWrapperMethod type, I think implementing it in the separate patch is better. (It should be very small I guess).
After that patch is landed, we can use it in this patch.
> Source/JavaScriptCore/parser/ParserModes.h:181
> + return SourceParseModeSet(SourceParseMode::AsyncGeneratorFunctionMode).contains(parseMode);
Let's name AsyncGeneratorWrapperFunctionMode.
> Source/JavaScriptCore/parser/ParserModes.h:193
> + SourceParseMode::AsyncGeneratorMethodMode).contains(parseMode);
Let's name AsyncGeneratorWrapperMethodMode.
> Source/JavaScriptCore/parser/ParserModes.h:211
> + SourceParseMode::AsyncGeneratorFunctionMode,
Let's name AsyncGeneratorWrapperFunctionMode.
> Source/JavaScriptCore/parser/ParserModes.h:213
> + SourceParseMode::AsyncGeneratorMethodMode).contains(parseMode);
Let's name AsyncGeneratorWrapperMethodMode.
> Source/JavaScriptCore/runtime/AsyncGeneratorFunctionConstructor.cpp:49
> + // Number of arguments for constructor
This comment is unnecessary.
> Source/JavaScriptCore/runtime/AsyncGeneratorFunctionConstructor.cpp:56
> + // TODO: Use Async Generator instead
We do not use TODO. Use FIXME instead.
And if you use FIXME, the comment should have a bug URL.
> Source/JavaScriptCore/runtime/AsyncGeneratorPrototype.h:37
> + static const unsigned StructureFlags = Base::StructureFlags | HasStaticPropertyTable;
We do not have static property table for this. Drop HasStaticPropertyTable.
> Source/JavaScriptCore/runtime/FunctionExecutable.h:126
> + bool isAsyncGenerator() const { return SourceParseModeSet(SourceParseMode::AsyncGeneratorFunctionMode, SourceParseMode::AsyncGeneratorBodyMode).contains(parseMode()); }
Let's name AsyncGeneratorWrapperFunctionMode.
> Source/JavaScriptCore/runtime/FunctionExecutable.h:141
> + SourceParseMode::AsyncGeneratorFunctionMode,
Let's name AsyncGeneratorWrapperFunctionMode.
> Source/JavaScriptCore/runtime/JSFunction.cpp:355
> + } else if (thisObject->jsExecutable()->parseMode() == SourceParseMode::AsyncGeneratorFunctionMode)
Let's name AsyncGeneratorWrapperFunctionMode.
Comment on attachment 300609[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=300609&action=review
I've left a few runtime comments, but I do understand that this patch does not claim to implement the runtime perfectly.
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:90
> + generator.@asyncGeneratorResumePromise.@then(
I believe it's observable to implement the queue this way --- AsyncGeneratorResolve and AsyncGeneratorReject immediately begin resuming subsequent requests if the queue is not empty, but this waits until a later turn. I think this could cause weird races between implementations which match the spec and implementations which do this.
Maybe Domenic can indicate if this is the intent or not
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:686
> + bool needTryCatch = isAsyncFunctionOrAsyncGeneratorWrapperParseMode(parseMode) && !isSimpleParameterList;
async generators don't need a try/catch for the parameter list, they just throw synchronously if parameters cause problems (https://tc39.github.io/proposal-async-iteration/#sec-asyncgenerator-definitions-evaluatebody), and do not return a Promise
> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:142
> + enum class Kind { Escaped, Object, Activation, Function, GeneratorFunction, AsyncFunction, AsyncGeneratorFunction };
I think I would personally prefer it if DFG/FTL passes were untouched in the initial patch, but I suppose that will be left up to reviewers.
> Source/JavaScriptCore/parser/Parser.h:261
> + setIsGenerator();
This stuff is getting very complicated, I feel there has to be a better way to track function scope types. I feel this is very error prone.
Comment on attachment 305431[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=305431&action=review> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:77
I think it might be wise to make AsyncGeneratorResumeNext() behave as a loop, so that this recursion can be avoided when there are multiple yields and multiple requests in the queue, unless the spec is changed to get rid of the "evaluate the next item in queue after a yield immediately".
I'm sure I've said this in this bug before, but I see that we're still doing this recursively, and without PTC (though I'm not sure if PTC would help much in this case).
Comment on attachment 305431[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=305431&action=review>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:77
>> + @asyncGeneratorResumeNext(generator);
>
> I think it might be wise to make AsyncGeneratorResumeNext() behave as a loop, so that this recursion can be avoided when there are multiple yields and multiple requests in the queue, unless the spec is changed to get rid of the "evaluate the next item in queue after a yield immediately".
>
> I'm sure I've said this in this bug before, but I see that we're still doing this recursively, and without PTC (though I'm not sure if PTC would help much in this case).
Thanks for quick feedback!
Yeah, I remember your comment https://bugs.webkit.org/show_bug.cgi?id=166695#c39, however I've tried to be close to spec as much as I can, so I implemented (p 6.4.3.3.11).
But you are right, using loop is better in this place, so I'll try to switch to approach with loop.
Comment on attachment 305431[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=305431&action=review>>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:77
>>> + @asyncGeneratorResumeNext(generator);
>>
>> I think it might be wise to make AsyncGeneratorResumeNext() behave as a loop, so that this recursion can be avoided when there are multiple yields and multiple requests in the queue, unless the spec is changed to get rid of the "evaluate the next item in queue after a yield immediately".
>>
>> I'm sure I've said this in this bug before, but I see that we're still doing this recursively, and without PTC (though I'm not sure if PTC would help much in this case).
>
> Thanks for quick feedback!
>
> Yeah, I remember your comment https://bugs.webkit.org/show_bug.cgi?id=166695#c39, however I've tried to be close to spec as much as I can, so I implemented (p 6.4.3.3.11).
> But you are right, using loop is better in this place, so I'll try to switch to approach with loop.
I added loop into AsyncGeneratorResumeNext in new patch, hope it decrees recursion level. Could you please take a look if it make sense?
Comment on attachment 305496[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=305496&action=review
Looks good overall, just a couple comments.
Obviously, it would be great if Sam or one of the other JSC core folks could give their opinion on it, since I can't sign off on WebKit patches.
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:57
> + @asyncGeneratorResumeNext(generator);
Parameterizing the recursion seems weird to me still. I've suggested some changes that make this unnecessary
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:80
> + @asyncGeneratorResumeNext(generator);
ditto here
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:114
> + const restartResumeNext = true;
There's no need to recurse here, really. You can just call AsyncGeneratorResumeNext at the end of the method instead (and, you could even PTC to it in that case, since there's no generator on the stack at this point)
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:125
> + const restartResumeNext = true;
ditto here
Comment on attachment 305496[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=305496&action=review>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:114
>> + const restartResumeNext = true;
>
> There's no need to recurse here, really. You can just call AsyncGeneratorResumeNext at the end of the method instead (and, you could even PTC to it in that case, since there's no generator on the stack at this point)
I fixed in new patch, I hope it is what you mean.
Comment on attachment 305500[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=305500&action=review> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:117
> + } else {
So the last thing is these handlers.
You could simplify this:
```
wrappedValue.@promise.@then(
function(result) {
const isDelegetedYield = generator.@asyncGeneratorSuspendReason === @AsyncGeneratorSuspendReasonDelegatedYield;
generator.@asyncGeneratorSuspendReason = @AsyncGeneratorSuspendReasonNone;
if (isDelegetedYield) {
const isDone = false;
@asyncGeneratorResolve(generator, result.value, isDone);
}
return @asyncGeneratorResumeNext(generator);
},
function(error) {
generator.@generatorState = @AsyncGeneratorStateCompleted; // see note below
@asyncGeneratorReject(generator, error);
@asyncGeneratorResumeNext(generator);
}
```
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:123
> + generator.@generatorState = @AsyncGeneratorStateCompleted;
This actually looks wrong to me, per the current spec. Unless the committee decides to go with the "yielded rejections don't affect control flow of the generator" thing (which is admittedly pretty popular), closing the generator should be left up to @asyncGeneratorResume().
Even if the proposal is changed, this will prevent finally blocks from being reached.
E.g.
```js
let reject;
async function* foo() {
try {
yield new Promise(function(unused, rej) { reject = rej; });
} finally {
doImportantCleanupStuff(); // In this patch, this appears to be unreachable
return 0;
}
}
foo().next().then(x => print(x), e => print(e));
reject("Oop!");
```
I could be wrong about this, but I _believe_ this is broken in this patch due to the generator state change here, and I think it is supposed to work in the current spec
Comment on attachment 305500[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=305500&action=review>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:117
>> + } else {
>
> So the last thing is these handlers.
>
> You could simplify this:
>
> ```
> wrappedValue.@promise.@then(
> function(result) {
> const isDelegetedYield = generator.@asyncGeneratorSuspendReason === @AsyncGeneratorSuspendReasonDelegatedYield;
> generator.@asyncGeneratorSuspendReason = @AsyncGeneratorSuspendReasonNone;
> if (isDelegetedYield) {
> const isDone = false;
> @asyncGeneratorResolve(generator, result.value, isDone);
> }
> return @asyncGeneratorResumeNext(generator);
> },
> function(error) {
> generator.@generatorState = @AsyncGeneratorStateCompleted; // see note below
> @asyncGeneratorReject(generator, error);
> @asyncGeneratorResumeNext(generator);
> }
> ```
Yeah, it seems my patch is not clear enough, because in this asyncGeneratorResume function in resolve callback for wrappedValue, is handled case when we suspend execution control flow because of await or yield*: AsyncGeneratorSuspendReasonAwait/AsyncGeneratorSuspendReasonDelegatedYield
For suspended await we need run current function again because because we need to continue execution of function until faced with 'yield' or new await.
For value from async generator function that invoked by yield* we need to unwrap value, because it returns promise.
>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:123
>> + generator.@generatorState = @AsyncGeneratorStateCompleted;
>
> This actually looks wrong to me, per the current spec. Unless the committee decides to go with the "yielded rejections don't affect control flow of the generator" thing (which is admittedly pretty popular), closing the generator should be left up to @asyncGeneratorResume().
>
> Even if the proposal is changed, this will prevent finally blocks from being reached.
>
> E.g.
>
> ```js
> let reject;
> async function* foo() {
> try {
> yield new Promise(function(unused, rej) { reject = rej; });
> } finally {
> doImportantCleanupStuff(); // In this patch, this appears to be unreachable
> return 0;
> }
> }
>
> foo().next().then(x => print(x), e => print(e));
> reject("Oop!");
> ```
>
> I could be wrong about this, but I _believe_ this is broken in this patch due to the generator state change here, and I think it is supposed to work in the current spec
I see following result:
do important staff
returned value
Oop!
Is this result close to spec or 'important staff' should be returned after Oop!
To test I used little bit modified function:
```
async function* foo() {
try {
yield new Promise(function(unused, rej) { reject = rej; });
} finally {
print('do important staff');
return 'returned value';
}
}
var f= foo()
f.next().then(({value}) => print(value), e => print(e));
reject("Oop!");
f.next().then(({value}) => print(value), e => print(e));
drainMicrotasks();
```
Comment on attachment 305500[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=305500&action=review>>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:123
>>> + generator.@generatorState = @AsyncGeneratorStateCompleted;
>>
>> This actually looks wrong to me, per the current spec. Unless the committee decides to go with the "yielded rejections don't affect control flow of the generator" thing (which is admittedly pretty popular), closing the generator should be left up to @asyncGeneratorResume().
>>
>> Even if the proposal is changed, this will prevent finally blocks from being reached.
>>
>> E.g.
>>
>> ```js
>> let reject;
>> async function* foo() {
>> try {
>> yield new Promise(function(unused, rej) { reject = rej; });
>> } finally {
>> doImportantCleanupStuff(); // In this patch, this appears to be unreachable
>> return 0;
>> }
>> }
>>
>> foo().next().then(x => print(x), e => print(e));
>> reject("Oop!");
>> ```
>>
>> I could be wrong about this, but I _believe_ this is broken in this patch due to the generator state change here, and I think it is supposed to work in the current spec
>
> I see following result:
>
> do important staff
> returned value
> Oop!
>
> Is this result close to spec or 'important staff' should be returned after Oop!
> To test I used little bit modified function:
> ```
> async function* foo() {
> try {
> yield new Promise(function(unused, rej) { reject = rej; });
> } finally {
> print('do important staff');
> return 'returned value';
> }
> }
>
> var f= foo()
> f.next().then(({value}) => print(value), e => print(e));
> reject("Oop!");
> f.next().then(({value}) => print(value), e => print(e));
> drainMicrotasks();
> ```
In #jslang I made a gist and spent some time analyzing it, didn't get through the whole thing, but I pretty much get the same result, so I think it's okay. https://gist.github.com/caitp/9af4de75c5ac6ff7d22b366472617e21
And yeah, the finally block would be reached if another request is queued up, regardless of whether the rejection affects control flow or not. But it still shouldn't close the generator, that doesn't really make sense.
Created attachment 307034[details]
Archive of layout-test-results from ews126 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 307013[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=307013&action=review
Wowza, this is a huge patch. I need to review the spec before I can responsibly review this. I'll get back to this tomorrow.
Maybe Yusuke/Caitlin can also take a look, since they implemented generators/async functions.
> Source/JavaScriptCore/ChangeLog:8
> + Current implementation is draft version of Async Iteration.
Can you link to the spec here?
> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:153
> + function(result) { promiseCapability.@resolve.@call(@undefined, { value: result, done }); },
> + function(error) { promiseCapability.@reject.@call(@undefined, error); });
style: indentation should be 4 lines from the statement's starting indentation
Comment on attachment 307013[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=307013&action=review
I haven't gone over the whole patch yet.
So, I'm a bit confused about what is gained from having a new "suspend reason" for yield*. Is the goal here to shrink the generated bytecode by performing the Await logic in the runtime function?
I've checked for compliance with the couple of spec changes that have landed upstream since I first implemented this in v8, and left a few pointers that might be helpful for improving compliance.
> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:62
> + const { value, done } = nextResult;
Per https://github.com/tc39/proposal-async-iteration/commit/395b2e3b2f5acb62f9fae11c5e189423d4af50e6, you need to load `done` before `value` in all of these methods. This is intended to match what yield* does in async generators, and there are tests verifying this in test262.
> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:104
> + const { value, done } = returnResult;
ditto here
> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:147
> + const { value, done } = throwResult;
ditto here
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4777
> + const Identifier& propertyName = parseMode() == SourceParseMode::AsyncGeneratorBodyMode ? propertyNames().asyncIteratorSymbol : propertyNames().iteratorSymbol;
I might suggest putting this logic into an `emitGetIterator(Register iterable)` helper to make it easier to maintain and understand
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:-4783
> -
nit: removing this whitespace is noise in the diff
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4881
> + emitYieldPoint(value.get(), JSAsyncGeneratorFunction::AsyncGeneratorSuspendReason::Await);
For readability, how about something like `Register emitAwait(dstRegister, promiseRegister)`? This pattern is repeating a lot and it's more to think about than is desirable maybe
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4906
> emitIteratorNextWithValue(value.get(), iterator.get(), value.get(), node);
I think the correct thing is:
```
// ...
emitLabel(nextElement.get());
emitIteratorNextWithValue(value.get(), iterator.get(), value.get(), node);
if (parseMode() == SourceParseMode::AsyncGeneratorBodyMode) {
// If generatorKind is async, then set innerResult to ? Await(innerResult).
RefPtr<RegisterID> result = emitYield(value.get(), JSAsyncGeneratorFunction::AsyncGeneratorSuspendReason::Await);
emitMove(value.get(), result.get());
}
emitJumpIfTrue(emitGetById(newTemporary(), value.get(), propertyNames().done), loopDone.get());
emitGetById(value.get(), value.get(), propertyNames().value);
emitJump(loopStart.get();
// ...
```
I don't think the generator state check or the other stuff is needed for async generators. I believe you're trying to handle the Awaits inside @asyncGeneratorResumeNext(), but AFAICT this separates the logic from the control flow here, which likely will cause bugs.
Instead, I think you should just add Awaits in the places the spec requires you to
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4912
> + emitEqualityOp(op_stricteq, condition.get(), state.get(), emitLoad(nullptr, jsNumber(static_cast<int32_t>(JSGeneratorFunction::GeneratorState::Completed))));
Is this logic not handled by AsyncGeneratorResumeNext()?
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4914
> + emitJumpIfTrue(emitGetById(newTemporary(), value.get(), propertyNames().done), loopDone.get());
This looks like it should be loading the "value" property, not "done"
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4930
> +
Nit: unnecessary linefeed
> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3494
> +
nit: trailing whitespace sneaking in
> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3497
> +
nit: trailing whitespace sneaking in
> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3502
> +
here too
> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3507
> +
and here
> Source/JavaScriptCore/dfg/DFGNode.h:594
> + ASSERT(m_op == NewFunction || m_op == NewGeneratorFunction || m_op == NewAsyncFunction || m_op == NewAsyncGeneratorFunction);
I still would prefer if changes to DFG/FTL could be deferred to a later bug, in order to keep the initial implementation as simple as possible.
> Source/JavaScriptCore/parser/ParserModes.h:61
> + AsyncGeneratorBodyMode = 0b00000000000000001000000000000000,
I would suggest making AsyncGenerator<*>Mode == Generator<*>Mode | AsyncFunction<*>Mode, in order to make it easier for the parser (eg, easier to determine that `yield` or `await` is legal). But, if it's already done this way without hurting performance, I suppose it's fine.
> Source/JavaScriptCore/runtime/AsyncGeneratorFunctionConstructor.cpp:37
> +const ClassInfo AsyncGeneratorFunctionConstructor::s_info = { "AsyncGenerator", &Base::s_info, nullptr, CREATE_METHOD_TABLE(AsyncGeneratorFunctionConstructor) };
className should be "AsyncGeneratorFunction", similar to the className for GeneratorFunctionConstructor
> Source/JavaScriptCore/runtime/FunctionConstructor.cpp:114
> + prefix = "{async function *";
I don't see a change to functionProtoFuncToString() in FunctionPrototype.cpp, but just FYI this formatting contradicts the test262 tests. We seem to have decided on "async function*" (https://github.com/tc39/test262/commit/da764cafa28ea15b194ac545dc1b67c707c62296), though the proposal doesn't really get into it IIRC.
This comment is just about "you should add the change to functionProtoFuncToString(), with no space between `function` and `*`"
Comment on attachment 307013[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=307013&action=review>> Source/JavaScriptCore/ChangeLog:8
>> + Current implementation is draft version of Async Iteration.
>
> Can you link to the spec here?
Done
>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:62
>> + const { value, done } = nextResult;
>
> Per https://github.com/tc39/proposal-async-iteration/commit/395b2e3b2f5acb62f9fae11c5e189423d4af50e6, you need to load `done` before `value` in all of these methods. This is intended to match what yield* does in async generators, and there are tests verifying this in test262.
I've changed to const { done, value } = nextResult;
I hope it Is this correct
>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:104
>> + const { value, done } = returnResult;
>
> ditto here
Done
>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:147
>> + const { value, done } = throwResult;
>
> ditto here
Done
>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:153
>> + function(error) { promiseCapability.@reject.@call(@undefined, error); });
>
> style: indentation should be 4 lines from the statement's starting indentation
Done
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4777
>> + const Identifier& propertyName = parseMode() == SourceParseMode::AsyncGeneratorBodyMode ? propertyNames().asyncIteratorSymbol : propertyNames().iteratorSymbol;
>
> I might suggest putting this logic into an `emitGetIterator(Register iterable)` helper to make it easier to maintain and understand
Done
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:-4783
>> -
>
> nit: removing this whitespace is noise in the diff
Done
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4881
>> + emitYieldPoint(value.get(), JSAsyncGeneratorFunction::AsyncGeneratorSuspendReason::Await);
>
> For readability, how about something like `Register emitAwait(dstRegister, promiseRegister)`? This pattern is repeating a lot and it's more to think about than is desirable maybe
Done
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4906
>> emitIteratorNextWithValue(value.get(), iterator.get(), value.get(), node);
>
> I think the correct thing is:
>
> ```
> // ...
> emitLabel(nextElement.get());
> emitIteratorNextWithValue(value.get(), iterator.get(), value.get(), node);
>
> if (parseMode() == SourceParseMode::AsyncGeneratorBodyMode) {
> // If generatorKind is async, then set innerResult to ? Await(innerResult).
> RefPtr<RegisterID> result = emitYield(value.get(), JSAsyncGeneratorFunction::AsyncGeneratorSuspendReason::Await);
> emitMove(value.get(), result.get());
> }
>
> emitJumpIfTrue(emitGetById(newTemporary(), value.get(), propertyNames().done), loopDone.get());
> emitGetById(value.get(), value.get(), propertyNames().value);
> emitJump(loopStart.get();
> // ...
> ```
>
> I don't think the generator state check or the other stuff is needed for async generators. I believe you're trying to handle the Awaits inside @asyncGeneratorResumeNext(), but AFAICT this separates the logic from the control flow here, which likely will cause bugs.
>
> Instead, I think you should just add Awaits in the places the spec requires you to
I see, I'll try to do this..
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4912
>> + emitEqualityOp(op_stricteq, condition.get(), state.get(), emitLoad(nullptr, jsNumber(static_cast<int32_t>(JSGeneratorFunction::GeneratorState::Completed))));
>
> Is this logic not handled by AsyncGeneratorResumeNext()?
Not sure that I get, what you mean. yield * is handled by this part of code, in asyncGeneratorResume that called from asyncGeneratorResumeNext we set AsyncGeneratorState::Completed that we is checked there. Also I see that I'm using wrong enum name, GeneratorState instead of AsyncGeneratorState
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4914
>> + emitJumpIfTrue(emitGetById(newTemporary(), value.get(), propertyNames().done), loopDone.get());
>
> This looks like it should be loading the "value" property, not "done"
Nope. In current patch it leads to error. We need to check if Iterator that is invoked by yield * is finished, so we are checking done and if it is finished we go to loopDone, later we load value from value register (line 4842:4929 - emitGetById(value.get(), value.get(), propertyNames().value);)
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4930
>> +
>
> Nit: unnecessary linefeed
Done
>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3494
>> +
>
> nit: trailing whitespace sneaking in
Done
>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3497
>> +
>
> nit: trailing whitespace sneaking in
Done
>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3502
>> +
>
> here too
Done
>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3507
>> +
>
> and here
Done
>> Source/JavaScriptCore/dfg/DFGNode.h:594
>> + ASSERT(m_op == NewFunction || m_op == NewGeneratorFunction || m_op == NewAsyncFunction || m_op == NewAsyncGeneratorFunction);
>
> I still would prefer if changes to DFG/FTL could be deferred to a later bug, in order to keep the initial implementation as simple as possible.
Ok. Not sure that I know how to avoid use DFG/B3 part with passing stress tests.
>> Source/JavaScriptCore/runtime/AsyncGeneratorFunctionConstructor.cpp:37
>> +const ClassInfo AsyncGeneratorFunctionConstructor::s_info = { "AsyncGenerator", &Base::s_info, nullptr, CREATE_METHOD_TABLE(AsyncGeneratorFunctionConstructor) };
>
> className should be "AsyncGeneratorFunction", similar to the className for GeneratorFunctionConstructor
Done
>> Source/JavaScriptCore/runtime/FunctionConstructor.cpp:114
>> + prefix = "{async function *";
>
> I don't see a change to functionProtoFuncToString() in FunctionPrototype.cpp, but just FYI this formatting contradicts the test262 tests. We seem to have decided on "async function*" (https://github.com/tc39/test262/commit/da764cafa28ea15b194ac545dc1b67c707c62296), though the proposal doesn't really get into it IIRC.
>
> This comment is just about "you should add the change to functionProtoFuncToString(), with no space between `function` and `*`"
Done
Comment on attachment 311243[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=311243&action=review
Other than that, I'm pretty happy with it
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4969
> + emitEqualityOp(op_stricteq, condition.get(), state.get(), emitLoad(nullptr, jsNumber(static_cast<int32_t>(JSAsyncGeneratorFunction::AsyncGeneratorState::Completed))));
As mentioned on earlier patch, I don't think you need this. `state === Completed` should already never be true at this point, and if it is currently true sometimes, there's something else wrong.
View in context: https://bugs.webkit.org/attachment.cgi?id=307013&action=review>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4914
>>> + emitJumpIfTrue(emitGetById(newTemporary(), value.get(), propertyNames().done), loopDone.get());
>>
>> This looks like it should be loading the "value" property, not "done"
>
> Nope. In current patch it leads to error. We need to check if Iterator that is invoked by yield * is finished, so we are checking done and if it is finished we go to loopDone, later we load value from value register (line 4842:4929 - emitGetById(value.get(), value.get(), propertyNames().value);)
You're right.
On another note, I think the AsyncGeneratorState read is out of place here, because this code should be unreachable if the generator is closed.
>>> Source/JavaScriptCore/dfg/DFGNode.h:594
>>> + ASSERT(m_op == NewFunction || m_op == NewGeneratorFunction || m_op == NewAsyncFunction || m_op == NewAsyncGeneratorFunction);
>>
>> I still would prefer if changes to DFG/FTL could be deferred to a later bug, in order to keep the initial implementation as simple as possible.
>
> Ok. Not sure that I know how to avoid use DFG/B3 part with passing stress tests.
You can limit failing tests to not run with optimizing compilers, using some combination of run-flags. Probably `//@ runNoJIT` and maybe a few other ones would be good too.
Created attachment 311255[details]
Archive of layout-test-results from ews103 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 311256[details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 311262[details]
Archive of layout-test-results from ews124 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Created attachment 311263[details]
Archive of layout-test-results from ews115 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
(In reply to Saam Barati from comment #73)
> Does this need to be reviewed or is the review here stale?
Currently, I'm fixing comments from caitp and failing tests. So I think it will be better to review next patch.
Comment on attachment 311243[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=311243&action=review> Source/JavaScriptCore/parser/Parser.cpp:2731
> + parseMode = SourceParseMode::GeneratorWrapperFunctionMode;
This would need to be SourceParseMode::GeneratorWrapperMethodMode (see assertion failure in parser-syntax-check)
> JSTests/stress/generator-class-methods-syntax.js:30
> +`, `SyntaxError: Cannot declare a generator function named 'constructor'.`);
Why change this? the error message from Parser.cpp line 2811 is unchanged.
Comment on attachment 311243[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=311243&action=review
I've prototyped suggested changes to the parser (diff is at https://www.diffchecker.com/lB9TEMZ6). I didn't get into the object literal parse method, but I'll look at that a bit tomorrow to see if it can be cleaned up a bit.
> Source/JavaScriptCore/parser/Parser.cpp:-2643
> - isAsync = !isGenerator && !isAsyncMethod;
I think we can clean this up a lot.
Something like this:
```
case ASYNC:
if (!isGeneratorMethodParseMode(parseMode) && !isAsyncMethodParseMode(parseMode)) {
ident = m_token.m_data.ident;
next();
if (match(OPENPAREN) || match(COLON) || match(EQUAL) || m_lexer->prevTerminator())
break; <-- If we get here, we can just treat this as an identifier, and avoid doing the getter/setter checks which are guaranteed to be false
isAsync = true; << maybe unneeded, or specific to non-generators?
if (UNLIKELY(consume(TIMES))) {
isAsyncGenerator = true;
parseMode = SourceParseMode::AsyncGeneratorWrapperMethodMode;
} else {
parseMode = SourceParseMode::AsyncMethodMode;
}
goto parseMethod;
}
FALLTHROUGH; <-- In the fall through case, just treat ASYNC as an identifier.
```
This allows deleting some code from the more common IDENT path, and I think it allows deleting the `wasAsync` state, etc. Can probably get rid of `isAsync` and `isAsyncGenerator` as well.
> Source/JavaScriptCore/parser/Parser.cpp:2763
> + if (!isGeneratorMethodParseMode(parseMode) && !isAsyncMethodParseMode(parseMode) && !isAsyncGeneratorMethodParseMode(parseMode) && (matchIdentifierOrKeyword() || match(STRING) || match(DOUBLE) || match(INTEGER) || match(OPENBRACKET))) {
Instead of all of these `!isGeneratorMethodPArseMode(parseMode) && !isAsyncMethod...`, how about just `if (parseMode == SourceParseMode::MethodMode && ...`? Easier to read maybe
> Source/JavaScriptCore/parser/Parser.cpp:2768
> + parseMode = (UNLIKELY(isAsyncGenerator))
if my comments above are followed, I believe we can get rid of this whole branch
> Source/JavaScriptCore/parser/Parser.cpp:2805
> parseMode = SourceParseMode::AsyncMethodMode;
this overwrites the parse mode of an async generator, which will probably cause problems when lazily compiled code is reparsed.
> Source/JavaScriptCore/parser/Parser.cpp:3806
> + parseMode = SourceParseMode::GeneratorWrapperFunctionMode;
GeneratorWrapperMethodMode, like in parseClass()
> Source/JavaScriptCore/parser/Parser.cpp:3812
> + mightBeTimes = true;
I think this is slightly broken logic:
isAsync might end up being false, e.g. in the case of `*async() {}`. I think this code will still set isAsyncGenerator = true if you do `*async* foo() {}`, though I'm not totally positive. A regression test would be good here.
Is there any way we could make the parsing model be a bit closer to what I suggested for parseClass()? I'm pretty happy with the design I proposed up there, it's much cleaner than what I originally checked in IMO.
> Source/JavaScriptCore/parser/Parser.cpp:3833
> + mightBeTimes = false;
If we do end up needing `mightBeTimes` (can't handle this in `case ASYNC` as suggested before), maybe we could name it something clearer, like `maybeAsyncGenerator` or something.
> Source/JavaScriptCore/parser/Parser.cpp:3873
> + parseMode = SourceParseMode::AsyncGeneratorWrapperMethodMode;
we can delete these branches if we handle this in the token switch under `case ASYNC:` as suggested in parseClass()
> Source/JavaScriptCore/parser/Parser.cpp:3930
> + ASSERT(parseMode == SourceParseMode::MethodMode || parseMode == SourceParseMode::GeneratorWrapperFunctionMode || parseMode == SourceParseMode::AsyncMethodMode || parseMode == SourceParseMode::AsyncGeneratorWrapperMethodMode);
How about `ASSERT(isMethodParseMode(parseMode));`? Again, this line uses GeneratorWrapperFunctionMode rather than MethodMode
> Source/JavaScriptCore/parser/ParserModes.h:172
> + SourceParseMode::GeneratorWrapperFunctionMode).contains(parseMode);
s/Function/Method --- But do we really need this in place of just `parseMode == SourceParseMode::GeneratorWrapperMethodMode` ?
> Source/JavaScriptCore/parser/ParserModes.h:182
> + return SourceParseModeSet(SourceParseMode::AsyncGeneratorWrapperFunctionMode).contains(parseMode);
s/Function/Method/ --- same comment as before, though.
(In reply to Caitlin Potter (:caitp) from comment #76)
> Comment on attachment 311243[details]
> Patch
>
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=311243&action=review
>
> I've prototyped suggested changes to the parser (diff is at
> https://www.diffchecker.com/lB9TEMZ6). I didn't get into the object literal
> parse method, but I'll look at that a bit tomorrow to see if it can be
> cleaned up a bit.
>
> > Source/JavaScriptCore/parser/Parser.cpp:-2643
> > - isAsync = !isGenerator && !isAsyncMethod;
>
> I think we can clean this up a lot.
>
> Something like this:
>
> ```
> case ASYNC:
> if (!isGeneratorMethodParseMode(parseMode) &&
> !isAsyncMethodParseMode(parseMode)) {
> ident = m_token.m_data.ident;
> next();
> if (match(OPENPAREN) || match(COLON) || match(EQUAL) ||
> m_lexer->prevTerminator())
> break; <-- If we get here, we can just treat this as
> an identifier, and avoid doing the getter/setter checks which are guaranteed
> to be false
>
> isAsync = true; << maybe unneeded, or specific to
> non-generators?
> if (UNLIKELY(consume(TIMES))) {
> isAsyncGenerator = true;
> parseMode =
> SourceParseMode::AsyncGeneratorWrapperMethodMode;
> } else {
> parseMode = SourceParseMode::AsyncMethodMode;
> }
> goto parseMethod;
> }
> FALLTHROUGH; <-- In the fall through case, just treat ASYNC as
> an identifier.
> ```
>
> This allows deleting some code from the more common IDENT path, and I think
> it allows deleting the `wasAsync` state, etc. Can probably get rid of
> `isAsync` and `isAsyncGenerator` as well.
>
> > Source/JavaScriptCore/parser/Parser.cpp:2763
> > + if (!isGeneratorMethodParseMode(parseMode) && !isAsyncMethodParseMode(parseMode) && !isAsyncGeneratorMethodParseMode(parseMode) && (matchIdentifierOrKeyword() || match(STRING) || match(DOUBLE) || match(INTEGER) || match(OPENBRACKET))) {
>
> Instead of all of these `!isGeneratorMethodPArseMode(parseMode) &&
> !isAsyncMethod...`, how about just `if (parseMode ==
> SourceParseMode::MethodMode && ...`? Easier to read maybe
>
> > Source/JavaScriptCore/parser/Parser.cpp:2768
> > + parseMode = (UNLIKELY(isAsyncGenerator))
>
> if my comments above are followed, I believe we can get rid of this whole
> branch
>
> > Source/JavaScriptCore/parser/Parser.cpp:2805
> > parseMode = SourceParseMode::AsyncMethodMode;
>
> this overwrites the parse mode of an async generator, which will probably
> cause problems when lazily compiled code is reparsed.
>
> > Source/JavaScriptCore/parser/Parser.cpp:3806
> > + parseMode = SourceParseMode::GeneratorWrapperFunctionMode;
>
> GeneratorWrapperMethodMode, like in parseClass()
>
> > Source/JavaScriptCore/parser/Parser.cpp:3812
> > + mightBeTimes = true;
>
> I think this is slightly broken logic:
>
> isAsync might end up being false, e.g. in the case of `*async() {}`. I think
> this code will still set isAsyncGenerator = true if you do `*async* foo()
> {}`, though I'm not totally positive. A regression test would be good here.
>
> Is there any way we could make the parsing model be a bit closer to what I
> suggested for parseClass()? I'm pretty happy with the design I proposed up
> there, it's much cleaner than what I originally checked in IMO.
>
> > Source/JavaScriptCore/parser/Parser.cpp:3833
> > + mightBeTimes = false;
>
> If we do end up needing `mightBeTimes` (can't handle this in `case ASYNC` as
> suggested before), maybe we could name it something clearer, like
> `maybeAsyncGenerator` or something.
>
> > Source/JavaScriptCore/parser/Parser.cpp:3873
> > + parseMode = SourceParseMode::AsyncGeneratorWrapperMethodMode;
>
> we can delete these branches if we handle this in the token switch under
> `case ASYNC:` as suggested in parseClass()
>
> > Source/JavaScriptCore/parser/Parser.cpp:3930
> > + ASSERT(parseMode == SourceParseMode::MethodMode || parseMode == SourceParseMode::GeneratorWrapperFunctionMode || parseMode == SourceParseMode::AsyncMethodMode || parseMode == SourceParseMode::AsyncGeneratorWrapperMethodMode);
>
> How about `ASSERT(isMethodParseMode(parseMode));`? Again, this line uses
> GeneratorWrapperFunctionMode rather than MethodMode
>
> > Source/JavaScriptCore/parser/ParserModes.h:172
> > + SourceParseMode::GeneratorWrapperFunctionMode).contains(parseMode);
>
> s/Function/Method --- But do we really need this in place of just `parseMode
> == SourceParseMode::GeneratorWrapperMethodMode` ?
>
> > Source/JavaScriptCore/parser/ParserModes.h:182
> > + return SourceParseModeSet(SourceParseMode::AsyncGeneratorWrapperFunctionMode).contains(parseMode);
>
> s/Function/Method/ --- same comment as before, though.
Thanks for review.
I've mostly fixed all your comments, I've back to recursion call of the asyncGeneratorResumeNext, to be more close to spec. I will back to loop after implementation of new version of spec.
Also I decided to left DFG/FTL part in patch, it is not so big change.
Created attachment 312349[details]
Archive of layout-test-results from ews101 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 312350[details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 312352[details]
Archive of layout-test-results from ews117 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 312356[details]
Archive of layout-test-results from ews126 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Comment on attachment 312340[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=312340&action=review
Just a few more points about the parser, but otherwise the changes look really nice. I'll take a look at the runtime again once Domenic uploads the rest of the spec change
> Source/JavaScriptCore/parser/Parser.cpp:3826
> + failIfTrue(m_lexer->prevTerminator(), "Expected a property name following keyword 'async'");
I am not sure this is the best way to go here.
Example:
`({
async
: 1
})`
I think this line would make that an error, but it should be legal, right?
> Source/JavaScriptCore/parser/Parser.cpp:3840
> + parseMode = parseMode = SourceParseMode::AsyncMethodMode;
please remove the duplicate `parseMode = `
> Source/JavaScriptCore/parser/Parser.cpp:3851
> + if (UNLIKELY(!isIdentSet)) {
I'm not sure I see the point in the `isIdentSet` thing --- it seems to add complexity without necessarily being worthwhile. I'm happy to be convinced otherwise if it turns out to shave off parse time.
Comment on attachment 312340[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=312340&action=review> Source/JavaScriptCore/parser/Parser.cpp:3828
> + goto parseProperty;
Won't this most likely end up on the default case below (particularly for COLON and EQUAL) and report an error? That doesn't seem right. Let me take a look at this...
Yeah, just tried this in the repl:
```
>>> ({
... async
... : 1 })
Unexpected token ':'. Expected a property name following keyword 'async'.:3
>>> ({ async = 1 })
Unexpected token '='. Expected a property name.:1
>>> ({ async: 1 })
Unexpected token ':'. Expected a property name.:1
```
there are some problems here. Let me see if I can sort this out and I'll send you some suggestions
Comment on attachment 312340[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=312340&action=review> Source/JavaScriptCore/parser/Parser.cpp:3819
> + if (!isGeneratorMethodParseMode(parseMode) && !isAsyncMethodParseMode(parseMode)) {
Here's what I came up with for the ASYNC case:
```
case ASYNC:
if (parseMode == SourceParseMode::MethodMode) {
SavePoint savePoint = createSavePoint();
next();
if (match(COLON) || match(OPENPAREN) || match(COMMA) || match(CLOSEBRACE)) {
restoreSavePoint(savePoint);
wasIdent = true;
goto namedProperty;
}
failIfTrue(m_lexer->prevTerminator(), "Expected a property name following keyword 'async'");
isAsync = true;
if (UNLIKELY(consume(TIMES))) {
isAsyncGenerator = true;
parseMode = SourceParseMode::AsyncGeneratorWrapperMethodMode;
} else
parseMode = parseMode = SourceParseMode::AsyncMethodMode;
goto parseProperty;
}
FALLTHROUGH;
```
So, this would produce a bit of a hiccup for properties named "async" because of the save state creation/restoration, but it's probably not going to show up on any benchmarks, and the cost should be pretty minor.
I've removed the `match(EQUAL)` thing, because shorthand property names can't have initializers, and I've re-arranged the prevTerminator check to get a better error message for the following case:
```
({ async
foo() {} })
```
I think this should fixup the parser issues from the last patch.
Comment on attachment 312340[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=312340&action=review
I've had a quick look, left a few comments. Looking forward to seeing this land in JSC.
> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:29
> + if (typeof syncIterator !== 'object') new @TypeError('Only objects can be wrapped by async-from-sync wrapper');
Please use the intrinsic `@isObject(syncIterator)`. `typeof syncIterator !== 'object'` does not correspond to the spec clause
> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:32
> + asyncIterator.@syncIterator = syncIterator;
If the syncIterator is a parameter to the constructor, we can take advantage of TCO. Might be a good idea?
> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:40
> + this.next = @next;
I don't understand these 3 lines. Why not just use the prototype object for this instead? It shouldn't be observable either way AFAIK, so if this is faster in JSC I suppose it's fine.
> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:55
> + if (typeof O !== 'object' || typeof O.@syncIterator === 'undefined') {
So, it's possible to get at these functions `(next, throw, return)` using function.caller, that's the only way this could really happen, though. In either case, the shape of the async-from-sync iterator object should always be immutable, so it's probably cheaper to implement something which simply checks the shape of the object rather than performing the named lookup twice (I certainly don't think that's a blocker, and implementing a new type-checking intrinsic may be more trouble than it's worth).
Also again, `typeof O !== 'object'` is not enough to match the spec clause. A test cause you could use to verify this:
```js
function customIterator() {}
let value = 0;
customIterator.next = function() {
let done = ++i > 5;
return { value, done };
}
async function f() {
let sum = 0;
for await (let x of customIterator) {
console.log(x);
sum += x;
}
}
f().then(
function(sum) {
assert(sum === 15);
},
function(error) {
fail(error); // will fail because of the rejection on line line 56
});
```
I'm sure this has shown up in other places in this patch, I'll try to mark them down so they're easy to find and fix.
> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:62
> + const { done, value } = nextResult;
IteratorComplete() needs to convert `done` to a boolean value. This step could be deferred until the resolve call on line 66. This is observable due to the function.caller thing mentioned above.
> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:83
> + if (typeof O !== 'object' || typeof O.@syncIterator === 'undefined') {
Also `@isObject(O)` rather than typeof here.
> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:126
> + if (typeof O !== 'object' || typeof O.@syncIterator === 'undefined') {
Also `@isObject(O)` rather than typeof here.
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:74
> + const valueWrapperCapability = @newPromiseCapability(@Promise);
In https://github.com/tc39/proposal-async-iteration/pull/102/, AsyncGeneratorResolve no longer performs value unwrapping
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:92
> + @asyncGeneratorResolve(generator, value, false);
Per https://github.com/tc39/proposal-async-iteration/pull/102, AsyncGeneratorYield now needs to perform `Set _value_ to ? Await(_value_).`.
I think you'll need to insert Awaits in the parser/bytecode-generator, I'll leave notes where I think is appropriate. You can probably keep this helper as it is, though.
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:142
> + @awaitValue(generator, value, onFulfilled);
This doesn't make a lot of sense to me. It looks like if any awaited Promise is rejected, the generator is immediately closed/aborted, even if there's a try/catch or try/finally around the await. Is this not true?
```js
async function* g() {
try {
await Promise.reject("doop");
} finally {
yield* [1, 2, 3]; // Is this line reachable in this implementation?
}
}
```
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:182
> + return @asyncGeneratorResolve(generator, next.value, true);https://github.com/tc39/proposal-async-iteration/pull/102/ stuff: you need to do Promise unwrapping with some new closures (similar to the Async-from-Sync Iterator ones) which do AsyncGeneratorResolve / AsyncGeneratorReject respectively.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4806
> + emitYieldPoint(argument, result);
Per https://github.com/tc39/proposal-async-iteration/pull/102, there's an implicit `await` for async generator yields. You'll need to await the argument here (at least for normal yields)
Also new in that spec change, the `.return` handler needs to await the sent value before continuing.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4852
> + const Identifier& propertyName = parseMode() == SourceParseMode::AsyncGeneratorBodyMode ? propertyNames().asyncIteratorSymbol : propertyNames().iteratorSymbol;
I like this helper --- It's probably a good idea to either leave a comment indicating that this will always pick the iterator type based on function type, or else to make it possible to specify the iterator type using something like the hint parameter in the spec method. This would allow it to be used by other GetIterator callers.
Comment on attachment 312340[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=312340&action=review>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:55
>> + if (typeof O !== 'object' || typeof O.@syncIterator === 'undefined') {
>
> So, it's possible to get at these functions `(next, throw, return)` using function.caller, that's the only way this could really happen, though. In either case, the shape of the async-from-sync iterator object should always be immutable, so it's probably cheaper to implement something which simply checks the shape of the object rather than performing the named lookup twice (I certainly don't think that's a blocker, and implementing a new type-checking intrinsic may be more trouble than it's worth).
>
> Also again, `typeof O !== 'object'` is not enough to match the spec clause. A test cause you could use to verify this:
>
> ```js
> function customIterator() {}
> let value = 0;
> customIterator.next = function() {
> let done = ++i > 5;
> return { value, done };
> }
>
> async function f() {
> let sum = 0;
> for await (let x of customIterator) {
> console.log(x);
> sum += x;
> }
> }
>
> f().then(
> function(sum) {
> assert(sum === 15);
> },
> function(error) {
> fail(error); // will fail because of the rejection on line line 56
> });
> ```
>
> I'm sure this has shown up in other places in this patch, I'll try to mark them down so they're easy to find and fix.
I may have been mistaken about it being observable through function.caller (though it is in v8, for now). ishell@ had pointed out the annex B forbidden extensions clause which presumably prevents the builtin Async-from-Sync Iterator methods from being returns to the caller.
> If an implementation extends non-strict or built-in function objects with an own property named "caller" the value of that property, as observed using [[Get]] or [[GetOwnProperty]], must not be a strict function object. If it is an accessor property, the function that is the value of the property's [[Get]] attribute must never return a strict function when called.
The other part of that above comment still applies though. I think not being returnable via Function.caller makes the type checks in the Async-from-Sync Iterator methods redundant, they could be assertions instead.
Comment on attachment 313999[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=313999&action=review
Coming along nicely, great work so far.
> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:35
> + promiseCapability.@reject.@call(@undefined, new @TypeError('syncIterator property is missing.'));
Is this still reachable? As discussed previously, I don't think these methods are accessible by users (not through function.caller anyways), so it should be impossible for the receiver to have the wrong type.
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:113
> + @awaitValue(generator, value, asyncGeneratorYieldAwaited);
You need to resume the generator with a throw completion if this await fails, like you do for non-yield awaits. Otherwise catch/finally handlers aren't invoked. This is different from how it was when AsyncGeneratorResolve did the unwrapping itself, rather than using Await. The default "AsyncGeneratorReject" call is really only needed for uncaught exceptions.
```
async function* g() {
try {
yield Promise.reject("donuts");
assertUnreachable();
} catch (e) {
assert(e === "donuts");
yield "churros";
}
}
let it = g();
it.next()
.then(x => assert(x.value === "churros"), e => assertUnreachable())
.finally(() => {
it.next()
.then(x => assert(x.value === undefined), assert(x.done === true), e => assertUnreachable());
});
```
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:128
> + @asyncGeneratorReject(generator, error);
above comment reflects this
> Source/JavaScriptCore/parser/Parser.cpp:2733
> + bool wasAsync = false;
wasAsync still not used
> Source/JavaScriptCore/parser/Parser.cpp:2760
> + isAsyncGenerator = true;
`isAsyncGenerator` still isn't actually used by anything, AFAICT.`isAsync` is no longer used.
> Source/JavaScriptCore/parser/Parser.cpp:3807
> + bool isAsyncGenerator = false;
isAsyncGenerator and isAsync still not used
Comment on attachment 314046[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=314046&action=review> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:216
> + return @asyncGeneratorResolve(generator, next.value, true);
You need to do the unwrapping from lines 205-208 for this line too (https://tc39.github.io/proposal-async-iteration/#sec-asyncgeneratorresumenext steps 10.a and 10.b) --- You can probbably combine them similar to how it's done in the spec
Test case:
```js
async function* g() { yield 1; }
let it = g();
it.next();
it.next(); // state is now @AsyncGeneratorStateCompleted
it.return(Promise.resolve("Should be a string!")).then(
function({ value }) {
print(value === "Should be a string!"); // Should be true, but is false due to Promise not being unwrapped
});
```
> Source/JavaScriptCore/parser/Parser.cpp:3905
> + if (complete || (wasIdent && !isGeneratorMethodParseMode(parseMode) && (*ident == m_vm->propertyNames->get || *ident == m_vm->propertyNames->set)) || isAsync)
You don't need `isAsync` here because the name following the `async` keyword was already lexed on line 3877. You can acceptably use the else-clause in this case.
Unrelated, but could someone explain why we use `nextExpectIdentifier()` in the else clause, since in non-error cases it will always end up on the slow path anyways?
Comment on attachment 314046[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=314046&action=review>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:216
>> + return @asyncGeneratorResolve(generator, next.value, true);
>
> You need to do the unwrapping from lines 205-208 for this line too (https://tc39.github.io/proposal-async-iteration/#sec-asyncgeneratorresumenext steps 10.a and 10.b) --- You can probbably combine them similar to how it's done in the spec
>
> Test case:
>
> ```js
> async function* g() { yield 1; }
> let it = g();
> it.next();
> it.next(); // state is now @AsyncGeneratorStateCompleted
> it.return(Promise.resolve("Should be a string!")).then(
> function({ value }) {
> print(value === "Should be a string!"); // Should be true, but is false due to Promise not being unwrapped
> });
> ```
Ohh, I missed that. Fixed in new patch.
>> Source/JavaScriptCore/parser/Parser.cpp:3905
>> + if (complete || (wasIdent && !isGeneratorMethodParseMode(parseMode) && (*ident == m_vm->propertyNames->get || *ident == m_vm->propertyNames->set)) || isAsync)
>
> You don't need `isAsync` here because the name following the `async` keyword was already lexed on line 3877. You can acceptably use the else-clause in this case.
>
> Unrelated, but could someone explain why we use `nextExpectIdentifier()` in the else clause, since in non-error cases it will always end up on the slow path anyways?
Fixed
Comment on attachment 314159[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=314159&action=review
Haven't looked at all of the changes, but here's a few more comments about yield* in particular, since it's fresh in my mind from v8 work.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4849
> + if (parseMode() == SourceParseMode::AsyncGeneratorBodyMode) {
I don't think this is an acceptable way to do this. The decision making about whether to actually perform an await does not belong inside this function, but in the caller function. Also, the way this is written, it's not useful for any other situation where you'd want to emit an Await.
Can we refactor this to be more like this:
```
switch (parseMode()) {
case SourceParseMode::AsyncGeneratorBodyMode:
// Do the stuff...
case SourceParseMode::AsyncFunctionBodyMode:
// Do the stuff...
case SourceParseMode::AsyncArrowFunctionBodyMode:
// Do the stuff...
default:
RELEASE_ASSERT_NOT_REACHED();
return nullptr;
}
```
and thus defer the decision making to the caller?
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4870
> + emitJumpIfFalse(emitIsUndefined(newTemporary(), iterator.get()), asyncIteratorFound.get());
You need to also jump if the iterator method is `null` (The GetMethod() operation from `Set method to ? GetMethod(obj, @@asyncIterator).` converts "null" to undefined).
Additionally, it looks like `op_is_undefined` takes the masqueradesAsUndefined flag into account, which I think would lead to incorrect behaviour if you did something like `for await (let x of { [Symbol.asyncIterator]: document.all }) {}`. Kind of a weird case. It should try to call document.all as a function, but it will try to load the sync iterator instead.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4910
> + emitYieldPoint(value.get(), JSAsyncGeneratorFunction::AsyncGeneratorSuspendReason::Yield);
Per spec: `If generatorKind is async, then let received be AsyncGeneratorYield(? IteratorValue(innerResult)).`
So, when we do the yield, we need to await the value component first
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4913
> Ref<Label> returnLabel = newLabel();
Some comments (can't add them inline :( since your changes don't affect those lines)
for Throw resumption: If the "throw" method is not found, you need to perform AsyncIteratorClose(), which Awaits the result of the .return method, preventing execution from continuing until async cleanup has been completed. You also need to await the result of the "throw" method if it is present, before checking .done etc.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4967
> + emitAwait(value.get());
This Await needs to happen before testing if innerResult is an object or not.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4971
>
Again, the `emitGetById(value.get(), value.get(), propertyNames().value);` is only technically correct for async generators. For sync generators, supposed to yield the same iterator result returned from the .return() call.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4990
> emitIteratorNextWithValue(value.get(), iterator.get(), value.get(), node);
emitIteratorNextWithValue() does its own "iterator result is not an object" exception before the value has been awaited, which is incorrect for async generators. Maybe need to make that function conditionally await the return value (but please don't have it check parseMode() itself).
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4995
> + Ref<Label> iteratorValueIsObject = newLabel();
Per the above comment, we can remove this block, it will be redundant once the await is added in the appropriate place
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:5001
> emitGetById(value.get(), value.get(), propertyNames().value);
This is correct for async generators, but it seems like it was incorrect for sync generators, which should have yielded the exact iterator result returned from whichever iterator method.
No need to worry about it in this patch, but this is clearly a bug, worth filing:
```
var O = { [Symbol.iterator]() { return this; }, next() { return { floof: 1, done: false, value: 1 }; } };
function* g() { yield* O; }
let it = g();
it.next(); // { done: false, value: 1; } --- should be { floof: 1, done: false, value: 1 }, same object returned from .next();
```
Comment on attachment 314159[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=314159&action=review
Thanks for review. I've most comments
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4849
>> + if (parseMode() == SourceParseMode::AsyncGeneratorBodyMode) {
>
> I don't think this is an acceptable way to do this. The decision making about whether to actually perform an await does not belong inside this function, but in the caller function. Also, the way this is written, it's not useful for any other situation where you'd want to emit an Await.
>
> Can we refactor this to be more like this:
>
> ```
> switch (parseMode()) {
> case SourceParseMode::AsyncGeneratorBodyMode:
> // Do the stuff...
> case SourceParseMode::AsyncFunctionBodyMode:
> // Do the stuff...
> case SourceParseMode::AsyncArrowFunctionBodyMode:
> // Do the stuff...
> default:
> RELEASE_ASSERT_NOT_REACHED();
> return nullptr;
> }
> ```
>
> and thus defer the decision making to the caller?
done
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4870
>> + emitJumpIfFalse(emitIsUndefined(newTemporary(), iterator.get()), asyncIteratorFound.get());
>
> You need to also jump if the iterator method is `null` (The GetMethod() operation from `Set method to ? GetMethod(obj, @@asyncIterator).` converts "null" to undefined).
>
> Additionally, it looks like `op_is_undefined` takes the masqueradesAsUndefined flag into account, which I think would lead to incorrect behaviour if you did something like `for await (let x of { [Symbol.asyncIterator]: document.all }) {}`. Kind of a weird case. It should try to call document.all as a function, but it will try to load the sync iterator instead.
I've add one more condition with checking for `null`
Hmm, I'll build webkit and look to the document.all function
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4910
>> + emitYieldPoint(value.get(), JSAsyncGeneratorFunction::AsyncGeneratorSuspendReason::Yield);
>
> Per spec: `If generatorKind is async, then let received be AsyncGeneratorYield(? IteratorValue(innerResult)).`
>
> So, when we do the yield, we need to await the value component first
It is start We do AsyncGeneratorYield in each point.
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4913
>> Ref<Label> returnLabel = newLabel();
>
> Some comments (can't add them inline :( since your changes don't affect those lines)
>
> for Throw resumption: If the "throw" method is not found, you need to perform AsyncIteratorClose(), which Awaits the result of the .return method, preventing execution from continuing until async cleanup has been completed. You also need to await the result of the "throw" method if it is present, before checking .done etc.
Yeah, it does by emitIteratorClose() method. Await for throw is done after branchOnResult label
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4967
>> + emitAwait(value.get());
>
> This Await needs to happen before testing if innerResult is an object or not.
Done
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4990
>> emitIteratorNextWithValue(value.get(), iterator.get(), value.get(), node);
>
> emitIteratorNextWithValue() does its own "iterator result is not an object" exception before the value has been awaited, which is incorrect for async generators. Maybe need to make that function conditionally await the return value (but please don't have it check parseMode() itself).
Done
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4995
>> + Ref<Label> iteratorValueIsObject = newLabel();
>
> Per the above comment, we can remove this block, it will be redundant once the await is added in the appropriate place
I've just remove checking isObject from emitIteratorNextWithValue. We reuse this block in `throw` case as well.
>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:5001
>> emitGetById(value.get(), value.get(), propertyNames().value);
>
> This is correct for async generators, but it seems like it was incorrect for sync generators, which should have yielded the exact iterator result returned from whichever iterator method.
>
> No need to worry about it in this patch, but this is clearly a bug, worth filing:
>
> ```
> var O = { [Symbol.iterator]() { return this; }, next() { return { floof: 1, done: false, value: 1 }; } };
> function* g() { yield* O; }
> let it = g();
> it.next(); // { done: false, value: 1; } --- should be { floof: 1, done: false, value: 1 }, same object returned from .next();
> ```
Bug is created
Created attachment 316213[details]
Archive of layout-test-results from ews103 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 316215[details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 316217[details]
Archive of layout-test-results from ews115 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 316220[details]
Archive of layout-test-results from ews124 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Comment on attachment 316237[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=316237&action=review
Few more comments --- Please take another look at the earlier runtime comments I had provided, because I think there may be some duplicates here :)
> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:29
> + if (typeof syncIterator !== 'object') new @TypeError('Only objects can be wrapped by async-from-sync wrapper');
Use `@isObject()` to get the proper `Type(O) is Object` behaviour. `typeof null` is "object", after all.
Also, shouldn't this be `@throwTypeError` instead of `new @TypeError`?
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:73
> + const { promiseCapability } = next;
AsyncGeneratorResolve no longer does this unwrapping (https://tc39.github.io/proposal-async-iteration/#sec-asyncgeneratorresolve). I think I left this as a comment on a previous patch.
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:91
> + generator.@asyncGeneratorSuspendReason = @AsyncGeneratorSuspendReasonYield;
So, I see that you're trying to do the `await` in AsyncGeneratorResolve, but there's a problem: the resolve and reject closures won't do exactly what's needed.
The `@asyncGeneratorResumeNext(generator)` call on line 81 doesn't make sense, because the generator is effectively suspended for an Await (so it won't/shouldn't do the resume). Instead, the ResumeNext() needs to happen in the promise resolve/reject closures.
A good way to model it:
```
function AsyncGeneratorYield(generator, value) {
generator.@asyncGeneratorSuspendReason = @AsyncGeneratorYield;
const onFulfilled = function(value) {
@asyncGeneratorResolve(generator, value, false);
return @asyncGeneratorResumeNext(generator);
}
@awaitValue(generator, value, onFulfilled);
}
```
This will resume the generator correctly after the Awaits. It's possible the algorithm may need a slight adjustment, but I believe it's correct.
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:105
> + @asyncGeneratorReject(generator, error);
this should be
```js
const onRejected = function(error) {
@doAsyncGeneratorBodyCall(generator, error, @GeneratorResumeModeThrow);
return @asyncGeneratorResumeNext(generator);
}
```
this should allow catch handlers to fire if the `await` fails.
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:182
> + return @asyncGeneratorResolve(generator, next.value, true);
we need to `await` the sent value before performing @asyncGeneratorResolve --- in this case, if the `await` succeeds or fails, there's no need to perform `@doAsyncGeneratorBodyCall()` --- unlike other cases where it is needed. For this reason, the promise unwrapping can't happen within @asyncGeneratorResolve.
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:198
> +function isExectionState(generator)
this looks like a typo. `isExecutingState` maybe? `isExecutionState`?
Comment on attachment 316237[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=316237&action=review>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:29
>> + if (typeof syncIterator !== 'object') new @TypeError('Only objects can be wrapped by async-from-sync wrapper');
>
> Use `@isObject()` to get the proper `Type(O) is Object` behaviour. `typeof null` is "object", after all.
>
> Also, shouldn't this be `@throwTypeError` instead of `new @TypeError`?
OMG! I've applied last fixed to outdate patch. (I was after vacation :-( )
>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:73
>> + const { promiseCapability } = next;
>
> AsyncGeneratorResolve no longer does this unwrapping (https://tc39.github.io/proposal-async-iteration/#sec-asyncgeneratorresolve). I think I left this as a comment on a previous patch.
Fixed, by using correct patch
>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:91
>> + generator.@asyncGeneratorSuspendReason = @AsyncGeneratorSuspendReasonYield;
>
> So, I see that you're trying to do the `await` in AsyncGeneratorResolve, but there's a problem: the resolve and reject closures won't do exactly what's needed.
>
> The `@asyncGeneratorResumeNext(generator)` call on line 81 doesn't make sense, because the generator is effectively suspended for an Await (so it won't/shouldn't do the resume). Instead, the ResumeNext() needs to happen in the promise resolve/reject closures.
>
> A good way to model it:
>
> ```
> function AsyncGeneratorYield(generator, value) {
> generator.@asyncGeneratorSuspendReason = @AsyncGeneratorYield;
>
> const onFulfilled = function(value) {
> @asyncGeneratorResolve(generator, value, false);
>
> return @asyncGeneratorResumeNext(generator);
> }
> @awaitValue(generator, value, onFulfilled);
> }
>
> ```
>
> This will resume the generator correctly after the Awaits. It's possible the algorithm may need a slight adjustment, but I believe it's correct.
Yeah, you are right, could you please revalidate if it correct in the latest patch.
>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:105
>> + @asyncGeneratorReject(generator, error);
>
> this should be
>
> ```js
> const onRejected = function(error) {
> @doAsyncGeneratorBodyCall(generator, error, @GeneratorResumeModeThrow);
>
> return @asyncGeneratorResumeNext(generator);
> }
> ```
>
> this should allow catch handlers to fire if the `await` fails.
Fixed in new patch.
>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:182
>> + return @asyncGeneratorResolve(generator, next.value, true);
>
> we need to `await` the sent value before performing @asyncGeneratorResolve --- in this case, if the `await` succeeds or fails, there's no need to perform `@doAsyncGeneratorBodyCall()` --- unlike other cases where it is needed. For this reason, the promise unwrapping can't happen within @asyncGeneratorResolve.
Rewritted in new patch.
>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:198
>> +function isExectionState(generator)
>
> this looks like a typo. `isExecutingState` maybe? `isExecutionState`?
Fixed
Comment on attachment 316312[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=316312&action=review
OK --- a few more comments, I think this could start using a look from Yusuke and Saam, though.
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:36
> +function asyncGeneratorGetFirstItemFromQueue(generator)
nit: rename `Get` to `Take` / `Remove`, to be clear about intent here (the queue no longer owns/references the request)
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:95
> + @asyncGeneratorResumeNext(generator);
Good opportunity for a tail-call here
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:111
> + generator.@asyncGeneratorSuspendReason = @AsyncGeneratorSuspendReasonNone;
Shouldn't this be @AsyncGeneratorSuspendReasonAwait? The WebInspector might eventually care about the distinction
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:174
> + @awaitValue(generator, value, onFulfilled);
In this case, you need a different reject handler which does not try to re-enter the generator body
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:219
> + @asyncGeneratorReject(generator, next.value);
since @asyncGeneratorReject recurses into asyncGeneratorResumeNext, it's a good opportunity for a tail-call (both here and in @asyncGeneratorReject)
Arg, I left a few more comments in BytecodeGenerator.cpp, but they didn't seem to get submitted! I'll try to summarize here (at least the ones which are still in memory):
1) in emitIteratorClose, don't use the function parseMode to determine whether to perform AsyncIteratorClose or not. Ordinary for-of loops (for example) should not await the result of the .return method.
2) Don't do the `await` in `doAsyncGeneratorBodyCall` after resuming, when the state becomes completed. The spec only asks you to `await` the operand of ReturnStatements with an explicit operand (https://tc39.github.io/proposal-async-iteration/#sec-return-statement). The extra Await is technically observable, so it's better if implementations match up. Instead, this should be handled in BytecodeGenerator.
Comment on attachment 316312[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=316312&action=review>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:36
>> +function asyncGeneratorGetFirstItemFromQueue(generator)
>
> nit: rename `Get` to `Take` / `Remove`, to be clear about intent here (the queue no longer owns/references the request)
Renamed to asyncGeneratorDeueue
>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:95
>> + @asyncGeneratorResumeNext(generator);
>
> Good opportunity for a tail-call here
Done
>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:111
>> + generator.@asyncGeneratorSuspendReason = @AsyncGeneratorSuspendReasonNone;
>
> Shouldn't this be @AsyncGeneratorSuspendReasonAwait? The WebInspector might eventually care about the distinction
Fixed
>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:174
>> + @awaitValue(generator, value, onFulfilled);
>
> In this case, you need a different reject handler which does not try to re-enter the generator body
Fixed
>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:219
>> + @asyncGeneratorReject(generator, next.value);
>
> since @asyncGeneratorReject recurses into asyncGeneratorResumeNext, it's a good opportunity for a tail-call (both here and in @asyncGeneratorReject)
Done
(In reply to Caitlin Potter (:caitp) from comment #120)
> Arg, I left a few more comments in BytecodeGenerator.cpp, but they didn't
> seem to get submitted! I'll try to summarize here (at least the ones which
> are still in memory):
>
> 1) in emitIteratorClose, don't use the function parseMode to determine
> whether to perform AsyncIteratorClose or not. Ordinary for-of loops (for
> example) should not await the result of the .return method.
>
> 2) Don't do the `await` in `doAsyncGeneratorBodyCall` after resuming, when
> the state becomes completed. The spec only asks you to `await` the operand
> of ReturnStatements with an explicit operand
> (https://tc39.github.io/proposal-async-iteration/#sec-return-statement). The
> extra Await is technically observable, so it's better if implementations
> match up. Instead, this should be handled in BytecodeGenerator.
Fixed both comments.
Thanks for patience in reviewing of my patches!
Comment on attachment 316652[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=316652&action=review
I think it's starting to look pretty good. Just a couple nits.
I noticed that for-await-of isn't implemented in this CL. Are you planning on adding that after?
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4858
> + RefPtr<RegisterID> iterator = emitGetIteratorByCurrentFunctionType(argument);
nit: Can we do something more like:
```
RefPtr<RegisterID> iterator = parseMode() == SourceParseMode::AsyncGeneratorBodyMode ? emitGetAsyncIterator(argument) : emitGetIterator(argument);
```
where `emitGetAsyncIterator` includes the creation of the async-from-sync iterator if the @@asyncIterator method isn't found? Then you could reuse it for for-await-of loops, once those are implemented. That's sort of what I was talking about --- either have 2 separate functions for the different GetIterator algorithms, or pass some kind of flag to the function as a parameter indicating how it should work.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:781
> + void emitIteratorClose(RegisterID* iterator, const ThrowableExpressionData* node, bool doEmitAwait = false);
nit: I think it would be preferred to use an enum instead of a boolean for `doEmitAwait`, but I dunno. See what proper reviewers think, I guess. As with GetIterator, I'd also be ok with a separate function implementing that algorithm (`emitAsyncIteratorClose` I guess). Up to you / other reviewers.
(In reply to Caitlin Potter (:caitp) from comment #125)
> Comment on attachment 316652[details]
> Patch
>
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316652&action=review
>
> I think it's starting to look pretty good. Just a couple nits.
>
> I noticed that for-await-of isn't implemented in this CL. Are you planning
> on adding that after?
Yeah, I've tried to decrease size of the patch. There separate issue for for-await-of syntax https://bugs.webkit.org/show_bug.cgi?id=166698>
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4858
> > + RefPtr<RegisterID> iterator = emitGetIteratorByCurrentFunctionType(argument);
>
> nit: Can we do something more like:
>
> ```
> RefPtr<RegisterID> iterator = parseMode() ==
> SourceParseMode::AsyncGeneratorBodyMode ? emitGetAsyncIterator(argument) :
> emitGetIterator(argument);
> ```
>
> where `emitGetAsyncIterator` includes the creation of the async-from-sync
> iterator if the @@asyncIterator method isn't found? Then you could reuse it
> for for-await-of loops, once those are implemented. That's sort of what I
> was talking about --- either have 2 separate functions for the different
> GetIterator algorithms, or pass some kind of flag to the function as a
> parameter indicating how it should work.
Oh OK. I see what you mean
>
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:781
> > + void emitIteratorClose(RegisterID* iterator, const ThrowableExpressionData* node, bool doEmitAwait = false);
>
> nit: I think it would be preferred to use an enum instead of a boolean for
> `doEmitAwait`, but I dunno. See what proper reviewers think, I guess. As
> with GetIterator, I'd also be ok with a separate function implementing that
> algorithm (`emitAsyncIteratorClose` I guess). Up to you / other reviewers.
Yeah, I'll check with reviewer
Thanks for review! I'll try to fix soon
Comment on attachment 316652[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=316652&action=review>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4858
>>> + RefPtr<RegisterID> iterator = emitGetIteratorByCurrentFunctionType(argument);
>>
>> nit: Can we do something more like:
>>
>> ```
>> RefPtr<RegisterID> iterator = parseMode() == SourceParseMode::AsyncGeneratorBodyMode ? emitGetAsyncIterator(argument) : emitGetIterator(argument);
>> ```
>>
>> where `emitGetAsyncIterator` includes the creation of the async-from-sync iterator if the @@asyncIterator method isn't found? Then you could reuse it for for-await-of loops, once those are implemented. That's sort of what I was talking about --- either have 2 separate functions for the different GetIterator algorithms, or pass some kind of flag to the function as a parameter indicating how it should work.
>
> Oh OK. I see what you mean
Fixed
Comment on attachment 317254[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=317254&action=review
This patch is huge. I've read most of it, but not all. Posting comments for now. r- for now because I found a couple bugs in builtins and Yusuke's comments.
> Source/JavaScriptCore/ChangeLog:13
> + # await - wait until promise will be resolved and than continue
than => then
> Source/JavaScriptCore/ChangeLog:15
> + The main difference between async function and async generator that,
generator that => generator is that
> Source/JavaScriptCore/ChangeLog:26
> + # The behavior of yield* is modified to support
> + delegation to async iterables
only inside an async generator?
> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:41
> + function(result) { promiseCapability.@resolve.@call(@undefined, { done: !!done, value: result }); },
Is this !!done in the spec? I think it may be observable.
> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:61
> + promiseCapability.@resolve.@call(@undefined, {value: returnValue, done: true});
style nit: Your object literals are not consistent with having a space around the after/before "{" "}" or not. I'm not sure what our official style in JSC is, but we should probably pick something and try to stick with it.
> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:127
> + if (!@isObject(syncIterator)) @throwTypeError('Only objects can be wrapped by async-from-sync wrapper');
style nit: newline after the if(...)
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:37
> + return queue.shift();
this is wrong, you need to use private methods here otherwise you're using an untrusted "shift" method being on ArrayPrototype.
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:102
> + function asyncGeneratorYieldAwaited(result)
> + {
> + generator.@asyncGeneratorSuspendReason = @AsyncGeneratorSuspendReasonYield;
> + @asyncGeneratorResolve(generator, result, false);
> + }
> +
> + generator.@asyncGeneratorSuspendReason = @AsyncGeneratorSuspendReasonAwait;
> +
> + @awaitValue(generator, value, asyncGeneratorYieldAwaited);
Why can't you just immediately resolve here? Won't we always have a regular object we need to resolve? Maybe I'm missing something.
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:114
> + const onRejected = onRejectedCallback || function(result) { @doAsyncGeneratorBodyCall(generator, result, @GeneratorResumeModeThrow); };
you only call this function with truthy values. Why the "||"?
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:137
> + if (generator.@generatorState === @AsyncGeneratorStateExecuting) {
> + generator.@generatorState = @AsyncGeneratorStateCompleted;
> + }
style: no braces
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:155
> + if (generator.@asyncGeneratorSuspendReason === @AsyncGeneratorSuspendReasonYield) {
> + return @asyncGeneratorYield(generator, value, resumeMode);
> + }
style: no braces
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:186
> +
> +
style: remove one line here please
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:211
> + } else if (state === @AsyncGeneratorStateCompleted) {
> + return @asyncGeneratorResolve(generator, @undefined, true);
> + }
style: no braces
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:232
> + if (generator.@asyncGeneratorQueue === @undefined)
> + generator.@asyncGeneratorQueue = [];
Why not do this during construction?
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:234
> + generator.@asyncGeneratorQueue.push({resumeMode, value, promiseCapability});
you need to use a private name here.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:64
> + enum EmitAwait { Yes, No };
style: enum class please
> Source/JavaScriptCore/jit/JITOpcodes.cpp:1043
> + else if (opcodeID == op_new_async_generator_func_exp)
> + callOperation(operationNewAsyncGeneratorFunction, dst, regT0, function);
> + else {
> + ASSERT(opcodeID == op_new_async_generator_func_exp);
> + callOperation(operationNewAsyncGeneratorFunction, dst, regT0, function);
Ditto to what Yusuke said here
> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1185
> + dataLogF("Creating async generator function!\n");
style: de-indent 4 spaces
> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1300
> + dispatch(4)
should be: dispatch(constexpr op_new_asyc_generator_func_length)
now that Keith made this possible
> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1305
> + dispatch(4)
ditto
> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:683
> + m_asyncGeneratorFunctionPrototype->putDirectWithoutTransition(vm, vm.propertyNames->constructor, asyncGeneratorFunctionConstructor, DontEnum | ReadOnly);
nit: This feels like it should be in AsyncGeneratorFunctionPrototype create.
Comment on attachment 317254[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=317254&action=review
I've split this patches according to suggestion from reviewers. Parser part with fixed comments https://bugs.webkit.org/show_bug.cgi?id=175210
Runtime part will be uploaded after approve or land the parser part and will include all fixed comment for current patch
>> Source/JavaScriptCore/parser/Parser.cpp:529
>> : SourceParseMode::AsyncFunctionBodyMode;
>
> Change it like
>
> if (isAsyncGeneratorFunctionParseMode(parseMode)
> innerParseMode = SourceParseMode::AsyncGeneratorBodyMode;
> else if (parseMode == SourceParseMode::AsyncArrowFunctionMode)
> innerParseMode = SourceParseMode::AsyncArrowFunctionBodyMode;
> else
> innerParseMode = SourceParseMode::AsyncFunctionBodyMode;
Done
>> Source/JavaScriptCore/parser/Parser.cpp:2488
>> : SourceParseMode::AsyncFunctionBodyMode;
>
> Ditto. Since this functionality is shown again, let's spawn this to static function.
Done
>> Source/JavaScriptCore/parser/Parser.cpp:2821
>> + break;
>
> Nice.
Oh, Thanks Katlyn Potter! She rewrote this part for me
>> Source/JavaScriptCore/parser/Parser.cpp:3927
>> + failIfTrue(isGeneratorMethodParseMode(parseMode) || isAsyncMethodParseMode(parseMode), "Expected a parenthesis for argument list");
>
> Let's use `parseMode != SourceParseMode::MethodMode` consistently.
Done
Comment on attachment 317254[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=317254&action=review>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:41
>> + function(result) { promiseCapability.@resolve.@call(@undefined, { done: !!done, value: result }); },
>
> Is this !!done in the spec? I think it may be observable.
Hmm, according to spec it is https://tc39.github.io/ecma262/#sec-iteratorcomplete toBoolean operation https://tc39.github.io/ecma262/#sec-toboolean. I thought it is the same as `!!`. should I replace by something?
>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:61
>> + promiseCapability.@resolve.@call(@undefined, {value: returnValue, done: true});
>
> style nit: Your object literals are not consistent with having a space around the after/before "{" "}" or not. I'm not sure what our official style in JSC is, but we should probably pick something and try to stick with it.
Done
>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:127
>> + if (!@isObject(syncIterator)) @throwTypeError('Only objects can be wrapped by async-from-sync wrapper');
>
> style nit: newline after the if(...)
Done
>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:37
>> + return queue.shift();
>
> this is wrong, you need to use private methods here otherwise you're using an untrusted "shift" method being on ArrayPrototype.
Done
>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:102
>> + @awaitValue(generator, value, asyncGeneratorYieldAwaited);
>
> Why can't you just immediately resolve here? Won't we always have a regular object we need to resolve? Maybe I'm missing something.
As I remember it is to make hidden await in yield i.e.:
```
let resolver;
async function * foo() {
yield 1;//step 1
yield new Promise(resolve=>{ resolver = resolve;});// step 2
yield 3;//step 3
}
const iter = foo();
iter.next().then(v=>print(v.value, v.done));
iter.next().then(v=>print(v.value, v.done));
iter.next().then(v=>print(v.value, v.done));
drainMicrotasks();
resolver(2);
```
Without this step we move father promise to step 3, and don't wait until promise in step 2 will be resolved
>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:114
>> + const onRejected = onRejectedCallback || function(result) { @doAsyncGeneratorBodyCall(generator, result, @GeneratorResumeModeThrow); };
>
> you only call this function with truthy values. Why the "||"?
Fixed
>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:137
>> + }
>
> style: no braces
Done
>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:155
>> + }
>
> style: no braces
Done
>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:186
>> +
>
> style: remove one line here please
Done
>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:211
>> + }
>
> style: no braces
Done
>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:232
>> + generator.@asyncGeneratorQueue = [];
>
> Why not do this during construction?
Ohh, Not sure that a I know how to set empty array within Bytecode
>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:234
>> + generator.@asyncGeneratorQueue.push({resumeMode, value, promiseCapability});
>
> you need to use a private name here.
Dome
2017-01-05 15:09 PST, GSkachkov
2017-01-09 11:33 PST, GSkachkov
2017-01-13 11:39 PST, GSkachkov
2017-01-14 07:31 PST, GSkachkov
2017-01-28 02:44 PST, GSkachkov
2017-01-28 03:23 PST, GSkachkov
2017-01-28 03:44 PST, GSkachkov
2017-01-28 05:00 PST, Build Bot
2017-01-28 05:04 PST, Build Bot
2017-01-28 05:09 PST, Build Bot
2017-01-28 05:14 PST, Build Bot
2017-01-28 05:20 PST, GSkachkov
2017-01-28 09:02 PST, GSkachkov
2017-01-28 10:20 PST, Build Bot
2017-01-28 10:36 PST, Build Bot
2017-01-28 10:43 PST, Build Bot
2017-01-28 13:14 PST, GSkachkov
2017-01-28 13:49 PST, GSkachkov
2017-02-02 13:09 PST, GSkachkov
2017-02-02 14:35 PST, GSkachkov
2017-02-04 01:25 PST, GSkachkov
2017-03-25 02:22 PDT, GSkachkov
2017-03-25 14:55 PDT, GSkachkov
2017-03-26 12:29 PDT, GSkachkov
2017-03-27 12:18 PDT, GSkachkov
2017-03-27 13:19 PDT, GSkachkov
2017-04-11 13:55 PDT, GSkachkov
2017-04-13 12:40 PDT, GSkachkov
2017-04-13 15:35 PDT, Build Bot
2017-05-25 10:40 PDT, GSkachkov
2017-05-25 11:48 PDT, Build Bot
2017-05-25 11:55 PDT, Build Bot
2017-05-25 12:20 PDT, Build Bot
2017-05-25 12:28 PDT, Build Bot
2017-06-08 14:01 PDT, GSkachkov
2017-06-08 15:06 PDT, Build Bot
2017-06-08 15:18 PDT, Build Bot
2017-06-08 15:19 PDT, Build Bot
2017-06-08 15:41 PDT, Build Bot
2017-06-28 00:06 PDT, GSkachkov
2017-06-28 00:31 PDT, GSkachkov
2017-06-28 10:44 PDT, GSkachkov
2017-06-28 12:10 PDT, GSkachkov
2017-06-29 13:43 PDT, GSkachkov
2017-07-22 16:04 PDT, GSkachkov
2017-07-22 17:08 PDT, Build Bot
2017-07-22 17:23 PDT, Build Bot
2017-07-22 17:28 PDT, Build Bot
2017-07-22 17:37 PDT, Build Bot
2017-07-23 14:28 PDT, GSkachkov
2017-07-24 13:13 PDT, GSkachkov
2017-07-26 12:21 PDT, GSkachkov
2017-07-28 11:24 PDT, GSkachkov
2017-08-01 13:57 PDT, GSkachkov
2017-08-04 10:15 PDT, GSkachkov
2017-08-05 11:34 PDT, GSkachkov