fixes #52, fields shadowing getters/setters or other fields

lib/src/codegen/js_codegen.dart

Index: lib/src/codegen/js_codegen.dart
diff --git a/lib/src/codegen/js_codegen.dart b/lib/src/codegen/js_codegen.dart
index 39b05cb6877099d2bbd4b4a575399ebb72cff31c..1fcd3a4afb3214ed3e11863aa3845578b97ba1a2 100644
--- a/lib/src/codegen/js_codegen.dart
+++ b/lib/src/codegen/js_codegen.dart
@@ -27,6 +27,7 @@ import 'package:dev_compiler/src/options.dart';
 import 'package:dev_compiler/src/utils.dart';
 
 import 'code_generator.dart';
+import 'js_field_storage.dart';
 import 'js_names.dart' show JSTemporary, invalidJSStaticMethodName;
 import 'js_metalet.dart';
 import 'js_printer.dart' show writeJsLibrary;
@@ -51,8 +52,13 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
   /// The global extension method table.
   final HashMap<String, List<InterfaceType>> _extensionMethods;
 
+  /// Information that is precomputed for this library, indicates which fields
+  /// need storage slots.
+  final HashSet<FieldElement> _fieldsNeedingStorage;
+
   /// The variable for the target of the current `..` cascade expression.
   SimpleIdentifier _cascadeTarget;
+
   /// The variable for the current catch clause
   SimpleIdentifier _catchParameter;
 
@@ -63,9 +69,8 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
   final _lazyFields = <VariableDeclaration>[];
   final _properties = <FunctionDeclaration>[];
   final _privateNames = new HashMap<String, JSTemporary>();
-  final _pendingPrivateNames = <JSTemporary>[];
   final _extensionMethodNames = new HashSet<String>();
-  final _pendingExtensionMethodNames = <String>[];
+  final _pendingSymbols = <JS.Identifier>[];
   final _temps = new HashMap<Element, JSTemporary>();
 
   /// The name for the library's exports inside itself.
@@ -85,7 +90,8 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
   /// Memoized results of [_inLibraryCycle].
   final _libraryCycleMemo = new HashMap<LibraryElement, bool>();
 
-  JSCodegenVisitor(this.libraryInfo, this.rules, this._extensionMethods);
+  JSCodegenVisitor(this.libraryInfo, this.rules, this._extensionMethods,
+      this._fieldsNeedingStorage);
 
   LibraryElement get currentLibrary => libraryInfo.library;
   TypeProvider get types => rules.provider;
@@ -119,6 +125,9 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
 
     for (var unit in library.partsThenLibrary) body.add(_visit(unit));
 
+    // Ensure field slots for any fields we override from other libraries.
+    _fieldsNeedingStorage.forEach(_overrideField);
+
     assert(_pendingClasses.isEmpty);
 
     if (_exports.isNotEmpty) body.add(js.comment('Exports:'));
@@ -143,11 +152,8 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
     ]);
   }
 
-  JS.Statement _initPrivateSymbol(JSTemporary tmp) =>
-      js.statement('let # = $_SYMBOL(#);', [tmp, js.string(tmp.name, "'")]);
-
-  JS.Statement _initExtensionMethodSymbol(String name) => js.statement(
-      'let # = $_SYMBOL(#);', [new JS.Identifier(name), js.string(name, "'")]);
+  JS.Statement _initSymbol(JS.Identifier id) =>
+      js.statement('let # = $_SYMBOL(#);', [id, js.string(id.name, "'")]);
 
   // TODO(jmesserly): this is a temporary workaround for `Symbol` in core,
   // until we have better name tracking.
@@ -171,16 +177,10 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
       if (child is! FunctionDeclaration) _flushLibraryProperties(body);
 
       var code = _visit(child);
-
       if (code != null) {
-        if (_pendingPrivateNames.isNotEmpty) {
-          body.addAll(_pendingPrivateNames.map(_initPrivateSymbol));
-          _pendingPrivateNames.clear();
-        }
-        if (_pendingExtensionMethodNames.isNotEmpty) {
-          body.addAll(
-              _pendingExtensionMethodNames.map(_initExtensionMethodSymbol));
-          _pendingExtensionMethodNames.clear();
+        if (_pendingSymbols.isNotEmpty) {
+          body.addAll(_pendingSymbols.map(_initSymbol));
+          _pendingSymbols.clear();
         }
         body.add(code);
       }
@@ -190,7 +190,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
     _flushLazyFields(body);
     _flushLibraryProperties(body);
 
-    assert(_pendingPrivateNames.isEmpty);
+    assert(_pendingSymbols.isEmpty);
     return _statement(body);
   }
 
@@ -317,8 +317,9 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
   @override
   JS.Statement visitClassDeclaration(ClassDeclaration node) {
     // If we've already emitted this class, skip it.
-    var type = node.element.type;
-    if (_pendingClasses.remove(node.element) == null) return null;
+    var classElem = node.element;
+    var type = classElem.type;
+    if (_pendingClasses.remove(classElem) == null) return null;
 
     var jsName = getAnnotationValue(node, _isJsNameAnnotation);
     if (jsName != null) return _emitJsType(node.name.name, jsName);
@@ -340,7 +341,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
         _classHeritage(node), _emitClassMethods(node, ctors, fields));
 
     var body =
-        _finishClassMembers(node.element, classExpr, ctors, staticFields);
+        _finishClassMembers(classElem, classExpr, ctors, fields, staticFields);
     currentClass = null;
 
     return _finishClassDef(type, body);
@@ -551,12 +552,13 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
     var isObject = type.isObject;
     var name = node.name.name;
 
-    var jsMethods = <JS.Method>[];
     // 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, name, fields));
     }
+
     for (var member in node.members) {
       if (member is ConstructorDeclaration) {
         jsMethods.add(_emitConstructor(member, name, fields, isObject));
@@ -585,12 +587,11 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
 
   /// Emit class members that need to come after the class declaration, such
   /// as static fields. See [_emitClassMethods] for things that are emitted
-  /// insite the ES6 `class { ... }` node.
+  /// inside the ES6 `class { ... }` node.
   JS.Statement _finishClassMembers(ClassElement classElem,
       JS.ClassExpression cls, List<ConstructorDeclaration> ctors,
-      List<FieldDeclaration> staticFields) {
+      List<FieldDeclaration> fields, List<FieldDeclaration> staticFields) {
     var name = classElem.name;
-
     var body = <JS.Statement>[];
     body.add(new JS.ClassDeclaration(cls));
 
@@ -613,6 +614,16 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
       }
     }
 
+    // Instance fields, if they override getter/setter pairs
+    for (FieldDeclaration member in fields) {
+      for (VariableDeclaration fieldDecl in member.fields.variables) {
+        var field = fieldDecl.element;
+        if (_fieldsNeedingStorage.remove(field)) {
+          body.add(_overrideField(field));
+        }
+      }
+    }
+
     // Static fields
     var lazyStatics = <VariableDeclaration>[];
     for (FieldDeclaration member in staticFields) {
@@ -633,6 +644,14 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
     return _statement(body);
   }
 
+  JS.Statement _overrideField(FieldElement e) {
+    var cls = e.enclosingElement;
+    return js.statement('dart.virtualField(#, #)', [
+      cls.name,
+      _emitMemberName(e.name, type: cls.type)
+    ]);
+  }
+
   /// Generates the implicit default constructor for class C of the form
   /// `C() : super() {}`.
   JS.Method _emitImplicitConstructor(
@@ -640,7 +659,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
     // If we don't have a method body, skip this.
     if (fields.isEmpty) return null;
 
-    dynamic body = _initializeFields(fields);
+    dynamic body = _initializeFields(node, fields);
     var superCall = _superConstructorCall(node);
     if (superCall != null) body = [[body, superCall]];
     return new JS.Method(
@@ -730,7 +749,8 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
       // These are expanded into each non-redirecting constructor.
       // In the future we may want to create an initializer function if we have
       // multiple constructors, but it needs to be balanced against readability.
-      body.add(_initializeFields(fields, node.parameters, node.initializers));
+      body.add(_initializeFields(
+          node.parent, fields, node.parameters, node.initializers));
 
       var superCall = node.initializers.firstWhere(
           (i) => i is SuperConstructorInvocation, orElse: () => null);
@@ -779,20 +799,21 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
   ///   2. field initializing parameters,
   ///   3. constructor field initializers,
   ///   4. initialize fields not covered in 1-3
-  JS.Statement _initializeFields(List<FieldDeclaration> fields,
+  JS.Statement _initializeFields(
+      ClassDeclaration classDecl, List<FieldDeclaration> fieldDecls,
       [FormalParameterList parameters,
       NodeList<ConstructorInitializer> initializers]) {
-    var body = <JS.Statement>[];
 
     // Run field initializers if they can have side-effects.
+    var fields = new Map<FieldElement, JS.Expression>();
     var unsetFields = new Map<FieldElement, VariableDeclaration>();
-    for (var declaration in fields) {
-      for (var field in declaration.fields.variables) {
-        if (_isFieldInitConstant(field)) {
-          unsetFields[field.element] = field;
+    for (var declaration in fieldDecls) {
+      for (var fieldNode in declaration.fields.variables) {
+        var element = fieldNode.element;
+        if (_isFieldInitConstant(fieldNode)) {
+          unsetFields[element] = fieldNode;
         } else {
-          body.add(js.statement(
-              '# = #;', [_visit(field.name), _visitInitializer(field)]));
+          fields[element] = _visitInitializer(fieldNode);
         }
       }
     }
@@ -800,16 +821,9 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
     // Initialize fields from `this.fieldName` parameters.
     if (parameters != null) {
       for (var p in parameters.parameters) {
-        if (p is DefaultFormalParameter) p = p.parameter;
-        if (p is FieldFormalParameter) {
-          var field = (p.element as FieldFormalParameterElement).field;
-          // Use the getter to initialize the field. This is a bit strange, but
-          // final fields don't have a setter element that we could use instead.
-
-          var memberName =
-              _emitMemberName(field.name, type: field.enclosingElement.type);
-          body.add(js.statement('this.# = #;', [memberName, _visit(p)]));
-          unsetFields.remove(field);
+        var element = p.element;
+        if (element is FieldFormalParameterElement) {
+          fields[element.field] = _visit(p);
         }
       }
     }
@@ -818,30 +832,56 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
     if (initializers != null) {
       for (var init in initializers) {
         if (init is ConstructorFieldInitializer) {
-          body.add(js.statement(
-              '# = #;', [_visit(init.fieldName), _visit(init.expression)]));
-          unsetFields.remove(init.fieldName.staticElement);
+          fields[init.fieldName.staticElement] = _visit(init.expression);
         }
       }
     }
 
+    for (var f in fields.keys) unsetFields.remove(f);
+
     // Initialize all remaining fields
-    unsetFields.forEach((field, fieldNode) {
+    unsetFields.forEach((element, fieldNode) {
       JS.Expression value;
       if (fieldNode.initializer != null) {
         value = _visit(fieldNode.initializer);
       } else {
-        var type = rules.elementType(field);
+        var type = rules.elementType(element);
         value = new JS.LiteralNull();
         if (rules.maybeNonNullableType(type)) {
           value = js.call('dart.as(#, #)', [value, _emitTypeName(type)]);
         }
       }
-      var memberName =
-          _emitMemberName(field.name, type: field.enclosingElement.type);
-      body.add(js.statement('this.# = #;', [memberName, value]));
+      fields[element] = value;
     });
 
+    var classElem = classDecl.element;
+    bool classCanBeMixin = !classElem.type.isObject &&
+        classElem.supertype.isObject &&
+        classElem.mixins.isEmpty;
+
+    var body = <JS.Statement>[];
+    fields.forEach((FieldElement e, JS.Expression initialValue) {
+      var access = _emitMemberName(e.name, type: e.enclosingElement.type);
+
+      // Fields on private types can't be extended outside this library.
+      // Private fields on public types also can't be extended, as long as the
+      // type can't be mixed in.
+      bool isSealed = classElem.isPrivate || e.isPrivate && !classCanBeMixin;
+
+      JS.Statement init;
+      if (isSealed && !_fieldsNeedingStorage.contains(e)) {
+        // Concrete field: doesn't override anything else, and is sealed so it
+        // isn't and can't be overridden by anything else.
+        init = js.statement('this.# = #;', [access, initialValue]);
+      } else {
+        init = js.statement('dart.initField(#, this, #, #);', [
+          classDecl.name.name,
+          access,
+          initialValue
+        ]);
+      }
+      body.add(init);
+    });
     return _statement(body);
   }
 
@@ -2206,10 +2246,11 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
     if (name.startsWith('_')) {
       return _privateNames.putIfAbsent(name, () {
         var t = new JSTemporary(name);
-        _pendingPrivateNames.add(t);
+        _pendingSymbols.add(t);
         return t;
       });
     }
+
     // Check for extension method:
     var extLibrary = _findExtensionLibrary(name, type);
 
@@ -2231,10 +2272,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
     }
 
     if (extLibrary != null) {
-      return js.call('#.#', [
-        _libraryName(extLibrary),
-        _propertyName(_addExtensionMethodName(name, extLibrary))
-      ]);
+      return _extensionMethodName(name, extLibrary);
     }
 
     return _propertyName(name);
@@ -2258,18 +2296,18 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
     return extLibrary;
   }
 
-  String _addExtensionMethodName(String name, LibraryElement extLibrary) {
-    var extensionMethodName = '\$$name';
-    if (extLibrary == currentLibrary) {
+  JS.Expression _extensionMethodName(String name, LibraryElement library) {
+    var extName = '\$$name';
+    if (library == currentLibrary) {
       // TODO(jacobr): need to do a better job ensuring that extension method
       // name symbols do not conflict with other symbols before we can let
       // user defined libraries define extension methods.
-      if (_extensionMethodNames.add(extensionMethodName)) {
-        _pendingExtensionMethodNames.add(extensionMethodName);
-        _addExport(extensionMethodName);
+      if (_extensionMethodNames.add(extName)) {
+        _pendingSymbols.add(new JS.Identifier(extName));
+        _addExport(extName);
       }
     }
-    return extensionMethodName;
+    return js.call('#.#', [_libraryName(library), _propertyName(extName)]);
   }
 
   bool _externalOrNative(node) =>
@@ -2323,7 +2361,8 @@ class JSGenerator extends CodeGenerator {
   TypeProvider get types => rules.provider;
 
   String generateLibrary(LibraryUnit unit, LibraryInfo info) {
-    var codegen = new JSCodegenVisitor(info, rules, _extensionMethods);
+    var fields = findFieldOverrides(unit);
+    var codegen = new JSCodegenVisitor(info, rules, _extensionMethods, fields);
     var module = codegen.emitLibrary(unit);
     var dir = path.join(outDir, jsOutputPath(info, root));
     return writeJsLibrary(module, dir, emitSourceMaps: options.emitSourceMaps);