From 8aa473344a9857f4eaa1461488f44e5500662cd3 Mon Sep 17 00:00:00 2001 From: Nariman Abdullin Date: Mon, 23 May 2022 17:30:50 +0300 Subject: [PATCH 1/8] ### What's done: fixed substitution issue in boolean expression rule + preserve order in variables --- .../rules/chapter3/BooleanExpressionsRule.kt | 174 ++++++++++++------ .../chapter3/BooleanExpressionsRuleFixTest.kt | 18 ++ .../BooleanExpressionsRuleWarnTest.kt | 18 +- .../NegativeExpressionExpected.kt | 8 + .../NegativeExpressionTest.kt | 8 + .../boolean_expressions/OrderIssueExpected.kt | 6 + .../boolean_expressions/OrderIssueTest.kt | 6 + .../SubstitutionIssueExpected.kt | 7 + .../SubstitutionIssueTest.kt | 7 + 9 files changed, 185 insertions(+), 67 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..d53971ac07 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 @@ -1,16 +1,9 @@ package org.cqfn.diktat.ruleset.rules.chapter3 -import org.cqfn.diktat.common.config.rules.RulesConfig -import org.cqfn.diktat.ruleset.constants.Warnings.COMPLEX_BOOLEAN_EXPRESSION -import org.cqfn.diktat.ruleset.rules.DiktatRule -import org.cqfn.diktat.ruleset.utils.KotlinParser -import org.cqfn.diktat.ruleset.utils.findAllNodesWithCondition -import org.cqfn.diktat.ruleset.utils.findLeafWithSpecificType -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 @@ -19,13 +12,18 @@ import com.pinterest.ktlint.core.ast.ElementType.PARENTHESIZED import com.pinterest.ktlint.core.ast.ElementType.PREFIX_EXPRESSION import com.pinterest.ktlint.core.ast.isLeaf import com.pinterest.ktlint.core.ast.isPartOfComment +import org.cqfn.diktat.common.config.rules.RulesConfig +import org.cqfn.diktat.ruleset.constants.Warnings.COMPLEX_BOOLEAN_EXPRESSION +import org.cqfn.diktat.ruleset.rules.DiktatRule +import org.cqfn.diktat.ruleset.utils.KotlinParser +import org.cqfn.diktat.ruleset.utils.findAllNodesWithCondition +import org.cqfn.diktat.ruleset.utils.findLeafWithSpecificType +import org.cqfn.diktat.ruleset.utils.logicalInfixMethods 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.psiUtil.parents -import java.lang.RuntimeException - /** * Rule that checks if the boolean expression can be simplified. */ @@ -42,17 +40,18 @@ 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 expressionReplacement = ExpressionReplacement() + val correctedExpression = formatBooleanExpressionAsString(node, expressionReplacement) + if (expressionReplacement.isEmpty()) { // this happens, if we haven't found any expressions that can be simplified return } + val orderTokenMapper = OrderTokenMapper() // If there are method calls in conditions val expr: Expression = try { - ExprParser.parse(correctedExpression) + ExprParser.parse(correctedExpression, orderTokenMapper) } 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,14 +60,14 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( throw exc } } - val distributiveLawString = checkDistributiveLaw(expr, mapOfExpressionToChar, node) + val distributiveLawString = checkDistributiveLaw(expr, expressionReplacement, 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, expressionReplacement, orderTokenMapper) } } } @@ -88,44 +87,35 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( * @return formatted string representation of expression */ @Suppress("UnsafeCallOnNullableType", "ForbiddenComment") - internal fun formatBooleanExpressionAsString(node: ASTNode, mapOfExpressionToChar: HashMap): String { - val (booleanBinaryExpressions, otherBinaryExpressions) = node.collectElementaryExpressions() - val logicalExpressions = otherBinaryExpressions.filter { + internal fun formatBooleanExpressionAsString(node: ASTNode, expressionReplacement: ExpressionReplacement): String { + val isLogicalExpression = { it: ASTNode -> // keeping only boolean expressions, keeping things like `a + b < 6` and excluding `a + b` (it.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 } + it.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 - .map { it.psi as KtBinaryExpression } - .flatMap { listOf(it.left!!.node, it.right!!.node) } - .map { - // remove parentheses around expression, if there are any - (it.psi as? KtParenthesizedExpression)?.expression?.node ?: it - } - .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" + node.collectElementaryExpressions() + .flatMap { + if (it.isBooleanBinaryExpression()) { + it.extractElementaryBooleanExpressions() + } else { + if (isLogicalExpression(it)) listOf(it) else listOf() + } } - 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() + .forEach { expression -> + expressionReplacement.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()) - } + correctedExpression = expressionReplacement.replaceExpressions(correctedExpression) // jBool library is using & as && and | as || - return "(${correctedExpression - .replace("&&", "&") - .replace("||", "|")})" + return "(${ + correctedExpression + .replace("&&", "&") + .replace("||", "|") + })" } /** @@ -138,10 +128,23 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( (astNode == this || astNode.parents().takeWhile { it != this } .all { it.elementType in setOf(BINARY_EXPRESSION, PARENTHESIZED, PREFIX_EXPRESSION) }) } - .partition { - val operationReferenceText = (it.psi as KtBinaryExpression).operationReference.text - operationReferenceText == "&&" || operationReferenceText == "||" - } + + private fun ASTNode.isBooleanBinaryExpression() = setOf("&&", "||") + .contains((this.psi as KtBinaryExpression).operationReference.text) + + // Boolean expressions like `a > 5 && b < 7` or `x.isEmpty() || (y.isNotEmpty())` we convert to individual parts. + private fun ASTNode.extractElementaryBooleanExpressions() = listOf(this) + .map { it.psi as KtBinaryExpression } + .flatMap { listOf(it.left!!.node, it.right!!.node) } + .map { + // remove parentheses around expression, if there are any + (it.psi as? KtParenthesizedExpression)?.expression?.node ?: it + } + .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" + } private fun ASTNode.textWithoutComments() = findAllNodesWithCondition(withSelf = false) { it.isLeaf() @@ -153,7 +156,8 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( private fun fixBooleanExpression( node: ASTNode, simplifiedExpr: Expression, - mapOfExpressionToChar: HashMap + expressionReplacement: ExpressionReplacement, + orderTokenMapper: OrderTokenMapper ) { var correctKotlinBooleanExpression = simplifiedExpr .toString() @@ -165,10 +169,9 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( .drop(1) .dropLast(1) } + correctKotlinBooleanExpression = orderTokenMapper.restoreOrder(correctKotlinBooleanExpression) + correctKotlinBooleanExpression = expressionReplacement.restoreFullExpression(correctKotlinBooleanExpression) - mapOfExpressionToChar.forEach { (key, value) -> - correctKotlinBooleanExpression = correctKotlinBooleanExpression.replace(value.toString(), key) - } node.replaceChild(node.firstChildNode, KotlinParser().createNode(correctKotlinBooleanExpression)) } @@ -179,12 +182,12 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( */ private fun checkDistributiveLaw( expr: Expression, - mapOfExpressionToChar: HashMap, + expressionReplacement: ExpressionReplacement, 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(), expressionReplacement) ?: return null + val correctSymbolsSequence = expressionReplacement.getReplacements().toMutableList() correctSymbolsSequence.remove(commonDistributiveOperand) correctSymbolsSequence.add(0, commonDistributiveOperand) val expressionsLogicalOperator = expr.toString().first { it == '&' || it == '|' } @@ -218,13 +221,13 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( private fun getCommonDistributiveOperand( node: ASTNode, expression: String, - mapOfExpressionToChar: HashMap + expressionReplacement: ExpressionReplacement ): 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 (expressionReplacement.size() < DISTRIBUTIVE_LAW_MIN_EXPRESSIONS || numberOfOperationReferences < DISTRIBUTIVE_LAW_MIN_OPERATIONS || !isSequenceAlternate(operationSequence)) { return null @@ -269,6 +272,61 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( } } + internal inner class ExpressionReplacement { + private val replacements = LinkedHashMap() + + fun isEmpty(): Boolean = replacements.isEmpty() + + fun size(): Int = replacements.size + + fun addExpression(expressionAstNode: ASTNode) { + val expression = expressionAstNode.textWithoutComments() + replacements.computeIfAbsent(expression) { + ('A'.code + replacements.size).toChar() + } + } + + fun replaceExpressions(fullExpression: String): String { + var resultExpression = fullExpression + replacements.forEach { (refExpr, replacement) -> + resultExpression = resultExpression.replace(refExpr, replacement.toString()) + } + return resultExpression + } + + fun restoreFullExpression(fullExpression: String): String { + var resultExpression = fullExpression + replacements.values.forEachIndexed { index, value -> + resultExpression = resultExpression.replace(value.toString(), "%${index + 1}\$s") + } + resultExpression = resultExpression.format(*replacements.keys.toTypedArray()) + return resultExpression + } + + fun getReplacements(): Collection { + return replacements.values + } + } + + // it's Char to Char actually, but will keep it String for simplicity + inner class OrderTokenMapper : TokenMapper { + private val variables = HashMap() + + override fun getVariable(name: String): String { + return variables.computeIfAbsent(name) { + ('A'.code + variables.size).toChar().toString() + } + } + + fun restoreOrder(expression: String): String { + var resultExpression = expression + variables.values.forEachIndexed { index, value -> + resultExpression = resultExpression.replace(value, "%${index + 1}\$s") + } + return resultExpression.format(*variables.keys.toTypedArray()) + } + } + private fun KtBinaryExpression.isXorExpression() = operationReference.text == "xor" companion object { 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..5d667ad9bf 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 @@ -1,13 +1,12 @@ package org.cqfn.diktat.ruleset.chapter3 +import com.pinterest.ktlint.core.LintError +import generated.WarningNames import org.cqfn.diktat.ruleset.constants.Warnings -import org.cqfn.diktat.ruleset.rules.DIKTAT_RULE_SET_ID +//import org.cqfn.diktat.ruleset.rules.DIKTAT_RULE_SET_ID import org.cqfn.diktat.ruleset.rules.chapter3.BooleanExpressionsRule import org.cqfn.diktat.ruleset.utils.KotlinParser import org.cqfn.diktat.util.LintTestBase - -import com.pinterest.ktlint.core.LintError -import generated.WarningNames import org.junit.jupiter.api.Assertions import org.junit.jupiter.api.Tag import org.junit.jupiter.api.Test @@ -15,7 +14,7 @@ import java.io.ByteArrayOutputStream import java.io.PrintStream class BooleanExpressionsRuleWarnTest : LintTestBase(::BooleanExpressionsRule) { - private val ruleId = "$DIKTAT_RULE_SET_ID:${BooleanExpressionsRule.NAME_ID}" + private val ruleId = "diktat-ruleset:${BooleanExpressionsRule.NAME_ID}" @Test @Tag(WarningNames.COMPLEX_BOOLEAN_EXPRESSION) @@ -205,7 +204,7 @@ class BooleanExpressionsRuleWarnTest : LintTestBase(::BooleanExpressionsRule) { fun `test makeCorrectExpressionString method #11 - variable in condition and binary expression`() { checkExpressionFormatter( "::testContainerId.isInitialized || a > 2", - "(B | A)", + "(A | B)", 2 ) } @@ -283,10 +282,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.ExpressionReplacement = rule.ExpressionReplacement() + 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..e60a6bd879 --- /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) { + } +} \ No newline at end of file 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..8c399976ec --- /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())) { + } +} \ No newline at end of file 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..4af5a17172 --- /dev/null +++ b/diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/SubstitutionIssueExpected.kt @@ -0,0 +1,7 @@ +package test.paragraph3.boolean_expressions + +fun foo() { + if (isABC_A && isABC_B && isABC_C) { + + } +} \ No newline at end of file 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..78ff0d3187 --- /dev/null +++ b/diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/SubstitutionIssueTest.kt @@ -0,0 +1,7 @@ +package test.paragraph3.boolean_expressions + +fun foo() { + if (isABC_A && isABC_B && isABC_C && (isEmpty() || !isEmpty())) { + + } +} \ No newline at end of file From df090472b8691b2b27f95eef340c5b15258bba0d Mon Sep 17 00:00:00 2001 From: Nariman Abdullin Date: Tue, 24 May 2022 11:13:10 +0300 Subject: [PATCH 2/8] ### What's done: supported negative expression and additional substitution issue (replacement of sub-word) --- .../rules/chapter3/BooleanExpressionsRule.kt | 175 +++++++++--------- .../BooleanExpressionsRuleWarnTest.kt | 11 +- .../NegativeExpressionExpected.kt | 2 +- .../NegativeExpressionTest.kt | 2 +- .../SubstitutionIssueExpected.kt | 5 +- .../SubstitutionIssueTest.kt | 5 +- 6 files changed, 102 insertions(+), 98 deletions(-) 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 d53971ac07..307e8ad867 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 @@ -1,5 +1,13 @@ package org.cqfn.diktat.ruleset.rules.chapter3 +import org.cqfn.diktat.common.config.rules.RulesConfig +import org.cqfn.diktat.ruleset.constants.Warnings.COMPLEX_BOOLEAN_EXPRESSION +import org.cqfn.diktat.ruleset.rules.DiktatRule +import org.cqfn.diktat.ruleset.utils.KotlinParser +import org.cqfn.diktat.ruleset.utils.findAllNodesWithCondition +import org.cqfn.diktat.ruleset.utils.findLeafWithSpecificType +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 @@ -12,18 +20,14 @@ import com.pinterest.ktlint.core.ast.ElementType.PARENTHESIZED import com.pinterest.ktlint.core.ast.ElementType.PREFIX_EXPRESSION import com.pinterest.ktlint.core.ast.isLeaf import com.pinterest.ktlint.core.ast.isPartOfComment -import org.cqfn.diktat.common.config.rules.RulesConfig -import org.cqfn.diktat.ruleset.constants.Warnings.COMPLEX_BOOLEAN_EXPRESSION -import org.cqfn.diktat.ruleset.rules.DiktatRule -import org.cqfn.diktat.ruleset.utils.KotlinParser -import org.cqfn.diktat.ruleset.utils.findAllNodesWithCondition -import org.cqfn.diktat.ruleset.utils.findLeafWithSpecificType -import org.cqfn.diktat.ruleset.utils.logicalInfixMethods 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 + /** * Rule that checks if the boolean expression can be simplified. */ @@ -41,17 +45,16 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( @Suppress("TooGenericExceptionCaught") private fun checkBooleanExpression(node: ASTNode) { // This class is used to assign a variable name for every elementary boolean expression. It is required for jbool to operate. - val expressionReplacement = ExpressionReplacement() - val correctedExpression = formatBooleanExpressionAsString(node, expressionReplacement) - if (expressionReplacement.isEmpty()) { + 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 } - val orderTokenMapper = OrderTokenMapper() // If there are method calls in conditions val expr: Expression = try { - ExprParser.parse(correctedExpression, orderTokenMapper) + ExprParser.parse(correctedExpression, expressionsReplacement.getTokenMapper()) } 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) @@ -60,21 +63,21 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( throw exc } } - val distributiveLawString = checkDistributiveLaw(expr, expressionReplacement, 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, expressionReplacement, orderTokenMapper) + 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 @@ -83,38 +86,45 @@ 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, expressionReplacement: ExpressionReplacement): String { - val isLogicalExpression = { it: ASTNode -> + internal fun formatBooleanExpressionAsString(node: ASTNode, expressionsReplacement: ExpressionsReplacement): String { + val (booleanBinaryExpression, otherBinaryExpression) = node.collectElementaryExpressions() + val logicalExpressions = otherBinaryExpression.filter { // keeping only boolean expressions, keeping things like `a + b < 6` and excluding `a + b` (it.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 } + it.parents().takeWhile { it != node }.none { (it.psi as? KtBinaryExpression)?.isXorExpression() ?: false } } - node.collectElementaryExpressions() - .flatMap { - if (it.isBooleanBinaryExpression()) { - it.extractElementaryBooleanExpressions() - } else { - if (isLogicalExpression(it)) listOf(it) else listOf() - } + // Boolean expressions like `a > 5 && b < 7` or `x.isEmpty() || (y.isNotEmpty())` we convert to individual parts. + val elementaryBooleanExpressions = booleanBinaryExpression + .map { it.psi as KtBinaryExpression } + .flatMap { listOf(it.left!!.node, it.right!!.node) } + .map { + // remove parentheses around expression, if there are any + it.removeAllParentheses() } - .forEach { expression -> - expressionReplacement.addExpression(expression) + .filterNot { + // finally, if parts are binary expressions themselves, they should be present in our lists and we will process them later. + 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" } + (logicalExpressions + elementaryBooleanExpressions).forEach { expression -> + expressionsReplacement.addExpression(expression) + } // Prepare final formatted string var correctedExpression = node.textWithoutComments() // At first, substitute all elementary expressions with variables - correctedExpression = expressionReplacement.replaceExpressions(correctedExpression) + correctedExpression = expressionsReplacement.replaceExpressions(correctedExpression) // jBool library is using & as && and | as || - return "(${ - correctedExpression - .replace("&&", "&") - .replace("||", "|") + return "(${correctedExpression + .replace("&&", "&") + .replace("||", "|") })" } @@ -128,23 +138,15 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( (astNode == this || astNode.parents().takeWhile { it != this } .all { it.elementType in setOf(BINARY_EXPRESSION, PARENTHESIZED, PREFIX_EXPRESSION) }) } + .partition { + val operationReferenceText = (it.psi as KtBinaryExpression).operationReference.text + operationReferenceText == "&&" || operationReferenceText == "||" + } - private fun ASTNode.isBooleanBinaryExpression() = setOf("&&", "||") - .contains((this.psi as KtBinaryExpression).operationReference.text) - - // Boolean expressions like `a > 5 && b < 7` or `x.isEmpty() || (y.isNotEmpty())` we convert to individual parts. - private fun ASTNode.extractElementaryBooleanExpressions() = listOf(this) - .map { it.psi as KtBinaryExpression } - .flatMap { listOf(it.left!!.node, it.right!!.node) } - .map { - // remove parentheses around expression, if there are any - (it.psi as? KtParenthesizedExpression)?.expression?.node ?: it - } - .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" - } + 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() @@ -156,8 +158,7 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( private fun fixBooleanExpression( node: ASTNode, simplifiedExpr: Expression, - expressionReplacement: ExpressionReplacement, - orderTokenMapper: OrderTokenMapper + expressionsReplacement: ExpressionsReplacement ) { var correctKotlinBooleanExpression = simplifiedExpr .toString() @@ -169,8 +170,7 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( .drop(1) .dropLast(1) } - correctKotlinBooleanExpression = orderTokenMapper.restoreOrder(correctKotlinBooleanExpression) - correctKotlinBooleanExpression = expressionReplacement.restoreFullExpression(correctKotlinBooleanExpression) + correctKotlinBooleanExpression = expressionsReplacement.restoreFullExpression(correctKotlinBooleanExpression) node.replaceChild(node.firstChildNode, KotlinParser().createNode(correctKotlinBooleanExpression)) } @@ -182,12 +182,12 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( */ private fun checkDistributiveLaw( expr: Expression, - expressionReplacement: ExpressionReplacement, + expressionsReplacement: ExpressionsReplacement, node: ASTNode ): String? { // checking that expression can be considered as distributive law - val commonDistributiveOperand = getCommonDistributiveOperand(node, expr.toString(), expressionReplacement) ?: return null - val correctSymbolsSequence = expressionReplacement.getReplacements().toMutableList() + val commonDistributiveOperand = getCommonDistributiveOperand(node, expr.toString(), expressionsReplacement)?.toString() ?: return null + val correctSymbolsSequence = expressionsReplacement.getReplacements().toMutableList() correctSymbolsSequence.remove(commonDistributiveOperand) correctSymbolsSequence.add(0, commonDistributiveOperand) val expressionsLogicalOperator = expr.toString().first { it == '&' || it == '|' } @@ -198,7 +198,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 -> @@ -221,13 +221,13 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( private fun getCommonDistributiveOperand( node: ASTNode, expression: String, - expressionReplacement: ExpressionReplacement + 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 (expressionReplacement.size() < DISTRIBUTIVE_LAW_MIN_EXPRESSIONS || + if (expressionsReplacement.size() < DISTRIBUTIVE_LAW_MIN_EXPRESSIONS || numberOfOperationReferences < DISTRIBUTIVE_LAW_MIN_OPERATIONS || !isSequenceAlternate(operationSequence)) { return null @@ -272,58 +272,59 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( } } - internal inner class ExpressionReplacement { - private val replacements = LinkedHashMap() + + // it's String to Char(to Char) actually, but will keep it String for simplicity + internal inner class ExpressionsReplacement { + private val replacements = LinkedHashMap() + private val variables = HashMap() + private val orderTokenMapper = TokenMapper { name -> getLetter(variables, name) } fun isEmpty(): Boolean = replacements.isEmpty() fun size(): Int = replacements.size + fun getTokenMapper(): TokenMapper = orderTokenMapper + fun addExpression(expressionAstNode: ASTNode) { - val expression = expressionAstNode.textWithoutComments() - replacements.computeIfAbsent(expression) { - ('A'.code + replacements.size).toChar() - } + val expressionText = expressionAstNode.textWithoutComments() + // support case when `boolean_expression` matches to `!boolean_expression` + val expression = if (expressionText.startsWith('!')) expressionText.substring(1) else expressionText + getLetter(replacements, expression) } fun replaceExpressions(fullExpression: String): String { var resultExpression = fullExpression - replacements.forEach { (refExpr, replacement) -> - resultExpression = resultExpression.replace(refExpr, replacement.toString()) - } + replacements.keys + .sortedByDescending { it.length } + .forEach { refExpr -> + resultExpression = resultExpression.replace(refExpr, replacements[refExpr]!!) + } return resultExpression } fun restoreFullExpression(fullExpression: String): String { + // restore order var resultExpression = fullExpression + variables.values.forEachIndexed { index, value -> + resultExpression = resultExpression.replace(value, "%${index + 1}\$s") + } + resultExpression = resultExpression.format(*variables.keys.toTypedArray()) + // restore expression replacements.values.forEachIndexed { index, value -> - resultExpression = resultExpression.replace(value.toString(), "%${index + 1}\$s") + resultExpression = resultExpression.replace(value, "%${index + 1}\$s") } resultExpression = resultExpression.format(*replacements.keys.toTypedArray()) return resultExpression } - fun getReplacements(): Collection { + fun getReplacements(): Collection { return replacements.values } - } - // it's Char to Char actually, but will keep it String for simplicity - inner class OrderTokenMapper : TokenMapper { - private val variables = HashMap() - - override fun getVariable(name: String): String { - return variables.computeIfAbsent(name) { - ('A'.code + variables.size).toChar().toString() - } - } - - fun restoreOrder(expression: String): String { - var resultExpression = expression - variables.values.forEachIndexed { index, value -> - resultExpression = resultExpression.replace(value, "%${index + 1}\$s") + private fun getLetter(letters: HashMap, key: String): String { + return letters.computeIfAbsent(key) { + ('A'.code + letters.size).toChar().toString() } - return resultExpression.format(*variables.keys.toTypedArray()) } } 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 5d667ad9bf..451b2f2bbe 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 @@ -1,11 +1,12 @@ package org.cqfn.diktat.ruleset.chapter3 -import com.pinterest.ktlint.core.LintError -import generated.WarningNames import org.cqfn.diktat.ruleset.constants.Warnings -//import org.cqfn.diktat.ruleset.rules.DIKTAT_RULE_SET_ID +import org.cqfn.diktat.ruleset.rules.DIKTAT_RULE_SET_ID import org.cqfn.diktat.ruleset.rules.chapter3.BooleanExpressionsRule import org.cqfn.diktat.ruleset.utils.KotlinParser + +import com.pinterest.ktlint.core.LintError +import generated.WarningNames import org.cqfn.diktat.util.LintTestBase import org.junit.jupiter.api.Assertions import org.junit.jupiter.api.Tag @@ -14,7 +15,7 @@ import java.io.ByteArrayOutputStream import java.io.PrintStream class BooleanExpressionsRuleWarnTest : LintTestBase(::BooleanExpressionsRule) { - private val ruleId = "diktat-ruleset:${BooleanExpressionsRule.NAME_ID}" + private val ruleId = "$DIKTAT_RULE_SET_ID:${BooleanExpressionsRule.NAME_ID}" @Test @Tag(WarningNames.COMPLEX_BOOLEAN_EXPRESSION) @@ -283,7 +284,7 @@ class BooleanExpressionsRuleWarnTest : LintTestBase(::BooleanExpressionsRule) { System.setErr(PrintStream(stream)) val node = KotlinParser().createNode(expression) val rule = BooleanExpressionsRule(emptyList()) - val map: BooleanExpressionsRule.ExpressionReplacement = rule.ExpressionReplacement() + val map: BooleanExpressionsRule.ExpressionsReplacement = rule.ExpressionsReplacement() val result = rule.formatBooleanExpressionAsString(node, map) Assertions.assertEquals(expectedRepresentation, result) Assertions.assertEquals(expectedMapSize, map.size()) 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 index e60a6bd879..7adfb8c272 100644 --- a/diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/NegativeExpressionExpected.kt +++ b/diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/NegativeExpressionExpected.kt @@ -5,4 +5,4 @@ fun foo() { } if (bar) { } -} \ No newline at end of file +} 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 index 8c399976ec..1c978aad7f 100644 --- a/diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/NegativeExpressionTest.kt +++ b/diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/NegativeExpressionTest.kt @@ -5,4 +5,4 @@ fun foo() { } if (bar && (isEmpty() || !isEmpty())) { } -} \ No newline at end of file +} 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 index 4af5a17172..97b6b78355 100644 --- a/diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/SubstitutionIssueExpected.kt +++ b/diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/SubstitutionIssueExpected.kt @@ -2,6 +2,7 @@ package test.paragraph3.boolean_expressions fun foo() { if (isABC_A && isABC_B && isABC_C) { - } -} \ No newline at end of file + 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 index 78ff0d3187..71051b7326 100644 --- a/diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/SubstitutionIssueTest.kt +++ b/diktat-rules/src/test/resources/test/paragraph3/boolean_expressions/SubstitutionIssueTest.kt @@ -2,6 +2,7 @@ package test.paragraph3.boolean_expressions fun foo() { if (isABC_A && isABC_B && isABC_C && (isEmpty() || !isEmpty())) { - } -} \ No newline at end of file + if (isAdd && isAdded() && (isEmpty() || !isEmpty())) { + } +} From a4af748d2e709221e32ee6f475668a0138fe414d Mon Sep 17 00:00:00 2001 From: Nariman Abdullin Date: Tue, 24 May 2022 11:19:16 +0300 Subject: [PATCH 3/8] ### What's done: fixed small typos --- .../ruleset/rules/chapter3/BooleanExpressionsRule.kt | 9 ++++----- .../ruleset/chapter3/BooleanExpressionsRuleWarnTest.kt | 2 +- 2 files changed, 5 insertions(+), 6 deletions(-) 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 307e8ad867..f817c70fe8 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 @@ -91,15 +91,15 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( */ @Suppress("UnsafeCallOnNullableType", "ForbiddenComment") internal fun formatBooleanExpressionAsString(node: ASTNode, expressionsReplacement: ExpressionsReplacement): String { - val (booleanBinaryExpression, otherBinaryExpression) = node.collectElementaryExpressions() - val logicalExpressions = otherBinaryExpression.filter { + val (booleanBinaryExpressions, otherBinaryExpressions) = node.collectElementaryExpressions() + val logicalExpressions = otherBinaryExpressions.filter { // keeping only boolean expressions, keeping things like `a + b < 6` and excluding `a + b` (it.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 } } // Boolean expressions like `a > 5 && b < 7` or `x.isEmpty() || (y.isNotEmpty())` we convert to individual parts. - val elementaryBooleanExpressions = booleanBinaryExpression + val elementaryBooleanExpressions = booleanBinaryExpressions .map { it.psi as KtBinaryExpression } .flatMap { listOf(it.left!!.node, it.right!!.node) } .map { @@ -124,8 +124,7 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( // jBool library is using & as && and | as || return "(${correctedExpression .replace("&&", "&") - .replace("||", "|") - })" + .replace("||", "|")})" } /** 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 451b2f2bbe..d81274cf40 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 @@ -4,10 +4,10 @@ import org.cqfn.diktat.ruleset.constants.Warnings import org.cqfn.diktat.ruleset.rules.DIKTAT_RULE_SET_ID import org.cqfn.diktat.ruleset.rules.chapter3.BooleanExpressionsRule import org.cqfn.diktat.ruleset.utils.KotlinParser +import org.cqfn.diktat.util.LintTestBase import com.pinterest.ktlint.core.LintError import generated.WarningNames -import org.cqfn.diktat.util.LintTestBase import org.junit.jupiter.api.Assertions import org.junit.jupiter.api.Tag import org.junit.jupiter.api.Test From 5d4f8e358580f61ac275dc1e88c5adc07cc907d2 Mon Sep 17 00:00:00 2001 From: Nariman Abdullin Date: Tue, 24 May 2022 17:15:23 +0300 Subject: [PATCH 4/8] ### What's done: fixed test + issues found by static code analysis --- .../rules/chapter3/BooleanExpressionsRule.kt | 100 ++++++++++++------ .../BooleanExpressionsRuleWarnTest.kt | 6 +- 2 files changed, 73 insertions(+), 33 deletions(-) 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 f817c70fe8..b73ef94daf 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 @@ -108,19 +108,18 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( } .filterNot { // finally, if parts are binary expressions themselves, they should be present in our lists and we will process them later. - 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" + 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" } (logicalExpressions + elementaryBooleanExpressions).forEach { expression -> expressionsReplacement.addExpression(expression) } // Prepare final formatted string - var correctedExpression = node.textWithoutComments() // At first, substitute all elementary expressions with variables - correctedExpression = expressionsReplacement.replaceExpressions(correctedExpression) + val correctedExpression = expressionsReplacement.replaceExpressions(node.textWithoutComments()) // jBool library is using & as && and | as || return "(${correctedExpression .replace("&&", "&") @@ -186,7 +185,7 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( ): String? { // checking that expression can be considered as distributive law val commonDistributiveOperand = getCommonDistributiveOperand(node, expr.toString(), expressionsReplacement)?.toString() ?: return null - val correctSymbolsSequence = expressionsReplacement.getReplacements().toMutableList() + val correctSymbolsSequence = expressionsReplacement.getTokens().toMutableList() correctSymbolsSequence.remove(commonDistributiveOperand) correctSymbolsSequence.add(0, commonDistributiveOperand) val expressionsLogicalOperator = expr.toString().first { it == '&' || it == '|' } @@ -271,64 +270,105 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( } } + private fun KtBinaryExpression.isXorExpression() = operationReference.text == "xor" - // it's String to Char(to Char) actually, but will keep it String for simplicity - internal inner class ExpressionsReplacement { - private val replacements = LinkedHashMap() - private val variables = HashMap() - private val orderTokenMapper = TokenMapper { name -> getLetter(variables, name) } + /** + * 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 + */ + private inner class ExpressionsReplacement { + private val expressionToToken: HashMap = LinkedHashMap() + private val tokenToOrderedToken: HashMap = HashMap() + private val orderedTokenMapper: TokenMapper = TokenMapper { name -> getLetter(tokenToOrderedToken, name) } - fun isEmpty(): Boolean = replacements.isEmpty() + /** + * Returns true if this object contains no replacements. + * + * @return true if this object contains no replacements + */ + fun isEmpty(): Boolean = expressionToToken.isEmpty() - fun size(): Int = replacements.size + /** + * Returns the number of replacements in this object. + * + * @return the number of replacements in this object + */ + fun size(): Int = expressionToToken.size - fun getTokenMapper(): TokenMapper = orderTokenMapper + /** + * Returns the TokenMapper for first call ExprParser which remembers the order of expression. + * + * @return the TokenMapper for first call ExprParser which remembers the order of expression + */ + fun getTokenMapper(): TokenMapper = orderedTokenMapper + /** + * 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(replacements, expression) + 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 - replacements.keys + expressionToToken.keys .sortedByDescending { it.length } .forEach { refExpr -> - resultExpression = resultExpression.replace(refExpr, replacements[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 - variables.values.forEachIndexed { index, value -> + tokenToOrderedToken.values.forEachIndexed { index, value -> resultExpression = resultExpression.replace(value, "%${index + 1}\$s") } - resultExpression = resultExpression.format(*variables.keys.toTypedArray()) + resultExpression = resultExpression.format(args = tokenToOrderedToken.keys.toTypedArray()) // restore expression - replacements.values.forEachIndexed { index, value -> + expressionToToken.values.forEachIndexed { index, value -> resultExpression = resultExpression.replace(value, "%${index + 1}\$s") } - resultExpression = resultExpression.format(*replacements.keys.toTypedArray()) + resultExpression = resultExpression.format(args = expressionToToken.keys.toTypedArray()) return resultExpression } - fun getReplacements(): Collection { - return replacements.values + /** + * 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 { + return expressionToToken.values } - private fun getLetter(letters: HashMap, key: String): String { - return letters.computeIfAbsent(key) { + private fun getLetter(letters: HashMap, key: String) = letters + .computeIfAbsent(key) { ('A'.code + letters.size).toChar().toString() } - } } - private fun KtBinaryExpression.isXorExpression() = operationReference.text == "xor" - 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/BooleanExpressionsRuleWarnTest.kt b/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter3/BooleanExpressionsRuleWarnTest.kt index d81274cf40..9f9dd66197 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 @@ -205,7 +205,7 @@ class BooleanExpressionsRuleWarnTest : LintTestBase(::BooleanExpressionsRule) { fun `test makeCorrectExpressionString method #11 - variable in condition and binary expression`() { checkExpressionFormatter( "::testContainerId.isInitialized || a > 2", - "(A | B)", + "(B | A)", 2 ) } @@ -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 }) {} @@ -284,7 +284,7 @@ class BooleanExpressionsRuleWarnTest : LintTestBase(::BooleanExpressionsRule) { System.setErr(PrintStream(stream)) val node = KotlinParser().createNode(expression) val rule = BooleanExpressionsRule(emptyList()) - val map: BooleanExpressionsRule.ExpressionsReplacement = rule.ExpressionsReplacement() + val map: BooleanExpressionsRule.ExpressionsReplacementImpl = rule.ExpressionsReplacementImpl() val result = rule.formatBooleanExpressionAsString(node, map) Assertions.assertEquals(expectedRepresentation, result) Assertions.assertEquals(expectedMapSize, map.size()) From 305924f038861edab4fca4948b6c20747f196b83 Mon Sep 17 00:00:00 2001 From: Nariman Abdullin Date: Wed, 25 May 2022 08:59:47 +0300 Subject: [PATCH 5/8] ### What's done: fixed compile issue --- .../diktat/ruleset/rules/chapter3/BooleanExpressionsRule.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 b73ef94daf..bc578844e2 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 @@ -276,7 +276,7 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( * 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 */ - private inner class ExpressionsReplacement { + internal inner class ExpressionsReplacement { private val expressionToToken: HashMap = LinkedHashMap() private val tokenToOrderedToken: HashMap = HashMap() private val orderedTokenMapper: TokenMapper = TokenMapper { name -> getLetter(tokenToOrderedToken, name) } From 95fdee9599bbf690e3848d783b6816f90cb94b51 Mon Sep 17 00:00:00 2001 From: Nariman Abdullin Date: Wed, 25 May 2022 09:33:52 +0300 Subject: [PATCH 6/8] ### What's done: fixed compile issue + diktat findings --- .../rules/chapter3/BooleanExpressionsRule.kt | 35 +++++++++---------- .../BooleanExpressionsRuleWarnTest.kt | 2 +- 2 files changed, 17 insertions(+), 20 deletions(-) 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 bc578844e2..1e99bf3236 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 @@ -92,11 +92,13 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( @Suppress("UnsafeCallOnNullableType", "ForbiddenComment") 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 @@ -110,7 +112,10 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( // finally, if parts are binary expressions themselves, they should be present in our lists and we will process them later. 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 || + (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" } @@ -158,19 +163,15 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( simplifiedExpr: Expression, 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) - } - correctKotlinBooleanExpression = expressionsReplacement.restoreFullExpression(correctKotlinBooleanExpression) - - node.replaceChild(node.firstChildNode, KotlinParser().createNode(correctKotlinBooleanExpression)) + node.replaceChild(node.firstChildNode, + KotlinParser().createNode(expressionsReplacement.restoreFullExpression(correctKotlinBooleanExpression))) } /** @@ -278,7 +279,7 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( */ internal inner class ExpressionsReplacement { private val expressionToToken: HashMap = LinkedHashMap() - private val tokenToOrderedToken: HashMap = HashMap() + private val tokenToOrderedToken: HashMap = HashMap() private val orderedTokenMapper: TokenMapper = TokenMapper { name -> getLetter(tokenToOrderedToken, name) } /** @@ -318,7 +319,6 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( * Replaces registered expressions in provided expression * * @param fullExpression full boolean expression in kotlin - * * @return full expression in jbool format */ @Suppress("UnsafeCallOnNullableType") @@ -336,7 +336,6 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( * 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 { @@ -359,9 +358,7 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( * * @return collection of token are used to construct full expression in jbool format */ - fun getTokens(): Collection { - return expressionToToken.values - } + fun getTokens(): Collection = expressionToToken.values private fun getLetter(letters: HashMap, key: String) = letters .computeIfAbsent(key) { 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 9f9dd66197..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 @@ -284,7 +284,7 @@ class BooleanExpressionsRuleWarnTest : LintTestBase(::BooleanExpressionsRule) { System.setErr(PrintStream(stream)) val node = KotlinParser().createNode(expression) val rule = BooleanExpressionsRule(emptyList()) - val map: BooleanExpressionsRule.ExpressionsReplacementImpl = rule.ExpressionsReplacementImpl() + val map: BooleanExpressionsRule.ExpressionsReplacement = rule.ExpressionsReplacement() val result = rule.formatBooleanExpressionAsString(node, map) Assertions.assertEquals(expectedRepresentation, result) Assertions.assertEquals(expectedMapSize, map.size()) From a82c8bbc02f60e564c56b24c3192cb761c01429d Mon Sep 17 00:00:00 2001 From: Nariman Abdullin Date: Wed, 25 May 2022 09:38:29 +0300 Subject: [PATCH 7/8] ### What's done: diktat finding with tabs\whitespaces --- .../rules/chapter3/BooleanExpressionsRule.kt | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) 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 1e99bf3236..bdad7a0ead 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 @@ -111,13 +111,13 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( .filterNot { // finally, if parts are binary expressions themselves, they should be present in our lists and we will process them later. 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" + // !(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" } (logicalExpressions + elementaryBooleanExpressions).forEach { expression -> expressionsReplacement.addExpression(expression) From f753cd9bed3d03d25561b1874043865477e3b563 Mon Sep 17 00:00:00 2001 From: Nariman Abdullin Date: Wed, 25 May 2022 13:03:46 +0300 Subject: [PATCH 8/8] fixed review note ### What's done: * removed tokenMapper function and opened the variable orderedTokenMapper --- .../rules/chapter3/BooleanExpressionsRule.kt | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) 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 bdad7a0ead..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 @@ -54,7 +54,7 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( // If there are method calls in conditions val expr: Expression = try { - ExprParser.parse(correctedExpression, expressionsReplacement.getTokenMapper()) + 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) @@ -280,7 +280,11 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( internal inner class ExpressionsReplacement { private val expressionToToken: HashMap = LinkedHashMap() private val tokenToOrderedToken: HashMap = HashMap() - private val orderedTokenMapper: TokenMapper = TokenMapper { name -> getLetter(tokenToOrderedToken, name) } + + /** + * 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. @@ -296,13 +300,6 @@ class BooleanExpressionsRule(configRules: List) : DiktatRule( */ fun size(): Int = expressionToToken.size - /** - * Returns the TokenMapper for first call ExprParser which remembers the order of expression. - * - * @return the TokenMapper for first call ExprParser which remembers the order of expression - */ - fun getTokenMapper(): TokenMapper = orderedTokenMapper - /** * Register an expression for further replacement *