From 60da953814001fa33be6ed75077b9ea895bd99a5 Mon Sep 17 00:00:00 2001 From: Andrey Vlasovskikh Date: Tue, 27 Oct 2020 22:09:19 +0300 Subject: [PATCH] PY-45206 Resolve instance attributes found in the same scope using control flow We have to define a resolve order so that the attribute is never defined in terms of itself. The main result is preventing recursion in the type checker in case of an infinite loop. It used to cause unstable type inference based on the order in which we process elements. A side effect of this change is a more precise behaviour of resolve for attributes where they are clearly not defined yet like: class C: def f(self): print(self.foo) # foo is now unresolved self.foo = 0 See more examples in the unit tests for this fix. GitOrigin-RevId: a791ed9a16df64935bf70e2933428c0950e5e7d3 --- .../python/psi/impl/PyClassImpl.java | 8 +- .../impl/references/PyQualifiedReference.java | 2 +- .../psi/resolve/PyResolveProcessor.java | 22 +++--- .../python/psi/types/PyClassTypeImpl.java | 76 ++++++++++++++++++- python/testData/resolve/InstanceAttrAbove.py | 5 ++ .../InstanceAttrBelowEarlierByControlFlow.py | 10 +++ .../InstanceAttrBothEarlierAndLater.py | 6 ++ .../resolve/InstanceAttrInheritedAndAbove.py | 10 +++ .../resolve/InstanceAttrInheritedAndBelow.py | 11 +++ .../resolve/InstanceAttrOtherMethod.py | 7 ++ .../InstanceAttrOtherMethodAndAbove.py | 8 ++ .../InstanceAttrOtherMethodAndBelow.py | 9 +++ .../resolve/NoResolveInstanceAttrBelow.py | 5 ++ .../resolve/NoResolveInstanceAttrSameLine.py | 4 + .../com/jetbrains/python/Py3ResolveTest.java | 55 ++++++++++++++ .../com/jetbrains/python/Py3TypeTest.java | 35 +++++++++ .../com/jetbrains/python/PyTypeTest.java | 2 +- 17 files changed, 258 insertions(+), 17 deletions(-) create mode 100644 python/testData/resolve/InstanceAttrAbove.py create mode 100644 python/testData/resolve/InstanceAttrBelowEarlierByControlFlow.py create mode 100644 python/testData/resolve/InstanceAttrBothEarlierAndLater.py create mode 100644 python/testData/resolve/InstanceAttrInheritedAndAbove.py create mode 100644 python/testData/resolve/InstanceAttrInheritedAndBelow.py create mode 100644 python/testData/resolve/InstanceAttrOtherMethod.py create mode 100644 python/testData/resolve/InstanceAttrOtherMethodAndAbove.py create mode 100644 python/testData/resolve/InstanceAttrOtherMethodAndBelow.py create mode 100644 python/testData/resolve/NoResolveInstanceAttrBelow.py create mode 100644 python/testData/resolve/NoResolveInstanceAttrSameLine.py diff --git a/python/python-psi-impl/src/com/jetbrains/python/psi/impl/PyClassImpl.java b/python/python-psi-impl/src/com/jetbrains/python/psi/impl/PyClassImpl.java index 4b98a7e53a55..f351aa557081 100644 --- a/python/python-psi-impl/src/com/jetbrains/python/psi/impl/PyClassImpl.java +++ b/python/python-psi-impl/src/com/jetbrains/python/psi/impl/PyClassImpl.java @@ -1245,19 +1245,17 @@ public class PyClassImpl extends PyBaseElementImpl implements PyCla @Override public boolean processInstanceLevelDeclarations(@NotNull PsiScopeProcessor processor, @Nullable PsiElement location) { - final Map declarationsInMethod = new HashMap<>(); final PyFunction instanceMethod = PsiTreeUtil.getStubOrPsiParentOfType(location, PyFunction.class); final PyClass containingClass = instanceMethod != null ? instanceMethod.getContainingClass() : null; if (instanceMethod != null && containingClass != null && CompletionUtilCoreImpl.getOriginalElement(containingClass) == this) { - collectInstanceAttributes(instanceMethod, declarationsInMethod); - for (PyTargetExpression targetExpression : declarationsInMethod.values()) { - if (!processor.execute(targetExpression, ResolveState.initial())) { + for (PyTargetExpression target : getTargetExpressions(instanceMethod)) { + if (PyUtil.isInstanceAttribute(target) && !processor.execute(target, ResolveState.initial())) { return false; } } } for (PyTargetExpression expr : getInstanceAttributes()) { - if (declarationsInMethod.containsKey(expr.getName())) { + if (instanceMethod != null && ScopeUtil.getScopeOwner(expr) == instanceMethod) { continue; } if (!processor.execute(expr, ResolveState.initial())) return false; diff --git a/python/python-psi-impl/src/com/jetbrains/python/psi/impl/references/PyQualifiedReference.java b/python/python-psi-impl/src/com/jetbrains/python/psi/impl/references/PyQualifiedReference.java index b14361c6d356..e223ce150423 100644 --- a/python/python-psi-impl/src/com/jetbrains/python/psi/impl/references/PyQualifiedReference.java +++ b/python/python-psi-impl/src/com/jetbrains/python/psi/impl/references/PyQualifiedReference.java @@ -137,7 +137,7 @@ public class PyQualifiedReference extends PyReferenceImpl { return false; } for (PyExpression ex : collectAssignedAttributes(qName, qualifier)) { - if (referencedName.equals(ex.getName())) { + if (referencedName.equals(ex.getName()) && !PyUtil.isInstanceAttribute(ex)) { ret.poke(ex, RatedResolveResult.RATE_NORMAL); return true; } diff --git a/python/python-psi-impl/src/com/jetbrains/python/psi/resolve/PyResolveProcessor.java b/python/python-psi-impl/src/com/jetbrains/python/psi/resolve/PyResolveProcessor.java index cba85a0b8fce..dc9bb172251d 100644 --- a/python/python-psi-impl/src/com/jetbrains/python/psi/resolve/PyResolveProcessor.java +++ b/python/python-psi-impl/src/com/jetbrains/python/psi/resolve/PyResolveProcessor.java @@ -29,7 +29,7 @@ public class PyResolveProcessor implements PsiScopeProcessor { private final boolean myLocalResolve; @NotNull private final Map myResults = Maps.newLinkedHashMap(); @NotNull private final Map myImplicitlyImportedResults = Maps.newLinkedHashMap(); - @Nullable private ScopeOwner myOwner; + @Nullable protected ScopeOwner myOwner; public PyResolveProcessor(@NotNull String name) { this(name, false); @@ -99,21 +99,25 @@ public class PyResolveProcessor implements PsiScopeProcessor { return definer.multiResolveName(myName); } - private boolean tryAddResult(@Nullable PsiElement element, @Nullable PyImportedNameDefiner definer) { + protected boolean tryAddResult(@Nullable PsiElement element, @Nullable PyImportedNameDefiner definer) { final ScopeOwner owner = ScopeUtil.getScopeOwner(definer != null ? definer : element); if (myOwner == null) { myOwner = owner; } final boolean sameScope = owner == myOwner; if (sameScope) { - // XXX: In 'from foo import foo' inside __init__.py the preferred result is explicitly imported 'foo' - if (definer instanceof PyFromImportStatement) { - myImplicitlyImportedResults.put(element, definer); - } - else { - myResults.put(element, definer); - } + addResult(element, definer); } return sameScope; } + + protected final void addResult(@Nullable PsiElement element, @Nullable PyImportedNameDefiner definer) { + // XXX: In 'from foo import foo' inside __init__.py the preferred result is explicitly imported 'foo' + if (definer instanceof PyFromImportStatement) { + myImplicitlyImportedResults.put(element, definer); + } + else { + myResults.put(element, definer); + } + } } diff --git a/python/python-psi-impl/src/com/jetbrains/python/psi/types/PyClassTypeImpl.java b/python/python-psi-impl/src/com/jetbrains/python/psi/types/PyClassTypeImpl.java index 5466e2c63091..e7345902c08d 100644 --- a/python/python-psi-impl/src/com/jetbrains/python/psi/types/PyClassTypeImpl.java +++ b/python/python-psi-impl/src/com/jetbrains/python/psi/types/PyClassTypeImpl.java @@ -8,12 +8,15 @@ import com.intellij.psi.*; import com.intellij.psi.scope.PsiScopeProcessor; import com.intellij.psi.util.PsiTreeUtil; import com.intellij.util.ArrayUtilRt; +import com.intellij.util.ObjectUtils; import com.intellij.util.ProcessingContext; import com.intellij.util.Processor; import com.intellij.util.containers.ContainerUtil; import com.jetbrains.python.PyNames; import com.jetbrains.python.codeInsight.PyCustomMember; import com.jetbrains.python.codeInsight.PyCustomMemberUtils; +import com.jetbrains.python.codeInsight.controlflow.ScopeOwner; +import com.jetbrains.python.codeInsight.dataflow.scope.ScopeUtil; import com.jetbrains.python.psi.*; import com.jetbrains.python.psi.impl.PyBuiltinCache; import com.jetbrains.python.psi.impl.PyCallExpressionHelper; @@ -22,6 +25,7 @@ import com.jetbrains.python.psi.impl.ResolveResultList; import com.jetbrains.python.psi.impl.references.PyReferenceImpl; import com.jetbrains.python.psi.resolve.*; import com.jetbrains.python.pyi.PyiUtil; +import com.jetbrains.python.refactoring.PyDefUseUtil; import com.jetbrains.python.toolbox.Maybe; import one.util.streamex.EntryStream; import one.util.streamex.StreamEx; @@ -445,7 +449,7 @@ public class PyClassTypeImpl extends UserDataHolderBase implements PyClassType { @NotNull String name, @Nullable PyExpression location, @NotNull TypeEvalContext context) { - final PyResolveProcessor processor = new PyResolveProcessor(name); + final PyAttributesProcessor processor = new PyAttributesProcessor(name, location); final Map results; if (isDefinition || cls.processInstanceLevelDeclarations(processor, location)) { @@ -754,4 +758,74 @@ public class PyClassTypeImpl extends UserDataHolderBase implements PyClassType { } return new PyClassTypeImpl(pyClass, isDefinition); } + + /** + *

Control flow aware Python attributes resolver.

+ * + *

It respects control flow if a resolve candidate is defined in the same scope as the location we resolve the attribute from.

+ * + *

Since an attribute doesn't have to be defined in the same method we use it, we have to assume that an attribute we cannot + * resolve via the control flow graph is defined in some other method. If the attribute is not resolved via the graph, but is defined + * in a sibling if-elif-else branch, we assume it will become available in our branch eventually in subsequent method calls.

+ */ + private static final class PyAttributesProcessor extends PyResolveProcessor { + @Nullable private final PyExpression myLocation; + + PyAttributesProcessor(@NotNull String name, @Nullable PyExpression location) { + super(name); + myLocation = location; + } + + @Override + protected boolean tryAddResult(@Nullable PsiElement element, @Nullable PyImportedNameDefiner definer) { + PsiElement psiElement = definer != null ? definer : element; + if (inSameScope(psiElement, myLocation)) { + if (PsiTreeUtil.isAncestor(psiElement, myLocation, false) || + PyDefUseUtil.isDefinedBefore(psiElement, myLocation) || + inDifferentBranchesOfSameIfStatement(psiElement, myLocation)) { + if (myOwner == null) { + myOwner = ScopeUtil.getScopeOwner(psiElement); + } + addResult(element, definer); + } + return true; + } + return super.tryAddResult(element, definer); + } + + private static boolean inSameScope(@Nullable PsiElement e1, @Nullable PsiElement e2) { + if (e1 == null || e2 == null) return false; + ScopeOwner o1 = ScopeUtil.getScopeOwner(e1); + ScopeOwner o2 = ScopeUtil.getScopeOwner(e2); + return o1 != null && o1 == o2; + } + + private static boolean inDifferentBranchesOfSameIfStatement(@NotNull PsiElement e1, @NotNull PsiElement e2) { + PyIfStatement ifStatement = ObjectUtils.tryCast(PsiTreeUtil.findCommonParent(e1, e2), PyIfStatement.class); + if (ifStatement == null) return false; + List parts = getIfStatementParts(ifStatement); + PyStatementPart p1 = findIfStatementPartByElement(e1, parts); + PyStatementPart p2 = findIfStatementPartByElement(e2, parts); + return p1 != p2; + } + + private static PyStatementPart findIfStatementPartByElement(@NotNull PsiElement element, @NotNull List parts) { + return StreamEx.of(parts) + .filter(part -> PsiTreeUtil.isAncestor(part, element, true)) + .findFirst() + .orElse(null); + } + + @NotNull + private static List getIfStatementParts(@NotNull PyIfStatement statement) { + List parts = new ArrayList<>(); + parts.add(statement.getIfPart()); + parts.addAll(Arrays.asList(statement.getElifParts())); + PyElsePart elsePart = statement.getElsePart(); + if (elsePart != null) { + parts.add(elsePart); + } + return parts; + } + } } diff --git a/python/testData/resolve/InstanceAttrAbove.py b/python/testData/resolve/InstanceAttrAbove.py new file mode 100644 index 000000000000..cae5b3284315 --- /dev/null +++ b/python/testData/resolve/InstanceAttrAbove.py @@ -0,0 +1,5 @@ +class C: + def f(self): + self.foo = 1 + return self.foo +# diff --git a/python/testData/resolve/InstanceAttrBelowEarlierByControlFlow.py b/python/testData/resolve/InstanceAttrBelowEarlierByControlFlow.py new file mode 100644 index 000000000000..97657cbc31f5 --- /dev/null +++ b/python/testData/resolve/InstanceAttrBelowEarlierByControlFlow.py @@ -0,0 +1,10 @@ +class C: + def f(self): + c = False + while True: + if c: + return self.foo + # + else: + c = True + self.foo = 1 diff --git a/python/testData/resolve/InstanceAttrBothEarlierAndLater.py b/python/testData/resolve/InstanceAttrBothEarlierAndLater.py new file mode 100644 index 000000000000..e4fa8d146f1b --- /dev/null +++ b/python/testData/resolve/InstanceAttrBothEarlierAndLater.py @@ -0,0 +1,6 @@ +class C: + def f(self): + self.foo = 1 + if self.foo: + # + self.foo = 0 diff --git a/python/testData/resolve/InstanceAttrInheritedAndAbove.py b/python/testData/resolve/InstanceAttrInheritedAndAbove.py new file mode 100644 index 000000000000..bb744c32f6fc --- /dev/null +++ b/python/testData/resolve/InstanceAttrInheritedAndAbove.py @@ -0,0 +1,10 @@ +class B: + def g(self): + self.foo = 0 + + +class C(B): + def f(self): + self.foo = 1 + return self.foo + # diff --git a/python/testData/resolve/InstanceAttrInheritedAndBelow.py b/python/testData/resolve/InstanceAttrInheritedAndBelow.py new file mode 100644 index 000000000000..0cf51ee51178 --- /dev/null +++ b/python/testData/resolve/InstanceAttrInheritedAndBelow.py @@ -0,0 +1,11 @@ +class B: + def g(self): + self.foo = 0 + + +class C(B): + def f(self): + x = self.foo + # + self.foo = 1 + return x diff --git a/python/testData/resolve/InstanceAttrOtherMethod.py b/python/testData/resolve/InstanceAttrOtherMethod.py new file mode 100644 index 000000000000..f13ebe6f3c2c --- /dev/null +++ b/python/testData/resolve/InstanceAttrOtherMethod.py @@ -0,0 +1,7 @@ +class C: + def f(self): + return self.foo + # + + def g(self): + self.foo = 1 diff --git a/python/testData/resolve/InstanceAttrOtherMethodAndAbove.py b/python/testData/resolve/InstanceAttrOtherMethodAndAbove.py new file mode 100644 index 000000000000..f93d8034f52a --- /dev/null +++ b/python/testData/resolve/InstanceAttrOtherMethodAndAbove.py @@ -0,0 +1,8 @@ +class C: + def g(self): + self.foo = 0 + + def f(self): + self.foo = 1 + return self.foo + # diff --git a/python/testData/resolve/InstanceAttrOtherMethodAndBelow.py b/python/testData/resolve/InstanceAttrOtherMethodAndBelow.py new file mode 100644 index 000000000000..613edfcb9a0b --- /dev/null +++ b/python/testData/resolve/InstanceAttrOtherMethodAndBelow.py @@ -0,0 +1,9 @@ +class C: + def f(self): + x = self.foo + # + self.foo = 1 + return x + + def g(self): + self.foo = 0 diff --git a/python/testData/resolve/NoResolveInstanceAttrBelow.py b/python/testData/resolve/NoResolveInstanceAttrBelow.py new file mode 100644 index 000000000000..2a80b00ebe00 --- /dev/null +++ b/python/testData/resolve/NoResolveInstanceAttrBelow.py @@ -0,0 +1,5 @@ +class C: + def f(self): + x = self.foo + # + self.foo = 1 diff --git a/python/testData/resolve/NoResolveInstanceAttrSameLine.py b/python/testData/resolve/NoResolveInstanceAttrSameLine.py new file mode 100644 index 000000000000..cdc08acad5d4 --- /dev/null +++ b/python/testData/resolve/NoResolveInstanceAttrSameLine.py @@ -0,0 +1,4 @@ +class C: + def f(self): + self.foo = [1, 2, self.foo] + # diff --git a/python/testSrc/com/jetbrains/python/Py3ResolveTest.java b/python/testSrc/com/jetbrains/python/Py3ResolveTest.java index e13d07346a0c..3364188ea4a4 100644 --- a/python/testSrc/com/jetbrains/python/Py3ResolveTest.java +++ b/python/testSrc/com/jetbrains/python/Py3ResolveTest.java @@ -17,6 +17,8 @@ package com.jetbrains.python; import com.intellij.psi.*; import com.intellij.psi.util.PsiTreeUtil; +import com.intellij.util.ObjectUtils; +import com.jetbrains.python.codeInsight.dataflow.scope.ScopeUtil; import com.jetbrains.python.fixtures.PyResolveTestCase; import com.jetbrains.python.psi.*; import com.jetbrains.python.psi.impl.PyBuiltinCache; @@ -775,4 +777,57 @@ public class Py3ResolveTest extends PyResolveTestCase { public void testTypeVarClassObjectBoundAttribute() { assertNull(doResolve()); } + + public void testInstanceAttrAbove() { + assertResolvesTo(PyTargetExpression.class, "foo"); + } + + public void testNoResolveInstanceAttrBelow() { + assertUnresolved(); + } + + public void testNoResolveInstanceAttrSameLine() { + assertUnresolved(); + } + + public void testInstanceAttrOtherMethod() { + assertResolvesTo(PyTargetExpression.class, "foo"); + } + + public void testInstanceAttrOtherMethodAndAbove() { + final PyTargetExpression target = assertResolvesTo(PyTargetExpression.class, "foo"); + final PyFunction function = ObjectUtils.tryCast(ScopeUtil.getScopeOwner(target), PyFunction.class); + assertNotNull(function); + assertEquals("f", function.getName()); + } + + public void testInstanceAttrOtherMethodAndBelow() { + final PyTargetExpression target = assertResolvesTo(PyTargetExpression.class, "foo"); + final PyFunction function = ObjectUtils.tryCast(ScopeUtil.getScopeOwner(target), PyFunction.class); + assertNotNull(function); + assertEquals("g", function.getName()); + } + + public void testInstanceAttrInheritedAndAbove() { + final PyTargetExpression target = assertResolvesTo(PyTargetExpression.class, "foo"); + final PyFunction function = ObjectUtils.tryCast(ScopeUtil.getScopeOwner(target), PyFunction.class); + assertNotNull(function); + assertEquals("f", function.getName()); + } + + public void testInstanceAttrInheritedAndBelow() { + final PyTargetExpression target = assertResolvesTo(PyTargetExpression.class, "foo"); + final PyFunction function = ObjectUtils.tryCast(ScopeUtil.getScopeOwner(target), PyFunction.class); + assertNotNull(function); + assertEquals("g", function.getName()); + } + + public void testInstanceAttrBelowEarlierByControlFlow() { + assertResolvesTo(PyTargetExpression.class, "foo"); + } + + public void testInstanceAttrBothEarlierAndLater() { + PyTargetExpression target = assertResolvesTo(PyTargetExpression.class, "foo"); + assertEquals("self.foo = 1", target.getParent().getText()); + } } diff --git a/python/testSrc/com/jetbrains/python/Py3TypeTest.java b/python/testSrc/com/jetbrains/python/Py3TypeTest.java index df2e8fec02e3..1b98aa2fb2a4 100644 --- a/python/testSrc/com/jetbrains/python/Py3TypeTest.java +++ b/python/testSrc/com/jetbrains/python/Py3TypeTest.java @@ -4,6 +4,7 @@ package com.jetbrains.python; import com.intellij.openapi.project.Project; import com.intellij.psi.PsiFile; import com.jetbrains.python.fixtures.PyTestCase; +import com.jetbrains.python.inspections.PyTypeCheckerInspectionTest; import com.jetbrains.python.psi.PyExpression; import com.jetbrains.python.psi.types.TypeEvalContext; import org.jetbrains.annotations.NotNull; @@ -1071,9 +1072,43 @@ public class Py3TypeTest extends PyTestCase { " expr = m"); } + /** + * @see #testRecursiveDictTopDown() + * @see PyTypeCheckerInspectionTest#testRecursiveDictAttribute() + */ + public void testRecursiveDictBottomUp() { + String text = "class C:\n" + + " def f(self, x):\n" + + " self.foo = x\n" + + " self.foo = {'foo': self.foo}\n" + + " expr = self.foo\n"; + myFixture.configureByText(PythonFileType.INSTANCE, text); + PyExpression dict = myFixture.findElementByText("{'foo': self.foo}", PyExpression.class); + assertExpressionType("Dict[str, Any]", dict); + final PyExpression expr = myFixture.findElementByText("expr", PyExpression.class); + assertExpressionType("Dict[str, Any]", expr); + } + + public void testRecursiveDictTopDown() { + String text = "class C:\n" + + " def f(self, x):\n" + + " self.foo = x\n" + + " self.foo = {'foo': self.foo}\n" + + " expr = self.foo\n"; + myFixture.configureByText(PythonFileType.INSTANCE, text); + final PyExpression expr = myFixture.findElementByText("expr", PyExpression.class); + assertExpressionType("Dict[str, Any]", expr); + PyExpression dict = myFixture.findElementByText("{'foo': self.foo}", PyExpression.class); + assertExpressionType("Dict[str, Any]", dict); + } + private void doTest(final String expectedType, final String text) { myFixture.configureByText(PythonFileType.INSTANCE, text); final PyExpression expr = myFixture.findElementByText("expr", PyExpression.class); + assertExpressionType(expectedType, expr); + } + + private void assertExpressionType(String expectedType, PyExpression expr) { final Project project = expr.getProject(); final PsiFile containingFile = expr.getContainingFile(); assertType(expectedType, expr, TypeEvalContext.codeAnalysis(project, containingFile)); diff --git a/python/testSrc/com/jetbrains/python/PyTypeTest.java b/python/testSrc/com/jetbrains/python/PyTypeTest.java index 1769e3fab14a..69a6caa2fcc4 100644 --- a/python/testSrc/com/jetbrains/python/PyTypeTest.java +++ b/python/testSrc/com/jetbrains/python/PyTypeTest.java @@ -2487,7 +2487,7 @@ public class PyTypeTest extends PyTestCase { // PY-21175 public void testLazyAttributeInitialization() { - doTest("int", + doTest("Union[int, Any]", "class C:\n" + " def __init__(self):\n" + " self.attr = None\n" +