Community
Participate
Working Groups
BETA_JAVA8: Java 8 allows code carrying methods in interfaces in the form of static methods (not inherited by subtypes even though public; They can be accessed only statically using the declaring interface's type reference.) and default instance methods. There are various issues in completing on these method names as shown by the snippet below: | indicates cursor position. // --- interface I { static void staticMethod() {} default void defaultMethod() { stat| // no proposals here } } public class X implements I { public void foo(I i) { i.defaultMethod(); // proposal for defaultMethod offered here I.stat| // no proposals here. this.d| // no proposals here. } }
Anirban, please follow up. TIA.
Hello, I'm working on this. Expect a patch within a week. Thanks Anirban
Hello, Still working on this. Some issues with the regression. Will take a few more days. Thanks Anirban
Created attachment 229209 [details] Uploading patch for review Hello, Uploading patch for review. This has some regression failure though. Thanks Anirban
Created attachment 229258 [details] Applicable regression clean patch Hello, Uploading new regression clean patch for review. Thanks Anirban
Here are the review comments: (1) MessageSend.java: Remove the check for receiver being null. As can be seen from other usages of this.receiver in this file, receiver field can never be null. (2) CastExpression: receiverIsImplicitThis() should chain to invocation site and not return false. See how isSuperAccess is implemented in the same place. (3) Same comment for Scope.java (4) Please remove all formatting changes introduced in CompletionEngine.java We don't want to reformat code unaltered by a fix. (5) The patch generates additional compiler warnings. Please address all of these warnings. After these are addressed, please post a tested patch and I'll review one more time.
Hello Srikanth, Thanks for the comments#6. 1-3 : will do I need some clarifications about (4) and (5) though. >> (4) Please remove all formatting changes introduced in CompletionEngine.java We don't want to reformat code unaltered by a fix. These are not intended to be meaningless formatting. This code reorganization is supposed to implement functionality to solve a requirement in this bug. With the 'default' function definition in an interface, a class hierarchy, implementing the interface, can completely do away with the ever implementing the method. This was not the case earlier. Any method declaration in an interface had to be implemented somewhere in the class hierarchy. The suggestions were being made by only the function definitions in the class hierarchy. But, with the advent of 'default' definitions, the class hierarchy need not define the method, but still expect the same in the suggestions. Hence the super-interfaces has to be searched for default methods. If you look at the code reorganization closely, you'll see that previously the if block was completely bypassing the 'findInterfaceMethods(...)' call, if it not abstract, etc. But now, this call is not getting bypassed completely, but, if the conditions are do not satisfy, the calls is done only to find the default methods (getOnlyDefaultMethodsOfInterface == true). Thus this gets the default methods too in the suggestion. Please let me know if you need further clarification. >> (5) The patch generates additional compiler warnings. Please address all of these warnings. I ran the regression and it came clean. I thought, that this means both error, warnings and other outputs are matching. I don't understand what I'm missing, but if you may please provide any testcase or run scenarios, where I can see excess warnings, I'll be happy to clean them. Thanks Anirban
(In reply to comment #7) > These are not intended to be meaningless formatting. This code > reorganization is supposed to implement functionality to solve a requirement > in this bug. Anirban, Why is this code block: if (hasPotentialDefaultAbstractMethods && (currentType.isAbstract() || currentType.isTypeVariable() || currentType.isIntersectionType() || currentType.isEnum())){ changed to: if (hasPotentialDefaultAbstractMethods && (currentType.isAbstract() || currentType.isTypeVariable() || currentType.isIntersectionType() || currentType.isEnum())) { ?? Likewise there are other changes. Please go through every diff in this file in the comparator and eliminate noise changes that are not material to the fix. > >> (5) The patch generates additional compiler warnings. Please address all of > these warnings. > I ran the regression and it came clean. Please look in the problem markers view. Your patch generates additional warnings in CompletionTests.java. This has nothing to do with regression tests.
Created attachment 229949 [details] Patch accommodating review comments. Patch accommodating review comments.
Review comments: (1) Please use the same code formatting conventions as you see used in the project: if(condition) ==> if (condition) (2) The changes in CompletionEngine look pretty complicated and can stand for simplification. Basically, in the existing code, there is a short circuiting of searching in super interfaces for methods when the current type is concrete. This is because a concrete type for it to compile successfully must implement all interface methods and so search of the type should provide us with all the completion proposals as it is and searching superinterfaces cannot augment the set of proposals in any way. Not so with Java8. A concrete type may not implement default methods of its superinterfaces and so the methods it implements may not be a superset of methods contracts of its super interfaces ==> For Java 8, super interfaces MUST always be searched in the present scheme of things or else we will miss some proposals. Given this observation, we can simplify the fix quite bit.
Created attachment 230642 [details] Alternate simpler fix Anirban, please take a look and let me know if you agree.
(In reply to comment #10) > (2) The changes in CompletionEngine look pretty complicated and can stand > for simplification. Basically, in the existing code, there is a short > circuiting of searching in super interfaces for methods when the current > type > is concrete. This is because a concrete type for it to compile successfully > must implement all interface methods and so search of the type should > provide > us with all the completion proposals as it is and searching superinterfaces > cannot augment the set of proposals in any way. See that this short circuiting is rendered ineffective by the patch in comment#9.
Hello Srikanth, I agree with the patch. Thanks Anirban
Fix and test released here: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=7c6fe4f21cb5ba3a5ee61e01b5b0b2742e043690 Thanks Anirban.
