From a8c57f11afe9934c5006fc4d8dd08614d16b7880 Mon Sep 17 00:00:00 2001 From: Alexey Sedunov Date: Sat, 22 Apr 2017 14:45:29 +0300 Subject: [PATCH] Move: Implement more accurate 'protected' visibility check #KT-17545 Fixed --- .../moveDeclarations/moveConflictUtils.kt | 62 +++++++++++++++---- .../after/bar/Older.kt | 8 +++ .../after/foo/test.kt | 12 ++++ .../before/foo/test.kt | 17 +++++ .../protectedMembersExternalRefs.test | 5 ++ .../after/bar/Younger.kt | 12 ++++ .../after/foo/test.kt | 9 +++ .../before/foo/test.kt | 17 +++++ .../protectedMembersInternalRefs.test | 5 ++ .../refactoring/move/MoveTestGenerated.java | 12 ++++ 10 files changed, 148 insertions(+), 11 deletions(-) create mode 100644 idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersExternalRefs/after/bar/Older.kt create mode 100644 idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersExternalRefs/after/foo/test.kt create mode 100644 idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersExternalRefs/before/foo/test.kt create mode 100644 idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersExternalRefs/protectedMembersExternalRefs.test create mode 100644 idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersInternalRefs/after/bar/Younger.kt create mode 100644 idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersInternalRefs/after/foo/test.kt create mode 100644 idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersInternalRefs/before/foo/test.kt create mode 100644 idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersInternalRefs/protectedMembersInternalRefs.test diff --git a/idea/src/org/jetbrains/kotlin/idea/refactoring/move/moveDeclarations/moveConflictUtils.kt b/idea/src/org/jetbrains/kotlin/idea/refactoring/move/moveDeclarations/moveConflictUtils.kt index f0d55dbef4a..55b16c8ae1b 100644 --- a/idea/src/org/jetbrains/kotlin/idea/refactoring/move/moveDeclarations/moveConflictUtils.kt +++ b/idea/src/org/jetbrains/kotlin/idea/refactoring/move/moveDeclarations/moveConflictUtils.kt @@ -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) { 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 { + return generateSequence(this) { it.targetAwareContainingDescriptor() }.drop(1) + } + + fun DeclarationDescriptor.targetAwareContainingClass(): ClassDescriptor? { + return targetAwareContainers().firstIsInstanceOrNull() + } + + 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() + val referrerDescriptor = referrer?.resolveToDescriptor() ?: return true + isProtectedVisible(referrerDescriptor) + } + else -> isVisibleIn(targetContainer) + } + } + for (declaration in elementsToMove - doNotGoIn) { declaration.forEachDescendantOfType { 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()) } diff --git a/idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersExternalRefs/after/bar/Older.kt b/idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersExternalRefs/after/bar/Older.kt new file mode 100644 index 00000000000..ef31afdce00 --- /dev/null +++ b/idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersExternalRefs/after/bar/Older.kt @@ -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 +} \ No newline at end of file diff --git a/idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersExternalRefs/after/foo/test.kt b/idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersExternalRefs/after/foo/test.kt new file mode 100644 index 00000000000..0d52a08fa9e --- /dev/null +++ b/idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersExternalRefs/after/foo/test.kt @@ -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 +} \ No newline at end of file diff --git a/idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersExternalRefs/before/foo/test.kt b/idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersExternalRefs/before/foo/test.kt new file mode 100644 index 00000000000..dcb405fc633 --- /dev/null +++ b/idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersExternalRefs/before/foo/test.kt @@ -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 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 +} \ No newline at end of file diff --git a/idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersExternalRefs/protectedMembersExternalRefs.test b/idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersExternalRefs/protectedMembersExternalRefs.test new file mode 100644 index 00000000000..3e3d7eb8a06 --- /dev/null +++ b/idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersExternalRefs/protectedMembersExternalRefs.test @@ -0,0 +1,5 @@ +{ + "mainFile": "foo/test.kt", + "type": "MOVE_KOTLIN_TOP_LEVEL_DECLARATIONS", + "targetPackage": "bar" +} diff --git a/idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersInternalRefs/after/bar/Younger.kt b/idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersInternalRefs/after/bar/Younger.kt new file mode 100644 index 00000000000..d3d443ac2fa --- /dev/null +++ b/idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersInternalRefs/after/bar/Younger.kt @@ -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 +} \ No newline at end of file diff --git a/idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersInternalRefs/after/foo/test.kt b/idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersInternalRefs/after/foo/test.kt new file mode 100644 index 00000000000..5f5617da06f --- /dev/null +++ b/idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersInternalRefs/after/foo/test.kt @@ -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 +} + diff --git a/idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersInternalRefs/before/foo/test.kt b/idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersInternalRefs/before/foo/test.kt new file mode 100644 index 00000000000..fb5a21d426b --- /dev/null +++ b/idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersInternalRefs/before/foo/test.kt @@ -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 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 +} \ No newline at end of file diff --git a/idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersInternalRefs/protectedMembersInternalRefs.test b/idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersInternalRefs/protectedMembersInternalRefs.test new file mode 100644 index 00000000000..3e3d7eb8a06 --- /dev/null +++ b/idea/testData/refactoring/move/kotlin/moveTopLevelDeclarations/misc/protectedMembersInternalRefs/protectedMembersInternalRefs.test @@ -0,0 +1,5 @@ +{ + "mainFile": "foo/test.kt", + "type": "MOVE_KOTLIN_TOP_LEVEL_DECLARATIONS", + "targetPackage": "bar" +} diff --git a/idea/tests/org/jetbrains/kotlin/idea/refactoring/move/MoveTestGenerated.java b/idea/tests/org/jetbrains/kotlin/idea/refactoring/move/MoveTestGenerated.java index 25faeb03e2e..a568f4d6535 100644 --- a/idea/tests/org/jetbrains/kotlin/idea/refactoring/move/MoveTestGenerated.java +++ b/idea/tests/org/jetbrains/kotlin/idea/refactoring/move/MoveTestGenerated.java @@ -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");