Try empty context when assigning to union typed variables #14151
Conversation
This comment has been minimized.
This comment has been minimized.
|
This PR also fixes #13936 🎉 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
OK, I am now happy with |
This comment has been minimized.
This comment has been minimized.
| if ( | ||
| isinstance(get_proper_type(lvalue_type), UnionType) | ||
| # Skip literal types, as they have special logic (for better errors). | ||
| and not isinstance(get_proper_type(rvalue_type), LiteralType) |
There was a problem hiding this comment.
Did you check the performance impact of this? It seems that it should be easy to make this faster in many situations.
One idea would be to first infer the type without the context (if the context is a union type), and only if the inferred type is not compatible with the lvalue type (or there is another error), we'd try again and use the lvalue type as context.
Another idea would to check if the rhs is a reference to a simple variable/attribute or a call to a non-generic function/method. We should be able to skip the re-inferring of the type in this case.
If the performance impact is negligible, we don't need to bother. Both assignment statements and union types are very common, so there might be some impact. I'm a little worried that if we have many bug fixes which each introduce, say, a 1% slowdown, we could easily regress performance a lot over time.
There was a problem hiding this comment.
Yes, I tried to measure, but it is quite small, so it is hard to measure reliably (<0.5% on mypy self-check). But I agree we can still save some time here. I will probably go with the second idea (skipping "trivial" r.h.s.) since this looks like an easy and safe thing.
|
Diff from mypy_primer, showing the effect of this PR on open source code: operator (https://github.com/canonical/operator)
- ops/testing.py:374: error: Argument 1 has incompatible type "Optional[List[int]]"; expected "MutableSequence[Any]" [arg-type]
- ops/testing.py:375: error: Item "None" of "Optional[List[int]]" has no attribute "__iter__" (not iterable) [union-attr]
discord.py (https://github.com/Rapptz/discord.py)
- discord/ext/commands/bot.py:1300: error: Incompatible types in assignment (expression has type "Union[List[str], str, None]", variable has type "Union[List[str], str]") [assignment]
+ discord/ext/commands/bot.py:1300: error: Incompatible types in assignment (expression has type "Optional[str]", variable has type "Union[List[str], str]") [assignment]
pandas (https://github.com/pandas-dev/pandas)
+ pandas/core/reshape/merge.py:1306: error: Unused "type: ignore" comment
+ pandas/core/reshape/merge.py:1328: error: Unused "type: ignore" comment
|
|
If there are no more comments, I will merge this later today or tomorrow. |
Fixes #4805
Fixes #13936
It is known that mypy can overuse outer type context sometimes (especially when it is a union). This prevents a common use case for narrowing types (see issue and test cases). This is a somewhat major semantic change, but I think it should match better what a user would expect.