-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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.
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
[used before def] improve handling of global definitions in local scopes #14517
Changes from 12 commits
3a14986
5415d4e
66a7802
342c05d
ddc4418
8e69cd4
286840f
33561c2
cff35c6
43ddea4
b5a02a4
78b0f55
9adbe70
39c22d5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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( | ||
|
@@ -171,7 +173,7 @@ class ScopeType(Enum): | |
Global = 1 | ||
Class = 2 | ||
Func = 3 | ||
Generator = 3 | ||
Generator = 4 | ||
|
||
|
||
class Scope: | ||
|
@@ -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 | ||
|
@@ -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() | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
for name in implicit_module_attrs: | ||
self.tracker.record_definition(name) | ||
|
||
|
@@ -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: | ||
|
@@ -415,17 +426,24 @@ def visit_match_stmt(self, o: MatchStmt) -> None: | |
|
||
def visit_func_def(self, o: FuncDef) -> None: | ||
self.process_definition(o.name) | ||
self.tracker.enter_scope(ScopeType.Func) | ||
super().visit_func_def(o) | ||
self.tracker.exit_scope() | ||
|
||
def visit_func(self, o: FuncItem) -> None: | ||
if o.is_dynamic() and not self.options.check_untyped_defs: | ||
return | ||
if o.arguments is not None: | ||
for arg in o.arguments: | ||
self.tracker.record_definition(arg.variable.name) | ||
super().visit_func(o) | ||
|
||
args = o.arguments or [] | ||
# Process initializers (defaults) outside the function scope. | ||
for arg in args: | ||
if arg.initializer is not None: | ||
arg.initializer.accept(self) | ||
|
||
self.tracker.enter_scope(ScopeType.Func) | ||
for arg in args: | ||
self.process_definition(arg.variable.name) | ||
super().visit_var(arg.variable) | ||
o.body.accept(self) | ||
self.tracker.exit_scope() | ||
|
||
def visit_generator_expr(self, o: GeneratorExpr) -> None: | ||
self.tracker.enter_scope(ScopeType.Generator) | ||
|
@@ -603,8 +621,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) | ||
|
There was a problem hiding this comment.
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!