Avoid false unreachable and redundant-expr warnings in loops more robustly and efficiently, and avoid multiple revealed type notes for the same line.
#19118
Conversation
… robustly and efficiently, and avoid multiple `revealed type` notes for the same line. This change is an improvement over 9685171. Besides fixing the regression reported in python#18606 and removing the duplicates reported in python#18511, it should significantly reduce the performance regression reported in python#18991. At least running `Measure-command {python runtests.py self}` on my computer (with removed cache) is 10 % faster.
for more information, see https://pre-commit.ci
This comment has been minimized.
This comment has been minimized.
|
@JukkaL: I thought that later responses to Alternatively, I could just remove the |
This comment has been minimized.
This comment has been minimized.
|
I implemented a collection mechanism for the types revealed in different iteration steps. In the case of unions, the items are sorted alphabetically without differentiating upper and lower case letters. At first glance, the reason for the Mypy primer diff (https://github.com/pydata/xarray) is not clear to me. I asked for a review of those who opened the mentioned issues. Thanks in advance! |
|
xarray is #19121, we have it on all current PRs |
|
@sterliakov: Thanks, good to know. (Anybody already working on it?) |
|
@tyralla I'm a bit swamped, there are no related PRs open - so probably not yet? It is likely related to #18976, but I have no idea why it's inconsistent - any MRE extracted from that fails every time, not randomly. https://mypy-play.net/?mypy=master&python=3.12&flags=strict&gist=50f4ae811a650f9e90c6f52269f59d29 |
mypy/checker.py
Outdated
| # We collect errors that indicate unreachability or redundant expressions | ||
| # in the first iteration and remove them in subsequent iterations if the | ||
| # related statement becomes reachable or non-redundant due to changes in | ||
| # partial types: |
There was a problem hiding this comment.
What if an unreachable statement is within another unreachable statement?
There was a problem hiding this comment.
I am not exactly sure what you are asking for.
Is this an example that fits your question?
[case testAvoidUnreachableCodeInUnreachableCodeWithPartialType]
# flags: --warn-unreachable --python-version 3.11
x = None
y = None
while x is None and y is None:
reveal_type(x) # N: Revealed type is "None"
reveal_type(y) # N: Revealed type is "None"
if x is None and y is not None:
x = 1 # E: Statement is unreachable
if y is None:
y = 1
[builtins fixtures/bool.pyi]In this case, the results are the same for 1.15 and the current state of this PR.
There was a problem hiding this comment.
something like this:
x = None
y = None
while True:
if x is not None:
print("a")
if y is not None:
print("b") # this is unreachable
x = 1(I haven't run the PR on this so I'm not sure the result, but it should warn about unreachablity. mypy 1.15 does. I'm worried this change won't)
There was a problem hiding this comment.
This example should print b. Is there a typo?
There was a problem hiding this comment.
I edited it afterwards, it shouldn't anymore.
There was a problem hiding this comment.
Yes, you were totally right. The large temporarily unreachable code section shadowed the persistent unreachable code section. I added your test case and extended the current logic a bit. It's late now, so I'll postpone double-checking until tomorrow.
Can you see any more potential pitfalls?
There was a problem hiding this comment.
I again looked at it. To me, the logic appears a little special, but clear and straightforward. Of course, I cannot exclude that there are more special cases my solution does not cover. So I wait for the next review.
What kind of test case could convince you? |
|
Also BTW I notice your "restart CI" commit -- you could use |
I didn't know. That's helpful, thanks! |
…UnreachableLines` provided by @A5rocks and extend the current logic to fix it.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
| for candidate in set(itertools.chain(*uselessness_errors)): | ||
| if all( | ||
| (candidate in errors) or (candidate[2] in lines) | ||
| for errors, lines in zip(uselessness_errors, unreachable_lines) |
There was a problem hiding this comment.
Why can't this just use the last round of unreachability warnings?
Fixes #18606
Closes #18511
Improves #18991
This change is an improvement over 9685171. Besides fixing the regression reported in #18606 and removing the duplicates reported in #18511, it should significantly reduce the performance regression reported in #18991. At least running
Measure-command {python runtests.py self}on my computer (with removed cache) is 10 % faster.