From d2b1f518c8d11102689ea991819620ece87359d2 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Mon, 25 Nov 2024 15:26:07 +0100 Subject: [PATCH] [java-refactoring] IDEA-360614 Replace with single implementation: no downcast when 'this' is used (cherry picked from commit f2ea406a5cf229cf9e63b6c2b53b9d86f245a6d9) IJ-CR-150207 GitOrigin-RevId: 0eec797e7e9d105bf594aabd85533bd5c5395dd1 --- .../codeInsight/ChangeContextUtil.java | 49 ++++++++++--------- .../inline/InlineMethodProcessor.java | 8 ++- .../InlineSingleImplementationCastOnThis.java | 20 ++++++++ ...eSingleImplementationCastOnThis.java.after | 20 ++++++++ ...leImplementationCastOnThisUnnecessary.java | 17 +++++++ ...ementationCastOnThisUnnecessary.java.after | 18 +++++++ .../refactoring/inline/InlineMethodTest.java | 10 ++++ 7 files changed, 119 insertions(+), 23 deletions(-) rename java/{java-psi-impl => java-analysis-impl}/src/com/intellij/codeInsight/ChangeContextUtil.java (90%) create mode 100644 java/java-tests/testData/refactoring/inlineMethod/InlineSingleImplementationCastOnThis.java create mode 100644 java/java-tests/testData/refactoring/inlineMethod/InlineSingleImplementationCastOnThis.java.after create mode 100644 java/java-tests/testData/refactoring/inlineMethod/InlineSingleImplementationCastOnThisUnnecessary.java create mode 100644 java/java-tests/testData/refactoring/inlineMethod/InlineSingleImplementationCastOnThisUnnecessary.java.after diff --git a/java/java-psi-impl/src/com/intellij/codeInsight/ChangeContextUtil.java b/java/java-analysis-impl/src/com/intellij/codeInsight/ChangeContextUtil.java similarity index 90% rename from java/java-psi-impl/src/com/intellij/codeInsight/ChangeContextUtil.java rename to java/java-analysis-impl/src/com/intellij/codeInsight/ChangeContextUtil.java index 4a9a09f9f8a8..ce0b910cb114 100644 --- a/java/java-psi-impl/src/com/intellij/codeInsight/ChangeContextUtil.java +++ b/java/java-analysis-impl/src/com/intellij/codeInsight/ChangeContextUtil.java @@ -5,9 +5,7 @@ import com.intellij.lang.ASTNode; import com.intellij.openapi.diagnostic.Logger; import com.intellij.openapi.util.Key; import com.intellij.psi.*; -import com.intellij.psi.util.InheritanceUtil; -import com.intellij.psi.util.MethodSignatureUtil; -import com.intellij.psi.util.PsiTreeUtil; +import com.intellij.psi.util.*; import com.intellij.refactoring.util.RefactoringChangeUtil; import com.intellij.util.IncorrectOperationException; import org.jetbrains.annotations.NotNull; @@ -45,10 +43,9 @@ public final class ChangeContextUtil { scope.putUserData(HARD_REF_TO_AST, scope.getNode()); } - if (scope instanceof PsiThisExpression){ + if (scope instanceof PsiThisExpression thisExpr){ scope.putCopyableUserData(ENCODED_KEY, ""); - PsiThisExpression thisExpr = (PsiThisExpression)scope; final PsiJavaCodeReferenceElement qualifier = thisExpr.getQualifier(); if (qualifier == null){ PsiClass thisClass = RefactoringChangeUtil.getThisClass(thisExpr); @@ -63,10 +60,9 @@ public final class ChangeContextUtil { } } } - else if (scope instanceof PsiReferenceExpression){ + else if (scope instanceof PsiReferenceExpression refExpr){ scope.putCopyableUserData(ENCODED_KEY, ""); - PsiReferenceExpression refExpr = (PsiReferenceExpression)scope; PsiExpression qualifier = refExpr.getQualifierExpression(); if (qualifier == null){ final JavaResolveResult resolveResult = refExpr.advancedResolve(false); @@ -117,8 +113,7 @@ public final class ChangeContextUtil { if (scope.getCopyableUserData(ENCODED_KEY) != null) { scope.putCopyableUserData(ENCODED_KEY, null); - if (scope instanceof PsiThisExpression) { - PsiThisExpression thisExpr = (PsiThisExpression)scope; + if (scope instanceof PsiThisExpression thisExpr) { scope = decodeThisExpression(thisExpr, thisClass, thisAccessExpr); } else if (scope instanceof PsiReferenceExpression) { @@ -172,7 +167,7 @@ public final class ChangeContextUtil { return thisExpr; } } - return thisExpr.replace(thisAccessExpr); + return maybeRemoveCast((PsiExpression)thisExpr.replace(thisAccessExpr)); } } } @@ -184,7 +179,7 @@ public final class ChangeContextUtil { else { if (qualifierClass != null) { if (qualifierClass.equals(thisClass) && thisAccessExpr != null && thisAccessExpr.isValid()) { - return thisExpr.replace(thisAccessExpr); + return maybeRemoveCast((PsiExpression)thisExpr.replace(thisAccessExpr)); } } } @@ -192,6 +187,19 @@ public final class ChangeContextUtil { return thisExpr; } + private static PsiExpression maybeRemoveCast(@NotNull PsiExpression expression) { + if (PsiUtil.skipParenthesizedExprDown(expression) instanceof PsiTypeCastExpression cast && + RedundantCastUtil.isCastRedundant(cast)) { + expression = (PsiExpression)expression.replace(Objects.requireNonNull(cast.getOperand())); + } + if (expression.getParent() instanceof PsiParenthesizedExpression parens && + parens.getParent() instanceof PsiExpression grandParent && + PsiPrecedenceUtil.areParenthesesNeeded(grandParent, expression, false)) { + return (PsiExpression)parens.replace(expression); + } + return expression; + } + private static PsiReferenceExpression decodeReferenceExpression(@NotNull PsiReferenceExpression refExpr, PsiExpression thisAccessExpr, PsiClass thisClass) { @@ -224,14 +232,14 @@ public final class ChangeContextUtil { if (refMember.equals(refElement) || (refElement instanceof PsiMethod && refMember instanceof PsiMethod && MethodSignatureUtil.isSuperMethod((PsiMethod)refMember, (PsiMethod)refElement))) { - if (thisAccessExpr instanceof PsiThisExpression && ((PsiThisExpression)thisAccessExpr).getQualifier() == null) { + if (thisAccessExpr instanceof PsiThisExpression thisExpression && thisExpression.getQualifier() == null) { //Trivial qualifier needQualifier = false; } else { final PsiClass currentClass = findThisClass(refExpr, refMember); - if (thisAccessExpr instanceof PsiThisExpression){ - PsiJavaCodeReferenceElement thisQualifier = ((PsiThisExpression)thisAccessExpr).getQualifier(); + if (thisAccessExpr instanceof PsiThisExpression thisExpression){ + PsiJavaCodeReferenceElement thisQualifier = thisExpression.getQualifier(); PsiClass thisExprClass = thisQualifier != null ? (PsiClass)thisQualifier.resolve() : RefactoringChangeUtil.getThisClass(refExpr); @@ -244,6 +252,7 @@ public final class ChangeContextUtil { if (needQualifier){ refExpr.setQualifierExpression(thisAccessExpr); + maybeRemoveCast(refExpr.getQualifierExpression()); } } else if (thisClass != null && realParentClass != null && PsiTreeUtil.isAncestor(realParentClass, thisClass, true)) { @@ -267,8 +276,7 @@ public final class ChangeContextUtil { refExpr.putCopyableUserData(CAN_REMOVE_QUALIFIER_KEY, null); if (couldRemove == Boolean.FALSE && canRemoveQualifier(refExpr)){ - PsiReferenceExpression newRefExpr = (PsiReferenceExpression)factory.createExpressionFromText( - refExpr.getReferenceName(), null); + PsiReferenceExpression newRefExpr = (PsiReferenceExpression)factory.createExpressionFromText(refExpr.getReferenceName(), null); refExpr = (PsiReferenceExpression)refExpr.replace(newRefExpr); } } @@ -302,8 +310,7 @@ public final class ChangeContextUtil { PsiElement refElement = refExpr.resolve(); if (refElement == null) return false; PsiElementFactory factory = JavaPsiFacade.getElementFactory(refExpr.getProject()); - if (refExpr.getParent() instanceof PsiMethodCallExpression){ - PsiMethodCallExpression methodCall = (PsiMethodCallExpression)refExpr.getParent(); + if (refExpr.getParent() instanceof PsiMethodCallExpression methodCall){ PsiMethodCallExpression newMethodCall = (PsiMethodCallExpression)factory.createExpressionFromText( refExpr.getReferenceName() + "()", refExpr); newMethodCall.getArgumentList().replace(methodCall.getArgumentList()); @@ -314,8 +321,7 @@ public final class ChangeContextUtil { return false; } else { - PsiReferenceExpression newRefExpr = (PsiReferenceExpression)factory.createExpressionFromText( - refExpr.getReferenceName(), refExpr); + PsiReferenceExpression newRefExpr = (PsiReferenceExpression)factory.createExpressionFromText(refExpr.getReferenceName(), refExpr); PsiElement newRefElement = newRefExpr.resolve(); return refElement.equals(newRefElement); } @@ -327,8 +333,7 @@ public final class ChangeContextUtil { } private static PsiElement qualifyThis(PsiElement scope, PsiClass thisClass) throws IncorrectOperationException { - if (scope instanceof PsiThisExpression){ - PsiThisExpression thisExpr = (PsiThisExpression)scope; + if (scope instanceof PsiThisExpression thisExpr){ if (thisExpr.getQualifier() == null){ if (thisClass instanceof PsiAnonymousClass) return null; return RefactoringChangeUtil.createThisExpression(thisClass.getManager(), thisClass); diff --git a/java/java-impl-refactorings/src/com/intellij/refactoring/inline/InlineMethodProcessor.java b/java/java-impl-refactorings/src/com/intellij/refactoring/inline/InlineMethodProcessor.java index 5fe1a72a1665..7ea002b7595b 100644 --- a/java/java-impl-refactorings/src/com/intellij/refactoring/inline/InlineMethodProcessor.java +++ b/java/java-impl-refactorings/src/com/intellij/refactoring/inline/InlineMethodProcessor.java @@ -885,7 +885,13 @@ public class InlineMethodProcessor extends BaseRefactoringProcessor { else if (qualifier instanceof PsiSuperExpression) { qualifier = myFactory.createExpressionFromText("this", null); } - thisVar.getInitializer().replace(qualifier); + else if (qualifier.getType() != null && !thisVar.getType().isAssignableFrom(qualifier.getType())) { + PsiTypeCastExpression cast = (PsiTypeCastExpression)myFactory.createExpressionFromText("(A)b", null); + Objects.requireNonNull(cast.getOperand()).replace(qualifier); + Objects.requireNonNull(cast.getCastType()).replace(myFactory.createTypeElement(thisVar.getType())); + qualifier = cast; + } + Objects.requireNonNull(thisVar.getInitializer()).replace(qualifier); } } diff --git a/java/java-tests/testData/refactoring/inlineMethod/InlineSingleImplementationCastOnThis.java b/java/java-tests/testData/refactoring/inlineMethod/InlineSingleImplementationCastOnThis.java new file mode 100644 index 000000000000..a60e65470e5e --- /dev/null +++ b/java/java-tests/testData/refactoring/inlineMethod/InlineSingleImplementationCastOnThis.java @@ -0,0 +1,20 @@ +public class InlineSingleImplementation { + interface MyIface { + void mySimpleMethod(); + } + + static class MyIfaceImpl implements MyIface { + @Override + public void mySimpleMethod() { + extracted(); + } + + private void extracted() { + System.out.println("Impl"); + } + } + + void test(MyIface iface) { + iface.mySimpleMethod(); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/inlineMethod/InlineSingleImplementationCastOnThis.java.after b/java/java-tests/testData/refactoring/inlineMethod/InlineSingleImplementationCastOnThis.java.after new file mode 100644 index 000000000000..1d841132af69 --- /dev/null +++ b/java/java-tests/testData/refactoring/inlineMethod/InlineSingleImplementationCastOnThis.java.after @@ -0,0 +1,20 @@ +public class InlineSingleImplementation { + interface MyIface { + void mySimpleMethod(); + } + + static class MyIfaceImpl implements MyIface { + @Override + public void mySimpleMethod() { + extracted(); + } + + private void extracted() { + System.out.println("Impl"); + } + } + + void test(MyIface iface) { + ((MyIfaceImpl) iface).extracted(); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/inlineMethod/InlineSingleImplementationCastOnThisUnnecessary.java b/java/java-tests/testData/refactoring/inlineMethod/InlineSingleImplementationCastOnThisUnnecessary.java new file mode 100644 index 000000000000..126c7f74f6da --- /dev/null +++ b/java/java-tests/testData/refactoring/inlineMethod/InlineSingleImplementationCastOnThisUnnecessary.java @@ -0,0 +1,17 @@ +public class InlineSingleImplementation { + interface MyIface { + void mySimpleMethod(); + } + + static class MyIfaceImpl implements MyIface { + @Override + public void mySimpleMethod() { + System.out.println(toString()); + System.out.println(this.toString()); + } + } + + void test(MyIface iface) { + iface.mySimpleMethod(); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/inlineMethod/InlineSingleImplementationCastOnThisUnnecessary.java.after b/java/java-tests/testData/refactoring/inlineMethod/InlineSingleImplementationCastOnThisUnnecessary.java.after new file mode 100644 index 000000000000..72538657f02c --- /dev/null +++ b/java/java-tests/testData/refactoring/inlineMethod/InlineSingleImplementationCastOnThisUnnecessary.java.after @@ -0,0 +1,18 @@ +public class InlineSingleImplementation { + interface MyIface { + void mySimpleMethod(); + } + + static class MyIfaceImpl implements MyIface { + @Override + public void mySimpleMethod() { + System.out.println(toString()); + System.out.println(this.toString()); + } + } + + void test(MyIface iface) { + System.out.println(iface.toString()); + System.out.println(iface.toString()); + } +} \ No newline at end of file diff --git a/java/java-tests/testSrc/com/intellij/java/refactoring/inline/InlineMethodTest.java b/java/java-tests/testSrc/com/intellij/java/refactoring/inline/InlineMethodTest.java index 0dd49f0e16be..b4e4c17a508b 100644 --- a/java/java-tests/testSrc/com/intellij/java/refactoring/inline/InlineMethodTest.java +++ b/java/java-tests/testSrc/com/intellij/java/refactoring/inline/InlineMethodTest.java @@ -612,6 +612,16 @@ public class InlineMethodTest extends LightRefactoringTestCase { TestDialogManager.setTestDialog(TestDialog.YES, getTestRootDisposable()); BaseRefactoringProcessor.ConflictsInTestsException.withIgnoredConflicts(() -> doTest()); } + + public void testInlineSingleImplementationCastOnThis() { + TestDialogManager.setTestDialog(TestDialog.YES, getTestRootDisposable()); + doTest(); + } + + public void testInlineSingleImplementationCastOnThisUnnecessary() { + TestDialogManager.setTestDialog(TestDialog.YES, getTestRootDisposable()); + doTest(); + } @Override protected Sdk getProjectJDK() {