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

Bugfix for boolean expressions #909

Merged
merged 16 commits into from
Jun 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -139,8 +139,8 @@ class IdentifierNaming(configRules: List<RulesConfig>) : DiktatRule(
// node is a symbol declaration with present identifier
val identifierText = identifier.text
if (identifierText.startsWith('`') && identifierText.endsWith('`')) {
// the only exception is test method with @Test annotation
if (!(node.elementType == ElementType.FUN && node.hasTestAnnotation())) {
val isTestFun = node.elementType == ElementType.FUN && node.hasTestAnnotation()
if (!isTestFun) {
BACKTICKS_PROHIBITED.warn(configRules, emitWarn, isFixMode, identifierText, identifier.startOffset, identifier)
}
return true
Expand Down Expand Up @@ -422,10 +422,9 @@ class IdentifierNaming(configRules: List<RulesConfig>) : DiktatRule(
isVariable: Boolean
) {
nodes.forEach {
if (it.text != "_" && !(it.isTextLengthInRange(MIN_IDENTIFIER_LENGTH..MAX_IDENTIFIER_LENGTH) ||
oneCharIdentifiers.contains(it.text) && isVariable || isValidCatchIdentifier(it)

)
val isValidOneCharVariable = oneCharIdentifiers.contains(it.text) && isVariable
if (it.text != "_" && !it.isTextLengthInRange(MIN_IDENTIFIER_LENGTH..MAX_IDENTIFIER_LENGTH) &&
!isValidOneCharVariable && !isValidCatchIdentifier(it)
) {
IDENTIFIER_LENGTH.warn(configRules, emitWarn, isFixMode, it.text, it.startOffset, it)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class KdocComments(configRules: List<RulesConfig>) : DiktatRule(
override fun logic(node: ASTNode) {
val config = configRules.getCommonConfiguration()
val filePath = node.getRootNode().getFilePath()
if (!(node.hasTestAnnotation() || isLocatedInTest(filePath.splitPathToDirs(), config.testAnchors))) {
if (!node.hasTestAnnotation() && !isLocatedInTest(filePath.splitPathToDirs(), config.testAnchors)) {
when (node.elementType) {
FILE -> checkTopLevelDoc(node)
CLASS -> checkClassElements(node)
Expand Down Expand Up @@ -86,7 +86,7 @@ class KdocComments(configRules: List<RulesConfig>) : DiktatRule(
@Suppress("UnsafeCallOnNullableType", "ComplexMethod")
private fun checkValueParameter(node: ASTNode) {
if (node.parents().none { it.elementType == PRIMARY_CONSTRUCTOR } ||
!(node.hasChildOfType(VAL_KEYWORD) || node.hasChildOfType(VAR_KEYWORD))) {
!node.hasChildOfType(VAL_KEYWORD) && !node.hasChildOfType(VAR_KEYWORD)) {
return
}
val prevComment = if (node.treePrev.elementType == WHITE_SPACE &&
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,44 +4,57 @@ 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.options.ExprOptions
import com.bpodgursky.jbool_expressions.parsers.ExprParser
import com.bpodgursky.jbool_expressions.rules.RuleSet
import com.bpodgursky.jbool_expressions.rules.RulesHelper
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
Expand All @@ -51,7 +64,7 @@ class BooleanExpressionsRule(configRules: List<RulesConfig>) : DiktatRule(
val simplifiedExpression = distributiveLawString?.let {
ExprParser.parse(distributiveLawString)
}
?: RuleSet.simplify(expr)
?: 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)
Expand All @@ -60,32 +73,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
petertrr marked this conversation as resolved.
Show resolved Hide resolved
* (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>,
Expand Down Expand Up @@ -196,6 +252,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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ class NewlinesRule(configRules: List<RulesConfig>) : DiktatRule(

private fun handleOperatorWithLineBreakAfter(node: ASTNode) {
// [node] should be either EQ or OPERATION_REFERENCE which has single child
if (!(node.elementType == EQ || node.firstChildNode.elementType in lineBreakAfterOperators || node.isInfixCall())) {
if (node.elementType != EQ && node.firstChildNode.elementType !in lineBreakAfterOperators && !node.isInfixCall()) {
return
}

Expand Down Expand Up @@ -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)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class AvoidUtilityClass(configRules: List<RulesConfig>) : DiktatRule(
override fun logic(node: ASTNode) {
val config = configRules.getCommonConfiguration()
val filePath = node.getRootNode().getFilePath()
if (!(node.hasTestAnnotation() || isLocatedInTest(filePath.splitPathToDirs(), config.testAnchors))) {
if (!node.hasTestAnnotation() && !isLocatedInTest(filePath.splitPathToDirs(), config.testAnchors)) {
@Suppress("COLLAPSE_IF_STATEMENTS")
if (node.elementType == OBJECT_DECLARATION || node.elementType == CLASS) {
checkClass(node)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ class RunInScript(private val configRules: List<RulesConfig>) : Rule("run-script
private fun checkScript(node: ASTNode) {
val isLambdaArgument = node.firstChildNode.hasChildOfType(LAMBDA_ARGUMENT)
val isLambdaInsideValueArgument = node.firstChildNode.findChildByType(VALUE_ARGUMENT_LIST)?.findChildByType(VALUE_ARGUMENT)?.findChildByType(LAMBDA_EXPRESSION) != null
if (!(isLambdaArgument || isLambdaInsideValueArgument)) {
if (!isLambdaArgument && !isLambdaInsideValueArgument) {
warnRunInScript(node)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 `{ }`
*/
Expand Down
Loading