PY-17265 Check that target module doesn't contain function with this name already...

diff --git a/python/src/com/jetbrains/python/PyBundle.properties b/python/src/com/jetbrains/python/PyBundle.properties
index 00ed915e4bb08cdcd89cc690d704da3d84e81ce8..255e646118e59d5fb464fcd9db2079a474eb8997 100644 (file)
--- a/python/src/com/jetbrains/python/PyBundle.properties
+++ b/python/src/com/jetbrains/python/PyBundle.properties
@@ -645,6 +645,10 @@ refactoring.extract.super.class.no.members.allowed=None of members could be extr
 
 # move
 refactoring.move.choose.destination.file.title=Choose Destination File
+refactoring.move.error.destination.file.contains.class.$0=Destination file already contains class named ''{0}''
+refactoring.move.error.destination.file.contains.function.$0=Destination file already contains function named ''{0}()''
+refactoring.move.error.destination.file.contains.global.variable.$0=Destination file already contains global variable named ''{0}''
+refactoring.move.error.cannot.use.module.name.$0=Cannot use module name ''{0}'' in imports
 
 # move module members (top-level)
 refactoring.move.module.members=Move module members
@@ -655,10 +659,6 @@ refactoring.move.module.members.dialog.description.function.$0=Move function {0}
 refactoring.move.module.members.dialog.description.variable.$0=Move global variable {0}
 refactoring.move.module.members.dialog.description.selection=Move selected elements
 refactoring.move.module.members.error.cannot.place.elements.into.nonpython.file=Cannot place elements into a non-Python file
-refactoring.move.module.members.error.destination.file.contains.class.$0=Destination file already contains class named ''{0}''
-refactoring.move.module.members.error.destination.file.contains.function.$0=Destination file already contains function named ''{0}()''
-refactoring.move.module.members.error.destination.file.contains.global.variable.$0=Destination file already contains global variable named ''{0}''
-refactoring.move.module.members.error.cannot.use.module.name.$0=Cannot use module name ''{0}'' in imports
 refactoring.move.module.members.error.selection=Cannot perform refactoring using selected element(s)
 
 # Make function top-level

diff --git a/python/src/com/jetbrains/python/refactoring/makeFunctionTopLevel/PyBaseMakeFunctionTopLevelProcessor.java b/python/src/com/jetbrains/python/refactoring/makeFunctionTopLevel/PyBaseMakeFunctionTopLevelProcessor.java
index 0fb9a680b572c7fd651fac1df30b6ecd38aabf5f..00d1ef3979b920a2aab9e5b6f71ca887fa497249 100644 (file)
--- a/python/src/com/jetbrains/python/refactoring/makeFunctionTopLevel/PyBaseMakeFunctionTopLevelProcessor.java
+++ b/python/src/com/jetbrains/python/refactoring/makeFunctionTopLevel/PyBaseMakeFunctionTopLevelProcessor.java
@@ -18,6 +18,7 @@ package com.jetbrains.python.refactoring.makeFunctionTopLevel;
 import com.intellij.codeInsight.controlflow.ControlFlow;
 import com.intellij.codeInsight.controlflow.Instruction;
 import com.intellij.openapi.application.ApplicationManager;
+import com.intellij.openapi.util.Condition;
 import com.intellij.openapi.util.text.StringUtil;
 import com.intellij.psi.PsiElement;
 import com.intellij.psi.PsiFile;
@@ -28,7 +29,10 @@ import com.intellij.refactoring.ui.UsageViewDescriptorAdapter;
 import com.intellij.usageView.UsageInfo;
 import com.intellij.usageView.UsageViewDescriptor;
 import com.intellij.util.ArrayUtil;
+import com.intellij.util.IncorrectOperationException;
+import com.intellij.util.containers.ContainerUtil;
 import com.intellij.util.containers.HashSet;
+import com.jetbrains.python.PyBundle;
 import com.jetbrains.python.PyNames;
 import com.jetbrains.python.codeInsight.controlflow.ControlFlowCache;
 import com.jetbrains.python.codeInsight.controlflow.ReadWriteInstruction;
@@ -41,6 +45,7 @@ import com.jetbrains.python.psi.resolve.PyResolveContext;
 import com.jetbrains.python.psi.types.TypeEvalContext;
 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 java.util.ArrayList;
@@ -105,17 +110,43 @@ public abstract class PyBaseMakeFunctionTopLevelProcessor extends BaseRefactorin
 
     assert ApplicationManager.getApplication().isWriteAccessAllowed();
 
+    final PyFile targetFile = PyUtil.getOrCreateFile(myDestinationPath, myProject);
+    if (targetFile.findTopLevelFunction(myFunction.getName()) != null) {
+      throw new IncorrectOperationException(
+        PyBundle.message("refactoring.move.error.destination.file.contains.function.$0", myFunction.getName()));
+    }
+    if (importsRequired(usages, targetFile)) {
+      PyMoveRefactoringUtil.checkValidImportableFile(targetFile, targetFile.getVirtualFile());
+    }
+    
     // 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 PyFile newFile = PyUtil.getOrCreateFile(myDestinationPath, myProject);
-    final PyFunction newFunction = insertFunction(createNewFunction(newParameters), newFile);
+    final PyFunction newFunction = insertFunction(createNewFunction(newParameters), targetFile);
 
     myFunction.delete();
 
     updateImports(newFunction, usages);
   }
 
+  private boolean importsRequired(@NotNull UsageInfo[] usages, final PyFile targetFile) {
+    return ContainerUtil.exists(usages, new Condition<UsageInfo>() {
+      @Override
+      public boolean value(UsageInfo info) {
+        final PsiElement element = info.getElement();
+        if (element == null) {
+          return false;
+        }
+        return !belongsToFunction(element) && info.getFile() != targetFile;
+      }
+    });
+  }
+
+  private boolean belongsToFunction(PsiElement element) {
+    return PsiTreeUtil.isAncestor(myFunction, element, false);
+  }
+
+
   private void updateImports(@NotNull PyFunction newFunction, @NotNull UsageInfo[] usages) {
     final Set<PsiFile> usageFiles = new HashSet<PsiFile>();
     for (UsageInfo usage : usages) {
@@ -206,7 +237,7 @@ public abstract class PyBaseMakeFunctionTopLevelProcessor extends BaseRefactorin
               if (isFromEnclosingScope(resolved)) {
                 result.readsFromEnclosingScope.add(element);
               }
-              else if (!PsiTreeUtil.isAncestor(myFunction, resolved, false)) {
+              else if (!belongsToFunction(resolved)) {
                 myExternalReads.add(resolved);
               }
               if (resolved instanceof PyParameter && ((PyParameter)resolved).isSelf()) {
@@ -244,8 +275,8 @@ public abstract class PyBaseMakeFunctionTopLevelProcessor extends BaseRefactorin
   }
 
   private boolean isFromEnclosingScope(@NotNull PsiElement element) {
-    return element.getContainingFile() == mySourceFile &&
-           !PsiTreeUtil.isAncestor(myFunction, element, false) &&
+    return PyUtil.inSameFile(element, myFunction) &&
+           !belongsToFunction(element) &&
            !(ScopeUtil.getScopeOwner(element) instanceof PsiFile); 
   }
 

diff --git a/python/src/com/jetbrains/python/refactoring/move/PyMoveModuleMembersProcessor.java b/python/src/com/jetbrains/python/refactoring/move/PyMoveModuleMembersProcessor.java
index 4a05f3d48758b348ea002ba23a901d10de7723fd..1e1afcf741b9b851f814a2ee5115d94f41b3d0a2 100644 (file)
--- a/python/src/com/jetbrains/python/refactoring/move/PyMoveModuleMembersProcessor.java
+++ b/python/src/com/jetbrains/python/refactoring/move/PyMoveModuleMembersProcessor.java
@@ -19,11 +19,9 @@ import com.intellij.openapi.application.ApplicationManager;
 import com.intellij.openapi.command.CommandProcessor;
 import com.intellij.openapi.project.Project;
 import com.intellij.openapi.util.Condition;
-import com.intellij.openapi.vfs.VirtualFile;
 import com.intellij.psi.PsiElement;
 import com.intellij.psi.PsiNamedElement;
 import com.intellij.psi.util.PsiTreeUtil;
-import com.intellij.psi.util.QualifiedName;
 import com.intellij.refactoring.BaseRefactoringProcessor;
 import com.intellij.refactoring.ui.UsageViewDescriptorAdapter;
 import com.intellij.refactoring.util.CommonRefactoringUtil;
@@ -35,9 +33,7 @@ import com.intellij.util.containers.ContainerUtil;
 import com.intellij.util.containers.MultiMap;
 import com.jetbrains.python.PyBundle;
 import com.jetbrains.python.psi.*;
-import com.jetbrains.python.psi.resolve.QualifiedNameFinder;
 import com.jetbrains.python.refactoring.PyRefactoringUtil;
-import com.jetbrains.python.refactoring.classes.PyClassRefactoringUtil;
 import org.jetbrains.annotations.NotNull;
 
 import java.util.ArrayList;
@@ -47,7 +43,6 @@ import java.util.List;
 /**
  * Group found usages by moved elements and move each of these elements using {@link PyMoveSymbolProcessor}.
  *
- * attribute's
  * @author vlan
  * @author Mikhail Golubev
  *
@@ -100,36 +95,45 @@ public class PyMoveModuleMembersProcessor extends BaseRefactoringProcessor {
     for (UsageInfo usage : usages) {
       usagesByElement.putValue(((MyUsageInfo)usage).myMovedElement, usage);
     }
-    CommandProcessor.getInstance().executeCommand(myElements[0].getProject(), () -> ApplicationManager.getApplication().runWriteAction(() -> {
-      final PyFile destination = PyUtil.getOrCreateFile(myDestination, myProject);
-      CommonRefactoringUtil.checkReadOnlyStatus(myProject, destination);
-      for (final PsiNamedElement e : myElements) {
-        // TODO: Check for resulting circular imports
-        CommonRefactoringUtil.checkReadOnlyStatus(myProject, e);
-        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.module.members.error.destination.file.contains.class.$0", e.getName()));
-        }
-        if (e instanceof PyFunction && destination.findTopLevelFunction(e.getName()) != null) {
-          throw new IncorrectOperationException(
-            PyBundle.message("refactoring.move.module.members.error.destination.file.contains.function.$0", e.getName()));
-        }
-        if (e instanceof PyTargetExpression && destination.findTopLevelAttribute(e.getName()) != null) {
-          throw new IncorrectOperationException(
-            PyBundle.message("refactoring.move.module.members.error.destination.file.contains.global.variable.$0", e.getName()));
-        }
-        final Collection<UsageInfo> usageInfos = usagesByElement.get(e);
-        final boolean usedFromOutside = ContainerUtil.exists(usageInfos, usageInfo -> {
-          final PsiElement element = usageInfo.getElement();
-          return element != null && !PsiTreeUtil.isAncestor(e, element, false);
+    CommandProcessor.getInstance().executeCommand(myElements[0].getProject(), new Runnable() {
+      public void run() {
+        ApplicationManager.getApplication().runWriteAction(new Runnable() {
+          public void run() {
+            final PyFile destination = PyUtil.getOrCreateFile(myDestination, myProject);
+            CommonRefactoringUtil.checkReadOnlyStatus(myProject, destination);
+            for (final PsiNamedElement e : myElements) {
+              // TODO: Check for resulting circular imports
+              CommonRefactoringUtil.checkReadOnlyStatus(myProject, e);
+              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.error.destination.file.contains.class.$0", e.getName()));
+              }
+              if (e instanceof PyFunction && destination.findTopLevelFunction(e.getName()) != null) {
+                throw new IncorrectOperationException(
+                  PyBundle.message("refactoring.move.error.destination.file.contains.function.$0", e.getName()));
+              }
+              if (e instanceof PyTargetExpression && destination.findTopLevelAttribute(e.getName()) != null) {
+                throw new IncorrectOperationException(
+                  PyBundle.message("refactoring.move.error.destination.file.contains.global.variable.$0", e.getName()));
+              }
+              final Collection<UsageInfo> usageInfos = usagesByElement.get(e);
+              final boolean usedFromOutside = ContainerUtil.exists(usageInfos, new Condition<UsageInfo>() {
+                @Override
+                public boolean value(UsageInfo usageInfo) {
+                  final PsiElement element = usageInfo.getElement();
+                  return element != null && !PsiTreeUtil.isAncestor(e, element, false);
+                }
+              });
+              if (usedFromOutside) {
+                PyMoveRefactoringUtil.checkValidImportableFile(e, destination.getVirtualFile());
+              }
+              new PyMoveSymbolProcessor(e, destination, usageInfos, myElements).moveElement();
+            }
+          }
         });
-        if (usedFromOutside) {
-          checkValidImportableFile(e, destination.getVirtualFile());
-        }
-        new PyMoveSymbolProcessor(e, destination, usageInfos, myElements).moveElement();
       }
-    }), REFACTORING_NAME, null);
+    }, REFACTORING_NAME, null);
   }
 
   @NotNull
@@ -138,17 +142,6 @@ public class PyMoveModuleMembersProcessor extends BaseRefactoringProcessor {
     return REFACTORING_NAME;
   }
 
-  /**
-   * Additionally contains referenced element.
-   */
-  private static void checkValidImportableFile(PsiElement anchor, VirtualFile file) {
-    final QualifiedName qName = QualifiedNameFinder.findShortestImportableQName(anchor, file);
-    if (!PyClassRefactoringUtil.isValidQualifiedName(qName)) {
-      throw new IncorrectOperationException(PyBundle.message("refactoring.move.module.members.error.cannot.use.module.name.$0",
-                                                             file.getName()));
-    }
-  }
-
   private static class MyUsageInfo extends UsageInfo {
     private final PsiElement myMovedElement;
     public MyUsageInfo(@NotNull UsageInfo usageInfo, @NotNull PsiElement element) {

diff --git a/python/src/com/jetbrains/python/refactoring/move/PyMoveRefactoringUtil.java b/python/src/com/jetbrains/python/refactoring/move/PyMoveRefactoringUtil.java
new file mode 100644 (file)
index 0000000..04c1265
--- /dev/null
+++ b/python/src/com/jetbrains/python/refactoring/move/PyMoveRefactoringUtil.java
@@ -0,0 +1,47 @@
+/*
+ * Copyright 2000-2015 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 com.jetbrains.python.refactoring.move;
+
+import com.intellij.openapi.vfs.VirtualFile;
+import com.intellij.psi.PsiElement;
+import com.intellij.psi.util.QualifiedName;
+import com.intellij.util.IncorrectOperationException;
+import com.jetbrains.python.PyBundle;
+import com.jetbrains.python.psi.resolve.QualifiedNameFinder;
+import com.jetbrains.python.refactoring.classes.PyClassRefactoringUtil;
+import org.jetbrains.annotations.NotNull;
+
+/**
+ * @author Mikhail Golubev
+ */
+public class PyMoveRefactoringUtil {
+  private PyMoveRefactoringUtil() {
+  }
+
+  /**
+   * Checks that given file has importable name.
+   *
+   * @param anchor arbitrary PSI element to determine project, module, etc.
+   * @param file   virtual file of the module to check
+   * @see QualifiedNameFinder#findShortestImportableName(PsiElement, VirtualFile)
+   */
+  public static void checkValidImportableFile(@NotNull PsiElement anchor, @NotNull VirtualFile file) {
+    final QualifiedName qName = QualifiedNameFinder.findShortestImportableQName(anchor, file);
+    if (!PyClassRefactoringUtil.isValidQualifiedName(qName)) {
+      throw new IncorrectOperationException(PyBundle.message("refactoring.move.error.cannot.use.module.name.$0", file.getName()));
+    }
+  }
+}

diff --git a/python/testData/refactoring/makeFunctionTopLevel/localFunctionNameCollision.py b/python/testData/refactoring/makeFunctionTopLevel/localFunctionNameCollision.py
new file mode 100644 (file)
index 0000000..8485b91
--- /dev/null
+++ b/python/testData/refactoring/makeFunctionTopLevel/localFunctionNameCollision.py
@@ -0,0 +1,7 @@
+def func():
+    def ne<caret>sted():
+        pass
+
+
+def nested():
+    pass
\ No newline at end of file

diff --git a/python/testData/refactoring/makeFunctionTopLevel/methodNotImportableDestinationFile/before/main.py b/python/testData/refactoring/makeFunctionTopLevel/methodNotImportableDestinationFile/before/main.py
new file mode 100644 (file)
index 0000000..c160485
--- /dev/null
+++ b/python/testData/refactoring/makeFunctionTopLevel/methodNotImportableDestinationFile/before/main.py
@@ -0,0 +1,6 @@
+class C:
+    def met<caret>hod(self):
+        pass
+
+
+C().method()

diff --git a/python/testData/refactoring/makeFunctionTopLevel/methodNotImportableDestinationFile/before/not-importable.py b/python/testData/refactoring/makeFunctionTopLevel/methodNotImportableDestinationFile/before/not-importable.py
new file mode 100644 (file)
index 0000000..e69de29

diff --git a/python/testSrc/com/jetbrains/python/refactoring/PyMakeFunctionTopLevelTest.java b/python/testSrc/com/jetbrains/python/refactoring/PyMakeFunctionTopLevelTest.java
index 711cf04a67516b90e2881dff793512ad8260058e..689766d3d2361a6e4cbeb2327c8db727730a2daf 100644 (file)
--- a/python/testSrc/com/jetbrains/python/refactoring/PyMakeFunctionTopLevelTest.java
+++ b/python/testSrc/com/jetbrains/python/refactoring/PyMakeFunctionTopLevelTest.java
@@ -237,6 +237,14 @@ public class PyMakeFunctionTopLevelTest extends PyTestCase {
     doTestSuccess();
   }
 
+  public void testMethodNotImportableDestinationFile() throws IOException {
+    doMultiFileTest("not-importable.py", PyBundle.message("refactoring.move.error.cannot.use.module.name.$0", "not-importable.py"));
+  }
+
+  public void testLocalFunctionNameCollision() {
+    doTestFailure(PyBundle.message("refactoring.move.error.destination.file.contains.function.$0", "nested"));
+  }
+
   @Override
   protected String getTestDataPath() {
     return super.getTestDataPath() + "/refactoring/makeFunctionTopLevel/";