Boolean simplification inspections improved

IDEA-192236 Support opposite expression pairs like `x == null` and `x != null` in boolean expression simplifiers
IDEA-192161 Support "a && b || a" case in "Simplifiable boolean expression" inspection
This commit is contained in:
Tagir Valeev
2018-05-18 16:42:46 +07:00
parent 9fc0350746
commit 4ee504d004
10 changed files with 240 additions and 83 deletions
@@ -28,7 +28,10 @@ import com.siyeh.ig.BaseInspection;
import com.siyeh.ig.BaseInspectionVisitor;
import com.siyeh.ig.InspectionGadgetsFix;
import com.siyeh.ig.PsiReplacementUtil;
import com.siyeh.ig.psiutils.*;
import com.siyeh.ig.psiutils.BoolUtils;
import com.siyeh.ig.psiutils.CommentTracker;
import com.siyeh.ig.psiutils.ParenthesesUtils;
import com.siyeh.ig.psiutils.SideEffectChecker;
import org.jetbrains.annotations.Nls;
import org.jetbrains.annotations.NotNull;
@@ -133,17 +136,7 @@ public class BooleanExpressionMayBeConditionalInspection extends BaseInspection
}
final PsiExpression expression1 = ParenthesesUtils.stripParentheses(lBinaryExpression.getLOperand());
final PsiExpression expression2 = ParenthesesUtils.stripParentheses(rBinaryExpression.getLOperand());
if (expression1 == null || expression2 == null ||
ParenthesesUtils.stripParentheses(lBinaryExpression.getROperand()) == null ||
ParenthesesUtils.stripParentheses(rBinaryExpression.getROperand()) == null) {
return;
}
if (EquivalenceChecker.getCanonicalPsiEquivalence().expressionsAreEquivalent(BoolUtils.getNegated(expression1), expression2) &&
!SideEffectChecker.mayHaveSideEffects(expression2)) {
registerError(expression);
}
else if (EquivalenceChecker.getCanonicalPsiEquivalence().expressionsAreEquivalent(expression1, BoolUtils.getNegated(expression2)) &&
!SideEffectChecker.mayHaveSideEffects(expression1)) {
if (BoolUtils.areExpressionsOpposite(expression1, expression2) && !SideEffectChecker.mayHaveSideEffects(expression1)) {
registerError(expression);
}
}
@@ -19,21 +19,22 @@ import com.intellij.codeInspection.CleanupLocalInspectionTool;
import com.intellij.codeInspection.ProblemDescriptor;
import com.intellij.openapi.project.Project;
import com.intellij.psi.*;
import com.intellij.psi.tree.IElementType;
import com.intellij.psi.util.PsiUtil;
import com.siyeh.InspectionGadgetsBundle;
import com.siyeh.ig.BaseInspection;
import com.siyeh.ig.BaseInspectionVisitor;
import com.siyeh.ig.InspectionGadgetsFix;
import com.siyeh.ig.PsiReplacementUtil;
import com.siyeh.ig.psiutils.BoolUtils;
import com.siyeh.ig.psiutils.CommentTracker;
import com.siyeh.ig.psiutils.EquivalenceChecker;
import com.siyeh.ig.psiutils.ParenthesesUtils;
import com.siyeh.ig.psiutils.*;
import org.jetbrains.annotations.Nls;
import org.jetbrains.annotations.NonNls;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import java.util.Arrays;
import static com.intellij.util.ObjectUtils.tryCast;
/**
* @author Bas Leijdekkers
*/
@@ -118,22 +119,42 @@ public class SimplifiableBooleanExpressionInspection extends BaseInspection impl
@NonNls
static String calculateReplacementExpression(PsiBinaryExpression expression, CommentTracker commentTracker) {
final PsiExpression rhs1 = ParenthesesUtils.stripParentheses(expression.getROperand());
if (rhs1 == null) {
return null;
}
final PsiExpression lhs = ParenthesesUtils.stripParentheses(expression.getLOperand());
if (!(lhs instanceof PsiBinaryExpression)) {
return null;
}
final PsiBinaryExpression binaryExpression = (PsiBinaryExpression)lhs;
final PsiExpression rhs2 = binaryExpression.getROperand();
if (rhs2 == null) {
return null;
}
return ParenthesesUtils.getText(commentTracker.markUnchanged(rhs1), ParenthesesUtils.OR_PRECEDENCE) + "||" +
ParenthesesUtils.getText(commentTracker.markUnchanged(rhs2), ParenthesesUtils.OR_PRECEDENCE);
PsiPolyadicExpression conjunction =
tryCast(PsiUtil.skipParenthesizedExprDown(expression.getLOperand()), PsiPolyadicExpression.class);
if (conjunction == null) return null;
final PsiExpression rightDisjunct = ParenthesesUtils.stripParentheses(expression.getROperand());
if (rightDisjunct == null) return null;
if (hasOperand(conjunction, rightDisjunct)) return commentTracker.text(rightDisjunct);
PsiExpression[] operands = conjunction.getOperands();
boolean isFirst;
if (BoolUtils.areExpressionsOpposite(operands[0], rightDisjunct)) {
isFirst = true;
}
else if (BoolUtils.areExpressionsOpposite(operands[operands.length - 1], rightDisjunct)) {
isFirst = false;
}
else {
return null;
}
String conjunctionRemnant;
if (operands.length == 2) {
conjunctionRemnant = commentTracker.text(operands[isFirst ? 1 : 0], ParenthesesUtils.OR_PRECEDENCE);
}
else {
if (isFirst) {
conjunctionRemnant = commentTracker.rangeText(operands[1], operands[operands.length - 1]);
}
else {
conjunctionRemnant = commentTracker.rangeText(operands[0], operands[operands.length - 2]);
}
if (expression.getLOperand() instanceof PsiParenthesizedExpression) {
conjunctionRemnant = "(" + conjunctionRemnant + ")";
}
}
return isFirst ?
commentTracker.text(rightDisjunct, ParenthesesUtils.OR_PRECEDENCE) + "||" + conjunctionRemnant :
conjunctionRemnant + "||" + commentTracker.text(rightDisjunct, ParenthesesUtils.OR_PRECEDENCE);
}
@Override
@@ -146,21 +167,13 @@ public class SimplifiableBooleanExpressionInspection extends BaseInspection impl
@Override
public void visitPrefixExpression(PsiPrefixExpression expression) {
super.visitPrefixExpression(expression);
final IElementType tokenType = expression.getOperationTokenType();
if (!JavaTokenType.EXCL.equals(tokenType)) {
return;
}
final PsiExpression operand = ParenthesesUtils.stripParentheses(expression.getOperand());
if (!(operand instanceof PsiBinaryExpression)) {
return;
}
final PsiBinaryExpression binaryExpression = (PsiBinaryExpression)operand;
final IElementType binaryTokenType = binaryExpression.getOperationTokenType();
if (!JavaTokenType.XOR.equals(binaryTokenType)) {
return;
}
final PsiExpression lhs = ParenthesesUtils.stripParentheses(binaryExpression.getLOperand());
final PsiExpression rhs = ParenthesesUtils.stripParentheses(binaryExpression.getROperand());
if (!JavaTokenType.EXCL.equals(expression.getOperationTokenType())) return;
PsiBinaryExpression maybeXor = tryCast(PsiUtil.skipParenthesizedExprDown(expression.getOperand()), PsiBinaryExpression.class);
if (maybeXor == null || !JavaTokenType.XOR.equals(maybeXor.getOperationTokenType())) return;
final PsiExpression lhs = ParenthesesUtils.stripParentheses(maybeXor.getLOperand());
final PsiExpression rhs = ParenthesesUtils.stripParentheses(maybeXor.getROperand());
if (lhs == null || rhs == null) {
return;
}
@@ -168,28 +181,29 @@ public class SimplifiableBooleanExpressionInspection extends BaseInspection impl
}
@Override
public void visitBinaryExpression(PsiBinaryExpression expression) {
super.visitBinaryExpression(expression);
final IElementType tokenType1 = expression.getOperationTokenType();
if (!JavaTokenType.OROR.equals(tokenType1)) {
return;
public void visitBinaryExpression(PsiBinaryExpression disjunction) {
super.visitBinaryExpression(disjunction);
if (!JavaTokenType.OROR.equals(disjunction.getOperationTokenType())) return;
PsiPolyadicExpression conjunction =
tryCast(PsiUtil.skipParenthesizedExprDown(disjunction.getLOperand()), PsiPolyadicExpression.class);
if (conjunction == null || !JavaTokenType.ANDAND.equals(conjunction.getOperationTokenType())) return;
final PsiExpression rightDisjunct = ParenthesesUtils.stripParentheses(disjunction.getROperand());
if (hasOperand(conjunction, rightDisjunct) && !SideEffectChecker.mayHaveSideEffects(conjunction)) {
registerError(disjunction, disjunction);
}
final PsiExpression lhs1 = ParenthesesUtils.stripParentheses(expression.getLOperand());
if (!(lhs1 instanceof PsiBinaryExpression)) {
return;
PsiExpression[] operands = conjunction.getOperands();
if ((BoolUtils.areExpressionsOpposite(operands[0], rightDisjunct) ||
BoolUtils.areExpressionsOpposite(operands[operands.length - 1], rightDisjunct)) &&
!SideEffectChecker.mayHaveSideEffects(rightDisjunct)) {
registerError(disjunction, disjunction);
}
final PsiBinaryExpression binaryExpression = (PsiBinaryExpression)lhs1;
final IElementType tokenType2 = binaryExpression.getOperationTokenType();
if (!JavaTokenType.ANDAND.equals(tokenType2)) {
return;
}
final PsiExpression lhs2 = ParenthesesUtils.stripParentheses(binaryExpression.getLOperand());
final PsiExpression rhs1 = ParenthesesUtils.stripParentheses(expression.getROperand());
final PsiExpression negated = BoolUtils.getNegated(rhs1);
if (!EquivalenceChecker.getCanonicalPsiEquivalence().expressionsAreEquivalent(lhs2, negated)) {
return;
}
registerError(expression, expression);
}
}
private static boolean hasOperand(PsiPolyadicExpression polyadic, PsiExpression operand) {
if (operand == null) return false;
EquivalenceChecker equivalence = EquivalenceChecker.getCanonicalPsiEquivalence();
return Arrays.stream(polyadic.getOperands()).anyMatch(op -> equivalence.expressionsAreEquivalent(op, operand));
}
}
@@ -21,6 +21,7 @@ import com.intellij.openapi.project.Project;
import com.intellij.psi.PsiConditionalExpression;
import com.intellij.psi.PsiExpression;
import com.intellij.psi.PsiType;
import com.intellij.psi.util.PsiPrecedenceUtil;
import com.siyeh.InspectionGadgetsBundle;
import com.siyeh.ig.BaseInspection;
import com.siyeh.ig.BaseInspectionVisitor;
@@ -28,7 +29,6 @@ import com.siyeh.ig.InspectionGadgetsFix;
import com.siyeh.ig.PsiReplacementUtil;
import com.siyeh.ig.psiutils.BoolUtils;
import com.siyeh.ig.psiutils.CommentTracker;
import com.siyeh.ig.psiutils.EquivalenceChecker;
import com.siyeh.ig.psiutils.ParenthesesUtils;
import org.jetbrains.annotations.NonNls;
import org.jetbrains.annotations.NotNull;
@@ -88,13 +88,14 @@ public class SimplifiableConditionalExpressionInspection extends BaseInspection
final PsiExpression condition = expression.getCondition();
assert thenExpression != null;
assert elseExpression != null;
if (EquivalenceChecker.getCanonicalPsiEquivalence().expressionsAreEquivalent(BoolUtils.getNegated(thenExpression), elseExpression)) {
return ParenthesesUtils.getText(tracker.markUnchanged(condition), ParenthesesUtils.EQUALITY_PRECEDENCE) + " != " +
BoolUtils.getNegatedExpressionText(thenExpression, ParenthesesUtils.EQUALITY_PRECEDENCE, tracker);
}
else if (EquivalenceChecker.getCanonicalPsiEquivalence().expressionsAreEquivalent(thenExpression, BoolUtils.getNegated(elseExpression))) {
return ParenthesesUtils.getText(tracker.markUnchanged(condition), ParenthesesUtils.EQUALITY_PRECEDENCE) + " == " +
ParenthesesUtils.getText(tracker.markUnchanged(thenExpression), ParenthesesUtils.EQUALITY_PRECEDENCE);
if (BoolUtils.areExpressionsOpposite(thenExpression, elseExpression)) {
if (BoolUtils.isNegation(thenExpression)) {
return ParenthesesUtils.getText(tracker.markUnchanged(condition), PsiPrecedenceUtil.RELATIONAL_PRECEDENCE) + " != " +
ParenthesesUtils.getText(tracker.markUnchanged(elseExpression), PsiPrecedenceUtil.RELATIONAL_PRECEDENCE);
} else {
return ParenthesesUtils.getText(tracker.markUnchanged(condition), PsiPrecedenceUtil.RELATIONAL_PRECEDENCE) + " == " +
ParenthesesUtils.getText(tracker.markUnchanged(thenExpression), PsiPrecedenceUtil.RELATIONAL_PRECEDENCE);
}
}
if (BoolUtils.isTrue(thenExpression)) {
final String elseExpressionText = ParenthesesUtils.getText(tracker.markUnchanged(elseExpression), ParenthesesUtils.OR_PRECEDENCE);
@@ -137,15 +138,9 @@ public class SimplifiableConditionalExpressionInspection extends BaseInspection
}
final boolean thenConstant = BoolUtils.isFalse(thenExpression) || BoolUtils.isTrue(thenExpression);
final boolean elseConstant = BoolUtils.isFalse(elseExpression) || BoolUtils.isTrue(elseExpression);
if (thenConstant == elseConstant) {
if (EquivalenceChecker.getCanonicalPsiEquivalence()
.expressionsAreEquivalent(BoolUtils.getNegated(thenExpression), elseExpression) ||
EquivalenceChecker.getCanonicalPsiEquivalence().expressionsAreEquivalent(thenExpression, BoolUtils.getNegated(elseExpression))) {
registerError(expression, expression);
}
return;
if (thenConstant != elseConstant || BoolUtils.areExpressionsOpposite(thenExpression, elseExpression)) {
registerError(expression, expression);
}
registerError(expression, expression);
}
}
}
@@ -15,9 +15,11 @@
*/
package com.siyeh.ig.psiutils;
import com.intellij.codeInspection.dataFlow.value.DfaRelationValue;
import com.intellij.openapi.util.text.StringUtil;
import com.intellij.psi.*;
import com.intellij.psi.tree.IElementType;
import com.intellij.psi.util.PsiUtil;
import com.intellij.util.Function;
import org.jetbrains.annotations.Contract;
import org.jetbrains.annotations.NonNls;
@@ -203,4 +205,44 @@ public class BoolUtils {
}
return PsiKeyword.FALSE.equals(expression.getText());
}
/**
* Checks whether two supplied boolean expressions are opposite to each other (e.g. "a == null" and "a != null")
*
* @param expression1 first expression
* @param expression2 second expression
* @return true if it's determined that the expressions are opposite to each other.
*/
@Contract(value = "null, _ -> false; _, null -> false", pure = true)
public static boolean areExpressionsOpposite(@Nullable PsiExpression expression1, @Nullable PsiExpression expression2) {
expression1 = PsiUtil.skipParenthesizedExprDown(expression1);
expression2 = PsiUtil.skipParenthesizedExprDown(expression2);
if (expression1 == null || expression2 == null) return false;
EquivalenceChecker equivalence = EquivalenceChecker.getCanonicalPsiEquivalence();
if (isNegation(expression1)) {
return equivalence.expressionsAreEquivalent(getNegated(expression1), expression2);
}
if (isNegation(expression2)) {
return equivalence.expressionsAreEquivalent(getNegated(expression2), expression1);
}
if (expression1 instanceof PsiBinaryExpression && expression2 instanceof PsiBinaryExpression) {
PsiBinaryExpression binOp1 = (PsiBinaryExpression)expression1;
PsiBinaryExpression binOp2 = (PsiBinaryExpression)expression2;
DfaRelationValue.RelationType rel1 = DfaRelationValue.RelationType.fromElementType(binOp1.getOperationTokenType());
DfaRelationValue.RelationType rel2 = DfaRelationValue.RelationType.fromElementType(binOp2.getOperationTokenType());
if (rel1 == null || rel2 == null) return false;
PsiType type = binOp1.getLOperand().getType();
// a > b and a <= b are not strictly opposite due to NaN semantics
if (type == null || type.equals(PsiType.FLOAT) || type.equals(PsiType.DOUBLE)) return false;
if (rel1 == rel2.getNegated()) {
return equivalence.expressionsAreEquivalent(binOp1.getLOperand(), binOp2.getLOperand()) &&
equivalence.expressionsAreEquivalent(binOp1.getROperand(), binOp2.getROperand());
}
if (rel1.getFlipped() == rel2.getNegated()) {
return equivalence.expressionsAreEquivalent(binOp1.getLOperand(), binOp2.getROperand()) &&
equivalence.expressionsAreEquivalent(binOp1.getROperand(), binOp2.getLOperand());
}
}
return false;
}
}
@@ -79,6 +79,39 @@ public class CommentTracker {
return element;
}
/**
* Marks the range of elements as unchanged and returns their text. The unchanged elements are assumed to be preserved
* in the resulting code as is, so the comments from them will not be extracted.
*
* @param firstElement first element to mark
* @param lastElement last element to mark (must be equal to firstElement or its sibling)
* @return a text to be inserted into refactored code
* @throws IllegalArgumentException if firstElement and lastElements are not siblings or firstElement goes after last element
*/
public String rangeText(@NotNull PsiElement firstElement, @NotNull PsiElement lastElement) {
checkState();
PsiElement e;
StringBuilder result = new StringBuilder();
for (e = firstElement; e != null && e != lastElement; e = e.getNextSibling()) {
addIgnored(e);
result.append(e.getText());
}
if (e == null) {
throw new IllegalArgumentException("Elements must be siblings: " + firstElement + " and " + lastElement);
}
addIgnored(lastElement);
result.append(lastElement.getText());
return result.toString();
}
/**
* Marks the range of elements as unchanged. The unchanged elements are assumed to be preserved
* in the resulting code as is, so the comments from them will not be extracted.
*
* @param firstElement first element to mark
* @param lastElement last element to mark (must be equal to firstElement or its sibling)
* @throws IllegalArgumentException if firstElement and lastElements are not siblings or firstElement goes after last element
*/
public void markRangeUnchanged(@NotNull PsiElement firstElement, @NotNull PsiElement lastElement) {
checkState();
PsiElement e;
@@ -0,0 +1,5 @@
class D {
void f(int x, int y, boolean b) {
final boolean sss = b == (x > y);
}
}
@@ -0,0 +1,5 @@
class D {
void f(int x, int y, boolean b) {
final boolean sss = b ? <caret>x > y : x <= y;
}
}
@@ -36,4 +36,8 @@ public class SimplifiableConditionalExpressionFixTest extends IGQuickFixesTestCa
public void testInverted() {
doTest();
}
public void testBinary() {
doTest();
}
}
@@ -75,6 +75,20 @@ public class BooleanExpressionMayBeConditionalFixTest extends IGQuickFixesTestCa
"}");
}
public void testComparison() {
doTest(InspectionGadgetsBundle.message("if.may.be.conditional.quickfix"),
"class X {\n" +
" boolean test(int x, int y, int z) {\n" +
" return (x > y && z == 1) || /**/(x <= y && z == 2);\n" +
" }\n" +
"}",
"class X {\n" +
" boolean test(int x, int y, int z) {\n" +
" return x > y ? z == 1 : z == 2;\n" +
" }\n" +
"}");
}
@Override
protected BaseInspection getInspection() {
return new BooleanExpressionMayBeConditionalInspection();
@@ -45,6 +45,58 @@ public class SimplifiableBooleanExpressionFixTest extends IGQuickFixesTestCase {
"}");
}
public void testAndOrExpression3() {
doMemberTest(InspectionGadgetsBundle.message("constant.conditional.expression.simplify.quickfix"),
"boolean fff(boolean a, boolean b, boolean c) {" +
" return a && b && c/**/|| !a;" +
"}",
"boolean fff(boolean a, boolean b, boolean c) {" +
" return !a || b && c;" +
"}");
}
public void testAndOrExpression3Middle() {
// While this particular case could be safely transformed to "a && c || !b", the order of execution is changed which may
// affect dereferencing (e.g. "a != null && b != null && a.foo(b.bar()) || b == null") is safe, but replacement is not.
// Proper replacement would be "(a || !b) && (!b || c)", but it's not shorter than the original code
assertQuickfixNotAvailable(InspectionGadgetsBundle.message("constant.conditional.expression.simplify.quickfix"),
"class X {\n" +
" boolean fff(boolean a, boolean b, boolean c) { \n" +
" return a && b && c/**/ || !b;\n" +
" }\n" +
"}");
}
public void testAndOrExpression3Parentheses() {
doMemberTest(InspectionGadgetsBundle.message("constant.conditional.expression.simplify.quickfix"),
"boolean fff(boolean a, boolean b, boolean c) {" +
" return (a && b && !c)/**/|| c;" +
"}",
"boolean fff(boolean a, boolean b, boolean c) {" +
" return (a && b) || c;" +
"}");
}
public void testAndOrExpressionComparisons() {
doMemberTest(InspectionGadgetsBundle.message("constant.conditional.expression.simplify.quickfix"),
"boolean fff(int a, int b, int c) {" +
" return a > b && b > c /**/|| a <= b;" +
"}",
"boolean fff(int a, int b, int c) {" +
" return a <= b || b > c;" +
"}");
}
public void testAndOrNonNegated() {
doMemberTest(InspectionGadgetsBundle.message("constant.conditional.expression.simplify.quickfix"),
"boolean fff(int a, int b, int c) {" +
" return a > b && b > c && b > 0 /**/|| (b > c);" +
"}",
"boolean fff(int a, int b, int c) {" +
" return b > c;" +
"}");
}
@Override
protected BaseInspection getInspection() {
return new SimplifiableBooleanExpressionInspection();