Community
Participate
Working Groups
Build ID: M20070212-1330 Steps To Reproduce: 1. Compile the following code with the Eclipse compiler and Sun’s compiler (1.4.2_11). 2. Run Test in both environments. The Eclipse env will produce a stack overflow while the Sun environment will produce “set(1)”. 3. In the eclipse env, put a breakpoint in Int.set(long) to see what it’s doing. NOTE: I found this in 3.2.2, but I can't choose this as the version found in. public interface Num { void set(int value); void set(long value); void set(double value); } public abstract class Int implements Num { // Uncomment out the next line as a work-around //public abstract void set(int value); public void set(long value) { set((int)value); } public void set(double value) { set((int)value); } } public class Test extends Int { public void set(int value) { System.out.println("set(" + value + ")"); } public static void main(String[] args) { Test test = new Test(); test.set(1L); } } More information: "void set(double)" must be defined to reproduce the bug. This can also be "void set(float)", but not "void set(short)". Workaround is to define "void set(int)" as an abstract method in Int
Reproduced with I20070205-1824.
I believe that ClassScope#findMethod should, for abstract classes, collect methods for implemented interfaces as well before narrowing the candidates list.
Created attachment 59468 [details] Suggested fix + test cases Kent, could you please review the suggested changes?
I think we should duplicate the code at lines 1126-1127: if (isCompliant14 && (receiverType.isAbstract()... return findDefaultAbstractMethod(receiverType, selector, argumentTypes... and do it AFTER we have decided which method is the mostSpecificMethod. We only have a problem when there is more than 1 valid match. When there is a single match in the abstract class Int, we find the interface method correctly.
Kent, I believe that if we wait until a most specific method is found, then set(double) will get elected and we won't find set(int). Hence my approach that adds a few methods at the end of the candidates in the only cases where the receiver is abstract. My fix is amendable anyway since I miss the case when the interface is implemented by another class is in the receiver hierarchy. I'll make a new attempt asap.
Created attachment 59545 [details] Fix + test cases Covers deeper hierarchy cases.
I don't buy it. When there is only 1 choice, we still walk the interfaces and find set(int). So why would it be any different if we picked the mostSpecific and then searched the interfaces. Look at the places we call findDefaultAbstractMethod() from findMethod().
(In reply to comment #7) > I don't buy it. > > When there is only 1 choice, we still walk the interfaces and find set(int). Looking at the latest call to findDefaultAbstractMethod (not the one guarded by 1.4 compatibility), I can see what you mean: it checks if it gets an exact match, then if and only if it doesn't it calls findDefaultAbstractMethod. So we could do that. Could you please elaborate on the advantages of doing so?
We want to avoid searching interfaces if we can. Prior to 1.4, default abstract methods were added to any class that implemented an interface & didn't provide its own implementation of each method. With this change, do we need any calls to findDefaultAbstractMethod ? If every branch thru the method requires searching the interfaces, then can we removed all calls to findDefaultAbstractMethod ?
(In reply to comment #9) > We want to avoid searching interfaces if we can. I would only search interfaces for abstract classes. > With this change, do we need any calls to findDefaultAbstractMethod ? My change only targets abstract classes, hence the other cases when we currently call findDefaultAbstractMethod would continue to do so. One concern I have with the separate call on findDefaultAbstractMethod for the case at hand is that the condition under which we need to call it is quite complex. We cannot limit ourselves to the case where there is a most specific method that is not an exact match. There are also cases where we have an invalid method returned by mostSpecificMethod that should still trigger findDefaultAbstractMethod (see test084 in the next attachment). This may trigger a few calls to findDefaultAbstractMethod that we would rather avoid. I also feared that separating the methods coming from inherited classes and from interfaces would hurt, but thinking more about it, I could not come up with any case that would support that. I'll craft a patch using findDefaultAbstractMethod and we'll discuss the respective merits of both approaches then.
Created attachment 59636 [details] Tests cases (augmented) Adds test084.
Created attachment 59643 [details] Alternate fix (using findDefaultAbstractMethod) Note that we cannot rely on the match test to avoid the call, since it won't reject set(double).
My preference goes to the patch of comment #6, which I'll release if there is no objection.
I'll get back to you today
Created attachment 60024 [details] Suggested change
Assuming that deferring the interfaces walk as late as possible is always desireable, we would go with your patch. I still consider that since in the case of abstract classes we will walk their interfaces anyway, we have the choice to walk them upfront or later (my patch made no effort to avoid double walk hence it would have to be reworked anyway). Making an informed decision here would involve sorting out if a single pass on the complete collection is faster than two separate passes on partial collections, but I am unsure the question deserves so much more time. Having said that, I have three further suggestions: - receiverType.isInterface() is called multiple times throughout the method, including as an initialization value for mustBePublic; I would initialize receiverTypeIsInterface once and for all and use it everywhere; - at the very beginning of the method, I would emphasize the role of the (unchanged) receiverType over currentType and replace the 'current*' pattern by a 'receiver*' pattern in the first if statement; - the comment for the 'if (candidatesCount == 0)' statement is inaccurate, since we will grab an exact match for some abstract classes. I will attach a patch that reflects those. Please let me know what you think.
Created attachment 60148 [details] Suggested fix: Kent's + changes suggested above Note: must use patch 59636 for the test cases.
looks good as for the single walk vs. 2 walks - findMethod() is called all the time. We can afford to look at changes if they'll have a performance benefit. How many calls to findDefaultAbstractMethod() can be removed if we add interface methods upfront? Do we slow down the hierarchy walk if more methods are in the found collection? Or do we not add interface methods because we found an implementation for them already?
Created attachment 60207 [details] Single walk We could try this except that we need to find out why we're failing 2 tests in GenericTypeTest (#137 & #316).
Created attachment 60210 [details] Working single walk Fixes 2 failures in GenericTypeTests
I ran performance tests today that gave a slight advantage to https://bugs.eclipse.org/bugs/attachment.cgi?id=60148&action=view over https://bugs.eclipse.org/bugs/attachment.cgi?id=60210&action=view, so I will release the former if Kent is OK with that. (Methodology was: magnify findMethod by repeating calls to its implementation, use FullSourceWorkspaceBuildTests#testFullBuildDefault, repeat ten times and collect results.)
its fine with me
Released for 3.3 M6.
Verified for 3.3 M6 using build I20070320-0010
