Allow assignments to multiple targets from union types #4067
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…-union Conflicts: mypy/checker.py test-data/unit/check-unions.test
Closed
JukkaL
reviewed
Oct 10, 2017
| b: B | ||
|
|
||
| a: Union[Tuple[int, int], Tuple[int, object]] | ||
| (x[0], b.x) = a |
There was a problem hiding this comment.
Add test similar to this but where there is a type error.
| @@ -1417,3 +1417,33 @@ async def main() -> None: | |||
| [out] | |||
| _testAsyncioGatherPreciseType.py:9: error: Revealed type is 'builtins.str' | |||
| _testAsyncioGatherPreciseType.py:10: error: Revealed type is 'builtins.str' | |||
|
|
|||
| [case testNoCrashOnGenericUnionUnpacking] | |||
There was a problem hiding this comment.
Pythoneval test cases have a big fixed overhead, so I'd suggest combining all of these three test cases into a single test case.
mypy/binder.py
Outdated
| @@ -37,6 +42,10 @@ def __init__(self) -> None: | |||
| self.unreachable = False | |||
|
|
|||
|
|
|||
| if MYPY: | |||
| Assigns = DefaultDict[Expression, List[Tuple[Type, Optional[Type]]]] | |||
There was a problem hiding this comment.
Add comment that explains what this is used for.
mypy/checker.py
Outdated
| item_type = iterable.args[0] | ||
| if not isinstance(item_type, AnyType): | ||
| return item_type | ||
| # Try also structural typing |
There was a problem hiding this comment.
This relies on map_instance_to_supertype returning Iterable[Any] in case there is no explicit base class. Add a note about it here and also update docstring of map_instance_to_supertype to mention it (currently it says that the second argument must be a superclass).
| @@ -1716,7 +1749,7 @@ def split_around_star(self, items: List[T], star_index: int, | |||
| returns in: ([1,2], [3,4,5], [6,7]) | |||
| """ | |||
| nr_right_of_star = length - star_index - 1 | |||
| right_index = nr_right_of_star if -nr_right_of_star != 0 else len(items) | |||
| right_index = -nr_right_of_star if nr_right_of_star != 0 else len(items) | |||
There was a problem hiding this comment.
This bug was exposed by one of the tests. It's quite surprising that it never appeared before, since the old logic took first types after nr_right_of_star, instead of the same number counting from the end of items.
EDIT: fixed wording.
mypy/checker.py
Outdated
| rvalue_type: UnionType, context: Context, | ||
| infer_lvalue_type: bool) -> None: | ||
| transposed = tuple([] for _ in lvalues) # type: Tuple[List[Type], ...] | ||
| with self.binder.accumulate_type_assignments() as assignments: |
There was a problem hiding this comment.
Add comment about why we have this.
mypy/checker.py
Outdated
| infer_lvalue_type: bool) -> None: | ||
| transposed = tuple([] for _ in lvalues) # type: Tuple[List[Type], ...] | ||
| with self.binder.accumulate_type_assignments() as assignments: | ||
| for item in rvalue_type.items: |
There was a problem hiding this comment.
Add short comment describing what we are doing here. For example, something like "Type check the assignment separately for each union item and collect the inferred lvalue types for each union item.".
mypy/checker.py
Outdated
| for t, lv in zip(transposed, lvalues): | ||
| t.append(self.type_map.pop(lv, AnyType(TypeOfAny.special_form))) | ||
| union_types = tuple(UnionType.make_simplified_union(col) for col in transposed) | ||
| for expr, items in assignments.items(): |
There was a problem hiding this comment.
Again, add comment that describes the purpose of this look in one sentence.
mypy/checker.py
Outdated
| UnionType.make_simplified_union(types), | ||
| UnionType.make_simplified_union(declared_types), | ||
| False) | ||
| for union, lv in zip(union_types, lvalues): |
There was a problem hiding this comment.
One more comment for this loop.
| c, *d = x | ||
| reveal_type(c) # E: Revealed type is 'builtins.int' | ||
| reveal_type(d) # E: Revealed type is 'builtins.list[builtins.int*]' | ||
| [builtins fixtures/tuple.pyi] |
There was a problem hiding this comment.
Ideas for additional test cases:
- Multiple lvalues such as
<first> = <second> = <rvalue>. - Multiple assignment in a for loop (
for x, y in ...). - None of the union items are iterable.
- None of the union items have the correct number of items (try with too few and too many).
|
@JukkaL Thanks for review! I think I addressed all comments in new commits. |
JukkaL
reviewed
Oct 11, 2017
mypy/checker.py
Outdated
| @@ -1639,18 +1639,35 @@ def check_multi_assignment(self, lvalues: List[Lvalue], | |||
| def check_multi_assignment_from_union(self, lvalues: List[Expression], rvalue: Expression, | |||
| rvalue_type: UnionType, context: Context, | |||
| infer_lvalue_type: bool) -> None: | |||
| """Check assignment to multiple lavlue targets when rvalue type is a Union[...]. | |||
mypy/binder.py
Outdated
| If this map is not None, actual binding is deferred until all items in | ||
| the union are processed (a union of collected items is later bound | ||
| manually by the caller). | ||
| """ | ||
| self.type_assignments = defaultdict(list) | ||
| yield self.type_assignments | ||
| self.type_assignments = None |
There was a problem hiding this comment.
What about things like (x, (y, z)) = ... where there are two levels of union type multiple assignment? Would we need to restore old mapping perhaps? This would be worth testing as well.
There was a problem hiding this comment.
Yes, you are totally right. Will push a commit in a second. Also testing this uncovered another flaw in my implementation: I need to iterate over nested lvalues in for loops (added a test for this as well).
|
This causes a crash on internal Dropbox code. It happens on a line like this: for error_label, error_type in error_set:I haven't isolated a small repro yet, but here's the traceback: |
|
The type of |
|
The inferred partial from typing import Dict, Tuple, Set, Any
a: Any
d: Dict[str, Tuple[Set[Tuple[str, str]], str]]
x, _ = d.get(a, (None, None))
reveal_type(x)
for y in x: passOutput from mypy: |
|
OK, I will take a look now. |
|
It looks like the simplest solution is to not infer partial types while unpacking unions, anyway the resulting type will be |
JukkaL
approved these changes
Oct 11, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #3859
Fixes #3240
Fixes #1855
Fixes #1575
This is a simple fix of various bugs and a crash based on the idea originally proposed in #2219. The idea is to check assignment for every item in a union. However, in contrast to #2219, I think it will be more expected/consistent to construct a union of the resulting types instead of a join, for example:
Thanks to @elazarg for initial work on this!