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

lib/src/compiler/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.
 
 import 'dart:collection' show HashSet;
 import 'package:analyzer/dart/ast/ast.dart' show Identifier;
 import 'package:analyzer/dart/element/element.dart';
 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);
       for (var field in cls.fields) {
         if (!field.isSynthetic && !overrides.contains(field)) {
           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;
   for (var superclass in superclasses) {
     var superprop = getProperty(superclass, library, field.name);
     if (superprop != null) {
       // If we find an abstract getter/setter pair, stop the search.
       var getter = superprop.getter;
       var setter = superprop.setter;
       if (!extensionTypes.contains(superclass) &&
           (getter == null || getter.isAbstract) &&
           (setter == null || setter.isAbstract)) {
         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);
+      }
     }
   }
 
   // If this we found a super property, then this property overrides it.
   if (found) overrides.add(field);
 }
 
 FieldElement getProperty(
     ClassElement cls, LibraryElement fromLibrary, String name) {
   // Properties from a different library are not accessible.
@@ skipping 16 matching lines @@
       if (mixin != null) result.add(mixin);
     }
     var supertype = cls.supertype;
     if (supertype == null) break;
 
     cls = supertype.element;
     result.add(cls);
   }
   return result;
 }