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.

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

Merged
merged 14 commits into from
Mar 1, 2023

Conversation

ilinum
Copy link
Collaborator

@ilinum ilinum commented Jan 24, 2023

While working on #14483, we discovered that variable inheritance didn't work quite right. In particular, functions would inherit variables from outer scope. On the surface, this is what you want but actually, they only inherit the scope if there isn't a colliding definition within that scope.

Here's an example:

class c: pass

def f0() -> None:
    s = c()  # UnboundLocalError is raised when this code is executed.
    class c: pass

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

This PR also fixes issues with builtins (exactly the same example as above but instead of c we have a builtin).

Fixes #14213 (as much as is reasonable to do)

@@ -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!

@@ -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.

@ilinum ilinum changed the title Used before def multipass [used before def] improve handling of global definitions in local scopes Jan 24, 2023
@github-actions

This comment has been minimized.

@ilinum ilinum marked this pull request as draft January 24, 2023 16:24
@ilinum ilinum marked this pull request as ready for review February 8, 2023 17:35
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@ilinum ilinum requested review from ilevkivskyi and JukkaL February 24, 2023 16:23
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.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 1, 2023

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for making this faster!

@JukkaL JukkaL merged commit a618110 into python:master Mar 1, 2023
@ilinum ilinum deleted the used-before-def-multipass branch March 2, 2023 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve handling of global scope for partially defined check
2 participants