Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[used before def] improve handling of global definitions in local scopes #14517

Merged
merged 14 commits into from
Mar 1, 2023
Merged

[used before def] improve handling of global definitions in local scopes #14517

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
3a14986
[used before def] rework builtin handling
ilinum Jan 21, 2023
5415d4e
store SymbolTable instance + update comment
ilinum Jan 23, 2023
66a7802
Merge branch 'master' into used-before-def-multipass
ilinum Jan 23, 2023
342c05d
rework scope inheritance in defs
ilinum Jan 23, 2023
ddc4418
extend fixes to all non-generator scopes
ilinum Jan 23, 2023
8e69cd4
minor refactor
ilinum Jan 23, 2023
286840f
Merge branch 'master' into used-before-def-scope
ilinum Jan 24, 2023
33561c2
test whitespace
ilinum Jan 24, 2023
cff35c6
Merge branch 'master' into used-before-def-scope
ilinum Feb 8, 2023
43ddea4
fix mypyc test
ilinum Feb 8, 2023
b5a02a4
fix default args
ilinum Feb 8, 2023
78b0f55
fix tests
ilinum Feb 8, 2023
9adbe70
Merge branch 'master' into used-before-def-multipass
ilinum Mar 1, 2023
39c22d5
more efficient builtin handling
ilinum Mar 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 18 additions & 9 deletions mypy/partially_defined.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,9 @@ def copy(self) -> BranchState:


class BranchStatement:
def __init__(self, initial_state: BranchState) -> None:
def __init__(self, initial_state: BranchState | None = None) -> None:
if initial_state is None:
initial_state = BranchState()
self.initial_state = initial_state
self.branches: list[BranchState] = [
BranchState(
Expand Down Expand Up @@ -171,7 +173,7 @@ class ScopeType(Enum):
Global = 1
Class = 2
Func = 3
Generator = 3
Generator = 4
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This didn't matter until we needed to handle generators differently. Generators actually do inherit the scope!



class Scope:
Expand Down Expand Up @@ -199,7 +201,7 @@ class DefinedVariableTracker:

def __init__(self) -> None:
# There's always at least one scope. Within each scope, there's at least one "global" BranchingStatement.
self.scopes: list[Scope] = [Scope([BranchStatement(BranchState())], ScopeType.Global)]
self.scopes: list[Scope] = [Scope([BranchStatement()], ScopeType.Global)]
# disable_branch_skip is used to disable skipping a branch due to a return/raise/etc. This is useful
# in things like try/except/finally statements.
self.disable_branch_skip = False
Expand All @@ -216,9 +218,11 @@ def _scope(self) -> Scope:

def enter_scope(self, scope_type: ScopeType) -> None:
assert len(self._scope().branch_stmts) > 0
self.scopes.append(
Scope([BranchStatement(self._scope().branch_stmts[-1].branches[-1])], scope_type)
)
initial_state = None
if scope_type == ScopeType.Generator:
# Generators are special because they inherit the outer scope.
initial_state = self._scope().branch_stmts[-1].branches[-1]
self.scopes.append(Scope([BranchStatement(initial_state)], scope_type))

def exit_scope(self) -> None:
self.scopes.pop()
Expand Down Expand Up @@ -328,6 +332,11 @@ def __init__(
self.loops: list[Loop] = []
self.try_depth = 0
self.tracker = DefinedVariableTracker()
builtins_mod = names.get("__builtins__", None)
if builtins_mod:
assert isinstance(builtins_mod.node, MypyFile)
for name in builtins_mod.node.names:
self.tracker.record_definition(name)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this might degrade performance, since builtins have over 200 definitions, and it looks like we are iterating over all of them for every module. Can you measure the time spent in this loop when performing a self check, for example?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have found a way to deal with this that doesn't involve calling record_definition every time! So performance should be unchanged.

for name in implicit_module_attrs:
self.tracker.record_definition(name)

Expand All @@ -342,13 +351,15 @@ def variable_may_be_undefined(self, name: str, context: Context) -> None:
def process_definition(self, name: str) -> None:
# Was this name previously used? If yes, it's a used-before-definition error.
if not self.tracker.in_scope(ScopeType.Class):
# Errors in class scopes are caught by the semantic analyzer.
refs = self.tracker.pop_undefined_ref(name)
for ref in refs:
if self.loops:
self.variable_may_be_undefined(name, ref)
else:
self.var_used_before_def(name, ref)
else:
# Errors in class scopes are caught by the semantic analyzer.
pass
self.tracker.record_definition(name)

def visit_global_decl(self, o: GlobalDecl) -> None:
Expand Down Expand Up @@ -603,8 +614,6 @@ def visit_starred_pattern(self, o: StarredPattern) -> None:
super().visit_starred_pattern(o)

def visit_name_expr(self, o: NameExpr) -> None:
if o.name in self.builtins:
return
if self.tracker.is_possibly_undefined(o.name):
# A variable is only defined in some branches.
self.variable_may_be_undefined(o.name, o)
Expand Down
120 changes: 78 additions & 42 deletions test-data/unit/check-possibly-undefined.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,6 @@ def f0() -> None:
y = x
x = 1 # No error.


[case testGlobalDeclarationAfterUsage]
# flags: --enable-error-code possibly-undefined --enable-error-code used-before-def
def f0() -> None:
Expand All @@ -219,6 +218,7 @@ def f0() -> None:
x = 1 # No error.

x = 2

[case testVarDefinedInOuterScope]
# flags: --enable-error-code possibly-undefined --enable-error-code used-before-def
def f0() -> None:
Expand All @@ -227,13 +227,57 @@ def f0() -> None:

f0()
x = 1

[case testDefinedInOuterScopeNoError]
# flags: --enable-error-code possibly-undefined --enable-error-code used-before-def
def foo() -> None:
bar()

def bar() -> None:
foo()

[case testClassFromOuterScopeRedefined]
# flags: --enable-error-code possibly-undefined --enable-error-code used-before-def
class c: pass

def f0() -> None:
s = c() # E: Name "c" is used before definition
class c: pass


def f1() -> None:
s = c() # No error.


def f2() -> None:
s = c() # E: Name "c" is used before definition
if int():
class c: pass

[case testVarFromOuterScopeRedefined]
# flags: --enable-error-code possibly-undefined --enable-error-code used-before-def
x = 0

def f0() -> None:
y = x # E: Name "x" is used before definition
x = 0

def f1() -> None:
y = x # No error.

def f2() -> None:
y = x # E: Name "x" is used before definition
global x

def f3() -> None:
global x
y = x # No error.

def f4() -> None:
if int():
x = 0
y = x # E: Name "x" may be undefined

[case testFuncParams]
# flags: --enable-error-code possibly-undefined
def foo(a: int) -> None:
Expand Down Expand Up @@ -829,67 +873,56 @@ def f4() -> None:
x = z # E: Name "z" is used before definition
z: int = 2

[case testUsedBeforeDefImportsBasic]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when I wrote these import tests, I misunderstood how they would actually behave at runtime.

# flags: --enable-error-code used-before-def
[case testUsedBeforeDefImportsBasicImportNoError]
# flags: --enable-error-code used-before-def --enable-error-code possibly-undefined --disable-error-code no-redef
import foo # type: ignore
import x.y # type: ignore

def f0() -> None:
a = foo # No error.
foo: int = 1
a = foo # No error.
foo: int = 1

def f1() -> None:
a = y # E: Name "y" is used before definition
y: int = 1
[case testUsedBeforeDefImportsDotImport]
# flags: --enable-error-code used-before-def --enable-error-code possibly-undefined --disable-error-code no-redef
import x.y # type: ignore

def f2() -> None:
a = x # No error.
x: int = 1
a = y # E: Name "y" is used before definition
y: int = 1

def f3() -> None:
a = x.y # No error.
x: int = 1
b = x # No error.
x: int = 1

c = x.y # No error.
x: int = 1

[case testUsedBeforeDefImportBasicRename]
# flags: --enable-error-code used-before-def
# flags: --enable-error-code used-before-def --disable-error-code=no-redef
import x.y as z # type: ignore
from typing import Any

def f0() -> None:
a = z # No error.
z: int = 1

def f1() -> None:
a = x # E: Name "x" is used before definition
x: int = 1
a = z # No error.
z: int = 1

def f2() -> None:
a = x.y # E: Name "x" is used before definition
x: Any = 1
a = x # E: Name "x" is used before definition
x: int = 1

def f3() -> None:
a = y # E: Name "y" is used before definition
y: int = 1
a = y # E: Name "y" is used before definition
y: int = 1

[case testUsedBeforeDefImportFrom]
# flags: --enable-error-code used-before-def
# flags: --enable-error-code used-before-def --disable-error-code no-redef
from foo import x # type: ignore

def f0() -> None:
a = x # No error.
x: int = 1
a = x # No error.
x: int = 1

[case testUsedBeforeDefImportFromRename]
# flags: --enable-error-code used-before-def
# flags: --enable-error-code used-before-def --disable-error-code no-redef
from foo import x as y # type: ignore

def f0() -> None:
a = y # No error.
y: int = 1
a = y # No error.
y: int = 1

def f1() -> None:
a = x # E: Name "x" is used before definition
x: int = 1
a = x # E: Name "x" is used before definition
x: int = 1

[case testUsedBeforeDefFunctionDeclarations]
# flags: --enable-error-code used-before-def
Expand All @@ -905,10 +938,13 @@ def f0() -> None:
# flags: --enable-error-code used-before-def

def f0() -> None:
s = type(123)
s = type(123) # E: Name "type" is used before definition
type = "abc"
a = type

def f1() -> None:
s = type(123)

[case testUsedBeforeDefBuiltinsMultipass]
# flags: --enable-error-code used-before-def

Expand Down