From b04031da9e5c6378bf2f3dd0f417184c14a5a3b5 Mon Sep 17 00:00:00 2001 From: Fabio Zadrozny Date: Sun, 1 Oct 2023 07:44:10 -0300 Subject: [PATCH] Imports found inside a typing.TYPE_CHECKING will be considered undefined if the scope that uses it requires it to be available when not type-checking. --- .../AbstractScopeAnalyzerVisitor.java | 45 ++++++++---- .../analysis/visitors/DuplicationChecker.java | 2 +- .../pydev/analysis/visitors/GenAndTok.java | 8 ++- .../python/pydev/analysis/visitors/Scope.java | 64 ++++++++++++----- .../pydev/analysis/visitors/ScopeItems.java | 64 ++++++++++++++--- .../OccurrencesAnalyzerPy310Test.java | 71 +++++++++++++++++++ 6 files changed, 211 insertions(+), 43 deletions(-) diff --git a/plugins/com.python.pydev.analysis/src/com/python/pydev/analysis/scopeanalysis/AbstractScopeAnalyzerVisitor.java b/plugins/com.python.pydev.analysis/src/com/python/pydev/analysis/scopeanalysis/AbstractScopeAnalyzerVisitor.java index 0d0e104009..dea3262307 100644 --- a/plugins/com.python.pydev.analysis/src/com/python/pydev/analysis/scopeanalysis/AbstractScopeAnalyzerVisitor.java +++ b/plugins/com.python.pydev.analysis/src/com/python/pydev/analysis/scopeanalysis/AbstractScopeAnalyzerVisitor.java @@ -415,7 +415,11 @@ public Object visitFunctionDef(FunctionDef node) throws Exception { if (args.annotation != null) { for (exprType expr : args.annotation) { if (expr != null) { - startScope(Scope.SCOPE_TYPE_ANNOTATION, expr); + int scopeType = Scope.SCOPE_TYPE_ANNOTATION; + if (futureAnnotationsImported) { + scopeType = Scope.SCOPE_TYPE_ANNOTATION_STR; + } + startScope(scopeType, expr); expr.accept(visitor); endScope(expr); } @@ -424,7 +428,13 @@ public Object visitFunctionDef(FunctionDef node) throws Exception { //visit the return if (node.returns != null) { + int scopeType = Scope.SCOPE_TYPE_ANNOTATION; + if (futureAnnotationsImported) { + scopeType = Scope.SCOPE_TYPE_ANNOTATION_STR; + } + startScope(scopeType, node.returns); node.returns.accept(visitor); + endScope(node.returns); } //then the decorators (no, still not in method scope) @@ -886,9 +896,9 @@ protected static void visitCallAttr(Call c, VisitorBase base) throws Exception { @Override public Object visitFor(For node) throws Exception { - scope.addIfSubScope(); + scope.addStatementSubScope(); Object ret = super.visitFor(node); - scope.removeIfSubScope(); + scope.removeStatementSubScope(); return ret; } @@ -905,7 +915,11 @@ public Object visitAssign(Assign node) throws Exception { } if (node.type != null) { - startScope(Scope.SCOPE_TYPE_ANNOTATION, node.type); + int scopeType = Scope.SCOPE_TYPE_ANNOTATION; + if (futureAnnotationsImported) { + scopeType = Scope.SCOPE_TYPE_ANNOTATION_STR; + } + startScope(scopeType, node.type); node.type.accept(this); endScope(node.type); } @@ -927,7 +941,7 @@ public Object visitAssign(Assign node) throws Exception { */ @Override public Object visitIf(If node) throws Exception { - scope.addIfSubScope(); + scope.addIfSubScope(node); Object r = super.visitIf(node); scope.removeIfSubScope(); return r; @@ -938,9 +952,9 @@ public Object visitIf(If node) throws Exception { */ @Override public Object visitWhile(While node) throws Exception { - scope.addIfSubScope(); + scope.addStatementSubScope(); Object r = super.visitWhile(node); - scope.removeIfSubScope(); + scope.removeStatementSubScope(); return r; } @@ -954,9 +968,9 @@ public Object visitTryExcept(TryExcept node) throws Exception { @Override public Object visitTryFinally(TryFinally node) throws Exception { - scope.addIfSubScope(); + scope.addStatementSubScope(); Object r = super.visitTryFinally(node); - scope.removeIfSubScope(); + scope.removeStatementSubScope(); return r; } @@ -1116,14 +1130,17 @@ protected void endScope(SimpleNode node) { for (Found found : foundItems) { //the scope where it is defined must be an outer scope so that we can say it was defined later... final GenAndTok foundItemFirst = found.getSingle(); - //if something was not defined in a method, if we are in the class definition, it won't be found. if ((probablyNotDefinedFirst.scopeFound.getScopeType() & Scope.ACCEPTED_METHOD_AND_LAMBDA) != 0 && m.getScopeType() != Scope.SCOPE_TYPE_CLASS) { if (foundItemFirst.scopeId < probablyNotDefinedFirst.scopeId) { - found.setUsed(true); - setUsed = true; + if (scope.typeCheckingDefinitionAndUsageOk(foundItemFirst, + probablyNotDefinedFirst.inTypeChecking, + probablyNotDefinedFirst.scopeFound.getScopeType())) { + found.setUsed(true); + setUsed = true; + } } } } @@ -1186,6 +1203,7 @@ protected boolean markRead(IToken token, String rep, boolean addToNotDefined, bo int acceptedScopes = 0; ScopeItems currScopeItems = scope.getCurrScopeItems(); + boolean inTypeChecking = currScopeItems.isInTypeChecking(); if ((currScopeItems.getScopeType() & Scope.ACCEPTED_METHOD_AND_LAMBDA) != 0) { acceptedScopes = Scope.ACCEPTED_METHOD_SCOPES; @@ -1210,7 +1228,8 @@ protected boolean markRead(IToken token, String rep, boolean addToNotDefined, bo //search for it while (found == false && it.hasNext()) { String nextTokToSearch = it.next(); - foundAs = scope.findFirst(nextTokToSearch, true, acceptedScopes); + foundAs = scope.findFirst(nextTokToSearch, true, acceptedScopes, inTypeChecking, + currScopeItems.getScopeType()); found = foundAs != null; if (found) { foundAsStr = nextTokToSearch; diff --git a/plugins/com.python.pydev.analysis/src/com/python/pydev/analysis/visitors/DuplicationChecker.java b/plugins/com.python.pydev.analysis/src/com/python/pydev/analysis/visitors/DuplicationChecker.java index a2524b8af2..eeaccade11 100644 --- a/plugins/com.python.pydev.analysis/src/com/python/pydev/analysis/visitors/DuplicationChecker.java +++ b/plugins/com.python.pydev.analysis/src/com/python/pydev/analysis/visitors/DuplicationChecker.java @@ -104,7 +104,7 @@ private boolean canReplaceScope(SimpleNode currNode, SimpleNode node) { */ private void checkDuplication(String name, SimpleNode node) { if (stack.size() > 0) { - if (!scope.getPrevScopeItems().getIsInSubSubScope()) { + if (!scope.getPrevScopeItems().getIsInStatementSubScope()) { SimpleNode currNode = stack.peek().get(name); if (currNode != null) { if (hasTypingOverloadDecorator(currNode) || hasTypingOverloadDecorator(node)) { diff --git a/plugins/com.python.pydev.analysis/src/com/python/pydev/analysis/visitors/GenAndTok.java b/plugins/com.python.pydev.analysis/src/com/python/pydev/analysis/visitors/GenAndTok.java index 220a4a227c..95d4bded65 100644 --- a/plugins/com.python.pydev.analysis/src/com/python/pydev/analysis/visitors/GenAndTok.java +++ b/plugins/com.python.pydev.analysis/src/com/python/pydev/analysis/visitors/GenAndTok.java @@ -19,7 +19,7 @@ public final class GenAndTok { /** * This is the token that is from the current module that created the token (if on some wild import) - * + * * May be equal to tok */ public final IToken generator; @@ -44,11 +44,17 @@ public final class GenAndTok { */ public final ScopeItems scopeFound; + /** + * Determines whether it was in a typing.TYPE_CHECKING if when it was found. + */ + public final boolean inTypeChecking; + public GenAndTok(IToken generator, IToken tok, int scopeId, ScopeItems scopeFound) { this.generator = generator; this.tok = tok; this.scopeId = scopeId; this.scopeFound = scopeFound; + this.inTypeChecking = scopeFound.isInTypeChecking(); } @Override diff --git a/plugins/com.python.pydev.analysis/src/com/python/pydev/analysis/visitors/Scope.java b/plugins/com.python.pydev.analysis/src/com/python/pydev/analysis/visitors/Scope.java index 3c5611ad29..70101b7337 100644 --- a/plugins/com.python.pydev.analysis/src/com/python/pydev/analysis/visitors/Scope.java +++ b/plugins/com.python.pydev.analysis/src/com/python/pydev/analysis/visitors/Scope.java @@ -19,6 +19,7 @@ import org.python.pydev.core.IToken; import org.python.pydev.core.IterTokenEntry; import org.python.pydev.core.TokensList; +import org.python.pydev.parser.jython.ast.If; import org.python.pydev.parser.jython.ast.TryExcept; import org.python.pydev.shared_core.string.FastStringBuffer; import org.python.pydev.shared_core.structure.FastStack; @@ -176,7 +177,8 @@ public Scope(AbstractScopeAnalyzerVisitor visitor, IPythonNature nature, String * - imports such as import os.path (one token is created for os and one for os.path) */ public void addImportTokens(TokensList list, IToken generator, ICompletionCache completionCache) { - ScopeItems.TryExceptInfo withinExceptNode = scope.peek().getTryExceptImportError(); + ScopeItems currScope = scope.peek(); + ScopeItems.TryExceptInfo withinExceptNode = currScope.getTryExceptImportError(); //only report undefined imports if we're not inside a try..except ImportError. boolean reportUndefinedImports = withinExceptNode == null; @@ -197,11 +199,10 @@ public void addImportTokens(TokensList list, IToken generator, ICompletionCache requireTokensToBeImports = true; } - ScopeItems m = scope.peek(); for (IterTokenEntry entry : list) { IToken o = entry.getToken(); //System.out.println("adding: "+o.getRepresentation()); - Found found = addToken(generator, m, o, o.getRepresentation()); + Found found = addToken(generator, currScope, o, o.getRepresentation()); if (withinExceptNode != null) { withinExceptNode.addFoundImportToTryExcept(found); //may mark previous as used... } @@ -253,16 +254,18 @@ public Found addToken(IToken generator, ScopeItems m, IToken o, String rep) { generator = o; } - Found found = findFirst(rep, false); + Found found = findFirst(rep, false, m.isInTypeChecking(), m.getScopeType()); boolean isReimport = false; if (!isInMethodDefinition && found != null) { //it will be removed from the scope if (found.isImport() && generator.isImport()) { - isReimport = true; + if (!found.getSingle().inTypeChecking) { + isReimport = true; + } //keep on going, as it still might be used or unused } else { - if (!found.isUsed() && !m.getIsInSubSubScope()) { // it was not used, and we're not in an if scope... + if (!found.isUsed() && !m.getIsInStatementSubScope()) { // it was not used, and we're not in an if scope... //this kind of unused message should only happen if we are at the same scope... if (found.getSingle().scopeFound.getScopeId() == getCurrScopeId()) { @@ -290,6 +293,9 @@ public Found addToken(IToken generator, ScopeItems m, IToken o, String rep) { } Found newFound = new Found(o, generator, m.getScopeId(), m); + if (m.isInTypeChecking()) { + newFound.setUsed(true); // Don't report imports inside of typing.TYPE_CHECKING as unused. + } if (isReimport) { if (m.getTryExceptImportError() == null) { //we don't want to add reimport messages if we're within a try..except @@ -377,39 +383,61 @@ public List findInScopes(String name, boolean setUsed) { return ret; } - public Found findFirst(String name, boolean setUsed) { - return findFirst(name, setUsed, ACCEPTED_ALL_SCOPES); + public Found findFirst(String name, boolean setUsed, boolean inTypeChecking, int currScopeType) { + return findFirst(name, setUsed, ACCEPTED_ALL_SCOPES, inTypeChecking, currScopeType); + } + + public boolean typeCheckingDefinitionAndUsageOk(GenAndTok definition, boolean usageTypeChecking, + int currScopeType) { + // This if is not needed because a SCOPE_TYPE_ANNOTATION_STR is used instead of a SCOPE_TYPE_ANNOTATION + // when visitor.futureAnnotationsImported == true. + // if (visitor.futureAnnotationsImported) { + // } + if ((currScopeType & SCOPE_TYPE_ANNOTATION_STR) != 0) { + return true; + } + if (definition.inTypeChecking && !usageTypeChecking) { + return false; + } + return true; } - public Found findFirst(String name, boolean setUsed, int acceptedScopes) { + public Found findFirst(String name, boolean setUsed, int acceptedScopes, boolean inTypeChecking, + int currScopeType) { Iterator topDown = scope.topDownIterator(); while (topDown.hasNext()) { ScopeItems m = topDown.next(); if ((m.getScopeType() & acceptedScopes) != 0) { Found f = m.getLastAppearance(name); if (f != null) { - if (setUsed) { - f.setUsed(true); + if (typeCheckingDefinitionAndUsageOk(f.getSingle(), inTypeChecking, currScopeType)) { + if (setUsed) { + f.setUsed(true); + } + return f; } - return f; } } } return null; } - public void addIfSubScope() { - scope.peek().addIfSubScope(); - } - - public boolean getIsInIfSubScope() { - return scope.peek().getIsInIfSubScope(); + public void addIfSubScope(If node) { + scope.peek().addIfSubScope(node); } public void removeIfSubScope() { scope.peek().removeIfSubScope(); } + public void addStatementSubScope() { + scope.peek().addStatementSubScope(); + } + + public void removeStatementSubScope() { + scope.peek().removeStatementSubScope(); + } + public void addTryExceptSubScope(TryExcept node) { scope.peek().addTryExceptSubScope(node); } diff --git a/plugins/com.python.pydev.analysis/src/com/python/pydev/analysis/visitors/ScopeItems.java b/plugins/com.python.pydev.analysis/src/com/python/pydev/analysis/visitors/ScopeItems.java index 92bafa766c..3c2c88c380 100644 --- a/plugins/com.python.pydev.analysis/src/com/python/pydev/analysis/visitors/ScopeItems.java +++ b/plugins/com.python.pydev.analysis/src/com/python/pydev/analysis/visitors/ScopeItems.java @@ -13,9 +13,11 @@ import java.util.Collection; import java.util.HashMap; import java.util.List; +import java.util.ListIterator; import java.util.Map; import org.python.pydev.core.IToken; +import org.python.pydev.parser.jython.ast.If; import org.python.pydev.parser.jython.ast.TryExcept; import org.python.pydev.parser.jython.ast.excepthandlerType; import org.python.pydev.parser.visitors.NodeUtils; @@ -83,7 +85,8 @@ public ScopeItems.TryExceptInfo getTryExceptImportError() { */ public final Map> namesToIgnore = new HashMap>(); - public int ifSubScope = 0; + public final FastStack ifNodes = new FastStack<>(10); + public int statementSubScope = 0; public final FastStack tryExceptSubScope = new FastStack(10); private final int scopeId; private final int scopeType; @@ -140,32 +143,73 @@ public void put(String rep, Found found) { foundItems.add(found); } - public void addIfSubScope() { - ifSubScope++; + // Cache on whether it's currently type-checking. + private Boolean inTypeChecking = null; + + public boolean isInTypeChecking() { + if (statementSubScope == 0 || this.ifNodes.size() == 0) { + return false; + } + + if (inTypeChecking != null) { + return inTypeChecking; + } + + inTypeChecking = false; + for (ListIterator it = this.ifNodes.iterator(); it.hasNext();) { + If ifStatement = it.next(); + if (isTypeCheckingIfStatement(ifStatement)) { + inTypeChecking = true; + break; + } + } + return inTypeChecking; + } + + private boolean isTypeCheckingIfStatement(If ifStatement) { + String rep = NodeUtils.getFullRepresentationString(ifStatement.test); + if (rep != null && (rep.equals("typing.TYPE_CHECKING") || rep.equals("TYPE_CHECKING"))) { + return true; + } + return false; + } + + public void addIfSubScope(If node) { + this.ifNodes.push(node); + statementSubScope++; + inTypeChecking = null; } public void removeIfSubScope() { - ifSubScope--; + this.ifNodes.pop(); + statementSubScope--; + inTypeChecking = null; + } + + public void addStatementSubScope() { + statementSubScope++; + } + + public void removeStatementSubScope() { + statementSubScope--; } public void addTryExceptSubScope(TryExcept node) { tryExceptSubScope.push(new TryExceptInfo(node)); + statementSubScope++; } public void removeTryExceptSubScope() { tryExceptSubScope.pop(); + statementSubScope--; } public FastStack getCurrTryExceptNodes() { return tryExceptSubScope; } - public boolean getIsInSubSubScope() { - return ifSubScope != 0 || tryExceptSubScope.size() != 0; - } - - public boolean getIsInIfSubScope() { - return ifSubScope != 0; + public boolean getIsInStatementSubScope() { + return statementSubScope != 0; } /** diff --git a/plugins/org.python.pydev/tests_analysis/com/python/pydev/analysis/OccurrencesAnalyzerPy310Test.java b/plugins/org.python.pydev/tests_analysis/com/python/pydev/analysis/OccurrencesAnalyzerPy310Test.java index bf7e41db87..757e6a63b1 100644 --- a/plugins/org.python.pydev/tests_analysis/com/python/pydev/analysis/OccurrencesAnalyzerPy310Test.java +++ b/plugins/org.python.pydev/tests_analysis/com/python/pydev/analysis/OccurrencesAnalyzerPy310Test.java @@ -628,4 +628,75 @@ public void testTypingInfoInStrBad3() { assertEquals(2, messages[0].getEndLine(doc)); assertEquals(32, messages[0].getStartCol(doc)); } + + public void testTypingInTypeChecking() { + // i.e.: when an import is added when 'TYPE_CHECKING' it should not + // be considered a reimport afterwards. + doc = new Document("" + + "import typing\n" + + "if typing.TYPE_CHECKING:\n" + + " import functools\n" + + "def method() -> 'functools.partial':\n" + + " import functools\n" + + " print(functools.partial)\n"); + checkNoError(); + } + + public void testTypingInTypeCheckingUndefined() { + // i.e.: It's defined only when type-checking, so, it's an + // error to try to actually access it. + doc = new Document("" + + "import typing\n" + + "if typing.TYPE_CHECKING:\n" + + " import functools\n" + + "def method() -> 'functools.partial':\n" + + " print(functools.partial)\n"); + IMessage[] messages = checkError("Undefined variable: functools\n"); + assertEquals(1, messages.length); + assertEquals(5, messages[0].getStartLine(doc)); + } + + public void testTypingInTypeCheckingDefinedWithFutureImport() { + doc = new Document("from __future__ import annotations\n" + + "\n" + + "import typing\n" + + "\n" + + "if typing.TYPE_CHECKING:\n" + + " import functools\n" + + "\n" + + "\n" + + "def method() -> functools.partial:\n" + + " pass\n" + + ""); + checkNoError(); + } + + public void testTypingInStrMarksAsUsed() { + doc = new Document("import typing\n" + + "\n" + + "if typing.TYPE_CHECKING:\n" + + " import functools\n" + + "\n" + + "\n" + + "def method() -> 'functools.partial':\n" + + " pass\n" + + ""); + checkNoError(); + } + + public void testTypingInTypeCheckingUndefinedWithoutFutureImport() { + doc = new Document("import typing\n" + + "\n" + + "if typing.TYPE_CHECKING:\n" + + " import functools\n" + + "\n" + + "\n" + + "def method() -> functools.partial:\n" + + " pass\n" + + ""); + // i.e.: it's only defined when type checking! + IMessage[] messages = checkError("Undefined variable: functools\n"); + assertEquals(1, messages.length); + assertEquals(7, messages[0].getStartLine(doc)); + } }