Added logic for parameters and supertypes lists to NewlinesRule (#431)

### What's done: * Changed logic * Added tests * Updated code style guide
saveourtool · Oct 30, 2020 · e273989 · e273989 · 0pdd · Oct 30, 2020
1 parent f3da291
commit e273989
Show file tree

Hide file tree

Showing 10 changed files with 239 additions and 22 deletions.
diff --git a/diktat-analysis.yml b/diktat-analysis.yml
@@ -192,7 +192,8 @@
   configuration: {}
 - name: WRONG_NEWLINES
   enabled: true
-  configuration: {}
+  configuration:
+    maxParametersInOneLine: 2
 - name: TOO_MANY_CONSECUTIVE_SPACES
   enabled: true
   configuration:

diff --git a/diktat-rules/pom.xml b/diktat-rules/pom.xml
@@ -10,8 +10,8 @@
         <groupId>org.cqfn.diktat</groupId>
         <artifactId>diktat-parent</artifactId>
         <version>0.1.3-SNAPSHOT</version>
-
     </parent>
+
     <properties>
         <maven.compiler.source>1.8</maven.compiler.source>
         <maven.compiler.target>1.8</maven.compiler.target>

diff --git a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/files/NewlinesRule.kt b/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/files/NewlinesRule.kt
@@ -8,6 +8,7 @@ import com.pinterest.ktlint.core.ast.ElementType.BLOCK
 import com.pinterest.ktlint.core.ast.ElementType.BLOCK_COMMENT
 import com.pinterest.ktlint.core.ast.ElementType.CALLABLE_REFERENCE_EXPRESSION
 import com.pinterest.ktlint.core.ast.ElementType.CALL_EXPRESSION
+import com.pinterest.ktlint.core.ast.ElementType.CLASS
 import com.pinterest.ktlint.core.ast.ElementType.COLON
 import com.pinterest.ktlint.core.ast.ElementType.COLONCOLON
 import com.pinterest.ktlint.core.ast.ElementType.COMMA
@@ -21,6 +22,8 @@ import com.pinterest.ktlint.core.ast.ElementType.EOL_COMMENT
 import com.pinterest.ktlint.core.ast.ElementType.EQ
 import com.pinterest.ktlint.core.ast.ElementType.FUN
 import com.pinterest.ktlint.core.ast.ElementType.FUNCTION_LITERAL
+import com.pinterest.ktlint.core.ast.ElementType.FUNCTION_TYPE
+import com.pinterest.ktlint.core.ast.ElementType.FUNCTION_TYPE_RECEIVER
 import com.pinterest.ktlint.core.ast.ElementType.IDENTIFIER
 import com.pinterest.ktlint.core.ast.ElementType.IF
 import com.pinterest.ktlint.core.ast.ElementType.IMPORT_DIRECTIVE
@@ -37,46 +40,44 @@ import com.pinterest.ktlint.core.ast.ElementType.PACKAGE_DIRECTIVE
 import com.pinterest.ktlint.core.ast.ElementType.PLUS
 import com.pinterest.ktlint.core.ast.ElementType.PLUSEQ
 import com.pinterest.ktlint.core.ast.ElementType.PRIMARY_CONSTRUCTOR
-import com.pinterest.ktlint.core.ast.ElementType.REFERENCE_EXPRESSION
 import com.pinterest.ktlint.core.ast.ElementType.RETURN
 import com.pinterest.ktlint.core.ast.ElementType.RETURN_KEYWORD
 import com.pinterest.ktlint.core.ast.ElementType.SAFE_ACCESS
 import com.pinterest.ktlint.core.ast.ElementType.SAFE_ACCESS_EXPRESSION
 import com.pinterest.ktlint.core.ast.ElementType.SECONDARY_CONSTRUCTOR
 import com.pinterest.ktlint.core.ast.ElementType.SEMICOLON
-import com.pinterest.ktlint.core.ast.ElementType.VALUE_ARGUMENT
+import com.pinterest.ktlint.core.ast.ElementType.SUPER_TYPE_LIST
 import com.pinterest.ktlint.core.ast.ElementType.VALUE_ARGUMENT_LIST
+import com.pinterest.ktlint.core.ast.ElementType.VALUE_PARAMETER
 import com.pinterest.ktlint.core.ast.ElementType.VALUE_PARAMETER_LIST
 import com.pinterest.ktlint.core.ast.ElementType.WHITE_SPACE
-import com.pinterest.ktlint.core.ast.isLeaf
 import com.pinterest.ktlint.core.ast.nextCodeSibling
 import com.pinterest.ktlint.core.ast.parent
 import com.pinterest.ktlint.core.ast.prevCodeSibling
+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.REDUNDANT_SEMICOLON
 import org.cqfn.diktat.ruleset.constants.Warnings.WRONG_NEWLINES
 import org.cqfn.diktat.ruleset.utils.appendNewlineMergingWhiteSpace
 import org.cqfn.diktat.ruleset.utils.emptyBlockList
 import org.cqfn.diktat.ruleset.utils.extractLineOfText
 import org.cqfn.diktat.ruleset.utils.findAllNodesWithSpecificType
+import org.cqfn.diktat.ruleset.utils.getIdentifierName
 import org.cqfn.diktat.ruleset.utils.isBeginByNewline
 import org.cqfn.diktat.ruleset.utils.isEol
 import org.cqfn.diktat.ruleset.utils.isFollowedByNewline
 import org.cqfn.diktat.ruleset.utils.isSingleLineIfElse
 import org.cqfn.diktat.ruleset.utils.leaveOnlyOneNewLine
+import org.cqfn.diktat.ruleset.utils.log
+import org.jetbrains.kotlin.backend.common.onlyIf
 import org.jetbrains.kotlin.com.intellij.lang.ASTNode
 import org.jetbrains.kotlin.com.intellij.psi.PsiElement
 import org.jetbrains.kotlin.com.intellij.psi.impl.source.tree.LeafPsiElement
 import org.jetbrains.kotlin.com.intellij.psi.impl.source.tree.PsiWhiteSpaceImpl
-import org.jetbrains.kotlin.com.intellij.psi.tree.IElementType
 import org.jetbrains.kotlin.com.intellij.psi.tree.TokenSet
-import org.jetbrains.kotlin.ir.expressions.IrStatementOrigin
-import org.jetbrains.kotlin.psi.KtDotQualifiedExpression
-import org.jetbrains.kotlin.psi.KtExpression
-import org.jetbrains.kotlin.psi.KtExpressionImpl
-import org.jetbrains.kotlin.psi.KtPostfixExpression
-import org.jetbrains.kotlin.psi.KtQualifiedExpression
-import org.jetbrains.kotlin.psi.KtSafeQualifiedExpression
+import org.jetbrains.kotlin.psi.KtParameterList
+import org.jetbrains.kotlin.psi.KtSuperTypeList
 import org.jetbrains.kotlin.psi.psiUtil.children
 import org.jetbrains.kotlin.psi.psiUtil.parents
 import org.jetbrains.kotlin.psi.psiUtil.siblings
@@ -91,6 +92,7 @@ import org.jetbrains.kotlin.psi.psiUtil.siblings
  * 6. Ensures that function or constructor name isn't separated from `(` by space or newline
  * 7. Ensures that in multiline lambda newline follows arrow or, in case of lambda without explicit parameters, opening brace
  * 8. Checks that functions with single `return` are simplified to functions with expression body
+ * 9. parameter or argument lists and supertype lists that have more than 2 elements should be separated by newlines
  */
 @Suppress("ForbiddenComment")
 class NewlinesRule(private val configRules: List<RulesConfig>) : Rule("newlines") {
@@ -107,8 +109,11 @@ class NewlinesRule(private val configRules: List<RulesConfig>) : Rule("newlines"
         private val dropChainValues = TokenSet.create(EOL_COMMENT, WHITE_SPACE, BLOCK_COMMENT, KDOC)
     }
 
-    private lateinit var emitWarn: ((offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit)
+    private val configuration by lazy {
+        NewlinesRuleConfiguration(configRules.getRuleConfig(WRONG_NEWLINES)?.configuration ?: emptyMap())
+    }
     private var isFixMode: Boolean = false
+    private lateinit var emitWarn: ((offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit)
 
     override fun visit(node: ASTNode,
                        autoCorrect: Boolean,
@@ -124,9 +129,13 @@ class NewlinesRule(private val configRules: List<RulesConfig>) : Rule("newlines"
             COMMA -> handleComma(node)
             BLOCK -> handleLambdaBody(node)
             RETURN -> handleReturnStatement(node)
+            SUPER_TYPE_LIST, VALUE_PARAMETER_LIST -> handleList(node)
         }
     }
 
+    /**
+     * Check that EOL semicolon is used only in enums
+     */
     private fun handleSemicolon(node: ASTNode) {
         if (node.isEol() && node.treeParent.elementType != ENUM_ENTRY) {
             // semicolon at the end of line which is not part of enum members declarations
@@ -213,6 +222,9 @@ class NewlinesRule(private val configRules: List<RulesConfig>) : Rule("newlines"
         }
     }
 
+    /**
+     * Check that newline is not placed before a comma
+     */
     private fun handleComma(node: ASTNode) {
         val prevNewLine = node
                 .parent({ it.treePrev != null }, strict = false)
@@ -296,6 +308,69 @@ class NewlinesRule(private val configRules: List<RulesConfig>) : Rule("newlines"
                 }
     }
 
+    /**
+     * Checks that members of [VALUE_PARAMETER_LIST] (list of function parameters at declaration site) are separated with newlines.
+     * Also checks that entries of [SUPER_TYPE_LIST] are separated by newlines.
+     */
+    private fun handleList(node: ASTNode) {
+        if (node.elementType == VALUE_PARAMETER_LIST && node.treeParent.elementType.let { it == FUNCTION_TYPE || it == FUNCTION_TYPE_RECEIVER }) {
+            // do not check other value lists
+            return
+        }
+
+        val (numEntries, entryType) = when (node.elementType) {
+            VALUE_PARAMETER_LIST -> (node.psi as KtParameterList).parameters.size to "value parameters"
+            SUPER_TYPE_LIST -> (node.psi as KtSuperTypeList).entries.size to "supertype list entries"
+            else -> {
+                log.warn("Unexpected node element type ${node.elementType}")
+                return
+            }
+        }
+        if (numEntries > configuration.maxParametersInOneLine) {
+            when (node.elementType) {
+                VALUE_PARAMETER_LIST -> handleFirstValueParameter(node)
+            }
+
+            handleValueParameterList(node, entryType)
+        }
+    }
+
+    private fun handleFirstValueParameter(node: ASTNode) = node
+            .children()
+            .takeWhile { !it.textContains('\n') }
+            .filter { it.elementType == VALUE_PARAMETER }
+            .toList()
+            .onlyIf({ size > 1 }) {
+                WRONG_NEWLINES.warnAndFix(configRules, emitWarn, isFixMode, "first parameter should be placed on a separate line " +
+                        "or all other parameters should be aligned with it in declaration of <${node.getParentIdentifier()}>", node.startOffset, node) {
+                    node.appendNewlineMergingWhiteSpace(it.first().treePrev.takeIf { it.elementType == WHITE_SPACE }, it.first())
+                }
+            }
+
+    private fun handleValueParameterList(node: ASTNode, entryType: String) = node
+            .children()
+            .filter {
+                it.elementType == COMMA &&
+                        !it.treeNext.run { elementType == WHITE_SPACE && textContains('\n') }
+            }
+            .toList()
+            .onlyIf({ isNotEmpty() }) { invalidCommas ->
+                WRONG_NEWLINES.warnAndFix(configRules, emitWarn, isFixMode,
+                    "$entryType should be placed on different lines in declaration of <${node.getParentIdentifier()}>", node.startOffset, node) {
+                    invalidCommas.forEach { comma ->
+                        val nextWhiteSpace = comma.treeNext.takeIf { it.elementType == WHITE_SPACE }
+                        comma.appendNewlineMergingWhiteSpace(nextWhiteSpace, nextWhiteSpace?.treeNext ?: comma.treeNext)
+                    }
+                }
+            }
+
+    @Suppress("UnsafeCallOnNullableType")
+    private fun ASTNode.getParentIdentifier() = when (treeParent.elementType) {
+        PRIMARY_CONSTRUCTOR -> treeParent.treeParent
+        SECONDARY_CONSTRUCTOR -> parent(CLASS)!!
+        else -> treeParent
+    }
+        .getIdentifierName()?.text
 
     private fun ASTNode.getOrderedCallExpressions(psi: PsiElement, result: MutableList<ASTNode>) {
         // if statements here have the only right order - don't change it
@@ -373,3 +448,10 @@ class NewlinesRule(private val configRules: List<RulesConfig>) : Rule("newlines"
             firstChildNode.elementType == IDENTIFIER &&
             treeParent.elementType == BINARY_EXPRESSION
 }
+
+private class NewlinesRuleConfiguration(config: Map<String, String>) : RuleConfiguration(config) {
+    /**
+     * If the number of parameters on one line is more than this threshold, all parameters should be placed on separate lines.
+     */
+    val maxParametersInOneLine = config["maxParametersInOneLine"]?.toInt() ?: 2
+}
diff --git a/diktat-rules/src/main/resources/diktat-analysis-huawei.yml b/diktat-rules/src/main/resources/diktat-analysis-huawei.yml
@@ -191,7 +191,8 @@
   configuration: {}
 - name: WRONG_NEWLINES
   enabled: true
-  configuration: {}
+  configuration:
+    maxParametersInOneLine: 2
 - name: TOO_MANY_CONSECUTIVE_SPACES
   enabled: true
   configuration:

diff --git a/diktat-rules/src/main/resources/diktat-analysis.yml b/diktat-rules/src/main/resources/diktat-analysis.yml
@@ -193,7 +193,8 @@
   configuration: {}
 - name: WRONG_NEWLINES
   enabled: true
-  configuration: {}
+  configuration:
+    maxParametersInOneLine: 2
 - name: TOO_MANY_CONSECUTIVE_SPACES
   enabled: true
   configuration:

diff --git a/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter3/files/NewlinesRuleFixTest.kt b/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter3/files/NewlinesRuleFixTest.kt
@@ -48,4 +48,10 @@ class NewlinesRuleFixTest : FixTestBase("test/paragraph3/newlines", ::NewlinesRu
     fun `should replace functions with only return with expression body`() {
         fixAndCompare("ExpressionBodyExpected.kt", "ExpressionBodyTest.kt")
     }
+
+    @Test
+    @Tag(WarningNames.WRONG_NEWLINES)
+    fun `should insert newlines in a long parameter or supertype list`() {
+        fixAndCompare("ParameterListExpected.kt", "ParameterListTest.kt")
+    }
 }
diff --git a/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter3/files/NewlinesRuleWarnTest.kt b/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter3/files/NewlinesRuleWarnTest.kt
@@ -11,6 +11,7 @@ import org.junit.jupiter.api.Disabled
 import org.junit.jupiter.api.Tag
 import org.junit.jupiter.api.Test
 
+@Suppress("LargeClass")
 class NewlinesRuleWarnTest : LintTestBase(::NewlinesRule) {
     private val ruleId = "$DIKTAT_RULE_SET_ID:newlines"
     private val shouldBreakAfter = "${WRONG_NEWLINES.warnText()} should break a line after and not before"
@@ -579,4 +580,75 @@ class NewlinesRuleWarnTest : LintTestBase(::NewlinesRule) {
                 LintError(19, 20, ruleId,"${WRONG_NEWLINES.warnText()} should follow functional style at .", true)
         )
     }
+
+    @Test
+    @Tag(WarningNames.WRONG_NEWLINES)
+    fun `should suggest newlines in a long argument list of a constructor`() {
+        lintMethod(
+            """
+                |class Foo(val arg1: Int, arg2: Int) { }
+                |
+                |class Foo(val arg1: Int, arg2: Int, arg3: Int) { 
+                |    constructor(arg1: Int, arg2: String, arg3: String) : this(arg1, 0, 0) { }
+                |}
+                |
+                |class Foo(val arg1: Int,
+                |          var arg2: Int,
+                |          arg3: Int) { }
+            """.trimMargin(),
+            LintError(3, 10, ruleId, "${WRONG_NEWLINES.warnText()} first parameter should be placed on a separate line or all other parameters " +
+                    "should be aligned with it in declaration of <Foo>", true),
+            LintError(3, 10, ruleId, "${WRONG_NEWLINES.warnText()} value parameters should be placed on different lines in declaration of <Foo>", true),
+            LintError(4, 16, ruleId, "${WRONG_NEWLINES.warnText()} first parameter should be placed on a separate line or all other parameters " +
+                    "should be aligned with it in declaration of <Foo>", true),
+            LintError(4, 16, ruleId, "${WRONG_NEWLINES.warnText()} value parameters should be placed on different lines in declaration of <Foo>", true)
+        )
+    }
+
+    @Test
+    @Tag(WarningNames.WRONG_NEWLINES)
+    fun `should suggest newlines in a long argument list`() {
+        lintMethod(
+            """
+                |fun foo(arg1: Int, arg2: Int) { }
+                |
+                |fun bar(arg1: Int, arg2: Int, arg3: Int) { }
+                |
+                |// should not trigger on functional types
+                |fun bar(arg1: (_arg1: Int, _arg2: Int, _arg3: Int) -> Int) { }
+                |
+                |// should not trigger on functional type receivers
+                |fun bar(arg1: Foo.(_arg1: Int, _arg2: Int, _arg3: Int) -> Int) { }
+                |
+                |fun baz(arg1: Int,
+                |        arg2: Int,
+                |        arg3: Int) { }
+            """.trimMargin(),
+            LintError(3, 8, ruleId, "${WRONG_NEWLINES.warnText()} first parameter should be placed on a separate line or all other parameters " +
+                    "should be aligned with it in declaration of <bar>", true),
+            LintError(3, 8, ruleId, "${WRONG_NEWLINES.warnText()} value parameters should be placed on different lines in declaration of <bar>", true)
+        )
+    }
+
+    @Test
+    @Tag(WarningNames.WRONG_NEWLINES)
+    fun `should suggest newlines in a long supertype list`() {
+        lintMethod(
+            """
+                |class Foo :
+                |    FooBase<Bar>(),
+                |    BazInterface,
+                |    BazSuperclass { }
+                |    
+                |class Foo : FooBase<Bar>(), BazInterface,
+                |    BazSuperclass { }
+                |
+                |class Foo : FooBase<Bar>(), BazInterface, BazSuperclass { }
+                |
+                |class Foo : FooBase<Bar>() { }
+            """.trimMargin(),
+            LintError(6, 13, ruleId, "${WRONG_NEWLINES.warnText()} supertype list entries should be placed on different lines in declaration of <Foo>", true),
+            LintError(9, 13, ruleId, "${WRONG_NEWLINES.warnText()} supertype list entries should be placed on different lines in declaration of <Foo>", true)
+        )
+    }
 }
diff --git a/diktat-rules/src/test/resources/test/paragraph3/newlines/ParameterListExpected.kt b/diktat-rules/src/test/resources/test/paragraph3/newlines/ParameterListExpected.kt
@@ -0,0 +1,26 @@
+package test.paragraph3.newlines
+
+fun bar(
+arg1: Int,
+ arg2: Int,
+ arg3: Int) { }
+
+class Foo : FooBase<Bar>(),
+ BazInterface,
+ BazSuperclass { }
+
+class Foo(val arg1: Int, arg2: Int) { }
+
+class Foo(
+val arg1: Int,
+ arg2: Int,
+ arg3: Int) {
+    constructor(
+arg1: Int,
+ arg2: String,
+ arg3: String) : this(arg1, 0, 0) { }
+}
+
+class Foo(val arg1: Int,
+          var arg2: Int,
+          arg3: Int) { }
diff --git a/diktat-rules/src/test/resources/test/paragraph3/newlines/ParameterListTest.kt b/diktat-rules/src/test/resources/test/paragraph3/newlines/ParameterListTest.kt
@@ -0,0 +1,15 @@
+package test.paragraph3.newlines
+
+fun bar(arg1: Int, arg2: Int, arg3: Int) { }
+
+class Foo : FooBase<Bar>(), BazInterface, BazSuperclass { }
+
+class Foo(val arg1: Int, arg2: Int) { }
+
+class Foo(val arg1: Int, arg2: Int, arg3: Int) {
+    constructor(arg1: Int, arg2: String, arg3: String) : this(arg1, 0, 0) { }
+}
+
+class Foo(val arg1: Int,
+          var arg2: Int,
+          arg3: Int) { }