Bugfix for boolean expressions

diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter3/BooleanExpressionsRule.kt

@@ -4,44 +4,56 @@ 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.findAllDescendantsWithSpecificType
 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.parsers.ExprParser
 import com.bpodgursky.jbool_expressions.rules.RuleSet
 import com.pinterest.ktlint.core.ast.ElementType
 import com.pinterest.ktlint.core.ast.ElementType.BINARY_EXPRESSION
+import com.pinterest.ktlint.core.ast.ElementType.CONDITION
 import com.pinterest.ktlint.core.ast.ElementType.OPERATION_REFERENCE
 import com.pinterest.ktlint.core.ast.ElementType.PARENTHESIZED
+import com.pinterest.ktlint.core.ast.ElementType.PREFIX_EXPRESSION
 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 warns if the boolean expression can be simplified.
+ * Rule that checks if the boolean expression can be simplified.
  */
 class BooleanExpressionsRule(configRules: List<RulesConfig>) : DiktatRule(
     "boolean-expressions-rule",
     configRules,
     listOf(COMPLEX_BOOLEAN_EXPRESSION)) {
     override fun logic(node: ASTNode) {
-        if (node.elementType == ElementType.CONDITION) {
+        if (node.elementType == CONDITION) {
             checkBooleanExpression(node)
         }
     }
 
     @Suppress("TooGenericExceptionCaught")
     private fun checkBooleanExpression(node: ASTNode) {
-        // This map is used to assign a variable name for every elementary boolean expression.
+        // 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 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)
         } catch (exc: RuntimeException) {
             if (exc.message?.startsWith("Unrecognized!") == true) {
-                // this comes up if there is an unparsable expression. For example a.and(b)
+                // this comes up if there is an unparsable expression (jbool doesn't have own exception type). For example a.and(b)
                 return
             } else {
                 throw exc
@@ -60,32 +72,75 @@ class BooleanExpressionsRule(configRules: List<RulesConfig>) : DiktatRule(
     }
 
     /**
+     * Converts a complex boolean expression into a string representation, mapping each elementary expression to a letter token.
+     * These tokens are collected into [mapOfExpressionToChar].
+     * For example:
+     * ```
+     * (a > 5 && b != 2) -> A & B
+     * (a > 5 || false) -> A | false
+     * (a > 5 || x.foo()) -> A | B
+     * ```
+     *
      * @param node
-     * @param mapOfExpressionToChar
-     * @return corrected string
+     * @param mapOfExpressionToChar a mutable map for expression->token
+     * @return formatted string representation of expression
      */
+    @Suppress("UnsafeCallOnNullableType", "ForbiddenComment")
     internal fun formatBooleanExpressionAsString(node: ASTNode, mapOfExpressionToChar: HashMap<String, Char>): String {
-        // `A` character in ASCII
-        var characterAsciiCode = 'A'.code
-        node
-            .findAllNodesWithCondition({ it.elementType == BINARY_EXPRESSION })
-            .filterNot { it.text.contains("&&") || it.text.contains("||") }
-            .forEach { expression ->
-                mapOfExpressionToChar.computeIfAbsent(expression.text) {
-                    characterAsciiCode++.toChar()
-                }
+        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 = 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
             }
-        // Library is using & as && and | as ||.
-        var correctedExpression = "(${node
-            .text
-            .replace("&&", "&")
-            .replace("||", "|")})"
+            .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"
+            }
+        var characterAsciiCode = 'A'.code  // `A` character in ASCII
+        (logicalExpressions + elementaryBooleanExpressions).forEach { expression ->
+            mapOfExpressionToChar.computeIfAbsent(expression.text) {
+                // Every elementary expression is assigned a single-letter variable.
+                characterAsciiCode++.toChar()
+            }
+        }
+        // Prepare final formatted string
+        var correctedExpression = node.text
+        // At first, substitute all elementary expressions with variables
         mapOfExpressionToChar.forEach { (refExpr, char) ->
             correctedExpression = correctedExpression.replace(refExpr, char.toString())
         }
-        return correctedExpression
+        // jBool library is using & as && and | as ||
+        return "(${correctedExpression
+            .replace("&&", "&")
+            .replace("||", "|")})"
     }
 
+    /**
+     * Split the complex expression into elementary parts
+     */
+    private fun ASTNode.collectElementaryExpressions() = this
+        .findAllNodesWithCondition({ astNode ->
+            astNode.elementType == BINARY_EXPRESSION &&
+                    // filter out boolean conditions in nested lambdas, e.g. `if (foo.filter { a && b })`
+                    (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 fixBooleanExpression(
         node: ASTNode,
         simplifiedExpr: Expression<String>,
@@ -196,6 +251,8 @@ class BooleanExpressionsRule(configRules: List<RulesConfig>) : DiktatRule(
         }
     }
 
+    private fun KtBinaryExpression.isXorExpression() = operationReference.text == "xor"
+
     companion object {
         const val DISTRIBUTIVE_LAW_MIN_EXPRESSIONS = 3
         const val DISTRIBUTIVE_LAW_MIN_OPERATIONS = 3

diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter3/files/NewlinesRule.kt

@@ -463,8 +463,8 @@ class NewlinesRule(configRules: List<RulesConfig>) : DiktatRule(
     private fun ASTNode.getOrderedCallExpressions(psi: PsiElement, result: MutableList<ASTNode>) {
         // if statements here have the only right order - don't change it
 
-        if (psi.children.isNotEmpty() && (!psi.isFirstChildElementType(DOT_QUALIFIED_EXPRESSION) &&
-                !psi.isFirstChildElementType(SAFE_ACCESS_EXPRESSION))) {
+        if (psi.children.isNotEmpty() && !(psi.isFirstChildElementType(DOT_QUALIFIED_EXPRESSION) ||
+                psi.isFirstChildElementType(SAFE_ACCESS_EXPRESSION))) {
             val firstChild = psi.firstChild
             if (firstChild.isFirstChildElementType(DOT_QUALIFIED_EXPRESSION) ||
                     firstChild.isFirstChildElementType(SAFE_ACCESS_EXPRESSION)) {

diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter4/VariableGenericTypeDeclarationRule.kt

@@ -54,7 +54,7 @@ class VariableGenericTypeDeclarationRule(configRules: List<RulesConfig>) : Dikta
                 ?.typeArgumentsAsTypes
         }
 
-        if ((rightSide != null && leftSide != null) &&
+        if (!(rightSide == null || leftSide == null) &&
                 rightSide.size == leftSide.size &&
                 rightSide.zip(leftSide).all { (first, second) -> first.text == second.text }) {
             GENERIC_VARIABLE_WRONG_DECLARATION.warnAndFix(configRules, emitWarn, isFixMode,

diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/utils/AstConstants.kt

@@ -22,6 +22,11 @@ internal const val EMPTY_BLOCK_TEXT = "{}"
  */
 internal val standardMethods = listOf("main", "equals", "hashCode", "toString", "clone", "finalize")
 
+/**
+ * List of infix methods that return Boolean
+ */
+internal val logicalInfixMethods = setOf("==", "!=", ">", "<", ">=", "<=", "in", "!in", "xor")
+
 /**
  * List of element types present in empty code block `{ }`
  */

diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter3/BooleanExpressionsRuleWarnTest.kt

@@ -11,6 +11,8 @@ import generated.WarningNames
 import org.junit.jupiter.api.Assertions
 import org.junit.jupiter.api.Tag
 import org.junit.jupiter.api.Test
+import java.io.ByteArrayOutputStream
+import java.io.PrintStream
 
 class BooleanExpressionsRuleWarnTest : LintTestBase(::BooleanExpressionsRule) {
     private val ruleId = "$DIKTAT_RULE_SET_ID:boolean-expressions-rule"
@@ -135,36 +137,147 @@ class BooleanExpressionsRuleWarnTest : LintTestBase(::BooleanExpressionsRule) {
 
     @Test
     fun `test makeCorrectExpressionString method #1`() {
-        val firstCondition = KotlinParser().createNode("a > 5 && b < 6")
-        val returnedString = BooleanExpressionsRule(emptyList()).formatBooleanExpressionAsString(firstCondition, HashMap())
-        Assertions.assertEquals(returnedString, "(A & B)")
+        checkExpressionFormatter("a > 5 && b < 6", "(A & B)", 2)
     }
 
     @Test
     fun `test makeCorrectExpressionString method #2`() {
-        val firstCondition = KotlinParser().createNode("a > 5 && b < 6 && c > 7 || a > 5")
-        val returnedString = BooleanExpressionsRule(emptyList()).formatBooleanExpressionAsString(firstCondition, HashMap())
-        Assertions.assertEquals(returnedString, "(A & B & C | A)")
+        checkExpressionFormatter("a > 5 && b < 6 && c > 7 || a > 5", "(A & B & C | A)", 3)
     }
 
     @Test
     fun `test makeCorrectExpressionString method #3`() {
-        val firstCondition = KotlinParser().createNode("a > 5 && b < 6 && (c > 3 || b < 6) && a > 5")
-        val returnedString = BooleanExpressionsRule(emptyList()).formatBooleanExpressionAsString(firstCondition, HashMap())
-        Assertions.assertEquals(returnedString, "(A & B & (C | B) & A)")
+        checkExpressionFormatter("a > 5 && b < 6 && (c > 3 || b < 6) && a > 5", "(A & B & (C | B) & A)", 3)
     }
 
     @Test
     fun `test makeCorrectExpressionString method #4`() {
-        val firstCondition = KotlinParser().createNode("a > 5 && b < 6 && (c > 3 || b < 6) && a > 666")
-        val returnedString = BooleanExpressionsRule(emptyList()).formatBooleanExpressionAsString(firstCondition, HashMap())
-        Assertions.assertEquals(returnedString, "(A & B & (C | B) & D)")
+        checkExpressionFormatter("a > 5 && b < 6 && (c > 3 || b < 6) && a > 666", "(A & B & (C | B) & D)", 4)
     }
 
     @Test
-    fun `test makeCorrectExpressionString method #5`() {
-        val firstCondition = KotlinParser().createNode("a.and(b)")
-        val returnedString = BooleanExpressionsRule(emptyList()).formatBooleanExpressionAsString(firstCondition, HashMap())
-        Assertions.assertEquals(returnedString, "(a.and(b))")
+    fun `test makeCorrectExpressionString method #5 - should not convert single expressions`() {
+        checkExpressionFormatter("a.and(b)", "(a.and(b))", 0)
+    }
+
+    @Test
+    fun `test makeCorrectExpressionString method #6 - should not convert single expressions`() {
+        checkExpressionFormatter("x.isFoo()", "(x.isFoo())", 0)
+    }
+
+    @Test
+    fun `test makeCorrectExpressionString method #7`() {
+        checkExpressionFormatter("x.isFoo() && true", "(A & true)", 1)
+    }
+
+    @Test
+    fun `test makeCorrectExpressionString method #8`() {
+        checkExpressionFormatter(
+            "a > 5 && b > 6 || c > 7 && a > 5",
+            "(A & B | C & A)",
+            3
+        )
+    }
+
+    @Test
+    fun `test makeCorrectExpressionString method #9 - should not account for boolean operators in nested lambdas`() {
+        checkExpressionFormatter(
+            """
+                nextNode != null && nextNode.findChildByType(CALL_EXPRESSION)?.text?.let {
+                    it == "trimIndent()" || it == "trimMargin()"
+                } == true
+            """.trimIndent(),
+            "(A & B)",
+            2
+        )
+    }
+
+    @Test
+    fun `test makeCorrectExpressionString method #10 - single variable in condition`() {
+        checkExpressionFormatter(
+            "::testContainerId.isInitialized",
+            "(::testContainerId.isInitialized)",
+            0
+        )
+    }
+
+    @Test
+    fun `test makeCorrectExpressionString method #11 - variable in condition and binary expression`() {
+        checkExpressionFormatter(
+            "::testContainerId.isInitialized || a > 2",
+            "(B | A)",
+            2
+        )
+    }
+
+    @Test
+    @Suppress("TOO_LONG_FUNCTION", "LongMethod")
+    fun `regression - should not log ANTLR errors when parsing is not required`() {
+        val stream = ByteArrayOutputStream()
+        System.setErr(PrintStream(stream))
+        lintMethod(
+            """
+                fun foo() {
+                    // nested boolean expressions in lambdas
+                    if (currentProperty.nextSibling { it.elementType == PROPERTY } == nextProperty) {}
+
+                    if (!(rightSide == null || leftSide == null) &&
+                        rightSide.size == leftSide.size &&
+                        rightSide.zip(leftSide).all { (first, second) -> first.text == second.text }) {}
+
+                    // nested lambda with if-else
+                    if (currentProperty.nextSibling { if (it.elementType == PROPERTY) true else false } == nextProperty) {}
+
+                    // nested boolean expressions in lambdas with multi-line expressions
+                    if (node.elementType == TYPE_REFERENCE && node
+                        .parents()
+                        .map { it.elementType }
+                        .none { it == SUPER_TYPE_LIST || it == TYPEALIAS }) {}
+
+                    // binary expression with boolean literal
+                    if (result?.flag == true) {}
+
+                    if (leftOffset + binaryText.length > wrongBinaryExpression.maximumLineLength && index != 0) {}
+
+                    // with in and !in
+                    if (!isImportOrPackage && previousNonWhiteSpaceNode in acc.last()) {}
+                    if (node.elementType == LABEL_QUALIFIER && node.text !in labels && node.treeParent.elementType in stopWords) {}
+
+                    if ((node.treeNext.elementType == RBRACE) xor (node.treePrev.elementType == LBRACE)) {}
+
+                    if (listOfNodesBeforeNestedIf.any { it.elementType !in allowedTypes } ||
+                        listOfNodesAfterNestedIf.any { it.elementType !in allowedTypes }) {
+                            return null
+                    }
+                    if ((parentNode.psi as KtIfExpression).`else` != null ||
+                        (nestedIfNode.psi as KtIfExpression).`else` != null) {}
+                    if (listOfWarnings.add(currNode.startOffset to currNode)) {}
+                }
+            """.trimIndent()
+        )
+        System.setErr(System.err)
+        val stderr = stream.toString()
+        Assertions.assertTrue(stderr.isEmpty()) {
+            "stderr should be empty, but got the following:${System.lineSeparator()}$stderr"
+        }
+    }
+
+    private fun checkExpressionFormatter(
+        expression: String,
+        expectedRepresentation: String,
+        expectedMapSize: Int
+    ) {
+        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)
+        Assertions.assertEquals(expectedRepresentation, result)
+        Assertions.assertEquals(expectedMapSize, map.size)
+        System.setErr(System.err)
+        val stderr = stream.toString()
+        Assertions.assertTrue(stderr.isEmpty()) {
+            "stderr should be empty, but got the following:${System.lineSeparator()}$stderr"
+        }
     }
 }