Bugzilla – Full Text Bug Listing
| Summary: | Method.getMethods() returns different methods (compared to standard compiler) for public/non-public inheritance hierarchies | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Bartosz Bankowski <bbankowski> | ||||||
| Component: | Core | Assignee: | Stephan Herrmann <stephan.herrmann> | ||||||
| Status: | RESOLVED NOT_ECLIPSE | QA Contact: | |||||||
| Severity: | major | ||||||||
| Priority: | P3 | CC: | amj87.iitr, daniel_megert, jarthana, pwebster, satyam.kandula, stephan.herrmann | ||||||
| Version: | 3.6.2 | ||||||||
| Target Milestone: | 4.14 M3 | ||||||||
| Hardware: | PC | ||||||||
| OS: | Windows XP | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
Created attachment 193429 [details]
Sources to reproduce the problem
From a quick look at the compiled byte code: Eclipse indeed generates an extra bridge method into AbstractChild: public bridge synthetic void method(Object arg1, String arg2) This method is unnecessary, because it has the same signature as the inherited method and it does nothing but delegate (no cast involved). I haven't seen a paragraph in the spec that defines when bridges must (not) be created. So maybe the bytecode is legal, but maybe we should still try to avoid creating such unnecessary bridge methods? Will consider a fix for this in 3.8 M4 See eclipse bug https://bugs.eclipse.org/bugs/show_bug.cgi?id=288658 and related sun bugs http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6342411. Need to see why those considerations don't kick in for javac in this test case. In particular, need to construct a test case similar to the one in http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6342411 using comment#0 test case to see if the IllegalAccessException shows up. (In reply to comment #4) > See eclipse bug https://bugs.eclipse.org/bugs/show_bug.cgi?id=288658 and > related sun bugs http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6342411. > > Need to see why those considerations don't kick in for javac in this test > case. Duh. Eclipse compiler misses the nuance that such a bridge would need to be introduced only when inheriting public methods introduced by non public classes from a named package. Patch will follow shortly. Simpler test case that can be cut & pasted into package explorer:
import java.lang.reflect.Method;
interface Interface<M, T> {
void method(T x, M m);
}
abstract class Abstract<T> implements Interface<String, T> {
@Override
public void method(T x, String string) {
}
}
public class X extends Abstract<Integer> {
public static void main(String[] args) {
printMethods(X.class);
}
private static void printMethods(Class<?> clazz) {
System.out.println("Methods for class: " + clazz.getName());
for (Method m : clazz.getMethods()) {
if (m.getName().startsWith("method")) {
System.out.println(m);
}
}
}
}
Turns out the regression tests added for bug 288658 are actually testing the current scenario (i.e default package) I need to write junits for that bug now :) Released fix and tests in 3.8 stream via commit id : f966b547afe1ce6e5e600680e6f3625db1aa756f. Ayush, this also needs to be backported to 3.7.2 after a bit more testing as this is a regression in 3.7 relative to 3.6. Released in 3.7 maintenance via commit cc817186788a86e60b58427a9b7e989e5093af0a Verified for 3.8M4 using build I20111202-0800. Released in 362+java7 branch via commit b317a1f8cb626a10310b9e4d19e0361f15b998da javac is behaving the same if the package is default or not. With this patch, Eclipse is behaving similar to javac if the package is default and not otherwise. I believe there could be more to the observations in comment 5. (In reply to comment #12) > javac is behaving the same if the package is default or not. With this patch, > Eclipse is behaving similar to javac if the package is default and not > otherwise. I believe there could be more to the observations in comment 5. Not sure I understand, can you post a test case that shows material difference ? Thanks. Run the following test with both javac and Eclipse. They differ in the output. This is the simpler test case in comment 6, put in a package. ###### package pkg1; import java.lang.reflect.Method; interface Interface<M, T> { void method(T x, M m); } abstract class Abstract<T> implements Interface<String, T> { @Override public void method(T x, String string) { } } public class X extends Abstract<Integer> { public static void main(String[] args) { printMethods(X.class); } private static void printMethods(Class<?> clazz) { System.out.println("Methods for class: " + clazz.getName()); for (Method m : clazz.getMethods()) { if (m.getName().startsWith("method")) { System.out.println(m); } } } } ######## Some more update:
I don't think javac differs depends on the defaultness of the package. I think there is an issue with javac itself whenever type parameters are used.
For eg: Use this testcase in a default package
######
class AbstractY {
public void method() {}
}
public class Y extends AbstractY {
public static void main(String[] args) {
printMethods(Y.class);
}
private static void printMethods(Class<?> clazz) {
System.out.println("Methods for class: " + clazz.getName());
for (Method m : clazz.getMethods()) {
if (m.getName().startsWith("method")) {
System.out.println(m);
}
}
}
}
########
Run with javac and ecj. ecj prints AbstractY.method(), but javac prints Y.method().
I will do some investigations and update.
(In reply to comment #15) > I will do some investigations and update. Thanks, if this isn't a regression relative to 3.7.1, please open a follow up defect. I verified that the scenario specified in comment#14 is not a regression. i.e for the default package case, we now conform with java7. For the named package case we differ from javac7, but not from 3.7.1 (i.e there is no regression). (In reply to comment #15) > Some more update: > I don't think javac differs depends on the defaultness of the package. I think > there is an issue with javac itself whenever type parameters are used. I agree there could be more here. Bridge method generation is not specified in JLS and so there are some differences historically that we are trying to bridge ;-) The defaultness came into picture from following the reasoning for java.lang.IllegalAccessException discussed in https://bugs.eclipse.org/bugs/show_bug.cgi?id=288658 If the concerned public method belongs to a package default type but that package happens to be the default package, the visibility barrier goes away and so there is no need for a bridge. So I think the current fix is actually correct, but your testing has uncovered some more nuances where we differ. (In reply to comment #15) > Some more update: > I don't think javac differs depends on the defaultness of the package. I think > there is an issue with javac itself whenever type parameters are used. You may also want to see if the abstractness or not of the superclass has any bearing. (In reply to comment #16) > So I think the current fix is actually correct, but your testing > has uncovered some more nuances where we differ. I am not sure about this. Please look at the test case in comment 15. javac and Eclipse 3.7.1 are similar for this test case but there is a change in 3.7.2. (In reply to comment #17) > You may also want to see if the abstractness or not of the superclass has > any bearing. This doesn't have any bearing. (In reply to comment #18) > (In reply to comment #16) > > So I think the current fix is actually correct, but your testing > > has uncovered some more nuances where we differ. > I am not sure about this. Please look at the test case in comment 15. javac and > Eclipse 3.7.1 are similar for this test case but there is a change in 3.7.2. There _is_ a change in behavior for this test case, but I don't think it is a regression, it is a progression. I think what it is showing is that there is many inconsistencies in javac's bridge generation algorithm and it is nowhere nearly as straightforward as as the statement "Specifically, we would generate a bridge method when a public method is inherited from a nonpublic class into a public class." in http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6342411 (In reply to comment #16) > (In reply to comment #15) > > > I will do some investigations and update. > > Thanks, if this isn't a regression relative to 3.7.1, please open > a follow up defect. I meant to say if this isn't a regression relative to 3.7 and/or 3.7.1. I think the problem is with substitution of the type parameter that the method
takes causes the problem.
###
abstract class AbstractY<T> {
public void method(T a) {}
}
public class Y extends AbstractY<String> {
}
####
javac breaks or differs in this condition. Note that the parameter that the
method 'method' takes is substituted by it's subclass. I still don't understand
completely but in my opinion ECJ3.7.1 behaviour is correct.
(In reply to comment #20) > "Specifically, we would generate a bridge method when a public method > is inherited from a nonpublic class into a public class." in > http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6342411 I agree that this is the right thing to do, but with this bug fix, there is a change in behaviour for classes in default package. I don't think that is right. As I understand from 6342411, a bridge method is being added so that things work fine with reflection. So, the defaultness of the package should not cause change in behaviour. >There _is_ a change in behavior for this test case, but I don't think it is >a regression, it is a progression. It could be either way depending upon what is the expected behaviour. What should be the behaviour of the testcase in comment 22? Should the bridge method be generated or not? BTW, this bug fix breaks (I believe it breaks and not differs) the test in comment 15. Hence, this may be a regression. (In reply to comment #16) > The defaultness came into picture from following the reasoning > for java.lang.IllegalAccessException discussed in > https://bugs.eclipse.org/bugs/show_bug.cgi?id=288658 > > If the concerned public method belongs to a package default > type but that package happens to be the default package, > the visibility barrier goes away and so there is no need for > a bridge. > > So I think the current fix is actually correct, but your testing > has uncovered some more nuances where we differ. On thinking more about this, I think I understand what Satyam is trying to say: I have been guilty of muddled thinking in treating default package as though it were a globally accessible package and claiming per earlier that the visibility barrier goes away and so there is no need for a bridge... Satyam, thanks for persisting and for putting together a test case that will show that this package barrier does exist between default and named packages that can be used a test case when we work on this further. I think we should back put this fix for 3.7.2 and restore status quo as of 3.7.1 and continue future work only for 3.8.x streams. Created attachment 209730 [details]
zip of the project
Project to show that the current behavior is wrong. Run <default>.Y in this project.
> I think we should back put this fix for 3.7.2 and restore status quo
> as of 3.7.1 and continue future work only for 3.8.x streams.
Talked to Srikanth. I agree that this is the best thing to do at this point.
(In reply to comment #26) > > I think we should back put this fix for 3.7.2 and restore status quo > > as of 3.7.1 and continue future work only for 3.8.x streams. > > Talked to Srikanth. I agree that this is the best thing to do at this point. We will shortly restore the behavior to what it was at 3.8M3/3.7.1/3.6.2 time for the respective streams by backing out the "fix", which fortunately is very small (only two lines of code change not counting tests.) Apologies for the confusion. Satyam, once again thanks for insistence/ persistence in the face of my inclination to dismiss it rather readily. Reverted from 3.7 maintenance branch via commit 52ffa6a622336b1154708c0ba10ecff9e5941c7f Pushing revert into 3.6.2+java7 branch fails with Can't connect to any repository: ssh://ajain@git.eclipse.org/gitroot/jdt/eclipse.jdt.core.git (An internal Exception occurred during push: ssh://ajain@git.eclipse.org/gitroot/jdt/eclipse.jdt.core.git: Short read of block.) Weird. :\ Reverted in 3.6.2+java7 branch via commit 6ddf29924a61a973ae1bd8add4af0942f5b5d8fb Reverted in master via commit 99e06cdf0656ed0f6c3eb604300a8baae7ee3beb This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie. Trying to take stock of this store: Example from comment 15 showed an interim regression in ecj, has been reverted to the state where we agree with javac. Example from comment 14 used to show a difference between ecj and javac, but starting with version 11 javac produces the same result as ecj. The same is true for example 0. => Apparently this wasn't a bug in ecj to begin with. |
Build Identifier: 20110218-0911 Given the following interfaces and classes: Interface.java: interface Interface<M, X> { void method(X x, M m); } Abstract.java abstract class Abstract<X> implements Interface<String, X> { @Override public void method(X x, String string) { } } PublicAbstract.java: public abstract class PublicAbstract<X> implements Interface<String, X> { @Override public void method(X x, String string) { } } AbstractChild.java: public class AbstractChild extends Abstract<Integer> { } PublicAbstractChild: public class PublicAbstractChild extends PublicAbstract<Integer> { } And the program to test it (Main): import java.lang.reflect.Method; public class Main { public static void main(String[] args) { printMethods(PublicAbstractChild.class); printMethods(AbstractChild.class); } private static void printMethods(Class<?> clazz) { System.out.println("Methods for class: " + clazz.getName()); for (Method m : clazz.getMethods()) { if (m.getName().startsWith("method")) { System.out.println(m); } } } } When I have classes compiled by Eclipse the result is: Methods for class: PublicAbstractChild public void PublicAbstract.method(java.lang.Object,java.lang.String) public void PublicAbstract.method(java.lang.Object,java.lang.Object) Methods for class: AbstractChild public void AbstractChild.method(java.lang.Object,java.lang.String) public void Abstract.method(java.lang.Object,java.lang.Object) When I have classes compiled by Oracle JDK (tested on versions 1.6.0_24 and 1.6.0_20) the result is: Methods for class: PublicAbstractChild public void PublicAbstract.method(java.lang.Object,java.lang.String) public void PublicAbstract.method(java.lang.Object,java.lang.Object) Methods for class: AbstractChild public void Abstract.method(java.lang.Object,java.lang.String) public void Abstract.method(java.lang.Object,java.lang.Object) Results are different which causes major problems using reflection. Reproducible: Always Steps to Reproduce: 1. Take the code described in the details section. 2. Compile it and run Main within Eclipse. 3. Compile it with Oracle JDK 1.6.0_2x and run Main. 4. Compare the results.