[async-iteration] implement Async-from-Sync Iterator

[async-iteration] implement Async-from-Sync Iterator

Introduce a new Object to allow GetIterator("async") to function when the
iterable does not have a Symbol.asyncIterator method.

This patch has been split out from https://codereview.chromium.org/2622833002/
and incorporates test cases.

BUG=v8:5855, v8:4483
R=jgruber@chromium.org, rmcilroy@chromium.org, neis@chromium.org
TBR=hpayer@chromium.org, bmeurer@chromium.org

Review-Url: https://codereview.chromium.org/2645313003
Cr-Commit-Position: refs/heads/master@{#43419}
Committed: https://chromium.googlesource.com/v8/v8/+/0423341034c767d91128c9fd68e3f51e60359c1c

[caitp, 2017-01-23 17:29:16]

Description was changed from

==========
[async-iteration] implement Async-from-Sync Iterator

Introduce a new Object to allow GetIterator("async") to function when the
iterable does not have a Symbol.asyncIterator method.

This patch has been split out from https://codereview.chromium.org/2622833002/
and incorporates test cases.

BUG=v8:5855, v8:4483
R=adamk@chromium.org, littledan@chromium.org, gsathya@chromium.org
==========

to

==========
[async-iteration] implement Async-from-Sync Iterator

Introduce a new Object to allow GetIterator("async") to function when the
iterable does not have a Symbol.asyncIterator method.

This patch has been split out from https://codereview.chromium.org/2622833002/
and incorporates test cases.

BUG=v8:5855, v8:4483
R=adamk@chromium.org, littledan@chromium.org, gsathya@chromium.org
==========

[caitp, 2017-01-23 17:29:16]

caitp@igalia.com changed reviewers:
+ bmeurer@chromium.org

[caitp, 2017-01-23 17:34:28]

caitp@igalia.com changed reviewers:
+ rmcilroy@chromium.org

[caitp, 2017-01-23 17:34:29]

Hi, PTAL

This is another chunk of code split out from the big WIP CL, implementing a
small subset of the spec, with test coverage.

Most of this is implemented via JS builtins, with a few changes to
interpreter-intrinsics for the creation of the async-iterator.

[Benedikt Meurer, 2017-01-23 18:26:51]

bmeurer@chromium.org changed reviewers:
+ neis@chromium.org

[caitp, 2017-01-23 20:34:25]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 20:34:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 20:36:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 20:36:19]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/19745)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/19814)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/19714)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/33137)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/21457)

[caitp, 2017-01-23 20:39:58]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 20:40:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 20:41:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 20:41:39]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/19747)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/19816)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/19716)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/33139)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/21459)

[caitp, 2017-01-23 21:53:16]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 21:53:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 22:28:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 22:28:31]

Dry run: This issue passed the CQ dry run.

[jgruber, 2017-01-24 16:48:39]

jgruber@chromium.org changed reviewers:
+ jgruber@chromium.org

[jgruber, 2017-01-24 16:48:40]

A couple of DBC, only looked at builtins-async.

https://codereview.chromium.org/2645313003/diff/80001/src/builtins/builtins-a...
File src/builtins/builtins-async-iterator.cc (right):

https://codereview.chromium.org/2645313003/diff/80001/src/builtins/builtins-a...
src/builtins/builtins-async-iterator.cc:59: Node* const fast_iter_result_map =
This is used a couple of times in this file, can we extract the fast path check
into a function?

https://codereview.chromium.org/2645313003/diff/80001/src/builtins/builtins-a...
src/builtins/builtins-async-iterator.cc:103:
GotoIf(TaggedIsSmi(var_done.value()), &to_boolean);
Can this ever be needed for the fast path?

https://codereview.chromium.org/2645313003/diff/80001/src/builtins/builtins-a...
src/builtins/builtins-async-iterator.cc:128: Node* const map =
LoadContextElement(
Same here, can we extract creation of the closure?

https://codereview.chromium.org/2645313003/diff/80001/src/builtins/builtins-a...
src/builtins/builtins-async-iterator.cc:157: LoadContextElement(native_context,
Context::MAKE_TYPE_ERROR_INDEX);
This is starting to pop up in several places, maybe we should add a helper
method to CSA for throwing errors at some point.

https://codereview.chromium.org/2645313003/diff/80001/src/builtins/builtins-a...
src/builtins/builtins-async-iterator.cc:162: HeapConstant(method_name),
iterator);
Maybe move method_name and method_str into this scope.

https://codereview.chromium.org/2645313003/diff/80001/src/builtins/builtins-a...
src/builtins/builtins-async-iterator.cc:183:
TF_BUILTIN(AsyncFromSyncIteratorPrototypeReturn, AsyncBuiltinsAssembler) {
These 3 functions seem to have very similar structure, perhaps we can avoid some
more duplicate code.

[caitp, 2017-01-24 16:58:36]

I'll address the other comments in a bit, thanks for taking a look

https://codereview.chromium.org/2645313003/diff/80001/src/builtins/builtins-a...
File src/builtins/builtins-async-iterator.cc (right):

https://codereview.chromium.org/2645313003/diff/80001/src/builtins/builtins-a...
src/builtins/builtins-async-iterator.cc:103:
GotoIf(TaggedIsSmi(var_done.value()), &to_boolean);
On 2017/01/24 16:48:39, jgruber wrote:
> Can this ever be needed for the fast path?

I'm not sure. Does each ObjectLiteral in source code produce its own Map, even
if it should be identical to other Maps (such as the builtin IteratorResult
map)? If that's the case, the fast path can skip the BooleanMap check as well.

[rmcilroy, 2017-01-25 11:07:37]

interpreter/ changes LGTM, I didn't look at the rest though.

[caitp, 2017-02-14 22:28:18]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 22:28:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[caitp, 2017-02-14 22:29:40]

On 2017/01/25 11:07:37, rmcilroy wrote:
> interpreter/ changes LGTM, I didn't look at the rest though.

I've added requested bytecode generator tests for for-await-of here.

[commit-bot: I haz the power, 2017-02-14 23:08:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-14 23:08:49]

Dry run: Try jobs failed on following builders:
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/20932)
  v8_linux_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)

[caitp, 2017-02-15 19:44:06]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 19:44:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[caitp, 2017-02-15 19:48:49]

So, for-await-of landed, this CL is next in line and ready for a proper look.

I could especially use a look at the new TF builtins from jgruber, gsathya (and
Benedikt or other compilers folks if interested)

[caitp, 2017-02-15 20:00:40]

https://codereview.chromium.org/2645313003/diff/120001/test/cctest/interprete...
File test/cctest/interpreter/bytecode_expectations/ForAwaitOf.golden (right):

https://codereview.chromium.org/2645313003/diff/120001/test/cctest/interprete...
test/cctest/interpreter/bytecode_expectations/ForAwaitOf.golden:53:
B(CallJSRuntime), U8(%UnknownIntrinsicIndex), R(5), U8(1),
Should be fixed by https://codereview.chromium.org/2695323002, I believe

[commit-bot: I haz the power, 2017-02-15 20:29:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 20:29:57]

Dry run: Try jobs failed on following builders:
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/21000)
  v8_linux_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)

[caitp, 2017-02-15 20:57:21]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 20:57:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 21:39:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 21:39:11]

Dry run: Try jobs failed on following builders:
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/21003)
  v8_linux_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)

[caitp, 2017-02-15 22:04:09]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 22:04:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[caitp, 2017-02-15 22:12:46]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 22:12:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[caitp, 2017-02-15 22:18:01]

https://codereview.chromium.org/2645313003/diff/80001/src/builtins/builtins-a...
File src/builtins/builtins-async-iterator.cc (right):

https://codereview.chromium.org/2645313003/diff/80001/src/builtins/builtins-a...
src/builtins/builtins-async-iterator.cc:59: Node* const fast_iter_result_map =
On 2017/01/24 16:48:39, jgruber wrote:
> This is used a couple of times in this file, can we extract the fast path
check
> into a function?

I've moved the "Load IteratorResult properties" into a separate function to
simplify this. PTAL

https://codereview.chromium.org/2645313003/diff/80001/src/builtins/builtins-a...
src/builtins/builtins-async-iterator.cc:103:
GotoIf(TaggedIsSmi(var_done.value()), &to_boolean);
On 2017/01/24 16:58:36, caitp wrote:
> On 2017/01/24 16:48:39, jgruber wrote:
> > Can this ever be needed for the fast path?
> 
> I'm not sure. Does each ObjectLiteral in source code produce its own Map, even
> if it should be identical to other Maps (such as the builtin IteratorResult
> map)? If that's the case, the fast path can skip the BooleanMap check as well.

I'm pretty sure it's safe to trust that the map will always have a proper
boolean for `done`, I don't think this map can actually be produced manually in
JS code. Someone should let me know if I'm wrong about that.

https://codereview.chromium.org/2645313003/diff/80001/src/builtins/builtins-a...
src/builtins/builtins-async-iterator.cc:128: Node* const map =
LoadContextElement(
On 2017/01/24 16:48:39, jgruber wrote:
> Same here, can we extract creation of the closure?

Done

https://codereview.chromium.org/2645313003/diff/80001/src/builtins/builtins-a...
src/builtins/builtins-async-iterator.cc:157: LoadContextElement(native_context,
Context::MAKE_TYPE_ERROR_INDEX);
On 2017/01/24 16:48:39, jgruber wrote:
> This is starting to pop up in several places, maybe we should add a helper
> method to CSA for throwing errors at some point.

I've added one to this CL, I don't think it's too bad. Readable at least.

https://codereview.chromium.org/2645313003/diff/80001/src/builtins/builtins-a...
src/builtins/builtins-async-iterator.cc:162: HeapConstant(method_name),
iterator);
On 2017/01/24 16:48:39, jgruber wrote:
> Maybe move method_name and method_str into this scope.

I'm keeping the c-string near the top of the function so it's easy to see that
it matches the right one

https://codereview.chromium.org/2645313003/diff/80001/src/builtins/builtins-a...
src/builtins/builtins-async-iterator.cc:183:
TF_BUILTIN(AsyncFromSyncIteratorPrototypeReturn, AsyncBuiltinsAssembler) {
On 2017/01/24 16:48:39, jgruber wrote:
> These 3 functions seem to have very similar structure, perhaps we can avoid
some
> more duplicate code.

Maybe, I've made them more concise with the helper functions, though. I'll see
if they can share some logic

https://codereview.chromium.org/2645313003/diff/160001/src/code-stub-assembler.h
File src/code-stub-assembler.h (right):

https://codereview.chromium.org/2645313003/diff/160001/src/code-stub-assemble...
src/code-stub-assembler.h:1246: return CallJS(CodeFactory::Call(isolate()),
context, make_type_error,
Maybe it would make sense for MakeTypeError (and friends) to be a runtime
function instead of a C++ builtin. At least it would avoid needing to load it
from native_context, and avoid installing it per context. Would make the CSA
helper simpler

[commit-bot: I haz the power, 2017-02-15 22:49:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 22:49:29]

Dry run: Try jobs failed on following builders:
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/21009)
  v8_linux_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)

[Benedikt Meurer, 2017-02-16 05:16:39]

https://codereview.chromium.org/2645313003/diff/200001/src/builtins/builtins-...
File src/builtins/builtins-async-iterator.cc (right):

https://codereview.chromium.org/2645313003/diff/200001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:16: class ValueUnwrapContext {
Can we wrap these helpers into an anonymous namespace?

https://codereview.chromium.org/2645313003/diff/200001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:168: // TODO(caitp): Use TailCallStub
once it's possible for a JS-linkage builtin
Is the TailCall required by the specification? Because if it's for performance
reasons, then we shouldn't do it, especially since we measured in the past that
tail call from JS to CodeStub wasn't more efficient.

https://codereview.chromium.org/2645313003/diff/200001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:351:
CallStub(CodeFactory::GetProperty(isolate()), context, sync_iterator,
How about introducing a helper method on the CSA for this? Like

  GetProperty(Node* context, Node* object, Handle<Name> name)

We have this all over the code base now.

[caitp, 2017-02-16 14:24:55]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 14:25:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[caitp, 2017-02-16 14:25:12]

https://codereview.chromium.org/2645313003/diff/200001/src/builtins/builtins-...
File src/builtins/builtins-async-iterator.cc (right):

https://codereview.chromium.org/2645313003/diff/200001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:16: class ValueUnwrapContext {
On 2017/02/16 05:16:38, Benedikt Meurer wrote:
> Can we wrap these helpers into an anonymous namespace?

Done.

https://codereview.chromium.org/2645313003/diff/200001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:168: // TODO(caitp): Use TailCallStub
once it's possible for a JS-linkage builtin
On 2017/02/16 05:16:38, Benedikt Meurer wrote:
> Is the TailCall required by the specification? Because if it's for performance
> reasons, then we shouldn't do it, especially since we measured in the past
that
> tail call from JS to CodeStub wasn't more efficient.

Not required by the spec. My reasoning is, it should be a stub call to make
these builtins smaller rather than duplicating a lot of code, and a tailcall
should help reduce odds of a stack overflow. Those are the only reasons for it.

https://codereview.chromium.org/2645313003/diff/200001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:351:
CallStub(CodeFactory::GetProperty(isolate()), context, sync_iterator,
On 2017/02/16 05:16:38, Benedikt Meurer wrote:
> How about introducing a helper method on the CSA for this? Like
> 
>   GetProperty(Node* context, Node* object, Handle<Name> name)
> 
> We have this all over the code base now.

Done.

[commit-bot: I haz the power, 2017-02-16 14:43:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 14:43:12]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)

[caitp, 2017-02-16 14:55:41]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 14:55:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[caitp, 2017-02-16 14:55:57]

https://codereview.chromium.org/2645313003/diff/240001/src/builtins/builtins-...
File src/builtins/builtins-async-iterator.cc (right):

https://codereview.chromium.org/2645313003/diff/240001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:175: Node* const error =
This is unreachable in the current spec proposal, so I'm just removing this and
changing it to an assertion that `iterator` is an async-from-sync iterator. I've
messaged the editor about it and he agrees, so the spec should change

[caitp, 2017-02-16 14:58:03]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 14:58:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[caitp, 2017-02-16 15:12:32]

https://codereview.chromium.org/2645313003/diff/240001/src/builtins/builtins-...
File src/builtins/builtins-async-iterator.cc (right):

https://codereview.chromium.org/2645313003/diff/240001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:175: Node* const error =
On 2017/02/16 14:55:56, caitp wrote:
> This is unreachable in the current spec proposal, so I'm just removing this
and
> changing it to an assertion that `iterator` is an async-from-sync iterator.
I've
> messaged the editor about it and he agrees, so the spec should change

but, it is reachable with function.caller, so removing this stuff doesn't work
:( I guess the spec change can't happen. Was worth a try :)

[commit-bot: I haz the power, 2017-02-16 15:23:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 15:23:14]

Dry run: This issue passed the CQ dry run.

[caitp, 2017-02-16 16:32:52]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 16:32:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 17:01:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 17:01:07]

Dry run: This issue passed the CQ dry run.

[jgruber, 2017-02-16 17:22:00]

On 2017/02/15 19:48:49, caitp wrote:
> So, for-await-of landed, this CL is next in line and ready for a proper look.
> 
> I could especially use a look at the new TF builtins from jgruber, gsathya
(and
> Benedikt or other compilers folks if interested)

Sorry for the delay, I'll take a look first thing tomorrow.

[neis, 2017-02-17 10:34:26]

I'll also send out a review today or Monday.

[jgruber, 2017-02-17 12:28:24]

👌👌 lgtm once comments addressed.

Didn't look at the tests in detail.

https://codereview.chromium.org/2645313003/diff/320001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2645313003/diff/320001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:792: void
Genesis::CreateAsyncIteratorMaps(Handle<JSFunction> empty) {
The argument is unused.

https://codereview.chromium.org/2645313003/diff/320001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:801:
async_iterator_prototype_iterator->shared()->set_native(true);
set_native is done by SimpleCreateFunction AFAIK.
Maybe we could add a new SimpleInstallFunction variant that wraps these calls.

https://codereview.chromium.org/2645313003/diff/320001/src/builtins/builtins-...
File src/builtins/builtins-async-iterator.cc (right):

https://codereview.chromium.org/2645313003/diff/320001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:28: std::pair<Node*, Node*>
LoadIteratorResult(Node* const context,
Let's document what this returns.

https://codereview.chromium.org/2645313003/diff/320001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:34: Node* CreateUnwrapClosure(Node*
native_context, Node* done);
Nit: I think it'd help readability if we stay consistent with const usage -
either mark every single assignment Node* const (I prefer that personally), or
just keep them all non-const.

https://codereview.chromium.org/2645313003/diff/320001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:83:
Branch(IsBooleanMap(LoadMap(var_done.value())), &done, &to_boolean);
Nit: Looks like we have IsBoolean() now.

https://codereview.chromium.org/2645313003/diff/320001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:124: Variable var_exception(this,
MachineRepresentation::kTagged);
Nit: var_exception(this, MR::kTagged, UndefinedConstant())

https://codereview.chromium.org/2645313003/diff/320001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:187: Node* const promise =
AllocateAndInitJSPromise(context);
Doesn't the proposal say to reject promiseCapability (i.e. the promise created
in line 122)? Not sure if I'm reading the spec correctly.

If creating a new promise here is valid, please use another (non-shadowing)
name.

https://codereview.chromium.org/2645313003/diff/320001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:209: Variable var_exception(this,
MachineRepresentation::kTagged);
Ditto

https://codereview.chromium.org/2645313003/diff/320001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:290: Bind(&if_receiverisincompatible);
I think you could refactor this into a ThrowIfNotAsyncFromSyncIterator method
(including the type checks) and use that in these 3 methods.

https://codereview.chromium.org/2645313003/diff/320001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:330: var_exception.Bind(parameter);
Ditto

https://codereview.chromium.org/2645313003/diff/320001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:418: Node* const done =
LoadContextElement(context, ValueUnwrapContext::kDoneSlot);
CSA_ASSERT(IsBoolean)

https://codereview.chromium.org/2645313003/diff/320001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:426: Node* native_context, Node* done) {
Please add a CSA_ASSERT(IsNativeContext && IsBoolean)

https://codereview.chromium.org/2645313003/diff/320001/src/code-stub-assembler.h
File src/code-stub-assembler.h (right):

https://codereview.chromium.org/2645313003/diff/320001/src/code-stub-assemble...
src/code-stub-assembler.h:1250: Node* MakeTypeError(MessageTemplate::Template
message, Node* context,
Great to have this and the above. We should update current use sites in a
separate CL.

https://codereview.chromium.org/2645313003/diff/320001/src/code-stub-assemble...
src/code-stub-assembler.h:1251: TArgs... args) {
The MakeTypeError builtin can only handle up to 3 args, please add a
STATIC_ASSERT(sizeof...(TArgs) <= 3);

https://codereview.chromium.org/2645313003/diff/320001/src/compiler/code-asse...
File src/compiler/code-assembler.cc (right):

https://codereview.chromium.org/2645313003/diff/320001/src/compiler/code-asse...
src/compiler/code-assembler.cc:190: Node* CodeAssembler::CStringConstant(const
char* str) {
Just an idea - maybe it'd make sense to add an option to create an internalized
string. That can be important e.g. if we use it for property loads.

https://codereview.chromium.org/2645313003/diff/320001/src/interpreter/interp...
File src/interpreter/interpreter-intrinsics.cc (right):

https://codereview.chromium.org/2645313003/diff/320001/src/interpreter/interp...
src/interpreter/interpreter-intrinsics.cc:329: __
CallRuntime(Runtime::kThrowSymbolIteratorInvalid, context));
Can we TailCallRuntime from here (v8:5939)?. Otherwise please add a comment that
line 330 is unreachable.

https://codereview.chromium.org/2645313003/diff/320001/src/runtime/runtime-in...
File src/runtime/runtime-internal.cc (right):

https://codereview.chromium.org/2645313003/diff/320001/src/runtime/runtime-in...
src/runtime/runtime-internal.cc:525: Handle<HeapObject>::cast(sync_iterator));
We just asserted it's a JSReceiver - can we make the NewJSAsyncFromSyncIterator
arguments / the field in objects.h a JSReceiver?

[caitp, 2017-02-17 14:56:05]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-17 14:56:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[caitp, 2017-02-17 14:56:11]

https://codereview.chromium.org/2645313003/diff/320001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2645313003/diff/320001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:792: void
Genesis::CreateAsyncIteratorMaps(Handle<JSFunction> empty) {
On 2017/02/17 12:28:22, jgruber wrote:
> The argument is unused.

Removed (will re-add in the next CL I guess)

https://codereview.chromium.org/2645313003/diff/320001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:801:
async_iterator_prototype_iterator->shared()->set_native(true);
On 2017/02/17 12:28:22, jgruber wrote:
> set_native is done by SimpleCreateFunction AFAIK.
> Maybe we could add a new SimpleInstallFunction variant that wraps these calls.

Removed duplicate call

https://codereview.chromium.org/2645313003/diff/320001/src/builtins/builtins-...
File src/builtins/builtins-async-iterator.cc (right):

https://codereview.chromium.org/2645313003/diff/320001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:28: std::pair<Node*, Node*>
LoadIteratorResult(Node* const context,
On 2017/02/17 12:28:23, jgruber wrote:
> Let's document what this returns.

Done.

https://codereview.chromium.org/2645313003/diff/320001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:83:
Branch(IsBooleanMap(LoadMap(var_done.value())), &done, &to_boolean);
On 2017/02/17 12:28:23, jgruber wrote:
> Nit: Looks like we have IsBoolean() now.

Done.

https://codereview.chromium.org/2645313003/diff/320001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:124: Variable var_exception(this,
MachineRepresentation::kTagged);
On 2017/02/17 12:28:23, jgruber wrote:
> Nit: var_exception(this, MR::kTagged, UndefinedConstant())

Done.

https://codereview.chromium.org/2645313003/diff/320001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:187: Node* const promise =
AllocateAndInitJSPromise(context);
On 2017/02/17 12:28:23, jgruber wrote:
> Doesn't the proposal say to reject promiseCapability (i.e. the promise created
> in line 122)? Not sure if I'm reading the spec correctly.
> 
> If creating a new promise here is valid, please use another (non-shadowing)
> name.

You're right, and I have no idea why there was a new Promise there. It's gone!

https://codereview.chromium.org/2645313003/diff/320001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:209: Variable var_exception(this,
MachineRepresentation::kTagged);
On 2017/02/17 12:28:23, jgruber wrote:
> Ditto

Done.

https://codereview.chromium.org/2645313003/diff/320001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:290: Bind(&if_receiverisincompatible);
On 2017/02/17 12:28:23, jgruber wrote:
> I think you could refactor this into a ThrowIfNotAsyncFromSyncIterator method
> (including the type checks) and use that in these 3 methods.

Done.

https://codereview.chromium.org/2645313003/diff/320001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:330: var_exception.Bind(parameter);
On 2017/02/17 12:28:23, jgruber wrote:
> Ditto

Done.

https://codereview.chromium.org/2645313003/diff/320001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:418: Node* const done =
LoadContextElement(context, ValueUnwrapContext::kDoneSlot);
On 2017/02/17 12:28:23, jgruber wrote:
> CSA_ASSERT(IsBoolean)

Done.

https://codereview.chromium.org/2645313003/diff/320001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:426: Node* native_context, Node* done) {
On 2017/02/17 12:28:23, jgruber wrote:
> Please add a CSA_ASSERT(IsNativeContext && IsBoolean)

Done.

https://codereview.chromium.org/2645313003/diff/320001/src/code-stub-assembler.h
File src/code-stub-assembler.h (right):

https://codereview.chromium.org/2645313003/diff/320001/src/code-stub-assemble...
src/code-stub-assembler.h:1251: TArgs... args) {
On 2017/02/17 12:28:23, jgruber wrote:
> The MakeTypeError builtin can only handle up to 3 args, please add a
> STATIC_ASSERT(sizeof...(TArgs) <= 3);

Done.

https://codereview.chromium.org/2645313003/diff/320001/src/compiler/code-asse...
File src/compiler/code-assembler.cc (right):

https://codereview.chromium.org/2645313003/diff/320001/src/compiler/code-asse...
src/compiler/code-assembler.cc:190: Node* CodeAssembler::CStringConstant(const
char* str) {
On 2017/02/17 12:28:23, jgruber wrote:
> Just an idea - maybe it'd make sense to add an option to create an
internalized
> string. That can be important e.g. if we use it for property loads.

I think most of the time in those situations we end up using string constants
from heap-symbols.h, but maybe it would be more readable to do it that way.

How would you imagine writing it? `CStringConstant("foo", true); to internalize
"foo"? Or maybe someting more like `InternalizedCStringConstant("foo");`

https://codereview.chromium.org/2645313003/diff/320001/src/interpreter/interp...
File src/interpreter/interpreter-intrinsics.cc (right):

https://codereview.chromium.org/2645313003/diff/320001/src/interpreter/interp...
src/interpreter/interpreter-intrinsics.cc:329: __
CallRuntime(Runtime::kThrowSymbolIteratorInvalid, context));
On 2017/02/17 12:28:23, jgruber wrote:
> Can we TailCallRuntime from here (v8:5939)?. Otherwise please add a comment
that
> line 330 is unreachable.

Done.

https://codereview.chromium.org/2645313003/diff/320001/src/runtime/runtime-in...
File src/runtime/runtime-internal.cc (right):

https://codereview.chromium.org/2645313003/diff/320001/src/runtime/runtime-in...
src/runtime/runtime-internal.cc:525: Handle<HeapObject>::cast(sync_iterator));
On 2017/02/17 12:28:23, jgruber wrote:
> We just asserted it's a JSReceiver - can we make the
NewJSAsyncFromSyncIterator
> arguments / the field in objects.h a JSReceiver?

This _may_ be a problem for heap verifier, if verification happens before the
field is set. I believe that's why I left it as a HeapObject.

I'll try to change it, but it will likely cause crashes in tests

[commit-bot: I haz the power, 2017-02-17 15:22:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 15:22:54]

Dry run: This issue passed the CQ dry run.

[jgruber, 2017-02-17 16:13:39]

https://codereview.chromium.org/2645313003/diff/320001/src/compiler/code-asse...
File src/compiler/code-assembler.cc (right):

https://codereview.chromium.org/2645313003/diff/320001/src/compiler/code-asse...
src/compiler/code-assembler.cc:190: Node* CodeAssembler::CStringConstant(const
char* str) {
On 2017/02/17 14:56:10, caitp wrote:
> On 2017/02/17 12:28:23, jgruber wrote:
> > Just an idea - maybe it'd make sense to add an option to create an
> internalized
> > string. That can be important e.g. if we use it for property loads.
> 
> I think most of the time in those situations we end up using string constants
> from heap-symbols.h, but maybe it would be more readable to do it that way.
> 
> How would you imagine writing it? `CStringConstant("foo", true); to
internalize
> "foo"? Or maybe someting more like `InternalizedCStringConstant("foo");`

Hmm, I guess both aren't really satisfying. This is probably a nonissue, I'm
fine with sticking to your current solution.

[neis, 2017-02-20 09:41:21]

Great tests. A few minor comments below.  Looking at the other files now.

https://codereview.chromium.org/2645313003/diff/240001/src/builtins/builtins-...
File src/builtins/builtins-async.h (right):

https://codereview.chromium.org/2645313003/diff/240001/src/builtins/builtins-...
src/builtins/builtins-async.h:18: // Async Iterator
I think you can drop this comment.

https://codereview.chromium.org/2645313003/diff/240001/test/mjsunit/harmony/a...
File test/mjsunit/harmony/async-from-sync-iterator.js (right):

https://codereview.chromium.org/2645313003/diff/240001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:20: throw error;
What's the point of catching and rethrowing here?

https://codereview.chromium.org/2645313003/diff/240001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:22:
assertUnreachable("generator is closed");
I was at first confused by this. Could you just replace it with "yield 5" or
something like that?

https://codereview.chromium.org/2645313003/diff/240001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:74: assertEquals({ value:
"return-after-completed", done: true }, iter_result);
You already tested .return above, didn't you?

https://codereview.chromium.org/2645313003/diff/240001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:75: 
Can you somehow separate the tests using gen (above) from those using sync
(below) and from those using neither at the very end?  Maybe have three async
functions at the toplevel.

https://codereview.chromium.org/2645313003/diff/240001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:98: function sync(array,
features, log = []) {
There shouldn't be a default value for log (see comment further down).

https://codereview.chromium.org/2645313003/diff/240001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:112: log.push({ method:
"throw", sent, value, done });
I think you can drop value here and in return below.

https://codereview.chromium.org/2645313003/diff/240001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:122: return { value, done };
Not sure why you use undefined as the value in return but sent as the value in
throw.

https://codereview.chromium.org/2645313003/diff/240001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:134: iter =
%CreateAsyncFromSyncIterator(sync(["sync-value"], 0));
You forgot to pass the log.

https://codereview.chromium.org/2645313003/diff/240001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:176:
assertUnreachable('Expected `iter.next(\'c\') to throw, but did not throw');
It's the await that throws, not the iter.next call (same in some other cases
below).

https://codereview.chromium.org/2645313003/diff/240001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:257: }
Can you also test the case where the result of the sync .return is not an
object?

And where the iterator value produced by the sync .return is a rejected promise
(here: the argument to .return is a rejected promise).

https://codereview.chromium.org/2645313003/diff/240001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:278: // the sent value.
Do you mean "does not exist" here?

https://codereview.chromium.org/2645313003/diff/240001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:331: log = [];
A function call cannot return a "normal" completion.

https://codereview.chromium.org/2645313003/diff/240001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:437: }
Could you please add
- a test where the sync iterator's next/throw/return returns an object whose
"value" property is a promise that is (or becomes) resolved/rejected,
- a test where the sync iterator's next/throw/return returns a non-object.

[neis, 2017-02-20 11:27:59]

https://codereview.chromium.org/2645313003/diff/340001/src/builtins/builtins-...
File src/builtins/builtins-async-iterator.cc (right):

https://codereview.chromium.org/2645313003/diff/340001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:82: GotoIf(TaggedIsSmi(iter_result),
&if_slowpath);
I think LoadIteratorResult should either assume that iter_result is a JSReceiver
or throw a TypeError if it isn't.  See also the comment in .next.

https://codereview.chromium.org/2645313003/diff/340001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:137: Node*
AsyncFromSyncBuiltinsAssembler::CreateUnwrapClosure(Node* native_context,
It's funny that they call this "unwrap" function.

https://codereview.chromium.org/2645313003/diff/340001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:156: Node* const parameter =
Parameter(1);
nit: call this "value", then it will also match the spec comments.

https://codereview.chromium.org/2645313003/diff/340001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:181: 
This seems to be missing the Object check of IteratorNext.  (Same in .throw and
.return I think.)  See also the comment in LoadIteratorResult.

https://codereview.chromium.org/2645313003/diff/340001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:197: Node* const on_resolve =
CreateUnwrapClosure(native_context, done);
nit: s/on_resolve/on_fulfilled/

https://codereview.chromium.org/2645313003/diff/340001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:245: Variable var_done(this,
MachineRepresentation::kTagged);
These seem unnecessary, see below.

https://codereview.chromium.org/2645313003/diff/340001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:282: 
You should get rid of var_value, var_done, resolve_promise, and then merge the
two blocks.

https://codereview.chromium.org/2645313003/diff/340001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:305: }
Could you try to factor out this code?  Seems to basically be the same in all
three functions.

https://codereview.chromium.org/2645313003/diff/340001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:341: //
IfAbruptRejectPromise(nextResult, promiseCapability).
This shouldn't be here.

https://codereview.chromium.org/2645313003/diff/340001/src/interpreter/interp...
File src/interpreter/interpreter-intrinsics.cc (right):

https://codereview.chromium.org/2645313003/diff/340001/src/interpreter/interp...
src/interpreter/interpreter-intrinsics.cc:312: __ GotoIf(__
TaggedIsSmi(sync_iterator), &not_receiver);
IsJSReceiver already does this.

https://codereview.chromium.org/2645313003/diff/340001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2645313003/diff/340001/src/objects.h#newcode1...
src/objects.h:10432: class JSAsyncFromSyncIterator : public JSObject {
Please add a comment here saying what these objects are and pointing to the spec
draft.

https://codereview.chromium.org/2645313003/diff/340001/src/objects.h#newcode1...
src/objects.h:10435: 
nit: remove empty line.

[caitp, 2017-02-20 17:09:35]

I think I've covered all of these --- rebased version up shortly

https://codereview.chromium.org/2645313003/diff/240001/src/builtins/builtins-...
File src/builtins/builtins-async.h (right):

https://codereview.chromium.org/2645313003/diff/240001/src/builtins/builtins-...
src/builtins/builtins-async.h:18: // Async Iterator
On 2017/02/20 09:41:20, neis wrote:
> I think you can drop this comment.

Done. I actually removed all changes to builtins-async.h since none of this
needs to be shared with the other users

https://codereview.chromium.org/2645313003/diff/240001/test/mjsunit/harmony/a...
File test/mjsunit/harmony/async-from-sync-iterator.js (right):

https://codereview.chromium.org/2645313003/diff/240001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:20: throw error;
On 2017/02/20 09:41:20, neis wrote:
> What's the point of catching and rethrowing here?

it's a basic control flow test. The .throw call gets us here rather than to
line22, and rethrowing verifies that the .throw() promise is rejected.

https://codereview.chromium.org/2645313003/diff/240001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:22:
assertUnreachable("generator is closed");
On 2017/02/20 09:41:20, neis wrote:
> I was at first confused by this. Could you just replace it with "yield 5" or
> something like that?

I'm not entirely sure what you mean --- after the uncaught `throw` on line 20,
it should be impossible to resume the generator again, because the generator
should be closed. This is part of the "test that async-from-sync delegates to
sync-iterator" behaviour, since it's the sync iterator which is deciding not to
resume execution.

https://codereview.chromium.org/2645313003/diff/240001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:74: assertEquals({ value:
"return-after-completed", done: true }, iter_result);
On 2017/02/20 09:41:20, neis wrote:
> You already tested .return above, didn't you?

it's good to have an extra one thrown in as it helps to ensure that we didn't
accidentally restart the generator, but I can remove it

https://codereview.chromium.org/2645313003/diff/240001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:75: 
On 2017/02/20 09:41:20, neis wrote:
> Can you somehow separate the tests using gen (above) from those using sync
> (below) and from those using neither at the very end?  Maybe have three async
> functions at the toplevel.

Acknowledged.

https://codereview.chromium.org/2645313003/diff/240001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:98: function sync(array,
features, log = []) {
On 2017/02/20 09:41:20, neis wrote:
> There shouldn't be a default value for log (see comment further down).

Removed the default value, and added a %AbortJS() call if it's not supplied
(tests still passed even without passing it, presumably because the exception
was eaten)

https://codereview.chromium.org/2645313003/diff/240001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:112: log.push({ method:
"throw", sent, value, done });
On 2017/02/20 09:41:20, neis wrote:
> I think you can drop value here and in return below.

Done.

https://codereview.chromium.org/2645313003/diff/240001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:122: return { value, done };
On 2017/02/20 09:41:20, neis wrote:
> Not sure why you use undefined as the value in return but sent as the value in
> throw.

It's pretty inconsequential to the test either way, I'll make them the same

https://codereview.chromium.org/2645313003/diff/240001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:134: iter =
%CreateAsyncFromSyncIterator(sync(["sync-value"], 0));
On 2017/02/20 09:41:20, neis wrote:
> You forgot to pass the log.

Good catch, thx

https://codereview.chromium.org/2645313003/diff/240001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:176:
assertUnreachable('Expected `iter.next(\'c\') to throw, but did not throw');
On 2017/02/20 09:41:20, neis wrote:
> It's the await that throws, not the iter.next call (same in some other cases
> below).

throw/reject are admittedly being used inferchangeably, is that confusing?

https://codereview.chromium.org/2645313003/diff/240001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:278: // the sent value.
On 2017/02/20 09:41:20, neis wrote:
> Do you mean "does not exist" here?

"if [sync_iterator] does not have a .throw method"

https://codereview.chromium.org/2645313003/diff/340001/src/builtins/builtins-...
File src/builtins/builtins-async-iterator.cc (right):

https://codereview.chromium.org/2645313003/diff/340001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:156: Node* const parameter =
Parameter(1);
On 2017/02/20 11:27:59, neis wrote:
> nit: call this "value", then it will also match the spec comments.

I opted to call it `parameter` so that I could use the `Node* const` convention
everyhere, as `value` is used below for the iter_result property name.

How about `sent_value`?

https://codereview.chromium.org/2645313003/diff/340001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:181: 
On 2017/02/20 11:27:58, neis wrote:
> This seems to be missing the Object check of IteratorNext.  (Same in .throw
and
> .return I think.)  See also the comment in LoadIteratorResult.

Added the object check in LoadIteratorResult().

https://codereview.chromium.org/2645313003/diff/340001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:197: Node* const on_resolve =
CreateUnwrapClosure(native_context, done);
On 2017/02/20 11:27:59, neis wrote:
> nit: s/on_resolve/on_fulfilled/

done

https://codereview.chromium.org/2645313003/diff/340001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:282: 
On 2017/02/20 11:27:59, neis wrote:
> You should get rid of var_value, var_done, resolve_promise, and then merge the
> two blocks.

Done.

https://codereview.chromium.org/2645313003/diff/340001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:305: }
On 2017/02/20 11:27:59, neis wrote:
> Could you try to factor out this code?  Seems to basically be the same in all
> three functions.

Done.

https://codereview.chromium.org/2645313003/diff/340001/src/interpreter/interp...
File src/interpreter/interpreter-intrinsics.cc (right):

https://codereview.chromium.org/2645313003/diff/340001/src/interpreter/interp...
src/interpreter/interpreter-intrinsics.cc:312: __ GotoIf(__
TaggedIsSmi(sync_iterator), &not_receiver);
On 2017/02/20 11:27:59, neis wrote:
> IsJSReceiver already does this.

```
Node* CodeStubAssembler::IsJSReceiver(Node* object) {
  STATIC_ASSERT(LAST_JS_OBJECT_TYPE == LAST_TYPE);
  return IsJSReceiverInstanceType(LoadInstanceType(object));
}
```

This looks unsafe if {object} is an Smi.

https://codereview.chromium.org/2645313003/diff/340001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2645313003/diff/340001/src/objects.h#newcode1...
src/objects.h:10432: class JSAsyncFromSyncIterator : public JSObject {
On 2017/02/20 11:27:59, neis wrote:
> Please add a comment here saying what these objects are and pointing to the
spec
> draft.

Done.

https://codereview.chromium.org/2645313003/diff/340001/src/objects.h#newcode1...
src/objects.h:10435: 
On 2017/02/20 11:27:59, neis wrote:
> nit: remove empty line.

Done.

[caitp, 2017-02-20 17:19:51]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-20 17:19:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-20 17:46:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-20 17:46:07]

Dry run: This issue passed the CQ dry run.

[neis, 2017-02-20 18:26:54]

https://codereview.chromium.org/2645313003/diff/340001/src/interpreter/interp...
File src/interpreter/interpreter-intrinsics.cc (right):

https://codereview.chromium.org/2645313003/diff/340001/src/interpreter/interp...
src/interpreter/interpreter-intrinsics.cc:312: __ GotoIf(__
TaggedIsSmi(sync_iterator), &not_receiver);
On 2017/02/20 17:09:35, caitp wrote:
> On 2017/02/20 11:27:59, neis wrote:
> > IsJSReceiver already does this.
> 
> ```
> Node* CodeStubAssembler::IsJSReceiver(Node* object) {
>   STATIC_ASSERT(LAST_JS_OBJECT_TYPE == LAST_TYPE);
>   return IsJSReceiverInstanceType(LoadInstanceType(object));
> }
> ```
> 
> This looks unsafe if {object} is an Smi.

Sorry, I thought you were using the version defined in this file.

[caitp, 2017-02-20 20:09:20]

https://codereview.chromium.org/2645313003/diff/340001/src/interpreter/interp...
File src/interpreter/interpreter-intrinsics.cc (right):

https://codereview.chromium.org/2645313003/diff/340001/src/interpreter/interp...
src/interpreter/interpreter-intrinsics.cc:312: __ GotoIf(__
TaggedIsSmi(sync_iterator), &not_receiver);
On 2017/02/20 18:26:54, neis wrote:
> On 2017/02/20 17:09:35, caitp wrote:
> > On 2017/02/20 11:27:59, neis wrote:
> > > IsJSReceiver already does this.
> > 
> > ```
> > Node* CodeStubAssembler::IsJSReceiver(Node* object) {
> >   STATIC_ASSERT(LAST_JS_OBJECT_TYPE == LAST_TYPE);
> >   return IsJSReceiverInstanceType(LoadInstanceType(object));
> > }
> > ```
> > 
> > This looks unsafe if {object} is an Smi.
> 
> Sorry, I thought you were using the version defined in this file.

Acknowledged.

[neis, 2017-02-21 12:48:09]

lgtm modulo the assertUnreachable and the second
Generate_AsyncFromSyncIteratorMethod (see below). Thanks!

https://codereview.chromium.org/2645313003/diff/240001/test/mjsunit/harmony/a...
File test/mjsunit/harmony/async-from-sync-iterator.js (right):

https://codereview.chromium.org/2645313003/diff/240001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:20: throw error;
On 2017/02/20 17:09:34, caitp wrote:
> On 2017/02/20 09:41:20, neis wrote:
> > What's the point of catching and rethrowing here?
> 
> it's a basic control flow test. The .throw call gets us here rather than to
> line22, and rethrowing verifies that the .throw() promise is rejected.

I don't get your point, but feel free to keep it as is.

https://codereview.chromium.org/2645313003/diff/240001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:22:
assertUnreachable("generator is closed");
On 2017/02/20 17:09:35, caitp wrote:
> On 2017/02/20 09:41:20, neis wrote:
> > I was at first confused by this. Could you just replace it with "yield 5" or
> > something like that?
> 
> I'm not entirely sure what you mean --- after the uncaught `throw` on line 20,
> it should be impossible to resume the generator again, because the generator
> should be closed. This is part of the "test that async-from-sync delegates to
> sync-iterator" behaviour, since it's the sync iterator which is deciding not
to
> resume execution.

What confused me is that this line is not unreachable per se, it's just that the
current set of tests doesn't request more than two elements from the iterator.

https://codereview.chromium.org/2645313003/diff/340001/src/builtins/builtins-...
File src/builtins/builtins-async-iterator.cc (right):

https://codereview.chromium.org/2645313003/diff/340001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:156: Node* const parameter =
Parameter(1);
On 2017/02/20 17:09:35, caitp wrote:
> On 2017/02/20 11:27:59, neis wrote:
> > nit: call this "value", then it will also match the spec comments.
> 
> I opted to call it `parameter` so that I could use the `Node* const`
convention
> everyhere, as `value` is used below for the iter_result property name.
> 
> How about `sent_value`?

Fine too.

https://codereview.chromium.org/2645313003/diff/360001/src/builtins/builtins-...
File src/builtins/builtins-async-iterator.cc (right):

https://codereview.chromium.org/2645313003/diff/360001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:51: }
I think it's clearer if you remove the second one and just inline it inside
AsyncFromSyncIteratorPrototypeNext.

https://codereview.chromium.org/2645313003/diff/360001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:302: "[Async-from-Sync
Iterator].prototype.return");
Nice!

https://codereview.chromium.org/2645313003/diff/360001/test/mjsunit/harmony/a...
File test/mjsunit/harmony/async-from-sync-iterator.js (right):

https://codereview.chromium.org/2645313003/diff/360001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:73: if (log === void 0)
%AbortJS("`log` is undefined");
Nit: move this up.

https://codereview.chromium.org/2645313003/diff/360001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:263: kNext|kNextUnchanged,
log));
Nit: kNextUnchanged includes kNext.

https://codereview.chromium.org/2645313003/diff/360001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:650: f().catch(function() {
assertUnreachable("No error should have occurred"); });
This assertUnreachable would go unnoticed.

[caitp, 2017-02-21 14:36:53]

https://codereview.chromium.org/2645313003/diff/240001/test/mjsunit/harmony/a...
File test/mjsunit/harmony/async-from-sync-iterator.js (right):

https://codereview.chromium.org/2645313003/diff/240001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:22:
assertUnreachable("generator is closed");
On 2017/02/21 12:48:08, neis wrote:
> On 2017/02/20 17:09:35, caitp wrote:
> > On 2017/02/20 09:41:20, neis wrote:
> > > I was at first confused by this. Could you just replace it with "yield 5"
or
> > > something like that?
> > 
> > I'm not entirely sure what you mean --- after the uncaught `throw` on line
20,
> > it should be impossible to resume the generator again, because the generator
> > should be closed. This is part of the "test that async-from-sync delegates
to
> > sync-iterator" behaviour, since it's the sync iterator which is deciding not
> to
> > resume execution.
> 
> What confused me is that this line is not unreachable per se, it's just that
the
> current set of tests doesn't request more than two elements from the iterator.

It is unreachable, because the throw on line 20 results in the generator being
closed, so subsequent requests never resume the generator

https://codereview.chromium.org/2645313003/diff/360001/src/builtins/builtins-...
File src/builtins/builtins-async-iterator.cc (right):

https://codereview.chromium.org/2645313003/diff/360001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:51: }
On 2017/02/21 12:48:08, neis wrote:
> I think it's clearer if you remove the second one and just inline it inside
> AsyncFromSyncIteratorPrototypeNext.

Done.

https://codereview.chromium.org/2645313003/diff/360001/test/mjsunit/harmony/a...
File test/mjsunit/harmony/async-from-sync-iterator.js (right):

https://codereview.chromium.org/2645313003/diff/360001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:73: if (log === void 0)
%AbortJS("`log` is undefined");
On 2017/02/21 12:48:09, neis wrote:
> Nit: move this up.

Acknowledged.

https://codereview.chromium.org/2645313003/diff/360001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:263: kNext|kNextUnchanged,
log));
On 2017/02/21 12:48:09, neis wrote:
> Nit: kNextUnchanged includes kNext.

Acknowledged.

https://codereview.chromium.org/2645313003/diff/360001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-from-sync-iterator.js:650: f().catch(function() {
assertUnreachable("No error should have occurred"); });
On 2017/02/21 12:48:09, neis wrote:
> This assertUnreachable would go unnoticed.

Good point, using %AbortJS instead.

[caitp, 2017-02-23 23:56:33]

Ahh, so, are people generally happy with the approach in this CL? Let me know
and I'll land this tomorrow and move on to addressing the async generator stuff
and adding test cases for it.

[caitp, 2017-02-23 23:58:43]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 23:58:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 00:31:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 00:31:29]

Dry run: This issue passed the CQ dry run.

[caitp, 2017-02-24 17:38:37]

On 2017/02/23 23:56:33, caitp wrote:
> Ahh, so, are people generally happy with the approach in this CL? Let me know
> and I'll land this tomorrow and move on to addressing the async generator
stuff
> and adding test cases for it.

Alright, gonna let this fly and start finishing up this project

[caitp, 2017-02-24 17:38:50]

The CQ bit was checked by caitp@igalia.com

[caitp, 2017-02-24 17:38:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, jgruber@chromium.org, neis@chromium.org
Link to the patchset: https://codereview.chromium.org/2645313003/#ps420001
(title: "cleanmerge")

[commit-bot: I haz the power, 2017-02-24 17:39:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 17:44:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 17:44:51]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/35461)

[caitp, 2017-02-24 17:46:34]

Description was changed from

==========
[async-iteration] implement Async-from-Sync Iterator

Introduce a new Object to allow GetIterator("async") to function when the
iterable does not have a Symbol.asyncIterator method.

This patch has been split out from https://codereview.chromium.org/2622833002/
and incorporates test cases.

BUG=v8:5855, v8:4483
R=adamk@chromium.org, littledan@chromium.org, gsathya@chromium.org
==========

to

==========
[async-iteration] implement Async-from-Sync Iterator

Introduce a new Object to allow GetIterator("async") to function when the
iterable does not have a Symbol.asyncIterator method.

This patch has been split out from https://codereview.chromium.org/2622833002/
and incorporates test cases.

BUG=v8:5855, v8:4483
R=adamk@chromium.org, littledan@chromium.org, gsathya@chromium.org
TBR=hpayer@chromium.org, bmeurer@chromium.org
==========

[caitp, 2017-02-24 17:46:57]

Description was changed from

==========
[async-iteration] implement Async-from-Sync Iterator

Introduce a new Object to allow GetIterator("async") to function when the
iterable does not have a Symbol.asyncIterator method.

This patch has been split out from https://codereview.chromium.org/2622833002/
and incorporates test cases.

BUG=v8:5855, v8:4483
R=adamk@chromium.org, littledan@chromium.org, gsathya@chromium.org
TBR=hpayer@chromium.org, bmeurer@chromium.org
==========

to

==========
[async-iteration] implement Async-from-Sync Iterator

Introduce a new Object to allow GetIterator("async") to function when the
iterable does not have a Symbol.asyncIterator method.

This patch has been split out from https://codereview.chromium.org/2622833002/
and incorporates test cases.

BUG=v8:5855, v8:4483
R=jgruber@chromium.org, rmcilroy@chromium.org, neis@chromium.org
TBR=hpayer@chromium.org, bmeurer@chromium.org
==========

[caitp, 2017-02-24 17:47:07]

The CQ bit was checked by caitp@igalia.com

[commit-bot: I haz the power, 2017-02-24 17:47:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 17:48:51]

CQ is committing da patch.

Bot data: {"patchset_id": 420001, "attempt_start_ts": 1487958427500980,
"parent_rev": "8cfe45b6f1880008d09d4310458b6e6fcb0522dd", "commit_rev":
"0423341034c767d91128c9fd68e3f51e60359c1c"}

[commit-bot: I haz the power, 2017-02-24 17:48:58]

Description was changed from

==========
[async-iteration] implement Async-from-Sync Iterator

Introduce a new Object to allow GetIterator("async") to function when the
iterable does not have a Symbol.asyncIterator method.

This patch has been split out from https://codereview.chromium.org/2622833002/
and incorporates test cases.

BUG=v8:5855, v8:4483
R=jgruber@chromium.org, rmcilroy@chromium.org, neis@chromium.org
TBR=hpayer@chromium.org, bmeurer@chromium.org
==========

to

==========
[async-iteration] implement Async-from-Sync Iterator

Introduce a new Object to allow GetIterator("async") to function when the
iterable does not have a Symbol.asyncIterator method.

This patch has been split out from https://codereview.chromium.org/2622833002/
and incorporates test cases.

BUG=v8:5855, v8:4483
R=jgruber@chromium.org, rmcilroy@chromium.org, neis@chromium.org
TBR=hpayer@chromium.org, bmeurer@chromium.org

Review-Url: https://codereview.chromium.org/2645313003
Cr-Commit-Position: refs/heads/master@{#43419}
Committed:
https://chromium.googlesource.com/v8/v8/+/0423341034c767d91128c9fd68e3f51e603...
==========

[commit-bot: I haz the power, 2017-02-24 17:49:00]

Committed patchset #22 (id:420001) as
https://chromium.googlesource.com/v8/v8/+/0423341034c767d91128c9fd68e3f51e603...