[refactor] js_codegen: remove currentClass, simplify visitSimpleIdentifier

We were tracking currentClass for no real reason.

Also attempted to discern what visitSimpleIdentifier was actually doing. It now prefers the original element (the "field" not the synthetic getter/setter) whenever possible. Previously was a mismash.

This little change tripped on the (larger, not fixed yet) issue #138 ... but I think I found a way around the issue for now, by tweaking the private TypeDataArray class.

[email protected]

Review URL: https://codereview.chromium.org/1095563003

Author: John Messerly

pkg/dev_compiler/lib/runtime/dart/_native_typed_data.js

@@ -582,9 +582,6 @@ var _native_typed_data;
   dart.defineNamedConstructor(NativeByteData, 'view');
   let _setRangeFast = Symbol('_setRangeFast');
   class NativeTypedArray extends NativeTypedData {
-    get length() {
-      return dart.as(this.length, core.int);
-    }
     [_setRangeFast](start, end, source, skipCount) {
       let targetLength = this.length;
       this[_checkIndex](start, dart.notNull(targetLength) + 1);
@@ -606,6 +603,9 @@ var _native_typed_data;
   }
   NativeTypedArray[dart.implements] = () => [_js_helper.JavaScriptIndexingBehavior];
   class NativeTypedArrayOfDouble extends dart.mixin(NativeTypedArray, collection.ListMixin$(core.double), _internal.FixedLengthListMixin$(core.double)) {
+    get [core.$length]() {
+      return dart.as(this.length, core.int);
+    }
     [core.$get](index) {
       this[_checkIndex](index, this[core.$length]);
       return this[index];
@@ -625,6 +625,9 @@ var _native_typed_data;
     }
   }
   class NativeTypedArrayOfInt extends dart.mixin(NativeTypedArray, collection.ListMixin$(core.int), _internal.FixedLengthListMixin$(core.int)) {
+    get [core.$length]() {
+      return dart.as(this.length, core.int);
+    }
     [core.$set](index, value) {
       this[_checkIndex](index, this[core.$length]);
       this[index] = value;

pkg/dev_compiler/lib/src/codegen/js_codegen.dart

@@ -65,7 +65,6 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
   /// The variable for the current catch clause
   SimpleIdentifier _catchParameter;
 
-  ClassDeclaration currentClass;
   ConstantEvaluator _constEvaluator;
 
   final _exports = new Set<String>();
@@ -319,8 +318,6 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
     var jsName = getAnnotationValue(node, _isJsNameAnnotation);
     if (jsName != null) return _emitJsType(node.name.name, jsName);
 
-    currentClass = node;
-
     var ctors = <ConstructorDeclaration>[];
     var fields = <FieldDeclaration>[];
     var staticFields = <FieldDeclaration>[];
@@ -337,7 +334,6 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
 
     var body =
         _finishClassMembers(classElem, classExpr, ctors, fields, staticFields);
-    currentClass = null;
 
     return _finishClassDef(type, body);
   }
@@ -1016,49 +1012,55 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
   /// going through the qualified library name if necessary.
   @override
   JS.Expression visitSimpleIdentifier(SimpleIdentifier node) {
-    var e = node.staticElement;
-    if (e == null) {
+    var accessor = node.staticElement;
+    if (accessor == null) {
       return js.commentExpression(
           'Unimplemented unknown name', new JS.Identifier(node.name));
     }
 
-    var variable = e is PropertyAccessorElement ? e.variable : e;
-    var name = variable.name;
+    // Get the original declaring element. If we had a property accessor, this
+    // indirects back to a (possibly synthetic) field.
+    var element = accessor;
+    if (element is PropertyAccessorElement) element = accessor.variable;
+    var name = element.name;
 
     // library member
-    if (e.enclosingElement is CompilationUnitElement &&
-        (e.library != libraryInfo.library ||
-            variable is TopLevelVariableElement && !variable.isConst)) {
-      return js.call('#.#', [_libraryName(e.library), name]);
+    if (element.enclosingElement is CompilationUnitElement &&
+        (element.library != libraryInfo.library ||
+            element is TopLevelVariableElement && !element.isConst)) {
+      return js.call('#.#', [_libraryName(element.library), name]);
     }
 
-    // instance member
-    if (currentClass != null && _needsImplicitThis(e)) {
-      return js.call(
-          'this.#', _emitMemberName(name, type: currentClass.element.type));
-    }
+    // 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;
 
-    // static member
-    if (e is ExecutableElement &&
-        e.isStatic &&
-        variable.enclosingElement is ClassElement) {
-      var className = (variable.enclosingElement as ClassElement).name;
-      return js.call('#.#', [className, _emitMemberName(name, isStatic: true)]);
+      // For instance methods, we add implicit-this.
+      // 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.
+      var target = isStatic ? new JS.Identifier(type.name) : new JS.This();
+      var member = _emitMemberName(name, isStatic: isStatic, type: type);
+      return new JS.PropertyAccess(target, member);
     }
 
     // initializing formal parameter, e.g. `Point(this.x)`
-    if (e is ParameterElement && e.isInitializingFormal && e.isPrivate) {
+    if (element is ParameterElement &&
+        element.isInitializingFormal &&
+        element.isPrivate) {
       /// Rename private names so they don't shadow the private field symbol.
       /// The renamer would handle this, but it would prefer to rename the
       /// temporary used for the private symbol. Instead rename the parameter.
-      return _getTemp(e, '${name.substring(1)}');
+      return _getTemp(element, '${name.substring(1)}');
     }
 
-    if (_isTemporary(e)) {
+    if (_isTemporary(element)) {
       if (name[0] == '#') {
         return new JS.InterpolatedExpression(name.substring(1));
       } else {
-        return _getTemp(e, name);
+        return _getTemp(element, name);
       }
     }
 
@@ -2312,10 +2314,6 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
   }
 
   DartType getStaticType(Expression e) => rules.getStaticType(e);
-
-  static bool _needsImplicitThis(Element e) =>
-      e is PropertyAccessorElement && !e.variable.isStatic ||
-          e is ClassMemberElement && !e.isStatic && e is! ConstructorElement;
 }
 
 class JSGenerator extends CodeGenerator {

pkg/dev_compiler/test/generated_sdk/lib/_internal/compiler/js_lib/native_typed_data.dart

@@ -857,7 +857,8 @@ class NativeByteData extends NativeTypedData implements ByteData {
 
 abstract class NativeTypedArray extends NativeTypedData
     implements JavaScriptIndexingBehavior {
-  int get length => JS('JSUInt32', '#.length', this);
+  // TODO(jmesserly): moved `length` to subclass to (somewhat) mitigate
+  // <https://github.com/dart-lang/dev_compiler/issues/138>
 
   void _setRangeFast(int start, int end,
       NativeTypedArray source, int skipCount) {
@@ -887,6 +888,8 @@ abstract class NativeTypedArrayOfDouble
     extends NativeTypedArray
         with ListMixin<double>, FixedLengthListMixin<double> {
 
+  int get length => JS('JSUInt32', '#.length', this);
+
   num operator[](int index) {
     _checkIndex(index, length);
     return JS('num', '#[#]', this, index);
@@ -912,6 +915,8 @@ abstract class NativeTypedArrayOfInt
         with ListMixin<int>, FixedLengthListMixin<int>
     implements List<int> {
 
+  int get length => JS('JSUInt32', '#.length', this);
+
   // operator[]() is not here since different versions have different return
   // types
 

pkg/dev_compiler/tool/input_sdk/private/native_typed_data.dart

@@ -857,7 +857,8 @@ class NativeByteData extends NativeTypedData implements ByteData {
 
 abstract class NativeTypedArray extends NativeTypedData
     implements JavaScriptIndexingBehavior {
-  int get length => JS('JSUInt32', '#.length', this);
+  // TODO(jmesserly): moved `length` to subclass to (somewhat) mitigate
+  // <https://github.com/dart-lang/dev_compiler/issues/138>
 
   void _setRangeFast(int start, int end,
       NativeTypedArray source, int skipCount) {
@@ -887,6 +888,8 @@ abstract class NativeTypedArrayOfDouble
     extends NativeTypedArray
         with ListMixin<double>, FixedLengthListMixin<double> {
 
+  int get length => JS('JSUInt32', '#.length', this);
+
   num operator[](int index) {
     _checkIndex(index, length);
     return JS('num', '#[#]', this, index);
@@ -912,6 +915,8 @@ abstract class NativeTypedArrayOfInt
         with ListMixin<int>, FixedLengthListMixin<int>
     implements List<int> {
 
+  int get length => JS('JSUInt32', '#.length', this);
+
   // operator[]() is not here since different versions have different return
   // types