BooleanExpressionRule fixes

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<RulesConfig>) : 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<String, Char> = 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<String> = try {
-            ExprParser.parse(correctedExpression)
+            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)
@@ -61,21 +63,21 @@ class BooleanExpressionsRule(configRules: List<RulesConfig>) : 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,11 +86,11 @@ class BooleanExpressionsRule(configRules: List<RulesConfig>) : 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, Char>): String {
+    internal fun formatBooleanExpressionAsString(node: ASTNode, expressionsReplacement: ExpressionsReplacement): String {
         val (booleanBinaryExpressions, otherBinaryExpressions) = node.collectElementaryExpressions()
         val logicalExpressions = otherBinaryExpressions.filter {
             // keeping only boolean expressions, keeping things like `a + b < 6` and excluding `a + b`
@@ -102,26 +104,23 @@ class BooleanExpressionsRule(configRules: List<RulesConfig>) : 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())
-        }
+        correctedExpression = expressionsReplacement.replaceExpressions(correctedExpression)
         // jBool library is using & as && and | as ||
         return "(${correctedExpression
             .replace("&&", "&")
@@ -143,6 +142,11 @@ class BooleanExpressionsRule(configRules: List<RulesConfig>) : 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,7 +157,7 @@ class BooleanExpressionsRule(configRules: List<RulesConfig>) : DiktatRule(
     private fun fixBooleanExpression(
         node: ASTNode,
         simplifiedExpr: Expression<String>,
-        mapOfExpressionToChar: HashMap<String, Char>
+        expressionsReplacement: ExpressionsReplacement
     ) {
         var correctKotlinBooleanExpression = simplifiedExpr
             .toString()
@@ -165,10 +169,8 @@ class BooleanExpressionsRule(configRules: List<RulesConfig>) : DiktatRule(
                 .drop(1)
                 .dropLast(1)
         }
+        correctKotlinBooleanExpression = expressionsReplacement.restoreFullExpression(correctKotlinBooleanExpression)
 
-        mapOfExpressionToChar.forEach { (key, value) ->
-            correctKotlinBooleanExpression = correctKotlinBooleanExpression.replace(value.toString(), key)
-        }
         node.replaceChild(node.firstChildNode, KotlinParser().createNode(correctKotlinBooleanExpression))
     }
 
@@ -179,12 +181,12 @@ class BooleanExpressionsRule(configRules: List<RulesConfig>) : DiktatRule(
      */
     private fun checkDistributiveLaw(
         expr: Expression<String>,
-        mapOfExpressionToChar: HashMap<String, Char>,
+        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.getReplacements().toMutableList()
         correctSymbolsSequence.remove(commonDistributiveOperand)
         correctSymbolsSequence.add(0, commonDistributiveOperand)
         val expressionsLogicalOperator = expr.toString().first { it == '&' || it == '|' }
@@ -195,7 +197,7 @@ class BooleanExpressionsRule(configRules: List<RulesConfig>) : DiktatRule(
     /**
      * Returns correct result string in distributive law
      */
-    private fun returnNeededDistributiveExpression(firstLogicalOperator: Char, symbols: List<Char>): String {
+    private fun returnNeededDistributiveExpression(firstLogicalOperator: Char, symbols: List<String>): 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<RulesConfig>) : DiktatRule(
     private fun getCommonDistributiveOperand(
         node: ASTNode,
         expression: String,
-        mapOfExpressionToChar: HashMap<String, Char>
+        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
@@ -269,6 +271,62 @@ class BooleanExpressionsRule(configRules: List<RulesConfig>) : DiktatRule(
         }
     }
 
+
+    // it's String to Char(to Char) actually, but will keep it String for simplicity
+    internal inner class ExpressionsReplacement {
+        private val replacements = LinkedHashMap<String, String>()
+        private val variables = HashMap<String, String>()
+        private val orderTokenMapper = TokenMapper { name -> getLetter(variables, name) }
+
+        fun isEmpty(): Boolean = replacements.isEmpty()
+
+        fun size(): Int = replacements.size
+
+        fun getTokenMapper(): TokenMapper<String> = orderTokenMapper
+
+        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)
+        }
+
+        fun replaceExpressions(fullExpression: String): String {
+            var resultExpression = fullExpression
+            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, "%${index + 1}\$s")
+            }
+            resultExpression = resultExpression.format(*replacements.keys.toTypedArray())
+            return resultExpression
+        }
+
+        fun getReplacements(): Collection<String> {
+            return replacements.values
+        }
+
+        private fun getLetter(letters: HashMap<String, String>, key: String): String {
+            return letters.computeIfAbsent(key) {
+                ('A'.code + letters.size).toChar().toString()
+            }
+        }
+    }
+
     private fun KtBinaryExpression.isXorExpression() = operationReference.text == "xor"
 
     companion object {

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")
+    }
 }

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",
-            "(B | A)",
+            "(A | B)",
             2
         )
     }
@@ -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<String, Char> = 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()) {

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) {
+    }
+}

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())) {
+    }
+}

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) {
+    }
+}

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())) {
+    }
+}

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()) {
+    }
+}

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())) {
+    }
+}