From bd411cb482b04787cd2533ab5d15aca01877bcc4 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Fri, 7 Oct 2022 17:21:27 +0200 Subject: [PATCH] [java-refactoring] Inline method: do not inline parameters that have non-pure method call in initializer To compensate somewhat, inline parameters that are executed as the first expression inside the method (or previous expressions are harmless) GitOrigin-RevId: ff4bc5ce45bfb3e12a83dc8ccb97b1767558bcbe --- .../inline/InlineMethodProcessor.java | 12 --- .../intellij/refactoring/util/InlineUtil.java | 82 ++++++++++++++++++- .../inlineMethod/NewWithSideEffect.java | 21 +++++ .../inlineMethod/NewWithSideEffect.java.after | 18 ++++ .../inlineMethod/NotAStatement.java.after | 3 +- .../refactoring/inline/InlineMethodTest.java | 2 + 6 files changed, 122 insertions(+), 16 deletions(-) create mode 100644 java/java-tests/testData/refactoring/inlineMethod/NewWithSideEffect.java create mode 100644 java/java-tests/testData/refactoring/inlineMethod/NewWithSideEffect.java.after 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 8e78ebf958ae..b2f4e1e26ece 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 @@ -755,18 +755,6 @@ public class InlineMethodProcessor extends BaseRefactoringProcessor { return null; } - @NotNull - private PsiExpression inlineParameterReference(@NotNull PsiReferenceExpression expression, BlockData blockData) { - if (expression.getQualifierExpression() != null) return expression; - PsiElement resolve = expression.resolve(); - if (!(resolve instanceof PsiParameter)) return expression; - int paramIdx = ArrayUtil.find(myMethod.getParameterList().getParameters(), resolve); - if (paramIdx < 0) return expression; - PsiExpression initializer = blockData.parmVars[paramIdx].getInitializer(); - if (initializer == null) return expression; - return InlineUtil.inlineInitializer((PsiVariable)resolve, initializer, expression); - } - private void substituteMethodTypeParams(PsiElement scope, final PsiSubstitutor substitutor) { InlineUtil.substituteTypeParams(scope, substitutor, myFactory); } diff --git a/java/java-impl-refactorings/src/com/intellij/refactoring/util/InlineUtil.java b/java/java-impl-refactorings/src/com/intellij/refactoring/util/InlineUtil.java index 19984bc7a7c3..250ca880d49c 100644 --- a/java/java-impl-refactorings/src/com/intellij/refactoring/util/InlineUtil.java +++ b/java/java-impl-refactorings/src/com/intellij/refactoring/util/InlineUtil.java @@ -5,6 +5,7 @@ import com.intellij.codeInsight.BlockUtils; import com.intellij.codeInsight.ChangeContextUtil; import com.intellij.codeInsight.daemon.impl.analysis.HighlightControlFlowUtil; import com.intellij.codeInsight.daemon.impl.quickfix.SimplifyBooleanExpressionFix; +import com.intellij.codeInspection.dataFlow.JavaMethodContractUtil; import com.intellij.codeInspection.redundantCast.RemoveRedundantCastUtil; import com.intellij.java.refactoring.JavaRefactoringBundle; import com.intellij.openapi.diagnostic.Logger; @@ -476,12 +477,86 @@ public final class InlineUtil implements CommonJavaInlineUtil { isAccessedForWriting = true; } } + if (refs.size() == 1 && !isAccessedForWriting && isFirstUse(variable, refs.get(0))) return true; PsiExpression initializer = variable.getInitializer(); return canInlineParameterOrThisVariable(variable.getProject(), initializer, false, false, refs.size(), isAccessedForWriting); } + private static boolean isFirstUse(PsiLocalVariable variable, PsiReferenceExpression expression) { + if (!(variable.getParent() instanceof PsiDeclarationStatement decl)) return false; + PsiStatement statement = PsiTreeUtil.getNextSiblingOfType(decl, PsiStatement.class); + if (statement == null) return false; + PsiElement parent; + PsiExpression cur = expression; + while (true) { + parent = cur.getParent(); + if (parent instanceof PsiPolyadicExpression poly) { + if (poly.getOperands()[0] != cur) return false; + cur = poly; + } else if (parent instanceof PsiParenthesizedExpression || + parent instanceof PsiTypeCastExpression || + parent instanceof PsiUnaryExpression || + parent instanceof PsiInstanceOfExpression || + parent instanceof PsiSwitchExpression) { + cur = (PsiExpression)parent; + } else if (parent instanceof PsiConditionalExpression cond) { + if (cond.getCondition() != cur) return false; + cur = cond; + } else if (parent instanceof PsiExpressionList list) { + for (PsiExpression expr : list.getExpressions()) { + if (expr == cur) break; + if (!ExpressionUtils.isSafelyRecomputableExpression(expr)) return false; + } + if (!(list.getParent() instanceof PsiCallExpression call)) return false; + PsiExpression qualifier = call instanceof PsiMethodCallExpression methodCall ? methodCall.getMethodExpression().getQualifierExpression() : + call instanceof PsiNewExpression newExpression ? newExpression.getQualifier() : null; + if (qualifier != null && !ExpressionUtils.isSafelyRecomputableExpression(qualifier)) return false; + cur = call; + } + else if (parent instanceof PsiReferenceExpression ref) { + if (parent.getParent() instanceof PsiMethodCallExpression call) { + cur = call; + } else { + cur = ref; + } + } else if (parent instanceof PsiArrayAccessExpression arr) { + if (arr.getIndexExpression() == cur && !ExpressionUtils.isSafelyRecomputableExpression(arr)) return false; + cur = arr; + } else if (parent instanceof PsiArrayInitializerExpression init) { + for (PsiExpression initializer : init.getInitializers()) { + if (initializer == cur) break; + if (!ExpressionUtils.isSafelyRecomputableExpression(initializer)) return false; + } + cur = init; + } + else if (parent instanceof PsiNewExpression newExpression) { + cur = newExpression; + } + else if (parent instanceof PsiLocalVariable var) { + return var.getParent() == statement; + } + else if (parent instanceof PsiStatement) { + if (parent != statement) return false; + if (parent instanceof PsiAssertStatement || + parent instanceof PsiReturnStatement || + parent instanceof PsiIfStatement || + parent instanceof PsiExpressionStatement || + parent instanceof PsiSynchronizedStatement || + parent instanceof PsiSwitchStatement || + parent instanceof PsiWhileStatement || + parent instanceof PsiForeachStatement) { + return true; + } + return false; + } + else { + return false; + } + } + } + private static boolean canInlineParameterOrThisVariable(Project project, PsiExpression initializer, boolean shouldBeFinal, @@ -572,7 +647,8 @@ public final class InlineUtil implements CommonJavaInlineUtil { return false; } } - return true; //TODO: "suspicious" places to review by user! + PsiMethod method = ((PsiCallExpression)initializer).resolveMethod(); + return method == null || JavaMethodContractUtil.isPure(method); } else if (initializer instanceof PsiLiteralExpression) { return true; @@ -648,7 +724,9 @@ public final class InlineUtil implements CommonJavaInlineUtil { boolean shouldBeFinal = variable.hasModifierProperty(PsiModifier.FINAL) && strictlyFinal; Project project = variable.getProject(); - if (canInlineParameterOrThisVariable(project, initializer, shouldBeFinal, strictlyFinal, refs.size(), isAccessedForWriting)) { + boolean canInline = refs.size() == 1 && !isAccessedForWriting && isFirstUse(variable, refs.get(0)) || + canInlineParameterOrThisVariable(project, initializer, shouldBeFinal, strictlyFinal, refs.size(), isAccessedForWriting); + if (canInline) { if (shouldBeFinal) { declareUsedLocalsFinal(initializer, true); } diff --git a/java/java-tests/testData/refactoring/inlineMethod/NewWithSideEffect.java b/java/java-tests/testData/refactoring/inlineMethod/NewWithSideEffect.java new file mode 100644 index 000000000000..a30a48d5be54 --- /dev/null +++ b/java/java-tests/testData/refactoring/inlineMethod/NewWithSideEffect.java @@ -0,0 +1,21 @@ +public class Foo { + public static void main(String[] args) { + foo(new X()); + } + + static void foo(X x) { + System.out.println(1); + System.out.println(x); + } + + static class X { + X() { + System.out.println(0); + } + + @Override + public String toString() { + return "2"; + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/inlineMethod/NewWithSideEffect.java.after b/java/java-tests/testData/refactoring/inlineMethod/NewWithSideEffect.java.after new file mode 100644 index 000000000000..a1076366694b --- /dev/null +++ b/java/java-tests/testData/refactoring/inlineMethod/NewWithSideEffect.java.after @@ -0,0 +1,18 @@ +public class Foo { + public static void main(String[] args) { + X x = new X(); + System.out.println(1); + System.out.println(x); + } + + static class X { + X() { + System.out.println(0); + } + + @Override + public String toString() { + return "2"; + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/inlineMethod/NotAStatement.java.after b/java/java-tests/testData/refactoring/inlineMethod/NotAStatement.java.after index 2c1e0c4fbfb8..6451116f5d81 100644 --- a/java/java-tests/testData/refactoring/inlineMethod/NotAStatement.java.after +++ b/java/java-tests/testData/refactoring/inlineMethod/NotAStatement.java.after @@ -1,7 +1,6 @@ class AAA { void fff(Project myProject) { - final String[] strings = new String[1]; - ensureFilesWritable(strings).hasReadonlyFiles(); + ensureFilesWritable(new String[1]).hasReadonlyFiles(); } private Status ensureFilesWritable(final String[] strings) { 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 22cadfd4e6fc..649e78f82eeb 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 @@ -563,6 +563,8 @@ public class InlineMethodTest extends LightRefactoringTestCase { public void testTernaryBranch() { doTest(); } public void testTernaryBranchCollapsible() { doTest(); } + public void testNewWithSideEffect() { doTest(); } + @Override protected Sdk getProjectJDK() { return getTestName(false).contains("Src") ? IdeaTestUtil.getMockJdk17() : super.getProjectJDK();