Bug 305103 - [refactoring] Comment deleted on 'Move type to new file' refactoring
Summary: [refactoring] Comment deleted on 'Move type to new file' refactoring
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.6   Edit
Hardware: PC Windows XP
: P3 major (vote)
Target Milestone: 3.7 M1   Edit
Assignee: Raksha Vasisht CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on: 306524
Blocks:
  Show dependency tree
 
Reported: 2010-03-09 01:21 EST by Deepak Azad CLA
Modified: 2010-08-03 03:19 EDT (History)
2 users (show)

See Also:


Attachments
Fix for the new CU (1.87 KB, patch)
2010-06-21 05:58 EDT, Raksha Vasisht CLA
no flags Details | Diff
Fix + tests (20.55 KB, patch)
2010-07-28 13:54 EDT, Raksha Vasisht CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Deepak Azad CLA 2010-03-09 01:21:28 EST
For each code snippet below
- Move X to new file (problem is independent creating an instance variable)
- The comment gets deleted. Its neither in new file nor the old file.
- There can be some empty lines between the comment and class X, the comment will still be deleted

However if the comment is a javadoc (begins with /**) then its moved to new file.

Marking as major as there is loss of data and the user may not realize it right away. Also I do not see any junits which test these (comments) cases.

1)
package p;

class A {
	/*
	 * A very important comment.
	 */
	class X {

	}
}

2)
package p;

class A {
	/*
	 * A very important comment.	
	 */
	
	
		
	class X {

	}
	
}

3)
package p;

class A {
	// A very important comment.
	
		
	class X {

	}
	
}

4)
package p;

class A {	
		
	class X {

	}
	// A very important comment.
	
}
5)
package p;

class A {	
		
	class X {

	}	
	/* A very important comment.*/
	
}

6)
package p;

class A {	
		
	class X {

	}	
	/** A very important comment.*/
	
}


Things that work fine -
Javadoc comment before Class X
Empty lines between Class X and the comment when comment occurs after the class

1)
package p;

class A {
	/**
	 * A very important comment.
	 */
	class X {

	}
}
2)
package p;

class A {	
		
	class X {

	}
	
        // A very important comment.
	
}
3)
package p;

class A {	
		
	class X {

	}	
	
        /* A very important comment.*/
	
}

4)
package p;

class A {	
		
	class X {

	}	
	
        /** A very important comment.*/
	
}
Comment 1 Markus Keller CLA 2010-03-10 11:01:06 EST
I don't think that's new (same problem in old refactoring). Raksha, can you please have a look? Maybe we should set a NoCommentSourceRangeComputer or another source range computer to the AST rewrite.
Comment 2 Deepak Azad CLA 2010-03-10 12:18:56 EST
Yes the problem exists in 3.5 as well.

Also the problem is with Secondary types as well. For instance, moving X in the following also deletes the comment.

package p;
class A {    

}

/*
 * A very important comment.    
 */

class X {

}
Comment 3 Markus Keller CLA 2010-04-08 11:08:21 EDT
Probably not for 3.6, waiting on decision in bug 306524.
Comment 4 Raksha Vasisht CLA 2010-06-21 05:58:44 EDT
Created attachment 172314 [details]
Fix for the new CU

Markus, how about using a source range computer even for computing the new source range for the moved CU? This takes the extended range of the declaration node into account rather than just the simple range which helps in copying the comments which belong to the extended range of the node along with the declaration to the new CU. Since deleting the comments from the old CU which belong to the new CU in ASTRewriteAnalyzer is also done using the same source range computer, there wont be any inconsistencies. 

I tried this approach:
ASTNodes.getNodeSource(declaration, true, false), but that returns null in this case

ASTNode root= node.getRoot();
if (root instanceof CompilationUnit) {
			CompilationUnit astRoot= (CompilationUnit) root;
			ITypeRoot typeRoot= astRoot.getTypeRoot();

since the java type root of the astRoot is null.
Comment 5 Markus Keller CLA 2010-06-22 10:41:13 EDT
(In reply to comment #4)
+1, makes sense.
Comment 6 Raksha Vasisht CLA 2010-07-28 13:54:12 EDT
Created attachment 175427 [details]
Fix + tests

Added some tests and adjusted some for the changes in JDT-Core(bug 306524) and UI.
Comment 7 Raksha Vasisht CLA 2010-07-29 04:22:10 EDT
(In reply to comment #6)
> Created an attachment (id=175427) [details] [diff]
> Fix + tests

Committed to HEAD after Ayush committed the fix for bug 306524.
Comment 8 Dani Megert CLA 2010-08-03 03:19:20 EDT
Verified in I20100802-1800.