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

lib/src/codegen/js_codegen.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.
 
 library dev_compiler.src.codegen.js_codegen;
 
 import 'dart:collection' show HashSet, HashMap;
 import 'dart:io' show Directory, File;
 
 import 'package:analyzer/analyzer.dart' hide ConstantEvaluator;
@@ skipping 24 matching lines @@
 
 bool _isAnnotationType(Annotation m, String name) => m.name.name == name;
 
 Annotation _getAnnotation(AnnotatedNode node, String name) => node.metadata
     .firstWhere((annotation) => _isAnnotationType(annotation, name),
         orElse: () => null);
 
 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;
 
   // TODO(jmesserly): this is needed because RestrictedTypeRules can send
   // messages to CheckerReporter, for things like missing types.
   // We should probably refactor so this can't happen, as codegen would be too
   // late to be issuing these messages.
   final CheckerReporter _checkerReporter;
 
   /// The variable for the target of the current `..` cascade expression.
   SimpleIdentifier _cascadeTarget;
   /// The variable for the current catch clause
   SimpleIdentifier _catchParameter;
 
   ClassDeclaration currentClass;
   ConstantEvaluator _constEvaluator;
 
   final _exports = <String>[];
   final _lazyFields = <VariableDeclaration>[];
   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].
   final _pendingClasses = new HashMap<ClassElement, CompilationUnitMember>();
 
   /// Memoized results of [_lazyClass].
   final _lazyClassMemo = new HashMap<ClassElement, bool>();
 
   /// Memoized results of [_inLibraryCycle].
   final _libraryCycleMemo = new HashMap<LibraryElement, bool>();
@@ skipping 58 matching lines @@
         name,
         name,
         defaultValue
       ])
     ]);
   }
 
   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 {
     var name = currentLibrary.name;
     if (name == 'dart.core' || name == 'dart._internal') return 'dart.JsSymbol';
     return 'Symbol';
   }
 
   @override
   JS.Statement visitCompilationUnit(CompilationUnit node) {
     var source = node.element.source;
 
     _constEvaluator = new ConstantEvaluator(source, rules.provider);
     _checkerReporter.enterSource(source);
 
     // TODO(jmesserly): scriptTag, directives.
     var body = <JS.Statement>[];
     for (var child in node.declarations) {
       // Attempt to group adjacent fields/properties.
       if (child is! TopLevelVariableDeclaration) _flushLazyFields(body);
       if (child is! FunctionDeclaration) _flushLibraryProperties(body);
 
       var code = _visit(child);
 
       if (code != null) {
         if (_pendingPrivateNames.isNotEmpty) {
           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);
       }
     }
 
     // Flush any unwritten fields/properties.
     _flushLazyFields(body);
     _flushLibraryProperties(body);
 
     _checkerReporter.leaveSource();
 
@@ skipping 312 matching lines @@
       heritage = _emitTypeName(rules.provider.objectType);
     }
     if (node.withClause != null) {
       var mixins = _visitList(node.withClause.mixinTypes);
       mixins.insert(0, heritage);
       heritage = js.call('dart.mixin(#)', [mixins]);
     }
     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;
     var isObject = element.type.isObject;
     var name = node.name.name;
 
     var jsMethods = <JS.Method>[];
     // Iff no constructor is specified for a class C, it implicitly has a
     // default constructor `C() : super() {}`, unless C is class Object.
     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

       }
     }
 
     // Support for adapting dart:core Iterator/Iterable to ES6 versions.
     // This lets them use for-of loops transparently.
     // https://github.com/lukehoban/es6features#iterators--forof
     if (element.library.isDartCore && element.name == 'Iterable') {
       JS.Fun body = js.call('''function() {
         var iterator = this.iterator;
         return {
@@ skipping 314 matching lines @@
   }
 
   JS.Expression _defaultParamValue(FormalParameter param) {
     if (param is DefaultFormalParameter && param.defaultValue != null) {
       return _visit(param.defaultValue);
     } else {
       return new JS.LiteralNull();
     }
   }
 
-  @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;
     }
 
     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,
         isStatic: node.isStatic);
   }
 
   @override
   JS.Statement visitFunctionDeclaration(FunctionDeclaration node) {
     assert(node.parent is CompilationUnit);
 
@@ skipping 144 matching lines @@
   }
 
   bool _needQualifiedName(Element element) {
     var lib = element.library;
 
     return lib != null &&
         (lib != currentLibrary ||
             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(#, #, #)', [
         _visit(target),
         js.string(id.name, "'"),
         _visit(rhs)
       ]);
     } else {
       return null;
     }
   }
 
   @override
   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;
     }
 
     if (parent is ExpressionStatement &&
         rhs is CascadeExpression &&
         _isStateless(lhs, rhs)) {
       // Special case: cascade assignment to a variable in a statement.
       // We can reuse the variable to desugar it:
       //    result = []..length = length;
       // becomes:
@@ skipping 54 matching lines @@
       } else {
         return js.call('dart.dinvokef(#, #)', [_visit(node.methodName), args]);
       }
     }
 
     // TODO(jmesserly): if this resolves to a getter returning a function with
     // a call method, we don't generate the `.call` correctly.
 
     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);
     }
 
     return js.call('#(#)', [targetJs, _visit(node.argumentList)]);
   }
 
   /// Emits code for the `JS(...)` builtin.
   _emitForeignJS(MethodInvocation node) {
     var e = node.methodName.staticElement;
@@ skipping 394 matching lines @@
   JS.Expression _emitPostfixIncrement(Expression expr, Token op) {
     var type = rules.getStaticType(expr);
     assert(type != null);
     var tmp = _createTemporary('x', type);
 
     // Increment and write
     var one = AstBuilder.integerLiteral(1);
     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(#)", [
       _visit(tmp),
       write,
       _visit(tmp),
       _visit(expr)
     ]);
   }
 
@@ skipping 15 matching lines @@
     }
 
     assert(op.lexeme == '++' || op.lexeme == '--');
     return _emitPostfixIncrement(expr, op);
   }
 
   JS.Expression _emitPrefixIncrement(Token op, Expression expr) {
     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
   JS.Expression visitPrefixExpression(PrefixExpression node) {
     return _emitPrefixExpression(node.operator, node.operand);
   }
 
   JS.Expression _emitPrefixExpression(Token op, Expression expr) {
     var dispatchType = rules.getStaticType(expr);
     if (unaryOperationIsPrimitive(dispatchType)) {
@@ skipping 107 matching lines @@
   JS.This visitThisExpression(ThisExpression node) => new JS.This();
 
   @override
   JS.Super visitSuperExpression(SuperExpression node) => new JS.Super();
 
   @override
   visitPrefixedIdentifier(PrefixedIdentifier node) {
     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].
   /// Those two nodes are special because they're both allowed on left side of
   /// an assignment expression and cascades.
   Expression _getTarget(node) {
     assert(node is IndexExpression || node is PropertyAccess);
     return node.isCascaded ? _cascadeTarget : node.target;
   }
 
@@ skipping 371 matching lines @@
   ///
   /// Equality is a bit special, it is generated via the Dart `equals` runtime
   /// helper, that checks for null. The user defined method is called '=='.
   ///
   JS.Expression _jsMemberName(String name,
       {bool unary: false, bool isStatic: false}) {
     if (name.startsWith('_')) {
       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;
 
   FunctionBody _functionBody(node) =>
       node is FunctionDeclaration ? node.functionExpression.body : node.body;
 
   /// Choose a canonical name from the library element.
   /// This never uses the library's name (the identifier in the `library`
   /// declaration) as it doesn't have any meaningful rules enforced.
   JS.Identifier _libraryName(LibraryElement library) {
@@ skipping 208 matching lines @@
 
     // TODO(jmesserly): in many cases marking the end will be unncessary.
     printer.mark(_location(node.end));
   }
 
   String _getIdentifier(AstNode node) {
     if (node is SimpleIdentifier) return node.name;
     return null;
   }
 }