Fix some bugs regarding native fields.

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

Index: dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
diff --git a/dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart b/dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
index 0104ffbcd1d36917c6f060d5e2c9481a0d52144b..5143648cd7de7ae3c7c2251bfa844ee078cf1e6b 100644
--- a/dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
+++ b/dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
@@ -77,6 +77,7 @@ class CodeEmitterTask extends CompilerTask {
   String isolateProperties;
   String classesCollector;
   final Set<ClassElement> neededClasses = new Set<ClassElement>();
+  final Set<ClassElement> rtiNeededClasses = new Set<ClassElement>();
   final List<ClassElement> regularClasses = <ClassElement>[];
   final List<ClassElement> deferredClasses = <ClassElement>[];
   final List<ClassElement> nativeClasses = <ClassElement>[];
@@ -1678,7 +1679,9 @@ class CodeEmitterTask extends CompilerTask {
                        ClassBuilder builder,
                        String superName,
                        { bool classIsNative: false,
-                         bool emitStatics: false }) {
+                         bool emitStatics: false,
+                         bool onlyForRti: false }) {
+    assert(!emitStatics || !onlyForRti);
     StringBuffer buffer = new StringBuffer();
     if (emitStatics) {
       assert(invariant(classElement, superName == null, message: superName));
@@ -1698,75 +1701,86 @@ class CodeEmitterTask extends CompilerTask {
     var fieldMetadata = [];
     bool hasMetadata = false;
 
-    visitClassFields(classElement, emitStatics,
-                     (Element member,
-                      String name,
-                      String accessorName,
-                      bool needsGetter,
-                      bool needsSetter,
-                      bool needsCheckedSetter) {
-      // Ignore needsCheckedSetter - that is handled below.
-      bool needsAccessor = (needsGetter || needsSetter);
-      // We need to output the fields for non-native classes so we can auto-
-      // generate the constructor.  For native classes there are no
-      // constructors, so we don't need the fields unless we are generating
-      // accessors at runtime.
-      if (!classIsNative || needsAccessor) {
-        buffer.write(separator);
-        separator = ',';
-        var metadata = buildMetadataFunction(member);
-        if (metadata != null) {
-          hasMetadata = true;
-        } else {
-          metadata = new jsAst.LiteralNull();
-        }
-        fieldMetadata.add(metadata);
-        recordMangledField(member, accessorName, member.name.slowToString());
-        if (!needsAccessor) {
-          // Emit field for constructor generation.
-          assert(!classIsNative);
-          buffer.write(name);
-        } else {
-          // Emit (possibly renaming) field name so we can add accessors at
-          // runtime.
-          buffer.write(accessorName);
-          if (name != accessorName) {
-            buffer.write(':$name');
-            // Only the native classes can have renaming accessors.
-            assert(classIsNative);
-          }
-
-          int getterCode = 0;
-          if (needsGetter) {
-            // 01:  function() { return this.field; }
-            // 10:  function(receiver) { return receiver.field; }
-            // 11:  function(receiver) { return this.field; }
-            getterCode += backend.fieldHasInterceptedGetter(member) ? 2 : 0;
-            getterCode += backend.isInterceptorClass(classElement) ? 0 : 1;
-            // TODO(sra): 'isInterceptorClass' might not be the correct test for
-            // methods forced to use the interceptor convention because the
-            // method's class was elsewhere mixed-in to an interceptor.
-            assert(!member.isInstanceMember() || getterCode != 0);
-          }
-          int setterCode = 0;
-          if (needsSetter) {
-            // 01:  function(value) { this.field = value; }
-            // 10:  function(receiver, value) { receiver.field = value; }
-            // 11:  function(receiver, value) { this.field = value; }
-            setterCode += backend.fieldHasInterceptedSetter(member) ? 2 : 0;
-            setterCode += backend.isInterceptorClass(classElement) ? 0 : 1;
-            assert(!member.isInstanceMember() || setterCode != 0);
+    if (!onlyForRti) {
+      visitClassFields(classElement, emitStatics,

[ngeoffray, 2013/07/22 12:04:03]

MAke the following anonymous function a function in the emitter?

[ahe, 2013/07/22 12:32:05]

I thought of that, but it would require more parameters.  At least the following
local variables are captured: classIsNative, fieldMetadata, and hasMetadata.

+                       (Element member,
+                        String name,
+                        String accessorName,
+                        bool needsGetter,
+                        bool needsSetter,
+                        bool needsCheckedSetter) {
+        // Ignore needsCheckedSetter - that is handled below.
+        bool needsAccessor = (needsGetter || needsSetter);
+        // We need to output the fields for non-native classes so we can auto-
+        // generate the constructor.  For native classes there are no
+        // constructors, so we don't need the fields unless we are generating
+        // accessors at runtime.
+        if (!classIsNative || needsAccessor) {
+          buffer.write(separator);
+          separator = ',';
+          var metadata = buildMetadataFunction(member);
+          if (metadata != null) {
+            hasMetadata = true;
+          } else {
+            metadata = new jsAst.LiteralNull();
           }
-          int code = getterCode + (setterCode << 2);
-          if (code == 0) {
-            compiler.reportInternalError(
-                member, 'Internal error: code is 0 ($classElement/$member)');
+          fieldMetadata.add(metadata);
+          recordMangledField(member, accessorName, member.name.slowToString());
+          if (!needsAccessor) {
+            // Emit field for constructor generation.
+            assert(!classIsNative);
+            buffer.write(name);
           } else {
-            buffer.write(FIELD_CODE_CHARACTERS[code - FIRST_FIELD_CODE]);
+            // Emit (possibly renaming) field name so we can add accessors at
+            // runtime.
+            buffer.write(accessorName);
+            if (name != accessorName) {
+              buffer.write(':$name');
+              // Only the native classes can have renaming accessors.
+              assert(classIsNative);
+            }
+
+            int getterCode = 0;
+            if (needsGetter) {
+              // 01:  function() { return this.field; }
+              // 10:  function(receiver) { return receiver.field; }
+              // 11:  function(receiver) { return this.field; }
+              if (member.isInstanceMember()) {

[sra1, 2013/07/19 22:07:57]

When is this false?

My understanding is that member must be a field.
If member can be static, what is the meaning of generating a getter or setter
that uses 'this'?

[ahe, 2013/07/20 12:16:52]

For static fields.
This creates a "compact field specification" for static fields, and is used by
mirrors to tell which static fields exist. We don't actually create getters and
setters for static fields, so the mirror system need some information, and I
felt it made sense to reuse the existing mechanism for instance fields.

[ahe, 2013/07/22 12:32:05]

This is documented in CL 19676002.

+                getterCode += backend.fieldHasInterceptedGetter(member) ? 2 : 0;
+                getterCode += backend.isInterceptorClass(classElement) ? 0 : 1;
+                // TODO(sra): 'isInterceptorClass' might not be the correct
+                // test for methods forced to use the interceptor convention
+                // because the method's class was elsewhere mixed-in to an
+                // interceptor.
+                assert(getterCode != 0);
+              } else {
+                getterCode = 1;
+              }
+            }
+            int setterCode = 0;
+            if (needsSetter) {
+              // 01:  function(value) { this.field = value; }
+              // 10:  function(receiver, value) { receiver.field = value; }
+              // 11:  function(receiver, value) { this.field = value; }
+              if (member.isInstanceMember()) {
+                setterCode += backend.fieldHasInterceptedSetter(member) ? 2 : 0;
+                setterCode += backend.isInterceptorClass(classElement) ? 0 : 1;
+                assert(setterCode != 0);
+              } else {
+                setterCode = 1;
+              }
+            }
+            int code = getterCode + (setterCode << 2);
+            if (code == 0) {

[sra1, 2013/07/19 22:07:57]

This function is getting a bit long.
Maybe break out the computation of 'code'?

[ahe, 2013/07/20 12:16:52]

Good idea.

[ahe, 2013/07/22 12:32:05]

I'll do that in another CL to reduce conflicts with CL 19676002.

+              compiler.reportInternalError(
+                  member, 'Internal error: code is 0 ($classElement/$member)');
+            } else {
+              buffer.write(FIELD_CODE_CHARACTERS[code - FIRST_FIELD_CODE]);
+            }
           }
         }
-      }
-    });
+      });
+    }
 
     bool fieldsAdded = buffer.length > bufferClassLength;
     String compactClassData = buffer.toString();
@@ -1812,8 +1826,10 @@ class CodeEmitterTask extends CompilerTask {
    * Invariant: [classElement] must be a declaration element.
    */
   void generateClass(ClassElement classElement, CodeBuffer buffer) {
+    final onlyForRti = rtiNeededClasses.contains(classElement);
+
     assert(invariant(classElement, classElement.isDeclaration));
-    assert(invariant(classElement, !classElement.isNative()));
+    assert(invariant(classElement, !classElement.isNative() || onlyForRti));
 
     needsDefineClass = true;
     String className = namer.getName(classElement);
@@ -1836,7 +1852,7 @@ class CodeEmitterTask extends CompilerTask {
     emitClassConstructor(classElement, builder);
     emitSuper(superName, builder);
     emitRuntimeName(runtimeName, builder);
-    emitClassFields(classElement, builder, superName);
+    emitClassFields(classElement, builder, superName, onlyForRti: onlyForRti);
     emitClassGettersSetters(classElement, builder);
     if (!classElement.isMixinApplication) {
       emitInstanceMembers(classElement, builder);
@@ -3093,46 +3109,13 @@ if (typeof document !== "undefined" && document.readyState !== "complete") {
         .toSet();
     neededClasses.addAll(mixinClasses);
 
-    // 3a. Add classes that are referenced by type arguments or substitutions in
-    //     argument checks.
-    // TODO(karlklose): merge this case with 3b when unifying argument and
-    // object checks.
-    RuntimeTypes rti = backend.rti;
-    backend.rti.getRequiredArgumentClasses(backend).forEach((ClassElement c) {
-      // Types that we represent with JS native types (like int and String) do
-      // not need a class definition as we use the interceptor classes instead.
-      if (!rti.isJsNative(c)) {
-        addClassWithSuperclasses(c);
-      }
-    });
-
-    // 3b. Add classes that are referenced by substitutions in object checks and
-    //     their superclasses.
-    TypeChecks requiredChecks =
-        backend.rti.computeChecks(neededClasses, checkedClasses);
-    Set<ClassElement> classesUsedInSubstitutions =
-        rti.getClassesUsedInSubstitutions(backend, requiredChecks);
-    addClassesWithSuperclasses(classesUsedInSubstitutions);
-
-    // 3c. Add classes that contain checked generic function types. These are
-    //     needed to store the signature encoding.
-    for (FunctionType type in checkedFunctionTypes) {
-      ClassElement contextClass = Types.getClassContext(type);
-      if (contextClass != null) {
-        neededClasses.add(contextClass);
-      }
-    }
-
-    // 4. Finally, sort the classes.
-    List<ClassElement> sortedClasses = Elements.sortedByPosition(neededClasses);
-
-    // If we need noSuchMethod support, we run through all needed
+    // 3. If we need noSuchMethod support, we run through all needed
     // classes to figure out if we need the support on any native
     // class. If so, we let the native emitter deal with it.
     if (compiler.enabledNoSuchMethod) {
       SourceString noSuchMethodName = Compiler.NO_SUCH_METHOD;
       Selector noSuchMethodSelector = compiler.noSuchMethodSelector;
-      for (ClassElement element in sortedClasses) {
+      for (ClassElement element in neededClasses) {
         if (!element.isNative()) continue;
         Element member = element.lookupLocalMember(noSuchMethodName);
         if (member == null) continue;
@@ -3143,8 +3126,18 @@ if (typeof document !== "undefined" && document.readyState !== "complete") {
       }
     }
 
+    // 4. Find all classes needed for rti.

[sra1, 2013/07/19 22:07:57]

Start at 4?

[ahe, 2013/07/20 12:16:52]

I'm not sure what you mean by that. If this step was the first, I wouldn't be
able to compute the set difference.

I have two sets: classes that are instantiated, and classes that are needed for
RTI.

The classes in RTI, that are not instantiated used to be created as "holders".
Now we use the regular class mechanism to generate them.

HOWEVER, the classes not instantiated must be treated specially because they
should only be empty shells.  Their members have not been compiled, in
particular, their members are not intercepted (which is what is causing the
assertion failure on line 1755). Please see my analysis in CL
https://codereview.chromium.org/14018036/.

+    computeRtiNeededClasses();
+    rtiNeededClasses.removeAll(neededClasses);
+    neededClasses.addAll(rtiNeededClasses);
+
+    // 5. Finally, sort the classes.
+    List<ClassElement> sortedClasses = Elements.sortedByPosition(neededClasses);
+
     for (ClassElement element in sortedClasses) {
-      if (element.isNative()) {
+      if (rtiNeededClasses.contains(element)) {
+        regularClasses.add(element);
+      } else if (element.isNative()) {
         // For now, native classes cannot be deferred.
         nativeClasses.add(element);
       } else if (isDeferred(element)) {
@@ -3155,6 +3148,61 @@ if (typeof document !== "undefined" && document.readyState !== "complete") {
     }
   }
 
+  Set<ClassElement> computeRtiNeededClasses() {
+    void addClassWithSuperclasses(ClassElement cls) {
+      if (cls.name == const SourceString('CssStyleDeclaration')) {
+        throw 'hest';
+      }
+      rtiNeededClasses.add(cls);
+      for (ClassElement superclass = cls.superclass;
+          superclass != null;
+          superclass = superclass.superclass) {
+        if (superclass.name == const SourceString('CssStyleDeclaration')) {
+          throw 'fisk';

[ngeoffray, 2013/07/22 12:04:03]

fisk -> fish. Or you could just get rid of this code :-)

[ahe, 2013/07/22 12:32:05]

This is debug code I overlooked.

+        }
+        rtiNeededClasses.add(superclass);
+      }
+    }
+
+    void addClassesWithSuperclasses(Iterable<ClassElement> classes) {
+      for (ClassElement cls in classes) {
+        addClassWithSuperclasses(cls);
+      }
+    }
+
+    // 1.  Add classes that are referenced by type arguments or substitutions in
+    //     argument checks.
+    // TODO(karlklose): merge this case with 2 when unifying argument and
+    // object checks.
+    RuntimeTypes rti = backend.rti;
+    rti.getRequiredArgumentClasses(backend).forEach((ClassElement c) {
+      // Types that we represent with JS native types (like int and String) do
+      // not need a class definition as we use the interceptor classes instead.
+      if (!rti.isJsNative(c)) {
+        addClassWithSuperclasses(c);
+      }
+    });
+
+    // 2.  Add classes that are referenced by substitutions in object checks and
+    //     their superclasses.
+    TypeChecks requiredChecks =
+        rti.computeChecks(rtiNeededClasses, checkedClasses);
+    Set<ClassElement> classesUsedInSubstitutions =
+        rti.getClassesUsedInSubstitutions(backend, requiredChecks);
+    addClassesWithSuperclasses(classesUsedInSubstitutions);
+
+    // 3.  Add classes that contain checked generic function types. These are
+    //     needed to store the signature encoding.
+    for (FunctionType type in checkedFunctionTypes) {
+      ClassElement contextClass = Types.getClassContext(type);
+      if (contextClass != null) {
+        rtiNeededClasses.add(contextClass);
+      }
+    }
+
+    return rtiNeededClasses;
+  }
+
   // Optimize performance critical one shot interceptors.
   jsAst.Statement tryOptimizeOneShotInterceptor(Selector selector,
                                                 Set<ClassElement> classes) {