Code actions should return textedits with proper formatting

[fbricon]

See #1151 (comment):

Initial file uses spaces for indentation, new created file contains a mix of tabs (for new code) and spaces (moved code):

For the mixed tab and space issue, this is a common tricky issue for all refactor and quick fix features.

Currently the quick fix and refactoring processor in the upstream jdt core will use the project settings to indent the new generated AST Node. However, most of time, the user will not configure it manually, that means the jdt will use the default tab as indentation.

Current workaround is you need manually click 'Format Document' context menu or modify org.eclipse.jdt.core.prefs setting with following:

org.eclipse.jdt.core.formatter.tabulation.size: 4
org.eclipse.jdt.core.formatter.tabulation.char: space

I believe we need to rework it in future and find a general solution to deal with it. (Read the format info from client through workspace/getConfiguration protocol, See microsoft/language-server-protocol#780)

@jdneo @testforstephen @akaroml any volunteer?

[testforstephen]

@jdneo Given you have a look at the formatting issue before, could you take over this area?

[jdneo]

@testforstephen Sure. Let me take a look.

[jdneo]

Since the formatting issue has to be fixed case by case, below is a list of the code actions we support now, some effort is needed to check if any of them need to migrate to the new implementation.

• AssignParamToField
• Move... series
• Extract... series
• Inline... series
• Convert Anonymous to Nested
• Convert Lamda <-> Nested
• Convert VarType <-> ResolvedType
• Add Static Import
• Convert For Loop
• Override/Implement Methods…
• Organize Imports
• Generate Getters/Setters…
• Generate hashCode() and equals()…
• Generate toString()…
• Generate Delegate Methods…
• Generate Constructor using Fields…
• Generate Constructors from Superclass…
• Inverse Condition
• Inverse Local Variable

[testforstephen]

At the moment, the JDT code actions are all using ASTRewrite.rewriteAST() to generate the text edit, where it reads the formatting options from both the project and JavaCore.getOptions() by default. See
https://github.com/eclipse/eclipse.jdt.core/blob/master/org.eclipse.jdt.core/dom/org/eclipse/jdt/core/dom/rewrite/ASTRewrite.java#L273-L295.

In order to make code actions to return textedits with the correct formatting, it is critical to pass the correct formatting to ASTRewrite.

If the client only supports configuring formatting options at the workspace level, we just need to reuse JavaCore.setOptions() to convert the client's formatting options to JDT preferences and ASTRewrite will automatically get the correct formatting.

However, if the client is allowed to configure indentation for each Java file (as VS Code does), it may not be appropriate to continue to reuse JavaCore to pass on formatting preferences, as this would cause JavaCore to change each time a new Java file is opened. The alternative is to explicitly pass the formatting settings for each file individually to the code action proposal, which requires us to refactor the upstream code action proposal to accept custom formatting options.

[testforstephen]

Another idea is to add two methods getOptions() and setOptions() to the ICompilationUnit interface, which can be used to save the indentation settings for each Java file.

	/**
	 * Sets the ICompilationUnit custom options. All and only the options explicitly included in the given table
	 * are remembered; all previous option settings are forgotten, including ones not explicitly
	 * mentioned.
	 * <p>
	 * For a complete description of the configurable options, see <code>JavaCore#getDefaultOptions</code>.
	 * </p>
	 *
	 * @param newOptions the new options (key type: <code>String</code>; value type: <code>String</code>),
	 *   or <code>null</code> to flush all custom options (clients will automatically get the global JavaCore options).
	 * @see JavaCore#getDefaultOptions()
	 */
	void setOptions(Map<String, String> newOptions);

	/**
	 * Returns the table of the current custom options for this ICompilationUnit. ICompilationUnit remember their custom options,
	 * in other words, only the options different from the JavaProject and JavaCore global options for the workspace.
	 * A boolean argument allows to directly merge the ICompilationUnit options with the project and global ones from <code>JavaCore</code>.
	 * <p>
	 * For a complete description of the configurable options, see <code>JavaCore#getDefaultOptions</code>.
	 * </p>
	 *
	 * @param inheritJavaCoreOptions - boolean indicating whether project and JavaCore options should be inherited as well
	 * @return table of current settings of all options
	 *   (key type: <code>String</code>; value type: <code>String</code>)
	 * @see JavaCore#getDefaultOptions()
	 */
	Map<String, String> getOptions(boolean inheritJavaCoreOptions);

And then let ASTRewrite.java to read the formatting from the ICompilationUnit by default.

public TextEdit rewriteAST() throws JavaModelException, IllegalArgumentException {
		ASTNode rootNode= getRootNode();
		if (rootNode == null) {
			return new MultiTextEdit(); // no changes
		}

		ASTNode root= rootNode.getRoot();
		if (!(root instanceof CompilationUnit)) {
			throw new IllegalArgumentException("This API can only be used if the AST is created from a compilation unit or class file"); //$NON-NLS-1$
		}
		CompilationUnit astRoot= (CompilationUnit) root;
		ITypeRoot typeRoot = astRoot.getTypeRoot();
		if (typeRoot == null || typeRoot.getBuffer() == null) {
			throw new IllegalArgumentException("This API can only be used if the AST is created from a compilation unit or class file"); //$NON-NLS-1$
		}

		char[] content= typeRoot.getBuffer().getCharacters();
		LineInformation lineInfo= LineInformation.create(astRoot);
		String lineDelim= typeRoot.findRecommendedLineSeparator();
                
                 // =====> Add new code here
                 Map options = typeRoot instanceof ICompilationUnit) ? ((ICompilationUnit) typeRoot).getOptions(true) :
                          typeRoot.getJavaProject().getOptions(true);

		return internalRewriteAST(content, lineInfo, lineDelim, astRoot.getCommentList(), options, rootNode, (RecoveryScannerData)astRoot.getStatementsRecoveryData());
	}

This approach has the minimal code changes on the upstream JDT code.

@rgrunber @jdneo WDYT?

[jdneo]

That seems to be a more feasible solution.

[testforstephen]

Created an issue https://bugs.eclipse.org/bugs/show_bug.cgi?id=569336 in upstream to discuss about supporting custom formatting options at file granularity.