Community
Participate
Working Groups
I20100424-2000 FUP of 236306 (1) public class X { public static void main(String args[]) { int xyk = 10; int xyz = xy|; } } Completing at the | symbol proposes xyz, which would immediately lead to an error (uninitialized) (2) public class X { int xyk = 0; int xyz = xy|; int xyp = 1; } Completing at the | symbol proposes xyz and xyp, which would immediately lead to an error (field referenced b4 definition) (3) String newText = String.format(newText, null) proposal still shows up if newText declaration were to happen as a field and not as local.
Created attachment 168132 [details] proposed fix v1.0 + regression tests In the completion engine, we record the fields and local variables inside which completion is taking place in CompletionEngine$uninterestingBindings. We can simply use this to eliminate the recorded fields and locals from being proposed. This is what this patch does. Remastered tests: CompletionTests#testCompletionEmptyTypeName2() CompletionTests#testCompletionEmptyTypeName3() CompletionTests#testCompletionKeywordFalse5() Added regression tests: CompletionTests#testBug310427a() CompletionTests#testBug310427b()
(In reply to comment #0) Note that this only fixes cases (1) and (2). Case (3) is a different case where we are looking for argument proposals after content assist has added its proposed method and is more like what was fixed in bug 236306. So i'll raise a new bug pertaining to case (3).
(In reply to comment #2) > (In reply to comment #0) >So i'll raise a new bug pertaining to case (3). Raised bug 312603
Created attachment 168427 [details] Proposed fix + regression test I change the patch a little. Otherwise it looks good.
Targetting 3.7M1
Released in HEAD for 3.7M1. Added tests: CompletionTests#testBug310427a() CompletionTests#testBug310427a()
Reopening as build I20100802-1800 fails the case 2 in comment#0. I don't also see a regression test that ensures that we don't propose a field ahead of its declaration.
(In reply to comment #7) > Reopening as build I20100802-1800 fails the case 2 in comment#0. > I don't also see a regression test that ensures that we don't > propose a field ahead of its declaration. I'll take a look.
Moving to M2.
Created attachment 175971 [details] proposed fix for case 2 + test Srikanth please review. TIA
(In reply to comment #10) > Created an attachment (id=175971) [details] > proposed fix for case 2 + test > > Srikanth please review. TIA This patch still allows ahead of declaration use of variables if the variables are locals to a method. Try completing at | public class X { int xyk = 0; int xyz = xyk; int xyp = 1; public void foo() { int xyk = 0; int xyz = xy|; int xyp = 1; } }
(In reply to comment #11) [...] > Try completing at | > > public class X { > int xyk = 0; > int xyz = xyk; > int xyp = 1; > > public void foo() { > int xyk = 0; > int xyz = xy|; > int xyp = 1; > } > } On completing at the specified location, we get the following proposals: FIELDS - xyk, xyz, xyp LOCAL VARS - xyk So the proposal you see for xyp is for the field with the same name and hence valid. Also you do get a proposal for the field xyz, but inserting it would give an error since the local var xyz is in the process of declaration. However, this error can be removed by just qualifying it as this.xyz if this was the original intention. So proposal xyz is also valid.
(In reply to comment #12) > (In reply to comment #11) > [...] > > Try completing at | > > > > public class X { > > int xyk = 0; > > int xyz = xyk; > > int xyp = 1; > > > > public void foo() { > > int xyk = 0; > > int xyz = xy|; > > int xyp = 1; > > } > > } > > On completing at the specified location, we get the following proposals: > FIELDS - xyk, xyz, xyp > LOCAL VARS - xyk You are right. Apologies for the false alarm. A couple of comments: (1) Please comment out the line inside the junit type's static initializer block. TESTS_NAMES = new String[] { "testBug310427c"}; (2) You could have used the check field.id >= fieldDeclaration.binding.id instead and gotten rid of the uninterested bindings approach (at least for fields). As it is there are two separate techniques used to filter out the invalid proposals - the fix may be simpler with just one instead. It is your call to decide whether you want to do something about (2).
Created attachment 176145 [details] proposed fix for case 2 + test with small changes >(1) Please comment out the line inside the junit type's static >initializer block. Done (2) You could have used the check field.id >= fieldDeclaration.binding.id instead and gotten rid of the uninterested bindings approach (at least for fields). As it is there are two separate techniques used to filter out the invalid proposals - the fix may be simpler with just one instead. Done.
(In reply to comment #14) > Created an attachment (id=176145) [details] > proposed fix for case 2 + test with small changes Patch looks good.
Released in HEAD for 3.7M2.
Verified for 3.7M2 using build I20100909-1700
