Support selecting TypedDicts from unions #7184
Conversation
There was a problem hiding this comment.
I'm glad that we finally have proper support for isinstance with TypedDict types! I left a few minor comments, mostly about possible ways of making this more general. It's okay to make changes in a follow-up PR if you want to merge this now and there are no doubts about correctness.
mypy/meet.py
Outdated
| if (narrowed.type.fullname() == 'builtins.dict' and | ||
| all(isinstance(t, AnyType) for t in narrowed.args)): | ||
| return declared | ||
| return narrowed |
There was a problem hiding this comment.
Hmm, I'm not sure this is the best thing to do, but it's consistent with what we did previously. One option would be to call recursively and narrow the fallback type instead. It's okay to leave like this for now, but maybe at least add a TODO comment.
There was a problem hiding this comment.
I can try adding the recursive call unless this will cause troubles in internal code bases.
| @@ -54,6 +54,12 @@ def narrow_declared_type(declared: Type, narrowed: Type) -> Type: | |||
| return TypeType.make_normalized(narrow_declared_type(declared.item, narrowed.item)) | |||
| elif isinstance(declared, (Instance, TupleType, TypeType, LiteralType)): | |||
| return meet_types(declared, narrowed) | |||
| elif isinstance(declared, TypedDictType) and isinstance(narrowed, Instance): | |||
| # Special case useful for selecting TypedDicts from unions using isinstance(x, dict). | |||
| if (narrowed.type.fullname() == 'builtins.dict' and | |||
There was a problem hiding this comment.
What about Mapping as the narrowed type? Consider at least adding a TODO comment.
There was a problem hiding this comment.
It looks like ABCs (here and in your other comment below) should be supported by normal mechanisms, but there is an unrelated bug that blocks this. I have a fix, but I want to check it against internal code bases again.
| return True | ||
| if isinstance(item, TypedDictType) and isinstance(supertype, Instance): | ||
| # Special case useful for selecting TypedDicts from unions using isinstance(x, dict). | ||
| if supertype.type.fullname() == 'builtins.dict': |
There was a problem hiding this comment.
What about Mapping (and other ABCs)? Maybe at least add a TODO item.
| supertype = erase_type(supertype) | ||
| if is_proper_subtype(erase_type(item), supertype, ignore_promotions=ignore_promotions): | ||
| return True | ||
| if isinstance(supertype, Instance) and supertype.type.is_protocol: |
There was a problem hiding this comment.
Why do we need the is_protocol check? It wasn't there previously.
There was a problem hiding this comment.
This whole branch was initially added as a crutch to support isinstance() with protocols (see the TODO I moved). The point is that erase_type() only erases type arguments but not attribute types. (Well, the check is still unsafe, this is why we have the issue).
Also for non-protocol classes this branch should be a no-op (unless there is a bug in is_proper_subtype()).
It is a relatively common pattern to narrow down typed dicts from unions with non-dict types using
isinstance(x, dict). Currently mypy infersDict[Any, Any]after such checks which is suboptimal.I propose to special-case this in
narrow_declared_type()andrestrict_subtype_away(). Using this opportunity I factored out special cases from the latter in a separate helper function.Using this opportunity I also fix an old type erasure bug in
isinstance()checks (type should be erased after mapping to supertype, not before).