Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
839323003:
Implementation of async-await transformation on js ast. (Closed)
|
Created:
5 years, 11 months ago by sigurdm Modified:
5 years, 10 months ago CC:
reviews_dartlang.org Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionImplementation of async-await transformation on js ast.
BUG=
R=floitsch@google.com, johnniwinther@google.com
Committed: https://code.google.com/p/dart/source/detail?r=43591
Patch Set 1 #Patch Set 2 : #
Total comments: 18
Patch Set 3 : Address comments, implement more stuff, more tests #Patch Set 4 : pre- and post-increment #Patch Set 5 : Most things should work, except async*. #Patch Set 6 : rebase #Patch Set 7 : Added a few tests #
Total comments: 8
Patch Set 8 : Implement new ssa-nodes in ssa-tracer. #
Total comments: 254
Patch Set 9 : Address Johnni's comments #Patch Set 10 : Small fixes to pass unittest #Patch Set 11 : Improvements to asyncstar support #Patch Set 12 : Address comments. Introduce async*. #Patch Set 13 : Removed change to sdk/lib/async/stream_impl #Patch Set 14 : Add missing functions to the MockCompiler's library #
Total comments: 6
Patch Set 15 : Undid changes to parentheses in tests now that Issue 22183 is solved. #Patch Set 16 : Go back to generating return of wrapped errors when possible. #
Total comments: 84
Patch Set 17 : Address Stephan's comments #Patch Set 18 : Removed the extra LocalsHandler #
Total comments: 49
Patch Set 19 : Address comments. Fix setting of the handler in finallies... #
Total comments: 118
Patch Set 20 : Introduced prefix to import of js, ran dartformat on rewrite_async.dart #Patch Set 21 : Address comments - add TODO for buggy error behaviour. #
Total comments: 16
Patch Set 22 : Fixed test. Addressed comments #Patch Set 23 : Rebase + fix issue with this-rebinding in sync*. #
Total comments: 10
Patch Set 24 : Address comments #Messages
Total messages: 35 (8 generated)
floitsch@google.com changed reviewers: + floitsch@google.com
DBC. (didn't really did a thorough review, but globally looks good.) https://codereview.chromium.org/839323003/diff/20001/pkg/compiler/lib/src/js/... File pkg/compiler/lib/src/js/rewrite_async.dart (right): https://codereview.chromium.org/839323003/diff/20001/pkg/compiler/lib/src/js/... pkg/compiler/lib/src/js/rewrite_async.dart:51: bool result = node.accept(this); don't call it "result". https://codereview.chromium.org/839323003/diff/20001/pkg/compiler/lib/src/js/... pkg/compiler/lib/src/js/rewrite_async.dart:71: // TODO: implement visitArrayHole should be trivial. There is no expression in the hole, and it doesn't have a side-effect. https://codereview.chromium.org/839323003/diff/20001/pkg/compiler/lib/src/js/... pkg/compiler/lib/src/js/rewrite_async.dart:81: bool x = visit(node.leftHandSide); x -> left y -> right https://codereview.chromium.org/839323003/diff/20001/pkg/compiler/lib/src/js/... pkg/compiler/lib/src/js/rewrite_async.dart:200: loopsAndSwitches.add(node); I would add the loopsAndSwitches only for the body. It doesn't make a difference, since the init, condition and update can not have a break/continue anyways, but still. https://codereview.chromium.org/839323003/diff/20001/pkg/compiler/lib/src/js/... pkg/compiler/lib/src/js/rewrite_async.dart:476: /// The order is important, therefore the type is explicitly LinkedHashMap. New line before. https://codereview.chromium.org/839323003/diff/20001/pkg/compiler/lib/src/js/... pkg/compiler/lib/src/js/rewrite_async.dart:546: bool transform(Node node) { shouldTransform https://codereview.chromium.org/839323003/diff/20001/pkg/compiler/lib/src/js/... pkg/compiler/lib/src/js/rewrite_async.dart:575: if (result is Literal) return result; I guess this will be wrong, once you have list-literals that contain side-effects. https://codereview.chromium.org/839323003/diff/20001/pkg/compiler/lib/src/js/... pkg/compiler/lib/src/js/rewrite_async.dart:657: helperBody = new Try(new Block([helperBody]), consider using the `js` templates. You will actually end up reusing the template, and it's easier to read. https://codereview.chromium.org/839323003/diff/20001/pkg/compiler/lib/src/js/... pkg/compiler/lib/src/js/rewrite_async.dart:718: throw("The transformer should never meet: $node"); If the function is intendend to stay, make it have a better error-message. Start with: throw "Internal error: trying to visit $node"; (If the error is encountered by real users, they don't want to hear that the node is not unreachable. It clearly is, otherwise they wouldn't see the error). Eventually it should throw a real exception and/or the element that caused the issue.
pre- and post-increment
sigurdm@google.com changed reviewers: + herhut@google.com, johnniwinther@google.com, kmillikin@google.com
Johnni: could you look at the registration/enqueueing part. Stephan: could you look at the type inference + changes to the ssa builder and type-stuff. The async* is not working yet (specifically canceling the stream). It will either be removed or finished before commit. https://codereview.chromium.org/839323003/diff/20001/pkg/compiler/lib/src/js/... File pkg/compiler/lib/src/js/rewrite_async.dart (right): https://codereview.chromium.org/839323003/diff/20001/pkg/compiler/lib/src/js/... pkg/compiler/lib/src/js/rewrite_async.dart:51: bool result = node.accept(this); On 2015/01/20 16:06:15, floitsch wrote: > don't call it "result". Called it containsAwait https://codereview.chromium.org/839323003/diff/20001/pkg/compiler/lib/src/js/... pkg/compiler/lib/src/js/rewrite_async.dart:71: // TODO: implement visitArrayHole On 2015/01/20 16:06:15, floitsch wrote: > should be trivial. There is no expression in the hole, and it doesn't have a > side-effect. Done. https://codereview.chromium.org/839323003/diff/20001/pkg/compiler/lib/src/js/... pkg/compiler/lib/src/js/rewrite_async.dart:81: bool x = visit(node.leftHandSide); On 2015/01/20 16:06:15, floitsch wrote: > x -> left > y -> right Done. https://codereview.chromium.org/839323003/diff/20001/pkg/compiler/lib/src/js/... pkg/compiler/lib/src/js/rewrite_async.dart:200: loopsAndSwitches.add(node); On 2015/01/20 16:06:15, floitsch wrote: > I would add the loopsAndSwitches only for the body. > It doesn't make a difference, since the init, condition and update can not have > a break/continue anyways, but still. Good point Done https://codereview.chromium.org/839323003/diff/20001/pkg/compiler/lib/src/js/... pkg/compiler/lib/src/js/rewrite_async.dart:476: /// The order is important, therefore the type is explicitly LinkedHashMap. On 2015/01/20 16:06:15, floitsch wrote: > New line before. Done. https://codereview.chromium.org/839323003/diff/20001/pkg/compiler/lib/src/js/... pkg/compiler/lib/src/js/rewrite_async.dart:546: bool transform(Node node) { On 2015/01/20 16:06:14, floitsch wrote: > shouldTransform Done. https://codereview.chromium.org/839323003/diff/20001/pkg/compiler/lib/src/js/... pkg/compiler/lib/src/js/rewrite_async.dart:575: if (result is Literal) return result; On 2015/01/20 16:06:15, floitsch wrote: > I guess this will be wrong, once you have list-literals that contain > side-effects. Actually list-literals are not Literal, but ObjectInitializers and ArrayInitializers, so I think this will work. But I will add a comment https://codereview.chromium.org/839323003/diff/20001/pkg/compiler/lib/src/js/... pkg/compiler/lib/src/js/rewrite_async.dart:657: helperBody = new Try(new Block([helperBody]), On 2015/01/20 16:06:15, floitsch wrote: > consider using the `js` templates. > You will actually end up reusing the template, and it's easier to read. Yeah - it is better like that. https://codereview.chromium.org/839323003/diff/20001/pkg/compiler/lib/src/js/... pkg/compiler/lib/src/js/rewrite_async.dart:718: throw("The transformer should never meet: $node"); On 2015/01/20 16:06:15, floitsch wrote: > If the function is intendend to stay, make it have a better error-message. > Start with: > throw "Internal error: trying to visit $node"; > > (If the error is encountered by real users, they don't want to hear that the > node is not unreachable. It clearly is, otherwise they wouldn't see the error). > > Eventually it should throw a real exception and/or the element that caused the > issue. Done.
Resolution/enqueuing changes LGTM. https://codereview.chromium.org/839323003/diff/120001/pkg/compiler/lib/src/js... File pkg/compiler/lib/src/js_backend/backend.dart (right): https://codereview.chromium.org/839323003/diff/120001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_backend/backend.dart:66: static final Uri DART_ASYNC = new Uri(scheme: 'dart', path: 'async'); Not needed (exists on [Compiler]). https://codereview.chromium.org/839323003/diff/120001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_backend/backend.dart:1603: Element getFuture() { This is [Compiler.futureClass]. https://codereview.chromium.org/839323003/diff/120001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_backend/backend.dart:1763: asyncLibrary = library; Not needed. https://codereview.chromium.org/839323003/diff/120001/pkg/compiler/lib/src/re... File pkg/compiler/lib/src/resolution/members.dart (right): https://codereview.chromium.org/839323003/diff/120001/pkg/compiler/lib/src/re... pkg/compiler/lib/src/resolution/members.dart:504: Backend backend = compiler.backend; Unneeded?
LGTM with comments. Next time please don't create such a massive CL. I haven't completely reviewed the test that tests the verbatim translation. I haven't completely reviewed the async* transformation after I saw that there are still some important parts missing. Can we remove async* from this CL, and commit it once it's ready? I would like to see more tests. Ok, if they come after Wednesday. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/cl... File pkg/compiler/lib/src/closure.dart (right): https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/cl... pkg/compiler/lib/src/closure.dart:351: final Set<Local> variablesUsedInTryOrGenerator = new Set<Local>(); Add comment. Not every variable in a generator is in this set. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/cl... pkg/compiler/lib/src/closure.dart:593: // TODO(ngeoffray): only do this if the variable is mutated. We should have an "isEffectivelyFinal" getter on variables. Johnni already talked about it some time ago. Merge the TODOs (here and in line 600) and ask Johnni if we can put his ldap into the TODO. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... File pkg/compiler/lib/src/js/nodes.dart (right): https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/nodes.dart:537: class Yield extends Statement { Add comment. There exists a JS yield, and this one is different. Also explain why it is a statement and not an expression. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... File pkg/compiler/lib/src/js/rewrite_async.dart (right): https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:34: List<Node> targetsAndTries = new List<Node>(); Only contains tries if they have a finally. At the very least add a comment, but maybe there is a better name. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:69: int highWaterMark = 0; comment what this is. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:89: /// Rewrites a sync&/async/async* function to an equivalent normal function. sync* https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:94: analysis = new Analysis(); too generic. What does it analyse? https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:126: deallocateTempVar([howMany = 1]) { void deallocateTempVar. Add "void" to all methods that don't return anything. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:164: bool hasJumpViaFinally = false; hasJumpThroughFinally ? https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:167: /// `shouldTranslate` is false. where is `shouldTranslate` ? Why is it not a [shouldTranslate] reference? https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:171: bool hasJumpViaOuterLabel = false; hasJumpThroughOuter label ? hasJumpToOuterSwitch ? https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:190: void beginLabel(int label) { Add comment. Since it has a side-effect it is important to know which. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:192: labelledParts[label] = currentStatementBuffer = new List<Statement>(); nit: I don't like multiple assignments in the same line. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:196: /// Returns a statement assigning to the `__goto` variable. This should be first and only mention of "__goto". (It is pretty clear what it means, so I'm ok with leaving it). https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:204: /// Returns a block with the same effect as [addGoto]. Improve the comment. It doesn't talk about the "break". What does "with the same effect" mean? https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:216: /// Will also insert a comment describing the label if available. New line before. Also inserts a ... https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:247: void visitExpressionIgnoreResult(Expression node) { Add comment. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:258: /// Will emit an assignment of [result] to a temporary variable unless it can Emits https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:259: /// be guaranteed that evaluating [result] has no side-effect. Explain what happens otherwise. Maybe best explained with an example. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:280: // Stores the result of evaluation first expression in a temporary if the ... evaluating the first ... https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:281: // second contains an await. await or yield https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:295: withExpressions(List<Expression> nodes, fn(List<Expression> results)) { comment. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:335: "return #thenHelper($returnValue, null, $controllerName, null)", In order to use "js.statement" all non-holes must have a final set of names. "returnValue" and "controllerName" are not necessarily always the same. I would make it a non-cacheable template, or make $returnValue and $controllerName holes. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:343: "return #thenHelper(null, null, $controllerName, null)", ditto and the same for all other js-templates in this file. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:351: /// Initializing a controller. (Will be a completer for async, No need for future-tense. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:353: /// The controller will be passed to each call to [thenHelper]. The controller is passed ... https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:354: Statement initializer() { generateInitializer? "initializer" is a getter name. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:393: try { indent https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:404: // Helper. Comment does not add any value. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:448: function (#params) { indent. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:519: return #thenHelper(#expression, $helperName, $controllerName, null); indent. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:523: return #thenHelper(#expression, $helperName, $controllerName, indent. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:537: bool isResult(Expression node) { what is 'result' ? It looks like it is a temporary and not the actual result of a function. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:543: if (shouldTransform(node.right)) { if (shouldTransform(node.right) && (node.op == "||" || node.op == "&&")) { ... } https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:681: List<int> finallyStack = new List<int>(); since it doesn't only contain finally's maybe rename? https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:682: for (Node n in targetsAndTries.reversed) { Node node in ... https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:684: finallyStack.add(finallyLabels[n]); assert that the try has a finally. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:686: finallyStack.add(targetLabel); indentation. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:767: ? new LiteralNumber("0") // Dummy init. Why can't you just call visitExpression instead of replacing the empty fors with dummy values? https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:794: // If there is no update continuing the loop is the same as going to the no update, continuing ... (add comma) https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:796: int continueLabel = node.update == null (node.update == null) https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:805: condition is LiteralBool && condition.value == true) { This doesn't make sense. The second part will always be caught by the first one. I guess this should be: if (condition != null && !(condition is LiteralBool && condition.value == true)) { https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:838: List<Statement> resultBuffer = currentStatementBuffer = new List(); Nit: split assignments. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:883: throw "Unsupported"; use a real error object here (and in the other unsupported cases). (InternalError, UnsupportedError, UnimplementedError, ...) https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:943: bool storeTarget = node.arguments.any(shouldTransform); You should be able to skip this. The targets of 'new' should be classes that don't need to be stored. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:965: throw "Unexpected"; Replace with real error. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1073: // The default case is handled last. // The goto for the default case is added after all non-default clauses have been handled. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1108: throw "Unknown switchclause $i"; Use real exception. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1150: Catch catchPart = node.catchPart == null ? (node.catchPart == null) https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1155: node.finallyPart == null ? null : translateInBlock(node.finallyPart); (node.finallyPart == null) https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1163: errorHandlerLabels.add(handlerLabel); Add more comments. Basically we set it, when we enter, and we have to pop it on every path (normal and error exit). https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1182: String errorRename = freshName(node.catchPart.declaration.name); Why is that necessary? Why can't we just use the original name? I assume it's because catch variables shadow variables of the function. If that's the case it needs a comment. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1194: addStatement(js.statement("$nextName = [#];", Shouldn't this be in the catch-part? https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1213: // But the initialization is done here. lowercase "but". https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1236: orElse: () { orElse: () => new Pair(null, null) https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1237: return new Pair(null, null); Don't create a new Pair for every variable use that isn't renamed. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1261: addStatement(new If.noElse(new Prefix("!", condition), If dart2js emits While loops with `while(true)` optimize for that case (but I think it's covered by the `for` case). https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1277: addStatement(goto(label)); Add comment explaining that this doesn't do the jump, but only sets the goto-variable. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1287: throw "TODO"; Throw "good" error. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1291: addStatement(goto(label)); add comment. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1293: return #thenHelper(#yieldExpression(#expression), indent. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1313: class Analysis extends NodeVisitor { generic type? https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1325: Fun function; currentFunction? add comment. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1335: analyze(Fun node) { missing return type. same for other functions. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1365: bool value = node.value == null ? false : visit(node.value); (node.value == null) https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1447: (LabeledStatement stm) => stm.label == node.targetLabel); assert that the target is indeed a loop. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1481: bool init = node.init == null ? false : visit(node.init); parenthesis around all the conditions. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1519: throw "Internal error: $node is not handled by the async-transform."; real errors. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1558: throw "Internal error: $node is not handled by the async-transform."; real error (and below). https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1665: bool finallyPart = node.finallyPart == null parenthesis. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1700: // Note that `return visit(node.condition) || visit(node.body)` would lead Remove comment. That's true for all "||" parts. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... File pkg/compiler/lib/src/js_backend/backend.dart (right): https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_backend/backend.dart:1648: return classElement same line. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_backend/backend.dart:1659: return classElement same line. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/re... File pkg/compiler/lib/src/resolution/resolution.dart (right): https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/re... pkg/compiler/lib/src/resolution/resolution.dart:18: import '../js_backend/js_backend.dart'; That feels fishy. Why would the resolution need to know anything about the js_backend? https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... File pkg/compiler/lib/src/ssa/builder.dart (right): https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:24: if (element is FunctionElement) { For another CL: the FunctionCompiler should *not* be in the ssa-builder file. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:33: .rewrite(result); The "rewrite" drowns in the whole expression. Create a rewriter first, and then call 'rewrite' on it. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:5204: push(new HYield(yielded, node.hasStar)); I think there is a function that does this. Maybe 'add' or something. (in any case: push followed by pop should not be necessary). https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:5357: no need for the new line. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:5359: if (node.isAsync) { Move the code into a separate function. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:5360: // The async for is implemented with a StreamIterator. async-for https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:5369: void buildInitializer() { nested functions should have a new line before and after. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:5407: handleLoop(node, buildInitializer, buildCondition, () {}, buildBody); Since you already give a name to the buildInitializer, might as well do the same for the updater. Alternatively assign them to local variables, so that they have a name: Function buildInitializer = () {}; Function buildUpdate = () {}; handleLoop(...); https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:5953: buildProtected(void buildTry(), void buildFinally()) { return type. Change name, since it's not a general "buildProtected". https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:5959: // in a non-dominated block. Does this comment apply for us? Do we really need to clone the localshandler? https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:5977: // the catch or finally block. It won't have any catch block. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:5980: SubGraph catchGraph = null; Remove variable. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:5981: HLocalValue exception = null; Remove (and inline) ? https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:5995: addExitTrySuccessor(successor) { add return type. (especially for nested functions). (also missing type for argument). https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:5996: if (successor == null) return; Check should be unnecessary. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:6010: // has 1) the body, 2) the catch, 3) the finally, and 4) the exit no catch. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:6016: // The body has either the catch or the finally block as successor. no catch. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:6504: tooDifficult = true; How can this happen, when we already return if there is an async-modifier? https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... File pkg/compiler/lib/src/ssa/codegen.dart (right): https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/codegen.dart:2073: pushStatement(new js.ExpressionStatement(value)); I'm not sure I agree with this. There is a reason the `return` is here. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... File pkg/compiler/lib/src/ssa/nodes.dart (right): https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/nodes.dart:2246: bool canThrow() => inputs[0].canThrow(); // TODO is this true? I don't think that's correct. I believe an await can throw, if the future is not "correct". var myFakeFuture = new BadFuture(); // (throwing for `then`); await myFakeFuture; // BOOM. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/nodes.dart:2256: bool canThrow() => inputs[0].canThrow(); // TODO is this true? ditto. I don't think this is correct. This should only be true, if yield(x) can throw for any evaluated x. That is, if the following can throw: var t = x; yield(t); https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... File sdk/lib/_internal/compiler/js_lib/js_helper.dart (right): https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3462: // If helperCallback is null we complete the completer with object. Explain more. Is errorCallback necessary?... https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3465: thenHelper(object, Types. https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3480: class WrappedJsFunction { I don't understand this. Why not: Function _wrapJsFunction(dynamic fun, Completer completer) { return (result) { try { JS('', '#(#)', fun, result); } catch(e, st) { completer.completeError(e, st); } }; } https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3496: // If helperCallback or errorCallback throws we add the error to the stream Add more documentation what exactly is expected and allowed. https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3498: streamHelper(object, add real dartdoc. types. https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3502: print("Called with $controller"); debug line. https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3513: // TODO(sigurdm): Handle yield star from async*. Don't silently ignore the value. https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3543: // Used to avoid creating two identical closure classes in [streamHelper]. Ditto: I don't like "call" classes. Never sure if it trips the compiler in unexpected ways. https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3551: print(controller); debugprint. https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3586: final helper; should not be public. https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3589: bool stillRunning = true; Should not be public. https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3589: bool stillRunning = true; I don't think we use "stillRunning" in other iterators. For consistency we should try to use the same names. https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3590: bool runningNested = false; should not be public. https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3595: : helper = ((arg) => JS('', '#(#)', helper, arg)); don't call them the same. Call the argument jsHelperCallback or something that indicates that it is a JS function. https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3623: final outerHelper; should not be public. https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3625: SyncStarIterable(this.outerHelper); Document what this is. https://codereview.chromium.org/839323003/diff/140001/sdk/lib/async/stream_im... File sdk/lib/async/stream_impl.dart (right): https://codereview.chromium.org/839323003/diff/140001/sdk/lib/async/stream_im... sdk/lib/async/stream_impl.dart:1041: ? new _Future<Null>.immediate(null) This does not belong into this CL. Create a new CL (with test case), and check with Lasse. Also just return `null`. (after all, the subscription was already allowed to return null). https://codereview.chromium.org/839323003/diff/140001/tests/compiler/dart2js/... File tests/compiler/dart2js/async_await_js_transform_test.dart (right): https://codereview.chromium.org/839323003/diff/140001/tests/compiler/dart2js/... tests/compiler/dart2js/async_await_js_transform_test.dart:21: Expect.stringEquals(expected, printer.outBuffer.getText()); This is extremely brittle. Any change in the transformer will require changes to the test. We will probably revisit, when optimizations are implemented (and there is a lot of potential for them...) https://codereview.chromium.org/839323003/diff/140001/tests/language/await_fo... File tests/language/await_for_cancel_test.dart (right): https://codereview.chromium.org/839323003/diff/140001/tests/language/await_fo... tests/language/await_for_cancel_test.dart:68: timer = new Timer.periodic(const Duration(milliseconds: 10), tick); I think there are some engines where a timer != 0 doesn't work. https://codereview.chromium.org/839323003/diff/140001/tests/language/await_fo... File tests/language/await_for_test.dart (right): https://codereview.chromium.org/839323003/diff/140001/tests/language/await_fo... tests/language/await_for_test.dart:28: while (await (it.moveNext())) { why this change? https://codereview.chromium.org/839323003/diff/140001/tests/language/await_re... File tests/language/await_regression_test.dart (right): https://codereview.chromium.org/839323003/diff/140001/tests/language/await_re... tests/language/await_regression_test.dart:16: var a = await (later('Asterix').then((tonic) { why this change? https://codereview.chromium.org/839323003/diff/140001/tests/language/await_re... tests/language/await_regression_test.dart:19: var o = await (manana('Obelix').then(manana)); ditto. https://codereview.chromium.org/839323003/diff/140001/tests/language/language... File tests/language/language_dart2js.status (right): https://codereview.chromium.org/839323003/diff/140001/tests/language/language... tests/language/language_dart2js.status:9: async_test/none: CompileTimeError # Issue 22184 I have reviewed a CL. Make sure this isn't already committed (and if it is, ask Johnni to close the bug). https://codereview.chromium.org/839323003/diff/140001/tests/language/syncstar... File tests/language/syncstar_yield_test.dart (right): https://codereview.chromium.org/839323003/diff/140001/tests/language/syncstar... tests/language/syncstar_yield_test.dart:39: Expect.listEquals([null, -1, 0, 1, 2, 3, 0, 1, 2, 3], foo2(4).take(10).toList()); long line. https://codereview.chromium.org/839323003/diff/140001/tests/language/syncstar... tests/language/syncstar_yield_test.dart:41: Iterator i1 = t.iterator; it1 https://codereview.chromium.org/839323003/diff/140001/tests/language/syncstar... tests/language/syncstar_yield_test.dart:42: Iterator i2 = t.iterator; it2 https://codereview.chromium.org/839323003/diff/140001/tests/language/syncstar... tests/language/syncstar_yield_test.dart:46: Expect.equals(3, i2.current); Expect.isFalse(i1.moveNext()); Expect.isFalse(i2.moveNext());
Thanks for the review - and sorry for the huge CL. I added (seemingly working) support for async* now. Could you PTAL at those parts? Mainly in js_helper.dart. https://codereview.chromium.org/839323003/diff/120001/pkg/compiler/lib/src/js... File pkg/compiler/lib/src/js_backend/backend.dart (right): https://codereview.chromium.org/839323003/diff/120001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_backend/backend.dart:66: static final Uri DART_ASYNC = new Uri(scheme: 'dart', path: 'async'); On 2015/02/02 10:45:10, Johnni Winther wrote: > Not needed (exists on [Compiler]). Done https://codereview.chromium.org/839323003/diff/120001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_backend/backend.dart:1603: Element getFuture() { On 2015/02/02 10:45:10, Johnni Winther wrote: > This is [Compiler.futureClass]. And was not used anymore. Thanks. https://codereview.chromium.org/839323003/diff/120001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_backend/backend.dart:1763: asyncLibrary = library; On 2015/02/02 10:45:10, Johnni Winther wrote: > Not needed. Done https://codereview.chromium.org/839323003/diff/120001/pkg/compiler/lib/src/re... File pkg/compiler/lib/src/resolution/members.dart (right): https://codereview.chromium.org/839323003/diff/120001/pkg/compiler/lib/src/re... pkg/compiler/lib/src/resolution/members.dart:504: Backend backend = compiler.backend; On 2015/02/02 10:45:10, Johnni Winther wrote: > Unneeded? Yes. Thanks! https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/cl... File pkg/compiler/lib/src/closure.dart (right): https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/cl... pkg/compiler/lib/src/closure.dart:351: final Set<Local> variablesUsedInTryOrGenerator = new Set<Local>(); On 2015/02/02 22:00:06, floitsch wrote: > Add comment. > Not every variable in a generator is in this set. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/cl... pkg/compiler/lib/src/closure.dart:593: // TODO(ngeoffray): only do this if the variable is mutated. On 2015/02/02 22:00:06, floitsch wrote: > We should have an "isEffectivelyFinal" getter on variables. > Johnni already talked about it some time ago. > > Merge the TODOs (here and in line 600) and ask Johnni if we can put his ldap > into the TODO. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... File pkg/compiler/lib/src/js/nodes.dart (right): https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/nodes.dart:537: class Yield extends Statement { On 2015/02/02 22:00:06, floitsch wrote: > Add comment. > There exists a JS yield, and this one is different. > Also explain why it is a statement and not an expression. I was not aware of js yield, I renamed this to DartYield, and added a comment, so all confusion should be avoided. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... File pkg/compiler/lib/src/js/rewrite_async.dart (right): https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:34: List<Node> targetsAndTries = new List<Node>(); On 2015/02/02 22:00:07, floitsch wrote: > Only contains tries if they have a finally. > At the very least add a comment, but maybe there is a better name. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:69: int highWaterMark = 0; On 2015/02/02 22:00:07, floitsch wrote: > comment what this is. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:89: /// Rewrites a sync&/async/async* function to an equivalent normal function. On 2015/02/02 22:00:09, floitsch wrote: > sync* Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:94: analysis = new Analysis(); On 2015/02/02 22:00:09, floitsch wrote: > too generic. What does it analyse? Called it PreTranslationAnalysis. Better suggestions are welcome. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:126: deallocateTempVar([howMany = 1]) { On 2015/02/02 22:00:09, floitsch wrote: > void deallocateTempVar. > > Add "void" to all methods that don't return anything. This function is unused. It has been removed. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:164: bool hasJumpViaFinally = false; On 2015/02/02 22:00:08, floitsch wrote: > hasJumpThroughFinally ? Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:167: /// `shouldTranslate` is false. On 2015/02/02 22:00:09, floitsch wrote: > where is `shouldTranslate` ? I meant shouldTransform > Why is it not a [shouldTranslate] reference? It is now. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:171: bool hasJumpViaOuterLabel = false; On 2015/02/02 22:00:10, floitsch wrote: > hasJumpThroughOuter label ? > hasJumpToOuterSwitch ? Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:190: void beginLabel(int label) { On 2015/02/02 22:00:06, floitsch wrote: > Add comment. > Since it has a side-effect it is important to know which. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:192: labelledParts[label] = currentStatementBuffer = new List<Statement>(); On 2015/02/02 22:00:08, floitsch wrote: > nit: I don't like multiple assignments in the same line. I am not particularly fond of them either, but here it seemed natural. But it is not dear to my heart, and I removed it. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:196: /// Returns a statement assigning to the `__goto` variable. This should be On 2015/02/02 22:00:07, floitsch wrote: > first and only mention of "__goto". (It is pretty clear what it means, so I'm ok > with leaving it). I changed to "variable named [gotoName]". https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:204: /// Returns a block with the same effect as [addGoto]. On 2015/02/02 22:00:08, floitsch wrote: > Improve the comment. > It doesn't talk about the "break". > What does "with the same effect" mean? I made it more explicit. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:216: /// Will also insert a comment describing the label if available. On 2015/02/02 22:00:08, floitsch wrote: > New line before. > > Also inserts a ... Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:247: void visitExpressionIgnoreResult(Expression node) { On 2015/02/02 22:00:08, floitsch wrote: > Add comment. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:258: /// Will emit an assignment of [result] to a temporary variable unless it can On 2015/02/02 22:00:07, floitsch wrote: > Emits Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:259: /// be guaranteed that evaluating [result] has no side-effect. On 2015/02/02 22:00:09, floitsch wrote: > Explain what happens otherwise. > > Maybe best explained with an example. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:280: // Stores the result of evaluation first expression in a temporary if the On 2015/02/02 22:00:09, floitsch wrote: > ... evaluating the first ... Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:281: // second contains an await. On 2015/02/02 22:00:07, floitsch wrote: > await or yield Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:295: withExpressions(List<Expression> nodes, fn(List<Expression> results)) { On 2015/02/02 22:00:08, floitsch wrote: > comment. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:335: "return #thenHelper($returnValue, null, $controllerName, null)", On 2015/02/02 22:00:07, floitsch wrote: > In order to use "js.statement" all non-holes must have a final set of names. > "returnValue" and "controllerName" are not necessarily always the same. > > I would make it a non-cacheable template, or make $returnValue and > $controllerName holes. Added a TODO. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:343: "return #thenHelper(null, null, $controllerName, null)", On 2015/02/02 22:00:08, floitsch wrote: > ditto and the same for all other js-templates in this file. Acknowledged. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:351: /// Initializing a controller. (Will be a completer for async, On 2015/02/02 22:00:08, floitsch wrote: > No need for future-tense. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:353: /// The controller will be passed to each call to [thenHelper]. On 2015/02/02 22:00:08, floitsch wrote: > The controller is passed ... Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:354: Statement initializer() { On 2015/02/02 22:00:08, floitsch wrote: > generateInitializer? > > "initializer" is a getter name. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:393: try { On 2015/02/02 22:00:07, floitsch wrote: > indent Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:404: // Helper. On 2015/02/02 22:00:06, floitsch wrote: > Comment does not add any value. Removed https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:448: function (#params) { On 2015/02/02 22:00:08, floitsch wrote: > indent. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:519: return #thenHelper(#expression, $helperName, $controllerName, null); On 2015/02/02 22:00:09, floitsch wrote: > indent. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:523: return #thenHelper(#expression, $helperName, $controllerName, On 2015/02/02 22:00:09, floitsch wrote: > indent. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:537: bool isResult(Expression node) { On 2015/02/02 22:00:09, floitsch wrote: > what is 'result' ? > It looks like it is a temporary and not the actual result of a function. Added explanatory comment https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:543: if (shouldTransform(node.right)) { On 2015/02/02 22:00:07, floitsch wrote: > if (shouldTransform(node.right) && > (node.op == "||" || node.op == "&&")) { > ... > } Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:681: List<int> finallyStack = new List<int>(); On 2015/02/02 22:00:07, floitsch wrote: > since it doesn't only contain finally's maybe rename? Renamed to `jumpStack`. Better? https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:682: for (Node n in targetsAndTries.reversed) { On 2015/02/02 22:00:07, floitsch wrote: > Node node in ... Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:684: finallyStack.add(finallyLabels[n]); On 2015/02/02 22:00:08, floitsch wrote: > assert that the try has a finally. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:686: finallyStack.add(targetLabel); On 2015/02/02 22:00:09, floitsch wrote: > indentation. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:767: ? new LiteralNumber("0") // Dummy init. On 2015/02/02 22:00:08, floitsch wrote: > Why can't you just call visitExpression instead of replacing the empty fors with > dummy values? I have to use withExpressions to get the right values stored in temporaries. But I changed withExpressions to handle null values. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:794: // If there is no update continuing the loop is the same as going to the On 2015/02/02 22:00:09, floitsch wrote: > no update, continuing ... (add comma) Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:796: int continueLabel = node.update == null On 2015/02/02 22:00:09, floitsch wrote: > (node.update == null) Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:805: condition is LiteralBool && condition.value == true) { On 2015/02/02 22:00:07, floitsch wrote: > This doesn't make sense. > The second part will always be caught by the first one. I guess this should be: > if (condition != null && > !(condition is LiteralBool && condition.value == true)) { Good catch. Thanks. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:838: List<Statement> resultBuffer = currentStatementBuffer = new List(); On 2015/02/02 22:00:07, floitsch wrote: > Nit: split assignments. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:883: throw "Unsupported"; On 2015/02/02 22:00:08, floitsch wrote: > use a real error object here (and in the other unsupported cases). > (InternalError, UnsupportedError, UnimplementedError, ...) Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:943: bool storeTarget = node.arguments.any(shouldTransform); On 2015/02/02 22:00:08, floitsch wrote: > You should be able to skip this. The targets of 'new' should be classes that > don't need to be stored. I like to keep with the general Javascript semantics. The output will be the same when the class is not awaited. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:965: throw "Unexpected"; On 2015/02/02 22:00:08, floitsch wrote: > Replace with real error. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1073: // The default case is handled last. On 2015/02/02 22:00:10, floitsch wrote: > // The goto for the default case is added after all non-default clauses have > been handled. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1108: throw "Unknown switchclause $i"; On 2015/02/02 22:00:09, floitsch wrote: > Use real exception. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1150: Catch catchPart = node.catchPart == null ? On 2015/02/02 22:00:07, floitsch wrote: > (node.catchPart == null) Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1155: node.finallyPart == null ? null : translateInBlock(node.finallyPart); On 2015/02/02 22:00:09, floitsch wrote: > (node.finallyPart == null) Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1163: errorHandlerLabels.add(handlerLabel); On 2015/02/02 22:00:07, floitsch wrote: > Add more comments. > Basically we set it, when we enter, and we have to pop it on every path (normal > and error exit). Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1182: String errorRename = freshName(node.catchPart.declaration.name); On 2015/02/02 22:00:07, floitsch wrote: > Why is that necessary? > Why can't we just use the original name? > > I assume it's because catch variables shadow variables of the function. If > that's the case it needs a comment. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1194: addStatement(js.statement("$nextName = [#];", On 2015/02/02 22:00:09, floitsch wrote: > Shouldn't this be in the catch-part? It belongs in the catch-part before the finally label, but it is only needed if there is a finally part. Therefore it is here. I added comment. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1213: // But the initialization is done here. On 2015/02/02 22:00:07, floitsch wrote: > lowercase "but". Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1236: orElse: () { On 2015/02/02 22:00:06, floitsch wrote: > orElse: () => new Pair(null, null) Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1237: return new Pair(null, null); On 2015/02/02 22:00:08, floitsch wrote: > Don't create a new Pair for every variable use that isn't renamed. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1261: addStatement(new If.noElse(new Prefix("!", condition), On 2015/02/02 22:00:07, floitsch wrote: > If dart2js emits While loops with `while(true)` optimize for that case (but I > think it's covered by the `for` case). I think it can happen, and added special handling. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1277: addStatement(goto(label)); On 2015/02/02 22:00:07, floitsch wrote: > Add comment explaining that this doesn't do the jump, but only sets the > goto-variable. I renamed goto -> setGotoVariable and added a comment. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1287: throw "TODO"; On 2015/02/02 22:00:09, floitsch wrote: > Throw "good" error. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1293: return #thenHelper(#yieldExpression(#expression), On 2015/02/02 22:00:08, floitsch wrote: > indent. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1313: class Analysis extends NodeVisitor { On 2015/02/02 22:00:09, floitsch wrote: > generic type? Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1325: Fun function; On 2015/02/02 22:00:07, floitsch wrote: > currentFunction? > > add comment. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1335: analyze(Fun node) { On 2015/02/02 22:00:08, floitsch wrote: > missing return type. > same for other functions. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1365: bool value = node.value == null ? false : visit(node.value); On 2015/02/02 22:00:07, floitsch wrote: > (node.value == null) Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1447: (LabeledStatement stm) => stm.label == node.targetLabel); On 2015/02/02 22:00:08, floitsch wrote: > assert that the target is indeed a loop. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1481: bool init = node.init == null ? false : visit(node.init); On 2015/02/02 22:00:08, floitsch wrote: > parenthesis around all the conditions. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1519: throw "Internal error: $node is not handled by the async-transform."; On 2015/02/02 22:00:09, floitsch wrote: > real errors. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1558: throw "Internal error: $node is not handled by the async-transform."; On 2015/02/02 22:00:09, floitsch wrote: > real error (and below). Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1558: throw "Internal error: $node is not handled by the async-transform."; On 2015/02/02 22:00:09, floitsch wrote: > real error (and below). Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1665: bool finallyPart = node.finallyPart == null On 2015/02/02 22:00:06, floitsch wrote: > parenthesis. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1700: // Note that `return visit(node.condition) || visit(node.body)` would lead On 2015/02/02 22:00:09, floitsch wrote: > Remove comment. That's true for all "||" parts. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... File pkg/compiler/lib/src/js_backend/backend.dart (right): https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_backend/backend.dart:1648: return classElement On 2015/02/02 22:00:10, floitsch wrote: > same line. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js_backend/backend.dart:1659: return classElement On 2015/02/02 22:00:10, floitsch wrote: > same line. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/re... File pkg/compiler/lib/src/resolution/resolution.dart (right): https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/re... pkg/compiler/lib/src/resolution/resolution.dart:18: import '../js_backend/js_backend.dart'; On 2015/02/02 22:00:10, floitsch wrote: > That feels fishy. Why would the resolution need to know anything about the > js_backend? It is actually not needed anymore. It was a hack during development that I forgot to remove. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... File pkg/compiler/lib/src/ssa/builder.dart (right): https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:24: if (element is FunctionElement) { On 2015/02/02 22:00:10, floitsch wrote: > For another CL: the FunctionCompiler should *not* be in the ssa-builder file. Acknowledged. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:33: .rewrite(result); On 2015/02/02 22:00:11, floitsch wrote: > The "rewrite" drowns in the whole expression. > Create a rewriter first, and then call 'rewrite' on it. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:5204: push(new HYield(yielded, node.hasStar)); On 2015/02/02 22:00:10, floitsch wrote: > I think there is a function that does this. Maybe 'add' or something. > (in any case: push followed by pop should not be necessary). Right, add does it. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:5357: On 2015/02/02 22:00:10, floitsch wrote: > no need for the new line. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:5359: if (node.isAsync) { On 2015/02/02 22:00:10, floitsch wrote: > Move the code into a separate function. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:5360: // The async for is implemented with a StreamIterator. On 2015/02/02 22:00:10, floitsch wrote: > async-for Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:5369: void buildInitializer() { On 2015/02/02 22:00:11, floitsch wrote: > nested functions should have a new line before and after. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:5407: handleLoop(node, buildInitializer, buildCondition, () {}, buildBody); On 2015/02/02 22:00:10, floitsch wrote: > Since you already give a name to the buildInitializer, might as well do the same > for the updater. > Alternatively assign them to local variables, so that they have a name: > > Function buildInitializer = () {}; > Function buildUpdate = () {}; > handleLoop(...); Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:5953: buildProtected(void buildTry(), void buildFinally()) { On 2015/02/02 22:00:11, floitsch wrote: > return type. > > Change name, since it's not a general "buildProtected". Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:5959: // in a non-dominated block. On 2015/02/02 22:00:11, floitsch wrote: > Does this comment apply for us? > > Do we really need to clone the localshandler? Not sure - I will ask https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:5977: // the catch or finally block. On 2015/02/02 22:00:10, floitsch wrote: > It won't have any catch block. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:5980: SubGraph catchGraph = null; On 2015/02/02 22:00:11, floitsch wrote: > Remove variable. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:5981: HLocalValue exception = null; On 2015/02/02 22:00:11, floitsch wrote: > Remove (and inline) ? Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:5995: addExitTrySuccessor(successor) { On 2015/02/02 22:00:10, floitsch wrote: > add return type. (especially for nested functions). > (also missing type for argument). Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:5996: if (successor == null) return; On 2015/02/02 22:00:10, floitsch wrote: > Check should be unnecessary. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:6010: // has 1) the body, 2) the catch, 3) the finally, and 4) the exit On 2015/02/02 22:00:11, floitsch wrote: > no catch. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:6016: // The body has either the catch or the finally block as successor. On 2015/02/02 22:00:10, floitsch wrote: > no catch. Done. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:6504: tooDifficult = true; On 2015/02/02 22:00:10, floitsch wrote: > How can this happen, when we already return if there is an async-modifier? Right - I added this one first, then realized that it was not enough. But now it is redundant. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... File pkg/compiler/lib/src/ssa/codegen.dart (right): https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/codegen.dart:2073: pushStatement(new js.ExpressionStatement(value)); On 2015/02/02 22:00:11, floitsch wrote: > I'm not sure I agree with this. > There is a reason the `return` is here. Right - we need another solution. Return in a sync* or async* has a different meaning. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... File pkg/compiler/lib/src/ssa/nodes.dart (right): https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/nodes.dart:2246: bool canThrow() => inputs[0].canThrow(); // TODO is this true? On 2015/02/02 22:00:11, floitsch wrote: > I don't think that's correct. > > I believe an await can throw, if the future is not "correct". > > var myFakeFuture = new BadFuture(); // (throwing for `then`); > await myFakeFuture; // BOOM. Thanks https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/nodes.dart:2256: bool canThrow() => inputs[0].canThrow(); // TODO is this true? On 2015/02/02 22:00:11, floitsch wrote: > ditto. I don't think this is correct. > This should only be true, if yield(x) can throw for any evaluated x. That is, if > the following can throw: var t = x; yield(t); Done. https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... File sdk/lib/_internal/compiler/js_lib/js_helper.dart (right): https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3462: // If helperCallback is null we complete the completer with object. On 2015/02/02 22:00:12, floitsch wrote: > Explain more. > > Is errorCallback necessary?... The error callback will redirect the control flow to the right error handler, and is not constant. There might be other ways to implement it though... https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3465: thenHelper(object, On 2015/02/02 22:00:11, floitsch wrote: > Types. Done. https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3480: class WrappedJsFunction { On 2015/02/02 22:00:11, floitsch wrote: > I don't understand this. > > Why not: > Function _wrapJsFunction(dynamic fun, Completer completer) { > return (result) { > try { > JS('', '#(#)', fun, result); > } catch(e, st) { > completer.completeError(e, st); > } > }; > } That is better, thanks https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3496: // If helperCallback or errorCallback throws we add the error to the stream On 2015/02/02 22:00:11, floitsch wrote: > Add more documentation what exactly is expected and allowed. Done. https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3498: streamHelper(object, On 2015/02/02 22:00:12, floitsch wrote: > add real dartdoc. > > types. Done. https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3502: print("Called with $controller"); On 2015/02/02 22:00:11, floitsch wrote: > debug line. Done. https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3513: // TODO(sigurdm): Handle yield star from async*. On 2015/02/02 22:00:11, floitsch wrote: > Don't silently ignore the value. Done. https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3543: // Used to avoid creating two identical closure classes in [streamHelper]. On 2015/02/02 22:00:12, floitsch wrote: > Ditto: I don't like "call" classes. Never sure if it trips the compiler in > unexpected ways. Done. https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3551: print(controller); On 2015/02/02 22:00:11, floitsch wrote: > debugprint. Done. https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3586: final helper; On 2015/02/02 22:00:11, floitsch wrote: > should not be public. Done. https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3589: bool stillRunning = true; On 2015/02/02 22:00:11, floitsch wrote: > Should not be public. Done. https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3589: bool stillRunning = true; On 2015/02/02 22:00:12, floitsch wrote: > I don't think we use "stillRunning" in other iterators. For consistency we > should try to use the same names. None of the other Iterators I could find maintains a done state. We can also avoid it by relying on the helper function to return ITERATION_ENDED repeatedly - this already holds, but seems more brittle. https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3590: bool runningNested = false; On 2015/02/02 22:00:12, floitsch wrote: > should not be public. Done. https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3595: : helper = ((arg) => JS('', '#(#)', helper, arg)); On 2015/02/02 22:00:11, floitsch wrote: > don't call them the same. > > Call the argument jsHelperCallback or something that indicates that it is a JS > function. Done. https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3623: final outerHelper; On 2015/02/02 22:00:11, floitsch wrote: > should not be public. Done. https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3625: SyncStarIterable(this.outerHelper); On 2015/02/02 22:00:12, floitsch wrote: > Document what this is. Done. https://codereview.chromium.org/839323003/diff/140001/sdk/lib/async/stream_im... File sdk/lib/async/stream_impl.dart (right): https://codereview.chromium.org/839323003/diff/140001/sdk/lib/async/stream_im... sdk/lib/async/stream_impl.dart:1041: ? new _Future<Null>.immediate(null) On 2015/02/02 22:00:12, floitsch wrote: > This does not belong into this CL. > > Create a new CL (with test case), and check with Lasse. > Also just return `null`. (after all, the subscription was already allowed to > return null). Ok. https://codereview.chromium.org/839323003/diff/140001/tests/compiler/dart2js/... File tests/compiler/dart2js/async_await_js_transform_test.dart (right): https://codereview.chromium.org/839323003/diff/140001/tests/compiler/dart2js/... tests/compiler/dart2js/async_await_js_transform_test.dart:21: Expect.stringEquals(expected, printer.outBuffer.getText()); On 2015/02/02 22:00:12, floitsch wrote: > This is extremely brittle. Any change in the transformer will require changes to > the test. > > We will probably revisit, when optimizations are implemented (and there is a lot > of potential for them...) True - these tests were mostly useful while developing - to give a clear picture of exactly what I wanted to generate. https://codereview.chromium.org/839323003/diff/140001/tests/language/await_fo... File tests/language/await_for_cancel_test.dart (right): https://codereview.chromium.org/839323003/diff/140001/tests/language/await_fo... tests/language/await_for_cancel_test.dart:68: timer = new Timer.periodic(const Duration(milliseconds: 10), tick); On 2015/02/02 22:00:12, floitsch wrote: > I think there are some engines where a timer != 0 doesn't work. You mean duration = 0; I changed it. https://codereview.chromium.org/839323003/diff/140001/tests/language/await_fo... File tests/language/await_for_test.dart (right): https://codereview.chromium.org/839323003/diff/140001/tests/language/await_fo... tests/language/await_for_test.dart:28: while (await (it.moveNext())) { On 2015/02/02 22:00:12, floitsch wrote: > why this change? This is because of Issue 22183. Hopefully it will soon be solved. https://codereview.chromium.org/839323003/diff/140001/tests/language/await_re... File tests/language/await_regression_test.dart (right): https://codereview.chromium.org/839323003/diff/140001/tests/language/await_re... tests/language/await_regression_test.dart:16: var a = await (later('Asterix').then((tonic) { On 2015/02/02 22:00:12, floitsch wrote: > why this change? ditto https://codereview.chromium.org/839323003/diff/140001/tests/language/await_re... tests/language/await_regression_test.dart:19: var o = await (manana('Obelix').then(manana)); On 2015/02/02 22:00:12, floitsch wrote: > ditto. ditto https://codereview.chromium.org/839323003/diff/140001/tests/language/language... File tests/language/language_dart2js.status (right): https://codereview.chromium.org/839323003/diff/140001/tests/language/language... tests/language/language_dart2js.status:9: async_test/none: CompileTimeError # Issue 22184 On 2015/02/02 22:00:12, floitsch wrote: > I have reviewed a CL. Make sure this isn't already committed (and if it is, ask > Johnni to close the bug). Done. https://codereview.chromium.org/839323003/diff/140001/tests/language/syncstar... File tests/language/syncstar_yield_test.dart (right): https://codereview.chromium.org/839323003/diff/140001/tests/language/syncstar... tests/language/syncstar_yield_test.dart:39: Expect.listEquals([null, -1, 0, 1, 2, 3, 0, 1, 2, 3], foo2(4).take(10).toList()); On 2015/02/02 22:00:12, floitsch wrote: > long line. Done. https://codereview.chromium.org/839323003/diff/140001/tests/language/syncstar... tests/language/syncstar_yield_test.dart:41: Iterator i1 = t.iterator; On 2015/02/02 22:00:12, floitsch wrote: > it1 Done. https://codereview.chromium.org/839323003/diff/140001/tests/language/syncstar... tests/language/syncstar_yield_test.dart:42: Iterator i2 = t.iterator; On 2015/02/02 22:00:12, floitsch wrote: > it2 Done. https://codereview.chromium.org/839323003/diff/140001/tests/language/syncstar... tests/language/syncstar_yield_test.dart:46: Expect.equals(3, i2.current); On 2015/02/02 22:00:12, floitsch wrote: > Expect.isFalse(i1.moveNext()); > Expect.isFalse(i2.moveNext()); Done.
Patchset #14 (id:260001) has been deleted
https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... File pkg/compiler/lib/src/ssa/codegen.dart (right): https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/codegen.dart:2073: pushStatement(new js.ExpressionStatement(value)); On 2015/02/03 16:59:32, sigurdm wrote: > On 2015/02/02 22:00:11, floitsch wrote: > > I'm not sure I agree with this. > > There is a reason the `return` is here. > > Right - we need another solution. Return in a sync* or async* has a different > meaning. I will omit the return only when in an async* or sync* function.
I only looked at the SSA and type inference part. We need a better story for type inference in the long run but this will work for now. https://codereview.chromium.org/839323003/diff/280001/pkg/compiler/lib/src/in... File pkg/compiler/lib/src/inferrer/simple_types_inferrer.dart (right): https://codereview.chromium.org/839323003/diff/280001/pkg/compiler/lib/src/in... pkg/compiler/lib/src/inferrer/simple_types_inferrer.dart:590: } else Put else and if on one line. https://codereview.chromium.org/839323003/diff/280001/pkg/compiler/lib/src/ss... File pkg/compiler/lib/src/ssa/builder.dart (right): https://codereview.chromium.org/839323003/diff/280001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:6432: if (functionExpression.asyncModifier != null) return false; Nit: Why not move one line up? I would prefer to include this into the InlineWeeder, so that we have one place where all the decisions are grouped. https://codereview.chromium.org/839323003/diff/280001/pkg/compiler/lib/src/ss... File pkg/compiler/lib/src/ssa/codegen.dart (right): https://codereview.chromium.org/839323003/diff/280001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/codegen.dart:87: Element element = work.element; Is this not always a FunctionElement? It was before...
https://codereview.chromium.org/839323003/diff/280001/pkg/compiler/lib/src/in... File pkg/compiler/lib/src/inferrer/simple_types_inferrer.dart (right): https://codereview.chromium.org/839323003/diff/280001/pkg/compiler/lib/src/in... pkg/compiler/lib/src/inferrer/simple_types_inferrer.dart:590: } else On 2015/02/04 09:42:49, herhut wrote: > Put else and if on one line. Thanks https://codereview.chromium.org/839323003/diff/280001/pkg/compiler/lib/src/ss... File pkg/compiler/lib/src/ssa/builder.dart (right): https://codereview.chromium.org/839323003/diff/280001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:6432: if (functionExpression.asyncModifier != null) return false; On 2015/02/04 09:42:49, herhut wrote: > Nit: Why not move one line up? > > I would prefer to include this into the InlineWeeder, so that we have one place > where all the decisions are grouped. Done. https://codereview.chromium.org/839323003/diff/280001/pkg/compiler/lib/src/ss... File pkg/compiler/lib/src/ssa/codegen.dart (right): https://codereview.chromium.org/839323003/diff/280001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/codegen.dart:87: Element element = work.element; On 2015/02/04 09:42:49, herhut wrote: > Is this not always a FunctionElement? It was before... Right. I will assert that it is.
https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... File pkg/compiler/lib/src/ssa/builder.dart (right): https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:5959: // in a non-dominated block. On 2015/02/03 16:59:32, sigurdm wrote: > On 2015/02/02 22:00:11, floitsch wrote: > > Does this comment apply for us? > > > > Do we really need to clone the localshandler? > > Not sure - I will ask I discussed with Karl, and you are right - it is only needed for the catch-block. I will remove it.
Haven't completely finished yet. Here are my comments so far. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... File pkg/compiler/lib/src/js/rewrite_async.dart (right): https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1194: addStatement(js.statement("$nextName = [#];", On 2015/02/03 16:59:29, sigurdm wrote: > On 2015/02/02 22:00:09, floitsch wrote: > > Shouldn't this be in the catch-part? > > It belongs in the catch-part before the finally label, but it is only needed if > there is a finally part. Therefore it is here. I added comment. But it's also only needed if there is a catch part. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1237: return new Pair(null, null); On 2015/02/03 16:59:27, sigurdm wrote: > On 2015/02/02 22:00:08, floitsch wrote: > > Don't create a new Pair for every variable use that isn't renamed. > > Done. where? https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1335: analyze(Fun node) { On 2015/02/03 16:59:29, sigurdm wrote: > On 2015/02/02 22:00:08, floitsch wrote: > > missing return type. > > same for other functions. > > Done. not for this function. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... File pkg/compiler/lib/src/ssa/builder.dart (right): https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:5953: buildProtected(void buildTry(), void buildFinally()) { On 2015/02/03 16:59:32, sigurdm wrote: > On 2015/02/02 22:00:11, floitsch wrote: > > return type. > > > > Change name, since it's not a general "buildProtected". > > Done. That's not what I meant. If the buildProtected works in the general case, then keep the name. I was under the impression that it is specialized for the buildAsyncForIn. (It definitely would be if you don't clone the locals). https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:5959: // in a non-dominated block. On 2015/02/03 16:59:32, sigurdm wrote: > On 2015/02/02 22:00:11, floitsch wrote: > > Does this comment apply for us? > > > > Do we really need to clone the localshandler? > > Not sure - I will ask I would say no, since the only variable that is used in the finally is the iterator, and that one is finally. It might be safer to do a clone, though. https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... File sdk/lib/_internal/compiler/js_lib/js_helper.dart (right): https://codereview.chromium.org/839323003/diff/140001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3462: // If helperCallback is null we complete the completer with object. On 2015/02/03 16:59:32, sigurdm wrote: > On 2015/02/02 22:00:12, floitsch wrote: > > Explain more. > > > > Is errorCallback necessary?... > > The error callback will redirect the control flow to the right error handler, > and is not constant. There might be other ways to implement it though... Yes. I was talking about another implementation. For another CL, but the errorcallback only encodes one information (the jump-target). It could just be passed to the callback as second argument. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/cl... File pkg/compiler/lib/src/closure.dart (right): https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/cl... pkg/compiler/lib/src/closure.dart:351: // Variables that are used in a try must be treated as boxed because the Make dartcomment. Add new line before "Also parameters..." paragraph. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... File pkg/compiler/lib/src/js/rewrite_async.dart (right): https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:39: // representing the target of a return. And all enclosing try-blocks that have return, and all https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:211: /// Each buffer will end up as its own case part in the big state-switch. New line before. No need for future tense. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:229: /// Will also insert a comment describing the label if available. No need for future tense. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:242: /// Will also insert a comment describing the label if available. No need for future tense. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:298: /// Returns the temporary variable or the result itself. New line before. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:351: /// All results before the last node where `shouldTransform(node)` we store in are stored https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:421: /// The controller is be passed to each call to [thenHelper]. -be- https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:608: /// visiting it returns [resultName], we can avoid reduntdantly assigning it redundantly https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:747: Statement breakStatement() { has a getter name. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:747: Statement breakStatement() { Inline into addBreak. The only other use can be replaced by an addBreak https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1238: // We set the error handler here. It must be cleared on every path out. out; normal and error exit. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1316: orElse: () => new Pair(null, null)).b; The ".b" disappears now. It must be on its own line. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1356: if (isSyncStar) { move the isSyncStar check into the withExpression: int label = newLabel("after yield"); withExpression(node.expression, (Expression expression) { if (isSyncStar) { addSyncYield(label, node); // or a better name if you have one. } else { addAsyncYield(label, node); }), store: false); Make the individual parts functions. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1382: return #thenHelper(#yieldExpression(#expression), thenHelper really isn't the best name anymore... (Here and in other locations). https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1396: "finallyList": new ArrayInitializer(enclosingFinallyLabels.map( Generate the array-initializer before. put ".map" and ".toList()" at the beginning of the lines. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1430: PreTranslationAnalysis(this.unsupported); Type unupported here: PreTranslationAnalysis(void unsupported(...)): this.unsupported = unsupported; This way we have at least an idea of the signature. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1557: assert(targets[node] is Loop || targets[node] is LabeledStatement); If the target is a labeledstatement, then its containing node should be a loop. That's what I wanted to assert. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/ss... File pkg/compiler/lib/src/ssa/builder.dart (right): https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:6055: null, add comment what this is. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/ss... File pkg/compiler/lib/src/ssa/codegen.dart (right): https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/codegen.dart:2075: // `return <expr>;` is illegal in a sync* or async* function. That's not true. It's legal, but you cannot "abuse" it. // `return <expr>;` is treated specially in sync* or async* functions. Don't mislead the async-translator by introducing `return` nodes. https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... File sdk/lib/_internal/compiler/js_lib/js_helper.dart (right): https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3499: onError: _wrapJsFunction(errorCallback, completer)); this loses the stacktrace. https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3503: // Used to avoid creating two identical closure classes in [thenHelper]. Remove comment. https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3504: Function _wrapJsFunction(dynamic /* js function */ function, name is too general. https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3518: /// Called by the transformed function for each original return, await, yield, Start by explaining what it does, not who uses it. "Implements the runtime support for async* functions". (or better). https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3521: /// On a return call with helperCallback == null, and we close the stream. ? https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3523: /// If object is an [IterationMarker] it indicates a yield or yield*. reference [object] https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3523: /// If object is an [IterationMarker] it indicates a yield or yield*. what indicates? https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3524: /// If the stream subscription has been canceled we schedule the errorCallback has been canceled when? https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3524: /// If the stream subscription has been canceled we schedule the errorCallback new line before? or "In that case..." ? https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3524: /// If the stream subscription has been canceled we schedule the errorCallback don't use "we" (here and in the rest of the documentation). If the stream subscription has been canceled, schedules the errorCallback ... https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3525: /// that should run through enclosing finally blocks before exiting. that feels weird. A 'cancel' is not an error. Why do we call the errorCallback? If cancel is like an error (for implementation purposes), mention that. https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3527: /// If it is a single-yield we add the value to the stream. If what is a single-yield? I assume it's the [object], but it's not clear. https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3536: /// If object is not and [IterationMarker] it indicates an await. not what? Reference [object]. (Whenever you would want dartdoc to be highlighted when hovering over an argument you should reference the argument) https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3562: Stream stream = object.value; can't you just controller.addStream(object.value).then((...)) ? https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3639: // Used to avoid creating identical closure classes in [streamHelper]. Remove comment. https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3645: } catch (e, st) { I believe that errors in async* are fatal. Maybe Lasse or Kevin knows (without needing to go through the spec). https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3680: // if [runningNested] this will be the nested iterator, otherwise it will be no need for future tense. Start sentence with capital character. https://codereview.chromium.org/839323003/diff/320001/tests/language/asyncsta... File tests/language/asyncstar_yield_test.dart (right): https://codereview.chromium.org/839323003/diff/320001/tests/language/asyncsta... tests/language/asyncstar_yield_test.dart:10: Stream timedCounter(int maxCount) { class seems unused. https://codereview.chromium.org/839323003/diff/320001/tests/language/asyncsta... tests/language/asyncstar_yield_test.dart:96: main () async { you still need the async helper package. https://codereview.chromium.org/839323003/diff/320001/tests/language/asyncsta... tests/language/asyncstar_yield_test.dart:99: Expect.listEquals([null, -1, 0, 1, 2, 3, 0, 1, 2, 3], await (foo3(4).take(10).toList())); long lines. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... File pkg/compiler/lib/src/js/rewrite_async.dart (right): https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:444: // [visitYield]. visitDartYield
Went through all files now. Still need to have a second closer look at the async* functionality. I'm also missing a test, that checks if a async* stream can be listened to multiple times or not. (Maybe there is already one). https://codereview.chromium.org/839323003/diff/320001/tests/language/asyncsta... File tests/language/asyncstar_yieldstar_test.dart (right): https://codereview.chromium.org/839323003/diff/320001/tests/language/asyncsta... tests/language/asyncstar_yieldstar_test.dart:39: // Cancelling the stream while it is yield*-ing from the sub-stream. Canceling https://codereview.chromium.org/839323003/diff/320001/tests/language/asyncsta... tests/language/asyncstar_yieldstar_test.dart:42: Expect.isTrue(await (finalized.future)); Also ensure that the loop didn't continue needlessly. Basically make sure that the 'i' index is correct.
LGTM once all comments are addressed. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... File pkg/compiler/lib/src/js/rewrite_async.dart (right): https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:13: import "js.dart"; Since you export without a prefix, you should "show" the classes/statics that you want to see. The js-library is relatively nasty: it has "number", "string" as top-level variables. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:26: Remove empty line. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:27: class AsyncRewriter extends NodeVisitor { Add comment that you should look at visitFun, visitDartYield, and visitAwait for comments. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:68: final Expression thenHelper; Comments on what these are. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:68: final Expression thenHelper; I believe I have written it somewhere else: this is not just a "then"-helper anymore. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:68: final Expression thenHelper; I'm not sure it's worth to have only one variable. With the exception of one occurrence we know in what function-type we are (sync*, async or async*). It would be more readable to have different fields. Potentially even create a BaseRewriter, and then only overload the functions that have anything to do with async. That would also allow to have better documentation, since the functions don't need to explain their implementation for all 3 different cases. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:388: /// (the result might been stored in [returnValueName] by some finally block). might have been https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:390: /// Returning from a sync* will return a [endOfIteration] marker. Returning from a sync* returns an [endOfIteration] marker. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:391: void addReturn() { Comments don't mention async* https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:397: switch (async) { How do these guarantee that we go through the finallies? For example: foo() sync* { try { return; } finally { yield 499; } } There are even cases, where the finally stops the return: foo() sync* { while (true) { try { return; } finally { yield 499; continue; } } } If you don't handle these cases yet, just add a test and fix it in a later CL. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:419: /// Initializing a controller. (This is a Completer for async, Initializes the controller/completer. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:439: visitFun(Fun node) { Add long comment on how you translate from one to the other. With (simplified) examples. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:513: """, {"params": node.params, move "params" into the next line or align the others. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:514: "helperBody": helperBody, In a follow-up CL: rename this to something else "helper" is overused. (I would have also expected that the one in js_helper is the "helper"...) https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:579: Expression visitAwait(Await node) { Add comment explaining what you do here. Ideally with an example (including the switch around it). https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1354: visitDartYield(DartYield node) { Add doc what's happening here. Ideally with examples. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1384: if (#notEmptyFinallyList) { remove {}. (It's stupid, but otherwise they stay in the output). https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1397: (int label) => new LiteralNumber("$label")).toList()), js.number(label) https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1398: "destinationOnCancel": new LiteralNumber("$destinationOnCancel") js.number(destinationOnCancel) https://codereview.chromium.org/839323003/diff/360001/sdk/lib/_internal/compi... File sdk/lib/_internal/compiler/js_lib/js_helper.dart (right): https://codereview.chromium.org/839323003/diff/360001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3543: /// stream has been canceled call the errorCallBack. errorCallback
https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... File pkg/compiler/lib/src/js/rewrite_async.dart (right): https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1194: addStatement(js.statement("$nextName = [#];", On 2015/02/04 12:31:27, floitsch wrote: > On 2015/02/03 16:59:29, sigurdm wrote: > > On 2015/02/02 22:00:09, floitsch wrote: > > > Shouldn't this be in the catch-part? > > > > It belongs in the catch-part before the finally label, but it is only needed > if > > there is a finally part. Therefore it is here. I added comment. > > But it's also only needed if there is a catch part. No - it is always needed, to ensure we run the finally part if an exception was thrown. Added comment to clarify. https://codereview.chromium.org/839323003/diff/140001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1237: return new Pair(null, null); On 2015/02/04 12:31:27, floitsch wrote: > On 2015/02/03 16:59:27, sigurdm wrote: > > On 2015/02/02 22:00:08, floitsch wrote: > > > Don't create a new Pair for every variable use that isn't renamed. > > > > Done. > > where? Sorry was to quick on clicking done on this. Done now https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/cl... File pkg/compiler/lib/src/closure.dart (right): https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/cl... pkg/compiler/lib/src/closure.dart:351: // Variables that are used in a try must be treated as boxed because the On 2015/02/04 12:31:28, floitsch wrote: > Make dartcomment. > > Add new line before "Also parameters..." paragraph. Done. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... File pkg/compiler/lib/src/js/rewrite_async.dart (right): https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:39: // representing the target of a return. And all enclosing try-blocks that have On 2015/02/04 12:31:28, floitsch wrote: > return, and all Done. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:211: /// Each buffer will end up as its own case part in the big state-switch. On 2015/02/04 12:31:28, floitsch wrote: > New line before. > No need for future tense. Done. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:229: /// Will also insert a comment describing the label if available. On 2015/02/04 12:31:28, floitsch wrote: > No need for future tense. Done. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:242: /// Will also insert a comment describing the label if available. On 2015/02/04 12:31:28, floitsch wrote: > No need for future tense. Done. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:298: /// Returns the temporary variable or the result itself. On 2015/02/04 12:31:28, floitsch wrote: > New line before. Done. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:351: /// All results before the last node where `shouldTransform(node)` we store in On 2015/02/04 12:31:28, floitsch wrote: > are stored Done. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:421: /// The controller is be passed to each call to [thenHelper]. On 2015/02/04 12:31:28, floitsch wrote: > -be- Done. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:608: /// visiting it returns [resultName], we can avoid reduntdantly assigning it On 2015/02/04 12:31:28, floitsch wrote: > redundantly Done. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:747: Statement breakStatement() { On 2015/02/04 12:31:28, floitsch wrote: > Inline into addBreak. The only other use can be replaced by an addBreak Done. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:747: Statement breakStatement() { On 2015/02/04 12:31:28, floitsch wrote: > has a getter name. Acknowledged. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1238: // We set the error handler here. It must be cleared on every path out. On 2015/02/04 12:31:28, floitsch wrote: > out; normal and error exit. Done. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1316: orElse: () => new Pair(null, null)).b; On 2015/02/04 12:31:28, floitsch wrote: > The ".b" disappears now. > It must be on its own line. Done. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1356: if (isSyncStar) { On 2015/02/04 12:31:28, floitsch wrote: > move the isSyncStar check into the withExpression: > int label = newLabel("after yield"); > withExpression(node.expression, (Expression expression) { > if (isSyncStar) { > addSyncYield(label, node); // or a better name if you have one. > } else { > addAsyncYield(label, node); > }), store: false); > > Make the individual parts functions. Done. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1382: return #thenHelper(#yieldExpression(#expression), On 2015/02/04 12:31:28, floitsch wrote: > thenHelper really isn't the best name anymore... (Here and in other locations). True - will add todo to rename https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1396: "finallyList": new ArrayInitializer(enclosingFinallyLabels.map( On 2015/02/04 12:31:28, floitsch wrote: > Generate the array-initializer before. > put ".map" and ".toList()" at the beginning of the lines. Done. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1430: PreTranslationAnalysis(this.unsupported); On 2015/02/04 12:31:28, floitsch wrote: > Type unupported here: > PreTranslationAnalysis(void unsupported(...)): this.unsupported = unsupported; > > This way we have at least an idea of the signature. Done. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1557: assert(targets[node] is Loop || targets[node] is LabeledStatement); On 2015/02/04 12:31:28, floitsch wrote: > If the target is a labeledstatement, then its containing node should be a loop. > That's what I wanted to assert. Aha - makes more sense. Done https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/ss... File pkg/compiler/lib/src/ssa/builder.dart (right): https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/builder.dart:6055: null, On 2015/02/04 12:31:28, floitsch wrote: > add comment what this is. Done. https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/ss... File pkg/compiler/lib/src/ssa/codegen.dart (right): https://codereview.chromium.org/839323003/diff/320001/pkg/compiler/lib/src/ss... pkg/compiler/lib/src/ssa/codegen.dart:2075: // `return <expr>;` is illegal in a sync* or async* function. On 2015/02/04 12:31:28, floitsch wrote: > That's not true. > It's legal, but you cannot "abuse" it. > > // `return <expr>;` is treated specially in sync* or async* functions. Don't > mislead the async-translator by introducing `return` nodes. I think it is true. (spec p. 109 reads: It is a compile-time error if a return statement of the form return e; appears in a generative constructor (10.6.1).) https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... File sdk/lib/_internal/compiler/js_lib/js_helper.dart (right): https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3499: onError: _wrapJsFunction(errorCallback, completer)); On 2015/02/04 12:31:29, floitsch wrote: > this loses the stacktrace. True - this won't work. We have to find a good solution. The problem is the tension between js's Error with stacktrace and Darts's error next to stacktrace. https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3503: // Used to avoid creating two identical closure classes in [thenHelper]. On 2015/02/04 12:31:29, floitsch wrote: > Remove comment. Done. https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3504: Function _wrapJsFunction(dynamic /* js function */ function, On 2015/02/04 12:31:29, floitsch wrote: > name is too general. Made it more specific. https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3518: /// Called by the transformed function for each original return, await, yield, On 2015/02/04 12:31:29, floitsch wrote: > Start by explaining what it does, not who uses it. > > "Implements the runtime support for async* functions". (or better). Done. https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3521: /// On a return call with helperCallback == null, and we close the stream. On 2015/02/04 12:31:29, floitsch wrote: > ? Reworded https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3523: /// If object is an [IterationMarker] it indicates a yield or yield*. On 2015/02/04 12:31:29, floitsch wrote: > reference [object] Done. https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3523: /// If object is an [IterationMarker] it indicates a yield or yield*. On 2015/02/04 12:31:29, floitsch wrote: > what indicates? Tried to reword. https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3523: /// If object is an [IterationMarker] it indicates a yield or yield*. On 2015/02/04 12:31:29, floitsch wrote: > what indicates? Done. https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3524: /// If the stream subscription has been canceled we schedule the errorCallback On 2015/02/04 12:31:29, floitsch wrote: > new line before? or "In that case..." ? Done. https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3524: /// If the stream subscription has been canceled we schedule the errorCallback On 2015/02/04 12:31:29, floitsch wrote: > don't use "we" (here and in the rest of the documentation). I will try to avoid it in the future. I use it to avoid too much passive. > If the stream subscription has been canceled, schedules the errorCallback ... done https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3525: /// that should run through enclosing finally blocks before exiting. On 2015/02/04 12:31:29, floitsch wrote: > that feels weird. A 'cancel' is not an error. Why do we call the errorCallback? > If cancel is like an error (for implementation purposes), mention that. errorCallback is used for running the finallies in the case of a yield if there has been a cancel. I tried to explain it better. https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3527: /// If it is a single-yield we add the value to the stream. On 2015/02/04 12:31:29, floitsch wrote: > If what is a single-yield? I assume it's the [object], but it's not clear. Reworded https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3536: /// If object is not and [IterationMarker] it indicates an await. On 2015/02/04 12:31:29, floitsch wrote: > not what? > Reference [object]. > > (Whenever you would want dartdoc to be highlighted when hovering over an > argument you should reference the argument) Acknowledged. https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3562: Stream stream = object.value; On 2015/02/04 12:31:29, floitsch wrote: > can't you just controller.addStream(object.value).then((...)) ? Brilliant - had not seen that! https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3639: // Used to avoid creating identical closure classes in [streamHelper]. On 2015/02/04 12:31:29, floitsch wrote: > Remove comment. Done. https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3645: } catch (e, st) { On 2015/02/04 12:31:29, floitsch wrote: > I believe that errors in async* are fatal. > Maybe Lasse or Kevin knows (without needing to go through the spec). Acknowledged. We will have to find out. https://codereview.chromium.org/839323003/diff/320001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3680: // if [runningNested] this will be the nested iterator, otherwise it will be On 2015/02/04 12:31:29, floitsch wrote: > no need for future tense. > Start sentence with capital character. Done. https://codereview.chromium.org/839323003/diff/320001/tests/language/asyncsta... File tests/language/asyncstar_yield_test.dart (right): https://codereview.chromium.org/839323003/diff/320001/tests/language/asyncsta... tests/language/asyncstar_yield_test.dart:10: Stream timedCounter(int maxCount) { On 2015/02/04 12:31:29, floitsch wrote: > class seems unused. Removed https://codereview.chromium.org/839323003/diff/320001/tests/language/asyncsta... tests/language/asyncstar_yield_test.dart:96: main () async { On 2015/02/04 12:31:29, floitsch wrote: > you still need the async helper package. Done. https://codereview.chromium.org/839323003/diff/320001/tests/language/asyncsta... tests/language/asyncstar_yield_test.dart:99: Expect.listEquals([null, -1, 0, 1, 2, 3, 0, 1, 2, 3], await (foo3(4).take(10).toList())); On 2015/02/04 12:31:29, floitsch wrote: > long lines. Done. https://codereview.chromium.org/839323003/diff/320001/tests/language/asyncsta... File tests/language/asyncstar_yieldstar_test.dart (right): https://codereview.chromium.org/839323003/diff/320001/tests/language/asyncsta... tests/language/asyncstar_yieldstar_test.dart:39: // Cancelling the stream while it is yield*-ing from the sub-stream. On 2015/02/04 15:31:52, floitsch wrote: > Canceling Done. https://codereview.chromium.org/839323003/diff/320001/tests/language/asyncsta... tests/language/asyncstar_yieldstar_test.dart:42: Expect.isTrue(await (finalized.future)); On 2015/02/04 15:31:52, floitsch wrote: > Also ensure that the loop didn't continue needlessly. Basically make sure that > the 'i' index is correct. Done. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... File pkg/compiler/lib/src/js/rewrite_async.dart (right): https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:13: import "js.dart"; On 2015/02/04 17:06:10, floitsch wrote: > Since you export without a prefix, you should "show" the classes/statics that > you want to see. > The js-library is relatively nasty: it has "number", "string" as top-level > variables. Hurray for quick-fixes! There is a lot of used names. Maybe it is better to go with a prefix? https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:26: On 2015/02/04 17:06:10, floitsch wrote: > Remove empty line. Done. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:27: class AsyncRewriter extends NodeVisitor { On 2015/02/04 17:06:09, floitsch wrote: > Add comment that you should look at visitFun, visitDartYield, and visitAwait for > comments. Done. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:68: final Expression thenHelper; On 2015/02/04 17:06:10, floitsch wrote: > Comments on what these are. Done. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:68: final Expression thenHelper; On 2015/02/04 17:06:10, floitsch wrote: > I'm not sure it's worth to have only one variable. With the exception of one > occurrence we know in what function-type we are (sync*, async or async*). > > It would be more readable to have different fields. > > Potentially even create a BaseRewriter, and then only overload the functions > that have anything to do with async. > That would also allow to have better documentation, since the functions don't > need to explain their implementation for all 3 different cases. Good idea - I also split the "controller" to a controller and a completer. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:68: final Expression thenHelper; On 2015/02/04 17:06:09, floitsch wrote: > I believe I have written it somewhere else: this is not just a "then"-helper > anymore. Acknowledged. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:388: /// (the result might been stored in [returnValueName] by some finally block). On 2015/02/04 17:06:09, floitsch wrote: > might have been Done. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:390: /// Returning from a sync* will return a [endOfIteration] marker. On 2015/02/04 17:06:10, floitsch wrote: > Returning from a sync* returns an [endOfIteration] marker. Done. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:391: void addReturn() { On 2015/02/04 17:06:09, floitsch wrote: > Comments don't mention async* Done. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:397: switch (async) { On 2015/02/04 17:06:10, floitsch wrote: > How do these guarantee that we go through the finallies? > > For example: > foo() sync* { > try { > return; > } finally { > yield 499; > } > } > > There are even cases, where the finally stops the return: > > foo() sync* { > while (true) { > try { > return; > } finally { > yield 499; > continue; > } > } > } > > If you don't handle these cases yet, just add a test and fix it in a later CL. This only generates the return that all returns will eventually jump to. visitReturn generates the jump that goes through the finally blocks. I tried to improve the comment to clarify. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:419: /// Initializing a controller. (This is a Completer for async, On 2015/02/04 17:06:10, floitsch wrote: > Initializes the controller/completer. > The comment here was wrong - I have tried to rewrite. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:439: visitFun(Fun node) { On 2015/02/04 17:06:09, floitsch wrote: > Add long comment on how you translate from one to the other. With (simplified) > examples. Done. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:444: // [visitYield]. On 2015/02/04 12:31:30, floitsch wrote: > visitDartYield Done. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:513: """, {"params": node.params, On 2015/02/04 17:06:10, floitsch wrote: > move "params" into the next line or align the others. Done. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:514: "helperBody": helperBody, On 2015/02/04 17:06:10, floitsch wrote: > In a follow-up CL: rename this to something else "helper" is overused. > > (I would have also expected that the one in js_helper is the "helper"...) Acknowledged. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:579: Expression visitAwait(Await node) { On 2015/02/04 17:06:09, floitsch wrote: > Add comment explaining what you do here. > Ideally with an example (including the switch around it). I refer to the visitFun documentation. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1354: visitDartYield(DartYield node) { On 2015/02/04 17:06:10, floitsch wrote: > Add doc what's happening here. > Ideally with examples. Done - no examples though https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1384: if (#notEmptyFinallyList) { On 2015/02/04 17:06:10, floitsch wrote: > remove {}. > (It's stupid, but otherwise they stay in the output). Done. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1397: (int label) => new LiteralNumber("$label")).toList()), On 2015/02/04 17:06:10, floitsch wrote: > js.number(label) Done (here and elsewhere) https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1398: "destinationOnCancel": new LiteralNumber("$destinationOnCancel") On 2015/02/04 17:06:10, floitsch wrote: > js.number(destinationOnCancel) Done. https://codereview.chromium.org/839323003/diff/360001/sdk/lib/_internal/compi... File sdk/lib/_internal/compiler/js_lib/js_helper.dart (right): https://codereview.chromium.org/839323003/diff/360001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3543: /// stream has been canceled call the errorCallBack. On 2015/02/04 17:06:10, floitsch wrote: > errorCallback Done.
New patchsets have been uploaded after l-g-t-m from floitsch@google.com
Still lots of comments, but as usual many are just nits (and/or comment changes). I think the catch-translation is not right, though. Otherwise LGTM. https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... File pkg/compiler/lib/src/js/rewrite_async.dart (right): https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:13: import "js.dart"; On 2015/02/05 14:06:07, sigurdm wrote: > On 2015/02/04 17:06:10, floitsch wrote: > > Since you export without a prefix, you should "show" the classes/statics that > > you want to see. > > The js-library is relatively nasty: it has "number", "string" as top-level > > variables. > > Hurray for quick-fixes! > > There is a lot of used names. > Maybe it is better to go with a prefix? Hm. probably yes. In other components it's usally "js" or "jsAst". https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:68: final Expression thenHelper; On 2015/02/05 14:06:07, sigurdm wrote: > On 2015/02/04 17:06:10, floitsch wrote: > > I'm not sure it's worth to have only one variable. With the exception of one > > occurrence we know in what function-type we are (sync*, async or async*). > > > > It would be more readable to have different fields. > > > > Potentially even create a BaseRewriter, and then only overload the functions > > that have anything to do with async. > > That would also allow to have better documentation, since the functions don't > > need to explain their implementation for all 3 different cases. > > Good idea - I also split the "controller" to a controller and a completer. The more I think about this, I think that we should create three different rewriters (with one base class). In a future CL. (I think I will come see you in person...) https://codereview.chromium.org/839323003/diff/360001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:419: /// Initializing a controller. (This is a Completer for async, On 2015/02/05 14:06:07, sigurdm wrote: > On 2015/02/04 17:06:10, floitsch wrote: > > Initializes the controller/completer. > > > > The comment here was wrong - I have tried to rewrite. better. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/cl... File pkg/compiler/lib/src/closure.dart (right): https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/cl... pkg/compiler/lib/src/closure.dart:354: /// Also parameters to a `sync*` generator must be boxed, because of the way we long line. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... File pkg/compiler/lib/src/js/rewrite_async.dart (right): https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:132: /// It is a parameter to the [helperName] function. Doesn't really help in understanding what it's for. If it really is only for the three cases (awaited expression, conditional or lazy boolean operator), maybe give examples? Why is it a parameter to the helper? https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:133: String resultName; Future CL: consider: js.Expression result = uncachedExpressionTemplate(resultName); Basically these could be the only JS expressions that are uncached, and then it's not the "name" anymore, either. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:136: /// and called to do a new iteation for sync*. iteration https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:139: /// The Completer that will finish an async function. What if we are in a sync* or async* ? https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:142: /// The StreamController that controls an async* function. What if we are in a sync* or async function? https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:185: /// - A Javascript function that will take the program to the right error that takes... It's not just taking the program to the right error-handler. It is executing the helper. Maybe just "A JavaScript function that is executed if the future completed with an error". That function is responsible for executing the right error handler (and/or finallies). https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:204: /// - The [completerName] controllerName ? https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:205: /// - A Javascript function that will take the program to the right error ditto. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:216: /// How to initialize the [completerName] variable. It's not a "how". The constructor that is used to initialize the [completerName] variable. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:221: /// How to initialize the [controllerName] variable. ditto. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:224: final Expression newController; It's inconsistent that "newCompleter" and newIterable are constructors but "newController" apparently is a static function. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:226: /// Creates an Iterable for a sync* method. Called with [helperName] Missing trailing ".". https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:229: /// Creates a marker showing that iteration is over. A JS Expression that creates ... https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:234: /// Creates a marker indicating a 'yield' statement. ditto. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:239: /// Creates a marker indication a 'yield*' statement. ditto. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:400: /// Return a block that has a goto to [label] including the break. Returns https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:418: /// Add a goto to [label] including the break. Adds https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:511: /// This is because node1 must be evaluated before visiting node2, I'm not sure I fully understand the comment. My best attempt: Calls [fn] with the value of evaluating [node1] and [node2]. Both nodes are evaluated in order. If node2 must be transformed (see [shouldTransform]), then the evaluation of node1 is added to the current statement-list and the result is stored in a temporary variable. The evaluation of node2 is then free to emit statements without affecting the result of node1. This is necessary, because await or yield expressions have to emit statements, and these statements could affect the value of node1. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:566: /// through all the enclosing finally blocks) the jump to here is made in blocks). The jump ... https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:577: void addReturn() { addFooter ? addExit ? addFunctionExit ? (`addReturn` is probably fine, but I have to think of a return statement, which confused me last time). https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:636: /// Awaits and yields in async/async* are simulated by calling are translated to a call to ... Similarly remove "simulate" in the rest. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:636: /// Awaits and yields in async/async* are simulated by calling Maybe: Awaits in async/async* are translated to code that remembers the current location (so the function can resume from where it was) followed by a call to the [thenHelper]. The helper sets up the waiting for the awaited value and returns a future which is immediately returned by the translated await. Yields in async* are translated to a call to the [streamHelper]. They, too, need to be prepared to be interrupted in case the stream is paused. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:639: /// Yield/yield* in a sync* function is simulated by returning the value. Yield/yield* in a sync* function is translated to a return of the value, wrapped into a "IterationMarker" that signals the type (yield or yield*). Sync* functions are executed on demand (when the user requests a value) by the Iterable which knows how to handle these values. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:657: /// goto = 1 Add comment, that "goto = 1" remembers the location of where to continue. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:676: /// Finally is implemented by letting the translation of each flow-path that `Finally` https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:676: /// Finally is implemented by letting the translation of each flow-path that A `finally` clause is compiled similar to normal code, with the additional complexity that `finally` clauses need to know where to jump to after the clause is done. In the translation, each flow-path that enters a `finally` sets up the variable [nextName] with a stack of finallies and a final jump-target (exit, catch, ...). https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:703: /// function helper(result) { comment that "result" could be either the value of the awaited future, or an error in error-case. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:711: /// next = [3]; // After the finally (2) continue normally after the try. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:714: /// case 1: // catch handler for inner try (implicit). (implicit) catch handler for the inner try. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:715: /// next = [3]; // destination after the finally. why is this not "next = [4]" ? If we enter this exception handler, we are encountering an exception, so we have to eventually reach the outer exception handler (which seems to be "4"). https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:724: /// goto = 5; // finally handler for outer try. Why is there no break? https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:728: /// goto = 4; why is this here? It looks unnecessary anyway. Even if we happen to generate it, keep it out (unless there is a meaning to it). // Fall through to finally. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1078: /// the target. For that [nextName] is used as a stack of places to go. Maybe reference visitFun. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1197: // No condition is the same as true. So the check is not needed. I'm not sure I understand the comment. Remove? https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1538: void visitTry(Try node) { Reference visitFun. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1558: setHandler(); maybe rename to setErrorHandler https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1578: // is needed to avoid collisions. collisions. See Ecma 262, 3rd edition, section 12.14. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1595: // if an exception is raised the finally part has to be run. I don't get this. If the try-body executes without a throw it sets the nextName in 1656 (to afterFinallyLabel). So for non-erroneous execution the nextName is already set up. That means that we only need to set it to 'after-finally' for the catch, so we should be able to just do it as last instruction in the 'catch' section. What is missing, though: how does a catch-clause reach it's finally if it throws itself. I believe it has to set up a $nextName to point to the current finally followed by the next catch-handler (possibly passing through other finallies). Basically: <try>: handler = <handlerId>; ... nextTargets = [<afterFinally>] <catch>: handler = <finally>; // Go to finally if we throw. nextTargets = [<otherFinally>, <outerCatch>]; // And then to the next catch. <if explicit catch clause> catchHandler(); nextTargets = [<afterFinally>] <endif> <finally>: handler = <outerHandler>; https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1675: /// Translates a yield/yield* in an sync* function. in a sync* https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1701: List<int> enclosingFinallyLabels = <int>[returnLabel]; I think "exitLabel" would read easier. (Here in the other places). https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1708: .map((int label) => number(label)) .map(number).toList() and yes: we should have a prefix for the js-library. https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... File sdk/lib/_internal/compiler/js_lib/js_helper.dart (right): https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:28: import 'dart:async' maybe not your code, but should be easy: move "show" to the import line. (see below for the foreign_helper). and sort the shows. https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3523: /// When the async* function wants to return it call this with calls this functions. The streamHelper takes this as signal to close the stream. https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3526: /// If the async* function wants to do a yield or yield* it will call this with it calls this function https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3533: /// If [object] is a single-yield [IterationMarker], add the value of the adds https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3534: /// [IterationMarker] to the stream, next: if the stream subscription has been to the stream. If the stream... (unless I'm misunderstanding something). https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3538: /// If [object] is a yield-star [IterationMarker], start listening to the starts ... and adds ... schedules https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3543: /// If the async* function wants to do an await it will call this with [object] calls this function with [object] not an [IterationMarker] ? https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3547: /// Then set up [helperCallback] to be called on successfull completion of the The [helperCallback] is called on successful completion of the future. https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3572: new Future.value(null).then( Why? Do we have to wait a microtask before we can continue? If yes, just use the scheduleMicrotask instead of a Future.value (unless I'm misunderstanding). https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3578: controller.addStream(stream).then((_) { You might need to listen to the error too. I'm not sure if that can happen, or if this future is always without error. If it never has an error (because the errors are just piped through) then add a comment. Please add it to your TODO list, to determine if errors from yield* are canceling the stream, or just passed through verbatim. (For now doing what `addStream` does, is enough). https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3607: return controller.addStream(stream, cancelOnError: false); I see. So I guess the future cannot have an error. https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3615: if (!isAdding) { Add comment. https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3615: if (!isAdding) { Check with Lasse, if 'isPaused' on the controller is as good as having an "onPause" callback. https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3619: stopRunning = true; Please make sure (test), that an incoming stream is correctly canceled. That is, if you cancel during a yield* that the source-stream is canceled. https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3650: static yieldStar(dynamic /* Iterable or Stream */ yielded) { maybe just "values" ? https://codereview.chromium.org/839323003/diff/380001/tests/compiler/dart2js/... File tests/compiler/dart2js/async_await_js_transform_test.dart (right): https://codereview.chromium.org/839323003/diff/380001/tests/compiler/dart2js/... tests/compiler/dart2js/async_await_js_transform_test.dart:120: __next = [11]; The example doesn't have an explicit 'catch', and yet we continue immediately after the finally (at [11]), instead of jumping to '[2]'. In the example it doesn't influence the result, since everything basically ends up at the loop-header, but I believe the translation for try/catch is wrong (see my comments in the translator).
New patchsets have been uploaded after l-g-t-m from floitsch@google.com
https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/cl... File pkg/compiler/lib/src/closure.dart (right): https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/cl... pkg/compiler/lib/src/closure.dart:354: /// Also parameters to a `sync*` generator must be boxed, because of the way we On 2015/02/05 20:17:20, floitsch wrote: > long line. Done. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... File pkg/compiler/lib/src/js/rewrite_async.dart (right): https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:132: /// It is a parameter to the [helperName] function. On 2015/02/05 20:17:22, floitsch wrote: > Doesn't really help in understanding what it's for. > > If it really is only for the three cases (awaited expression, conditional or > lazy boolean operator), maybe give examples? > > Why is it a parameter to the helper? Tried to give an example. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:133: String resultName; On 2015/02/05 20:17:22, floitsch wrote: > Future CL: > > consider: > js.Expression result = uncachedExpressionTemplate(resultName); > > Basically these could be the only JS expressions that are uncached, and then > it's not the "name" anymore, either. I started doing something like this, but one problem is that the js template system does not allow holes in declaration positions, so I cannot have: js("function #helper() { ... }") https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:136: /// and called to do a new iteation for sync*. On 2015/02/05 20:17:21, floitsch wrote: > iteration Done. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:139: /// The Completer that will finish an async function. On 2015/02/05 20:17:21, floitsch wrote: > What if we are in a sync* or async* ? Done. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:142: /// The StreamController that controls an async* function. On 2015/02/05 20:17:21, floitsch wrote: > What if we are in a sync* or async function? Done. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:185: /// - A Javascript function that will take the program to the right error On 2015/02/05 20:17:22, floitsch wrote: > that takes... > > It's not just taking the program to the right error-handler. It is executing the > helper. > > Maybe just "A JavaScript function that is executed if the future completed with > an error". That function is responsible for executing the right error handler > (and/or finallies). Done. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:204: /// - The [completerName] On 2015/02/05 20:17:20, floitsch wrote: > controllerName ? Done. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:205: /// - A Javascript function that will take the program to the right error On 2015/02/05 20:17:21, floitsch wrote: > ditto. Done. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:216: /// How to initialize the [completerName] variable. On 2015/02/05 20:17:22, floitsch wrote: > It's not a "how". > > The constructor that is used to initialize the [completerName] variable. Done. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:221: /// How to initialize the [controllerName] variable. On 2015/02/05 20:17:21, floitsch wrote: > ditto. Done. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:224: final Expression newController; On 2015/02/05 20:17:21, floitsch wrote: > It's inconsistent that "newCompleter" and newIterable are constructors but > "newController" apparently is a static function. true. Made them all constructors. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:226: /// Creates an Iterable for a sync* method. Called with [helperName] On 2015/02/05 20:17:22, floitsch wrote: > Missing trailing ".". Done. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:229: /// Creates a marker showing that iteration is over. On 2015/02/05 20:17:21, floitsch wrote: > A JS Expression that creates ... Done. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:234: /// Creates a marker indicating a 'yield' statement. On 2015/02/05 20:17:21, floitsch wrote: > ditto. Done. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:239: /// Creates a marker indication a 'yield*' statement. On 2015/02/05 20:17:21, floitsch wrote: > ditto. Done. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:400: /// Return a block that has a goto to [label] including the break. On 2015/02/05 20:17:22, floitsch wrote: > Returns Done. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:418: /// Add a goto to [label] including the break. On 2015/02/05 20:17:22, floitsch wrote: > Adds Done. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:511: /// This is because node1 must be evaluated before visiting node2, On 2015/02/05 20:17:21, floitsch wrote: > I'm not sure I fully understand the comment. My best attempt: > > Calls [fn] with the value of evaluating [node1] and [node2]. > > Both nodes are evaluated in order. > > If node2 must be transformed (see [shouldTransform]), then the evaluation of > node1 is added to the current statement-list and the result is stored in a > temporary variable. The evaluation of node2 is then free to emit statements > without affecting the result of node1. > > This is necessary, because await or yield expressions have to emit statements, > and these statements could affect the value of node1. Better. Thanks. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:566: /// through all the enclosing finally blocks) the jump to here is made in On 2015/02/05 20:17:22, floitsch wrote: > blocks). The jump ... Done. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:577: void addReturn() { On 2015/02/05 20:17:22, floitsch wrote: > addFooter ? > addExit ? > addFunctionExit ? > > (`addReturn` is probably fine, but I have to think of a return statement, which > confused me last time). I go with addExit. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:636: /// Awaits and yields in async/async* are simulated by calling On 2015/02/05 20:17:21, floitsch wrote: > Maybe: > Awaits in async/async* are translated to code that remembers the current > location (so the function can resume from where it was) followed by a call to > the [thenHelper]. The helper sets up the waiting for the awaited value and > returns a future which is immediately returned by the translated await. > > Yields in async* are translated to a call to the [streamHelper]. They, too, need > to be prepared to be interrupted in case the stream is paused. Better, thanks. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:636: /// Awaits and yields in async/async* are simulated by calling On 2015/02/05 20:17:21, floitsch wrote: > are translated to a call to ... > > Similarly remove "simulate" in the rest. Done. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:639: /// Yield/yield* in a sync* function is simulated by returning the value. On 2015/02/05 20:17:21, floitsch wrote: > Yield/yield* in a sync* function is translated to a return of the value, wrapped > into a "IterationMarker" that signals the type (yield or yield*). Sync* > functions are executed on demand (when the user requests a value) by the > Iterable which knows how to handle these values. Done. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:657: /// goto = 1 On 2015/02/05 20:17:21, floitsch wrote: > Add comment, that "goto = 1" remembers the location of where to continue. Done. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:676: /// Finally is implemented by letting the translation of each flow-path that On 2015/02/05 20:17:21, floitsch wrote: > A `finally` clause is compiled similar to normal code, with the additional > complexity that `finally` clauses need to know where to jump to after the clause > is done. In the translation, each flow-path that enters a `finally` sets up the > variable [nextName] with a stack of finallies and a final jump-target (exit, > catch, ...). Done. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:676: /// Finally is implemented by letting the translation of each flow-path that On 2015/02/05 20:17:22, floitsch wrote: > `Finally` Acknowledged. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:703: /// function helper(result) { On 2015/02/05 20:17:23, floitsch wrote: > comment that "result" could be either the value of the awaited future, or an > error in error-case. Done. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:711: /// next = [3]; On 2015/02/05 20:17:20, floitsch wrote: > // After the finally (2) continue normally after the try. Done. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:714: /// case 1: // catch handler for inner try (implicit). On 2015/02/05 20:17:20, floitsch wrote: > (implicit) catch handler for the inner try. Done. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:715: /// next = [3]; // destination after the finally. On 2015/02/05 20:17:22, floitsch wrote: > why is this not "next = [4]" ? > If we enter this exception handler, we are encountering an exception, so we have > to eventually reach the outer exception handler (which seems to be "4"). Good point, this is a bug indeed. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:724: /// goto = 5; // finally handler for outer try. On 2015/02/05 20:17:22, floitsch wrote: > Why is there no break? It is a mistake. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:728: /// goto = 4; On 2015/02/05 20:17:21, floitsch wrote: > why is this here? > It looks unnecessary anyway. Even if we happen to generate it, keep it out > (unless there is a meaning to it). > > // Fall through to finally. Another mistake https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1078: /// the target. For that [nextName] is used as a stack of places to go. On 2015/02/05 20:17:21, floitsch wrote: > Maybe reference visitFun. Done. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1197: // No condition is the same as true. So the check is not needed. On 2015/02/05 20:17:20, floitsch wrote: > I'm not sure I understand the comment. > Remove? Done. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1538: void visitTry(Try node) { On 2015/02/05 20:17:21, floitsch wrote: > Reference visitFun. Done. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1558: setHandler(); On 2015/02/05 20:17:22, floitsch wrote: > maybe rename to setErrorHandler Done. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1578: // is needed to avoid collisions. On 2015/02/05 20:17:22, floitsch wrote: > collisions. See Ecma 262, 3rd edition, section 12.14. Done. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1595: // if an exception is raised the finally part has to be run. On 2015/02/05 20:17:21, floitsch wrote: > I don't get this. > > If the try-body executes without a throw it sets the nextName in 1656 (to > afterFinallyLabel). > So for non-erroneous execution the nextName is already set up. > That means that we only need to set it to 'after-finally' for the catch, so we > should be able to just do it as last instruction in the 'catch' section. > > What is missing, though: how does a catch-clause reach it's finally if it throws > itself. > I believe it has to set up a $nextName to point to the current finally followed > by the next catch-handler (possibly passing through other finallies). > > Basically: > > <try>: > handler = <handlerId>; > ... > nextTargets = [<afterFinally>] > <catch>: > handler = <finally>; // Go to finally if we throw. > nextTargets = [<otherFinally>, <outerCatch>]; // And then to the next catch. > <if explicit catch clause> > catchHandler(); > nextTargets = [<afterFinally>] > <endif> > <finally>: > handler = <outerHandler>; I agree - this is a bug. Thanks for spotting. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1675: /// Translates a yield/yield* in an sync* function. On 2015/02/05 20:17:21, floitsch wrote: > in a sync* Done. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1701: List<int> enclosingFinallyLabels = <int>[returnLabel]; On 2015/02/05 20:17:22, floitsch wrote: > I think "exitLabel" would read easier. (Here in the other places). Done. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1708: .map((int label) => number(label)) On 2015/02/05 20:17:22, floitsch wrote: > .map(number).toList() > > and yes: we should have a prefix for the js-library. Done. https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... File sdk/lib/_internal/compiler/js_lib/js_helper.dart (right): https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:28: import 'dart:async' On 2015/02/05 20:17:23, floitsch wrote: > maybe not your code, but should be easy: > move "show" to the import line. (see below for the foreign_helper). > and sort the shows. Done. https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3523: /// When the async* function wants to return it call this with On 2015/02/05 20:17:23, floitsch wrote: > calls this functions. The streamHelper takes this as signal to close the stream. Done. https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3526: /// If the async* function wants to do a yield or yield* it will call this with On 2015/02/05 20:17:23, floitsch wrote: > it calls this function Done. https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3533: /// If [object] is a single-yield [IterationMarker], add the value of the On 2015/02/05 20:17:23, floitsch wrote: > adds Done. https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3534: /// [IterationMarker] to the stream, next: if the stream subscription has been On 2015/02/05 20:17:23, floitsch wrote: > to the stream. If the stream... > > (unless I'm misunderstanding something). Done. https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3538: /// If [object] is a yield-star [IterationMarker], start listening to the On 2015/02/05 20:17:23, floitsch wrote: > starts ... and adds ... schedules Done. https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3543: /// If the async* function wants to do an await it will call this with [object] On 2015/02/05 20:17:23, floitsch wrote: > calls this function with [object] not an [IterationMarker] ? Done. https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3547: /// Then set up [helperCallback] to be called on successfull completion of the On 2015/02/05 20:17:23, floitsch wrote: > The [helperCallback] is called on successful completion of the future. Done. https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3572: new Future.value(null).then( On 2015/02/05 20:17:23, floitsch wrote: > Why? > Do we have to wait a microtask before we can continue? I thought that it was necessary to suspend execution when yielding. But I was wrong. Execution should just continue. I leave it for now, with a TODO. > If yes, just use the scheduleMicrotask instead of a Future.value (unless I'm > misunderstanding). It was a hack, because _wrapJsFunctionForStream() takes one argument. Fixed. https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3578: controller.addStream(stream).then((_) { On 2015/02/05 20:17:23, floitsch wrote: > You might need to listen to the error too. I'm not sure if that can happen, or > if this future is always without error. > If it never has an error (because the errors are just piped through) then add a > comment. As you saw below, I explicitly ask errors to be passed through. Added a comment. > > Please add it to your TODO list, to determine if errors from yield* are > canceling the stream, or just passed through verbatim. (For now doing what > `addStream` does, is enough). As far as I understand the spec, errors are passed through like we do here. But the spec just says: ..., Next, for each element x of o: ... Not mentioning errors explicitly. We should mention this to Gilad. Added TODO. https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3607: return controller.addStream(stream, cancelOnError: false); On 2015/02/05 20:17:23, floitsch wrote: > I see. So I guess the future cannot have an error. Done. https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3615: if (!isAdding) { On 2015/02/05 20:17:23, floitsch wrote: > Check with Lasse, if 'isPaused' on the controller is as good as having an > "onPause" callback. According to him, it is just as good. https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3615: if (!isAdding) { On 2015/02/05 20:17:23, floitsch wrote: > Add comment. Done. https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3619: stopRunning = true; On 2015/02/05 20:17:23, floitsch wrote: > Please make sure (test), that an incoming stream is correctly canceled. That is, > if you cancel during a yield* that the source-stream is canceled. Modified asyncstar_yieldstar_test to do that. https://codereview.chromium.org/839323003/diff/380001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3650: static yieldStar(dynamic /* Iterable or Stream */ yielded) { On 2015/02/05 20:17:23, floitsch wrote: > maybe just "values" ? Done. https://codereview.chromium.org/839323003/diff/380001/tests/compiler/dart2js/... File tests/compiler/dart2js/async_await_js_transform_test.dart (right): https://codereview.chromium.org/839323003/diff/380001/tests/compiler/dart2js/... tests/compiler/dart2js/async_await_js_transform_test.dart:120: __next = [11]; On 2015/02/05 20:17:23, floitsch wrote: > The example doesn't have an explicit 'catch', and yet we continue immediately > after the finally (at [11]), instead of jumping to '[2]'. > In the example it doesn't influence the result, since everything basically ends > up at the loop-header, but I believe the translation for try/catch is wrong (see > my comments in the translator). Acknowledged.
almost no comments left. LGTM. https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... File pkg/compiler/lib/src/js/rewrite_async.dart (right): https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:132: /// It is a parameter to the [helperName] function. On 2015/02/06 14:26:32, sigurdm wrote: > On 2015/02/05 20:17:22, floitsch wrote: > > Doesn't really help in understanding what it's for. > > > > If it really is only for the three cases (awaited expression, conditional or > > lazy boolean operator), maybe give examples? > > > > Why is it a parameter to the helper? > > Tried to give an example. Never mind. I was again confused by the similarity in names of "thenHelper" and "helperName". https://codereview.chromium.org/839323003/diff/420001/pkg/compiler/lib/src/js... File pkg/compiler/lib/src/js/rewrite_async.dart (right): https://codereview.chromium.org/839323003/diff/420001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:179: /// Initializes the [completerName] variable. I don't think that's correct. It's the constructor that is used to initialize the completerName. https://codereview.chromium.org/839323003/diff/420001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:184: /// Initializes the [controllerName] variable. Not correct either. It's a JS Expression (in this case a static-call target) that returns a new controller which is then used to initialize [controllerName]. https://codereview.chromium.org/839323003/diff/420001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:189: /// Creates an Iterable for a sync* method. Called with [helperName]. ditto. This seems to be a constructor. https://codereview.chromium.org/839323003/diff/420001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:613: /// Yields in async* are translated to a call to the [streamHelper]. They, new line before. Actually currently they always interrupt. (I'm ok with keeping the comment until we have completely figured out what we want/need to do). https://codereview.chromium.org/839323003/diff/420001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1061: /// See also [visitFun]. New line before. https://codereview.chromium.org/839323003/diff/420001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1438: bool allCaseExpressionsUntransformed = !node.cases.every( put the "!node.cases.every(" into the next line. https://codereview.chromium.org/839323003/diff/420001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1438: bool allCaseExpressionsUntransformed = !node.cases.every( Remove the leading "!". Eventually there should be a test for this. (ok with next CL). https://codereview.chromium.org/839323003/diff/420001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1438: bool allCaseExpressionsUntransformed = !node.cases.every( alternatively: !node.cases.any((js.SwitchClause x) { return x is js.Case && shouldTransform(x.expression); });
New patchsets have been uploaded after l-g-t-m from floitsch@google.com
https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... File pkg/compiler/lib/src/js/rewrite_async.dart (right): https://codereview.chromium.org/839323003/diff/380001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:132: /// It is a parameter to the [helperName] function. On 2015/02/06 14:58:43, floitsch wrote: > On 2015/02/06 14:26:32, sigurdm wrote: > > On 2015/02/05 20:17:22, floitsch wrote: > > > Doesn't really help in understanding what it's for. > > > > > > If it really is only for the three cases (awaited expression, conditional or > > > lazy boolean operator), maybe give examples? > > > > > > Why is it a parameter to the helper? > > > > Tried to give an example. > > Never mind. > I was again confused by the similarity in names of "thenHelper" and > "helperName". Acknowledged. https://codereview.chromium.org/839323003/diff/420001/pkg/compiler/lib/src/js... File pkg/compiler/lib/src/js/rewrite_async.dart (right): https://codereview.chromium.org/839323003/diff/420001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:179: /// Initializes the [completerName] variable. On 2015/02/06 14:58:43, floitsch wrote: > I don't think that's correct. > It's the constructor that is used to initialize the completerName. Done. https://codereview.chromium.org/839323003/diff/420001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:184: /// Initializes the [controllerName] variable. On 2015/02/06 14:58:43, floitsch wrote: > Not correct either. > > It's a JS Expression (in this case a static-call target) that returns a new > controller which is then used to initialize [controllerName]. Done. https://codereview.chromium.org/839323003/diff/420001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:189: /// Creates an Iterable for a sync* method. Called with [helperName]. On 2015/02/06 14:58:44, floitsch wrote: > ditto. This seems to be a constructor. Done. https://codereview.chromium.org/839323003/diff/420001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:613: /// Yields in async* are translated to a call to the [streamHelper]. They, On 2015/02/06 14:58:44, floitsch wrote: > new line before. > > Actually currently they always interrupt. (I'm ok with keeping the comment until > we have completely figured out what we want/need to do). Done. https://codereview.chromium.org/839323003/diff/420001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1061: /// See also [visitFun]. On 2015/02/06 14:58:43, floitsch wrote: > New line before. Done. https://codereview.chromium.org/839323003/diff/420001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1438: bool allCaseExpressionsUntransformed = !node.cases.every( On 2015/02/06 14:58:43, floitsch wrote: > alternatively: > !node.cases.any((js.SwitchClause x) { > return x is js.Case && shouldTransform(x.expression); > }); Acknowledged. https://codereview.chromium.org/839323003/diff/420001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1438: bool allCaseExpressionsUntransformed = !node.cases.every( On 2015/02/06 14:58:44, floitsch wrote: > put the "!node.cases.every(" into the next line. Done. https://codereview.chromium.org/839323003/diff/420001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:1438: bool allCaseExpressionsUntransformed = !node.cases.every( On 2015/02/06 14:58:43, floitsch wrote: > Remove the leading "!". > > Eventually there should be a test for this. (ok with next CL). The variable was named wrongly.
Made a change to how "this" is cached for sync* functions. PTAL
Made a change to how "this" is cached for sync* functions. PTAL
Made a change to how "this" is cached for sync* functions. PTAL
Still LGTM. https://codereview.chromium.org/839323003/diff/460001/pkg/compiler/lib/src/js... File pkg/compiler/lib/src/js/rewrite_async.dart (right): https://codereview.chromium.org/839323003/diff/460001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:179: /// Contructor used to initializes the [completerName] variable. initialize https://codereview.chromium.org/839323003/diff/460001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:184: /// Contructor used to initializes the [controllerName] variable. initialize https://codereview.chromium.org/839323003/diff/460001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:616: /// canceled. (Currently we always suspend - this is different from the spec, Missing closing parenthesis. https://codereview.chromium.org/839323003/diff/460001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:794: // Sync* functions must remember this on the level of the outer function. `this` https://codereview.chromium.org/839323003/diff/460001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:806: if (#needsThis) { Nit: Remove '{' and '}'. It's a limitation of the template-system. It will keep the "{}" otherwise.
New patchsets have been uploaded after l-g-t-m from floitsch@google.com
Message was sent while issue was closed.
Committed patchset #24 (id:480001) manually as 43591 (presubmit successful).
Message was sent while issue was closed.
Forgot to post my last review responses https://codereview.chromium.org/839323003/diff/460001/pkg/compiler/lib/src/js... File pkg/compiler/lib/src/js/rewrite_async.dart (right): https://codereview.chromium.org/839323003/diff/460001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:179: /// Contructor used to initializes the [completerName] variable. On 2015/02/09 11:53:57, floitsch wrote: > initialize Done. https://codereview.chromium.org/839323003/diff/460001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:184: /// Contructor used to initializes the [controllerName] variable. On 2015/02/09 11:53:58, floitsch wrote: > initialize Done. https://codereview.chromium.org/839323003/diff/460001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:616: /// canceled. (Currently we always suspend - this is different from the spec, On 2015/02/09 11:53:58, floitsch wrote: > Missing closing parenthesis. Done. https://codereview.chromium.org/839323003/diff/460001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:794: // Sync* functions must remember this on the level of the outer function. On 2015/02/09 11:53:57, floitsch wrote: > `this` Done. https://codereview.chromium.org/839323003/diff/460001/pkg/compiler/lib/src/js... pkg/compiler/lib/src/js/rewrite_async.dart:806: if (#needsThis) { On 2015/02/09 11:53:57, floitsch wrote: > Nit: Remove '{' and '}'. > > It's a limitation of the template-system. It will keep the "{}" otherwise. Done.
Message was sent while issue was closed.
lrn@google.com changed reviewers: + lrn@google.com
Message was sent while issue was closed.
Old unsent comments. https://codereview.chromium.org/839323003/diff/360001/sdk/lib/_internal/compi... File sdk/lib/_internal/compiler/js_lib/js_helper.dart (right): https://codereview.chromium.org/839323003/diff/360001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:30: DeferredLoadException, Indent to align with Future? Or at least by four. https://codereview.chromium.org/839323003/diff/360001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3497: Future future = object is Future ? object : new Future.value(object); Maybe just: if (object is Future) { object.then(...); } else { _wrapJsFunction(helperCallback, completer)(object); } return completer.future;
Message was sent while issue was closed.
https://codereview.chromium.org/839323003/diff/360001/sdk/lib/_internal/compi... File sdk/lib/_internal/compiler/js_lib/js_helper.dart (right): https://codereview.chromium.org/839323003/diff/360001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:30: DeferredLoadException, On 2015/02/13 11:13:20, Lasse Reichstein Nielsen wrote: > Indent to align with Future? > Or at least by four. Will do in future CL https://codereview.chromium.org/839323003/diff/360001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3497: Future future = object is Future ? object : new Future.value(object); On 2015/02/13 11:13:20, Lasse Reichstein Nielsen wrote: > Maybe just: > > if (object is Future) { > object.then(...); > } else { > _wrapJsFunction(helperCallback, completer)(object); > } > return completer.future; > That would not suspend execution? The spec is specific 16.29: "Otherwise, if e evaluates to an object o that is not an instance of Future, then let f be the result of calling Future.value() with o as its argument; otherwise let f be the result of evaluating e." |
