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 crash when a star expression is used in isinstance #6659

Merged
merged 6 commits into from Apr 12, 2019

Conversation

@onlined
Copy link
Contributor

commented Apr 11, 2019

Fixes #4986.

Fix
@ilevkivskyi
Copy link
Collaborator

left a comment

Thanks! The fix looks good, I just have a suggestion for few extra tests.

@@ -3949,6 +3949,8 @@ def flatten(t: Expression) -> List[Expression]:
"""Flatten a nested sequence of tuples/lists into one list of nodes."""
if isinstance(t, TupleExpr) or isinstance(t, ListExpr):
return [b for a in t.items for b in flatten(a)]
elif isinstance(t, StarExpr):
return flatten(t.expr)

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 11, 2019

Collaborator

Technically this adds some non-trivial functionality. Could you please add tests that check the behavior (i.e. whether types are narrowed or nor) for situations like:

isinstance(x, *(str, int))  # This should fail IIUC
isinstance(x, (int, *(str, list)))  # This should behave the same as (int, str, list)

This comment has been minimized.

Copy link
@onlined

onlined Apr 11, 2019

Author Contributor

I did not understand the difference between the second one and my proposed test.

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 11, 2019

Collaborator

Basically your test only tests that mypy doesn't crash, while I want:

  • check that type narrowing happens when it should, and doesn't happen when it shouldn't
  • check both situations where tuple appears in place and where it is referenced by a variable

This comment has been minimized.

Copy link
@onlined

onlined Apr 11, 2019

Author Contributor

I got it, thanks.

This comment has been minimized.

Copy link
@onlined

onlined Apr 11, 2019

Author Contributor

Done.

onlined added some commits Apr 11, 2019

@ilevkivskyi
Copy link
Collaborator

left a comment

Thanks, there is one more comment about the added tests.

test-data/unit/check-isinstance.test Outdated Show resolved Hide resolved

onlined and others added some commits Apr 12, 2019

@ilevkivskyi
Copy link
Collaborator

left a comment

Thanks, LGTM now.

@ilevkivskyi ilevkivskyi merged commit 9a4cfae into python:master Apr 12, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.