diff --git a/diktat-analysis.yml b/diktat-analysis.yml index df66840744..4d6ea1c15d 100644 --- a/diktat-analysis.yml +++ b/diktat-analysis.yml @@ -300,4 +300,9 @@ enabled: true # Checks if there is empty primary constructor - name: EMPTY_PRIMARY_CONSTRUCTOR + enabled: true +# In case of not using field keyword in property accessors, +# there should be explicit backing property with the name of real property +# Example: val table get() {if (_table == null) ...} -> table should have _table +- name: NO_CORRESPONDING_PROPERTY enabled: true \ No newline at end of file diff --git a/diktat-rules/src/main/kotlin/generated/WarningNames.kt b/diktat-rules/src/main/kotlin/generated/WarningNames.kt index 387e6f33a4..9a7f3d4ce0 100644 --- a/diktat-rules/src/main/kotlin/generated/WarningNames.kt +++ b/diktat-rules/src/main/kotlin/generated/WarningNames.kt @@ -221,4 +221,6 @@ public object WarningNames { public const val EXTENSION_FUNCTION_SAME_SIGNATURE: String = "EXTENSION_FUNCTION_SAME_SIGNATURE" public const val EMPTY_PRIMARY_CONSTRUCTOR: String = "EMPTY_PRIMARY_CONSTRUCTOR" + + public const val NO_CORRESPONDING_PROPERTY: String = "NO_CORRESPONDING_PROPERTY" } diff --git a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/constants/Warnings.kt b/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/constants/Warnings.kt index 6bf1d364fe..14449b4a9b 100644 --- a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/constants/Warnings.kt +++ b/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/constants/Warnings.kt @@ -131,6 +131,7 @@ enum class Warnings(val canBeAutoCorrected: Boolean, val ruleId: String, private TRIVIAL_ACCESSORS_ARE_NOT_RECOMMENDED(true, "6.1.10", "trivial property accessors are not recommended"), EXTENSION_FUNCTION_SAME_SIGNATURE(false, "6.2.2", "extension functions should not have same signature if their receiver classes are related"), EMPTY_PRIMARY_CONSTRUCTOR(true,"6.1.3", "avoid empty primary constructor"), + NO_CORRESPONDING_PROPERTY(false, "6.1.7", "backing property should have the same name, but there is no corresponding property") ; /** diff --git a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/DiktatRuleSetProvider.kt b/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/DiktatRuleSetProvider.kt index 64d98bf7e9..d84fb58c48 100644 --- a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/DiktatRuleSetProvider.kt +++ b/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/DiktatRuleSetProvider.kt @@ -79,6 +79,7 @@ class DiktatRuleSetProvider(private val diktatConfigFile: String = "diktat-analy ::CustomGetterSetterRule, ::CompactInitialization, // other rules + ::ImplicitBackingPropertyRule, ::StringTemplateFormatRule, ::DataClassesRule, ::LocalVariablesRule, diff --git a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/ImplicitBackingPropertyRule.kt b/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/ImplicitBackingPropertyRule.kt new file mode 100644 index 0000000000..2677036941 --- /dev/null +++ b/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/ImplicitBackingPropertyRule.kt @@ -0,0 +1,110 @@ +package org.cqfn.diktat.ruleset.rules + +import com.pinterest.ktlint.core.Rule +import com.pinterest.ktlint.core.ast.ElementType.BLOCK +import com.pinterest.ktlint.core.ast.ElementType.CLASS_BODY +import com.pinterest.ktlint.core.ast.ElementType.DOT_QUALIFIED_EXPRESSION +import com.pinterest.ktlint.core.ast.ElementType.FILE +import com.pinterest.ktlint.core.ast.ElementType.GET_KEYWORD +import com.pinterest.ktlint.core.ast.ElementType.IDENTIFIER +import com.pinterest.ktlint.core.ast.ElementType.PROPERTY +import com.pinterest.ktlint.core.ast.ElementType.PROPERTY_ACCESSOR +import com.pinterest.ktlint.core.ast.ElementType.REFERENCE_EXPRESSION +import com.pinterest.ktlint.core.ast.ElementType.RETURN +import com.pinterest.ktlint.core.ast.ElementType.SET_KEYWORD +import com.pinterest.ktlint.core.ast.ElementType.VALUE_PARAMETER +import org.cqfn.diktat.common.config.rules.RulesConfig +import org.cqfn.diktat.ruleset.constants.Warnings.NO_CORRESPONDING_PROPERTY +import org.cqfn.diktat.ruleset.utils.findAllNodesWithSpecificType +import org.cqfn.diktat.ruleset.utils.getFirstChildWithType +import org.cqfn.diktat.ruleset.utils.getIdentifierName +import org.cqfn.diktat.ruleset.utils.hasAnyChildOfTypes +import org.cqfn.diktat.ruleset.utils.hasChildOfType +import org.cqfn.diktat.ruleset.utils.prettyPrint +import org.jetbrains.kotlin.com.intellij.lang.ASTNode +import org.jetbrains.kotlin.psi.KtProperty + +/** + * This rule checks if there is a backing property for field with property accessors, in case they don't use field keyword + */ +class ImplicitBackingPropertyRule(private val configRules: List) : Rule("implicit-backing-property") { + private lateinit var emitWarn: ((offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit) + private var isFixMode: Boolean = false + + override fun visit(node: ASTNode, + autoCorrect: Boolean, + emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit) { + emitWarn = emit + isFixMode = autoCorrect + + if (node.elementType == CLASS_BODY) { + findAllProperties(node) + } + } + + @Suppress("UnsafeCallOnNullableType") + private fun findAllProperties(node: ASTNode) { + val properties = node.getChildren(null).filter { it.elementType == PROPERTY } + + val propsWithBackSymbol = properties + .filter { it.getFirstChildWithType(IDENTIFIER)!!.text.startsWith("_") } + .map { + it.getFirstChildWithType(IDENTIFIER)!!.text + } + + properties.filter { it.hasAnyChildOfTypes(PROPERTY_ACCESSOR) }.forEach { + validateAccessors(it, propsWithBackSymbol) + } + } + + private fun validateAccessors(node: ASTNode, propsWithBackSymbol: List) { + val accessors = node.findAllNodesWithSpecificType(PROPERTY_ACCESSOR).filter { it.hasChildOfType(BLOCK) } // exclude get with expression body + + accessors.filter { it.hasChildOfType(GET_KEYWORD) }.forEach { handleGetAccessors(it, node, propsWithBackSymbol) } + accessors.filter { it.hasChildOfType(SET_KEYWORD) }.forEach { handleSetAccessors(it, node, propsWithBackSymbol) } + } + + @Suppress("UnsafeCallOnNullableType") + private fun handleGetAccessors(accessor: ASTNode, node: ASTNode, propsWithBackSymbol: List) { + val refExprs = accessor + .findAllNodesWithSpecificType(RETURN) + .filterNot { it.hasChildOfType(DOT_QUALIFIED_EXPRESSION) } + .flatMap { it.findAllNodesWithSpecificType(REFERENCE_EXPRESSION) } + + val localProps = accessor + .findAllNodesWithSpecificType(PROPERTY) + .map { (it.psi as KtProperty).name!! } + // If refExprs is empty then we assume that it returns some constant + if (refExprs.isNotEmpty()) { + handleReferenceExpressions(node, refExprs, propsWithBackSymbol, localProps) + } + } + + @Suppress("UnsafeCallOnNullableType") + private fun handleSetAccessors(accessor: ASTNode, node: ASTNode, propsWithBackSymbol: List) { + val refExprs = accessor.findAllNodesWithSpecificType(REFERENCE_EXPRESSION) + + // In set we don't check for local properties. At least one reference expression should contain field or _prop + if (refExprs.isNotEmpty()) { + handleReferenceExpressions(node, refExprs, propsWithBackSymbol, null) + } + } + + @Suppress("UnsafeCallOnNullableType") + private fun handleReferenceExpressions(node: ASTNode, + expressions: List, + backingPropertiesNames: List, + localProperties: List?) { + if (expressions.none { + backingPropertiesNames.contains(it.text) || it.text == "field" || localProperties?.contains(it.text) == true + }) { + raiseWarning(node, node.getFirstChildWithType(IDENTIFIER)!!.text) + } + } + + private fun raiseWarning(node: ASTNode, propName: String) { + NO_CORRESPONDING_PROPERTY.warn(configRules, emitWarn, isFixMode, + "$propName has no corresponding property with name _$propName", node.startOffset, node) + } + +} diff --git a/diktat-rules/src/main/resources/diktat-analysis-huawei.yml b/diktat-rules/src/main/resources/diktat-analysis-huawei.yml index 9be9160d39..abacac11c0 100644 --- a/diktat-rules/src/main/resources/diktat-analysis-huawei.yml +++ b/diktat-rules/src/main/resources/diktat-analysis-huawei.yml @@ -298,4 +298,9 @@ enabled: true # Checks if there is empty primary constructor - name: EMPTY_PRIMARY_CONSTRUCTOR + enabled: true +# In case of not using field keyword in property accessors, +# there should be explicit backing property with the name of real property +# Example: val table get() {if (_table == null) ...} -> table should have _table +- name: NO_CORRESPONDING_PROPERTY enabled: true \ No newline at end of file diff --git a/diktat-rules/src/main/resources/diktat-analysis.yml b/diktat-rules/src/main/resources/diktat-analysis.yml index 9b5b082ba9..385c80565e 100644 --- a/diktat-rules/src/main/resources/diktat-analysis.yml +++ b/diktat-rules/src/main/resources/diktat-analysis.yml @@ -300,4 +300,9 @@ enabled: true # Checks if there is empty primary constructor - name: EMPTY_PRIMARY_CONSTRUCTOR + enabled: true +# In case of not using field keyword in property accessors, +# there should be explicit backing property with the name of real property +# Example: val table get() {if (_table == null) ...} -> table should have _table +- name: NO_CORRESPONDING_PROPERTY enabled: true \ No newline at end of file diff --git a/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter6/ImplicitBackingPropertyWarnTest.kt b/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter6/ImplicitBackingPropertyWarnTest.kt new file mode 100644 index 0000000000..54cc0cf0b2 --- /dev/null +++ b/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter6/ImplicitBackingPropertyWarnTest.kt @@ -0,0 +1,165 @@ +package org.cqfn.diktat.ruleset.chapter6 + +import com.pinterest.ktlint.core.LintError +import generated.WarningNames.NO_CORRESPONDING_PROPERTY +import org.cqfn.diktat.ruleset.constants.Warnings +import org.cqfn.diktat.ruleset.rules.DIKTAT_RULE_SET_ID +import org.cqfn.diktat.ruleset.rules.ImplicitBackingPropertyRule +import org.cqfn.diktat.util.LintTestBase +import org.junit.jupiter.api.Tag +import org.junit.jupiter.api.Test + +class ImplicitBackingPropertyWarnTest: LintTestBase(::ImplicitBackingPropertyRule) { + private val ruleId = "$DIKTAT_RULE_SET_ID:implicit-backing-property" + + @Test + @Tag(NO_CORRESPONDING_PROPERTY) + fun `not trigger on backing property`() { + lintMethod( + """ + |class Some(val a: Int = 5) { + | private var _table: Map? = null + | val table:Map + | get() { + | if (_table == null) { + | _table = HashMap() + | } + | return _table ?: throw AssertionError("Set to null by another thread") + | } + | set(value) { field = value } + |} + """.trimMargin() + ) + } + + @Test + @Tag(NO_CORRESPONDING_PROPERTY) + fun `trigger on backing property`() { + lintMethod( + """ + |class Some(val a: Int = 5) { + | private var a: Map? = null + | val table:Map + | get() { + | if (a == null) { + | a = HashMap() + | } + | return a ?: throw AssertionError("Set to null by another thread") + | } + | set(value) { field = value } + |} + """.trimMargin(), + LintError(3,4,ruleId, "${Warnings.NO_CORRESPONDING_PROPERTY.warnText()} table has no corresponding property with name _table") + ) + } + + @Test + @Tag(NO_CORRESPONDING_PROPERTY) + fun `don't trigger on regular backing property`() { + lintMethod( + """ + |class Some(val a: Int = 5) { + | private var _a: Map? = null + | private val _some:Int? = null + |} + """.trimMargin() + ) + } + + @Test + @Tag(NO_CORRESPONDING_PROPERTY) + fun `don't trigger on regular property`() { + lintMethod( + """ + |class Some(val a: Int = 5) { + | private var a: Map? = null + | private val some:Int? = null + | private val _prop: String? = null + |} + """.trimMargin() + ) + } + + @Test + @Tag(NO_CORRESPONDING_PROPERTY) + fun `should not trigger if property has field in accessor`() { + lintMethod( + """ + |class Some(val a: Int = 5) { + | val table:Map + | set(value) { field = value } + | val _table: Map? = null + | + | val some: Int + | get() = 3 + |} + """.trimMargin() + ) + } + + @Test + @Tag(NO_CORRESPONDING_PROPERTY) + fun `should not trigger on property with constant return`() { + lintMethod( + """ + |class Some(val a: Int = 5) { + | val table:Int + | get() { + | return 3 + | } + | set(value) { field = value } + |} + """.trimMargin() + ) + } + + @Test + @Tag(NO_CORRESPONDING_PROPERTY) + fun `should not trigger on property with chain call return`() { + lintMethod( + """ + |class Some(val a: Int = 5) { + | val table:Int + | get() { + | val some = listOf(1,2,3) + | return some.filter { it -> it == 3}.first() + | } + | set(value) { field = value } + |} + """.trimMargin() + ) + } + + @Test + @Tag(NO_CORRESPONDING_PROPERTY) + fun `should not trigger set accessor`() { + lintMethod( + """ + |class Some(val a: Int = 5) { + | val foo + | set(value) { + | if(isDelegate) log.debug(value) + | field = value + | } + |} + """.trimMargin() + ) + } + + @Test + @Tag(NO_CORRESPONDING_PROPERTY) + fun `should trigger set accessor`() { + lintMethod( + """ + |class Some(val a: Int = 5) { + | val foo + | set(value) { + | if(isDelegate) log.debug(value) + | a = value + | } + |} + """.trimMargin(), + LintError(2,4 ,ruleId, "${Warnings.NO_CORRESPONDING_PROPERTY.warnText()} foo has no corresponding property with name _foo") + ) + } +} diff --git a/info/available-rules.md b/info/available-rules.md index e1e15fdf8d..7f513d8c1d 100644 --- a/info/available-rules.md +++ b/info/available-rules.md @@ -99,6 +99,7 @@ | 6 | 6.1.4 | MULTIPLE_INIT_BLOCKS | Checks that classes have only one init block | yes | no | - | | 6 | 6.1.5 | USELESS_SUPERTYPE | Check: if override function can be removed | yes| - | | | 6 | 6.1.6 | CLASS_SHOULD_NOT_BE_ABSTRACT | Checks: if abstract class has any abstract method. If not, warns that class should not be abstract
Fix: deletes abstract modifier | yes | - | - | +| 6 | 6.1.7 | NO_CORRESPONDING_PROPERTY | Checks: if in case of using "backing property" scheme, the name of real and back property are the same | no | - | - | | 6 | 6.1.9 | WRONG_NAME_OF_VARIABLE_INSIDE_ACCESSOR | Check: used the name of a variable in the custom getter or setter | no | - | | 6 | 6.1.10 | TRIVIAL_ACCESSORS_ARE_NOT_RECOMMENDED | Check: if there are any trivial getters or setters
Fix: Delete trivial getter or setter | yes | - | - | | 6 | 6.1.8 | CUSTOM_GETTERS_SETTERS | Check: Inspection that checks that no custom getters and setters are used for properties | no | - | - |