IDEA-CR-45845: new ordering rules for completion and autoimport quickfix (PY-20976)

items defined in the project > items from the standard library > items from other libraries
doesn't start or end with _ > starts with _ > starts with _ > starts and ends with __
items with no leading _ in import path > with leading _ in import path
function/variable/class > module/directory
less items in the import path > more items in the import path

GitOrigin-RevId: 7fba600668d7f7eab4dbd5d3891811cfe2501b89
This commit is contained in:
aleksei.kniazev
2019-08-01 12:04:14 +03:00
committed by intellij-monorepo-bot
parent b7808a2377
commit a4c6f8d8bd
80 changed files with 334 additions and 52 deletions
@@ -90,7 +90,7 @@ public class PyClassNameCompletionContributor extends PyExtendedCompletionContri
if (!sourceQNames.contains(QualifiedName.fromDottedString(clsQName).removeLastComponent())) return le;
return PrioritizedLookupElement.withPriority(le, PythonCompletionWeigher.WEIGHT_DELTA);
return PrioritizedLookupElement.withPriority(le, PythonCompletionWeigher.PRIORITY_WEIGHT);
};
}
@@ -8,16 +8,24 @@ import com.intellij.codeInsight.completion.CompletionParameters
import com.intellij.codeInsight.completion.CompletionResultSet
import com.intellij.codeInsight.lookup.LookupElementBuilder
import com.intellij.codeInsight.lookup.TailTypeDecorator
import com.intellij.openapi.module.ModuleUtilCore
import com.intellij.openapi.projectRoots.Sdk
import com.intellij.openapi.util.io.FileUtil
import com.intellij.openapi.vfs.VirtualFile
import com.intellij.psi.PsiDirectory
import com.intellij.psi.PsiElement
import com.intellij.psi.PsiFile
import com.intellij.psi.PsiFileSystemItem
import com.intellij.psi.util.QualifiedName
import com.jetbrains.python.PyNames
import com.jetbrains.python.codeInsight.dataflow.scope.ScopeUtil
import com.jetbrains.python.codeInsight.imports.PythonImportUtils
import com.jetbrains.python.psi.PyClass
import com.jetbrains.python.psi.PyFile
import com.jetbrains.python.psi.PyFunction
import com.jetbrains.python.psi.resolve.QualifiedNameFinder
import com.jetbrains.python.psi.types.TypeEvalContext
import com.jetbrains.python.sdk.PythonSdkType
import icons.PythonIcons
/**
@@ -97,3 +105,67 @@ fun createLookupElementBuilder(file: PsiFile, element: PsiFileSystemItem): Looku
.withTailText(tailText, true)
.withIcon(element.getIcon(0))
}
private const val ELEMENT_TYPE = 10
private const val LOCATION = 100
private const val PRIVATE_API = 1_000
private const val LOCATION_NOT_YET_IMPORTED = 10_000
private const val UNDERSCORE_IN_NAME = 100_000
const val FALLBACK_WEIGHT = -1_000_000
/**
* Determines weight of suggested completion/import item.
* @param completionLocation file, in which the completion is taking place
* @param nameOnly indicates that we just need to check the name for underscores
*/
fun computeCompletionWeight(element: PsiElement, elementName: String?, path: QualifiedName?, completionLocation: PsiFile?, nameOnly: Boolean): Int {
var weight = 0
val name = elementName ?: return FALLBACK_WEIGHT
weight -= when {
name.startsWith("__") && name.endsWith("__") -> UNDERSCORE_IN_NAME * 3
name.startsWith("__") -> UNDERSCORE_IN_NAME * 2
name.startsWith("_") -> UNDERSCORE_IN_NAME
else -> 0
}
if (nameOnly) return weight
var vFile: VirtualFile? = null
var sdk: Sdk? = null
val containingFile = element.containingFile
if (element is PsiDirectory) {
vFile = element.virtualFile
sdk = PythonSdkType.findPythonSdk(element)
}
else if (containingFile != null) {
vFile = containingFile.virtualFile
sdk = PythonSdkType.findPythonSdk(containingFile)
}
val importPath = path ?: QualifiedNameFinder.findShortestImportableQName(element.containingFile) ?: return FALLBACK_WEIGHT
if (completionLocation != null && !PythonImportUtils.hasImportsFrom(completionLocation, importPath)) {
weight -= LOCATION_NOT_YET_IMPORTED
}
val privatePathComponents = importPath.components.count{ it.startsWith("_") } * PRIVATE_API
weight -= privatePathComponents + importPath.componentCount
if (vFile != null) {
weight -= when {
PythonSdkType.isStdLib(vFile, sdk) -> LOCATION
ModuleUtilCore.findModuleForFile(vFile, element.project) == null -> LOCATION * 2
else -> 0
}
}
weight -= when(element) {
is PsiDirectory -> ELEMENT_TYPE * 2
is PyFile -> ELEMENT_TYPE
else -> 0
}
return weight
}
@@ -19,11 +19,15 @@ import com.intellij.codeInsight.completion.CompletionLocation;
import com.intellij.codeInsight.completion.CompletionWeigher;
import com.intellij.codeInsight.lookup.LookupElement;
import com.intellij.codeInsight.lookup.LookupElementPresentation;
import com.intellij.openapi.diagnostic.Logger;
import com.intellij.psi.PsiElement;
import com.intellij.psi.PsiFile;
import com.intellij.psi.util.PsiUtilCore;
import com.jetbrains.python.PythonLanguage;
import org.jetbrains.annotations.NonNls;
import com.jetbrains.python.psi.PyReferenceExpression;
import org.jetbrains.annotations.NotNull;
/**
* Weighs down items starting with two underscores.
* <br/>
@@ -31,15 +35,13 @@ import org.jetbrains.annotations.NotNull;
*/
public class PythonCompletionWeigher extends CompletionWeigher {
public static final int WEIGHT_DELTA = 5;
@NonNls private static final String SINGLE_UNDER = "_";
@NonNls private static final String DOUBLE_UNDER = "__";
public static final int PRIORITY_WEIGHT = 5;
private static final Logger LOG = Logger.getInstance(PythonCompletionWeigher.class);
@Override
public Comparable weigh(@NotNull final LookupElement element, @NotNull final CompletionLocation location) {
if (!PsiUtilCore.findLanguageFromElement(location.getCompletionParameters().getPosition()).isKindOf(PythonLanguage.getInstance())) {
return 0;
return PyCompletionUtilsKt.FALLBACK_WEIGHT;
}
final String name = element.getLookupString();
@@ -48,13 +50,18 @@ public class PythonCompletionWeigher extends CompletionWeigher {
if ("dict key".equals(presentation.getTypeText())) {
return element.getLookupString().length();
}
if (name.startsWith(DOUBLE_UNDER)) {
if (name.endsWith(DOUBLE_UNDER)) return -4 * WEIGHT_DELTA; // __foo__ is lowest
else return -2 * WEIGHT_DELTA; // __foo is lower than normal
PsiElement psiElement = element.getPsiElement();
PsiFile file = location.getCompletionParameters().getOriginalFile();
if (psiElement != null) {
if (psiElement.getContainingFile() == file) return PRIORITY_WEIGHT;
PsiElement dummyParent = location.getCompletionParameters().getPosition().getParent();
boolean isQualified = dummyParent instanceof PyReferenceExpression && ((PyReferenceExpression)dummyParent).isQualified();
int completionWeight = PyCompletionUtilsKt.computeCompletionWeight(psiElement, name, null, file, isQualified);
LOG.debug("Combined weight for completion item ", name, ": ", completionWeight);
return completionWeight;
}
if (name.startsWith(SINGLE_UNDER)) {
return -1 * WEIGHT_DELTA;
}
return 0; // default
return PyCompletionUtilsKt.FALLBACK_WEIGHT;
}
}
@@ -1,20 +1,17 @@
// Copyright 2000-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE file.
package com.jetbrains.python.codeInsight.imports;
import com.intellij.openapi.module.Module;
import com.intellij.openapi.project.Project;
import com.intellij.openapi.roots.ProjectFileIndex;
import com.intellij.openapi.roots.ProjectRootManager;
import com.intellij.openapi.util.Comparing;
import com.intellij.openapi.diagnostic.Logger;
import com.intellij.openapi.util.text.StringUtil;
import com.intellij.openapi.vfs.VirtualFile;
import com.intellij.psi.*;
import com.intellij.psi.util.QualifiedName;
import com.intellij.util.containers.ContainerUtil;
import com.jetbrains.python.codeInsight.completion.PyCompletionUtilsKt;
import com.jetbrains.python.psi.*;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import java.util.Comparator;
import java.util.List;
/**
@@ -31,11 +28,13 @@ import java.util.List;
*/
// visibility is intentionally package-level
public class ImportCandidateHolder implements Comparable<ImportCandidateHolder> {
private static final Logger LOG = Logger.getInstance(ImportCandidateHolder.class);
@NotNull private final SmartPsiElementPointer<PsiElement> myImportable;
@Nullable private final SmartPsiElementPointer<PyImportElement> myImportElement;
@NotNull private final SmartPsiElementPointer<PsiFileSystemItem> myFile;
@Nullable private final QualifiedName myPath;
@Nullable private final String myAsName;
private final int myRelevance;
/**
* Creates new instance.
@@ -57,6 +56,9 @@ public class ImportCandidateHolder implements Comparable<ImportCandidateHolder>
myImportElement = importElement != null ? pointerManager.createSmartPsiElementPointer(importElement) : null;
myPath = path;
myAsName = asName;
String name = importable instanceof PsiNamedElement ? ((PsiNamedElement)importable).getName() : null;
myRelevance = PyCompletionUtilsKt.computeCompletionWeight(importable, name, path, null, false);
LOG.debug("Computed relevance for import item ", name, ": ", myRelevance);
assert importElement != null || path != null; // one of these must be present
}
@@ -150,36 +152,16 @@ public class ImportCandidateHolder implements Comparable<ImportCandidateHolder>
@Override
public int compareTo(@NotNull ImportCandidateHolder other) {
final int lRelevance = getRelevance();
final int rRelevance = other.getRelevance();
if (rRelevance != lRelevance) {
return rRelevance - lRelevance;
}
if (myPath != null && other.myPath != null) {
// prefer shorter paths
final int lengthDiff = myPath.getComponentCount() - other.myPath.getComponentCount();
if (lengthDiff != 0) {
return lengthDiff;
}
}
return Comparing.compare(myPath, other.myPath);
if (myImportElement != null && other.myImportElement == null) return -1;
if (myImportElement == null && other.myImportElement != null) return 1;
return Comparator
.comparing(ImportCandidateHolder::getRelevance).reversed()
.thenComparing(ImportCandidateHolder::getPath)
.compare(this, other);
}
int getRelevance() {
if (myImportElement != null) return 4;
final Project project = myImportable.getProject();
final PsiFile psiFile = myImportable.getContainingFile();
final VirtualFile vFile = psiFile == null ? null : psiFile.getVirtualFile();
if (vFile == null) return 0;
final ProjectFileIndex fileIndex = ProjectRootManager.getInstance(project).getFileIndex();
// files under project source are most relevant
final Module module = fileIndex.getModuleForFile(vFile);
if (module != null) return 3;
// then come files directly under Lib
if (vFile.getParent().getName().equals("Lib")) return 2;
// tests we don't want
if (vFile.getParent().getName().equals("test")) return 0;
return 1;
public int getRelevance() {
return myRelevance;
}
@Nullable
@@ -27,6 +27,7 @@ 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.sdk.PythonSdkType;
import one.util.streamex.StreamEx;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
@@ -274,4 +275,14 @@ public final class PythonImportUtils {
}
return PsiTreeUtil.getParentOfType(ref_element, PyStringLiteralExpression.class, false, PyStatement.class) == null;
}
public static boolean hasImportsFrom(@NotNull PsiFile file, @NotNull QualifiedName qName) {
if (file instanceof PyFile) {
PyFile pyFile = (PyFile)file;
return StreamEx.of(pyFile.getFromImports()).map(PyFromImportStatement::getImportSourceQName)
.nonNull()
.anyMatch(qName::equals);
}
return false;
}
}
@@ -118,7 +118,7 @@ public class CompletionVariantsProcessor extends VariantsProcessor {
if (parent instanceof PyKeywordArgument) {
final String keyword = ((PyKeywordArgument)parent).getKeyword();
if (item.getLookupString().equals(keyword)) {
return PrioritizedLookupElement.withPriority(item, PythonCompletionWeigher.WEIGHT_DELTA);
return PrioritizedLookupElement.withPriority(item, PythonCompletionWeigher.PRIORITY_WEIGHT);
}
}
@@ -0,0 +1 @@
from bar import path
@@ -0,0 +1 @@
path = "something"
@@ -0,0 +1 @@
from foo import path
@@ -0,0 +1,2 @@
path = "something"
@@ -0,0 +1,3 @@
path = "something"
pat<caret>
@@ -1,3 +1,3 @@
import exceptions
import collections
exceptions
collections
@@ -1 +1 @@
exc<caret>
collec<caret>
@@ -0,0 +1,2 @@
foo = 1
bar = 2
@@ -0,0 +1,3 @@
from b import bar
fo<caret>o
@@ -0,0 +1 @@
fo<caret>o
@@ -0,0 +1 @@
pa<caret>th
@@ -0,0 +1 @@
fo<caret>o
@@ -0,0 +1 @@
fo<caret>o
@@ -0,0 +1 @@
fo<caret>o
@@ -0,0 +1,2 @@
def __foo__():
return "private"
@@ -0,0 +1,2 @@
def _foo():
return "private"
@@ -0,0 +1,2 @@
def foo():
return "public"
@@ -0,0 +1 @@
fo<caret>o
@@ -0,0 +1 @@
foo = "private"
@@ -0,0 +1 @@
foo = "non-private"
@@ -0,0 +1 @@
fo<caret>o
@@ -0,0 +1 @@
from foo import path
@@ -0,0 +1 @@
path = "something"
@@ -0,0 +1,4 @@
<warning descr="Unused import statement">from sys import argv</warning>
<error descr="Unresolved reference 'path'">pa<caret>th</error>
@@ -0,0 +1 @@
<error descr="Unresolved reference 'path'">pa<caret>th</error>
@@ -0,0 +1 @@
<error descr="Unresolved reference 'foo'">fo<caret>o</error>
@@ -0,0 +1 @@
<error descr="Unresolved reference 'foo'">fo<caret>o</error>
@@ -0,0 +1,2 @@
def foo(): # function takes precedence over module
pass
@@ -0,0 +1 @@
<error descr="Unresolved reference 'foo'">fo<caret>o</error>
@@ -0,0 +1 @@
path = "not as deep, but from private package"
@@ -0,0 +1 @@
path = "deep, but not private"
@@ -0,0 +1 @@
<error descr="Unresolved reference 'path'">pa<caret>th</error>
@@ -0,0 +1,4 @@
<warning descr="Unused import statement">from sys import argv</warning>
<error descr="Unresolved reference 'path'">pa<caret>th</error>
@@ -0,0 +1 @@
path = "local value, so it should take precedence if there is no import"
@@ -3,9 +3,21 @@ package com.jetbrains.python;
import com.intellij.codeInsight.completion.CompletionType;
import com.intellij.codeInsight.lookup.Lookup;
import com.intellij.codeInsight.lookup.LookupElement;
import com.intellij.psi.PsiDirectory;
import com.intellij.psi.PsiElement;
import com.intellij.psi.PsiFileSystemItem;
import com.intellij.psi.codeStyle.CommonCodeStyleSettings;
import com.jetbrains.python.codeInsight.PyCodeInsightSettings;
import com.jetbrains.python.fixtures.PyTestCase;
import com.jetbrains.python.psi.PyQualifiedNameOwner;
import com.jetbrains.python.psi.resolve.QualifiedNameFinder;
import one.util.streamex.StreamEx;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import java.util.List;
import java.util.Objects;
/**
* @author yole
@@ -84,6 +96,51 @@ public class PyClassNameCompletionTest extends PyTestCase {
doTest();
}
// PY-20976
public void testOrderingLexicographicalBaseline() {
doTestCompletionOrder("a.foo", "b.foo");
}
// PY-20976
public void testOrderingLocalBeforeStdlib() {
doTestCompletionOrder("local_pkg.path", "local_pkg.local_module.path", "sys.path");
}
// PY-20976
public void testOrderingUnderscoreInPath() {
doTestCompletionOrder("b.foo", "_a.foo");
}
// PY-20976
public void testOrderingSymbolBeforeModule() {
doTestCompletionOrder("b.foo", "a.foo");
}
// PY-20976
public void testOrderingModuleBeforePackage() {
doTestCompletionOrder("b.foo", "a.foo");
}
// PY-20976
public void testOrderingFileHasImportFromSameFile() {
doTestCompletionOrder("b.foo", "a.foo");
}
// PY-20976
public void testOrderingPathComponentsNumber() {
doTestCompletionOrder("c.foo", "b.c.foo", "a.b.c.foo");
}
// PY-20976
public void testCombinedOrdering() {
doTestCompletionOrder("main.path", "first.foo.path", "sys.path", "_second.bar.path");
}
// PY-20976
public void testOrderingUnderscoreInName() {
doTestCompletionOrder("c.foo", "b._foo", "a.__foo__");
}
private void doTest() {
final String path = "/completion/className/" + getTestName(true);
myFixture.copyDirectoryToProject(path, "");
@@ -94,4 +151,30 @@ public class PyClassNameCompletionTest extends PyTestCase {
}
myFixture.checkResultByFile(path + "/" + getTestName(true) + ".after.py", true);
}
private void doTestCompletionOrder(@NotNull String... expected) {
myFixture.copyDirectoryToProject("/completion/className/" + getTestName(true), "");
myFixture.configureByFile("main.py");
myFixture.complete(CompletionType.BASIC, 2);
List<String> qNames = StreamEx.of(myFixture.getLookupElements())
.map(LookupElement::getPsiElement)
.nonNull()
.map(PyClassNameCompletionTest::extractQualifiedName)
.toList();
assertContainsInRelativeOrder(qNames, expected);
}
@Nullable
private static String extractQualifiedName(@NotNull PsiElement element) {
if (element instanceof PyQualifiedNameOwner) {
return ((PyQualifiedNameOwner)element).getQualifiedName();
}
else {
final PsiFileSystemItem item;
if (element instanceof PsiDirectory) item = (PsiDirectory)element;
else item = element.getContainingFile();
return Objects.toString(QualifiedNameFinder.findShortestImportableQName(item));
}
}
}
@@ -2,6 +2,7 @@
package com.jetbrains.python.fixtures;
import com.google.common.base.Joiner;
import com.google.common.collect.Lists;
import com.intellij.application.options.CodeStyle;
import com.intellij.codeInsight.lookup.LookupElement;
import com.intellij.codeInsight.lookup.LookupEx;
@@ -509,5 +510,21 @@ public abstract class PyTestCase extends UsefulTestCase {
Disposer.register(myFixture.getProjectDisposable(), () -> PsiTestUtil.removeExcludedRoot(module, dir));
}
public <T> void assertContainsInRelativeOrder(@NotNull final Iterable<T> actual, @Nullable final T... expected) {
final List<T> actualList = Lists.newArrayList(actual);
if (expected.length > 0) {
T prev = expected[0];
int prevIndex = actualList.indexOf(prev);
assertTrue(prevIndex >= 0);
for (int i = 1; i < expected.length; i++) {
final T next = expected[i];
final int nextIndex = actualList.indexOf(next);
assertTrue(next + " is not found in " + actualList, nextIndex >= 0);
assertTrue(prev + " should precede " + next + " in " + actualList, prevIndex < nextIndex);
prev = next;
prevIndex = nextIndex;
}
}
}
}
@@ -135,6 +135,50 @@ public class PyAddImportQuickFixTest extends PyQuickFixTestCase {
doMultiFileAutoImportTest("Import 'mod.bar()'");
}
// PY-20976
public void testCombinedElementOrdering() {
doTestProposedImportsOrdering("path", "path from sys", "first.path", "first.second.path()", "os.path", "first._third.path");
}
// PY-20976
public void testOrderingLocalBeforeStdlib() {
doTestProposedImportsOrdering("path", "pkg.path", "sys.path", "os.path");
}
// PY-20976
public void testOrderingUnderscoreInPath() {
doTestProposedImportsOrdering("path", "first.second.path", "sys.path", "os.path", "_private.path");
}
// PY-20976
public void testOrderingSymbolBeforeModule() {
doTestProposedImportsOrdering("foo", "first.module.foo()", "first.a.foo");
}
// PY-20976
public void testOrderingModuleBeforePackage() {
doTestProposedImportsOrdering("foo", "b.foo", "a.foo");
}
// PY-20976
public void testOrderingPathComponentsNumber() {
doTestProposedImportsOrdering("foo", "c.foo", "b.c.foo", "a.b.c.foo");
}
// PY-20976
public void testOrderingWithExistingImport() {
doTestProposedImportsOrdering("path", "path from sys", "src.path", "os.path");
}
private void doTestProposedImportsOrdering(@NotNull String text, @NotNull String... expected) {
doMultiFileAutoImportTest("Import", fix -> {
final List<String> candidates = ContainerUtil.map(fix.getCandidates(), c -> c.getPresentableText(text));
assertNotNull(candidates);
assertContainsInRelativeOrder(candidates, expected);
return false;
});
}
private void doMultiFileAutoImportTest(@NotNull String hintPrefix) {
doMultiFileAutoImportTest(hintPrefix, null);
}