Re: RFR: 8009411 : getMethods should not inherit static methods from interfaces
Looks fine Joel; thanks, -Joe On 10/21/2013 04:12 AM, Joel Borggrén-Franck wrote: Hi All, http://cr.openjdk.java.net/~jfranck/8009411/webrev.03/ New webrev including the test originally contributed by Peter (I noticed the commit doesn't include Peter in contributed by, I have fixed that in my local copy). Static methods on implemented interfaces are now ignored in the search for methods in getMethod() , and are filtered out from getMethods() calls. Tests run: jdk_lang (including the updated testing with this commit) jdk_beans1, jdk_beans2, jdk_beans3 please review cheers /Joel On 26 sep 2013, at 22:53, Joel Borggrén-Franck joel.fra...@oracle.com wrote: Hi again, New webrev with more tests contributed by Amy Lu. http://cr.openjdk.java.net/~jfranck/8009411/webrev.02/ No change to non-test code since webrev.01 cheers /Joel On 2013-09-12, Joel Borggrén-Franck wrote: Hi again, New webrev: http://cr.openjdk.java.net/~jfranck/8009411/webrev.01/ Thanks to Remi and Peter for the quick feedback, I've updated the code to use for-each as well as fixing getMethod(...). Andreas Lundblad also added ~100 testcases for getMethod(), both positive and negative. Please review cheers /Joel On 2013-09-09, Joel Borggrén-Franck wrote: Hi Pleaser review fix for 8009411 : getMethods should not inherit static methods from interfaces The issue is that since we added static methods to interfaces those have erroneously been reflected in getMethods of implementing classes. This fix filters out static interface methods from superinterfaces when adding methods. I have also added a note to the javadoc for both getMembers and getDeclaredMembers pointing this out (though it is implied from JLS). Webrev is based on the clarification to getMethods and friends out for review on this list. Webrev: http://cr.openjdk.java.net/~jfranck/8009411/webrev.00/ Bug is here: http://bugs.sun.com/view_bug.do?bug_id=8009411 For oracle reviewers, ccc is approved. cheers /Joel
Re: RFR: 8009411 : getMethods should not inherit static methods from interfaces
Hi All, http://cr.openjdk.java.net/~jfranck/8009411/webrev.03/ New webrev including the test originally contributed by Peter (I noticed the commit doesn't include Peter in contributed by, I have fixed that in my local copy). Static methods on implemented interfaces are now ignored in the search for methods in getMethod() , and are filtered out from getMethods() calls. Tests run: jdk_lang (including the updated testing with this commit) jdk_beans1, jdk_beans2, jdk_beans3 please review cheers /Joel On 26 sep 2013, at 22:53, Joel Borggrén-Franck joel.fra...@oracle.com wrote: Hi again, New webrev with more tests contributed by Amy Lu. http://cr.openjdk.java.net/~jfranck/8009411/webrev.02/ No change to non-test code since webrev.01 cheers /Joel On 2013-09-12, Joel Borggrén-Franck wrote: Hi again, New webrev: http://cr.openjdk.java.net/~jfranck/8009411/webrev.01/ Thanks to Remi and Peter for the quick feedback, I've updated the code to use for-each as well as fixing getMethod(...). Andreas Lundblad also added ~100 testcases for getMethod(), both positive and negative. Please review cheers /Joel On 2013-09-09, Joel Borggrén-Franck wrote: Hi Pleaser review fix for 8009411 : getMethods should not inherit static methods from interfaces The issue is that since we added static methods to interfaces those have erroneously been reflected in getMethods of implementing classes. This fix filters out static interface methods from superinterfaces when adding methods. I have also added a note to the javadoc for both getMembers and getDeclaredMembers pointing this out (though it is implied from JLS). Webrev is based on the clarification to getMethods and friends out for review on this list. Webrev: http://cr.openjdk.java.net/~jfranck/8009411/webrev.00/ Bug is here: http://bugs.sun.com/view_bug.do?bug_id=8009411 For oracle reviewers, ccc is approved. cheers /Joel
Re: RFR: 8009411 : getMethods should not inherit static methods from interfaces
On 09/12/2013 03:37 PM, Joel Borggrén-Franck wrote:
Hi again,
New webrev: http://cr.openjdk.java.net/~jfranck/8009411/webrev.01/
Thanks to Remi and Peter for the quick feedback, I've updated the code
to use for-each as well as fixing getMethod(...).
Andreas Lundblad also added ~100 testcases for getMethod(), both
positive and negative.
Please review
cheers
/Joel
Hi Joel,
This looks correct now. I just wonder about the following corner case.
Suppose you have:
public interface A {
default void m() {
}
}
public interface B extends A {
static void m() {
}
}
public interface C extends B {
}
This does not actually compile, and I think this is correct:
/home/peter/work/local/DefaultVsStaticInterfaceMethodCrash/src/B.java
Error:Error:line (2)java: m() in B clashes with m() in A
overriding method is static
But if interface A at first does not have method m(), it compiles. If
later default method m() is added to interface A and A is compiled
separately, then what happens?
The C.class.getMethods() returns a 1 element array containing A.m(), but
C.class.getMethod(m) throws NoSuchMethodException.
This seems inconsistent, but it's a corner case that can only happen
with separate compilation.
Regards, Peter
On 2013-09-09, Joel Borggrén-Franck wrote:
Hi
Pleaser review fix for 8009411 : getMethods should not inherit static methods
from interfaces
The issue is that since we added static methods to interfaces those have
erroneously been reflected in getMethods of implementing classes. This fix
filters out static interface methods from superinterfaces when adding methods.
I have also added a note to the javadoc for both getMembers and
getDeclaredMembers pointing this out (though it is implied from JLS). Webrev is
based on the clarification to getMethods and friends out for review on this
list.
Webrev: http://cr.openjdk.java.net/~jfranck/8009411/webrev.00/
Bug is here: http://bugs.sun.com/view_bug.do?bug_id=8009411
For oracle reviewers, ccc is approved.
cheers
/Joel
Re: RFR: 8009411 : getMethods should not inherit static methods from interfaces
On 09/13/2013 12:18 PM, Peter Levart wrote:
The C.class.getMethods() returns a 1 element array containing A.m(),
but C.class.getMethod(m) throws NoSuchMethodException.
This seems inconsistent, but it's a corner case that can only happen
with separate compilation.
Sorry Joel, I must have tested the unpatched code for
C.class.getMethods(). In is in fact consistent with
C.calss.getMethod(m). Both calls don't return the A.m() method. in
getMethod(m) case the recursion is stoped when B.m() static method is
encountered and in getMethods() case the inherited method A.m() is
removed from inheritedMethods array by the following in
privateGetPublicMethods():
// Filter out all local methods from inherited ones
for (int i = 0; i methods.length(); i++) {
Method m = methods.get(i);
inheritedMethods.removeByNameAndSignature(m);
}
...when collecting B's methods...
But the question remains whether A.m() should be seen by invokeinterface
C.m() in spite of the fact that there is static B.m() in between and
whether reflection should follow that.
Regards, Peter
Re: RFR: 8009411 : getMethods should not inherit static methods from interfaces
Hi Peter,
Interesting case, thanks for the testing.
On Sep 13, 2013, at 1:15 PM, Peter Levart peter.lev...@gmail.com wrote:
On 09/13/2013 12:18 PM, Peter Levart wrote:
The C.class.getMethods() returns a 1 element array containing A.m(), but
C.class.getMethod(m) throws NoSuchMethodException.
This seems inconsistent, but it's a corner case that can only happen with
separate compilation.
Sorry Joel, I must have tested the unpatched code for C.class.getMethods().
In is in fact consistent with C.calss.getMethod(m). Both calls don't return
the A.m() method. in getMethod(m) case the recursion is stoped when B.m()
static method is encountered and in getMethods() case the inherited method
A.m() is removed from inheritedMethods array by the following in
privateGetPublicMethods():
// Filter out all local methods from inherited ones
for (int i = 0; i methods.length(); i++) {
Method m = methods.get(i);
inheritedMethods.removeByNameAndSignature(m);
}
...when collecting B's methods...
But the question remains whether A.m() should be seen by invokeinterface
C.m() in spite of the fact that there is static B.m() in between and whether
reflection should follow that.
I can't see any reason why an invokeinterface C.m() should not find a default
method just because there is a static method with the same name and sig in
between. However the separate compilation setup required to arrange this is
non-trivial in itself :)
I also believe that reflection should accommodate separate compilation to the
best extent possible. In this case I would prefer if A.m() were found and I
think the fix in both reasonably small and without compatibility concerns since
this is new territory.
cheers
/Joel
Re: RFR: 8009411 : getMethods should not inherit static methods from interfaces
On 09/13/2013 02:55 PM, Joel Borggrén-Franck wrote:
Hi Peter,
Interesting case, thanks for the testing.
On Sep 13, 2013, at 1:15 PM, Peter Levart peter.lev...@gmail.com wrote:
On 09/13/2013 12:18 PM, Peter Levart wrote:
The C.class.getMethods() returns a 1 element array containing A.m(), but
C.class.getMethod(m) throws NoSuchMethodException.
This seems inconsistent, but it's a corner case that can only happen with
separate compilation.
Sorry Joel, I must have tested the unpatched code for C.class.getMethods(). In is in fact
consistent with C.calss.getMethod(m). Both calls don't return the A.m() method. in
getMethod(m) case the recursion is stoped when B.m() static method is encountered and
in getMethods() case the inherited method A.m() is removed from inheritedMethods array by the
following in privateGetPublicMethods():
// Filter out all local methods from inherited ones
for (int i = 0; i methods.length(); i++) {
Method m = methods.get(i);
inheritedMethods.removeByNameAndSignature(m);
}
...when collecting B's methods...
But the question remains whether A.m() should be seen by invokeinterface C.m()
in spite of the fact that there is static B.m() in between and whether
reflection should follow that.
I can't see any reason why an invokeinterface C.m() should not find a default method just
because there is a static method with the same name and sig in between.
However the separate compilation setup required to arrange this is non-trivial in itself
:)
It is non-trivial to arrange in a unit test. See the following for one
possible trick:
http://cr.openjdk.java.net/~plevart/jdk8-tl/AnnotationType/webrev.05/test/java/lang/annotation/AnnotationType/AnnotationTypeRuntimeAssumptionTest.java.html
It's expectable that such situations will arise in practice. interface A
can be a part of a vendor X library that's adding default methods in a
new release, for example, and B C can be part of a vendor Y app that
is using the library...
Regards, Peter
I also believe that reflection should accommodate separate compilation to the
best extent possible. In this case I would prefer if A.m() were found and I
think the fix in both reasonably small and without compatibility concerns since
this is new territory.
cheers
/Joel
Re: RFR: 8009411 : getMethods should not inherit static methods from interfaces
On Sep 13, 2013, at 3:18 PM, Peter Levart peter.lev...@gmail.com wrote:
On 09/13/2013 02:55 PM, Joel Borggrén-Franck wrote:
Hi Peter,
Interesting case, thanks for the testing.
On Sep 13, 2013, at 1:15 PM, Peter Levart peter.lev...@gmail.com wrote:
On 09/13/2013 12:18 PM, Peter Levart wrote:
The C.class.getMethods() returns a 1 element array containing A.m(), but
C.class.getMethod(m) throws NoSuchMethodException.
This seems inconsistent, but it's a corner case that can only happen with
separate compilation.
Sorry Joel, I must have tested the unpatched code for C.class.getMethods().
In is in fact consistent with C.calss.getMethod(m). Both calls don't
return the A.m() method. in getMethod(m) case the recursion is stoped
when B.m() static method is encountered and in getMethods() case the
inherited method A.m() is removed from inheritedMethods array by the
following in privateGetPublicMethods():
// Filter out all local methods from inherited ones
for (int i = 0; i methods.length(); i++) {
Method m = methods.get(i);
inheritedMethods.removeByNameAndSignature(m);
}
...when collecting B's methods...
But the question remains whether A.m() should be seen by invokeinterface
C.m() in spite of the fact that there is static B.m() in between and
whether reflection should follow that.
I can't see any reason why an invokeinterface C.m() should not find a
default method just because there is a static method with the same name and
sig in between. However the separate compilation setup required to arrange
this is non-trivial in itself :)
It is non-trivial to arrange in a unit test. See the following for one
possible trick:
http://cr.openjdk.java.net/~plevart/jdk8-tl/AnnotationType/webrev.05/test/java/lang/annotation/AnnotationType/AnnotationTypeRuntimeAssumptionTest.java.html
It's expectable that such situations will arise in practice. interface A can
be a part of a vendor X library that's adding default methods in a new
release, for example, and B C can be part of a vendor Y app that is using
the library...
I fully agree this will most probably occur in practice. But the situation is
more complicated for an invoke, since there must have been a A.m() present when
C was compiled in order to have an invokeinterface to be there in the first
place. Just adding a default to A won't trigger this since there won't be an
invokeinterface C.m().
Something like this might do:
Compile A { void m(); }, B {} and C {} together.
Compile A { } B { static m()} together
Compile A { default m(); }
Do you have a preference for how we should do this?
cheers
/Joel
Re: RFR: 8009411 : getMethods should not inherit static methods from interfaces
On 09/13/2013 03:38 PM, Joel Borggrén-Franck wrote:
I fully agree this will most probably occur in practice. But the situation is
more complicated for an invoke, since there must have been a A.m() present when
C was compiled in order to have an invokeinterface to be there in the first
place. Just adding a default to A won't trigger this since there won't be an
invokeinterface C.m().
Something like this might do:
Compile A { void m(); }, B {} and C {} together.
Compile A { } B { static m()} together
Compile A { default m(); }
Do you have a preference for how we should do this?
cheers
/Joel
Something like that, yes:
http://cr.openjdk.java.net/~plevart/jdk8-tl/StaticVsDefaultInterfaceMethods/StaticInterfaceMethodInWayOfDefault.java
It shows that VM throws IncompatibleClassChangeError if at invoke time
the search for method encounters a static method in it's path. Is this
correct behavior? Should reflection also throw a similar exception? This
could only be done when requesting a specific method (getMethod(name,
parameterTypes)), but what to do with getMethods() then?
Running using _v1 classes:
C.m() called
Running using _v2 classes:
Exception in thread main java.lang.IncompatibleClassChangeError:
Expecting non-static method StaticInterfaceMethodInWayOfDefault$B.m()V
at
StaticInterfaceMethodInWayOfDefault$TestTask$1.m(StaticInterfaceMethodInWayOfDefault.java)
at
StaticInterfaceMethodInWayOfDefault$TestTask.run(StaticInterfaceMethodInWayOfDefault.java:36)
at
StaticInterfaceMethodInWayOfDefault.main(StaticInterfaceMethodInWayOfDefault.java:55)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
at
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:491)
at com.intellij.rt.execution.application.AppMain.main(AppMain.java:120)
Regards, Peter
Re: RFR: 8009411 : getMethods should not inherit static methods from interfaces
Hi again, New webrev: http://cr.openjdk.java.net/~jfranck/8009411/webrev.01/ Thanks to Remi and Peter for the quick feedback, I've updated the code to use for-each as well as fixing getMethod(...). Andreas Lundblad also added ~100 testcases for getMethod(), both positive and negative. Please review cheers /Joel On 2013-09-09, Joel Borggrén-Franck wrote: Hi Pleaser review fix for 8009411 : getMethods should not inherit static methods from interfaces The issue is that since we added static methods to interfaces those have erroneously been reflected in getMethods of implementing classes. This fix filters out static interface methods from superinterfaces when adding methods. I have also added a note to the javadoc for both getMembers and getDeclaredMembers pointing this out (though it is implied from JLS). Webrev is based on the clarification to getMethods and friends out for review on this list. Webrev: http://cr.openjdk.java.net/~jfranck/8009411/webrev.00/ Bug is here: http://bugs.sun.com/view_bug.do?bug_id=8009411 For oracle reviewers, ccc is approved. cheers /Joel
Re: RFR: 8009411 : getMethods should not inherit static methods from interfaces
On 2013-09-09, Remi Forax wrote:
On 09/09/2013 07:27 PM, Joel Borggrén-Franck wrote:
On 9 sep 2013, at 19:00, Joel Borggrén-Franck joel.fra...@oracle.com wrote:
The issue is that since we added static methods to interfaces those have
erroneously been reflected in getMethods of implementing classes. This fix
filters out static interface methods from superinterfaces when adding
methods. I have also added a note to the javadoc for both getMembers and
getDeclaredMembers pointing this out (though it is implied from JLS).
Correcting myself, I added a note to getMethods() and getMethod(String name
…)
getDeclaredMethod{s} shouldn't need a note.
cheers
/Joel
also Joel you can use for-each and avoid to load ma[i] twice
(even if the JIT will certainly avoid the double load in this
specific case).
void addAllNonStatic(Method[] methodArray) {
for (Method method:methodArray) {
if (!Modifier.isStatic(method.getModifiers())) {
add(method);
}
}
}
I should use foreach if possible in any case, looks much cleaner.
cheres
/Joel
Re: RFR: 8009411 : getMethods should not inherit static methods from interfaces
Hi Joel,
The fix is ok for getMethods(), but I think the getMethod(String name,
Class?... parameterTypes) should also be fixed. Currently the
following code:
public class StaticInterfaceMethodTest {
public interface A {
static void m() {}
}
public interface B extends A {
}
public static void main(String[] args) throws Exception {
B.class.getMethod(m);
}
}
...succeeds, but should throw NoSuchMethodException, I think.
Regards, Peter
On 09/09/2013 07:00 PM, Joel Borggrén-Franck wrote:
Hi
Pleaser review fix for 8009411 : getMethods should not inherit static methods
from interfaces
The issue is that since we added static methods to interfaces those have
erroneously been reflected in getMethods of implementing classes. This fix
filters out static interface methods from superinterfaces when adding methods.
I have also added a note to the javadoc for both getMembers and
getDeclaredMembers pointing this out (though it is implied from JLS). Webrev is
based on the clarification to getMethods and friends out for review on this
list.
Webrev: http://cr.openjdk.java.net/~jfranck/8009411/webrev.00/
Bug is here: http://bugs.sun.com/view_bug.do?bug_id=8009411
For oracle reviewers, ccc is approved.
cheers
/Joel
RFR: 8009411 : getMethods should not inherit static methods from interfaces
Hi Pleaser review fix for 8009411 : getMethods should not inherit static methods from interfaces The issue is that since we added static methods to interfaces those have erroneously been reflected in getMethods of implementing classes. This fix filters out static interface methods from superinterfaces when adding methods. I have also added a note to the javadoc for both getMembers and getDeclaredMembers pointing this out (though it is implied from JLS). Webrev is based on the clarification to getMethods and friends out for review on this list. Webrev: http://cr.openjdk.java.net/~jfranck/8009411/webrev.00/ Bug is here: http://bugs.sun.com/view_bug.do?bug_id=8009411 For oracle reviewers, ccc is approved. cheers /Joel
Re: RFR: 8009411 : getMethods should not inherit static methods from interfaces
Hi Peter,
You are correct, thanks for noticing this. Also after reading your mail I
looked through the test in test/java/lang/refledt/DefaultStaticTest/ and just
realized the tests doesn't cover getMethod() at all. So this change needs more
tests as well.
I'll post a new webrev later this week.
cheers
/Joel
On 9 sep 2013, at 19:24, Peter Levart peter.lev...@gmail.com wrote:
Hi Joel,
The fix is ok for getMethods(), but I think the getMethod(String name,
Class?... parameterTypes) should also be fixed. Currently the following
code:
public class StaticInterfaceMethodTest {
public interface A {
static void m() {}
}
public interface B extends A {
}
public static void main(String[] args) throws Exception {
B.class.getMethod(m);
}
}
...succeeds, but should throw NoSuchMethodException, I think.
Regards, Peter
On 09/09/2013 07:00 PM, Joel Borggrén-Franck wrote:
Hi
Pleaser review fix for 8009411 : getMethods should not inherit static
methods from interfaces
The issue is that since we added static methods to interfaces those have
erroneously been reflected in getMethods of implementing classes. This fix
filters out static interface methods from superinterfaces when adding
methods. I have also added a note to the javadoc for both getMembers and
getDeclaredMembers pointing this out (though it is implied from JLS). Webrev
is based on the clarification to getMethods and friends out for review on
this list.
Webrev:
http://cr.openjdk.java.net/~jfranck/8009411/webrev.00/
Bug is here:
http://bugs.sun.com/view_bug.do?bug_id=8009411
For oracle reviewers, ccc is approved.
cheers
/Joel
Re: RFR: 8009411 : getMethods should not inherit static methods from interfaces
On 9 sep 2013, at 19:00, Joel Borggrén-Franck joel.fra...@oracle.com wrote:
The issue is that since we added static methods to interfaces those have
erroneously been reflected in getMethods of implementing classes. This fix
filters out static interface methods from superinterfaces when adding
methods. I have also added a note to the javadoc for both getMembers and
getDeclaredMembers pointing this out (though it is implied from JLS).
Correcting myself, I added a note to getMethods() and getMethod(String name …)
getDeclaredMethod{s} shouldn't need a note.
cheers
/Joel
Re: RFR: 8009411 : getMethods should not inherit static methods from interfaces
On 09/09/2013 07:27 PM, Joel Borggrén-Franck wrote:
On 9 sep 2013, at 19:00, Joel Borggrén-Franck joel.fra...@oracle.com wrote:
The issue is that since we added static methods to interfaces those have
erroneously been reflected in getMethods of implementing classes. This fix
filters out static interface methods from superinterfaces when adding methods.
I have also added a note to the javadoc for both getMembers and
getDeclaredMembers pointing this out (though it is implied from JLS).
Correcting myself, I added a note to getMethods() and getMethod(String name …)
getDeclaredMethod{s} shouldn't need a note.
cheers
/Joel
also Joel you can use for-each and avoid to load ma[i] twice
(even if the JIT will certainly avoid the double load in this specific
case).
void addAllNonStatic(Method[] methodArray) {
for (Method method:methodArray) {
if (!Modifier.isStatic(method.getModifiers())) {
add(method);
}
}
}
cheers,
Rémi
