Community
Participate
Working Groups
The sorting of imports according to configured "import groups" during Import Rewrite is broken; it produces inconsistent and incorrect results. The problem is with how imports not matching any defined "import group" are sorted, in the absence of a match-all ("*") import group. The current behavior (not documented in user documentation or UI, but referred to within internal code as "best match") attempts to place each unmatched import adjacent to or between the lexicographically closest import group (e.g. by placing "import java.new;" after a "java.net" import group). However good this idea may or may not sound in theory, it does not work in practice, and I have identified three different ways in which the existing implementation produces clearly undesirable results. ----- 1. An example of one of the ways in which this behavior is broken: Configured import order: java.net com.google Types referenced in source file: com.acme.BirdSeed com.acme.Dynamite com.google.Tgif java.net.Socket java.new.Bar org.linux.Kernel Output of Organize imports, given the above: import java.net.Socket; import java.new.Bar; import com.acme.BirdSeed; import com.acme.Dynamite; import com.google.Tgif; import org.linux.Kernel; The "best match" import sorting places java.new after java.net, and places com.acme before com.google, but it places java.new before com.acme within one block. This partially ordered block of imports is clearly not desirable. ----- Two other ways in which the existing behavior is clearly wrong (I can provide examples): - Because of the way the existing code buckets imports within import groups, the order of unmatched imports produced by Organize Imports is dependent upon the order of source file contents (e.g. reordering fields declared to be of various imported types will change the output of Organize Imports). - "Best match" sorting of unmatched imports sometimes places static and non-static unmatched imports (which have nothing in common and have no rational reason to be placed with each other) together in one block of imports. I have nearly completed a re-implementation of ImportRewriteAnalyzer which fixes this and several other issues. (I previously filed Bug 423202 asking either to change the existing sorting behavior or to better document it, but have since come to see that the existing behavior is broken in more ways than I had previously realized and is not worth documenting as-is.)
*** Bug 423202 has been marked as a duplicate of this bug. ***
A question for JDT experts: I have noticed that Organize Imports preserves the comments of deleted imports (by moving them the end of the imports region), but that an incremental import rewrite (e.g. as part of a refactoring) simply deletes such comments. I cannot find a justification for this difference in behavior and would like to make the behavior consistent (i.e., either preserve comments of removed imports in both cases or delete comments of removed imports in both cases) unless there is a good reason for the difference.
(In reply to John Glassmyer from comment #2) > A question for JDT experts: > > I have noticed that Organize Imports preserves the comments of deleted > imports (by moving them the end of the imports region), but that an > incremental import rewrite (e.g. as part of a refactoring) simply deletes > such comments. > > I cannot find a justification for this difference in behavior and would like > to make the behavior consistent (i.e., either preserve comments of removed > imports in both cases or delete comments of removed imports in both cases) > unless there is a good reason for the difference. Organize imports is expected to preserve the comments - Please see the related discussions in bug 24804, bug 378024. For incremental deletion, the trailing comments are deleted now - I would say we can keep the comments to keep the same behavior for incremental deletions as well. Dani: Any comments on/ issues with the above?
(In reply to Manoj Palat from comment #3) > (In reply to John Glassmyer from comment #2) > > A question for JDT experts: > > > > I have noticed that Organize Imports preserves the comments of deleted > > imports (by moving them the end of the imports region), but that an > > incremental import rewrite (e.g. as part of a refactoring) simply deletes > > such comments. > > > > I cannot find a justification for this difference in behavior and would like > > to make the behavior consistent (i.e., either preserve comments of removed > > imports in both cases or delete comments of removed imports in both cases) > > unless there is a good reason for the difference. > > Organize imports is expected to preserve the comments - Please see the > related discussions in bug 24804, bug 378024. For incremental deletion, the > trailing comments are deleted now - I would say we can keep the comments to > keep the same behavior for incremental deletions as well. > Dani: Any comments on/ issues with the above? Right, not just "can keep", but "must keep".
(In reply to Manoj Palat from comment #3) > Organize imports is expected to preserve the comments - Please see the > related discussions in bug 24804, bug 378024. I have looked at 24804 and 378024, and those bugs seem to address different issues than the one I am asking about; specifically, they address the loss and/or relocation of comments associated with preserved imports (i.e. imports present before and after the import rewrite). There is no question that the comments of preserved/non-removed imports must be preserved with their imports. What I am asking about, on the other hand, is what should be done with the comments associated with imports removed by the import rewrite. Currently, Organize Imports keeps the comments of removed imports and moves them to the end of the import region, but an incremental import rewrite discards the comments of removed imports altogether. I am wondering whether this difference in behavior is accidental.
(In reply to John Glassmyer from comment #5) > (In reply to Manoj Palat from comment #3) > > Organize imports is expected to preserve the comments - Please see the > > related discussions in bug 24804, bug 378024. > > I have looked at 24804 and 378024, and those bugs seem to address different > issues than the one I am asking about; specifically, they address the loss > and/or relocation of comments associated with preserved imports (i.e. > imports present before and after the import rewrite). > > There is no question that the comments of preserved/non-removed imports must > be preserved with their imports. > > What I am asking about, on the other hand, is what should be done with the > comments associated with imports removed by the import rewrite. Currently, > Organize Imports keeps the comments of removed imports and moves them to the > end of the import region, but an incremental import rewrite discards the > comments of removed imports altogether. I am wondering whether this > difference in behavior is accidental. Personally, I'd like them to be preserved in their (previous) relative position in both cases.
(In reply to Dani Megert from comment #6) > Personally, I'd like them to be preserved in their (previous) relative > position in both cases. Probably this has not been done before because guessing the correct position for a comment (when the associated import is removed) is not generally possible. See #17 on bug 24804. What I have done so far (based on observation of existing behavior and tests) is to relocate comments of single imports to corresponding on-demand ones, when single imports are folded into on-demands, and vice-versa, when on-demands are expanded into single imports, and to relocate comments of otherwise removed imports to the end of the import region. This much can be done predictably and consistently, at least. Alternatively, perhaps it's more elegant to do in the case of Organize Imports what is already done in the case of an incremental rewrite, and simply discard the comments associated with removed import declarations. This mirrors what is done with the javadoc and comments of types and class members removed from a compilation unit (i.e. the comments are simply discarded along with their associated elements). Is there a reason to treat the comments of import declarations differently than the comments of other Java elements in this respect?
Actually, thinking more about what I've just written, perhaps it's not even responsible to relocate comments following the folding and expansion of on-demand imports, as comments that are meaningful for an on-demand won't necessarily be meaningful for corresponding single imports, and vice-versa. Now I'm thinking that comments of removed imports should always either be moved to the end of the import region, or be discarded altogether, with the latter option being more consistent with what's done for the comments of other removed Java elements.
Change to jdt.core are here: https://git.eclipse.org/r/39615 Change to jdt.ui (fixing tests) is here: https://git.eclipse.org/r/39616
(In reply to John Glassmyer from comment #9) > Change to jdt.core are here: > https://git.eclipse.org/r/39615 > John: Thanks for the patch! I am looking at the test cases in the patch and see that you have addressed several issues/bugs in the patch. For the sake of clarity and to make the review more effective, please a) list down here what all are the changes (item-wise) that have been addressed by this patch. b) For all the bugs that you have addressed in this patch, add the bug number in the "Depends on" entry of this bug.
John: Here is the first glance comments based on test file: - The following tests are added new in the patch. However, they pass even without the rest of the patch. If they are additional tests checking the existing functionality, please document it here; else if these test new functionality please correct and repost: - testOrganizeNoImportsWithOneLineDelim - testOrganizeNoImportsWithTwoLinesDelim - testOrganizeNoImportsWithJavadoc - testNoEdits - testAddImportWithCommentBetweenImportsAndType In testAddImports_bug121428(), "package pack1;" statement is removed both in the input and the expected output. 1) Is there a reason for this removal? 2)If this is not relevant, please revert this 3) There are similar changes elsewhere in the file - please have a look at them and address as mentioned. [A concatenation as done in testPackageInfo is fine though] - A general comment : If there are non-relevant changes that comes under code cleanup, please bundle them together and upload as a separate patch if found required. - testAddImports_bug_24804_5 is removed. why?
Pasting John's gerrit commit message here for the sake of completeness Rewrite of ImportRewriteAnalyzer, fixing several issues. Fixes the following Eclipse bugs: Bug 71761 - [import rewrite] ImportRewrite should let me add explicit import to existing on demand import Bug 318437 - Organize Imports ignores Number of Imports needed for .* Bug 360789 - Organize imports changes static imports to .* even when that introduces compile errors Bug 412929 - [organize import] Adding a type results in adding a package and later does not honor order Bug 430303 - import group sorting is broken Bug 457051 - comment is discarded when reducing imports to an on-demand import Bug 457089 - imports are improperly reordered in the presence of a floating comment Some changes in behavior, which are reflected in the tests: ImportRewriteAnalyzer no longer attempts to place unmatched imports into their "best match" import groups, instead always placing all unmatched imports together in a single match-all group. The relocation and/or removal of comments associated with removed imports has been made more consistent. The behavior of removing comments has been made consistent between the two rewrite modes (incremental mode and "Organize Imports" mode): if there is a corresponding on-demand import for a removed single import (or vice-versa), comments are moved there; otherwise they are removed entirely. (This behavior is also more consistent with the removal of comments when deleting e.g. a field or a type.) Comments relocated from single import(s) to an on-demand import (or vice-versa) are now always placed on line(s) preceding the destination import, even if they had been trailing comments. This behavior makes more sense especially in cases where multiple single imports are reduced into an on-demand import or vice-versa. The scope of the fix for bug 121428 has been reduced, in that comments after a package declaration are no longer treated as file header comments. This is clearly beneficial in testBug376930_4, where the leading comments of java.util.* are no longer apparently reassigned to java.io.*. Blank lines around and between comments are now preserved, e.g. in testBug378024c_1. This preserves existing formatting and prevents comments from becoming associated with different imports. Change-Id: I1ae4edb45f7dc19f4263410038a9d023ac2fe054 Signed-off-by: John Glassmyer <jogl@google.com>
Thanks, Manoj. To answer your questions: - The five tests you mention (testOrganizeNoImportsWithOneLineDelim, testOrganizeNoImportsWithTwoLinesDelim, testOrganizeNoImportsWithJavadoc, testNoEdits, testAddImportWithCommentBetweenImportsAndType) do indeed test existing/preserved functionality. - In testAddImports_bug121428, the test is changed so that the compilation unit no longer contains a package declaration. This is intentional. The idea is that the fix for bug 121428 was overly broad, and as part of this change I have reduced the scope of that fix. That bug (121428) was about the handling of file header comments. You can see that the code snippet in the original bug report does not include a package declaration, and that this led to ambiguity as to whether comments preceding the first import were file header comments or import comments. The existing fix for the bug is to treat all comments before the first import as file header comments, even if they occur after a package declaration. I have reduced the scope of this fix so that comments occurring after a package declaration are no longer considered to be header comments. This helps to keep the first import's comments together with that import when imports are moved or imports are inserted before it. I briefly mentioned this change in the commit description. - You also mentioned "similar changes elsewhere in the file". Please point these out so that I can address them specifically. - It was not my intent to do non-relevant code cleanup with this change. Please point out any cases where I seem to have made unnecessary "cleanup"-type changes so that I can address them. - testAddImports_bug24804_5 is removed. This test expected comments of totally removed (e.g. not folded into an on-demand import) imports to be preserved in the compilation unit, and moved to the end of the imports region, in Organize Imports mode (but not in incremental rewrite mode). As noted in comments #2 through #7 on this bug, and as mentioned in the commit description, I have changed the handling of such removed-import comments in Organize Imports mode to be consistent with that in incremental rewrite mode, so that the comments of removed imports are not preserved. Thus, testAddImports_bug24804_5 is no longer relevant.
(In reply to John Glassmyer from comment #13) Thanks for the detailed reply. There was only one pair of changes - as described for testAddImports_bug121428. One minor point is that though pack1 is removed, it is still created with IPackageFragment pack1. The non-relevant code changes meant code cleanup changes such as moving the "\n" to the preceding line as in some of the tests. These can be ignored. I have reviewed the code and have the following comments. - Well written Code - Thanks! - Compilation error due to the use of StringBuilder in ImportRewriteTest:testAddImportWithCommentBetweenImportsAndType since the project corresponding is still in 1.4. Please use StringBuffer instead [Bug 458098 would move this project to 1.6 soon though] - ImportRewriteTest:testCuInDefaultPackageWithNoExistingImports failed - please check. - in ImportRewriteAnalyzer:subtractImports(), the remainingImports.removeAll(importsToSubtract); statement does not have an effect?[since the remainingImports is populated only if the entry is not present in importsToSubtract]. We need to submit this patch for CQ process to get approval and hence need to be submitted soon [read in a couple of days or so]. Lets target to get this in for the review asap.
Thanks, Manoj. I have updated the two changes to fix several more failing jdt.ui tests, which unfortunately I only discovered recently. Some of these were cases where the new test results were clearly superior, but a few of them revealed unhandled cases in my code. I have added unit tests covering the latter cases to ImportRewriteTest in jdt.core. As for the issues you raised: - I have replaced StringBuffer with StringBuilder in ImportRewriteTest:testAddImportWithCommentBetweenImportsAndType. - I have been unable to reproduce the failure you saw of ImportRewriteTest:testCuInDefaultPackageWithNoExistingImports. If it continues to fail for you, please post more details (JUnit stack trace, etc.) so that I can investigate. - I have removed the unnecessary call to removeAll(). Thanks for catching that!
(In reply to John Glassmyer from comment #15) > > - I have been unable to reproduce the failure you saw of > ImportRewriteTest:testCuInDefaultPackageWithNoExistingImports. If it > continues to fail for you, please post more details (JUnit stack trace, > etc.) so that I can investigate. Thanks John for the modifications; I will take look. Meanwhile the issue I see in the testCuInDefaultPackageWithNoExistingImports() is that an addition "\r" character is being added just before the "\n" character in all the lines which causes the test failure [and hence visibly the output looks exactly same as the expected one but during comparison it fails].
(In reply to Manoj Palat from comment #16) > Thanks John for the modifications; I will take look. Meanwhile the issue I > in the testCuInDefaultPackageWithNoExistingImports() is that an addition > "\r" character is being added just before the "\n" character in all the > lines which causes the test failure [and hence visibly the output looks > exactly same as the expected one but during comparison it fails]. I'm guessing that you were running the test on Windows (whereas I was running it on Linux). The compilation unit source in that test doesn't contain any newlines, so findRecommendedLineSeparator was falling back to the system default line separator. I've resolved this by setting the Platform "line.separator" preference for the project in test setup. I'm not too familiar with how preferences work; please let me know if there's a better way to do this.
Noopur: can you please review the changes in jdt.ui as mentioned in comment 9? They are only test changes.
Created attachment 250373 [details] Modified Patch set 4 (In reply to John Glassmyer from comment #17) > I'm guessing that you were running the test on Windows (whereas I was > running it on Linux). The compilation unit source in that test doesn't > contain any newlines, so findRecommendedLineSeparator was falling back to > the system default line separator. > > I've resolved this by setting the Platform "line.separator" preference for > the project in test setup. I'm not too familiar with how preferences work; > please let me know if there's a better way to do this. Yes, I am running on Windows. Ideally, you should not be changing the test setup, rather you can use the line.separator (using System.getProperty("line.separator")) and use that string as delimiters. The patch set 4 does not have this failure though. However, there are a couple of points to be noted: a) StringBuilder still crops up in 10 places (these are the new tests that are added). I have changed them in the attachment. b) ImportEditor: lines [420 - 431] are deemed dead code. I have removed them in the attachment. However, you may want to have a look at the same and confirm that these do not hide any missing functionality. The attachment essentially is patch 4 with the above two changes. pl let know on point b. I have requested jdt.ui to have a look on the jdt.ui changes patch and once they give a green, we can submit this for CQ review.
Taking jdt.core patch from comment #19 and jdt.ui patch set 2 from comment #9, running all JDT UI tests on Windows shows 6 test failures. Run org.eclipse.jdt.ui.tests.refactoring.ccp.AllTests to see the failures. Five are due to newline differences after import and one is due to additional import not removed after move refactoring. JDT UI patch can be reviewed after all the tests are green.
CQ submitted: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=9210
(In reply to Manoj Palat from comment #21) > CQ submitted: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=9210 John: Please note that the fix can be committed only after all the tests are green. If the failures are less and minor then it can be committed but with a follow-up bug to address the failures (this is subject to the seriousness of failures). CQ process has been initiated in parallel due to the upcoming deadline and also assuming that the fixes to the failures will be minor.
(In reply to Manoj Palat from comment #22) > (In reply to Manoj Palat from comment #21) > > CQ submitted: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=9210 > > John: Please note that the fix can be committed only after all the tests are > green. If the failures are less and minor then it can be committed but with > a follow-up bug to address the failures (this is subject to the seriousness > of failures). CQ process has been initiated in parallel due to the upcoming > deadline and also assuming that the fixes to the failures will be minor. CQ approval is done.
Thanks, Noopur. I have fixed the broken refactoring.ccp tests. Previously I had been running the wrong "all tests" suite. Unfortunately it took me a while to address the failures you pointed out, as attempting to run the refactoring.ccp tests was hanging Eclipse and I was only able to run the tests by temporarily replacing RefactoringTestSetup with Java16Setup in each test class. Though now they seem to run fine even with RefactoringTestSetup. I have made the changes Manoj suggested to the jdt.core patch, as well.
(In reply to John Glassmyer from comment #24) > Thanks, Noopur. I have fixed the broken refactoring.ccp tests. > > I have made the changes Manoj suggested to the jdt.core patch, as well. Thanks John, all JDT UI tests are now green. However, I see unnecessary import is added in the following tests that results in "import never used" warning: org.eclipse.jdt.ui.tests.refactoring/resources/InferTypeArguments/testCuGetTakeClassStayRaw/out/A.java org.eclipse.jdt.ui.tests.refactoring/resources/MultiMove/test2/out/p2/C.java
(In reply to Noopur Gupta from comment #25) > Thanks John, all JDT UI tests are now green. However, I see unnecessary > import is added in the following tests that results in "import never used" > warning: > > org.eclipse.jdt.ui.tests.refactoring/resources/InferTypeArguments/ > testCuGetTakeClassStayRaw/out/A.java > > org.eclipse.jdt.ui.tests.refactoring/resources/MultiMove/test2/out/p2/C.java Thanks, Noopur. In InferTypeArgumentsTests.testCuGetTakeClassStayRaw, jdt.ui refactoring code is explicitly adding an import of a member type (p.A.Z) of the compilation unit's main type. ImportRewriteAnalyzer does not know whether it is safe to omit this import (e.g. it could be needed by another top-level class in the same compilation unit). In this case, the higher-level jdt.ui refactoring code should be changed to either not call addImport for this member type, or to use an ImportRewriteContext when doing so that is capable of recognizing that the import is not needed. Regarding MultiMoveTests.test2, I have changed ImportRewriteAnalyzer to reduce to on-demands (.* imports) during incremental rewrites not only when adding imports to that import container but also when removing imports from that import container. Doing so causes the unnecessary import to be removed in this case.
I have addressed all of the concerns raised here. Is this good to submit?
(In reply to John Glassmyer from comment #27) > I have addressed all of the concerns raised here. Is this good to submit? Markus is taking a look at the ui patch - would wait to see if he has any comments.
I didn't find any issues. Rebased on master and pushed as http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=b1e2ed2bb1403d1f729ccda27eb7b9ebe7009bed and http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=aaa5ad5f2614e4859ea7ac5c0bb6ca0f40d68260
Verified for 4.5 M6 with build I20150316-2000.
