Bugzilla – Full Text Bug Listing
| Summary: | Ordering of comments between imports not preserved | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Ayushman Jain <amj87.iitr> | ||||||||||
| Component: | Core | Assignee: | Stephan Herrmann <stephan.herrmann> | ||||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||||
| Severity: | normal | ||||||||||||
| Priority: | P3 | CC: | daniel_megert, jarthana, satyam.kandula, srikanth_sankaran | ||||||||||
| Version: | 3.8 | Flags: | stephan.herrmann:
review+
|
||||||||||
| Target Milestone: | 4.3 M5 | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | All | ||||||||||||
| Whiteboard: | |||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Ayushman Jain
Tentatively marking for M7, since I have a working fix which passes all tests. If review cannot be finished in time, will move it out. I'll submit a patch once bug 376930 is released. > HEAD + fix for bug 376734 This doesn't make much sense since HEAD/master already contains the fix for bug 376734, no? Did you mean bug 376930? (In reply to comment #2) > > HEAD + fix for bug 376734 > > This doesn't make much sense since HEAD/master already contains the fix for bug > 376734, no? Did you mean bug 376930? Yes bug 376930 is what I meant. Sorry for the confusion. I pushed a topic branch ajain/bug378024 with the changes. Satyam, would you like to volunteer for a review? :) A bit of explanation about the fix: 1) The first part of the fix deals with preserving comment positions when all imports are removed and re-added. This is done when a user presses CTRL+SHIFT+O. Without the fix the preserveCommentRanges[] array stores the comment positions but it moves all of them to the end. In the fix, a new method IRA.addImportDeclEntry(..) now reads that array and initializes the freshly added imports with trailing and leading comments, if those imports existed earlier as well. This fix thus preserves not just the comments, but also their positions, when CTRL+SHIFT+O is done. Comments nt part of any extended range are also added as dummy import entries to preserve their order between imports. 2) The other part is to correct a few problems while dealing with comments when imports are restored, and only new ones are added or a few removed. This operation happens during quick fix, refactoring, etc. In some cases, comments were still not preserved. In case we remove an import, all its trailing and leading comments were getting lost. This is fixed in IRA.removeImport(..). Comments not part of any extended range are now better handled in IRA.addExistingImports(..). Stephan, will you be able to spare a few cycles for a second review? After Satyam pointed out some weird rendering in the editor on the topic branch, I noticed that some edits were malformed after the fix, such as a delete edit and an insert edit starting at the same offset. I investigated and removed some changes that, in hindsight, were really not necessary and were causing mangled edits. The patch is leaner now. I've pushed the changes to the branch. It will help to compare the changed file with the version before the commits for the fix started to get a clear picture of what really changed. Also, one problem that will no longer be tackled is the case of removal of imports. ie. //lead import bla; // comment // trail If bla is removed, //comment will also be removed along with it. This was always the case and is nothing new. We can fix this case later with another bug. Handling it for this bug was complicating the patch. (In reply to comment #7) > After Satyam pointed out some weird rendering in the editor on the topic > branch, I noticed that some edits were malformed after the fix, such as a > delete edit and an insert edit starting at the same offset. I investigated and > removed some changes that, in hindsight, were really not necessary and were > causing mangled edits. The patch is leaner now. Ayush, I haven't looked at the patch yet - but it looks to me if this is long standing issue, we should let the sleeping dog lie and fix it just post 3.8. Given what we know of the imminent changes, I would hate to have to fire fight on some behavior that was around for a while and was not reported by customers and tagged as a blocker. Can you please identify the earliest version that displays the bug ? Let me know if you strongly disagree with the request to defer. Thinking a bit more about it, this also strikes me as a bit of a pedantic/academic case - while it is not unimaginable that imports would be preceded by comments, this doesn't look like a common case which would explain this problem not being found/reported by customers. (In reply to comment #9) > Thinking a bit more about it, this also strikes me as a bit > of a pedantic/academic case - while it is not unimaginable that > imports would be preceded by comments, this doesn't look like > a common case which would explain this problem not being found/reported > by customers. The customers did not find this issue until now because until 3.7, the comments are completely lost. And if you look at the bug 24804, you'll see a host of duplicated filed for the comment deletion issue. Since from 3.8 onwards, comments are preserved but their position is changed, we'll see "zombie changes in my file on saving" bugs post 3.8. (In reply to comment #8) > Can you please identify the earliest version that displays the bug ? It has been there ever since Import Rewriter was implemented i.e. as far back as 3.3 > Let me know if you strongly disagree with the request to defer. No, I don't strongly disagree, but would prefer to see it through atleast in 4.3M1. (In reply to comment #10) Thanks for the clarification Ayush. > > Let me know if you strongly disagree with the request to defer. > No, I don't strongly disagree, but would prefer to see it through atleast in > 4.3M1. Sounds good. Let us proceed with the reviews and incorporate any comments from there and publish an up to date patch here - we will release it just post 3.8 shipment. Comments from reading the changes:
In the new method addImportDeclEntry():
(1) I'd suggest to extract this.preserveExistingCommentsRanges[j].getLength() to a local, just as you did for the offset :)
(2) In the block starting with
} else if ((currExtendedStart + currExtendedLen) != (currOffset + currLength)){
you seem to assume that offset is either exactly == (currOffset + currLength) or completely outside the current entry.
Could it happen that there is a small gap between (currOffset + currLength) and the offset, which may still be within the extended range?
If not, what prevents that situation?
(3) Any reasons, not to combine the checks before the method's invocation and at the start of the method body? In both cases we may opt out and directly create a ImportDeclEntry instead.
Some changes in addExistingImports() seem to be obsolete.
Other than that I'm a bit at loss identifying the purpose of each individual change, since as you say this addresses several problems at once.
(In reply to comment #12) > Comments from reading the changes: > > In the new method addImportDeclEntry(): > > (1) I'd suggest to extract this.preserveExistingCommentsRanges[j].getLength() > to a local, just as you did for the offset :) Yup, good point. > (2) In the block starting with > } else if ((currExtendedStart + currExtendedLen) != (currOffset + > currLength)){ > you seem to assume that offset is either exactly == (currOffset + currLength) > or completely outside the current entry. > Could it happen that there is a small gap between (currOffset + currLength) and > the offset, which may still be within the extended range? > If not, what prevents that situation? There could be a gap, but the whole idea of this method is to calculate preceeding and trailing comments for each import. In case the offset of a comment starts after the (currOffset + currLength) boundary, it cannot be part of the trailing range and is thus not interesting to us. > (3) Any reasons, not to combine the checks before the method's invocation and > at the start of the method body? In both cases we may opt out and directly > create a ImportDeclEntry instead. Right, we can. I however like the separate checks, so that a reader clearly knows that the method needs to be called only when the imports have to be preserved (quick fix, etc.), and not in other cases (CTRL+SHIFT+O) > Some changes in addExistingImports() seem to be obsolete. I do see a redudnant variable nextExtendedOffsetLine, but not sure exactly what others are. > Other than that I'm a bit at loss identifying the purpose of each individual > change, since as you say this addresses several problems at once. I think the core part of the fix is explained in comment 5. There are a lot of changes that had to be done as 'adjustments' to take care of the new way of calculating preceeding and trailing comments. I do have tests added for all these I'm sure. Created attachment 219961 [details]
proposed fix v1.0 + regression tests
While the changes are in the topic branch, here is the latest patch that applies on master as of today.
It'll be good if jdt.ui tests could be run for this and this be released to master. I've already tested the patch and also run all jdt.core tests. Everything looks ok.
Tentatively targetting for 4.3M2
Stephan, please review and release for M3. (In reply to comment #15) > Stephan, please review and release for M3. sure, will do. Stephan, do you expect to be able to get to this for M3? Feel free to move out to M4 otherwise. Sorry, this escaped my attention. As I don't want to rush it I'm moving it to M4. Here's what I found during reviewing the patch from comment 14: COSMETICS: (1) two unused locals in addExistingImports() -> removed (2) simplification from new IRegion[preserveCommentsLen = (preserveCommentsLen - 1)] to new IRegion[--preserveCommentsLen] LOGIC: (3) I'm not 100% sure about allImportsAddedToStar in getResultingEdits(): At first I thought instead of setting allImportsAddedToStar=true we could simply "continue", but further testing showed that this misses the handling of onDemandConflicts. Frankly, I'm not comfortable with the various non-normalized combinations of 6 individual predicates in the body of the k-loop. When looking for tests that would demonstrate what exactly this logic should look like I found the following (assume *-threshold is 1): package pack1; import java.util.Map; /* lead Entry */ import java.util.Map.Entry; public class C1 { public static void main(String[] args) { HashMap h; Map.Entry e= null; Entry e2= null; } } Trigger importing of HashMap (e.g., by code completion) fails to collate all into java.util.*, producing these imports: import java.util.Map; import java.util.*; /* lead Entry */ import java.util.Map.Entry; The explicit import of Map shouldn't be there and the same example without comments does produce the correct result. This could already be caused prior to getResultingEdits() because Map and HashMap are captured in different PackageEntries. This needs more investigation. Created attachment 224197 [details]
Required changes in JDT/UI tests
These changes would be required in JDT/UI tests when the current patch is released.
Changes look OK to me but should be approved by others.
Created attachment 224198 [details] updated patch I've fixed the issues from comment 19 item (3): - collating into *-import was prevented by a comment PackageEntry that disturbed the grouping of import decls. -> Fixed by representing the comment by an ImportDeclEntry only. This required fine tuning regarding the insertion/preservation of newlines (some of which has impact on JDT/UI tests ...) - made logic inside getResultingEdits() more readable by introducing some boolean locals. -> includes a bug fix where a predicate (o != null && o.m()) was incorrectly unfolded to (o == null || o.m()) (this never surfaced, perhaps because o is never null(?)) Created attachment 224199 [details] updated changes for JDT/UI tests Here's an update of required changes in JDT/UI tests (only expecting one fewer newlines than previous patch). Two more tests require updating: InferTypeArgumentsTest.{testJUnit,testJUnitWithCloneNotRaw}, where the expected result lacks a newline between import groups. In these tests comparison is against a src-zip (?) of which I don't know how this was created, so I'm not sure what's the best approach to update the tests. I briefly tried to avoid the issue by javaProject.setOption(DefaultCodeFormatterConstants.FORMATTER_BLANK_LINES_BETWEEN_IMPORT_GROUPS, "0"); but then the inverse problem occurred in a different location. Please let me know if these test changes are OK. If there's a +1 from JDT/UI regarding these test changes I'm ready to release the patch from comment 21. Dani, could you please comment on comment 22? TIA. (In reply to comment #23) > Dani, could you please comment on comment 22? TIA. This is fine. Just ping me when you want to go ahead. To be finished early in M5. Can we go ahead and close this? I released the patch for 4.3 M5 via commit 919024c6945d276058446a723145394a23656d2f Leaving the bug open for handshake with JDT/UI: their tests need adjusting, comment 22 has the details, thanks. (In reply to comment #27) > I released the patch for 4.3 M5 via commit > 919024c6945d276058446a723145394a23656d2f > > Leaving the bug open for handshake with JDT/UI: their tests need adjusting, > comment 22 has the details, thanks. Fixed with http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=7deee1b79cf1789be4e7e257b25c31d2e6c9d19e (In reply to comment #28) > (In reply to comment #27) > > I released the patch for 4.3 M5 via commit > > 919024c6945d276058446a723145394a23656d2f > > > > Leaving the bug open for handshake with JDT/UI: their tests need adjusting, > > comment 22 has the details, thanks. > > Fixed with > http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/ > ?id=7deee1b79cf1789be4e7e257b25c31d2e6c9d19e Did you check InferTypeArgumentsTest (see comment 22)? cleaning up stale review flag. (In reply to comment #29) > (In reply to comment #28) > > (In reply to comment #27) > > > I released the patch for 4.3 M5 via commit > > > 919024c6945d276058446a723145394a23656d2f > > > > > > Leaving the bug open for handshake with JDT/UI: their tests need adjusting, > > > comment 22 has the details, thanks. > > > > Fixed with > > http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/ > > ?id=7deee1b79cf1789be4e7e257b25c31d2e6c9d19e > > Did you check InferTypeArgumentsTest (see comment 22)? Oops, didn't fully read comment 22 and assumed your patch is complete. (In reply to comment #31) > (In reply to comment #29) > > (In reply to comment #28) > > > (In reply to comment #27) > > > > I released the patch for 4.3 M5 via commit > > > > 919024c6945d276058446a723145394a23656d2f > > > > > > > > Leaving the bug open for handshake with JDT/UI: their tests need adjusting, > > > > comment 22 has the details, thanks. > > > > > > Fixed with > > > http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/ > > > ?id=7deee1b79cf1789be4e7e257b25c31d2e6c9d19e > > > > Did you check InferTypeArgumentsTest (see comment 22)? > > Oops, didn't fully read comment 22 and assumed your patch is complete. OK. I was almost 100% sure that I ran the tests when adding comment 24 and indeed, the InferTypeArgumentsTests are green. So, I don't know what the problem was on your end. (In reply to comment #32) > OK. I was almost 100% sure that I ran the tests when adding comment 24 and > indeed, the InferTypeArgumentsTests are green. that's good news :) > So, I don't know what the problem was on your end. I can't reproduce either. Sorry for the churn. Alerted by bug 397739 I rechecked and found this: - during code review I created two local branches, one with Ayush's last state and one with my polish - when I finally released the fix I pushed the wrong branch (unaware of the other one) While Ayush's patch is good, this version still has the problem from comment 19. Re-opening to push my changes, too. *** Bug 397739 has been marked as a duplicate of this bug. *** I released the missing changes from code review via commit 4acde1e0a5179616c6bfc8db1555f7556fcd927f. At this point I do see the failures in InferTypeArgumentsTests (JDT/UI). Dani, sorry to bother you once more to have a look at these two tiny issues. (In reply to comment #36) > I released the missing changes from code review via commit > 4acde1e0a5179616c6bfc8db1555f7556fcd927f. > > At this point I do see the failures in InferTypeArgumentsTests (JDT/UI). > > Dani, sorry to bother you once more to have a look at these two tiny issues. np. Fixed with http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=1d67bcb40c63b0f2a68e3ed3b0a91fcdf0885738 (In reply to comment #37) Thanks, and thanks for catching this via bug 397739! (In reply to comment #38) > (In reply to comment #37) > > Thanks, and thanks for catching this via bug 397739! RenamePackageTests.testImportFromMultiRoots2()was also failing. Fixed now with http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=1eb4ac014de23c6931919520c6a4b755a59833ad Verified for 4.3 M5 with build I20130130-2000 |