Avoid false `unreachable` and `redundant-expr` warnings in loops more robustly and efficiently, and avoid multiple `revealed type` notes for the same line. #19118

tyralla · 2025-05-20T05:18:20Z

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.

… 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

tyralla · 2025-05-20T06:58:16Z

@JukkaL: I thought that later responses to reveal_type are always better than earlier ones. However, this is not the case for testNewRedefineTryStatement test case. Maybe the most general approach would be to collect all the union items of the individual responses and to combine them in the final note. I could try this, if you think it's a good way to go.

Alternatively, I could just remove the reveal_type part of this pull request, as it seems less important than the other two addressed issues.

This reverts commit b6930e7.

tyralla · 2025-05-22T16:05:49Z

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!

sterliakov · 2025-05-22T22:16:19Z

xarray is #19121, we have it on all current PRs

tyralla · 2025-05-23T05:33:09Z

@sterliakov: Thanks, good to know. (Anybody already working on it?)

sterliakov · 2025-05-23T13:01:12Z

@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

A5rocks

I'm not immediately convinced this is robust!

A5rocks · 2025-05-23T16:01:26Z

mypy/checker.py

+                    # 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:


What if an unreachable statement is within another unreachable statement?

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.

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)

This example should print b. Is there a typo?

I edited it afterwards, it shouldn't anymore.

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?

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.

tyralla · 2025-05-23T19:00:14Z

I'm not immediately convinced this is robust!

What kind of test case could convince you?

A5rocks · 2025-05-23T20:39:51Z

Also BTW I notice your "restart CI" commit -- you could use git commit --allow-empty to avoid having to push a dummy change + a revert. (just a recommendation in case you didn't know)

tyralla · 2025-05-23T20:45:20Z

Also BTW I notice your "restart CI" commit -- you could use git commit --allow-empty to avoid having to push a dummy change + a revert. (just a recommendation in case you didn't know)

I didn't know. That's helpful, thanks!

@A5rocks

…UnreachableLines` provided by @A5rocks and extend the current logic to fix it.

github-actions · 2025-05-24T00:59:44Z

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

A5rocks

sorry, I think I misunderstood things.

A5rocks · 2025-05-25T07:01:46Z

mypy/checker.py

+            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)


Why can't this just use the last round of unreachability warnings?

That is a good question. I initially implemented it so in 229e4a6. However, in the last function definition of test case testNewRedefineTryStatement, the first response to reveal_type does provide complete information, but the second (and final) response does not:

def f4() -> None: while int(): try: x = 1 if int(): x = "" break if int(): while int(): if int(): x = None break finally: reveal_type(x) # N: Revealed type is "Union[builtins.int, builtins.str, None]" \ # N: Revealed type is "Union[builtins.int, None]"

I have no experience with the new --allow-redefinition-new option so far, and have not tried to investigate why this happens. I just extended my approach in c24c950.

Wouldn't that case also allow something not allowed for str? Eg does that error for:

if x is not None: print(x + 5)

Above the reveal_type

Oh, after re-reading, it seems that I have not hit the point of your question. Another try: There might be some scope for optimisations. However, I am unsure what can possibly happen when running Mypy with different configurations (like in the f4 example discussed above), so I preferred to implement this part of the approach as robustly as possible.

On the other hand it's possible to be too robust: mypy only prints the errors for the last round right? So it's possible (through some bug elsewhere, like forgetting to propagate line numbers maybe?) to have unreachable code in the last run that isn't marked -- and so have no signal that the code there isn't checked.

On the other other hand, just using the last iteration's unreachability errors might be wrong in presence of bugs like in f4, but it's simpler (and implementation is trivial to get right) and could never mislead the user about what code could get errors.

Basically I think it's better to match the semantics of how error reporting happens (ie during the last round of checking) then to have only one type of error be correct in face of bugs.

Edit: actually I might be wrong about my assumptions here, let me play with your PR a bit and I'll update this.

well there seems to be a bug in your logic anyways (I can't see where, it looks right to me):

def f4() -> None: while int(): try: x = 1 if int(): x = "" break if int(): while int(): if int(): x = None break finally: if isinstance(x, str): print("hello :)") # E: Statement is unreachable reveal_type(x) # N: Revealed type is "Union[builtins.int, builtins.str, None]"

Anyways, turns out I was wrong about mypy only erroring in the final cycle. But the above bug should be enough for now and IMO a type becoming a subtype at the same place in the code seems like a bug.

@JukkaL since maybe some of my other assumptions are wrong. Do you think that looping over code multiple times but with subtypes should be considered a bug?

Are you certain? This is what I get:

# flags: --allow-redefinition-new --local-partial-types def f4() -> None: while int(): try: x = 1 if int(): x = "" break if int(): while int(): if int(): x = None break finally: if isinstance(x, str): reveal_type(x) # N: Revealed type is "builtins.str" reveal_type(x) # N: Revealed type is "Union[builtins.int, builtins.str, None]"

(I had to add isinstance to exception.pyi to get this running.)

I just added that to a repro.py file and ran mypy repro.py --allow-redefinition-new --local-partial-types --warn-unreachable but maybe I checked your branch out incorrectly?

My bad; the -warn-unreachable flag was missing in the test case. Now I get both Statement is unreachable and Revealed type is "builtins.str". However, the same happens when I check the modified test with Mypy-Master. Therefore, I don't know if this actually points to a flaw in my current approach. I will investigate tomorrow....

...I had no time for a detailed investigation yet, but I tend to think the problem with the currently discussed test case has nothing to do with this PR, because the response to reveal_type and the unreachable error for the same line happen at the same iteration step.

Maybe the first hint on the cause for this discrepancy: expr.node.type = Union[builtins.int, builtins.str, None] but TypeChecker.lookup_type(expr) = Union[builtins.int, None].

tyralla and others added 2 commits May 20, 2025 07:12

[pre-commit.ci] auto fixes from pre-commit.com hooks

c4bea4e

for more information, see https://pre-commit.ci

This comment has been minimized.

Sign in to view

tyralla requested a review from JukkaL May 20, 2025 06:58

tyralla added 3 commits May 22, 2025 10:02

report all revealed types, not only those of the last iterations step

c24c950

restart CI

b6930e7

Revert "restart CI"

01b6140

This reverts commit b6930e7.

This comment has been minimized.

Sign in to view

tyralla requested review from hauntsaninja and A5rocks May 22, 2025 16:02

A5rocks reviewed May 23, 2025

View reviewed changes

Add the test case `testPersistentUnreachableLinesNestedInInpersistent…

15f5740

…UnreachableLines` provided by @A5rocks and extend the current logic to fix it.

tyralla requested a review from A5rocks May 24, 2025 00:41

A5rocks reviewed May 25, 2025

View reviewed changes

Uh oh!

Avoid false unreachable and redundant-expr warnings in loops more robustly and efficiently, and avoid multiple revealed type notes for the same line. #19118

Are you sure you want to change the base?

Avoid false unreachable and redundant-expr warnings in loops more robustly and efficiently, and avoid multiple revealed type notes for the same line. #19118

Uh oh!

Conversation

tyralla commented May 20, 2025

Uh oh!

This comment has been minimized.

tyralla commented May 20, 2025

Uh oh!

This comment has been minimized.

tyralla commented May 22, 2025

Uh oh!

sterliakov commented May 22, 2025

Uh oh!

tyralla commented May 23, 2025

Uh oh!

sterliakov commented May 23, 2025

Uh oh!

A5rocks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

A5rocks May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyralla commented May 23, 2025

Uh oh!

A5rocks commented May 23, 2025

Uh oh!

tyralla commented May 23, 2025

Uh oh!

github-actions bot commented May 24, 2025

Uh oh!

A5rocks left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

A5rocks May 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Avoid false `unreachable` and `redundant-expr` warnings in loops more robustly and efficiently, and avoid multiple `revealed type` notes for the same line. #19118

Avoid false `unreachable` and `redundant-expr` warnings in loops more robustly and efficiently, and avoid multiple `revealed type` notes for the same line. #19118

A5rocks May 23, 2025 •

edited

Loading

A5rocks left a comment •

edited

Loading

A5rocks May 25, 2025 •

edited

Loading