Skip to content

Isinstance for if expressions - #896

Merged
JukkaL merged 2 commits into
python:masterfrom
jhance:if-expr-isinstance
Oct 10, 2015
Merged

JukkaL merged 2 commits into
python:masterfrom
jhance:if-expr-isinstance

Conversation

@jhance

@jhance jhance commented Oct 9, 2015

Copy link
Copy Markdown
Collaborator

jhance added 2 commits October 9, 2015 00:36
If we have an expression of the form "not instance"
then we can recurse and just switch the else and if
expressions. We might as well take this opportunity.
We simply check for isinstance when visiting conditionals
and correctly push for the if/else expressions, giving them new types
only for those particular expressions. After that, the type of the
variable being modified returns to its original type.
@o11c

o11c commented Oct 9, 2015

Copy link
Copy Markdown
Contributor

What about isinstance(obj, A) and isinstance(obj2, B) ?

@jhance

jhance commented Oct 9, 2015

Copy link
Copy Markdown
Collaborator Author

See comment in find_isinstance_check in checker.py). Its not supported by if-statements as far as I'm aware, so I'm not supporting it for if-expressions. I don't mind tackling this, but its a separate issue.

Adding this would require modifying find_isinstance_check to be smarter and also returning a Dict[Node, Tuple[Type, Type]] or so. Its kind of tricky, because as you can imagine isinstance(obj, A) and isinstance(obj, B) should resolve obj to whichever is a subclass of the other (or be impossible).

@JukkaL

JukkaL commented Oct 10, 2015

Copy link
Copy Markdown
Collaborator

Yeah, multiple insinstance checks is a separate issue (I added #900 for it).

@JukkaL

JukkaL commented Oct 10, 2015

Copy link
Copy Markdown
Collaborator

Looks good, thanks for the PR!

JukkaL added a commit that referenced this pull request Oct 10, 2015
@JukkaL
JukkaL merged commit 73fc6b1 into python:master Oct 10, 2015
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.

3 participants