KEMBAR78
Add checks for overlapping TupleTypes by elazarg · Pull Request #4238 · python/mypy · GitHub
Skip to content

Conversation

elazarg
Copy link
Contributor

@elazarg elazarg commented Nov 13, 2017

Fix #4237.

This is not complete or bulletproof, but it's an improvement.

I don't see where should there be a special case for namedtuple; maybe there shouldn't.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this! Looks good, just one question.

mypy/meet.py Outdated
if all(is_overlapping_types(ti, si, use_promotions)
for ti, si in zip(t.items, s.items)):
return True
# TODO: Tuple[A, ...]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this may work already -- Tuple[A, ...] is represented as an Instance, and the new check below should handle this (TupleType vs. Instance). Can you add a test case and remove this comment if I was correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The case was not handled well, so I added support for it

Since unions can overlap with basically everything, checking for them first
gives more confidence in later checks, such as tuple types and type types.
@elazarg
Copy link
Contributor Author

elazarg commented Nov 15, 2017

Added support for Tuple[A, ...].
I also fixed the use of fallback - I believe that comparing fallbacks is incorrect.

@JukkaL the logic is quite different now, so it needs to be re-reviewed... Sorry about that :\

There are many tests that can be added - involving combinations of tuples, namedtuple, varargs, instances and unions; but I prefer to do it in at a different time.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants