Bug 325481 - [assist] fields declared after a particular field are proposed in its initialization
Summary: [assist] fields declared after a particular field are proposed in its initial...
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.7   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.7 M4   Edit
Assignee: Ayushman Jain CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-16 11:25 EDT by Ayushman Jain CLA
Modified: 2010-12-07 03:22 EST (History)
5 users (show)

See Also:
satyam.kandula: review+


Attachments
proposed fix v1.0 + regression tests (5.27 KB, patch)
2010-09-16 14:29 EDT, Ayushman Jain CLA
no flags Details | Diff
fix for next i build (1.92 KB, patch)
2010-11-23 05:17 EST, Ayushman Jain CLA
no flags Details | Diff
regression test (6.73 KB, patch)
2010-11-24 01:02 EST, Ayushman Jain CLA
no flags Details | Diff
regression test updated (9.45 KB, text/plain)
2010-11-25 03:43 EST, Ayushman Jain CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ayushman Jain CLA 2010-09-16 11:25:12 EDT
For the following cases:

public Test {
    String field1 = "";
    String field2 = String.format(fie|<CTRL-SPACE here>
    String field3 = "hello";
    String field4 = "";
}


I get proposals for field2 (the one being declared) , as well as field3 and field4 (that are declared after field2). 

All these proposals should be removed. Bug 310427 fixed the case when the initialization was being done by simple assignment to another field. But this case where the assist node parent is not the field itself, still needs to be handled
Comment 1 Ayushman Jain CLA 2010-09-16 14:29:01 EDT
Created attachment 179057 [details]
proposed fix v1.0 + regression tests

Here's the patch that fixes the completion engine's find fields method. It first determines whether we are inside a field's initializer while invoking content assist, and if we are it records that field's id. Next, every time a field is considered to be proposed, we make sure that its id is less than the recorded id. (meaning that it has been declared before the field being declared currently)

Note that with this, we dont need the fix for 310427, since it works irrespective of whether the field is being initialized by assigning to another field or some other way.
Comment 2 Ayushman Jain CLA 2010-09-16 14:31:16 EDT
Frederic, please review.
This fix takes the condition field.initialization != null to check whether content assist is being asked inside the field's initialization. This is consistent with your observation on bug 312603#c2 .

TIA
Comment 3 Ayushman Jain CLA 2010-09-20 07:26:02 EDT
Satyam, please review since Fred's not available. Thanks!
Comment 4 Satyam Kandula CLA 2010-09-21 07:44:27 EDT
One minor suggestion: The if's can be combined - You can choose to ignore :). 
Otherwise the patch is good. +1.
Comment 5 Ayushman Jain CLA 2010-09-22 02:33:05 EDT
Released in HEAD for 3.7M3. Combined the if's according to comment 4
Comment 6 Srikanth Sankaran CLA 2010-10-26 01:29:42 EDT
(In reply to comment #5)
> Released in HEAD for 3.7M3. Combined the if's according to comment 4

Deja vu all over again ? 

public class Test {
    public static void main() {
        String field1 = "";
        String field2 = String.format(fie|);
        String field3 = "hello";
	String field4 = "";
    }
}

Completing at | proposes field2 which proposal if you accept
you get an immediate compiler error.
Comment 7 Srikanth Sankaran CLA 2010-10-26 01:31:25 EDT
Verified for 3.7 M3 using Build id: I20101025-1800.

Ayush if you agree the scenario in comment 6 is a
bug, please raise a follow on defect.
Comment 8 Ayushman Jain CLA 2010-10-26 02:07:45 EDT
(In reply to comment #7)
> Verified for 3.7 M3 using Build id: I20101025-1800.
> 
> Ayush if you agree the scenario in comment 6 is a
> bug, please raise a follow on defect.

Raised bug 328674. Not reopening this since the case for fields is fixed. The failing case is for locals.
Comment 9 Dani Megert CLA 2010-11-23 03:59:40 EST
Ayush, this fix is not good it causes major bug 330842. Please revert for today's I-build and work on a better fix.
Comment 10 Ayushman Jain CLA 2010-11-23 04:58:42 EST
(In reply to comment #9)
> Ayush, this fix is not good it causes major bug 330842. Please revert for
> today's I-build and work on a better fix.

This shows us one case in which the observations made in bug 312603#c2 dont hold true. Since the type from which proposals are requested is in a different compilation unit, SourceTypeConverter is used to parse it and obtain a CU. And this parsing is done through the parser, not CompletionParser. Hence we cannot conclude in all cases that consumeExitVariableWithInitialization() will always set the FieldDeclaration$initialization to null.
Comment 11 Ayushman Jain CLA 2010-11-23 05:17:18 EST
Created attachment 183654 [details]
fix for next i build

This patch fixes the issue in this case. I'll add another test later.
Comment 12 Ayushman Jain CLA 2010-11-23 05:23:09 EST
Released in HEAD for 3.7M4
Comment 13 Ayushman Jain CLA 2010-11-23 05:24:44 EST
*** Bug 330842 has been marked as a duplicate of this bug. ***
Comment 14 Dani Megert CLA 2010-11-23 05:58:24 EST
(In reply to comment #12)
> Released in HEAD for 3.7M4

It doesn't seem to be tagged yet for the I-build.
Comment 15 Ayushman Jain CLA 2010-11-24 01:02:09 EST
Created attachment 183729 [details]
regression test

Released in HEAD.
Comment 16 Ayushman Jain CLA 2010-11-25 03:43:29 EST
Created attachment 183833 [details]
regression test updated

Reverted the above test patch.

The above regression test added some new files due to which a couple of other tests had to be altered. Released in HEAD.
Comment 17 Srikanth Sankaran CLA 2010-12-07 03:22:27 EST
Verified for 3.7M4 using build id I20101205-2000