Bug 252379 - Organize imports deletes needed static import.
Summary: Organize imports deletes needed static import.
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.4.1   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.6 M5   Edit
Assignee: Jay Arthanareeswaran CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-10-28 10:54 EDT by Brian Miller CLA
Modified: 2010-03-01 14:30 EST (History)
6 users (show)

See Also:
Olivier_Thomann: review+

Attachments
Patch for the problem (2.04 KB, patch)
2010-01-06 09:13 EST, Nanda Firdausi CLA 		no flags Details | Diff
Updated patch with regression test (6.21 KB, patch)
2010-01-11 03:41 EST, Jay Arthanareeswaran CLA 		Olivier_Thomann: iplog+
Details | Diff
Proposed fix (6.49 KB, patch)
2010-01-15 13:17 EST, Olivier Thomann CLA 		no flags Details | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Miller CLA 2008-10-28 10:54:24 EDT 
Build ID: M20080911-1700

Steps To Reproduce:
Please Organize Imports on class Bug.  See the result is illegal since a needed static import is wrongly removed.

More information:
--------------- bug/Bug.java -------------
package bug;
import static bug.CaseType.all;
import static bug.ShareLevel.all;
import org.eclipse.jface.wizard.Wizard;
import org.eclipse.jface.wizard.WizardPage;
import org.eclipse.swt.widgets.Composite;
import bug.CaseType;
import bug.ShareLevel;
class Bug{
	void run(Wizard wizard){
		wizard.addPage(new WizardPage("pickCase",null,null){
			public void createControl(final Composite parent){
				for(CaseType cat:all())
					cat.hashCode();
				class Share{
					Share(final ShareLevel sharing){
						sharing.hashCode();
					}
				}
				new Share(all);
			}});
	}
}

---------------------- bug/CaseType.java ------------
package bug;
enum CaseType{
	one;
	static CaseType[]all(){return null;}
}

---------------- bug/ShareLevel.java ----------------
package bug;
enum ShareLevel{all}
Comment 1 Dani Megert CLA 2008-10-28 11:47:28 EDT 
Looks like a bug in the org.eclipse.jdt.core.dom.rewrite.ImportRewrite.

Paste the following to get the test case quicker:

package bug;
import static bug.CaseType.all;
import static bug.ShareLevel.all;
import bug.CaseType;
import bug.ShareLevel;
class Bug{

 public void createControl(){
	 
                                for(CaseType cat:all())
                                        cat.hashCode();
                                class Share{
                                        Share(final ShareLevel sharing){
                                                sharing.hashCode();
                                        }
                                }
                                new Share(all);
                        };
}
package bug;
enum CaseType{
        one;
        static CaseType[]all(){return null;}
}
package bug;
enum ShareLevel{all}
Comment 2 Nanda Firdausi CLA 2010-01-06 09:13:13 EST 
Created attachment 155406 [details]
Patch for the problem
Comment 3 Olivier Thomann CLA 2010-01-06 09:25:55 EST 
Jay, please review the patch and add regression tests.
Comment 4 Jay Arthanareeswaran CLA 2010-01-11 03:41:47 EST 
Created attachment 155715 [details]
Updated patch with regression test

The first patch looks alright. Ideally it would have been better to have the static import comparison in the ImportRewrite#compareImport(). However, I do see that we will have to move few things around. So left it that way and made one minor change (a variable name change) and added a regression test.
Comment 5 Olivier Thomann CLA 2010-01-15 13:17:55 EST 
Created attachment 156258 [details]
Proposed fix

In the line:
(this.importsKindMap.get(curr.substring(1)).equals(this.importsKindMap.get(qualifier + "." + name)))),
is it sure that this.importsKindMap.get(curr.substring(1) cannot return null ?

I don't like to see 's' used for a prefix instead of using the constant field STATIC_PREFIX.
Also all strings should be either externalized or tagged with the NON-NLS tag.

I slightly change the patch. Jay, let me know whay you think.
If everything is ok, I'll release early next week.
Comment 6 Olivier Thomann CLA 2010-01-21 11:45:52 EST 
Released the last patch for 3.6M5.
Added regression test:
org.eclipse.jdt.core.tests.rewrite.describing.ImportRewriteTest#testBug252379
Comment 7 Satyam Kandula CLA 2010-01-25 02:15:09 EST 
Verified for 3.6M5 using Build id: I20100122-0800
Comment 8 Srikanth Sankaran CLA 2010-01-25 02:18:29 EST 
Verified.