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

Better handling of compile-time relational operators #3158

Merged
merged 1 commit into from Jun 9, 2017

Conversation

Projects
None yet
3 participants
@kirbyfan64
Contributor

kirbyfan64 commented Apr 12, 2017

Inspired by (but doesn't fix [yet!]) #3156. (A "fix" would to be to add an early return to check_boolean_op if e.right_unreachable is True, but I'm not sure if that change would have other repercussions that I'm not aware of.)

Now stuff like this works better:

if PY2 and sys.platform == 'linux':
    # ...
else:
    # ...

a = PY2 and 's'  # Type is str under Python 2, bool under Python 3.
Show outdated Hide outdated test-data/unit/check-unreachable-code.test
b = PY3 and 's'
c = PY2 or 's'
d = PY3 or 's'
reveal_type(a) # E: Revealed type is 'builtins.bool'

This comment has been minimized.

@gvanrossum

gvanrossum Apr 13, 2017

Member

Shouldn't this be Union[bool, str]? Same for all of these, really.

@gvanrossum

gvanrossum Apr 13, 2017

Member

Shouldn't this be Union[bool, str]? Same for all of these, really.

This comment has been minimized.

@gvanrossum

gvanrossum Apr 13, 2017

Member

Oh wait this is testing the special-casing for PY2 and PY3 as well. I guess it's okay. I may not get to the full review until next week.

@gvanrossum

gvanrossum Apr 13, 2017

Member

Oh wait this is testing the special-casing for PY2 and PY3 as well. I guess it's okay. I may not get to the full review until next week.

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum May 10, 2017

Member

@ilevkivskyi Could you review this?
@kirbyfan64 Can you fix the merge conflict?

Member

gvanrossum commented May 10, 2017

@ilevkivskyi Could you review this?
@kirbyfan64 Can you fix the merge conflict?

@gvanrossum gvanrossum requested a review from ilevkivskyi May 10, 2017

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi May 10, 2017

Collaborator

OK, will review this tomorrow.

Collaborator

ilevkivskyi commented May 10, 2017

OK, will review this tomorrow.

@kirbyfan64

This comment has been minimized.

Show comment
Hide comment
@kirbyfan64

kirbyfan64 May 11, 2017

Contributor

Sorry for the delays; I kinda got hung up on finals...

Contributor

kirbyfan64 commented May 11, 2017

Sorry for the delays; I kinda got hung up on finals...

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum May 11, 2017

Member

NP, hope finals went/are going/will go well!

Member

gvanrossum commented May 11, 2017

NP, hope finals went/are going/will go well!

@ilevkivskyi

Mostly looks good to me. Just have few ideas for additional tests.

Show outdated Hide outdated test-data/unit/check-unreachable-code.test
PY2 = f()
PY3 = f()
a = PY2 and 's'

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 12, 2017

Collaborator

I would add few more tests exchanging the order of 's' and PY2/PY3. So that e.g. 's' and False == False.

@ilevkivskyi

ilevkivskyi May 12, 2017

Collaborator

I would add few more tests exchanging the order of 's' and PY2/PY3. So that e.g. 's' and False == False.

This comment has been minimized.

@kirbyfan64

kirbyfan64 Jun 9, 2017

Contributor

@ilevkivskyi That doesn't work, though, since infer_condition_value doesn't take constants into account...

@kirbyfan64

kirbyfan64 Jun 9, 2017

Contributor

@ilevkivskyi That doesn't work, though, since infer_condition_value doesn't take constants into account...

Show outdated Hide outdated test-data/unit/check-unreachable-code.test
@@ -41,6 +41,7 @@ else:
y = 'x' / 1 # E: Unsupported left operand type for / ("str")
y

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 12, 2017

Collaborator

This empty line is unnecessary.

@ilevkivskyi

ilevkivskyi May 12, 2017

Collaborator

This empty line is unnecessary.

x = 'foo'
else:
x = 3
reveal_type(x) # E: Revealed type is 'builtins.str'

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 12, 2017

Collaborator

I would also add few more tests here and for and, so that more combinations of True and False are tested.

@ilevkivskyi

ilevkivskyi May 12, 2017

Collaborator

I would also add few more tests here and for and, so that more combinations of True and False are tested.

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Jun 3, 2017

Collaborator

@kirbyfan64 Are you going to continue working on this PR (here are just few tests missing and a merge conflict needs to be fixed)?

Collaborator

ilevkivskyi commented Jun 3, 2017

@kirbyfan64 Are you going to continue working on this PR (here are just few tests missing and a merge conflict needs to be fixed)?

@kirbyfan64

This comment has been minimized.

Show comment
Hide comment
@kirbyfan64

kirbyfan64 Jun 3, 2017

Contributor

My motherboard died. Somehow, I feel like this has been one of the craziest college years I've ever had...

Contributor

kirbyfan64 commented Jun 3, 2017

My motherboard died. Somehow, I feel like this has been one of the craziest college years I've ever had...

@kirbyfan64

This comment has been minimized.

Show comment
Hide comment
@kirbyfan64

kirbyfan64 Jun 9, 2017

Contributor

@ilevkivskyi Done!

Contributor

kirbyfan64 commented Jun 9, 2017

@ilevkivskyi Done!

@ilevkivskyi

Thank! LGTM now.

@ilevkivskyi ilevkivskyi merged commit de80e76 into python:master Jun 9, 2017

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