Uh oh!
There was an error while loading. Please reload this page.
Avoid false unreachable and redundant-expr warnings in loops. - #18433
Conversation
Fixespython#18348Fixespython#13973Fixespython#11612Fixespython#8721Fixespython#8865Fixespython#7204 I manually checked all the listed issues. Some of them were already partly fixed by python#18180.
This comment has been minimized.
This comment has been minimized.
tyralla
commented
Jan 9, 2025
Such a long primer diff. It seems like I enabled |
Uh oh!
There was an error while loading. Please reload this page.
This comment has been minimized.
This comment has been minimized.
tyralla
commented
Jan 10, 2025
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. |
I can reproduce the pyodide problem with Mypy 1.14.1 if I repeat the classA: ...
classB: ...
a: Aifisinstance(a, B):
c=aifisinstance(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
commented
Jan 10, 2025
It's sort-of caused by the change here: at the final iteration (when partials don't change) you 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
commented
Jan 11, 2025
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: classA: ...
classB: ...
a: Acs= []
x: int|strx=1foriinrange(2):
ifisinstance(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, 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
left a comment
There was a problem hiding this comment.
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.
| partials_old = partials_new | ||
| # If necessary, reset the modified options and make up for the postponed error checks: | ||
| if warn_unreachable or warn_redundant: |
There was a problem hiding this comment.
IMO this if and the two ifs above are only making the logic more obscure. It should be simply: store, set, restore.
There was a problem hiding this comment.
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
commented
Jan 11, 2025
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 |
tyralla
commented
Jan 12, 2025
@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. |
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
commented
Jan 12, 2025
Seems like the refactoring did not break anything, so this should be ready for merging. |
Hmm, unrelated to this PR, but schemathesis stacktrace is interesting |
hauntsaninja
commented
Jan 12, 2025
Thank you for this PR! |
ilevkivskyi
commented
Jan 13, 2025
Yes, please go ahead. |
…f instances (#18506) While working on #18433, we encountered [this bug.](#18433 (comment)). @ilevkivskyi identified [the underlying problem](#18433 (comment)), and we decided to try to reuse previously created ad hoc intersections of instances instead of always creating new ones. While working on this PR, I realised that reusing intersections requires more complete names. Currently, module and type variable specifications are not included, which could result in mistakes when using these names as identifiers. So, I switched to more complete names. Now, for example, `<subclass of "A" and "A">` becomes `<subclass of "mod1.A[builtins.int]" and "mod2.A">`. Hence, I had to adjust many existing test cases where `reveal_type` is used. `testReuseIntersectionForRepeatedIsinstanceCalls` confirms that the mentioned bug is fixed. `testIsInstanceAdHocIntersectionIncrementalNestedClass` and `testIsInstanceAdHocIntersectionIncrementalUnions` are in a separate commit. I think they are not really necessary, so we might prefer to remove them. I added them originally because I had to adjust `lookup_fully_qualified` a little. The change is very simple, but I could not create a test case where it is not sufficient. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
grihabor
commented
Jan 23, 2025
🎉 Thank you for the fix! |
…f instances (python#18506) While working on python#18433, we encountered [this bug.](python#18433 (comment)). @ilevkivskyi identified [the underlying problem](python#18433 (comment)), and we decided to try to reuse previously created ad hoc intersections of instances instead of always creating new ones. While working on this PR, I realised that reusing intersections requires more complete names. Currently, module and type variable specifications are not included, which could result in mistakes when using these names as identifiers. So, I switched to more complete names. Now, for example, `<subclass of "A" and "A">` becomes `<subclass of "mod1.A[builtins.int]" and "mod2.A">`. Hence, I had to adjust many existing test cases where `reveal_type` is used. `testReuseIntersectionForRepeatedIsinstanceCalls` confirms that the mentioned bug is fixed. `testIsInstanceAdHocIntersectionIncrementalNestedClass` and `testIsInstanceAdHocIntersectionIncrementalUnions` are in a separate commit. I think they are not really necessary, so we might prefer to remove them. I added them originally because I had to adjust `lookup_fully_qualified` a little. The change is very simple, but I could not create a test case where it is not sufficient. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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.