Bug 570413
- Check the criteria is a comparable object
- Add JavaDoc
Change-Id: I989be4f006586059b190d4fd9b299194454a2e75
Signed-off-by: Fabrice Tiercelin <fabrice.tiercelin@yahoo.fr>
diff --git a/org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/util/ControlWorkflowMatcher.java b/org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/util/ControlWorkflowMatcher.java
index c16f1a6..4ef642a 100644
--- a/org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/util/ControlWorkflowMatcher.java
+++ b/org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/util/ControlWorkflowMatcher.java
@@ -35,6 +35,92 @@
/**
* Implementation of the control workflow builder.
+ *
+ * The aim is to match a conditional part of code like:
+ *
+ * <pre>
+ * if (c1) {
+ * if (c2) {
+ * return A;
+ * } else {
+ * return B;
+ * }
+ * } else {
+ * if (c3) {
+ * return C;
+ * } else {
+ * return D;
+ * }
+ * }
+ * </pre>
+ *
+ * Any cases should be handled (no remaining cases). It is expressed this way
+ * <ul>
+ * <li>One case (c1, c2) goes to A</li>
+ * <li>One case (c1, not c2) goes to B</li>
+ * <li>One case (not c1, c3) goes to C</li>
+ * <li>One case (not c1, not c3) goes to D</li>
+ * </ul>
+ *
+ * It can handle as many conditions as you want.
+ * The conditions and the expressions are analyzed as ordinary matcher using <code>org.eclipse.jdt.internal.corext.dom.ASTSemanticMatcher</code>.
+ * All the remaining part of the structure may have lots of different forms:
+ *
+ * <pre>
+ * if (c1 && c2) {
+ * return A;
+ * } else if (c1 && not c2) {
+ * return B;
+ * } else if (not c1 && not c3) {
+ * return D;
+ * } else if (not c1 && c3) {
+ * return C;
+ * }
+ * </pre>
+ *
+ * <pre>
+ * if (c1) {
+ * if (c2) {
+ * return A;
+ * }
+ * return B;
+ * } else {
+ * if (c3) {
+ * return C;
+ * }
+ * return D;
+ * }
+ * </pre>
+ *
+ * <pre>
+ * if (c1) {
+ * if (not c2) {
+ * return B;
+ * } else {
+ * return A;
+ * }
+ * } else {
+ * if (not c3) {
+ * return D;
+ * } else {
+ * return C;
+ * }
+ * }
+ * </pre>
+ *
+ * <pre>
+ * if (c1) {
+ * return c2 ? A : B;
+ * } else {
+ * return c3 ? C :D;
+ * }
+ *
+ * if (not c1) {
+ * return c3 ? C :D;
+ * }
+ *
+ * return not c2 ? A : B;
+ * </pre>
*/
public final class ControlWorkflowMatcher implements ControlWorkflowMatcherCompletable, ControlWorkflowMatcherRunnable {
/**
diff --git a/org.eclipse.jdt.ui.tests/ui/org/eclipse/jdt/ui/tests/quickfix/CleanUpTest1d8.java b/org.eclipse.jdt.ui.tests/ui/org/eclipse/jdt/ui/tests/quickfix/CleanUpTest1d8.java
index d1c1db8..4a7946a 100644
--- a/org.eclipse.jdt.ui.tests/ui/org/eclipse/jdt/ui/tests/quickfix/CleanUpTest1d8.java
+++ b/org.eclipse.jdt.ui.tests/ui/org/eclipse/jdt/ui/tests/quickfix/CleanUpTest1d8.java
@@ -1591,6 +1591,13 @@
+ "import java.util.Locale;\n" //
+ "\n" //
+ "public class E {\n" //
+ + " private Comparator<Date> refactorField = new Comparator<Date>() {\n" //
+ + " @Override\n" //
+ + " public int compare(Date o1, Date o2) {\n" //
+ + " return o1.toString().compareTo(o2.toString());\n" //
+ + " }\n" //
+ + " };\n" //
+ + "\n" //
+ " public List<Date> useMethodRef(List<Date> listToSort) {\n" //
+ " // Keep this comment\n" //
+ " Comparator<Date> comparator = new Comparator<Date>() {\n" //
@@ -1988,6 +1995,8 @@
+ "import java.util.Locale;\n" //
+ "\n" //
+ "public class E {\n" //
+ + " private Comparator<Date> refactorField = Comparator.comparing(Date::toString);\n" //
+ + "\n" //
+ " public List<Date> useMethodRef(List<Date> listToSort) {\n" //
+ " // Keep this comment\n" //
+ " Comparator<Date> comparator = Comparator.comparing(Date::toString);\n" //
@@ -2243,6 +2252,21 @@
+ " return comparator;\n" //
+ " }\n" //
+ "\n" //
+ + " public int compareTo(E anc) {\n" //
+ + " return 0;\n" //
+ + " }\n" //
+ + "\n" //
+ + " public E getNewInstance() {\n" //
+ + " return new E();\n" //
+ + " }\n" //
+ + "\n" //
+ + " private Comparator<E> doNotRefactorNotComparableObjects = new Comparator<E>() {\n" //
+ + " @Override\n" //
+ + " public int compare(E o1, E o2) {\n" //
+ + " return o1.getNewInstance().compareTo(o2.getNewInstance());\n" //
+ + " }\n" //
+ + " };\n" //
+ + "\n" //
+ " public Comparator<Date> doNotRemoveSecondaryMethod(List<Date> listToSort) {\n" //
+ " Comparator<Date> comparator = new Comparator<Date>() {\n" //
+ "\n" //
diff --git a/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/fix/ComparingOnCriteriaCleanUp.java b/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/fix/ComparingOnCriteriaCleanUp.java
index bc6b863..f7bfd73 100644
--- a/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/fix/ComparingOnCriteriaCleanUp.java
+++ b/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/fix/ComparingOnCriteriaCleanUp.java
@@ -353,6 +353,26 @@
return true;
}
+ private boolean isComparable(ITypeBinding classBinding) {
+ if ("java.lang.Comparable".equals(classBinding.getErasure().getQualifiedName())) { //$NON-NLS-1$
+ return true;
+ }
+
+ ITypeBinding superClass= classBinding.getSuperclass();
+
+ if (superClass != null && isComparable(superClass)) {
+ return true;
+ }
+
+ for (ITypeBinding binding : classBinding.getInterfaces()) {
+ if (isComparable(binding)) {
+ return true;
+ }
+ }
+
+ return false;
+ }
+
private boolean isReturnedExpressionToRefactor(final Expression returnExpression, final AtomicReference<Expression> criteria,
final AtomicBoolean isForward, final SimpleName name1,
final SimpleName name2) {
@@ -368,7 +388,7 @@
if (compareToMethod != null && compareToMethod.getExpression() != null) {
ITypeBinding comparisonType= compareToMethod.getExpression().resolveTypeBinding();
- if (comparisonType != null) {
+ if (comparisonType != null && isComparable(comparisonType)) {
if (compareToMethod.getExpression() != null
&& ASTNodes.usesGivenSignature(compareToMethod, comparisonType.getQualifiedName(), "compareTo", comparisonType.getQualifiedName())) { //$NON-NLS-1$
return isRefactorComparisonToRefactor(criteria, isForward, name1, name2, compareToMethod.getExpression(), (Expression) compareToMethod.arguments().get(0));
