[Webkit-unassigned] [Bug 166695] [ESNext] Async iteration - Implement Async Generator
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jan 30 04:05:37 PST 2017
https://bugs.webkit.org/show_bug.cgi?id=166695
--- Comment #35 from Yusuke Suzuki <utatane.tea at gmail.com> ---
Comment on attachment 300053
--> https://bugs.webkit.org/attachment.cgi?id=300053
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. at resolve({ value, done });
Drop this (see the latter comment).
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:71
> + wrappedValue. at 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. at reject(error);
return here.
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:79
> + }
Move `promiseCapability. at resolve({ value, done });` here for both state case.
> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:89
> + generator. at 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.
--
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170130/c63d1cc2/attachment-0001.html>
More information about the webkit-unassigned
mailing list