Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2637403008:
[async-iteration] add support for for-await-of loops in Async Functions (Closed)
Description[async-iteration] add support for for-await-of loops in Async Functions
When --harmony-async-iteration is enabled, it is now possible to
use the for-await-of loop, which uses the Async Iteration protocol
rather than the ordinary ES6 Iteration protocol.
the Async-from-Sync Iterator object is not implemented in this CL,
and so for-await-of loops will abort execution if the iterated object
does not have a Symbol.asyncIterator() method. Async-from-Sync
Iterators are implemented seperately in https://codereview.chromium.org/2645313003/
BUG=v8:5855, v8:4483
R=neis@chromium.org, littledan@chromium.org, adamk@chromium.org
Review-Url: https://codereview.chromium.org/2637403008
Cr-Commit-Position: refs/heads/master@{#43224}
Committed: https://chromium.googlesource.com/v8/v8/+/76ab55e3d37f31e1f343ad35c293404bfd259457
Patch Set 1 #
Total comments: 43
Patch Set 2 : some requested changes #Patch Set 3 : edit some comments #
Total comments: 2
Patch Set 4 : add parser tests, fix parser code and fix mjsunit tests #Patch Set 5 : fix compiler error on trybots #Patch Set 6 : fix win32 compiler error #Patch Set 7 : rebase #
Total comments: 4
Patch Set 8 : Add iterable TDZ tests, create iterable TDZ block, and fix style in the test #Patch Set 9 : fix copyright line in for-await-of.js #Patch Set 10 : [async-iteration] add support for for-await-of loops in Async Functions #
Total comments: 8
Patch Set 11 : more tests, fix nits #Patch Set 12 : rebase #Patch Set 13 : remove changes to ParserTarget / PreParserTarget #
Total comments: 1
Patch Set 14 : remove comment, and refactor tests a bit (and enable a test that works correctly) #Patch Set 15 : Rebase now that try/finally is fixed #
Total comments: 1
Patch Set 16 : ...and uncomment the previously failing tests... #
Total comments: 17
Patch Set 17 : change ParseScopedStatement call for clarity, add some tests, and other bits #Patch Set 18 : remove that comment #
Messages
Total messages: 88 (45 generated)
Hey, This CL includes for-await-of components of https://codereview.chromium.org/2622833002/ by themselves, with a bit of test coverage (more before landing). Currently, this is broken with lexical declarations, because the CreateForEachStatementTDZ() line tries to re-declare variables already declared by ParseVariableDeclarations() a few lines above, and it's hard to sort out why (might be broken in the other CL too, dunno). So, I'd love a hand with sorting out the frontend issues, and then some help verifying that the new AsyncIteratorClose algorithm is doing the right thing everywhere.
rmcilroy@chromium.org changed reviewers: + rmcilroy@chromium.org
Took a quick look at the interpreter parts, a couple of suggestions but that part looks good overall to me. https://codereview.chromium.org/2637403008/diff/1/src/interpreter/bytecode-ge... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/2637403008/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:2915: FeedbackVectorSlot call_slot2 = expr->AsyncIteratorCallFeedbackSlot(); nit - could you name these async_load_slot, etc. https://codereview.chromium.org/2637403008/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:2916: Register tmp = register_allocator()->NewRegister(); Unused https://codereview.chromium.org/2637403008/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:2923: builder()->JumpIfNull(&async_iterator_null); You could use TestUndetectable, JumpIfTrue here instead of two jumps. You could also add a TODO to add a JumpIfUndetectable bytecode itself if it's useful in a followup CL. https://codereview.chromium.org/2637403008/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:2947: Register sync_iterator = args[0]; No need to allocate a register list for a single register, just pass allocate sync_iterator with NewRegister and pass it into the overload for CallRuntime. https://codereview.chromium.org/2637403008/diff/1/src/interpreter/interpreter... File src/interpreter/interpreter-intrinsics.cc (right): https://codereview.chromium.org/2637403008/diff/1/src/interpreter/interpreter... src/interpreter/interpreter-intrinsics.cc:407: __ NoContextConstant()); Could we avoid adding this as an intrinsic that just calls a runtime call and just call the runtime function itself?
Hi Caitlin, here's a first round of comments. I'm not very familiar with the code in parser-base, please let Adam and Dan have a close look at that. https://codereview.chromium.org/2637403008/diff/1/src/ast/ast.h File src/ast/ast.h (right): https://codereview.chromium.org/2637403008/diff/1/src/ast/ast.h#newcode2970 src/ast/ast.h:2970: enum Hint { kNormal, kAsync }; Can you instead use the IteratorType enum that you use in the parser? Maybe move it here. https://codereview.chromium.org/2637403008/diff/1/src/interpreter/bytecode-ge... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/2637403008/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:2919: obj, async_iterator_symbol(), feedback_index(load_slot2)); Nit: please format in the same style as below. https://codereview.chromium.org/2637403008/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:2927: method, args, feedback_index(call_slot), Call::NAMED_PROPERTY_CALL); Shouldn't this be call_slot2 here? And in the Call below call_slot instead of call_slot2? https://codereview.chromium.org/2637403008/diff/1/src/interpreter/interpreter... File src/interpreter/interpreter-intrinsics.cc (right): https://codereview.chromium.org/2637403008/diff/1/src/interpreter/interpreter... src/interpreter/interpreter-intrinsics.cc:407: __ NoContextConstant()); If this will be implemented in a separate CL, please note that in the description of this CL. https://codereview.chromium.org/2637403008/diff/1/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2637403008/diff/1/src/parsing/parser.cc#newco... src/parsing/parser.cc:1759: // %ThrowIteratorResultNotAnObject(result) Comment needs an update. https://codereview.chromium.org/2637403008/diff/1/src/parsing/parser.cc#newco... src/parsing/parser.cc:1979: Comment needs an update. https://codereview.chromium.org/2637403008/diff/1/src/parsing/parser.cc#newco... src/parsing/parser.cc:2089: Statement* Parser::InitializeForAwaitOfStatement(ForEachStatement* for_each, Can't you just parameterize the existing InitializeForOfStatement over IteratorType? https://codereview.chromium.org/2637403008/diff/1/src/parsing/parser.cc#newco... src/parsing/parser.cc:2110: // iterator = GetIterator(iterable) Comment needs an update. https://codereview.chromium.org/2637403008/diff/1/src/parsing/parser.cc#newco... src/parsing/parser.cc:2121: // %ThrowIteratorResultNotAnObject(result) Comment needs an update. https://codereview.chromium.org/2637403008/diff/1/src/parsing/parser.cc#newco... src/parsing/parser.cc:4843: // This function is only used in RewriteYieldStar and doesn't need to be touched in this CL. https://codereview.chromium.org/2637403008/diff/1/src/parsing/parser.cc#newco... src/parsing/parser.cc:5086: // Comment needs an update. https://codereview.chromium.org/2637403008/diff/1/src/parsing/parser.cc#newco... src/parsing/parser.cc:5115: // try { %_Call(iteratorReturn, iterator) } catch (_) { } Comment needs an update. https://codereview.chromium.org/2637403008/diff/1/src/parsing/parser.cc#newco... src/parsing/parser.cc:5163: } We must also do await on the result of this call. https://codereview.chromium.org/2637403008/diff/1/src/parsing/parser.cc#newco... src/parsing/parser.cc:5243: // #BuildIteratorCloseForCompletion(#iterator, completion) Comment needs an update. https://codereview.chromium.org/2637403008/diff/1/src/runtime/runtime-interna... File src/runtime/runtime-internal.cc (right): https://codereview.chromium.org/2637403008/diff/1/src/runtime/runtime-interna... src/runtime/runtime-internal.cc:230: DCHECK(args.length() == 0); DCHECK_EQ https://codereview.chromium.org/2637403008/diff/1/test/mjsunit/harmony/for-aw... File test/mjsunit/harmony/for-await-of.js (right): https://codereview.chromium.org/2637403008/diff/1/test/mjsunit/harmony/for-aw... test/mjsunit/harmony/for-await-of.js:5: // Flags: --harmony-async-iteration --allow-natives-syntax --no-lazy Why do you require --no-lazy? https://codereview.chromium.org/2637403008/diff/1/test/mjsunit/harmony/for-aw... test/mjsunit/harmony/for-await-of.js:21: next() { The formatting here seems odd. https://codereview.chromium.org/2637403008/diff/1/test/mjsunit/harmony/for-aw... test/mjsunit/harmony/for-await-of.js:33: let test; nit: add blank line https://codereview.chromium.org/2637403008/diff/1/test/mjsunit/harmony/for-aw... test/mjsunit/harmony/for-await-of.js:170: // Ensure strict mode applies to block with use-strict-directive Do you mean "does not apply"? Maybe replace the comment and body with let strict = (function() { return this === undefined; })(); assertFalse(strict); (Also in similar tests above.) https://codereview.chromium.org/2637403008/diff/1/test/mjsunit/harmony/for-aw... test/mjsunit/harmony/for-await-of.js:528: } Great tests! Please add some that exercise the iterator closing stuff. And later also some that test the case of GetIterator where Symbol.asyncIterator doesn't exist.
Thanks, will go over these comments in more detail shortly https://codereview.chromium.org/2637403008/diff/1/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2637403008/diff/1/src/parsing/parser.cc#newco... src/parsing/parser.cc:2089: Statement* Parser::InitializeForAwaitOfStatement(ForEachStatement* for_each, On 2017/01/20 14:38:15, neis wrote: > Can't you just parameterize the existing InitializeForOfStatement over > IteratorType? It's doable, but harder to read IMO . Whatever people want here i guess https://codereview.chromium.org/2637403008/diff/1/src/parsing/parser.cc#newco... src/parsing/parser.cc:4843: // On 2017/01/20 14:38:15, neis wrote: > This function is only used in RewriteYieldStar and doesn't need to be touched in > this CL. That may be a bug then. You're supposed to IteratorClose on break, based on my read of the spec "If LoopContinues(result, labelSet) is false, return ? IteratorClose(iterator, UpdateEmpty(result, V))." LoopContinues looks like it will return false on break continuations, thus performing IteratorClose
so, it seems like `throw` in for-await-of is broken, based on the last test I added (for some reason, the function returns normally). I think this is some problem with the BuildIteratorCloseForCompletion / FinalizeIteratorUse stuff, which seems to interact poorly with async functions right now. Or maybe I'm not Awaiting something that needs awaiting. https://codereview.chromium.org/2637403008/diff/1/src/interpreter/bytecode-ge... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/2637403008/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:2915: FeedbackVectorSlot call_slot2 = expr->AsyncIteratorCallFeedbackSlot(); On 2017/01/20 10:34:01, rmcilroy wrote: > nit - could you name these async_load_slot, etc. done https://codereview.chromium.org/2637403008/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:2916: Register tmp = register_allocator()->NewRegister(); On 2017/01/20 10:34:01, rmcilroy wrote: > Unused It's gone https://codereview.chromium.org/2637403008/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:2923: builder()->JumpIfNull(&async_iterator_null); On 2017/01/20 10:34:01, rmcilroy wrote: > You could use TestUndetectable, JumpIfTrue here instead of two jumps. You could > also add a TODO to add a JumpIfUndetectable bytecode itself if it's useful in a > followup CL. TestUndetectable checks for the Undetectable map bit (e.g. for HTMLAllCollection, used to make `document.all == undefined` evaluate to true even though it's not). All this bytecode seems to do is check fi the operand is a HeapObject with the map bit set. But, that's not the logic that makes sense in this case. A JumpIfNullOrUndefined opcode would be convenient, though. This pattern is used elsewhere in the bytecode-generator, see VisitForInStatement https://codereview.chromium.org/2637403008/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:2927: method, args, feedback_index(call_slot), Call::NAMED_PROPERTY_CALL); On 2017/01/20 14:38:15, neis wrote: > Shouldn't this be call_slot2 here? And in the Call below call_slot instead of > call_slot2? Renamed these things to `load_iter_slot`, `call_iter_slot`, `load_async_iter_slot`, and `call_async_iter_slot`, and use them in the right places. https://codereview.chromium.org/2637403008/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:2947: Register sync_iterator = args[0]; On 2017/01/20 10:34:01, rmcilroy wrote: > No need to allocate a register list for a single register, just pass allocate > sync_iterator with NewRegister and pass it into the overload for CallRuntime. Something like this? ``` // Return CreateAsyncFromSyncIterator(syncIterator) // alias `method` register as it's no longer used Register sync_iter = method; builder() ->StoreAccumulatorInRegister(sync_iter) .CallRuntime(Runtime::kInlineCreateAsyncFromSyncIterator, sync_iter); ``` or just allocate a new register? https://codereview.chromium.org/2637403008/diff/1/src/interpreter/interpreter... File src/interpreter/interpreter-intrinsics.cc (right): https://codereview.chromium.org/2637403008/diff/1/src/interpreter/interpreter... src/interpreter/interpreter-intrinsics.cc:407: __ NoContextConstant()); On 2017/01/20 14:38:15, neis wrote: > If this will be implemented in a separate CL, please note that in the > description of this CL. I would have thought the TODO() indicated this. I think, for now, I'll leave the interpreter intrinsics untouched and just call the runtime directly from bytecode-generator, as rmcilroy is asking https://codereview.chromium.org/2637403008/diff/1/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2637403008/diff/1/src/parsing/parser.cc#newco... src/parsing/parser.cc:2089: Statement* Parser::InitializeForAwaitOfStatement(ForEachStatement* for_each, On 2017/01/20 17:05:34, caitp wrote: > On 2017/01/20 14:38:15, neis wrote: > > Can't you just parameterize the existing InitializeForOfStatement over > > IteratorType? > > It's doable, but harder to read IMO > . Whatever people want here i guess Eh, it actually gets pretty ugly to try to do it this way. The preparser doesn't know about ForOfStatements, so the patch would have to teach it about them to do that. Otherwise, InitializeForOfStatement would need to be changed to accept a ForEachStatement as its parameter. Either way, those changes are unrelated, and reduce type safety a bit. Far as I can see, the only real alternative is to alter the ForEachStatement ast node to include the VisitMode in the ForEachStatement node itself, so that it knows which initialize method to invoke from InitializeForEachStatement. I'll include the simplest change I guess. https://codereview.chromium.org/2637403008/diff/1/test/mjsunit/harmony/for-aw... File test/mjsunit/harmony/for-await-of.js (right): https://codereview.chromium.org/2637403008/diff/1/test/mjsunit/harmony/for-aw... test/mjsunit/harmony/for-await-of.js:5: // Flags: --harmony-async-iteration --allow-natives-syntax --no-lazy On 2017/01/20 14:38:16, neis wrote: > Why do you require --no-lazy? This was from debugging the issue with lexical variables (there's a syntax error on the first test which uses a lexical declaration in the loop head, because the desugaring tries to declare it twice for some reason). The --no-lazy will disappear, but it does for the time being make it easier to debug that problem. Need parser experts to help figure that out https://codereview.chromium.org/2637403008/diff/1/test/mjsunit/harmony/for-aw... test/mjsunit/harmony/for-await-of.js:21: next() { On 2017/01/20 14:38:16, neis wrote: > The formatting here seems odd. I think clang-format doesn't really know how to deal with modern JavaScript. I'll manually format these later I guess. https://codereview.chromium.org/2637403008/diff/1/test/mjsunit/harmony/for-aw... test/mjsunit/harmony/for-await-of.js:33: let test; On 2017/01/20 14:38:16, neis wrote: > nit: add blank line Done. https://codereview.chromium.org/2637403008/diff/1/test/mjsunit/harmony/for-aw... test/mjsunit/harmony/for-await-of.js:170: // Ensure strict mode applies to block with use-strict-directive On 2017/01/20 14:38:16, neis wrote: > Do you mean "does not apply"? > > Maybe replace the comment and body with > > let strict = (function() { return this === undefined; })(); > assertFalse(strict); > > (Also in similar tests above.) sgtm, done https://codereview.chromium.org/2637403008/diff/1/test/mjsunit/harmony/for-aw... test/mjsunit/harmony/for-await-of.js:528: } On 2017/01/20 14:38:16, neis wrote: > Great tests! > > Please add some that exercise the iterator closing stuff. > > And later also some that test the case of GetIterator where Symbol.asyncIterator > doesn't exist. I think the tests where "Symbol.asyncIterator" isn't present will have to wait, since currently the bytecode generator will call a runtime function which aborts execution when that happens (I'm okay with this since the feature is behind a flag). That will all happen when the Async-from-Sync Iterator stuff lands, I guess. About the IteratorClose stuff, I'm not fully aware of all of the interactions that for-await-of should have with it. It's basically just testing that it gets called in a specific order when break, throw or return happens, right? If that's all it is, I'll add some new tests shortly.
The CQ bit was checked by caitp@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/3...)
The CQ bit was checked by caitp@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/3...) v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/21361)
The CQ bit was checked by caitp@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
caitp@igalia.com changed reviewers: + marja@chromium.org, nikolaos@chromium.org
I guess throw works just fine, and it was rather an error in the test, not an error in the desugaring. So that's good news. I've added a bunch of new syntax tests in test-parsing.cc. https://codereview.chromium.org/2637403008/diff/40001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2637403008/diff/40001/src/parsing/parser-base... src/parsing/parser-base.h:5649: impl()->CreateForEachStatementTDZ(impl()->NullBlock(), for_info, ok); Nickie / Marja: can you explain why CreateForEachStatementTDZ seems to work in ForOf statements? It looks like it always redeclares variables that were declared previously in ParseVariableDeclaration (looks like the same scope in both functions), which is always an error for lexical variables. Removing it seems to make this work as expected, although it might be broken with eval(), not sure.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by caitp@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2637403008/diff/40001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2637403008/diff/40001/src/parsing/parser-base... src/parsing/parser-base.h:5649: impl()->CreateForEachStatementTDZ(impl()->NullBlock(), for_info, ok); On 2017/01/21 16:50:33, caitp wrote: > Nickie / Marja: can you explain why CreateForEachStatementTDZ seems to work in > ForOf statements? > > It looks like it always redeclares variables that were declared previously in > ParseVariableDeclaration (looks like the same scope in both functions), which is > always an error for lexical variables. Removing it seems to make this work as > expected, although it might be broken with eval(), not sure. Since I didn't know the answer upfront, I used the opportunity to poke the code a bit to understand it better. Here's how ParseForStatement works: - The variable is indeed declared twice, but not in the same scope, but two different scopes: - ParseVariableDeclarations doesn't call DeclareAndInitializeVariables, since it's a for context. - The first declaration is done by DesugarBindingInForEachStatement and a couple of lines above that you see a BlockState which creates a new scope, so, it declares the variable in that scope. - The second declaration is done by CreateForEachStatementTDZ, and it uses the parent scope (like explained in the comment above Parser::DesugarBindingInForEachStatement). Your problem seems to be that you're calling both DesugarBindingInForEachStatement and CreateForEachStatementTDZ in the same scope (not creating a new BlockState for the former).
could you also add tests for the tdz? https://codereview.chromium.org/2637403008/diff/120001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2637403008/diff/120001/src/parsing/parser-bas... src/parsing/parser-base.h:5641: template <typename Impl> This is quite a lot of code duplication with ParseForStatement. There are differences between what's allowed for for-each statement and what's allowed for for await, but are those differences substantial enough that this function need to be duplicated? E.g., for async(... in ...) is not allowed, but what are the other differences that makes it not feasible to use ParseForStatement? (ParseForStatement is quite big and difficult to understand though as I just found out, trying to answer your question.. :) )
On 2017/01/23 05:34:10, marja wrote: > https://codereview.chromium.org/2637403008/diff/40001/src/parsing/parser-base.h > File src/parsing/parser-base.h (right): > > https://codereview.chromium.org/2637403008/diff/40001/src/parsing/parser-base... > src/parsing/parser-base.h:5649: > impl()->CreateForEachStatementTDZ(impl()->NullBlock(), for_info, ok); > On 2017/01/21 16:50:33, caitp wrote: > > Nickie / Marja: can you explain why CreateForEachStatementTDZ seems to work in > > ForOf statements? > > > > It looks like it always redeclares variables that were declared previously in > > ParseVariableDeclaration (looks like the same scope in both functions), which > is > > always an error for lexical variables. Removing it seems to make this work as > > expected, although it might be broken with eval(), not sure. > > Since I didn't know the answer upfront, I used the opportunity to poke the code > a bit to understand it better. Here's how ParseForStatement works: > > - The variable is indeed declared twice, but not in the same scope, but two > different scopes: > - ParseVariableDeclarations doesn't call DeclareAndInitializeVariables, since > it's a for context. > - The first declaration is done by DesugarBindingInForEachStatement and a couple > of lines above that you see a BlockState which creates a new scope, so, it > declares the variable in that scope. > - The second declaration is done by CreateForEachStatementTDZ, and it uses the > parent scope (like explained in the comment above > Parser::DesugarBindingInForEachStatement). > > Your problem seems to be that you're calling both > DesugarBindingInForEachStatement and CreateForEachStatementTDZ in the same scope > (not creating a new BlockState for the former). It's probably worth adding a comment to explain this, thanks for taking the time. I walked through the code very carefully and did not see a new scope / BlockState, but I suppose i missed it. On 2017/01/23 05:50:02, marja wrote: > could you also add tests for the tdz? Absolutely https://codereview.chromium.org/2637403008/diff/120001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2637403008/diff/120001/src/parsing/parser-bas... src/parsing/parser-base.h:5641: template <typename Impl> On 2017/01/23 05:50:02, marja wrote: > (ParseForStatement is quite big and difficult to understand though as I just > found out, trying to answer your question.. :) ) This is the main reason for the different function, it's much, much easier to understand this way, for me, and probably others.
On 2017/01/20 14:38:16, neis wrote: > https://codereview.chromium.org/2637403008/diff/1/src/parsing/parser.cc#newco... > src/parsing/parser.cc:5163: } > We must also do await on the result of this call. This is still missing in the latest patch set. Please also add tests for that.
Hmm, yeah, for-await-of seems to be much more restricted wrt what is allowed, whereas ParseForStatement tries to handle all possible cases in one function and is pretty scary. It's a trade off - e.g., some features like the tdz you could've gotten for free if you'd integrated to ParseForStatement. That said, I'm not objecting this approach either and I'll look into refactoring ParseForStatement so that it's less scary. Maybe we can unify them in the future again.
Sorry for not taking part in this earlier. I was only involved in the last refactoring of ParseForStatement and Marja already answered the question about CreateForEachStatementTDZ --- thanks again, it would have taken me at least the same time to go through this and try to remember/understand the details. ParseForStatement (all of its 250 lines) is quite ugly and if anybody can come up with a better refactoring, that will be great. The problem as I see it is that it tries to handle many different cases of loops. So, I think Caitlin did the right thing, separating ParseForAwaitStatement. Here the syntax helps. I wish this could be done so easily for the other cases of loops, to further split ParseForStatement, but unfortunately the syntax is mixed there. Identifying common code between ParseForStatement and ParseForAwaitStatement and refactoring will definitely be a step in the right direction.
I've added tests and re-added the the CreateForEachStatementTDZ() call, in the correct scope. Seems to work as expected now https://codereview.chromium.org/2637403008/diff/1/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2637403008/diff/1/src/parsing/parser.cc#newco... src/parsing/parser.cc:5163: } On 2017/01/20 14:38:15, neis wrote: > We must also do await on the result of this call. Done.
Georg asked me to take a look at this, but is the plan to rebase this on Marja's refactor after that lands?
On 2017/01/23 19:25:18, adamk wrote: > Georg asked me to take a look at this, but is the plan to rebase this on Marja's > refactor after that lands? I wasn't planning to refactor based on that immediately, but maybe in a followup. I expect it will involve adding a new VisitMode to the ForEachStatement, and modifying the behaviour of ParseForEachWith/WithoutDeclarations based on the VisitMode flag.
The CQ bit was checked by caitp@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by caitp@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/23 19:28:43, caitp wrote: > On 2017/01/23 19:25:18, adamk wrote: > > Georg asked me to take a look at this, but is the plan to rebase this on > Marja's > > refactor after that lands? > > I wasn't planning to refactor based on that immediately, but maybe in a > followup. I expect it will involve adding a new VisitMode to the > ForEachStatement, and modifying the behaviour of > ParseForEachWith/WithoutDeclarations based on the VisitMode flag. I landed the refactoring; maybe you'd like to have a look at whether it's better to rebase your work on it now, or whether ParseForStatement is still too tedious to work with.
https://codereview.chromium.org/2637403008/diff/120001/test/mjsunit/harmony/f... File test/mjsunit/harmony/for-await-of.js (right): https://codereview.chromium.org/2637403008/diff/120001/test/mjsunit/harmony/f... test/mjsunit/harmony/for-await-of.js:330: // Ensure strict mode applies to block with use-strict-directive Fix or remove comment. https://codereview.chromium.org/2637403008/diff/180001/src/ast/ast.h File src/ast/ast.h (right): https://codereview.chromium.org/2637403008/diff/180001/src/ast/ast.h#newcode2979 src/ast/ast.h:2979: using Hint = IteratorType; I think it's fine to use IteratorType directly below. https://codereview.chromium.org/2637403008/diff/180001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2637403008/diff/180001/src/parsing/parser.cc#... src/parsing/parser.cc:4879: Comment needs an update. https://codereview.chromium.org/2637403008/diff/180001/src/parsing/parser.h File src/parsing/parser.h (right): https://codereview.chromium.org/2637403008/diff/180001/src/parsing/parser.h#n... src/parsing/parser.h:458: // %ThrowIteratorResultNotAnObject(result) Comment needs an update. https://codereview.chromium.org/2637403008/diff/180001/src/runtime/runtime-in... File src/runtime/runtime-internal.cc (right): https://codereview.chromium.org/2637403008/diff/180001/src/runtime/runtime-in... src/runtime/runtime-internal.cc:230: DCHECK(args.length() == 0); DCHECK_EQ
Could you add a few tests that would have caught the missing await? Maybe by making the return method use Promise.resolve().then(...) so that the await is necessary for the stuff to be written to the log. Also one test should trigger the exception path where the awaited result is not an object. Apart from this and the minor things in the previous mail, I'm pretty happy with the CL.
https://codereview.chromium.org/2637403008/diff/1/src/interpreter/interpreter... File src/interpreter/interpreter-intrinsics.cc (right): https://codereview.chromium.org/2637403008/diff/1/src/interpreter/interpreter... src/interpreter/interpreter-intrinsics.cc:407: __ NoContextConstant()); On 2017/01/20 20:17:19, caitp wrote: > On 2017/01/20 14:38:15, neis wrote: > > If this will be implemented in a separate CL, please note that in the > > description of this CL. > > I would have thought the TODO() indicated this. Sure, but many people read the CL description while only a few read the code.
Description was changed from ========== [async-iteration] add support for for-await-of loops in Async Functions When --harmony-async-iteration is enabled, it is now possible to use the for-await-of loop, which uses the Async Iteration protocol rather than the ordinary ES6 Iteration protocol. BUG=v8:5855, v8:4483 R=neis@chromium.org, littledan@chromium.org, adamk@chromium.org ========== to ========== [async-iteration] add support for for-await-of loops in Async Functions When --harmony-async-iteration is enabled, it is now possible to use the for-await-of loop, which uses the Async Iteration protocol rather than the ordinary ES6 Iteration protocol. the Async-from-Sync Iterator object is not implemented in this CL, and so for-await-of loops will abort execution if the iterated object does not have a Symbol.asyncIterator() method. Async-from-Sync Iterators are implemented seperately in https://codereview.chromium.org/2645313003/ BUG=v8:5855, v8:4483 R=neis@chromium.org, littledan@chromium.org, adamk@chromium.org ==========
Description was changed from ========== [async-iteration] add support for for-await-of loops in Async Functions When --harmony-async-iteration is enabled, it is now possible to use the for-await-of loop, which uses the Async Iteration protocol rather than the ordinary ES6 Iteration protocol. the Async-from-Sync Iterator object is not implemented in this CL, and so for-await-of loops will abort execution if the iterated object does not have a Symbol.asyncIterator() method. Async-from-Sync Iterators are implemented seperately in https://codereview.chromium.org/2645313003/ BUG=v8:5855, v8:4483 R=neis@chromium.org, littledan@chromium.org, adamk@chromium.org ========== to ========== [async-iteration] add support for for-await-of loops in Async Functions When --harmony-async-iteration is enabled, it is now possible to use the for-await-of loop, which uses the Async Iteration protocol rather than the ordinary ES6 Iteration protocol. the Async-from-Sync Iterator object is not implemented in this CL, and so for-await-of loops will abort execution if the iterated object does not have a Symbol.asyncIterator() method. Async-from-Sync Iterators are implemented seperately in https://codereview.chromium.org/2645313003/ BUG=v8:5855, v8:4483 R=neis@chromium.org, littledan@chromium.org, adamk@chromium.org ==========
On 2017/01/24 12:53:01, neis wrote:
> Could you add a few tests that would have caught the missing await? Maybe by
> making the return method use Promise.resolve().then(...) so that the await is
> necessary for the stuff to be written to the log. Also one test should trigger
> the exception path where the awaited result is not an object.
>
> Apart from this and the minor things in the previous mail, I'm pretty happy
with
> the CL.
There's a bit of a problem with AsyncIteratorClose and return statements in
loops.
in async functions, the return statement is desugared to just resolve the
Promise allocated at the start of the method, and this happens before
IteratorClose actually occurs. So, even though IteratorClose does happen, and is
awaited, it's awaited after promise resolution has actually happened, and thus
is unable to cause the async function Promise to be rejected.
This is reproducible via the following simple d8 test:
```
(async function() {
let testDone = false;
async function test() {
let collection = [1, 2, 3, 4, 5];
let sync_iter = collection[Symbol.iterator]();
let sum = 0;
let async_iter = {
[Symbol.asyncIterator]() { return this; },
next() {
return Promise.resolve(sync_iter.next());
},
return() {
return Promise.resolve({
value: new Promise(function(resolve, reject) {
Promise.resolve().then(function() {
testDone = true;
print(".iterator.return() -> reject Promise")
reject("return! " + sum);
});
}),
done: true
});
}
};
let i = 0;
for await (let x of async_iter) {
sum += x;
if (++i === 2) {
print("'return sum' -> resolve Promise");
return sum;
}
}
}
try {
await test();
} catch (e) {
if (!testDone)
throw new Error("did not Await .return().value\n" + e.stack);
throw e;
}
throw new Error("Promise should be rejected");
})().then(undefined, function(error) {
print(error.toString());
});
```
This test logs the following:
```
'return sum' -> resolve Promise
.iterator.return() -> reject Promise
Error: Promise should be rejected
```
The AST indicates that the Promise is fulfilled very early, before the
IteratorClose occurs, and though the IteratorClose is awaited, it's not possible
to reject the already-fulfilled Promise.
To get this right, there needs to be a way to postpone resolution of the Promise
until after the IteratorClose algorithm is completed.
On 2017/01/24 19:34:46, caitp wrote:
> On 2017/01/24 12:53:01, neis wrote:
> > Could you add a few tests that would have caught the missing await? Maybe by
> > making the return method use Promise.resolve().then(...) so that the await
is
> > necessary for the stuff to be written to the log. Also one test should
trigger
> > the exception path where the awaited result is not an object.
> >
> > Apart from this and the minor things in the previous mail, I'm pretty happy
> with
> > the CL.
>
> There's a bit of a problem with AsyncIteratorClose and return statements in
> loops.
>
> in async functions, the return statement is desugared to just resolve the
> Promise allocated at the start of the method, and this happens before
> IteratorClose actually occurs. So, even though IteratorClose does happen, and
is
> awaited, it's awaited after promise resolution has actually happened, and thus
> is unable to cause the async function Promise to be rejected.
>
> This is reproducible via the following simple d8 test:
>
> ```
> (async function() {
> let testDone = false;
> async function test() {
> let collection = [1, 2, 3, 4, 5];
> let sync_iter = collection[Symbol.iterator]();
> let sum = 0;
> let async_iter = {
> [Symbol.asyncIterator]() { return this; },
> next() {
> return Promise.resolve(sync_iter.next());
> },
> return() {
> return Promise.resolve({
> value: new Promise(function(resolve, reject) {
> Promise.resolve().then(function() {
> testDone = true;
> print(".iterator.return() -> reject Promise")
> reject("return! " + sum);
> });
> }),
> done: true
> });
> }
> };
>
> let i = 0;
> for await (let x of async_iter) {
> sum += x;
> if (++i === 2) {
> print("'return sum' -> resolve Promise");
> return sum;
> }
> }
> }
>
> try {
> await test();
> } catch (e) {
> if (!testDone)
> throw new Error("did not Await .return().value\n" + e.stack);
> throw e;
> }
> throw new Error("Promise should be rejected");
> })().then(undefined, function(error) {
> print(error.toString());
> });
> ```
>
> This test logs the following:
>
> ```
> 'return sum' -> resolve Promise
> .iterator.return() -> reject Promise
> Error: Promise should be rejected
> ```
>
> The AST indicates that the Promise is fulfilled very early, before the
> IteratorClose occurs, and though the IteratorClose is awaited, it's not
possible
> to reject the already-fulfilled Promise.
>
> To get this right, there needs to be a way to postpone resolution of the
Promise
> until after the IteratorClose algorithm is completed.
Maybe it would make sense to do all of this desugaring in BytecodeGenerator,
rather than via the AST. It looks very challenging to fix the ReturnStatement
rewriting, since it gets desugared 3 different ways depending on various
factors.
But, doing the desugaring in BytecodeGenerator is outside of the scope of this
CL. would it be alright to just have a disabled failing test upstream with a
TODO to fix this?
think I've addressed all of those comments, and added a lot of new tests. So as noted above, some of those tests fail due to interacting poorly with the current AST desugaring of return and IteratorClose in async functions. I believe all of the tests would behave incorrectly if there were nested for-of/for-await-of loops, at the very least in the case of return and throw statements. https://codereview.chromium.org/2637403008/diff/120001/test/mjsunit/harmony/f... File test/mjsunit/harmony/for-await-of.js (right): https://codereview.chromium.org/2637403008/diff/120001/test/mjsunit/harmony/f... test/mjsunit/harmony/for-await-of.js:330: // Ensure strict mode applies to block with use-strict-directive On 2017/01/24 12:49:19, neis wrote: > Fix or remove comment. removed https://codereview.chromium.org/2637403008/diff/180001/src/ast/ast.h File src/ast/ast.h (right): https://codereview.chromium.org/2637403008/diff/180001/src/ast/ast.h#newcode2979 src/ast/ast.h:2979: using Hint = IteratorType; On 2017/01/24 12:49:19, neis wrote: > I think it's fine to use IteratorType directly below. done https://codereview.chromium.org/2637403008/diff/180001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2637403008/diff/180001/src/parsing/parser.cc#... src/parsing/parser.cc:4879: On 2017/01/24 12:49:19, neis wrote: > Comment needs an update. done https://codereview.chromium.org/2637403008/diff/180001/src/parsing/parser.h File src/parsing/parser.h (right): https://codereview.chromium.org/2637403008/diff/180001/src/parsing/parser.h#n... src/parsing/parser.h:458: // %ThrowIteratorResultNotAnObject(result) On 2017/01/24 12:49:19, neis wrote: > Comment needs an update. done https://codereview.chromium.org/2637403008/diff/180001/src/runtime/runtime-in... File src/runtime/runtime-internal.cc (right): https://codereview.chromium.org/2637403008/diff/180001/src/runtime/runtime-in... src/runtime/runtime-internal.cc:230: DCHECK(args.length() == 0); On 2017/01/24 12:49:19, neis wrote: > DCHECK_EQ Done.
Interpreter changes LGTM, but could you add a test snippet to test-bytecode-generator to get bytecode expectations for async iteration. https://codereview.chromium.org/2637403008/diff/1/src/interpreter/bytecode-ge... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/2637403008/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:2923: builder()->JumpIfNull(&async_iterator_null); On 2017/01/20 20:17:19, caitp wrote: > On 2017/01/20 10:34:01, rmcilroy wrote: > > You could use TestUndetectable, JumpIfTrue here instead of two jumps. You > could > > also add a TODO to add a JumpIfUndetectable bytecode itself if it's useful in > a > > followup CL. > > TestUndetectable checks for the Undetectable map bit (e.g. for > HTMLAllCollection, used to make `document.all == undefined` evaluate to true > even though it's not). All this bytecode seems to do is check fi the operand is > a HeapObject with the map bit set. > > But, that's not the logic that makes sense in this case. A JumpIfNullOrUndefined > opcode would be convenient, though. This pattern is used elsewhere in the > bytecode-generator, see VisitForInStatement Ahh make sense, I forgot about the weird document.all behaviour. Could you add a TODO about adding a JumpIfNullOrUndefined bytecode, thanks. https://codereview.chromium.org/2637403008/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:2947: Register sync_iterator = args[0]; On 2017/01/20 20:17:19, caitp wrote: > On 2017/01/20 10:34:01, rmcilroy wrote: > > No need to allocate a register list for a single register, just pass allocate > > sync_iterator with NewRegister and pass it into the overload for CallRuntime. > > Something like this? > > ``` > // Return CreateAsyncFromSyncIterator(syncIterator) > // alias `method` register as it's no longer used > Register sync_iter = method; > builder() > ->StoreAccumulatorInRegister(sync_iter) > .CallRuntime(Runtime::kInlineCreateAsyncFromSyncIterator, sync_iter); > ``` Yeah this is fine > or just allocate a new register?
On 2017/01/24 19:34:46, caitp wrote:
> On 2017/01/24 12:53:01, neis wrote:
> > Could you add a few tests that would have caught the missing await? Maybe by
> > making the return method use Promise.resolve().then(...) so that the await
is
> > necessary for the stuff to be written to the log. Also one test should
trigger
> > the exception path where the awaited result is not an object.
> >
> > Apart from this and the minor things in the previous mail, I'm pretty happy
> with
> > the CL.
>
> There's a bit of a problem with AsyncIteratorClose and return statements in
> loops.
>
> in async functions, the return statement is desugared to just resolve the
> Promise allocated at the start of the method, and this happens before
> IteratorClose actually occurs. So, even though IteratorClose does happen, and
is
> awaited, it's awaited after promise resolution has actually happened, and thus
> is unable to cause the async function Promise to be rejected.
>
> This is reproducible via the following simple d8 test:
>
> ```
> (async function() {
> let testDone = false;
> async function test() {
> let collection = [1, 2, 3, 4, 5];
> let sync_iter = collection[Symbol.iterator]();
> let sum = 0;
> let async_iter = {
> [Symbol.asyncIterator]() { return this; },
> next() {
> return Promise.resolve(sync_iter.next());
> },
> return() {
> return Promise.resolve({
> value: new Promise(function(resolve, reject) {
> Promise.resolve().then(function() {
> testDone = true;
> print(".iterator.return() -> reject Promise")
> reject("return! " + sum);
> });
> }),
> done: true
> });
> }
> };
>
> let i = 0;
> for await (let x of async_iter) {
> sum += x;
> if (++i === 2) {
> print("'return sum' -> resolve Promise");
> return sum;
> }
> }
> }
>
> try {
> await test();
> } catch (e) {
> if (!testDone)
> throw new Error("did not Await .return().value\n" + e.stack);
> throw e;
> }
> throw new Error("Promise should be rejected");
> })().then(undefined, function(error) {
> print(error.toString());
> });
> ```
>
> This test logs the following:
>
> ```
> 'return sum' -> resolve Promise
> .iterator.return() -> reject Promise
> Error: Promise should be rejected
> ```
>
> The AST indicates that the Promise is fulfilled very early, before the
> IteratorClose occurs, and though the IteratorClose is awaited, it's not
possible
> to reject the already-fulfilled Promise.
>
> To get this right, there needs to be a way to postpone resolution of the
Promise
> until after the IteratorClose algorithm is completed.
Hi Caitlin,
it's possible that there is indeed a problem but I don't understand your test.
We are not supposed to await on async_iter.result().value, but on
async_iter.result().
I wanted to play around with your patch but I can't find a commit that it applies to. Could you rebase again? THanks!
On 2017/01/25 10:38:45, neis wrote:
> On 2017/01/24 19:34:46, caitp wrote:
> > On 2017/01/24 12:53:01, neis wrote:
> > > Could you add a few tests that would have caught the missing await? Maybe
by
> > > making the return method use Promise.resolve().then(...) so that the await
> is
> > > necessary for the stuff to be written to the log. Also one test should
> trigger
> > > the exception path where the awaited result is not an object.
> > >
> > > Apart from this and the minor things in the previous mail, I'm pretty
happy
> > with
> > > the CL.
> >
> > There's a bit of a problem with AsyncIteratorClose and return statements in
> > loops.
> >
> > in async functions, the return statement is desugared to just resolve the
> > Promise allocated at the start of the method, and this happens before
> > IteratorClose actually occurs. So, even though IteratorClose does happen,
and
> is
> > awaited, it's awaited after promise resolution has actually happened, and
thus
> > is unable to cause the async function Promise to be rejected.
> >
> > This is reproducible via the following simple d8 test:
> >
> > ```
> > (async function() {
> > let testDone = false;
> > async function test() {
> > let collection = [1, 2, 3, 4, 5];
> > let sync_iter = collection[Symbol.iterator]();
> > let sum = 0;
> > let async_iter = {
> > [Symbol.asyncIterator]() { return this; },
> > next() {
> > return Promise.resolve(sync_iter.next());
> > },
> > return() {
> > return Promise.resolve({
> > value: new Promise(function(resolve, reject) {
> > Promise.resolve().then(function() {
> > testDone = true;
> > print(".iterator.return() -> reject Promise")
> > reject("return! " + sum);
> > });
> > }),
> > done: true
> > });
> > }
> > };
> >
> > let i = 0;
> > for await (let x of async_iter) {
> > sum += x;
> > if (++i === 2) {
> > print("'return sum' -> resolve Promise");
> > return sum;
> > }
> > }
> > }
> >
> > try {
> > await test();
> > } catch (e) {
> > if (!testDone)
> > throw new Error("did not Await .return().value\n" + e.stack);
> > throw e;
> > }
> > throw new Error("Promise should be rejected");
> > })().then(undefined, function(error) {
> > print(error.toString());
> > });
> > ```
> >
> > This test logs the following:
> >
> > ```
> > 'return sum' -> resolve Promise
> > .iterator.return() -> reject Promise
> > Error: Promise should be rejected
> > ```
> >
> > The AST indicates that the Promise is fulfilled very early, before the
> > IteratorClose occurs, and though the IteratorClose is awaited, it's not
> possible
> > to reject the already-fulfilled Promise.
> >
> > To get this right, there needs to be a way to postpone resolution of the
> Promise
> > until after the IteratorClose algorithm is completed.
>
> Hi Caitlin,
>
> it's possible that there is indeed a problem but I don't understand your test.
> We are not supposed to await on async_iter.result().value, but on
> async_iter.result().
Async-from-Sync Iterator will await on async_iter.return().value, by awaiting
the result of the internal [sync_iter].return().value. The test case sort of
emulates that.
On 2017/01/25 10:55:39, neis wrote:
> I wanted to play around with your patch but I can't find a commit that it
> applies to. Could you rebase again? THanks!
One moment
https://codereview.chromium.org/2637403008/diff/240001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2637403008/diff/240001/src/parsing/parser.cc#... src/parsing/parser.cc:4654: // if (!IS_RECEIVER(output)) %ThrowIterResultNotAnObject(output); Please undo the comment changes here since the code doesn't (yet) change.
The CQ bit was checked by caitp@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Now that the try/finally bug has been fixed, all of the new tests are passing. PTAL.
https://codereview.chromium.org/2637403008/diff/270001/src/interpreter/consta... File src/interpreter/constant-array-builder.cc (right): https://codereview.chromium.org/2637403008/diff/270001/src/interpreter/consta... src/interpreter/constant-array-builder.cc:326: #undef ENTRY_LOOKUP +leszeks@chromium.org << does this seem like a reasonable way to do this? It might be nicer than making changes to this file every time a new per-isolate constant is needed, though I don't expect that to happen often.
caitp@igalia.com changed reviewers: + leszeks@chromium.org
The CQ bit was checked by caitp@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
parsing/* and ast/* LGTM (w/ nit) Please make sure you have LGTM for all files before committing. https://codereview.chromium.org/2637403008/diff/290001/src/parsing/pattern-re... File src/parsing/pattern-rewriter.cc (right): https://codereview.chromium.org/2637403008/diff/290001/src/parsing/pattern-re... src/parsing/pattern-rewriter.cc:435: IteratorType::kNormal, kNoSourcePosition)); Nit: would it make sense to add default params to GetNewIterator, so that you don't need to specify IteratorType:kNormal and kNoSourcePosition in all places?
I'll take another look today.
Parsing and AST look good, just a minor comment. https://codereview.chromium.org/2637403008/diff/290001/src/parsing/parser.h File src/parsing/parser.h (right): https://codereview.chromium.org/2637403008/diff/290001/src/parsing/parser.h#n... src/parsing/parser.h:473: Statement* InitializeForOfStatement(ForEachStatement* stmt, Expression* each, Maybe it's trivial, but it strikes me as odd that a method called "InitializeForOfStatement" now operates on a "ForEachStatement". (How does this relate to "InitializeForEachStatement"?) Looking at the changes in the function's implementation, where a DCHECK is used for making sure that the ForEachStatement is really a ForOfStatement, I'm wondering what was the purpose of this change.
https://codereview.chromium.org/2637403008/diff/290001/src/parsing/parser.h File src/parsing/parser.h (right): https://codereview.chromium.org/2637403008/diff/290001/src/parsing/parser.h#n... src/parsing/parser.h:473: Statement* InitializeForOfStatement(ForEachStatement* stmt, Expression* each, On 2017/02/14 13:35:24, nickie wrote: > Maybe it's trivial, but it strikes me as odd that a method called > "InitializeForOfStatement" now operates on a "ForEachStatement". (How does this > relate to "InitializeForEachStatement"?) > > Looking at the changes in the function's implementation, where a DCHECK is used > for making sure that the ForEachStatement is really a ForOfStatement, I'm > wondering what was the purpose of this change. The purpose was to allow InitializeForOfStatement to be invoked directly by the parser, rather than through InitializeForEachStatement(). There are some other ways to do this, but they'd involve more changes to ast.h, which would necessarily result in adding some unreachable switch cases to a few methods. https://codereview.chromium.org/2637403008/diff/290001/src/parsing/pattern-re... File src/parsing/pattern-rewriter.cc (right): https://codereview.chromium.org/2637403008/diff/290001/src/parsing/pattern-re... src/parsing/pattern-rewriter.cc:435: IteratorType::kNormal, kNoSourcePosition)); On 2017/02/14 09:30:43, marja wrote: > Nit: would it make sense to add default params to GetNewIterator, so that you > don't need to specify IteratorType:kNormal and kNoSourcePosition in all places? I felt it was better to be explicit about the type of iterator in all cases, but even if I didn't think so, once yield* support is added, all of these will use a variable iterator type.
lgtm
If my answers for those comments are satisfactory, I'd like to land this soon so I can move forward with the various async-iteration CLs. Let me know, thanks
I have a partial answer to Nikolaos's question, along with a few stylistic concerns and one spec question. https://codereview.chromium.org/2637403008/diff/290001/src/ast/ast.h File src/ast/ast.h (right): https://codereview.chromium.org/2637403008/diff/290001/src/ast/ast.h#newcode2967 src/ast/ast.h:2967: IteratorType hint() const { return hint_; } Naming nit: why not "type" instead of "hint"? https://codereview.chromium.org/2637403008/diff/290001/src/ast/ast.h#newcode2979 src/ast/ast.h:2979: // Can we just re-use the other slots? Looks like the answer to this is no? My read of the BytecodeGenerator is that both sets of slots are used there for the async case. Please remove the comment before landing; if it's still an open question, I'm curious why. https://codereview.chromium.org/2637403008/diff/290001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2637403008/diff/290001/src/parsing/parser-bas... src/parsing/parser-base.h:5761: if (!IsNextLetKeyword()) goto parse_lhs; This is the first goto in the parser. Given that it's also in side a switch statement, it seems overcomplicated, given that this is equivalent to: Token::Value tok = peek(); if (tok == Token::VAR || tok == Token::CONST || (tok == Token::LET && IsNextLetKeyword())) { // parse declaration } else { // parse lhs } https://codereview.chromium.org/2637403008/diff/290001/src/parsing/parser-bas... src/parsing/parser-base.h:5837: // For legacy compat reasons, give for loops similar treatment to Seems odd that new syntax still maintains this legacy compat stuff. Is this what the spec says? I see "labelled function declarations" and "function declarations in ifstatements" in Annex B, but I don't see where this production would happen. https://codereview.chromium.org/2637403008/diff/290001/src/parsing/parser.h File src/parsing/parser.h (right): https://codereview.chromium.org/2637403008/diff/290001/src/parsing/parser.h#n... src/parsing/parser.h:473: Statement* InitializeForOfStatement(ForEachStatement* stmt, Expression* each, On 2017/02/14 14:16:37, caitp wrote: > On 2017/02/14 13:35:24, nickie wrote: > > Maybe it's trivial, but it strikes me as odd that a method called > > "InitializeForOfStatement" now operates on a "ForEachStatement". (How does > this > > relate to "InitializeForEachStatement"?) > > > > Looking at the changes in the function's implementation, where a DCHECK is > used > > for making sure that the ForEachStatement is really a ForOfStatement, I'm > > wondering what was the purpose of this change. > > The purpose was to allow InitializeForOfStatement to be invoked directly by the > parser, rather than through InitializeForEachStatement(). > > There are some other ways to do this, but they'd involve more changes to ast.h, > which would necessarily result in adding some unreachable switch cases to a few > methods. I can think of a variety of solutions for this that don't involve updating any switch statements. For example, you could add a NewForOfStatement() to AstNodeFactory (and PreParserFactory). Or you could add an AsForOfStatement() method to PreParserStatement. The former already has another ideal caller in parser.cc, while the latter is quite minimal. https://codereview.chromium.org/2637403008/diff/290001/src/runtime/runtime.h File src/runtime/runtime.h (right): https://codereview.chromium.org/2637403008/diff/290001/src/runtime/runtime.h#... src/runtime/runtime.h:331: F(CreateAsyncFromSyncIterator, 1, 1) Nit: Besides AllowDynamicFunction, the rest of this list appears to be sorted.
https://codereview.chromium.org/2637403008/diff/290001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2637403008/diff/290001/src/parsing/parser-bas... src/parsing/parser-base.h:5761: if (!IsNextLetKeyword()) goto parse_lhs; On 2017/02/14 18:47:53, adamk wrote: > This is the first goto in the parser. Given that it's also in side a switch > statement, it seems overcomplicated, given that this is equivalent to: > > Token::Value tok = peek(); > if (tok == Token::VAR || tok == Token::CONST || (tok == Token::LET && > IsNextLetKeyword())) { > // parse declaration > } else { > // parse lhs > } Seems harder to read, to me, but alright https://codereview.chromium.org/2637403008/diff/290001/src/parsing/parser-bas... src/parsing/parser-base.h:5837: // For legacy compat reasons, give for loops similar treatment to On 2017/02/14 18:47:53, adamk wrote: > Seems odd that new syntax still maintains this legacy compat stuff. Is this what > the spec says? I see "labelled function declarations" and "function declarations > in ifstatements" in Annex B, but I don't see where this production would happen. The spec doesn't say anything about any of this, but this behaviour matches other loops. The behaviour seems to be "if a function declaration occurs outside of a StatementList, and not as the body of an If or Else, throw an exception" --- If the `legacy` parameter is false, then in sloppy mode, it will not throw an exception, and will block-scope the function instead. This seems really confusing, in that "legacy" seems to mean "disallow-legacy-behaviour".
The CQ bit was checked by caitp@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2637403008/diff/290001/src/ast/ast.h File src/ast/ast.h (right): https://codereview.chromium.org/2637403008/diff/290001/src/ast/ast.h#newcode2967 src/ast/ast.h:2967: IteratorType hint() const { return hint_; } On 2017/02/14 18:47:53, adamk wrote: > Naming nit: why not "type" instead of "hint"? This is the spec's name, see https://tc39.github.io/proposal-async-iteration/#sec-getiterator . Maybe it's following the pattern of ToPrimitive (which is also not quite a hint...)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/25 09:20:44, rmcilroy wrote: > Interpreter changes LGTM, but could you add a test snippet to > test-bytecode-generator to get bytecode expectations for async iteration. Doing this is a bit awkward in this CL. The UNREACHABLE() in Runtime_CreateAsyncFromSyncIterator gets hit in these tests due to CreateAsyncFromSyncIterator not being implemented here, maybe it makes sense to add these in https://codereview.chromium.org/2645313003/ instead. https://codereview.chromium.org/2637403008/diff/290001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2637403008/diff/290001/src/parsing/parser-bas... src/parsing/parser-base.h:5761: if (!IsNextLetKeyword()) goto parse_lhs; On 2017/02/14 18:47:53, adamk wrote: > This is the first goto in the parser. Given that it's also in side a switch > statement, it seems overcomplicated, given that this is equivalent to: > > Token::Value tok = peek(); > if (tok == Token::VAR || tok == Token::CONST || (tok == Token::LET && > IsNextLetKeyword())) { > // parse declaration > } else { > // parse lhs > } Done. https://codereview.chromium.org/2637403008/diff/290001/src/parsing/parser.h File src/parsing/parser.h (right): https://codereview.chromium.org/2637403008/diff/290001/src/parsing/parser.h#n... src/parsing/parser.h:473: Statement* InitializeForOfStatement(ForEachStatement* stmt, Expression* each, On 2017/02/14 18:47:53, adamk wrote: > On 2017/02/14 14:16:37, caitp wrote: > > On 2017/02/14 13:35:24, nickie wrote: > > > Maybe it's trivial, but it strikes me as odd that a method called > > > "InitializeForOfStatement" now operates on a "ForEachStatement". (How does > > this > > > relate to "InitializeForEachStatement"?) > > > > > > Looking at the changes in the function's implementation, where a DCHECK is > > used > > > for making sure that the ForEachStatement is really a ForOfStatement, I'm > > > wondering what was the purpose of this change. > > > > The purpose was to allow InitializeForOfStatement to be invoked directly by > the > > parser, rather than through InitializeForEachStatement(). > > > > There are some other ways to do this, but they'd involve more changes to > ast.h, > > which would necessarily result in adding some unreachable switch cases to a > few > > methods. > > I can think of a variety of solutions for this that don't involve updating any > switch statements. For example, you could add a NewForOfStatement() to > AstNodeFactory (and PreParserFactory). Or you could add an AsForOfStatement() > method to PreParserStatement. The former already has another ideal caller in > parser.cc, while the latter is quite minimal. Done via NewForOf... https://codereview.chromium.org/2637403008/diff/290001/src/runtime/runtime.h File src/runtime/runtime.h (right): https://codereview.chromium.org/2637403008/diff/290001/src/runtime/runtime.h#... src/runtime/runtime.h:331: F(CreateAsyncFromSyncIterator, 1, 1) On 2017/02/14 18:47:53, adamk wrote: > Nit: Besides AllowDynamicFunction, the rest of this list appears to be sorted. fixed sorting (Didn't touch AllowDynamicFunction though)
lgtm
...think that's the last of the comments. My understanding is that people are happy with the parser, ast and interpreter side, as well as the tests, so I'd like to throw the switch and CQ this. https://codereview.chromium.org/2637403008/diff/290001/src/ast/ast.h File src/ast/ast.h (right): https://codereview.chromium.org/2637403008/diff/290001/src/ast/ast.h#newcode2979 src/ast/ast.h:2979: // Can we just re-use the other slots? On 2017/02/14 18:47:53, adamk wrote: > Looks like the answer to this is no? My read of the BytecodeGenerator is that > both sets of slots are used there for the async case. Please remove the comment > before landing; if it's still an open question, I'm curious why. Done
The CQ bit was checked by caitp@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by caitp@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org, neis@chromium.org, marja@chromium.org, adamk@chromium.org Link to the patchset: https://codereview.chromium.org/2637403008/#ps330001 (title: "remove that comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 330001, "attempt_start_ts": 1487187435911300,
"parent_rev": "70105d5dde65d312b781089f95a595f8df85b4bd", "commit_rev":
"76ab55e3d37f31e1f343ad35c293404bfd259457"}
Message was sent while issue was closed.
Description was changed from ========== [async-iteration] add support for for-await-of loops in Async Functions When --harmony-async-iteration is enabled, it is now possible to use the for-await-of loop, which uses the Async Iteration protocol rather than the ordinary ES6 Iteration protocol. the Async-from-Sync Iterator object is not implemented in this CL, and so for-await-of loops will abort execution if the iterated object does not have a Symbol.asyncIterator() method. Async-from-Sync Iterators are implemented seperately in https://codereview.chromium.org/2645313003/ BUG=v8:5855, v8:4483 R=neis@chromium.org, littledan@chromium.org, adamk@chromium.org ========== to ========== [async-iteration] add support for for-await-of loops in Async Functions When --harmony-async-iteration is enabled, it is now possible to use the for-await-of loop, which uses the Async Iteration protocol rather than the ordinary ES6 Iteration protocol. the Async-from-Sync Iterator object is not implemented in this CL, and so for-await-of loops will abort execution if the iterated object does not have a Symbol.asyncIterator() method. Async-from-Sync Iterators are implemented seperately in https://codereview.chromium.org/2645313003/ BUG=v8:5855, v8:4483 R=neis@chromium.org, littledan@chromium.org, adamk@chromium.org Review-Url: https://codereview.chromium.org/2637403008 Cr-Commit-Position: refs/heads/master@{#43224} Committed: https://chromium.googlesource.com/v8/v8/+/76ab55e3d37f31e1f343ad35c293404bfd2... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:330001) as https://chromium.googlesource.com/v8/v8/+/76ab55e3d37f31e1f343ad35c293404bfd2... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
