From cd45e79ab272055de8165914bfc07f481268f1a2 Mon Sep 17 00:00:00 2001 From: Mikhail Golubev Date: Fri, 21 Oct 2016 20:39:54 +0300 Subject: [PATCH] PY-17265 Placement of inserted function is consistent with Move refactoring Namely, if a function is being moved to another file it will be inserted at its end unless there is a top-level usage of it. In this case the generated function will be inserted right before the first such usage so as not to produce unresolved references. If function stays in the same file, it will be inserted after its original parent statement i.e. another function or a class, again if there is no conflicting usages. --- .../codeInsight/imports/AddImportHelper.java | 9 +++++ .../move/PyMoveRefactoringUtil.java | 38 +++++++++++++++++++ .../PyBaseMakeFunctionTopLevelProcessor.java | 29 +++++++------- .../moduleMembers/PyMoveSymbolProcessor.java | 30 ++------------- .../after/main.py | 2 + .../after/other.py | 14 +++++++ .../before/main.py | 3 ++ .../before/other.py | 10 +++++ ...nSameFileClassAndUsageNotTopLevel.after.py | 10 +++++ ...ositionSameFileClassAndUsageNotTopLevel.py | 7 ++++ .../after/main.py | 2 + .../after/other.py | 20 ++++++++++ .../before/main.py | 3 ++ .../before/other.py | 15 ++++++++ .../PyMakeFunctionTopLevelTest.java | 16 ++++++-- 15 files changed, 161 insertions(+), 47 deletions(-) create mode 100644 python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionNoUsageInAnotherFile/after/main.py create mode 100644 python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionNoUsageInAnotherFile/after/other.py create mode 100644 python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionNoUsageInAnotherFile/before/main.py create mode 100644 python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionNoUsageInAnotherFile/before/other.py create mode 100644 python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionSameFileClassAndUsageNotTopLevel.after.py create mode 100644 python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionSameFileClassAndUsageNotTopLevel.py create mode 100644 python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionUsageInAnotherFile/after/main.py create mode 100644 python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionUsageInAnotherFile/after/other.py create mode 100644 python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionUsageInAnotherFile/before/main.py create mode 100644 python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionUsageInAnotherFile/before/other.py diff --git a/python/src/com/jetbrains/python/codeInsight/imports/AddImportHelper.java b/python/src/com/jetbrains/python/codeInsight/imports/AddImportHelper.java index 97c037de48fa..45b9d40e8825 100644 --- a/python/src/com/jetbrains/python/codeInsight/imports/AddImportHelper.java +++ b/python/src/com/jetbrains/python/codeInsight/imports/AddImportHelper.java @@ -151,6 +151,15 @@ public class AddImportHelper { return PsiTreeUtil.getParentOfType(anchor, PyStatement.class, false); } + /** + * Returns position in the file after all leading comments, docstring and import statements. + *

+ * Returned PSI element is intended to be used as "anchor" parameter for {@link PsiElement#addBefore(PsiElement, PsiElement)}, + * hence {@code null} means that element to be inserted will be the first in the file. + * + * @param file target file where some new top-level element is going to be inserted + * @return anchor PSI element as described + */ @Nullable public static PsiElement getFileInsertPosition(final PsiFile file) { return getInsertPosition(file, null, null); diff --git a/python/src/com/jetbrains/python/refactoring/move/PyMoveRefactoringUtil.java b/python/src/com/jetbrains/python/refactoring/move/PyMoveRefactoringUtil.java index 6f5bbb7a08f8..b77c39d0f9ca 100644 --- a/python/src/com/jetbrains/python/refactoring/move/PyMoveRefactoringUtil.java +++ b/python/src/com/jetbrains/python/refactoring/move/PyMoveRefactoringUtil.java @@ -18,15 +18,26 @@ package com.jetbrains.python.refactoring.move; import com.intellij.openapi.util.text.StringUtil; import com.intellij.openapi.vfs.VirtualFile; import com.intellij.psi.PsiElement; +import com.intellij.psi.PsiFile; import com.intellij.psi.PsiNamedElement; +import com.intellij.psi.util.PsiUtilCore; import com.intellij.psi.util.QualifiedName; +import com.intellij.usageView.UsageInfo; import com.intellij.util.IncorrectOperationException; import com.jetbrains.python.PyBundle; +import com.jetbrains.python.codeInsight.dataflow.scope.ScopeUtil; import com.jetbrains.python.psi.PyFunction; import com.jetbrains.python.psi.PyQualifiedNameOwner; +import com.jetbrains.python.psi.impl.PyImportStatementNavigator; +import com.jetbrains.python.psi.impl.PyPsiUtils; import com.jetbrains.python.psi.resolve.QualifiedNameFinder; import com.jetbrains.python.refactoring.classes.PyClassRefactoringUtil; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +import java.util.List; +import java.util.Objects; +import java.util.Optional; /** * @author Mikhail Golubev @@ -78,4 +89,31 @@ public class PyMoveRefactoringUtil { } return ""; } + + /** + * Returns anchor PSI element for {@link PsiElement#addBefore(PsiElement, PsiElement)}. + *

+ * If there are any usages at file's level returns the top-level parent element for the first of them, + * otherwise return {@code null} which means that the element can be safely inserted at the end of the file. + * + * @param usages usages of the original element + * @param destination file where original/generated element is to be moved + * @return anchor element as described + */ + @Nullable + public static PsiElement findLowestPossibleTopLevelInsertionPosition(@NotNull List usages, @NotNull PsiFile destination) { + return findFirstTopLevelUsageInFile(usages, destination) + .map(element -> PyPsiUtils.getParentRightBefore(element, element.getContainingFile())) + .orElse(null); + } + + @NotNull + private static Optional findFirstTopLevelUsageInFile(@NotNull List usages, @NotNull PsiFile destination) { + return usages.stream() + .map(UsageInfo::getElement) + .filter(Objects::nonNull) + .filter(element -> ScopeUtil.getScopeOwner(element) == destination) + .filter(element -> PyImportStatementNavigator.getImportStatementByElement(element) == null) + .min(PsiUtilCore::compareElementsByPosition); + } } diff --git a/python/src/com/jetbrains/python/refactoring/move/makeFunctionTopLevel/PyBaseMakeFunctionTopLevelProcessor.java b/python/src/com/jetbrains/python/refactoring/move/makeFunctionTopLevel/PyBaseMakeFunctionTopLevelProcessor.java index 995947d1977c..65c78d1bbf16 100644 --- a/python/src/com/jetbrains/python/refactoring/move/makeFunctionTopLevel/PyBaseMakeFunctionTopLevelProcessor.java +++ b/python/src/com/jetbrains/python/refactoring/move/makeFunctionTopLevel/PyBaseMakeFunctionTopLevelProcessor.java @@ -37,7 +37,6 @@ import com.jetbrains.python.codeInsight.controlflow.ControlFlowCache; import com.jetbrains.python.codeInsight.controlflow.ReadWriteInstruction; import com.jetbrains.python.codeInsight.controlflow.ScopeOwner; import com.jetbrains.python.codeInsight.dataflow.scope.ScopeUtil; -import com.jetbrains.python.codeInsight.imports.AddImportHelper; import com.jetbrains.python.psi.*; import com.jetbrains.python.psi.impl.PyPsiUtils; import com.jetbrains.python.psi.resolve.PyResolveContext; @@ -46,11 +45,9 @@ import com.jetbrains.python.refactoring.PyRefactoringUtil; import com.jetbrains.python.refactoring.classes.PyClassRefactoringUtil; import com.jetbrains.python.refactoring.move.PyMoveRefactoringUtil; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; -import java.util.ArrayList; -import java.util.Collection; -import java.util.List; -import java.util.Set; +import java.util.*; import static com.jetbrains.python.psi.PyUtil.as; @@ -117,11 +114,12 @@ public abstract class PyBaseMakeFunctionTopLevelProcessor extends BaseRefactorin if (importsRequired(usages, targetFile)) { PyMoveRefactoringUtil.checkValidImportableFile(targetFile, targetFile.getVirtualFile()); } - + + final PsiElement position = PyMoveRefactoringUtil.findLowestPossibleTopLevelInsertionPosition(Arrays.asList(usages), targetFile); // We should update usages before we generate and insert new function, because we have to update its usages inside // (e.g. recursive calls) it first updateUsages(newParameters, usages); - final PyFunction newFunction = insertFunction(createNewFunction(newParameters), targetFile); + final PyFunction newFunction = insertFunction(createNewFunction(newParameters), targetFile, position); myFunction.delete(); @@ -200,17 +198,16 @@ public abstract class PyBaseMakeFunctionTopLevelProcessor extends BaseRefactorin } @NotNull - protected PyFunction insertFunction(@NotNull PyFunction newFunction, PyFile newFile) { - final PyFunction replacement; + protected PyFunction insertFunction(@NotNull PyFunction newFunction, @NotNull PyFile newFile, @Nullable PsiElement anchor) { if (mySourceFile == newFile) { - final PsiElement anchor; - anchor = PyPsiUtils.getParentRightBefore(myFunction, mySourceFile); - replacement = (PyFunction)mySourceFile.addAfter(newFunction, anchor); + // In the same file try inserting generated function at the top level but preferably right after the original scope owner + final PsiElement surroundingStatement = PyPsiUtils.getParentRightBefore(myFunction, mySourceFile); + if (anchor == null || surroundingStatement.getTextRange().getEndOffset() < anchor.getTextRange().getStartOffset()) { + return (PyFunction)mySourceFile.addAfter(newFunction, surroundingStatement); + } } - else { - replacement = (PyFunction)newFile.addAfter(newFunction, AddImportHelper.getFileInsertPosition(newFile)); - } - return replacement; + // Insert at the end or before first top-level usage in the file + return (PyFunction)newFile.addBefore(newFunction, anchor); } @NotNull diff --git a/python/src/com/jetbrains/python/refactoring/move/moduleMembers/PyMoveSymbolProcessor.java b/python/src/com/jetbrains/python/refactoring/move/moduleMembers/PyMoveSymbolProcessor.java index 90c087280291..7ab7f3b9c6f4 100644 --- a/python/src/com/jetbrains/python/refactoring/move/moduleMembers/PyMoveSymbolProcessor.java +++ b/python/src/com/jetbrains/python/refactoring/move/moduleMembers/PyMoveSymbolProcessor.java @@ -29,6 +29,7 @@ import com.jetbrains.python.psi.*; import com.jetbrains.python.psi.resolve.PyResolveContext; import com.jetbrains.python.psi.resolve.QualifiedNameFinder; import com.jetbrains.python.refactoring.classes.PyClassRefactoringUtil; +import com.jetbrains.python.refactoring.move.PyMoveRefactoringUtil; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -94,33 +95,8 @@ public class PyMoveSymbolProcessor { @NotNull private PsiElement addElementToFile(@NotNull PsiElement element) { - final PsiElement firstUsage = findFirstTopLevelWithUsageAtDestination(); - if (firstUsage != null) { - return myDestinationFile.addBefore(element, firstUsage); - } - else { - return myDestinationFile.add(element); - } - } - - @Nullable - private PsiElement findFirstTopLevelWithUsageAtDestination() { - final List topLevelAtDestination = ContainerUtil.mapNotNull(myUsages, usage -> { - final PsiElement element = usage.getElement(); - if (element != null && ScopeUtil.getScopeOwner(element) == myDestinationFile && getImportStatementByElement(element) == null) { - return findTopLevelParent(element); - } - return null; - }); - if (topLevelAtDestination.isEmpty()) { - return null; - } - return Collections.min(topLevelAtDestination, PsiUtilCore::compareElementsByPosition); - } - - @Nullable - private PsiElement findTopLevelParent(@NotNull PsiElement element) { - return PsiTreeUtil.findFirstParent(element, element1 -> element1.getParent() == myDestinationFile); + final PsiElement anchor = PyMoveRefactoringUtil.findLowestPossibleTopLevelInsertionPosition(myUsages, myDestinationFile); + return myDestinationFile.addBefore(element, anchor); } private void updateSingleUsage(@NotNull PsiElement usage, @NotNull PsiNamedElement newElement) { diff --git a/python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionNoUsageInAnotherFile/after/main.py b/python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionNoUsageInAnotherFile/after/main.py new file mode 100644 index 000000000000..646b07aed7f4 --- /dev/null +++ b/python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionNoUsageInAnotherFile/after/main.py @@ -0,0 +1,2 @@ +class C: + pass diff --git a/python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionNoUsageInAnotherFile/after/other.py b/python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionNoUsageInAnotherFile/after/other.py new file mode 100644 index 000000000000..fcac8ca8e521 --- /dev/null +++ b/python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionNoUsageInAnotherFile/after/other.py @@ -0,0 +1,14 @@ +def already_existing1(): + pass + + +def already_existing2(): + pass + + +def already_existing3(): + pass + + +def method(): + pass \ No newline at end of file diff --git a/python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionNoUsageInAnotherFile/before/main.py b/python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionNoUsageInAnotherFile/before/main.py new file mode 100644 index 000000000000..3ae685e65d51 --- /dev/null +++ b/python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionNoUsageInAnotherFile/before/main.py @@ -0,0 +1,3 @@ +class C: + def method(self): + pass diff --git a/python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionNoUsageInAnotherFile/before/other.py b/python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionNoUsageInAnotherFile/before/other.py new file mode 100644 index 000000000000..213b8c887a31 --- /dev/null +++ b/python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionNoUsageInAnotherFile/before/other.py @@ -0,0 +1,10 @@ +def already_existing1(): + pass + + +def already_existing2(): + pass + + +def already_existing3(): + pass diff --git a/python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionSameFileClassAndUsageNotTopLevel.after.py b/python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionSameFileClassAndUsageNotTopLevel.after.py new file mode 100644 index 000000000000..143941ce05bd --- /dev/null +++ b/python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionSameFileClassAndUsageNotTopLevel.after.py @@ -0,0 +1,10 @@ +def method(): + pass + + +if True: + class C: + pass + + + method() \ No newline at end of file diff --git a/python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionSameFileClassAndUsageNotTopLevel.py b/python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionSameFileClassAndUsageNotTopLevel.py new file mode 100644 index 000000000000..8873a35a3e01 --- /dev/null +++ b/python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionSameFileClassAndUsageNotTopLevel.py @@ -0,0 +1,7 @@ +if True: + class C: + def method(self): + pass + + + C().method() \ No newline at end of file diff --git a/python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionUsageInAnotherFile/after/main.py b/python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionUsageInAnotherFile/after/main.py new file mode 100644 index 000000000000..646b07aed7f4 --- /dev/null +++ b/python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionUsageInAnotherFile/after/main.py @@ -0,0 +1,2 @@ +class C: + pass diff --git a/python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionUsageInAnotherFile/after/other.py b/python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionUsageInAnotherFile/after/other.py new file mode 100644 index 000000000000..7c47ab79419d --- /dev/null +++ b/python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionUsageInAnotherFile/after/other.py @@ -0,0 +1,20 @@ +def already_existing1(): + pass + + +def already_existing2(): + pass + + +from main import C + + +def method(): + pass + + +method() + + +def already_existing3(): + pass diff --git a/python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionUsageInAnotherFile/before/main.py b/python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionUsageInAnotherFile/before/main.py new file mode 100644 index 000000000000..3ae685e65d51 --- /dev/null +++ b/python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionUsageInAnotherFile/before/main.py @@ -0,0 +1,3 @@ +class C: + def method(self): + pass diff --git a/python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionUsageInAnotherFile/before/other.py b/python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionUsageInAnotherFile/before/other.py new file mode 100644 index 000000000000..5a5e09672329 --- /dev/null +++ b/python/testData/refactoring/makeFunctionTopLevel/methodInsertionPositionUsageInAnotherFile/before/other.py @@ -0,0 +1,15 @@ +def already_existing1(): + pass + + +def already_existing2(): + pass + + +from main import C + +C().method() + + +def already_existing3(): + pass diff --git a/python/testSrc/com/jetbrains/python/refactoring/PyMakeFunctionTopLevelTest.java b/python/testSrc/com/jetbrains/python/refactoring/PyMakeFunctionTopLevelTest.java index f3f5b9c6f179..41709fe7bf0b 100644 --- a/python/testSrc/com/jetbrains/python/refactoring/PyMakeFunctionTopLevelTest.java +++ b/python/testSrc/com/jetbrains/python/refactoring/PyMakeFunctionTopLevelTest.java @@ -23,11 +23,7 @@ import com.intellij.openapi.vfs.VirtualFile; import com.intellij.psi.PsiElement; import com.intellij.psi.PsiFile; import com.intellij.psi.PsiReference; -import com.intellij.refactoring.RefactoringBundle; -import com.intellij.refactoring.actions.MoveAction; -import com.intellij.refactoring.util.CommonRefactoringUtil; import com.intellij.testFramework.PlatformTestUtil; -import com.intellij.testFramework.TestActionEvent; import com.intellij.util.IncorrectOperationException; import com.jetbrains.python.PyBundle; import com.jetbrains.python.PyTokenTypes; @@ -276,6 +272,18 @@ public class PyMakeFunctionTopLevelTest extends PyTestCase { doTestFailure(PyBundle.message("refactoring.move.error.destination.file.contains.function.$0", "nested")); } + public void testMethodInsertionPositionSameFileClassAndUsageNotTopLevel() { + doTestSuccess(); + } + + public void testMethodInsertionPositionUsageInAnotherFile() throws IOException { + doMultiFileTest("other.py", null); + } + + public void testMethodInsertionPositionNoUsageInAnotherFile() throws IOException { + doMultiFileTest("other.py", null); + } + @Override protected String getTestDataPath() { return super.getTestDataPath() + "/refactoring/makeFunctionTopLevel/";