Fixed VARIABLE_NAME_INCORRECT_FORMAT false positive when a property…

… has a backing field (#1810) ### What's done: - fixed warning `VARIABLE_NAME_INCORRECT_FORMAT` on `backing field`. - added `ASTNode.isCorrectBackingField(ASTNode)` method. - added warning tests, which trigger on incorrect `backing field` format. Closes #1711
saveourtool · Nov 22, 2023 · b01cd64 · b01cd64
1 parent 51195ee
commit b01cd64
Show file tree

Hide file tree

Showing 3 changed files with 162 additions and 2 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,10 +30,15 @@ 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
@@ -43,6 +48,7 @@ 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
@@ -139,6 +145,36 @@ 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
      */
@@ -149,9 +185,11 @@ class IdentifierNaming(configRules: List<RulesConfig>) : DiktatRule(
         "ComplexMethod",
         "UnsafeCallOnNullableType",
     )
+
     private fun checkVariableName(node: ASTNode): List<ASTNode> {
         // special case for Destructuring declarations that can be treated as parameters in lambda:
         var namesOfVariables = extractVariableIdentifiers(node)
+
         // Only local private properties will be autofix in order not to break code if there are usages in other files.
         // Destructuring declarations are only allowed for local variables/values, so we don't need to calculate `isFix` for every node in `namesOfVariables`
         val isPublicOrNonLocalProperty = if (node.elementType == KtNodeTypes.PROPERTY) (node.psi as KtProperty).run { !isLocal && !isPrivate() } else false
@@ -188,8 +226,9 @@ class IdentifierNaming(configRules: List<RulesConfig>) : DiktatRule(
                             (variableName as LeafPsiElement).rawReplaceWithText(variableName.text.toDeterministic { toUpperSnakeCase() })
                         }
                     }
-                } 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.
+                } 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)) {
                     VARIABLE_NAME_INCORRECT_FORMAT.warnOnlyOrWarnAndFix(
                         configRules = configRules,
                         emit = emitWarn,

diff --git a/...rules/src/test/kotlin/com/saveourtool/diktat/ruleset/chapter1/IdentifierNamingWarnTest.kt b/...rules/src/test/kotlin/com/saveourtool/diktat/ruleset/chapter1/IdentifierNamingWarnTest.kt
@@ -23,7 +23,14 @@ import com.saveourtool.diktat.ruleset.rules.chapter1.IdentifierNaming
 import com.saveourtool.diktat.util.LintTestBase
 
 import com.saveourtool.diktat.api.DiktatError
+import com.saveourtool.diktat.ruleset.constants.Warnings
+import com.saveourtool.diktat.ruleset.utils.getFirstChildWithType
+import com.saveourtool.diktat.ruleset.utils.hasAnyChildOfTypes
+import com.saveourtool.diktat.ruleset.utils.prettyPrint
 import generated.WarningNames
+import org.jetbrains.kotlin.KtNodeTypes
+import org.jetbrains.kotlin.com.intellij.lang.ASTNode
+import org.jetbrains.kotlin.lexer.KtTokens
 import org.junit.jupiter.api.Tag
 import org.junit.jupiter.api.Tags
 import org.junit.jupiter.api.Test
@@ -629,6 +636,119 @@ class IdentifierNamingWarnTest : LintTestBase(::IdentifierNaming) {
         lintMethod(code)
     }
 
+    @Test
+    @Tag(WarningNames.VARIABLE_NAME_INCORRECT_FORMAT)
+    fun `should not trigger on 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(WarningNames.VARIABLE_NAME_INCORRECT_FORMAT)
+    fun `should trigger on backing field with setter`() {
+        val code =
+            """
+                    |package com.example
+                    |
+                    |class MutableTableContainer {
+                    |   private 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(4, 16, ruleId, "${VARIABLE_NAME_INCORRECT_FORMAT.warnText()} _table", true),
+        )
+    }
+
+    @Test
+    @Tag(WarningNames.VARIABLE_NAME_INCORRECT_FORMAT)
+    fun `should trigger on backing field with no matching property`() {
+        val code =
+            """
+                    |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()
+        lintMethod(code,
+            DiktatError(4, 16, ruleId, "${VARIABLE_NAME_INCORRECT_FORMAT.warnText()} __table", true),
+        )
+    }
+
+    @Test
+    @Tag(WarningNames.VARIABLE_NAME_INCORRECT_FORMAT)
+    fun `should trigger on backing field with unmatched type`() {
+        val code =
+            """
+                    |package com.example
+                    |
+                    |class MutableTableContainer {
+                    |   private var _table: Map<String, Double>? = 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()
+        lintMethod(code,
+            DiktatError(4, 16, ruleId, "${VARIABLE_NAME_INCORRECT_FORMAT.warnText()} _table", true),
+        )
+    }
+
     @Test
     @Tag(WarningNames.CONFUSING_IDENTIFIER_NAMING)
     fun `confusing identifier naming`() {

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
@@ -103,3 +103,4 @@ class CustomGetterSetterWarnTest : LintTestBase(::CustomGetterSetterRule) {
         )
     }
 }
+
Original file line number	Diff line number	Diff line change
Expand Up		@@ -103,3 +103,4 @@ class CustomGetterSetterWarnTest : LintTestBase(::CustomGetterSetterRule) {
		)
		}
		}