From 2e6d3a1baa6a09d290d2522922966133bb7a237f Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Tue, 16 Apr 2019 12:07:02 +0700 Subject: [PATCH] InlineMethodProcessor: improved handling of unused return value in case of multiple returns; misc warnings/typos fixed --- .../inline/InlineMethodProcessor.java | 54 ++++++++----------- .../inlineMethod/NotAStatement3.java | 15 ++++++ .../inlineMethod/NotAStatement3.java.after | 14 +++++ .../inlineMethod/NotAStatement4.java | 16 ++++++ .../inlineMethod/NotAStatement4.java.after | 18 +++++++ .../refactoring/inline/InlineMethodTest.java | 8 +++ 6 files changed, 93 insertions(+), 32 deletions(-) create mode 100644 java/java-tests/testData/refactoring/inlineMethod/NotAStatement3.java create mode 100644 java/java-tests/testData/refactoring/inlineMethod/NotAStatement3.java.after create mode 100644 java/java-tests/testData/refactoring/inlineMethod/NotAStatement4.java create mode 100644 java/java-tests/testData/refactoring/inlineMethod/NotAStatement4.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 4233727cdaa3..a02aae8cf622 100644 --- a/java/java-impl/src/com/intellij/refactoring/inline/InlineMethodProcessor.java +++ b/java/java-impl/src/com/intellij/refactoring/inline/InlineMethodProcessor.java @@ -2,6 +2,7 @@ package com.intellij.refactoring.inline; import com.intellij.codeInsight.AnnotationUtil; +import com.intellij.codeInsight.BlockUtils; import com.intellij.codeInsight.ChangeContextUtil; import com.intellij.codeInsight.ExpressionUtil; import com.intellij.codeInsight.daemon.impl.quickfix.RemoveUnusedVariableUtil; @@ -200,9 +201,10 @@ public class InlineMethodProcessor extends BaseRefactoringProcessor { if (!myInlineThisOnly) { final PsiMethod[] superMethods = myMethod.findSuperMethods(); for (PsiMethod method : superMethods) { - final String message = method.hasModifierProperty(PsiModifier.ABSTRACT) ? RefactoringBundle - .message("inlined.method.implements.method.from.0", method.getContainingClass().getQualifiedName()) : RefactoringBundle - .message("inlined.method.overrides.method.from.0", method.getContainingClass().getQualifiedName()); + String className = Objects.requireNonNull(method.getContainingClass()).getQualifiedName(); + final String message = method.hasModifierProperty(PsiModifier.ABSTRACT) ? + RefactoringBundle.message("inlined.method.implements.method.from.0", className) : + RefactoringBundle.message("inlined.method.overrides.method.from.0", className); conflicts.putValue(method, message); } @@ -321,7 +323,7 @@ public class InlineMethodProcessor extends BaseRefactoringProcessor { /** * Given a set of referencedElements, returns a map from containers (in a sense of ConflictsUtil.getContainer) - * to subsets of referencedElemens that are not accessible from that container + * to subsets of referencedElements that are not accessible from that container * * @param referencedElements * @param usages @@ -893,36 +895,26 @@ public class InlineMethodProcessor extends BaseRefactoringProcessor { for (PsiReturnStatement returnStatement : returnStatements) { final PsiExpression returnValue = returnStatement.getReturnValue(); if (returnValue == null) continue; - PsiStatement statement; if (tailCallType == InlineUtil.TailCallType.Simple) { - if (returnStatement.getNextSibling() == myMethodCopy.getBody().getLastBodyElement() && - RemoveUnusedVariableUtil.checkSideEffects(returnValue, null, new ArrayList<>())) { - PsiExpressionStatement exprStatement = (PsiExpressionStatement) myFactory.createStatementFromText("a;", null); - exprStatement.getExpression().replace(returnValue); - PsiElement returnParent = returnStatement.getParent(); - exprStatement = (PsiExpressionStatement)returnParent.addBefore(exprStatement, returnStatement); - if (!PsiUtil.isStatement(exprStatement)) { - PsiExpression expression = exprStatement.getExpression(); - List sideEffects = SideEffectChecker.extractSideEffectExpressions(expression); - CommentTracker ct = new CommentTracker(); - sideEffects.forEach(ct::markUnchanged); - for (PsiStatement sideEffect : StatementExtractor.generateStatements(sideEffects, expression)) { - returnParent.addBefore(sideEffect, exprStatement); - } - ct.deleteAndRestoreComments(exprStatement); - } - statement = myFactory.createStatementFromText("return;", null); - } else { - statement = (PsiStatement)returnStatement.copy(); + List sideEffects = SideEffectChecker.extractSideEffectExpressions(returnValue); + CommentTracker ct = new CommentTracker(); + sideEffects.forEach(ct::markUnchanged); + PsiStatement[] statements = StatementExtractor.generateStatements(sideEffects, returnValue); + ct.delete(returnValue); + if (statements.length > 0) { + PsiStatement lastAdded = BlockUtils.addBefore(returnStatement, statements); + // Could be wrapped into {}, so returnStatement might be non-physical anymore + returnStatement = Objects.requireNonNull(PsiTreeUtil.getNextSiblingOfType(lastAdded, PsiReturnStatement.class)); } + ct.insertCommentsBefore(returnStatement); } else { - statement = myFactory.createStatementFromText(resultName + "=0;", null); + PsiStatement statement = myFactory.createStatementFromText(resultName + "=0;", null); statement = (PsiStatement)myCodeStyleManager.reformat(statement); PsiAssignmentExpression assignment = (PsiAssignmentExpression)((PsiExpressionStatement)statement).getExpression(); assignment.getRExpression().replace(returnValue); + returnStatement.replace(statement); } - returnStatement.replace(statement); } } @@ -978,7 +970,6 @@ public class InlineMethodProcessor extends BaseRefactoringProcessor { do { parentClass = PsiTreeUtil.getParentOfType(parentClass, PsiClass.class, true); if (InheritanceUtil.isInheritorOrSelf(parentClass, containingClass, true)) { - LOG.assertTrue(parentClass != null); final String childClassName = parentClass.getName(); qualifier = myFactory.createExpressionFromText(childClassName != null ? childClassName + ".this" : "this", null); break; @@ -1129,7 +1120,7 @@ public class InlineMethodProcessor extends BaseRefactoringProcessor { //TODO: other cases return false; */ - return true; //TODO: "suspicous" places to review by user! + return true; //TODO: "suspicious" places to review by user! } else { if (isAccessedForWriting) { @@ -1169,7 +1160,7 @@ public class InlineMethodProcessor extends BaseRefactoringProcessor { return false; } } - return true; //TODO: "suspicous" places to review by user! + return true; //TODO: "suspicious" places to review by user! } else if (initializer instanceof PsiLiteralExpression) { return true; @@ -1615,8 +1606,7 @@ public class InlineMethodProcessor extends BaseRefactoringProcessor { for (PsiReturnStatement aReturn : returns) { int offset = controlFlow.getEndOffset(aReturn); - while (true) { - if (offset == instructions.size()) break; + while (offset != instructions.size()) { Instruction instruction = instructions.get(offset); if (instruction instanceof GoToInstruction) { offset = ((GoToInstruction)instruction).offset; @@ -1626,7 +1616,7 @@ public class InlineMethodProcessor extends BaseRefactoringProcessor { } else if (instruction instanceof ConditionalThrowToInstruction) { // In case of "conditional throw to", control flow will not be altered - // If exception handler is in method, we will inline it to ivokation site + // If exception handler is in method, we will inline it to invocation site // If exception handler is at invocation site, execution will continue to get there offset++; } diff --git a/java/java-tests/testData/refactoring/inlineMethod/NotAStatement3.java b/java/java-tests/testData/refactoring/inlineMethod/NotAStatement3.java new file mode 100644 index 000000000000..bfba019ede7f --- /dev/null +++ b/java/java-tests/testData/refactoring/inlineMethod/NotAStatement3.java @@ -0,0 +1,15 @@ +import java.util.*; + +class AAA { + boolean addIntoTwoSets(Set set1, Set set2, String value) { + if (set1.isEmpty()) + return set1.add(value) & set2.add(value); + return false; + } + + void usage() { + Set s1 = new HashSet<>(); + Set s2 = new HashSet<>(); + addIntoTwoSets(s1, s2, "foo"); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/inlineMethod/NotAStatement3.java.after b/java/java-tests/testData/refactoring/inlineMethod/NotAStatement3.java.after new file mode 100644 index 000000000000..bad21c672024 --- /dev/null +++ b/java/java-tests/testData/refactoring/inlineMethod/NotAStatement3.java.after @@ -0,0 +1,14 @@ +import java.util.*; + +class AAA { + + void usage() { + Set s1 = new HashSet<>(); + Set s2 = new HashSet<>(); + if (s1.isEmpty()) { + s1.add("foo"); + s2.add("foo"); + return; + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/inlineMethod/NotAStatement4.java b/java/java-tests/testData/refactoring/inlineMethod/NotAStatement4.java new file mode 100644 index 000000000000..788fce6712be --- /dev/null +++ b/java/java-tests/testData/refactoring/inlineMethod/NotAStatement4.java @@ -0,0 +1,16 @@ +import java.util.*; + +class AAA { + boolean addIntoTwoSets(Set set1, Set set2, String value) { + if (set1.isEmpty()) + return set1.add(value) & set2.add(value); + else + return set1.add(value) | set2.add(value); + } + + void foo() { + Set s1 = new HashSet<>(); + Set s2 = new HashSet<>(); + addIntoTwoSets(s1, s2, "foo"); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/inlineMethod/NotAStatement4.java.after b/java/java-tests/testData/refactoring/inlineMethod/NotAStatement4.java.after new file mode 100644 index 000000000000..1fdc4a79e8df --- /dev/null +++ b/java/java-tests/testData/refactoring/inlineMethod/NotAStatement4.java.after @@ -0,0 +1,18 @@ +import java.util.*; + +class AAA { + + void foo() { + Set s1 = new HashSet<>(); + Set s2 = new HashSet<>(); + 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/testSrc/com/intellij/java/refactoring/inline/InlineMethodTest.java b/java/java-tests/testSrc/com/intellij/java/refactoring/inline/InlineMethodTest.java index 335f20e10f5e..ccd585d76e4f 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 @@ -240,6 +240,14 @@ public class InlineMethodTest extends LightRefactoringTestCase { public void testNotAStatement2() { doTest(); } + + public void testNotAStatement3() { + doTest(); + } + + public void testNotAStatement4() { + doTest(); + } public void testInSuperCall() { doTestConflict("Inline cannot be applied to multiline method in constructor call");