Author: rj...@google.com
Date: Tue Mar 31 07:01:58 2009
New Revision: 5113

Modified:
    branches/snapshot-2009.03.20/branch-info.txt
     
branches/snapshot-2009.03.20/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java
     
branches/snapshot-2009.03.20/user/test/com/google/gwt/dev/jjs/test/CompilerTest.java

Log:
A rollback of /tr...@c4974 and /tr...@c4943 to undo a compiler bug breaking  
method overrieds



Modified: branches/snapshot-2009.03.20/branch-info.txt
==============================================================================
--- branches/snapshot-2009.03.20/branch-info.txt        (original)
+++ branches/snapshot-2009.03.20/branch-info.txt        Tue Mar 31 07:01:58 2009
@@ -6,4 +6,4 @@
  /branches/snapshot-2009.03.03 was created (r5066) as a straight copy from  
/trunk/@5065

  Merges:
-
+A rollback of /tr...@c4974 and /tr...@c4943 was merged (r????) into this  
branch to undo a compiler bug breaking method overrieds

Modified:  
branches/snapshot-2009.03.20/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java
==============================================================================
---  
branches/snapshot-2009.03.20/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java
       
(original)
+++  
branches/snapshot-2009.03.20/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java
       
Tue Mar 31 07:01:58 2009
@@ -173,7 +173,6 @@
  import org.eclipse.jdt.internal.compiler.lookup.ReferenceBinding;
  import org.eclipse.jdt.internal.compiler.lookup.SourceTypeBinding;
  import org.eclipse.jdt.internal.compiler.lookup.SyntheticArgumentBinding;
-import org.eclipse.jdt.internal.compiler.lookup.SyntheticMethodBinding;
  import org.eclipse.jdt.internal.compiler.lookup.TypeBinding;
  import org.eclipse.jdt.internal.compiler.lookup.TypeIds;
  import org.eclipse.jdt.internal.compiler.lookup.VariableBinding;
@@ -185,14 +184,19 @@
  import java.lang.reflect.InvocationTargetException;
  import java.lang.reflect.Method;
  import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
  import java.util.Collections;
  import java.util.HashMap;
+import java.util.HashSet;
  import java.util.IdentityHashMap;
  import java.util.Iterator;
+import java.util.LinkedHashSet;
  import java.util.LinkedList;
  import java.util.List;
  import java.util.Map;
  import java.util.Queue;
+import java.util.Set;
  import java.util.TreeSet;

  /**
@@ -222,6 +226,55 @@
     * them to our AST.
     */
    private static class JavaASTGenerationVisitor {
+
+    /**
+     * Find all interface methods declared to be implemented by a specified
+     * class.
+     */
+    private static Collection<MethodBinding> findInterfaceMethods(
+        ReferenceBinding clazzBinding) {
+      List<MethodBinding> methods = new ArrayList<MethodBinding>();
+      Set<ReferenceBinding> seen = new HashSet<ReferenceBinding>();
+      findInterfaceMethodsRecursive(clazzBinding, methods, seen);
+      return methods;
+    }
+
+    private static void findInterfaceMethodsRecursive(
+        ReferenceBinding clazzBinding, List<MethodBinding> methods,
+        Set<ReferenceBinding> seen) {
+      if (!seen.add(clazzBinding)) {
+        return;
+      }
+
+      if (clazzBinding.isInterface()) {
+        methods.addAll(Arrays.asList(clazzBinding.methods()));
+      }
+
+      ReferenceBinding superclass = clazzBinding.superclass();
+      if (superclass != null) {
+        findInterfaceMethodsRecursive(superclass, methods, seen);
+      }
+
+      ReferenceBinding[] interfaces = clazzBinding.superInterfaces();
+      if (interfaces != null) {
+        for (ReferenceBinding supinterf : interfaces) {
+          findInterfaceMethodsRecursive(supinterf, methods, seen);
+        }
+      }
+    }
+
+    private static boolean inheritsMethodWithIdenticalSignature(
+        JClassType clazz, JMethod method) {
+      for (JClassType sup = clazz.extnds; sup != null; sup = sup.extnds) {
+        for (JMethod m : sup.methods) {
+          if (JTypeOracle.methodsDoMatch(m, method)) {
+            return true;
+          }
+        }
+      }
+      return false;
+    }
+
      private static InternalCompilerException translateException(JNode node,
          Throwable e) {
        if (e instanceof OutOfMemoryError) {
@@ -289,6 +342,11 @@
       * </p>
       *
       * <p>
+     * This method assumes that method overrides are already recorded for  
the
+     * class and all its inherited classes and interfaces.
+     * </p>
+     *
+     * <p>
       * The need for these bridges was pointed out in issue 3064. The goal  
is
       * that virtual method calls through an interface type are translated  
to
       * JavaScript that will function correctly. If the interface signature
@@ -302,31 +360,57 @@
       * case, a bridge method should be added that overrides the interface  
method
       * and then calls the implementation method.
       * </p>
-     *
-     * <p>
-     * This method should only be called once all regular, non-bridge  
methods
-     * have been installed on the GWT types.
-     * </p>
       */
-    public void addBridgeMethods(SourceTypeBinding clazzBinding) {
+    public void addBridgeMethods(ReferenceBinding clazzBinding,
+        JProgram program, Set<ReferenceBinding> typesWithBridges) {
        if (clazzBinding.isInterface() || clazzBinding.isAbstract()) {
          // Only add bridges in concrete classes, to simplify matters.
          return;
        }

-      JClassType clazz = (JClassType) typeMap.get(clazzBinding);
+      if (!typesWithBridges.add(clazzBinding)) {
+        // Nothing to do -- this class already has bridges added.
+        return;
+      }

        /*
-       * The JDT adds bridge methods in all the places GWT needs them. Look
-       * through the bridge methods the JDT added.
+       * Add to the superclass first, so that bridge methods end up as  
high in
+       * the hierarchy as possible.
         */
-      if (clazzBinding.syntheticMethods() != null) {
-        for (SyntheticMethodBinding synthmeth :  
clazzBinding.syntheticMethods()) {
-          if (synthmeth.purpose == SyntheticMethodBinding.BridgeMethod) {
-            JMethod implmeth = (JMethod)  
typeMap.get(synthmeth.targetMethod);
+      if (clazzBinding.superclass() != null) {
+        addBridgeMethods(clazzBinding.superclass(), program,  
typesWithBridges);
+      }

-            createBridgeMethod(clazz, synthmeth, implmeth);
+      JClassType clazz = (JClassType) typeMap.get(clazzBinding);
+
+      for (MethodBinding imethBinding :  
findInterfaceMethods(clazzBinding)) {
+        JMethod interfmeth = (JMethod) typeMap.get(imethBinding);
+        JMethod implmeth = (JMethod) typeMap.get(findMethodImplementing(
+            imethBinding, clazzBinding));
+
+        assert (!implmeth.isStatic());
+
+        if (!implmeth.overrides.contains(interfmeth)) {
+          if (JTypeOracle.methodsDoMatch(interfmeth, implmeth)) {
+            /*
+             * Two cases are caught here. First, a bridge method might  
already
+             * be included in a superclass. In that case, there's nothing  
to do.
+             * Second, the bridge method might have the exact signature as  
the
+             * bridged-to method. In that case, leave out the bridge  
method. It
+             * makes things harder on the optimizers, but it avoids adding  
a
+             * bridge method that must later be removed. Further, such a  
bridge
+             * method would need to be different from the current ones in  
order
+             * to avoid an infinite recursion.
+             */
+            continue;
            }
+
+          if (inheritsMethodWithIdenticalSignature(clazz, interfmeth)) {
+            // An equivalent bridge has already been added in a superclass
+            continue;
+          }
+
+          createBridgeMethod(program, clazz, interfmeth, implmeth);
          }
        }
      }
@@ -2033,44 +2117,29 @@
        }
      }

-    private boolean classHasMethodOverriding(JClassType clazz, JMethod  
over) {
-      for (JMethod meth : clazz.methods) {
-        if (meth.overrides.contains(over)) {
-          return true;
-        }
-      }
-
-      if (clazz.extnds != null && classHasMethodOverriding(clazz.extnds,  
over)) {
-        return true;
-      }
-
-      return false;
-    }
-
      /**
       * Create a bridge method. It calls a same-named method with the same
       * arguments, but with a different type signature.
+     *
+     * @param program The program being modified
       * @param clazz The class to put the bridge method in
-     * @param jdtBridgeMethod The corresponding bridge method added in the  
JDT
+     * @param interfmeth The interface method to bridge from
       * @param implmeth The implementation method to bridge to
       */
-    private void createBridgeMethod(JClassType clazz,  
SyntheticMethodBinding jdtBridgeMethod,
-        JMethod implmeth) {
+    private void createBridgeMethod(JProgram program, JClassType clazz,
+        JMethod interfmeth, JMethod implmeth) {
        SourceInfo info = program.createSourceInfoSynthetic(
            GenerateJavaAST.class, "bridge method");

        // create the method itself
        JMethod bridgeMethod = program.createMethod(info,
-          jdtBridgeMethod.selector, clazz,
-          (JType) typeMap.get(jdtBridgeMethod.returnType.erasure()), false,
-          false, true, false, false);
-      int paramIdx = 0;
-      for (TypeBinding jdtParamType : jdtBridgeMethod.parameters) {
-        String paramName = "p" + paramIdx++;
-        JType paramType = (JType) typeMap.get(jdtParamType.erasure());
+          interfmeth.getName().toCharArray(), clazz, interfmeth.getType(),
+          false, false, true, false, false);
+      for (JParameter param : interfmeth.params) {
          program.createParameter(program.createSourceInfoSynthetic(
              GenerateJavaAST.class, "part of a bridge method"),
-            paramName.toCharArray(), paramType, true, false, bridgeMethod);
+            param.getName().toCharArray(), param.getType(), true, false,
+            bridgeMethod);
        }
        bridgeMethod.freezeParamTypes();

@@ -2104,16 +2173,9 @@
        JMethodBody body = (JMethodBody) bridgeMethod.getBody();
        body.getStatements().add(callOrReturn);

-      // add overrides, but only for interface methods that the class does  
not
-      // already override
-      List<JMethod> overrides = new ArrayList<JMethod>();
-      tryFindUpRefs(bridgeMethod, overrides);
-      for (JMethod over : overrides) {
-        if (!classHasMethodOverriding(clazz, over)) {
-          bridgeMethod.overrides.add(over);
-          bridgeMethod.overrides.addAll(over.overrides);
-        }
-      }
+      // make the bridge override the interface method
+      bridgeMethod.overrides.add(interfmeth);
+      bridgeMethod.overrides.addAll(interfmeth.overrides);
      }

      private JDeclarationStatement createDeclaration(SourceInfo info,
@@ -2294,6 +2356,27 @@
      }

      /**
+     * Search the class hierarchy starting at <code>clazz</code> looking  
for a
+     * method implementing <code>imeth</code>. Look only in classes, not
+     * interfaces.
+     */
+    private MethodBinding findMethodImplementing(MethodBinding interfmeth,
+        ReferenceBinding clazz) {
+      for (MethodBinding tryMethod :  
clazz.getMethods(interfmeth.selector)) {
+        if (methodParameterErasuresAreEqual(interfmeth, tryMethod)) {
+          return tryMethod;
+        }
+      }
+
+      if (clazz.superclass() == null) {
+        throw new InternalCompilerException("Could not find implementation  
of "
+            + interfmeth + " for class " + clazz);
+      }
+
+      return findMethodImplementing(interfmeth, clazz.superclass());
+    }
+
+    /**
       * Get a new label of a particular name, or create a new one if it  
doesn't
       * exist already.
       */
@@ -2431,6 +2514,32 @@
      }

      /**
+     * Check whether two methods have matching parameter types. Assumes the
+     * selectors match.
+     */
+    private boolean methodParameterErasuresAreEqual(MethodBinding meth1,
+        MethodBinding meth2) {
+      /*
+       * Don't use MethodBinding.areParameterErasuresEqual because that  
method
+       * assumes equal types are ==, but that's not necessarily true in  
this
+       * context.
+       */
+      if (meth1.parameters.length != meth2.parameters.length) {
+        return false;
+      }
+
+      for (int i = 0; i < meth1.parameters.length; i++) {
+        TypeBinding type1 = meth1.parameters[i].erasure();
+        TypeBinding type2 = meth2.parameters[i].erasure();
+        if (typeMap.get(type1) != typeMap.get(type2)) {
+          return false;
+        }
+      }
+
+      return true;
+    }
+
+    /**
       * Sometimes a variable reference can be to a local or parameter in an  
an
       * enclosing method. This is a tricky situation to detect. There's no
       * obvious way to tell, but the clue we can get from JDT is that the  
local's
@@ -2541,15 +2650,13 @@

      /**
       * For a given method, try to find all methods that it  
overrides/implements.
-     * This version does not use JDT.
+     * An experimental version that doesn't use JDT. Right now it's only  
used to
+     * resolve upRefs for Object.getClass(), which is synthetic on  
everything
+     * other than object.
       */
      private void tryFindUpRefs(JMethod method) {
-      tryFindUpRefs(method, method.overrides);
-    }
-
-    private void tryFindUpRefs(JMethod method, List<JMethod> overrides) {
        if (method.getEnclosingType() != null) {
-        tryFindUpRefsRecursive(method, method.getEnclosingType(),  
overrides);
+        tryFindUpRefsRecursive(method, method.getEnclosingType());
        }
      }

@@ -2566,14 +2673,28 @@
       * methods that it overrides/implements.
       */
      private void tryFindUpRefsRecursive(JMethod method,
-        JReferenceType searchThisType, List<JMethod> overrides) {
+        JReferenceType searchThisType) {

        // See if this class has any uprefs, unless this class is myself
        if (method.getEnclosingType() != searchThisType) {
-        for (JMethod upRef : searchThisType.methods) {
-          if (JTypeOracle.methodsDoMatch(method, upRef)
-              && !overrides.contains(upRef)) {
-            overrides.add(upRef);
+        outer : for (JMethod upRef : searchThisType.methods) {
+          if (upRef.isStatic()) {
+            continue;
+          }
+          if (!upRef.getName().equals(method.getName())) {
+            continue;
+          }
+          if (upRef.params.size() != method.params.size()) {
+            continue;
+          }
+          for (int i = 0, c = upRef.params.size(); i < c; ++i) {
+            if (upRef.params.get(i).getType() !=  
method.params.get(i).getType()) {
+              continue outer;
+            }
+          }
+
+          if (!method.overrides.contains(upRef)) {
+            method.overrides.add(upRef);
              break;
            }
          }
@@ -2581,12 +2702,12 @@

        // recurse super class
        if (searchThisType.extnds != null) {
-        tryFindUpRefsRecursive(method, searchThisType.extnds, overrides);
+        tryFindUpRefsRecursive(method, searchThisType.extnds);
        }

        // recurse super interfaces
        for (JInterfaceType intf : searchThisType.implments) {
-        tryFindUpRefsRecursive(method, intf, overrides);
+        tryFindUpRefsRecursive(method, intf);
        }
      }

@@ -2973,14 +3094,17 @@
      // Construct the basic AST.
      JavaASTGenerationVisitor v = new JavaASTGenerationVisitor(typeMap,
          jprogram, options);
-    for (TypeDeclaration type : types) {
-      v.processType(type);
-    }
-    for (TypeDeclaration type : types) {
-      v.addBridgeMethods(type.binding);
+    for (int i = 0; i < types.length; ++i) {
+      v.processType(types[i]);
      }
      Collections.sort(jprogram.getDeclaredTypes(), new HasNameSort());

+    // add any necessary bridge methods
+    Set<ReferenceBinding> typesWithBridges = new  
HashSet<ReferenceBinding>();
+    for (TypeDeclaration decl : types) {
+      v.addBridgeMethods(decl.binding, jprogram, typesWithBridges);
+    }
+
      // Process JSNI.
      Map<JsniMethodBody, AbstractMethodDeclaration> jsniMethodMap =  
v.getJsniMethodMap();
      new JsniRefGenerationVisitor(jprogram, jsProgram,  
jsniMethodMap).accept(jprogram);
@@ -2998,6 +3122,32 @@
          IProblem.ExternalProblemNotFixable, null, ProblemSeverities.Error,
          info.getStartPos(), info.getEndPos(), info.getStartLine(),  
startColumn);
      compResult.record(problem, methodDeclaration);
+  }
+
+  private static void addSuperclassesAndInterfaces(JReferenceType clazz,
+      Set<JReferenceType> supers) {
+    if (clazz == null) {
+      return;
+    }
+    if (supers.contains(clazz)) {
+      return;
+    }
+    supers.add(clazz);
+    addSuperclassesAndInterfaces(clazz.extnds, supers);
+    for (JReferenceType intf : clazz.implments) {
+      addSuperclassesAndInterfaces(intf, supers);
+    }
+  }
+
+  /**
+   * Returns a collection of all inherited classes and interfaces, plus the
+   * class itself.
+   */
+  private static Collection<JReferenceType> allSuperClassesAndInterfaces(
+      JClassType clazz) {
+    Set<JReferenceType> supers = new LinkedHashSet<JReferenceType>();
+    addSuperclassesAndInterfaces(clazz, supers);
+    return supers;
    }

    /**

Modified:  
branches/snapshot-2009.03.20/user/test/com/google/gwt/dev/jjs/test/CompilerTest.java
==============================================================================
---  
branches/snapshot-2009.03.20/user/test/com/google/gwt/dev/jjs/test/CompilerTest.java
     
(original)
+++  
branches/snapshot-2009.03.20/user/test/com/google/gwt/dev/jjs/test/CompilerTest.java
     
Tue Mar 31 07:01:58 2009
@@ -20,20 +20,12 @@

  import junit.framework.Assert;

-import java.util.EventListener;
-
  /**
   * Miscellaneous tests of the Java to JavaScript compiler.
   */
  @SuppressWarnings("unused")
  public class CompilerTest extends GWTTestCase {

-  interface Silly { }
-
-  interface SillyComparable<T extends Silly> extends Comparable<T> {
-    int compareTo(T obj);
-  }
-
    private abstract static class AbstractSuper {
      public static String foo() {
        if (FALSE) {
@@ -47,23 +39,6 @@
    private abstract static class Apple implements Fruit {
    }

-  private abstract static class Bm2BaseEvent {
-  }
-
-  private abstract static class Bm2ComponentEvent extends Bm2BaseEvent {
-  }
-
-  private static class Bm2KeyNav<E extends Bm2ComponentEvent> implements
-      Bm2Listener<E> {
-    public int handleEvent(Bm2ComponentEvent ce) {
-      return 5;
-    }
-  }
-
-  private interface Bm2Listener<E extends Bm2BaseEvent> extends  
EventListener {
-    int handleEvent(E be);
-  }
-
    private static class ConcreteSub extends AbstractSuper {
      public static String foo() {
        if (FALSE) {
@@ -265,7 +240,7 @@
     * superclass, it can be necessary to add a bridge method that overrides  
the
     * interface method and calls the inherited method.
     */
-  public void testBridgeMethods1() {
+  public void testBridgeMethods() {
      abstract class AbstractFoo {
        public int compareTo(AbstractFoo o) {
          return 0;
@@ -287,39 +262,10 @@
      Comparable<AbstractFoo> comparable1 = new MyFooSub();
      assertEquals(0, comparable1.compareTo(new MyFoo()));

+
      Comparable<AbstractFoo> comparable2 = new MyFoo();
      assertEquals(0, comparable2.compareTo(new MyFooSub()));
-  }
-
-  /**
-   * Issue 3304. Doing superclasses first is tricky when the superclass is  
a raw
-   * type.
-   */
-  @SuppressWarnings("unchecked")
-  public void testBridgeMethods2() {
-    Bm2KeyNav<?> obs = new Bm2KeyNav() {
-    };
-    assertEquals(5, obs.handleEvent(null));
-  }
-
-  /**
-   * When adding a bridge method, be sure to handle transitive overrides
-   * relationships.
-   */
-  public void testBridgeMethods3() {
-    class AbstractFoo implements Silly {
-      public int compareTo(AbstractFoo obj) {
-        if (FALSE) {
-          return compareTo(obj);
-        }
-        return 0;
-      }
-    }
-    class MyFoo extends AbstractFoo implements  
SillyComparable<AbstractFoo> {
-    }
-
-    assertEquals(0, new MyFoo().compareTo(new MyFoo()));
-  }
+}

    public void testCastOptimizer() {
      Granny g = new Granny();

--~--~---------~--~----~------------~-------~--~----~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~----------~----~----~----~------~----~------~--~---

Reply via email to