Do not suggest "convert to object" for sealed sub-classes with state

So #KT-24816 Fixed
2018-06-08 14:45:45 +03:00
parent 813d7fcb6a
commit a09f6e14a5
8 changed files with 118 additions and 3 deletions
@@ -1,5 +1,5 @@
 <html>
 <body>
-This inspection reports direct inheritors of <b>sealed class</b> that have default constructors only and thus can be objects.
+This inspection reports direct inheritors of <b>sealed class</b> that have no state and thus can be objects.
 </body>
 </html>
@@ -20,13 +20,18 @@ import com.intellij.codeInspection.ProblemHighlightType
 import com.intellij.codeInspection.ProblemsHolder
 import com.intellij.psi.PsiElementVisitor
 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.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.KtProperty
 import org.jetbrains.kotlin.psi.KtVisitorVoid
+import org.jetbrains.kotlin.resolve.DescriptorToSourceUtils
+import org.jetbrains.kotlin.resolve.descriptorUtil.getSuperClassNotAny

 class CanSealedSubClassBeObjectInspection : AbstractKotlinInspection() {
    override fun buildVisitor(holder: ProblemsHolder, isOnTheFly: Boolean, session: LocalInspectionToolSession): PsiElementVisitor {
@@ -35,13 +40,16 @@ class CanSealedSubClassBeObjectInspection : AbstractKotlinInspection() {
                if (!klass.hasModifier(KtTokens.SEALED_KEYWORD)) return
                if (klass.getModalityFromDescriptor() != KtTokens.SEALED_KEYWORD) return

-                klass.getSubclasses()
+                val candidates = klass.getSubclasses()
                    .withEmptyConstructors()
                    .thatAreFinal()
                    .thatHasNoTypeParameters()
                    .thatHasNoInnerClasses()
                    .thatHasNoCompanionObjects()
-                    .forEach { reportPossibleObject(it) }
+                    .thatHasNoState()
+                if (candidates.isEmpty() || !klass.hasNoState() || !klass.baseClassHasNoState()) return
+
+                candidates.forEach { reportPossibleObject(it) }
            }

            private fun reportPossibleObject(klass: KtClass) {
@@ -56,6 +64,14 @@ class CanSealedSubClassBeObjectInspection : AbstractKotlinInspection() {
        }
    }

+    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>()
@@ -83,6 +99,28 @@ class CanSealedSubClassBeObjectInspection : AbstractKotlinInspection() {
        return filter { klass -> klass.hasNoInnerClass() }
    }

+    private fun List<KtClass>.thatHasNoState(): List<KtClass> {
+        return filter { it.hasNoState() }
+    }
+
+    private fun KtClass.hasNoState(): Boolean {
+        if (primaryConstructor?.valueParameters?.isNotEmpty() == true) return false
+        val body = getBody()
+        return body == null || run {
+            val properties = body.declarations.filterIsInstance<KtProperty>()
+            properties.none { property ->
+                // Simplified "backing field required"
+                when {
+                    property.isAbstract() -> false
+                    property.initializer != null -> true
+                    property.delegate != null -> false
+                    !property.isVar -> property.getter == null
+                    else -> property.getter == null || property.setter == null
+                }
+            }
+        }
+    }
+
    private fun KtClass.hasNoInnerClass(): Boolean {
        val internalClasses = getBody()
            ?.declarations
@@ -0,0 +1,7 @@
+// PROBLEM: none
+
+abstract class Base(var x: String)
+
+sealed class Sealed(s: String) : Base(s)
+
+<caret>class Derived : Sealed("123")
@@ -0,0 +1,18 @@
+// WITH_RUNTIME
+
+abstract class Base {
+    var s: String
+        get() = "Hello"
+        set(value) {}
+}
+
+sealed class Sealed : Base() {
+    open val x: List<Int>
+        get() = emptyList()
+}
+
+<caret>class Derived : Sealed() {
+    var length: Int
+        get() = s.length
+        set(value) {}
+}
@@ -0,0 +1,18 @@
+// WITH_RUNTIME
+
+abstract class Base {
+    var s: String
+        get() = "Hello"
+        set(value) {}
+}
+
+sealed class Sealed : Base() {
+    open val x: List<Int>
+        get() = emptyList()
+}
+
+object Derived : Sealed() {
+    var length: Int
+        get() = s.length
+        set(value) {}
+}
@@ -0,0 +1,8 @@
+// PROBLEM: none
+// WITH_RUNTIME
+
+sealed class SC {
+    <caret>class U : SC() {
+        val a = mutableListOf<String>()
+    }
+}
@@ -0,0 +1,6 @@
+// PROBLEM: none
+
+sealed class SC {
+    var u = 0
+    <caret>class C : SC() {}
+}
@@ -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("baseState.kt")
+        public void testBaseState() throws Exception {
+            runTest("idea/testData/inspectionsLocal/convertSealedSubClassToObject/baseState.kt");
+        }
+
        @TestMetadata("convertSubClassWithModifiers.kt")
        public void testConvertSubClassWithModifiers() throws Exception {
            runTest("idea/testData/inspectionsLocal/convertSealedSubClassToObject/convertSubClassWithModifiers.kt");
@@ -1240,6 +1245,11 @@ public class LocalInspectionTestGenerated extends AbstractLocalInspectionTest {
            runTest("idea/testData/inspectionsLocal/convertSealedSubClassToObject/convertSubClassWithoutParentheses.kt");
        }

+        @TestMetadata("fakeState.kt")
+        public void testFakeState() throws Exception {
+            runTest("idea/testData/inspectionsLocal/convertSealedSubClassToObject/fakeState.kt");
+        }
+
        @TestMetadata("generic.kt")
        public void testGeneric() throws Exception {
            runTest("idea/testData/inspectionsLocal/convertSealedSubClassToObject/generic.kt");
@@ -1255,6 +1265,16 @@ public class LocalInspectionTestGenerated extends AbstractLocalInspectionTest {
            runTest("idea/testData/inspectionsLocal/convertSealedSubClassToObject/openSubclass.kt");
        }

+        @TestMetadata("ownState.kt")
+        public void testOwnState() throws Exception {
+            runTest("idea/testData/inspectionsLocal/convertSealedSubClassToObject/ownState.kt");
+        }
+
+        @TestMetadata("sealedState.kt")
+        public void testSealedState() throws Exception {
+            runTest("idea/testData/inspectionsLocal/convertSealedSubClassToObject/sealedState.kt");
+        }
+
        @TestMetadata("withCompanion.kt")
        public void testWithCompanion() throws Exception {
            runTest("idea/testData/inspectionsLocal/convertSealedSubClassToObject/withCompanion.kt");