Remove virtualField helper, just emit getters/setters in codegen.

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 'package:analyzer/analyzer.dart' hide ConstantEvaluator;
 import 'package:analyzer/dart/ast/ast.dart' hide ConstantEvaluator;
 import 'package:analyzer/dart/ast/token.dart' show Token, TokenType;
 import 'package:analyzer/dart/element/element.dart';
@@ skipping 63 matching lines @@
   Expression _cascadeTarget;
 
   /// The variable for the current catch clause
   SimpleIdentifier _catchParameter;
 
   /// In an async* function, this represents the stream controller parameter.
   JS.TemporaryId _asyncStarController;
 
   final _privateNames =
       new HashMap<LibraryElement, HashMap<String, JS.TemporaryId>>();
+  final _virtualFields = new HashMap<VariableDeclaration, JS.TemporaryId>();

[Jennifer Messerly, 2016/04/25 22:47:04]

Generally we don't use AST nodes as hash keys. Shouldn't we be able to not visit
this AST node twice?

(if it's unavoidable, the Element is more typically used to uniquely identify
something.)

[Harry Terkelsen, 2016/04/26 18:29:27]

Done.

+  final _virtualFieldSymbols = new HashMap<ClassElement, List<JS.Statement>>();

[Jennifer Messerly, 2016/04/25 22:47:04]

do we need this map? could we simply keep a List<JS.Statement> scoped to the
class? I can't imagine this would be needed outside of visitClassDeclaration's
call tree

[Harry Terkelsen, 2016/04/26 18:29:27]

Done.

   final _initializingFormalTemps =
       new HashMap<ParameterElement, JS.TemporaryId>();
 
   final _dartxVar = new JS.Identifier('dartx');
   final _runtimeLibVar = new JS.Identifier('dart');
   final namedArgumentTemp = new JS.TemporaryId('opts');
 
   final _hasDeferredSupertype = new HashSet<ClassElement>();
 
   /// The  type provider from the current Analysis [context].
@@ skipping 430 matching lines @@
     for (var member in node.members) {
       if (member is ConstructorDeclaration) {
         ctors.add(member);
       } else if (member is FieldDeclaration) {
         (member.isStatic ? staticFields : fields).add(member);
       } else if (member is MethodDeclaration) {
         methods.add(member);
       }
     }
 
+    JS.Expression className;
+    if (classElem.typeParameters.isNotEmpty) {
+      className = new JS.Identifier(classElem.name);

[Jennifer Messerly, 2016/04/25 22:47:04]

there was a comment below:
"// Generic classes will be defined inside ..."

can you move it up here? it goes with this line.

[Harry Terkelsen, 2016/04/26 18:29:27]

Done.

+    } else {
+      className = _emitTopLevelName(classElem);
+    }
+
     var allFields = new List.from(fields)..addAll(staticFields);
     var classExpr = _emitClassExpression(
-        classElem, _emitClassMethods(node, ctors, fields),
+        classElem, _emitClassMethods(node, ctors, fields, className, classElem),
         fields: allFields);
 
     var body = <JS.Statement>[];
     var extensions = _extensionsToImplement(classElem);
     _initExtensionSymbols(classElem, methods, fields, body);
 
     // Emit the class, e.g. `core.Object = class Object { ... }`
-    JS.Expression className = _defineClass(classElem, classExpr, body);
+    _defineClass(classElem, className, classExpr, body);
 
     // Emit things that come after the ES6 `class ... { ... }`.
     _setBaseClass(classElem, className, body);
     _defineNamedConstructors(ctors, body, className);
-    _emitVirtualFields(classElem, fields, className, body);
+    _emitVirtualFieldSymbols(classElem, body);
     _emitClassSignature(methods, classElem, ctors, extensions, className, body);
     _defineExtensionMembers(extensions, className, body);
     _emitClassMetadata(node.metadata, className, body);
 
     JS.Statement classDef = _statement(body);
     var typeFormals = classElem.typeParameters;
     if (typeFormals.isNotEmpty) {
       classDef = _defineClassTypeArguments(classElem, typeFormals, classDef);
     }
 
     body = <JS.Statement>[classDef];
     _emitStaticFields(staticFields, classElem, body);
     _registerExtensionType(classElem, body);
     return _statement(body);
   }
 
-  JS.Expression _defineClass(ClassElement classElem,
+  void _defineClass(ClassElement classElem, JS.Expression className,
       JS.ClassExpression classExpr, List<JS.Statement> body) {
-    JS.Expression className;
     if (classElem.typeParameters.isNotEmpty) {
       // Generic classes will be defined inside a function that closes over the
       // type parameter. So we can use their local variable name directly.
-      className = classExpr.name;
       body.add(new JS.ClassDeclaration(classExpr));
     } else {
-      className = _emitTopLevelName(classElem);
       body.add(js.statement('# = #;', [className, classExpr]));
     }
-    return className;
+  }
+
+  void _emitVirtualFieldSymbols(ClassElement elem, List<JS.Statement> body) {
+    if (_virtualFieldSymbols[elem] == null) return;
+    body.addAll(_virtualFieldSymbols[elem]);
   }
 
   List<JS.Identifier> _emitTypeFormals(List<TypeParameterElement> typeFormals) {
     return typeFormals
         .map((t) => new JS.Identifier(t.name))
         .toList(growable: false);
   }
 
   /// Emits a field declaration for TypeScript & Closure's ES6_TYPED
   /// (e.g. `class Foo { i: string; }`)
@@ skipping 156 matching lines @@
               [value], js.statement('{ this.# = #; }', [name, value]));
           method = new JS.Method(_elementMemberName(field.setter), fn,
               isSetter: true);
           jsMethods.add(method);
         }
       }
     }
     return jsMethods;
   }
 
-  List<JS.Method> _emitClassMethods(ClassDeclaration node,
-      List<ConstructorDeclaration> ctors, List<FieldDeclaration> fields) {
+  List<JS.Method> _emitClassMethods(
+      ClassDeclaration node,
+      List<ConstructorDeclaration> ctors,
+      List<FieldDeclaration> fields,
+      JS.Expression className,
+      ClassElement classElem) {

[Jennifer Messerly, 2016/04/25 22:47:04]

This parameter isn't needed.

See the very next line `var element = node.element` :)

Feel free to rename "element" to "classElem" or vice versa in the other method,
if it makes the code more clear.

[Harry Terkelsen, 2016/04/26 18:29:27]

Done.

     var element = node.element;
     var type = element.type;
     var isObject = type.isObject;
 
     // Iff no constructor is specified for a class C, it implicitly has a
     // default constructor `C() : super() {}`, unless C is class Object.
     var jsMethods = <JS.Method>[];
     if (ctors.isEmpty && !isObject) {
       jsMethods.add(_emitImplicitConstructor(node, fields));
     }
 
     bool hasJsPeer = findAnnotation(element, isJsPeerInterface) != null;
     var superclasses = getSuperclasses(element);
 
     bool hasIterator = false;
     for (var m in node.members) {
       if (m is ConstructorDeclaration) {
         jsMethods.add(_emitConstructor(m, type, fields, isObject));
       } else if (m is MethodDeclaration) {
         jsMethods.add(_emitMethodDeclaration(type, m));
 
         if (m.element is PropertyAccessorElement) {
           jsMethods.add(_emitSuperAccessorWrapper(m, type, superclasses));
         }
 
         if (!hasJsPeer && m.isGetter && m.name.name == 'iterator') {
           hasIterator = true;
           jsMethods.add(_emitIterable(type));
         }
-      } else if (m is FieldDeclaration && _extensionTypes.contains(element)) {
-        jsMethods.addAll(_emitNativeFieldAccessors(m));
+      } else if (m is FieldDeclaration) {
+        if (_extensionTypes.contains(element)) {
+          jsMethods.addAll(_emitNativeFieldAccessors(m));
+          continue;
+        }
+        if (m.isStatic) continue;
+        for (VariableDeclaration field in m.fields.variables) {
+          var propertyOverrideResult = checkForPropertyOverride(
+              field.element, superclasses, _extensionTypes);
+          if (propertyOverrideResult.foundGetter ||

[Jennifer Messerly, 2016/04/25 22:47:04]

consider: a shorter name here.

"overrideInfo" or "overrideResult" might read nicely:

if (overrideInfo.foundGetter || overrideInfo.foundSetter) {
 ...
}

[Harry Terkelsen, 2016/04/26 18:29:27]

Done.

+              propertyOverrideResult.foundSetter) {
+            jsMethods.addAll(
+                _emitVirtualFieldAccessor(field, type, className, classElem));

[Jennifer Messerly, 2016/04/25 22:47:04]

fyi -- you probably don't need to pass classElem or type here.

Fields can find their class pretty easily via `.enclosingElement` and from the
class you can get the type via `.type`, for example:

var fieldElem = fieldNode.element;
var classElem = fieldElem.enclosingElement;
var classType = classElem.type;

in general, it's nice to avoid extra parameters when possible, as it makes the
API easier to use and prevents bugs where the wrong thing was passed down.

[Harry Terkelsen, 2016/04/26 18:29:27]

Done.

+          }
+        }
       }
     }
 
     // If the type doesn't have an `iterator`, but claims to implement Iterable,
     // we inject the adaptor method here, as it's less code size to put the
     // helper on a parent class. This pattern is common in the core libraries
     // (e.g. IterableMixin<E> and IterableBase<E>).
     //
     // (We could do this same optimization for any interface with an `iterator`
     // method, but that's more expensive to check for, so it doesn't seem worth
     // it. The above case for an explicit `iterator` method will catch those.)
     if (!hasJsPeer && !hasIterator && _implementsIterable(type)) {
       jsMethods.add(_emitIterable(type));
     }
 
     return jsMethods.where((m) => m != null).toList(growable: false);
   }
 
+  /// This is called whenever a derived class needs to introduce a new field,
+  /// shadowing a field or getter/setter pair on its parent.
+  ///
+  /// This is important because otherwise, trying to read or write the field
+  /// would end up calling the getter or setter, and one of those might not even
+  /// exist, resulting in a runtime error. Even if they did exist, that's the
+  /// wrong behavior if a new field was declared.
+  List<JS.Method> _emitVirtualFieldAccessor(VariableDeclaration field,
+      InterfaceType type, JS.Expression className, ClassElement classElem) {
+    var virtualField = _emitVirtualFieldSymbol(field, className, classElem);
+    var result = <JS.Method>[];
+    var name = _emitMemberName(field.element.name, type: type);
+    var getter = js.call('function() { return this[#]; }', [virtualField]);
+    result.add(new JS.Method(name, getter, isGetter: true));
+
+    if (field.isFinal) {
+      var setter = js.call(
+          'function(value) {'

[Jennifer Messerly, 2016/04/25 22:47:04]

style comment: triple quote style is nicer for these long ones, IMO

var setter = js.call('''
  function(value) {
    ...
  }
''');

that way we can edit without worrying about all of the quote symbols

[Harry Terkelsen, 2016/04/26 18:29:27]

Done.

+          '  var f = this[#];'
+          '  if (f === undefined) {'

[Jennifer Messerly, 2016/04/25 22:47:04]

Hmmmm, I'm a little confused about what's going on here.

Is this for the field initializer (that we generate in the JS `constructor()`?

If so, the best way to handle that is to have it use your virtualField symbol
storage slot directly:

constructor() {
  this[... virtual field symbol ] = ...;
}


Should be a pretty straightforward fix in _initializeFields.


Otherwise I don't think it will work -- we treat "null" and "undefined" as
synonyms, so you could hit a bug:


class B { int get x => _x; set x(int v) => print(v); }

void returnsNull() {} // but really returns JS undefined.
class D extends B { final int x = returnsNull(); }

main() {
  dynamic d = new D();
  d.x = 42; // oops! this sets d.x instead of printing "42" 
}

[Harry Terkelsen, 2016/04/26 18:29:27]

Done.

+          '    this[#] = value;'
+          '  } else {'
+          '    super[#] = value;'
+          '  }'
+          '}',
+          [virtualField, virtualField, name]);
+      result.add(new JS.Method(name, setter, isSetter: true));
+    } else {
+      var setter =
+          js.call('function(value) { this[#] = value; }', [virtualField]);
+      result.add(new JS.Method(name, setter, isSetter: true));
+    }
+
+    return result;
+  }
+
+  /// Emit a getter or setter that simply forwards to the superclass getter or
+  /// setter. This is needed because in ES6, if you only override a getter
+  /// (alternatively, a setter), then there is an implicit override of the
+  /// setter (alternatively, the getter) that does nothing.
   JS.Method _emitSuperAccessorWrapper(MethodDeclaration method,
       InterfaceType type, List<ClassElement> superclasses) {
     var methodElement = method.element as PropertyAccessorElement;
     var field = methodElement.variable;
     if (!field.isSynthetic) return null;
     var propertyOverrideResult = checkForPropertyOverride(
         methodElement.variable, superclasses, _extensionTypes);
 
     // Generate a corresponding virtual getter / setter.
     var name = _elementMemberName(methodElement,
@@ skipping 88 matching lines @@
       newBaseClass = js.call('dart.global.#', [jsPeerName]);
     } else if (_hasDeferredSupertype.contains(classElem)) {
       newBaseClass = _emitTypeName(classElem.type.superclass);
     }
     if (newBaseClass != null) {
       body.add(
           js.statement('dart.setBaseClass(#, #);', [className, newBaseClass]));
     }
   }
 
-  /// Emits instance fields, if they are virtual
-  /// (in other words, they override a getter/setter pair).
-  void _emitVirtualFields(
-      ClassElement classElement,
-      List<FieldDeclaration> fields,
-      JS.Expression className,
-      List<JS.Statement> body) {
-    List<ClassElement> superclasses = getSuperclasses(classElement);
-    for (FieldDeclaration member in fields) {
-      for (VariableDeclaration field in member.fields.variables) {
-        var propertyOverrideResult = checkForPropertyOverride(
-            field.element, superclasses, _extensionTypes);
-        if (propertyOverrideResult.foundGetter ||
-            propertyOverrideResult.foundSetter) {
-          body.add(_overrideField(className, field.element));
-        }
-      }
-    }
-  }
-
   void _defineNamedConstructors(List<ConstructorDeclaration> ctors,
       List<JS.Statement> body, JS.Expression className) {
     for (ConstructorDeclaration member in ctors) {
       if (member.name != null && member.factoryKeyword == null) {
         body.add(js.statement('dart.defineNamedConstructor(#, #);',
             [className, _emitMemberName(member.name.name, isStatic: true)]));
       }
     }
   }
 
@@ skipping 186 matching lines @@
     if (_extensionTypes.contains(element)) types.add(element);
     for (var m in type.mixins.reversed) {
       _collectExtensions(m, types);
     }
     for (var i in type.interfaces) {
       _collectExtensions(i, types);
     }
     _collectExtensions(type.superclass, types);
   }
 
-  JS.Statement _overrideField(JS.Expression className, FieldElement e) {
-    var cls = e.enclosingElement;
-    return js.statement('dart.virtualField(#, #)',
-        [className, _emitMemberName(e.name, type: cls.type)]);
-  }
-
   /// Generates the implicit default constructor for class C of the form
   /// `C() : super() {}`.
   JS.Method _emitImplicitConstructor(
       ClassDeclaration node, List<FieldDeclaration> fields) {
     assert(_hasUnnamedConstructor(node.element) == fields.isNotEmpty);
 
     // If we don't have a method body, skip this.
     var superCall = _superConstructorCall(node.element);
     if (fields.isEmpty && superCall == null) return null;
 
@@ skipping 2510 matching lines @@
     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;
     });
   }
 
+  JS.TemporaryId _emitVirtualFieldSymbol(VariableDeclaration field,
+      JS.Expression className, ClassElement classElem) {
+    return _virtualFields.putIfAbsent(field, () {
+      var fieldName = _emitMemberName(field.element.name,
+          type: (field.element.enclosingElement as ClassElement).type);
+      var id = new JS.TemporaryId(field.name.name);
+      _virtualFieldSymbols.putIfAbsent(classElem, () => <JS.Statement>[]).add(js
+          .statement('const # = Symbol(#.name + "." + #.toString());',
+              [id, className, fieldName]));
+      return id;
+    });
+  }
+
   bool _externalOrNative(node) =>
       node.externalKeyword != null || _functionBody(node) is NativeFunctionBody;
 
   FunctionBody _functionBody(node) =>
       node is FunctionDeclaration ? node.functionExpression.body : node.body;
 
   /// Returns the canonical name to refer to the Dart library.
   JS.Identifier emitLibraryName(LibraryElement library) {
     // It's either one of the libraries in this module, or it's an import.
     return _libraries[library] ??
@@ skipping 102 matching lines @@
 }
 
 bool isLibraryPrefix(Expression node) =>
     node is SimpleIdentifier && node.staticElement is PrefixElement;
 
 LibraryElement _getLibrary(AnalysisContext c, String uri) =>
     c.computeLibraryElement(c.sourceFactory.forUri(uri));
 
 bool _isDartRuntime(LibraryElement l) =>
     l.isInSdk && l.source.uri.toString() == 'dart:_runtime';