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

Distinguish redundant-expr warnings from unreachable warnings #9125

Merged
merged 7 commits into from
Sep 24, 2020

Conversation

llchan
Copy link
Contributor

@llchan llchan commented Jul 10, 2020

There are redundant expression warnings such as "Left operand of 'or' is always false" and "Left operand of 'and' is always true" that are previously categorized as unreachable. This is a bit of a miscategorization because in these cases all parts of the expression are still evaluated at runtime, compared to situations when the opposite condition is true and the right operand is short-circuited away (which is a stronger hint that something is wrong).

This PR introduces a new redundant-expr code and splits the redudant-but-not-short-circuiting cases out from the unreachable code.

Fixes #8711

@llchan
Copy link
Contributor Author

llchan commented Jul 10, 2020

Note to reviewer: I'm not familiar with the mypyc/dmypy side of things, so if any corresponding changes need to happen there let me know.

@JukkaL
Copy link
Collaborator

JukkaL commented Jul 22, 2020

Thanks for the PR!

Since we already have lots of command line flags, are okay with waiting until #8975 has been implemented (#9172 is the PR) and then making the new error code disabled by default, instead of using a command-line flag? Our longer-term plan is to remove several of the existing command-line flags and to use error codes that can be individually disabled and enabled instead.

@llchan
Copy link
Contributor Author

llchan commented Jul 23, 2020

Sure, can wait on it. I'll subscribe to that PR, but if I overlook the notification feel free to ping me here.

@llchan
Copy link
Contributor Author

llchan commented Aug 12, 2020

@JukkaL I've rebased off master and updated this to use the new --enable-error-code functionality

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 the updates! This is almost ready, just a few remaining comments.

Also, it would be good to document the new error code, as otherwise it won't be easily discoverable.

test-data/unit/check-redundant-expr.test Outdated Show resolved Hide resolved
test-data/unit/check-redundant-expr.test Outdated Show resolved Hide resolved
mypy/errorcodes.py Outdated Show resolved Hide resolved
@llchan
Copy link
Contributor Author

llchan commented Aug 14, 2020

Done

@llchan
Copy link
Contributor Author

llchan commented Sep 22, 2020

@JukkaL + maintainers: anything else you need from me here?

@gvanrossum
Copy link
Member

gvanrossum commented Sep 22, 2020 via email

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

LGTM, two minor nits.

IIRC Jukka asked for documentation; I don’t see any documentation diffs in your PR. How/where are the error codes documented?

mypy/checkexpr.py Outdated Show resolved Hide resolved
# Note that we perform these checks *before* we take into account
# the analysis from the semanal phase below. We assume that nodes
# marked as unreachable during semantic analysis were done so intentionally.
# So, we shouldn't report an error.
Copy link
Member

Choose a reason for hiding this comment

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

Do you understand what this comment refers to? I wonder if the two nearly-identical comment blocks shouldn’t be refactored (the second could just say “similar as above comment”).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know what exactly that comment means. When I split the conditions into two, I erred on the side of "explicit is better than implicit" and made sure the comment was copied to both places, so that a future change doesnt inadvertently lead to a dangling comment reference.

@gvanrossum
Copy link
Member

(OT) Hey @llchan, would you be interested in taking a crack at #9054 ?

@gvanrossum
Copy link
Member

Okay, let's leave that comment as is then. But what about documentation?

@llchan
Copy link
Contributor Author

llchan commented Sep 24, 2020

(OT) Hey @llchan, would you be interested in taking a crack at #9054 ?

Is that related to this PR somehow, or are you asking for general help? To be quite honest I'm not sure I'm familiar enough with the mypy internals to fix that in a time-efficient way (this PR was a little more straightforward since I was taking an existing error code and splitting it into two). I can experiment with it, since I'm sure we will hit that at some point now that we're using 3.8, but it may be better to assign it to an expert.

@llchan
Copy link
Contributor Author

llchan commented Sep 24, 2020

Documentation coming in the next push, working on it now

@gvanrossum
Copy link
Member

(OT) Hey @llchan, would you be interested in taking a crack at #9054 ?

Is that related to this PR somehow, or are you asking for general help? To be quite honest I'm not sure I'm familiar enough with the mypy internals to fix that in a time-efficient way (this PR was a little more straightforward since I was taking an existing error code and splitting it into two). I can experiment with it, since I'm sure we will hit that at some point now that we're using 3.8, but it may be better to assign it to an expert.

It's only related to this PR because the problem is (likely) in the same area of mypy, and I believe you have shown that you know your way around that part. But if you feel that you're not up to it that's totally fine!

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@gvanrossum gvanrossum merged commit f2dfd91 into python:master Sep 24, 2020
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.

Unreachable with always true/false operands to and/or
3 participants