IDEA-80689 Groovy: Extract Closure Parameter applied to a command expression results with invalid code

plugins/groovy/src/org/jetbrains/plugins/groovy/lang/psi/impl/ApplicationStatementUtil.java

@@ -0,0 +1,104 @@
+/*
+ * Copyright 2000-2012 JetBrains s.r.o.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.jetbrains.plugins.groovy.lang.psi.impl;
+
+import com.intellij.openapi.diagnostic.Logger;
+import com.intellij.openapi.project.Project;
+import com.intellij.psi.PsiElement;
+import org.jetbrains.plugins.groovy.lang.psi.GroovyPsiElementFactory;
+import org.jetbrains.plugins.groovy.lang.psi.api.statements.arguments.GrArgumentList;
+import org.jetbrains.plugins.groovy.lang.psi.api.statements.expressions.GrApplicationStatement;
+import org.jetbrains.plugins.groovy.lang.psi.api.statements.expressions.GrCommandArgumentList;
+import org.jetbrains.plugins.groovy.lang.psi.api.statements.expressions.GrExpression;
+import org.jetbrains.plugins.groovy.lang.psi.api.statements.expressions.GrReferenceExpression;
+import org.jetbrains.plugins.groovy.lang.psi.api.statements.expressions.path.GrMethodCallExpression;
+
+/**
+ * @author Max Medvedev
+ */
+public class ApplicationStatementUtil {
+  private static final Logger LOG = Logger.getInstance(ApplicationStatementUtil.class);
+
+  public static GrExpression convertToMethodCallExpression(GrExpression expr) {
+    final Project project = expr.getProject();
+    final GroovyPsiElementFactory factory = GroovyPsiElementFactory.getInstance(project);
+
+    boolean copied = false;
+    if (expr instanceof GrApplicationStatement) {
+      expr = convertAppInternal(factory, (GrApplicationStatement)expr);
+      copied = true;
+    }
+
+    if (expr instanceof GrReferenceExpression &&
+        ((GrReferenceExpression)expr).getDotToken() == null &&
+        ((GrReferenceExpression)expr).getQualifier() != null) {
+      expr = convertRefInternal(factory, ((GrReferenceExpression)expr));
+      copied = true;
+    }
+
+
+    if (!shouldManage(expr)) return expr;
+    if (!copied) expr = (GrExpression)expr.copy();
+
+    for (PsiElement child = expr.getFirstChild(); child != null; child = child.getFirstChild()) {
+      if (child instanceof GrApplicationStatement) {
+        child = child.replace(convertAppInternal(factory, (GrApplicationStatement)child));
+      }
+      else if (child instanceof GrReferenceExpression &&
+               ((GrReferenceExpression)child).getDotToken() == null &&
+               ((GrReferenceExpression)child).getQualifier() != null) {
+        child = child.replace(convertRefInternal(factory, ((GrReferenceExpression)child)));
+      }
+    }
+
+    return expr;
+  }
+
+  private static boolean shouldManage(GrExpression expr) {
+    for (PsiElement child = expr.getFirstChild(); child != null; child = child.getFirstChild()) {
+      if (child instanceof GrApplicationStatement) {
+        return true;
+      }
+      else if (child instanceof GrReferenceExpression &&
+               ((GrReferenceExpression)child).getDotToken() == null &&
+               ((GrReferenceExpression)child).getQualifier() != null) {
+        return true;
+      }
+    }
+    return false;
+  }
+
+  private static GrReferenceExpression convertRefInternal(GroovyPsiElementFactory factory, GrReferenceExpression ref) {
+    ref.addAfter(factory.createDotToken("."), ref.getQualifier());
+    return ref;
+  }
+
+  private static GrMethodCallExpression convertAppInternal(GroovyPsiElementFactory factory, GrApplicationStatement app) {
+    final GrCommandArgumentList list = app.getArgumentList();
+
+    final GrMethodCallExpression prototype = (GrMethodCallExpression)factory.createExpressionFromText("foo()");
+    prototype.getInvokedExpression().replace(app.getInvokedExpression());
+    final GrArgumentList pList = prototype.getArgumentList();
+    LOG.assertTrue(pList != null);
+
+    final PsiElement anchor = pList.getRightParen();
+    for (PsiElement ch = list.getFirstChild(); ch != null; ch = ch.getNextSibling()) {
+      pList.addBefore(ch, anchor);
+    }
+
+    return prototype;
+  }
+}

plugins/groovy/src/org/jetbrains/plugins/groovy/lang/psi/impl/PsiImplUtil.java

@@ -110,16 +110,15 @@ public class PsiImplUtil {
 
   private static boolean isAfterIdentifier(PsiElement el) {
     final PsiElement prev = GeeseUtil.getPreviousNonWhitespaceToken(el);
-    return prev != null && prev.getNode().getElementType() == GroovyTokenTypes.mIDENT;
+    return prev != null && prev.getNode().getElementType() == mIDENT;
   }
 
   public static GrExpression replaceExpression(GrExpression oldExpr, GrExpression newExpr, boolean removeUnnecessaryParentheses) {
     PsiElement oldParent = oldExpr.getParent();
     if (oldParent == null) throw new PsiInvalidElementAccessException(oldExpr);
 
-    if (newExpr instanceof GrApplicationStatement && !(oldExpr instanceof GrApplicationStatement)) {
-      GroovyPsiElementFactory factory = GroovyPsiElementFactory.getInstance(oldExpr.getProject());
-      newExpr = factory.createMethodCallByAppCall(((GrApplicationStatement)newExpr));
+    if (!(oldExpr instanceof GrApplicationStatement)) {
+      newExpr = ApplicationStatementUtil.convertToMethodCallExpression(newExpr);
     }
 
     // Remove unnecessary parentheses

plugins/groovy/src/org/jetbrains/plugins/groovy/refactoring/extract/ExtractUtil.java

@@ -30,13 +30,16 @@ import org.jetbrains.plugins.groovy.lang.psi.GroovyRecursiveElementVisitor;
 import org.jetbrains.plugins.groovy.lang.psi.api.statements.GrStatement;
 import org.jetbrains.plugins.groovy.lang.psi.api.statements.GrVariable;
 import org.jetbrains.plugins.groovy.lang.psi.api.statements.GrVariableDeclaration;
-import org.jetbrains.plugins.groovy.lang.psi.api.statements.expressions.*;
+import org.jetbrains.plugins.groovy.lang.psi.api.statements.expressions.GrAssignmentExpression;
+import org.jetbrains.plugins.groovy.lang.psi.api.statements.expressions.GrExpression;
+import org.jetbrains.plugins.groovy.lang.psi.api.statements.expressions.GrReferenceExpression;
 import org.jetbrains.plugins.groovy.lang.psi.api.statements.expressions.path.GrMethodCallExpression;
 import org.jetbrains.plugins.groovy.lang.psi.api.statements.params.GrParameter;
 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.ApplicationStatementUtil;
 import org.jetbrains.plugins.groovy.lang.psi.impl.PsiImplUtil;
 import org.jetbrains.plugins.groovy.lang.psi.impl.statements.expressions.TypesUtil;
 import org.jetbrains.plugins.groovy.lang.psi.util.PsiUtil;
@@ -311,21 +314,8 @@ public class ExtractUtil {
       boolean addReturn = !isVoid && expr != null && expr.getType() != null && expr.getType() != PsiType.VOID;
       if (addReturn) {
         buffer.append("return ");
-        if (expr instanceof GrApplicationStatement) {
-          final GrApplicationStatement appStatement = (GrApplicationStatement)expr;
-
-          buffer.append(appStatement.getInvokedExpression().getText());
-          buffer.append('(');
-
-          final GrCommandArgumentList argList = appStatement.getArgumentList();
-          if (argList != null) {
-            buffer.append(argList.getText());
-          }
-          buffer.append(')');
-        }
-        else {
-          buffer.append(expr.getText());
-        }
+        expr = ApplicationStatementUtil.convertToMethodCallExpression(expr);
+        buffer.append(expr.getText());
       }
       else {
         buffer.append(expr != null ? expr.getText() : "");

plugins/groovy/src/org/jetbrains/plugins/groovy/refactoring/extract/GroovyExtractChooser.java

@@ -113,7 +113,9 @@ public class GroovyExtractChooser {
       throw new GrRefactoringError(GroovyRefactoringBundle.message("selected.block.should.represent.an.expression"));
     }
 
-    if (ExtractUtil.isSingleExpression(statements) && statement0.getParent() instanceof GrAssignmentExpression && ((GrAssignmentExpression)statement0.getParent()).getLValue()==statement0) {
+    if (ExtractUtil.isSingleExpression(statements) &&
+        statement0.getParent() instanceof GrAssignmentExpression &&
+        ((GrAssignmentExpression)statement0.getParent()).getLValue() == statement0) {
       throw new GrRefactoringError(GroovyRefactoringBundle.message("selected.expression.should.not.be.lvalue"));
     }
 

plugins/groovy/src/org/jetbrains/plugins/groovy/refactoring/introduce/GrIntroduceHandlerBase.java

@@ -107,9 +107,7 @@ public abstract class GrIntroduceHandlerBase<Settings extends GrIntroduceSetting
       final PsiElement resolved = resolveResult.getElement();
       return resolved instanceof PsiMethod && !resolveResult.isInvokedOnProperty() || resolved instanceof PsiClass;
     }
-    if (expression instanceof GrApplicationStatement) {
-      return !PsiUtil.isExpressionStatement(expression);
-    }
+
     if (expression instanceof GrClosableBlock && expression.getParent() instanceof GrStringInjection) return true;
 
     return false;

plugins/groovy/src/org/jetbrains/plugins/groovy/refactoring/introduce/parameter/GrIntroduceParameterDialog.java

@@ -15,6 +15,8 @@
  */
 package org.jetbrains.plugins.groovy.refactoring.introduce.parameter;
 
+import com.intellij.openapi.application.AccessToken;
+import com.intellij.openapi.application.WriteAction;
 import com.intellij.openapi.ui.Splitter;
 import com.intellij.openapi.ui.VerticalFlowLayout;
 import com.intellij.openapi.util.Ref;
@@ -44,7 +46,6 @@ import org.jetbrains.plugins.groovy.lang.psi.api.statements.GrVariable;
 import org.jetbrains.plugins.groovy.lang.psi.api.statements.expressions.GrExpression;
 import org.jetbrains.plugins.groovy.lang.psi.api.statements.params.GrParameter;
 import org.jetbrains.plugins.groovy.lang.psi.api.statements.typedef.members.GrMethod;
-import org.jetbrains.plugins.groovy.lang.psi.util.GroovyCommonClassNames;
 import org.jetbrains.plugins.groovy.refactoring.GrRefactoringError;
 import org.jetbrains.plugins.groovy.refactoring.GroovyNameSuggestionUtil;
 import org.jetbrains.plugins.groovy.refactoring.GroovyRefactoringBundle;
@@ -281,7 +282,14 @@ public class GrIntroduceParameterDialog extends RefactoringDialog implements GrI
     }
 
     final ExtractClosureHelperImpl mockHelper = new ExtractClosureHelperImpl(myInfo, "__test___n_", false, new TIntArrayList(), false, 0);
-    final PsiType returnType = ExtractClosureProcessorBase.generateClosure(mockHelper).getReturnType();
+    final PsiType returnType;
+    final AccessToken token = WriteAction.start();
+    try {
+      returnType = ExtractClosureProcessorBase.generateClosure(mockHelper).getReturnType();
+    }
+    finally {
+      token.finish();
+    }
 
     box.addClosureTypesFrom(returnType, mockHelper.getContext());
     if (expr == null && var == null) {
@@ -401,12 +409,11 @@ public class GrIntroduceParameterDialog extends RefactoringDialog implements GrI
   protected void doAction() {
     saveSettings();
     final GrParametersOwner toReplaceIn = myInfo.getToReplaceIn();
-    final PsiType selectedType = myTypeComboBox.getSelectedType();
 
     final GrExpression expr = findExpr();
     final GrVariable var = findVar();
 
-    if ((expr == null && var == null) || selectedType != null && selectedType.equalsToText(GroovyCommonClassNames.GROOVY_LANG_CLOSURE)) {
+    if (myTypeComboBox.isClosureSelected()) {
       GrIntroduceParameterSettings settings = new ExtractClosureHelperImpl(myInfo,
                                                                            myNameSuggestionsField.getEnteredName(),
                                                                            myDeclareFinalCheckBox.isSelected(),

plugins/groovy/src/org/jetbrains/plugins/groovy/refactoring/introduce/parameter/java2groovy/GroovyIntroduceParameterMethodUsagesProcessor.java

@@ -59,8 +59,7 @@ import org.jetbrains.plugins.groovy.refactoring.GroovyRefactoringUtil;
  *         Date: Apr 18, 2009 3:16:24 PM
  */
 public class GroovyIntroduceParameterMethodUsagesProcessor implements IntroduceParameterMethodUsagesProcessor {
-  private static final Logger LOG = Logger
-    .getInstance("#org.jetbrains.plugins.groovy.refactoring.introduce.parameter.java2groovy.GroovyIntroduceParameterMethodUsagesProcessor");
+  private static final Logger LOG = Logger.getInstance(GroovyIntroduceParameterMethodUsagesProcessor.class);
 
   private static boolean isGroovyUsage(UsageInfo usage) {
     final PsiElement el = usage.getElement();
@@ -115,7 +114,8 @@ public class GroovyIntroduceParameterMethodUsagesProcessor implements IntroduceP
 
       GrExpression newArg = addClosureToCall(initializer, argList);
       if (newArg == null) {
-        newArg = (GrExpression)argList.addAfter(initializer, anchor);
+        final PsiElement dummy = argList.addAfter(factory.createExpressionFromText("1"), anchor);
+        newArg = ((GrExpression)dummy).replaceWithExpression((GrExpression)initializer, true);
       }
       final PsiMethod methodToReplaceIn = data.getMethodToReplaceIn();
       new OldReferencesResolver(callExpression, newArg, methodToReplaceIn, data.getReplaceFieldsWithGetters(), initializer,

plugins/groovy/test/org/jetbrains/plugins/groovy/refactoring/extract/closure/ExtractClosureTest.groovy

@@ -253,6 +253,20 @@ class Some {
          }
      }
 }
+''')
+  }
+
+  void testAppStatement() {
+    doTest('''
+void foo() {
+    def s = <selection><caret>"zxcvbn".substring 2 charAt(1)</selection>
+}
+foo()
+''',  '''
+void foo(Closure<Character> closure) {
+    def s = <selection><caret>closure()</selection>
+}
+foo {return "zxcvbn".substring(2).charAt(1)}
 ''')
   }
 }

plugins/groovy/test/org/jetbrains/plugins/groovy/refactoring/extract/method/ExtractMethodTest.groovy

@@ -16,14 +16,13 @@
 
 package org.jetbrains.plugins.groovy.refactoring.extract.method;
 
-import com.intellij.openapi.util.text.StringUtil;
-import com.intellij.psi.impl.source.PostprocessReformattingAspect;
-import com.intellij.refactoring.util.CommonRefactoringUtil;
-import org.jetbrains.plugins.groovy.GroovyFileType;
-import org.jetbrains.plugins.groovy.LightGroovyTestCase;
-import org.jetbrains.plugins.groovy.util.TestUtils;
 
-import java.util.List;
+import com.intellij.openapi.util.text.StringUtil
+import com.intellij.psi.impl.source.PostprocessReformattingAspect
+import com.intellij.refactoring.util.CommonRefactoringUtil
+import org.jetbrains.plugins.groovy.GroovyFileType
+import org.jetbrains.plugins.groovy.LightGroovyTestCase
+import org.jetbrains.plugins.groovy.util.TestUtils
 
 /**
  * @author ilyas
@@ -31,29 +30,29 @@ import java.util.List;
 public class ExtractMethodTest extends LightGroovyTestCase {
   @Override
   protected String getBasePath() {
-    return TestUtils.getTestDataPath() + "groovy/refactoring/extractMethod/";
+    return TestUtils.testDataPath + "groovy/refactoring/extractMethod/";
   }
 
-  private void doAntiTest(String errorMessage) throws Exception {
+  private void doAntiTest(String errorMessage) {
     GroovyExtractMethodHandler handler = configureFromText(readInput().get(0));
     try {
-      handler.invoke(getProject(), myFixture.getEditor(), myFixture.getFile(), null);
+      handler.invoke(project, myFixture.editor, myFixture.file, null);
       assertTrue(false);
     }
     catch (CommonRefactoringUtil.RefactoringErrorHintException e) {
-      assertEquals(errorMessage, e.getLocalizedMessage());
+      assertEquals(errorMessage, e.localizedMessage);
     }
   }
 
   private List<String> readInput() {
-    return TestUtils.readInput(getTestDataPath() + getTestName(true) + ".test");
+    return TestUtils.readInput(testDataPath + getTestName(true) + ".test");
   }
 
   private void doTest() {
     final List<String> data = readInput();
     GroovyExtractMethodHandler handler = configureFromText(data.get(0));
-    handler.invoke(getProject(), myFixture.getEditor(), myFixture.getFile(), null);
-    PostprocessReformattingAspect.getInstance(getProject()).doPostponedFormatting();
+    handler.invoke(project, myFixture.editor, myFixture.file, null);
+    PostprocessReformattingAspect.getInstance(project).doPostponedFormatting();
     myFixture.checkResult(StringUtil.trimEnd(data.get(1), "\n"));
   }
 
@@ -64,7 +63,7 @@ public class ExtractMethodTest extends LightGroovyTestCase {
     fileText = TestUtils.removeEndMarker(fileText);
     myFixture.configureByText(GroovyFileType.GROOVY_FILE_TYPE, fileText);
 
-    myFixture.getEditor().getSelectionModel().setSelection(startOffset, endOffset);
+    myFixture.editor.selectionModel.setSelection(startOffset, endOffset);
     return new GroovyExtractMethodHandler();
   }
 
@@ -114,4 +113,5 @@ public class ExtractMethodTest extends LightGroovyTestCase {
   public void testWildCardReturnType() {doTest();}
   public void testParamChangedInsideExtractedMethod() {doTest();}
 
+  public void testTerribleAppStatement() {doTest()}
 }

plugins/groovy/test/org/jetbrains/plugins/groovy/refactoring/introduceParameter/GrIntroduceParameterTest.java

@@ -295,4 +295,5 @@ public class GrIntroduceParameterTest extends LightCodeInsightFixtureTestCase {
   public void testClosureArgWithEmptyArgList() {doTest(IntroduceParameterRefactoring.REPLACE_FIELDS_WITH_GETTERS_NONE, true, false);}
 
   public void testScriptMethod() {doTest(IntroduceParameterRefactoring.REPLACE_FIELDS_WITH_GETTERS_NONE, true, false);}
+  public void testAppStatement() {doTest(IntroduceParameterRefactoring.REPLACE_FIELDS_WITH_GETTERS_NONE, false, false);}
 }

plugins/groovy/testdata/groovy/refactoring/extractMethod/terribleAppStatement.test

@@ -0,0 +1,11 @@
+def foo() {
+    def s = <begin>"zxcvbn".substring 2 charAt(1)<end>
+}
+-----
+def foo() {
+    def s = testMethod()
+}
+
+private char testMethod() {
+    return "zxcvbn".substring(2).charAt(1)
+}

plugins/groovy/testdata/refactoring/introduceParameterGroovy/appStatement/AppStatementAfter.groovy

@@ -0,0 +1 @@
+new A().foo("zxcvbn".substring(2).charAt(1))

plugins/groovy/testdata/refactoring/introduceParameterGroovy/appStatement/AppStatementBefore.groovy

@@ -0,0 +1 @@
+new A().foo()

plugins/groovy/testdata/refactoring/introduceParameterGroovy/appStatement/AppStatementMyClass.groovy

@@ -0,0 +1,5 @@
+class A {
+  void foo() {
+    def s = <selection>"zxcvbn".substring(2).charAt(1)</selection>
+  }
+}

plugins/groovy/testdata/refactoring/introduceParameterGroovy/incorrectArgumentList/IncorrectArgumentListAfter.groovy

@@ -1 +1 @@
-new A().foo(27 + 4.5)
+new A().foo(27+ 4.5)