CUSTOM_GETTERS_SETTERS false positive when a property has a backing…

… field (#1815) ### What's done: - fixed warning `CUSTOM_GETTERS_SETTERS` on properties with backing field. - added `ASTNode.isPrivate()`, `ASTNode.hasSetterGetter()` methods - added `isPairPropertyBackingField(propertyNode: ASTNode?, backingFieldNode: ASTNode?)` function - added warning tests. Closes #1709
saveourtool · Nov 27, 2023 · 2030a14 · 2030a14
1 parent b5c1a37
commit 2030a14
Show file tree

Hide file tree

Showing 4 changed files with 131 additions and 36 deletions.
diff --git a/...t-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter1/IdentifierNaming.kt b/...t-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<RulesConfig>) : 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<RulesConfig>) : 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/...s/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter6/CustomGetterSetterRule.kt b/...s/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter6/CustomGetterSetterRule.kt
@@ -33,6 +33,11 @@ class CustomGetterSetterRule(configRules: List<RulesConfig>) : 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
@@ -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<Unit>) {
 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<ASTNode>? = 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<ASTNode> = 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/...les/src/test/kotlin/com/saveourtool/diktat/ruleset/chapter6/CustomGetterSetterWarnTest.kt b/...les/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<String, Int>? = null
+                    |
+                    |   val table: Map<String, Int>
+                    |       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<String, Int>? = null
+                    |                set(value) {
+                    |                    field = value
+                    |                }
+                    |
+                    |   val table: Map<String, Int>
+                    |       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)
+        )
+    }
 }