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

lib/src/codegen/js_field_storage.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_field_storage;
+
+import 'dart:collection' show HashMap, HashSet;
+import 'package:analyzer/src/generated/ast.dart' show Identifier;
+import 'package:analyzer/src/generated/element.dart';
+
+import 'package:dev_compiler/src/info.dart' show LibraryUnit;
+
+/// We use a storage slot for fields that override or can be overridden by
+/// getter/setter pairs.
+HashSet<FieldElement> findFieldsNeedingStorage(LibraryUnit library) {
+  var propertyOverrides = new HashSet<PropertyInducingElement>();
+  for (var unit in library.partsThenLibrary) {
+    for (var cls in unit.element.types) {
+      var visited = new HashSet<PropertyInducingElement>();
+      for (var accessor in cls.accessors) {
+        var prop = accessor.variable;
+        if (visited.add(prop) && !propertyOverrides.contains(prop)) {
+          checkForPropertyOverride(prop, propertyOverrides);
+        }
+      }
+    }
+  }
+
+  // Return only the field overrides.
+  return new HashSet<FieldElement>.from(
+      propertyOverrides.where((a) => a is FieldElement));
+}
+
+void checkForPropertyOverride(PropertyInducingElement prop,
+    HashSet<PropertyInducingElement> propertyOverrides) {
+
+  // Public fields on public types always need storage, because they could be
+  // extended later in another compilation unit.
+  var library = prop.library;
+  var cls = prop.enclosingElement;
+  if (prop.isPublic && cls.isPublic) {
+    propertyOverrides.add(prop);
+    return;
+  }
+
+  bool found = false;
+  for (var superclass in superclasses(cls)) {
+    var superprop = getProperty(superclass, library, prop.name);
+    if (superprop != null) {
+      found = true;
+      // Record that the super property is overridden.
+      if (superprop.library == library) propertyOverrides.add(superprop);
+    }
+  }
+
+  // If this we found a super property, then this property overrides it.
+  if (found) propertyOverrides.add(prop);
+}
+
+PropertyInducingElement getProperty(
+    ClassElement cls, LibraryElement fromLibrary, String name) {
+  // Properties from a different library are not accessible.
+  if (Identifier.isPrivateName(name) && cls.library != fromLibrary) {
+    return null;
+  }
+  for (var accessor in cls.accessors) {
+    var prop = accessor.variable;
+    if (prop.name == name) return prop;
+  }
+  return null;
+}
+
+Iterable<ClassElement> superclasses(ClassElement cls) sync* {

[Jacob, 2015/04/17 20:28:53]

bold move... using sync* :)
maybe chat with sra  if this code is performance critical.
Seems like we might as well just compute the list of ClassElements rather than
being clever.

[Jennifer Messerly, 2015/04/20 17:44:34]

Good point. Forgot about performance. Will change it.

That said, the algorithm in this file is probably slow enough already that sync*
is unlikely to be the main cause of slowness ;-)

Actually, it looks like we can precompute the supertypes and pass in to
checkForPropertyOverride, so there's a slight algorithm improvement.
nit: Iterables aren't "being clever". They've been used in other languages for
years and they aren't thought of as clever. We even have for-in loops in Dart
and Iterable as one of the main types in dart:core. sync* just makes them
cleaner to implement, and we already use it in DDC too.

I get that sync* currently has some perf issues compared to hand coded
iterables, but that shouldn't really be a long term issue (and wasn't in C# that
I recall, it has a very similar feature to sync*). Happy to avoid it now in
perf-critical code though (which this file might be).

[Brian Wilkerson, 2015/04/20 17:59:19]

DBC: This looks like information that other clients might also want to be able
to get from the element model. It would be a shame if they were forced to
re-implement it. I'd encourage you to consider making this a getter on
ClassElement rather than a utility method that takes a ClassElement as a
parameter.

[Jennifer Messerly, 2015/04/20 18:30:39]

Good point. What's the best workflow there? I'm guessing we still need a utility
method until we can release a new package:analyzer and roll forward our dep?

+  var visited = new HashSet<ClassElement>();
+  while (cls != null && visited.add(cls)) {
+    for (var mixinType in cls.mixins.reversed) {
+      var mixin = mixinType.element;
+      if (mixin != null) yield mixin;
+    }
+    var supertype = cls.supertype;
+    if (supertype == null) return;
+
+    cls = supertype.element;
+    yield cls;
+  }
+}