review: fix: Ensure CU-level elements are included when renaming a top-level type

[jdmcmahan]

fix #5086

This PR modifies the Refactoring.changeTypeName logic to prevent loss of information from the original class.

The current logic effectively creates a new compilation unit to house the renamed type. However, the type by itself only contains the source code elements for the class. All other SourceFragments outside of the class block, such as imports, comments, and whitespace, are lost in this conversion. The most apparent issue is that SniperJavaPrettyPrinter does not have the source context to properly print the full class file after a rename.

This can easily be illustrated with the existing TestSniperPrinter#testClassRename1 test (which currently does not have assertions for these external elements):

Original input:

package spoon.test.prettyprinter.testclasses;

import java.util.ArrayList;
import java.util.List;

/**
 * The content of this file 
 * 

 * 		should not be changed
 * Because DJPP should print only modified content again 
 */
public
@Deprecated
abstract class /* even this comment stays here together with all SPACES and EOLs*/ ToBeChanged<T, K> /*before extends*/
	extends ArrayList<T /* let's confuse > it */ > implements List<T>,
	Cloneable
{
	
	
	/**/
	final
	//
	private String string = "a"
			+ "b" + "c"+"d";
	
	//and spaces here are wanted too
	
	
	public <T, K> void andSomeOtherMethod(
			int param1,
			String param2         , List<?>[][]... twoDArrayOfLists)
	{/**/
		System.out.println("aaa"
				+ "xyz");
	/*x*/}
	List<?>[][] twoDArrayOfLists = new List<?>[7][];
}

//and what about this comment? 

Current result of TestSniperPrinter#testClassRename1:

package spoon.test.prettyprinter.testclasses;/**
 * The content of this file 
 * 

 * 		should not be changed
 * Because DJPP should print only modified content again 
 */
public
@Deprecated
abstract class /* even this comment stays here together with all SPACES and EOLs*/ Bar<T, K> /*before extends*/
	extends ArrayList<T /* let's confuse > it */ > implements List<T>,
	Cloneable
{
	
	
	/**/
	final
	//
	private String string = "a"
			+ "b" + "c"+"d";
	
	//and spaces here are wanted too
	
	
	public <T, K> void andSomeOtherMethod(
			int param1,
			String param2         , List<?>[][]... twoDArrayOfLists)
	{/**/
		System.out.println("aaa"
				+ "xyz");
	/*x*/}
	List<?>[][] twoDArrayOfLists = new List<?>[7][];
}

The change proposed here simply reuses the original compilation unit object to ensure top-level elements (and their source context) are retained. This comes with two consequences of note:

• Since compilation units are made up of type references instead of the types themselves, we must do some introspection to ensure the original type declaration is properly renamed. Note that the existing Query for type references is not sufficient since the reference in question is a child of the compilation unit (not the package) and therefore isn't captured with the default scope.
• Since the renamed type no longer matches the file name, we need to tweak CtCompilationUnit#getMainType to be more lenient with the top-level type. I believe it's safe to assume that a single declared type should always be considered the main type, especially given the rarity (and discouraged practice) of having multiple top-level types in a single file. We could further refine this logic for robustness (possibly adding a check on the type visibility) but this may be getting into some remote edge cases that even the existing logic doesn't cover.

Thanks in advance for the review! Don't hesitate to @ me if there are other considerations to discuss.

Other solutions explored

Copying elements into the new compilation unit

This method involved manually copying the imports and comments into the compilation unit created by CompilationUnitFactory#addType in the current logic. It's also important to set the parent of these elements to avoid consistency errors downstream.

However, this devolved into directly manipulating the SourceFragments of the compilation unit in an attempt to recreate whitespace around the new elements. This proved futile as (naturally) we can only add instances of ElementSourceFragment as children of CtCompilationUnit#getOriginalSourceFragment, and wrapping whitespace as an element felt hacky at best.

Setting the file to the same source as the original compilation unit helped to regenerate the source context, but at that point we've seemingly defeated the purpose of copying the compilation unit in the first place.

Copying the source file along with the compilation unit

Another possibility is to copy the source file along with the compilation unit. The big question here is where to copy the source file to on the filesystem. We cannot copy the file to Environment#getSourceOutputDirectory prematurely because the contents will be cleared on JDTBasedSpoonCompiler#generateProcessedSourceFilesUsingCUs (unless this logic is refined, perhaps by creating the file if it doesn't exist to satisfy the comments here

spoon/src/main/java/spoon/support/compiler/jdt/JDTBasedSpoonCompiler.java

Lines 567 to 569 in 028a123

	// order is important here, as the new file needs to exist so the CompilationUnit
	// can fetch its (still empty) source code. See CtCompilationUnitImpl#getOriginalSourceCode
	// (this will be called by getCompilationUnitInputStream(cu))

). Environment#getBinaryOutputDirectory might be a better candidate, but it's unclear whether a Maven generated-sources-type approach is appropriate for its intended use case.

A more creative solution could involve a virtual/in-memory file, although we are limited to instances of java.io.File which VirtualFile and the like does not implement. Something like Apache Commons VFS may prove useful, but this would require a new dependency as well as extensive refactoring without a provider that adheres to java.io.File.

Exposing new capabilities from CompilationUnitFactory

If the issue is that CompilationUnitFactory only accepts CtType for building a new compilation unit, perhaps another solution would be to add methods that include CU-level context. Unfortunately, this doesn't solve many of the other challenges discussed here but does provide the benefit of modularizing the solution for use cases outside of renaming a top-level type. This also gives us access to the cachedCompilationUnits internals to ensure consistency if needed.