From 3180debb4b2689ddeb77f40e1ea7b16254bec192 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Fri, 19 Apr 2019 17:37:54 +0700 Subject: [PATCH] IDEABKL-4394 Automatically remove redundant `result` variable when inlining the method --- .../streamToLoop/StreamToLoopInspection.java | 14 +- .../inline/InlineMethodProcessor.java | 171 +++++++++--------- .../refactoring/inline/InlineTransformer.java | 4 + .../inlineMethod/InlineWithTry.java.after | 2 - ...InitializerIsNotSideEffectsFree.java.after | 5 +- .../inlineMethod/RawSubstitution.java.after | 7 +- .../inlineMethod/ReuseResultVar.java | 16 ++ .../inlineMethod/ReuseResultVar.java.after | 12 ++ .../inlineMethod/SingleReturn1.java | 2 +- .../inlineMethod/SingleReturn1.java.after | 5 +- .../inlineMethod/SingleReturn1NotFinal.java | 14 ++ .../SingleReturn1NotFinal.java.after | 11 ++ .../refactoring/inline/InlineMethodTest.java | 8 + .../ig/psiutils/VariableAccessUtils.java | 14 ++ 14 files changed, 175 insertions(+), 110 deletions(-) create mode 100644 java/java-tests/testData/refactoring/inlineMethod/ReuseResultVar.java create mode 100644 java/java-tests/testData/refactoring/inlineMethod/ReuseResultVar.java.after create mode 100644 java/java-tests/testData/refactoring/inlineMethod/SingleReturn1NotFinal.java create mode 100644 java/java-tests/testData/refactoring/inlineMethod/SingleReturn1NotFinal.java.after diff --git a/java/java-impl/src/com/intellij/codeInspection/streamToLoop/StreamToLoopInspection.java b/java/java-impl/src/com/intellij/codeInspection/streamToLoop/StreamToLoopInspection.java index 817662b1f36b..b51a09382425 100644 --- a/java/java-impl/src/com/intellij/codeInspection/streamToLoop/StreamToLoopInspection.java +++ b/java/java-impl/src/com/intellij/codeInspection/streamToLoop/StreamToLoopInspection.java @@ -14,7 +14,6 @@ import com.intellij.profile.codeInspection.InspectionProjectProfileManager; import com.intellij.psi.*; import com.intellij.psi.codeStyle.JavaCodeStyleManager; import com.intellij.psi.impl.source.PsiImmediateClassType; -import com.intellij.psi.search.searches.ReferencesSearch; import com.intellij.psi.tree.IElementType; import com.intellij.psi.util.InheritanceUtil; import com.intellij.psi.util.PsiTreeUtil; @@ -495,7 +494,8 @@ public class StreamToLoopInspection extends AbstractBaseJavaLocalInspectionTool if (kind != ResultKind.UNKNOWN && myStreamExpression.getParent() instanceof PsiVariable) { PsiVariable var = (PsiVariable)myStreamExpression.getParent(); if (isCompatibleType(var, type, mostAbstractAllowedType) && - var.getParent() instanceof PsiDeclarationStatement && (kind == ResultKind.FINAL || canUseAsNonFinal(var))) { + var.getParent() instanceof PsiDeclarationStatement && + (kind == ResultKind.FINAL || VariableAccessUtils.canUseAsNonFinal(ObjectUtils.tryCast(var, PsiLocalVariable.class)))) { PsiDeclarationStatement declaration = (PsiDeclarationStatement)var.getParent(); if(declaration.getDeclaredElements().length == 1) { myStreamExpression = declaration; @@ -547,16 +547,6 @@ public class StreamToLoopInspection extends AbstractBaseJavaLocalInspectionTool isCompatibleType(var, superType, mostAbstractAllowedType)); } - @Contract("null -> false") - private static boolean canUseAsNonFinal(PsiVariable var) { - if (!(var instanceof PsiLocalVariable)) return false; - PsiElement block = PsiUtil.getVariableCodeBlock(var, null); - return block != null && ReferencesSearch.search(var).allMatch(ref -> { - PsiElement context = PsiTreeUtil.getParentOfType(ref.getElement(), PsiClass.class, PsiLambdaExpression.class); - return context == null || PsiTreeUtil.isAncestor(context, block, false); - }); - } - public PsiElement makeFinalReplacement() { LOG.assertTrue(myStreamExpression != null); if (myFinisher == null || myStreamExpression instanceof PsiStatement) { 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 fef678b29af2..084d5ab499ce 100644 --- a/java/java-impl/src/com/intellij/refactoring/inline/InlineMethodProcessor.java +++ b/java/java-impl/src/com/intellij/refactoring/inline/InlineMethodProcessor.java @@ -4,6 +4,7 @@ package com.intellij.refactoring.inline; import com.intellij.codeInsight.AnnotationUtil; import com.intellij.codeInsight.ChangeContextUtil; import com.intellij.codeInsight.ExpressionUtil; +import com.intellij.codeInsight.daemon.impl.analysis.HighlightControlFlowUtil; import com.intellij.codeInsight.daemon.impl.quickfix.RemoveUnusedVariableUtil; import com.intellij.history.LocalHistory; import com.intellij.history.LocalHistoryAction; @@ -47,7 +48,9 @@ import com.intellij.util.JavaPsiConstructorUtil; import com.intellij.util.ObjectUtils; import com.intellij.util.containers.MultiMap; import com.siyeh.ig.psiutils.CommentTracker; +import com.siyeh.ig.psiutils.ExpressionUtils; import com.siyeh.ig.psiutils.SideEffectChecker; +import com.siyeh.ig.psiutils.VariableAccessUtils; import org.jetbrains.annotations.NonNls; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -57,6 +60,8 @@ import java.util.function.Function; import java.util.function.Predicate; import java.util.stream.Stream; +import static com.intellij.util.ObjectUtils.tryCast; + public class InlineMethodProcessor extends BaseRefactoringProcessor { private static final Logger LOG = Logger.getInstance("#com.intellij.refactoring.inline.InlineMethodProcessor"); @@ -678,7 +683,7 @@ public class InlineMethodProcessor extends BaseRefactoringProcessor { for (PsiElement e = firstAdded; e != anchor; e = e.getNextSibling()) { if (e instanceof PsiDeclarationStatement) { PsiElement[] elements = ((PsiDeclarationStatement)e).getDeclaredElements(); - PsiLocalVariable var = ObjectUtils.tryCast(ArrayUtil.getFirstElement(elements), PsiLocalVariable.class); + PsiLocalVariable var = tryCast(ArrayUtil.getFirstElement(elements), PsiLocalVariable.class); if (var != null) { String name = var.getName(); LOG.assertTrue(name != null); @@ -729,6 +734,7 @@ public class InlineMethodProcessor extends BaseRefactoringProcessor { ChangeContextUtil.decodeContextInfo(anchorParent, thisClass, thisAccessExpr); PsiElement callParent = methodCall.getParent(); + PsiReferenceExpression resultUsage = null; if (callParent instanceof PsiLambdaExpression) { methodCall.delete(); } @@ -741,8 +747,8 @@ public class InlineMethodProcessor extends BaseRefactoringProcessor { } else { if (blockData.resultVar != null) { - PsiExpression expr = myFactory.createExpressionFromText(blockData.resultVar.getName(), null); - new CommentTracker().replaceAndRestoreComments(methodCall, expr); + PsiExpression expr = myFactory.createExpressionFromText(Objects.requireNonNull(blockData.resultVar.getName()), null); + resultUsage = (PsiReferenceExpression)new CommentTracker().replaceAndRestoreComments(methodCall, expr); } else { //?? @@ -758,8 +764,8 @@ public class InlineMethodProcessor extends BaseRefactoringProcessor { final boolean strictlyFinal = parameter.hasModifierProperty(PsiModifier.FINAL) && isStrictlyFinal(parameter); inlineParmOrThisVariable(parmVars[i], strictlyFinal); } - if (resultVar != null) { - inlineResultVariable(resultVar); + if (resultVar != null && resultUsage != null) { + inlineResultVariable(resultVar, resultUsage); } ChangeContextUtil.clearContextInfo(anchorParent); @@ -1211,105 +1217,98 @@ public class InlineMethodProcessor extends BaseRefactoringProcessor { } } - /* - private boolean isFieldNonModifiable(PsiField field) { - if (field.hasModifierProperty(PsiModifier.FINAL)){ - return true; - } - PsiElement[] refs = myManager.getSearchHelper().findReferences(field, null, false); - for(int i = 0; i < refs.length; i++){ - PsiReferenceExpression ref = (PsiReferenceExpression)refs[i]; - if (PsiUtil.isAccessedForWriting(ref)) { - PsiElement container = ref.getParent(); - while(true){ - if (container instanceof PsiMethod || - container instanceof PsiField || - container instanceof PsiClassInitializer || - container instanceof PsiFile) break; - container = container.getParent(); + private void inlineResultVariable(@NotNull PsiLocalVariable resultVar, @NotNull PsiReferenceExpression resultUsage) throws IncorrectOperationException { + PsiElement context = PsiUtil.getVariableCodeBlock(resultVar, null); + if (context == null) return; + List references = VariableAccessUtils.getVariableReferences(resultVar, context); + if (resultVar.getInitializer() == null) { + PsiAssignmentExpression assignment = null; + for (PsiReferenceExpression ref : references) { + if (ref.getParent() instanceof PsiAssignmentExpression && ((PsiAssignmentExpression)ref.getParent()).getLExpression().equals(ref)) { + if (assignment != null) { + assignment = null; + break; + } + else { + assignment = (PsiAssignmentExpression)ref.getParent(); + } } - if (container instanceof PsiMethod && ((PsiMethod)container).isConstructor()) continue; - return false; + } + + if (assignment != null) { + inlineSingleAssignment(resultVar, assignment, resultUsage); + return; } } - return true; + tryReplaceWithTarget(resultVar, resultUsage, context, references); } - */ - private void inlineResultVariable(PsiVariable resultVar) throws IncorrectOperationException { - PsiAssignmentExpression assignment = null; - PsiReferenceExpression resultUsage = null; - for (PsiReference ref1 : ReferencesSearch.search(resultVar, myRefactoringScope, false)) { - PsiReferenceExpression ref = (PsiReferenceExpression)ref1; - if (ref.getParent() instanceof PsiAssignmentExpression && ((PsiAssignmentExpression)ref.getParent()).getLExpression().equals(ref)) { - if (assignment != null) { - assignment = null; - break; - } - else { - assignment = (PsiAssignmentExpression)ref.getParent(); - } - } - else { - LOG.assertTrue(resultUsage == null, "old:" + resultUsage + "; new:" + ref); - resultUsage = ref; + /** + * If result of the method is an initializer of another var, try to reuse that var to store the result. + */ + private static void tryReplaceWithTarget(@NotNull PsiLocalVariable variable, + @NotNull PsiReferenceExpression usage, + PsiElement context, + List references) { + PsiLocalVariable target = tryCast(PsiUtil.skipParenthesizedExprUp(usage.getParent()), PsiLocalVariable.class); + if (target == null) return; + String name = target.getName(); + if (name == null || !target.getType().equals(variable.getType())) return; + PsiDeclarationStatement declaration = tryCast(target.getParent(), PsiDeclarationStatement.class); + if (declaration == null || declaration.getDeclaredElements().length != 1) return; + PsiModifierList modifiers = target.getModifierList(); + if (modifiers != null && modifiers.getAnnotations().length != 0) return; + boolean effectivelyFinal = HighlightControlFlowUtil.isEffectivelyFinal(variable, context, null); + if (!effectivelyFinal && !VariableAccessUtils.canUseAsNonFinal(target)) return; + + for (PsiReferenceExpression reference : references) { + ExpressionUtils.bindReferenceTo(reference, name); + } + if (effectivelyFinal && target.hasModifierProperty(PsiModifier.FINAL)) { + PsiModifierList modifierList = variable.getModifierList(); + if (modifierList != null) { + modifierList.setModifierProperty(PsiModifier.FINAL, true); } } + variable.setName(name); + new CommentTracker().deleteAndRestoreComments(declaration); + } - if (assignment == null) return; - boolean condition = assignment.getParent() instanceof PsiExpressionStatement; - LOG.assertTrue(condition); + private void inlineSingleAssignment(@NotNull PsiVariable resultVar, + @NotNull PsiAssignmentExpression assignment, + @NotNull PsiReferenceExpression resultUsage) { + LOG.assertTrue(assignment.getParent() instanceof PsiExpressionStatement); // SCR3175 fixed: inline only if declaration and assignment is in the same code block. if (!(assignment.getParent().getParent() == resultVar.getParent().getParent())) return; - if (resultUsage != null) { - String name = resultVar.getName(); - PsiDeclarationStatement declaration = - myFactory.createVariableDeclarationStatement(name, resultVar.getType(), assignment.getRExpression()); - declaration = (PsiDeclarationStatement)assignment.getParent().replace(declaration); - resultVar.getParent().delete(); - resultVar = (PsiVariable)declaration.getDeclaredElements()[0]; + String name = Objects.requireNonNull(resultVar.getName()); + PsiDeclarationStatement declaration = + myFactory.createVariableDeclarationStatement(name, resultVar.getType(), assignment.getRExpression()); + declaration = (PsiDeclarationStatement)assignment.getParent().replace(declaration); + resultVar.getParent().delete(); + resultVar = (PsiVariable)declaration.getDeclaredElements()[0]; - PsiElement parentStatement = RefactoringUtil.getParentStatement(resultUsage, true); - PsiElement next = declaration.getNextSibling(); - boolean canInline = false; - while (true) { - if (next == null) break; - if (parentStatement.equals(next)) { - canInline = true; - break; - } - if (next instanceof PsiStatement) break; - next = next.getNextSibling(); - } - - if (canInline) { - InlineUtil.inlineVariable(resultVar, resultVar.getInitializer(), resultUsage); - declaration.delete(); + PsiElement parentStatement = RefactoringUtil.getParentStatement(resultUsage, true); + PsiElement next = declaration.getNextSibling(); + boolean canInline = false; + while (true) { + if (next == null) break; + if (next.equals(parentStatement)) { + canInline = true; + break; } + if (next instanceof PsiStatement) break; + next = next.getNextSibling(); } - else { - PsiExpression rExpression = assignment.getRExpression(); - while (rExpression instanceof PsiReferenceExpression) rExpression = ((PsiReferenceExpression)rExpression).getQualifierExpression(); - if (rExpression == null) { - assignment.delete(); - } - else if (!PsiUtil.isStatement(rExpression)) { - if (RemoveUnusedVariableUtil.checkSideEffects(rExpression, resultVar, new ArrayList<>())) { - //keep result variable - return; - } - assignment.delete(); - } - else { - assignment.replace(rExpression); - } - resultVar.delete(); + + if (canInline) { + InlineUtil.inlineVariable(resultVar, resultVar.getInitializer(), resultUsage); + declaration.delete(); } } private static final Key MARK_KEY = Key.create(""); - public PsiReferenceExpression[] addBracesWhenNeeded(PsiReferenceExpression[] refs) throws IncorrectOperationException { + private PsiReferenceExpression[] addBracesWhenNeeded(PsiReferenceExpression[] refs) throws IncorrectOperationException { ArrayList refsVector = new ArrayList<>(); ArrayList addedBracesVector = new ArrayList<>(); myAddedClassInitializers = new HashMap<>(); 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 8510ba0315b1..28f348e30010 100644 --- a/java/java-impl/src/com/intellij/refactoring/inline/InlineTransformer.java +++ b/java/java-impl/src/com/intellij/refactoring/inline/InlineTransformer.java @@ -65,6 +65,10 @@ public abstract class InlineTransformer { @Override public PsiLocalVariable transformBody(PsiMethod methodCopy, PsiReferenceExpression callSite, PsiType returnType) { if (returnType == null || PsiType.VOID.equals(returnType)) return null; + if (callSite.getParent() instanceof PsiMethodCallExpression && ExpressionUtils.isVoidContext((PsiExpression)callSite.getParent())) { + InlineTransformer.extractReturnValues(methodCopy, false); + return null; + } PsiCodeBlock block = Objects.requireNonNull(methodCopy.getBody()); Project project = methodCopy.getProject(); PsiElementFactory factory = JavaPsiFacade.getElementFactory(project); diff --git a/java/java-tests/testData/refactoring/inlineMethod/InlineWithTry.java.after b/java/java-tests/testData/refactoring/inlineMethod/InlineWithTry.java.after index dc25d00ea0da..02302a823227 100644 --- a/java/java-tests/testData/refactoring/inlineMethod/InlineWithTry.java.after +++ b/java/java-tests/testData/refactoring/inlineMethod/InlineWithTry.java.after @@ -1,8 +1,6 @@ class A { { - int result; try { - result = 0; } catch (Error e) { throw e; } diff --git a/java/java-tests/testData/refactoring/inlineMethod/PreserveResultedVariableIfInitializerIsNotSideEffectsFree.java.after b/java/java-tests/testData/refactoring/inlineMethod/PreserveResultedVariableIfInitializerIsNotSideEffectsFree.java.after index 07ac21a6373b..99060aeb2c5c 100644 --- a/java/java-tests/testData/refactoring/inlineMethod/PreserveResultedVariableIfInitializerIsNotSideEffectsFree.java.after +++ b/java/java-tests/testData/refactoring/inlineMethod/PreserveResultedVariableIfInitializerIsNotSideEffectsFree.java.after @@ -19,8 +19,9 @@ class Main { public final void doSomething(Object obj) { try { - Object result; - result = null == null ? fooBar() : null; + if (null == null) { + fooBar(); + } } catch (Exception e) { e.printStackTrace(); } diff --git a/java/java-tests/testData/refactoring/inlineMethod/RawSubstitution.java.after b/java/java-tests/testData/refactoring/inlineMethod/RawSubstitution.java.after index 3bdbc002fb30..e2560ca82afa 100644 --- a/java/java-tests/testData/refactoring/inlineMethod/RawSubstitution.java.after +++ b/java/java-tests/testData/refactoring/inlineMethod/RawSubstitution.java.after @@ -4,12 +4,11 @@ public class NotRaw { class Raw extends NotRaw { void foo() { - Object result; + Object o; Object tt = null; if ( null == null) { - result = null; + o = null; } else - result = null; - Object o = result; + o = null; } } \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/inlineMethod/ReuseResultVar.java b/java/java-tests/testData/refactoring/inlineMethod/ReuseResultVar.java new file mode 100644 index 000000000000..592e254448ad --- /dev/null +++ b/java/java-tests/testData/refactoring/inlineMethod/ReuseResultVar.java @@ -0,0 +1,16 @@ +import java.util.*; + +class Test { + void useTest() { + String color = makeColor(Math.random() > 0.5); + System.out.println("Color is " + color); + } + + private String makeColor(boolean b) { + if (b) { + return "Foo"; + } else { + return "Fie"; + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/inlineMethod/ReuseResultVar.java.after b/java/java-tests/testData/refactoring/inlineMethod/ReuseResultVar.java.after new file mode 100644 index 000000000000..e1189f13c58b --- /dev/null +++ b/java/java-tests/testData/refactoring/inlineMethod/ReuseResultVar.java.after @@ -0,0 +1,12 @@ +class Test { + void useTest() { + String color; + if (Math.random() > 0.5) { + color = "Foo"; + } else { + color = "Fie"; + } + System.out.println("Color is " + color); + } + +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/inlineMethod/SingleReturn1.java b/java/java-tests/testData/refactoring/inlineMethod/SingleReturn1.java index ccf1827b752f..70c613a4c351 100644 --- a/java/java-tests/testData/refactoring/inlineMethod/SingleReturn1.java +++ b/java/java-tests/testData/refactoring/inlineMethod/SingleReturn1.java @@ -8,7 +8,7 @@ class Tester { } void caller(String v) { - String g = callee(v); + final String g = callee(v); System.out.println(g); } } diff --git a/java/java-tests/testData/refactoring/inlineMethod/SingleReturn1.java.after b/java/java-tests/testData/refactoring/inlineMethod/SingleReturn1.java.after index 5e1308b166f4..6f13c41ec34c 100644 --- a/java/java-tests/testData/refactoring/inlineMethod/SingleReturn1.java.after +++ b/java/java-tests/testData/refactoring/inlineMethod/SingleReturn1.java.after @@ -1,11 +1,10 @@ class Tester { void caller(String v) { - String result = null; + String g = null; if (v != null) { - result = v; + g = v; } - String g = result; System.out.println(g); } } diff --git a/java/java-tests/testData/refactoring/inlineMethod/SingleReturn1NotFinal.java b/java/java-tests/testData/refactoring/inlineMethod/SingleReturn1NotFinal.java new file mode 100644 index 000000000000..53f6bd27e62d --- /dev/null +++ b/java/java-tests/testData/refactoring/inlineMethod/SingleReturn1NotFinal.java @@ -0,0 +1,14 @@ +class Tester { + // IDEA-37432 + String callee(String x) { + if (x == null) { + return null; + } + return x; + } + + void caller(String v) { + String g = callee(v); + Runnable r = () -> System.out.println(g); + } +} diff --git a/java/java-tests/testData/refactoring/inlineMethod/SingleReturn1NotFinal.java.after b/java/java-tests/testData/refactoring/inlineMethod/SingleReturn1NotFinal.java.after new file mode 100644 index 000000000000..c0ee679dd143 --- /dev/null +++ b/java/java-tests/testData/refactoring/inlineMethod/SingleReturn1NotFinal.java.after @@ -0,0 +1,11 @@ +class Tester { + + void caller(String v) { + String result = null; + if (v != null) { + result = v; + } + String g = result; + Runnable r = () -> System.out.println(g); + } +} 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 7e7f2582395f..107d33b4fc87 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 @@ -257,6 +257,10 @@ public class InlineMethodTest extends LightRefactoringTestCase { doTestAssertBadReturn(); } + public void testSingleReturn1NotFinal() { + doTestAssertBadReturn(); + } + public void testSingleReturn2() { doTestAssertBadReturn(); } @@ -474,6 +478,10 @@ public class InlineMethodTest extends LightRefactoringTestCase { public void testUnusedResult() { doTest(); } + + public void testReuseResultVar() { + doTest(); + } @Override protected Sdk getProjectJDK() { diff --git a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/VariableAccessUtils.java b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/VariableAccessUtils.java index c0eb176ca7f9..78132b948a4b 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/VariableAccessUtils.java +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/VariableAccessUtils.java @@ -515,6 +515,20 @@ public class VariableAccessUtils { return false; } + /** + * @param var variable to check + * @return true if given variable doesn't need to be effectively final (i.e. not used inside lambdas/classes) + */ + @Contract("null -> false") + public static boolean canUseAsNonFinal(PsiLocalVariable var) { + if (var == null) return false; + PsiElement block = PsiUtil.getVariableCodeBlock(var, null); + return block != null && ReferencesSearch.search(var).allMatch(ref -> { + PsiElement context = PsiTreeUtil.getParentOfType(ref.getElement(), PsiClass.class, PsiLambdaExpression.class); + return context == null || PsiTreeUtil.isAncestor(context, block, false); + }); + } + private static class VariableCollectingVisitor extends JavaRecursiveElementWalkingVisitor { private final Set usedVariables = new HashSet<>();