Extension method support to move us closer to a valid List implementation.

lib/src/codegen/js_codegen.dart

Index: lib/src/codegen/js_codegen.dart
diff --git a/lib/src/codegen/js_codegen.dart b/lib/src/codegen/js_codegen.dart
index d1cd2a15ac8f67b9e600fa1083e731704f33118a..033c70ad5ddf514b37332f58b6ebf3e0dbd0a8dd 100644
--- a/lib/src/codegen/js_codegen.dart
+++ b/lib/src/codegen/js_codegen.dart
@@ -42,6 +42,12 @@ Annotation _getAnnotation(AnnotatedNode node, String name) => node.metadata
 Annotation _getJsNameAnnotation(AnnotatedNode node) =>
     _getAnnotation(node, "JsName");
 
+// TODO(jacobr): we would like to do something like the following
+// but we don't have summary support yet.
+// bool _supportJsExtensionMethod(AnnotatedNode node) =>
+//    _getAnnotation(node, "SupportJsExtensionMethod") != null;
+
+
 class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
   final LibraryInfo libraryInfo;
   final TypeRules rules;
@@ -65,6 +71,14 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
   final _properties = <FunctionDeclaration>[];
   final _privateNames = new HashSet<String>();
   final _pendingPrivateNames = <String>[];
+  final _extensionMethodNames = new HashSet<String>();
+  final _pendingExtensionMethodNames = <String>[];
+
+  // TODO(jacobr): determine the the set of types with extension methods from
+  // the annotations rather than hard coding the list once the analyzer
+  // supports summaries.
+  List<InterfaceType> get _JsExtensionMethodTypes =>

[Jennifer Messerly, 2015/04/03 16:25:28]

nit: should be lower case _jsExtension...

[Jacob, 2015/04/03 20:25:58]

Done.

+      <InterfaceType>[rules.provider.listType, rules.provider.iterableType];
 
   /// Classes we have not emitted yet. Values can be [ClassDeclaration] or
   /// [ClassTypeAlias].
@@ -143,6 +157,9 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
   JS.Statement _initPrivateSymbol(String name) => js.statement(
       'let # = $_SYMBOL(#);', [new JSTemporary(name), js.string(name, "'")]);
 
+  JS.Statement _initExtensionMethodSymbol(String name) => js.statement(
+      'let # = $_SYMBOL(#);', [new JS.Identifier(name, allowRename: false), js.string(name, "'")]);

[Jennifer Messerly, 2015/04/03 16:25:28]

hmm, making this allowRename: false is a bit problematic. It could conflict with
top level items (classes and functions). Why do we need to prevent renaming? It
should be fine ... you just want to export it with the right name. For example:

# some bogus renamed symbol
let map$12345 = Symbol("map");

    class List {
      [map$12345](f) { ... }
    }

# export it with the right name
exports.$map = map$12345;

[Jacob, 2015/04/03 20:25:58]

Talked offline. Added checks when we try to add the same name to export twice.
Long term the solution is we need static errors if extension methods and top
level names conflict. If dart adds extension method support to the core language
this would probably be a general restriction.

+
   // TODO(jmesserly): this is a temporary workaround for `Symbol` in core,
   // until we have better name tracking.
   String get _SYMBOL {
@@ -172,6 +189,10 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
           body.addAll(_pendingPrivateNames.map(_initPrivateSymbol));
           _pendingPrivateNames.clear();
         }
+        if (_pendingExtensionMethodNames.isNotEmpty) {
+          body.addAll(_pendingExtensionMethodNames.map(_initExtensionMethodSymbol));

[Jennifer Messerly, 2015/04/03 16:25:28]

run formatter?

pub run dart_style:format -w lib/src/codegen/js_codegen.dart


(... if all files are checked in to git, ./tool/presubmit.dart will do it for
you.)

[Jacob, 2015/04/03 20:25:58]

Done.

+          _pendingExtensionMethodNames.clear();
+        }
         body.add(code);
       }
     }
@@ -504,8 +525,76 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
     return heritage;
   }
 
+  // TODO(jacobr): why doesn't the generic i1.isSubTypeOf(i2) work?

[Jennifer Messerly, 2015/04/03 16:25:28]

did you try rules.isSubTypeOf(i1, i2) ? that will hit our code in
checker/rules.dart

[Jacob, 2015/04/03 20:25:59]

That doesn't work. The output changes dramatically and incorrectly if I change
that.
Likely because of the generic type arguments.

[Jennifer Messerly, 2015/04/03 21:08:05]

Doh! Well it'd be good to figure out. Something seems a bit off about writing
our own subtype check as a workaround =)

so I started debugging this by adding prints to visitClassDeclaration, and this
one seemed to work:
    var extType = rules.provider.iterableDynamicType;
    if (node.element.type.isSubtypeOf(extType)) {
      print('!!! found ${extType} subtype ${node.element.type}');
    }

does that match your code?

they make that via: _iterableDynamicType =
_iterableType.substitute4(<DartType>[_dynamicType]);
so you can make one for List<dynamic>:

var listDynamicType =
rules.provider.listType.substitute4([rules.provider.dynamicType]);

we could alternatively make a helper:

InterfaceType _fillDynamicTypeArgs(InterfaceType t) {
  var d = rules.provider.dynamicType;
  return t.substitute4(new List.filled(t.typeArguments.length, d));
}


but probably not worth it with just 1 type that needs it.

[vsm, 2015/04/03 21:15:35]

It'd be good to use the rules.isSubTypeOf.  I suspect - as John's getting at -
the problem is that i2 is the generic version of the type - Iterable<T> - and
not the dynamic version - Iterable<dynamic>.

+  bool _isInterfaceSubTypeOf(InterfaceType i1, InterfaceType i2) {
+    if (i1 == i2) return true;
+
+    if (i1.element == i2.element) {
+      return true;
+    }
+
+    if (i2.isDartCoreFunction) {
+      if (i1.element.getMethod("call") != null) return true;
+    }
+
+    if (i1 == rules.provider.objectType) return false;
+
+    if (_isInterfaceSubTypeOf(i1.superclass, i2)) return true;
+
+    for (final parent in i1.interfaces) {
+      if (_isInterfaceSubTypeOf(parent, i2)) return true;
+    }
+
+    for (final parent in i1.mixins) {
+      if (_isInterfaceSubTypeOf(parent, i2)) return true;
+    }
+
+    return false;
+  }
+
   /// Emit class members that can be generated as methods.
   /// Anything not handled here will be addressed in [_finishClassMembers].
+  List<InterfaceType> getMatchingExtensionMethodTypes(InterfaceType type) {

[Jennifer Messerly, 2015/04/03 16:25:28]

is this method just:
    _JsExtensionMethodTypes.where((t) => _isInterfaceSubTypeOf(type, t));

(it looks like you only use the iterable API, so toList can be skipped to)

[Jacob, 2015/04/03 20:25:58]

done

+    var extensionTypes = <InterfaceType>[];
+    for (var extensionType in _JsExtensionMethodTypes) {
+      if (_isInterfaceSubTypeOf(type, extensionType)) {
+        extensionTypes.add(extensionType);
+      }
+    }
+    return extensionTypes;
+  }
+
+  LibraryElement lookupExtensionLibrary(Iterable<InterfaceType> extensionTypes, String name, {bool isGetter: false, bool isSetter: false}) {

[Jennifer Messerly, 2015/04/03 16:25:28]

i'd probably use "get" instead of "lookup" for consistency with analyzer APIs.
also makes it consistent with getMatchingExtensionMethodTypes above.

[Jacob, 2015/04/03 20:25:58]

Done.

+    var extensionLibrary;
+    assert (!isGetter || !isSetter);
+    for (var extensionType in extensionTypes) {

[Jennifer Messerly, 2015/04/03 16:25:28]

I'd probably just name this "type" and "extensionLibrary" -> "library" ... that
they deal with extensions is included in the context of this method

[Jacob, 2015/04/03 20:25:58]

Done.

+      var match;
+      if (isGetter) {
+        match = extensionType.getGetter(name);

[Jennifer Messerly, 2015/04/03 16:25:28]

I was looking at the package:analyzer implementation just now, noticed a few
things:
* these are all linear searches in analyzer. We might need to cache, as this
lookup will be done frequently.
* InterfaceType.methods is just `type.element.methods` but with generic type
substitution. That's probably just extra overhead for us. I bet we could just do
type.element.methods.
* we could simplify the call sites and just look up by name. I don't think we
need isGetter/isSetter.

with those changes it would be something like:

    /// Maps class element to its extension methods.
    final _extensionMethods = new HashMap<ClassElement, HashSet<String>>();

    _initExtensionMethods() {      
      for (var type in _jsExtensionMethodTypes) {
        var names = new HashSet<String>();
        var e = type.element;
        names.addAll(e.methods.map((m) => m.name));
        names.addAll(e.accessors.map((m) => m.name));
       _extensionMethods[e] = names;
      }
    }

    LibraryElement getExtensionLibrary(Iterable<InterfaceType> types, String
name) { 
      LibraryElement library = null;
      for (var type in types) {
        var element = type.element;
        if (_extensionMethods[element].contains(name)) {
          assert(library == null || library == element.library);
          library = element.library;
        }
      }
      return library;
    }

[Jacob, 2015/04/03 20:25:58]

Done.

+      } else if (isSetter) {
+        match = extensionType.getSetter(name);
+      } else {
+        match = extensionType.getMethod(name);
+      }
+      // It is possible that the same method could need to be an extension
+      // method for classes defined in multiple libraries. Instead of handling
+      // that case we currently assert. The correct behavior is unclear so we
+      // might need to prevent this with a compile time error.
+      if (match != null) {
+        assert(extensionLibrary == null || extensionLibrary == extensionType.element.library);
+        extensionLibrary = extensionType.element.library;
+      }
+    }
+    return extensionLibrary;
+  }
+
+  JS.Expression nameIfExtension(DartType targetType, String name, {bool isGetter: false, bool isSetter: false}) {

[Jennifer Messerly, 2015/04/03 16:25:28]

everywhere we call this we do rules.getStaticType(expr) ... maybe just take an
expr instead?

[Jacob, 2015/04/03 20:25:59]

Done.

+    if (targetType is! InterfaceType) return null;
+    var extensionLibrary = lookupExtensionLibrary(getMatchingExtensionMethodTypes(targetType), name, isGetter: isGetter, isSetter: isSetter);
+    if (extensionLibrary == null) return null;

[Jennifer Messerly, 2015/04/03 16:25:28]

from the places we call this, we end up with code like:
    methodName != null ? methodName: js.string('set')

can we move that logic inside this method? And just make this method handle all
member names, extension or not? It seems like this method could be merged with
_jsMemberName

[Jacob, 2015/04/03 20:25:59]

There are a lot of places we call emitMemberName (new name for _jsMemberName
where extensions are not relevant. I'm not sure it would be a net negative or
positive to merge the logic together. We'd have to pass in a null target to a
lot of places 

Good point... now that we have gotten rid of distinguishing based on
getter/setter/method this is trivial

+    return js.call('#.#', [_libraryName(extensionLibrary), _extensionMethodName(name)]);
+  }
+
   List<JS.Method> _emitClassMethods(ClassDeclaration node,
       List<ConstructorDeclaration> ctors, List<FieldDeclaration> fields) {
     var element = node.element;
@@ -518,12 +607,12 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
     if (ctors.isEmpty && !isObject) {
       jsMethods.add(_emitImplicitConstructor(node, name, fields));
     }
-
+    var extensionTypes = getMatchingExtensionMethodTypes(element.type);
     for (var member in node.members) {
       if (member is ConstructorDeclaration) {
         jsMethods.add(_emitConstructor(member, name, fields, isObject));
       } else if (member is MethodDeclaration) {
-        jsMethods.add(_visit(member));
+        jsMethods.add(_visitMethodDeclaration(member, extensionTypes));

[Jennifer Messerly, 2015/04/03 16:25:28]

one thing to watch out for: _visit will associate a source span, whereas
directly calling the method won't. That said, I think it's fine in this case.
Source spans are more needed for Statements, where the user could put a
breakpoint.

[Jacob, 2015/04/03 20:25:59]

good to know

       }
     }
 
@@ -858,8 +947,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
     }
   }
 
-  @override
-  JS.Method visitMethodDeclaration(MethodDeclaration node) {
+  JS.Method _visitMethodDeclaration(MethodDeclaration node, List<InterfaceType> extensionTypes) {

[Jennifer Messerly, 2015/04/03 16:25:28]

_emitMethodDeclaration? At some point we sort of started a convention that
"visit" was for the tree visit methods, and _emit* was other methods that return
JS ASts. That said, maybe it isn't a useful convention and we should just go
_visit.

Another random thought for after this CL, should we get rid of "_" prefixes
everywhere? I've noticed that almost all of this file is private API, and using
so much "_" starts to get ugly. We're already in lib/src/... so it would be fine
to just go public names for methods (maybe leave fields private?)

[Jacob, 2015/04/03 20:25:58]

_might _as _well _remove _the _underscores as a _followup _cl. _they _are _not
_bothering_ _me _much _ but_ don't add_ value

[Jennifer Messerly, 2015/04/03 21:08:05]

haha :)

     if (node.isAbstract || _externalOrNative(node)) {
       return null;
     }
@@ -867,7 +955,25 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
     var params = _visit(node.parameters);
     if (params == null) params = [];
 
-    return new JS.Method(_jsMemberName(node.name.name, isStatic: node.isStatic),
+    var extensionLibrary;
+    var memberName;
+    if (node.isStatic == false &&

[Jennifer Messerly, 2015/04/03 16:25:28]

!node.isStatic

[Jacob, 2015/04/03 20:25:58]

Done.

+    (extensionLibrary = lookupExtensionLibrary(extensionTypes, node.name.name, isGetter: node.isGetter, isSetter: node.isSetter)) != null) {

[Jennifer Messerly, 2015/04/03 16:25:28]

style wise, I try to avoid assignment expression in "if". I'd probably refactor
this as:

if (!node.isStatic) {
  var extensionLibrary = lookupExtensionLibrary(extensionTypes, node.name.name,
i
      sGetter: node.isGetter, isSetter: node.isSetter);
  if (extensionLibrary != null) {
  ...
}

[Jacob, 2015/04/03 20:25:59]

Done.

+      var extensionMethodName = _extensionMethodNameRaw(node.name.name);
+      if (extensionLibrary == libraryInfo.library.library) {
+        // TODO(jacobr): need to do a better job ensuring that extension method
+        // name symbols do not conflict with other symbols before we can let
+        // user defined libraries define extension methods.
+        if (_extensionMethodNames.add(extensionMethodName)) {
+          _pendingExtensionMethodNames.add(extensionMethodName);
+          _exports.add(extensionMethodName);
+        }
+      }
+      memberName= js.call('#.#', [_libraryName(extensionLibrary), js.string(extensionMethodName, "'")]);
+    } else {
+      memberName = _jsMemberName(node.name.name, isStatic: node.isStatic);
+    }
+    return new JS.Method(memberName,
         new JS.Fun(params, _visit(node.body)),
         isGetter: node.isGetter,
         isSetter: node.isSetter,
@@ -1032,7 +1138,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
             element is ClassElement && _lazyClass(element));
   }
 
-  JS.Node _emitDPutIfDynamic(
+  JS.Node _emitDSetIfDynamic(
       Expression target, SimpleIdentifier id, Expression rhs) {
     if (rules.isDynamicTarget(target)) {
       return js.call('dart.dput(#, #, #)', [
@@ -1049,27 +1155,27 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
   JS.Node visitAssignmentExpression(AssignmentExpression node) {
     var lhs = node.leftHandSide;
     var rhs = node.rightHandSide;
-    return _emitAssignment(lhs, rhs, node.parent);
+    return _emitSet(lhs, rhs, node.parent);
   }
 
-  JS.Node _emitAssignment(Expression lhs, Expression rhs, [AstNode parent]) {
+  JS.Node _emitSet(Expression lhs, Expression rhs, [AstNode parent]) {
     if (lhs is IndexExpression) {
       String code;
       var target = _getTarget(lhs);
       if (rules.isDynamicTarget(target)) {
         code = 'dart.dsetindex(#, #, #)';
-      } else {
-        code = '#.set(#, #)';
+        return js.call(code, [_visit(target), _visit(lhs.index), _visit(rhs)]);
       }
-      return js.call(code, [_visit(target), _visit(lhs.index), _visit(rhs)]);
+      var methodName = nameIfExtension(rules.getStaticType(target), "[]=");
+      return js.call('#.#(#, #)', [_visit(target), methodName != null ? methodName: js.string('set'), _visit(lhs.index), _visit(rhs)]);
     }
 
     if (lhs is PropertyAccess) {
-      var result = _emitDPutIfDynamic(_getTarget(lhs), lhs.propertyName, rhs);
+      var result = _emitDSetIfDynamic(_getTarget(lhs), lhs.propertyName, rhs);

[Jennifer Messerly, 2015/04/03 16:25:28]

ideas: _tryEmitDynamicSet? _maybeEmitDynamicSet? _handleDynamicSet?

[Jacob, 2015/04/03 20:25:58]

Done.

       if (result != null) return result;
     } else if (lhs is PrefixedIdentifier) {
       // TODO(vsm): Is this the right code if the prefix is a library?
-      var result = _emitDPutIfDynamic(lhs.prefix, lhs.identifier, rhs);
+      var result = _emitDSetIfDynamic(lhs.prefix, lhs.identifier, rhs);
       if (result != null) return result;
     }
 
@@ -1144,7 +1250,8 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
 
     var targetJs;
     if (target != null) {
-      targetJs = js.call('#.#', [_visit(target), node.methodName.name]);
+      var methodNameJs = nameIfExtension(rules.getStaticType(target), node.methodName.name);
+      targetJs = js.call('#.#', [_visit(target), methodNameJs != null ? methodNameJs : node.methodName.name]);
     } else {
       targetJs = _visit(node.methodName);
     }
@@ -1559,7 +1666,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
     one.staticType = rules.provider.intType;
     var increment = AstBuilder.binaryExpression(tmp, op.lexeme[0], one);
     increment.staticType = type;
-    var write = _emitAssignment(expr, increment);
+    var write = _emitSet(expr, increment);
 
     var bindThis = _maybeBindThis(expr);
     return js.call("((#) => (#, #))$bindThis(#)", [
@@ -1595,7 +1702,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
     var one = AstBuilder.integerLiteral(1);
     one.staticType = rules.provider.intType;
     var increment = AstBuilder.binaryExpression(expr, op.lexeme[0], one);
-    return _emitAssignment(expr, increment);
+    return _emitSet(expr, increment);
   }
 
   @override
@@ -1723,38 +1830,44 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
     if (node.prefix.staticElement is PrefixElement) {
       return _visit(node.identifier);
     } else {
-      return _visitGet(node.prefix, node.identifier);
+      return _emitGet(node.prefix, node.identifier);
     }
   }
 
   @override
   visitPropertyAccess(PropertyAccess node) =>
-      _visitGet(_getTarget(node), node.propertyName);
+      _emitGet(_getTarget(node), node.propertyName);
 
   /// Shared code for [PrefixedIdentifier] and [PropertyAccess].
-  _visitGet(Expression target, SimpleIdentifier name) {
+  _emitGet(Expression target, SimpleIdentifier name) {
     if (rules.isDynamicTarget(target)) {
       return js.call(
           'dart.dload(#, #)', [_visit(target), js.string(name.name, "'")]);
     } else {
       var e = name.staticElement;
-      return js.call('#.#', [
+      bool isStatic = e is ExecutableElement && e.isStatic;
+      var memberName;
+      if (!isStatic) {
+        memberName = nameIfExtension(rules.getStaticType(target), name.name, isGetter: true);
+      }
+
+      var ret = js.call('#.#', [
         _visit(target),
-        _jsMemberName(name.name, isStatic: e is ExecutableElement && e.isStatic)
+        memberName != null ? memberName : _jsMemberName(name.name, isStatic: isStatic)
       ]);
+      return ret;
     }
   }
 
   @override
   visitIndexExpression(IndexExpression node) {
     var target = _getTarget(node);
-    var code;
     if (rules.isDynamicTarget(target)) {
-      code = 'dart.dindex(#, #)';
-    } else {
-      code = '#.get(#)';
+      return js.call('dart.dindex(#, #)', [_visit(target), _visit(node.index)]);
     }
-    return js.call(code, [_visit(target), _visit(node.index)]);
+
+    var targetJs = nameIfExtension(rules.getStaticType(target), '[]');
+    return js.call('#.#(#)', [_visit(target), targetJs != null ? targetJs : js.string('get'), _visit(node.index)]);
   }
 
   /// Gets the target of a [PropertyAccess] or [IndexExpression].
@@ -2146,23 +2259,33 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
       if (_privateNames.add(name)) _pendingPrivateNames.add(name);
       return new JSTemporary(name);
     }
-    if (name == '[]') {
-      name = 'get';
-    } else if (name == '[]=') {
-      name = 'set';
-    } else if (unary && name == '-') {
-      name = 'unary-';
-    } else if (isStatic && invalidJSStaticMethodName(name)) {
+    return _propertyName(_transformMemberNameHelper(name, unary: unary, isStatic: isStatic));
+  }
+
+  String _transformMemberNameHelper(String name,

[Jennifer Messerly, 2015/04/03 16:25:28]

maybe call this one _jsMemberName, and the other one _emitMemberName? (since
_emit methods return JS.Nodes)

[Jacob, 2015/04/03 20:25:58]

that is better. done

+                                    {bool unary: false, bool isStatic: false}) {
+    if (name == '[]') return 'get';
+    if (name == '[]=') return 'set';
+    if (unary && name == '-') return 'unary-';
+    if (isStatic && invalidJSStaticMethodName(name)) {
       // Choose an string name. Use an invalid identifier so it won't conflict
       // with any valid member names.
       // TODO(jmesserly): this works around the problem, but I'm pretty sure we
       // don't need it, as static methods seemed to work. The only concrete
       // issue we saw was in the defineNamedConstructor helper function.
-      name = '$name*';
+      return '$name*';
     }
-    return _propertyName(name);
+    return name;
   }
 
+
+  // TODO(jacobr): we need to avoid possible collisions between extension
+  // methods names and regular names.

[Jennifer Messerly, 2015/04/03 16:28:27]

I think if you change _initExtensionMethodSymbol to be `new JSTemporary(name)`
instead of `new JS.Identifier(name, allowRename: false)` then this TODO goes
away :)

JSTemporaries are for all local variables introduced into the JS code by the
compiler. They'll automatically be renamed to avoid conflicts.

[Jacob, 2015/04/03 20:25:58]

Done.

+  JS.LiteralString _extensionMethodName(String name) =>
+      js.string(_extensionMethodNameRaw(name), "'");
+
+  String _extensionMethodNameRaw(String name) => '\$${_transformMemberNameHelper(name)}';
+
   bool _externalOrNative(node) =>
       node.externalKeyword != null || _functionBody(node) is NativeFunctionBody;