From b01cd644b895b2700b58b722ed72fc0204c23c5c Mon Sep 17 00:00:00 2001 From: Daria Pleshkova <81246075+diphtongue@users.noreply.github.com> Date: Wed, 22 Nov 2023 13:52:47 +0300 Subject: [PATCH] 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 --- .../rules/chapter1/IdentifierNaming.kt | 43 ++++++- .../chapter1/IdentifierNamingWarnTest.kt | 120 ++++++++++++++++++ .../chapter6/CustomGetterSetterWarnTest.kt | 1 + 3 files changed, 162 insertions(+), 2 deletions(-) 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 c74031d75d..5dc57cd8dc 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,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) : 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) : DiktatRule( "ComplexMethod", "UnsafeCallOnNullableType", ) + private fun checkVariableName(node: ASTNode): List { // 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) : 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/diktat-rules/src/test/kotlin/com/saveourtool/diktat/ruleset/chapter1/IdentifierNamingWarnTest.kt b/diktat-rules/src/test/kotlin/com/saveourtool/diktat/ruleset/chapter1/IdentifierNamingWarnTest.kt index 39352bd853..180f0e64e8 100644 --- a/diktat-rules/src/test/kotlin/com/saveourtool/diktat/ruleset/chapter1/IdentifierNamingWarnTest.kt +++ b/diktat-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? = 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(WarningNames.VARIABLE_NAME_INCORRECT_FORMAT) + fun `should trigger on backing field with setter`() { + val code = + """ + |package com.example + | + |class MutableTableContainer { + | private 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(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? = 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() + 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? = 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() + 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/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 5a12249803..20244f6ec9 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 @@ -103,3 +103,4 @@ class CustomGetterSetterWarnTest : LintTestBase(::CustomGetterSetterRule) { ) } } +