allow 'super' in async and finally blocks

lib/src/compiler/code_generator.dart

Index: lib/src/compiler/code_generator.dart
diff --git a/lib/src/compiler/code_generator.dart b/lib/src/compiler/code_generator.dart
index 1276c85eaeaeed3e9b6f4a7bbe8d4cff67e002fc..753a7d464f297b54006304ed7a75b992447c0639 100644
--- a/lib/src/compiler/code_generator.dart
+++ b/lib/src/compiler/code_generator.dart
@@ -116,6 +116,10 @@ class CodeGenerator extends GeneralizingAstVisitor
 
   String _buildRoot;
 
+  bool _superAllowed = true;
+
+  List<JS.Method> _superHelpers = <JS.Method>[];
+
   CodeGenerator(AnalysisContext c, this.options, this._extensionTypes)
       : context = c,
         types = c.typeProvider,
@@ -832,7 +836,7 @@ class CodeGenerator extends GeneralizingAstVisitor
         jsMethods
             .add(_emitConstructor(m, type, fields, virtualFields, isObject));
       } else if (m is MethodDeclaration) {
-        jsMethods.add(_emitMethodDeclaration(type, m));
+        jsMethods.add(_emitMethodDeclaration(m));
 
         if (m.element is PropertyAccessorElement) {
           jsMethods.add(_emitSuperAccessorWrapper(m, type, superclasses));
@@ -868,6 +872,10 @@ class CodeGenerator extends GeneralizingAstVisitor
       jsMethods.add(_emitIterable(type));
     }
 
+    // Add all of the super helper methods
+    jsMethods.addAll(_superHelpers);
+    _superHelpers.clear();
+
     return jsMethods.where((m) => m != null).toList(growable: false);
   }
 
@@ -1616,7 +1624,7 @@ class CodeGenerator extends GeneralizingAstVisitor
     }
   }
 
-  JS.Method _emitMethodDeclaration(DartType type, MethodDeclaration node) {
+  JS.Method _emitMethodDeclaration(MethodDeclaration node) {
     if (node.isAbstract) {
       return null;
     }
@@ -1939,8 +1947,11 @@ class CodeGenerator extends GeneralizingAstVisitor
     } else {
       _asyncStarController = null;
     }
+    var savedSuperAllowed = _superAllowed;
+    _superAllowed = false;
     // Visit the body with our async* controller set.
     var jsBody = _visit(body);
+    _superAllowed = savedSuperAllowed;
     _asyncStarController = savedController;
 
     DartType returnType = _getExpectedReturnType(element);
@@ -2359,6 +2370,11 @@ class CodeGenerator extends GeneralizingAstVisitor
     }
 
     jsTarget = new JS.PropertyAccess(jsTarget, memberName);
+
+    if (target is SuperExpression && !_superAllowed) {
+      return _emitSuperForwarder(node, jsTarget, typeArgs, args);
+    }
+
     if (typeArgs != null) jsTarget = new JS.Call(jsTarget, typeArgs);
 
     if (DynamicInvoke.get(node.methodName)) {

[Jennifer Messerly, 2016/05/03 18:56:18]

Is there a way to restructure this code so it's reused? it looks like
_emitSuperForwarder wants to use the rest of this code.

(It's always nice to try and keep the code reused, that way if someone tweaks
this code here later on, they won't regress how "super" is handled)

[Harry Terkelsen, 2016/05/05 23:37:35]

There is a way ;)

@@ -2371,6 +2387,56 @@ class CodeGenerator extends GeneralizingAstVisitor
     return new JS.Call(jsTarget, args);
   }
 
+  JS.Expression _emitSuperForwarder(
+      MethodInvocation node,
+      JS.PropertyAccess jsTarget,
+      List<JS.Expression> typeArgs,
+      List<JS.Expression> args) {
+    var helperMethod = _getSuperHelperFor(node, jsTarget, typeArgs, args);
+    var newTarget = new JS.PropertyAccess(new JS.This(), helperMethod);
+    var helperArgs = <JS.Expression>[];
+    if (typeArgs != null) {
+      helperArgs.addAll(typeArgs);
+    }
+    helperArgs.addAll(args);
+    return new JS.Call(newTarget, helperArgs);

[Jennifer Messerly, 2016/05/03 18:56:18]

FYI: you could build these like:

return js.call('this.#(#)', helperMethod, helperArgs);

and avoid the need for "newTarget" ... sra@ designed the `js.call` and
`js.statement` system to be pretty fast, it only parses once, and caches a
factory function that quickly creates the AST.

That said, it's always a bit of taste thing, whether to use raw JS AST nodes or
js.call. I tend to use raw nodes if it feels more natural (especially if I'm
just creating 1-2 nodes) or if we don't have JS AST parser support yet (e.g.
there's no `js.method(...)` helper for making a JS.Method) and use
js.call/js.statement for more complex things.

[Harry Terkelsen, 2016/05/05 23:37:34]

Done.

+  }
+
+  JS.Expression _getSuperHelperFor(

[Jennifer Messerly, 2016/05/03 18:56:18]

Probably worth a doc comment about what this method is for & how it works.

+      MethodInvocation node,
+      JS.PropertyAccess jsTarget,
+      List<JS.Expression> typeArgs,

[Jennifer Messerly, 2016/05/03 18:56:18]

Hmmm, this is an interesting approach. It's one super forwarder per call site?

I probably would've tried to get the super method signature (e.g. the super
MethodElement), and just emit a forwarder that forwards everything. Not sure
which approach would end up simpler, but that way every call site to "super.foo"
could use the same helper. That's unlikely to be a big deal though.

Anyway, since this is a simple+clean implementation as you have it, I don't
think it's worth changing now :). It's so well factored. Just throwing out some
other ideas so you know your options.


Another trick is to use runtime helpers if it's easier:

class MyDerived extends MyBaseClass {
  someMethod() {
    try { ... } finally { dart.superCall(MyDerived, 'foo', this, arg, to, foo);
}
  }
}


function superCall(cls, name, self, ...args) {
  let baseClass = cls.__proto__;
  let superMethod = baseClass.prototype[name];
  superMethod.call(self, args);
}


In fact it's possible to avoid all of this by just desugaring ES6 super in
place:


class MyDerived extends MyBaseClass {
  someMethod() {
    try { ... } finally { MyBaseClass.prototype.foo.apply(this, arg, to, foo); }
  }
}

(if MyBaseClass was a mixin application, just save it to a temp first)


... but anyway since you've got the forwarding methods already, and they'll
likely perform better, it seems alright to keep, if the Symbol fix below is
done.

+      List<JS.Expression> args) {
+    var counter = 0;
+    var helperArgs =
+        new List.generate(typeArgs?.length ?? 0 + args.length, (index) {
+      new JS.Identifier('a${counter++}');

[Jennifer Messerly, 2016/05/03 18:56:18]

this should be symbolized so it doesn't collide with methods:


let super$foo = Symbol('super.foo');

class D extends B {
  [super$foo](... args ... ) {
    ... impl ...
  }
}


we have a couple of places we generate Symbols, for example, how private method
names are handled, this could likely work in a similar way.

Also since "super$foo" will be a JS.TemporaryId, you won't need a counter --
renaming will be handled automagically.

[Harry Terkelsen, 2016/05/05 23:37:34]

Done.

+    }, growable: false);
+
+    var newTarget;
+    if (typeArgs != null) {
+      newTarget = new JS.Call(jsTarget, helperArgs.sublist(0, typeArgs.length));
+    } else {
+      newTarget = jsTarget;
+    }
+
+    var forwardedCall;
+    if (DynamicInvoke.get(node.methodName)) {
+      forwardedCall = js.call('dart.dcall(#, #)',
+          [newTarget, helperArgs.sublist(typeArgs?.length ?? 0)]);
+    } else {
+      forwardedCall =
+          new JS.Call(newTarget, helperArgs.sublist(typeArgs?.length ?? 0));
+    }
+
+    var helperMethod =
+        new JS.Fun(helperArgs, new JS.Block([new JS.Return(forwardedCall)]));
+    var helperMethodName =
+        js.string('super\$${node.methodName.name}${_superHelpers.length}', "'");

[Jennifer Messerly, 2016/05/03 18:56:18]

based on comment above, "helperMethodName" will just be the JS.TemporaryId we
create to hold the method symbol.

[Harry Terkelsen, 2016/05/05 23:37:34]

Done.

+    _superHelpers.add(new JS.Method(helperMethodName, helperMethod));
+    return helperMethodName;
+  }
+
   /// Emits a function call, to a top-level function, local function, or
   /// an expression.
   JS.Expression _emitFunctionCall(InvocationExpression node) {
@@ -3723,8 +3789,12 @@ class CodeGenerator extends GeneralizingAstVisitor
 
   @override
   visitTryStatement(TryStatement node) {
-    return new JS.Try(_visit(node.body), _visitCatch(node.catchClauses),
-        _visit(node.finallyBlock));
+    var savedSuperAllowed = _superAllowed;
+    _superAllowed = false;
+    var finallyBlock = _visit(node.finallyBlock);
+    _superAllowed = savedSuperAllowed;
+    return new JS.Try(
+        _visit(node.body), _visitCatch(node.catchClauses), finallyBlock);
   }
 
   _visitCatch(NodeList<CatchClause> clauses) {