From 0aac1b7ed7ab390b500be625a37cac4de9854c11 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Fri, 19 Apr 2019 15:02:07 +0700 Subject: [PATCH] InlineMethod: better transformation when return value is unused --- .../inline/InlineMethodProcessor.java | 2 +- .../refactoring/inline/InlineTransformer.java | 27 +++++++++++-------- ...oSingleReturnWithFinishedUnusedResult.java | 16 +++++++++++ ...eReturnWithFinishedUnusedResult.java.after | 13 +++++++++ .../inlineMethod/NotAStatement4.java.after | 2 -- .../inlineMethod/UnusedResult.java | 13 +++++++++ .../inlineMethod/UnusedResult.java.after | 10 +++++++ .../refactoring/inline/InlineMethodTest.java | 8 ++++++ 8 files changed, 77 insertions(+), 14 deletions(-) create mode 100644 java/java-tests/testData/refactoring/inlineMethod/ConvertToSingleReturnWithFinishedUnusedResult.java create mode 100644 java/java-tests/testData/refactoring/inlineMethod/ConvertToSingleReturnWithFinishedUnusedResult.java.after create mode 100644 java/java-tests/testData/refactoring/inlineMethod/UnusedResult.java create mode 100644 java/java-tests/testData/refactoring/inlineMethod/UnusedResult.java.after diff --git a/java/java-impl/src/com/intellij/refactoring/inline/InlineMethodProcessor.java b/java/java-impl/src/com/intellij/refactoring/inline/InlineMethodProcessor.java index 8c925226473d..79c1863ff524 100644 --- a/java/java-impl/src/com/intellij/refactoring/inline/InlineMethodProcessor.java +++ b/java/java-impl/src/com/intellij/refactoring/inline/InlineMethodProcessor.java @@ -817,7 +817,7 @@ public class InlineMethodProcessor extends BaseRefactoringProcessor { addSynchronization(ref, block, originalStatements, thisVar); - PsiLocalVariable resultVar = transformer.transformBody(myMethodCopy, returnType); + PsiLocalVariable resultVar = transformer.transformBody(myMethodCopy, ref, returnType); return new BlockData(block, thisVar, parmVars, resultVar); } diff --git a/java/java-impl/src/com/intellij/refactoring/inline/InlineTransformer.java b/java/java-impl/src/com/intellij/refactoring/inline/InlineTransformer.java index e052d4e29637..6b0061bc9817 100644 --- a/java/java-impl/src/com/intellij/refactoring/inline/InlineTransformer.java +++ b/java/java-impl/src/com/intellij/refactoring/inline/InlineTransformer.java @@ -13,10 +13,7 @@ import com.intellij.psi.util.PsiTreeUtil; import com.intellij.psi.util.PsiUtil; import com.intellij.refactoring.util.InlineUtil; import com.intellij.util.containers.ContainerUtil; -import com.siyeh.ig.psiutils.CommentTracker; -import com.siyeh.ig.psiutils.SideEffectChecker; -import com.siyeh.ig.psiutils.StatementExtractor; -import com.siyeh.ig.psiutils.VariableNameGenerator; +import com.siyeh.ig.psiutils.*; import org.jetbrains.annotations.NotNull; import java.util.Arrays; @@ -40,10 +37,11 @@ public abstract class InlineTransformer { * Transforms method body in the way so it can be inserted into the call site. May declare result variable if necessary. * * @param methodCopy non-physical copy of the method to be inlined (may be changed by this call) + * @param callSite method call * @param returnType substituted method return type * @return result variable or null if unnecessary */ - public abstract PsiLocalVariable transformBody(PsiMethod methodCopy, PsiType returnType); + public abstract PsiLocalVariable transformBody(PsiMethod methodCopy, PsiReferenceExpression callSite, PsiType returnType); static class NormalTransformer extends InlineTransformer { @Override @@ -57,7 +55,7 @@ public abstract class InlineTransformer { } @Override - public PsiLocalVariable transformBody(PsiMethod methodCopy, PsiType returnType) { + public PsiLocalVariable transformBody(PsiMethod methodCopy, PsiReferenceExpression callSite, PsiType returnType) { if (returnType == null || PsiType.VOID.equals(returnType)) return null; PsiCodeBlock block = Objects.requireNonNull(methodCopy.getBody()); Project project = methodCopy.getProject(); @@ -92,7 +90,7 @@ public abstract class InlineTransformer { } @Override - public PsiLocalVariable transformBody(PsiMethod methodCopy, PsiType returnType) { + public PsiLocalVariable transformBody(PsiMethod methodCopy, PsiReferenceExpression callSite, PsiType returnType) { return null; } } @@ -116,7 +114,7 @@ public abstract class InlineTransformer { } @Override - public PsiLocalVariable transformBody(PsiMethod methodCopy, PsiType returnType) { + public PsiLocalVariable transformBody(PsiMethod methodCopy, PsiReferenceExpression callSite, PsiType returnType) { extractReturnValues(methodCopy, true); return null; } @@ -135,13 +133,14 @@ public abstract class InlineTransformer { } @Override - public PsiLocalVariable transformBody(PsiMethod methodCopy, PsiType returnType) { + public PsiLocalVariable transformBody(PsiMethod methodCopy, PsiReferenceExpression callSite, PsiType returnType) { extractReturnValues(methodCopy, false); return null; } } private static void extractReturnValues(PsiMethod methodCopy, boolean replaceWithContinue) { + PsiCodeBlock block = Objects.requireNonNull(methodCopy.getBody()); PsiReturnStatement[] returnStatements = PsiUtil.findReturnStatements(methodCopy); for (PsiReturnStatement returnStatement : returnStatements) { final PsiExpression returnValue = returnStatement.getReturnValue(); @@ -158,7 +157,9 @@ public abstract class InlineTransformer { } ct.insertCommentsBefore(returnStatement); } - if (replaceWithContinue) { + if (ControlFlowUtils.blockCompletesWithStatement(block, returnStatement)) { + new CommentTracker().deleteAndRestoreComments(returnStatement); + } else if (replaceWithContinue) { new CommentTracker().replaceAndRestoreComments(returnStatement, "continue;"); } } @@ -176,7 +177,11 @@ public abstract class InlineTransformer { } @Override - public PsiLocalVariable transformBody(PsiMethod methodCopy, PsiType returnType) { + public PsiLocalVariable transformBody(PsiMethod methodCopy, PsiReferenceExpression callSite, PsiType returnType) { + if (callSite.getParent() instanceof PsiMethodCallExpression && ExpressionUtils.isVoidContext((PsiExpression)callSite.getParent())) { + InlineTransformer.extractReturnValues(methodCopy, false); + returnType = PsiType.VOID; + } PsiCodeBlock block = Objects.requireNonNull(methodCopy.getBody()); List returns = Arrays.asList(PsiUtil.findReturnStatements(block)); FinishMarker marker = FinishMarker.defineFinishMarker(block, returnType, returns); diff --git a/java/java-tests/testData/refactoring/inlineMethod/ConvertToSingleReturnWithFinishedUnusedResult.java b/java/java-tests/testData/refactoring/inlineMethod/ConvertToSingleReturnWithFinishedUnusedResult.java new file mode 100644 index 000000000000..525aad10d19a --- /dev/null +++ b/java/java-tests/testData/refactoring/inlineMethod/ConvertToSingleReturnWithFinishedUnusedResult.java @@ -0,0 +1,16 @@ +class A { + String foo(int i) { + if (i > 0) { + if (i == 10) return null; + System.out.println(i); + } + return String.valueOf(i); + } + + void bar(int x) { + if (x > 0) { + foo(x); + } + System.out.println("x < 0"); + } +} diff --git a/java/java-tests/testData/refactoring/inlineMethod/ConvertToSingleReturnWithFinishedUnusedResult.java.after b/java/java-tests/testData/refactoring/inlineMethod/ConvertToSingleReturnWithFinishedUnusedResult.java.after new file mode 100644 index 000000000000..f6cb11c2b865 --- /dev/null +++ b/java/java-tests/testData/refactoring/inlineMethod/ConvertToSingleReturnWithFinishedUnusedResult.java.after @@ -0,0 +1,13 @@ +class A { + + void bar(int x) { + if (x > 0) { + if (x > 0) { + if (x != 10) { + System.out.println(x); + } + } + } + System.out.println("x < 0"); + } +} diff --git a/java/java-tests/testData/refactoring/inlineMethod/NotAStatement4.java.after b/java/java-tests/testData/refactoring/inlineMethod/NotAStatement4.java.after index 1fdc4a79e8df..57551840ae10 100644 --- a/java/java-tests/testData/refactoring/inlineMethod/NotAStatement4.java.after +++ b/java/java-tests/testData/refactoring/inlineMethod/NotAStatement4.java.after @@ -8,11 +8,9 @@ class AAA { if (s1.isEmpty()) { s1.add("foo"); s2.add("foo"); - return; } else { s1.add("foo"); s2.add("foo"); - return; } } } \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/inlineMethod/UnusedResult.java b/java/java-tests/testData/refactoring/inlineMethod/UnusedResult.java new file mode 100644 index 000000000000..308b073eedda --- /dev/null +++ b/java/java-tests/testData/refactoring/inlineMethod/UnusedResult.java @@ -0,0 +1,13 @@ +import java.util.*; + +class Test { + boolean test(Set set) { + return set.add("foo"); + } + + void use() { + Set set = new HashSet<>(); + test(set); + test(set); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/inlineMethod/UnusedResult.java.after b/java/java-tests/testData/refactoring/inlineMethod/UnusedResult.java.after new file mode 100644 index 000000000000..4cd5de199b10 --- /dev/null +++ b/java/java-tests/testData/refactoring/inlineMethod/UnusedResult.java.after @@ -0,0 +1,10 @@ +import java.util.*; + +class Test { + + void use() { + Set set = new HashSet<>(); + set.add("foo"); + set.add("foo"); + } +} \ 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 3ae364041177..12bf52bafd5f 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 @@ -467,6 +467,14 @@ public class InlineMethodTest extends LightRefactoringTestCase { doTestAssertBadReturn(); } + public void testConvertToSingleReturnWithFinishedUnusedResult() { + doTestAssertBadReturn(); + } + + public void testUnusedResult() { + doTest(); + } + @Override protected Sdk getProjectJDK() { return getTestName(false).contains("Src") ? IdeaTestUtil.getMockJdk17() : super.getProjectJDK();