Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed VARIABLE_NAME_INCORRECT_FORMAT false positive when a property has a backing field #1810

Merged
merged 4 commits into from
Nov 22, 2023
Merged

Fixed VARIABLE_NAME_INCORRECT_FORMAT false positive when a property has a backing field #1810

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4dce6b6
### What's done:
diphtongue Nov 14, 2023
96aeac1
Closes #1711
diphtongue Nov 20, 2023
d84c98c
### What's done:
diphtongue Nov 21, 2023
f9799af
Merge branch 'master' into b
diphtongue Nov 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 41 additions & 2 deletions ...t-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter1/IdentifierNaming.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
diphtongue marked this conversation as resolved.
Show resolved Hide resolved

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
*/
Expand All @@ -149,9 +185,11 @@ class IdentifierNaming(configRules: List<RulesConfig>) : DiktatRule(
"ComplexMethod",
"UnsafeCallOnNullableType",
)

diphtongue marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down Expand Up @@ -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,
Expand Down
120 changes: 120 additions & 0 deletions ...rules/src/test/kotlin/com/saveourtool/diktat/ruleset/chapter1/IdentifierNamingWarnTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -629,6 +636,119 @@ class IdentifierNamingWarnTest : LintTestBase(::IdentifierNaming) {
lintMethod(code)
}

@Test
@Tag(WarningNames.VARIABLE_NAME_INCORRECT_FORMAT)
fun `should not trigger on backing field`() {
diphtongue marked this conversation as resolved.
Show resolved Hide resolved
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`() {
Expand Down
1 change: 1 addition & 0 deletions ...les/src/test/kotlin/com/saveourtool/diktat/ruleset/chapter6/CustomGetterSetterWarnTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,3 +103,4 @@ class CustomGetterSetterWarnTest : LintTestBase(::CustomGetterSetterRule) {
)
}
}

Loading