Skip to content

Commit

Permalink
Fix crash with generic class definition in function (#13678)
Browse files Browse the repository at this point in the history
Fixes #12112.

The reason why mypy was crashing with a "Must not defer during final
iteration" error in the following snippet:

    from typing import TypeVar

    def test() -> None:
        T = TypeVar('T', bound='Foo')

        class Foo:
            def bar(self, foo: T) -> None:
                pass

...was because mypy did not seem to be updating the types of the `bar`
callable on each pass: the `bind_function_type_variables` method in
`typeanal.py` always returned the _old_ type variables instead of using
the new updated ones we found by calling `self.lookup_qualified(...)`.

This in turn prevented us from making any forward progress when mypy
generated a CallableType containing a placedholder type variable. So, we
repeated the semanal passes until we hit the limit and crashed.

I opted to fix this by having the function always return the newly-bound
TypeVarLikeType instead. (Hopefully this is safe -- the way mypy likes
mutating types always makes it hard to reason about this sort of stuff).

Interestingly, my fix for this bug introduced a regression in one of our
existing tests:

    from typing import NamedTuple, TypeVar

    T = TypeVar("T")
    NT = NamedTuple("NT", [("key", int), ("value", T)])

    # Test thinks the revealed type should be:
# def [T] (key: builtins.int, value: T`-1) -> Tuple[builtins.int, T`-1,
fallback=__main__.NT[T`-1]]
    #
    # ...but we started seeing:
# def [T, _NT <: Tuple[builtins.int, T`-1]] (key: builtins.int, value:
T`-1) -> Tuple[builtins.int, T`-1, fallback=test.WTF[T`-1]]
    reveal_type(NT)

What seems to be happening here is that during the first pass, we add
two type vars to the `tvar_scope` inside `bind_function_type_variables`:
`T` with id -1 and `_NT` with id -2.

But in the second pass, we lose track of the `T` typevar definition
and/or introduce a fresh scope somewhere and infer `_NT` with id -1
instead?

So now mypy thinks there are two type variables associated with this
NamedTuple, which results in the screwed-up type definition.

I wasn't really sure how to fix this, but I thought it was weird that:

1. We were using negative IDs instead of positive ones. (Class typevars
are supposed to use the latter).
2. We weren't wrapping this whole thing in a new tvar scope frame, given
we're nominally synthesizing a new class.

So I did that, and the tests started passing?

I wasn't able to repro this issue for TypedDicts, but opted to introduce
a new tvar scope frame there as well for consistency.
  • Loading branch information
Michael0x2a committed Sep 25, 2022
1 parent a3a5d73 commit 5094460
Show file tree
Hide file tree
Showing 6 changed files with 81 additions and 47 deletions.
80 changes: 42 additions & 38 deletions mypy/semanal.py
Original file line number Diff line number Diff line change
Expand Up @@ -2752,30 +2752,32 @@ def analyze_namedtuple_assign(self, s: AssignmentStmt) -> bool:
return False
lvalue = s.lvalues[0]
name = lvalue.name
internal_name, info, tvar_defs = self.named_tuple_analyzer.check_namedtuple(
s.rvalue, name, self.is_func_scope()
)
if internal_name is None:
return False
if isinstance(lvalue, MemberExpr):
self.fail("NamedTuple type as an attribute is not supported", lvalue)
return False
if internal_name != name:
self.fail(
'First argument to namedtuple() should be "{}", not "{}"'.format(
name, internal_name
),
s.rvalue,
code=codes.NAME_MATCH,
namespace = self.qualified_name(name)
with self.tvar_scope_frame(self.tvar_scope.class_frame(namespace)):
internal_name, info, tvar_defs = self.named_tuple_analyzer.check_namedtuple(
s.rvalue, name, self.is_func_scope()
)
if internal_name is None:
return False
if isinstance(lvalue, MemberExpr):
self.fail("NamedTuple type as an attribute is not supported", lvalue)
return False
if internal_name != name:
self.fail(
'First argument to namedtuple() should be "{}", not "{}"'.format(
name, internal_name
),
s.rvalue,
code=codes.NAME_MATCH,
)
return True
# Yes, it's a valid namedtuple, but defer if it is not ready.
if not info:
self.mark_incomplete(name, lvalue, becomes_typeinfo=True)
else:
self.setup_type_vars(info.defn, tvar_defs)
self.setup_alias_type_vars(info.defn)
return True
# Yes, it's a valid namedtuple, but defer if it is not ready.
if not info:
self.mark_incomplete(name, lvalue, becomes_typeinfo=True)
else:
self.setup_type_vars(info.defn, tvar_defs)
self.setup_alias_type_vars(info.defn)
return True

def analyze_typeddict_assign(self, s: AssignmentStmt) -> bool:
"""Check if s defines a typed dict."""
Expand All @@ -2789,22 +2791,24 @@ def analyze_typeddict_assign(self, s: AssignmentStmt) -> bool:
return False
lvalue = s.lvalues[0]
name = lvalue.name
is_typed_dict, info, tvar_defs = self.typed_dict_analyzer.check_typeddict(
s.rvalue, name, self.is_func_scope()
)
if not is_typed_dict:
return False
if isinstance(lvalue, MemberExpr):
self.fail("TypedDict type as attribute is not supported", lvalue)
return False
# Yes, it's a valid typed dict, but defer if it is not ready.
if not info:
self.mark_incomplete(name, lvalue, becomes_typeinfo=True)
else:
defn = info.defn
self.setup_type_vars(defn, tvar_defs)
self.setup_alias_type_vars(defn)
return True
namespace = self.qualified_name(name)
with self.tvar_scope_frame(self.tvar_scope.class_frame(namespace)):
is_typed_dict, info, tvar_defs = self.typed_dict_analyzer.check_typeddict(
s.rvalue, name, self.is_func_scope()
)
if not is_typed_dict:
return False
if isinstance(lvalue, MemberExpr):
self.fail("TypedDict type as attribute is not supported", lvalue)
return False
# Yes, it's a valid typed dict, but defer if it is not ready.
if not info:
self.mark_incomplete(name, lvalue, becomes_typeinfo=True)
else:
defn = info.defn
self.setup_type_vars(defn, tvar_defs)
self.setup_alias_type_vars(defn)
return True

def analyze_lvalues(self, s: AssignmentStmt) -> None:
# We cannot use s.type, because analyze_simple_literal_type() will set it.
Expand Down
3 changes: 2 additions & 1 deletion mypy/semanal_namedtuple.py
Original file line number Diff line number Diff line change
Expand Up @@ -321,11 +321,12 @@ def parse_namedtuple_args(
) -> None | (tuple[list[str], list[Type], list[Expression], str, list[TypeVarLikeType], bool]):
"""Parse a namedtuple() call into data needed to construct a type.
Returns a 5-tuple:
Returns a 6-tuple:
- List of argument names
- List of argument types
- List of default values
- First argument of namedtuple
- All typevars found in the field definition
- Whether all types are ready.
Return None if the definition didn't typecheck.
Expand Down
12 changes: 6 additions & 6 deletions mypy/typeanal.py
Original file line number Diff line number Diff line change
Expand Up @@ -1331,29 +1331,29 @@ def bind_function_type_variables(
) -> Sequence[TypeVarLikeType]:
"""Find the type variables of the function type and bind them in our tvar_scope"""
if fun_type.variables:
defs = []
for var in fun_type.variables:
var_node = self.lookup_qualified(var.name, defn)
assert var_node, "Binding for function type variable not found within function"
var_expr = var_node.node
assert isinstance(var_expr, TypeVarLikeExpr)
self.tvar_scope.bind_new(var.name, var_expr)
return fun_type.variables
binding = self.tvar_scope.bind_new(var.name, var_expr)
defs.append(binding)
return defs
typevars = self.infer_type_variables(fun_type)
# Do not define a new type variable if already defined in scope.
typevars = [
(name, tvar) for name, tvar in typevars if not self.is_defined_type_var(name, defn)
]
defs: list[TypeVarLikeType] = []
defs = []
for name, tvar in typevars:
if not self.tvar_scope.allow_binding(tvar.fullname):
self.fail(
f'Type variable "{name}" is bound by an outer class',
defn,
code=codes.VALID_TYPE,
)
self.tvar_scope.bind_new(name, tvar)
binding = self.tvar_scope.get_binding(tvar.fullname)
assert binding is not None
binding = self.tvar_scope.bind_new(name, tvar)
defs.append(binding)

return defs
Expand Down
29 changes: 29 additions & 0 deletions test-data/unit/check-classes.test
Original file line number Diff line number Diff line change
Expand Up @@ -1060,6 +1060,35 @@ def f() -> None:
a.g(a) # E: Too many arguments for "g" of "A"
[targets __main__, __main__.f]

[case testGenericClassWithinFunction]
from typing import TypeVar

def test() -> None:
T = TypeVar('T', bound='Foo')
class Foo:
def returns_int(self) -> int:
return 0

def bar(self, foo: T) -> T:
x: T = foo
reveal_type(x) # N: Revealed type is "T`-1"
reveal_type(x.returns_int()) # N: Revealed type is "builtins.int"
return foo
reveal_type(Foo.bar) # N: Revealed type is "def [T <: __main__.Foo@5] (self: __main__.Foo@5, foo: T`-1) -> T`-1"

[case testGenericClassWithInvalidTypevarUseWithinFunction]
from typing import TypeVar

def test() -> None:
T = TypeVar('T', bound='Foo')
class Foo:
invalid: T # E: Type variable "T" is unbound \
# N: (Hint: Use "Generic[T]" or "Protocol[T]" base class to bind "T" inside a class) \
# N: (Hint: Use "T" in function signature to bind "T" inside a function)

def bar(self, foo: T) -> T:
pass

[case testConstructNestedClass]
import typing
class A:
Expand Down
2 changes: 1 addition & 1 deletion test-data/unit/check-namedtuple.test
Original file line number Diff line number Diff line change
Expand Up @@ -1284,7 +1284,7 @@ from typing import NamedTuple, TypeVar

T = TypeVar("T")
NT = NamedTuple("NT", [("key", int), ("value", T)])
reveal_type(NT) # N: Revealed type is "def [T] (key: builtins.int, value: T`-1) -> Tuple[builtins.int, T`-1, fallback=__main__.NT[T`-1]]"
reveal_type(NT) # N: Revealed type is "def [T] (key: builtins.int, value: T`1) -> Tuple[builtins.int, T`1, fallback=__main__.NT[T`1]]"

nts: NT[str]
reveal_type(nts) # N: Revealed type is "Tuple[builtins.int, builtins.str, fallback=__main__.NT[builtins.str]]"
Expand Down
2 changes: 1 addition & 1 deletion test-data/unit/check-typeddict.test
Original file line number Diff line number Diff line change
Expand Up @@ -2550,7 +2550,7 @@ from typing import TypedDict, TypeVar

T = TypeVar("T")
TD = TypedDict("TD", {"key": int, "value": T})
reveal_type(TD) # N: Revealed type is "def [T] (*, key: builtins.int, value: T`-1) -> TypedDict('__main__.TD', {'key': builtins.int, 'value': T`-1})"
reveal_type(TD) # N: Revealed type is "def [T] (*, key: builtins.int, value: T`1) -> TypedDict('__main__.TD', {'key': builtins.int, 'value': T`1})"

tds: TD[str]
reveal_type(tds) # N: Revealed type is "TypedDict('__main__.TD', {'key': builtins.int, 'value': builtins.str})"
Expand Down

0 comments on commit 5094460

Please sign in to comment.