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

lib/src/codegen/js_codegen.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.
 
 library dev_compiler.src.codegen.js_codegen;
 
 import 'dart:collection' show HashSet, HashMap;
 
 import 'package:analyzer/analyzer.dart' hide ConstantEvaluator;
 import 'package:analyzer/src/generated/ast.dart' hide ConstantEvaluator;
 import 'package:analyzer/src/generated/constant.dart';
 import 'package:analyzer/src/generated/element.dart';
 import 'package:analyzer/src/generated/resolver.dart' show TypeProvider;
 import 'package:analyzer/src/generated/scanner.dart'
     show StringToken, Token, TokenType;
 import 'package:path/path.dart' as path;
 
 import 'package:dev_compiler/src/codegen/ast_builder.dart' show AstBuilder;
 
 // TODO(jmesserly): import from its own package
 import 'package:dev_compiler/src/js/js_ast.dart' as JS;
 import 'package:dev_compiler/src/js/js_ast.dart' show js;
 
 import 'package:dev_compiler/src/checker/rules.dart';
 import 'package:dev_compiler/src/info.dart';
 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;
 import 'side_effect_analysis.dart';
 
 // Various dynamic helpers we call.
 // If renaming these, make sure to check other places like the
 // dart_runtime.js file and comments.
 // TODO(jmesserly): ideally we'd have a "dynamic call" dart library we can
 // import and generate calls to, rather than dart_runtime.js
 const DPUT = 'dput';
 const DLOAD = 'dload';
 const DINDEX = 'dindex';
 const DSETINDEX = 'dsetindex';
 const DCALL = 'dcall';
 const DSEND = 'dsend';
 
 class JSCodegenVisitor extends GeneralizingAstVisitor with ConversionVisitor {
   final LibraryInfo libraryInfo;
   final TypeRules rules;
 
   /// 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;
 
   ClassDeclaration currentClass;
   ConstantEvaluator _constEvaluator;
 
   final _exports = new Set<String>();
   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.
   /// This much be a constant because we interpolate it into template strings,
   /// and otherwise it would break caching for them.
   /// `exports` was chosen as the most similar to ES module patterns.
   final JSTemporary _exportsVar = new JSTemporary('exports');
   final JSTemporary _namedArgTemp = new JSTemporary('opts');
 
   /// Classes we have not emitted yet. Values can be [ClassDeclaration] or
   /// [ClassTypeAlias].
   final _pendingClasses = new HashMap<Element, CompilationUnitMember>();
 
   /// Memoized results of [_lazyClass].
   final _lazyClassMemo = new HashMap<Element, bool>();
 
   /// 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;
 
   JS.Program emitLibrary(LibraryUnit library) {
     String jsDefaultValue = null;
     var unit = library.library;
     if (unit.directives.isNotEmpty) {
       var libraryDir = unit.directives.first;
       if (libraryDir is LibraryDirective) {
@@ skipping 37 matching lines @@
       js.statement("(function(#) { 'use strict'; #; })(# || (# = #));", [
         _exportsVar,
         body,
         name,
         name,
         defaultValue
       ])
     ]);
   }
 
-  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.
   String get _SYMBOL {
     var name = currentLibrary.name;
     if (name == 'dart.core' || name == 'dart._internal') return 'dart.JsSymbol';
     return 'Symbol';
   }
 
   @override
   JS.Statement visitCompilationUnit(CompilationUnit node) {
     var source = node.element.source;
 
     _constEvaluator = new ConstantEvaluator(source, types);
 
     // TODO(jmesserly): scriptTag, directives.
     var body = <JS.Statement>[];
     for (var child in node.declarations) {
       // Attempt to group adjacent fields/properties.
       if (child is! TopLevelVariableDeclaration) _flushLazyFields(body);
       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);
       }
     }
 
     // Flush any unwritten fields/properties.
     _flushLazyFields(body);
     _flushLibraryProperties(body);
 
-    assert(_pendingPrivateNames.isEmpty);
+    assert(_pendingSymbols.isEmpty);
     return _statement(body);
   }
 
   bool isPublic(String name) => !name.startsWith('_');
 
   /// Conversions that we don't handle end up here.
   @override
   visitConversion(Conversion node) {
     var from = node.baseType;
     var to = node.convertedType;
@@ skipping 128 matching lines @@
       if (member is ConstructorDeclaration) {
         ctors.add(member);
       } else if (member is FieldDeclaration) {
         (member.isStatic ? staticFields : fields).add(member);
       }
     }
 
     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(
+        node.element, classExpr, ctors, fields, staticFields);
     currentClass = null;
 
     return _finishClassDef(type, body);
   }
 
   @override
   JS.Statement visitEnumDeclaration(EnumDeclaration node) =>
       _unimplementedCall("Unimplemented enum: $node").toStatement();
 
   /// Given a class element and body, complete the class declaration.
@@ skipping 190 matching lines @@
     return heritage;
   }
 
   List<JS.Method> _emitClassMethods(ClassDeclaration node,
       List<ConstructorDeclaration> ctors, List<FieldDeclaration> fields) {
     var element = node.element;
     var type = element.type;
     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);
       }
     }
 
     // Support for adapting dart:core Iterator/Iterable to ES6 versions.
     // This lets them use for-of loops transparently.
     // https://github.com/lukehoban/es6features#iterators--forof
     if (element.library.isDartCore && element.name == 'Iterable') {
       JS.Fun body = js.call('''function() {
         var iterator = this.iterator;
         return {
           next() {
             var done = iterator.moveNext();
             return { done: done, current: done ? void 0 : iterator.current };
           }
         };
       }''');
       jsMethods.add(new JS.Method(js.call('$_SYMBOL.iterator'), body));
     }
     return jsMethods.where((m) => m != null).toList(growable: false);
   }
 
   /// 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));
 
     // Interfaces
     if (classElem.interfaces.isNotEmpty) {
       body.add(js.statement('#[dart.implements] = () => #;', [
         name,
         new JS.ArrayInitializer(
             classElem.interfaces.map(_emitTypeName).toList())
       ]));
@@ skipping 162 matching lines @@
       return null;
     }
 
     var supertypeName = element.supertype.name;
     var name = _constructorName(supertypeName, superCtorName);
 
     var args = node != null ? _visit(node.argumentList) : [];
     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;
+    return _temps.putIfAbsent(e, () {
+      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);
         }
       }
     }
 
     // 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);
         }
       }
     }
 
     // Run constructor field initializers such as `: foo = bar.baz`
     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);
   }
 
   FormalParameterList _parametersOf(node) {
     // Note: ConstructorDeclaration is intentionally skipped here so we can
     // emit the argument initializers in a different place.
     // TODO(jmesserly): clean this up. If we can model ES6 spread/rest args, we
     // could handle argument initializers more consistently in a separate
     // lowering pass.
     if (node is MethodDeclaration) return node.parameters;
@@ skipping 1344 matching lines @@
   /// for this transformation to happen, otherwise binary minus is assumed.
   ///
   /// Equality is a bit special, it is generated via the Dart `equals` runtime
   /// helper, that checks for null. The user defined method is called '=='.
   ///
   JS.Expression _emitMemberName(String name,
       {DartType type, bool unary: false, bool isStatic: false}) {
     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);
 
     if (name == '[]') {
       name = 'get';
     } else if (name == '[]=') {
       name = 'set';
     } else if (name == '-' && unary) {
       name = 'unary-';
     }
 
     if (isStatic && invalidJSStaticMethodName(name)) {
       // Choose an string name. Use an invalid identifier so it won't conflict
       // with any valid member names.
       // TODO(jmesserly): this works around the problem, but I'm pretty sure we
       // don't need it, as static methods seemed to work. The only concrete
       // issue we saw was in the defineNamedConstructor helper function.
       name = '$name*';
     }
 
     if (extLibrary != null) {
-      return js.call('#.#', [
-        _libraryName(extLibrary),
-        _propertyName(_addExtensionMethodName(name, extLibrary))
-      ]);
+      return _extensionMethodName(name, extLibrary);
     }
 
     return _propertyName(name);
   }
 
   LibraryElement _findExtensionLibrary(String name, DartType type) {
     if (type is! InterfaceType) return null;
 
     var extLibrary = null;
     var extensionTypes = _extensionMethods[name];
     if (extensionTypes != null) {
       // Normalize the type to ignore generics.
       type = fillDynamicTypeArgs(type, types);
       for (var t in extensionTypes) {
         if (rules.isSubTypeOf(type, t)) {
           assert(extLibrary == null || extLibrary == t.element.library);
           extLibrary = t.element.library;
         }
       }
     }
     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) =>
       node.externalKeyword != null || _functionBody(node) is NativeFunctionBody;
 
   FunctionBody _functionBody(node) =>
       node is FunctionDeclaration ? node.functionExpression.body : node.body;
 
   /// Choose a canonical name from the library element.
   /// This never uses the library's name (the identifier in the `library`
@@ skipping 33 matching lines @@
         ..addAll(e.accessors.map((m) => m.name));
       for (var name in names) {
         _extensionMethods.putIfAbsent(name, () => []).add(type);
       }
     }
   }
 
   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);
   }
 }
 
 /// Choose a canonical name from the library element.
 /// This never uses the library's name (the identifier in the `library`
 /// declaration) as it doesn't have any meaningful rules enforced.
 String jsLibraryName(LibraryElement library) => canonicalLibraryName(library);
@@ skipping 22 matching lines @@
   return filepath;
 }
 
 // TODO(jmesserly): validate the library. See issue #135.
 bool _isJsNameAnnotation(DartObjectImpl value) => value.type.name == 'JsName';
 
 // TODO(jacobr): we would like to do something like the following
 // but we don't have summary support yet.
 // bool _supportJsExtensionMethod(AnnotatedNode node) =>
 //    _getAnnotation(node, "SupportJsExtensionMethod") != null;