Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix yield from TupleType. #6902

Merged
merged 7 commits into from Jun 2, 2019

Conversation

@brandtbucher
Copy link
Contributor

commented May 26, 2019

Fixes #4444. Now yield from <tuple> works as expected!

We just add a branch to special-case TupleType, since neither they nor their fallbacks are properly handled by any of the other branches.

@ilevkivskyi
Copy link
Collaborator

left a comment

Thanks for working on this! I have some doubts about solution.

@@ -3406,8 +3406,14 @@ def visit_yield_from_expr(self, e: YieldFromExpr, allow_none_return: bool = Fals

# Check that the expr is an instance of Iterable and get the type of the iterator produced
# by __iter__.
if isinstance(subexpr_type, AnyType):
iter_type = AnyType(TypeOfAny.from_another_any, source_any=subexpr_type) # type: Type
if isinstance(subexpr_type, TupleType):

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi May 26, 2019

Collaborator

Adding this special case looks wrong. I think instead type_is_iterable() should be fixed (plus any potential fallout).

@brandtbucher

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

Thanks for the feedback @ilevkivskyi! I agree with your approach; the new solution is simpler, and it properly errors when trying to assign the return value of a tuple iterator.

Tests are passing, ready for another review!

@ilevkivskyi
Copy link
Collaborator

left a comment

This is better, but there are still questions.

@@ -2444,7 +2444,7 @@ def type_is_iterable(self, type: Type) -> bool:
type = type.fallback
return (is_subtype(type, self.named_generic_type('typing.Iterable',
[AnyType(TypeOfAny.special_form)])) and
isinstance(type, Instance))
isinstance(type, (Instance, TupleType)))

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi May 27, 2019

Collaborator

But why this is needed at all? What happens if you remove this and ...? This deserves at least a TODO (or better actually fixing it).

This comment has been minimized.

Copy link
@brandtbucher

brandtbucher May 28, 2019

Author Contributor

It looks like this Instance check was needed at the only other call site, in check_multi_assignment_from_iterable (right below). I moved this check out of the function call, since it seems to be a separate concern.

@@ -3411,6 +3411,10 @@ def visit_yield_from_expr(self, e: YieldFromExpr, allow_none_return: bool = Fals
elif self.chk.type_is_iterable(subexpr_type):
if is_async_def(subexpr_type) and not has_coroutine_decorator(return_type):
self.chk.msg.yield_from_invalid_operand_type(subexpr_type, e)
elif isinstance(subexpr_type, TupleType):
# Tuples get special handling here, since they "don't have __iter__"...

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi May 27, 2019

Collaborator

This is not true, tuples have an __iter__ (on the fallback).

This comment has been minimized.

Copy link
@brandtbucher

brandtbucher May 28, 2019

Author Contributor

Do they? I haven't been able to find it:

>>> subexpr_type
Tuple[]
>>> tuple_fallback(subexpr_type)
builtins.tuple[<nothing>]
>>> tuple_fallback(subexpr_type).type
<TypeInfo builtins.tuple>
>>> tuple_fallback(subexpr_type).type.mro
[<TypeInfo builtins.tuple>, <TypeInfo builtins.object>]
>>> tuple_fallback(subexpr_type).type.mro[0].names
{}
>>> tuple_fallback(subexpr_type).type.mro[1].names
{'__init__': <mypy.nodes.SymbolTableNode object at 0x1137ce5f8>}

The same is true of both full and empty tuples.

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi May 28, 2019

Collaborator

Do they?

Yes, they do. Please use a reasonable/relevant test fixture instead of the default one, e.g. fixtures/tuple.pyi.

This comment has been minimized.

Copy link
@brandtbucher

brandtbucher May 28, 2019

Author Contributor

Thanks @ilevkivskyi! I hadn’t realized that was why it was behaving so strangely. It looks like initial attempts to diagnose it in the issue made the same mistake, which was what led me down this path.

You’re right that this branching is no longer needed then. Fixing type_is_iterable solves the issue!

elif isinstance(subexpr_type, TupleType):
# Tuples get special handling here, since they "don't have __iter__"...
item_type = join.join_type_list(subexpr_type.items)
subexpr_type = self.chk.named_generic_type('typing.Iterable', [item_type])

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi May 27, 2019

Collaborator

Again, why this is needed? Why check_method_call_by_name() doesn't work?

This comment has been minimized.

Copy link
@brandtbucher

brandtbucher May 28, 2019

Author Contributor

The check_method_call_by_name call returns Any, and creates the error message:

error: "Tuple[]" has no attribute "__iter__" (not iterable).

The same is true for full tuples:

error: "Tuple[int, str]" has no attribute "__iter__" (not iterable)

This same message is printed for both TupleType objects and their fallbacks. See my comment above on this.

@ilevkivskyi
Copy link
Collaborator

left a comment

OK, thanks for updates, LGTM now!


def check_multi_assignment_from_iterable(self, lvalues: List[Lvalue], rvalue_type: Type,
context: Context,
infer_lvalue_type: bool = True) -> None:
if self.type_is_iterable(rvalue_type):
item_type = self.iterable_item_type(cast(Instance, rvalue_type))
if self.type_is_iterable(rvalue_type) and isinstance(rvalue_type, Instance):

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Jun 2, 2019

Collaborator

Looking more at this it seems like this still prohibits something like unpacking iterable class objects (like with enums, x, *y = Color). But this was also the case before and is beyond the scope of the original issue.

@ilevkivskyi ilevkivskyi merged commit ae99cad into python:master Jun 2, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@brandtbucher brandtbucher deleted the brandtbucher:yield-from-tuple branch Jun 2, 2019

PattenR added a commit to PattenR/mypy that referenced this pull request Jun 23, 2019

Fix yield from TupleType. (python#6902)
Fixes python#4444 by removing special-casing for instance types.
Now `yield from <tuple>` works as expected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.