Re: [cp-patches] RFC: fix in proxy creation of interfaces redefining toString, equals or hashCode
Mark Wielaard a écrit :
>Please do add a little comment for the isCoreObjectMethod() method
>saying this. Just for people like me that might want to be clever and
>update the code to be more efficient. Although your new Mauve patches
>will now catch them if they try this :)
>
>
>
Ok, commited with an extra comment for this method
2006-02-20 Olivier Jolly <[EMAIL PROTECTED]>
* java/lang/reflect/Proxy.java
(ProxyData.getProxyData): Skipped overriding of core methods.
(ProxyData.isCoreObjectMethod): New method.
>Thanks,
>
>Mark
>
>
Olivier
Index: Proxy.java
===
RCS file: /sources/classpath/classpath/java/lang/reflect/Proxy.java,v
retrieving revision 1.25
diff -u -r1.25 Proxy.java
--- Proxy.java 21 Oct 2005 00:30:28 - 1.25
+++ Proxy.java 20 Feb 2006 20:16:42 -
@@ -1,5 +1,5 @@
/* Proxy.java -- build a proxy class that implements reflected interfaces
- Copyright (C) 2001, 2002, 2003, 2004, 2005 Free Software Foundation, Inc.
+ Copyright (C) 2001, 2002, 2003, 2004, 2005, 2006 Free Software Foundation, Inc.
This file is part of GNU Classpath.
@@ -42,6 +42,7 @@
import java.io.Serializable;
import java.security.ProtectionDomain;
+import java.util.Arrays;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
@@ -732,6 +733,12 @@
int j = methods.length;
while (--j >= 0)
{
+ if (isCoreObjectMethod(methods[j]))
+{
+ // In the case of an attempt to redefine a public non-final
+ // method of Object, we must skip it
+ continue;
+}
ProxySignature sig = new ProxySignature(methods[j]);
ProxySignature old = (ProxySignature) method_set.put(sig, sig);
if (old != null)
@@ -752,6 +759,41 @@
}
return data;
}
+
+/**
+ * Checks whether the method is similar to a public non-final method of
+ * Object or not (i.e. with the same name and parameter types). Note that we
+ * can't rely, directly or indirectly (via Collection.contains) on
+ * Method.equals as it would also check the declaring class, what we do not
+ * want. We only want to check that the given method have the same signature
+ * as a core method (same name and parameter types)
+ *
+ * @param method the method to check
+ * @return whether the method has the same name and parameter types as
+ * Object.equals, Object.hashCode or Object.toString
+ * @see java.lang.Object#equals(Object)
+ * @see java.lang.Object#hashCode()
+ * @see java.lang.Object#toString()
+ */
+private static boolean isCoreObjectMethod(Method method)
+{
+ String methodName = method.getName();
+ if (methodName.equals("equals"))
+{
+ return Arrays.equals(method.getParameterTypes(),
+ new Class[] { Object.class });
+}
+ if (methodName.equals("hashCode"))
+{
+ return method.getParameterTypes().length == 0;
+}
+ if (methodName.equals("toString"))
+{
+ return method.getParameterTypes().length == 0;
+}
+ return false;
+}
+
} // class ProxyData
/**
Re: [cp-patches] RFC: fix in proxy creation of interfaces redefining toString, equals or hashCode
Hi Olivier, On Mon, 2006-02-20 at 14:23 +0100, Olivier Jolly wrote: > First I wanted to implement it with a contains, but unfortunately, > contains relies on equals, which will check that the declaring class are > the same before saying that two methods are equals. In this case, we > know we have the same methods expected that the declaring class are > differents, that's the whole point of the fix [...] Doh. Right. So much for the insightful peer review... Please do add a little comment for the isCoreObjectMethod() method saying this. Just for people like me that might want to be clever and update the code to be more efficient. Although your new Mauve patches will now catch them if they try this :) Thanks, Mark signature.asc Description: This is a digitally signed message part
Re: [cp-patches] RFC: fix in proxy creation of interfaces redefining toString, equals or hashCode
Olivier Jolly wrote: It's not an error for an interface to declare a method that matches an Object method (such as equals), and then for some bytecode somewhere to INVOKEINTERFACE that method, even though INVOKEVIRTUAL would "make more sense". E.g. code somewhere in Eclipse does this. No, it's not an error and it actually mostly works just like everyone expects it to. Excepted that Sun stated special cases in the javadoc of Proxy : "An invocation of the hashCode, equals, or toString methods declared in java.lang.Object on a proxy instance will be encoded and dispatched to the invocation handler's invoke method in the same manner as interface method invocations are encoded and dispatched, as described above. The declaring class of the Method object passed to invoke will be java.lang.Object." Ah, thanks... I knew there had to be a kludge in there somewhere :-) -Archie __ Archie Cobbs *CTO, Awarix* http://www.awarix.com
Re: [cp-patches] RFC: fix in proxy creation of interfaces redefining toString, equals or hashCode
Archie Cobbs wrote:
Olivier Jolly wrote:
[...]
What does the failure look like?
Within an InvocationHandler, the method instance which is given as
parameter is not equals to Object.class.getDeclaredMethod("...", ...)
if the interface the proxy is built on redefines one of the "core"
Object methods. Easymock is correctly expecting to catch all "equals"
call with Object.class.getDeclaredMethod("equals", new Class[]
{Object.class}).equals(method) which is not the case without my patch.
Still curious.. :-)
It's not an error for an interface to declare a method that matches
an Object method (such as equals), and then for some bytecode somewhere
to INVOKEINTERFACE that method, even though INVOKEVIRTUAL would "make
more sense". E.g. code somewhere in Eclipse does this.
No, it's not an error and it actually mostly works just like everyone
expects it to. Excepted that Sun stated special cases in the javadoc of
Proxy :
"An invocation of the hashCode, equals, or toString methods declared in
java.lang.Object on a proxy instance will be encoded and dispatched to
the invocation handler's invoke method in the same manner as interface
method invocations are encoded and dispatched, as described above. The
declaring class of the Method object passed to invoke will be
java.lang.Object."
So isn't this an easymock bug? In such a case, wouldn't easymock fail
to catch a valid equals() invocation?
Seen the javadoc, I think that easymock devs did the right thing and
maybe that Java designers did this on that very purpose : catching all
core methods in an InvocationHandler is easy
-Archie
Olivier
Re: [cp-patches] RFC: fix in proxy creation of interfaces redefining toString, equals or hashCode
Olivier Jolly wrote:
I bumped into a major problem with spring and easymock which boiled
down to being a subtle definition of a Proxy. In the case of the
creation of a proxy which defines the same method as Object (which is
non final and public), the method in the interface must be so that
getDeclaringClass is Object.class and not the actual interface.
Just curious.. are you sure this is a Classpath issue and not
a VM issue?
yes, as far as the VM isn't handling natively the creation of
java.lang.reflect.Proxy. In the Proxy.java source file of classpath
we're (incorrectly) building the proxy with methods that shouldn't be
there.
What does the failure look like?
Within an InvocationHandler, the method instance which is given as
parameter is not equals to Object.class.getDeclaredMethod("...", ...) if
the interface the proxy is built on redefines one of the "core" Object
methods. Easymock is correctly expecting to catch all "equals" call with
Object.class.getDeclaredMethod("equals", new Class[]
{Object.class}).equals(method) which is not the case without my patch.
Still curious.. :-)
It's not an error for an interface to declare a method that matches
an Object method (such as equals), and then for some bytecode somewhere
to INVOKEINTERFACE that method, even though INVOKEVIRTUAL would "make
more sense". E.g. code somewhere in Eclipse does this.
So isn't this an easymock bug? In such a case, wouldn't easymock fail
to catch a valid equals() invocation?
-Archie
__
Archie Cobbs *CTO, Awarix* http://www.awarix.com
Re: [cp-patches] RFC: fix in proxy creation of interfaces redefining toString, equals or hashCode
Archie Cobbs wrote:
Olivier Jolly wrote:
I bumped into a major problem with spring and easymock which boiled
down to being a subtle definition of a Proxy. In the case of the
creation of a proxy which defines the same method as Object (which is
non final and public), the method in the interface must be so that
getDeclaringClass is Object.class and not the actual interface.
Just curious.. are you sure this is a Classpath issue and not
a VM issue?
yes, as far as the VM isn't handling natively the creation of
java.lang.reflect.Proxy. In the Proxy.java source file of classpath
we're (incorrectly) building the proxy with methods that shouldn't be there.
What does the failure look like?
Within an InvocationHandler, the method instance which is given as
parameter is not equals to Object.class.getDeclaredMethod("...", ...) if
the interface the proxy is built on redefines one of the "core" Object
methods. Easymock is correctly expecting to catch all "equals" call with
Object.class.getDeclaredMethod("equals", new Class[]
{Object.class}).equals(method) which is not the case without my patch.
Also (very minor) would isCoreObjectMethod() be more simply implemented
using Object.class.getDeclaringMethod().. ?
Not at all, because the equality you would then use will prevent the
checked method (defined in an interface) from matching anything declared
in Object.class (because of the getDeclaringClass() difference).
Thanks for the feedback
-Archie
Olivier
Re: [cp-patches] RFC: fix in proxy creation of interfaces redefining toString, equals or hashCode
Olivier Jolly wrote: I bumped into a major problem with spring and easymock which boiled down to being a subtle definition of a Proxy. In the case of the creation of a proxy which defines the same method as Object (which is non final and public), the method in the interface must be so that getDeclaringClass is Object.class and not the actual interface. Just curious.. are you sure this is a Classpath issue and not a VM issue? What does the failure look like? Also (very minor) would isCoreObjectMethod() be more simply implemented using Object.class.getDeclaringMethod().. ? -Archie __ Archie Cobbs *CTO, Awarix* http://www.awarix.com
Re: [cp-patches] RFC: fix in proxy creation of interfaces redefining toString, equals or hashCode
Re all, Mark Wielaard wrote: I just submitted a mauve testlet to stress this point and here is the proposition of fix in Proxy.java (for non native Proxy implementations, that's it). When iterating over the methods to be added to the proxy, if any is matching an Object one (by its name and parameter types only), it is skipped. Since we're already adding them automatically, it is not missing of the resulting proxy and the declaring class remains Object.class 2006-02-19 Olivier Jolly * java/lang/reflect/Proxy.java (ProxyData.getProxyData): Skipped overriding of core methods. (ProxyData.isCoreObjectMethod): New method. Instead of adding a new method, could you just check ProxySignature.coreMethods.contains(sig)? Not that it matters a lot since I don't think the set in ProxySignature.coreMethods and what is checked in ProxyData.isCoreObjectMethod() will ever get out of sync. First I wanted to implement it with a contains, but unfortunately, contains relies on equals, which will check that the declaring class are the same before saying that two methods are equals. In this case, we know we have the same methods expected that the declaring class are differents, that's the whole point of the fix, hence this little method which acts like a contains with a very specific "equals". If we had a contains(Object, Comparator) on collections, I would have use it. Please feel free to commit it either as is or by checking coreMethods.contains(). The logic seems fine to me either way. Okey, I'll commit later today with this method if nobody comes out with a better solution by then... I have not yet passed the integrality of mauve tests to ensure regression, but I will by the time this patch is accepted :) Yeah. Mauve is getting a bit big. You can run a subset by defining KEYS (see the README). The autobuilder on builder.classpath.org will run all tests. I meant mauve is already a monster with KEYS=classpath :) Cheers, Take care Mark Olivier
Re: [cp-patches] RFC: fix in proxy creation of interfaces redefining toString, equals or hashCode
Hi Olivier, On Sun, 2006-02-19 at 22:43 +0100, Olivier Jolly wrote: > I bumped into a major problem with spring and easymock which boiled > down to being a subtle definition of a Proxy. In the case of the > creation of a proxy which defines the same method as Object (which is > non final and public), the method in the interface must be so that > getDeclaringClass is Object.class and not the actual interface. O. Good catch again! > I just submitted a mauve testlet to stress this point and here is the > proposition of fix in Proxy.java (for non native Proxy implementations, > that's it). When iterating over the methods to be added to the proxy, if > any is matching an Object one (by its name and parameter types only), it > is skipped. Since we're already adding them automatically, it is not > missing of the resulting proxy and the declaring class remains Object.class > > 2006-02-19 Olivier Jolly > > * java/lang/reflect/Proxy.java > (ProxyData.getProxyData): Skipped overriding of core methods. > (ProxyData.isCoreObjectMethod): New method. Instead of adding a new method, could you just check ProxySignature.coreMethods.contains(sig)? Not that it matters a lot since I don't think the set in ProxySignature.coreMethods and what is checked in ProxyData.isCoreObjectMethod() will ever get out of sync. Please feel free to commit it either as is or by checking coreMethods.contains(). The logic seems fine to me either way. > I have not yet passed the integrality of mauve tests to ensure > regression, but I will by the time this patch is accepted :) Yeah. Mauve is getting a bit big. You can run a subset by defining KEYS (see the README). The autobuilder on builder.classpath.org will run all tests. Cheers, Mark signature.asc Description: This is a digitally signed message part
[cp-patches] RFC: fix in proxy creation of interfaces redefining toString, equals or hashCode
Hi,
I bumped into a major problem with spring and easymock which boiled
down to being a subtle definition of a Proxy. In the case of the
creation of a proxy which defines the same method as Object (which is
non final and public), the method in the interface must be so that
getDeclaringClass is Object.class and not the actual interface.
I just submitted a mauve testlet to stress this point and here is the
proposition of fix in Proxy.java (for non native Proxy implementations,
that's it). When iterating over the methods to be added to the proxy, if
any is matching an Object one (by its name and parameter types only), it
is skipped. Since we're already adding them automatically, it is not
missing of the resulting proxy and the declaring class remains Object.class
2006-02-19 Olivier Jolly
* java/lang/reflect/Proxy.java
(ProxyData.getProxyData): Skipped overriding of core methods.
(ProxyData.isCoreObjectMethod): New method.
I have not yet passed the integrality of mauve tests to ensure
regression, but I will by the time this patch is accepted :)
Regards
Olivier
Index: Proxy.java
===
RCS file: /sources/classpath/classpath/java/lang/reflect/Proxy.java,v
retrieving revision 1.25
diff -u -r1.25 Proxy.java
--- Proxy.java 21 Oct 2005 00:30:28 - 1.25
+++ Proxy.java 19 Feb 2006 21:34:10 -
@@ -1,5 +1,5 @@
/* Proxy.java -- build a proxy class that implements reflected interfaces
- Copyright (C) 2001, 2002, 2003, 2004, 2005 Free Software Foundation, Inc.
+ Copyright (C) 2001, 2002, 2003, 2004, 2005, 2006 Free Software Foundation, Inc.
This file is part of GNU Classpath.
@@ -42,6 +42,7 @@
import java.io.Serializable;
import java.security.ProtectionDomain;
+import java.util.Arrays;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
@@ -732,6 +733,12 @@
int j = methods.length;
while (--j >= 0)
{
+ if (isCoreObjectMethod(methods[j]))
+{
+ // In the case of an attempt to redefine a public non-final
+ // method of Object, we must skip it
+ continue;
+}
ProxySignature sig = new ProxySignature(methods[j]);
ProxySignature old = (ProxySignature) method_set.put(sig, sig);
if (old != null)
@@ -752,6 +759,36 @@
}
return data;
}
+
+/**
+ * Checks whether the method is a public non-final method of Object or not
+ *
+ * @param method the method to check
+ * @return whether the method has the same name and parameter types as
+ * Object.equals, Object.hashCode or Object.toString
+ * @see java.lang.Object#equals(Object)
+ * @see java.lang.Object#hashCode()
+ * @see java.lang.Object#toString()
+ */
+private static boolean isCoreObjectMethod(Method method)
+{
+ String methodName = method.getName();
+ if (methodName.equals("equals"))
+{
+ return Arrays.equals(method.getParameterTypes(),
+ new Class[] { Object.class });
+}
+ if (methodName.equals("hashCode"))
+{
+ return method.getParameterTypes().length == 0;
+}
+ if (methodName.equals("toString"))
+{
+ return method.getParameterTypes().length == 0;
+}
+ return false;
+}
+
} // class ProxyData
/**
