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 @@ -128,6 +124,18 @@ class DataClassesRule(configRules: List<RulesConfig>) : DiktatRule(
return true
}

/**
* We do not exclude inner classes 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, enums, 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() || isEnum()

@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 @@ -239,6 +239,20 @@ class DataClassesRuleWarnTest : LintTestBase(::DataClassesRule) {
)
}

@Test
@Tag(USE_DATA_CLASS)
fun `should not trigger on enums`() {
lintMethod(
"""
Fixed Show fixed Hide fixed
|enum class Style(val str: String) {
| PASCAL_CASE("PascalCase"),
| SNAKE_CASE("UPPER_SNAKE_CASE"),
| ;
|}
""".trimMargin()
)
}

@Test
@Tag(USE_DATA_CLASS)
fun `should trigger on class with parameter in constructor`() {
Expand Down Expand Up @@ -288,6 +302,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