Summary: | ASTRewriteAnalyzer uses wrong starting offset in case of comments before a node | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | [Eclipse Project] JDT | Reporter: | Raksha Vasisht <raksha.vasisht> | ||||||
Component: | Core | Assignee: | Ayushman Jain <amj87.iitr> | ||||||
Status: | VERIFIED FIXED | QA Contact: | |||||||
Severity: | normal | ||||||||
Priority: | P3 | CC: | daniel_megert, deepakazad, markus.kell.r, Olivier_Thomann, srikanth_sankaran | ||||||
Version: | 3.6 | Flags: | Olivier_Thomann:
review+
|
||||||
Target Milestone: | 3.7 M1 | ||||||||
Hardware: | PC | ||||||||
OS: | Windows XP | ||||||||
Whiteboard: | |||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 305103 | ||||||||
Attachments: |
|
Description
Raksha Vasisht
2010-03-19 09:49:28 EDT
It looks like the rewriter removes everything from the insertion position till the end of the removed node. In this case the insertion position is the position after the opening brace. It should only remove the contents of the node from its starting position up to its ending position (start + length). This might leave some whitespaces behind (indentation of the current node). Any fix for this needs to be carefully reviewed, especially in situations where multiple consecutive nodes are removed. We have to make sure that no stray comments can stay around from between the removed nodes (e.g. when Extract Method moves multiple statements). If too risky, we may have to defer to 3.7. Olivier, can this be targeted early 3.7? Created attachment 172148 [details]
proposed fix v1.0 + regression test
There are 2 problems here :
1) JDT/Core problem: When we remove the type body, we also remove the leading and trailing separators, and with separators the comments are removed as well. The changes in ASTRewriterAnalyzer attempt to fix this. For leading separators, we move the current position to the start of the next node, and the prevEnd to the end of comments (if they're not part of the extended source range). This way the separator range is calculated omitting the comments. Similarly, for trailing comments, we set the end point to the extended end point of the node which we're removing, instead of setting it to the starting of next node as is done now. So in effect, for trailing comments we dont touch the separator at all. That means we dont even delete the whitespace from end of the current node until starting of the first subsequent comment. Consider what will happen if we also delete that whitespace instead of this approach:
// start
Type {
}
// end
becomes
// start
// end
because it tends to preserve the indentation of the removed line. Hence I preferred
// start
// end
because even though it doesnt remove whitespace, it preserves indentation of comments.
2) JDT/UI problem: When the comments are in the extended source range of the node (i.e. no space between comments and the declaration body), we correctly remove the comments along with the type. Ideally, for "move type to new file", these comments should move along with the type to the new file. JDT/UI doesnt do this currently.
Raksha, can you please run the UI tests on this patch and check if everything's ok? Thanks.
(In reply to comment #4) > 2) JDT/UI problem: When the comments are in the extended source range of the > node (i.e. no space between comments and the declaration body), we correctly > remove the comments along with the type. Ideally, for "move type to new file", > these comments should move along with the type to the new file. JDT/UI doesnt > do this currently. I agree, in MoveInnerToTopRefactoring#createNewSource(..), we should use: return ASTNodes.getNodeSource(declaration, true, false); We will do that in bug 305103. (In reply to comment #4) > Created an attachment (id=172148) [details] [diff] > proposed fix v1.0 + regression test > So in effect, for trailing comments we dont touch the separator at > all. That means we dont even delete the whitespace from end of the current node > until starting of the first subsequent comment. Consider what will happen if we > also delete that whitespace instead of this approach: > > // start > > Type { > } > > > // end > > > becomes > > // start > > // end > > because it tends to preserve the indentation of the removed line. Hence I > preferred > > // start > > > > > // end > > because even though it doesnt remove whitespace, it preserves indentation of > comments. > > 2) JDT/UI problem: When the comments are in the extended source range of the > node (i.e. no space between comments and the declaration body), we correctly > remove the comments along with the type. Ideally, for "move type to new file", > these comments should move along with the type to the new file. JDT/UI doesnt > do this currently. > > Raksha, can you please run the UI tests on this patch and check if everything's > ok? Thanks. I'm running the UI tests and I can already see some failures due to the change in white space removal approach. I can adjust all of them if this patch is released along with the JDT/UI problem in bug 305103. Ayush, I found another case where the deletion happens : package p; class A { int i; // First comment does not get deleted - OK // second comment does not belong to 'X' gets deleted - WRONG! //comment gets deleted - OK // second comment gets deleted- OK class X { } //comment gets deleted - OK //second comment gets deleted - OK // First comment does not get deleted - OK // second comment does not get deleted - OK } May be you need to use a 'while' rather than checking it once? Note: The second comment also gets deleted when there is space between first and second comment : package p; class A { int i; // First comment does not get deleted - OK // second comment does not belong to 'X' gets deleted - WRONG! //comment gets deleted - OK // second comment gets deleted- OK class X { } //comment gets deleted - OK //second comment gets deleted - OK // First comment does not get deleted - OK // second comment does not get deleted - OK } Created attachment 172349 [details]
improved fix + tests
Raksha, good catch.
Added a while loop to detect several comments.
Note that this deletion of the leading comments takes place if the type being removed is the last one. Added another test ASTRewritingModifyingRemoveTest.test013() for this.
Looks good. Released in HEAD for 3.7M1. Added tests org.eclipse.jdt.core.tests.rewrite.modifying.ASTRewritingModifyingTest.test0012() and test0013(). Verified for 3.7 M1 using build I20100802-1800 |