Move: Implement more accurate 'protected' visibility check

#KT-17545 Fixed
2017-04-22 14:45:29 +03:00
parent 480982b690
commit a8c57f11af
10 changed files with 148 additions and 11 deletions
@@ -50,13 +50,13 @@ import org.jetbrains.kotlin.renderer.DescriptorRenderer
 import org.jetbrains.kotlin.renderer.ParameterNameRenderingPolicy
 import org.jetbrains.kotlin.resolve.BindingContext
 import org.jetbrains.kotlin.resolve.DescriptorUtils
-import org.jetbrains.kotlin.resolve.calls.callUtil.getResolvedCall
 import org.jetbrains.kotlin.resolve.descriptorUtil.getImportableDescriptor
+import org.jetbrains.kotlin.resolve.descriptorUtil.isSubclassOf
 import org.jetbrains.kotlin.resolve.lazy.BodyResolveMode
-import org.jetbrains.kotlin.resolve.scopes.receivers.ExpressionReceiver
 import org.jetbrains.kotlin.resolve.source.KotlinSourceElement
 import org.jetbrains.kotlin.resolve.source.getPsi
 import org.jetbrains.kotlin.utils.SmartSet
+import org.jetbrains.kotlin.utils.addToStdlib.firstIsInstanceOrNull
 import java.util.*

 class MoveConflictChecker(
@@ -327,27 +327,67 @@ class MoveConflictChecker(

    private fun checkVisibilityInDeclarations(conflicts: MultiMap<PsiElement, String>) {
        val targetContainer = moveTarget.getContainerDescriptor() ?: return
+
+        fun DeclarationDescriptor.targetAwareContainingDescriptor(): DeclarationDescriptor? {
+            val defaultContainer = containingDeclaration
+            val psi = (this as? DeclarationDescriptorWithSource)?.source?.getPsi()
+            return if (psi != null && psi in allElementsToMove) targetContainer else defaultContainer
+        }
+
+        fun DeclarationDescriptor.targetAwareContainers(): Sequence<DeclarationDescriptor> {
+            return generateSequence(this) { it.targetAwareContainingDescriptor() }.drop(1)
+        }
+
+        fun DeclarationDescriptor.targetAwareContainingClass(): ClassDescriptor? {
+            return targetAwareContainers().firstIsInstanceOrNull<ClassDescriptor>()
+        }
+
+        fun DeclarationDescriptorWithVisibility.isProtectedVisible(referrerDescriptor: DeclarationDescriptor): Boolean {
+            val givenClassDescriptor = targetAwareContainingClass()
+            val referrerClassDescriptor = referrerDescriptor.targetAwareContainingClass() ?: return false
+            if (givenClassDescriptor != null && givenClassDescriptor.isCompanionObject) {
+                val companionOwner = givenClassDescriptor.targetAwareContainingClass()
+                if (companionOwner != null && referrerClassDescriptor.isSubclassOf(companionOwner)) return true
+            }
+            val whatDeclaration = DescriptorUtils.unwrapFakeOverrideToAnyDeclaration(this)
+            val classDescriptor = whatDeclaration.targetAwareContainingClass() ?: return false
+            if (referrerClassDescriptor.isSubclassOf(classDescriptor)) return true
+            return referrerDescriptor.targetAwareContainingDescriptor()?.let { isProtectedVisible(it) } ?: false
+        }
+
+        fun DeclarationDescriptorWithVisibility.isVisibleFrom(ref: PsiReference): Boolean {
+            val targetVisibility = visibility.normalize()
+            if (targetVisibility == Visibilities.PUBLIC) return true
+
+            return when (targetVisibility) {
+                Visibilities.PROTECTED -> {
+                    val referrer = ref.element.getStrictParentOfType<KtDeclaration>()
+                    val referrerDescriptor = referrer?.resolveToDescriptor() ?: return true
+                    isProtectedVisible(referrerDescriptor)
+                }
+                else -> isVisibleIn(targetContainer)
+            }
+        }
+
        for (declaration in elementsToMove - doNotGoIn) {
            declaration.forEachDescendantOfType<KtReferenceExpression> { refExpr ->
                refExpr.references
                        .forEach { ref ->
                            val target = ref.resolve() ?: return@forEach
                            if (isToBeMoved(target)) return@forEach
+
                            val targetDescriptor = when (target) {
                                                       is KtDeclaration -> target.resolveToDescriptor()
                                                       is PsiMember -> target.getJavaMemberDescriptor()
                                                       else -> null
-                                                   } ?: return@forEach
-                            if (targetDescriptor is CallableMemberDescriptor &&
-                                targetDescriptor.visibility.normalize() == Visibilities.PROTECTED) {
-                                val resolvedCall = refExpr.getResolvedCall(refExpr.analyze(BodyResolveMode.PARTIAL)) ?: return@forEach
-                                val dispatchReceiver = resolvedCall.dispatchReceiver
-                                if (dispatchReceiver is ExpressionReceiver && dispatchReceiver.expression is KtSuperExpression) return@forEach
-                                val receiverClass = resolvedCall.dispatchReceiver?.type?.constructor?.declarationDescriptor?.source?.getPsi()
-                                if (receiverClass != null && isToBeMoved(receiverClass)) return@forEach
+                                                   } as? DeclarationDescriptorWithVisibility ?: return@forEach
+
+                            var isVisible = targetDescriptor.isVisibleFrom(ref)
+                            if (isVisible && targetDescriptor is ConstructorDescriptor) {
+                                isVisible = targetDescriptor.containingDeclaration.isVisibleFrom(ref)
                            }

-                            if (!targetDescriptor.isVisibleIn(targetContainer)) {
+                            if (!isVisible) {
                                val message = "${render(declaration)} uses ${render(target)} which will be inaccessible after move"
                                conflicts.putValue(refExpr, message.capitalize())
                            }
@@ -0,0 +1,8 @@
+package bar
+
+open class Older {
+    protected object ProtectedObject { val inProtectedObject = 0 }
+    protected class ProtectedClass { val inProtectedClass = 0 }
+    protected fun protectedFun() = 0
+    protected var protectedVar = 0
+}
@@ -0,0 +1,12 @@
+package foo
+
+import bar.Older
+
+class Younger : Older() {
+    protected val v1: ProtectedObject = ProtectedObject
+    val v2 = ProtectedObject.inProtectedObject
+    protected val v3: ProtectedClass = ProtectedClass()
+    val v4 = ProtectedClass().inProtectedClass
+    val v5 = protectedFun()
+    val v6 = protectedVar
+}
@@ -0,0 +1,17 @@
+package foo
+
+open class <caret>Older {
+    protected object ProtectedObject { val inProtectedObject = 0 }
+    protected class ProtectedClass { val inProtectedClass = 0 }
+    protected fun protectedFun() = 0
+    protected var protectedVar = 0
+}
+
+class Younger : Older() {
+    protected val v1: ProtectedObject = ProtectedObject
+    val v2 = ProtectedObject.inProtectedObject
+    protected val v3: ProtectedClass = ProtectedClass()
+    val v4 = ProtectedClass().inProtectedClass
+    val v5 = protectedFun()
+    val v6 = protectedVar
+}
@@ -0,0 +1,5 @@
+{
+  "mainFile": "foo/test.kt",
+  "type": "MOVE_KOTLIN_TOP_LEVEL_DECLARATIONS",
+  "targetPackage": "bar"
+}
@@ -0,0 +1,12 @@
+package bar
+
+import foo.Older
+
+class Younger : Older() {
+    protected val v1: ProtectedObject = ProtectedObject
+    val v2 = ProtectedObject.inProtectedObject
+    protected val v3: ProtectedClass = ProtectedClass()
+    val v4 = ProtectedClass().inProtectedClass
+    val v5 = protectedFun()
+    val v6 = protectedVar
+}
@@ -0,0 +1,9 @@
+package foo
+
+open class Older {
+    protected object ProtectedObject { val inProtectedObject = 0 }
+    protected class ProtectedClass { val inProtectedClass = 0 }
+    protected fun protectedFun() = 0
+    protected var protectedVar = 0
+}
+
@@ -0,0 +1,17 @@
+package foo
+
+open class Older {
+    protected object ProtectedObject { val inProtectedObject = 0 }
+    protected class ProtectedClass { val inProtectedClass = 0 }
+    protected fun protectedFun() = 0
+    protected var protectedVar = 0
+}
+
+class <caret>Younger : Older() {
+    protected val v1: ProtectedObject = ProtectedObject
+    val v2 = ProtectedObject.inProtectedObject
+    protected val v3: ProtectedClass = ProtectedClass()
+    val v4 = ProtectedClass().inProtectedClass
+    val v5 = protectedFun()
+    val v6 = protectedVar
+}
@@ -0,0 +1,5 @@
+{
+  "mainFile": "foo/test.kt",
+  "type": "MOVE_KOTLIN_TOP_LEVEL_DECLARATIONS",
+  "targetPackage": "bar"
+}
@@ -576,6 +576,18 @@ public class MoveTestGenerated extends AbstractMoveTest {
        doTest(fileName);
    }

+    @TestMetadata("kotlin/moveTopLevelDeclarations/misc/protectedMembersExternalRefs/protectedMembersExternalRefs.test")
+    public void testKotlin_moveTopLevelDeclarations_misc_protectedMembersExternalRefs_ProtectedMembersExternalRefs() throws Exception {
+        String fileName = KotlinTestUtils.navigationMetadata("idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersExternalRefs/protectedMembersExternalRefs.test");
+        doTest(fileName);
+    }
+
+    @TestMetadata("kotlin/moveTopLevelDeclarations/misc/protectedMembersInternalRefs/protectedMembersInternalRefs.test")
+    public void testKotlin_moveTopLevelDeclarations_misc_protectedMembersInternalRefs_ProtectedMembersInternalRefs() throws Exception {
+        String fileName = KotlinTestUtils.navigationMetadata("idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersInternalRefs/protectedMembersInternalRefs.test");
+        doTest(fileName);
+    }
+
    @TestMetadata("kotlin/moveTopLevelDeclarations/misc/selfReferences/selfReferences.test")
    public void testKotlin_moveTopLevelDeclarations_misc_selfReferences_SelfReferences() throws Exception {
        String fileName = KotlinTestUtils.navigationMetadata("idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/selfReferences/selfReferences.test");