Fix crash with forward reference in TypedDict #3560
Conversation
Move join to happen after semantic analysis to avoid crash due to incomplete MRO in TypeInfo. The implementation uses a new semantic analysis 'fixup' phase. Fixes #3319.
| @@ -813,3 +813,29 @@ p = TaggedPoint(type='2d', x=42, y=1337) | |||
| p.get('x', 1 + 'y') # E: Unsupported operand types for + ("int" and "str") | |||
| [builtins fixtures/dict.pyi] | |||
| [typing fixtures/typing-full.pyi] | |||
|
|
|||
|
|
|||
| -- Special cases | |||
There was a problem hiding this comment.
Just have few comments:
- Will this PR also fix Avoid calling join during semantic analysis #3316 and Don't use joins during semantics analysis for typed dict #2489?
- There is a similar situation with
NamedTuple(it just usesfallback = self.named_type('__builtins__.tuple', [AnyType()])now). - Maybe it makes sense to add class syntax to the tests just in case?
There was a problem hiding this comment.
Good comments! My responses:
- This seems to remove the only remaining join during semantic analysis. Updated fixed issues in PR description.
- Added Use join to calculate better fallback for tuples #3575 about using join with tuples. This seems to affect ordinary tuples as well, not just named tuples.
- I'll add a test case using the class syntax.
| @@ -2366,11 +2374,19 @@ def fail_typeddict_arg(self, message: str, | |||
|
|
|||
| def build_typeddict_typeinfo(self, name: str, items: List[str], | |||
| types: List[Type]) -> TypeInfo: | |||
| mapping_value_type = join.join_type_list(types) | |||
| fallback = (self.named_type_or_none('typing.Mapping', | |||
There was a problem hiding this comment.
Just curious whether it would ever make a difference whether this was MutableMapping?
There was a problem hiding this comment.
Probably the most precise fallback would be just Dict, but probably we want to have some flexibility because Mapping is covariant in value type (unlike MutableMapping and Dict that are invariant).
There was a problem hiding this comment.
Hm, now you mention variance, why shouldn't this be invariant?
There was a problem hiding this comment.
My previous comment was just my interpretation of the status quo. As far as I am concerned, I would prefer fallback to be just Dict (this would be most precise and most similar to how tuples and named tuples work).
There was a problem hiding this comment.
The reason why we don't use a mutable type as a fallback is that it would compromise type safety. For example, if the fallback would be Dict[str, (join of value types)], then for {'a': 1, 'b': 'x'} the fallback type would be Dict[str, object]. Through the fallback type we could do things like del d['a'] or d['a'] = [1] which would break safety. Mapping only supports getting values, so it's safe. Also, since Mapping only provides read operations, it can be covariant in the value type.
Making a typed dict compatible with Dict[str, Any] without making this the fallback might be a reasonable thing to do, but it's unclear if that would result in ambiguity or other problems.
There was a problem hiding this comment.
Maybe I am missing something, but TypedDict is special-cased in cheker.check_indexed_assignment, however del d['a'] indeed goes through just checking for __delitem__ and the latter is looked up on the fallback. Maybe we can just special-case this too? TypedDicts at runtime are just dicts and have methods like update that can't be accessed on Mapping. Also TypedDict with total=False can support item deletion.
Anyway, it looks like both Mapping and Dict fallbacks require some special-casing, and it is not something where I have a strong opinion.
There was a problem hiding this comment.
Yeah, we could special case additional methods, but it wouldn't still be safe: a TypedDict is a subtype of the fallback type, and if the fallback type is Dict, we could upcast a TypedDict to the Dicttype and do things that the TypedDict wouldn't support, such as setting items with incompatible value types, or deleting required keys.
mypy/semanal.py
Outdated
| or self.object_type()) | ||
|
|
||
| def fixup() -> None: | ||
| mapping_value_type = join.join_type_list(types) | ||
| fallback.args[1] = mapping_value_type |
There was a problem hiding this comment.
I don't see much value in introducing an extra local variable here.
There was a problem hiding this comment.
I can replace it with a comment.
Move join to happen after semantic analysis to avoid crash due to
incomplete MRO in TypeInfo. The implementation uses a new semantic
analysis 'fixup' phase.
Fixes #3319. Fixes #2489. Fixes #3316.