dart2dart: Implementation of simple try/catch.

pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart

Index: pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart
diff --git a/pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart b/pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart
index 2f7e5fe7e2e94fdfd1e17765259374304641d032..5304abe1366298d212eb235723ffa464d8d5babc 100644
--- a/pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart
+++ b/pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart
@@ -57,6 +57,12 @@ class Builder extends cps_ir.Visitor<Node> {
   // is the mapping from continuations to labels.
   final Map<cps_ir.Continuation, Label> labels = <cps_ir.Continuation, Label>{};
 
+  // Code for continuations with exactly one use is inlined at the use site.

[karlklose, 2015/02/16 10:15:48]

Use '///'.

+  // This is not safe if the code is moved inside the scope of an exception
+  // handler (i.e., into a try block).  We keep a stack of singly-referenced
+  // continuations that are in scope without crossing a binding for a handler.
+  List<cps_ir.Continuation> safeForHandlers = <cps_ir.Continuation>[];
+
   ExecutableElement currentElement;
   cps_ir.Continuation returnContinuation;
 
@@ -370,12 +376,16 @@ class Builder extends cps_ir.Visitor<Node> {
 
   Statement visitLetCont(cps_ir.LetCont node) {
     // Introduce labels for continuations that need them.
+    int safeForHandlersLengthOnEntry = safeForHandlers.length;
     for (cps_ir.Continuation continuation in node.continuations) {
       if (continuation.hasMultipleUses) {
         labels[continuation] = new Label();
+      } else {
+        safeForHandlers.add(continuation);
       }
     }
     Statement body = visit(node.body);
+    safeForHandlers.length = safeForHandlersLengthOnEntry;
     // Continuations are bound at the same level, but they have to be
     // translated as if nested.  This is because the body can invoke any
     // of them from anywhere, so it must be nested inside all of them.
@@ -399,6 +409,17 @@ class Builder extends cps_ir.Visitor<Node> {
     return current;
   }
 
+  Statement visitLetHandler(cps_ir.LetHandler node) {
+    List<cps_ir.Continuation> saved = safeForHandlers;
+    safeForHandlers = <cps_ir.Continuation>[];
+    Statement tryBody = visit(node.body);
+    safeForHandlers = saved;
+    List<Variable> catchParameters =
+        node.handler.parameters.map(getVariable).toList();
+    Statement catchBody = visit(node.handler.body);
+    return new TryStatement(tryBody, catchParameters, catchBody);
+  }
+
   Statement visitInvokeStatic(cps_ir.InvokeStatic node) {
     // Calls are translated to direct style.
     List<Expression> arguments = translateArguments(node.arguments);
@@ -507,9 +528,13 @@ class Builder extends cps_ir.Visitor<Node> {
                   ? new Continue(labels[cont])
                   : new WhileTrue(labels[cont], visit(cont.body));
             } else {
-              return cont.hasExactlyOneUse
-                  ? visit(cont.body)
-                  : new Break(labels[cont]);
+              if (cont.hasExactlyOneUse) {
+                if (safeForHandlers.contains(cont)) {
+                  return visit(cont.body);
+                }
+                labels[cont] = new Label();

[asgerf, 2015/02/20 10:10:08]

I think you should create the Label inside visitLetHandler. Seems more
consistent since visitLetCont creates its own labels too.

I also think safeForHandlers could be turned into safeForInlining by only adding
singly-used continuations. That should simplify the check here.

[Kevin Millikin (Google), 2015/02/25 11:06:36]

I agree that the whole thing is complicated.

* This is a label for a singly-used continuation bound by LetCont.

* visitLetCont uses the existence of a label to detect whether the continuation
was inlined (I don't think it actually needed to be that way before, we chould
have just used hasExactlyOneUse).

* We can't easily tell in visitLetCont for singly-used continuations whether
their use will be safe to inline or not.

We could be consistent by always generating labels for multiply-used
continuations when we encounter their first invocation, but it looks like there
are a lot of places where that happens so I'm not sure if that's better.

Do you think we should just always generate labels in visitLetCont, and use
something like the label's use count to see if it was inlined?

I've changed the name to safeForInlining.  They were already singly-used.

[asgerf, 2015/02/25 11:55:11]

I guess we could also just let the statement rewriter do all the inlining, since
it's already doing that. There might a performance hit, though, and the output
from the tree tracer (before rewriting) would likely become unreadable.

Otherwise, just make it a bit less subtle and always create labels on-demand or
always ahead-of-time. Always on-demand seems simplest, though I may have
overlooked something:

if (safeForInlining.contains(cont)) {
    return visit(cont.body)
} else {
    Label label = labels.putIfAbsent(cont, () => new Label());
    return new Break(label);
}

+              }
+              return new Break(labels[cont]);
             }
           });
     }