Finalized JDT import organizer wrapper and testing.

Author: fvgh

_ext/eclipse-jdt/src/main/java/com/diffplug/spotless/extra/eclipse/java/EclipseJdtCleanUpStepImpl.java

@@ -31,29 +31,28 @@
 import org.eclipse.jdt.core.manipulation.JavaManipulation;
 import org.eclipse.jdt.core.manipulation.OrganizeImportsOperation;
 import org.eclipse.jdt.core.manipulation.SharedASTProviderCore;
-import org.eclipse.jdt.internal.core.JavaCorePreferenceInitializer;
 import org.eclipse.jdt.internal.corext.codemanipulation.CodeGenerationSettingsConstants;
 
 import com.diffplug.spotless.extra.eclipse.base.SpotlessEclipseFramework;
 
 /** Clean-up step which calls out to the Eclipse JDT clean-up / import sorter. */
 public class EclipseJdtCleanUpStepImpl {
 
-	/* The JDT UI shall be used for creating the settings. */
+	// The JDT UI shall be used for creating the settings.
 	private final static String JDT_UI_PLUGIN_ID = "org.eclipse.jdt.ui";
-
-	private final IJavaProject jdtConfiguration;
+	private final IJavaProject jdtConfiguration; //The project stores the JDT clean-up configuration
+	private final EclipseJdtHelper jdtHelper;
 
 	public EclipseJdtCleanUpStepImpl(Properties settings) throws Exception {
 		if (SpotlessEclipseFramework.setup(
 				core -> {
 					/*
-					 * For the Clean-Up, the indexer needs to exists (but is not used).
-					 * The indexer is not created in headless mode by the JDT.
-					 * To signal a non-headless mode, the platform state needs to by active
-					 * (it is only resolved by default).
+					 * Indexer needs to exist (but is not used) for JDT clean-up.
+					 * The indexer is not created in headless mode by JDT.
+					 * 'Active' platform state signals non-headless mode ('Resolved' is default state)..
 					 */
 					core.add(new org.eclipse.core.internal.registry.osgi.Activator());
+					
 					core.add(new org.eclipse.core.internal.runtime.PlatformActivator());
 					core.add(new org.eclipse.core.internal.preferences.Activator());
 					core.add(new org.eclipse.core.internal.runtime.Activator());
@@ -66,13 +65,12 @@ public EclipseJdtCleanUpStepImpl(Properties settings) throws Exception {
 					config.changeSystemLineSeparator();
 
 					/*
-					 * The default no content type specific handling is insufficient.
+					 * The default 'no content type specific handling' is insufficient.
 					 * The Java source type needs to be recognized by file extension.
 					 */
 					config.add(IContentTypeManager.class, new JavaContentTypeManager());
-					config.useSlf4J(EclipseJdtCleanUpStepImpl.class.getPackage().getName());
 
-					//Initialization of jdtConfiguration requires OS set
+					config.useSlf4J(EclipseJdtCleanUpStepImpl.class.getPackage().getName());
 					config.set(InternalPlatform.PROP_OS, "");
 				},
 				plugins -> {
@@ -82,13 +80,14 @@ public EclipseJdtCleanUpStepImpl(Properties settings) throws Exception {
 					plugins.add(new org.eclipse.core.internal.filesystem.Activator());
 					plugins.add(new JavaCore());
 				})) {
-			new JavaCorePreferenceInitializer().initializeDefaultPreferences();
 			initializeJdtUiDefaultSettings();
 		}
-		jdtConfiguration = EclipseJdtFactory.createProject(settings);
+		jdtHelper = EclipseJdtHelper.getInstance();
+		jdtConfiguration = jdtHelper.createProject(settings);
 	}
 
 	private static void initializeJdtUiDefaultSettings() {
+		//Following values correspond org.eclipse.jdt.ui.PreferenceConstants
 		JavaManipulation.setPreferenceNodeId(JDT_UI_PLUGIN_ID);
 		IEclipsePreferences prefs = DefaultScope.INSTANCE.getNode(JDT_UI_PLUGIN_ID);
 
@@ -103,12 +102,12 @@ private static void initializeJdtUiDefaultSettings() {
 	}
 
 	public String organizeImport(String raw) throws Exception {
-		ICompilationUnit sourceContainer = EclipseJdtFactory.createJavaSource(raw, jdtConfiguration);
-		CompilationUnit ast = SharedASTProviderCore.getAST(sourceContainer, SharedASTProviderCore.WAIT_YES, null);
-		OrganizeImportsOperation formatOperation = new OrganizeImportsOperation(sourceContainer, ast, true, false, true, null);
+		ICompilationUnit compilationUnit = jdtHelper.createCompilationUnit(raw, jdtConfiguration);
+		CompilationUnit ast = SharedASTProviderCore.getAST(compilationUnit, SharedASTProviderCore.WAIT_YES, null);
+		OrganizeImportsOperation formatOperation = new OrganizeImportsOperation(compilationUnit, ast, true, false, true, null);
 		try {
 			formatOperation.run(null);
-			return sourceContainer.getSource();
+			return compilationUnit.getSource();
 		} catch (OperationCanceledException | CoreException e) {
 			throw new IllegalArgumentException("Invalid java syntax for formatting.", e);
 		}

_ext/eclipse-jdt/src/main/java/com/diffplug/spotless/extra/eclipse/java/EclipseJdtFactory.java renamed to _ext/eclipse-jdt/src/main/java/com/diffplug/spotless/extra/eclipse/java/EclipseJdtHelper.java

@@ -15,9 +15,7 @@
  */
 package com.diffplug.spotless.extra.eclipse.java;
 
-import java.util.Collections;
 import java.util.HashMap;
-import java.util.Hashtable;
 import java.util.List;
 import java.util.Map;
 import java.util.Properties;
@@ -27,21 +25,24 @@
 import org.eclipse.core.resources.IFolder;
 import org.eclipse.core.resources.IProject;
 import org.eclipse.core.resources.IProjectDescription;
+import org.eclipse.core.resources.ProjectScope;
 import org.eclipse.core.resources.ResourcesPlugin;
 import org.eclipse.core.runtime.IProgressMonitor;
+import org.eclipse.core.runtime.preferences.IEclipsePreferences;
 import org.eclipse.jdt.core.IBuffer;
 import org.eclipse.jdt.core.ICompilationUnit;
 import org.eclipse.jdt.core.IJavaProject;
 import org.eclipse.jdt.core.IPackageFragment;
 import org.eclipse.jdt.core.IPackageFragmentRoot;
-import org.eclipse.jdt.core.IType;
 import org.eclipse.jdt.core.JavaCore;
 import org.eclipse.jdt.core.JavaModelException;
+import org.eclipse.jdt.core.manipulation.JavaManipulation;
 import org.eclipse.jdt.internal.core.BufferManager;
 import org.eclipse.jdt.internal.core.CompilationUnit;
 import org.eclipse.jdt.internal.core.DefaultWorkingCopyOwner;
-import org.eclipse.jdt.internal.core.JavaModelManager;
+import org.eclipse.jdt.internal.core.JavaCorePreferenceInitializer;
 import org.eclipse.jdt.internal.core.PackageFragment;
+import org.eclipse.jdt.internal.core.nd.indexer.Indexer;
 
 /**
  * Helper methods to create Java compilation unit.
@@ -51,20 +52,38 @@
  * (see {@code org.eclipse.core.internal.resources.LocationValidator} for details).
  * </p>
  */
-class EclipseJdtFactory extends OS {
+class EclipseJdtHelper extends OS {
 
 	private final static String ROOT_AS_SRC = "";
 	private final static String PROJECT_NAME = "spotless";
 	private final static String SOURCE_NAME = "source.java";
-	private final static AtomicInteger UNIQUE_PROJECT_ID = new AtomicInteger(0);
+	private static EclipseJdtHelper INSTANCE; 
 
-	private final static Map<String, String> DEFAULT_OPTIONS;
-
-	static {
-		Map<String, String> defaultOptions = new HashMap<>();
-		defaultOptions.put(JavaCore.COMPILER_SOURCE, getJavaCoreVersion());
-		DEFAULT_OPTIONS = Collections.unmodifiableMap(defaultOptions);
+	static synchronized EclipseJdtHelper getInstance() {
+		if(null == INSTANCE) {
+			INSTANCE = new EclipseJdtHelper();
+		}
+		return INSTANCE;
 	}
+	
+	private final AtomicInteger uniqueProjectId = new AtomicInteger(0);
+	private final Map<String, String> defaultOptions;
+	
+	private EclipseJdtHelper() {
+		defaultOptions = new HashMap<>();
+		defaultOptions.put(JavaCore.COMPILER_SOURCE, getJavaCoreVersion());
+		
+		/*
+		 * Assure that the 'allowed keys' are initialized, otherwise
+		 * JProject will not accept any options.
+		 */
+		new JavaCorePreferenceInitializer().initializeDefaultPreferences();
+		
+		/*
+		 * Don't run indexer in background (does not disable thread but the job scheduling)
+		 */
+		Indexer.getInstance().enableAutomaticIndexing(false);
+	} 
 
 	private static String getJavaCoreVersion() {
 		final String javaVersion = System.getProperty("java.version");
@@ -76,30 +95,41 @@ private static String getJavaCoreVersion() {
 		}
 		return orderedSupportedCoreVersions.get(orderedSupportedCoreVersions.size() - 1);
 	}
-
+	
 	/**
 	 * Creates a JAVA project and applies the configuration.
 	 * @param settings Configuration settings
 	 * @return Configured JAVA project
 	 * @throws Exception In case the project creation fails
 	 */
-	public final static IJavaProject createProject(Properties settings) throws Exception {
-		String uniqueProjectName = String.format("%s-%d", PROJECT_NAME, UNIQUE_PROJECT_ID.incrementAndGet());
+	IJavaProject createProject(Properties settings) throws Exception {
+		String uniqueProjectName = String.format("%s-%d", PROJECT_NAME, uniqueProjectId.incrementAndGet());
 		IProject project = ResourcesPlugin.getWorkspace().getRoot().getProject(uniqueProjectName);
 		// The project must be open before items (natures, folders, sources, ...) can be created
 		project.create(null);
 		project.open(0, null);
-		//If the project nature is not set, things like AST are not created for the Java projects
+		
+		//If the project nature is not set, the AST is not created for the compilation units
 		IProjectDescription description = project.getDescription();
 		description.setNatureIds(new String[]{JavaCore.NATURE_ID});
 		project.setDescription(description, null);
 		IJavaProject jProject = JavaCore.create(project);
 
-		Map<String, String> settingsMap = new HashMap<>(DEFAULT_OPTIONS);
+		Map<String, String> allSettings = new HashMap<>(defaultOptions);
 		settings.forEach((key, value) -> {
-			settingsMap.put(key.toString(), value.toString());
+			allSettings.put(key.toString(), value.toString());
 		});
-		jProject.setOptions(settingsMap);
+		//Configure JDT manipulation processor
+		IEclipsePreferences projectPrefs = new ProjectScope(project.getProject()).getNode(JavaManipulation.getPreferenceNodeId());
+		allSettings.forEach((key, value) -> {
+			projectPrefs.put(key.toString(), value.toString());
+		});		
+		/*
+		 * Configure options taken directly from the Java project (without qualifier).
+		 * Whether a setting is a Java project option or not, is filtered by the
+		 * JavaCorePreferenceInitializer, initialized by the constructor of this class.
+		 */
+		jProject.setOptions(allSettings);
 
 		// Eclipse source files require an existing source folder for creation
 		IPackageFragmentRoot src = jProject.getPackageFragmentRoot(jProject.getProject());
@@ -110,27 +140,22 @@ public final static IJavaProject createProject(Properties settings) throws Excep
 		// Eclipse clean-up requires an existing source file
 		pkg.createCompilationUnit(SOURCE_NAME, "", true, null);
 
-		//
-		disableSecondaryTypes(project);
-
 		return jProject;
 	}
 
-	private static void disableSecondaryTypes(IProject project) {
-		JavaModelManager.PerProjectInfo info = JavaModelManager.getJavaModelManager().getPerProjectInfo(project, true);
-		info.secondaryTypes = new Hashtable<String, Map<String, IType>>();
-	}
-
-	public static ICompilationUnit createJavaSource(String contents, IJavaProject jProject) throws Exception {
+	ICompilationUnit createCompilationUnit(String contents, IJavaProject jProject) throws Exception {
 		IPackageFragmentRoot src = jProject.getPackageFragmentRoot(jProject.getProject());
 		IPackageFragment pkg = src.getPackageFragment(ROOT_AS_SRC);
 		return new RamCompilationUnit((PackageFragment) pkg, contents);
 	}
 
-	/** Spotless keeps compilation units in RAM as long as they are worked on. */
+	/** Keep compilation units in RAM */
 	private static class RamCompilationUnit extends CompilationUnit {
 
-		//Each RMA compilation unit has its own buffer manager. A drop is therefore prevented.
+		/*
+		 * Each RAM compilation unit has its own buffer manager to
+		 * prevent dropping of CUs when a maximum size is reached.
+		 */
 		private final RamBufferManager manager;
 
 		RamCompilationUnit(PackageFragment parent, String contents) {
@@ -153,7 +178,7 @@ protected BufferManager getBufferManager() {
 
 		@Override
 		public void save(IProgressMonitor pm, boolean force) throws JavaModelException {
-			//RAM CU is never saved to disk
+			//RAM CU is never stored on disk
 		}
 
 		@Override
@@ -167,7 +192,7 @@ public boolean equals(Object obj) {
 		}
 	}
 
-	/** Work-around required package privileges when adding buffer for manager singleton */
+	/** Work around package privileges */
 	private static class RamBufferManager extends BufferManager {
 		void add(IBuffer buffer) {
 			addBuffer(buffer);

_ext/eclipse-jdt/src/main/java/com/diffplug/spotless/extra/eclipse/java/JavaContentTypeManager.java

@@ -18,12 +18,15 @@
 import org.eclipse.core.internal.content.ContentType;
 import org.eclipse.core.internal.content.ContentTypeCatalog;
 import org.eclipse.core.runtime.content.IContentType;
+import org.eclipse.core.runtime.content.IContentTypeMatcher;
+import org.eclipse.core.runtime.preferences.IScopeContext;
 import org.eclipse.jdt.internal.compiler.util.SuffixConstants;
 
 import com.diffplug.spotless.extra.eclipse.base.service.NoContentTypeSpecificHandling;
 
 /**
  * Java compilation unit validation requires Java content type to be recognized.
+ * All source is assumed to be Java. A content description is not required/provided.
  * <p>
  * See {@code org.eclipse.jdt.internal.core.util.Util} for details.
  * </p>
@@ -57,4 +60,8 @@ public IContentType[] getAllContentTypes() {
 		return new IContentType[]{contentType};
 	}
 
+	@Override
+	public IContentTypeMatcher getMatcher(ISelectionPolicy customPolicy, IScopeContext context) {
+		return this;
+	}
 }

_ext/eclipse-jdt/src/test/java/com/diffplug/spotless/extra/eclipse/java/EclipseJdtCleanUpStepImplTest.java

@@ -20,6 +20,9 @@
 import java.util.Properties;
 import java.util.function.Consumer;
 
+import org.assertj.core.util.Arrays;
+import org.eclipse.jdt.core.JavaCore;
+import org.eclipse.jdt.core.manipulation.CodeStyleConfiguration;
 import org.junit.BeforeClass;
 import org.junit.Test;
 
@@ -38,8 +41,42 @@ public void emptyInput() throws Throwable {
 	}
 
 	@Test
-	public void nominal() throws Throwable {
-		organizeImportTest("Simple", config -> {});
+	public void defaultConfiguration() throws Throwable {
+		for(String testFile:Arrays.array("Simple", "Statics", "Wildcards")) {
+			organizeImportTest(testFile, config -> {});
+		}
+	}
+	
+	@Test
+	public void defaultPackage() throws Throwable {
+		String input = TEST_DATA.input("Simple").replaceFirst("package .+", "");
+		String expected = TEST_DATA.afterOrganizedImports("Simple").replaceFirst("package .+", "");
+		organizeImportTest(input, expected, config -> {});
+	}
+	
+	@Test
+	public void invalidConfiguration() throws Throwable {
+		//Smoke test, no exceptions expected
+		organizeImportTest("", "", config -> {
+			config.put("invalid.key", "some.value");
+		});
+		organizeImportTest("", "", config -> {
+			config.put(JavaCore.COMPILER_SOURCE, "-42");
+		});
+		organizeImportTest("", "", config -> {
+			config.put(JavaCore.COMPILER_SOURCE, "Not an integer");
+		});
+	}
+	
+	@Test
+	public void customConfiguration() throws Throwable {
+		String defaultOrganizedInput = TEST_DATA.input("Configuration");
+		organizeImportTest(defaultOrganizedInput, defaultOrganizedInput, config -> {});
+		
+		String customOrganizedOutput = TEST_DATA.afterOrganizedImports("Configuration");
+		organizeImportTest(defaultOrganizedInput, customOrganizedOutput, config -> {
+			config.put(CodeStyleConfiguration.ORGIMPORTS_IMPORTORDER, "foo;#foo;");
+		});
 	}
 
 	private static void organizeImportTest(final String fileName, final Consumer<Properties> config) throws Exception {

_ext/eclipse-jdt/src/test/resources/Configuration.input

@@ -0,0 +1,12 @@
+package bar.foo;
+
+import static foo.bar.B.someMethod; //Default configuration places static imports before none-static
+
+import foo.bar.B;
+
+class A {
+	static {
+		someMethod();
+		new B();
+	}
+}

_ext/eclipse-jdt/src/test/resources/Configuration.organized

@@ -0,0 +1,12 @@
+package bar.foo;
+
+import foo.bar.B;
+
+import static foo.bar.B.someMethod; //Default configuration places static imports before none-static
+
+class A {
+	static {
+		someMethod();
+		new B();
+	}
+}

_ext/eclipse-jdt/src/test/resources/Simple.input

@@ -5,10 +5,10 @@ import javax.net.SocketFactory; //Wrong order in Other group
 import java.net.Socket; //Wrong within group
 import java.lang.System; //Can be removed since implicitly imported
 import java.io.IOException;
-import java.io.PrintStream; //Can be found and is not used
+import java.io.PrintStream; //Class can be found but is not used
 
-import foo.bar.C; //Cannot be found and is not used
-import foo.bar.B; //Cannot be found but is used
+import foo.bar.C; //Class cannot be found and is not used
+import foo.bar.B; //Class cannot be found but is used
 
 
 class A {