From eb2eba3b1bc538c4290f389bbbd563307d551c8e Mon Sep 17 00:00:00 2001 From: Eugene Auduchinok Date: Mon, 3 Aug 2015 17:22:57 +0300 Subject: [PATCH] In Introduce Variable Refactoring refactor, add more tests Refactor and fix --- .../goide/refactor/GoIntroduceOperation.java | 22 +++- .../refactor/GoIntroduceVariableBase.java | 109 ++++++++---------- .../refactor/GoIntroduceVariableHandler.java | 6 - .../caretOnCallParenthesis-after.go | 10 ++ .../caretOnCallParenthesis.go | 9 ++ .../multipleValueResult-after.go | 7 -- .../introduce-variable/replaceAll-after.go | 8 ++ .../refactor/introduce-variable/replaceAll.go | 7 ++ .../introduce-variable/replaceOnly-after.go | 8 ++ .../introduce-variable/replaceOnly.go | 7 ++ .../introduce-variable/topLevelExpression.go | 3 + .../introduce-variable/wrongSelection.go | 5 + .../refactor/GoIntroduceVariableTest.java | 44 +++++-- 13 files changed, 158 insertions(+), 87 deletions(-) create mode 100644 testData/refactor/introduce-variable/caretOnCallParenthesis-after.go create mode 100644 testData/refactor/introduce-variable/caretOnCallParenthesis.go delete mode 100644 testData/refactor/introduce-variable/multipleValueResult-after.go create mode 100644 testData/refactor/introduce-variable/replaceAll-after.go create mode 100644 testData/refactor/introduce-variable/replaceAll.go create mode 100644 testData/refactor/introduce-variable/replaceOnly-after.go create mode 100644 testData/refactor/introduce-variable/replaceOnly.go create mode 100644 testData/refactor/introduce-variable/topLevelExpression.go create mode 100644 testData/refactor/introduce-variable/wrongSelection.go diff --git a/src/com/goide/refactor/GoIntroduceOperation.java b/src/com/goide/refactor/GoIntroduceOperation.java index ed96bbfb32..4a81490d3e 100644 --- a/src/com/goide/refactor/GoIntroduceOperation.java +++ b/src/com/goide/refactor/GoIntroduceOperation.java @@ -22,6 +22,7 @@ import com.intellij.openapi.project.Project; import com.intellij.psi.PsiElement; import com.intellij.psi.PsiFile; +import org.jetbrains.annotations.NotNull; import java.util.LinkedHashSet; import java.util.List; @@ -32,17 +33,25 @@ public class GoIntroduceOperation { final private PsiFile myFile; private GoExpression myExpression; private List myOccurrences; + private PsiElement myContext; private LinkedHashSet mySuggestedNames; private String myName; private GoVarDefinition myVar; private boolean myReplaceAll; - public GoIntroduceOperation(Project project, Editor editor, PsiFile file) { + public GoIntroduceOperation(@NotNull Project project, @NotNull Editor editor, @NotNull PsiFile file) { myProject = project; myEditor = editor; myFile = file; } + public GoIntroduceOperation(@NotNull Project project, @NotNull Editor editor, @NotNull PsiFile file, boolean replaceAll) { + myProject = project; + myEditor = editor; + myFile = file; + myReplaceAll = replaceAll; + } + public Project getProject() { return myProject; } @@ -55,6 +64,7 @@ public PsiFile getFile() { return myFile; } + @NotNull public GoExpression getExpression() { return myExpression; } @@ -102,4 +112,14 @@ public boolean isReplaceAll() { public void setReplaceAll(boolean replaceAll) { myReplaceAll = replaceAll; } + + @NotNull + public PsiElement getContext() { + return myContext; + } + + @NotNull + public void setContext(PsiElement context) { + myContext = context; + } } diff --git a/src/com/goide/refactor/GoIntroduceVariableBase.java b/src/com/goide/refactor/GoIntroduceVariableBase.java index 6f43e67178..368e28983d 100644 --- a/src/com/goide/refactor/GoIntroduceVariableBase.java +++ b/src/com/goide/refactor/GoIntroduceVariableBase.java @@ -26,8 +26,12 @@ import com.intellij.openapi.editor.SelectionModel; import com.intellij.openapi.project.Project; import com.intellij.openapi.util.Pass; +import com.intellij.openapi.util.TextRange; import com.intellij.openapi.util.text.StringUtil; -import com.intellij.psi.*; +import com.intellij.psi.PsiDocumentManager; +import com.intellij.psi.PsiElement; +import com.intellij.psi.PsiFile; +import com.intellij.psi.PsiRecursiveElementVisitor; import com.intellij.psi.codeStyle.NameUtil; import com.intellij.psi.util.PsiTreeUtil; import com.intellij.refactoring.IntroduceTargetChooser; @@ -46,16 +50,21 @@ import java.util.List; public class GoIntroduceVariableBase { - protected void performAction(final GoIntroduceOperation operation) { + protected static void performAction(final GoIntroduceOperation operation) { SelectionModel selectionModel = operation.getEditor().getSelectionModel(); - GoExpression expression = - selectionModel.hasSelection() ? findExpressionInSelection(operation, selectionModel) : findExpressionAtOffset(operation); + boolean hasSelection = selectionModel.hasSelection(); + GoExpression expression = hasSelection ? findExpressionInSelection(operation, selectionModel) : findExpressionAtOffset(operation); if (expression instanceof GoParenthesesExpr) expression = ((GoParenthesesExpr)expression).getExpression(); if (expression == null) { - showCannotPerform(operation, "No expression found."); + String message = + RefactoringBundle.message(hasSelection ? "selected.block.should.represent.an.expression" : "refactoring.introduce.selection.error"); + showCannotPerform(operation, message); + return; + } + if (PsiTreeUtil.getParentOfType(expression, GoStatement.class) == null) { + showCannotPerform(operation, RefactoringBundle.message("refactoring.introduce.context.error")); return; } - List expressions = collectNestedExpressions(expression); if (expressions.isEmpty()) { int resultCount = GoInspectionUtil.getExpressionResultCount(expression); @@ -64,7 +73,8 @@ protected void performAction(final GoIntroduceOperation operation) { "Expression " + text + "()" + (resultCount == 0 ? " doesn't return a value." : " returns multiple values.")); return; } - if (expressions.size() == 1 || ApplicationManager.getApplication().isUnitTestMode()) { + expression = ContainerUtil.getFirstItem(expressions); + if (expressions.size() == 1 || hasSelection || ApplicationManager.getApplication().isUnitTestMode()) { operation.setExpression(expression); performOnElement(operation); } @@ -85,35 +95,29 @@ public String fun(GoExpression expression) { } @Nullable - protected static GoExpression findExpressionInSelection(GoIntroduceOperation operation, SelectionModel selectionModel) { - PsiFile file = operation.getFile(); - PsiElement element1 = file.findElementAt(selectionModel.getSelectionStart()); - PsiElement element2 = file.findElementAt(selectionModel.getSelectionEnd() - 1); - if (element1 instanceof PsiWhiteSpace) element1 = file.findElementAt(element1.getTextRange().getEndOffset()); - if (element2 instanceof PsiWhiteSpace) element2 = file.findElementAt(element2.getTextRange().getStartOffset() - 1); - if (element1 == null || element2 == null) return null; - - PsiElement parent = element1 == element2 ? element1 : PsiTreeUtil.findCommonParent(element1, element2); - return parent instanceof GoExpression ? (GoExpression)parent : PsiTreeUtil.getParentOfType(parent, GoExpression.class); + private static GoExpression findExpressionInSelection(GoIntroduceOperation operation, SelectionModel selectionModel) { + int start = selectionModel.getSelectionStart(); + int end = selectionModel.getSelectionEnd(); + return PsiTreeUtil.findElementOfClassAtRange(operation.getFile(), start, end, GoExpression.class); } @Nullable - protected static GoExpression findExpressionAtOffset(GoIntroduceOperation operation) { + private static GoExpression findExpressionAtOffset(GoIntroduceOperation operation) { PsiFile file = operation.getFile(); int offset = operation.getEditor().getCaretModel().getOffset(); - PsiElement element = file.findElementAt(offset); - GoParenthesesExpr parenthesesParent = PsiTreeUtil.getParentOfType(element, GoParenthesesExpr.class); - if (element instanceof PsiWhiteSpace || (parenthesesParent != null && parenthesesParent.getRparen() == element)) { - element = file.findElementAt(offset - 1); - } - return element instanceof GoExpression ? (GoExpression)element : PsiTreeUtil.getParentOfType(element, GoExpression.class); + GoExpression expr = PsiTreeUtil.getNonStrictParentOfType(file.findElementAt(offset), GoExpression.class); + GoExpression preExpr = PsiTreeUtil.getNonStrictParentOfType(file.findElementAt(offset - 1), GoExpression.class); + if (expr == null || preExpr != null && PsiTreeUtil.isAncestor(expr, preExpr, false)) return preExpr; + return expr; } + @NotNull private static List collectNestedExpressions(GoExpression expression) { List expressions = ContainerUtil.newArrayList(); while (expression != null) { - if (!(expression instanceof GoParenthesesExpr) && GoInspectionUtil.getExpressionResultCount(expression) == 1) { + if (!(expression instanceof GoParenthesesExpr) && GoInspectionUtil.getExpressionResultCount(expression) == 1 && + !(expression instanceof GoReferenceExpression && expression.getParent() instanceof GoCallExpr)) { expressions.add(expression); } expression = PsiTreeUtil.getParentOfType(expression, GoExpression.class); @@ -124,20 +128,15 @@ private static List collectNestedExpressions(GoExpression expressi protected static void performOnElement(final GoIntroduceOperation operation) { final GoExpression expression = operation.getExpression(); PsiElement statement = PsiTreeUtil.getParentOfType(expression, GoStatement.class); - if (statement == null) { - showCannotPerform(operation, RefactoringBundle.message("refactoring.introduce.context.error")); - return; - } + LinkedHashSet suggestedNames = getSuggestedNames(expression); operation.setSuggestedNames(suggestedNames); - operation.setOccurrences(getOccurrences(expression, PsiTreeUtil.getParentOfType(statement, GoBlock.class))); + operation.setOccurrences(getOccurrences(expression, PsiTreeUtil.getTopmostParentOfType(statement, GoBlock.class))); final Editor editor = operation.getEditor(); if (editor.getSettings().isVariableInplaceRenameEnabled()) { operation.setName(ContainerUtil.getFirstItem(suggestedNames)); if (ApplicationManager.getApplication().isUnitTestMode()) { - // todo rewrite for testing different replaceAll cases - operation.setReplaceAll(true); performInplaceIntroduce(operation); return; } @@ -151,13 +150,13 @@ public void pass(OccurrencesChooser.ReplaceChoice choice) { }); } //else { - // // todo show dialog here; set name, replaceAll + // // todo show dialog here; set name // performReplace(operation); //} } @NotNull - public static List getOccurrences(@NotNull final PsiElement pattern, @Nullable PsiElement context) { + private static List getOccurrences(@NotNull final PsiElement pattern, @Nullable PsiElement context) { if (context == null) return Collections.emptyList(); final List occurrences = ContainerUtil.newArrayList(); PsiRecursiveElementVisitor visitor = new PsiRecursiveElementVisitor() { @@ -176,29 +175,30 @@ public void visitElement(@NotNull PsiElement element) { private static void performInplaceIntroduce(GoIntroduceOperation operation) { performReplace(operation); new GoInplaceVariableIntroducer(operation).performInplaceRefactoring(operation.getSuggestedNames()); - // todo doesn't validate after rename } protected static void performReplace(final GoIntroduceOperation operation) { final Project project = operation.getProject(); final PsiElement expression = operation.getExpression(); - final String name = operation.getName(); - final boolean replaceAll = operation.isReplaceAll(); - final List occurrences = replaceAll ? operation.getOccurrences() : ContainerUtil.newArrayList(expression); - final PsiElement anchor = findAnchor(occurrences); + final List occurrences = operation.isReplaceAll() ? operation.getOccurrences() : Collections.singletonList(expression); + final GoBlock context = PsiTreeUtil.getNonStrictParentOfType(PsiTreeUtil.findCommonParent(occurrences), GoBlock.class); + if (context == null) { + showCannotPerform(operation, RefactoringBundle.message("refactoring.introduce.context.error")); + return; + } + final PsiElement anchor = findAnchor(occurrences, context); if (anchor == null) { showCannotPerform(operation, RefactoringBundle.message("refactoring.introduce.context.error")); return; } + final String name = operation.getName(); final List newOccurrences = ContainerUtil.newArrayList(); - WriteCommandAction.runWriteCommandAction(project, new Runnable() { @Override public void run() { - PsiElement introducePlace = anchor.getParent(); GoStatement declarationStatement = GoElementFactory.createShortVarDeclarationStatement(project, name, (GoExpression)expression); PsiElement newLine = GoElementFactory.createNewLine(project); - PsiElement statement = introducePlace.addBefore(declarationStatement, introducePlace.addBefore(newLine, anchor)); + PsiElement statement = context.addBefore(declarationStatement, context.addBefore(newLine, anchor)); operation.setVar(PsiTreeUtil.findChildOfType(statement, GoVarDefinition.class)); for (PsiElement occurrence : occurrences) { @@ -212,20 +212,12 @@ public void run() { PsiDocumentManager.getInstance(project).doPostponedOperationsAndUnblockDocument(operation.getEditor().getDocument()); } - protected static boolean isCommonAncestor(@NotNull PsiElement ancestor, @NotNull List occurrences) { - for (PsiElement element : occurrences) { - if (!PsiTreeUtil.isAncestor(ancestor, element, false)) return false; + private static PsiElement findAnchor(List occurrences, PsiElement context) { + PsiElement statement = PsiTreeUtil.getParentOfType(ContainerUtil.getFirstItem(occurrences), GoStatement.class); + while (statement != null && statement.getParent() != context) { + statement = statement.getParent(); } - return true; - } - - private static PsiElement findAnchor(@NotNull List occurrences) { - PsiElement statement = ContainerUtil.getFirstItem(occurrences); - while (statement != null) { - statement = PsiTreeUtil.getParentOfType(statement, GoStatement.class); - if (statement != null && isCommonAncestor(statement.getParent(), occurrences)) return statement; - } - return null; + return statement; } private static LinkedHashSet getNamesInContext(PsiElement context) { @@ -235,7 +227,6 @@ private static LinkedHashSet getNamesInContext(PsiElement context) { for (GoNamedElement namedElement : PsiTreeUtil.findChildrenOfType(context, GoNamedElement.class)) { names.add(namedElement.getName()); } - names.addAll(((GoFile)context.getContainingFile()).getImportMap().keySet()); GoFunctionDeclaration functionDeclaration = PsiTreeUtil.getParentOfType(context, GoFunctionDeclaration.class); @@ -285,11 +276,5 @@ public GoInplaceVariableIntroducer(GoIntroduceOperation operation) { super(operation.getVar(), operation.getEditor(), operation.getProject(), "Introduce Variable", ArrayUtil.toObjectArray(operation.getOccurrences(), PsiElement.class), null); } - - @Nullable - @Override - protected PsiElement checkLocalScope() { - return myElementToRename.getContainingFile(); - } } } diff --git a/src/com/goide/refactor/GoIntroduceVariableHandler.java b/src/com/goide/refactor/GoIntroduceVariableHandler.java index 81c594bd82..f2d941707a 100644 --- a/src/com/goide/refactor/GoIntroduceVariableHandler.java +++ b/src/com/goide/refactor/GoIntroduceVariableHandler.java @@ -16,8 +16,6 @@ package com.goide.refactor; -import com.intellij.codeInsight.template.impl.TemplateManagerImpl; -import com.intellij.codeInsight.template.impl.TemplateState; import com.intellij.openapi.actionSystem.DataContext; import com.intellij.openapi.editor.Editor; import com.intellij.openapi.project.Project; @@ -31,10 +29,6 @@ public class GoIntroduceVariableHandler extends GoIntroduceVariableBase implemen @Override public void invoke(@NotNull final Project project, final Editor editor, PsiFile file, DataContext dataContext) { if (!CommonRefactoringUtil.checkReadOnlyStatus(file)) return; - if (editor.getSettings().isVariableInplaceRenameEnabled()) { - final TemplateState templateState = TemplateManagerImpl.getTemplateState(editor); - if (templateState != null && !templateState.isFinished()) return; - } performAction(new GoIntroduceOperation(project, editor, file)); } diff --git a/testData/refactor/introduce-variable/caretOnCallParenthesis-after.go b/testData/refactor/introduce-variable/caretOnCallParenthesis-after.go new file mode 100644 index 0000000000..84baf0882c --- /dev/null +++ b/testData/refactor/introduce-variable/caretOnCallParenthesis-after.go @@ -0,0 +1,10 @@ +package a + +func a() { + i := 1 + var a = b(i) +} + +func b(c int) int { + return 1 +} \ No newline at end of file diff --git a/testData/refactor/introduce-variable/caretOnCallParenthesis.go b/testData/refactor/introduce-variable/caretOnCallParenthesis.go new file mode 100644 index 0000000000..e3b41b407a --- /dev/null +++ b/testData/refactor/introduce-variable/caretOnCallParenthesis.go @@ -0,0 +1,9 @@ +package a + +func a() { + var a = b(1) +} + +func b(c int) int { + return 1 +} \ No newline at end of file diff --git a/testData/refactor/introduce-variable/multipleValueResult-after.go b/testData/refactor/introduce-variable/multipleValueResult-after.go deleted file mode 100644 index 5151565846..0000000000 --- a/testData/refactor/introduce-variable/multipleValueResult-after.go +++ /dev/null @@ -1,7 +0,0 @@ -package a - -import "fmt" - -func a() { - a := fmt.Println() -} \ No newline at end of file diff --git a/testData/refactor/introduce-variable/replaceAll-after.go b/testData/refactor/introduce-variable/replaceAll-after.go new file mode 100644 index 0000000000..d8f615aa86 --- /dev/null +++ b/testData/refactor/introduce-variable/replaceAll-after.go @@ -0,0 +1,8 @@ +package a + +func a() { + i := 1 + 2 + var b = i + var c = 123 + var d = i +} \ No newline at end of file diff --git a/testData/refactor/introduce-variable/replaceAll.go b/testData/refactor/introduce-variable/replaceAll.go new file mode 100644 index 0000000000..9b78824380 --- /dev/null +++ b/testData/refactor/introduce-variable/replaceAll.go @@ -0,0 +1,7 @@ +package a + +func a() { + var b = 1 + 2 + var c = 123 + var d = 1 + 2 +} \ No newline at end of file diff --git a/testData/refactor/introduce-variable/replaceOnly-after.go b/testData/refactor/introduce-variable/replaceOnly-after.go new file mode 100644 index 0000000000..7da036e93e --- /dev/null +++ b/testData/refactor/introduce-variable/replaceOnly-after.go @@ -0,0 +1,8 @@ +package a + +func a() { + var b = 1 + 2 + var c = 123 + i := 1 + 2 + var d = i +} \ No newline at end of file diff --git a/testData/refactor/introduce-variable/replaceOnly.go b/testData/refactor/introduce-variable/replaceOnly.go new file mode 100644 index 0000000000..9b78824380 --- /dev/null +++ b/testData/refactor/introduce-variable/replaceOnly.go @@ -0,0 +1,7 @@ +package a + +func a() { + var b = 1 + 2 + var c = 123 + var d = 1 + 2 +} \ No newline at end of file diff --git a/testData/refactor/introduce-variable/topLevelExpression.go b/testData/refactor/introduce-variable/topLevelExpression.go new file mode 100644 index 0000000000..ec914a9356 --- /dev/null +++ b/testData/refactor/introduce-variable/topLevelExpression.go @@ -0,0 +1,3 @@ +package a + +var a = 1 + 2 \ No newline at end of file diff --git a/testData/refactor/introduce-variable/wrongSelection.go b/testData/refactor/introduce-variable/wrongSelection.go new file mode 100644 index 0000000000..b0976b47e4 --- /dev/null +++ b/testData/refactor/introduce-variable/wrongSelection.go @@ -0,0 +1,5 @@ +package a + +func a() { + var a = 1 + 2 +} \ No newline at end of file diff --git a/tests/com/goide/refactor/GoIntroduceVariableTest.java b/tests/com/goide/refactor/GoIntroduceVariableTest.java index 081fcb2e09..3682cf5c06 100644 --- a/tests/com/goide/refactor/GoIntroduceVariableTest.java +++ b/tests/com/goide/refactor/GoIntroduceVariableTest.java @@ -16,35 +16,57 @@ package com.goide.refactor; +import com.intellij.openapi.editor.Editor; +import com.intellij.openapi.project.Project; +import com.intellij.psi.PsiFile; +import com.intellij.refactoring.RefactoringBundle; import com.intellij.testFramework.fixtures.LightPlatformCodeInsightFixtureTestCase; +import org.jetbrains.annotations.NotNull; public class GoIntroduceVariableTest extends LightPlatformCodeInsightFixtureTestCase { + private static class IntroduceTest extends GoIntroduceVariableBase { + static void invoke(@NotNull Project project, @NotNull Editor editor, @NotNull PsiFile file, boolean replaceAll) { + performAction(new GoIntroduceOperation(project, editor, file, replaceAll)); + } + } + @Override protected String getTestDataPath() { return "testData/refactor/introduce-variable"; } - protected void doTest() { + private void doTest() { + doTest(true); + } + + private void doTest(boolean replaceAll) { String testName = getTestName(true); myFixture.configureByFile(testName + ".go"); - new GoIntroduceVariableHandler().invoke(myFixture.getProject(), myFixture.getEditor(), myFixture.getFile(), null); + IntroduceTest.invoke(myFixture.getProject(), myFixture.getEditor(), myFixture.getFile(), replaceAll); myFixture.checkResultByFile(testName + "-after.go"); } + private void doFailureTest(String msg) { + try { + doTest(); + } + catch (RuntimeException e) { + assertEquals("Cannot perform refactoring.\n" + msg, e.getMessage()); + } + } + public void testCaretAfterRightParenthesis() { doTest(); } public void testCaretOnRightParenthesis() { doTest(); } + public void testCaretOnCallParenthesis() { doTest(); } public void testNameSuggestOnGetterFunction() { doTest(); } public void testNameSuggestOnDefinedImportAlias() { doTest(); } public void testNameSuggestOnDefaultName() { doTest(); } public void testNameSuggestOnParamName() { doTest(); } - public void testMultipleValueResult() { - try { - doTest(); - } - catch (RuntimeException e) { - assertEquals("Cannot perform refactoring.\n" + - "Expression fmt.Println() returns multiple values.", e.getMessage()); - } - } + public void testMultipleValueResult() { doFailureTest("Expression fmt.Println() returns multiple values."); } + public void testWrongSelection() { doFailureTest(RefactoringBundle.message("selected.block.should.represent.an.expression")); } + public void testTopLevelExpression() { doFailureTest(RefactoringBundle.message("refactoring.introduce.context.error"));} + + public void testReplaceAll() { doTest(true); } + public void testReplaceOnly() { doTest(false);} }