[extract method with object] show error when variables can not be extracted

GitOrigin-RevId: 3be26214e06d11a2ce42ff5ddb89be146d338d47
2026-09-18 09:34:34 +07:00 · 2023-01-30 21:35:36 +00:00
parent 8a3a0b1d0b
commit 9ac124a247
13 changed files with 105 additions and 63 deletions
@@ -3,9 +3,8 @@ package com.intellij.refactoring.extractMethod.newImpl
 import com.intellij.openapi.util.TextRange
 import com.intellij.psi.PsiElement
 import com.intellij.psi.PsiFile
-import java.lang.RuntimeException

-class ExtractException(message: String, file: PsiFile, val problems: List<TextRange> = emptyList()): RuntimeException(message) {
+open class ExtractException(message: String, val file: PsiFile, val problems: List<TextRange> = emptyList()): RuntimeException(message) {
  constructor(message: String, problems: List<PsiElement>): this(message, problems.first().containingFile, problems.map { it.textRange })
  constructor(message: String, problem: PsiElement): this(message, listOf(problem))
  constructor(message: String, file: PsiFile): this(message, file, emptyList())
@@ -40,12 +40,7 @@ fun findExtractOptions(elements: List<PsiElement>): ExtractOptions {
  val flowOutput = findFlowOutput(analyzer)
                   ?: throw ExtractException(JavaRefactoringBundle.message("extract.method.error.many.exits"), elements.first())

-  val outVariables = analyzer.findOutputVariables()
-  if (outVariables.size >= 2 && canExtractStatementsFromScope(flowOutput.statements, elements)) {
-    throw ExtractMultipleVariablesException(outVariables, elements)
-  }
-
-  val variableData = findVariableData(analyzer, outVariables)
+  val variableData = findVariableData(analyzer, analyzer.findOutputVariables())

  val expression = elements.singleOrNull() as? PsiExpression

@@ -204,9 +199,9 @@ private fun findFlowData(analyzer: CodeFragmentAnalyzer, flowOutput: FlowOutput)

 private fun findVariableData(analyzer: CodeFragmentAnalyzer, variables: List<PsiVariable>): DataOutput {
  val variable = when {
+    variables.size > 1 -> throw ExtractMultipleVariablesException(variables, analyzer.elements)
    analyzer.elements.singleOrNull() is PsiExpression && variables.isNotEmpty() ->
      throw ExtractException(JavaRefactoringBundle.message("extract.method.error.variable.in.expression"), variables)
-    variables.size > 1 -> throw ExtractException(JavaRefactoringBundle.message("extract.method.error.many.outputs"), variables)
    variables.isEmpty() -> return EmptyOutput()
    else -> variables.single()
  }
@@ -1,7 +1,11 @@
 // Copyright 2000-2023 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license.
 package com.intellij.refactoring.extractMethod.newImpl

+import com.intellij.java.refactoring.JavaRefactoringBundle
 import com.intellij.psi.PsiElement
 import com.intellij.psi.PsiVariable
+import org.jetbrains.annotations.Nls

-class ExtractMultipleVariablesException(val variables: List<PsiVariable>, val scope: List<PsiElement>): RuntimeException()
+class ExtractMultipleVariablesException(val variables: List<PsiVariable>, val scope: List<PsiElement>): ExtractException(errorMessage(), variables)
+
+private fun errorMessage(): @Nls String = JavaRefactoringBundle.message("extract.method.error.many.outputs")
@@ -2,7 +2,6 @@
 package com.intellij.refactoring.extractMethod.newImpl

 import com.intellij.codeInsight.Nullability
-import com.intellij.codeInsight.highlighting.HighlightManager
 import com.intellij.ide.util.PropertiesComponent
 import com.intellij.java.refactoring.JavaRefactoringBundle
 import com.intellij.openapi.application.ReadAction
@@ -11,7 +10,6 @@ import com.intellij.openapi.application.invokeLater
 import com.intellij.openapi.command.CommandProcessor
 import com.intellij.openapi.diagnostic.Logger
 import com.intellij.openapi.editor.Editor
-import com.intellij.openapi.editor.colors.EditorColors
 import com.intellij.openapi.editor.ex.EditorSettingsExternalizable
 import com.intellij.openapi.progress.ProgressIndicator
 import com.intellij.openapi.progress.ProgressManager
@@ -19,10 +17,8 @@ import com.intellij.openapi.progress.Task
 import com.intellij.openapi.project.Project
 import com.intellij.openapi.util.TextRange
 import com.intellij.openapi.util.ThrowableComputable
-import com.intellij.openapi.wm.WindowManager
 import com.intellij.psi.*
 import com.intellij.psi.util.PsiEditorUtil
-import com.intellij.refactoring.HelpID
 import com.intellij.refactoring.JavaRefactoringSettings
 import com.intellij.refactoring.RefactoringBundle
 import com.intellij.refactoring.extractMethod.ExtractMethodDialog
@@ -34,6 +30,7 @@ import com.intellij.refactoring.extractMethod.newImpl.ExtractMethodPipeline.find
 import com.intellij.refactoring.extractMethod.newImpl.ExtractMethodPipeline.selectOptionWithTargetClass
 import com.intellij.refactoring.extractMethod.newImpl.ExtractMethodPipeline.withFilteredAnnotations
 import com.intellij.refactoring.extractMethod.newImpl.inplace.ExtractMethodPopupProvider
+import com.intellij.refactoring.extractMethod.newImpl.inplace.InplaceExtractUtils
 import com.intellij.refactoring.extractMethod.newImpl.inplace.InplaceMethodExtractor
 import com.intellij.refactoring.extractMethod.newImpl.inplace.extractInDialog
 import com.intellij.refactoring.extractMethod.newImpl.parameterObject.ParameterObjectExtractor
@@ -85,10 +82,8 @@ class MethodExtractor {
      invokeLater { ParameterObjectExtractor.run(editor, variables, e.scope) }
      return null
    }
-    catch (e: ExtractException) {
-      val message = JavaRefactoringBundle.message("extract.method.error.prefix") + " " + (e.message ?: "")
-      CommonRefactoringUtil.showErrorHint(file.project, editor, message, ExtractMethodHandler.getRefactoringName(), HelpID.EXTRACT_METHOD)
-      showError(editor, e.problems)
+    catch (exception: ExtractException) {
+      InplaceExtractUtils.showExtractErrorHint(editor, exception)
      return null
    }
  }
@@ -222,17 +217,6 @@ class MethodExtractor {
    return true
  }

-  fun showError(editor: Editor, ranges: List<TextRange>) {
-    val project = editor.project ?: return
-    if (ranges.isEmpty()) return
-    val highlightManager = HighlightManager.getInstance(project)
-    ranges.forEach { textRange ->
-      highlightManager.addRangeHighlight(editor, textRange.startOffset, textRange.endOffset,
-                                         EditorColors.SEARCH_RESULT_ATTRIBUTES, true, null)
-    }
-    WindowManager.getInstance().getStatusBar(project).info = RefactoringBundle.message("press.escape.to.remove.the.highlighting")
-  }
-
  fun prepareRefactoringElements(extractOptions: ExtractOptions): ExtractedElements {
    val dependencies = withFilteredAnnotations(extractOptions)
    val factory = PsiElementFactory.getInstance(dependencies.project)
@@ -20,6 +20,7 @@ import com.intellij.openapi.editor.Document
 import com.intellij.openapi.editor.Editor
 import com.intellij.openapi.editor.Inlay
 import com.intellij.openapi.editor.RangeMarker
+import com.intellij.openapi.editor.colors.EditorColors
 import com.intellij.openapi.editor.colors.TextAttributesKey
 import com.intellij.openapi.editor.event.DocumentEvent
 import com.intellij.openapi.editor.event.DocumentListener
@@ -34,11 +35,17 @@ import com.intellij.openapi.ui.popup.Balloon
 import com.intellij.openapi.util.Disposer
 import com.intellij.openapi.util.TextRange
 import com.intellij.openapi.vfs.VirtualFile
+import com.intellij.openapi.wm.WindowManager
 import com.intellij.psi.*
 import com.intellij.psi.util.PsiTreeUtil
 import com.intellij.psi.util.PsiUtil
+import com.intellij.refactoring.HelpID
+import com.intellij.refactoring.RefactoringBundle
+import com.intellij.refactoring.extractMethod.ExtractMethodHandler
+import com.intellij.refactoring.extractMethod.newImpl.ExtractException
 import com.intellij.refactoring.rename.inplace.TemplateInlayUtil
 import com.intellij.refactoring.suggested.range
+import com.intellij.refactoring.util.CommonRefactoringUtil
 import com.intellij.ui.GotItTooltip
 import com.intellij.util.SmartList
 import org.jetbrains.annotations.Nls
@@ -64,6 +71,24 @@ object InplaceExtractUtils {
    return true
  }

+  fun showExtractErrorHint(editor: Editor, exception: ExtractException){
+    val file = exception.file
+    val message = JavaRefactoringBundle.message("extract.method.error.prefix") + " " + (exception.message ?: "")
+    CommonRefactoringUtil.showErrorHint(file.project, editor, message, ExtractMethodHandler.getRefactoringName(), HelpID.EXTRACT_METHOD)
+    highlightErrors(editor, exception.problems)
+  }
+
+  private fun highlightErrors(editor: Editor, ranges: List<TextRange>) {
+    val project = editor.project ?: return
+    if (ranges.isEmpty()) return
+    val highlightManager = HighlightManager.getInstance(project)
+    ranges.forEach { textRange ->
+      highlightManager.addRangeHighlight(editor, textRange.startOffset, textRange.endOffset,
+                                         EditorColors.SEARCH_RESULT_ATTRIBUTES, true, null)
+    }
+    WindowManager.getInstance().getStatusBar(project).info = RefactoringBundle.message("press.escape.to.remove.the.highlighting")
+  }
+
  fun findTypeParameters(types: List<PsiType>): List<PsiTypeParameter>{
    return types.mapNotNull{ type -> PsiUtil.resolveClassInClassTypeOnly(type) as? PsiTypeParameter }
  }
@@ -5,12 +5,10 @@ import com.intellij.psi.*
 import com.intellij.psi.util.PsiTreeUtil
 import com.siyeh.ig.psiutils.TypeUtils

-class ClassParameterObjectBuilder(private val pojoClass: PsiClass, private val references: List<PsiReferenceExpression>): ParameterObjectBuilder {
+class ClassParameterObjectBuilder(private val pojoClass: PsiClass): ParameterObjectBuilder {
  companion object {
-    fun create(variables: List<PsiVariable>, scope: List<PsiElement>): ClassParameterObjectBuilder {
-      val pojoClass = createPojoClass(variables)
-      val affectedReferences = ParameterObjectUtils.findAffectedReferences(variables, scope.last().nextSibling)
-      return ClassParameterObjectBuilder(pojoClass, affectedReferences)
+    fun create(variables: List<PsiVariable>): ClassParameterObjectBuilder {
+      return ClassParameterObjectBuilder(createPojoClass(variables))
    }

    private fun createPojoClass(variables: List<PsiVariable>): PsiClass {
@@ -57,6 +55,4 @@ class ClassParameterObjectBuilder(private val pojoClass: PsiClass, private val r
    val place = replacement.textRange.startOffset
    return PsiTreeUtil.findElementOfClassAtOffset(replacement.containingFile, place, PsiReferenceExpression::class.java, false)
  }
-
-  override fun getAffectedReferences(): List<PsiReferenceExpression> = references
 }
@@ -11,5 +11,4 @@ interface ParameterObjectBuilder {
  fun createDeclaration(): PsiDeclarationStatement
  fun createReferenceReplacement(reference: PsiReferenceExpression): PsiExpression
  fun findVariableReferenceInReplacement(replacement: PsiExpression): PsiReferenceExpression?
-  fun getAffectedReferences(): List<PsiReferenceExpression>
 }
@@ -15,10 +15,12 @@ import com.intellij.openapi.util.Disposer
 import com.intellij.psi.*
 import com.intellij.psi.util.PsiTreeUtil
 import com.intellij.refactoring.extractMethod.ExtractMethodHandler
+import com.intellij.refactoring.extractMethod.newImpl.ExtractMultipleVariablesException
 import com.intellij.refactoring.extractMethod.newImpl.MethodExtractor
 import com.intellij.refactoring.extractMethod.newImpl.inplace.EditorState
 import com.intellij.refactoring.extractMethod.newImpl.inplace.ExtractMethodTemplateBuilder
 import com.intellij.refactoring.extractMethod.newImpl.inplace.InplaceExtractUtils
+import com.intellij.refactoring.extractMethod.newImpl.inplace.InplaceExtractUtils.createGreedyRangeMarker
 import com.intellij.refactoring.extractMethod.newImpl.inplace.TemplateField

 private data class IntroduceObjectResult(
@@ -32,22 +34,26 @@ object ParameterObjectExtractor {
    require(variables.isNotEmpty())
    require(scope.isNotEmpty())

+    val affectedReferences = ParameterObjectUtils.findAffectedReferences(variables, scope)
+    if (affectedReferences == null) {
+      InplaceExtractUtils.showExtractErrorHint(editor, ExtractMultipleVariablesException(variables, scope))
+      return
+    }
    val objectBuilder = if (HighlightingFeature.RECORDS.isAvailable(variables.first())) {
-      RecordParameterObjectBuilder.create(variables, scope)
+      RecordParameterObjectBuilder.create(variables)
    } else {
-      ClassParameterObjectBuilder.create(variables, scope)
+      ClassParameterObjectBuilder.create(variables)
    }
    val file = scope.first().containingFile
    val project = file.project
-    val extractRange = InplaceExtractUtils.createGreedyRangeMarker(file.viewProvider.document,
-                                                                   scope.first().textRange.union(scope.last().textRange))
+    val extractRange = createGreedyRangeMarker(file.viewProvider.document, scope.first().textRange.union(scope.last().textRange))
    val editorState = EditorState(editor)
    WriteCommandAction.writeCommandAction(project).run<Throwable> {
      try {
        val disposable = Disposer.newDisposable()
        val startMarkAction = StartMarkAction.start(editor, project, ExtractMethodHandler.getRefactoringName())
        Disposer.register(disposable) { FinishMarkAction.finish(project, editor, startMarkAction) }
-        val (introducedClass, declaration, replacements) = introduceObjectForVariables(objectBuilder, variables, scope.last())
+        val (introducedClass, declaration, replacements) = introduceObjectForVariables(objectBuilder, variables, affectedReferences, scope.last())
        val introducedVariableReferences = replacements.map { replacement ->
          objectBuilder.findVariableReferenceInReplacement(replacement) ?: throw IllegalStateException()
        }
@@ -84,8 +90,11 @@ object ParameterObjectExtractor {
    return preview
  }

-  private fun introduceObjectForVariables(builder: ParameterObjectBuilder, variables: List<PsiVariable>, placeForDeclaration: PsiElement): IntroduceObjectResult {
-    val referenceReplacements = builder.getAffectedReferences()
+  private fun introduceObjectForVariables(builder: ParameterObjectBuilder,
+                                          variables: List<PsiVariable>,
+                                          affectedReferences: List<PsiReferenceExpression>,
+                                          placeForDeclaration: PsiElement): IntroduceObjectResult {
+    val referenceReplacements = affectedReferences
      .map { reference -> reference.replace(builder.createReferenceReplacement(reference)) as PsiExpression }
    val classAnchor = PsiTreeUtil.getParentOfType(variables.first(), PsiMember::class.java) ?: throw IllegalStateException()
    val psiClass = builder.createClass()
@@ -24,24 +24,21 @@ object ParameterObjectUtils {
    return factory.createVariableDeclarationStatement("result", type, expression)
  }

-  fun findAffectedReferences(variables: List<PsiVariable>, startingElement: PsiElement?): List<PsiReferenceExpression> {
-    val startingPoint = startingElement?.textRange?.startOffset ?: return emptyList()
-    return  variables.flatMap { findAffectedReferences(it, startingPoint) }
+  fun findAffectedReferences(variables: List<PsiVariable>, scope: List<PsiElement>): List<PsiReferenceExpression>? {
+    return  variables.flatMap { findAffectedReferences(it, scope) ?: return null }
  }

-  private fun findAffectedReferences(variable: PsiVariable, startingOffset: Int): List<PsiReferenceExpression> {
+  private fun findAffectedReferences(variable: PsiVariable, scope: List<PsiElement>): List<PsiReferenceExpression>? {
+    val startingOffset = scope.last().textRange.endOffset
    val references = ReferencesSearch.search(variable)
      .mapNotNull { it.element as? PsiReferenceExpression }
      .filter { reference -> reference.textRange.startOffset >= startingOffset }
      .sortedBy { reference -> reference.textRange.startOffset }
-    val firstAssignment = references.find { reference -> PsiUtil.isAccessedForWriting(reference) }
-    val endPoint = PsiTreeUtil.getParentOfType(firstAssignment, PsiAssignmentExpression::class.java)?.textRange?.endOffset
-    return if (firstAssignment != null && endPoint != null) {
-      references.filter { reference -> reference.textRange.endOffset <= endPoint } - firstAssignment
-    }
-    else {
-      references
-    }
+    val firstAssignment = references.find { reference -> PsiUtil.isAccessedForWriting(reference) } ?: return references
+    val assignmentExpression = PsiTreeUtil.getParentOfType(firstAssignment, PsiAssignmentExpression::class.java)
+    if (assignmentExpression == null) return null
+    if (assignmentExpression.parent.parent != PsiTreeUtil.findCommonParent(assignmentExpression, scope.last())) return null
+    return references.filter { reference -> reference.textRange.endOffset <= assignmentExpression.textRange.endOffset } - firstAssignment
  }

 }
@@ -4,14 +4,12 @@ package com.intellij.refactoring.extractMethod.newImpl.parameterObject
 import com.intellij.psi.*
 import com.intellij.psi.util.PsiTreeUtil

-class RecordParameterObjectBuilder(private val record: PsiClass, private val references: List<PsiReferenceExpression>): ParameterObjectBuilder {
+class RecordParameterObjectBuilder(private val record: PsiClass): ParameterObjectBuilder {

  companion object {

-    fun create(variables: List<PsiVariable>, scope: List<PsiElement>): RecordParameterObjectBuilder {
-      val record = createRecord(variables)
-      val affectedReferences = ParameterObjectUtils.findAffectedReferences(variables, scope.last().nextSibling)
-      return RecordParameterObjectBuilder(record, affectedReferences)
+    fun create(variables: List<PsiVariable>): RecordParameterObjectBuilder {
+      return RecordParameterObjectBuilder(createRecord(variables))
    }

    private fun createRecord(variables: List<PsiVariable>): PsiClass {
@@ -37,6 +35,4 @@ class RecordParameterObjectBuilder(private val record: PsiClass, private val ref
    val place = replacement.textRange.startOffset
    return PsiTreeUtil.findElementOfClassAtOffset(replacement.containingFile, place, PsiReferenceExpression::class.java, false)
  }
-
-  override fun getAffectedReferences(): List<PsiReferenceExpression> = references
 }
@@ -0,0 +1,14 @@
+public class Test {
+
+  void test(boolean p) {
+    <selection>int x = 42;
+    int y = 0;
+    System.out.println();</selection>
+
+    if (p) {
+      x = (x + y)/2;
+    }
+
+    System.out.println("Point(" + x + ", " + y + ")");
+  }
+}
@@ -0,0 +1,12 @@
+public class Test {
+
+  void test(boolean p) {
+    <selection>int x = 42;
+    int y = 0;
+    System.out.println();</selection>
+
+    x++;
+
+    System.out.println("Point(" + x + ", " + y + ")");
+  }
+}
@@ -363,6 +363,18 @@ class ExtractMethodAndDuplicatesInplaceTest: LightJavaCodeInsightTestCase() {
    require(getActiveTemplate() != null)
  }

+  fun testIntroduceObjectFailedWithAssignment1(){
+    assertThrows(RefactoringErrorHintException::class.java) {
+      doTest()
+    }
+  }
+
+  fun testIntroduceObjectFailedWithAssignment2(){
+    assertThrows(RefactoringErrorHintException::class.java) {
+      doTest()
+    }
+  }
+
  fun testRefactoringListener(){
    templateTest {
      configureByFile("$BASE_PATH/${getTestName(false)}.java")