Ensure unused suppress warnings cleanup uses it own compiler options (eclipse-jdt#2124)

* Ensure unused suppress warnings cleanup uses it own compiler options

- modify CleanUpRequirements to have a new boolean setting regarding
  separate compiler options and add new constructor that sets it
  explicitly (otherwise, it defaults to false)
- modify CleanUpRefactoring.CleanUpFixpointIterator to not
  merge options from a cleanup that requires its own options and
  also modify the next() method to use these special options for
  such a cleanup which must also request a fresh AST and must be
  at the end of the cleanup list with others that do the same
- fixes eclipse-jdt#2123

Author: jjohnstn

org.eclipse.jdt.core.manipulation/META-INF/MANIFEST.MF

@@ -3,7 +3,7 @@ Automatic-Module-Name: org.eclipse.jdt.core.manipulation
 Bundle-ManifestVersion: 2
 Bundle-Name: %pluginName
 Bundle-SymbolicName: org.eclipse.jdt.core.manipulation; singleton:=true
-Bundle-Version: 1.22.100.qualifier
+Bundle-Version: 1.23.0.qualifier
 Bundle-Vendor: %providerName
 Bundle-Activator: org.eclipse.jdt.internal.core.manipulation.JavaManipulationPlugin
 Bundle-Localization: plugin

org.eclipse.jdt.core.manipulation/common/org/eclipse/jdt/internal/ui/fix/UnusedSuppressWarningsCleanUp.java

@@ -13,6 +13,7 @@
  *******************************************************************************/
 package org.eclipse.jdt.internal.ui.fix;
 
+import java.util.Hashtable;
 import java.util.Map;
 
 import org.eclipse.core.runtime.CoreException;
@@ -60,7 +61,7 @@ public CleanUpRequirements getRequirements() {
 		boolean requireAST= requireAST();
 		Map<String, String> requiredOptions= requireAST ? getRequiredOptions() : null;
 		// ask for fresh AST as we are setting all default options on and we run as last cleanup
-		return new CleanUpRequirements(requireAST, requireAST, false, requiredOptions);
+		return new CleanUpRequirements(requireAST, requireAST, false, requireAST, requiredOptions);
 	}
 
 	private boolean requireAST() {
@@ -69,7 +70,7 @@ private boolean requireAST() {
 
 	@Override
 	protected ICleanUpFix createFix(CompilationUnit compilationUnit) throws CoreException {
-		if (compilationUnit == null)
+		if (compilationUnit == null || !isEnabled(CleanUpConstants.REMOVE_UNNECESSARY_SUPPRESS_WARNINGS))
 			return null;
 
 		ICleanUpFix coreFix= fLiteral != null ? UnusedSuppressWarningsFixCore.createAllFix(fSavedCompilationUnit == null ? compilationUnit : fSavedCompilationUnit,
@@ -79,20 +80,18 @@ protected ICleanUpFix createFix(CompilationUnit compilationUnit) throws CoreExce
 
 	@Override
 	protected ICleanUpFix createFix(CompilationUnit compilationUnit, IProblemLocation[] problems) throws CoreException {
-		if (compilationUnit == null)
+		if (compilationUnit == null || !isEnabled(CleanUpConstants.REMOVE_UNNECESSARY_SUPPRESS_WARNINGS))
 			return null;
 
 		ICleanUpFix coreFix= UnusedSuppressWarningsFixCore.createAllFix(compilationUnit, fLiteral);
 		return coreFix;
 	}
 
 	private Map<String, String> getRequiredOptions() {
-		// This cleanup is run last and can require using all default options to maximize
-		// chance of finding an unused suppress warnings annotation.
-		// We ask for a fresh AST so all other cleanups are complete before we do this
-		Map<String, String> result= JavaCore.getOptions();
+		Map<String, String> result= new Hashtable<>();
 
 		if (isEnabled(CleanUpConstants.REMOVE_UNNECESSARY_SUPPRESS_WARNINGS)) {
+			result.putAll(JavaCore.getDefaultOptions());
 			result.put(JavaCore.COMPILER_PB_SUPPRESS_WARNINGS, JavaCore.ENABLED);
 			result.put(JavaCore.COMPILER_PB_SUPPRESS_OPTIONAL_ERRORS, JavaCore.ENABLED);
 			result.put(JavaCore.COMPILER_PB_UNUSED_WARNING_TOKEN, JavaCore.ERROR); // do not change to WARNING

org.eclipse.jdt.core.manipulation/common/org/eclipse/jdt/ui/cleanup/CleanUpRequirements.java

@@ -37,6 +37,7 @@ public final class CleanUpRequirements {
 
 	protected final boolean fRequiresChangedRegions;
 
+	protected final boolean fRequiresSeparateOptions;
 
 	/**
 	 * Create a new instance
@@ -47,11 +48,28 @@ public final class CleanUpRequirements {
 	 * @param compilerOptions map of compiler options or <code>null</code> if no requirements
 	 */
 	public CleanUpRequirements(boolean requiresAST, boolean requiresFreshAST, boolean requiresChangedRegions, Map<String, String> compilerOptions) {
+		this(requiresAST, requiresFreshAST, requiresChangedRegions, false, compilerOptions);
+	}
+
+	/**
+	 * Create a new instance
+	 *
+	 * @param requiresAST <code>true</code> if an AST is required
+	 * @param requiresFreshAST <code>true</code> if a fresh AST is required
+	 * @param requiresChangedRegions <code>true</code> if changed regions are required
+	 * @param requiresSeparateOptions <code>true</code> if clean up has options that should not be shared
+	 * @param compilerOptions map of compiler options or <code>null</code> if no requirements
+	 * @since 1.23
+	 */
+	public CleanUpRequirements(boolean requiresAST, boolean requiresFreshAST, boolean requiresChangedRegions,
+			boolean requiresSeparateOptions, Map<String, String> compilerOptions) {
 		Assert.isLegal(!requiresFreshAST || requiresAST, "Must not request fresh AST if no AST is required"); //$NON-NLS-1$
 		Assert.isLegal(compilerOptions == null || requiresAST, "Must not provide options if no AST is required"); //$NON-NLS-1$
+		Assert.isLegal(!requiresSeparateOptions || requiresFreshAST, "Must not require separate options if fresh AST not required"); //$NON-NLS-1$
 		fRequiresAST= requiresAST;
 		fRequiresFreshAST= requiresFreshAST;
 		fRequiresChangedRegions= requiresChangedRegions;
+		fRequiresSeparateOptions= requiresSeparateOptions;
 
 		fCompilerOptions= compilerOptions;
 		// Make sure that compile warnings are not suppressed since some clean ups work on reported warnings
@@ -86,6 +104,17 @@ public boolean requiresFreshAST() {
 		return fRequiresFreshAST;
 	}
 
+	/**
+	 * Tells whether separate compiler options are required as the options should not be shared with
+	 * other cleanups.
+	 *
+	 * @return <code>true</code> if the cleanup has its own options.
+	 * @since 1.23
+	 */
+	public boolean requiresSeparateOptions() {
+		return fRequiresSeparateOptions;
+	}
+
 	/**
 	 * Required compiler options.
 	 *

org.eclipse.jdt.ui/core extension/org/eclipse/jdt/internal/corext/fix/CleanUpRefactoring.java

@@ -82,15 +82,16 @@
 import org.eclipse.jdt.ui.JavaElementLabels;
 import org.eclipse.jdt.ui.cleanup.CleanUpContext;
 import org.eclipse.jdt.ui.cleanup.CleanUpOptions;
+import org.eclipse.jdt.ui.cleanup.CleanUpRequirements;
 import org.eclipse.jdt.ui.cleanup.ICleanUp;
 import org.eclipse.jdt.ui.cleanup.ICleanUpFix;
+import org.eclipse.jdt.ui.text.java.IProblemLocation;
 
 import org.eclipse.jdt.internal.ui.IJavaStatusConstants;
 import org.eclipse.jdt.internal.ui.JavaPlugin;
 import org.eclipse.jdt.internal.ui.fix.IMultiFix.MultiFixContext;
 import org.eclipse.jdt.internal.ui.fix.MapCleanUpOptions;
 import org.eclipse.jdt.internal.ui.refactoring.IScheduledRefactoring;
-import org.eclipse.jdt.ui.text.java.IProblemLocation;
 import org.eclipse.jdt.internal.ui.util.Progress;
 
 public class CleanUpRefactoring extends Refactoring implements IScheduledRefactoring {
@@ -329,6 +330,7 @@ private class CleanUpFixpointIterator {
 		private final Hashtable<ICompilationUnit, List<CleanUpChange>> fSolutions;
 		private final Hashtable<ICompilationUnit, ICompilationUnit> fWorkingCopies; // map from primary to working copy
 		private final Map<String, String> fCleanUpOptions;
+		private final Map<String, String> fSeparateOptions;
 		private final int fSize;
 		private int fIndex;
 
@@ -343,11 +345,15 @@ public CleanUpFixpointIterator(CleanUpTarget[] targets, ICleanUp[] cleanUps) {
 
 			fCleanUpOptions= new Hashtable<>();
 			for (ICleanUp cleanUp : cleanUps) {
-				Map<String, String> currentCleanUpOption= cleanUp.getRequirements().getCompilerOptions();
-				if (currentCleanUpOption != null)
+				CleanUpRequirements requirements= cleanUp.getRequirements();
+				Map<String, String> currentCleanUpOption= requirements.getCompilerOptions();
+				boolean needsSeparateOptions= requirements.requiresSeparateOptions();
+				if (currentCleanUpOption != null && !needsSeparateOptions)
 					fCleanUpOptions.putAll(currentCleanUpOption);
 			}
 
+			fSeparateOptions= new Hashtable<>();
+
 			fSize= targets.length;
 			fIndex= 1;
 		}
@@ -390,7 +396,11 @@ protected ASTParser createParser(IJavaProject project) {
 							result.setProject(project);
 
 							Map<String, String> options= RefactoringASTParser.getCompilerOptions(project);
-							options.putAll(fCleanUpOptions);
+							if (!fSeparateOptions.isEmpty()) {
+								options.putAll(fSeparateOptions);
+							} else {
+								options.putAll(fCleanUpOptions);
+							}
 							result.setCompilerOptions(options);
 							return result;
 						}
@@ -403,6 +413,8 @@ protected ASTParser createParser(IJavaProject project) {
 					}
 				}
 
+				fSeparateOptions.clear();
+
 				for (ICompilationUnit cu : sourceList) {
 					monitor.worked(1);
 
@@ -413,6 +425,17 @@ protected ASTParser createParser(IJavaProject project) {
 				}
 
 				fParseList= requestor.getUndoneElements();
+				// check if undone cleanup requires separate options in which case, set up special options.
+				if (fParseList != null && !fParseList.isEmpty()) {
+					ParseListElement element= fParseList.get(0);
+					if (element.getCleanUps() != null && element.getCleanUps().length > 0) {
+						ICleanUp cleanUp= element.getCleanUps()[0];
+						if (cleanUp.getRequirements().requiresSeparateOptions()) {
+							fSeparateOptions.putAll(cleanUp.getRequirements().getCompilerOptions());
+						}
+					}
+				}
+
 				fIndex= cuMonitor.getIndex();
 			} finally {
 			}