diff --git a/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter1/IdentifierNaming.kt b/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter1/IdentifierNaming.kt index 5dc57cd8dc..24fe0cfcba 100644 --- a/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter1/IdentifierNaming.kt +++ b/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter1/IdentifierNaming.kt @@ -30,15 +30,11 @@ import org.jetbrains.kotlin.KtNodeTypes.CLASS import org.jetbrains.kotlin.KtNodeTypes.DESTRUCTURING_DECLARATION import org.jetbrains.kotlin.KtNodeTypes.DESTRUCTURING_DECLARATION_ENTRY import org.jetbrains.kotlin.KtNodeTypes.FUNCTION_TYPE -import org.jetbrains.kotlin.KtNodeTypes.MODIFIER_LIST -import org.jetbrains.kotlin.KtNodeTypes.NULLABLE_TYPE import org.jetbrains.kotlin.KtNodeTypes.OBJECT_DECLARATION import org.jetbrains.kotlin.KtNodeTypes.PROPERTY -import org.jetbrains.kotlin.KtNodeTypes.PROPERTY_ACCESSOR import org.jetbrains.kotlin.KtNodeTypes.REFERENCE_EXPRESSION import org.jetbrains.kotlin.KtNodeTypes.TYPE_PARAMETER import org.jetbrains.kotlin.KtNodeTypes.TYPE_REFERENCE -import org.jetbrains.kotlin.KtNodeTypes.USER_TYPE import org.jetbrains.kotlin.KtNodeTypes.VALUE_PARAMETER_LIST import org.jetbrains.kotlin.com.intellij.lang.ASTNode import org.jetbrains.kotlin.com.intellij.psi.impl.source.tree.LeafPsiElement @@ -48,7 +44,6 @@ import org.jetbrains.kotlin.kdoc.parser.KDocKnownTag import org.jetbrains.kotlin.lexer.KtTokens import org.jetbrains.kotlin.lexer.KtTokens.CATCH_KEYWORD import org.jetbrains.kotlin.lexer.KtTokens.IDENTIFIER -import org.jetbrains.kotlin.lexer.KtTokens.PRIVATE_KEYWORD import org.jetbrains.kotlin.psi.KtParameter import org.jetbrains.kotlin.psi.KtPrimaryConstructor import org.jetbrains.kotlin.psi.KtProperty @@ -145,36 +140,6 @@ class IdentifierNaming(configRules: List) : DiktatRule( return false } - /** - * method checks that identifier is correct backing field - */ - private fun ASTNode.isCorrectBackingField(variableName: ASTNode): Boolean { - val propertyNodes = this.treeParent.getAllChildrenWithType(KtNodeTypes.PROPERTY) - val variableNameCut = variableName.text.drop(1) - // check that backing field name is correct - - if (variableName.text.startsWith("_") && variableNameCut.isLowerCamelCase()) { - val matchingNode = propertyNodes.find { propertyNode -> - val nodeType = this.getFirstChildWithType(TYPE_REFERENCE) - val propertyType = propertyNode.getFirstChildWithType(TYPE_REFERENCE) - // check that property and backing field has same type - val sameType = propertyType?.text == nodeType?.text - // check that property USER_TYPE is same as backing field NULLABLE_TYPE - val nodeNullableType = nodeType?.getFirstChildWithType(NULLABLE_TYPE) - val sameTypeWithNullable = propertyType?.getFirstChildWithType(USER_TYPE)?.text == - nodeNullableType?.getFirstChildWithType(USER_TYPE)?.text - val matchingNames = propertyNode.getFirstChildWithType(IDENTIFIER)?.text == variableNameCut - val isPrivate = this.getFirstChildWithType(MODIFIER_LIST)?.getFirstChildWithType(PRIVATE_KEYWORD) != null - - matchingNames && (sameType || sameTypeWithNullable) && isPrivate && - this.getFirstChildWithType(PROPERTY_ACCESSOR) == null && - propertyNode.getFirstChildWithType(PROPERTY_ACCESSOR) != null - } - return matchingNode?.let { true } ?: false - } - return false - } - /** * all checks for case and naming for vals/vars/constants */ @@ -228,7 +193,8 @@ class IdentifierNaming(configRules: List) : DiktatRule( } } else if (variableName.text != "_" && !variableName.text.isLowerCamelCase() && // variable name should be in camel case. The only exception is a list of industry standard variables like i, j, k. - !node.isCorrectBackingField(variableName)) { + !isPairPropertyBackingField(null, node) + ) { VARIABLE_NAME_INCORRECT_FORMAT.warnOnlyOrWarnAndFix( configRules = configRules, emit = emitWarn, diff --git a/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter6/CustomGetterSetterRule.kt b/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter6/CustomGetterSetterRule.kt index 52e9afdc0d..f63f70670d 100644 --- a/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter6/CustomGetterSetterRule.kt +++ b/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter6/CustomGetterSetterRule.kt @@ -33,6 +33,11 @@ class CustomGetterSetterRule(configRules: List) : DiktatRule( val isPrivateSetter = node.getFirstChildWithType(MODIFIER_LIST)?.hasAnyChildOfTypes(PRIVATE_KEYWORD) ?: false val isOverrideGetter = node.treeParent.getFirstChildWithType(MODIFIER_LIST)?.hasAnyChildOfTypes(OVERRIDE_KEYWORD) ?: false + // find matching node + if (isPairPropertyBackingField(node.treeParent, null)) { + return + } + setter?.let { // only private custom setters are allowed if (!isPrivateSetter) { diff --git a/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/utils/AstNodeUtils.kt b/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/utils/AstNodeUtils.kt index 83f84b483f..f669e8bcb7 100644 --- a/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/utils/AstNodeUtils.kt +++ b/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/utils/AstNodeUtils.kt @@ -32,6 +32,7 @@ import org.jetbrains.kotlin.KtNodeTypes.LONG_STRING_TEMPLATE_ENTRY import org.jetbrains.kotlin.KtNodeTypes.MODIFIER_LIST import org.jetbrains.kotlin.KtNodeTypes.OPERATION_REFERENCE import org.jetbrains.kotlin.KtNodeTypes.PARENTHESIZED +import org.jetbrains.kotlin.KtNodeTypes.PROPERTY import org.jetbrains.kotlin.KtNodeTypes.REFERENCE_EXPRESSION import org.jetbrains.kotlin.KtNodeTypes.TYPE_PARAMETER_LIST import org.jetbrains.kotlin.KtNodeTypes.VALUE_PARAMETER_LIST @@ -988,6 +989,20 @@ fun ASTNode.visit(visitor: AstNodeVisitor) { fun PsiElement.isLongStringTemplateEntry(): Boolean = node.elementType == LONG_STRING_TEMPLATE_ENTRY +/** + * Checks that node has child PRIVATE_KEYWORD + * + * @return true if node has child PRIVATE_KEYWORD + */ +fun ASTNode.isPrivate(): Boolean = this.getFirstChildWithType(MODIFIER_LIST)?.getFirstChildWithType(PRIVATE_KEYWORD) != null + +/** + * Checks that node has getter or setter + * + * @return true if node has getter or setter + */ +fun ASTNode.hasSetterOrGetter(): Boolean = this.getFirstChildWithType(KtNodeTypes.PROPERTY_ACCESSOR) != null + private fun ASTNode.isFollowedByNewlineCheck() = this.treeNext.elementType == WHITE_SPACE && this.treeNext.text.contains("\n") @@ -1100,6 +1115,14 @@ private fun ASTNode.hasExplicitIt(): Boolean { ?: false } +/** + * Gets list of property nodes + * + * @param node + * @return list of property nodes + */ +fun getPropertyNodes(node: ASTNode?): List? = node?.treeParent?.getAllChildrenWithType(PROPERTY) + /** * Checks node is located in file src/test/**/*Test.kt * @@ -1150,6 +1173,48 @@ fun doesLambdaContainIt(lambdaNode: ASTNode): Boolean { return hasNoParameters(lambdaNode) && hasIt } +/** + * Checks that property node has pair backing field node or backing field node has pair property node. Nodes make a pair of property node and backing field node if they have: + * 1. matching names (xyz -> _xyz) + * 2. same type (but backing field can be nullable), + * 3. backing field is private + * 4. backing field should have no accessors/modifiers + * 5. property should have at least an accessor, or a modifier, or both + * + * @param propertyNode if not null, trying to find matching backing field node + * @param backingFieldNode if not null, trying to find matching property node + * @return true if node has a pair + */ +@Suppress("CyclomaticComplexMethod") +internal fun isPairPropertyBackingField(propertyNode: ASTNode?, backingFieldNode: ASTNode?): Boolean { + val node = propertyNode ?: backingFieldNode + val propertyListNullable = (node?.treeParent?.getAllChildrenWithType(PROPERTY)) ?: return false + val propertyList: List = propertyListNullable + val nodeType = node.getFirstChildWithType(KtNodeTypes.TYPE_REFERENCE) + val nodeNullableType = nodeType?.getFirstChildWithType(KtNodeTypes.NULLABLE_TYPE) + val nodeName = node.getFirstChildWithType(IDENTIFIER)?.text + + val matchingNode = propertyList.find {pairNode -> + val propertyType = pairNode.getFirstChildWithType(KtNodeTypes.TYPE_REFERENCE) + // check that property and backing field has same type + val sameType = nodeType?.text == propertyType?.text + // check that property USER_TYPE is same as backing field NULLABLE_TYPE + val sameTypeWithNullable = propertyType?.getFirstChildWithType(KtNodeTypes.USER_TYPE)?.text == + nodeNullableType?.getFirstChildWithType(KtNodeTypes.USER_TYPE)?.text + // check matching names + val propertyName = pairNode.getFirstChildWithType(IDENTIFIER)?.text + val matchingNames = propertyNode?.let { nodeName == propertyName?.drop(1) } ?: run { nodeName?.drop(1) == propertyName } + + val isPrivate = propertyNode?.let { pairNode.isPrivate() } ?: run { node.isPrivate() } + val noSetterGetterBackingField = propertyNode?.let { !pairNode.hasSetterOrGetter() } ?: run { !node.hasSetterOrGetter() } + val hasSetterOrGetterProperty = propertyNode?.let { node.hasSetterOrGetter() } ?: run { pairNode.hasSetterOrGetter() } + + matchingNames && (sameType || sameTypeWithNullable) && isPrivate && + noSetterGetterBackingField && hasSetterOrGetterProperty + } + return matchingNode?.let { true } ?: false +} + private fun hasAnySuppressorForInspection( warningName: String, rule: Rule, diff --git a/diktat-rules/src/test/kotlin/com/saveourtool/diktat/ruleset/chapter6/CustomGetterSetterWarnTest.kt b/diktat-rules/src/test/kotlin/com/saveourtool/diktat/ruleset/chapter6/CustomGetterSetterWarnTest.kt index 20244f6ec9..bf0f469933 100644 --- a/diktat-rules/src/test/kotlin/com/saveourtool/diktat/ruleset/chapter6/CustomGetterSetterWarnTest.kt +++ b/diktat-rules/src/test/kotlin/com/saveourtool/diktat/ruleset/chapter6/CustomGetterSetterWarnTest.kt @@ -102,5 +102,64 @@ class CustomGetterSetterWarnTest : LintTestBase(::CustomGetterSetterRule) { DiktatError(7, 9, ruleId, "${Warnings.CUSTOM_GETTERS_SETTERS.warnText()} get"), ) } + + @Test + @Tag(CUSTOM_GETTERS_SETTERS) + fun `should not trigger on property with backing field`() { + lintMethod( + """ + |package com.example + | + |class MutableTableContainer { + | private var _table: Map? = null + | + | val table: Map + | get() { + | if (_table == null) { + | _table = hashMapOf() + | } + | return _table ?: throw AssertionError("Set to null by another thread") + | } + | set(value) { + | field = value + | } + | + |} + """.trimMargin(), + ) + } + + @Test + @Tag(CUSTOM_GETTERS_SETTERS) + fun `should trigger on backing field with setter`() { + val code = + """ + |package com.example + | + |class MutableTableContainer { + | var _table: Map? = null + | set(value) { + | field = value + | } + | + | val table: Map + | get() { + | if (_table == null) { + | _table = hashMapOf() + | } + | return _table ?: throw AssertionError("Set to null by another thread") + | } + | set(value) { + | field = value + | } + | + |} + """.trimMargin() + lintMethod(code, + DiktatError(5, 17, ruleId, "${Warnings.CUSTOM_GETTERS_SETTERS.warnText()} set", false), + DiktatError(10, 8, ruleId, "${Warnings.CUSTOM_GETTERS_SETTERS.warnText()} get", false), + DiktatError(16, 8, ruleId, "${Warnings.CUSTOM_GETTERS_SETTERS.warnText()} set", false) + ) + } }