diff --git a/java/java-impl/src/com/intellij/refactoring/introduceVariable/VariableExtractor.java b/java/java-impl/src/com/intellij/refactoring/introduceVariable/VariableExtractor.java index 820b9f1f19d8..b57bb39c6131 100644 --- a/java/java-impl/src/com/intellij/refactoring/introduceVariable/VariableExtractor.java +++ b/java/java-impl/src/com/intellij/refactoring/introduceVariable/VariableExtractor.java @@ -129,19 +129,14 @@ class VariableExtractor { return; } if (myAnchor instanceof PsiExpression) { - PsiExpression place = RefactoringUtil.ensureCodeBlock(((PsiExpression)myAnchor)); - if (place == null) { + CodeBlockSurrounder surrounder = CodeBlockSurrounder.forExpression((PsiExpression)myAnchor); + if (surrounder == null) { throw new RuntimeExceptionWithAttachments( "Cannot ensure code block: myAnchor type is " + myAnchor.getClass() + "; parent type is " + myAnchor.getParent().getClass(), new Attachment("context.txt", myContainer.getText())); } - PsiElement statement = RefactoringUtil.getParentStatement(place, false); - if (statement == null) { - throw new RuntimeExceptionWithAttachments( - "Cannot find parent statement for " + place.getClass() + "; parent type is " + place.getParent().getClass(), - new Attachment("context.txt", myContainer.getText())); - } - myAnchor = statement; + CodeBlockSurrounder.SurroundResult result = surrounder.surround(); + myAnchor = result.getAnchor(); } } @@ -328,13 +323,14 @@ class VariableExtractor { } } } - if (firstOccurrence != null && ControlFlowUtils.canExtractStatement(firstOccurrence) && + if (firstOccurrence != null && CodeBlockSurrounder.canSurround(firstOccurrence) && !PsiUtil.isAccessedForWriting(firstOccurrence)) { PsiExpression ancestorCandidate = ExpressionUtils.getTopLevelExpression(firstOccurrence); if (PsiTreeUtil.isAncestor(anchor, ancestorCandidate, false)) { PsiElement statement = RefactoringUtil.getParentStatement(ancestorCandidate, false); + PsiElement extractable = statement == null ? PsiTreeUtil.getParentOfType(ancestorCandidate, PsiField.class) : statement; if (allOccurrences.stream().allMatch(occurrence -> - PsiTreeUtil.isAncestor(statement, occurrence, false) && + PsiTreeUtil.isAncestor(extractable, occurrence, false) && (!PsiTreeUtil.isAncestor(ancestorCandidate, occurrence, false) || ReorderingUtils.canExtract(ancestorCandidate, occurrence) == ThreeState.NO))) { return firstOccurrence; diff --git a/java/java-tests/testData/refactoring/introduceVariable/FieldInitializer.after.java b/java/java-tests/testData/refactoring/introduceVariable/FieldInitializer.after.java new file mode 100644 index 000000000000..6369eb93e36d --- /dev/null +++ b/java/java-tests/testData/refactoring/introduceVariable/FieldInitializer.after.java @@ -0,0 +1,15 @@ +class Test { + String s1 = getString(); + String s; + + { + if (s1 != null) { + String temp = s1.trim(); + s = temp; + } else { + s = ""; + } + } + + native String getString(); +} diff --git a/java/java-tests/testData/refactoring/introduceVariable/FieldInitializer.java b/java/java-tests/testData/refactoring/introduceVariable/FieldInitializer.java new file mode 100644 index 000000000000..122fc8b9cd9a --- /dev/null +++ b/java/java-tests/testData/refactoring/introduceVariable/FieldInitializer.java @@ -0,0 +1,6 @@ +class Test { + String s1 = getString(); + String s = s1 != null ? s1.trim() : ""; + + native String getString(); +} diff --git a/java/java-tests/testData/refactoring/introduceVariable/ReturnOrAndChain.after.java b/java/java-tests/testData/refactoring/introduceVariable/ReturnOrAndChain.after.java new file mode 100644 index 000000000000..7a7cd27dee1c --- /dev/null +++ b/java/java-tests/testData/refactoring/introduceVariable/ReturnOrAndChain.after.java @@ -0,0 +1,8 @@ +class Test { + boolean test(String s1, String s2) { + if (s1 == null) return true; + if (s2 == null) return false; + String temp = s2.trim(); + return s1.equals(temp); + } +} diff --git a/java/java-tests/testData/refactoring/introduceVariable/ReturnOrAndChain.java b/java/java-tests/testData/refactoring/introduceVariable/ReturnOrAndChain.java new file mode 100644 index 000000000000..12a7129455db --- /dev/null +++ b/java/java-tests/testData/refactoring/introduceVariable/ReturnOrAndChain.java @@ -0,0 +1,5 @@ +class Test { + boolean test(String s1, String s2) { + return s1 == null || s2 != null && s1.equals(s2.trim()); + } +} diff --git a/java/java-tests/testSrc/com/intellij/java/refactoring/IntroduceVariableTest.java b/java/java-tests/testSrc/com/intellij/java/refactoring/IntroduceVariableTest.java index 9fdc68f44ec8..038bd3531248 100644 --- a/java/java-tests/testSrc/com/intellij/java/refactoring/IntroduceVariableTest.java +++ b/java/java-tests/testSrc/com/intellij/java/refactoring/IntroduceVariableTest.java @@ -145,7 +145,9 @@ public class IntroduceVariableTest extends LightJavaCodeInsightTestCase { public void testIfConditionAndChain() { doTest("temp", true, false, false, JAVA_LANG_STRING); } public void testReturnAndChain() { doTest("temp", true, false, false, JAVA_LANG_STRING); } public void testReturnOrChain() { doTest("temp", true, false, false, JAVA_LANG_STRING); } + public void testReturnOrAndChain() { doTest("temp", true, false, false, JAVA_LANG_STRING); } public void testReturnTernary() { doTest("temp", true, false, false, JAVA_LANG_STRING); } + public void testFieldInitializer() { doTest("temp", true, false, false, JAVA_LANG_STRING); } public void testAssignTernary() { doTest("temp", true, false, false, JAVA_LANG_STRING); } public void testEnsureCodeBlockAroundBreakStatementJava13Preview() { doTest("temp", true, false, false, JAVA_LANG_STRING); } public void testEnsureCodeBlockForThrowsJava13Preview() { doTest("temp", true, false, false, JAVA_LANG_STRING); } diff --git a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/ReorderingUtils.java b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/ReorderingUtils.java index 637952dcf6bb..0d0be710a326 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/ReorderingUtils.java +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/ReorderingUtils.java @@ -34,10 +34,10 @@ public class ReorderingUtils { public static ThreeState canExtract(@NotNull PsiExpression ancestor, @NotNull PsiExpression expression) { if (expression == ancestor) return ThreeState.YES; if (PsiUtil.isConstantExpression(expression)) return ThreeState.YES; - PsiExpression parent = ObjectUtils.tryCast(expression.getParent(), PsiExpression.class); + PsiElement parent = expression.getParent(); if (parent instanceof PsiExpressionList) { - PsiExpression gParent = ObjectUtils.tryCast(parent.getParent(), PsiExpression.class); - if (gParent instanceof PsiCallExpression) { + PsiExpression gParent = ObjectUtils.tryCast(parent.getParent(), PsiCallExpression.class); + if (gParent != null) { PsiExpression[] args = ((PsiExpressionList)parent).getExpressions(); int index = ArrayUtil.indexOf(args, expression); ThreeState result = ThreeState.YES; @@ -47,29 +47,30 @@ public class ReorderingUtils { break; } } - return and(result, () -> canExtract(ancestor, parent)); + return and(result, () -> canExtract(ancestor, gParent)); } } - if (parent == null) { + PsiExpression expressionParent = ObjectUtils.tryCast(parent, PsiExpression.class); + if (expressionParent == null) { if (PsiTreeUtil.isAncestor(ancestor, expression, true)) { return ThreeState.UNSURE; } throw new IllegalArgumentException("Should be an ancestor"); } - if (parent instanceof PsiParenthesizedExpression || parent instanceof PsiInstanceOfExpression || - parent instanceof PsiTypeCastExpression) { - return canExtract(ancestor, parent); + if (expressionParent instanceof PsiParenthesizedExpression || expressionParent instanceof PsiInstanceOfExpression || + expressionParent instanceof PsiTypeCastExpression) { + return canExtract(ancestor, expressionParent); } - if (parent instanceof PsiReferenceExpression) { - if (((PsiReferenceExpression)parent).getQualifierExpression() == expression) { - return canExtract(ancestor, parent); + if (expressionParent instanceof PsiReferenceExpression) { + if (((PsiReferenceExpression)expressionParent).getQualifierExpression() == expression) { + return canExtract(ancestor, expressionParent); } } - if (parent instanceof PsiConditionalExpression) { - PsiConditionalExpression ternary = (PsiConditionalExpression)parent; + if (expressionParent instanceof PsiConditionalExpression) { + PsiConditionalExpression ternary = (PsiConditionalExpression)expressionParent; PsiExpression condition = ternary.getCondition(); if (condition == expression) { - return canExtract(ancestor, parent); + return canExtract(ancestor, expressionParent); } ThreeState result; if (isSideEffectFree(condition, false) && @@ -80,25 +81,25 @@ public class ReorderingUtils { areConditionsNecessaryFor(new PsiExpression[]{condition}, expression, ternary.getElseExpression() == expression); result = isNecessary ? ThreeState.NO : ThreeState.UNSURE; } - return and(result, () -> canExtract(ancestor, parent)); + return and(result, () -> canExtract(ancestor, expressionParent)); } - if (parent instanceof PsiLambdaExpression) { + if (expressionParent instanceof PsiLambdaExpression) { return ThreeState.NO; } - if (parent instanceof PsiUnaryExpression) { - if (PsiUtil.isIncrementDecrementOperation(parent)) return ThreeState.NO; - return canExtract(ancestor, parent); + if (expressionParent instanceof PsiUnaryExpression) { + if (PsiUtil.isIncrementDecrementOperation(expressionParent)) return ThreeState.NO; + return canExtract(ancestor, expressionParent); } - if (parent instanceof PsiPolyadicExpression) { - PsiPolyadicExpression polyadic = (PsiPolyadicExpression)parent; + if (expressionParent instanceof PsiPolyadicExpression) { + PsiPolyadicExpression polyadic = (PsiPolyadicExpression)expressionParent; PsiExpression[] operands = polyadic.getOperands(); int index = ArrayUtil.indexOf(operands, expression); if (index == 0) { - return canExtract(ancestor, parent); + return canExtract(ancestor, expressionParent); } IElementType tokenType = polyadic.getOperationTokenType(); if (tokenType.equals(JavaTokenType.ANDAND) || tokenType.equals(JavaTokenType.OROR)) { - return and(canMoveToStart(polyadic, index), () -> canExtract(ancestor, parent)); + return and(canMoveToStart(polyadic, index), () -> canExtract(ancestor, expressionParent)); } ThreeState result = ThreeState.YES; for (int i=0; i canExtract(ancestor, parent)); + return and(result, () -> canExtract(ancestor, expressionParent)); } - if (parent instanceof PsiAssignmentExpression) { - if (expression == ((PsiAssignmentExpression)parent).getLExpression()) return ThreeState.NO; - return canExtract(ancestor, parent); + if (expressionParent instanceof PsiAssignmentExpression) { + if (expression == ((PsiAssignmentExpression)expressionParent).getLExpression()) return ThreeState.NO; + return canExtract(ancestor, expressionParent); } - return and(ThreeState.UNSURE, () -> canExtract(ancestor, parent)); + return and(ThreeState.UNSURE, () -> canExtract(ancestor, expressionParent)); } @NotNull