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

diktat-analysis.yml

@@ -322,6 +322,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

@@ -190,5 +190,8 @@ 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"
 }

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

@@ -116,6 +116,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"),
     ;
 

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

@@ -8,6 +8,7 @@ import org.cqfn.diktat.common.config.rules.RulesConfigReader
 import org.cqfn.diktat.ruleset.constants.Warnings
 import org.cqfn.diktat.ruleset.rules.calculations.AccurateCalculationsRule
 import org.cqfn.diktat.ruleset.rules.classes.DataClassesRule
+import org.cqfn.diktat.ruleset.rules.classes.SingleConstructorRule
 import org.cqfn.diktat.ruleset.rules.comments.CommentsRule
 import org.cqfn.diktat.ruleset.rules.comments.HeaderCommentRule
 import org.cqfn.diktat.ruleset.rules.files.BlankLinesRule
@@ -79,12 +80,13 @@ class DiktatRuleSetProvider(private val diktatConfigFile: String = "diktat-analy
                 ::FunctionLength,
                 ::LambdaParameterOrder,
                 ::FunctionArgumentsSize,
+                ::SingleConstructorRule,
                 ::BlankLinesRule,
                 ::NullableTypeRule,
-                ::WhiteSpaceRule,
                 ::WhenMustHaveElseRule,
                 ::ImmutableValNoVarRule,
                 ::AvoidNestedFunctionsRule,
+                ::WhiteSpaceRule,
                 ::FileStructureRule,  // this rule should be right before indentation because it should operate on already valid code
                 ::NewlinesRule,  // newlines need to be inserted right before fixing indentation
                 ::IndentationRule  // indentation rule should be the last because it fixes formatting after all the changes done by previous rules

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

@@ -0,0 +1,151 @@
+package org.cqfn.diktat.ruleset.rules.classes
+
+import com.pinterest.ktlint.core.Rule
+import com.pinterest.ktlint.core.ast.ElementType
+import com.pinterest.ktlint.core.ast.ElementType.CLASS
+import com.pinterest.ktlint.core.ast.ElementType.CLASS_BODY
+import com.pinterest.ktlint.core.ast.ElementType.LBRACE
+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.cqfn.diktat.common.config.rules.RulesConfig
+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.findAllNodesWithSpecificType
+import org.cqfn.diktat.ruleset.utils.getAllChildrenWithType
+import org.cqfn.diktat.ruleset.utils.getIdentifierName
+import org.cqfn.diktat.ruleset.utils.isGoingAfter
+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.KtProperty
+import org.jetbrains.kotlin.psi.KtSecondaryConstructor
+import org.jetbrains.kotlin.psi.KtThisExpression
+import org.jetbrains.kotlin.psi.psiUtil.asAssignment
+
+class SingleConstructorRule(private val config: List<RulesConfig>) : Rule("single-constructor") {
+    private lateinit var emitWarn: ((offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit)
+    private var isFixMode: Boolean = false
+    private val kotlinParser by lazy { KotlinParser() }
+
+    override fun visit(
+        node: ASTNode,
+        autoCorrect: Boolean,
+        emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit
+    ) {
+        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.
+     * - Create primary constructor moving all properties that we collected.
+     * - Create init block with other statements from the secondary constructor.
+     * - Finally, remove the secondary constructor.
+     */
+    @Suppress("UnsafeCallOnNullableType")
+    private fun convertConstructorToPrimary(classNode: ASTNode, secondaryCtor: ASTNode) {
+        val (assignments, otherStatements) = (secondaryCtor.psi as KtSecondaryConstructor)
+            .bodyBlockExpression
+            ?.statements
+            ?.partition { (it as? KtBinaryExpression)?.asAssignment() != null }
+            ?: emptyList<KtExpression>() to emptyList()
+
+        val (qualifiedAssignedProperties, assignedProperties) = assignments
+            .map {
+                // non-null assert is safe because of predicate in partitioning
+                (it as KtBinaryExpression).asAssignment()!!.left!!
+            }
+            .filter {
+                // todo use secondaryCtor.findAllVariablesWithAssignments()
+                (it as? KtDotQualifiedExpression)?.run {
+                    receiverExpression is KtThisExpression && selectorExpression is KtNameReferenceExpression
+                } ?: false ||
+                        it is KtNameReferenceExpression
+            }
+            .partition { it is KtDotQualifiedExpression }
+            .let { (qualified, simple) ->
+                qualified.map { (it as KtDotQualifiedExpression).selectorExpression as KtNameReferenceExpression } to
+                        simple.map { it as KtNameReferenceExpression }
+            }
+
+        val assignedClassProperties = qualifiedAssignedProperties + assignedProperties
+            .run {
+                val localProperties = secondaryCtor.findAllNodesWithSpecificType(ElementType.PROPERTY)
+                filterNot { reference ->
+                    // check for shadowing
+                    localProperties.any {
+                        reference.node.isGoingAfter(it) && (it.psi as KtProperty).name == reference.name
+                    }
+                }
+            }
+
+        // find properties declarations
+        val declarationsAssignedInCtor = assignedClassProperties
+            .mapNotNull { reference ->
+                (classNode.psi as KtClass).getProperties()
+                    .firstOrNull { it.nameIdentifier?.text == reference.getReferencedName() }
+            }
+            .distinct()
+
+        classNode.convertSecondaryConstructorToPrimary(secondaryCtor, declarationsAssignedInCtor, otherStatements)
+    }
+
+    @Suppress("UnsafeCallOnNullableType", "NestedBlockDepth")
+    private fun ASTNode.convertSecondaryConstructorToPrimary(secondaryCtor: ASTNode,
+                                                             declarationsAssignedInCtor: List<KtProperty>,
+                                                             otherStatements: List<KtExpression>) {
+        require(elementType == CLASS)
+
+        val primaryCtorNode = kotlinParser.createPrimaryConstructor("(${declarationsAssignedInCtor.joinToString(", ") { it.text }})").node
+        addChild(primaryCtorNode, findChildByType(CLASS_BODY))
+        declarationsAssignedInCtor.forEach { ktProperty ->
+            ktProperty.node.let { treeParent.removeChild(it) }
+        }
+
+        if (otherStatements.isNotEmpty()) {
+            findChildByType(CLASS_BODY)?.run {
+                val beforeNode = findChildByType(LBRACE)!!.let { if (it.treeNext.elementType == WHITE_SPACE) it.treeNext else it }
+                val classInitializer = kotlinParser.createNode(
+                    """
+                    |init {
+                    |    ${otherStatements.joinToString("\n") { it.text }}
+                    |}
+                """.trimMargin()
+                )
+                addChild(PsiWhiteSpaceImpl("\n"), beforeNode)
+                addChild(classInitializer, beforeNode)
+            }
+        }
+
+        findChildByType(CLASS_BODY)?.removeChild(secondaryCtor)
+    }
+}

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

@@ -321,6 +321,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

@@ -323,6 +323,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/test/kotlin/org/cqfn/diktat/ruleset/chapter6/SingleConstructorRuleFixTest.kt

@@ -0,0 +1,21 @@
+package org.cqfn.diktat.ruleset.chapter6
+
+import generated.WarningNames
+import org.cqfn.diktat.ruleset.rules.classes.SingleConstructorRule
+import org.cqfn.diktat.util.FixTestBase
+import org.junit.jupiter.api.Tag
+import org.junit.jupiter.api.Test
+
+class SingleConstructorRuleFixTest : FixTestBase("test/chapter6/classes", ::SingleConstructorRule) {
+    @Test
+    @Tag(WarningNames.SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY)
+    fun `should convert simple secondary constructor to primary`() {
+        fixAndCompare("SimpleConstructorExpected.kt", "SimpleConstructorTest.kt")
+    }
+
+    @Test
+    @Tag(WarningNames.SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY)
+    fun `should convert secondary constructor to a primary and init block`() {
+        fixAndCompare("ConstructorWithInitExpected.kt", "ConstructorWithInitTest.kt")
+    }
+}

diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter6/SingleConstructorRuleWarnTest.kt

@@ -0,0 +1,43 @@
+package org.cqfn.diktat.ruleset.chapter6
+
+import com.pinterest.ktlint.core.LintError
+import generated.WarningNames
+import org.cqfn.diktat.ruleset.constants.Warnings.SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY
+import org.cqfn.diktat.ruleset.rules.DIKTAT_RULE_SET_ID
+import org.cqfn.diktat.ruleset.rules.classes.SingleConstructorRule
+import org.cqfn.diktat.util.LintTestBase
+import org.junit.jupiter.api.Tag
+import org.junit.jupiter.api.Test
+
+class SingleConstructorRuleWarnTest: LintTestBase(::SingleConstructorRule) {
+    private val ruleId = "$DIKTAT_RULE_SET_ID:single-constructor"
+
+    @Test
+    @Tag(WarningNames.SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY)
+    fun `should suggest to convert single constructor to primary - positive example`() {
+        lintMethod(
+            """
+                |class Test(var a: Int) { }
+                |
+                |class Test private constructor(var a: Int) { }
+            """.trimMargin()
+        )
+    }
+
+    @Test
+    @Tag(WarningNames.SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY)
+    fun `should suggest to convert single constructor to primary`() {
+        lintMethod(
+            """
+                |class Test { 
+                |    var a: Int
+                |
+                |    constructor(a: Int) {
+                |        this.a = a
+                |    }
+                |}
+            """.trimMargin(),
+            LintError(1, 1, ruleId, "${SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY.warnText()} in class <Test>", true)
+        )
+    }
+}