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

lib/src/compiler/code_generator.dart

Index: lib/src/compiler/code_generator.dart
diff --git a/lib/src/compiler/code_generator.dart b/lib/src/compiler/code_generator.dart
index 2c19b39ef6f2c7ac0a9af6e10978145d953ec503..4aab66c4c08c5621a08003d4c404ebeec3689d2e 100644
--- a/lib/src/compiler/code_generator.dart
+++ b/lib/src/compiler/code_generator.dart
@@ -81,6 +81,8 @@ class CodeGenerator extends GeneralizingAstVisitor
 
   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>();
 
@@ -531,9 +533,16 @@ class CodeGenerator extends GeneralizingAstVisitor
       }
     }
 
+    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>[];
@@ -541,12 +550,12 @@ class CodeGenerator extends GeneralizingAstVisitor
     _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);
@@ -563,19 +572,20 @@ class CodeGenerator extends GeneralizingAstVisitor
     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) {
@@ -752,8 +762,12 @@ class CodeGenerator extends GeneralizingAstVisitor
     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;
@@ -783,8 +797,21 @@ class CodeGenerator extends GeneralizingAstVisitor
           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.

+          }
+        }
       }
     }
 
@@ -803,6 +830,46 @@ class CodeGenerator extends GeneralizingAstVisitor
     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;
@@ -911,26 +978,6 @@ class CodeGenerator extends GeneralizingAstVisitor
     }
   }
 
-  /// 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) {
@@ -1137,12 +1184,6 @@ class CodeGenerator extends GeneralizingAstVisitor
     _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(
@@ -3673,6 +3714,19 @@ class CodeGenerator extends GeneralizingAstVisitor
     });
   }
 
+  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;