diff --git a/python/src/com/jetbrains/python/PyBundle.properties b/python/src/com/jetbrains/python/PyBundle.properties index b34d5617ddf4..77337c0e411d 100644 --- a/python/src/com/jetbrains/python/PyBundle.properties +++ b/python/src/com/jetbrains/python/PyBundle.properties @@ -617,6 +617,7 @@ refactoring.move.class.or.function.to.file=To file: refactoring.move.class.or.function.error.cannot.place.elements.into.nonpython.file=Cannot place elements into a non-Python file refactoring.move.class.or.function.error.destination.file.contains.class.$0=Destination file already contains class named ''{0}'' refactoring.move.class.or.function.error.destination.file.contains.function.$0=Destination file already contains function named ''{0}()'' +refactoring.move.class.or.function.error.destination.file.contains.global.variable.$0=Destination file already contains global variable named ''{0}'' refactoring.move.class.or.function.error.cannot.use.module.name.$0=Cannot use module name ''{0}'' in imports refactoring.move.class.or.function.error.selection=Cannot perform refactoring using selected element(s) diff --git a/python/src/com/jetbrains/python/findUsages/PyFindUsagesHandlerFactory.java b/python/src/com/jetbrains/python/findUsages/PyFindUsagesHandlerFactory.java index 90bdf215a46f..b4d50d6af945 100644 --- a/python/src/com/jetbrains/python/findUsages/PyFindUsagesHandlerFactory.java +++ b/python/src/com/jetbrains/python/findUsages/PyFindUsagesHandlerFactory.java @@ -20,10 +20,7 @@ import com.intellij.find.findUsages.FindUsagesHandlerFactory; import com.intellij.openapi.ui.Messages; import com.intellij.psi.PsiElement; import com.intellij.psi.PsiFileSystemItem; -import com.jetbrains.python.psi.PyClass; -import com.jetbrains.python.psi.PyFile; -import com.jetbrains.python.psi.PyFunction; -import com.jetbrains.python.psi.PyUtil; +import com.jetbrains.python.psi.*; import com.jetbrains.python.psi.impl.PyImportedModule; import com.jetbrains.python.psi.search.PySuperMethodsSearch; import org.jetbrains.annotations.NotNull; @@ -42,7 +39,8 @@ public class PyFindUsagesHandlerFactory extends FindUsagesHandlerFactory { return element instanceof PyClass || (element instanceof PyFile && PyUtil.isPackage((PyFile)element)) || element instanceof PyImportedModule || - element instanceof PyFunction; + element instanceof PyFunction || + element instanceof PyTargetExpression; } @Nullable @@ -88,6 +86,9 @@ public class PyFindUsagesHandlerFactory extends FindUsagesHandlerFactory { if (element instanceof PyClass) { return new PyClassFindUsagesHandler((PyClass)element); } + if (element instanceof PyTargetExpression) { + return new PyTargetExpressionFindUsagesHandler(((PyTargetExpression)element)); + } return null; } diff --git a/python/src/com/jetbrains/python/findUsages/PyTargetExpressionFindUsagesHandler.java b/python/src/com/jetbrains/python/findUsages/PyTargetExpressionFindUsagesHandler.java new file mode 100644 index 000000000000..5956973ed7fd --- /dev/null +++ b/python/src/com/jetbrains/python/findUsages/PyTargetExpressionFindUsagesHandler.java @@ -0,0 +1,14 @@ +package com.jetbrains.python.findUsages; + +import com.intellij.find.findUsages.FindUsagesHandler; +import com.jetbrains.python.psi.PyTargetExpression; +import org.jetbrains.annotations.NotNull; + +/** + * @author Mikhail Golubev + */ +public class PyTargetExpressionFindUsagesHandler extends FindUsagesHandler { + public PyTargetExpressionFindUsagesHandler(@NotNull PyTargetExpression psiElement) { + super(psiElement); + } +} diff --git a/python/src/com/jetbrains/python/refactoring/move/PyDependentModuleMembersCollector.java b/python/src/com/jetbrains/python/refactoring/move/PyDependentModuleMembersCollector.java index cebb4cc81c18..62d960c70402 100644 --- a/python/src/com/jetbrains/python/refactoring/move/PyDependentModuleMembersCollector.java +++ b/python/src/com/jetbrains/python/refactoring/move/PyDependentModuleMembersCollector.java @@ -1,8 +1,8 @@ package com.jetbrains.python.refactoring.move; import com.intellij.psi.PsiElement; +import com.intellij.psi.PsiNamedElement; import com.intellij.refactoring.classMembers.DependentMembersCollectorBase; -import com.jetbrains.python.psi.PyElement; import com.jetbrains.python.psi.PyFile; import com.jetbrains.python.psi.PyRecursiveElementVisitor; import com.jetbrains.python.psi.PyUtil; @@ -11,13 +11,13 @@ import org.jetbrains.annotations.NotNull; /** * Collects dependencies of the top-level symbols in the given module. This information is used then to highlight them - * in "Move ..." dialog the same way as it's done for members of classes in various class-related refactorings. + * in "Move" dialog the same way as it's done for members of classes in various class-related refactorings. * * @see PyModuleMemberInfoModel * * @author Mikhail Golubev */ -public class PyDependentModuleMembersCollector extends DependentMembersCollectorBase { +public class PyDependentModuleMembersCollector extends DependentMembersCollectorBase { private final PyFile myModule; public PyDependentModuleMembersCollector(@NotNull PyFile module) { @@ -26,15 +26,17 @@ public class PyDependentModuleMembersCollector extends DependentMembersCollector } @Override - public void collect(final PyElement member) { + public void collect(final PsiNamedElement member) { if (member.getContainingFile() == myModule) { final PyResolveContext resolveContext = PyResolveContext.defaultContext(); - member.accept(new PyRecursiveElementVisitor() { + final PsiElement memberBody = PyMoveModuleMemberUtil.expandNamedElementBody(member); + assert memberBody != null; + memberBody.accept(new PyRecursiveElementVisitor() { @Override public void visitElement(PsiElement element) { for (PsiElement result : PyUtil.multiResolveTopPriority(element, resolveContext)) { if (isValidSameModuleDependency(result) && result != member) { - myCollection.add(((PyElement)result)); + myCollection.add((PsiNamedElement)result); } } super.visitElement(element); @@ -44,6 +46,6 @@ public class PyDependentModuleMembersCollector extends DependentMembersCollector } private boolean isValidSameModuleDependency(@NotNull PsiElement element) { - return PyMoveModuleMembersDelegate.canMoveElement(element) && element.getContainingFile() == myModule; + return PyMoveModuleMemberUtil.isMovableModuleMember(element) && element.getContainingFile() == myModule; } } diff --git a/python/src/com/jetbrains/python/refactoring/move/PyMoveModuleMemberUtil.java b/python/src/com/jetbrains/python/refactoring/move/PyMoveModuleMemberUtil.java new file mode 100644 index 000000000000..b80bdc784677 --- /dev/null +++ b/python/src/com/jetbrains/python/refactoring/move/PyMoveModuleMemberUtil.java @@ -0,0 +1,86 @@ +package com.jetbrains.python.refactoring.move; + +import com.intellij.psi.PsiElement; +import com.intellij.psi.PsiNamedElement; +import com.jetbrains.python.psi.*; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +import static com.jetbrains.python.psi.PyUtil.as; + +/** + * @author Mikhail Golubev + */ +public class PyMoveModuleMemberUtil { + private PyMoveModuleMemberUtil() { + // Utility class + } + + /** + * Checks that given element is suitable for the "Move" refactoring. Currently it means that it's either top-level function, class or + * target expression (global variable, constant). + * + * @param element PSI element to check + * @return whether this element is acceptable for "Move ..." refactoring + */ + public static boolean isMovableModuleMember(@NotNull PsiElement element) { + return isMovableElement(element) && PyUtil.isTopLevel(element); + } + + public static boolean isMovableElement(@NotNull PsiElement element) { + return element instanceof PyClass || element instanceof PyFunction || isTargetOfSimpleAssignment(element); + } + + /** + * Checks that given element is target of the simplest Python assignment suitable for "Move" refactoring: + * exactly it's unqualified name in the right-hand side of assignment statement with single target, e.g. {@code CONST} in {@code CONST = 42}. + * There should be neither unpacking, nor chained assignment, nor clarifying parenthesis in assignment target. + *

+ * Such target expression at the top-level of its module can be treated as definition of global variable (supposedly constant). + * + * @param element PSI element to check + */ + public static boolean isTargetOfSimpleAssignment(@NotNull PsiElement element) { + final PyTargetExpression target = as(element, PyTargetExpression.class); + if (target == null || target.isQualified()) { + return false; + } + final PyAssignmentStatement assignment = as(target.getParent(), PyAssignmentStatement.class); + return assignment != null && assignment.getTargets().length == 1; + } + + /** + * Expands given named element to the closet parent suitable for "Move" refactoring. In particular for target expression + * it returns parental assignment statement if any and element itself for functions and classes. + * + * @see #extractNamedElement(PsiElement) + */ + @Nullable + public static PsiElement expandNamedElementBody(@NotNull PsiNamedElement element) { + if (element instanceof PyClass || element instanceof PyFunction) { + return element; + } + else if (element instanceof PyTargetExpression && element.getParent() instanceof PyAssignmentStatement) { + return element.getParent(); + } + return null; + } + + /** + * Performs operation opposite to the {@link #expandNamedElementBody}, in particular it shrinks assignment statement back + * to the first target expression. + * + * @see #expandNamedElementBody(PsiNamedElement) + */ + @Nullable + public static PsiNamedElement extractNamedElement(@NotNull PsiElement element) { + if (element instanceof PyClass || element instanceof PyFunction) { + return (PsiNamedElement)element; + } + final PyAssignmentStatement assignment = as(element, PyAssignmentStatement.class); + if (assignment != null) { + return as(assignment.getTargets()[0], PyTargetExpression.class); + } + return null; + } +} diff --git a/python/src/com/jetbrains/python/refactoring/move/PyMoveModuleMembersDelegate.java b/python/src/com/jetbrains/python/refactoring/move/PyMoveModuleMembersDelegate.java index b6ac9586e721..21d8e5b7781c 100644 --- a/python/src/com/jetbrains/python/refactoring/move/PyMoveModuleMembersDelegate.java +++ b/python/src/com/jetbrains/python/refactoring/move/PyMoveModuleMembersDelegate.java @@ -31,8 +31,6 @@ import com.intellij.refactoring.util.CommonRefactoringUtil; import com.intellij.util.IncorrectOperationException; import com.intellij.util.containers.ContainerUtil; import com.jetbrains.python.PyBundle; -import com.jetbrains.python.psi.PyClass; -import com.jetbrains.python.psi.PyFunction; import com.jetbrains.python.psi.PyUtil; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -46,24 +44,13 @@ public class PyMoveModuleMembersDelegate extends MoveHandlerDelegate { @Override public boolean canMove(PsiElement[] elements, @Nullable PsiElement targetContainer) { for (PsiElement element : elements) { - if (!canMoveElement(element)) { + if (!PyMoveModuleMemberUtil.isMovableModuleMember(element)) { return false; } } return super.canMove(elements, targetContainer); } - /** - * Checks that given element is suitable for "Move ..." refactoring. Currently it means that it's top-level function, class or - * target expression. - * - * @param element PSI element to check - * @return whether this element is acceptable for "Move ..." refactoring - */ - public static boolean canMoveElement(@NotNull PsiElement element) { - return (element instanceof PyClass || element instanceof PyFunction) && PyUtil.isTopLevel(element); - } - @Override public void doMove(Project project, PsiElement[] elements, @@ -107,7 +94,7 @@ public class PyMoveModuleMembersDelegate extends MoveHandlerDelegate { @Nullable PsiReference reference, @Nullable Editor editor) { final PsiNamedElement e = getElementToMove(element); - if (e instanceof PyClass || e instanceof PyFunction) { + if (e != null && PyMoveModuleMemberUtil.isMovableElement(e)) { if (PyUtil.isTopLevel(e)) { PsiElement targetContainer = null; if (editor != null) { diff --git a/python/src/com/jetbrains/python/refactoring/move/PyMoveModuleMembersProcessor.java b/python/src/com/jetbrains/python/refactoring/move/PyMoveModuleMembersProcessor.java index 0eb618aa7dfc..f2482e696f28 100644 --- a/python/src/com/jetbrains/python/refactoring/move/PyMoveModuleMembersProcessor.java +++ b/python/src/com/jetbrains/python/refactoring/move/PyMoveModuleMembersProcessor.java @@ -110,14 +110,21 @@ public class PyMoveModuleMembersProcessor extends BaseRefactoringProcessor { for (final PsiNamedElement e : myElements) { // TODO: Check for resulting circular imports CommonRefactoringUtil.checkReadOnlyStatus(myProject, e); - assert e instanceof PyClass || e instanceof PyFunction; + assert e instanceof PyClass || e instanceof PyFunction || e instanceof PyTargetExpression; if (e instanceof PyClass && destination.findTopLevelClass(e.getName()) != null) { - throw new IncorrectOperationException(PyBundle.message("refactoring.move.class.or.function.error.destination.file.contains.class.$0", - e.getName())); + throw new IncorrectOperationException( + PyBundle.message("refactoring.move.class.or.function.error.destination.file.contains.class.$0", + e.getName())); } if (e instanceof PyFunction && destination.findTopLevelFunction(e.getName()) != null) { - throw new IncorrectOperationException(PyBundle.message("refactoring.move.class.or.function.error.destination.file.contains.function.$0", - e.getName())); + throw new IncorrectOperationException( + PyBundle.message("refactoring.move.class.or.function.error.destination.file.contains.function.$0", + e.getName())); + } + if (e instanceof PyTargetExpression && destination.findTopLevelAttribute(e.getName()) != null) { + throw new IncorrectOperationException( + PyBundle.message("refactoring.move.class.or.function.error.destination.file.contains.global.variable.$0", + e.getName())); } final Collection usageInfos = usagesByElement.get(e); final boolean usedFromOutside = ContainerUtil.exists(usageInfos, new Condition() { @@ -146,24 +153,30 @@ public class PyMoveModuleMembersProcessor extends BaseRefactoringProcessor { private static void moveElement(@NotNull PsiNamedElement element, @NotNull Collection usages, @NotNull PyFile destination) { final PsiFile file = element.getContainingFile(); - PyClassRefactoringUtil.rememberNamedReferences(element); - final PsiNamedElement newElement = addToFile(element, destination, usages); - for (UsageInfo usage : usages) { - final PsiElement usageElement = usage.getElement(); - if (usageElement != null) { - updateUsage(usageElement, element, newElement); + final PsiElement oldElementBody = PyMoveModuleMemberUtil.expandNamedElementBody(element); + if (oldElementBody != null) { + PyClassRefactoringUtil.rememberNamedReferences(oldElementBody); + final PsiElement newElementBody = addToFile(oldElementBody, destination, usages); + final PsiNamedElement newElement = PyMoveModuleMemberUtil.extractNamedElement(newElementBody); + assert newElement != null; + for (UsageInfo usage : usages) { + final PsiElement usageElement = usage.getElement(); + if (usageElement != null) { + updateUsage(usageElement, element, newElement); + } + } + PyClassRefactoringUtil.restoreNamedReferences(newElementBody, element); + // TODO: Remove extra empty lines after the removed element + oldElementBody.delete(); + if (file != null) { + PyClassRefactoringUtil.optimizeImports(file); } - } - PyClassRefactoringUtil.restoreNamedReferences(newElement, element); - // TODO: Remove extra empty lines after the removed element - element.delete(); - if (file != null) { - PyClassRefactoringUtil.optimizeImports(file); } } - private static PsiNamedElement addToFile(@NotNull PsiNamedElement element, @NotNull final PyFile destination, - @NotNull Collection usages) { + @NotNull + private static PsiElement addToFile(@NotNull PsiElement element, @NotNull final PyFile destination, + @NotNull Collection usages) { List topLevelAtDestination = new ArrayList(); for (UsageInfo usage : usages) { final PsiElement e = usage.getElement(); @@ -180,7 +193,7 @@ public class PyMoveModuleMembersProcessor extends BaseRefactoringProcessor { } } if (topLevelAtDestination.isEmpty()) { - return (PsiNamedElement)(destination.add(element)); + return destination.add(element); } else { Collections.sort(topLevelAtDestination, new Comparator() { @@ -190,7 +203,7 @@ public class PyMoveModuleMembersProcessor extends BaseRefactoringProcessor { }; }); final PsiElement firstUsage = topLevelAtDestination.get(0); - return (PsiNamedElement)destination.addBefore(element, firstUsage); + return destination.addBefore(element, firstUsage); } } diff --git a/python/testData/refactoring/move/movableTopLevelAssignmentDetection.py b/python/testData/refactoring/move/movableTopLevelAssignmentDetection.py new file mode 100644 index 000000000000..8d7f87286df9 --- /dev/null +++ b/python/testData/refactoring/move/movableTopLevelAssignmentDetection.py @@ -0,0 +1,7 @@ +X1, X2 = 42, 'spam' +(X3) = 42 +X4, = 42 +X5, *other = [42] +X6 = X7 = 42 +X8 = 42 + diff --git a/python/testData/refactoring/move/topLevelVariable/after/src/a.py b/python/testData/refactoring/move/topLevelVariable/after/src/a.py new file mode 100644 index 000000000000..e0fb7d133f30 --- /dev/null +++ b/python/testData/refactoring/move/topLevelVariable/after/src/a.py @@ -0,0 +1,11 @@ +class C: + def m(self): + return 1 + + +def func(): + return 2 + + +X = 1 + diff --git a/python/testData/refactoring/move/topLevelVariable/after/src/b.py b/python/testData/refactoring/move/topLevelVariable/after/src/b.py new file mode 100644 index 000000000000..ec7d7fdfce3d --- /dev/null +++ b/python/testData/refactoring/move/topLevelVariable/after/src/b.py @@ -0,0 +1,3 @@ +from a import X, func, C + +Y = X * func() + C().m() \ No newline at end of file diff --git a/python/testData/refactoring/move/topLevelVariable/before/src/a.py b/python/testData/refactoring/move/topLevelVariable/before/src/a.py new file mode 100644 index 000000000000..498175b07955 --- /dev/null +++ b/python/testData/refactoring/move/topLevelVariable/before/src/a.py @@ -0,0 +1,12 @@ +class C: + def m(self): + return 1 + + +def func(): + return 2 + + +X = 1 + +Y = X * func() + C().m() \ No newline at end of file diff --git a/python/testData/refactoring/move/topLevelVariable/before/src/b.py b/python/testData/refactoring/move/topLevelVariable/before/src/b.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/python/testSrc/com/jetbrains/python/refactoring/PyMoveTest.java b/python/testSrc/com/jetbrains/python/refactoring/PyMoveTest.java index 85eb018e9a27..82336f72e7e6 100644 --- a/python/testSrc/com/jetbrains/python/refactoring/PyMoveTest.java +++ b/python/testSrc/com/jetbrains/python/refactoring/PyMoveTest.java @@ -20,6 +20,7 @@ import com.intellij.ide.fileTemplates.FileTemplateManager; import com.intellij.openapi.project.Project; import com.intellij.openapi.vfs.VirtualFile; import com.intellij.psi.*; +import com.intellij.psi.search.ProjectScope; import com.intellij.refactoring.move.moveFilesOrDirectories.MoveFilesOrDirectoriesProcessor; import com.intellij.testFramework.PlatformTestUtil; import com.intellij.util.IncorrectOperationException; @@ -29,14 +30,18 @@ import com.jetbrains.python.fixtures.PyTestCase; import com.jetbrains.python.psi.LanguageLevel; import com.jetbrains.python.psi.PyClass; import com.jetbrains.python.psi.PyFunction; +import com.jetbrains.python.psi.PyTargetExpression; import com.jetbrains.python.psi.stubs.PyClassNameIndex; import com.jetbrains.python.psi.stubs.PyFunctionNameIndex; +import com.jetbrains.python.psi.stubs.PyVariableNameIndex; import com.jetbrains.python.refactoring.move.PyMoveModuleMembersProcessor; import org.jetbrains.annotations.Nullable; import java.io.IOException; import java.util.Collection; +import static com.jetbrains.python.refactoring.move.PyMoveModuleMemberUtil.isMovableModuleMember; + /** * @author vlan */ @@ -55,6 +60,28 @@ public class PyMoveTest extends PyTestCase { doMoveSymbolTest("C", "b.py"); } + // PY-11923 + public void testTopLevelVariable() { + doMoveSymbolTest("Y", "b.py"); + } + + // PY-11923 + public void testMovableTopLevelAssignmentDetection() { + runWithLanguageLevel(LanguageLevel.PYTHON30, new Runnable() { + public void run() { + myFixture.configureByFile("/refactoring/move/" + getTestName(true) + ".py"); + assertFalse(isMovableModuleMember(findFirstNamedElement("X1"))); + assertFalse(isMovableModuleMember(findFirstNamedElement("X3"))); + assertFalse(isMovableModuleMember(findFirstNamedElement("X2"))); + assertFalse(isMovableModuleMember(findFirstNamedElement("X4"))); + assertFalse(isMovableModuleMember(findFirstNamedElement("X5"))); + assertFalse(isMovableModuleMember(findFirstNamedElement("X6"))); + assertFalse(isMovableModuleMember(findFirstNamedElement("X7"))); + assertTrue(isMovableModuleMember(findFirstNamedElement("X8"))); + } + }); + } + // PY-3929 // PY-4095 public void testImportAs() { @@ -318,14 +345,19 @@ public class PyMoveTest extends PyTestCase { @Nullable private PsiNamedElement findFirstNamedElement(String name) { - final Collection classes = PyClassNameIndex.find(name, myFixture.getProject(), false); + final Project project = myFixture.getProject(); + final Collection classes = PyClassNameIndex.find(name, project, false); if (classes.size() > 0) { return classes.iterator().next(); } - final Collection functions = PyFunctionNameIndex.find(name, myFixture.getProject()); + final Collection functions = PyFunctionNameIndex.find(name, project); if (functions.size() > 0) { return functions.iterator().next(); } + final Collection targets = PyVariableNameIndex.find(name, project, ProjectScope.getAllScope(project)); + if (targets.size() > 0) { + return targets.iterator().next(); + } return null; } }