Move: Optimize conflict analysis

- Do not process conflicts twice wgen moving a directory - Check move target before resolving usage context declaration - Skip visibility check for usages referring to public declarations - Skip module conflict check for declarations with unchanged module - Do not report lambdas as usage container (use enclosing declaration)
2017-09-14 18:26:41 +03:00
parent fefa2304a0
commit 963747ce04
7 changed files with 51 additions and 26 deletions
@@ -149,7 +149,8 @@ fun PsiElement.getUsageContext(): PsiElement {
                this,
                KtPropertyAccessor::class.java,
                KtProperty::class.java,
-                KtFunction::class.java,
+                KtNamedFunction::class.java,
+                KtConstructor::class.java,
                KtClassOrObject::class.java
        ) ?: containingFile
        else -> ConflictsUtil.getContainer(this)
@@ -96,7 +96,8 @@ class MoveDeclarationsDescriptor @JvmOverloads constructor(
        val deleteSourceFiles: Boolean = false,
        val moveCallback: MoveCallback? = null,
        val openInEditor: Boolean = false,
-        val allElementsToMove: List<PsiElement>? = null
+        val allElementsToMove: List<PsiElement>? = null,
+        val analyzeConflicts: Boolean = true
 )

 class ConflictUsageInfo(element: PsiElement, val messages: Collection<String>) : UsageInfo(element)
@@ -226,8 +227,10 @@ class MoveKotlinDeclarationsProcessor(

            internalUsages += descriptor.delegate.findInternalUsages(descriptor)
            collectUsages(kotlinToLightElements, externalUsages)
-            conflictChecker.checkAllConflicts(externalUsages, internalUsages, conflicts)
-            descriptor.delegate.collectConflicts(descriptor, internalUsages, conflicts)
+            if (descriptor.analyzeConflicts) {
+                conflictChecker.checkAllConflicts(externalUsages, internalUsages, conflicts)
+                descriptor.delegate.collectConflicts(descriptor, internalUsages, conflicts)
+            }

            usages += internalUsages
            usages += externalUsages
@@ -48,6 +48,8 @@ import org.jetbrains.kotlin.idea.refactoring.getUsageContext
 import org.jetbrains.kotlin.idea.refactoring.move.KotlinMoveUsage
 import org.jetbrains.kotlin.idea.search.and
 import org.jetbrains.kotlin.idea.search.not
+import org.jetbrains.kotlin.idea.util.projectStructure.getModule
+import org.jetbrains.kotlin.idea.util.projectStructure.module
 import org.jetbrains.kotlin.lexer.KtTokens
 import org.jetbrains.kotlin.psi.*
 import org.jetbrains.kotlin.psi.psiUtil.contains
@@ -164,11 +166,11 @@ class MoveConflictChecker(
    // Based on RefactoringConflictsUtil.analyzeModuleConflicts
    fun analyzeModuleConflictsInUsages(project: Project,
                                       usages: Collection<UsageInfo>,
-                                       sourceRoot: VirtualFile,
+                                       targetFile: VirtualFile,
                                       conflicts: MultiMap<PsiElement, String>) {
-        val targetModule = ModuleUtilCore.findModuleForFile(sourceRoot, project) ?: return
+        val targetModule = targetFile.getModule(project) ?: return

-        val isInTestSources = ModuleRootManager.getInstance(targetModule).fileIndex.isInTestSourceContent(sourceRoot)
+        val isInTestSources = ModuleRootManager.getInstance(targetModule).fileIndex.isInTestSourceContent(targetFile)
        NextUsage@ for (usage in usages) {
            val element = usage.element ?: continue
            if (PsiTreeUtil.getParentOfType(element, PsiImportStatement::class.java, false) != null) continue
@@ -177,9 +179,7 @@ class MoveConflictChecker(
            val resolveScope = element.resolveScope
            if (resolveScope.isSearchInModuleContent(targetModule, isInTestSources)) continue

-            val usageFile = element.containingFile
-            val usageVFile = usageFile.virtualFile ?: continue
-            val usageModule = ModuleUtilCore.findModuleForFile(usageVFile, project) ?: continue
+            val usageModule = element.module ?: continue
            val scopeDescription = RefactoringUIUtil.getDescription(element.getUsageContext(), true)
            val referencedElement = (if (usage is MoveRenameUsageInfo) usage.referencedElement else usage.element) ?: error(usage)
            val message = if (usageModule == targetModule && isInTestSources) {
@@ -200,9 +200,9 @@ class MoveConflictChecker(

    fun checkModuleConflictsInUsages(externalUsages: MutableSet<UsageInfo>, conflicts: MultiMap<PsiElement, String>) {
        val newConflicts = MultiMap<PsiElement, String>()
-        val sourceRoot = moveTarget.targetFile ?: return
+        val targetFile = moveTarget.targetFile ?: return

-        analyzeModuleConflictsInUsages(project, externalUsages, sourceRoot, newConflicts)
+        analyzeModuleConflictsInUsages(project, externalUsages, targetFile, newConflicts)
        if (!newConflicts.isEmpty) {
            val referencedElementsToSkip = newConflicts.keySet().mapNotNullTo(HashSet()) { it.namedUnwrappedElement }
            externalUsages.removeIf {
@@ -242,8 +242,8 @@ class MoveConflictChecker(
            internalUsages: MutableSet<UsageInfo>,
            conflicts: MultiMap<PsiElement, String>
    ) {
-        val sourceRoot = moveTarget.targetFile ?: return
-        val targetModule = ModuleUtilCore.findModuleForFile(sourceRoot, project) ?: return
+        val targetFile = moveTarget.targetFile ?: return
+        val targetModule = targetFile.getModule(project) ?: return
        val resolveScope = targetModule.getScopeWithPlatformAwareDependencies()

        fun isInScope(targetElement: PsiElement, targetDescriptor: DeclarationDescriptor): Boolean {
@@ -255,7 +255,7 @@ class MoveConflictChecker(
            val renderedImportableTarget = DESCRIPTOR_RENDERER_FOR_COMPARISON.render(importableDescriptor)
            val renderedTarget by lazy { DESCRIPTOR_RENDERER_FOR_COMPARISON.render(targetDescriptor) }

-            val targetModuleInfo = getModuleInfoByVirtualFile(project, sourceRoot)
+            val targetModuleInfo = getModuleInfoByVirtualFile(project, targetFile)
            val dummyFile = KtPsiFactory(targetElement.project).createFile("dummy.kt", "").apply {
                moduleInfo = targetModuleInfo
                targetPlatform = TargetPlatformDetector.getPlatform(targetModule)
@@ -288,6 +288,8 @@ class MoveConflictChecker(

        val referencesToSkip = HashSet<KtReferenceExpression>()
        for (declaration in elementsToMove - doNotGoIn) {
+            if (declaration.module == targetModule) continue
+
            declaration.forEachDescendantOfType<KtReferenceExpression> { refExpr ->
                val targetDescriptor = refExpr.analyze(BodyResolveMode.PARTIAL)[BindingContext.REFERENCE_TARGET, refExpr] ?: return@forEachDescendantOfType

@@ -328,15 +330,19 @@ class MoveConflictChecker(
            val referencedElement = usage.referencedElement?.namedUnwrappedElement as? KtNamedDeclaration ?: continue
            val referencedDescriptor = resolutionFacade.resolveToDescriptor(referencedElement)

+            if (referencedDescriptor is DeclarationDescriptorWithVisibility
+                && referencedDescriptor.visibility == Visibilities.PUBLIC) continue
+
            val container = element.getUsageContext()
            if (!declarationToContainers.getOrPut(referencedElement) { HashSet<PsiElement>() }.add(container)) continue

+            val targetContainer = moveTarget.getContainerDescriptor() ?: continue
+
            val referencingDescriptor = when (container) {
                                            is KtDeclaration -> container.unsafeResolveToDescriptor()
                                            is PsiMember -> container.getJavaMemberDescriptor()
                                            else -> null
                                        } ?: continue
-            val targetContainer = moveTarget.getContainerDescriptor() ?: continue
            val descriptorToCheck = referencedDescriptor.asPredicted(targetContainer) ?: continue

            if (referencedDescriptor.isVisibleIn(referencingDescriptor) && !descriptorToCheck.isVisibleIn(referencingDescriptor)) {
@@ -32,6 +32,8 @@ import org.jetbrains.kotlin.idea.refactoring.invokeOnceOnCommandFinish
 import org.jetbrains.kotlin.idea.refactoring.move.moveDeclarations.KotlinDirectoryMoveTarget
 import org.jetbrains.kotlin.idea.refactoring.move.moveDeclarations.MoveKotlinDeclarationsProcessor
 import org.jetbrains.kotlin.idea.refactoring.move.moveDeclarations.analyzeConflictsInFile
+import org.jetbrains.kotlin.idea.runSynchronouslyWithProgress
+import org.jetbrains.kotlin.idea.util.application.runReadAction
 import org.jetbrains.kotlin.name.FqName
 import org.jetbrains.kotlin.psi.KtFile
 import java.util.*
@@ -68,7 +70,7 @@ class KotlinMoveDirectoryWithClassesHelper : MoveDirectoryWithClassesHelper() {
            project: Project) {
        filesToMove
                .filterIsInstance<KtFile>()
-                .mapTo(result) { FileUsagesWrapper(it, fileHandler.findUsages(it, null, searchInComments, searchInNonJavaFiles), null) }
+                .mapTo(result) { FileUsagesWrapper(it, fileHandler.findUsages(it, null, false), null) }
    }

    override fun preprocessUsages(
@@ -83,8 +85,12 @@ class KotlinMoveDirectoryWithClassesHelper : MoveDirectoryWithClassesHelper() {
        for ((index, usageInfo) in infos.withIndex()) {
            if (usageInfo !is FileUsagesWrapper) continue

-            analyzeConflictsInFile(usageInfo.psiFile, usageInfo.usages, moveTarget, files, conflicts) {
-                infos[index] = usageInfo.copy(usages = it)
+            project.runSynchronouslyWithProgress("Analyzing conflicts in ${usageInfo.psiFile.name}", false) {
+                runReadAction {
+                    analyzeConflictsInFile(usageInfo.psiFile, usageInfo.usages, moveTarget, files, conflicts) {
+                        infos[index] = usageInfo.copy(usages = it)
+                    }
+                }
            }
        }
    }
@@ -103,7 +109,7 @@ class KotlinMoveDirectoryWithClassesHelper : MoveDirectoryWithClassesHelper() {
    ): Boolean {
        if (file !is KtFile) return false

-        val moveDeclarationsProcessor = fileHandler.initMoveProcessor(file, moveDestination)
+        val moveDeclarationsProcessor = fileHandler.initMoveProcessor(file, moveDestination, false)
        val moveContextMap = getOrCreateMoveContextMap()
        moveContextMap[file] = MoveContext(moveDestination, moveDeclarationsProcessor)
        if (moveDeclarationsProcessor != null) {
@@ -73,7 +73,7 @@ class MoveKotlinFileHandler : MoveFileHandler() {
        return ContainerChangeInfo(ContainerInfo.Package(oldPackageName), ContainerInfo.Package(newPackageName.toSafe()))
    }

-    fun initMoveProcessor(psiFile: PsiFile, newParent: PsiDirectory?): MoveKotlinDeclarationsProcessor? {
+    fun initMoveProcessor(psiFile: PsiFile, newParent: PsiDirectory?, withConflicts: Boolean): MoveKotlinDeclarationsProcessor? {
        if (psiFile !is KtFile) return null
        val packageNameInfo = psiFile.getPackageNameInfo(newParent, false) ?: return null

@@ -98,7 +98,8 @@ class MoveKotlinFileHandler : MoveFileHandler() {
                        moveTarget = moveTarget,
                        delegate = MoveDeclarationsDelegate.TopLevel,
                        scanEntireFile = true,
-                        allElementsToMove = psiFile.allElementsToMove
+                        allElementsToMove = psiFile.allElementsToMove,
+                        analyzeConflicts = withConflicts
                ),
                Mover.Idle
        )
@@ -114,11 +115,19 @@ class MoveKotlinFileHandler : MoveFileHandler() {
            newParent: PsiDirectory?,
            searchInComments: Boolean,
            searchInNonJavaFiles: Boolean
+    ): List<UsageInfo> {
+        return findUsages(psiFile, newParent, true)
+    }
+
+    fun findUsages(
+            psiFile: PsiFile,
+            newParent: PsiDirectory?,
+            withConflicts: Boolean
    ): List<UsageInfo> {
        if (psiFile !is KtFile) return emptyList()

        val usages = arrayListOf<UsageInfo>(FileInfo(psiFile))
-        initMoveProcessor(psiFile, newParent)?.let {
+        initMoveProcessor(psiFile, newParent, withConflicts)?.let {
            usages += it.findUsages()
            usages += it.getConflictsAsUsages()
        }
@@ -127,7 +136,7 @@ class MoveKotlinFileHandler : MoveFileHandler() {

    override fun prepareMovedFile(file: PsiFile, moveDestination: PsiDirectory, oldToNewMap: MutableMap<PsiElement, PsiElement>) {
        if (file !is KtFile) return
-        val moveProcessor = initMoveProcessor(file, moveDestination) ?: return
+        val moveProcessor = initMoveProcessor(file, moveDestination, false) ?: return
        val moveContext = MoveContext(file, moveProcessor)
        oldToNewMap[moveContext] = moveContext
        val packageNameInfo = file.getPackageNameInfo(moveDestination, true) ?: return
@@ -1,5 +1,5 @@
 // WITH_RUNTIME
-// SHOULD_FAIL_WITH: 'foo' in class B will require class instance, 'foo' in function test() will require class instance, 'foo' in function test() will require class instance, 'foo' in lambda &lt;anonymous&gt;() will require class instance
+// SHOULD_FAIL_WITH: 'foo' in class B will require class instance, 'foo' in function test() will require class instance, 'foo' in function test() will require class instance, 'foo' in function test() will require class instance
 class A {
    companion object {
        class B {
@@ -1,5 +1,5 @@
 // WITH_RUNTIME
-// SHOULD_FAIL_WITH: 'foo' in class B will require class instance, 'foo' in function test() will require class instance, 'foo' in function test() will require class instance, 'foo' in lambda &lt;anonymous&gt;() will require class instance
+// SHOULD_FAIL_WITH: 'foo' in class B will require class instance, 'foo' in function test() will require class instance, 'foo' in function test() will require class instance, 'foo' in function test() will require class instance
 class A {
    companion object {
        class B {