azalea/kotlin-fork
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Sealed sub-class -> object: handle equals, suggest "add equals"

Browse Source 
Before this commit, sealed sub-class without state was considered
a style issue.
After this commit, sealed sub-class without state AND custom equals
is considered a probable bug,
because comparison of its instances is very fragile.
Alternative fix (generate equals & hashCode by identity) is added.
This commit is contained in:
Mikhail Glukhikh
2018-06-13 18:05:46 +03:00
parent ecd2709a11
commit 9ef89447b3
27 changed files with 222 additions and 51 deletions
Download Patch File Download Diff File Expand all files Collapse all files
idea/resources/inspectionDescriptions/CanSealedSubClassBeObject.html
+2 -1
View File
@@ -1,5 +1,6 @@
<html>
<body>
This inspection reports direct inheritors of <b>sealed class</b> that have no state and thus can be objects.
This inspection reports direct inheritors of <b>sealed class</b> that have no state and no overridden <b>equals</b>.
It's highly recommended to override <b>equals</b> to provide comparison stability or convert <b>class</b> to <b>object</b> with the same effect.
</body>
</html>
idea/src/META-INF/plugin.xml
+3 -3
View File
@@ -2838,11 +2838,11 @@ The Kotlin plugin provides language support in IntelliJ IDEA and Android Studio.
/>
<localInspection implementationClass="org.jetbrains.kotlin.idea.inspections.CanSealedSubClassBeObjectInspection"
displayName="Refactor Sealed Sub-class to Object"
displayName="Sealed sub-class without state and overridden equals"
groupPath="Kotlin"
groupName="Style issues"
groupName="Probable bugs"
enabledByDefault="true"
level="INFO"
level="WEAK WARNING"
language="kotlin"
/>
idea/src/META-INF/plugin.xml.172
+3 -3
View File
@@ -2836,11 +2836,11 @@ The Kotlin plugin provides language support in IntelliJ IDEA and Android Studio.
/>
<localInspection implementationClass="org.jetbrains.kotlin.idea.inspections.CanSealedSubClassBeObjectInspection"
displayName="Refactor Sealed Sub-class to Object"
displayName="Sealed sub-class without state and overridden equals"
groupPath="Kotlin"
groupName="Style issues"
groupName="Probable bugs"
enabledByDefault="true"
level="INFO"
level="WEAK WARNING"
language="kotlin"
/>
idea/src/META-INF/plugin.xml.173
+3 -3
View File
@@ -2836,11 +2836,11 @@ The Kotlin plugin provides language support in IntelliJ IDEA and Android Studio.
/>
<localInspection implementationClass="org.jetbrains.kotlin.idea.inspections.CanSealedSubClassBeObjectInspection"
displayName="Refactor Sealed Sub-class to Object"
displayName="Sealed sub-class without state and overridden equals"
groupPath="Kotlin"
groupName="Style issues"
groupName="Probable bugs"
enabledByDefault="true"
level="INFO"
level="WEAK WARNING"
language="kotlin"
/>
idea/src/META-INF/plugin.xml.182
+3 -3
View File
@@ -2837,11 +2837,11 @@ The Kotlin plugin provides language support in IntelliJ IDEA and Android Studio.
/>
<localInspection implementationClass="org.jetbrains.kotlin.idea.inspections.CanSealedSubClassBeObjectInspection"
displayName="Refactor Sealed Sub-class to Object"
displayName="Sealed sub-class without state and overridden equals"
groupPath="Kotlin"
groupName="Style issues"
groupName="Probable bugs"
enabledByDefault="true"
level="INFO"
level="WEAK WARNING"
language="kotlin"
/>
idea/src/META-INF/plugin.xml.as31
+3 -3
View File
@@ -2836,11 +2836,11 @@ The Kotlin plugin provides language support in IntelliJ IDEA and Android Studio.
/>
<localInspection implementationClass="org.jetbrains.kotlin.idea.inspections.CanSealedSubClassBeObjectInspection"
displayName="Refactor Sealed Sub-class to Object"
displayName="Sealed sub-class without state and overridden equals"
groupPath="Kotlin"
groupName="Style issues"
groupName="Probable bugs"
enabledByDefault="true"
level="INFO"
level="WEAK WARNING"
language="kotlin"
/>
idea/src/META-INF/plugin.xml.as32
+3 -3
View File
@@ -2836,11 +2836,11 @@ The Kotlin plugin provides language support in IntelliJ IDEA and Android Studio.
/>
<localInspection implementationClass="org.jetbrains.kotlin.idea.inspections.CanSealedSubClassBeObjectInspection"
displayName="Refactor Sealed Sub-class to Object"
displayName="Sealed sub-class without state and overridden equals"
groupPath="Kotlin"
groupName="Style issues"
groupName="Probable bugs"
enabledByDefault="true"
level="INFO"
level="WEAK WARNING"
language="kotlin"
/>
idea/src/org/jetbrains/kotlin/idea/inspections/CanSealedSubClassBeObjectInspection.kt
+32 -17
View File
@@ -23,15 +23,18 @@ import org.jetbrains.kotlin.asJava.classes.KtLightClassImpl
import org.jetbrains.kotlin.idea.caches.resolve.resolveToDescriptorIfAny
import org.jetbrains.kotlin.idea.core.getModalityFromDescriptor
import org.jetbrains.kotlin.idea.quickfix.sealedSubClassToObject.ConvertSealedSubClassToObjectFix
import org.jetbrains.kotlin.idea.quickfix.sealedSubClassToObject.GenerateIdentityEqualsFix
import org.jetbrains.kotlin.idea.refactoring.isAbstract
import org.jetbrains.kotlin.idea.search.declarationsSearch.HierarchySearchRequest
import org.jetbrains.kotlin.idea.search.declarationsSearch.searchInheritors
import org.jetbrains.kotlin.lexer.KtTokens
import org.jetbrains.kotlin.psi.KtClass
import org.jetbrains.kotlin.psi.KtNamedFunction
import org.jetbrains.kotlin.psi.KtProperty
import org.jetbrains.kotlin.psi.KtVisitorVoid
import org.jetbrains.kotlin.resolve.DescriptorToSourceUtils
import org.jetbrains.kotlin.resolve.descriptorUtil.getSuperClassNotAny
import org.jetbrains.kotlin.util.OperatorNameConventions
class CanSealedSubClassBeObjectInspection : AbstractKotlinInspection() {
override fun buildVisitor(holder: ProblemsHolder, isOnTheFly: Boolean, session: LocalInspectionToolSession): PsiElementVisitor {
@@ -46,8 +49,8 @@ class CanSealedSubClassBeObjectInspection : AbstractKotlinInspection() {
.thatHasNoTypeParameters()
.thatHasNoInnerClasses()
.thatHasNoCompanionObjects()
.thatHasNoState()
if (candidates.isEmpty() || !klass.hasNoState() || !klass.baseClassHasNoState()) return
.thatHasNoStateOrEquals()
if (candidates.isEmpty() || !klass.hasNoStateOrEquals() || !klass.baseClassHasNoStateOrEquals()) return
candidates.forEach { reportPossibleObject(it) }
}
@@ -56,22 +59,15 @@ class CanSealedSubClassBeObjectInspection : AbstractKotlinInspection() {
val keyword = klass.getClassOrInterfaceKeyword() ?: return
holder.registerProblem(
keyword,
"Sealed Sub-class should be changed To Object",
"Sealed sub-class has no state and no overridden equals",
ProblemHighlightType.GENERIC_ERROR_OR_WARNING,
ConvertSealedSubClassToObjectFix()
ConvertSealedSubClassToObjectFix(),
GenerateIdentityEqualsFix()
)
}
}
}
private tailrec fun KtClass.baseClassHasNoState(): Boolean {
val descriptor = resolveToDescriptorIfAny() ?: return false
val superDescriptor = descriptor.getSuperClassNotAny() ?: return true // No super class -- no state
val superClass = DescriptorToSourceUtils.descriptorToDeclaration(superDescriptor) as? KtClass ?: return false
if (!superClass.hasNoState()) return false
return superClass.baseClassHasNoState()
}
private fun KtClass.getSubclasses(): List<KtLightClassImpl> {
return HierarchySearchRequest(this, this.useScope, false)
.searchInheritors().filterIsInstance<KtLightClassImpl>()
@@ -99,16 +95,24 @@ class CanSealedSubClassBeObjectInspection : AbstractKotlinInspection() {
return filter { klass -> klass.hasNoInnerClass() }
}
private fun List<KtClass>.thatHasNoState(): List<KtClass> {
return filter { it.hasNoState() }
private fun List<KtClass>.thatHasNoStateOrEquals(): List<KtClass> {
return filter { it.hasNoStateOrEquals() }
}
private fun KtClass.hasNoState(): Boolean {
private tailrec fun KtClass.baseClassHasNoStateOrEquals(): Boolean {
val descriptor = resolveToDescriptorIfAny() ?: return false
val superDescriptor = descriptor.getSuperClassNotAny() ?: return true // No super class -- no state
val superClass = DescriptorToSourceUtils.descriptorToDeclaration(superDescriptor) as? KtClass ?: return false
if (!superClass.hasNoStateOrEquals()) return false
return superClass.baseClassHasNoStateOrEquals()
}
private fun KtClass.hasNoStateOrEquals(): Boolean {
if (primaryConstructor?.valueParameters?.isNotEmpty() == true) return false
val body = getBody()
return body == null || run {
val properties = body.declarations.filterIsInstance<KtProperty>()
properties.none { property ->
val declarations = body.declarations
declarations.filterIsInstance<KtProperty>().none { property ->
// Simplified "backing field required"
when {
property.isAbstract() -> false
@@ -117,6 +121,11 @@ class CanSealedSubClassBeObjectInspection : AbstractKotlinInspection() {
!property.isVar -> property.getter == null
else -> property.getter == null || property.setter == null
}
} && declarations.filterIsInstance<KtNamedFunction>().none { function ->
val name = function.name
val valueParameters = function.valueParameters
val noTypeParameters = function.typeParameters.isEmpty()
noTypeParameters && (name == EQUALS && valueParameters.size == 1 || name == HASH_CODE && valueParameters.isEmpty())
}
}
}
@@ -128,4 +137,10 @@ class CanSealedSubClassBeObjectInspection : AbstractKotlinInspection() {
return internalClasses.none { klass -> klass.isInner() }
}
companion object {
val EQUALS = OperatorNameConventions.EQUALS.asString()
const val HASH_CODE = "hashCode"
}
}
idea/src/org/jetbrains/kotlin/idea/quickfix/sealedSubClassToObject/ConvertSealedSubClassToObjectFix.kt
+1 -1
View File
@@ -22,7 +22,7 @@ import org.jetbrains.kotlin.psi.psiUtil.getParentOfType
class ConvertSealedSubClassToObjectFix : LocalQuickFix {
override fun getFamilyName() = "Convert Sealed Sub-class to Object"
override fun getFamilyName() = "Convert sealed sub-class to object"
companion object {
val JAVA_LANG = Language.findLanguageByID("JAVA")
idea/src/org/jetbrains/kotlin/idea/quickfix/sealedSubClassToObject/GenerateIdentityEqualsFix.kt
+48
View File
@@ -0,0 +1,48 @@
/*
* Copyright 2010-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license
* that can be found in the license/LICENSE.txt file.
*/
package org.jetbrains.kotlin.idea.quickfix.sealedSubClassToObject
import com.intellij.codeInspection.LocalQuickFix
import com.intellij.codeInspection.ProblemDescriptor
import com.intellij.openapi.project.Project
import org.jetbrains.kotlin.lexer.KtTokens
import org.jetbrains.kotlin.psi.KtClass
import org.jetbrains.kotlin.psi.KtPsiFactory
import org.jetbrains.kotlin.psi.KtPsiFactory.*
import org.jetbrains.kotlin.psi.KtPsiFactory.CallableBuilder.Target.*
import org.jetbrains.kotlin.psi.psiUtil.getParentOfType
class GenerateIdentityEqualsFix : LocalQuickFix {
override fun applyFix(project: Project, descriptor: ProblemDescriptor) {
val klass = descriptor.psiElement.getParentOfType<KtClass>(false) ?: return
val factory = KtPsiFactory(klass)
val equalsFunction = factory.createFunction(
CallableBuilder(FUNCTION).apply {
modifier(KtTokens.OVERRIDE_KEYWORD.value)
typeParams()
name("equals")
param("other", "Any?")
returnType("Boolean")
blockBody("return this === other")
}.asString()
)
klass.addDeclaration(equalsFunction)
val hashCodeFunction = factory.createFunction(
CallableBuilder(FUNCTION).apply {
modifier(KtTokens.OVERRIDE_KEYWORD.value)
typeParams()
name("hashCode")
returnType("Int")
blockBody("return System.identityHashCode(this)")
}.asString()
)
klass.addDeclaration(hashCodeFunction)
}
override fun getFamilyName() = "Generate equals & hashCode by identity"
}
idea/testData/inspections/sealedSubClassCanBeObject/inspectionData/expected.xml
Vendored
+12 -12
View File
@@ -4,47 +4,47 @@
<line>8</line>
<module>light_idea_test_case</module>
<entry_point TYPE="file" FQNAME="temp:///src/sealed.kt" />
<problem_class severity="INFORMATION" attribute_key="INFO_ATTRIBUTES">Sealed Sub-class should be changed To Object</problem_class>
<description>Sealed Sub-class should be changed To Object</description>
<problem_class severity="WEAK WARNING" attribute_key="INFO_ATTRIBUTES">Sealed sub-class without state and overridden equals</problem_class>
<description>Sealed sub-class has no state and no overridden equals</description>
</problem>
<problem>
<file>sealed.kt</file>
<line>10</line>
<module>light_idea_test_case</module>
<entry_point TYPE="file" FQNAME="temp:///src/sealed.kt" />
<problem_class severity="INFORMATION" attribute_key="INFO_ATTRIBUTES">Sealed Sub-class should be changed To Object</problem_class>
<description>Sealed Sub-class should be changed To Object</description>
<problem_class severity="WEAK WARNING" attribute_key="INFO_ATTRIBUTES">Sealed sub-class without state and overridden equals</problem_class>
<description>Sealed sub-class has no state and no overridden equals</description>
</problem>
<problem>
<file>sealed.kt</file>
<line>13</line>
<module>light_idea_test_case</module>
<entry_point TYPE="file" FQNAME="temp:///src/sealed.kt" />
<problem_class severity="INFORMATION" attribute_key="INFO_ATTRIBUTES">Sealed Sub-class should be changed To Object</problem_class>
<description>Sealed Sub-class should be changed To Object</description>
<problem_class severity="WEAK WARNING" attribute_key="INFO_ATTRIBUTES">Sealed sub-class without state and overridden equals</problem_class>
<description>Sealed sub-class has no state and no overridden equals</description>
</problem>
<problem>
<file>sealed.kt</file>
<line>15</line>
<module>light_idea_test_case</module>
<entry_point TYPE="file" FQNAME="temp:///src/sealed.kt" />
<problem_class severity="INFORMATION" attribute_key="INFO_ATTRIBUTES">Sealed Sub-class should be changed To Object</problem_class>
<description>Sealed Sub-class should be changed To Object</description>
<problem_class severity="WEAK WARNING" attribute_key="INFO_ATTRIBUTES">Sealed sub-class without state and overridden equals</problem_class>
<description>Sealed sub-class has no state and no overridden equals</description>
</problem>
<problem>
<file>sealed.kt</file>
<line>41</line>
<module>light_idea_test_case</module>
<entry_point TYPE="file" FQNAME="temp:///src/sealed.kt" />
<problem_class severity="INFORMATION" attribute_key="INFO_ATTRIBUTES">Sealed Sub-class should be changed To Object</problem_class>
<description>Sealed Sub-class should be changed To Object</description>
<problem_class severity="WEAK WARNING" attribute_key="INFO_ATTRIBUTES">Sealed sub-class without state and overridden equals</problem_class>
<description>Sealed sub-class has no state and no overridden equals</description>
</problem>
<problem>
<file>sealed.kt</file>
<line>45</line>
<module>light_idea_test_case</module>
<entry_point TYPE="file" FQNAME="temp:///src/sealed.kt" />
<problem_class severity="INFORMATION" attribute_key="INFO_ATTRIBUTES">Sealed Sub-class should be changed To Object</problem_class>
<description>Sealed Sub-class should be changed To Object</description>
<problem_class severity="WEAK WARNING" attribute_key="INFO_ATTRIBUTES">Sealed sub-class without state and overridden equals</problem_class>
<description>Sealed sub-class has no state and no overridden equals</description>
</problem>
</problems>
idea/testData/inspectionsLocal/convertSealedSubClassToObject/baseEquals.kt
Vendored
+18
View File
@@ -0,0 +1,18 @@
// PROBLEM: none
abstract class Base {
open val prop: Int
get() = 13
override fun equals(other: Any?): Boolean {
if (other !is Base) return false
return prop == other.prop
}
}
sealed class SC : Base() {
<caret>class U : SC()
override val prop: Int
get() = 42
}
idea/testData/inspectionsLocal/convertSealedSubClassToObject/convertSubClassWithModifiers.kt
Vendored
+2
View File
@@ -1,3 +1,5 @@
// FIX: Convert sealed sub-class to object
sealed class Sealed
private <caret>class SubSealed : Sealed()
idea/testData/inspectionsLocal/convertSealedSubClassToObject/convertSubClassWithModifiers.kt.after
Vendored
+2
View File
@@ -1,3 +1,5 @@
// FIX: Convert sealed sub-class to object
sealed class Sealed
private object SubSealed : Sealed()
idea/testData/inspectionsLocal/convertSealedSubClassToObject/convertSubClassWithParentheses.kt
Vendored
+2
View File
@@ -1,3 +1,5 @@
// FIX: Convert sealed sub-class to object
sealed class Sealed
<caret>class SubSealed() : Sealed()
idea/testData/inspectionsLocal/convertSealedSubClassToObject/convertSubClassWithParentheses.kt.after
Vendored
+2
View File
@@ -1,3 +1,5 @@
// FIX: Convert sealed sub-class to object
sealed class Sealed
object SubSealed : Sealed()
idea/testData/inspectionsLocal/convertSealedSubClassToObject/convertSubClassWithoutParentheses.kt
Vendored
+2
View File
@@ -1,3 +1,5 @@
// FIX: Convert sealed sub-class to object
sealed class Sealed
<caret>class SubSealed : Sealed()
idea/testData/inspectionsLocal/convertSealedSubClassToObject/convertSubClassWithoutParentheses.kt.after
Vendored
+2
View File
@@ -1,3 +1,5 @@
// FIX: Convert sealed sub-class to object
sealed class Sealed
object SubSealed : Sealed()
idea/testData/inspectionsLocal/convertSealedSubClassToObject/fakeState.kt
Vendored
+1
View File
@@ -1,3 +1,4 @@
// FIX: Convert sealed sub-class to object
// WITH_RUNTIME
abstract class Base {
idea/testData/inspectionsLocal/convertSealedSubClassToObject/fakeState.kt.after
Vendored
+1
View File
@@ -1,3 +1,4 @@
// FIX: Convert sealed sub-class to object
// WITH_RUNTIME
abstract class Base {
idea/testData/inspectionsLocal/convertSealedSubClassToObject/noEquals.kt
Vendored
+13
View File
@@ -0,0 +1,13 @@
// FIX: Generate equals & hashCode by identity
abstract class Base {
open val prop: Int
get() = 13
}
sealed class SC : Base() {
<caret>class U : SC()
override val prop: Int
get() = 42
}
idea/testData/inspectionsLocal/convertSealedSubClassToObject/noEquals.kt.after
Vendored
+21
View File
@@ -0,0 +1,21 @@
// FIX: Generate equals & hashCode by identity
abstract class Base {
open val prop: Int
get() = 13
}
sealed class SC : Base() {
class U : SC() {
override fun equals(other: Any?): Boolean {
return this === other
}
override fun hashCode(): Int {
return System.identityHashCode(this)
}
}
override val prop: Int
get() = 42
}
idea/testData/inspectionsLocal/convertSealedSubClassToObject/ownEquals.kt
Vendored
+9
View File
@@ -0,0 +1,9 @@
// PROBLEM: none
sealed class SC {
<caret>class U : SC() {
override fun equals(other: Any?): Boolean {
return this === other
}
}
}
idea/testData/inspectionsLocal/convertSealedSubClassToObject/sealedEquals.kt
Vendored
+12
View File
@@ -0,0 +1,12 @@
// PROBLEM: none
sealed class SC {
<caret>class U : SC()
fun foo() = 42
override fun equals(other: Any?): Boolean {
if (other !is SC) return false
return foo() == other.foo()
}
}
idea/testData/multiFileLocalInspections/convertSealedSubClassToObject/convertCallableReferenceUsages/convertCallableReferenceUsages.test
Vendored
+1 -1
View File
@@ -1,5 +1,5 @@
{
"mainFile": "Seal.kt",
"inspectionClass": "org.jetbrains.kotlin.idea.inspections.CanSealedSubClassBeObjectInspection",
"fix": "Convert Sealed Sub-class to Object"
"fix": "Convert sealed sub-class to object"
}
idea/testData/multiFileLocalInspections/convertSealedSubClassToObject/convertInOtherFiles/convertInOtherFiles.test
Vendored
+1 -1
View File
@@ -1,5 +1,5 @@
{
"mainFile": "Seal.kt",
"inspectionClass": "org.jetbrains.kotlin.idea.inspections.CanSealedSubClassBeObjectInspection",
"fix": "Convert Sealed Sub-class to Object"
"fix": "Convert sealed sub-class to object"
}
idea/tests/org/jetbrains/kotlin/idea/inspections/LocalInspectionTestGenerated.java
Generated
+20
View File
@@ -1225,6 +1225,11 @@ public class LocalInspectionTestGenerated extends AbstractLocalInspectionTest {
KotlinTestUtils.assertAllTestsPresentByMetadata(this.getClass(), new File("idea/testData/inspectionsLocal/convertSealedSubClassToObject"), Pattern.compile("^([\\w\\-_]+)\\.(kt|kts)$"), TargetBackend.ANY, true);
}
@TestMetadata("baseEquals.kt")
public void testBaseEquals() throws Exception {
runTest("idea/testData/inspectionsLocal/convertSealedSubClassToObject/baseEquals.kt");
}
@TestMetadata("baseState.kt")
public void testBaseState() throws Exception {
runTest("idea/testData/inspectionsLocal/convertSealedSubClassToObject/baseState.kt");
@@ -1255,6 +1260,11 @@ public class LocalInspectionTestGenerated extends AbstractLocalInspectionTest {
runTest("idea/testData/inspectionsLocal/convertSealedSubClassToObject/generic.kt");
}
@TestMetadata("noEquals.kt")
public void testNoEquals() throws Exception {
runTest("idea/testData/inspectionsLocal/convertSealedSubClassToObject/noEquals.kt");
}
@TestMetadata("nonEmptyConstructor.kt")
public void testNonEmptyConstructor() throws Exception {
runTest("idea/testData/inspectionsLocal/convertSealedSubClassToObject/nonEmptyConstructor.kt");
@@ -1265,11 +1275,21 @@ public class LocalInspectionTestGenerated extends AbstractLocalInspectionTest {
runTest("idea/testData/inspectionsLocal/convertSealedSubClassToObject/openSubclass.kt");
}
@TestMetadata("ownEquals.kt")
public void testOwnEquals() throws Exception {
runTest("idea/testData/inspectionsLocal/convertSealedSubClassToObject/ownEquals.kt");
}
@TestMetadata("ownState.kt")
public void testOwnState() throws Exception {
runTest("idea/testData/inspectionsLocal/convertSealedSubClassToObject/ownState.kt");
}
@TestMetadata("sealedEquals.kt")
public void testSealedEquals() throws Exception {
runTest("idea/testData/inspectionsLocal/convertSealedSubClassToObject/sealedEquals.kt");
}
@TestMetadata("sealedState.kt")
public void testSealedState() throws Exception {
runTest("idea/testData/inspectionsLocal/convertSealedSubClassToObject/sealedState.kt");