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..89c5ce707afa8fc829ddc391b9b191620ee4611e 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' show findFieldsNeedingStorage;
 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. See [_fieldStorage] for more explanation.
+  final HashSet<FieldElement> _fieldNeedsStorage;
+
   /// 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._fieldNeedsStorage);
 
   LibraryElement get currentLibrary => libraryInfo.library;
   TypeProvider get types => rules.provider;
@@ -143,11 +149,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 +174,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 +187,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
     _flushLazyFields(body);
     _flushLibraryProperties(body);
 
-    assert(_pendingPrivateNames.isEmpty);
+    assert(_pendingSymbols.isEmpty);
     return _statement(body);
   }
 
@@ -317,8 +314,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);
@@ -339,8 +337,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
     var classExpr = new JS.ClassExpression(new JS.Identifier(type.name),
         _classHeritage(node), _emitClassMethods(node, ctors, fields));
 
-    var body =
-        _finishClassMembers(node.element, classExpr, ctors, staticFields);
+    var body = _finishClassMembers(classElem, classExpr, ctors, staticFields);
     currentClass = null;
 
     return _finishClassDef(type, body);
@@ -551,17 +548,20 @@ 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));
       } else if (member is MethodDeclaration) {
         jsMethods.add(_emitMethodDeclaration(type, member));
+      } else if (member is FieldDeclaration) {
+        _emitFieldDeclaration(jsMethods, member);
       }
     }
 
@@ -585,12 +585,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) {
     var name = classElem.name;
-
     var body = <JS.Statement>[];
     body.add(new JS.ClassDeclaration(cls));
 
@@ -773,26 +772,70 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
     return js.statement('super.#(#);', [name, args])..sourceInformation = node;
   }
 
+  /// Emits the [field]'s induced getter/setter into [methods].
+  void _emitFieldDeclaration(List<JS.Method> methods, FieldDeclaration fields) {
+    if (fields.isStatic) return;
+
+    for (var field in fields.fields.variables) {
+      // For fields that use storage slots, we need to emit a getter/setter
+      // pair, otherwise we don't.
+      var storage = _fieldStorage(field.element);
+      if (storage == null) continue;
+
+      var e = field.element;
+      var memberName = _emitMemberName(e.name, type: e.enclosingElement.type);
+      methods.add(new JS.Method(
+          memberName, js.call('function() { return this[#]; }', storage),
+          isGetter: true));
+      if (!field.isFinal) {
+        methods.add(new JS.Method(memberName,
+            js.call('function(value) { this[#] = value; }', storage),
+            isSetter: true));
+      }
+    }
+  }
+
+  /// We use a storage slot for fields that override or can be overridden by
+  /// getter/setter pairs. If the field element [e] needs storage, this will
+  /// return a temporary that contains the JS `Symbol` with storage. Otherwise
+  /// it will return null to indicate that no storage is needed.
+  JSTemporary _fieldStorage(FieldElement e) {
+    if (!_fieldNeedsStorage.contains(e)) return null;
+
+    // TODO(jmesserly): if we know two types can't be (or aren't, in the case of
+    // private classes) mixed together, we could reuse field storage. This would
+    // result in less `name$` `name$0` temps being generated. We'd likely need
+    // to push this into the [findFieldsNeedingStorage] analysis method.
+    return _temps.putIfAbsent(e, () {
+      // TODO(jmesserly): maybe we could have two candidate names, and one of
+      // them is ClassName_fieldName? That might be more readable in case of
+      // conflict.
+      var t = new JSTemporary(e.name);
+      _pendingSymbols.add(t);
+      return t;
+    });
+  }
+
   /// Initialize fields. They follow the sequence:
   ///
   ///   1. field declaration initializer if non-const,
   ///   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(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 +843,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 +854,36 @@ 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 body = <JS.Statement>[];
+    fields.forEach((FieldElement e, JS.Expression initialValue) {
+      var access = _fieldStorage(e);
+      if (access == null) {
+        access = _emitMemberName(e.name, type: e.enclosingElement.type);
+      }
+      body.add(js.statement('this.# = #;', [access, initialValue]));
+    });
     return _statement(body);
   }
 
@@ -2206,10 +2248,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 +2274,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 +2298,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 +2363,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 = findFieldsNeedingStorage(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);