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.

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

Recommendation 6.1.10 no tivial getters or setters are allowed #499

Merged
merged 10 commits into from
Nov 9, 2020
4 changes: 4 additions & 0 deletions diktat-analysis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -344,5 +344,9 @@
configuration: {}
# Checks that there are abstract functions in abstract class
- name: CLASS_SHOULD_NOT_BE_ABSTRACT
enabled: true
configuration: {}
# Checks if there are any trivial getters or setters
- name: TRIVIAL_ACCESSORS_ARE_NOT_RECOMMENDED
enabled: true
configuration: {}
3 changes: 3 additions & 0 deletions diktat-rules/src/main/kotlin/generated/WarningNames.kt
Original file line number Diff line number Diff line change
Expand Up @@ -203,4 +203,7 @@ public object WarningNames {
public const val MULTIPLE_INIT_BLOCKS: String = "MULTIPLE_INIT_BLOCKS"

public const val CLASS_SHOULD_NOT_BE_ABSTRACT: String = "CLASS_SHOULD_NOT_BE_ABSTRACT"

public const val TRIVIAL_ACCESSORS_ARE_NOT_RECOMMENDED: String =
"TRIVIAL_ACCESSORS_ARE_NOT_RECOMMENDED"
}
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ enum class Warnings(private val canBeAutoCorrected: Boolean, private val warn: S
WRONG_NAME_OF_VARIABLE_INSIDE_ACCESSOR(false, "Use `field` keyword instead of property name inside property accessors"),
MULTIPLE_INIT_BLOCKS(true, "Avoid using multiple `init` blocks, this logic can be moved to constructors or properties declarations"),
CLASS_SHOULD_NOT_BE_ABSTRACT(true, "class should not be abstract, because it has no abstract functions"),
TRIVIAL_ACCESSORS_ARE_NOT_RECOMMENDED(true, "trivial property accessors are not recommended")
;

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ class DiktatRuleSetProvider(private val diktatConfigFile: String = "diktat-analy
::EnumsSeparated,
::SingleLineStatementsRule,
::MultipleModifiersSequence,
::TrivialPropertyAccessors,
// other rules
::StringTemplateFormatRule,
::DataClassesRule,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
package org.cqfn.diktat.ruleset.rules

import com.pinterest.ktlint.core.Rule
import com.pinterest.ktlint.core.ast.ElementType.BINARY_EXPRESSION
import com.pinterest.ktlint.core.ast.ElementType.BLOCK
import com.pinterest.ktlint.core.ast.ElementType.BLOCK_COMMENT
import com.pinterest.ktlint.core.ast.ElementType.EOL_COMMENT
import com.pinterest.ktlint.core.ast.ElementType.LBRACE
import com.pinterest.ktlint.core.ast.ElementType.PROPERTY_ACCESSOR
import com.pinterest.ktlint.core.ast.ElementType.RBRACE
import com.pinterest.ktlint.core.ast.ElementType.REFERENCE_EXPRESSION
import com.pinterest.ktlint.core.ast.ElementType.VALUE_PARAMETER_LIST
import com.pinterest.ktlint.core.ast.ElementType.WHITE_SPACE
import com.pinterest.ktlint.core.ast.isWhiteSpace
import org.cqfn.diktat.common.config.rules.RulesConfig
import org.cqfn.diktat.ruleset.constants.Warnings
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.hasChildOfType
import org.cqfn.diktat.ruleset.utils.prettyPrint
import org.jetbrains.kotlin.com.intellij.lang.ASTNode
import org.jetbrains.kotlin.psi.KtBinaryExpression
import org.jetbrains.kotlin.psi.KtPropertyAccessor

/**
* This rule checks if there are any trivial getters and setters and, if so, deletes them
*/
class TrivialPropertyAccessors(private val configRules: List<RulesConfig>) : Rule("trivial-property-accessors") {
private lateinit var emitWarn: ((offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit)
private var isFixMode: Boolean = false

companion object {
private val EXCESS_CHILDREN_TYPES = listOf(LBRACE, RBRACE, WHITE_SPACE, EOL_COMMENT, BLOCK_COMMENT)
}

override fun visit(node: ASTNode,
autoCorrect: Boolean,
emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit) {
emitWarn = emit
isFixMode = autoCorrect

if (node.elementType == PROPERTY_ACCESSOR) {
handlePropertyAccessors(node)
print(node.prettyPrint())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

}
}

private fun handlePropertyAccessors(node: ASTNode) {
if ((node.psi as KtPropertyAccessor).isGetter) {
handleGetAccessor(node)
} else {
handleSetAccessor(node)
}
}

@Suppress("UnsafeCallOnNullableType")
private fun handleSetAccessor(node: ASTNode) {
val valueParamName = node
.getFirstChildWithType(VALUE_PARAMETER_LIST)!!
.firstChildNode
.getIdentifierName()!!
.text

if (node.hasChildOfType(BLOCK)) {
val block = node.getFirstChildWithType(BLOCK)!!

val blockChildren = block.getChildren(null).filter { it.elementType !in EXCESS_CHILDREN_TYPES }

if (blockChildren.size == 1
&& blockChildren.first().elementType == BINARY_EXPRESSION
&& (blockChildren.first().psi as KtBinaryExpression).left?.text == "field"
&& (blockChildren.first().psi as KtBinaryExpression).right?.text == valueParamName
) {
raiseWarning(node)
}
}
}

@Suppress("UnsafeCallOnNullableType")
private fun handleGetAccessor(node: ASTNode) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment that this handles both = field and { return field }

val references = node.findAllNodesWithSpecificType(REFERENCE_EXPRESSION)
if (references.singleOrNull()?.text == "field") {
raiseWarning(node)
}
}

private fun raiseWarning(node: ASTNode) {
Warnings.TRIVIAL_ACCESSORS_ARE_NOT_RECOMMENDED.warnAndFix(configRules, emitWarn, isFixMode, node.text, node.startOffset, node) {
val property = (node.psi as KtPropertyAccessor).property.node
if (node.treePrev.isWhiteSpace()) {
property.removeChild(node.treePrev)
}
property.removeChild(node)
}
}
}
4 changes: 4 additions & 0 deletions diktat-rules/src/main/resources/diktat-analysis-huawei.yml
Original file line number Diff line number Diff line change
Expand Up @@ -343,5 +343,9 @@
configuration: {}
# Checks that there are abstract functions in abstract class
- name: CLASS_SHOULD_NOT_BE_ABSTRACT
enabled: true
configuration: {}
# Checks if there are any trivial getters or setters
- name: TRIVIAL_ACCESSORS_ARE_NOT_RECOMMENDED
enabled: true
configuration: {}
4 changes: 4 additions & 0 deletions diktat-rules/src/main/resources/diktat-analysis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -345,5 +345,9 @@
configuration: { }
# Checks that there are abstract functions in abstract class
- name: CLASS_SHOULD_NOT_BE_ABSTRACT
enabled: true
configuration: {}
# Checks if there are any trivial getters or setters
- name: TRIVIAL_ACCESSORS_ARE_NOT_RECOMMENDED
enabled: true
configuration: {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package org.cqfn.diktat.ruleset.chapter6

import generated.WarningNames
import generated.WarningNames.TRIVIAL_ACCESSORS_ARE_NOT_RECOMMENDED
import org.cqfn.diktat.util.FixTestBase
import org.cqfn.diktat.ruleset.rules.TrivialPropertyAccessors
import org.junit.jupiter.api.Tag
import org.junit.jupiter.api.Test

class TrivialPropertyAccessorsFixTest : FixTestBase("test/chapter6/properties", ::TrivialPropertyAccessors) {
@Test
@Tag(TRIVIAL_ACCESSORS_ARE_NOT_RECOMMENDED)
fun `fix trivial setters and getters`() {
fixAndCompare("TrivialPropertyAccessorsExpected.kt", "TrivialPropertyAccessorsTest.kt")
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
package org.cqfn.diktat.ruleset.chapter6

import com.pinterest.ktlint.core.LintError
import generated.WarningNames.TRIVIAL_ACCESSORS_ARE_NOT_RECOMMENDED
import org.cqfn.diktat.ruleset.constants.Warnings
import org.cqfn.diktat.ruleset.rules.DIKTAT_RULE_SET_ID
import org.cqfn.diktat.ruleset.rules.TrivialPropertyAccessors
import org.cqfn.diktat.util.LintTestBase
import org.junit.jupiter.api.Tag
import org.junit.jupiter.api.Test

class TrivialPropertyAccessorsWarnTest : LintTestBase(::TrivialPropertyAccessors) {
private val ruleId = "$DIKTAT_RULE_SET_ID:trivial-property-accessors"

@Test
@Tag(TRIVIAL_ACCESSORS_ARE_NOT_RECOMMENDED)
fun `should trigger on trivial getter and setter`() {
lintMethod(
"""
|class Test {
| val prop: Int = 0
| get() { return field }
| set(value) { field = value }
|}
""".trimMargin(),
LintError(3, 8, ruleId, "${Warnings.TRIVIAL_ACCESSORS_ARE_NOT_RECOMMENDED.warnText()} get() { return field }", true),
LintError(4, 8, ruleId, "${Warnings.TRIVIAL_ACCESSORS_ARE_NOT_RECOMMENDED.warnText()} set(value) { field = value }", true)
)
}

@Test
@Tag(TRIVIAL_ACCESSORS_ARE_NOT_RECOMMENDED)
fun `should trigger on trivial getter and setter equal case`() {
lintMethod(
"""
|class Test {
| val prop: Int = 0
| get() = field
|}
""".trimMargin(),
LintError(3, 8, ruleId, "${Warnings.TRIVIAL_ACCESSORS_ARE_NOT_RECOMMENDED.warnText()} get() = field", true)
)
}

@Test
@Tag(TRIVIAL_ACCESSORS_ARE_NOT_RECOMMENDED)
fun `should not trigger on getter and setter`() {
lintMethod(
"""
|class Test {
| val prop: Int = 0
| get() {
| val b = someLogic(field)
| return b
| }
| set(value) {
| val res = func(value)
| field = res
| }
|}
""".trimMargin()
)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package test.chapter6.properties

class Some {
var prop: Int = 0

val prop2: Int = 0

var propNotChange: Int = 7
get() { return someCoolLogic(field) }
set(value) { anotherCoolLogic(value) }
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package test.chapter6.properties

class Some {
var prop: Int = 0
get() = field
set(value) { field = value }

val prop2: Int = 0
get() { return field }

var propNotChange: Int = 7
get() { return someCoolLogic(field) }
set(value) { anotherCoolLogic(value) }
}
3 changes: 2 additions & 1 deletion info/available-rules.md
Original file line number Diff line number Diff line change
Expand Up @@ -95,4 +95,5 @@
| 6 | 6.1.2 | USE_DATA_CLASS | Check: if class can be made as data class | no | - | yes |
| 6 | 6.1.4 | MULTIPLE_INIT_BLOCKS | Checks that classes have only one init block | yes | no | - |
| 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<br>Fix: deletes abstract modifier | yes | - | - |
| 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.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 <br> Fix: Delete trivial getter or setter | yes | - | - |