Repository: python/mypy
Pull request: [used before def] improve handling of global definitions in local scopes
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:
```python
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)

Problem statement:
Improve handling of global scope for partially defined check
Currently, partially defined check (responsible for two error codes: `partially-defined` and `use-before-def`) doesn't handle variables defined in outer scope very consistently.

Here's an example that generates no error right now:
```python
def foo() -> None:
  global x
  print(x)  # No error here right now.

foo()  # This will result in a runtime error.
x = 1
```

However, in some cases, the check also generates use-before-def errors:
```python
def foo() -> None:
  global x
  print(x)  # E: Name "x" is used before definition[use-before-def]
  x = 1

x = 1
```

Note that the same applies to inner functions.

The question is: what should the behavior be here? Should we generate a `may be defined` error in both cases? Or, perhaps, we should generate no error in either case (because it's a pretty common pattern).

@JukkaL @ilevkivskyi do you have thoughts?

Hints from issue discussion:
I think it should be "may be undefined" in both cases. IMO the general rule is roughly like this: if mypy can prove it will always fail, it is "use before definition", and if mypy cannot prove it will not fail, it is "may be undefined". So it is fine to have some false negatives for the first, and some false positives for the second. But not vice versa.
I agree with the thought behind this (when to use which error code). 

However, I'm a bit worried that it will generate too many false positives and I'm thinking that maybe we should explicitly assume that everything from outer scope is always defined, even if definition comes after.

I recently stumbled upon an interesting example:
```python
class C:
    def f0(self) -> None:
        self.f1()  # If we report cross-scope errors, this will be a `partially-defined` error.
    def f1(self) -> None:
        pass
```
So for the example with classes above, we will definitely need some special handling (make class scope special). I'm not convinced that the noise is worth it.

The example with method is obviously safe, but attribute access is totally different story and will require separate handling independently of what we do here.

Coming back to variables, the `partially-defined` error code is going to stay off by default, so people who will opt in to it, will likely prefer false positives over false negatives (especially if there is a simple workaround like put variables at top of module). As a random data point, I have fixed a bad bug caused by use of a forward global ~year ago.

Note however, code like this should not generate any errors:
```python
def foo() -> None:
    bar()

def bar() -> None:
    foo()
```
Because this is actually a very common pattern, and a totally fine one. If it is hard to implement the global variable error without also causing an error in this last example, it is fine to leave it as a follow up issue. (But we definitely should not give the `use-before-def` error as we currently do.)
The check is a bit non-trivial to implement (since we need to handle var vs def declaration in scope differently). For now, #14222 will make it not generate an error. 
After #14517 is merged, this issue will largely be resolved.
We still don't detect the following case:
```python
def foo() -> None:
  global x
  print(x)  # No error here right now.

foo()  # This will result in a runtime error.
x = 1
```

But I don't see how we can handle this in a reasonable way. We either need to flag all such uses as possibly-undefined (which will cause a *ton* of errors -- any time you're using a global var). Or we'll need to track the order in which functions are called (which seems very difficult to implement).

Commit message: fix tests

Patch to review:
diff --git a/mypy/partially_defined.py b/mypy/partially_defined.py
index af09493c9cae..22385a902ddc 100644
--- a/mypy/partially_defined.py
+++ b/mypy/partially_defined.py
@@ -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)
         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)
diff --git a/mypyc/test-data/run-sets.test b/mypyc/test-data/run-sets.test
index 56c946933fac..8d178d03a75b 100644
--- a/mypyc/test-data/run-sets.test
+++ b/mypyc/test-data/run-sets.test
@@ -141,8 +141,8 @@ def test_in_set() -> None:
         assert main_set(item), f"{item!r} should be in set_main"
         assert not main_negated_set(item), item
 
-    assert non_final_name_set(non_const)
     global non_const
+    assert non_final_name_set(non_const)
     non_const = "updated"
     assert non_final_name_set("updated")
 
diff --git a/test-data/unit/check-functions.test b/test-data/unit/check-functions.test
index c23bbb77f643..b76abd31e3dc 100644
--- a/test-data/unit/check-functions.test
+++ b/test-data/unit/check-functions.test
@@ -491,62 +491,61 @@ if int():
 
 [case testDefaultArgumentExpressions]
 import typing
+class B: pass
+class A: pass
+
 def f(x: 'A' = A()) -> None:
     b = x # type: B # E: Incompatible types in assignment (expression has type "A", variable has type "B")
     a = x # type: A
-
-class B: pass
-class A: pass
 [out]
 
 [case testDefaultArgumentExpressions2]
 import typing
-def f(x: 'A' = B()) -> None: # E: Incompatible default for argument "x" (default has type "B", argument has type "A")
-    b = x # type: B      # E: Incompatible types in assignment (expression has type "A", variable has type "B")
-    a = x # type: A
-
 class B: pass
 class A: pass
 
+def f(x: 'A' = B()) -> None: # E: Incompatible default for argument "x" (default has type "B", argument has type "A")
+    b = x # type: B      # E: Incompatible types in assignment (expression has type "A", variable has type "B")
+    a = x # type: A
 [case testDefaultArgumentExpressionsGeneric]
 from typing import TypeVar
 T = TypeVar('T', bound='A')
-def f(x: T = B()) -> None: # E: Incompatible default for argument "x" (default has type "B", argument has type "T")
-    b = x # type: B      # E: Incompatible types in assignment (expression has type "T", variable has type "B")
-    a = x # type: A
 
 class B: pass
 class A: pass
 
+def f(x: T = B()) -> None: # E: Incompatible default for argument "x" (default has type "B", argument has type "T")
+    b = x # type: B      # E: Incompatible types in assignment (expression has type "T", variable has type "B")
+    a = x # type: A
 [case testDefaultArgumentsWithSubtypes]
 import typing
+class A: pass
+class B(A): pass
+
 def f(x: 'B' = A()) -> None: # E: Incompatible default for argument "x" (default has type "A", argument has type "B")
     pass
 def g(x: 'A' = B()) -> None:
     pass
-
-class A: pass
-class B(A): pass
 [out]
 
 [case testMultipleDefaultArgumentExpressions]
 import typing
+class A: pass
+class B: pass
+
 def f(x: 'A' = B(), y: 'B' = B()) -> None: # E: Incompatible default for argument "x" (default has type "B", argument has type "A")
     pass
 def h(x: 'A' = A(), y: 'B' = B()) -> None:
     pass
-
-class A: pass
-class B: pass
 [out]
 
 [case testMultipleDefaultArgumentExpressions2]
 import typing
-def g(x: 'A' = A(), y: 'B' = A()) -> None: # E: Incompatible default for argument "y" (default has type "A", argument has type "B")
-    pass
-
 class A: pass
 class B: pass
+
+def g(x: 'A' = A(), y: 'B' = A()) -> None: # E: Incompatible default for argument "y" (default has type "A", argument has type "B")
+    pass
 [out]
 
 [case testDefaultArgumentsAndSignatureAsComment]
@@ -2612,7 +2611,7 @@ def f() -> int: ...
 [case testLambdaDefaultTypeErrors]
 lambda a=(1 + 'asdf'): a  # E: Unsupported operand types for + ("int" and "str")
 lambda a=nonsense: a  # E: Name "nonsense" is not defined
-def f(x: int = i):  # E: Name "i" is not defined  # E: Name "i" is used before definition
+def f(x: int = i):  # E: Name "i" is not defined
     i = 42
 
 [case testRevealTypeOfCallExpressionReturningNoneWorks]
diff --git a/test-data/unit/check-possibly-undefined.test b/test-data/unit/check-possibly-undefined.test
index 29c4868e97af..d4f1b6a2b9fd 100644
--- a/test-data/unit/check-possibly-undefined.test
+++ b/test-data/unit/check-possibly-undefined.test
@@ -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:
@@ -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:
@@ -227,6 +227,7 @@ def f0() -> None:
 
 f0()
 x = 1
+
 [case testDefinedInOuterScopeNoError]
 # flags: --enable-error-code possibly-undefined --enable-error-code used-before-def
 def foo() -> None:
@@ -234,6 +235,51 @@ def foo() -> None:
 
 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
+
+glob = c()
+def f3(x: c = glob) -> None:
+    glob = 123
+
+[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:
@@ -829,67 +875,56 @@ def f4() -> None:
         x = z  # E: Name "z" is used before definition
     z: int = 2
 
-[case testUsedBeforeDefImportsBasic]
-# 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
@@ -905,10 +940,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
 


Code review comments:
diff_hunk: @@ -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)
path: mypy/partially_defined.py
text: @user1:
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? 

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