allow 'super' in async and finally blocks

lib/src/compiler/code_generator.dart

 // Copyright (c) 2015, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 import 'dart:collection' show HashMap, HashSet;
 import 'dart:math' show min, max;
 
 import 'package:analyzer/analyzer.dart' hide ConstantEvaluator;
 import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/dart/ast/token.dart' show Token, TokenType;
@@ skipping 98 matching lines @@
   FunctionBody _currentFunction;
 
   /// Helper class for emitting elements in the proper order to allow
   /// JS to load the module.
   ElementLoader _loader;
 
   BuildUnit _buildUnit;
 
   String _buildRoot;
 
+  bool _superAllowed = true;
+
+  List<JS.Method> _superHelpers = <JS.Method>[];
+
   CodeGenerator(AnalysisContext c, this.options, this._extensionTypes)
       : context = c,
         types = c.typeProvider,
         _asyncStreamIterator =
             _getLibrary(c, 'dart:async').getType('StreamIterator').type,
         _jsArray = _getLibrary(c, 'dart:_interceptors').getType('JSArray'),
         dartCoreLibrary = _getLibrary(c, 'dart:core'),
         dartJSLibrary = _getLibrary(c, 'dart:js');
 
   LibraryElement get currentLibrary => _loader.currentElement.library;
@@ skipping 696 matching lines @@
     }
 
     bool hasJsPeer = findAnnotation(element, isJsPeerInterface) != null;
 
     bool hasIterator = false;
     for (var m in node.members) {
       if (m is ConstructorDeclaration) {
         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));
         }
 
         if (!hasJsPeer && m.isGetter && m.name.name == 'iterator') {
           hasIterator = true;
           jsMethods.add(_emitIterable(type));
         }
       } else if (m is FieldDeclaration) {
@@ skipping 15 matching lines @@
     // helper on a parent class. This pattern is common in the core libraries
     // (e.g. IterableMixin<E> and IterableBase<E>).
     //
     // (We could do this same optimization for any interface with an `iterator`
     // method, but that's more expensive to check for, so it doesn't seem worth
     // it. The above case for an explicit `iterator` method will catch those.)
     if (!hasJsPeer && !hasIterator && _implementsIterable(type)) {
       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);
   }
 
   /// This is called whenever a derived class needs to introduce a new field,
   /// shadowing a field or getter/setter pair on its parent.
   ///
   /// This is important because otherwise, trying to read or write the field
   /// would end up calling the getter or setter, and one of those might not even
   /// exist, resulting in a runtime error. Even if they did exist, that's the
   /// wrong behavior if a new field was declared.
@@ skipping 728 matching lines @@
       return new JS.Fun(params, js.statement('{ return this.#; }', [name]));
     } else if (node.isSetter) {
       return new JS.Fun(
           params, js.statement('{ this.# = #; }', [name, params.last]));
     } else {
       return new JS.Fun(
           params, js.statement('{ return this.#(#); }', [name, params]));
     }
   }
 
-  JS.Method _emitMethodDeclaration(DartType type, MethodDeclaration node) {
+  JS.Method _emitMethodDeclaration(MethodDeclaration node) {
     if (node.isAbstract) {
       return null;
     }
 
     JS.Fun fn;
     if (_externalOrNative(node)) {
       fn = _emitNativeFunctionBody(node);
       // TODO(vsm): Remove if / when we handle the static case above.
       if (fn == null) return null;
     } else {
@@ skipping 302 matching lines @@
     // `await` is generated as `yield`.
     // runtime/_generators.js has an example of what the code is generated as.
     var savedController = _asyncStarController;
     List jsParams = visitFormalParameterList(parameters);
     if (kind == 'asyncStar') {
       _asyncStarController = new JS.TemporaryId('stream');
       jsParams.insert(0, _asyncStarController);
     } 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);
     JS.Expression gen = new JS.Fun(jsParams, jsBody,
         isGenerator: true, returnType: emitTypeRef(returnType));
     if (JS.This.foundIn(gen)) {
       gen = js.call('#.bind(this)', gen);
     }
 
     var T = _emitTypeName(returnType);
@@ skipping 373 matching lines @@
 
     var target = _getTarget(node);
     if (target == null || isLibraryPrefix(target)) {
       return _emitFunctionCall(node);
     }
 
     return _emitMethodCall(target, node);
   }
 
   /// Emits a (possibly generic) instance method call.
   JS.Expression _emitMethodCall(Expression target, MethodInvocation node) {

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

do we need the same fix in _emitAccess?

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

Done.

     var type = getStaticType(target);
     var name = node.methodName.name;
     var element = node.methodName.staticElement;
     bool isStatic = element is ExecutableElement && element.isStatic;
     var memberName = _emitMemberName(name, type: type, isStatic: isStatic);
 
     JS.Expression jsTarget = _visit(target);
     var typeArgs = _emitInvokeTypeArguments(node);
     List<JS.Expression> args = _visit(node.argumentList);
     if (DynamicInvoke.get(target)) {
       if (typeArgs != null) {
         return js.call('dart.dgsend(#, #, #, #)',
             [jsTarget, new JS.ArrayInitializer(typeArgs), memberName, args]);
       } else {
         return js.call('dart.dsend(#, #, #)', [jsTarget, memberName, args]);
       }
     }
     if (_isObjectMemberCall(target, name)) {
       // Object methods require a helper for null checks & native types.
       assert(typeArgs == null); // Object methods don't take type args.
       return js.call('dart.#(#, #)', [memberName, jsTarget, args]);
     }
 
     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 ;)

       // This is a dynamic call to a statically known target. For example:
       //     class Foo { Function bar; }
       //     new Foo().bar(); // dynamic call
       return js.call('dart.dcall(#, #)', [jsTarget, args]);
     }
 
     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) {
     var fn = _visit(node.function);
     var args = _visit(node.argumentList);
     if (DynamicInvoke.get(node.function)) {
       var typeArgs = _emitInvokeTypeArguments(node);
       if (typeArgs != null) {
         return js.call('dart.dgcall(#, #, #)',
             [fn, new JS.ArrayInitializer(typeArgs), args]);
@@ skipping 1332 matching lines @@
   }
 
   @override
   visitContinueStatement(ContinueStatement node) {
     var label = node.label;
     return new JS.Continue(label?.name);
   }
 
   @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) {
     if (clauses == null || clauses.isEmpty) return null;
 
     // TODO(jmesserly): need a better way to get a temporary variable.
     // This could incorrectly shadow a user's name.
     var savedCatch = _catchParameter;
 
     if (clauses.length == 1 && clauses.single.exceptionParameter != null) {
@@ skipping 487 matching lines @@
 }
 
 bool isLibraryPrefix(Expression node) =>
     node is SimpleIdentifier && node.staticElement is PrefixElement;
 
 LibraryElement _getLibrary(AnalysisContext c, String uri) =>
     c.computeLibraryElement(c.sourceFactory.forUri(uri));
 
 bool _isDartRuntime(LibraryElement l) =>
     l.isInSdk && l.source.uri.toString() == 'dart:_runtime';