Community
Participate
Working Groups
HEAD. The following test case, when run in ImportRewriteTest, fails public void testAddImportsX() throws Exception { IPackageFragment pack1= this.sourceFolder.createPackageFragment("pack1", false, null); StringBuffer buf= new StringBuffer(); buf.append( "package pack1;\n" + "\n" + "import java.util.*; // test\n" + "import java.util.Map.Entry;\n" + "//comment 2\n" + "import java.util.Map.SomethingElse;\n" + "// commen 3\n" + "\n" + "public class C {\n" + " public static void main(String[] args) {\n" + " HashMap h;\n" + "\n" + " Map.Entry e= null;\n" + " Entry e2= null;\n" + "\n" + " PrintWriter pw;\n" + " System.out.println(\"hello\");\n" + " }\n" + "}"); ICompilationUnit cu= pack1.createCompilationUnit("C.java", buf.toString(), false, null); String[] order= new String[] { "java", "java.util", "com", "pack" }; ImportRewrite imports= newImportsRewrite(cu, order, 1, 1, true); imports.setUseContextToFilterImplicitImports(true); imports.addImport("java.io.PrintWriter"); apply(imports); buf= new StringBuffer(); buf.append( "package pack1;\n" + "\n" + "import java.io.*;\n" + "\n" + "import java.util.*;\n" + "import java.util.Map.Entry;\n" + "\n" + "public class C {\n" + " public static void main(String[] args) {\n" + " HashMap h;\n" + "\n" + " Map.Entry e= null;\n" + " Entry e2= null;\n" + "\n" + " PrintWriter pw;\n" + " System.out.println(\"hello\");\n" + " }\n" + "}"); assertEqualString(cu.getSource(), buf.toString()); } This is because of bad calculation of comment ranges before and after an import in org.eclipse.jdt.internal.core.dom.rewrite.ImportRewriteAnalyzer.addExistingImports(CompilationUnit) lines 281 - 297. This blows up in org.eclipse.jdt.internal.core.dom.rewrite.ImportRewriteAnalyzer.getResultingEdits(IProgressMonitor) line 775
Sorry forgot to put the problematic output. Here it is: Rama Satyam V Kandula: import java.util.*; // test import java.util.Map.Entry;methingElse; // commen 3 //comment 2 import java.util.Map.SomethingElse; // commen 3 This is bad and should be fixed.
To reproduce this in the Editor (thanks Satyam for the steps): 1. Have the given code i.e. class C in the editor. 2. In Preferences, set the organize imports options so that the thresholds for folding into * are 1. 3. On PrintWriter pw, chose the quick fix to add import for java.io.PrintWriter. 4. The code is broken.
Created attachment 214162 [details] proposed fix v1.0 + regression tests The patch correct some incorrect calculations for the comments leading and trailing an import. Previously, the comments range of next import was being added as the comments range for current import. Also corrected through a new variable(nextLineOffset) is (1)the length of the import source range so that it excludes leading comments of next import and (2)the length of a dummy import for comments to exclude the leading comments of next import. (This is because the 'nextOffset' is the start of the next import but nextLineOffset is simply the start of the next line after the current import.)
Created attachment 214163 [details] jdt ui tests Added a couple of tests to ImportOrganizeTests as well.
Srikanth, this will need your +1 for backport
Change looks good to me.
Dani, this needs your +1 for backport
Thanks for the review Olivier!
(In reply to comment #5) > Srikanth, this will need your +1 for backport. As comment#1 shows, this bug results in corruption/malformation of what was originally proper source code, hence this fix should be backported. Thanks for the review Olivier. Ayush, please proceed with backport (Dani has already set the target to 3.6.2+java7)
The fix is not good. It changes the location of comments and also fails a JDT UI test. Test Case: Organize imports for: package bugs; /* * I must not be moved! */ import java.io.Serializable; public class BUG implements Serializable { } ==> the source should be left unchanged but with the patch I get: package bugs; import java.io.Serializable; /* * I must not be moved! */ public class BUG implements Serializable { }
(In reply to comment #10) > The fix is not good. It changes the location of comments and also fails a JDT > UI test. > > Test Case: > Organize imports for: > > package bugs; > /* > * I must not be moved! > */ > import java.io.Serializable; > > public class BUG implements Serializable { > } > > ==> the source should be left unchanged but with the patch I get: > > package bugs; > import java.io.Serializable; > /* > * I must not be moved! > */ > > public class BUG implements Serializable { > } This is a slightly modified version of a pre-existing problem, which only now shows up because I included the first comment also in the replace range being considered. See that the following example is already broken prior to this patch: /* * I must not be moved! */ import java.awt.List; /* * I must not be moved! */ import java.io.Serializable; /* * I must not be moved! */ import java.util.HashMap; public class Snippet implements Serializable { void foo() { List l = new List(); HashMap<String, String> h = new HashMap<String, String>(); } } When you organize imports, all the comments get grouped together at the end. The problem arises because of the fact that org.eclipse.jdt.internal.corext.codemanipulation.OrganizeImportsOperation.createTextEdit(IProgressMonitor) starts the import rewriting by setting the restoreExistingImports parameter to false. This means that the rewrite first removes all imports and then adds them back again, irrespective of whether some of them is to be removed or all are to be preserved. Our mechanism obviously fails because all the imports are re-added and we don't know whether they are new imports or an already existing import is being re-added. Hence, the grouping of the comments at the end. I'm looking at how to fix our mechanism to accomodate this.
(In reply to comment #11) > /* > * I must not be moved! > */ > import java.awt.List; > /* > * I must not be moved! > */ > import java.io.Serializable; > /* > * I must not be moved! > */ > import java.util.HashMap; Ayush, we don't have to fix this case. It is OK, if we could stop moving the comment before the first import. Look at bug 24804 comment 29.
Created attachment 214666 [details] proposed fix v1.1 + regression tests This patch takes care ONLY of comment 10. However, there's no guarantee of preserving the comments between 2 imports. I have a fix for that but will attach in a separate bug. As noted in comment 12, that doesn't need to be fixed immediately or for the backport. In this patch, I've reverted to the old way of not including leading comments of the first import in the replace range. Dani, please check if this patch is ok. Thanks
Created attachment 214667 [details] jdt ui tests v2 Updated the new jdt ui tests I added.
Created attachment 214680 [details] proposed fix v1.2 + regression tests This patch just makes 2 minor changes pointed out by Satyam: 1. The leading comments are always preserved, even a new import is only added after them. i.e. // comment 1 java.util.Map; If i add java.io.* to the above, this will change to //comment 1 java.io*; java.util.map; and NOT to java.io*; //comment 1 java.util.map; See ImportRewriteTests.testBug376930_2 for example 2. Removed a redundant if condition I ran org.eclipse.jdt.ui.tests.AutomatedSuite and all tests pass.
Created attachment 214682 [details] proposed fix v1.3 + regression tests The point 2 in the above comments still remained somehow in the last patch. Git didn't update my patch or something. So here's the final patch
Patch looks good to me.
Dani, Please review for the backport. Thanks.
(In reply to comment #18) > Dani, Please review for the backport. Thanks. -1. It still fails the test case in comment 10. With the latest fix it no longer re-orders the comment but it still changes the file. That's not acceptable.
(In reply to comment #19) > (In reply to comment #18) > > Dani, Please review for the backport. Thanks. > > -1. It still fails the test case in comment 10. With the latest fix it no > longer re-orders the comment but it still changes the file. That's not > acceptable. It looks like something was wrong in my workspace. I tried again several times and the patch didn't change the file anymore. I also ran the JDT UI tests and they all pass. +1 to backport.
Released in master via commit 4eaab794bf6dc1418675f45525d0cd74715d55ef
Backported to R3_7_maintenance via commit 33ce1f83edb1d6e1538bec8626c417bd76e3d1a8
Backported to R3_6_maintenance_Java7 via http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=R3_6_maintenance_Java7&id=24432bed9bb8d5fb1dd2198348d9980357da067a
Verified for 3.8 M7 using Build id: I20120430-2000
Released in 362+ via http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=R3_6_maintenance&id=8462dab82f576431859045179a5c6b834586b6f0
