Extract Function: Allow the user to proceed with extraction when non-critical errors are found

#KT-4941 Fixed

idea/src/org/jetbrains/jet/plugin/debugger/evaluate/KotlinEvaluationBuilder.kt

@@ -62,6 +62,7 @@ import org.jetbrains.jet.plugin.caches.resolve.getAnalysisResults
 import org.jetbrains.jet.lang.psi.JetCodeFragment
 import org.jetbrains.jet.lang.psi.JetImportList
 import org.jetbrains.jet.lang.psi.JetExpression
+import org.jetbrains.jet.plugin.refactoring.extractFunction.AnalysisResult.Status
 
 object KotlinEvaluationBuilder: EvaluatorBuilder {
     override fun build(codeFragment: PsiElement, position: SourcePosition?): ExpressionEvaluator {
@@ -286,11 +287,11 @@ private fun getFunctionForExtractedFragment(
             if (nextSibling == null) return null
 
             val analysisResult = ExtractionData(tmpFile, Collections.singletonList(newDebugExpression), nextSibling).performAnalysis()
-            if (analysisResult is MaybeError) {
+            if (analysisResult.status != Status.SUCCESS) {
-                throw EvaluateExceptionUtil.createEvaluateException(analysisResult.error)
+                throw EvaluateExceptionUtil.createEvaluateException(analysisResult.messages.makeString("\n"))
             }
 
-            val validationResult = (analysisResult as MaybeValue).value.validate()
+            val validationResult = analysisResult.descriptor!!.validate()
             if (!validationResult.conflicts.isEmpty()) {
                 throw EvaluateExceptionUtil.createEvaluateException("Some declarations are unavailable")
             }

idea/src/org/jetbrains/jet/plugin/refactoring/extractFunction/ExtractKotlinFunctionHandler.kt

@@ -35,19 +35,21 @@ import org.jetbrains.jet.plugin.refactoring.getAllExtractionContainers
 import com.intellij.refactoring.IntroduceTargetChooser
 import com.intellij.openapi.util.Pass
 import com.intellij.openapi.application.ApplicationManager
-import org.jetbrains.jet.lang.psi.JetPsiFactory
 import org.jetbrains.jet.lang.psi.psiUtil.getOutermostParentContainedIn
 import org.jetbrains.jet.plugin.refactoring.checkConflictsInteractively
 import org.jetbrains.jet.plugin.refactoring.executeWriteCommand
-import org.jetbrains.jet.plugin.util.MaybeError
-import org.jetbrains.jet.plugin.util.MaybeValue
 import org.jetbrains.jet.lang.psi.JetBlockExpression
 import org.jetbrains.jet.lang.psi.JetClassOrObject
 import org.jetbrains.jet.lang.psi.JetClassBody
-import org.jetbrains.jet.lang.psi.JetNamedFunction
-import org.jetbrains.jet.plugin.codeInsight.ShortenReferences
 import kotlin.test.fail
 import org.jetbrains.jet.lang.psi.JetDeclarationWithBody
+import org.jetbrains.jet.plugin.refactoring.extractFunction.AnalysisResult.Status
+import com.intellij.openapi.ui.popup.JBPopupFactory
+import com.intellij.openapi.ui.MessageType
+import javax.swing.event.HyperlinkEvent
+import com.intellij.refactoring.BaseRefactoringProcessor.ConflictsInTestsException
+import com.intellij.ui.awt.RelativePoint
+import com.intellij.openapi.ui.popup.Balloon.Position
 
 public class ExtractKotlinFunctionHandler : RefactoringActionHandler {
     fun doInvoke(
@@ -59,26 +61,59 @@ public class ExtractKotlinFunctionHandler : RefactoringActionHandler {
         val project = file.getProject()
 
         val analysisResult = ExtractionData(file, elements, nextSibling).performAnalysis()
-        if (analysisResult is MaybeError) {
+
-            showErrorHint(project, editor, analysisResult.error)
+        if (ApplicationManager.getApplication()!!.isUnitTestMode() && analysisResult.status != Status.SUCCESS) {
-            return
+            throw ConflictsInTestsException(analysisResult.messages)
         }
 
-        val validationResult = (analysisResult as MaybeValue).value.validate()
+        fun proceedWithExtraction() {
-        if (!project.checkConflictsInteractively(validationResult.conflicts)) return
+            val validationResult = analysisResult.descriptor!!.validate()
+            if (!project.checkConflictsInteractively(validationResult.conflicts)) return
 
-        val descriptor =
+            val descriptor =
-                if (ApplicationManager.getApplication()!!.isUnitTestMode()) {
+                    if (ApplicationManager.getApplication()!!.isUnitTestMode()) {
-                    validationResult.descriptor
+                        validationResult.descriptor
-                }
+                    }
-                else {
+                    else {
-                    val dialog = KotlinExtractFunctionDialog(project, validationResult)
+                        val dialog = KotlinExtractFunctionDialog(project, validationResult)
-                    if (!dialog.showAndGet()) return
+                        if (!dialog.showAndGet()) return
 
-                    dialog.getCurrentDescriptor()
+                        dialog.getCurrentDescriptor()
-                }
+                    }
 
-        project.executeWriteCommand(EXTRACT_FUNCTION) { descriptor.generateFunction() }
+            project.executeWriteCommand(EXTRACT_FUNCTION) { descriptor.generateFunction() }
+        }
+
+        val message = analysisResult.messages.makeString("\n")
+        when (analysisResult.status) {
+            Status.CRITICAL_ERROR -> {
+                showErrorHint(project, editor, message)
+            }
+
+            Status.NON_CRITICAL_ERROR -> {
+                val anchorPoint = RelativePoint(
+                        editor.getContentComponent(),
+                        editor.visualPositionToXY(editor.getSelectionModel().getSelectionStartPosition()!!)
+                )
+                JBPopupFactory.getInstance()!!
+                        .createHtmlTextBalloonBuilder(
+                                "$message<br/><br/><a href=\"EXTRACT\">Proceed with extraction</a>",
+                                MessageType.WARNING,
+                                { event ->
+                                    if (event?.getEventType() == HyperlinkEvent.EventType.ACTIVATED) {
+                                        proceedWithExtraction()
+                                    }
+                                }
+                        )
+                        .setHideOnClickOutside(true)
+                        .setHideOnFrameResize(false)
+                        .setHideOnLinkClick(true)
+                        .createBalloon()
+                        .show(anchorPoint, Position.below)
+            }
+
+            Status.SUCCESS -> proceedWithExtraction()
+        }
     }
 
     override fun invoke(project: Project, editor: Editor, file: PsiFile, dataContext: DataContext?) {

idea/src/org/jetbrains/jet/plugin/refactoring/extractFunction/ExtractionDescriptor.kt

@@ -31,6 +31,8 @@ import org.jetbrains.jet.lang.psi.psiUtil.replaced
 import org.jetbrains.jet.lang.psi.JetQualifiedExpression
 import org.jetbrains.jet.lang.psi.JetTypeParameter
 import org.jetbrains.jet.lang.psi.JetTypeConstraint
+import kotlin.properties.Delegates
+import org.jetbrains.jet.plugin.refactoring.extractFunction.AnalysisResult.Status
 
 data class Parameter(
         val argumentText: String,
@@ -132,6 +134,18 @@ data class ExtractionDescriptor(
         val controlFlow: ControlFlow
 )
 
+class AnalysisResult (
+        val descriptor: ExtractionDescriptor?,
+        val status: Status,
+        val messages: List<String>
+) {
+    enum class Status {
+        SUCCESS
+        NON_CRITICAL_ERROR
+        CRITICAL_ERROR
+    }
+}
+
 class ExtractionDescriptorWithConflicts(
         val descriptor: ExtractionDescriptor,
         val conflicts: MultiMap<PsiElement, String>

idea/src/org/jetbrains/jet/plugin/refactoring/extractFunction/extractFunctionUtils.kt

@@ -31,13 +31,10 @@ import org.jetbrains.jet.lang.descriptors.DeclarationDescriptor
 import org.jetbrains.jet.lang.descriptors.CallableDescriptor
 import org.jetbrains.jet.plugin.refactoring.JetRefactoringBundle
 import org.jetbrains.jet.lang.cfg.pseudocode.PseudocodeUtil
-import org.jetbrains.jet.plugin.util.MaybeValue
-import org.jetbrains.jet.plugin.util.Maybe
 import org.jetbrains.jet.lang.psi.psiUtil.isInsideOf
 import java.util.HashSet
 import org.jetbrains.jet.lang.cfg.pseudocode.WriteValueInstruction
 import org.jetbrains.jet.lang.descriptors.VariableDescriptor
-import org.jetbrains.jet.plugin.util.MaybeError
 import org.jetbrains.jet.lang.psi.psiUtil.appendElement
 import org.jetbrains.jet.plugin.refactoring.createTempCopy
 import org.jetbrains.jet.lang.psi.psiUtil.getParentByType
@@ -92,7 +89,6 @@ import org.jetbrains.jet.lang.descriptors.TypeParameterDescriptor
 import org.jetbrains.jet.utils.DFS
 import org.jetbrains.jet.utils.DFS.Neighbors
 import org.jetbrains.jet.utils.DFS.VisitedWithSet
-import org.jetbrains.jet.utils.DFS.AbstractNodeHandler
 import org.jetbrains.jet.utils.DFS.CollectingNodeHandler
 import org.jetbrains.jet.lang.resolve.BindingContextUtils
 import org.jetbrains.jet.plugin.caches.resolve.getLazyResolveSession
@@ -100,6 +96,7 @@ import org.jetbrains.jet.plugin.project.AnalyzerFacadeWithCache
 import org.jetbrains.jet.lang.diagnostics.Errors
 import org.jetbrains.jet.lang.psi.JetTypeReference
 import org.jetbrains.jet.lang.psi.JetTypeParameterListOwner
+import org.jetbrains.jet.plugin.refactoring.extractFunction.AnalysisResult.Status
 import org.jetbrains.jet.lang.descriptors.FunctionDescriptor
 
 private val DEFAULT_FUNCTION_NAME = "myFun"
@@ -147,13 +144,16 @@ private fun List<Instruction>.analyzeControlFlow(
         bindingContext: BindingContext,
         parameters: Set<Parameter>,
         inferredType: JetType?
-): Maybe<ControlFlow, String> {
+): Pair<ControlFlow, String?> {
     val outParameters = parameters.filterTo(HashSet<Parameter>()) { it.mirrorVarName != null }
 
     if (outParameters.size > 1) {
         val outValuesStr = outParameters.map { it.argumentText }.sort().makeString(separator = "\n")
 
-        return MaybeError("${JetRefactoringBundle.message("selected.code.fragment.has.multiple.output.values")!!}\n$outValuesStr")
+        return Pair(
+                DefaultControlFlow,
+                "${JetRefactoringBundle.message("selected.code.fragment.has.multiple.output.values")!!}\n$outValuesStr"
+        )
     }
 
     val exitPoints = getExitPoints()
@@ -188,19 +188,21 @@ private fun List<Instruction>.analyzeControlFlow(
 
     if (outParameters.isNotEmpty()) {
         if (hasMeaningfulType || valuedReturnExits.isNotEmpty() || jumpExits.isNotEmpty()) {
-            return MaybeError(JetRefactoringBundle.message("selected.code.fragment.has.output.values.and.exit.points")!!)
+            return Pair(DefaultControlFlow, JetRefactoringBundle.message("selected.code.fragment.has.output.values.and.exit.points")!!)
         }
 
-        return MaybeValue(ParameterUpdate(outParameters.first()))
+        return Pair(ParameterUpdate(outParameters.first()), null)
     }
 
-    val multipleExitsError =
+    val multipleExitsError = Pair(
-            MaybeError<ControlFlow, String>(JetRefactoringBundle.message("selected.code.fragment.has.multiple.exit.points")!!)
+            DefaultControlFlow,
+            JetRefactoringBundle.message("selected.code.fragment.has.multiple.exit.points")!!
+    )
 
     if (hasMeaningfulType) {
         if (valuedReturnExits.isNotEmpty() || jumpExits.isNotEmpty()) return multipleExitsError
 
-        return MaybeValue(ExpressionEvaluation(inferredType!!))
+        return Pair(ExpressionEvaluation(inferredType!!), null)
     }
 
     if (valuedReturnExits.isNotEmpty()) {
@@ -210,22 +212,22 @@ private fun List<Instruction>.analyzeControlFlow(
             if (valuedReturnExits.size != 1) return multipleExitsError
 
             val element = valuedReturnExits.first!!.getElement()
-            return MaybeValue(ConditionalJump(listOf(element), element))
+            return Pair(ConditionalJump(listOf(element), element), null)
         }
 
         if (!valuedReturnExits.checkEquivalence(false)) return multipleExitsError
-        return MaybeValue(ExpressionEvaluationWithCallSiteReturn(valuedReturnExits.getResultType(bindingContext)))
+        return Pair(ExpressionEvaluationWithCallSiteReturn(valuedReturnExits.getResultType(bindingContext)), null)
     }
 
     if (jumpExits.isNotEmpty()) {
         if (!jumpExits.checkEquivalence(true)) return multipleExitsError
 
         val elements = jumpExits.map { it.getElement() }
-        if (defaultExits.isNotEmpty()) return MaybeValue(ConditionalJump(elements, elements.first!!))
+        if (defaultExits.isNotEmpty()) return Pair(ConditionalJump(elements, elements.first!!), null)
-        return MaybeValue(UnconditionalJump(elements, elements.first!!))
+        return Pair(UnconditionalJump(elements, elements.first!!), null)
     }
 
-    return MaybeValue(DefaultControlFlow)
+    return Pair(DefaultControlFlow, null)
 }
 
 private fun ExtractionData.createTemporaryCodeBlock(): JetBlockExpression {
@@ -306,7 +308,7 @@ private fun ExtractionData.inferParametersInfo(
         replacementMap: MutableMap<Int, Replacement>,
         parameters: MutableSet<Parameter>,
         typeParameters: MutableSet<TypeParameter>
-): MaybeError<ExtractionDescriptor, String>? {
+): String? {
     val varNameValidator = JetNameValidatorImpl(
             commonParent.getParentByType(javaClass<JetExpression>()),
             originalElements.first,
@@ -324,7 +326,7 @@ private fun ExtractionData.inferParametersInfo(
         val selector = (ref.getParent() as? JetCallExpression) ?: ref
         val superExpr = (selector.getParent() as? JetQualifiedExpression)?.getReceiverExpression() as? JetSuperExpression
         if (superExpr != null) {
-            return MaybeError(JetRefactoringBundle.message("cannot.extract.super.call")!!)
+            return JetRefactoringBundle.message("cannot.extract.super.call")!!
         }
 
         val receiverArgument = resolvedCall?.getReceiverArgument()
@@ -343,7 +345,7 @@ private fun ExtractionData.inferParametersInfo(
 
         if (classObjectClassDescriptor != null) {
             if (!classObjectClassDescriptor.canBeReferencedViaImport()) {
-                return MaybeError(JetRefactoringBundle.message("cannot.extract.non.local.declaration.ref")!!)
+                return JetRefactoringBundle.message("cannot.extract.non.local.declaration.ref")!!
             }
 
             replacementMap[refInfo.offsetInBody] = FqNameReplacement(DescriptorUtils.getFqNameSafe(originalDescriptor))
@@ -367,6 +369,8 @@ private fun ExtractionData.inferParametersInfo(
                         }
                         else bindingContext[BindingContext.EXPRESSION_TYPE, originalRef] ?: DEFAULT_PARAMETER_TYPE
 
+                if (!parameterType.processTypeIfExtractable(bindingContext, typeParameters, nonDenotableTypes)) continue
+
                 val existingParameter = extractedDescriptorToParameter[descriptorToExtract]
                 val parameter: Parameter =
                         if (existingParameter != null) {
@@ -431,7 +435,7 @@ private fun ExtractionData.inferParametersInfo(
             DescriptorRenderer.HTML.renderType(it)
         }.sort().makeString(separator = "\n")
 
-        return MaybeError("${JetRefactoringBundle.message("parameter.types.are.not.denotable")!!}\n$typeStr")
+        return "${JetRefactoringBundle.message("parameter.types.are.not.denotable")!!}\n$typeStr"
     }
 
     parameters.addAll(extractedDescriptorToParameter.values())
@@ -443,7 +447,7 @@ private fun ExtractionData.checkLocalDeclarationsWithNonLocalUsages(
         allInstructions: List<Instruction>,
         localInstructions: List<Instruction>,
         bindingContext: BindingContext
-): MaybeError<ExtractionDescriptor, String>? {
+): String? {
     // todo: non-locally used declaration can be turned into the output value
 
     val declarations = ArrayList<JetNamedDeclaration>()
@@ -460,13 +464,13 @@ private fun ExtractionData.checkLocalDeclarationsWithNonLocalUsages(
 
     if (declarations.isNotEmpty()) {
         val localVarStr = declarations.map { it.getName()!! }.sort().makeString(separator = "\n")
-        return MaybeError("${JetRefactoringBundle.message("variables.are.used.outside.of.selected.code.fragment")!!}\n$localVarStr")
+        return "${JetRefactoringBundle.message("variables.are.used.outside.of.selected.code.fragment")!!}\n$localVarStr"
     }
 
     return null
 }
 
-private fun ExtractionData.checkDeclarationsMovingOutOfScope(controlFlow: ControlFlow): MaybeError<ExtractionDescriptor, String>? {
+private fun ExtractionData.checkDeclarationsMovingOutOfScope(controlFlow: ControlFlow): String? {
     val declarationsOutOfScope = HashSet<JetNamedDeclaration>()
     if (controlFlow is JumpBasedControlFlow) {
         controlFlow.elementToInsertAfterCall.accept(
@@ -483,19 +487,23 @@ private fun ExtractionData.checkDeclarationsMovingOutOfScope(controlFlow: Contro
 
     if (declarationsOutOfScope.isNotEmpty()) {
         val declStr = declarationsOutOfScope.map { it.getName()!! }.sort().makeString(separator = "\n")
-        return MaybeError("${JetRefactoringBundle.message("declarations.will.move.out.of.scope")!!}\n$declStr")
+        return "${JetRefactoringBundle.message("declarations.will.move.out.of.scope")!!}\n$declStr"
     }
 
     return null
 }
 
-fun ExtractionData.performAnalysis(): Maybe<ExtractionDescriptor, String> {
+fun ExtractionData.performAnalysis(): AnalysisResult {
-    if (originalElements.empty) return MaybeError(JetRefactoringBundle.message("cannot.refactor.no.expresson")!!)
+    if (originalElements.empty) {
+        return AnalysisResult(null, Status.CRITICAL_ERROR, listOf(JetRefactoringBundle.message("cannot.refactor.no.expresson")!!))
+    }
 
     val commonParent = PsiTreeUtil.findCommonParent(originalElements)!!
 
     val enclosingDeclaration = commonParent.getParentByType(javaClass<JetDeclarationWithBody>())
-    if (enclosingDeclaration == null) return MaybeError(JetRefactoringBundle.message("cannot.refactor.no.container")!!)
+    if (enclosingDeclaration == null) {
+        return AnalysisResult(null, Status.CRITICAL_ERROR, listOf(JetRefactoringBundle.message("cannot.refactor.no.container")!!))
+    }
 
     val resolveSession = originalFile.getLazyResolveSession()
     val bindingContext = resolveSession.resolveToElement(enclosingDeclaration.getBodyExpression())
@@ -513,17 +521,17 @@ fun ExtractionData.performAnalysis(): Maybe<ExtractionDescriptor, String> {
     val parameterError = inferParametersInfo(
             commonParent, localInstructions, bindingContext, inferredResultType, replacementMap, parameters, typeParameters
     )
-    if (parameterError != null) return parameterError
+    if (parameterError != null) {
+        return AnalysisResult(null, Status.CRITICAL_ERROR, listOf(parameterError))
+    }
 
-    val controlFlowInfo = localInstructions.analyzeControlFlow(bindingContext, parameters, inferredResultType)
+    val messages = ArrayList<String>()
-    if (controlFlowInfo is MaybeError) return MaybeError(controlFlowInfo.error)
-    val controlFlow = (controlFlowInfo as MaybeValue).value
 
-    val localDeclError = checkLocalDeclarationsWithNonLocalUsages(pseudocode.getInstructions(), localInstructions, bindingContext)
+    val (controlFlow, controlFlowMessage) = localInstructions.analyzeControlFlow(bindingContext, parameters, inferredResultType)
-    if (localDeclError != null) return localDeclError
+    controlFlowMessage?.let { messages.add(it) }
 
-    val outOfScopeError = checkDeclarationsMovingOutOfScope(controlFlow)
+    checkLocalDeclarationsWithNonLocalUsages(pseudocode.getInstructions(), localInstructions, bindingContext)?.let { messages.add(it) }
-    if (outOfScopeError != null) return outOfScopeError
+    checkDeclarationsMovingOutOfScope(controlFlow)?.let { messages.add(it) }
 
     val functionNameValidator = JetNameValidatorImpl(
             nextSibling.getParent(),
@@ -536,7 +544,7 @@ fun ExtractionData.performAnalysis(): Maybe<ExtractionDescriptor, String> {
     val receiverParameter = if (receiverCandidates.size == 1) receiverCandidates.first() else null
     receiverParameter?.let { parameters.remove(it) }
 
-    return MaybeValue(
+    return AnalysisResult(
             ExtractionDescriptor(
                     this,
                     functionName,
@@ -546,7 +554,9 @@ fun ExtractionData.performAnalysis(): Maybe<ExtractionDescriptor, String> {
                     typeParameters.sortBy { it.originalDeclaration.getName()!! },
                     replacementMap,
                     controlFlow
-            )
+            ),
+            if (messages.empty) Status.SUCCESS else Status.NON_CRITICAL_ERROR,
+            messages
     )
 }