From d8a3f626b673ac8990edb76d1cbb5bee87a4581c Mon Sep 17 00:00:00 2001 From: Ilyas Selimov Date: Fri, 3 Dec 2021 13:55:33 +0700 Subject: [PATCH] IDEA-251722 - remove plain hashCode method during class-to-record conversion GitOrigin-RevId: 1db63c1bdeec14f3ec6ef2aced3e089572633dcc --- .../classCanBeRecord/ConvertToRecordFix.java | 22 ++++ .../ConvertToRecordProcessor.java | 123 +++++++++++++++++- .../afterObjectsHashNegative.java | 10 ++ .../afterObjectsHashPositive.java | 5 + .../afterOldStyleHashNegative.java | 11 ++ .../afterOldStyleHashPositive.java | 4 + .../beforeObjectsHashNegative.java | 17 +++ .../beforeObjectsHashPositive.java | 17 +++ .../beforeOldStyleHashNegative.java | 13 ++ .../beforeOldStyleHashPositive.java | 26 ++++ 10 files changed, 247 insertions(+), 1 deletion(-) create mode 100644 java/java-tests/testData/inspection/classCanBeRecord/afterObjectsHashNegative.java create mode 100644 java/java-tests/testData/inspection/classCanBeRecord/afterObjectsHashPositive.java create mode 100644 java/java-tests/testData/inspection/classCanBeRecord/afterOldStyleHashNegative.java create mode 100644 java/java-tests/testData/inspection/classCanBeRecord/afterOldStyleHashPositive.java create mode 100644 java/java-tests/testData/inspection/classCanBeRecord/beforeObjectsHashNegative.java create mode 100644 java/java-tests/testData/inspection/classCanBeRecord/beforeObjectsHashPositive.java create mode 100644 java/java-tests/testData/inspection/classCanBeRecord/beforeOldStyleHashNegative.java create mode 100644 java/java-tests/testData/inspection/classCanBeRecord/beforeOldStyleHashPositive.java diff --git a/java/java-impl-inspections/src/com/intellij/codeInspection/classCanBeRecord/ConvertToRecordFix.java b/java/java-impl-inspections/src/com/intellij/codeInspection/classCanBeRecord/ConvertToRecordFix.java index 89444770e683..89830dd8800f 100644 --- a/java/java-impl-inspections/src/com/intellij/codeInspection/classCanBeRecord/ConvertToRecordFix.java +++ b/java/java-impl-inspections/src/com/intellij/codeInspection/classCanBeRecord/ConvertToRecordFix.java @@ -22,6 +22,7 @@ import com.intellij.util.containers.ContainerUtil; import com.intellij.util.containers.MultiMap; import com.siyeh.ig.InspectionGadgetsFix; import com.siyeh.ig.callMatcher.CallMatcher; +import com.siyeh.ig.psiutils.MethodUtils; import com.siyeh.ig.psiutils.TypeUtils; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -116,6 +117,9 @@ public class ConvertToRecordFix extends InspectionGadgetsFix { private final MultiMap myFieldAccessors = new MultiMap<>(new LinkedHashMap<>()); private final List myOrdinaryMethods = new SmartList<>(); private final List myConstructors = new SmartList<>(); + + private PsiMethod myEqualsMethod; + private PsiMethod myHashCodeMethod; private Map myFieldAccessorsCache; @@ -153,6 +157,16 @@ public class ConvertToRecordFix extends InspectionGadgetsFix { return myConstructors.size() == 1 ? myConstructors.get(0).myConstructor : null; } + @Nullable + PsiMethod getEqualsMethod() { + return myEqualsMethod; + } + + @Nullable + PsiMethod getHashCodeMethod() { + return myHashCodeMethod; + } + private boolean isValid() { if (myConstructors.size() > 1) return false; if (myConstructors.size() == 1) { @@ -190,6 +204,14 @@ public class ConvertToRecordFix extends InspectionGadgetsFix { myConstructors.add(new RecordConstructorCandidate(method, myFieldAccessors.keySet())); continue; } + if (MethodUtils.isEquals(method)) { + myEqualsMethod = method; + continue; + } + if (MethodUtils.isHashCode(method)) { + myHashCodeMethod = method; + continue; + } if (!throwsOnlyUncheckedExceptions(method)) { myOrdinaryMethods.add(method); continue; diff --git a/java/java-impl-inspections/src/com/intellij/codeInspection/classCanBeRecord/ConvertToRecordProcessor.java b/java/java-impl-inspections/src/com/intellij/codeInspection/classCanBeRecord/ConvertToRecordProcessor.java index 53ae6a31edae..0026f9a8d6f2 100644 --- a/java/java-impl-inspections/src/com/intellij/codeInspection/classCanBeRecord/ConvertToRecordProcessor.java +++ b/java/java-impl-inspections/src/com/intellij/codeInspection/classCanBeRecord/ConvertToRecordProcessor.java @@ -14,6 +14,7 @@ import com.intellij.psi.*; import com.intellij.psi.codeStyle.CodeStyleManager; import com.intellij.psi.javadoc.PsiDocComment; import com.intellij.psi.javadoc.PsiDocTag; +import com.intellij.psi.tree.IElementType; import com.intellij.psi.util.PsiTreeUtil; import com.intellij.refactoring.BaseRefactoringProcessor; import com.intellij.refactoring.rename.RenameProcessor; @@ -28,6 +29,7 @@ import com.intellij.util.*; import com.intellij.util.containers.ContainerUtil; import com.intellij.util.containers.MultiMap; import com.intellij.util.containers.SmartHashSet; +import com.siyeh.ig.callMatcher.CallMatcher; import one.util.streamex.StreamEx; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -219,9 +221,10 @@ public class ConvertToRecordProcessor extends BaseRefactoringProcessor { } nextElement = nextElement.getNextSibling(); } - + List redundantObjectMethods = findRedundantObjectMethods(); PsiClass result = (PsiClass)psiClass.replace(recordBuilder.build()); tryToCompactCanonicalCtor(result); + removeRedundantObjectMethods(result, redundantObjectMethods); generateJavaDocForDocumentedFields(result); CodeStyleManager.getInstance(myProject).reformat(result); } @@ -237,6 +240,118 @@ public class ConvertToRecordProcessor extends BaseRefactoringProcessor { } } + @NotNull + private List findRedundantObjectMethods() { + PsiMethod equalsMethod = myRecordCandidate.getEqualsMethod(); + PsiMethod hashCodeMethod = myRecordCandidate.getHashCodeMethod(); + if (equalsMethod == null && hashCodeMethod == null) return Collections.emptyList(); + List result = new SmartList<>(); + Set fields = myRecordCandidate.getFieldAccessors().keySet(); + // todo for equals method + if (hashCodeMethod != null) { + var hashCodeVisitor = new HashCodeVisitor(fields); + hashCodeMethod.accept(hashCodeVisitor); + if (hashCodeVisitor.myNonVisitedFields.isEmpty()) { + result.add(hashCodeMethod); + } + } + return result; + } + + private static class HashCodeVisitor extends JavaRecursiveElementWalkingVisitor { + private final CallMatcher OBJECTS_CALL = CallMatcher.staticCall(CommonClassNames.JAVA_UTIL_OBJECTS, "hash", "hashCode"); + private final CallMatcher FLOAT_CALL = CallMatcher.staticCall(CommonClassNames.JAVA_LANG_FLOAT, "floatToIntBits"); + private final CallMatcher DOUBLE_CALL = CallMatcher.staticCall(CommonClassNames.JAVA_LANG_DOUBLE, "doubleToLongBits"); + + private final CallMatcher INSTANCE_CALL = CallMatcher.instanceCall(CommonClassNames.JAVA_LANG_OBJECT, "hashCode"); + + private final Set myNonVisitedFields; + + private HashCodeVisitor(@NotNull Set fields) { + myNonVisitedFields = new SmartHashSet<>(fields); + } + + @Override + public void visitReferenceExpression(PsiReferenceExpression expression) { + PsiField field = ObjectUtils.tryCast(expression.resolve(), PsiField.class); + if (field == null) return; + PsiType fieldType = field.getType(); + if (PsiType.CHAR.equals(fieldType) || PsiType.SHORT.equals(fieldType)) { + PsiElement parent = PsiTreeUtil.skipParentsOfType(expression, PsiParenthesizedExpression.class); + if (parent instanceof PsiTypeCastExpression) { + PsiTypeElement castType = ((PsiTypeCastExpression)parent).getCastType(); + if (castType != null && PsiType.INT.equals(castType.getType())) { + myNonVisitedFields.remove(field); + } + } + } + else { + myNonVisitedFields.remove(field); + } + } + + @Override + public void visitMethodCallExpression(PsiMethodCallExpression expression) { + if (OBJECTS_CALL.test(expression)) { + for (PsiExpression argument : expression.getArgumentList().getExpressions()) { + PsiReferenceExpression fieldRef = ObjectUtils.tryCast(argument, PsiReferenceExpression.class); + if (fieldRef == null || !myNonVisitedFields.remove(fieldRef.resolve())) { + stopWalking(); + return; + } + } + return; + } + + PsiType expectedType = null; + if (FLOAT_CALL.test(expression)) { + expectedType = PsiType.FLOAT; + } + else if (DOUBLE_CALL.test(expression)) { + expectedType = PsiType.DOUBLE; + } + if (expectedType != null) { + PsiExpression[] expressions = expression.getArgumentList().getExpressions(); + if (expressions.length == 1) { + PsiReferenceExpression refExpr = ObjectUtils.tryCast(expressions[0], PsiReferenceExpression.class); + if (refExpr != null) { + PsiField field = ObjectUtils.tryCast(refExpr.resolve(), PsiField.class); + if (field != null && expectedType.equals(field.getType())) { + myNonVisitedFields.remove(field); + return; + } + } + } + stopWalking(); + return; + } + + if (INSTANCE_CALL.test(expression)) { + PsiExpression qualifier = expression.getMethodExpression().getQualifierExpression(); + PsiReferenceExpression fieldRef = ObjectUtils.tryCast(qualifier, PsiReferenceExpression.class); + if (fieldRef != null) { + PsiField field = ObjectUtils.tryCast(fieldRef.resolve(), PsiField.class); + if (field != null) { + myNonVisitedFields.remove(field); + return; + } + } + } + stopWalking(); + } + + @Override + public void visitPolyadicExpression(PsiPolyadicExpression expression) { + super.visitPolyadicExpression(expression); + IElementType operationType = expression.getOperationTokenType(); + if (!JavaTokenType.PLUS.equals(operationType) && !JavaTokenType.MINUS.equals(operationType) && + !JavaTokenType.ASTERISK.equals(operationType) && !JavaTokenType.GTGTGT.equals(operationType) && + !JavaTokenType.EQEQ.equals(operationType) && !JavaTokenType.NE.equals(operationType)) { + stopWalking(); + } + } + } + private static void tryToCompactCanonicalCtor(@NotNull PsiClass record) { PsiMethod canonicalCtor = ArrayUtil.getFirstElement(record.getConstructors()); if (canonicalCtor != null) { @@ -254,6 +369,12 @@ public class ConvertToRecordProcessor extends BaseRefactoringProcessor { } } + private static void removeRedundantObjectMethods(@NotNull PsiClass record, @NotNull List redundantObjectMethods) { + redundantObjectMethods.stream().map(method -> record.findMethodBySignature(method, false)) + .filter(Objects::nonNull) + .forEach(PsiMethod::delete); + } + private void generateJavaDocForDocumentedFields(@NotNull PsiClass record) { Map comments = new LinkedHashMap<>(); for (PsiField field : myRecordCandidate.getFieldAccessors().keySet()) { diff --git a/java/java-tests/testData/inspection/classCanBeRecord/afterObjectsHashNegative.java b/java/java-tests/testData/inspection/classCanBeRecord/afterObjectsHashNegative.java new file mode 100644 index 000000000000..cd6f588ea70a --- /dev/null +++ b/java/java-tests/testData/inspection/classCanBeRecord/afterObjectsHashNegative.java @@ -0,0 +1,10 @@ +// "Convert to a record" "true" +import java.util.Objects; + +record Test(boolean booleanValue, char charValue, String stringValue, long longValue, float floatValue, + double doubleValue, double[] arrayValue) { + @Override + public int hashCode() { + return Objects.hash(booleanValue, stringValue, longValue, floatValue, doubleValue, arrayValue); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/classCanBeRecord/afterObjectsHashPositive.java b/java/java-tests/testData/inspection/classCanBeRecord/afterObjectsHashPositive.java new file mode 100644 index 000000000000..b8c7986a495c --- /dev/null +++ b/java/java-tests/testData/inspection/classCanBeRecord/afterObjectsHashPositive.java @@ -0,0 +1,5 @@ +// "Convert to a record" "true" + +record Test(boolean booleanValue, char charValue, String stringValue, long longValue, float floatValue, + double doubleValue, double[] arrayValue) { +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/classCanBeRecord/afterOldStyleHashNegative.java b/java/java-tests/testData/inspection/classCanBeRecord/afterOldStyleHashNegative.java new file mode 100644 index 000000000000..949266f12a83 --- /dev/null +++ b/java/java-tests/testData/inspection/classCanBeRecord/afterOldStyleHashNegative.java @@ -0,0 +1,11 @@ +// "Convert to a record" "true" +record Test(double[] arrayValue) { + @Override + public int hashCode() { + int result = 17; + + result = 37 * result + Arrays.hashCode(arrayValue); + + return result; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/classCanBeRecord/afterOldStyleHashPositive.java b/java/java-tests/testData/inspection/classCanBeRecord/afterOldStyleHashPositive.java new file mode 100644 index 000000000000..e018771311f1 --- /dev/null +++ b/java/java-tests/testData/inspection/classCanBeRecord/afterOldStyleHashPositive.java @@ -0,0 +1,4 @@ +// "Convert to a record" "true" +record Test(boolean booleanValue, char charValue, String stringValue, long longValue, float floatValue, + double doubleValue, double[] arrayValue) { +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/classCanBeRecord/beforeObjectsHashNegative.java b/java/java-tests/testData/inspection/classCanBeRecord/beforeObjectsHashNegative.java new file mode 100644 index 000000000000..c6c68be913ca --- /dev/null +++ b/java/java-tests/testData/inspection/classCanBeRecord/beforeObjectsHashNegative.java @@ -0,0 +1,17 @@ +// "Convert to a record" "true" +import java.util.Objects; + +class Test { + final boolean booleanValue; + final char charValue; + final String stringValue; + final long longValue; + final float floatValue; + final double doubleValue; + final double[] arrayValue; + + @Override + public int hashCode() { + return Objects.hash(booleanValue, stringValue, longValue, floatValue, doubleValue, arrayValue); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/classCanBeRecord/beforeObjectsHashPositive.java b/java/java-tests/testData/inspection/classCanBeRecord/beforeObjectsHashPositive.java new file mode 100644 index 000000000000..e8aea86c2b44 --- /dev/null +++ b/java/java-tests/testData/inspection/classCanBeRecord/beforeObjectsHashPositive.java @@ -0,0 +1,17 @@ +// "Convert to a record" "true" +import java.util.Objects; + +class Test { + final boolean booleanValue; + final char charValue; + final String stringValue; + final long longValue; + final float floatValue; + final double doubleValue; + final double[] arrayValue; + + @Override + public int hashCode() { + return Objects.hash(booleanValue, charValue, stringValue, longValue, floatValue, doubleValue, arrayValue); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/classCanBeRecord/beforeOldStyleHashNegative.java b/java/java-tests/testData/inspection/classCanBeRecord/beforeOldStyleHashNegative.java new file mode 100644 index 000000000000..dedadc0b18af --- /dev/null +++ b/java/java-tests/testData/inspection/classCanBeRecord/beforeOldStyleHashNegative.java @@ -0,0 +1,13 @@ +// "Convert to a record" "true" +class Test { + final double[] arrayValue; + + @Override + public int hashCode() { + int result = 17; + + result = 37 * result + Arrays.hashCode(arrayValue); + + return result; + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/classCanBeRecord/beforeOldStyleHashPositive.java b/java/java-tests/testData/inspection/classCanBeRecord/beforeOldStyleHashPositive.java new file mode 100644 index 000000000000..87e311cd9a3c --- /dev/null +++ b/java/java-tests/testData/inspection/classCanBeRecord/beforeOldStyleHashPositive.java @@ -0,0 +1,26 @@ +// "Convert to a record" "true" +class Test { + final boolean booleanValue; + final char charValue; + final String stringValue; + final long longValue; + final float floatValue; + final double doubleValue; + final double[] arrayValue; + + @Override + public int hashCode() { + int result = 17; + + result = 37 * result + (booleanValue ? 1 : 0); + result = 37 * result + (int) charValue; + result = 37 * result + (stringValue == null ? 0 : stringValue.hashCode()); + result = 37 * result + (int) (longValue - (longValue >>> 32)); + result = 37 * result + Float.floatToIntBits(floatValue); + result = 37 * result + (int) (Double.doubleToLongBits(doubleValue) - (Double.doubleToLongBits(doubleValue) >>> 32)); + + result = 37 * result + arrayValue.hashCode(); + + return result; + } +} \ No newline at end of file