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

Add warning to suggest to use inline classes when a class has single property #716

Merged
merged 21 commits into from
Jan 27, 2021
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
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
3 changes: 3 additions & 0 deletions diktat-analysis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -430,4 +430,7 @@
enabled: true
# If there exists negated version of function you should prefer it instead of !functionCall
- name: INVERSE_FUNCTION_PREFERRED
enabled: true
# Checks if class can be converted to inline class
- name: INLINE_CLASS_CAN_BE_USED
enabled: true
2 changes: 2 additions & 0 deletions diktat-rules/src/main/kotlin/generated/WarningNames.kt
Original file line number Diff line number Diff line change
Expand Up @@ -239,4 +239,6 @@ public object WarningNames {
public const val OBJECT_IS_PREFERRED: String = "OBJECT_IS_PREFERRED"

public const val INVERSE_FUNCTION_PREFERRED: String = "INVERSE_FUNCTION_PREFERRED"

public const val INLINE_CLASS_CAN_BE_USED: String = "INLINE_CLASS_CAN_BE_USED"
}
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ enum class Warnings(
AVOID_USING_UTILITY_CLASS(false, "6.4.1", "avoid using utility classes/objects, use extensions functions"),
OBJECT_IS_PREFERRED(true, "6.4.2", "it is better to use object for stateless classes"),
INVERSE_FUNCTION_PREFERRED(true, "5.1.4", "it is better to use inverse function"),
INLINE_CLASS_CAN_BE_USED(true, "6.1.12", "inline class can be used"),
;

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ import org.cqfn.diktat.ruleset.rules.chapter6.UselessSupertype
import org.cqfn.diktat.ruleset.rules.chapter6.classes.AbstractClassesRule
import org.cqfn.diktat.ruleset.rules.chapter6.classes.CompactInitialization
import org.cqfn.diktat.ruleset.rules.chapter6.classes.DataClassesRule
import org.cqfn.diktat.ruleset.rules.chapter6.classes.InlineClassesRule
import org.cqfn.diktat.ruleset.rules.chapter6.classes.SingleConstructorRule
import org.cqfn.diktat.ruleset.rules.chapter6.classes.SingleInitRule
import org.cqfn.diktat.ruleset.rules.chapter6.classes.StatelessClassesRule
Expand Down Expand Up @@ -153,6 +154,7 @@ class DiktatRuleSetProvider(private var diktatConfigFile: String = DIKTAT_ANALYS
::CustomGetterSetterRule,
::CompactInitialization,
// other rules
::InlineClassesRule,
::CheckInverseMethodRule,
::StatelessClassesRule,
::ImplicitBackingPropertyRule,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
package org.cqfn.diktat.ruleset.rules.chapter6.classes

import org.cqfn.diktat.common.config.rules.RulesConfig
import org.cqfn.diktat.ruleset.constants.EmitType
import org.cqfn.diktat.ruleset.constants.Warnings.INLINE_CLASS_CAN_BE_USED
import org.cqfn.diktat.ruleset.utils.getFirstChildWithType
import org.cqfn.diktat.ruleset.utils.hasChildOfType

import com.pinterest.ktlint.core.Rule
import com.pinterest.ktlint.core.ast.ElementType.CLASS
import com.pinterest.ktlint.core.ast.ElementType.CONSTRUCTOR_CALLEE
import com.pinterest.ktlint.core.ast.ElementType.FINAL_KEYWORD
import com.pinterest.ktlint.core.ast.ElementType.INTERNAL_KEYWORD
import com.pinterest.ktlint.core.ast.ElementType.MODIFIER_LIST
import com.pinterest.ktlint.core.ast.ElementType.PRIVATE_KEYWORD
import com.pinterest.ktlint.core.ast.ElementType.PROTECTED_KEYWORD
import com.pinterest.ktlint.core.ast.ElementType.PUBLIC_KEYWORD
import com.pinterest.ktlint.core.ast.ElementType.SUPER_TYPE_LIST
import com.pinterest.ktlint.core.ast.ElementType.VAR_KEYWORD
import com.pinterest.ktlint.core.ast.children
import org.jetbrains.kotlin.com.intellij.lang.ASTNode
import org.jetbrains.kotlin.psi.KtClass
import org.jetbrains.kotlin.psi.psiUtil.visibilityModifierType

/**
* This rule checks if inline class can be used.
*/
class InlineClassesRule(private val configRule: List<RulesConfig>) : Rule("inline-classes") {
private var isFixMode: Boolean = false
private lateinit var emitWarn: EmitType

override fun visit(
node: ASTNode,
autoCorrect: Boolean,
emit: EmitType
) {
emitWarn = emit
isFixMode = autoCorrect

if (node.elementType == CLASS) {
handleClasses(node.psi as KtClass)
}
}

private fun handleClasses(classPsi: KtClass) {
// Fixme: In Kotlin 1.4.30 inline classes may be used with internal constructors. When it will be released need to check it
if (hasValidProperties(classPsi) &&
!isExtendingClass(classPsi.node) &&
classPsi.node.getFirstChildWithType(MODIFIER_LIST)?.getChildren(null)?.all { it.elementType in goodModifiers } != false) {
INLINE_CLASS_CAN_BE_USED.warnAndFix(configRule, emitWarn, isFixMode, "class ${classPsi.name}", classPsi.node.startOffset, classPsi.node) {
// Fixme: since it's an experimental feature we shouldn't do fixer
}
}
}

private fun hasValidProperties(classPsi: KtClass): Boolean {
if (classPsi.getProperties().size == 1 && !classPsi.hasExplicitPrimaryConstructor()) {
return !classPsi.getProperties().single().isVar
} else if (classPsi.getProperties().isEmpty() && classPsi.hasExplicitPrimaryConstructor()) {
return classPsi.primaryConstructorParameters.size == 1 &&
!classPsi.primaryConstructorParameters.first().node.hasChildOfType(VAR_KEYWORD) &&
classPsi.primaryConstructor?.visibilityModifierType()?.value?.let { it == "public" } ?: true
}
return false
}

private fun isExtendingClass(node: ASTNode): Boolean =
node
.getFirstChildWithType(SUPER_TYPE_LIST)
?.children()
?.any { it.hasChildOfType(CONSTRUCTOR_CALLEE) }
?: false

companion object {
val goodModifiers = listOf(PUBLIC_KEYWORD, PRIVATE_KEYWORD, FINAL_KEYWORD, PROTECTED_KEYWORD, INTERNAL_KEYWORD)
}
}
3 changes: 3 additions & 0 deletions diktat-rules/src/main/resources/diktat-analysis-huawei.yml
Original file line number Diff line number Diff line change
Expand Up @@ -430,4 +430,7 @@
enabled: true
# If there exists negated version of function you should prefer it instead of !functionCall
- name: INVERSE_FUNCTION_PREFERRED
enabled: true
# Checks if class can be converted to inline class
- name: INLINE_CLASS_CAN_BE_USED
enabled: true
3 changes: 3 additions & 0 deletions diktat-rules/src/main/resources/diktat-analysis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -427,4 +427,7 @@
enabled: true
# If there exists negated version of function you should prefer it instead of !functionCall
- name: INVERSE_FUNCTION_PREFERRED
enabled: true
# Checks if class can be converted to inline class
- name: INLINE_CLASS_CAN_BE_USED
enabled: true
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
package org.cqfn.diktat.ruleset.chapter6

import org.cqfn.diktat.ruleset.constants.Warnings.INLINE_CLASS_CAN_BE_USED
import org.cqfn.diktat.ruleset.rules.DIKTAT_RULE_SET_ID
import org.cqfn.diktat.ruleset.rules.chapter6.classes.InlineClassesRule
import org.cqfn.diktat.util.LintTestBase

import com.pinterest.ktlint.core.LintError
import generated.WarningNames
import org.junit.jupiter.api.Tag
import org.junit.jupiter.api.Test

class InlineClassesWarnTest : LintTestBase(::InlineClassesRule) {
private val ruleId = "$DIKTAT_RULE_SET_ID:inline-classes"

@Test
@Tag(WarningNames.INLINE_CLASS_CAN_BE_USED)
fun `should not trigger on inline class`() {
lintMethod(
"""
|inline class Name(val s: String) {}
""".trimMargin()
)
}

@Test
@Tag(WarningNames.INLINE_CLASS_CAN_BE_USED)
fun `should trigger on regular class`() {
lintMethod(
"""
|class Some {
| val config = Config()
|}
""".trimMargin(),
LintError(1, 1, ruleId, "${INLINE_CLASS_CAN_BE_USED.warnText()} class Some", true)
)
}

@Test
@Tag(WarningNames.INLINE_CLASS_CAN_BE_USED)
fun `should trigger on class with appropriate modifiers`() {
lintMethod(
"""
|final class Some {
| val config = Config()
|}
""".trimMargin(),
LintError(1, 1, ruleId, "${INLINE_CLASS_CAN_BE_USED.warnText()} class Some", true)
)
}

@Test
@Tag(WarningNames.INLINE_CLASS_CAN_BE_USED)
fun `should not trigger on class with inappropriate modifiers`() {
lintMethod(
"""
|abstract class Some {
| val config = Config()
|}
""".trimMargin()
)
}

@Test
@Tag(WarningNames.INLINE_CLASS_CAN_BE_USED)
fun `should trigger on class with val prop in constructor`() {
lintMethod(
"""
|class Some(val anything: Int) {
|
|}
""".trimMargin(),
LintError(1, 1, ruleId, "${INLINE_CLASS_CAN_BE_USED.warnText()} class Some", true)
)
}

@Test
@Tag(WarningNames.INLINE_CLASS_CAN_BE_USED)
fun `should not trigger on class with var prop #1`() {
lintMethod(
"""
|class Some(var anything: Int) {
|
|}
""".trimMargin()
)
}

@Test
@Tag(WarningNames.INLINE_CLASS_CAN_BE_USED)
fun `should not trigger on class with var prop #2`() {
lintMethod(
"""
|class Some {
| var some = 3
|}
""".trimMargin()
)
}

@Test
@Tag(WarningNames.INLINE_CLASS_CAN_BE_USED)
fun `should not trigger on class that extends class`() {
lintMethod(
"""
|class Some : Any() {
| val some = 3
|}
""".trimMargin()
)
}

@Test
@Tag(WarningNames.INLINE_CLASS_CAN_BE_USED)
fun `should trigger on class that extends interface`() {
lintMethod(
"""
|class Some : Any {
| val some = 3
|}
""".trimMargin(),
LintError(1, 1, ruleId, "${INLINE_CLASS_CAN_BE_USED.warnText()} class Some", true)
)
}

@Test
@Tag(WarningNames.INLINE_CLASS_CAN_BE_USED)
fun `should not trigger on class with internal constructor`() {
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure it's prohibited? IDEA doesn't seem to highlight it in red.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As IDEA highlighted, constructor should be public, otherwise class can't be inline

Copy link
Member

Choose a reason for hiding this comment

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

idk, my idea doesn't :)
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Strange, I have this
image
IntelliJ IDEA 2020.3 (Ultimate Edition)
Build #IU-203.5981.155, built on December 1, 2020

Copy link
Member

Choose a reason for hiding this comment

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

hmm, do you try this code in diktat? I would expect results to be the same with same kotlin version. Which version of kotlin plugin do you have in idea? My is 203-1.4.30-RC-IJ6682.9 for idea 2020.3.2. Let's check if this code compiles with kotlin 1.4.21.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now I have 1.4.21 and this code doesn't compile

Copy link
Member

Choose a reason for hiding this comment

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

OK, that is a bug in IDEA plugin. Maybe, it'll be allowed in 1.4.30, but let's allow only public for now.

lintMethod(
"""
|class LocalCommandExecutor internal constructor(private val command: String) {
|
|}
""".trimMargin()
)
}

@Test
@Tag(WarningNames.INLINE_CLASS_CAN_BE_USED)
fun `should trigger on class with public constructor`() {
lintMethod(
"""
|class LocalCommandExecutor public constructor(private val command: String) {
|
|}
""".trimMargin(),
LintError(1, 1, ruleId, "${INLINE_CLASS_CAN_BE_USED.warnText()} class LocalCommandExecutor", true)
)
}

@Test
@Tag(WarningNames.INLINE_CLASS_CAN_BE_USED)
fun `should trigger on class with annotation before the constructor`() {
lintMethod(
"""
|class LocalCommandExecutor @Inject constructor(private val command: String) {
|
|}
""".trimMargin(),
LintError(1, 1, ruleId, "${INLINE_CLASS_CAN_BE_USED.warnText()} class LocalCommandExecutor", true)
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,10 @@ import kotlinx.serialization.encodeToString
* Special test that checks that developer has not forgotten to add his warning to a diktat-analysis.yml
* This file is needed to be in tact with latest changes in Warnings.kt
*/
class RulesConfigYamlTest {
private val pathMap = mapOf("diktat-analysis.yml" to "diKTat/diktat-rules/src/main/resources/diktat-analysis.yml",
"diktat-analysis-huawei.yml" to "diKTat/diktat-rules/src/main/resources/diktat-analysis-huawei.yml",
"parent/diktat-analysis.yml" to "diKTat/diktat-analysis.yml")

inline class RulesConfigYamlTest(private val pathMap: Map<String, String> =
mapOf("diktat-analysis.yml" to "diKTat/diktat-rules/src/main/resources/diktat-analysis.yml",
"diktat-analysis-huawei.yml" to "diKTat/diktat-rules/src/main/resources/diktat-analysis-huawei.yml",
"parent/diktat-analysis.yml" to "diKTat/diktat-analysis.yml")) {
@Test
fun `read rules config yml`() {
compareRulesAndConfig("diktat-analysis.yml")
Expand Down
1 change: 1 addition & 0 deletions info/available-rules.md
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@
| 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 | no | - |
| 6 | 6.1.8 | CUSTOM_GETTERS_SETTERS | Check: Inspection that checks that no custom getters and setters are used for properties | no | no | - |
| 6 | 6.1.11 | COMPACT_OBJECT_INITIALIZATION | Checks if class instantiation can be wrapped in `apply` for better readability | | no | |
| 6 | 6.1.12 | INLINE_CLASS_CAN_BE_USED | Check: warns if class can be transferred to the inline class. | no | no | yes |
| 6 | 6.2.2 | EXTENSION_FUNCTION_SAME_SIGNATURE | Checks if extension function has the same signature as another extension function and their classes are related | no | no | + |
| 6 | 6.4.1 | AVOID_USING_UTILITY_CLASS | Checks if there is class/object that can be replace with extension function | no | no | - |
| 6 | 6.4.2 | OBJECT_IS_PREFERRED | Checks: if class is stateless it is preferred to use `object` | yes | no | + |
1 change: 1 addition & 0 deletions info/guide/guide-TOC.md
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ I [Preface](#c0)
* [6.1.9 Never use the name of a variable in the custom getter or setter (possible_bug)](#r6.1.9)
* [6.1.10 No trivial getters and setters are allowed in the code](#r6.1.10)
* [6.1.11 Use 'apply' for grouping object initialization](#r6.1.11)
* [6.1.12 Prefer Inline classes when a class has a single property](#r6.1.12)
* [6.2 Classes](#c6.2)
* [6.2.1 Use extension functions for making logic of classes less coupled](#r6.2.1)
* [6.2.2 No extension functions with the same name and signature if they extend base and inheritor classes (possible_bug)](#r6.2.2)
Expand Down
17 changes: 17 additions & 0 deletions info/guide/guide-chapter-6.md
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,23 @@ fun main() {
}
```

### <a name="r6.1.12"></a> 6.1.12 Prefer Inline classes when a class has a single property
Copy link
Member

@orchestr7 orchestr7 Jan 26, 2021

Choose a reason for hiding this comment

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

example?
Bad example - Good example

and more info about inline classes

If a class has only one immutable property, then it can be converted to the inline class.

Sometimes it is necessary for business logic to create a wrapper around some type. However, it introduces runtime overhead due to additional heap allocations. Moreover, if the wrapped type is primitive, the performance hit is terrible, because primitive types are usually heavily optimized by the runtime, while their wrappers don't get any special treatment.

**Invalid example**:
```kotlin
class Password {
val value: String
}
```

**Valid example**:
```kotlin
inline class Password(val value: String)
```

<!-- =============================================================================== -->
### <a name="c6.2"></a>6.2 Extension functions
This section describes the rules of using extension functions in your code.
Expand Down