Community
Participate
Working Groups
from bug 111882 comment 0 Array type should be filtered while completing in case of a switch class X { static char[] AN_ARRAY = new char[10]; static int AN_INT_VALUE = 0; static int ANOTHER_VALUE = 1; void foo(int i) { switch (i) { case AN // <-- complete after the 'AN' } } } AN_ARRAY shouldn't be proposed.
Currently proposals which are not compatible with the expected type are proposed because an indirect access to a valid proposal is still possible. But when the type is an array there is no possible indirect proposal, so we could filtered these proposals. We could add the same behavior for final classes and types with no compatible members.
Created attachment 170505 [details] proposed fix v1.0 + regression tests This patch does the following two things: 1) It inhibits array types from showing up in proposals inside case expressions 2) Since case expressions must be constant expressions, it bumps up the priority of final variables and fields by 1 so that they always show up at the top of the list. (1) is achieved by introducing a new boolean CompletionEngine$assistNodeIsInsideCase. If the proposal being considered is an array type and this flag is set, it is filtered off. (2) is achieved by a new relevance constant RelevanceConstants$R_FINAL. This gets added to the relevance computation of final variables and fields when completing inside a case expression.
(In reply to comment #1) > We could add the same behavior for final classes and types with no compatible > members. (1) Final classes shouldnt be filtered off. Consider the following scenario: ----------8<------------------------------ final class TryFinal { public static final int abc= 1; } public class TryMe { static final int TANOTHER_VALUE = 1; final class TryFinalInner { public static final int abc2= 1; } void foo(int i) { switch (i) { case T: System.out.println();// <-- complete after the 'AN' } } } The proposals TANOTHER_VALUE, TryFinal.abc and TryFinalInner.abc2 are all correct. Hence final classes shouldnt be left out. (2) Types with no compatible member types can be filtered off. But this will involve going through all the types defined and then going through each member, which will be very type consuming if either, or both of these are large in number. Adding such time complexity to content assist could result in a lot of chaos. So IMHO, filtration should not to be done so deeply.
Comments on the patch : (1) In the following example, class X { static char[] AN_ARRAY = new char[10]; static int AN_INT_VALUE = 0; static int ANOTHER_VALUE = 1; void foo(int i, int [] AN_ARGUMENT_ARRAY) { switch (i) { case AN:break; } } } AN_ARGUMENT_ARRAY is proposed, I would have expected it to be suppressed pretty much like AN_ARRAY was suppressed. Likewise for locals. (2) I would suggest the setting of this.assistNodeIsInsideCase happen only in the relevant situations by invoking a new method pretty much along the lines org.eclipse.jdt.internal.codeassist.CompletionEngine.assistNodeIsExtendedType(ASTNode, ASTNode) is used. (3) Should R_FINAL be something other than just a boost by 1 ? It sounds like a very small boost given that any non final things we propose there will cause a compile error. We don't want this to be very that this comes to dominate either. (4) dont ==> should be don't in comments. Otherwise eclipse draws a line line indicating a spelling error and it is distracting needlessly. Likewise arent ==> aren't proriority ==> priority
Created attachment 170675 [details] improved fix + regression tests incorporated the review comments into this new patch. Points 1 and 4 from comment 4 have been taken care of as is. point 2 - setting of the new flag CompletionEngine$assistNodeIsInsideCase has been moved to individual completion functions for single name ref, qualified name ref, single type ref and qualified type ref. This hasnt been done for parameterized type ref because inside a case expression they wont be used. point 3 - bumped up the relevance of the new constant R_FINAL to 3 from 1. This should suffice to bring the constants on top of the list in this case. If we come across a case in the future when a non-final variable still appears with a greater relevance for some reason, we can increase the relevance number further.
(In reply to comment #5) > Created an attachment (id=170675) [details] > improved fix + regression tests > > incorporated the review comments into this new patch. I gave this patch a quick once over and it looks fine. Just a couple of comments: for (int i = 0; i < ((SwitchStatement) astNodeParent).caseCount; i++) { is better coded as for (int i = 0, caseCount = ((SwitchStatement) astNodeParent).caseCount; i < caseCount; i++) { This should have been pointed out with the earlier patch itself. Sorry, I missed it.
Created attachment 172318 [details] updated patch updated patch to incorporate comment 6 and updated for HEAD.
Released in HEAD for 3.7M1. Added tests: CompletionTests.testBug195346a() CompletionTests.testBug195346b()
Verified for 3.7 M1 using build I20100802-180
