Fix handling of static names for JS interop and improve test coverage.

pkg/dev_compiler/lib/src/compiler/code_generator.dart

Index: pkg/dev_compiler/lib/src/compiler/code_generator.dart
diff --git a/pkg/dev_compiler/lib/src/compiler/code_generator.dart b/pkg/dev_compiler/lib/src/compiler/code_generator.dart
index 361f5ec0198127d03ab3f0f684ebeaa356228e5d..24fc526734ba6e9979bd19f7997e3042faf858b8 100644
--- a/pkg/dev_compiler/lib/src/compiler/code_generator.dart
+++ b/pkg/dev_compiler/lib/src/compiler/code_generator.dart
@@ -324,11 +324,8 @@ class CodeGenerator extends GeneralizingAstVisitor
       elementJSName = getAnnotationName(e, isPublicJSAnnotation) ?? '';
     }
 
-    if (e is TopLevelVariableElement &&
-        e.getter != null &&
-        (e.getter.isExternal ||
-            findAnnotation(e.getter, isPublicJSAnnotation) != null)) {
-      elementJSName = getAnnotationName(e.getter, isPublicJSAnnotation) ?? '';
+    if (e is TopLevelVariableElement) {
+      elementJSName = _jsInteropStaticMemberName(e);
     }
     if (elementJSName == null) return null;
 
@@ -353,6 +350,33 @@ class CodeGenerator extends GeneralizingAstVisitor
     return access;
   }
 
+  String _jsInteropStaticMemberName(Element e, {String name}) {
+    if (e == null || e.library == null ||
+        findAnnotation(e.library, isPublicJSAnnotation) == null) {
+      return null;
+    }
+    if (e is PropertyInducingElement) {
+      // Assume properties have consistent JS names for getters and setters.
+      return _jsInteropStaticMemberName(e.getter, name: e.name) ??
+          _jsInteropStaticMemberName(e.setter, name: e.name);
+    }
+    if (e is ExecutableElement &&
+        e.isExternal &&
+        findAnnotation(e, isPublicJSAnnotation) != null) {
+      return getAnnotationName(e, isPublicJSAnnotation) ?? name ?? e.name;
+    }
+    return null;
+  }
+
+  JS.Expression _emitJSInteropStaticMemberName(Element e) {
+    var name = _jsInteropStaticMemberName(e);
+    if (name == null) return null;
+    // We do not support statics names with JS annotations containing dots.
+    // See https://github.com/dart-lang/sdk/issues/27926
+    assert(!name.contains('.'));

[Jennifer Messerly, 2016/11/29 20:54:52]

This should be an error or a throw, rather than an assert. asserts should mean
invariants that (we believe) cannot be broken under any circumstances. This one
could trip because of user input.

[Jacob, 2016/11/29 21:37:33]

Done.

+    return js.string(name);
+  }
+
   /// Flattens blocks in [items] to a single list.
   ///
   /// This will not flatten blocks that are marked as being scopes.
@@ -2705,7 +2729,8 @@ class CodeGenerator extends GeneralizingAstVisitor
     if (element is ClassMemberElement && element is! ConstructorElement) {
       bool isStatic = element.isStatic;
       var type = element.enclosingElement.type;
-      var member = _emitMemberName(name, isStatic: isStatic, type: type);
+      var member = _emitMemberName(name,
+          isStatic: isStatic, type: type, element: element);
 
       if (isStatic) {
         var dynType = _emitStaticAccess(type);
@@ -3271,7 +3296,8 @@ class CodeGenerator extends GeneralizingAstVisitor
     ClassElement classElement = element.enclosingElement;
     var type = classElement.type;
     var dynType = _emitStaticAccess(type);
-    var member = _emitMemberName(element.name, isStatic: true, type: type);
+    var member = _emitMemberName(element.name,
+        isStatic: true, type: type, element: element);
     return _visit(rhs).toAssignExpression(
         annotate(new JS.PropertyAccess(dynType, member), lhs));
   }
@@ -3282,7 +3308,7 @@ class CodeGenerator extends GeneralizingAstVisitor
       JS.Expression jsTarget, Element element, JS.Expression value) {
     String memberName = element.name;
     var type = (element.enclosingElement as ClassElement).type;
-    var name = _emitMemberName(memberName, type: type);
+    var name = _emitMemberName(memberName, type: type, element: element);
     return value.toAssignExpression(
         annotate(new JS.PropertyAccess(jsTarget, name), lhs));
   }
@@ -3430,21 +3456,28 @@ class CodeGenerator extends GeneralizingAstVisitor
         return _emitStaticAccess(
             (element.enclosingElement as ClassElement).type);
       }
+      if (element is FieldElement) {
+        var enclosingElement = element.enclosingElement;
+        if (enclosingElement is ClassElement) {

[Jennifer Messerly, 2016/11/29 20:54:52]

the "is" check is unnecessary -- FieldElement is a ClassMemberElement, here is
how it is defined in Analyzer:

abstract class FieldElement
    implements ClassMemberElement ...

abstract class ClassMemberElement implements Element {
  @override
  ClassElement get enclosingElement;
  // ...
}

(I'm surprised this wasn't a hint since that evaluates to "true")

[Jacob, 2016/11/29 21:37:33]

Surprised I ever thought that is check was needed. 

+          return _emitStaticAccess(enclosingElement.type);
+        }
+      }
     }
     return _visit(target);
   }
 
-  /// Emits a (possibly generic) instance method call.
+  /// Emits a (possibly generic) instance, or static method call.
   JS.Expression _emitMethodCallInternal(
       Expression target,
       MethodInvocation node,
       List<JS.Expression> args,
       List<JS.Expression> typeArgs) {
     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);
+    var name = node.methodName.name;
+    var memberName =
+        _emitMemberName(name, type: type, isStatic: isStatic, element: element);
 
     JS.Expression jsTarget = _emitTarget(target, element, isStatic);
     if (isDynamicInvoke(target) || isDynamicInvoke(node.methodName)) {
@@ -4807,7 +4840,7 @@ class CodeGenerator extends GeneralizingAstVisitor
       String memberName, List<JS.Expression> typeArgs) {
     bool isStatic = member is ClassMemberElement && member.isStatic;
     var name = _emitMemberName(memberName,
-        type: getStaticType(target), isStatic: isStatic);
+        type: getStaticType(target), isStatic: isStatic, element: member);
     if (isDynamicInvoke(target)) {
       if (_inWhitelistCode(target)) {
         var vars = <JS.MetaLetVariable, JS.Expression>{};
@@ -5426,9 +5459,14 @@ class CodeGenerator extends GeneralizingAstVisitor
   /// helper, that checks for null. The user defined method is called '=='.
   ///
   JS.Expression _emitMemberName(String name,
-      {DartType type, bool isStatic: false, bool useExtension}) {
-    // Static members skip the rename steps.
-    if (isStatic) return _propertyName(name);
+      {DartType type,
+      bool isStatic: false,
+      bool useExtension,
+      Element element}) {
+    // Static members skip the rename steps and may require JS interop renames.
+    if (isStatic) {
+      return _emitJSInteropStaticMemberName(element) ?? _propertyName(name);
+    }
 
     if (name.startsWith('_')) {
       return _emitPrivateNameSymbol(currentLibrary, name);