Field property naming fix - issues 14096, 14806

sdk/lib/_internal/compiler/implementation/js_backend/namer.dart

Index: sdk/lib/_internal/compiler/implementation/js_backend/namer.dart
diff --git a/sdk/lib/_internal/compiler/implementation/js_backend/namer.dart b/sdk/lib/_internal/compiler/implementation/js_backend/namer.dart
index e22454b9d7c423ac28d4d59d52eb8d5cb9bf1733..42a16602225e9b47907ada115e2c092ef2abfab0 100644
--- a/sdk/lib/_internal/compiler/implementation/js_backend/namer.dart
+++ b/sdk/lib/_internal/compiler/implementation/js_backend/namer.dart
@@ -440,31 +440,53 @@ class Namer implements ClosureNamer {
   String invocationMirrorInternalName(Selector selector)
       => invocationName(selector);
 
-  String instanceFieldName(Element element) {
+  String fieldPropertyName(Element element) {

[ngeoffray, 2013/11/11 10:31:32]

Please add a comment on the difference between fieldPropertyName and
fieldAccessorName.

[sra1, 2013/11/14 00:26:31]

Done.

+    if (element.isInstanceMember()) return instanceFieldPropertyName(element);

[ngeoffray, 2013/11/11 10:31:32]

Use return ... ? ... : ...;

[sra1, 2013/11/14 00:26:31]

Done.

+    return getNameOfField(element);
+  }
+
+  String fieldAccessorName(Element element) {
+    if (element.isInstanceMember()) return instanceFieldAccessorName(element);

[ngeoffray, 2013/11/11 10:31:32]

Ditto.

[sra1, 2013/11/14 00:26:31]

Done.

+    return getNameOfField(element);
+  }
+
+  String instanceFieldAccessorName(Element element) {
     String proposedName = privateName(element.getLibrary(), element.name);
     return getMappedInstanceName(proposedName);
   }
 
-  // Construct a new name for the element based on the library and class it is
-  // in.  The name here is not important, we just need to make sure it is
-  // unique.  If we are minifying, we actually construct the name from the
-  // minified versions of the class and instance names, but the result is
-  // minified once again, so that is not visible in the end result.
-  String shadowedFieldName(Element fieldElement) {
-    // Check for following situation: Native field ${fieldElement.name} has
-    // fixed JSName ${fieldElement.nativeName()}, but a subclass shadows this
-    // name.  We normally handle that by renaming the superclass field, but we
-    // can't do that because native fields have fixed JavaScript names.
-    // In practice this can't happen because we can't inherit from native
-    // classes.
-    assert (!fieldElement.hasFixedBackendName());
-
-    String libraryName = getNameOfLibrary(fieldElement.getLibrary());
-    String className = getNameOfClass(fieldElement.getEnclosingClass());
-    String instanceName = instanceFieldName(fieldElement);
+  String instanceFieldPropertyName(Element element) {
+    if (element.hasFixedBackendName()) {
+      return element.fixedBackendName();
+    }
+    // If a class is used anywhere as a mixin, we must make the name unique so
+    // that it does not accidentally shadow.  Also, the mixin name must be
+    // constant over all mixins.
+    if (!compiler.world.isUsedAsMixin(element.getEnclosingClass())) {
+      if (notShadowingAnotherField(element)) {

[ngeoffray, 2013/11/11 10:31:32]

Remove an if and use '&&' ?

[sra1, 2013/11/14 00:26:31]

I prefer not to write !a && b  when a is complex, so I restructured the
function.

+        String proposedName = privateName(element.getLibrary(), element.name);
+        return getMappedInstanceName(proposedName);
+      }
+    }
+
+    // Construct a new name for the element based on the library and class it is
+    // in.  The name here is not important, we just need to make sure it is
+    // unique.  If we are minifying, we actually construct the name from the
+    // minified version of the class name, but the result is minified once
+    // again, so that is not visible in the end result.
+    String libraryName = getNameOfLibrary(element.getLibrary());
+    String className = getNameOfClass(element.getEnclosingClass());
+    String instanceName = privateName(element.getLibrary(), element.name);
     return getMappedInstanceName('$libraryName\$$className\$$instanceName');
   }
 
+  bool notShadowingAnotherField(Element element) {
+    // TODO(sra): Search entire chain to ensure not shadowing anther *field*.

[ngeoffray, 2013/11/11 10:31:32]

anter -> another

[sra1, 2013/11/14 00:26:31]

Done.

+    // This version it tricked into using an ugly name by shadowing of an
+    // abstract getter.

[ngeoffray, 2013/11/11 10:31:32]

What would it require to fix this?

[sra1, 2013/11/14 00:26:31]

Done.

+    return element.getEnclosingClass().lookupSuperMember(element.name) == null;
+  }
+
   String setterName(Element element) {
     // We dynamically create setters from the field-name. The setter name must
     // therefore be derived from the instance field-name.
@@ -718,7 +740,11 @@ class Namer implements ClosureNamer {
       } else if (element.kind == ElementKind.SETTER) {
         return setterName(element);
       } else if (element.kind == ElementKind.FIELD) {
-        return instanceFieldName(element);
+        // TODO(sra): Audit all paths here - do they mean the getter or the
+        // property?

[ngeoffray, 2013/11/11 10:31:32]

Remove TODO? I guess otherwise it does not pass any test.

[sra1, 2013/11/14 00:26:31]

Done.

+        compiler.internalError('getName for bad kind: ${element.kind}',
+                               node: element.parseNode(compiler));
+        return instanceFieldAccessorName(element);
       } else {
         compiler.internalError('getName for bad kind: ${element.kind}',
                                node: element.parseNode(compiler));