Emit forwarding getter/setter when overriding just a getter or setter.

lib/src/compiler/js_field_storage.dart

Index: lib/src/compiler/js_field_storage.dart
diff --git a/lib/src/compiler/js_field_storage.dart b/lib/src/compiler/js_field_storage.dart
index 1a70229d68fd707575d61d240f87916191f5791f..e18cba693897bb9a49c2e663171d43f615315acc 100644
--- a/lib/src/compiler/js_field_storage.dart
+++ b/lib/src/compiler/js_field_storage.dart
@@ -9,9 +9,11 @@ import 'extension_types.dart';
 
 /// We use a storage slot for fields that override or can be overridden by

[Jennifer Messerly, 2016/04/20 21:16:40]

Update this comment?  And method name?

[Harry Terkelsen, 2016/04/20 23:33:28]

Done.

 /// getter/setter pairs.
-HashSet<FieldElement> findFieldsNeedingStorage(
-    Iterable<CompilationUnitElement> units, ExtensionTypeSet extensionTypes) {
-  var overrides = new HashSet<FieldElement>();
+void findFieldsNeedingStorage(

[Jennifer Messerly, 2016/04/20 21:16:40]

Oh wow!

So it looks like this old code was doing way more than it needed to! :)

The whole idea of visiting compilation units in a pre-pass was to do fancier
analysis. I thought it was ripped out but apparently not.

So I think you can radically simplify this...

Basically: just wait until we're going to emit a field (or a getter or a
setter). At that point, use checkPropertyOverride.

Then we don't need hash sets and stuff. It also keeps the logic more localized.

[Harry Terkelsen, 2016/04/20 23:33:28]

Done.

+    HashSet<FieldElement> overrides,
+    HashSet<FieldElement> propertyOverrides,
+    Iterable<CompilationUnitElement> units,
+    ExtensionTypeSet extensionTypes) {
   for (var unit in units) {
     for (var cls in unit.types) {
       var superclasses = getSuperclasses(cls);
@@ -20,20 +22,29 @@ HashSet<FieldElement> findFieldsNeedingStorage(
           checkForPropertyOverride(
               field, superclasses, overrides, extensionTypes);
         }
+        if (field.isSynthetic) {
+          if (field.setter == null) {
+            checkForPropertyOverride(
+                field, superclasses, propertyOverrides, extensionTypes,
+                checkGetter: false);
+          } else if (field.getter == null) {
+            checkForPropertyOverride(
+                field, superclasses, propertyOverrides, extensionTypes,
+                checkSetter: false);
+          }
+        }
       }
     }
   }
-
-  return overrides;
 }
 
 void checkForPropertyOverride(
     FieldElement field,
     List<ClassElement> superclasses,
     HashSet<FieldElement> overrides,
-    ExtensionTypeSet extensionTypes) {
-  assert(!field.isSynthetic);
-
+    ExtensionTypeSet extensionTypes,
+    {bool checkGetter: true,
+    bool checkSetter: true}) {
   var library = field.library;
 
   bool found = false;
@@ -49,9 +60,15 @@ void checkForPropertyOverride(
         break;
       }
 
-      found = true;
-      // Record that the super property is overridden.
-      if (superprop.library == library) overrides.add(superprop);
+      // TODO(vsm): Get rid of redundant check here.

[Jennifer Messerly, 2016/04/20 21:16:40]

+1 we should clean this up. Here's a refactor:


// just typed this in code review tool, please check for syntax and such :)
bool foundGetter = false;
bool foundSetter = false;

for (var superclass in superclasses) {
  // Stop if we reach a native type.
  if (extensionTypes.contains(superclass)) break;

  var superprop = getProperty(superclass, library, field.name);
  if (superprop == null) continue;

  var getter = superprop.getter;
  bool hasGetter = getter != null && !getter.isAbstract;
  if (hasGetter) foundGetter = true;

  var setter = superprop.setter;
  bool hasSetter =  setter != null && !setter.isAbstract;
  if (hasSetter) foundSetter = true;

  // Stop if this is an abstract getter/setter
  // TODO: why were we doing this?
  if (!hasGetter && !hasSetter) break;
}

// TODO: or wrap this in an class that stores the getter/setter elements. that
might be clean.
return new Tuple2<bool, bool>(foundGetter, foundSetter);


The result of the function is used by the caller. Therefore no "checkGetter"
"checkSetter" needs to be passed down. Nor "overrides"

[Harry Terkelsen, 2016/04/20 23:33:28]

Done.

+      if (checkGetter && getter != null && !getter.isAbstract ||
+          checkSetter && setter != null && !setter.isAbstract) {
+        found = true;
+        // TODO(vsm): Why do we need this?

[Jennifer Messerly, 2016/04/20 21:16:40]

this is not needed anymore. Please dead code remove :)

[Harry Terkelsen, 2016/04/20 23:33:28]

Done.

+        // Record that the super property is overridden.
+        if (checkGetter && checkSetter && superprop.library == library)
+          overrides.add(superprop);
+      }
     }
   }