From ef4d492659941a02805caed5f984e238e42003ae Mon Sep 17 00:00:00 2001 From: Michael Lee Date: Sun, 25 Sep 2022 17:20:42 -0700 Subject: [PATCH 1/4] Stop TypedDictAnalyzer from leaking synthetic types Fixes #10007 Mypy currently crashes when you try: 1. Creating a class-based TypedDict containing a malformed type hint 2. Asking it to compute fine-grained dependencies, either via running dmypy or by setting the `--cache-fine-grained` flag. Here is the exact sequence of events that leads to this crash: 1. Since the type annotation is malformed, semanal initially gives the type annotation a type of `RawExpressionType`. 2. TypedDictAnalyzer (correctly) determines determines that the type of the malformed type annotation should be treated as just `Any` in: https://github.com/python/mypy/blob/f5ce4ee6ca7e2d8bb1cde8a2b49865f53bbacff5/mypy/semanal_typeddict.py#L289 3. TypedDictAnalyzer forgets to modify `stmt.type` like we normally do after calling `self.anal_type` in normal classes: https://github.com/python/mypy/blob/f5ce4ee6ca7e2d8bb1cde8a2b49865f53bbacff5/mypy/semanal.py#L3022 4. Mypy _does_ use the `Any` type when constructing the TypeInfo for the TypedDict. This is why mypy will not crash under most conditions: the correct type is being used in most places. 5. Setting `--cache-fine-grained` will make mypy perform one final pass against the AST to compute fine-grained dependencies. As a part of this process, it traverses the AssigmentStatement's `type` field using TypeTriggersVisitor. 6. TypeTriggersVisitor is _not_ a SyntheticTypeVisitor. So, the visitor trips an assert when we try traversing into the RawExpressionType. Interestingly, this same crash does not occur for NamedTuples despite the fact that NamedTupleAnalyzer also does not set `stmt.type`: https://github.com/python/mypy/blob/f5ce4ee6ca7e2d8bb1cde8a2b49865f53bbacff5/mypy/semanal_namedtuple.py#L177 It turns out this is because semanal.py will end up calling the `analyze_class_body_common(...)` function after NamedTupleAnalyzer runs, but _not_ after TypedDictAnalyzer runs: - https://github.com/python/mypy/blob/f5ce4ee6ca7e2d8bb1cde8a2b49865f53bbacff5/mypy/semanal.py#L1510 - https://github.com/python/mypy/blob/f5ce4ee6ca7e2d8bb1cde8a2b49865f53bbacff5/mypy/semanal.py#L1479 I'm not sure why this is: ideally, the two analyzers ought to have as similar semantics as possible. But refactoring this felt potentially disruptive, so I went for the narrower route of just patching TypedDictAnalyzer. --- mypy/semanal_typeddict.py | 6 +++++ test-data/unit/check-semanal-error.test | 30 +++++++++++++++++++++++++ test-data/unit/semanal-typeddict.test | 2 +- 3 files changed, 37 insertions(+), 1 deletion(-) diff --git a/mypy/semanal_typeddict.py b/mypy/semanal_typeddict.py index fd6b1bbd2bbf..0c161c9f183d 100644 --- a/mypy/semanal_typeddict.py +++ b/mypy/semanal_typeddict.py @@ -294,6 +294,12 @@ def analyze_typeddict_classdef_fields( ) if analyzed is None: return None, [], set() # Need to defer + # TypedDictAnalyzer sets the AssignmentStmt type here, but + # NamedTupleAnalyzer doesn't and instead has semanal.py set it + # by calling analyze_class_body_common after. + # + # TODO: Resolve this inconsistency? + stmt.type = analyzed types.append(analyzed) # ...despite possible minor failures that allow further analyzis. if stmt.type is None or hasattr(stmt, "new_syntax") and not stmt.new_syntax: diff --git a/test-data/unit/check-semanal-error.test b/test-data/unit/check-semanal-error.test index c6cf45d96691..787a2980ac55 100644 --- a/test-data/unit/check-semanal-error.test +++ b/test-data/unit/check-semanal-error.test @@ -152,3 +152,33 @@ class C: x: P[int] = C() [builtins fixtures/tuple.pyi] [out] + +[case testSemanalDoesNotLeakSyntheticTypes] +# flags: --cache-fine-grained +from typing import Generic, NamedTuple, TypedDict, TypeVar +from dataclasses import dataclass + +T = TypeVar('T') +class Wrap(Generic[T]): pass + +invalid_1: 1 + 2 # E: Invalid type comment or annotation +invalid_2: Wrap[1 + 2] # E: Invalid type comment or annotation + +class A: + invalid_1: 1 + 2 # E: Invalid type comment or annotation + invalid_2: Wrap[1 + 2] # E: Invalid type comment or annotation + +class B(NamedTuple): + invalid_1: 1 + 2 # E: Invalid type comment or annotation + invalid_2: Wrap[1 + 2] # E: Invalid type comment or annotation + +class C(TypedDict): + invalid_1: 1 + 2 # E: Invalid type comment or annotation + invalid_2: Wrap[1 + 2] # E: Invalid type comment or annotation + +@dataclass +class D: + invalid_1: 1 + 2 # E: Invalid type comment or annotation + invalid_2: Wrap[1 + 2] # E: Invalid type comment or annotation +[builtins fixtures/dict.pyi] +[typing fixtures/typing-typeddict.pyi] diff --git a/test-data/unit/semanal-typeddict.test b/test-data/unit/semanal-typeddict.test index b9eb6e0c2b13..9ce89155c308 100644 --- a/test-data/unit/semanal-typeddict.test +++ b/test-data/unit/semanal-typeddict.test @@ -42,4 +42,4 @@ MypyFile:1( NameExpr(x) TempNode:4( Any) - str?))) + builtins.str))) From 1f9a1c025c92394d00b19183981943580375f218 Mon Sep 17 00:00:00 2001 From: Michael Lee Date: Thu, 29 Sep 2022 01:32:39 -0700 Subject: [PATCH 2/4] Rebase and update comment --- mypy/semanal_typeddict.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/mypy/semanal_typeddict.py b/mypy/semanal_typeddict.py index 0c161c9f183d..696ce2da4894 100644 --- a/mypy/semanal_typeddict.py +++ b/mypy/semanal_typeddict.py @@ -298,7 +298,12 @@ def analyze_typeddict_classdef_fields( # NamedTupleAnalyzer doesn't and instead has semanal.py set it # by calling analyze_class_body_common after. # - # TODO: Resolve this inconsistency? + # This is because unlike TypedDicts, NamedTuples support method + # definitions. So, we must handle some of what analyze_class_body_common + # does here -- including modifying `stmt.type`. + # + # TODO: Find some way of refactoring and partially unifying + # these two codepaths? stmt.type = analyzed types.append(analyzed) # ...despite possible minor failures that allow further analyzis. From 6956bf0e154d746bd2b85ef6481b355591b593b9 Mon Sep 17 00:00:00 2001 From: Michael Lee Date: Sun, 23 Oct 2022 17:39:40 -0700 Subject: [PATCH 3/4] Add missing has_placeholder check --- mypy/semanal_typeddict.py | 2 +- test-data/unit/check-typeddict.test | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/mypy/semanal_typeddict.py b/mypy/semanal_typeddict.py index 696ce2da4894..4a0c98509685 100644 --- a/mypy/semanal_typeddict.py +++ b/mypy/semanal_typeddict.py @@ -292,7 +292,7 @@ def analyze_typeddict_classdef_fields( allow_placeholder=not self.options.disable_recursive_aliases and not self.api.is_func_scope(), ) - if analyzed is None: + if analyzed is None or has_placeholder(analyzed): return None, [], set() # Need to defer # TypedDictAnalyzer sets the AssignmentStmt type here, but # NamedTupleAnalyzer doesn't and instead has semanal.py set it diff --git a/test-data/unit/check-typeddict.test b/test-data/unit/check-typeddict.test index 4c68b7b692ff..29821f0d9830 100644 --- a/test-data/unit/check-typeddict.test +++ b/test-data/unit/check-typeddict.test @@ -2176,6 +2176,27 @@ Foo = TypedDict('Foo', {'camelCaseKey': str}) value: Foo = {} # E: Missing key "camelCaseKey" for TypedDict "Foo" [builtins fixtures/dict.pyi] +[case testTypedDictWithDeferredFieldTypeEval] +from typing import Generic, TypeVar, TypedDict, NamedTuple + +class Foo(TypedDict): + # Inner[ForceDeferredEval] will be a placeholder type on first pass; + # confirm we infer the correct type on the second. + x: Outer[Inner[ForceDeferredEval]] + +var: Foo +reveal_type(var) # N: Revealed type is "TypedDict('__main__.Foo', {'x': __main__.Outer[__main__.Inner[__main__.ForceDeferredEval]]})" + +T1 = TypeVar("T1") +class Outer(Generic[T1]): pass + +T2 = TypeVar("T2", bound=ForceDeferredEval) +class Inner(Generic[T2]): pass + +class ForceDeferredEval: pass +[builtins fixtures/dict.pyi] +[typing fixtures/typing-typeddict.pyi] + -- Required[] [case testDoesRecognizeRequiredInTypedDictWithClass] From bcc768cbd92dc777d50ae0b4ec95a56e12b84230 Mon Sep 17 00:00:00 2001 From: Michael Lee Date: Sun, 23 Oct 2022 18:33:33 -0700 Subject: [PATCH 4/4] Try performing has_placeholder check more narrowly --- mypy/semanal_typeddict.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/mypy/semanal_typeddict.py b/mypy/semanal_typeddict.py index 4a0c98509685..287e2cb6c488 100644 --- a/mypy/semanal_typeddict.py +++ b/mypy/semanal_typeddict.py @@ -292,7 +292,7 @@ def analyze_typeddict_classdef_fields( allow_placeholder=not self.options.disable_recursive_aliases and not self.api.is_func_scope(), ) - if analyzed is None or has_placeholder(analyzed): + if analyzed is None: return None, [], set() # Need to defer # TypedDictAnalyzer sets the AssignmentStmt type here, but # NamedTupleAnalyzer doesn't and instead has semanal.py set it @@ -304,7 +304,8 @@ def analyze_typeddict_classdef_fields( # # TODO: Find some way of refactoring and partially unifying # these two codepaths? - stmt.type = analyzed + if not has_placeholder(analyzed): + stmt.type = analyzed types.append(analyzed) # ...despite possible minor failures that allow further analyzis. if stmt.type is None or hasattr(stmt, "new_syntax") and not stmt.new_syntax: