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

Data class rule: removing several cases from the scope of the inspection #1471

Merged
merged 8 commits into from
Jul 25, 2022
Original file line number Diff line number Diff line change
Expand Up @@ -5,27 +5,24 @@ import org.cqfn.diktat.ruleset.constants.Warnings.USE_DATA_CLASS
import org.cqfn.diktat.ruleset.rules.DiktatRule
import org.cqfn.diktat.ruleset.utils.*

import com.pinterest.ktlint.core.ast.ElementType.ABSTRACT_KEYWORD
import com.pinterest.ktlint.core.ast.ElementType.BLOCK
import com.pinterest.ktlint.core.ast.ElementType.CLASS
import com.pinterest.ktlint.core.ast.ElementType.CLASS_BODY
import com.pinterest.ktlint.core.ast.ElementType.CLASS_INITIALIZER
import com.pinterest.ktlint.core.ast.ElementType.ENUM_KEYWORD
import com.pinterest.ktlint.core.ast.ElementType.FUN
import com.pinterest.ktlint.core.ast.ElementType.INNER_KEYWORD
import com.pinterest.ktlint.core.ast.ElementType.MODIFIER_LIST
import com.pinterest.ktlint.core.ast.ElementType.OPEN_KEYWORD
import com.pinterest.ktlint.core.ast.ElementType.PRIMARY_CONSTRUCTOR
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.SEALED_KEYWORD
import com.pinterest.ktlint.core.ast.ElementType.SUPER_TYPE_LIST
import org.jetbrains.kotlin.com.intellij.lang.ASTNode
import org.jetbrains.kotlin.psi.KtClass
import org.jetbrains.kotlin.psi.KtClassBody
import org.jetbrains.kotlin.psi.KtExpression
import org.jetbrains.kotlin.psi.KtPrimaryConstructor
import org.jetbrains.kotlin.psi.psiUtil.isAbstract

/**
* This rule checks if class can be made as data class
Expand All @@ -42,11 +39,10 @@ class DataClassesRule(configRules: List<RulesConfig>) : DiktatRule(
}

private fun handleClass(node: ASTNode) {
with((node.psi as KtClass)) {
if (isData() || isInterface()) {
return
}
if ((node.psi as KtClass).isDefinitelyNotDataClass()) {
return
}

if (node.canBeDataClass()) {
raiseWarn(node)
}
Expand Down Expand Up @@ -101,14 +97,14 @@ class DataClassesRule(configRules: List<RulesConfig>) : DiktatRule(
.none { it.elementType in badModifiers } &&
classBody?.getAllChildrenWithType(FUN)
?.isEmpty()
?: false &&
?: false &&
Fixed Show fixed Hide fixed
getFirstChildWithType(SUPER_TYPE_LIST) == null
}
return classBody?.getFirstChildWithType(FUN) == null &&
getFirstChildWithType(SUPER_TYPE_LIST) == null &&
// if there is any prop with logic in accessor then don't recommend to convert class to data class
classBody?.let(::areGoodProps)
?: true
?: true
Fixed Show fixed Hide fixed
}

/**
Expand All @@ -128,6 +124,18 @@ class DataClassesRule(configRules: List<RulesConfig>) : DiktatRule(
return true
}

/**
* We do not exclude inner classes and enums here as if they have no
* methods, then we definitely can refactor the code and make them data classes.
* We only exclude: value/inline classes, annotations, interfaces, abstract classes,
* sealed classes and data classes itself. For sure there will be other corner cases,
* for example, simple classes in Spring marked with @Entity annotation.
* For these classes we expect users to Suppress warning manually for each corner case.
**/
private fun KtClass.isDefinitelyNotDataClass() =
isValue() || isAnnotation() || isInterface() || isData() ||
isSealed() || isInline() || isAbstract()

@Suppress("UnsafeCallOnNullableType")
private fun areGoodAccessors(accessors: List<ASTNode>): Boolean {
accessors.forEach {
Expand All @@ -146,6 +154,6 @@ class DataClassesRule(configRules: List<RulesConfig>) : DiktatRule(

companion object {
const val NAME_ID = "data-classes"
private val badModifiers = listOf(OPEN_KEYWORD, ABSTRACT_KEYWORD, INNER_KEYWORD, SEALED_KEYWORD, ENUM_KEYWORD)
private val badModifiers = listOf(OPEN_KEYWORD)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,58 @@ class DataClassesRuleWarnTest : LintTestBase(::DataClassesRule) {
)
}

@Test
@Tag(USE_DATA_CLASS)
fun `annotation classes bug`() {
lintMethod(
"""
|@Retention(AnnotationRetention.SOURCE)
|@Target(AnnotationTarget.CLASS)
|annotation class NavGraphDestination(
| val name: String = Defaults.NULL,
| val routePrefix: String = Defaults.NULL,
| val deepLink: Boolean = false,
|) {
| object Defaults {
| const val NULL = "@null"
| }
|}
""".trimMargin()
)
}

@Test
@Tag(USE_DATA_CLASS)
fun `value or inline classes bug`() {
lintMethod(
"""
|@JvmInline
|value class Password(private val s: String)
|val securePassword = Password("Don't try this in production")
""".trimMargin()
)
}

@Test
@Tag(USE_DATA_CLASS)
fun `sealed classes bug`() {
lintMethod(
"""
|sealed class Password(private val s: String)
""".trimMargin()
)
}

@Test
@Tag(USE_DATA_CLASS)
fun `inner classes bug`() {
lintMethod(
"""
|inner class Password(private val s: String)
""".trimMargin()
)
}

@Test
@Tag(USE_DATA_CLASS)
fun `shouldn't trigger on interface`() {
Expand Down