IDEA-64819 improve 'Extract method' refactoring for Groovy: support multiple output parameters

2026-09-27 10:03:11 +07:00 · 2011-01-31 17:31:22 +03:00
parent a792d338c3
commit 2af7879345
12 changed files with 303 additions and 55 deletions
@@ -149,11 +149,19 @@ public class GroovyPsiElementFactoryImpl extends GroovyPsiElementFactory {
      text.append(typeText).append(" ");
    }

+    if (identifiers.length > 1 && initializer != null) {
+      text.append('(');
+    }
    for (int i = 0; i < identifiers.length; i++) {
      if (i > 0) text.append(", ");
      String identifier = identifiers[i];
      text.append(identifier);
    }
+
+    if (identifiers.length > 1 && initializer != null) {
+      text.append(')');
+    }
+
    GrExpression expr;

    if (initializer != null) {
@@ -18,12 +18,9 @@ package org.jetbrains.plugins.groovy.refactoring.extractMethod;

 import com.intellij.openapi.project.Project;
 import com.intellij.openapi.util.Condition;
-import com.intellij.psi.PsiElement;
-import com.intellij.psi.PsiModifier;
-import com.intellij.psi.PsiType;
+import com.intellij.psi.*;
 import com.intellij.util.containers.ContainerUtil;
 import org.jetbrains.annotations.NotNull;
-import org.jetbrains.annotations.Nullable;
 import org.jetbrains.plugins.groovy.lang.psi.api.statements.GrStatement;
 import org.jetbrains.plugins.groovy.lang.psi.api.statements.blocks.GrCodeBlock;
 import org.jetbrains.plugins.groovy.lang.psi.api.statements.branch.GrReturnStatement;
@@ -40,7 +37,7 @@ import java.util.*;
 public class ExtractMethodInfoHelper {

  private final Map<String, ParameterInfo> myInputNamesMap = new HashMap<String, ParameterInfo>();
-  private final String myOutputName;
+  private final VariableInfo[] myOutputNames;
  private final PsiType myOutputType;
  private final GrMemberOwner myTargetClass;
  private final boolean myIsStatic;
@@ -52,7 +49,7 @@ public class ExtractMethodInfoHelper {
  private final GrStatement[] myStatements;

  public ExtractMethodInfoHelper(VariableInfo[] inputInfos,
-                                 VariableInfo outputInfo,
+                                 VariableInfo[] outputInfos,
                                 PsiElement[] innerElements,
                                 GrStatement[] statements,
                                 GrMemberOwner targetClass,
@@ -80,19 +77,22 @@ public class ExtractMethodInfoHelper {
    }

    PsiType outputType = PsiType.VOID;
-    if (outputInfo != null) {
-      myOutputName = outputInfo.getName();
-      outputType = outputInfo.getType();
+    myOutputNames = outputInfos;
+    if (outputInfos.length > 0) {
+      if (outputInfos.length == 1) {
+        outputType = outputInfos[0].getType();
+      }
+      else {
+        outputType = JavaPsiFacade.getElementFactory(myProject).createTypeFromText(CommonClassNames.JAVA_UTIL_LIST, myTargetClass);
+      }
    }
    else if (ExtractMethodUtil.isSingleExpression(statements)) {
      final GrStatement lastExpr = statements[statements.length - 1];
      if (!(lastExpr.getParent() instanceof GrCodeBlock)) {
        outputType = ((GrExpression)lastExpr).getType();
      }
-      myOutputName = null;
    }
    else {
-      myOutputName = null;
      if (myIsReturnStatement) {
        assert returnStatements.size() > 0;
        List<PsiType> types = new ArrayList<PsiType>(returnStatements.size());
@@ -103,7 +103,7 @@ public class ExtractMethodInfoHelper {
              types.add(returnValue.getType());
            }
          }
-          else if (statement instanceof GrExpression){
+          else if (statement instanceof GrExpression) {
            types.add(((GrExpression)statement).getType());
          }
        }
@@ -130,9 +130,9 @@ public class ExtractMethodInfoHelper {
    return infos;
  }

-  @Nullable
-  public String getOutputName() {
-    return myOutputName;
+  @NotNull
+  public VariableInfo[] getOutputNames() {
+    return myOutputNames;
  }

  /**
@@ -22,6 +22,7 @@ import com.intellij.refactoring.ui.ConflictsDialog;
 import com.intellij.refactoring.util.CommonRefactoringUtil;
 import com.intellij.util.ArrayUtil;
 import com.intellij.util.IncorrectOperationException;
+import com.intellij.util.containers.HashMap;
 import gnu.trove.TObjectHashingStrategy;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
@@ -46,6 +47,8 @@ import org.jetbrains.plugins.groovy.lang.psi.api.statements.typedef.GrTypeDefini
 import org.jetbrains.plugins.groovy.lang.psi.api.statements.typedef.members.GrMethod;
 import org.jetbrains.plugins.groovy.lang.psi.api.util.GrStatementOwner;
 import org.jetbrains.plugins.groovy.lang.psi.api.util.GrVariableDeclarationOwner;
+import org.jetbrains.plugins.groovy.lang.psi.dataFlow.reachingDefs.VariableInfo;
+import org.jetbrains.plugins.groovy.lang.psi.impl.statements.expressions.TypesUtil;
 import org.jetbrains.plugins.groovy.lang.psi.impl.synthetic.GroovyScriptClass;
 import org.jetbrains.plugins.groovy.lang.psi.util.PsiUtil;
 import org.jetbrains.plugins.groovy.lang.resolve.ResolveUtil;
@@ -53,10 +56,7 @@ import org.jetbrains.plugins.groovy.refactoring.GroovyRefactoringBundle;
 import org.jetbrains.plugins.groovy.refactoring.GroovyRefactoringUtil;
 import org.jetbrains.plugins.groovy.refactoring.inline.GroovyInlineMethodUtil;

-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.List;
-import java.util.Map;
+import java.util.*;

 /**
 * @author ilyas
@@ -91,21 +91,84 @@ public class ExtractMethodUtil {
  }

  @NotNull
-  static GrStatement createResultStatement(ExtractMethodInfoHelper helper, @NotNull String methodName) {
-    String name = helper.getOutputName();
+  static GrStatement[] createResultStatement(ExtractMethodInfoHelper helper, @NotNull String methodName) {
+    VariableInfo[] outputVars = helper.getOutputNames();
+
    PsiType type = helper.getOutputType();
    GrStatement[] statements = helper.getStatements();
    GrMethodCallExpression callExpression = createMethodCallByHelper(methodName, helper);
-    if ((name == null || PsiType.VOID.equals(type)) && !helper.isReturnStatement()) return callExpression;
+
+    if ((outputVars.length == 0 || PsiType.VOID.equals(type)) && !helper.isReturnStatement()) return new GrStatement[]{callExpression};
    GroovyPsiElementFactory factory = GroovyPsiElementFactory.getInstance(helper.getProject());
    if (helper.isReturnStatement()) {
-      return factory.createStatementFromText("return " + callExpression.getText());
-    } else if (name != null && mustAddVariableDeclaration(statements, name)) {
-      return factory.createVariableDeclaration(ArrayUtil.EMPTY_STRING_ARRAY, callExpression,
-                                               type.equalsToText("java.lang.Object") ? null : type, name);
-    } else {
-      return factory.createExpressionFromText(name + "= " + callExpression.getText());
+      return new GrStatement[]{factory.createStatementFromText("return " + callExpression.getText())};
    }
+
+    assert outputVars.length > 0;
+
+    final List<VariableInfo> mustAdd = mustAddVariableDeclaration(statements, outputVars);
+    if (mustAdd.size() == 0) {
+      return new GrStatement[]{createAssignment(outputVars, callExpression, helper)};
+    }
+    if (mustAdd.size() == outputVars.length && outputVars.length == 1) {
+      final GrVariableDeclaration decl = factory.createVariableDeclaration(ArrayUtil.EMPTY_STRING_ARRAY, callExpression,
+                                                                           outputVars[0].getType(), outputVars[0].getName());
+      return new GrVariableDeclaration[]{decl};
+    }
+    List<GrStatement> result = generateVarDeclarations(mustAdd, helper.getProject(), null);
+    result.add(createAssignment(outputVars, callExpression, helper));
+
+    return result.toArray(new GrStatement[result.size()]);
+  }
+
+  private static List<GrStatement> generateVarDeclarations(List<VariableInfo> varInfos,
+                                                           Project project,
+                                                           @Nullable GrExpression initializer) {
+    List<GrStatement> result = new ArrayList<GrStatement>();
+    if (varInfos.size() == 0) return result;
+
+    GroovyPsiElementFactory factory = GroovyPsiElementFactory.getInstance(project);
+    boolean distinctDeclaration = haveDifferentTypes(varInfos);
+
+    if (distinctDeclaration) {
+      for (VariableInfo info : varInfos) {
+        result.add(factory.createVariableDeclaration(ArrayUtil.EMPTY_STRING_ARRAY, null, info.getType(), info.getName()));
+      }
+    }
+    else {
+      String[] names = new String[varInfos.size()];
+      for (int i = 0, mustAddLength = varInfos.size(); i < mustAddLength; i++) {
+        names[i] = varInfos.get(i).getName();
+      }
+      result.add(factory.createVariableDeclaration(ArrayUtil.EMPTY_STRING_ARRAY, initializer, varInfos.get(0).getType(), names));
+    }
+    return result;
+  }
+
+  private static boolean haveDifferentTypes(List<VariableInfo> varInfos) {
+    if (varInfos.size() < 2) return true;
+    Set<String> diffTypes = new com.intellij.util.containers.hash.HashSet<String>();
+    for (VariableInfo info : varInfos) {
+      final PsiType t = info.getType();
+      diffTypes.add(t == null ? null : TypesUtil.unboxPrimitiveTypeWrapper(t).getCanonicalText());
+    }
+    return diffTypes.size() > 1;
+  }
+
+  private static GrStatement createAssignment(VariableInfo[] infos, GrMethodCallExpression callExpression, ExtractMethodInfoHelper helper) {
+    StringBuilder text = new StringBuilder();
+    if (infos.length > 1) text.append('(');
+    for (VariableInfo info : infos) {
+      text.append(info.getName()).append(", ");
+    }
+    if (infos.length > 1) {
+      text.replace(text.length() - 2, text.length(), ") =");
+    }
+    else {
+      text.replace(text.length() - 2, text.length(), " = ");
+    }
+    text.append(callExpression.getText());
+    return GroovyPsiElementFactory.getInstance(helper.getProject()).createExpressionFromText(text.toString());
  }

  static boolean validateMethod(GrMethod method, ExtractMethodInfoHelper helper) {
@@ -154,16 +217,31 @@ public class ExtractMethodUtil {
  /*
  To declare or not a variable to which method call result will be assigned.
   */
-  private static boolean mustAddVariableDeclaration(@NotNull GrStatement[] statements, @NotNull String varName) {
+  private static List<VariableInfo> mustAddVariableDeclaration(@NotNull GrStatement[] statements, @NotNull VariableInfo[] vars) {
+    Map<String, VariableInfo> names = new HashMap<String, VariableInfo>();
+    for (VariableInfo var : vars) {
+      names.put(var.getName(), var);
+    }
+    List<VariableInfo> result = new ArrayList<VariableInfo>();
+
    for (GrStatement statement : statements) {
      if (statement instanceof GrVariableDeclaration) {
-        GrVariableDeclaration declaration = (GrVariableDeclaration) statement;
+        GrVariableDeclaration declaration = (GrVariableDeclaration)statement;
        for (GrVariable variable : declaration.getVariables()) {
-          if (varName.equals(variable.getName())) return true;
+          final VariableInfo removed = names.remove(variable.getName());
+          if (removed != null) {
+            result.add(removed);
+          }
        }
      }
    }
-    return ResolveUtil.resolveProperty(statements[0], varName) == null;
+    for (String varName : names.keySet()) {
+      if (ResolveUtil.resolveProperty(statements[0], varName) == null) {
+        result.add(names.get(varName));
+      }
+    }
+
+    return result;
  }

  private static boolean containVariableDeclaration(@NotNull GrStatement[] statements, @NotNull String varName) {
@@ -226,33 +304,50 @@ public class ExtractMethodUtil {
    buffer.append(") { \n");

    GroovyPsiElementFactory factory = GroovyPsiElementFactory.getInstance(helper.getProject());
-    String outputName = helper.getOutputName();
+    VariableInfo[] outputInfos = helper.getOutputNames();

    ParameterInfo[] infos = helper.getParameterInfos();
-    boolean outputIsParameter = false;
-    if (outputName != null) {
-      for (ParameterInfo info : infos) {
-        if (outputName.equals(info.getOldName())) {
-          outputIsParameter = true;
+    boolean[] outputIsParameter = new boolean[outputInfos.length];
+    for (VariableInfo outputName : outputInfos) {
+      for (int i = 0; i < infos.length; i++) {
+        if (outputName.getName().equals(infos[i].getOldName())) {
+          outputIsParameter[i] = true;
        }
      }
    }
-    if (type != PsiType.VOID && outputName != null && !outputIsParameter &&
-        !mustAddVariableDeclaration(helper.getStatements(), outputName) &&
-        !containVariableDeclaration(helper.getStatements(), outputName)) {
-      GrVariableDeclaration decl = factory.createVariableDeclaration(ArrayUtil.EMPTY_STRING_ARRAY, null, type, outputName);
-      buffer.append(decl.getText()).append("\n");
+
+    final Set<VariableInfo> mustAdd = new HashSet<VariableInfo>(mustAddVariableDeclaration(helper.getStatements(), outputInfos));
+
+    List<VariableInfo> genDecl = new ArrayList<VariableInfo>();
+    for (int i = 0; i < outputInfos.length; i++) {
+      VariableInfo info = outputInfos[i];
+      if (type != PsiType.VOID && !outputIsParameter[i] &&
+          !mustAdd.contains(info) &&
+          !containVariableDeclaration(helper.getStatements(), info.getName())) {
+        genDecl.add(info);
+      }
    }
-    if (!ExtractMethodUtil.isSingleExpression(helper.getStatements())) {
+    final List<GrStatement> statements = generateVarDeclarations(genDecl, helper.getProject(), null);
+    for (GrStatement statement : statements) {
+      buffer.append(statement.getText()).append('\n');
+    }
+
+    if (!isSingleExpression(helper.getStatements())) {
      for (PsiElement element : helper.getInnerElements()) {
        buffer.append(element.getText());
      }
      //append return statement
-      if (type != PsiType.VOID && outputName != null) {
+      if (type != PsiType.VOID && outputInfos.length > 0) {
        buffer.append("\n return ");
-        buffer.append(outputName);
+        if (outputInfos.length > 1) buffer.append('[');
+        for (VariableInfo info : outputInfos) {
+          buffer.append(info.getName()).append(", ");
+        }
+        buffer.delete(buffer.length() - 2, buffer.length());
+        if (outputInfos.length > 1) buffer.append(']');
      }
-    } else {
+    }
+    else {
      GrExpression expr = (GrExpression)PsiUtil.skipParentheses((GrExpression)helper.getStatements()[0], false);
      buffer.append(PsiType.VOID.equals(type) ? "" : "return ").append(expr != null ? expr.getText() : "");
    }
@@ -160,7 +160,7 @@ public class GroovyExtractMethodHandler implements RefactoringActionHandler {
    FragmentVariableInfos fragmentVariableInfos = ReachingDefinitionsCollector.obtainVariableFlowInformation(statement0, statements[statements.length - 1]);
    VariableInfo[] inputInfos = fragmentVariableInfos.getInputVariableNames();
    VariableInfo[] outputInfos = fragmentVariableInfos.getOutputVariableNames();
-    if (outputInfos.length > 1 ||
+    if (/*outputInfos.length > 1 ||*/
        outputInfos.length == 1 && returnStatements.size() > 0) {
      String message = GroovyRefactoringBundle.message("multiple.output.values");
      showErrorMessage(message, project, editor);
@@ -187,10 +187,9 @@ public class GroovyExtractMethodHandler implements RefactoringActionHandler {
      return false;
    }

-    VariableInfo outputInfo = outputInfos.length == 0 ? null : outputInfos[0];
    boolean canBeStatic = ExtractMethodUtil.canBeStatic(statement0);

-    ExtractMethodInfoHelper helper = new ExtractMethodInfoHelper(inputInfos, outputInfo, elements, statements, owner, canBeStatic, returnStatements);
+    ExtractMethodInfoHelper helper = new ExtractMethodInfoHelper(inputInfos, outputInfos, elements, statements, owner, canBeStatic, returnStatements);

    final String methodName;
    if (!ApplicationManager.getApplication().isUnitTestMode()) {
@@ -230,11 +229,15 @@ public class GroovyExtractMethodHandler implements RefactoringActionHandler {

          if (declarationOwner != null && !ExtractMethodUtil.isSingleExpression(helper.getStatements())) {
            // Replace set of statements
-            final GrStatement newStatement = ExtractMethodUtil.createResultStatement(helper, methodName);
+            final GrStatement[] newStatement = ExtractMethodUtil.createResultStatement(helper, methodName);
            // add call statement
            final GrStatement[] statements = helper.getStatements();
            assert statements.length > 0;
-            realStatement = declarationOwner.addStatementBefore(newStatement, statements[0]);
+            realStatement = null;
+            for (GrStatement statement : newStatement) {
+              realStatement = declarationOwner.addStatementBefore(statement, statements[0]);
+            }
+            assert realStatement != null;
            // remove old statements
            ExtractMethodUtil.removeOldStatements(declarationOwner, helper);
            PsiImplUtil.removeNewLineAfter(realStatement);
@@ -63,7 +63,7 @@ public class ExtractMethodTest extends LightGroovyTestCase {
  public void testClos_em() throws Throwable { doTest(); }
  public void testEm1() throws Throwable { doTest(); }
  public void testEnum1() throws Throwable { doTest(); }
-  public void testErr1() throws Throwable { doAntiTest("There are multiple output values for the selected code fragment"); }
+  public void testErr1() throws Throwable { doTest(); }
  public void testExpr1() throws Throwable { doTest(); }
  public void testExpr2() throws Throwable { doTest(); }
  public void testExpr3() throws Throwable { doTest(); }
@@ -90,4 +90,10 @@ public class ExtractMethodTest extends LightGroovyTestCase {
  public void testClosureIt() throws Throwable { doTest(); }
  public void testImplicitReturn() {doTest();}

+  public void testMultiOutput1() {doTest();}
+  public void testMultiOutput2() {doTest();}
+  public void testMultiOutput3() {doTest();}
+  public void testMultiOutput4() {doTest();}
+  public void testMultiOutput5() {doTest();}
+
 }
@@ -12,4 +12,24 @@ www = kkk + 3
 println(yyy)
 println(www)
 -----
-FAILED: There are multiple output values for the selected code fragment
+iii = 6
+int kkk = 5
+def vv = 6
+def gg = 7
+
+
+int www, yyy
+(yyy, www) = testMethod(kkk, vv, gg)
+
+private List testMethod(int kkk, int vv, int gg) {
+  println(kkk + iii + (vv + gg))
+  yyy = kkk + 2
+  www = kkk + 3
+  return [yyy, www]
+}
+
+
+
+
+println(yyy)
+println(www)
@@ -0,0 +1,25 @@
+def a = 5
+
+<begin>print a
+def b = 6;
+a++
+print a+b<end>
+
+print a+b
+-----
+def a = 5
+
+int b
+(a, b) = testMethod(a)
+
+private List testMethod(int a) {
+  print a
+  def b = 6;
+  a++
+  print a + b
+  return [a, b]
+}
+
+
+
+print a+b
@@ -0,0 +1,25 @@
+def a = 5
+
+<begin>print a
+int b = 6;
+int c = 7
+print a+b<end>
+
+print a+b+c
+-----
+def a = 5
+
+int b, c
+(b, c) = testMethod(a)
+
+private List testMethod(int a) {
+  print a
+  int b = 6;
+  int c = 7
+  print a + b
+  return [b, c]
+}
+
+
+
+print a+b+c
@@ -0,0 +1,25 @@
+def a = 5
+
+<begin>print a
+def b = 6;
+def c = 7
+print a+b<end>
+
+print a+b+c
+-----
+def a = 5
+
+int b, c
+(b, c) = testMethod(a)
+
+private List testMethod(int a) {
+  print a
+  def b = 6;
+  def c = 7
+  print a + b
+  return [b, c]
+}
+
+
+
+print a+b+c
@@ -0,0 +1,20 @@
+def a = 5
+
+<begin>def b = 6
+print a+b<end>
+
+print a+b
+-----
+def a = 5
+
+int b = testMethod(a)
+
+private int testMethod(int a) {
+  def b = 6
+  print a + b
+  return b
+}
+
+
+
+print a+b
@@ -0,0 +1,21 @@
+def a = 5
+
+<begin>def b = 6
+print a+b
+a = 5<end>
+print a+b
+-----
+def a = 5
+
+int b
+(a, b) = testMethod(a)
+
+private List testMethod(int a) {
+  def b = 6
+  print a + b
+  a = 5
+  return [a, b]
+}
+
+
+print a+b
@@ -3,4 +3,4 @@ class MyClass {
 }

 print MyClass.metaClass.getMethods()
-print MyClass.field
+print MyClass.<warning descr="Access to 'field' exceeds its access rights">field</warning>