From e126ef68c1dd2adb1bb438e48f97e8bc5ac786fc Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Fri, 12 Oct 2018 17:09:56 +0700 Subject: [PATCH] VariableExtractor: further refactoring; support while conditions --- .../IntroduceVariableBase.java | 3 +- .../introduceVariable/VariableExtractor.java | 122 ++++++++++-------- .../introduceVariable/SCR26075.after.java | 5 +- .../introduceVariable/SCR26075For.after.java | 15 +++ .../introduceVariable/SCR26075For.java | 14 ++ .../WhileCondition.after.java | 14 ++ .../introduceVariable/WhileCondition.java | 12 ++ .../WhileCondition2.after.java | 16 +++ .../introduceVariable/WhileCondition2.java | 14 ++ .../refactoring/IntroduceVariableTest.java | 12 ++ 10 files changed, 166 insertions(+), 61 deletions(-) create mode 100644 java/java-tests/testData/refactoring/introduceVariable/SCR26075For.after.java create mode 100644 java/java-tests/testData/refactoring/introduceVariable/SCR26075For.java create mode 100644 java/java-tests/testData/refactoring/introduceVariable/WhileCondition.after.java create mode 100644 java/java-tests/testData/refactoring/introduceVariable/WhileCondition.java create mode 100644 java/java-tests/testData/refactoring/introduceVariable/WhileCondition2.after.java create mode 100644 java/java-tests/testData/refactoring/introduceVariable/WhileCondition2.java diff --git a/java/java-impl/src/com/intellij/refactoring/introduceVariable/IntroduceVariableBase.java b/java/java-impl/src/com/intellij/refactoring/introduceVariable/IntroduceVariableBase.java index e2e49561b184..43a88ff220c8 100644 --- a/java/java-impl/src/com/intellij/refactoring/introduceVariable/IntroduceVariableBase.java +++ b/java/java-impl/src/com/intellij/refactoring/introduceVariable/IntroduceVariableBase.java @@ -665,7 +665,6 @@ public abstract class IntroduceVariableBase extends IntroduceHandlerBase { final boolean cantChangeFinalModifier = (hasWriteAccess || inFinalContext) && allChoice; PsiExpression[] allOccurrences = Arrays.stream(occurrences) - .filter(occurrence -> !(expr.equals(occurrence) && expr.getParent() instanceof PsiExpressionStatement)) .filter(occurrence -> allChoice || (noWriteChoice && !PsiUtil.isAccessedForWriting(occurrence)) || expr.equals(occurrence)) .toArray(PsiExpression[]::new); if (choice.isChain()) { @@ -1055,7 +1054,7 @@ public abstract class IntroduceVariableBase extends IntroduceHandlerBase { public static void checkInLoopCondition(PsiExpression occurence, MultiMap conflicts) { final PsiElement loopForLoopCondition = RefactoringUtil.getLoopForLoopCondition(occurence); - if (loopForLoopCondition == null) return; + if (loopForLoopCondition == null || loopForLoopCondition instanceof PsiWhileStatement) return; final List referencedVariables = RefactoringUtil.collectReferencedVariables(occurence); final List modifiedInBody = new ArrayList<>(); for (PsiVariable psiVariable : referencedVariables) { 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 91110a5b60b2..77560638eeb7 100644 --- a/java/java-impl/src/com/intellij/refactoring/introduceVariable/VariableExtractor.java +++ b/java/java-impl/src/com/intellij/refactoring/introduceVariable/VariableExtractor.java @@ -1,6 +1,7 @@ // Copyright 2000-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE file. package com.intellij.refactoring.introduceVariable; +import com.intellij.codeInsight.BlockUtils; import com.intellij.codeInsight.NullableNotNullManager; import com.intellij.openapi.application.ApplicationManager; import com.intellij.openapi.diagnostic.Logger; @@ -17,7 +18,11 @@ import com.intellij.refactoring.util.FieldConflictsResolver; import com.intellij.refactoring.util.RefactoringUtil; import com.intellij.util.ArrayUtilRt; import com.intellij.util.IncorrectOperationException; +import com.intellij.util.ObjectUtils; +import com.intellij.util.containers.ContainerUtil; import com.siyeh.ig.psiutils.CommentTracker; +import com.siyeh.ig.psiutils.ExpressionUtils; +import one.util.streamex.StreamEx; import org.jetbrains.annotations.NotNull; import java.util.Arrays; @@ -38,10 +43,7 @@ class VariableExtractor { private PsiElement myAnchor; private final PsiElement myContainer; private final PsiExpression[] myOccurrences; - private final boolean myIsInsideLoop; private final boolean myReplaceSelf; - private final boolean myDeleteSelf; - private final boolean myReplaceLoop; private final FieldConflictsResolver myFieldConflictsResolver; private final LogicalPosition myPosition; @@ -57,34 +59,16 @@ class VariableExtractor { myOccurrences = occurrences; mySettings = settings; myContainer = anchorStatement.getParent(); - myIsInsideLoop = RefactoringUtil.isLoopOrIf(myContainer); - myAnchor = correctAnchor(expression, anchorStatement, myIsInsideLoop); + myAnchor = correctAnchor(expression, anchorStatement, occurrences); myReplaceSelf = settings.isReplaceLValues() || !RefactoringUtil.isAssignmentLHS(expression); - PsiElement expressionParent = expression.getParent(); - myReplaceLoop = myIsInsideLoop ? expressionParent instanceof PsiExpressionStatement - : myContainer instanceof PsiLambdaExpression && expressionParent == myContainer; PsiCodeBlock newDeclarationScope = PsiTreeUtil.getParentOfType(myContainer, PsiCodeBlock.class, false); myFieldConflictsResolver = new FieldConflictsResolver(settings.getEnteredName(), newDeclarationScope); - - myDeleteSelf = shouldDeleteSelf(anchorStatement, expressionParent); myPosition = editor != null ? editor.getCaretModel().getLogicalPosition() : null; } - private boolean shouldDeleteSelf(PsiElement origAnchor, PsiElement expressionParent) { - if (!myIsInsideLoop && myReplaceSelf && expressionParent instanceof PsiExpressionStatement && myAnchor.equals(origAnchor)) { - PsiElement parent = expressionParent.getParent(); - return parent instanceof PsiCodeBlock || - //fabrique - parent instanceof PsiCodeFragment; - } - return false; - } - private SmartPsiElementPointer extractVariable() { ApplicationManager.getApplication().assertWriteAccessAllowed(); try { - PsiElement statement = myExpression.getParent(); - final PsiExpression newExpr = myFieldConflictsResolver.fixInitializer(myExpression); PsiExpression initializer = RefactoringUtil.unparenthesizeExpression(newExpr); final SmartTypePointer selectedType = SmartTypePointerManager.getInstance(myProject).createSmartTypePointer( @@ -97,33 +81,23 @@ class VariableExtractor { PsiType type = stripNullabilityAnnotationsFromTargetType(selectedType, myProject); replaceOccurrences(newExpr); - PsiElement declaration = createDeclaration(type, mySettings.getEnteredName(), initializer); - if (!myIsInsideLoop) { - declaration = addDeclaration(declaration, initializer); - if (myDeleteSelf) { - commentTracker.deleteAndRestoreComments(statement); - if (myEditor != null) { - myEditor.getCaretModel().moveToLogicalPosition(myPosition); - myEditor.getCaretModel().moveToOffset(declaration.getTextRange().getEndOffset()); - myEditor.getScrollingModel().scrollToCaret(ScrollType.RELATIVE); - myEditor.getSelectionModel().removeSelection(); - } + ensureCodeBlock(); + + PsiVariable var = createVariable(type, initializer); + + if (myAnchor instanceof PsiExpressionStatement && + ExpressionUtils.isReferenceTo(((PsiExpressionStatement)myAnchor).getExpression(), var)) { + commentTracker.deleteAndRestoreComments(myAnchor); + if (myEditor != null) { + myEditor.getCaretModel().moveToLogicalPosition(myPosition); + myEditor.getCaretModel().moveToOffset(var.getTextRange().getEndOffset()); + myEditor.getScrollingModel().scrollToCaret(ScrollType.RELATIVE); + myEditor.getSelectionModel().removeSelection(); } } - PsiVariable var = (PsiVariable)(declaration instanceof PsiDeclarationStatement - ? ((PsiDeclarationStatement)declaration).getDeclaredElements()[0] - : declaration); highlight(var); - if (declaration instanceof PsiDeclarationStatement) { - declaration = RefactoringUtil.putStatementInLoopBody((PsiStatement)declaration, myContainer, myAnchor, - myReplaceSelf && myReplaceLoop); - } - declaration = JavaCodeStyleManager.getInstance(myProject).shortenClassReferences(declaration); - var = (PsiVariable)(declaration instanceof PsiDeclarationStatement - ? ((PsiDeclarationStatement)declaration).getDeclaredElements()[0] - : declaration); PsiUtil.setModifierProperty(var, PsiModifier.FINAL, mySettings.isDeclareFinal()); myFieldConflictsResolver.fix(); return SmartPointerManager.getInstance(myProject).createSmartPsiElementPointer(var); @@ -134,6 +108,24 @@ class VariableExtractor { return null; } + private PsiVariable createVariable(PsiType type, PsiExpression initializer) { + PsiElement declaration = createDeclaration(type, mySettings.getEnteredName(), initializer); + declaration = addDeclaration(declaration, initializer, myAnchor); + declaration = JavaCodeStyleManager.getInstance(myProject).shortenClassReferences(declaration); + return (PsiVariable)(declaration instanceof PsiDeclarationStatement + ? ((PsiDeclarationStatement)declaration).getDeclaredElements()[0] + : declaration); + } + + private void ensureCodeBlock() { + if (myAnchor instanceof PsiStatement && RefactoringUtil.isLoopOrIf(myAnchor.getParent())) { + myAnchor = BlockUtils.expandSingleStatementToBlockStatement((PsiStatement)myAnchor); + } + if (myAnchor instanceof PsiExpression) { + myAnchor = RefactoringUtil.getParentStatement(RefactoringUtil.ensureCodeBlock(((PsiExpression)myAnchor)), false); + } + } + private void highlight(PsiVariable var) { if (myEditor != null) { PsiElement[] occurrences = @@ -145,10 +137,9 @@ class VariableExtractor { private void replaceOccurrences(PsiExpression newExpr) { PsiElementFactory elementFactory = JavaPsiFacade.getElementFactory(myProject); PsiExpression ref = elementFactory.createExpressionFromText(mySettings.getEnteredName(), null); - boolean needReplaceSelf = !myDeleteSelf && myReplaceSelf; + boolean needReplaceSelf = myReplaceSelf; if (mySettings.isReplaceAllOccurrences()) { for (PsiExpression occurrence : myOccurrences) { - if (myDeleteSelf && occurrence.equals(myExpression)) continue; PsiExpression correctedOccurrence = occurrence.equals(myExpression) ? newExpr : occurrence; correctedOccurrence = RefactoringUtil.outermostParenthesizedExpression(correctedOccurrence); if (mySettings.isReplaceLValues() || !RefactoringUtil.isAssignmentLHS(correctedOccurrence)) { @@ -180,9 +171,9 @@ class VariableExtractor { return elementFactory.createVariableDeclarationStatement(name, type, initializer, myContainer); } - private PsiElement addDeclaration(PsiElement declaration, PsiExpression initializer) { - if (myAnchor instanceof PsiDeclarationStatement) { - final PsiElement[] declaredElements = ((PsiDeclarationStatement)myAnchor).getDeclaredElements(); + private static PsiElement addDeclaration(PsiElement declaration, PsiExpression initializer, PsiElement anchor) { + if (anchor instanceof PsiDeclarationStatement) { + final PsiElement[] declaredElements = ((PsiDeclarationStatement)anchor).getDeclaredElements(); if (declaredElements.length > 1) { final int[] usedFirstVar = new int[]{-1}; initializer.accept(new JavaRecursiveElementWalkingVisitor() { @@ -199,11 +190,11 @@ class VariableExtractor { final PsiVariable psiVariable = (PsiVariable)declaredElements[usedFirstVar[0]]; psiVariable.normalizeDeclaration(); final PsiDeclarationStatement parDeclarationStatement = PsiTreeUtil.getParentOfType(psiVariable, PsiDeclarationStatement.class); - return myContainer.addAfter(declaration, parDeclarationStatement); + return anchor.getParent().addAfter(declaration, parDeclarationStatement); } } } - return myContainer.addBefore(declaration, myAnchor); + return anchor.getParent().addBefore(declaration, anchor); } @NotNull @@ -229,14 +220,31 @@ class VariableExtractor { } @NotNull - private static PsiElement correctAnchor(PsiExpression expr, @NotNull PsiElement anchorStatement, boolean isInsideLoop) { - PsiElement child = anchorStatement; - if (!isInsideLoop) { - child = locateAnchor(child); - if (IntroduceVariableBase.isFinalVariableOnLHS(expr)) { - child = child.getNextSibling(); + private static PsiElement correctAnchor(PsiExpression expr, + @NotNull PsiElement anchorStatement, + PsiExpression[] occurrences) { + if (anchorStatement instanceof PsiWhileStatement) { + PsiExpression condition = ((PsiWhileStatement)anchorStatement).getCondition(); + if (condition != null) { + PsiExpression expression = StreamEx + .of(occurrences).filter(occ -> PsiTreeUtil.isAncestor(condition, occ, false)) + .minBy(e -> e.getTextRange().getStartOffset()).orElse(null); + if (expression != null) { + PsiPolyadicExpression polyadic = ObjectUtils.tryCast(PsiUtil.skipParenthesizedExprDown(condition), PsiPolyadicExpression.class); + if (polyadic != null && JavaTokenType.ANDAND.equals(polyadic.getOperationTokenType())) { + PsiExpression anchor = ContainerUtil.find(polyadic.getOperands(), op -> PsiTreeUtil.isAncestor(op, expression, false)); + LOG.assertTrue(anchor != null); + return anchor; + } + return condition; + } } } + if (RefactoringUtil.isLoopOrIf(anchorStatement.getParent())) return anchorStatement; + PsiElement child = locateAnchor(anchorStatement); + if (IntroduceVariableBase.isFinalVariableOnLHS(expr)) { + child = child.getNextSibling(); + } return child == null ? anchorStatement : child; } diff --git a/java/java-tests/testData/refactoring/introduceVariable/SCR26075.after.java b/java/java-tests/testData/refactoring/introduceVariable/SCR26075.after.java index c93d770a0820..b07a3d1941fc 100644 --- a/java/java-tests/testData/refactoring/introduceVariable/SCR26075.after.java +++ b/java/java-tests/testData/refactoring/introduceVariable/SCR26075.after.java @@ -7,8 +7,9 @@ class C { void test () { C c = new C(); - String wrong = c.f(); - while (wrong != null) { + while (true) { + String ok = c.f(); + if (!(ok != null)) break; c = c.getParent(); } } diff --git a/java/java-tests/testData/refactoring/introduceVariable/SCR26075For.after.java b/java/java-tests/testData/refactoring/introduceVariable/SCR26075For.after.java new file mode 100644 index 000000000000..b82256031e10 --- /dev/null +++ b/java/java-tests/testData/refactoring/introduceVariable/SCR26075For.after.java @@ -0,0 +1,15 @@ +class C { + String f () { + return null; + } + + C getParent() {return null;} + + void test () { + C c = new C(); + String wrong = c.f(); + for (; wrong != null;) { + c = c.getParent(); + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/introduceVariable/SCR26075For.java b/java/java-tests/testData/refactoring/introduceVariable/SCR26075For.java new file mode 100644 index 000000000000..90731ca51c9a --- /dev/null +++ b/java/java-tests/testData/refactoring/introduceVariable/SCR26075For.java @@ -0,0 +1,14 @@ +class C { + String f () { + return null; + } + + C getParent() {return null;} + + void test () { + C c = new C(); + for (;c.f() != null;) { + c = c.getParent(); + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/introduceVariable/WhileCondition.after.java b/java/java-tests/testData/refactoring/introduceVariable/WhileCondition.after.java new file mode 100644 index 000000000000..b37add3362e9 --- /dev/null +++ b/java/java-tests/testData/refactoring/introduceVariable/WhileCondition.after.java @@ -0,0 +1,14 @@ +class Test { + + Node findRoot(Node n) { + while(true) { + Node temp = n.getParent(); + if (!(temp != null)) break; + n = temp; + } + return n; + } +} +interface Node { + Node getParent(); +} diff --git a/java/java-tests/testData/refactoring/introduceVariable/WhileCondition.java b/java/java-tests/testData/refactoring/introduceVariable/WhileCondition.java new file mode 100644 index 000000000000..0cef6e8bd01e --- /dev/null +++ b/java/java-tests/testData/refactoring/introduceVariable/WhileCondition.java @@ -0,0 +1,12 @@ +class Test { + + Node findRoot(Node n) { + while(n.getParent() != null) { + n = n.getParent(); + } + return n; + } +} +interface Node { + Node getParent(); +} diff --git a/java/java-tests/testData/refactoring/introduceVariable/WhileCondition2.after.java b/java/java-tests/testData/refactoring/introduceVariable/WhileCondition2.after.java new file mode 100644 index 000000000000..83775cd84ce0 --- /dev/null +++ b/java/java-tests/testData/refactoring/introduceVariable/WhileCondition2.after.java @@ -0,0 +1,16 @@ +class Test { + + boolean native isCancelled(); + + Node findRoot(Node n) { + while(!isCancelled()) { + Node temp = n.getParent(); + if (!(temp != null)) break; + n = temp; + } + return n; + } +} +interface Node { + Node getParent(); +} diff --git a/java/java-tests/testData/refactoring/introduceVariable/WhileCondition2.java b/java/java-tests/testData/refactoring/introduceVariable/WhileCondition2.java new file mode 100644 index 000000000000..733c23daea30 --- /dev/null +++ b/java/java-tests/testData/refactoring/introduceVariable/WhileCondition2.java @@ -0,0 +1,14 @@ +class Test { + + boolean native isCancelled(); + + Node findRoot(Node n) { + while(!isCancelled() && n.getParent() != null) { + n = n.getParent(); + } + return n; + } +} +interface Node { + Node getParent(); +} 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 bbbfdba1ea38..509048f190a6 100644 --- a/java/java-tests/testSrc/com/intellij/java/refactoring/IntroduceVariableTest.java +++ b/java/java-tests/testSrc/com/intellij/java/refactoring/IntroduceVariableTest.java @@ -155,6 +155,10 @@ public class IntroduceVariableTest extends LightCodeInsightTestCase { } public void testSCR26075() { + doTest(new MockIntroduceVariableHandler("ok", false, false, false, CommonClassNames.JAVA_LANG_STRING)); + } + + public void testSCR26075For() { doTest(new MockIntroduceVariableHandler("wrong", false, false, false, CommonClassNames.JAVA_LANG_STRING) { @Override protected void assertValidationResult(boolean validationResult) { @@ -192,6 +196,14 @@ public class IntroduceVariableTest extends LightCodeInsightTestCase { doTest(new MockIntroduceVariableHandler("temp", false, false, false, "int")); } + public void testWhileCondition() { + doTest(new MockIntroduceVariableHandler("temp", true, false, false, "Node")); + } + + public void testWhileCondition2() { + doTest(new MockIntroduceVariableHandler("temp", true, false, false, "Node")); + } + public void testSCR40281() { doTest(new MockIntroduceVariableHandler("temp", false, false, false, "Set.Entry>")); }