From 6ae43d6012c381fbe3fa732b96c326c268f6d37f Mon Sep 17 00:00:00 2001 From: Bas Leijdekkers Date: Tue, 6 Apr 2010 08:08:02 +0200 Subject: [PATCH] IDEA-53497 ('Merge nested IFs' refactoring ignores change in the condition) --- .../ipp/psiutils/VariableAccessUtils.java | 52 ++++- .../ipp/psiutils/VariableAssignedVisitor.java | 196 ++++++++++++++++++ .../trivialif/MergeParallelIfsIntention.java | 20 +- .../trivialif/MergeParallelIfsPredicate.java | 40 ++-- 4 files changed, 285 insertions(+), 23 deletions(-) create mode 100644 plugins/IntentionPowerPak/src/com/siyeh/ipp/psiutils/VariableAssignedVisitor.java diff --git a/plugins/IntentionPowerPak/src/com/siyeh/ipp/psiutils/VariableAccessUtils.java b/plugins/IntentionPowerPak/src/com/siyeh/ipp/psiutils/VariableAccessUtils.java index 0482259163d6..6ccc4aaf0eac 100644 --- a/plugins/IntentionPowerPak/src/com/siyeh/ipp/psiutils/VariableAccessUtils.java +++ b/plugins/IntentionPowerPak/src/com/siyeh/ipp/psiutils/VariableAccessUtils.java @@ -1,5 +1,5 @@ /* - * Copyright 2009 Bas Leijdekkers + * Copyright 2009-2010 Bas Leijdekkers * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -20,6 +20,11 @@ import com.intellij.psi.tree.IElementType; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import java.util.Collection; +import java.util.Collections; +import java.util.HashSet; +import java.util.Set; + public class VariableAccessUtils { private VariableAccessUtils() { @@ -137,4 +142,49 @@ public class VariableAccessUtils { final PsiElement referent = referenceExpression.resolve(); return variable.equals(referent); } + + public static boolean isAnyVariableAssigned( + @NotNull Collection variables, + @Nullable PsiElement context) { + if (context == null) { + return false; + } + final VariableAssignedVisitor visitor = + new VariableAssignedVisitor(variables, true); + context.accept(visitor); + return visitor.isAssigned(); + } + + public static Set collectUsedVariables( + PsiElement context) { + if (context == null) { + return Collections.EMPTY_SET; + } + final VariableCollectingVisitor visitor = + new VariableCollectingVisitor(); + context.accept(visitor); + return visitor.getUsedVariables(); + } + + private static class VariableCollectingVisitor + extends JavaRecursiveElementVisitor { + + private final Set usedVariables = new HashSet(); + + @Override + public void visitReferenceExpression( + PsiReferenceExpression expression) { + super.visitReferenceExpression(expression); + final PsiElement target = expression.resolve(); + if (!(target instanceof PsiVariable)) { + return; + } + final PsiVariable variable = (PsiVariable)target; + usedVariables.add(variable); + } + + public Set getUsedVariables() { + return usedVariables; + } + } } \ No newline at end of file diff --git a/plugins/IntentionPowerPak/src/com/siyeh/ipp/psiutils/VariableAssignedVisitor.java b/plugins/IntentionPowerPak/src/com/siyeh/ipp/psiutils/VariableAssignedVisitor.java new file mode 100644 index 000000000000..5956e36e2bdf --- /dev/null +++ b/plugins/IntentionPowerPak/src/com/siyeh/ipp/psiutils/VariableAssignedVisitor.java @@ -0,0 +1,196 @@ +/* + * Copyright 2003-2010 Dave Griffith, Bas Leijdekkers + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.siyeh.ipp.psiutils; + +import com.intellij.psi.*; +import com.intellij.psi.tree.IElementType; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +import java.util.Collection; + +class VariableAssignedVisitor extends JavaRecursiveElementVisitor{ + + @NotNull private final Collection variables; + private final boolean recurseIntoClasses; + private boolean assigned = false; + + public VariableAssignedVisitor(@NotNull Collection variables, + boolean recurseIntoClasses){ + this.variables = variables; + this.recurseIntoClasses = recurseIntoClasses; + } + + @Override public void visitElement(@NotNull PsiElement element){ + if(assigned){ + return; + } + super.visitElement(element); + } + + @Override public void visitAssignmentExpression( + @NotNull PsiAssignmentExpression assignment){ + if(assigned){ + return; + } + super.visitAssignmentExpression(assignment); + final PsiExpression lhs = assignment.getLExpression(); + for (PsiVariable variable : variables) { + if(mayEvaluateToVariable(lhs, variable)){ + assigned = true; + } + } + } + + @Override + public void visitClass(PsiClass aClass) { + if(!recurseIntoClasses){ + return; + } + if(assigned){ + return; + } + super.visitClass(aClass); + } + + @Override public void visitPrefixExpression( + @NotNull PsiPrefixExpression prefixExpression){ + if(assigned){ + return; + } + super.visitPrefixExpression(prefixExpression); + final PsiJavaToken operationSign = prefixExpression.getOperationSign(); + final IElementType tokenType = operationSign.getTokenType(); + if(!tokenType.equals(JavaTokenType.PLUSPLUS) && + !tokenType.equals(JavaTokenType.MINUSMINUS)){ + return; + } + final PsiExpression operand = prefixExpression.getOperand(); + for (PsiVariable variable : variables) { + if(mayEvaluateToVariable(operand, variable)){ + assigned = true; + } + } + } + + @Override public void visitPostfixExpression( + @NotNull PsiPostfixExpression postfixExpression){ + if(assigned){ + return; + } + super.visitPostfixExpression(postfixExpression); + final PsiJavaToken operationSign = postfixExpression.getOperationSign(); + final IElementType tokenType = operationSign.getTokenType(); + if(!tokenType.equals(JavaTokenType.PLUSPLUS) && + !tokenType.equals(JavaTokenType.MINUSMINUS)){ + return; + } + final PsiExpression operand = postfixExpression.getOperand(); + for (PsiVariable variable : variables) { + if(mayEvaluateToVariable(operand, variable)){ + assigned = true; + } + } + } + + public static boolean mayEvaluateToVariable( + @Nullable PsiExpression expression, + @NotNull PsiVariable variable) { + if (expression == null){ + return false; + } + if(expression instanceof PsiBinaryExpression) { + final PsiBinaryExpression binaryExpression = + (PsiBinaryExpression)expression; + final PsiExpression lOperand = binaryExpression.getLOperand(); + final PsiExpression rOperand = binaryExpression.getROperand(); + return mayEvaluateToVariable(lOperand, variable) || + mayEvaluateToVariable(rOperand, variable); + } + if(expression instanceof PsiParenthesizedExpression){ + final PsiParenthesizedExpression parenthesizedExpression = + (PsiParenthesizedExpression)expression; + final PsiExpression containedExpression = + parenthesizedExpression.getExpression(); + return mayEvaluateToVariable(containedExpression, variable); + } + if(expression instanceof PsiTypeCastExpression){ + final PsiTypeCastExpression typeCastExpression = + (PsiTypeCastExpression)expression; + final PsiExpression containedExpression = + typeCastExpression.getOperand(); + return mayEvaluateToVariable(containedExpression, variable); + } + if(expression instanceof PsiConditionalExpression){ + final PsiConditionalExpression conditional = + (PsiConditionalExpression) expression; + final PsiExpression thenExpression = conditional.getThenExpression(); + final PsiExpression elseExpression = conditional.getElseExpression(); + return mayEvaluateToVariable(thenExpression, variable) || + mayEvaluateToVariable(elseExpression, variable); + } + if(expression instanceof PsiArrayAccessExpression){ + final PsiElement parent = expression.getParent(); + if (parent instanceof PsiArrayAccessExpression){ + return false; + } + final PsiType type = variable.getType(); + if (!(type instanceof PsiArrayType)) { + return false; + } + final PsiArrayType arrayType = (PsiArrayType)type; + final int dimensions = arrayType.getArrayDimensions(); + if (dimensions <= 1) { + return false; + } + PsiArrayAccessExpression arrayAccessExpression = + (PsiArrayAccessExpression)expression; + PsiExpression arrayExpression = + arrayAccessExpression.getArrayExpression(); + int count = 1; + while (arrayExpression instanceof PsiArrayAccessExpression) { + arrayAccessExpression = + (PsiArrayAccessExpression)arrayExpression; + arrayExpression = arrayAccessExpression.getArrayExpression(); + count++; + } + return count != dimensions && + mayEvaluateToVariable(arrayExpression, variable); + } + return evaluatesToVariable(expression, variable); + } + + public static boolean evaluatesToVariable( + @Nullable PsiExpression expression, + @NotNull PsiVariable variable) { + final PsiExpression strippedExpression = + ParenthesesUtils.stripParentheses(expression); + if(strippedExpression == null){ + return false; + } + if (!(expression instanceof PsiReferenceExpression)) { + return false; + } + final PsiReferenceExpression referenceExpression = + (PsiReferenceExpression) expression; + final PsiElement referent = referenceExpression.resolve(); + return variable.equals(referent); + } + + public boolean isAssigned(){ + return assigned; + } +} \ No newline at end of file diff --git a/plugins/IntentionPowerPak/src/com/siyeh/ipp/trivialif/MergeParallelIfsIntention.java b/plugins/IntentionPowerPak/src/com/siyeh/ipp/trivialif/MergeParallelIfsIntention.java index a08745671c6d..1d97c213a0d0 100644 --- a/plugins/IntentionPowerPak/src/com/siyeh/ipp/trivialif/MergeParallelIfsIntention.java +++ b/plugins/IntentionPowerPak/src/com/siyeh/ipp/trivialif/MergeParallelIfsIntention.java @@ -1,5 +1,5 @@ /* - * Copyright 2003-2006 Dave Griffith, Bas Leijdekkers + * Copyright 2003-2010 Dave Griffith, Bas Leijdekkers * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -25,11 +25,13 @@ import org.jetbrains.annotations.NonNls; public class MergeParallelIfsIntention extends Intention { + @Override @NotNull public PsiElementPredicate getElementPredicate() { return new MergeParallelIfsPredicate(); } + @Override public void processIntention(PsiElement element) throws IncorrectOperationException { final PsiJavaToken token = (PsiJavaToken)element; @@ -57,23 +59,23 @@ public class MergeParallelIfsIntention extends Intention { final PsiStatement firstThenBranch = firstStatement.getThenBranch(); final PsiStatement secondThenBranch = secondStatement.getThenBranch(); @NonNls String statement = "if(" + conditionText + ')' + - printStatementsInSequence(firstThenBranch, - secondThenBranch); + printStatementsInSequence(firstThenBranch, + secondThenBranch); final PsiStatement firstElseBranch = firstStatement.getElseBranch(); final PsiStatement secondElseBranch = secondStatement.getElseBranch(); if (firstElseBranch != null || secondElseBranch != null) { if (firstElseBranch instanceof PsiIfStatement - && secondElseBranch instanceof PsiIfStatement - && MergeParallelIfsPredicate.ifStatementsCanBeMerged( + && secondElseBranch instanceof PsiIfStatement + && MergeParallelIfsPredicate.ifStatementsCanBeMerged( (PsiIfStatement)firstElseBranch, (PsiIfStatement)secondElseBranch)) { statement += "else " + - mergeIfStatements((PsiIfStatement)firstElseBranch, - (PsiIfStatement)secondElseBranch); + mergeIfStatements((PsiIfStatement)firstElseBranch, + (PsiIfStatement)secondElseBranch); } else { statement += "else" + - printStatementsInSequence(firstElseBranch, - secondElseBranch); + printStatementsInSequence(firstElseBranch, + secondElseBranch); } } return statement; diff --git a/plugins/IntentionPowerPak/src/com/siyeh/ipp/trivialif/MergeParallelIfsPredicate.java b/plugins/IntentionPowerPak/src/com/siyeh/ipp/trivialif/MergeParallelIfsPredicate.java index fc69581a1ba9..04fe5fd6cb62 100644 --- a/plugins/IntentionPowerPak/src/com/siyeh/ipp/trivialif/MergeParallelIfsPredicate.java +++ b/plugins/IntentionPowerPak/src/com/siyeh/ipp/trivialif/MergeParallelIfsPredicate.java @@ -1,5 +1,5 @@ /* - * Copyright 2003-2006 Dave Griffith, Bas Leijdekkers + * Copyright 2003-2010 Dave Griffith, Bas Leijdekkers * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -21,7 +21,9 @@ import com.siyeh.ipp.base.PsiElementPredicate; import com.siyeh.ipp.psiutils.ControlFlowUtils; import com.siyeh.ipp.psiutils.EquivalenceChecker; import com.siyeh.ipp.psiutils.ErrorUtil; +import com.siyeh.ipp.psiutils.VariableAccessUtils; +import java.util.Collection; import java.util.HashSet; import java.util.Set; @@ -32,7 +34,6 @@ class MergeParallelIfsPredicate implements PsiElementPredicate{ return false; } final PsiJavaToken token = (PsiJavaToken) element; - final PsiElement parent = token.getParent(); if(!(parent instanceof PsiIfStatement)){ return false; @@ -51,7 +52,19 @@ class MergeParallelIfsPredicate implements PsiElementPredicate{ if(ErrorUtil.containsError(nextIfStatement)){ return false; } - return ifStatementsCanBeMerged(ifStatement, nextIfStatement); + if(!ifStatementsCanBeMerged(ifStatement, nextIfStatement)){ + return false; + } + final PsiExpression condition = ifStatement.getCondition(); + final Set variables = + VariableAccessUtils.collectUsedVariables(condition); + final PsiStatement thenBranch = ifStatement.getThenBranch(); + if(VariableAccessUtils.isAnyVariableAssigned(variables, thenBranch)){ + return false; + } + final PsiStatement elseBranch = ifStatement.getElseBranch(); + return !VariableAccessUtils.isAnyVariableAssigned(variables, + elseBranch); } public static boolean ifStatementsCanBeMerged(PsiIfStatement statement1, @@ -63,8 +76,8 @@ class MergeParallelIfsPredicate implements PsiElementPredicate{ } final PsiExpression firstCondition = statement1.getCondition(); final PsiExpression secondCondition = statement2.getCondition(); - if(! EquivalenceChecker.expressionsAreEquivalent(firstCondition, - secondCondition)){ + if(!EquivalenceChecker.expressionsAreEquivalent(firstCondition, + secondCondition)){ return false; } final PsiStatement nextThenBranch = statement2.getThenBranch(); @@ -73,11 +86,11 @@ class MergeParallelIfsPredicate implements PsiElementPredicate{ } final PsiStatement nextElseBranch = statement2.getElseBranch(); return elseBranch == null || nextElseBranch == null || - canBeMerged(elseBranch, nextElseBranch); + canBeMerged(elseBranch, nextElseBranch); } private static boolean canBeMerged(PsiStatement statement1, - PsiStatement statement2){ + PsiStatement statement2){ if(!ControlFlowUtils.statementMayCompleteNormally(statement1)){ return false; } @@ -89,13 +102,13 @@ class MergeParallelIfsPredicate implements PsiElementPredicate{ final Set statement2Declarations = calculateTopLevelDeclarations(statement2); return !containsConflictingDeclarations(statement2Declarations, - statement1); + statement1); } private static boolean containsConflictingDeclarations( - Set declarations, PsiStatement statement) { + Set declarations, PsiElement context) { final DeclarationVisitor visitor = new DeclarationVisitor(declarations); - statement.accept(visitor); + context.accept(visitor); return visitor.hasConflict(); } @@ -119,18 +132,19 @@ class MergeParallelIfsPredicate implements PsiElementPredicate{ } private static void addDeclarations(PsiDeclarationStatement statement, - Set declaredVars){ + Collection declaredVariables){ final PsiElement[] elements = statement.getDeclaredElements(); for(final PsiElement element : elements){ if(element instanceof PsiVariable){ final PsiVariable variable = (PsiVariable) element; final String name = variable.getName(); - declaredVars.add(name); + declaredVariables.add(name); } } } - private static class DeclarationVisitor extends JavaRecursiveElementWalkingVisitor{ + private static class DeclarationVisitor + extends JavaRecursiveElementWalkingVisitor{ private final Set declarations; private boolean hasConflict = false;