(WIP) Collapse if statements rule (#801)

diktat-analysis.yml

@@ -379,6 +379,11 @@
 # Inspection that checks if string template has redundant quotes
 - name: STRING_TEMPLATE_QUOTES
   enabled: true
+# Check if there are redundant nested if-statements, which could be collapsed into a single one by concatenating their conditions
+- name: COLLAPSE_IF_STATEMENTS
+  enabled: true
+  configuration:
+    startCollapseFromNestedLevel: 2
 # Checks that floating-point values are not used in arithmetic expressions
 - name: FLOAT_IN_ACCURATE_CALCULATIONS
   enabled: true

diktat-rules/src/main/kotlin/generated/WarningNames.kt

@@ -183,6 +183,8 @@ public object WarningNames {
 
     public const val FILE_NAME_MATCH_CLASS: String = "FILE_NAME_MATCH_CLASS"
 
+    public const val COLLAPSE_IF_STATEMENTS: String = "COLLAPSE_IF_STATEMENTS"
+
     public const val NULLABLE_PROPERTY_TYPE: String = "NULLABLE_PROPERTY_TYPE"
 
     public const val TYPE_ALIAS: String = "TYPE_ALIAS"

diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/constants/Chapters.kt

@@ -13,6 +13,7 @@ import org.jetbrains.kotlin.org.jline.utils.Levenshtein
  */
 @Suppress("WRONG_DECLARATIONS_ORDER")
 enum class Chapters(val number: String, val title: String) {
+    DUMMY("0", "Dummy"),
     NAMING("1", "Naming"),
     COMMENTS("2", "Comments"),
     TYPESETTING("3", "General"),

diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/constants/Warnings.kt

@@ -130,6 +130,7 @@ enum class Warnings(
     STRING_TEMPLATE_CURLY_BRACES(true, "3.15.2", "string template has redundant curly braces"),
     STRING_TEMPLATE_QUOTES(true, "3.15.2", "string template has redundant quotes"),
     FILE_NAME_MATCH_CLASS(true, "3.1.2", "file name is incorrect - it should match with the class described in it if there is the only one class declared"),
+    COLLAPSE_IF_STATEMENTS(true, "3.16.1", "avoid using redundant nested if-statements, which could be collapsed into a single one"),
 
     // ======== chapter 4 ========
     NULLABLE_PROPERTY_TYPE(true, "4.3.1", "try to avoid use of nullable types"),

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

@@ -18,6 +18,7 @@ import org.cqfn.diktat.ruleset.rules.chapter3.AnnotationNewLineRule
 import org.cqfn.diktat.ruleset.rules.chapter3.BlockStructureBraces
 import org.cqfn.diktat.ruleset.rules.chapter3.BracesInConditionalsAndLoopsRule
 import org.cqfn.diktat.ruleset.rules.chapter3.ClassLikeStructuresOrderRule
+import org.cqfn.diktat.ruleset.rules.chapter3.CollapseIfStatementsRule
 import org.cqfn.diktat.ruleset.rules.chapter3.ConsecutiveSpacesRule
 import org.cqfn.diktat.ruleset.rules.chapter3.EmptyBlock
 import org.cqfn.diktat.ruleset.rules.chapter3.EnumsSeparated
@@ -206,6 +207,7 @@ class DiktatRuleSetProvider(private var diktatConfigFile: String = DIKTAT_ANALYS
             ::AvoidNestedFunctionsRule,
             ::ExtensionFunctionsSameNameRule,
             ::LambdaLengthRule,
+            ::CollapseIfStatementsRule,
             // formatting: moving blocks, adding line breaks, indentations etc.
             ::BlockStructureBraces,
             ::ConsecutiveSpacesRule,

diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter1/IdentifierNaming.kt

@@ -371,7 +371,9 @@ class IdentifierNaming(configRules: List<RulesConfig>) : DiktatRule(
         // We don't need to ask subclasses to rename superclass methods
         if (!node.isOverridden()) {
             // if function has Boolean return type in 99% of cases it is much better to name it with isXXX or hasXXX prefix
+            @Suppress("COLLAPSE_IF_STATEMENTS")
             if (functionReturnType != null && functionReturnType == PrimitiveType.BOOLEAN.typeName.asString()) {
+                @Suppress("COLLAPSE_IF_STATEMENTS")
                 if (allMethodPrefixes.none { functionName.text.startsWith(it) }) {
                     FUNCTION_BOOLEAN_PREFIX.warnAndFix(configRules, emitWarn, isFixMode, functionName.text, functionName.startOffset, functionName) {
                         // FixMe: add agressive autofix for this

diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter2/kdoc/CommentsFormatting.kt

@@ -302,22 +302,19 @@ class CommentsFormatting(configRules: List<RulesConfig>) : DiktatRule(
             WRONG_NEWLINES_AROUND_KDOC.warnAndFix(configRules, emitWarn, isFixMode, node.text, node.startOffset, node) {
                 node.treeParent.treeParent.addChild(PsiWhiteSpaceImpl("\n"), node.treeParent)
             }
-        } else if (node.treeParent.elementType != FILE) {
-            if (node.treeParent.treePrev.numNewLines() == 1 || node.treeParent.treePrev.numNewLines() > 2) {
-                WRONG_NEWLINES_AROUND_KDOC.warnAndFix(configRules, emitWarn, isFixMode, node.text, node.startOffset, node) {
-                    (node.treeParent.treePrev as LeafPsiElement).replaceWithText("\n\n")
-                }
+        } else if (node.treeParent.elementType != FILE &&
+                (node.treeParent.treePrev.numNewLines() == 1 || node.treeParent.treePrev.numNewLines() > 2)) {
+            WRONG_NEWLINES_AROUND_KDOC.warnAndFix(configRules, emitWarn, isFixMode, node.text, node.startOffset, node) {
+                (node.treeParent.treePrev as LeafPsiElement).replaceWithText("\n\n")
             }
         }
     }
 
     private fun checkFirstCommentSpaces(node: ASTNode) {
-        if (node.treePrev.isWhiteSpace()) {
-            if (node.treePrev.numNewLines() > 1 ||
-                    node.treePrev.numNewLines() == 0) {
-                FIRST_COMMENT_NO_BLANK_LINE.warnAndFix(configRules, emitWarn, isFixMode, node.text, node.startOffset, node) {
-                    (node.treePrev as LeafPsiElement).replaceWithText("\n")
-                }
+        if (node.treePrev.isWhiteSpace() &&
+                (node.treePrev.numNewLines() > 1 || node.treePrev.numNewLines() == 0)) {
+            FIRST_COMMENT_NO_BLANK_LINE.warnAndFix(configRules, emitWarn, isFixMode, node.text, node.startOffset, node) {
+                (node.treePrev as LeafPsiElement).replaceWithText("\n")
             }
         }
     }

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

@@ -0,0 +1,164 @@
+package org.cqfn.diktat.ruleset.rules.chapter3
+
+import org.cqfn.diktat.common.config.rules.RuleConfiguration
+import org.cqfn.diktat.common.config.rules.RulesConfig
+import org.cqfn.diktat.common.config.rules.getRuleConfig
+import org.cqfn.diktat.ruleset.constants.Warnings
+import org.cqfn.diktat.ruleset.rules.DiktatRule
+import org.cqfn.diktat.ruleset.utils.KotlinParser
+
+import com.pinterest.ktlint.core.ast.ElementType.BINARY_EXPRESSION
+import com.pinterest.ktlint.core.ast.ElementType.BLOCK_COMMENT
+import com.pinterest.ktlint.core.ast.ElementType.EOL_COMMENT
+import com.pinterest.ktlint.core.ast.ElementType.IF
+import com.pinterest.ktlint.core.ast.ElementType.KDOC
+import com.pinterest.ktlint.core.ast.ElementType.LBRACE
+import com.pinterest.ktlint.core.ast.ElementType.OPERATION_REFERENCE
+import com.pinterest.ktlint.core.ast.ElementType.RBRACE
+import com.pinterest.ktlint.core.ast.ElementType.THEN
+import com.pinterest.ktlint.core.ast.ElementType.WHITE_SPACE
+import com.pinterest.ktlint.core.ast.children
+import org.jetbrains.kotlin.com.intellij.lang.ASTNode
+import org.jetbrains.kotlin.psi.KtBlockExpression
+import org.jetbrains.kotlin.psi.KtIfExpression
+
+import java.util.Stack
+
+typealias placeOfWarningForCurrentNode = Pair<Int, ASTNode>
+
+/**
+ * Rule for redundant nested if-statements, which could be collapsed into a single one
+ */
+class CollapseIfStatementsRule(configRules: List<RulesConfig>) : DiktatRule(
+    "collapse-if",
+    configRules,
+    listOf(
+        Warnings.COLLAPSE_IF_STATEMENTS
+    )
+) {
+    private val configuration by lazy {
+        CollapseIfStatementsConfiguration(
+            configRules.getRuleConfig(Warnings.COLLAPSE_IF_STATEMENTS)?.configuration ?: emptyMap()
+        )
+    }
+
+    // We hold the warnings, which we raised, since in case of multi nested if-statement,
+    // there are could be several identical warning for one line
+    private val listOfWarnings: MutableSet<placeOfWarningForCurrentNode> = mutableSetOf()
+
+    override fun logic(node: ASTNode) {
+        if (node.elementType == IF) {
+            process(node)
+        }
+    }
+
+    private fun process(node: ASTNode) {
+        val startCollapseFromLevel = configuration.startCollapseFromNestedLevel
+        val listOfNestedNodes: Stack<ASTNode> = Stack()
+
+        var nestedIfNode = findNestedIf(node)
+        while (nestedIfNode != null) {
+            listOfNestedNodes.push(nestedIfNode)
+            nestedIfNode = findNestedIf(nestedIfNode)
+        }
+        val nestedLevel = listOfNestedNodes.size + 1
+        if (nestedLevel < startCollapseFromLevel) {
+            return
+        }
+        while (listOfNestedNodes.isNotEmpty()) {
+            val currNode = listOfNestedNodes.pop()
+            // Since the external `if` statement is not the direct parent,
+            // we need multiple steps to take the required one
+            // BLOCK -> THEN -> IF
+            val currParentNode = currNode.treeParent.treeParent.treeParent
+            if (listOfWarnings.add(currNode.startOffset to currNode)) {
+                Warnings.COLLAPSE_IF_STATEMENTS.warnAndFix(
+                    configRules, emitWarn, isFixMode,
+                    "avoid using redundant nested if-statements", currNode.startOffset, currNode
+                ) {
+                    collapse(currParentNode, currNode)
+                }
+            }
+        }
+    }
+
+    private fun findNestedIf(parentNode: ASTNode): ASTNode? {
+        val parentThenNode = (parentNode.psi as KtIfExpression).then?.node ?: return null
+        val nestedIfNode = parentThenNode.findChildByType(IF) ?: return null
+        // Nested `if` node should be the last child, but actually,
+        // the last children are WHITESPACE and `}`, so take treePrev
+        if ((nestedIfNode.psi as KtIfExpression).node != parentThenNode.lastChildNode.treePrev.treePrev) {
+            return null
+        }
+        // We won't collapse, if statements some of them have `else` node
+        if ((parentNode.psi as KtIfExpression).`else`?.node != null ||
+                (nestedIfNode.psi as KtIfExpression).`else`?.node != null) {
+            return null
+        }
+        // We monitor which types of nodes are followed before nested `if`
+        // and we allow only a limited number of types to pass through.
+        // Otherwise discovered `if` is not nested
+        val listOfNodesBeforeNestedIf = parentThenNode.getChildren(null).takeWhile { it.elementType != IF }
+        val allowedTypes = listOf(LBRACE, WHITE_SPACE, RBRACE, KDOC, BLOCK_COMMENT, EOL_COMMENT)
+        if (listOfNodesBeforeNestedIf.any { it.elementType !in allowedTypes }) {
+            return null
+        }
+        return nestedIfNode
+    }
+
+    private fun collapse(parentNode: ASTNode, nestedNode: ASTNode) {
+        collapseConditions(parentNode, nestedNode)
+        collapseThenBlocks(parentNode, nestedNode)
+    }
+
+    private fun collapseConditions(parentNode: ASTNode, nestedNode: ASTNode) {
+        // Merge parent and nested conditions
+        val parentCondition = (parentNode.psi as KtIfExpression).condition?.text
+        val nestedCondition = (nestedNode.psi as KtIfExpression).condition
+        // If the nested condition is compound,
+        // we need to put it to the brackets, according algebra of logic
+        val mergeCondition =
+                if (nestedCondition?.node?.elementType == BINARY_EXPRESSION &&
+                        nestedCondition.node?.findChildByType(OPERATION_REFERENCE)?.text == "||"
+                ) {
+                    "if ($parentCondition && (${nestedCondition.text})) {}"
+                } else {
+                    "if ($parentCondition && ${nestedCondition?.text}) {}"
+                }
+
+        val newParentIfNode = KotlinParser().createNode(mergeCondition)
+        // Remove THEN block
+        newParentIfNode.removeChild(newParentIfNode.lastChildNode)
+        // Remove old `if` from parent
+        parentNode.removeRange(parentNode.firstChildNode, parentNode.findChildByType(THEN))
+        // Add to parent all child from new `if` node
+        var addAfter = parentNode.firstChildNode
+        newParentIfNode.getChildren(null).forEachIndexed { index, child ->
+            parentNode.addChild(child, addAfter)
+            addAfter = parentNode.children().drop(index + 1).first()
+        }
+    }
+
+    private fun collapseThenBlocks(parentNode: ASTNode, nestedNode: ASTNode) {
+        // Merge parent and nested `THEN` blocks
+        val nestedThenNode = (nestedNode.psi as KtIfExpression).then
+        val nestedThenText = (nestedThenNode as KtBlockExpression).statements.joinToString("\n") { it.text }
+        val newNestedNode = KotlinParser().createNode(nestedThenText)
+        val parentThenNode = (parentNode.psi as KtIfExpression).then?.node
+        parentThenNode?.replaceChild(nestedNode, newNestedNode)
+    }
+
+    /**
+     * [RuleConfiguration] for configuration
+     */
+    class CollapseIfStatementsConfiguration(config: Map<String, String>) : RuleConfiguration(config) {
+        /**
+         *  Collapse statements only if nested level more than this value
+         */
+        val startCollapseFromNestedLevel = config["startCollapseFromNestedLevel"]?.toInt() ?: DEFAULT_NESTED_LEVEL
+    }
+
+    companion object {
+        private const val DEFAULT_NESTED_LEVEL = 2
+    }
+}

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

@@ -31,10 +31,8 @@ class EnumsSeparated(configRules: List<RulesConfig>) : DiktatRule(
     configRules,
     listOf(ENUMS_SEPARATED)) {
     override fun logic(node: ASTNode) {
-        if (node.elementType == CLASS && node.hasChildOfType(CLASS_BODY)) {
-            if (node.isClassEnum()) {
-                checkEnumEntry(node)
-            }
+        if (node.elementType == CLASS && node.hasChildOfType(CLASS_BODY) && node.isClassEnum()) {
+            checkEnumEntry(node)
         }
     }
 

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

@@ -24,21 +24,16 @@ class LongNumericalValuesSeparatedRule(configRules: List<RulesConfig>) : DiktatR
         val configuration = LongNumericalValuesConfiguration(
             configRules.getRuleConfig(LONG_NUMERICAL_VALUES_SEPARATED)?.configuration ?: emptyMap())
 
-        if (node.elementType == INTEGER_LITERAL) {
-            if (!isValidConstant(node.text, configuration, node)) {
-                LONG_NUMERICAL_VALUES_SEPARATED.warnAndFix(configRules, emitWarn, isFixMode, node.text, node.startOffset, node) {
-                    fixIntegerConstant(node, configuration.maxBlockLength)
-                }
+        if (node.elementType == INTEGER_LITERAL && !isValidConstant(node.text, configuration, node)) {
+            LONG_NUMERICAL_VALUES_SEPARATED.warnAndFix(configRules, emitWarn, isFixMode, node.text, node.startOffset, node) {
+                fixIntegerConstant(node, configuration.maxBlockLength)
             }
         }
 
-        if (node.elementType == FLOAT_LITERAL) {
-            if (!isValidConstant(node.text, configuration, node)) {
-                val parts = node.text.split(".")
-
-                LONG_NUMERICAL_VALUES_SEPARATED.warnAndFix(configRules, emitWarn, isFixMode, node.text, node.startOffset, node) {
-                    fixFloatConstantPart(parts[0], parts[1], configuration, node)
-                }
+        if (node.elementType == FLOAT_LITERAL && !isValidConstant(node.text, configuration, node)) {
+            val parts = node.text.split(".")
+            LONG_NUMERICAL_VALUES_SEPARATED.warnAndFix(configRules, emitWarn, isFixMode, node.text, node.startOffset, node) {
+                fixFloatConstantPart(parts[0], parts[1], configuration, node)
             }
         }
     }

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

@@ -36,6 +36,7 @@ class StringConcatenationRule(configRules: List<RulesConfig>) : DiktatRule(
         STRING_CONCATENATION
     )
 ) {
+    @Suppress("COLLAPSE_IF_STATEMENTS")
     override fun logic(node: ASTNode) {
         if (node.elementType == BINARY_EXPRESSION) {
             // searching top-level binary expression to detect any operations with "plus" (+)

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

@@ -137,6 +137,7 @@ class NullChecksRule(configRules: List<RulesConfig>) : DiktatRule(
             if (parent != null && parent.elementType == VALUE_ARGUMENT) {
                 val grandParent = parent.treeParent
                 if (grandParent != null && grandParent.elementType == VALUE_ARGUMENT_LIST && isRequireFun(grandParent.treePrev)) {
+                    @Suppress("COLLAPSE_IF_STATEMENTS")
                     if (listOf(ElementType.EXCLEQ, ElementType.EXCLEQEQEQ).contains(condition.operationToken)) {
                         warnAndFixOnNullCheck(
                             condition,

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

@@ -37,6 +37,7 @@ class TypeAliasRule(configRules: List<RulesConfig>) : DiktatRule(
      */
     private fun checkTypeReference(node: ASTNode, config: TypeAliasConfiguration) {
         if (node.textLength > config.typeReferenceLength) {
+            @Suppress("COLLAPSE_IF_STATEMENTS")
             if (node.findAllDescendantsWithSpecificType(LT).size > 1 || node.findAllDescendantsWithSpecificType(VALUE_PARAMETER).size > 1) {
                 TYPE_ALIAS.warn(configRules, emitWarn, isFixMode, "too long type reference", node.startOffset, node)
             }

diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter6/AvoidUtilityClass.kt

@@ -32,6 +32,7 @@ class AvoidUtilityClass(configRules: List<RulesConfig>) : DiktatRule(
         val config = configRules.getCommonConfiguration()
         val filePath = node.getRootNode().getFilePath()
         if (!(node.hasTestAnnotation() || isLocatedInTest(filePath.splitPathToDirs(), config.testAnchors))) {
+            @Suppress("COLLAPSE_IF_STATEMENTS")
             if (node.elementType == OBJECT_DECLARATION || node.elementType == CLASS) {
                 checkClass(node)
             }

diktat-rules/src/main/resources/diktat-analysis-huawei.yml

@@ -379,6 +379,11 @@
 # Inspection that checks if string template has redundant quotes
 - name: STRING_TEMPLATE_QUOTES
   enabled: true
+# Check if there are redundant nested if-statements, which could be collapsed into a single one by concatenating their conditions
+- name: COLLAPSE_IF_STATEMENTS
+  enabled: true
+  configuration:
+    startCollapseFromNestedLevel: 2
 # Checks that floating-point values are not used in arithmetic expressions
 - name: FLOAT_IN_ACCURATE_CALCULATIONS
   enabled: true

diktat-rules/src/main/resources/diktat-analysis.yml

@@ -375,6 +375,11 @@
 # Inspection that checks if string template has redundant quotes
 - name: STRING_TEMPLATE_QUOTES
   enabled: true
+# Check if there are redundant nested if-statements, which could be collapsed into a single one by concatenating their conditions
+- name: COLLAPSE_IF_STATEMENTS
+  enabled: true
+  configuration:
+    startCollapseFromNestedLevel: 2
 # Checks that floating-point values are not used in arithmetic expressions
 - name: FLOAT_IN_ACCURATE_CALCULATIONS
   enabled: true

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

@@ -0,0 +1,24 @@
+package org.cqfn.diktat.ruleset.chapter3
+
+import org.cqfn.diktat.common.config.rules.RulesConfig
+import org.cqfn.diktat.ruleset.constants.Warnings
+import org.cqfn.diktat.ruleset.rules.chapter3.CollapseIfStatementsRule
+import org.cqfn.diktat.util.FixTestBase
+
+import generated.WarningNames
+import org.junit.jupiter.api.Tag
+import org.junit.jupiter.api.Test
+
+class CollapseIfStatementsRuleFixTest : FixTestBase(
+    "test/paragraph3/collapse_if",
+    ::CollapseIfStatementsRule,
+    listOf(
+        RulesConfig(Warnings.COLLAPSE_IF_STATEMENTS.name, true, emptyMap())
+    )
+) {
+    @Test
+    @Tag(WarningNames.COLLAPSE_IF_STATEMENTS)
+    fun `collapse if`() {
+        fixAndCompare("CollapseIfStatementsExpected.kt", "CollapseIfStatementsTest.kt")
+    }
+}