Bug 166698 – [ESNext] Async iteration - Implement async iteration statement: for-await-of

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[GSkachkov]

async function *foo() {...}

for await (const item of foo()) {
  debug(item);
}

[GSkachkov]

Created attachment 319132 [details]
Patch

WiP. Patch is mostly done, working on adding more tests

[Yusuke Suzuki]

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?

[GSkachkov]

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

[GSkachkov]

Created attachment 319202 [details]
Patch

Fixed comments and more tests

[Yusuke Suzuki]

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?).

[GSkachkov]

Created attachment 319354 [details]
Patch

Fix comments && added tests

[GSkachkov]

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 (...) {} }
}
```

[Yusuke Suzuki]

Comment on attachment 319354 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 319354 [details]
Patch

Clearing flags on attachment: 319354

Committed r221358: <http://trac.webkit.org/changeset/221358>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34158241>