From 15ed2e83841c1f6830812156df23d6917cbffb4c Mon Sep 17 00:00:00 2001 From: "Anna.Kozlova" Date: Mon, 8 Oct 2018 18:54:27 +0200 Subject: [PATCH] inline chained calls: ensure qualifiers with side effects are preserved (IDEA-199990; IDEA-147989) --- .../quickfix/RemoveUnusedVariableUtil.java | 6 +-- .../inline/InlineMethodProcessor.java | 11 +++-- .../inlineMethod/ChainedBuilderCall.java | 42 +++++++++++++++++++ .../ChainedBuilderCall.java.after | 38 +++++++++++++++++ .../MakeTypesDenotable.java.after | 1 + ...rWithSideEffectsOnInliningEmptyMethod.java | 26 ++++++++++++ ...ideEffectsOnInliningEmptyMethod.java.after | 24 +++++++++++ .../refactoring/inline/InlineMethodTest.java | 8 ++++ 8 files changed, 150 insertions(+), 6 deletions(-) create mode 100644 java/java-tests/testData/refactoring/inlineMethod/ChainedBuilderCall.java create mode 100644 java/java-tests/testData/refactoring/inlineMethod/ChainedBuilderCall.java.after create mode 100644 java/java-tests/testData/refactoring/inlineMethod/MissedQualifierWithSideEffectsOnInliningEmptyMethod.java create mode 100644 java/java-tests/testData/refactoring/inlineMethod/MissedQualifierWithSideEffectsOnInliningEmptyMethod.java.after diff --git a/java/java-analysis-impl/src/com/intellij/codeInsight/daemon/impl/quickfix/RemoveUnusedVariableUtil.java b/java/java-analysis-impl/src/com/intellij/codeInsight/daemon/impl/quickfix/RemoveUnusedVariableUtil.java index 92c506af1014..77772881ec07 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInsight/daemon/impl/quickfix/RemoveUnusedVariableUtil.java +++ b/java/java-analysis-impl/src/com/intellij/codeInsight/daemon/impl/quickfix/RemoveUnusedVariableUtil.java @@ -54,9 +54,9 @@ public class RemoveUnusedVariableUtil { return !writes.isEmpty(); } - static PsiElement replaceElementWithExpression(PsiExpression expression, - PsiElementFactory factory, - PsiElement element) throws IncorrectOperationException { + public static PsiElement replaceElementWithExpression(PsiExpression expression, + PsiElementFactory factory, + PsiElement element) throws IncorrectOperationException { PsiElement elementToReplace = element; PsiElement expressionToReplaceWith = expression; if (element.getParent() instanceof PsiExpressionStatement) { 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 915804bef048..7f879965b54c 100644 --- a/java/java-impl/src/com/intellij/refactoring/inline/InlineMethodProcessor.java +++ b/java/java-impl/src/com/intellij/refactoring/inline/InlineMethodProcessor.java @@ -1026,8 +1026,14 @@ public class InlineMethodProcessor extends BaseRefactoringProcessor { private void inlineParmOrThisVariable(PsiLocalVariable variable, boolean strictlyFinal) throws IncorrectOperationException { PsiReference firstRef = ReferencesSearch.search(variable).findFirst(); + PsiExpression initializer = variable.getInitializer(); if (firstRef == null) { - variable.getParent().delete(); //Q: side effects? + if (initializer != null && SideEffectChecker.mayHaveSideEffects(initializer)) { + RemoveUnusedVariableUtil.replaceElementWithExpression(initializer, PsiElementFactory.SERVICE.getInstance(myProject), variable); + } + else { + variable.getParent().delete(); + } return; } @@ -1043,7 +1049,6 @@ public class InlineMethodProcessor extends BaseRefactoringProcessor { } } - PsiExpression initializer = variable.getInitializer(); boolean shouldBeFinal = variable.hasModifierProperty(PsiModifier.FINAL) && strictlyFinal; if (canInlineParmOrThisVariable(initializer, shouldBeFinal, strictlyFinal, refs.size(), isAccessedForWriting)) { if (shouldBeFinal) { @@ -1153,7 +1158,7 @@ public class InlineMethodProcessor extends BaseRefactoringProcessor { return false; } else if (initializer instanceof PsiCallExpression) { - if (accessCount > 1) return false; + if (accessCount != 1) return false;//don't allow deleting probable side effects or multiply those side effects if (initializer instanceof PsiNewExpression) { final PsiArrayInitializerExpression arrayInitializer = ((PsiNewExpression)initializer).getArrayInitializer(); if (arrayInitializer != null) { diff --git a/java/java-tests/testData/refactoring/inlineMethod/ChainedBuilderCall.java b/java/java-tests/testData/refactoring/inlineMethod/ChainedBuilderCall.java new file mode 100644 index 000000000000..efc9debdced2 --- /dev/null +++ b/java/java-tests/testData/refactoring/inlineMethod/ChainedBuilderCall.java @@ -0,0 +1,42 @@ + +class MyObjBuilder { + private NewType memberVar1; + + public NewType convertToNewType(LegacyType arg) { + return new NewType(); + } + + public MyObjBuilder memberVar1(LegacyType arg) { + memberVar1(convertToNewType(arg)); + return this; + } + + public MyObjBuilder memberVar1(NewType arg) { + this.memberVar1 = arg; + return this; + } + + public MyObj memberVar2() { + return new MyObj(); + } +} + +class Main { + public static void main(String[] args) { + LegacyType lt = new LegacyType(); + MyObj obj = MyObj.builder() + .memberVar1(lt) + .memberVar2(); + } +} + +class NewType { +} + +class MyObj { + public static MyObjBuilder builder() { + return new MyObjBuilder(); + } +} + +class LegacyType {} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/inlineMethod/ChainedBuilderCall.java.after b/java/java-tests/testData/refactoring/inlineMethod/ChainedBuilderCall.java.after new file mode 100644 index 000000000000..5bf1248c9990 --- /dev/null +++ b/java/java-tests/testData/refactoring/inlineMethod/ChainedBuilderCall.java.after @@ -0,0 +1,38 @@ + +class MyObjBuilder { + private NewType memberVar1; + + public NewType convertToNewType(LegacyType arg) { + return new NewType(); + } + + public MyObjBuilder memberVar1(NewType arg) { + this.memberVar1 = arg; + return this; + } + + public MyObj memberVar2() { + return new MyObj(); + } +} + +class Main { + public static void main(String[] args) { + LegacyType lt = new LegacyType(); + MyObjBuilder myObjBuilder = MyObj.builder(); + myObjBuilder.memberVar1(myObjBuilder.convertToNewType(lt)); + MyObj obj = myObjBuilder + .memberVar2(); + } +} + +class NewType { +} + +class MyObj { + public static MyObjBuilder builder() { + return new MyObjBuilder(); + } +} + +class LegacyType {} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/inlineMethod/MakeTypesDenotable.java.after b/java/java-tests/testData/refactoring/inlineMethod/MakeTypesDenotable.java.after index e0444cb6e478..ede96c11617e 100644 --- a/java/java-tests/testData/refactoring/inlineMethod/MakeTypesDenotable.java.after +++ b/java/java-tests/testData/refactoring/inlineMethod/MakeTypesDenotable.java.after @@ -4,6 +4,7 @@ import java.util.function.Function; class B { void foo(B sequences){ + sequences.bar(t -> t); Object t1 = null; } diff --git a/java/java-tests/testData/refactoring/inlineMethod/MissedQualifierWithSideEffectsOnInliningEmptyMethod.java b/java/java-tests/testData/refactoring/inlineMethod/MissedQualifierWithSideEffectsOnInliningEmptyMethod.java new file mode 100644 index 000000000000..c731e5d15b56 --- /dev/null +++ b/java/java-tests/testData/refactoring/inlineMethod/MissedQualifierWithSideEffectsOnInliningEmptyMethod.java @@ -0,0 +1,26 @@ +class Outer { + + public void checkIt() { + Inner inner = new Inner(); + inner.step1().step2().step3(); + System.out.println(" INNER.I = " + inner.i); + } + + class Inner { + int i = 3; + + public Inner step1() { + return this; + } + + public Inner step2() { + i = 2; + return this; + + } + + + public void step3() { + } + } +} diff --git a/java/java-tests/testData/refactoring/inlineMethod/MissedQualifierWithSideEffectsOnInliningEmptyMethod.java.after b/java/java-tests/testData/refactoring/inlineMethod/MissedQualifierWithSideEffectsOnInliningEmptyMethod.java.after new file mode 100644 index 000000000000..cbf390bb4afc --- /dev/null +++ b/java/java-tests/testData/refactoring/inlineMethod/MissedQualifierWithSideEffectsOnInliningEmptyMethod.java.after @@ -0,0 +1,24 @@ +class Outer { + + public void checkIt() { + Inner inner = new Inner(); + inner.step1().step2(); + System.out.println(" INNER.I = " + inner.i); + } + + class Inner { + int i = 3; + + public Inner step1() { + return this; + } + + public Inner step2() { + i = 2; + return this; + + } + + + } +} 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 152107ceb1ab..e0b5975b416b 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 @@ -416,6 +416,14 @@ public class InlineMethodTest extends LightRefactoringTestCase { doTest(); } + public void testChainedBuilderCall() { + doTest(); + } + + public void testMissedQualifierWithSideEffectsOnInliningEmptyMethod() { + doTest(); + } + public void testNotTailCallInsideIf() { doTestAssertBadReturn(); }