From 4656b80ae5bb948ed4f9e1f33c07855b21810a7a Mon Sep 17 00:00:00 2001 From: Nariman Abdullin Date: Wed, 25 May 2022 13:52:20 +0300 Subject: [PATCH] BooleanExpressionRule fixes (#1295) ### What's done: * fixed substitution issue in boolean expression rule + preserve order in variables * supported negative expression and additional substitution issue (replacement of sub-word) Co-authored-by: Andrey Kuleshov --- .../rules/chapter3/BooleanExpressionsRule.kt | 178 +++++++++++++----- .../chapter3/BooleanExpressionsRuleFixTest.kt | 18 ++ .../BooleanExpressionsRuleWarnTest.kt | 9 +- .../NegativeExpressionExpected.kt | 8 + .../NegativeExpressionTest.kt | 8 + .../boolean_expressions/OrderIssueExpected.kt | 6 + .../boolean_expressions/OrderIssueTest.kt | 6 + .../SubstitutionIssueExpected.kt | 8 + .../SubstitutionIssueTest.kt | 8 + 9 files changed, 202 insertions(+), 47 deletions(-) create mode 100644 diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/NegativeExpressionExpected.kt create mode 100644 diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/NegativeExpressionTest.kt create mode 100644 diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/OrderIssueExpected.kt create mode 100644 diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/OrderIssueTest.kt create mode 100644 diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/SubstitutionIssueExpected.kt create mode 100644 diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/SubstitutionIssueTest.kt diff --git a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter3/BooleanExpressionsRule.kt b/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter3/BooleanExpressionsRule.kt index fb548ba6b0..8e2aec9276 100644 --- a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter3/BooleanExpressionsRule.kt +++ b/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter3/BooleanExpressionsRule.kt @@ -11,6 +11,7 @@ import org.cqfn.diktat.ruleset.utils.logicalInfixMethods import com.bpodgursky.jbool_expressions.Expression import com.bpodgursky.jbool_expressions.options.ExprOptions import com.bpodgursky.jbool_expressions.parsers.ExprParser +import com.bpodgursky.jbool_expressions.parsers.TokenMapper import com.bpodgursky.jbool_expressions.rules.RulesHelper import com.pinterest.ktlint.core.ast.ElementType.BINARY_EXPRESSION import com.pinterest.ktlint.core.ast.ElementType.CONDITION @@ -22,6 +23,7 @@ import com.pinterest.ktlint.core.ast.isPartOfComment import org.jetbrains.kotlin.com.intellij.lang.ASTNode import org.jetbrains.kotlin.psi.KtBinaryExpression import org.jetbrains.kotlin.psi.KtParenthesizedExpression +import org.jetbrains.kotlin.psi.KtPrefixExpression import org.jetbrains.kotlin.psi.psiUtil.parents import java.lang.RuntimeException @@ -42,17 +44,17 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( @Suppress("TooGenericExceptionCaught") private fun checkBooleanExpression(node: ASTNode) { - // This map is used to assign a variable name for every elementary boolean expression. It is required for jbool to operate. - val mapOfExpressionToChar: HashMap = HashMap() - val correctedExpression = formatBooleanExpressionAsString(node, mapOfExpressionToChar) - if (mapOfExpressionToChar.isEmpty()) { + // This class is used to assign a variable name for every elementary boolean expression. It is required for jbool to operate. + val expressionsReplacement = ExpressionsReplacement() + val correctedExpression = formatBooleanExpressionAsString(node, expressionsReplacement) + if (expressionsReplacement.isEmpty()) { // this happens, if we haven't found any expressions that can be simplified return } // If there are method calls in conditions val expr: Expression = try { - ExprParser.parse(correctedExpression) + ExprParser.parse(correctedExpression, expressionsReplacement.orderedTokenMapper) } catch (exc: RuntimeException) { if (exc.message?.startsWith("Unrecognized!") == true) { // this comes up if there is an unparsable expression (jbool doesn't have own exception type). For example a.and(b) @@ -61,21 +63,21 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( throw exc } } - val distributiveLawString = checkDistributiveLaw(expr, mapOfExpressionToChar, node) + val distributiveLawString = checkDistributiveLaw(expr, expressionsReplacement, node) val simplifiedExpression = distributiveLawString?.let { ExprParser.parse(distributiveLawString) } ?: RulesHelper.applySet(expr, RulesHelper.demorganRules(), ExprOptions.noCaching()) if (expr != simplifiedExpression) { COMPLEX_BOOLEAN_EXPRESSION.warnAndFix(configRules, emitWarn, isFixMode, node.text, node.startOffset, node) { - fixBooleanExpression(node, simplifiedExpression, mapOfExpressionToChar) + fixBooleanExpression(node, simplifiedExpression, expressionsReplacement) } } } /** * Converts a complex boolean expression into a string representation, mapping each elementary expression to a letter token. - * These tokens are collected into [mapOfExpressionToChar]. + * These tokens are collected into [expressionsReplacement]. * For example: * ``` * (a > 5 && b != 2) -> A & B @@ -84,17 +86,19 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( * ``` * * @param node - * @param mapOfExpressionToChar a mutable map for expression->token + * @param expressionsReplacement a special class for replacements expression->token * @return formatted string representation of expression */ @Suppress("UnsafeCallOnNullableType", "ForbiddenComment") - internal fun formatBooleanExpressionAsString(node: ASTNode, mapOfExpressionToChar: HashMap): String { + internal fun formatBooleanExpressionAsString(node: ASTNode, expressionsReplacement: ExpressionsReplacement): String { val (booleanBinaryExpressions, otherBinaryExpressions) = node.collectElementaryExpressions() - val logicalExpressions = otherBinaryExpressions.filter { + val logicalExpressions = otherBinaryExpressions.filter { otherBinaryExpression -> // keeping only boolean expressions, keeping things like `a + b < 6` and excluding `a + b` - (it.psi as KtBinaryExpression).operationReference.text in logicalInfixMethods && + (otherBinaryExpression.psi as KtBinaryExpression).operationReference.text in logicalInfixMethods && // todo: support xor; for now skip all expressions that are nested in xor - it.parents().takeWhile { it != node }.none { (it.psi as? KtBinaryExpression)?.isXorExpression() ?: false } + otherBinaryExpression.parents() + .takeWhile { it != node } + .none { (it.psi as? KtBinaryExpression)?.isXorExpression() ?: false } } // Boolean expressions like `a > 5 && b < 7` or `x.isEmpty() || (y.isNotEmpty())` we convert to individual parts. val elementaryBooleanExpressions = booleanBinaryExpressions @@ -102,26 +106,25 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( .flatMap { listOf(it.left!!.node, it.right!!.node) } .map { // remove parentheses around expression, if there are any - (it.psi as? KtParenthesizedExpression)?.expression?.node ?: it + it.removeAllParentheses() } .filterNot { // finally, if parts are binary expressions themselves, they should be present in our lists and we will process them later. - // `true` and `false` are valid tokens for jBool, so we keep them. - it.elementType == BINARY_EXPRESSION || it.text == "true" || it.text == "false" + it.elementType == BINARY_EXPRESSION || + // !(a || b) should be skipped too, `a` and `b` should be present later + (it.psi as? KtPrefixExpression)?.lastChild + ?.node + ?.removeAllParentheses() + ?.elementType == BINARY_EXPRESSION || + // `true` and `false` are valid tokens for jBool, so we keep them. + it.text == "true" || it.text == "false" } - var characterAsciiCode = 'A'.code // `A` character in ASCII (logicalExpressions + elementaryBooleanExpressions).forEach { expression -> - mapOfExpressionToChar.computeIfAbsent(expression.textWithoutComments()) { - // Every elementary expression is assigned a single-letter variable. - characterAsciiCode++.toChar() - } + expressionsReplacement.addExpression(expression) } // Prepare final formatted string - var correctedExpression = node.textWithoutComments() // At first, substitute all elementary expressions with variables - mapOfExpressionToChar.forEach { (refExpr, char) -> - correctedExpression = correctedExpression.replace(refExpr, char.toString()) - } + val correctedExpression = expressionsReplacement.replaceExpressions(node.textWithoutComments()) // jBool library is using & as && and | as || return "(${correctedExpression .replace("&&", "&") @@ -143,6 +146,11 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( operationReferenceText == "&&" || operationReferenceText == "||" } + private fun ASTNode.removeAllParentheses(): ASTNode { + val result = (this.psi as? KtParenthesizedExpression)?.expression?.node ?: return this + return result.removeAllParentheses() + } + private fun ASTNode.textWithoutComments() = findAllNodesWithCondition(withSelf = false) { it.isLeaf() } @@ -153,23 +161,17 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( private fun fixBooleanExpression( node: ASTNode, simplifiedExpr: Expression, - mapOfExpressionToChar: HashMap + expressionsReplacement: ExpressionsReplacement ) { - var correctKotlinBooleanExpression = simplifiedExpr + val correctKotlinBooleanExpression = simplifiedExpr .toString() .replace("&", "&&") .replace("|", "||") + .removePrefix("(") + .removeSuffix(")") - if (simplifiedExpr.toString().first() == '(' && simplifiedExpr.toString().last() == ')') { - correctKotlinBooleanExpression = correctKotlinBooleanExpression - .drop(1) - .dropLast(1) - } - - mapOfExpressionToChar.forEach { (key, value) -> - correctKotlinBooleanExpression = correctKotlinBooleanExpression.replace(value.toString(), key) - } - node.replaceChild(node.firstChildNode, KotlinParser().createNode(correctKotlinBooleanExpression)) + node.replaceChild(node.firstChildNode, + KotlinParser().createNode(expressionsReplacement.restoreFullExpression(correctKotlinBooleanExpression))) } /** @@ -179,12 +181,12 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( */ private fun checkDistributiveLaw( expr: Expression, - mapOfExpressionToChar: HashMap, + expressionsReplacement: ExpressionsReplacement, node: ASTNode ): String? { // checking that expression can be considered as distributive law - val commonDistributiveOperand = getCommonDistributiveOperand(node, expr.toString(), mapOfExpressionToChar) ?: return null - val correctSymbolsSequence = mapOfExpressionToChar.values.toMutableList() + val commonDistributiveOperand = getCommonDistributiveOperand(node, expr.toString(), expressionsReplacement)?.toString() ?: return null + val correctSymbolsSequence = expressionsReplacement.getTokens().toMutableList() correctSymbolsSequence.remove(commonDistributiveOperand) correctSymbolsSequence.add(0, commonDistributiveOperand) val expressionsLogicalOperator = expr.toString().first { it == '&' || it == '|' } @@ -195,7 +197,7 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( /** * Returns correct result string in distributive law */ - private fun returnNeededDistributiveExpression(firstLogicalOperator: Char, symbols: List): String { + private fun returnNeededDistributiveExpression(firstLogicalOperator: Char, symbols: List): String { val secondSymbol = if (firstLogicalOperator == '&') '|' else '&' // this is used to alter symbols val resultString = StringBuilder() symbols.forEachIndexed { index, symbol -> @@ -218,13 +220,13 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( private fun getCommonDistributiveOperand( node: ASTNode, expression: String, - mapOfExpressionToChar: HashMap + expressionsReplacement: ExpressionsReplacement ): Char? { val operationSequence = expression.filter { it == '&' || it == '|' } val numberOfOperationReferences = operationSequence.length // There should be three operands and three operation references in order to consider the expression // Moreover the operation references between operands should alternate. - if (mapOfExpressionToChar.size < DISTRIBUTIVE_LAW_MIN_EXPRESSIONS || + if (expressionsReplacement.size() < DISTRIBUTIVE_LAW_MIN_EXPRESSIONS || numberOfOperationReferences < DISTRIBUTIVE_LAW_MIN_OPERATIONS || !isSequenceAlternate(operationSequence)) { return null @@ -271,6 +273,96 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( private fun KtBinaryExpression.isXorExpression() = operationReference.text == "xor" + /** + * A special class to replace expressions (and restore it back) + * Note: mapping is String to Char(and Char to Char) actually, but will keep it as String for simplicity + */ + internal inner class ExpressionsReplacement { + private val expressionToToken: HashMap = LinkedHashMap() + private val tokenToOrderedToken: HashMap = HashMap() + + /** + * TokenMapper for first call ExprParser which remembers the order of expression. + */ + val orderedTokenMapper: TokenMapper = TokenMapper { name -> getLetter(tokenToOrderedToken, name) } + + /** + * Returns true if this object contains no replacements. + * + * @return true if this object contains no replacements + */ + fun isEmpty(): Boolean = expressionToToken.isEmpty() + + /** + * Returns the number of replacements in this object. + * + * @return the number of replacements in this object + */ + fun size(): Int = expressionToToken.size + + /** + * Register an expression for further replacement + * + * @param expressionAstNode astNode which contains boolean expression + */ + fun addExpression(expressionAstNode: ASTNode) { + val expressionText = expressionAstNode.textWithoutComments() + // support case when `boolean_expression` matches to `!boolean_expression` + val expression = if (expressionText.startsWith('!')) expressionText.substring(1) else expressionText + getLetter(expressionToToken, expression) + } + + /** + * Replaces registered expressions in provided expression + * + * @param fullExpression full boolean expression in kotlin + * @return full expression in jbool format + */ + @Suppress("UnsafeCallOnNullableType") + fun replaceExpressions(fullExpression: String): String { + var resultExpression = fullExpression + expressionToToken.keys + .sortedByDescending { it.length } + .forEach { refExpr -> + resultExpression = resultExpression.replace(refExpr, expressionToToken[refExpr]!!) + } + return resultExpression + } + + /** + * Restores full expression by replacing tokens and restoring the order + * + * @param fullExpression full boolean expression in jbool format + * @return full boolean expression in kotlin + */ + fun restoreFullExpression(fullExpression: String): String { + // restore order + var resultExpression = fullExpression + tokenToOrderedToken.values.forEachIndexed { index, value -> + resultExpression = resultExpression.replace(value, "%${index + 1}\$s") + } + resultExpression = resultExpression.format(args = tokenToOrderedToken.keys.toTypedArray()) + // restore expression + expressionToToken.values.forEachIndexed { index, value -> + resultExpression = resultExpression.replace(value, "%${index + 1}\$s") + } + resultExpression = resultExpression.format(args = expressionToToken.keys.toTypedArray()) + return resultExpression + } + + /** + * Returns collection of token are used to construct full expression in jbool format. + * + * @return collection of token are used to construct full expression in jbool format + */ + fun getTokens(): Collection = expressionToToken.values + + private fun getLetter(letters: HashMap, key: String) = letters + .computeIfAbsent(key) { + ('A'.code + letters.size).toChar().toString() + } + } + companion object { const val DISTRIBUTIVE_LAW_MIN_EXPRESSIONS = 3 const val DISTRIBUTIVE_LAW_MIN_OPERATIONS = 3 diff --git a/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter3/BooleanExpressionsRuleFixTest.kt b/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter3/BooleanExpressionsRuleFixTest.kt index b7f65478b1..f5f4d0722b 100644 --- a/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter3/BooleanExpressionsRuleFixTest.kt +++ b/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter3/BooleanExpressionsRuleFixTest.kt @@ -25,4 +25,22 @@ class BooleanExpressionsRuleFixTest : FixTestBase("test/paragraph3/boolean_expre fun `check same expressions`() { fixAndCompare("SameExpressionsInConditionExpected.kt", "SameExpressionsInConditionTest.kt") } + + @Test + @Tag(WarningNames.COMPLEX_BOOLEAN_EXPRESSION) + fun `check substitution works properly`() { + fixAndCompare("SubstitutionIssueExpected.kt", "SubstitutionIssueTest.kt") + } + + @Test + @Tag(WarningNames.COMPLEX_BOOLEAN_EXPRESSION) + fun `check ordering is persisted`() { + fixAndCompare("OrderIssueExpected.kt", "OrderIssueTest.kt") + } + + @Test + @Tag(WarningNames.COMPLEX_BOOLEAN_EXPRESSION) + fun `check handling of negative expression`() { + fixAndCompare("NegativeExpressionExpected.kt", "NegativeExpressionTest.kt") + } } diff --git a/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter3/BooleanExpressionsRuleWarnTest.kt b/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter3/BooleanExpressionsRuleWarnTest.kt index be89cb539b..380c25171b 100644 --- a/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter3/BooleanExpressionsRuleWarnTest.kt +++ b/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter3/BooleanExpressionsRuleWarnTest.kt @@ -234,7 +234,7 @@ class BooleanExpressionsRuleWarnTest : LintTestBase(::BooleanExpressionsRule) { // nested boolean expressions in lambdas if (currentProperty.nextSibling { it.elementType == PROPERTY } == nextProperty) {} - if (!(rightSide == null || leftSide == null) && + if (rightSide != null && leftSide != null && rightSide.size == leftSide.size && rightSide.zip(leftSide).all { (first, second) -> first.text == second.text }) {} @@ -283,10 +283,11 @@ class BooleanExpressionsRuleWarnTest : LintTestBase(::BooleanExpressionsRule) { val stream = ByteArrayOutputStream() System.setErr(PrintStream(stream)) val node = KotlinParser().createNode(expression) - val map: java.util.HashMap = HashMap() - val result = BooleanExpressionsRule(emptyList()).formatBooleanExpressionAsString(node, map) + val rule = BooleanExpressionsRule(emptyList()) + val map: BooleanExpressionsRule.ExpressionsReplacement = rule.ExpressionsReplacement() + val result = rule.formatBooleanExpressionAsString(node, map) Assertions.assertEquals(expectedRepresentation, result) - Assertions.assertEquals(expectedMapSize, map.size) + Assertions.assertEquals(expectedMapSize, map.size()) System.setErr(System.err) val stderr = stream.toString() Assertions.assertTrue(stderr.isEmpty()) { diff --git a/diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/NegativeExpressionExpected.kt b/diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/NegativeExpressionExpected.kt new file mode 100644 index 0000000000..7adfb8c272 --- /dev/null +++ b/diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/NegativeExpressionExpected.kt @@ -0,0 +1,8 @@ +package test.paragraph3.boolean_expressions + +fun foo() { + if (bar) { + } + if (bar) { + } +} diff --git a/diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/NegativeExpressionTest.kt b/diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/NegativeExpressionTest.kt new file mode 100644 index 0000000000..1c978aad7f --- /dev/null +++ b/diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/NegativeExpressionTest.kt @@ -0,0 +1,8 @@ +package test.paragraph3.boolean_expressions + +fun foo() { + if (bar && (!isEmpty() || isEmpty())) { + } + if (bar && (isEmpty() || !isEmpty())) { + } +} diff --git a/diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/OrderIssueExpected.kt b/diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/OrderIssueExpected.kt new file mode 100644 index 0000000000..19bf50ba34 --- /dev/null +++ b/diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/OrderIssueExpected.kt @@ -0,0 +1,6 @@ +package test.paragraph3.boolean_expressions + +fun foo() { + if (isB && isA && isC) { + } +} diff --git a/diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/OrderIssueTest.kt b/diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/OrderIssueTest.kt new file mode 100644 index 0000000000..0fa146a93b --- /dev/null +++ b/diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/OrderIssueTest.kt @@ -0,0 +1,6 @@ +package test.paragraph3.boolean_expressions + +fun foo() { + if (isB && isA && isC && (isEmpty() || !isEmpty())) { + } +} diff --git a/diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/SubstitutionIssueExpected.kt b/diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/SubstitutionIssueExpected.kt new file mode 100644 index 0000000000..97b6b78355 --- /dev/null +++ b/diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/SubstitutionIssueExpected.kt @@ -0,0 +1,8 @@ +package test.paragraph3.boolean_expressions + +fun foo() { + if (isABC_A && isABC_B && isABC_C) { + } + if (isAdd && isAdded()) { + } +} diff --git a/diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/SubstitutionIssueTest.kt b/diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/SubstitutionIssueTest.kt new file mode 100644 index 0000000000..71051b7326 --- /dev/null +++ b/diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/SubstitutionIssueTest.kt @@ -0,0 +1,8 @@ +package test.paragraph3.boolean_expressions + +fun foo() { + if (isABC_A && isABC_B && isABC_C && (isEmpty() || !isEmpty())) { + } + if (isAdd && isAdded() && (isEmpty() || !isEmpty())) { + } +}