mypy icon indicating copy to clipboard operation
mypy copied to clipboard

Improve the handling of "iteration dependent" errors and notes in finally clauses.

Open tyralla opened this issue 7 months ago • 5 comments

Fixes #19269

This PR refactors the logic implemented in #19118 (which only targeted repeatedly checked loops) and applies it to repeatedly checked finally clauses.

I moved nearly all relevant code to the class LoopErrorWatcher, which now has the more general name IterationErrorWatcher, to avoid code duplication. However, one duplication is left, which concerns error reporting. It would be nice and easy to move this functionality to IterationErrorWatcher, too, but this would result in import cycles, and I am unsure if working with TYPE_CHECKING and postponed importing is acceptable in such cases (both for Mypy and Mypyc).

After the refactoring, it should not be much effort to apply the logic to other cases where code sections are analysed iteratively. However, the only thing that comes to my mind is the repeated checking of functions with arguments that contain constrained type variables. I will check it. If anyone finds a similar case and the solution is as simple as expected, we could add the fix to this PR, of course.

tyralla avatar Jun 10 '25 15:06 tyralla

Oh, constrained type variables are indeed a problem, and I just realised this was very recently reported in #19256.

tyralla avatar Jun 10 '25 15:06 tyralla

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

xarray (https://github.com/pydata/xarray)
+ xarray/backends/api.py:1943: error: Unused "type: ignore" comment  [unused-ignore]

github-actions[bot] avatar Jun 10 '25 16:06 github-actions[bot]

According to mypy primer, this fixes at least one real-code problem.

@A5rocks and @JukkaL: Perhaps reviewing this would not require too much effort for you, as #19118 is still fresh on our minds?

tyralla avatar Jun 10 '25 16:06 tyralla

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

xarray (https://github.com/pydata/xarray)
+ xarray/backends/api.py:1943: error: Unused "type: ignore" comment  [unused-ignore]

github-actions[bot] avatar Jun 10 '25 18:06 github-actions[bot]

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

xarray (https://github.com/pydata/xarray)
+ xarray/backends/api.py:1943: error: Unused "type: ignore" comment  [unused-ignore]

github-actions[bot] avatar Jun 16 '25 18:06 github-actions[bot]

Thank you for the review, @JukkaL! It's great that you've found the existing limitations of my current "revealed type recombination" approach. Besides the simple fix requested for this PR, I played a little and tend to think that storing the original type objects in IterationErrorWatcher.revealed_types might, in fact, be preferable. My initial idea is to define a method like Errors.get_current_iterationerrorwatcher, which could then be used by MessageBuilder.reveal_type to store the original type objects in IterationErrorWatcher.revealed_types instead of constructing a note that is filtered out anyway. Such an approach should be much cleaner and more efficient, I suppose.

I could work on it soon, and it would be good to have this fixed in 1.17 as well. If you think this is not possible, I could also temporarily disable the "revealed type recombination" approach, as the (in my opinion, more relevant) new unreachable functionality does not depend on it.

tyralla avatar Jun 20 '25 07:06 tyralla

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

xarray (https://github.com/pydata/xarray)
+ xarray/backends/api.py:1943: error: Unused "type: ignore" comment  [unused-ignore]

github-actions[bot] avatar Jun 20 '25 07:06 github-actions[bot]