Fix some bugs regarding native fields.

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

 // Copyright (c) 2012, 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.
 
 part of js_backend;
 
 /**
  * A function element that represents a closure call. The signature is copied
  * from the given element.
  */
@@ skipping 59 matching lines @@
   ConstantEmitter constantEmitter;
   NativeEmitter nativeEmitter;
   CodeBuffer mainBuffer;
   final CodeBuffer deferredLibraries = new CodeBuffer();
   final CodeBuffer deferredConstants = new CodeBuffer();
   /** Shorter access to [isolatePropertiesName]. Both here in the code, as
       well as in the generated code. */
   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>[];
   final List<Selector> trivialNsmHandlers = <Selector>[];
   final Map<String, String> mangledFieldNames = <String, String>{};
   final Map<String, String> mangledGlobalFieldNames = <String, String>{};
   final Set<String> recordedMangledNames = new Set<String>();
   final Set<String> interceptorInvocationNames = new Set<String>();
 
   /// A list of JS expressions that represent metadata, parameter names and
@@ skipping 1581 matching lines @@
     }
     assert(invariant(member, previousName == memberName,
                      message: '$previousName != ${memberName}'));
   }
 
   /// Returns `true` if fields added.
   bool emitClassFields(ClassElement classElement,
                        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));
     } else {
       assert(invariant(classElement, superName != null));
       String nativeName =
           namer.getPrimitiveInterceptorRuntimeName(classElement);
       if (nativeName != null) {
         buffer.write('$nativeName/');
       }
       buffer.write('$superName;');
     }
     int bufferClassLength = buffer.length;
 
     String separator = '';
 
     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);
+    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();
           }
+          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);
-          }
-          int code = getterCode + (setterCode << 2);
-          if (code == 0) {
-            compiler.reportInternalError(
-                member, 'Internal error: code is 0 ($classElement/$member)');
-          } else {
-            buffer.write(FIELD_CODE_CHARACTERS[code - FIRST_FIELD_CODE]);
+            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();
     jsAst.Expression classDataNode = js.string(compactClassData);
     if (hasMetadata) {
       fieldMetadata.insert(0, classDataNode);
       classDataNode = new jsAst.ArrayInitializer.from(fieldMetadata);
     }
     builder.addProperty('', classDataNode);
     return fieldsAdded;
@@ skipping 25 matching lines @@
       });
     });
   }
 
   /**
    * Documentation wanted -- johnniwinther
    *
    * 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);
 
     ClassElement superclass = classElement.superclass;
     String superName = "";
     if (superclass != null) {
       superName = namer.getName(superclass);
     }
     String runtimeName =
         namer.getPrimitiveInterceptorRuntimeName(classElement);
 
     if (classElement.isMixinApplication) {
       String mixinName = namer.getName(computeMixinClass(classElement));
       superName = '$superName+$mixinName';
       needsMixinSupport = true;
     }
 
     ClassBuilder builder = new ClassBuilder();
     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);
     }
     emitIsTests(classElement, builder);
 
     emitClassBuilderWithReflectionData(
         className, classElement, builder, buffer);
   }
 
@@ skipping 1236 matching lines @@
     // 1. We need to generate all classes that are instantiated.
     addClassesWithSuperclasses(instantiatedClasses);
 
     // 2. Add all classes used as mixins.
     Set<ClassElement> mixinClasses = neededClasses
         .where((ClassElement element) => element.isMixinApplication)
         .map(computeMixinClass)
         .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;
         if (noSuchMethodSelector.applies(member, compiler)) {
           nativeEmitter.handleNoSuchMethod = true;
           break;
         }
       }
     }
 
+    // 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)) {
         deferredClasses.add(element);
       } else {
         regularClasses.add(element);
       }
     }
   }
 
+  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) {
     jsAst.Expression isNumber(String variable) {
       return js('typeof $variable == "number"');
     }
 
     jsAst.Expression isNotObject(String variable) {
       return js('typeof $variable != "object"');
     }
@@ skipping 739 matching lines @@
 
 const String HOOKS_API_USAGE = """
 // The code supports the following hooks:
 // dartPrint(message)   - if this function is defined it is called
 //                        instead of the Dart [print] method.
 // dartMainRunner(main) - if this function is defined, the Dart [main]
 //                        method will not be invoked directly.
 //                        Instead, a closure that will invoke [main] is
 //                        passed to [dartMainRunner].
 """;