<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [ESNext] Async iteration - Implement Async Generator"
href="https://bugs.webkit.org/show_bug.cgi?id=166695#c35">Comment # 35</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [ESNext] Async iteration - Implement Async Generator"
href="https://bugs.webkit.org/show_bug.cgi?id=166695">bug 166695</a>
from <span class="vcard"><a class="email" href="mailto:utatane.tea@gmail.com" title="Yusuke Suzuki <utatane.tea@gmail.com>"> <span class="fn">Yusuke Suzuki</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=300053&action=diff" name="attach_300053" title="Patch">attachment 300053</a> <a href="attachment.cgi?id=300053&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=300053&action=review">https://bugs.webkit.org/attachment.cgi?id=300053&action=review</a>
Nice! Could you fix gtk/efl/win build failures?
<span class="quote">> Source/JavaScriptCore/ChangeLog:10
> + and async function. This will be fixed in <a class="bz_bug_link
bz_status_NEW "
title="NEW - [ESNext] Async iteration - Implement Async Generator runtime in specification compliance"
href="show_bug.cgi?id=166848">https://bugs.webkit.org/show_bug.cgi?id=166848</a></span >
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.
<span class="quote">> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:30
> +// FIXME: Current implementation is not follow spec in part of
> +// AsyncGenerator Abstract Operations
> +// (<a href="https://tc39.github.io/proposal-async-iteration/#sec-asyncgenerator-abstract-operations">https://tc39.github.io/proposal-async-iteration/#sec-asyncgenerator-abstract-operations</a>),
> +// to align implementation with spec created issue
> +// <a class="bz_bug_link
bz_status_NEW "
title="NEW - [ESNext] Async iteration - Implement Async Generator runtime in specification compliance"
href="show_bug.cgi?id=166848">https://bugs.webkit.org/show_bug.cgi?id=166848</a></span >
What is the observable behavior?
<span class="quote">> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:54
> +
> + promiseCapability.@resolve({ value, done });</span >
Drop this (see the latter comment).
<span class="quote">> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:71
> + wrappedValue.@promise.@then(</span >
Is this `@then` use correct?
<span class="quote">> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:73
> + function(error) { @asyncGeneratorResume(generator, promiseCapability, error, @GeneratorResumeModeThrow); });</span >
return here.
<span class="quote">> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:78
> + promiseCapability.@reject(error);</span >
return here.
<span class="quote">> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:79
> + }</span >
Move `promiseCapability.@resolve({ value, done });` here for both state case.
<span class="quote">> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:89
> + generator.@asyncGeneratorResumePromise.@then(</span >
Is this `@then` correct?
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:545
> + case SourceParseMode::AsyncGeneratorMethodMode:</span >
Let's name AsyncGeneratorWrapperMethodMode.
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:546
> + case SourceParseMode::AsyncGeneratorFunctionMode:</span >
Let's name AsyncGeneratorWrapperFunctionMode.
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3164
> + case SourceParseMode::AsyncGeneratorFunctionMode:</span >
Let's name AsyncGeneratorWrapperFunctionMode.
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3165
> + case SourceParseMode::AsyncGeneratorMethodMode:</span >
Let's name AsyncGeneratorWrapperMethodMode.
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3222
> + else if (function->parseMode() == SourceParseMode::AsyncGeneratorFunctionMode)</span >
Let's name AsyncGeneratorWrapperFunctionMode.
<span class="quote">> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3471
> + case SourceParseMode::AsyncGeneratorMethodMode:</span >
Let's name AsyncGeneratorWrapperMethodMode.
<span class="quote">> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3472
> + case SourceParseMode::AsyncGeneratorFunctionMode: {</span >
Let's name AsyncGeneratorWrapperFunctionMode.
<span class="quote">> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6432
> + // TODO: Rename to JSAsyncGenerator</span >
We do not use `TODO:`. Use `FIXME:` instead.
And if you use FIXME, the comment should have the bug URL.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:482
> +// TODO: Think about rename to parseAsyncFunctionOrAsyncGeneratorSourceElements</span >
We do not use TODO. Use `FIXME` instead.
And if you use FIXME, the comment should have a bug URL.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:1988
> + case SourceParseMode::AsyncGeneratorFunctionMode:</span >
Let's name AsyncGeneratorWrapperFunctionMode.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:1991
> + case SourceParseMode::AsyncGeneratorMethodMode:</span >
Let's name AsyncGeneratorWrapperMethodMode.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:2270
> + semanticFail("Cannot declare async function named 'await'"); // TODO: Fix text message for async generators</span >
We do not use TODO. Use FIXME instead.
And if you use FIXME, the comment should have a bug URL.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:2510
> + parseMode = SourceParseMode::AsyncGeneratorFunctionMode;</span >
Let's name AsyncGeneratorWrapperFunctionMode.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:2696
> + ident = m_token.m_data.ident;</span >
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.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:2705
> + ? SourceParseMode::AsyncGeneratorMethodMode</span >
Let's name AsyncGeneratorWrapperMethodMode.
<span class="quote">> 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'");
> }</span >
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.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:3745
> + isAsyncGenerator = true;</span >
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?
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:3798
> + parseMode = SourceParseMode::AsyncGeneratorMethodMode;</span >
Let's name AsyncGeneratorWrapperMethodMode.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:3855
> + ASSERT(parseMode == SourceParseMode::MethodMode || parseMode == SourceParseMode::GeneratorWrapperFunctionMode || parseMode == SourceParseMode::AsyncMethodMode || parseMode == SourceParseMode::AsyncGeneratorMethodMode);</span >
Let's name AsyncGeneratorWrapperMethodMode.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:4164
> + parseMode = SourceParseMode::AsyncGeneratorFunctionMode;</span >
Let's name AsyncGeneratorWrapperFunctionMode.
<span class="quote">> Source/JavaScriptCore/parser/Parser.h:279
> + case SourceParseMode::AsyncGeneratorMethodMode:</span >
Let's name AsyncGeneratorWrapperMethodMode.
<span class="quote">> Source/JavaScriptCore/parser/Parser.h:280
> + case SourceParseMode::AsyncGeneratorFunctionMode:</span >
Let's name AsyncGeneratorWrapperFunctionMode.
<span class="quote">> Source/JavaScriptCore/parser/ParserModes.h:62
> + AsyncGeneratorFunctionMode = 0b00000000000000010000000000000000,</span >
Let's name AsyncGeneratorWrapperFunctionMode.
<span class="quote">> Source/JavaScriptCore/parser/ParserModes.h:63
> + AsyncGeneratorMethodMode = 0b00000000000000100000000000000000,</span >
Let's name AsyncGeneratorWrapperMethodMode.
<span class="quote">> Source/JavaScriptCore/parser/ParserModes.h:110
> + SourceParseMode::AsyncGeneratorFunctionMode,</span >
Let's name AsyncGeneratorWrapperFunctionMode.
<span class="quote">> Source/JavaScriptCore/parser/ParserModes.h:111
> + SourceParseMode::AsyncGeneratorMethodMode).contains(parseMode);</span >
Let's name AsyncGeneratorWrappeMethodMode.
<span class="quote">> Source/JavaScriptCore/parser/ParserModes.h:117
> + SourceParseMode::AsyncGeneratorFunctionMode,</span >
Let's name AsyncGeneratorWrapperFunctionMode.
<span class="quote">> Source/JavaScriptCore/parser/ParserModes.h:119
> + SourceParseMode::AsyncGeneratorMethodMode,</span >
Let's name AsyncGeneratorWrapperMethodMode.
<span class="quote">> Source/JavaScriptCore/parser/ParserModes.h:137
> + SourceParseMode::AsyncGeneratorFunctionMode,</span >
Let's name AsyncGeneratorWrapperFunctionMode.
<span class="quote">> Source/JavaScriptCore/parser/ParserModes.h:138
> + SourceParseMode::AsyncGeneratorMethodMode).contains(parseMode);</span >
Let's name AsyncGeneratorWrapperMethodMode.
<span class="quote">> Source/JavaScriptCore/parser/ParserModes.h:146
> + SourceParseMode::AsyncGeneratorFunctionMode,</span >
Let's name AsyncGeneratorWrapperFunctionMode.
<span class="quote">> Source/JavaScriptCore/parser/ParserModes.h:147
> + SourceParseMode::AsyncGeneratorMethodMode,</span >
Let's name AsyncGeneratorWrapperMethodMode.
<span class="quote">> Source/JavaScriptCore/parser/ParserModes.h:170
> + // FIXME: GeneratorWrapperFunctionMode is not guaranteed to be a method.</span >
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.
<span class="quote">> Source/JavaScriptCore/parser/ParserModes.h:181
> + return SourceParseModeSet(SourceParseMode::AsyncGeneratorFunctionMode).contains(parseMode);</span >
Let's name AsyncGeneratorWrapperFunctionMode.
<span class="quote">> Source/JavaScriptCore/parser/ParserModes.h:193
> + SourceParseMode::AsyncGeneratorMethodMode).contains(parseMode);</span >
Let's name AsyncGeneratorWrapperMethodMode.
<span class="quote">> Source/JavaScriptCore/parser/ParserModes.h:211
> + SourceParseMode::AsyncGeneratorFunctionMode,</span >
Let's name AsyncGeneratorWrapperFunctionMode.
<span class="quote">> Source/JavaScriptCore/parser/ParserModes.h:213
> + SourceParseMode::AsyncGeneratorMethodMode).contains(parseMode);</span >
Let's name AsyncGeneratorWrapperMethodMode.
<span class="quote">> Source/JavaScriptCore/runtime/AsyncGeneratorFunctionConstructor.cpp:49
> + // Number of arguments for constructor</span >
This comment is unnecessary.
<span class="quote">> Source/JavaScriptCore/runtime/AsyncGeneratorFunctionConstructor.cpp:56
> + // TODO: Use Async Generator instead</span >
We do not use TODO. Use FIXME instead.
And if you use FIXME, the comment should have a bug URL.
<span class="quote">> Source/JavaScriptCore/runtime/AsyncGeneratorPrototype.h:37
> + static const unsigned StructureFlags = Base::StructureFlags | HasStaticPropertyTable;</span >
We do not have static property table for this. Drop HasStaticPropertyTable.
<span class="quote">> Source/JavaScriptCore/runtime/FunctionExecutable.h:126
> + bool isAsyncGenerator() const { return SourceParseModeSet(SourceParseMode::AsyncGeneratorFunctionMode, SourceParseMode::AsyncGeneratorBodyMode).contains(parseMode()); }</span >
Let's name AsyncGeneratorWrapperFunctionMode.
<span class="quote">> Source/JavaScriptCore/runtime/FunctionExecutable.h:141
> + SourceParseMode::AsyncGeneratorFunctionMode,</span >
Let's name AsyncGeneratorWrapperFunctionMode.
<span class="quote">> Source/JavaScriptCore/runtime/JSFunction.cpp:355
> + } else if (thisObject->jsExecutable()->parseMode() == SourceParseMode::AsyncGeneratorFunctionMode)</span >
Let's name AsyncGeneratorWrapperFunctionMode.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>