fix #30030, fix #27327 - fix tearoffs and various Object member bugs

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 364 matching lines @@
     var properties = <JS.Property>[];
     _libraries.forEach((library, value) {
       // TODO(jacobr): we could specify a short library name instead of the
       // full library uri if we wanted to save space.
       properties.add(new JS.Property(
           js.string(jsLibraryDebuggerName(_libraryRoot, library)), value));
     });
     return new JS.ObjectInitializer(properties, multiline: true);
   }
 
-  List<String> _getJSName(Element e) {
-    if (e.library == null ||
-        findAnnotation(e.library, isPublicJSAnnotation) == null) {
-      return null;
-    }
+  String _getJSNameWithoutGlobal(Element e) {

[vsm, 2017/06/30 18:08:47]

This will still support something like:

@JS('foo.bar.baz')
external get baz;

right?  Looks like we were returning a List<String> only to join it together
later on.  I think you're just skipping that unneeded step.

[Jennifer Messerly, 2017/07/05 21:17:16]

exactly, it just skips the redundant step. I believe there's a test for this too
(under lib/html).

+    if (e.library == null || !_isJSNative(e.library)) return null;
+    bool isJS =
+        e is PropertyAccessorElement && e.variable is TopLevelVariableElement
+            ? e.isExternal
+            : _isJSNative(e);

[vsm, 2017/06/30 18:08:47]

Maybe this should e.isExternal || _isJSNative(e) ?  Or the former should be
folded into the latter?  I think external can appear non-top level.

[Jennifer Messerly, 2017/07/05 21:17:16]

That's what I was wondering too. This is exactly the same logic we used before,
I was refactoring it to figure out what as going on with `external` and this was
my conclusion :)

It didn't make sense to me either. After a chat with JacobR I've refactored it
to make more sense. Everything needs `external` except classes, near as I can
tell.

+    if (!isJS) return null;
 
     var libraryJSName = getAnnotationName(e.library, isPublicJSAnnotation);
-    var libraryPrefix = <String>[];
-    if (libraryJSName != null && libraryJSName.isNotEmpty) {
-      libraryPrefix.addAll(libraryJSName.split('.'));
-    }
-
-    var original = e;
-    var variableElement = e;
-    if (original is PropertyAccessorElement) {
-      variableElement = original.variable;
-      if (original.isSynthetic) e = variableElement;
-    }
-
-    String elementJSName;
-    if (findAnnotation(e, isPublicJSAnnotation) != null) {
-      elementJSName = getAnnotationName(e, isPublicJSAnnotation) ?? '';
-    }
-
-    if (variableElement is TopLevelVariableElement) {
-      elementJSName = _jsInteropStaticMemberName(original);
-    }
-    if (elementJSName == null) return null;
-
-    var elementJSParts = <String>[];
-    if (elementJSName.isNotEmpty) {
-      elementJSParts.addAll(elementJSName.split('.'));
-    } else {
-      elementJSParts.add(e.name);
-    }
-
-    return libraryPrefix..addAll(elementJSParts);
+    var jsName = getAnnotationName(e, isPublicJSAnnotation);
+    jsName ??= e is PropertyAccessorElement ? e.variable.name : e.name;
+    return libraryJSName != null ? '$libraryJSName.$jsName' : jsName;
   }
 
   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));
+    var jsName = _getJSNameWithoutGlobal(e);
+    return jsName != null ? _emitJSInteropForJSName(jsName) : null;
+  }
+
+  JS.Expression _emitJSInteropForJSName(String name) {
+    var access = _callHelper('global');
+    for (var part in name.split('.')) {
+      access = new JS.PropertyAccess(access, js.escapedString(part, "'"));
     }
     return access;
   }
 
-  String _jsInteropStaticMemberName(Element e) {
-    if (e?.library == null ||
-        findAnnotation(e.library, isPublicJSAnnotation) == null) {
-      return null;
-    }
-    if (e is ExecutableElement && e.isExternal) {
-      return getAnnotationName(e, isPublicJSAnnotation) ??
-          (e is PropertyAccessorElement ? e.variable : e).name;
-    }
-    if (e is PropertyInducingElement && e.getter.isExternal) {
-      return getAnnotationName(e, isPublicJSAnnotation) ?? e.name;
-    }
-    return null;
-  }
-
-  String _emitJSInteropStaticMemberName(Element e) {
-    var name = _jsInteropStaticMemberName(e);
+  JS.Expression _emitJSInteropStaticMemberName(Element e) {
+    if (e?.library == null || !_isJSNative(e.library)) return null;
+    bool isJS = e is ExecutableElement && e.isExternal ||
+        e is PropertyInducingElement && e.getter.isExternal;
+    if (!isJS) return null;
+    var name = getAnnotationName(e, isPublicJSAnnotation);
     // We do not support statics names with JS annotations containing dots.
     // See https://github.com/dart-lang/sdk/issues/27926
-    if (name != null && name.contains('.')) {
-      throw new UnsupportedError(
-          'We do not support JS annotations containing dots on static members. '
-          'See https://github.com/dart-lang/sdk/issues/27926');
+    if (name != null) {
+      if (name.contains('.')) {
+        throw new UnsupportedError(
+            'We do not support JS annotations containing dots on static members. '
+            'See https://github.com/dart-lang/sdk/issues/27926');
+      }
+    } else {
+      name = e is PropertyAccessorElement ? e.variable.name : e.name;
     }
-    return name;
+    return js.escapedString(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);
@@ skipping 372 matching lines @@
       ]));
     }
 
     if (isGeneric) {
       return _defineClassTypeArguments(
           classElem, typeFormals, _statement(block));
     }
     return _statement(block);
   }
 
-  JS.Statement _emitJsType(Element e) {
+  JS.Statement _emitJSType(Element e) {
     var jsTypeName = getAnnotationName(e, isJSAnnotation);
     if (jsTypeName == null || jsTypeName == e.name) return null;
 
     // We export the JS type as if it was a Dart type. For example this allows
     // `dom.InputElement` to actually be HTMLInputElement.
     // TODO(jmesserly): if we had the JS name on the Element, we could just
     // generate it correctly when we refer to it.
     return js.statement('# = #;', [_emitTopLevelName(e), jsTypeName]);
   }
 
   @override
   JS.Statement visitClassDeclaration(ClassDeclaration node) {
     var classElem = resolutionMap.elementDeclaredByClassDeclaration(node);
 
     // If this class is annotated with `@JS`, then there is nothing to emit.
     if (findAnnotation(classElem, isPublicJSAnnotation) != null) return null;
 
     // If this is a JavaScript type, emit it now and then exit.
-    var jsTypeDef = _emitJsType(classElem);
+    var jsTypeDef = _emitJSType(classElem);
     if (jsTypeDef != null) return jsTypeDef;
 
     var ctors = <ConstructorDeclaration>[];
     var allFields = <FieldDeclaration>[];
     var fields = <FieldDeclaration>[];
     var staticFields = <FieldDeclaration>[];
     var methods = <MethodDeclaration>[];
 
     for (var member in node.members) {
       if (member is ConstructorDeclaration) {
@@ skipping 802 matching lines @@
         _runtimeModule,
         _propertyName(jsPeerName),
         _emitTopLevelName(classElem)
       ]));
     }
   }
 
   JS.Statement _setBaseClass(ClassElement classElem, JS.Expression className,
       List<String> jsPeerNames, List<JS.Statement> body) {
     var typeFormals = classElem.typeParameters;
-    if (jsPeerNames.isNotEmpty && typeFormals.isNotEmpty) {
-      for (var peer in jsPeerNames) {
-        // TODO(jmesserly): we should just extend Array in the first place
-        var newBaseClass = _callHelper('global.#', [peer]);
-        body.add(_callHelperStatement(
-            'setExtensionBaseClass(#, #);', [className, newBaseClass]));
-      }
+    if (jsPeerNames.length == 1 && typeFormals.isNotEmpty) {
+      var newBaseClass = _callHelper('global.#', jsPeerNames[0]);
+      body.add(_callHelperStatement(
+          'setExtensionBaseClass(#, #);', [className, newBaseClass]));

[vsm, 2017/06/30 18:08:47]

Ha - I was just trying to figure why JSArray's proto was different from
everything else while looking at cross-frame code.  Thanks for the pointer.  :-)

[Jennifer Messerly, 2017/07/05 21:17:16]

yup! it's pretty crazy ... hope it's a tad better documented now

     } else if (_hasDeferredSupertype.contains(classElem)) {
       // TODO(vsm): consider just threading the deferred supertype through
       // instead of recording classElem in a set on the class and recomputing
       var newBaseClass = _emitType(classElem.type.superclass,
           nameType: false, subClass: classElem, className: className);
       if (classElem.type.mixins.isNotEmpty) {
         var mixins = classElem.type.mixins
             .map((t) => _emitType(t, nameType: false))
             .toList();
         mixins.insert(0, newBaseClass);
@@ skipping 1034 matching lines @@
     }
     return _callHelper('gbind(#, #)', [simpleId, typeArgs]);
   }
 
   /// Emits a simple identifier, handling implicit `this` as well as
   /// going through the qualified library name if necessary, but *not* handling
   /// inferred generic function instantiation.
   JS.Expression _emitSimpleIdentifier(SimpleIdentifier node) {
     var accessor = resolutionMap.staticElementForIdentifier(node);
     if (accessor == null) {
-      return _callHelper(
-          'throw(Error("compile error: unresolved identifier: " + #))',
-          js.escapedString(node.name ?? '<null>'));
+      return _throwUnsafe('unresolved identifier: ' + (node.name ?? '<null>'));
     }
 
     // Get the original declaring element. If we had a property accessor, this
     // indirects back to a (possibly synthetic) field.
     var element = accessor;
     if (accessor is PropertyAccessorElement) element = accessor.variable;
 
     // type literal
     if (element is TypeDefiningElement) {
       _declareBeforeUse(element);
@@ skipping 24 matching lines @@
       var member = _emitMemberName(name,
           isStatic: isStatic, type: type, element: accessor);
 
       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) {
-        // To be safe always use the symbolized name when binding on a native
-        // class as bind assumes the name will match the name class sigatures
-        // which is symbolized for native classes.
-        var safeName = _emitMemberName(name,
-            isStatic: isStatic,
-            type: type,
-            element: accessor,
-            alwaysSymbolizeNative: true);
-        return _callHelper('bind(this, #)', safeName);
+      if (element is MethodElement &&
+          !inInvocationContext(node) &&
+          !_isJSNative(element.enclosingElement)) {
+        return _callHelper('bind(this, #)', member);
       }
       return js.call('this.#', member);
     }
 
     if (element is ParameterElement) {
       return _emitParameter(element);
     }
 
     // If this is one of our compiler's temporary variables, return its JS form.
     if (element is TemporaryVariableElement) {
@@ skipping 242 matching lines @@
       return _callHelper('dynamic');
     } else if (type.isBottom) {
       return _callHelper('bottom');
     }
 
     var element = type.element;
     if (element is TypeDefiningElement) {
       _declareBeforeUse(element);
     }
 
-    var interop = _emitJSInterop(element);
     // Type parameters don't matter as JS interop types cannot be reified.
     // We have to use lazy JS types because until we have proper module
     // loading for JS libraries bundled with Dart libraries, we will sometimes
     // need to load Dart libraries before the corresponding JS libraries are
     // actually loaded.
     // Given a JS type such as:
     //     @JS('google.maps.Location')
     //     class Location { ... }
     // We can't emit a reference to MyType because the JS library that defines
     // it may be loaded after our code. So for now, we use a special lazy type
     // object to represent MyType.
     // Anonymous JS types do not have a corresponding concrete JS type so we
     // have to use a helper to define them.
-    if (interop != null) {
-      if (_isObjectLiteral(element)) {
-        return _callHelper(
-            'lazyAnonymousJSType(#)', js.string(element.displayName));
-      } else {
-        return _callHelper('lazyJSType(() => #, #)',
-            [interop, js.string(_getJSName(element).join('.'))]);
-      }
+    if (_isObjectLiteral(element)) {
+      return _callHelper('anonymousJSType(#)', js.escapedString(element.name));
+    }
+    var jsName = _getJSNameWithoutGlobal(element);
+    if (jsName != null) {
+      return _callHelper('lazyJSType(() => #, #)',
+          [_emitJSInteropForJSName(jsName), js.escapedString(jsName)]);
     }
 
     // TODO(jmesserly): like constants, should we hoist function types out of
     // methods? Similar issue with generic types. For all of these, we may want
     // to canonicalize them too, at least when inside the same library.
     var name = type.name;
     if (name == '' || name == null || lowerTypedef) {
       // TODO(jmesserly): should we change how typedefs work? They currently
       // go through use similar logic as generic classes. This makes them
       // different from universal function types.
@@ skipping 14 matching lines @@
       if (args.any((a) => !a.isDynamic)) {
         jsArgs = args.map((x) => _emitType(x,
             nameType: nameType,
             hoistType: hoistType,
             subClass: subClass,
             className: className));
       } else if (lowerGeneric || element == subClass) {
         jsArgs = [];
       }
       if (jsArgs != null) {
-        var genericName = _emitTopLevelName(element, suffix: '\$');
+        var genericName = _emitTopLevelNameNoInterop(element, suffix: '\$');
         var typeRep = js.call('#(#)', [genericName, jsArgs]);
         return nameType
             ? _typeTable.nameType(type, typeRep, hoistType: hoistType)
             : typeRep;
       }
     }
 
     return _emitTopLevelNameNoInterop(element);
   }
 
   JS.PropertyAccess _emitTopLevelName(Element e, {String suffix: ''}) {
-    var interop = _emitJSInterop(e);
-    if (interop != null) return interop;
-    return _emitTopLevelNameNoInterop(e, suffix: suffix);
+    return _emitJSInterop(e) ?? _emitTopLevelNameNoInterop(e, suffix: suffix);
   }
 
   JS.PropertyAccess _emitTopLevelNameNoInterop(Element e, {String suffix: ''}) {
-    String name = getJSExportName(e) + suffix;
+    var name = getJSExportName(e) ??
+        (e is PropertyAccessorElement ? e.variable.name : e.name);

[vsm, 2017/06/30 18:08:47]

Consider factoring out - this guarded expr seems frequent.

[Jennifer Messerly, 2017/07/05 21:17:16]

Done.

     return new JS.PropertyAccess(
-        emitLibraryName(e.library), _propertyName(name));
+        emitLibraryName(e.library), _propertyName(name + suffix));
   }
 
   @override
   JS.Expression visitAssignmentExpression(AssignmentExpression node) {
     var left = node.leftHandSide;
     var right = node.rightHandSide;
     if (node.operator.type == TokenType.EQ) return _emitSet(left, right);
     var op = node.operator.lexeme;
     assert(op.endsWith('='));
     op = op.substring(0, op.length - 1); // remove trailing '='
@@ skipping 277 matching lines @@
     if (!_superAllowed && jsTarget is JS.Super) {
       return _getSuperHelper(member, jsName)
         ..sourceInformation = jsTarget.sourceInformation;
     }
     return new JS.PropertyAccess(jsTarget, jsName);
   }
 
   JS.Expression _getSuperHelper(Element member, JS.Expression jsName) {
     var jsMethod = _superHelpers.putIfAbsent(member.name, () {
       if (member is PropertyAccessorElement) {
-        var field = member.variable as FieldElement;
-        var name = field.name;
         var isSetter = member.isSetter;
         var fn = js.call(
             isSetter
                 ? 'function(x) { super[#] = x; }'
                 : 'function() { return super[#]; }',
             [jsName]);
-        return new JS.Method(new JS.TemporaryId(name), fn,
+        return new JS.Method(new JS.TemporaryId(member.variable.name), fn,
             isGetter: !isSetter, isSetter: isSetter);
       } else {
         var method = member as MethodElement;
         var name = method.name;
         // For generic methods, we can simply pass along the type arguments,
         // and let the resulting closure accept the actual arguments.
         List<JS.Identifier> params;
         if (method.typeParameters.isNotEmpty) {
           params = _emitTypeFormals(method.typeParameters);
         } else {
@@ skipping 15 matching lines @@
   }
 
   JS.Expression _emitMethodCall(Expression target, MethodInvocation node) {
     var args = _emitArgumentList(node.argumentList);
     var typeArgs = _emitInvokeTypeArguments(node);
 
     var type = getStaticType(target);
     var element = node.methodName.staticElement;
     bool isStatic = element is ExecutableElement && element.isStatic;
     var name = node.methodName.name;
-    var memberName =
+    var jsName =
         _emitMemberName(name, type: type, isStatic: isStatic, element: element);
 
     JS.Expression jsTarget = _emitTarget(target, element, isStatic);
     if (isDynamicInvoke(target) || isDynamicInvoke(node.methodName)) {
       if (typeArgs != null) {
         return _callHelper('#(#, #, #, #)', [
           _emitDynamicOperationName('dgsend'),
           jsTarget,
           new JS.ArrayInitializer(typeArgs),
-          memberName,
+          jsName,
           args
         ]);
       } else {
         return _callHelper('#(#, #, #)',
-            [_emitDynamicOperationName('dsend'), jsTarget, memberName, args]);
+            [_emitDynamicOperationName('dsend'), jsTarget, jsName, args]);
       }
     }
     if (_isObjectMemberCall(target, name)) {
       assert(typeArgs == null); // Object methods don't take type args.
       return _callHelper('#(#, #)', [name, jsTarget, args]);
     }
-    jsTarget = _emitTargetAccess(jsTarget, memberName, element);
+    jsTarget = _emitTargetAccess(jsTarget, jsName, element);
     if (typeArgs != null) jsTarget = new JS.Call(jsTarget, typeArgs);
 
     return new JS.Call(jsTarget, args);
   }
 
   JS.Expression _emitDynamicInvoke(
       InvocationExpression node, JS.Expression fn, List<JS.Expression> args) {
     var typeArgs = _emitInvokeTypeArguments(node);
     if (typeArgs != null) {
       return _callHelper(
@@ skipping 83 matching lines @@
     return f.typeArguments.skip(g.typeArguments.length);
   }
 
   /// Emits code for the `JS(...)` macro.
   _emitForeignJS(MethodInvocation node) {
     var e = node.methodName.staticElement;
     if (isInlineJS(e)) {
       var args = node.argumentList.arguments;
       // arg[0] is static return type, used in `RestrictedStaticTypeAnalyzer`
       var code = args[1];
-      List<AstNode> templateArgs;
+      List<Expression> templateArgs;
       String source;
       if (code is StringInterpolation) {
         if (args.length > 2) {
           throw new ArgumentError(
               "Can't mix template args and string interpolation in JS calls.");
         }
         templateArgs = <Expression>[];
         source = code.elements.map((element) {
           if (element is InterpolationExpression) {
             templateArgs.add(element.expression);
@@ skipping 22 matching lines @@
             _extensionTypes.isNativeClass(containingClass.element)) {
           var constructorName = source.substring(4, source.indexOf('('));
           var className = containingClass.name.name;
           if (className == constructorName) {
             source =
                 source.replaceFirst('new $className(', 'new self.$className(');
           }
         }
       }
 
+      JS.Expression visitTemplateArg(Expression arg) {
+        if (arg is InvocationExpression) {
+          var e = arg is MethodInvocation
+              ? arg.methodName.staticElement
+              : (arg as FunctionExpressionInvocation).staticElement;
+          if (e?.name == 'getGenericClass' &&
+              e.library.name == 'dart._runtime' &&
+              arg.argumentList.arguments.length == 1) {
+            var typeArg = arg.argumentList.arguments[0];
+            if (typeArg is SimpleIdentifier) {
+              var typeElem = typeArg.staticElement;
+              if (typeElem is TypeDefiningElement &&
+                  typeElem.type is ParameterizedType) {
+                return _emitTopLevelNameNoInterop(typeElem, suffix: '\$');
+              }
+            }
+          }
+        }
+        return _visit(arg);
+      }
+
       // TODO(rnystrom): The JS() calls are almost never nested, and probably
       // really shouldn't be, but there are at least a couple of calls in the
       // HTML library where an argument to JS() is itself a JS() call. If those
       // go away, this can just assert(!_isInForeignJS).
       // Inside JS(), type names evaluate to the raw runtime type, not the
       // wrapped Type object.
       var wasInForeignJS = _isInForeignJS;
       _isInForeignJS = true;
+      var jsArgs = templateArgs.map(visitTemplateArg).toList();
+      _isInForeignJS = wasInForeignJS;
 
-      var template = js.parseForeignJS(source);
-      var result = template.instantiate(_visitList(templateArgs));
-
-      _isInForeignJS = wasInForeignJS;
+      var result = js.parseForeignJS(source).instantiate(jsArgs);
 
       // `throw` is emitted as a statement by `parseForeignJS`.
       assert(result is JS.Expression || node.parent is ExpressionStatement);
       return result;
     }
     return null;
   }
 
   @override
   JS.Expression visitFunctionExpressionInvocation(
@@ skipping 307 matching lines @@
       ConstructorElement element,
       DartType type,
       SimpleIdentifier name,
       ArgumentList argumentList,
       bool isConst) {
     JS.Expression emitNew() {
       JS.Expression ctor;
       bool isFactory = false;
       bool isNative = false;
       if (element == null) {
-        ctor = _callHelper(
-            'throw(Error("compile error: unresolved constructor: " '
-            '+ # + "." + #))',
-            [
-              js.escapedString(type?.name ?? '<null>'),
-              js.escapedString(name?.name ?? '<unnamed>')
-            ]);
+        ctor = _throwUnsafe('unresolved constructor: ${type?.name ?? '<null>'}'
+            '.${name?.name ?? '<unnamed>'}');
       } else {
         ctor = _emitConstructorName(element, type, name);
         isFactory = element.isFactory;
         var classElem = element.enclosingElement;
         isNative = _isJSNative(classElem);
       }
       var args = _emitArgumentList(argumentList);
       // Native factory constructors are JS constructors - use new here.
       return isFactory && !isNative
           ? new JS.Call(ctor, args)
           : new JS.New(ctor, args);
     }
 
     if (element != null && _isObjectLiteral(element.enclosingElement)) {
       return _emitObjectLiteral(argumentList);
     }
     if (isConst) return _emitConst(emitNew);
     return emitNew();
   }
 
-  bool _isObjectLiteral(ClassElement classElem) {
-    return findAnnotation(classElem, isPublicJSAnnotation) != null &&
+  bool _isObjectLiteral(Element classElem) {
+    return _isJSNative(classElem) &&
         findAnnotation(classElem, isJSAnonymousAnnotation) != null;
   }
 
-  bool _isJSNative(ClassElement classElem) =>
-      findAnnotation(classElem, isPublicJSAnnotation) != null;
+  bool _isJSNative(Element e) =>
+      findAnnotation(e, isPublicJSAnnotation) != null;
 
   JS.Expression _emitObjectLiteral(ArgumentList argumentList) {
     var args = _emitArgumentList(argumentList);
     if (args.isEmpty) {
       return js.call('{}');
     }
     assert(args.single is JS.ObjectInitializer);
     return args.single;
   }
 
@@ skipping 237 matching lines @@
     }
     return false;
   }
 
   int _asIntInRange(Expression expr, int low, int high) {
     expr = expr.unParenthesized;
     if (expr is IntegerLiteral) {
       if (expr.value >= low && expr.value <= high) return expr.value;
       return null;
     }
-    int finishIdentifier(SimpleIdentifier identifier) {
-      Element staticElement = identifier.staticElement;
-      if (staticElement is PropertyAccessorElement && staticElement.isGetter) {
-        PropertyInducingElement variable = staticElement.variable;
-        int value = variable?.computeConstantValue()?.toIntValue();
-        if (value != null && value >= low && value <= high) return value;
-      }
+
+    Identifier id;
+    if (expr is SimpleIdentifier) {
+      id = expr;
+    } else if (expr is PrefixedIdentifier && !expr.isDeferred) {
+      id = expr.identifier;
+    } else {
       return null;
     }
-
-    if (expr is SimpleIdentifier) {
-      return finishIdentifier(expr);
-    } else if (expr is PrefixedIdentifier && !expr.isDeferred) {
-      return finishIdentifier(expr.identifier);
+    var elmement = id.staticElement;

[vsm, 2017/06/30 18:08:47]

elmement -> element

[Jennifer Messerly, 2017/07/05 21:17:16]

Done.

+    if (elmement is PropertyAccessorElement && elmement.isGetter) {
+      var variable = elmement.variable;
+      int value = variable?.computeConstantValue()?.toIntValue();
+      if (value != null && value >= low && value <= high) return value;
     }
     return null;
   }
 
   bool _isDefinitelyNonNegative(Expression expr) {
     expr = expr.unParenthesized;
     if (expr is IntegerLiteral) {
       return expr.value >= 0;
     }
     if (_nodeIsBitwiseOperation(expr)) return true;
@@ skipping 447 matching lines @@
   JS.Expression _emitAccess(
       Expression target, SimpleIdentifier memberId, DartType resultType) {
     var accessor = memberId.staticElement;
     // If `member` is a getter/setter, get the corresponding
     var field =
         accessor is PropertyAccessorElement ? accessor.variable : accessor;
     String memberName = memberId.name;
     var typeArgs = _getTypeArgs(accessor, resultType);
 
     bool isStatic = field is ClassMemberElement && field.isStatic;
-    var name = _emitMemberName(memberName,
+    var jsName = _emitMemberName(memberName,
         type: getStaticType(target), isStatic: isStatic, element: accessor);
     if (isDynamicInvoke(target)) {
       return _callHelper('#(#, #)',
-          [_emitDynamicOperationName('dload'), _visit(target), name]);
+          [_emitDynamicOperationName('dload'), _visit(target), jsName]);
     }
 
     var jsTarget = _emitTarget(target, accessor, isStatic);
 
     var isSuper = jsTarget is JS.Super;
     if (isSuper &&
         accessor.isSynthetic &&
         field is FieldElementImpl &&
         !virtualFields.isVirtual(field)) {
       // If super.x is a sealed field, then x is an instance property since
       // subclasses cannot override x.
       jsTarget = annotate(new JS.This(), target);
     }
 
     JS.Expression result;
-    if (accessor is MethodElement && !isStatic) {
-      // Tear-off methods: explicitly bind it.
-      // To be safe always use the symbolized name when binding on a native
-      // class as bind assumes the name will match the name class signatures
-      // which is symbolized for native classes.
-      var safeName = _emitMemberName(memberName,
-          type: getStaticType(target),
-          isStatic: isStatic,
-          element: accessor,
-          alwaysSymbolizeNative: true);
+    if (_isObjectMemberCall(target, memberName)) {
+      if (_isObjectMethod(memberName)) {
+        result = _callHelper('bind(#, #)', [jsTarget, jsName]);
+      } else {
+        result = _callHelper('#(#)', [memberName, jsTarget]);
+      }
+    } else if (accessor is MethodElement &&
+        !isStatic &&
+        !_isJSNative(accessor.enclosingElement)) {
       if (isSuper) {
         result = _callHelper('bind(this, #, #)',
-            [safeName, _emitTargetAccess(jsTarget, name, accessor)]);
-      } else if (_isObjectMemberCall(target, memberName)) {
-        var fn = js.call(
-            memberName == 'noSuchMethod'
-                ? 'function(i) { return #.#(this, i); }'
-                : 'function() { return #.#(this); }',
-            [_runtimeModule, memberName]);
-        result = _callHelper(
-            'bind(#, #, #)', [jsTarget, _propertyName(memberName), fn]);
+            [jsName, _emitTargetAccess(jsTarget, jsName, accessor)]);
       } else {
-        result = _callHelper('bind(#, #)', [jsTarget, safeName]);
+        result = _callHelper('bind(#, #)', [jsTarget, jsName]);
       }
-    } else if (_isObjectMemberCall(target, memberName)) {
-      result = _callHelper('#(#)', [memberName, jsTarget]);
     } else {
-      result = _emitTargetAccess(jsTarget, name, accessor);
+      result = _emitTargetAccess(jsTarget, jsName, accessor);
     }
-    if (typeArgs == null) {
-      return result;
-    }
-    return _callHelper('gbind(#, #)', [result, typeArgs]);
+    return typeArgs == null
+        ? result
+        : _callHelper('gbind(#, #)', [result, typeArgs]);
   }
 
   JS.LiteralString _emitDynamicOperationName(String name) =>
       js.string(options.replCompile ? '${name}Repl' : name);
 
   /// Emits a generic send, like an operator method.
   ///
   /// **Please note** this function does not support method invocation syntax
   /// `obj.name(args)` because that could be a getter followed by a call.
   /// See [visitMethodInvocation].
@@ skipping 490 matching lines @@
     JS.Expression result = _visit(node);
     if (isNullable(node)) result = _callHelper('test(#)', result);
     return result;
   }
 
   /// Like [_emitMemberName], but for declaration sites.
   ///
   /// Unlike call sites, we always have an element available, so we can use it
   /// directly rather than computing the relevant options for [_emitMemberName].
   JS.Expression _declareMemberName(ExecutableElement e, {bool useExtension}) {
-    var name = (e is PropertyAccessorElement) ? e.variable.name : e.name;
+    var name = e is PropertyAccessorElement ? e.variable.name : e.name;
     return _emitMemberName(name,
         isStatic: e.isStatic,
         useExtension:
-            useExtension ?? _extensionTypes.isNativeClass(e.enclosingElement));
+            useExtension ?? _extensionTypes.isNativeClass(e.enclosingElement),
+        element: e);
   }
 
   /// This handles member renaming for private names and operators.
   ///
   /// Private names are generated using ES6 symbols:
   ///
   ///     // At the top of the module:
   ///     let _x = Symbol('_x');
   ///     let _y = Symbol('_y');
   ///     ...
@@ skipping 26 matching lines @@
   ///
   /// 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,
-      bool alwaysSymbolizeNative: false,
       Element element}) {
     // Static members skip the rename steps and may require JS interop renames.
     if (isStatic) {
-      return _propertyName(_emitJSInteropStaticMemberName(element) ?? name);
+      return _emitJSInteropStaticMemberName(element) ?? _propertyName(name);
+    }
+
+    // We allow some (illegal in Dart) member names to be used in our private
+    // SDK code. These renames need to be included at every declaration,
+    // including overrides in subclasses.
+    if (element != null) {
+      var runtimeName = getJSExportName(element);
+      if (runtimeName != null) return _propertyName(runtimeName);
     }
 
     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';
         break;
       case '[]=':
         name = '_set';
         break;
       case 'unary-':
         name = '_negate';
         break;
       case 'constructor':
       case 'prototype':
         name = '_$name';
         break;
     }
 
     var result = _propertyName(name);
 
-    if (useExtension == null) {
-      // Dart "extension" methods. Used for JS Array, Boolean, Number, String.
-      var baseType = type;
-      while (baseType is TypeParameterType) {
-        baseType = (baseType.element as TypeParameterElement).bound;
-      }
-      useExtension = baseType is InterfaceType &&
-          _isSymbolizedMember(baseType, name, alwaysSymbolizeNative);
-    }
+    useExtension ??= _isSymbolizedMember(type, name);
 
     return useExtension
         ? js.call('#.#', [_extensionSymbolsModule, result])
         : result;
   }
 
   var _forwardingCache = new HashMap<Element, Map<String, ExecutableElement>>();
 
   Element _lookupForwardedMember(ClassElement element, String name) {
     // We only care about public methods.
@@ skipping 16 matching lines @@
     return member;
   }
 
   /// Don't symbolize native members that just forward to the underlying
   /// native member.  We limit this to non-renamed members as the receiver
   /// may be a mock type.
   ///
   /// Note, this is an underlying assumption here that, if another native type
   /// subtypes this one, it also forwards this member to its underlying native
   /// one without renaming.
-  bool _isSymbolizedMember(
-      InterfaceType type, String name, bool alwaysSymbolizeNative) {
+  bool _isSymbolizedMember(DartType type, String name) {
     // Object members are handled separately.
     if (isObjectMember(name)) {
       return false;
     }
 
-    var element = type.element;
-    if (_extensionTypes.isNativeClass(element)) {
-      var member = _lookupForwardedMember(element, name);
+    while (type is TypeParameterType) {
+      type = (type as TypeParameterType).bound;
+    }
+    if (type is InterfaceType) {
+      var element = type.element;
+      if (_extensionTypes.isNativeClass(element)) {
+        var member = _lookupForwardedMember(element, name);
 
-      // Fields on a native class are implicitly native.
-      // Methods/getters/setters are marked external/native.
-      if (member is FieldElement ||
-          member is ExecutableElement && member.isExternal) {
-        var jsName = getAnnotationName(member, isJsName);
-        return alwaysSymbolizeNative || (jsName != null && jsName != name);
-      } else {
-        // Non-external members must be symbolized.
-        return true;
+        // Fields on a native class are implicitly native.
+        // Methods/getters/setters are marked external/native.
+        if (member is FieldElement ||
+            member is ExecutableElement && member.isExternal) {
+          var jsName = getAnnotationName(member, isJsName);
+          return jsName != null && jsName != name;
+        } else {
+          // Non-external members must be symbolized.
+          return true;
+        }
       }
+      // If the receiver *may* be a native type (i.e., an interface allowed to
+      // be implemented by a native class), conservatively symbolize - we don't
+      // whether it'll be implemented via forwarding.

[vsm, 2017/06/30 18:08:48]

"whether" -> "know whether"

[Jennifer Messerly, 2017/07/05 21:17:16]

oh funny, we must've missed this in the original CL (this typo was in the
original code). But yeah, good catch, fixed!

+      // TODO(vsm): Consider CHA here to be less conservative.
+      return _extensionTypes.isNativeInterface(element);
     }
-    // If the receiver *may* be a native type (i.e., an interface allowed to
-    // be implemented by a native class), conservatively symbolize - we don't
-    // whether it'll be implemented via forwarding.
-    // TODO(vsm): Consider CHA here to be less conservative.
-    return _extensionTypes.isNativeInterface(element);
+    return false;
   }
 
   JS.TemporaryId _emitPrivateNameSymbol(LibraryElement library, String name) {
     return _privateNames
         .putIfAbsent(library, () => new HashMap())
         .putIfAbsent(name, () {
       var id = new JS.TemporaryId(name);
       _moduleItems.add(
           js.statement('const # = Symbol(#);', [id, js.string(id.name, "'")]));
       return id;
@@ skipping 43 matching lines @@
     switch (name) {
       case 'hashCode':
       case 'toString':
       case 'noSuchMethod':
       case 'runtimeType':
         return true;
     }
     return false;
   }
 
+  bool _isObjectMethod(String name) =>
+      name == 'toString' || name == 'noSuchMethod';
+
   // TODO(leafp): Various analyzer pieces computed similar things.
   // Share this logic somewhere?
   DartType _getExpectedReturnType(ExecutableElement element) {
     FunctionType functionType = element.type;
     if (functionType == null) {
       return DynamicTypeImpl.instance;
     }
     var type = functionType.returnType;
 
     InterfaceType expectedType = null;
@@ skipping 38 matching lines @@
 
   JS.Statement _callHelperStatement(String code, args) {
     if (args is List) {
       args.insert(0, _runtimeModule);
     } else {
       args = [_runtimeModule, args];
     }
     return js.statement('#.$code', args);
   }
 
+  JS.Expression _throwUnsafe(String message) => _callHelper(
+      'throw(Error(#))', js.escapedString("compile error: $message"));
+
   _unreachable(AstNode node) {
     throw new UnsupportedError(
         'tried to generate an unreachable node: `$node`');
   }
 
   /// Unused, see methods for emitting declarations.
   @override
   visitAnnotation(node) => _unreachable(node);
 
   /// Unused, see [_emitArgumentList].
@@ skipping 207 matching lines @@
   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 = resolutionMap
       .elementDeclaredByCompilationUnit(target.root as CompilationUnit)
       .library;
   var imports = containingLibrary.getImportsWithPrefix(prefix);
   return imports.length == 1 && imports[0].isDeferred;
 }