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

pkg/dev_compiler/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';
@@ skipping 306 matching lines @@
     var libraryPrefix = <String>[];
     if (libraryJSName != null && libraryJSName.isNotEmpty) {
       libraryPrefix.addAll(libraryJSName.split('.'));
     }
 
     String elementJSName;
     if (findAnnotation(e, isPublicJSAnnotation) != null) {
       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;
 
     var elementJSParts = <String>[];
     if (elementJSName.isNotEmpty) {
       elementJSParts.addAll(elementJSName.split('.'));
     } else {
       elementJSParts.add(e.name);
     }
 
     return libraryPrefix..addAll(elementJSParts);
   }
 
   JS.Expression _emitJSInterop(Element e) {
     var jsName = _getJSName(e);
     if (jsName == null) return null;
     var fullName = ['global']..addAll(jsName);
     JS.Expression access = _runtimeModule;
     for (var part in fullName) {
       access = new JS.PropertyAccess(access, js.string(part));
     }
     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.
   void _copyAndFlattenBlocks(
       List<JS.ModuleItem> result, Iterable<JS.ModuleItem> items) {
     for (var item in items) {
       if (item is JS.Block && !item.isScope) {
         _copyAndFlattenBlocks(result, item.statements);
       } else {
         result.add(item);
@@ skipping 2332 matching lines @@
       return _emitTopLevelName(element);
     }
 
     var name = element.name;
 
     // Unqualified class member. This could mean implicit-this, or implicit
     // call to a static from the same class.
     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);
         return new JS.PropertyAccess(dynType, member);
       }
 
       // For instance members, we add implicit-this.
       // For method tear-offs, we ensure it's a bound method.
       var tearOff = element is MethodElement && !inInvocationContext(node);
       if (tearOff) return _callHelper('bind(this, #)', member);
@@ skipping 545 matching lines @@
 
   /// Emits assignment to a static field element or property.
   JS.Expression _emitSetStaticProperty(
       Expression lhs, Element element, Expression rhs) {
     // For static methods, we add the raw type name, without generics or
     // library prefix. We don't need those because static calls can't use
     // the generic type.
     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));
   }
 
   /// Emits an assignment to the [element] property of instance referenced by
   /// [jsTarget].
   JS.Expression _emitWriteInstanceProperty(Expression lhs,
       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));
   }
 
   JS.Expression _emitSetSuper(Expression lhs, SuperExpression target,
       SimpleIdentifier id, Expression rhs) {
     // TODO(sra): Determine whether and access helper is required for the
     // setter. For now fall back on the r-value path.
     return _visit(rhs).toAssignExpression(_visit(lhs));
   }
@@ skipping 127 matching lines @@
 
   JS.Expression _emitTarget(Expression target, Element element, bool isStatic) {
     if (isStatic) {
       if (element is ConstructorElement) {
         return _emitConstructorAccess(element.enclosingElement.type);
       }
       if (element is ExecutableElement) {
         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)) {
       if (_inWhitelistCode(target)) {
         var vars = <JS.MetaLetVariable, JS.Expression>{};
         var l = _visit(_bindValue(vars, 'l', target));
         jsTarget = new JS.MetaLet(vars, [
           js.call('(#[(#[#._extensionType]) ? #[#] : #]).bind(#)', [
             l,
             l,
@@ skipping 1342 matching lines @@
     return _emitFunctionTypeArguments(type, instantiated);
   }
 
   JS.LiteralString _emitDynamicOperationName(String name) =>
       js.string(options.replCompile ? '${name}Repl' : name);
 
   JS.Expression _emitAccessInternal(Expression target, Element member,
       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>{};
         var l = _visit(_bindValue(vars, 'l', target));
         return new JS.MetaLet(vars, [
           js.call('(#[#._extensionType]) ? #[#[#]] : #.#',
               [l, _runtimeModule, l, _extensionSymbolsModule, name, l, name])
         ]);
       }
       return _callHelper('#(#, #)',
@@ skipping 598 matching lines @@
   ///
   /// This follows the same pattern as ECMAScript 6 Map:
   /// <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map>
   ///
   /// Unary minus looks like: `x._negate()`.
   ///
   /// 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 _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);
     }
 
     // When generating synthetic names, we use _ as the prefix, since Dart names
     // won't have this (eliminated above), nor will static names reach here.
     switch (name) {
       case '[]':
         name = '_get';
@@ skipping 275 matching lines @@
   var targetIdentifier = target as SimpleIdentifier;
 
   if (targetIdentifier.staticElement is! PrefixElement) return false;
   var prefix = targetIdentifier.staticElement as PrefixElement;
 
   // The library the prefix is referring to must come from a deferred import.
   var containingLibrary = (target.root as CompilationUnit).element.library;
   var imports = containingLibrary.getImportsWithPrefix(prefix);
   return imports.length == 1 && imports[0].isDeferred;
 }