Avoid false `unreachable` and `redundant-expr` warnings in loops. #18433

tyralla · 2025-01-09T23:23:46Z

Fixes #18348
Fixes #13973
Fixes #11612
Fixes #8721
Fixes #8865
Fixes #7204

I manually checked all the listed issues. Some of them were already partly fixed by #18180.

Fixes python#18348 Fixes python#13973 Fixes python#11612 Fixes python#8721 Fixes python#8865 Fixes python#7204 I manually checked all the listed issues. Some of them were already partly fixed by python#18180.

tyralla · 2025-01-09T23:50:03Z

Such a long primer diff. It seems like I enabled redundant-expr by accident or something like that. I will check it tomorrow.

mypy/checker.py

tyralla · 2025-01-10T06:41:09Z

Regarding pypa/bandersnatch and home-assistant/core, the diff looks good. Regarding pyodide/pyodide, I neither understand why this error is emitted now nor why it is emitted in general (confusing error message). Maybe you know this behaviour already. I could investigate later.

tyralla · 2025-01-10T17:27:04Z

I can reproduce the pyodide problem with Mypy 1.14.1 if I repeat the isinstance check by writing it twice (instead of writing it in a loop), so it is not caused by the change proposed by this PR:

class A: ...
class B: ...

a: A
if isinstance(a, B):
    c = a
if isinstance(a, B):
    c = a  # E: Incompatible types in assignment (expression has type "__main__.<subclass of "A" and "B">1", variable has type "__main__.<subclass of "A" and "B">")  [assignment]

sterliakov · 2025-01-10T17:56:01Z

It's sort-of caused by the change here: at the final iteration (when partials don't change) you accept the body once again. This breaks any such inference propagated outside from isinstance (used to infer type of something external to the loop).

I don't believe this is a big deal though (various issues arise with building dicts/lists/... in a non-trivial loop, it's often necessary to annotate the collection anyway).

tyralla · 2025-01-11T10:27:12Z

I do not entirely understand what you are trying to say. However, the same problem can already occur with earlier Mypy versions (that do not check for the number of partials like the master branch Mypy does now) when writing a loop like this:

class A: ...
class B: ...

a: A
cs = []
x: int | str
x = 1
for i in range(2):
    if isinstance(a, B):
        cs.append(a)  # E: Incompatible types in assignment (expression has type "__main__.<subclass of "A" and "B">1", variable has type "__main__.<subclass of "A" and "B">")  [assignment]
    x = "x"

Here, update_from_options causes that the loop body is accepted twice.

So, in my opinion, this issue does not need to bother us in this PR. However, as related false positives would likely be reported more often (in case this PR gets accepted) and as I am curious about the underlying cause, I could try to fix it in a separate PR.

ilevkivskyi

LG. Regarding the isinstance() regression: why did we start indexing the ad-hoc intersections in the first place? IMO they all should be mutually compatible.

ilevkivskyi · 2025-01-11T21:34:07Z

mypy/checker.py

            while True:
                with self.binder.frame_context(can_skip=True, break_frame=2, continue_frame=1):
                    self.accept(body)
                partials_new = sum(len(pts.map) for pts in self.partial_types)
                if (partials_new == partials_old) and not self.binder.last_pop_changed:
                    break
                partials_old = partials_new
+
+            # If necessary, reset the modified options and make up for the postponed error checks:
+            if warn_unreachable or warn_redundant:


IMO this if and the two ifs above are only making the logic more obscure. It should be simply: store, set, restore.

I removed the two ifs above (one of them by using set.discard instead of set.remove). However, removing the third if would mean unnecessary calls of self.accept(body) in cases where neither unreachable nor redundant-expr are enabled. But I adjusted the latter according to @hauntsaninja's suggestion, which might look a little clearer.

ilevkivskyi · 2025-01-11T21:47:02Z

I couldn't track when we started distinguishing ad-hoc intersections, it looks like it is there since long time ago. Anyway, IMO instead of using gen_unique_name() we can simply pull an existing TypeInfo if it is in the current symbol table. Maybe I am missing something, but IIUC these fake TypeInfos should behave as close to the "real" intersections as possible.

tyralla · 2025-01-12T21:42:05Z

@ilevkivskyi: Thank you for investigating. If you can also see no good reason for this behaviour, I will remove this kind of intersection distinguishing in another PR to see if our test suite and the Mypy primer do not complain.

github-actions · 2025-01-12T21:50:24Z

Diff from mypy_primer, showing the effect of this PR on open source code:

bandersnatch (https://github.com/pypa/bandersnatch)
+ src/bandersnatch_storage_plugins/s3.py:62: error: Unused "type: ignore" comment  [unused-ignore]

core (https://github.com/home-assistant/core)
+ homeassistant/components/recorder/migration.py:2280: error: Unused "type: ignore" comment  [unused-ignore]
+ homeassistant/components/recorder/migration.py:2302: error: Unused "type: ignore" comment  [unused-ignore]
+ homeassistant/components/schedule/__init__.py:76: error: Unused "type: ignore" comment  [unused-ignore]
+ homeassistant/components/twentemilieu/calendar.py:73: error: Unused "type: ignore" comment  [unused-ignore]

schemathesis (https://github.com/schemathesis/schemathesis)
-   File "/tmp/mypy_primer/old_mypy/venv/bin/mypy", line 8, in <module>
+   File "/tmp/mypy_primer/new_mypy/venv/bin/mypy", line 8, in <module>
-   File "/tmp/mypy_primer/old_mypy/venv/lib/python3.12/site-packages/mypy/__main__.py", line 15, in console_entry
+   File "/tmp/mypy_primer/new_mypy/venv/lib/python3.12/site-packages/mypy/__main__.py", line 15, in console_entry
-   File "/tmp/mypy_primer/old_mypy/venv/lib/python3.12/site-packages/mypy/main.py", line 119, in main
+   File "/tmp/mypy_primer/new_mypy/venv/lib/python3.12/site-packages/mypy/main.py", line 119, in main
-   File "/tmp/mypy_primer/old_mypy/venv/lib/python3.12/site-packages/mypy/main.py", line 203, in run_build
+   File "/tmp/mypy_primer/new_mypy/venv/lib/python3.12/site-packages/mypy/main.py", line 203, in run_build
-   File "/tmp/mypy_primer/old_mypy/venv/lib/python3.12/site-packages/mypy/build.py", line 191, in build
+   File "/tmp/mypy_primer/new_mypy/venv/lib/python3.12/site-packages/mypy/build.py", line 191, in build
-   File "/tmp/mypy_primer/old_mypy/venv/lib/python3.12/site-packages/mypy/build.py", line 267, in _build
+   File "/tmp/mypy_primer/new_mypy/venv/lib/python3.12/site-packages/mypy/build.py", line 267, in _build
-   File "/tmp/mypy_primer/old_mypy/venv/lib/python3.12/site-packages/mypy/build.py", line 2947, in dispatch
+   File "/tmp/mypy_primer/new_mypy/venv/lib/python3.12/site-packages/mypy/build.py", line 2947, in dispatch
-   File "/tmp/mypy_primer/old_mypy/venv/lib/python3.12/site-packages/mypy/build.py", line 976, in write_deps_cache
+   File "/tmp/mypy_primer/new_mypy/venv/lib/python3.12/site-packages/mypy/build.py", line 976, in write_deps_cache

pyodide (https://github.com/pyodide/pyodide)
+ src/py/pyodide/_state.py:21: error: Incompatible types in assignment (expression has type "pyodide._state.<subclass of "ModuleType" and "JsProxy">2", target has type "pyodide._state.<subclass of "ModuleType" and "JsProxy">")  [assignment]

tyralla · 2025-01-12T22:02:14Z

Seems like the refactoring did not break anything, so this should be ready for merging.

hauntsaninja · 2025-01-12T22:24:00Z

Hmm, unrelated to this PR, but schemathesis stacktrace is interesting AssertionError: Module must be either parsed or cached. Seems like it comes from 3999ab88 in schemathesis (note mypy_primer uses --cache-dir=/dev/null). Opened #18454

hauntsaninja · 2025-01-12T22:24:22Z

Thank you for this PR!

ilevkivskyi · 2025-01-13T22:33:11Z

If you can also see no good reason for this behaviour, I will remove this kind of intersection distinguishing in another PR to see if our test suite and the Mypy primer do not complain.

Yes, please go ahead.

Avoid false unreachable and redundant-expr warnings in loops.

53d894a

Fixes python#18348 Fixes python#13973 Fixes python#11612 Fixes python#8721 Fixes python#8865 Fixes python#7204 I manually checked all the listed issues. Some of them were already partly fixed by python#18180.

This comment has been minimized.

Sign in to view

Do not enable REDUNDANT_EXPR accidentally.

9b54ab1

hauntsaninja reviewed Jan 10, 2025

View reviewed changes

mypy/checker.py Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

tyralla requested a review from hauntsaninja January 10, 2025 17:27

ilevkivskyi approved these changes Jan 11, 2025

View reviewed changes

refactor

f0fac6d

hauntsaninja merged commit 9685171 into python:master Jan 12, 2025
18 checks passed

hauntsaninja mentioned this pull request Jan 13, 2025

"if condition is always false [redundant-expr]" false positive when variable set in a loop after the conditional #14993

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid false `unreachable` and `redundant-expr` warnings in loops. #18433

Avoid false `unreachable` and `redundant-expr` warnings in loops. #18433

tyralla commented Jan 9, 2025

This comment has been minimized.

tyralla commented Jan 9, 2025

This comment has been minimized.

tyralla commented Jan 10, 2025

tyralla commented Jan 10, 2025 •

edited

Loading

sterliakov commented Jan 10, 2025

tyralla commented Jan 11, 2025

ilevkivskyi left a comment

ilevkivskyi Jan 11, 2025

tyralla Jan 12, 2025

ilevkivskyi commented Jan 11, 2025

tyralla commented Jan 12, 2025

github-actions bot commented Jan 12, 2025

tyralla commented Jan 12, 2025

hauntsaninja commented Jan 12, 2025 •

edited

Loading

hauntsaninja commented Jan 12, 2025

ilevkivskyi commented Jan 13, 2025

Avoid false unreachable and redundant-expr warnings in loops. #18433

Avoid false unreachable and redundant-expr warnings in loops. #18433

Conversation

tyralla commented Jan 9, 2025

This comment has been minimized.

tyralla commented Jan 9, 2025

This comment has been minimized.

tyralla commented Jan 10, 2025

tyralla commented Jan 10, 2025 • edited Loading

sterliakov commented Jan 10, 2025

tyralla commented Jan 11, 2025

ilevkivskyi left a comment

Choose a reason for hiding this comment

ilevkivskyi Jan 11, 2025

Choose a reason for hiding this comment

tyralla Jan 12, 2025

Choose a reason for hiding this comment

ilevkivskyi commented Jan 11, 2025

tyralla commented Jan 12, 2025

github-actions bot commented Jan 12, 2025

tyralla commented Jan 12, 2025

hauntsaninja commented Jan 12, 2025 • edited Loading

hauntsaninja commented Jan 12, 2025

ilevkivskyi commented Jan 13, 2025

Avoid false `unreachable` and `redundant-expr` warnings in loops. #18433

Avoid false `unreachable` and `redundant-expr` warnings in loops. #18433

tyralla commented Jan 10, 2025 •

edited

Loading

hauntsaninja commented Jan 12, 2025 •

edited

Loading