Fixes to union simplification, isinstance and more (try 2)#3086
Fixes to union simplification, isinstance and more (try 2)#3086ilevkivskyi merged 11 commits intomasterfrom
Conversation
The main change is that unions containing Any are no longer simplified to just Any. Also union simplification now has a deterministic result unlike previously, when result could depend on the order of items in a union (this is true modulo remaining bugs). This required changes in various other places to keep the existing semantics, and resulted in some fixes to existing test cases. I also had to fix some tangentially related minor bugs that were triggered by the other changes. We generally don't have fully constructed TypeInfos so we can't do proper union simplification during semantic analysis. Just implement simple-minded simplification that deals with the cases we care about. Fixes #2978. Fixes #1914.
|
Now this should fix #1850 as well. |
|
@ilevkivskyi Do you have time to review this (at least the delta from #3025)? |
|
@gvanrossum I can do this tomorrow morning. |
ilevkivskyi
left a comment
ilevkivskyi
left a comment
There was a problem hiding this comment.
Sorry for a delay with my review. Here are few minor additional comments/questions.
| # E: Revealed type is 'builtins.list[builtins.float]' | ||
| reveal_type(l) \ | ||
| # E: Revealed type is 'builtins.list[Union[builtins.bool, builtins.int, builtins.float]]' | ||
| [builtins fixtures/list.pyi] |
There was a problem hiding this comment.
It would also add a test in the spirit of #2861, involving an additional type (e.g. str):
from typing import List, Union
a = [''] # type: List[Union[str, List[Union[int, float, bool]]]]| and (not isinstance(declared_type, UnionType) | ||
| or not any(isinstance(item, NoneTyp) for item in declared_type.items))): | ||
| # Assigning an Any value doesn't affect the type to avoid false negatives, unless | ||
| # there is an Any item in a declared union type. |
There was a problem hiding this comment.
Do you mean "unless there is a None item in a declared union type"? (because you are checking for NoneTyp)
There was a problem hiding this comment.
The comment was correct -- updated
| [case testInferOptionalAnyType] | ||
| from typing import Any | ||
| x = None | ||
| a = None # type: Any |
There was a problem hiding this comment.
Is this issue specific to Union[None, Any] or to arbitrary declared optional type? In the latter case I would another test case with int instead of Any in the declared type.
There was a problem hiding this comment.
Added test cases for non-optional union type and a union type without Any items
|
The crash in |
Actually I think I introduced those when I did a careless merge. |
| if bool(): | ||
| x = a | ||
| # TODO: Maybe we should infer Any as the type instead. | ||
| reveal_type(x) # E: Revealed type is 'Any' |
There was a problem hiding this comment.
Do we need a TODO item also here? I think the TODO item in the test above is enough, since we already have Any here. Or am I missing something?
There was a problem hiding this comment.
Nope, that was due to copy-paste
|
I think this is ready now. I am going to merge it when the tests pass. Please stop me if you think it is not ready yet. |
|
This is good to go. |
4 participants
This is an updated version of #3025 with an extra fix for strict-optional mode.