The proposed fix will disambiguate setter overloads by ranking them according
to the 'cost' of conversion. Fewer arguments will be preferred over many
arguments (e.g. setValue(boolean) will be preferred over
setValue(boolean, boolean)). Within a group os setters with the same
number of arguments, Strings will be preferred over other primitive
types (boxed or not); and primitive types will be preferred over
non-primitive types. The fix also reduces the need of two passes
over the list of setters during disambiguation, now done in one pass.
Review at http://gwt-code-reviews.appspot.com/993801
Review by: rjrjr@google.com
git-svn-id: https://google-web-toolkit.googlecode.com/svn/trunk@9181 8db76d5a-ed1c-0410-87a9-c151d255dfc7
diff --git a/user/src/com/google/gwt/uibinder/rebind/model/OwnerFieldClass.java b/user/src/com/google/gwt/uibinder/rebind/model/OwnerFieldClass.java
index c8a7052..4cabead 100644
--- a/user/src/com/google/gwt/uibinder/rebind/model/OwnerFieldClass.java
+++ b/user/src/com/google/gwt/uibinder/rebind/model/OwnerFieldClass.java
@@ -30,6 +30,7 @@
import java.util.ArrayList;
import java.util.Collection;
+import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
@@ -42,6 +43,31 @@
* actually present as a field in the owner.
*/
public class OwnerFieldClass {
+
+ private static final int defaultCost = 4;
+ private static final Map<String, Integer> typeRank;
+ static {
+ HashMap<String, Integer> tmpTypeRank = new HashMap<String, Integer>();
+ tmpTypeRank.put("java.lang.String", 1);
+ tmpTypeRank.put("boolean", 2);
+ tmpTypeRank.put("byte", 2);
+ tmpTypeRank.put("char", 2);
+ tmpTypeRank.put("double", 2);
+ tmpTypeRank.put("float", 2);
+ tmpTypeRank.put("int", 2);
+ tmpTypeRank.put("long", 2);
+ tmpTypeRank.put("short", 2);
+ tmpTypeRank.put("java.lang.Boolean", 3);
+ tmpTypeRank.put("java.lang.Byte", 3);
+ tmpTypeRank.put("java.lang.Character", 3);
+ tmpTypeRank.put("java.lang.Double", 3);
+ tmpTypeRank.put("java.lang.Float", 3);
+ tmpTypeRank.put("java.lang.Integer", 3);
+ tmpTypeRank.put("java.lang.Long", 3);
+ tmpTypeRank.put("java.lang.Short", 3);
+ typeRank = Collections.unmodifiableMap(tmpTypeRank);
+ }
+
/**
* Gets or creates the descriptor for the given field class.
*
@@ -72,7 +98,7 @@
private final Map<String, Pair<JMethod, Integer>> uiChildren = new HashMap<String, Pair<JMethod, Integer>>();
private JConstructor uiConstructor;
-
+
/**
* Default constructor. This is package-visible for testing only.
*
@@ -105,10 +131,7 @@
*/
public JMethod getSetter(String propertyName)
throws UnableToCompleteException {
- // TODO(rjrjr) This fails for CheckBox#setValue(Boolean) because it
- // also finds CheckBox#setValue(Boolean, Boolean). Must fix for 2.0,
- // when CheckBox#setChecked will go away and CheckBox#setValue must be used
-
+
if (ambiguousSetters != null && ambiguousSetters.contains(propertyName)) {
logger.die("Ambiguous setter requested: " + rawType.getName() + "."
+ propertyName);
@@ -139,41 +162,44 @@
* use. Not having a proper setter is not an error unless of course the user
* tries to use it.
*
- * @param propertySetters the collection of setters
- * @return the setter to use, or null if none is good enough
+ * @param propertyName the name of the property/setter.
+ * @param propertySetters the collection of setters.
+ * @return the setter to use, or null if none is good enough.
*/
- private JMethod disambiguateSetters(Collection<JMethod> propertySetters) {
+ private JMethod disambiguateSetters(String propertyName,
+ Collection<JMethod> propertySetters) {
+
+ // if only have one overload, there is no need to rank them.
if (propertySetters.size() == 1) {
return propertySetters.iterator().next();
}
-
- // Pick the string setter, if there's one
+
+ // rank overloads and pick the one with minimum 'cost' of conversion.
+ JMethod preferredMethod = null;
+ int minRank = Integer.MAX_VALUE;
for (JMethod method : propertySetters) {
- JParameter[] parameters = method.getParameters();
- if (parameters.length == 1
- && parameters[0].getType().getQualifiedSourceName().equals(
- "java.lang.String")) {
- return method;
+ int rank = rankMethodOnParameters(method);
+ if (rank < minRank) {
+ minRank = rank;
+ preferredMethod = method;
+ ambiguousSetters.remove(propertyName);
+ } else if (rank == minRank &&
+ !sameParameterTypes(preferredMethod, method)) {
+ // sameParameterTypes test is necessary because a setter can be
+ // overridden by a subclass and that is not considered ambiguous.
+ if (!ambiguousSetters.contains(propertyName)) {
+ ambiguousSetters.add(propertyName);
+ }
}
}
-
- // Check if all setters aren't just the same one being overridden in parent
- // classes.
- JMethod firstMethod = null;
- for (JMethod method : propertySetters) {
- if (firstMethod == null) {
- firstMethod = method;
- continue;
- }
-
- // If the method is not the same as the first one, there's still an
- // ambiguity. Being equal means having the same parameter types.
- if (!sameParameterTypes(method, firstMethod)) {
- return null;
- }
+
+ // if the setter is ambiguous, return null.
+ if (ambiguousSetters.contains(propertyName)) {
+ return null;
}
-
- return firstMethod;
+
+ // the setter is not ambiguous therefore return the preferred overload.
+ return preferredMethod;
}
/**
@@ -230,23 +256,16 @@
Map<String, Collection<JMethod>> allSetters = findAllSetters(fieldType);
// Pass two - disambiguate
+ ambiguousSetters = new HashSet<String>();
for (String propertyName : allSetters.keySet()) {
Collection<JMethod> propertySetters = allSetters.get(propertyName);
- JMethod setter = disambiguateSetters(propertySetters);
-
- // If no setter could be disambiguated for this property, add it to the
- // set of ambiguous setters. This is later consulted if and only if the
- // setter is used.
- if (setter == null) {
- if (ambiguousSetters == null) {
- ambiguousSetters = new HashSet<String>();
- }
-
- ambiguousSetters.add(propertyName);
- }
-
+ JMethod setter = disambiguateSetters(propertyName, propertySetters);
setters.put(propertyName, setter);
}
+
+ if (ambiguousSetters.size() == 0) {
+ ambiguousSetters = null;
+ }
}
/**
@@ -313,7 +332,43 @@
&& method.getName().startsWith("set") && method.getName().length() > 3
&& method.getReturnType() == JPrimitiveType.VOID;
}
-
+
+ /**
+ * Ranks given method based on parameter conversion cost. A lower rank is
+ * preferred over a higher rank since it has a lower cost of conversion.
+ *
+ * The ranking criteria is as follows:
+ * 1) methods with fewer arguments are preferred. for instance:
+ * 'setValue(int)' is preferred 'setValue(int, int)'.
+ * 2) within a set of overloads with the same number of arguments:
+ * 2.1) String has the lowest cost = 1
+ * 2.2) primitive types, cost = 2
+ * 2.3) boxed primitive types, cost = 3
+ * 2.4) any (reference types, etc), cost = 4.
+ * 3) if a setter is overridden by a subclass and have the exact same argument
+ * types, it will not be considered ambiguous.
+ *
+ * The cost mapping is defined in
+ * {@link #typeRank typeRank }
+ * @param method.
+ * @return the rank of the method.
+ */
+ private int rankMethodOnParameters(JMethod method) {
+ JParameter[] params = method.getParameters();
+ int rank = 0;
+ for (int i = 0; i < Math.min(params.length, 10); i++) {
+ JType paramType = params[i].getType();
+ int cost = defaultCost;
+ if (typeRank.containsKey(paramType.getQualifiedSourceName())) {
+ cost = typeRank.get(paramType.getQualifiedSourceName());
+ }
+ assert (cost >= 0 && cost <= 0x07);
+ rank = rank | (cost << (3 * i));
+ }
+ assert (rank >= 0);
+ return rank;
+ }
+
/**
* Checks whether two methods have the same parameter types.
*
@@ -338,7 +393,6 @@
}
}
- // No types were different
return true;
}
}
diff --git a/user/test/com/google/gwt/uibinder/rebind/model/OwnerFieldClassTest.java b/user/test/com/google/gwt/uibinder/rebind/model/OwnerFieldClassTest.java
index 4e55392..9c84e79 100644
--- a/user/test/com/google/gwt/uibinder/rebind/model/OwnerFieldClassTest.java
+++ b/user/test/com/google/gwt/uibinder/rebind/model/OwnerFieldClassTest.java
@@ -28,6 +28,7 @@
import com.google.gwt.uibinder.rebind.JClassTypeAdapter;
import com.google.gwt.uibinder.rebind.MortalLogger;
import com.google.gwt.uibinder.rebind.UiBinderContext;
+import com.google.gwt.user.client.ui.CheckBox;
import com.google.gwt.user.client.ui.Label;
import junit.framework.TestCase;
@@ -142,6 +143,142 @@
}
}
+ /**
+ *
+ * base class for setters disambiguation tests.
+ *
+ */
+ public class baseSetters {
+ public baseSetters() {
+ }
+
+ // setvalue1 is not ambiguous
+ public void setValue1(boolean b) {
+ }
+
+ public void setValue1(Boolean b) {
+ }
+
+ // derived wins
+ public void setValue2(Integer b) {
+ }
+
+ // this overload wins
+ public void setValue3(int b) {
+ }
+
+ // this is not ambiguous since derived
+ // has the exact same signature
+ public void setValue4(int b) {
+ }
+
+ // setvalue5 is ambiguous
+ public void setValue5(float f) {
+ }
+
+ public void setValue5(double d) {
+ }
+
+ // string always wins
+ public void setValue6(String s) {
+ }
+
+ public void setValue6(char s) {
+ }
+
+ public void setValue6(Object s) {
+ }
+
+ // primitive wins
+ public void setValue7(int s) {
+ }
+
+ public void setValue7(StringBuffer s) {
+ }
+ }
+
+ /**
+ *
+ * derived class for setter disambiguation tests.
+ *
+ */
+ public class derivedSetters extends baseSetters {
+ public derivedSetters() {
+ super();
+ }
+
+ public void setValue2(int b) {
+ }
+
+ public void setValue3(Integer b) {
+ }
+
+ public void setValue4(int b) {
+ }
+ }
+
+ /**
+ * Regression test.
+ */
+ public void testCheckBoxValueSetters() throws Exception {
+ JClassType cbClassType = gwtTypeAdapter.adaptJavaClass(CheckBox.class);
+ OwnerFieldClass settersClass = OwnerFieldClass.getFieldClass(cbClassType,
+ MortalLogger.NULL, uiBinderCtx);
+ JMethod setValueSetter = settersClass.getSetter("value");
+ assertNotNull(setValueSetter);
+ }
+
+ public void testDisambiguateClassHierarchySettersBase() throws Exception {
+ // ensure that primitive types win over boxed primitive types.
+ JClassType baseClassType = gwtTypeAdapter.adaptJavaClass(baseSetters.class);
+ OwnerFieldClass settersClass = OwnerFieldClass.getFieldClass(baseClassType,
+ MortalLogger.NULL, uiBinderCtx);
+ JMethod setValueSetter = settersClass.getSetter("value1");
+ assertNotNull(setValueSetter);
+ }
+
+ public void testDisambiguateClassHierarchySettersDerived() throws Exception {
+ // ensure that primitive types win over boxed primitive types
+ // in a class hierarchy.
+ JClassType derivedClass = gwtTypeAdapter.adaptJavaClass(derivedSetters.class);
+ OwnerFieldClass settersClass = OwnerFieldClass.getFieldClass(derivedClass,
+ MortalLogger.NULL, uiBinderCtx);
+
+ // base.value1(boolean) and base.value1(Boolean) is never ambiguous
+ // must return boolean
+ assertNotNull(settersClass.getSetter("value1"));
+ assertMethod(settersClass.getSetter("value1"), "setValue1", JPrimitiveType.BOOLEAN);
+
+ // base.value2(Integer) and derived.value2(int) is not ambiguous - must be int
+ assertNotNull(settersClass.getSetter("value2"));
+ assertMethod(settersClass.getSetter("value2"), "setValue2", JPrimitiveType.INT);
+
+ // base.value3 (int) and derived.value3(Integer) is not ambiguous - must be int.
+ assertNotNull(settersClass.getSetter("value3"));
+
+ // base.value4(int) and derived.value4(int) is not ambiguous.
+ assertNotNull(settersClass.getSetter("value4"));
+
+ // base.value5(float) and base.value5(double) is ambiguous
+ try {
+ settersClass.getSetter("value5");
+ fail("Expected exception not thrown");
+ } catch (UnableToCompleteException utce) {
+ // Expected
+ }
+
+ // value6 has multiple overload but string always wins
+ // base.value6(string), base.value6(char) and base.value6(object)
+ assertNotNull(settersClass.getSetter("value6"));
+ assertMethod(settersClass.getSetter("value6"), "setValue6",
+ gwtTypeAdapter.adaptJavaClass(String.class));
+
+ // base.value7(object) and base.value7(int) is not ambiguous - must be int.
+ assertNotNull(settersClass.getSetter("value7"));
+ assertMethod(settersClass.getSetter("value7"), "setValue7",
+ JPrimitiveType.INT);
+ }
+
public void testOwnerFieldClass_setters() throws Exception {
JClassType settersType = gwtTypeAdapter.adaptJavaClass(SettersTestClass.class);
JClassType stringType = gwtTypeAdapter.adaptJavaClass(String.class);
