Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BooleanExpressionRule fixes #1295

Merged
merged 10 commits into from
May 25, 2022
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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)
Expand All @@ -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
Expand All @@ -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`
Expand All @@ -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("&&", "&")
Expand All @@ -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()
}
Expand All @@ -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()
Expand All @@ -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))
}

Expand All @@ -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 == '|' }
Expand All @@ -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 ->
Expand All @@ -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
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a getter for this is redundant, because you can have a public val with the same effect as private field with public getter

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for suggestion, fixed


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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
}
Expand Down Expand Up @@ -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()) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package test.paragraph3.boolean_expressions

fun foo() {
if (bar) {
}
if (bar) {
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package test.paragraph3.boolean_expressions

fun foo() {
if (bar && (!isEmpty() || isEmpty())) {
}
if (bar && (isEmpty() || !isEmpty())) {
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package test.paragraph3.boolean_expressions

fun foo() {
if (isB && isA && isC) {
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package test.paragraph3.boolean_expressions

fun foo() {
if (isB && isA && isC && (isEmpty() || !isEmpty())) {
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package test.paragraph3.boolean_expressions

fun foo() {
if (isABC_A && isABC_B && isABC_C) {
}
if (isAdd && isAdded()) {
}
}
Original file line number Diff line number Diff line change
@@ -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())) {
}
}