PY-19773 Remember already suggested importable elements using their canonical names
authorMikhail Golubev <mikhail.golubev@jetbrains.com>
Wed, 29 Jun 2016 21:44:18 +0000 (00:44 +0300)
committerMikhail Golubev <mikhail.golubev@jetbrains.com>
Thu, 30 Jun 2016 05:50:59 +0000 (08:50 +0300)
The problem was caused by the fact that we ignore imported element
"request" in "flask/__init__.py" at first and mark this module as
already checked (using the name "flask") and then refuse to suggest
"flask.globals.requests" because its canonical qualified name is
"flask.request".

I've also added a test on importing functions of os.path module since
it's a well-known case where several different symbols should be imported
using the same name (provided by PyStdlibCanonicalPathProvider).
My first attempt to fix the problem by saving actual import candidates
instead of their canonical qualified names has broken exactly this
scenario.

13 files changed:
python/src/com/jetbrains/python/codeInsight/imports/AutoImportQuickFix.java
python/src/com/jetbrains/python/codeInsight/imports/ImportCandidateHolder.java
python/src/com/jetbrains/python/codeInsight/imports/PythonImportUtils.java
python/testData/quickFixes/AddImportQuickFixTest/osPathFunctions/main.py [new file with mode: 0644]
python/testData/quickFixes/AddImportQuickFixTest/osPathFunctions/main_after.py [new file with mode: 0644]
python/testData/quickFixes/AddImportQuickFixTest/osPathFunctions/ntpath.py [new file with mode: 0644]
python/testData/quickFixes/AddImportQuickFixTest/osPathFunctions/os.py [new file with mode: 0644]
python/testData/quickFixes/AddImportQuickFixTest/osPathFunctions/posixpath.py [new file with mode: 0644]
python/testData/quickFixes/AddImportQuickFixTest/reexportedName/flask/__init__.py [new file with mode: 0644]
python/testData/quickFixes/AddImportQuickFixTest/reexportedName/flask/globals.py [new file with mode: 0644]
python/testData/quickFixes/AddImportQuickFixTest/reexportedName/main.py [new file with mode: 0644]
python/testData/quickFixes/AddImportQuickFixTest/reexportedName/main_after.py [new file with mode: 0644]
python/testSrc/com/jetbrains/python/quickFixes/AddImportQuickFixTest.java [new file with mode: 0644]

index a4d8b3fd1206293d77f831122e5ca886ef00a807..af2f5f8cdaecbaf5fb572348374405ce7d699ae9 100644 (file)
@@ -200,8 +200,9 @@ public class AutoImportQuickFix extends LocalQuickFixOnPsiElement implements Hig
     Collections.sort(myImports);
   }
 
-  public int getCandidatesCount() {
-    return myImports.size();
+  @NotNull
+  public List<ImportCandidateHolder> getCandidates() {
+    return Collections.unmodifiableList(myImports);
   }
 
   public boolean hasOnlyFunctions() {
index 7a89e911db52371d3b16320956f32f9f767bdef6..c60178052609244b845dcd33628cb8690669835b 100644 (file)
@@ -46,7 +46,7 @@ import java.util.List;
  * @author dcheryasov
  */
 // visibility is intentionally package-level
-class ImportCandidateHolder implements Comparable<ImportCandidateHolder> {
+public class ImportCandidateHolder implements Comparable<ImportCandidateHolder> {
   private final PsiElement myImportable;
   private final PyImportElement myImportElement;
   private final PsiFileSystemItem myFile;
index be95a74f338a9ca271272961c6c4a089eeccf5e9..92fa177b2cd0a50cc07dd7ac39f9bd5664656d66 100644 (file)
@@ -88,19 +88,19 @@ public final class PythonImportUtils {
   private static AutoImportQuickFix addCandidates(PyElement node, PsiReference reference, String refText, @Nullable String asName) {
     final boolean qualify = !PyCodeInsightSettings.getInstance().PREFER_FROM_IMPORT;
     AutoImportQuickFix fix = new AutoImportQuickFix(node, reference.getClass(), refText, qualify);
-    Set<String> seenFileNames = new HashSet<String>(); // true import names
+    Set<String> seenCandidateNames = new HashSet<>(); // true import names
 
-    PsiFile existingImportFile = addCandidatesFromExistingImports(node, refText, fix, seenFileNames);
-    if (fix.getCandidatesCount() == 0 || fix.hasProjectImports() || Registry.is("python.import.always.ask")) {
+    PsiFile existingImportFile = addCandidatesFromExistingImports(node, refText, fix, seenCandidateNames);
+    if (fix.getCandidates().isEmpty()|| fix.hasProjectImports() || Registry.is("python.import.always.ask")) {
       // maybe some unimported file has it, too
       ProgressManager.checkCanceled(); // before expensive index searches
-      addSymbolImportCandidates(node, refText, asName, fix, seenFileNames, existingImportFile);
+      addSymbolImportCandidates(node, refText, asName, fix, seenCandidateNames, existingImportFile);
     }
 
     for(PyImportCandidateProvider provider: Extensions.getExtensions(PyImportCandidateProvider.EP_NAME)) {
       provider.addImportCandidates(reference, refText, fix);
     }
-    if (fix.getCandidatesCount() > 0) {
+    if (!fix.getCandidates().isEmpty()) {
       fix.sortCandidates();
       return fix;
     }
@@ -112,27 +112,21 @@ public final class PythonImportUtils {
    * collect all such statements and analyze.
    * NOTE: It only makes sense to look at imports in file scope - there is no guarantee that an import in a local scope will
    * be visible from the scope where the auto-import was invoked
-   *
-   * @param node
-   * @param refText
-   * @param fix
-   * @param seenFileNames
-   * @return
    */
   @Nullable
   private static PsiFile addCandidatesFromExistingImports(PyElement node, String refText, AutoImportQuickFix fix,
-                                                          Set<String> seenFileNames) {
-    PsiFile existingImportFile = null; // if there's a matching existing import, this it the file it imports
+                                                          Set<String> seenCandidateNames) {
+    PsiFile existingImportFile = null; // if there's a matching existing import, this is the file it imports
     PsiFile file = node.getContainingFile();
     if (file instanceof PyFile) {
       PyFile pyFile = (PyFile)file;
       for (PyImportElement importElement : pyFile.getImportTargets()) {
-        existingImportFile = addImportViaElement(refText, fix, seenFileNames, existingImportFile, importElement, importElement.resolve());
+        existingImportFile = addImportViaElement(refText, fix, seenCandidateNames, existingImportFile, importElement, importElement.resolve());
       }
       for (PyFromImportStatement fromImportStatement : pyFile.getFromImports()) {
         if (!fromImportStatement.isStarImport() && fromImportStatement.getImportElements().length > 0) {
           PsiElement source = fromImportStatement.resolveImportSource();
-          existingImportFile = addImportViaElement(refText, fix, seenFileNames, existingImportFile, fromImportStatement.getImportElements()[0], source);
+          existingImportFile = addImportViaElement(refText, fix, seenCandidateNames, existingImportFile, fromImportStatement.getImportElements()[0], source);
         }
       }
     }
@@ -141,46 +135,33 @@ public final class PythonImportUtils {
 
   private static PsiFile addImportViaElement(String refText,
                                              AutoImportQuickFix fix,
-                                             Set<String> seenFileNames,
+                                             Set<String> seenCandidateNames,
                                              PsiFile existingImportFile,
                                              PyImportElement importElement,
                                              PsiElement source) {
     PyFile sourceFile = as(PyUtil.turnDirIntoInit(source), PyFile.class);
     if (sourceFile instanceof PyFileImpl) {
-      PyStatement importStatement = importElement.getContainingImportStatement();
-      String refName = null;
-      if (importStatement instanceof PyFromImportStatement) {
-        QualifiedName qName = ((PyFromImportStatement)importStatement).getImportSourceQName();
-        if (qName != null) {
-          refName = qName.toString();
-        }
-      }
-      else {
-        QualifiedName importReferenceQName = importElement.getImportedQName();
-        if (importReferenceQName != null) {
-          refName = importReferenceQName.toString();
-        }
-      }
-      if (refName != null) {
-        if (seenFileNames.contains(refName)) {
-          return existingImportFile;
-        }
-        seenFileNames.add(refName);
-      }
 
       PsiElement res = sourceFile.findExportedName(refText);
+      final String name = res instanceof PyQualifiedNameOwner ? ((PyQualifiedNameOwner)res).getQualifiedName() : null;
+      if (name != null && seenCandidateNames.contains(name)) {
+        return existingImportFile;
+      }
       // allow importing from this source if it either declares the name itself or represents a higher-level package that reexports the name
       if (res != null && !(res instanceof PyFile) && !(res instanceof PyImportElement) && res.getContainingFile() != null &&
           PsiTreeUtil.isAncestor(source, res.getContainingFile(), false)) {
         existingImportFile = sourceFile;
         fix.addImport(res, sourceFile, importElement);
+        if (name != null) {
+          seenCandidateNames.add(name);
+        }
       }
     }
     return existingImportFile;
   }
 
   private static void addSymbolImportCandidates(PyElement node, String refText, @Nullable String asName, AutoImportQuickFix fix,
-                                                Set<String> seenFileNames, PsiFile existingImportFile) {
+                                                Set<String> seenCandidateNames, PsiFile existingImportFile) {
     Project project = node.getProject();
     List<PsiElement> symbols = new ArrayList<PsiElement>();
     symbols.addAll(PyClassNameIndex.find(refText, project, true));
@@ -198,13 +179,17 @@ public final class PythonImportUtils {
           PsiFileSystemItem srcfile = symbol instanceof PsiFileSystemItem ? ((PsiFileSystemItem)symbol).getParent() : symbol.getContainingFile();
           if (srcfile != null && isAcceptableForImport(node, existingImportFile, srcfile)) {
             QualifiedName importPath = QualifiedNameFinder.findCanonicalImportPath(symbol, node);
-            if (symbol instanceof PsiFileSystemItem && importPath != null) {
+            if (importPath == null) {
+              continue;
+            }
+            if (symbol instanceof PsiFileSystemItem) {
               importPath = importPath.removeTail(1);
             }
-            if (importPath != null && !seenFileNames.contains(importPath.toString())) {
+            final String symbolImportQName = importPath.append(refText).toString();
+            if (!seenCandidateNames.contains(symbolImportQName)) {
               // a new, valid hit
               fix.addImport(symbol, srcfile, importPath, asName);
-              seenFileNames.add(importPath.toString()); // just in case, again
+              seenCandidateNames.add(symbolImportQName);
             }
           }
         }
diff --git a/python/testData/quickFixes/AddImportQuickFixTest/osPathFunctions/main.py b/python/testData/quickFixes/AddImportQuickFixTest/osPathFunctions/main.py
new file mode 100644 (file)
index 0000000..d7519db
--- /dev/null
@@ -0,0 +1 @@
+<error descr="Unresolved reference 'join'">j<caret>oin</error>
\ No newline at end of file
diff --git a/python/testData/quickFixes/AddImportQuickFixTest/osPathFunctions/main_after.py b/python/testData/quickFixes/AddImportQuickFixTest/osPathFunctions/main_after.py
new file mode 100644 (file)
index 0000000..0f5e4be
--- /dev/null
@@ -0,0 +1,3 @@
+from os.path import join
+
+join
\ No newline at end of file
diff --git a/python/testData/quickFixes/AddImportQuickFixTest/osPathFunctions/ntpath.py b/python/testData/quickFixes/AddImportQuickFixTest/osPathFunctions/ntpath.py
new file mode 100644 (file)
index 0000000..ad48541
--- /dev/null
@@ -0,0 +1,2 @@
+def join(*args):
+    pass
\ No newline at end of file
diff --git a/python/testData/quickFixes/AddImportQuickFixTest/osPathFunctions/os.py b/python/testData/quickFixes/AddImportQuickFixTest/osPathFunctions/os.py
new file mode 100644 (file)
index 0000000..d87b27c
--- /dev/null
@@ -0,0 +1,4 @@
+if windows():
+    import ntpath as path
+else:
+    import posixpath as path
diff --git a/python/testData/quickFixes/AddImportQuickFixTest/osPathFunctions/posixpath.py b/python/testData/quickFixes/AddImportQuickFixTest/osPathFunctions/posixpath.py
new file mode 100644 (file)
index 0000000..947e683
--- /dev/null
@@ -0,0 +1,2 @@
+def join(*args):
+    pass
diff --git a/python/testData/quickFixes/AddImportQuickFixTest/reexportedName/flask/__init__.py b/python/testData/quickFixes/AddImportQuickFixTest/reexportedName/flask/__init__.py
new file mode 100644 (file)
index 0000000..660573b
--- /dev/null
@@ -0,0 +1,6 @@
+from .globals import request
+
+
+class Flask:
+    def __init__(self, *args, **kwargs):
+        pass
diff --git a/python/testData/quickFixes/AddImportQuickFixTest/reexportedName/flask/globals.py b/python/testData/quickFixes/AddImportQuickFixTest/reexportedName/flask/globals.py
new file mode 100644 (file)
index 0000000..a8b8cf7
--- /dev/null
@@ -0,0 +1 @@
+request = object()
\ No newline at end of file
diff --git a/python/testData/quickFixes/AddImportQuickFixTest/reexportedName/main.py b/python/testData/quickFixes/AddImportQuickFixTest/reexportedName/main.py
new file mode 100644 (file)
index 0000000..e3aa6d5
--- /dev/null
@@ -0,0 +1,5 @@
+from flask import Flask
+
+app = Flask(__name__)
+
+<error descr="Unresolved reference 'request'"><caret>request</error>
\ No newline at end of file
diff --git a/python/testData/quickFixes/AddImportQuickFixTest/reexportedName/main_after.py b/python/testData/quickFixes/AddImportQuickFixTest/reexportedName/main_after.py
new file mode 100644 (file)
index 0000000..ea85c02
--- /dev/null
@@ -0,0 +1,6 @@
+from flask import Flask
+from flask import request
+
+app = Flask(__name__)
+
+request
\ No newline at end of file
diff --git a/python/testSrc/com/jetbrains/python/quickFixes/AddImportQuickFixTest.java b/python/testSrc/com/jetbrains/python/quickFixes/AddImportQuickFixTest.java
new file mode 100644 (file)
index 0000000..969b7ce
--- /dev/null
@@ -0,0 +1,71 @@
+/*
+ * Copyright 2000-2016 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.quickFixes;
+
+import com.intellij.codeInsight.intention.IntentionAction;
+import com.intellij.codeInspection.ex.QuickFixWrapper;
+import com.intellij.util.containers.ContainerUtil;
+import com.jetbrains.python.PyQuickFixTestCase;
+import com.jetbrains.python.codeInsight.imports.AutoImportQuickFix;
+import com.jetbrains.python.codeInsight.imports.ImportCandidateHolder;
+import com.jetbrains.python.inspections.unresolvedReference.PyUnresolvedReferencesInspection;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+
+import java.util.List;
+import java.util.function.Consumer;
+
+/**
+ * @author Mikhail Golubev
+ */
+public class AddImportQuickFixTest extends PyQuickFixTestCase {
+
+  // PY-19773
+  public void testReexportedName() throws Exception {
+    doMultiFileAutoImportTest("Import 'flask.request'");
+  }
+
+  public void testOsPathFunctions() throws Exception {
+    doMultiFileAutoImportTest("Import", fix -> {
+      final List<ImportCandidateHolder> candidates = fix.getCandidates();
+      final List<String> names = ContainerUtil.map(candidates, c -> c.getPresentableText("join"));
+      assertSameElements(names, "os.path.join(path, *paths)");
+    });
+  }
+
+  private void doMultiFileAutoImportTest(@NotNull String hintPrefix) {
+    doMultiFileAutoImportTest(hintPrefix, null);
+  }
+
+  private void doMultiFileAutoImportTest(@NotNull String hintPrefix, @Nullable Consumer<AutoImportQuickFix> checkQuickfix) {
+    myFixture.copyDirectoryToProject(getTestName(true), "");
+    myFixture.enableInspections(PyUnresolvedReferencesInspection.class);
+    final String entryPoint = "main";
+    myFixture.configureByFile(entryPoint + ".py");
+    myFixture.checkHighlighting(true, false, false);
+    final List<IntentionAction> intentions = myFixture.filterAvailableIntentions(hintPrefix);
+    final IntentionAction intention = ContainerUtil.find(intentions, action -> {
+      return action instanceof QuickFixWrapper && ((QuickFixWrapper)action).getFix() instanceof AutoImportQuickFix;
+    });
+    assertNotNull("Auto import quick fix starting with '" + hintPrefix + "' wasn't found", intention);
+    final AutoImportQuickFix quickfix = (AutoImportQuickFix)((QuickFixWrapper)intention).getFix();
+    if (checkQuickfix != null) {
+      checkQuickfix.accept(quickfix);
+    }
+    myFixture.launchAction(intention);
+    myFixture.checkResultByFile(getTestName(true) + "/" + entryPoint + "_after.py", true);
+  }
+}