Rule 6.1.1 Primary constructor should be defined implicitly in the declaration of class

diktat-analysis.yml

@@ -326,6 +326,10 @@
 - name: KDOC_NO_CONSTRUCTOR_PROPERTY
   enabled: true
   configuration: {}
+# if a class has single constructor, it should be converted to a primary constructor
+- name: SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY
+  enabled: true
+  configuration: {}
 # Checks if class can be made as data class
 - name: USE_DATA_CLASS
   enabled: true

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

@@ -192,6 +192,9 @@ public object WarningNames {
     public const val WRONG_OVERLOADING_FUNCTION_ARGUMENTS: String =
             "WRONG_OVERLOADING_FUNCTION_ARGUMENTS"
 
+    public const val SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY: String =
+            "SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY"
+
     public const val USE_DATA_CLASS: String = "USE_DATA_CLASS"
 
     public const val WRONG_NAME_OF_VARIABLE_INSIDE_ACCESSOR: String =

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

@@ -119,6 +119,7 @@ enum class Warnings(private val canBeAutoCorrected: Boolean, private val warn: S
     WRONG_OVERLOADING_FUNCTION_ARGUMENTS(false, "use default argument instead of function overloading"),
 
     // ======== chapter 6 ========
+    SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY(true, "if a class has single constructor, it should be converted to a primary constructor"),
     USE_DATA_CLASS(false, "this class can be converted to a data class"),
     WRONG_NAME_OF_VARIABLE_INSIDE_ACCESSOR(false, "Use `field` keyword instead of property name inside property accessors"),
     MULTIPLE_INIT_BLOCKS(true, "Avoid using multiple `init` blocks, this logic can be moved to constructors or properties declarations"),

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

@@ -9,6 +9,7 @@ import org.cqfn.diktat.ruleset.constants.Warnings
 import org.cqfn.diktat.ruleset.rules.calculations.AccurateCalculationsRule
 import org.cqfn.diktat.ruleset.rules.classes.AbstractClassesRule
 import org.cqfn.diktat.ruleset.rules.classes.DataClassesRule
+import org.cqfn.diktat.ruleset.rules.classes.SingleConstructorRule
 import org.cqfn.diktat.ruleset.rules.classes.SingleInitRule
 import org.cqfn.diktat.ruleset.rules.comments.CommentsRule
 import org.cqfn.diktat.ruleset.rules.comments.HeaderCommentRule
@@ -53,6 +54,7 @@ class DiktatRuleSetProvider(private val diktatConfigFile: String = "diktat-analy
         val rules = listOf(
                 // comments & documentation
                 ::CommentsRule,
+                ::SingleConstructorRule,  // this rule can add properties to a primary constructor, so should be before KdocComments
                 ::KdocComments,
                 ::KdocMethods,
                 ::KdocFormatting,

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

@@ -0,0 +1,222 @@
+package org.cqfn.diktat.ruleset.rules.classes
+
+import org.cqfn.diktat.common.config.rules.RulesConfig
+import org.cqfn.diktat.ruleset.constants.EmitType
+import org.cqfn.diktat.ruleset.constants.Warnings.SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY
+import org.cqfn.diktat.ruleset.utils.KotlinParser
+import org.cqfn.diktat.ruleset.utils.getAllChildrenWithType
+import org.cqfn.diktat.ruleset.utils.getIdentifierName
+import org.cqfn.diktat.ruleset.utils.isGoingAfter
+
+import com.pinterest.ktlint.core.Rule
+import com.pinterest.ktlint.core.ast.ElementType.CLASS
+import com.pinterest.ktlint.core.ast.ElementType.CLASS_BODY
+import com.pinterest.ktlint.core.ast.ElementType.MODIFIER_LIST
+import com.pinterest.ktlint.core.ast.ElementType.PRIMARY_CONSTRUCTOR
+import com.pinterest.ktlint.core.ast.ElementType.SECONDARY_CONSTRUCTOR
+import com.pinterest.ktlint.core.ast.ElementType.WHITE_SPACE
+import org.jetbrains.kotlin.com.intellij.lang.ASTNode
+import org.jetbrains.kotlin.com.intellij.psi.impl.source.tree.PsiWhiteSpaceImpl
+import org.jetbrains.kotlin.psi.KtBinaryExpression
+import org.jetbrains.kotlin.psi.KtClass
+import org.jetbrains.kotlin.psi.KtDotQualifiedExpression
+import org.jetbrains.kotlin.psi.KtExpression
+import org.jetbrains.kotlin.psi.KtNameReferenceExpression
+import org.jetbrains.kotlin.psi.KtParameter
+import org.jetbrains.kotlin.psi.KtProperty
+import org.jetbrains.kotlin.psi.KtSecondaryConstructor
+import org.jetbrains.kotlin.psi.KtThisExpression
+import org.jetbrains.kotlin.psi.psiUtil.asAssignment
+import org.jetbrains.kotlin.psi.psiUtil.collectDescendantsOfType
+
+/**
+ * This rule ensures that if a class has a single constructor, this constructor is primary.
+ * Secondary constructor is converted into primary, statements that are not assignments are moved into an `init` block.
+ */
+class SingleConstructorRule(private val config: List<RulesConfig>) : Rule("single-constructor") {
+    private var isFixMode: Boolean = false
+    private val kotlinParser by lazy { KotlinParser() }
+    private lateinit var emitWarn: EmitType
+
+    override fun visit(
+            node: ASTNode,
+            autoCorrect: Boolean,
+            emit: EmitType
+    ) {
+        emitWarn = emit
+        isFixMode = autoCorrect
+
+        if (node.elementType == CLASS) {
+            handleClassConstructors(node)
+        }
+    }
+
+    private fun handleClassConstructors(node: ASTNode) {
+        if (node.findChildByType(PRIMARY_CONSTRUCTOR) == null) {
+            // class has no primary constructor, need to count secondary constructors
+            node
+                    .findChildByType(CLASS_BODY)
+                    ?.getAllChildrenWithType(SECONDARY_CONSTRUCTOR)
+                    ?.singleOrNull()
+                    ?.let { secondaryCtor ->
+                        SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY.warnAndFix(
+                                config, emitWarn, isFixMode, "in class <${node.getIdentifierName()?.text}>",
+                                node.startOffset, node
+                        ) {
+                            convertConstructorToPrimary(node, secondaryCtor)
+                        }
+                    }
+        }
+    }
+
+    /**
+     * This method does the following:
+     * - Inside the single secondary constructor find all assignments.
+     * - Some of assigned values will have `this` qualifier, they are definitely class properties.
+     * - For other assigned variables that are not declared in the same scope we check if they are properties and whether they depend only on constructor parameters.
+     * - Create primary constructor moving all properties that we collected.
+     * - Create init block with other statements from the secondary constructor, including initialization of properties that require local variables or complex calls.
+     * - Finally, remove the secondary constructor.
+     */
+    @Suppress("GENERIC_VARIABLE_WRONG_DECLARATION")
+    private fun convertConstructorToPrimary(classNode: ASTNode, secondaryCtor: ASTNode) {
+        val secondaryCtorArguments = (secondaryCtor.psi as KtSecondaryConstructor).valueParameters
+
+        // split all statements into assignments and all other statements (including comments)
+        val (assignments, otherStatements) = (secondaryCtor.psi as KtSecondaryConstructor)
+                .bodyBlockExpression
+                ?.statements
+                ?.partition { it is KtBinaryExpression && it.asAssignment() != null }
+                ?.run { first.map { it as KtBinaryExpression } to second }
+                ?: emptyList<KtBinaryExpression>() to emptyList()
+
+        val classProperties = (classNode.psi as KtClass).getProperties()
+        val localProperties = secondaryCtor.psi.collectDescendantsOfType<KtProperty> { it.isLocal }
+        // find all references to class properties that are getting assigned in a constructor
+        val assignmentsToReferences = assignments.associateWithAssignedReference(localProperties, classProperties)
+
+        // Split all assignments into trivial (that are just assigned from a constructor parameter) and non-trivial.
+        // Logic for non-trivial assignments should than be kept and moved into a dedicated `init` block.
+        val (trivialAssignments, nonTrivialAssignments) = assignmentsToReferences
+                .toList()
+                .partition { (assignment, _) ->
+                    assignment.right.let { rhs ->
+                        rhs is KtNameReferenceExpression && rhs.getReferencedName() in secondaryCtorArguments.map { it.name }
+                    }
+                }
+                .let { it.first.toMap() to it.second.toMap() }
+
+        // find corresponding properties' declarations
+        val declarationsAssignedInCtor = trivialAssignments
+                .mapNotNull { (_, reference) ->
+                    (classNode.psi as KtClass).getProperties()
+                            .firstOrNull { it.nameIdentifier?.text == reference.getReferencedName() }
+                }
+                .distinct()
+
+        classNode.convertSecondaryConstructorToPrimary(secondaryCtor, declarationsAssignedInCtor, nonTrivialAssignments, otherStatements)
+    }
+
+    @Suppress("UnsafeCallOnNullableType")
+    private fun List<KtBinaryExpression>.associateWithAssignedReference(localProperties: List<KtProperty>, classProperties: List<KtProperty>) =
+            associateWith {
+                // non-null assert is safe because of predicate in partitioning
+                it.asAssignment()!!.left!!
+            }
+                    .filterValues { left ->
+                        // we keep only statements where property is referenced via this (like `this.foo = ...`)
+                        left is KtDotQualifiedExpression && left.receiverExpression is KtThisExpression && left.selectorExpression is KtNameReferenceExpression ||
+                                // or directly (like `foo = ...`)
+                                left is KtNameReferenceExpression && localProperties.none {
+                                    // check for shadowing
+                                    left.node.isGoingAfter(it.node) && it.name == left.name
+                                }
+                    }
+                    .mapValues { (_, left) ->
+                        when (left) {
+                            is KtDotQualifiedExpression -> left.selectorExpression as KtNameReferenceExpression
+                            is KtNameReferenceExpression -> left
+                            else -> error("Unexpected psi class ${left::class} with text ${left.text}")
+                        }
+                    }
+                    .filterValues { left -> left.getReferencedName() in classProperties.mapNotNull { it.name } }
+
+    @Suppress("NestedBlockDepth", "GENERIC_VARIABLE_WRONG_DECLARATION")
+    private fun ASTNode.convertSecondaryConstructorToPrimary(
+            secondaryCtor: ASTNode,
+            declarationsAssignedInCtor: List<KtProperty>,
+            nonTrivialAssignments: Map<KtBinaryExpression, KtNameReferenceExpression>,
+            otherStatements: List<KtExpression>
+    ) {
+        require(elementType == CLASS)
+
+        val localProperties = secondaryCtor.psi.collectDescendantsOfType<KtProperty> { it.isLocal }
+        // find all arguments that are not directly assigned into properties
+        val nonTrivialSecondaryCtorParameters = getNonTrivialParameters(secondaryCtor, nonTrivialAssignments.keys, localProperties)
+
+        val primaryCtorNode = createPrimaryCtor(secondaryCtor, declarationsAssignedInCtor, nonTrivialSecondaryCtorParameters)
+        addChild(primaryCtorNode, findChildByType(CLASS_BODY))
+        declarationsAssignedInCtor.forEach { ktProperty ->
+            ktProperty.node.run {
+                treePrev.takeIf { it.elementType == WHITE_SPACE }?.let { treeParent.removeChild(it) }
+                treeParent.removeChild(this)
+            }
+        }
+
+        if (otherStatements.isNotEmpty() || nonTrivialAssignments.isNotEmpty()) {
+            findChildByType(CLASS_BODY)?.run {
+                val classInitializer = kotlinParser.createNode(
+                        """
+                            |init {
+                            |    ${(otherStatements + nonTrivialAssignments.keys).joinToString("\n") { it.text }}
+                            |}
+                        """.trimMargin()
+                )
+                addChild(classInitializer, secondaryCtor)
+                addChild(PsiWhiteSpaceImpl("\n"), secondaryCtor)
+            }
+        }
+
+        secondaryCtor
+                .run { treePrev.takeIf { it.elementType == WHITE_SPACE } ?: treeNext }
+                .takeIf { it.elementType == WHITE_SPACE }
+                ?.run { treeParent.removeChild(this) }
+        findChildByType(CLASS_BODY)?.removeChild(secondaryCtor)
+    }
+
+    @Suppress("UnsafeCallOnNullableType")
+    private fun getNonTrivialParameters(secondaryCtor: ASTNode,
+                                        nonTrivialAssignments: Collection<KtBinaryExpression>,
+                                        localProperties: List<KtProperty>) = (secondaryCtor.psi as KtSecondaryConstructor)
+            .valueParameters.run {
+                val dependencies = nonTrivialAssignments
+                        .flatMap { it.left!!.collectDescendantsOfType<KtNameReferenceExpression>() }
+                        .filterNot { ref ->
+                            localProperties.any { ref.node.isGoingAfter(it.node) && ref.getReferencedName() == it.name }
+                        }
+                        .map { it.getReferencedName() }
+                filter {
+                    it.name in dependencies
+                }
+            }
+
+    private fun createPrimaryCtor(secondaryCtor: ASTNode,
+                                  declarationsAssignedInCtor: List<KtProperty>,
+                                  valueParameters: List<KtParameter>) = kotlinParser.createPrimaryConstructor(
+            (secondaryCtor
+                    .findChildByType(MODIFIER_LIST)
+                    ?.text
+                    ?.plus(" constructor ")
+                    ?: "") +
+                    "(" +
+                    declarationsAssignedInCtor.run {
+                        joinToString(
+                                ", ",
+                                postfix = if (isNotEmpty() && valueParameters.isNotEmpty()) ", " else ""
+                        ) { it.text }
+                    } +
+                    valueParameters.joinToString(", ") { it.text } +
+                    ")"
+    )
+            .node
+}

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

@@ -11,6 +11,7 @@ import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment
 import org.jetbrains.kotlin.com.intellij.lang.ASTNode
 import org.jetbrains.kotlin.com.intellij.mock.MockProject
 import org.jetbrains.kotlin.com.intellij.openapi.Disposable
+import org.jetbrains.kotlin.com.intellij.openapi.project.Project
 import org.jetbrains.kotlin.com.intellij.openapi.util.UserDataHolderBase
 import org.jetbrains.kotlin.com.intellij.pom.PomModel
 import org.jetbrains.kotlin.com.intellij.pom.PomModelAspect
@@ -25,6 +26,36 @@ import org.jetbrains.kotlin.resolve.ImportPath
 import sun.reflect.ReflectionFactory
 
 class KotlinParser {
+    private val project: Project by lazy {
+        val compilerConfiguration = CompilerConfiguration()
+        compilerConfiguration.put(CLIConfigurationKeys.MESSAGE_COLLECTOR_KEY, MessageCollector.NONE) // mute the output logging to process it themselves
+        val pomModel: PomModel = object : UserDataHolderBase(), PomModel {
+            override fun runTransaction(transaction: PomTransaction) {
+                (transaction as PomTransactionBase).run()
+            }
+
+            @Suppress("UNCHECKED_CAST")
+            override fun <T : PomModelAspect> getModelAspect(aspect: Class<T>): T? {
+                if (aspect == TreeAspect::class.java) {
+                    val constructor = ReflectionFactory.getReflectionFactory().newConstructorForSerialization(
+                        aspect,
+                        Any::class.java.getDeclaredConstructor(*arrayOfNulls<Class<*>>(0))
+                    )
+                    return constructor.newInstance() as T
+                }
+                return null
+            }
+        } // I don't really understand what's going on here, but thanks to this, you can use this node in the future
+        val project = KotlinCoreEnvironment.createForProduction(
+            Disposable {},
+            compilerConfiguration,
+            EnvironmentConfigFiles.JVM_CONFIG_FILES
+        ).project // create project
+        project as MockProject
+        project.registerService(PomModel::class.java, pomModel)
+        project
+    }
+    private val ktPsiFactory by lazy { KtPsiFactory(project, true) }
 
     /**
      * Set idea.io.use.nio2 in system property to true
@@ -38,6 +69,8 @@ class KotlinParser {
         return makeNode(text, isPackage) ?: throw KotlinParseException("Your text is not valid")
     }
 
+    fun createPrimaryConstructor(text: String) = ktPsiFactory.createPrimaryConstructor(text)
+
     /**
      * This method create a node based on text.
      * @param isPackage - flag to check if node will contains package.
@@ -47,35 +80,8 @@ class KotlinParser {
      */
     @Suppress("UnsafeCallOnNullableType")
     private fun makeNode(text: String, isPackage: Boolean = false): ASTNode? {
-        val compilerConfiguration = CompilerConfiguration()
-        compilerConfiguration.put(CLIConfigurationKeys.MESSAGE_COLLECTOR_KEY, MessageCollector.NONE) // mute the output logging to process it themselves
-        val pomModel: PomModel = object : UserDataHolderBase(), PomModel {
-            override fun runTransaction(transaction: PomTransaction) {
-                (transaction as PomTransactionBase).run()
-            }
-
-            @Suppress("UNCHECKED_CAST")
-            override fun <T : PomModelAspect> getModelAspect(aspect: Class<T>): T? {
-                if (aspect == TreeAspect::class.java) {
-                    val constructor = ReflectionFactory.getReflectionFactory().newConstructorForSerialization(
-                            aspect,
-                            Any::class.java.getDeclaredConstructor(*arrayOfNulls<Class<*>>(0))
-                    )
-                    return constructor.newInstance() as T
-                }
-                return null
-            }
-        } // I don't really understand what's going on here, but thanks to this, you can use this node in the future
-        val project = KotlinCoreEnvironment.createForProduction(
-                Disposable {},
-                compilerConfiguration,
-                EnvironmentConfigFiles.JVM_CONFIG_FILES
-        ).project // create project
-        project as MockProject
-        project.registerService(PomModel::class.java, pomModel)
         if (text.isEmpty())
             return null
-        val ktPsiFactory = KtPsiFactory(project, true)
         if (text.trim().isEmpty())
             return ktPsiFactory.createWhiteSpace(text).node
         var node: ASTNode = if (isPackage || isContainKDoc(text)) {

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

@@ -19,9 +19,9 @@ import org.jetbrains.kotlin.psi.psiUtil.parents
 import org.jetbrains.kotlin.psi.psiUtil.referenceExpression
 
 /**
- *  @param node - root node of a type File that is used to search all declared properties (variables)
+ *  @param node root node of a type File that is used to search all declared properties (variables)
  *  it should be ONLY node of File elementType
- *  @param filterForVariables - condition to filter
+ *  @param filterForVariables condition to filter
  */
 abstract class VariablesSearch(val node: ASTNode, private val filterForVariables: (KtProperty) -> Boolean) {
 

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

@@ -325,6 +325,10 @@
 - name: KDOC_NO_CONSTRUCTOR_PROPERTY
   enabled: true
   configuration: {}
+# if a class has single constructor, it should be converted to a primary constructor
+- name: SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY
+  enabled: true
+  configuration: {}
 # Checks if class can be made as data class
 - name: USE_DATA_CLASS
   enabled: true

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

@@ -327,6 +327,10 @@
 - name: KDOC_NO_CONSTRUCTOR_PROPERTY
   enabled: true
   configuration: {}
+# if a class has single constructor, it should be converted to a primary constructor
+- name: SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY
+  enabled: true
+  configuration: {}
 # Checks if class can be made as data class
 - name: USE_DATA_CLASS
   enabled: true