From 63dda51358e77b2692343849292c9de170b92fb7 Mon Sep 17 00:00:00 2001 From: Peter Trifanov Date: Tue, 20 Oct 2020 14:47:05 +0300 Subject: [PATCH 01/13] Rule 6.1.1 ### What's done: * Refactoring --- .../src/main/kotlin/generated/WarningNames.kt | 3 + .../cqfn/diktat/ruleset/constants/Warnings.kt | 3 + .../ruleset/rules/DiktatRuleSetProvider.kt | 4 +- .../rules/classes/SingleConstructorRule.kt | 122 ++++++++++++++++++ .../ruleset/utils/search/VariablesSearch.kt | 4 +- .../chapter6/SingleConstructorRuleFixTest.kt | 23 ++++ .../chapter6/SingleConstructorRuleWarnTest.kt | 43 ++++++ .../classes/SimpleConstructorExpected.kt | 19 +++ .../chapter6/classes/SimpleConstructorTest.kt | 27 ++++ 9 files changed, 245 insertions(+), 3 deletions(-) create mode 100644 diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/classes/SingleConstructorRule.kt create mode 100644 diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter6/SingleConstructorRuleFixTest.kt create mode 100644 diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter6/SingleConstructorRuleWarnTest.kt create mode 100644 diktat-rules/src/test/resources/test/chapter6/classes/SimpleConstructorExpected.kt create mode 100644 diktat-rules/src/test/resources/test/chapter6/classes/SimpleConstructorTest.kt diff --git a/diktat-rules/src/main/kotlin/generated/WarningNames.kt b/diktat-rules/src/main/kotlin/generated/WarningNames.kt index 00e0d51ce7..786f87b4ca 100644 --- a/diktat-rules/src/main/kotlin/generated/WarningNames.kt +++ b/diktat-rules/src/main/kotlin/generated/WarningNames.kt @@ -189,4 +189,7 @@ 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" } 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 582d570d1b..14cdb883c6 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 @@ -114,6 +114,9 @@ enum class Warnings(private val canBeAutoCorrected: Boolean, private val warn: S TOO_MANY_PARAMETERS(false, "function has too many parameters"), NESTED_BLOCK(false, "function has too many nested blocks and should be simplified"), 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"), ; /** 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 cabeb6afde..946deff0cf 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 @@ -4,6 +4,7 @@ import com.pinterest.ktlint.core.RuleSet import com.pinterest.ktlint.core.RuleSetProvider import org.cqfn.diktat.common.config.rules.RulesConfigReader import org.cqfn.diktat.ruleset.rules.calculations.AccurateCalculationsRule +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 @@ -61,12 +62,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 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..d99aad96a4 --- /dev/null +++ b/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/classes/SingleConstructorRule.kt @@ -0,0 +1,122 @@ +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.COMMA +import com.pinterest.ktlint.core.ast.ElementType.LPAR +import com.pinterest.ktlint.core.ast.ElementType.PRIMARY_CONSTRUCTOR +import com.pinterest.ktlint.core.ast.ElementType.RPAR +import com.pinterest.ktlint.core.ast.ElementType.SECONDARY_CONSTRUCTOR +import com.pinterest.ktlint.core.ast.ElementType.VALUE_PARAMETER_LIST +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.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.CompositeElement +import org.jetbrains.kotlin.com.intellij.psi.impl.source.tree.LeafPsiElement +import org.jetbrains.kotlin.psi.KtBinaryExpression +import org.jetbrains.kotlin.psi.KtClass +import org.jetbrains.kotlin.psi.KtDotQualifiedExpression +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) : Rule("single-constructor") { + private lateinit var emitWarn: ((offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit) + private var isFixMode: Boolean = false + + 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 + ) { + // 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 will check later if they are properties. + val (qualifiedAssignedProperties, assignedProperties) = (secondaryCtor.psi as KtSecondaryConstructor).bodyBlockExpression + ?.statements + ?.mapNotNull { (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 } + } + ?: (emptyList() to emptyList()) + + + 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 -> + (node.psi as KtClass).getProperties() + .firstOrNull { it.nameIdentifier?.text == reference.getReferencedName() } + } + .distinct() + + // move them; todo: with their initial values (unless they are `lateinit`) to the primary + val primaryCtorNode = CompositeElement(PRIMARY_CONSTRUCTOR) + node.addChild(primaryCtorNode, node.findChildByType(CLASS_BODY)) + val valueParameterList = CompositeElement(VALUE_PARAMETER_LIST) + primaryCtorNode.addChild(valueParameterList) + valueParameterList.addChild(LeafPsiElement(LPAR, "(")) + declarationsAssignedInCtor.forEachIndexed { index, ktProperty -> + valueParameterList.addChild(ktProperty.node.clone() as ASTNode, null) + if (index != declarationsAssignedInCtor.size - 1) { + valueParameterList.addChild(LeafPsiElement(COMMA, ","), null) + } + ktProperty.node.run { treeParent.removeChild(this) } + } + valueParameterList.addChild(LeafPsiElement(RPAR, ")")) + + node.findChildByType(CLASS_BODY)?.removeChild(secondaryCtor) + + // move all other operations to init block + } + } + } + } +} 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/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..964d274378 --- /dev/null +++ b/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter6/SingleConstructorRuleFixTest.kt @@ -0,0 +1,23 @@ +package org.cqfn.diktat.ruleset.chapter6 + +import generated.WarningNames +import org.cqfn.diktat.util.FixTestBase +import org.cqfn.diktat.ruleset.rules.classes.SingleConstructorRule +import org.junit.jupiter.api.Disabled +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) + @Disabled("Not implemented yet") + fun `should convert secondary constructor to a primary and init block`() { + TODO() + } +} \ No newline at end of file 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/SimpleConstructorExpected.kt b/diktat-rules/src/test/resources/test/chapter6/classes/SimpleConstructorExpected.kt new file mode 100644 index 0000000000..d4d9f8d8d3 --- /dev/null +++ b/diktat-rules/src/test/resources/test/chapter6/classes/SimpleConstructorExpected.kt @@ -0,0 +1,19 @@ +package test.chapter6.classes + +class Test (var a: Int){ + + + +} + +class Test (var a: Int){ + + + +} + +class Test (var a: Int){ + + + +} \ 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 From e0e02de3000cdbf7cdda7b75c45c6c21bc2bc4ec Mon Sep 17 00:00:00 2001 From: Peter Trifanov Date: Tue, 20 Oct 2020 18:17:40 +0300 Subject: [PATCH 02/13] Rule 6.1.1 ### What's done: * Refactoring --- diktat-analysis.yml | 3 +++ diktat-rules/src/main/resources/diktat-analysis-huawei.yml | 3 +++ diktat-rules/src/main/resources/diktat-analysis.yml | 3 +++ info/available-rules.md | 1 + 4 files changed, 10 insertions(+) diff --git a/diktat-analysis.yml b/diktat-analysis.yml index e3f311d5c0..c323f8e4ec 100644 --- a/diktat-analysis.yml +++ b/diktat-analysis.yml @@ -317,5 +317,8 @@ configuration: {} # Checks that property in constructor doesn't contains comment - name: KDOC_NO_CONSTRUCTOR_PROPERTY + enabled: true + configuration: {} +- name: "SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY" enabled: true configuration: {} \ No newline at end of file diff --git a/diktat-rules/src/main/resources/diktat-analysis-huawei.yml b/diktat-rules/src/main/resources/diktat-analysis-huawei.yml index 5c7e89d7fe..005154a3d1 100644 --- a/diktat-rules/src/main/resources/diktat-analysis-huawei.yml +++ b/diktat-rules/src/main/resources/diktat-analysis-huawei.yml @@ -314,5 +314,8 @@ configuration: {} # Checks that property in constructor doesn't contains comment - name: KDOC_NO_CONSTRUCTOR_PROPERTY + enabled: true + configuration: {} +- name: "SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY" enabled: true configuration: {} \ No newline at end of file diff --git a/diktat-rules/src/main/resources/diktat-analysis.yml b/diktat-rules/src/main/resources/diktat-analysis.yml index 98a236b47c..f16e0134fa 100644 --- a/diktat-rules/src/main/resources/diktat-analysis.yml +++ b/diktat-rules/src/main/resources/diktat-analysis.yml @@ -317,5 +317,8 @@ configuration: {} # Checks that property in constructor doesn't contains comment - name: KDOC_NO_CONSTRUCTOR_PROPERTY + enabled: true + configuration: {} +- name: "SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY" enabled: true configuration: {} \ No newline at end of file diff --git a/info/available-rules.md b/info/available-rules.md index 4847da44d3..f2af7afa6a 100644 --- a/info/available-rules.md +++ b/info/available-rules.md @@ -90,3 +90,4 @@ | 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 | \ No newline at end of file From 675f36af4d9b801e64128d660767804c4aaacba9 Mon Sep 17 00:00:00 2001 From: Peter Trifanov Date: Wed, 21 Oct 2020 13:24:43 +0300 Subject: [PATCH 03/13] Rule 6.1.1 ### What's done: * Create init block for other statements * Refactoring --- .../rules/classes/SingleConstructorRule.kt | 152 +++++++++++------- .../chapter6/SingleConstructorRuleFixTest.kt | 6 +- .../classes/ConstructorWithInitExpected.kt | 11 ++ .../classes/ConstructorWithInitTest.kt | 11 ++ .../classes/SimpleConstructorExpected.kt | 3 + 5 files changed, 125 insertions(+), 58 deletions(-) create mode 100644 diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithInitExpected.kt create mode 100644 diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithInitTest.kt 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 index d99aad96a4..f1d6823049 100644 --- 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 @@ -5,23 +5,30 @@ 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.COMMA +import com.pinterest.ktlint.core.ast.ElementType.LBRACE import com.pinterest.ktlint.core.ast.ElementType.LPAR import com.pinterest.ktlint.core.ast.ElementType.PRIMARY_CONSTRUCTOR +import com.pinterest.ktlint.core.ast.ElementType.RBRACE import com.pinterest.ktlint.core.ast.ElementType.RPAR import com.pinterest.ktlint.core.ast.ElementType.SECONDARY_CONSTRUCTOR import com.pinterest.ktlint.core.ast.ElementType.VALUE_PARAMETER_LIST +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.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.BlockExpressionElementType import org.jetbrains.kotlin.com.intellij.lang.ASTNode import org.jetbrains.kotlin.com.intellij.psi.impl.source.tree.CompositeElement 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.lexer.KtTokens 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 @@ -55,68 +62,105 @@ class SingleConstructorRule(private val config: List) : Rule("singl ?.let { secondaryCtor -> SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY.warnAndFix( config, emitWarn, isFixMode, "in class <${node.getIdentifierName()?.text}>", - node.startOffset, node - ) { - // 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 will check later if they are properties. - val (qualifiedAssignedProperties, assignedProperties) = (secondaryCtor.psi as KtSecondaryConstructor).bodyBlockExpression - ?.statements - ?.mapNotNull { (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 } - } - ?: (emptyList() to emptyList()) + node.startOffset, node) { + convertConstructorToPrimary(node, secondaryCtor) + } + } + } + } + + @Suppress("UnsafeCallOnNullableType") + private fun convertConstructorToPrimary(classNode: ASTNode, secondaryCtor: ASTNode) { + // 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 will check later if they are properties. + + val (assignments, otherStatements) = (secondaryCtor.psi as KtSecondaryConstructor).bodyBlockExpression + ?.statements + ?.partition { (it as? KtBinaryExpression)?.asAssignment() != null } + ?: emptyList() 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 - } - } - } + 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() - // find properties declarations - val declarationsAssignedInCtor = assignedClassProperties - .mapNotNull { reference -> - (node.psi as KtClass).getProperties() - .firstOrNull { it.nameIdentifier?.text == reference.getReferencedName() } - } - .distinct() + // move them; todo: with their initial values (unless they are `lateinit`) to the primary + classNode.convertSecondaryConstructorToPrimary(secondaryCtor, declarationsAssignedInCtor, otherStatements) + } - // move them; todo: with their initial values (unless they are `lateinit`) to the primary - val primaryCtorNode = CompositeElement(PRIMARY_CONSTRUCTOR) - node.addChild(primaryCtorNode, node.findChildByType(CLASS_BODY)) - val valueParameterList = CompositeElement(VALUE_PARAMETER_LIST) - primaryCtorNode.addChild(valueParameterList) - valueParameterList.addChild(LeafPsiElement(LPAR, "(")) - declarationsAssignedInCtor.forEachIndexed { index, ktProperty -> - valueParameterList.addChild(ktProperty.node.clone() as ASTNode, null) - if (index != declarationsAssignedInCtor.size - 1) { - valueParameterList.addChild(LeafPsiElement(COMMA, ","), null) - } - ktProperty.node.run { treeParent.removeChild(this) } - } - valueParameterList.addChild(LeafPsiElement(RPAR, ")")) + private fun ASTNode.convertSecondaryConstructorToPrimary(secondaryCtor: ASTNode, + declarationsAssignedInCtor: List, + otherStatements: List) { + require(elementType == CLASS) - node.findChildByType(CLASS_BODY)?.removeChild(secondaryCtor) + val primaryCtorNode = CompositeElement(PRIMARY_CONSTRUCTOR) + addChild(primaryCtorNode, findChildByType(CLASS_BODY)) + val valueParameterList = CompositeElement(VALUE_PARAMETER_LIST) + primaryCtorNode.addChild(valueParameterList) + valueParameterList.addChild(LeafPsiElement(LPAR, "(")) + declarationsAssignedInCtor.forEachIndexed { index, ktProperty -> + valueParameterList.addChild(ktProperty.node.clone() as ASTNode, null) + if (index != declarationsAssignedInCtor.size - 1) { + valueParameterList.addChild(LeafPsiElement(COMMA, ","), null) + } + ktProperty.node.run { treeParent.removeChild(this) } + } + valueParameterList.addChild(LeafPsiElement(RPAR, ")")) - // move all other operations to init block + if (otherStatements.isNotEmpty()) { + findChildByType(CLASS_BODY) + ?.run { + val beforeNode = + findChildByType(LBRACE)!!.let { if (it.treeNext.elementType == WHITE_SPACE) it.treeNext else it } + val classInitializer = CompositeElement(ElementType.CLASS_INITIALIZER) + addChild(PsiWhiteSpaceImpl("\n"), beforeNode) + addChild(classInitializer, beforeNode) + classInitializer.addChild(LeafPsiElement(ElementType.INIT_KEYWORD, KtTokens.INIT_KEYWORD.value)) + val initBlock = BlockExpressionElementType().createCompositeNode() + classInitializer.addChild(initBlock) + initBlock.addChild(LeafPsiElement(LBRACE, "{")) + otherStatements.forEach { + initBlock.addChild(PsiWhiteSpaceImpl("\n")) + initBlock.addChild(it.node.clone() as ASTNode) } + initBlock.addChild(PsiWhiteSpaceImpl("\n")) + initBlock.addChild(LeafPsiElement(RBRACE, "}")) } } + + findChildByType(CLASS_BODY)?.removeChild(secondaryCtor) } } 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 index 964d274378..dc926dec85 100644 --- 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 @@ -1,9 +1,8 @@ package org.cqfn.diktat.ruleset.chapter6 import generated.WarningNames -import org.cqfn.diktat.util.FixTestBase import org.cqfn.diktat.ruleset.rules.classes.SingleConstructorRule -import org.junit.jupiter.api.Disabled +import org.cqfn.diktat.util.FixTestBase import org.junit.jupiter.api.Tag import org.junit.jupiter.api.Test @@ -16,8 +15,7 @@ class SingleConstructorRuleFixTest : FixTestBase("test/chapter6/classes", ::Sing @Test @Tag(WarningNames.SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY) - @Disabled("Not implemented yet") fun `should convert secondary constructor to a primary and init block`() { - TODO() + fixAndCompare("ConstructorWithInitExpected.kt", "ConstructorWithInitTest.kt") } } \ 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..2b75ef588f --- /dev/null +++ b/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithInitExpected.kt @@ -0,0 +1,11 @@ +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/SimpleConstructorExpected.kt b/diktat-rules/src/test/resources/test/chapter6/classes/SimpleConstructorExpected.kt index d4d9f8d8d3..ede03d6c09 100644 --- a/diktat-rules/src/test/resources/test/chapter6/classes/SimpleConstructorExpected.kt +++ b/diktat-rules/src/test/resources/test/chapter6/classes/SimpleConstructorExpected.kt @@ -13,6 +13,9 @@ class Test (var a: Int){ } class Test (var a: Int){ +init{ +var a = 14 +} From 5d2290781733da7ebcd76574f6e376840b714d7c Mon Sep 17 00:00:00 2001 From: Peter Trifanov Date: Wed, 21 Oct 2020 13:34:46 +0300 Subject: [PATCH 04/13] Rule 6.1.1 ### What's done: * Use KotlinParser --- .../rules/classes/SingleConstructorRule.kt | 72 ++++++++----------- .../cqfn/diktat/ruleset/utils/KotlinParser.kt | 60 +++++++++------- .../classes/ConstructorWithInitExpected.kt | 4 +- .../classes/SimpleConstructorExpected.kt | 4 +- 4 files changed, 65 insertions(+), 75 deletions(-) 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 index f1d6823049..301d704202 100644 --- 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 @@ -4,27 +4,19 @@ 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.COMMA import com.pinterest.ktlint.core.ast.ElementType.LBRACE -import com.pinterest.ktlint.core.ast.ElementType.LPAR import com.pinterest.ktlint.core.ast.ElementType.PRIMARY_CONSTRUCTOR -import com.pinterest.ktlint.core.ast.ElementType.RBRACE -import com.pinterest.ktlint.core.ast.ElementType.RPAR import com.pinterest.ktlint.core.ast.ElementType.SECONDARY_CONSTRUCTOR -import com.pinterest.ktlint.core.ast.ElementType.VALUE_PARAMETER_LIST 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.BlockExpressionElementType import org.jetbrains.kotlin.com.intellij.lang.ASTNode -import org.jetbrains.kotlin.com.intellij.psi.impl.source.tree.CompositeElement -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.lexer.KtTokens import org.jetbrains.kotlin.psi.KtBinaryExpression import org.jetbrains.kotlin.psi.KtClass import org.jetbrains.kotlin.psi.KtDotQualifiedExpression @@ -38,6 +30,7 @@ import org.jetbrains.kotlin.psi.psiUtil.asAssignment class SingleConstructorRule(private val config: List) : 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, @@ -69,13 +62,19 @@ class SingleConstructorRule(private val config: List) : Rule("singl } } + /** + * 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) { - // 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 will check later if they are properties. - - val (assignments, otherStatements) = (secondaryCtor.psi as KtSecondaryConstructor).bodyBlockExpression + val (assignments, otherStatements) = (secondaryCtor.psi as KtSecondaryConstructor) + .bodyBlockExpression ?.statements ?.partition { (it as? KtBinaryExpression)?.asAssignment() != null } ?: emptyList() to emptyList() @@ -117,7 +116,6 @@ class SingleConstructorRule(private val config: List) : Rule("singl } .distinct() - // move them; todo: with their initial values (unless they are `lateinit`) to the primary classNode.convertSecondaryConstructorToPrimary(secondaryCtor, declarationsAssignedInCtor, otherStatements) } @@ -126,39 +124,25 @@ class SingleConstructorRule(private val config: List) : Rule("singl otherStatements: List) { require(elementType == CLASS) - val primaryCtorNode = CompositeElement(PRIMARY_CONSTRUCTOR) + val primaryCtorNode = kotlinParser.createPrimaryConstructor("(${declarationsAssignedInCtor.joinToString(", ") { it.text }})").node addChild(primaryCtorNode, findChildByType(CLASS_BODY)) - val valueParameterList = CompositeElement(VALUE_PARAMETER_LIST) - primaryCtorNode.addChild(valueParameterList) - valueParameterList.addChild(LeafPsiElement(LPAR, "(")) - declarationsAssignedInCtor.forEachIndexed { index, ktProperty -> - valueParameterList.addChild(ktProperty.node.clone() as ASTNode, null) - if (index != declarationsAssignedInCtor.size - 1) { - valueParameterList.addChild(LeafPsiElement(COMMA, ","), null) - } - ktProperty.node.run { treeParent.removeChild(this) } + declarationsAssignedInCtor.forEach { ktProperty -> + ktProperty.node.let { treeParent.removeChild(it) } } - valueParameterList.addChild(LeafPsiElement(RPAR, ")")) if (otherStatements.isNotEmpty()) { - findChildByType(CLASS_BODY) - ?.run { - val beforeNode = - findChildByType(LBRACE)!!.let { if (it.treeNext.elementType == WHITE_SPACE) it.treeNext else it } - val classInitializer = CompositeElement(ElementType.CLASS_INITIALIZER) - addChild(PsiWhiteSpaceImpl("\n"), beforeNode) - addChild(classInitializer, beforeNode) - classInitializer.addChild(LeafPsiElement(ElementType.INIT_KEYWORD, KtTokens.INIT_KEYWORD.value)) - val initBlock = BlockExpressionElementType().createCompositeNode() - classInitializer.addChild(initBlock) - initBlock.addChild(LeafPsiElement(LBRACE, "{")) - otherStatements.forEach { - initBlock.addChild(PsiWhiteSpaceImpl("\n")) - initBlock.addChild(it.node.clone() as ASTNode) - } - initBlock.addChild(PsiWhiteSpaceImpl("\n")) - initBlock.addChild(LeafPsiElement(RBRACE, "}")) - } + 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) 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/test/resources/test/chapter6/classes/ConstructorWithInitExpected.kt b/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithInitExpected.kt index 2b75ef588f..0751292e52 100644 --- a/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithInitExpected.kt +++ b/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithInitExpected.kt @@ -1,8 +1,8 @@ package test.chapter6.classes class Test (var a: Int){ -init{ -println("Lorem ipsum") +init { + println("Lorem ipsum") foo() } diff --git a/diktat-rules/src/test/resources/test/chapter6/classes/SimpleConstructorExpected.kt b/diktat-rules/src/test/resources/test/chapter6/classes/SimpleConstructorExpected.kt index ede03d6c09..c36887ea28 100644 --- a/diktat-rules/src/test/resources/test/chapter6/classes/SimpleConstructorExpected.kt +++ b/diktat-rules/src/test/resources/test/chapter6/classes/SimpleConstructorExpected.kt @@ -13,8 +13,8 @@ class Test (var a: Int){ } class Test (var a: Int){ -init{ -var a = 14 +init { + var a = 14 } From 477127db4d722290a8978cce64ac57be23c71b0c Mon Sep 17 00:00:00 2001 From: Peter Trifanov Date: Wed, 21 Oct 2020 13:54:09 +0300 Subject: [PATCH 05/13] Rule 6.1.1 ### What's done: * Code style --- .../cqfn/diktat/ruleset/rules/classes/SingleConstructorRule.kt | 1 + .../diktat/ruleset/chapter6/SingleConstructorRuleFixTest.kt | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) 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 index 301d704202..31b963f866 100644 --- 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 @@ -119,6 +119,7 @@ class SingleConstructorRule(private val config: List) : Rule("singl classNode.convertSecondaryConstructorToPrimary(secondaryCtor, declarationsAssignedInCtor, otherStatements) } + @Suppress("UnsafeCallOnNullableType", "NestedBlockDepth") private fun ASTNode.convertSecondaryConstructorToPrimary(secondaryCtor: ASTNode, declarationsAssignedInCtor: List, otherStatements: List) { 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 index dc926dec85..1375a64b98 100644 --- 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 @@ -18,4 +18,4 @@ class SingleConstructorRuleFixTest : FixTestBase("test/chapter6/classes", ::Sing fun `should convert secondary constructor to a primary and init block`() { fixAndCompare("ConstructorWithInitExpected.kt", "ConstructorWithInitTest.kt") } -} \ No newline at end of file +} From b75df9a80cae95e500880d1fed8872b2c6b50bfd Mon Sep 17 00:00:00 2001 From: Peter Trifanov Date: Wed, 21 Oct 2020 13:55:19 +0300 Subject: [PATCH 06/13] Rule 6.1.1 ### What's done: * Code style --- diktat-analysis.yml | 3 ++- diktat-rules/src/main/resources/diktat-analysis-huawei.yml | 3 ++- diktat-rules/src/main/resources/diktat-analysis.yml | 3 ++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/diktat-analysis.yml b/diktat-analysis.yml index 3291f324ca..28ecf57a09 100644 --- a/diktat-analysis.yml +++ b/diktat-analysis.yml @@ -322,6 +322,7 @@ - name: KDOC_NO_CONSTRUCTOR_PROPERTY enabled: true configuration: {} -- name: "SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY" +# if a class has single constructor, it should be converted to a primary constructor +- name: SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY enabled: true configuration: {} \ No newline at end of file diff --git a/diktat-rules/src/main/resources/diktat-analysis-huawei.yml b/diktat-rules/src/main/resources/diktat-analysis-huawei.yml index e21833ff5b..56ec5ef98f 100644 --- a/diktat-rules/src/main/resources/diktat-analysis-huawei.yml +++ b/diktat-rules/src/main/resources/diktat-analysis-huawei.yml @@ -321,6 +321,7 @@ - name: KDOC_NO_CONSTRUCTOR_PROPERTY enabled: true configuration: {} -- name: "SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY" +# if a class has single constructor, it should be converted to a primary constructor +- name: SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY enabled: true configuration: {} \ No newline at end of file diff --git a/diktat-rules/src/main/resources/diktat-analysis.yml b/diktat-rules/src/main/resources/diktat-analysis.yml index 6920f65cfd..d62c010f53 100644 --- a/diktat-rules/src/main/resources/diktat-analysis.yml +++ b/diktat-rules/src/main/resources/diktat-analysis.yml @@ -323,6 +323,7 @@ - name: KDOC_NO_CONSTRUCTOR_PROPERTY enabled: true configuration: {} -- name: "SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY" +# if a class has single constructor, it should be converted to a primary constructor +- name: SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY enabled: true configuration: {} \ No newline at end of file From 8a7c30ac43b9fb2e0f24f991bf9dea68f17af3d4 Mon Sep 17 00:00:00 2001 From: Peter Trifanov Date: Fri, 23 Oct 2020 16:03:46 +0300 Subject: [PATCH 07/13] Rule 6.1.1 ### What's done: * Fixes --- .../ruleset/rules/classes/SingleConstructorRule.kt | 6 +++++- .../ruleset/chapter6/SingleConstructorRuleFixTest.kt | 6 ++++++ .../chapter6/classes/ConstructorWithModifiersExpected.kt | 7 +++++++ .../chapter6/classes/ConstructorWithModifiersTest.kt | 9 +++++++++ info/diktat-kotlin-coding-style-guide-en.md | 6 +++--- 5 files changed, 30 insertions(+), 4 deletions(-) create mode 100644 diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithModifiersExpected.kt create mode 100644 diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithModifiersTest.kt 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 index 31b963f866..0a6482acef 100644 --- 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 @@ -5,6 +5,7 @@ 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.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 @@ -125,7 +126,10 @@ class SingleConstructorRule(private val config: List) : Rule("singl otherStatements: List) { require(elementType == CLASS) - val primaryCtorNode = kotlinParser.createPrimaryConstructor("(${declarationsAssignedInCtor.joinToString(", ") { it.text }})").node + val primaryCtorNode = kotlinParser.createPrimaryConstructor( + (secondaryCtor.findChildByType(MODIFIER_LIST)?.text?.plus(" constructor ") ?: "") + + "(${declarationsAssignedInCtor.joinToString(", ") { it.text }})" + ).node addChild(primaryCtorNode, findChildByType(CLASS_BODY)) declarationsAssignedInCtor.forEach { ktProperty -> ktProperty.node.let { treeParent.removeChild(it) } 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 index 1375a64b98..56be65d5da 100644 --- 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 @@ -18,4 +18,10 @@ class SingleConstructorRuleFixTest : FixTestBase("test/chapter6/classes", ::Sing 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") + } } 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..2bc95546ed --- /dev/null +++ b/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithModifiersExpected.kt @@ -0,0 +1,7 @@ +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/info/diktat-kotlin-coding-style-guide-en.md b/info/diktat-kotlin-coding-style-guide-en.md index 2ca2f5370c..42406e953e 100644 --- a/info/diktat-kotlin-coding-style-guide-en.md +++ b/info/diktat-kotlin-coding-style-guide-en.md @@ -1624,8 +1624,8 @@ Good example: # 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. Bad example: ```kotlin class Test { @@ -1642,7 +1642,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) { // ... } From 12b90c3ac691d54f395198a257561b849361c755 Mon Sep 17 00:00:00 2001 From: Peter Trifanov Date: Thu, 5 Nov 2020 13:14:27 +0300 Subject: [PATCH 08/13] Rule 6.1.1 ### What's done: * Added Fixme and disabled test --- .../rules/classes/SingleConstructorRule.kt | 17 ++++++++++++++--- .../chapter6/SingleConstructorRuleFixTest.kt | 8 ++++++++ .../AssignmentWithLocalPropertyExpected.kt | 8 ++++++++ .../classes/AssignmentWithLocalPropertyTest.kt | 10 ++++++++++ 4 files changed, 40 insertions(+), 3 deletions(-) create mode 100644 diktat-rules/src/test/resources/test/chapter6/classes/AssignmentWithLocalPropertyExpected.kt create mode 100644 diktat-rules/src/test/resources/test/chapter6/classes/AssignmentWithLocalPropertyTest.kt 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 index 0a6482acef..1504227779 100644 --- 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 @@ -10,6 +10,7 @@ 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.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.findAllNodesWithSpecificType @@ -28,15 +29,21 @@ import org.jetbrains.kotlin.psi.KtSecondaryConstructor import org.jetbrains.kotlin.psi.KtThisExpression import org.jetbrains.kotlin.psi.psiUtil.asAssignment +/** + * 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. + * FixMe: Code is being broken if properties initialization depends on local variables. In these cases local variables should + * not be moved to an `init` block but rather wrapped together with the assignment into a `run` block. + */ class SingleConstructorRule(private val config: List) : 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() } + private lateinit var emitWarn: EmitType override fun visit( node: ASTNode, autoCorrect: Boolean, - emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit + emit: EmitType ) { emitWarn = emit isFixMode = autoCorrect @@ -127,7 +134,11 @@ class SingleConstructorRule(private val config: List) : Rule("singl require(elementType == CLASS) val primaryCtorNode = kotlinParser.createPrimaryConstructor( - (secondaryCtor.findChildByType(MODIFIER_LIST)?.text?.plus(" constructor ") ?: "") + + (secondaryCtor + .findChildByType(MODIFIER_LIST) + ?.text + ?.plus(" constructor ") + ?: "") + "(${declarationsAssignedInCtor.joinToString(", ") { it.text }})" ).node addChild(primaryCtorNode, findChildByType(CLASS_BODY)) 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 index 56be65d5da..45dfa2ecf7 100644 --- 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 @@ -3,6 +3,7 @@ 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.Disabled import org.junit.jupiter.api.Tag import org.junit.jupiter.api.Test @@ -24,4 +25,11 @@ class SingleConstructorRuleFixTest : FixTestBase("test/chapter6/classes", ::Sing fun `should convert secondary constructor to a primary saving modifiers`() { fixAndCompare("ConstructorWithModifiersExpected.kt", "ConstructorWithModifiersTest.kt") } + + @Test + @Tag(WarningNames.SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY) + @Disabled("Not yet implemented") + fun `should wrap assignments and required local variables into a run block`() { + fixAndCompare("AssignmentWithLocalPropertyExpected.kt", "AssignmentWithLocalPropertyTest.kt") + } } 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..a89b173773 --- /dev/null +++ b/diktat-rules/src/test/resources/test/chapter6/classes/AssignmentWithLocalPropertyExpected.kt @@ -0,0 +1,8 @@ +package test.chapter6.classes + +class Foo(a: Int) { + val a = run { + val f = F(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 From 179128f0184eb9734d06ad0104c035b811d4dec7 Mon Sep 17 00:00:00 2001 From: Peter Trifanov Date: Fri, 6 Nov 2020 13:26:36 +0300 Subject: [PATCH 09/13] Rule 6.1.1 ### What's done: * Added more logic on init block creation --- .../rules/classes/SingleConstructorRule.kt | 134 +++++++++++------- .../chapter6/SingleConstructorRuleFixTest.kt | 11 +- .../AssignmentWithLocalPropertyExpected.kt | 13 +- ...onstructorWithCustomAssignmentsExpected.kt | 10 ++ .../ConstructorWithCustomAssignmentsTest.kt | 9 ++ .../classes/ConstructorWithInitExpected.kt | 4 +- .../classes/SimpleConstructorExpected.kt | 4 +- 7 files changed, 123 insertions(+), 62 deletions(-) create mode 100644 diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithCustomAssignmentsExpected.kt create mode 100644 diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithCustomAssignmentsTest.kt 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 index 1504227779..9eb26415f7 100644 --- 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 @@ -1,19 +1,15 @@ 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.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.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.findAllNodesWithSpecificType import org.cqfn.diktat.ruleset.utils.getAllChildrenWithType import org.cqfn.diktat.ruleset.utils.getIdentifierName import org.cqfn.diktat.ruleset.utils.isGoingAfter @@ -28,12 +24,11 @@ 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. - * FixMe: Code is being broken if properties initialization depends on local variables. In these cases local variables should - * not be moved to an `init` block but rather wrapped together with the assignment into a `run` block. */ class SingleConstructorRule(private val config: List) : Rule("single-constructor") { private var isFixMode: Boolean = false @@ -63,7 +58,8 @@ class SingleConstructorRule(private val config: List) : Rule("singl ?.let { secondaryCtor -> SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY.warnAndFix( config, emitWarn, isFixMode, "in class <${node.getIdentifierName()?.text}>", - node.startOffset, node) { + node.startOffset, node + ) { convertConstructorToPrimary(node, secondaryCtor) } } @@ -74,90 +70,128 @@ class SingleConstructorRule(private val config: List) : Rule("singl * 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. + * - 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. + * - 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("UnsafeCallOnNullableType") 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 as? KtBinaryExpression)?.asAssignment() != null } - ?: emptyList() to emptyList() + ?.partition { it is KtBinaryExpression && it.asAssignment() != null } + ?.run { first.map { it as KtBinaryExpression } to second } + ?: emptyList() 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 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) - 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 - } + // 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 properties declarations - val declarationsAssignedInCtor = assignedClassProperties - .mapNotNull { reference -> + // 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, otherStatements) + 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("UnsafeCallOnNullableType", "NestedBlockDepth") - private fun ASTNode.convertSecondaryConstructorToPrimary(secondaryCtor: ASTNode, - declarationsAssignedInCtor: List, - otherStatements: List) { + 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 neededSecondaryCtorArguments = (secondaryCtor.psi as KtSecondaryConstructor).valueParameters.run { + val dependencies = nonTrivialAssignments.keys + .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 + } + } + val primaryCtorNode = kotlinParser.createPrimaryConstructor( (secondaryCtor .findChildByType(MODIFIER_LIST) ?.text ?.plus(" constructor ") ?: "") + - "(${declarationsAssignedInCtor.joinToString(", ") { it.text }})" + "(${ + declarationsAssignedInCtor.run { + joinToString( + ", ", + postfix = if (isNotEmpty() && neededSecondaryCtorArguments.isNotEmpty()) ", " else "" + ) { it.text } + } + }" + + "${neededSecondaryCtorArguments.joinToString(", ") { it.text }})" ).node addChild(primaryCtorNode, findChildByType(CLASS_BODY)) declarationsAssignedInCtor.forEach { ktProperty -> ktProperty.node.let { treeParent.removeChild(it) } } - if (otherStatements.isNotEmpty()) { + if (otherStatements.isNotEmpty() || nonTrivialAssignments.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 }} + | ${(otherStatements + nonTrivialAssignments.keys).joinToString("\n") { it.text }} |} """.trimMargin() ) - addChild(PsiWhiteSpaceImpl("\n"), beforeNode) - addChild(classInitializer, beforeNode) + addChild(classInitializer, secondaryCtor) + addChild(PsiWhiteSpaceImpl("\n"), secondaryCtor) } } 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 index 45dfa2ecf7..1b0d96b0e4 100644 --- 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 @@ -3,7 +3,6 @@ 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.Disabled import org.junit.jupiter.api.Tag import org.junit.jupiter.api.Test @@ -28,8 +27,14 @@ class SingleConstructorRuleFixTest : FixTestBase("test/chapter6/classes", ::Sing @Test @Tag(WarningNames.SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY) - @Disabled("Not yet implemented") - fun `should wrap assignments and required local variables into a run block`() { + 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/resources/test/chapter6/classes/AssignmentWithLocalPropertyExpected.kt b/diktat-rules/src/test/resources/test/chapter6/classes/AssignmentWithLocalPropertyExpected.kt index a89b173773..c436ab8d90 100644 --- a/diktat-rules/src/test/resources/test/chapter6/classes/AssignmentWithLocalPropertyExpected.kt +++ b/diktat-rules/src/test/resources/test/chapter6/classes/AssignmentWithLocalPropertyExpected.kt @@ -1,8 +1,11 @@ package test.chapter6.classes -class Foo(a: Int) { - val a = run { - val f = F(a) - f.foo() - } +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/ConstructorWithCustomAssignmentsExpected.kt b/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithCustomAssignmentsExpected.kt new file mode 100644 index 0000000000..014c1a1766 --- /dev/null +++ b/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithCustomAssignmentsExpected.kt @@ -0,0 +1,10 @@ +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 index 0751292e52..297ddd3e98 100644 --- a/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithInitExpected.kt +++ b/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithInitExpected.kt @@ -1,11 +1,11 @@ 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/SimpleConstructorExpected.kt b/diktat-rules/src/test/resources/test/chapter6/classes/SimpleConstructorExpected.kt index c36887ea28..103f42109f 100644 --- a/diktat-rules/src/test/resources/test/chapter6/classes/SimpleConstructorExpected.kt +++ b/diktat-rules/src/test/resources/test/chapter6/classes/SimpleConstructorExpected.kt @@ -13,10 +13,10 @@ class Test (var a: Int){ } class Test (var a: Int){ + + init { var a = 14 } - - } \ No newline at end of file From cf12d57f165d1a5f66bab62c04c248b999f5645b Mon Sep 17 00:00:00 2001 From: Peter Trifanov Date: Fri, 6 Nov 2020 13:40:42 +0300 Subject: [PATCH 10/13] Rule 6.1.1 ### What's done: * Fixes & smoke test example --- .../ruleset/rules/DiktatRuleSetProvider.kt | 2 +- .../rules/classes/SingleConstructorRule.kt | 30 ++++++++++++------- .../AssignmentWithLocalPropertyExpected.kt | 1 - ...onstructorWithCustomAssignmentsExpected.kt | 1 - .../classes/ConstructorWithInitExpected.kt | 2 -- .../ConstructorWithModifiersExpected.kt | 3 -- .../classes/SimpleConstructorExpected.kt | 8 ----- .../smoke/src/main/kotlin/Example3Expected.kt | 2 +- .../smoke/src/main/kotlin/Example3Test.kt | 9 ++++-- 9 files changed, 28 insertions(+), 30 deletions(-) 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 0c0502db3a..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 @@ -54,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, @@ -92,7 +93,6 @@ class DiktatRuleSetProvider(private val diktatConfigFile: String = "diktat-analy ::FunctionLength, ::LambdaParameterOrder, ::FunctionArgumentsSize, - ::SingleConstructorRule, ::BlankLinesRule, ::FileSize, ::NullableTypeRule, 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 index 9eb26415f7..853bbab959 100644 --- 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 @@ -6,6 +6,7 @@ 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.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 @@ -166,19 +167,23 @@ class SingleConstructorRule(private val config: List) : Rule("singl ?.text ?.plus(" constructor ") ?: "") + - "(${ - declarationsAssignedInCtor.run { - joinToString( - ", ", - postfix = if (isNotEmpty() && neededSecondaryCtorArguments.isNotEmpty()) ", " else "" - ) { it.text } - } - }" + - "${neededSecondaryCtorArguments.joinToString(", ") { it.text }})" - ).node + "(" + + declarationsAssignedInCtor.run { + joinToString( + ", ", + postfix = if (isNotEmpty() && neededSecondaryCtorArguments.isNotEmpty()) ", " else "" + ) { it.text } + } + + neededSecondaryCtorArguments.joinToString(", ") { it.text } + + ")" + ) + .node addChild(primaryCtorNode, findChildByType(CLASS_BODY)) declarationsAssignedInCtor.forEach { ktProperty -> - ktProperty.node.let { treeParent.removeChild(it) } + ktProperty.node.run { + treePrev.takeIf { it.elementType == WHITE_SPACE }?.let { treeParent.removeChild(it) } + treeParent.removeChild(this) + } } if (otherStatements.isNotEmpty() || nonTrivialAssignments.isNotEmpty()) { @@ -195,6 +200,9 @@ class SingleConstructorRule(private val config: List) : Rule("singl } } + secondaryCtor.run { treePrev.takeIf { it.elementType == WHITE_SPACE } ?: treeNext } + .takeIf { it.elementType == WHITE_SPACE } + ?.run { treeParent.removeChild(this) } findChildByType(CLASS_BODY)?.removeChild(secondaryCtor) } } diff --git a/diktat-rules/src/test/resources/test/chapter6/classes/AssignmentWithLocalPropertyExpected.kt b/diktat-rules/src/test/resources/test/chapter6/classes/AssignmentWithLocalPropertyExpected.kt index c436ab8d90..8480318136 100644 --- a/diktat-rules/src/test/resources/test/chapter6/classes/AssignmentWithLocalPropertyExpected.kt +++ b/diktat-rules/src/test/resources/test/chapter6/classes/AssignmentWithLocalPropertyExpected.kt @@ -7,5 +7,4 @@ class Foo (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 index 014c1a1766..f77c436041 100644 --- a/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithCustomAssignmentsExpected.kt +++ b/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithCustomAssignmentsExpected.kt @@ -6,5 +6,4 @@ class Test (a: Int){ init { 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 index 297ddd3e98..42ec5599f4 100644 --- a/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithInitExpected.kt +++ b/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithInitExpected.kt @@ -2,10 +2,8 @@ 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/ConstructorWithModifiersExpected.kt b/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithModifiersExpected.kt index 2bc95546ed..1cf51521c2 100644 --- a/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithModifiersExpected.kt +++ b/diktat-rules/src/test/resources/test/chapter6/classes/ConstructorWithModifiersExpected.kt @@ -1,7 +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/SimpleConstructorExpected.kt b/diktat-rules/src/test/resources/test/chapter6/classes/SimpleConstructorExpected.kt index 103f42109f..a59f3c0e47 100644 --- a/diktat-rules/src/test/resources/test/chapter6/classes/SimpleConstructorExpected.kt +++ b/diktat-rules/src/test/resources/test/chapter6/classes/SimpleConstructorExpected.kt @@ -1,22 +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/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 From acffcd55c047ec84b4d9cdaba2ff497595399c32 Mon Sep 17 00:00:00 2001 From: Peter Trifanov Date: Fri, 6 Nov 2020 14:03:18 +0300 Subject: [PATCH 11/13] Rule 6.1.1 ### What's done: * Code style edits applied --- info/guide/diktat-coding-convention.md | 6 +++--- info/guide/guide-chapter-6.md | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) 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) { // ... } From c2b6db36dd00e74bf66de66907d9844acdfdeb9f Mon Sep 17 00:00:00 2001 From: Peter Trifanov Date: Fri, 6 Nov 2020 15:04:57 +0300 Subject: [PATCH 12/13] Rule 6.1.1 ### What's done: * Code style --- .../rules/classes/SingleConstructorRule.kt | 207 ++++++++++-------- 1 file changed, 110 insertions(+), 97 deletions(-) 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 index 853bbab959..22012fc8d4 100644 --- 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 @@ -1,12 +1,5 @@ package org.cqfn.diktat.ruleset.rules.classes -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.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 @@ -14,6 +7,14 @@ 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 @@ -21,6 +22,7 @@ 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 @@ -37,9 +39,9 @@ class SingleConstructorRule(private val config: List) : Rule("singl private lateinit var emitWarn: EmitType override fun visit( - node: ASTNode, - autoCorrect: Boolean, - emit: EmitType + node: ASTNode, + autoCorrect: Boolean, + emit: EmitType ) { emitWarn = emit isFixMode = autoCorrect @@ -53,17 +55,17 @@ class SingleConstructorRule(private val config: List) : Rule("singl 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) + .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) + } } - } } } @@ -76,16 +78,17 @@ class SingleConstructorRule(private val config: List) : Rule("singl * - 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() + .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 } @@ -95,89 +98,63 @@ class SingleConstructorRule(private val config: List) : Rule("singl // 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 } + .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() } + .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() + .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 - } + associateWith { + // non-null assert is safe because of predicate in partitioning + it.asAssignment()!!.left!! } - .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 } } + .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("UnsafeCallOnNullableType", "NestedBlockDepth") + @Suppress("UnsafeCallOnNullableType", "NestedBlockDepth", "GENERIC_VARIABLE_WRONG_DECLARATION") private fun ASTNode.convertSecondaryConstructorToPrimary( - secondaryCtor: ASTNode, - declarationsAssignedInCtor: List, - nonTrivialAssignments: Map, - otherStatements: List + 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 neededSecondaryCtorArguments = (secondaryCtor.psi as KtSecondaryConstructor).valueParameters.run { - val dependencies = nonTrivialAssignments.keys - .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 - } - } + val nonTrivialSecondaryCtorParameters = getNonTrivialParameters(secondaryCtor, nonTrivialAssignments.keys, localProperties) - val primaryCtorNode = kotlinParser.createPrimaryConstructor( - (secondaryCtor - .findChildByType(MODIFIER_LIST) - ?.text - ?.plus(" constructor ") - ?: "") + - "(" + - declarationsAssignedInCtor.run { - joinToString( - ", ", - postfix = if (isNotEmpty() && neededSecondaryCtorArguments.isNotEmpty()) ", " else "" - ) { it.text } - } + - neededSecondaryCtorArguments.joinToString(", ") { it.text } + - ")" - ) - .node + val primaryCtorNode = createPrimaryCtor(secondaryCtor, declarationsAssignedInCtor, nonTrivialSecondaryCtorParameters) addChild(primaryCtorNode, findChildByType(CLASS_BODY)) declarationsAssignedInCtor.forEach { ktProperty -> ktProperty.node.run { @@ -189,20 +166,56 @@ class SingleConstructorRule(private val config: List) : Rule("singl if (otherStatements.isNotEmpty() || nonTrivialAssignments.isNotEmpty()) { findChildByType(CLASS_BODY)?.run { val classInitializer = kotlinParser.createNode( - """ - |init { - | ${(otherStatements + nonTrivialAssignments.keys).joinToString("\n") { it.text }} - |} - """.trimMargin() + """ + |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) } + secondaryCtor + .run { treePrev.takeIf { it.elementType == WHITE_SPACE } ?: treeNext } + .takeIf { it.elementType == WHITE_SPACE } + ?.run { treeParent.removeChild(this) } findChildByType(CLASS_BODY)?.removeChild(secondaryCtor) } + + 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 } From 5f6ab69dc4563bb6d701c49d95a193028069564a Mon Sep 17 00:00:00 2001 From: Peter Trifanov Date: Fri, 6 Nov 2020 15:25:37 +0300 Subject: [PATCH 13/13] Rule 6.1.1 ### What's done: * Code style --- .../cqfn/diktat/ruleset/rules/classes/SingleConstructorRule.kt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 index 22012fc8d4..952c0b9f41 100644 --- 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 @@ -141,7 +141,7 @@ class SingleConstructorRule(private val config: List) : Rule("singl } .filterValues { left -> left.getReferencedName() in classProperties.mapNotNull { it.name } } - @Suppress("UnsafeCallOnNullableType", "NestedBlockDepth", "GENERIC_VARIABLE_WRONG_DECLARATION") + @Suppress("NestedBlockDepth", "GENERIC_VARIABLE_WRONG_DECLARATION") private fun ASTNode.convertSecondaryConstructorToPrimary( secondaryCtor: ASTNode, declarationsAssignedInCtor: List, @@ -184,6 +184,7 @@ class SingleConstructorRule(private val config: List) : Rule("singl findChildByType(CLASS_BODY)?.removeChild(secondaryCtor) } + @Suppress("UnsafeCallOnNullableType") private fun getNonTrivialParameters(secondaryCtor: ASTNode, nonTrivialAssignments: Collection, localProperties: List) = (secondaryCtor.psi as KtSecondaryConstructor)