Extract Superclass: Report private members used in abstracted members

#KT-16284 Fixed

idea/src/org/jetbrains/kotlin/idea/refactoring/introduce/extractClass/ExtractSuperRefactoring.kt

@@ -45,12 +45,11 @@ import org.jetbrains.kotlin.idea.core.replaced
 import org.jetbrains.kotlin.idea.refactoring.introduce.insertDeclaration
 import org.jetbrains.kotlin.idea.refactoring.memberInfo.KotlinMemberInfo
 import org.jetbrains.kotlin.idea.refactoring.memberInfo.getChildrenToAnalyze
-import org.jetbrains.kotlin.idea.refactoring.memberInfo.resolveToDescriptorWrapperAware
 import org.jetbrains.kotlin.idea.refactoring.memberInfo.toJavaMemberInfo
 import org.jetbrains.kotlin.idea.refactoring.move.moveDeclarations.KotlinMoveTargetForDeferredFile
 import org.jetbrains.kotlin.idea.refactoring.move.moveDeclarations.KotlinMoveTargetForExistingElement
 import org.jetbrains.kotlin.idea.refactoring.move.moveDeclarations.MoveConflictChecker
-import org.jetbrains.kotlin.idea.refactoring.pullUp.checkPrivateMembersWithUsages
+import org.jetbrains.kotlin.idea.refactoring.pullUp.checkVisibilityInAbstractedMembers
 import org.jetbrains.kotlin.idea.references.mainReference
 import org.jetbrains.kotlin.idea.runSynchronouslyWithProgress
 import org.jetbrains.kotlin.idea.util.IdeDescriptorRenderers
@@ -175,15 +174,7 @@ class ExtractSuperRefactoring(
                         ExtractSuperClassUtil.checkSuperAccessible(targetParent, conflicts, originalClass.toLightClass())
                     }
 
-                    if (isExtractInterface) {
-                        val resolutionFacade = originalClass.getResolutionFacade()
-
-                        val membersToMove = elementsToMove.filterIsInstance<KtNamedDeclaration>()
-                        for (member in membersToMove) {
-                            val memberDescriptor = member.resolveToDescriptorWrapperAware(resolutionFacade)
-                            checkPrivateMembersWithUsages(member, memberDescriptor, originalClass, membersToMove, conflicts)
-                        }
-                    }
+                    checkVisibilityInAbstractedMembers(memberInfos, originalClass.getResolutionFacade(), conflicts)
                 }
             }
 

idea/src/org/jetbrains/kotlin/idea/refactoring/pullUp/pullUpConflictsUtils.kt

@@ -31,12 +31,15 @@ import org.jetbrains.kotlin.idea.refactoring.memberInfo.KotlinMemberInfo
 import org.jetbrains.kotlin.idea.refactoring.memberInfo.getChildrenToAnalyze
 import org.jetbrains.kotlin.idea.refactoring.memberInfo.resolveToDescriptorWrapperAware
 import org.jetbrains.kotlin.idea.references.KtReference
+import org.jetbrains.kotlin.idea.references.mainReference
+import org.jetbrains.kotlin.idea.resolve.ResolutionFacade
 import org.jetbrains.kotlin.idea.search.declarationsSearch.HierarchySearchRequest
 import org.jetbrains.kotlin.idea.search.declarationsSearch.searchInheritors
 import org.jetbrains.kotlin.idea.util.IdeDescriptorRenderers
 import org.jetbrains.kotlin.lexer.KtTokens
 import org.jetbrains.kotlin.psi.*
 import org.jetbrains.kotlin.psi.psiUtil.containingClassOrObject
+import org.jetbrains.kotlin.psi.psiUtil.forEachDescendantOfType
 import org.jetbrains.kotlin.psi.psiUtil.parentsWithSelf
 import org.jetbrains.kotlin.renderer.DescriptorRenderer
 import org.jetbrains.kotlin.renderer.ParameterNameRenderingPolicy
@@ -67,23 +70,53 @@ fun checkConflicts(project: Project,
             checkAccidentalOverrides(member, memberDescriptor, conflicts)
             checkInnerClassToInterface(member, memberDescriptor, conflicts)
             checkVisibility(memberInfo, memberDescriptor, conflicts)
-            if (isInterfaceTarget) {
-                checkPrivateMembersWithUsages(member, memberDescriptor, sourceClass, pullUpData.membersToMove, conflicts)
-            }
         }
     }
+    checkVisibilityInAbstractedMembers(memberInfos, pullUpData.resolutionFacade, conflicts)
 
     project.checkConflictsInteractively(conflicts, onShowConflicts, onAccept)
 }
 
+internal fun checkVisibilityInAbstractedMembers(
+    memberInfos: List<KotlinMemberInfo>,
+    resolutionFacade: ResolutionFacade,
+    conflicts: MultiMap<PsiElement, String>
+) {
+    val membersToMove = ArrayList<KtNamedDeclaration>()
+    val membersToAbstract = ArrayList<KtNamedDeclaration>()
+
+    for (memberInfo in memberInfos) {
+        val member = memberInfo.member ?: continue
+        (if (memberInfo.isToAbstract) membersToAbstract else membersToMove).add(member)
+    }
+
+    for (member in membersToAbstract) {
+        val memberDescriptor = member.resolveToDescriptorWrapperAware(resolutionFacade)
+        member.forEachDescendantOfType<KtSimpleNameExpression> {
+            val target = it.mainReference.resolve() as? KtNamedDeclaration ?: return@forEachDescendantOfType
+            if (!willBeMoved(target, membersToMove)) return@forEachDescendantOfType
+            if (target.hasModifier(KtTokens.PRIVATE_KEYWORD)) {
+                val targetDescriptor = target.resolveToDescriptorWrapperAware(resolutionFacade)
+                val memberText = memberDescriptor.renderForConflicts()
+                val targetText = targetDescriptor.renderForConflicts()
+                val message = "$memberText uses $targetText which won't be accessible from the subclass."
+                conflicts.putValue(target, message.capitalize())
+            }
+        }
+    }
+}
+
+internal fun willBeMoved(element: PsiElement, membersToMove: Collection<KtNamedDeclaration>) =
+    element.parentsWithSelf.any { it in membersToMove }
+
 internal fun willBeUsedInSourceClass(
-        member: PsiElement,
-        sourceClass: KtClassOrObject,
-        membersToMove: Collection<KtNamedDeclaration>
+    member: PsiElement,
+    sourceClass: KtClassOrObject,
+    membersToMove: Collection<KtNamedDeclaration>
 ): Boolean {
     return !ReferencesSearch
-            .search(member, LocalSearchScope(sourceClass), false)
-            .all { it.element.parentsWithSelf.any { it in membersToMove } }
+        .search(member, LocalSearchScope(sourceClass), false)
+        .all { willBeMoved(it.element, membersToMove) }
 }
 
 private val CALLABLE_RENDERER = IdeDescriptorRenderers.SOURCE_CODE_SHORT_NAMES_NO_ANNOTATIONS.withOptions {
@@ -224,18 +257,4 @@ private fun KotlinPullUpData.checkVisibility(
                 }
         )
     }
-}
-
-internal fun checkPrivateMembersWithUsages(
-        member: KtNamedDeclaration,
-        memberDescriptor: DeclarationDescriptor,
-        sourceClass: KtClassOrObject,
-        membersToMove: Collection<KtNamedDeclaration>,
-        conflicts: MultiMap<PsiElement, String>
-) {
-    if (member.hasModifier(KtTokens.PRIVATE_KEYWORD) &&
-        willBeUsedInSourceClass(member, sourceClass, membersToMove)) {
-        val message = "${memberDescriptor.renderForConflicts()} can't be moved to the interface because it's private and has usages in the original class"
-        conflicts.putValue(member, message.capitalize())
-    }
 }

idea/testData/refactoring/extractInterface/abstractedMemberUsingPrivate.kt

@@ -0,0 +1,9 @@
+// NAME: I
+
+// SIBLING:
+class <caret>PrivateRef {
+    // INFO: {checked: "true", toAbstract: "false"}
+    private fun privateFun() = 0
+    // INFO: {checked: "true", toAbstract: "true"}
+    fun refer() = privateFun()
+}

idea/testData/refactoring/extractInterface/abstractedMemberUsingPrivate.kt.conflicts

@@ -0,0 +1 @@
+Function 'fun refer(): Int' uses function 'fun privateFun(): Int' which won't be accessible from the subclass.

idea/testData/refactoring/extractInterface/privateMemberWithUsages.kt.after

@@ -0,0 +1,11 @@
+interface I {
+    // INFO: {checked: "true"}
+    fun privateFun() = 0
+}
+
+// NAME: I
+// SIBLING:
+class Foo : I {
+
+    fun refer() = privateFun()
+}

idea/testData/refactoring/extractInterface/privateMemberWithUsages.kt.conflicts

@@ -1 +0,0 @@
-Function 'fun privateFun(): Int' can't be moved to the interface because it's private and has usages in the original class

idea/testData/refactoring/extractSuperclass/abstractedMemberUsingPrivate.kt

@@ -0,0 +1,9 @@
+// NAME: PrivateRefBase
+
+// SIBLING:
+class <caret>PrivateRef {
+    // INFO: {checked: "true", toAbstract: "false"}
+    private fun privateFun() = 0
+    // INFO: {checked: "true", toAbstract: "true"}
+    fun refer() = privateFun()
+}

idea/testData/refactoring/extractSuperclass/abstractedMemberUsingPrivate.kt.conflicts

@@ -0,0 +1 @@
+Function 'fun refer(): Int' uses function 'fun privateFun(): Int' which won't be accessible from the subclass.

idea/testData/refactoring/pullUp/k2k/privateMemberWithUsagesToInterface.kt.after

@@ -0,0 +1,9 @@
+interface I {
+    // INFO: {checked: "true"}
+    fun privateFun() = 0
+}
+
+class Foo : I {
+
+    fun refer() = privateFun()
+}

idea/testData/refactoring/pullUp/k2k/privateMemberWithUsagesToInterface.kt.messages

@@ -1 +0,0 @@
-Function 'fun privateFun(): Int' can't be moved to the interface because it's private and has usages in the original class

idea/tests/org/jetbrains/kotlin/idea/refactoring/introduce/ExtractionTestGenerated.java

@@ -3931,6 +3931,11 @@ public class ExtractionTestGenerated extends AbstractExtractionTest {
             KotlinTestUtils.runTest(this::doExtractSuperclassTest, TargetBackend.ANY, testDataFilePath);
         }
 
+        @TestMetadata("abstractedMemberUsingPrivate.kt")
+        public void testAbstractedMemberUsingPrivate() throws Exception {
+            runTest("idea/testData/refactoring/extractSuperclass/abstractedMemberUsingPrivate.kt");
+        }
+
         @TestMetadata("addSuperclassNoSecondaryConstructors.kt")
         public void testAddSuperclassNoSecondaryConstructors() throws Exception {
             runTest("idea/testData/refactoring/extractSuperclass/addSuperclassNoSecondaryConstructors.kt");
@@ -4029,6 +4034,11 @@ public class ExtractionTestGenerated extends AbstractExtractionTest {
             KotlinTestUtils.runTest(this::doExtractInterfaceTest, TargetBackend.ANY, testDataFilePath);
         }
 
+        @TestMetadata("abstractedMemberUsingPrivate.kt")
+        public void testAbstractedMemberUsingPrivate() throws Exception {
+            runTest("idea/testData/refactoring/extractInterface/abstractedMemberUsingPrivate.kt");
+        }
+
         @TestMetadata("addInterface.kt")
         public void testAddInterface() throws Exception {
             runTest("idea/testData/refactoring/extractInterface/addInterface.kt");