Comment 1 Olivier Thomann 2010-04-08 09:33:58 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).

Comment 2 Markus Keller 2010-04-08 09:55:47 EDT

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.

Comment 3 Dani Megert 2010-06-08 04:37:20 EDT

Olivier, can this be targeted early 3.7?

Comment 4 Ayushman Jain 2010-06-17 14:46:42 EDT

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.

Comment 5 Markus Keller 2010-06-18 10:55:10 EDT

(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.

Comment 6 Raksha Vasisht 2010-06-18 12:17:34 EDT

(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.

Comment 7 Raksha Vasisht 2010-06-21 06:20:47 EDT

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
            
}

Comment 8 Ayushman Jain 2010-06-21 14:01:44 EDT

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.

Comment 9 Olivier Thomann 2010-07-21 11:27:10 EDT

Looks good.

Comment 10 Ayushman Jain 2010-07-29 03:45:42 EDT

Released in HEAD for 3.7M1.
Added tests org.eclipse.jdt.core.tests.rewrite.modifying.ASTRewritingModifyingTest.test0012() and test0013().

Comment 11 Srikanth Sankaran 2010-08-04 03:09:18 EDT

Verified for 3.7 M1 using build I20100802-1800