async function *foo() {...} for await (const item of foo()) { debug(item); }
Created attachment 319132 [details] Patch WiP. Patch is mostly done, working on adding more tests
Comment on attachment 319132 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319132&action=review Looks good. But r- due to the bug and the test coverage. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4301 > + bool isForAwait = forLoopNode != nullptr ? forLoopNode->isForAwait() : false; style: we do not use `xxx != nullptr`. I think this can be `bool isForAwait = forLoopNode && forLoopNode->isForAwait();` > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4419 > + emitIteratorNext(value.get(), iterator.get(), node, isForAwait ? JSC::EmitAwait::Yes : JSC::EmitAwait::No); JSC:: is not necessary. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4432 > + emitIteratorClose(iterator.get(), node, isForAwait ? JSC::EmitAwait::Yes : JSC::EmitAwait::No); JSC:: is not necessary. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4584 > +RegisterID* BytecodeGenerator::emitIteratorNext(RegisterID* dst, RegisterID* iterator, const ThrowableExpressionData* node, JSC::EmitAwait doEmitAwait) JSC:: is not necessary. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:783 > + RegisterID* emitIteratorNext(RegisterID* dst, RegisterID* iterator, const ThrowableExpressionData* node, JSC::EmitAwait = JSC::EmitAwait::No); JSC:: is not necessary. > Source/JavaScriptCore/parser/Parser.cpp:1280 > + } I think this accidentally allows `for await (;;)`, `for await (let v = 42;;)` etc. If `for await` comes, only for-of style is allowed. And could you add a syntax test to cover cases like this?
Working on fixes. Also found one more issue in my patch that for await did not support the async-from-sync i.e. `for await (let v of [1, 2, 3])` Will be fixed in new patch
Created attachment 319202 [details] Patch Fixed comments and more tests
Comment on attachment 319202 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319202&action=review Nice! I think we have several bugs. > Source/JavaScriptCore/parser/Parser.cpp:1490 > + ASSERT(!isAwaitFor); In this `enumerationLoop:` labelled case, we do not check isAwaitFor. This means that `for await (expr of expr);` can be parsed. (And this assertion could fail.). Let's fix INTOKEN parsing as the same to `for await (var xxx of xxx)` case. And could you add tests for `for await (expr of expr);` and `for await (expr in expr);` cases? > Source/JavaScriptCore/parser/Parser.h:734 > + m_isAsyncFunctionBoundary = false; Why is this necessary? If it is necessary, is this enough? (We only set false for arrow functions. Why is this unnecessary for the other types of functions?).
Created attachment 319354 [details] Patch Fix comments && added tests
Comment on attachment 319202 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319202&action=review >> Source/JavaScriptCore/parser/Parser.cpp:1490 >> + ASSERT(!isAwaitFor); > > In this `enumerationLoop:` labelled case, we do not check isAwaitFor. > This means that `for await (expr of expr);` can be parsed. (And this assertion could fail.). > Let's fix INTOKEN parsing as the same to `for await (var xxx of xxx)` case. > And could you add tests for `for await (expr of expr);` and `for await (expr in expr);` cases? Yeah, you are right, I missed `if (!consume(INTOKEN)) {` in this part. Fixed and added test. >> Source/JavaScriptCore/parser/Parser.h:734 >> + m_isAsyncFunctionBoundary = false; > > Why is this necessary? If it is necessary, is this enough? (We only set false for arrow functions. Why is this unnecessary for the other types of functions?). There is a issue that, arrow function inherits async property from parent async function scope, I think it is wrong because we can't use `await` within arrow function that is declared async function. In my patch this lead that it was allow to use for await in arrow function that is declared in async function, that is wrong and should lead to Syntax Error: ``` async function () { const arr = () => { for await (...) {} } } ```
Comment on attachment 319354 [details] Patch r=me
Comment on attachment 319354 [details] Patch Clearing flags on attachment: 319354 Committed r221358: <http://trac.webkit.org/changeset/221358>
All reviewed patches have been landed. Closing bug.
<rdar://problem/34158241>