diff --git a/diktat-analysis.yml b/diktat-analysis.yml index b3d835e50c..5ab0f0885a 100644 --- a/diktat-analysis.yml +++ b/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 diff --git a/diktat-rules/src/main/kotlin/generated/WarningNames.kt b/diktat-rules/src/main/kotlin/generated/WarningNames.kt index 647e2ec6c8..159fe3eca0 100644 --- a/diktat-rules/src/main/kotlin/generated/WarningNames.kt +++ b/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 = diff --git a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/constants/Warnings.kt b/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/constants/Warnings.kt index 3d07080314..ef17b1f977 100644 --- a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/constants/Warnings.kt +++ b/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"), diff --git a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/DiktatRuleSetProvider.kt b/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/DiktatRuleSetProvider.kt index d9fb77976a..bae979dcec 100644 --- a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/DiktatRuleSetProvider.kt +++ b/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, diff --git a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/classes/SingleConstructorRule.kt b/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/classes/SingleConstructorRule.kt new file mode 100644 index 0000000000..952c0b9f41 --- /dev/null +++ b/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) : 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() to emptyList() + + val classProperties = (classNode.psi as KtClass).getProperties() + val localProperties = secondaryCtor.psi.collectDescendantsOfType { 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.associateWithAssignedReference(localProperties: List, classProperties: List) = + 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, + nonTrivialAssignments: Map, + otherStatements: List + ) { + require(elementType == CLASS) + + val localProperties = secondaryCtor.psi.collectDescendantsOfType { 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, + localProperties: List) = (secondaryCtor.psi as KtSecondaryConstructor) + .valueParameters.run { + val dependencies = nonTrivialAssignments + .flatMap { it.left!!.collectDescendantsOfType() } + .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, + valueParameters: List) = 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 +} diff --git a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/utils/KotlinParser.kt b/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/utils/KotlinParser.kt index 61946af6d3..7343c47394 100644 --- a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/utils/KotlinParser.kt +++ b/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 getModelAspect(aspect: Class): T? { + if (aspect == TreeAspect::class.java) { + val constructor = ReflectionFactory.getReflectionFactory().newConstructorForSerialization( + aspect, + Any::class.java.getDeclaredConstructor(*arrayOfNulls>(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 getModelAspect(aspect: Class): T? { - if (aspect == TreeAspect::class.java) { - val constructor = ReflectionFactory.getReflectionFactory().newConstructorForSerialization( - aspect, - Any::class.java.getDeclaredConstructor(*arrayOfNulls>(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)) { diff --git a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/utils/search/VariablesSearch.kt b/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/utils/search/VariablesSearch.kt index d98a41ead9..83ff050e6d 100644 --- a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/utils/search/VariablesSearch.kt +++ b/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) { diff --git a/diktat-rules/src/main/resources/diktat-analysis-huawei.yml b/diktat-rules/src/main/resources/diktat-analysis-huawei.yml index eb9b406278..0d2b898f74 100644 --- a/diktat-rules/src/main/resources/diktat-analysis-huawei.yml +++ b/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 diff --git a/diktat-rules/src/main/resources/diktat-analysis.yml b/diktat-rules/src/main/resources/diktat-analysis.yml index 827ca90ccd..d18ebca442 100644 --- a/diktat-rules/src/main/resources/diktat-analysis.yml +++ b/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 diff --git a/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter6/SingleConstructorRuleFixTest.kt b/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter6/SingleConstructorRuleFixTest.kt new file mode 100644 index 0000000000..1b0d96b0e4 --- /dev/null +++ b/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter6/SingleConstructorRuleFixTest.kt @@ -0,0 +1,40 @@ +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") + } + + @Test + @Tag(WarningNames.SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY) + fun `should convert secondary constructor to a primary saving modifiers`() { + fixAndCompare("ConstructorWithModifiersExpected.kt", "ConstructorWithModifiersTest.kt") + } + + @Test + @Tag(WarningNames.SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY) + fun `should keep custom assignments when converting secondary constructor`() { + fixAndCompare("ConstructorWithCustomAssignmentsExpected.kt", "ConstructorWithCustomAssignmentsTest.kt") + } + + + @Test + @Tag(WarningNames.SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY) + fun `should keep assignments and required local variables in an init block`() { + fixAndCompare("AssignmentWithLocalPropertyExpected.kt", "AssignmentWithLocalPropertyTest.kt") + } +} diff --git a/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter6/SingleConstructorRuleWarnTest.kt b/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter6/SingleConstructorRuleWarnTest.kt new file mode 100644 index 0000000000..8d1aaa7d21 --- /dev/null +++ b/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 ", true) + ) + } +} diff --git a/diktat-rules/src/test/resources/test/chapter6/classes/AssignmentWithLocalPropertyExpected.kt b/diktat-rules/src/test/resources/test/chapter6/classes/AssignmentWithLocalPropertyExpected.kt new file mode 100644 index 0000000000..8480318136 --- /dev/null +++ b/diktat-rules/src/test/resources/test/chapter6/classes/AssignmentWithLocalPropertyExpected.kt @@ -0,0 +1,10 @@ +package test.chapter6.classes + +class Foo (a: Int){ + val a: Int + + init { + val f = F(a) +this.a = f.foo() +} +} \ No newline at end of file diff --git a/diktat-rules/src/test/resources/test/chapter6/classes/AssignmentWithLocalPropertyTest.kt b/diktat-rules/src/test/resources/test/chapter6/classes/AssignmentWithLocalPropertyTest.kt new file mode 100644 index 0000000000..e873ad2071 --- /dev/null +++ b/diktat-rules/src/test/resources/test/chapter6/classes/AssignmentWithLocalPropertyTest.kt @@ -0,0 +1,10 @@ +package test.chapter6.classes + +class Foo { + val a: Int + + constructor(a: Int) { + val f = F(a) + this.a = f.foo() + } +} \ No newline at end of file diff --git a/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithCustomAssignmentsExpected.kt b/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithCustomAssignmentsExpected.kt new file mode 100644 index 0000000000..f77c436041 --- /dev/null +++ b/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithCustomAssignmentsExpected.kt @@ -0,0 +1,9 @@ +package test.chapter6.classes + +class Test (a: Int){ + var a: Int + + init { + this.a = a + 42 +} +} \ No newline at end of file diff --git a/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithCustomAssignmentsTest.kt b/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithCustomAssignmentsTest.kt new file mode 100644 index 0000000000..4119fdf3ce --- /dev/null +++ b/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithCustomAssignmentsTest.kt @@ -0,0 +1,9 @@ +package test.chapter6.classes + +class Test { + var a: Int + + constructor(a: Int) { + this.a = a + 42 + } +} \ No newline at end of file diff --git a/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithInitExpected.kt b/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithInitExpected.kt new file mode 100644 index 0000000000..42ec5599f4 --- /dev/null +++ b/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithInitExpected.kt @@ -0,0 +1,9 @@ +package test.chapter6.classes + +class Test (var a: Int){ + +init { + println("Lorem ipsum") +foo() +} +} \ No newline at end of file diff --git a/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithInitTest.kt b/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithInitTest.kt new file mode 100644 index 0000000000..76018e193f --- /dev/null +++ b/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithInitTest.kt @@ -0,0 +1,11 @@ +package test.chapter6.classes + +class Test { +var a: Int + +constructor(a: Int) { + println("Lorem ipsum") + foo() + this.a = a +} +} \ No newline at end of file diff --git a/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithModifiersExpected.kt b/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithModifiersExpected.kt new file mode 100644 index 0000000000..1cf51521c2 --- /dev/null +++ b/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithModifiersExpected.kt @@ -0,0 +1,4 @@ +package test.chapter6.classes + +class Test @Annotation private constructor (var a: Int){ +} \ No newline at end of file diff --git a/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithModifiersTest.kt b/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithModifiersTest.kt new file mode 100644 index 0000000000..c9278e5209 --- /dev/null +++ b/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithModifiersTest.kt @@ -0,0 +1,9 @@ +package test.chapter6.classes + +class Test { +var a: Int + +@Annotation private constructor(a: Int) { + this.a = a +} +} \ No newline at end of file diff --git a/diktat-rules/src/test/resources/test/chapter6/classes/SimpleConstructorExpected.kt b/diktat-rules/src/test/resources/test/chapter6/classes/SimpleConstructorExpected.kt new file mode 100644 index 0000000000..a59f3c0e47 --- /dev/null +++ b/diktat-rules/src/test/resources/test/chapter6/classes/SimpleConstructorExpected.kt @@ -0,0 +1,14 @@ +package test.chapter6.classes + +class Test (var a: Int){ +} + +class Test (var a: Int){ +} + +class Test (var a: Int){ + +init { + var a = 14 +} +} \ No newline at end of file diff --git a/diktat-rules/src/test/resources/test/chapter6/classes/SimpleConstructorTest.kt b/diktat-rules/src/test/resources/test/chapter6/classes/SimpleConstructorTest.kt new file mode 100644 index 0000000000..c34bcccb6b --- /dev/null +++ b/diktat-rules/src/test/resources/test/chapter6/classes/SimpleConstructorTest.kt @@ -0,0 +1,27 @@ +package test.chapter6.classes + +class Test { +var a: Int + +constructor(a: Int) { + this.a = a +} +} + +class Test { +var a: Int + +constructor(_a: Int) { + a = _a +} +} + +class Test { +var a: Int + +constructor(_a: Int) { + var a = 14 + a = _a + this.a = _a +} +} \ No newline at end of file diff --git a/diktat-rules/src/test/resources/test/smoke/src/main/kotlin/Example3Expected.kt b/diktat-rules/src/test/resources/test/smoke/src/main/kotlin/Example3Expected.kt index 6e371e65fa..715b3dfc0d 100644 --- a/diktat-rules/src/test/resources/test/smoke/src/main/kotlin/Example3Expected.kt +++ b/diktat-rules/src/test/resources/test/smoke/src/main/kotlin/Example3Expected.kt @@ -18,6 +18,6 @@ fun mains() { port = "8080" timeout = 100 } - .doRequest() + httpClient.doRequest() } diff --git a/diktat-rules/src/test/resources/test/smoke/src/main/kotlin/Example3Test.kt b/diktat-rules/src/test/resources/test/smoke/src/main/kotlin/Example3Test.kt index a53cb43d5d..3b78e1a839 100644 --- a/diktat-rules/src/test/resources/test/smoke/src/main/kotlin/Example3Test.kt +++ b/diktat-rules/src/test/resources/test/smoke/src/main/kotlin/Example3Test.kt @@ -1,10 +1,15 @@ package test.smoke.src.main.kotlin -class HttpClient(var name: String) { +class HttpClient { + var name: String var url: String = "" var port: String = "" var timeout = 0 + constructor(name: String) { + this.name = name + } + fun doRequest() {} } @@ -15,5 +20,5 @@ fun mains() { port = "8080" timeout = 100 } - .doRequest() + httpClient.doRequest() } \ No newline at end of file diff --git a/info/available-rules.md b/info/available-rules.md index cab2971467..ff8ef36a4f 100644 --- a/info/available-rules.md +++ b/info/available-rules.md @@ -91,6 +91,7 @@ | 5 | 5.2.1 | LAMBDA_IS_NOT_LAST_PARAMETER | Checks that lambda inside function parameters isn't in the end | no | - | | 5 | 5.2.2 | TOO_MANY_PARAMETERS | Check: if function contains more parameters than allowed | no | maxParameterListSize | | 5 | 5.2.3 | WRONG_OVERLOADING_FUNCTION_ARGUMENTS | Check: function has overloading instead use default arguments | no | -| +| 6 | 6.1.1 | SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY | Check: warns if there is only one secondary constructor in a class
Fix: converts it to a primary constructor | yes | no | Support more complicated logic of constructor conversion | | 6 | 6.1.2 | USE_DATA_CLASS | Check: if class can be made as data class | no | - | yes | | 6 | 6.1.4 | MULTIPLE_INIT_BLOCKS | Checks that classes have only one init block | yes | no | - | | 6 | 6.1.6 | CLASS_SHOULD_NOT_BE_ABSTRACT | Checks: if abstract class has any abstract method. If not, warns that class should not be abstract
Fix: deletes abstract modifier | yes | - | - | diff --git a/info/diktat-kotlin-coding-style-guide-en.md b/info/diktat-kotlin-coding-style-guide-en.md new file mode 100644 index 0000000000..e69de29bb2 diff --git a/info/guide/diktat-coding-convention.md b/info/guide/diktat-coding-convention.md index 9ce4d90433..728cca652f 100644 --- a/info/guide/diktat-coding-convention.md +++ b/info/guide/diktat-coding-convention.md @@ -1667,8 +1667,8 @@ private fun foo() { # 6. Classes, interfaces and functions ### 6.1 Classes -### Rule 6.1.1: Primary constructor should be defined implicitly in the declaration of the class. -In case class contains only one explicit constructor - it should be converted to implicit primary constructor. +### Rule 6.1.1: When a class has a single constructor, it should be defined as a primary constructor in the declaration of the class. +In case class contains only one explicit constructor - it should be converted to a primary constructor. **Invalid example**: ```kotlin @@ -1686,7 +1686,7 @@ class Test(var a: Int) { // ... } -// in case of any annotations or modifer used on constructor: +// in case of any annotations or modifiers used on constructor: class Test private constructor(var a: Int) { // ... } diff --git a/info/guide/guide-chapter-6.md b/info/guide/guide-chapter-6.md index c577b18bbc..1f3f403298 100644 --- a/info/guide/guide-chapter-6.md +++ b/info/guide/guide-chapter-6.md @@ -1,8 +1,8 @@ # 6. Classes, interfaces and functions ### 6.1 Classes -### Rule 6.1.1: Primary constructor should be defined implicitly in the declaration of the class. -In case class contains only one explicit constructor - it should be converted to implicit primary constructor. +### Rule 6.1.1: When a class has a single constructor, it should be defined as a primary constructor in the declaration of the class. +In case class contains only one explicit constructor - it should be converted to a primary constructor. **Invalid example**: ```kotlin @@ -20,7 +20,7 @@ class Test(var a: Int) { // ... } -// in case of any annotations or modifer used on constructor: +// in case of any annotations or modifiers used on constructor: class Test private constructor(var a: Int) { // ... }